fix(golf/messages): the follow-through #1830 owed — a press that survives a thumb, a menu that closes, a message you can copy by njrini99-code · Pull Request #1835 · njrini99-code/helmv3

njrini99-code · 2026-09-04T22:23:49Z

Follow-up to #1830. That PR made two good moves — press-and-hold replacing the
floating kebab, and conversation-kind.ts as the one group derivation — and
both stopped one step short. The gaps land on the affordance it made
load-bearing.

⚠️ Read this before merge order: #1833 preserves all six

#1833 (agent/messages-instant-entry) rewrites MessageThreadPane.tsx
(+786/−137) and FairwayMessages.tsx (+186/−88) — the same two files. It does
not fix any of these; it rewrote around them. Checked against its head:

Defect	Still present in #1833
Unscoped `select-none` on own bubbles	`MessageThreadPane.tsx:1647`
`onPointerMove: cancelLongPress`, no slop	`MessageThreadPane.tsx:649`
Raw `conv?.is_group` participant fetch	`FairwayMessages.tsx:224`
No edit/delete reset on conversation switch	all three `setEditingMessageId(null)` are in handlers (456/465/482)

So whichever of the two lands second needs a rebase, and if #1833 lands alone
these six ship again inside the rebuild. Happy to re-apply this on top of
#1833 instead — say the word.

The six

Own messages became uncopyable on desktop. select-none went on unscoped,
but the menu carrying Copy is lg:hidden and the hover row surviving at lg
has only Edit and Delete. From lg up it removed selection where nothing gave
it back. #1830's own stated reason was iOS-specific ("so iOS cannot race its
menu against ours"), so this is overreach, not a decision. Scoped to max-lg:
— exactly where Copy exists. The callout suppression stays unscoped; it is
inert off iOS Safari.

The press cancelled on the tremor of holding still. onPointerMove: cancelLongPress fired on sub-pixel jitter, and a resting thumb is never
perfectly still. With the kebab gone this gesture is the only route to Copy,
Edit and Delete on a phone, so an unreliable press is an unreachable menu.
10px of slop — above finger jitter, far below what a real scroll covers in
450ms. exceedsLongPressSlop is exported because the threshold is the
behaviour; asserting it through the rendered component would mean synthesising
pointer coordinates jsdom does not model.

The menu had one way out and it was the X. Long-press opens it without
moving the page, so it floated on beside a bubble that had scrolled away. Now
closes on the next press elsewhere and when the thread scrolls under it. Its
own listener, not a branch in the stick-to-bottom handler: that one is about
scroll position and is torn down per conversation.

State outlived the thread it belonged to. Edit mode, its draft, the delete
prompt and the open menu all survived a conversation switch. Nothing could be
misdelivered — Save and Confirm only render inside the matching bubble — but
coming back re-entered that bubble mid-edit holding text from before. This is
the hazard the keyed composer is documented against, for the four pieces of
state that were not keyed with it.

One consumer never got the one derivation. FairwayMessages still asked
raw is_group before fetching group participants, so a flagged two-person DM
fired three extra queries to build a map the pane never reads. The rail (144)
and pane (812) were converted; this was missed. Line 227 is deliberately not
converted — that is find-or-create, not presentation, and changing it would
make a flagged DM newly eligible to match.

Plus: an empty thread showed nothing while the other person typed (the
indicator lived inside the has-messages branch), and each conversation now gets
its own one-shot attachment-retry budget — the bounding set was never cleared,
so an attachment that lost the commit-order race once stayed on its retry chip
for the rest of the session.

Two greens that verified nothing

team_communications required a baseball spec for a golf feature.
required_checks named e2e/messages.spec.ts; all nine of its page.goto
calls target /baseball/dashboard/messages, while the feature's routes are
golf. Re-pointed at e2e/golf-critical-paths.spec.ts, which actually loads
/golf/dashboard/messages.

And that spec self-skips on credentials Playwright itself injected.
hasGolfCoachAuth reads GOLFHELM_COACH_* at module level. Both are in
.env.local, Playwright prints injected env (80) from .env.local, and the
constant is still false — the injection does not reach the spec module's
top-level read. Same command, one change:

npx playwright test ... -g "messages loads a conversation"   1 skipped, exit 0
set -a; . ./.env.local; set +a; <same command>               1 passed,  exit 0

Both green; the first verified nothing. Recorded in quality-gates.md §2, with
the export written into the registry entry beside the check.

shipping.md §3 also gains the sandboxed-npm run dev trap: an EMFILE flood
and a .next/dev restart loop that prints ✓ Ready while curl gets Empty reply from server. The obvious diagnosis (a second dev server) was wrong —
lsof -a -p <pid> -d cwd put the others in a worktree with their own .next.

Verification

typecheck, lint --max-warnings 0, npm test (1563 files, 15229 passed / 10
skipped), and next build all green. max-lg:select-none confirmed present in
the emitted CSS bundle — a variant that silently failed to compile would look
identical in the diff.

Real browser: with the credentials exported, all 11 golf critical-path
tests pass against Chromium, including messages loads a conversation.

markdown:ratchet fails +4 on the branch this was authored on, and fails
identically with these files stashed — pre-existing drift from concurrent work,
not from this change. Baseline deliberately not raised.

Not device-verified. Long-press timing and the 10px slop are exactly the
class of thing that reasons correctly and can still be wrong under a thumb.

🤖 Generated with Claude Code

https://claude.ai/code/session_019euu9jtJM6WvMVj17coqZ1

…ives a thumb, a menu that closes, a message you can copy #1830 replaced the floating kebab with press-and-hold and made `conversation-kind.ts` the one group derivation. Both were right; both were left one step short, and the gaps land on the affordance that PR made load-bearing. **Own messages became uncopyable on a desktop.** `select-none` went onto own bubbles unscoped, and the menu carrying Copy is `lg:hidden` while the hover row that survives at `lg` has only Edit and Delete. So from `lg` up the PR removed text selection at exactly the widths where nothing gave it back — the PR's own reasoning for the opt-out ("so iOS cannot race its menu against ours") never reached that far. Scoped to `max-lg:`, which is precisely where Copy exists. The callout suppression stays unscoped; it is inert off iOS Safari. **The press cancelled on the tremor of holding still.** `onPointerMove: cancelLongPress` fired on sub-pixel jitter, and a resting thumb is never perfectly still. Since the kebab is gone this gesture is the ONLY route to Copy, Edit and Delete on a phone, so an unreliable press is an unreachable menu. 10px of slop: above finger jitter, far below what a real scroll covers in 450ms. `exceedsLongPressSlop` is exported because the threshold IS the behaviour and the values either side of it are what a test can pin — synthesising pointer coordinates through the rendered component would be testing jsdom. **The menu had one way out, and it was the X.** Long-press opens it without moving the page, so it floated on beside a bubble that had scrolled away. It now closes on the next press anywhere else and when the thread scrolls under it. Its own listener, not a branch inside the stick-to-bottom handler: that one is about scroll POSITION and is torn down per conversation; this one is about a menu being open. **State outlived the thread it belonged to.** Edit mode, its draft, the delete prompt and the open menu all survived a conversation switch, so coming back re-entered a bubble mid-edit holding text from before and re-asked a delete the user had walked away from. Nothing could be MISDELIVERED — Save and Confirm only render inside the matching bubble — but this is the hazard the keyed composer is documented against, for the four pieces of state that were not keyed with it. **One consumer never got the one derivation.** `FairwayMessages` still asked raw `is_group` before fetching group participants, so a flagged two-person DM fired three extra queries to build a map the pane then never reads — its `isGroup` is the participant-count derivation, so it resolves the sender from `other_participant`. The rail (144) and the pane (812) were converted; this was missed. Line 227 is deliberately NOT converted: that is find-or-create, not presentation, and changing it would make a flagged DM newly eligible to match. Also: an empty thread showed nothing while the other person typed — the indicator lived inside the has-messages branch — and "say hello below" is the wrong thing to say to someone watching a reply being written. And each conversation now gets its own one-shot attachment-retry budget; the bounding set was never cleared, so an attachment that lost the commit-order race once stayed on its retry chip for the rest of the session, and leaving the thread and coming back could not earn it another attempt. Verified: typecheck, `lint --max-warnings 0`, `npm test` (1563 files, 15229 passed / 10 skipped) and `next build` all green, and `max-lg:select-none` is present in the emitted CSS bundle — a variant that silently failed to compile would have looked identical in the diff. NOT device-verified, and the dev server could not show it either: the page redirects to login and this session has no credentials, so the browser evidence here is that the route boots and builds, not that the gesture feels right on glass. Long-press timing and the 10px slop are exactly the class of thing that reasons correctly and can still be wrong under a thumb. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019euu9jtJM6WvMVj17coqZ1 (cherry picked from commit b8e82f3)

The positive assertion already pins the fix: if `max-lg:` is ever dropped from the own-bubble class, `toContain` fails. The negative one restated that by string prefix and would additionally fail on a reformat of the `cn()` call — reporting the absence of a string rather than the loss of desktop copyability, which is a tripwire pointing at the wrong thing. (Landed separately because an `--amend` raced another session's commit onto this shared checkout and folded the change into `docs(rules): eleven claims…`. That commit is restored verbatim as this one's parent; nothing of it was rewritten.) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019euu9jtJM6WvMVj17coqZ1 (cherry picked from commit ef9bb47)

…re, and two greens that verified nothing **`team_communications` required a baseball spec for a golf feature.** `required_checks` named `npm run test:e2e -- e2e/messages.spec.ts`. Every one of that spec's nine `page.goto` calls targets `/baseball/dashboard/messages`, and this feature's `routes:` are `src/app/golf/(dashboard)/dashboard/messages`. So the one required check for the feature could not execute a line of it, and running it to green read as coverage. Re-pointed at `e2e/golf-critical-paths.spec.ts`, which actually loads `/golf/dashboard/messages`. The baseball spec stays under `code.tests` — annotated for what it is — because it is still the messaging spec; it is just not evidence about this feature. **And that spec self-skips on credentials Playwright itself injected.** `hasGolfCoachAuth` is a module-level read of `GOLFHELM_COACH_EMAIL` / `GOLFHELM_COACH_PASSWORD`. Both are in `.env.local`, and Playwright prints `injected env (80) from .env.local` at startup — yet the constant is false, because that injection does not reach the spec module's top-level read. Same command, one change, measured today: npx playwright test ... -g "messages loads a conversation" 1 skipped, exit 0 set -a; . ./.env.local; set +a; <same command> 1 passed, exit 0 Both green; the first verified nothing. That is `quality-gates.md` §2's category exactly — a gate that cannot fail — so it is recorded there, and the export is written into the registry entry beside the check so the next reader does not have to rediscover it. With the credentials exported, all 11 golf critical-path tests pass against a real browser. **`npm run dev` inside the Bash sandbox fails while reporting success.** An EMFILE watcher flood (with `ulimit -n` at 1048576, so it is the sandbox's limit, not the shell's) and a `.next/dev was deleted → Restarting` loop. The log prints `✓ Ready in 133ms`; `curl` gets `Empty reply from server` and then refuses to connect. The obvious diagnosis — a second dev server fighting over `.next/` — was wrong: `lsof -a -p <pid> -d cwd` put the other two `next` processes in a worktree with their own `.next`. Outside the sandbox it is ready in 158ms and answers 200. Recorded in `shipping.md` §3 with the instruction the episode actually earns: curl the server before reporting it as running. Verified: `docs:check` (all five gates), `check-registry-globs` (0 dead of 635). `markdown:ratchet` fails +4 on this branch, and it fails identically with these three files stashed — the drift is pre-existing from concurrent work here, not from this change. Baseline deliberately NOT raised. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019euu9jtJM6WvMVj17coqZ1 (cherry picked from commit d52e348)

qodo-code-review · 2026-09-04T22:23:53Z

ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing

vercel · 2026-09-04T22:23:54Z

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment

Project	Deployment	Actions	Updated
helmv3	Ignored	Preview	Sep 4, 2026 10:27pm UTC

coderabbitai · 2026-09-04T22:24:00Z

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Team

Run ID: 539c6344-b579-4d0e-9e2a-427ec8bfcc69

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

supabase · 2026-09-04T22:24:06Z

This pull request has been ignored for the connected project qmnssrrolpinvwjjnufo because there are no changes detected in supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.

Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

`open-pr-residue` requires the key set here to equal the live open-PR set, so opening a PR without a row makes the check fail — but a row for THIS PR cannot be on main while the PR is in flight. The file's own schema resolves that: a row absent from main but present at the PR's own head is IN FLIGHT, not unclassified. This is that row, at that head. HUMAN_TEST_PENDING rather than ACTIVE, and the distinction is doing work: the change is finished and green, and what it is waiting for is a thumb. A 450ms hold with 10px of slop cannot be verified by a test that synthesises its own pointer events. It is also waiting on a merge-order call against #1833. worktree_policy PARK_IF_REPRODUCIBLE releases the checkout — it exists only to have cherry-picked and pushed this branch, and parking keeps the branch. Both gates have to permit, so the workspace marker is being released in the same step; neither one implies the other. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019euu9jtJM6WvMVj17coqZ1

…e has ended `open-pr-residue` requires the key set to equal the open-PR set, and a row for an in-flight PR lives at that PR's own head rather than on main — so #1836's row rides its own branch. Verified both directions: without it the check names #1836 as unclassified; with it, it does not. Also deletes #1725 and #1738, both MERGED, both past the transitional grace this file's own `$comment` defines — the tool's output says in as many words to delete them "in the PR you are already opening", and that is this one. Leaving them is the exact failure the comment describes: a current-state registry asserting things that stopped being true. **`open-pr-residue` still FAILS, for reasons this PR does not own.** Twelve open PRs carry no row at all — #1834, #1833, #1832, #1831, #1829, #1827, #1759 and five dependabot PRs. That was already true before this commit and is not mine to reconcile; a row is a statement about someone else's work. Recording it here so the red is legible rather than mysterious. Note for merge order: #1835 also edits this file, so whichever lands second resolves that conflict. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019euu9jtJM6WvMVj17coqZ1

* fix(ci): migration-lockdown can finally gate — drop the paths filter that made it un-requireable block-historical-edits only ran when a PR touched supabase/migrations/**, so a GitHub required-status-check pointed at it would leave every other PR waiting on "Expected" forever — the same phantom-context failure mode already fixed once for `all` and `CodeQL` (see branch-protection.md). Moved the path check inside the job as a first step (reusing changed-files.sh, same pattern review-gate.yml uses) so the workflow always runs and always reports a conclusion, then the existing AM-diff lockdown logic runs unchanged when migrations actually changed. Proved both paths locally in a throwaway worktree: an edit to a pre-baseline migration exits 1 with the violation printed, and a docs-only diff exits 0 without running the lockdown step at all (see wave3-A8.md for the transcript). Also added merge_group as a trigger (ci.yml and review-gate.yml both have it; migration-lockdown needs it too once it's a required context, or a queued merge would wait on a context that never fires for merge-queue events) and documented the target six-context required-status-checks state in branch-protection.md — applying it on GitHub is a separate step. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * fix(ci): stop claude-code.yml from spawning a run per bot review comment Measured: 100 of the last 100 runs were skipped (94) or cancelled (6), 0 succeeded — every CodeQL/CodeRabbit/Qodo/Supabase inline review comment restarted this workflow for a job that always gates out on the author check. Dropped the `pull_request_review_comment` trigger; `issue_comment` still covers `@claude` in an ordinary conversation-tab comment, and the `issues: labeled` path and the author-association + `@claude` gate on the job itself are untouched. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * fix(ci): gate feature-awareness on code-relevant paths, drop redundant p/ci semgrep scan feature-awareness.yml's "Build context pack" job ran unconditionally on every PR (17-45s + a checkout/Node-setup) for a report that maps changed files through memory/registry.yml — nothing to map on a docs-only or CI-config-only diff. Added a detect-changes job that copies ci.yml's own classification verbatim (same fail-closed rule: non-PR events and any diff-classification failure resolve to code=true) so the two workflows never disagree about what counts as docs-only, and gated `report` behind its `code` output. review-gate.yml's semgrep job dropped the "Scan with default p/ci ruleset" step — a bare community ruleset with no repo-specific tuning, redundant with the custom-rules step in the same job (the actual AGENTS.md hard-rule enforcement) and/or Semgrep Cloud Platform's hosted scan. The `all` aggregate reads job-level needs.*.result for `semgrep`, not per-step outcomes, so removing an unidentified step needed no change there. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * docs(ci): fix six doc contradictions the pr-checks-review audit found Each of these was a doc asserting something the workflows no longer do, found by the read-only PR-check audit (§4): - docs/CI_RUNBOOK.md's `CodeQL` row said "Hard gate" directly under a callout that already established it's a phantom for gating purposes — the row now matches the callout: not required, real alert-delta signal. - docs/CI_RUNBOOK.md's `Vercel` / `Vercel Preview Comments` row described a live advisory check; it stopped posting between PR #1835 and #1839 since git deploys are disconnected. Row now says so. - .claude/rules/integrations.md claimed Lighthouse CI "runs ... in CircleCI's `lighthouse-preview` job on every push" — that job has never existed (.circleci/README.md already corrected the same claim). Now describes the actual local-only `npm run lighthouse` path. - .github/workflows/pr-smoke.yml's header and a job comment both claimed "Hard a11y enforcement remains on `main` via playwright.yml" — that workflow is `workflow_dispatch`-only with no push/pull_request trigger at all, so nothing currently enforces a11y automatically anywhere. Also corrected the header's stale "Build smoke lives in playwright.yml -> Smoke checks" (that job was deleted 2026-09-02). - ci.yml's own top-of-file note said "Static checks ... sixteen named steps"; it's 24 today and was already stale at nine before that. Replaced the count with a `grep -c` recipe, per shipping.md §1 (never write a count into prose) — which turns out to apply inside workflow comments too. - quality-gates.md and code-review-tooling.md each named only `ci.yml` and `review-gate.yml` as the PR-time GitHub Actions surface. Both now name every workflow that posts a check or comment on a PR — codeql, sentry-snapshots, feature-awareness, pr-smoke, migration-lockdown, claude-code — one line each, no counts. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * docs(ci): wrap three long lines and regenerate the document inventory The markdown ratchet counted one new MD013 finding and the inventory check found two link-count rows stale after the rebase. Verified: markdown ratchet no regressions, docs:check exit 0. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * docs(ci): keep the markdown ratchet flat after the runbook row rewrites Two rewritten table rows count as one more MD013 line than the rows they replaced; two list-continuation lines elsewhere in the runbook are wrapped so the file's count goes down instead. Inventory regenerated. Verified: markdown ratchet exit 0 (no regressions), docs:check exit 0. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>

njrini99-code and others added 3 commits September 4, 2026 18:23

njrini99-code mentioned this pull request Sep 5, 2026

feat(golf/messages): the approved design — unread lifts, the tail marks the end, and the token that had no utility #1836

Open

Navigation Menu

fix(golf/messages): the follow-through #1830 owed — a press that survives a thumb, a menu that closes, a message you can copy - #1835

fix(golf/messages): the follow-through #1830 owed — a press that survives a thumb, a menu that closes, a message you can copy#1835
njrini99-code wants to merge 4 commits into
mainfrom
agent/golf-messages-1830-followthrough

njrini99-code commented Sep 4, 2026

Uh oh!

qodo-code-review Bot commented Sep 4, 2026

Uh oh!

vercel Bot commented Sep 4, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Sep 4, 2026 •

edited

Loading

Review skipped

Uh oh!

supabase Bot commented Sep 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

njrini99-code commented Sep 4, 2026

⚠️ Read this before merge order: #1833 preserves all six

The six

Two greens that verified nothing

Verification

Uh oh!

qodo-code-review Bot commented Sep 4, 2026

Uh oh!

vercel Bot commented Sep 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot commented Sep 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Uh oh!

supabase Bot commented Sep 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

vercel Bot commented Sep 4, 2026 •

edited

Loading

coderabbitai Bot commented Sep 4, 2026 •

edited

Loading