diff --git a/.planning/ROADMAP.md b/.planning/ROADMAP.md index aa26fcf85..3967f0d60 100644 --- a/.planning/ROADMAP.md +++ b/.planning/ROADMAP.md @@ -438,7 +438,7 @@ Plans: **Depends on**: Phase 19 **Research**: Unlikely (fixing issues identified in Phase 19) **Requirements**: SESS-01, SESS-02, SESS-03 -**Plans**: TBD (1-2 plans expected) +**Plans**: 1 plan **Success Criteria:** 1. All useEffect hooks that set up subscriptions/intervals have cleanup return functions @@ -448,7 +448,7 @@ Plans: 5. No orphaned polling or listeners accumulate during normal session use Plans: -- [ ] 22-01: TBD +- [ ] 22-01-PLAN.md - Callback cleanup hardening with useRef pattern (SESS-01) #### Phase 23: Verification **Goal**: Validate session stability after all fixes are applied diff --git a/.planning/phases/22-session-screen-fixes/22-01-PLAN.md b/.planning/phases/22-session-screen-fixes/22-01-PLAN.md new file mode 100644 index 000000000..e66023f81 --- /dev/null +++ b/.planning/phases/22-session-screen-fixes/22-01-PLAN.md @@ -0,0 +1,164 @@ +--- +phase: 22-session-screen-fixes +plan: 01 +type: execute +wave: 1 +depends_on: [] +files_modified: + - jam-ui/src/components/client/JKSessionScreen.js +autonomous: true + +must_haves: + truths: + - "Session screen cleanup runs on component unmount regardless of leave handler execution" + - "Callback registration uses refs to ensure cleanup always has access to current callbacks" + - "Session can be exited via any path (leave button, navigation, browser close) without leaking callbacks" + artifacts: + - path: "jam-ui/src/components/client/JKSessionScreen.js" + provides: "Improved callback cleanup pattern with useRef" + contains: "registeredCallbacksRef" + key_links: + - from: "JKSessionScreen cleanup useEffect" + to: "unregisterMessageCallback" + via: "registeredCallbacksRef.current" + pattern: "registeredCallbacksRef\\.current\\.forEach" +--- + + +Harden session screen callback cleanup to ensure cleanup runs reliably on all exit paths + +Purpose: Defensive improvement to SESS-01 finding - ensure callback unregistration works even if normal leave flow doesn't execute (browser crash, navigation away, etc.) + +Output: JKSessionScreen.js with improved cleanup reliability using useRef pattern + + + +@/Users/nuwan/.claude/get-shit-done/workflows/execute-plan.md +@/Users/nuwan/.claude/get-shit-done/templates/summary.md + + + +@.planning/PROJECT.md +@.planning/ROADMAP.md +@.planning/phases/19-audit-and-discovery/19-01-SUMMARY.md + +Source files: +@jam-ui/src/components/client/JKSessionScreen.js + + + + + + Task 1: Improve callback cleanup reliability with useRef + jam-ui/src/components/client/JKSessionScreen.js + +The current implementation stores registered callbacks in useState, but the cleanup useEffect can have stale closure issues if the state was already cleared. The fix uses useRef to maintain a stable reference that cleanup can always access. + +**Changes to make:** + +1. Add a useRef to store callbacks (around line 293, after the useState declaration): +```javascript +const [registeredCallbacks, setRegisteredCallbacks] = useState([]); +const registeredCallbacksRef = useRef([]); +``` + +2. Update the setRegisteredCallbacks call (around line 629) to also update the ref: +```javascript +// Store registered callbacks for cleanup +setRegisteredCallbacks(callbacksToRegister); +registeredCallbacksRef.current = callbacksToRegister; +``` + +3. Update the unregisterMessageCallbacks function (around lines 296-301) to use the ref for cleanup reliability: +```javascript +const unregisterMessageCallbacks = useCallback(() => { + // Use ref for cleanup to avoid stale closure issues + const callbacks = registeredCallbacksRef.current; + callbacks.forEach(({ type, callback }) => { + unregisterMessageCallback(type, callback); + }); + registeredCallbacksRef.current = []; + setRegisteredCallbacks([]); +}, [unregisterMessageCallback]); +``` + +Note: Remove `registeredCallbacks` from the useCallback dependency array since we're now using the ref. + +**Why this works:** +- useRef maintains a stable reference across renders +- The cleanup function in useEffect will always have access to the current callbacks via ref +- This ensures cleanup runs correctly even if the component unmounts unexpectedly + + +1. Run `npm run lint` in jam-ui directory - should pass (or have no new lint errors) +2. Manually verify the code changes match the pattern described +3. Run existing tests: `cd jam-ui && npm test -- --grep "session"` (if any session tests exist) + + +- registeredCallbacksRef declared alongside registeredCallbacks state +- unregisterMessageCallbacks uses ref instead of state for cleanup +- Callback registration stores to both state and ref +- Cleanup will work reliably on all exit paths + + + + + Task 2: Document cleanup verification and update plan artifacts + N/A (verification task) + +Verify the fix by tracing through the code paths: + +1. **Normal leave flow (Leave button):** + - User clicks Leave → handleLeaveSubmit runs → unregisterMessageCallbacks() called + - Ref is cleared, then state is cleared + - useEffect cleanup also runs but ref is empty (no double unregister, which is fine) + +2. **Navigation away (browser back, route change):** + - Component unmounts → useEffect cleanup runs → unregisterMessageCallbacks() called + - Ref has current callbacks → callbacks are unregistered + - No memory leak + +3. **Browser close/crash:** + - Connection closes → jamServer cleanup handles it on server side + - This is out of scope for frontend cleanup (server handles orphaned connections) + +The fix ensures path 2 (navigation away) works correctly, which was the edge case identified in SESS-01. + + +1. Read through JKSessionScreen.js to confirm changes are correct +2. Verify the cleanup useEffect at ~line 934 will work with the new ref-based approach + + +- All exit paths verified to trigger cleanup +- Navigation away path confirmed to work with ref pattern +- No additional code changes needed + + + + + + +After completing all tasks: + +1. **Lint check:** `cd jam-ui && npm run lint` - no new errors +2. **Build check:** `cd jam-ui && npm run build` - builds successfully +3. **Code review:** Verify useRef pattern is correctly implemented +4. **Test execution:** Run any existing session-related tests + +Manual verification (optional, for UAT): +- Open session, then navigate away using browser back button +- Check console for cleanup logs (if debug logging is present) +- No errors should appear in console + + + +1. registeredCallbacksRef is used alongside registeredCallbacks state +2. unregisterMessageCallbacks reads from ref (not state) for cleanup +3. Callback registration stores to both state and ref +4. Code builds and passes lint +5. Existing tests continue to pass + + + +After completion, create `.planning/phases/22-session-screen-fixes/22-01-SUMMARY.md` +