Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Owner
Author
|
@copilot review but do not make fixes |
📝 WalkthroughWalkthroughThe PR adds GitHub repository access verification with a new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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 |
Copilot stopped work on behalf of
arul28 due to an error
April 27, 2026 04:29
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/desktop/src/renderer/components/automations/components/RuleEditorPanel.tsx (2)
459-486:⚠️ Potential issue | 🟠 Major
missionsEnabledis declared as a required prop but never destructured/used.The destructuring at lines 459–471 drops
missionsEnabled, but line 477 still requires it in the prop type. The parent (RulesTab.tsx:645) keeps passingmissionsEnabled={missionsEnabled}, so today this just silently ignores the value, but the type still forces every caller to thread it through. This contradicts the AI summary's claim that the parameter was removed and leaves a dead, mandatory contract on the component.Either fully remove the prop (and stop passing it from
RulesTab.tsx) or actually consume it. Recommended cleanup:♻️ Drop the unused prop from the type
}: { draft: AutomationRuleDraft; setDraft: (draft: AutomationRuleDraft) => void; lanes: Array<{ id: string; name: string }>; suites: TestSuiteDefinition[]; - missionsEnabled: boolean; issues: AutomationDraftIssue[]; requiredConfirmations: AutomationDraftConfirmationRequirement[];And in
apps/desktop/src/renderer/components/automations/RulesTab.tsxaround line 645, drop the now-unused prop:<RuleEditorPanel draft={draft} setDraft={setDraft} lanes={lanes.map((lane) => ({ id: lane.id, name: lane.name }))} suites={suites} - missionsEnabled={missionsEnabled} issues={issues}(Verify
missionsEnabledis still needed elsewhere inRulesTab.tsx; if not, remove its computation too.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/automations/components/RuleEditorPanel.tsx` around lines 459 - 486, RuleEditorPanel declares missionsEnabled in its prop type but does not destructure or use it; remove missionsEnabled from the RuleEditorPanel prop type and its parameter list (the function signature) and then remove the missionsEnabled={missionsEnabled} prop passed from RulesTab (or stop computing it there if unused) so the component signature and callers are consistent; search for RuleEditorPanel and RulesTab to update the component props and the caller(s) that pass missionsEnabled accordingly.
214-226:⚠️ Potential issue | 🟡 MinorNormalize legacy
git.pr_*trigger types before saving to prevent UI select mismatches.When a persisted rule carries a
git.pr_*type (valid perAUTOMATION_TRIGGER_TYPES), the renderer maps it to thegithubfamily intriggerFamilyForType()(line 217). However,TRIGGER_OPTIONS.githubonly lists the canonicalgithub.pr_*variants, leaving the select element with no matching option. This causes the browser to silently rewrite the type on the next user interaction.The issue is that
normalizeDraft()in the main process validates but does not normalize legacy trigger types before persisting. Agit.pr_*type passes validation and is saved as-is, even thoughnormalizeTriggerType()exists to convert it togithub.pr_*. Consider applying the same normalization during save that already happens at runtime inautomationService.ts(lines 312–316), so persisted rules always use canonical trigger names and render options match their values.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/automations/components/RuleEditorPanel.tsx` around lines 214 - 226, Persisted rules may contain legacy git.pr_* trigger types which map to the github family in triggerFamilyForType but don't match TRIGGER_OPTIONS.github, causing the select to lose its option; fix this by applying normalizeTriggerType when saving/normalizing drafts (the same normalization used in automationService.normalizeDraft flow) so that normalizeDraft (or the save path) converts git.pr_* to github.pr_* before validation/persisting; update the save/normalize path that calls normalizeDraft to call normalizeTriggerType on rule.trigger.type (or run the existing normalizeTriggerType utility) so persisted rules always use canonical github.pr_* names and the UI select matches.apps/desktop/src/renderer/components/terminals/SessionListPane.tsx (1)
49-76:⚠️ Potential issue | 🟡 MinorLane accent applied as raw color may hurt label readability and breaks header hover.
Three concerns with the new
accentColorstyling (all stem from the same wiring):
- Label contrast (line 73):
style={{ color: accentColor }}paints the label in the lane's raw hex. With dark or low-luminance lane colors on the dark sidebar surface, the label can become hard to read. The icon path already keeps a sane fallback (var(--color-muted-fg)), but the label gets the raw color with no luminance handling. Consider mixing toward a foreground-readable tint, e.g.color-mix(in srgb, ${accentColor} 70%, var(--color-fg)), or only tinting the icon/border and leaving the label attext-fg/90.- Hover background suppressed (lines 56–58):
style.background = accentBgis always set (even in the no-accent fallback), so thehover:bg-white/[0.04]class on the same element has no effect — inline styles win over the:hoverclass. If hover feedback is desired, drive the background from a CSS variable + class, or move the hover state into JS (onMouseEnter/Leave) to override the inline value.- 2px alignment shift (line 60): The left border is only added when
accentColoris truthy, so in the by-lane view, lanes withlane.colorset are indented 2px further than lanes without a color and than the orphan-lane fallback at lines 354–371. Either always reserve 2px (transparent border when no accent) or pass through an accent for the orphan rows so groups line up.♻️ Suggested adjustment
- const accentBg = accentColor - ? `color-mix(in srgb, ${accentColor} 8%, transparent)` - : "rgba(255, 255, 255, 0.02)"; + const accentBg = accentColor + ? `color-mix(in srgb, ${accentColor} 8%, transparent)` + : "transparent"; return ( <div className="mt-0.5 first:mt-0"> <button type="button" className="sticky top-0 z-10 flex w-full items-center gap-1.5 rounded-md px-2 py-1.5 text-left transition-colors backdrop-blur-xl cursor-pointer select-none hover:bg-white/[0.04]" style={{ background: accentBg, borderBottom: "1px solid rgba(255, 255, 255, 0.04)", - borderLeft: accentColor ? `2px solid ${accentColor}` : undefined, + borderLeft: `2px solid ${accentColor ?? "transparent"}`, }} ... > ... - <span - className="min-w-0 flex-1 truncate text-[11px] font-semibold text-fg/90" - style={accentColor ? { color: accentColor } : undefined} - > + <span + className="min-w-0 flex-1 truncate text-[11px] font-semibold text-fg/90" + style={ + accentColor + ? { color: `color-mix(in srgb, ${accentColor} 70%, var(--color-fg))` } + : undefined + } + > {label} </span>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/terminals/SessionListPane.tsx` around lines 49 - 76, The current accent wiring in SessionListPane causes readability and hover issues: change the label span so it doesn't use the raw accentColor (replace style={{ color: accentColor }} with a readable mix, e.g. color-mix(in srgb, ${accentColor} 70%, var(--color-fg)) or keep text-fg/90 and only tint icon/border), stop overriding the :hover class by removing the always-set inline background when no accent (drive background via a CSS variable/class or only set style.background when accentColor is present), and ensure the left border reservation is consistent by always setting borderLeft (use a transparent 2px border when accentColor is falsy) so rows remain aligned; look for accentBg, accentColor, the style prop on the container button, the borderLeft assignment, and the label span to apply these fixes.
🧹 Nitpick comments (6)
apps/desktop/src/renderer/components/automations/ActionList.tsx (1)
190-282: Optional: addaria-haspopup/aria-expandedto the menu trigger.The trigger button opens a
role="menu"popup but doesn't advertise that to assistive tech. Addingaria-haspopup="menu"andaria-expanded={open}would let screen readers announce the toggle state. Same applies to theActionPickercombobox inAdeActionEditor.tsx.♻️ Suggested change
<button type="button" onClick={() => setOpen((value) => !value)} + aria-haspopup="menu" + aria-expanded={open} className={cn(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/automations/ActionList.tsx` around lines 190 - 282, The AddStepBar trigger button does not announce the menu state to assistive tech; update the button in AddStepBar to include aria-haspopup="menu" and aria-expanded={open} so screen readers know it opens a menu and its current state, and apply the same pattern to the ActionPicker trigger in AdeActionEditor.tsx (add aria-haspopup="listbox" or "menu" and aria-expanded bound to its open state) to ensure proper accessibility semantics.apps/desktop/src/renderer/components/automations/adeActionSchemas.ts (1)
2100-2102: Optional: index schemas bydomain.actionfor O(1) lookups.
findAdeActionSchemais called per render inAdeActionEditor(and once per registry item insideActionPicker.matches), each time doing a linear scan over ~200+ entries. Building aMap<string, AdeActionSchema>keyed by${domain}.${action}once at module load would remove the quadratic factor in the picker's match loop.♻️ Suggested change
-export function findAdeActionSchema(domain: string, action: string): AdeActionSchema | undefined { - return ADE_ACTION_SCHEMAS.find((s) => s.domain === domain && s.action === action); -} +const SCHEMA_INDEX: ReadonlyMap<string, AdeActionSchema> = new Map( + ADE_ACTION_SCHEMAS.map((s) => [`${s.domain}.${s.action}`, s]), +); + +export function findAdeActionSchema(domain: string, action: string): AdeActionSchema | undefined { + return SCHEMA_INDEX.get(`${domain}.${action}`); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/automations/adeActionSchemas.ts` around lines 2100 - 2102, Replace the O(n) scan in findAdeActionSchema with an indexed Map built once at module load: create a module-level Map<string, AdeActionSchema> (key = `${domain}.${action}`) populated from ADE_ACTION_SCHEMAS when the file initializes, then change findAdeActionSchema(domain, action) to compute the same key and return map.get(key). Ensure the Map is exported or referenced in this module only and remove the .find usage so callers (e.g., AdeActionEditor and ActionPicker.matches) get O(1) lookups.apps/desktop/src/main/services/lanes/laneService.test.ts (1)
2307-2326: Add one more idempotency regression test for case-only color differences.Current idempotent coverage is same-string only. Add a case-variant scenario (same logical hex color, different case) to lock behavior.
Suggested test addition
+ it("treats same-color different-case updates as idempotent for conflict probing", async () => { + const { db, service } = await setup(); + db.run("update lanes set color = ? where id = ?", ["#A78BFA", "lane-parent"]); + db.run("update lanes set color = ? where id = ?", ["#a78bfa", "lane-child"]); + + expect(() => + service.updateAppearance({ laneId: "lane-child", color: "#A78BFA", icon: "star" }), + ).not.toThrow(); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/lanes/laneService.test.ts` around lines 2307 - 2326, Add a new test (or extend the existing idempotency test) that covers case-only hex differences: seed two lanes ("lane-parent" and "lane-child") with the same logical color but different casing (e.g., db.run set parent to "#A78BFA" and child to "#a78bfa"), then call service.updateAppearance({ laneId: "lane-child", color: "#A78BFA", icon: "star" }) and assert it does not throw and that the stored color/icon for "lane-child" are updated as expected; reference the existing test harness, the service.updateAppearance call, and the DB queries/selects used to verify color and icon.apps/ios/ADE/Views/Lanes/LaneComponents.swift (1)
353-360: Consider computinglaneAccentonce inLaneStackCardfor clarity.You currently resolve the same palette color twice in the same block; caching it locally would simplify the view and avoid duplicate work.
♻️ Optional cleanup
HStack(alignment: .center, spacing: 10) { LaneStatusIndicator(bucket: snapshot.runtime.bucket, size: 10) .adeMatchedGeometry(id: isSelectedTransitionSource ? "lane-icon-\(snapshot.lane.id)" : nil, in: transitionNamespace) - if let laneAccent = LaneColorPalette.color(forHex: snapshot.lane.color) { + let laneAccent = LaneColorPalette.color(forHex: snapshot.lane.color) + if let laneAccent { Circle() .fill(laneAccent) .frame(width: 7, height: 7) } Text(snapshot.lane.name) .font(.subheadline.weight(.semibold)) - .foregroundStyle(LaneColorPalette.color(forHex: snapshot.lane.color) ?? ADEColor.textPrimary) + .foregroundStyle(laneAccent ?? ADEColor.textPrimary)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ios/ADE/Views/Lanes/LaneComponents.swift` around lines 353 - 360, Compute the lane accent once in the LaneStackCard view by assigning LaneColorPalette.color(forHex: snapshot.lane.color) to a local let (e.g., laneAccent) and reuse that variable for both the Circle().fill(...) and the Text.foregroundStyle(...) fallback, replacing the second call to LaneColorPalette.color(forHex: ...) so you don't resolve the palette twice and the view is clearer.apps/desktop/src/main/services/github/githubService.test.ts (1)
564-572: Optional: tighten the missing-token assertion.This test relies on
getStatus()not issuing any HTTP probes when the token is absent, but it doesn't verify that. Addingexpect(mockFetch).not.toHaveBeenCalled()would lock in the "no network when unauthenticated" contract and make a regression here visible as a failing test rather than a vague mock state.♻️ Suggested addition
it("missing token returns not-connected status with all the new fields populated", async () => { stubOriginRemote(); const status = await makeService().getStatus(); expect(status.tokenStored).toBe(false); expect(status.connected).toBe(false); expect(status.repoAccessOk).toBeNull(); expect(status.repoAccessError).toBeNull(); + expect(mockFetch).not.toHaveBeenCalled(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/github/githubService.test.ts` around lines 564 - 572, Tighten the "missing token" test by asserting that no HTTP probes were made: after calling makeService().getStatus() in the test, add an assertion that mockFetch (the fetch/mock used by the githubService tests) was not called to ensure getStatus() does not perform network requests when no token is stored; locate this in the "missing token returns not-connected status..." test that uses stubOriginRemote(), makeService(), and getStatus().apps/desktop/src/renderer/components/prs/tabs/GitHubTab.tsx (1)
174-178: Consider precomputing a laneId→color map once.This hook performs a linear
findon every call. For long PR lists, a memoized map in the parent would reduce repeated lookups.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/prs/tabs/GitHubTab.tsx` around lines 174 - 178, The hook useLaneColorById does a linear find each call; instead compute a laneId→color map once (e.g., in the parent component GitHubTab using useMemo over s.lanes from useAppStore) and then either (a) replace useLaneColorById with a hook that accepts the precomputed map or (b) add a new selector/useMemo named laneColorMap and change call sites to index into laneColorMap[laneId] — update all usages of useLaneColorById to use the map to avoid repeated s.lanes.find scans.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/main/services/github/githubService.ts`:
- Around line 388-390: The classic-PAT branch currently returns
access.hasRequiredAccess unconditionally; change it so that when args.tokenType
=== "classic" you also require repoAccessOk when a repo is known (i.e., return
access.hasRequiredAccess && repoAccessOk if repo is present), using the same
repoAccessOk value used elsewhere in this function, so classic tokens are only
considered connected when they both have required scopes
(getGitHubTokenAccessState(...) -> access.hasRequiredAccess) and actual access
to the active repo.
- Around line 215-238: The probeRepoAccess function currently calls fetch
without an AbortController; wrap the fetch in an AbortController with a timeout
(reuse the same pattern as detectRepo) by creating a controller, passing
controller.signal to fetch, and scheduling a timer (e.g., setTimeout) to call
controller.abort() after the desired timeout; clear the timer on success or
error and ensure the catch branch treats an aborted request (AbortError) the
same way as other errors (returning { ok: false, error: ... }). Update
probeRepoAccess to add the controller, signal, timeout, and proper cleanup
around the existing fetch logic.
In `@apps/desktop/src/main/services/lanes/laneService.ts`:
- Around line 2883-2896: The current check in laneService.ts runs the conflict
query even when the only change is case (e.g. "#a78bfa" -> "#A78BFA"); update
the conditional that guards the DB conflict probe to perform a case-insensitive
idempotence check first (compare normalizedColor.toLowerCase() with
lane.color?.toLowerCase()) and only run the db.get conflict query when those
lowercase values differ; keep the existing conflict query and error throw in
place (look for the normalizedColor check and the db.get block).
In `@apps/desktop/src/renderer/browserMock.ts`:
- Around line 3059-3101: The mock returns static snapshots and leaves
onStatusChanged as noop, so AppShell doesn't receive immediate banner updates;
update the browser mock to support subscribing and emitting status changes: add
an internal listener list and implement onStatusChanged to register callbacks
(and return an unsubscribe), then modify setToken (resolvedArg) and clearToken
(resolved) handlers to update the internal status object (same shape as
getStatus) and invoke all registered listeners with the new status; ensure
detectRepo/listRepo* remain unchanged and use the same status shape fields
(tokenStored, tokenType, userLogin, scopes, checkedAt, repoAccessOk, connected,
etc.) so AppShell's reactions work in browser-preview.
In `@apps/desktop/src/renderer/components/automations/AdeActionEditor.tsx`:
- Around line 495-521: The json branch in AdeActionEditor.tsx currently calls
onChange(raw) when JSON.parse fails, which stores a malformed string into a
json-typed param; update the textarea onChange handler (the block handling
param.type === "json") to stop writing the raw string back via onChange on parse
error and instead retain the previous parsed value (value) and set a local parse
error state (similar to JsonArgsEditor's parseError pattern) to surface an
inline error message; only call onChange with the parsed object when JSON.parse
succeeds and ensure the placeholder/textarea still displays the user's raw text
without persisting it into args until it parses.
- Around line 309-324: The component currently coerces array-shaped
AdeActionValue.args into an empty object and then setArg (in ActionParamsEditor)
overwrites the original array, silently destroying data; update the logic to
detect array-shaped args (e.g., const argsAreArray = Array.isArray(value.args))
and do NOT coerce arrays to {} — if argsAreArray is true, render or switch to
the raw JSON editor (or show a confirmation/warning) and disable the
keyed-schema form and setArg path so edits cannot overwrite the array; keep
setArg and onChange behavior only for object-shaped args (i.e., guard setArg
with !argsAreArray) so legacy/externally authored array payloads are preserved.
- Around line 539-549: In PlaceholderRow the window.setTimeout callback can run
after the component unmounts causing a React state-update warning; fix by
storing the timeout id in a ref (e.g., timeoutRef) whenever you call
window.setTimeout in the copy function, clear any existing timeout before
setting a new one, and add a useEffect cleanup that calls
clearTimeout(timeoutRef.current) on unmount (and also clear the timeout on
subsequent clicks in copy) so setCopied is never called after unmount.
In `@apps/desktop/src/renderer/components/automations/adeActionSchemas.ts`:
- Around line 1-4: Update the top-of-file comment in adeActionSchemas.ts to
reference the full repo-relative path for the ADE actions registry: replace
`src/main/services/adeActions/registry.ts` with
`apps/desktop/src/main/services/adeActions/registry.ts` so the comment in this
file (adeActionSchemas.ts) accurately points to the registry implementation used
by the automation rule editor.
- Line 559: The schema for the "draft" boolean in adeActionSchemas.ts is marked
required: true while also supplying defaultValue: false; update the "draft"
parameter to be optional to match other boolean params (i.e., remove or set
required to false) while keeping defaultValue: false so the UI doesn't force an
explicit toggle; locate the "draft" entry in the schema used by
pr.createFromLane and adjust the required flag accordingly.
In `@apps/desktop/src/renderer/components/lanes/CreateLaneDialog.tsx`:
- Around line 128-133: The effect in CreateLaneDialog.tsx currently sets a
default color whenever selectedColor is null while open, which causes a cleared
selection to be immediately repopulated; change the effect so it only sets a
default color when the dialog is opened (i.e., when open transitions from false
-> true). Implement a prevOpen ref (e.g., prevOpenRef) and in the useEffect
check if open === true && prevOpenRef.current === false && selectedColor ===
null before calling nextAvailableColor(lanes) and setSelectedColor(next); then
update prevOpenRef.current = open—this preserves the nullable selectedColor
while the dialog is open but still assigns a default on initial open.
In `@apps/desktop/src/renderer/components/lanes/laneColorPalette.ts`:
- Around line 29-32: getLaneAccent may index LANE_FALLBACK_COLORS with a
negative fallbackIndex (because JS % preserves sign), returning undefined;
update getLaneAccent to normalize fallbackIndex into the valid 0..length-1 range
before indexing (e.g. compute const idx = ((fallbackIndex % len) + len) % len
where len = LANE_FALLBACK_COLORS.length) and use that idx to select the fallback
color so the function always returns a string.
In `@apps/desktop/src/renderer/components/lanes/LanesPage.tsx`:
- Around line 1658-1663: The catch block swallowing errors when applying a lane
color (inside the if (createSelectedColor) branch around
window.ade.lanes.updateAppearance({ laneId: lane.id, color: createSelectedColor
})) should surface the failure; update the catch to log the caught error with
context (include lane.id and createSelectedColor) and/or trigger a user-visible
notification so color-apply failures are discoverable for debugging and UX.
In `@apps/desktop/src/renderer/components/lanes/ManageLaneDialog.tsx`:
- Around line 298-323: The apply function and the LaneColorPicker onChange
currently allow calls even when an update is already in progress; guard against
concurrent updates by checking the busy state (or a disabled flag) before
initiating updateAppearance. Modify apply to return immediately if busy is true,
and update the onChange handler to avoid invoking apply when busy (e.g.,
onChange={(next) => { if (!busy) void apply(next); }}). Also pass the
busy/disabled state into the LaneColorPicker (if it supports a disabled prop) or
disable its UI while busy so users cannot trigger overlapping updates; keep
existing setBusy/setError behavior around the call to
window.ade.lanes.updateAppearance in apply.
In `@apps/desktop/src/renderer/components/terminals/WorkViewArea.tsx`:
- Around line 510-564: The tab close control is nested inside the outer tab
<button> (in the WorkViewArea tab rendering) which creates invalid interactive
nesting; refactor by moving the close affordance out of the inner content so it
is a sibling element to the tab button (keep selection onClick/onContextMenu on
the main <button> via onSelectItem(session.id) and handleContextMenu(session,
e), and wire the sibling close element to call onCloseItem(session.id>),
preserving e.stopPropagation(), keyboard support (Enter/Space) and isBusy
guarding; remove role="button" and tabIndex from the inner markup and replicate
the visual styles (width/height/color/opacity/group-hover behavior and X icon)
on the new sibling element; apply the same change for the second equivalent tab
rendering (the other tab list) so both places use a sibling close control
instead of nested interactive elements.
In `@apps/ios/ADE/Views/Lanes/LaneColorPalette.swift`:
- Around line 36-38: The Scanner usage in the color parsing block (variables
cleaned and value) can return true after only consuming a prefix; update the
logic to create a Scanner instance (e.g., let scanner = Scanner(string:
cleaned)), call scanner.scanHexInt64(&value) and then validate the scanner
consumed the entire string by checking scanner.scanLocation == cleaned.count (or
perform explicit char validation) before proceeding; ensure you still respect
the existing length guards (cleaned.count == 6 || cleaned.count == 8) and return
nil if the scanner did not consume the full input.
In `@apps/ios/ADE/Views/Lanes/LaneColorSwatchPicker.swift`:
- Around line 39-61: The "No color" Button does not expose selection state to
VoiceOver; update the Button (the Button that calls onSelect(nil) and visually
checks selectedHex == nil) to add accessibilityAddTraits so it includes
.isSelected when selectedHex == nil and always includes .isButton, matching the
color swatch pattern (use accessibilityAddTraits(selectedHex == nil ?
[.isSelected, .isButton] : [.isButton])).
In `@apps/ios/ADE/Views/Lanes/LaneCreateSheet.swift`:
- Around line 339-345: The catch block that swallows errors when calling
syncService.updateLaneAppearance(created.id, color: hex) should present
non‑fatal user feedback and log the error rather than silently ignoring it;
update the catch to (1) log the caught error (e.g., via a logger or print) and
(2) surface a UI notification by setting/using a local `@State` or viewModel
property (e.g., showColorApplyError / colorApplyErrorMessage) so the sheet can
display a non‑blocking alert/toast indicating the color couldn't be applied
while leaving the lane creation successful. Ensure this change is made inside
the existing if let selectedColorHex ... try await ... catch block so behavior
remains non‑fatal.
In `@apps/ios/ADE/Views/Lanes/LaneListViewParts.swift`:
- Around line 513-522: The applyLaneColor(_ hex: String?, to laneId: String)
async method calls syncService.updateLaneAppearance(...) but then reloads the UI
with reload(refreshRemote: false), causing stale cached data; change the reload
call to reload(refreshRemote: true) so the UI performs a server refresh after
updateLaneAppearance and immediately reflects the color change. Ensure you only
modify the reload(...) invocation in applyLaneColor to pass true.
In `@apps/ios/ADE/Views/Lanes/LaneTreeView.swift`:
- Around line 166-175: The headline Text should not reuse the resolved
laneAccent tint because some palette colors fail WCAG contrast; update the views
that currently color the headline (the Text(snapshot.lane.name) usage in
LaneTreeView / LaneComponents.swift and the similar usage in
WorkRootComponents.swift) to always use ADEColor.textPrimary for the headline
while leaving the Circle().fill(laneAccent) (and LaneStatusIndicator) to convey
the color; alternatively, if you prefer dynamic coloring, run laneAccent through
a luminance-contrast helper (create/apply a helper named something like
accessibleAccent(for:)) and only apply the adjusted color to Text when it meets
a 3:1 contrast ratio against ADEColor.surfaceBackground, falling back to
ADEColor.textPrimary otherwise.
In `@apps/ios/ADE/Views/Work/WorkRootComponents.swift`:
- Around line 700-711: The lane name text is using the raw
LaneColorPalette.color(forHex:) which can yield poor contrast; add a new
LaneColorPalette.textColor(forHex:) that adjusts luminance or enforces a minimum
contrast (e.g., lighten/darken or pick white/black fallback) and update the
Text(session.laneName).foregroundStyle(...) to use
LaneColorPalette.textColor(forHex: lane?.color) while keeping
Circle().fill(LaneColorPalette.color(forHex: lane?.color)) for the 6×6 accent
dot so the accent remains the raw user color and the label uses the
contrast-safe color.
---
Outside diff comments:
In
`@apps/desktop/src/renderer/components/automations/components/RuleEditorPanel.tsx`:
- Around line 459-486: RuleEditorPanel declares missionsEnabled in its prop type
but does not destructure or use it; remove missionsEnabled from the
RuleEditorPanel prop type and its parameter list (the function signature) and
then remove the missionsEnabled={missionsEnabled} prop passed from RulesTab (or
stop computing it there if unused) so the component signature and callers are
consistent; search for RuleEditorPanel and RulesTab to update the component
props and the caller(s) that pass missionsEnabled accordingly.
- Around line 214-226: Persisted rules may contain legacy git.pr_* trigger types
which map to the github family in triggerFamilyForType but don't match
TRIGGER_OPTIONS.github, causing the select to lose its option; fix this by
applying normalizeTriggerType when saving/normalizing drafts (the same
normalization used in automationService.normalizeDraft flow) so that
normalizeDraft (or the save path) converts git.pr_* to github.pr_* before
validation/persisting; update the save/normalize path that calls normalizeDraft
to call normalizeTriggerType on rule.trigger.type (or run the existing
normalizeTriggerType utility) so persisted rules always use canonical
github.pr_* names and the UI select matches.
In `@apps/desktop/src/renderer/components/terminals/SessionListPane.tsx`:
- Around line 49-76: The current accent wiring in SessionListPane causes
readability and hover issues: change the label span so it doesn't use the raw
accentColor (replace style={{ color: accentColor }} with a readable mix, e.g.
color-mix(in srgb, ${accentColor} 70%, var(--color-fg)) or keep text-fg/90 and
only tint icon/border), stop overriding the :hover class by removing the
always-set inline background when no accent (drive background via a CSS
variable/class or only set style.background when accentColor is present), and
ensure the left border reservation is consistent by always setting borderLeft
(use a transparent 2px border when accentColor is falsy) so rows remain aligned;
look for accentBg, accentColor, the style prop on the container button, the
borderLeft assignment, and the label span to apply these fixes.
---
Nitpick comments:
In `@apps/desktop/src/main/services/github/githubService.test.ts`:
- Around line 564-572: Tighten the "missing token" test by asserting that no
HTTP probes were made: after calling makeService().getStatus() in the test, add
an assertion that mockFetch (the fetch/mock used by the githubService tests) was
not called to ensure getStatus() does not perform network requests when no token
is stored; locate this in the "missing token returns not-connected status..."
test that uses stubOriginRemote(), makeService(), and getStatus().
In `@apps/desktop/src/main/services/lanes/laneService.test.ts`:
- Around line 2307-2326: Add a new test (or extend the existing idempotency
test) that covers case-only hex differences: seed two lanes ("lane-parent" and
"lane-child") with the same logical color but different casing (e.g., db.run set
parent to "#A78BFA" and child to "#a78bfa"), then call
service.updateAppearance({ laneId: "lane-child", color: "#A78BFA", icon: "star"
}) and assert it does not throw and that the stored color/icon for "lane-child"
are updated as expected; reference the existing test harness, the
service.updateAppearance call, and the DB queries/selects used to verify color
and icon.
In `@apps/desktop/src/renderer/components/automations/ActionList.tsx`:
- Around line 190-282: The AddStepBar trigger button does not announce the menu
state to assistive tech; update the button in AddStepBar to include
aria-haspopup="menu" and aria-expanded={open} so screen readers know it opens a
menu and its current state, and apply the same pattern to the ActionPicker
trigger in AdeActionEditor.tsx (add aria-haspopup="listbox" or "menu" and
aria-expanded bound to its open state) to ensure proper accessibility semantics.
In `@apps/desktop/src/renderer/components/automations/adeActionSchemas.ts`:
- Around line 2100-2102: Replace the O(n) scan in findAdeActionSchema with an
indexed Map built once at module load: create a module-level Map<string,
AdeActionSchema> (key = `${domain}.${action}`) populated from ADE_ACTION_SCHEMAS
when the file initializes, then change findAdeActionSchema(domain, action) to
compute the same key and return map.get(key). Ensure the Map is exported or
referenced in this module only and remove the .find usage so callers (e.g.,
AdeActionEditor and ActionPicker.matches) get O(1) lookups.
In `@apps/desktop/src/renderer/components/prs/tabs/GitHubTab.tsx`:
- Around line 174-178: The hook useLaneColorById does a linear find each call;
instead compute a laneId→color map once (e.g., in the parent component GitHubTab
using useMemo over s.lanes from useAppStore) and then either (a) replace
useLaneColorById with a hook that accepts the precomputed map or (b) add a new
selector/useMemo named laneColorMap and change call sites to index into
laneColorMap[laneId] — update all usages of useLaneColorById to use the map to
avoid repeated s.lanes.find scans.
In `@apps/ios/ADE/Views/Lanes/LaneComponents.swift`:
- Around line 353-360: Compute the lane accent once in the LaneStackCard view by
assigning LaneColorPalette.color(forHex: snapshot.lane.color) to a local let
(e.g., laneAccent) and reuse that variable for both the Circle().fill(...) and
the Text.foregroundStyle(...) fallback, replacing the second call to
LaneColorPalette.color(forHex: ...) so you don't resolve the palette twice and
the view is clearer.
🪄 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: 60de06ca-c44c-4517-a98b-3dbd877e71dc
⛔ Files ignored due to path filters (7)
docs/features/automations/README.mdis excluded by!docs/**docs/features/automations/triggers-and-actions.mdis excluded by!docs/**docs/features/lanes/README.mdis excluded by!docs/**docs/features/onboarding-and-settings/README.mdis excluded by!docs/**docs/features/pull-requests/README.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/**
📒 Files selected for processing (52)
.claude/commands/finalize.mdapps/desktop/src/main/services/github/githubService.test.tsapps/desktop/src/main/services/github/githubService.tsapps/desktop/src/main/services/ipc/registerIpc.tsapps/desktop/src/main/services/lanes/laneService.test.tsapps/desktop/src/main/services/lanes/laneService.tsapps/desktop/src/preload/global.d.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/browserMock.tsapps/desktop/src/renderer/components/app/AppShell.tsxapps/desktop/src/renderer/components/automations/ActionList.tsxapps/desktop/src/renderer/components/automations/ActionRow.tsxapps/desktop/src/renderer/components/automations/AdeActionEditor.tsxapps/desktop/src/renderer/components/automations/adeActionSchemas.tsapps/desktop/src/renderer/components/automations/components/RuleEditorPanel.tsxapps/desktop/src/renderer/components/automations/shared.tsapps/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/ChatGitToolbar.tsxapps/desktop/src/renderer/components/lanes/CreateLaneDialog.tsxapps/desktop/src/renderer/components/lanes/LaneAccentDot.tsxapps/desktop/src/renderer/components/lanes/LaneColorPicker.tsxapps/desktop/src/renderer/components/lanes/LaneContextMenu.tsxapps/desktop/src/renderer/components/lanes/LaneStackPane.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/laneUtils.test.tsapps/desktop/src/renderer/components/missions/ManageMissionDialog.tsxapps/desktop/src/renderer/components/missions/MissionHeader.tsxapps/desktop/src/renderer/components/prs/tabs/GitHubTab.tsxapps/desktop/src/renderer/components/prs/tabs/QueueTab.tsxapps/desktop/src/renderer/components/settings/GitHubSection.tsxapps/desktop/src/renderer/components/shared/ProviderModelSelector.tsxapps/desktop/src/renderer/components/terminals/SessionCard.tsxapps/desktop/src/renderer/components/terminals/SessionListPane.tsxapps/desktop/src/renderer/components/terminals/WorkViewArea.tsxapps/desktop/src/renderer/components/terminals/workSessionTiling.test.tsapps/desktop/src/renderer/components/terminals/workSessionTiling.tsapps/desktop/src/renderer/components/ui/SmartTooltip.tsxapps/desktop/src/shared/ipc.tsapps/desktop/src/shared/types/git.tsapps/ios/ADE/Views/Lanes/LaneColorPalette.swiftapps/ios/ADE/Views/Lanes/LaneColorSwatchPicker.swiftapps/ios/ADE/Views/Lanes/LaneComponents.swiftapps/ios/ADE/Views/Lanes/LaneCreateSheet.swiftapps/ios/ADE/Views/Lanes/LaneListViewParts.swiftapps/ios/ADE/Views/Lanes/LaneManageSheet.swiftapps/ios/ADE/Views/Lanes/LaneTreeView.swiftapps/ios/ADE/Views/Work/WorkRootComponents.swift
💤 Files with no reviewable changes (1)
- apps/desktop/src/renderer/components/automations/shared.ts
Comment on lines
+215
to
+238
| const probeRepoAccess = async ( | ||
| token: string, | ||
| repo: GitHubRepoRef, | ||
| ): Promise<{ ok: boolean; error: string | null }> => { | ||
| try { | ||
| const response = await fetch( | ||
| `https://api.github.com/repos/${encodeURIComponent(repo.owner)}/${encodeURIComponent(repo.name)}`, | ||
| { | ||
| method: "GET", | ||
| headers: { | ||
| accept: "application/vnd.github+json", | ||
| authorization: `Bearer ${token}`, | ||
| "user-agent": "ade-desktop", | ||
| }, | ||
| }, | ||
| ); | ||
| if (response.ok) return { ok: true, error: null }; | ||
| const payload = (await response.json().catch(() => ({}))) as Record<string, unknown>; | ||
| const message = asString(payload.message) || `HTTP ${response.status}`; | ||
| return { ok: false, error: `${response.status}: ${message}` }; | ||
| } catch (error) { | ||
| return { ok: false, error: error instanceof Error ? error.message : String(error) }; | ||
| } | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n apps/desktop/src/main/services/github/githubService.ts | head -300Repository: arul28/ADE
Length of output: 12832
🏁 Script executed:
cat -n apps/desktop/src/main/services/github/githubService.ts | sed -n '238,400p'Repository: arul28/ADE
Length of output: 7282
🏁 Script executed:
cat -n apps/desktop/src/main/services/github/githubService.ts | sed -n '396,450p'Repository: arul28/ADE
Length of output: 2412
🏁 Script executed:
cat -n apps/desktop/src/main/services/github/githubService.ts | sed -n '450,480p'Repository: arul28/ADE
Length of output: 1129
🏁 Script executed:
rg -n "AbortController|timeoutMs" apps/desktop/src/main/services/github/githubService.tsRepository: arul28/ADE
Length of output: 160
🏁 Script executed:
rg -n "AbortController" apps/desktop/src/main --type ts --type tsx -B 2 -A 2Repository: arul28/ADE
Length of output: 82
🏁 Script executed:
rg -n "AbortController" apps/desktop/src/main -B 2 -A 2Repository: arul28/ADE
Length of output: 24595
🏁 Script executed:
rg -n "getStatus" apps/desktop/src/main/services/github/githubService.ts | head -20Repository: arul28/ADE
Length of output: 171
🏁 Script executed:
rg -n "github.*getStatus\|getStatus.*github" apps/desktop/src/main --type ts -B 2 -A 2 | head -50Repository: arul28/ADE
Length of output: 36
🏁 Script executed:
grep -r "getStatus" apps/desktop/src/main --include="*.ts" | grep -i "github\|service" | head -20Repository: arul28/ADE
Length of output: 2298
🏁 Script executed:
rg -n "github.*\.getStatus\|githubService.*getStatus" apps/desktop/src --type ts -B 2 -A 2 | head -60Repository: arul28/ADE
Length of output: 36
🏁 Script executed:
sed -n '640,660p' apps/desktop/src/main/services/github/githubService.tsRepository: arul28/ADE
Length of output: 532
Add a timeout to the repo probe.
The probeRepoAccess fetch at lines 220–229 runs without an AbortController or timeout signal. If GitHub stalls after /user succeeds, getStatus() never settles and the Settings UI can hang indefinitely on the main process. Wrap the probe in an AbortController with a timeout, following the pattern already established in detectRepo() (line 180) and throughout the codebase (e.g., usageTrackingService, modelsDevService).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/services/github/githubService.ts` around lines 215 -
238, The probeRepoAccess function currently calls fetch without an
AbortController; wrap the fetch in an AbortController with a timeout (reuse the
same pattern as detectRepo) by creating a controller, passing controller.signal
to fetch, and scheduling a timer (e.g., setTimeout) to call controller.abort()
after the desired timeout; clear the timer on success or error and ensure the
catch branch treats an aborted request (AbortError) the same way as other errors
(returning { ok: false, error: ... }). Update probeRepoAccess to add the
controller, signal, timeout, and proper cleanup around the existing fetch logic.
Comment on lines
+388
to
+390
| if (args.tokenType === "classic") { | ||
| const access = getGitHubTokenAccessState(args.scopes); | ||
| return access.hasRequiredAccess; |
There was a problem hiding this comment.
Don’t report classic PATs as connected when repo access failed.
connected is now the renderer’s “GitHub is usable for this project” flag, but this branch ignores repoAccessOk for classic tokens. A classic PAT with the right scopes but no access to the active repo will still come back as connected, which leads to a green connected state alongside a repo-access error. When repo is known, classic tokens should also require repoAccessOk === true.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/services/github/githubService.ts` around lines 388 -
390, The classic-PAT branch currently returns access.hasRequiredAccess
unconditionally; change it so that when args.tokenType === "classic" you also
require repoAccessOk when a repo is known (i.e., return access.hasRequiredAccess
&& repoAccessOk if repo is present), using the same repoAccessOk value used
elsewhere in this function, so classic tokens are only considered connected when
they both have required scopes (getGitHubTokenAccessState(...) ->
access.hasRequiredAccess) and actual access to the active repo.
Comment on lines
+2883
to
+2896
| if (normalizedColor && normalizedColor !== lane.color) { | ||
| const conflict = db.get<{ name: string }>( | ||
| `select name from lanes | ||
| where project_id = ? | ||
| and id != ? | ||
| and archived_at is null | ||
| and lower(color) = lower(?) | ||
| limit 1`, | ||
| [projectId, laneId, normalizedColor] | ||
| ); | ||
| if (conflict) { | ||
| throw new Error(`Color already in use by lane "${conflict.name}"`); | ||
| } | ||
| } |
There was a problem hiding this comment.
Make idempotent color comparison case-insensitive before conflict probing.
Right now, case-only changes (e.g. #a78bfa → #A78BFA) still trigger the conflict query. On legacy duplicated colors, that can block non-color updates unexpectedly.
Proposed fix
- if (normalizedColor && normalizedColor !== lane.color) {
+ const nextColorKey = normalizedColor?.trim().toLowerCase() ?? null;
+ const currentColorKey = lane.color?.trim().toLowerCase() ?? null;
+ if (nextColorKey && nextColorKey !== currentColorKey) {
const conflict = db.get<{ name: string }>(
`select name from lanes
where project_id = ?
and id != ?
and archived_at is null
- and lower(color) = lower(?)
+ and lower(trim(color)) = ?
limit 1`,
- [projectId, laneId, normalizedColor]
+ [projectId, laneId, nextColorKey]
);
if (conflict) {
throw new Error(`Color already in use by lane "${conflict.name}"`);
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/services/lanes/laneService.ts` around lines 2883 -
2896, The current check in laneService.ts runs the conflict query even when the
only change is case (e.g. "#a78bfa" -> "#A78BFA"); update the conditional that
guards the DB conflict probe to perform a case-insensitive idempotence check
first (compare normalizedColor.toLowerCase() with lane.color?.toLowerCase()) and
only run the db.get conflict query when those lowercase values differ; keep the
existing conflict query and error throw in place (look for the normalizedColor
check and the db.get block).
Comment on lines
+3059
to
+3101
| getStatus: resolved({ | ||
| tokenStored: true, | ||
| tokenDecryptionFailed: false, | ||
| storageScope: "app", | ||
| tokenType: "classic", | ||
| repo: { owner: "arul28", name: "ADE" }, | ||
| userLogin: "arul", | ||
| scopes: ["repo", "workflow"], | ||
| checkedAt: new Date().toISOString(), | ||
| repoAccessOk: true, | ||
| repoAccessError: null, | ||
| connected: true, | ||
| }), | ||
| setToken: resolvedArg({ | ||
| tokenStored: true, | ||
| tokenDecryptionFailed: false, | ||
| storageScope: "app", | ||
| tokenType: "classic", | ||
| repo: { owner: "arul28", name: "ADE" }, | ||
| userLogin: "arul", | ||
| scopes: ["repo", "workflow"], | ||
| checkedAt: new Date().toISOString(), | ||
| repoAccessOk: true, | ||
| repoAccessError: null, | ||
| connected: true, | ||
| }), | ||
| clearToken: resolved({ | ||
| tokenStored: false, | ||
| tokenDecryptionFailed: false, | ||
| storageScope: "app", | ||
| tokenType: "unknown", | ||
| repo: { owner: "arul28", name: "ADE" }, | ||
| userLogin: null, | ||
| scopes: [], | ||
| checkedAt: null, | ||
| repoAccessOk: null, | ||
| repoAccessError: null, | ||
| connected: false, | ||
| }), | ||
| detectRepo: resolved({ owner: "arul28", name: "ADE" }), | ||
| listRepoLabels: resolved([]), | ||
| listRepoCollaborators: resolved([]), | ||
| onStatusChanged: noop, |
There was a problem hiding this comment.
Make the GitHub browser mock emit status changes.
AppShell now depends on onStatusChanged for immediate banner updates, but this mock still returns fixed snapshots and onStatusChanged: noop. In browser-preview mode, token save/clear flows will stay stale and won't validate the new behavior.
Suggested direction
+let mockGithubStatus = {
+ tokenStored: true,
+ tokenDecryptionFailed: false,
+ storageScope: "app",
+ tokenType: "classic",
+ repo: { owner: "arul28", name: "ADE" },
+ userLogin: "arul",
+ scopes: ["repo", "workflow"],
+ checkedAt: new Date().toISOString(),
+ repoAccessOk: true,
+ repoAccessError: null,
+ connected: true,
+};
+
+const githubStatusListeners = new Set<(status: any) => void>();
+const emitGithubStatus = () => {
+ for (const listener of githubStatusListeners) listener({ ...mockGithubStatus });
+};
+
github: {
- getStatus: resolved({
- ...
- }),
- setToken: resolvedArg({
- ...
- }),
- clearToken: resolved({
- ...
- }),
+ getStatus: async () => ({ ...mockGithubStatus }),
+ setToken: async () => {
+ mockGithubStatus = { ...mockGithubStatus, tokenStored: true, connected: true, checkedAt: new Date().toISOString() };
+ emitGithubStatus();
+ return { ...mockGithubStatus };
+ },
+ clearToken: async () => {
+ mockGithubStatus = { ...mockGithubStatus, tokenStored: false, userLogin: null, scopes: [], checkedAt: null, repoAccessOk: null, repoAccessError: null, connected: false };
+ emitGithubStatus();
+ return { ...mockGithubStatus };
+ },
detectRepo: resolved({ owner: "arul28", name: "ADE" }),
listRepoLabels: resolved([]),
listRepoCollaborators: resolved([]),
- onStatusChanged: noop,
+ onStatusChanged: (listener: (status: any) => void) => {
+ githubStatusListeners.add(listener);
+ return () => githubStatusListeners.delete(listener);
+ },
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/renderer/browserMock.ts` around lines 3059 - 3101, The mock
returns static snapshots and leaves onStatusChanged as noop, so AppShell doesn't
receive immediate banner updates; update the browser mock to support subscribing
and emitting status changes: add an internal listener list and implement
onStatusChanged to register callbacks (and return an unsubscribe), then modify
setToken (resolvedArg) and clearToken (resolved) handlers to update the internal
status object (same shape as getStatus) and invoke all registered listeners with
the new status; ensure detectRepo/listRepo* remain unchanged and use the same
status shape fields (tokenStored, tokenType, userLogin, scopes, checkedAt,
repoAccessOk, connected, etc.) so AppShell's reactions work in browser-preview.
Comment on lines
+309
to
+324
| const args = (value.args && typeof value.args === "object" && !Array.isArray(value.args)) | ||
| ? (value.args as Record<string, unknown>) | ||
| : {}; | ||
|
|
||
| const setArg = (name: string, next: unknown) => { | ||
| const nextArgs = { ...args }; | ||
| if (next === undefined || next === "" || next === null) { | ||
| delete nextArgs[name]; | ||
| } else { | ||
| nextArgs[name] = next; | ||
| } | ||
| onChange({ | ||
| ...value, | ||
| args: Object.keys(nextArgs).length === 0 ? undefined : nextArgs, | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Array-typed args are silently dropped when editing a schema action.
AdeActionValue.args is typed as Record<string, unknown> | unknown[], but ActionParamsEditor coerces array args to {} and then setArg writes { [name]: value } over value.args. If the user lands on an action whose stored args is an array (legacy rules, externally authored payloads, etc.), the first parameter edit silently destroys the original array contents.
At minimum, fall back to the raw JSON editor when the args shape doesn't match the schema's keyed-form expectation, or warn the user before overwriting.
♻️ Suggested guard
- const args = (value.args && typeof value.args === "object" && !Array.isArray(value.args))
- ? (value.args as Record<string, unknown>)
- : {};
+ const argsAreArray = Array.isArray(value.args);
+ const args = (value.args && typeof value.args === "object" && !argsAreArray)
+ ? (value.args as Record<string, unknown>)
+ : {};
+ // If the persisted args are an array, force the raw JSON view so the user
+ // can see/edit them instead of silently coercing to `{}` on first keystroke.…and gate the schema form on !argsAreArray so array-shaped payloads stay in the JSON editor.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/renderer/components/automations/AdeActionEditor.tsx` around
lines 309 - 324, The component currently coerces array-shaped
AdeActionValue.args into an empty object and then setArg (in ActionParamsEditor)
overwrites the original array, silently destroying data; update the logic to
detect array-shaped args (e.g., const argsAreArray = Array.isArray(value.args))
and do NOT coerce arrays to {} — if argsAreArray is true, render or switch to
the raw JSON editor (or show a confirmation/warning) and disable the
keyed-schema form and setArg path so edits cannot overwrite the array; keep
setArg and onChange behavior only for object-shaped args (i.e., guard setArg
with !argsAreArray) so legacy/externally authored array payloads are preserved.
Comment on lines
+39
to
+61
| Button { | ||
| onSelect(nil) | ||
| } label: { | ||
| ZStack { | ||
| Circle() | ||
| .fill(Color.clear) | ||
| .frame(width: 30, height: 30) | ||
| .overlay( | ||
| Circle() | ||
| .strokeBorder(Color.white.opacity(0.35), style: StrokeStyle(lineWidth: 1, dash: [3])) | ||
| ) | ||
| Image(systemName: "xmark") | ||
| .font(.system(size: 11, weight: .semibold)) | ||
| .foregroundStyle(ADEColor.textSecondary) | ||
| if selectedHex == nil { | ||
| Circle() | ||
| .stroke(ADEColor.textPrimary, lineWidth: 2) | ||
| .frame(width: 36, height: 36) | ||
| } | ||
| } | ||
| } | ||
| .buttonStyle(.plain) | ||
| .accessibilityLabel("No color") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and view the full file
find . -name "LaneColorSwatchPicker.swift" -type fRepository: arul28/ADE
Length of output: 109
🏁 Script executed:
# View the complete file to understand the swatch implementations
cat -n apps/ios/ADE/Views/Lanes/LaneColorSwatchPicker.swiftRepository: arul28/ADE
Length of output: 2553
🏁 Script executed:
# Search for accessibility modifiers in the file
rg "accessibility|isSelected" apps/ios/ADE/Views/Lanes/LaneColorSwatchPicker.swift -A 2 -B 2Repository: arul28/ADE
Length of output: 899
🏁 Script executed:
# Check if other swatch buttons in the file use isSelected
rg "\.isSelected|\.accessibilityAddTraits|\.accessibilityRemoveTraits" apps/ios/ADE/Views/Lanes/ -A 1 -B 1Repository: arul28/ADE
Length of output: 903
Add .isSelected accessibility trait to the "No color" button.
This button visually indicates selection when selectedHex == nil (the ring overlay), but it never adds .accessibilityAddTraits(), so VoiceOver cannot tell that "No color" is the current choice. Add the trait to match the pattern used for the color swatches:
.accessibilityLabel("No color")
.accessibilityAddTraits(selectedHex == nil ? [.isSelected, .isButton] : [.isButton])
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ios/ADE/Views/Lanes/LaneColorSwatchPicker.swift` around lines 39 - 61,
The "No color" Button does not expose selection state to VoiceOver; update the
Button (the Button that calls onSelect(nil) and visually checks selectedHex ==
nil) to add accessibilityAddTraits so it includes .isSelected when selectedHex
== nil and always includes .isButton, matching the color swatch pattern (use
accessibilityAddTraits(selectedHex == nil ? [.isSelected, .isButton] :
[.isButton])).
Comment on lines
+339
to
+345
| if let hex = selectedColorHex, !hex.isEmpty { | ||
| do { | ||
| try await syncService.updateLaneAppearance(created.id, color: hex) | ||
| } catch { | ||
| // Non-fatal: lane was created, color failed (likely uniqueness collision). | ||
| } | ||
| } |
There was a problem hiding this comment.
Don’t silently swallow lane-color update failures after create.
If color assignment fails, the sheet currently succeeds with no user feedback. That can leave users thinking the chosen color was applied when it wasn’t.
💡 Suggested non-fatal feedback
if let hex = selectedColorHex, !hex.isEmpty {
do {
try await syncService.updateLaneAppearance(created.id, color: hex)
} catch {
- // Non-fatal: lane was created, color failed (likely uniqueness collision).
+ // Non-fatal: lane was created, but color update failed.
+ queuedNotice = "Lane was created, but its color could not be applied. You can update it from lane settings."
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ios/ADE/Views/Lanes/LaneCreateSheet.swift` around lines 339 - 345, The
catch block that swallows errors when calling
syncService.updateLaneAppearance(created.id, color: hex) should present
non‑fatal user feedback and log the error rather than silently ignoring it;
update the catch to (1) log the caught error (e.g., via a logger or print) and
(2) surface a UI notification by setting/using a local `@State` or viewModel
property (e.g., showColorApplyError / colorApplyErrorMessage) so the sheet can
display a non‑blocking alert/toast indicating the color couldn't be applied
while leaving the lane creation successful. Ensure this change is made inside
the existing if let selectedColorHex ... try await ... catch block so behavior
remains non‑fatal.
Comment on lines
+513
to
+522
| @MainActor | ||
| func applyLaneColor(_ hex: String?, to laneId: String) async { | ||
| do { | ||
| try await syncService.updateLaneAppearance(laneId, color: hex ?? "") | ||
| await reload(refreshRemote: false) | ||
| } catch { | ||
| ADEHaptics.error() | ||
| errorMessage = error.localizedDescription | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="apps/ios/ADE/Services/SyncService.swift"
rg -n -C4 'func (updateLaneAppearance|refreshLaneSnapshots|fetchLaneListSnapshots)' "$FILE"Repository: arul28/ADE
Length of output: 1305
🏁 Script executed:
FILE="apps/ios/ADE/Services/SyncService.swift"
# Get full updateLaneAppearance implementation
rg -n -A10 'func updateLaneAppearance\(' "$FILE" | head -20Repository: arul28/ADE
Length of output: 438
🏁 Script executed:
FILE="apps/ios/ADE/Views/Lanes/LaneListViewParts.swift"
# Find and examine the reload() function
rg -n -B2 -A10 'func reload\(' "$FILE"Repository: arul28/ADE
Length of output: 588
🏁 Script executed:
FILE="apps/ios/ADE/Views/Lanes/LaneListViewParts.swift"
# Check how archive/unarchive mutations are handled
rg -n -B2 -A8 'func (archiveLane|restoreLane)' "$FILE"Repository: arul28/ADE
Length of output: 36
🏁 Script executed:
FILE="apps/ios/ADE/Services/SyncService.swift"
rg -n -A20 'func updateLaneAppearance\(' "$FILE"Repository: arul28/ADE
Length of output: 848
🏁 Script executed:
FILE="apps/ios/ADE/Views/Lanes/LaneListViewParts.swift"
# Search more broadly for mutation functions
rg -n 'func (archive|restore|update|delete)' "$FILE" | head -20Repository: arul28/ADE
Length of output: 36
🏁 Script executed:
FILE="apps/ios/ADE/Views/Lanes/LaneListViewParts.swift"
# Look at the full applyLaneColor function context and surrounding mutations
rg -n -B15 'func applyLaneColor' "$FILE"Repository: arul28/ADE
Length of output: 600
🏁 Script executed:
FILE="apps/ios/ADE/Views/Lanes/LaneListViewParts.swift"
# Find where archiveLane/unarchiveLane are invoked
rg -n -B3 -A5 'syncService\.(archiveLane|unarchiveLane)' "$FILE"Repository: arul28/ADE
Length of output: 695
🏁 Script executed:
FILE="apps/ios/ADE/Views/Lanes/LaneListViewParts.swift"
# Get the full applyLaneColor function with more context
sed -n '513,522p' "$FILE"Repository: arul28/ADE
Length of output: 354
Change reload(refreshRemote: false) to reload(refreshRemote: true) for consistency.
The archiveLane() and unarchiveLane() calls in this file both use reload(refreshRemote: true) after their respective mutations. Since updateLaneAppearance() has the same backend pattern—it sends a command without updating the local cache—the color change needs a server refresh to appear in the UI. Using reload(refreshRemote: false) will display stale cached data until a later refresh occurs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ios/ADE/Views/Lanes/LaneListViewParts.swift` around lines 513 - 522, The
applyLaneColor(_ hex: String?, to laneId: String) async method calls
syncService.updateLaneAppearance(...) but then reloads the UI with
reload(refreshRemote: false), causing stale cached data; change the reload call
to reload(refreshRemote: true) so the UI performs a server refresh after
updateLaneAppearance and immediately reflects the color change. Ensure you only
modify the reload(...) invocation in applyLaneColor to pass true.
Comment on lines
+166
to
+175
| let laneAccent = LaneColorPalette.color(forHex: snapshot.lane.color) | ||
| VStack(alignment: .leading, spacing: 10) { | ||
| HStack(alignment: .firstTextBaseline, spacing: 8) { | ||
| LaneStatusIndicator(bucket: snapshot.runtime.bucket, size: 9) | ||
| if let laneAccent { | ||
| Circle().fill(laneAccent).frame(width: 7, height: 7) | ||
| } | ||
| Text(snapshot.lane.name) | ||
| .font(.headline) | ||
| .foregroundStyle(ADEColor.textPrimary) | ||
| .foregroundStyle(laneAccent ?? ADEColor.textPrimary) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
WCAG contrast guidance for tinting headline text with palette colors on a dark surface
💡 Result:
WCAG contrast guidance for tinting headline text with palette colors on a dark surface requires calculating the contrast ratio between the tinted text color and the dark background color using the WCAG formula, ensuring it meets the minimum thresholds based on text size and boldness. For WCAG 2.2 AA (standard compliance level): - Normal text (<18pt or <14pt bold): 4.5:1 minimum. - Large headlines (≥18pt or ≥14pt bold): 3:1 minimum. For AAA (enhanced): 7:1 normal, 4.5:1 large. Tinted palette colors (e.g., brand hues) must have sufficient luminance difference from the dark surface. Hue/saturation aids branding but does not substitute for luminance contrast; low-luminance tints on dark backgrounds often fail. Test every tinted color explicitly against the background. On dark surfaces, WCAG 2 contrast formula may overestimate readability for light text; aim higher (e.g., 7:1+ for small text) or use WCAG 3's APCA, which is stricter for light-on-dark and font-aware (higher contrast needed for thin/small fonts, lower for bold/large). Practical steps: 1. Use tools like WebAIM Contrast Checker to compute ratios. 2. Adjust tint lightness/chroma in OKLCH/HSLuv (darken for light-on-dark). 3. Create separate "on-dark" palette variants (e.g., lighter tints). 4. Verify large headlines qualify as "large text" (24px+ or 18.5px bold). 5. Account for anti-aliasing/font rendering: thin fonts need ~30% higher contrast. Examples: Pure blue tints struggle on dark (add green); aim for near-white (#E9ECF1) or high-luminance colors for body, adjusted tints for headlines. WCAG 2.2 remains the enforced standard (as of 2026-04-27); WCAG 3/APCA is emerging best practice.
Citations:
- 1: https://www.w3.org/WAI/GL/WCAG3/2021/how-tos/visual-contrast-of-text/
- 2: https://www.w3.org/WAI/WCAG21/Understanding/contrast-minimum
- 3: https://www.w3.org/WAI/WCAG22/Understanding/contrast-enhanced.html
- 4: https://webaim.org/articles/contrast/
- 5: https://www.colorpalettegenerator.ai/guide/wcag-color-contrast-guide
- 6: https://blog.greeden.me/en/2026/02/23/complete-accessibility-guide-for-dark-mode-and-high-contrast-color-design-contrast-validation-respecting-os-settings-icons-images-and-focus-visibility-wcag-2-1-aa/
- 7: https://toolsfast.app/developer/color-accessibility-guide/
🏁 Script executed:
# Check if the file exists and read the specific lines
if [ -f "apps/ios/ADE/Views/Lanes/LaneTreeView.swift" ]; then
echo "=== File exists. Reading lines 160-180 for context ==="
sed -n '160,180p' "apps/ios/ADE/Views/Lanes/LaneTreeView.swift" | cat -n
else
echo "File not found at specified path. Searching for LaneTreeView.swift..."
find . -name "LaneTreeView.swift" -type f
fiRepository: arul28/ADE
Length of output: 996
🏁 Script executed:
# Search for LaneColorPalette definition
echo "=== Searching for LaneColorPalette ==="
rg "LaneColorPalette" --type swift -A 10 -B 2Repository: arul28/ADE
Length of output: 15022
🏁 Script executed:
# Search for ADEColor.surfaceBackground definition
echo "=== Searching for surfaceBackground definition ==="
rg "surfaceBackground" --type swift -B 2 -A 2Repository: arul28/ADE
Length of output: 40713
🏁 Script executed:
# Search for any contrast helpers or accessibility utilities
echo "=== Searching for contrast-related helpers ==="
rg "contrast|accessibility|luminance" --type swift -iRepository: arul28/ADE
Length of output: 41532
Review accessibility concern: palette color contrast on lane headlines.
Once laneAccent is non-nil, both the leading Circle and headline Text use the same tint. The dot suffices to signal lane color identity; tinting the headline as well risks contrast failures against ADEColor.surfaceBackground for certain palette entries—particularly saturated yellows (amber), pinks, and pale hues on both light (#faf8f5) and dark (#16141e) surfaces. Per WCAG 2.2, large headlines require 3:1 minimum contrast (AA); many palette colors lack sufficient luminance separation.
This pattern appears in LaneComponents.swift (2 locations) and WorkRootComponents.swift as well, so a repo-wide fix is warranted. Consider either:
- Keep headline at
ADEColor.textPrimaryand let the dot carry color signaling. - Run resolved accents through a luminance-aware helper before applying to text.
♻️ Suggested adjustment (dot-only color signal)
if let laneAccent {
Circle().fill(laneAccent).frame(width: 7, height: 7)
}
Text(snapshot.lane.name)
.font(.headline)
- .foregroundStyle(laneAccent ?? ADEColor.textPrimary)
+ .foregroundStyle(ADEColor.textPrimary)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ios/ADE/Views/Lanes/LaneTreeView.swift` around lines 166 - 175, The
headline Text should not reuse the resolved laneAccent tint because some palette
colors fail WCAG contrast; update the views that currently color the headline
(the Text(snapshot.lane.name) usage in LaneTreeView / LaneComponents.swift and
the similar usage in WorkRootComponents.swift) to always use
ADEColor.textPrimary for the headline while leaving the
Circle().fill(laneAccent) (and LaneStatusIndicator) to convey the color;
alternatively, if you prefer dynamic coloring, run laneAccent through a
luminance-contrast helper (create/apply a helper named something like
accessibleAccent(for:)) and only apply the adjusted color to Text when it meets
a 3:1 contrast ratio against ADEColor.surfaceBackground, falling back to
ADEColor.textPrimary otherwise.
Comment on lines
+700
to
+711
| if let laneAccent = LaneColorPalette.color(forHex: lane?.color) { | ||
| Circle() | ||
| .fill(laneAccent) | ||
| .frame(width: 6, height: 6) | ||
| } else { | ||
| Image(systemName: "arrow.triangle.branch") | ||
| .font(.system(size: 10, weight: .semibold)) | ||
| .foregroundStyle(ADEColor.textMuted) | ||
| } | ||
| Text(session.laneName) | ||
| .font(.caption2) | ||
| .foregroundStyle(ADEColor.textMuted) | ||
| .foregroundStyle(LaneColorPalette.color(forHex: lane?.color) ?? ADEColor.textMuted) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect LaneColorPalette to confirm whether color(forHex:) is intended for text or only for accents/dots.
fd -t f 'LaneColorPalette.swift' apps/ios | xargs -I {} sh -c 'echo "===== {} ====="; cat "{}"'Repository: arul28/ADE
Length of output: 3092
Add contrast-safe text color mapping for lane names.
LaneColorPalette.color(forHex:) returns the raw user-selected hex color without luminance adjustment. Using it directly for Text(session.laneName).foregroundStyle(...) against the providerTintColor.opacity(0.10) row background creates a risk of low contrast and poor readability, especially if users pick dark or saturated colors.
Add a separate LaneColorPalette.textColor(forHex:) method that applies luminance adjustment (e.g., ensure minimum contrast ratio or lighten dark colors) and use it for the lane name text at line 711. Keep the raw color(forHex:) for the 6×6 accent dot, which is less sensitive to contrast issues.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ios/ADE/Views/Work/WorkRootComponents.swift` around lines 700 - 711, The
lane name text is using the raw LaneColorPalette.color(forHex:) which can yield
poor contrast; add a new LaneColorPalette.textColor(forHex:) that adjusts
luminance or enforces a minimum contrast (e.g., lighten/darken or pick
white/black fallback) and update the Text(session.laneName).foregroundStyle(...)
to use LaneColorPalette.textColor(forHex: lane?.color) while keeping
Circle().fill(LaneColorPalette.color(forHex: lane?.color)) for the 6×6 accent
dot so the accent remains the raw user color and the label uses the
contrast-safe color.
…tions, and iOS Includes paste-image fallback in AgentChatComposer (read clipboardData.items when files is empty so screenshot/image paste works on macOS). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
arul28
force-pushed
the
ade/fixes-to-buncha-things-17615678
branch
from
b6980f5 to
3109b61
Compare
Owner
Author
|
@copilot review but do not make fixes |
Copilot stopped work on behalf of
arul28 due to an error
April 27, 2026 04:45
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
Describe the change.
What Changed
Key files and behaviors.
Validation
How you tested.
Risks
Anything to watch.
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Greptile Summary
This PR delivers four main features: fine-grained GitHub token support with a live repo-access probe (
GET /repos/{owner}/{name}), lane color customization with a uniqueness constraint enforced in SQLite, tiling layout presets ("rows"/"columns"/"auto") for multi-session work areas, and a schema-driven automation action editor. The GitHub banner now surfaces the real failure reason rather than showing a generic "not connected" state, and a newIPC.githubStatusChangedpush event keeps all windows in sync immediately after a token change.Confidence Score: 5/5
PR is safe to merge; all findings are P2 style suggestions only.
No P0 or P1 issues found. Both flagged items are P2: a slightly misleading banner fallback message for auth-failure vs. scope-failure cases, and a case-sensitive idempotency guard in the lane color update path that could silently normalise hex casing. Well-tested with good coverage across backend, service, and UI layers.
apps/desktop/src/renderer/components/app/AppShell.tsx (banner message accuracy), apps/desktop/src/main/services/lanes/laneService.ts (color guard casing)
Important Files Changed
Sequence Diagram
sequenceDiagram participant UI as Settings / AppShell participant Preload as Preload (IPC) participant Main as Main Process participant GitHub as GitHub API UI->>Preload: getStatus({ forceRefresh: true }) Preload->>Main: ipcMain githubGetStatus Main->>GitHub: GET /user (validateToken) GitHub-->>Main: userLogin, scopes, tokenType alt fine-grained token + repo present Main->>GitHub: GET /repos/{owner}/{name} (probeRepoAccess) GitHub-->>Main: 200 OK or 403/404 end Main->>Main: computeConnected(tokenType, scopes, repoAccessOk) Main-->>Preload: GitHubStatus { connected, repoAccessOk, ... } Preload-->>UI: status Note over UI,Main: On setToken / clearToken Main->>Main: broadcastGithubStatus(status) Main-->>Preload: IPC.githubStatusChanged event Preload-->>UI: onStatusChanged callback → setGithubStatusPrompt To Fix All With AI
Reviews (2): Last reviewed commit: "fixes to a bunch of things across lanes,..." | Re-trigger Greptile