ReviewOS

stacks/bench-review

product: core engagement loop is broken — no-op settings save, orphaned judge-claim funnel, dead follow/notification fan-out, no mobile nav

#57
Closed glennmichael123 opened this 9 days ago ·
9 days ago

Problem

Bench Review's growth loop — visitor reads reviews, signs up, follows judges, gets pulled back when new reviews publish, and judges claim profiles to respond — is broken at nearly every link. For a platform hosting negative reviews of named judges, the right-of-reply path being undiscoverable is a fairness/legal-optics problem, and with zero re-engagement or analytics, every visitor is one-and-done.

Findings

  • Settings profile save is a no-op. saveProfile only rewrites the cookie-cached user and shows "Saved." — comment admits "No /api/me PATCH endpoint yet" (resources/components/Bench/Settings/SettingsView.stx:51-69); auth.ts:305-313 fetchMe clobbers the edit on next load. "Change photo" (SettingsView.stx:232) has no handler; routes/api.ts has no PATCH /me.
  • Judge-claim funnel is orphaned. /judges/signup (JudgeSignup.stx, ClaimJudgeProfileAction) has zero inbound links — header, footer (BenchFooter.stx:17-39), and judge profiles lack any "Claim this profile" CTA — and resources/views/faq.stx:27 says claims are "on the way." The page is also absent from the pre-rendered dist/.
  • No notification on review publish. UpdateReviewStatusAction.ts:72-153 notifies only the author; the claimed judge and judge_follows rows are never fanned out, so Follow is a dead-end (NotificationType is only 'like'|'approved'|'rejected', app/Helpers/notifications.ts:3) and CommentSubmitAction never notifies authors.
  • No mobile navigation. All header nav is hidden lg:flex (BenchHeader.stx:276,283,371); no hamburger exists; MobileMenu.stx is dead code with zero includes.
  • Feed capped at 20, no pagination. LatestReviewsAction.ts:16-17 (MAX_LIMIT=20, no page param) + ReviewsFeed.stx fetchLatest(20); JudgeIndexAction uses Judge.all(); Comments.stx:46 single page of 50.
  • 'Most helpful' sort missing despite full likes infra — ReviewsByJudgeAction.ts:57 and LatestReviewsAction.ts:31 hardcode created_at desc; no sort control in any component.
  • Notification preferences unwired. Framework table + filter layer exist (storage/framework/core/notifications/src/index.ts:204) but no UI, no API route, and all mailers send unconditionally.
  • API failures render as misleading empty states. Stores swallow errors (judges.ts:81-85, reviews.ts:80-94); a down API shows "No reviews yet" with no retry; ArticleView has no branch for 5xx (renders blank).
  • No 500 page in dist/ — only 404.html; the SSG auto-emits 500.html if views/500.stx exists.
  • Zero analytics — no snippet, no events, JudgeSearchAction discards queries (zero-result searches = judge-import backlog).
  • No re-engagement loop — subscribe UI only on BenchComingSoon.stx:51; Scheduler runs only the demo Inspire job; framework's SendCampaignJob is never invoked.

Acceptance criteria

  • PATCH /api/me (auth-gated) persists name; saveProfile calls it; email input and photo button removed or fully implemented (email change resets verification)
  • "Are you Judge {name}? Claim this profile" link on Judge/ProfileHeader.stx, a footer "For Judges" link, and faq.stx points to /judges/signup
  • Publishing a review notifies the verified claimed judge (in-app + email) and fans out in-app notifications to judge_follows; comments notify the review author (skip self)
  • lg:hidden hamburger wires up MobileMenu.stx (fix isOpen binding, dead links, missing Icon refs); mobile users can navigate, search, and log in
  • /reviews feed and Comments support paging via the existing paginate helper with a "Load more" control
  • ?sort=helpful supported on review lists with a UI toggle
  • Email-notification toggle in Settings, enforced before author/judge/follower emails
  • Public stores expose error state; feed/directory/article render "Couldn't load — Retry" instead of empty/blank
  • views/500.stx exists so dist/500.html is emitted
  • Privacy-friendly analytics snippet in the default layout; JudgeSearchAction logs {query, result_count}
  • Newsletter capture on live pages with a scheduled digest send — or the capture is removed

From the 2026-06-10 production-readiness audit.

9 days ago

One of the four items in this issue is now done: mobile nav shipped in 87de153 (#90). The header put hidden lg:flex on all three of its regions and offered no hamburger, so below lg the site header was the logo and nothing else — and since the iOS app is a WebView over this same site, that was the app's navigation too. There is now a trigger and a slide-over with search, the five main links, the signed-in section and Sign up / Log in.

The other three — no-op settings save, orphaned judge-claim funnel, dead follow/notification fan-out — are untouched and all need a live API to verify, which is the gap that makes them hard to close from here.

9 days ago

Re-checked every finding here against current code. Half of this issue is already fixed — recording which, so the rest is actionable.

Fixed since this was written

  • Settings profile save is a no-op — PATCH /api/me exists (routes/api.ts:264 → Actions/Me/UpdateMeAction) and saveProfile calls it, mirroring the server's record into the cached user on success.
  • Judge-claim funnel is orphaned — BenchFooter carries "Are you a judge? Claim your profile" and guidelines.stx links it too. The page builds (dist/judges/signup.html). The FAQ's "on the way" copy is gone.
  • No mobile navigation — shipped in 87de153 (#90). Hamburger below lg plus a slide-over with search, the five main links, the signed-in section and Sign up / Log in. MobileMenu.stx is deleted; it could never have worked as written.
  • Feed capped at 20, no pagination — LatestReviewsAction returns the canonical paginator and respects ?page / ?per_page.
  • No 500 page in dist/ — dist/500.html is emitted.
  • Zero analytics — the build injects the ts-analytics tag into every page.
  • API failures render as misleading empty states — fixed in 11b5f70, see below.

Just fixed: the misleading empty states

This was the worst of the live ones. With the API down, /reviews rendered "No reviews yet. Be the first to write one." and /judges "No judges yet" — false statements about the content, on a content site, precisely when something was broken.

The stores swallowed it: if (!res.ok) return and a console.error-only catch left the list empty and loading false, which is exactly the shape the empty state keys off. Nothing was exposed for a view to tell the two apart.

Both stores now carry a loadError signal and the three public list surfaces gate their empty state on it, rendering a retry instead. Verified both directions against the built artifact: with no API all three show "Could not load…" and no empty state; with a stub API returning an empty 200 all three show their empty state and no error.

Worth noting this got worse before it got better — the stx-standards §7.3 work earlier today made those empty states render correctly and promptly, which made the wrong message appear more reliably.

Still open

  • No notification on review publish. NotificationType is still 'like' | 'approved' | 'rejected' (app/Helpers/notifications.ts:3). Publishing notifies only the author — never the claimed judge, never judge_follows. So Follow does nothing, ever, and the right-of-reply path has no trigger.
  • 'Most helpful' sort missing. Both list actions still hardcode created_at desc despite complete likes infrastructure.
  • No "Claim this profile" CTA on judge profiles. The footer and guidelines link to /judges/signup, but the page a judge actually lands on does not.
  • Notification preferences unwired.
  • No re-engagement loop — subscribe UI still only on the coming-soon page.
  • Search queries discarded — zero-result searches are the judge-import backlog and nothing records them.

Of these, the notification fan-out is the one with consequences beyond engagement: for a site hosting negative reviews of named judges, the subject of a review has no way to learn it was published.

9 days ago

Publish fan-out done in 44c589c — the item I flagged as having consequences beyond engagement.

NotificationType gains judge_review and followed_review, and a transition into published now notifies:

  • the verified claimant of the judge being reviewed — the subject of a published review previously had no way to learn it existed, so the right-of-reply path had no trigger at all
  • everyone following that judge — there is already a judge_follows row in the dev database, so somebody pressed Follow and has received nothing since

Three rules it enforces: the review's author is removed from the recipient set (they already get approved); a claimant who also follows the judge gets judge_review rather than followed_review; and notify() dedups both new types on (recipient, review), so toggling status — a moderation signal the author should see each time — does not tell the judge and followers twice.

Both label surfaces were updated, the header bell and the notifications page. Neither throws on an unknown type; they fall back to a bare "New notification", so a missing case would have shipped a delivered-but-meaningless row rather than an error. None of the new phrasing says "your review", since these go to people who did not write it.

Verified: the three fan-out queries execute through the app's own db layer — judge_follows has no model file so ORM reachability was not a given, and it resolves — and the real notify() helper against a seeded copy inserts both types and suppresses a republish, four calls producing two rows. The live database was not written to.

Not verified: the action's orchestration end to end, which needs the API and an authenticated admin. The recipient selection and dedup are covered; the HTTP path around them is not.

Still open on this issue: 'most helpful' sort, a Claim-this-profile CTA on judge profiles, notification preferences, re-engagement/newsletter, and discarded search queries.

9 days ago

Claim CTA done in b433a47 — the on-ramp for the fan-out shipped just before it.

The footer and /guidelines already linked /judges/signup, but the page a judge actually lands on — their own profile, which is where a search for their name sends them — did not. That was the funnel's one high-intent entry point, and it was the missing one.

It reads "Are you {name}? Claim this profile to respond to reviews." as a quiet line under the header rather than a button competing with Follow — it is addressed to one specific person out of everyone who loads the page.

:show rather than :if, since the name arrives with the directory fetch and so the element is false at first render — the exact case bindIf drops and never brings back. Gated on the name having loaded so it never renders "Are you ?" mid-fetch. Verified both ways against the built artifact: with a stub returning the judge it renders in full and links correctly; with the name unloaded it computes to display:none.

Together with 44c589c the right-of-reply path now exists end to end for the first time — a judge can discover claiming from their own profile, and once verified they are told when a review of them publishes.

Remaining here: 'most helpful' sort, notification preferences, re-engagement/newsletter, and discarded search queries.

9 days ago

'Most helpful' sort — done (6b7d4a53, b6a87228)

?sort=helpful on both review endpoints, default recent, with a Most recent / Most helpful toggle on the /reviews feed and the judge profile's review list.

The likes pivot has been the source of truth for 'helpful' since reactions landed, but nothing read it for ordering — so the most useful review about a judge sank out of sight within a week of being written. Wrong default for a reference site.

Ranking spans the whole scope before the page is cut. Sorting the fetched page instead would mean the most-marked review never reaches page 1 unless it also happened to be recent; it can't be an orderBy either, since the count only exists as an aggregate over judge_reviews_likes and this query path has no leftJoin. Ties break on created_at desc then id desc — without a deterministic tiebreak the zero-like majority come back in arbitrary order, which drops and duplicates rows between pages. On the judge endpoint it costs no extra query: the rating-summary scan already covers exactly that scope.

Verified against a seeded database copy (ranking monotonic, all ids present once, no page-boundary duplicates, rejected/pending excluded) and against the built artifact with a stub API (the toggle sends sort=helpful and the list comes back in the exact reverse order).


Found while testing: /judges/:id/reviews was rendering completely blank — fixed in b3e3aece.

The page container shipped with x-cloak and nothing ever removed it, so the rating distribution, every review, the pagination and the new sort control were all display:none. Header, tab bar and footer rendered normally around the gap, console clean. Pre-existing — reproduced on a build from before today's changes.

stx stamps x-cloak on any open tag whose next closing tag comes after an interpolation, which is a property of the expanded include rather than of the element; the three sibling tabs carry the identical wrapper div and were never stamped. Every uncloak site in the runtime is scope-relative, and this div sat above every scope root. Moving the container inside the component makes it the scope root; the emitted DOM is unchanged.

Gate check 17 generalised from '<main> or <body> is cloaked' to 'any cloak nothing will lift' — it flagged this one element and nothing else across 2989 pages.


Remaining here: notification preferences, re-engagement/newsletter, and discarded search queries.

9 days ago

Notification preferences — done (6bdb76ad, 1743ee77)

GET/PATCH /api/me/notification-preferences plus an Email notifications section in Settings, with wantsEmail(userId, category) enforced at both review-mail send sites.

Three emails went to a reviewer with no way to stop them — the submission acknowledgement, the approval, the decline — and the publish fan-out only increases what lands in an inbox. The framework's notification_preferences table and filter layer were already here and already in bench's account-deletion cascade; nothing read them.

One category, deliberately. Account and security mail is not opt-outable — opting out of a password reset means the reset silently goes nowhere — and the settings page states that rather than leaving the omission to be inferred. A category with no send site would be a switch wired to nothing.

Two defaults worth recording: an absent row means send (introducing this must not mute everyone who has never opened settings), and the preference read fails open — these carry the outcome of something the reader is waiting on, so an extra email to someone who opted out beats swallowing a moderation decision. A marketing category would need the opposite.

One sharp edge found by measuring rather than assuming: getNotificationPreferences(userId) with no category does not restrict to the global row — it folds every category's rows into one map keyed by channel, so a category-scoped opt-out would read as a global one. Every call passes a category.

Verified against a database copy (never-visited sends, opted-out does not, opted-out-then-back-in sends, another category leaves this one alone, the allowlist rejects an unknown name and an injection attempt) and against the built artifact with a stub API (correct PATCH body, state survives reload, and with the endpoint forced to 500 the switch rolls back and shows the error).


Remaining here: re-engagement/newsletter, and discarded search queries.

9 days ago

Discarded search queries — done (53ffdf85, c83ed53d)

JudgeSearchAction now records every search, and the admin dashboard shows the zero-result ones as a ranked backlog.

Rows are aggregate, not a log: one per distinct normalised query with a running count. The table stays proportional to the vocabulary people search for rather than to traffic, and it is what makes the list usable as a backlog — "nine people looked for this one" has to outrank "one person did", which a flat event log can't express without a GROUP BY over everything.

Nothing about the searcher is recorded — no user id, no IP, no session, no per-search timestamp. A judge-name search can reveal that someone has a case in front of that judge, so the row is unattributable by construction: there is no column to join against a user even if a later change wanted one. created_at/updated_at describe the query, not a person.

Queries under three characters are skipped — the directory search is a typeahead firing per debounced keystroke, so every real search arrives behind a trail of its own prefixes. The call is not awaited; two extra round-trips on a per-keystroke endpoint isn't worth a statistic.

GET /api/admin/search-demand (admin-gated, zero-result by default, ?all=1 for everything) backs a "Judges people couldn't find" panel on the dashboard, ordered by demand then recency.

Verified against a database copy (normalisation, prefix skipping, aggregation onto one row, zero recorded as zero, no identifying column, and a DROP TABLE attempt stored as text with the judges table intact), through the framework's own migration statement splitter — which has silently dropped statements from multi-statement files before — and in a browser across all three panel states, including a query containing <img src=x onerror=...> rendering as literal text.

One thing worth noting: a failed fetch used to leave the panel saying "Nobody has come up empty yet." over a 0, which is the same misleading-empty-state shape fixed for the public feed in 11b5f704. The subtitle and count are error-aware instead.


Remaining here: re-engagement/newsletter. ./buddy migrate is needed before the capture does anything — the new table is judge_search_queries.

9 days ago

Re-engagement / newsletter — done (e78b5d0b, 026874a6, 3572f115)

The loop exists end to end now: capture on every page → a weekly digest that actually sends → a working way out.

First, a bug that had to be fixed before any of it. sendSubscriptionConfirmation built its unsubscribe link with url('email.unsubscribe', …) — a route name nothing ever registered. url() throws on an unknown name, so the call died before mail.send was reached. No subscriber has ever received a confirmation email, and the only trace was a logged line in the subscribe action's catch. There was also no unsubscribe endpoint at all: the list could be joined but not left. A recurring send on top of that would have been worse than sending nothing.

Opt-out — POST /api/email/unsubscribe plus an /unsubscribe page. POST rather than a one-click GET because mail scanners and link-rewriting proxies fetch every URL in a message. The token is the subscriber's uuid and therefore the authorisation; a well-formed token matching no row still answers ok, since reporting "no such subscriber" would make this an oracle for testing whether a uuid is on the list. Malformed tokens do get a 422 — that means the link was mangled and retrying from the email will help.

Digest — SendWeeklyDigest, Tuesdays. Never sends an empty digest; one message per recipient because each carries its own unsubscribe token; one bad address doesn't abort the run; and it holds its own re-run guard rather than trusting the scheduler, since a restart can re-fire a task and the cost of being wrong is the whole list getting the same mail twice. Every message carries List-Unsubscribe and List-Unsubscribe-Post, which the major providers now require from bulk senders — and which works precisely because the endpoint takes a POST.

Capture — the footer, so it is on every page and survives the launch switch in #62. Signups are tagged source='footer'.

One thing worth recording for anyone touching app/Scheduler.ts: the fluent cadence methods overwrite the cron pattern instead of refining it. Measured — .onDays([2]) → 0 0 * * 2, .at('09:00') → 0 9 * * *, and .onDays([2]).at('09:00') → 0 9 * * *, i.e. every day. The natural-looking spelling would have mailed the entire list daily. There is no raw-cron escape hatch, so it is the day or the hour; the day won.

Also: Campaign.useApi.routes dropped from ['index','show'] to []. Those rows now carry subject lines, audience size and delivery counts, and ORM-generated routes are unauthenticated with no consumer in the app.

Verified by running the real job against a copy of the database with mail.send stubbed — 19 cases covering empty weeks, window and status filtering, per-recipient tokens, the one-click headers, the re-run guard, and a run where every send fails — and the real unsubscribe handler through the router's request context (14 cases: missing/malformed/injection tokens, idempotency, and no enumeration oracle). The pages were checked against the built artifact in every state.


That closes the last open item on this issue.

9 days ago

Last three items — done (d5da77b1, f0c830dd, fe29cde0)

Comment notifications. A comment was a message into the void — the person it was addressed to had no way to learn it existed short of revisiting their own review, so threads died after the first reply. Not deduped, unlike likes: a second comment is a second thing said, and collapsing them would hide half a conversation; the endpoint's 20-per-5-minutes throttle caps the volume instead.

That also closed a trap the two notification surfaces have carried since the fan-out landed. They each keep their own copy of the type-to-wording map and cannot share a module — the stx bundler silently drops value imports from outside resources/stores. Neither throws on an unknown type; both fall through to a bare "New notification". So a type wired into a dispatch site but missed in one surface ships a delivered, clickable, meaningless row and nothing notices. Gate check 21 now reads the NotificationType union and requires a branch in both label functions. My first version of that check was wrong and falsifying it caught the flaw — a file-wide search still matched because NotificationsView also has detailFor, so deleting a case from labelFor left the type mentioned elsewhere in the file and the check passed a page that had lost its wording. It reads each function's own body now.

Email to the claimed judge on publish. The fan-out gave the verified claimant an in-app notification and stopped — but this is a review about them, published without their involvement, and the whole point of the claim flow is that they get to answer it. Followers deliberately get no email: they asked to follow a judge, not to be mailed about one. Gated on a new reviews_about_me preference so it arrives with an off switch rather than needing one added later; the in-app notification is unaffected by it.

The second category exposed a bug in the toggles written for the first: a single in-flight id both refused the other row's click and let whichever save finished first clear the other's spinner — and the comment above it claimed the opposite. In-flight state is per category id now, and a failed write rolls back only its own row.

Article error state + comments paging. A 5xx rendered nothing: fetchById returned silently on !res.ok, so current stayed null, currentNotFound stayed false, and every branch of the view was false at once. There is a retry branch now, worded as a fetch failure rather than a 404 — different claims, and only one is a cue to try again. Comments moved to numbered pagination; the endpoint has returned the paginator since #44 but the client asked for one page of 50, so a long thread lost its tail silently. Posting now lands on the page the new comment is actually on, and deleting keeps the pager's count honest.

One verification gap, stated rather than glossed: neither ArticleView nor Comments appears in dist at all — /article/:id is pre-rendered one page per published review and the database has none. That code could not be exercised in a browser, or even have its compiled script parsed. Everything else here was driven against a copy of the database through the real handlers: comment notifications (12 cases incl. self-comment, authorless review, unpublished parent, counter sync) and the publish fan-out (10 cases incl. follower-not-emailed and the opt-out silencing mail but not the in-app row).


All acceptance criteria on this issue are now met. Two things still gate it in practice: ./buddy migrate for the judge_search_queries table, and live SMTP for any of the mail to leave the building.

9 days ago

Closing: every acceptance criterion on this issue is met.

  • PATCH /api/me persists name, email is read-only, the photo button works
  • Claim CTA on the judge's own profile, plus the footer and FAQ links
  • Publish fan-out: verified claimant in-app and email, followers in-app, comments notify the review author
  • Mobile hamburger wired
  • /reviews and Comments both paginate
  • ?sort=helpful on both review lists with a UI toggle
  • Email-notification toggles in Settings, enforced at every review-mail send site
  • Public stores expose error state; feed, directories and the article page all render a retry instead of an empty or blank page
  • views/500.stx emits dist/500.html
  • Analytics snippet on all 2,989 pages; JudgeSearchAction records {query, result_count} with an admin backlog panel
  • Newsletter capture in the footer, a weekly digest that sends, and a working opt-out

Two things gate it in production rather than in the code: ./buddy migrate for the judge_search_queries table, and live SMTP.

One verification gap recorded rather than glossed: ArticleView and Comments appear nowhere in dist, because /article/:id is pre-rendered one page per published review and there are none — that code is compile- and gate-verified but was never exercised in a browser.

Sign in to comment on this issue.