Add Lane Modal Button Toast#689
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
📝 WalkthroughWalkthroughThis PR adds lane lifecycle event emission (create/archive/delete) propagated from laneService through desktop main/preload/IPC and buffered in ade-cli; adds ade-cli default lane setup (auto template/env init); introduces a renderer toast store and UI wired to lane/rebase events and scattered action toasts; refactors lane creation into a new CreateLaneDialogHost used by LanesPage and SessionListPane; and adds an iOS "Add Lane" sheet flow. ChangesLane lifecycle events
ade-cli default lane setup
Renderer toast system
Create-lane dialog refactor
iOS Add Lane
Estimated code review effort: 4 (Complex) | ~75 minutes 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 |
|
@copilot review but do not make fixes |
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
@codex review |
![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: 0e91364d1e
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3edb6d662a
ℹ️ 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".
| } else if (actionName === "force push") { | ||
| showToast({ title: `Force-pushed ${laneLabel}`, tone: "success" }); | ||
| } else if (actionName === "pull") { | ||
| showToast({ title: `Pulled ${laneLabel}`, tone: "success" }); |
There was a problem hiding this comment.
When the Pull action runs window.ade.git.sync, a merge/rebase that stops with unmerged files resolves instead of throwing (gitOperationsService treats detected conflicts as a successful return), so this new global toast can say the lane was pulled even though the user still has to resolve conflicts. In that conflict scenario, check getConflictState after the sync or suppress the success toast before reporting completion.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 4c62a96 — after a pull, the pane now checks getConflictState and shows a conflicts-remaining error toast instead of the success toast when the sync stopped on merge/rebase conflicts.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/ade-cli/src/tuiClient/app.tsx (1)
9223-9254: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
new-lane-from-unstagedskips default lane setup.Every other lane-creation path in this file (
/new lane, andcreate/createChild/importBranchundersubmitRightForm) now callsrunLaneSetupAfterCreateafter creation, butcreateFromUnstageddoes not. Since this also produces a brand-new worktree, it likely needs the same template/env initialization; otherwise lanes rescued from unstaged work will silently lack the default environment setup that other new lanes get.🔧 Suggested fix
addNotice(`Moved unstaged work to ${created.name}.`, "success"); + runLaneSetupAfterCreate(conn, created); await refreshState();🤖 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/tuiClient/app.tsx` around lines 9223 - 9254, The new-lane-from-unstaged flow in app.tsx creates a brand-new lane but never runs the same post-create initialization used elsewhere. After the conn.action call in the new-lane-from-unstaged branch, invoke runLaneSetupAfterCreate for the created lane, matching the behavior already used by /new lane and the submitRightForm create/createChild/importBranch paths. Keep the existing selection, drawer, and notice updates, but ensure the setup runs before refreshing state so rescued lanes get the default template/env setup.apps/desktop/src/main/services/lanes/laneService.ts (1)
3824-3857: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMissing lifecycle event on importBranch's post-commit recovery path.
When
laneInsertedistrueand a later step throws (e.g.computeLaneStatus), this catch block recovers and returns a validLaneSummary— i.e. the lane was actually created successfully — butbroadcastLifecycleEvent({ type: "lane-created", ... })is never called here, unlike the straight-through success path just above (lines 3816-3823). Users hitting this recovery branch will get a created lane with no "Lane created" toast.🐛 Proposed fix
return toLaneSummary({ row: persistedRow, status: status ?? { dirty: false, ahead: 0, behind: 0, remoteBehind: -1, rebaseInProgress: false }, parentStatus, childCount: 0, stackDepth, }); + // (emit broadcastLifecycleEvent with the summary above before returning)🤖 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/lanes/laneService.ts` around lines 3824 - 3857, The post-commit recovery path in laneService’s importBranch flow returns a valid LaneSummary when laneInserted is true, but it skips the lane-created lifecycle notification. Mirror the successful path by calling broadcastLifecycleEvent with type "lane-created" before returning from this catch branch, using the same lane data/message payload as the normal success case. Keep the recovery behavior intact so the summary is still returned after the event is emitted.
🧹 Nitpick comments (5)
apps/ios/ADE/Views/Work/WorkRootScreen.swift (1)
96-96: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSwiftLint flags
addLaneSheetPresentedas non-private.Matches the existing convention in this struct where other
@Stateproperties (e.g.sessions,filterPanelOpen) are also non-private, so this isn't a new regression, but consider tightening visibility here and across the struct if addressing lint debt.🤖 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.swift` at line 96, SwiftLint is flagging the `WorkRootScreen` state property `addLaneSheetPresented` as non-private; update the `WorkRootScreen` struct to tighten visibility by making this `@State` property private, and if you’re addressing the lint debt consistently, review the other `@State` members in the same struct (such as `sessions` and `filterPanelOpen`) and apply the same visibility convention where appropriate.Source: Linters/SAST tools
apps/ade-cli/src/tuiClient/__tests__/adeApi.test.ts (1)
59-109: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd test coverage for the "failed" setup outcome.
Both tests only cover the "completed"
overallStatus.runLaneSetupAfterCreate(app.tsx) specifically branches onoverallStatus === "failed"to extract the failed step and surface a notice — that path is untested here, so a regression in step/status shape wouldn't be caught.✅ Suggested additional test
it("surfaces a failed overallStatus without throwing", async () => { const connection = { action: async (domain: string, action: string) => { if (action === "listTemplates") return []; if (action === "getDefaultTemplate") return null; if (action === "initEnv") { return { laneId: "lane-1", steps: [{ kind: "env-files", label: "Env files", status: "failed", error: "boom" }], startedAt: "2026-01-01T00:00:00.000Z", overallStatus: "failed", }; } throw new Error(`unexpected action ${action}`); }, } as unknown as AdeCodeConnection; const result = await runDefaultLaneSetup(connection, "lane-1"); expect(result.progress.overallStatus).toBe("failed"); });🤖 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/tuiClient/__tests__/adeApi.test.ts` around lines 59 - 109, Add a test in runDefaultLaneSetup coverage for the failed setup path so the overallStatus "failed" shape is validated, not just "completed". Update the existing describe("runDefaultLaneSetup") block in adeApi.test.ts to include a case where the connection.action mock returns a failed initEnv payload with steps, then assert runDefaultLaneSetup returns progress.overallStatus as "failed" without throwing. Use the existing runDefaultLaneSetup and AdeCodeConnection symbols to keep the test aligned with the branch that runLaneSetupAfterCreate depends on.apps/desktop/src/main/services/lanes/laneService.ts (1)
5492-5569: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
attach()doesn't emit a lane-created lifecycle event.
attach()inserts a new lane row and returns aLaneSummary, functionally the same "a new lane now exists" outcome ascreateWorktreeLane/importBranch, both of which callbroadcastLifecycleEvent({ type: "lane-created", ... }). This path is missing that call, so attaching an existing worktree as a lane won't surface a "Lane created" toast, unlike other creation flows.Consider adding a matching
broadcastLifecycleEventcall before thereturn toLaneSummary(...)at the end ofattach(), for consistency with the other creation paths.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/lanes/laneService.ts` around lines 5492 - 5569, The attach() flow creates and persists a new lane but never emits the same lane-created lifecycle event used by createWorktreeLane and importBranch. Add a broadcastLifecycleEvent call in attach() after the lane row is inserted and before returning toLaneSummary, using the new lane’s row data so attached lanes trigger the same creation toast and lifecycle behavior as the other creation paths.apps/desktop/src/renderer/components/app/toast/ToastStack.tsx (2)
82-86: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard the action
onClickso dismissal always happens.If
toast.action.onClick()throws,dismissToast(toast.id)is skipped and the toast stays stuck.🛡️ Proposed fix
onClick={() => { - toast.action?.onClick(); - dismissToast(toast.id); + try { + toast.action?.onClick(); + } finally { + dismissToast(toast.id); + } }}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/components/app/toast/ToastStack.tsx` around lines 82 - 86, The toast action handler in ToastStack should ensure dismissToast(toast.id) runs even if toast.action?.onClick() throws. Update the onClick callback for the toast action so the action invocation is wrapped in error-safe flow, and make dismissal happen in a finally-like path so the toast is always removed. Use the ToastStack component and its toast.action/onClick and dismissToast symbols to locate the handler.
46-55: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a live region so toasts are announced to screen readers.
Toast cards render without
role/aria-live, so screen-reader users won't be notified when a new toast appears (e.g. push/rebase success or failure).♿ Proposed fix
<div key={toast.id} + role="status" + aria-live="polite" className={cn( "ade-toast-enter pointer-events-auto overflow-hidden rounded-xl border px-3 py-3 shadow-float backdrop-blur", tone.panel, )}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/components/app/toast/ToastStack.tsx` around lines 46 - 55, The toast cards rendered by ToastStack are missing an accessibility live region, so screen readers won’t announce new notifications. Update the toast container rendered in ToastStack (where each toast <div> is created) to expose appropriate live-region semantics by adding the right role and aria-live attributes, and ensure the toast content is announced when new items appear without changing the existing pause/resume behavior.
🤖 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.
Outside diff comments:
In `@apps/ade-cli/src/tuiClient/app.tsx`:
- Around line 9223-9254: The new-lane-from-unstaged flow in app.tsx creates a
brand-new lane but never runs the same post-create initialization used
elsewhere. After the conn.action call in the new-lane-from-unstaged branch,
invoke runLaneSetupAfterCreate for the created lane, matching the behavior
already used by /new lane and the submitRightForm
create/createChild/importBranch paths. Keep the existing selection, drawer, and
notice updates, but ensure the setup runs before refreshing state so rescued
lanes get the default template/env setup.
In `@apps/desktop/src/main/services/lanes/laneService.ts`:
- Around line 3824-3857: The post-commit recovery path in laneService’s
importBranch flow returns a valid LaneSummary when laneInserted is true, but it
skips the lane-created lifecycle notification. Mirror the successful path by
calling broadcastLifecycleEvent with type "lane-created" before returning from
this catch branch, using the same lane data/message payload as the normal
success case. Keep the recovery behavior intact so the summary is still returned
after the event is emitted.
---
Nitpick comments:
In `@apps/ade-cli/src/tuiClient/__tests__/adeApi.test.ts`:
- Around line 59-109: Add a test in runDefaultLaneSetup coverage for the failed
setup path so the overallStatus "failed" shape is validated, not just
"completed". Update the existing describe("runDefaultLaneSetup") block in
adeApi.test.ts to include a case where the connection.action mock returns a
failed initEnv payload with steps, then assert runDefaultLaneSetup returns
progress.overallStatus as "failed" without throwing. Use the existing
runDefaultLaneSetup and AdeCodeConnection symbols to keep the test aligned with
the branch that runLaneSetupAfterCreate depends on.
In `@apps/desktop/src/main/services/lanes/laneService.ts`:
- Around line 5492-5569: The attach() flow creates and persists a new lane but
never emits the same lane-created lifecycle event used by createWorktreeLane and
importBranch. Add a broadcastLifecycleEvent call in attach() after the lane row
is inserted and before returning toLaneSummary, using the new lane’s row data so
attached lanes trigger the same creation toast and lifecycle behavior as the
other creation paths.
In `@apps/desktop/src/renderer/components/app/toast/ToastStack.tsx`:
- Around line 82-86: The toast action handler in ToastStack should ensure
dismissToast(toast.id) runs even if toast.action?.onClick() throws. Update the
onClick callback for the toast action so the action invocation is wrapped in
error-safe flow, and make dismissal happen in a finally-like path so the toast
is always removed. Use the ToastStack component and its toast.action/onClick and
dismissToast symbols to locate the handler.
- Around line 46-55: The toast cards rendered by ToastStack are missing an
accessibility live region, so screen readers won’t announce new notifications.
Update the toast container rendered in ToastStack (where each toast <div> is
created) to expose appropriate live-region semantics by adding the right role
and aria-live attributes, and ensure the toast content is announced when new
items appear without changing the existing pause/resume behavior.
In `@apps/ios/ADE/Views/Work/WorkRootScreen.swift`:
- Line 96: SwiftLint is flagging the `WorkRootScreen` state property
`addLaneSheetPresented` as non-private; update the `WorkRootScreen` struct to
tighten visibility by making this `@State` property private, and if you’re
addressing the lint debt consistently, review the other `@State` members in the
same struct (such as `sessions` and `filterPanelOpen`) and apply the same
visibility convention where appropriate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 850275af-7798-4e07-b6e2-70e89395d87f
⛔ Files ignored due to path filters (6)
docs/ARCHITECTURE.mdis excluded by!docs/**docs/features/lanes/README.mdis excluded by!docs/**docs/features/lanes/runtime.mdis excluded by!docs/**docs/features/terminals-and-sessions/README.mdis excluded by!docs/**docs/features/terminals-and-sessions/ui-surfaces.mdis excluded by!docs/**docs/perf/work-tab-action-inventory.mdis excluded by!docs/**
📒 Files selected for processing (29)
apps/ade-cli/src/bootstrap.tsapps/ade-cli/src/tuiClient/__tests__/adeApi.test.tsapps/ade-cli/src/tuiClient/adeApi.tsapps/ade-cli/src/tuiClient/app.tsxapps/desktop/src/main/main.tsapps/desktop/src/main/services/lanes/laneService.tsapps/desktop/src/preload/global.d.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/browserMock.tsapps/desktop/src/renderer/components/app/AppShell.aiStatus.test.tsxapps/desktop/src/renderer/components/app/AppShell.tsxapps/desktop/src/renderer/components/app/toast/ToastStack.tsxapps/desktop/src/renderer/components/app/toast/toastStore.test.tsapps/desktop/src/renderer/components/app/toast/toastStore.tsapps/desktop/src/renderer/components/app/toast/useLaneEventToasts.tsapps/desktop/src/renderer/components/graph/WorkspaceGraphPage.tsxapps/desktop/src/renderer/components/history/historyLaneActions.tsapps/desktop/src/renderer/components/lanes/CreateLaneDialog.tsxapps/desktop/src/renderer/components/lanes/CreateLaneDialogHost.tsxapps/desktop/src/renderer/components/lanes/LaneDialogShell.tsxapps/desktop/src/renderer/components/lanes/LaneGitActionsPane.tsxapps/desktop/src/renderer/components/lanes/LanesPage.tsxapps/desktop/src/renderer/components/terminals/SessionListPane.tsxapps/desktop/src/renderer/index.cssapps/desktop/src/shared/ipc.tsapps/desktop/src/shared/types/lanes.tsapps/ios/ADE/Views/Work/WorkPreviews.swiftapps/ios/ADE/Views/Work/WorkRootComponents.swiftapps/ios/ADE/Views/Work/WorkRootScreen.swift
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 participant
Summary
Describe the change.
What Changed
Key files and behaviors.
Validation
How you tested.
Risks
Anything to watch.
Summary by CodeRabbit
New Features
Bug Fixes
Greptile Summary
This PR wires up a shared toast store across the desktop app, CLI, and iOS, surfacing lane lifecycle notifications (created, archived, deleted), background env-setup failures with retryable sticky toasts, and success toasts for push/sync/rebase actions in
LaneGitActionsPaneandhistoryLaneActions. The create-lane dialog logic is extracted fromLanesPageinto a standaloneCreateLaneDialogHostwith two post-create behaviours (close-on-createfor the Work sidebar,stay-open-setupfor the Lanes tab), and iOS gets a new Add Lane button in the Work sidebar.toastStore+useLaneEventToasts+ToastStackprovide a centralized, timer-managed toast system with pause-on-hover, capped at 4 concurrent toasts, backed by full test coverage.CreateLaneDialogHostowns all create-lane form state, orchestratesclose-on-create(env-setup runs detached) andstay-open-setup(dialog stays open, streams progress), surfacing retryable failure toasts for the former.runDefaultLaneSetupwhich re-fetches the template list on each call, providing a fresh-validation fallback if the default template was deleted.Confidence Score: 5/5
Safe to merge; the change is well-structured with full test coverage for the new toast store and only one edge-case quality gap in the detached setup retry path.
The new toast system, lifecycle event plumbing, and CreateLaneDialogHost refactor are all correct and well-tested. The only noteworthy gap is that the detached background setup retry in close-on-create mode passes the same potentially-stale templateId on every retry without re-validating, which can leave the user stuck in a retry loop if the template is deleted after lane creation — but this requires an unlikely race and does not affect data integrity.
apps/desktop/src/renderer/components/lanes/CreateLaneDialogHost.tsx — the applyLaneEnvSetup helper in the detached setup path.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[User clicks Add Lane] --> B{Which surface?} B -->|Work sidebar / CLI| C["close-on-create mode"] B -->|Lanes tab| D["stay-open-setup mode"] C --> E[CreateLaneDialogHost opens] D --> F[CreateLaneDialogHost opens] E --> G[handleCreateSubmit] F --> G G --> H[create / createChild / importBranch] H --> I["laneService.broadcastLifecycleEvent(lane-created)"] I --> J["IPC → renderer onLifecycleEvent"] J --> K["useLaneEventToasts → showToast (Lane created)"] H --> L{behavior?} L -->|close-on-create| M[resetState + onOpenChange false] M --> N["runDetachedLaneSetup (background)"] N --> O{setup result} O -->|failed| P["showSetupFailureToast (sticky, Retry)"] O -->|success| Q["dismissToast (failure toast)"] L -->|stay-open-setup| R["runSetupForCreatedLane (in-dialog streaming)"] R --> S{setup result} S -->|failed| T[in-dialog error + Retry button] S -->|success| U[resetState + close dialog] K -.->|fires for both modes| V["⚠ toast + open dialog visible simultaneously in stay-open mode (known)"]%%{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[User clicks Add Lane] --> B{Which surface?} B -->|Work sidebar / CLI| C["close-on-create mode"] B -->|Lanes tab| D["stay-open-setup mode"] C --> E[CreateLaneDialogHost opens] D --> F[CreateLaneDialogHost opens] E --> G[handleCreateSubmit] F --> G G --> H[create / createChild / importBranch] H --> I["laneService.broadcastLifecycleEvent(lane-created)"] I --> J["IPC → renderer onLifecycleEvent"] J --> K["useLaneEventToasts → showToast (Lane created)"] H --> L{behavior?} L -->|close-on-create| M[resetState + onOpenChange false] M --> N["runDetachedLaneSetup (background)"] N --> O{setup result} O -->|failed| P["showSetupFailureToast (sticky, Retry)"] O -->|success| Q["dismissToast (failure toast)"] L -->|stay-open-setup| R["runSetupForCreatedLane (in-dialog streaming)"] R --> S{setup result} S -->|failed| T[in-dialog error + Retry button] S -->|success| U[resetState + close dialog] K -.->|fires for both modes| V["⚠ toast + open dialog visible simultaneously in stay-open mode (known)"]Comments Outside Diff (1)
apps/desktop/src/renderer/components/app/toast/useLaneEventToasts.ts, line 183-197 (link)useLaneEventToastssubscribes globally, so it fires a "Lane created" success toast for everylane-createdlifecycle event — including those triggered from the Lanes tab whereCreateLaneDialogHostis instay-open-setupmode and the dialog remains open streaming env-setup progress. The user then sees both the open dialog (still showing setup status) and a corner toast with a "View" action pointing at the same lane. Clicking "View" navigates away while the setup dialog is still mounted. This is harmless but visually redundant in the Lanes tab flow. Consider suppressing the toast when the create dialog is currently open, or scoping it to theclose-on-createpath only.Prompt To Fix With AI
Reviews (4): Last reviewed commit: "ship: suppress pull success toast when c..." | Re-trigger Greptile