test: improve test coverage for search#42
Merged
Conversation
Added more tests for repositories, services and more.
📝 WalkthroughWalkthroughRefactors search manager for lazy/async initialization, adds localization fields to song DTO/repository, updates search indexing/search flows to await admin/search index operations, standardizes error responses and messages, adjusts test lifecycles and adds many new unit/integration tests, and makes packaging/config and logger changes. Changes
Sequence Diagram(s)sequenceDiagram
participant Repo as SongRepository
participant Service as SongSearchService
participant Emb as Embedding API
participant Manager as SearchManager
participant Meili as MeiliSearch (admin/search)
participant Log as appLogger
Service->>Repo: getDetailsById(id)
Repo-->>Service: songDetails (with localized fields)
Service->>Manager: getAdminIndex(localeIndex) (await)
Manager->>Meili: getIndex(...)
Meili-->>Manager: indexHandle
Manager-->>Service: indexHandle
Service->>Emb: create embeddings for document (optional)
Emb-->>Service: embeddings (or null)
Service->>Manager: addDocuments(adminIndex, documents)
Manager->>Meili: addDocuments(...)
Meili-->>Manager: taskUid
Manager->>Meili: waitForTask(taskUid)
Meili-->>Manager: task finished
Manager-->>Service: sync complete
Service->>Log: warn/error if sync failed (async)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/backend/src/errorHandler.ts (1)
87-111:⚠️ Potential issue | 🔴 CriticalAdd a fallback for unrecognized
BetterAuthAPIErrors.This branch only stores an error response for the listed conflict codes, invalid-auth codes, or statuses below 500. If Better Auth returns a 5xx,
store.error[traceId]is never initialized here, and Line 129 then destructuresstore.error[traceId]unconditionally. That turns an upstream auth failure into a secondary crash in the error handler.Proposed fix
} else if (error instanceof BetterAuthAPIError) { const bodyCode = error.body?.code || ""; if (bodyCode === "USERNAME_IS_ALREADY_TAKEN") { setErrorResponse(409, { code: bodyCode, message: "error.username-taken", }); } else if (bodyCode === "USER_ALREADY_EXISTS_USE_ANOTHER_EMAIL") { setErrorResponse(409, { code: bodyCode, message: "error.email-taken", }); } else if (AUTH_INVALID_CODES.includes(bodyCode as AuthInvalidCode)) { setErrorResponse(401, { code: "INVALID_CREDENTIALS", message: "error.invalid-cred", }); } else if (error.statusCode < 500) { setErrorResponse(error.statusCode, { code: "UNAUTHORIZED", message: error.body?.message, }); + } else { + setErrorResponse(500, { + code: "INTERNAL_SERVER_ERROR", + message: "error.internal", + }); + store.error[traceId].known = false; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend/src/errorHandler.ts` around lines 87 - 111, The BetterAuthAPIError branch can exit without initializing store.error[traceId] for unrecognized or 5xx errors; update the BetterAuthAPIError handling (the block checking bodyCode, AUTH_INVALID_CODES, and error.statusCode) to add a final fallback that always calls setErrorResponse with a safe default (e.g., 500 and a generic message/code) so store.error[traceId] is initialized; ensure this fallback covers any error.statusCode >= 500 or unknown bodyCode values and keep using the same setErrorResponse helper so subsequent destructuring of store.error[traceId] cannot crash.packages/core/tests/unit/SongService.test.ts (1)
70-76:⚠️ Potential issue | 🟠 MajorMissing
awaiton.rejectsassertions will cause tests to pass incorrectly.When using
.rejectswith Bun's test framework, you mustawaitthe assertion; otherwise the promise rejection is not properly caught and the test may pass even if the code doesn't throw.🐛 Proposed fix
- expect(songService.getDetails(999)).rejects.toThrow(AppError); - expect(songService.getDetails(999)).rejects.toThrow("error.song.notfound"); - expect(songService.getDetails(999)).rejects.toMatchObject({ + await expect(songService.getDetails(999)).rejects.toThrow(AppError); + await expect(songService.getDetails(999)).rejects.toThrow("error.song.notfound"); + await expect(songService.getDetails(999)).rejects.toMatchObject({ code: "NOT_FOUND", statusCode: 404, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/tests/unit/SongService.test.ts` around lines 70 - 76, The tests call songService.getDetails(999) with .rejects but forget to await the assertion, so the rejections aren't properly asserted; update the three assertions to await the promises (e.g., await expect(songService.getDetails(999)).rejects.toThrow(AppError); await expect(songService.getDetails(999)).rejects.toThrow("error.song.notfound"); await expect(songService.getDetails(999)).rejects.toMatchObject({ code: "NOT_FOUND", statusCode: 404 });) so that songService.getDetails, AppError and the rejection shape are correctly verified.
🧹 Nitpick comments (8)
AGENTS.md (1)
37-38: Clarifyrepository.interface.tsvs interface-location rule.Requiring
repository.interface.tscan conflict with the “no local interfaces for API contracts” rule unless explicitly scoped to repository-only contracts.As per coding guidelines: "Do not define local interfaces scoped to a single file for API contracts; define them centrally in
dto.ts."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AGENTS.md` around lines 37 - 38, The comment flags that requiring repository.interface.ts may clash with the "no local interfaces for API contracts" rule; update the project convention and code to ensure repository.interface.ts only contains repository-scoped contracts (not API DTOs) by moving any API-facing interfaces into the central dto.ts and documenting the scope: scan repository.interface.ts and remove or relocate any API contract types into dto.ts, keep only repository-specific interfaces and update imports/usages accordingly (refer to the files container.ts and repository.interface.ts and central dto.ts when making changes).packages/core/src/modules/index.ts (1)
1-2: Use alias-based re-exports to match import-path policy.These relative re-exports conflict with the project’s “no relative paths” rule for TypeScript modules.
As per coding guidelines: "
**/*.{ts,tsx}: ... No relative paths allowed."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/modules/index.ts` around lines 1 - 2, The current module index uses relative re-exports (export * from "./catalog"; export * from "./auth";) which violates the no-relative-paths rule; replace those with the project's configured path aliases (e.g., export * from "@core/catalog"; export * from "@core/auth") or whatever alias is defined in tsconfig/paths so imports comply with the import-path policy, ensuring the symbols exported by the index remain identical; adjust any tsconfig/paths if needed to resolve the aliases.packages/core/tests/utils.ts (1)
15-15: Rename exported constant to match constant naming convention.
OriginalMeiliSearchshould follow UPPER_SNAKE_CASE to stay consistent with project rules.As per coding guidelines: "Use UPPER_SNAKE_CASE for constants (e.g., `MAX_RATE_LIMIT`)."Proposed rename
-export const OriginalMeiliSearch = await import("meilisearch"); +export const ORIGINAL_MEILI_SEARCH = await import("meilisearch");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/tests/utils.ts` at line 15, The exported constant OriginalMeiliSearch should be renamed to UPPER_SNAKE_CASE (e.g., ORIGINAL_MEILISEARCH); update the export declaration that currently declares OriginalMeiliSearch and replace its identifier with ORIGINAL_MEILISEARCH, and then update all references in the codebase/tests that import or use OriginalMeiliSearch to use ORIGINAL_MEILISEARCH so the renamed exported constant remains consistent and compiles.packages/core/src/modules/catalog/song/dto.ts (1)
35-40: Keep localization fields consistent between create and update DTOs.
CreateSongRequestSchemanow acceptslanguage,localizedNames, andlocalizedDescriptions, butUpdateSongRequestSchemadoes not. That makes these fields effectively write-once.♻️ Proposed parity update
export const UpdateSongRequestSchema = z.object({ name: z.string().nullish(), type: SongTypeSchema.nullish(), + language: z.string().nullish(), + localizedNames: z.record(z.string(), z.string()).nullish(), + localizedDescriptions: z.record(z.string(), z.string()).nullish(), duration: z.int().nullish(), description: z.string().nullish(), coverUrl: z.url().nullish(), publishedAt: z.iso.datetime().nullish(), });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/modules/catalog/song/dto.ts` around lines 35 - 40, The UpdateSongRequestSchema is missing the localization fields present in CreateSongRequestSchema, making language/localizedNames/localizedDescriptions write-once; update the UpdateSongRequestSchema to include language: z.string().nullish(), localizedNames: z.record(z.string(), z.string()).nullish(), and localizedDescriptions: z.record(z.string(), z.string()).nullish() (matching the types used in CreateSongRequestSchema) so both DTOs are consistent for localization updates.packages/core/tests/integration/SongSearchService.test.ts (1)
55-65: Avoidas neverin these test doubles.Both assertions suppress real contract drift in the embedding client mock and the “missing manager” case, so this suite can keep compiling after the dependency shape changes. Prefer explicit mock types instead.
As per coding guidelines "Do not use inline type assertions with
as SomeType, use type annotations instead."Also applies to: 204-207
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/tests/integration/SongSearchService.test.ts` around lines 55 - 65, The mock uses `as never`, which hides type drift; replace inline `as never` assertions with explicit mock types: annotate `mockEmbeddingManager` as `Partial<EmbeddingManager>` (or the real EmbeddingManager/Client interface from your codebase) and implement `embeddings.post` accordingly so the compiler checks the shape, and for the "missing manager" case (lines ~204-207) declare the variable as `EmbeddingManager | undefined` (or `Partial<EmbeddingManager> | undefined`) instead of using `as never`; ensure you import the proper type name (EmbeddingManager or the actual interface) and remove all `as never` usages.packages/core/src/search/manager.ts (2)
54-54:syncAllSettings()not awaited—errors will be silently swallowed.If
syncAllSettings()throws, the error is logged but initialization still completes. Consider whether settings sync failures should block client initialization or if this fire-and-forget behavior is intentional.If sync failures should block initialization:
- this.syncAllSettings(); + await this.syncAllSettings();If fire-and-forget is intentional, consider adding a comment explaining why and wrapping in try-catch:
// Fire-and-forget: settings sync is best-effort this.syncAllSettings().catch((e) => appLogger.warn("Settings sync failed", e));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/search/manager.ts` at line 54, The call to syncAllSettings() in the initialization path is currently fire-and-forget and will swallow errors; decide whether failures should block initialization or be best-effort. If they should block, await the promise where syncAllSettings() is invoked (e.g., in the constructor/init method that calls syncAllSettings) so initialization awaits completion and surfaces errors; if they are intentionally best-effort, convert the call to an explicit fire-and-forget with a catch that logs the error (use appLogger.warn or similar) and add a short comment like "Fire-and-forget: settings sync is best-effort" next to the syncAllSettings() call to make intent clear. Ensure you reference the syncAllSettings() method and the surrounding init/constructor code when making the change.
46-53: Type assertion workaround to mutate readonly properties.The
as unknown as { client: MeiliSearch }pattern circumvents TypeScript's readonly checking. This works but is fragile.Consider declaring the properties as mutable but undefined initially:
- private constructor( - private client?: MeiliSearch, - private adminClient?: MeiliSearch - ) {} + private client: MeiliSearch | undefined; + private adminClient: MeiliSearch | undefined; + + private constructor(client?: MeiliSearch, adminClient?: MeiliSearch) { + this.client = client; + this.adminClient = adminClient; + }Then remove the
as unknown ascasts:- (this as unknown as { client: MeiliSearch }).client = new MeiliSearch({ + this.client = new MeiliSearch({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/search/manager.ts` around lines 46 - 53, The code is using unsafe type assertions "(this as unknown as { client: MeiliSearch })" to bypass readonly checks when assigning MeiliSearch instances to client and adminClient; change the class property declarations for client and adminClient to be mutable and optionally undefined (e.g., declare client?: MeiliSearch and adminClient?: MeiliSearch or client: MeiliSearch | undefined) so you can assign to them directly in the constructor/initializer, then remove the "as unknown as" casts and perform direct assignments to this.client and this.adminClient; update any code that expects them to be non-null to handle the optional/undefined state or initialize them immediately to avoid null checks.packages/core/tests/unit/SearchManager.test.ts (1)
179-220: Consider extracting hardcoded settings to avoid drift.The
matchingSettingsobject duplicates the expectedINDEX_SETTINGSvalues. IfINDEX_SETTINGSis updated inpackages/core/src/search/config.ts, this test could pass incorrectly or fail unexpectedly.♻️ Optional: Import and use actual INDEX_SETTINGS
+import { INDEX_SETTINGS } from "../../src/search/config"; + // In test: - const matchingSettings = { - searchableAttributes: [ - "name", - "lyrics", - // ... hardcoded values - ], - // ... - }; + const matchingSettings = INDEX_SETTINGS.song;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/tests/unit/SearchManager.test.ts` around lines 179 - 220, The test defines a hardcoded matchingSettings object that duplicates the canonical INDEX_SETTINGS and can drift; replace matchingSettings with an import of the real INDEX_SETTINGS (or a shallow clone/merge if locale-specific adjustments are required) from the module that exports INDEX_SETTINGS (e.g., packages/core/src/search/config.ts) and use that in the mocked index returned to SearchManager.create so the test asserts against the actual configuration used by SearchManager.syncAllSettings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@AGENTS.md`:
- Around line 32-37: The document mixes two different canonical module paths;
standardize all occurrences to a single path (pick one, e.g.,
packages/core/src/modules/<module>/) and update every reference accordingly
(including examples like dto.ts, service.ts, repository.ts, container.ts and any
later rules that refer to packages/core/modules/<module>/dto.ts) so the tree and
subsequent rules use the same path format throughout the file.
In `@apps/backend/bunfig.toml`:
- Around line 2-13: The coverage exclusion list currently ignores authentication
code via the pattern "**/modules/auth/**"; remove that pattern from the
coveragePathIgnorePatterns array so auth modules are included in coverage
baselines (i.e., delete the "**/modules/auth/**" entry from the array in
bunfig.toml), keep surrounding formatting/commas intact to preserve valid TOML.
In `@apps/backend/tests/auth.test.ts`:
- Line 1: Remove the unused beforeEach import from the test import statement:
edit the import that currently lists beforeEach, beforeAll, afterAll, describe,
expect, test and delete the beforeEach symbol so the import only includes the
actually used helpers (beforeAll, afterAll, describe, expect, test) to resolve
the CI lint error.
In `@packages/core/bunfig.toml`:
- Around line 2-13: Remove the auth module exclusion from the test coverage
ignore list: edit the coveragePathIgnorePatterns array (the
coveragePathIgnorePatterns setting) and delete the entry "**/modules/auth/**" so
that auth code (modules/auth) is included in coverage reports; leave the other
existing patterns intact.
In `@packages/core/src/modules/catalog/song/container.ts`:
- Line 4: The import in container.ts currently uses a relative path
("../../../search") for SongSearchService and searchManager; replace that
relative import with the project's TypeScript path alias for the search module
(i.e., import SongSearchService and searchManager from the configured alias
instead of a relative path) so the file follows the repository import-path rule
and keeps external/workspace/alias ordering.
In `@packages/core/src/modules/catalog/song/service.ts`:
- Around line 34-38: The traceTask callback currently fires
this.search.sync(result.id) without awaiting it, causing the trace to finish
before indexing completes; update the traceTask usages in the create, update,
and delete flows (the traceTask(... async () => { ... }) blocks that call
this.search.sync(...)) to await the this.search.sync(...) promise and handle
errors by catching and logging/swallowing rejections inside the async callback
so the trace encompasses the full indexing operation while still preventing
throws from bubbling.
In `@packages/core/src/search/catalog/song.ts`:
- Around line 54-57: The arrow function expression assigned to getName is
missing a trailing semicolon which breaks Biome formatting; update the getName
declaration (the const getName = () => { ... } inside
packages/core/src/search/catalog/song.ts) to end with a semicolon so the
statement is properly terminated.
In `@packages/core/tests/integration/SongRepository.test.ts`:
- Around line 19-27: The CI format error is caused by unformatted code in the
test's beforeAll block; run the Biome formatter on this test file (targeting the
beforeAll block in SongRepository.test.ts), e.g., execute the project's Biome
CLI/IDE formatting command for the file, review the changes, stage and commit
the formatted file so the CI `format` check passes. Ensure you don't alter test
logic—only apply automatic formatting.
In `@packages/core/tests/integration/SongSearchService.test.ts`:
- Around line 203-212: The test creates a SongSearchService instance
(serviceWithoutManager) and asserts serviceWithoutManager.search("test")
rejects, but the expect(...).rejects assertion isn't awaited; update the test so
the rejection assertion is awaited (or return the promise) so the test runner
waits for expect(...).rejects.toThrow to execute; locate the test "throws error
when search manager is not available" and modify the line with
expect(serviceWithoutManager.search("test")).rejects.toThrow(...) to await the
expect or return the expect promise to ensure the async assertion runs.
- Around line 68-77: The beforeEach hook calls indexZh.deleteAllDocuments() and
indexEn.deleteAllDocuments() but doesn't wait for Meili's asynchronous deletion
tasks to complete, causing race conditions; change the hook to capture the
returned task(s) from indexZh.deleteAllDocuments() and
indexEn.deleteAllDocuments() and then await Meili to finish those tasks (e.g.,
by calling the client's/task-wait helper such as client.waitForTask(task.uid) or
equivalent) before returning from beforeEach so both deletions are fully
completed before tests run.
- Around line 4-6: Import SongSearchService from the public package (`@cvsa/core`)
instead of a relative path, and avoid directly importing the SearchManager
class; either re-export SearchManager from search/index.ts or switch the test to
use the already exported searchManager instance (or another public factory) from
the package. Update the test imports so SongSearchService comes from
"@cvsa/core" and replace the relative import of SearchManager with the
package-exported SearchManager or the exported searchManager instance to satisfy
the no-relative-paths guideline.
In `@packages/core/tests/unit/SongSearchService.test.ts`:
- Around line 271-281: The test "throws when search manager not available" is
missing an await on the .rejects assertion; update the test so the Promise
returned by serviceWithoutManager.search("test") is awaited with await
expect(...).rejects.toThrow(...). Locate the SongSearchService instantiation
(serviceWithoutManager) and the call to search(...) and add the await before
expect(...).rejects.toThrow to ensure the rejection is properly awaited; mirror
the same fix applied in SongService.test.ts if present.
In `@packages/core/tests/unit/SongService.test.ts`:
- Around line 128-137: The three assertions in the "throws NOT_FOUND error when
song does not exist" test are missing await on their .rejects chains, so they
never actually await the promise rejection; update the test to await the
rejection assertions (e.g., await expect(songService.update(999,
updateInput)).rejects.toThrow(AppError), await
expect(...).rejects.toThrow("error.song.notfound"), and await
expect(...).rejects.toMatchObject({ code: "NOT_FOUND", statusCode: 404 })), or
assign the promise to a variable (const p = songService.update(999,
updateInput)) and await expect(p).rejects... for each check to ensure the
rejection is properly asserted.
- Around line 163-172: The test is missing awaits on the promise rejection
assertions causing false positives; update the three assertions that call
songService.delete(999) so they await the rejects matcher (i.e., use await
expect(songService.delete(999)).rejects.toThrow(AppError), await
expect(...).rejects.toThrow("error.song.notfound"), and await
expect(...).rejects.toMatchObject({ code: "NOT_FOUND", statusCode: 404 }));
locate these in the test block referencing mockRepository.getById and
songService.delete and add the awaits.
In `@packages/db/package.json`:
- Line 14: The "reset" npm script currently runs "prisma migrate reset"
unguarded; change the "reset" script in package.json to enforce an environment
safeguard by wrapping the command in a small node check (or reuse the project
pattern from "test:setup") that aborts if NODE_ENV === "production" (or only
allows explicit NODE_ENV values like "development" or "test"), or require an
explicit override env var (e.g., FORCE_DB_RESET=true) before executing prisma
migrate reset; update the script name "reset" accordingly and ensure the guard
emits a clear error and exits non-zero when the check fails.
---
Outside diff comments:
In `@apps/backend/src/errorHandler.ts`:
- Around line 87-111: The BetterAuthAPIError branch can exit without
initializing store.error[traceId] for unrecognized or 5xx errors; update the
BetterAuthAPIError handling (the block checking bodyCode, AUTH_INVALID_CODES,
and error.statusCode) to add a final fallback that always calls setErrorResponse
with a safe default (e.g., 500 and a generic message/code) so
store.error[traceId] is initialized; ensure this fallback covers any
error.statusCode >= 500 or unknown bodyCode values and keep using the same
setErrorResponse helper so subsequent destructuring of store.error[traceId]
cannot crash.
In `@packages/core/tests/unit/SongService.test.ts`:
- Around line 70-76: The tests call songService.getDetails(999) with .rejects
but forget to await the assertion, so the rejections aren't properly asserted;
update the three assertions to await the promises (e.g., await
expect(songService.getDetails(999)).rejects.toThrow(AppError); await
expect(songService.getDetails(999)).rejects.toThrow("error.song.notfound");
await expect(songService.getDetails(999)).rejects.toMatchObject({ code:
"NOT_FOUND", statusCode: 404 });) so that songService.getDetails, AppError and
the rejection shape are correctly verified.
---
Nitpick comments:
In `@AGENTS.md`:
- Around line 37-38: The comment flags that requiring repository.interface.ts
may clash with the "no local interfaces for API contracts" rule; update the
project convention and code to ensure repository.interface.ts only contains
repository-scoped contracts (not API DTOs) by moving any API-facing interfaces
into the central dto.ts and documenting the scope: scan repository.interface.ts
and remove or relocate any API contract types into dto.ts, keep only
repository-specific interfaces and update imports/usages accordingly (refer to
the files container.ts and repository.interface.ts and central dto.ts when
making changes).
In `@packages/core/src/modules/catalog/song/dto.ts`:
- Around line 35-40: The UpdateSongRequestSchema is missing the localization
fields present in CreateSongRequestSchema, making
language/localizedNames/localizedDescriptions write-once; update the
UpdateSongRequestSchema to include language: z.string().nullish(),
localizedNames: z.record(z.string(), z.string()).nullish(), and
localizedDescriptions: z.record(z.string(), z.string()).nullish() (matching the
types used in CreateSongRequestSchema) so both DTOs are consistent for
localization updates.
In `@packages/core/src/modules/index.ts`:
- Around line 1-2: The current module index uses relative re-exports (export *
from "./catalog"; export * from "./auth";) which violates the no-relative-paths
rule; replace those with the project's configured path aliases (e.g., export *
from "@core/catalog"; export * from "@core/auth") or whatever alias is defined
in tsconfig/paths so imports comply with the import-path policy, ensuring the
symbols exported by the index remain identical; adjust any tsconfig/paths if
needed to resolve the aliases.
In `@packages/core/src/search/manager.ts`:
- Line 54: The call to syncAllSettings() in the initialization path is currently
fire-and-forget and will swallow errors; decide whether failures should block
initialization or be best-effort. If they should block, await the promise where
syncAllSettings() is invoked (e.g., in the constructor/init method that calls
syncAllSettings) so initialization awaits completion and surfaces errors; if
they are intentionally best-effort, convert the call to an explicit
fire-and-forget with a catch that logs the error (use appLogger.warn or similar)
and add a short comment like "Fire-and-forget: settings sync is best-effort"
next to the syncAllSettings() call to make intent clear. Ensure you reference
the syncAllSettings() method and the surrounding init/constructor code when
making the change.
- Around line 46-53: The code is using unsafe type assertions "(this as unknown
as { client: MeiliSearch })" to bypass readonly checks when assigning
MeiliSearch instances to client and adminClient; change the class property
declarations for client and adminClient to be mutable and optionally undefined
(e.g., declare client?: MeiliSearch and adminClient?: MeiliSearch or client:
MeiliSearch | undefined) so you can assign to them directly in the
constructor/initializer, then remove the "as unknown as" casts and perform
direct assignments to this.client and this.adminClient; update any code that
expects them to be non-null to handle the optional/undefined state or initialize
them immediately to avoid null checks.
In `@packages/core/tests/integration/SongSearchService.test.ts`:
- Around line 55-65: The mock uses `as never`, which hides type drift; replace
inline `as never` assertions with explicit mock types: annotate
`mockEmbeddingManager` as `Partial<EmbeddingManager>` (or the real
EmbeddingManager/Client interface from your codebase) and implement
`embeddings.post` accordingly so the compiler checks the shape, and for the
"missing manager" case (lines ~204-207) declare the variable as
`EmbeddingManager | undefined` (or `Partial<EmbeddingManager> | undefined`)
instead of using `as never`; ensure you import the proper type name
(EmbeddingManager or the actual interface) and remove all `as never` usages.
In `@packages/core/tests/unit/SearchManager.test.ts`:
- Around line 179-220: The test defines a hardcoded matchingSettings object that
duplicates the canonical INDEX_SETTINGS and can drift; replace matchingSettings
with an import of the real INDEX_SETTINGS (or a shallow clone/merge if
locale-specific adjustments are required) from the module that exports
INDEX_SETTINGS (e.g., packages/core/src/search/config.ts) and use that in the
mocked index returned to SearchManager.create so the test asserts against the
actual configuration used by SearchManager.syncAllSettings.
In `@packages/core/tests/utils.ts`:
- Line 15: The exported constant OriginalMeiliSearch should be renamed to
UPPER_SNAKE_CASE (e.g., ORIGINAL_MEILISEARCH); update the export declaration
that currently declares OriginalMeiliSearch and replace its identifier with
ORIGINAL_MEILISEARCH, and then update all references in the codebase/tests that
import or use OriginalMeiliSearch to use ORIGINAL_MEILISEARCH so the renamed
exported constant remains consistent and compiles.
🪄 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 (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 019d3a18-b238-4373-bd86-556c51ea2bc7
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (39)
.tokeignoreAGENTS.mdapps/backend/bunfig.tomlapps/backend/src/errorHandler.tsapps/backend/tests/auth.test.tsapps/backend/tests/song.test.tslocale/modules/backend/messages/en.jsonlocale/modules/backend/messages/zh.jsonpackages/core/bunfig.tomlpackages/core/package.jsonpackages/core/src/index.tspackages/core/src/internal.tspackages/core/src/modules/auth/betterAuth.tspackages/core/src/modules/auth/index.tspackages/core/src/modules/auth/user/dto.tspackages/core/src/modules/auth/user/index.tspackages/core/src/modules/catalog/index.tspackages/core/src/modules/catalog/song/container.tspackages/core/src/modules/catalog/song/dto.tspackages/core/src/modules/catalog/song/index.tspackages/core/src/modules/catalog/song/repository.interface.tspackages/core/src/modules/catalog/song/repository.tspackages/core/src/modules/catalog/song/service.tspackages/core/src/modules/index.tspackages/core/src/search/catalog/song.tspackages/core/src/search/interface.tspackages/core/src/search/manager.tspackages/core/tests/integration/SongRepository.test.tspackages/core/tests/integration/SongSearchService.test.tspackages/core/tests/unit/SearchManager.test.tspackages/core/tests/unit/SongSearchService.test.tspackages/core/tests/unit/SongService.test.tspackages/core/tests/utils.tspackages/db/package.jsonpackages/db/prisma/models/core/artist.prismapackages/db/prisma/models/core/artistRole.prismapackages/db/prisma/models/core/singer.prismapackages/logger/package.jsonpackages/logger/src/pinoSetup.ts
💤 Files with no reviewable changes (1)
- .tokeignore
Comment on lines
+32
to
+37
| packages/core/src/modules/[module]/ | ||
| ├── index.ts # Public API exports | ||
| ├── dto.ts # Zod schemas (Validation & Types) | ||
| ├── service.ts # Business logic | ||
| ├── repository.ts # Prisma DB operations | ||
| ├── container.ts # DI container |
There was a problem hiding this comment.
Unify the core module path across this document.
This tree uses packages/core/src/modules/[module]/, but later rules still reference packages/core/modules/<module>/dto.ts. Please keep one canonical path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@AGENTS.md` around lines 32 - 37, The document mixes two different canonical
module paths; standardize all occurrences to a single path (pick one, e.g.,
packages/core/src/modules/<module>/) and update every reference accordingly
(including examples like dto.ts, service.ts, repository.ts, container.ts and any
later rules that refer to packages/core/modules/<module>/dto.ts) so the tree and
subsequent rules use the same path format throughout the file.
Comment on lines
+2
to
+13
| coveragePathIgnorePatterns = [ | ||
| "**/db/prisma/**", | ||
| "**/db/src/**", | ||
| "**/env/src/**", | ||
| "**/logger/src/**", | ||
| "**/modules/auth/**", | ||
| "src/error/**", | ||
| "**/**.interface.ts", | ||
| "src/utils/randomId.ts", | ||
| "src/types/**", | ||
| "**/observability/src/**", | ||
| ] No newline at end of file |
There was a problem hiding this comment.
Don’t exclude auth paths from coverage baselines.
Ignoring **/modules/auth/** can mask regressions in security-critical flows. Keep auth modules in reported coverage.
Suggested adjustment
coveragePathIgnorePatterns = [
"**/db/prisma/**",
"**/db/src/**",
"**/env/src/**",
"**/logger/src/**",
- "**/modules/auth/**",
"src/error/**",
"**/**.interface.ts",
"src/utils/randomId.ts",
"src/types/**",
"**/observability/src/**",
]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| coveragePathIgnorePatterns = [ | |
| "**/db/prisma/**", | |
| "**/db/src/**", | |
| "**/env/src/**", | |
| "**/logger/src/**", | |
| "**/modules/auth/**", | |
| "src/error/**", | |
| "**/**.interface.ts", | |
| "src/utils/randomId.ts", | |
| "src/types/**", | |
| "**/observability/src/**", | |
| ] | |
| coveragePathIgnorePatterns = [ | |
| "**/db/prisma/**", | |
| "**/db/src/**", | |
| "**/env/src/**", | |
| "**/logger/src/**", | |
| "src/error/**", | |
| "**/**.interface.ts", | |
| "src/utils/randomId.ts", | |
| "src/types/**", | |
| "**/observability/src/**", | |
| ] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/backend/bunfig.toml` around lines 2 - 13, The coverage exclusion list
currently ignores authentication code via the pattern "**/modules/auth/**";
remove that pattern from the coveragePathIgnorePatterns array so auth modules
are included in coverage baselines (i.e., delete the "**/modules/auth/**" entry
from the array in bunfig.toml), keep surrounding formatting/commas intact to
preserve valid TOML.
| @@ -1,12 +1,13 @@ | |||
| import { afterAll, beforeEach, describe, expect, test } from "bun:test"; | |||
| import { afterAll, beforeAll, beforeEach, describe, expect, test } from "bun:test"; | |||
There was a problem hiding this comment.
Remove the unused beforeEach import.
CI is already flagging this import block, and beforeEach is no longer referenced after the hook refactor.
🧰 Tools
🪛 GitHub Actions: CI
[warning] 1-1: Biome lint/correctness/noUnusedImports: Several imports are unused (e.g., afterAll, beforeAll, beforeEach, describe, expect, test from bun:test).
🪛 GitHub Check: Test and Lint
[warning] 1-1: lint/correctness/noUnusedImports
Several of these imports are unused.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/backend/tests/auth.test.ts` at line 1, Remove the unused beforeEach
import from the test import statement: edit the import that currently lists
beforeEach, beforeAll, afterAll, describe, expect, test and delete the
beforeEach symbol so the import only includes the actually used helpers
(beforeAll, afterAll, describe, expect, test) to resolve the CI lint error.
Comment on lines
+2
to
+13
| coveragePathIgnorePatterns = [ | ||
| "**/db/prisma/**", | ||
| "**/db/src/**", | ||
| "**/env/src/**", | ||
| "**/logger/src/**", | ||
| "**/modules/auth/**", | ||
| "src/error/**", | ||
| "**/**.interface.ts", | ||
| "src/utils/randomId.ts", | ||
| "src/types/**", | ||
| "**/observability/src/**", | ||
| ] No newline at end of file |
There was a problem hiding this comment.
Avoid excluding core auth modules from coverage accounting.
**/modules/auth/** is a high-risk area; excluding it weakens coverage signal for core business/auth logic.
Suggested adjustment
coveragePathIgnorePatterns = [
"**/db/prisma/**",
"**/db/src/**",
"**/env/src/**",
"**/logger/src/**",
- "**/modules/auth/**",
"src/error/**",
"**/**.interface.ts",
"src/utils/randomId.ts",
"src/types/**",
"**/observability/src/**",
]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| coveragePathIgnorePatterns = [ | |
| "**/db/prisma/**", | |
| "**/db/src/**", | |
| "**/env/src/**", | |
| "**/logger/src/**", | |
| "**/modules/auth/**", | |
| "src/error/**", | |
| "**/**.interface.ts", | |
| "src/utils/randomId.ts", | |
| "src/types/**", | |
| "**/observability/src/**", | |
| ] | |
| coveragePathIgnorePatterns = [ | |
| "**/db/prisma/**", | |
| "**/db/src/**", | |
| "**/env/src/**", | |
| "**/logger/src/**", | |
| "src/error/**", | |
| "**/**.interface.ts", | |
| "src/utils/randomId.ts", | |
| "src/types/**", | |
| "**/observability/src/**", | |
| ] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/bunfig.toml` around lines 2 - 13, Remove the auth module
exclusion from the test coverage ignore list: edit the
coveragePathIgnorePatterns array (the coveragePathIgnorePatterns setting) and
delete the entry "**/modules/auth/**" so that auth code (modules/auth) is
included in coverage reports; leave the other existing patterns intact.
| import { SongRepository } from "./repository"; | ||
| import { SongService } from "./service"; | ||
| import { SongSearchService, searchManager } from "../../search"; | ||
| import { SongSearchService, searchManager } from "../../../search"; |
There was a problem hiding this comment.
Replace relative search import with the project alias path.
Line 4 uses a relative import (../../../search), which violates the repository import-path rule for TypeScript files.
As per coding guidelines, **/*.{ts,tsx}: Order imports from farthest to closest: External packages → Workspace packages → Path aliases → No relative paths allowed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/modules/catalog/song/container.ts` at line 4, The import in
container.ts currently uses a relative path ("../../../search") for
SongSearchService and searchManager; replace that relative import with the
project's TypeScript path alias for the search module (i.e., import
SongSearchService and searchManager from the configured alias instead of a
relative path) so the file follows the repository import-path rule and keeps
external/workspace/alias ordering.
Comment on lines
+203
to
+212
| test("throws error when search manager is not available", async () => { | ||
| const serviceWithoutManager = new SongSearchService( | ||
| songRepository, | ||
| undefined as never, | ||
| mockEmbeddingManager | ||
| ); | ||
|
|
||
| expect(serviceWithoutManager.search("test")).rejects.toThrow( | ||
| "Search or embedding service not available" | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "SongSearchService.test.ts" -type fRepository: project-cvsa/cvsa
Length of output: 173
🏁 Script executed:
cat -n packages/core/tests/integration/SongSearchService.test.ts | sed -n '200,220p'Repository: project-cvsa/cvsa
Length of output: 673
🌐 Web query:
Jest expect().rejects.toThrow() await requirement
💡 Result:
In Jest, expect(promise).rejects.toThrow does not strictly require await—it works in both non-async tests (with return) and async tests (with await). However, using await in an async test function is the recommended modern approach. Non-async example (must return the promise): test('rejects to octopus', => { return expect(Promise.reject(new Error('octopus'))).rejects.toThrow('octopus'); }); Async example (recommended, uses await): test('rejects to octopus', async => { await expect(Promise.reject(new Error('octopus'))).rejects.toThrow('octopus'); }); Both make the test wait for the promise rejection. The await version is syntactic sugar for the returned promise version and is preferred for readability. Omitting return or await causes the test to complete prematurely.
Citations:
- 1: https://jestjs.io/docs/expect
- 2: https://jestjs.io/docs/en/expect
- 3: https://github.com/jestjs/jest/blob/v29.7.0/docs/ExpectAPI.md
- 4: https://jestjs.io/docs/asynchronous
Await the rejection assertion.
Without await or return, the async test function completes before expect().rejects.toThrow() executes, allowing the test to pass even if search() doesn't reject as expected.
Proposed fix
- expect(serviceWithoutManager.search("test")).rejects.toThrow(
+ await expect(serviceWithoutManager.search("test")).rejects.toThrow(
"Search or embedding service not available"
);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/tests/integration/SongSearchService.test.ts` around lines 203 -
212, The test creates a SongSearchService instance (serviceWithoutManager) and
asserts serviceWithoutManager.search("test") rejects, but the
expect(...).rejects assertion isn't awaited; update the test so the rejection
assertion is awaited (or return the promise) so the test runner waits for
expect(...).rejects.toThrow to execute; locate the test "throws error when
search manager is not available" and modify the line with
expect(serviceWithoutManager.search("test")).rejects.toThrow(...) to await the
expect or return the expect promise to ensure the async assertion runs.
Comment on lines
+271
to
+281
| test("throws when search manager not available", async () => { | ||
| const serviceWithoutManager = new SongSearchService( | ||
| mockRepository, | ||
| undefined as never, | ||
| mockEmbeddingManager as never | ||
| ); | ||
|
|
||
| expect(serviceWithoutManager.search("test")).rejects.toThrow( | ||
| "Search or embedding service not available" | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Missing await on .rejects assertion.
Same issue as in SongService.test.ts—the .rejects assertion needs await.
🐛 Proposed fix
test("throws when search manager not available", async () => {
const serviceWithoutManager = new SongSearchService(
mockRepository,
undefined as never,
mockEmbeddingManager as never
);
- expect(serviceWithoutManager.search("test")).rejects.toThrow(
+ await expect(serviceWithoutManager.search("test")).rejects.toThrow(
"Search or embedding service not available"
);
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/tests/unit/SongSearchService.test.ts` around lines 271 - 281,
The test "throws when search manager not available" is missing an await on the
.rejects assertion; update the test so the Promise returned by
serviceWithoutManager.search("test") is awaited with await
expect(...).rejects.toThrow(...). Locate the SongSearchService instantiation
(serviceWithoutManager) and the call to search(...) and add the await before
expect(...).rejects.toThrow to ensure the rejection is properly awaited; mirror
the same fix applied in SongService.test.ts if present.
Comment on lines
+128
to
+137
| test("throws NOT_FOUND error when song does not exist", async () => { | ||
| mockRepository.getById.mockResolvedValueOnce(null); | ||
|
|
||
| expect(songService.update(999, updateInput)).rejects.toThrow(AppError); | ||
| expect(songService.update(999, updateInput)).rejects.toThrow("error.song.notfound"); | ||
| expect(songService.update(999, updateInput)).rejects.toMatchObject({ | ||
| code: "NOT_FOUND", | ||
| statusCode: 404, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Missing await on .rejects assertions.
Same issue as above—the .rejects assertions need await to properly verify the rejection.
🐛 Proposed fix
test("throws NOT_FOUND error when song does not exist", async () => {
mockRepository.getById.mockResolvedValueOnce(null);
- expect(songService.update(999, updateInput)).rejects.toThrow(AppError);
- expect(songService.update(999, updateInput)).rejects.toThrow("error.song.notfound");
- expect(songService.update(999, updateInput)).rejects.toMatchObject({
+ await expect(songService.update(999, updateInput)).rejects.toThrow(AppError);
+ await expect(songService.update(999, updateInput)).rejects.toThrow("error.song.notfound");
+ await expect(songService.update(999, updateInput)).rejects.toMatchObject({
code: "NOT_FOUND",
statusCode: 404,
});
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| test("throws NOT_FOUND error when song does not exist", async () => { | |
| mockRepository.getById.mockResolvedValueOnce(null); | |
| expect(songService.update(999, updateInput)).rejects.toThrow(AppError); | |
| expect(songService.update(999, updateInput)).rejects.toThrow("error.song.notfound"); | |
| expect(songService.update(999, updateInput)).rejects.toMatchObject({ | |
| code: "NOT_FOUND", | |
| statusCode: 404, | |
| }); | |
| }); | |
| test("throws NOT_FOUND error when song does not exist", async () => { | |
| mockRepository.getById.mockResolvedValueOnce(null); | |
| await expect(songService.update(999, updateInput)).rejects.toThrow(AppError); | |
| await expect(songService.update(999, updateInput)).rejects.toThrow("error.song.notfound"); | |
| await expect(songService.update(999, updateInput)).rejects.toMatchObject({ | |
| code: "NOT_FOUND", | |
| statusCode: 404, | |
| }); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/tests/unit/SongService.test.ts` around lines 128 - 137, The
three assertions in the "throws NOT_FOUND error when song does not exist" test
are missing await on their .rejects chains, so they never actually await the
promise rejection; update the test to await the rejection assertions (e.g.,
await expect(songService.update(999, updateInput)).rejects.toThrow(AppError),
await expect(...).rejects.toThrow("error.song.notfound"), and await
expect(...).rejects.toMatchObject({ code: "NOT_FOUND", statusCode: 404 })), or
assign the promise to a variable (const p = songService.update(999,
updateInput)) and await expect(p).rejects... for each check to ensure the
rejection is properly asserted.
Comment on lines
+163
to
+172
| test("throws NOT_FOUND error when song does not exist", async () => { | ||
| mockRepository.getById.mockResolvedValueOnce(null); | ||
|
|
||
| expect(songService.delete(999)).rejects.toThrow(AppError); | ||
| expect(songService.delete(999)).rejects.toThrow("error.song.notfound"); | ||
| expect(songService.delete(999)).rejects.toMatchObject({ | ||
| code: "NOT_FOUND", | ||
| statusCode: 404, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Missing await on .rejects assertions.
Same issue—please add await to these assertions.
🐛 Proposed fix
test("throws NOT_FOUND error when song does not exist", async () => {
mockRepository.getById.mockResolvedValueOnce(null);
- expect(songService.delete(999)).rejects.toThrow(AppError);
- expect(songService.delete(999)).rejects.toThrow("error.song.notfound");
- expect(songService.delete(999)).rejects.toMatchObject({
+ await expect(songService.delete(999)).rejects.toThrow(AppError);
+ await expect(songService.delete(999)).rejects.toThrow("error.song.notfound");
+ await expect(songService.delete(999)).rejects.toMatchObject({
code: "NOT_FOUND",
statusCode: 404,
});
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| test("throws NOT_FOUND error when song does not exist", async () => { | |
| mockRepository.getById.mockResolvedValueOnce(null); | |
| expect(songService.delete(999)).rejects.toThrow(AppError); | |
| expect(songService.delete(999)).rejects.toThrow("error.song.notfound"); | |
| expect(songService.delete(999)).rejects.toMatchObject({ | |
| code: "NOT_FOUND", | |
| statusCode: 404, | |
| }); | |
| }); | |
| test("throws NOT_FOUND error when song does not exist", async () => { | |
| mockRepository.getById.mockResolvedValueOnce(null); | |
| await expect(songService.delete(999)).rejects.toThrow(AppError); | |
| await expect(songService.delete(999)).rejects.toThrow("error.song.notfound"); | |
| await expect(songService.delete(999)).rejects.toMatchObject({ | |
| code: "NOT_FOUND", | |
| statusCode: 404, | |
| }); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/tests/unit/SongService.test.ts` around lines 163 - 172, The
test is missing awaits on the promise rejection assertions causing false
positives; update the three assertions that call songService.delete(999) so they
await the rejects matcher (i.e., use await
expect(songService.delete(999)).rejects.toThrow(AppError), await
expect(...).rejects.toThrow("error.song.notfound"), and await
expect(...).rejects.toMatchObject({ code: "NOT_FOUND", statusCode: 404 }));
locate these in the test block referencing mockRepository.getById and
songService.delete and add the awaits.
| "test:setup": "NODE_ENV=test bunx --bun prisma db push --accept-data-loss", | ||
| "studio": "bunx --bun prisma studio" | ||
| "studio": "bunx --bun prisma studio", | ||
| "reset": "bunx --bun prisma migrate reset" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether reset is used in automation paths that may run with non-dev env.
# Expected: no CI/deploy usage of `bun run reset` or `prisma migrate reset` without explicit safe env gating.
rg -n -C3 'bun run reset|prisma migrate reset|npm run reset|pnpm run reset|yarn reset' .github package.json '**/package.json' || true
rg -n -C3 'DATABASE_URL|NODE_ENV' .github '**/*.yml' '**/*.yaml' '**/package.json' || trueRepository: project-cvsa/cvsa
Length of output: 271
🏁 Script executed:
# First, locate and examine the packages/db/package.json file
fd -t f package.json | grep -E "(packages/db|db/)" | head -20Repository: project-cvsa/cvsa
Length of output: 85
🏁 Script executed:
# Search for GitHub workflows that might use the reset script
fd -t f -e yml -e yaml .github/ 2>/dev/null | head -20Repository: project-cvsa/cvsa
Length of output: 43
🏁 Script executed:
# Search for references to reset, DATABASE_URL, and NODE_ENV in the codebase
rg -i "bun run reset|migrate reset" --type json --type yaml -g ".github" 2>/dev/null || trueRepository: project-cvsa/cvsa
Length of output: 43
🏁 Script executed:
# Get the full content of packages/db/package.json to understand context
cat packages/db/package.json 2>/dev/null || echo "File not found"Repository: project-cvsa/cvsa
Length of output: 902
🏁 Script executed:
# Search for any prisma setup or migration scripts in the entire repo
rg -l "prisma migrate|DATABASE_URL" --max-count 20Repository: project-cvsa/cvsa
Length of output: 166
🏁 Script executed:
# Search for any references to "reset" command in the entire codebase
rg -i "bun run reset|\"reset\"|reset:" --type json --type ts --type js -C 2 | head -50Repository: project-cvsa/cvsa
Length of output: 374
🏁 Script executed:
# Check the prisma configuration file for any safety mechanisms
cat packages/db/prisma.config.ts 2>/dev/null || echo "File not found"Repository: project-cvsa/cvsa
Length of output: 375
🏁 Script executed:
# Check src/prisma.ts for any related safety mechanisms
cat packages/db/src/prisma.ts 2>/dev/null || echo "File not found"Repository: project-cvsa/cvsa
Length of output: 1403
🏁 Script executed:
# Look for any scripts directory or tooling that might call reset
fd -t f -e ts -e js -e sh "script" packages/db 2>/dev/null | head -20Repository: project-cvsa/cvsa
Length of output: 43
🏁 Script executed:
# Check for any GitHub Actions or automation in root
fd -t f -e yml -e yaml | head -30Repository: project-cvsa/cvsa
Length of output: 79
Add environment safeguard to reset script to prevent accidental production data loss.
The reset script runs prisma migrate reset without environment protection. This destructive operation will drop and recreate the database schema, making it dangerous if run against a production database URL. The project already establishes the pattern of using NODE_ENV guards for similar operations (see test:setup). Apply the same protection here:
- "reset": "bunx --bun prisma migrate reset"
+ "reset": "NODE_ENV=development bunx --bun prisma migrate reset"Alternatively, gate to development/test environments with explicit rejection:
- "reset": "bunx --bun prisma migrate reset"
+ "reset": "sh -c '[ \"$NODE_ENV\" = \"development\" ] || [ \"$NODE_ENV\" = \"test\" ] || { echo \"Refusing reset outside development/test\"; exit 1; }; bunx --bun prisma migrate reset --force'"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "reset": "bunx --bun prisma migrate reset" | |
| "reset": "NODE_ENV=development bunx --bun prisma migrate reset" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/db/package.json` at line 14, The "reset" npm script currently runs
"prisma migrate reset" unguarded; change the "reset" script in package.json to
enforce an environment safeguard by wrapping the command in a small node check
(or reuse the project pattern from "test:setup") that aborts if NODE_ENV ===
"production" (or only allows explicit NODE_ENV values like "development" or
"test"), or require an explicit override env var (e.g., FORCE_DB_RESET=true)
before executing prisma migrate reset; update the script name "reset"
accordingly and ensure the guard emits a clear error and exits non-zero when the
check fails.
alikia2x
changed the title
alikia2x
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/core/tests/unit/SongService.test.ts (1)
131-136:⚠️ Potential issue | 🟠 MajorAwait
.rejectsassertions and avoid re-invoking the same failing call.At Line 131-133 and Line 166-168, the rejection assertions are not awaited, so these tests can pass without actually validating failures.
🐛 Proposed fix
test("throws NOT_FOUND error when song does not exist", async () => { mockRepository.getById.mockResolvedValueOnce(null); - expect(songService.update(999, updateInput)).rejects.toThrow(AppError); - expect(songService.update(999, updateInput)).rejects.toThrow("error.song.notfound"); - expect(songService.update(999, updateInput)).rejects.toMatchObject({ + const updatePromise = songService.update(999, updateInput); + await expect(updatePromise).rejects.toThrow(AppError); + await expect(updatePromise).rejects.toThrow("error.song.notfound"); + await expect(updatePromise).rejects.toMatchObject({ code: "NOT_FOUND", statusCode: 404, }); }); ... test("throws NOT_FOUND error when song does not exist", async () => { mockRepository.getById.mockResolvedValueOnce(null); - expect(songService.delete(999)).rejects.toThrow(AppError); - expect(songService.delete(999)).rejects.toThrow("error.song.notfound"); - expect(songService.delete(999)).rejects.toMatchObject({ + const deletePromise = songService.delete(999); + await expect(deletePromise).rejects.toThrow(AppError); + await expect(deletePromise).rejects.toThrow("error.song.notfound"); + await expect(deletePromise).rejects.toMatchObject({ code: "NOT_FOUND", statusCode: 404, }); });#!/bin/bash # Find un-awaited rejects assertions in test files rg -nP --type=ts '^\s*expect\([^\n]*\)\.rejects\.' packages/core/testsAlso applies to: 166-171
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/tests/unit/SongService.test.ts` around lines 131 - 136, The failing assertions call songService.update(999, updateInput) multiple times without awaiting the .rejects chains; change to create a single promise (e.g. const promise = songService.update(999, updateInput)) and then await each assertion: await expect(promise).rejects.toThrow(AppError); await expect(promise).rejects.toThrow("error.song.notfound"); await expect(promise).rejects.toMatchObject({ code: "NOT_FOUND", statusCode: 404 }); this ensures the rejection is awaited and the same invocation is asserted against (references: SongService.test.ts, songService.update, updateInput).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/tests/unit/SongService.test.ts`:
- Around line 86-95: The tests claim search.sync is called but never assert it;
update the affected test cases (e.g., the "creates song and calls search.sync"
test that calls songService.create and other similar blocks) to include an
explicit assertion that mockSearchService.sync was called (for example using
toHaveBeenCalled or toHaveBeenCalledWith as appropriate) after the existing
expect(mockRepository.create) assertions so the tests verify that
mockSearchService.sync is invoked by songService.create.
- Line 82: Replace the inline assertion `type: "ORIGINAL" as const` in
SongService.test.ts with an explicit type annotation on the containing
object/variable: remove the `as const` and annotate the object (or the variable
holding it) with the appropriate type or enum used by the code under test (e.g.,
SongType/TrackType or the specific interface used in the test), so the `type`
property is typed via the object's declared type instead of an inline `as const`
assertion.
---
Duplicate comments:
In `@packages/core/tests/unit/SongService.test.ts`:
- Around line 131-136: The failing assertions call songService.update(999,
updateInput) multiple times without awaiting the .rejects chains; change to
create a single promise (e.g. const promise = songService.update(999,
updateInput)) and then await each assertion: await
expect(promise).rejects.toThrow(AppError); await
expect(promise).rejects.toThrow("error.song.notfound"); await
expect(promise).rejects.toMatchObject({ code: "NOT_FOUND", statusCode: 404 });
this ensures the rejection is awaited and the same invocation is asserted
against (references: SongService.test.ts, songService.update, updateInput).
🪄 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 (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 6595474f-f12c-4615-960f-ad5d9662b2e4
📒 Files selected for processing (4)
apps/backend/src/errorHandler.tspackages/core/src/search/catalog/song.tspackages/core/tests/integration/SongRepository.test.tspackages/core/tests/unit/SongService.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/backend/src/errorHandler.ts
- packages/core/tests/integration/SongRepository.test.ts
- packages/core/src/search/catalog/song.ts
| describe("create", () => { | ||
| const createInput = { | ||
| name: "New Song", | ||
| type: "ORIGINAL" as const, |
There was a problem hiding this comment.
Avoid inline type assertions in test input objects.
At Line 82, type: "ORIGINAL" as const violates the no-inline-assertion rule. Prefer an explicit object type annotation.
♻️ Proposed fix
- const createInput = {
+ const createInput: { name: string; type: "ORIGINAL"; duration: number } = {
name: "New Song",
- type: "ORIGINAL" as const,
+ type: "ORIGINAL",
duration: 200,
};As per coding guidelines, **/*.{ts,tsx}: Never use inline type assertions with as SomeType.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| type: "ORIGINAL" as const, | |
| const createInput: { name: string; type: "ORIGINAL"; duration: number } = { | |
| name: "New Song", | |
| type: "ORIGINAL", | |
| duration: 200, | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/tests/unit/SongService.test.ts` at line 82, Replace the inline
assertion `type: "ORIGINAL" as const` in SongService.test.ts with an explicit
type annotation on the containing object/variable: remove the `as const` and
annotate the object (or the variable holding it) with the appropriate type or
enum used by the code under test (e.g., SongType/TrackType or the specific
interface used in the test), so the `type` property is typed via the object's
declared type instead of an inline `as const` assertion.
Comment on lines
+86
to
+95
| test("creates song and calls search.sync", async () => { | ||
| const result = await songService.create(createInput); | ||
|
|
||
| expect(result).toMatchObject({ | ||
| name: "Test Song", | ||
| type: "ORIGINAL", | ||
| duration: 180, | ||
| }); | ||
| expect(mockRepository.create).toHaveBeenCalledWith(createInput); | ||
| }); |
There was a problem hiding this comment.
Tests state search.sync is called, but they never assert it.
At Line 86, 116, and 156 (and related blocks), the test names claim sync invocation, but assertions only check repository calls. Please assert mockSearchService.sync explicitly to match intent and prevent silent regressions.
Also applies to: 97-110, 116-126, 139-152, 156-161, 174-182
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/tests/unit/SongService.test.ts` around lines 86 - 95, The tests
claim search.sync is called but never assert it; update the affected test cases
(e.g., the "creates song and calls search.sync" test that calls
songService.create and other similar blocks) to include an explicit assertion
that mockSearchService.sync was called (for example using toHaveBeenCalled or
toHaveBeenCalledWith as appropriate) after the existing
expect(mockRepository.create) assertions so the tests verify that
mockSearchService.sync is invoked by songService.create.
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 of Changes
Test Infrastructure & Coverage
apps/backend/tests/auth.test.tsandapps/backend/tests/song.test.ts:beforeEachhooks to suite-levelbeforeAllhooksawait prisma.$connect()in setupGET /v2/song/:id/details) inapps/backend/tests/song.test.tsIntegration & Unit Tests
packages/core/tests/integration/SongRepository.test.tscoveringgetDetailsById()methodpackages/core/tests/integration/SongSearchService.test.tsfor search functionality with MeiliSearchpackages/core/tests/unit/SearchManager.test.tsfor SearchManager behavior with mocked clientspackages/core/tests/unit/SongSearchService.test.tscovering sync and search operationspackages/core/tests/unit/SongService.test.tswith new test suites for create, update, and delete operationsOriginalMeiliSearchexport topackages/core/tests/utils.tsSearch Manager Refactoring
SearchManagerinpackages/core/src/search/manager.ts:initializeClients()andensureInitialized()methodsgetAdminIndex()andgetSearchIndex()async methodsappLoggerand changed utility import sourcesConfiguration & Build
prisma/ignore pattern from.tokeignoreapps/backend/bunfig.tomlandpackages/core/bunfig.tomlwith patterns to exclude specific directories and file types from coverage reportingpackages/core/package.jsontest scripts to run concurrently (added--concurrentflag)packages/db/package.jsonto add a newresetscript for Prisma migration resetSong Management & Search
CreateSongRequestSchemainpackages/core/src/modules/catalog/song/dto.ts:language,localizedNames,localizedDescriptionsSongRepository.create()to persist language and localized metadata fieldsSongServiceto useappLoggerinstead ofconsole.errorand refactored search index synchronization to use asynctraceTaskpackages/core/src/search/catalog/song.tswith language-aware getName() logic and added task synchronization in sync operationspackages/core/src/modules/catalog/song/container.tsfor search manager access