Conversation
Add a lightweight tagging system for organising monitors on the dashboard. - New `tags` and `monitor_tags` tables with Drizzle schema, relations, and cascade deletes - Tag CRUD API (GET/POST/PATCH/DELETE /api/tags) with tier gating (free=0, pro=10, power=unlimited) - Monitor-tag assignment API (PUT /api/monitors/:id/tags) with replace semantics and ownership validation - Pro tier limited to 2 tags per monitor, power tier unlimited - 10 preset colours validated server-side via Zod enum - Case-insensitive tag name uniqueness per user - GET /api/monitors and GET /api/monitors/:id now include tags array (N+1 free via batched join) - Frontend: TagBadge, TagManager (CRUD dialog), TagPicker (popover with checkboxes) - Dashboard: tag filter bar with OR-logic multi-tag filtering (client-side) - MonitorCard: tag badges with +N overflow - MonitorDetails: inline tag management with TagPicker - CreateMonitorDialog: optional tag picker on creation - 46 new tests covering schemas, tier limits, filter logic, and ownership validation https://claude.ai/code/session_014Rnw8oXjKXwmywjXuh8Vxw
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (15)
📝 WalkthroughWalkthroughAdds end-to-end monitor tagging: DB schema and storage for tags and monitor_tags, new server routes for tag CRUD and monitor-tag assignment with tier/ownership validation, client hooks and components (TagBadge/TagPicker/TagManager), dashboard filtering, monitor/tag UI, and comprehensive tests and docs updates. Changes
Sequence DiagramssequenceDiagram
participant User as User (Client)
participant UI as TagPicker / CreateMonitorDialog
participant API as API Server
participant DB as Database
participant Cache as QueryClient Cache
User->>UI: Select tags and submit monitor
UI->>API: POST /api/monitors/create (monitor data)
API->>DB: Create monitor record
DB-->>API: Monitor created (id)
UI->>API: PUT /api/monitors/:id/tags { tagIds }
API->>DB: Verify ownership & tag ownership, enforce limits
alt validation fails
DB-->>API: Error (403/422)
API-->>UI: Error response
UI-->>User: Show error toast
else success
DB->>DB: set monitor_tags entries
DB-->>API: Updated monitor with tags
API->>Cache: Invalidate monitor queries
API-->>UI: 200 with updated tags
UI-->>User: Render tags
end
sequenceDiagram
participant User as User (Client)
participant Dashboard as Dashboard Component
participant TagAPI as useTags Hook
participant MonAPI as useMonitors Hook
participant DB as Database
participant Cache as QueryClient Cache
User->>Dashboard: Load page
Dashboard->>TagAPI: fetch tags
TagAPI->>DB: SELECT tags WHERE user_id=?
DB-->>TagAPI: [tags]
TagAPI->>Cache: store tags
Dashboard->>MonAPI: fetch monitors (with-tags)
MonAPI->>DB: SELECT monitors JOIN monitor_tags...
DB-->>MonAPI: [monitors with tags]
User->>Dashboard: Toggle tag filter(s)
Dashboard->>Dashboard: Filter monitors where ANY monitor.tags IN selectedTagIds
Dashboard-->>User: Render filtered monitors
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 19
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/src/components/CreateMonitorDialog.tsx`:
- Around line 42-49: The selectedTagIds state (selectedTagIds,
setSelectedTagIds) persists across dialog closes; update CreateMonitorDialog to
clear tag selection whenever the dialog is closed or unmounted by resetting
selectedTagIds to [] — for example, call setSelectedTagIds([]) inside the dialog
onClose handler and/or add a useEffect that watches the dialog open prop (e.g.,
isOpen or visible) and resets selectedTagIds when it becomes false (and
optionally on cleanup/unmount) so cancelling/dismissing the dialog never retains
prior selections.
- Around line 201-208: The "Tags (optional)" label is unbound and not
accessible; either replace the <label> element with a plain text container
(e.g., <div> or <span>) or associate it with the TagPicker by giving the
TagPicker a stable id (e.g., tagPickerId) and wiring the label via htmlFor or
aria-labelledby; update the CreateMonitorDialog markup where TagPicker is
rendered and, if adding an id, update TagPicker props (and its root element) to
accept and apply that id so screen readers correctly announce the label.
In `@client/src/components/MonitorCard.tsx`:
- Around line 172-183: The response schema for the monitors endpoint in
shared/routes.ts is missing the tags field so Zod strips tags during validation;
update the response schema at the array entry (currently z.array(z.custom<typeof
monitors.$inferSelect>())) to include the tags property the server returns
(e.g., merge/infer monitors.$inferSelect with { tags: { id: number; name:
string; colour: string }[] }) so the parsed type from
api.monitors.list.responses[200].parse() preserves monitor.tags; ensure the
shape matches storage.getMonitorsWithTags output and rebuild types so
client/src/hooks/use-monitors.ts and client/src/components/MonitorCard.tsx
receive tags.
In `@client/src/components/TagBadge.tsx`:
- Around line 22-34: The remove button in TagBadge (the conditional block using
onRemove and rendering <button> with the <X /> icon) needs an accessible label;
add an aria-label to the button (e.g. "Remove tag" or if TagBadge exposes a
label/children use a dynamic label like `Remove ${label}`) so screen readers
convey the button's purpose, and keep existing event handlers and classes
unchanged.
In `@client/src/components/TagManager.tsx`:
- Around line 125-130: The Edit and Delete icon-only Button components lack
accessible names; update the two Button elements that call startEdit(tag) and
setConfirmDeleteId(tag.id) to include descriptive aria-label attributes (e.g.,
aria-label={`Edit tag ${tag.name ?? tag.id}`} and aria-label={`Delete tag
${tag.name ?? tag.id}`}) so screen readers can announce their purpose; keep
existing props (size, variant, onClick, className) and do not remove the Pencil
or Trash2 icons.
- Around line 36-39: The code in TagManager.tsx unsafely casts user tier via
(user as any)?.tier — replace this pattern by adding a shared helper getUserTier
that validates against TAG_LIMITS and returns a UserTier (default "free"), then
use const tier = getUserTier(user) to compute limit = TAG_LIMITS[tier] ??
TAG_LIMITS.free and atLimit/userTags logic; ensure the helper is exported from a
shared utils module and imported into TagManager to remove the any cast while
keeping existing symbols (TAG_LIMITS, UserTier, userTags) intact.
- Around line 51-55: The update/delete handlers (handleUpdate, handleDelete)
currently only pass onSuccess to the mutation calls (updateTag, deleteTag) so
failures are silent; add onError callbacks that call the app toast (useToast) to
surface failures to users and optionally roll back UI state (e.g., keep editing
or clear selection). Import and call useToast inside the component, then pass
onError: (err) => toast({ title: "Tag update failed", description: err?.message
|| "Unknown error", status: "error" }) for updateTag and a similar message for
deleteTag; keep the existing onSuccess behavior (setEditingId(null) or UI
updates) intact.
In `@client/src/hooks/use-tags.ts`:
- Around line 68-70: useUpdateTag and useDeleteTag currently fail to invalidate
monitor caches so renamed/deleted tags remain stale; update both mutation
onSuccess handlers to call queryClient.invalidateQueries for
api.monitors.list.path and api.monitors.get.path in addition to the existing
invalidations (keep existing api.tags.list.path invalidation and toast).
Specifically, inside useUpdateTag and useDeleteTag replace or extend the
onSuccess callback to call queryClient.invalidateQueries({ queryKey:
[api.monitors.list.path] }) and queryClient.invalidateQueries({ queryKey:
[api.monitors.get.path] }) so both the monitor list and all monitor detail
caches are refreshed after tag changes.
In `@client/src/pages/Dashboard.tsx`:
- Around line 246-248: The filter uses unsafe casts "(m as any).tags" in the
calculation of filteredMonitors; update the monitor typing instead: define a
shared MonitorWithTags type (e.g. Monitor & { tags?: { id: number; name: string;
colour: string }[] }) and change the return type of useMonitors (and any related
hooks/handlers) to MonitorWithTags[] so the expression becomes m.tags?.some(...)
without as any; update any references to Monitor where tags are expected to use
MonitorWithTags to preserve type safety.
- Around line 208-217: The "All" filter button currently can act as a submit
button in forms; update the JSX button element (the one with onClick={() =>
setSelectedTagIds([])} that reads the selectedTagIds state) to include an
explicit type="button" attribute so it does not trigger form submission; ensure
you add the attribute to the same button element rather than changing its
behavior in setSelectedTagIds or surrounding markup.
- Around line 221-237: The tag filter buttons rendered in Dashboard.tsx (the
<button> that calls setSelectedTagIds in its onClick for tag.id and uses
isActive/tag.name/tag.colour) need an explicit type="button" attribute to
prevent them from acting as submit buttons in forms; add type="button" to that
button element so clicks only toggle selection via setSelectedTagIds.
In `@client/src/pages/MonitorDetails.tsx`:
- Around line 49-51: Replace the unsafe cast "(user as any)?.tier" by making the
user object's type include a properly typed tier and then deriving userTier from
that typed property; update the User type (or extend it) so it declares tier?:
UserTier (or string union matching UserTier) and then compute const userTier =
(user?.tier ?? "free") as UserTier with a runtime fallback to "free" (or
validate unknown strings and map them to "free") so
TAG_ASSIGNMENT_LIMITS[userTier] stays type-safe; reference symbols: user, User
type (from `@shared/models/auth`), UserTier, userTier, and TAG_ASSIGNMENT_LIMITS.
- Around line 354-363: The repeated (monitor as any).tags casts mean the Monitor
type returned by useMonitor is missing tags—add a typed variant (e.g., type
MonitorWithTags = Monitor & { tags?: { id:number; name:string; colour:string }[]
}) and update usages in MonitorDetails (replace casts around monitor with
MonitorWithTags or narrow the type where TagBadge is rendered) so TagBadge
receives a properly typed tag prop; also add error handling around
setMonitorTags (use the mutation's onError / catch its promise and show user
feedback via toast or set an error state) so failed tag updates surface to the
user instead of failing silently.
In `@server/routes.ts`:
- Around line 2197-2207: The pre-read uniqueness check using listUserTags is
racy; wrap the call to storage.createTag (and the similar storage.updateTag
block around lines 2232-2248) in a try/catch, detect the DB uniqueness violation
(constraint name tags_user_name_lower_uniq or the DB-specific unique-constraint
error code) and respond with res.status(409).json({ message: "A tag with this
name already exists.", code: "TAG_NAME_CONFLICT" }) instead of letting it fall
through to the 500 handler; keep the existing pre-check but add this DB-level
mapping so concurrent requests return TAG_NAME_CONFLICT.
- Around line 2219-2223: Validate and parse route params with a Zod schema
before converting or calling storage: replace direct Number(req.params.id) usage
by parsing req.params through a Zod params schema (e.g., a schema that validates
"id" is an integer/coercible string and transforms to Number) and return a 400
on parse failures; then use the parsed numeric id when calling storage.getTag
(and the other endpoints that convert req.params.id) so invalid IDs fail fast
instead of producing NaN and bubbling into DB calls.
- Around line 307-317: Update the shared API contract to expose the enriched
monitor shape (including tags) so client hooks get correct typings: modify the
types referenced by api.monitors.list.responses[200] and
api.monitors.get.responses[200] in the shared/ types to match the shape returned
by storage.getMonitorsWithTags and storage.getMonitorWithTags (i.e., include a
tags field), export that enriched Monitor type from shared/ and update any
imports to use the shared/ alias so the server handlers (app.get for
api.monitors.list.path and api.monitors.get.path) and client hooks consume the
same enriched type.
- Around line 2170-2214: The route handler for app.post(api.tags.create.path)
returns inconsistent error payloads—update every JSON error response in this
handler (including the Zod validation catch for z.ZodError, the free-plan and
tier-limit branches, and any 4xx branches like name conflict) to always return
an object with both message and code fields (e.g., change res.status(400).json({
message: err.errors[0].message }) to res.status(400).json({ message:
err.errors[0].message, code: "INVALID_INPUT" }) and ensure the Zod catch, the
TAG_LIMIT_REACHED branches, and the TAG_NAME_CONFLICT branch all include
appropriate code strings); audit the nearby handlers referenced in the comment
(the same file ranges) and make the same change so every error path from
createTagSchema.parse, storage.countUserTags, storage.listUserTags and
storage.createTag returns { message, code } consistently.
In `@server/storage.ts`:
- Around line 476-480: setMonitorTags currently issues a delete then optional
insert outside a transaction, so an interrupt or interleaving request can leave
the monitor with no tags; fix by wrapping the delete and insert into a single
database transaction (use the project's db.transaction/txn API) so both
operations are committed or rolled back together, perform the delete inside the
transaction even when tagIds is empty, run the insert (monitorTags.insert or
db.insert(...).values(...)) within the same transaction context, and await the
transaction promise so errors cause rollback.
In `@shared/routes.ts`:
- Around line 88-90: Update setMonitorTagsSchema to reject zero and duplicate
IDs at validation: change the element validator to require positive integers (so
0 is invalid) and add an array-level refine that checks uniqueness (e.g.,
compare new Set(tagIds).size to tagIds.length) and returns a clear error message
like "duplicate tag ids" when duplicates are found; reference the
setMonitorTagsSchema and the tagIds field to locate where to apply these
changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d83118af-3b0b-4dd6-a0c8-a1dfed4c1bb1
📒 Files selected for processing (14)
client/src/components/CreateMonitorDialog.tsxclient/src/components/MonitorCard.tsxclient/src/components/TagBadge.tsxclient/src/components/TagManager.tsxclient/src/components/TagPicker.tsxclient/src/hooks/use-tags.tsclient/src/pages/Dashboard.tsxclient/src/pages/MonitorDetails.tsxserver/routes.tsserver/routes/tags.test.tsserver/storage.tsshared/models/auth.tsshared/routes.tsshared/schema.ts
| {monitor.tags && monitor.tags.length > 0 && ( | ||
| <div className="flex flex-wrap gap-1 pt-1"> | ||
| {monitor.tags.slice(0, 2).map((tag) => ( | ||
| <TagBadge key={tag.id} tag={tag} /> | ||
| ))} | ||
| {monitor.tags.length > 2 && ( | ||
| <span className="inline-flex items-center rounded-full bg-secondary/50 px-2 py-0.5 text-xs text-muted-foreground"> | ||
| +{monitor.tags.length - 2} more | ||
| </span> | ||
| )} | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check monitors list route definition and response schema
rg -n "monitors.*list" --type=ts -A 10 --max-count=20
# Look for getMonitors usage and implementation
rg -n "getMonitors" --type=ts -A 5 --max-count=20Repository: bd73-com/fetchthechange
Length of output: 23586
🏁 Script executed:
# Verify the response schema includes tags by checking the endpoint definition
rg -n "api.monitors.list" --type=ts shared/routes.ts -A 3 -B 1Repository: bd73-com/fetchthechange
Length of output: 49
🏁 Script executed:
# Check the shared/routes.ts file to see API definition
cat shared/routes.ts | head -100
# Also check for Monitor type definition to see if tags are included
rg -n "type Monitor" --type=ts -A 10Repository: bd73-com/fetchthechange
Length of output: 10675
🏁 Script executed:
# Find the complete api.monitors.list definition with response schema
rg -n "monitors: {" -A 50 shared/routes.ts | head -100Repository: bd73-com/fetchthechange
Length of output: 152
🏁 Script executed:
# Find the api.monitors.list definition
rg -n "monitors:" -A 100 shared/routes.ts | grep -A 50 "list:"Repository: bd73-com/fetchthechange
Length of output: 2743
🏁 Script executed:
# Check the Monitor type definition from the monitors table
rg -n "export const monitors" -A 30 shared/schema.tsRepository: bd73-com/fetchthechange
Length of output: 2531
🏁 Script executed:
# Check what the actual server endpoint returns and how it's typed
cat server/routes.ts | sed -n '307,315p'
# Also verify the getMonitorsWithTags return type
cat server/storage.ts | sed -n '483,510p'Repository: bd73-com/fetchthechange
Length of output: 1866
🏁 Script executed:
# Check if the schema is used for actual validation anywhere
rg -n "api.monitors.list.responses" --type=ts -B 2 -A 2Repository: bd73-com/fetchthechange
Length of output: 477
🏁 Script executed:
# Double-check the client-side type by looking at how MonitorCard uses the data
cat client/src/components/MonitorCard.tsx | head -50Repository: bd73-com/fetchthechange
Length of output: 2258
Mismatch between API schema definition and server implementation: tags are returned but stripped by validation.
The server endpoint correctly calls storage.getMonitorsWithTags(userId) at server/routes.ts:307-310, which returns monitors with tags included. However, the API response schema defined in shared/routes.ts:104 only specifies the base Monitor type without tags. When the client hook validates the response with api.monitors.list.responses[200].parse() at client/src/hooks/use-monitors.ts:13, Zod strips the tags from the response because they're not in the schema definition. This causes the tags to always be undefined in the component, making the tag rendering feature non-functional despite the defensive checks.
Fix required: Update the response schema in shared/routes.ts:104 from z.array(z.custom<typeof monitors.$inferSelect>()) to explicitly include the tags type that the server actually returns, such as: z.array(z.custom<typeof monitors.$inferSelect & { tags: { id: number; name: string; colour: string }[] }>()).
The component's defensive check is appropriate, but it will always guard against undefined because the schema validation prevents tags from reaching the component.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/src/components/MonitorCard.tsx` around lines 172 - 183, The response
schema for the monitors endpoint in shared/routes.ts is missing the tags field
so Zod strips tags during validation; update the response schema at the array
entry (currently z.array(z.custom<typeof monitors.$inferSelect>())) to include
the tags property the server returns (e.g., merge/infer monitors.$inferSelect
with { tags: { id: number; name: string; colour: string }[] }) so the parsed
type from api.monitors.list.responses[200].parse() preserves monitor.tags;
ensure the shape matches storage.getMonitorsWithTags output and rebuild types so
client/src/hooks/use-monitors.ts and client/src/components/MonitorCard.tsx
receive tags.
| const tier = ((user as any)?.tier || "free") as UserTier; | ||
| const limit = TAG_LIMITS[tier] ?? TAG_LIMITS.free; | ||
| const atLimit = userTags.length >= limit; | ||
| const isFree = tier === "free"; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Same type safety issue with tier casting.
Consistent with MonitorDetails, the (user as any)?.tier pattern should be addressed project-wide. Consider creating a helper function to safely extract and validate the user tier.
♻️ Suggested helper function (could be in a shared utils file)
import { TAG_LIMITS, type UserTier } from "@shared/models/auth";
export function getUserTier(user: { tier?: string } | null | undefined): UserTier {
const tier = user?.tier;
return tier && tier in TAG_LIMITS ? (tier as UserTier) : "free";
}Then use: const tier = getUserTier(user);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/src/components/TagManager.tsx` around lines 36 - 39, The code in
TagManager.tsx unsafely casts user tier via (user as any)?.tier — replace this
pattern by adding a shared helper getUserTier that validates against TAG_LIMITS
and returns a UserTier (default "free"), then use const tier = getUserTier(user)
to compute limit = TAG_LIMITS[tier] ?? TAG_LIMITS.free and atLimit/userTags
logic; ensure the helper is exported from a shared utils module and imported
into TagManager to remove the any cast while keeping existing symbols
(TAG_LIMITS, UserTier, userTags) intact.
server/routes.ts
Outdated
| const userId = req.user.claims.sub; | ||
| const tagId = Number(req.params.id); | ||
|
|
||
| const existing = await storage.getTag(tagId, userId); | ||
| if (!existing) return res.status(404).json({ message: "Not found" }); |
There was a problem hiding this comment.
Validate :id params before converting them.
Number(req.params.id) can produce NaN, and these values are passed straight into storage calls. Bad path input should fail fast with a validation response, not bubble into a DB error path.
As per coding guidelines, all request body, params, and query values MUST be validated with Zod schemas before use.
Also applies to: 2259-2263, 2276-2284
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/routes.ts` around lines 2219 - 2223, Validate and parse route params
with a Zod schema before converting or calling storage: replace direct
Number(req.params.id) usage by parsing req.params through a Zod params schema
(e.g., a schema that validates "id" is an integer/coercible string and
transforms to Number) and return a 400 on parse failures; then use the parsed
numeric id when calling storage.getTag (and the other endpoints that convert
req.params.id) so invalid IDs fail fast instead of producing NaN and bubbling
into DB calls.
Tests cover GET/POST/PATCH/DELETE /api/tags, PUT /api/monitors/:id/tags, and verifies monitors list/get now include tags. Covers tier limits, duplicate name detection, foreign tag ownership, and assignment limits. https://claude.ai/code/session_014Rnw8oXjKXwmywjXuh8Vxw
- Catch PostgreSQL 23505 (unique constraint violation) on both POST and PATCH tag routes so concurrent duplicate-name requests return 409 instead of 500. - Wrap setMonitorTags DELETE+INSERT in a db.transaction() to prevent orphaned state on mid-operation failures. https://claude.ai/code/session_014Rnw8oXjKXwmywjXuh8Vxw
Add Tags FAQ section to Support page, update pricing feature lists on Pricing and LandingPage, add tags row to README tier table, and update docsAccuracy test to account for the new Tags section. https://claude.ai/code/session_014Rnw8oXjKXwmywjXuh8Vxw
- Reset tag selection when CreateMonitorDialog closes (bug: stale tags)
- Invalidate monitor cache on tag update (stale renamed tags)
- Reject tagId 0 and duplicates in setMonitorTagsSchema
- Validate route params (reject NaN/negative IDs with 400)
- Add consistent {message, code} on all tag route error responses
- Add type="button" to dashboard filter buttons
- Add aria-labels to icon-only buttons (TagBadge, TagManager)
- Replace unbound <label> with <span> in CreateMonitorDialog
https://claude.ai/code/session_014Rnw8oXjKXwmywjXuh8Vxw
The tags feature (PR #86) added new tables but missed the startup ensureTable migration, causing getMonitorsWithTags() to fail with a SQL error on databases that haven't run drizzle-kit push. https://claude.ai/code/session_012qN2riMykNDMYS26j3TJkY
* fix: ensure tags and monitor_tags tables exist at startup The tags feature (PR #86) added new tables but missed the startup ensureTable migration, causing getMonitorsWithTags() to fail with a SQL error on databases that haven't run drizzle-kit push. https://claude.ai/code/session_012qN2riMykNDMYS26j3TJkY * test: make tag DDL assertions more specific Address CodeRabbit feedback: replace weak `includes("tags")` assertion (which also matches "monitor_tags") with `includes("CREATE TABLE IF NOT EXISTS tags")`. Add dedicated test for tag table DDL columns, indexes, and constraints, following the pattern of existing table tests. https://claude.ai/code/session_012qN2riMykNDMYS26j3TJkY --------- Co-authored-by: Claude <noreply@anthropic.com>
Summary
Changes
tagsandmonitor_tagstables with relations and cascade deletesgetMonitorsWithTagsvia batched joinTest plan
npm run checkpasses (only pre-existing scraper.ts errors)npm run testpasses (1083 tests, 46 new)npm run buildsucceedshttps://claude.ai/code/session_014Rnw8oXjKXwmywjXuh8Vxw
Summary by CodeRabbit
New Features
Documentation
Tests