Skip to content

504: adds taggedPersonIds in create comment and update comment hooks#593

Open
DarrellRoberts wants to merge 15 commits into
developfrom
darrell/feature/tagging-request
Open

504: adds taggedPersonIds in create comment and update comment hooks#593
DarrellRoberts wants to merge 15 commits into
developfrom
darrell/feature/tagging-request

Conversation

@DarrellRoberts
Copy link
Copy Markdown
Collaborator

@DarrellRoberts DarrellRoberts commented Jun 3, 2026

Description

Adds taggedPersonIds in the create comment request and in the update comment request when the user tags another user

Branch created and based from darrell/feature/autocomplete

Related Issues

Closes #504

Changes

  • Adds taggedPersonIds in EntityComments
  • Adds taggedPersonIds to useUpdateComment
  • Adds taggedPersonIds to useCreateComment

Screenshots / Demos

If UI-related, add before/after or GIFs.

Checklist

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

@DarrellRoberts DarrellRoberts self-assigned this Jun 3, 2026
@nadavosa
Copy link
Copy Markdown
Collaborator

nadavosa commented Jun 3, 2026

Code Review

This PR stacks on #537 (not yet merged). The actual new logic is the async taggedPersonIds resolution in handleAddComment and handleSaveEdit — but it has several real issues.

🔴 Blockers

1. Raw fetch instead of axios — auth headers missing

const response = await fetch(`${apiPathUser}/${id}`);

The rest of the app uses axios which carries auth headers automatically. A raw fetch has no auth and will return 401 in production. Use axios or the existing fetchData utility.

2. apiPathUser trailing slash → double-slash URL

apiPathUser ends with / (unfixed from #537), making this …/user//4. Fix the trailing slash in constants.ts.

3. undefined not filtered — wrong null check

taggedPersonIds = resolvedPersonIds?.filter((id) => id !== null);

If data.data.personId is undefined (missing field), undefined !== null is true and it passes the filter. Use:

.filter((id): id is number => id != null)

4. Button not disabled during async fetch gap

isCreating only becomes true after createComment is called, but there's now an async gap (the Promise.all) before that. The button stays enabled, allowing double-submission. Add a local isFetching state to cover the whole async flow.


🟡 Non-blocking

5. Extra API calls unnecessary

useCommentTag already fetches all coordinators. If ApiUserGet includes personId, extract it from the already-loaded list instead of making N additional fetches:

taggedPersonIds = tags
  .map(tag => users?.find(u => u.id === tag.id)?.personId)
  .filter((id): id is number => id != null);

6. Silent failure on fetch error

The catch just console.errors and drops the tag silently. Show a toast so the user knows the tag wasn't saved.

7. Duplicated logic

handleAddComment and handleSaveEdit have identical person-ID resolution code. Extract a resolveTaggedPersonIds(tags) helper.


Note: This PR also inherits #537's open issues (trailing slash, dead useDebounce). Recommend fixing #537 first, then rebasing this branch.

@DarrellRoberts
Copy link
Copy Markdown
Collaborator Author

thanks @nadavosa ! I've resolved the Blockers 1,2,3,4 along with non-blocker 6 and 7. 5 is not possible as we have personId only on a single user fetch, not on the whole user fetch

@DarrellRoberts
Copy link
Copy Markdown
Collaborator Author

have also rebased with the autocomplete branch which should fix blocker 2

@nadavosa nadavosa force-pushed the darrell/feature/tagging-request branch from 1813f4a to d9c1554 Compare June 6, 2026 16:38
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.

modifiy comment mutation hook to use tagging data

2 participants