feat: implement frontend tags management system 187 #192
Conversation
github-actions
bot
added
area:frontend:web
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a complete frontend Tags module following hexagonal architecture: domain models/schemas/repositories and use cases; application composables/services with DI; infrastructure (API, DI container/initialization, Pinia store, services); Vue components/views; routing and navigation integration; i18n keys; extensive tests; minor subscribers DI/routing refactor; main.ts bootstrapping. Changes
Sequence Diagram(s)sequenceDiagram
participant App as App (main.ts)
participant TagDI as Tags DI (initialization)
participant Store as TagStore (Pinia)
participant UC as UseCases (Fetch/Create/Update/Delete)
participant Repo as TagRepository (TagApi)
App->>TagDI: configureTagServiceProvider(DefaultTagServiceProvider)
App->>TagDI: configureStoreFactory(() => useTagStore())
App->>TagDI: initializeTagsModule()
TagDI->>UC: createUseCases(repository)
TagDI->>Store: initializeStore(useCases)
UC->>Repo: wired to TagApi
sequenceDiagram
participant User as User
participant Page as TagPage.vue
participant Comp as TagForm/TagList/DeleteConfirmation
participant AppSrv as useTags (TagService)
participant Store as TagStore
participant UC as UseCases
participant Repo as TagApi (HTTP)
User->>Page: Navigate to /audience/tags
Page->>AppSrv: fetchTags()
AppSrv->>Store: fetchTags()
Store->>UC: FetchTags.execute()
UC->>Repo: GET /api/.../tags
Repo-->>UC: Tag[]
UC-->>Store: Tag[]
Store-->>AppSrv: state updated
AppSrv-->>Page: tags/isLoading/error
User->>Comp: Submit create/edit/delete
Comp->>Page: emit submit/edit/delete
Page->>AppSrv: createTag/updateTag/deleteTag
AppSrv->>Store: action call
Store->>UC: corresponding use case
UC->>Repo: POST/PATCH/DELETE
Repo-->>UC: result/void
UC-->>Store: updated Tag(s)
Store-->>AppSrv: state updated
AppSrv-->>Page: update UI
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Deploying hatchgrid-landing-page with
|
| Latest commit: |
654a90f
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://df57ddc8.hatchgrid.pages.dev |
| Branch Preview URL: | https://feature-implement-frontend-t.hatchgrid.pages.dev |
There was a problem hiding this comment.
Pull Request Overview
This pull request introduces a comprehensive tags management system for the frontend, implementing a hexagonal architecture pattern consistent with the existing codebase. The system provides full CRUD functionality with Vue.js components, state management, API integration, and thorough test coverage.
- Implementation of tags domain with models, use cases, and repository patterns following clean architecture principles
- Complete Vue.js component library for tag management (forms, lists, modals) with accessibility features
- Pinia store integration with error handling and loading states
- Comprehensive test coverage for domain logic and Vue components
Reviewed Changes
Copilot reviewed 76 out of 77 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/src/content/docs/structure.md | Updated documentation to reflect new tag module structure |
| client/apps/web/src/tag/* | Complete tag module implementation with hexagonal architecture |
| TagForm.vue | Main form component for creating/editing tags with validation |
| TagList.vue | Component for displaying and managing lists of tags |
| TagPage.vue | Main page component orchestrating tag management functionality |
Comments suppressed due to low confidence (3)
client/apps/web/src/tag/infrastructure/views/components/TagForm.vue:1
- [nitpick] Avoid using console.error for production error logging. Consider using a proper logging service or error tracking system.
<script setup lang="ts">
client/apps/web/src/tag/infrastructure/views/components/TagForm.vue:1
- [nitpick] Avoid using console.error for production error logging. Consider using a proper logging service or error tracking system.
<script setup lang="ts">
client/apps/web/src/tag/domain/usecases/shared/ValidationUtils.ts:1
- [nitpick] This TODO comment indicates awareness of the need for proper logging. Consider implementing a proper logging service instead of console.warn for production code.
/**
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| createdTags.push(tag); | ||
| } catch (err) { | ||
| // Log error but continue with other tags | ||
| console.warn(`Failed to create tag ${tagData.name}:`, err); |
There was a problem hiding this comment.
[nitpick] Avoid using console.warn for production error logging. Consider using a proper logging service or error tracking system.
|
|
||
| try { | ||
| const response = await axios.post<TagResponse>( | ||
| `${this.baseUrl}${TagApi.ENDPOINTS.tags(workspaceId as string)}/${crypto.randomUUID()}`, |
There was a problem hiding this comment.
Using crypto.randomUUID() in the URL for creating tags is incorrect. The ID should be generated by the backend, not included in the URL path for POST requests.
| `${this.baseUrl}${TagApi.ENDPOINTS.tags(workspaceId as string)}/${crypto.randomUUID()}`, | |
| `${this.baseUrl}${TagApi.ENDPOINTS.tags(workspaceId as string)}`, |
| return true; | ||
| } catch (repositoryError) { | ||
| // Repository errors should be logged but treated as "cannot delete" | ||
| console.error("Repository error in canDelete:", repositoryError); |
There was a problem hiding this comment.
[nitpick] Avoid using console.error for production error logging. Consider using a proper logging service or error tracking system.
Deploying hatchgrid-docs with
|
| Latest commit: |
654a90f
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://fa323d26.hatchgrid-4b2.pages.dev |
| Branch Preview URL: | https://feature-implement-frontend-t.hatchgrid-4b2.pages.dev |
…anagement-System-187
Deploying hatchgrid-web-app with
|
| Latest commit: |
654a90f
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://baeee67a.hatchgrid-web-app.pages.dev |
| Branch Preview URL: | https://feature-implement-frontend-t.hatchgrid-web-app.pages.dev |
Qodana for JVMIt seems all right 👌 No new problems were found according to the checks applied 💡 Qodana analysis was run in the pull request mode: only the changed files were checked Contact Qodana teamContact us at [email protected]
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 149
🔭 Outside diff range comments (5)
client/apps/web/src/shared/config/navigation.test.ts (1)
44-46: Store mock path mismatch — tests won’t intercept the store used by navigation.tsnavigation.ts imports the store from "@/authentication/infrastructure/store", but this test mocks "@/authentication/infrastructure/store/useAuthStore". As a result, the mock won’t be applied and tests may pull the real store.
Update the mock path to match the import used by navigation.ts:
-vi.mock("@/authentication/infrastructure/store/useAuthStore", () => ({ - useAuthStore: () => mockAuthStore, -})); +vi.mock("@/authentication/infrastructure/store", () => ({ + useAuthStore: () => mockAuthStore, +}));client/apps/web/src/tag/domain/usecases/shared/TagErrors.ts (3)
1-56: Name collision with store’sTagErrorinterface.
client/apps/web/src/tag/infrastructure/store/tag.store.tsexports an interface namedTagErrorthat conflicts with this domain class. This will create confusing imports and possible re-export ambiguity.Proposed fix (outside this file):
- Rename the store interface to
TagStoreError, or- Compose from the domain error via an augmentation:
// client/apps/web/src/tag/infrastructure/store/tag.store.ts import type { TagError as DomainTagError } from "@/tag/domain/usecases/shared/TagErrors"; export interface TagStoreError extends DomainTagError { readonly timestamp: Date; }Then update imports/exports in the store barrel to use
TagStoreError.
1-56: Optional: Add atoJSON()for safe serialization (store/logging).This helps when persisting errors in the store or sending telemetry.
export abstract class TagError extends Error { ... + toJSON() { + return { + name: this.name, + message: this.message, + code: this.code, + details: this.details ?? null, + }; + } }
1-56: Consolidate Tag Error Definitions Across LayersWe’ve identified duplicate error class definitions and a conflicting interface, which can lead to ambiguity and type mismatches. Please unify all Tag-related errors in one place and have downstream code import them:
• Duplicate classes in API layer:
– client/apps/web/src/tag/infrastructure/api/TagApi.ts defines bothTagValidationError(lines 29–37) andTagNotFoundError(lines 39–47).
• Domain definitions already exist here:
– client/apps/web/src/tag/domain/usecases/shared/TagErrors.ts definesTagValidationError,TagNotFoundError, and the abstract baseTagError.
• Store layer conflict:
– client/apps/web/src/tag/infrastructure/store/tag.store.ts declares anexport interface TagError, shadowing the class of the same name.Action items:
- Remove the duplicated classes in TagApi.ts; import and re-throw the domain errors from TagErrors.ts instead.
- Resolve the interface/class name collision in tag.store.ts (e.g. rename the interface or use the existing
TagErrorclass).- Update all
throw new …Errorsites in both API and store layers to reference the centralized definitions..kiro/specs/frontend-tags-implementation/requirements.md (1)
95-102: Add architecture/test acceptance criteria for Offline First.To align with repo guidelines, specify ACs for offline behavior: cache, local mutations queue, and retry.
Examples:
- When offline, tag list is served from IndexedDB cache.
- Create/update/delete queue locally and sync on reconnect with rollback on conflicts.
- Tests cover offline flows using MSW and fake timers.
♻️ Duplicate comments (4)
client/apps/web/src/tag/domain/models/schemas.test.ts (1)
23-24: .ts import extensions used in tests — keep consistent with TS config.Same caveat as noted elsewhere: using “.ts” in import specifiers can break if TS flags aren’t set. Keep it if your config supports it, or drop extensions consistently.
See the verification script attached to the related comment in: client/apps/web/src/tag/infrastructure/store/index.ts (Line 8-14).
client/apps/web/src/tag/domain/usecases/DeleteTag.ts (1)
127-131: Replace console.error with structured logging (duplicate of prior feedback)Use a proper logger/error tracking instead of
console.errorin production paths.Suggested interim guard:
- console.error("Repository error in canDelete:", repositoryError); + if (import.meta?.env?.DEV) { + // eslint-disable-next-line no-console + console.error("Repository error in canDelete:", repositoryError); + }client/apps/web/src/tag/infrastructure/store/tag.store.ts (1)
287-294: Replace console.warn with structured logging or store error handlingUsing console.warn in production paths hinders observability and contradicts prior guidance.
Consider:
- Injecting a logger and logging contextual metadata.
- Or accumulating per-item errors and exposing them via store error state for the UI.
Example minimal change:
-console.warn(`Failed to create tag ${tagData.name}:`, err); +// TODO: inject a logger and capture this error with context +// logger.warn('TAG_CREATE_FAILED', { name: tagData.name, error: String(err) });client/apps/web/src/tag/infrastructure/api/TagApi.ts (1)
254-261: Stop sending a random UUID in the POST path; POST to the collection URLUsing crypto.randomUUID() in the URL with POST is incorrect and conflicts with REST and offline-first patterns (which would use PUT with client-generated IDs). Post to the collection and let the backend assign an ID.
-const response = await axios.post<TagResponse>( - `${this.baseUrl}${TagApi.ENDPOINTS.tags(workspaceId as string)}/${crypto.randomUUID()}`, - tag, - { - withCredentials: true, - }, -); +const response = await axios.post<TagResponse>( + `${this.baseUrl}${TagApi.ENDPOINTS.tags(workspaceId as string)}`, + tag, + { withCredentials: true }, +);If the backend requires client-generated IDs for offline-first, switch to PUT at
/workspace/{workspaceId}/tag/{id}(with a deterministic UUID provided by the caller) and adjust the CreateTag use case accordingly.#!/bin/bash # Verify backend routes for tag creation to confirm POST vs PUT and path shape rg -nP -C3 '(workspace.*/tag|/tags?)' server --hidden || true rg -nP -C3 '(POST|PUT).*(tag|tags)' server --hidden || true
| export class DefaultTagServiceProvider implements TagServiceProvider { | ||
| private serviceInstance: TagServiceImpl | null = null; | ||
|
|
||
| getTagService(): TagServiceImpl { | ||
| if (!this.serviceInstance) { | ||
| this.serviceInstance = new TagServiceImpl(); | ||
| } | ||
| return this.serviceInstance; | ||
| } | ||
|
|
||
| /** | ||
| * Reset the service instance (for testing) | ||
| */ | ||
| reset(): void { | ||
| this.serviceInstance = null; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Return the interface type to decouple consumers from the concrete impl.
The class implements TagServiceProvider (which returns TagService). Returning TagServiceImpl works structurally but tightens coupling unnecessarily. Return TagService and keep the cached field typed as TagService as well.
Apply this diff:
-import type { TagServiceProvider } from "../../application/services/TagService";
+import type {
+ TagService,
+ TagServiceProvider,
+} from "../../application/services/TagService";
import { TagServiceImpl } from "./TagServiceImpl";
@@
export class DefaultTagServiceProvider implements TagServiceProvider {
- private serviceInstance: TagServiceImpl | null = null;
+ private serviceInstance: TagService | null = null;
- getTagService(): TagServiceImpl {
+ getTagService(): TagService {
if (!this.serviceInstance) {
this.serviceInstance = new TagServiceImpl();
}
return this.serviceInstance;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export class DefaultTagServiceProvider implements TagServiceProvider { | |
| private serviceInstance: TagServiceImpl | null = null; | |
| getTagService(): TagServiceImpl { | |
| if (!this.serviceInstance) { | |
| this.serviceInstance = new TagServiceImpl(); | |
| } | |
| return this.serviceInstance; | |
| } | |
| /** | |
| * Reset the service instance (for testing) | |
| */ | |
| reset(): void { | |
| this.serviceInstance = null; | |
| } | |
| } | |
| import type { | |
| TagService, | |
| TagServiceProvider, | |
| } from "../../application/services/TagService"; | |
| import { TagServiceImpl } from "./TagServiceImpl"; | |
| export class DefaultTagServiceProvider implements TagServiceProvider { | |
| private serviceInstance: TagService | null = null; | |
| getTagService(): TagService { | |
| if (!this.serviceInstance) { | |
| this.serviceInstance = new TagServiceImpl(); | |
| } | |
| return this.serviceInstance; | |
| } | |
| /** | |
| * Reset the service instance (for testing) | |
| */ | |
| reset(): void { | |
| this.serviceInstance = null; | |
| } | |
| } |
🤖 Prompt for AI Agents
client/apps/web/src/tag/infrastructure/services/TagServiceProvider.ts around
lines 11 to 27: the provider currently caches and returns the concrete
TagServiceImpl which unnecessarily couples consumers to the implementation;
change the cached field type to TagService (private serviceInstance: TagService
| null = null), change the getTagService() signature to return TagService, and
keep instantiating new TagServiceImpl() when serviceInstance is null; ensure any
imports/types are updated accordingly and leave reset() to clear the
serviceInstance to null.
| export { | ||
| type LoadingStates, | ||
| type TagError, | ||
| type TagStore, | ||
| type TagStoreState, | ||
| useTagStore, | ||
| } from "./tag.store.ts"; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider removing the .ts extension in re-exports (or verify TS config supports it).
If your TS config does not enable allowImportingTsExtensions/verbatimModuleSyntax, importing from "./tag.store.ts" can fail under tsc or certain tooling. Prefer extensionless internal imports for TS or confirm the config is set.
Apply this diff if you prefer the extensionless style:
export {
type LoadingStates,
type TagError,
type TagStore,
type TagStoreState,
useTagStore,
-} from "./tag.store.ts";
+} from "./tag.store";Run to verify config and scan for .ts extension imports:
🏁 Script executed:
#!/bin/bash
# Check TS config flags that allow .ts extensions in imports
rg -nP 'allowImportingTsExtensions|verbatimModuleSyntax|moduleResolution' -- tsconfig*.json **/tsconfig*.json -C2 || true
# List TS/JS imports that include ".ts" explicitly (sanity check)
rg -nP 'from\s+["\'].*\.ts["\']' --type=ts --type=tsx --type=vue -C1 || trueLength of output: 341
I didn’t get any TS-config flags or .ts-extension imports from the last run—let’s try a more robust check:
🏁 Script executed:
#!/bin/bash
# 1) For each tsconfig*.json, search for flags that allow .ts extensions
fd -HI tsconfig.json -x bash -lc '
echo "=== {} ==="
rg -nP "(allowImportingTsExtensions|verbatimModuleSyntax)" -C2 {}
' || true
# 2) List any explicit “.ts” imports in TS/JS/Vue files
grep -R -n "from .*\.ts" . --include="*.ts" --include="*.tsx" --include="*.vue" || trueLength of output: 11549
Let’s inspect the Web app’s tsconfig to see if it enables .ts imports (or extends a config that does):
🏁 Script executed:
#!/bin/bash
# Dump the Web app tsconfig
echo "===== client/apps/web/tsconfig.json ====="
sed -n '1,200p' client/apps/web/tsconfig.json || true
# Show any “extends” fields so we can follow inheritance
echo
echo "=== extends entries ==="
grep -n '"extends"' client/apps/web/tsconfig.json || trueLength of output: 578
Let’s check the base configs to see if .ts imports are allowed downstream:
🏁 Script executed:
#!/bin/bash
echo "=== client/config/tsconfig.json ==="
sed -n '1,200p' client/config/tsconfig.json || true
echo
echo "=== client/apps/web/tsconfig.app.json ==="
sed -n '1,200p' client/apps/web/tsconfig.app.json || true
echo
echo "=== client/apps/web/tsconfig.node.json ==="
sed -n '1,200p' client/apps/web/tsconfig.node.json || trueLength of output: 1975
Remove .ts extension in re-export
The Web app’s tsconfig.app.json (used by tsc for your browser code) does not enable allowImportingTsExtensions or verbatimModuleSyntax, so from "./tag.store.ts" will fail under tsc. Update this re-export to the extensionless path:
• client/apps/web/src/tag/infrastructure/store/index.ts, line 14
-} from "./tag.store.ts";
+} from "./tag.store";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export { | |
| type LoadingStates, | |
| type TagError, | |
| type TagStore, | |
| type TagStoreState, | |
| useTagStore, | |
| } from "./tag.store.ts"; | |
| export { | |
| type LoadingStates, | |
| type TagError, | |
| type TagStore, | |
| type TagStoreState, | |
| useTagStore, | |
| } from "./tag.store"; |
🤖 Prompt for AI Agents
In client/apps/web/src/tag/infrastructure/store/index.ts around lines 8 to 14,
the re-export uses a .ts extension which breaks tsc because the app tsconfig
doesn't allow importing .ts extensions; remove the ".ts" extension and re-export
from the extensionless module path (change from "./tag.store.ts" to
"./tag.store") so the import resolves under the current tsconfig.
| const getTagColorClasses = (color: string) => { | ||
| const colorMap = { | ||
| red: "bg-red-100 text-red-800 dark:bg-red-900/20 dark:text-red-400", | ||
| green: | ||
| "bg-green-100 text-green-800 dark:bg-green-900/20 dark:text-green-400", | ||
| blue: "bg-blue-100 text-blue-800 dark:bg-blue-900/20 dark:text-blue-400", | ||
| yellow: | ||
| "bg-yellow-100 text-yellow-800 dark:bg-yellow-900/20 dark:text-yellow-400", | ||
| purple: | ||
| "bg-purple-100 text-purple-800 dark:bg-purple-900/20 dark:text-purple-400", | ||
| gray: "bg-gray-100 text-gray-800 dark:bg-gray-900/20 dark:text-gray-400", | ||
| } as const; | ||
|
|
||
| return colorMap[color as keyof typeof colorMap] || colorMap.gray; | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Avoid re-allocating the color map on every call; hoist and type it once
The color map is rebuilt on every invocation. Hoist it to module scope and type the keys to catch typos at compile time and provide a safe fallback.
Apply this diff:
-/**
- * Get color classes for tag display
- */
-const getTagColorClasses = (color: string) => {
- const colorMap = {
- red: "bg-red-100 text-red-800 dark:bg-red-900/20 dark:text-red-400",
- green:
- "bg-green-100 text-green-800 dark:bg-green-900/20 dark:text-green-400",
- blue: "bg-blue-100 text-blue-800 dark:bg-blue-900/20 dark:text-blue-400",
- yellow:
- "bg-yellow-100 text-yellow-800 dark:bg-yellow-900/20 dark:text-yellow-400",
- purple:
- "bg-purple-100 text-purple-800 dark:bg-purple-900/20 dark:text-purple-400",
- gray: "bg-gray-100 text-gray-800 dark:bg-gray-900/20 dark:text-gray-400",
- } as const;
-
- return colorMap[color as keyof typeof colorMap] || colorMap.gray;
-};
+/**
+ * Map tag color to Tailwind classes (dark mode aware)
+ */
+const COLOR_CLASS_MAP = {
+ red: "bg-red-100 text-red-800 dark:bg-red-900/20 dark:text-red-400",
+ green: "bg-green-100 text-green-800 dark:bg-green-900/20 dark:text-green-400",
+ blue: "bg-blue-100 text-blue-800 dark:bg-blue-900/20 dark:text-blue-400",
+ yellow: "bg-yellow-100 text-yellow-800 dark:bg-yellow-900/20 dark:text-yellow-400",
+ purple: "bg-purple-100 text-purple-800 dark:bg-purple-900/20 dark:text-purple-400",
+ gray: "bg-gray-100 text-gray-800 dark:bg-gray-900/20 dark:text-gray-400",
+} as const;
+type KnownTagColor = keyof typeof COLOR_CLASS_MAP;
+
+/**
+ * Get color classes for tag display
+ */
+const getTagColorClasses = (color: string) =>
+ COLOR_CLASS_MAP[(color as KnownTagColor)] ?? COLOR_CLASS_MAP.gray;If you have a TagColor type/enum in the domain, prefer using it over string.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const getTagColorClasses = (color: string) => { | |
| const colorMap = { | |
| red: "bg-red-100 text-red-800 dark:bg-red-900/20 dark:text-red-400", | |
| green: | |
| "bg-green-100 text-green-800 dark:bg-green-900/20 dark:text-green-400", | |
| blue: "bg-blue-100 text-blue-800 dark:bg-blue-900/20 dark:text-blue-400", | |
| yellow: | |
| "bg-yellow-100 text-yellow-800 dark:bg-yellow-900/20 dark:text-yellow-400", | |
| purple: | |
| "bg-purple-100 text-purple-800 dark:bg-purple-900/20 dark:text-purple-400", | |
| gray: "bg-gray-100 text-gray-800 dark:bg-gray-900/20 dark:text-gray-400", | |
| } as const; | |
| return colorMap[color as keyof typeof colorMap] || colorMap.gray; | |
| }; | |
| /** | |
| * Map tag color to Tailwind classes (dark mode aware) | |
| */ | |
| const COLOR_CLASS_MAP = { | |
| red: "bg-red-100 text-red-800 dark:bg-red-900/20 dark:text-red-400", | |
| green: "bg-green-100 text-green-800 dark:bg-green-900/20 dark:text-green-400", | |
| blue: "bg-blue-100 text-blue-800 dark:bg-blue-900/20 dark:text-blue-400", | |
| yellow: "bg-yellow-100 text-yellow-800 dark:bg-yellow-900/20 dark:text-yellow-400", | |
| purple: "bg-purple-100 text-purple-800 dark:bg-purple-900/20 dark:text-purple-400", | |
| gray: "bg-gray-100 text-gray-800 dark:bg-gray-900/20 dark:text-gray-400", | |
| } as const; | |
| type KnownTagColor = keyof typeof COLOR_CLASS_MAP; | |
| /** | |
| * Get color classes for tag display | |
| */ | |
| const getTagColorClasses = (color: string) => | |
| COLOR_CLASS_MAP[(color as KnownTagColor)] ?? COLOR_CLASS_MAP.gray; |
| <template> | ||
| <div | ||
| class="flex items-center space-x-4 p-4 border rounded-lg hover:bg-muted/50 focus-within:ring-2 focus-within:ring-primary/20 transition-colors" | ||
| data-testid="tag-item" | ||
| > | ||
| <!-- Tag Color Badge --> | ||
| <div class="flex-shrink-0"> | ||
| <span | ||
| :class="getTagColorClasses(tag.color)" | ||
| class="inline-flex items-center px-3 py-1 rounded-full text-sm font-medium" | ||
| data-testid="tag-badge" | ||
| > | ||
| {{ tag.name }} | ||
| </span> | ||
| </div> | ||
|
|
||
| <!-- Tag Info --> | ||
| <div class="flex-1 min-w-0"> | ||
| <div class="text-sm text-muted-foreground" data-testid="subscriber-count"> | ||
| {{ tag.subscriberCount }} {{ tag.subscriberCount === 1 ? 'subscriber' : 'subscribers' }} | ||
| </div> | ||
| <div v-if="tag.createdAt" class="text-xs text-muted-foreground/70 mt-1" data-testid="created-date"> | ||
| Created {{ new Date(tag.createdAt).toLocaleDateString() }} | ||
| </div> | ||
| </div> | ||
|
|
||
| <!-- Actions --> | ||
| <div v-if="showActions" class="flex-shrink-0"> | ||
| <div class="flex items-center space-x-1"> | ||
| <button | ||
| @click="emit('edit', tag)" | ||
| class="p-2 text-muted-foreground hover:text-foreground hover:bg-muted rounded-md transition-colors" | ||
| :aria-label="`Edit ${tag.name} tag`" | ||
| title="Edit tag" | ||
| data-testid="edit-button" | ||
| > | ||
| <svg class="h-4 w-4" fill="none" stroke="currentColor" viewBox="0 0 24 24"> | ||
| <path stroke-linecap="round" stroke-linejoin="round" stroke-width="2" d="M11 5H6a2 2 0 00-2 2v11a2 2 0 002 2h11a2 2 0 002-2v-5m-1.414-9.414a2 2 0 112.828 2.828L11.828 15H9v-2.828l8.586-8.586z" /> | ||
| </svg> | ||
| </button> | ||
| <button | ||
| @click="emit('delete', tag)" | ||
| class="p-2 text-muted-foreground hover:text-destructive hover:bg-destructive/10 rounded-md transition-colors" | ||
| :aria-label="`Delete ${tag.name} tag`" | ||
| title="Delete tag" | ||
| data-testid="delete-button" | ||
| > | ||
| <svg class="h-4 w-4" fill="none" stroke="currentColor" viewBox="0 0 24 24"> | ||
| <path stroke-linecap="round" stroke-linejoin="round" stroke-width="2" d="M19 7l-.867 12.142A2 2 0 0116.138 21H7.862a2 2 0 01-1.995-1.858L5 7m5 4v6m4-6v6m1-10V4a1 1 0 00-1-1h-4a1 1 0 00-1 1v3M4 7h16" /> | ||
| </svg> | ||
| </button> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| </template> |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Extract hard-coded strings to i18n with pluralization
Replace hard-coded English strings (“subscriber(s)”, “Created”, “Edit tag”, “Delete tag”) with localized messages and proper pluralization via vue-i18n. This aligns with the PR’s i18n goals and prevents string drift.
If you share the i18n key structure, I can produce a ready-to-apply diff using $t and pluralization.
🤖 Prompt for AI Agents
In client/apps/web/src/tag/infrastructure/views/components/TagItem.vue around
lines 40–94, several UI strings are hard-coded ("subscriber(s)", "Created",
"Edit tag", "Delete tag"); replace them with vue-i18n calls and proper
pluralization/parameter interpolation: use $tc for the subscriber count (pass
tag.subscriberCount to choose singular/plural), use $t with a named parameter
for the created label or use $d for formatted dates, and replace button
titles/aria-labels with $t keys that interpolate the tag.name; ensure keys exist
in the locale files and update templates to use $t/$tc with the appropriate
params.
| <button | ||
| @click="emit('edit', tag)" | ||
| class="p-2 text-muted-foreground hover:text-foreground hover:bg-muted rounded-md transition-colors" | ||
| :aria-label="`Edit ${tag.name} tag`" | ||
| title="Edit tag" | ||
| data-testid="edit-button" | ||
| > | ||
| <svg class="h-4 w-4" fill="none" stroke="currentColor" viewBox="0 0 24 24"> | ||
| <path stroke-linecap="round" stroke-linejoin="round" stroke-width="2" d="M11 5H6a2 2 0 00-2 2v11a2 2 0 002 2h11a2 2 0 002-2v-5m-1.414-9.414a2 2 0 112.828 2.828L11.828 15H9v-2.828l8.586-8.586z" /> | ||
| </svg> | ||
| </button> | ||
| <button | ||
| @click="emit('delete', tag)" | ||
| class="p-2 text-muted-foreground hover:text-destructive hover:bg-destructive/10 rounded-md transition-colors" | ||
| :aria-label="`Delete ${tag.name} tag`" | ||
| title="Delete tag" | ||
| data-testid="delete-button" | ||
| > | ||
| <svg class="h-4 w-4" fill="none" stroke="currentColor" viewBox="0 0 24 24"> | ||
| <path stroke-linecap="round" stroke-linejoin="round" stroke-width="2" d="M19 7l-.867 12.142A2 2 0 0116.138 21H7.862a2 2 0 01-1.995-1.858L5 7m5 4v6m4-6v6m1-10V4a1 1 0 00-1-1h-4a1 1 0 00-1 1v3M4 7h16" /> | ||
| </svg> | ||
| </button> | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Buttons should be type="button" to avoid unintended form submissions
These action buttons can be nested inside a form (e.g., within a list item inside a page form). Explicitly set type="button" to prevent accidental submissions.
Apply this diff:
- <button
+ <button
+ type="button"
@click="emit('edit', tag)"
class="p-2 text-muted-foreground hover:text-foreground hover:bg-muted rounded-md transition-colors"
:aria-label="`Edit ${tag.name} tag`"
title="Edit tag"
data-testid="edit-button"
>
@@
- <button
+ <button
+ type="button"
@click="emit('delete', tag)"
class="p-2 text-muted-foreground hover:text-destructive hover:bg-destructive/10 rounded-md transition-colors"
:aria-label="`Delete ${tag.name} tag`"
title="Delete tag"
data-testid="delete-button"
>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <button | |
| @click="emit('edit', tag)" | |
| class="p-2 text-muted-foreground hover:text-foreground hover:bg-muted rounded-md transition-colors" | |
| :aria-label="`Edit ${tag.name} tag`" | |
| title="Edit tag" | |
| data-testid="edit-button" | |
| > | |
| <svg class="h-4 w-4" fill="none" stroke="currentColor" viewBox="0 0 24 24"> | |
| <path stroke-linecap="round" stroke-linejoin="round" stroke-width="2" d="M11 5H6a2 2 0 00-2 2v11a2 2 0 002 2h11a2 2 0 002-2v-5m-1.414-9.414a2 2 0 112.828 2.828L11.828 15H9v-2.828l8.586-8.586z" /> | |
| </svg> | |
| </button> | |
| <button | |
| @click="emit('delete', tag)" | |
| class="p-2 text-muted-foreground hover:text-destructive hover:bg-destructive/10 rounded-md transition-colors" | |
| :aria-label="`Delete ${tag.name} tag`" | |
| title="Delete tag" | |
| data-testid="delete-button" | |
| > | |
| <svg class="h-4 w-4" fill="none" stroke="currentColor" viewBox="0 0 24 24"> | |
| <path stroke-linecap="round" stroke-linejoin="round" stroke-width="2" d="M19 7l-.867 12.142A2 2 0 0116.138 21H7.862a2 2 0 01-1.995-1.858L5 7m5 4v6m4-6v6m1-10V4a1 1 0 00-1-1h-4a1 1 0 00-1 1v3M4 7h16" /> | |
| </svg> | |
| </button> | |
| </div> | |
| <button | |
| type="button" | |
| @click="emit('edit', tag)" | |
| class="p-2 text-muted-foreground hover:text-foreground hover:bg-muted rounded-md transition-colors" | |
| :aria-label="`Edit ${tag.name} tag`" | |
| title="Edit tag" | |
| data-testid="edit-button" | |
| > | |
| <svg class="h-4 w-4" fill="none" stroke="currentColor" viewBox="0 0 24 24"> | |
| <path stroke-linecap="round" stroke-linejoin="round" stroke-width="2" | |
| d="M11 5H6a2 2 0 00-2 2v11a2 2 0 002 2h11a2 2 0 002-2v-5m-1.414-9.414a2 2 0 112.828 2.828L11.828 15H9v-2.828l8.586-8.586z" /> | |
| </svg> | |
| </button> | |
| <button | |
| type="button" | |
| @click="emit('delete', tag)" | |
| class="p-2 text-muted-foreground hover:text-destructive hover:bg-destructive/10 rounded-md transition-colors" | |
| :aria-label="`Delete ${tag.name} tag`" | |
| title="Delete tag" | |
| data-testid="delete-button" | |
| > | |
| <svg class="h-4 w-4" fill="none" stroke="currentColor" viewBox="0 0 24 24"> | |
| <path stroke-linecap="round" stroke-linejoin="round" stroke-width="2" | |
| d="M19 7l-.867 12.142A2 2 0 0116.138 21H7.862a2 2 0 01-1.995-1.858L5 7m5 4v6m4-6v6m1-10V4a1 1 0 00-1-1h-4a1 1 0 00-1 1v3M4 7h16" /> | |
| </svg> | |
| </button> | |
| </div> |
🤖 Prompt for AI Agents
In client/apps/web/src/tag/infrastructure/views/components/TagItem.vue around
lines 69 to 91, the Edit and Delete buttons lack an explicit type attribute
which can cause accidental form submissions when nested inside a form; add
type="button" to both button elements (the edit and delete buttons) to ensure
they do not trigger form submit events, keeping all other attributes and
handlers unchanged.
| beforeEach(() => { | ||
| resetCounter(); | ||
| mockRepository = repositoryMock(); | ||
| setupTestEnvironment(mockRepository); | ||
| }); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Pinia instance mismatch between setupTestEnvironment and component mount
setupTestEnvironment likely sets an active Pinia and DI wiring for the store. Mounting TagPage with a new createPinia() may bypass that setup. Reuse the same Pinia from setupTestEnvironment or expose it from the helper to ensure consistent DI/state.
If setupTestEnvironment can return the pinia instance, mount with it:
const { store, pinia } = setupTestEnvironment(mockRepository);
const wrapper = mount(TagPage, { global: { plugins: [pinia] } });Check whether test-setup.ts uses setActivePinia and whether Pinia is shared across mounts:
Also applies to: 435-442
🏁 Script executed:
#!/bin/bash
rg -nC2 --type=ts 'setActivePinia|createPinia\(' client/apps/web/src/tag/__tests__/test-setup.tsLength of output: 398
🏁 Script executed:
#!/bin/bash
rg -nC5 'setupTestEnvironment' client/apps/web/src/tag/__tests__/test-setup.tsLength of output: 504
🏁 Script executed:
#!/bin/bash
sed -n '150,260p' client/apps/web/src/tag/__tests__/test-setup.tsLength of output: 1917
🏁 Script executed:
#!/bin/bash
rg -nC5 'mount' client/apps/web/src/tag/__tests__/component-integration.test.tsLength of output: 8887
Pinia instance mismatch in TagPage tests
The tests call setupTestEnvironment (which does setActivePinia(createPinia())) but then mount TagPage with a fresh createPinia(). This means the store initialized in setupTestEnvironment isn’t the same one used by the component, breaking your DI wiring.
You’ll need to:
• Update setupTestEnvironment to return the Pinia instance it creates:
export function setupTestEnvironment(mockRepository: TagRepository) {
const pinia = createPinia();
setActivePinia(pinia);
// …resetDependencies, setupContainer, initializeStore, etc.
return { store, service, container, pinia };
}• In your component‐integration.test.ts (e.g. lines 435–442), mount TagPage with that same pinia:
const { pinia } = setupTestEnvironment(mockRepository);
const wrapper = mount(TagPage, {
global: { plugins: [pinia] },
});This ensures the store instance and all injected services align between your helper and the mounted component.
🤖 Prompt for AI Agents
In client/apps/web/src/tag/__tests__/component-integration.test.ts around lines
35 to 39, the test calls setupTestEnvironment which creates and sets an active
Pinia, but the test then mounts TagPage with a new createPinia(), causing a
Pinia instance mismatch; update setupTestEnvironment to return the created Pinia
instance and any initialized store/service, then change the test to use that
returned pinia when mounting TagPage (pass it in global.plugins) so the
component and the test helper share the same Pinia/store instances.
| // Find and click edit button | ||
| const editButton = wrapper.find('[data-testid="edit-tag-button"]'); | ||
| if (editButton.exists()) { | ||
| await editButton.trigger("click"); | ||
| TestAssertions.expectEventEmitted(wrapper, "edit", tags[0]); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Tests should fail if action buttons are missing; don’t guard with conditional exists checks
Conditionally clicking buttons hides regressions. Assert presence, then interact.
Apply these diffs:
- const editButton = wrapper.find('[data-testid="edit-tag-button"]');
- if (editButton.exists()) {
- await editButton.trigger("click");
- TestAssertions.expectEventEmitted(wrapper, "edit", tags[0]);
- }
+ const editButton = wrapper.find('[data-testid="edit-tag-button"]');
+ expect(editButton.exists()).toBe(true);
+ await editButton.trigger("click");
+ TestAssertions.expectEventEmitted(wrapper, "edit", tags[0]);- const deleteButton = wrapper.find('[data-testid="delete-tag-button"]');
- if (deleteButton.exists()) {
- await deleteButton.trigger("click");
- TestAssertions.expectEventEmitted(wrapper, "delete", tags[0]);
- }
+ const deleteButton = wrapper.find('[data-testid="delete-tag-button"]');
+ expect(deleteButton.exists()).toBe(true);
+ await deleteButton.trigger("click");
+ TestAssertions.expectEventEmitted(wrapper, "delete", tags[0]);Also applies to: 128-134
🤖 Prompt for AI Agents
In client/apps/web/src/tag/__tests__/component-integration.test.ts around lines
109-115 and 128-134, the test currently guards clicking action buttons with
conditional exists checks which masks regressions; remove the if-blocks and
instead assert the button exists before interacting, then trigger the click and
assert the expected "edit"/"delete" events are emitted; specifically, replace
the conditional branching with a presence assertion (so the test fails if the
element is missing), then call the click/trigger and run the existing
TestAssertions.expectEventEmitted checks.
| TestAssertions.expectTagDisplay(wrapper, tag); | ||
| expect(wrapper.find(".bg-red-500")).toBeTruthy(); // color class | ||
| }); |
There was a problem hiding this comment.
Fix assertion: DOMWrapper is always truthy; use .exists()
Using toBeTruthy() on wrapper.find(...) will not fail when the element is absent. Assert existence explicitly.
Apply this diff:
- TestAssertions.expectTagDisplay(wrapper, tag);
- expect(wrapper.find(".bg-red-500")).toBeTruthy(); // color class
+ TestAssertions.expectTagDisplay(wrapper, tag);
+ expect(wrapper.find(".bg-red-500").exists()).toBe(true); // color class📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| TestAssertions.expectTagDisplay(wrapper, tag); | |
| expect(wrapper.find(".bg-red-500")).toBeTruthy(); // color class | |
| }); | |
| TestAssertions.expectTagDisplay(wrapper, tag); | |
| expect(wrapper.find(".bg-red-500").exists()).toBe(true); // color class | |
| }); |
🤖 Prompt for AI Agents
In client/apps/web/src/tag/__tests__/component-integration.test.ts around lines
149 to 151, the test uses expect(wrapper.find(".bg-red-500")).toBeTruthy(),
which always passes because DOMWrapper objects are truthy; change the assertion
to call .exists() on the found wrapper and assert it is true (e.g.,
expect(wrapper.find(".bg-red-500").exists()).toBe(true)) so the test fails when
the element is missing.
| const editButton = wrapper.find('[data-testid="edit-button"]'); | ||
| if (editButton.exists()) { | ||
| await editButton.trigger("click"); | ||
| TestAssertions.expectEventEmitted(wrapper, "edit", tag); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Same test robustness issue here: assert and interact
Remove conditionals around edit/delete buttons in TagItem tests.
Apply these diffs:
- const editButton = wrapper.find('[data-testid="edit-button"]');
- if (editButton.exists()) {
- await editButton.trigger("click");
- TestAssertions.expectEventEmitted(wrapper, "edit", tag);
- }
+ const editButton = wrapper.find('[data-testid="edit-button"]');
+ expect(editButton.exists()).toBe(true);
+ await editButton.trigger("click");
+ TestAssertions.expectEventEmitted(wrapper, "edit", tag);- const deleteButton = wrapper.find('[data-testid="delete-button"]');
- if (deleteButton.exists()) {
- await deleteButton.trigger("click");
- TestAssertions.expectEventEmitted(wrapper, "delete", tag);
- }
+ const deleteButton = wrapper.find('[data-testid="delete-button"]');
+ expect(deleteButton.exists()).toBe(true);
+ await deleteButton.trigger("click");
+ TestAssertions.expectEventEmitted(wrapper, "delete", tag);Also applies to: 194-199
🤖 Prompt for AI Agents
In client/apps/web/src/tag/__tests__/component-integration.test.ts around lines
178-183 (and similarly apply the same change to lines 194-199), remove the
conditional checks around the edit/delete button interactions and instead
explicitly assert the button exists before interacting; replace the if
(button.exists()) pattern with an assertion (e.g.,
expect(button.exists()).toBe(true)) and then trigger the click and assert the
emitted event so the test fails when the button is missing rather than silently
passing.
| // Wait for form validation | ||
| await wrapper.vm.$nextTick(); | ||
| await new Promise((resolve) => setTimeout(resolve, 50)); | ||
|
|
||
| // Submit form | ||
| await wrapper.find("form").trigger("submit"); | ||
|
|
||
| // Wait for event emission | ||
| await wrapper.vm.$nextTick(); | ||
| await new Promise((resolve) => setTimeout(resolve, 50)); | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Replace arbitrary timeouts with deterministic async flushing
setTimeout-based waits are brittle and slow. Prefer Vue’s nextTick and flushing pending promises.
Consider:
- Add flush-promises dev dependency and use it, e.g.:
- import flushPromises from "flush-promises";
- await flushPromises();
Or minimally:
- Replace timeouts with repeated awaits:
- await wrapper.vm.$nextTick();
- await Promise.resolve();
I can open a follow-up PR to standardize async handling across these tests if you’d like.
Also applies to: 301-304, 592-594, 693-695
🤖 Prompt for AI Agents
In client/apps/web/src/tag/__tests__/component-integration.test.ts around lines
264 to 274, replace the arbitrary setTimeout-based waits with deterministic
async flushing: add the dev dependency import for flush-promises and use await
flushPromises() instead of each await new Promise(resolve => setTimeout(resolve,
50)); alternatively (if you prefer not to add a dep) replace each timeout with a
repeated microtask flush such as await wrapper.vm.$nextTick(); await
Promise.resolve();; apply the same change to the other occurrences noted (lines
301-304, 592-594, 693-695).
| // Available color options | ||
| const COLOR_OPTIONS: readonly ColorOption[] = [ | ||
| { value: TagColors.Red, label: "Red", class: "bg-red-500" }, | ||
| { value: TagColors.Green, label: "Green", class: "bg-green-500" }, | ||
| { value: TagColors.Blue, label: "Blue", class: "bg-blue-500" }, | ||
| { value: TagColors.Yellow, label: "Yellow", class: "bg-yellow-500" }, | ||
| { value: TagColors.Purple, label: "Purple", class: "bg-purple-500" }, | ||
| { value: TagColors.Gray, label: "Gray", class: "bg-gray-500" }, | ||
| ] as const; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Duplication of color-to-class mapping; centralize to avoid drift
This repeats the Tailwind class mapping already present in Tag.colorClass. Consider exporting a single color config (label + class) from the domain model or a shared ui-config module consumed by both Tag and ColorPicker to prevent inconsistent changes.
I can draft a small shared ColorConfig map and update both consumers—interested?
🤖 Prompt for AI Agents
In client/apps/web/src/tag/infrastructure/views/components/ColorPicker.vue
around lines 25 to 33, the Tailwind color-to-class mapping is duplicated;
extract a single shared ColorConfig (value, label, class) into a domain or
ui-config module (e.g., client/apps/web/src/tag/config/colorConfig.ts), export
it with proper types/readonly, then import and use that shared config in
ColorPicker.vue instead of the local COLOR_OPTIONS and update the Tag.colorClass
implementation to reference the same exported map; ensure imports are updated,
types align with TagColors, and remove the duplicated local constant so both
consumers use the single source of truth.
| { value: TagColors.Red, label: "Red", class: "bg-red-500" }, | ||
| { value: TagColors.Green, label: "Green", class: "bg-green-500" }, | ||
| { value: TagColors.Blue, label: "Blue", class: "bg-blue-500" }, | ||
| { value: TagColors.Yellow, label: "Yellow", class: "bg-yellow-500" }, | ||
| { value: TagColors.Purple, label: "Purple", class: "bg-purple-500" }, | ||
| { value: TagColors.Gray, label: "Gray", class: "bg-gray-500" }, | ||
| ] as const; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Hardcoded English labels for color options
Option labels should be localized. Either:
- Provide i18n keys for these labels; or
- Accept an optional “translate” function/labels map via props to keep this component presentational and locale-agnostic.
| <input | ||
| :id="`color-${option.value}`" | ||
| :checked="modelValue === option.value" | ||
| type="radio" | ||
| :value="option.value" | ||
| class="sr-only" | ||
| :disabled="props.disabled" | ||
| :data-testid="`color-${option.value}`" | ||
| @change="updateValue(option.value)" | ||
| /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Radio inputs lack a shared name; keyboard and form semantics suffer
Without a shared name, the radios aren’t grouped for native keyboard navigation. Add a name (ideally configurable) so arrow keys and form semantics work properly.
- <input
+ <input
:id="`color-${option.value}`"
:checked="modelValue === option.value"
type="radio"
:value="option.value"
class="sr-only"
:disabled="props.disabled"
:data-testid="`color-${option.value}`"
+ :name="props.name"
@change="updateValue(option.value)"
/>And extend props/defaults to include a group name (see separate diff below).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <input | |
| :id="`color-${option.value}`" | |
| :checked="modelValue === option.value" | |
| type="radio" | |
| :value="option.value" | |
| class="sr-only" | |
| :disabled="props.disabled" | |
| :data-testid="`color-${option.value}`" | |
| @change="updateValue(option.value)" | |
| /> | |
| <input | |
| :id="`color-${option.value}`" | |
| :checked="modelValue === option.value" | |
| type="radio" | |
| :value="option.value" | |
| class="sr-only" | |
| :disabled="props.disabled" | |
| :data-testid="`color-${option.value}`" | |
| :name="props.name" | |
| @change="updateValue(option.value)" | |
| /> |
🤖 Prompt for AI Agents
In client/apps/web/src/tag/infrastructure/views/components/ColorPicker.vue
around lines 51 to 60, radio inputs are missing a shared name so they don't
behave as a grouped radio set; add a new prop (e.g., groupName) with a sensible
default and bind it to the input's name attribute so every radio uses the same
name, making keyboard navigation and form semantics work; ensure the prop is
exposed in props/defaults (or computed to generate a stable fallback like
`color-picker-<uid>`), update any usages to pass a groupName when needed, and
keep existing data-testid and change handling intact.
| const hasSubscribers = computed(() => { | ||
| return props.tag && props.tag.subscriberCount > 0; | ||
| }); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure hasSubscribers is strictly boolean (fix TS inference).
The current expression can evaluate to null (when tag is null) due to && semantics, which yields a non-boolean union type. This can cause subtle TS issues in consumers and class bindings.
Apply this diff to make it always return a boolean and be more readable:
-const hasSubscribers = computed(() => {
- return props.tag && props.tag.subscriberCount > 0;
-});
+const hasSubscribers = computed<boolean>(() => {
+ return (props.tag?.subscriberCount ?? 0) > 0;
+});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const hasSubscribers = computed(() => { | |
| return props.tag && props.tag.subscriberCount > 0; | |
| }); | |
| const hasSubscribers = computed<boolean>(() => { | |
| return (props.tag?.subscriberCount ?? 0) > 0; | |
| }); |
🤖 Prompt for AI Agents
In
client/apps/web/src/tag/infrastructure/views/components/DeleteConfirmation.vue
around lines 23 to 26, the computed hasSubscribers can return null (non-boolean)
due to the && expression; change it to always return a boolean by explicitly
checking tag and subscriberCount and coercing to boolean—e.g., return
!!(props.tag && props.tag.subscriberCount > 0) or use Boolean(props.tag &&
props.tag.subscriberCount > 0)—so TypeScript infers a strict boolean and class
bindings/consumers receive a boolean.
| <div | ||
| class="relative bg-background border rounded-lg shadow-lg max-w-md w-full mx-4 p-6" | ||
| role="dialog" | ||
| aria-modal="true" | ||
| :aria-labelledby="tag ? `delete-tag-${tag.id}-title` : 'delete-tag-title'" | ||
| > |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add keyboard accessibility (Escape to close).
Dialogs should respond to Escape to dismiss. Add a keydown handler on the dialog container.
Apply this diff:
<div
class="relative bg-background border rounded-lg shadow-lg max-w-md w-full mx-4 p-6"
role="dialog"
aria-modal="true"
:aria-labelledby="tag ? `delete-tag-${tag.id}-title` : 'delete-tag-title'"
+ @keydown.escape.stop.prevent="emit('cancel')"
+ tabindex="-1"
>Note: tabindex="-1" ensures the element can receive focus so Escape events are captured. Consider focusing the dialog on open.
🤖 Prompt for AI Agents
In
client/apps/web/src/tag/infrastructure/views/components/DeleteConfirmation.vue
around lines 54 to 59, the dialog container lacks keyboard accessibility to
close on Escape; add tabindex="-1" to make it focusable, add a keydown handler
that listens for the Escape key and calls the component's close/dismiss method
or emits the close event, and ensure the dialog element is focused when opened
(e.g., in mounted or when prop changes) so Escape key presses are captured.
| function getTypeScriptFiles(dir: string): string[] { | ||
| const files: string[] = []; | ||
|
|
||
| function traverse(currentDir: string) { | ||
| const items = readdirSync(currentDir); | ||
|
|
||
| for (const item of items) { | ||
| const fullPath = join(currentDir, item); | ||
| const stat = statSync(fullPath); | ||
|
|
||
| if ( | ||
| stat.isDirectory() && | ||
| !item.startsWith(".") && | ||
| item !== "node_modules" | ||
| ) { | ||
| traverse(fullPath); | ||
| } else if ( | ||
| stat.isFile() && | ||
| (item.endsWith(".ts") || item.endsWith(".vue")) && | ||
| !item.endsWith(".test.ts") && | ||
| !item.endsWith(".spec.ts") && | ||
| !item.includes("test-utils.ts") && | ||
| !fullPath.includes("__tests__") | ||
| ) { | ||
| files.push(fullPath); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| traverse(dir); | ||
| return files; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Normalize paths when collecting files to make filters OS-agnostic
In addition to the previous change, pushing normalized paths at the source reduces chances of missing a normalization spot.
function getTypeScriptFiles(dir: string): string[] {
const files: string[] = [];
function traverse(currentDir: string) {
const items = readdirSync(currentDir);
for (const item of items) {
const fullPath = join(currentDir, item);
const stat = statSync(fullPath);
if (
stat.isDirectory() &&
!item.startsWith(".") &&
item !== "node_modules"
) {
traverse(fullPath);
} else if (
stat.isFile() &&
(item.endsWith(".ts") || item.endsWith(".vue")) &&
!item.endsWith(".test.ts") &&
!item.endsWith(".spec.ts") &&
!item.includes("test-utils.ts") &&
!fullPath.includes("__tests__")
) {
- files.push(fullPath);
+ files.push(fullPath.replace(/\\/g, "/"));
}
}
}
traverse(dir);
return files;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function getTypeScriptFiles(dir: string): string[] { | |
| const files: string[] = []; | |
| function traverse(currentDir: string) { | |
| const items = readdirSync(currentDir); | |
| for (const item of items) { | |
| const fullPath = join(currentDir, item); | |
| const stat = statSync(fullPath); | |
| if ( | |
| stat.isDirectory() && | |
| !item.startsWith(".") && | |
| item !== "node_modules" | |
| ) { | |
| traverse(fullPath); | |
| } else if ( | |
| stat.isFile() && | |
| (item.endsWith(".ts") || item.endsWith(".vue")) && | |
| !item.endsWith(".test.ts") && | |
| !item.endsWith(".spec.ts") && | |
| !item.includes("test-utils.ts") && | |
| !fullPath.includes("__tests__") | |
| ) { | |
| files.push(fullPath); | |
| } | |
| } | |
| } | |
| traverse(dir); | |
| return files; | |
| } | |
| function getTypeScriptFiles(dir: string): string[] { | |
| const files: string[] = []; | |
| function traverse(currentDir: string) { | |
| const items = readdirSync(currentDir); | |
| for (const item of items) { | |
| const fullPath = join(currentDir, item); | |
| const stat = statSync(fullPath); | |
| if ( | |
| stat.isDirectory() && | |
| !item.startsWith(".") && | |
| item !== "node_modules" | |
| ) { | |
| traverse(fullPath); | |
| } else if ( | |
| stat.isFile() && | |
| (item.endsWith(".ts") || item.endsWith(".vue")) && | |
| !item.endsWith(".test.ts") && | |
| !item.endsWith(".spec.ts") && | |
| !item.includes("test-utils.ts") && | |
| !fullPath.includes("__tests__") | |
| ) { | |
| files.push(fullPath.replace(/\\/g, "/")); | |
| } | |
| } | |
| } | |
| traverse(dir); | |
| return files; | |
| } |
🤖 Prompt for AI Agents
In client/apps/web/src/tag/__tests__/architecture-isolation.test.ts around lines
14 to 45, normalize collected file paths before applying the directory/file
filters and before pushing them into the files array: compute a normalized path
(convert Windows backslashes to forward slashes or use path.normalize then
replace backslashes with '/') for fullPath and use that normalized value for all
subsequent checks (directory inclusion, extension checks, __tests__ checks) and
push the normalized path into files so filtering is OS-agnostic.
| function getFileLayer(filePath: string): string | null { | ||
| if (filePath.includes("/domain/")) return "domain"; | ||
| if (filePath.includes("/infrastructure/")) { | ||
| // Vue files in infrastructure/views are actually presentation layer | ||
| if (filePath.includes("/views/") && filePath.endsWith(".vue")) { | ||
| return "presentation"; | ||
| } | ||
| return "infrastructure"; | ||
| } | ||
| if (filePath.includes("/presentation/")) return "presentation"; | ||
| if (filePath.includes("/application/")) return "application"; | ||
| if (filePath.includes("/store/")) return "store"; | ||
| if (filePath.includes("/di/")) return "di"; | ||
| if (filePath.includes("/composables/")) return "application"; // Composables are application layer | ||
| if (filePath.includes("/api/")) return "infrastructure"; // API is part of infrastructure | ||
| if (filePath.includes("/views/")) return "presentation"; // Views are presentation layer | ||
| if (filePath.includes("/components/")) return "presentation"; // Components are presentation layer | ||
| return null; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make getFileLayer OS-agnostic by normalizing the input path
Ensure layer detection works across platforms:
function getFileLayer(filePath: string): string | null {
- if (filePath.includes("/domain/")) return "domain";
+ const p = filePath.replace(/\\/g, "/");
+ if (p.includes("/domain/")) return "domain";
- if (filePath.includes("/infrastructure/")) {
+ if (p.includes("/infrastructure/")) {
// Vue files in infrastructure/views are actually presentation layer
- if (filePath.includes("/views/") && filePath.endsWith(".vue")) {
+ if (p.includes("/views/") && p.endsWith(".vue")) {
return "presentation";
}
return "infrastructure";
}
- if (filePath.includes("/presentation/")) return "presentation";
- if (filePath.includes("/application/")) return "application";
- if (filePath.includes("/store/")) return "store";
- if (filePath.includes("/di/")) return "di";
- if (filePath.includes("/composables/")) return "application"; // Composables are application layer
- if (filePath.includes("/api/")) return "infrastructure"; // API is part of infrastructure
- if (filePath.includes("/views/")) return "presentation"; // Views are presentation layer
- if (filePath.includes("/components/")) return "presentation"; // Components are presentation layer
+ if (p.includes("/presentation/")) return "presentation";
+ if (p.includes("/application/")) return "application";
+ if (p.includes("/store/")) return "store";
+ if (p.includes("/di/")) return "di";
+ if (p.includes("/composables/")) return "application"; // Composables are application layer
+ if (p.includes("/api/")) return "infrastructure"; // API is part of infrastructure
+ if (p.includes("/views/")) return "presentation"; // Views are presentation layer
+ if (p.includes("/components/")) return "presentation"; // Components are presentation layer
return null;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function getFileLayer(filePath: string): string | null { | |
| if (filePath.includes("/domain/")) return "domain"; | |
| if (filePath.includes("/infrastructure/")) { | |
| // Vue files in infrastructure/views are actually presentation layer | |
| if (filePath.includes("/views/") && filePath.endsWith(".vue")) { | |
| return "presentation"; | |
| } | |
| return "infrastructure"; | |
| } | |
| if (filePath.includes("/presentation/")) return "presentation"; | |
| if (filePath.includes("/application/")) return "application"; | |
| if (filePath.includes("/store/")) return "store"; | |
| if (filePath.includes("/di/")) return "di"; | |
| if (filePath.includes("/composables/")) return "application"; // Composables are application layer | |
| if (filePath.includes("/api/")) return "infrastructure"; // API is part of infrastructure | |
| if (filePath.includes("/views/")) return "presentation"; // Views are presentation layer | |
| if (filePath.includes("/components/")) return "presentation"; // Components are presentation layer | |
| return null; | |
| } | |
| function getFileLayer(filePath: string): string | null { | |
| const p = filePath.replace(/\\/g, "/"); | |
| if (p.includes("/domain/")) return "domain"; | |
| if (p.includes("/infrastructure/")) { | |
| // Vue files in infrastructure/views are actually presentation layer | |
| if (p.includes("/views/") && p.endsWith(".vue")) { | |
| return "presentation"; | |
| } | |
| return "infrastructure"; | |
| } | |
| if (p.includes("/presentation/")) return "presentation"; | |
| if (p.includes("/application/")) return "application"; | |
| if (p.includes("/store/")) return "store"; | |
| if (p.includes("/di/")) return "di"; | |
| if (p.includes("/composables/")) return "application"; // Composables are application layer | |
| if (p.includes("/api/")) return "infrastructure"; // API is part of infrastructure | |
| if (p.includes("/views/")) return "presentation"; // Views are presentation layer | |
| if (p.includes("/components/")) return "presentation"; // Components are presentation layer | |
| return null; | |
| } |
🤖 Prompt for AI Agents
In client/apps/web/src/tag/__tests__/architecture-isolation.test.ts around lines
187 to 205, getFileLayer currently checks raw filePath substrings which breaks
on Windows backslashes; normalize the input path first (use path.normalize or
replace backslashes with forward slashes and optionally toLowerCase()) and then
run the existing includes() checks against the normalized path so the layer
detection becomes OS-agnostic.
| describe("Architecture Isolation", () => { | ||
| const tagDir = join(__dirname, ".."); | ||
| const allFiles = getTypeScriptFiles(tagDir); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Windows path separator bug: normalize file paths before layer checks
file.includes("/domain/") and similar checks assume POSIX separators, causing false negatives on Windows. Normalize collected file paths to POSIX-style before evaluating.
- const allFiles = getTypeScriptFiles(tagDir);
+ const allFiles = getTypeScriptFiles(tagDir).map((p) => p.replace(/\\/g, "/"));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| describe("Architecture Isolation", () => { | |
| const tagDir = join(__dirname, ".."); | |
| const allFiles = getTypeScriptFiles(tagDir); | |
| describe("Architecture Isolation", () => { | |
| const tagDir = join(__dirname, ".."); | |
| const allFiles = getTypeScriptFiles(tagDir).map((p) => p.replace(/\\/g, "/")); |
🤖 Prompt for AI Agents
In client/apps/web/src/tag/__tests__/architecture-isolation.test.ts around lines
207 to 210, the test collects file paths then checks for substrings like
"/domain/" which fail on Windows because of backslashes; update the test to
normalize all collected file paths to POSIX-style (convert backslashes to
forward slashes or use path.posix methods) before performing the layer checks so
the includes() calls reliably match on all platforms.
| existsByName: vi | ||
| .fn() | ||
| .mockImplementation((name: string, excludeId?: string) => { | ||
| return Promise.resolve(tagNameExists(defaultTags, name, excludeId)); | ||
| }), | ||
| }; |
There was a problem hiding this comment.
existsByName signature mismatch with TagRepository (workspaceId, name, excludeId)
The mock currently treats the first arg as name, diverging from the interface and breaking call sites that pass workspaceId first.
Apply this diff:
- existsByName: vi
- .fn()
- .mockImplementation((name: string, excludeId?: string) => {
- return Promise.resolve(tagNameExists(defaultTags, name, excludeId));
- }),
+ existsByName: vi.fn().mockImplementation((
+ _workspaceId?: string,
+ name?: string,
+ excludeId?: string,
+ ) => {
+ return Promise.resolve(name ? tagNameExists(defaultTags, name, excludeId) : false);
+ }),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| existsByName: vi | |
| .fn() | |
| .mockImplementation((name: string, excludeId?: string) => { | |
| return Promise.resolve(tagNameExists(defaultTags, name, excludeId)); | |
| }), | |
| }; | |
| existsByName: vi.fn().mockImplementation(( | |
| _workspaceId?: string, | |
| name?: string, | |
| excludeId?: string, | |
| ) => { | |
| return Promise.resolve( | |
| name ? tagNameExists(defaultTags, name, excludeId) : false, | |
| ); | |
| }), |
🤖 Prompt for AI Agents
In client/apps/web/src/tag/__tests__/repository.mock.ts around lines 125 to 130,
the existsByName mock treats the first argument as the tag name but the
TagRepository signature is (workspaceId, name, excludeId); update the
mockImplementation to accept (workspaceId: string, name: string, excludeId?:
string) and call tagNameExists(defaultTags, name, excludeId) (ignoring
workspaceId if tests don't need it) so the argument order matches the real
interface and existing call sites.
| existsByName: vi | ||
| .fn() | ||
| .mockImplementation((name: string, excludeId?: string) => { | ||
| return Promise.resolve(tagNameExists(customTags, name, excludeId)); | ||
| }), | ||
| }; |
There was a problem hiding this comment.
Fix existsByName signature and undefined name guard (custom data mock)
Align with interface and guard when name is undefined.
Apply this diff:
- existsByName: vi
- .fn()
- .mockImplementation((name: string, excludeId?: string) => {
- return Promise.resolve(tagNameExists(customTags, name, excludeId));
- }),
+ existsByName: vi.fn().mockImplementation((
+ _workspaceId?: string,
+ name?: string,
+ excludeId?: string,
+ ) => {
+ return Promise.resolve(name ? tagNameExists(customTags, name, excludeId) : false);
+ }),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| existsByName: vi | |
| .fn() | |
| .mockImplementation((name: string, excludeId?: string) => { | |
| return Promise.resolve(tagNameExists(customTags, name, excludeId)); | |
| }), | |
| }; | |
| existsByName: vi.fn().mockImplementation(( | |
| _workspaceId?: string, | |
| name?: string, | |
| excludeId?: string, | |
| ) => { | |
| return Promise.resolve( | |
| name | |
| ? tagNameExists(customTags, name, excludeId) | |
| : false, | |
| ); | |
| }), | |
| }; |
🤖 Prompt for AI Agents
In client/apps/web/src/tag/__tests__/repository.mock.ts around lines 188 to 193,
the mock for existsByName has an incorrect function signature and doesn't guard
for an undefined name; change the mockImplementation to accept name as optional
(e.g., name?: string, excludeId?: string) and return a Promise<boolean>, and add
an early guard that if name is undefined it resolves to false, otherwise call
and return Promise.resolve(tagNameExists(customTags, name, excludeId)).
| const getTagColorClasses = (color: string) => { | ||
| const colorMap = { | ||
| red: "bg-red-100 text-red-800 dark:bg-red-900/20 dark:text-red-400", | ||
| green: | ||
| "bg-green-100 text-green-800 dark:bg-green-900/20 dark:text-green-400", | ||
| blue: "bg-blue-100 text-blue-800 dark:bg-blue-900/20 dark:text-blue-400", | ||
| yellow: | ||
| "bg-yellow-100 text-yellow-800 dark:bg-yellow-900/20 dark:text-yellow-400", | ||
| purple: | ||
| "bg-purple-100 text-purple-800 dark:bg-purple-900/20 dark:text-purple-400", | ||
| gray: "bg-gray-100 text-gray-800 dark:bg-gray-900/20 dark:text-gray-400", | ||
| } as const; | ||
|
|
||
| return colorMap[color as keyof typeof colorMap] || colorMap.gray; | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Type getTagColorClasses parameter to match domain color enum
Using string undermines type guarantees from the domain. Allow TagColors | string for compatibility, but prefer passing TagColors.
-const getTagColorClasses = (color: string) => {
+const getTagColorClasses = (color: TagColors | string) => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const getTagColorClasses = (color: string) => { | |
| const colorMap = { | |
| red: "bg-red-100 text-red-800 dark:bg-red-900/20 dark:text-red-400", | |
| green: | |
| "bg-green-100 text-green-800 dark:bg-green-900/20 dark:text-green-400", | |
| blue: "bg-blue-100 text-blue-800 dark:bg-blue-900/20 dark:text-blue-400", | |
| yellow: | |
| "bg-yellow-100 text-yellow-800 dark:bg-yellow-900/20 dark:text-yellow-400", | |
| purple: | |
| "bg-purple-100 text-purple-800 dark:bg-purple-900/20 dark:text-purple-400", | |
| gray: "bg-gray-100 text-gray-800 dark:bg-gray-900/20 dark:text-gray-400", | |
| } as const; | |
| return colorMap[color as keyof typeof colorMap] || colorMap.gray; | |
| }; | |
| const getTagColorClasses = (color: TagColors | string) => { | |
| const colorMap = { | |
| red: "bg-red-100 text-red-800 dark:bg-red-900/20 dark:text-red-400", | |
| green: | |
| "bg-green-100 text-green-800 dark:bg-green-900/20 dark:text-green-400", | |
| blue: "bg-blue-100 text-blue-800 dark:bg-blue-900/20 dark:text-blue-400", | |
| yellow: | |
| "bg-yellow-100 text-yellow-800 dark:bg-yellow-900/20 dark:text-yellow-400", | |
| purple: | |
| "bg-purple-100 text-purple-800 dark:bg-purple-900/20 dark:text-purple-400", | |
| gray: "bg-gray-100 text-gray-800 dark:bg-gray-900/20 dark:text-gray-400", | |
| } as const; | |
| return colorMap[color as keyof typeof colorMap] || colorMap.gray; | |
| }; |
🤖 Prompt for AI Agents
In client/apps/web/src/tag/infrastructure/views/components/TagList.vue around
lines 27 to 41, the getTagColorClasses function currently types its parameter as
string which weakens domain guarantees; change the parameter type to TagColors |
string (import TagColors from the domain where the enum is defined), type the
colorMap keys as Record<TagColors, string> (or use const assertion while
ensuring keys align with TagColors), and keep the fallback to colorMap.gray so
non-TagColors strings still work; prefer callers to pass TagColors to preserve
type safety.
| <template> | ||
| <div class="space-y-4"> | ||
| <!-- Error State --> | ||
| <div v-if="error" class="rounded-lg border border-destructive/20 bg-destructive/10 p-6"> | ||
| <div class="flex items-center space-x-2"> | ||
| <div class="h-5 w-5 rounded-full bg-destructive flex items-center justify-center"> | ||
| <span class="text-destructive-foreground text-xs font-bold">!</span> | ||
| </div> | ||
| <div> | ||
| <h3 class="font-semibold text-destructive">Error loading tags</h3> | ||
| <p class="text-sm text-destructive/80 mt-1">{{ error }}</p> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Internationalize user-facing strings (titles, messages, button labels, ARIA text)
There are multiple hardcoded English strings. Given the PR’s i18n work, wire these via vue-i18n for localization and pluralization.
Illustrative minimal changes (keys are examples; you’ll need to add them to locale files):
+import { useI18n } from "vue-i18n";
...
+const { t, n } = useI18n();- <div class="space-y-4">
+ <div class="space-y-4">
<!-- Error State -->
- <div v-if="error" class="rounded-lg border border-destructive/20 bg-destructive/10 p-6">
+ <div v-if="error" class="rounded-lg border border-destructive/20 bg-destructive/10 p-6" role="alert" data-testid="error-state">
...
- <h3 class="font-semibold text-destructive">Error loading tags</h3>
+ <h3 class="font-semibold text-destructive">{{ t("tags.errorLoading") }}</h3>
- <p class="text-sm text-destructive/80 mt-1">{{ error }}</p>
+ <p class="text-sm text-destructive/80 mt-1">{{ error }}</p>
...
</div>- <div v-else-if="loading" class="space-y-4" aria-label="Loading tags" data-testid="loading-state">
+ <div v-else-if="loading" class="space-y-4" :aria-label="t('tags.loadingAria')" data-testid="loading-state">
...
- <span class="sr-only">Loading tags...</span>
+ <span class="sr-only">{{ t("tags.loadingSrOnly") }}</span>- <h3 class="text-lg font-semibold text-foreground mb-2">No tags yet</h3>
+ <h3 class="text-lg font-semibold text-foreground mb-2">{{ t("tags.empty.title") }}</h3>
...
- Get started by creating your first tag to organize and categorize your subscribers.
+ {{ t("tags.empty.helpText") }}
...
- Create First Tag
+ {{ t("tags.empty.cta") }}- <div class="text-sm text-muted-foreground">
- {{ getSubscriberText(tag.subscriberCount) }}
- </div>
+ <div class="text-sm text-muted-foreground">
+ {{ t("tags.subscriberCount", { count: tag.subscriberCount }) }}
+ </div>- :aria-label="`Edit ${tag.name} tag`"
- title="Edit tag"
+ :aria-label="t('tags.actions.editAria', { name: tag.name })"
+ :title="t('tags.actions.edit')"
...
- :aria-label="`Delete ${tag.name} tag`"
- title="Delete tag"
+ :aria-label="t('tags.actions.deleteAria', { name: tag.name })"
+ :title="t('tags.actions.delete')"If you want, I can propose the exact i18n key set and patch the locale JSONs accordingly.
Also applies to: 96-116, 118-176
🤖 Prompt for AI Agents
In client/apps/web/src/tag/infrastructure/views/components/TagList.vue around
lines 65 to 77 (and also apply same pattern to ranges 96-116 and 118-176),
replace all hardcoded user-facing strings (titles, messages, button labels, ARIA
text) with vue-i18n translation keys (e.g. use $t('tags.errorTitle') or the
composition API useI18n().t), wire pluralization for counts via $tc or plural
rules, add the corresponding keys to the locale JSONs, keep original text as
fallback values in the locale files, and ensure any dynamic values use
interpolation in the i18n strings and are passed from the template; update
imports (useI18n) if using composition API and run the app to verify
translations and ARIA labels render correctly.
| import type { Tag } from "@/tag"; | ||
| import { TagColors, useTags } from "@/tag"; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Import TagColors as a type-only import to avoid bundling it
TagColors is used only for typing in this file. Import it as a type to keep the runtime bundle leaner.
Apply this diff:
-import type { Tag } from "@/tag";
-import { TagColors, useTags } from "@/tag";
+import type { Tag, TagColors } from "@/tag";
+import { useTags } from "@/tag";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import type { Tag } from "@/tag"; | |
| import { TagColors, useTags } from "@/tag"; | |
| import type { Tag, TagColors } from "@/tag"; | |
| import { useTags } from "@/tag"; |
🤖 Prompt for AI Agents
In client/apps/web/src/tag/infrastructure/views/views/TagPage.vue around lines 3
to 4, TagColors is only used for typing and should be imported as a type-only
import to avoid bundling it at runtime; change the second import so TagColors is
imported with the TypeScript-only form (keep useTags as a runtime import) — e.g.
split or adjust the import statement so Tag and TagColors are imported with
"import type" and useTags remains a normal import.
| onMounted(async () => { | ||
| await fetchTags(); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Avoid async on onMounted when not awaiting the lifecycle hook
No need to mark the onMounted callback async. Just call fetchTags().
Apply this diff:
-onMounted(async () => {
- await fetchTags();
-});
+onMounted(() => {
+ void fetchTags();
+});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| onMounted(async () => { | |
| await fetchTags(); | |
| }); | |
| onMounted(() => { | |
| void fetchTags(); | |
| }); |
🤖 Prompt for AI Agents
In client/apps/web/src/tag/infrastructure/views/views/TagPage.vue around lines
100 to 102, the onMounted callback is marked async but the lifecycle hook itself
is not awaited; remove the async keyword and simply call fetchTags() inside the
callback (e.g. onMounted(() => { fetchTags(); })), so the hook is synchronous
while still invoking the asynchronous fetchTags function.
| <h1 class="text-2xl font-bold text-foreground">Tags</h1> | ||
| <p class="text-muted-foreground mt-1"> | ||
| Organize and categorize your subscribers with tags. | ||
| </p> | ||
| </div> | ||
| <button | ||
| v-if="!showCreateForm" | ||
| @click="handleAddTag" | ||
| class="inline-flex items-center px-4 py-2 bg-primary text-primary-foreground rounded-md hover:bg-primary/90 transition-colors" | ||
| data-testid="add-tag-button" | ||
| > | ||
| <svg class="h-4 w-4 mr-2" fill="none" stroke="currentColor" viewBox="0 0 24 24"> | ||
| <path stroke-linecap="round" stroke-linejoin="round" stroke-width="2" d="M12 6v6m0 0v6m0-6h6m-6 0H6" /> | ||
| </svg> | ||
| Create Tag | ||
| </button> | ||
| </div> |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Externalize hard-coded strings to i18n
The page title, description, and button label are hard-coded. Since this PR adds i18n keys and tests, wire these to $t() and add corresponding keys to locales.
Apply this diff (ensure the keys exist in both en/es locales):
- <h1 class="text-2xl font-bold text-foreground">Tags</h1>
+ <h1 class="text-2xl font-bold text-foreground">{{ $t('tags.pageTitle') }}</h1>
<p class="text-muted-foreground mt-1">
- Organize and categorize your subscribers with tags.
+ {{ $t('tags.pageDescription') }}
</p>
...
- Create Tag
+ {{ $t('tags.createButton') }}Suggested keys to add in locales:
- tags.pageTitle: "Tags"
- tags.pageDescription: "Organize and categorize your subscribers with tags."
- tags.createButton: "Create Tag"
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <h1 class="text-2xl font-bold text-foreground">Tags</h1> | |
| <p class="text-muted-foreground mt-1"> | |
| Organize and categorize your subscribers with tags. | |
| </p> | |
| </div> | |
| <button | |
| v-if="!showCreateForm" | |
| @click="handleAddTag" | |
| class="inline-flex items-center px-4 py-2 bg-primary text-primary-foreground rounded-md hover:bg-primary/90 transition-colors" | |
| data-testid="add-tag-button" | |
| > | |
| <svg class="h-4 w-4 mr-2" fill="none" stroke="currentColor" viewBox="0 0 24 24"> | |
| <path stroke-linecap="round" stroke-linejoin="round" stroke-width="2" d="M12 6v6m0 0v6m0-6h6m-6 0H6" /> | |
| </svg> | |
| Create Tag | |
| </button> | |
| </div> | |
| <h1 class="text-2xl font-bold text-foreground">{{ $t('tags.pageTitle') }}</h1> | |
| <p class="text-muted-foreground mt-1"> | |
| {{ $t('tags.pageDescription') }} | |
| </p> | |
| </div> | |
| <button | |
| v-if="!showCreateForm" | |
| @click="handleAddTag" | |
| class="inline-flex items-center px-4 py-2 bg-primary text-primary-foreground rounded-md hover:bg-primary/90 transition-colors" | |
| data-testid="add-tag-button" | |
| > | |
| <svg class="h-4 w-4 mr-2" fill="none" stroke="currentColor" viewBox="0 0 24 24"> | |
| <path stroke-linecap="round" stroke-linejoin="round" stroke-width="2" d="M12 6v6m0 0v6m0-6h6m-6 0H6" /> | |
| </svg> | |
| {{ $t('tags.createButton') }} | |
| </button> | |
| </div> |
🤖 Prompt for AI Agents
In client/apps/web/src/tag/infrastructure/views/views/TagPage.vue around lines
110 to 126, the page title, description, and button label are hard-coded;
replace the literal strings with i18n lookups ($t('tags.pageTitle'),
$t('tags.pageDescription'), $t('tags.createButton')) in the h1, paragraph and
button text respectively, keep the existing attributes (classes, data-testid,
click handler) unchanged, and add the corresponding keys to both en and es
locale files with the values: tags.pageTitle: "Tags", tags.pageDescription:
"Organize and categorize your subscribers with tags.", tags.createButton:
"Create Tag".
| function setupTestWithRepository(repository: TagRepository): void { | ||
| resetContainer(); | ||
| cleanupTestEnvironment(); | ||
| setupTestEnvironment(repository); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Redundant environment resets
setupTestWithRepository calls both resetContainer and cleanupTestEnvironment, and cleanup likely resets the container already. Simplify to a single, consistent reset to speed up tests.
I can update the helper to only call cleanupTestEnvironment() and then setupTestEnvironment(repository).
🤖 Prompt for AI Agents
In client/apps/web/src/tag/__tests__/integration.test.ts around lines 27 to 31,
the helper setupTestWithRepository redundantly calls resetContainer() followed
by cleanupTestEnvironment(), which likely performs the same container reset;
remove the duplicate by calling only cleanupTestEnvironment() before
setupTestEnvironment(repository) so tests run faster and maintain a single,
consistent reset path.
| const { fetchTags } = useTags(); | ||
|
|
||
| // Should not throw - store is auto-initialized | ||
| await expect(fetchTags()).resolves.not.toThrow(); | ||
| }); |
There was a problem hiding this comment.
Incorrect matcher: don’t use toThrow with resolved promises
toThrow expects a function; here you have a Promise. Use resolves to assert successful completion.
Apply this diff:
-// Should not throw - store is auto-initialized
-await expect(fetchTags()).resolves.not.toThrow();
+// Should not reject - store is auto-initialized
+await expect(fetchTags()).resolves.toBeUndefined();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const { fetchTags } = useTags(); | |
| // Should not throw - store is auto-initialized | |
| await expect(fetchTags()).resolves.not.toThrow(); | |
| }); | |
| const { fetchTags } = useTags(); | |
| // Should not reject - store is auto-initialized | |
| await expect(fetchTags()).resolves.toBeUndefined(); | |
| }); |
🤖 Prompt for AI Agents
In client/apps/web/src/tag/__tests__/integration.test.ts around lines 283 to
287, the test uses toThrow on a Promise which is invalid; replace the assertion
to use Jest's resolves matcher (e.g., await
expect(fetchTags()).resolves.toBeUndefined() if fetchTags returns void, or await
expect(fetchTags()).resolves.toEqual(<expectedValue>) / .resolves.toBeDefined()
as appropriate) so the test asserts the Promise resolves rather than incorrectly
passing a Promise to toThrow.
| describe("Performance Integration", () => { | ||
| it("should handle large datasets efficiently", async () => { | ||
| // Configure repository with large dataset | ||
| const largeDataRepository = repositoryMockForScenario("large"); | ||
| setupTestWithRepository(largeDataRepository); | ||
| configureContainer({ customRepository: largeDataRepository }); | ||
|
|
||
| const { tags, fetchTags } = useTags(); | ||
|
|
||
| const startTime = performance.now(); | ||
| await fetchTags(); | ||
| const endTime = performance.now(); | ||
|
|
||
| expect(tags.value).toHaveLength(1000); | ||
| expect(endTime - startTime).toBeLessThan(1000); // Should complete within 1 second | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Avoid time-based assertions to reduce flakiness
Asserting completion under 1s can be flaky across CI environments. Focus on correctness and maybe relative performance checks without strict thresholds.
Replace the threshold with a sanity assertion, or drop timing assertion altogether:
-const startTime = performance.now();
await fetchTags();
-const endTime = performance.now();
...
-expect(endTime - startTime).toBeLessThan(1000);
+// Optionally assert dataset size only; drop timing constraint📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| describe("Performance Integration", () => { | |
| it("should handle large datasets efficiently", async () => { | |
| // Configure repository with large dataset | |
| const largeDataRepository = repositoryMockForScenario("large"); | |
| setupTestWithRepository(largeDataRepository); | |
| configureContainer({ customRepository: largeDataRepository }); | |
| const { tags, fetchTags } = useTags(); | |
| const startTime = performance.now(); | |
| await fetchTags(); | |
| const endTime = performance.now(); | |
| expect(tags.value).toHaveLength(1000); | |
| expect(endTime - startTime).toBeLessThan(1000); // Should complete within 1 second | |
| }); | |
| describe("Performance Integration", () => { | |
| it("should handle large datasets efficiently", async () => { | |
| // Configure repository with large dataset | |
| const largeDataRepository = repositoryMockForScenario("large"); | |
| setupTestWithRepository(largeDataRepository); | |
| configureContainer({ customRepository: largeDataRepository }); | |
| const { tags, fetchTags } = useTags(); | |
| await fetchTags(); | |
| expect(tags.value).toHaveLength(1000); | |
| // Optionally assert dataset size only; drop timing constraint | |
| }); |
| it("should debounce rapid operations", async () => { | ||
| const { createTag } = useTags(); | ||
|
|
||
| // Rapid fire multiple creates | ||
| const promises = Array.from({ length: 5 }, (_, i) => | ||
| createTag({ | ||
| name: `Rapid Tag ${i}`, | ||
| color: TagColors.Blue, | ||
| }), | ||
| ); | ||
|
|
||
| await Promise.all(promises); | ||
|
|
||
| // All should complete successfully | ||
| expect(mockRepository.create).toHaveBeenCalledTimes(5); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Test name and behavior mismatch: no debouncing is asserted
The test fires 5 parallel creates and asserts 5 calls; that’s the opposite of debouncing.
- Rename to “should handle rapid consecutive creates”.
- Or implement and assert actual debouncing (e.g., coalescing rapid calls to 1 create).
🤖 Prompt for AI Agents
In client/apps/web/src/tag/__tests__/integration.test.ts around lines 415 to
430, the test named "should debounce rapid operations" fires 5 parallel
createTag calls and asserts that mockRepository.create was called 5 times, which
contradicts debouncing; either rename the test to "should handle rapid
consecutive creates" to reflect the current behavior, or implement true
debouncing by updating the tag creation logic to coalesce rapid calls (e.g.,
buffer calls within a short timeframe and call repository.create once) and
change the assertion to expect a single repository.create call (or the expected
coalesced count); update the test name or implementation and assertions
accordingly so name and behavior match.
| export interface UseTagFormReturn { | ||
| readonly handleSubmit: ( | ||
| onSubmit: (data: TagFormData) => void, | ||
| ) => (e?: Event) => Promise<void>; | ||
| readonly errors: Ref<Record<string, string | undefined>>; | ||
| readonly defineField: (name: string) => FormFieldReturn; | ||
| readonly resetForm: (options?: { values?: Partial<TagFormData> }) => void; | ||
| readonly meta: Ref<{ valid: boolean; dirty: boolean; touched: boolean }>; | ||
| readonly isFormDisabled: Ref<boolean>; | ||
| readonly formTitle: Ref<string>; | ||
| readonly submitButtonText: Ref<string>; | ||
| readonly isCreateMode: Ref<boolean>; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Tighten defineField typing and surface only valid field names
The current signature allows any string; restrict it to "name" | "color" for safer usage.
Apply this diff:
export interface UseTagFormReturn {
readonly handleSubmit: (
onSubmit: (data: TagFormData) => void,
) => (e?: Event) => Promise<void>;
readonly errors: Ref<Record<string, string | undefined>>;
- readonly defineField: (name: string) => FormFieldReturn;
+ readonly defineField: (name: "name" | "color") => FormFieldReturn;
readonly resetForm: (options?: { values?: Partial<TagFormData> }) => void;
readonly meta: Ref<{ valid: boolean; dirty: boolean; touched: boolean }>;
readonly isFormDisabled: Ref<boolean>;
readonly formTitle: Ref<string>;
readonly submitButtonText: Ref<string>;
readonly isCreateMode: Ref<boolean>;
}
...
return {
handleSubmit: enhancedHandleSubmit,
errors,
- defineField: (name: string) => defineField(name as "name" | "color"),
+ defineField: (name: "name" | "color") => defineField(name),
resetForm,
meta,
isFormDisabled,
formTitle,
submitButtonText,
isCreateMode,
};Also applies to: 102-106
🤖 Prompt for AI Agents
In client/apps/web/src/tag/application/composables/useTagForm.ts around lines
32-44 (and also update the similar signature at lines 102-106), the defineField
function is typed too loosely as (name: string) — tighten it to only accept the
valid field keys by changing its parameter type to "name" | "color" (and update
FormFieldReturn usage if needed). Replace occurrences and any related overloads
so consumers can only call defineField("name") or defineField("color"), and
adjust any internal implementations or tests to match the narrowed type.
| const isDataLoaded = computed(() => tags.value.length > 0); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don’t conflate “data loaded” with “non-empty list”
isDataLoaded should indicate completion of the initial fetch, even when the list is empty. Introduce a dedicated hasFetched flag that resets on resetState and is set on successful fetch.
Apply this diff:
const tags: Ref<Tag[]> = ref([]);
const loading: Ref<LoadingStates> = ref({ ...defaultLoadingStates });
const error: Ref<TagError | null> = ref(null);
+const hasFetched: Ref<boolean> = ref(false);
...
-const isDataLoaded = computed(() => tags.value.length > 0);
+const isDataLoaded = computed(() => hasFetched.value);
...
const resetState = () => {
tags.value = [];
loadingUtils.reset();
errorUtils.clear();
+ hasFetched.value = false;
};
...
await withAsyncAction(
"fetch",
() => useCases?.fetchTags.execute(workspaceId) ?? Promise.resolve([]),
(result) => {
- tags.value = result as Tag[];
+ tags.value = result as Tag[];
+ hasFetched.value = true;
},
"FETCH_TAGS_ERROR",
"Failed to fetch tags",
);Also applies to: 124-128, 156-168
🤖 Prompt for AI Agents
In client/apps/web/src/tag/infrastructure/store/tag.store.ts around lines 81-82
(and also update usages at 124-128 and 156-168), replace the computed
isDataLoaded = computed(() => tags.value.length > 0) with a dedicated
ref<boolean> hasFetched = ref(false) initialized false; change isDataLoaded to
computed(() => hasFetched.value); in the fetch routine set hasFetched.value =
true after a successful fetch (not on empty result) and in resetState reset
hasFetched.value = false; update any logic in the other ranges that assumed
non-empty == loaded to instead use isDataLoaded/hasFetched.
| // Higher-order function to handle common action patterns | ||
| const withAsyncAction = async <T>( | ||
| loadingKey: keyof LoadingStates, | ||
| operation: () => Promise<T>, | ||
| onSuccess: (result: T) => void, | ||
| errorCode: string, | ||
| defaultErrorMessage: string, | ||
| ): Promise<void> => { | ||
| ensureInitialized(); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add JSDoc to withAsyncAction for maintainability
This higher-order function is central to error/loading handling. Document its behavior and guarantees.
I can add a concise JSDoc block describing parameters, return, and side effects (loading/error updates).
🤖 Prompt for AI Agents
In client/apps/web/src/tag/infrastructure/store/tag.store.ts around lines 130 to
138, add a concise JSDoc block immediately above the withAsyncAction function
that documents its generic type parameter, each parameter (loadingKey: keyof
LoadingStates, operation: () => Promise<T>, onSuccess: (result: T) => void,
errorCode: string, defaultErrorMessage: string), the Promise<void> return, and
its side effects (calls ensureInitialized, updates loading state, sets/clears
error state, invokes onSuccess on success, and handles/logs exceptions); keep it
short (one-sentence summary, one line per parameter, one line for return, one
line for side effects/guarantees).
| const createTag = async (tagData: CreateTagData): Promise<Tag> => { | ||
| let createdTag: Tag | null = null; | ||
|
|
||
| const workspaceStore = useWorkspaceStoreProvider()(); | ||
| const workspaceId = workspaceStore.currentWorkspace?.id ?? ""; | ||
|
|
||
| await withAsyncAction( | ||
| "create", | ||
| () => | ||
| useCases?.createTag.execute(workspaceId, tagData) ?? | ||
| Promise.resolve(null), | ||
| (result) => { | ||
| if (result) { | ||
| createdTag = result as Tag; | ||
| tags.value = [...tags.value, createdTag]; | ||
| } | ||
| }, | ||
| "CREATE_TAG_ERROR", | ||
| "Failed to create tag", | ||
| ); | ||
|
|
||
| if (!createdTag) { | ||
| throw new Error("Failed to create tag: No result returned from use case"); | ||
| } | ||
|
|
||
| return createdTag; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid throwing for control flow; return null on validation/domain errors
Throwing when createdTag is null breaks the store’s centralized error semantics and forces callers to catch exceptions. Prefer returning null to signal failure and keep error state in the store.
Apply this diff:
-const createTag = async (tagData: CreateTagData): Promise<Tag> => {
+const createTag = async (tagData: CreateTagData): Promise<Tag | null> => {
let createdTag: Tag | null = null;
const workspaceStore = useWorkspaceStoreProvider()();
const workspaceId = workspaceStore.currentWorkspace?.id ?? "";
await withAsyncAction(
"create",
() =>
useCases?.createTag.execute(workspaceId, tagData) ??
Promise.resolve(null),
(result) => {
if (result) {
createdTag = result as Tag;
tags.value = [...tags.value, createdTag];
}
},
"CREATE_TAG_ERROR",
"Failed to create tag",
);
- if (!createdTag) {
- throw new Error("Failed to create tag: No result returned from use case");
- }
-
- return createdTag;
+ return createdTag;
};Ensure upstream composables/services already treat null as “validation/error” (integration tests suggest they do).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const createTag = async (tagData: CreateTagData): Promise<Tag> => { | |
| let createdTag: Tag | null = null; | |
| const workspaceStore = useWorkspaceStoreProvider()(); | |
| const workspaceId = workspaceStore.currentWorkspace?.id ?? ""; | |
| await withAsyncAction( | |
| "create", | |
| () => | |
| useCases?.createTag.execute(workspaceId, tagData) ?? | |
| Promise.resolve(null), | |
| (result) => { | |
| if (result) { | |
| createdTag = result as Tag; | |
| tags.value = [...tags.value, createdTag]; | |
| } | |
| }, | |
| "CREATE_TAG_ERROR", | |
| "Failed to create tag", | |
| ); | |
| if (!createdTag) { | |
| throw new Error("Failed to create tag: No result returned from use case"); | |
| } | |
| return createdTag; | |
| }; | |
| const createTag = async (tagData: CreateTagData): Promise<Tag | null> => { | |
| let createdTag: Tag | null = null; | |
| const workspaceStore = useWorkspaceStoreProvider()(); | |
| const workspaceId = workspaceStore.currentWorkspace?.id ?? ""; | |
| await withAsyncAction( | |
| "create", | |
| () => | |
| useCases?.createTag.execute(workspaceId, tagData) ?? | |
| Promise.resolve(null), | |
| (result) => { | |
| if (result) { | |
| createdTag = result as Tag; | |
| tags.value = [...tags.value, createdTag]; | |
| } | |
| }, | |
| "CREATE_TAG_ERROR", | |
| "Failed to create tag", | |
| ); | |
| return createdTag; | |
| }; |
🤖 Prompt for AI Agents
In client/apps/web/src/tag/infrastructure/store/tag.store.ts around lines 170 to
196, the createTag function throws when createdTag is null which uses exceptions
for control flow; change the function to return null on failure instead: update
the function signature to return Promise<Tag | null>, remove the throw block,
and at the end return null when no result was produced (keeping the createdTag
return when present). Ensure callers handle a null return (upstream
composables/services already expect this per tests).
| const updateTag = async ( | ||
| id: string, | ||
| tagData: UpdateTagData, | ||
| ): Promise<Tag> => { | ||
| let updatedTag: Tag | null = null; | ||
|
|
||
| await withAsyncAction( | ||
| "update", | ||
| () => { | ||
| const workspaceStore = useWorkspaceStoreProvider()(); | ||
| const workspaceId = workspaceStore.currentWorkspace?.id ?? ""; | ||
| return ( | ||
| useCases?.updateTag.execute(workspaceId, id, tagData) ?? | ||
| Promise.resolve(null) | ||
| ); | ||
| }, | ||
| (result) => { | ||
| if (result) { | ||
| updatedTag = result as Tag; | ||
| const index = tags.value.findIndex((tag) => tag.id === id); | ||
| if (index !== -1 && updatedTag) { | ||
| // More efficient array update using direct assignment | ||
| const newTags = [...tags.value]; | ||
| newTags[index] = updatedTag; | ||
| tags.value = newTags; | ||
| } | ||
| } | ||
| }, | ||
| "UPDATE_TAG_ERROR", | ||
| "Failed to update tag", | ||
| ); | ||
|
|
||
| if (!updatedTag) { | ||
| throw new Error("Failed to update tag: No result returned from use case"); | ||
| } | ||
|
|
||
| return updatedTag; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Align updateTag’s error semantics with createTag
Mirror the same non-throwing pattern for updateTag to keep error handling consistent and avoid surprise exceptions.
Apply this diff:
-const updateTag = async (
+const updateTag = async (
id: string,
tagData: UpdateTagData,
-): Promise<Tag> => {
+): Promise<Tag | null> => {
let updatedTag: Tag | null = null;
await withAsyncAction(
"update",
() => {
const workspaceStore = useWorkspaceStoreProvider()();
const workspaceId = workspaceStore.currentWorkspace?.id ?? "";
return (
useCases?.updateTag.execute(workspaceId, id, tagData) ??
Promise.resolve(null)
);
},
(result) => {
if (result) {
updatedTag = result as Tag;
const index = tags.value.findIndex((tag) => tag.id === id);
if (index !== -1 && updatedTag) {
// More efficient array update using direct assignment
const newTags = [...tags.value];
newTags[index] = updatedTag;
tags.value = newTags;
}
}
},
"UPDATE_TAG_ERROR",
"Failed to update tag",
);
- if (!updatedTag) {
- throw new Error("Failed to update tag: No result returned from use case");
- }
-
- return updatedTag;
+ return updatedTag;
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const updateTag = async ( | |
| id: string, | |
| tagData: UpdateTagData, | |
| ): Promise<Tag> => { | |
| let updatedTag: Tag | null = null; | |
| await withAsyncAction( | |
| "update", | |
| () => { | |
| const workspaceStore = useWorkspaceStoreProvider()(); | |
| const workspaceId = workspaceStore.currentWorkspace?.id ?? ""; | |
| return ( | |
| useCases?.updateTag.execute(workspaceId, id, tagData) ?? | |
| Promise.resolve(null) | |
| ); | |
| }, | |
| (result) => { | |
| if (result) { | |
| updatedTag = result as Tag; | |
| const index = tags.value.findIndex((tag) => tag.id === id); | |
| if (index !== -1 && updatedTag) { | |
| // More efficient array update using direct assignment | |
| const newTags = [...tags.value]; | |
| newTags[index] = updatedTag; | |
| tags.value = newTags; | |
| } | |
| } | |
| }, | |
| "UPDATE_TAG_ERROR", | |
| "Failed to update tag", | |
| ); | |
| if (!updatedTag) { | |
| throw new Error("Failed to update tag: No result returned from use case"); | |
| } | |
| return updatedTag; | |
| }; | |
| const updateTag = async ( | |
| id: string, | |
| tagData: UpdateTagData, | |
| ): Promise<Tag | null> => { | |
| let updatedTag: Tag | null = null; | |
| await withAsyncAction( | |
| "update", | |
| () => { | |
| const workspaceStore = useWorkspaceStoreProvider()(); | |
| const workspaceId = workspaceStore.currentWorkspace?.id ?? ""; | |
| return ( | |
| useCases?.updateTag.execute(workspaceId, id, tagData) ?? | |
| Promise.resolve(null) | |
| ); | |
| }, | |
| (result) => { | |
| if (result) { | |
| updatedTag = result as Tag; | |
| const index = tags.value.findIndex((tag) => tag.id === id); | |
| if (index !== -1 && updatedTag) { | |
| // More efficient array update using direct assignment | |
| const newTags = [...tags.value]; | |
| newTags[index] = updatedTag; | |
| tags.value = newTags; | |
| } | |
| } | |
| }, | |
| "UPDATE_TAG_ERROR", | |
| "Failed to update tag", | |
| ); | |
| return updatedTag; | |
| }; |
🤖 Prompt for AI Agents
In client/apps/web/src/tag/infrastructure/store/tag.store.ts around lines
198-235, updateTag currently throws an Error when no result is returned; change
it to the same non-throwing pattern used by createTag by making the function
return a nullable Tag (update the signature to Promise<Tag | null>), remove the
throw for missing updatedTag, and simply return updatedTag (which may be null);
also update any callers/types to handle a nullable result and ensure the
withAsyncAction usage remains the same.
| const findTagsByColor = (color: string): Tag[] => { | ||
| if (!color || color.trim() === "") { | ||
| return []; | ||
| } | ||
| return tags.value.filter((tag) => tag.color === color); | ||
| }; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Type the color parameter as TagColors and simplify guard
Tag.color is TagColors; comparing to a string undermines type safety and forces extra string checks.
Apply this diff:
-const findTagsByColor = (color: string): Tag[] => {
- if (!color || color.trim() === "") {
- return [];
- }
- return tags.value.filter((tag) => tag.color === color);
-};
+const findTagsByColor = (color: Tag["color"] | null | undefined): Tag[] => {
+ if (!color) return [];
+ return tags.value.filter((tag) => tag.color === color);
+};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const findTagsByColor = (color: string): Tag[] => { | |
| if (!color || color.trim() === "") { | |
| return []; | |
| } | |
| return tags.value.filter((tag) => tag.color === color); | |
| }; | |
| const findTagsByColor = (color: Tag["color"] | null | undefined): Tag[] => { | |
| if (!color) return []; | |
| return tags.value.filter((tag) => tag.color === color); | |
| }; |
🤖 Prompt for AI Agents
In client/apps/web/src/tag/infrastructure/store/tag.store.ts around lines 257 to
263, change the findTagsByColor signature to accept a TagColors (not string) and
simplify the guard — remove the string.trim() check and any string-specific
logic; just return [] if color is falsy (or undefined) and otherwise filter
tags.value by tag.color === color. Ensure the parameter type is TagColors (or
optional TagColors if callers may pass undefined) to preserve type safety.
Description
This pull request introduces a comprehensive tags management system for the frontend, following a hexagonal architecture pattern consistent with the existing subscribers module. It includes full CRUD functionality, state management, API integration, Vue.js components, and thorough test coverage. Additionally, it adds new navigation i18n keys for both English and Spanish locales.
Tags Management System Implementation:
Internationalization (i18n) Updates:
client/apps/web/src/i18n/locales/en/global.json,client/apps/web/src/i18n/load.locale.test.ts) [1] [2] [3]Screenshots / Evidence
If there are visual changes, add screenshots. For APIs, include request/response examples if applicable.
Breaking changes or migrations?
Merge checklist