Add Oh My Pi runtime compatibility and model preservation - #93
Merged
Conversation
Added src/adapter.ts that dynamically detects whether the extension is running under Pi (@mariozechner) or Oh My Pi (@oh-my-pi) by probing package availability via dynamic import() — Bun's createRequire.resolve does not walk far enough up the directory tree for globally-installed packages. - Replaced direct value imports from @mariozechner packages in ui-helpers.ts and widget.ts with imports from ./adapter.js - Added EXTENSION_DIR constant (.omp or .pi) replacing hardcoded .pi paths in config.ts, cooldown.ts, model-locks.ts, fetchers/common.ts - Added MODEL_SELECTOR_DEBUG=1 env var for adapter resolution debugging - Updated package.json peerDependencies with both package scopes, all marked optional via peerDependenciesMeta - Type imports remain on @mariozechner (erased at compile time) - String indirection for dynamic imports avoids TS following into unresolvable @oh-my-pi source during type-check
- Added modelRegistry.authStorage credential resolution to zai fetcher, matching the pattern used by all other fetchers (anthropic, copilot, gemini, codex, antigravity). OMP stores credentials in SQLite, not auth.json, so fetchers must query authStorage to find keys. - Removed catch-all default mappings (kiro -> google, zai -> openai). Buckets must be explicitly mapped or they remain unmapped — no silent fallback to an arbitrary model. - Updated fetchZaiUsage signature to accept modelRegistry parameter. - Fixed test call sites for updated signature.
- Added SQLite-based auth loading in common.ts when running under OMP (falls back from legacy JSON file to OMP's SQLite auth store) - Added credential check for Codex provider using auth storage API - Deduplicated Codex file credentials against piAuth/registry accounts to avoid double-counting usage for the same account - Added test for skipping .codex file credential when account already covered by piAuth
- Add OMP_PROVIDER_MAP for normalizing OMP provider IDs to extension names - Add OMP usage report structural types (UsageReport, UsageLimit, etc.) - Add convertOmpUsageReports() to convert OMP UsageReport[] to UsageSnapshot[] - Add convertLimitsToWindows() to map OMP limits to extension RateWindow[] - Add fetchOmpUsages() to delegate to authStorage.fetchUsageReports() - Modify fetchAllUsages() to detect OMP and use built-in usage reports - Fall back to extension fetchers for providers OMP doesn't cover (e.g. kiro) - Import isOmp from adapter and formatReset from fetchers/common
The extension's module context may not have the theme singleton initialized (e.g. when the host runtime mirrors the extension into a temp directory). Instead of importing getSelectListTheme from adapter.ts, build the select list theme inline from the callback's theme parameter, which is always available.
Replace 8 dynamic import("./src/widget.js") calls with static import
of getWidgetState. OMP loads extensions from a temp directory where
relative paths don't resolve at runtime — static imports are handled
by the extension loader, but dynamic import() uses standard ESM
resolution against the temp path and fails with:
Cannot find module './src/widget.js' from
'/private/var/folders/.../omp-legacy-pi-file/...'
…lity # Conflicts: # index.ts # package.json
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an OMP-aware runtime adapter, migrates runtime config/state paths to EXTENSION_DIR, integrates OMP auth/usage sources (SQLite) into fetchers, centralizes selector role-preservation, introduces model-provider affinity, converts several dynamic imports to static, and adds TypeDoc gating, docs, and tests. ChangesOMP Core Adapter and Config Integration
Authentication and Credential Discovery
Usage Fetching and OMP Integration
Selector and UI Integration
Documentation and CI Tooling
Test Coverage for OMP and Features
Sequence Diagram(s)sequenceDiagram
participant Adapter
participant Config
participant Fetchers
participant Selector
participant Widget
Adapter->>Config: determine EXTENSION_DIR and isOmp
Adapter->>Fetchers: provide isOmp/EXTENSION_DIR helpers
Fetchers->>Adapter: loadPiAuth() (auth.json or SQLite fallback)
Fetchers->>Selector: UsageSnapshot[] (convertOmpUsageReports)
Selector->>Adapter: withPreservedOmpDefaultModelRole(setModel)
Selector->>Widget: renderUsageWidget(updated snapshots)
🎯 4 (Complex) | ⏱️ ~60 minutes
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/fetchers/codex.ts (1)
94-107:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
seenAccountsis populated but not enforced before fetch.Right now only the
.codexbranch consultsseenAccounts, and new file credentials never add theiraccountIdback into the set. Same-account credentials from piAuth, registry, or later auth files still survive untilfetchAllCodexUsages(), so the code issues duplicate Codex usage requests before the final dedupe.Also applies to: 126-140, 165-180
🤖 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 `@src/fetchers/codex.ts` around lines 94 - 107, The loop building Codex credentials doesn't prevent duplicate-account fetches because seenAccounts is only checked in the .codex branch and file-derived credentials don't add their accountId to seenAccounts; update the credential-collection logic (around getPiCodexAuths, the loop that constructs CodexCredential and where registry/file branches add credentials) to consult seenAccounts before adding any credential with an accountId and to add that accountId to seenAccounts immediately when a credential is accepted; ensure the same check is applied in all branches that push into credentials (piAuth, registry, file-based parsing) so duplicate account fetches are prevented before fetchAllCodexUsages is called.
🤖 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 `@scripts/typedoc-check.cjs`:
- Around line 2-3: The script converts thresholdText to threshold but doesn't
validate it; update the startup logic that reads const [file, thresholdText] =
process.argv.slice(2) and const threshold = Number(thresholdText) to verify
threshold is a finite number (e.g., using Number.isFinite or !Number.isNaN) and
within an expected range (0–100) before continuing; if validation fails, print a
clear error mentioning thresholdText and exit with a non‑zero code so the
percent < threshold comparison cannot silently pass on invalid input.
- Around line 10-11: The code reads and parses coverage JSON and directly
accesses coverage.percent; add defensive validation in the typedoc-check flow by
wrapping JSON.parse in a try/catch to handle malformed JSON, verify that the
parsed object (coverage) is non-null and has an own property "percent", and
ensure that Number(coverage.percent) yields a finite number; if validation
fails, log a clear error via console.error or processLogger and exit with a
non-zero code, otherwise proceed to use the validated percent value. Use the
symbols coverage, percent and the JSON read/parse block in
scripts/typedoc-check.cjs to locate where to add the try/catch,
typeof/Number.isFinite checks and the failure path.
In `@src/adapter.ts`:
- Around line 41-52: The mocks currently use "as any" (DynamicBorder, Container,
truncateToWidth, SelectList, Spacer, Text) which weakens types; remove the "as
any" casts and give each mock a proper type by importing the original
component/function types from the source (or declaring a small interface that
matches the real API) and typing the mocks accordingly (e.g., type ContainerMock
= { new(): { addChild(): void; render(): any[] } } or use Partial/Mocked types
from your test framework), and type truncateToWidth as (s: string) => string so
the mock signatures match the real implementations.
In `@src/config.ts`:
- Around line 729-732: The regex building for cleaning correctedPath incorrectly
interpolates EXTENSION_DIR into new RegExp(...) without escaping, so characters
like '.' act as wildcards; fix by escaping EXTENSION_DIR before interpolation
(create or reuse an escapeRegExp utility to replace regex metacharacters) and
use the escaped value when constructing the RegExp used in correctedPath =
originalPath.replace(new RegExp(...), ""); ensure the same escaped symbol is
used wherever EXTENSION_DIR is embedded into dynamic regexes.
In `@src/credential-check.ts`:
- Around line 137-139: The branch that accepts provider === "zai" should also
check for a Zai key stored in modelRegistry.authStorage, not only
resolveZaiApiKey(piAuth); update the conditional around provider === "zai" to
return true if resolveZaiApiKey(piAuth) OR a Zai key can be retrieved from
modelRegistry.authStorage (use the same resolution/lookup semantics you use
elsewhere for Zai keys), referencing resolveZaiApiKey, piAuth, modelRegistry,
and authStorage so registry-only keys are accepted.
In `@src/fetchers/common.ts`:
- Around line 65-67: The code uses execAsync with a shell-interpolated sqlite3
command which risks command injection via dbPath; replace the shell call with a
non-shell child process invocation (e.g., use child_process.execFile or spawn)
and pass the program and arguments as an array instead of interpolating dbPath
and SQL into a string: call execFile('sqlite3', ['-json', dbPath, 'SELECT
provider, data FROM auth_credentials']) or spawn('sqlite3', [...]) and
await/promisify the result, keeping the existing variable names (dbPath,
execAsync usage) or replacing execAsync with a promisified execFile wrapper so
the command runs without a shell and arguments are passed safely.
In `@src/ui-helpers.ts`:
- Around line 90-105: The selectListTheme currently uses unsafe any casts for
(theme as any).nav.cursor and selectListTheme as any; replace these by declaring
a narrow structural type for the symbols shape (e.g. { cursor: string }) and a
SelectList theme shape that matches the adapter, then use a runtime guard "nav"
in theme && typeof (theme as any).nav?.cursor === "string" to read
theme.nav.cursor safely (falling back to ">"), and cast selectListTheme to that
explicit theme interface when constructing new SelectList(items, Math.min(...),
selectListTheme as YourSelectListTheme). Update the references to
selectListTheme, theme.nav.cursor, and SelectList in the same area to remove all
any usages while preserving the OMP/Pi fallback behavior.
In `@src/usage-fetchers.ts`:
- Around line 152-155: The code computes usedPercent incorrectly by falling back
to limit.amount.used as a percent; update the fallback to derive a percentage
from used and limit instead. In the block using usedPercent (referencing
usedPercent, limit.amount.usedFraction, limit.amount.used and
limit.amount.limit) replace the ternary fallback so that when
limit.amount.usedFraction is undefined you compute (limit.amount.used /
limit.amount.limit) * 100, guarding against missing or zero limit.amount.limit
(e.g., return 0 or the raw used value if limit is falsy) to avoid
division-by-zero.
- Around line 107-120: The grouping currently uses a single account derived once
per report and pushes all report.limits into that bucket; instead iterate each
limit in report.limits, compute the account for that specific limit (prefer
limit.scope?.accountId, then fall back to the report-level
metadata/email/accountId/account/username), build the key
(`${provider}|${account ?? ""}`) per-limit, get-or-create the group in the
groups Map (same logic as current `let group = groups.get(key); if (!group) {
group = { provider, displayName, account, limits: [] }; groups.set(key, group);
}`) and push only that single limit into its group so snapshots are split by
each limit's account.
- Around line 168-171: Replace direct Date construction with a luxon DateTime
parse: instead of new Date(limit.window.resetsAt) create a DateTime via
DateTime.fromMillis(limit.window.resetsAt) (or fromSeconds(...) if the epoch is
in seconds), check its .isValid (not Number.isNaN), and use that DateTime to
compute window.resetDescription via formatReset (or adapt formatReset to accept
a DateTime). Keep conversion to a native Date only at the UsageSnapshot boundary
(e.g., where UsageSnapshot expects a JS Date) so window.resetsAt stores a
DateTime or is left unset if invalid; update references to window.resetsAt and
formatReset accordingly.
In `@tests/adapter-omp-loader-compat.test.ts`:
- Around line 45-48: The first test builds executableSource by manually
filtering lines starting with "//" which is inconsistent with the second test
that uses the stripComments helper; replace the manual filter with a call to
stripComments(source) (or the existing helper name used in the other test) and
assign its result to executableSource (optionally followed by .trim() or
normalizing newlines if the other test does so) so both tests use the same
comment-stripping logic; update references to the source variable and ensure
stripComments is imported/available in the test file.
In `@tests/omp-usage-conversion.test.ts`:
- Around line 323-344: The test's time-dependent assertion can flake because
Date.now() is called twice; modify the test around convertOmpUsageReports to
freeze time using fake timers and set a fixed Luxon DateTime.now() reference,
compute futureMs relative to that fixed now, call convertOmpUsageReports while
timers are frozen so formatReset/Date.now() use the same base, then restore
timers; update the test to assert win.resetsAt is a Date with getTime() ===
futureMs and resetDescription === "2h" using the fixed Luxon reference and
ensure you reference convertOmpUsageReports and formatReset in the change.
---
Outside diff comments:
In `@src/fetchers/codex.ts`:
- Around line 94-107: The loop building Codex credentials doesn't prevent
duplicate-account fetches because seenAccounts is only checked in the .codex
branch and file-derived credentials don't add their accountId to seenAccounts;
update the credential-collection logic (around getPiCodexAuths, the loop that
constructs CodexCredential and where registry/file branches add credentials) to
consult seenAccounts before adding any credential with an accountId and to add
that accountId to seenAccounts immediately when a credential is accepted; ensure
the same check is applied in all branches that push into credentials (piAuth,
registry, file-based parsing) so duplicate account fetches are prevented before
fetchAllCodexUsages is called.
🪄 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 Plus
Run ID: 5dae0fbf-54cf-4411-b7dd-6a6461fe4cea
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (34)
README.mdconfig/model-selector.example.jsondocs/README.mddocs/omp-compatibility.mdindex.tspackage.jsonscripts/ci.shscripts/typedoc-check.cjssrc/adapter.tssrc/config.tssrc/cooldown.tssrc/credential-check.tssrc/fetchers/codex.tssrc/fetchers/common.tssrc/fetchers/zai.tssrc/model-locks.tssrc/model-provider-affinity.tssrc/selector.tssrc/types.tssrc/ui-helpers.tssrc/usage-fetchers.tssrc/widget.tssrc/wizard.tstests/adapter-omp-loader-compat.test.tstests/config.test.tstests/model-provider-affinity.test.tstests/omp-default-preservation.test.tstests/omp-usage-conversion.test.tstests/selector-branches.test.tstests/selector-locking-disabled.test.tstests/usage-fetchers-branches.test.tstests/usage-fetchers.test.tstests/wizard-config-reload.test.tstsconfig.json
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/config-remove-mapping.test.ts`:
- Around line 188-193: After calling updateProviderSettings with an existing
invalid value, add a concrete assertion that verifies the observed behavior for
that case: e.g., immediately assert whether the call succeeds and the
stored/provider settings for "group-b" now equal the normalized/updated value
(check via the same accessor or return value you use elsewhere in the test), or
assert that the call throws/returns a validation error if that is the intended
behavior; reference updateProviderSettings and the groupId "group-b" when adding
the assertion so the test explicitly documents the expected handling of invalid
existing settings.
🪄 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 Plus
Run ID: 453e8a0b-f9b6-4732-a54c-852a408f18e3
📒 Files selected for processing (1)
tests/config-remove-mapping.test.ts
Contributor
|
Note Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
CTristan
changed the title
Contributor
|
✅ Unit tests committed locally. Commit: |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@tests/omp-usage-fetchers.test.ts`:
- Around line 88-90: The test teardown should always unstub globals to avoid
leak when a test fails; update the afterEach block that currently calls
vi.resetModules() to also call vi.unstubAllGlobals() (or replace/reset order as
appropriate) so that globals are restored unconditionally after each test;
target the afterEach helper in tests/omp-usage-fetchers.test.ts and ensure
vi.unstubAllGlobals() is invoked alongside vi.resetModules().
- Line 1: Remove the unused node:fs import in the test file by deleting the
import statement that declares "fs" (the unused import at the top of
tests/omp-usage-fetchers.test.ts); if file interaction is actually required,
replace it with the specific used API or mock and update tests to reference that
symbol, otherwise simply remove the import to satisfy lint/CI.
- Around line 63-70: In makeOmpReport(), replace the use of Date.now() with
Luxon's DateTime.now().toMillis() to conform to project-wide date/time handling
(ensure DateTime from 'luxon' is imported if not already); update the fetchedAt
assignment in the makeOmpReport function to use DateTime.now().toMillis() so
tests and code use consistent time utilities.
In `@tests/typedoc-check.test.ts`:
- Around line 7-10: The SCRIPT_PATH resolution uses new
URL(import.meta.url).pathname which breaks on Windows; update
tests/typedoc-check.test.ts to use Node's fileURLToPath: import { fileURLToPath
} from "url" and replace path.dirname(new URL(import.meta.url).pathname) with
path.dirname(fileURLToPath(import.meta.url)) when computing SCRIPT_PATH so the
script path is resolved correctly across platforms.
In `@tests/zai-registry-key.test.ts`:
- Line 24: Update the test fixture's nextResetTime to use Luxon instead of
native Date: import DateTime from luxon if not already imported, then replace
the current new Date(Date.now() + 86400000).toISOString() usage in the
nextResetTime field with DateTime.now().plus({ days: 1 }).toISO() (or equivalent
Luxon expression) so the test follows the project's Luxon date/time convention.
🪄 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 Plus
Run ID: 01f81602-af42-4c7a-8bd0-575bbda704c2
📒 Files selected for processing (5)
tests/codex-credential-authstorage.test.tstests/omp-load-pi-auth.test.tstests/omp-usage-fetchers.test.tstests/typedoc-check.test.tstests/zai-registry-key.test.ts
* Add defensive validation for TypeDoc coverage and OMP credentials. * Use execFile for sqlite3 queries to avoid shell injection. * Improve OMP usage account grouping and percentage derivation. * Fix UI component type casting for TUI theme compatibility. * Enhance test coverage for OMP loader and credential detection.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
tests/omp-usage-fetchers.test.ts (2)
81-83:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlways unstub globals in shared teardown for failure-safe isolation.
At Line 81-83, this suite resets modules but does not always restore global stubs. If a test fails before its manual
vi.unstubAllGlobals(), the next tests inherit leaked globals.Proposed fix
afterEach(() => { vi.resetModules(); + vi.unstubAllGlobals(); });🤖 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 `@tests/omp-usage-fetchers.test.ts` around lines 81 - 83, The afterEach teardown currently only calls vi.resetModules() and can leak global stubs if a test fails; update the shared afterEach (the teardown that contains vi.resetModules()) to also call vi.unstubAllGlobals() (or vi.restoreAllMocks() if appropriate) to ensure all global stubs are always restored; locate the afterEach block in tests/omp-usage-fetchers.test.ts and add vi.unstubAllGlobals() alongside vi.resetModules() so teardown is failure-safe.
63-63:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse Luxon instead of
Date.now()in this TypeScript test helper.Line 63 uses
Date.now(), but this project standardizes on Luxon for date/time handling.Proposed fix
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { DateTime } from "luxon"; @@ - fetchedAt: Date.now(), + fetchedAt: DateTime.now().toMillis(),As per coding guidelines,
**/*.{ts,tsx}: "Use luxon for date/time handling throughout the project".🤖 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 `@tests/omp-usage-fetchers.test.ts` at line 63, Replace the use of Date.now() for the fetchedAt field with Luxon: import { DateTime } from 'luxon' at the top and change fetchedAt: Date.now() to fetchedAt: DateTime.now().toMillis() (or DateTime.now().toJSDate() if the field expects a Date object); update any typings accordingly so the test helper uses Luxon DateTime consistently.src/ui-helpers.ts (1)
103-106:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
theme.nav.cursorbefore dereferencing.At Line 104-106,
"nav" in themeis not enough to safely read.nav.cursor; this can throw ifnavexists but is undefined or malformed, which breaks the UI path.Proposed fix
symbols: { - cursor: - "nav" in theme - ? (theme as unknown as { nav: { cursor: string } }).nav.cursor - : ">", + cursor: + typeof (theme as { nav?: { cursor?: unknown } }).nav?.cursor === + "string" + ? (theme as { nav?: { cursor?: string } }).nav!.cursor + : ">", },🤖 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 `@src/ui-helpers.ts` around lines 103 - 106, The code reads theme.nav.cursor after only checking "nav" in theme, which can throw if theme.nav is undefined or not an object; update the ternary to guard safely (e.g., verify theme.nav is truthy and typeof theme.nav === "object" and 'cursor' in theme.nav) before accessing .cursor so you fall back to ">" otherwise; locate the expression that sets cursor (the ternary using "nav" in theme and (theme as unknown as { nav: { cursor: string } }).nav.cursor) and replace it with a safe check that reads cursor only when theme.nav exists and has a cursor property.tests/typedoc-check.test.ts (1)
7-10:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
fileURLToPathfor cross-platform script path resolution.At Line 8, using
.pathnamecan produce invalid Windows paths (e.g.,/C:/...), causing this test to fail on Windows runners.Proposed fix
import * as path from "node:path"; +import { fileURLToPath } from "node:url"; import { afterEach, describe, expect, it } from "vitest"; const SCRIPT_PATH = path.resolve( - path.dirname(new URL(import.meta.url).pathname), + path.dirname(fileURLToPath(import.meta.url)), "../scripts/typedoc-check.cjs", );🤖 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 `@tests/typedoc-check.test.ts` around lines 7 - 10, The SCRIPT_PATH construction uses path.dirname(new URL(import.meta.url).pathname) which yields POSIX-style paths like "/C:/..." on Windows; replace the pathname usage with fileURLToPath(import.meta.url) and compute SCRIPT_PATH via path.resolve(path.dirname(fileURLToPath(import.meta.url)), "../scripts/typedoc-check.cjs") so path resolution is correct cross-platform — update the reference in this test where SCRIPT_PATH is defined and import fileURLToPath from 'url' if not already imported.
🤖 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 `@src/fetchers/codex.ts`:
- Around line 96-116: The current addCredential function discards later
credentials with the same accountId (seenAccounts) which prevents trying
fallback tokens if the first is expired; change addCredential (and the similar
blocks around the other ranges) to only deduplicate by accessToken (seenTokens)
and allow multiple credentials per account by removing the accountId
early-return — i.e., stop using seenAccounts to block pushes, or replace
seenAccounts with a map accountId -> credential[] and push additional
credentials for the same accountId so the fetch logic can try fallbacks; ensure
you still add to seenTokens to avoid duplicate tokens.
In `@tests/omp-load-pi-auth.test.ts`:
- Around line 103-121: The test is mocking child_process.exec but production
uses execFileAsync (promisified execFile), so update the per-test mock to stub
child_process.execFile (or the same mocked function used at top-level) instead
of exec; specifically, change the mockImplementation in
tests/omp-load-pi-auth.test.ts to target child_process.execFile (so
execFileAsync in src/fetchers/common.ts receives the mocked behavior) and ensure
the mocked callback signature matches execFile's (err, stdout, stderr) and
returns the same mock ReturnType as the top-level setup.
---
Duplicate comments:
In `@src/ui-helpers.ts`:
- Around line 103-106: The code reads theme.nav.cursor after only checking "nav"
in theme, which can throw if theme.nav is undefined or not an object; update the
ternary to guard safely (e.g., verify theme.nav is truthy and typeof theme.nav
=== "object" and 'cursor' in theme.nav) before accessing .cursor so you fall
back to ">" otherwise; locate the expression that sets cursor (the ternary using
"nav" in theme and (theme as unknown as { nav: { cursor: string } }).nav.cursor)
and replace it with a safe check that reads cursor only when theme.nav exists
and has a cursor property.
In `@tests/omp-usage-fetchers.test.ts`:
- Around line 81-83: The afterEach teardown currently only calls
vi.resetModules() and can leak global stubs if a test fails; update the shared
afterEach (the teardown that contains vi.resetModules()) to also call
vi.unstubAllGlobals() (or vi.restoreAllMocks() if appropriate) to ensure all
global stubs are always restored; locate the afterEach block in
tests/omp-usage-fetchers.test.ts and add vi.unstubAllGlobals() alongside
vi.resetModules() so teardown is failure-safe.
- Line 63: Replace the use of Date.now() for the fetchedAt field with Luxon:
import { DateTime } from 'luxon' at the top and change fetchedAt: Date.now() to
fetchedAt: DateTime.now().toMillis() (or DateTime.now().toJSDate() if the field
expects a Date object); update any typings accordingly so the test helper uses
Luxon DateTime consistently.
In `@tests/typedoc-check.test.ts`:
- Around line 7-10: The SCRIPT_PATH construction uses path.dirname(new
URL(import.meta.url).pathname) which yields POSIX-style paths like "/C:/..." on
Windows; replace the pathname usage with fileURLToPath(import.meta.url) and
compute SCRIPT_PATH via
path.resolve(path.dirname(fileURLToPath(import.meta.url)),
"../scripts/typedoc-check.cjs") so path resolution is correct cross-platform —
update the reference in this test where SCRIPT_PATH is defined and import
fileURLToPath from 'url' if not already imported.
🪄 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 Plus
Run ID: fe203e18-c506-433a-8a4a-5f6c19d7e805
📒 Files selected for processing (36)
scripts/typedoc-check.cjssrc/adapter.tssrc/candidates.tssrc/config.tssrc/cooldown.tssrc/credential-check.tssrc/fetchers/anthropic.tssrc/fetchers/antigravity.tssrc/fetchers/codex.tssrc/fetchers/common.tssrc/fetchers/copilot.tssrc/fetchers/gemini.tssrc/fetchers/kiro.tssrc/fetchers/minimax.tssrc/fetchers/zai.tssrc/model-locks.tssrc/model-provider-affinity.tssrc/selector.tssrc/types.tssrc/ui-helpers.tssrc/usage-fetchers.tssrc/widget.tssrc/wizard.tstests/adapter-omp-loader-compat.test.tstests/codex-credential-authstorage.test.tstests/config-remove-mapping.test.tstests/kiro.test.tstests/load-pi-auth-sqlite.test.tstests/omp-load-pi-auth.test.tstests/omp-usage-conversion.test.tstests/omp-usage-fetchers.test.tstests/provider-credential-detection.test.tstests/typedoc-check.test.tstests/usage-fetchers-branches.test.tstests/usage-fetchers.test.tstsconfig.json
Keep Codex credential discovery deduplicated by access token while allowing multiple distinct tokens for the same account to be tried. This preserves fallback credentials when an earlier token is expired.\n\nAdded coverage for same-account fallback behavior and updated OMP/Codex tests to match the execFile-backed auth fallback and cross-platform path handling.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/usage-fetchers-branches.test.ts`:
- Around line 765-807: Update the test case names to reflect the actual behavior
asserted: instead of saying credentials are "skipped", rename the test
containing the assertion calling fetchAllCodexUsages (the one currently titled
"should skip .codex file credential when account already covered by piAuth") to
something like "should fetch distinct credentials then deduplicate by account
(retry-then-dedup)" and update the other test around lines 809-868 similarly
(the other test that asserts toHaveBeenCalledTimes(5)) so both names describe
"fetch all distinct tokens then dedupe by account" behavior; keep tests,
assertions, and the call to fetchAllCodexUsages intact.
🪄 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 Plus
Run ID: 50695ac6-325a-494e-b214-72772b4f47a4
📒 Files selected for processing (6)
src/fetchers/codex.tstests/codex-same-account-fallback.test.tstests/omp-load-pi-auth.test.tstests/omp-usage-fetchers.test.tstests/typedoc-check.test.tstests/usage-fetchers-branches.test.ts
Avoid reading theme.nav.cursor unless the custom UI theme exposes an object with a cursor property, falling back to the default cursor otherwise. Rename Codex credential tests to describe fetch-then-deduplicate behavior and cover the missing-nav cursor fallback.
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.
Summary by CodeRabbit
New Features
Documentation
Chores
Tests