Repository navigation
fix(web): arrow and page keys scroll the pane under the pointer - #695
Conversation
Codex Review Summary: ✅ CompletedThis 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. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 691d5226dd
ℹ️ 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".
691d522 to
38a6720
Compare
The source-file editor focuses inside a shadow root, where `closest` cannot reach the surrounding workspace area. The router then missed that the editor and the pointer's scroller share a pane, and took the editor's arrow and page keys. Pane, layer and key-owner lookups now continue through shadow hosts. Carry-Group: incubator Carry-Observation: Follows up the router commit in the same PR; same mechanism, same group. Reported by Codex review on PR #695. Co-Authored-By: Claude <[email protected]>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 38a6720975
ℹ️ 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".
…ting panes' headers - A configured binding on a scroll key wins even when its handler listens outside the shortcut dispatcher, as thread navigation does. - Over a pane's header, the pane's scroll container counts even while its content fits, so the key stops there instead of scrolling the focused pane. Carry-Group: incubator Carry-Observation: Follows up the router commit in the same PR; same mechanism, same group. Reported by Codex review on PR #695. Co-Authored-By: Claude <[email protected]>
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Workspace regions that hold focus no longer page their scroller from workspaceFocus, and the dispatcher loses the fork-only `fallback` scope that served it. The LastCode scroll-key router (docs/lastcode/keyboard-scrolling.md) now covers a focused region along with pointer-targeted scrolling, so workspaceFocus.ts and dispatcher.ts return to upstream. Carry-Group: incubator Carry-Observation: Owned by incubator because incubator already edits these workspaceFocus lines (same-base replay conflicted under upstream-bugfixes). Removes the pageFocusedArea part of lastCode #530; its page-distance and controller-reattach changes in pageScrollController/useTimelineFollow stay. Carry-Supersedes: lastCode #530 (pageFocusedArea, dispatcher fallback scope) Co-Authored-By: Claude <[email protected]>
The sidebar no longer binds its own PageUp/PageDown listener. The LastCode scroll-key router pages whatever pane is under the pointer, the sidebar and its resize rail included, and adds arrow keys. Carry-Group: incubator Carry-Observation: Owned by incubator because incubator already edits sidebar.tsx and this test (same-base replay conflicted under legacy-sidebar). sidebarPageScroll handled only page keys, so arrows over the sidebar went to the focused timeline once upstream made it focusable (pingdotgg#27209). Carry-Supersedes: lastCode #301 Co-Authored-By: Claude <[email protected]>
Upstream sends arrow and page keys to the focused element. Since the timeline scroller became focusable (upstream pingdotgg#27209), ArrowUp/ArrowDown with the pointer over the sidebar scrolled the conversation. LastCode's per-surface patches covered page keys only and kept regressing as upstream moved. One router, lastcodeScrollKeys.ts, now owns the rule: the pointer picks the pane, focus decides inside it. Text fields and keyboard widgets keep their keys over their own pane (the composer over its conversation keeps caret moves, prompt recall and its page handoff); any other key parks the pointer for arrows until the mouse moves; suggestion lists, the terminal, open menus, keybindings and modified keys are left alone. The timeline registers as an owner so routed scrolls release follow and page above the composer. It replaces the hover-card key handler and the timeline page fallback. docs/lastcode/keyboard-scrolling.md records the desired behaviour, the upstream seams it rests on, and when a checkpoint is fighting upstream. Carry-Group: incubator Carry-Observation: Inseparable from its tests and docs; the timeline owner registration and hover-card removal replace incubator code in the same mechanism. Carry-Supersedes: lastCode #654 timeline page fallback; thread hover scroll keys Carry-Applies-To: upstream 7e1a058 (focus-driven native scrolling, pingdotgg#27209) Co-Authored-By: Claude <[email protected]>
Review follow-ups for the scroll-key router: - A pane under the pointer stops the key even while its content fits, so a short sidebar no longer lets the key fall through to the focused pane. - Workspace areas inside one dialog are separate panes, so a dialog's search field no longer keeps keys aimed at its content area. - A touch or pen tap forgets the remembered mouse position. - The no-pointer fallback leaves the terminal region alone. The behaviour table now states the sidebar-row and modified-arrow rules and that keys inside an embedded frame cannot be routed. Carry-Group: incubator Carry-Observation: Follows up the router commit in the same PR; same mechanism, same group. Co-Authored-By: Claude <[email protected]>
…ane focus Second round of review follow-ups for the scroll-key router: - The scroller chain stops at overscroll-behavior contain/none, like the wheel, so a contained list no longer moves its enclosing pane. - Shortcut recorders ([data-keybinding-capture]) keep every key, so a PageUp/PageDown binding can be recorded with the pointer elsewhere. - When the pane under the pointer has nothing to scroll and focus is inside it, the focused element gets the key, so settings-section and sidebar-row arrow navigation work in panes whose content fits. Carry-Group: incubator Carry-Observation: Follows up the router commit in the same PR; same mechanism, same group. Co-Authored-By: Claude <[email protected]>
With the pointer over non-scrolling chrome inside a workspace pane, such as the sidebar's search header beside its list, scroll keys now scroll that pane's list instead of falling through to the focused pane. Carry-Group: incubator Carry-Observation: Follows up the router commit in the same PR; same mechanism, same group. Co-Authored-By: Claude <[email protected]>
The source-file editor focuses inside a shadow root, where `closest` cannot reach the surrounding workspace area. The router then missed that the editor and the pointer's scroller share a pane, and took the editor's arrow and page keys. Pane, layer and key-owner lookups now continue through shadow hosts. Carry-Group: incubator Carry-Observation: Follows up the router commit in the same PR; same mechanism, same group. Reported by Codex review on PR #695. Co-Authored-By: Claude <[email protected]>
…ting panes' headers - A configured binding on a scroll key wins even when its handler listens outside the shortcut dispatcher, as thread navigation does. - Over a pane's header, the pane's scroll container counts even while its content fits, so the key stops there instead of scrolling the focused pane. Carry-Group: incubator Carry-Observation: Follows up the router commit in the same PR; same mechanism, same group. Reported by Codex review on PR #695. Co-Authored-By: Claude <[email protected]>
a4370f0 to
6b005e0
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Upstream sends arrow and page keys to whichever element has focus. Since upstream made the timeline scroller focusable (#27209), ArrowUp/ArrowDown with the pointer over the LastCode sidebar scrolled the conversation. LastCode's earlier fixes were four separate patches that handled page keys only, and they kept regressing as upstream changed.
Fix
One router,
apps/web/src/lastcodeScrollKeys.ts, owns one rule: the pointer picks the pane; focus decides what happens inside it.It replaces the old
sidebarPageScroll,threadHoverScrollKeys, theworkspaceFocuspane paging and the dispatcherfallbackscope, and the timeline's page fallback.workspaceFocus.tsanddispatcher.tsgo back to upstream.docs/lastcode/keyboard-scrolling.mdis the behaviour contract for checkpoint rebases. It records the desired behaviour table, the upstream seams the router depends on, and when to stop and report a clash with upstream. Checkpoint recovery inrelease.mdlinks to it.All eight source commits belong to the incubator carry group.
Proof
691d5226dd5: with the router disabled, ArrowUp with the timeline focused and the pointer over the sidebar scrolled the conversation (420→340). Enabled, the same steps scroll the sidebar (300→220) and the conversation stays at 420.apps/web/src/lastcodeScrollKeys.browser.test.tshas one case per row of the behaviour table. Each review fix's assertion was mutation-checked to fail without the fix.6b005e0c486againstc5dd72b8a02, with a matching validation receipt.check:changed origin/lastcode/mainpasses format, lint, both knip checks, affected typechecks, all 90 tests across 9 files, and all 7 React compiler checks.git range-diffconfirms all eight scrolling patches are unchanged by the rebase.Local review: 3 rounds with gpt-6.1-terra (xhigh), not clean; churn-gate MEDIUM (converging, no cuts). Every finding from the final round is fixed.
Done by Claude Opus 5.5 in Claude Code (T3 Code).
🤖 Generated with Claude Code