Mobile hub/chat overhaul, cross-project chat quick-look, and Lanes tab perf#692
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
Warning Review limit reached
Next review available in: 5 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds cross-project chat quick look, lane snapshot/detail conditional responses, approved-plan full-auto handling, and iOS Hub, lane, Work chat, and decoding updates. ChangesCross-Project Chat Quick Look
Lane Snapshot/Detail Conditional Response Caching
Approved Plan Full-Auto Permission Mode
iOS Hub Inline Composer Refactor
iOS Lane Card and Detail UI Refactor
iOS Work Chat Approval/Question Resolved Card Rendering
iOS Chat Event Decoding Robustness
Estimated code review effort: 5 (Critical) | ~120 minutes Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
|
@copilot review but do not make fixes |
…perf iOS hub: inline keyboard composer replaces the sheet drawer (draft/settings survive collapse; scrim + swipe/tap-outside dismiss), project-card badges and lane-row noise removed, hub chat-open now waits for real project hydration (no more blank transcripts / "Session unavailable"). Chat: file-change approvals get an actionable badge above the composer, question/approval/plan cards redesigned (provider-logo headers, option rows, compact resolved chips, plan preview + full-plan sheet), status-based composer locks removed, chat-event snapshot decode hardened (@ADELossyArray + notice kinds) so one unknown event can't wipe a transcript, plan event mapping fixed. Cross-project chat quick-look: chat_subscribe/history accept a project override (capability-gated, hello feature flag); the brain streams a foreign lane's transcript read-only without booting its runtime; phone opens hub chats without switching projects, falling back to activation on older brains. Codex plan approval now hands the session straight to full access. Lanes tab: rebase tag off list cards, collapsible Files/Stashes/History, wrapping header/chips, commit row split, manage-sheet desktop parity (+adopt-attached); perf — no-op lane_state_snapshots writes suppressed (conditional CRR upsert), signature/ifNoneMatch conditional lane list/detail responses, per-lane detail invalidation, per-row render signatures. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
9ea8bbf to
bbc88df
Compare
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 (3)
apps/ade-cli/src/services/sync/syncHostService.ts (1)
1905-1929: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve foreign chat transcript paths across peer handoff.
foreignChatTranscriptPathsneeds to be included in the handoff snapshot and restored withsubscribedChatSessionIds; otherwise cross-project quick-look chat subscriptions lose their resolved transcript path after a project switch andpumpChatEventssilently stops tailing them. UpdateSyncPeerHandoffSnapshotas well.🤖 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/sync/syncHostService.ts` around lines 1905 - 1929, Preserve foreign chat transcript paths during peer handoff: the handoff snapshot currently restores subscribed chat sessions in syncHostService.ts but drops foreignChatTranscriptPaths, causing cross-project quick-look subscriptions to lose their resolved transcript path and stop tailing. Add foreignChatTranscriptPaths to SyncPeerHandoffSnapshot, include it when building and serializing the snapshot, and restore it alongside subscribedChatSessionIds in the handoff/reconnect path so pumpChatEvents can continue using the correct path after a project switch.apps/ios/ADE/Services/SyncService.swift (1)
3865-3897: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGuard
ifNoneMatchon cached lane rows before treatingnotModifiedas ready.replaceLaneSnapshots(_:)deletes and repopulates the lane snapshot tables, whileresetLanePayloadSignatures()only runs on project scope changes. That leaves a path where the signature survives but the local lane rows are gone, sorefreshLaneSnapshots()can return.readywith an empty list after anotModifiedresponse.🤖 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/Services/SyncService.swift` around lines 3865 - 3897, The refresh flow in `refreshLaneSnapshots()` can mark lanes/files ready on a `notModified` response even when the local lane rows were wiped, because `ifNoneMatch` is still sent from `laneSnapshotSignatures` after `replaceLaneSnapshots(_:)`. Update the `ifNoneMatch` handling to only use cached signatures when the lane snapshot rows are actually present, and in the `payload.notModified` branch avoid setting `.ready` unless the cached lane data is still valid; use the existing `laneSnapshotSignatureKey`, `laneSnapshotSignatures`, and `replaceLaneSnapshots(_:)` logic to keep the cache and table contents in sync.apps/ios/ADE/Views/Lanes/LaneDetailGitActionsPane.swift (1)
361-381: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMove the accessibility combination onto the disclosure button only. The parent
HStackcombines the trailingButton, so VoiceOver exposesNew lane with changesas part of the disclosure control instead of a separate action. The otherdisclosureHeadercall sites don’t pass a trailing control, so they aren’t affected.🤖 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/Lanes/LaneDetailGitActionsPane.swift` around lines 361 - 381, The accessibility grouping in filesSection is too broad because the trailing “New lane with changes” Button is being combined with the disclosure control, causing VoiceOver to treat it as part of the header action. Update disclosureHeader so the accessibility combine is applied only to the disclosure button itself, and keep the trailing Button as a separate accessible action; use the filesSection/disclosureHeader call site and the onToggle/disclosureHeader structure to locate the fix.Source: Path instructions
🧹 Nitpick comments (5)
apps/ios/ADE/Views/Hub/HubComposerDrawer.swift (1)
565-599: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated send-button block between minimized and expanded densities.
The
ADEComposerSendButton(...)call (enabled/sending/labels/action) is duplicated almost verbatim at lines 589-597 and 644-652. Extracting a small private computed view (e.g.sendButton) would avoid the two copies drifting out of sync (e.g. if accessibility text or thedispatch()wiring changes later).♻️ Proposed extraction
+ private var sendButton: some View { + ADEComposerSendButton( + enabled: canSend && !busy, + sending: busy, + accessibilityLabelText: "Start chat", + disabledAccessibilityLabel: "Enter a message to start" + ) { + dispatch() + } + } + private var composerCard: some View { ... if !isExpanded { - ADEComposerSendButton( - enabled: canSend && !busy, - sending: busy, - accessibilityLabelText: "Start chat", - disabledAccessibilityLabel: "Enter a message to start" - ) { - dispatch() - } + sendButton .transition(.opacity) } ... if !isDictating { - ADEComposerSendButton( - enabled: canSend && !busy, - sending: busy, - accessibilityLabelText: "Start chat", - disabledAccessibilityLabel: "Enter a message to start" - ) { - dispatch() - } + sendButton }Also applies to: 601-656
🤖 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/Hub/HubComposerDrawer.swift` around lines 565 - 599, The send button configuration is duplicated in both composer density states, so extract the repeated ADEComposerSendButton setup into a single private computed view or helper used by composerCard and the expanded controls. Keep the shared enabled/sending/accessibility/action wiring in one place, and have both minimized and expanded layouts reference that helper so changes to labels or dispatch() only happen once.apps/ios/ADE/Views/Work/WorkPlanComposerViews.swift (1)
386-420: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLikely duplicate of the plan-strip's action button components.
The doc comments explicitly call out mirroring
WorkPlanComposerStrip's buttons ("matching the plan badge's action buttons"). IfWorkPlanComposerStriphas near-identical compact/detail action button views, consider extracting one shared component instead of maintaining two copies of the same capsule-button styling and dismiss-after-decision logic.Also applies to: 493-530
🤖 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/WorkPlanComposerViews.swift` around lines 386 - 420, The compact approval action button in WorkApprovalCompactActionButton appears to duplicate the plan strip’s button styling and behavior, so consolidate the shared capsule-button implementation instead of keeping parallel copies. Refactor the near-identical action button views used by WorkPlanComposerStrip and the collapsed approval strip into a reusable component, then have both call sites reuse that shared View while preserving their existing labels, tint, filled state, and action handling.apps/ios/ADE/Views/Work/WorkStatusAndFormattingHelpers.swift (1)
46-61: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
sessionStatusparameter is now dead in the freeform-gating helpers.
workChatComposerBlocksFreeformInputno longer readssessionStatus(gating is purelypendingInputCount > 0now), but the parameter is still declared and threaded through call sites andworkChatComposerPlaceholder. Consider dropping the unused parameter (or documenting why it's intentionally kept for future use) to avoid confusing future readers into thinking status still matters here.🤖 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/WorkStatusAndFormattingHelpers.swift` around lines 46 - 61, The freeform-gating logic now depends only on pending inputs, so `sessionStatus` is unused and misleading in `workChatComposerBlocksFreeformInput` and `workChatComposerPlaceholder`. Remove `sessionStatus` from these helper signatures and update all call sites that pass it, keeping the gating based on `pendingInputCount` alone. If you intend to preserve the parameter, document that it is intentionally unused; otherwise, delete it to avoid suggesting session status still affects composer behavior.apps/ade-cli/src/services/sync/syncRemoteCommandService.test.ts (1)
378-432: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd conditional-response coverage for
lanes.getDetail, not justlanes.refreshSnapshots.This suite thoroughly covers the new signature/
ifNoneMatchbehavior forlanes.refreshSnapshots(fresh payload → matchingifNoneMatch→notModified: true→ staleifNoneMatch→ fresh payload again), butlanes.getDetailgets the samerespondWithSignaturetreatment insyncRemoteCommandService.ts(Lines 2059-2063) with no equivalent test here. Givenlanes.getDetail'semptyResponseis{}(vs. refreshSnapshots' typed empty shell), it's worth a dedicated regression test to confirm the envelope only carriessignature/notModifiedand that a stale/missingifNoneMatchstill returns the fullLaneDetailPayload.✅ Suggested additional test outline
describe("lanes.getDetail conditional responses", () => { it("returns notModified shell for matching ifNoneMatch, full payload otherwise", async () => { // mock args.laneService / buildLaneDetailPayload equivalent dependencies // assert first.signature matches /^[0-9a-f]{64}$/ // assert second (matching ifNoneMatch) === { signature, notModified: true } // assert stale ifNoneMatch returns full payload again }); });🤖 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/sync/syncRemoteCommandService.test.ts` around lines 378 - 432, The conditional-response tests currently cover lanes.refreshSnapshots, but lanes.getDetail uses the same respondWithSignature path and is missing regression coverage. Add a dedicated test in syncRemoteCommandService.test.ts around createSyncRemoteCommandService/execute for lanes.getDetail that verifies the first call returns a full LaneDetailPayload with a 64-char signature, a matching ifNoneMatch returns only the signature plus notModified: true, and a stale ifNoneMatch returns the full payload again. Make sure the assertions reflect lanes.getDetail’s emptyResponse behavior and that the envelope does not include extra payload fields when notModified is true.apps/ios/ADE/Views/Lanes/LaneDetailGitActionsPane.swift (1)
361-381: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: repeated disclosure boilerplate could be a small helper.
The
disclosureHeader(...) ; if xDisclosure.expanded { xContent }pattern is duplicated three times (Files/Stashes/History). Not urgent, but a tinydisclosureSection(title:badge:disclosure:trailing:content:)wrapper would remove the duplication.Also applies to: 450-462, 504-516
🤖 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/Lanes/LaneDetailGitActionsPane.swift` around lines 361 - 381, The Files/Stashes/History blocks in LaneDetailGitActionsPane repeat the same disclosure pattern, so extract a small reusable disclosureSection(title:badge:disclosure:trailing:content:) helper to centralize the shared disclosureHeader plus expanded-content logic. Refactor the existing filesSection and the other two matching sections to use this helper, keeping their unique trailing actions and content views intact.
🤖 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/sync/syncHostService.ts`:
- Around line 3160-3184: In resolveForeignChatTranscriptPath, explicit foreign
requests should not return null and fall through to
args.sessionService.get(sessionId); instead, distinguish “no foreign scope
requested” from “foreign scope requested but provider could not resolve it” and
have the caller reject the latter so cross-project reads fail closed. Update the
handling around resolveForeignChatTranscriptPath and the 4395-4408 call site to
use a clear sentinel or error path for unresolvable foreign payloads, while
keeping the local-session fallback only for non-foreign requests. Also extend
the host short-circuit logic to treat requestedRootPath matching
args.projectRoot the same as projectIdMatchesHost so host-owned rootPath-only
payloads stay on the local path.
In `@apps/desktop/src/main/services/chat/agentChatService.ts`:
- Around line 15594-15598: The approved-plan handling in agentChatService is
over-promoting Codex to unrestricted access by setting permissionMode to
full-auto, which maps to no approvals and danger-full-access. Update the
approved-plan path in managed.session so it keeps Codex on workspace-write while
only disabling further approval prompts, and adjust
applyLegacyPermissionModeToNativeControls accordingly if needed. Use the
permissionMode handling near the approved-plan branch and the
applyLegacyPermissionModeToNativeControls call as the key points to change.
In `@apps/ios/ADE/Views/Lanes/LaneDetailSectionChrome.swift`:
- Around line 68-81: `LaneSectionDisclosure` currently relies on a synthesized
memberwise initializer that is effectively inaccessible because `pinnedByUser`
is private, so callers in `LaneDetailGitActionsPane` cannot construct it. Add an
explicit public/internal initializer on `LaneSectionDisclosure` with a defaulted
`expanded` parameter (for example, an `init(expanded: Bool = false)`), and
ensure it initializes both `expanded` and `pinnedByUser` so existing
`syncAuto(hasContent:)` and `toggle()` behavior remains unchanged.
In `@apps/ios/ADE/Views/Work/WorkChatRichCardViews.swift`:
- Around line 1201-1209: Hide structured options for secret questions by gating
both option rendering and selected-option lookup behind question.isSecret in
WorkChatRichCardViews. Update chosenOption so it skips secret questions instead
of returning a matched WorkPendingQuestionOption, and make the secure-response
card’s option UI, resolution pill, and accessibility text use the same
secret-question check so labels/descriptions cannot leak.
---
Outside diff comments:
In `@apps/ade-cli/src/services/sync/syncHostService.ts`:
- Around line 1905-1929: Preserve foreign chat transcript paths during peer
handoff: the handoff snapshot currently restores subscribed chat sessions in
syncHostService.ts but drops foreignChatTranscriptPaths, causing cross-project
quick-look subscriptions to lose their resolved transcript path and stop
tailing. Add foreignChatTranscriptPaths to SyncPeerHandoffSnapshot, include it
when building and serializing the snapshot, and restore it alongside
subscribedChatSessionIds in the handoff/reconnect path so pumpChatEvents can
continue using the correct path after a project switch.
In `@apps/ios/ADE/Services/SyncService.swift`:
- Around line 3865-3897: The refresh flow in `refreshLaneSnapshots()` can mark
lanes/files ready on a `notModified` response even when the local lane rows were
wiped, because `ifNoneMatch` is still sent from `laneSnapshotSignatures` after
`replaceLaneSnapshots(_:)`. Update the `ifNoneMatch` handling to only use cached
signatures when the lane snapshot rows are actually present, and in the
`payload.notModified` branch avoid setting `.ready` unless the cached lane data
is still valid; use the existing `laneSnapshotSignatureKey`,
`laneSnapshotSignatures`, and `replaceLaneSnapshots(_:)` logic to keep the cache
and table contents in sync.
In `@apps/ios/ADE/Views/Lanes/LaneDetailGitActionsPane.swift`:
- Around line 361-381: The accessibility grouping in filesSection is too broad
because the trailing “New lane with changes” Button is being combined with the
disclosure control, causing VoiceOver to treat it as part of the header action.
Update disclosureHeader so the accessibility combine is applied only to the
disclosure button itself, and keep the trailing Button as a separate accessible
action; use the filesSection/disclosureHeader call site and the
onToggle/disclosureHeader structure to locate the fix.
---
Nitpick comments:
In `@apps/ade-cli/src/services/sync/syncRemoteCommandService.test.ts`:
- Around line 378-432: The conditional-response tests currently cover
lanes.refreshSnapshots, but lanes.getDetail uses the same respondWithSignature
path and is missing regression coverage. Add a dedicated test in
syncRemoteCommandService.test.ts around createSyncRemoteCommandService/execute
for lanes.getDetail that verifies the first call returns a full
LaneDetailPayload with a 64-char signature, a matching ifNoneMatch returns only
the signature plus notModified: true, and a stale ifNoneMatch returns the full
payload again. Make sure the assertions reflect lanes.getDetail’s emptyResponse
behavior and that the envelope does not include extra payload fields when
notModified is true.
In `@apps/ios/ADE/Views/Hub/HubComposerDrawer.swift`:
- Around line 565-599: The send button configuration is duplicated in both
composer density states, so extract the repeated ADEComposerSendButton setup
into a single private computed view or helper used by composerCard and the
expanded controls. Keep the shared enabled/sending/accessibility/action wiring
in one place, and have both minimized and expanded layouts reference that helper
so changes to labels or dispatch() only happen once.
In `@apps/ios/ADE/Views/Lanes/LaneDetailGitActionsPane.swift`:
- Around line 361-381: The Files/Stashes/History blocks in
LaneDetailGitActionsPane repeat the same disclosure pattern, so extract a small
reusable disclosureSection(title:badge:disclosure:trailing:content:) helper to
centralize the shared disclosureHeader plus expanded-content logic. Refactor the
existing filesSection and the other two matching sections to use this helper,
keeping their unique trailing actions and content views intact.
In `@apps/ios/ADE/Views/Work/WorkPlanComposerViews.swift`:
- Around line 386-420: The compact approval action button in
WorkApprovalCompactActionButton appears to duplicate the plan strip’s button
styling and behavior, so consolidate the shared capsule-button implementation
instead of keeping parallel copies. Refactor the near-identical action button
views used by WorkPlanComposerStrip and the collapsed approval strip into a
reusable component, then have both call sites reuse that shared View while
preserving their existing labels, tint, filled state, and action handling.
In `@apps/ios/ADE/Views/Work/WorkStatusAndFormattingHelpers.swift`:
- Around line 46-61: The freeform-gating logic now depends only on pending
inputs, so `sessionStatus` is unused and misleading in
`workChatComposerBlocksFreeformInput` and `workChatComposerPlaceholder`. Remove
`sessionStatus` from these helper signatures and update all call sites that pass
it, keeping the gating based on `pendingInputCount` alone. If you intend to
preserve the parameter, document that it is intentionally unused; otherwise,
delete it to avoid suggesting session status still affects composer behavior.
🪄 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: 300b174b-1397-4e20-b9af-32f39496e6e1
⛔ Files ignored due to path filters (6)
docs/ARCHITECTURE.mdis excluded by!docs/**docs/features/chat/agent-routing.mdis excluded by!docs/**docs/features/lanes/README.mdis excluded by!docs/**docs/features/sync-and-multi-device/README.mdis excluded by!docs/**docs/features/sync-and-multi-device/ios-companion.mdis excluded by!docs/**docs/features/sync-and-multi-device/remote-commands.mdis excluded by!docs/**
📒 Files selected for processing (39)
apps/ade-cli/src/bootstrap.tsapps/ade-cli/src/cli.tsapps/ade-cli/src/services/sync/rosterBuilder.test.tsapps/ade-cli/src/services/sync/rosterBuilder.tsapps/ade-cli/src/services/sync/syncHostService.test.tsapps/ade-cli/src/services/sync/syncHostService.tsapps/ade-cli/src/services/sync/syncRemoteCommandService.test.tsapps/ade-cli/src/services/sync/syncRemoteCommandService.tsapps/ade-cli/src/services/sync/syncService.tsapps/desktop/src/main/services/chat/agentChatService.test.tsapps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/lanes/laneService.test.tsapps/desktop/src/main/services/lanes/laneService.tsapps/desktop/src/shared/types/lanes.tsapps/desktop/src/shared/types/sync.tsapps/ios/ADE/Models/RemoteModels.swiftapps/ios/ADE/Services/Database.swiftapps/ios/ADE/Services/SyncService.swiftapps/ios/ADE/Views/Hub/HubComponents.swiftapps/ios/ADE/Views/Hub/HubComposerDrawer.swiftapps/ios/ADE/Views/Hub/HubScreen+ChatNavigation.swiftapps/ios/ADE/Views/Hub/HubScreen.swiftapps/ios/ADE/Views/Lanes/LaneComponents.swiftapps/ios/ADE/Views/Lanes/LaneDetailGitActionsPane.swiftapps/ios/ADE/Views/Lanes/LaneDetailScreen.swiftapps/ios/ADE/Views/Lanes/LaneDetailSectionChrome.swiftapps/ios/ADE/Views/Lanes/LaneHelpers.swiftapps/ios/ADE/Views/Lanes/LaneManageSheet.swiftapps/ios/ADE/Views/Work/WorkChatComposerAndInputViews.swiftapps/ios/ADE/Views/Work/WorkChatRichCardViews.swiftapps/ios/ADE/Views/Work/WorkChatSessionView+Timeline.swiftapps/ios/ADE/Views/Work/WorkChatSessionView.swiftapps/ios/ADE/Views/Work/WorkEventMapping.swiftapps/ios/ADE/Views/Work/WorkModels.swiftapps/ios/ADE/Views/Work/WorkPlanComposerViews.swiftapps/ios/ADE/Views/Work/WorkSessionDestinationView.swiftapps/ios/ADE/Views/Work/WorkStatusAndFormattingHelpers.swiftapps/ios/ADE/Views/Work/WorkTimelineHelpers.swiftapps/ios/ADETests/ADETests.swift
💤 Files with no reviewable changes (2)
- apps/ios/ADE/Views/Lanes/LaneHelpers.swift
- apps/ios/ADE/Views/Work/WorkChatComposerAndInputViews.swift
…ng, wait-loop early exits - syncHostService: unresolvable explicit-foreign chat_subscribe now fails closed (empty snapshot) instead of falling back to a local session with the same id; rootPath-only payloads naming the host's own project route local - rosterBuilder: drop the always-false resolve equality check (the startsWith containment guard is the real defense) - iOS: secure-response questions hide their option list and never surface the chosen option via the resolution pill or accessibility text - iOS: hub hydration wait exits early when the connection has settled into disconnected/error instead of spinning the full 6s timeout - LaneSectionDisclosure: explicit zero-arg init 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: 010c021b02
ℹ️ 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".
Offline-queued commands targeting a foreign project (cross-project quick-look chat sends) could never drain: the flush loop only picked operations matching the ACTIVE project, and replayed without the stored scope. Command envelopes route cross-project on the host, so drain them on a host match and replay with the operation's own projectId/rootPath; file requests stay project-gated. 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: 3b46cdd3aa
ℹ️ 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".
…findings - notModified lane-detail shells skip presence decoration (decorating the fieldless shell dereferenced detail.lane and failed the command) - conditional signatures fold in a lane-presence stamp so a presence-only change (another device opening a lane) invalidates cached copies instead of serving notModified with stale devicesOpen - rejected foreign chat subscriptions never register in the pump (and foreign quick-look subscriptions are dropped from shared-listener handoff snapshots) so the pump can never stream a same-id local session's transcript 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: 9230528408
ℹ️ 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".
- A CLI session created from the hub toast carried no toolType, so the cross-project branch treated it as a chat quick-look and streamed a chat transcript that CLI sessions never write. The stub now carries the CLI marker and non-chat sessions take the activation path (terminal UI). - The lane row's presence icon derives from device platforms; the render signature hashed only the count, so a same-count platform swap left a stale icon. Hash the sorted platform list (+ regression coverage). 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: 0dd9e1eac5
ℹ️ 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".
…back; scoped session meta - Quick-look reads (summary, canonical history, older-page loads) no longer issue project-scoped commands that boot the foreign runtime — the chat_subscribe snapshot + live tail serve the read path; sends/approvals still route scoped (a mutation legitimately needs the runtime) - Foreign transcript resolver falls back to the legacy <transcripts>/<id>.chat.jsonl location so older sessions aren't blank - work.updateSessionMeta (rename) and work.stopRuntime route to the session's project scope instead of the active project Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
Actionable comments posted: 2
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/WorkSessionDestinationView.swift (1)
318-321: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winEnforce the cross-project read-only contract at the send gates.
isCrossProjectis documented as a read-only quick look, but compose/send eligibility still ignores it. If aninitialSessionis present, the composer can still send/steer into the scoped foreign project.Proposed fix
var canComposeChatMessages: Bool { - session != nil || initialSession != nil + !isCrossProject && (session != nil || initialSession != nil) } var canSendChatMessages: Bool { + guard !isCrossProject else { return false } workChatCanSendMessages( isLive: isLive, hostReachable: hostReachable, chatSendQueueable: syncService.isRemoteActionQueueable("chat.send") ) }🤖 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/WorkSessionDestinationView.swift` around lines 318 - 321, `WorkSessionDestinationView` currently treats `isCrossProject` as informational only, so compose/send paths can still proceed when `initialSession` is set. Update the send-eligibility logic and any compose/submit gate in this view to explicitly block sending whenever `crossProjectContext` is non-nil, even if `initialSession` exists, and make sure the UI/actions driven by `isCrossProject` respect this read-only 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/sync/syncHostService.ts`:
- Around line 4411-4417: Local replay/live chat events are leaking into foreign
subscriptions because the subscription logic in syncHostService still treats
everything as keyed only by sessionId. Update the handling around
resolveForeignChatScope so foreign quick-look sessions are registered and
matched by the resolved foreign transcript path, not the local session id, and
make sure rejected scopes still remove the session as they do today. Also guard
the live event path in broadcastChatEvent and the replay buffer lookup so only
the resolved foreign transcript path is used for foreign subscriptions,
preventing collisions with local session ids.
In `@apps/ios/ADE/Views/Work/WorkSessionDestinationView.swift`:
- Around line 1237-1246: The `shouldHydrateCanonicalEventTail` guard only skips
canonical history, but `loadTranscript` can still continue into
`fetchChatTranscriptPage(...)` and `subscribeTerminal(...)` for cross-project
quick look. Update `WorkSessionDestinationView` so the `loadTranscript` flow
returns or branches early when `isCrossProject` quick look is active, preventing
any remaining host transcript fetches and keeping the mode limited to
snapshot/tail subscription only.
---
Outside diff comments:
In `@apps/ios/ADE/Views/Work/WorkSessionDestinationView.swift`:
- Around line 318-321: `WorkSessionDestinationView` currently treats
`isCrossProject` as informational only, so compose/send paths can still proceed
when `initialSession` is set. Update the send-eligibility logic and any
compose/submit gate in this view to explicitly block sending whenever
`crossProjectContext` is non-nil, even if `initialSession` exists, and make sure
the UI/actions driven by `isCrossProject` respect this read-only 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: bc359de4-a6cc-4abf-ba62-90b424c549d2
📒 Files selected for processing (10)
apps/ade-cli/src/services/sync/rosterBuilder.tsapps/ade-cli/src/services/sync/syncHostService.tsapps/ade-cli/src/services/sync/syncRemoteCommandService.tsapps/ade-cli/src/services/sync/syncService.tsapps/ios/ADE/Services/SyncService.swiftapps/ios/ADE/Views/Hub/HubScreen+ChatNavigation.swiftapps/ios/ADE/Views/Hub/HubScreen.swiftapps/ios/ADE/Views/Lanes/LaneComponents.swiftapps/ios/ADE/Views/Work/WorkSessionDestinationView.swiftapps/ios/ADETests/ADETests.swift
🚧 Files skipped from review as they are similar to previous changes (7)
- apps/ade-cli/src/services/sync/syncService.ts
- apps/ade-cli/src/services/sync/rosterBuilder.ts
- apps/ios/ADE/Views/Hub/HubScreen+ChatNavigation.swift
- apps/ade-cli/src/services/sync/syncRemoteCommandService.ts
- apps/ios/ADE/Views/Lanes/LaneComponents.swift
- apps/ios/ADETests/ADETests.swift
- apps/ios/ADE/Services/SyncService.swift
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 02e8d52950
ℹ️ 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".
…ast read-boot vectors - broadcastChatEvent skips peers whose subscription for the id is a foreign quick-look, and foreign subscribes never resume from the active project's replay buffers — a colliding session id can no longer splice local live events into a foreign feed - quick-look also skips the chat.getTranscript fallback fetch and terminal subscribe (both active-project/boot vectors); reads stay on the chat_subscribe snapshot + pump Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
For unbooted projects, terminal_sessions.status can be persisted as idle (it is the DB default and normal chat state), but this maps every non-running/non-failed row to ended. The new hub roster will therefore show idle chats in unopened projects as ended until the project is booted, hiding the true idle/resumable state; return idle when the row status is idle.
ℹ️ 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".
The injected-service path (syncService.ts) already salts conditional lane signatures with the presence stamp; the fallback createSyncRemoteCommandService inside createSyncHostService did not, so embedders on that path could serve notModified with stale devicesOpen. Share one hoisted stamp helper. 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: 2cc2a9f435
ℹ️ 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".
| const foreignTranscriptPath = peer.foreignChatTranscriptPaths.get(sessionId); | ||
| const transcriptPath = foreignTranscriptPath ?? args.sessionService.get(sessionId)?.transcriptPath; |
There was a problem hiding this comment.
Keep foreign pump events out of local replay buffers
When a peer is in cross-project quick-look mode, these lines make the existing transcript pump read a foreign project's JSONL, but the loop below still calls recordChatEventSeq(event), which writes into the shared chatEventReplayBuffers keyed only by event.sessionId. If that foreign project has a session id that collides with an active-project session, a later local chat_subscribe can replay those foreign entries from the active project's buffer and splice another project's transcript into the local resume stream; keep foreign quick-look sequencing separate from the active-project replay buffer.
Useful? React with 👍 / 👎.
Summary
iOS hub: inline keyboard composer (sheet drawer removed) with draft/settings surviving collapse, scrim + swipe/tap-outside dismissal; project-card running/attention badges removed; lane rows show only chat count; hub chat-open now waits for real project hydration — no more blank transcripts or "Session unavailable".
Chat cards & composer: file-change approvals get an actionable "Provider · Approval" badge above the composer (Accept / Accept all / Decline); question/approval/plan cards redesigned (provider-logo headers, clean option rows with the chosen answer, compact resolved chips, plan preview + shared full-plan sheet); status-based composer locks and phantom "Waiting for prompt details" banners removed; chat-event snapshot decode hardened (
@ADELossyArray+ missing notice kinds) so one unknown event can no longer wipe a whole transcript; cross-enum plan event mapping fixed (labeled bindings).Cross-project chat quick-look:
chat_subscribe/chat_unsubscribeand history accept an optional project override, capability-gated by acrossProjectChathello feature flag; the brain streams a foreign lane's transcript read-only (path-confined resolver, no runtime boot); the phone opens hub chats without switching projects and falls back to the activation flow on older brains.Codex plan approval now transitions the session straight to full access (was edit mode).
Lanes tab: rebase tag removed from list cards (kept in detail); collapsible Files/Stashes/History with content-aware defaults; wrapping header/title/chips; commit row split from secondary actions; manage-sheet desktop parity (+ adopt-attached). Perf: no-op
lane_state_snapshotswrites suppressed (conditional CRR upsert — kills the 400ms changeset-pump list-reload churn), sha256 signature /ifNoneMatch/notModifiedconditional lane list & detail responses, per-lane detail invalidation, per-row render signatures.Verification
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Greptile Summary
This PR delivers three large blocks of work: a cross-project chat "quick-look" that streams a foreign project's transcript read-only without a project switch, conditional lane list/detail responses with sha256 +
ifNoneMatchcaching to suppress the 400 ms changeset-pump churn on no-op writes, and a broad iOS chat/hub UI overhaul (new plan/question/approval cards, inline hub composer,@ADELossyArraydecode hardening).chat_subscribenow accepts optionalprojectId/projectRootPath; the host resolver is the path-confined security boundary (session-id allowlist, startsWith guard), and the broadcast pump correctly skips foreign quick-look peers (peer.foreignChatTranscriptPaths.has(event.sessionId)guard — fixing the live-event collision raised in the prior review).WHEREclause suppresses no-oplane_state_snapshotswrites;respondWithSignatureaddssignature/notModifiedto lane list and detail responses; per-lanelaneDetailRevisionsreplaces the global fan-out so an unrelated lane's cache write no longer reloads every open detail screen.AgentChatNoticeKindnow falls back to.infofor unrecognised values, and@ADELossyArraydrops bad envelope elements instead of discarding the whole transcript; the labeled-binding fix inWorkEventMapping.swiftcorrects a silentturnId/explanationswap that caused plan cards to stop merging correctly.Confidence Score: 4/5
Safe to merge for the lane caching and decode hardening changes; the cross-project quick-look feature is gated behind a hello capability flag so older brains are unaffected, but the outstanding question about whether the iOS composer is intentionally left active for foreign sessions should be confirmed before shipping that path to users.
The broadcast-isolation fix directly addresses the most critical correctness gap from the prior review. The labeled-binding fix in WorkEventMapping, the no-op upsert WHERE clause, and the lossy-array/noticeKind hardening are all well-targeted and tested. The cross-project scope routing is consistently applied across every chat command in SyncService. The one open point — whether a user viewing a hub quick-look can send messages into the foreign project's runtime via the still-wired composer — was raised in the prior round and remains unconfirmed.
apps/ios/ADE/Views/Work/WorkSessionDestinationView.swift and apps/ios/ADE/Views/Work/WorkChatSessionView.swift — the isCrossProject flag gates transcript/artifact reads but the composer send/approve path is still wired through chatCommandScope to the foreign project.
Important Files Changed
Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant Phone as iOS Client participant Host as Sync Host (Brain) participant FS as Foreign Project FS Note over Phone,Host: Cross-Project Chat Quick-Look Phone->>Host: hello Host-->>Phone: hello_ok (crossProjectChat: enabled) Phone->>Phone: setCrossProjectChatScope(sessionId, foreignProjectId) Phone->>Host: "chat_subscribe { sessionId, projectId: foreign }" Host->>Host: resolveForeignChatScope → foreign Host->>Host: foreignChatProvider.resolveTranscriptPath() [security boundary] Host->>FS: readForeignChatSnapshot(transcriptPath, maxBytes) FS-->>Host: events[] Host-->>Phone: "chat_subscribe_ack { events, truncated }" loop Transcript pump 200ms Host->>FS: readChatTranscriptEventsSince(foreignPath, offset) FS-->>Host: new events Host-->>Phone: chat_event via pump only end Phone->>Host: "chat_unsubscribe { sessionId }" Host->>Host: foreignChatTranscriptPaths.delete(sessionId) Note over Phone,Host: Conditional Lane Detail ifNoneMatch Phone->>Host: "lanes.getDetail { laneId, ifNoneMatch: abc123 }" Host->>Host: buildLaneDetailPayload + sha256 + presenceStamp alt signature unchanged Host-->>Phone: notModified true + signature Phone->>Phone: serve cached DB row else signature changed Host-->>Phone: full payload + new signature Phone->>Phone: replaceLaneDetail + cache signature end%%{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"}}}%% sequenceDiagram participant Phone as iOS Client participant Host as Sync Host (Brain) participant FS as Foreign Project FS Note over Phone,Host: Cross-Project Chat Quick-Look Phone->>Host: hello Host-->>Phone: hello_ok (crossProjectChat: enabled) Phone->>Phone: setCrossProjectChatScope(sessionId, foreignProjectId) Phone->>Host: "chat_subscribe { sessionId, projectId: foreign }" Host->>Host: resolveForeignChatScope → foreign Host->>Host: foreignChatProvider.resolveTranscriptPath() [security boundary] Host->>FS: readForeignChatSnapshot(transcriptPath, maxBytes) FS-->>Host: events[] Host-->>Phone: "chat_subscribe_ack { events, truncated }" loop Transcript pump 200ms Host->>FS: readChatTranscriptEventsSince(foreignPath, offset) FS-->>Host: new events Host-->>Phone: chat_event via pump only end Phone->>Host: "chat_unsubscribe { sessionId }" Host->>Host: foreignChatTranscriptPaths.delete(sessionId) Note over Phone,Host: Conditional Lane Detail ifNoneMatch Phone->>Host: "lanes.getDetail { laneId, ifNoneMatch: abc123 }" Host->>Host: buildLaneDetailPayload + sha256 + presenceStamp alt signature unchanged Host-->>Phone: notModified true + signature Phone->>Phone: serve cached DB row else signature changed Host-->>Phone: full payload + new signature Phone->>Phone: replaceLaneDetail + cache signature endComments Outside Diff (5)
apps/ade-cli/src/services/sync/syncHostService.ts, line 3265-3267 (link)broadcastChatEventonly checkssessionId, so a peer quick-looking a foreign transcript whose id collides with an active-project chat will receive this host project's livechat_events. The subscribe path deliberately avoids falling back on same-id local sessions, but this broadcast bypassesforeignChatTranscriptPaths; filter these peers out or key subscriptions by project scope before sending local events.Prompt To Fix With AI
General comment
projectId/projectRootPathinside thechat_subscribecase, but the common inbound project-scope validation still treatschat_subscribeandchat_unsubscribeas ordinary single-project envelopes. A request withprojectId: foreign-projectis rejected withproject_mismatchbefore the switch reaches the cross-project logic, andchat_unsubscribeis rejected the same way before cleanup can run. This contradicts the PR contract that these commands accept an optional foreign project override.SYNC_HOST_PROJECT_SCOPED_INBOUND_ENVELOPE_TYPESstill includeschat_subscribeandchat_unsubscribe, soresolveSyncHostInboundProjectScope()enforces host project equality for the exact commands that should allow explicit foreign scopes.resolveSyncHostInboundProjectScope()to allowchat_subscribe/chat_unsubscribewith explicit foreign scope through to the command handler while preserving fail-closed behavior insideresolveForeignChatScope()for unregistered foreign projects.General comment
implementationCapabilities.hasCrossProjectImpl=falseandadvertisesFlag=false; opening a foreign hub chat with a cross-project subscription payload still takesactivation-fallback, changesactiveProjectAfterfromproject-activetoproject-foreign, and therefore does not satisfy the claimed read-only quick-look/no-project-switch contract.apps/ade-cli/src/services/sync/syncHostService.tschat_subscribe path still only usesargs.sessionService.get(sessionId)andagentChatService.getChatEventHistory(sessionId)for local sessions. It lacks the claimed foreign scope resolver/transcript path map/foreign snapshot path, and the hello feature detection does not advertisecrossProjectChatfrom a foreign chat provider.features.crossProjectChatwhen a foreign transcript provider exists, resolve explicit foreignprojectId/projectRootPathscopes securely, serve byte-capped snapshots from the foreign transcript JSONL, register foreign transcript paths for the pump, and avoid falling back to active-project local sessions for rejected foreign scopes.apps/ade-cli/src/services/sync/syncHostService.ts, line 3267-3269 (link)broadcastChatEventonly checkssubscribedChatSessionIds, so a cross-project quick-look forsessionIdalso receives active-project live events with the same id. The subscribe path storesforeignChatTranscriptPathsto avoid this collision, but this push path bypasses that guard and can merge another project's transcript into the quick-look until the polling tail catches up.Artifacts
Repro: focused SyncHostService test harness with foreign subscription and active-project broadcast collision
Repro: verbose Vitest output showing the collision harness passed by observing the leaked chat_event
Prompt To Fix With AI
apps/ios/ADE/Views/Work/WorkSessionDestinationView.swift, line 1011-1012 (link)isCrossProjectsessions are documented as read-only, but these flags still enable the composer and wireonSend/approval handlers for foreign chats. BecauseSyncServicenow routeschat.send,chat.approve, and question responses throughchatCommandScope, opening a hub quick-look can mutate the foreign session instead of only streaming its transcript. Gate compose/send/actions off whenisCrossProjectis true.Prompt To Fix With AI
Reviews (8): Last reviewed commit: "Wire lane presence stamp into the host's..." | Re-trigger Greptile