Skip to content

fix: Inline validation for empty and duplicate tags in Tag Editor #586

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 1 commit into
base: main
Choose a base branch
from
Open

fix: Inline validation for empty and duplicate tags in Tag Editor #586

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
48 changes: 34 additions & 14 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 @@ -99,17 +99,27 @@ class TagsRouteState extends State<TagsRoute> {
Map<String, TagMetadata>? _pendingTags;
ListBuilder<String>? draftTags;

void _addTag(String tag) {
if (tag.isNotEmpty) {
// Add this condition to ensure the tag is not empty
if (draftTags == null) {
draftTags = ListBuilder([tag]);
} else {
List<String> _parseTags(String input) {
return input
.split(',')
.map((e) => e.trim())
.where((e) => e.isNotEmpty)
.toList();
}

void _addTags(List<String> tags) {
if (tags.isEmpty) return;

draftTags ??= ListBuilder<String>();

for (final tag in tags) {
if (!draftTags!.build().contains(tag)) {
Copy link

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

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

In _addTags, calling draftTags!.build() inside the loop allocates a new immutable list on every iteration. Use draftTags!.contains(tag) (ListBuilder supports contains) or build once before the loop / keep a Set for membership checks to avoid repeated allocations and quadratic behavior as tags grow.

Suggested change
if (!draftTags!.build().contains(tag)) {
if (!draftTags!.contains(tag)) {

Copilot uses AI. Check for mistakes.
draftTags!.add(tag);
}
widget.callback(draftTags);
setState(() {});
}

widget.callback(draftTags);
setState(() {});
}

void _removeTag(String tag) {
Expand Down Expand Up @@ -197,7 +207,7 @@ class TagsRouteState extends State<TagsRoute> {
!(draftTags?.build().contains(tag.key) ?? false)))
FilterChip(
backgroundColor: TaskWarriorColors.grey,
onSelected: (_) => _addTag(tag.key),
onSelected: (_) => _addTags([tag.key]),
label: Text(
'${tag.key} ${tag.value.frequency}',
),
Expand Down Expand Up @@ -234,11 +244,22 @@ class TagsRouteState extends State<TagsRoute> {
color: tColors.primaryTextColor,
),
validator: (value) {
if (value != null) {
if (value.isNotEmpty && value.contains(" ")) {
final tags = _parseTags(value ?? '');

if (tags.isEmpty) {
return "Please enter a tag";
}

for (final tag in tags) {
if (tag.contains(' ')) {
return "Tags cannot contain spaces";
}

if (draftTags?.build().contains(tag) ?? false) {
Copy link

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

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

draftTags?.build().contains(tag) in the validator has the same repeated-allocation issue as in _addTags. Prefer draftTags?.contains(tag) (or build once outside the loop) to avoid creating a new BuiltList during every validation pass (which can be triggered frequently while typing).

Suggested change
if (draftTags?.build().contains(tag) ?? false) {
if (draftTags?.contains(tag) ?? false) {

Copilot uses AI. Check for mistakes.
return "Tag already exists";
}
}
Comment on lines 246 to 261
Copy link

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

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

The validator introduces hard-coded English error strings ("Please enter a tag", "Tags cannot contain spaces", "Tag already exists"), which bypasses the app’s localization approach via SentenceManager (e.g., sentences.tagAlreadyExists, sentences.tagShouldNotContainSpaces). Please switch these to localized SentenceManager(...).sentences entries, and add a new sentences key for the empty-input message so it’s translated consistently.

Copilot uses AI. Check for mistakes.

return null;
},
autofocus: true,
Expand All @@ -264,9 +285,8 @@ class TagsRouteState extends State<TagsRoute> {
onPressed: () {
if (formKey.currentState!.validate()) {
try {
validateTaskTags(controller.text);
_addTag(controller.text);
// Navigator.of(context).pop();
final tags = _parseTags(controller.text);
_addTags(tags);
Get.back();
} on FormatException catch (e, trace) {
Comment on lines 287 to 291
Copy link

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

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

This change introduces new validation behavior (empty input, duplicates, comma-separated submission, and keeping the dialog open on invalid submit), but there are no widget tests covering it. Please add a widget test for TagsRoute that asserts inline errors are shown and that the dialog only closes when a valid tag (or tag list) is submitted.

Copilot uses AI. Check for mistakes.
logError(e, trace);
Expand Down
Loading