feat(session): host leave promote and endSession callables#161
Conversation
WalkthroughAdds host-leave transfer or session termination, canonical code cleanup, callable APIs, typed client services, and map-session UI handling with tests and release metadata. ChangesHost leave lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Player
participant useMapSessionChrome
participant FirebaseCallable
participant Firestore
Player->>useMapSessionChrome: Confirm leave or end
useMapSessionChrome->>FirebaseCallable: Call leaveHostSession or endSession
FirebaseCallable->>Firestore: Validate host and update session
Firestore-->>FirebaseCallable: Promotion or ended result
FirebaseCallable-->>useMapSessionChrome: Return lifecycle result
useMapSessionChrome-->>Player: Exit session
Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@functions/session/hostLeave.mjs`:
- Around line 53-59: Update endSessionHandler and endSessionCanonical so host
authorization is validated inside the same transaction that ends the session.
Pass uid into endSessionCanonical, re-check the transaction-read session’s
hostUid against uid before applying the update, and retain the existing pre-read
only if needed for non-authorization data; ensure a concurrent host transfer
cannot allow a former host to end the session.
In `@src/hooks/map-screen/useMapSessionChrome.test.ts`:
- Around line 350-413: Add edge-case tests around handleEndSession and
handleLeaveSession for rejected callable operations: mock mockEndSession and
mockLeaveHostSession to reject, assert mockEndRemoteSession receives the session
ID and captureException receives the original error, and cover both-fail
scenarios asserting the alert behavior. Preserve the existing happy-path
assertions and include both alone and not-alone leave branches.
- Around line 377-413: Extend the “confirms promote copy when host leave has
another player” test to verify the promotion leave flow completes, not just the
confirmation text. After invoking handleLeaveSession, assert that the
appropriate exitSession behavior is called, matching the sibling leave-flow test
and the resolved promoted result from mockLeaveHostSession.
In `@src/services/session/sessionLifecycle.test.ts`:
- Around line 21-26: Update the test case for leaveHostSession to capture its
resolved return value and assert that it equals the expected result.data shape,
including the contract fields action and newHostUid. Keep the existing
httpsCallable routing and sessionId payload assertions, and make the assertion
behavior-focused on the value consumed by useMapSessionChrome.
- Around line 36-42: Add a test alongside the existing leaveHostSession
not-configured case that mocks isFirebaseConfigured to return false, calls
endSession with a representative session ID, and asserts it rejects with
"Firebase is not configured.".
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 62e25c0a-f101-48e0-92c3-cabcad7e0ac1
📒 Files selected for processing (11)
.changeset/host-leave-transfer.mdfunctions/handlers/session.mjsfunctions/index.mjsfunctions/session/endSessionCanonical.mjsfunctions/session/hostLeave.mjsfunctions/session/pickHostPromotee.mjsfunctions/test/hostLeave.test.mjssrc/hooks/map-screen/useMapSessionChrome.test.tssrc/hooks/map-screen/useMapSessionChrome.tssrc/services/session/sessionLifecycle.test.tssrc/services/session/sessionLifecycle.ts
| export async function endSessionHandler(db, uid, sessionId) { | ||
| const sessionRef = db.collection("sessions").doc(sessionId); | ||
| const sessionDoc = await sessionRef.get(); | ||
| assertHostSession(sessionDoc, uid); | ||
| await endSessionCanonical(db, sessionDoc, { gameOutcome: "ended_early" }); | ||
| return { ok: true }; | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Host authorization check is not transaction-consistent in endSessionHandler.
assertHostSession here runs against a plain, non-transactional sessionRef.get() (Line 55), then endSessionCanonical opens its own transaction and re-reads fresh data (endSessionCanonical.mjs Lines 45-51) but never re-validates hostUid against uid. If hostUid changes between the initial read and the transaction commit (e.g. a concurrent host-transfer via leaveHostSessionHandler), the session can still be ended by a caller who is no longer the host at commit time — unlike leaveHostSessionHandler, which correctly validates inside the transaction (Lines 31-38).
🔒 Suggested fix: validate host status inside the transaction
export async function endSessionHandler(db, uid, sessionId) {
const sessionRef = db.collection("sessions").doc(sessionId);
- const sessionDoc = await sessionRef.get();
- assertHostSession(sessionDoc, uid);
- await endSessionCanonical(db, sessionDoc, { gameOutcome: "ended_early" });
+ await db.runTransaction(async (tx) => {
+ const sessionSnap = await tx.get(sessionRef);
+ const data = assertHostSession(sessionSnap, uid);
+ applyEndSessionInTx(tx, db, sessionRef, data, "ended_early");
+ });
return { ok: true };
}As per path instructions, functions/** should be reviewed for "Authentication and authorization on callable/HTTP endpoints."
🤖 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 `@functions/session/hostLeave.mjs` around lines 53 - 59, Update
endSessionHandler and endSessionCanonical so host authorization is validated
inside the same transaction that ends the session. Pass uid into
endSessionCanonical, re-check the transaction-read session’s hostUid against uid
before applying the update, and retain the existing pre-read only if needed for
non-authorization data; ensure a concurrent host transfer cannot allow a former
host to end the session.
Source: Path instructions
| it("calls endSession callable when host ends the session", async () => { | ||
| vi.spyOn(window, "confirm").mockReturnValue(true); | ||
|
|
||
| const { result } = renderHook(() => | ||
| useMapSessionChrome({ | ||
| session: remoteSession, | ||
| isHost: true, | ||
| annotations: [], | ||
| mapShellRef: { current: null }, | ||
| exportLegendRef: { current: null }, | ||
| clearAllAnnotations: vi.fn(async () => undefined), | ||
| setSelectedAnnotationId: vi.fn(), | ||
| closeSettingsPanel: vi.fn(), | ||
| resetTimer: vi.fn(), | ||
| }), | ||
| ); | ||
|
|
||
| await act(async () => { | ||
| await result.current.handleEndSession(); | ||
| }); | ||
|
|
||
| expect(mockEndSession).toHaveBeenCalledWith("session-remote"); | ||
| expect(exitSession).toHaveBeenCalledWith( | ||
| expect.objectContaining({ reason: "end", sessionId: "session-remote" }), | ||
| ); | ||
| }); | ||
|
|
||
| it("confirms promote copy when host leave has another player", async () => { | ||
| vi.spyOn(window, "confirm").mockReturnValue(true); | ||
| mockLeaveHostSession.mockResolvedValueOnce({ | ||
| action: "promoted", | ||
| newHostUid: "seeker-2", | ||
| }); | ||
|
|
||
| const { result } = renderHook(() => | ||
| useMapSessionChrome({ | ||
| session: { | ||
| ...remoteSession, | ||
| memberUids: ["host-1", "seeker-2"], | ||
| memberRoles: { | ||
| "host-1": "seeker", | ||
| "seeker-2": "seeker", | ||
| }, | ||
| }, | ||
| isHost: true, | ||
| annotations: [], | ||
| mapShellRef: { current: null }, | ||
| exportLegendRef: { current: null }, | ||
| clearAllAnnotations: vi.fn(async () => undefined), | ||
| setSelectedAnnotationId: vi.fn(), | ||
| closeSettingsPanel: vi.fn(), | ||
| resetTimer: vi.fn(), | ||
| }), | ||
| ); | ||
|
|
||
| await act(async () => { | ||
| await result.current.handleLeaveSession(); | ||
| }); | ||
|
|
||
| expect(window.confirm).toHaveBeenCalledWith( | ||
| "Another player will become host so others can keep playing. Leave anyway?", | ||
| ); | ||
| expect(mockLeaveHostSession).toHaveBeenCalledWith("session-remote"); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
No coverage for the callable-failure/fallback branches.
All new tests exercise only the happy path of mockLeaveHostSession/mockEndSession. The PR's client-side fallback logic — handleEndSession falling back to endRemoteSession when endSession rejects (useMapSessionChrome.ts:191-220), and handleLeaveSession's alone-and-failed / not-alone-and-failed branches (useMapSessionChrome.ts:222-290) — is untested here. These are exactly the "client-side fallback when callables are unavailable" paths called out in the PR objectives, and are arguably the highest-risk new logic in this layer (silent data/session-state divergence if the fallback itself misbehaves).
Consider adding cases where mockEndSession/mockLeaveHostSession reject once, asserting mockEndRemoteSession is invoked and captureException is called with the original error, plus the "both fail → alert" case.
As per path instructions for **/*.test.{ts,tsx}, tests should cover "Edge cases for new logic".
🤖 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 `@src/hooks/map-screen/useMapSessionChrome.test.ts` around lines 350 - 413, Add
edge-case tests around handleEndSession and handleLeaveSession for rejected
callable operations: mock mockEndSession and mockLeaveHostSession to reject,
assert mockEndRemoteSession receives the session ID and captureException
receives the original error, and cover both-fail scenarios asserting the alert
behavior. Preserve the existing happy-path assertions and include both alone and
not-alone leave branches.
Source: Path instructions
| it("confirms promote copy when host leave has another player", async () => { | ||
| vi.spyOn(window, "confirm").mockReturnValue(true); | ||
| mockLeaveHostSession.mockResolvedValueOnce({ | ||
| action: "promoted", | ||
| newHostUid: "seeker-2", | ||
| }); | ||
|
|
||
| const { result } = renderHook(() => | ||
| useMapSessionChrome({ | ||
| session: { | ||
| ...remoteSession, | ||
| memberUids: ["host-1", "seeker-2"], | ||
| memberRoles: { | ||
| "host-1": "seeker", | ||
| "seeker-2": "seeker", | ||
| }, | ||
| }, | ||
| isHost: true, | ||
| annotations: [], | ||
| mapShellRef: { current: null }, | ||
| exportLegendRef: { current: null }, | ||
| clearAllAnnotations: vi.fn(async () => undefined), | ||
| setSelectedAnnotationId: vi.fn(), | ||
| closeSettingsPanel: vi.fn(), | ||
| resetTimer: vi.fn(), | ||
| }), | ||
| ); | ||
|
|
||
| await act(async () => { | ||
| await result.current.handleLeaveSession(); | ||
| }); | ||
|
|
||
| expect(window.confirm).toHaveBeenCalledWith( | ||
| "Another player will become host so others can keep playing. Leave anyway?", | ||
| ); | ||
| expect(mockLeaveHostSession).toHaveBeenCalledWith("session-remote"); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
"Promote copy" test doesn't verify the leave flow completes.
Per useMapSessionChrome.ts:222-290, handleLeaveSession's confirm message is computed from session.memberUids/hostUid before leaveHostSession is even called, so the resolved {action: "promoted", newHostUid: "seeker-2"} set on mockLeaveHostSession (lines 379-382) has no bearing on the assertions made — it's only proven that the confirm copy matches when another player exists. Unlike the sibling test at lines 343-348, this test never asserts exitSession was called, so it doesn't confirm the promotion path actually completes the leave flow.
Proposed addition
expect(window.confirm).toHaveBeenCalledWith(
"Another player will become host so others can keep playing. Leave anyway?",
);
expect(mockLeaveHostSession).toHaveBeenCalledWith("session-remote");
+ expect(exitSession).toHaveBeenCalledWith(
+ expect.objectContaining({ reason: "leave", sessionId: "session-remote" }),
+ );
});As per path instructions for **/*.test.{ts,tsx}, tests should have "Behavior-focused assertions on changed code paths".
📝 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.
| it("confirms promote copy when host leave has another player", async () => { | |
| vi.spyOn(window, "confirm").mockReturnValue(true); | |
| mockLeaveHostSession.mockResolvedValueOnce({ | |
| action: "promoted", | |
| newHostUid: "seeker-2", | |
| }); | |
| const { result } = renderHook(() => | |
| useMapSessionChrome({ | |
| session: { | |
| ...remoteSession, | |
| memberUids: ["host-1", "seeker-2"], | |
| memberRoles: { | |
| "host-1": "seeker", | |
| "seeker-2": "seeker", | |
| }, | |
| }, | |
| isHost: true, | |
| annotations: [], | |
| mapShellRef: { current: null }, | |
| exportLegendRef: { current: null }, | |
| clearAllAnnotations: vi.fn(async () => undefined), | |
| setSelectedAnnotationId: vi.fn(), | |
| closeSettingsPanel: vi.fn(), | |
| resetTimer: vi.fn(), | |
| }), | |
| ); | |
| await act(async () => { | |
| await result.current.handleLeaveSession(); | |
| }); | |
| expect(window.confirm).toHaveBeenCalledWith( | |
| "Another player will become host so others can keep playing. Leave anyway?", | |
| ); | |
| expect(mockLeaveHostSession).toHaveBeenCalledWith("session-remote"); | |
| }); | |
| expect(window.confirm).toHaveBeenCalledWith( | |
| "Another player will become host so others can keep playing. Leave anyway?", | |
| ); | |
| expect(mockLeaveHostSession).toHaveBeenCalledWith("session-remote"); | |
| expect(exitSession).toHaveBeenCalledWith( | |
| expect.objectContaining({ reason: "leave", sessionId: "session-remote" }), | |
| ); | |
| }); |
🤖 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 `@src/hooks/map-screen/useMapSessionChrome.test.ts` around lines 377 - 413,
Extend the “confirms promote copy when host leave has another player” test to
verify the promotion leave flow completes, not just the confirmation text. After
invoking handleLeaveSession, assert that the appropriate exitSession behavior is
called, matching the sibling leave-flow test and the resolved promoted result
from mockLeaveHostSession.
Source: Path instructions
| it("calls leaveHostSession with the session id", async () => { | ||
| await leaveHostSession("session-42"); | ||
|
|
||
| expect(httpsCallable).toHaveBeenCalledWith({}, "leaveHostSession"); | ||
| expect(callable).toHaveBeenCalledWith({ sessionId: "session-42" }); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Assert the resolved return value of leaveHostSession.
The test verifies routing/payload but never checks that leaveHostSession actually returns result.data (per the upstream contract in sessionLifecycle.ts:8-22). Since the resolved action/newHostUid fields are the contract that useMapSessionChrome relies on, this is worth asserting directly here.
Proposed addition
it("calls leaveHostSession with the session id", async () => {
- await leaveHostSession("session-42");
+ const result = await leaveHostSession("session-42");
expect(httpsCallable).toHaveBeenCalledWith({}, "leaveHostSession");
expect(callable).toHaveBeenCalledWith({ sessionId: "session-42" });
+ expect(result).toEqual({ action: "ended" });
});As per path instructions for **/*.test.{ts,tsx}, tests should have "Behavior-focused assertions on changed code paths".
📝 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.
| it("calls leaveHostSession with the session id", async () => { | |
| await leaveHostSession("session-42"); | |
| expect(httpsCallable).toHaveBeenCalledWith({}, "leaveHostSession"); | |
| expect(callable).toHaveBeenCalledWith({ sessionId: "session-42" }); | |
| }); | |
| it("calls leaveHostSession with the session id", async () => { | |
| const result = await leaveHostSession("session-42"); | |
| expect(httpsCallable).toHaveBeenCalledWith({}, "leaveHostSession"); | |
| expect(callable).toHaveBeenCalledWith({ sessionId: "session-42" }); | |
| expect(result).toEqual({ action: "ended" }); | |
| }); |
🤖 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 `@src/services/session/sessionLifecycle.test.ts` around lines 21 - 26, Update
the test case for leaveHostSession to capture its resolved return value and
assert that it equals the expected result.data shape, including the contract
fields action and newHostUid. Keep the existing httpsCallable routing and
sessionId payload assertions, and make the assertion behavior-focused on the
value consumed by useMapSessionChrome.
Source: Path instructions
| it("throws when Firebase is not configured", async () => { | ||
| isFirebaseConfigured.mockReturnValueOnce(false); | ||
|
|
||
| await expect(leaveHostSession("session-42")).rejects.toThrow( | ||
| "Firebase is not configured.", | ||
| ); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Missing "not configured" test for endSession.
Only leaveHostSession is tested for the not-configured path, but endSession has the identical isFirebaseConfigured() guard (per sessionLifecycle.ts:24-35). This edge case for endSession is untested.
Proposed addition
await expect(leaveHostSession("session-42")).rejects.toThrow(
"Firebase is not configured.",
);
});
+
+ it("throws endSession when Firebase is not configured", async () => {
+ isFirebaseConfigured.mockReturnValueOnce(false);
+
+ await expect(endSession("session-42")).rejects.toThrow(
+ "Firebase is not configured.",
+ );
+ });
});As per path instructions for **/*.test.{ts,tsx}, tests should cover "Edge cases for new logic".
📝 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.
| it("throws when Firebase is not configured", async () => { | |
| isFirebaseConfigured.mockReturnValueOnce(false); | |
| await expect(leaveHostSession("session-42")).rejects.toThrow( | |
| "Firebase is not configured.", | |
| ); | |
| }); | |
| it("throws when Firebase is not configured", async () => { | |
| isFirebaseConfigured.mockReturnValueOnce(false); | |
| await expect(leaveHostSession("session-42")).rejects.toThrow( | |
| "Firebase is not configured.", | |
| ); | |
| }); | |
| it("throws endSession when Firebase is not configured", async () => { | |
| isFirebaseConfigured.mockReturnValueOnce(false); | |
| await expect(endSession("session-42")).rejects.toThrow( | |
| "Firebase is not configured.", | |
| ); | |
| }); | |
| }); |
🤖 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 `@src/services/session/sessionLifecycle.test.ts` around lines 36 - 42, Add a
test alongside the existing leaveHostSession not-configured case that mocks
isFirebaseConfigured to return false, calls endSession with a representative
session ID, and asserts it rejects with "Firebase is not configured.".
Source: Path instructions
* feat(session): add leaveHostSession and endSession callables * feat(session): wire host leave promote and endSession callable * fix(session): end alone host leave inside promote transaction * fix(session): fall back to client end when callables unavailable
1 participant
Summary
leaveHostSession/endSessionApp Check callables: promote another member (seeker-first) or end alone withended_earlyin one transaction; End frees join codes viaendSessionCanonical.Test plan
cd functions && node --test test/hostLeave.test.mjs test/endSessionCanonical.test.mjsnpm test -- src/services/session/sessionLifecycle.test.ts src/hooks/map-screen/useMapSessionChrome.test.tsSummary by CodeRabbit
New Features
Bug Fixes