packages/types/src/storygraph.ts (1)

49-56: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider narrowing matchMethod/matchError to string unions.

matchMethod and matchError are typed as loose string | null. Since these values are almost certainly produced from a fixed set of internal match strategies/error codes (e.g. 'isbn' | 'title_author' | 'manual'), a string-literal union would give consumers (client components rendering match status) better type safety and autocomplete.

🤖 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/types/src/storygraph.ts` around lines 49 - 56,
`StorygraphLinkedBook` currently exposes `matchMethod` and `matchError` as broad
`string | null`, which weakens type safety for consumers. Narrow these fields in
the `StorygraphLinkedBook` interface to string-literal unions that reflect the
fixed internal match strategies/error codes used by the matching logic, and
update any related types/usages so components consuming this model get accurate
autocomplete and compile-time validation.

server/src/db/schema/storygraph.ts (1)

28-52: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Consider a standalone index on book_id for cascade-delete performance.

The only index covering book_id is the composite (user_id, book_id) unique constraint, with user_id as the leading column. Deleting a row from books (ON DELETE CASCADE) will need to locate matching storygraph_book_state rows by book_id alone, which this composite index cannot efficiently serve.

♻️ Suggested addition

   (t) => [unique('storygraph_book_state_user_book_uidx').on(t.userId, t.bookId)],
+  // plus a separate index() on t.bookId for efficient cascade deletes

🤖 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 `@server/src/db/schema/storygraph.ts` around lines 28 - 52, Add a standalone
index on bookId in storygraphBookState to support efficient ON DELETE CASCADE
lookups from books. The existing unique constraint
storygraph_book_state_user_book_uidx only helps when userId is part of the
filter, so update the pgTable definition in storygraphBookState to include a
separate index on the bookId column alongside the current unique constraint.

server/src/modules/storygraph/storygraph-queue.service.test.ts (1)

26-48: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider a concurrent-call test and stronger timing assertions.

None of these tests invoke throttle concurrently for the same userId (e.g. via Promise.all), so the race condition in the underlying implementation (see comment on storygraph-queue.service.ts) wouldn't be caught by this suite. Also, "should throttle within the interval" and "should not wait when interval has passed" don't assert the actual wait duration (e.g. via setTimeout spy args), only that the call resolves.

🤖 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 `@server/src/modules/storygraph/storygraph-queue.service.test.ts` around lines
26 - 48, The current StorygraphQueueService tests only verify that throttle
resolves, so they miss same-user concurrency races and do not validate the
actual delay. Update the throttle specs in StorygraphQueueService tests to
include a concurrent same-user case using Promise.all on throttle for the same
userId, and add stronger timing assertions by spying on setTimeout (or
equivalent timer usage) to assert the expected wait duration in the throttled
path and no wait once STORYGRAPH_MIN_INTERVAL_MS has elapsed.

server/src/modules/storygraph/storygraph-client.service.ts (1)

79-91: 🩺 Stability & Availability | 🔵 Trivial | ⚖️ Poor tradeoff

Network-level errors aren't retried, only HTTP status failures are.

A thrown fetch error (DNS failure, connection reset, etc.) is rethrown immediately with no retry, while 429/5xx responses do get the retry/backoff treatment. For an external, occasionally-flaky scraping target, retrying transient network errors the same way would improve resiliency.

🤖 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 `@server/src/modules/storygraph/storygraph-client.service.ts` around lines 79 -
91, The retry logic in executeWithRetry only handles HTTP 429/5xx responses,
while thrown fetch/network errors are logged and rethrown immediately. Update
the catch block to treat transient network failures like other retryable cases:
log the error, then if attempt is still below STORYGRAPH_MAX_RETRIES, apply the
same exponential backoff and recurse to executeWithRetry instead of throwing
right away. Keep the existing errorClass/error sanitization and use the same
retry path as the response.status branch.

server/src/modules/storygraph/storygraph-book-match.service.ts (1)

38-76: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Fallback matching attempts continue after a likely session failure.

If searchForBook fails due to redirectedToSignIn (invalid/expired cookies) on the ISBN13 attempt, matchBook still proceeds to try ISBN10 and title search — each of which will fail identically since the session is invalid, wasting throttled requests to the external site for every book being synced.

Consider short-circuiting (e.g., returning a distinct "invalid session" signal) so callers can abort the whole sync run early rather than retrying with different search terms.

🤖 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 `@server/src/modules/storygraph/storygraph-book-match.service.ts` around lines
38 - 76, The fallback flow in matchBook continues calling searchForBook after a
redirectedToSignIn failure, which wastes requests when cookies are invalid.
Update matchBook and the searchForBook result handling so a sign-in redirect
returns a distinct invalid-session signal that short-circuits the ISBN10/title
fallbacks. Ensure callers can detect this condition and abort the sync run early
instead of continuing retries.

server/src/modules/storygraph/dto/storygraph.dto.ts (1)

39-51: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

No format constraint on input/editionId beyond length.

LinkStorygraphBookDto.input accepts any non-empty string up to 2048 chars, which is then interpolated directly into a StoryGraph request path in StorygraphBookMatchService.resolveManualInput/extractBookIdFromInput without further validation or encoding (see server/src/modules/storygraph/storygraph-book-match.service.ts lines 171-195). Consider adding a pattern/regex constraint here (e.g., restrict to safe URL/id characters) as defense-in-depth, since this is the entry point for user-supplied StoryGraph identifiers.

🤖 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 `@server/src/modules/storygraph/dto/storygraph.dto.ts` around lines 39 - 51,
The StoryGraph DTOs currently only enforce non-empty length on
LinkStorygraphBookDto.input and SetStorygraphEditionDto.editionId, leaving
user-supplied identifiers too permissive before they reach
StorygraphBookMatchService.resolveManualInput and extractBookIdFromInput.
Tighten validation in storygraph.dto.ts by adding a pattern/regex constraint to
both fields to allow only safe StoryGraph URL/id characters, keeping the
existing length limits, so malformed or unexpected inputs are rejected at the
boundary.

server/src/modules/storygraph/storygraph-settings.service.test.ts (1)

126-181: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Missing regression test for empty-string cookie overwrite on update.

Given the empty-string overwrite bug flagged in storygraph-settings.service.ts (lines 43-55), consider adding a test case where upsertSettings is called on an existing row with sessionCookie: '' (or whitespace) and asserting it either throws or preserves the existing cookie.

🤖 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 `@server/src/modules/storygraph/storygraph-settings.service.test.ts` around
lines 126 - 181, The upsertSettings tests do not cover the empty-string cookie
overwrite regression in Storygraph settings updates. Add a case in
storygraph-settings.service.test.ts for makeService().upsertSettings when an
existing row is present and sessionCookie is passed as an empty string or
whitespace, and assert the behavior matches the intended fix in upsertSettings
(either reject it with BadRequestException or preserve the existing stored
cookie). Use the existing upsertSettings and
mockRepo.findSettings/mockRepo.upsertSettings setup to verify the cookie field
is not overwritten incorrectly.

server/src/modules/storygraph/storygraph-sync.service.test.ts (1)

1-505: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Solid coverage overall; consider adding regression tests for the two issues raised on the service file.

Neither the response.status vs retry.status mismatch in updateStatus's retry-fallback path nor the cancelSync-then-immediate-syncAll race is currently exercised. A test asserting the thrown error message uses the retry's status when the retry also fails, and a test simulating cancelSync() followed immediately by a second syncAll() call (verifying only one runSyncAll proceeds), would guard these paths going forward.

🤖 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 `@server/src/modules/storygraph/storygraph-sync.service.test.ts` around lines 1
- 505, The StorygraphSyncService test suite is missing regression coverage for
the retry-fallback status handling in updateStatus and the cancelSync/syncAll
race condition. Add a test around updateStatus that forces both the initial
status update and the retry to fail, and assert the thrown error
message/reporting uses the retry response’s status from the retry path rather
than the original response. Also add a syncAll/cancelSync race test on
StorygraphSyncService that calls cancelSync() and then immediately triggers
syncAll() again, verifying only one runSyncAll execution proceeds and the
cancelled run does not continue.

server/src/modules/storygraph/storygraph.controller.test.ts (1)

13-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Missing test coverage for cancelSync.

mockSyncService.cancelSync is defined but never exercised by a test, even though the sync orchestration layer description explicitly calls out cancellation support. Consider adding a delegation test for the controller's cancel-sync endpoint for parity with the other mocked methods.

Also applies to: 32-122

🤖 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 `@server/src/modules/storygraph/storygraph.controller.test.ts` around lines 13
- 24, The controller test suite defines mockSyncService.cancelSync but never
verifies it is used, leaving the cancel-sync path untested. Add a delegation
test in storygraph.controller.test.ts for the StorygraphController cancel-sync
endpoint that asserts the controller calls mockSyncService.cancelSync with the
expected arguments, matching the existing coverage pattern used for syncAll,
getSyncStatus, and the other service methods.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

server/src/modules/storygraph/storygraph-queue.service.test.ts (1)

14-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Missing vi.restoreAllMocks() in top-level afterEach.

Two separate tests (lines 19 and 60) call vi.spyOn(global, 'setTimeout') but the shared afterEach (lines 14-16) only restores real timers, never restores mocks/spies. Since these tests run sequentially against the same global.setTimeout, the spy created in the first test is never torn down before the second one re-spies the same global, which can cause nested/stale mock wrapping and flaky call-count assertions depending on how Vitest's spyOn handles an already-spied target interacting with useFakeTimers/useRealTimers transitions.

🧪 Suggested fix

   afterEach(() => {
     vi.useRealTimers();
+    vi.restoreAllMocks();
   });

Also applies to: 18-24, 58-71

🤖 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 `@server/src/modules/storygraph/storygraph-queue.service.test.ts` around lines
14 - 16, The shared cleanup in storygraph-queue.service.test.ts only switches
timers back, but it leaves the global setTimeout spies active across tests.
Update the top-level afterEach to also call vi.restoreAllMocks alongside
vi.useRealTimers so the vi.spyOn(global, 'setTimeout') calls in the affected
tests are fully torn down before the next test runs. Keep the fix centralized in
the existing afterEach near the test setup so the storygraph queue tests don’t
re-spy on a mocked global.

client/src/features/storygraph/composables/useStorygraphBookSyncState.ts (1)

109-125: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Client-side duplication of the server's toggle-override logic.

The syncOverride/effectiveReason/canSyncNow derivation here mirrors resolveStorygraphBookSyncOverrideForToggle in server/src/modules/storygraph/storygraph-sync-policy.ts. Keeping this pure branching logic in one shared, importable location (e.g., @bookorbit/types or a shared utils package) would remove the risk of the two copies drifting apart as the sync policy evolves.

🤖 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 `@client/src/features/storygraph/composables/useStorygraphBookSyncState.ts`
around lines 109 - 125, The toggle-state derivation in setSyncEnabled is
duplicating the server sync policy logic, so move the
syncOverride/effectiveReason/canSyncNow branching into a shared importable
helper instead of keeping a client-only copy. Reuse the existing
resolveStorygraphBookSyncOverrideForToggle behavior from storygraph-sync-policy
in a shared location such as `@bookorbit/types` or a shared utils package, then
have useStorygraphBookSyncState call that helper when building state.value. Keep
the client and server paths aligned by making the composable depend on the
shared symbol rather than inline branching.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes