Fix media database API degradation#153
Merged
Merged
Conversation
|
Warning Review limit reached
More reviews will be available in 1 minute and 49 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughCentralize media-related CoreAPI error classification/logging, add a mediaGenerate feature gate and DEFAULT_GAMES_INDEX, and update ConnectionProvider, Search, MediaDatabaseCard, translations, and tests to gracefully handle unsupported Core or missing media-database setup. ChangesMedia database error handling and feature gating
Sequence Diagram(s)(suppressed — changes are primarily error classification, UI gating, and tests; no new multi-component sequential workflow requiring visualization) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
src/routes/-pages/Search.tsx (1)
13-13: ⚡ Quick winMake the
coreApiimport consistent (optional)
Search.tsximports@/lib/coreApi.ts, but this repo already imports@/lib/coreApi.tsin other places andtsconfig.jsonsets"allowImportingTsExtensions": true, so the extension itself isn’t inherently invalid. For consistency withsrc/components/ConnectionProvider.tsx, you can drop the.tsextension.🔧 Proposed fix
-import { CoreAPI, isExpectedMediaDatabaseError } from "`@/lib/coreApi.ts`"; +import { CoreAPI, isExpectedMediaDatabaseError } from "`@/lib/coreApi`";🤖 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 `@src/routes/-pages/Search.tsx` at line 13, The import in Search.tsx uses the .ts extension which is inconsistent with other imports (e.g., ConnectionProvider.tsx); update the import statement that currently reads "`@/lib/coreApi.ts`" to "`@/lib/coreApi`" so it matches the repo style and tsconfig setting, and run a quick search for any other imports with the .ts extension of coreApi to normalize them as well.src/components/ConnectionProvider.tsx (1)
597-608: ⚡ Quick winConfirm reset semantics for
totalMedia, and dedupe the reset literal
IndexResponse.totalMediais optional, andsetGamesIndexreplaces the entiregamesIndexobject (set({ gamesIndex: index })), so omittingtotalMediain this reset won’t leave staletotalMedia—it becomesundefined.- The same
setGamesIndex({ exists: false, ... totalFiles: 0 })reset literal is duplicated insrc/components/ConnectionProvider.tsxandsrc/routes/-pages/Search.tsx(no corresponding reset literal found insrc/components/MediaDatabaseCard.tsx). Extracting a shared constant/helper would keep them in sync.🤖 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 `@src/components/ConnectionProvider.tsx` around lines 597 - 608, The reset block in setGamesIndex (used in ConnectionProvider and duplicated in Search) should be deduplicated into a shared constant/helper (e.g., DEFAULT_GAMES_INDEX or buildDefaultGamesIndex()) and imported/used from both places; also make the reset explicit about IndexResponse.totalMedia semantics by either including totalMedia: undefined (to document that it is intentionally cleared) or setting a specific numeric default if you want 0, and update the setGamesIndex(...) call in the isExpectedMediaDatabaseError handler to use that shared constant instead of the duplicated literal.src/lib/coreApi.ts (1)
101-117: 💤 Low valueConsider adding structured metadata to warning logs.
While expected errors are correctly downgraded to
logger.warn, the warning call doesn't include structured metadata (category,action). Adding this context would improve filtering and debugging, even for expected failures.♻️ Enhanced warning with metadata
function logMediaApiFailure( label: string, action: string, error: unknown, severity: "error" | "warning" = "error", ): void { if (isExpectedMediaDatabaseError(error)) { - logger.warn(`${label}:`, error); + logger.warn(`${label}:`, error, { + category: "api", + action, + }); return; } logger.error(`${label}:`, error, { category: "api", action, severity, }); }🤖 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 `@src/lib/coreApi.ts` around lines 101 - 117, The warning branch in logMediaApiFailure currently calls logger.warn without structured metadata; update the branch that checks isExpectedMediaDatabaseError(error) so it calls logger.warn with the same metadata keys used by logger.error (include category: "api" and action and severity) to ensure warnings carry the same context for filtering and debugging; keep the existing message and error object and set severity to "warning".src/components/MediaDatabaseCard.tsx (1)
144-195: 💤 Low value
generateErroris set on cancel/resume failure but never cleared on success.
handleUpdateDatabaseclearsgenerateError(Line 114) before retrying, buthandleCancelUpdate/handleResumeUpdateonly set it. A prior inline error can linger after a subsequent successful cancel/resume. Low likelihood since these handlers only run during indexing, but clearing on entry keeps the inline message consistent.♻️ Optional: clear stale error on cancel/resume entry
const handleCancelUpdate = async () => { setCancelRequested(true); + setGenerateError(null); try {const handleResumeUpdate = async () => { setResumeRequested(true); + setGenerateError(null); try {🤖 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 `@src/components/MediaDatabaseCard.tsx` around lines 144 - 195, The handlers handleCancelUpdate and handleResumeUpdate currently set generateError on failures but never clear it on entry, allowing stale error messages to persist after a later successful action; modify both functions to clear the generateError state at the start (e.g., call setGenerateError(null) or the appropriate empty value) before setting setCancelRequested/setResumeRequested and performing the API call, so any previous error is removed when a new cancel/resume is attempted.
🤖 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 `@src/__tests__/unit/components/ConnectionProvider.test.tsx`:
- Around line 86-89: The test mock for isExpectedMediaDatabaseError is doing
case-sensitive matching; update the predicate (isExpectedMediaDatabaseError) to
first assert the error is an Error and then convert error.message to lowercase
(e.g. const msg = (error as Error).message.toLowerCase()) before checking
msg.includes("no such table: dbconfig") || msg.includes("method not found") so
it mirrors the production case-insensitive logic used in coreApi.
In `@src/__tests__/unit/components/MediaDatabaseCard.test.tsx`:
- Around line 17-22: The test mocks for isMissingMediaDatabaseSetupError and
isExpectedMediaDatabaseError perform case-sensitive string checks; update them
to mirror production by normalizing the error message to lowercase before
matching (e.g., cast error to Error, compute const msg = (error instanceof Error
&& error.message) ? error.message.toLowerCase() : ""; then use msg.includes("no
such table: dbconfig") and msg.includes("method not found") in
isMissingMediaDatabaseSetupError and isExpectedMediaDatabaseError respectively)
so the predicates behave case-insensitively like the production implementation.
In `@src/__tests__/unit/routes/create.search.test.tsx`:
- Around line 53-56: The test mock for isExpectedMediaDatabaseError is doing
case-sensitive string matching; update it to normalize the error message to
lowercase (or use case-insensitive matching) before checking substrings so it
mirrors the production implementation in src/lib/coreApi.ts (e.g., call
.toString().toLowerCase() on the error or error.message and then check for "no
such table: dbconfig" and "method not found"); ensure the predicate still
accepts unknown errors but performs case-insensitive substring checks as in
production.
In `@src/lib/coreApi.ts`:
- Around line 78-85: In isUnsupportedMediaApiError, remove the redundant
instanceof CoreApiError branch (the check "error instanceof CoreApiError &&
message.includes('method not found')") since message.includes('method not
found') already covers it; then verify and correct the literal "query or system
is required" used in the predicate (or, better, switch to checking a stable
error code/property on CoreApiError if available) so that
getErrorMessage(error).toLowerCase() is compared against the exact
backend/contract message or a canonical error code instead of an unverified
substring.
---
Nitpick comments:
In `@src/components/ConnectionProvider.tsx`:
- Around line 597-608: The reset block in setGamesIndex (used in
ConnectionProvider and duplicated in Search) should be deduplicated into a
shared constant/helper (e.g., DEFAULT_GAMES_INDEX or buildDefaultGamesIndex())
and imported/used from both places; also make the reset explicit about
IndexResponse.totalMedia semantics by either including totalMedia: undefined (to
document that it is intentionally cleared) or setting a specific numeric default
if you want 0, and update the setGamesIndex(...) call in the
isExpectedMediaDatabaseError handler to use that shared constant instead of the
duplicated literal.
In `@src/components/MediaDatabaseCard.tsx`:
- Around line 144-195: The handlers handleCancelUpdate and handleResumeUpdate
currently set generateError on failures but never clear it on entry, allowing
stale error messages to persist after a later successful action; modify both
functions to clear the generateError state at the start (e.g., call
setGenerateError(null) or the appropriate empty value) before setting
setCancelRequested/setResumeRequested and performing the API call, so any
previous error is removed when a new cancel/resume is attempted.
In `@src/lib/coreApi.ts`:
- Around line 101-117: The warning branch in logMediaApiFailure currently calls
logger.warn without structured metadata; update the branch that checks
isExpectedMediaDatabaseError(error) so it calls logger.warn with the same
metadata keys used by logger.error (include category: "api" and action and
severity) to ensure warnings carry the same context for filtering and debugging;
keep the existing message and error object and set severity to "warning".
In `@src/routes/-pages/Search.tsx`:
- Line 13: The import in Search.tsx uses the .ts extension which is inconsistent
with other imports (e.g., ConnectionProvider.tsx); update the import statement
that currently reads "`@/lib/coreApi.ts`" to "`@/lib/coreApi`" so it matches the
repo style and tsconfig setting, and run a quick search for any other imports
with the .ts extension of coreApi to normalize them as well.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 12008a4a-2925-4543-80b7-409667c5bbcb
📒 Files selected for processing (10)
src/__tests__/unit/components/ConnectionProvider.test.tsxsrc/__tests__/unit/components/MediaDatabaseCard.test.tsxsrc/__tests__/unit/coreApi.api-contract.test.tssrc/__tests__/unit/routes/create.search.test.tsxsrc/components/ConnectionProvider.tsxsrc/components/MediaDatabaseCard.tsxsrc/lib/coreApi.tssrc/lib/featureGates.tssrc/routes/-pages/Search.tsxsrc/translations/en-US.json
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
Closes #143
Validated with targeted tests, format check, typecheck, and lint.
Summary by CodeRabbit
New Features
Bug Fixes
UI Updates
Tests