test: address PR170 review residual risks (2, 3, 5)#175
Conversation
- Extract lifecycle helpers (inferProviderFromMessageType, resolveProjectName, enrichSessionEventPayload, buildLifecycleMessageFromPayload) into server/utils/sessionLifecycle.js for testability; server/index.js delegates to the extracted module with real DB/fs deps via a shared singleton. - Add 33 unit tests for the extracted lifecycle functions covering provider inference, project name resolution, payload enrichment, and lifecycle message construction (risk 2). - Add 2 codexQueue tests for order preservation under concurrent temp→settled promotions and promotion-then-reconciliation interleave (risk 3). - Document localStorage message cache key migration in CHANGELOG (risk 5). - Risk 4 (provider array consolidation) deferred — too invasive for this PR. All 149 tests pass. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
b207b52
into
OpenLAIR:redo-session-lifecycle-after-167-revert
There was a problem hiding this comment.
Pull request overview
This PR extracts session lifecycle helper logic from server/index.js into a dedicated utility module to improve testability, adds unit tests for the extracted helpers, and expands queue reconciliation tests to cover ordering edge cases; it also documents a localStorage key migration.
Changes:
- Extracted session lifecycle helper functions into
server/utils/sessionLifecycle.jsand delegated server runtime calls via injected deps. - Added unit tests for lifecycle helper behavior and new
codexQueueordering tests around reconciliation/promotion interleavings. - Added a CHANGELOG migration note for provider-scoped localStorage message cache keys.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/components/chat/utils/tests/codexQueue.test.ts | Adds concurrency/order-preservation test cases for temp→settled reconciliation and steer promotion interleavings. |
| server/utils/sessionLifecycle.js | New extracted lifecycle helper module (provider inference, project name resolution, payload enrichment, lifecycle message construction). |
| server/index.js | Replaces inlined lifecycle helpers with delegating wrappers wired to real fs/DB deps via a shared singleton. |
| server/tests/session-lifecycle.test.mjs | Adds unit tests covering the extracted lifecycle helper functions. |
| CHANGELOG.md | Documents localStorage message cache key migration guidance for upgrades. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * filesystem path. When `isKnownPath` and `encodePath` callbacks are | ||
| * provided the function validates the path before encoding; otherwise it | ||
| * performs a simple passthrough (useful in unit tests that don't need a | ||
| * real filesystem). |
There was a problem hiding this comment.
The JSDoc for resolveProjectName says that when deps aren’t provided it “performs a simple passthrough”, but the implementation returns null when isKnownPath/encodePath aren’t functions. Please update the comment to match the actual behavior (or adjust the function to do the described passthrough).
| * filesystem path. When `isKnownPath` and `encodePath` callbacks are | |
| * provided the function validates the path before encoding; otherwise it | |
| * performs a simple passthrough (useful in unit tests that don't need a | |
| * real filesystem). | |
| * filesystem path. When `isKnownPath` and `encodePath` callbacks are | |
| * provided, the function validates the path before encoding it. Without | |
| * both callbacks, path-based resolution is skipped and the function returns | |
| * `null` unless a non-empty explicit `projectName` was provided. |
| - Ensured session lifecycle protocol is emitted consistently for Nano just like other providers. | ||
|
|
||
| #### Migration notes | ||
| - **Message cache key change**: Session message cache keys are now scoped by provider (`chat_messages_{project}_{provider}_{session}`). Existing localStorage entries keyed the old way (`chat_messages_{project}_{session}`) will not be read by default. To migrate, set `allowLegacyFallback: true` when calling `getSessionMessageCacheKeys()`. Users upgrading from builds prior to this sync may see empty transcript history on first load for previously-active sessions; the data is still in localStorage and can be recovered by enabling the legacy fallback or manually re-keying the entries. |
There was a problem hiding this comment.
The changelog refers to getSessionMessageCacheKeys(), but that identifier doesn’t appear in the codebase (the current helper is buildSessionMessageCacheCandidateKeys(...) in src/components/chat/utils/sessionMessageCache.ts). Please update the migration note to reference the correct function name/API so readers can actually apply allowLegacyFallback: true.
| - **Message cache key change**: Session message cache keys are now scoped by provider (`chat_messages_{project}_{provider}_{session}`). Existing localStorage entries keyed the old way (`chat_messages_{project}_{session}`) will not be read by default. To migrate, set `allowLegacyFallback: true` when calling `getSessionMessageCacheKeys()`. Users upgrading from builds prior to this sync may see empty transcript history on first load for previously-active sessions; the data is still in localStorage and can be recovered by enabling the legacy fallback or manually re-keying the entries. | |
| - **Message cache key change**: Session message cache keys are now scoped by provider (`chat_messages_{project}_{provider}_{session}`). Existing localStorage entries keyed the old way (`chat_messages_{project}_{session}`) will not be read by default. To migrate, set `allowLegacyFallback: true` when calling `buildSessionMessageCacheCandidateKeys(...)`. Users upgrading from builds prior to this sync may see empty transcript history on first load for previously-active sessions; the data is still in localStorage and can be recovered by enabling the legacy fallback or manually re-keying the entries. |
All 149 tests pass.