fix(core): implement Session save/restore with JSON file persistence#1713
Open
ionfwsrijan wants to merge 2 commits into
Open
fix(core): implement Session save/restore with JSON file persistence#1713ionfwsrijan wants to merge 2 commits into
ionfwsrijan wants to merge 2 commits into
Conversation
save() and restore() were empty stubs — auto-save loop ran but persisted nothing. Now writes _data as JSON to ~/.termui/session.json (default) or a custom storagePath. restore() is called in the constructor to reload on startup. Errors are silently handled to avoid crashes on read-only filesystems or corrupt files.
📝 WalkthroughWalkthrough
ChangesSession filesystem persistence
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/core/src/session/Session.ts (1)
42-68: Enhance persistence tests with behavioral coverage.The current test only verifies
save()andrestore()do not throw, which would pass even if these methods were still no-ops. Add tests that:
- Use a temp
storagePathand verify data persists across Session instances- Confirm corrupt JSON and non-object JSON gracefully fall back without restoring stale data
- Validate that
_storagePathis actually being used🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/session/Session.ts` around lines 42 - 68, The tests for the save() and restore() methods in the Session class only verify that these methods do not throw errors, which means they would pass even if the methods were non-functional. Enhance the test suite by adding tests that use a temporary storagePath to verify session data actually persists to disk and can be successfully restored across different Session instances, add tests that confirm corrupt JSON and non-object JSON are gracefully handled without restoring corrupted data, and validate that the _storagePath property is actually being used in the file operations. These behavioral tests will ensure the persistence logic is working correctly rather than just verifying it doesn't throw.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/session/Session.ts`:
- Around line 43-49: Implement atomic file writing to prevent session data
corruption on failed saves. In the try block where writeFileSync writes directly
to this._storagePath, modify the logic to first write to a temporary file (you
can use a temp filename like this._storagePath with a .tmp extension), and only
after the temporary file write succeeds, replace the original file at
this._storagePath with the temporary file. This ensures that if a write
operation fails due to a kill signal, disk-full error, or partial write, the
existing valid session.json remains untouched instead of being left truncated or
corrupted.
- Around line 61-63: The restore() method has two issues: the JSON.parse() call
lacks an explicit type annotation which violates TypeScript coding guidelines,
and the runtime guard that checks typeof parsed === 'object' && parsed !== null
accepts arrays which can corrupt _data since SessionData expects a record type.
Add an explicit type annotation to the JSON.parse call and update the guard
condition to also reject arrays by adding a check like !Array.isArray(parsed) to
the existing condition, ensuring only plain objects pass through.
---
Nitpick comments:
In `@packages/core/src/session/Session.ts`:
- Around line 42-68: The tests for the save() and restore() methods in the
Session class only verify that these methods do not throw errors, which means
they would pass even if the methods were non-functional. Enhance the test suite
by adding tests that use a temporary storagePath to verify session data actually
persists to disk and can be successfully restored across different Session
instances, add tests that confirm corrupt JSON and non-object JSON are
gracefully handled without restoring corrupted data, and validate that the
_storagePath property is actually being used in the file operations. These
behavioral tests will ensure the persistence logic is working correctly rather
than just verifying it doesn't throw.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1d9a2395-acf0-4554-808f-e1c49cd3e9b8
📒 Files selected for processing (1)
packages/core/src/session/Session.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/session/Session.test.ts`:
- Around line 35-45: The testPath variable on line 35 uses a hardcoded path
string '/tmp/termui-test-session.json' which can cause test collisions in
parallel test runs and platform compatibility issues. Move the testPath
initialization inside the beforeEach block and generate a unique temporary
directory for each test using mkdtempSync(tmpdir()) from the os module, then
construct the session file path within that unique directory. This ensures each
test gets its own isolated temp file and cleanup happens correctly with
removeSync or similar for the entire directory.
- Line 70: The code on line 70 uses an inline require('node:fs').writeFileSync()
call instead of importing writeFileSync at the top of the file with other
node:fs imports. Add writeFileSync to the existing node:fs import statement at
line 3 (alongside any other fs imports already present), then replace the inline
require('node:fs').writeFileSync() call on line 70 with just writeFileSync() to
maintain consistency with the file's ESM import pattern.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 73ecbdb3-f392-4833-a218-36a90421ae97
📒 Files selected for processing (1)
packages/core/src/session/Session.test.ts
Contributor
Author
|
@Karanjot786 Please review this |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Sessionis an in-memory-only store with no persistence. When the terminal app restarts, all session state is lost. Widgets like form inputs, wizard steps, and multi-page flows cannot survive a restart.Changes
Session.save(): Serializesthis._datato JSON and writes it to~/.termui/session.json(configurable viastoragePathoption)Session.restore(): Reads the file from disk and merges data into_datarestore()on creationnode:fs,node:path,node:osimports as required by project conventionsTesting
Closes #1710
Summary by CodeRabbit
Release Notes
New Features
storagePathoption to customize where session data is stored.Bug Fixes
Tests