Repository navigation
fix: Improve label colors validations - #183
Conversation
|
Warning Review limit reached
Next review available in: 12 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughLabelModel's TextColor and BackgroundColor properties change from nullable Color? to non-nullable Color with default values and simplified Required attributes. AddLabelModal and EditLabelModal are updated to bind color inputs directly and persist colors without nullable/null-forgiving handling; EditLabelModal's EditContext now uses _labelModel. ChangesLabel Color Non-Nullability
Related PRs: None identified. Suggested labels: enhancement, bug Suggested reviewers: dkorecko Poem: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@Ticky.Base/Models/LabelModel.cs`:
- Around line 9-15: The [Required] attributes on LabelModel.TextColor and
LabelModel.BackgroundColor are ineffective because these Color structs can’t be
null, so replace them with validation that explicitly rejects invalid values
such as Color.Empty or add a custom validator on LabelModel to enforce allowed
colors. Keep the Display metadata if needed, but remove the misleading Required
annotations from these properties.
In `@Ticky.Web/Components/Dialogs/AddLabelModal.razor`:
- Around line 18-27: The Required attributes on LabelModel.TextColor and
LabelModel.BackgroundColor are ineffective because these non-nullable Color
properties are always initialized, so the ValidationMessage fields in
AddLabelModal and the matching label modal will never show. Update LabelModel to
use real validation for unset/default colors, such as IValidatableObject or a
custom validation attribute that rejects Color.Empty/default, and keep the modal
bindings and ValidationMessage references pointing at the same TextColor and
BackgroundColor members.
- Around line 19-25: The color inputs in AddLabelModal and EditLabelModal are
binding values through ColorTranslator.ToHtml, which can output named colors
instead of the hex format required by input type="color". Update the value
binding in the color picker fields and any related helper logic so the displayed
value is always serialized as `#rrggbb`, using the existing
OnTextColorUpdated/OnBackgroundColorUpdated handlers and LabelModel color
properties as the reference points.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 844a1563-8763-4c18-9baa-2a03a5062f15
📒 Files selected for processing (3)
Ticky.Base/Models/LabelModel.csTicky.Web/Components/Dialogs/AddLabelModal.razorTicky.Web/Components/Dialogs/EditLabelModal.razor
| [Required] | ||
| [Display(Name = "Text color")] | ||
| public Color? TextColor { get; set; } | ||
| public Color TextColor { get; set; } = Color.White; | ||
|
|
||
| [Required(AllowEmptyStrings = false)] | ||
| [Required] | ||
| [Display(Name = "Background color")] | ||
| public Color? BackgroundColor { get; set; } | ||
| public Color BackgroundColor { get; set; } = Color.Red; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
[Required] does nothing on these Color properties.
TextColor and BackgroundColor are non-nullable structs, so [Required] will never fail here and gives a false sense of validation. If the goal is to reject unset/invalid colors, use a check for Color.Empty or a custom validator instead.
🤖 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 `@Ticky.Base/Models/LabelModel.cs` around lines 9 - 15, The [Required]
attributes on LabelModel.TextColor and LabelModel.BackgroundColor are
ineffective because these Color structs can’t be null, so replace them with
validation that explicitly rejects invalid values such as Color.Empty or add a
custom validator on LabelModel to enforce allowed colors. Keep the Display
metadata if needed, but remove the misleading Required annotations from these
properties.
| <Name For="() => _labelModel.TextColor" /> | ||
| <input type="color" value="@(_labelModel.TextColor.HasValue ? ColorTranslator.ToHtml(_labelModel.TextColor.Value) : string.Empty)" @onchange="OnTextColorUpdated" /> | ||
| <input type="color" value="@ColorTranslator.ToHtml(_labelModel.TextColor)" @onchange="OnTextColorUpdated" /> | ||
| <ValidationMessage For="() => _labelModel.TextColor" /> | ||
| </div> | ||
|
|
||
| <div class="form-group"> | ||
| <Name For="() => _labelModel.BackgroundColor" /> | ||
| <input type="color" value="@(_labelModel.BackgroundColor.HasValue ? ColorTranslator.ToHtml(_labelModel.BackgroundColor.Value) : string.Empty)" @onchange="OnBackgroundColorUpdated" /> | ||
| <input type="color" value="@ColorTranslator.ToHtml(_labelModel.BackgroundColor)" @onchange="OnBackgroundColorUpdated" /> | ||
| <ValidationMessage For="() => _labelModel.BackgroundColor" /> | ||
| </div> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate relevant files and symbols.
git ls-files | rg 'Ticky\.Web/Components/Dialogs/(AddLabelModal|EditLabelModal)\.razor|LabelModel|Models|ViewModels'
rg -n --hidden --glob '!**/bin/**' --glob '!**/obj/**' \
'TextColor|BackgroundColor|IValidatableObject|Color\.Empty|\[Required\]' \
Ticky.Web 2>/dev/null || true
# Show file sizes before reading.
for f in \
Ticky.Web/Components/Dialogs/AddLabelModal.razor \
Ticky.Web/Components/Dialogs/EditLabelModal.razor \
Ticky.Web/Models/LabelModel.cs
do
if [ -f "$f" ]; then
wc -l "$f"
fi
done
# Read the relevant files if present.
for f in \
Ticky.Web/Components/Dialogs/AddLabelModal.razor \
Ticky.Web/Components/Dialogs/EditLabelModal.razor \
Ticky.Web/Models/LabelModel.cs
do
if [ -f "$f" ]; then
echo "===== $f ====="
cat -n "$f"
fi
doneRepository: dkorecko/Ticky
Length of output: 16349
🏁 Script executed:
#!/bin/bash
set -euo pipefail
f=Ticky.Base/Models/LabelModel.cs
if [ -f "$f" ]; then
wc -l "$f"
cat -n "$f"
fi
# Look for any validation hooks that might make Color validation effective.
rg -n --hidden --glob '!**/bin/**' --glob '!**/obj/**' \
'class LabelModel|IValidatableObject|Validate\(|Color\.Empty|BackgroundColor|TextColor|RequiredAttribute|Display\(' \
Ticky.Base Ticky.Web 2>/dev/null || trueRepository: dkorecko/Ticky
Length of output: 13218
[Required] on LabelModel.TextColor / BackgroundColor is inert.
Color is non-nullable and already initialized, so these attributes can never fail; the color ValidationMessages in both label modals won’t ever surface. If the goal is to reject default/unset colors, use custom validation in LabelModel (for example IValidatableObject or a custom attribute that rejects Color.Empty/default).
🤖 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 `@Ticky.Web/Components/Dialogs/AddLabelModal.razor` around lines 18 - 27, The
Required attributes on LabelModel.TextColor and LabelModel.BackgroundColor are
ineffective because these non-nullable Color properties are always initialized,
so the ValidationMessage fields in AddLabelModal and the matching label modal
will never show. Update LabelModel to use real validation for unset/default
colors, such as IValidatableObject or a custom validation attribute that rejects
Color.Empty/default, and keep the modal bindings and ValidationMessage
references pointing at the same TextColor and BackgroundColor members.
2 participants
Summary by CodeRabbit
New Features
Bug Fixes