Skip to content

Home dock Channels: type-ahead, page commands and focus target find the Channels page - #33869

Merged
lawrencecchen merged 3 commits into
feat-cmux-nextfrom
fix/home-dock-channels-readers
Nov 3, 2026
Merged

lawrencecchen merged 3 commits into
feat-cmux-nextfrom
fix/home-dock-channels-readers

Conversation

@lawrencecchen

@lawrencecchen lawrencecchen commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Follow-up of #29386 (its review, note 3). The docked Home's Channels page is a page inside the dock's view (HomeDockView, a PageWebViewContainer), not the tab's content itself. #29386 taught TabContent.childWindowTab to find it through PageWebView.shown(in:). Two other page readers still looked for content as? PageWebView only:

  • KeyRouter.focusedPage: the key router named no focused page for Channels.
  • InternalPageView.focusTarget: on a WebKit page (a fallback, or a Chromium page with no session yet), the dock view, which takes no keys, became the first responder.

Both now use PageWebView.shown(in:): the content itself when it is a page, else the page that a container shows now (Channels while Channels shows). Messages mode is unchanged: the dock shows no page there, and its primary input still comes first in focusTarget.

The type-ahead (TabContent.inputReadiness) stays on the old rule on purpose. The review of 181170d found that the Channels page focuses no input when it opens, so a type-ahead hold would eat its own bare keys (j, k, r, t, c, e) until its 2 s limit. The standalone Channels tab (content is the page itself) has that hold today; this PR does not change it.

