Skip to content

Fix: Inline validation for empty and duplicate tags in Tag Editor #519

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
mulikruchi07 wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Fix: Inline validation for empty and duplicate tags in Tag Editor #519

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9a0929a
fix(detail): avoid backend auto-start; save user-selected start only
mulikruchi07 Dec 7, 2025
b4e9efc
Fix: prevent empty and duplicate tags & show inline validation messages
mulikruchi07 Dec 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 25 additions & 11 deletions lib/app/modules/detailRoute/controllers/detail_route_controller.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@ class DetailRouteController extends GetxController {
var onEdit = false.obs;
var isReadOnly = false.obs;

// Track whether user explicitly selected a start date
bool startEdited = false;

@override
void onInit() {
super.onInit();
Expand Down Expand Up @@ -50,13 +53,19 @@ class DetailRouteController extends GetxController {
}

if (name == 'start') {
debugPrint('Start Value Changed to $newValue');
startEdited = true; // MARK AS USER-SELECTED
startValue.value = newValue;
}
initValues();
}

Future<void> saveChanges() async {
// If start was never edited AND backend auto-generated it (start == entry)
if (!startEdited &&
modify.original.start != null &&
modify.original.start!.isAtSameMomentAs(modify.original.entry)) {
modify.set('start', null); // remove auto start
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Suggested change
modify.set('start', null); // remove auto start
modify.set('start', null);

Copilot uses AI. Check for mistakes.
}
var now = DateTime.now().toUtc();
modify.save(modified: () => now);
onEdit.value = false;
Expand Down Expand Up @@ -106,7 +115,20 @@ class DetailRouteController extends GetxController {
statusValue.value = modify.draft.status;
entryValue.value = modify.draft.entry;
modifiedValue.value = modify.draft.modified;
startValue.value ??= null;
final originalStart = modify.original.start;
final originalEntry = modify.original.entry;

final backendAutoStart = (originalStart != null &&
originalStart.isAtSameMomentAs(originalEntry));

// START DATE LOGIC (THE FIX)
if (startEdited) {
startValue.value = modify.draft.start;
} else if (backendAutoStart) {
startValue.value = null; // Do not show backend auto start
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Suggested change
startValue.value = null; // Do not show backend auto start
startValue.value = null; // Hide backend-generated start date (start == entry) from UI

Copilot uses AI. Check for mistakes.
} else {
startValue.value = modify.draft.start; // Existing meaningful start
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Suggested change
startValue.value = modify.draft.start; // Existing meaningful start
startValue.value = modify.draft.start; // Show user-set or pre-existing start date (not auto-generated)

Copilot uses AI. Check for mistakes.
}
endValue.value = modify.draft.end;
dueValue.value = modify.draft.due;
waitValue.value = modify.draft.wait;
Expand Down Expand Up @@ -148,15 +170,7 @@ class DetailRouteController extends GetxController {
const Duration(milliseconds: 500),
() {
SaveTourStatus.getDetailsTourStatus().then((value) => {
if (value == false)
{
tutorialCoachMark.show(context: context),
}
else
{
// ignore: avoid_print
print('User has seen this page'),
}
if (!value) {tutorialCoachMark.show(context: context)}
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Copilot uses AI. Check for mistakes.
});
},
);
Expand Down
15 changes: 11 additions & 4 deletions lib/app/modules/detailRoute/views/tags_widget.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,10 +234,17 @@ class TagsRouteState extends State<TagsRoute> {
color: tColors.primaryTextColor,
),
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();
Comment on lines +237 to +240
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. A tag with trailing/leading spaces could pass validation but be added with spaces
  2. 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.

Copilot uses AI. Check for mistakes.
if (tag.contains(" ")) {
return "Tags cannot contain spaces";
}
final currentTags =
draftTags?.build().toList() ?? <String>[];
if (currentTags.contains(tag)) {
return "Tag already exists";
}
return null;
Comment on lines 236 to 249
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Empty input validation ("Please enter a tag")
  2. Duplicate tag detection ("Tag already exists")
  3. Space validation ("Tags cannot contain spaces")
  4. Successful tag addition after validation passes

This ensures the validator function works correctly with the form state and user interactions.

Copilot uses AI. Check for mistakes.
},
Expand Down
1 change: 0 additions & 1 deletion lib/app/utils/taskfunctions/draft.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@

import 'package:taskwarrior/app/models/models.dart';
import 'package:taskwarrior/app/utils/taskfunctions/patch.dart';

Expand Down
1 change: 1 addition & 0 deletions lib/app/utils/taskfunctions/modify.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ class Modify {

Task get draft => _draft.draft;
int get id => _draft.original.id!;
Task get original => _draft.original;

Map<dynamic, Map> get changes {
var result = <dynamic, Map>{};
Expand Down