Skip to content

refactor(cli): migrate series assets onto shared request execution - #518

Open
justinmakaila wants to merge 1 commit into
mainfrom
refactor/cli-assets-series
Open

justinmakaila wants to merge 1 commit into
mainfrom
refactor/cli-assets-series

Conversation

@justinmakaila

@justinmakaila justinmakaila commented Oct 18, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Migrate iracing-data assets series onto the request/executor architecture introduced in #516 and extended in car-assets PR #517. This PR targets the actual car branch, not main.

Stack: #516 → #517 (car) → #518 (this series PR) → #519 (track). This PR targets the car branch, not main.

Scope

  • Examples or documentation
  • Dependencies or maintenance

Execution path

  • Old: Commander → series-specific scope factory → token resolution + generated SeriesApi → locally duplicated safe-request-error handling → shared returned-link fetch → hand-written JSON serialization/output and diagnostics.
  • New: local Zod RawOptionsSchema and OptionsSchema → Commander dispatch → makeSeriesAssetsService owns IRacingAPI.withAccessToken(token).series.getSeriesAssets() → existing shared request/output executor → existing injected returned-link materializer (outside generated-request error mapping) → JSON stdout and success diagnostics.
  • Removes series/scope.ts; adds lazy typed IRacingAPI.series accessor.
  • Car logic, shared executor, and returned-link transport are not duplicated or changed.
  • Tracks still use their existing scope until the next stack layer. No additional CLI flags.

Generated Files

None.

Package and Release Notes

None. This only changes private CLI internals.

Validation

The local repository was unavailable in this execution environment; verification below is from GitHub Actions, not local command execution.

  • pnpm --filter @iracing-data/cli test — cannot execute locally in this environment; repository/dependencies not present.
  • pnpm verify:js — not locally run for same reason.
  • pnpm verify:repo — not locally run for same reason.
  • GitHub Actions CI run #39167998975 — passed repository verification, lint, style, CLI/unit tests, generated-client checks and Rust checks; Docker offline recovery also passed.

Replaced the series scope-based tests with schema/Commander/service and generated API selection, credential and error boundary, once-only materialization, stdout fidelity, no registration-time execution, and OpenAPI parity assertions.

Notes for Reviewers

Review relative to refactor/cli-assets-car (#517), not main. Most important files are commands/assets/series/service.ts, src/api.ts, and the updated series tests. The inherited materializer security behavior comes from #517; there are no new asset downloads or output modes.

Related: #515, #516, #517.

Summary by CodeRabbit

  • New Features
    • Added series asset downloads to the CLI, using the shared request and output process. Series asset output is provided in JSON format.
  • Bug Fixes
    • Improved handling of series request and credential errors, preserving the cause while avoiding exposure of sensitive error details.

@coderabbitai

coderabbitai Bot commented Oct 18, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Important

Review skipped

Review was skipped as selected files did not have any reviewable changes.

⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: 72be52db-21e4-4901-a261-033136fdf358
📥 Commits

Reviewing files that changed from the base of the PR and between 39b414e and 83bd834.

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

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: 34f1b7bc-7bc8-49c7-a137-dfd9c97844be
📥 Commits

Reviewing files that changed from the base of the PR and between 966bee7 and 39b414e.

📒 Files selected for processing: 13
  • apps/iracing-data-cli/src/api.ts
  • apps/iracing-data-cli/src/commands/assets/README.md
  • apps/iracing-data-cli/src/commands/assets/index.ts
  • apps/iracing-data-cli/src/commands/assets/series/command.ts
  • apps/iracing-data-cli/src/commands/assets/series/index.ts
  • apps/iracing-data-cli/src/commands/assets/series/schema.ts
  • apps/iracing-data-cli/src/commands/assets/series/scope.ts
  • apps/iracing-data-cli/src/commands/assets/series/service.ts
  • apps/iracing-data-cli/src/program.ts
  • apps/iracing-data-cli/test/assets-parity.test.cjs
  • apps/iracing-data-cli/test/assets.test.cjs
  • apps/iracing-data-cli/test/series-assets.test.cjs
  • apps/iracing-data-cli/test/track-assets.test.cjs
💤 Files with no reviewable changes: 1
  • apps/iracing-data-cli/src/commands/assets/series/scope.ts

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

📜 Recent review details
🧰 Additional context used
📚 Code guidelines: 1
AGENTS.md — auto-discovered
📓 Path-based instructions: 2
Applicability reference: apps/iracing-data-mcp/AGENTS.md: # iracing-data-mcp guidance Inherits [root guidance](../../AGENTS.md).

📄 CodeRabbit inference engine (AGENTS.md)

Files:

  • apps/iracing-data-cli/test/assets-parity.test.cjs
  • apps/iracing-data-cli/test/assets.test.cjs
  • apps/iracing-data-cli/test/track-assets.test.cjs
  • apps/iracing-data-cli/src/commands/assets/series/schema.ts
  • apps/iracing-data-cli/src/api.ts
  • apps/iracing-data-cli/src/commands/assets/series/service.ts
  • apps/iracing-data-cli/src/commands/assets/README.md
  • apps/iracing-data-cli/src/commands/assets/index.ts
  • apps/iracing-data-cli/src/commands/assets/series/command.ts
  • apps/iracing-data-cli/src/commands/assets/series/index.ts
  • apps/iracing-data-cli/test/series-assets.test.cjs
  • apps/iracing-data-cli/src/program.ts
Applicability reference: apps/iracing-data-mcp/AGENTS.md: # iracing-data-mcp guidance Inherits [root guidance](../../AGENTS.md).

📄 CodeRabbit inference engine (AGENTS.md)

Files:

  • apps/iracing-data-cli/src/commands/assets/index.ts
  • apps/iracing-data-cli/src/commands/assets/series/command.ts
  • apps/iracing-data-cli/src/commands/assets/series/index.ts
  • apps/iracing-data-cli/src/program.ts
🔇 Additional comments: 12
apps/iracing-data-cli/src/api.ts: 1

1-6: LGTM!

Also applies to: 12-12, 43-52

apps/iracing-data-cli/src/commands/assets/series/schema.ts: 1

1-18: LGTM!

apps/iracing-data-cli/src/commands/assets/series/service.ts: 1

1-41: LGTM!

apps/iracing-data-cli/src/commands/assets/series/command.ts: 1

1-1: LGTM!

Also applies to: 3-4, 7-9, 11-11, 18-18

apps/iracing-data-cli/src/commands/assets/series/index.ts: 1

1-6: LGTM!

apps/iracing-data-cli/test/series-assets.test.cjs: 1

4-203: LGTM!

apps/iracing-data-cli/src/commands/assets/index.ts: 1

6-6: LGTM!

Also applies to: 14-25, 30-34, 40-40, 49-49

apps/iracing-data-cli/src/program.ts: 1

5-5: LGTM!

Also applies to: 37-40

apps/iracing-data-cli/test/assets-parity.test.cjs: 1

59-63: LGTM!

apps/iracing-data-cli/test/assets.test.cjs: 1

66-70: LGTM!

apps/iracing-data-cli/test/track-assets.test.cjs: 1

50-54: LGTM!

apps/iracing-data-cli/src/commands/assets/README.md: 1

3-3: LGTM!

📝 Walkthrough

The series assets command now uses a service backed by the shared document request executor. The CLI exposes a cached Series API client, validates series command options, and registers the service in the assets command.

Changes: Series Assets Command

Layer / File(s) Summary
Series API and option contracts
apps/iracing-data-cli/src/api.ts, apps/iracing-data-cli/src/commands/assets/series/schema.ts
IRacingAPI lazily creates and caches a SeriesApi client. The options schema accepts credentials and sets JSON as the output format.
Service execution and command delegation
apps/iracing-data-cli/src/commands/assets/series/service.ts, apps/iracing-data-cli/src/commands/assets/series/command.ts, apps/iracing-data-cli/src/commands/assets/series/index.ts, apps/iracing-data-cli/src/commands/assets/series/scope.ts, apps/iracing-data-cli/test/series-assets.test.cjs
The command parses options and delegates execution to SeriesAssetsService. The service uses the shared request executor to fetch series assets, materialize the returned link, and publish output. Tests cover option validation, successful execution, and error paths. The former command-scope implementation is removed.
Assets registration and integration checks
apps/iracing-data-cli/src/commands/assets/index.ts, apps/iracing-data-cli/src/program.ts, apps/iracing-data-cli/src/commands/assets/README.md, apps/iracing-data-cli/test/assets-parity.test.cjs, apps/iracing-data-cli/test/assets.test.cjs, apps/iracing-data-cli/test/track-assets.test.cjs
The assets command requires a series service, and the CLI program constructs it with shared dependencies. Related tests pass service stubs. The README reflects the series executor migration.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Refactor

Sequence Diagram(s)

sequenceDiagram
  participant Commander
  participant SeriesAssetsCommand
  participant OptionsSchema
  participant SeriesAssetsService
  participant DocumentRequestService
  participant SeriesApi
  participant LinkMaterializer
  Commander->>SeriesAssetsCommand: Invoke with CLI options
  SeriesAssetsCommand->>OptionsSchema: Parse credentials
  OptionsSchema-->>SeriesAssetsCommand: Return JSON options
  SeriesAssetsCommand->>SeriesAssetsService: Execute options
  SeriesAssetsService->>DocumentRequestService: Execute series assets request
  DocumentRequestService->>SeriesApi: Fetch series assets with access token
  SeriesApi-->>DocumentRequestService: Return series assets data link
  DocumentRequestService->>LinkMaterializer: Materialize returned data link
Loading
Merge Risk: ⚪ Minimal · up to 39b41

The series-assets migration appears ready to merge after normal checks; no actionable risk remains identified.

Pre-merge checks: Passed 5
Check name Status Explanation
Title check Passed The title clearly identifies the main change: migrating series assets to shared request execution.
Description check Passed The description covers the change, scope, generated files, release impact, validation status, and reviewer notes. It clearly states that local checks were unavailable and reports the successful CI run…
Docstring Coverage Passed Docstring coverage is 80.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 11 files. (1 skipped: 1 …
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Description check

Explanation

The description covers the change, scope, generated files, release impact, validation status, and reviewer notes. It clearly states that local checks were unavailable and reports the successful CI run.

Full details: Docstring Coverage

Explanation

Docstring coverage is 80.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 11 files. (1 skipped: 1 unsupported.)

✨ Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

A rabbit checked the series list,
Then passed the task along with a twist.
JSON came ready, credentials in tow,
A link was fetched for assets to flow.
The hare hops on; the tests all know.

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

@justinmakaila
justinmakaila force-pushed the refactor/cli-assets-series branch 4 times, most recently from 7b845ec to b6cc1ba Compare October 18, 2026 21:28
@justinmakaila
justinmakaila added this pull request to stack #520 October 18, 2026 22:02
@justinmakaila
justinmakaila force-pushed the refactor/cli-assets-series branch from b6cc1ba to a1376a2 Compare October 19, 2026 03:19
Base automatically changed from refactor/cli-assets-car to main October 19, 2026 17:10
@justinmakaila
justinmakaila force-pushed the refactor/cli-assets-series branch from a1376a2 to 39b414e Compare October 19, 2026 17:10
@justinmakaila
justinmakaila force-pushed the refactor/cli-assets-series branch from 39b414e to 83bd834 Compare October 21, 2026 23:03

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant