fix(dashboard): stop Planning Mode retry loop, make AI sessions multi-tab#2101
Merged
Conversation
Contributor
|
Warning Review limit reached
Next review available in: 11 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: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (49)
📝 WalkthroughWalkthroughPlanning and onboarding flows now clear answered questions before continuing or retrying, restore persisted questions only while awaiting input, and remove per-tab locking and ownership synchronization from session APIs, storage, routes, and UI. Regression tests cover retries, deepening, restoration, cancellation, and multi-tab behavior. ChangesPlanning and onboarding lifecycle
Lock-free session architecture
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant PlanningModeModal
participant PlanningAPI
participant SessionStore
participant SessionSSE
PlanningModeModal->>PlanningAPI: submit, retry, stop, or cancel without tabId
PlanningAPI->>SessionStore: update persisted session
SessionStore-->>SessionSSE: emit session update
SessionSSE-->>PlanningModeModal: stream session state
PlanningModeModal->>SessionStore: poll current session when SSE events are missed
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Contributor
Greptile SummaryThis PR fixes answered-question retries and makes AI interview sessions lock-free across tabs. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (6): Last reviewed commit: "fix: address PR #2101 review (onboarding..." | Re-trigger Greptile |
![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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/dashboard/src/planning.ts (1)
830-841: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard restored questions on recovered rows.
recoverStaleSessions()can flip stalegeneratingrows toawaiting_inputbeforebuildSessionFromRow()runs, so pre-fix rows with an already-answeredcurrentQuestionstill get restored and can re-trigger the SSE retry loop after restart. Gate restoration on the answered-history state, not status alone.🤖 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 `@packages/dashboard/src/planning.ts` around lines 830 - 841, The currentQuestion restoration in buildSessionFromRow must also check the answered-history state, not only row.status === "awaiting_input". Reuse the existing indicator that the question has already been answered to prevent recovered stale rows from restoring and re-emitting their prior question, while preserving restoration for genuinely unanswered awaiting-input rows.
🤖 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 `@packages/dashboard/src/__tests__/planning-answered-question-reemit.test.ts`:
- Around line 115-121: Remove the polling helper waitFor and replace its uses
with deterministic promises exposed by setupAgent for “first question emitted”
and “hung turn entered.” Await those signals directly in the affected tests,
preserving the existing assertions while eliminating the repeated 5 ms timers
and timeout-based synchronization.
---
Outside diff comments:
In `@packages/dashboard/src/planning.ts`:
- Around line 830-841: The currentQuestion restoration in buildSessionFromRow
must also check the answered-history state, not only row.status ===
"awaiting_input". Reuse the existing indicator that the question has already
been answered to prevent recovered stale rows from restoring and re-emitting
their prior question, while preserving restoration for genuinely unanswered
awaiting-input rows.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0f866965-c8ac-480d-9335-c1a151f6492d
📒 Files selected for processing (5)
.changeset/planning-answered-question-retry-loop.mdpackages/dashboard/src/__tests__/agent-onboarding.test.tspackages/dashboard/src/__tests__/planning-answered-question-reemit.test.tspackages/dashboard/src/agent-onboarding.tspackages/dashboard/src/planning.ts
Contributor
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 (1)
packages/dashboard/app/components/PlanningModeModal.tsx (1)
1958-1986: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winReplace manual state cleanup with
resetDetailState().Both task creation completion flows clear the active session selection but handle the right pane state inconsistently.
handleCreateTasksFromBreakdownmanually duplicates 13 lines of state-clearing logic, whilehandleCreateTaskleavesview.typeset to"summary", which can cause a minor state glitch where the modal renders an unselected summary pane if reopened later. CallingresetDetailState()in both handlers guarantees a clean, consistent view reset (including branch settings) when unsetting the active session.
packages/dashboard/app/components/PlanningModeModal.tsx#L1958-L1986: Replace the 13 manually duplicated state-reset lines with a call toresetDetailState()and append it to the dependencies array.packages/dashboard/app/components/PlanningModeModal.tsx#L1890-L1907: Add aresetDetailState()call right beforesetSelectedSessionId(null)and append it to the dependencies array.♻️ Proposed fixes for the state resets
For
handleCreateTasksFromBreakdown:setPlanningSessions((prev) => dedupeSessionsById(prev.filter((s) => s.id !== completedSessionId))); // Reset and close - setInitialPlan(""); - setView({ type: "initial" }); - setError(null); - setResponseHistory([]); - setConversationHistory([]); - setEditedSummary(null); - setStreamingOutput(""); - setPlanningModelProvider(undefined); - setPlanningModelId(undefined); - setPlanningThinkingLevel(""); - setPlanningDepth("medium"); - setCustomQuestionCount(""); - currentSessionIdRef.current = null; + resetDetailState(); setSelectedSessionId(null); handleClose(); } catch (err) { setError(getErrorMessage(err) || t("planning.failedCreateTasks", "Failed to create tasks")); } finally { setIsCreatingFromBreakdown(false); } - }, [baseBranch, branchMode, branchName, handleClose, view, onTasksCreated, projectId, workflowId]); + }, [baseBranch, branchMode, branchName, handleClose, view, onTasksCreated, projectId, workflowId, resetDetailState]);For
handleCreateTask:onTaskCreated(task); // Single-task creation should preserve completed planning history, so // only clear the active selection before closing; keep the sidebar row // in local state to match persisted server truth. + resetDetailState(); setSelectedSessionId(null); handleClose(); } catch (err) { setError(getErrorMessage(err) || t("planning.failedCreateTask", "Failed to create task")); } finally { setIsCreatingTask(false); } - }, [baseBranch, branchMode, branchName, editedSummary, view, projectId, workflowId, onTaskCreated, handleClose]); + }, [baseBranch, branchMode, branchName, editedSummary, view, projectId, workflowId, onTaskCreated, handleClose, resetDetailState]);🤖 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 `@packages/dashboard/app/components/PlanningModeModal.tsx` around lines 1958 - 1986, In packages/dashboard/app/components/PlanningModeModal.tsx at lines 1958-1986, update handleCreateTasksFromBreakdown to replace the duplicated detail-state cleanup with resetDetailState(), preserving session-list cleanup and modal reset, and add resetDetailState to its dependencies. At lines 1890-1907, update handleCreateTask to call resetDetailState() immediately before setSelectedSessionId(null), and add it to that handler’s dependencies.
🤖 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 `@packages/dashboard/app/components/PlanningModeModal.tsx`:
- Around line 1958-1986: In
packages/dashboard/app/components/PlanningModeModal.tsx at lines 1958-1986,
update handleCreateTasksFromBreakdown to replace the duplicated detail-state
cleanup with resetDetailState(), preserving session-list cleanup and modal
reset, and add resetDetailState to its dependencies. At lines 1890-1907, update
handleCreateTask to call resetDetailState() immediately before
setSelectedSessionId(null), and add it to that handler’s dependencies.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fbb183dc-5536-429f-a726-74634c8c95a9
📒 Files selected for processing (12)
.changeset/planning-multi-tab-lock-free.mdpackages/dashboard/app/api/legacy.tspackages/dashboard/app/components/PlanningModeModal.tsxpackages/dashboard/app/components/__tests__/PlanningModeModal.autosize.test.tsxpackages/dashboard/app/components/__tests__/PlanningModeModal.initial.test.tsxpackages/dashboard/app/components/__tests__/PlanningModeModal.planning-flow.test.tsxpackages/dashboard/app/components/__tests__/PlanningModeModal.ui-interactions.test.tsxpackages/dashboard/app/hooks/__tests__/useBackgroundSessions.test.tspackages/dashboard/app/hooks/useBackgroundSessions.tspackages/dashboard/src/__tests__/planning-generation-cancellation.test.tspackages/dashboard/src/__tests__/routes-planning.test.tspackages/dashboard/src/routes/register-planning-subtask-routes.ts
💤 Files with no reviewable changes (3)
- packages/dashboard/app/components/tests/PlanningModeModal.initial.test.tsx
- packages/dashboard/app/components/tests/PlanningModeModal.autosize.test.tsx
- packages/dashboard/app/components/tests/PlanningModeModal.ui-interactions.test.tsx
gsxdsm
changed the title
gsxdsm
force-pushed
the
feature/improve-planning
branch
from
July 15, 2026 01:14
87917c4 to
a402a9c
Compare
gsxdsm
added a commit
that referenced
this pull request
Jul 15, 2026
…stic tests) Mirror Planning Mode's successful respond contract when onboarding generation fails after an answer, and replace real 5ms polling in planning re-emit tests with deterministic setupAgent promises.
Contributor
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 (6)
packages/dashboard/app/components/MissionManager.tsx (1)
4218-4222: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate or remove stale FNXC comment.
The
FNXC:MissionDraftDiscardcomment states that the discard confirmation must send the current browser tab ID. Since per-tab session locking has been removed and thediscardMissionInterviewDraftcall no longer includes the tab ID, this comment should be updated to reflect the new behavior or removed entirely.♻️ Proposed fix to remove the stale comment
- /* - FNXC:MissionDraftDiscard 2026-06-24-02:42: - The mission draft Discard confirmation must send the current browser tab id so a draft locked by this tab can be removed while a draft actively owned by another tab returns the lock warning and stays visible. - */ await discardMissionInterviewDraft(sessionId, projectId);🤖 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 `@packages/dashboard/app/components/MissionManager.tsx` around lines 4218 - 4222, Remove the stale FNXC:MissionDraftDiscard comment immediately preceding the discardMissionInterviewDraft call, since per-tab locking and the tab ID requirement no longer apply; leave the discard call unchanged.Source: Coding guidelines
packages/dashboard/src/routes/register-planning-subtask-routes.ts (1)
826-828: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGuard
req.bodybefore destructuring
express.json()still leavesreq.bodyundefined for empty or non-JSON requests in Express 5, so these handlers can throw a rawTypeErrorbefore the intendedsessionId/responsesvalidation and return a 500 instead of a 400. Useconst { ... } = req.body ?? {};in:
packages/dashboard/src/routes/register-planning-subtask-routes.ts(/planning/respond,/subtasks/cancel,/planning/cancel)packages/dashboard/src/mission-routes.ts(/interview/respond,/interview/cancel,/milestones/:milestoneId/interview/respond,/slices/:sliceId/interview/respond)🤖 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 `@packages/dashboard/src/routes/register-planning-subtask-routes.ts` around lines 826 - 828, Guard request-body destructuring with an empty-object fallback so missing bodies reach the existing validation and return 400 responses. Update the handlers for /planning/respond, /subtasks/cancel, and /planning/cancel in packages/dashboard/src/routes/register-planning-subtask-routes.ts, and /interview/respond, /interview/cancel, /milestones/:milestoneId/interview/respond, and /slices/:sliceId/interview/respond in packages/dashboard/src/mission-routes.ts; each affected site requires changing its body destructuring to use req.body ?? {}.packages/dashboard/app/components/PlanningModeModal.tsx (4)
672-694: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
draftSessionIdRefis never cleared, leaving a stale draft id after delete/archive/failed-start.
resetDetailStateclears every other piece of session-scoped state (currentSessionIdRef, model selections, etc.) but notdraftSessionIdRef.current.handleNewSession(line 1416) andhandleClose(line 1717) both explicitly clear it before/alongside their own reset, which shows this is an intentional invariant elsewhere — buthandleDeleteSessionandhandleArchiveSessionroute throughresetDetailStatewhen the deleted/archived session is the currently-selected one, and neither clearsdraftSessionIdReffirst. If that session was adraft, the ref keeps pointing at now-deleted/hidden id. Consequences: the composer's auto-draft-create-on-type guard (if (draftSessionIdRef.current || ...) return;) silently stops creating new drafts, and a subsequent "Start Planning" click passes the stale id intostartPlanningStreaming(line 1079).handleStartPlanning's own catch block (1087-1092) has the identical gap on a failed start.🐛 Clear the ref at the shared reset point
const resetDetailState = useCallback(() => { setInitialPlan(""); setView({ type: "initial" }); setError(null); setResponseHistory([]); setConversationHistory([]); setEditedSummary(null); setBranchMode("project-default"); setBranchName(""); setBaseBranch(""); setStreamingOutput(""); setIsReconnecting(false); setIsRetrying(false); resetPlanningAutoRetryBudget(); setIsRefiningSummary(false); refineSummaryInFlightRef.current = false; setPlanningModelProvider(undefined); setPlanningModelId(undefined); setPlanningThinkingLevel(""); setPlanningDepth("medium"); setCustomQuestionCount(""); currentSessionIdRef.current = null; + draftSessionIdRef.current = null; }, [resetPlanningAutoRetryBudget]);} catch (err) { setIsReconnecting(false); setError(getErrorMessage(err) || t("planning.failedStartSession", "Failed to start planning session")); setView({ type: "initial" }); currentSessionIdRef.current = null; + draftSessionIdRef.current = null; }Also applies to: 1082-1092
🤖 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 `@packages/dashboard/app/components/PlanningModeModal.tsx` around lines 672 - 694, Update resetDetailState to clear draftSessionIdRef.current alongside currentSessionIdRef.current, ensuring all session-scoped refs are reset when deleting, archiving, or otherwise resetting the selected session. Also clear the ref in handleStartPlanning’s failed-start catch path so a failed draft start cannot leave a stale draft id.
913-1012: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winSame TOCTOU pattern in
startPlanningRetry's session-refresh catch.After
await fetchAiSession(retryTarget.sessionId)in the"not in an error state"recovery branch, the code unconditionally doescurrentSessionIdRef.current = session.id;and then mutatesview/conversationHistory/etc., with no check thatretryTarget.sessionIdis still the session the user is actively viewing. This shares the same root cause as theonErrorhandler above (see that comment) — this will be addressed together in the consolidated comment below.🤖 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 `@packages/dashboard/app/components/PlanningModeModal.tsx` around lines 913 - 1012, Guard the session-refresh recovery in startPlanningRetry by verifying that the fetched session still matches the currently active retryTarget.sessionId before assigning currentSessionIdRef or updating view, conversation history, connection state, or retry state. If the session is stale, exit without applying those updates, consistent with the existing onError TOCTOU protection.
845-894: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winTOCTOU:
onErrormutates shared session state without re-checking staleness.Unlike the sibling
onThinking/onQuestion/onSummaryhandlers in this sameconnectToPlanningStreamcall,onErrornever callsisStaleEvent(). After theawait fetchAiSession(sessionId)(and the furtherawait startPlanningAutoRetryRef.current(...)), the code unconditionally doessetView(...)andcurrentSessionIdRef.current = sessionId(line 892). If the user switches to a different session while this recovery is in flight (e.g. viahandleSelectSession/loadSession), this stale callback will overwrite the newly-selected session's view with the old session's error and hijackcurrentSessionIdRef/the stream connection — and since the erroneous error view isn't tied to the actually-active session, it won't self-correct until some other event happens to fire for the real session.🔒 Add the same staleness guard used by the other handlers
onError: (message) => { const errorMessage = message || t("planning.sessionFailed", "Session failed while contacting the AI."); + if (isStaleEvent()) return; setIsReconnecting(true); (async () => { try { const session = await fetchAiSession(sessionId); + if (isStaleEvent()) return; if ( session && (session.status === "generating" || session.status === "awaiting_input") ) { connectToPlanningStream(sessionId); return; } } catch { // fall through to error view below } + if (isStaleEvent()) return; setIsReconnecting(false); if (await startPlanningAutoRetryRef.current(sessionId, errorMessage)) { return; } setIsRetrying(false); setIsAutoRetrying(false); setIsRefiningSummary(false); refineSummaryInFlightRef.current = false; setError(null); setView((prev) => { if (prev.type === "question" || prev.type === "summary" || prev.type === "error") { return { type: "error", session: prev.session, errorMessage }; } return { type: "error", session: { sessionId, currentQuestion: null, summary: null }, errorMessage, }; }); setStreamingOutput(""); - currentSessionIdRef.current = sessionId; })(); },🤖 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 `@packages/dashboard/app/components/PlanningModeModal.tsx` around lines 845 - 894, Update the onError recovery flow inside connectToPlanningStream to call the existing isStaleEvent() guard before any shared-state mutation, especially after fetchAiSession and startPlanningAutoRetryRef.current complete. If the callback is stale, return without updating the view, retry flags, streaming output, or currentSessionIdRef; preserve the existing handling for the active session.
593-670: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winFallback poll doesn't gate on
isOpen, unlike every other similar effect.Every other subscription/effect in this component that shouldn't run while the modal is hidden explicitly checks
isOpenfirst (e.g. lines 1291, 1306, 1334, 1363). This 8-second fallback poll only checksview.type !== "loading". Since the component doesn't unmount whenisOpenbecomes false (confirmed by the dedicated "reset when closes" effect at 1626-1633, which exists specifically because state/refs persist across close/reopen), closing the modal mid-generation leaves this poll hittingfetchAiSessionevery 8s in the background until the session resolves.⚡ Gate the poll on isOpen too
useEffect(() => { - if (view.type !== "loading") return; + if (!isOpen || view.type !== "loading") return; let cancelled = false; ... const interval = setInterval(tick, 8000); return () => { cancelled = true; clearInterval(interval); }; - }, [projectId, resetPlanningAutoRetryBudget, t, view.type]); + }, [isOpen, projectId, resetPlanningAutoRetryBudget, t, view.type]);🤖 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 `@packages/dashboard/app/components/PlanningModeModal.tsx` around lines 593 - 670, Update the loading fallback poll effect around the tick function to return early when isOpen is false, alongside the existing view.type !== "loading" guard. Include isOpen in the effect dependencies so the interval is stopped and recreated correctly when the modal opens or closes; preserve the existing polling behavior while the modal is open and loading.
🤖 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 `@packages/dashboard/app/components/MissionManager.tsx`:
- Around line 4218-4222: Remove the stale FNXC:MissionDraftDiscard comment
immediately preceding the discardMissionInterviewDraft call, since per-tab
locking and the tab ID requirement no longer apply; leave the discard call
unchanged.
In `@packages/dashboard/app/components/PlanningModeModal.tsx`:
- Around line 672-694: Update resetDetailState to clear
draftSessionIdRef.current alongside currentSessionIdRef.current, ensuring all
session-scoped refs are reset when deleting, archiving, or otherwise resetting
the selected session. Also clear the ref in handleStartPlanning’s failed-start
catch path so a failed draft start cannot leave a stale draft id.
- Around line 913-1012: Guard the session-refresh recovery in startPlanningRetry
by verifying that the fetched session still matches the currently active
retryTarget.sessionId before assigning currentSessionIdRef or updating view,
conversation history, connection state, or retry state. If the session is stale,
exit without applying those updates, consistent with the existing onError TOCTOU
protection.
- Around line 845-894: Update the onError recovery flow inside
connectToPlanningStream to call the existing isStaleEvent() guard before any
shared-state mutation, especially after fetchAiSession and
startPlanningAutoRetryRef.current complete. If the callback is stale, return
without updating the view, retry flags, streaming output, or
currentSessionIdRef; preserve the existing handling for the active session.
- Around line 593-670: Update the loading fallback poll effect around the tick
function to return early when isOpen is false, alongside the existing view.type
!== "loading" guard. Include isOpen in the effect dependencies so the interval
is stopped and recreated correctly when the modal opens or closes; preserve the
existing polling behavior while the modal is open and loading.
In `@packages/dashboard/src/routes/register-planning-subtask-routes.ts`:
- Around line 826-828: Guard request-body destructuring with an empty-object
fallback so missing bodies reach the existing validation and return 400
responses. Update the handlers for /planning/respond, /subtasks/cancel, and
/planning/cancel in
packages/dashboard/src/routes/register-planning-subtask-routes.ts, and
/interview/respond, /interview/cancel,
/milestones/:milestoneId/interview/respond, and
/slices/:sliceId/interview/respond in packages/dashboard/src/mission-routes.ts;
each affected site requires changing its body destructuring to use req.body ??
{}.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bef8e44e-e823-4409-898e-d3d9f5241bc7
📒 Files selected for processing (42)
.changeset/planning-multi-tab-lock-free.mdpackages/core/src/async-ai-session-store.tspackages/core/src/index.tspackages/core/src/postgres/schema/project.tspackages/dashboard/app/__tests__/app-cli-action-wiring.test.tsxpackages/dashboard/app/api/legacy.tspackages/dashboard/app/components/BackgroundTasksIndicator.tsxpackages/dashboard/app/components/MilestoneSliceInterviewModal.tsxpackages/dashboard/app/components/MissionInterviewModal.tsxpackages/dashboard/app/components/MissionManager.tsxpackages/dashboard/app/components/PlanningModeModal.csspackages/dashboard/app/components/PlanningModeModal.tsxpackages/dashboard/app/components/SubtaskBreakdownModal.tsxpackages/dashboard/app/components/__tests__/ExecutorStatusBar.test.tsxpackages/dashboard/app/components/__tests__/MilestoneSliceInterviewModal.test.tsxpackages/dashboard/app/components/__tests__/MissionInterviewModal.test.tsxpackages/dashboard/app/components/__tests__/MissionManager.delete-confirm.test.tsxpackages/dashboard/app/components/__tests__/PlanningModeModal.dedupeSessionsById.test.tspackages/dashboard/app/components/__tests__/PlanningModeModal.planning-flow.test.tsxpackages/dashboard/app/components/__tests__/PlanningModeModal.ui-interactions.test.tsxpackages/dashboard/app/components/__tests__/SessionNotificationBanner.test.tsxpackages/dashboard/app/components/__tests__/SubtaskBreakdownModal.test.tsxpackages/dashboard/app/components/__tests__/utility-mobile.test.tsxpackages/dashboard/app/components/dashboard/__tests__/DashboardBanners.test.tsxpackages/dashboard/app/hooks/__tests__/useAiSessionSync.test.tspackages/dashboard/app/hooks/__tests__/useBackgroundSessions.test.tspackages/dashboard/app/hooks/useAiSessionSync.tspackages/dashboard/app/hooks/useBackgroundSessions.tspackages/dashboard/app/hooks/useSessionLock.tspackages/dashboard/app/utils/__tests__/appLifecycle.test.tspackages/dashboard/app/utils/getSessionTabId.tspackages/dashboard/src/__tests__/planning-answered-question-reemit.test.tspackages/dashboard/src/__tests__/routes-planning-tracking.test.tspackages/dashboard/src/__tests__/routes-planning.test.tspackages/dashboard/src/ai-session-store.tspackages/dashboard/src/milestone-slice-interview.tspackages/dashboard/src/mission-interview.tspackages/dashboard/src/mission-routes.tspackages/dashboard/src/planning.tspackages/dashboard/src/routes.tspackages/dashboard/src/routes/register-planning-subtask-routes.tspackages/dashboard/src/subtask-breakdown.ts
💤 Files with no reviewable changes (20)
- packages/dashboard/app/utils/getSessionTabId.ts
- packages/dashboard/app/components/tests/PlanningModeModal.dedupeSessionsById.test.ts
- packages/dashboard/app/components/tests/ExecutorStatusBar.test.tsx
- packages/dashboard/app/utils/tests/appLifecycle.test.ts
- packages/dashboard/src/milestone-slice-interview.ts
- packages/dashboard/app/tests/app-cli-action-wiring.test.tsx
- packages/dashboard/app/components/dashboard/tests/DashboardBanners.test.tsx
- packages/dashboard/app/components/tests/SessionNotificationBanner.test.tsx
- packages/dashboard/app/components/PlanningModeModal.css
- packages/dashboard/src/tests/routes-planning-tracking.test.ts
- packages/core/src/index.ts
- packages/dashboard/app/hooks/useSessionLock.ts
- packages/dashboard/src/subtask-breakdown.ts
- packages/dashboard/src/mission-interview.ts
- packages/dashboard/app/components/tests/PlanningModeModal.ui-interactions.test.tsx
- packages/dashboard/app/components/tests/utility-mobile.test.tsx
- packages/dashboard/src/tests/planning-answered-question-reemit.test.ts
- packages/dashboard/app/components/tests/MilestoneSliceInterviewModal.test.tsx
- packages/dashboard/src/planning.ts
- packages/dashboard/app/components/tests/PlanningModeModal.planning-flow.test.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- .changeset/planning-multi-tab-lock-free.md
…stions Planning sessions kept session.currentQuestion pointing at the just-answered question through the entire next generation. The SSE stream route's catch-up path re-emits currentQuestion to every fresh connection, and each FN-7946 auto-retry opens one — so after a generation error the client was handed the answered question again, which reset the bounded auto-retry budget and re-showed a stale question, looping forever. Enforce the invariant that currentQuestion is only set while the session is genuinely awaiting input: - submitResponse clears it the moment an answer is accepted (normal turns and the deepening checkpoint), preserving the legacy 200 respond contract on generation failure - retrySession scrubs stale questions persisted by pre-fix builds - buildSessionFromRow only restores questions from awaiting_input rows - didSubmitSameAnswer now compares against the last history entry so the duplicate-submit 409 message survives - agent onboarding gets the same fix (its SSE route also re-emits currentQuestion on connect) and retries with a next-question prompt Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…s-tab locks Remove all cross-tab coordination from planning: the persisted session row is the single source of truth and every tab can read and interact. - /planning/* routes no longer call checkSessionLock or parse tabId; a stale tabId from an older client is ignored, never 409'd. Subtask and mission interview routes keep their existing lock behavior. - PlanningModeModal drops useSessionLock, useAiSessionSync broadcasts (update/completed/lock/unlock/heartbeat), sessionTabId, lockSessionId state, and the Take Control overlay. Tabs stay current via the per-session SSE stream plus global ai_session:updated events that useBackgroundSessions already consumes; concurrent writes resolve via the server's generation-in-progress guard. - Planning API client functions lose their tabId params. - The 8s stuck-poll now resolves the session id inside each tick; the removed lock state was the dependency that previously re-armed it after Start Planning resolved the session id. - Fix pre-existing PG-cutover break in the cancellation test (getSession is async). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Follow-up to the planning multi-tab change, extending it to every AI interview surface. The persisted session row is the single source of truth; any tab may read and interact with any session. Server: - Delete the /ai-sessions/:id/lock, /lock/force, and /lock/beacon routes. - Drop checkSessionLock from every planning, subtask, mission, and milestone/slice route; a tabId from an older client is ignored, never 409'd. All tabId body parsing is gone. - Drop acquireLock/releaseLock/forceAcquireLock/getLockHolder/ releaseStaleLocks from AiSessionStore and their @fusion/core async helpers (acquireAiSessionLock et al) plus core's re-exports. - Remove lockedByTab/lockedAt from AiSessionRow/AiSessionSummary, the upsert SQL, and every session producer. Client: - Delete useSessionLock and the now-orphaned getSessionTabId util. - Strip the Take Control overlay, "active in another tab" banners, and BackgroundTasksIndicator's active-elsewhere gate/confirm/lock badge. - Reduce useAiSessionSync to session-status sync: no activeTabMap, broadcastLock/Unlock/Heartbeat, owningTabId, tab:* messages, or stale-heartbeat sweep. - Drop tabId params from the session API client and remove the lock CSS. The ai_sessions.locked_by_tab/locked_at columns are retained as dead, always-NULL columns with a deprecation note: dropping them is an irreversible migration and older installed binaries still name those columns in their upsert. They can be dropped once no such binary can reach the database. Lock-conflict tests are rewritten to assert the inverse — routes and modals stay fully interactive while another tab "holds" a lock, and no lock API is ever called. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…stic tests) Mirror Planning Mode's successful respond contract when onboarding generation fails after an answer, and replace real 5ms polling in planning re-emit tests with deterministic setupAgent promises.
gsxdsm
force-pushed
the
feature/improve-planning
branch
from
July 15, 2026 01:37
a1d843a to
6fc73a8
Compare
This was referenced Jul 18, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Reported: planning gets stuck in a cycle of retrying and regenerating after a response was already supplied.
After the user answers a planning question,
submitResponsepushed the answer to history but leftsession.currentQuestionpointing at the just-answered question for the whole next generation. The planning SSE route's catch-up path re-emitscurrentQuestionto every fresh connection — and each FN-7946 auto-retry (#2073) opens a fresh connection. So after any generation error:Fix
Invariant:
currentQuestionis only set while the session is genuinely awaiting user input.submitResponseclears it the moment an answer is accepted (normal turns and the deepening checkpoint), while preserving the legacy 200 respond contract on generation failure (the modal ignores the body and lets the SSE error drive recovery).retrySessionscrubs stale questions persisted by pre-fix builds before regenerating.buildSessionFromRowonly restores a question when the persisted row isawaiting_input.didSubmitSameAnswernow compares against the last history entry so the duplicate-submit 409 message survives.currentQuestionon connect); retry now asks the next question instead of re-asking the answered one.Surface enumeration: mission and milestone interviews keep questions the same way but their SSE routes never re-emit on connect, and the auto-retry budget machinery is Planning-Mode-only — planning + onboarding were the two affected surfaces.
Symptom Verification
planning-answered-question-reemit.test.tsassertscurrentQuestionis cleared mid-generation, on generation failure, on retry, and on restore from non-awaiting_inputrows — so the SSE catch-up path has nothing stale to re-emit. All 5 tests fail against pre-fix code and pass with the fix; an onboarding regression test covers the sibling surface.Verification
routes-planning.test.tsfail identically without this change — pre-existing on the branch); all 69PlanningModeModal.planning-flowclient tests pass;tsc --noEmitclean;pnpm check:changesetspasses.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Follow-up: Planning Mode is now multi-tab via DB state (lock-free)
Second commit removes all cross-tab coordination from planning — the persisted session row is the single source of truth and multiple tabs can read and interact with the same session:
/planning/*routes no longer runcheckSessionLockor parsetabId; a staletabIdfrom an older client is ignored instead of 409'd. Subtask/mission interview routes keep their existing lock behavior.PlanningModeModaldropsuseSessionLock, theuseAiSessionSyncBroadcastChannel broadcasts,sessionTabId/lockSessionIdstate, and the "Take Control" overlay. Tabs stay current via the per-session SSE stream plus the globalai_session:updatedeventsuseBackgroundSessionsalready consumes; concurrent writes resolve via the server's generation-in-progress guard (409).tabIdparams.planning-generation-cancellation.test.ts(getSessionis async).Verification: 144 client planning tests and 137 server planning tests pass (the 3 remaining
routes-planning.test.tsfailures are pre-existing on the branch and fail identically without these changes);tsc --noEmitand eslint clean on changed files;pnpm check:changesetspasses. Lock-conflict route tests were rewritten to assert lock-free semantics, plus a new modal test proving a session stays fully interactive with no lock acquisition even when another tab is active.Follow-up 2: the per-tab session lock is gone entirely
Third commit extends the multi-tab model from planning to every AI interview surface (planning, subtask breakdown, mission interview, milestone/slice interview) and deletes the lock machinery root and branch.
Server
/ai-sessions/:id/lock,/lock/force, and/lock/beaconroutes.checkSessionLockfrom every planning/subtask/mission/milestone route (both copies —routes.tsandmission-routes.ts). AtabIdfrom an older client is ignored, never 409'd; alltabIdbody parsing is gone.acquireLock/releaseLock/forceAcquireLock/getLockHolder/releaseStaleLocksfromAiSessionStore, plus the@fusion/coreasync helpers (acquireAiSessionLocket al) and core's re-exports.lockedByTab/lockedAtfromAiSessionRow/AiSessionSummary, the upsert SQL, and all four session producers.Client
useSessionLockand the now-orphanedgetSessionTabIdutil.BackgroundTasksIndicator's active-elsewhere gate (the confirm prompt and lock badge — sessions now just open).useAiSessionSyncto what its own comments already called it — a low-latency status supplement to SSE: noactiveTabMap,broadcastLock/Unlock/Heartbeat,owningTabId,tab:*messages, or stale-heartbeat sweep.tabIdfrom every session API client function; removed the lock CSS.Deliberately kept: the two DB columns.
ai_sessions.locked_by_tab/locked_atremain as dead, always-NULL columns with a deprecation note. Dropping them is an irreversible migration, and released binaries still name those columns explicitly in their upsert — an older install pointed at the same database would fail every session write. They can be dropped once no such binary can reach it. No code reads or writes them.Verification: 397 client tests and 137 server planning tests pass (the same 3
routes-planning.test.tsfailures are pre-existing — verified identical on a clean stash);tsc --noEmitclean for@fusion/coreand@fusion/dashboard; eslint clean on all changed files; the 30 PGschema-appliertests pass (they exercise the retained columns);pnpm check:changesetspasses. The lock-conflict route tests and both modal lock tests were rewritten to assert the inverse: routes and modals stay fully interactive while another tab "holds" a lock, and the lock API is never called.