Refine Work session lifecycle and sidebar - #973
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
Warning Review limit reached
Next review available in: 35 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (21)
📝 WalkthroughWalkthroughChangesSession lifecycle and authorization
Command palette threads
Desktop work surface
iOS presentation
Estimated code review effort: 5 (Critical) | ~90+ minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/desktop/src/main/services/lanes/laneService.ts (1)
2258-2291: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winEnforce the reserved primary color in
updateAppearance.This backfill/insert makes Primary purple, but
updateAppearancecan later set its color to any value. Reject or normalize primary-lane color updates so the stated cross-surface invariant persists.Proposed fix
const normalizedColor = color === undefined ? lane.color : color; +if ( + lane.lane_type === "primary" + && normalizedColor !== PRIMARY_LANE_COLOR +) { + throw new Error("Primary lane color is fixed."); +}🤖 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/desktop/src/main/services/lanes/laneService.ts` around lines 2258 - 2291, Update updateAppearance to enforce PRIMARY_LANE_COLOR whenever the target lane is a primary lane: reject requested color changes or normalize them to the reserved color before persisting. Preserve the existing appearance-update behavior for non-primary lanes and ensure any primary update cannot store another color.Source: Coding guidelines
apps/ade-cli/src/cli.ts (1)
6959-7014: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
chat settle/chat unsettlefall through to a generic, unhelpful error instead of the documented guidance.Unlike
buildSessionPlan, which explicitly rejects removed subcommands with a clearCliUsageError(Line 6931:`Unknown session subcommand '${sub}'. Try: show, snooze, wake, clear-woke.`),buildChatPlanno longer has any case for"settle"/"unsettle". They fall through to the generic catch-all at Lines 7847-7851 and get dispatched as domain"chat", action"settle"/"unsettle"— actions that don't exist (settlement now lives only under the"session"domain). Users get an opaque "unknown action" failure instead of the help text you just wrote at Lines 1715-1717 ("'chat settle' / 'chat unsettle' were removed: ... report your outcome with 'chat note'").🐛 Proposed fix: explicit rejection mirroring buildSessionPlan
+ if (sub === "settle" || sub === "unsettle") { + throw new CliUsageError( + "'chat settle' / 'chat unsettle' were removed: only the user (or a merged PR) settles a session — report your outcome with 'chat note'.", + ); + } if (sub === "list" || sub === "ls") {🤖 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/ade-cli/src/cli.ts` around lines 6959 - 7014, Update buildChatPlan to explicitly reject the removed "settle" and "unsettle" subcommands with a CliUsageError containing the documented guidance to use "chat note". Place this handling before the generic chat action fallback, while preserving existing behavior for all supported subcommands.
🧹 Nitpick comments (10)
apps/desktop/src/shared/laneColorPalette.ts (1)
54-55: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNormalize both sides of the reserved-colour comparison.
The filter only excludes purple because
PRIMARY_LANE_COLORhappens to be written lowercase; changing the constant's casing would silently return purple to the allocation pool and break the "purple always means Primary" invariant this module documents.🛡️ Proposed hardening
-export const ALLOCATABLE_LANE_COLORS: readonly LaneColor[] = LANE_COLOR_PALETTE - .filter((entry) => entry.hex.toLowerCase() !== PRIMARY_LANE_COLOR); +const PRIMARY_LANE_COLOR_KEY = PRIMARY_LANE_COLOR.toLowerCase(); +export const ALLOCATABLE_LANE_COLORS: readonly LaneColor[] = LANE_COLOR_PALETTE + .filter((entry) => entry.hex.toLowerCase() !== PRIMARY_LANE_COLOR_KEY);🤖 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/desktop/src/shared/laneColorPalette.ts` around lines 54 - 55, Update the filter in ALLOCATABLE_LANE_COLORS to normalize PRIMARY_LANE_COLOR as well as entry.hex before comparing them, preserving the exclusion of the reserved primary color regardless of the constant’s casing.apps/desktop/src/renderer/components/terminals/SessionHoverCard.tsx (1)
215-215: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAlso release the module-level handoff entry on unmount.
Rows in this list unmount constantly (re-sort, filter, collapse). If a row unmounts while it owns
activeHoverCard, the module keeps a closure over the dead hook plus a detachedtriggernode until the next hover.♻️ Proposed cleanup
- React.useEffect(() => clearTimers, [clearTimers]); + React.useEffect(() => () => { + clearTimers(); + if (activeHoverCard?.rowId === rowId) activeHoverCard = null; + }, [clearTimers, rowId]);🤖 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/desktop/src/renderer/components/terminals/SessionHoverCard.tsx` at line 215, Update the cleanup effect in the hover-card hook to release the module-level activeHoverCard handoff when its owning row unmounts, but only if that entry belongs to the current hook/trigger. Preserve the existing clearTimers cleanup and avoid clearing a newer row’s active handoff.apps/desktop/src/renderer/components/terminals/SessionListPane.test.tsx (1)
25-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRecorder accumulates across tests and
cardPropsForreturns the first, not the latest, props.
sessionCardPropsForTestis only truncated inside two describes, and.findcontradicts the doc comment ("most recent render pass") — a re-render in the same test makes the assertion read the stale prop set. Clearing it in the sharedafterEachand usingfindLastremoves the trap without touching any current expectation.♻️ Proposed tightening
function cardPropsFor(sessionId: string): Record<string, unknown> | undefined { - return sessionCardPropsForTest.find( + return sessionCardPropsForTest.findLast( (props) => (props.session as TerminalSessionSummary | undefined)?.id === sessionId, ); }Plus
sessionCardPropsForTest.length = 0;in eachafterEach.🤖 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/desktop/src/renderer/components/terminals/SessionListPane.test.tsx` around lines 25 - 47, Update the shared test cleanup for sessionCardPropsForTest so it is cleared in the common afterEach rather than only within individual describe blocks. Change cardPropsFor to return the last matching session props from the most recent render, using findLast or equivalent, while preserving its existing session-id filtering behavior.apps/desktop/src/renderer/components/terminals/SessionListPane.tsx (2)
961-980: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRe-playing a synthetic
keydownto open the palette is a renderer-only workaround.This depends on AppShell's listener being on
window, in the bubble phase, un-swallowed by anything upstream, and on its matcher accepting a synthesized event — none of which this file can guarantee. Exposing anopenCommandPaletteaction (store or context) alongside AppShell's key handler removes the coupling and keeps the shortcut chip logic here purely cosmetic.As per coding guidelines, "prefer fixing the underlying service or shared type rather than adding renderer-only workarounds".
🤖 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/desktop/src/renderer/components/terminals/SessionListPane.tsx` around lines 961 - 980, Replace the synthetic keydown workaround in openCommandPalette with the shared command-palette open action exposed by AppShell’s existing state, store, or context alongside its keyboard handler. Update the search button to invoke that action directly, and keep commandPaletteBinding only for displaying the shortcut chip rather than parsing or dispatching keyboard events.Source: Coding guidelines
152-196: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the keybinding combo parser for shortcut chips.
parsePrimaryComboinSessionListPane.tsxre-implements the sameMod/platform resolution already inlib/keybindings, and it even still uses deprecatednavigator.platform. Export a small combo resolver fromlib/keybindingsand use it here so chips and matching always come from one source of truth.🤖 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/desktop/src/renderer/components/terminals/SessionListPane.tsx` around lines 152 - 196, Replace the local parsePrimaryCombo implementation in SessionListPane with a small exported combo resolver from lib/keybindings that performs the existing Mod/platform resolution without deprecated navigator.platform usage. Import and reuse that resolver in shortcutChipLabel, preserving the current chip formatting while ensuring display and matching share one source of truth.apps/desktop/src/renderer/components/lanes/laneContextMenuItems.tsx (1)
493-500: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSwatch buttons are non-
menuitemchildren of arole="menu"container.
LaneMenuGroupsrenders this custom node directly inside the menu (and inside theMenuSubmenupanel), so AT sees plain buttons as invalid menu children. Considerrole="menuitemradio"witharia-checkedfor the swatches androle="menuitem"for the clear button.🤖 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/desktop/src/renderer/components/lanes/laneContextMenuItems.tsx` around lines 493 - 500, Update the swatch buttons rendered by LaneMenuGroups to use role="menuitemradio" with aria-checked reflecting isSelected, and update the clear button to use role="menuitem", ensuring all direct children of the role="menu" and MenuSubmenu panel have valid menu-item semantics.apps/desktop/src/renderer/components/lanes/LaneContextMenu.tsx (1)
89-120: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the duplicated entry renderer.
The submenu and inline branches map entries with byte-identical logic; one helper keeps them from drifting.
♻️ Proposed refactor
+function renderEntry(entry: LaneMenuGroup["entries"][number]) { + if (entry.kind === "custom") { + return <React.Fragment key={entry.key}>{entry.node}</React.Fragment>; + } + return ( + <HoverButton + key={entry.key} + style={menuItemStyle} + dataTour={entry.dataTour} + onClick={entry.onSelect} + > + {entry.label} + </HoverButton> + ); +}Then use
{group.entries.map(renderEntry)}in both branches.🤖 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/desktop/src/renderer/components/lanes/LaneContextMenu.tsx` around lines 89 - 120, Extract the duplicated entry-mapping JSX into a shared renderEntry helper near the LaneContextMenu component, preserving the existing custom Fragment and HoverButton behavior, keys, props, and handlers. Replace both group.entries.map callbacks in the submenu and inline branches with group.entries.map(renderEntry).apps/desktop/src/renderer/components/ui/MenuSubmenu.tsx (1)
233-237: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPanel declares
role="menu"but its children are plain<button>s.ARIA requires a
menuto containmenuitem/menuitemcheckbox/menuitemradiochildren; the session menu passes bare buttons (onlyLaneActionsSubmenusetsrole="menuitem"). Screen readers then report a menu with zero items, and the arrow-key model implemented inmoveFocushas no ARIA counterpart. Either apply the role in the panel (wrap children or document the requirement) or droprole="menu"and expose the panel as a plain group.Also,
focusableItemsonly matchesbutton:not([disabled]), so any anchor or input a consumer places in a panel is skipped by arrow navigation.Also applies to: 271-274
🤖 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/desktop/src/renderer/components/ui/MenuSubmenu.tsx` around lines 233 - 237, Update MenuSubmenu’s panel semantics and focus management: either ensure every supported child receives an appropriate menuitem role while preserving the existing menu keyboard model, or remove role="menu" and expose the panel as a plain group. Expand focusableItems beyond enabled buttons so anchors and other supported interactive descendants are included in moveFocus, while still excluding disabled elements.apps/desktop/src/renderer/components/terminals/TerminalsPage.test.tsx (1)
665-702: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStore vs. hook lane/focus mocks are the same function, so these two assertions can't tell the paths apart.
baseWorkspreads...fns, soworkMocks.currentWork.selectLane === workMocks.fns.selectLane(same forfocusSession). Lines 699-700 therefore pass whether the handler usedselectLaneInStore/focusSessionInStoreorwork.selectLane/work.focusSession— which is exactly the distinction this test exists to pin down. Give the store selectors their ownvi.fn()s.♻️ Sketch
const fns = { - selectLane: vi.fn(), - focusSession: vi.fn(), + selectLane: vi.fn(), // hook (work.*) + focusSession: vi.fn(), + storeSelectLane: vi.fn(), // app store + storeFocusSession: vi.fn(),…and wire
selectLane: workMocks.fns.storeSelectLane/focusSession: workMocks.fns.storeFocusSessioninto theuseAppStoreselector object, asserting on those in this test.🤖 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/desktop/src/renderer/components/terminals/TerminalsPage.test.tsx` around lines 665 - 702, Update the TerminalsPage test mocks so store selectors use distinct vi.fn() instances from the hook functions: add dedicated storeSelectLane and storeFocusSession mocks, wire them into the useAppStore selector object, and assert the foreign-session handler calls those store mocks while preserving the existing hook-call assertions.apps/ade-cli/src/tuiClient/sessionLifecycle.ts (1)
2-2: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the direct desktop renderer import for snooze presets.
resolveSnoozePresetsis imported from../../desktop/src/renderer/lib/sessionSnooze, and this already mixes renderer-only code into a CLI target. If the CLI needs these utils, move/reexport them from a shared package or keep an own copy instead of importing throughapps/desktop/src/renderer.🤖 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/ade-cli/src/tuiClient/sessionLifecycle.ts` at line 2, Remove the direct renderer import of resolveSnoozePresets from the session lifecycle module. Provide the CLI with this utility through a shared package or a local CLI-safe implementation, ensuring no dependency on apps/desktop/src/renderer remains.
🤖 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 `@apps/ade-cli/src/adeRpcServer.test.ts`:
- Around line 1584-1591: Extend the denial matrix loop containing settleAttempt
cases to include the CTO-only bulk writer action unsettleSessions, using the
appropriate sessionIds argument for chat-1. Preserve the existing assertions and
coverage for the other settlement actions.
In `@apps/desktop/resources/ade-cli-help.txt`:
- Around line 51-53: Replace “File a session's lifecycle” with “Manage a
session's lifecycle” in all five generated command index entries:
apps/desktop/resources/ade-cli-help.txt lines 51-53, 188-190, 747-749, 947-949,
and 1362-1364.
In `@apps/desktop/src/renderer/components/app/commandPaletteThreads.tsx`:
- Around line 444-460: Remove the aria-hidden attribute from the
ThreadOverflowNote list item so assistive technologies can announce the “Showing
shown of total threads” status. Keep the existing non-focusable li structure,
styling, and conditional rendering unchanged.
- Around line 342-349: Update the contextParts construction near the
secondary-line logic to use the entry’s routing-bound project displayName for
foreign rows instead of the current tab’s projectName. Preserve the existing
project/branch/lane ordering and fallback behavior, while ensuring cross-machine
threads are labeled with their own project identity.
In `@apps/desktop/src/renderer/components/chat/AgentChatPane.tsx`:
- Around line 11786-11791: Update the lifecycleBanner conditional and
ChatLifecycleBanner sessionId prop to use the resolved composerSessionId instead
of selectedSessionId, matching the existing composer control targeting behavior
during chat switches.
In `@apps/desktop/src/renderer/components/chat/ChatSurfaceShell.tsx`:
- Around line 10-11: Move the h-8 height utility out of CHAT_SHELL_HEADER_CLASS
so the shell wrapper retains only horizontal padding and the existing spacing
behavior. Apply h-8 directly to the WorkSurfaceHeader element, while preserving
the wrapper’s space-y-1 (or equivalent) gap when the session tabs row is
rendered.
In `@apps/desktop/src/renderer/components/terminals/LaneActionsSubmenu.tsx`:
- Around line 69-77: Add role="menuitem" to the LaneActionsSubmenu MenuSubmenu
trigger, matching the role passed by LaneContextMenu and preserving the existing
fallback child behavior.
- Around line 53-66: Update useLaneMenuActions and the LaneMenuActions type to
expose an onAppearanceChanged refresh callback, include it in the hook’s
memoization dependencies, and pass it through LaneActionsSubmenu into
buildLaneMenuGroups so the color swatch receives the callback and refreshes
after selection.
In `@apps/desktop/src/renderer/components/terminals/SessionHoverCard.tsx`:
- Around line 204-213: Track the delayed-open state with a pending-open state
value: set it when the open timer is scheduled, and clear it in clearTimers and
open. Update the scroll/resize effect guard to depend on this state so listeners
are armed during the delay, while preserving cancellation for already-open
cards.
- Around line 318-344: Make SessionHoverCard actions keyboard reachable by
adding an assistive-technology-discoverable keyboard path from the session row,
rather than relying only on the hover-triggered tooltip. Update the relevant
session-row keyboard handler to invoke the same action used by row.onActivate,
and ensure the activated control is discoverable without relying on the inner
tabIndex={-1} button or pointer hover.
In `@apps/desktop/src/renderer/components/ui/MenuSubmenu.tsx`:
- Around line 195-221: Update closeNow to restore focus to the submenu trigger
whenever the closing panel contains document.activeElement, including
pointer-timeout closures after keyboard opening. Preserve existing close
behavior and avoid moving focus when the panel does not currently own focus; use
the trigger reference already associated with the submenu.
In `@apps/ios/ADE/Views/AttentionDrawer/AttentionDrawerModel.swift`:
- Around line 636-644: Update the WorkspaceSnapshot fallback classification near
the local snapshot handling to map phase == "blocked" to .blocked instead of
.running, while preserving .stale for disconnected hosts and existing
classifications for other phases. Add a regression test covering a local
WorkspaceSnapshot with a blocked phase and verify it produces the neutral
blocked presentation.
In `@apps/ios/ADEWidgets/ADEAgentActivityWidget.swift`:
- Around line 288-304: Update the headline logic near activeCount and the
primary-agent selection so completed runs are not counted as active work. Derive
the count from genuinely in-flight phases such as starting or running, or
preserve the host-reported active count separately, then ensure the zero-active
path at the headline generation logic produces the completed/result wording
instead of “1 agent working.”
---
Outside diff comments:
In `@apps/ade-cli/src/cli.ts`:
- Around line 6959-7014: Update buildChatPlan to explicitly reject the removed
"settle" and "unsettle" subcommands with a CliUsageError containing the
documented guidance to use "chat note". Place this handling before the generic
chat action fallback, while preserving existing behavior for all supported
subcommands.
In `@apps/desktop/src/main/services/lanes/laneService.ts`:
- Around line 2258-2291: Update updateAppearance to enforce PRIMARY_LANE_COLOR
whenever the target lane is a primary lane: reject requested color changes or
normalize them to the reserved color before persisting. Preserve the existing
appearance-update behavior for non-primary lanes and ensure any primary update
cannot store another color.
---
Nitpick comments:
In `@apps/ade-cli/src/tuiClient/sessionLifecycle.ts`:
- Line 2: Remove the direct renderer import of resolveSnoozePresets from the
session lifecycle module. Provide the CLI with this utility through a shared
package or a local CLI-safe implementation, ensuring no dependency on
apps/desktop/src/renderer remains.
In `@apps/desktop/src/renderer/components/lanes/LaneContextMenu.tsx`:
- Around line 89-120: Extract the duplicated entry-mapping JSX into a shared
renderEntry helper near the LaneContextMenu component, preserving the existing
custom Fragment and HoverButton behavior, keys, props, and handlers. Replace
both group.entries.map callbacks in the submenu and inline branches with
group.entries.map(renderEntry).
In `@apps/desktop/src/renderer/components/lanes/laneContextMenuItems.tsx`:
- Around line 493-500: Update the swatch buttons rendered by LaneMenuGroups to
use role="menuitemradio" with aria-checked reflecting isSelected, and update the
clear button to use role="menuitem", ensuring all direct children of the
role="menu" and MenuSubmenu panel have valid menu-item semantics.
In `@apps/desktop/src/renderer/components/terminals/SessionHoverCard.tsx`:
- Line 215: Update the cleanup effect in the hover-card hook to release the
module-level activeHoverCard handoff when its owning row unmounts, but only if
that entry belongs to the current hook/trigger. Preserve the existing
clearTimers cleanup and avoid clearing a newer row’s active handoff.
In `@apps/desktop/src/renderer/components/terminals/SessionListPane.test.tsx`:
- Around line 25-47: Update the shared test cleanup for sessionCardPropsForTest
so it is cleared in the common afterEach rather than only within individual
describe blocks. Change cardPropsFor to return the last matching session props
from the most recent render, using findLast or equivalent, while preserving its
existing session-id filtering behavior.
In `@apps/desktop/src/renderer/components/terminals/SessionListPane.tsx`:
- Around line 961-980: Replace the synthetic keydown workaround in
openCommandPalette with the shared command-palette open action exposed by
AppShell’s existing state, store, or context alongside its keyboard handler.
Update the search button to invoke that action directly, and keep
commandPaletteBinding only for displaying the shortcut chip rather than parsing
or dispatching keyboard events.
- Around line 152-196: Replace the local parsePrimaryCombo implementation in
SessionListPane with a small exported combo resolver from lib/keybindings that
performs the existing Mod/platform resolution without deprecated
navigator.platform usage. Import and reuse that resolver in shortcutChipLabel,
preserving the current chip formatting while ensuring display and matching share
one source of truth.
In `@apps/desktop/src/renderer/components/terminals/TerminalsPage.test.tsx`:
- Around line 665-702: Update the TerminalsPage test mocks so store selectors
use distinct vi.fn() instances from the hook functions: add dedicated
storeSelectLane and storeFocusSession mocks, wire them into the useAppStore
selector object, and assert the foreign-session handler calls those store mocks
while preserving the existing hook-call assertions.
In `@apps/desktop/src/renderer/components/ui/MenuSubmenu.tsx`:
- Around line 233-237: Update MenuSubmenu’s panel semantics and focus
management: either ensure every supported child receives an appropriate menuitem
role while preserving the existing menu keyboard model, or remove role="menu"
and expose the panel as a plain group. Expand focusableItems beyond enabled
buttons so anchors and other supported interactive descendants are included in
moveFocus, while still excluding disabled elements.
In `@apps/desktop/src/shared/laneColorPalette.ts`:
- Around line 54-55: Update the filter in ALLOCATABLE_LANE_COLORS to normalize
PRIMARY_LANE_COLOR as well as entry.hex before comparing them, preserving the
exclusion of the reserved primary color regardless of the constant’s casing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e10003a4-06d0-4b2b-927c-ea9eca0c64e7
⛔ Files ignored due to path filters (9)
docs/ARCHITECTURE.mdis excluded by!docs/**docs/features/ade-code/README.mdis excluded by!docs/**docs/features/agents/README.mdis excluded by!docs/**docs/features/cto/README.mdis excluded by!docs/**docs/features/lanes/README.mdis excluded by!docs/**docs/features/sync-and-multi-device/ios-companion.mdis excluded by!docs/**docs/features/terminals-and-sessions/README.mdis excluded by!docs/**docs/features/terminals-and-sessions/ui-surfaces.mdis excluded by!docs/**docs/features/web-client/README.mdis excluded by!docs/**
📒 Files selected for processing (91)
apps/ade-cli/README.mdapps/ade-cli/src/adeRpcServer.test.tsapps/ade-cli/src/adeRpcServer.tsapps/ade-cli/src/cli.test.tsapps/ade-cli/src/cli.tsapps/ade-cli/src/services/push/pushPublisherService.test.tsapps/ade-cli/src/services/push/pushPublisherService.tsapps/ade-cli/src/tuiClient/__tests__/adeApi.test.tsapps/ade-cli/src/tuiClient/__tests__/sessionLifecycle.test.tsxapps/ade-cli/src/tuiClient/adeApi.tsapps/ade-cli/src/tuiClient/app.tsxapps/ade-cli/src/tuiClient/commands.tsapps/ade-cli/src/tuiClient/sessionLifecycle.tsapps/desktop/resources/ade-cli-help.txtapps/desktop/resources/agent-skills/ade-cli-control-plane/SKILL.mdapps/desktop/src/main/services/adeActions/registry.test.tsapps/desktop/src/main/services/adeActions/registry.tsapps/desktop/src/main/services/ai/tools/ctoOperatorTools.tsapps/desktop/src/main/services/ai/tools/systemPrompt.test.tsapps/desktop/src/main/services/chat/cursorSdkSystemPrompt.test.tsapps/desktop/src/main/services/lanes/laneService.tsapps/desktop/src/main/services/sessions/sessionService.tsapps/desktop/src/main/services/usage/usageStatsStore.tsapps/desktop/src/main/services/usage/usageTrackingService.test.tsapps/desktop/src/preload/preload.test.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/components/app/CommandPalette.test.tsxapps/desktop/src/renderer/components/app/CommandPalette.tsxapps/desktop/src/renderer/components/app/commandPaletteSearch.tsxapps/desktop/src/renderer/components/app/commandPaletteThreads.tsxapps/desktop/src/renderer/components/attention/AttentionCenter.cssapps/desktop/src/renderer/components/attention/AttentionCenter.tsxapps/desktop/src/renderer/components/attention/HeaderAttentionControl.cssapps/desktop/src/renderer/components/attention/HeaderAttentionControl.test.tsxapps/desktop/src/renderer/components/attention/attentionHeaderSummary.tsapps/desktop/src/renderer/components/attention/attentionPresentation.test.tsapps/desktop/src/renderer/components/attention/attentionPresentation.tsapps/desktop/src/renderer/components/chat/AgentChatPane.tsxapps/desktop/src/renderer/components/chat/ChatLifecycleBanner.test.tsxapps/desktop/src/renderer/components/chat/ChatLifecycleBanner.tsxapps/desktop/src/renderer/components/chat/ChatSurfaceShell.tsxapps/desktop/src/renderer/components/lanes/LaneContextMenu.test.tsxapps/desktop/src/renderer/components/lanes/LaneContextMenu.tsxapps/desktop/src/renderer/components/lanes/laneColorPalette.tsapps/desktop/src/renderer/components/lanes/laneContextMenuItems.tsxapps/desktop/src/renderer/components/lanes/laneDesignTokens.tsapps/desktop/src/renderer/components/lanes/laneUtils.test.tsapps/desktop/src/renderer/components/terminals/LaneActionsSubmenu.tsxapps/desktop/src/renderer/components/terminals/LanePrBadge.tsxapps/desktop/src/renderer/components/terminals/SessionCard.test.tsxapps/desktop/src/renderer/components/terminals/SessionCard.tsxapps/desktop/src/renderer/components/terminals/SessionContextMenu.test.tsxapps/desktop/src/renderer/components/terminals/SessionContextMenu.tsxapps/desktop/src/renderer/components/terminals/SessionHoverCard.tsxapps/desktop/src/renderer/components/terminals/SessionInfoPopover.tsxapps/desktop/src/renderer/components/terminals/SessionListPane.test.tsxapps/desktop/src/renderer/components/terminals/SessionListPane.tsxapps/desktop/src/renderer/components/terminals/SessionSnoozeControl.tsxapps/desktop/src/renderer/components/terminals/SessionStatusSlot.tsxapps/desktop/src/renderer/components/terminals/TerminalsPage.test.tsxapps/desktop/src/renderer/components/terminals/TerminalsPage.tsxapps/desktop/src/renderer/components/terminals/sessionLifecycleActions.tsapps/desktop/src/renderer/components/terminals/useWorkLaneContextMenu.tsxapps/desktop/src/renderer/components/terminals/useWorkSessions.tsapps/desktop/src/renderer/components/ui/MenuSubmenu.tsxapps/desktop/src/renderer/components/work/WorkSurfaceHeader.test.tsxapps/desktop/src/renderer/components/work/WorkSurfaceHeader.tsxapps/desktop/src/renderer/index.cssapps/desktop/src/renderer/lib/sessionSnooze.test.tsapps/desktop/src/renderer/lib/sessionSnooze.tsapps/desktop/src/renderer/lib/terminalAttention.test.tsapps/desktop/src/renderer/lib/terminalAttention.tsapps/desktop/src/renderer/webclient/shell/sessionLifecycleChrome.tsapps/desktop/src/shared/adeCliGuidance.test.tsapps/desktop/src/shared/adeCliGuidance.tsapps/desktop/src/shared/laneColorPalette.tsapps/desktop/src/shared/sessionCanonicalState.tsapps/desktop/src/shared/sessionStatusPresentation.tsapps/ios/ADE/Shared/ADEAgentActivityAttributes.swiftapps/ios/ADE/Shared/ADESharedModels.swiftapps/ios/ADE/Shared/ADESharedTheme.swiftapps/ios/ADE/Views/AttentionDrawer/AttentionDrawerModel.swiftapps/ios/ADE/Views/AttentionDrawer/AttentionDrawerSheet.swiftapps/ios/ADE/Views/Work/WorkRootComponents.swiftapps/ios/ADE/Views/Work/WorkSessionCanonicalState.swiftapps/ios/ADE/Views/Work/WorkSessionGrouping.swiftapps/ios/ADETests/ADETests.swiftapps/ios/ADETests/AttentionDrawerModelTests.swiftapps/ios/ADETests/WorkSessionCanonicalStateTests.swiftapps/ios/ADEWidgets/ADEAgentActivityWidget.swiftapps/ios/ADEWidgets/ADELockScreenWidget.swift
💤 Files with no reviewable changes (1)
- apps/ade-cli/README.md
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fd3266484e
ℹ️ 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".
| // The inline input is gone (search now opens the command palette), but the | ||
| // query itself still drives every filter in this file and the setter stays in | ||
| // the contract so the parent can keep restoring/clearing it. | ||
| setQ: _setQ, |
There was a problem hiding this comment.
When an upgrading user has a nonempty persisted WorkProjectViewState.search, useWorkSessions still applies that value to local sessions and this component also applies q to foreign rows, but the removed input was the only UI that called setQ. Opening the command palette does not change this separate persisted query, so matching sessions remain silently hidden—potentially behind a misleading “No sessions” state—with no way to clear the filter in Work. Clear/migrate the old value or retain an active-filter indicator and reset action.
Useful? React with 👍 / 👎.
| let hiddenActiveCount = max(0, safeState.activeCount - safeState.runs.count) | ||
| let activeCount = hiddenActiveCount + visibleInFlightCount |
There was a problem hiding this comment.
When the publisher has more than three runs and a recent completed/failed/waiting row occupies one of the capped runs slots, subtracting the entire visible roster from activeCount loses omitted working runs. For example, four working runs plus one newly completed run produce activeCount == 4 and a three-row roster containing the completion and two working runs; this computes only three working runs. The payload needs a working-count field or the hidden remainder must be derived from visible entries that actually contribute to the producer's count, rather than from runs.count.
Useful? React with 👍 / 👎.
1 participant
Summary
Validation
Summary by CodeRabbit