fix: make blocked search setup actionable - #596
Conversation
🦋 Changeset detectedLatest commit: ef8f7cc The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Warning Review limit reached
Next review available in: 34 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughThe change adds editable NZB and Newznab settings, transactional provider persistence, credential sanitization, route-specific health diagnostics, and actionable frontend guidance for search and download configuration failures. ChangesProvider configuration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant SettingsPage
participant SearchTab
participant useConfig
participant system_router
participant system_service
participant Database
SettingsPage->>SearchTab: Open search settings
SearchTab->>useConfig: Fetch provider configuration
useConfig->>system_router: GET /api/config/providers
system_router->>system_service: Get sanitized providers
system_service-->>useConfig: Provider identities and key status
SearchTab->>useConfig: Save enabled state and providers
useConfig->>system_router: Update provider configuration
system_router->>system_service: Validate provider payload
system_service->>Database: Persist providers and enablement
Database-->>system_service: Transaction result
system_service-->>SearchTab: Sanitized provider data
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
frankieramirez
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: 4
🧹 Nitpick comments (5)
comicarr/app/config/registry.py (1)
372-387: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider validating the
NZB_DOWNLOADERenum on write.
NZB_DOWNLOADERis now writable through the settings API.update_configfilters keys but does not check the value range.process_kwargscoerces any digit string toint, so a payload such as{"nzb_downloader": 9}persists an unmapped value.configure()then leavesUSE_SABNZBD,USE_NZBGET, andUSE_BLACKHOLEallFalse, andget_safe_configreports the label"None". A server-side range check keeps the stored enum inside 0-3.🤖 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 `@comicarr/app/config/registry.py` around lines 372 - 387, Add server-side validation for NZB_DOWNLOADER in the update_config/process_kwargs write path, allowing only integer values from 0 through 3 before persisting the setting. Reject out-of-range values such as 9 while preserving valid updates and the existing ConfigKey behavior.tests/unit/test_system_domain.py (1)
811-833: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe
api_key_setassertion does not prove post-persistence state.
apply_transactionis mocked, soctx.config.EXTRA_NEWZNABSstill holds the pre-update tuple that contains"secret"._safe_provider_projectionreads that same attribute, so line 833 passes even if the service dropped the credential. The persisted-value assertion on line 831 is the one that carries the guarantee.Set
ctx.config.EXTRA_NEWZNABSfrom the capturedapply_transactionargument before asserting the projection, or drop line 833.Add a case where the row omits
idand the stored host contains userinfo. That path exercises theby_identityfallback flagged incomicarr/app/system/service.py.🤖 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 `@tests/unit/test_system_domain.py` around lines 811 - 833, Update test_update_providers_accepts_safe_objects_and_preserves_blank_existing_key so the mocked configuration reflects the captured EXTRA_NEWZNABS transaction before asserting result["providers"][0]["api_key_set"], or remove that assertion. Add a test case with a provider row lacking id and a stored host containing userinfo to exercise the by_identity fallback in update_providers.frontend/tests/pages/SettingsPage.test.ts (1)
121-126: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRestore the global stub in a teardown hook.
vi.unstubAllGlobals()runs inline at line 126. If any assertion between lines 122 and 125 fails, the test aborts and theconfirmstub leaks into the remaining tests in this file.Register the restore in
afterEachso it always runs.♻️ Proposed teardown hook
describe("settings configuration", () => { + afterEach(() => { + vi.unstubAllGlobals(); + }); +Also import
afterEachfromvitestand remove the inline call at line 126.🤖 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 `@frontend/tests/pages/SettingsPage.test.ts` around lines 121 - 126, Import afterEach from vitest and register vi.unstubAllGlobals() in an afterEach teardown for SettingsPage tests. Remove the inline teardown call from the test containing the confirm stub, while preserving the existing assertions and stub setup.frontend/src/pages/SettingsPage.tsx (2)
59-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
httpOriginis defined twice with different signatures. Both files implement the same HTTP/HTTPS origin parsing, and both use it to decide whether a stored API key must be re-entered. The signatures already differ, so the two credential-rebinding checks can drift apart.
frontend/src/pages/SettingsPage.tsx#L59-L68: move this implementation into a shared module underfrontend/src/lib/and import it here.frontend/src/components/settings/SearchTab.tsx#L32-L41: delete the local copy and import the shared helper.As per coding guidelines: "Place frontend pages and components under
frontend/src/, and put API-client code infrontend/src/lib/."🤖 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 `@frontend/src/pages/SettingsPage.tsx` around lines 59 - 68, Move the shared httpOrigin implementation from frontend/src/pages/SettingsPage.tsx:59-68 into a module under frontend/src/lib/, then import and use it in SettingsPage.tsx. Remove the duplicate local implementation from frontend/src/components/settings/SearchTab.tsx:32-41 and import the shared helper there, preserving both credential-rebinding checks.Source: Coding guidelines
91-98: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winBlock SPA navigation when indexer changes are dirty.
providerDirtyonly checks section links already rendered in Settings, andbeforeunloadonly covers full unload. A/settingsroute change fromAppSidebaroruseNavigate("/settings")can remove the component before the indexed form is saved, causing edits to be discarded without a prompt. Add React Router v7 block/confirm behavior whileproviderDirtyis true.🤖 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 `@frontend/src/pages/SettingsPage.tsx` around lines 91 - 98, Update the SettingsPage navigation guard around providerDirty to use React Router v7’s blocker/confirmation behavior, so any SPA navigation that would unmount the settings form is blocked while unsaved indexer changes exist. Preserve the existing section-link prompt, allow navigation after confirmation, and ensure non-dirty navigation remains unaffected.
🤖 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 `@comicarr/app/system/service.py`:
- Around line 483-509: Update the by_identity construction in the object_payload
provider normalization flow to key legacy entries by the sanitized host produced
by _safe_provider_host, while retaining the stored name and existing six-field
eligibility. Ensure the lookup using row.get("name") and row.get("host") matches
the client projection so credential preservation works when no id is present.
In `@comicarr/search.py`:
- Around line 1343-1362: Prefix each changed backend log message with its
required stable bracketed context: in comicarr/search.py lines 1343-1362, update
the timeout, connection-error, and request-error logs to use [NZB-SEARCH]; in
comicarr/sabnzbd.py lines 83-86, prefix the sender failure with [SAB-SEND]; and
in comicarr/sabnzbd.py lines 131-133, prefix the queue-monitor failure with
[SAB-QUEUE].
In `@frontend/src/components/settings/SearchTab.tsx`:
- Around line 194-284: Update the suffix calculation in the providers.map render
block so each row’s field IDs are unique even when saved and newly added
indexers share numeric values. Use a stable per-row identifier that cannot
collide, then keep the existing suffix-based id and htmlFor construction
unchanged.
In `@frontend/src/pages/SettingsPage.tsx`:
- Around line 176-188: Update the SABnzbd validation block in the settings form
to run only when SABnzbd is the selected NZB client and sab_host has a non-empty
value. Preserve the existing URL and API-key-change checks within that scope, so
unrelated saves and empty stored hosts bypass SABnzbd validation.
---
Nitpick comments:
In `@comicarr/app/config/registry.py`:
- Around line 372-387: Add server-side validation for NZB_DOWNLOADER in the
update_config/process_kwargs write path, allowing only integer values from 0
through 3 before persisting the setting. Reject out-of-range values such as 9
while preserving valid updates and the existing ConfigKey behavior.
In `@frontend/src/pages/SettingsPage.tsx`:
- Around line 59-68: Move the shared httpOrigin implementation from
frontend/src/pages/SettingsPage.tsx:59-68 into a module under frontend/src/lib/,
then import and use it in SettingsPage.tsx. Remove the duplicate local
implementation from frontend/src/components/settings/SearchTab.tsx:32-41 and
import the shared helper there, preserving both credential-rebinding checks.
- Around line 91-98: Update the SettingsPage navigation guard around
providerDirty to use React Router v7’s blocker/confirmation behavior, so any SPA
navigation that would unmount the settings form is blocked while unsaved indexer
changes exist. Preserve the existing section-link prompt, allow navigation after
confirmation, and ensure non-dirty navigation remains unaffected.
In `@frontend/tests/pages/SettingsPage.test.ts`:
- Around line 121-126: Import afterEach from vitest and register
vi.unstubAllGlobals() in an afterEach teardown for SettingsPage tests. Remove
the inline teardown call from the test containing the confirm stub, while
preserving the existing assertions and stub setup.
In `@tests/unit/test_system_domain.py`:
- Around line 811-833: Update
test_update_providers_accepts_safe_objects_and_preserves_blank_existing_key so
the mocked configuration reflects the captured EXTRA_NEWZNABS transaction before
asserting result["providers"][0]["api_key_set"], or remove that assertion. Add a
test case with a provider row lacking id and a stored host containing userinfo
to exercise the by_identity fallback in update_providers.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c2d9a51b-a001-4a8e-b609-1e0e86f70f5c
⛔ Files ignored due to path filters (1)
frontend/src/types/config.generated.tsis excluded by!**/*.generated.*
📒 Files selected for processing (25)
comicarr/app/config/registry.pycomicarr/app/search/health.pycomicarr/app/system/router.pycomicarr/app/system/service.pycomicarr/config.pycomicarr/sabnzbd.pycomicarr/search.pyconfig.ini.samplefrontend/src/components/settings/AcquisitionHealthTab.tsxfrontend/src/components/settings/DownloadClientsTab.tsxfrontend/src/components/settings/SearchTab.tsxfrontend/src/hooks/useConfig.tsfrontend/src/lib/configSave.tsfrontend/src/lib/healthBand.tsfrontend/src/pages/SeriesDetailPage.tsxfrontend/src/pages/SettingsPage.tsxfrontend/src/types/config.tsfrontend/src/types/index.tsfrontend/tests/components/AcquisitionHealthTab.test.tsxfrontend/tests/lib/healthBand.test.tsfrontend/tests/pages/SeriesDetailPage.test.tsxfrontend/tests/pages/SettingsPage.test.tstests/unit/test_sab_addfile_handoff.pytests/unit/test_search_health.pytests/unit/test_system_domain.py
1 participant
Summary
Root cause
Issue #589 showed the NZB route as disabled even when SABnzbd itself was ready. The backend also requires an enabled Newznab provider, but the modern UI did not expose that configuration and surfaced only a generic disabled state. Users therefore could not identify or repair the actual blocker from the web interface.
Safety and reliability
Validation
uv run pytest tests/unit -v— 2,156 passednpm run test:run— 389 passeduv run npm run lint— backend/frontend lint, formatting, generated config types, and guards passedFixes #589
Summary by CodeRabbit
New Features
Bug Fixes