fix(web): show connecting state on launching agent during Auto Run launch (Gap 1 follow-up to #946) - #974
Closed
chr1syy wants to merge 9 commits into
Closed
fix(web): show connecting state on launching agent during Auto Run launch (Gap 1 follow-up to #946)#974chr1syy wants to merge 9 commits into
chr1syy wants to merge 9 commits into
Conversation
Adds parity with desktop's WorktreeRunSection so mobile users can dispatch an Auto Run to a separate git worktree, optionally creating a PR on completion. - New WS handlers `get_git_branches` and `list_worktrees` (SSH-aware via the session's `sessionSshRemoteConfig`). - Surfaces `isGitRepo` + `worktreeBasePath` on `SessionData` so mobile can gate the section and compute the worktree path preview. - Extends `useAutoRun.launchAutoRun` to forward the optional `worktree` payload through `configure_auto_run`, and adds `loadGitBranches` / `listWorktrees` helpers. - New `AutoRunWorktreeSection` mobile component (toggle, base-branch picker, sanitized branch input + path preview, "create PR on completion" checkbox), wired into `AutoRunSetupSheet` above the Prompt section. - Tests: handler validation/payload shape, hook payload threading, and section rendering / config emission. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CI failure fix:
- Add `setGetGitBranchesForSessionCallback` / `setListWorktreesForSessionCallback`
to the WebServer mock in `web-server-factory.test.ts` so the factory's
callback registration doesn't blow up the suite.
Review feedback (greptile + coderabbit):
- `messageHandlers.ts`: defensively coerce non-Error rejections in the two new
handlers (`error instanceof Error ? error.message : String(error)`).
- `web-server-factory.ts`: extract `resolveSessionGitContext()` helper to
deduplicate SSH resolution and drop the no-op `|| undefined`. Stop
swallowing exec/SSH failures into empty-result success — let them
propagate so Sentry sees real infra/SSH regressions; only `exitCode !== 0`
(legitimately "not a git repo") still maps to empty results.
- `useAutoRun.ts`: `loadGitBranches` / `listWorktrees` no longer swallow
transport errors; let rejections propagate so the section can render an
actual error state.
- `AutoRunWorktreeSection.tsx`: log caught exceptions via `webLogger.error`,
replace `branchLoadError` boolean with a `branchLoadStatus`
('idle' | 'loading' | 'error') tri-state so the picker shows
"Failed to load" instead of a stuck "Loading…" after a fetch failure.
- `AutoRunWorktreeSection.tsx` + `AutoRunSetupSheet.tsx`: `onChange` now
emits a discriminated `AutoRunWorktreeState` union
('disabled' | 'enabled-valid' | 'enabled-invalid'). The sheet blocks
launch and shows an inline warning when status is 'enabled-invalid' —
prevents an enabled-but-invalid worktree config from silently falling
through to a normal Auto Run on the main checkout.
Tests:
- Updated `AutoRunWorktreeSection.test.tsx` to assert the new state union
and added a "Failed to load" placeholder regression test.
- Updated `useAutoRun.test.ts` to assert the new error-propagation contract
for `loadGitBranches` / `listWorktrees`.
Scoped vitest: 196 pass / 0 fail. Lint, prettier, type-check clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Resolves conflicts where Run-in-Worktree extensions and the upstream Auto Run parity work both touched the same surfaces: - src/main/web-server/types.ts (SessionData / SessionBroadcastData) - src/main/web-server/web-server-factory.ts (sessions emit + new git callbacks) - src/web/hooks/useAutoRun.ts (LaunchConfig, exports, state shape) - src/web/hooks/useWebSocket.ts (SessionData) - src/web/mobile/App.tsx (useAutoRun destructuring + sheet props wiring) - src/web/mobile/AutoRunSetupSheet.tsx (imports, props, reset effect, layout) All conflicts resolved by keeping both sides' fields/handlers — Run-in- Worktree props live alongside upstream's playbook/error-recovery wiring. Also addresses one new review comment on commit 9ce0e33: - web-server-factory.ts:resolveSessionGitContext now fails loudly when SSH is enabled but the configured remote can't be resolved instead of silently falling back to local git (would have leaked the run to the wrong machine for SSH-backed sessions). Updated the App.test.tsx useAutoRun mock to expose the playbook / error-recovery / worktree methods that App.tsx now destructures. Scoped vitest: 188 pass / 0 fail. Lint, prettier, type-check clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…t spawn failures Addresses two CodeRabbit review comments on f304407: - messageHandlers.ts (handleGetGitBranches / handleListWorktrees): the new `.catch` blocks were calling `this.sendError` directly, which surfaces a websocket error to the client but never reaches Sentry. Routed both through `reportHandlerError(...)` so production diagnostics see SSH/git regressions; the user-facing message and shape are unchanged. - web-server-factory.ts (getGitBranchesForSession / listWorktreesForSession): `execGit` returns `exitCode: number | string`, where a string code (ENOENT, EPERM, …) means git never ran. The previous fallback collapsed those failures into `{ branches: [] }` / `{ worktrees: [] }`, making broken local/SSH execution look like a healthy empty repo. Now we explicitly throw on a non-numeric exitCode so the real cause propagates and only numeric non-zero codes (legitimate "not a git repo") still map to empty results. Scoped vitest: 188 pass / 0 fail. Lint, prettier, type-check clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… AutoRun Mobile/web AutoRun's "Dispatch to a separate worktree" was relying on the chokidar watcher in useWorktreeHandlers to discover the new on-disk worktree and attach it to a parent. The watcher's find-first heuristic attached the child to whichever sibling agent's worktreeConfig.basePath matched first, producing the wrong-parent attachment reported in PR RunMaestro#946. Extract spawnWorktreeAgentAndDispatch into a shared util and call it from the remote configureAutoRun handler, mirroring desktop's useAutoRunHandlers parent resolution. The remote path now spawns the child session explicitly with the correct parentSessionId before startBatchRun runs, and forwards worktreeTarget so startBatchRun skips its redundant setupWorktree. Also closes Greptile P1: AutoRunWorktreeSection now emits enabled-invalid when createPROnCompletion is true but baseBranch is still empty (race window between loadBranches resolution and the user toggling PR creation). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- useAppRemoteEventListeners: stop pre-populating batchConfig.worktree with the raw mobile payload. The user-typed branch name and computed path can drift from the values spawnWorktreeAgentAndDispatch resolves (sanitized branch, or an existingPath returned by `git worktree add`). Mirror the desktop launch shape: leave batchConfig.worktree undefined unless the spawn helper writes it back (createPROnCompletion=true), rely on worktreeTarget + the spawned session's cwd otherwise. - worktreeSpawn: drop the dead `!branchName && gitBranches[0]` fallback (branchName is always set before that point) and remove the duplicate Sentry capture from the gitService.getBranches call — the IPC wrapper already reports failures via captureException. Keep a bare try/catch to preserve the non-fatal contract. - AutoRunWorktreeSection: gate the "Branch name is required" warning on branchLoadStatus !== 'loading' so it doesn't flash before the seeded default arrives. - AutoRunWorktreeSection.test: wrap the trailing `resolveBranches!` resolution in act() so its component state updates are inside React's act boundary. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…unch Gap 1 follow-up: on web/mobile, the launching agent's status indicator stayed green throughout an Auto Run launch, leaving users without any visual confirmation. Switch launchAutoRun to use sendRequest so the caller can await the configure_auto_run_result, optimistically flip the session to 'connecting' (pulsing orange) before sending, and revert if the server reports failure. Subsequent session_state_change broadcasts overwrite the optimistic value as the agent transitions to busy. Also exposes setLocalSessionState on useSessions so other consumers can drive the same optimistic-update pattern. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds run-in-worktree Auto Run: new git/worktree types and WebSocket handlers, main-process git callbacks and parsing, a shared spawnWorktreeAgentAndDispatch utility, hook and renderer wiring (async launch, optimistic session state), a mobile AutoRunWorktreeSection UI, and tests. ChangesRun-in-Worktree Auto Run Feature
Sequence Diagram(s)sequenceDiagram
participant MobileUI
participant useAutoRun
participant WebSocket
participant WebServer
participant CallbackRegistry
MobileUI->>useAutoRun: loadGitBranches / listWorktrees
useAutoRun->>WebSocket: sendRequest get_git_branches / list_worktrees
WebSocket->>WebServer: forward request
WebServer->>CallbackRegistry: getGitBranchesForSession / listWorktreesForSession
CallbackRegistry-->>WebServer: GitBranchesResult / ListWorktreesResult
WebServer-->>WebSocket: response
WebSocket-->>useAutoRun: result
useAutoRun-->>MobileUI: branches / worktrees
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR brings web/mobile's Auto Run launch indicator to parity with desktop by flipping the launching session's
Confidence Score: 3/5Safe for non-worktree launches; worktree-enabled launches on slow repos or SSH remotes will time out and falsely report failure on the mobile client while the desktop continues running the batch. The new src/web/hooks/useAutoRun.ts (timeout on Important Files Changed
Sequence DiagramsequenceDiagram
participant Web as Mobile/Web Client
participant WS as WebServer (main)
participant Renderer as Desktop Renderer
participant Git as Git (local/SSH)
Web->>Web: setLocalState(sessionId, 'connecting')
Web->>WS: "configure_auto_run {launch:true, worktree?, requestId}"
WS->>Renderer: IPC: configure_auto_run event
alt worktree enabled
Renderer->>Git: git worktree add (can be slow)
Git-->>Renderer: worktree created
Renderer->>Git: getBranches(worktreePath)
Git-->>Renderer: branches
Renderer->>Renderer: spawnWorktreeAgentAndDispatch()
end
Renderer->>WS: "sendRemoteConfigureAutoRunResponse {success: true}"
WS->>Web: "configure_auto_run_result {success, requestId}"
alt success
Web->>Web: keep 'connecting' state
Renderer->>Web: session_state_change to busy
else failure or timeout
Web->>Web: revert state to previousState
end
Renderer->>Renderer: startBatchRun() fire-and-forget
Reviews (1): Last reviewed commit: "fix(web): show connecting state on launc..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.github.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/renderer/hooks/batch/useAutoRunHandlers.ts (1)
279-295:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReport unexpected worktree spawn failures to Sentry.
spawnWorktreeAgentAndDispatchalready usesnullfor expected failures, so anything that lands in thiscatchis the unexpected path. Right now it is only logged/toasted and then returned, which loses the error in production.As per coding guidelines: "Do not silently swallow errors. Let unhandled exceptions bubble up to Sentry for error tracking in production."🛠️ Suggested fix
+import { captureException } from '../../utils/sentry'; ... try { const newSessionId = await spawnWorktreeAgentAndDispatch(parentForSpawn, config); if (!newSessionId) return; // Error already shown via toast targetSessionId = newSessionId; } catch (err) { + captureException(err, { + extra: { + parentSessionId: parentForSpawn.id, + worktreeTarget: config.worktreeTarget, + }, + }); window.maestro.logger.log( 'error', `Failed to spawn worktree agent: ${err instanceof Error ? err.message : String(err)}`, 'AutoRunHandlers' );🤖 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 `@src/renderer/hooks/batch/useAutoRunHandlers.ts` around lines 279 - 295, The catch block around spawnWorktreeAgentAndDispatch swallows unexpected errors (logs/toasts then returns), losing them for production error tracking; instead, after logging and notifyToast, rethrow the caught error so it bubbles to the global error handler/Sentry (or explicitly call your app's Sentry capture function if available) — update the catch in useAutoRunHandlers (the block that sets targetSessionId from spawnWorktreeAgentAndDispatch and calls window.maestro.logger.log and notifyToast) to not return silently but propagate the error.
🧹 Nitpick comments (2)
src/main/web-server/web-server-factory.ts (1)
1605-1701: ⚡ Quick winAdd focused coverage for the new git/worktree callbacks.
This path now carries the SSH resolution and the “numeric git exit => empty result, non-numeric exec failure => throw” split, but the factory test only adds setter stubs. A couple of direct assertions around the registered callbacks would catch regressions in the mobile Auto Run worktree flow.
🤖 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 `@src/main/web-server/web-server-factory.ts` around lines 1605 - 1701, Add unit tests that exercise the registered callbacks instead of only verifying setters: create a fake sessionsStore entry and invoke the callback retrieved from server.setGetGitBranchesForSessionCallback to assert execGit/resolveSessionGitContext are used and that non-numeric execGit.exitCode throws while numeric non-zero returns empty branches (use parseGitBranches path for zero exit); likewise invoke the callback registered via server.setListWorktreesForSessionCallback to assert non-numeric exitCode throws and numeric non-zero returns an empty worktrees array while exitCode=0 parses stdout into worktree items. Reference the setter registration functions (server.setGetGitBranchesForSessionCallback, server.setListWorktreesForSessionCallback), execGit, resolveSessionGitContext, parseGitBranches and sessionsStore to locate and wire the tests.src/__tests__/web/mobile/App.test.tsx (1)
631-655: ⚡ Quick winNo test coverage for the optimistic
connectingstate — the PR's core feature.The mock is correctly updated to match the expanded
useAutoRuncontract, but the file adds no tests that exercise the App-level behaviour introduced by this PR:
- Happy path:
launchAutoRunis called → the launching session's local state is optimistically set toconnectingbefore the server responds.- Failure path:
launchAutoRunresolves with{ success: false }→ the session state reverts to its pre-launch value.- Overwrite path: a subsequent
session_state_changebroadcast (e.g.busy) replaces the optimistic state.Because this optimistic update lives in
App.tsx(not in the hook or message handlers), it can only be regression-tested here.useAutoRun.test.ts/useSessions.test.ts/messageHandlers.test.tswill not catch a bug whereApp.tsxforgets to callsetLocalSessionStateor reverts unconditionally.✅ Suggested test stubs
+ describe('auto-run launch — optimistic connecting state', () => { + it('sets session state to connecting before launchAutoRun resolves', async () => { + // Arrange: expose a deferred promise so we can check state mid-flight + let resolveLaunch!: (v: { success: boolean }) => void; + const launchPromise = new Promise<{ success: boolean }>((r) => { resolveLaunch = r; }); + const { useAutoRun } = await import('../../../web/hooks/useAutoRun'); + (useAutoRun as ReturnType<typeof vi.fn>).mockReturnValueOnce({ + .../* spread existing mock fields */, + launchAutoRun: vi.fn().mockReturnValue(launchPromise), + }); + + render(<MobileApp />); + await act(async () => { + mockHandlers.onSessionsUpdate?.([createMockSession({ id: 'session-1', state: 'idle' })]); + }); + + // Act: trigger launch (via the real UI path or a mock handler) + // ... trigger Auto Run launch here ... + + // Assert: session state should be 'connecting' before server responds + // expect(screen.getByTestId('autorun-indicator')).toHaveAttribute('data-state', 'connecting'); + + // Resolve and confirm state transitions onward + await act(async () => { resolveLaunch({ success: true }); }); + }); + + it('reverts session state when launchAutoRun reports failure', async () => { + const { useAutoRun } = await import('../../../web/hooks/useAutoRun'); + (useAutoRun as ReturnType<typeof vi.fn>).mockReturnValueOnce({ + .../* spread existing mock fields */, + launchAutoRun: vi.fn().mockResolvedValue({ success: false }), + }); + + render(<MobileApp />); + await act(async () => { + mockHandlers.onSessionsUpdate?.([createMockSession({ id: 'session-1', state: 'idle' })]); + }); + + // Act: trigger launch and await + // Assert: session state reverts to 'idle' + }); + });🤖 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 `@src/__tests__/web/mobile/App.test.tsx` around lines 631 - 655, Tests are missing for the optimistic "connecting" session state implemented in App.tsx; add tests in src/__tests__/web/mobile/App.test.tsx that mock useAutoRun (already updated) and exercise App-level behavior: (1) simulate calling launchAutoRun and assert that setLocalSessionState (or the visible session state in the UI) is optimistically set to "connecting" before the mocked launchAutoRun resolves, (2) simulate launchAutoRun resolving to { success: false } and assert the session state reverts to the prior value, and (3) emit a session_state_change broadcast (e.g. "busy") after launching and assert that the broadcasted state overwrites the optimistic "connecting" state; reference the useAutoRun mock and App.tsx handlers that call setLocalSessionState/session_state_change to locate where to trigger and assert these behaviors.
🤖 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 `@src/renderer/hooks/remote/useAppRemoteEventListeners.ts`:
- Around line 479-516: The catch block in useAppRemoteEventListeners handling
spawnWorktreeAgentAndDispatch only logs/toasts and returns; add an explicit
Sentry report using captureException (imported from src/utils/sentry.ts) so
unexpected thrown errors are captured with context before sending the IPC
failure: call captureException(err, { extra: { responseChannel, spawnConfig,
batchConfig, targetSessionId } }) (or captureMessage when err is not an Error)
immediately inside the catch, then continue the existing logger.error,
notifyToast, and window.maestro.process.sendRemoteConfigureAutoRunResponse flow.
In `@src/renderer/utils/worktreeSpawn.ts`:
- Around line 64-67: The code builds basePath and worktreePath assuming POSIX
slashes; change to use Node's path utilities so Windows separators are handled:
use path.dirname(parentSession.cwd) instead of the regex to strip last segment,
derive basePath via parentSession.worktreeConfig?.basePath ||
path.join(path.dirname(parentSession.cwd), 'worktrees'), then set worktreePath =
path.join(basePath, branchName) and ensure any later splitting of worktreePath
(e.g., for existing-closed checks) uses path.sep or path.normalize so Windows
paths split correctly; update the code around the symbols parentSession.cwd,
parentSession.worktreeConfig, basePath, worktreePath, and branchName (also apply
the same change to the other occurrence referencing these variables).
In `@src/web/hooks/useAutoRun.ts`:
- Around line 223-247: The catch in launchAutoRun currently swallows all
exceptions; import captureException (and optionally captureMessage) from
src/utils/sentry.ts and update the catch block in useAutoRun.ts so you first
report the error with captureException(error, { extra: { sessionId, config } }),
then handle expected/recoverable errors explicitly (e.g., check error.message or
a known error code like 'NETWORK_ERROR' and return { success: false, error: ...
} only for those), and re-throw unexpected errors so they bubble to Sentry/upper
layers; keep the function name launchAutoRun and the sendRequest call intact
while adding the reporting and conditional re-throw logic.
In `@src/web/mobile/App.tsx`:
- Around line 1604-1616: The optimistic "connecting" update currently only
mutates the session object via setSessions (using sessionId), but the UI header
reads activeTab.state and only animates 'busy', so update the same source the
header uses: when you setSessions for the launching session (in the block
referencing previousState and sessionId), also update the active tab state
(activeTab.state) or, better, route the change through the local session-state
helper used elsewhere (e.g., call the helper that centralizes session <-> tab
state) so the header sees 'connecting'; additionally ensure the header/status
logic treats 'connecting' as an animated state (equivalent to 'busy') so the
pulsing indicator appears.
In `@src/web/mobile/AutoRunSetupSheet.tsx`:
- Around line 1382-1393: The worktree warning div (conditional on
worktreeState.status === 'enabled-invalid') is currently rendered inside the
horizontal action/footer row so it compresses the Cancel/Launch buttons on
narrow screens; move that conditional block out of the footer/action row
container (or change the footer container to a column flex) so the warning
appears above the buttons as a full-width banner. Locate the conditional using
worktreeState in AutoRunSetupSheet (the JSX block that renders Run-in-Worktree:
{worktreeState.reason}) and either relocate it to render before the
footer/action row or change the footer's style from row to column to avoid
squeezing the buttons. Ensure styles (fontSize, color, marginBottom, textAlign)
are preserved after the move.
In `@src/web/mobile/AutoRunWorktreeSection.tsx`:
- Around line 153-176: The effect in AutoRunWorktreeSection is emitting
"enabled-invalid" for an empty newBranchName while branches are still loading;
update the useEffect to short-circuit when branchLoadStatus === 'loading' (or
treat loading as a distinct pending state) before any branch-name validation so
it does not call onChange({ status: 'enabled-invalid', reason: ... }) for
newBranchName.trim() while loadBranches is in progress; ensure this early return
appears before the branchClean check and before the createPR/baseBranch guard so
functions/variables like useEffect, branchLoadStatus, onChange, newBranchName,
createPR and baseBranch are used as-is.
---
Outside diff comments:
In `@src/renderer/hooks/batch/useAutoRunHandlers.ts`:
- Around line 279-295: The catch block around spawnWorktreeAgentAndDispatch
swallows unexpected errors (logs/toasts then returns), losing them for
production error tracking; instead, after logging and notifyToast, rethrow the
caught error so it bubbles to the global error handler/Sentry (or explicitly
call your app's Sentry capture function if available) — update the catch in
useAutoRunHandlers (the block that sets targetSessionId from
spawnWorktreeAgentAndDispatch and calls window.maestro.logger.log and
notifyToast) to not return silently but propagate the error.
---
Nitpick comments:
In `@src/__tests__/web/mobile/App.test.tsx`:
- Around line 631-655: Tests are missing for the optimistic "connecting" session
state implemented in App.tsx; add tests in src/__tests__/web/mobile/App.test.tsx
that mock useAutoRun (already updated) and exercise App-level behavior: (1)
simulate calling launchAutoRun and assert that setLocalSessionState (or the
visible session state in the UI) is optimistically set to "connecting" before
the mocked launchAutoRun resolves, (2) simulate launchAutoRun resolving to {
success: false } and assert the session state reverts to the prior value, and
(3) emit a session_state_change broadcast (e.g. "busy") after launching and
assert that the broadcasted state overwrites the optimistic "connecting" state;
reference the useAutoRun mock and App.tsx handlers that call
setLocalSessionState/session_state_change to locate where to trigger and assert
these behaviors.
In `@src/main/web-server/web-server-factory.ts`:
- Around line 1605-1701: Add unit tests that exercise the registered callbacks
instead of only verifying setters: create a fake sessionsStore entry and invoke
the callback retrieved from server.setGetGitBranchesForSessionCallback to assert
execGit/resolveSessionGitContext are used and that non-numeric execGit.exitCode
throws while numeric non-zero returns empty branches (use parseGitBranches path
for zero exit); likewise invoke the callback registered via
server.setListWorktreesForSessionCallback to assert non-numeric exitCode throws
and numeric non-zero returns an empty worktrees array while exitCode=0 parses
stdout into worktree items. Reference the setter registration functions
(server.setGetGitBranchesForSessionCallback,
server.setListWorktreesForSessionCallback), execGit, resolveSessionGitContext,
parseGitBranches and sessionsStore to locate and wire the tests.
🪄 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: 8552071d-1ef8-4d7c-85b3-9b16450d930a
📒 Files selected for processing (20)
src/__tests__/main/web-server/handlers/messageHandlers.test.tssrc/__tests__/main/web-server/web-server-factory.test.tssrc/__tests__/web/hooks/useAutoRun.test.tssrc/__tests__/web/hooks/useSessions.test.tssrc/__tests__/web/mobile/App.test.tsxsrc/__tests__/web/mobile/AutoRunWorktreeSection.test.tsxsrc/main/web-server/WebServer.tssrc/main/web-server/handlers/messageHandlers.tssrc/main/web-server/managers/CallbackRegistry.tssrc/main/web-server/types.tssrc/main/web-server/web-server-factory.tssrc/renderer/hooks/batch/useAutoRunHandlers.tssrc/renderer/hooks/remote/useAppRemoteEventListeners.tssrc/renderer/utils/worktreeSpawn.tssrc/web/hooks/useAutoRun.tssrc/web/hooks/useSessions.tssrc/web/hooks/useWebSocket.tssrc/web/mobile/App.tsxsrc/web/mobile/AutoRunSetupSheet.tsxsrc/web/mobile/AutoRunWorktreeSection.tsx
Collaborator
|
@chr1syy thanks for the quick follow-up on Gap 1 — the optimistic-state pattern is the right approach and the test coverage is appreciated. A few concerns I'd like addressed before approving, mostly echoing the bot reviewers but flagging the ones that actually matter for this codebase:
The remaining bot comments (Windows path separators in Note this branch is stacked on #946 and the diff currently shows both branches' commits, so I'm scoping my review to the new commit on top. Once #946 lands, this should auto-rebase. |
…h indicator
- useAutoRun.ts: extend the configure_auto_run timeout to 60s when
worktree dispatch is enabled (greptile P1). Worktree launches block
on `git worktree add` plus an upstream getBranches round-trip on the
server, which routinely exceeds the default 10s and surfaces as a
spurious "Request timed out" failure.
- useAutoRun.ts: log unexpected launch failures via webLogger before
converting them into `{ success: false }` (CodeRabbit). The web bundle
has no Sentry wired up, so the logger is the equivalent surface.
- mobile/App.tsx: read the pre-launch session state from the current
sessions snapshot before scheduling the optimistic update instead of
capturing it as a side effect inside the `setSessions` updater
(greptile P2). React 18 Concurrent Mode is allowed to re-run updater
callbacks for speculative/interrupted renders.
- mobile/App.tsx: header indicator now prefers the session-level
`connecting` state over the active tab's state, and the dot animates
on both `busy` and `connecting` so the launching agent's pulsing
orange feedback actually pulses (CodeRabbit).
Note: the remaining four review comments target files introduced by
PR RunMaestro#946 (`useAppRemoteEventListeners.ts`, `worktreeSpawn.ts`,
`AutoRunSetupSheet.tsx`, `AutoRunWorktreeSection.tsx`) and belong with
that PR rather than this Gap 1 follow-up.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…amini + bots)
Maintainer review (pedramamini):
- useAutoRun.ts: re-throw unknown errors instead of swallowing them.
Per CLAUDE.md → Error Handling & Sentry, only known transport
rejections ("Request timed out", "WebSocket not connected") are
caught and converted into `{ success: false }`; anything else
re-throws so unhandled-rejection / Sentry surfaces capture it.
- mobile/App.tsx: wrap launchAutoRun in handleAutoRunLaunch with a
try/catch that reverts the optimistic indicator before re-throwing
the rejection so the UI never gets stuck on "connecting".
Bot follow-ups requested by pedramamini ("the remaining bot comments
look legit too — please give them a pass while you're in there"):
- useAppRemoteEventListeners.ts: captureException in the worktree
spawn catch with launch context (sessionId, parentSessionId,
worktree, responseChannel) so unexpected throws surface to Sentry.
- worktreeSpawn.ts: handle Windows path separators when deriving the
fallback basePath and when extracting the branchName from an
existing-closed worktree path. Replace POSIX-only regexes with
separator-agnostic ones (`[\\/]` and `[\\/][^\\/]+$`).
- AutoRunSetupSheet.tsx: pull the "Run-in-Worktree: <reason>" warning
out of the Cancel/Launch row so it renders as a full-width banner
above the action row instead of squeezing the buttons on narrow
screens.
- AutoRunWorktreeSection.tsx: add an `enabled-loading` state to the
AutoRunWorktreeState union and emit it while branches are still
loading (or in the brief gap between toggle-on and the load
effect's `setBranchLoadStatus('loading')` committing). Detection
uses `branches.length === 0 && branchLoadStatus !== 'error'` so
the transient pre-load window is also covered. The parent sheet
treats `enabled-loading` the same as `enabled-invalid` for the
Launch button (disabled) but suppresses the invalid-config banner.
Tests:
- useAutoRun.test.ts: rename the timeout/disconnect case to spell
out the known-transport-error contract, add a WebSocket-not-
connected case, and add a "re-throws unexpected errors" case.
- AutoRunWorktreeSection.test.tsx: rewrite the createPR-with-empty-
baseBranch race test to assert the new `enabled-loading` behavior
and that no `enabled-invalid` flash fires during the loading
window. The original guarantee — never emitting `enabled-valid`
with `prTargetBranch: ''` — still holds.
Scoped tests pass: 95 (useAutoRun 12 + useSessions 77 + worktree
section 6 + new). ESLint/Prettier/tsc clean for touched files.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/__tests__/web/mobile/AutoRunWorktreeSection.test.tsx (1)
192-198: ⚡ Quick winConsider initializing
resolveBranchesto avoid non-null assertion.The deferred assignment pattern requires a non-null assertion on line 248. A clearer approach would initialize the variable or restructure to make TypeScript happy without
!.♻️ Proposed refactoring
- let resolveBranches: (value: { branches: string[]; currentBranch?: string }) => void; + let resolveBranches: (value: { branches: string[]; currentBranch?: string }) => void = () => {}; const slowLoadBranches = vi.fn( () => new Promise<{ branches: string[]; currentBranch?: string }>((resolve) => { resolveBranches = resolve; }) );Then remove the
!on line 248:await act(async () => { - resolveBranches!({ branches: ['main'], currentBranch: 'main' }); + resolveBranches({ branches: ['main'], currentBranch: 'main' }); });🤖 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 `@src/__tests__/web/mobile/AutoRunWorktreeSection.test.tsx` around lines 192 - 198, The test uses a deferred Promise with resolveBranches declared but later non-null asserted; to fix, initialize resolveBranches with a typed no-op so TypeScript won't require the `!` (e.g., set resolveBranches to a function matching the signature { branches: string[]; currentBranch?: string } => void) before creating slowLoadBranches, then remove the non-null assertion where resolveBranches is invoked; reference the variables resolveBranches and slowLoadBranches in the test to locate the change.
🤖 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.
Nitpick comments:
In `@src/__tests__/web/mobile/AutoRunWorktreeSection.test.tsx`:
- Around line 192-198: The test uses a deferred Promise with resolveBranches
declared but later non-null asserted; to fix, initialize resolveBranches with a
typed no-op so TypeScript won't require the `!` (e.g., set resolveBranches to a
function matching the signature { branches: string[]; currentBranch?: string }
=> void) before creating slowLoadBranches, then remove the non-null assertion
where resolveBranches is invoked; reference the variables resolveBranches and
slowLoadBranches in the test to locate the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e7a5d820-d4c0-40a6-b6e0-b66284870e2a
📒 Files selected for processing (8)
src/__tests__/web/hooks/useAutoRun.test.tssrc/__tests__/web/mobile/AutoRunWorktreeSection.test.tsxsrc/renderer/hooks/remote/useAppRemoteEventListeners.tssrc/renderer/utils/worktreeSpawn.tssrc/web/hooks/useAutoRun.tssrc/web/mobile/App.tsxsrc/web/mobile/AutoRunSetupSheet.tsxsrc/web/mobile/AutoRunWorktreeSection.tsx
🚧 Files skipped from review as they are similar to previous changes (7)
- src/renderer/hooks/remote/useAppRemoteEventListeners.ts
- src/web/mobile/AutoRunSetupSheet.tsx
- src/renderer/utils/worktreeSpawn.ts
- src/web/mobile/App.tsx
- src/tests/web/hooks/useAutoRun.test.ts
- src/web/mobile/AutoRunWorktreeSection.tsx
- src/web/hooks/useAutoRun.ts
pedramamini
pushed a commit
that referenced
this pull request
May 10, 2026
- useAutoRun.ts: extend the configure_auto_run timeout to 60s when
worktree dispatch is enabled (greptile P1). Worktree launches block
on `git worktree add` plus an upstream getBranches round-trip on the
server, which routinely exceeds the default 10s and surfaces as a
spurious "Request timed out" failure.
- useAutoRun.ts: log unexpected launch failures via webLogger before
converting them into `{ success: false }` (CodeRabbit). The web bundle
has no Sentry wired up, so the logger is the equivalent surface.
- mobile/App.tsx: read the pre-launch session state from the current
sessions snapshot before scheduling the optimistic update instead of
capturing it as a side effect inside the `setSessions` updater
(greptile P2). React 18 Concurrent Mode is allowed to re-run updater
callbacks for speculative/interrupted renders.
- mobile/App.tsx: header indicator now prefers the session-level
`connecting` state over the active tab's state, and the dot animates
on both `busy` and `connecting` so the launching agent's pulsing
orange feedback actually pulses (CodeRabbit).
Note: the remaining four review comments target files introduced by
PR #946 (`useAppRemoteEventListeners.ts`, `worktreeSpawn.ts`,
`AutoRunSetupSheet.tsx`, `AutoRunWorktreeSection.tsx`) and belong with
that PR rather than this Gap 1 follow-up.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
pedramamini
pushed a commit
that referenced
this pull request
May 10, 2026
Maintainer review (pedramamini):
- useAutoRun.ts: re-throw unknown errors instead of swallowing them.
Per CLAUDE.md → Error Handling & Sentry, only known transport
rejections ("Request timed out", "WebSocket not connected") are
caught and converted into `{ success: false }`; anything else
re-throws so unhandled-rejection / Sentry surfaces capture it.
- mobile/App.tsx: wrap launchAutoRun in handleAutoRunLaunch with a
try/catch that reverts the optimistic indicator before re-throwing
the rejection so the UI never gets stuck on "connecting".
Bot follow-ups requested by pedramamini ("the remaining bot comments
look legit too — please give them a pass while you're in there"):
- useAppRemoteEventListeners.ts: captureException in the worktree
spawn catch with launch context (sessionId, parentSessionId,
worktree, responseChannel) so unexpected throws surface to Sentry.
- worktreeSpawn.ts: handle Windows path separators when deriving the
fallback basePath and when extracting the branchName from an
existing-closed worktree path. Replace POSIX-only regexes with
separator-agnostic ones (`[\\/]` and `[\\/][^\\/]+$`).
- AutoRunSetupSheet.tsx: pull the "Run-in-Worktree: <reason>" warning
out of the Cancel/Launch row so it renders as a full-width banner
above the action row instead of squeezing the buttons on narrow
screens.
- AutoRunWorktreeSection.tsx: add an `enabled-loading` state to the
AutoRunWorktreeState union and emit it while branches are still
loading (or in the brief gap between toggle-on and the load
effect's `setBranchLoadStatus('loading')` committing). Detection
uses `branches.length === 0 && branchLoadStatus !== 'error'` so
the transient pre-load window is also covered. The parent sheet
treats `enabled-loading` the same as `enabled-invalid` for the
Launch button (disabled) but suppresses the invalid-config banner.
Tests:
- useAutoRun.test.ts: rename the timeout/disconnect case to spell
out the known-transport-error contract, add a WebSocket-not-
connected case, and add a "re-throws unexpected errors" case.
- AutoRunWorktreeSection.test.tsx: rewrite the createPR-with-empty-
baseBranch race test to assert the new `enabled-loading` behavior
and that no `enabled-invalid` flash fires during the loading
window. The original guarantee — never emitting `enabled-valid`
with `prTargetBranch: ''` — still holds.
Scoped tests pass: 95 (useAutoRun 12 + useSessions 77 + worktree
section 6 + new). ESLint/Prettier/tsc clean for touched files.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collaborator
|
Cherry-picked the three review-feedback commits (8369c46, fd6efdc, c8b8118) directly onto Now on rc:
Author attribution preserved. Closing in favor of those commits. |
8 tasks
Collaborator
|
@chr1syy heads up — closed in favor of cherry-picks landed directly on |
4 tasks
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
2 participants
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
Follow-up to #946. Addresses Gap 1 from the post-review notes: https://gist.github.com/chr1syy/67630166ff5217a97d7b1fad91201f3b
On web/mobile, the launching agent's status indicator stayed green throughout an Auto Run launch (with or without the worktree toggle from #946), giving users no visual confirmation that the launch actually fired. Desktop already flips the indicator to busy/connecting during the spawn — this brings web/mobile to parity.
This PR is stacked on top of #946. Until #946 merges, the diff here will include both branches' commits; after #946 merges into `rc`, this PR auto-rebases and shows only the single follow-up commit.
Changes
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests