Skip to content

761: Adds PATCH hook to read tagged comment notifications#762

Merged
need4deed merged 11 commits into
developfrom
darrell/feat/patch-tagged-comments-read
Jul 13, 2026
Merged

761: Adds PATCH hook to read tagged comment notifications#762
need4deed merged 11 commits into
developfrom
darrell/feat/patch-tagged-comments-read

Conversation

@DarrellRoberts

Copy link
Copy Markdown
Collaborator

Description

Creates a PATCH hook for tagged comment notifications. When the user clicks on the notification, it triggers this request and writes the readAt property, reducing the number of notifications for tagged comments

Related Issues

Closes #761

Changes

  • Creates usePatchTaggedComments hook
  • Adds a noToast property to useMutation hook, as I didn't want a toast notification everytime a user clicks on an unread notificaiton (if it is needed then it's also no problem)
  • Adds a scrollTo sideeffect to Profile Layout if it has parameter scrollTo with setTimeout. This was so when the user clicks on the notification, the scrollbar navigates to the Coordinator Comments
  • Background-color of tagged-notification depends on whether the notification has been read

Screenshots / Demos

tag_patch

Checklist

  • WITHIN THE SCOPE OF AN ISSUE; No unnecessary files included
  • Tests added/updated
  • Documentation updated
  • CI passes

@DarrellRoberts DarrellRoberts self-assigned this Jul 3, 2026
@need4deed need4deed requested a review from nadavosa July 7, 2026 15:17
@nadavosa

nadavosa commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator

Review

Adds a usePatchTaggedComments hook that marks a tagged comment as read when clicked, updates notification badge/row styling, and adds scroll-to-comment behavior.

Findings (most severe first):

  1. src/components/Dashboard/Home/TaggedNotification.tsx:26-29await updateReadTagComment({ read_at: new Date() }) awaits react-query's mutate (fire-and-forget, returns void), not mutateAsync. The await is a no-op — the click navigates away before the PATCH/invalidation actually completes. Should use mutateAsync.

  2. src/components/Dashboard/Home/NewestTaggedComments.tsx:22-24handleIsRead treats "no matching tag found" as read (undefined !== nulltrue). While useCurrentUser() is loading, personId defaults to -1, so the tag lookup finds nothing, the notification renders as already-read, and the mark-as-read PATCH gets silently skipped on click for that render.

  3. src/components/Layout/DashboardLayout/NotificationBadge.tsx:30-32 — Unread count flattens all tagged persons per comment (tagComments?.flatMap(c => c.taggedPersons)?.filter(c => !c.readAt)), not just the current user's entry. A comment tagging both User A and User B, where only B hasn't read it, still increments A's badge — count is contaminated by other users' read state.

  4. SDK-contract gap — src/hooks/usePatchTaggedComments.ts:5-15 — Invents PATCH ${apiPathComment}/${commentId}/read with a snake_case read_at body field that doesn't exist in the published need4deed-sdk (v0.0.118) — no endpoint, and every other field/hook in the codebase uses camelCase readAt (e.g. useUpdateComment.ts PATCHes ${apiPathComment}/${commentId} directly, camelCase body). Per .claude/shared-rules.md, API-surface changes should go through the SDK contract first rather than being guessed locally in fe.

  5. Reuse nituseUpdateComment.ts already PATCHes the same comment resource; this adds a near-duplicate one-off hook for a sibling sub-resource instead of extending that existing path.

Findings 1-3 are concrete correctness bugs with reproducible failure scenarios; 4-5 are rule/process concerns rather than crashes.

@DarrellRoberts

Copy link
Copy Markdown
Collaborator Author

thanks @nadavosa for reviewing! All good points, especially point 3. which I completely missed (needed to test with multiple tags).
I disagree only with point 5. as useUpdateComment is a completely different hook which edits a comment with a different endpoint so should be separate.

I've now pushed my changes

@nadavosa nadavosa left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Claude went through this:
Likely bug — TaggedNotification.tsx handleReadTag:

const handleReadTag = async (e: React.MouseEvent) => {
  if (isRead) return;
  e.preventDefault();
  await updateReadTagComment({ id: personId, readAt: new Date() });
};

Since this cancels the wrapped <Link>'s navigation via preventDefault(), and there's no navigation call after the mutation resolves, the first click on an unread notification only marks it read — it doesn't take the user anywhere. They'd need to click the same notification a second time (once isRead flips to true and the early-return skips preventDefault) to actually navigate to the comment. Given the PR description says clicking should scroll to Coordinator Comments, this seems like it should either not call preventDefault at all (fire-and-forget the mark-as-read alongside normal navigation), or navigate programmatically after the mutation resolves.

Minor — NewestTaggedComments.tsx handleIsRead:

return taggedComment?.readAt !== null;

If taggedComment isn't found in taggedPersons (shouldn't normally happen, but there's no guard), undefined !== null evaluates to true, silently treating "not found" as "read". Probably safer as !!taggedComment?.readAt.

Good catch (no action needed): I double-checked the scrollTo: "coordinator-comments" unification against the old per-entity-type hash targets (opportunity-volunteers-container, communication-tracker-container) — those actually pointed at unrelated sections (volunteer matching / communication log), not the comments section. The new approach correctly targets the shared EntityComments container used by all three entity types, so this is an improvement over the previous behavior.

@DarrellRoberts

Copy link
Copy Markdown
Collaborator Author

thanks @nadavosa , have pushed my changes

@need4deed need4deed merged commit 72cabf5 into develop Jul 13, 2026
1 check passed
@need4deed need4deed deleted the darrell/feat/patch-tagged-comments-read branch July 13, 2026 15:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feature: create PATCH request for read tagging notifications

3 participants