Conversation
📝 WalkthroughWalkthroughthe pr updates release notes documentation links and refactors the storage layer's account loading logic during export operations. a new helper function Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Notes
Suggested labels
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
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 |
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/README.md (1)
29-48: 🧹 Nitpick | 🔵 Trivialinconsistent labeling for v1.1.10.md within this file.
line 29 labels
releases/v1.1.10.mdas "stable release notes" in the user guides table, but line 48 labels the same file as "current stable release notes" in the reference section. use consistent terminology: either "stable" or "current stable" throughout.📝 proposed fix for consistency
option 1: align table to reference section (recommended):
-| [releases/v1.1.10.md](releases/v1.1.10.md) | Stable release notes | +| [releases/v1.1.10.md](releases/v1.1.10.md) | Current stable release notes |option 2: align reference section to table:
-| [releases/v1.1.10.md](releases/v1.1.10.md) | Current stable release notes | +| [releases/v1.1.10.md](releases/v1.1.10.md) | Stable release notes |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/README.md` around lines 29 - 48, The entry for releases/v1.1.10.md is labeled inconsistently; change the label in the user-guides table (the first table under the guides list that contains "releases/v1.1.10.md") to match the reference section by renaming its label from "Stable release notes" to "Current stable release notes" so both occurrences of releases/v1.1.10.md use the same terminology.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/storage.ts`:
- Around line 1929-1978: loadAccountsForExport currently returns
"missing-storage" before attempting legacy or WAL recovery and never calls
loadAccountsFromJournal; reorder and extend the control flow so the function
first attempts migrateLegacyProjectStorageIfNeeded and loadAccountsFromPath, and
only if loadAccountsFromPath throws ENOENT after trying loadAccountsFromJournal
(call loadAccountsFromJournal like loadAccountsForImport does) should it fall
back to migratedLegacyStorage or finally return the missing-storage marker;
preserve existing resetMarkerPath checks at the same early and catch locations,
run saveAccountsUnlocked on successful migration as before, and ensure you
reference the functions loadAccountsForExport, loadAccountsFromPath,
loadAccountsFromJournal, migrateLegacyProjectStorageIfNeeded, and
saveAccountsUnlocked when making the changes and updating the vitest cases
around storage.test.ts to cover transient EBUSY/EPERM rename failures and failed
canonical-path persists.
In `@test/storage.test.ts`:
- Around line 959-984: Add a new vitest that exercises the concurrency-sensitive
branch: use withAccountStorageTransaction() (or the helper that triggers it) to
start a transaction on the original storage file, then call
setStoragePathDirect(...) to switch to a second storage file that only has the
`.bak` backup, and finally call exportAccounts(exportPath) and assert it rejects
with /No accounts to export/ and that existsSync(exportPath) is false; this
ensures the branch in lib/storage.ts (the path that skips withStorageLock when a
transaction is active and storage path changes) is covered and deterministic
under vitest.
---
Outside diff comments:
In `@docs/README.md`:
- Around line 29-48: The entry for releases/v1.1.10.md is labeled
inconsistently; change the label in the user-guides table (the first table under
the guides list that contains "releases/v1.1.10.md") to match the reference
section by renaming its label from "Stable release notes" to "Current stable
release notes" so both occurrences of releases/v1.1.10.md use the same
terminology.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c6fed824-745c-41aa-ac94-3620e91541d8
📒 Files selected for processing (4)
README.mddocs/README.mdlib/storage.tstest/storage.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
docs/**
⚙️ CodeRabbit configuration file
keep README, SECURITY, and docs consistent with actual CLI flags and workflows. whenever behavior changes, require updated upgrade notes and mention new npm scripts.
Files:
docs/README.md
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/storage.test.ts
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/storage.ts
🔇 Additional comments (2)
docs/README.md (1)
29-31: verify release note files exist and check consistency.the user guides table references v1.1.10.md, v0.1.9.md, and v0.1.8.md. these match README.md:294-296, which is correct. however, verify the files exist on dev branch (same verification script as README.md applies).
see verification script in README.md:294-296 review comment.
README.md (1)
294-296: release note files atdocs/releases/all exist on dev branch—v1.1.10.md, v0.1.9.md, v0.1.8.md, and v0.1.0-beta.0.md are all present. the link updates at readme.md:294-296 won't introduce broken links.
1 participant
Summary
Validation
note: greptile review for oc-chatgpt-multi-auth. cite files like
lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.Greptile Summary
two focused fixes:
exportAccountsnow loads vialoadAccountsForExport— a new private function that intentionally skips backup recovery — so a missing primary storage file correctly rejects instead of silently exporting stale backup data. docs and readme release-note links are updated to point at files that actually exist on the dev branch (v1.1.10,v0.1.9,v0.1.8).lib/storage.ts:loadAccountsForExportmirrorsloadAccountsInternalexcept it returns empty storage (not backup recovery) when the primary path is absent;saveAccountsUnlockedis lock-safe when called from insidewithStorageLock; no reentrancy or deadlock risktest/storage.test.ts: regression test for backup-only rejection on the no-transaction path; cleanup falls under the existingafterEach fs.rm; thetransactionState.active && different-pathbranch has no corresponding coverage (P2 note)README.md/docs/README.md: brokenv1.2.0.mdlinks removed; all three replacement files confirmed present indocs/releases/Confidence Score: 5/5
safe to merge — fix is correct, no regressions, all changed doc links verified present on dev
the logic change is well-scoped: backup recovery is intentionally removed from the export path, saveAccountsUnlocked is lock-safe inside withStorageLock, and the regression test covers the primary failure mode. the only remaining finding is a P2 coverage gap for the active-transaction-different-path branch, which does not affect merge safety.
test/storage.test.ts — active-transaction-different-path branch has no backup-rejection test
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[exportAccounts called] --> B{transactionState.active\nAND same storagePath?} B -- yes --> C[use transaction snapshot] B -- no --> D{transactionState.active\nAND different storagePath?} D -- yes --> E[loadAccountsForExport\nno lock] D -- no --> F[withStorageLock\nloadAccountsForExport] E --> G[loadAccountsForExport] F --> G G --> H{reset marker exists?} H -- yes --> I[return empty storage] H -- no --> J{primary path exists?} J -- no --> K[return empty storage\nmissing-storage] J -- yes --> L[loadAccountsFromPath] L --> M{parse error?} M -- ENOENT --> N[return migratedLegacyStorage] M -- other error --> O[throw] M -- success --> P[maybe save migration\nreturn normalized] C --> Q{accounts.length == 0?} I --> Q K --> Q N --> Q P --> Q Q -- yes --> R[throw: No accounts to export] Q -- no --> S[write export file]Prompt To Fix All With AI
Reviews (2): Last reviewed commit: "fix-export-primary-storage-only" | Re-trigger Greptile
@coderabbitai ignore
note: greptile review for oc-chatgpt-multi-auth. cite files like
lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.Greptile Summary
two focused fixes:
exportAccountsnow loads vialoadAccountsForExport, which intentionally skips backup recovery so a missing primary storage file correctly rejects instead of silently exporting stale backup data. doc and readme release-note links are updated to point at files that actually exist on dev (v1.1.10,v0.1.9,v0.1.8).lib/storage.ts:loadAccountsForExportmirrorsloadAccountsInternalbut removes the backup-candidate recovery loop — backup-only state now correctly yields 0 accounts and throws on export. WAL recovery and legacy migration are preserved intentionally.lib/storage.ts(P2):migrateLegacyProjectStorageIfNeeded(saveAccountsUnlocked)fires at the top ofloadAccountsForExportbefore path checks — on the no-lock branch (active transaction + different path),saveAccountsUnlockedcan write to the alternate path withoutstorageMutex. narrow edge case (only when a legacy migration is pending on the alternate path) but relevant on windows.test/storage.test.ts: four new regression tests — backup-only rejection, WAL recovery, legacy migration export, and the previously-uncovered active-transaction-different-path branch — all verified correct.README.md/docs/README.md: brokenv1.2.0.mdlinks removed; replacement files confirmed present indocs/releases/.Confidence Score: 5/5
safe to merge — fix is correct, all three export branches now covered by regression tests, doc links verified present
only remaining finding is a P2 edge case: saveAccountsUnlocked can fire without the mutex on the no-lock branch if a legacy migration is triggered on an alternate path. the scenario requires an active transaction on a different path AND a pending legacy migration simultaneously — too narrow to block merge. all P0/P1 paths are clean.
lib/storage.ts line 1933-1934 — saveAccountsUnlocked without lock on no-lock export branch
Important Files Changed
loadAccountsForExportthat mirrorsloadAccountsInternalbut omits backup recovery so a backup-only state correctly rejects; wires it intoexportAccountsreplacing the old paths;saveAccountsUnlockedis lock-safe insidewithStorageLockbut can fire without the mutex on the active-transaction-different-path branch if a legacy migration is triggered (P2 edge case)Sequence Diagram
sequenceDiagram participant caller participant exportAccounts participant loadAccountsForExport participant withStorageLock caller->>exportAccounts: exportAccounts(filePath) exportAccounts->>exportAccounts: read transactionSnapshotContext alt same-path active transaction exportAccounts->>exportAccounts: use transactionState.snapshot else different-path active transaction (no lock) exportAccounts->>loadAccountsForExport: loadAccountsForExport() loadAccountsForExport->>loadAccountsForExport: migrate legacy (saveAccountsUnlocked, no mutex) loadAccountsForExport->>loadAccountsForExport: check reset marker / primary path / WAL loadAccountsForExport-->>exportAccounts: storage | null else no active transaction exportAccounts->>withStorageLock: withStorageLock(loadAccountsForExport) withStorageLock->>loadAccountsForExport: loadAccountsForExport() loadAccountsForExport->>loadAccountsForExport: migrate legacy (saveAccountsUnlocked, inside lock) loadAccountsForExport->>loadAccountsForExport: check reset marker / primary path / WAL loadAccountsForExport-->>withStorageLock: storage | null withStorageLock-->>exportAccounts: storage | null end exportAccounts->>exportAccounts: accounts.length == 0? → throw "No accounts to export" exportAccounts->>exportAccounts: write export file (mode 0o600)Prompt To Fix All With AI
Reviews (3): Last reviewed commit: "fix: restore export recovery paths" | Re-trigger Greptile
Context used: