feat: add --continue flag for session resumption #999
Conversation
Add persistent session storage to resume previous conversations. Sessions are stored per project with:
- Full conversation history (IContent[] format for API context)
- UI display state (HistoryItem[] for exact visual restoration)
- Project hash validation to prevent cross-project session loading
- Automatic save on turn completion, automatic restore on --continue flag
New SessionPersistenceService handles persistence logic. Sessions saved to:
~/.llxprt/tmp/{project-hash}/chats/persisted-session-{timestamp}.json
Closes vybestack#147
WalkthroughAdds a CLI Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI Entry
participant ArgParser as Arg Parser (yargs)
participant SessionSvc as SessionPersistenceService
participant Storage as Disk Storage
participant UI as AppContainer / UI
participant AI as AI Context / HistoryService
CLI->>ArgParser: Parse args (--continue)
ArgParser-->>CLI: CliArgs{ continue: true }
CLI->>SessionSvc: new SessionPersistenceService(storage, sessionId)
CLI->>SessionSvc: loadMostRecent()
activate SessionSvc
SessionSvc->>Storage: readdir & readFile latest
Storage-->>SessionSvc: persisted JSON
SessionSvc->>SessionSvc: validate projectHash & version
SessionSvc-->>CLI: PersistedSession | null
deactivate SessionSvc
CLI->>UI: render AppContainer(restoredSession)
UI->>UI: validate/convert uiHistory
UI->>AI: restore core history into AI context
AI-->>UI: history restored / warning
UI->>UI: show restoration status
alt On user-turn completion (streaming -> idle)
UI->>SessionSvc: save(history, metadata, uiHistory)
activate SessionSvc
SessionSvc->>Storage: write .tmp -> rename (atomic)
Storage-->>SessionSvc: persisted
deactivate SessionSvc
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (4 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
LLxprt PR Review – PR #999Issue Alignment
Side Effects
Code Quality
Tests & Coverage
Verdict |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/core/src/storage/SessionPersistenceService.ts (2)
94-105: Potential timestamp collision in high-frequency scenarios.The timestamp-based filename uses millisecond precision but replaces colons and dots, which could theoretically cause collisions if two instances are created within the same millisecond. This is unlikely in practice but worth noting.
🔎 Consider adding session ID to filename for uniqueness
- const timestamp = new Date().toISOString().replace(/[:.]/g, '-'); - this.sessionFilePath = path.join( - this.chatsDir, - `${PERSISTED_SESSION_PREFIX}${timestamp}.json`, - ); + const timestamp = new Date().toISOString().replace(/[:.]/g, '-'); + this.sessionFilePath = path.join( + this.chatsDir, + `${PERSISTED_SESSION_PREFIX}${timestamp}-${sessionId.slice(0, 8)}.json`, + );
266-285: Consider handling backup failure more gracefully.The backup operation catches and logs errors but doesn't propagate them. While this is intentional (to avoid blocking on backup failures), a corrupted file that fails to backup will remain in place and be retried on next load, potentially causing repeated errors.
The current behavior is acceptable since the backup is best-effort, but you could consider deleting the corrupted file if backup fails to prevent repeated errors.
packages/core/src/storage/SessionPersistenceService.test.ts (1)
226-246: Same issue - tests mockexistsSyncbut implementation usesreaddir.These tests also mock
existsSyncwhich isn't used by the implementation. However, they do properly mockreaddir, so the tests should pass - theexistsSyncmock is just redundant.Remove the redundant
vi.mocked(fs.existsSync).mockReturnValue(true)lines since the implementation doesn't useexistsSync.packages/cli/src/ui/AppContainer.tsx (1)
665-705: Polling interval should be cleaned up on success.The interval is cleared when historyService is found, which is correct. However, consider adding a timeout to avoid infinite polling if historyService never becomes available.
🔎 Consider adding a timeout for core history restoration
useEffect(() => { if (!restoredSession || coreHistoryRestoredRef.current) { return; } + const timeout = setTimeout(() => { + clearInterval(checkInterval); + debug.warn('Timed out waiting for history service to restore core history'); + addItem( + { + type: 'warning', + text: 'Could not restore AI context - history service unavailable.', + }, + Date.now(), + ); + }, 30000); // 30 second timeout const checkInterval = setInterval(() => { // ... existing code ... }, 100); return () => { clearInterval(checkInterval); + clearTimeout(timeout); }; }, [restoredSession, config, addItem]);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
packages/cli/src/config/config.loadMemory.test.tspackages/cli/src/config/config.tspackages/cli/src/gemini.tsxpackages/cli/src/ui/App.tsxpackages/cli/src/ui/AppContainer.tsxpackages/core/src/config/config.tspackages/core/src/index.tspackages/core/src/storage/SessionPersistenceService.test.tspackages/core/src/storage/SessionPersistenceService.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Don't useany- Always specify proper types. Useunknownif the type is truly unknown and add proper type guards.
Do not useconsole.logorconsole.debug- Use the sophisticated logging system instead. Log files are written to ~/.llxprt/debug/
Fix all linting errors, including warnings aboutanytypes
Files:
packages/core/src/config/config.tspackages/core/src/storage/SessionPersistenceService.test.tspackages/cli/src/config/config.tspackages/cli/src/ui/App.tsxpackages/cli/src/config/config.loadMemory.test.tspackages/core/src/index.tspackages/cli/src/gemini.tsxpackages/core/src/storage/SessionPersistenceService.tspackages/cli/src/ui/AppContainer.tsx
🧬 Code graph analysis (4)
packages/core/src/storage/SessionPersistenceService.test.ts (1)
packages/core/src/storage/SessionPersistenceService.ts (3)
SessionPersistenceService(88-286)PersistedSession(56-77)getProjectHash(247-250)
packages/cli/src/ui/App.tsx (2)
packages/core/src/index.ts (1)
PersistedSession(359-359)packages/core/src/storage/SessionPersistenceService.ts (1)
PersistedSession(56-77)
packages/cli/src/gemini.tsx (1)
packages/core/src/storage/SessionPersistenceService.ts (2)
PersistedSession(56-77)SessionPersistenceService(88-286)
packages/cli/src/ui/AppContainer.tsx (2)
packages/core/src/storage/SessionPersistenceService.ts (2)
PersistedSession(56-77)SessionPersistenceService(88-286)packages/cli/src/ui/types.ts (2)
HistoryItem(246-246)IndividualToolCallDisplay(47-56)
🔇 Additional comments (31)
packages/cli/src/config/config.loadMemory.test.ts (1)
328-328: LGTM!The test correctly includes the new
continuefield to match the updatedCliArgsinterface.packages/core/src/config/config.ts (2)
362-362: LGTM!The
continueSessionparameter is properly defined as optional in the interface and stored as a private readonly field in the Config class, following the established patterns in this codebase.Also applies to: 523-523
661-661: LGTM!The initialization properly defaults to
falsewhen not provided, and theisContinueSession()accessor provides appropriate encapsulation. The implementation follows existing patterns in the Config class.Also applies to: 862-864
packages/cli/src/config/config.ts (4)
170-170: LGTM!The
continuefield is properly added to theCliArgsinterface with the correct type matching other optional boolean CLI flags.
358-364: LGTM!The CLI option is well-defined with a clear description and appropriate defaults. The ability to combine
--continuewith--promptis explicitly documented in the help text, enabling users to resume a session and immediately send a new message.
651-651: LGTM!The argument mapping correctly extracts the
continueflag from the yargs result and assigns it to theCliArgsobject, following the established pattern for other CLI arguments.
1329-1329: LGTM!The
continueSessionparameter is correctly passed to the Config constructor with a safe default value offalse, properly integrating the CLI flag with the core configuration system.packages/core/src/storage/SessionPersistenceService.ts (7)
1-14: LGTM - License, imports, and logging setup are correct.The file properly uses
DebugLoggerper coding guidelines instead ofconsole.log. Imports are appropriate for the functionality.
16-51: LGTM - Type definitions are well-structured.The
PersistedToolCallandPersistedUIHistoryIteminterfaces are properly documented with JSDoc comments and use appropriate types.
53-77: LGTM - PersistedSession interface is well-designed.The schema includes versioning for future migrations, project hash for validation, and optional metadata. The
version: 1literal type is a good practice for schema evolution.
124-162: LGTM - Atomic write pattern is correctly implemented.The save method properly:
- Creates directory recursively
- Writes to a temp file first
- Renames atomically to the final path
- Logs operations via DebugLogger
- Re-throws errors after logging
167-234: LGTM - Load logic with proper validation and error handling.The
loadMostRecentmethod correctly:
- Handles missing directory (ENOENT)
- Filters and sorts session files by timestamp
- Validates project hash to prevent cross-project session loading
- Validates schema version
- Backs up corrupted files gracefully
239-242: Verify locale-dependent date formatting behavior.
toLocaleString()produces different output based on system locale. If this string is used for anything other than display (e.g., parsing), it could cause issues.Ensure this formatted time is only used for user-facing display and not for any date comparisons or parsing operations.
247-250: LGTM - Project hash implementation is secure.Using SHA-256 to hash the project root path is appropriate for validation purposes. The hash is deterministic and collision-resistant.
packages/cli/src/ui/App.tsx (2)
8-8: LGTM - Type import is correct.Using
typeimport forPersistedSessionis appropriate since it's only used for type annotations.
26-32: LGTM - AppProps interface properly extended.The
restoredSessionprop is correctly typed as optional and uses the exportedPersistedSessiontype from core.packages/core/src/index.ts (1)
357-362: LGTM - Public API exports are correctly structured.The exports properly expose the
SessionPersistenceServiceclass and the three related types (PersistedSession,PersistedUIHistoryItem,PersistedToolCall) for external consumption.packages/cli/src/gemini.tsx (3)
77-79: LGTM - Imports are correctly structured.The import of
SessionPersistenceServiceclass andPersistedSessiontype follows the pattern used elsewhere in the file.
223-265: Console output uses chalk for user-facing messaging - acceptable.The
console.logcalls here are intentional user-facing CLI output (startup messages) using chalk for formatting, not debug logging. This is appropriate for CLI UX and doesn't violate the logging guidelines which prohibitconsole.logfor debug purposes.The session restoration logic properly:
- Creates the persistence service with storage and session ID
- Awaits the async load operation
- Provides clear user feedback for all scenarios (session found with/without prompt, no session found)
306-306: LGTM - Correctly converts null to undefined for prop.The
restoredSession ?? undefinedconversion properly handles thenullreturn fromloadMostRecent()to match the optional prop type inAppWrapper.packages/core/src/storage/SessionPersistenceService.test.ts (4)
1-33: LGTM - Test setup is well-structured.The mock setup properly overrides
node:fspromises API while preserving the actual module structure. The import of realcryptoallows testing actual hash behavior.
374-397: LGTM - Error handling tests are comprehensive.Tests properly verify that readdir and readFile failures are handled gracefully by returning null.
451-491: Testing private method via type casting is acceptable for thorough coverage.While testing private methods is generally discouraged, verifying the project hash consistency and format is important for security. The
as unknown as { getProjectHash(): string }pattern is a reasonable pragmatic choice.
493-537: LGTM - Edge case tests are valuable.Testing empty arrays, large histories, and special characters in paths helps ensure robustness.
packages/cli/src/ui/AppContainer.tsx (7)
22-23: LGTM - Imports are correctly structured.The new imports for
IndividualToolCallDisplay,SessionPersistenceService,PersistedSession,IContent,ToolCallBlock, andToolResponseBlockare all properly imported and used in the file.Also applies to: 68-73
154-178: LGTM - Valid history types set is properly defined at module level.Defining
VALID_HISTORY_TYPESat module level ensures a stable reference for use in validation callbacks, avoiding unnecessary re-creation.
384-485: Well-structured conversion with appropriate status mapping.The
convertToUIHistoryfunction properly:
- Collects tool responses into a map for lookup
- Handles human, AI text, and tool call blocks
- Maps tool response status correctly (Success if response exists, Pending otherwise)
One consideration: tools without responses are marked as
Pending, which is accurate for conversion but may look odd if the session was interrupted mid-tool-execution.
496-567: LGTM - Validation logic is robust with good fallback behavior.The validation:
- Uses duck typing for flexibility with schema changes
- Warns about unknown types but allows them (forward compatibility)
- Validates type-specific fields appropriately
- Returns both valid items and invalid count for error reporting
569-663: LGTM - UI restoration with graceful degradation.The restoration effect properly:
- Uses ref to prevent double-execution
- Validates UI history before loading
- Falls back to conversion if all items are invalid
- Reports restoration status and any warnings to user
- Handles errors gracefully with user notification
1812-1861: Session persistence implementation is well-designed.The persistence logic:
- Uses stable dependencies (
storage,sessionId) to avoid recreating the service- Detects turn completion via streaming state transition
- Saves both core history and UI history for exact restoration
- Uses fire-and-forget with
.catch()to avoid blocking UIOne consideration: the
historyvariable passed tosave()on line 1856 is the UI history fromuseHistory, which is correct for display restoration.
1825-1834: This suggestion is not applicable—StreamingState.Activedoes not exist.The
StreamingStateenum contains only three values:Idle,Responding, andWaitingForConfirmation. The current check correctly covers all active states by including bothRespondingandWaitingForConfirmation, which are the only non-idle streaming states in the codebase.Likely an incorrect or invalid review comment.
…n and error handling Add comprehensive improvements to session persistence for --continue flag: - Add type-safe session interfaces (PersistedUIHistoryItem, PersistedToolCall) - Add 30 unit tests for SessionPersistenceService covering all code paths - Add runtime validation for persisted UI history with graceful fallback - Add user notifications when core history restoration fails - Fix memory leak in useMemo by using stable dependencies - Add clear messaging when combining --continue with --prompt flags - Replace sync fs.existsSync with async readdir (eliminates race condition) - Update --continue help text to document --prompt compatibility Fixes issue vybestack#147 session resumption concerns.
sogoiii
force-pushed
the
feature/issue-147-continue
branch
from
8e207e3 to
f4e97ee
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/cli/src/ui/AppContainer.tsx (1)
384-485: Consider extracting history conversion to a separate utility module.The
convertToUIHistoryfunction is well-implemented but quite large (100+ lines). Extracting it to a separate utility module would improve:
- Testability (easier to unit test in isolation)
- Maintainability (clearer separation of concerns)
- Reusability (could be used elsewhere if needed)
💡 Suggested extraction approach
Create a new file
packages/cli/src/ui/utils/historyConversion.ts:export function convertToUIHistory(history: IContent[]): HistoryItem[] { // Move the entire function body here }Then import and use in AppContainer:
+import { convertToUIHistory } from './utils/historyConversion.js'; + export const AppContainer = (props: AppContainerProps) => { - const convertToUIHistory = useCallback( - (history: IContent[]): HistoryItem[] => { - // ... 100 lines - }, - [], - ); + // Use the imported utility directlypackages/core/src/storage/SessionPersistenceService.ts (1)
177-244: Consider runtime schema validation for loaded sessions.The method properly uses async filesystem operations and validates project hash and version. However, the JSON.parse at line 208 trusts the structure beyond basic version checking.
Consider adding runtime type guards for critical fields:
💡 Suggested validation approach
function isValidPersistedSession(obj: unknown): obj is PersistedSession { if (typeof obj !== 'object' || obj === null) return false; const session = obj as Record<string, unknown>; return ( typeof session.version === 'number' && typeof session.sessionId === 'string' && typeof session.projectHash === 'string' && typeof session.createdAt === 'string' && typeof session.updatedAt === 'string' && Array.isArray(session.history) ); } // In loadMostRecent: const parsed = JSON.parse(content); if (!isValidPersistedSession(parsed)) { logger.warn('Session file has invalid structure'); await this.backupCorruptedSession(); return null; } const session = parsed;This would catch structural corruption earlier and provide clearer error messages.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/cli/src/config/config.tspackages/cli/src/gemini.tsxpackages/cli/src/ui/AppContainer.tsxpackages/core/src/index.tspackages/core/src/storage/SessionPersistenceService.test.tspackages/core/src/storage/SessionPersistenceService.tsproject-plans/20260103-continue-improvements/SESSION-CONTINUE-IMPROVEMENTS.md
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/core/src/index.ts
- packages/core/src/storage/SessionPersistenceService.test.ts
- packages/cli/src/gemini.tsx
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Don't useany- Always specify proper types. Useunknownif the type is truly unknown and add proper type guards.
Do not useconsole.logorconsole.debug- Use the sophisticated logging system instead. Log files are written to ~/.llxprt/debug/
Fix all linting errors, including warnings aboutanytypes
Files:
packages/cli/src/config/config.tspackages/core/src/storage/SessionPersistenceService.tspackages/cli/src/ui/AppContainer.tsx
**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
Documentation-only changes (*.md files, docs/) do NOT require build/test/lint cycle
Files:
project-plans/20260103-continue-improvements/SESSION-CONTINUE-IMPROVEMENTS.md
🧠 Learnings (1)
📚 Learning: 2025-12-12T23:31:19.937Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 779
File: project-plans/20251212issue488/PLAN.md:1-7
Timestamp: 2025-12-12T23:31:19.937Z
Learning: Do not validate or review plan documents in the vybestack/llxprt-code repository after implementation. Specifically, skip validation for files under project-plans (e.g., project-plans/**/PLAN.md) once the feature is implemented; these documents are not checked in reviews post-implementation.
Applied to files:
project-plans/20260103-continue-improvements/SESSION-CONTINUE-IMPROVEMENTS.md
🧬 Code graph analysis (1)
packages/core/src/storage/SessionPersistenceService.ts (2)
packages/core/src/tools/tools.ts (2)
ToolResultDisplay(538-538)ToolCallConfirmationDetails(600-604)packages/core/src/services/history/IContent.ts (1)
IContent(21-40)
🪛 markdownlint-cli2 (0.18.1)
project-plans/20260103-continue-improvements/SESSION-CONTINUE-IMPROVEMENTS.md
76-76: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
89-89: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
101-101: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
190-190: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
251-251: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
276-276: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
879-879: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
894-894: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
925-925: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🔇 Additional comments (14)
packages/cli/src/config/config.ts (4)
170-170: LGTM: CLI argument type definition follows established patterns.The
continuefield typeboolean | undefinedis consistent with other optional CLI flags in the interface.
651-651: LGTM: Argument parsing follows standard pattern.The mapping from
result.continueto the CliArgs structure is consistent with other CLI arguments.
1329-1329: LGTM: Config constructor receives the continue flag with proper defaulting.The nullish coalescing operator (
??) correctly handles undefined values and provides a sensible default offalse.
358-364: The description references--promptbut the actual flag is--message.The
--continueoption on lines 358-364 claims it "Can be combined with --prompt," but there is no--promptoption in the CLI. The correct flag name is--message. This should be corrected in the description to maintain consistency with the actual CLI interface.Likely an incorrect or invalid review comment.
packages/cli/src/ui/AppContainer.tsx (6)
22-22: LGTM: Session persistence types properly imported and integrated.The new imports from
@vybestack/llxprt-code-coreprovide the necessary types for session restoration. The optionalrestoredSessionprop inAppContainerPropsmaintains backward compatibility.Also applies to: 68-72, 140-140
154-178: LGTM: Module-level Set provides stable validation reference.Defining
VALID_HISTORY_TYPESat module level ensures:
- Stable reference across component re-renders
- Efficient O(1) type validation lookups
- Comprehensive coverage of known history item types
496-567: LGTM: Comprehensive validation with forward compatibility.The validation functions properly check required fields and handle type-specific requirements. The decision to allow unknown types (lines 509-511) is good for forward compatibility—older versions can still load sessions created by newer versions without failing on new item types.
570-663: LGTM: Robust UI history restoration with graceful fallbacks.The restoration logic properly:
- Validates UI history items before use
- Falls back to core history conversion when needed
- Provides clear user feedback about restoration status
- Handles partial corruption without failing entirely
The multi-layered approach (validate → use valid items → or fallback → notify user) ensures a good user experience even with corrupted data.
665-705: LGTM: Critical user notification prevents confusion from partial restoration.The core history restoration properly handles the async availability of
historyServicewith polling. Most importantly, the error handling (lines 689-698) warns users when AI context restoration fails, preventing the confusing state where the UI shows previous messages but the AI has no memory of them.
1812-1861: LGTM: Session persistence uses stable dependencies and proper turn completion detection.The implementation correctly addresses potential issues:
Lines 1814-1819: Using
[storage, sessionId]as dependencies instead of[config]prevents unnecessary recreation ofSessionPersistenceService(avoids memory leak and file path churn).Lines 1825-1830: The turn completion detection properly tracks when streaming goes from Active/WaitingForConfirmation to Idle, ensuring persistence at the right moments.
Lines 1856: Saving both core history and UI history enables exact restoration of both AI context and display state.
Lines 1858-1860: Error handling logs failures but doesn't crash the app, maintaining stability.
packages/core/src/storage/SessionPersistenceService.ts (4)
24-87: LGTM: Well-designed type definitions with versioning and flexibility.The persisted types strike a good balance:
PersistedToolCall(lines 24-41): Comprehensive tool display informationPersistedUIHistoryItem(lines 48-61): Permissive structure accommodates different UI item typesPersistedSession(lines 66-87): Includesversion: 1for future schema migrationsThe optional
uiHistoryfield addresses type safety concerns while maintaining backward compatibility.
104-115: LGTM: Constructor creates project-scoped session files with sortable timestamps.The implementation correctly:
- Isolates sessions per project using
storage.getProjectTempDir()- Uses ISO timestamp with filesystem-safe characters (line 110)
- Enables "most recent" lookup via alphabetical sorting of filenames
When combined with stable dependencies in AppContainer's
useMemo, this creates one file per session.
134-172: LGTM: Atomic write pattern ensures data integrity.The save method implements best practices:
- Line 141: Creates directory recursively (idempotent)
- Lines 154-161: Atomic write pattern (write to temp, then rename) prevents corruption from crashes or kills
- Line 158: Pretty-printed JSON aids debugging
- Lines 169-171: Proper error handling with re-throw
249-296: LGTM: Well-designed helper methods with proper error handling.The helper methods provide good utility:
- Line 249-252:
formatSessionTimeuses locale-aware formatting- Lines 257-260: SHA-256 hash provides reliable project identification
- Lines 265-271: Lazy initialization with caching for
createdAtmaintains timestamp stability- Lines 276-295: Corruption backup preserves data for debugging without blocking functionality
…st mocks - Add 30s timeout to core history restoration polling to prevent infinite waits when historyService unavailable - Fix test mock for directory-not-exist case to properly use readdir ENOENT error instead of unused existsSync - Remove redundant existsSync mocks from all loadMostRecent tests
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/cli/src/ui/AppContainer.tsx (4)
753-753: Replace console.error with debug logger.Per coding guidelines, use the sophisticated logging system instead of
console.error. ADebugLoggeris already available (line 130).🔎 Proposed fix
+const rawModeLogger = new DebugLogger('llxprt:ui:rawmode'); + const restoreTerminalStateAfterEditor = useCallback(() => { const editorState = externalEditorStateRef.current; // ... existing code ... if (editorState?.rawModeManaged && setRawMode) { try { setRawMode(true); } catch (error) { - console.error('Failed to re-enable raw mode:', error); + rawModeLogger.error('Failed to re-enable raw mode:', error); } } // ... rest of function }, [setRawMode, stdin]); const handleExternalEditorOpen = useCallback(() => { // ... existing code ... if (setRawMode) { try { setRawMode(false); externalEditorStateRef.current.rawModeManaged = true; } catch (error) { - console.error('Failed to disable raw mode:', error); + rawModeLogger.error('Failed to disable raw mode:', error); } } // ... rest of function }, [setRawMode, stdin, stdout]);Also applies to: 817-817
1100-1100: Replace console.error with debug logger.Per coding guidelines, use the sophisticated logging system instead of
console.error.🔎 Proposed fix
+const modelLogger = new DebugLogger('llxprt:ui:model'); +const memoryLogger = new DebugLogger('llxprt:ui:memory'); + const openProviderModelDialog = useCallback(async () => { try { const models = await runtime.listAvailableModels(); setProviderModels(models); } catch (e) { - console.error('Failed to load models:', e); + modelLogger.error('Failed to load models:', e); setProviderModels([]); } await openProviderModelDialogRaw(); }, [openProviderModelDialogRaw, runtime]); const performMemoryRefresh = useCallback(async () => { // ... existing code ... } catch (error) { const errorMessage = getErrorMessage(error); addItem( { type: MessageType.ERROR, text: `Error refreshing memory: ${errorMessage}`, }, Date.now(), ); - console.error('Error refreshing memory:', error); + memoryLogger.error('Error refreshing memory:', error); } }, [config, addItem, settings.merged]);Also applies to: 1231-1231
1117-1119: Replace console.debug with debug logger.Per coding guidelines, use the sophisticated logging system instead of
console.debug.🔎 Proposed fix
+const modelLogger = new DebugLogger('llxprt:ui:model'); + const checkModelChange = () => { const configModel = config.getModel(); const providerModel = runtime.getActiveModelName(); const effectiveModel = providerModel && providerModel.trim() !== '' ? providerModel : configModel; if (effectiveModel !== currentModel) { - console.debug( - `[Model Update] Updating footer from ${currentModel} to ${effectiveModel}`, - ); + modelLogger.debug( + () => `Updating footer from ${currentModel} to ${effectiveModel}`, + ); setCurrentModel(effectiveModel); } };
1219-1219: Replace guarded console.log with debug logger.Even though these are guarded by debug flags, per coding guidelines use the sophisticated logging system. Debug loggers support conditional logging via their
log/debugmethods with function arguments.🔎 Proposed fix
+const memoryLogger = new DebugLogger('llxprt:ui:memory'); + const performMemoryRefresh = useCallback(async () => { // ... existing code ... addItem( { type: MessageType.INFO, text: `Memory refreshed successfully. ${/* ... */}`, }, Date.now(), ); - if (config.getDebugMode()) { - console.log( - `[DEBUG] Refreshed memory content in config: ${memoryContent.substring(0, 200)}...`, - ); - } + memoryLogger.debug( + () => `Refreshed memory content in config: ${memoryContent.substring(0, 200)}...`, + ); }, [config, addItem, settings.merged]); const handleGlobalKeypress = useCallback( (key: Key) => { - // Debug log keystrokes if enabled - if (settings.merged.debugKeystrokeLogging) { - console.log('[DEBUG] Keystroke:', JSON.stringify(key)); - } + // Debug log keystrokes (controlled by logger's enabled state) + const keystrokeLogger = new DebugLogger('llxprt:ui:keystrokes'); + keystrokeLogger.debug(() => `Keystroke: ${JSON.stringify(key)}`); // ... rest of handler }, // ... dependencies );Note: DebugLogger's
debug()method only logs when the logger is enabled, so the conditional check is unnecessary.Also applies to: 1571-1571
♻️ Duplicate comments (1)
packages/core/src/storage/SessionPersistenceService.test.ts (1)
217-225: Past issue resolved: Test now properly simulates missing directory.The test correctly mocks
readdirto reject with an ENOENT error, matching the implementation's error handling path. This addresses the previous review concern about mockingexistsSyncwhen the implementation usesreaddir.
🧹 Nitpick comments (1)
packages/core/src/storage/SessionPersistenceService.test.ts (1)
443-483: Consider alternatives for testing private methods.The tests access private
getProjectHash()using type assertions (as unknown as { getProjectHash(): string }). While functional, consider these alternatives for cleaner test code:
- Make
getProjectHash()package-private (internal visibility) for testing- Test hash consistency indirectly through saved session data
- Export a test-only utility that exposes internal methods
Alternative approach using saved session data
Instead of accessing the private method directly, you could verify hash consistency by examining the
projectHashfield in saved sessions:it('should generate consistent hash for same project', async () => { + vi.mocked(fs.promises.mkdir).mockResolvedValue(undefined); + vi.mocked(fs.promises.writeFile).mockResolvedValue(); + vi.mocked(fs.promises.rename).mockResolvedValue(); + const service1 = new SessionPersistenceService(storage, 'session1'); const service2 = new SessionPersistenceService(storage, 'session2'); - // Access private method via any - const hash1 = ( - service1 as unknown as { getProjectHash(): string } - ).getProjectHash(); - const hash2 = ( - service2 as unknown as { getProjectHash(): string } - ).getProjectHash(); + let hash1: string = ''; + let hash2: string = ''; + + vi.mocked(fs.promises.writeFile).mockImplementation(async (_path, content) => { + const parsed = JSON.parse(content as string) as PersistedSession; + if (!hash1) hash1 = parsed.projectHash; + else hash2 = parsed.projectHash; + }); + + await service1.save([], undefined, []); + await service2.save([], undefined, []); expect(hash1).toBe(hash2); });
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/cli/src/ui/AppContainer.tsxpackages/core/src/storage/SessionPersistenceService.test.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Don't useany- Always specify proper types. Useunknownif the type is truly unknown and add proper type guards.
Do not useconsole.logorconsole.debug- Use the sophisticated logging system instead. Log files are written to ~/.llxprt/debug/
Fix all linting errors, including warnings aboutanytypes
Files:
packages/core/src/storage/SessionPersistenceService.test.tspackages/cli/src/ui/AppContainer.tsx
🔇 Additional comments (10)
packages/core/src/storage/SessionPersistenceService.test.ts (4)
50-73: LGTM: Constructor tests are well-structured.The tests properly verify path generation and timestamp uniqueness with appropriate timing delays.
75-211: LGTM: Comprehensive coverage of save() functionality.The tests validate:
- Atomic write operations (temp file → rename)
- Complete data serialization with proper field validation
- Timestamp preservation/update behavior
- Error handling for all filesystem operations
227-390: LGTM: Thorough testing of session loading and validation.Excellent coverage including:
- File filtering and chronological sorting
- Project hash and version validation
- Corrupted JSON handling with backup creation
- Graceful error handling for filesystem failures
485-529: LGTM: Good edge case coverage.The tests verify handling of boundary conditions including empty histories, large datasets (1000 items), and special characters in paths.
packages/cli/src/ui/AppContainer.tsx (6)
154-178: LGTM: Well-designed validation constant.The module-level
VALID_HISTORY_TYPESSet provides a stable reference for validation across component renders. The comprehensive list of types ensures robust validation during session restoration.
384-485: LGTM: Well-structured history conversion with proper type guards.The
convertToUIHistoryfunction correctly:
- Maps tool responses by callId for efficient lookup
- Converts IContent blocks to HistoryItem format
- Handles multiple result formats (string, object with output field, generic object)
- Uses type assertions followed by runtime type checks for safety
The two-pass approach (building toolResponseMap first) is a good optimization.
496-567: LGTM: Robust validation with forward compatibility.The validation utilities provide:
- Duck-typed validation for flexibility across schema versions
- Forward compatibility by allowing unknown types with warnings (line 509-512)
- Type-specific validation for critical fields (tool_group requires tools array, text types allow optional text)
- Graceful degradation by filtering invalid items while preserving valid ones
569-663: LGTM: Well-designed UI history restoration with fallback.The restoration logic provides:
- Duplicate prevention via
sessionRestoredRef- Validation of persisted UI history before use
- Graceful fallback to core history conversion when UI history is invalid
- Clear user feedback about restoration status and any data loss
- Comprehensive error handling with user-facing warnings
The two-path approach (saved UI history vs. converted) ensures the best possible restoration experience.
665-723: LGTM: Robust core history restoration with timeout protection.The core history restoration provides:
- Polling for history service availability (appropriate for async initialization)
- 30-second timeout to prevent infinite waits (as noted in commit messages)
- Clear separation from UI restoration (allows UI to show while waiting for AI context)
- Critical user warning when AI context cannot be restored (lines 709-715)
The timeout implementation addresses the concern mentioned in the commit messages about avoiding infinite waits when the history service is unavailable.
1830-1879: LGTM: Well-implemented automatic session persistence.The persistence logic correctly:
- Creates service with stable dependencies to avoid path changes (lines 1834-1837)
- Detects turn completion by tracking streaming state transitions (lines 1844-1852)
- Saves both core history and UI history for complete restoration (line 1874)
- Includes session metadata (provider, model, token count) for diagnostics
- Handles errors gracefully with debug logging (appropriate for automatic background operation)
The trigger condition (was active, now idle) ensures sessions are saved after each complete turn.
2 participants
Summary
Add
--continue(-C) flag that resumes the most recent conversation session for the current project.Features
--continuewith--promptto resume and immediately send a new messageUsage
Technical Details
New files:
SessionPersistenceService.ts- Core persistence logic with atomic writesSessionPersistenceService.test.ts- 30 unit tests with comprehensive coverageSession storage location:
Exported types:
PersistedSession- Full session schemaPersistedUIHistoryItem- UI history item interfacePersistedToolCall- Tool call display informationValidation & error handling:
Changed Files
Testing
Closes #147
Summary by CodeRabbit
New Features
Bug Fixes / Reliability
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.