fix(22): revise plan based on checker feedback
- Add acknowledgment that SESS-02/SESS-03 were verified clean in Phase 19 - Merge Task 2 (verification-only) into Task 1's verify section - Plan now has 1 auto task producing code artifacts Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
parent
f172739a82
commit
41513b81af
|
|
@ -30,6 +30,12 @@ Harden session screen callback cleanup to ensure cleanup runs reliably on all ex
|
|||
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
|
||||
|
||||
**Note on SESS-02 and SESS-03:** Phase 19 audit verified these requirements already satisfied:
|
||||
- SESS-02: useJamServer.js - All timers properly cleaned up (no issues found)
|
||||
- SESS-03: useSessionStats.js, useSessionModel.js - All timers properly cleaned up (no issues found)
|
||||
|
||||
Only SESS-01 requires code changes. This plan addresses SESS-01 exclusively.
|
||||
</objective>
|
||||
|
||||
<execution_context>
|
||||
|
|
@ -91,54 +97,31 @@ Note: Remove `registeredCallbacks` from the useCallback dependency array since w
|
|||
</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)
|
||||
2. Run `cd jam-ui && npm run build` - builds successfully
|
||||
3. Verify the code changes match the pattern described:
|
||||
- registeredCallbacksRef declared alongside registeredCallbacks state
|
||||
- unregisterMessageCallbacks uses ref instead of state for cleanup
|
||||
- Callback registration stores to both state and ref
|
||||
4. Run existing tests: `cd jam-ui && npm test -- --grep "session"` (if any session tests exist)
|
||||
5. Trace through exit path logic to confirm cleanup works:
|
||||
- Normal leave flow (Leave button): handleLeaveSubmit → unregisterMessageCallbacks() → ref cleared, state cleared
|
||||
- Navigation away (browser back, route change): unmount → useEffect cleanup → unregisterMessageCallbacks() → ref has callbacks → callbacks unregistered
|
||||
- Browser close/crash: Connection closes → server handles orphaned connections (out of scope for frontend)
|
||||
</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
|
||||
- Code builds and passes lint
|
||||
- Existing tests continue to pass
|
||||
</done>
|
||||
</task>
|
||||
|
||||
</tasks>
|
||||
|
||||
<verification>
|
||||
After completing all tasks:
|
||||
After completing the task:
|
||||
|
||||
1. **Lint check:** `cd jam-ui && npm run lint` - no new errors
|
||||
2. **Build check:** `cd jam-ui && npm run build` - builds successfully
|
||||
|
|
|
|||
Loading…
Reference in New Issue