Refine Work session lifecycle and sidebar - #973
Merged
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
Owner
Author
|
@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".
Comment on lines
+808
to
+811
| // 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 👍 / 👎.
Comment on lines
+181
to
+182
| 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 👍 / 👎.
This was referenced Jul 31, 2026
arul28
added a commit
that referenced
this pull request
Aug 10, 2026
Four independent fixes from the t3code competitor audit (P0 items 3 and 7). Renderer crash recovery (main.ts). `render-process-gone` only logged, so a dead renderer meant a permanent white screen while the agents behind it kept running — recoverable only by restarting ADE. It now reloads the canonical renderer URL after 500 ms. Renderer state rehydrates from the main process, so a reload costs a repaint, not data. The retry budget (3 per rolling 60 s) lives in a separate testable module because the failure it guards against and the failure it could cause are the same shape: a renderer that dies during boot would otherwise reload-loop forever. Recovery covers every reason except `clean-exit`, including `killed`. Work grid membership cap. Every grid tile renders a full live session surface with `terminalVisible`, and membership was unbounded — N tiles meant N whole AgentChatPanes against a ~4 GB renderer heap, which is what made a renderer OOM reachable in normal use. Capped at 6, enforced in the membership helper, at the drop layer (a full grid stops advertising its drop target, so no phantom affordance), and in the persisted-state normalizer so a set saved by an older build is trimmed on load rather than rebuilt. PR-merge auto-settlement blast radius. A merged PR with no declared `chatSessionIds` swept every non-settled chat in its lane, and that path deliberately bypasses settlement blockers — so one merge could file chats belonging to a different, still-open PR. PRs opened outside ADE legitimately arrive with no links, so the sweep is kept but bounded: it is skipped entirely when another PR in the lane is still live (ownership is genuinely ambiguous — that PR's own merge should file its work), and it never touches a session another PR explicitly claims. An explicit link still wins outright. AI review-resolver diff context. The `prRefreshIssueInventory` handoff dropped `diffHunk`, so the model addressed review comments having never seen the code they point at. Restored per thread and capped at 2 KB, trimmed from the front: a diff hunk ends at the commented line, so the tail is the part the comment is about. Also removes `getSettlementBlockers`. Its only consumer was the `settleSelfSession` agent action, deleted in #973 on the explicit product decision that agents must not file their own Work rows. With no caller it read as "settlement consults blockers" when nothing does; that rationale now lives where the types were. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
arul28
added a commit
that referenced
this pull request
Aug 10, 2026
* Recover a crashed renderer; bound grid heap; fix two PR-path defects Four independent fixes from the t3code competitor audit (P0 items 3 and 7). Renderer crash recovery (main.ts). `render-process-gone` only logged, so a dead renderer meant a permanent white screen while the agents behind it kept running — recoverable only by restarting ADE. It now reloads the canonical renderer URL after 500 ms. Renderer state rehydrates from the main process, so a reload costs a repaint, not data. The retry budget (3 per rolling 60 s) lives in a separate testable module because the failure it guards against and the failure it could cause are the same shape: a renderer that dies during boot would otherwise reload-loop forever. Recovery covers every reason except `clean-exit`, including `killed`. Work grid membership cap. Every grid tile renders a full live session surface with `terminalVisible`, and membership was unbounded — N tiles meant N whole AgentChatPanes against a ~4 GB renderer heap, which is what made a renderer OOM reachable in normal use. Capped at 6, enforced in the membership helper, at the drop layer (a full grid stops advertising its drop target, so no phantom affordance), and in the persisted-state normalizer so a set saved by an older build is trimmed on load rather than rebuilt. PR-merge auto-settlement blast radius. A merged PR with no declared `chatSessionIds` swept every non-settled chat in its lane, and that path deliberately bypasses settlement blockers — so one merge could file chats belonging to a different, still-open PR. PRs opened outside ADE legitimately arrive with no links, so the sweep is kept but bounded: it is skipped entirely when another PR in the lane is still live (ownership is genuinely ambiguous — that PR's own merge should file its work), and it never touches a session another PR explicitly claims. An explicit link still wins outright. AI review-resolver diff context. The `prRefreshIssueInventory` handoff dropped `diffHunk`, so the model addressed review comments having never seen the code they point at. Restored per thread and capped at 2 KB, trimmed from the front: a diff hunk ends at the commented line, so the tail is the part the comment is about. Also removes `getSettlementBlockers`. Its only consumer was the `settleSelfSession` agent action, deleted in #973 on the explicit product decision that agents must not file their own Work rows. With no caller it read as "settlement consults blockers" when nothing does; that rationale now lives where the types were. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Put the mobile sync wire on a diet Two changes that cut what a phone downloads, both measured against this machine's live data (P0 items 1 and 2). Chat events crossed the wire uncompacted. `commitChatEvent` built two envelopes: the stored one went through `compactChatEventForStorage`, the live one handed to `eventSubscribers` — which is what the sync host pushes to phones and web — went out whole, transformed only by inline-image redaction. The same event was therefore multi-megabyte on live push and small after reconnect hydration, a size change users could observe. The deeper defect was two compaction implementations with two cap tables, which drifted the way duplicated policy does. `tool_result.structured` was added to the event and to neither table. On a real 8 MB transcript it had grown to 4.53 MB — 56.6% of the entire file, and ten times larger than `result`, the field that IS capped. So this collapses both into shared/chatEventCompaction, where a new heavy field cannot be capped on one side and forgotten on the other. `structured` is now bounded on disk and dropped from the wire outright, along with `toolResultMeta`. Everything ADE reads from `structured` (grep totals, bash timeout and cwd hints, subagent enrichment) is projected into typed fields on the same event at construction time; past that point nothing consumes it. `structured` is not a coding key in the iOS decoder at all, and `toolResultMeta` is written once and never read on any surface — the phone was downloading and parsing megabytes to produce something it discarded. Removing a field no client decodes is backward-compatible by construction, so this needs no capability gate; anything that added or reshaped a field would. Measured on a real thread, replaying every event through the new wire path: 7.99 MB → 3.46 MB overall, and tool_result 5.10 MB → 0.57 MB across 356 records. That transcript is already storage-compacted, so the live-push saving is larger than the 2.3x shown. `pull_request_snapshots` no longer rides the mobile changeset pump. It was 10.65 MB of a 26.8 MB synced project database — 39.7%, 258 rows averaging 42 KB, one `files_json` at 1.58 MB — for data the phone already fetched a second time itself. iOS reads the table in exactly one SELECT, the per-PR detail query, and reaches that data on demand through `prs.refresh`, which is in the REQUIRED remote-command set, so no paired build loses PR detail. Lists and badges are unaffected: slim `pull_requests` rows still sync, and though four iOS projections name the table as an invalidation trigger, no projection query reads a column from it. Already-paired devices keep the rows they have, so previously-opened PRs still render offline. It also ends a scroll-driven write path — the Lanes page's visible-lane refresh upserts here, so scrolling was pushing changesets to every phone. Note for the record: the research report said the `github_pr_*` siblings were already excluded and this table was merely overlooked. They are not excluded; the set had no PR tables at all. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Get iOS off the main actor on the streaming path, and stop trusting dead sockets Three iOS fixes from the t3code audit (P0 items 4, 5, 6). Chat events decoded on the main actor. Every `chat_event` re-serialized `[String: Any]` to Data and ran JSONDecoder on the main thread — and the two gates that reject an event ran AFTER that decode, so every duplicate replayed after a reconnect was fully decoded and then discarded. Both gates read the raw dictionary now and run first; the decode moved to `Task.detached`, mirroring the `changeset_batch` path that already did this correctly. Ordering is preserved for the same reason it is there: receiveLoop awaits each frame's handleIncoming before reading the next, so two chat events are never in flight at once. Two details the reordering had to get right — the sequence watermark still advances only after a successful decode, because advancing it first would burn it on an event that never applied and lose that event on re-subscribe; and the post-await connection-generation guard keeps a frame that arrives across a teardown from mutating the new session. The `chat_subscribe` snapshot decode (up to 256 KiB, landing exactly as a thread opens) gets the same treatment. Full transcript re-parsed per streaming tick. `makeWorkChatTranscript` ran over the whole fallback entry array — a page of 240-600 KB — on the main actor on every live delta, roughly 6-7 times a second while streaming. Nothing on that path consumed it: `workChatShouldPreferFallbackTranscript` short-circuits on an active turn before it ever reads the transcript, and the delta-append merge branch never touches it. The parameter is an autoclosure now, with the cheap status guards ordered ahead of it, and the call site memoizes so the two branches that do need it still build it at most once. Foreground resume trusted a socket iOS may have already suspended. Resumes are classified by how long the app was backgrounded, which is the only evidence available: under 10s the socket is probably real, so the existing liveness probe runs alongside the refreshes instead of the refreshes going out unchecked; at or above 10s the session is replaced outright, because mobile operating systems commonly suspend sockets without delivering a close event and probing one only buys a round trip we are about to spend anyway. Trusting it showed "Connected" over a dead pipe for up to the ~35-42s the heartbeat took to notice. The ladder now resets on every foreground, including from the terminal unreachable state that previously needed a manual tap to escape and meanwhile retried only on a 30-40s heartbeat. A resume also gets the manual button's connection strength — stale in-flight attempt cancelled, full candidate sweep rather than live-only, since the route that worked before a suspension is often the one that died with it — but deliberately not its user-intent side effects, so a deliberate "pause auto-reconnect" is still honored. NWPathMonitor racing and cursor-based resume are untouched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Fix the defects /quality found in the P0 branch A dual-track review of the three preceding commits, plus a re-review of these fixes. Every finding either landed here or is named in the summary; nothing was deferred. The branch was red. `apps/desktop/src/main/services/sync/syncHostService.test.ts` asserted the retired redaction wording; only the ade-cli copy of that expectation had been updated. Two more tests asserted the old copy after it changed here. Chat-event compaction was not idempotent, and two paths applied it twice. Hydration and the replay ring compact events that already came off disk compacted, and a second pass was not a no-op: the wrapper's newline-dense preview re-serializes with JSON escaping and lands back over the cap, so each pass made the payload BIGGER (16.7 KB → 17.1 KB → 18.0 KB) while overwriting `originalBytes` with the previous pass's size and destroying the real one. That inverted the whole point of the change on exactly the paths the lane was meant to slim, and falsified the module's own claim that a live push and a reconnect hydration agree byte for byte. Compaction now recognizes its own output. By SHAPE, deliberately, not by a marker key: the first attempt stamped `__adeCompacted` on the wrapper, and every surface that shows an object tool result renders it as a JSON dump — the desktop card and its collapsed preview, the TUI one-liner, the iOS Result block — so the marker became the first line the user read. Shape detection also recognizes wrappers already written to disk, which a marker never could, and it is bounded by size so recognizing our own output cannot become a cap bypass. Compaction copy is now user-neutral. The same text reaches phones, so it can no longer talk about "stored chat history". The renderer's truncated-diff matcher accepts both wordings — transcripts on disk carry the old one. Also from the review: result byte-accounting is no longer restated when only `structured` was capped (it was zeroing a real prior measurement); the grid drop affordance gates on persisted membership, which is what the cap actually counts; the iOS post-decode guards re-check the subscription, since an unsubscribe can land while the decode runs off-actor; the foreground-resume decision moved into SyncRecoveryPolicy.swift as a pure function beside the other 26 rules, which deleted a test-only accessor and turned two fixture-heavy tests into one direct assertion; lane-sweep scope resolution became a named discriminated union instead of an always-run loop whose output was dead on the common path; and the running-command-output cap stopped leaking out of the module built to own it. New regression tests cover idempotence on every branch, the byte-accounting rule, legacy wrapper recognition, the forged-wrapper bypass, the reorder of a full grid, and both copy wordings. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Cover the P0 branch: analytics, parity, and the defects CodeRabbit found Report a lost renderer as a product fact. The branch introduced a new product-level failure category and nothing recorded it, so `ade_renderer_recovered` joins the closed taxonomy beside `ade_brain_recovered` — the existing precedent for "a component died and we recovered it". It carries `crash_reason` and whether the reload was still allowed, nothing else. `crash_reason` is its own property key rather than the shared `reason`, which is pinned to the auto-update abort set and would have been weakened by widening; its values are Electron's closed enum, normalized to `unknown` so a future Electron string cannot widen what crosses the boundary. Volume is bounded by the recovery budget itself: a boot-crash loop stops trying rather than emitting forever. A boundary test pins that the window URL and title — both in scope at the crash site — cannot ride along. Fixes from an independent CodeRabbit review of the committed branch: - `reviewThreadDiffHunk` returned up to `MAX_CHARS + 4`, because the "...\n" marker was added after the budget instead of inside it. The cap is a promise about what reaches the prompt. My own test had asserted `<= cap + 4`, encoding the bug rather than catching it. - The renderer recovery budget read `Date.now()`. It is a rolling time window, so a wall-clock correction mid-crash-storm would either free the budget early or freeze it; it now reads `performance.now()`. - Declared chat sessions were found by filtering a 500-row lane listing, so a merged PR could silently fail to file a session it had explicitly named if a long-lived lane pushed it past that page. Declared sessions now resolve by id and keep an explicit lane check, which the lane-scoped listing had given for free. The sweep keeps the bounded listing — it is a guess, and a guess should stay bounded. Mobile parity found a real consequence of the wire diet. Phones now receive compacted `file_change` diffs on the live push, not only after hydration, and iOS counted the wrapper's `----- BEGIN FIRST PREVIEW -----` separators as deletions while deriving `+N / -N` from head-and-tail previews that omit the middle — wrong twice over, rendered as exact. It now reports nothing, matching desktop's `summarizeDiffStats`, and recognizes both the old and new notice wording since transcripts on disk carry the old text. VoiceOver gets the reason rather than a pair of zeros it would read as fact. Docs follow the code: the compaction module and its idempotence invariant, the wire-vs-storage agreement, the changeset-diet rule (a table needs an on-demand path and a required remote command before it can leave the pump), merge settlement scope, renderer crash recovery, the iOS resume classifier, and the grid tile cap. CLI and TUI parity: no changes required. The TUI reads chat events over the local RPC socket, so it sees the stored form and never the wire form; both compaction filter sites are mobile-peer-gated; and the removed settlement-blocker symbols have no remaining references. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Make the renderer-crash report survive the crash storm it reports Ship-phase quality revalidation of the previous commit found the new `ade_renderer_recovered` event structurally guaranteed to lose the occurrence that matters most. The recovery budget allows 3 reloads per rolling 60 s and the event's per-minute analytics budget was also 3, so the three successful reloads always consumed it and the fourth occurrence — the one reporting that the window stayed down — was dropped as rate-limited every time. The minute budget is now one above the recovery budget, with a regression test that captures four in a row and asserts the `recovered: false` one lands. Two hand-maintained copies of Electron's reason enum already disagreed: the normalizer listed seven values, the analytics allowlist six plus `unknown`. The allowlist is now derived from the normalizer's exported set, so a value one knows and the other does not can no longer ship an event stripped of its only payload. The crash handler gates analytics on the exported `isRecoverableRenderProcessGone` rather than re-testing `!== "clean-exit"` inline, so the analytics gate and the recovery gate cannot drift apart either. Smaller items from the same pass: the per-window recovery reporter is hoisted once instead of duplicated at both `createWindow` sites, so a third site cannot silently skip it; settlement scope selection is a named `switch` rather than a three-level nested ternary; a test helper drops an `unknown` cast it did not need; a Swift doc comment is reattached to the function it describes; and the `pull_request_snapshots` measurement reads in one unit everywhere — 11.2 MB of 28.1 MB, 39.7% — instead of MB in the code comment and MiB in the docs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Fence untrusted review content before it reaches the resolver Greptile flagged the diff hunk restored earlier in this branch as a prompt- injection vector, and it is right. `prRefreshIssueInventory` hands review data to an agent that also holds `prReplyToReviewThread` and `prResolveReviewThread`, neither of which asks for confirmation, so instruction-shaped text written by whoever opened the PR could steer real GitHub review-state mutations. Comment bodies had the same exposure and predate this branch, so the whole class is swept rather than just the field that drew the comment: review-thread diffs, review comments, and issue comments are each wrapped in an explicit fence that names them as data written by an outside contributor and says not to follow instructions inside. The wrapping happens in code on every value rather than being asked for in a prompt, the tool description states the same contract, and any occurrence of the fence marker inside a payload is defanged so content cannot close its own fence and speak as ADE. The regression test plants a forged END marker followed by an instruction and asserts exactly one BEGIN and one END survive per field. * Fix the review findings from PR #1056 Greptile and CodeRabbit between them found eight things, all verified against the code before changing anything. The renderer-recovery event reported the attempt, not the outcome. It emitted `recovered: true` the moment the budget allowed a reload, so a reload that was attempted and then failed still went out as a recovery. It now reports after `loadURL` settles — true on resolve, false on reject, and false when the budget refuses outright — so the metric describes what the user got rather than what ADE tried. Compaction could be skipped entirely by a payload it could not measure. When `JSON.stringify` throws (a BigInt anywhere in the object), the fallback measured `String(value)` — "[object Object]", 15 bytes, under every cap — so the original unbounded payload was stored and sent untouched, which is the one case a cap exists for. Unmeasurable is now treated as must-compact. A circular reference never reached this path; inline-image redaction breaks the cycle first. Compaction also ran once per subscriber. `sendChatEvent` compacted inside the per-peer loop, so one live event serialized and binary-searched its payload again for every peer watching that session. It is memoized against the envelope now — once per event, no matter how many peers. The shortened-diff matcher was unanchored, on both desktop and iOS, so a real diff whose own changed lines quoted the notice strings — editing the compactor, for instance — was classified as compacted and reported as zero additions and deletions. Both now require the header at the start. iOS measured the suspension gap on the wall clock, the same defect already fixed on the desktop budget: a device whose clock moves backward during a long suspension would report a short or negative gap and go on to trust a socket iOS had already suspended. It uses `systemUptime` now. And a shortened diff no longer draws a `-0` deletion badge through the delete-kind branch, which contradicted the VoiceOver label beside it. One test weakness: `bytes(undefined)` is 0, so upper-bound assertions about `structured` passed just as happily if compaction had deleted the field instead of bounding it. Size claims about fields that must survive now go through a helper that also asserts a lower bound. Regression tests: reload-rejection reporting, the unserializable payload, the anchored matcher on both platforms, and the fenced-content case from the earlier commit. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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
Validation
Summary by CodeRabbit