feat(app): desktop composer UX — boxed surface, inline selectors, focus & scrollbar fixes - #59
Merged
Merged
Conversation
Redesign the chat composer into a taller multiline input (auto-grows to 10 lines) with a footer toolbar: model + thinking-effort selectors on the left, add/send on the right. Desktop always shows the full form; mobile keeps its one-line compact state when unfocused and expands to the desktop form on focus. The selectors reuse the existing /model and /thinking flows. The model selector renders nothing until session.meta arrives with a non-empty models list; the thinking selector is hidden when the agent reports no thinking support. Model/Thinking are removed from the desktop header overflow menu (now Rename / Quit only) and the read-only model-thinking line is dropped from the mobile session header, since both are now editable inline in the composer.
Wrap the footer selectors in an Expanded row of Flexible children so long model/thinking labels ellipsize instead of triggering a RenderFlex overflow on narrow (phone) widths. Add tests for the compact-hidden footer and the narrow-width overflow regression (CodeRabbit review).
Replace the bare-Text narrow-width test with one using the actual ComposerModelSelector/ComposerThinkingSelector and a long model name, asserting the label is single-line ellipsized (not just that Flexible wrapping avoids overflow). Moved to composer_selectors_test.dart where Riverpod is wired (CodeRabbit review).
PR #56 switched every worktree row to the call_merge symbol. Restore the prior fork_right (branch) icon for worktrees without an open PR, keeping call_merge (accent) only where a PR is open.
session.meta (model + thinking + selectable models) stopped being emitted when pi-mirroring was removed in #26, so the app's model/thinking selectors had no data. Re-source it from pi's rpc protocol: query get_state + get_available_models at boot and emit a merged session.meta; map the composer's model/thinking actions to set_model/set_thinking_level and re-query on success. Note: ACP (0.26) exposes only session *modes*, not a model list or thinking level, so ACP agents (codex) can't feed these selectors — tracked separately.
ACP agents (e.g. codex) have no model/thinking concept — only session modes (ask/code/architect). Surface them as a third composer selector: - server (acp.ts): capture newSession modes, emit them via session.meta, keep them in sync on current_mode_update, and map the 'mode' action to ACP's session/set_session_mode. - app: extend SessionMeta with a modes state; add ComposerModeSelector (hidden unless modes exist) wired into the desktop + mobile composer footers. Native pi sessions carry no modes (selector hidden); ACP sessions carry no model/thinking (those selectors hidden). Tests added on both sides.
Deduplicate the repeated {provider,id,name} shape into a shared MetaModel alias (CodeRabbit nitpick).
Wrap the mode bottom-sheet Column in a SingleChildScrollView so a long list of agent modes (or large text scaling) stays reachable (CodeRabbit review).
The idle-focus reclaim treated any node inside the scope as idle, so focusing the composer TextField (a descendant of the scope) immediately yanked focus back to the fallback node, making the chat input impossible to type into. Restrict the descendant check to empty FocusScopeNodes so real focusable leaves keep their focus. Adds a headless regression test plus a macOS e2e proving the input is hit-testable and accepts text.
Group the input, model/thinking/mode selectors, attach and send buttons into a single rounded box with one static background (darker than the transcript, no hover shift). Remove the top divider, hide the input scrollbar, and start the expanded field at 3 rows.
Add a global MaterialScrollBehavior that suppresses scrollbars across the desktop app (transcript, sidebar, pickers). Content still scrolls via trackpad/wheel.
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
Contributor
📝 WalkthroughWalkthroughThe change adds model, thinking, and mode metadata across ACP and pi adapters, exposes selectors in composer footers, updates desktop composer and menu layouts, and adds coverage for composer behavior, metadata synchronization, focus handling, worktree icons, and desktop menus. ChangesComposer metadata and desktop UI
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ComposerModeSelector
participant StoreController
participant AcpAdapter
participant ACP
ComposerModeSelector->>StoreController: Send mode action
StoreController->>AcpAdapter: Dispatch mode selection
AcpAdapter->>ACP: setSessionMode
ACP-->>AcpAdapter: current_mode_update
AcpAdapter-->>ComposerModeSelector: Emit updated session.meta
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
app/test/desktop/desktop_sidebar_test.dart (1)
206-242: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover non-open PRs in this regression test.
This test distinguishes
state: 'OPEN'frompr == null, so it would still pass if a closed or merged PR rendered the merge icon. Add a closed/merged fixture and assert that it does not produceSymbols.call_merge.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/test/desktop/desktop_sidebar_test.dart` around lines 206 - 242, The regression test around “worktree icon: merge symbol only when a PR is open” currently covers only an open PR and a missing PR. Add a worktree fixture with a closed or merged PullRequest, include its session in the test setup, and update the icon assertions so only the open PR produces Symbols.call_merge while the closed/merged PR uses the non-merge icon.app/lib/ui/composer/composer.dart (1)
141-145: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider theme tokens instead of hardcoded hex for
boxColor.
0xFF0E0E0E/0xFFEFEFEFare magic constants disconnected from the rest of the theme-driven styling in this file. If the app's color scheme changes (e.g. dynamic/Material You theming), this box won't track it.Please check whether
makitDarkTheme/makitLightThemealready define an equivalent surface-container token that could replace these literals.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/lib/ui/composer/composer.dart` around lines 141 - 145, Update the boxColor calculation to use the existing surface-container color token from makitDarkTheme or makitLightTheme instead of hardcoded hex values, while preserving Colors.transparent when widget.glass is true and selecting the appropriate theme value for the current brightness.server/src/adapters/acp.test.ts (1)
80-130: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider adding coverage for the agent-initiated
current_mode_updatepath.This test only covers
captureModes(vianewSession) and the client-initiatedmodeaction. It doesn't exerciseagentRef.update(...)sending acurrent_mode_updatenotification (the path handled inacp.ts'sbuildClient().sessionUpdate), which is exactly the branch with thecurrentModeId/modeIdfield-name bug flagged inacp.ts. A test that sends{ sessionUpdate: "current_mode_update", modeId: "..." }and asserts the re-emittedsession.meta.modes.currentwould have caught it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/adapters/acp.test.ts` around lines 80 - 130, Extend the test around ScriptedAgent and session.meta to cover an agent-initiated current_mode_update notification. Send an update through agentRef.update with sessionUpdate set to current_mode_update and a modeId, then wait for the emitted session.meta and assert modes.current matches that modeId, exercising buildClient().sessionUpdate.server/src/adapters/pi.ts (1)
441-450: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
set_model/set_thinking_levelresponse handling is untested.
normalizeModel(evt.data)on line 444 assumes theset_modelresponse payload is the model object directly (matching thedocs/rpc.mdcomment), but pi.test.ts only exercisesget_state/get_available_modelsresponses, not these two branches. If the actual response shape differs,normalizeModeljust returnsnulland the code falls through torequestMeta()— low risk since the followingget_statere-query is the well-tested path — but a quick test would remove the ambiguity.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/adapters/pi.ts` around lines 441 - 450, The response branches for set_model and set_thinking_level in the pi adapter lack test coverage. Add focused tests that simulate successful responses for both commands, verify set_model normalizes and adopts the returned model via metaModel, and verify both branches call requestMeta to re-query state; use the existing pi.test.ts response fixtures and assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@app/lib/ui/composer/composer.dart`:
- Around line 141-145: Update the boxColor calculation to use the existing
surface-container color token from makitDarkTheme or makitLightTheme instead of
hardcoded hex values, while preserving Colors.transparent when widget.glass is
true and selecting the appropriate theme value for the current brightness.
In `@app/test/desktop/desktop_sidebar_test.dart`:
- Around line 206-242: The regression test around “worktree icon: merge symbol
only when a PR is open” currently covers only an open PR and a missing PR. Add a
worktree fixture with a closed or merged PullRequest, include its session in the
test setup, and update the icon assertions so only the open PR produces
Symbols.call_merge while the closed/merged PR uses the non-merge icon.
In `@server/src/adapters/acp.test.ts`:
- Around line 80-130: Extend the test around ScriptedAgent and session.meta to
cover an agent-initiated current_mode_update notification. Send an update
through agentRef.update with sessionUpdate set to current_mode_update and a
modeId, then wait for the emitted session.meta and assert modes.current matches
that modeId, exercising buildClient().sessionUpdate.
In `@server/src/adapters/pi.ts`:
- Around line 441-450: The response branches for set_model and
set_thinking_level in the pi adapter lack test coverage. Add focused tests that
simulate successful responses for both commands, verify set_model normalizes and
adopts the returned model via metaModel, and verify both branches call
requestMeta to re-query state; use the existing pi.test.ts response fixtures and
assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5f1f96a5-de4e-42fa-ad56-fcf1663d8e1c
📒 Files selected for processing (19)
app/integration_test/desktop/composer_e2e_test.dartapp/lib/desktop/chat/desktop_chat_pane.dartapp/lib/desktop/chat/desktop_sidebar.dartapp/lib/desktop/chat/keymap_scope.dartapp/lib/desktop/desktop_app.dartapp/lib/store/models.dartapp/lib/ui/composer/composer.dartapp/lib/ui/composer/composer_selectors.dartapp/lib/ui/session/session_screen.dartapp/test/composer_selectors_test.dartapp/test/composer_test.dartapp/test/desktop/desktop_chat_pane_menu_test.dartapp/test/desktop/desktop_chat_pane_test.dartapp/test/desktop/desktop_sidebar_test.dartapp/test/desktop/keymap_scope_test.dartserver/src/adapters/acp.test.tsserver/src/adapters/acp.tsserver/src/adapters/pi.test.tsserver/src/adapters/pi.ts
The sidebar keyed the merge icon on pr != null, so a closed/merged PR would still render call_merge despite the intent. Gate it on the PR state being OPEN and cover a merged-PR fixture in the regression test.
Replace the hardcoded composer box hex with named kComposerBoxLight / kComposerBoxDark constants in theme.dart, alongside the rest of the palette.
…ches Add coverage for the agent-initiated current_mode_update notification (re-emits session.meta) and for pi's set_model/set_thinking_level responses adopting the model and re-querying state.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/src/adapters/pi.test.ts (1)
51-60: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winWait for boot commands instead of sleeping.
The 150ms delay is scheduler-dependent. Delayed boot writes can make these tests flaky or contaminate assertions after
writes.length = 0. Poll for both boot command types before asserting or resetting writes.
server/src/adapters/pi.test.ts#L51-L60: wait untilget_stateandget_available_modelsappear.server/src/adapters/pi.test.ts#L116-L124: wait for the same boot commands before clearingwrites.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/adapters/pi.test.ts` around lines 51 - 60, Replace the scheduler-dependent delay in the boot assertions with polling that waits until both get_state and get_available_models commands appear before asserting. Apply the same boot-command wait in server/src/adapters/pi.test.ts lines 51-60 and lines 116-124, ensuring the second site waits before clearing writes so delayed boot commands cannot contaminate later assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@server/src/adapters/pi.test.ts`:
- Around line 51-60: Replace the scheduler-dependent delay in the boot
assertions with polling that waits until both get_state and get_available_models
commands appear before asserting. Apply the same boot-command wait in
server/src/adapters/pi.test.ts lines 51-60 and lines 116-124, ensuring the
second site waits before clearing writes so delayed boot commands cannot
contaminate later assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 481237ba-3348-40d8-bef2-2302d3a2fa87
📒 Files selected for processing (6)
app/lib/app/theme.dartapp/lib/desktop/chat/desktop_sidebar.dartapp/lib/ui/composer/composer.dartapp/test/desktop/desktop_sidebar_test.dartserver/src/adapters/acp.test.tsserver/src/adapters/pi.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- app/lib/desktop/chat/desktop_sidebar.dart
- app/test/desktop/desktop_sidebar_test.dart
- app/lib/ui/composer/composer.dart
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reopens the work formerly tracked in #57 (closed when the branch was renamed from `chore/snapshot-v004` to `feat/desktop-composer-ux`).
What changed
TextField, so the chat input is clickable/typeable again. Regression + macOS e2e tests added.Testing
Summary by CodeRabbit