Fix composer focus when selecting channels and DMs - #307
Conversation
Signed-off-by: Smartie <fb3185faacfc3760b6b4fe0085a8878d8dd537f7a7671c7d3184fe1aa1b04df4@buzz.block.builderlab.xyz>
Signed-off-by: Smartie <fb3185faacfc3760b6b4fe0085a8878d8dd537f7a7671c7d3184fe1aa1b04df4@buzz.block.builderlab.xyz>
|
Independent agent review of local head Blockers in the reviewed diff: none. The first head's lifecycle focus regression was a concrete blocker: deleting a selected channel falls back to another sidebar row, which must keep focus ( The new browser case is justified by real sidebar navigation → ProseMirror native focus and keyboard typing, including warm channel/DM returns; the component tests cover mount/retarget/disabled focus policy. One browser case added, none removed. I independently inspected the full diff and Optional, non-gating: A click on the already selected sidebar row may leave focus on that row because no composer remount occurs. The requested switch-to-channel/DM behavior is covered; only extend this if reselect-to-compose is explicitly intended. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f6c5e2e5c4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -0,0 +1,37 @@ | |||
| import { test, expect } from "./fixture.mjs"; | |||
There was a problem hiding this comment.
The reviewed commit's raw message has no Signed-off-by trailer, so it does not satisfy the repository's per-commit DCO requirement and the DCO check will reject this exact squashed commit even though the message reports a pass for the earlier f6c5e2e5 head. Recreate the commit with a sign-off from its actual author and verify the check at the new head.
AGENTS.md reference: AGENTS.md:L153-L160
Useful? React with 👍 / 👎.
| (previous === document.activeElement || | ||
| (!previous.isConnected && document.activeElement === document.body)) | ||
| ) | ||
| input.current?.focus(); |
There was a problem hiding this comment.
When returning to a conversation with a nonempty draft, its keyed composer is reconstructed and ProseMirror's newly created editor state starts with its selection at the beginning; this focus call preserves that selection, so immediate typing prepends text to the draft instead of continuing it. Set the initial selection to the draft's end when applying this navigation focus, and extend the real-browser case to type during a warm return—the current test only types during the first visits to empty composers.
AGENTS.md reference: AGENTS.md:L105-L108
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 189a221. Navigation autofocus now uses the existing editor value/setSelectionRange API to place the caret at the restored draft end, only after the explicit-focus-owner guard succeeds. Extended the existing browser case to type on both warm channel and DM returns. Before the fix it reproduced continuedDraft for Beta; after the fix all 22 composer-focus/lifecycle/message-actions cases pass in Chromium and WebKit. Full Vitest: 4,240 passed; hooks passed. CI rerunning.
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
P2 remains at 189a22163fcad0d278bffd9abffeb9cfcc1f41f9: synchronize the browser caret after taking focus. This is follow-up evidence for this same finding, not another issue.
My first unchanged-production run of the updated checked-in composer-focus.spec.mjs failed in Chromium at line 30: expected Draft for Beta continued, received continuedDraft for Beta. The other 23 executions passed. Ten subsequent repeats passed, so this is intermittent, not a deterministic failure or a resolved issue.
At MessageComposer.tsx:224–226, editor is the enhanced DOM root, not an EditorView. EditableInput.tsx:322–335,1135–1178 makes setSelectionRange dispatch a ProseMirror selection while unfocused. ProseMirror skips writing that selection to the DOM without focus (prosemirror-view/src/selection.ts:50–59); native focus schedules a 20ms reconciliation (input.ts:781–791). Immediate keydown/beforeinput can instead read the browser’s initial caret back through syncNativeSelection (EditableInput.tsx:683–719,898–901,1008–1009) and prepend text.
Smallest repair: call editor.focus() before editor.setSelectionRange(end, end), matching the existing restoration paths at MessageComposer.tsx:352–361; retain the explicit-focus-owner guard. Independent review reproduced immediate-typing failure in 4/4 Chromium runs, then verified 16/16 Chromium/WebKit executions of its probe plus the checked-in journey with only those two lines swapped. That is experimental evidence, not a fix present on this PR.
Keep channel and DM warm-return append assertions, but exercise typing immediately after the focus handoff, without the extra pre-typing text wait, sleeps, or retries masking the ordering defect. No broader focus service or draft-storage change is needed.
wesbillman
left a comment
There was a problem hiding this comment.
Star Lord automated source review (via Wes’s account)
Reviewed head f6c5e2e5c4db61508541816563a8d1d7920a221d against base c53fb83c6910c66865a5e8232a1f55cce95954f4 (merge base e5ea85ea95bfaa77e8b83938066f1c96a4f0ae79). One actionable P2 finding, inline: warm-return typing starts before the saved draft. This independently corroborates the existing caret-placement comment rather than adding a second distinct issue.
The rest of the bounded change is proportionate: it reuses the conversation remount key, defaults other composer consumers to no autofocus, excludes exact message/thread targets, and yields to the sidebar’s explicit lifecycle focus restoration. The added browser journey has a genuine native-focus/typing boundary; mounted tests cover the update/disabled/competing-layout-effect policy. No tests were removed. Reselecting the already-open conversation is outside this mount-only scope and is not a requested expansion.
The existing DCO warning is not supported at this head: both outgoing commits (8aaa37c0 and f6c5e2e5) contain their actual author’s Signed-off-by, and the exact-head hosted DCO Check succeeded.
Validation limits: source analysis only; no tests, builds, installs, PR-code execution, app launch, or native/human acceptance performed. Immutable source extracts were verified; git diff --check passed. The PR’s reported Vitest/22-browser-case results were not rerun. One CI snapshot at approximately 22:45 UTC showed Chromium and WebKit shard 2/3 failed: both logs time out at gifs.spec.mjs:443 locating the Pages → Messages button. This is unresolved CI evidence, not an established autofocus regression; other browser shards were still running. Final-head human confirmation of delete-channel fallback focus remains explicitly outstanding in the PR. This COMMENT is neither approval nor merge authorization.
| (previous === document.activeElement || | ||
| (!previous.isConnected && document.activeElement === document.body)) | ||
| ) | ||
| input.current?.focus(); |
There was a problem hiding this comment.
[P2] Position the caret for immediate typing into a restored draft
On a warm channel/DM return the keyed composer remounts and restores its nonempty draft (MessageComposer.tsx:198–202), but EditableInput.tsx:320–321,771–774 creates a fresh ProseMirror state without a selection. The pinned prosemirror-state@1.4.4 defaults that to Selection.atStart (source). This new focus call does not advance the caret, so the advertised click-and-type warm return prepends new text instead of continuing the saved draft.
Within the successful mount-autofocus branch, initialize the caret at the restored draft’s end using the existing setSelectionRange API (or restore a deliberately retained selection), without changing the explicit-focus-owner guard. Extend the browser case to type again on a warm return and assert the resulting draft; it currently types only on the two empty first visits. This corroborates existing comment 4109195945; no separate repair is needed for the duplicate observation.
Signed-off-by: Smartie <fb3185faacfc3760b6b4fe0085a8878d8dd537f7a7671c7d3184fe1aa1b04df4@buzz.block.builderlab.xyz>
|
Follow-up review only of the P2 fix at |
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Reviewed head 189a22163fcad0d278bffd9abffeb9cfcc1f41f9 against base c53fb83c6910c66865a5e8232a1f55cce95954f4 (merge base e5ea85ea95bfaa77e8b83938066f1c96a4f0ae79). Changes required: the existing P2 warm-return caret defect remains intermittent after the latest fix. Evidence and the two-line repair are in the existing finding’s follow-up.
- Validation: current-head run of the complete composer-focus, message-actions, and channel-lifecycle browser files plus a temporary probe: 23 passed, 1 Chromium warm-return append failure. Ten subsequent composer-focus repeats passed; they do not negate the captured failure. Deletion fallback, Reply-close restoration, and copied-message deep-link cases passed in both engines. No production files changed in my checkout.
- Other gates: current-head JavaScript, browser measurements, security checks, and DCO passed; Rust/tool integration and all six browser shards were still running at the delivery snapshot. Earlier red GIF shards reproduce on main, separate from this defect. Broad suites were not duplicated locally; native/live-relay and final-head human confirmation remain unverified.
- Exit criteria: synchronize focus and caret before immediate typing, retain the explicit-owner guard, and verify channel/DM warm returns in both engines. Minimalness 9/10, elegance 9/10, correctness 8/10 for this one reproduced defect. Already-selected-row refocusing remains outside scope.
…ad-on-send * origin/main: (58 commits) Keep profile avatar cutouts transparent and align the header gutter (#319) Restore sidebar status icons beside names (#316) docs(mentions): specify portable mention rules (#343) fix(agents): wait for native host operations (#331) Simplify channel templates and report setup failures accurately (#318) feat(agents): Harnesses Goose install (slice 3/5) (#279) feat(agents): Harnesses status card in Settings (slice 2/5, stacked on #272) (#277) Fix timer operation ownership and stabilize timing regressions (#317) Restore cached workspace before relay startup (#311) test(browser): wait for the app's own quota cooldown before retrying (#284) docs: define Harnesses setup and global agent defaults (#272) Make mention choices consistent and stable (#258) Discover saved relay agents without changing the page (#224) feat: add persistent dev log levels and relay traffic summaries (#306) Polish inline message reactions and previews (#213) feat(identity): add native macOS import, creation and backup (#308) fix(status): reopen a Today status as Today near 16:00 (#275) test: use current navigation for GIF send roundtrip (#309) Fix composer focus when selecting channels and DMs (#307) fix: retire mention searches after chips and refuted prose (#303) ... # Conflicts: # src/features/messages/MessageComposer.test.tsx # src/features/messages/MessageComposer.tsx
Latest update: P2 restored-draft caret (189a221)
Navigation autofocus now places the caret at the restored draft end. The existing browser case types again on warm channel AND DM returns; reproduced prepending on f6c5e2e, passes on 189a221. Final-head full Vitest: 4,240 passed; focused Chromium/WebKit: 22 passed; hooks passed. Hosted CI rerunning. Returned to draft for follow-up review and human confirmation of this changed behavior. Prior-head evidence below is historical, not latest-head acceptance.
Summary
Origin: Buzz channel
dec3c452-cf15-4472-bb71-8de17a571993, thread5c72d5ddc0d0aa1017e0a3711f3b3b1f470decdf17baea2f97fac751ffcd5e3f.Evidence at f6c5e2e
pnpm test:browser --project chromium --project webkit --no-deps tests/browser/composer-focus.spec.mjs tests/browser/message-actions.spec.mjs tests/browser/channel-lifecycle.spec.mjs: 22 passed.Review and human confirmation
Remaining / human try
Hosted CI pending; native GUI and full integration/browser suites were not rerun locally. To confirm the follow-up: delete a disposable selected channel and verify focus lands on the fallback sidebar row, not its composer.
Original acceptance steps: