refactor: extract named backup discovery

[ndycode]

Summary

• extract named-backup discovery out of lib/storage.ts into a dedicated helper module
• keep the existing backup discovery and sorting behavior intact while continuing the storage split in small slices

What Changed

• added lib/storage/named-backups.ts with collectNamedBackups(...) and the NamedBackupSummary type
• updated lib/storage.ts to call the extracted helper and re-export the summary type expected by the current public surface
• preserved the current named-backup behavior through the storage test suite

Validation

• npm run test -- test/storage.test.ts
• npm run lint
• npm run typecheck
• npm run build

Risk and Rollback

• Risk level: low
• Rollback plan: revert 45cda79 to restore the inline named-backup discovery implementation

Additional Notes

• this continues the storage split after the path-state extraction in the same isolated worktree and stacked review flow

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

clean slice extraction — collectNamedBackups and NamedBackupSummary are moved from lib/storage.ts into a dedicated lib/storage/named-backups.ts helper, and AccountIdentityRef/toAccountIdentityRef are promoted to public exports in lib/storage/identity.ts. behavior is fully preserved and the existing storage-last-backup.test.ts integration suite validates the key scenarios end-to-end.

• lib/storage/named-backups.ts — new module with CollectNamedBackupsDeps interface enabling dependency injection for readDir, stat, loadAccountsFromPath, and an optional logDebug; logic is identical to what was in storage.ts
• lib/storage.ts — drops the inline implementation, delegates to collectNamedBackups with real node:fs/promises deps, and re-exports NamedBackupSummary to preserve the public surface
• lib/storage/identity.ts — AccountIdentityRef and toAccountIdentityRef made export so storage.ts can import them without re-declaring
• p2 — readDir type is overly broad: typeof import("node:fs").promises.readdir carries every overload; a narrower single-signature interface matching the one actual call would be easier to stub in unit tests
• p2 — no dedicated unit tests for the DI surface: storage-last-backup.test.ts covers behavior via the real fs through getNamedBackups(), but no test exercises collectNamedBackups with injected mocks directly — worth a follow-up test/storage/named-backups.test.ts to stay above the 80% coverage threshold and to cover the logDebug-optional and unknown-error-rethrow branches

Confidence Score: 4/5

• safe to merge — pure extraction with no logic change; two p2 follow-ups remain but neither blocks the happy path
• behavior is identical to pre-refactor, existing integration tests cover the main scenarios, typecheck/lint/build all pass. score stays at 4 rather than 5 because the DI interface was introduced specifically for testability but no unit tests against it were added, and the broad readDir type will make future mocking friction-y.
• lib/storage/named-backups.ts — readDir type breadth and missing unit tests for the DI surface

Important Files Changed

Filename	Overview
lib/storage/named-backups.ts	new helper module for named-backup discovery with injectable deps — logic faithfully extracted from storage.ts; readDir type is overly broad and no dedicated unit tests were added against the DI surface
lib/storage.ts	cleanly delegates to collectNamedBackups with real fs deps, re-exports NamedBackupSummary, and drops the now-redundant inline implementation; no logic change
lib/storage/identity.ts	exports AccountIdentityRef and toAccountIdentityRef that were previously private in storage.ts — no logic change, purely visibility promotion

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["getNamedBackups()"] -->|injects real fs deps| B["collectNamedBackups(storagePath, deps)"]
    B --> C["deps.readDir(backupRoot, {withFileTypes:true})"]
    C -->|ENOENT| D["return []"]
    C -->|other error| E["throw"]
    C -->|entries| F["filter: isFile + .json"]
    F --> G["deps.stat(candidatePath) — statsBefore"]
    G --> H["deps.loadAccountsFromPath(candidatePath)"]
    H -->|null or 0 accounts| I["skip entry"]
    H -->|valid accounts| J["deps.stat(candidatePath) — statsAfter"]
    J -->|mtime changed| K["deps.logDebug? stale mtime warning"]
    J --> L["push to candidates[]"]
    G -->|throws| M["deps.logDebug? skip + continue"]
    L --> N["sort by mtimeMs desc, then fileName asc"]
    N --> O["return NamedBackupSummary[]"]

Loading

Prompt To Fix All With AI

This is a comment left during a code review.
Path: lib/storage/named-backups.ts
Line: 12

Comment:
**overly broad `readDir` type makes mocking harder**

`typeof import("node:fs").promises.readdir` captures every overload (returns `string[]`, `Buffer[]`, or `Dirent[]` depending on options). this makes the type hard to satisfy in a unit-test stub and means TypeScript has to resolve overloads through the interface property at every call site. a narrower signature matching exactly what `collectNamedBackups` uses would be cleaner:

```suggestion
	readDir: (
		path: string,
		options: { withFileTypes: true; encoding: BufferEncoding },
	) => Promise<{ isFile(): boolean; name: string }[]>;
```

this is a better fit for the DI pattern and makes future unit tests trivial to write.

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: lib/storage/named-backups.ts
Line: 20-22

Comment:
**no unit tests cover the DI surface**

the DI (`CollectNamedBackupsDeps`) was introduced specifically so `collectNamedBackups` can be tested in isolation without touching the real filesystem. `storage-last-backup.test.ts` exercises the behavior via `getNamedBackups()` end-to-end (real fs), which is good, but none of the unit tests inject mock deps directly against `collectNamedBackups`.

missing vitest coverage for injected paths:
- `readDir` returning an unknown `code` error (should re-throw)
- `logDebug` is omitted (optional) — skipped-entry logging is silently dropped
- `stat` throwing mid-loop — currently caught and swallowed via `logDebug`

a companion `test/storage/named-backups.test.ts` using stub deps would close this gap without real fs I/O and keep coverage above the 80% threshold required by the project.

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (2): Last reviewed commit: "Merge main into refactor/pr3-storage-nam..." | Re-trigger Greptile}

[chatgpt-codex-connector]

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

[coderabbitai]

Warning

Rate limit exceeded

@ndycode has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 29 minutes and 58 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 4c2bfa83-213d-4712-a896-f1c2acc6546f

📥 Commits

Reviewing files that changed from the base of the PR and between f3604f5 and e782cdc.

📒 Files selected for processing (3)

• lib/storage.ts
• lib/storage/identity.ts
• lib/storage/named-backups.ts

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch refactor/pr3-storage-named-backups-helper

✨ Simplify code

• Create PR with simplified code
• Commit simplified code in branch refactor/pr3-storage-named-backups-helper

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[ndycode]

Closing because this work is now included in main via #318 and #319.