Skip to content

refactor: extract account manager cache entry wrapper#291

Closed
ndycode wants to merge 1 commit intorefactor/pr4-runtime-ui-runtime-bridgefrom
refactor/pr4-runtime-account-manager-entry-wrapper
Closed

refactor: extract account manager cache entry wrapper#291
ndycode wants to merge 1 commit intorefactor/pr4-runtime-ui-runtime-bridgefrom
refactor/pr4-runtime-account-manager-entry-wrapper

Conversation

@ndycode
Copy link
Copy Markdown
Owner

@ndycode ndycode commented Mar 22, 2026

Summary

  • extract the account manager cache/reload entry wrappers out of index.ts
  • keep the underlying cache state helper behavior unchanged while shrinking the runtime entrypoint
  • add a focused test for the new runtime entry helper

Validation

  • npm run typecheck
  • npm run lint -- index.ts lib/runtime/account-manager-cache-entry.ts test/account-manager-cache-entry.test.ts
  • npm run test -- test/account-manager-cache-entry.test.ts

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

this pr extracts two inline closures from index.ts (invalidateAccountManagerCache and reloadAccountManagerFromDisk) into a dedicated lib/runtime/account-manager-cache-entry.ts module and adds a focused vitest suite. the refactoring is behaviorally identical to the original: setReloadInFlight is still called synchronously (no await precedes it inside the entry wrapper), so the concurrency-critical accountReloadInFlight assignment timing is unchanged.

  • index.ts — delegates both cache operations to the new entry wrappers via setter callbacks; no logic change
  • lib/runtime/account-manager-cache-entry.ts — new wrapper module; reloadAccountManagerFromDiskEntry is marked async despite having no await, which adds an unnecessary microtask wrap (P2)
  • test/account-manager-cache-entry.test.ts — covers the happy path only; the in-flight dedup path, authFallback forwarding, and rejection propagation are not tested — worth adding to maintain the 80% threshold as the suite grows

Confidence Score: 4/5

  • safe to merge — refactoring is behaviorally equivalent, no concurrency regression introduced
  • the core logic is correct and synchronous assignment timing of accountReloadInFlight is preserved. only two non-blocking p2s remain: the unnecessary async keyword and missing vitest coverage for the dedup/error/authFallback paths.
  • test/account-manager-cache-entry.test.ts — add dedup, authFallback, and rejection tests before the suite grows further

Important Files Changed

Filename Overview
lib/runtime/account-manager-cache-entry.ts new module; logic is correct and equivalent to what it replaced in index.ts — only nit is the unnecessary async on reloadAccountManagerFromDiskEntry (no await present)
test/account-manager-cache-entry.test.ts happy-path only; missing dedup/in-flight, authFallback forwarding, and rejection-propagation tests — all three are straightforward to add and keep the 80% threshold healthy
index.ts clean delegation to the new entry wrappers; synchronous assignment of accountReloadInFlight is preserved (setReloadInFlight runs before any await), so no timing or concurrency regression

Sequence Diagram

sequenceDiagram
    participant C as caller
    participant IDX as index.ts (reloadAccountManagerFromDisk)
    participant E as account-manager-cache-entry.ts (reloadAccountManagerFromDiskEntry)
    participant S as account-manager-cache.ts (reloadAccountManagerFromDiskState)
    participant D as AccountManager.loadFromDisk

    C->>IDX: reloadAccountManagerFromDisk(authFallback?)
    IDX->>E: reloadAccountManagerFromDiskEntry({currentReloadInFlight, ...})
    E->>S: reloadAccountManagerFromDiskState({currentReloadInFlight, loadFromDisk, authFallback, onLoaded, onSettled})
    alt currentReloadInFlight is non-null (dedup)
        S-->>E: existing Promise<TManager>
    else no in-flight reload
        S->>D: loadFromDisk(authFallback?)
        D-->>S: AccountManager
        S->>S: onLoaded(manager) → cachedAccountManager=manager
        S->>S: onSettled() → accountReloadInFlight=null
        S-->>E: new Promise<TManager>
    end
    E->>IDX: setReloadInFlight(inFlight) → accountReloadInFlight=inFlight
    E-->>IDX: Promise<TManager>
    IDX-->>C: Promise<AccountManager>
Loading

Fix All in Codex

Prompt To Fix All With AI
This is a comment left during a code review.
Path: lib/runtime/account-manager-cache-entry.ts
Line: 16

Comment:
**unnecessary `async` — no `await` inside**

`reloadAccountManagerFromDiskEntry` has no `await`, so the `async` keyword only adds an extra microtask wrap and an implicit outer promise shell. the function body already runs synchronously, so `setReloadInFlight` is called at the same tick either way — but it's cleaner to drop `async` and keep the return type explicit:

```suggestion
export function reloadAccountManagerFromDiskEntry<TManager>(params: {
```

drop `async` here and change the return type annotation accordingly. the callers in `index.ts` all `await` the result anyway, so nothing breaks.

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

---

This is a comment left during a code review.
Path: test/account-manager-cache-entry.test.ts
Line: 25-41

Comment:
**missing vitest coverage — three untested paths**

the reload test only exercises the happy path with `currentReloadInFlight: null`. three cases are completely uncovered:

1. **dedup / in-flight reuse** — when `currentReloadInFlight` is a non-null promise, `setReloadInFlight` is still called (with the existing promise), and the entry returns without starting a new load. this is the concurrency-critical path, and it's unverified.

2. **`authFallback` forwarding** — no assertion that `reloadState` receives `authFallback` when provided. a typo or missing key in the spread would be invisible here.

3. **rejection propagation** — no test that a rejected `reloadAccountManagerFromDiskState` rejects the returned promise. given the 80 % coverage threshold in this repo, these gaps are worth closing before the suite grows further.

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

Last reviewed commit: "refactor: extract ac..."

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Mar 22, 2026

Important

Review skipped

Auto reviews are limited based on label configuration.

🚫 Review skipped — only excluded labels are configured. (1)
  • skip-review

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 5db54d5f-1309-49e2-b7d9-66ad6bccfc76

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch refactor/pr4-runtime-account-manager-entry-wrapper
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch refactor/pr4-runtime-account-manager-entry-wrapper

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

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

@chatgpt-codex-connector
Copy link
Copy Markdown

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.

@ndycode
Copy link
Copy Markdown
Owner Author

ndycode commented Mar 23, 2026

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

@ndycode ndycode closed this Mar 23, 2026
@ndycode ndycode deleted the refactor/pr4-runtime-account-manager-entry-wrapper branch March 24, 2026 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant