refactor(api): move transfer artifacts into oRPC - #669
Conversation
|
Warning Review limit reached
Next review available in: 46 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughメディアソースのエクスポートとインポートをoRPCベースのジョブ処理へ移行しました。成果物取得用のサーバーAPIと、ジョブ画面からのBlobダウンロード処理を追加しました。旧HTTPルートを削除しました。 ChangesソースAPIと成果物取得
Estimated code review effort: 3 (Moderate) | ~25 minutes Mergeability Score: 🟠 High · up to The change moves source exports and job artifact downloads to a new transfer path while removing the legacy routes. As implemented, users may be able to retrieve another user's artifact if they know its job ID, and stalled exports may never finish; the PR is not merge-ready until authorization and bounded cancellation or timeout behavior are addressed. Sequence Diagram(s)sequenceDiagram
participant User
participant V2JobsScreen
participant jobsRouter
participant ArtifactFile
User->>V2JobsScreen: 成果物のダウンロードを選択
V2JobsScreen->>jobsRouter: downloadArtifact(jobId)
jobsRouter->>ArtifactFile: ファイルを検証して読み取る
ArtifactFile-->>jobsRouter: ReadableStream
jobsRouter-->>V2JobsScreen: 成果物ストリーム
V2JobsScreen-->>User: Blobとして保存
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
apps/server/src/tests/unit/infrastructure/api-clients/sources-api-ext.test.ts (1)
28-80: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winポーリングとエラー経路のテストが不足しています。
jobs.getはmockResolvedValueで常にcompletedを返します。そのためwaitForExportのループ、失敗、キャンセル、および成果物欠落の分岐が検証されません。以下のケースを追加してください。
jobs.getがpending→completedを返す遷移(mockResolvedValueOnceを使用)。status: "failed"で例外が送出されること。artifact: nullで"Export completed without an artifact"が送出されること。
waitForExportの待機は500msです。テストではvi.useFakeTimers()を使ってください。🤖 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 `@apps/server/src/tests/unit/infrastructure/api-clients/sources-api-ext.test.ts` around lines 28 - 80, Expand the tests around fetchSourceDump and waitForExport to cover polling and error paths: use vi.useFakeTimers() and mock jobs.get with mockResolvedValueOnce to transition from pending to completed, advance timers so the poll resolves, and assert the download succeeds. Add cases asserting status "failed" rejects and completed with artifact: null rejects with "Export completed without an artifact"; restore real timers after each test.apps/server/src/infrastructure/api/routers/jobs-router.ts (1)
155-170: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
statとcreateReadStreamは同じファイル記述子を共有していません。現在の実装は
fs.statでパスを検査し、その後createReadStreamで同じパスを再度開きます。検査と使用の間にパスが差し替えられる可能性が残ります。JobTransferDirectoryはサーバ管理下なので実害は限定的です。厳密にしたい場合はfs.openでハンドルを取得し、fstatとcreateReadStream({ fd })を同じハンドルで使ってください。また、oRPCが返却ストリームを消費しない経路(出力検証エラーや接続断)では、ファイル記述子が解放されない可能性があります。ストリームのエラーとキャンセルを監視して
destroyすることを検討してください。🤖 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 `@apps/server/src/infrastructure/api/routers/jobs-router.ts` around lines 155 - 170, Update the artifact handling around the jobs-router return path to open the file once with fs.open, validate it with fstat, and pass the same handle to createReadStream instead of re-opening the path. Preserve the existing NOT_FOUND behavior for missing or non-file artifacts. Ensure the returned stream releases the file descriptor on stream errors, cancellation, or other early termination paths.packages/ui/src/screens/v2-jobs-screen.tsx (1)
396-409: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win共有
Buttonコンポーネントの使用を検討してください。同じコンポーネント内の
cancelとretryのアクションはButton(422行、441行)を使います。新しいダウンロード操作は生の<button>にクラスを直接指定しています。スタイルの一貫性が失われます。
DESIGN.mdを確認し、bun run design:lintで結果を検証してください。パスの指示に「Before implementing UI, consult
DESIGN.mdand validate the result withbun run design:lint」があります。🤖 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 `@packages/ui/src/screens/v2-jobs-screen.tsx` around lines 396 - 409, Replace the raw button in the download action with the shared Button component, matching the existing cancel and retry actions in the same component while preserving the current download behavior, disabled state, busy state, label, icon, and styling intent. Consult DESIGN.md before implementing and validate the result with bun run design:lint.Source: Coding guidelines
🤖 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 `@apps/server/src/infrastructure/api-clients/sources-api.ts`:
- Around line 14-25:
共有パッケージにoRPCクライアントを引数として受け取るエクスポート用ポーリング・ダウンロードヘルパーを抽出し、タイムアウトとBlobのMIMEタイプ処理も一元化してください。apps/server/src/infrastructure/api-clients/sources-api.tsのwaitForExport（14-25行）は共有ヘルパーへ移し、apps/tauri/src/infrastructure/api-clients/sources-api.tsのwaitForExport（13-24行）は削除してclientを渡して共有ヘルパーを呼び出すよう更新してください。
- Around line 14-25: Update waitForExport to support a bounded wait, using a
timeout and/or AbortSignal so polling cannot continue indefinitely when a job
remains pending or in_progress. Check the timeout or cancellation between polls
and terminate with an appropriate error while preserving the existing completed,
failed, and cancelled handling; use exponential backoff if consistent with the
surrounding API-client conventions.
Apply the same fix in `@apps/tauri/src/infrastructure/api-clients/sources-api.ts`
around lines 13 - 24: 同じ無期限ポーリングがデスクトップ側にも存在します。
- Around line 120-124: Update the artifact download flow around the `new
Response(stream).blob()` call to preserve the MIME type from
`completedJob.artifact.contentType`. Construct the returned Blob with the
downloaded stream data and that content type so its `type` matches the completed
artifact metadata.
Apply the same fix in `@apps/tauri/src/infrastructure/api-clients/sources-api.ts`
around lines 34 - 38: 同じ MIME タイプ欠落がデスクトップ側にも存在します。
In `@apps/server/src/infrastructure/api/routers/jobs-router.ts`:
- Around line 131-147: downloadArtifact
のハンドラーに既存の認証ミドルウェアを適用し、認証済みユーザーだけが実行できるようにしてください。取得した job
の所有者がリクエストの認証ユーザーと一致することを、成果物を返す前に検証し、不一致時は NOT_FOUND
または既存の認可エラーを返してください。appRouter の直接処理経路でも認証コンテキストが利用されるよう、関連するルート設定を更新してください。
In `@apps/server/src/routes/v2/jobs.tsx`:
- Around line 69-79: Update the download flow around anchor.click() to defer
URL.revokeObjectURL(url) to the next task, ensuring the browser can start the
download before cleanup. Avoid the unnecessary Blob copy by applying
job.artifact.contentType through the Response headers when creating the blob,
and use that blob directly for the object URL.
In `@packages/core/src/domain/contract/jobs.contract.ts`:
- Around line 14-16: Update the downloadArtifact contract in jobs.contract.ts so
its OpenAPI representation defines an application/octet-stream response without
relying on z.instanceof(ReadableStream), while preserving RPC support for the
returned stream. Regenerate apps/server/public/openapi.json by running bun run
gen:spec and verify /jobs/downloadArtifact is included.
In `@packages/ui/src/screens/v2-jobs-screen.tsx`:
- Around line 243-252: Update the download function to add a catch block around
props.onDownload(job), matching the exception-swallowing behavior used by retry
and cancel, while preserving the existing finally block that resets
isDownloading.
---
Nitpick comments:
In `@apps/server/src/infrastructure/api/routers/jobs-router.ts`:
- Around line 155-170: Update the artifact handling around the jobs-router
return path to open the file once with fs.open, validate it with fstat, and pass
the same handle to createReadStream instead of re-opening the path. Preserve the
existing NOT_FOUND behavior for missing or non-file artifacts. Ensure the
returned stream releases the file descriptor on stream errors, cancellation, or
other early termination paths.
In
`@apps/server/src/tests/unit/infrastructure/api-clients/sources-api-ext.test.ts`:
- Around line 28-80: Expand the tests around fetchSourceDump and waitForExport
to cover polling and error paths: use vi.useFakeTimers() and mock jobs.get with
mockResolvedValueOnce to transition from pending to completed, advance timers so
the poll resolves, and assert the download succeeds. Add cases asserting status
"failed" rejects and completed with artifact: null rejects with "Export
completed without an artifact"; restore real timers after each test.
In `@packages/ui/src/screens/v2-jobs-screen.tsx`:
- Around line 396-409: Replace the raw button in the download action with the
shared Button component, matching the existing cancel and retry actions in the
same component while preserving the current download behavior, disabled state,
busy state, label, icon, and styling intent. Consult DESIGN.md before
implementing and validate the result with bun run design:lint.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4eb504d6-9bd8-4200-ac33-b94384791eb1
📒 Files selected for processing (13)
apps/server/src/infrastructure/api-clients/shared/endpoints.tsapps/server/src/infrastructure/api-clients/sources-api.tsapps/server/src/infrastructure/api/routers/jobs-router.tsapps/server/src/routes/api/jobs.$jobId.artifact.tsapps/server/src/routes/api/sources.$mediaSourceId.dump.tsapps/server/src/routes/api/sources.$mediaSourceId.import-lancedb.tsapps/server/src/routes/api/sources.$mediaSourceId.import.tsapps/server/src/routes/v2/jobs.tsxapps/server/src/tests/unit/infrastructure/api-clients/sources-api-ext.test.tsapps/tauri/src/infrastructure/api-clients/sources-api.tspackages/core/src/domain/contract/jobs.contract.tspackages/core/src/domain/jobs/schemas.tspackages/ui/src/screens/v2-jobs-screen.tsx
💤 Files with no reviewable changes (6)
- apps/server/src/routes/api/jobs.$jobId.artifact.ts
- apps/server/src/routes/api/sources.$mediaSourceId.import.ts
- packages/core/src/domain/jobs/schemas.ts
- apps/server/src/infrastructure/api-clients/shared/endpoints.ts
- apps/server/src/routes/api/sources.$mediaSourceId.dump.ts
- apps/server/src/routes/api/sources.$mediaSourceId.import-lancedb.ts
1 participant
概要
旧RESTのsource transfer routeとjob artifact routeを削除し、転送処理をoRPCへ統合します。
変更内容
検証
Summary by CodeRabbit