Skip to content

503 feat: creates autocomplete for tagging feature#537

Open
DarrellRoberts wants to merge 16 commits into
developfrom
darrell/feature/auto-complete
Open

503 feat: creates autocomplete for tagging feature#537
DarrellRoberts wants to merge 16 commits into
developfrom
darrell/feature/auto-complete

Conversation

@DarrellRoberts
Copy link
Copy Markdown
Collaborator

@DarrellRoberts DarrellRoberts commented May 15, 2026

Description

Triggers autocomplete menu for when user triggers a user tag @

PLEASE NOTE: my be wasn't working when I implemented this, so it's quite likely that there are bugs. Please test with a running be that can fetch from /user

Related Issues

Closes #503

Changes

  • Installs textarea-caret library (neccessary for position of autocomplete). Link to library
  • Adds useDebounce hook to /hooks
  • Adds <Autocomplete> component along with logic
  • Modifies useComment hook

Screenshots / Demos

image image

Checklist

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

@ivannissimrch
Copy link
Copy Markdown
Collaborator

I tested with BE running against local Docker.

@ triggers the autocomplete dropdown at caret position. Selecting a user closes the dropdown and inserts text.

  • Missing name fields in BE response: /user returns user account data, but no name fields (firstName, fullName).
    tsx renders user.fullName, which is undefined, so the dropdown shows avatars only, and selecting a user inserts @undefined into the comment.

-Search filter not working: Typing after @ sends the correct query param, but the BE returns all users unfiltered.

Screenshot from 2026-05-17 09-39-03

@need4deed
Copy link
Copy Markdown
Contributor

@DarrellRoberts did you see this comment of ivan?

@nadavosa
Copy link
Copy Markdown
Collaborator

nadavosa commented May 18, 2026

some more inputs

  • Hardcoded user IDs in CommentDisplay: the autocomplete is gated on [4, 6, 67].includes(userId). This will silently break for every other user and must be removed before merge.

  • Wrong guard operator — if the intent is "hide when no users found", the condition should be !users || users.length === 0, not &&. With && the list never hides when both are falsy together (they always are when empty), so verify the exact logic you want.

  • Caret position calculated on wrong string: textarea-caret is called with the full comment value, but it should receive only the text up to the cursor (comment.slice(0, cursorPosition)). Otherwise the overlay drifts as the user types past the @.

  • userIds missing from useCallback deps: the callback closes over userIds but it isn't listed in the dependency array, so stale IDs will be used after the first autocomplete selection.

  • Fragile count mutation: incrementing a useRef counter to force a re-render is a pattern that fights React. Use useState or restructure so the state that actually changed drives the render.

  • AutocompleteRow defined inside Autocomplete: this recreates the component function on every render, which causes React to unmount/remount it each time. Move it to module scope or its own file.

  • CSS typo user-selects: the property is user-select. Browsers silently ignore unknown properties, so text selection won't be suppressed.

  • showAutocomplete effect doesn't track cursor movement: the effect that decides whether to show the dropdown only fires when comment changes. If the user moves the caret with arrow keys (without changing text) the dropdown state won't update.

  • Redundant optional chaining on guaranteed array: users?.mapusers is initialised as [] so the ? adds noise without safety. Same pattern appears in a few other places.

  • Unnecessary handleUserSelect wrapper: it only calls onUserSelect(user) and can be replaced with onUserSelect directly in the JSX.

  • Hardcoded limit=5 in the fetch URL: should come from a constant or config so it's easy to change.

  • Trailing slash in apiPathUser: double-check the backend route — a trailing slash can cause a redirect or 404 depending on the server config.

  • No keyboard navigation / no click-outside: users expect / to move through suggestions and Escape or a click outside to close the list. Without these the feature feels incomplete.

Copy link
Copy Markdown
Collaborator

@ivannissimrch ivannissimrch left a comment

Choose a reason for hiding this comment

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

I tested with BE running against local Docker.

@ triggers the autocomplete dropdown at caret position. Selecting a user closes the dropdown and inserts text.

Missing name fields in BE response: /user returns user account data, but no name fields (firstName, fullName).
tsx renders user.fullName, which is undefined, so the dropdown shows avatars only, and selecting a user inserts @undefined into the comment.
-Search filter not working: Typing after @ sends the correct query param, but the BE returns all users unfiltered.

Copy link
Copy Markdown
Collaborator

@nadavosa nadavosa left a comment

Choose a reason for hiding this comment

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

Thanks for building this — the overlay technique for syntax highlighting is a solid approach. The author flagged it wasn't tested against a running BE, so I'll treat this as a pre-merge review rather than a final one. Several issues need to be addressed before this can ship.


🔴 Blockers

1. Hard-coded user IDs in production code — CommentDisplay.tsx

const { renderHighlightedText } = useCommentTag(content, () => null, null, [4, 6, 67]);

[4, 6, 67] are clearly test/debug IDs that must not reach production. This also reveals a deeper design issue: to render tagged users as links in existing comments, the IDs need to come from the comment data itself, not hard-coded. The comment content "@Alice" has no embedded ID — the tag-to-ID mapping needs to be stored separately (e.g. in a taggedUserIds field on the TimedText type or derived from a lookup). This is a feature gap that needs a plan before merging.


2. Wrong logic in useLayoutEffectAutocomplete.tsx

const el = textAreaRef?.current;
if (!el && !debouncedUserFilter) return;           // ← AND should be OR
const textBeforeCaret = debouncedUserFilter?.substring(0, el?.selectionStart);  // ← wrong input
const lastAtIndex = textBeforeCaret?.lastIndexOf("@");

Two bugs here:

a) !el && !debouncedUserFilter only returns early when both are falsy. If el is null but debouncedUserFilter has a value, execution continues and crashes on el.selectionStart. Should be !el || !debouncedUserFilter.

b) debouncedUserFilter.substring(0, el.selectionStart) takes a substring of the search term (e.g. "john") using the cursor index of the full textarea. These are completely different strings. The caret position calculation is using the wrong input — it should derive position from the full textarea value and the index of the @ character, not from the debounced search term.


3. CSS typo — styles.ts

user-selects: none;   // ← invalid CSS property

Should be user-select: none; (no trailing s). As written, the overlay is mouse-interactive despite visually blocking the textarea, which may cause click-through issues.


🟡 Non-blocking issues

4. Nested AutocompleteRow inside AutocompleteRowAutocomplete.tsx

<AutocompleteRow key={user.id} onClick={...}>
  <AvatarImg ... />
  <AutocompleteRow>{user.fullName}</AutocompleteRow>  {/* wrong */}
</AutocompleteRow>

The inner AutocompleteRow should be a span or a plain div. Nesting the styled row component inside itself produces unexpected layout (two sets of flex + hover + padding).


5. Trailing slash on apiPathUserconstants.ts

export const apiPathUser = `/${apiPrefix}/user/`;

All other path constants have no trailing slash (apiPathVolunteer, apiPathOpportunity, etc.). This will produce …/user//me if anyone concatenates it with "me". Remove the trailing slash.


6. API always fires when no @ is typed — Autocomplete.tsx

When newCommentText has no @, userFilter returns "". The query guard is enabled: debouncedUserFilter !== null, so "" passes through and fires an API call for all coordinators on every keystroke before any @ is typed.

Change enabled to:

enabled: debouncedUserFilter !== null && debouncedUserFilter.length > 0,

or, even simpler, only render <Autocomplete> when showAutocomplete is true (which is already gated) and don't mount the component at all when hidden, so the query never runs.


7. handleUserSelect is an unnecessary passthrough — Autocomplete.tsx

const handleUserSelect = (userId: number, firstName: string) => {
  handleTagAdd(userId, firstName);
};

This just calls handleTagAdd directly with no extra logic. Remove it and use onClick={() => handleTagAdd(user.id, user.firstName)}.


8. showAutocomplete effect doesn't track cursor movement — useCommentTag.tsx

const cursorPosition = textAreaRef.current?.selectionStart;

selectionStart is read inside a useEffect that only depends on [value]. If the user moves the cursor without changing text (e.g. clicking elsewhere in the textarea), the autocomplete state won't update. Consider also tracking a selectionStart state or listening to onSelect/onClick on the textarea.


9. No keyboard navigation — Autocomplete.tsx

The dropdown is click-only. Standard UX expectation for an autocomplete is to support ArrowDown/ArrowUp to move through results and Enter or Tab to confirm. Consider this a known gap to address in a follow-up if shipping now.


Nits

  • resolvedAvatarUrl in Autocomplete.tsx is a trivial wrapper — inline it as getImageUrl(user.avatarUrl || defaultAvatarVolunteerProfile).
  • NewCommentSection now has a fixed height: 200px which may feel rigid. Consider min-height instead.
  • AddCommentButton has aspect-ratio: 4/1 — this makes the button change width based on its height, which can look unexpected at different viewports. The previous approach of fixed padding is more predictable.
  • textarea-caret is in dependencies but @types/textarea-caret is in devDependencies. That's correct as-is.

@nadavosa
Copy link
Copy Markdown
Collaborator

Hey, cool feature! A few things to fix before this can merge:

Must fix:

  • CommentDisplay has [4, 6, 67] hardcoded — looks like debug IDs, remove them. Also need to think about where real tag IDs come from when displaying existing comments (the comment text alone doesn't carry them)
  • The caret position calculation in Autocomplete is using the search term string (debouncedUserFilter) instead of the full textarea value — so the dropdown will appear in the wrong spot
  • if (!el && !debouncedUserFilter) should be || not &&, otherwise it crashes when el is null
  • CSS typo: user-selectsuser-select

Smaller things:

  • Nested <AutocompleteRow> inside <AutocompleteRow> in the render — the inner one should be a span
  • apiPathUser has a trailing slash, other path constants don't
  • API fires for all coordinators even before the user types @ — add && debouncedUserFilter.length > 0 to enabled

@DarrellRoberts DarrellRoberts self-assigned this May 22, 2026
@DarrellRoberts
Copy link
Copy Markdown
Collaborator Author

thanks @nadavosa and @ivannissimrch for checking!

It should work now with the users

@nadavosa regarding Keyboard navgiation, I haven't implement that yet as it presents another challenge (the Autocomplete is a <div> not a <select> tag/component. If you feel it's essential, I can work on it but if not, I can continue with the next stage of the tagging

@DarrellRoberts
Copy link
Copy Markdown
Collaborator Author

@nadavosa ignore my comment above.

I've now added keyboard navigation but I'll honest I pretty much "vibe-coded" this to save time.

From what I tested though, it works fine and I just needed to tweak it a bit.

Copy link
Copy Markdown
Collaborator

@nadavosa nadavosa left a comment

Choose a reason for hiding this comment

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

Thanks for the updates @DarrellRoberts — several things look much better. Re-checking against the previous blockers:

Fixed ✅

  • Hard-coded [4, 6, 67] user IDs are gone
  • useCommentTag is now a real hook (state, effects, callbacks)
  • Variable shadowing (valuematch) fixed
  • user-select: none CSS typo fixed
  • Keyboard navigation (ArrowUp/Down/Enter/Escape) added

Still needs fixing:

🔴 Blocker: useLayoutEffect caret calculation still wrong — Autocomplete.tsx

const el = textAreaRef?.current;
if (!el && !userFilter) return;                                          // ← still &&, should be ||
const textBeforeCaret = userFilter?.substring(0, el?.selectionStart);   // ← still wrong input

Two issues persist:

a) !el && !userFilter — if el is null but userFilter has a value, this doesn't return early. The second if (!el) return below saves you from a crash, but textBeforeCaret is set to a nonsensical value first. Change to !el || !userFilter.

b) userFilter.substring(0, el.selectionStart)userFilter is the debounced search term (e.g. "joh"), not the full textarea content. Taking a substring of "joh" up to the cursor position of the full textarea makes no sense. The @ character will never be found in textBeforeCaret, so lastAtIndex is always -1, and the dropdown always uses el.selectionStart as the position index instead of the actual @ position. The dropdown appears in the wrong spot when the @ is not at position 0.

Fix: use the full textarea value instead:

const el = textAreaRef?.current;
if (!el || !userFilter) return;
const textBeforeCaret = el.value.substring(0, el.selectionStart);
const lastAtIndex = textBeforeCaret.lastIndexOf("@");

🔴 Blocker: useCommentTag fires a user-list API call for every rendered comment — useCommentTag.tsx

const { data: users } = useGetQuery<ApiUserGet[]>({
  queryKey: ["users"],
  apiPath: apiPathUser,
  params: { sortOrder: SortOrder.NewToOld },
  staleTime: cacheTTL,
  // no `enabled` guard
});

CommentDisplay calls useCommentTag(content) — one call per existing comment on the page. Each call mounts this hook, and since there's no enabled guard, each fires the /user/ query. With 10 comments that's 10 concurrent API calls on page load (deduplicated by React Query's cache, but still runs once per key mount).

Fix: add enabled: false when not in edit mode (i.e. when setNewCommentText is not provided):

const { data: users } = useGetQuery<ApiUserGet[]>({
  ...
  enabled: !!setNewCommentText,  // only fetch when in comment-input mode
});

🟡 Non-blocking: apiPathUser trailing slash — constants.ts

export const apiPathUser = `/${apiPrefix}/user/`;   // ← trailing slash
export const apiPathMe   = `/${apiPrefix}/user/me`; // ← no trailing slash on siblings

Inconsistent with all other path constants. If apiPathMe is ever derived from apiPathUser + "me", this produces …/user//me. Remove the trailing slash.


🟡 Non-blocking: fixed height: 200px on NewCommentSectionstyles.ts

A hard fixed height clips long comments and the shadow scroll approach means users can't tell there's more content. Use min-height: 200px; max-height: 300px; overflow-y: auto and remove the overflow-y: hidden to give it room to breathe.


🟡 Non-blocking: no focus indicator — styles.ts

TextArea has outline: none and border: none, and NewCommentSection has no :focus-within style. Keyboard users get no visual feedback that the field is active. Add:

export const NewCommentSection = styled.div`
  ...
  &:focus-within {
    border-color: var(--color-midnight);
  }
`;

Nits

  • handleUserSelect in Autocomplete.tsx is still a passthrough — use onClick={() => handleTagAdd(user.id, user.firstName)} directly
  • resolvedAvatarUrl helper can be inlined as getImageUrl(user.avatarUrl || defaultAvatarVolunteerProfile)

Copy link
Copy Markdown
Collaborator

@nadavosa nadavosa left a comment

Choose a reason for hiding this comment

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

Two blockers remain (useLayoutEffect caret calc + API call on every rendered comment). Details in the comment above.

@DarrellRoberts
Copy link
Copy Markdown
Collaborator Author

DarrellRoberts commented May 28, 2026

thanks @nadavosa !

  1. For your useLayoutEffect point, adding the (!el || !userFilter) actually breaks the component but I found a better solution as userFilter is never falsy but null, e.g.
 if (!el || userFilter === null) return;

Fair point though on the textBeforeCaret point which I have changed

  1. For API call, that was a good catch. I have added the !!setNewComment dep

  2. I've added the focus-within styling and changed the height to min-height and max-height. However, the overflow-y: hidden is there so that two vertical scrollbars (rather than one) are not shown within the Textbox so I've not changed this property.

So these blockers should now be fixed.

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.

create autocomplete component that is triggered by user tag

4 participants