Testing

  • Red first: HomeDockChannelsPageReadersTests (CmuxNextAppTests), commit a33a4f6. Fleet step de1f3ed1f96753a127a810dc failed on its focus-target assertion (focusTarget → the dock). (Its inputReadiness assertion was the wrong rule; the head replaces it with a guard that the type-ahead gets no readiness for a container's page.)
  • Green at 181170d: step 1c75dbbabce3f97f63f64ead passed. Green at the head 0446888: step cd0a8762dd5366479fbdbbaa (HomeDockChannelsPageReadersTests, HomeTypeToFocusTests, InternalPageParkingTests): all three passed, exit 0.
  • Compile of the head: job 6cb3db8f29f6eb205f2396f0 (CmuxNext with tests, app, cmux-tui) passed.

Observed / Not run

Observed: the red and green steps above, and the compile.
Not run: a journey that types into Channels at its first open (debug.key sequence mode has no event timestamp on the capture minis; STATUS 1582 notes). The test uses a test container, not the real HomeDockView; HomeDockView.shownPageView's mode gate and the KeyRouter.focusedPage change have no test of their own (both rest on code reading and the review).

Changelog

Fixed: Home's Channels view takes the keyboard when Chromium cannot show it and it reopens in WebKit

Proof

No capture: no visible change; the change is which view takes the keys.

…page through its container

HomeDockChannelsPageReadersTests: TabContent.inputReadiness and InternalPageView.focusTarget read only
'content as? PageWebView', so for the docked Home in Channels mode (the page inside HomeDockView, a
PageWebViewContainer) the type-ahead has no input readiness and the focus target is the dock view.
#29386 review note 3. Fails here.
…s focus target find the Home dock's Channels page

#29386 review note 3. TabContent.inputReadiness, KeyRouter.focusedPage and InternalPageView.focusTarget
now find a tab's page through PageWebView.shown(in:), the rule TabContent.childWindowTab already uses:
the content itself when it is a page, else the page a PageWebViewContainer shows (the docked Home's
Channels page while Channels shows). Before, for Channels: no type-ahead (keys typed at its first open
were not held for the page), no page commands or page context from the key router, and on a WebKit
fallback the dock view (which takes no keys) became first responder. Messages mode is unchanged: the
dock shows no page there, and its primary input still comes first in focusTarget.
@lawrencecchen lawrencecchen added the risk:normal Risk tier (docs/ci/risk-tiers.md) label Nov 3, 2026
@github-actions

github-actions Bot commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

cmux status (one comment, edited in place; each line is updated by the job that owns it)

Review 0446888: HQ feed card · gallery before/after runs · checks

App of this head (app.zip, kept 1 day)

gh workflow run reload-build.yml -R manaflow-ai/cmux --ref feat-cmux-next -f tag=pr33869 -f ref=fix/home-dock-channels-readers -f platform=macos -f backend_mode=local

red: cmux-next checks (god files, concurrency, crash safety, l10n); NEW: green on the base 61092fe; do not merge until green; see lane-rules

Shipped in:

@blacksmith-sh

blacksmith-sh Bot commented Nov 3, 2026 •

Copy link
Copy Markdown

Found 10 test failures on Blacksmith runners:

Failures

Test View Logs
/tmp/tmp8haid9fh/summary-missing.json (live):/summary-missing.json (live): View Logs
/tmp/tmpbqlqqyc7/summary-fail.json (live):/summary-fail.json (live): View Logs
[new_workspaces] debug.new_workspace timed 0 of 50 new workspace rows/
[new_workspaces] debug.new_workspace timed 0 of 50 new workspace rows
View Logs
[new_workspaces] the window draws no frames/[new_workspaces] the window draws no frames View Logs
[tabs] debug.tab_switch timed 0 of 40 switches/
[tabs] debug.tab_switch timed 0 of 40 switches
View Logs
[tabs] the window draws no frames/[tabs] the window draws no frames View Logs
test_every_routed_input_triggers_the_workflow (main.PullRequestTiers.test_every_rou
ted_input_triggers_the_workflow) (path='skills/cmux-customization/SKILL.md')
View Logs
test_every_routed_input_triggers_the_workflow (main.PullRequestTiers.test_every_rou
ted_input_triggers_the_workflow) (path='skills/cmux-customization/references/
examples.md')
View Logs
test_every_routed_input_triggers_the_workflow (main.PullRequestTiers.test_every_rou
ted_input_triggers_the_workflow) (path='skills/cmux-customization/references/
local-app-views.md')
View Logs
test_every_routed_input_triggers_the_workflow (main.PullRequestTiers.test_every_rou
ted_input_triggers_the_workflow) (path='skills/cmux-customization/references/
viewer-types.md')
View Logs

Fix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need.

@teamleaderleo teamleaderleo left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review (codex gpt-6-astra high, next-review, since=20261103T135059Z): SHIP at 181170d

SHIP at 181170d

Visual verdict: no images; Leo tries the build

Checked the changed readers against dock mode switching, type-ahead delivery, and page-command routing; no correctness findings. macOS tests were not rerun in this box.

Follow-ups: none

@lawrencecchen

Copy link
Copy Markdown
Contributor Author

Review (claude-opus-5.5-review-subagent): FIX at 181170d
reviewer-session: a86b4741-3421-42e3-9576-6a5a640cc7f7
author-session: abd34adf46f857456
perf claim: none

Read-only review of the diff 044672e..181170d, plus the readers' callers at the head. I did not run a build or a test on this machine. I read the log of fleet step 1c75dbbabce3f97f63f64ead: it is done, exit 0, aws-m4pro-8, and its 3 suites passed (HomeDockChannelsPageReadersTests, HomeTypeToFocusTests, InternalPageParkingTests).

Finding 1 (FIX, correctness): on the docked Channels view, the type-ahead now eats the page's own single-key shortcuts until the person first focuses a text field

  • TabContent.inputReadiness (TabContent.swift:42) now returns the Channels page's PageInputReadiness when the dock's mode is .channels.

  • KeyRouter.interceptKeyDown runs typesAhead for every bare key (KeyRouter.swift:225-226). With a readiness value, typesAhead (KeyRouter+TypeAhead.swift:73-78) sets pageInputPending = !readiness.isReady. decide then returns .typeAhead for each printable key on a .page focus (KeyRouter.swift:128). The router consumes the key (decided.add, return true), and CmuxApplication.sendEvent drops it (CmuxApplication.swift:133). The Chromium page window counts as .content (KeyRouter+Hooks.swift:116-131), so this also happens while Channels' page window has the keys.

  • isReady becomes true only the first time an editable element in the document gets focus (PageInputReadiness.swift, the report script: if (!sent && editable(document.activeElement))). The home-channels page does not focus its composer when it opens or when it gets focus:

    • It does not use contentFocus (webviews/src/pages/shared/pageFocus.ts; only pages/editor uses it).
    • focusComposer runs only from the reply key (HomeChannelsPage.tsx:256-258, 336-343).
    • The comment at HomeChannelsPage.tsx:241-242 says on purpose that the first render never moves focus.
    • HomeDockView.focusContent(.channels) calls focusPage() only (HomeDockView.swift:76-80; PageWebView.swift:541-547), and that focuses no element.
  • The page has bare keys that work outside a text field: j/k step, r reply, t thread, c copy link, e resolve (webviews/src/pages/home-channels/useHomeKeys.ts:50-54). The page is not a bare-key owner (ActionContext.bareKeyOwners is diff/markdown only), so routesBareKey does not protect these keys.

  • Result: open Channels from the sidebar and press j, k or r. Before this PR the page got the key, because inputReadiness was nil for the dock. Now the router holds the key for up to 2 s (TypeAheadQueue) and then drops it. The held keys reach the page only if something focuses an editable within those 2 s. Nothing does, because r, the key that would focus the composer, is itself held. The PR's premise ("keys typed at Channels' first open were not held for the page") assumes that the page focuses a primary input when it opens. home-channels does not.

  • This defect already exists for the standalone Channels page tab (HomeChannelsPageTab.swift: there the content is the PageWebView itself). This PR moves it into the dock, which is the sidebar's Channels entry.

  • Possible fixes (the author decides):

    1. Make home-channels claim focus like the editor page: contentFocus on focusContent / window focus, which focuses the main composer when nothing has focus. This also fixes the standalone tab. Do not use it if focusing the composer would change j/k behaviour by design.
    2. Hold type-ahead only for pages that declare an input they focus on open, for example a PageDescriptor flag, so a page with no such input never gets keys queued.
    3. Keep inputReadiness off for Channels and land only the focusedPage / focusTarget changes.

    Whatever the fix, add a regression test: a bare j on a Channels page whose readiness is not ready must reach the page (decide must not return .typeAhead for it).

Other callers checked (no defect found)

  • inputReadiness also feeds facts.listFocus and pageEditableFocused (KeyRouter+Context.swift:146-147, 183-186). On Channels, the composer now sets textInputFocus and a focused list sets listFocus. This is the same behaviour as the standalone Channels tab. In Messages mode, shownPageView is nil (HomeDockView.swift:19), so nothing changes there.
  • focusedPage callers:
    • FocusEffectApplier.publish (FocusEffectApplier.swift:412) and keyContext pageContext: the id cmux.home-channels matches none of the diff, markdown or editor ids, so no context bits are added. The editor checks (codeEditorPageID, Context.swift:138) do not apply.
    • routesBareKey (BareKeys.swift:29): not an owner.
    • PageCommandHandlers (markdown/diff commands; the Channels descriptor commands are PageNativeOp.commands + quote, with no overlap except find/back/forward, which the markdown/diff action ids gate behind their own context).
    • find (TerminalHandlers+Input.swift:20): ownsSearchField is false for Channels, so the path is unchanged.
    • FilePageService.showsReadOnly: the Channels page is not a file tab, so false.
    • pageReportsUnhandledKeys (chromiumPageHasKeys): guarded on .browserPage, unchanged.
  • focusTarget: FocusEffectApplier .page (FocusEffectApplier.swift:136-144) returns through childWindowTab for a Chromium Channels page first. Then landKeyboardInPrimaryInput runs, and it is false in Channels mode because HomeDockView.primaryInputView is nil (HomeDockView+PrimaryInputTarget.swift:12). Only after these does it read focusTarget, so only a WebKit fallback (or a Chromium page that has no session yet) changes: it gets the page's focusView and no longer the dock view. In Messages mode, primaryInputView comes first and is unchanged. Switching back to Messages calls messages?.focusPrimaryInput() (HomeDockView.swift:77), and that moves the responder, so the change does not block it. The other focusTarget readers (TopPageHost, TopHomePageView, ResourceControl) do not host the dock.
  • HomeTypeToFocus, New Tab and agent paths: .agent readiness is unchanged. primaryInput (PrimaryInputTarget.swift:101-104) still reads page.content, so the dock's Messages typing path is unchanged.

Finding 2 (minor, PR body)

The Testing section still says "step 1c75dbbabce3f97f63f64ead (...): RESULT." Replace RESULT with the result: passed at 181170d, exit 0, 3 suites passed.

Finding 3 (test scope, acceptable)

The stand-in Dock container is acceptable for the reader rule, because HomeDockView needs AppServices and the readers depend only on PageWebViewContainer. Two gaps remain:

  • No test covers HomeDockView.shownPageView's mode gate (mode == .channels). The Messages-mode result therefore rests on reading only. The body says this.
  • No test covers the KeyRouter.focusedPage change, which is the third reader.
    Neither gap blocks the PR alone. The regression in Finding 1 needs its own test.

Review of #33869 (FIX at 181170d): TabContent.inputReadiness through PageWebView.shown(in:) gave the
docked Channels page a type-ahead hold. That page focuses no input when it opens, so its readiness stays
not ready and the hold ate its own bare keys (j, k, r, t, c, e) until the 2 s limit. inputReadiness reads
a page tab's own page again; the test now guards that a page a container shows gets no readiness.
KeyRouter.focusedPage and InternalPageView.focusTarget keep the shown(in:) rule. The standalone Channels
tab (its content is the page) has the same hold on bare keys today; that is not changed here.
@lawrencecchen

Copy link
Copy Markdown
Contributor Author

Review (claude-opus-5.5-review-subagent): SHIP at 0446888
reviewer-session: a86b4741-3421-42e3-9576-6a5a640cc7f7
author-session: abd34adf46f857456
perf claim: none

Read-only review of the delta 181170d..0446888 (one commit) and of the new PR body. I did not run a build or a test on this machine. I read the fleet results with cmux-ci status and cmux-ci log: green step cd0a8762dd5366479fbdbbaa is done at 0446888, exit 0, "All 3 suites passed" (HomeDockChannelsPageReadersTests, HomeTypeToFocusTests, InternalPageParkingTests). Compile job 6cb3db8f29f6eb205f2396f0 is done at 0446888, and its log shows "Build complete!".

Finding 1 of the previous review: closed

  • TabContent.inputReadiness (TabContent.swift:42) reads view.content as? PageWebView again, so the docked Channels page gets no readiness.
  • typesAhead (KeyRouter+TypeAhead.swift:73-74) returns false at its guard let readiness, so bare j/k/r/t/c/e reach the page as they did before the PR.
  • focusedReadiness (KeyRouter+Context.swift:183-186) is nil for the dock again, so the listFocus and pageEditableFocused facts for docked Channels are the same as on the base.
  • The new guard theTypeAheadDoesNotHoldKeysForThePageTheDockShows fails if someone routes a container's page into inputReadiness again. Because the type-ahead path needs a non-nil readiness, I agree that a separate test at the decide level is not necessary.

Remaining changes: no defect found

  • KeyRouter.focusedPage uses shown(in:). The id cmux.home-channels sets no context bits (pageContext, FocusEffectApplier.swift:412). It is not a bare-key owner. ownsSearchField is false, so find behaves as before. The markdown and diff page actions need markdownFocused or diffViewerFocused, which Channels does not set.
  • InternalPageView.focusTarget uses shown(in:). Only the WebKit fallback, or a Chromium page with no session yet, changes: the page's focusView gets the keyboard instead of the dock view. In Messages mode, primaryInputView comes first and shownPageView is nil, so nothing changes there. A switch back to Messages still moves the keyboard through focusPrimaryInput.

PR body

  • It is correct and complete. It names the two readers that changed and says why the type-ahead stays on the old rule.
  • It says that the standalone Channels tab keeps its old hold. That is still a defect, but it is not in this PR's scope; I suggest a follow-up, for example making home-channels focus its composer when it gets the keyboard.
  • It lists the red, green and compile results, and the "Observed / Not run" section names the gaps in test coverage.

@lawrencecchen

Copy link
Copy Markdown
Contributor Author

Checks at 0446888 before ready-to-land: 69 pass, 1 pending, 1 red. The red is a base finding, not this PR's: cmux-next checks (god files, concurrency, crash safety, l10n) fails its "Tier routing" step (tests/test_cmux_next_route.py: four skills/cmux-customization/ files miss the pull_request paths filter). This PR touches none of those files. The same step is red on unrelated PRs #33920 and #33915, and #33917 (merged 14:06Z, base red from #33848) fixed it on feat-cmux-next after this branch was cut. Receipt: #33869 (comment).

@lawrencecchen lawrencecchen added the ready-to-land Reviewed at this head; next-autoland merges it into feat-cmux-next when its only reds are base reds label Nov 3, 2026
@lawrencecchen
lawrencecchen merged commit 8e2e21b into feat-cmux-next Nov 3, 2026
213 of 217 checks passed
@lawrencecchen
lawrencecchen deleted the fix/home-dock-channels-readers branch November 3, 2026 14:44

This branch was previously deployed

1 inactive deployment
ci-routing-agent — 04468884 Deployed Nov 3, 2026 by lawrencecchen via cmux-tui tree preflight #106348
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-to-land Reviewed at this head; next-autoland merges it into feat-cmux-next when its only reds are base reds risk:normal Risk tier (docs/ci/risk-tiers.md)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants