fix(settings): enforce maxConcurrentDownloads limit of 1–20 per PRD §6.10#53
fix(settings): enforce maxConcurrentDownloads limit of 1–20 per PRD §6.10#53
Conversation
…6.10 - Update backend validation from 1–100 to 1–20 - Update frontend input max attribute from 100 to 20 - Add tests verifying the new constraint boundaries - Update CHANGELOG.md with the fix
📝 WalkthroughWalkthroughThis PR tightens Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR correctly enforces the PRD §6.10 limit for The PR also bundles 50+ newly committed coverage HTML/JSON artifacts ( Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant UI as SettingsView (Frontend)
participant IPC as Tauri IPC
participant CMD as UpdateConfigCommand
participant VAL as validate_config()
participant STORE as ConfigStore
UI->>UI: "input max=20 enforced in DOM"
UI->>IPC: "invoke("settings_update", { patch: { maxConcurrentDownloads: N } })"
IPC->>CMD: handle_update_config(cmd)
CMD->>STORE: get_config() → current
CMD->>CMD: apply_patch(current, patch) → merged
CMD->>VAL: "validate_config(&merged)"
alt "N < 1 or N > 20"
VAL-->>CMD: Err("must be 1-20")
CMD-->>IPC: AppError::Validation
IPC-->>UI: error response
else 1 ≤ N ≤ 20
VAL-->>CMD: Ok(())
CMD->>STORE: update_config(patch)
STORE-->>CMD: updated AppConfig
CMD-->>IPC: Ok(updated)
IPC-->>UI: updated config
end
Reviews (1): Last reviewed commit: "fix(settings): enforce maxConcurrentDown..." | Re-trigger Greptile |
| const input = label.closest('div')?.parentElement?.querySelector('input'); | ||
| expect(input).toBeDefined(); | ||
| expect(input?.max).toBe('20'); |
There was a problem hiding this comment.
toBeDefined() doesn't catch a null input element
querySelector returns null (not undefined) when no element matches, so expect(null).toBeDefined() passes silently. If the DOM structure changes and the input can't be found, the first assertion still passes and the failure surface moves to the toBe('20') assertion with a confusing "undefined is not '20'" message. Use not.toBeNull() (or the recommended not.toBeFalsy()) to make the guard meaningful.
| const input = label.closest('div')?.parentElement?.querySelector('input'); | |
| expect(input).toBeDefined(); | |
| expect(input?.max).toBe('20'); | |
| expect(input).not.toBeNull(); | |
| expect(input?.max).toBe('20'); |
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (4)
coverage/layouts/Sidebar.tsx.html (1)
1-217: Consider excluding coverage reports from version control.This appears to be an auto-generated Istanbul coverage HTML report. Coverage artifacts are typically:
- Added to
.gitignoreto keep the repository clean- Generated on-demand during CI/CD and stored as build artifacts
- Published to a coverage service (Codecov, Coveralls) rather than committed
Committing these files increases repository size and creates noise in diffs.
📁 Add to .gitignore
+# Coverage reports +coverage/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@coverage/layouts/Sidebar.tsx.html` around lines 1 - 217, This file is an auto-generated Istanbul coverage HTML (contains the Sidebar.tsx report) and should not be committed; delete the coverage/layouts/Sidebar.tsx.html from the repo, add the coverage output pattern (e.g., coverage/ or **/coverage/*.html) to .gitignore, and update CI to generate and upload coverage artifacts (or publish to a service) instead of committing; look for references to "Sidebar.tsx" and the top-level "coverage" directory in the diff to locate the offending artifact.coverage/lib/index.html (1)
1-131: Avoid committing generatedcoverage/**HTML artifacts in this fix PR.These are build outputs (including run-specific timestamps) and add unrelated churn. Keep coverage reports as CI artifacts and ignore
coverage/in VCS.🧹 Suggested cleanup
# .gitignore +coverage/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@coverage/lib/index.html` around lines 1 - 131, The PR includes generated coverage HTML (e.g., coverage/lib/index.html) which should not be committed; remove these build artifacts from the branch, add coverage/ to .gitignore, and unstage/remove the files from git history in this commit (e.g., git rm --cached coverage/... and commit the removal) so only the source changes remain while CI stores coverage as artifacts.src/views/SettingsView/__tests__/Sections.test.tsx (1)
111-117: Make this constraint test resilient and cover the full1–20range.The current selector relies on DOM structure (
closest(...).parentElement?.querySelector(...)), which can break on harmless markup changes. Query by accessible role/name and assert both bounds.♻️ Suggested test update
it('should cap maxConcurrentDownloads input at 20 per PRD §6.10', () => { renderWithQuery(<DownloadsSection config={mockConfig} />); - const label = screen.getByText('Max concurrent downloads'); - const input = label.closest('div')?.parentElement?.querySelector('input'); - expect(input).toBeDefined(); - expect(input?.max).toBe('20'); + const input = screen.getByRole('spinbutton', { name: /max concurrent downloads/i }); + expect(input).toHaveAttribute('min', '1'); + expect(input).toHaveAttribute('max', '20'); });Based on learnings: Write tests for all public functions and critical paths.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/SettingsView/__tests__/Sections.test.tsx` around lines 111 - 117, Replace the fragile DOM-traversal selector with an accessible query to locate the numeric input for the DownloadsSection: use getByRole('spinbutton', { name: /Max concurrent downloads/i }) to get the input element, then assert both input.min === '1' and input.max === '20' to cover the full 1–20 constraint; optionally, add a small loop that sets values 1..20 via fireEvent.change and asserts no validation error to ensure the entire range is accepted.coverage/api/hooks.ts.html (1)
182-184: Add a test for theinvalidateKeyssuccess path.The embedded source shows Line 182–Line 184 (
options?.invalidateKeysloop) is uncovered, which is a cache-consistency path inuseTauriMutation.Based on learnings: Write tests for all public functions and critical paths.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@coverage/api/hooks.ts.html` around lines 182 - 184, Tests are missing for the invalidateKeys success path inside useTauriMutation; add a unit test that calls useTauriMutation with options.invalidateKeys set (e.g., an array of query keys), mock or stub queryClientInstance.invalidateQueries, invoke the mutation's onSuccess handler (or trigger a successful mutation) and assert that queryClientInstance.invalidateQueries was called for each key; ensure the test imports useTauriMutation and supplies a mocked QueryClient or context so the loop over options.invalidateKeys and calls to invalidateQueries are executed and verified.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/settings.json:
- Around line 2-4: The PR unintentionally adds .claude/settings.json enabling
"superpowers@claude-plugins-official" which is a tooling/security change
unrelated to the maxConcurrentDownloads fix; either remove that file from the
changeset (revert/remove .claude/settings.json or exclude it from the commit) or
add explicit documented approval and rationale in the commit message and PR
description plus a brief security review note; reference the exact JSON key
"enabledPlugins" and the plugin identifier "superpowers@claude-plugins-official"
when making the change so reviewers can verify the removal or the provided
approval.
In `@AGENT.md`:
- Line 1: AGENT.md currently contains only the placeholder text "CLAUDE.md" —
replace it with actionable agent documentation or remove it; update AGENT.md to
either (a) provide real instructions for using or running the agent (purpose,
setup steps, configuration keys, example commands, and expected behavior) or (b)
convert the placeholder into a clear markdown link and short explanation (e.g.,
"See CLAUDE.md for X, or follow these steps..."); ensure the file includes a
meaningful header/title and a concise commit message describing the change when
committing (use AGENT.md and the referenced "CLAUDE.md" string to locate the
placeholder).
In `@coverage/clover.xml`:
- Around line 7-487: The committed coverage XML contains absolute local paths
(e.g., path="/home/matvei/projets/vortex/src/...") which leak workstation
details; remove the generated file coverage/clover.xml from version control and
stop committing coverage outputs: delete or git rm --cached coverage/clover.xml,
add coverage/ (or coverage/clover.xml) to .gitignore, commit that change, and
re-run CI locally to ensure coverage artifacts are not tracked; optionally
configure your coverage tool (nyc/istanbul or whatever generates clover) to emit
relative paths or strip the project root when regenerating to prevent future
leaks.
In `@coverage/hooks/index.html`:
- Around line 2-145: This PR accidentally includes generated coverage artifacts
(e.g., coverage/hooks/index.html); remove all tracked coverage/** files from the
commit and add a .gitignore rule to ignore coverage/ going forward. In practice,
remove the files from the index (unstage/remove from git tracking), commit that
removal, add "coverage/" to .gitignore and commit the .gitignore change, and
verify the branch no longer contains any coverage/* entries (so only source
changes like the maxConcurrentDownloads validation remain). Also confirm CI
still produces coverage reports but they are not committed.
In `@coverage/layouts/index.html`:
- Around line 2-145: The committed generated coverage artifact
coverage/layouts/index.html should be removed from the PR and coverage output
excluded from source control: revert or remove the file from the commit (undo
the change to index.html), run git rm --cached on the committed coverage file(s)
(or the coverage directory) to stop tracking them, add coverage/ to .gitignore
to prevent future commits, and create a follow-up commit that stages the
.gitignore change and removal so only source/test changes (e.g., your
maxConcurrentDownloads fix) remain in the PR.
In `@coverage/prettify.css`:
- Line 1: Remove committed coverage artifacts and stop tracking them: add an
entry for coverage/ to .gitignore, remove the tracked coverage files from git
history (git rm -r --cached coverage or equivalent) and commit that change, and
update CI to generate and upload coverage reports as CI artifacts or to a
coverage service (Codecov/Coveralls). Also revert/remove the prettify.css
addition from the commit so third-party generated files are not stored in the
repo and ensure future runs write reports only to the coverage/ directory which
is now ignored.
In `@coverage/theme/index.html`:
- Around line 1-131: Remove the generated coverage HTML from the PR by deleting
the coverage index.html for the theme (the file whose <title> is "Code coverage
report for theme" and which contains the <table class="coverage-summary">),
revert that file from this commit, and update commits so only source changes for
the maxConcurrentDownloads validation remain; also ensure coverage outputs are
excluded from future commits by adding the coverage output pattern to the repo's
ignore rules (e.g., .gitignore) and committing that change.
In `@coverage/views/DownloadsView/DownloadsView.tsx.html`:
- Around line 2-225: Remove the generated Istanbul reports: delete the entire
coverage/ directory and any coverage/**/*.html, coverage/**/*.css,
coverage/**/*.js files from the commit and repo (unstage and remove them if
already staged), then add coverage/ (or the patterns coverage/**) to .gitignore
so these artifacts are not tracked in future commits; finally re-commit the
change so the PR only contains the real code edits (the validated
maxConcurrentDownloads logic in update_config.rs and its tests remain
untouched).
---
Nitpick comments:
In `@coverage/api/hooks.ts.html`:
- Around line 182-184: Tests are missing for the invalidateKeys success path
inside useTauriMutation; add a unit test that calls useTauriMutation with
options.invalidateKeys set (e.g., an array of query keys), mock or stub
queryClientInstance.invalidateQueries, invoke the mutation's onSuccess handler
(or trigger a successful mutation) and assert that
queryClientInstance.invalidateQueries was called for each key; ensure the test
imports useTauriMutation and supplies a mocked QueryClient or context so the
loop over options.invalidateKeys and calls to invalidateQueries are executed and
verified.
In `@coverage/layouts/Sidebar.tsx.html`:
- Around line 1-217: This file is an auto-generated Istanbul coverage HTML
(contains the Sidebar.tsx report) and should not be committed; delete the
coverage/layouts/Sidebar.tsx.html from the repo, add the coverage output pattern
(e.g., coverage/ or **/coverage/*.html) to .gitignore, and update CI to generate
and upload coverage artifacts (or publish to a service) instead of committing;
look for references to "Sidebar.tsx" and the top-level "coverage" directory in
the diff to locate the offending artifact.
In `@coverage/lib/index.html`:
- Around line 1-131: The PR includes generated coverage HTML (e.g.,
coverage/lib/index.html) which should not be committed; remove these build
artifacts from the branch, add coverage/ to .gitignore, and unstage/remove the
files from git history in this commit (e.g., git rm --cached coverage/... and
commit the removal) so only the source changes remain while CI stores coverage
as artifacts.
In `@src/views/SettingsView/__tests__/Sections.test.tsx`:
- Around line 111-117: Replace the fragile DOM-traversal selector with an
accessible query to locate the numeric input for the DownloadsSection: use
getByRole('spinbutton', { name: /Max concurrent downloads/i }) to get the input
element, then assert both input.min === '1' and input.max === '20' to cover the
full 1–20 constraint; optionally, add a small loop that sets values 1..20 via
fireEvent.change and asserts no validation error to ensure the entire range is
accepted.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ab514a91-c653-42c7-93b6-3744619d2571
⛔ Files ignored due to path filters (2)
coverage/favicon.pngis excluded by!**/*.pngcoverage/sort-arrow-sprite.pngis excluded by!**/*.png
📒 Files selected for processing (59)
.claude/settings.json.codexAGENT.mdCHANGELOG.mdcoverage/api/client.ts.htmlcoverage/api/hooks.ts.htmlcoverage/api/index.htmlcoverage/api/queries.ts.htmlcoverage/base.csscoverage/block-navigation.jscoverage/clover.xmlcoverage/components/ui/badge.tsx.htmlcoverage/components/ui/button.tsx.htmlcoverage/components/ui/checkbox.tsx.htmlcoverage/components/ui/dropdown-menu.tsx.htmlcoverage/components/ui/index.htmlcoverage/components/ui/input.tsx.htmlcoverage/components/ui/progress.tsx.htmlcoverage/components/ui/tooltip.tsx.htmlcoverage/coverage-final.jsoncoverage/hooks/index.htmlcoverage/hooks/useDownloadEvents.ts.htmlcoverage/hooks/useDownloadProgress.ts.htmlcoverage/hooks/useTauriEvent.ts.htmlcoverage/index.htmlcoverage/layouts/AppLayout.tsx.htmlcoverage/layouts/Sidebar.tsx.htmlcoverage/layouts/StatusBar.tsx.htmlcoverage/layouts/index.htmlcoverage/lib/format.ts.htmlcoverage/lib/index.htmlcoverage/lib/utils.ts.htmlcoverage/prettify.csscoverage/prettify.jscoverage/sorter.jscoverage/stores/downloadStore.ts.htmlcoverage/stores/index.htmlcoverage/stores/layout-store.ts.htmlcoverage/stores/settingsStore.ts.htmlcoverage/stores/uiStore.ts.htmlcoverage/theme/index.htmlcoverage/theme/theme-provider.tsx.htmlcoverage/theme/useTheme.ts.htmlcoverage/types/index.htmlcoverage/types/layout.ts.htmlcoverage/views/DownloadsView/ActionsBar.tsx.htmlcoverage/views/DownloadsView/DownloadsTable.tsx.htmlcoverage/views/DownloadsView/DownloadsView.tsx.htmlcoverage/views/DownloadsView/EtaCell.tsx.htmlcoverage/views/DownloadsView/FilterBar.tsx.htmlcoverage/views/DownloadsView/ProgressCell.tsx.htmlcoverage/views/DownloadsView/SearchBar.tsx.htmlcoverage/views/DownloadsView/SpeedCell.tsx.htmlcoverage/views/DownloadsView/StateIndicator.tsx.htmlcoverage/views/DownloadsView/index.htmlsrc-tauri/src/application/commands/update_config.rssrc/routes/__tests__/routes.test.tsxsrc/views/SettingsView/DownloadsSection.tsxsrc/views/SettingsView/__tests__/Sections.test.tsx
| @@ -0,0 +1 @@ | |||
| CLAUDE.md No newline at end of file | |||
There was a problem hiding this comment.
Replace the placeholder with real guidance (or remove the file).
AGENT.md currently contains only CLAUDE.md, which is non-actionable and looks like an accidental placeholder. Either add actual agent instructions or convert this into a clear markdown link + explanation.
Proposed fix
-CLAUDE.md
+# Agent guidance
+
+See [CLAUDE.md](./CLAUDE.md) for the full configuration and usage notes.Based on learnings: "Use meaningful commit messages and document code changes clearly".
📝 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.
| CLAUDE.md | |
| # Agent guidance | |
| See [CLAUDE.md](./CLAUDE.md) for the full configuration and usage notes. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@AGENT.md` at line 1, AGENT.md currently contains only the placeholder text
"CLAUDE.md" — replace it with actionable agent documentation or remove it;
update AGENT.md to either (a) provide real instructions for using or running the
agent (purpose, setup steps, configuration keys, example commands, and expected
behavior) or (b) convert the placeholder into a clear markdown link and short
explanation (e.g., "See CLAUDE.md for X, or follow these steps..."); ensure the
file includes a meaningful header/title and a concise commit message describing
the change when committing (use AGENT.md and the referenced "CLAUDE.md" string
to locate the placeholder).
There was a problem hiding this comment.
1 issue found across 61 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/routes/__tests__/routes.test.tsx">
<violation number="1" location="src/routes/__tests__/routes.test.tsx:7">
P2: Routing tests duplicate the route table instead of exercising the production router, so they can miss regressions in App.tsx/AppLayout and the real route elements.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
- Replace toBeDefined() with not.toBeNull() for null-safe assertion - Remove accidentally committed coverage/, .claude/, .codex, AGENT.md - Add coverage/ and Claude Code local files to .gitignore - Rewrite routes test to derive AppRoutes from ROUTES config (no duplication)
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/routes/__tests__/routes.test.tsx (2)
9-25: These redirect tests don’t verify the production router.
AppRoutes(Line 9 onward) re-implements routing/redirect behavior, so the assertions on Line 56 and Line 61 can still pass ifsrc/App.tsxdrifts. Todaysrc/App.tsxhard-codes redirects at Line 27 and Line 38, so this suite gives false confidence about actual app routing.Also applies to: 56-64
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/__tests__/routes.test.tsx` around lines 9 - 25, The test re-implements routing in AppRoutes instead of exercising the real router in src/App.tsx, so keep tests aligned with production behavior by rendering the actual App (or the router exported from it) rather than recreating ROUTES locally; replace the AppRoutes helper with a render of the real App component (or import the router/Routes from src/App.tsx) and drive navigation via MemoryRouter/initialEntries if needed so the assertions that currently pass against the fake AppRoutes will validate the real redirects implemented in src/App.tsx.
28-30: Avoid hard-coding route count unless it is a strict product invariant.The exact
10assertion on Line 28 will create churn for legitimate route additions/removals. Prefer asserting required paths/features instead of a fixed total.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/__tests__/routes.test.tsx` around lines 28 - 30, The test currently hard-codes ROUTES length to 10 which will break on valid route changes; modify the test that references ROUTES (the "should define exactly 10 routes" case) to instead assert required route entries exist—e.g., verify ROUTES contains specific route objects or paths (use ROUTES array checks like arrayContaining on the route path strings or route names) and/or validate required features (protected flag, component existence) for those required routes rather than asserting an exact total count.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/routes/__tests__/routes.test.tsx`:
- Around line 9-25: The test re-implements routing in AppRoutes instead of
exercising the real router in src/App.tsx, so keep tests aligned with production
behavior by rendering the actual App (or the router exported from it) rather
than recreating ROUTES locally; replace the AppRoutes helper with a render of
the real App component (or import the router/Routes from src/App.tsx) and drive
navigation via MemoryRouter/initialEntries if needed so the assertions that
currently pass against the fake AppRoutes will validate the real redirects
implemented in src/App.tsx.
- Around line 28-30: The test currently hard-codes ROUTES length to 10 which
will break on valid route changes; modify the test that references ROUTES (the
"should define exactly 10 routes" case) to instead assert required route entries
exist—e.g., verify ROUTES contains specific route objects or paths (use ROUTES
array checks like arrayContaining on the route path strings or route names)
and/or validate required features (protected flag, component existence) for
those required routes rather than asserting an exact total count.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 05e5f751-99d2-45f2-98e2-6ab3d3c83ee7
📒 Files selected for processing (3)
.gitignoresrc/routes/__tests__/routes.test.tsxsrc/views/SettingsView/__tests__/Sections.test.tsx
✅ Files skipped from review due to trivial changes (2)
- .gitignore
- src/views/SettingsView/tests/Sections.test.tsx
There was a problem hiding this comment.
1 issue found across 59 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/routes/__tests__/routes.test.tsx">
<violation number="1" location="src/routes/__tests__/routes.test.tsx:18">
P2: App routing coverage is now self-referential and no longer verifies the real router/component mapping or the canonical default redirect.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Summary
Fixes #49: The settings validation for
maxConcurrentDownloadswas incorrectly accepting values up to 100, violating the PRD §6.10 specification which limits the maximum to 20.Changes
• Backend: Updated Rust validation in
update_confighandler from1..=100to1..=20• Frontend: Changed settings UI input max constraint from 100 to 20
• Added tests covering the new boundary conditions (rejects 21, accepts 20)
• Updated CHANGELOG.md with the fix
Type
fix
Summary by cubic
Enforces the PRD §6.10 limit by capping
maxConcurrentDownloadsat 1–20 across backend and settings UI. Fixes #49 and prevents saving invalid values.maxConcurrentDownloadsin backend validation and settings input.ROUTEScovering redirects and route count.coverage/and Claude local files to.gitignore, remove stray files, and update CHANGELOG.Written for commit 13a7ed6. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests
Documentation