Fix: Inline validation for empty and duplicate tags in Tag Editor #519
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces inline validation for the tag editor to prevent empty and duplicate tags, but also includes unrelated changes to start date handling logic that are not mentioned in the PR description.
Key Changes:
- Added validation in the tag editor to display "Please enter a tag" for empty inputs and "Tag already exists" for duplicates
- Modified the Modify class to expose the original task via a getter
- Introduced start date tracking logic to distinguish user-edited dates from backend auto-generated ones (unrelated to PR title/description)
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| lib/app/modules/detailRoute/views/tags_widget.dart | Enhanced tag input validation with empty and duplicate checks |
| lib/app/utils/taskfunctions/modify.dart | Added original getter to access the original task state |
| lib/app/utils/taskfunctions/draft.dart | Removed leading empty line (whitespace cleanup) |
| lib/app/modules/detailRoute/controllers/detail_route_controller.dart | Added start date editing tracking and auto-start suppression logic (not mentioned in PR description) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (!startEdited && | ||
| modify.original.start != null && | ||
| modify.original.start!.isAtSameMomentAs(modify.original.entry)) { | ||
| modify.set('start', null); // remove auto start |
There was a problem hiding this comment.
[nitpick] The inline comment "remove auto start" is redundant given the self-documenting nature of the code (modify.set('start', null)). The larger block comment above (lines 63-66) already explains the intent. Consider removing this inline comment to reduce noise.
| modify.set('start', null); // remove auto start | |
| modify.set('start', null); |
| if (startEdited) { | ||
| startValue.value = modify.draft.start; | ||
| } else if (backendAutoStart) { | ||
| startValue.value = null; // Do not show backend auto start |
There was a problem hiding this comment.
[nitpick] The inline comment "Do not show backend auto start" could be more descriptive. Consider: "Hide backend-generated start date (start == entry) from UI" to better explain why this is being hidden.
| startValue.value = null; // Do not show backend auto start | |
| startValue.value = null; // Hide backend-generated start date (start == entry) from UI |
| } else if (backendAutoStart) { | ||
| startValue.value = null; // Do not show backend auto start | ||
| } else { | ||
| startValue.value = modify.draft.start; // Existing meaningful start |
There was a problem hiding this comment.
[nitpick] The comment "Existing meaningful start" could be clearer. Consider: "Show user-set or pre-existing start date (not auto-generated)" to better distinguish this case from the other branches.
| startValue.value = modify.draft.start; // Existing meaningful start | |
| startValue.value = modify.draft.start; // Show user-set or pre-existing start date (not auto-generated) |
| if (value == null || value.trim().isEmpty) { | ||
| return "Please enter a tag"; | ||
| } | ||
| final tag = value.trim(); |
There was a problem hiding this comment.
The validator trims the input value (line 240: final tag = value.trim()) for validation, but the actual submission on line 274 uses the untrimmed controller.text. This inconsistency could lead to bugs:
- A tag with trailing/leading spaces could pass validation but be added with spaces
- Duplicate detection in the validator could fail because it checks trimmed values while existing tags might have spaces
The validator should check the untrimmed value, or the submission code should trim before adding. Since tags shouldn't have leading/trailing spaces, consider adding a comment here explaining that the trimming matches the submission logic, or better yet, ensure the submission code (line 274-275) uses the trimmed value.
| validator: (value) { | ||
| if (value != null) { | ||
| if (value.isNotEmpty && value.contains(" ")) { | ||
| return "Tags cannot contain spaces"; | ||
| } | ||
| if (value == null || value.trim().isEmpty) { | ||
| return "Please enter a tag"; | ||
| } | ||
| final tag = value.trim(); | ||
| if (tag.contains(" ")) { | ||
| return "Tags cannot contain spaces"; | ||
| } | ||
| final currentTags = | ||
| draftTags?.build().toList() ?? <String>[]; | ||
| if (currentTags.contains(tag)) { | ||
| return "Tag already exists"; | ||
| } | ||
| return null; |
There was a problem hiding this comment.
The new validation logic for empty tags and duplicate tags is not covered by tests. Given that the repository has comprehensive test coverage for validation functions (see test/utils/taskfunctions/validate_test.dart), widget-level tests should be added to verify:
- Empty input validation ("Please enter a tag")
- Duplicate tag detection ("Tag already exists")
- Space validation ("Tags cannot contain spaces")
- Successful tag addition after validation passes
This ensures the validator function works correctly with the form state and user interactions.
| // ignore: avoid_print | ||
| print('User has seen this page'), | ||
| } | ||
| if (!value) {tutorialCoachMark.show(context: context)} |
There was a problem hiding this comment.
[nitpick] This code style change from an if-else block to a ternary-like expression should be reverted or explained. The original code was more readable with explicit if-else blocks. If this change is intentional for consistency with project style, it should be in a separate commit focused on code style improvements, not bundled with functional changes.
1 participant
Description
This PR introduces validation and displays clear inline error messages:
The dialog now remains open until a valid tag is submitted.
Fixes #518
Screenshots
Checklist