Desktop UI cleanup and Linear issue resolve flows#368
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
@copilot review but do not make fixes |
📝 WalkthroughWalkthroughAdds a browser runtime bridge with Vite live preview, implements Linear issue resolve modals and quick view, refactors Work/Terminals chrome and sessions UI, introduces lane color allocation and custom VCS icons, updates chat drawers/composer and various components/tests, and overhauls related CSS. ChangesEnd-to-end desktop renderer updates: bridge, Linear flows, Work chrome, lane colors/icons, and UI refactors
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
|
Capy auto-review is paused for this organization because the monthly auto-review limit has been reached. Increase the limit or turn it off in billing settings to resume automatic reviews. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review
Scope: 97 file(s), +5381 / −2329
Verdict: Needs changes
This PR refreshes desktop shell styling, consolidates Work/chat chrome, adds Linear issue resolve modals (create lane / new chat flows), a Vite browser-runtime bridge for local preview, and fixes the Linear addedLabelIds mutation. The main gaps are silent failures on lane creation and fragile handoff of Linear issue context into Work.
🔒 Security
[Medium] Dev browser bridge allows cross-origin RPC from any website
File: apps/desktop/scripts/browser-runtime-bridge.mjs:L79-L86 and L216-L227
Issue: The bridge listens on 127.0.0.1 but responds with Access-Control-Allow-Origin: *, and /sync forwards arbitrary method/params to the ADE JSON-RPC socket with no shared secret.
Attack path: While dev:vite:live / dev:browser-bridge is running, a malicious page the user has open in another tab can fetch("http://127.0.0.1:18765/sync", …) (or /action) and invoke project-scoped RPC (sync pairing, lane actions, Linear token setup via the patched mock path).
Fix: Drop Access-Control-Allow-Origin: *; allow only the Vite origin (e.g. http://localhost:5173), or require a bridge token header generated at startup and checked on every POST. Prefer proxying only through Vite without a directly reachable bridge port.
🐛 Functionality
[High] Linear resolve modals hide lane creation failures
File: apps/desktop/src/renderer/components/app/LinearQuickViewButton.tsx:L207-L228
Issue: handleCreateLaneAttached and handleResolveInNewLane await createLaneForIssue() inside try/finally with no catch. When lanes.create rejects (e.g. branch already exists for the issue slug), the promise rejects, busy state clears, and the modal stays open with no error surfaced.
Repro: Connect Linear → open quick view → pick an issue that already has a lane/branch → Resolve issue in new chat in new lane (or Create lane attached) again.
Fix: Wrap creation in try/catch, show the error in the modal (or app toast), and only call closeModal() / navigate on success.
[Medium] Same Linear issue is not re-attached on a second Work open
File: apps/desktop/src/renderer/components/chat/AgentChatPane.tsx:L4691-L4704
Issue: consumedInitialLinearIssueContextRef is keyed only by initialLinearIssueContext.id. Resolving the same issue into a different lane in the same session skips re-attachment because the ref still matches, while onInitialLinearIssueContextConsumed may already have cleared module pending state.
Repro: Resolve ADE-123 into lane A (context attaches) → return to Linear quick view → Resolve … in existing lane B for the same issue → Work opens without the Linear context attachment.
Fix: Key consumption by ${laneId}:${issue.id} (reset when laneId changes) or remount AgentChatPane with key={${laneId}:${issue.id}} from WorkStartSurface.
[Medium] Module-level pending Work context is single-slot
File: apps/desktop/src/renderer/lib/linearIssueWorkNavigation.ts:L11-L26
Issue: pending is one global variable; a second requestLinearIssueWorkContext overwrites the first before Work consumes it.
Repro: Trigger resolve-to-Work for lane A, then quickly for lane B before the Work draft mounts; opening lane A loses its pending issue/model.
Fix: Store a Map<laneId, PendingLinearIssueWorkContext> or pass context through router/store state instead of a singleton.
🎨 UI/UX
[Low] Linear top-bar control stays hidden for 8s after project open
File: apps/desktop/src/renderer/components/app/LinearQuickViewButton.tsx:L24 and L94-L102
Issue: Initial visibility waits INITIAL_VISIBILITY_CHECK_DELAY_MS (8s) before the first getLinearConnectionStatus call, so connected users see no Linear entry in the header/menu for several seconds on cold load (focus refresh helps only after interaction).
Fix: Run the first visibility check immediately (keep the delayed path only for disconnected projects to avoid polling), or show a disabled control until the check completes.
[Low] Resolve-in-new-lane confirm gives no feedback when model is empty
File: apps/desktop/src/renderer/components/app/LinearIssueResolveModals.tsx:L191-L196
Issue: Confirm calls if (!modelId.trim()) return; with no inline error; the primary button stays enabled when model recents are empty.
Repro: Open resolve modal with empty model recents → click Create lane and open chat → nothing happens.
Fix: Disable the confirm button when !modelId.trim() and show a short validation hint under the model picker.
Notes
- The Linear GraphQL fix (
addedLabelIdsvslabelIds) and lane auto-color allocation look correct. GlowMenuincludesaria-label/aria-pressedon tab buttons; good pattern for the new Work tab strip.- Browser bridge risk is limited to local dev when the bridge process is running; production Electron IPC is unchanged.
- Did not run desktop typecheck/tests in this review pass.
Sent by Cursor Automation: BUGBOT in Versic
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Found one substantive regression in the reviewed commits: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
apps/desktop/src/renderer/components/chat/AgentChatPane.tsx (3)
6985-7047:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHide or disable the Chat actions trigger until a session exists.
When
selectedSessionIdisnull, this button is still rendered, but the empty-state branch never renderschatActionsPanelContent. Clicking it toggles the state without ever showing a drawer.Suggested fix
- {(showWorkspaceChrome && laneId) || canShowHandoff ? ( + {selectedSessionId && ((showWorkspaceChrome && laneId) || canShowHandoff) ? ( <SmartTooltip content={{ label: chatActionsOpen ? "Close chat actions" : "Open chat actions",🤖 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/chat/AgentChatPane.tsx` around lines 6985 - 7047, The Chat actions button can be toggled even when no session exists (selectedSessionId is null) which opens nothing because chatActionsPanelContent is empty; update the render and click logic in AgentChatPane so the button is hidden or disabled when selectedSessionId is falsy: include selectedSessionId in the conditional that renders the SmartTooltip/button (or add an early guard) and, inside the onClick handler for setChatActionsOpen, no-op if selectedSessionId is null (prevent toggling); adjust aria-disabled/tooltip text accordingly and reference chatActionsOpen, setChatActionsOpen, selectedSessionId, and chatActionsPanelContent when making the change.
6953-6961:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClose Cursor Cloud when opening App Control from the header.
This toggle leaves
cursorCloudPaneOpenuntouched, so it's possible to end up with both right-side panes mounted at once. The composer-level App Control toggle already closes Cursor Cloud, so this path should match it.Suggested fix
onClick={() => { setAppControlOpen((current) => { const next = !current; if (next) { setChatActionsOpen(false); setIosSimulatorOpen(false); + setCursorCloudPaneOpen(false); } return next; }); }}🤖 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/chat/AgentChatPane.tsx` around lines 6953 - 6961, When toggling App Control via the header in the onClick handler for setAppControlOpen, ensure Cursor Cloud is closed the same way the composer-level toggle does: when computing next = !current, if next is true call setChatActionsOpen(false), setIosSimulatorOpen(false) and also call setCursorCloudPaneOpen(false) so the Cursor Cloud pane is unmounted when App Control opens; leave behavior unchanged when closing (i.e., only close Cursor Cloud on open).
4125-4135:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDon't clear the restored chat-actions state here.
This effect runs after the new companion-state hydration and forces
chatActionsOpenback tofalseon every session/lane switch, so the per-sessionsessionStoragestate you just added never actually survives a restore.Suggested fix
useEffect(() => { setAttachments([]); setContextAttachments([]); setPromptSuggestion(null); - setChatActionsOpen(false); setHandoffBusy(false); optimisticOutgoingMessageRef.current = null; setOptimisticOutgoingMessage(null);🤖 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/chat/AgentChatPane.tsx` around lines 4125 - 4135, The effect that resets state on session/lane change currently forces chatActionsOpen to false which overrides the per-session restore from sessionStorage; remove the unconditional setChatActionsOpen(false) (or guard it by checking whether a restored value exists) inside the useEffect that depends on selectedSessionId and laneId so the hydrated chat-actions state can survive a session/lane switch; keep the other resets (setAttachments, setContextAttachments, setPromptSuggestion, setHandoffBusy, optimisticOutgoingMessageRef.current, setOptimisticOutgoingMessage) unchanged.apps/desktop/src/renderer/components/terminals/LaneCombobox.tsx (1)
161-185:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestore visible/semantic active option state for keyboard navigation.
highlightedIndexchanges on Arrow keys (Line 169 onward), but options don’t render any highlighted/active state anymore (Line 425 onward). Keyboard users can’t tell which option Enter will select.Suggested patch
- <div ref={listRef} className="ade-lane-popover-list"> + <div ref={listRef} className="ade-lane-popover-list" role="listbox"> {items.length === 0 ? ( @@ - items.map((item) => { + items.map((item, index) => { const isSelected = item.id === value; const isAutoCreate = item.id === AUTO_CREATE_LANE_OPTION_ID; @@ <button key={item.id} type="button" + role="option" + aria-selected={isSelected} className="ade-lane-popover-item ade-lane-popover-item-featured" data-selected={isSelected ? "true" : undefined} + data-highlighted={index === highlightedIndex ? "true" : undefined} + onMouseEnter={() => setHighlightedIndex(index)} onClick={() => selectItem(item.id)} > @@ <button key={item.id} type="button" + role="option" + aria-selected={isSelected} className="ade-lane-popover-item" data-selected={isSelected ? "true" : undefined} + data-highlighted={index === highlightedIndex ? "true" : undefined} + onMouseEnter={() => setHighlightedIndex(index)} onClick={() => selectItem(item.id)}As per coding guidelines,
apps/desktop/src/**: Electron desktop app — check for IPC security, proper main/renderer process separation, and React best practices.Also applies to: 412-517
🤖 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/LaneCombobox.tsx` around lines 161 - 185, The keyboard handler (handleKeyDown) updates highlightedIndex but the option render path no longer reflects that state, so restore visible and semantic active state by: in the options rendering code (where items are mapped and option elements are created) add a comparison (index === highlightedIndex) to apply a visible active CSS class and set aria-selected or aria-current accordingly, give each option a stable id and set the container's aria-activedescendant to that id when highlighted, and ensure the Enter handler (selectItem) continues to use items[highlightedIndex] so keyboard selection matches the visible/semantic highlight; update any role attributes to role="listbox"/role="option" if missing to support screen readers.
🧹 Nitpick comments (2)
apps/desktop/src/renderer/components/app/TopBar.tsx (1)
329-330: ⚡ Quick winAvoid partial ARIA menu semantics in the compact status popover.
The popover is marked as a
menu/menuitem, but it behaves like regular tabbable buttons (no menu keyboard model like roving focus/arrow key navigation). This can confuse assistive tech. Either implement full menu semantics or switch to dialog/popover semantics with native button roles.As per coding guidelines
apps/desktop/src/**: Electron desktop app — check for IPC security, proper main/renderer process separation, and React best practices.Also applies to: 427-429, 447-449
🤖 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/app/TopBar.tsx` around lines 329 - 330, The TopBar component currently assigns menu/menuitem roles to the compact status popover (e.g., the element using role={layout === "menu-row" ? "menuitem" : undefined} and related elements), but it does not implement the full ARIA menu keyboard model; update the component to either implement full menu semantics (roving focus, arrow-key navigation, proper aria-activedescendant management) for the elements that use role="menu"/"menuitem" or, preferably, remove the partial menu semantics and switch to proper popover/dialog semantics by removing role="menu"/"menuitem", using native <button> elements or role="button", and adding aria-haspopup, aria-expanded, and aria-controls as appropriate for the popover trigger; make the same change for the other instances in this file where menu/menuitem roles are set (the other compact status popover occurrences).apps/desktop/src/renderer/components/terminals/WorkViewArea.tsx (1)
1893-1904: ⚡ Quick winExtract duplicated “New Chat” tab-strip button into a shared renderer.
The same JSX appears in both grouped and ungrouped tab-strip paths; extracting it will prevent style/behavior drift.
Suggested refactor
+ function NewChatTabButton({ onClick }: { onClick: () => void }) { + return ( + <SmartTooltip content={{ label: "New Chat", description: "Start a new AI chat session in the current lane." }}> + <button + type="button" + className="ade-work-new-chat-btn inline-flex shrink-0 items-center justify-center" + onClick={onClick} + aria-label="Start a new chat" + > + <Plus size={11} weight="bold" /> + </button> + </SmartTooltip> + ); + } ... - {visibleSessions.length === 0 ? ( - <SmartTooltip content={{ label: "New Chat", description: "Start a new AI chat session in the current lane." }}> - <button ...> - <Plus size={11} weight="bold" /> - </button> - </SmartTooltip> - ) : null} + {visibleSessions.length === 0 ? <NewChatTabButton onClick={() => onShowDraftKind("chat")} /> : null}Also applies to: 2057-2068
🤖 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/WorkViewArea.tsx` around lines 1893 - 1904, The duplicated "New Chat" tab-strip button JSX in WorkViewArea.tsx (appearing in both grouped and ungrouped tab-strip paths) should be extracted into a single renderer to avoid drift: create a small local component or render function (e.g., NewChatButton or renderNewChatButton) that accepts the needed props (at minimum onShowDraftKind) and returns the SmartTooltip-wrapped button with the same className, onClick handler, aria-label and Plus icon props; then replace the duplicated blocks in both tab-strip branches with this new renderer so styling and behavior remain consistent.
🤖 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/desktop/scripts/browser-runtime-bridge.mjs`:
- Around line 79-85: Replace the wildcard CORS header by validating the incoming
Origin and returning a specific allowed origin: update sendJson to read the
request Origin, compare it to a small allowlist (configured via env or a
constant) and only set "Access-Control-Allow-Origin" to the matched origin (omit
or return 403 if not allowed); apply the same Origin-allowlist logic to the
other runtime endpoints/handlers in the file (the handlers in the region
covering the runtime action/sync endpoints referenced around lines 171-245) and
ensure preflight (OPTIONS) responses use the same allowlist and allowed
methods/headers.
In `@apps/desktop/scripts/dev-vite-live.mjs`:
- Around line 72-83: The bridge exit handler currently doesn't propagate its
exit/signal to the wrapper; capture the bridge exit results by storing the exit
code and signal in module-scope variables inside bridge.on("exit", (code,
signal) => ...) (e.g., bridgeExitCode, bridgeExitSignal) before calling
shutdown("SIGTERM"). Then change the vite.on handler to accept (code, signal)
and when deciding process.exit, prefer a non-zero exit: if code is a number use
it, otherwise if signal is set convert it to an exit code via
util.convertProcessSignalToExitCode(signal) (or use the stored
bridgeExitSignal/bridgeExitCode if the bridge triggered shutdown) and pass that
to process.exit so signal terminations are not treated as success.
In `@apps/desktop/src/renderer/browserRuntimeBridge.ts`:
- Around line 87-116: The patch currently replaces ade.cto.setLinearToken and
ade.cto.clearLinearToken with bridge-backed calls but does not update the
renderer mock API aliases setLinearOAuthClient and clearLinearOAuthClient,
leaving the OAuth set/clear behavior unmapped; inside patchLinearMethods
add/update ade.cto.setLinearOAuthClient and ade.cto.clearLinearOAuthClient to
perform the same bridgePost actions as ade.cto.setLinearToken and
ade.cto.clearLinearToken (use the same domains/actions:
"linear_credentials"/"setToken" and "clearToken" then return the
"linear_issue_tracker"/"getConnectionStatus" bridgePost) so both the old and new
method names are backed by the bridge.
In `@apps/desktop/src/renderer/components/app/LinearIssueBrowser.tsx`:
- Around line 164-172: The hasActiveFilters function treats a whitespace-only
search as an active filter because it compares filters.query directly; change
the comparison to use the normalized query (e.g., filters.query.trim()) so that
whitespace-only queries are considered equal to DEFAULT_FILTERS.query. Update
the check in hasActiveFilters to normalize filters.query before comparing
(reference hasActiveFilters, DEFAULT_FILTERS, and the search usage that calls
filters.query.trim()) so Reset filters and persistence behavior match the actual
search logic.
In `@apps/desktop/src/renderer/components/app/LinearIssueResolveModals.tsx`:
- Around line 280-283: The function openLinearIssueExternalUrl currently calls
window.ade.app.openExternal directly which can fail if the IPC bridge is
unavailable; change it to use the shared helper openExternalUrl(url) instead
(preserve the early return when url is null/undefined) so failures are handled
safely and avoid unhandled rejections; update references inside
openLinearIssueExternalUrl to call openExternalUrl with the same url parameter
and remove the direct window.ade.app.openExternal invocation.
In `@apps/desktop/src/renderer/components/app/LinearQuickViewButton.tsx`:
- Around line 207-240: The three handlers (handleCreateLaneAttached,
handleResolveInNewLane, handleResolveInExistingLane) currently use try/finally
only, so async rejections (from createLaneForIssue, openWorkDraft, etc.) can
become unhandled; add explicit catch blocks to each async handler to log the
error (use the same logger/context you already use in this file), show a
user-visible error (toast/modal) and ensure closeModal() still runs, e.g., move
closeModal into try or call it in both success and error paths, and keep
setBusyModal(null) in finally; reference createLaneForIssue, openLaneInLanesTab,
openWorkDraft, setBusyModal and closeModal when implementing these changes.
In `@apps/desktop/src/renderer/components/lanes/linearProjectIcon.tsx`:
- Around line 147-148: Validate and normalize the tile background color before
applying it to the style: instead of using the raw color prop in the style
(background: color ?? LINEAR_BRAND.primaryBright), run the color through the
same normalization routine used by the glyph path (the glyph's color
normalization function or helper) and fall back to a normalized
LINEAR_BRAND.primaryBright when the normalized value is invalid; update the
background assignment to use that normalized/fallback value so invalid CSS
values are never applied.
In `@apps/desktop/src/renderer/components/terminals/LaneChip.tsx`:
- Around line 155-156: The onContextMenu handler is being forwarded directly
(onLaneContextMenu) which lets parent/context handlers also run; update the
places where onContextMenu is passed (the LaneChip lane control elements using
onLaneContextMenu) to wrap the callback in a short inline handler that calls
event.preventDefault() and event.stopPropagation() and then invokes
onLaneContextMenu(event) (only if handler exists); apply the same change to both
occurrences so context-menu events are isolated like click/mousedown handlers.
In `@apps/desktop/src/renderer/components/terminals/WorkViewArea.tsx`:
- Around line 1412-1417: The close button (data-close-tab-session-id, className
"ade-work-tab-close", title using isBusy) is only revealed on hover so keyboard
users cannot see it when focused; update the button's styling to also become
visible on keyboard focus by adding focus-visible and/or focus state classes
(e.g., focus:opacity-100 or focus-visible:opacity-100 and an appropriate focus
ring like focus:outline-none focus:ring) so tabbing shows the control and
maintains the existing hover behavior; ensure the change is applied to the same
button element inside WorkViewArea.tsx so keyboard users can access the "Remove
from Work view" action.
In `@apps/desktop/src/renderer/index.css`:
- Line 451: Add a blank line before the CSS declaration "height:
var(--shell-project-tab-height);" (and the other identical declaration flagged
by stylelint) so the rule declaration-empty-line-before is satisfied; if the
declaration is the first property in its ruleset, instead add a stylelint
exemption comment (stylelint-disable-next-line declaration-empty-line-before)
immediately above that declaration to avoid a false positive. Ensure you update
both occurrences flagged by the linter.
In `@apps/desktop/src/renderer/lib/linearIssueWorkNavigation.ts`:
- Around line 29-39: The current peekLinearIssueWorkContext and
consumeLinearIssueWorkContext read pending without validating its requestedAt
timestamp, allowing stale contexts to be returned; update both functions to
accept a requestedAt (or compare to now) and only return/consume pending when
pending.laneId === laneId AND pending.requestedAt matches the provided
requestedAt (or pending.requestedAt is within a MAX_PENDING_AGE_MS threshold);
if the timestamp check fails, return null and do not clear pending (for peek) or
clear only when the timestamp matches (for consume). Ensure you reference and
use the pending.requestedAt property, add a MAX_PENDING_AGE_MS constant if using
age-based validation, and update both function signatures
(peekLinearIssueWorkContext, consumeLinearIssueWorkContext) and their callers
accordingly.
In `@apps/desktop/src/shared/laneColorPalette.ts`:
- Around line 63-67: The function laneColorName does not trim the input hex
string, so whitespace-padded values won't match; update laneColorName to first
trim the incoming hex (e.g., const normalized = hex.trim()), return null if
normalized is empty, then use normalized.toLowerCase() when comparing against
LANE_COLOR_PALETTE entries (entry.hex.toLowerCase()) so trimmed values resolve
correctly; keep using LANE_COLOR_PALETTE and the existing find/?.name logic.
---
Outside diff comments:
In `@apps/desktop/src/renderer/components/chat/AgentChatPane.tsx`:
- Around line 6985-7047: The Chat actions button can be toggled even when no
session exists (selectedSessionId is null) which opens nothing because
chatActionsPanelContent is empty; update the render and click logic in
AgentChatPane so the button is hidden or disabled when selectedSessionId is
falsy: include selectedSessionId in the conditional that renders the
SmartTooltip/button (or add an early guard) and, inside the onClick handler for
setChatActionsOpen, no-op if selectedSessionId is null (prevent toggling);
adjust aria-disabled/tooltip text accordingly and reference chatActionsOpen,
setChatActionsOpen, selectedSessionId, and chatActionsPanelContent when making
the change.
- Around line 6953-6961: When toggling App Control via the header in the onClick
handler for setAppControlOpen, ensure Cursor Cloud is closed the same way the
composer-level toggle does: when computing next = !current, if next is true call
setChatActionsOpen(false), setIosSimulatorOpen(false) and also call
setCursorCloudPaneOpen(false) so the Cursor Cloud pane is unmounted when App
Control opens; leave behavior unchanged when closing (i.e., only close Cursor
Cloud on open).
- Around line 4125-4135: The effect that resets state on session/lane change
currently forces chatActionsOpen to false which overrides the per-session
restore from sessionStorage; remove the unconditional setChatActionsOpen(false)
(or guard it by checking whether a restored value exists) inside the useEffect
that depends on selectedSessionId and laneId so the hydrated chat-actions state
can survive a session/lane switch; keep the other resets (setAttachments,
setContextAttachments, setPromptSuggestion, setHandoffBusy,
optimisticOutgoingMessageRef.current, setOptimisticOutgoingMessage) unchanged.
In `@apps/desktop/src/renderer/components/terminals/LaneCombobox.tsx`:
- Around line 161-185: The keyboard handler (handleKeyDown) updates
highlightedIndex but the option render path no longer reflects that state, so
restore visible and semantic active state by: in the options rendering code
(where items are mapped and option elements are created) add a comparison (index
=== highlightedIndex) to apply a visible active CSS class and set aria-selected
or aria-current accordingly, give each option a stable id and set the
container's aria-activedescendant to that id when highlighted, and ensure the
Enter handler (selectItem) continues to use items[highlightedIndex] so keyboard
selection matches the visible/semantic highlight; update any role attributes to
role="listbox"/role="option" if missing to support screen readers.
---
Nitpick comments:
In `@apps/desktop/src/renderer/components/app/TopBar.tsx`:
- Around line 329-330: The TopBar component currently assigns menu/menuitem
roles to the compact status popover (e.g., the element using role={layout ===
"menu-row" ? "menuitem" : undefined} and related elements), but it does not
implement the full ARIA menu keyboard model; update the component to either
implement full menu semantics (roving focus, arrow-key navigation, proper
aria-activedescendant management) for the elements that use
role="menu"/"menuitem" or, preferably, remove the partial menu semantics and
switch to proper popover/dialog semantics by removing role="menu"/"menuitem",
using native <button> elements or role="button", and adding aria-haspopup,
aria-expanded, and aria-controls as appropriate for the popover trigger; make
the same change for the other instances in this file where menu/menuitem roles
are set (the other compact status popover occurrences).
In `@apps/desktop/src/renderer/components/terminals/WorkViewArea.tsx`:
- Around line 1893-1904: The duplicated "New Chat" tab-strip button JSX in
WorkViewArea.tsx (appearing in both grouped and ungrouped tab-strip paths)
should be extracted into a single renderer to avoid drift: create a small local
component or render function (e.g., NewChatButton or renderNewChatButton) that
accepts the needed props (at minimum onShowDraftKind) and returns the
SmartTooltip-wrapped button with the same className, onClick handler, aria-label
and Plus icon props; then replace the duplicated blocks in both tab-strip
branches with this new renderer so styling and behavior remain consistent.
🪄 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
Run ID: efe3eb7f-66b9-4fa5-9cb5-639bd728c70d
⛔ Files ignored due to path filters (7)
README.mdis excluded by!*.mddocs/ARCHITECTURE.mdis excluded by!docs/**docs/features/agents/README.mdis excluded by!docs/**docs/features/chat/README.mdis excluded by!docs/**docs/features/history/README.mdis excluded by!docs/**docs/features/lanes/README.mdis excluded by!docs/**docs/features/linear-integration/README.mdis excluded by!docs/**
📒 Files selected for processing (90)
apps/ade-cli/src/tuiClient/app.tsxapps/desktop/README.mdapps/desktop/package.jsonapps/desktop/resources/agent-skills/ade-app-control/SKILL.mdapps/desktop/scripts/browser-runtime-bridge.mjsapps/desktop/scripts/dev-vite-live.mjsapps/desktop/src/main/main.tsapps/desktop/src/main/services/ai/tools/orchestrationTools.tsapps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/cto/linearAuth.test.tsapps/desktop/src/main/services/cto/linearClient.tsapps/desktop/src/main/services/lanes/laneService.tsapps/desktop/src/main/services/notifications/apnsBridgeService.tsapps/desktop/src/main/services/orchestration/manifestNormalization.tsapps/desktop/src/main/services/orchestration/orchestrationService.tsapps/desktop/src/renderer/browserMock.tsapps/desktop/src/renderer/browserRuntimeBridge.tsapps/desktop/src/renderer/components/app/CommandPalette.tsxapps/desktop/src/renderer/components/app/LinearIssueBrowser.tsxapps/desktop/src/renderer/components/app/LinearIssueResolveModals.tsxapps/desktop/src/renderer/components/app/LinearQuickViewButton.tsxapps/desktop/src/renderer/components/app/TabNav.test.tsxapps/desktop/src/renderer/components/app/TabNav.tsxapps/desktop/src/renderer/components/app/TopBar.test.tsxapps/desktop/src/renderer/components/app/TopBar.tsxapps/desktop/src/renderer/components/chat/AgentChatComposer.test.tsxapps/desktop/src/renderer/components/chat/AgentChatComposer.tsxapps/desktop/src/renderer/components/chat/AgentChatPane.submit.test.tsxapps/desktop/src/renderer/components/chat/AgentChatPane.tsxapps/desktop/src/renderer/components/chat/ChatActionsDrawerPanel.tsxapps/desktop/src/renderer/components/chat/ChatBuiltInBrowserPanel.tsxapps/desktop/src/renderer/components/chat/ChatComposerShell.tsxapps/desktop/src/renderer/components/chat/ChatGitToolbar.test.tsxapps/desktop/src/renderer/components/chat/ChatGitToolbar.tsxapps/desktop/src/renderer/components/chat/ChatIosSimulatorPanel.tsxapps/desktop/src/renderer/components/chat/ChatSurfaceShell.tsxapps/desktop/src/renderer/components/chat/ChatTerminalDrawer.tsxapps/desktop/src/renderer/components/files/FilesExplorer.tsxapps/desktop/src/renderer/components/files/FilesPage.test.tsxapps/desktop/src/renderer/components/files/FilesPage.tsxapps/desktop/src/renderer/components/history/CommitHistoryView.tsxapps/desktop/src/renderer/components/history/HistoryPage.tsxapps/desktop/src/renderer/components/history/historyGitActions.tsapps/desktop/src/renderer/components/history/historyLaneActions.tsapps/desktop/src/renderer/components/lanes/BranchPickerView.tsxapps/desktop/src/renderer/components/lanes/LaneGitActionsPane.tsxapps/desktop/src/renderer/components/lanes/LanesPage.tsxapps/desktop/src/renderer/components/lanes/ManageLaneDialog.tsxapps/desktop/src/renderer/components/lanes/laneColorPalette.tsapps/desktop/src/renderer/components/lanes/linearProjectIcon.tsxapps/desktop/src/renderer/components/onboarding/HelpMenu.tsxapps/desktop/src/renderer/components/orchestration/PlanMarkdown.tsxapps/desktop/src/renderer/components/orchestration/TaskCard.tsxapps/desktop/src/renderer/components/prs/CreatePrModal.tsxapps/desktop/src/renderer/components/prs/detail/PrDetailPane.tsxapps/desktop/src/renderer/components/prs/shared/PrLaneCleanupBanner.tsxapps/desktop/src/renderer/components/prs/shared/PrRequestAiReviewDialog.tsxapps/desktop/src/renderer/components/review/ReviewPage.tsxapps/desktop/src/renderer/components/run/RunPage.test.tsxapps/desktop/src/renderer/components/settings/ChatAppearancePreview.tsxapps/desktop/src/renderer/components/settings/GitHubSection.tsxapps/desktop/src/renderer/components/shared/ModelPicker/ModelPicker.tsxapps/desktop/src/renderer/components/shared/ModelPicker/ReasoningEffortPicker.tsxapps/desktop/src/renderer/components/terminals/LaneChip.test.tsxapps/desktop/src/renderer/components/terminals/LaneChip.tsxapps/desktop/src/renderer/components/terminals/LaneCombobox.tsxapps/desktop/src/renderer/components/terminals/OrchestratorComposerEntry.test.tsxapps/desktop/src/renderer/components/terminals/SessionCard.tsxapps/desktop/src/renderer/components/terminals/SessionListPane.test.tsxapps/desktop/src/renderer/components/terminals/SessionListPane.tsxapps/desktop/src/renderer/components/terminals/TerminalsPage.tsxapps/desktop/src/renderer/components/terminals/WorkSidebar.tsxapps/desktop/src/renderer/components/terminals/WorkStartSurface.tsxapps/desktop/src/renderer/components/terminals/WorkViewArea.tsxapps/desktop/src/renderer/components/terminals/useWorkSessions.test.tsapps/desktop/src/renderer/components/terminals/useWorkSessions.tsapps/desktop/src/renderer/components/ui/FloatingPane.tsxapps/desktop/src/renderer/components/ui/GlowMenu.tsxapps/desktop/src/renderer/components/ui/LaneBranchInline.tsxapps/desktop/src/renderer/components/ui/PaneTilingLayout.tsxapps/desktop/src/renderer/components/ui/vcsIcons.tsxapps/desktop/src/renderer/components/usage/HeaderUsageControl.tsxapps/desktop/src/renderer/index.cssapps/desktop/src/renderer/lib/laneNavigation.tsapps/desktop/src/renderer/lib/linearIssueWorkNavigation.tsapps/desktop/src/shared/adeCliGuidance.tsapps/desktop/src/shared/laneColorPalette.tsapps/desktop/src/shared/types/cto.tsapps/desktop/vite.config.tsscripts/dev-shared.mjs
💤 Files with no reviewable changes (5)
- apps/desktop/src/renderer/components/chat/ChatBuiltInBrowserPanel.tsx
- apps/desktop/src/main/services/orchestration/manifestNormalization.ts
- apps/desktop/src/renderer/components/chat/ChatGitToolbar.test.tsx
- apps/desktop/src/main/services/ai/tools/orchestrationTools.ts
- apps/desktop/src/renderer/components/orchestration/TaskCard.tsx
| function sendJson(res, statusCode, payload) { | ||
| res.writeHead(statusCode, { | ||
| "Content-Type": "application/json; charset=utf-8", | ||
| "Access-Control-Allow-Origin": "*", | ||
| "Access-Control-Allow-Methods": "GET, POST, OPTIONS", | ||
| "Access-Control-Allow-Headers": "Content-Type", | ||
| }); |
There was a problem hiding this comment.
Restrict bridge origins for privileged runtime actions.
The bridge currently allows Access-Control-Allow-Origin: * while exposing generic runtime action/sync endpoints. Any web page can attempt cross-origin calls to this localhost service during dev:vite:live.
🔒 Suggested hardening
+const allowedOrigins = new Set([
+ "http://localhost:5173",
+ "http://127.0.0.1:5173",
+]);
+
+function resolveAllowedOrigin(req) {
+ const origin = req.headers.origin;
+ return typeof origin === "string" && allowedOrigins.has(origin) ? origin : null;
+}
+
-function sendJson(res, statusCode, payload) {
+function sendJson(res, statusCode, payload, origin = null) {
res.writeHead(statusCode, {
"Content-Type": "application/json; charset=utf-8",
- "Access-Control-Allow-Origin": "*",
+ ...(origin ? { "Access-Control-Allow-Origin": origin, Vary: "Origin" } : {}),
"Access-Control-Allow-Methods": "GET, POST, OPTIONS",
"Access-Control-Allow-Headers": "Content-Type",
});
res.end(JSON.stringify(payload));
}
const server = http.createServer(async (req, res) => {
+ const origin = resolveAllowedOrigin(req);
+
if (req.method === "OPTIONS") {
- sendJson(res, 204, {});
+ if (!origin) {
+ sendJson(res, 403, { ok: false, error: "Forbidden origin." });
+ return;
+ }
+ sendJson(res, 204, {}, origin);
return;
}
+
+ if (req.method === "POST" && !origin) {
+ sendJson(res, 403, { ok: false, error: "Forbidden origin." });
+ return;
+ }Also applies to: 171-245
🤖 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/scripts/browser-runtime-bridge.mjs` around lines 79 - 85,
Replace the wildcard CORS header by validating the incoming Origin and returning
a specific allowed origin: update sendJson to read the request Origin, compare
it to a small allowlist (configured via env or a constant) and only set
"Access-Control-Allow-Origin" to the matched origin (omit or return 403 if not
allowed); apply the same Origin-allowlist logic to the other runtime
endpoints/handlers in the file (the handlers in the region covering the runtime
action/sync endpoints referenced around lines 171-245) and ensure preflight
(OPTIONS) responses use the same allowlist and allowed methods/headers.
| bridge.on("exit", (code, signal) => { | ||
| if (shuttingDown) return; | ||
| process.stderr.write( | ||
| `[ade] browser runtime bridge exited (${signal ?? code ?? "unknown"})\n`, | ||
| ); | ||
| shutdown("SIGTERM"); | ||
| }); | ||
|
|
||
| vite.on("exit", (code) => { | ||
| shutdown("SIGTERM"); | ||
| process.exit(typeof code === "number" ? code : 0); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the current exit-path behavior in the wrapper.
rg -n -C3 'bridge\.on\("exit"|vite\.on\("exit"|process\.exit' apps/desktop/scripts/dev-vite-live.mjsRepository: arul28/ADE
Length of output: 698
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="apps/desktop/scripts/dev-vite-live.mjs"
# Show the shutdown helper + surrounding exit handlers
nl -ba "$FILE" | sed -n '1,140p'
# Specifically locate any `shutdown(` implementation and any `bridge.kill/vite.kill` usage
rg -n "function shutdown|const shutdown|shutdown\(" "$FILE"
rg -n "bridge\.kill|vite\.kill|shuttingDown|bridge\.on\(\"exit\"|vite\.on\(\"exit\"" "$FILE"Repository: arul28/ADE
Length of output: 97
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="apps/desktop/scripts/dev-vite-live.mjs"
# Show the relevant region without `nl`
cat -n "$FILE" | sed -n '1,160p'
rg -n "function shutdown|const shutdown|shutdown\(" "$FILE" || true
rg -n "bridge\.kill|vite\.kill|shuttingDown|bridge\.on\\(\"exit\"\\)|vite\.on\\(\"exit\"\\)|process\.exit" "$FILE" || trueRepository: arul28/ADE
Length of output: 3630
🌐 Web query:
Node.js child_process 'exit' event (code, signal) code null when killed by signal documentation
💡 Result:
In Node.js child process handling, the 'exit' event callback parameters are (code, signal). When a child process is killed by a signal, code is null and signal is the signal name (e.g., 'SIGKILL'), and Node guarantees exactly one of code or signal is non-null. [1] Specifically, Node documents that for the child process 'exit' event: code is the final exit code if the child exited on its own, or null if it terminated due to a signal; signal is the string name of the signal if it was terminated due to a signal, or null otherwise. [1] Node also notes that when code is null due to signal termination, you can convert the signal to a POSIX exit code using util.convertProcessSignalToExitCode. [1] Example shape (matches the documentation behavior): child.on('exit', (code, signal) => { /* code === null when killed by signal */ }); [1] References: Node.js official child_process documentation. [1]
Citations:
Propagate bridge termination as a non-zero wrapper exit code (don’t map signal exits to 0)
In apps/desktop/scripts/dev-vite-live.mjs (lines 72-83), the wrapper exits 0 when the vite child terminates due to a signal: vite.on("exit", (code) => process.exit(typeof code === "number" ? code : 0)). Node’s child_process exit event provides code === null when killed by a signal, and the current handler treats that as success. Since a bridge exit triggers shutdown("SIGTERM"), bridge failures can be masked as a successful wrapper exit.
- Store the bridge’s exit code/signal in
bridge.on("exit", ...). - Update
vite.on("exit", ...)to accept(code, signal)and exit non-zero whencodeisnull/signalis set (preferutil.convertProcessSignalToExitCode(signal)).
🤖 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/scripts/dev-vite-live.mjs` around lines 72 - 83, The bridge exit
handler currently doesn't propagate its exit/signal to the wrapper; capture the
bridge exit results by storing the exit code and signal in module-scope
variables inside bridge.on("exit", (code, signal) => ...) (e.g., bridgeExitCode,
bridgeExitSignal) before calling shutdown("SIGTERM"). Then change the vite.on
handler to accept (code, signal) and when deciding process.exit, prefer a
non-zero exit: if code is a number use it, otherwise if signal is set convert it
to an exit code via util.convertProcessSignalToExitCode(signal) (or use the
stored bridgeExitSignal/bridgeExitCode if the bridge triggered shutdown) and
pass that to process.exit so signal terminations are not treated as success.
| function patchLinearMethods(ade: any) { | ||
| ade.cto.getLinearConnectionStatus = async () => | ||
| bridgePost("/action", { domain: "linear_issue_tracker", action: "getConnectionStatus" }); | ||
| ade.cto.getLinearQuickView = async () => | ||
| bridgePost("/action", { domain: "linear_issue_tracker", action: "getQuickView" }); | ||
| ade.cto.getLinearIssuePickerData = async () => | ||
| bridgePost("/action", { domain: "linear_issue_tracker", action: "getIssuePickerData" }); | ||
| ade.cto.searchLinearIssues = async (args: Record<string, unknown> = {}) => | ||
| bridgePost("/action", { domain: "linear_issue_tracker", action: "searchIssues", args }); | ||
| ade.cto.getLinearProjects = async () => | ||
| bridgePost("/action", { domain: "linear_issue_tracker", action: "listProjects" }); | ||
| ade.cto.setLinearToken = async (args: { token: string }) => { | ||
| await bridgePost("/action", { | ||
| domain: "linear_credentials", | ||
| action: "setToken", | ||
| arg: args.token, | ||
| }); | ||
| return bridgePost("/action", { | ||
| domain: "linear_issue_tracker", | ||
| action: "getConnectionStatus", | ||
| }); | ||
| }; | ||
| ade.cto.clearLinearToken = async () => { | ||
| await bridgePost("/action", { domain: "linear_credentials", action: "clearToken" }); | ||
| return bridgePost("/action", { | ||
| domain: "linear_issue_tracker", | ||
| action: "getConnectionStatus", | ||
| }); | ||
| }; | ||
| } |
There was a problem hiding this comment.
Patch the existing Linear OAuth method names, not only new aliases.
Line 98 and Line 109 override setLinearToken/clearLinearToken, but the renderer mock API shape in this PR uses setLinearOAuthClient/clearLinearOAuthClient. That leaves OAuth set/clear on mock handlers instead of bridge-backed calls.
Suggested fix
function patchLinearMethods(ade: any) {
@@
- ade.cto.setLinearToken = async (args: { token: string }) => {
+ const setLinearOAuthClient = async (args: { token: string }) => {
await bridgePost("/action", {
domain: "linear_credentials",
action: "setToken",
arg: args.token,
});
return bridgePost("/action", {
domain: "linear_issue_tracker",
action: "getConnectionStatus",
});
};
- ade.cto.clearLinearToken = async () => {
+ const clearLinearOAuthClient = async () => {
await bridgePost("/action", { domain: "linear_credentials", action: "clearToken" });
return bridgePost("/action", {
domain: "linear_issue_tracker",
action: "getConnectionStatus",
});
};
+ ade.cto.setLinearOAuthClient = setLinearOAuthClient;
+ ade.cto.clearLinearOAuthClient = clearLinearOAuthClient;
+ // Keep backward-compat aliases if any caller still uses old names.
+ ade.cto.setLinearToken = setLinearOAuthClient;
+ ade.cto.clearLinearToken = clearLinearOAuthClient;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function patchLinearMethods(ade: any) { | |
| ade.cto.getLinearConnectionStatus = async () => | |
| bridgePost("/action", { domain: "linear_issue_tracker", action: "getConnectionStatus" }); | |
| ade.cto.getLinearQuickView = async () => | |
| bridgePost("/action", { domain: "linear_issue_tracker", action: "getQuickView" }); | |
| ade.cto.getLinearIssuePickerData = async () => | |
| bridgePost("/action", { domain: "linear_issue_tracker", action: "getIssuePickerData" }); | |
| ade.cto.searchLinearIssues = async (args: Record<string, unknown> = {}) => | |
| bridgePost("/action", { domain: "linear_issue_tracker", action: "searchIssues", args }); | |
| ade.cto.getLinearProjects = async () => | |
| bridgePost("/action", { domain: "linear_issue_tracker", action: "listProjects" }); | |
| ade.cto.setLinearToken = async (args: { token: string }) => { | |
| await bridgePost("/action", { | |
| domain: "linear_credentials", | |
| action: "setToken", | |
| arg: args.token, | |
| }); | |
| return bridgePost("/action", { | |
| domain: "linear_issue_tracker", | |
| action: "getConnectionStatus", | |
| }); | |
| }; | |
| ade.cto.clearLinearToken = async () => { | |
| await bridgePost("/action", { domain: "linear_credentials", action: "clearToken" }); | |
| return bridgePost("/action", { | |
| domain: "linear_issue_tracker", | |
| action: "getConnectionStatus", | |
| }); | |
| }; | |
| } | |
| function patchLinearMethods(ade: any) { | |
| ade.cto.getLinearConnectionStatus = async () => | |
| bridgePost("/action", { domain: "linear_issue_tracker", action: "getConnectionStatus" }); | |
| ade.cto.getLinearQuickView = async () => | |
| bridgePost("/action", { domain: "linear_issue_tracker", action: "getQuickView" }); | |
| ade.cto.getLinearIssuePickerData = async () => | |
| bridgePost("/action", { domain: "linear_issue_tracker", action: "getIssuePickerData" }); | |
| ade.cto.searchLinearIssues = async (args: Record<string, unknown> = {}) => | |
| bridgePost("/action", { domain: "linear_issue_tracker", action: "searchIssues", args }); | |
| ade.cto.getLinearProjects = async () => | |
| bridgePost("/action", { domain: "linear_issue_tracker", action: "listProjects" }); | |
| const setLinearOAuthClient = async (args: { token: string }) => { | |
| await bridgePost("/action", { | |
| domain: "linear_credentials", | |
| action: "setToken", | |
| arg: args.token, | |
| }); | |
| return bridgePost("/action", { | |
| domain: "linear_issue_tracker", | |
| action: "getConnectionStatus", | |
| }); | |
| }; | |
| const clearLinearOAuthClient = async () => { | |
| await bridgePost("/action", { domain: "linear_credentials", action: "clearToken" }); | |
| return bridgePost("/action", { | |
| domain: "linear_issue_tracker", | |
| action: "getConnectionStatus", | |
| }); | |
| }; | |
| ade.cto.setLinearOAuthClient = setLinearOAuthClient; | |
| ade.cto.clearLinearOAuthClient = clearLinearOAuthClient; | |
| // Keep backward-compat aliases if any caller still uses old names. | |
| ade.cto.setLinearToken = setLinearOAuthClient; | |
| ade.cto.clearLinearToken = clearLinearOAuthClient; | |
| } |
🤖 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/browserRuntimeBridge.ts` around lines 87 - 116, The
patch currently replaces ade.cto.setLinearToken and ade.cto.clearLinearToken
with bridge-backed calls but does not update the renderer mock API aliases
setLinearOAuthClient and clearLinearOAuthClient, leaving the OAuth set/clear
behavior unmapped; inside patchLinearMethods add/update
ade.cto.setLinearOAuthClient and ade.cto.clearLinearOAuthClient to perform the
same bridgePost actions as ade.cto.setLinearToken and ade.cto.clearLinearToken
(use the same domains/actions: "linear_credentials"/"setToken" and "clearToken"
then return the "linear_issue_tracker"/"getConnectionStatus" bridgePost) so both
the old and new method names are backed by the bridge.
| function hasActiveFilters(filters: LinearIssueBrowserFilters): boolean { | ||
| return ( | ||
| filters.projectId !== DEFAULT_FILTERS.projectId | ||
| || filters.statePreset !== DEFAULT_FILTERS.statePreset | ||
| || filters.assigneeId !== DEFAULT_FILTERS.assigneeId | ||
| || filters.priority !== DEFAULT_FILTERS.priority | ||
| || filters.query !== DEFAULT_FILTERS.query | ||
| || filters.sort !== DEFAULT_FILTERS.sort | ||
| ); |
There was a problem hiding this comment.
Normalize whitespace query before considering filters active.
On Line 170, filters.query is compared as-is, but search uses filters.query.trim() (Line 360). A whitespace-only query is treated as an active filter, causing no-op state persistence and a misleading “Reset filters” affordance.
Proposed fix
function hasActiveFilters(filters: LinearIssueBrowserFilters): boolean {
+ const normalizedQuery = filters.query.trim();
return (
filters.projectId !== DEFAULT_FILTERS.projectId
|| filters.statePreset !== DEFAULT_FILTERS.statePreset
|| filters.assigneeId !== DEFAULT_FILTERS.assigneeId
|| filters.priority !== DEFAULT_FILTERS.priority
- || filters.query !== DEFAULT_FILTERS.query
+ || normalizedQuery !== DEFAULT_FILTERS.query
|| filters.sort !== DEFAULT_FILTERS.sort
);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function hasActiveFilters(filters: LinearIssueBrowserFilters): boolean { | |
| return ( | |
| filters.projectId !== DEFAULT_FILTERS.projectId | |
| || filters.statePreset !== DEFAULT_FILTERS.statePreset | |
| || filters.assigneeId !== DEFAULT_FILTERS.assigneeId | |
| || filters.priority !== DEFAULT_FILTERS.priority | |
| || filters.query !== DEFAULT_FILTERS.query | |
| || filters.sort !== DEFAULT_FILTERS.sort | |
| ); | |
| function hasActiveFilters(filters: LinearIssueBrowserFilters): boolean { | |
| const normalizedQuery = filters.query.trim(); | |
| return ( | |
| filters.projectId !== DEFAULT_FILTERS.projectId | |
| || filters.statePreset !== DEFAULT_FILTERS.statePreset | |
| || filters.assigneeId !== DEFAULT_FILTERS.assigneeId | |
| || filters.priority !== DEFAULT_FILTERS.priority | |
| || normalizedQuery !== DEFAULT_FILTERS.query | |
| || filters.sort !== DEFAULT_FILTERS.sort | |
| ); | |
| } |
🤖 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/app/LinearIssueBrowser.tsx` around lines
164 - 172, The hasActiveFilters function treats a whitespace-only search as an
active filter because it compares filters.query directly; change the comparison
to use the normalized query (e.g., filters.query.trim()) so that whitespace-only
queries are considered equal to DEFAULT_FILTERS.query. Update the check in
hasActiveFilters to normalize filters.query before comparing (reference
hasActiveFilters, DEFAULT_FILTERS, and the search usage that calls
filters.query.trim()) so Reset filters and persistence behavior match the actual
search logic.
| export function openLinearIssueExternalUrl(url: string | null | undefined): void { | ||
| if (!url) return; | ||
| void window.ade.app.openExternal(url); | ||
| } |
There was a problem hiding this comment.
Use the shared external-open helper to avoid bridge failures in renderer.
Line 282 directly calls window.ade.app.openExternal(url). If the bridge is unavailable/rejecting, this can throw or produce unhandled rejections. Use the existing openExternalUrl helper for safe fallback behavior.
Proposed fix
+import { openExternalUrl } from "../../lib/openExternal";
...
export function openLinearIssueExternalUrl(url: string | null | undefined): void {
- if (!url) return;
- void window.ade.app.openExternal(url);
+ openExternalUrl(url);
}As per coding guidelines apps/desktop/src/**: Electron desktop app — check for IPC security, proper main/renderer process separation, and React best practices.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function openLinearIssueExternalUrl(url: string | null | undefined): void { | |
| if (!url) return; | |
| void window.ade.app.openExternal(url); | |
| } | |
| import { openExternalUrl } from "../../lib/openExternal"; | |
| export function openLinearIssueExternalUrl(url: string | null | undefined): void { | |
| openExternalUrl(url); | |
| } |
🤖 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/app/LinearIssueResolveModals.tsx` around
lines 280 - 283, The function openLinearIssueExternalUrl currently calls
window.ade.app.openExternal directly which can fail if the IPC bridge is
unavailable; change it to use the shared helper openExternalUrl(url) instead
(preserve the early return when url is null/undefined) so failures are handled
safely and avoid unhandled rejections; update references inside
openLinearIssueExternalUrl to call openExternalUrl with the same url parameter
and remove the direct window.ade.app.openExternal invocation.
| onContextMenu={onLaneContextMenu} | ||
| > |
There was a problem hiding this comment.
Stop context-menu bubbling on lane controls.
Line 155 and Line 165 forward onContextMenu without stopping propagation, so parent row/menu handlers can fire alongside lane-level handling. Align this with the existing click/mousedown isolation.
Suggested fix
- onContextMenu={onLaneContextMenu}
+ onContextMenu={(event) => {
+ event.stopPropagation();
+ onLaneContextMenu?.(event);
+ }}
...
- onContextMenu={onLaneContextMenu}
+ onContextMenu={(event) => {
+ event.stopPropagation();
+ onLaneContextMenu?.(event);
+ }}As per coding guidelines, apps/desktop/src/**: Electron desktop app — check for IPC security, proper main/renderer process separation, and React best practices.
Also applies to: 165-166
🤖 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/LaneChip.tsx` around lines 155
- 156, The onContextMenu handler is being forwarded directly (onLaneContextMenu)
which lets parent/context handlers also run; update the places where
onContextMenu is passed (the LaneChip lane control elements using
onLaneContextMenu) to wrap the callback in a short inline handler that calls
event.preventDefault() and event.stopPropagation() and then invokes
onLaneContextMenu(event) (only if handler exists); apply the same change to both
occurrences so context-menu events are isolated like click/mousedown handlers.
| <button | ||
| type="button" | ||
| data-close-tab-session-id={session.id} | ||
| title={isBusy ? "Removing..." : "Remove from Work view"} | ||
| className="inline-flex items-center justify-center opacity-0 group-hover/tab:opacity-100 transition-opacity" | ||
| className="ade-work-tab-close inline-flex items-center justify-center opacity-0 transition-opacity group-hover/tab:opacity-100" | ||
| style={{ |
There was a problem hiding this comment.
Make tab-close control visible for keyboard focus.
The close button is hover-revealed only, so keyboard users can tab to an effectively invisible control.
Suggested fix
- className="ade-work-tab-close inline-flex items-center justify-center opacity-0 transition-opacity group-hover/tab:opacity-100"
+ className="ade-work-tab-close inline-flex items-center justify-center opacity-0 transition-opacity group-hover/tab:opacity-100 group-focus-within/tab:opacity-100 focus-visible:opacity-100"As per coding guidelines, apps/desktop/src/**: Electron desktop app — check for IPC security, proper main/renderer process separation, and React best practices.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <button | |
| type="button" | |
| data-close-tab-session-id={session.id} | |
| title={isBusy ? "Removing..." : "Remove from Work view"} | |
| className="inline-flex items-center justify-center opacity-0 group-hover/tab:opacity-100 transition-opacity" | |
| className="ade-work-tab-close inline-flex items-center justify-center opacity-0 transition-opacity group-hover/tab:opacity-100" | |
| style={{ | |
| <button | |
| type="button" | |
| data-close-tab-session-id={session.id} | |
| title={isBusy ? "Removing..." : "Remove from Work view"} | |
| className="ade-work-tab-close inline-flex items-center justify-center opacity-0 transition-opacity group-hover/tab:opacity-100 group-focus-within/tab:opacity-100 focus-visible:opacity-100" | |
| style={{ |
🤖 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/WorkViewArea.tsx` around lines
1412 - 1417, The close button (data-close-tab-session-id, className
"ade-work-tab-close", title using isBusy) is only revealed on hover so keyboard
users cannot see it when focused; update the button's styling to also become
visible on keyboard focus by adding focus-visible and/or focus state classes
(e.g., focus:opacity-100 or focus-visible:opacity-100 and an appropriate focus
ring like focus:outline-none focus:ring) so tabbing shows the control and
maintains the existing hover behavior; ensure the change is applied to the same
button element inside WorkViewArea.tsx so keyboard users can access the "Remove
from Work view" action.
|
|
||
| .ade-shell-project-tab { | ||
| --project-tab-accent: var(--color-accent); | ||
| height: var(--shell-project-tab-height); |
There was a problem hiding this comment.
Fix stylelint failures at Line 451 and Line 2716.
Both lines are failing declaration-empty-line-before; this can break lint-gated CI for the PR.
Also applies to: 2716-2716
🧰 Tools
🪛 Stylelint (17.12.0)
[error] 451-451: Expected empty line before declaration (declaration-empty-line-before)
(declaration-empty-line-before)
🤖 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/index.css` at line 451, Add a blank line before the
CSS declaration "height: var(--shell-project-tab-height);" (and the other
identical declaration flagged by stylelint) so the rule
declaration-empty-line-before is satisfied; if the declaration is the first
property in its ruleset, instead add a stylelint exemption comment
(stylelint-disable-next-line declaration-empty-line-before) immediately above
that declaration to avoid a false positive. Ensure you update both occurrences
flagged by the linter.
| export function laneColorName(hex: string | null | undefined): string | null { | ||
| if (!hex) return null; | ||
| const lower = hex.toLowerCase(); | ||
| return LANE_COLOR_PALETTE.find((entry) => entry.hex.toLowerCase() === lower)?.name ?? null; | ||
| } |
There was a problem hiding this comment.
Normalize hex consistently in laneColorName.
laneColorName doesn’t trim input, so whitespace-padded values won’t resolve even when valid.
💡 Suggested fix
export function laneColorName(hex: string | null | undefined): string | null {
if (!hex) return null;
- const lower = hex.toLowerCase();
+ const lower = hex.trim().toLowerCase();
+ if (!lower) return null;
return LANE_COLOR_PALETTE.find((entry) => entry.hex.toLowerCase() === lower)?.name ?? null;
}🤖 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 63 - 67, The
function laneColorName does not trim the input hex string, so whitespace-padded
values won't match; update laneColorName to first trim the incoming hex (e.g.,
const normalized = hex.trim()), return null if normalized is empty, then use
normalized.toLowerCase() when comparing against LANE_COLOR_PALETTE entries
(entry.hex.toLowerCase()) so trimmed values resolve correctly; keep using
LANE_COLOR_PALETTE and the existing find/?.name logic.
Refresh Work, Lanes, chat, and shell surfaces with shared lane/VCS chrome, add Linear quick-view resolve actions with confirmation modals and Work navigation, and land related dev/runtime helpers on the branch. Co-authored-by: Cursor <cursoragent@cursor.com>
Desktop UI cleanup — simplify control flow, DRY repeated patterns, fix Linear addedLabelIds mutation, update tests for new drawer selectors, and align docs with orchestration rename. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
arul28
force-pushed
the
ade/ui-clean-up-3470f34e
branch
from
b904257 to
10e44ad
Compare
|
@copilot review but do not make fixes |
- Remove test for deliberately-removed Refresh button in UsageQuotaPanel - Add 60s TTL to pending Linear issue work context to prevent stale consumption Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@copilot review but do not make fixes |
Summary
readString,resolveActivityUpdateToken,tokenTypeDetectionLabel,conflictDescription), DRY repeated patterns (heading attrs, APNS topic, connection status dot, commit graph colors)issueUpdatemutation to useaddedLabelIdsinstead oflabelIds(was replacing all labels instead of appending)"duplicate"to Linear state group ordering and usehasActiveFilters()for filter comparisonCSSPropertiesinline styles from TaskCard (already handled by Tailwindline-clamp-3)Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
UI/UX Improvements
Integrations
Greptile Summary
This PR refactors the desktop UI with control-flow simplifications (lookup tables, shared helpers), adds Linear issue resolve workflows (create lane, resolve in new/existing lane), introduces automatic lane color allocation, and fixes the Linear label mutation to use
addedLabelIdsinstead oflabelIds. It also adds a browser-based dev preview runtime bridge and consolidates the proof/handoff/agents UI into a singleChatActionsDrawerPanel.issueUpdatenow usesaddedLabelIdsso existing labels are appended rather than replaced.allocateLaneColorhelper inshared/laneColorPalette.tsis wired into fourlaneServiceinsert paths and is project-scoped via the per-project DB.LinearIssueResolveModals+linearIssueWorkNavigationmodule drives "create lane", "resolve in new lane", and "resolve in existing lane" workflows with a 60-second TTL on the pending context singleton.filterStatusfilter logic removed fromfilteredSessions(UI control was already removed),proofDrawerOpenmigrated tochatActionsOpen/Tabwith legacy migration, andChatGitToolbarstripped of inline commit/diff UI.Confidence Score: 5/5
Safe to merge — the addedLabelIds fix is correct, lane color allocation is project-scoped, and the new pending context TTL addresses the stale-context scenario from the prior review.
All substantive logic changes (label mutation fix, color auto-allocation SQL inserts, pending-context TTL) are straightforward and well-contained. The one new finding is a cosmetic rendering edge case in LinearProjectIcon where an absent project color produces an invalid CSS background on the letter chip; it is visible only for Linear projects with no color configured and does not affect data correctness or navigation.
apps/desktop/src/renderer/components/lanes/linearProjectIcon.tsx — the non-glyph letter chip uses ?? instead of || for its background fallback.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[LinearQuickViewButton] -->|opens| B[LinearIssueBrowser] B -->|resolveActions.onOpenModal| C{LinearIssueResolveModalKind} C -->|create-lane| D[CreateLaneAttachedModal] C -->|resolve-new-lane| E[ResolveInNewLaneModal] C -->|resolve-existing-lane| F[ResolveInExistingLaneModal] D -->|onConfirm| G[createLaneForIssue] E -->|onConfirm modelId| G G -->|lane created| H[laneService.create auto-allocates color] E -->|after lane created| I[openWorkDraftForLinearIssue] F -->|onConfirm laneId+modelId| I I -->|requestLinearIssueWorkContext TTL=60s| J[linearIssueWorkNavigation singleton] I -->|navigate| K[Work #/work?laneId=...] K -->|peekLinearIssueWorkContext| L[AgentChatPane initialLinearIssueContext] L -->|consumed| M[consumeLinearIssueWorkContext clears singleton]Comments Outside Diff (1)
apps/desktop/src/renderer/components/app/LinearQuickViewButton.tsx, line 1328-1337 (link)busyModalflicker inhandleResolveInExistingLanehandleResolveInExistingLaneis declaredasyncand wraps a synchronous block intry/finally.setBusyModal("resolve-existing-lane")is set and then immediately cleared in the same microtask, so the loading indicator on the resolve-action buttons never renders. Consider either making the function synchronous or adding anawaitbeforeopenWorkDraftif async navigation is expected.Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (3): Last reviewed commit: "ship: iteration 1 — fix test-desktop (2)..." | Re-trigger Greptile