docs(22): create phase plan for Session Screen Fixes
Phase 22: Session Screen Fixes - 1 plan in 1 wave - SESS-01: Harden callback cleanup with useRef pattern - Defensive improvement for reliable cleanup on all exit paths Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
Nuwan
parent
849465c43b
commit
f172739a82
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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"
|
||||
---
|
||||
|
||||
<objective>
|
||||
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
|
||||
</objective>
|
||||
|
||||
<execution_context>
|
||||
@/Users/nuwan/.claude/get-shit-done/workflows/execute-plan.md
|
||||
@/Users/nuwan/.claude/get-shit-done/templates/summary.md
|
||||
</execution_context>
|
||||
|
||||
<context>
|
||||
@.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
|
||||
</context>
|
||||
|
||||
<tasks>
|
||||
|
||||
<task type="auto">
|
||||
<name>Task 1: Improve callback cleanup reliability with useRef</name>
|
||||
<files>jam-ui/src/components/client/JKSessionScreen.js</files>
|
||||
<action>
|
||||
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
|
||||
</action>
|
||||
<verify>
|
||||
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)
|
||||
</verify>
|
||||
<done>
|
||||
- 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
|
||||
</done>
|
||||
</task>
|
||||
|
||||
<task type="auto">
|
||||
<name>Task 2: Document cleanup verification and update plan artifacts</name>
|
||||
<files>N/A (verification task)</files>
|
||||
<action>
|
||||
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.
|
||||
</action>
|
||||
<verify>
|
||||
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
|
||||
</verify>
|
||||
<done>
|
||||
- All exit paths verified to trigger cleanup
|
||||
- Navigation away path confirmed to work with ref pattern
|
||||
- No additional code changes needed
|
||||
</done>
|
||||
</task>
|
||||
|
||||
</tasks>
|
||||
|
||||
<verification>
|
||||
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
|
||||
</verification>
|
||||
|
||||
<success_criteria>
|
||||
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
|
||||
</success_criteria>
|
||||
|
||||
<output>
|
||||
After completion, create `.planning/phases/22-session-screen-fixes/22-01-SUMMARY.md`
|
||||
</output>
|
||||
Loading…
Reference in New Issue