Mobile Ui Fixes#212
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
@copilot review but do not make fixes |
📝 WalkthroughWalkthroughAdds desktop image/icon IPC and project icon resolution, structured CLI launch commands and PTY/session refinements, image preview/clipboard flows for chat attachments and project-tab icons, and extensive iOS UI, sync, live-activity, and project-icon support including chat-summary caching. 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 |
Here's my review of this PR: OverviewLarge PR touching iOS (work terminal, lane detail, commit sheet, home screen) and desktop (TopBar icons, chat image attachments, PTY service, App visibility). Well-scoped for a "mobile UI fixes" round. ConcernsiOS —
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![capy-ai[bot]](https://avatars.githubusercontent.com/in/1915919?s=60&v=4){kind=small}
| let lastActivity = | ||
| Self.parseIso8601(summary?.lastActivityAt ?? "") | ||
| ?? Self.parseIso8601(session.chatIdleSinceAt ?? "") | ||
| ?? started |
There was a problem hiding this comment.
[🟠 High] [🔵 Bug]
The old code used Self.parseIso8601(session.endedAt ?? "") ?? now which defaulted to now for running sessions (no endedAt), so the recency gate always passed. The new fallback chain summary?.lastActivityAt → chatIdleSinceAt → started uses started as the terminal fallback. For a running session with no cached summary (before the user opens the Work screen) and no chatIdleSinceAt (nil for actively-running chats), lastActivity resolves to started. Once the session is older than runningRecencyCutoff (120 seconds), the check lastActivity >= runningRecencyCutoff fails and the session is excluded from runningAgents, making the Live Activity empty despite an actively-streaming chat.
// apps/ios/ADE/Services/SyncService.swift
let summary = chatSummaryCache[session.id]
let lastActivity =
Self.parseIso8601(summary?.lastActivityAt ?? "")
?? Self.parseIso8601(session.chatIdleSinceAt ?? "")
?? started // ← falls back to session start time, not `now`The chatSummaryCache is only populated when the user navigates to the Work list or opens a chat detail (via cacheChatSummaries/cacheChatSummary). On a cold launch with a running session, the cache is empty, so any session older than 2 minutes silently vanishes from the lock screen. Fix by falling back to now for sessions in running runtimeState (preserving old behavior for the recency gate), or use a separate timestamp for the elapsed-time display vs. the recency filter.
| let lastActivity = | |
| Self.parseIso8601(summary?.lastActivityAt ?? "") | |
| ?? Self.parseIso8601(session.chatIdleSinceAt ?? "") | |
| ?? started | |
| let lastActivity = | |
| Self.parseIso8601(summary?.lastActivityAt ?? "") | |
| ?? Self.parseIso8601(session.chatIdleSinceAt ?? "") | |
| ?? (isRunningRuntime ? now : started) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
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/Services/LiveActivityCoordinator.swift (1)
235-288:⚠️ Potential issue | 🟠 MajorAwaiting-input Live Activities can no longer surface.
selectAttention(...)still supports the awaiting-input branch, but this method hardcodesawaitingInputCountto0and publishesawaitingInputCount: 0. When all chats are waiting for user input,sessionsis empty andattentionbecomes nil, soreconcileNow(...)tears the Live Activity down instead of showing the existing “Tap to respond” state.🔧 Suggested fix
public func reconcile( with sessions: [AgentSnapshot], prs: [PrSnapshot] = [], + awaitingInputCount: Int = 0, focusedLaneName: String? = nil ) { @@ await self.reconcileNow( with: sessions, prs: prs, + awaitingInputCount: awaitingInputCount, focusedLaneName: focusedLaneName ) } } } private func reconcileNow( with sessions: [AgentSnapshot], prs: [PrSnapshot], + awaitingInputCount: Int, focusedLaneName: String? ) async { @@ - let desiredState = makeContentState(sessions: sessions, prs: prs) + let desiredState = makeContentState( + sessions: sessions, + prs: prs, + awaitingInputCount: awaitingInputCount + ) } private func makeContentState( sessions: [AgentSnapshot], - prs: [PrSnapshot] + prs: [PrSnapshot], + awaitingInputCount: Int ) -> ADESessionAttributes.ContentState { @@ let attention: ADESessionAttributes.ContentState.Attention? = selectAttention( sessions: sorted, prs: prs, - awaitingInputCount: 0, + awaitingInputCount: awaitingInputCount, failingChecks: failingChecks, awaitingReviews: awaitingReviews, mergeReady: mergeReady ) @@ - awaitingInputCount: 0, + awaitingInputCount: awaitingInputCount, idleCount: 0, generatedAt: Date() ) }
SyncService.reconcile(...)should then threadawaitingInputSessionsCountback through this API.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ios/ADE/Services/LiveActivityCoordinator.swift` around lines 235 - 288, makeContentState currently hardcodes awaitingInputCount to 0 which prevents the awaiting-input attention state from appearing; compute the real awaiting-input count (e.g., let awaitingInputCount = sessions.filter { $0.awaitingInput }.count or from sorted/activeSessions as appropriate), pass that variable into selectAttention(sessions:..., awaitingInputCount: awaitingInputCount, ...) and set awaitingInputCount: awaitingInputCount in the returned ADESessionAttributes.ContentState so the Live Activity can show the “Tap to respond” state.
🧹 Nitpick comments (6)
apps/desktop/src/main/services/sync/syncRemoteCommandService.test.ts (1)
431-439: Consider matching async semantics across conflict-service mocks.The other conflictService methods in this mock (
getLaneStatus,listOverlaps,getBatchAssessment) usemockResolvedValue, butdismissRebase/deferRebaseuse barevi.fn(). If the real implementations are async (see the source-file comment), these sync mocks will hide await-related bugs. Once the source-side awaits are settled, consider:- dismissRebase: vi.fn(), - deferRebase: vi.fn(), + dismissRebase: vi.fn().mockResolvedValue(undefined), + deferRebase: vi.fn().mockResolvedValue(undefined),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/sync/syncRemoteCommandService.test.ts` around lines 431 - 439, The mock in createMockConflictService declares dismissRebase and deferRebase as vi.fn() (sync) while other methods use mockResolvedValue; update dismissRebase and deferRebase to be async mocks (e.g., vi.fn().mockResolvedValue(undefined) or mockResolvedValue(null)) so their semantics match getLaneStatus/listOverlaps/getBatchAssessment and won't hide await-related issues in callers that await these methods.apps/ios/ADE/Views/Lanes/LaneDetailScreen.swift (1)
243-251:showBranchPickersheet won't dismiss cleanly ifdetailbecomes nil mid-flight.Using
isPresented:with conditional content (if let detail) means SwiftUI keeps the sheet "presented" but renders nothing ifdetailflips to nil during a refresh — leaving a blank sheet on screen. Prefer.sheet(item:)keyed off the detail (or off a dedicatedBranchPickerContextvalue), so the sheet dismisses with the data.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ios/ADE/Views/Lanes/LaneDetailScreen.swift` around lines 243 - 251, The sheet uses isPresented: $showBranchPicker with conditional content if let detail which can leave a blank sheet when detail becomes nil; change to using sheet(item:) (or introduce a BranchPickerContext struct) so the sheet is keyed to the detail value and will dismiss automatically when the item becomes nil. Update the UI in LaneDetailScreen to present LaneBranchPickerSheet via .sheet(item: $selectedBranchPickerItem) (or .sheet(item: $detail) if appropriate), move the branchRef and laneId into that item/context, and have onComplete call loadDetail(refreshRemote: true) and then clear the item to dismiss the sheet; remove the isPresented usage and ensure showBranchPicker is replaced by the item binding.apps/desktop/src/shared/types/sync.ts (1)
227-227: Operational note: be mindful of catalog payload growth.Inlining a base64
iconDataUrlper project summary multiplies catalog payload size by N projects. Worth confirming the producer inmain.ts(a) downsizes/normalizes icons before encoding, and (b) caps per-icon bytes so a pathological favicon (e.g., a 1 MB PNG embedded as a.ico) can't bloat aproject_catalog/project_catalog_chunkenvelope and stall sync over a slow link.If icons can grow large, consider serving them on-demand via a separate command (icon-by-projectId) rather than fan-out on every catalog broadcast.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/shared/types/sync.ts` at line 227, The project_summary field iconDataUrl can cause unbounded payload growth; update the producer in main.ts that builds project_catalog/project_catalog_chunk to (a) normalize/resize icons before base64-encoding (e.g., downscale to a fixed small favicon size and convert to an efficient format), (b) enforce a hard byte-size cap per icon and skip/replace oversized icons with a placeholder, and (c) if icons may still be large, switch to serving icons on-demand via a separate handler/command (e.g., icon-by-projectId) and send only a lightweight icon reference in the project summary instead of the full iconDataUrl. Ensure the code paths that populate iconDataUrl perform these checks and fallbacks to prevent large fan-out payloads.apps/ios/ADE/App/ContentView.swift (2)
129-145: Minor: simplifyattachedComputerLabelto drop the force-unwrap.
trimmedHost?.isEmpty == falsefollowed bytrimmedHost!is safe but stylistically fragile if the trimming changes later. A single optional binding reads cleaner:- private var attachedComputerLabel: String { - let trimmedHost = syncService.hostName?.trimmingCharacters(in: .whitespacesAndNewlines) - let host = (trimmedHost?.isEmpty == false) ? trimmedHost! : nil + private var attachedComputerLabel: String { + let trimmedHost = syncService.hostName?.trimmingCharacters(in: .whitespacesAndNewlines) + let host = (trimmedHost?.isEmpty == false) ? trimmedHost : nil switch syncService.connectionState {(Once
trimmedHost?.isEmpty == falseis true,trimmedHostis statically non-nil, sohostends up asString?either way and theif let hostarms still work.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ios/ADE/App/ContentView.swift` around lines 129 - 145, The attachedComputerLabel getter currently force-unwraps trimmedHost when creating host; change that to use a single optional binding so we never use "!" — e.g. compute trimmedHost with syncService.hostName?.trimmingCharacters(...), then set host via if let host = trimmedHost, !host.isEmpty (or use guard) so all subsequent switch cases use that non‑force-unwrapped host optional; update references to the local host variable in the switch (in the .connected/.syncing, .connecting, .error cases) to use the safely bound host.
367-382: Cache key bloat and unbounded growth inprojectHomeIconImageCache.Two small concerns worth addressing before this ships to many-project users:
- Key size —
dataUrl as NSStringretains the entire base64 payload (potentially up to ~1.33MB given the desktop-side 1MB cap) as the cache key, in addition to the decodedUIImagevalue. For a catalog with N projects, you’re effectively storing each icon twice (encoded + decoded).- No eviction policy —
NSCacheis created withoutcountLimit/totalCostLimit, so it can grow unboundedly across project switches. WhileNSCachedoes evict under memory pressure, an explicitcountLimitmakes the steady-state predictable.Synchronous base64 decode +
UIImage(data:)also happens on the main thread insidebody; for typical favicons this is negligible, but a 1MB SVG/PNG could cause a hitch on first display.♻️ Suggested refactor
-private let projectHomeIconImageCache = NSCache<NSString, UIImage>() - -private func projectHomeIconImage(from dataUrl: String?) -> UIImage? { - guard let dataUrl, !dataUrl.isEmpty else { return nil } - let cacheKey = dataUrl as NSString - if let cached = projectHomeIconImageCache.object(forKey: cacheKey) { - return cached - } - guard let commaIndex = dataUrl.firstIndex(of: ",") else { return nil } - let base64 = String(dataUrl[dataUrl.index(after: commaIndex)...]) - guard let data = Data(base64Encoded: base64), - let image = UIImage(data: data) - else { return nil } - projectHomeIconImageCache.setObject(image, forKey: cacheKey) - return image -} +private let projectHomeIconImageCache: NSCache<NSString, UIImage> = { + let cache = NSCache<NSString, UIImage>() + cache.countLimit = 64 + return cache +}() + +private func projectHomeIconImage(from dataUrl: String?) -> UIImage? { + guard let dataUrl, !dataUrl.isEmpty else { return nil } + // Hash the data URL so the cache key stays small even for ~1MB payloads. + let cacheKey = String(dataUrl.hashValue) as NSString + if let cached = projectHomeIconImageCache.object(forKey: cacheKey) { + return cached + } + guard let commaIndex = dataUrl.firstIndex(of: ",") else { return nil } + let base64 = String(dataUrl[dataUrl.index(after: commaIndex)...]) + guard let data = Data(base64Encoded: base64), + let image = UIImage(data: data) + else { return nil } + projectHomeIconImageCache.setObject(image, forKey: cacheKey, cost: data.count) + return image +}As per coding guidelines: “iOS Swift app — check for memory management, Swift conventions, and proper SwiftUI patterns.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ios/ADE/App/ContentView.swift` around lines 367 - 382, projectHomeIconImageCache currently stores the full dataUrl string as the cache key and has no limits, causing memory bloat; update projectHomeIconImage(from:) to derive a compact key (e.g., SHA256 or other short digest of dataUrl) instead of using dataUrl as NSString, set sensible limits on projectHomeIconImageCache (countLimit and/or totalCostLimit) and use the image/data size as the cache cost when calling setObject(_:forKey:cost:). Also move expensive work off the main thread by decoding the base64/Data -> UIImage on a background queue (or use an async loader) and then cache the result and deliver it back to the main thread for UI update.apps/ios/ADETests/ADETests.swift (1)
5-13: Freeze the “recent” fixture timestamp once per test run.These defaults now call
Date()on every helper invocation, so fixtures created in the same test no longer share a stable timestamp. That makes recency-based ordering depend on helper call timing instead of explicit test inputs.♻️ Suggested change
-private func recentIso8601Fixture() -> String { - let f = ISO8601DateFormatter() - f.formatOptions = [.withInternetDateTime, .withFractionalSeconds] - return f.string(from: Date()) -} +private let recentIso8601Fixture: String = { + let formatter = ISO8601DateFormatter() + formatter.formatOptions = [.withInternetDateTime, .withFractionalSeconds] + return formatter.string(from: Date()) +}() - lastActivityAt: String = recentIso8601Fixture() + lastActivityAt: String = recentIso8601Fixture - startedAt: String = recentIso8601Fixture() + startedAt: String = recentIso8601FixtureAlso applies to: 6224-6224, 6276-6276
🤖 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 5 - 13, The helper recentIso8601Fixture() currently returns a new ISO string each call (using Date()), causing fixtures in the same test to have different "now" timestamps; change it to compute the ISO string once per test run and return that cached value on subsequent calls (e.g., initialize a static/let frozenIso8601 value using ISO8601DateFormatter and Date() once, and have recentIso8601Fixture() return that frozenIso8601) so all fixtures share a stable recent timestamp; apply the same pattern to any other fixture helpers that call Date() (the comment notes similar instances).
🤖 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/ipc/registerIpc.ts`:
- Around line 1989-2003: The handler ipcMain.handle for IPC.appGetImageDataUrl
currently uses blocking fs.statSync and fs.readFileSync which can freeze the
main process; change it to use the non-blocking fs.promises API (await
fs.promises.stat(filePath) and await fs.promises.readFile(filePath)), keep the
handler async/await throughout, preserve the same validation (isFile and size
limit) and build the dataUrl with inferImageMimeType(filePath) from the awaited
buffer; apply the same conversion for the other similar handlers around the same
area that use sync file I/O.
- Around line 1895-1913: In resolveAllowedRendererPath, do a filesystem
canonicalization of the resolved path before performing the allowlist
containment check: after calling resolveRendererSuppliedPath use fs.realpath (or
fs.realpathSync) to get the real/canonical target and use that canonical path
for the subsequent resolvePathWithinRoot checks; also canonicalize the allowed
directory roots returned by getAllowedDirs (or explicitly reject if realpath
fails) so symlinks cannot bypass the allowlist, and ensure any realpath errors
are caught and converted into the existing "outside allowed directories" or
"Missing path" error flow.
In `@apps/desktop/src/main/services/pty/ptyService.ts`:
- Around line 833-838: The current guard around the Codex storage lookup
prevents resolveCodexSessionIdFromStorage from running when reason ===
"session-list", which breaks recovery of crash/orphaned Codex sessions; update
the conditional in ensureResumeTargets() so that the codex fallback still runs
for "session-list" (i.e. remove the exclusion of "session-list" from the if that
checks effectiveToolType, cwd and reason) while keeping other exclusions like
"resume-launch" intact, so resolveCodexSessionIdFromStorage({ cwd, startedAt:
session.startedAt, maxStartDeltaMs: 10*60_000 }) will be called for session-list
recovery paths.
In `@apps/desktop/src/renderer/browserMock.ts`:
- Around line 1660-1661: The browser mock is missing project.resolveIcon which
TopBar expects; add a resolvedArg entry named resolveIcon on the project mock
that returns a Promise<ProjectIcon> with the shape { dataUrl: string | null;
sourcePath: string | null; mimeType: string | null } (use resolvedArg({ dataUrl:
null, sourcePath: null, mimeType: null })) adjacent to getImageDataUrl and
writeClipboardImage so project.resolveIcon exists and returns the correct typed
object.
In `@apps/desktop/src/renderer/components/app/App.tsx`:
- Around line 196-208: The effect fires based only on active causing
dispatchWorkSurfaceRevealed() to run before the real terminal UI is mounted;
update the effect to wait for projectHydrated as well and only schedule the
reveal when both active and projectHydrated are true (so PersistentWorkSurface
has mounted TerminalsPage). Concretely, change the guard in the effect to if
(!active || !projectHydrated) return; add projectHydrated to the dependency
array, and keep the existing requestAnimationFrame and setTimeout cleanup logic
around dispatchWorkSurfaceRevealed to ensure the reveal runs once the terminal
surface can render.
In `@apps/desktop/src/renderer/components/app/TopBar.tsx`:
- Around line 83-93: The effect in TopBar.tsx reads
projectIconCache.get(rootPath) before checking disabled, causing stale cached
icons to be used for missing projects; update the useEffect so it first checks
the disabled flag and returns (calling setIcon(null) if disabled) before
attempting to read projectIconCache, and ensure you still clear failed via
setFailed(false) as before; modify the logic around
projectIconCache.get(rootPath), rootPath, disabled, setIcon, and setFailed to
enforce the disabled branch runs before using the cache.
In `@apps/desktop/src/renderer/components/chat/ChatAttachmentTray.tsx`:
- Around line 6-8: The attachmentName function currently only splits on "/" so
Windows-style paths like "C:\..." remain intact; update attachmentName(path:
string) to split on both forward and backward slashes (e.g. use a regex like
/[\\/]/ or equivalent) or use a platform-aware basename utility, then return the
last segment or the original path if empty, ensuring both "/" and "\" separators
are handled.
- Around line 214-219: The remove button in ChatAttachmentTray (the button that
calls onRemove(attachment.path) and uses title={`Remove ${attachment.path}`})
lacks an accessible name for screen readers; add an explicit accessible name
such as aria-label or aria-labelledby (for example set aria-label to `Remove
${attachment.path}` or reference a visually hidden span with that text) on that
button so screen readers announce its purpose, while keeping the existing title
and onClick behavior intact.
In `@apps/desktop/src/renderer/components/lanes/useLaneWorkSessions.ts`:
- Around line 449-461: The launch construction mixes defaultLaunch.command/args
into caller overrides; change to only apply defaultLaunch values when the caller
provided no launch overrides. In useLaneWorkSessions (the block that builds
defaultLaunch and calls window.ade.pty.create), compute a flag like
useDefaultExec = !args.startupCommand && !args.command && !args.args, then set
startupCommand to args.startupCommand ?? (useDefaultExec ?
defaultLaunch?.startupCommand : undefined), command to args.command ??
(useDefaultExec ? defaultLaunch?.command : undefined), and args to args.args ??
(useDefaultExec ? defaultLaunch?.args : undefined) so caller-supplied
startupCommand/command/args never get mixed with default CLI exec data.
In `@apps/desktop/src/renderer/components/terminals/useWorkSessions.ts`:
- Around line 1028-1040: The create call currently mixes a caller-supplied
startupCommand with defaultLaunch.command/args, causing the PTY to perform the
direct-launch argv first; change the logic so defaultLaunch.command and
defaultLaunch.args are only used when the caller did NOT provide a
startupCommand. In other words, keep startupCommand: args.startupCommand ??
defaultLaunch?.startupCommand ?? undefined, but for command and args only fall
back to defaultLaunch when args.startupCommand is undefined (e.g., if
args.startupCommand is present, do not inject
defaultLaunch.command/defaultLaunch.args). Update the call around
defaultLaunch/buildTrackedCliLaunchCommand and the command/args props passed to
window.ade.pty.create accordingly.
In `@apps/ios/ADE/Assets.xcassets/BrandMark.imageset/Contents.json`:
- Around line 4-14: The BrandMark image set currently only supplies a 1x asset
("filename": "logo.png") while the entries for "scale": "2x" and "scale": "3x"
are empty; add appropriately named retina image files (e.g., logo@2x.png and
logo@3x.png) and set their "filename" values in BrandMark.imageset/Contents.json
so the entries with "scale": "2x" and "scale": "3x" reference the correct files,
ensuring iOS will use the proper high-resolution assets on retina displays.
In `@apps/ios/ADE/Services/SyncService.swift`:
- Around line 5688-5696: The code falls back to session.startedAt when computing
lastActivity, which wrongly treats long-lived active sessions as stale; update
the fallback logic in the block that computes lastActivity (the use of
chatSummaryCache, Self.parseIso8601, session.chatIdleSinceAt and
session.startedAt) so that for active sessions (endedAt == nil) you do NOT use
startedAt as the recency signal—instead use the current time (Date()) or another
recent activity timestamp (e.g. a streaming/heartbeat time) as the final
fallback; make the identical change in the other occurrence referenced (the
block around the lines that also use Self.parseIso8601 and
session.chatIdleSinceAt) so runningAgents sees a recent lastActivity for live
sessions.
- Around line 5634-5644: The chatSummaryCache mutators cacheChatSummaries(_:)
and cacheChatSummary(_:) update cached summaries but never refresh the derived
session snapshot; call refreshActiveSessionsAndSnapshot() after updating the
cache in both functions so the LA-reconcile state (modelId, lastActivityAt,
focused lane) is recomputed immediately; ensure the call is made on the correct
actor/queue (e.g. main actor or same execution context as
refreshActiveSessionsAndSnapshot) to avoid threading issues.
In `@apps/ios/ADE/Views/Lanes/LaneAdvancedScreen.swift`:
- Around line 194-205: The computed property branchSwitchDisabledReason
currently allows a mission lane with laneRole == nil to fall through as allowed;
update the logic in branchSwitchDisabledReason to treat any mission lane
(missionId != nil) as disallowing branch switching by collapsing the two
mission-specific branches into a single check (e.g., if missionId != nil) that
returns the appropriate "Branch switching is disabled for mission lanes" message
while keeping the existing attached-lane check using laneType.
In `@apps/ios/ADE/Views/Lanes/LaneCommitSheet.swift`:
- Around line 175-188: The code currently uses fragile substring matching in
isAiSetupError(_:) and aiSetupHintFor(_:) against error.localizedDescription
from git.generateCommitMessage; change the protocol so
GitGenerateCommitMessageResult includes a stable optional error code/enum (or
have generateCommitMessage throw a typed error) and update the iOS call sites to
detect that code/enum instead of parsing text; remove or repurpose
isAiSetupError(_:) to switch on the new error code and have aiSetupHintFor(_:)
return messages based on that code, ensuring locale-independent,
non-false-positive detection of "AI commit messages not configured" conditions.
In `@apps/ios/ADE/Views/Lanes/LaneDetailContentSections.swift`:
- Around line 83-99: The Pull button currently always shows label/a11y "Pull"
but calls action: behind > 0 ? onPull : onFetch; update the UI so the visible
label, accessibility label, and symbol reflect the actual action: when behind ==
0 render the button as "Fetch" (use the Fetch-appropriate symbol and
non-emphasized tint) and wire its action to onFetch, otherwise render as "Pull"
wired to onPull; modify the syncActionButton invocation (the instance that
currently uses symbol: "arrow.down.to.line.compact", label: "Pull", and action:
behind > 0 ? onPull : onFetch) to choose label, symbol, accessibility label,
tint/emphasize consistently based on the behind > 0 condition so sighted and
VoiceOver users see the correct affordance.
In `@apps/ios/ADE/Views/Lanes/LaneDetailScreen.swift`:
- Around line 277-290: The footer currently always renders the thin Rectangle
divider when detail != nil even if both gitStatusBanner and commitCTAButton
would render nothing; change the footer to render the divider only when at least
one of those child views will actually be shown (i.e., only if
detail.lane.status.dirty == true || !(detail.diffChanges?.staged.isEmpty ??
true) || there are nonzero unstaged/staged/stashCount values that make
gitStatusBanner visible). Locate the footer closure and add a conditional around
the Rectangle (or gate the whole VStack) using the same predicates used by
gitStatusBanner and commitCTAButton so the divider is omitted for a clean lane
with no stash/staged/unstaged changes.
In `@apps/ios/ADETests/ADETests.swift`:
- Around line 386-401: The test currently uses a stale discovery timestamp;
update the DiscoveredSyncHost.lastResolvedAt value passed to
service.applyDiscoveredHostsForTesting(...) to a fresh/recent ISO8601 timestamp
(e.g. "2026-04-24T00:00:01.000Z" or generate CurrentDate) so the host is treated
as a fresh discovery and the call to
service.automaticReconnectAddressesForTesting(profile) exercises shortcut
matching; adjust only the lastResolvedAt field on the DiscoveredSyncHost in this
test.
In `@apps/ios/ADEWidgets/ADELiveActivityViews.swift`:
- Around line 151-156: The single-session compact label uses s.modelId?.isEmpty
to decide whether to show modelId or providerSlug but doesn't trim whitespace
like the other surfaces, causing payloads like " " to render blank; update
this branch in ADELiveActivityViews (the Text(...) that currently uses s.modelId
and s.providerSlug) to call a shared helper that trims whitespace and returns
the model display string or falls back to s.providerSlug.lowercased(), and reuse
that same helper across all Live Activity subtitle/label branches so trimming
logic is consistent (create a function such as modelDisplayName(for session:
Session) or trimAndFallbackModelId(_:) and replace the direct s.modelId checks
with it).
---
Outside diff comments:
In `@apps/ios/ADE/Services/LiveActivityCoordinator.swift`:
- Around line 235-288: makeContentState currently hardcodes awaitingInputCount
to 0 which prevents the awaiting-input attention state from appearing; compute
the real awaiting-input count (e.g., let awaitingInputCount = sessions.filter {
$0.awaitingInput }.count or from sorted/activeSessions as appropriate), pass
that variable into selectAttention(sessions:..., awaitingInputCount:
awaitingInputCount, ...) and set awaitingInputCount: awaitingInputCount in the
returned ADESessionAttributes.ContentState so the Live Activity can show the
“Tap to respond” state.
---
Nitpick comments:
In `@apps/desktop/src/main/services/sync/syncRemoteCommandService.test.ts`:
- Around line 431-439: The mock in createMockConflictService declares
dismissRebase and deferRebase as vi.fn() (sync) while other methods use
mockResolvedValue; update dismissRebase and deferRebase to be async mocks (e.g.,
vi.fn().mockResolvedValue(undefined) or mockResolvedValue(null)) so their
semantics match getLaneStatus/listOverlaps/getBatchAssessment and won't hide
await-related issues in callers that await these methods.
In `@apps/desktop/src/shared/types/sync.ts`:
- Line 227: The project_summary field iconDataUrl can cause unbounded payload
growth; update the producer in main.ts that builds
project_catalog/project_catalog_chunk to (a) normalize/resize icons before
base64-encoding (e.g., downscale to a fixed small favicon size and convert to an
efficient format), (b) enforce a hard byte-size cap per icon and skip/replace
oversized icons with a placeholder, and (c) if icons may still be large, switch
to serving icons on-demand via a separate handler/command (e.g.,
icon-by-projectId) and send only a lightweight icon reference in the project
summary instead of the full iconDataUrl. Ensure the code paths that populate
iconDataUrl perform these checks and fallbacks to prevent large fan-out
payloads.
In `@apps/ios/ADE/App/ContentView.swift`:
- Around line 129-145: The attachedComputerLabel getter currently force-unwraps
trimmedHost when creating host; change that to use a single optional binding so
we never use "!" — e.g. compute trimmedHost with
syncService.hostName?.trimmingCharacters(...), then set host via if let host =
trimmedHost, !host.isEmpty (or use guard) so all subsequent switch cases use
that non‑force-unwrapped host optional; update references to the local host
variable in the switch (in the .connected/.syncing, .connecting, .error cases)
to use the safely bound host.
- Around line 367-382: projectHomeIconImageCache currently stores the full
dataUrl string as the cache key and has no limits, causing memory bloat; update
projectHomeIconImage(from:) to derive a compact key (e.g., SHA256 or other short
digest of dataUrl) instead of using dataUrl as NSString, set sensible limits on
projectHomeIconImageCache (countLimit and/or totalCostLimit) and use the
image/data size as the cache cost when calling setObject(_:forKey:cost:). Also
move expensive work off the main thread by decoding the base64/Data -> UIImage
on a background queue (or use an async loader) and then cache the result and
deliver it back to the main thread for UI update.
In `@apps/ios/ADE/Views/Lanes/LaneDetailScreen.swift`:
- Around line 243-251: The sheet uses isPresented: $showBranchPicker with
conditional content if let detail which can leave a blank sheet when detail
becomes nil; change to using sheet(item:) (or introduce a BranchPickerContext
struct) so the sheet is keyed to the detail value and will dismiss automatically
when the item becomes nil. Update the UI in LaneDetailScreen to present
LaneBranchPickerSheet via .sheet(item: $selectedBranchPickerItem) (or
.sheet(item: $detail) if appropriate), move the branchRef and laneId into that
item/context, and have onComplete call loadDetail(refreshRemote: true) and then
clear the item to dismiss the sheet; remove the isPresented usage and ensure
showBranchPicker is replaced by the item binding.
In `@apps/ios/ADETests/ADETests.swift`:
- Around line 5-13: The helper recentIso8601Fixture() currently returns a new
ISO string each call (using Date()), causing fixtures in the same test to have
different "now" timestamps; change it to compute the ISO string once per test
run and return that cached value on subsequent calls (e.g., initialize a
static/let frozenIso8601 value using ISO8601DateFormatter and Date() once, and
have recentIso8601Fixture() return that frozenIso8601) so all fixtures share a
stable recent timestamp; apply the same pattern to any other fixture helpers
that call Date() (the comment notes similar instances).
🪄 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: 805d478c-09ad-4f97-8dba-e2aa35644fea
⛔ Files ignored due to path filters (14)
.claude/scheduled_tasks.lockis excluded by!**/*.lockapps/ade-cli/package-lock.jsonis excluded by!**/package-lock.json,!**/package-lock.jsonapps/ios/ADE.xcodeproj/project.pbxprojis excluded by!**/*.xcodeproj/project.pbxprojapps/ios/ADE/Assets.xcassets/BrandMark.imageset/logo.pngis excluded by!**/*.pngdocs/ARCHITECTURE.mdis excluded by!docs/**docs/features/chat/composer-and-ui.mdis excluded by!docs/**docs/features/lanes/README.mdis excluded by!docs/**docs/features/project-home/README.mdis excluded by!docs/**docs/features/pull-requests/README.mdis excluded by!docs/**docs/features/sync-and-multi-device/ios-companion.mdis excluded by!docs/**docs/features/sync-and-multi-device/remote-commands.mdis excluded by!docs/**docs/features/terminals-and-sessions/README.mdis excluded by!docs/**docs/features/terminals-and-sessions/pty-and-processes.mdis excluded by!docs/**docs/features/terminals-and-sessions/ui-surfaces.mdis excluded by!docs/**
📒 Files selected for processing (53)
.gitignoreapps/desktop/src/main/main.tsapps/desktop/src/main/services/ipc/registerIpc.tsapps/desktop/src/main/services/projects/projectIconResolver.test.tsapps/desktop/src/main/services/projects/projectIconResolver.tsapps/desktop/src/main/services/pty/ptyService.test.tsapps/desktop/src/main/services/pty/ptyService.tsapps/desktop/src/main/services/sync/syncRemoteCommandService.test.tsapps/desktop/src/main/services/sync/syncRemoteCommandService.tsapps/desktop/src/preload/global.d.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/browserMock.tsapps/desktop/src/renderer/components/app/App.tsxapps/desktop/src/renderer/components/app/App.workKeepAlive.test.tsxapps/desktop/src/renderer/components/app/TopBar.test.tsxapps/desktop/src/renderer/components/app/TopBar.tsxapps/desktop/src/renderer/components/chat/ChatAttachmentTray.test.tsxapps/desktop/src/renderer/components/chat/ChatAttachmentTray.tsxapps/desktop/src/renderer/components/lanes/useLaneWorkSessions.tsapps/desktop/src/renderer/components/terminals/TerminalView.test.tsxapps/desktop/src/renderer/components/terminals/TerminalView.tsxapps/desktop/src/renderer/components/terminals/WorkStartSurface.tsxapps/desktop/src/renderer/components/terminals/WorkViewArea.tsxapps/desktop/src/renderer/components/terminals/cliLaunch.test.tsapps/desktop/src/renderer/components/terminals/cliLaunch.tsapps/desktop/src/renderer/components/terminals/useWorkSessions.tsapps/desktop/src/renderer/components/terminals/workSurfaceVisibility.tsapps/desktop/src/shared/ipc.tsapps/desktop/src/shared/types/core.tsapps/desktop/src/shared/types/sync.tsapps/ios/ADE/App/ContentView.swiftapps/ios/ADE/Assets.xcassets/BrandMark.imageset/Contents.jsonapps/ios/ADE/Models/RemoteModels.swiftapps/ios/ADE/Services/LiveActivityCoordinator.swiftapps/ios/ADE/Services/SyncService.swiftapps/ios/ADE/Shared/ADESharedModels.swiftapps/ios/ADE/Views/Lanes/LaneAdvancedScreen.swiftapps/ios/ADE/Views/Lanes/LaneCommitSheet.swiftapps/ios/ADE/Views/Lanes/LaneDetailContentSections.swiftapps/ios/ADE/Views/Lanes/LaneDetailGitSection.swiftapps/ios/ADE/Views/Lanes/LaneDetailScreen.swiftapps/ios/ADE/Views/Work/WorkArtifactTerminalViews.swiftapps/ios/ADE/Views/Work/WorkBrowserHelpers.swiftapps/ios/ADE/Views/Work/WorkModels.swiftapps/ios/ADE/Views/Work/WorkNavigationAndTranscriptHelpers.swiftapps/ios/ADE/Views/Work/WorkRootComponents.swiftapps/ios/ADE/Views/Work/WorkRootScreen+Actions.swiftapps/ios/ADE/Views/Work/WorkRootScreen.swiftapps/ios/ADE/Views/Work/WorkSessionGrouping.swiftapps/ios/ADE/Views/Work/WorkStatusAndFormattingHelpers.swiftapps/ios/ADETests/ADETests.swiftapps/ios/ADEWidgets/ADELiveActivity.swiftapps/ios/ADEWidgets/ADELiveActivityViews.swift
💤 Files with no reviewable changes (4)
- apps/ios/ADE/Views/Work/WorkModels.swift
- apps/ios/ADE/Views/Work/WorkBrowserHelpers.swift
- apps/ios/ADE/Views/Work/WorkRootComponents.swift
- apps/ios/ADE/Views/Work/WorkSessionGrouping.swift
| private func isAiSetupError(_ text: String) -> Bool { | ||
| let lower = text.lowercased() | ||
| return lower.contains("ai commit messages are off") | ||
| || lower.contains("commit messages model") | ||
| || lower.contains("choose a commit messages") | ||
| || lower.contains("not currently available") | ||
| } | ||
|
|
||
| private func aiSetupHintFor(_ text: String) -> String { | ||
| if text.lowercased().contains("are off") { | ||
| return "AI commit messages are turned off on the desktop. Open desktop Settings → AI → Commit Messages to enable it." | ||
| } | ||
| return "Pick a Commit Messages model on the desktop in Settings → AI → Commit Messages." | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find where the desktop emits these AI commit-message errors so we can see if a
# structured error code/type exists that the iOS side could key off instead.
rg -nP -C3 "ai commit messages are off|commit messages model|choose a commit messages|not currently available"
rg -nP -C3 'generateCommitMessage' --type=swiftRepository: arul28/ADE
Length of output: 3170
🏁 Script executed:
# Find all error messages related to "ai commit messages" on desktop
rg -n "ai commit messages are off|choose a commit messages" -iRepository: arul28/ADE
Length of output: 1592
🏁 Script executed:
# Find the GitGenerateCommitMessageResult type definition
rg -n "GitGenerateCommitMessageResult" -A 10 --type=swiftRepository: arul28/ADE
Length of output: 1761
🏁 Script executed:
# Look at how isAiSetupError is called in context to understand error handling
rg -n "isAiSetupError" -B 5 -A 5 --type=swiftRepository: arul28/ADE
Length of output: 1718
🏁 Script executed:
# Check if there are any error code enums or error types related to AI/commit messages
rg -n "enum.*Error|struct.*Error" --type=swift | head -30Repository: arul28/ADE
Length of output: 610
Add a structured error code or type to git.generateCommitMessage response instead of matching error message substrings.
The current implementation relies on fragile substring matching of error.localizedDescription ("not currently available", "commit messages model", etc.). This breaks if the desktop localizes or changes error messages. Additionally, transient backend errors containing "not currently available" will incorrectly trigger setup hints, blocking the suggest button.
Instead, extend GitGenerateCommitMessageResult with an optional error code field, or throw a custom typed error from the desktop (generateCommitMessage). The iOS side can then check the code/type directly rather than parsing English strings, making the setup detection locale-independent and eliminating false positives.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ios/ADE/Views/Lanes/LaneCommitSheet.swift` around lines 175 - 188, The
code currently uses fragile substring matching in isAiSetupError(_:) and
aiSetupHintFor(_:) against error.localizedDescription from
git.generateCommitMessage; change the protocol so GitGenerateCommitMessageResult
includes a stable optional error code/enum (or have generateCommitMessage throw
a typed error) and update the iOS call sites to detect that code/enum instead of
parsing text; remove or repurpose isAiSetupError(_:) to switch on the new error
code and have aiSetupHintFor(_:) return messages based on that code, ensuring
locale-independent, non-false-positive detection of "AI commit messages not
configured" conditions.
…y + copilot) Greptile P2: - registerIpc: validate projectResolveIcon rootPath via resolveAllowedRendererPath - TopBar: bound projectIconCache with LRU(24); honor disabled before cache lookup - ChatAttachmentTray: focus-trap lightbox (focus close on mount, Tab cycle, restore on unmount, Escape to close) CodeRabbit: - registerIpc: return realpath from resolveAllowedRendererPath (TOCTOU defense); switch image data URL handler to async fs.promises with size pre-check - ptyService: restore Codex storage fallback for session-list hydration (regression flagged); only resume-launch keeps the live-capture exclusion - App.tsx: gate PersistentWorkSurface reveal on project-hydrated state, not just `active` - TopBar: don't reuse cached icons for newly-disabled projects - ChatAttachmentTray: split attachmentName on /\\ for Windows paths; aria-label on file remove button - browserMock: add project.resolveIcon mock - cliLaunch/useLaneWorkSessions/useWorkSessions: extract resolveLaunchFields() so caller overrides are atomic — defaults only fire when no field supplied - SyncService: refresh active-session snapshot when chatSummary cache mutates; fall back to `now` for running sessions with no activity timestamp - LaneAdvancedScreen: deny branch-switch for any mission lane (incl. nil role) - LaneCommitSheet: TODO + broader needles for AI-setup error detection (proper structured error code is a follow-up) - LaneDetailContentSections: Pull/Fetch label + a11y reflect actual action - LaneDetailScreen: drop divider when footer renders no content - ADELiveActivityViews: trim modelId before isEmpty check - ADETests: shortcut-vs-saved-host test uses fresh discovery timestamp capy-ai (High Bug): - SyncService: long-lived running sessions no longer fall through the recency cutoff via stale `started` fallback Copilot (selected, high-impact): - ptyService: shrink readFilePrefix scan to 16 KB when scanning for required marker (session meta is at the top) - LaneCommitSheet: restore .onAppear focus on commit message field Skipped: BrandMark retina assets (no source); Copilot ANSI-renderer limitations (known/experimental, not regressions); Copilot design nits. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@copilot review but do not make fixes |
Here's my review of the latest iteration (commit What was addressedGreptile P2s — all resolved:
From prior copilot review — resolved:
Remaining issue: Greptile P1 —
|
- shell.ts: ANSI-C quote ($'...') args with embedded newlines/tabs/etc so the PTY shell-fallback doesn't fire PS2 continuation prompts mid-startup-command. Adds tests covering the codex preamble case. - ChatAttachmentTray: lock body scroll while ImageLightbox is open so the wheel doesn't move content behind the overlay; restore on close. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@copilot review but do not make fixes |
Here's my review of the latest iteration (commit What was addressed since last roundGreptile P1 — Copilot minor — Greptile P2 observations (new, from Greptile's second pass)1.
|
There was a problem hiding this comment.
Actionable comments posted: 4
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/Services/SyncService.swift (1)
5702-5736:⚠️ Potential issue | 🟠 MajorRunning sessions can still age out once the cached summary stops refreshing.
lastActivitynow preferschatSummaryCache[session.id].lastActivityAt, but that cache is only updated by the two mutators above. The livechat_eventpaths in this file do not advance the cache or recompute snapshots, so a long-running stream with a warmed-but-stale summary will still fall out ofrunningAgentsonce that cached timestamp gets older thanrunningRecencyCutoff, even while output is arriving. Downstream, the Live Activity roster disappears,focusedLaneNameclears, and the elapsed timer stalls until another summary fetch happens.💡 One way to keep active runs from aging out on stale cache data
- let lastActivity = - Self.parseIso8601(summary?.lastActivityAt ?? "") - ?? Self.parseIso8601(session.chatIdleSinceAt ?? "") - ?? (isRunningRuntime ? now : started) + let cachedLastActivity = + Self.parseIso8601(summary?.lastActivityAt ?? "") + ?? Self.parseIso8601(session.chatIdleSinceAt ?? "") + let lastActivity: Date + if isRunningRuntime { + if let cachedLastActivity, cachedLastActivity >= runningRecencyCutoff { + lastActivity = cachedLastActivity + } else { + lastActivity = now + } + } else { + lastActivity = cachedLastActivity ?? started + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ios/ADE/Services/SyncService.swift` around lines 5702 - 5736, The lastActivity calculation uses chatSummaryCache[session.id].lastActivityAt which can be stale; update it to consider live output activity (e.g., a session-level live timestamp updated by chat_event handlers) and/or use now while streaming so running sessions don't age out. Concretely: change the lastActivity logic in the AgentSnapshot construction (the lastActivity variable using Self.parseIso8601) to take the most recent of summary?.lastActivityAt, session.chatIdleSinceAt, a live-session timestamp such as session.lastOutputAt (or a new sessionLiveLastActivity you update from chat_event handlers), and finally now/started when appropriate; also ensure chat_event paths update that session live timestamp and recompute/append the snapshot (used by runningAgents and runningRecencyCutoff) so the live roster and elapsed timer remain current.
♻️ Duplicate comments (1)
apps/ios/ADEWidgets/ADELiveActivityViews.swift (1)
756-758:⚠️ Potential issue | 🟡 Minor
ExpandedRosterStripstill doesn't trimmodelId— and the four trimmed surfaces drifted onCharacterSet.This roster row is the only updated surface that did not adopt the trim-before-empty-check fix from the past review. A payload like
" "or"\n"will render as a visually-blank chip here while every other surface correctly falls back toproviderSlug.lowercased().Separately, the four trimmed call sites are now inconsistent:
- Line 158 (
WorkspaceCompactTrailing):.whitespacesAndNewlines- Lines 424, 613, 785 (
focusedSubtitle,LockRosterRow.subtitleText,FocusedCardBottom.subtitle):.whitespaces(does not include\n/\r)So a
modelIdof"\n"would fall back on the compact chip but render as a blank line on the expanded center, lock-screen roster, and focused-card subtitle — exactly the kind of drift the past review's "single shared helper" suggestion was meant to prevent.Recommend extracting one helper on
ActiveSession(or as a free function) and routing all five sites through it.Proposed fix
+@available(iOS 16.2, *) +extension ADESessionAttributes.ContentState.ActiveSession { + /// Display label for subtitles/chips: trimmed `modelId` when present and + /// non-blank, otherwise the lowercased provider slug. Trims newlines as + /// well as spaces so payloads like "\n" don't render an invisible chip. + var modelDisplayLabel: String { + let trimmed = (modelId ?? "").trimmingCharacters(in: .whitespacesAndNewlines) + return trimmed.isEmpty ? providerSlug.lowercased() : trimmed + } +}Then at lines 756-758:
- Text(session.modelId?.isEmpty == false - ? session.modelId! - : session.providerSlug.lowercased()) + Text(session.modelDisplayLabel)And apply the same
session.modelDisplayLabel/focused.modelDisplayLabelsubstitution at lines 157-160, 424-426, 613-615, and 785-787.As per coding guidelines: "iOS Swift app — check for memory management, Swift conventions, and proper SwiftUI patterns." Centralising this logic also removes the inline IIFE at lines 157-160, which is unidiomatic Swift/SwiftUI in a
Text(...)initializer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ios/ADEWidgets/ADELiveActivityViews.swift` around lines 756 - 758, ExpandedRosterStrip still uses session.modelId without trimming, causing whitespace-only values to render as blank; create a single helper (e.g., add a computed var modelDisplayLabel on ActiveSession or a free function) that returns session.modelId trimmed with .whitespacesAndNewlines and falls back to providerSlug.lowercased() when empty, then replace the direct uses in ExpandedRosterStrip (Text(...) using session.modelId), WorkspaceCompactTrailing, focusedSubtitle, LockRosterRow.subtitleText, and FocusedCardBottom.subtitle to read session.modelDisplayLabel (or focused.modelDisplayLabel) so all five sites use the same whitespace-and-newline trimming behavior and remove any inline IIFE usage.
🧹 Nitpick comments (4)
apps/ios/ADE/Views/Lanes/LaneAdvancedScreen.swift (1)
7-10: UnusedsnapshotandlaneIdproperties.
snapshot: LaneListSnapshotandlaneId: Stringare accepted from the caller (LaneDetailGitSection.advancedRow) but never referenced anywhere in this view's body, computed properties, or row handlers — the action closures already encapsulatelaneId(e.g.,syncService.stashPush(laneId: laneId, ...)). Carrying them adds dead surface area and forces unrelated callers to keep producing them. Consider dropping both unless you plan to render snapshot-derived state (e.g., dirty state, current branch from snapshot) directly here.♻️ Proposed simplification
struct LaneAdvancedScreen: View { - let snapshot: LaneListSnapshot let canRunLiveActions: Bool let disabledSubtitle: String? - let laneId: String let branchRef: String? let laneType: String? let missionId: String? let laneRole: String?And update the call site in
LaneDetailGitSection.swiftto dropsnapshot:andlaneId:accordingly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ios/ADE/Views/Lanes/LaneAdvancedScreen.swift` around lines 7 - 10, LaneAdvancedScreen declares unused properties snapshot: LaneListSnapshot and laneId: String; remove these stored properties and any initializer parameters tied to them from the LaneAdvancedScreen type, and update the caller in LaneDetailGitSection to stop passing snapshot: and laneId:. Ensure the action closures still capture the laneId where needed (they already do, e.g., syncService.stashPush(laneId: laneId, ...)), and if you later need snapshot-derived UI state add only the specific fields you render instead of the whole snapshot.apps/desktop/src/renderer/components/chat/ChatAttachmentTray.tsx (2)
80-85: Deadfocus:styles on a non-focusable wrapper.The wrapping
<div>has notabIndex, sofocus:outline-none focus:ring-1 focus:ring-white/25will never apply. If the intent was to highlight the tile while the inner image button is keyboard-focused, switch tofocus-within:(or move these classes to the inner<button>).♻️ Suggested adjustment
<div className={cn( - "group/image relative h-14 w-14 shrink-0 overflow-hidden rounded-md border p-0 text-left transition-colors focus:outline-none focus:ring-1 focus:ring-white/25", + "group/image relative h-14 w-14 shrink-0 overflow-hidden rounded-md border p-0 text-left transition-colors focus-within:outline-none focus-within:ring-1 focus-within:ring-white/25", toneClassName, )} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/chat/ChatAttachmentTray.tsx` around lines 80 - 85, The wrapper div in ChatAttachmentTray.tsx uses non-functional focus: classes (e.g., "focus:outline-none focus:ring-1 focus:ring-white/25") because it has no tabIndex; update the styling so keyboard focus works: either change the wrapper's focus: prefixed classes to focus-within: (so the tile highlights when the inner interactive element is focused) or move those focus classes onto the inner <button> element; reference the wrapper div with the class string including toneClassName and the inner button element in ChatAttachmentTray.tsx to apply the fix.
86-102: Image button is a silent no-op whendataUrlis null.When the image is still loading or
previewFailedistrue, the button is fully focusable and clickable butonClickonly opens the lightboxif (dataUrl). For the failed-preview case in particular, sighted and AT users encounter a button that looks/announces like "Open " yet does nothing. Either disable the button (or render it as a non-interactive container) when there's nodataUrl, or surface a distinct affordance for the failed state.♻️ Suggested adjustment
<button type="button" className="block h-full w-full p-0" - title={`Open ${name}`} - aria-label={`Open ${name}`} - onClick={() => { - if (dataUrl) setExpanded(true); - }} + title={dataUrl ? `Open ${name}` : name} + aria-label={dataUrl ? `Open ${name}` : name} + disabled={!dataUrl} + onClick={() => { + if (dataUrl) setExpanded(true); + }} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/chat/ChatAttachmentTray.tsx` around lines 86 - 102, The image button in ChatAttachmentTray is interactive even when dataUrl is null, causing a misleading "Open <name>" control; update the rendering so the button is non-interactive when no preview is available (dataUrl is falsy or previewFailed): either render a non-button container (span/div) in place of the <button> or set the button to disabled and add aria-disabled plus an appropriate title/aria-label (e.g., "Preview unavailable" or "Loading…") and ensure onClick only triggers setExpanded when dataUrl exists; update references to the button element and setExpanded in ChatAttachmentTray to reflect this change.apps/desktop/src/main/services/pty/ptyService.ts (1)
776-782: The 16KB prefix is adequate for current scenarios but the comment is misleading about coverage.The
"ADE session guidance"marker is logged as auser_messageJSONL line that comes aftersession_meta, not inside it. The comment at lines 777–779 ("session_meta sits at the very top, so 16 KB is more than enough") only holds if the marker were withinsession_meta— in reality, the 16KB window must span both the fullsession_metaline (which can includebase_instructionsup to several KB, as shown by the test case with 5000-char padding) plus the firstuser_messageline containing the marker.The test case "backfills a missing Codex resume target from storage when session_meta exceeds 1 KB" (line 1182) uses
base_instructions.text: "x".repeat(5000), demonstrating thatsession_metaalone can exceed 5 KB. With current Codex behavior, 16KB comfortably covers both lines, but ifbase_instructionsgrows beyond the test threshold, the window becomes fragile. The comment should clarify that 16KB covers early user-messages followingsession_meta, not justsession_metaitself.Non-blocking: consider updating the comment to reflect the actual structure (or bumping to 32–64 KB if defensive headroom is preferred).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/pty/ptyService.ts` around lines 776 - 782, The comment above the readFilePrefix call (in the args.requiredText check that uses readFilePrefix(candidate.filePath, 16 * 1024)) is misleading: the ADE marker appears in a subsequent user_message line after session_meta, so the 16KB window must cover the full session_meta line plus the first user_message, not just session_meta itself; update the comment to state this explicitly and either leave 16KB with that clarified rationale or, if you want extra defensive headroom, increase the buffer to 32*1024 or 64*1024 by changing the readFilePrefix size constant.
🤖 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/ipc/registerIpc.ts`:
- Around line 1922-1944: The handler currently uses inferImageMimeType(filePath)
which guesses type from extension and falls back to "image/png", allowing
non-image files to be returned as base64; update appGetImageDataUrl to strictly
validate file content before encoding: read the file header (or use a binary
mime detector/file-type library) to derive the actual MIME and ensure it matches
/^image\// and the file size is within the existing 10MB limit and inside the
approved roots; if validation fails, throw or return a clear error and do not
return base64. Reference inferImageMimeType and appGetImageDataUrl to locate
where to replace the ext-based guess with content-based detection and the
additional image-only check.
- Around line 2015-2022: The appWriteClipboardImage IPC handler currently calls
nativeImage.createFromPath synchronously without validating the path or file
size; update the IPC.appWriteClipboardImage handler to first resolve the path
with resolveAllowedRendererPath, then use fs.promises.stat to verify the target
exists and stat.isFile() and enforce the same 10 MB size cap (e.g. 10 * 1024 *
1024) before attempting to decode/write the image; if the file is missing, not a
file, or exceeds the size cap, throw a clear error and only then proceed to
create the nativeImage (or read the file) and call clipboard.writeImage(image).
In `@apps/desktop/src/renderer/components/app/App.tsx`:
- Around line 216-224: The effect watching only [active] can miss when the DOM
node mounts later (workSurfaceRef.current is null), so remove the imperative
useEffect and instead apply inert declaratively on the work surface div (the
element with ref workSurfaceRef) alongside the existing aria-hidden logic: set
inert={active ? undefined : ""} (or add/remove the attribute based on the active
boolean) and keep aria-hidden in sync; alternatively, if you prefer the effect
approach, expand its dependency array to [active, projectHydrated,
hasActiveProject, showWelcome] so it re-runs when the gating conditions change.
In `@apps/desktop/src/renderer/components/chat/ChatAttachmentTray.tsx`:
- Around line 295-305: The remove-button shows the full attachment.path in its
title but a basename via aria-label (attachmentName(attachment.path)), causing
inconsistent tooltips/announcements; update the button to use the same basename
for both by changing the title to use attachmentName(attachment.path) (the
onRemove handler and aria-label can remain as-is), ensuring the X button’s
tooltip and screen-reader label match the image variant behavior.
---
Outside diff comments:
In `@apps/ios/ADE/Services/SyncService.swift`:
- Around line 5702-5736: The lastActivity calculation uses
chatSummaryCache[session.id].lastActivityAt which can be stale; update it to
consider live output activity (e.g., a session-level live timestamp updated by
chat_event handlers) and/or use now while streaming so running sessions don't
age out. Concretely: change the lastActivity logic in the AgentSnapshot
construction (the lastActivity variable using Self.parseIso8601) to take the
most recent of summary?.lastActivityAt, session.chatIdleSinceAt, a live-session
timestamp such as session.lastOutputAt (or a new sessionLiveLastActivity you
update from chat_event handlers), and finally now/started when appropriate; also
ensure chat_event paths update that session live timestamp and recompute/append
the snapshot (used by runningAgents and runningRecencyCutoff) so the live roster
and elapsed timer remain current.
---
Duplicate comments:
In `@apps/ios/ADEWidgets/ADELiveActivityViews.swift`:
- Around line 756-758: ExpandedRosterStrip still uses session.modelId without
trimming, causing whitespace-only values to render as blank; create a single
helper (e.g., add a computed var modelDisplayLabel on ActiveSession or a free
function) that returns session.modelId trimmed with .whitespacesAndNewlines and
falls back to providerSlug.lowercased() when empty, then replace the direct uses
in ExpandedRosterStrip (Text(...) using session.modelId),
WorkspaceCompactTrailing, focusedSubtitle, LockRosterRow.subtitleText, and
FocusedCardBottom.subtitle to read session.modelDisplayLabel (or
focused.modelDisplayLabel) so all five sites use the same whitespace-and-newline
trimming behavior and remove any inline IIFE usage.
---
Nitpick comments:
In `@apps/desktop/src/main/services/pty/ptyService.ts`:
- Around line 776-782: The comment above the readFilePrefix call (in the
args.requiredText check that uses readFilePrefix(candidate.filePath, 16 * 1024))
is misleading: the ADE marker appears in a subsequent user_message line after
session_meta, so the 16KB window must cover the full session_meta line plus the
first user_message, not just session_meta itself; update the comment to state
this explicitly and either leave 16KB with that clarified rationale or, if you
want extra defensive headroom, increase the buffer to 32*1024 or 64*1024 by
changing the readFilePrefix size constant.
In `@apps/desktop/src/renderer/components/chat/ChatAttachmentTray.tsx`:
- Around line 80-85: The wrapper div in ChatAttachmentTray.tsx uses
non-functional focus: classes (e.g., "focus:outline-none focus:ring-1
focus:ring-white/25") because it has no tabIndex; update the styling so keyboard
focus works: either change the wrapper's focus: prefixed classes to
focus-within: (so the tile highlights when the inner interactive element is
focused) or move those focus classes onto the inner <button> element; reference
the wrapper div with the class string including toneClassName and the inner
button element in ChatAttachmentTray.tsx to apply the fix.
- Around line 86-102: The image button in ChatAttachmentTray is interactive even
when dataUrl is null, causing a misleading "Open <name>" control; update the
rendering so the button is non-interactive when no preview is available (dataUrl
is falsy or previewFailed): either render a non-button container (span/div) in
place of the <button> or set the button to disabled and add aria-disabled plus
an appropriate title/aria-label (e.g., "Preview unavailable" or "Loading…") and
ensure onClick only triggers setExpanded when dataUrl exists; update references
to the button element and setExpanded in ChatAttachmentTray to reflect this
change.
In `@apps/ios/ADE/Views/Lanes/LaneAdvancedScreen.swift`:
- Around line 7-10: LaneAdvancedScreen declares unused properties snapshot:
LaneListSnapshot and laneId: String; remove these stored properties and any
initializer parameters tied to them from the LaneAdvancedScreen type, and update
the caller in LaneDetailGitSection to stop passing snapshot: and laneId:. Ensure
the action closures still capture the laneId where needed (they already do,
e.g., syncService.stashPush(laneId: laneId, ...)), and if you later need
snapshot-derived UI state add only the specific fields you render instead of the
whole snapshot.
🪄 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: b0900681-a52e-4cad-94b9-1e4d535ee000
📒 Files selected for processing (19)
apps/desktop/src/main/services/ipc/registerIpc.tsapps/desktop/src/main/services/pty/ptyService.test.tsapps/desktop/src/main/services/pty/ptyService.tsapps/desktop/src/renderer/browserMock.tsapps/desktop/src/renderer/components/app/App.tsxapps/desktop/src/renderer/components/app/TopBar.tsxapps/desktop/src/renderer/components/chat/ChatAttachmentTray.tsxapps/desktop/src/renderer/components/lanes/useLaneWorkSessions.tsapps/desktop/src/renderer/components/terminals/cliLaunch.tsapps/desktop/src/renderer/components/terminals/useWorkSessions.tsapps/desktop/src/renderer/lib/shell.test.tsapps/desktop/src/renderer/lib/shell.tsapps/ios/ADE/Services/SyncService.swiftapps/ios/ADE/Views/Lanes/LaneAdvancedScreen.swiftapps/ios/ADE/Views/Lanes/LaneCommitSheet.swiftapps/ios/ADE/Views/Lanes/LaneDetailContentSections.swiftapps/ios/ADE/Views/Lanes/LaneDetailScreen.swiftapps/ios/ADETests/ADETests.swiftapps/ios/ADEWidgets/ADELiveActivityViews.swift
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/desktop/src/renderer/components/lanes/useLaneWorkSessions.ts
- apps/desktop/src/renderer/components/terminals/useWorkSessions.ts
- apps/desktop/src/renderer/components/terminals/cliLaunch.ts
- apps/desktop/src/renderer/components/app/TopBar.tsx
Greptile P1 / security: - projectIconResolver: use realpath-based resolvePathWithinRoot for both the source-file scan and the candidate-icon stat path so a `public -> /etc` symlink in a checked-out repo can't escape the project root. Defensive try/catch on stat/read returns "no icon" rather than crashing. - Added a regression test that symlinks `<root>/public` to an outside dir and asserts no icon is returned. CodeRabbit Critical: - registerIpc.appGetImageDataUrl no longer derives MIME from the path extension. Magic-byte sniffer (PNG, JPEG, GIF87/89, WebP, BMP, SVG with optional <?xml prelude / BOM / leading whitespace) plus stat preflight is the only path now; non-images throw "Path is not an image." CodeRabbit Major: - registerIpc.appWriteClipboardImage now runs the same readImageFileAndSniffMime preflight before nativeImage.createFromPath, so the renderer can't make the main process sync-read an arbitrarily large or non-image file. - App.tsx PersistentWorkSurface inert effect deps extended to [active, projectHydrated, hasActiveProject, showWelcome] so the effect re-runs once the early-return guards settle and the workSurfaceRef is a real node (fix for first cold /work mount). CodeRabbit Minor: - ChatAttachmentTray chip remove button: title now uses attachmentName(...) to match aria-label, so sighted hover and AT both see the basename. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@copilot review but do not make fixes |
Here's my review of commit What was addressed since last roundGreptile P1 — symlink traversal in CodeRabbit Critical — CodeRabbit Major — CodeRabbit (App.tsx) — ChatAttachmentTray chip remove button label: New observations1. ICO files silently fail through
|
- sniffImageMimeType: recognize ICO (00 00 01 00) so .ico chat attachments render their preview instead of failing through to "Path is not an image." - appWriteClipboardImage: hand the already-read buffer from readImageFileAndSniffMime to nativeImage.createFromBuffer so the file isn't re-read off the main thread (avoids a 10 MB sync read pair for the largest allowed images). Skipped: resolveLaunchFields permission-mode injection — verified via git history that this was a deliberate part of the lane's main feature commit (9d35235), not a review-loop regression. LiveActivityCoordinator focused-lane flicker — pre-existing behavior, out of scope. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@copilot review but do not make fixes |
…flake The "does not enter a run body while another health sweep owns it" test polls reconcileCalls in a 200 * 25ms = 5s window before asserting it reached the gated onTrackedSessionEnded callback. That window has flaked twice on the test-desktop(8) shard for this PR's pushes (and once on the shard rerun after the same failure) while passing 3/3 locally — the gated callback simply doesn't fire within 5s on a heavily loaded runner. Bump the wait window to 600 * 25ms = 15s and raise the test timeout from 15s to 30s. Test exits immediately when the callback fires, so the larger ceiling only kicks in for slow runners. No semantic change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@copilot review but do not make fixes |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
apps/desktop/src/renderer/components/app/App.tsx (1)
202-214: Double-dispatch causes the TerminalView reveal handler to run twice on the common path.
dispatchWorkSurfaceRevealed()is fired both from the RAF and the 120 ms backup timer. Inapps/desktop/src/renderer/components/terminals/TerminalView.tsx(lines 1120–1135), the listener does non-trivial work on every event —clearTextureAtlas(runtime),flushPendingFrameWrites(runtime), a fullterm.refresh(0, rows-1), focus, andscrollToBottom. When the terminal is already mounted at RAF time (the typical case), the 120 ms dispatch repeats all of that for no benefit.The 120 ms fallback is only useful when the listener attaches after the RAF dispatch. Consider gating the second dispatch so it only runs if the first one was issued before the listener could plausibly be attached, e.g. let the consumer mark itself as "first reveal handled" via a no-op flag, or coalesce by skipping the timer when the RAF callback has already executed:
♻️ Optional: skip the backup dispatch once RAF has fired
React.useEffect(() => { if (!shouldReveal) return; + let rafFired = false; const raf = window.requestAnimationFrame(() => { + rafFired = true; dispatchWorkSurfaceRevealed(); }); const settleTimer = window.setTimeout(() => { - dispatchWorkSurfaceRevealed(); + // Only re-dispatch if RAF hasn't run yet (e.g. tab was backgrounded); + // otherwise the terminal listener has already been notified and a + // second dispatch causes a redundant texture-atlas clear + redraw. + if (!rafFired) dispatchWorkSurfaceRevealed(); }, 120); return () => { window.cancelAnimationFrame(raf); window.clearTimeout(settleTimer); }; }, [shouldReveal]);If the second dispatch is intentional belt-and-suspenders for the late-mount race (TerminalView hasn't attached its listener yet when the RAF fires), a brief comment to that effect would help future readers, and the listener-side could deduplicate by tracking the last-revealed timestamp.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/app/App.tsx` around lines 202 - 214, The effect in App.tsx currently calls dispatchWorkSurfaceRevealed() twice (once in the RAF callback and again in the 120ms timeout), causing TerminalView's expensive reveal handler to run twice; fix this by coalescing the two dispatches: introduce a local flag (e.g. rafFired) inside the React.useEffect, set rafFired = true when the RAF callback runs and only call dispatchWorkSurfaceRevealed() from the timeout if rafFired is false, keeping the existing cancelAnimationFrame and clearTimeout cleanup and leaving dispatchWorkSurfaceRevealed() and TerminalView's reveal listener unchanged.apps/desktop/src/main/services/projects/projectIconResolver.test.ts (1)
38-44: Minor: write the "outside" fixture into an isolated tmp dir, not the sharedos.tmpdir().
writeFile(path.dirname(root), "outside.svg", ...)writesoutside.svgdirectly into the OS tmpdir (path.dirname(root)is/tmpor equivalent), which:
- Leaks a fixture file into the shared system tmpdir on every test run (no cleanup).
- Risks collisions with other tests/processes touching the system tmpdir.
The next test (lines 46-55) already uses the safer pattern of creating a separate
mkdtempSyncfor outside fixtures — consider matching that here.♻️ Proposed change
it("does not resolve linked icons outside the project root", () => { const root = makeProjectRoot(); - writeFile(path.dirname(root), "outside.svg", "<svg>outside</svg>"); - writeFile(root, "index.html", '<link rel="icon" href="../outside.svg">'); + const outside = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), "ade-outside-"))); + fs.writeFileSync(path.join(outside, "outside.svg"), "<svg>outside</svg>"); + const rel = path.relative(root, path.join(outside, "outside.svg")); + writeFile(root, "index.html", `<link rel="icon" href="${rel}">`); expect(resolveProjectIconPath(root)).toBeNull(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/projects/projectIconResolver.test.ts` around lines 38 - 44, The test creates the outside fixture in the shared OS tmpdir via writeFile(path.dirname(root), "outside.svg", ...), which can leak files and collide with other tests; change the test to create an isolated temporary directory (e.g., via fs.mkdtempSync like the adjacent test does), write "outside.svg" into that isolated dir, update the link href in the project index.html to point to the file in that isolated dir, and then call resolveProjectIconPath(root) to assert null; adjust references to writeFile, makeProjectRoot, and resolveProjectIconPath in the test accordingly.apps/desktop/src/main/services/projects/projectIconResolver.ts (1)
43-46: Optional: broadenrelmatching to common variants.
LINK_ICON_HTML_REandLINK_ICON_OBJ_REonly acceptrel="icon"orrel="shortcut icon". Real-world projects often only declare anapple-touch-icon,mask-icon, or use multi-tokenrellikerel="alternate icon". Those projects will fall through tonulleven though a valid icon exists.If you want broader coverage, consider matching any
relattribute whose space-separated tokens includeicon(e.g. acceptrel="..."and post-filter on tokens). Not a blocker — just a coverage gap to keep in mind.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/projects/projectIconResolver.ts` around lines 43 - 46, The current LINK_ICON_HTML_RE and LINK_ICON_OBJ_RE only match rel="icon" or "shortcut icon", so broaden them to accept any rel attribute whose space-separated tokens include an icon-like token (e.g., "icon", "apple-touch-icon", "mask-icon", "alternate icon"); update the regexes to look for rel=["'][^"']*icon[^"']*["'] (or otherwise capture the full rel value) and then post-filter the captured rel by splitting on whitespace/commas and checking tokens for either exact "icon" or tokens that end with or contain "icon" before accepting the matched href; change LINK_ICON_HTML_RE and LINK_ICON_OBJ_RE accordingly and ensure the code path that extracts the href still uses the same capture group for href.apps/desktop/src/renderer/components/chat/ChatAttachmentTray.tsx (2)
2-2: Consider aliasing theImageimport to avoid shadowing the globalImageconstructor.Importing
Imagefrom@phosphor-icons/reactshadows the browser'sImageconstructor (HTMLImageElement) within this module. It isn't a bug today (the constructor isn't used here), but it's a known footgun if anyone later writesnew Image()for preloading or canvas work, and it can confuse type tooling. An alias keeps the intent obvious at call sites.♻️ Suggested rename
-import { Copy, File, Image, X } from "@phosphor-icons/react"; +import { Copy, File, Image as ImageIcon, X } from "@phosphor-icons/react";And update the JSX usage:
- <Image size={18} weight="bold" /> + <ImageIcon size={18} weight="bold" />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/chat/ChatAttachmentTray.tsx` at line 2, The import of Image from "@phosphor-icons/react" shadows the global Image constructor; change the import to alias it (e.g., import { Image as PhosphorImage } from "@phosphor-icons/react") and update any JSX usages of <Image ... /> to use the new alias (<PhosphorImage ... />) so the module no longer hides the global HTMLImageElement constructor.
55-63: Minor: defensive guard onwriteClipboardImageis inconsistent withgetImageDataUrl.Line 33 optional-chains
window.ade?.app?.getImageDataUrlbefore invocation, butcopyImagecallswindow.ade.app.writeClipboardImagedirectly. Thetry/catchdoes swallow the resultingTypeErrorif the bridge is unavailable (and the user sees "Copy failed", which is acceptable), so this is purely a consistency nit — feel free to defer.♻️ Optional alignment
const copyImage = async (event: MouseEvent<HTMLButtonElement>) => { event.stopPropagation(); + if (!window.ade?.app?.writeClipboardImage) { + setCopyState("failed"); + return; + } try { await window.ade.app.writeClipboardImage(attachment.path); setCopyState("copied"); } catch { setCopyState("failed"); } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/chat/ChatAttachmentTray.tsx` around lines 55 - 63, The copyImage function calls window.ade.app.writeClipboardImage directly while getImageDataUrl uses optional chaining; make the call defensive and consistent by optional-chaining the bridge before invoking writeClipboardImage (i.e., check window.ade?.app?.writeClipboardImage exists) inside copyImage and handle the absent function path by setting setCopyState("failed") (or early-return) so the try/catch remains safe; update references in copyImage to mirror the pattern used for getImageDataUrl to avoid runtime TypeErrors.
🤖 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/ipc/registerIpc.ts`:
- Around line 2281-2295: The handler for IPC.projectResolveIcon is incorrectly
using the generic renderer allowlist via resolveAllowedRendererPath which only
trusts getAllowedDirs (Downloads/Documents/Temp) and blocks valid project roots
(causing missing icons); update the handler to validate rootPath against the
project-specific allowlist instead of resolveAllowedRendererPath — e.g., call
the project-aware validator (the function that trusts the current project root,
such as resolveAllowedProjectPath or the variant that uses
getAllowedProjectDirs) or adjust getAllowedDirs/getAllowedProjectDirs to include
the current project roots before calling resolveProjectIcon; ensure you still
trim/guard the incoming args, catch validation failures and return the same {
dataUrl: null, sourcePath: null, mimeType: null } shape when validation fails,
and keep the final call to resolveProjectIcon(validatedRoot).
---
Nitpick comments:
In `@apps/desktop/src/main/services/projects/projectIconResolver.test.ts`:
- Around line 38-44: The test creates the outside fixture in the shared OS
tmpdir via writeFile(path.dirname(root), "outside.svg", ...), which can leak
files and collide with other tests; change the test to create an isolated
temporary directory (e.g., via fs.mkdtempSync like the adjacent test does),
write "outside.svg" into that isolated dir, update the link href in the project
index.html to point to the file in that isolated dir, and then call
resolveProjectIconPath(root) to assert null; adjust references to writeFile,
makeProjectRoot, and resolveProjectIconPath in the test accordingly.
In `@apps/desktop/src/main/services/projects/projectIconResolver.ts`:
- Around line 43-46: The current LINK_ICON_HTML_RE and LINK_ICON_OBJ_RE only
match rel="icon" or "shortcut icon", so broaden them to accept any rel attribute
whose space-separated tokens include an icon-like token (e.g., "icon",
"apple-touch-icon", "mask-icon", "alternate icon"); update the regexes to look
for rel=["'][^"']*icon[^"']*["'] (or otherwise capture the full rel value) and
then post-filter the captured rel by splitting on whitespace/commas and checking
tokens for either exact "icon" or tokens that end with or contain "icon" before
accepting the matched href; change LINK_ICON_HTML_RE and LINK_ICON_OBJ_RE
accordingly and ensure the code path that extracts the href still uses the same
capture group for href.
In `@apps/desktop/src/renderer/components/app/App.tsx`:
- Around line 202-214: The effect in App.tsx currently calls
dispatchWorkSurfaceRevealed() twice (once in the RAF callback and again in the
120ms timeout), causing TerminalView's expensive reveal handler to run twice;
fix this by coalescing the two dispatches: introduce a local flag (e.g.
rafFired) inside the React.useEffect, set rafFired = true when the RAF callback
runs and only call dispatchWorkSurfaceRevealed() from the timeout if rafFired is
false, keeping the existing cancelAnimationFrame and clearTimeout cleanup and
leaving dispatchWorkSurfaceRevealed() and TerminalView's reveal listener
unchanged.
In `@apps/desktop/src/renderer/components/chat/ChatAttachmentTray.tsx`:
- Line 2: The import of Image from "@phosphor-icons/react" shadows the global
Image constructor; change the import to alias it (e.g., import { Image as
PhosphorImage } from "@phosphor-icons/react") and update any JSX usages of
<Image ... /> to use the new alias (<PhosphorImage ... />) so the module no
longer hides the global HTMLImageElement constructor.
- Around line 55-63: The copyImage function calls
window.ade.app.writeClipboardImage directly while getImageDataUrl uses optional
chaining; make the call defensive and consistent by optional-chaining the bridge
before invoking writeClipboardImage (i.e., check
window.ade?.app?.writeClipboardImage exists) inside copyImage and handle the
absent function path by setting setCopyState("failed") (or early-return) so the
try/catch remains safe; update references in copyImage to mirror the pattern
used for getImageDataUrl to avoid runtime TypeErrors.
🪄 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: 51ab7e5f-649d-41be-921a-d62ecc5c9012
📒 Files selected for processing (6)
apps/desktop/src/main/services/ipc/registerIpc.tsapps/desktop/src/main/services/orchestrator/aiOrchestratorService.test.tsapps/desktop/src/main/services/projects/projectIconResolver.test.tsapps/desktop/src/main/services/projects/projectIconResolver.tsapps/desktop/src/renderer/components/app/App.tsxapps/desktop/src/renderer/components/chat/ChatAttachmentTray.tsx
Iter 1's "validate rootPath against allowlist" fix used the generic file allowlist (project root + Downloads/Documents/Temp), which broke tab/catalog icon resolution for any project living outside those folders (e.g. ~/code/*). Switch to a project-root allowlist sourced from globalState.recentProjects + current project root, so the access boundary is the legitimate set of known projects. CI stays green; Greptile + CodeRabbit + Copilot all reported terminal on iter 5. This pass addresses the one new diff-line comment (coderabbitai #3146852140) inline because it's a clear regression I introduced earlier in the loop. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@copilot review but do not make fixes |
Summary
Describe the change.
What Changed
Key files and behaviors.
Validation
How you tested.
Risks
Anything to watch.
Summary by CodeRabbit
New Features
Improvements
Greptile Summary
This PR ships project icon support across desktop tabs and iOS, image attachment previews with a fullscreen lightbox and clipboard copy in the chat tray, ANSI-colour rendering in the iOS terminal view, a richer iOS commit workflow with per-file staging, and a fix for blank terminals after workspace tab switches. It also resolves several previously flagged issues: the
shell.tsnewline-quoting P1 (now using$'...'ANSI-C quoting), the unbounded icon cache (now LRU-capped at 24), and the missingrootPathvalidation inprojectResolveIcon(now checked against the current + recent project allowlist).mobileProjectIconDataUrlhelper inmain.tssilently drops SVG icons —nativeImage.createFromPathreturns an empty image for SVG, and the PNG-only raw-URL fallback missesimage/svg+xml. Sincefavicon.svgis the first candidate inICON_CANDIDATES, most modern web projects will show no icon on iOS.Confidence Score: 4/5
Safe to merge; the one finding is a P2 feature gap (SVG icons missing on iOS), not a crash or data-loss issue.
All previously flagged P1/security issues have been addressed. The only new finding is that SVG project icons are silently omitted from the iOS project list because
nativeImage.createFromPathdoesn't handle SVG and the fallback checks only for PNG MIME type — a P2 functional gap, not a regression or security issue.apps/desktop/src/main/main.ts —
mobileProjectIconDataUrlSVG handlingImportant Files Changed
mobileProjectIconDataUrl; SVG icons are silently dropped becausenativeImage.createFromPathdoesn't handle SVG, and the PNG-only fallback misses the most common modern favicon format.<link rel="icon">hrefs; usesresolvePathWithinRootto guard against symlink traversal, and caps reads at 1 MB.appGetImageDataUrl,appWriteClipboardImage, andprojectResolveIconIPC handlers; all three validate the renderer-supplied path through appropriate allowlists and use magic-byte sniffing for image MIME detection.ImageAttachmentPreviewwith thumbnail, copy-to-clipboard, and fullscreen lightbox;ImageLightboxnow implements focus trapping and Escape handling, addressing the previous review finding.ProjectTabIconbacked by an LRU-capped module-level cache (max 24 entries), addressing the previous unbounded-growth concern.$'...'quoting for arguments containing line terminators, preventing PTY line-discipline from firing on embedded\nmid-command.maxStartDeltaMs,notBeforeMs, andrequiredTextfilters; fixes the direct-command / shell-fallback launch logic withlaunchedDirectCommandflag.buildTrackedCliLaunchCommand(returns command+args+startupCommand); addsresolveLaunchFieldsto prevent caller overrides from being mixed with defaults.hiddenwith CSS-based visibility forPersistentWorkSurface; addsinertattribute management and dispatchesade:work-surface-revealedso TerminalView can redraw when the workspace becomes visible.ade:work-surface-revealedto clear texture atlas and force a full redraw when the workspace panel is shown, fixing stale/blank terminal state after tab switches.Textrenderer to ANSI-colour-preservingAttributedStringreplay; adds a Termius-style horizontal key bar with modifier chips (Esc, Tab, arrows, Ctrl combos).workspaceNamemutable so a focused-lane change can trigger end+restart to update the immutableActivityAttributesheader; changes default from"Workspace"to""so empty sessions don't show a stale label.Sequence Diagram
sequenceDiagram participant R as Renderer participant IPC as IPC (registerIpc) participant Res as projectIconResolver participant FS as Filesystem Note over R,FS: Desktop project icon (TopBar) R->>IPC: projectResolveIcon(rootPath) IPC->>IPC: resolveAllowedProjectRoot(rootPath) IPC->>Res: resolveProjectIcon(validatedRoot) Res->>FS: resolvePathWithinRoot (symlink-safe stat per candidate) FS-->>Res: first matching icon path Res->>FS: readFileSync (≤1 MB) FS-->>Res: icon bytes Res-->>IPC: {dataUrl, sourcePath, mimeType} IPC-->>R: ProjectIcon Note over R,FS: Image attachment preview (ChatAttachmentTray) R->>IPC: appGetImageDataUrl(path) IPC->>IPC: resolveAllowedRendererPath(path) IPC->>IPC: stat + size check (≤10 MB) IPC->>IPC: sniffImageMimeType (magic bytes) IPC-->>R: {dataUrl} R->>R: render thumbnail → ImageLightbox on click Note over R,FS: iOS mobile icon sync (main.ts) R->>IPC: listMobileSyncProjects IPC->>Res: resolveProjectIcon(root) per project Res-->>IPC: ProjectIcon IPC->>IPC: nativeImage.createFromPath → resize 64×64 IPC-->>R: SyncMobileProjectSummary[].iconDataUrlComments Outside Diff (1)
apps/desktop/src/renderer/lib/shell.ts, line 40-47 (link)quoteShellArgdoes not escape newlines inside double-quoted stringsThe POSIX-path quote function only escapes
",\,$, and backtick. The codex preamble prompt returned byworkTabCodexPreamblePrompt()is a multi-line string (it joins parts with"\n"). When passed throughcommandArrayToLine→quoteShellArg, the embedded newlines are placed literally inside a double-quoted argument instartupCommand.This only matters on the shell-fallback path: if
codexdirect-spawn fails butstartupCommandis set,pty.write(startupCommand + "\r")is used. A literal\ninside the double-quoted string would be interpreted by the shell as pressing Enter mid-command, causing it to execute the incomplete quoted string and likely emit a syntax error before the rest of the preamble is seen.The direct-spawn args path is unaffected (args are passed to the process verbatim). Fix: escape
\nand\rinsidequoteShellArg's double-quote branch.Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (7): Last reviewed commit: "ship: iter 6 (force-finalize) — fix iter..." | Re-trigger Greptile