Fix queue rules upper bound to never allow 0#543
Merged
Conversation
Contributor
Author
|
@sourcery-ai review |
Review Summary by QodoEnforce non-zero maximum completion percentage validation
WalkthroughsDescription• Enforce maximum completion percentage greater than 0 • Add validation to prevent invalid 0 value in backend • Update frontend error messages for completion range • Correct error message text to reflect new bounds Diagramflowchart LR
A["QueueRuleDto Range Constraint"] -->|"Update Range 0-100 to 1-100"| B["Backend Validation"]
B -->|"Add zero check"| C["QueueRule.Validate()"]
C -->|"Throw ValidationException"| D["Invalid Configuration Rejected"]
E["Frontend Component"] -->|"Add max <= 0 check"| F["stallCompletionError & slowCompletionError"]
F -->|"Display error message"| G["User Feedback"]
H["HTML Template"] -->|"Move error binding to Max field"| I["Correct Error Display"]
File Changes1. code/backend/Cleanuparr.Api/Features/QueueCleaner/Contracts/Requests/QueueRuleDto.cs
|
Code Review by Qodo
1.
|
Reviewer's guide (collapsed on small PRs)Reviewer's GuideEnforces a minimum MaxCompletionPercentage of 1% (no longer allowing 0) across backend validation, DTO annotations, and frontend UI validation for queue cleaner rules, and ensures the max completion error message is displayed on the correct input fields. Class diagram for queue cleaner max completion validation changesclassDiagram
class QueueRule {
+ushort MinCompletionPercentage
+ushort MaxCompletionPercentage
+void Validate()
}
class QueueRuleDto {
+ushort MinCompletionPercentage
+ushort MaxCompletionPercentage
}
class QueueCleanerComponent {
+stallMinCompletion
+stallMaxCompletion
+slowMinCompletion
+slowMaxCompletion
+string stallCompletionError()
+string slowCompletionError()
}
QueueRuleDto --> QueueRule : maps_to_domain_rule
QueueCleanerComponent --> QueueRuleDto : binds_via_API_requests
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The frontend still allows a max completion of 0 via
[min]="0"on theMax Completion %inputs while the backend now rejects 0; consider updating these inputs (and any related default values) to use a minimum of 1 so the UI constraints align with server-side validation. - You now validate
MaxCompletionPercentageboth with a[Range(1, 100)]attribute and explicit checks inQueueRule.Validate; consider consolidating this logic or ensuring the messages stay in sync to avoid future inconsistencies.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The frontend still allows a max completion of 0 via `[min]="0"` on the `Max Completion %` inputs while the backend now rejects 0; consider updating these inputs (and any related default values) to use a minimum of 1 so the UI constraints align with server-side validation.
- You now validate `MaxCompletionPercentage` both with a `[Range(1, 100)]` attribute and explicit checks in `QueueRule.Validate`; consider consolidating this logic or ensuring the messages stay in sync to avoid future inconsistencies.
## Individual Comments
### Comment 1
<location path="code/frontend/src/app/features/settings/queue-cleaner/queue-cleaner.component.html" line_range="265-270" />
<code_context>
hint="Apply the rule once completion percentage exceeds this value (0 includes items at 0% and above)"
- [error]="stallCompletionError()"
helpKey="queue-cleaner:stallRule.completionRange" />
<app-number-input label="Max Completion %" [(value)]="stallMaxCompletion" [min]="0" [max]="100" suffix="%"
hint="Apply the rule to items with a completion percentage less than or equal to this value"
+ [error]="stallCompletionError()"
</code_context>
<issue_to_address>
**issue (bug_risk):** Align the Max Completion % input range with the backend (min should be 1, not 0).
The DTO now enforces `[Range(1, 100)]` for `MaxCompletionPercentage` and the TS validators reject `max <= 0`, but both Max Completion inputs here still use `[min]="0"`, allowing users to pick `0` and then immediately get a validation error. Please update these to `[min]="1"` so the UI matches backend/client validation and avoids this confusing behavior.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Greptile SummaryThis PR fixes a bug where Key changes:
Confidence Score: 4/5Safe to merge — the fix correctly enforces the new constraint across all layers with one minor HTML attribute inconsistency that has no functional impact. The change is small, focused, and correctly applied at every validation layer (DTO, domain model, frontend signals). The only gap is that both Max Completion % app-number-input elements still carry [min]="0" instead of [min]="1", meaning the input widget itself won't prevent typing 0 — but the validation computed signal and backend guards will still catch it, so no invalid data can actually be persisted. code/frontend/src/app/features/settings/queue-cleaner/queue-cleaner.component.html — both Max Completion % inputs need [min]="1" Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[User enters MaxCompletion value] --> B{Value == 0?}
B -- Yes --> C[stallCompletionError / slowCompletionError returns 'Max percentage must be greater than 0']
C --> D[Error shown on Max Completion % field - Save blocked]
B -- No --> E{Value < Min?}
E -- Yes --> F[stallCompletionError / slowCompletionError returns 'Max must be >= Min']
F --> D
E -- No --> G[Frontend validation passes - Save request sent]
G --> H[QueueRuleDto Range validation - Range 1 to 100]
H -- Invalid --> I[400 Bad Request]
H -- Valid --> J[QueueRule.Validate called]
J -- MaxCompletion == 0 --> K[ValidationException thrown]
J -- MaxCompletion > 100 --> K
J -- MaxCompletion < MinCompletion --> K
J -- Valid --> L[Rule saved to DB]
Reviews (1): Last reviewed commit: "fixed queue rules upper bound to never a..." | Re-trigger Greptile |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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.
Relates to #541
Summary by Sourcery
Enforce a minimum maximum-completion percentage greater than zero for queue cleaner rules and align backend validation, API constraints, and frontend validation/UI with this requirement.
Bug Fixes: