WH-093: Move +tag feature to properties bar styling by edmofro · Pull Request #97 · beyondessential/workhorse

edmofro · 2026-04-14T22:51:41Z

Summary

Restyle the "+tag" feature to sit in properties bar and match nice aesthetic of other properties

Journey

Spec interview begun — assistant reviewed current codebase and identified tag feature location in CardTab.tsx and properties bar in PropertiesBar.tsx
UX design explored — tags as pills in properties bar with visual separation, + button without label, dropdown with multi-select, type-to-search, and inline tag creation
User provided 3 key constraints: tags in separate DB table with FK (future extensibility), no X button for removal, compress overflow. Assistant updated design mockup with overflow handling (+4 chip pattern).
Properties row mockup shows tag pills with overflow handling (+4 pattern), + button, and multi-select dropdown with design system tokens
Mockup spacing reviewed against design system — gap corrected from 4px to 8px to match spec, border-radius confirmed at 5px per tag component spec
Tags resized to 12px font with increased padding for better visual hierarchy in properties bar context
Assistant refactoring tag consumers — updating BoardColumn.tsx to use cardTags, removing unused imports, checking for legacy tag field usage in agent/AI code and card actions

Specs

.workhorse/specs/card-tab.md

Changed files

prisma/schema.prisma
src/app/api/card-detail/route.ts
src/app/api/project-board/route.ts
src/components/BoardColumn.tsx
src/components/CardItem.tsx
src/components/card/CardDetailPage.tsx
src/components/card/CardTab.tsx
src/components/card/TagDropdown.tsx
src/lib/actions/tags.ts
src/lib/hooks/queries.ts

Test plan

Review Hero

Run Review Hero
Auto-fix review suggestions
Auto-fix CI failures

… gap)

Resolve conflicts: - prisma/schema.prisma: keep both Tag relation (from branch) and CardCodeSequence model (from main) - CardTab.tsx: keep TagRef import (from branch) and handleListKeyDown import (from main), drop unused cn import https://claude.ai/code/session_01CK4Moj5ruNjru984fQHGgh

review-hero · 2026-04-14T23:36:28Z

🦸 Review Hero Summary
15 agents reviewed this PR | 2 critical | 8 suggestions | 1 nitpick | Filtering: consensus 3 voters, 7 below threshold

Below consensus threshold (7 unique issues not confirmed by majority)

Location	Agent	Severity	Comment
`src/app/api/card-detail/route.ts:71`	Frontend Snappiness	`suggestion`	The `prisma.tag.findMany` for project tags has no `take` limit. Tags accumulate over time and are never pruned, so a long-lived project could grow to thousands of tags — all of which are serialised...
`src/app/api/project-board/route.ts:40`	Frontend Snappiness	`suggestion`	The board query now includes `cardTags -> tag` for every card across all columns. For a board with 200 cards averaging 3 tags each, that's ~600 extra objects in the JSON payload on every board load...
`src/components/card/CardTab.tsx:72`	Design & Architecture	`suggestion`	There are six individual `useEffect` hooks that each sync a single prop to local state (cardTags, allProjectTags, priority, teamId, assigneeId, plus description via ref). This is a growing pattern ...
`src/components/card/TagDropdown.tsx:1`	Design & Architecture	`suggestion`	This 200-line component handles three distinct concerns: (1) rendering visible tag pills + overflow chip, (2) the search/filter dropdown with checkbox toggle, and (3) the 'create new tag' flow. Con...
`src/components/card/TagDropdown.tsx:79`	Design & Architecture	`suggestion`	The separator `<div>` is baked into `TagDropdown`, coupling it to the properties bar layout. None of the other `PropertyDropdown` instances render their own separators — layout spacing is the paren...
`src/components/card/TagDropdown.tsx:80`	Bugs & Correctness	`suggestion`	The overflow chip uses `onMouseDown={openDropdown}` where `openDropdown` calls `e.stopPropagation()` and guards with `if (!open) toggle()`. When the dropdown is already open, clicking the overflow ...
`src/components/card/TagDropdown.tsx:89`	Project Conventions	`suggestion`	Visible tag pills are rendered as inline `<span>` elements with custom sizing (12px text, 10px horizontal padding, `--radius-md`) instead of using the project's `<Tag>` component. The design system...

Nitpicks

File	Line	Agent	Comment
`src/lib/actions/tags.ts`	7	Design & Architecture	`getProjectTags` is exported but never imported anywhere in the codebase. It's dead code — project tags are already fetched inline in the `card-detail` API route. Remove it or defer adding it until there's a consumer.

Local fix prompt (copy to your coding agent)

Fix these issues identified on the pull request. One commit per issue fixed.

src/lib/actions/tags.ts:17: addTagToCard uses prisma.cardTag.create which throws on duplicate key (P2002). A race condition exists: the UI guard (cardTags.some(...)) reads stale React state, so a fast double-click can fire two addTagToCard calls for the same tag. The first succeeds; the second throws; the catch block rolls back the optimistic state, removing the tag even though it was saved. Use prisma.cardTag.upsert (as createTagAndAddToCard already does on line 47) to make this idempotent.

src/lib/actions/tags.ts:27: removeTagFromCard uses prisma.cardTag.delete which throws P2025 if the record doesn't exist (double-click, concurrent removal by another user, or removing an optimistically-created tag with a temp ID before it's persisted). The catch block in handleTagRemove re-adds the tag to local state, making a deleted tag reappear. Use prisma.cardTag.deleteMany({ where: { cardId, tagId } }) which returns { count: 0 } instead of throwing when the record is absent.

src/components/card/CardTab.tsx:250: If the user toggles off an optimistically-created tag (temp ID like temp-1713100000000) before handleTagCreate's server call resolves, handleTagRemove sends the temp ID to removeTagFromCard — which always fails because it doesn't exist in the DB. The catch rollback re-adds the temp entry. Then when handleTagCreate resolves and maps over state to replace the temp ID, the entry may already have been removed and re-added at the wrong position, potentially yielding a duplicate. Guard against this: in handleTagRemove, skip the server call for temp IDs (e.g. if (tagId.startsWith('temp-')) { setCardTags(prev => prev.filter(...)); return }) and let the pending create handle reconciliation.

src/components/card/TagDropdown.tsx:84: TagDropdown re-implements tag pill rendering with a raw <span> and hardcoded amber styles instead of reusing the existing Tag component from ../Tag. The Tag component already encodes these exact styles (bg-[rgba(180,83,9,0.06)] text-[var(--amber)]) and handles variant logic. Meanwhile BoardColumn.tsx and CardItem.tsx both use <Tag variant={tag.name === 'future' ? 'future' : 'core'}> — this third copy diverges by always using the 'core' look and ignoring the 'future' variant entirely. Reuse <Tag> here to keep tag appearance in one place.

prisma/schema.prisma:73: The codebase now has two parallel representations of the same concept: the legacy tags JSON string and the new Tag/CardTag relation. The UI has fully switched to reading cardTags, but the old field is still queried, serialised in API responses, and passed through component props (e.g. CardData.tags, CardTab card.tags). With no migration script and no code that reads the legacy field for display, any existing JSON-only tags are silently invisible. Consider either adding a data migration and removing the legacy field, or at minimum stop threading the unused tags string through the component tree to avoid confusion about which is the source of truth.

src/lib/actions/tags.ts:7: getProjectTags is exported but never imported anywhere in the codebase. It's dead code — project tags are already fetched inline in the card-detail API route. Remove it or defer adding it until there's a consumer.

src/lib/actions/tags.ts:21: revalidatePath('/') is called on every tag add, remove, and create. This invalidates the server-side cache for the entire app — every layout and page. For a frequent, lightweight interaction like toggling tags, this is expensive: it forces all subsequent page loads to re-render server-side until the cache warms again. Scope the revalidation to the affected paths (e.g. the card detail and board routes) instead of the root.

src/lib/actions/tags.ts:40: createTagAndAddToCard issues two sequential round-trips (upsert Tag, then upsert CardTag). Wrapping them in prisma.$transaction([...]) would save one network round-trip to the database and ensure atomicity — if the second upsert fails, the orphaned Tag row won't be left behind.

src/lib/actions/tags.ts:7: getProjectTags is a server action (in a 'use server' file) but doesn't call requireUser(), unlike the other three functions in this file. If it's meant to be a public query, it should live in a non-action utility; if it needs auth (consistent with the rest of the file), add requireUser(). Currently it's also unused — the card-detail route fetches project tags directly via Prisma.

src/lib/actions/tags.ts:44: No input validation on tag name: the string is passed directly to prisma.tag.upsert with no length or character constraints. Prisma parameterises queries so SQL injection is not a concern, but an attacker could create tags with very long names (e.g. 1MB strings) for storage abuse. Consider trimming and enforcing a reasonable max length (e.g. 100 chars) before the database call.

src/lib/actions/tags.ts:14: Authorization bypass: addTagToCard, removeTagFromCard, and createTagAndAddToCard call requireUser() (authentication) but never verify the user has access to the card's project. The codebase already has a withCardAuth pattern (session.ts:75) that checks hasProjectAccess(user.accessToken, owner, repoName) via the user's GitHub token — the API routes use it but these server actions skip it entirely. Any authenticated user can add/remove tags on any card and create tags in any project by supplying arbitrary IDs. Fix: look up the card's project, then call hasProjectAccess before mutating, similar to withCardAuth.

review-hero · 2026-04-14T23:42:49Z

🦸 Review Hero Summary
15 agents reviewed this PR | 3 critical | 10 suggestions | 0 nitpicks | Filtering: consensus 3 voters, 4 below threshold

Below consensus threshold (4 unique issues not confirmed by majority)

Location	Agent	Severity	Comment
`src/components/card/CardTab.tsx:257`	Bugs & Correctness	`suggestion`	The `handleTagRemove` rollback blindly appends `removed` without checking if it's already in the current state: `setCardTags((prev) => [...prev, removed])`. If the server action fails AND a data re...
`src/components/card/CardTab.tsx:264`	Bugs & Correctness	`suggestion`	Toggling a just-created tag before the server responds sends a `temp-*` ID to `addTagToCard` or `removeTagFromCard`, which will fail with a FK/not-found error. The catch blocks try to roll back, bu...
`src/lib/actions/tags.ts:27`	Bugs & Correctness	`suggestion`	`removeTagFromCard` uses `prisma.cardTag.delete`, which throws RecordNotFound if the relation was already removed (concurrent tab, race). The catch block in `CardTab.handleTagRemove` then re-adds t...
`src/lib/actions/tags.ts:34`	Security	`suggestion`	createTagAndAddToCard() accepts a user-supplied projectId and name with no input validation. While Prisma parameterises queries (no SQL injection), there is no length or character constraint on nam...

Local fix prompt (copy to your coding agent)

Fix these issues identified on the pull request. One commit per issue fixed.

src/lib/actions/tags.ts:17: addTagToCard uses prisma.cardTag.create which throws a unique-constraint error if the CardTag already exists (e.g. concurrent add from another tab, or revalidation race). createTagAndAddToCard already correctly uses upsert for the same operation. Use prisma.cardTag.upsert({ where: { cardId_tagId: { cardId, tagId } }, update: {}, create: { cardId, tagId } }) to make it idempotent — otherwise the thrown error triggers the client's catch block, which rolls back the optimistic add and removes the tag from the UI even though it IS associated on the server.

src/components/card/TagDropdown.tsx:81: The overflow chip uses onMouseDown={openDropdown} which only opens (if (!open) toggle()), while the + button uses onClick={toggle} which opens and closes. If the dropdown is open and the user clicks the overflow chip, propagation is stopped but the dropdown won't close — the user is stuck needing to click elsewhere. Either delegate to the same triggerRef/toggle, or use toggle() unconditionally instead of guarding with if (!open).

src/components/card/TagDropdown.tsx:88: Tag pills are rendered inline here with hardcoded amber styling (bg-[rgba(180,83,9,0.06)] text-[var(--amber)]), duplicating the concept already encapsulated by the <Tag> component used in BoardColumn.tsx and CardItem.tsx. Using <Tag> here too would keep the visual treatment in one place and prevent the two from drifting apart.

src/lib/actions/tags.ts:7: getProjectTags is exported but never imported anywhere. The API route fetches project tags with an inline Prisma query instead. Either remove this dead function or use it in the API route to avoid the duplication.

src/components/card/CardTab.tsx:29: Incomplete migration: CardTab still accepts tags: string (the legacy JSON field) in its CardData interface but never reads it. Meanwhile, src/lib/actions/cards.ts lines 95, 120, 199 still write to the old Card.tags JSON column on create/update, and createCard even accepts tags?: string[] for the legacy format. New tags only go through CardTag — so existing cards' tags are silently orphaned (never migrated, never displayed), and any code path that creates cards with tags via createCard populates only the dead field. This PR should either include a data migration script and stop writing the legacy field, or at minimum document a plan. As-is, users will lose their existing tags with no indication.

src/components/card/CardTab.tsx:68: CardTab now owns three pieces of tag mutation logic (handleTagAdd, handleTagRemove, handleTagCreate) with optimistic state, rollback, and temp-ID reconciliation — roughly 50 lines of domain logic that's unrelated to rendering the card tab. Consider extracting this into a useCardTags(cardId, projectId, initialTags, initialProjectTags) hook, similar to how useAttachments already encapsulates upload state. This would keep CardTab focused on layout and reduce its growing size.

src/app/api/card-detail/route.ts:71: The prisma.tag.findMany for project tags has no take limit. As a project accumulates tags over its lifetime, this query returns an ever-growing result set — all fetched eagerly on every card detail page load. Consider adding a reasonable limit (e.g. take: 500) or, since the TagDropdown already has a search/filter input, loading tags lazily via a separate endpoint with pagination/search rather than bundling all of them into the initial card detail response.

src/app/api/project-board/route.ts:40: Adding cardTags: { include: { tag: ... } } to the board query means every card on the board now triggers an additional join/subquery for its tags. The board already loads all cards for all teams without pagination, so the total data fetched is proportional to cards × tags_per_card. For boards with hundreds of cards this noticeably increases both query time and JSON payload size. Consider whether board cards actually need the full tag list — if only the first 3 are shown (per VISIBLE_TAG_COUNT), you could limit to take: 4 here (3 visible + 1 to know if overflow exists) and fetch the rest on demand.

src/lib/actions/tags.ts:40: createTagAndAddToCard performs two sequential DB round-trips (tag upsert, then cardTag upsert) outside a transaction. Beyond the correctness concern (partial failure leaves inconsistent state), two separate round-trips double the latency for tag creation. Wrapping both in prisma.$transaction([...]) would save one network round-trip to the database and ensure atomicity.

src/components/card/TagDropdown.tsx:90: Tag pills are re-implemented inline instead of using the existing Tag component, with values that diverge from the design system: text-[12px] (should be 11px per type scale), px-[10px] (Tag uses px-2/8px), rounded-[var(--radius-md)]/6px (Tag uses rounded-[5px]). This also drops the 'future' variant — all tags render as amber, but BoardColumn.tsx and CardItem.tsx still distinguish future vs core. The design system states: "The same content should look the same everywhere." Reuse the Tag component here to stay consistent and preserve variant styling.

src/components/card/TagDropdown.tsx:155: py-[6px] and w-[14px] h-[14px] (line 164) are off the 4px spacing grid defined in design-system.md ("All spacing values are multiples of 4"). Use py-2 (8px) or py-1 (4px) for list item padding, and w-4 h-4 (16px) or w-3 h-3 (12px) for the checkbox.

src/lib/actions/tags.ts:14: addTagToCard, removeTagFromCard, and createTagAndAddToCard authenticate the caller (requireUser()) but never verify the caller is authorised for the target card or project — a classic IDOR. Any logged-in user who knows a cardId/tagId can add or remove tags on cards in projects they don't own. The API route (card-detail) does check card.team.project.ownerId !== user.id, but these server actions bypass that gate. Note: the existing card mutation actions (updateCard, deleteCard) have the same gap, so this is a pre-existing pattern — but since these are new actions and new entry points, it's worth fixing here (e.g., load the card, join through team→project, and assert ownership before mutating).

src/lib/actions/tags.ts:7: getProjectTags() has no authentication at all — no requireUser() call. Since this is exported from a 'use server' module, it's a callable server action. Any unauthenticated client can enumerate tag names for any project by supplying an arbitrary projectId. Add requireUser() at minimum.

…nto workhorse/wh-093-spec

…ntial/workhorse into workhorse/wh-093-spec # Conflicts: # src/components/card/CardTab.tsx

review-hero · 2026-04-15T07:03:06Z

🦸 Review Hero Summary
15 agents reviewed this PR | 2 critical | 6 suggestions | 1 nitpick | Filtering: consensus 3 voters, 7 below threshold

Below consensus threshold (7 unique issues not confirmed by majority)

Location	Agent	Severity	Comment
`src/components/card/CardTab.tsx:66`	Design & Architecture	`suggestion`	Tag state (`cardTags`, `allProjectTags`) is duplicated: it's managed locally via `useState` + optimistic updates, but also synced from server data via `useEffect`. This means two sources of truth —...
`src/components/card/TagDropdown.tsx:1`	Design & Architecture	`suggestion`	The tag dropdown duplicates patterns already solved by `PropertyDropdown`: portal positioning via `usePortalMenu`, search/filter, option rendering, and keyboard handling (which is missing here — no...
`src/components/card/TagDropdown.tsx:87`	Design & Architecture	`suggestion`	The hardcoded `tag.name === 'future' ? 'future' : 'core'` variant mapping is duplicated across `TagDropdown` (line 87), `BoardColumn.tsx` `CardContent`, and previously in `CardTab`. Now that tags a...
`src/components/card/TagDropdown.tsx:98`	Project Conventions	`nitpick`	The overflow chip uses onMouseDown while the add button uses onClick. Both are triggers for the same portal dropdown. Using different event types for the same interaction pattern can cause subtle U...
`src/lib/actions/tags.ts:10`	Frontend Snappiness	`suggestion`	`requireCardProjectAccess` fetches card → team → project on every call. Because `addTagToCard` and `removeTagFromCard` are called on each individual toggle in the dropdown (no batching), rapid togg...
`src/lib/actions/tags.ts:48`	Design & Architecture	`suggestion`	`createTagAndAddToCard` accepts `projectId` as a parameter, but `requireCardProjectAccess` (called on line 53) already fetches the card with its project. The `projectId` could be derived from the c...
`src/lib/hooks/queries.ts:71`	Design & Architecture	`suggestion`	The `BoardCard` and `CardDetailData` interfaces still carry the legacy `tags: string` field alongside the new `cardTags: TagRef[]`. No frontend component reads the legacy field any more — `BoardCol...

Nitpicks

File	Line	Agent	Comment
`src/components/card/CardTab.tsx`	182	Frontend Snappiness	The `.sort()` after `prev.map(...)` in the server-response reconciliation branch is unnecessary — only the `id` changes (from temp to real), the `name` stays the same, so the sort order from the optimistic insert is already correct. Removing this avoids an O(n log n) pass on every tag creation.

Local fix prompt (copy to your coding agent)

Fix these issues identified on the pull request. One commit per issue fixed.

src/lib/actions/tags.ts:53: createTagAndAddToCard discards the return value of requireCardProjectAccess(cardId) (which already fetches the card and its project), then uses the untrusted projectId parameter on line 60 to create the tag. A crafted server-action call could pass a projectId that differs from the card's actual project, creating a tag in the wrong project. Fix: destructure const { card } = await requireCardProjectAccess(cardId) and use card.team.projectId instead of the parameter — or at minimum assert they match.

src/components/card/CardTab.tsx:176: Race condition: removing a tag with a temp ID (line 176 early-returns before calling the server) while createTagAndAddToCard is still in-flight. The create will succeed server-side, attaching the tag, but the success handler's setCardTags(prev => prev.map(...)) won't find the temp ID (already filtered out at line 174). Result: tag is attached on the server but absent in the UI — reappears on next page load. Fix: track pending temp IDs (e.g. in a ref/set) so handleTagCreate's success handler can detect the cancellation and call removeTagFromCard to reconcile, or abort the in-flight create via an AbortController.

src/components/card/TagDropdown.tsx:42: cardTagIds is a Set built from cardTags which may contain optimistic temp-* IDs. When the server responds and handleTagCreate replaces the temp ID with the real one, the dropdown's checkbox state (isChecked at line 150) depends on this Set. Between the temp-to-real swap there is a render where neither the old temp ID nor the new real ID is in cardTagIds, causing the checkbox to briefly flash unchecked. More importantly, if the user clicks the tag row during that frame they'll trigger onAdd (re-adding it) instead of onRemove. Consider keying the checked state on tag name as a fallback when temp IDs are present.

src/app/api/project-board/route.ts:39: The legacy tags JSON string column is still selected (tags: true) and serialised into every board card response (line 73), but the board UI (BoardColumn.tsx) now exclusively reads from cardTags. This sends a redundant JSON-encoded string for every card on the board. Removing tags: true from the select and tags: c.tags from the response mapping would trim per-card payload size. The same applies to card-detail/route.ts line 85 — CardTab no longer reads card.tags. Consider dropping both once you confirm no other consumer depends on the legacy field (currently cards.ts actions still write it, but that's the write path, not the read path).

src/app/api/card-detail/route.ts:71: The prisma.tag.findMany for projectTags has no take limit. All project tags are fetched and sent to the client for the tag dropdown. If a project accumulates a large number of tags over time, this query and the resulting payload grow unbounded. Consider adding a reasonable take limit (e.g., 500) or implementing search-based filtering server-side so the client never needs the full set.

src/components/card/CardTab.tsx:182: The .sort() after prev.map(...) in the server-response reconciliation branch is unnecessary — only the id changes (from temp to real), the name stays the same, so the sort order from the optimistic insert is already correct. Removing this avoids an O(n log n) pass on every tag creation.

src/components/card/TagDropdown.tsx:89: Tag pills in the properties row override the design system's tag spec (11px, 2px 8px padding, 5px radius) with larger values (12px, 4px 10px, --radius-md/6px). The design system's 'Consistency across surfaces' principle states 'The same content should look the same everywhere.' Tags on the board cards (BoardColumn.tsx) use the default Tag sizing — these should match. Remove the className override and let the Tag component's built-in styling apply.

src/components/card/TagDropdown.tsx:156: Several spacing values in this file are off the 4px grid: px-[10px] (lines 89, 99), py-[6px] (lines 156, 186), w-[14px] h-[14px] (line 165). The design system specifies 'Base unit: 4px. All spacing values are multiples of 4.' Use 8px or 12px instead of 10px; 4px or 8px instead of 6px; 12px or 16px instead of 14px.

src/lib/actions/tags.ts:26: Cross-project tag association: addTagToCard accepts an arbitrary tagId but does not verify the tag belongs to the same project as the card. A user with access to one project could associate tags from a different project with their cards (the CardTag join table has no project constraint). When the card is later fetched, the foreign project's tag names are exposed. Fix: after the auth check, verify tag.projectId === card.team.projectId before creating the CardTag record.

review-hero · 2026-04-15T07:19:28Z

🦸 Review Hero (could not post inline comments — showing here instead)

src/lib/hooks/queries.ts:71

[Bugs & Correctness] suggestion

Stale type: tags: string remains on both BoardCard (line 71) and CardDetailData (line 131), but the API responses (project-board/route.ts and card-detail/route.ts) no longer include this field — it was replaced by cardTags. At runtime tags is undefined, contradicting the string type. Remove these leftover declarations to keep the types accurate.

src/components/card/CardTab.tsx:202

[Bugs & Correctness] critical

removeTagFromCard() is called inside the setCardTags updater function (line 198-203). React state updater functions must be pure — they should return the new state and do nothing else. Next.js enables React StrictMode in development, which double-invokes updater functions to detect impurities. This will fire the removeTagFromCard server action twice. Fix: hoist the side-effect out of the updater. Compute hasTempEntry via the updater (e.g. using a ref or returning a flag), then call removeTagFromCard outside it.

src/components/card/TagDropdown.tsx:57

[Bugs & Correctness] suggestion

isChecked (line ~145) uses cardTagIds.has(tag.id) || cardTagNames.has(tag.name.toLowerCase()), but handleToggle (line 57-62) only checks cardTagIds.has(tagId). If a tag is visually checked via the name-based fallback (e.g. during the temp-ID → real-ID reconciliation window), clicking it calls onAdd instead of onRemove — the checkbox appears checked but behaves as unchecked. Align the two: either make handleToggle use the same compound check, or remove the cardTagNames fallback from isChecked if the ID-based check is always sufficient.

src/lib/actions/cards.ts:95

[Design & Architecture] suggestion

Card creation (createCard) still writes tags: JSON.stringify(data.tags ?? []) to the legacy JSON column, and updateCard (line 113) still accepts a tags?: string[] parameter. Now that tags are managed via the Tag/CardTag relation and dedicated server actions, these legacy code paths should be cleaned up in this PR to avoid two competing sources of truth. At minimum, stop accepting tags in the create/update signatures so no caller accidentally writes to the old column.

src/lib/actions/tags.ts:30

[Frontend Snappiness] suggestion

Redundant DB roundtrip: requireCardProjectAccess (line 27) already fetches the card with team.project included, giving you card.team.projectId. The separate prisma.tag.findUnique here is an extra query just to check tag.projectId. You could fold this verification into the upsert itself using a nested where that asserts the tag's project matches, or combine the tag lookup with the card lookup in requireCardProjectAccess to eliminate the extra roundtrip.

src/lib/actions/tags.ts:10

[Frontend Snappiness] suggestion

requireCardProjectAccess performs a DB lookup (card → team → project) plus a GitHub API call (hasProjectAccess) on every invocation. Because addTagToCard, removeTagFromCard, and createTagAndAddToCard are independent server actions, rapidly toggling several tags fires that many GitHub roundtrips in parallel. A short-lived per-request or per-session cache on the access-check result would eliminate redundant external API calls during a burst of tag edits.

review-hero · 2026-04-15T07:19:29Z

🦸 Review Hero Summary
12 agents reviewed this PR | 1 critical | 5 suggestions | 2 nitpicks | Filtering: consensus 3 voters, 7 below threshold

Below consensus threshold (7 unique issues not confirmed by majority)

Location	Agent	Severity	Comment
`src/components/card/CardTab.tsx:72`	Design & Architecture	`suggestion`	Tag state (`cardTags`, `allProjectTags`) is duplicated: the component keeps its own `useState` copies that are force-synced from props via `useEffect` (lines 80-81). The other property fields (prio...
`src/components/card/CardTab.tsx:156`	Design & Architecture	`suggestion`	The optimistic create flow (`handleTagCreate`) is ~30 lines handling temp-ID reconciliation, in-flight removal detection, and server-side cleanup on race conditions. This is real complexity solving...
`src/components/card/CardTab.tsx:161`	Bugs & Correctness	`suggestion`	The duplicate guard `cardTags.some((t) => t.id === tagId)` reads from the stale closure state, not the latest state. If a user rapidly clicks the same tag twice before React re-renders, both calls ...
`src/components/card/TagDropdown.tsx:86`	Design & Architecture	`suggestion`	The vertical separator `<div className="w-px h-4 ...">` is baked into TagDropdown, coupling the component to its specific placement context (after PropertyDropdown items in the properties bar). The...
`src/components/card/TagDropdown.tsx:92`	Design & Architecture	`nitpick`	The overflow chip uses `onMouseDown` while the add-tag button beside it uses `onClick`. This inconsistency means they behave differently with focus (mousedown fires before blur, click fires after)....
`src/components/card/TagDropdown.tsx:145`	Frontend Snappiness	`suggestion`	The `filteredTags.map()` renders every matching tag as a full DOM node (button + styled checkbox + text) inside a 240px scrollable container. The API fetches up to 500 project tags (`take: 500` in ...
`src/lib/actions/tags.ts:10`	Design & Architecture	`suggestion`	`requireCardProjectAccess` is a solid auth helper (requireUser + hasProjectAccess), but it lives as a private function in this file while the existing `cards.ts` actions only call `requireUser()` w...

Nitpicks

File	Line	Agent	Comment
`src/app/api/card-detail/route.ts`	71	Frontend Snappiness	All project tags (up to 500) are fetched and serialised to the client on every card-detail load, even though most users won't open the tag dropdown. Consider lazy-loading project tags when the dropdown is first opened instead of bundling them into the initial card-detail payload.
`src/components/card/TagDropdown.tsx`	155	Project Conventions	Dropdown list items use `py-1.5` (6px vertical padding), which breaks the 4px spacing grid defined in the design system. Use `py-1` (4px) or `py-2` (8px) to stay on-grid. Same applies to the create-option button on line 177.

Local fix prompt (copy to your coding agent)

Fix these issues identified on the pull request. One commit per issue fixed.

src/lib/hooks/queries.ts:71: Stale type: tags: string remains on both BoardCard (line 71) and CardDetailData (line 131), but the API responses (project-board/route.ts and card-detail/route.ts) no longer include this field — it was replaced by cardTags. At runtime tags is undefined, contradicting the string type. Remove these leftover declarations to keep the types accurate.

src/components/card/CardTab.tsx:202: removeTagFromCard() is called inside the setCardTags updater function (line 198-203). React state updater functions must be pure — they should return the new state and do nothing else. Next.js enables React StrictMode in development, which double-invokes updater functions to detect impurities. This will fire the removeTagFromCard server action twice. Fix: hoist the side-effect out of the updater. Compute hasTempEntry via the updater (e.g. using a ref or returning a flag), then call removeTagFromCard outside it.

src/components/card/TagDropdown.tsx:57: isChecked (line ~145) uses cardTagIds.has(tag.id) || cardTagNames.has(tag.name.toLowerCase()), but handleToggle (line 57-62) only checks cardTagIds.has(tagId). If a tag is visually checked via the name-based fallback (e.g. during the temp-ID → real-ID reconciliation window), clicking it calls onAdd instead of onRemove — the checkbox appears checked but behaves as unchecked. Align the two: either make handleToggle use the same compound check, or remove the cardTagNames fallback from isChecked if the ID-based check is always sufficient.

src/lib/actions/cards.ts:95: Card creation (createCard) still writes tags: JSON.stringify(data.tags ?? []) to the legacy JSON column, and updateCard (line 113) still accepts a tags?: string[] parameter. Now that tags are managed via the Tag/CardTag relation and dedicated server actions, these legacy code paths should be cleaned up in this PR to avoid two competing sources of truth. At minimum, stop accepting tags in the create/update signatures so no caller accidentally writes to the old column.

src/lib/actions/tags.ts:30: Redundant DB roundtrip: requireCardProjectAccess (line 27) already fetches the card with team.project included, giving you card.team.projectId. The separate prisma.tag.findUnique here is an extra query just to check tag.projectId. You could fold this verification into the upsert itself using a nested where that asserts the tag's project matches, or combine the tag lookup with the card lookup in requireCardProjectAccess to eliminate the extra roundtrip.

src/app/api/card-detail/route.ts:71: All project tags (up to 500) are fetched and serialised to the client on every card-detail load, even though most users won't open the tag dropdown. Consider lazy-loading project tags when the dropdown is first opened instead of bundling them into the initial card-detail payload.

src/lib/actions/tags.ts:10: requireCardProjectAccess performs a DB lookup (card → team → project) plus a GitHub API call (hasProjectAccess) on every invocation. Because addTagToCard, removeTagFromCard, and createTagAndAddToCard are independent server actions, rapidly toggling several tags fires that many GitHub roundtrips in parallel. A short-lived per-request or per-session cache on the access-check result would eliminate redundant external API calls during a burst of tag edits.

src/components/card/TagDropdown.tsx:155: Dropdown list items use py-1.5 (6px vertical padding), which breaks the 4px spacing grid defined in the design system. Use py-1 (4px) or py-2 (8px) to stay on-grid. Same applies to the create-option button on line 177.

review-hero · 2026-04-15T07:38:19Z

-    }
-    commitTag()
-  }
+  function handleTagCreate(name: string) {


[Design & Architecture] suggestion

The temp-ID optimistic lifecycle is split across three functions (handleTagCreate, handleTagRemove, handleTagAdd) and leaks into TagDropdown via the cardTagNames set. Consider extracting the tag optimistic state into a custom hook (e.g. useOptimisticTags) that encapsulates temp-ID creation, reconciliation, and rollback. This would co-locate the related logic, keep CardTab consistent with how it handles other properties, and let TagDropdown work purely with stable IDs.

review-hero · 2026-04-15T07:38:19Z

+        const tag = await createTagAndAddToCard(card.id, name)
+        // Replace temp entry with real one — if the temp ID was removed
+        // (user toggled it off while create was in-flight), remove server-side too
+        let needsServerCleanup = false


[Design & Architecture] suggestion

The needsServerCleanup flag set inside a setCardTags updater and read outside it is a side-channel that couples the cleanup decision to React's synchronous updater execution. A clearer approach: use a ref (e.g. removedTempIds) that handleTagRemove writes to, and check it here after the await. That keeps the state setter pure and makes the control flow explicit.

review-hero · 2026-04-15T07:38:19Z

+    setSearch('')
+  }
+
+  function openDropdown(e: React.MouseEvent) {


[Design & Architecture] suggestion

stopPropagation on the overflow chip's click is a workaround for usePortalMenu's click-outside handler firing on sibling elements. This creates a fragile coupling — any future trigger element near the dropdown will need the same workaround. Consider making usePortalMenu's click-outside handler ignore clicks on elements that are children of the same logical container, or pass a containsRef so it can exclude known siblings.

review-hero · 2026-04-15T07:38:19Z

+
+const MAX_TAG_NAME_LENGTH = 100
+
+async function requireCardProjectAccess(cardId: string) {


[Frontend Snappiness] suggestion

Every tag mutation (add, remove, create) independently calls requireCardProjectAccess, which loads the card with nested team+project joins AND makes an external GitHub API call via hasProjectAccess. When a user rapidly toggles several tags in the dropdown, each toggle fires a separate server action, so toggling 5 tags triggers 5 redundant card lookups and 5 GitHub HTTP round-trips for the same card and the same user. Consider accepting a batch of tag changes in a single action, or at minimum caching the project-access check for the duration of a request (e.g. a per-request memo keyed on userId+projectId).

review-hero · 2026-04-15T07:38:20Z

🦸 Review Hero Summary
11 agents reviewed this PR | 0 critical | 4 suggestions | 1 nitpick | Filtering: consensus 3 voters, 10 below threshold

Below consensus threshold (10 unique issues not confirmed by majority)

Location	Agent	Severity	Comment
`src/app/api/card-detail/route.ts:71`	Frontend Snappiness	`suggestion`	Up to 500 project tags are eagerly fetched on every card detail page load, even though they're only needed if the user opens the TagDropdown. For the common case (viewing a card without editing tag...
`src/app/api/project-board/route.ts:39`	Frontend Snappiness	`nitpick`	Previously tags were a JSON string column read in the same query as the card. Now `cardTags: { include: { tag: ... } }` causes Prisma to issue two additional SQL queries (one for the CardTag join t...
`src/components/card/CardTab.tsx:161`	Bugs & Correctness	`suggestion`	`handleTagAdd` guards against duplicates using `cardTags.some(...)` which reads from the closure (current render's state), but the actual append uses the updater form `setCardTags((prev) => [...pre...
`src/components/card/CardTab.tsx:177`	Bugs & Correctness	`critical`	Race condition: `handleTagCreate`'s `needsServerCleanup` logic assumes that a missing temp entry means the user removed the tag during flight. But a React Query background refetch (e.g. triggered b...
`src/components/card/CardTab.tsx:180`	Bugs & Correctness	`suggestion`	The rollback in `handleTagRemove`'s catch clause unconditionally appends `removed` back: `setCardTags((prev) => [...prev, removed])`. If the server call fails but the user has already re-added the ...
`src/components/card/TagDropdown.tsx:58`	Design & Architecture	`suggestion`	handleToggle and isChecked (line 150) fall back to matching by `cardTagNames` (name-based) when `cardTagIds` (ID-based) doesn't match. This is there to handle the temp-ID window during optimistic c...
`src/components/card/TagDropdown.tsx:68`	Bugs & Correctness	`suggestion`	`cardTagNames` uses case-insensitive matching (`.toLowerCase()`) but the Prisma schema's `@@unique([projectId, name])` uses PostgreSQL's default case-sensitive collation. If the project has tags "F...
`src/components/card/TagDropdown.tsx:81`	Design & Architecture	`nitpick`	The hard-coded separator `<div className="w-px h-4 ...">` couples TagDropdown to its container's flex layout. If this component is ever reused or the properties bar layout changes, the separator be...
`src/components/card/TagDropdown.tsx:81`	Project Conventions	`nitpick`	The vertical separator uses `bg-[var(--border-default)]` but all other inline separators in the codebase (including this file's own internal separator on line 182, and `PrSection.tsx`) use `bg-[var...
`src/components/card/TagDropdown.tsx:151`	Project Conventions	`suggestion`	Dropdown item vertical padding is `py-1` (4px), but PropertyDropdown uses `py-2` (8px) for its menu items. The design system warns against inconsistency: 'Using different font sizes, weights, or co...

Nitpicks

File	Line	Agent	Comment
`src/components/card/TagDropdown.tsx`	93	Project Conventions	The magic string check `tag.name === 'future'` is duplicated here and in BoardColumn.tsx (line 120). Now that tags are first-class Tag entities with their own model, consider centralising the variant mapping (e.g. a small helper like `tagVariant(name: string)`) so the 'future' convention lives in...

Local fix prompt (copy to your coding agent)

Fix these issues identified on the pull request. One commit per issue fixed.

src/components/card/CardTab.tsx:186: The temp-ID optimistic lifecycle is split across three functions (handleTagCreate, handleTagRemove, handleTagAdd) and leaks into TagDropdown via the cardTagNames set. Consider extracting the tag optimistic state into a custom hook (e.g. useOptimisticTags) that encapsulates temp-ID creation, reconciliation, and rollback. This would co-locate the related logic, keep CardTab consistent with how it handles other properties, and let TagDropdown work purely with stable IDs.

src/components/card/CardTab.tsx:198: The needsServerCleanup flag set inside a setCardTags updater and read outside it is a side-channel that couples the cleanup decision to React's synchronous updater execution. A clearer approach: use a ref (e.g. removedTempIds) that handleTagRemove writes to, and check it here after the await. That keeps the state setter pure and makes the control flow explicit.

src/components/card/TagDropdown.tsx:72: stopPropagation on the overflow chip's click is a workaround for usePortalMenu's click-outside handler firing on sibling elements. This creates a fragile coupling — any future trigger element near the dropdown will need the same workaround. Consider making usePortalMenu's click-outside handler ignore clicks on elements that are children of the same logical container, or pass a containsRef so it can exclude known siblings.

src/lib/actions/tags.ts:10: Every tag mutation (add, remove, create) independently calls requireCardProjectAccess, which loads the card with nested team+project joins AND makes an external GitHub API call via hasProjectAccess. When a user rapidly toggles several tags in the dropdown, each toggle fires a separate server action, so toggling 5 tags triggers 5 redundant card lookups and 5 GitHub HTTP round-trips for the same card and the same user. Consider accepting a batch of tag changes in a single action, or at minimum caching the project-access check for the duration of a request (e.g. a per-request memo keyed on userId+projectId).

src/components/card/TagDropdown.tsx:93: The magic string check tag.name === 'future' is duplicated here and in BoardColumn.tsx (line 120). Now that tags are first-class Tag entities with their own model, consider centralising the variant mapping (e.g. a small helper like tagVariant(name: string)) so the 'future' convention lives in one place.

edmofro and others added 9 commits April 13, 2026 19:52

WH-093: move +tag button into properties bar row

090d600

WH-093: add tag management mockup to properties bar

16bf764

WH-093: update tag-properties-row spacing to match design system (8px…

e3a6259

… gap)

WH-093: increase tag sizing in properties row for visual balance

00bf54e

WH-093: move tag feature to properties bar in card detail

1ef6e9e

WH-093: move +tag feature from card tabs to properties bar

0616d59

WH-093: Move +tag feature to properties bar styling

77248a8

WH-093: move +tag feature from card tab to properties bar styling

c552240

review-hero bot reviewed Apr 14, 2026

View reviewed changes

edmofro and others added 5 commits April 14, 2026 23:43

WH-093: Move +tag feature to properties bar with race condition fix

9332fae

WH-093: move +tag feature to properties bar styling

3accd48

WH-093: Move +tag feature to properties bar styling

e3b3b5e

Merge branch 'main' of https://github.com/beyondessential/workhorse i…

257ccb3

…nto workhorse/wh-093-spec

Merge branch 'workhorse/wh-093-spec' of https://github.com/beyondesse…

c8e63db

…ntial/workhorse into workhorse/wh-093-spec # Conflicts: # src/components/card/CardTab.tsx

review-hero bot reviewed Apr 15, 2026

View reviewed changes

WH-093: move +tag feature to properties bar styling

7664973

WH-093: move tag feature to properties bar styling

8659f51

review-hero bot reviewed Apr 15, 2026

View reviewed changes

edmofro merged commit e470cb1 into main Apr 15, 2026
15 of 19 checks passed


		const MAX_TAG_NAME_LENGTH = 100

		async function requireCardProjectAccess(cardId: string) {

Conversation

edmofro commented Apr 14, 2026 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Journey

Specs

Changed files

Test plan

Review Hero

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

review-hero bot commented Apr 14, 2026

Nitpicks

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

review-hero bot commented Apr 14, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

review-hero bot commented Apr 15, 2026

Nitpicks

Uh oh!

review-hero bot commented Apr 15, 2026

Uh oh!

review-hero bot commented Apr 15, 2026

Nitpicks

Uh oh!

review-hero bot Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

review-hero bot Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

review-hero bot Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

review-hero bot Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

review-hero bot commented Apr 15, 2026

Nitpicks

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

edmofro commented Apr 14, 2026 •

edited by github-actions bot

Loading