refactor: share forecast and report command helpers#382
Conversation
The forecast and report commands duplicated the same refreshed-account persistence flow and forecast-result serialization logic. That increases maintenance cost and makes future fixes easy to miss in one path. Extract the shared pieces into a single helper module and keep both commands delegating to the same implementation.
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 7 minutes and 14 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the 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 configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
✨ Finishing Touches🧪 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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| applyRefreshedAccountPatch, | ||
| persistRefreshedAccountPatch, | ||
| serializeForecastResults, | ||
| } from "../forecast-report-shared.js"; |
There was a problem hiding this comment.
RefreshedAccountPatch and AccountIdentityMatch
report.ts uses both types as explicit annotations (const refreshPatch: RefreshedAccountPatch at line 277 and const accountMatch: AccountIdentityMatch at line 289) but neither is in the import block. forecast.ts imports them correctly with type AccountIdentityMatch and type RefreshedAccountPatch — this was missed during the extraction. npm run typecheck will fail with "Cannot find name" errors.
| applyRefreshedAccountPatch, | |
| persistRefreshedAccountPatch, | |
| serializeForecastResults, | |
| } from "../forecast-report-shared.js"; | |
| import { | |
| applyRefreshedAccountPatch, | |
| persistRefreshedAccountPatch, | |
| serializeForecastResults, | |
| type AccountIdentityMatch, | |
| type RefreshedAccountPatch, | |
| } from "../forecast-report-shared.js"; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/codex-manager/commands/report.ts
Line: 15-18
Comment:
**Missing type imports for `RefreshedAccountPatch` and `AccountIdentityMatch`**
`report.ts` uses both types as explicit annotations (`const refreshPatch: RefreshedAccountPatch` at line 277 and `const accountMatch: AccountIdentityMatch` at line 289) but neither is in the import block. `forecast.ts` imports them correctly with `type AccountIdentityMatch` and `type RefreshedAccountPatch` — this was missed during the extraction. `npm run typecheck` will fail with "Cannot find name" errors.
```suggestion
import {
applyRefreshedAccountPatch,
persistRefreshedAccountPatch,
serializeForecastResults,
type AccountIdentityMatch,
type RefreshedAccountPatch,
} from "../forecast-report-shared.js";
```
How can I resolve this? If you propose a fix, please make it concise.
|
Superseded by #387, which rebuilds the full open PR stack onto one reviewed integration branch. |
|
Closing in favor of #387. |
1 participant
Problem
The
forecastandreportcommands duplicated the same refreshed-account persistence flow and forecast-result serialization logic.That increases maintenance cost and makes future fixes easy to miss in one path.
Fix
Extract the shared logic into a single helper module and keep both commands delegating to the same implementation.
Changes
lib/codex-manager/forecast-report-shared.tslib/codex-manager/commands/forecast.tslib/codex-manager/commands/report.tsValidation
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 the duplicated token-persistence and forecast-serialization logic from
forecast.tsandreport.tsinto a newlib/codex-manager/forecast-report-shared.tsmodule. the extraction is structurally correct and a good maintenance improvement — butreport.tsis missing thetype AccountIdentityMatchandtype RefreshedAccountPatchimports required for its explicit type annotations, which will breaknpm run typecheck. the new shared module also lacks dedicated unit tests.report.tsmissing type imports —RefreshedAccountPatchandAccountIdentityMatchare annotated at lines 277/289 but not imported;forecast.tsimports them correctly andreport.tswas overlooked — this is a guaranteedtscfailureforecast-report-shared.ts— theEBUSY/EPERMretry contract and patch-application logic are critical windows-filesystem safety invariants perAGENTS.md; they need an isolated test filereport.ts—RETRYABLE_WRITE_CODES/isRetryableWriteErrormirror the shared module's pattern; the two concerns are separate but could driftpersistRefreshedAccountPatch— no lock between theloadAccountsreload andsaveAccountsWithRetry; concurrentforecast --live+report --liveruns can race and drop a token patch, especially on windows whereEBUSYretries extend the write windowConfidence Score: 2/5
not safe to merge — report.ts has missing type imports that will fail tsc
the logic extraction is correct but the omission of RefreshedAccountPatch and AccountIdentityMatch type imports in report.ts is a guaranteed typecheck failure; tests pass because vitest uses esbuild which strips types without checking them. additionally no unit tests exist for the new shared module and the non-atomic write in persistRefreshedAccountPatch is now the shared path for both commands without a concurrency guard.
lib/codex-manager/commands/report.ts (missing type imports — blocks tsc), lib/codex-manager/forecast-report-shared.ts (no unit tests, non-atomic write)
Important Files Changed
Sequence Diagram
sequenceDiagram participant FC as forecast.ts participant RC as report.ts participant SH as forecast-report-shared.ts participant ST as Storage Note over FC,RC: both commands run --live; per-account loop (serial within each command) FC->>ST: loadAccounts() [initial] RC->>ST: loadAccounts() [initial] FC->>FC: hasUsableAccessToken? → false FC->>FC: queuedRefresh(refreshToken) FC->>SH: applyRefreshedAccountPatch(account, patch) [in-memory only] FC->>SH: persistRefreshedAccountPatch(storage, accountMatch, patch, deps) SH->>ST: loadAccounts() [reload latest to avoid clobbering concurrent writes] SH->>SH: structuredClone(latestStorage) SH->>SH: findMatchingAccountIndex × 2 (identity then patch fallback) SH->>SH: applyRefreshedAccountPatch(targetAccount, patch) SH->>ST: saveAccountsWithRetry [EBUSY/EPERM retry up to 3x, exponential backoff] SH-->>FC: return FC->>ST: fetchCodexQuotaSnapshot(probeAccessToken, probeAccountId) RC->>RC: hasUsableAccessToken? → false RC->>RC: queuedRefresh(refreshToken) RC->>SH: applyRefreshedAccountPatch(account, patch) [in-memory only] RC->>SH: persistRefreshedAccountPatch(storage, accountMatch, patch, deps) SH->>ST: loadAccounts() [reload latest] SH->>SH: structuredClone(latestStorage) SH->>SH: findMatchingAccountIndex × 2 SH->>SH: applyRefreshedAccountPatch(targetAccount, patch) SH->>ST: saveAccountsWithRetry [EBUSY/EPERM retry up to 3x] SH-->>RC: return RC->>ST: fetchCodexQuotaSnapshot(probeAccessToken, probeAccountId) FC->>SH: serializeForecastResults(results, liveQuotaByIndex, refreshFailures, formatFn) RC->>SH: serializeForecastResults(results, liveQuotaByIndex, refreshFailures, formatFn)Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "refactor: share forecast and report comm..." | Re-trigger Greptile