t3code-inspired features: model slug, PR ahead hints, chat UX, settings#163
Conversation
Inspired by upstream t3code PRs: - pingdotgg/t3code#1 (resolveModelSlug-style normalization) - pingdotgg/t3code#2092 (code block copy for touch / position / clipboard fallback) - pingdotgg/t3code#2057 (completion chime when agent turn settles) - pingdotgg/t3code#2081 (surface ahead-of-base for clean pushed branches on mobile create PR) Adds resolveModelSlug with optional provider hint, chat code copy controls and non-secure clipboard fallback, optional Web Audio completion sound with settings, and commitsAheadOfBase on mobile PR create eligibility with iOS subtitle. Co-authored-by: Arul Sharma <arul28@users.noreply.github.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (26)
Disabled knowledge base sources:
📝 WalkthroughWalkthroughAdds persisted appearance and chat preferences, chat UI scaling, configurable code-copy placement and clipboard fallback, a Web Audio agent-turn completion sound, a new numeric Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/ios/ADE/Models/RemoteModels.swift (1)
2098-2113:⚠️ Potential issue | 🟠 MajorImplement backward-compatible decoding for the new
commitsAheadOfBasefield.
PrCreateLaneEligibilityuses synthesizedCodablewithout custom decoding. The newly added non-optionalcommitsAheadOfBase: Intfield will cause the entirePrMobileSnapshotto fail decoding if an older desktop host omits this key. Implement a custominit(from:)usingdecodeIfPresentwith a default of0to maintain compatibility with payloads from hosts that predate this field.🛡️ Proposed backward-compatible decoding
struct PrCreateLaneEligibility: Codable, Identifiable, Equatable { var id: String { laneId } var laneId: String var laneName: String var parentLaneId: String? var repoOwner: String? var repoName: String? var defaultBaseBranch: String var defaultTitle: String var dirty: Bool /// Commits on the lane branch not on `defaultBaseBranch` (same signal as desktop lane status `ahead`). var commitsAheadOfBase: Int var hasExistingPr: Bool var canCreate: Bool var blockedReason: String? } + +extension PrCreateLaneEligibility { + private enum CodingKeys: String, CodingKey { + case laneId + case laneName + case parentLaneId + case repoOwner + case repoName + case defaultBaseBranch + case defaultTitle + case dirty + case commitsAheadOfBase + case hasExistingPr + case canCreate + case blockedReason + } + + init(from decoder: Decoder) throws { + let container = try decoder.container(keyedBy: CodingKeys.self) + laneId = try container.decode(String.self, forKey: .laneId) + laneName = try container.decode(String.self, forKey: .laneName) + parentLaneId = try container.decodeIfPresent(String.self, forKey: .parentLaneId) + repoOwner = try container.decodeIfPresent(String.self, forKey: .repoOwner) + repoName = try container.decodeIfPresent(String.self, forKey: .repoName) + defaultBaseBranch = try container.decode(String.self, forKey: .defaultBaseBranch) + defaultTitle = try container.decode(String.self, forKey: .defaultTitle) + dirty = try container.decode(Bool.self, forKey: .dirty) + commitsAheadOfBase = try container.decodeIfPresent(Int.self, forKey: .commitsAheadOfBase) ?? 0 + hasExistingPr = try container.decode(Bool.self, forKey: .hasExistingPr) + canCreate = try container.decode(Bool.self, forKey: .canCreate) + blockedReason = try container.decodeIfPresent(String.self, forKey: .blockedReason) + } +}Add a regression test where a
PrCreateLaneEligibilitypayload omitscommitsAheadOfBaseand decodes successfully with the default value of0.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ios/ADE/Models/RemoteModels.swift` around lines 2098 - 2113, PrCreateLaneEligibility is using synthesized Codable but the new non-optional commitsAheadOfBase will break decoding from older payloads; implement a custom init(from decoder: Decoder) in PrCreateLaneEligibility that decodes all properties normally but uses container.decodeIfPresent(Int.self, forKey: .commitsAheadOfBase) ?? 0 to default missing commitsAheadOfBase to 0, and keep the synthesized encode(by) behavior (or implement encode(to:) mirroring fields). Update or add a unit test that decodes a JSON/PrMobileSnapshot payload missing the commitsAheadOfBase key and asserts the decoded PrCreateLaneEligibility.commitsAheadOfBase == 0 to prevent regressions.
🧹 Nitpick comments (2)
apps/desktop/src/main/services/prs/prService.mobileSnapshot.test.ts (1)
285-309: Exercise a non-zero ahead count in this contract test.Line 309 only verifies the default from
makeLane, so a hardcoded0implementation would still pass. Set the eligible lane’sstatus.aheadto a positive value and assert that value is propagated.🧪 Proposed test tightening
- const eligible = makeLane({ id: "lane-feat", name: "feat", parentLaneId: null }); + const eligible = makeLane({ + id: "lane-feat", + name: "feat", + parentLaneId: null, + status: { dirty: false, ahead: 2, behind: 0, remoteBehind: 0, rebaseInProgress: false }, + }); ... - expect(eligibleEntry.commitsAheadOfBase).toBe(0); + expect(eligibleEntry.commitsAheadOfBase).toBe(2);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/prs/prService.mobileSnapshot.test.ts` around lines 285 - 309, The test currently relies on the default 0 ahead count; update the eligible lane created with makeLane to include a non-zero status.ahead (e.g., status: { ahead: 3 }) so the snapshot must carry that value, then change the assertion on eligibleEntry.commitsAheadOfBase to expect that positive number instead of 0; adjust references in the test around makeLane and the createCapabilities / getMobileSnapshot assertions accordingly.apps/ios/ADETests/ADETests.swift (1)
3604-3618: Assert the new field so the fixture additions are meaningful.The tests add
commitsAheadOfBasevalues, but they do not verify those values after decoding/filtering. A couple of direct assertions would catch mapping regressions.🧪 Proposed test assertions
let blocked = snapshot.createCapabilities.lanes.first(where: { $0.laneId == "lane-blocked" }) XCTAssertNotNil(blocked) XCTAssertFalse(blocked?.canCreate ?? true) XCTAssertTrue(blocked?.hasExistingPr ?? false) + XCTAssertEqual(blocked?.commitsAheadOfBase, 2) XCTAssertTrue((blocked?.blockedReason ?? "").contains("#7"))let eligibleOnly = capabilities.lanes.filter { $0.canCreate } XCTAssertEqual(eligibleOnly.map(\.laneId), ["lane-new"]) + XCTAssertEqual(eligibleOnly.first?.commitsAheadOfBase, 1) let blockedOnly = capabilities.lanes.filter { !$0.canCreate } + XCTAssertEqual(blockedOnly.first?.commitsAheadOfBase, 0) XCTAssertEqual(blockedOnly.first?.blockedReason, "Lane already has an open PR (`#12`).")Also applies to: 3820-3834
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ios/ADETests/ADETests.swift` around lines 3604 - 3618, Add assertions in ADETests.swift to verify the newly added commitsAheadOfBase field is decoded and filtered correctly: after decoding the fixture into your lanes collection (e.g., the variable that holds the decoded/filtered lanes such as "lanes" or "decodedLanes" in the test that uses the shown JSON fixture), add direct XCTAssertEqual checks for commitsAheadOfBase for the relevant lane entries (e.g., assert 0 for the first lane and 2 for the "lane-blocked" entry) so the test fails if mapping of commitsAheadOfBase regresses; mirror this change in the other test that covers the fixture at the 3820-3834 region.
🤖 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/renderer/lib/agentTurnCompletionSound.ts`:
- Around line 20-50: playAgentTurnCompletionSound currently closes the
AudioContext in the finally block which stops scheduled oscillators before they
can play; modify playAgentTurnCompletionSound so it does not call ctx.close()
immediately but instead defers closing until after the longest scheduled sound
completes (calculate the max scheduled duration used by playChime calls and the
direct oscillator branch, or attach "ended" handlers to created
OscillatorNodes), then call ctx.close() (or ctx.suspend()) in that delayed
callback and still catch/reject errors as done now; reference the
playAgentTurnCompletionSound function and the playChime/osc variables to locate
where to compute or observe the end time and schedule the deferred close.
In `@apps/desktop/src/shared/modelRegistry.ts`:
- Around line 884-889: The hinted path in resolveModelSlug currently lowercases
the entire model ref via resolveModelDescriptorForProvider which corrupts
case-sensitive IDs (e.g., lmstudio/Qwen/Qwen2.5-Coder); update the code so that
when providerHint is supplied resolveModelDescriptorForProvider is called in a
mode that preserves the original case (either by adding a preserveCase flag to
resolveModelDescriptorForProvider or by adding a new helper that skips the
toLowerCase step), and ensure resolveModelSlug passes the original trimmed
string (normalized) unchanged into that case-preserving call so the returned
descriptor.id retains its original casing.
In `@apps/ios/ADE/Views/PRs/CreatePrWizardView.swift`:
- Around line 44-55: The computed aheadNote is assigned to
CreatePrLaneOption.subtitle but never used when rendering the UI in
createStepOne, so the "N commits ahead…" hint doesn't appear; update
createStepOne to read selectedOption.subtitle (from the selected
CreatePrLaneOption) and render it in the step one UI (e.g., display it in the
subtitle label or secondary Text view alongside branch/title) so the subtitle is
visible when present.
---
Outside diff comments:
In `@apps/ios/ADE/Models/RemoteModels.swift`:
- Around line 2098-2113: PrCreateLaneEligibility is using synthesized Codable
but the new non-optional commitsAheadOfBase will break decoding from older
payloads; implement a custom init(from decoder: Decoder) in
PrCreateLaneEligibility that decodes all properties normally but uses
container.decodeIfPresent(Int.self, forKey: .commitsAheadOfBase) ?? 0 to default
missing commitsAheadOfBase to 0, and keep the synthesized encode(by) behavior
(or implement encode(to:) mirroring fields). Update or add a unit test that
decodes a JSON/PrMobileSnapshot payload missing the commitsAheadOfBase key and
asserts the decoded PrCreateLaneEligibility.commitsAheadOfBase == 0 to prevent
regressions.
---
Nitpick comments:
In `@apps/desktop/src/main/services/prs/prService.mobileSnapshot.test.ts`:
- Around line 285-309: The test currently relies on the default 0 ahead count;
update the eligible lane created with makeLane to include a non-zero
status.ahead (e.g., status: { ahead: 3 }) so the snapshot must carry that value,
then change the assertion on eligibleEntry.commitsAheadOfBase to expect that
positive number instead of 0; adjust references in the test around makeLane and
the createCapabilities / getMobileSnapshot assertions accordingly.
In `@apps/ios/ADETests/ADETests.swift`:
- Around line 3604-3618: Add assertions in ADETests.swift to verify the newly
added commitsAheadOfBase field is decoded and filtered correctly: after decoding
the fixture into your lanes collection (e.g., the variable that holds the
decoded/filtered lanes such as "lanes" or "decodedLanes" in the test that uses
the shown JSON fixture), add direct XCTAssertEqual checks for commitsAheadOfBase
for the relevant lane entries (e.g., assert 0 for the first lane and 2 for the
"lane-blocked" entry) so the test fails if mapping of commitsAheadOfBase
regresses; mirror this change in the other test that covers the fixture at the
3820-3834 region.
🪄 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: f5a64a96-5acb-4a99-8d3f-816492073709
📒 Files selected for processing (13)
apps/desktop/src/main/services/prs/prService.mobileSnapshot.test.tsapps/desktop/src/main/services/prs/prService.tsapps/desktop/src/renderer/components/chat/AgentChatPane.tsxapps/desktop/src/renderer/components/chat/CodeHighlighter.tsxapps/desktop/src/renderer/components/settings/GeneralSection.tsxapps/desktop/src/renderer/lib/agentTurnCompletionSound.tsapps/desktop/src/renderer/state/appStore.tsapps/desktop/src/shared/modelRegistry.test.tsapps/desktop/src/shared/modelRegistry.tsapps/desktop/src/shared/types/prs.tsapps/ios/ADE/Models/RemoteModels.swiftapps/ios/ADE/Views/PRs/CreatePrWizardView.swiftapps/ios/ADETests/ADETests.swift
- Dismiss missing-AI and GitHub setup banners per session (inspired by pingdotgg/t3code#773) - Resume suspended AudioContext, defer AudioContext.close, use global setTimeout - Add vitest coverage for sound helper and banner dismiss; extend appStore prefs test Co-authored-by: Arul Sharma <arul28@users.noreply.github.com>
![capy-ai[bot]](https://avatars.githubusercontent.com/in/1915919?s=60&v=4){kind=small}
| const prevTurn = completionSoundPrevTurnActiveRef.current; | ||
| const becameIdle = settled && prevTurn && !turnActive; | ||
| completionSoundPrevTurnActiveRef.current = turnActive; | ||
| if (becameIdle && completionSoundArmedRef.current) { |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
The new idle-transition effect treats any turnActive → false transition as a successful completion and plays the notification immediately:
// apps/desktop/src/renderer/components/chat/AgentChatPane.tsx
const prevTurn = completionSoundPrevTurnActiveRef.current;
const becameIdle = settled && prevTurn && !turnActive;
completionSoundPrevTurnActiveRef.current = turnActive;
if (becameIdle && completionSoundArmedRef.current) {
completionSoundArmedRef.current = false;
playAgentTurnCompletionSound(agentTurnCompletionSound);
}That also matches the existing interrupt() path in this same component, which optimistically does setTurnActiveBySession((prev) => ({ ...prev, [selectedSessionId]: false })) before awaiting the backend interrupt. In practice, clicking Stop will satisfy prevTurn && !turnActive and emit the new "completion" sound for an aborted turn. Suppress the next notification when the stop path initiated the transition, or gate playback on a confirmed non-interrupted terminal event instead of any idle transition.
| setProjectMissing(false); | ||
| }, [project?.rootPath]); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
// apps/desktop/src/renderer/components/app/AppShell.tsx
useEffect(() => {
setDismissedMissingAiBannerRoots({});
setDismissedGithubBannerRoots({});
}, [project?.rootPath]);This effect clears all dismissed-banner entries every time the active project.rootPath changes, so dismissing either banner in project A is forgotten as soon as the user opens project B. Because AppShell stays mounted across project changes and the state is already keyed by rootPath, switching back to project A makes the banner reappear even though the UI promises “Dismiss for this session.” Remove this reset effect (matching the existing dismissedContextBannerRoots behavior) or otherwise preserve per-root entries until the app session actually ends.
| defaultBaseBranch: eligibility.defaultBaseBranch, | ||
| defaultTitle: eligibility.defaultTitle, | ||
| subtitle: eligibility.blockedReason | ||
| subtitle: aheadNote |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
The new mobile hint is assigned into CreatePrLaneOption.subtitle, but CreatePrWizardView never reads that property when rendering Step 1, so the feature does not become visible in the UI. The changed code only stores the string:
// apps/ios/ADE/Views/PRs/CreatePrWizardView.swift
let aheadNote: String? =
eligibility.commitsAheadOfBase > 0
? "\(eligibility.commitsAheadOfBase) commit\(eligibility.commitsAheadOfBase == 1 ? "" : "s") ahead of \(eligibility.defaultBaseBranch)"
: nil
...
subtitle: aheadNoteI verified the rest of the view only renders Text("\(option.title) · \(option.branchRef)") in the Picker and Text("Source branch: ...") for the selected lane, with no selectedOption.subtitle/option.subtitle usage anywhere in the file. As shipped, eligible lanes with commitsAheadOfBase > 0 still show no ahead context. Render the subtitle in Step 1 (or in the picker row summary) and add a UI test that asserts the hint text appears.
| primaryBranchRef: primaryLane?.branchRef ?? null, | ||
| }); | ||
| const dirty = lane.status?.dirty === true; | ||
| const commitsAheadOfBase = Math.max(0, Number(lane.status?.ahead ?? 0) || 0); |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
This field is documented and displayed as "ahead of defaultBaseBranch", but the new value is taken from lane.status.ahead, which is not guaranteed to be measured against that branch. The changed code does:
// apps/desktop/src/main/services/prs/prService.ts
const defaultBaseBranch = resolveStableLaneBaseBranch({
lane,
parent,
primaryBranchRef: primaryLane?.branchRef ?? null,
});
const dirty = lane.status?.dirty === true;
const commitsAheadOfBase = Math.max(0, Number(lane.status?.ahead ?? 0) || 0);I verified computeLaneStatus() in @apps/desktop/src/main/services/lanes/laneService.ts computes ahead from git rev-list ${baseRef}...${branchRef}, while resolveStableLaneBaseBranch() in @apps/desktop/src/shared/laneBaseResolution.ts intentionally switches child lanes with non-primary parents to the parent branch. For stacked lanes where lane.baseRef is still the stored/base branch, mobile will report commits ahead of the parent while actually counting commits ahead of the older baseRef, overstating the hint by including parent commits. Compute this count against the same branch returned by resolveStableLaneBaseBranch, or rename the field/message so it matches the underlying metric.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/renderer/components/app/AppShell.tsx`:
- Around line 820-834: The dismiss buttons use only the visible "×" which is not
accessible; update the button elements (the one using currentProjectRoot and
setDismissedMissingAiBannerRoots, and the other at lines ~852-866) to include a
descriptive aria-label (for example aria-label="Dismiss missing AI banner for
this project") while preserving the existing title and data-testid attributes;
ensure the same pattern is applied to both dismiss controls so assistive tech
announces a meaningful name.
- Around line 489-492: The effect currently clears all dismissal maps on any
project?.rootPath change causing previously dismissed entries to be lost;
instead, update the effect so it preserves existing map entries and only ensure
there are entries for the new root (e.g., if project?.rootPath is defined and
not present in dismissedMissingAiBannerRoots or dismissedGithubBannerRoots add a
default entry for that root), by using the setters
setDismissedMissingAiBannerRoots and setDismissedGithubBannerRoots to merge the
existing maps with a new key for project?.rootPath (do not overwrite the whole
map). Locate the useEffect that references project?.rootPath and the state
setters setDismissedMissingAiBannerRoots / setDismissedGithubBannerRoots and
change the reset behavior to a merge/ensure-key approach so dismissals persist
across root switches.
In `@apps/desktop/src/renderer/lib/agentTurnCompletionSound.test.ts`:
- Around line 45-54: The test currently fires the deferred close timer
immediately; change it to capture the scheduled callback and delay instead: in
the test spy on globalThis.setTimeout (or vi.spyOn) to save the TimerHandler and
the delay argument when invoked by playAgentTurnCompletionSound, assert the
captured delay equals 450, assert ctx.close (the mocked close) has NOT been
called before invoking the captured callback, then call the captured callback
and assert close was called and resume was called; finally restore the mocked
setTimeout. Reference playAgentTurnCompletionSound and the mocked close/resume
functions when locating where to capture and assert the timer behavior.
In `@apps/desktop/src/renderer/state/appStore.test.ts`:
- Around line 158-171: Split the combined persistence check into two independent
tests: one that calls
useAppStore.getState().setCodeBlockCopyButtonPosition("bottom") and asserts both
the in-memory state (useAppStore.getState().codeBlockCopyButtonPosition) and
that mockLocalStorage.setItem was called with key "ade.userPreferences.v1" whose
payload includes codeBlockCopyButtonPosition:"bottom"; and a separate test that
calls useAppStore.getState().setAgentTurnCompletionSound("chime") and asserts
useAppStore.getState().agentTurnCompletionSound and that
mockLocalStorage.setItem was called with the same key and payload including
agentTurnCompletionSound:"chime". Ensure each test isolates persistence
verification by clearing or resetting mockLocalStorage.setItem.mock.calls (or
using beforeEach to reset mocks) before invoking the setter so you assert the
specific setter’s own persistence call.
🪄 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: a400ce15-6247-46c5-a240-ea31e1d63d85
📒 Files selected for processing (5)
apps/desktop/src/renderer/components/app/AppShell.test.tsxapps/desktop/src/renderer/components/app/AppShell.tsxapps/desktop/src/renderer/lib/agentTurnCompletionSound.test.tsapps/desktop/src/renderer/lib/agentTurnCompletionSound.tsapps/desktop/src/renderer/state/appStore.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/renderer/lib/agentTurnCompletionSound.ts
| it("persists code block copy position and agent completion sound", () => { | ||
| useAppStore.getState().setCodeBlockCopyButtonPosition("bottom"); | ||
| useAppStore.getState().setAgentTurnCompletionSound("chime"); | ||
| expect(useAppStore.getState().codeBlockCopyButtonPosition).toBe("bottom"); | ||
| expect(useAppStore.getState().agentTurnCompletionSound).toBe("chime"); | ||
| const calls = mockLocalStorage.setItem.mock.calls.filter( | ||
| ([key]) => key === "ade.userPreferences.v1", | ||
| ); | ||
| const latest = calls[calls.length - 1]; | ||
| expect(latest).toBeTruthy(); | ||
| expect(JSON.parse(latest![1])).toMatchObject({ | ||
| codeBlockCopyButtonPosition: "bottom", | ||
| agentTurnCompletionSound: "chime", | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Validate the appStore preference persistence tests.
npm --prefix apps/desktop run test:unit -- apps/desktop/src/renderer/state/appStore.test.tsRepository: arul28/ADE
Length of output: 213
🏁 Script executed:
# Check the actual test file content around lines 158-171
cat -n apps/desktop/src/renderer/state/appStore.test.ts | sed -n '155,175p'Repository: arul28/ADE
Length of output: 1014
🏁 Script executed:
# Check the implementation of the setters in appStore.ts
cat -n apps/desktop/src/renderer/state/appStore.ts | sed -n '555,585p'Repository: arul28/ADE
Length of output: 1409
🏁 Script executed:
# Search for both setter implementations to confirm they each call persistUserPreferences
rg "setCodeBlockCopyButtonPosition|setAgentTurnCompletionSound" apps/desktop/src/renderer/state/appStore.ts -A 10 -B 2Repository: arul28/ADE
Length of output: 1708
Isolate each preference setter's persistence check into a separate test.
The test calls both setCodeBlockCopyButtonPosition and setAgentTurnCompletionSound back-to-back, then checks only the final localStorage call. If setCodeBlockCopyButtonPosition stops persisting, the test would still pass—setAgentTurnCompletionSound persists the full preferences object including the already-updated codeBlockCopyButtonPosition from Zustand state. Split into two independent tests to catch regressions in either setter's persistence path.
🧪 Proposed test split
- it("persists code block copy position and agent completion sound", () => {
+ it("persists code block copy position", () => {
useAppStore.getState().setCodeBlockCopyButtonPosition("bottom");
- useAppStore.getState().setAgentTurnCompletionSound("chime");
expect(useAppStore.getState().codeBlockCopyButtonPosition).toBe("bottom");
- expect(useAppStore.getState().agentTurnCompletionSound).toBe("chime");
const calls = mockLocalStorage.setItem.mock.calls.filter(
([key]) => key === "ade.userPreferences.v1",
);
const latest = calls[calls.length - 1];
expect(latest).toBeTruthy();
expect(JSON.parse(latest![1])).toMatchObject({
codeBlockCopyButtonPosition: "bottom",
+ });
+ });
+
+ it("persists agent completion sound", () => {
+ useAppStore.getState().setAgentTurnCompletionSound("chime");
+ expect(useAppStore.getState().agentTurnCompletionSound).toBe("chime");
+ const calls = mockLocalStorage.setItem.mock.calls.filter(
+ ([key]) => key === "ade.userPreferences.v1",
+ );
+ const latest = calls[calls.length - 1];
+ expect(latest).toBeTruthy();
+ expect(JSON.parse(latest![1])).toMatchObject({
agentTurnCompletionSound: "chime",
});
});📝 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.
| it("persists code block copy position and agent completion sound", () => { | |
| useAppStore.getState().setCodeBlockCopyButtonPosition("bottom"); | |
| useAppStore.getState().setAgentTurnCompletionSound("chime"); | |
| expect(useAppStore.getState().codeBlockCopyButtonPosition).toBe("bottom"); | |
| expect(useAppStore.getState().agentTurnCompletionSound).toBe("chime"); | |
| const calls = mockLocalStorage.setItem.mock.calls.filter( | |
| ([key]) => key === "ade.userPreferences.v1", | |
| ); | |
| const latest = calls[calls.length - 1]; | |
| expect(latest).toBeTruthy(); | |
| expect(JSON.parse(latest![1])).toMatchObject({ | |
| codeBlockCopyButtonPosition: "bottom", | |
| agentTurnCompletionSound: "chime", | |
| }); | |
| it("persists code block copy position", () => { | |
| useAppStore.getState().setCodeBlockCopyButtonPosition("bottom"); | |
| expect(useAppStore.getState().codeBlockCopyButtonPosition).toBe("bottom"); | |
| const calls = mockLocalStorage.setItem.mock.calls.filter( | |
| ([key]) => key === "ade.userPreferences.v1", | |
| ); | |
| const latest = calls[calls.length - 1]; | |
| expect(latest).toBeTruthy(); | |
| expect(JSON.parse(latest![1])).toMatchObject({ | |
| codeBlockCopyButtonPosition: "bottom", | |
| }); | |
| }); | |
| it("persists agent completion sound", () => { | |
| useAppStore.getState().setAgentTurnCompletionSound("chime"); | |
| expect(useAppStore.getState().agentTurnCompletionSound).toBe("chime"); | |
| const calls = mockLocalStorage.setItem.mock.calls.filter( | |
| ([key]) => key === "ade.userPreferences.v1", | |
| ); | |
| const latest = calls[calls.length - 1]; | |
| expect(latest).toBeTruthy(); | |
| expect(JSON.parse(latest![1])).toMatchObject({ | |
| agentTurnCompletionSound: "chime", | |
| }); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/renderer/state/appStore.test.ts` around lines 158 - 171,
Split the combined persistence check into two independent tests: one that calls
useAppStore.getState().setCodeBlockCopyButtonPosition("bottom") and asserts both
the in-memory state (useAppStore.getState().codeBlockCopyButtonPosition) and
that mockLocalStorage.setItem was called with key "ade.userPreferences.v1" whose
payload includes codeBlockCopyButtonPosition:"bottom"; and a separate test that
calls useAppStore.getState().setAgentTurnCompletionSound("chime") and asserts
useAppStore.getState().agentTurnCompletionSound and that
mockLocalStorage.setItem was called with the same key and payload including
agentTurnCompletionSound:"chime". Ensure each test isolates persistence
verification by clearing or resetting mockLocalStorage.setItem.mock.calls (or
using beforeEach to reset mocks) before invoking the setter so you assert the
specific setter’s own persistence call.
- New Appearance settings: theme swatches, chat font size 12–24px with live ChatMarkdown preview, copy-button position + completion sound (moved from General) - chatFontSizePx in appStore; work chat scales via ChatSurfaceShell zoom - resolveModelSlug: exact getModelById before provider-hint lowercasing (review) - iOS Create PR lane subtitle when commitsAheadOfBase is 0 - A11y: label range input; fix preview sample template literal Co-authored-by: Arul Sharma <arul28@users.noreply.github.com>
| completionSoundArmedRef.current = true; | ||
| } | ||
| const sessionEnded = selectedSession?.status === "ended"; | ||
| const settled = |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
The new completion check treats any transition from turnActive=true to false as a successful finish as long as the session is not ended and not awaiting input:
// apps/desktop/src/renderer/components/chat/AgentChatPane.tsx
const settled =
Boolean(selectedSessionId)
&& !selectedSessionAwaitingInput
&& !sessionEnded;
const becameIdle = settled && prevTurn && !turnActive;This is too broad. I verified in @apps/desktop/src/renderer/components/chat/AgentChatPane.tsx:216-250 that deriveRuntimeState() clears turnActive for every done event, and in @apps/desktop/src/main/services/chat/agentChatService.ts:5178-5199 plus the interrupt/failure paths around 7913-7919, 7966-8003, and 12493-12498 that interrupted/failed turns are left in status = "idle", not ended. That means cancelling a run or hitting a provider error satisfies settled and still calls playAgentTurnCompletionSound(...), which contradicts the setting copy in @apps/desktop/src/renderer/components/settings/AppearanceSection.tsx that says the sound plays when the assistant "finishes a turn". Gate the sound on a verified successful turn outcome (for example the latest done.status === "completed" / status.turnStatus === "completed") instead of any idle transition.
| var defaultTitle: String | ||
| var dirty: Bool | ||
| /// Commits on the lane branch not on `defaultBaseBranch` (same signal as desktop lane status `ahead`). | ||
| var commitsAheadOfBase: Int |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
PrCreateLaneEligibility is still using synthesized Codable, and this diff adds commitsAheadOfBase as a required non-optional property. SyncService.fetchPrMobileSnapshot() decodes the host response directly into PrMobileSnapshot, so an older paired desktop that does not send this new key will throw keyNotFound and drop the richer mobile PR snapshot instead of degrading cleanly. Make this field decode with a default of 0 so mixed-version desktop/iOS pairs remain backward-compatible.
// apps/ios/ADE/Models/RemoteModels.swift
var defaultBaseBranch: String
var defaultTitle: String
var dirty: Bool
/// Commits on the lane branch not on `defaultBaseBranch` (same signal as desktop lane status `ahead`).
var commitsAheadOfBase: Int
var hasExistingPr: Bool| const normalized = modelRef.trim(); | ||
| if (!normalized.length) return undefined; | ||
| if (providerHint) { | ||
| const direct = getModelById(normalized); |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
resolveModelSlug() short-circuits on getModelById(normalized) whenever a provider hint is present:
// apps/desktop/src/shared/modelRegistry.ts
if (providerHint) {
const direct = getModelById(normalized);
if (direct && !direct.deprecated && matchesProviderGroup(direct, providerHint)) {
return direct.id;
}For providerHint === "opencode", a free-form ref like lmstudio/google/gemma-4-26b-a4b is parsed by getModelById() as a dynamic local model and returns lmstudio/google/gemma-4-26b-a4b, but inventory-backed OpenCode descriptors are canonicalized as encoded opencode/<provider>/<model> IDs via createDynamicOpenCodeModelDescriptor(..., { openCodeProviderId, openCodeModelId }). The new helper therefore produces the wrong canonical slug for OpenCode-discovered local models and never reaches the provider-specific resolver that could return the paired OpenCode ID. Resolve the provider-specific descriptor first, or special-case opencode so local refs prefer matching dynamic OpenCode inventory descriptors, and add a test for an inventory-discovered model ref.
| }); | ||
|
|
||
| describe("chat and notification preferences", () => { | ||
| it("persists code block copy position and agent completion sound", () => { |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
These new tests only mutate the already-imported singleton store and inspect setItem calls, so they never verify the path that actually matters for persistence: readInitialUserPreferences() reading ade.userPreferences.v1 at module import and seeding codeBlockCopyButtonPosition, agentTurnCompletionSound, and chatFontSizePx. A regression in readUnifiedUserPreferences() / normalization would still pass here because useAppStore is imported once at file scope and resetStore() overwrites state directly between tests. ts // apps/desktop/src/renderer/state/appStore.test.ts useAppStore.getState().setCodeBlockCopyButtonPosition("bottom"); useAppStore.getState().setAgentTurnCompletionSound("chime"); const latest = calls[calls.length - 1]; expect(JSON.parse(latest![1])).toMatchObject({ codeBlockCopyButtonPosition: "bottom", Replace at least one of these cases with a true round-trip test: seed mockStorage, call vi.resetModules(), dynamically re-import ./appStore, and assert the fresh store instance rehydrates the saved values (including clamped/defaulted cases for invalid payloads).
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
apps/desktop/src/renderer/components/chat/AgentChatPane.tsx (1)
840-859:⚠️ Potential issue | 🟠 MajorSuppress completion audio for interrupted turns.
This still treats the Stop path as a normal idle transition:
interrupt()setsturnActivetofalse, so the effect plays the completion sound even though the turn was aborted.🔇 Proposed guard for the interrupt path
const completionSoundPrevTurnActiveRef = useRef(false); const completionSoundArmedRef = useRef(true); + const completionSoundSuppressNextIdleRef = useRef(false); @@ useEffect(() => { completionSoundPrevTurnActiveRef.current = false; completionSoundArmedRef.current = true; + completionSoundSuppressNextIdleRef.current = false; }, [selectedSessionId]); @@ const becameIdle = settled && prevTurn && !turnActive; completionSoundPrevTurnActiveRef.current = turnActive; + if (becameIdle && completionSoundSuppressNextIdleRef.current) { + completionSoundSuppressNextIdleRef.current = false; + completionSoundArmedRef.current = false; + return; + } if (becameIdle && completionSoundArmedRef.current) { completionSoundArmedRef.current = false; playAgentTurnCompletionSound(agentTurnCompletionSound); } @@ const interrupt = useCallback(async () => { if (!selectedSessionId) return; // Let the stop button disappear immediately while the main-process interrupt finishes. + completionSoundSuppressNextIdleRef.current = true; setTurnActiveBySession((prev) => ({ ...prev, [selectedSessionId]: false }));Also applies to: 2403-2413
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/chat/AgentChatPane.tsx` around lines 840 - 859, The effect currently triggers playAgentTurnCompletionSound when a turn becomes idle based solely on turnActive and armed refs, but interrupt() sets turnActive=false and therefore causes aborted turns to play the completion sound; fix by introducing and using an "interrupt" guard: add a ref e.g. completionSoundInterruptedRef that interrupt() sets to true (and any new-turn path in the component or where turnActive becomes true clears it), then update the useEffect (the block using agentTurnCompletionSound, completionSoundPrevTurnActiveRef, completionSoundArmedRef, turnActive, selectedSessionAwaitingInput) to require completionSoundInterruptedRef.current === false (or clear it) before calling playAgentTurnCompletionSound so aborted/interrupt paths do not play the sound.
🤖 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/renderer/components/settings/AppearanceSection.tsx`:
- Around line 236-263: The copy-position toggle buttons rendered from
CODE_BLOCK_COPY_POSITION_IDS should expose their pressed state to assistive tech
by adding aria-pressed={codeBlockCopyButtonPosition === id} (and keeping the
same onClick that calls setCodeBlockCopyButtonPosition) and include an
accessible name (e.g., aria-label={`Code block copy position ${id}`} or use an
explicit visually-hidden label); likewise the agent turn completion sound select
should be associated with a real label and id (not just a visual <div>) — add an
id on the <select> and replace the visual label div with a <label htmlFor="...">
or add aria-labelledby/aria-label, and continue to use
setAgentTurnCompletionSound(agentTurnCompletionSound) on change (casting to
AgentTurnCompletionSound) so agentTurnCompletionSound remains the controlled
value.
- Around line 181-186: The anchor tag in AppearanceSection.tsx currently uses
target="_blank" to open an external PR link directly from the renderer, which is
insecure; either remove the external link text entirely or change it to call the
secure preload IPC (or a safe openExternal handler) instead of direct
navigation. Locate the JSX anchor within the div (the element referencing
"t3code `#2174`") inside the AppearanceSection component and replace the direct
<a> navigation with a click handler that calls the preload bridge method (or
sends an IPC message to the main process) to open the external URL via
electron.shell.openExternal, or simply drop the link text if you prefer no
external navigation. Ensure the new handler uses the existing preload API name
used across the app (or add one in the preload/main if missing) and keep the
visual styling (COLORS.accent) and text intact.
- Around line 206-217: The preview container in AppearanceSection.tsx is using
transform: `scale(${previewScale})` with width compensation which doesn't
reserve layout height; change the styling on the preview div to use the CSS
`zoom` property set to `previewScale` (matching the chat shell's approach used
in AgentChatPane) and remove the transform, transformOrigin, and manual
width/maxWidth adjustments so the card's layout height is preserved; keep other
styles like border, background, padding, maxHeight, and overflow unchanged and
verify the preview no longer overflows at large font sizes (e.g., 24px).
---
Duplicate comments:
In `@apps/desktop/src/renderer/components/chat/AgentChatPane.tsx`:
- Around line 840-859: The effect currently triggers
playAgentTurnCompletionSound when a turn becomes idle based solely on turnActive
and armed refs, but interrupt() sets turnActive=false and therefore causes
aborted turns to play the completion sound; fix by introducing and using an
"interrupt" guard: add a ref e.g. completionSoundInterruptedRef that interrupt()
sets to true (and any new-turn path in the component or where turnActive becomes
true clears it), then update the useEffect (the block using
agentTurnCompletionSound, completionSoundPrevTurnActiveRef,
completionSoundArmedRef, turnActive, selectedSessionAwaitingInput) to require
completionSoundInterruptedRef.current === false (or clear it) before calling
playAgentTurnCompletionSound so aborted/interrupt paths do not play the sound.
🪄 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: 03247596-9185-40fc-9a12-fe190fbd53f0
📒 Files selected for processing (11)
apps/desktop/src/renderer/components/app/CommandPalette.tsxapps/desktop/src/renderer/components/app/SettingsPage.tsxapps/desktop/src/renderer/components/chat/AgentChatPane.tsxapps/desktop/src/renderer/components/chat/ChatSurfaceShell.tsxapps/desktop/src/renderer/components/settings/AppearanceSection.tsxapps/desktop/src/renderer/components/settings/GeneralSection.tsxapps/desktop/src/renderer/state/appStore.test.tsapps/desktop/src/renderer/state/appStore.tsapps/desktop/src/shared/modelRegistry.test.tsapps/desktop/src/shared/modelRegistry.tsapps/ios/ADE/Views/PRs/CreatePrWizardView.swift
✅ Files skipped from review due to trivial changes (2)
- apps/ios/ADE/Views/PRs/CreatePrWizardView.swift
- apps/desktop/src/shared/modelRegistry.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/desktop/src/shared/modelRegistry.ts
- apps/desktop/src/renderer/state/appStore.test.ts
- apps/desktop/src/renderer/components/settings/GeneralSection.tsx
- apps/desktop/src/renderer/state/appStore.ts
| <div | ||
| style={{ | ||
| border: `1px solid ${COLORS.border}`, | ||
| background: COLORS.recessedBg, | ||
| padding: 14, | ||
| maxHeight: 280, | ||
| overflow: "auto", | ||
| transform: `scale(${previewScale})`, | ||
| transformOrigin: "top left", | ||
| width: `${100 / previewScale}%`, | ||
| maxWidth: `${100 / previewScale}%`, | ||
| }} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd apps/desktop && find . -name "AppearanceSection.tsx" -type fRepository: arul28/ADE
Length of output: 110
🏁 Script executed:
cat -n apps/desktop/src/renderer/components/settings/AppearanceSection.tsx | sed -n '1,250p'Repository: arul28/ADE
Length of output: 10505
🏁 Script executed:
grep -r "CHAT_FONT_SIZE_MIN_PX\|CHAT_FONT_SIZE_MAX_PX\|DEFAULT_CHAT_FONT_SIZE_PX" apps/desktop/src/renderer/state/ --include="*.ts*"Repository: arul28/ADE
Length of output: 903
🏁 Script executed:
grep -r "zoom\|scale" apps/desktop/src/renderer --include="*.tsx" --include="*.ts" -B 2 -A 2 | head -80Repository: arul28/ADE
Length of output: 7251
🏁 Script executed:
find apps/desktop/src/renderer -name "*.tsx" -o -name "*.ts" | xargs grep -l "ChatMarkdown\|chat.*timeline\|composer" | head -10Repository: arul28/ADE
Length of output: 717
🏁 Script executed:
grep -r "transform.*scale\|zoom.*font\|scale.*font" apps/desktop/src/renderer/components/chat --include="*.tsx" -B 2 -A 2Repository: arul28/ADE
Length of output: 36
🏁 Script executed:
cat -n apps/desktop/src/renderer/components/chat/chatMarkdown.tsx | head -100Repository: arul28/ADE
Length of output: 3836
🏁 Script executed:
cat -n apps/desktop/src/renderer/components/lanes/laneDesignTokens.tsx | head -100Repository: arul28/ADE
Length of output: 149
🏁 Script executed:
find apps/desktop/src/renderer -name "*designTokens*" -o -name "*design*tokens*"Repository: arul28/ADE
Length of output: 117
🏁 Script executed:
grep -r "laneDesignTokens\|COLORS\|cardStyle" apps/desktop/src/renderer/components/settings/AppearanceSection.tsxRepository: arul28/ADE
Length of output: 1842
🏁 Script executed:
find apps/desktop/src/renderer -path "*/lanes/*" -name "*.ts*" | head -10Repository: arul28/ADE
Length of output: 697
🏁 Script executed:
find apps/desktop/src -name "*laneDesignTokens*"Repository: arul28/ADE
Length of output: 116
🏁 Script executed:
cat -n apps/desktop/src/renderer/components/lanes/laneDesignTokens.ts | head -150Repository: arul28/ADE
Length of output: 4534
🏁 Script executed:
grep -r "zoom\|scale" apps/desktop/src/renderer/components/chat --include="*.tsx" -n | head -30Repository: arul28/ADE
Length of output: 1181
🏁 Script executed:
sed -n '710,730p' apps/desktop/src/renderer/components/chat/AgentChatPane.tsxRepository: arul28/ADE
Length of output: 1365
🏁 Script executed:
sed -n '200,235p' apps/desktop/src/renderer/components/settings/AppearanceSection.tsxRepository: arul28/ADE
Length of output: 1650
Match the preview scaling to the chat shell using the zoom property.
The live preview currently uses transform: scale(...) with width compensation, which doesn't reserve layout height and can cause visual overflow at larger font sizes (e.g., 24px). The actual chat shell uses zoom for scaling (see AgentChatPane.tsx line 717), which properly reserves space. Align the preview to use the same approach.
🖼️ Proposed fix to match shell scaling
- transform: `scale(${previewScale})`,
- transformOrigin: "top left",
- width: `${100 / previewScale}%`,
- maxWidth: `${100 / previewScale}%`,
+ zoom: previewScale,Verify by moving the slider to 24px and confirming the preview stays inside the card without overlapping "Chat & notifications".
📝 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.
| <div | |
| style={{ | |
| border: `1px solid ${COLORS.border}`, | |
| background: COLORS.recessedBg, | |
| padding: 14, | |
| maxHeight: 280, | |
| overflow: "auto", | |
| transform: `scale(${previewScale})`, | |
| transformOrigin: "top left", | |
| width: `${100 / previewScale}%`, | |
| maxWidth: `${100 / previewScale}%`, | |
| }} | |
| <div | |
| style={{ | |
| border: `1px solid ${COLORS.border}`, | |
| background: COLORS.recessedBg, | |
| padding: 14, | |
| maxHeight: 280, | |
| overflow: "auto", | |
| zoom: previewScale, | |
| }} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/renderer/components/settings/AppearanceSection.tsx` around
lines 206 - 217, The preview container in AppearanceSection.tsx is using
transform: `scale(${previewScale})` with width compensation which doesn't
reserve layout height; change the styling on the preview div to use the CSS
`zoom` property set to `previewScale` (matching the chat shell's approach used
in AgentChatPane) and remove the transform, transformOrigin, and manual
width/maxWidth adjustments so the card's layout height is preserved; keep other
styles like border, background, padding, maxHeight, and overflow unchanged and
verify the preview no longer overflows at large font sizes (e.g., 24px).
- Appearance: drop user-visible t3code/GitHub link; keep neutral copy - ChatSurfaceShell: scale header/body/footer via transform + inverse dimensions (Firefox-safe) instead of CSS zoom; contentScale prop - AgentChatPane: pass contentScale from chat font preference - agentTurnCompletionSound: module + function docstrings (CodeRabbit hint) - Add ChatSurfaceShell scale wrapper tests with cleanup between cases Co-authored-by: Arul Sharma <arul28@users.noreply.github.com>
| background: COLORS.recessedBg, | ||
| padding: 14, | ||
| maxHeight: 280, | ||
| overflow: "auto", |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
The live preview applies overflow: auto and transform: scale() to the same element. CSS overflow clips/scrolls against that element’s padding box, while transforms only change the visual rendering, not the scroll container’s layout box. As previewScale grows, the ChatMarkdown preview becomes visually taller but the scrollable region does not grow with it, so the bottom of the preview becomes unreachable at larger font sizes. Keep maxHeight/overflow on an outer wrapper and move the scale transform to an inner child.
// apps/desktop/src/renderer/components/settings/AppearanceSection.tsx
maxHeight: 280,
overflow: "auto",
transform: `scale(${previewScale})`,
transformOrigin: "top left",
width: `${100 / previewScale}%`,| document.body.appendChild(ta); | ||
| ta.select(); | ||
| const ok = document.execCommand("copy"); | ||
| document.body.removeChild(ta); | ||
| return Promise.resolve(ok); |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
The new fallback branch appends a hidden <textarea> to document.body, but it only removes that node on the success path. If ta.select() or document.execCommand("copy") throws, control jumps to the outer catch and the appended element stays in the DOM forever, so repeated failed copy attempts in the unsupported browsers this fallback targets will accumulate hidden nodes. Move the cleanup into a finally so the temporary textarea is always removed.
// apps/desktop/src/renderer/components/chat/CodeHighlighter.tsx
document.body.appendChild(ta);
ta.select();
const ok = document.execCommand("copy");
document.body.removeChild(ta);
return Promise.resolve(ok);| document.body.appendChild(ta); | |
| ta.select(); | |
| const ok = document.execCommand("copy"); | |
| document.body.removeChild(ta); | |
| return Promise.resolve(ok); | |
| document.body.appendChild(ta); | |
| try { | |
| ta.select(); | |
| return Promise.resolve(document.execCommand("copy")); | |
| } finally { | |
| ta.remove(); | |
| } |
Repair grepSearch JS-fallback tests by routing ripgrep exec through a test hook; relax TerminalView WebGL expectation for headless CI; play completion sound only on successful turns; preserve case-sensitive model refs with provider hints; improve Appearance preview layout, a11y, and clipboard cleanup; keep banner dismissals across project switches; make iOS commitsAheadOfBase backward compatible and show lane subtitles. Co-authored-by: Arul Sharma <arul28@users.noreply.github.com>
cursor
Bot
changed the title
| code: string; | ||
| language: string; | ||
| }) { | ||
| const copyButtonPosition = useAppStore((s) => s.codeBlockCopyButtonPosition); |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
This new store read only changes HighlightedCode, but semantic reference tracing shows HighlightedCode is currently used only by @apps/desktop/src/renderer/components/prs/shared/PrMarkdown.tsx:464; the actual chat renderer in @apps/desktop/src/renderer/components/chat/AgentChatMessageList.tsx:597-623 still renders fenced blocks with plain <pre>/<code> and never consults codeBlockCopyButtonPosition. That means the new Appearance setting labeled for chat code blocks has no effect where users expect it, and instead changes PR review markdown. Route chat fenced blocks through the shared highlighter/copy-button component or apply the same positioning logic in MarkdownBlock so the preference actually controls chat code blocks.
// apps/desktop/src/renderer/components/chat/CodeHighlighter.tsx
export const HighlightedCode = React.memo(function HighlightedCode({
code,
language,
}) {
const copyButtonPosition = useAppStore((s) => s.codeBlockCopyButtonPosition);| if (!SKIP_DIRS.has(entry.name) && !entry.name.startsWith(".")) { | ||
| // Skip only known bulky/tooling dirs — do not treat every dot-directory as | ||
| // ignorable so paths like `.github/` remain searchable when targeted. | ||
| if (!SKIP_DIRS.has(entry.name)) { |
There was a problem hiding this comment.
[🟠 High] [🔵 Bug]
Removing the dot-directory guard makes the auto-allowed grep tool recurse through every hidden subtree when the caller searches the repo root, including ADE’s machine-local .ade/secrets files. collectFiles() already walks an explicitly targeted hidden directory, so this change is not required to support path: ".github"; it only broadens default searches into sensitive metadata. Because grepSearch returns raw matching lines and has no redaction layer, an agent can now exfiltrate API keys / sync tokens with a normal repo-root grep. Restore the hidden-dir skip for broad walks and only descend into dot-directories when the requested target itself is inside one.
// apps/desktop/src/main/services/ai/tools/grepSearch.ts
if (entry.isDirectory()) {
// Skip only known bulky/tooling dirs — do not treat every dot-directory as
// ignorable so paths like `.github/` remain searchable when targeted.
if (!SKIP_DIRS.has(entry.name)) {
walk(path.join(current, entry.name));| const runtime = getTerminalRuntimeSnapshot("session-valid"); | ||
| expect(runtime?.renderer).toBe("webgl"); | ||
| // WebGL is unavailable in many CI / headless environments; DOM fallback is expected. | ||
| expect(runtime?.renderer === "webgl" || runtime?.renderer === "dom").toBe(true); |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
This change weakens the test instead of making it environment-tolerant. In this test harness @xterm/addon-webgl is already mocked, so the runtime should still reach the WebGL path unless the test explicitly forces failure. But createRuntime() initializes every runtime with renderer: "dom" before the async initRendererChain() runs, and this test only does a single flushAllTimers(). That means the new assertion can pass even when renderer initialization never completed, so it no longer validates either the preferred renderer path or a genuine fallback. Use the same extra settle loop as the later fallback test before asserting renderer state, or drop the renderer assertion entirely if this case is only meant to verify fit/PTY resize.
// apps/desktop/src/renderer/components/terminals/TerminalView.test.tsx
const runtime = getTerminalRuntimeSnapshot("session-valid");
// WebGL is unavailable in many CI / headless environments; DOM fallback is expected.
expect(runtime?.renderer === "webgl" || runtime?.renderer === "dom").toBe(true);…olish Skip hidden root-only dirs (e.g. .ade) in JS grep when searching the whole repo while still entering .github; add regression test. Route ChatMarkdown fenced blocks through HighlightedCode so copy placement applies in previews. Stub WebGL canvas in TerminalView tests instead of weakening assertions. Tighten Appearance section labels and helper copy to sentence case. Co-authored-by: Arul Sharma <arul28@users.noreply.github.com>
| if (becameIdle && completionSoundArmedRef.current) { | ||
| completionSoundArmedRef.current = false; | ||
| let lastDoneStatus: "completed" | "interrupted" | "failed" | null = null; | ||
| for (let i = selectedEventsForDisplay.length - 1; i >= 0; i -= 1) { |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
This effect decides whether to play the notification by scanning backward for the last "done" event anywhere in the session history, not the "done" that corresponds to the turn that just became idle.
// apps/desktop/src/renderer/components/chat/AgentChatPane.tsx
let lastDoneStatus: "completed" | "interrupted" | "failed" | null = null;
for (let i = selectedEventsForDisplay.length - 1; i >= 0; i -= 1) {
const ev = selectedEventsForDisplay[i]?.event;
if (ev?.type === "done") {
lastDoneStatus = ev.status;
break;
}
}I verified against the existing interrupt() flow in the same file: it sets turnActiveBySession[selectedSessionId] = false before window.ade.agentChat.interrupt() delivers the new done(interrupted) envelope, so becameIdle is true while the most recent historical done can still be the previous successful turn. That makes the success sound fire for an interrupted turn. Gate playback on the current turn’s terminal event instead of any prior done in selectedEventsForDisplay (for example by checking the newest event or tracking the active turn id/status).
| key={id} | ||
| type="button" | ||
| aria-pressed={codeBlockCopyButtonPosition === id} | ||
| aria-labelledby="code-block-copy-position-label" |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
aria-labelledby takes precedence over a button's own text, so both controls are exposed to assistive tech with the same accessible name from the shared label node instead of distinct Top/Bottom names. That leaves the new setting effectively ambiguous for screen-reader users: they can hear the pressed state, but not which option they are choosing. Keep the group label separate and give each button its own accessible name, for example with a per-button aria-label or a composed aria-labelledby that also includes the option text.
// apps/desktop/src/renderer/components/settings/AppearanceSection.tsx
<button
key={id}
type="button"
aria-pressed={codeBlockCopyButtonPosition === id}
aria-labelledby="code-block-copy-position-label"| aria-labelledby="code-block-copy-position-label" | |
| aria-label={`Code block copy button ${id === "bottom" ? "bottom" : "top"}`} |
| const [hovered, setHovered] = useState(false); | ||
|
|
||
| return ( | ||
| <button |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
The new theme picker uses plain buttons and conveys selection only through border/background styling. Because the swatches never expose aria-pressed, aria-checked, or radio semantics, assistive-tech users can focus Dark and Light but cannot tell which theme is currently active after the feature is enabled. Add a programmatic selected state to each swatch (or convert the pair to a proper radio group) so the current theme is announced.
// apps/desktop/src/renderer/components/settings/AppearanceSection.tsx
<button
type="button"
onClick={onClick}
onMouseEnter={() => setHovered(true)}
onMouseLeave={() => setHovered(false)}| const match = /language-([\w-]+)/.exec(className); | ||
| const language = match ? match[1] : "text"; | ||
| const codeText = extractPlainText(props?.children).replace(/\n$/, ""); | ||
| return <HighlightedCode code={codeText} language={language} />; |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
These added lines wire HighlightedCode only into ChatMarkdown:
// apps/desktop/src/renderer/components/chat/chatMarkdown.tsx
const language = match ? match[1] : "text";
const codeText = extractPlainText(props?.children).replace(/\n$/, "");
return <HighlightedCode code={codeText} language={language} />;I verified that ChatMarkdown is only consumed by @apps/desktop/src/renderer/components/chat/AgentQuestionModal.tsx and @apps/desktop/src/renderer/components/chat/ChatProposedPlanCard.tsx, while the main assistant transcript renderer in @apps/desktop/src/renderer/components/chat/AgentChatMessageList.tsx:550-623 still renders fenced blocks with raw <pre>/<code> and never reads codeBlockCopyButtonPosition; get_pr_diff also shows no transcript changes in this PR. As shipped, the new top/bottom preference works in previews and plan cards but not in the actual chat message timeline, which is the primary code-block surface. Reuse HighlightedCode or the shared markdown component from the transcript renderer so the setting applies consistently.
| if (SKIP_DIRS.has(entry.name)) continue; | ||
| const next = path.join(current, entry.name); | ||
| if ( | ||
| shouldSkipHiddenDirUnderRepoRoot(rootReal, current, entry.name, searchWholeRepo) |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
This change only skips dot-directories when searchWholeRepo is true, so targeted directory searches no longer preserve the old hidden-dir guard. In the new code:
// apps/desktop/src/main/services/ai/tools/grepSearch.ts
if (entry.isDirectory()) {
if (SKIP_DIRS.has(entry.name)) continue;
const next = path.join(current, entry.name);
if (shouldSkipHiddenDirUnderRepoRoot(rootReal, current, entry.name, searchWholeRepo)) {
continue;
}
walk(next);
}tool.execute({ path: "src", ... }) will now recurse into src/.cache, src/.storybook, etc., whereas the previous implementation skipped all entry.name.startsWith(".") directories and the PR note says targeted subdirectory searches are unchanged. That broadens the agent-visible search surface beyond the intended .github exception. Restore the old hidden-directory exclusion for non-root searches, and only special-case direct children of the repo root when searchWholeRepo is true.
| vi.stubGlobal("ResizeObserver", MockResizeObserver); | ||
| vi.stubGlobal("IntersectionObserver", MockIntersectionObserver); | ||
| // xterm WebGL path needs a getContext("webgl") that succeeds in jsdom / headless CI. | ||
| vi.stubGlobal( |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
This new stub is never consulted by the code under test, so the test still does not prove that jsdom can initialize the WebGL renderer. ```ts
// apps/desktop/src/renderer/components/terminals/TerminalView.test.tsx
vi.stubGlobal(
"HTMLCanvasElement",
class extends (globalThis as any).HTMLCanvasElement {
getContext(contextId: string) {
`TerminalView` sets `runtime.renderer = "webgl"` only through `initRendererChain()`/`setRenderer()` when `new WebglAddon()` succeeds, and this file already mocks `@xterm/addon-webgl` plus `Terminal.loadAddon()` without any `canvas.getContext()` call. As a result the happy-path assertion still passes because the mocked addon constructor succeeds, not because the real WebGL probe or async renderer init completed. To make this test meaningful again, either make the mock WebGL addon explicitly exercise a canvas WebGL probe, or stop mocking that addon in this test and wait for `runtime.renderer` to settle before asserting.
Normalize project config paths and improve run-page, chat UI, and related behavior. Summary of changes: - Project config: add path normalization helpers (normalizeConfigPath, projectRelativePath, normalizeProjectCwd/Command) and apply them when validating, saving, and returning snapshots so absolute project-root paths are converted to portable relative paths. Persist normalized shared/local in snapshots. - Tests: add unit test to verify project-root absolute paths are normalized and that saved ade.yaml contains portable paths. - RunPage refactor: introduce helpers to convert absolute paths to project-relative, build/update process config definitions, handle local vs shared placement (localOnly), and add upsert/remove helpers for processes and stack buttons. Adjust save logic and dependencies. - Grep search: surface descriptive "Invalid regex pattern" errors from JS fallback and improve glob handling to match bare filenames for patterns like **/*.ts; add tests for invalid regex and glob edge cases. - IPC: catch errors from resolveLaneOverlayContext during lane delete, log a warning, and continue. - App shell & store: move per-session banner-dismiss state into app store (avoid local-state leaks) and use store dismiss actions. - Chat & code UI: switch many inline styles to CSS variables, improve code copy button (auto/sticky mode), better diff coloring via styles, and style enhancements for code blocks, inline code, and tables. Wire code-copy position from store. - Agent sounds & settings: add volume and quiet-when-focused preferences, pass options to playAgentTurnCompletionSound, and update AppearanceSection UI (chat font-size swatches, sound controls, terminal options import). - Misc: make .ade/ade.yaml use project-relative script paths and update finalize.md to add a Mobile Parity agent checklist and instructions. Why: make config files portable across machines, provide clearer errors and more ergonomic UI/UX for chat/code blocks, and ensure consistent handling of local vs shared config changes when editing processes from the Run page.
|
|
||
| return ( | ||
| <button | ||
| type="button" |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
ThemeSwatch renders the current theme as a plain button with no selected-state semantics:
// apps/desktop/src/renderer/components/settings/AppearanceSection.tsx
<button
type="button"
onClick={onClick}
onMouseEnter={() => setHovered(true)}A screen reader can activate these buttons, but it cannot tell which theme is currently selected, so reopening Appearance loses that state for non-visual users. Add aria-pressed/aria-checked semantics (consistent with the repo’s other custom toggles) so the active theme is announced.
| type="button" | |
| type="button" aria-pressed={selected} |
| ))} | ||
| <option value="__custom__">Custom stack</option> | ||
| </select> | ||
| <input |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
The freeform font-family field has no programmatic label of its own:
// apps/desktop/src/renderer/components/settings/AppearanceSection.tsx
<label htmlFor={`${terminalFieldId}-fontFamily`}>
Font family
</label>
<select id={`${terminalFieldId}-fontFamily`} ... />
<input
value={terminalPreferences.fontFamily}The visible label points at the preset <select>, not the custom-stack <input>, so keyboard and screen-reader users tab into an unnamed text field and cannot tell what setting it edits. Give that input its own accessible name (dedicated label or aria-label).
| <input | |
| <input aria-label="Custom terminal font family" |
| {copyButtonPosition !== "auto" && ( | ||
| <CodeCopyButton code={trimmedCode} position={copyButtonPosition} /> | ||
| )} | ||
| <div className="overflow-x-auto whitespace-pre-wrap break-words px-4 py-3"> |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
In auto mode the button is rendered as position: sticky, but it is nested inside the same wrapper that still has overflow-x-auto, so CSS sticky binds to that nearest overflow ancestor instead of the transcript pane. MDN documents that any ancestor with overflow: auto/hidden/scroll becomes the sticky container even when it is not the element actually scrolling vertically, which means the new “tracks the viewport as you scroll” option will stay pinned within the code block rather than following chat scroll. Move the sticky row outside the horizontal overflow element, or split the horizontal scroller into a child wrapper under the sticky row. ```tsx
// apps/desktop/src/renderer/components/chat/CodeHighlighter.tsx
| // last segment (the filename pattern) so `src/*.ts` still matches `foo.ts`. | ||
| let pattern = glob.replace(/\*\*/g, "*"); | ||
| const lastSlash = pattern.lastIndexOf("/"); | ||
| if (lastSlash !== -1) pattern = pattern.slice(lastSlash + 1); |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
globToRegex now drops every path segment before compiling the fallback regex, so when rg is unavailable a request like src/*.ts or services/**/*.ts no longer restricts results to that subtree — it matches any *.ts / index.ts anywhere under the scan root. That changes the meaning of the glob argument relative to the primary ripgrep path and can return unrelated files to agents in fallback-only environments. Preserve and match against a relative file path instead of slicing to the last segment; only special-case the **/ collapse if needed.
// apps/desktop/src/main/services/ai/tools/grepSearch.ts
let pattern = glob.replace(/\*\*/g, "*");
const lastSlash = pattern.lastIndexOf("/");
if (lastSlash !== -1) pattern = pattern.slice(lastSlash + 1);| } | ||
|
|
||
| function projectRelativePath(projectRoot: string, absolutePath: string, basePath: string): string | null { | ||
| if (!path.isAbsolute(absolutePath)) return null; |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
projectRelativePath only treats host-native absolute paths as absolute, so a config committed from Windows is not normalized when opened on macOS/Linux (and vice versa). The changed code now does:
// apps/desktop/src/main/services/config/projectConfigService.ts
function projectRelativePath(projectRoot: string, absolutePath: string, basePath: string): string | null {
if (!path.isAbsolute(absolutePath)) return null;
try {
const resolved = resolvePathWithinRoot(projectRoot, absolutePath, { allowMissing: true });Node's path.isAbsolute() is platform-specific, and resolvePathWithinRoot() falls back to rebasing non-absolute candidates under the project root in @apps/desktop/src/main/services/shared/utils.ts, so values like C:\repo\scripts\dogfood.sh become C:/repo/... and are then treated as ./C:/repo/... on POSIX instead of being converted to a portable relative path. That breaks the new portability feature for cross-platform repositories and can leave an invalid executable/cwd in the saved snapshot. Detect foreign-platform absolutes before this check (for example with path.win32.isAbsolute/path.posix.isAbsolute) and convert them relative to projectRoot the same way as native absolutes.
Enhance grep glob matching and path normalization, add tests, and apply minor UI/accessibility tweaks.
- grepSearch: Normalize backslashes in file globs, detect when a glob includes directory components, and match against relative file paths when appropriate. Rewrote globToRegex to correctly handle **/, **, *, ?, and {a,b} patterns while preserving directory semantics. Added tests to exercise directory globs and JS fallback behavior.
- projectConfigService: Add utilities to detect absolute paths across platforms, infer project-relative paths from foreign-platform absolute paths (e.g. Windows paths on POSIX), and return portable relative paths for config saving. Added a test to ensure foreign absolute process paths are normalized to portable relative paths in saved config.
- CodeHighlighter: Adjust copy button rendering so the auto position renders consistently.
- AppearanceSection: Add aria-pressed to theme swatch button and an aria-label to the custom terminal font input to improve accessibility.
These changes improve cross-platform behavior for globs and config paths and address minor UX/accessibility issues.
| if (SKIP_DIRS.has(entry.name)) continue; | ||
| const next = path.join(current, entry.name); | ||
| if ( | ||
| shouldSkipHiddenDirUnderRepoRoot(rootReal, current, entry.name, searchWholeRepo) |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
The new walker removed the old entry.name.startsWith(".") guard and now only skips hidden directories when they are direct children of the repo root in a repo-wide search:
// apps/desktop/src/main/services/ai/tools/grepSearch.ts
if (entry.isDirectory()) {
if (SKIP_DIRS.has(entry.name)) continue;
const next = path.join(current, entry.name);
if (shouldSkipHiddenDirUnderRepoRoot(rootReal, current, entry.name, searchWholeRepo)) {That changes fallback behavior from ripgrep’s default filtering (rg skips hidden files/directories unless explicitly opted in): a targeted search like path: "src" will now recurse into src/.cache, and a repo-wide search will also descend into nested hidden directories such as src/.storybook. Since this code path is used exactly when rg is unavailable, those environments now get noisier, inconsistent results that the new tests do not cover. Restore the general hidden-directory skip in collectFiles and add the .github exception only for direct children of the repo root (while still allowing an explicitly targeted hidden root to be searched).
| } | ||
|
|
||
| const projectDirName = rootSegments[rootSegments.length - 1]; | ||
| const projectDirIndex = candidateSegments.lastIndexOf(projectDirName); |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
inferProjectRelativePath() now falls back to the last segment of projectRoot and treats any foreign absolute path containing that basename as if it belonged to the current repo. That means a config authored with an external tool path such as C:\tools\ADE\bin\review.exe for a project rooted at /home/me/ADE is rewritten to bin/review.exe, even though the executable is outside the project. Because save() persists the normalized argv, validation only constrains cwd, and the process/test runners later pass command[0] directly to spawn, this silently changes which binary ADE will execute. The problematic branch is:
// apps/desktop/src/main/services/config/projectConfigService.ts
const projectDirName = rootSegments[rootSegments.length - 1];
const projectDirIndex = candidateSegments.lastIndexOf(projectDirName);
if (projectDirIndex === -1) return null;
return candidateSegments.slice(projectDirIndex + 1).join("/") || ".";Tighten the foreign-path inference so it only rewrites when the full project-root suffix matches (or otherwise has stronger evidence that the path belongs to the same project); otherwise leave the absolute path unchanged.
2 participants
Latest (copy + review follow-up)
.ade) are skipped so broad searches do not walk machine-local trees;.githubis still searched. Added a unit test. Targeted subdirectory / single-file searches are unchanged.HighlightedCode(same path as PR markdown), so appearance settings for code-block copy placement apply to the live preview and any otherChatMarkdownsurfaces.HTMLCanvasElement#getContext("webgl")in jsdom so the test can assert the WebGL path again instead of accepting DOM fallback.Commit:
c8f027a9Earlier summary (still in this PR)
Model slug resolution, PR
commitsAheadOfBase, chat scaling/sound/copy controls, appearance tab, iOS parity, CI fixes for grepSearch test harness and vitest shards, and other review-driven fixes from prior pushes.Summary by CodeRabbit
New Features
UI
Tests
Greptile Summary
This PR adds model slug resolution, a new Appearance settings tab (theme swatches, chat font-size, code-copy button position, agent completion sounds), iOS PR wizard commits-ahead hints, per-project session-dismissible banners, and fixes the JS grep fallback to skip hidden root-level directories. Prior review concerns (AudioContext close timing, iOS subtitle never rendered) have been addressed.
One remaining issue: the Preview button in
AppearanceSectioncarries a comment claiming it skips the debounce for manual plays, but the implementation does not call__resetAgentTurnCompletionSoundDebounce(), so rapid preview clicks within the 1500 ms window will produce no sound.Confidence Score: 4/5
Safe to merge with one minor UX fix — all P0/P1 concerns from prior rounds are resolved.
The only new finding is the Preview button debounce bypass that's described in a comment but not implemented, causing silent no-ops for rapid preview clicks. This is a P2 UX issue, not a data-integrity or security problem. Prior P0 concerns (AudioContext premature close, iOS subtitle never rendered) have both been addressed in this revision.
apps/desktop/src/renderer/components/settings/AppearanceSection.tsx — preview button debounce bypass
Important Files Changed
Prompt To Fix All With AI
Reviews (8): Last reviewed commit: "Improve glob/path handling and small UI/..." | Re-trigger Greptile