feat(web): refine model and workbench controls - #5
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe web workbench adds shared sidebar and inspector toggles, removes focus-mode state, restructures layout behavior, and refactors ModelPicker into separate model and variant popovers. Route headers and interaction tests are updated, alongside a synchronous Git runner for an agent-core integration test. ChangesWeb workbench controls and selection
Agent worktree integration test
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
apps/web/src/components/features/ModelPicker.tsx (1)
42-64: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Home/Endare hijacked while typing in the search field.
movePickerFocusis bound on the popover, so keystrokes from the search input bubble into it.Home/EndcallpreventDefault()and move focus to the first/last option, breaking caret navigation in the query. Consider restrictingHome/Endhandling to[data-picker-option]targets.♻️ Proposed guard
function movePickerFocus( event: React.KeyboardEvent<HTMLElement>, root: HTMLDivElement | null, ): void { if (!["ArrowDown", "ArrowUp", "Home", "End"].includes(event.key)) return; + const fromOption = (event.target as HTMLElement)?.hasAttribute?.("data-picker-option") ?? false; + if (!fromOption && (event.key === "Home" || event.key === "End")) return; const options = Array.from(🤖 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 `@apps/web/src/components/features/ModelPicker.tsx` around lines 42 - 64, Update movePickerFocus so Home and End are handled only when event.target is a [data-picker-option] element; allow those keys to bubble normally from the search input without preventDefault or focus movement, while preserving existing option navigation behavior.apps/web/src/components/features/ModelPicker.test.tsx (2)
406-419: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winArrow navigation is only covered for the variant menu.
The model popover is the one with the search input and the
Home/Endinteraction, and Escape's focus restoration to the originating trigger is untested. Adding a case that opens the model picker on a large catalog and arrows down from the search field would cover the riskier path.🤖 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 `@apps/web/src/components/features/ModelPicker.test.tsx` around lines 406 - 419, Extend the ModelPicker keyboard-navigation tests with a large-catalog case that opens the model popover, focuses its search input, and verifies ArrowDown moves focus to the expected model option. Locate the existing renderPicker/openVariantPicker helpers and model-popover/search-input selectors, and preserve the current variant-menu test while covering Escape focus restoration if the surrounding test setup supports it.
120-136: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSimplify
changeSearchto the public input path. This is already a controlled<input type="search">; setting the native value and dispatching a bubblinginputevent should driveonChange, so the_valueTracker/__reactProps$…fallback can be removed.🤖 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 `@apps/web/src/components/features/ModelPicker.test.tsx` around lines 120 - 136, Simplify the changeSearch helper to set the search input’s native value and dispatch a bubbling input event through the public DOM path. Remove the _valueTracker manipulation, React __reactProps$ lookup, direct onChange invocation, and related fallback logic while preserving the existing input lookup and act wrapper.
🤖 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.
Inline comments:
In `@packages/agent-core/src/worktrees/service-managed-claim.integration.test.ts`:
- Around line 68-96: Update the sync runner’s Bun.spawnSync invocation to pass
input.maxOutputBytes through the supported maxBuffer option, ensuring stdout and
stderr are capped consistently with the normal runner. Derive the output and
truncation metadata from the capped result so the byte counts and truncation
flags accurately reflect whether either stream exceeded the configured limit.
---
Nitpick comments:
In `@apps/web/src/components/features/ModelPicker.test.tsx`:
- Around line 406-419: Extend the ModelPicker keyboard-navigation tests with a
large-catalog case that opens the model popover, focuses its search input, and
verifies ArrowDown moves focus to the expected model option. Locate the existing
renderPicker/openVariantPicker helpers and model-popover/search-input selectors,
and preserve the current variant-menu test while covering Escape focus
restoration if the surrounding test setup supports it.
- Around line 120-136: Simplify the changeSearch helper to set the search
input’s native value and dispatch a bubbling input event through the public DOM
path. Remove the _valueTracker manipulation, React __reactProps$ lookup, direct
onChange invocation, and related fallback logic while preserving the existing
input lookup and act wrapper.
In `@apps/web/src/components/features/ModelPicker.tsx`:
- Around line 42-64: Update movePickerFocus so Home and End are handled only
when event.target is a [data-picker-option] element; allow those keys to bubble
normally from the search input without preventDefault or focus movement, while
preserving existing option navigation behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2712dd7e-4efe-484f-89f9-09e4803f17c6
📒 Files selected for processing (31)
apps/web/package.jsonapps/web/src/components/features/ChatHeader.test.tsxapps/web/src/components/features/ChatHeader.tsxapps/web/src/components/features/ChatInput.tsxapps/web/src/components/features/ContextInspector.tsxapps/web/src/components/features/InspectorToggleButton.interaction.tsxapps/web/src/components/features/InspectorToggleButton.tsxapps/web/src/components/features/ModelPicker.test.tsxapps/web/src/components/features/ModelPicker.tsxapps/web/src/components/features/PanelToggleButton.tsxapps/web/src/components/features/Sidebar.interaction.tsxapps/web/src/components/features/Sidebar.tsxapps/web/src/components/features/SidebarToggleButton.interaction.tsxapps/web/src/components/features/SidebarToggleButton.tsxapps/web/src/components/features/compact-control-surface.test.tsapps/web/src/context/workbench-layout.interaction.tsxapps/web/src/context/workbench-layout.tsxapps/web/src/lib/workbench-layout.test.tsapps/web/src/lib/workbench-layout.tsapps/web/src/routes/automation-detail.tsxapps/web/src/routes/automations.tsxapps/web/src/routes/dashboard.test.tsxapps/web/src/routes/dashboard.tsxapps/web/src/routes/project-todos.interaction.tsxapps/web/src/routes/project-todos.test.tsxapps/web/src/routes/project-todos.tsxapps/web/src/routes/root-layout.interaction.tsxapps/web/src/routes/root-layout.tsxapps/web/src/routes/session.test.tsxapps/web/src/routes/session.tsxpackages/agent-core/src/worktrees/service-managed-claim.integration.test.ts
💤 Files with no reviewable changes (5)
- apps/web/src/components/features/Sidebar.interaction.tsx
- apps/web/src/lib/workbench-layout.test.ts
- apps/web/src/components/features/ChatInput.tsx
- apps/web/src/context/workbench-layout.interaction.tsx
- apps/web/src/lib/workbench-layout.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 31 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
Why
The combined model picker made model identity and option variants harder to scan and change independently. Panel controls also moved with panel content instead of remaining available from stable page headers. The managed-claim test exercised the intended Git lifecycle but could fail in Bun's asynchronous piped-spawn path rather than on product behavior.
This keeps each control aligned with the state it owns and preserves the real-Git integration coverage with a deterministic process adapter scoped to the test.
Validation
bun run typecheckbun run testbun run buildwhen the production package or Web UI is affectedgit diff --check origin/main...HEADDocumentation and security
Related issue
None.
Summary by CodeRabbit