feat(tags): tag categories, tag hub, and single-tag page - #901
Merged
Conversation
The delete-category test asserted survival via getOrCreateTag, which is get-or-insert: a hard-delete bug would silently recreate the row and the test would still pass. Query tag_definitions directly instead and check a non-default sortOrder, color, createdAt, and row count survive unchanged. Also swap the raw tag_definitions / category_id string literals in the tagCount subquery for the typed schema references already imported, so a schema rename breaks the build instead of failing silently at runtime.
tag_category was added to SYNC_ITEM_TYPES, RECORD_SYNC_ITEM_TYPES, and RECORD_CLOCK_REQUIRED_ITEM_TYPES but missed the fourth parallel list, ENCRYPTABLE_ITEM_TYPES. It's a record type carrying user-authored data and belongs in the encryptable set exactly as tag_definition does. Add a parity test asserting ENCRYPTABLE_ITEM_TYPES and RECORD_SYNC_ITEM_TYPES contain the same set of types, so a future item type added to one without the other fails immediately. Regenerate the IPC invoke map, whose crypto:* channel signatures embed the ENCRYPTABLE_ITEM_TYPES union type.
The update branch that returns 'applied' (remote clock cleanly newer, same device key) was never exercised. Added a test that upserts a row then re-upserts with a strictly greater clock on the same device to distinguish it from the concurrent/merge conflict path.
…ition-handler Review found no test exercised the null-vs-omitted distinction the !== undefined check on categoryId exists for. Adds one test seeding a category+sortOrder, applying a dominating-clock payload with an explicit categoryId: null, and asserting the category clears while sortOrder is untouched.
Annotate tagsService with Omit<TagsClientAPI, ...> & overrides for the three category methods that intentionally take flat args, instead of dropping the type entirely. Restores compile-time conformance for the ten pre-existing methods and listCategories/reorder.
…ailure updateTagColor succeeding then reorder failing left a tag created but uncategorized, invisible until an unrelated event revalidated the caches, while showing "Failed to create tag" - which is wrong, the tag was created. Refetch categories/note-tags on that branch and use a new tagsHub.errors.createTagFiledFailed message.
The prior test only asserted listCategories' call count, which fetchCategories drives on its own - the mocked useNoteTagsQuery had no refetch field, so createTag's void refetchNoteTags?.() call was an unobservable no-op and deleting it wouldn't fail the test. Added a refetch mock matching useNoteTagsQuery's real return shape and asserted it's called, since a tag's visibility in the hub comes from that data, not from listCategories. Also renamed the createTagFiledFailed i18n key to createTagCategorizeFailed (typo fix, same message), and reset the updateTagColor/reorder/refetchNoteTags mocks in beforeEach to match listCategories' existing reset style.
… reorder CategoryBlock already had working inline rename and delete-confirm UI, but the hub page rendered real category blocks without onRename/onDelete, so both affordances were visible but silently did nothing. Pass renameCategory/deleteCategory from useTagCategories() to the mapped category blocks only; the Uncategorized block keeps neither. Also add page-level tests for handleDragEnd (a tag moved across categories, a category reordered past another), asserting reorder() gets the exact payload. Verified by temporarily breaking the target-index resolution and confirming both new tests fail, then reverting.
TagViewPage always passed an undefined color prop into getTagColors, so the header chip fell back to the name-hash color even when the tag has a user-picked stored color. That mismatched the hub chip / sidebar, which resolve color from tag_definitions via useNoteTagsQuery. Look up the tag's row in useNoteTagsQuery (case-insensitive match) and prefer its stored color, falling back to the color prop then empty string when the query hasn't resolved or the tag isn't found.
…d inbox branches listTagItems' descendant/prefix guarantee was only exercised via notes; tasks and inbox shared the same tagMatches predicate but had no dedicated coverage, so a future per-source predicate split wouldn't be caught.
…ulk bar scope Ports tag-view.tsx's handleNoteOpen so tag-scoped rows route by kind (task tab, inbox with a fresh focusedAt token, note via sidebar navigation) while folder scope keeps its plain-tab handler. Wires Task 10's FilterBuilder lockedCondition and Task 12's BulkActionBar scope/selectedRows props, deriving selectedRows from the live rows + selection set on every render.
The note-only subset of the selection was filtered through the table row model, which is narrowed by the header search and, in the grouped table, by group expansion. Selection is pruned by neither, so a bulk delete or move issued while a search was active silently skipped selected rows that happened to be filtered out of view. Derive it from `notes` instead — kind is a property of a row, not of its visibility — and reuse the same memo for the shift-cmd-M shortcut, which was recomputing the filter inline.
Under tag scope a row can be a task or an inbox item, but the tag chip's remove handler and the bulk "add tag" action both call the notes-only `notesService.update` IPC over the raw selection. Gate the chip handler on kind and run the bulk action over the note-only subset, so the note rows of a mixed selection are still tagged while the rest are skipped. The row context menu acts on that same note-only subset, so label its bulk items with that count and hide them when the selection holds no notes — previously a task-only selection offered "Delete 0 Notes".
LayoutGrid read as a generic view switcher next to the tag rows it opens.
There was a problem hiding this comment.
🟡 Not ready to approve
A few schema/contract details need tightening (notably scope normalization and schema/index alignment) before the change can be safely treated as complete.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (4)
packages/contracts/src/folder-view-api.ts:86
scopeKeylowercases tag names but does not trim them, so callers that accidentally include leading/trailing whitespace will get a different cache key than the same logical tag (and it won’t match the schema change above unless every call path is guaranteed to go through Zod parsing). Trimming here makes the helper robust when it’s used directly.
packages/db-schema/src/schema/tag-definitions.ts:5- The SQL migration creates
idx_tag_definitions_categoryontag_definitions(category_id), but the Drizzle schema definition does not declare that index. This kind of schema/migration drift makes it easier to accidentally remove/rename an index later (and makes schema review harder). Consider declaring the index insqliteTable(...)to keep the schema aligned with the hand-written migration.
packages/contracts/src/folder-view-api.ts:77 ViewScopeSchemacurrently allows whitespace-only tag names (e.g.' 'passes.min(1)), which can create hard-to-debug cache keys and IPC requests that don't resolve to any real tag. Trim (and validate) bothpathandtagat the schema boundary so all downstream code can assume normalized values.
This issue also appears on line 84 of the same file.
apps/desktop/src/main/database/drizzle-data/meta/_journal.json:297
- PR description says the additive migration is
0038, but the code changes introduce0040_tag_categoriesand0041_tag_definition_views(as recorded in the migration journal). Please update the PR description so reviewers and release notes don’t reference the wrong migration number.
"idx": 40,
"version": "6",
"when": 1784892405671,
"tag": "0040_tag_categories",
"breakpoints": true
},
{
"idx": 41,
"version": "6",
"when": 1785585337000,
"tag": "0041_tag_definition_views",
"breakpoints": true
- Files reviewed: 122/124 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| style={{ backgroundColor: tagColors?.text }} | ||
| /> | ||
| {tagSegments.map((segment, i) => ( | ||
| <Fragment key={i}> |
There was a problem hiding this comment.
React Doctor · react-doctor/no-array-index-as-key (warning)
Your users can see & submit the wrong data when this list reorders or filters, so use a stable id like key={item.id}, not the array index "i".
Fix → Use a stable id from the item, like key={item.id} or key={item.slug}. Index keys break when the list reorders or filters.
This was referenced Aug 2, 2026
h4yfans
added a commit
that referenced
this pull request
Aug 3, 2026
Tag categories, the tag hub, and the single-tag page shipped in #901 but were unreachable from MCP, and vault_get_tags narrowed each tag down to name + count so an agent could not even tell that categories exist. - allowlist tags.listCategories as a desktop read operation - allowlist tags.createCategory / renameCategory / deleteCategory / reorder as write operations, so they run behind write approval - widen TagCount with color, icon, sort_order, category_id and category_name; getAllTagsWithCounts already carries every field except the category name, which the adapter joins from listTagCategories Closes #918
h4yfans
added a commit
that referenced
this pull request
Aug 3, 2026
Tag categories, the tag hub, and the single-tag page shipped in #901 but were unreachable from MCP, and vault_get_tags narrowed each tag down to name + count so an agent could not even tell that categories exist. - allowlist tags.listCategories as a desktop read operation - allowlist tags.createCategory / renameCategory / deleteCategory / reorder as write operations, so they run behind write approval - widen TagCount with color, icon, sort_order, category_id and category_name; getAllTagsWithCounts already carries every field except the category name, which the adapter joins from listTagCategories Closes #918
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
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.
What this does
Turns the flat tag namespace into an organizable system: tags can be grouped under named categories, and clicking a tag anywhere opens a full tag page (a table of everything tagged, replacing the cramped sidebar drill-down).
/name. A tag belongs to at most one category; unassigned tags fall under "Uncategorized".tagssingleton tab) — create / rename / delete categories, inline tag creation, drag-and-drop ordering, and a search across both categories and tags. Reached from the tag section's hub button.tagtab, one per tag) — a sharedFolderTableViewlisting the tag's notes, tasks, and inbox items (including/descendants), a kind filter, and header actions (rename / colour / icon / delete).Architecture & data
tag_categoriestable +category_id/sort_ordercolumns ontag_definitions. Additive hand-written migration (0038) — no reset, existing rows keep their values.category_id(a cascade FK broke sync before): a danglingcategory_idreads as uncategorized. Categories soft-delete; deleting a category never deletes its tags — they become uncategorized.tag_categoryrecord type (pull handler + push service + field-level clock), registered includingENCRYPTABLE_ITEM_TYPESso category names are encrypted on the wire. Category assignment rides on the existingtag_definitionsync.reorder.ts,filter.ts);listTagItemscorrectly spans the dual data/index databases and matches descendants without prefix collisions (work≠workshop).Backward compatibility
tag_categorytype on pull and cannot clobbercategory_id/sort_orderon atag_definitionwrite (field-level merge, tested including explicitnullun-assignment).Testing
ipc:check,i18n:check, architecture, contracts,docs:impact --strict,docs:build.Not yet done
dev:a/dev:b) is a manual check — commands are in the branch notes.use-tag-detail/use-task-tag-detailhooks.TagItemcarries no pinned flag.Design spec and implementation plan are under
docs/superpowers/.