fix(connection): serialize + validate the switch session mutation (idea-7)#1958
Open
barakb wants to merge 9 commits into
Open
fix(connection): serialize + validate the switch session mutation (idea-7)#1958barakb wants to merge 9 commits into
barakb wants to merge 9 commits into
Conversation
…ngle-tab) Follow-up to PR #1951: a detailed implementation plan closing the 🔴 Critical CodeRabbit thread (serialize the connection-switch session mutation). Scoped to the minimal single-tab fix agreed with @barakb; passed 5 rounds of rubber-duck review (severity corrected, next-auth update() no-op/null semantics handled, ticket-owned gate with target-keyed handoff, failure-rollback convergence, commit-before-delete removal). Cross-tab / server-authoritative options documented as out of scope. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Completed Working on "Code Review"✅ Review submitted: COMMENT. Total comments: 5 across 1 files. ✅ Workflow completed successfully. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds serialized and validated connection-session commits, ticket-based switch gating, rollback convergence across connection state layers, commit-before-delete removal handling, and unit/E2E coverage for overlapping switches. ChangesConnection switch convergence
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ConnectionManager
participant Provider
participant Session
participant GraphReset
User->>ConnectionManager: select connection B
ConnectionManager->>Provider: begin switch ticket
Provider->>Session: serialized validated commit B
User->>ConnectionManager: select connection C
ConnectionManager->>Provider: begin newer switch ticket
Provider->>Session: serialized validated commit C
Session-->>Provider: validate committed C
Provider->>GraphReset: reset state for C
GraphReset-->>Provider: release matching handoff
Possibly related PRs
Suggested reviewers: Poem
🚥 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 |
![overcut-ai[bot]](https://avatars.githubusercontent.com/in/1367736?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
Findings posted in this review:
- 0 BLOCKER
- 0 CRITICAL
- 5 MAJOR
- 0 MINOR
- 0 SUGGESTION
- 0 PRAISE
Affected files: 1 (idea-7-serialize-connection-switch-plan.md)
Key themes
- Gate lifecycle resilience: multiple findings point to potential stuck/deadlock states if ticket release or recovery paths are bypassed.
- Failure-path convergence: fallback behavior is under-specified for delete/commit races and stale-session outcomes.
- Validation of plan claims/tests: scope/closure wording and e2e overlap criteria need tightening to match actual guarantees.
Actionable next steps
- Add explicit bounded recovery behavior for stalled switch/commit flows, with unit/e2e coverage.
- Ensure ticket handoff release cannot be skipped by any reset-effect early return path.
- Strengthen
confirmRemovefallback when replacement commit retries fail, and tighten top-level scope wording plus overlap precondition assertions in e2e criteria.
Adds §1a with step-by-step timelines: the core A->B->C cookie/UI divergence under network reordering, the update() no-op-while-loading trap, failure-rollback divergence, gate-zeroing clobber, and confirmRemove deleted-connection. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
![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: 3
🤖 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 `@idea-7-serialize-connection-switch-plan.md`:
- Around line 198-199: Update the sign-out/auth-reset cleanup around
pendingSwitchTicketsRef and committedSwitchRef to invalidate queued and
in-flight sessionChainRef work with an authentication epoch. Tag each queued
commit with the epoch it was created under, advance the epoch during reset, and
ensure stale queued or late-settling commits cannot call the current
updateSessionRef or overwrite the newly authenticated session; drain or abort
in-flight commits before accepting the new session.
- Around line 3-6: Clarify the goal and acceptance criteria to limit the
convergence guarantee to serialized connection-switch mutations, explicitly
excluding same-tab focus-refetch GETs; alternatively, add reconciliation for
session reads so those refetches cannot overwrite the last successfully
committed connection. Update the out-of-scope wording consistently and preserve
the guarantee for overlapping A→B→C switch mutations.
- Around line 202-215: Define rollback behavior for a null last-good connection
in the connection-switch flow: when rollback resolves to null, remove
lastActiveConnectionId from localStorage instead of persisting a "null" string.
Update the rollback helper or switch handlers around handleSelect and
handleAddConnection to preserve this representation, and add coverage for reload
with no active connection so auto-select does not interpret a sentinel as a
connection ID.
🪄 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: 9408d1bb-e617-4001-9358-d04cd028d743
📒 Files selected for processing (1)
idea-7-serialize-connection-switch-plan.md
…ea-7) Implements the minimal single-tab fix from idea-7: - serializedRunner.ts: createSerializedRunner (FIFO) + commitWithValidation (waits for authenticated, validates result.activeConnectionId, bounded retries, throws on failure) + unit tests. - providers.tsx: commitActiveConnection wraps the raw next-auth update as the context updateSession; lastCommittedConnIdRef seeded from the session. Ticket-owned gate (Set<number>) + target-keyed committedSwitchRef + handoffConnectionSwitch + isConnectionSwitchInProgress; reset effect releases only the matching handed-off ticket (no global zero). Internal establishment/ migration writes routed through the serialized commit. - ConnectionManager.tsx: try/finally ticket lifecycle with handoff on win/ rollback; converge React+global+localStorage to the last committed connection on failure; commit-before-delete in confirmRemove; block removals while a switch is in progress; refetch the list on an ambiguous DELETE failure. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Adds an @admin test that holds the first switch's /api/auth/session update in-flight, triggers a second switch, releases the first, and asserts the JWT converges on the last-selected connection (via /api/auth/session directly, not a graph op which routes by X-Connection-Id). Requires ≥3 connections (adds two via the API). POM: getConnectionIds + selectConnectionById. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
🔒 Trivy Security Scan Results |
Contributor
Author
|
@coderabbitai full review — the implementation has landed since your last review (which was on the plan only). Please review the actual code changes in |
Contributor
|
Tip For best results, initiate chat on the files or code changes. #!/bin/bash
set -euo pipefail
fd -a 'serializedRunner.ts'
fd -a 'providers.tsx'
fd -a 'ConnectionManager.tsx'
fd -a 'provider.ts'
echo "---e2e---"
fd -a . -e spec.ts -p e2e | rg -i 'connection|admin' || true
fd -a . | rg -i 'e2e' | rg -i 'connection' || true
</shell>
Wait, I need proper shell block.
```shell
#!/bin/bash
set -euo pipefail
fd -a 'serializedRunner.ts'
fd -a 'providers.tsx'
fd -a 'ConnectionManager.tsx'
fd -a 'provider.ts'
echo "---e2e search---"
fd . -e spec.ts | rg -i 'connection|admin' || true |
- serializedRunner: remove the redundant tail.catch (tail is provably never rejecting), and add a null-target retry-exhaustion test. - package.json: add app/components/serializedRunner.ts to the test:coverage allowlist (now 100% line/branch/function), per idea-7 plan §5c/§9.2. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@app/components/ConnectionManager.tsx`:
- Around line 69-71: Update each rollback branch in ConnectionManager around
rollbackId to keep localStorage synchronized when the resolved ID is null:
retain setItem for non-null IDs, and remove the "lastActiveConnectionId" key
when rollbackId is null. Apply this consistently to all three indicated rollback
paths.
- Around line 115-120: Make connection removal and selection mutually exclusive
across the entire DELETE operation. In the removal handler containing the
isConnectionSwitchInProgress() preflight, acquire and retain a shared mutation
lock until deletion completes, and update handleSelect to reject or wait while
removal is active; ensure the lock is released on both success and failure so
the JWT cannot target a deleted connection.
- Around line 47-63: Update the switch initiation logic around
activeConnectionId and the pending-switch ticket so a displayed target is
skipped only when no switch is currently pending, allowing A→B→A to supersede B.
In the post-updateSession path, when the committed connId already matches React
state, complete the ticket directly rather than calling handoffConnectionSwitch,
since the reset effect will not rerun. Preserve stale-ticket checks and normal
effect handoff for targets that differ from the displayed connection.
In `@app/components/serializedRunner.ts`:
- Around line 80-118: Bound the session update await in commitWithValidation so
a hung deps.update call cannot block the serialized runner indefinitely. Add an
abortable timeout or equivalent cancellation boundary around deps.update, while
preserving validation, retry, and rollback behavior when the update times out or
is cancelled.
In `@app/providers.tsx`:
- Around line 1268-1275: Update both establishment paths using
commitActiveConnection so React/global connection state is not published before
the commit succeeds. On commitActiveConnection failure, roll back every state
layer to the prior connection (including the JWT-resolved session connection),
or defer publication until success; do not merely log and continue with target.
Preserve successful establishment behavior.
- Around line 708-715: Reset or cancel commitRunnerRef during the auth cleanup
that clears the switch refs, ensuring any queued serialized commits are
discarded when authentication drops. Update the commitRunnerRef initialization
in the commit runner flow so a new runner is created for the next authenticated
session and pending updateSession operations cannot cross sessions.
🪄 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: ac6a5fa9-9c97-4c5a-bc9a-e2564d25181f
📒 Files selected for processing (8)
app/components/ConnectionManager.tsxapp/components/provider.tsapp/components/serializedRunner.test.tsapp/components/serializedRunner.tsapp/providers.tsxe2e/logic/POM/connectionManagerComponent.tse2e/tests/connectionManager.spec.tspackage.json
barakb
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
This PR implements the “idea-7” design to make overlapping single-tab connection switches converge deterministically on the last-selected connection by serializing and validating next-auth session updates, tightening the connection-switch gate, and adding regressions tests.
Changes:
- Added a serialized + validated session commit helper (
serializedRunner.ts) and wiredproviders.tsxto expose it via the existingupdateSession({activeConnectionId})context API. - Reworked the connection-switch gate from a counter to a ticket set with reset-effect handoff/release semantics; updated
ConnectionManagerto use try/finally ticket lifecycle and safer rollback/remove flows. - Added unit tests for the serializer/validator and a Playwright regression for A→B→C overlap convergence; updated coverage allowlist.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Adds app/components/serializedRunner.ts to the strict coverage allowlist. |
| idea-7-serialize-connection-switch-plan.md | Documents the problem, rationale, design, risks, and test plan for “idea-7”. |
| e2e/tests/connectionManager.spec.ts | Adds an overlapping-switch regression that asserts /api/auth/session converges on the last selection. |
| e2e/logic/POM/connectionManagerComponent.ts | Adds POM helpers to list connection IDs and select by ID for the new e2e test. |
| app/providers.tsx | Implements serialized+validated updateSession, ticket-set switch gate, and reset-effect ticket handoff/release. |
| app/components/serializedRunner.ts | New pure utilities: FIFO serialization + validated commit with bounded retries and authenticated readiness wait. |
| app/components/serializedRunner.test.ts | Unit tests for serialization, waitUntil, delay, and commit-with-validation behavior. |
| app/components/provider.ts | Extends ConnectionContext with new gate/rollback helpers and updates ticket API. |
| app/components/ConnectionManager.tsx | Updates selection/add/remove flows to use the serialized commit + ticket handoff, and safer rollback behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- handleSelect: allow re-selecting the displayed connection while a switch is pending so A→B→A ends on A (skip the no-op only when no switch is in progress); when the committed target already equals React state, release the ticket directly instead of handing it to a reset effect that won't rerun. - Mutual exclusion: a removingRef blocks handleSelect while a removal's commit+DELETE is in flight, so a select can't target a connection being deleted. - Rollback now clears lastActiveConnectionId when the last-good target is null (all three handlers) so localStorage converges with React/global. - commitWithValidation: add an isCancelled hook; providers bumps an auth generation on sign-out so a queued commit can't fire into a later session. - Establishment/migration: publish React state only after the commit validates (roll back the global on failure) so a failed commit can't leave the UI on a connection the session doesn't reflect. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/providers.tsx (1)
711-728: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
useStatelazy initialization for pure renders.Mutating a ref directly during render violates React's pure render constraints and triggers lint warnings. As per path instructions regarding proper hooks usage, use
useStatewith an initializer function to safely create the serialized runner exactly once without relying on mutable refs.♻️ Proposed refactor
- const commitRunnerRef = useRef<((data: { activeConnectionId?: string | null }, isCancelled: () => boolean) => Promise<unknown>) | undefined>(undefined); - if (!commitRunnerRef.current) { - commitRunnerRef.current = createSerializedRunner((data: { activeConnectionId?: string | null }, isCancelled: () => boolean) => - commitWithValidation(data.activeConnectionId ?? null, { - update: (d) => updateSessionRef.current(d), - getStatus: () => statusRef.current, - onCommitted: (id) => { lastCommittedConnIdRef.current = id; }, - isCancelled, - })); - } - const commitActiveConnection = useCallback((data: { activeConnectionId?: string | null }) => { - const runner = commitRunnerRef.current; - if (!runner) return Promise.resolve(undefined); + const [commitRunner] = useState(() => + createSerializedRunner((data: { activeConnectionId?: string | null }, isCancelled: () => boolean) => + commitWithValidation(data.activeConnectionId ?? null, { + update: (d) => updateSessionRef.current(d), + getStatus: () => statusRef.current, + onCommitted: (id) => { lastCommittedConnIdRef.current = id; }, + isCancelled, + }) + ) + ); + + const commitActiveConnection = useCallback((data: { activeConnectionId?: string | null }) => { // Pin this commit to the current auth session; if a sign-out bumps the auth // generation while it is queued, it aborts instead of committing cross-session. const gen = authGenRef.current; - return runner(data, () => authGenRef.current !== gen); - }, []); + return commitRunner(data, () => authGenRef.current !== gen); + }, [commitRunner]);🤖 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 `@app/providers.tsx` around lines 711 - 728, Replace the render-time mutation of commitRunnerRef in the commit runner setup with lazy useState initialization, creating the serialized runner exactly once through the initializer function. Update commitActiveConnection to read the initialized runner from state while preserving its current auth-generation cancellation behavior and callback dependencies.Sources: Path instructions, Linters/SAST tools
🤖 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 `@app/providers.tsx`:
- Around line 1283-1285: Only roll back the global connection ID after initial
or migrated session commit failures when it still matches the connection being
committed, preventing newer user-initiated switches from being overwritten. In
app/providers.tsx lines 1283-1285, update the rollback to require
getActiveConnectionIdGlobal() === target alongside !cancelled; apply the
equivalent check at lines 1324-1326 using migratedConn.id.
---
Nitpick comments:
In `@app/providers.tsx`:
- Around line 711-728: Replace the render-time mutation of commitRunnerRef in
the commit runner setup with lazy useState initialization, creating the
serialized runner exactly once through the initializer function. Update
commitActiveConnection to read the initialized runner from state while
preserving its current auth-generation cancellation behavior and callback
dependencies.
🪄 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: 5b18deea-30db-45ec-9a83-3ac5de285841
📒 Files selected for processing (4)
app/components/ConnectionManager.tsxapp/components/serializedRunner.test.tsapp/components/serializedRunner.tsapp/providers.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- app/components/serializedRunner.test.ts
- app/components/ConnectionManager.tsx
- confirmRemove: surface a failure toast when the DELETE request throws (network error) — securedFetch only toasts for HTTP responses, so it was silent before. - e2e reorder test: capture the two temporary connections from the POST responses and delete them in a finally (switching the session back to the original first), so they don't accumulate in the shared admin storageState. Removed the now-unused getConnectionIds POM helper. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Only revert the global connection id to null on a failed establishment/migration commit if it still equals the target — otherwise a concurrent user switch that already moved the global would be clobbered. (CodeRabbit) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Comment on lines
+63
to
+73
| export interface CommitDeps { | ||
| /** Performs the session mutation (next-auth `useSession().update`). */ | ||
| update: (data: { activeConnectionId?: string | null }) => Promise<unknown>; | ||
| /** Current next-auth session status (`"loading" | "authenticated" | ...`). */ | ||
| getStatus: () => string; | ||
| /** Called with the target once a commit is validated as successful. */ | ||
| onCommitted?: (targetId: string | null) => void; | ||
| /** Abort the commit (without a write) when it becomes stale — e.g. the auth | ||
| * session changed (sign-out / re-login) while this commit was queued. */ | ||
| isCancelled?: () => boolean; | ||
| } |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@app/components/serializedRunner.ts`:
- Around line 109-114: Recheck the cancellation state immediately after the
`waitUntil` call and before `deps.update` in the serialized runner flow,
returning the existing cancellation result when the generation is stale. Add a
regression test covering authentication changing while readiness polling is
suspended, ensuring no session write occurs afterward.
In `@app/providers.tsx`:
- Around line 1272-1281: Update both provider-driven commit paths around
commitActiveConnection to register with the switch gate using the same
begin/latest/handoff/end lifecycle as ConnectionManager. Allocate a ticket
before changing global routing, verify it remains latest before publishing React
state, and always complete the ticket lifecycle on success, cancellation, or
failure. Ensure stale commits cannot expose or retain a removed pending target.
In `@e2e/tests/connectionManager.spec.ts`:
- Around line 68-90: Ensure the connection overlap test waits until B’s
session-update request is intercepted before selecting C: add a route-handler
signal and await it after selecting B, then select C and release the held
request. Also call releaseFirst() at the start of the existing finally block so
cleanup cannot remain blocked.
In `@idea-7-serialize-connection-switch-plan.md`:
- Around line 93-100: Add the text language identifier to every unlabeled fenced
code block in idea-7-serialize-connection-switch.md, including the timeline and
ASCII diagram examples near the referenced sections. Change each opening fence
to use ```text while preserving all diagram content unchanged.
- Around line 304-309: Update the removal sequence’s handoff call to pass the
replacement connection ID, using handoffConnectionSwitch(ticket, newActive)
after commitActiveConnection(newActive). Keep the ordering and surrounding
DELETE behavior unchanged, and align the documentation with the contract in §3b
and ConnectionManager.
🪄 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: d8d9e5a6-ef4b-463a-ad54-44d0dc82238d
📒 Files selected for processing (9)
app/components/ConnectionManager.tsxapp/components/provider.tsapp/components/serializedRunner.test.tsapp/components/serializedRunner.tsapp/providers.tsxe2e/logic/POM/connectionManagerComponent.tse2e/tests/connectionManager.spec.tsidea-7-serialize-connection-switch-plan.mdpackage.json
Comment on lines
+109
to
+114
| const ready = await waitUntil(() => deps.getStatus() === "authenticated", readyTimeoutMs, { | ||
| now: opts.now, | ||
| sleep, | ||
| }); | ||
| if (ready) { | ||
| const result = (await deps.update({ activeConnectionId: targetId })) as |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Recheck auth cancellation immediately before the session write.
If authentication changes during waitUntil, a subsequent login can make ready true and allow this stale generation to call update. Check isCancelled again after polling and before the write.
Proposed fix
if (ready) {
+ if (deps.isCancelled?.()) {
+ throw new Error(`Commit for ${targetId} cancelled (auth changed)`);
+ }
const result = (await deps.update({ activeConnectionId: targetId })) asAdd a regression test where authentication changes while readiness polling is suspended.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const ready = await waitUntil(() => deps.getStatus() === "authenticated", readyTimeoutMs, { | |
| now: opts.now, | |
| sleep, | |
| }); | |
| if (ready) { | |
| const result = (await deps.update({ activeConnectionId: targetId })) as | |
| const ready = await waitUntil(() => deps.getStatus() === "authenticated", readyTimeoutMs, { | |
| now: opts.now, | |
| sleep, | |
| }); | |
| if (ready) { | |
| if (deps.isCancelled?.()) { | |
| throw new Error(`Commit for ${targetId} cancelled (auth changed)`); | |
| } | |
| const result = (await deps.update({ activeConnectionId: targetId })) as |
🤖 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 `@app/components/serializedRunner.ts` around lines 109 - 114, Recheck the
cancellation state immediately after the `waitUntil` call and before
`deps.update` in the serialized runner flow, returning the existing cancellation
result when the generation is stale. Add a regression test covering
authentication changing while readiness polling is suspended, ensuring no
session write occurs afterward.
Comment on lines
+1272
to
+1281
| // Pin the global so the commit's own request routes to `target`, but | ||
| // publish React state only AFTER the JWT commit validates — a failed | ||
| // commit must not leave the UI on a connection the session doesn't | ||
| // reflect. Route through the serialized commit so a concurrent user | ||
| // switch can't be reordered against this establishment write. | ||
| setActiveConnectionIdGlobal(target); | ||
| // Sync activeConnectionId into the JWT so session.user reflects | ||
| // the correct connection's role/host/port. The JWT callback looks | ||
| // up the full connection details from Token DB. | ||
| if (!cancelled) { | ||
| await updateSessionRef.current({ | ||
| activeConnectionId: target, | ||
| }); | ||
| try { | ||
| await commitActiveConnection({ activeConnectionId: target }); | ||
| if (!cancelled) setActiveConnectionId(target); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Register provider-driven commits with the switch gate.
These paths change global routing while React/JWT state still disagrees, but never allocate a ticket. The gate therefore remains open; notably, the initial path has already exposed the connection list, allowing removal of the pending target.
Wrap both commits in the same begin/latest/handoff/end lifecycle used by ConnectionManager, and only publish React state when their ticket remains latest.
Also applies to: 1318-1323
🤖 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 `@app/providers.tsx` around lines 1272 - 1281, Update both provider-driven
commit paths around commitActiveConnection to register with the switch gate
using the same begin/latest/handoff/end lifecycle as ConnectionManager. Allocate
a ticket before changing global routing, verify it remains latest before
publishing React state, and always complete the ticket lifecycle on success,
cancellation, or failure. Ensure stale commits cannot expose or retain a removed
pending target.
Comment on lines
+93
to
+100
| ``` | ||
| t0 click B: beginConnectionSwitch()→ticket#1 global=B POST update(B) ──┐ (in flight) | ||
| t1 click C: beginConnectionSwitch()→ticket#2 global=C POST update(C) ─┐│ (in flight) | ||
| t2 server handles update(C) → Set-Cookie jwt(active=conn-C) ││ | ||
| t3 server handles update(B) → Set-Cookie jwt(active=conn-B) ││ | ||
| t4 update(C) response arrives → browser cookie=C handleSelect(C): isLatestSwitch(#2)=true → UI=C | ||
| t5 update(B) response arrives → browser cookie=B handleSelect(B): isLatestSwitch(#1)=false → no publish | ||
| ``` |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add language identifiers to the fenced examples.
Markdownlint reports MD040 for these six unlabeled fences. Use ```text for the timeline and ASCII diagrams so the documentation passes the configured Markdown checks.
Also applies to: 110-117, 123-129, 137-144, 148-153, 160-164
🧰 Tools
🪛 markdownlint-cli2 (0.23.0)
[warning] 93-93: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@idea-7-serialize-connection-switch-plan.md` around lines 93 - 100, Add the
text language identifier to every unlabeled fenced code block in
idea-7-serialize-connection-switch.md, including the timeline and ASCII diagram
examples near the referenced sections. Change each opening fence to use ```text
while preserving all diagram content unchanged.
Source: Linters/SAST tools
Comment on lines
+304
to
+309
| - `confirmRemove` — **commit the replacement before deleting** the old connection, and hand the ticket | ||
| to the reset effect (do **not** hold it across the DELETE — a hung DELETE would otherwise stick the | ||
| gate). Order: `beginConnectionSwitch` → `commitActiveConnection(newActive)` (validated) → | ||
| `handoffConnectionSwitch(ticket)` + publish React/global/localStorage → **then** DELETE the old row → | ||
| update `additionalConnections` **only after DELETE succeeds**. If the commit fails first, nothing was | ||
| deleted and we roll back to the last-good id (`finally` releases the ticket). If the DELETE fails |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Pass the target ID to the removal handoff.
The plan defines handoffConnectionSwitch(ticket, targetId) in §3b, but the removal sequence shortens this to handoffConnectionSwitch(ticket). Document the concrete call as handoffConnectionSwitch(ticket, newActive) so it matches the context contract and app/components/ConnectionManager.tsx.
🤖 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 `@idea-7-serialize-connection-switch-plan.md` around lines 304 - 309, Update
the removal sequence’s handoff call to pass the replacement connection ID, using
handoffConnectionSwitch(ticket, newActive) after
commitActiveConnection(newActive). Keep the ordering and surrounding DELETE
behavior unchanged, and align the documentation with the contract in §3b and
ConnectionManager.
Comment on lines
+56
to
+70
| // Need ≥3 connections to do A→B→C (selecting the active one is a no-op). Add two | ||
| // extra connections to the same local FalkorDB via the API, capturing their ids. | ||
| const added: string[] = []; | ||
| for (let i = 0; i < 2; i += 1) { | ||
| const res = await page.request.post(`${origin}/api/connections`, { | ||
| data: { host: "localhost", port: "6379" }, | ||
| }); | ||
| expect(res.ok()).toBe(true); | ||
| added.push((await res.json()).connection.id as string); | ||
| } | ||
| const [connB, connC] = added; | ||
|
|
||
| try { | ||
| await page.reload(); | ||
| await connManager.waitForPageIdle(); |
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
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.
What & why
Closes the 🔴 Critical CodeRabbit thread on #1951 — "the ticket still does not serialize the session mutation" (
app/components/ConnectionManager.tsx#L51-69+app/providers.tsx#L1615-1620). Overlapping connection switches (A→B→C in one tab) must converge the persisted JWT, the React UI, the module-global id, and the switch gate on the last-selected connection — never a superseded one.Design + rationale:
idea-7-serialize-connection-switch-plan.md(passed 5 rounds of rubber-duck review; §1a has worked examples).Scope (agreed with @barakb): minimal, single-tab
Code verification narrowed the real severity: header-routed graph ops already hit the correct connection (
X-Connection-Id+ Token DB role) and reload auto-select uses localStorage — so the residual bug is session/UI metadata divergence + header-less/fallback routing, not live query misrouting. Cross-tab / focus-refetch reordering and server-authoritative last-wins are out of scope (documented in §4).Implementation (landed)
app/components/serializedRunner.ts(new) —createSerializedRunner(FIFO) +commitWithValidation(waits forstatus === "authenticated", validatesresult.activeConnectionId === target, bounded retries, throws on failure; no non-aborting timeout). 100% unit-tested and added to thetest:coverageallowlist.app/providers.tsx—commitActiveConnectionwraps the raw next-authupdateas the contextupdateSession;lastCommittedConnIdRefseeded from the session. Ticket-owned gate:Set<number>of live tickets + target-keyedcommittedSwitchRef{ticket,targetId}+handoffConnectionSwitch(retires the prior handed-off ticket) +isConnectionSwitchInProgress. The reset effect releases only the matching handed-off ticket after graph state is re-pinned (incl. the early-return / rollback-to-same-id / first-establishment branches) — never a global zero. Internal establishment/migration writes routed through the serialized commit; auth-reset clears the gate.app/components/ConnectionManager.tsx—try/finallyticket lifecycle with ahandedOffflag across all three handlers; on failure, converge React + global + localStorage to the last-committed connection (via handoff when it changes React).confirmRemovedoes commit-before-delete (never points the session at a deleted row), blocks removals while a switch is in progress, updates the list only after DELETE, and refetches on an ambiguous DELETE failure.app/components/provider.ts— context type + defaults for the new API.Validation — all green
npm test(358) ·npm run test:coverage(100% incl.serializedRunner.ts) ·npm run lint(0 errors) ·npx tsc --noEmitconnectionManager.spec.ts,@admin, ≥3 connections): holds B's/api/auth/sessionupdate in-flight, triggers C, releases B, and asserts/api/auth/sessionconverges on C (asserted directly, not via a graph op which routes by header). Verified locally against FalkorDB v4.18.10; the pre-fix code leaves the cookie on B.Review
Monitoring + resolving Copilot & CodeRabbit threads. Not merging without @barakb's approval.
Summary by CodeRabbit