Autoresearch#286
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Owner
Author
|
@cursor review |
Owner
Author
|
@copilot review but do not make fixes |
📝 WalkthroughWalkthroughImplements perf-run system (main/IPC/preload/renderer, scenarios, aggregator, scripts), adds GitHub remote detection with IPC/preload APIs, refines lanes refresh/state and Git UX, introduces local-runtime disabled fast-paths, updates routing/UI clamps, and expands tests and SKILL docs. ChangesDesktop performance run framework
GitHub remote detection and IPC/preload surface
Local-runtime disabled fast-paths and sync IPC behavior
Lanes refresh model and Git history UX/error mapping
App routing bridge and drop handling
Renderer UI clamps, layout, and controls
Chat UI behavior, rendering, panels, and tests
Usage tracking JSONL streaming and limits
CLI server tests (ade-cli)
Skills docs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
|
Capy auto-review is paused for this organization because the monthly auto-review limit has been reached. Increase the limit or turn it off in billing settings to resume automatic reviews. |
Copilot stopped work on behalf of
arul28 due to an error
May 12, 2026 09:15
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/desktop/src/renderer/components/app/AppShell.tsx (1)
793-813:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReset
githubStatuson project-root change to prevent stale banner state.Switching from project A to B keeps A's
githubStatusuntil the delayed fetch runs (12–30s), so the banner displays incorrect status for B during that window.Suggested fix
useEffect(() => { let cancelled = false; if (!currentProjectRoot) { setGithubStatus(null); return; } + // Avoid cross-project banner bleed while waiting for delayed refresh. + setGithubStatus(null); const githubTimer = window.setTimeout(() => { void window.ade.github.getStatus().then((status) => { if (cancelled) return; setGithubStatus(status); }).catch(() => { if (cancelled) return; setGithubStatus(null); }); }, githubBannerDismissed ? GITHUB_STATUS_DISMISSED_BANNER_DELAY_MS : GITHUB_STATUS_STARTUP_DELAY_MS);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/components/app/AppShell.tsx` around lines 793 - 813, When the project root changes we need to clear any existing GitHub banner state immediately to avoid showing stale status; in the useEffect that depends on currentProjectRoot and githubBannerDismissed (the effect that declares cancelled, githubTimer, and calls window.ade.github.getStatus()), call setGithubStatus(null) right at the start of the effect whenever currentProjectRoot is truthy (i.e., not only when it's falsy) before scheduling the githubTimer. This ensures AppShell's githubStatus state is reset on project switches (referencing useEffect, currentProjectRoot, setGithubStatus, githubTimer, githubBannerDismissed) while preserving the existing delayed fetch and cleanup logic.apps/desktop/src/main/services/ipc/registerIpc.ts (1)
1940-1941:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPerf IPC events are still gated behind trace mode.
recordIpcInvokeAggregate()only runs inside the wrapper registered undertraceIpcInvokes. In packaged perf runs withoutADE_TRACE_IPC, that wrapper never installs, so the newipcInvokeperf events are never emitted. If perf logging is meant to work outside dev, the wrapper install condition needs to include perf mode too.Also applies to: 2154-2218
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/ipc/registerIpc.ts` around lines 1940 - 1941, The trace wrapper that installs recordIpcInvokeAggregate is only enabled when traceIpcInvokes is true, so packaged perf runs without ADE_TRACE_IPC never register the ipcInvoke perf events; change the traceIpcInvokes condition to also include your perf-mode flag (e.g. ADE_PERF or whatever runtime isPerfMode()/enablePerfLogging() you use) so the wrapper installs in perf runs too, and apply the same change to the other wrapper block that registers ipc invoke handlers (the later block that currently wraps recordIpcInvokeAggregate/traceEveryIpcInvoke logic) so both locations will emit ipcInvoke perf events outside dev when perf mode is enabled.
🧹 Nitpick comments (10)
apps/desktop/src/renderer/components/chat/ChatBuiltInBrowserPanel.test.tsx (1)
188-208: ⚡ Quick winVerify underlying behavior, not just UI text.
The test validates that the expected UI messages appear but doesn't verify that the screenshot capture flow was actually triggered or that the cancellation prevents context from being added.
Consider adding assertions to verify:
api.captureScreenshotwas called after clicking "Screenshot"onAddContextwas NOT called after cancelling✅ Suggested assertions to add
it("starts and cancels screenshot crop mode when chat context is available", async () => { const { api } = installBrowserApi(); api.captureScreenshot.mockResolvedValue({ dataUrl: "data:image/png;base64,iVBORw0KGgo=", width: 640, height: 360, capturedAt: "2026-05-12T00:00:00.000Z", }); - render(<ChatBuiltInBrowserPanel sessionId="chat-1" onAddContext={vi.fn()} />); + const onAddContext = vi.fn(); + render(<ChatBuiltInBrowserPanel sessionId="chat-1" onAddContext={onAddContext} />); fireEvent.click(await screen.findByText("Screenshot")); + await waitFor(() => expect(api.captureScreenshot).toHaveBeenCalled()); expect(await screen.findByText("Drag a browser region to attach the screenshot crop and nearby page context.")).toBeTruthy(); expect(screen.getByText("Cancel screenshot")).toBeTruthy(); fireEvent.click(screen.getByText("Cancel screenshot")); expect(await screen.findByText("Browser screenshot capture cancelled.")).toBeTruthy(); expect(screen.getByText("Screenshot")).toBeTruthy(); + expect(onAddContext).not.toHaveBeenCalled(); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/components/chat/ChatBuiltInBrowserPanel.test.tsx` around lines 188 - 208, The test currently only verifies UI text; update it to also assert the underlying behavior by (1) passing a mock for the onAddContext prop (e.g., a vi.fn()) to the rendered ChatBuiltInBrowserPanel and (2) after firing the "Screenshot" click assert that the installed browser API's captureScreenshot mock (api.captureScreenshot) was called, and then after clicking "Cancel screenshot" assert that the onAddContext mock was NOT called; locate these changes around the existing render(...) and fireEvent calls in the "starts and cancels screenshot crop mode when chat context is available" test so you verify api.captureScreenshot and onAddContext interactions for ChatBuiltInBrowserPanel using the installBrowserApi helper.apps/desktop/src/renderer/components/terminals/WorkViewArea.test.tsx (2)
212-278: 💤 Low valueConsider using data attributes instead of className assertions.
The test checks
pane().classNamefor the presence of "minimized" (lines 272, 275). While this works, it couples the test to implementation details. Consider havingFloatingPaneexpose adata-minimizedattribute for more robust testing.♻️ Suggested improvement
In
FloatingPanecomponent, add:<div data-pane-id="..." data-minimized={minimized ? "true" : "false"}>Then in the test:
-expect(pane().className).toContain("minimized"); +expect(pane().getAttribute("data-minimized")).toBe("true");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/components/terminals/WorkViewArea.test.tsx` around lines 212 - 278, The test currently asserts on pane().className contains "minimized" which couples it to styling; update the FloatingPane component to render a stable attribute (e.g., data-minimized with value "true"/"false") driven by its minimized prop, and then change the test in WorkViewArea.test.tsx to assert on that attribute via pane().getAttribute('data-minimized') (or use a queryBy selector for [data-minimized="true"]) instead of inspecting className; reference FloatingPane and the pane() helper in the test when making these edits.
323-325: 💤 Low valueClarify why all "Close tab" buttons are clicked.
The test clicks every element matching the title "Close tab" (line 323-324), but only asserts that
onCloseItemwas called with the specific session ID. If the intent is to test closing a specific tab, consider querying more precisely (e.g., by session ID or usinggetByTitleinstead ofgetAllByTitle).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/components/terminals/WorkViewArea.test.tsx` around lines 323 - 325, The test currently clicks every element returned by screen.getAllByTitle("Close tab") but only asserts on onCloseItem for a single session id; narrow the click target to the specific tab you want to test (e.g., query the close button for the session id or use getByTitle with a unique title per tab) or alternatively assert that onCloseItem was called for every session id clicked. Update the test around the click loop to locate the close button for the target session (reference onCloseItem and the test that renders the tab/session id) and perform a single click, then assert onCloseItem was called with that session id, or if keeping the loop verify the call count and arguments for each expected id.apps/desktop/src/renderer/components/terminals/WorkStartSurface.test.tsx (1)
9-12: 💤 Low valueConsider importing the actual store type for stronger type safety.
The inline type annotation for the
selectorparameter could drift from the actualappStoreshape. Consider importing the store's state type to ensure the mock stays in sync with the real implementation.♻️ Suggested improvement
+import type { AppState } from "../../state/appStore"; + vi.mock("../../state/appStore", () => ({ - useAppStore: (selector: (state: { selectedLaneId: string | null; selectLane: typeof selectLane }) => unknown) => + useAppStore: <T>(selector: (state: AppState) => T): T => selector({ selectedLaneId: null, selectLane }), }));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/components/terminals/WorkStartSurface.test.tsx` around lines 9 - 12, Replace the ad-hoc inline selector type with the real app store state type by importing the exported state/type from the module that defines the store (e.g., import { AppStoreState } from "../../state/appStore") and use that type for the selector parameter in the vi.mock for useAppStore; update references to selectLane/useAppStore to match the real signatures so the mock's shape stays in sync with the actual app store implementation.apps/desktop/src/renderer/components/terminals/SessionContextMenu.tsx (1)
68-89: 💤 Low valueVerify if
renamingdependency is necessary.The
useLayoutEffectincludesrenamingin its dependency array (line 89), which means the position will be recalculated when the user enters/exits rename mode. If the menu dimensions don't change when switching to rename mode, this dependency could be removed to avoid unnecessary layout recalculations.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/components/terminals/SessionContextMenu.tsx` around lines 68 - 89, The useLayoutEffect that computes and sets clampedPosition currently lists renaming in its dependency array causing unnecessary reflows; inspect whether the menu's DOM size or the values used (menu, menuRef.current.getBoundingClientRect()) actually change when renaming toggles and if not, remove renaming from the dependency array so the effect depends only on menu (and any other real sources like menuRef) — update the dependency list of the useLayoutEffect containing menu, menuRef, and setClampedPosition (the effect that reads menu, menuRef.current.getBoundingClientRect(), and calls setClampedPosition) to exclude renaming unless you confirm rename mode changes layout.apps/desktop/src/renderer/components/app/App.tsx (1)
331-346: 💤 Low valueConsider validating hash format before navigation.
The bridge navigates to
hash.slice(1)after checkinghash.startsWith("#/"), which handles the common case. However, edge cases like#//,#/../../, or hashes with query params might behave unexpectedly. Consider adding validation or normalization before callingnavigate.♻️ Suggested validation
const syncHashRoute = () => { const hash = window.location.hash; if (!hash.startsWith("#/")) return; - navigate(hash.slice(1), { replace: true }); + const path = hash.slice(1); + if (path && path !== "//" && !path.includes("..")) { + navigate(path, { replace: true }); + } };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/components/app/App.tsx` around lines 331 - 346, In BrowserHashRouteBridge (syncHashRoute), validate and normalize the extracted route before calling navigate: after confirming hash.startsWith("#/"), compute raw = hash.slice(1), split off any query/hash suffix (preserve suffix separately), collapse duplicate slashes, reject or reject-normalize any ".." path segments (e.g., remove or return root for paths containing traversal), ensure the path begins with a single "/", then reattach preserved query/hash if desired; wrap this in a try/catch and only call navigate(normalizedPath, { replace: true }) when normalizedPath passes these checks to avoid navigating to unsafe or malformed routes.scripts/reset-perf-pass.sh (1)
24-25: 💤 Low valueConsider adding a warning for destructive operation.
The
git clean -fdxcommand (line 25) will permanently delete all untracked and ignored files. While appropriate for a reset script, consider adding a warning message or confirmation prompt to prevent accidental data loss, especially if users might have uncommitted work in the perf-pass directory.
⚠️ Suggested warning+echo "[perf-pass] WARNING: this will delete all untracked files" echo "[perf-pass] clearing untracked" git clean -fdx🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/reset-perf-pass.sh` around lines 24 - 25, Add a clear, explicit warning and an optional confirmation before running the destructive command `git clean -fdx` in the reset script: update the block around the existing `echo "[perf-pass] clearing untracked"` / `git clean -fdx` to print a multi-line warning, prompt the user to confirm (Y/N), and only run `git clean -fdx` when the user confirms; also provide a non-interactive bypass (e.g., respect an env var like PERF_PASS_FORCE or CI) so automated runs are unaffected.apps/desktop/src/main/services/perf/perfIpc.ts (1)
21-64: ⚡ Quick winConsider adding
ipcMain.removeHandler()before handler registration for defensive idempotency.While
registerPerfIpcHandlers()is currently called once during initialization, addingipcMain.removeHandler()for each channel before registration (e.g.,IPC.perfGetConfig,IPC.perfRecordEvent,IPC.perfScenarioComplete,IPC.perfFinalize) would guard against potential re-registration errors if the function is invoked multiple times in future code paths.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/perf/perfIpc.ts` around lines 21 - 64, Add defensive idempotency by removing existing IPC handlers before registering new ones: in registerPerfIpcHandlers() call ipcMain.removeHandler(...) for each channel (IPC.perfGetConfig, IPC.perfRecordEvent, IPC.perfScenarioComplete, IPC.perfFinalize) immediately before the corresponding ipcMain.handle(...) so re-calling registerPerfIpcHandlers() won't throw or double-register handlers; keep the existing handler implementations (the anonymous callbacks that call getActiveRun, isRunActive, appendEvent, aggregate, etc.) unchanged.apps/desktop/src/renderer/perf/markers.ts (1)
47-56: 💤 Low valueChrome-specific
performance.memoryAPI may not be available in all contexts.The
performance.memoryproperty is a non-standard Chrome extension and may be unavailable depending on Electron configuration or future browser changes. The code handles this gracefully by returningnull, but the lack of availability logging could make debugging harder.Consider logging once when memory sampling is unavailable:
const memoryReader = () => { const perf = performance as Performance & { memory?: { usedJSHeapSize?: number; totalJSHeapSize?: number }; }; - if (!perf.memory) return null; + if (!perf.memory) { + if (perfActive) console.debug('[perf] performance.memory unavailable'); + return null; + } return { usedMB: Math.round((perf.memory.usedJSHeapSize ?? 0) / 1024 / 1024), totalMB: Math.round((perf.memory.totalJSHeapSize ?? 0) / 1024 / 1024), }; };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/perf/markers.ts` around lines 47 - 56, memoryReader currently returns null when performance.memory is absent but never logs this, making it hard to debug; update the memoryReader function to emit a single one-time warning when performance.memory is unavailable by adding a module-level flag (e.g., memorySamplingUnavailableLogged) and, inside memoryReader, if !perf.memory and the flag is false, call the chosen logger (console.warn or the app's logger) with a clear message and set the flag to true so subsequent calls don't spam logs; reference the memoryReader function and the new memorySamplingUnavailableLogged flag when implementing.scripts/run-perf-scenario.mjs (1)
14-17: ⚡ Quick winMigrate from deprecated
rmSyncrecursive option.
rmSyncwith{ recursive: true }is available in Node.js 22.x but is deprecated. Usefs.promises.rm()with{ recursive: true, force: true }instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/run-perf-scenario.mjs` around lines 14 - 17, The script currently uses the deprecated synchronous rmSync import; replace uses of rmSync with the promise API (node:fs.promises.rm or import { rm } from "node:fs/promises") and call it with options { recursive: true, force: true } (e.g., await rm(targetPath, { recursive: true, force: true })). Update the top imports to remove rmSync and add the promise rm (or import { promises as fsPromises } and use fsPromises.rm), and ensure the surrounding scope is async or use top-level await so the removal is awaited; keep all other logic unchanged and use the same target variable names where rmSync was previously invoked.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/desktop/src/main/services/github/githubService.ts`:
- Around line 256-257: The current code short-circuits and returns repo: null if
parseGitHubRepoFromRemoteUrl(configOriginUrl) returns null; change the logic in
the block that calls readOriginUrlFromGitConfig(projectRoot) so it only returns
early when parseGitHubRepoFromRemoteUrl(...) yields a non-null repo object,
otherwise continue to the runGit fallback (do not exit with null). Apply the
same change to the similar block around the other origin parsing (lines 268-271)
so both uses of parseGitHubRepoFromRemoteUrl permit falling through to runGit
when the parser fails.
In `@apps/desktop/src/main/services/ipc/registerIpc.ts`:
- Around line 1779-1847: The current module creates a single frozen
unavailableSyncSnapshot and returns it from buildUnavailableSyncSnapshot, which
incorrectly reports transferReadiness.ready = true and uses static timestamps;
change buildUnavailableSyncSnapshot to construct and return a new
SyncRoleSnapshot each call (create a fresh SyncDeviceRecord and SyncRoleSnapshot
inside buildUnavailableSyncSnapshot) and set transferReadiness.ready = false
with an appropriate blocker (e.g., ["local_runtime_daemon_disabled"]) and update
survivableState/survivableStateText and timestamps
(createdAt/updatedAt/lastSeenAt and snapshot updatedAt) so every request gets a
fresh, correctly blocked/unavailable snapshot; update references to
unavailableSyncSnapshot to only hold templates/constants if needed but ensure
buildUnavailableSyncSnapshot performs the full construction.
- Around line 2121-2129: The perfAppend call inside the isPerfRunActive() block
can throw and bubble up into the IPC handler; wrap the perfAppend(...)
invocation in a try/catch so telemetry writes are best-effort and cannot fail
the IPC call. Specifically, in registerIpc.ts around the code that builds the
ipcInvoke perf event, catch any error from perfAppend (while keeping the
existing isPerfRunActive check and event shape with
channel/winId/durationMs/failed) and swallow it or log it to a non-failing
logger so the IPC response is unaffected.
In `@apps/desktop/src/main/services/perf/aggregator.ts`:
- Around line 107-120: The readEvents helper currently loads the entire file
into memory with readFileSync(...).split("\n"), which can OOM for large JSONL
logs; replace it with a streaming line-by-line reader (use fs.createReadStream +
readline.createInterface) inside readEvents to parse each line and push parsed
Event objects as they are read, skipping malformed lines, and return the
accumulated array only after the stream closes; apply the same streaming fix to
the other JSONL-reading code block referenced (the similar pattern used in
aggregate()/finalize code) so no code path materializes the whole file in
memory.
- Around line 131-133: The current computation sets startedAt = events[0]!.ts
and endedAt = events[events.length - 1]!.ts which assumes events are strictly
ordered; instead scan the events array to compute the minimum timestamp for
startedAt and the maximum timestamp for endedAt (and handle the empty-events
case), then derive durationMs from those min/max values so out-of-order appends
won't skew metrics (update references to events, startedAt, endedAt, and
durationMs accordingly).
In `@apps/desktop/src/main/services/perf/perfIpc.ts`:
- Around line 58-63: The handler registered with ipcMain.handle for
IPC.perfFinalize currently calls aggregate(run.runId) without guarding against
exceptions; wrap the aggregate call in a try/catch inside the ipcMain.handle
callback (keeping the existing getActiveRun() early-return) and on error return
a deterministic shaped response like { ok: false, reason: "aggregate-error",
error: String(err) } so the IPC consumer always receives a typed failure
payload; reference the ipcMain.handle(IPC.perfFinalize, ...), getActiveRun(),
and aggregate(run.runId) symbols when making the change.
- Around line 34-39: Validate the incoming kind in the IPC.perfRecordEvent
handler instead of casting to never: check args.kind (from PerfRecordEventArgs)
against the allowed PerfEventKind values
("scenarioStart","scenarioEnd","mark","measure","webVital","longTask","ipcInvoke","processMetrics","rendererMemory","note")
and if it isn’t one of them return { ok: false, reason: "invalid-kind" }; only
then call appendEvent({ ts, kind, ...rest }). Also update the perfFinalize
handler to wrap the call to aggregate() in a try-catch and return a structured
failure (e.g. { ok: false, reason: "aggregate-failed" }) when aggregate() throws
so the IPC call does not reject unhandled.
In `@apps/desktop/src/main/services/perf/perfLog.ts`:
- Around line 36-55: The ADE_PERF_RUN_ID value is used directly in
initPerfRunFromEnv to build a filesystem path, allowing path traversal via
slashes or ..; fix by sanitizing/validating the runId returned by readEnvRunId
before using it: in readEnvRunId or immediately in initPerfRunFromEnv, reject or
normalize values that contain path separators or parent-dir segments and only
allow a safe whitelist (e.g., alphanumeric, hyphen, underscore) or fallback to
path.basename(runId) if appropriate; update ensureDir behavior to operate only
on the validated dir string (references: readEnvRunId, initPerfRunFromEnv,
ensureDir).
In `@apps/desktop/src/renderer/components/chat/CursorCloudInlineLaunch.test.tsx`:
- Around line 62-64: The test uses waitFor to assert a negative case which
causes unnecessary timeout; remove the waitFor wrapper and replace it with a
direct synchronous assertion —
expect(window.ade.ai.cursorCloudCreateRun).not.toHaveBeenCalled(); — immediately
after the synchronous cancel click/onClose action so the test does not wait;
update the assertion that currently references waitFor(...) to a plain
expect(...) and keep references to window.ade.ai.cursorCloudCreateRun and the
onClose/cancel interaction.
In `@apps/desktop/src/renderer/perf/harness/index.ts`:
- Around line 35-38: The setTimeout callback in start() calls
runScenario(cfg.scenario) but ignores the returned Promise, causing unhandled
rejections; update the setTimeout callback to handle the Promise returned by
runScenario (e.g., call runScenario(cfg.scenario as string).catch(err => { /*
log the error */ }) or wrap in an async IIFE and attach .catch) so any rejection
is logged/handled instead of failing silently; refer to the start function and
runScenario(cfg.scenario) call when making the change.
In `@apps/desktop/src/renderer/perf/scenarios/boot.ts`:
- Line 39: Replace the hard-coded macOS path assigned to the perfPass constant
with a platform-agnostic resolution: read process.env.ADE_PERF_PASS_DIR first
(use that if set), otherwise compute a default using the current user home
directory (os.homedir()) and path.join to append the relative "Projects/perf
pass" segment; update the perfPass constant to use this value and ensure the
module imports/uses Node's os and path utilities where perfPass is defined.
- Around line 45-47: The renderer is passing { rootPath: perfPass } to
window.ade?.project.openRepo despite the preload/IPC openRepo signature taking
no parameters; remove the incorrect argument and the fake type assertion. Update
the call at the site using window.ade?.project.openRepo (the const result =
await ... line) to invoke openRepo() with no args and drop the (args?: {
rootPath?: string }) cast, or alternatively if you prefer accepting a rootPath
change the preload declaration (openRepo: async (rootPath?: string) =>
Promise<ProjectInfo | null>) and the IPC handler to accept an optional rootPath
parameter so the renderer, preload, and IPC signatures match.
- Around line 66-68: The code asserts listTargets returns Promise<unknown[]>;
instead import and use the shared RemoteRuntimeTarget type and replace the
unknown[] assertion so targets is typed as Promise<RemoteRuntimeTarget[]> (or
const targets: RemoteRuntimeTarget[] after awaiting). Update the type import
(RemoteRuntimeTarget) and change the assertion on (window.ade as unknown as {
remoteRuntime?: { listTargets?: () => Promise<RemoteRuntimeTarget[]> } }) so
remoteRuntime?.listTargets?.() and the resulting targets variable use
RemoteRuntimeTarget to match the preload IPC contract.
In `@apps/desktop/src/renderer/perf/scenarios/index.ts`:
- Around line 114-132: The early-return branches in runScenario (the "scenario
not found" and the requiresClaude check) skip calling finalize(), leaving runs
without terminal artifacts; wrap the main body of runScenario in a try/finally
or ensure finalize() is invoked before returning in those branches so
finalization always runs. Specifically, when handling the missing scenario
(where you call window.ade?.perf?.recordEvent with note "scenarioNotFound") and
the requiresClaude block (where you call window.ade?.perf?.scenarioComplete),
make sure to call the same finalize() path (or call
window.ade?.perf?.scenarioComplete then finalize()) before returning so
runScenario always finishes with finalize() regardless of early exits.
In `@apps/desktop/src/renderer/perf/scenarios/lanes.ts`:
- Around line 6-9: The readiness check in the ctx.waitFor call uses a permissive
selector ("[data-route='lanes'], main") which can be true before the /lanes
content is rendered; change the condition to wait for a lanes-specific DOM
signal (e.g., only "[data-route='lanes']" or an inner element unique to the
lanes view like ".lanes-container" or ".lanes-list") so timing measures reflect
actual lanes readiness, and make the same change for the second ctx.waitFor
usage at the other occurrence.
- Around line 79-83: The selected element stored in scrollable may not actually
be scrollable; modify the selection/validation so after obtaining the node
(variable scrollable) you verify it's scrollable by checking computed style
overflow (overflow/overflowY contains "auto" or "scroll") and that scrollHeight
> clientHeight (or otherwise use document.scrollingElement as a fallback). If
the checks fail, try to locate a truly scrollable ancestor or fall back and keep
the ctx.assert(false, "no scrollable container found in lanes") behavior when no
scrollable element can be found. Ensure this validation occurs wherever
scrollable is used so the scroll loop records real scrolling.
In `@apps/desktop/src/renderer/perf/webVitals.ts`:
- Around line 59-68: The CLS accumulator is reset per PerformanceObserver
callback because `total` is local inside the `observe("layout-shift", ...)`
callback; change it to a persistent accumulator (e.g., module-level or
closure-scoped variable like `let cumulativeCLS = 0`) and in the
`observe("layout-shift", (entries) => { ... })` handler add each eligible
`entry.value` to `cumulativeCLS` (skip `entry.hadRecentInput`) and then call
`send("webVital", { metric: "CLS", value: cumulativeCLS })` when `cumulativeCLS
> 0` so CLS is cumulative across the page lifecycle rather than per callback
batch.
In `@scripts/perf-launch.mjs`:
- Around line 80-92: The stop function currently sends SIGTERM and schedules a
SIGKILL but immediately calls process.exit(code), preventing the parent from
waiting for the child; update stop to send SIGTERM to child (as it does), then
wait for the child's termination (listen for the child's 'exit' or 'close' event
or await a Promise that resolves on that event) with a timeout of ~5s, and only
if the timeout elapses send SIGKILL and wait again briefly before finally
calling process.exit(code); reference the stop function and the child variable
(and use SIGTERM / SIGKILL) so the signal send and the event-listening Promise
replace the immediate process.exit call.
- Around line 49-53: The code hardcodes a machine-specific defaultPerfPass
("/Users/admin/Projects/perf pass"); remove that constant and stop falling back
to it. Update the projectRoot assignment so it uses args.noProject to create a
temp dir via mkdtempSync(join(tmpdir(), "ade-perf-no-project-")), otherwise
prefer args.projectRoot or process.env.ADE_PERF_PASS_DIR and finally fall back
to a neutral value (e.g., process.cwd() or throw an informative error) instead
of the hardcoded path; change references to defaultPerfPass accordingly so no
machine-specific path remains.
In `@scripts/run-perf-scenario.mjs`:
- Around line 44-47: Replace the macOS-hardcoded defaultPerfPass and add cleanup
for temp dirs: make the default perf pass path platform-agnostic (derive from
os.homedir() and path.join or respect ADE_PERF_PASS_DIR) instead of
"/Users/admin/Projects/perf pass", and when noProject is true use
mkdtempSync(tmpdir(), ...) to create perfPassDir but also record that path and
ensure it is removed after use (or on process termination) by registering a
cleanup handler (process.on('exit') / 'SIGINT' / 'uncaughtException') that
removes the temp directory (fs.rmSync or fs.rmdirSync with recursive option).
Update references to defaultPerfPass, perfPassDir, noProject, mkdtempSync,
tmpdir and ADE_PERF_PASS_DIR accordingly.
---
Outside diff comments:
In `@apps/desktop/src/main/services/ipc/registerIpc.ts`:
- Around line 1940-1941: The trace wrapper that installs
recordIpcInvokeAggregate is only enabled when traceIpcInvokes is true, so
packaged perf runs without ADE_TRACE_IPC never register the ipcInvoke perf
events; change the traceIpcInvokes condition to also include your perf-mode flag
(e.g. ADE_PERF or whatever runtime isPerfMode()/enablePerfLogging() you use) so
the wrapper installs in perf runs too, and apply the same change to the other
wrapper block that registers ipc invoke handlers (the later block that currently
wraps recordIpcInvokeAggregate/traceEveryIpcInvoke logic) so both locations will
emit ipcInvoke perf events outside dev when perf mode is enabled.
In `@apps/desktop/src/renderer/components/app/AppShell.tsx`:
- Around line 793-813: When the project root changes we need to clear any
existing GitHub banner state immediately to avoid showing stale status; in the
useEffect that depends on currentProjectRoot and githubBannerDismissed (the
effect that declares cancelled, githubTimer, and calls
window.ade.github.getStatus()), call setGithubStatus(null) right at the start of
the effect whenever currentProjectRoot is truthy (i.e., not only when it's
falsy) before scheduling the githubTimer. This ensures AppShell's githubStatus
state is reset on project switches (referencing useEffect, currentProjectRoot,
setGithubStatus, githubTimer, githubBannerDismissed) while preserving the
existing delayed fetch and cleanup logic.
---
Nitpick comments:
In `@apps/desktop/src/main/services/perf/perfIpc.ts`:
- Around line 21-64: Add defensive idempotency by removing existing IPC handlers
before registering new ones: in registerPerfIpcHandlers() call
ipcMain.removeHandler(...) for each channel (IPC.perfGetConfig,
IPC.perfRecordEvent, IPC.perfScenarioComplete, IPC.perfFinalize) immediately
before the corresponding ipcMain.handle(...) so re-calling
registerPerfIpcHandlers() won't throw or double-register handlers; keep the
existing handler implementations (the anonymous callbacks that call
getActiveRun, isRunActive, appendEvent, aggregate, etc.) unchanged.
In `@apps/desktop/src/renderer/components/app/App.tsx`:
- Around line 331-346: In BrowserHashRouteBridge (syncHashRoute), validate and
normalize the extracted route before calling navigate: after confirming
hash.startsWith("#/"), compute raw = hash.slice(1), split off any query/hash
suffix (preserve suffix separately), collapse duplicate slashes, reject or
reject-normalize any ".." path segments (e.g., remove or return root for paths
containing traversal), ensure the path begins with a single "/", then reattach
preserved query/hash if desired; wrap this in a try/catch and only call
navigate(normalizedPath, { replace: true }) when normalizedPath passes these
checks to avoid navigating to unsafe or malformed routes.
In `@apps/desktop/src/renderer/components/chat/ChatBuiltInBrowserPanel.test.tsx`:
- Around line 188-208: The test currently only verifies UI text; update it to
also assert the underlying behavior by (1) passing a mock for the onAddContext
prop (e.g., a vi.fn()) to the rendered ChatBuiltInBrowserPanel and (2) after
firing the "Screenshot" click assert that the installed browser API's
captureScreenshot mock (api.captureScreenshot) was called, and then after
clicking "Cancel screenshot" assert that the onAddContext mock was NOT called;
locate these changes around the existing render(...) and fireEvent calls in the
"starts and cancels screenshot crop mode when chat context is available" test so
you verify api.captureScreenshot and onAddContext interactions for
ChatBuiltInBrowserPanel using the installBrowserApi helper.
In `@apps/desktop/src/renderer/components/terminals/SessionContextMenu.tsx`:
- Around line 68-89: The useLayoutEffect that computes and sets clampedPosition
currently lists renaming in its dependency array causing unnecessary reflows;
inspect whether the menu's DOM size or the values used (menu,
menuRef.current.getBoundingClientRect()) actually change when renaming toggles
and if not, remove renaming from the dependency array so the effect depends only
on menu (and any other real sources like menuRef) — update the dependency list
of the useLayoutEffect containing menu, menuRef, and setClampedPosition (the
effect that reads menu, menuRef.current.getBoundingClientRect(), and calls
setClampedPosition) to exclude renaming unless you confirm rename mode changes
layout.
In `@apps/desktop/src/renderer/components/terminals/WorkStartSurface.test.tsx`:
- Around line 9-12: Replace the ad-hoc inline selector type with the real app
store state type by importing the exported state/type from the module that
defines the store (e.g., import { AppStoreState } from "../../state/appStore")
and use that type for the selector parameter in the vi.mock for useAppStore;
update references to selectLane/useAppStore to match the real signatures so the
mock's shape stays in sync with the actual app store implementation.
In `@apps/desktop/src/renderer/components/terminals/WorkViewArea.test.tsx`:
- Around line 212-278: The test currently asserts on pane().className contains
"minimized" which couples it to styling; update the FloatingPane component to
render a stable attribute (e.g., data-minimized with value "true"/"false")
driven by its minimized prop, and then change the test in WorkViewArea.test.tsx
to assert on that attribute via pane().getAttribute('data-minimized') (or use a
queryBy selector for [data-minimized="true"]) instead of inspecting className;
reference FloatingPane and the pane() helper in the test when making these
edits.
- Around line 323-325: The test currently clicks every element returned by
screen.getAllByTitle("Close tab") but only asserts on onCloseItem for a single
session id; narrow the click target to the specific tab you want to test (e.g.,
query the close button for the session id or use getByTitle with a unique title
per tab) or alternatively assert that onCloseItem was called for every session
id clicked. Update the test around the click loop to locate the close button for
the target session (reference onCloseItem and the test that renders the
tab/session id) and perform a single click, then assert onCloseItem was called
with that session id, or if keeping the loop verify the call count and arguments
for each expected id.
In `@apps/desktop/src/renderer/perf/markers.ts`:
- Around line 47-56: memoryReader currently returns null when performance.memory
is absent but never logs this, making it hard to debug; update the memoryReader
function to emit a single one-time warning when performance.memory is
unavailable by adding a module-level flag (e.g.,
memorySamplingUnavailableLogged) and, inside memoryReader, if !perf.memory and
the flag is false, call the chosen logger (console.warn or the app's logger)
with a clear message and set the flag to true so subsequent calls don't spam
logs; reference the memoryReader function and the new
memorySamplingUnavailableLogged flag when implementing.
In `@scripts/reset-perf-pass.sh`:
- Around line 24-25: Add a clear, explicit warning and an optional confirmation
before running the destructive command `git clean -fdx` in the reset script:
update the block around the existing `echo "[perf-pass] clearing untracked"` /
`git clean -fdx` to print a multi-line warning, prompt the user to confirm
(Y/N), and only run `git clean -fdx` when the user confirms; also provide a
non-interactive bypass (e.g., respect an env var like PERF_PASS_FORCE or CI) so
automated runs are unaffected.
In `@scripts/run-perf-scenario.mjs`:
- Around line 14-17: The script currently uses the deprecated synchronous rmSync
import; replace uses of rmSync with the promise API (node:fs.promises.rm or
import { rm } from "node:fs/promises") and call it with options { recursive:
true, force: true } (e.g., await rm(targetPath, { recursive: true, force: true
})). Update the top imports to remove rmSync and add the promise rm (or import {
promises as fsPromises } and use fsPromises.rm), and ensure the surrounding
scope is async or use top-level await so the removal is awaited; keep all other
logic unchanged and use the same target variable names where rmSync was
previously invoked.
🪄 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: 7ab976e0-08f4-43d1-a58e-9a60856a1bfe
⛔ Files ignored due to path filters (7)
docs/ARCHITECTURE.mdis excluded by!docs/**docs/features/files-and-editor/editor-surfaces.mdis excluded by!docs/**docs/features/lanes/README.mdis excluded by!docs/**docs/features/remote-runtime/README.mdis excluded by!docs/**docs/features/remote-runtime/internal-architecture.mdis excluded by!docs/**docs/features/terminals-and-sessions/README.mdis excluded by!docs/**docs/perf/work-tab-action-inventory.mdis excluded by!docs/**
📒 Files selected for processing (72)
.agents/skills/ade-autoresearch/SKILL.md.agents/skills/ade-perf-boot/SKILL.md.agents/skills/ade-perf-lanes/SKILL.md.agents/skills/ade-perf-work/SKILL.mdapps/ade-cli/src/multiProjectRpcServer.test.tsapps/ade-cli/src/stdioRpcDaemon.test.tsapps/desktop/src/main/main.tsapps/desktop/src/main/services/git/gitOperationsService.test.tsapps/desktop/src/main/services/git/gitOperationsService.tsapps/desktop/src/main/services/github/githubService.test.tsapps/desktop/src/main/services/github/githubService.tsapps/desktop/src/main/services/ipc/registerIpc.tsapps/desktop/src/main/services/ipc/runtimeBridge.test.tsapps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.test.tsapps/desktop/src/main/services/perf/aggregator.tsapps/desktop/src/main/services/perf/metricsSampler.tsapps/desktop/src/main/services/perf/perfIpc.tsapps/desktop/src/main/services/perf/perfLog.tsapps/desktop/src/main/services/usage/usageTrackingService.test.tsapps/desktop/src/main/services/usage/usageTrackingService.tsapps/desktop/src/preload/global.d.tsapps/desktop/src/preload/preload.test.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/components/app/App.tsxapps/desktop/src/renderer/components/app/App.workKeepAlive.test.tsxapps/desktop/src/renderer/components/app/AppShell.tsxapps/desktop/src/renderer/components/app/CommandPalette.test.tsxapps/desktop/src/renderer/components/app/TopBar.test.tsxapps/desktop/src/renderer/components/chat/AgentChatComposer.test.tsxapps/desktop/src/renderer/components/chat/AgentChatMessageList.test.tsxapps/desktop/src/renderer/components/chat/AgentChatMessageList.tsxapps/desktop/src/renderer/components/chat/AgentChatPane.companionDrawers.test.tsxapps/desktop/src/renderer/components/chat/ChatAppControlPanel.test.tsxapps/desktop/src/renderer/components/chat/ChatBuiltInBrowserPanel.test.tsxapps/desktop/src/renderer/components/chat/ChatGitToolbar.test.tsxapps/desktop/src/renderer/components/chat/ChatIosSimulatorPanel.test.tsxapps/desktop/src/renderer/components/chat/ChatIosSimulatorPanel.tsxapps/desktop/src/renderer/components/chat/ChatSubagentsPanel.test.tsxapps/desktop/src/renderer/components/chat/ChatTerminalDrawer.test.tsxapps/desktop/src/renderer/components/chat/ChatWorkLogBlock.tsxapps/desktop/src/renderer/components/chat/CursorCloudInlineLaunch.test.tsxapps/desktop/src/renderer/components/files/FilesPage.test.tsxapps/desktop/src/renderer/components/files/FilesPage.tsxapps/desktop/src/renderer/components/lanes/CommitTimeline.test.tsxapps/desktop/src/renderer/components/lanes/CommitTimeline.tsxapps/desktop/src/renderer/components/lanes/LaneDiffPane.test.tsxapps/desktop/src/renderer/components/lanes/LaneGitActionsPane.test.tsxapps/desktop/src/renderer/components/lanes/LaneGitActionsPane.tsxapps/desktop/src/renderer/components/lanes/LanesPage.test.tsapps/desktop/src/renderer/components/lanes/LanesPage.tsxapps/desktop/src/renderer/components/terminals/LaneCombobox.tsxapps/desktop/src/renderer/components/terminals/MacosVmPanel.test.tsxapps/desktop/src/renderer/components/terminals/SessionContextMenu.tsxapps/desktop/src/renderer/components/terminals/SessionListPane.test.tsxapps/desktop/src/renderer/components/terminals/SessionListPane.tsxapps/desktop/src/renderer/components/terminals/WorkSidebar.tsxapps/desktop/src/renderer/components/terminals/WorkStartSurface.test.tsxapps/desktop/src/renderer/components/terminals/WorkViewArea.test.tsxapps/desktop/src/renderer/lib/useGithubProjectRemote.tsapps/desktop/src/renderer/main.tsxapps/desktop/src/renderer/perf/harness/index.tsapps/desktop/src/renderer/perf/markers.tsapps/desktop/src/renderer/perf/scenarios/boot.tsapps/desktop/src/renderer/perf/scenarios/index.tsapps/desktop/src/renderer/perf/scenarios/lanes.tsapps/desktop/src/renderer/perf/webVitals.tsapps/desktop/src/renderer/state/appStore.test.tsapps/desktop/src/renderer/state/appStore.tsapps/desktop/src/shared/ipc.tsscripts/perf-launch.mjsscripts/reset-perf-pass.shscripts/run-perf-scenario.mjs
Owner
Author
|
@cursor review |
Owner
Author
|
@copilot review but do not make fixes |
Copilot stopped work on behalf of
arul28 due to an error
May 12, 2026 10:11
Instruments ADE for an iterative perf-optimization loop driven by an agentskills.io SKILL. Adds main-process metrics sampler (app metrics + memory), renderer-side perfMark/perfMeasure + PerformanceObserver web vitals, a tap into the existing IPC trace wrapper, and a JSONL + summary aggregator that produces a single fitness score per scenario run. Includes per-tab scenario harness (lanes + boot), launch shortcuts (perf-launch.mjs / run-perf-scenario.mjs with --no-project and --tab), and the ade-autoresearch / ade-perf-lanes / ade-perf-boot skills under .agents/skills/. Smoke-verified end-to-end on the lanes tab — summary.json populated with real signal across all six fitness components. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(app): honor hash routes in browser router * perf(lanes): skip disabled local runtime bridge real lanes baseline failed at lanes.idle-at-rest with V8 OOM; optimized scenarios all passed, fitness 7028.82 * perf(lanes): suppress duplicate git actions fullscreen pane UI audit: Git Actions expand went from 2 toolbars to 1. Post-marker IPC changed from 30 calls / 223ms to 29 calls / 192ms, removing one duplicate ade.git.getSyncStatus. * perf(lanes): fast-path disabled sync runtime UI audit: sync IPC in local-runtime-disabled perf mode went from 4 failed calls / 1145ms to 3 successful calls / 2ms. * perf(lanes): scope lane refreshes during UI actions * ship: prepare lane for review * ship: iteration 1 - address snapshot refresh review * ship: iteration 2 - address refresh review * ship: iteration 3 - refresh git action overlays
Squash-merge Work tab optimization lane into ade/autoresearch-d23e8745 after automate/finalize and review fixes.
arul28
force-pushed
the
ade/autoresearch-d23e8745
branch
from
5da119c to
a5dc996
Compare
Owner
Author
|
@copilot review but do not make fixes |
Owner
Author
|
@cursor review |
Copilot stopped work on behalf of
arul28 due to an error
May 12, 2026 10:28
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/desktop/src/main/services/ipc/registerIpc.ts (1)
1886-1891:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDisabled mode still leaks into generic sync error paths.
requireSyncService()is now the shared fallback for most sync IPCs, but it still throws"Sync service is not available."whenADE_DISABLE_LOCAL_RUNTIME_DAEMON=1.syncGetStatusandsyncSetActiveLanePresencespecial-case disabled mode, whilesyncRefreshDiscovery,syncListDevices,syncGetTransferReadiness, pin/connect/disconnect, etc. still go through this helper. That means the renderer can successfully detect the disabled/unavailable state and then immediately hit user-visible errors on the next sync call. Fold the disabled-mode branch into the shared helper so the whole sync surface behaves consistently in this mode.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/ipc/registerIpc.ts` around lines 1886 - 1891, The helper requireSyncService currently throws when no service exists, which leaks a generic error while some IPCs special-case disabled mode; change requireSyncService to detect the disabled-local-runtime-daemon state (use the existing ADE_DISABLE_LOCAL_RUNTIME_DAEMON check or the same utility that other IPC handlers use) and instead return a consistent "disabled" sentinel that implements the same interface as createSyncService (or return a well-known DisabledSyncError instance) so all callers (e.g., syncGetStatus, syncSetActiveLanePresence, syncRefreshDiscovery, syncListDevices, syncGetTransferReadiness, pin/connect/disconnect) can handle disabled mode uniformly via this single shared path rather than getting the generic "Sync service is not available." exception.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/desktop/src/main/services/perf/aggregator.ts`:
- Around line 197-198: The case "processMetrics" branch blindly casts and pushes
ev into processSamples causing a crash later when code (e.g.,
s.processes.find(...) in the aggregator/finalize flow) expects a valid processes
array; validate ev before casting by checking that ev.processes exists and is an
Array (e.g., Array.isArray((ev as any).processes)) and that its items have
expected shape (or at least non-empty), and if the check fails, skip the sample
(optionally log or increment a dropped counter) instead of pushing to
processSamples; update the case "processMetrics" handling where processSamples
is appended and use the ProcessMetricSample type only after the validation.
- Around line 144-146: The aggregate function currently uses runId directly in
join(homedir(), ".ade", "perf-runs", runId) (the dir variable) which allows path
traversal; validate or sanitize runId before building paths by either (a)
rejecting any runId containing path separators or suspicious segments (e.g.,
"..", "/", "\\"), or (b) after computing dir with path.resolve, verify that the
resolved dir startsWith the expected base directory (path.resolve(homedir(),
".ade", "perf-runs")), and throw on mismatch; apply this check before creating
eventsPath/summaryPath so aggregate rejects traversal attempts and only operates
inside the intended perf-runs directory.
In `@apps/desktop/src/preload/preload.ts`:
- Around line 941-944: The githubRemoteStatusCache created by
createShortIpcCache for IPC.githubGetRemoteStatus is global and must be cleared
when the active project changes; add calls to invalidate/clear that cache (e.g.,
githubRemoteStatusCache.clear() or the cache's provided invalidate method) from
the code paths that change the active repo—specifically after openRepo, after
switchToPath, and inside the binding-swap/active-project-change handler so the
renderer won't show stale repo/hasOrigin state.
- Around line 6645-6654: The getRemoteStatus bridge currently always calls
ipcRenderer.invoke(IPC.githubGetRemoteStatus) causing remote windows to bypass
the active project runtime; change getRemoteStatus to route through the bound
project runtime the same way getStatus does: use the active runtime (e.g.,
boundProjectRuntime or runtimeFromBoundProject) when present to invoke
IPC.githubGetRemoteStatus, preserving the githubRemoteStatusCache and
clearAround behavior on opts?.forceRefresh; if no bound runtime is available
fall back to ipcRenderer.invoke(IPC.githubGetRemoteStatus) so local mode still
works. Ensure you update the code paths that reference getRemoteStatus,
githubRemoteStatusCache, and IPC.githubGetRemoteStatus to use the bound runtime
invocation pattern.
---
Outside diff comments:
In `@apps/desktop/src/main/services/ipc/registerIpc.ts`:
- Around line 1886-1891: The helper requireSyncService currently throws when no
service exists, which leaks a generic error while some IPCs special-case
disabled mode; change requireSyncService to detect the
disabled-local-runtime-daemon state (use the existing
ADE_DISABLE_LOCAL_RUNTIME_DAEMON check or the same utility that other IPC
handlers use) and instead return a consistent "disabled" sentinel that
implements the same interface as createSyncService (or return a well-known
DisabledSyncError instance) so all callers (e.g., syncGetStatus,
syncSetActiveLanePresence, syncRefreshDiscovery, syncListDevices,
syncGetTransferReadiness, pin/connect/disconnect) can handle disabled mode
uniformly via this single shared path rather than getting the generic "Sync
service is not available." exception.
🪄 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: 5ddaf9f0-859b-4540-ac43-8e021b3190cf
⛔ Files ignored due to path filters (7)
docs/ARCHITECTURE.mdis excluded by!docs/**docs/features/files-and-editor/editor-surfaces.mdis excluded by!docs/**docs/features/lanes/README.mdis excluded by!docs/**docs/features/remote-runtime/README.mdis excluded by!docs/**docs/features/remote-runtime/internal-architecture.mdis excluded by!docs/**docs/features/terminals-and-sessions/README.mdis excluded by!docs/**docs/perf/work-tab-action-inventory.mdis excluded by!docs/**
📒 Files selected for processing (73)
.agents/skills/ade-autoresearch/SKILL.md.agents/skills/ade-perf-boot/SKILL.md.agents/skills/ade-perf-lanes/SKILL.md.agents/skills/ade-perf-work/SKILL.mdapps/ade-cli/src/multiProjectRpcServer.test.tsapps/ade-cli/src/stdioRpcDaemon.test.tsapps/desktop/src/main/main.tsapps/desktop/src/main/services/git/gitOperationsService.test.tsapps/desktop/src/main/services/git/gitOperationsService.tsapps/desktop/src/main/services/github/githubService.test.tsapps/desktop/src/main/services/github/githubService.tsapps/desktop/src/main/services/ipc/registerIpc.tsapps/desktop/src/main/services/ipc/runtimeBridge.test.tsapps/desktop/src/main/services/perf/aggregator.tsapps/desktop/src/main/services/perf/metricsSampler.tsapps/desktop/src/main/services/perf/perfIpc.tsapps/desktop/src/main/services/perf/perfLog.tsapps/desktop/src/main/services/usage/usageTrackingService.test.tsapps/desktop/src/main/services/usage/usageTrackingService.tsapps/desktop/src/preload/global.d.tsapps/desktop/src/preload/preload.test.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/components/app/App.tsxapps/desktop/src/renderer/components/app/App.workKeepAlive.test.tsxapps/desktop/src/renderer/components/app/AppShell.tsxapps/desktop/src/renderer/components/app/CommandPalette.test.tsxapps/desktop/src/renderer/components/app/TopBar.test.tsxapps/desktop/src/renderer/components/chat/AgentChatComposer.test.tsxapps/desktop/src/renderer/components/chat/AgentChatMessageList.test.tsxapps/desktop/src/renderer/components/chat/AgentChatMessageList.tsxapps/desktop/src/renderer/components/chat/AgentChatPane.companionDrawers.test.tsxapps/desktop/src/renderer/components/chat/ChatAppControlPanel.test.tsxapps/desktop/src/renderer/components/chat/ChatBuiltInBrowserPanel.test.tsxapps/desktop/src/renderer/components/chat/ChatGitToolbar.test.tsxapps/desktop/src/renderer/components/chat/ChatIosSimulatorPanel.test.tsxapps/desktop/src/renderer/components/chat/ChatIosSimulatorPanel.tsxapps/desktop/src/renderer/components/chat/ChatSubagentsPanel.test.tsxapps/desktop/src/renderer/components/chat/ChatTerminalDrawer.test.tsxapps/desktop/src/renderer/components/chat/ChatWorkLogBlock.tsxapps/desktop/src/renderer/components/chat/CursorCloudInlineLaunch.test.tsxapps/desktop/src/renderer/components/files/FilesPage.test.tsxapps/desktop/src/renderer/components/files/FilesPage.tsxapps/desktop/src/renderer/components/lanes/CommitTimeline.test.tsxapps/desktop/src/renderer/components/lanes/CommitTimeline.tsxapps/desktop/src/renderer/components/lanes/LaneDiffPane.test.tsxapps/desktop/src/renderer/components/lanes/LaneGitActionsPane.test.tsxapps/desktop/src/renderer/components/lanes/LaneGitActionsPane.tsxapps/desktop/src/renderer/components/lanes/LanesPage.test.tsapps/desktop/src/renderer/components/lanes/LanesPage.tsxapps/desktop/src/renderer/components/terminals/LaneCombobox.tsxapps/desktop/src/renderer/components/terminals/MacosVmPanel.test.tsxapps/desktop/src/renderer/components/terminals/SessionContextMenu.tsxapps/desktop/src/renderer/components/terminals/SessionListPane.test.tsxapps/desktop/src/renderer/components/terminals/SessionListPane.tsxapps/desktop/src/renderer/components/terminals/WorkSidebar.tsxapps/desktop/src/renderer/components/terminals/WorkStartSurface.test.tsxapps/desktop/src/renderer/components/terminals/WorkViewArea.test.tsxapps/desktop/src/renderer/components/terminals/WorkViewArea.tsxapps/desktop/src/renderer/components/ui/FloatingPane.tsxapps/desktop/src/renderer/lib/useGithubProjectRemote.tsapps/desktop/src/renderer/main.tsxapps/desktop/src/renderer/perf/harness/index.tsapps/desktop/src/renderer/perf/markers.tsapps/desktop/src/renderer/perf/scenarios/boot.tsapps/desktop/src/renderer/perf/scenarios/index.tsapps/desktop/src/renderer/perf/scenarios/lanes.tsapps/desktop/src/renderer/perf/webVitals.tsapps/desktop/src/renderer/state/appStore.test.tsapps/desktop/src/renderer/state/appStore.tsapps/desktop/src/shared/ipc.tsscripts/perf-launch.mjsscripts/reset-perf-pass.shscripts/run-perf-scenario.mjs
✅ Files skipped from review due to trivial changes (9)
- .agents/skills/ade-perf-boot/SKILL.md
- apps/desktop/src/main/services/perf/metricsSampler.ts
- apps/desktop/src/renderer/components/terminals/WorkViewArea.tsx
- apps/desktop/src/renderer/main.tsx
- apps/desktop/src/renderer/components/terminals/MacosVmPanel.test.tsx
- apps/desktop/src/renderer/components/lanes/CommitTimeline.test.tsx
- apps/desktop/src/renderer/components/chat/ChatTerminalDrawer.test.tsx
- .agents/skills/ade-perf-lanes/SKILL.md
- apps/desktop/src/renderer/components/lanes/LanesPage.test.ts
🚧 Files skipped from review as they are similar to previous changes (58)
- apps/desktop/src/renderer/components/app/App.workKeepAlive.test.tsx
- apps/desktop/src/renderer/components/terminals/SessionListPane.test.tsx
- apps/desktop/src/renderer/components/chat/ChatWorkLogBlock.tsx
- apps/desktop/src/renderer/lib/useGithubProjectRemote.ts
- apps/desktop/src/main/services/usage/usageTrackingService.test.ts
- apps/desktop/src/renderer/components/chat/CursorCloudInlineLaunch.test.tsx
- apps/desktop/src/renderer/components/app/TopBar.test.tsx
- scripts/reset-perf-pass.sh
- apps/desktop/src/renderer/components/terminals/LaneCombobox.tsx
- apps/desktop/src/renderer/components/chat/ChatIosSimulatorPanel.tsx
- apps/desktop/src/renderer/components/terminals/SessionContextMenu.tsx
- apps/desktop/src/renderer/perf/scenarios/boot.ts
- apps/desktop/src/main/services/git/gitOperationsService.ts
- apps/desktop/src/renderer/components/terminals/WorkViewArea.test.tsx
- apps/ade-cli/src/stdioRpcDaemon.test.ts
- apps/desktop/src/renderer/components/chat/ChatSubagentsPanel.test.tsx
- apps/desktop/src/main/services/git/gitOperationsService.test.ts
- apps/desktop/src/shared/ipc.ts
- apps/desktop/src/preload/global.d.ts
- apps/desktop/src/renderer/components/terminals/WorkSidebar.tsx
- apps/desktop/src/renderer/components/chat/ChatGitToolbar.test.tsx
- apps/desktop/src/preload/preload.test.ts
- apps/desktop/src/main/services/github/githubService.test.ts
- apps/desktop/src/renderer/components/app/App.tsx
- apps/desktop/src/renderer/components/chat/ChatAppControlPanel.test.tsx
- apps/desktop/src/renderer/components/chat/ChatBuiltInBrowserPanel.test.tsx
- apps/desktop/src/main/services/usage/usageTrackingService.ts
- apps/desktop/src/renderer/components/lanes/CommitTimeline.tsx
- apps/desktop/src/renderer/components/chat/AgentChatComposer.test.tsx
- apps/desktop/src/main/services/ipc/runtimeBridge.test.ts
- apps/desktop/src/renderer/perf/markers.ts
- apps/desktop/src/main/services/perf/perfIpc.ts
- apps/desktop/src/renderer/components/app/CommandPalette.test.tsx
- apps/desktop/src/renderer/components/files/FilesPage.tsx
- apps/desktop/src/renderer/components/lanes/LanesPage.tsx
- apps/desktop/src/renderer/components/lanes/LaneGitActionsPane.test.tsx
- apps/ade-cli/src/multiProjectRpcServer.test.ts
- apps/desktop/src/main/services/perf/perfLog.ts
- scripts/run-perf-scenario.mjs
- apps/desktop/src/renderer/components/terminals/SessionListPane.tsx
- apps/desktop/src/renderer/components/files/FilesPage.test.tsx
- apps/desktop/src/renderer/components/app/AppShell.tsx
- apps/desktop/src/renderer/components/chat/ChatIosSimulatorPanel.test.tsx
- apps/desktop/src/renderer/perf/scenarios/lanes.ts
- apps/desktop/src/renderer/perf/harness/index.ts
- apps/desktop/src/renderer/components/lanes/LaneGitActionsPane.tsx
- apps/desktop/src/renderer/components/lanes/LaneDiffPane.test.tsx
- apps/desktop/src/renderer/components/chat/AgentChatMessageList.test.tsx
- .agents/skills/ade-autoresearch/SKILL.md
- apps/desktop/src/renderer/perf/scenarios/index.ts
- apps/desktop/src/renderer/perf/webVitals.ts
- apps/desktop/src/renderer/state/appStore.ts
- apps/desktop/src/main/main.ts
- apps/desktop/src/main/services/github/githubService.ts
- apps/desktop/src/renderer/components/chat/AgentChatPane.companionDrawers.test.tsx
- .agents/skills/ade-perf-work/SKILL.md
- scripts/perf-launch.mjs
- apps/desktop/src/renderer/state/appStore.test.ts
Owner
Author
|
@cursor review |
Owner
Author
|
@copilot review but do not make fixes |
Copilot stopped work on behalf of
arul28 due to an error
May 12, 2026 10:43
Owner
Author
|
@cursor review |
Owner
Author
|
@copilot review but do not make fixes |
Copilot stopped work on behalf of
arul28 due to an error
May 12, 2026 10:53
Owner
Author
|
@copilot review but do not make fixes |
Owner
Author
|
@cursor review |
Copilot stopped work on behalf of
arul28 due to an error
May 12, 2026 11:04
Owner
Author
|
@copilot review but do not make fixes |
Owner
Author
|
@cursor review |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit d1e0e01. Configure here.
| } | ||
| if (!inOriginSection) continue; | ||
| const url = line.match(/^\s*url\s*=\s*(.+?)\s*$/); | ||
| if (url?.[1]?.trim()) return url[1].trim(); |
There was a problem hiding this comment.
Git config parser ignores inline comments in URL values
Low Severity
readOriginUrlFromConfig uses the regex (.+?)\s*$ to capture the URL value, which does not strip standard git config inline comments (lines containing ; or # after the value). For a config line like url = git@github.com:acme/repo.git ; my comment, url[1].trim() returns git@github.com:acme/repo.git ; my comment, which parseGitHubRepoFromRemoteUrl will fail to parse. The fallthrough to git remote get-url origin handles this correctly, so no incorrect results are produced, but the synchronous fast-path is defeated for any config with inline comments.
Reviewed by Cursor Bugbot for commit d1e0e01. Configure here.
| if (localRuntimeDaemonDisabled) return buildUnavailableSyncSnapshot(); | ||
| throw new Error("Sync service is not available."); | ||
| } | ||
| return await service.getStatus({ |
There was a problem hiding this comment.
Inconsistent sync handler short-circuiting when daemon disabled
Low Severity
The syncGetStatus and syncSetActiveLanePresence handlers use resolveOptionalSyncService() before checking localRuntimeDaemonDisabled, while all other sync handlers (e.g., syncRefreshDiscovery, syncListDevices, syncConnectToBrain) short-circuit with the unavailable snapshot immediately after tryLocalRuntimeSync returns null. This inconsistency means syncSetActiveLanePresence unnecessarily resolves the sync service on every call in daemon-disabled mode, and syncGetStatus will use the real sync service if available even when daemon-disabled mode should be deterministic.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit d1e0e01. Configure here.
Copilot stopped work on behalf of
arul28 due to an error
May 12, 2026 11:32
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Describe the change.
What Changed
Key files and behaviors.
Validation
How you tested.
Risks
Anything to watch.
Note
Medium Risk
Medium risk because it changes main-process IPC behavior (sync/github/project open), adds new perf logging/sampling on the main thread when enabled, and adjusts git/github detection and git stash/history handling that affect core workflows.
Overview
Adds an opt-in perf-run telemetry pipeline gated by env (
ADE_PERF_RUN_ID): main-process event logging (perfLog), IPC hooks to recordmanualStep/scenario events and finalize intosummary.json(perfIpc,aggregator), plus a 1sapp.getAppMetrics()sampler and automatic IPC invoke tracing during perf runs.Hardens local-runtime-disabled mode (
ADE_DISABLE_LOCAL_RUNTIME_DAEMON=1) by short-circuiting preload local-runtime calls/event polling and returning deterministic “sync unavailable” snapshots across sync IPC handlers instead of slow failing probes.Improves GitHub + git operations: adds lightweight
github.getRemoteStatus(remote/origin detection via direct git config parsing) to avoid full auth checks for UI elements, skips repo probes for classic PATs ingetStatus, updates stash listing to ISO timestamps and adds user-friendly “missing lane worktree” errors for commit history.Includes assorted UI/test updates supporting these flows (router hash-to-path bridge for BrowserRouter, delayed GitHub status fetch in
AppShell, bounded/streaming local usage JSONL scans, and expanded unit tests for chat transcript actions and IPC/perf helpers).Reviewed by Cursor Bugbot for commit d1e0e01. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Greptile Summary
This PR introduces an env-gated performance measurement framework (
ADE_PERF_RUN_ID) spanning main and renderer processes, hardensADE_DISABLE_LOCAL_RUNTIME_DAEMONlaunches with stable "unavailable" sync snapshots, refines GitHub origin detection by reading git config directly (including worktree support), fixes the local usage-log scan to stream files with size/count limits, and adds a broad suite of component tests.perfLog,aggregator,metricsSampler, andperfIpcservices on the main side collect JSONL events and produce asummary.jsonwith fitness/component metrics; the renderer side addsmarkers,webVitals,scenarios, and a harness that drives automated scenario runs.ADE_DISABLE_LOCAL_RUNTIME_DAEMON=1, and the local runtime event pump is skipped entirely.IPC.projectOpenRepogains an optionalrootPatharg for programmatic project switching, a newIPC.githubGetRemoteStatusendpoint exposes lightweight origin detection, and thecreateShortIpcCacherace-condition (stale update from an older in-flight request) is fixed.Confidence Score: 5/5
Safe to merge — all new perf infrastructure is fully env-gated behind ADE_PERF_RUN_ID and the production code paths look correct.
The entire perf framework only activates when ADE_PERF_RUN_ID is set, so normal app launches are unaffected. The sync-service unavailable-snapshot changes are straightforward early returns. The createShortIpcCache race-condition fix and streaming readline improvements are clear wins. The synchronous I/O concerns are real but intentional trade-offs within the controlled perf-testing context and do not affect production behavior.
apps/desktop/src/main/services/perf/perfIpc.ts and apps/desktop/src/main/services/ipc/registerIpc.ts warrant a second look for the synchronous I/O in the finalize handler and the once-captured traceIpcInvokes flag.
Important Files Changed
Sequence Diagram
sequenceDiagram participant Main as Electron Main participant IPC as IPC Layer participant Renderer as Renderer participant Harness as Perf Harness participant Disk as ~/.ade/perf-runs/id/ Main->>Main: initPerfRunFromEnv() Main->>Main: startMetricsSampler() 1Hz Main->>IPC: registerIpc() traceIpcInvokes captured once Main->>IPC: registerPerfIpcHandlers() Renderer->>IPC: perf.getConfig() IPC-->>Renderer: active runId scenario Renderer->>Harness: initPerfRuntime() Harness->>Harness: setPerfActive installWebVitalsObservers startRendererMemorySampler Harness->>IPC: perf.recordEvent rendererPerfReady Harness->>Harness: setTimeout 1500ms Harness->>IPC: perf.recordEvent scenarioStart Harness->>Renderer: scenario.run ctx loop Every IPC call during run IPC->>Disk: appendFileSync ipcInvoke event end Harness->>IPC: perf.scenarioComplete IPC->>Disk: appendFileSync scenarioEnd Harness->>IPC: perf.finalize IPC->>Main: stopMetricsSampler IPC->>Disk: aggregate readSync events writeFileSync summary IPC-->>Renderer: ok summaryComments Outside Diff (1)
apps/desktop/src/main/services/github/githubService.ts, line 68-130 (link)readOriginUrlFromGitConfigusesfs.existsSync,fs.readFileSync, andfs.statSync— all synchronous. It is called fromdetectRepoanddetectOrigin, which are the implementations of IPC handlers invoked for every project status poll. Synchronous file reads block Electron's main process event loop for every call. For the typical small git config this is invisible, but under load (many rapid status calls) or on a network-mounted filesystem it will stall all IPC processing. Consider usingfs.promisesequivalents instead.Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (7): Last reviewed commit: "Align perf run id sanitization" | Re-trigger Greptile