Conversation
fix(ccip): address review feedback
fix(server): sync PostgreSQL and LanceDB discrepancies on startup
📝 WalkthroughWalkthroughCCIP ベクトル抽出、類似検索、vector 検索モード、ジョブ処理、LanceDB 永続化、削除連動、関連 UI と設定が追加された。OpenAPI、API クライアント、検索状態永続化、エージェントスキル文書も更新された。 ChangesCCIP ベクトル検索機能
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces CCIP (Character Contrastive Image Pretraining) vector extraction and similarity search capabilities to the application. It adds a new CcipVectorService, a LanceDB-based vector store, background jobs for extraction, and updates the UI to support triggering extraction and performing similarity searches. The review feedback highlights two important issues: first, a reactive infinite loop in media-sidebar.tsx caused by updating a SolidJS signal (ccipStatusRequestId) inside a createEffect that depends on it; second, a potential performance bottleneck in rust-ai-client.ts when executing CPU-heavy CCIP distance calculations concurrently over a large number of candidates using Promise.all, which should be throttled using asyncPool.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/ui/src/hooks/use-manager-page.ts (1)
413-429: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winタブ切り替えで前モードのスキャン結果を再利用してしまいます。
scannedMediaとselectedMediaが tagging / vectors で共通のままなので、Tagging でスキャンした直後に Vector Extraction へ切り替えると、その一覧を再スキャンなしでそのまま送信できます。packages/ui/src/screens/manager-screen.tsxLine 223-236 でも共有 state をそのまま開始処理に渡しているので、別モード向けの対象を誤って処理できます。activeTab()がtaggingとvectorsの間で変わるときに batch state をクリアするか、モードごとに state を分離してください。🤖 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/hooks/use-manager-page.ts` around lines 413 - 429, The shared scan/selection state is being reused across tagging and vectors, so switching tabs can carry the previous mode’s results into the next action. Update the manager page logic in use-manager-page (especially handleScan and the activeTab()-driven state) to clear scannedMedia and selectedMedia when switching between tagging and vectors, or split them into per-mode state. Also ensure manager-screen does not reuse the prior mode’s batch targets when starting processing, so each tab only submits items scanned for that mode.
🟡 Minor comments (5)
apps/server/src/tests/unit/application/services/maintenance-service.test.ts-401-409 (1)
401-409: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
createIfUniqueの検証が弱く、誤った同期ジョブの追加発行を見逃します。今の
toHaveBeenCalledWith(expect.objectContaining(...))だけだと、期待ジョブに加えて誤ったsync_lancedb_full/sync_lancedb_deltaが同時に積まれても通ります。ここは呼び出し回数と最終引数まで固定した方が、この分岐テストの価値が出ます。差分例
await service.performStartupChecks(); expect(mockQueueSourceLanceDBDelta).not.toHaveBeenCalled(); - expect(mockJobRepo.createIfUnique).toHaveBeenCalledWith( + expect(mockJobRepo.createIfUnique).toHaveBeenCalledTimes(1); + expect(mockJobRepo.createIfUnique).toHaveBeenLastCalledWith( expect.objectContaining({ type: "sync_lancedb_delta", mediaSourceId: "source-1", }), );- expect(mockJobRepo.createIfUnique).toHaveBeenCalledWith( + expect(mockJobRepo.createIfUnique).toHaveBeenCalledTimes(1); + expect(mockJobRepo.createIfUnique).toHaveBeenLastCalledWith( expect.objectContaining({ type: "sync_lancedb_delta", mediaSourceId: "source-1", }), );await service.performStartupChecks(); expect(mockQueueSourceLanceDBDelta).not.toHaveBeenCalled(); - expect(mockJobRepo.createIfUnique).toHaveBeenCalledWith( + expect(mockJobRepo.createIfUnique).toHaveBeenCalledTimes(1); + expect(mockJobRepo.createIfUnique).toHaveBeenLastCalledWith( expect.objectContaining({ type: "sync_lancedb_full", mediaSourceId: "source-1", }), );Also applies to: 452-457, 479-484
🤖 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/application/services/maintenance-service.test.ts` around lines 401 - 409, The `performStartupChecks` tests are too loose because `mockJobRepo.createIfUnique` is only checked with `toHaveBeenCalledWith(expect.objectContaining(...))`, so extra incorrect sync jobs can slip through. Tighten the assertions in these cases by verifying the exact number of `createIfUnique` calls and the full job payload for the expected `sync_lancedb_delta` path, using the existing `mockJobRepo.createIfUnique` and `service.performStartupChecks` symbols to locate the affected tests.apps/server/src/tests/unit/infrastructure/jobs/ccip-jobs.test.ts-88-130 (1)
88-130: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winジョブ契約の肝心な部分が未検証です。
ここだと
extractの引数取り回しと、親側job-completed/job-progressの発火条件が固定されていません。processCcipExtractionJobの責務そのものなので、引数と親イベントの有無まで明示的に縛った方が回帰を拾えます。差分例
- expect(extract).toHaveBeenCalled(); + expect(extract).toHaveBeenCalledWith( + "00000000-0000-4000-8000-000000000001", + "00000000-0000-4000-8000-000000000030", + false, + ); expect(jobRepository.incrementProgress).toHaveBeenCalledWith( "00000000-0000-4000-8000-000000000010", "00000000-0000-4000-8000-000000000020", @@ expect(jobRepository.markAsCompleted).toHaveBeenCalledWith( "00000000-0000-4000-8000-000000000010", { success: true }, ); + expect(publishJob).toHaveBeenCalledWith("job-completed", { + jobId: "00000000-0000-4000-8000-000000000010", + message: "CCIP vector extraction completed", + }); @@ await processCcipExtractionJob({ @@ }); + expect(extract).toHaveBeenCalledWith( + "00000000-0000-4000-8000-000000000001", + "00000000-0000-4000-8000-000000000031", + false, + ); expect(jobRepository.findById).not.toHaveBeenCalled(); expect(jobRepository.markAsCompleted).not.toHaveBeenCalled(); + expect(publishJob).not.toHaveBeenCalledWith( + "job-progress", + expect.objectContaining({ + jobId: "00000000-0000-4000-8000-000000000011", + }), + );🤖 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/jobs/ccip-jobs.test.ts` around lines 88 - 130, The current tests around processCcipExtractionJob only cover a subset of the job contract, so they should explicitly verify the extract call arguments and the parent event behavior. Update the assertions in ccip-jobs.test.ts to check the exact payload passed to extract, and in the processCcipExtractionJob flow confirm when job-completed and job-progress are emitted or skipped based on child progress counting. Use the existing symbols processCcipExtractionJob, extract, publishJob, and jobRepository.incrementProgress to keep the coverage tied to the real contract.packages/ui/src/hooks/use-manager-page.ts-436-459 (1)
436-459: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win開始 API の非成功レスポンスが握りつぶされています。
handleStartBatchCcipExtractionにはresult.success === falseまたはjobId欠落時の分岐がないので、setTaggingStatus("Starting...")のまま止まり、失敗理由も表示されません。直下のhandleStartBatchTaggingと同じ else 分岐が必要です。修正案
if (result.success && result.jobId) { toast.success(result.message); setTaggingStatus("Batch CCIP extraction in progress..."); setActiveJobId(result.jobId); setScannedMedia([]); setSelectedMedia(new Set<string>()); + } else { + const message = + result.message || "Failed to start batch CCIP extraction."; + toast.error(message); + setTaggingStatus(message); }🤖 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/hooks/use-manager-page.ts` around lines 436 - 459, handleStartBatchCcipExtraction is missing the non-success response handling that exists in handleStartBatchTagging, so failures leave the UI stuck at “Starting...”. Update the handleStartBatchCcipExtraction flow to explicitly handle the case where result.success is false or result.jobId is missing, and surface result.message to both toast and setTaggingStatus. Use the existing handleStartBatchCcipExtraction and handleStartBatchTagging functions as the reference points and keep the success path unchanged.packages/ui/src/hooks/use-current-search-persistence.ts-61-75 (1)
61-75: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winベクトル検索のソース絞り込みも保存・復元してください。
useSearchPageは vector モードでもselectedSourceをmediaSourceIdとして使っていますが、ここではsimilarityAnchorMediaId/similarityTopKしか永続化していません。再訪時に検索対象が「すべてのソース」へ戻り、同じ検索結果を再現できなくなります。💡 修正案
if (current.mode === "vector") { resetSearchState(); setSearchState({ mode: "vector", + selectedSource: + typeof current.selectedSource === "string" + ? current.selectedSource + : "", similarityAnchorMediaId: typeof current.similarityAnchorMediaId === "string" ? current.similarityAnchorMediaId : null, similarityTopK: @@ const presetData = { value: condition, sort: searchState.sortBy, order: searchState.sortOrder, mode: searchState.mode, + selectedSource: searchState.selectedSource, similarityAnchorMediaId: searchState.similarityAnchorMediaId, similarityTopK: searchState.similarityTopK, };Also applies to: 166-167
🤖 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/hooks/use-current-search-persistence.ts` around lines 61 - 75, Persist and restore the vector search source filter in useCurrentSearchPersistence’s vector-mode branch, since useSearchPage uses selectedSource as mediaSourceId even in vector mode and it is currently lost on reload. Update the vector handling around resetSearchState/setSearchState to include the source selection alongside similarityAnchorMediaId and similarityTopK, and make sure the restore logic reads that value back so revisiting the page preserves the same source-scoped search.apps/server/src/infrastructure/ai/rust-ai-client.ts-260-287 (1)
260-287: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win返却距離数を
candidatesと突き合わせてください。Line 268-287 は remote/native/fallback のどの経路でも返ってきた距離配列の件数を検証していないので、1 件でも欠けたり余分に返った場合に候補とスコアの対応が静かに壊れます。類似検索の順位付けを誤るので、返却前に
distances.length === candidates.lengthを必ず確認した方がいいです。修正案
async calculateCcipDistances( feature: number[], candidates: number[][], ): Promise<number[]> { + const ensureDistances = (distances: number[]) => { + if (distances.length !== candidates.length) { + throw new Error( + `Invalid CCIP distances response: expected ${candidates.length}, got ${distances.length}`, + ); + } + return distances; + }; + if (this.baseUrl) { if (!this.client) { throw new Error("Client is not initialized (baseUrl is empty)"); } const result = await this.client.ai.ccipDistances({ feature, candidates, }); - return result.distances; + return ensureDistances(result.distances); } if (!this.baseUrl) { const nativeModule: unknown = await import("dghs-imgutils-rs"); if (hasCcipDistances(nativeModule)) { - return await nativeModule.ccipDistances(feature, candidates); + return ensureDistances( + await nativeModule.ccipDistances(feature, candidates), + ); } } - return await Promise.all( - candidates.map(async (candidate) => { - const result = await this.calculateCcipDifference(feature, candidate); - return result.difference; - }), + return ensureDistances( + await Promise.all( + candidates.map(async (candidate) => { + const result = await this.calculateCcipDifference(feature, candidate); + return result.difference; + }), + ), ); }🤖 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/ai/rust-ai-client.ts` around lines 260 - 287, `calculateCcipDistances` は remote/native/fallback のどの経路でも返却される距離配列の件数を検証しておらず、`candidates` と対応が崩れる可能性があります。`this.client.ai.ccipDistances`、`nativeModule.ccipDistances`、および `Promise.all` のいずれの結果でも、返却前に距離配列の長さが `candidates.length` と一致することを確認し、`calculateCcipDistances` 内で不一致時は例外にしてください。
🧹 Nitpick comments (1)
apps/server/src/infrastructure/jobs/tagging-jobs.ts (1)
58-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win親ジョブ payload スキーマを共通化してください。
ccip-jobs.ts側にも同じ親 payload の Zod schema があり、こちらはtotal/processedがz.number()のままで検証条件がズレています。共通 schema に寄せると、進捗イベントと完了判定の契約 drift を防げます。🤖 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/jobs/tagging-jobs.ts` around lines 58 - 61, 親ジョブの payload スキーマ定義が `tagging-jobs.ts` と `ccip-jobs.ts` で重複しており、`total` / `processed` の検証条件もずれているため、共通の Zod schema にまとめて両方から参照するようにしてください。`parentPayloadSchema` を定義している箇所を基準に、進捗イベントと完了判定で同じ契約を使うよう整理し、`processedJobIds` を含む親 payload の検証ルールが一箇所で保守される形に修正してください。
🤖 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/public/openapi.json`:
- Around line 616-641: The OpenAPI entry for the new POST media.searchSimilar
endpoint is missing its requestBody and is emitting a useless generic 200
schema, so regenerate or fix the OpenAPI conversion for the Media router so the
Zod input/output from searchSimilar is reflected. Update the media.searchSimilar
operation in openapi.json to include the actual request body schema and a
concrete response schema, and make sure the same issue is addressed for the
other affected Media routes referenced in the diff so client generation
preserves the CCIP contract.
In `@apps/server/src/application/services/ccip-vector-service.ts`:
- Around line 1-16: The application-level getCcipVectorService() is directly
importing and instantiating LanceDbCcipVectorStore, which reverses the clean
architecture dependency direction. Move the composition of the vector store into
bootstrap/registry wiring and update CcipVectorService to accept an
ICcipVectorStore dependency instead of creating the infrastructure
implementation itself. Keep this file limited to assembling the service from
injected dependencies via services and remove the direct
~/infrastructure/ai/lancedb-ccip-vector-store reference.
In `@apps/server/src/application/services/directory-sync-service.ts`:
- Around line 63-71: The directory sync delete flow in directory-sync-service
currently swallows CCIP vector deletion failures after MediaRepository.delete,
which can leave orphaned LanceDB vectors behind. Update the delete path around
ccipVectorService.delete so failures are not treated as a harmless warn-only
case: either enqueue a retry/recovery job or propagate/fail the operation so it
is clearly separated from a completed deletion, and keep the logging in the same
block for directory sync context.
In `@apps/server/src/application/services/job-dispatch-service.ts`:
- Around line 31-35: `job-dispatch-service` in the application layer is directly
importing the infrastructure job implementation via the dynamic import for
`processCcipExtractionJob`, which violates the dependency direction. Move the
CCIP dispatch logic out of `JobDispatchService` into an infrastructure-side
dispatcher, or change `JobDispatchService` to depend only on an
application-level port/registry of job processors. Keep `job.type ===
"extract_ccip_vector"` handled through an abstracted processor contract instead
of importing `~/infrastructure/jobs/ccip-jobs` from the application service.
In `@apps/server/src/application/services/media-source-service.ts`:
- Around line 61-63: The delete flow in media-source-service’s source removal
path can leave orphan vectors because `sourceRepo.delete` and
`ccipVectorService.deleteBySource` are executed as separate non-atomic steps.
Update the delete logic around the source-removal method in `MediaSourceService`
to use a retryable/compensating flow (for example, outbox or background job) so
both the DB source delete and vector cleanup can recover from partial failure
instead of relying on simple ordering.
In `@apps/server/src/components/media/media-sidebar.tsx`:
- Around line 131-146: The CCIP extraction flow in handleCcipExtraction can
apply a stale result after the user switches media, causing the old jobId to
overwrite the current sidebar state. Capture the current media identifier/token
at the start of handleCcipExtraction, then after startCcipExtraction resolves
verify it still matches props.media (same approach as refreshCcipStatus) before
calling setCcipStatus and setActiveCcipJobId. If the media has changed, ignore
the returned result and avoid showing the success toast for the stale job.
In `@apps/server/src/infrastructure/ai/lancedb-ccip-vector-store.ts`:
- Around line 59-73: The `connection()` and `table()` methods in
`LancedbCcipVectorStore` currently cache rejected Promises, so a failed
`connect()` or `openOrCreateTable()` call will permanently poison the instance.
Update the lazy-initialization flow so `connectionPromise` and `tablePromise`
are cleared/reset when the awaited operation fails, and only keep the promise
cached after a successful resolution. Use the existing `connection()`,
`table()`, and `openOrCreateTable()` methods as the fix points.
- Around line 121-132: The upsert flow in CcipLanceDBVectorStore currently
performs a delete followed by an add inside serializeWrite, which leaves a
window where get/search/list can observe missing data and can drop the old
record if add fails. Refactor the CcipLanceDBVectorStore.upsert method to use an
atomic replacement strategy in the LanceDB table operation instead of separate
delete and add steps, keeping the record visible consistently throughout the
update.
In `@apps/server/src/infrastructure/api/routers/ai-router.ts`:
- Around line 490-519: `startBatchCcipExtraction` is creating a parent job with
`input.mediaSourceId` while the `db.query.medias.findMany` filter only checks
`input.mediaIds` and `medias.mediaType`, so mixed-source IDs can slip through.
Update the selection logic in this handler to also filter by `mediaSourceId`
when provided (and keep using `mediaIds`/`image` checks), then verify the
returned rows all belong to the same source before creating the job. If the
filtered count does not match the requested IDs or the source is inconsistent,
throw `BAD_REQUEST` from `startBatchCcipExtraction` before calling
`jobRepository.create`.
In `@apps/server/src/infrastructure/api/routers/media-router.ts`:
- Line 203: After deleteMedia, moveMedia, and bulkDeleteMedia finish, the direct
ccipVectorService.delete cleanup should not be on the critical API path because
failures there turn a completed media operation into a partial failure. Move the
vector deletion into the same application/service layer as MediaService or wrap
it in a compensating async cleanup with retry handling so the API response
reflects the primary operation outcome. Update the cleanup call sites in
media-router to avoid throwing after the main media mutation succeeds.
In `@apps/server/src/infrastructure/jobs/file-watcher-service.ts`:
- Around line 87-95: The file deletion flow in file-watcher-service leaves
orphaned vector data because MediaRepository.delete succeeds and
ccipVectorService.delete is only warned on failure. Update the remove path in
the file-watcher handler to use the same retry or compensating cleanup strategy
as directory sync, so vector deletion failures are not silently skipped. Make
the fix around the media removal logic that calls MediaRepository.delete and
ccipVectorService.delete, and ensure failures are handled in a way that
preserves storage consistency.
In `@packages/application/src/services/ccip-vector-service.ts`:
- Around line 159-164: `isCurrent()` is treating moved media as still current
because it only checks model, embedding version, and modified time. Update
`CcipVectorService.isCurrent()` to also compare `mediaSourceId` against the
record’s source so `extract()`, `getStatus()`, and source-filtered searches
don’t reuse vectors from a different source. Use the existing `CcipVectorRecord`
and `Media` fields in this method to make source identity part of the freshness
check.
In `@packages/application/src/services/media-processing-service.ts`:
- Around line 272-280: The `processMedia` flow in `MediaProcessingService` is
enqueueing duplicate `extract_ccip_vector` jobs for the same `mediaId`, so
switch this enqueue path to use `createIfUnique` instead of `create` and keep
the uniqueness keyed on the `payload.mediaId` used by `JobRepository`. Make sure
the `extract_ccip_vector` job creation path only inserts once even when
`processMedia` runs repeatedly or concurrently, and verify the CCIP job flow in
`ccip-jobs` still respects the existing duplicate suppression behavior.
In `@packages/core/src/domain/contract/ai.contract.ts`:
- Around line 53-55: `scanBatchCcipTargets` is exposing `mediaSchema` directly
from the public contract, which locks internal fields like `filePath` into the
API surface. Update `ai.contract.ts` to return a dedicated public Safe DTO
schema instead of `mediaSchema`, and make sure the corresponding
`scanBatchCcipTargets` contract type uses that new safe schema so the server
router can map from internal media data to the sanitized response.
In `@packages/core/src/domain/search/logic.ts`:
- Around line 15-16: The mode transition logic in search/logic.ts is dropping
the prior non-vector search state, so returning from vector mode cannot restore
the original filters or advanced condition. Update the mode-switching flow
around the vector branch and the state reconstruction logic in the search mode
handler so the previous mode and its fields are preserved when entering vector
mode, then restored when switching back from vector to simple/pro; use the
existing mode conversion logic in the search state functions to keep filter
values and advancedCondition intact.
In `@packages/core/src/domain/tagging/schemas.ts`:
- Around line 42-45: `ccipDistancesRequestSchema` currently accepts any-length
numeric arrays for `feature` and `candidates`, so tighten the contract to
enforce the fixed 768-dimensional vector shape at the boundary. Update the
schema in `schemas.ts` to validate that `feature` is exactly 768 numbers and
each inner array in `candidates` is also exactly 768 numbers, using the existing
`ccipDistancesRequestSchema` symbol so invalid lengths are rejected before
reaching the AI client or LanceDB.
In `@packages/ui/src/media-sidebar.tsx`:
- Around line 136-149: The CCIP extraction flow in extractCcipVector can apply a
late response to the wrong media after props.media changes. Capture the current
media key or a request token before calling props.startCcipExtraction, then
after the await verify the media is still the same before calling setCcipStatus
and setActiveCcipJobId. If the media changed, discard the result so
useCcipJobEvents keeps subscribing to the correct job.
---
Outside diff comments:
In `@packages/ui/src/hooks/use-manager-page.ts`:
- Around line 413-429: The shared scan/selection state is being reused across
tagging and vectors, so switching tabs can carry the previous mode’s results
into the next action. Update the manager page logic in use-manager-page
(especially handleScan and the activeTab()-driven state) to clear scannedMedia
and selectedMedia when switching between tagging and vectors, or split them into
per-mode state. Also ensure manager-screen does not reuse the prior mode’s batch
targets when starting processing, so each tab only submits items scanned for
that mode.
---
Minor comments:
In `@apps/server/src/infrastructure/ai/rust-ai-client.ts`:
- Around line 260-287: `calculateCcipDistances` は remote/native/fallback
のどの経路でも返却される距離配列の件数を検証しておらず、`candidates`
と対応が崩れる可能性があります。`this.client.ai.ccipDistances`、`nativeModule.ccipDistances`、および
`Promise.all` のいずれの結果でも、返却前に距離配列の長さが `candidates.length`
と一致することを確認し、`calculateCcipDistances` 内で不一致時は例外にしてください。
In `@apps/server/src/tests/unit/application/services/maintenance-service.test.ts`:
- Around line 401-409: The `performStartupChecks` tests are too loose because
`mockJobRepo.createIfUnique` is only checked with
`toHaveBeenCalledWith(expect.objectContaining(...))`, so extra incorrect sync
jobs can slip through. Tighten the assertions in these cases by verifying the
exact number of `createIfUnique` calls and the full job payload for the expected
`sync_lancedb_delta` path, using the existing `mockJobRepo.createIfUnique` and
`service.performStartupChecks` symbols to locate the affected tests.
In `@apps/server/src/tests/unit/infrastructure/jobs/ccip-jobs.test.ts`:
- Around line 88-130: The current tests around processCcipExtractionJob only
cover a subset of the job contract, so they should explicitly verify the extract
call arguments and the parent event behavior. Update the assertions in
ccip-jobs.test.ts to check the exact payload passed to extract, and in the
processCcipExtractionJob flow confirm when job-completed and job-progress are
emitted or skipped based on child progress counting. Use the existing symbols
processCcipExtractionJob, extract, publishJob, and
jobRepository.incrementProgress to keep the coverage tied to the real contract.
In `@packages/ui/src/hooks/use-current-search-persistence.ts`:
- Around line 61-75: Persist and restore the vector search source filter in
useCurrentSearchPersistence’s vector-mode branch, since useSearchPage uses
selectedSource as mediaSourceId even in vector mode and it is currently lost on
reload. Update the vector handling around resetSearchState/setSearchState to
include the source selection alongside similarityAnchorMediaId and
similarityTopK, and make sure the restore logic reads that value back so
revisiting the page preserves the same source-scoped search.
In `@packages/ui/src/hooks/use-manager-page.ts`:
- Around line 436-459: handleStartBatchCcipExtraction is missing the non-success
response handling that exists in handleStartBatchTagging, so failures leave the
UI stuck at “Starting...”. Update the handleStartBatchCcipExtraction flow to
explicitly handle the case where result.success is false or result.jobId is
missing, and surface result.message to both toast and setTaggingStatus. Use the
existing handleStartBatchCcipExtraction and handleStartBatchTagging functions as
the reference points and keep the success path unchanged.
---
Nitpick comments:
In `@apps/server/src/infrastructure/jobs/tagging-jobs.ts`:
- Around line 58-61: 親ジョブの payload スキーマ定義が `tagging-jobs.ts` と `ccip-jobs.ts`
で重複しており、`total` / `processed` の検証条件もずれているため、共通の Zod schema
にまとめて両方から参照するようにしてください。`parentPayloadSchema`
を定義している箇所を基準に、進捗イベントと完了判定で同じ契約を使うよう整理し、`processedJobIds` を含む親 payload
の検証ルールが一箇所で保守される形に修正してください。
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e2185e85-2e0c-4f35-b593-a07b5b538ede
📒 Files selected for processing (69)
.agents/skills/ai-service/SKILL.md.agents/skills/job-system/SKILL.md.agents/skills/job-system/agents/openai.yaml.agents/skills/media-search/SKILL.md.agents/skills/media-search/agents/openai.yaml.agents/skills/solid-imager/SKILL.mdAGENTS.mdapps/server/public/openapi.jsonapps/server/src/application/services/ccip-vector-service.tsapps/server/src/application/services/directory-sync-service.tsapps/server/src/application/services/job-dispatch-service.tsapps/server/src/application/services/lancedb-dump-service.tsapps/server/src/application/services/maintenance-service.tsapps/server/src/application/services/media-processing-service.tsapps/server/src/application/services/media-source-service.tsapps/server/src/components/media/media-sidebar.tsxapps/server/src/infrastructure/ai/lancedb-ccip-vector-store.tsapps/server/src/infrastructure/ai/rust-ai-client.tsapps/server/src/infrastructure/api-clients/ai-api.tsapps/server/src/infrastructure/api-clients/search-api.tsapps/server/src/infrastructure/api/routers/ai-router.tsapps/server/src/infrastructure/api/routers/media-router.tsapps/server/src/infrastructure/bootstrap.tsapps/server/src/infrastructure/jobs/ccip-jobs.tsapps/server/src/infrastructure/jobs/file-watcher-service.tsapps/server/src/infrastructure/jobs/job-worker.tsapps/server/src/infrastructure/jobs/tagging-jobs.tsapps/server/src/routes/manager.tsxapps/server/src/routes/search.tsxapps/server/src/tests/unit/application/services/ccip-vector-service.test.tsapps/server/src/tests/unit/application/services/directory-sync-service.test.tsapps/server/src/tests/unit/application/services/maintenance-service.test.tsapps/server/src/tests/unit/infrastructure/jobs/ccip-jobs.test.tsapps/server/src/tests/unit/infrastructure/jobs/job-worker.test.tsapps/server/src/tests/unit/infrastructure/jobs/tagging-jobs.test.tsapps/tauri/src/components/media/media-sidebar/media-sidebar-content.tsxapps/tauri/src/infrastructure/api-clients/search-api.tsapps/tauri/src/routes/manager.tsxapps/tauri/src/routes/search.tsxpackages/application/src/ports/ccip-vector-store.tspackages/application/src/ports/index.tspackages/application/src/ports/lancedb-dump-service.tspackages/application/src/ports/media-processing-service.tspackages/application/src/ports/tagging-service.tspackages/application/src/services/ccip-vector-service.tspackages/application/src/services/index.tspackages/application/src/services/lancedb-dump-service.tspackages/application/src/services/media-processing-service.tspackages/application/src/services/tagging-service.tspackages/core/src/domain/config/config-schema.tspackages/core/src/domain/contract/ai.contract.tspackages/core/src/domain/contract/media.contract.tspackages/core/src/domain/interfaces/ai-client.tspackages/core/src/domain/media/schemas.tspackages/core/src/domain/repositories/job-repository.tspackages/core/src/domain/search/logic.tspackages/core/src/domain/search/schema.tspackages/core/src/domain/tagging/schemas.tspackages/db/src/repositories/job-repository.tspackages/ui/src/hooks/use-current-search-persistence.tspackages/ui/src/hooks/use-manager-page.tspackages/ui/src/hooks/use-search-page.tspackages/ui/src/media-sidebar-content.tsxpackages/ui/src/media-sidebar.tsxpackages/ui/src/preset-manager.tsxpackages/ui/src/screens/config-screen.tsxpackages/ui/src/screens/manager-screen.tsxpackages/ui/src/search-control-panel.tsxpackages/ui/src/stores/search-store.ts
- media-sidebar: AbortController パターンでリアクティビティ問題を修正 - media-router: ベクター削除失敗時の try-catch 追加 - rust-ai-client: asyncPool で並列度を制限 (CPU過剰消費防止) - lancedb-ccip-vector-store: 失敗時 Promise リセット追加 - ai-router: mediaSourceId フィルタ追加 - media-source-service: ベクター削除の try-catch 追加 - ccip-vector-service: isCurrent に mediaSourceId チェック追加 - media-processing-service: createIfUnique で重複投入防止 - ai.contract: mediaSafeSchema 使用 (filePath 除外) - search/logic: ベクトルモード切替時の条件保持 - tagging/schemas: CCIP ベクトル長を 768 に固定 - ui/media-sidebar: メディア切り替え後の抽出結果反映を修正
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/server/src/components/media/media-sidebar.tsx (1)
120-126: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winメディア切替中の抽出フラグが別メディアに漏れます。
isExtractingCcipはコンポーネント共通 state ですが、media 変更時にリセットされず、finallyでも現在の media を確認せずに false に戻しています。これだと旧 media の開始リクエスト中に新しい media のボタンが無効化されたり、新しい media の抽出中に旧リクエストのfinallyが loading を消します。media key を capture してfinallyをガードし、切替時にもisExtractingCcip(false)を明示してください。差分案
createEffect(() => { props.media.id; props.media.mediaSourceId; + setIsExtractingCcip(false); setCcipStatus("missing"); setActiveCcipJobId(null); void refreshCcipStatus(); }); const handleCcipExtraction = async () => { setIsExtractingCcip(true); const currentMediaId = props.media.id; + const currentMediaSourceId = props.media.mediaSourceId; try { const result = await startCcipExtraction( - props.media.mediaSourceId, + currentMediaSourceId, currentMediaId, ccipStatus() === "ready" || ccipStatus() === "stale", ); - if (props.media.id !== currentMediaId) return; + if ( + props.media.id !== currentMediaId || + props.media.mediaSourceId !== currentMediaSourceId + ) + return; setCcipStatus("processing"); setActiveCcipJobId(result.jobId); toast.success("CCIP vector extraction queued"); } catch (error) { - if (props.media.id !== currentMediaId) return; + if ( + props.media.id !== currentMediaId || + props.media.mediaSourceId !== currentMediaSourceId + ) + return; toast.error(`Failed to extract CCIP vector: ${getErrorMessage(error)}`); } finally { - setIsExtractingCcip(false); + if ( + props.media.id === currentMediaId && + props.media.mediaSourceId === currentMediaSourceId + ) { + setIsExtractingCcip(false); + } } };Also applies to: 128-145
🤖 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/components/media/media-sidebar.tsx` around lines 120 - 126, The shared isExtractingCcip state in media-sidebar.tsx is leaking across media switches because it is not reset on media change and the async finally path can clear loading for a different media. Update the media change effect around createEffect, setCcipStatus, and refreshCcipStatus to explicitly set isExtractingCcip(false) when props.media.id or props.media.mediaSourceId changes, and capture the current media key inside the extraction flow so the finally block only clears loading if it still matches the same media. Use the existing props.media, refreshCcipStatus, and isExtractingCcip symbols to keep the guard local to the component logic.
♻️ Duplicate comments (1)
apps/server/src/infrastructure/api/routers/media-router.ts (1)
204-208: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftベクター削除をまだ同期 API パスで待っています。
例外は握りつぶせていますが、ここで
await ccipVectorService.delete(...)を残すと、ベクターストアが遅い/ハングしただけで delete / move / bulkDelete の成功応答まで巻き添えで遅延・タイムアウトします。主操作後の cleanup はジョブ化するか、成功応答後の非同期処理へ切り離した方がよいです。Also applies to: 242-246, 301-308
🤖 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/media-router.ts` around lines 204 - 208, The cleanup path in media-router still blocks the synchronous delete/move/bulkDelete flow by awaiting ccipVectorService.delete, so successful responses can be delayed or time out if the vector store is slow. Refactor the post-success cleanup in the relevant handlers in media-router (the delete, move, and bulkDelete flows) so the vector delete runs asynchronously after the main operation completes, or is dispatched to a background job, while keeping the existing warning logging in the catch path for failed cleanup.
🤖 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 `@packages/core/src/domain/media/schemas.ts`:
- Around line 157-158: `mediaSafeSchema` is using `omit({ filePath: true })`,
which can accidentally include future internal fields added to `mediaSchema`.
Update `mediaSafeSchema` in `schemas.ts` to explicitly whitelist only the public
fields using `pick(...)` or a dedicated `z.object(...)`, and keep `MediaSafe`
inferred from that safe schema. Ensure any API-facing mapping continues to use
`MediaSafe` so only explicitly allowed properties are exposed.
---
Outside diff comments:
In `@apps/server/src/components/media/media-sidebar.tsx`:
- Around line 120-126: The shared isExtractingCcip state in media-sidebar.tsx is
leaking across media switches because it is not reset on media change and the
async finally path can clear loading for a different media. Update the media
change effect around createEffect, setCcipStatus, and refreshCcipStatus to
explicitly set isExtractingCcip(false) when props.media.id or
props.media.mediaSourceId changes, and capture the current media key inside the
extraction flow so the finally block only clears loading if it still matches the
same media. Use the existing props.media, refreshCcipStatus, and
isExtractingCcip symbols to keep the guard local to the component logic.
---
Duplicate comments:
In `@apps/server/src/infrastructure/api/routers/media-router.ts`:
- Around line 204-208: The cleanup path in media-router still blocks the
synchronous delete/move/bulkDelete flow by awaiting ccipVectorService.delete, so
successful responses can be delayed or time out if the vector store is slow.
Refactor the post-success cleanup in the relevant handlers in media-router (the
delete, move, and bulkDelete flows) so the vector delete runs asynchronously
after the main operation completes, or is dispatched to a background job, while
keeping the existing warning logging in the catch path for failed cleanup.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 50e622ea-5784-4fce-97af-84de635fd02d
📒 Files selected for processing (13)
apps/server/src/application/services/media-source-service.tsapps/server/src/components/media/media-sidebar.tsxapps/server/src/infrastructure/ai/lancedb-ccip-vector-store.tsapps/server/src/infrastructure/ai/rust-ai-client.tsapps/server/src/infrastructure/api/routers/ai-router.tsapps/server/src/infrastructure/api/routers/media-router.tspackages/application/src/services/ccip-vector-service.tspackages/application/src/services/media-processing-service.tspackages/core/src/domain/contract/ai.contract.tspackages/core/src/domain/media/schemas.tspackages/core/src/domain/search/logic.tspackages/core/src/domain/tagging/schemas.tspackages/ui/src/media-sidebar.tsx
🚧 Files skipped from review as they are similar to previous changes (8)
- apps/server/src/application/services/media-source-service.ts
- packages/core/src/domain/search/logic.ts
- packages/application/src/services/media-processing-service.ts
- packages/core/src/domain/tagging/schemas.ts
- apps/server/src/infrastructure/ai/lancedb-ccip-vector-store.ts
- packages/ui/src/media-sidebar.tsx
- apps/server/src/infrastructure/api/routers/ai-router.ts
- packages/application/src/services/ccip-vector-service.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/ui/src/hooks/use-manager-page.ts (1)
459-483: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCCIP抽出開始の失敗分岐が抜けています。
startBatchCcipExtraction()がsuccess: falseもしくはjobIdなしで返った場合、この処理は何も通知せずtaggingStatusを"Starting..."のまま残します。handleStartBatchTagging()と同じ失敗分岐を入れないと、開始失敗が UI から判別できません。修正案
const result = await actions.startBatchCcipExtraction({ force: forceRetag(), mediaSourceId: selectedSourceId(), mediaIds: Array.from(selectedMedia()), }); if (result.success && result.jobId) { toast.success(result.message); setTaggingStatus("Batch CCIP extraction in progress..."); setActiveJobId(result.jobId); setScannedMedia([]); setSelectedMedia(new Set<string>()); + } else { + const message = + result.message || "Failed to start batch CCIP extraction."; + toast.error(message); + setTaggingStatus(message); }🤖 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/hooks/use-manager-page.ts` around lines 459 - 483, The handleStartBatchCcipExtraction flow is missing the explicit failure branch when actions.startBatchCcipExtraction returns success: false or no jobId, so the UI can stay stuck on "Starting...". Update this function to mirror handleStartBatchTagging by handling the non-success case inside the result check, showing an error toast and setting taggingStatus to an error message when the batch CCIP extraction does not actually start.
🤖 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.
Outside diff comments:
In `@packages/ui/src/hooks/use-manager-page.ts`:
- Around line 459-483: The handleStartBatchCcipExtraction flow is missing the
explicit failure branch when actions.startBatchCcipExtraction returns success:
false or no jobId, so the UI can stay stuck on "Starting...". Update this
function to mirror handleStartBatchTagging by handling the non-success case
inside the result check, showing an error toast and setting taggingStatus to an
error message when the batch CCIP extraction does not actually start.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a010c2ad-88b1-4c7b-9cc3-a0684951d056
📒 Files selected for processing (11)
apps/server/src/components/media/media-card-item.tsxapps/server/src/components/media/media-sidebar.tsxapps/server/src/components/media/thumbnail-image.tsxapps/server/src/routes/manager.tsxapps/tauri/src/components/media/media-card-item.tsxapps/tauri/src/components/media/thumbnail-image.tsxapps/tauri/src/routes/manager.tsxpackages/core/src/domain/media/schemas.tspackages/ui/src/hooks/use-manager-page.tspackages/ui/src/media-card-item.tsxpackages/ui/src/screens/manager-screen.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/server/src/routes/manager.tsx
- apps/tauri/src/routes/manager.tsx
- packages/core/src/domain/media/schemas.ts
- apps/server/src/components/media/media-sidebar.tsx
- packages/ui/src/screens/manager-screen.tsx
This was referenced Jun 29, 2026
Merged
Merged
This was referenced Jul 9, 2026
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