Work-tab status truthfulness: CLI status in Live Activity, attention capsules, lane PR badges, instant webhook PR freshness#707
Conversation
|
@copilot review but do not make fixes |
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
arul28
changed the title
📝 WalkthroughWalkthroughAdds CLI runtime-signal handling and PR-state refresh wiring, introduces shared session canonical-state helpers for desktop and iOS, shows lane PR badges, updates Work session UI behavior, and embeds an ADE icon in the pairing QR code. ChangesCLI runtime signal support
PR polling refresh on ingress
Desktop session attention state
Lane PR badges
iOS work session state and lane PR navigation
Pairing QR rendering update
Estimated code review effort: 4 (Complex) | ~40 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/ade-cli/src/services/push/pushPublisherService.ts (1)
64-64: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMake
PushCliRuntimeSignal.runtimeStateuse the shared terminal-state union.onCliRuntimeSignalcollapses every non-"waiting-input"/ non-terminal value into"running", so a typo or future PTY state is indistinguishable from an intentional update. ReusingTerminalRuntimeStatehere and handling"idle"explicitly would keep this mapping aligned withptyService.🤖 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/ade-cli/src/services/push/pushPublisherService.ts` at line 64, The PushCliRuntimeSignal.runtimeState field is too broad and should use the shared TerminalRuntimeState union instead of string. Update PushCliRuntimeSignal and the onCliRuntimeSignal handling in PushPublisherService so the runtime-state mapping stays aligned with ptyService, and make sure "idle" is handled explicitly rather than being collapsed into the generic running state.
🤖 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/ade-cli/src/services/push/pushPublisherService.test.ts`:
- Around line 336-354: The test is asserting exclusions by iterating over
publish.mock.calls, but with only a chat-owned shell and an unknown session the
tracking is filtered out before any publish payload is produced. Update the test
around pushPublisherService.test.ts and the
handleCliRuntimeSignal/resolveMissingMeta flow to assert the real outcome
instead: either verify publish was never called, or add a separate active run so
the flush produces a payload that can actually be checked for excluding shell-1
and ghost-1.
---
Nitpick comments:
In `@apps/ade-cli/src/services/push/pushPublisherService.ts`:
- Line 64: The PushCliRuntimeSignal.runtimeState field is too broad and should
use the shared TerminalRuntimeState union instead of string. Update
PushCliRuntimeSignal and the onCliRuntimeSignal handling in PushPublisherService
so the runtime-state mapping stays aligned with ptyService, and make sure "idle"
is handled explicitly rather than being collapsed into the generic running
state.
🪄 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: 3f5566f9-6776-4325-9195-93cbbbfd3192
⛔ Files ignored due to path filters (1)
docs/features/sync-and-multi-device/push-notifications.mdis excluded by!docs/**
📒 Files selected for processing (3)
apps/ade-cli/src/bootstrap.tsapps/ade-cli/src/services/push/pushPublisherService.test.tsapps/ade-cli/src/services/push/pushPublisherService.ts
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/components/settings/SyncDevicesSection.tsx (1)
13-13: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider moving
publicAssetUrlto a shared module.Importing this helper from an onboarding component file (
../onboarding/WelcomeVideoGate) into a settings component works, but couples two unrelated feature areas to the same file just for a small pure utility. Since it's now used in both onboarding and settings, extracting it to a shared utils/assets module would clean up the dependency direction.🤖 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/settings/SyncDevicesSection.tsx` at line 13, Move the shared pure helper publicAssetUrl out of the onboarding-specific WelcomeVideoGate module into a shared assets/utils module, then update SyncDevicesSection and any other callers to import it from that shared location. Keep the function name and behavior unchanged so both onboarding and settings can depend on it without coupling to the onboarding component file.
🤖 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 `@apps/desktop/src/renderer/components/settings/SyncDevicesSection.tsx`:
- Line 13: Move the shared pure helper publicAssetUrl out of the
onboarding-specific WelcomeVideoGate module into a shared assets/utils module,
then update SyncDevicesSection and any other callers to import it from that
shared location. Keep the function name and behavior unchanged so both
onboarding and settings can depend on it without coupling to the onboarding
component file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4c47a787-0616-41ca-9916-819e4c51a3b0
📒 Files selected for processing (3)
apps/ade-cli/src/services/push/pushPublisherService.test.tsapps/ade-cli/src/services/push/pushPublisherService.tsapps/desktop/src/renderer/components/settings/SyncDevicesSection.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/ade-cli/src/services/push/pushPublisherService.test.ts
- apps/ade-cli/src/services/push/pushPublisherService.ts
Wire ptyService.onSessionRuntimeSignal (OSC 133-derived running / waiting-input state, previously unconsumed by the brain) into the push publisher so tracked CLI sessions appear as run rows in the agent-runs Live Activity alongside chats. - Live Activity only, no alert pushes: a CLI agent returns to its prompt after every turn, so alerting on waiting-input would ping per turn. Failure alerts remain on the existing non-zero-exit path, which now also flips the run to its terminal phase. - Heartbeat re-fires of an unchanged state are ignored (no LA churn). - Chat-attached shells and unknown sessions are dropped via a resolveCliSession source (sessionService-backed) so a chat's shell never double-counts against its chat run. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Map the PTY `idle` runtime signal (12s quiet, no OSC prompt marker) to the `stale` Live Activity phase instead of `running` — a silent CLI no longer overstates as an active row. Stale rows are excluded from activeCount but hold the activity open (ending on a quiet spell would churn end -> push-to-start), prune after the 2h running TTL, and flip back to running when output resumes. - Tighten the shell/ghost exclusion test to assert no publish happens, and exercise exclusion against a real run's payload. - Restyle the mobile-pairing QR to match the welcome modal's ADE-branded TestFlight QR (accent-tinted tile, excavated ADE mark, level H). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Post-rebase reconciliation with #710: adapt publisher tests to the silent badge-sync item, and exclude CLI runs from the app-icon badge count — a CLI at its prompt reports waiting_for_input as its resting state (same reasoning as the no-alert rule), so counting it would pin the badge non-zero forever. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
One vocabulary for session status across the Work tab, Live Activity,
and notifications: sessionCanonicalState maps session inputs to
{phase, badge} with deterministic precedence — pendingInputItemId /
waiting-input > failed > stale (running + silent >= 20 min) > running,
with the preview-text heuristic consulted last (it can upgrade a plain
running session, never outvote runtime state). Badges exist only for
needs_you / failed / stale.
sessionIndicatorState now honors pendingInputItemId (previously
invisible to it) and gains sessionCapsuleBadge for the row capsules.
17 table-driven precedence + stale-boundary tests.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…oke the poller - Desktop SessionCard: one-word attention capsule (Needs you / Failed / Stale) from the shared canonical state; zero layout shift when calm; chat cards never double up with the existing Awaiting chip. - Desktop Work list lane headers: primary-open-PR badge (dot + #num + state) left of the session count, deep-linking to the PRs tab; ADE- mapped PRs only (GitHub-by-branch parity gap noted). - iOS: WorkSessionCanonicalState mirrors the vocabulary exactly (12 table-driven tests incl. the 20-min stale boundary); capsule renders on Work rows; the per-row WorkLanePrIndicator moves to the lane section header with the same cross-tab PR navigation. - PR freshness: automationIngressService gains onPrStateIngested; both the daemon and desktop-main wire it to prPollingService.poke(), so webhook-driven PR changes reach the panel immediately instead of waiting out the next scheduled poll tick. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
arul28
changed the title
|
@codex review |
arul28
force-pushed
the
ade/mobile-push-relay-platform-c1ebe009
branch
from
5f31785 to
515f496
Compare
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 515f496259
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (ingested?.processed && !ingested.duplicate && ingested.linkedPrIds.length > 0) { | ||
| args.onPrStateIngested?.(); | ||
| } |
There was a problem hiding this comment.
This new callback only runs through dispatchGithubWebhook, but hosted GitHub relay events are handled by the relay polling loop, which still calls prService.ingestGithubWebhook directly and ignores the result. In the relay-delivered webhook case this means onPrStateIngested is never fired, so PR changes from the relay still wait for the next PR polling tick instead of refreshing immediately. Please apply the same processed/non-duplicate check in the relay poll path after its ingest call.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2d33805: the relay poll loop's ingest now fires onPrStateIngested with the same changed-state guard as the local-webhook path.
| if (typeof args.exitCode === "number" && args.exitCode !== 0) { | ||
| return { phase: "failed", badge: BADGE_BY_KIND.failed }; |
There was a problem hiding this comment.
When a PTY fails before it has an exit code, the session is persisted with status: "failed" and exitCode: null, while the derived runtime state is just "exited"; this branch then falls through to ended and renders no Failed capsule. Since status === "failed" is already the persisted failure signal for those spawn/setup failures, the canonical state should treat it as failed even when exitCode is null.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2d33805: persisted status:"failed" (spawn/setup death, null exit code) is now a deterministic failed signal in the ended branch — mirrored in the Swift port, regression added.
| toolType: session.toolType, | ||
| pendingInputItemId: session.pendingInputItemId, | ||
| lastOutputPreview: session.lastOutputPreview, | ||
| lastActivityAt: workSessionActivityTimestamp(session: session, summary: summary), |
There was a problem hiding this comment.
For non-chat terminal rows this helper falls back to session.startedAt, because the iOS TerminalSessionSummary model does not carry the desktop lastActivityAt field. Any running terminal that has been open for more than 20 minutes will therefore get a Stale capsule even if it produced output seconds ago; the stale check needs the actual last-output/activity timestamp, or it should be disabled when that timestamp is unavailable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2d33805: stale detection now uses a stale-only timestamp (lastActivityAt/chatIdleSinceAt, nil when absent) instead of the startedAt display fallback — plain terminals can no longer false-Stale.
…stale, frozen stale TTL, relay-poll poke - SessionListPane's PR-badge hook no-ops when window.ade.prs is absent (partial test mocks / web preview) instead of throwing in the effect. - Persisted status:"failed" with a null exit code (spawn/setup death) now yields the Failed capsule on both platforms. - iOS stale detection uses a stale-only activity timestamp (real lastActivityAt / chatIdleSinceAt, never the startedAt fallback), so a plain terminal open >20min no longer reads Stale. - A stale CLI row's lastActiveAt stays frozen through idle heartbeats, so it ages past the 2h TTL and releases the Live Activity (named regression added). - The relay poll loop's ingest now fires onPrStateIngested like the local-webhook path, so relay-delivered PR changes poke the poller. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2d33805e21
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // reset that 2h clock and hold the stale Live Activity open forever. | ||
| if (phase !== "stale") existing.lastActiveAt = now(); | ||
| return; |
There was a problem hiding this comment.
When a CLI becomes stale, the service publishes that state once and then this same-state path returns without scheduling any future flush. pruneRuns() only runs from flush(), so a quiet tracked CLI with no later output/state change never reaches the 2h retirement path and can keep liveActivityStarted true until an unrelated event occurs. Schedule a delayed flush/poke for the stale TTL, or otherwise make stale heartbeats trigger pruning without refreshing lastActiveAt.
Useful? React with 👍 / 👎.
| // Deterministic signals first (pendingInputItemId now counts — it used to | ||
| // be invisible here); the preview-text heuristic is consulted LAST and can | ||
| // only upgrade a plain running session, never outvote runtime state. | ||
| if (args.pendingInputItemId || args.runtimeState === "waiting-input") return "running-needs-attention"; |
There was a problem hiding this comment.
This newly makes pendingInputItemId a deterministic needs-attention signal, but the aggregate Work attention path still calls sessionIndicatorState without that field in summarizeTerminalAttention. For sessions where the pending item arrives before runtimeState flips to waiting-input, the row can show a needs-you capsule while the TopBar/TabNav attention counts still classify it as merely running; thread pendingInputItemId through the summary/status-dot callers too.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/ios/ADE/Views/Work/WorkRootComponents.swift (1)
814-874: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftStale capsule needs a time-based refresh.
WorkSessionRowRenderSignatureonly changes on session-data updates, butcapsuleBadgederives.stalefromDate(). A silent running session can stay visually calm until some unrelated row update happens. Add a periodic invalidation path here—either a coarse time bucket in the signature or a timer-driven refresh—so the badge flips to Stale on its own.🤖 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/ios/ADE/Views/Work/WorkRootComponents.swift` around lines 814 - 874, WorkSessionRowRenderSignature is missing any time-based input, so capsules that derive .stale from Date() won’t re-render until unrelated session data changes. Add a coarse periodic refresh trigger by extending WorkSessionRowRenderSignature with a time bucket or similar timestamp-based field, and wire it through the init/session row rendering path so WorkSessionRow and capsuleBadge update automatically as time passes.
🧹 Nitpick comments (6)
apps/desktop/src/main/services/automations/automationIngressService.ts (1)
345-362: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated ingest-and-notify pattern; verify
ingestedshape assumptions.The webhook path (Lines 345-362) and relay path (Lines 608-624) both implement the identical "await ingestGithubWebhook → catch → log → check processed/duplicate/linkedPrIds → poke" sequence. Extracting a shared helper would remove the duplication and keep both callers consistent if the notify condition changes later.
Also,
ingested.linkedPrIds.lengthis accessed unconditionally wheneveringested?.processedis truthy — ifingestGithubWebhookcan ever resolve withprocessed: truebutlinkedPrIdsundefined/null, this throws. SinceingestGithubWebhook's implementation isn't in this diff, worth confirming the return type guaranteeslinkedPrIdsis always an array whenprocessedis true.♻️ Proposed refactor to de-duplicate the ingest+notify logic
+ const ingestAndMaybeNotify = async ( + githubEvent: string, + deliveryId: string, + payload: Record<string, unknown>, + failureEvent: string, + failureFields: Record<string, unknown>, + ) => { + const ingested = await args.prService?.ingestGithubWebhook({ + eventName: githubEvent, + deliveryId, + payload, + }).catch((error) => { + args.logger.warn(failureEvent, { + ...failureFields, + error: error instanceof Error ? error.message : String(error), + }); + return null; + }); + if (ingested?.processed && !ingested.duplicate && (ingested.linkedPrIds?.length ?? 0) > 0) { + args.onPrStateIngested?.(); + } + return ingested; + };Also applies to: 608-624
🤖 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/automations/automationIngressService.ts` around lines 345 - 362, The webhook and relay paths in automationIngressService duplicate the same ingest-and-notify flow, so extract that logic into a shared helper around ingestGithubWebhook, logging, and onPrStateIngested to keep the two callers consistent. While refactoring, verify the return shape from ingestGithubWebhook and guard the linkedPrIds access so the notify condition cannot throw if processed is true but linkedPrIds is missing or null. Use the existing automationIngressService flow and the ingestGithubWebhook result handling as the main points to consolidate.apps/desktop/src/renderer/lib/terminalAttention.ts (1)
97-118: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
sessionIndicatorStatere-implements the deterministic precedence rule instead of delegating tocanonicalSessionState.Line 109 duplicates the exact
pendingInputItemId || runtimeState === "waiting-input"check thatcanonicalSessionState(imported in this same file, used two functions below forsessionCapsuleBadge) already encodes as the single source of truth. The module doc forsessionCanonicalState.tsexplicitly states its purpose is to be "the ONE vocabulary" shared across the app precisely to avoid this kind of duplicated precedence logic drifting apart over time (e.g., if the deterministic condition is ever extended, this call site could silently fall out of sync).Consider deriving the tab-level indicator from
canonicalSessionState(...).phase(mappingneeds_you/failed/stale→"running-needs-attention", others →"running-active") rather than re-deriving it from raw fields.🤖 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/lib/terminalAttention.ts` around lines 97 - 118, sessionIndicatorState is duplicating the session precedence rules instead of using canonicalSessionState as the single source of truth. Update sessionIndicatorState in terminalAttention.ts to derive its running/attention result from canonicalSessionState(args).phase, mapping the attention phases to "running-needs-attention" and the rest to "running-active", so the logic stays aligned with the shared session vocabulary used elsewhere in the file, including sessionCapsuleBadge.apps/desktop/src/shared/sessionCanonicalState.ts (1)
96-98: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
pendingInputItemIdisn't trimmed before the truthiness check, unlike the iOS mirror.The iOS
workCanonicalSessionStatetrimspendingInputItemIdbefore checking emptiness (pending = pendingInputItemId?.trimmingCharacters(...) ?? ""), so a whitespace-only id is treated as absent there. Here,args.pendingInputItemIdis used as-is, so" "would be truthy and triggerneeds_you, while the same value on iOS would not. Given this module's stated purpose is to be the single cross-platform vocabulary, this is worth aligning.🩹 Proposed fix
- if (args.pendingInputItemId || args.runtimeState === "waiting-input") { + if (args.pendingInputItemId?.trim() || args.runtimeState === "waiting-input") {Based on learnings from the upstream contract (apps/ios/ADE/Views/Work/WorkSessionCanonicalState.swift:85-145), which trims
pendingInputItemIdbefore the emptiness check.🤖 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/shared/sessionCanonicalState.ts` around lines 96 - 98, Align the cross-platform pending-input check in sessionCanonicalState by trimming args.pendingInputItemId before evaluating whether it should enter the needs_you phase. Update the logic around the pendingInputItemId/runtimeState check so whitespace-only IDs are treated as absent, matching the iOS workCanonicalSessionState contract. Use the existing sessionCanonicalState flow and the needs_you branch with BADGE_BY_KIND.needs_you as the place to apply the trim-and-empty check.apps/desktop/src/shared/sessionCanonicalState.test.ts (1)
26-57: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a case for "chat idle + past stale threshold".
The precedence chain deliberately evaluates stale (step 3) before the idle-chat "ready" resting state, meaning a chat tool that's been silent past
SESSION_STALE_AFTER_MSwill resolve to"stale"rather than the calm"ready"phase. This is a subtle, easy-to-regress rule (mirrors iOS) that isn't currently covered by a dedicated case in this table.✅ Suggested case addition
["idle chat is ready (no badge)", { runtimeState: "idle", toolType: "claude-chat" }, "ready", null], + ["chat silent past threshold is stale, not ready", { lastActivityAt: silentSince, toolType: "claude-chat" }, "stale", "Stale"],🤖 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/shared/sessionCanonicalState.test.ts` around lines 26 - 57, Add a dedicated precedence test case in canonicalSessionState precedence to cover a claude-chat session that is idle but has lastActivityAt older than SESSION_STALE_AFTER_MS; this should assert that state() resolves to the stale phase with a Stale badge instead of ready. Place it alongside the existing cases in sessionCanonicalState.test.ts so the ordering rule in state() and the idle-chat handling remain locked in.apps/ios/ADE/Views/Work/WorkRootScreen+Actions.swift (1)
359-376: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared PR-navigation publishing logic.
openLanePullRequestre-implements the sameprId/prNumber/laneIdbranching already inopenPullRequest(Lines 341-357). Consider factoring a small private helper both call, so future changes toPrNavigationRequestconstruction only need to happen once.♻️ Proposed refactor
+ private func publishPrNavigation(tag: LanePrTag, laneId: String?) { + let trimmedLaneId = laneId?.trimmingCharacters(in: .whitespacesAndNewlines) ?? "" + if let prId = tag.prId, !prId.isEmpty { + syncService.requestedPrNavigation = PrNavigationRequest( + prId: prId, + prNumber: tag.githubPrNumber, + laneId: trimmedLaneId.isEmpty ? nil : trimmedLaneId + ) + } else { + syncService.requestedPrNavigation = PrNavigationRequest(prNumber: tag.githubPrNumber) + } + } + func openPullRequest(_ session: TerminalSessionSummary, tag: LanePrTag) { let laneId = resolvedWorkNavigationLaneId(for: session, lanes: lanes) Task { `@MainActor` in try? await Task.sleep(for: .milliseconds(450)) - if let prId = tag.prId, !prId.isEmpty { - syncService.requestedPrNavigation = PrNavigationRequest( - prId: prId, - prNumber: tag.githubPrNumber, - laneId: laneId.isEmpty ? nil : laneId - ) - } else { - syncService.requestedPrNavigation = PrNavigationRequest(prNumber: tag.githubPrNumber) - } + publishPrNavigation(tag: tag, laneId: laneId) } } func openLanePullRequest(tag: LanePrTag, laneId: String?) { - let trimmedLaneId = laneId?.trimmingCharacters(in: .whitespacesAndNewlines) ?? "" Task { `@MainActor` in - if let prId = tag.prId, !prId.isEmpty { - syncService.requestedPrNavigation = PrNavigationRequest( - prId: prId, - prNumber: tag.githubPrNumber, - laneId: trimmedLaneId.isEmpty ? nil : trimmedLaneId - ) - } else { - syncService.requestedPrNavigation = PrNavigationRequest(prNumber: tag.githubPrNumber) - } + publishPrNavigation(tag: tag, laneId: laneId) } }🤖 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/ios/ADE/Views/Work/WorkRootScreen`+Actions.swift around lines 359 - 376, `openLanePullRequest` duplicates the same `PrNavigationRequest` publishing branching already present in `openPullRequest`, so factor that `prId`/`prNumber`/`laneId` construction into a small private helper and have both methods call it. Keep the existing `syncService.requestedPrNavigation` handoff and `Task { `@MainActor` in ... }` flow in `WorkRootScreen+Actions`, but move the shared request-building logic into one place so future changes only need to be made once.apps/ade-cli/src/services/push/pushPublisherService.ts (1)
75-81: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winType
PushCliRuntimeSignal.runtimeStateasTerminalRuntimeState.
onSessionRuntimeSignalalready forwards that shared PTY union here, so reusing it keeps the wire contract aligned and prevents invalid states from slipping through as plain strings.🤖 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/ade-cli/src/services/push/pushPublisherService.ts` around lines 75 - 81, The PushCliRuntimeSignal.runtimeState field is too broad as a string and should reuse the shared PTY TerminalRuntimeState union. Update the PushCliRuntimeSignal type in pushPublisherService to reference TerminalRuntimeState for runtimeState so it stays aligned with onSessionRuntimeSignal and preserves the intended wire contract.
🤖 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/ade-cli/src/bootstrap.ts`:
- Around line 754-760: The desktop `onSessionRuntimeSignal` path is missing the
same runtime-state forwarding that `createAdeRuntime` uses in `ade-cli`. Update
the Electron main-process handler so it also invokes
`pushPublisherService.handleCliRuntimeSignal` with the signal payload (`laneId`,
`sessionId`, `runtimeState`) instead of only emitting `IPC.sessionsChanged`. If
desktop cannot access that service directly, route desktop runtime signals
through the same `createAdeRuntime`/publisher wiring so `running`,
`waiting_for_input`, and `stale` states are propagated consistently.
In `@apps/ade-cli/src/services/push/pushPublisherService.test.ts`:
- Around line 491-497: The badge assertion in pushPublisherService.test is
vacuous because `second.notifications` is usually empty for the CLI resting
state, so the loop never verifies anything. Update the test around
`publisher.handleCliRuntimeSignal`, `publish`, and `second.notifications` to
assert the actual invariant directly: confirm no notification item with a
nonzero `badge` is emitted, or assert that `second.notifications` is
undefined/empty when `runtimeState: "waiting-input"` is reported.
In `@apps/desktop/src/renderer/components/terminals/SessionListPane.tsx`:
- Around line 313-340: The LanePrHeaderBadge is currently rendered as an
interactive control inside the lane header’s native button, which creates
invalid nested-interactive markup. Update the LanePrHeaderBadge usage in
SessionListPane so it is hoisted outside the header button, or refactor the
outer lane header control into a non-button wrapper with explicit keyboard
handling; keep the existing LanePrHeaderBadge click/keyboard behavior, but
ensure it is not nested inside the header button element.
---
Outside diff comments:
In `@apps/ios/ADE/Views/Work/WorkRootComponents.swift`:
- Around line 814-874: WorkSessionRowRenderSignature is missing any time-based
input, so capsules that derive .stale from Date() won’t re-render until
unrelated session data changes. Add a coarse periodic refresh trigger by
extending WorkSessionRowRenderSignature with a time bucket or similar
timestamp-based field, and wire it through the init/session row rendering path
so WorkSessionRow and capsuleBadge update automatically as time passes.
---
Nitpick comments:
In `@apps/ade-cli/src/services/push/pushPublisherService.ts`:
- Around line 75-81: The PushCliRuntimeSignal.runtimeState field is too broad as
a string and should reuse the shared PTY TerminalRuntimeState union. Update the
PushCliRuntimeSignal type in pushPublisherService to reference
TerminalRuntimeState for runtimeState so it stays aligned with
onSessionRuntimeSignal and preserves the intended wire contract.
In `@apps/desktop/src/main/services/automations/automationIngressService.ts`:
- Around line 345-362: The webhook and relay paths in automationIngressService
duplicate the same ingest-and-notify flow, so extract that logic into a shared
helper around ingestGithubWebhook, logging, and onPrStateIngested to keep the
two callers consistent. While refactoring, verify the return shape from
ingestGithubWebhook and guard the linkedPrIds access so the notify condition
cannot throw if processed is true but linkedPrIds is missing or null. Use the
existing automationIngressService flow and the ingestGithubWebhook result
handling as the main points to consolidate.
In `@apps/desktop/src/renderer/lib/terminalAttention.ts`:
- Around line 97-118: sessionIndicatorState is duplicating the session
precedence rules instead of using canonicalSessionState as the single source of
truth. Update sessionIndicatorState in terminalAttention.ts to derive its
running/attention result from canonicalSessionState(args).phase, mapping the
attention phases to "running-needs-attention" and the rest to "running-active",
so the logic stays aligned with the shared session vocabulary used elsewhere in
the file, including sessionCapsuleBadge.
In `@apps/desktop/src/shared/sessionCanonicalState.test.ts`:
- Around line 26-57: Add a dedicated precedence test case in
canonicalSessionState precedence to cover a claude-chat session that is idle but
has lastActivityAt older than SESSION_STALE_AFTER_MS; this should assert that
state() resolves to the stale phase with a Stale badge instead of ready. Place
it alongside the existing cases in sessionCanonicalState.test.ts so the ordering
rule in state() and the idle-chat handling remain locked in.
In `@apps/desktop/src/shared/sessionCanonicalState.ts`:
- Around line 96-98: Align the cross-platform pending-input check in
sessionCanonicalState by trimming args.pendingInputItemId before evaluating
whether it should enter the needs_you phase. Update the logic around the
pendingInputItemId/runtimeState check so whitespace-only IDs are treated as
absent, matching the iOS workCanonicalSessionState contract. Use the existing
sessionCanonicalState flow and the needs_you branch with BADGE_BY_KIND.needs_you
as the place to apply the trim-and-empty check.
In `@apps/ios/ADE/Views/Work/WorkRootScreen`+Actions.swift:
- Around line 359-376: `openLanePullRequest` duplicates the same
`PrNavigationRequest` publishing branching already present in `openPullRequest`,
so factor that `prId`/`prNumber`/`laneId` construction into a small private
helper and have both methods call it. Keep the existing
`syncService.requestedPrNavigation` handoff and `Task { `@MainActor` in ... }`
flow in `WorkRootScreen+Actions`, but move the shared request-building logic
into one place so future changes only need to be made once.
🪄 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: b75847ac-ff55-45d7-93c9-aca02017f8fc
⛔ Files ignored due to path filters (2)
apps/ios/ADE.xcodeproj/project.pbxprojis excluded by!**/*.xcodeproj/project.pbxprojdocs/features/sync-and-multi-device/push-notifications.mdis excluded by!docs/**
📒 Files selected for processing (21)
apps/ade-cli/src/bootstrap.tsapps/ade-cli/src/services/push/pushPublisherService.test.tsapps/ade-cli/src/services/push/pushPublisherService.tsapps/desktop/src/main/main.tsapps/desktop/src/main/services/automations/automationIngressService.tsapps/desktop/src/renderer/components/settings/SyncDevicesSection.tsxapps/desktop/src/renderer/components/terminals/SessionCard.test.tsxapps/desktop/src/renderer/components/terminals/SessionCard.tsxapps/desktop/src/renderer/components/terminals/SessionListPane.tsxapps/desktop/src/renderer/lib/lanePrBadge.test.tsapps/desktop/src/renderer/lib/lanePrBadge.tsapps/desktop/src/renderer/lib/terminalAttention.tsapps/desktop/src/shared/sessionCanonicalState.test.tsapps/desktop/src/shared/sessionCanonicalState.tsapps/ios/ADE/Views/Work/WorkRootComponents.swiftapps/ios/ADE/Views/Work/WorkRootScreen+Actions.swiftapps/ios/ADE/Views/Work/WorkRootScreen.swiftapps/ios/ADE/Views/Work/WorkSessionCanonicalState.swiftapps/ios/ADE/Views/Work/WorkSessionGrouping.swiftapps/ios/ADE/Views/Work/WorkStatusAndFormattingHelpers.swiftapps/ios/ADETests/WorkSessionCanonicalStateTests.swift
✅ Files skipped from review due to trivial changes (1)
- apps/desktop/src/renderer/lib/lanePrBadge.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/renderer/components/settings/SyncDevicesSection.tsx
…dge-count assertion - The lane header is now a flex row (collapse-toggle button + badge/count cluster as siblings) instead of nesting the interactive PR badge inside a native button — invalid HTML that broke focus order/assistive tech. - Replace the vacuous zero-iteration badge loop with direct countAwaitingAttentionRuns assertions (CLI waiting run counts 0, chat waiting run counts 1). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 96597d047c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // (spawn/setup failures that die before an exit code), or a killed | ||
| // runtime — all deterministic "failed" signals a terminal-backed session | ||
| // reports. | ||
| if (typeof args.exitCode === "number" && args.exitCode !== 0) { |
There was a problem hiding this comment.
When a user intentionally stops a PTY with Ctrl-C/SIGTERM, ptyService.statusFromExit classifies exit codes 130/143 as disposed, but this unconditional non-zero exit check still turns those sessions into the red Failed capsule. In that normal Stop runtime path the session was deliberately ended, not failed, so the badge becomes misleading; gate this on a failed status or explicitly exclude disposed exit codes.
Useful? React with 👍 / 👎.
| if (isSilentPast(args.lastActivityAt, nowMs, SESSION_STALE_AFTER_MS)) { | ||
| return { phase: "stale", badge: BADGE_BY_KIND.stale }; |
There was a problem hiding this comment.
For chat sessions resting between turns, runtimeState is idle and lastActivityAt can easily be more than 20 minutes old; because the stale check runs before the idle-chat ready branch, ordinary old idle chats get a Stale capsule instead of staying calm. Gate this stale path to non-idle active runs (and mirror the same ordering fix in the Swift copy) so resting chats do not look stuck.
Useful? React with 👍 / 👎.
1 participant
Follow-up to #705/#710 — makes session status truthful end to end, producer to pixels.
Producer (push publisher)
ptyService.onSessionRuntimeSignal(OSC 133-derived) feeds tracked CLI sessions into the agent-runs Live Activity:running/waiting_for_inputrows;idlepublishes asstale(dimmed, not counted active, holds the activity open — no end→push-to-start churn); no alert pushes for CLI prompts (a CLI at its prompt is its resting state) and CLI prompts don't badge the app icon.One vocabulary (
shared/sessionCanonicalState.ts)Canonical
{phase, badge}mapping with locked precedence:pendingInputItemId/waiting-input(deterministic) > failed > stale (running + silent ≥ 20 min) > running, preview-text heuristic consulted LAST (fixes it outvoting deterministic signals;sessionIndicatorStatenow also honorspendingInputItemId).needs_you⇔ the Live Activity's waiting phases; wire names unchanged. 17 table-driven tests.Consumers
WorkSessionCanonicalStatemirrors the module exactly (12 unit tests incl. stale boundary); capsules on Work rows; the per-row PR indicator consolidates onto the lane section header with the samerequestedPrNavigationhandoff.PR panel freshness (webhook → UI, previously broken link)
Webhook events polled from the relay updated the DB silently — the panel waited for the PR poller's next scheduled tick.
automationIngressServicenow firesonPrStateIngestedwhen an ingested event changed PR state, wired toprPollingService.poke()in both the daemon and desktop-main, so webhook-driven changes surface in the panel immediately.Also in this PR
Idle→stale Greptile fix, ADE-branded pairing QR (matches the welcome modal's TestFlight QR treatment), post-#710 reconciliation.
Tests: ade-cli 1526 green; desktop canonical/attention/card/badge/ingress suites green; iOS build + 12 new tests green.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Greptile Summary
This PR wires CLI session runtime signals into the Live Activity pipeline, introduces a shared canonical session-state vocabulary (
sessionCanonicalState.ts/WorkSessionCanonicalState.swift) that locks the deterministic-signal precedence chain across desktop, iOS, and the push publisher, and fixes the previously broken webhook → PR-panel freshness link by poking the PR poller immediately on ingestion.ptyService.onSessionRuntimeSignalfeeds the push publisher; idle maps tostale(dimmed, not counted active, holds the activity open without churn); chat-owned shells are filtered viaresolveCliSession;countAwaitingAttentionRunsexcludes CLI runs to avoid a pinned-forever badge.pendingInputItemId/waiting-input> failed > stale > running with preview heuristic last) shared by desktop capsule badges, iOSWorkSessionStatusCapsule, and documented to map onto the Live Activity wire phases; 17 desktop + 12 iOS table-driven tests lock the boundary conditions.automationIngressServicenow firesonPrStateIngestedwhen an ingested event actually changed PR state, wired toprPollingService.poke()in both the daemon and desktop-main, so panel updates ride the webhook instead of the next scheduled tick.Confidence Score: 5/5
Safe to merge — the changes are additive and well-tested; the two previously flagged bugs are confirmed fixed.
All new logic is covered by table-driven tests on both desktop and iOS. The onPrStateIngested poke is gated on processed and non-duplicate conditions so it will not fire spuriously. The late-binding pattern for pushPublisherForPtySignals and prPollingServiceForIngress is consistent with existing bootstrap patterns.
No files require special attention.
Important Files Changed
kindfield,onCliRuntimeSignal,countAwaitingAttentionRunsCLI exclusion, stale-phase TTL pruning, andonPtyExitterminal-phase fix. Previously flagged idle→stale and TTL issues are addressed.onPrStateIngestedwhen an ingested event actually changed PR state (processed, non-duplicate, linked PRs present). Return null added to catch handlers for correct assignment typing.pendingInputItemIdtosessionIndicatorState, fixing it to honor deterministic signals; idle sessions no longer fall through to the preview heuristic. NewsessionCapsuleBadgedelegates to the canonical state module.<button>to<div>+ inner<button>to allow the interactive PR badge alongside the collapse toggle. AddsuseLanePrsByLaneIdhook andLanePrHeaderBadgecomponent.WorkSessionStatusCapsuleview, andworkSessionCapsuleBadgerow wrapper.AttentionCapsulefor three states (needs_you, failed, stale) next to the session title. Suppresses the capsule when the chat-specific amber chip already shows the same condition, preventing double-amber.pushPublisherForPtySignalsandprPollingServiceForIngressto enable the new onSessionRuntimeSignal→Live Activity bridge and webhook→immediate-poll paths.publicAssetUrlhelper as the welcome modal.WorkLanePrIndicatoras a dedicated button, outside the collapse-toggle button.WorkSessionRowrenders canonical attention capsule fromworkSessionCapsuleBadge.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[ptyService.onSessionRuntimeSignal] -->|running / waiting-input / idle| B[onCliRuntimeSignal] B -->|waiting-input| C[phase: waiting_for_input] B -->|idle| D[phase: stale] B -->|running| E[phase: running] F[ptyService.onPtyExit] -->|exitCode=0| G[phase: completed] F -->|exitCode not 0| H[phase: failed + alert push] C & E & G & H --> I[scheduleFlush] D --> I I --> J{activeCount > 0?} J -->|yes, not started| K[start Live Activity] J -->|no, dormantCount > 0| L[update - stale holds open] J -->|no, dormantCount=0, started| M[end Live Activity] N[GitHub webhook] --> O[automationIngressService] O -->|processed and linkedPrIds present| P[onPrStateIngested] P --> Q[prPollingService.poke] Q --> R[prs-updated to renderer] S[sessionCanonicalState] -->|pendingInputItemId or waiting-input| T[needs_you badge] S -->|non-zero exit or killed| U[failed badge] S -->|running + silent 20min| V[stale badge] S -->|calm states| W[no badge]%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% flowchart TD A[ptyService.onSessionRuntimeSignal] -->|running / waiting-input / idle| B[onCliRuntimeSignal] B -->|waiting-input| C[phase: waiting_for_input] B -->|idle| D[phase: stale] B -->|running| E[phase: running] F[ptyService.onPtyExit] -->|exitCode=0| G[phase: completed] F -->|exitCode not 0| H[phase: failed + alert push] C & E & G & H --> I[scheduleFlush] D --> I I --> J{activeCount > 0?} J -->|yes, not started| K[start Live Activity] J -->|no, dormantCount > 0| L[update - stale holds open] J -->|no, dormantCount=0, started| M[end Live Activity] N[GitHub webhook] --> O[automationIngressService] O -->|processed and linkedPrIds present| P[onPrStateIngested] P --> Q[prPollingService.poke] Q --> R[prs-updated to renderer] S[sessionCanonicalState] -->|pendingInputItemId or waiting-input| T[needs_you badge] S -->|non-zero exit or killed| U[failed badge] S -->|running + silent 20min| V[stale badge] S -->|calm states| W[no badge]Reviews (5): Last reviewed commit: "fix: ship round 2 — hoist PR badge out o..." | Re-trigger Greptile