Custom ID user dropdown#1310
Custom ID user dropdown#1310david-roper wants to merge 8 commits intoDouglasNeuroInformatics:mainfrom
Conversation
WalkthroughModified StartSessionForm component to add custom dropdown autocomplete for subject selection using portal-based rendering, integrated subject data fetching via useSubjectsQuery hook, implemented dynamic dropdown positioning and behavior, and restructured form submission payload to include structured subjectData object. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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: 1
🧹 Nitpick comments (2)
apps/web/src/components/StartSessionForm/StartSessionForm.tsx (2)
147-147: Consider scoping MutationObserver more narrowly.Observing
document.bodywithsubtree: truetriggers on every DOM mutation. If you can obtain a ref to the form container, scoping the observer would reduce overhead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/StartSessionForm/StartSessionForm.tsx` at line 147, The MutationObserver is currently observing document.body which triggers on every DOM mutation; scope it to the form container instead by adding a ref (e.g., formRef) in StartSessionForm, attach that ref to the top-level container element, and call observer.observe(formRef.current, { childList: true, subtree: true }) only when formRef.current exists; also keep the existing cleanup (observer.disconnect()) and null checks so the observer is created/observed and torn down safely.
191-221: Dropdown lacks accessibility attributes and keyboard navigation.The custom dropdown is missing:
role="listbox"on the containerrole="option"on each buttonaria-expandedon the input- Keyboard navigation (arrow keys, Escape to close)
Consider adding these for screen reader and keyboard-only users.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/StartSessionForm/StartSessionForm.tsx` around lines 191 - 221, The dropdown lacks ARIA roles and keyboard nav: add role="listbox" to the dropdown container (the element using dropdownRef and dropdownPos) and role="option" plus aria-selected to each mapped item (the buttons rendered from filteredSubjects where you currently call removeSubjectIdScope and handleSelectOption), set aria-expanded on the input that controls opening, and implement keyboard handlers on the input and dropdown to support ArrowUp/ArrowDown to move a focused/active index, Enter to select (calling handleSelectOption with the focused displayId), and Escape to close the dropdown; manage a focusedIndex state and ensure focus moves to the corresponding option (or use aria-activedescendant) and update aria-selected when index changes so screen readers and keyboard users can navigate properly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/components/StartSessionForm/StartSessionForm.tsx`:
- Around line 125-128: Remove the non-null assertion on document.querySelector
in the MutationObserver callback and guard against a null result: change the
retrieval of initialInput in the observer (the MutationObserver callback that
uses initialInput, input, and inputRef) to allow null (e.g., const initialInput
= document.querySelector('input[name="subjectId"]') as HTMLInputElement | null)
and add an explicit check (if (!initialInput) return or if (initialInput &&
initialInput !== inputRef.current) { ... }) before casting/using it so the code
won’t rely on a thrown non-null assertion when the input is absent (e.g., when
subjectIdentificationMethod === PERSONAL_INFO).
---
Nitpick comments:
In `@apps/web/src/components/StartSessionForm/StartSessionForm.tsx`:
- Line 147: The MutationObserver is currently observing document.body which
triggers on every DOM mutation; scope it to the form container instead by adding
a ref (e.g., formRef) in StartSessionForm, attach that ref to the top-level
container element, and call observer.observe(formRef.current, { childList: true,
subtree: true }) only when formRef.current exists; also keep the existing
cleanup (observer.disconnect()) and null checks so the observer is
created/observed and torn down safely.
- Around line 191-221: The dropdown lacks ARIA roles and keyboard nav: add
role="listbox" to the dropdown container (the element using dropdownRef and
dropdownPos) and role="option" plus aria-selected to each mapped item (the
buttons rendered from filteredSubjects where you currently call
removeSubjectIdScope and handleSelectOption), set aria-expanded on the input
that controls opening, and implement keyboard handlers on the input and dropdown
to support ArrowUp/ArrowDown to move a focused/active index, Enter to select
(calling handleSelectOption with the focused displayId), and Escape to close the
dropdown; manage a focusedIndex state and ensure focus moves to the
corresponding option (or use aria-activedescendant) and update aria-selected
when index changes so screen readers and keyboard users can navigate properly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: caeed869-8f64-4ae8-836a-8bdb916ca21f
📒 Files selected for processing (1)
apps/web/src/components/StartSessionForm/StartSessionForm.tsx
|
This implementation is terrible and a hack that will break on any change to libui internal implementation details. The reason it did it like this is because it is not possible in our form. To do this properly, I need to make some changes in libui. After that, it will need to be rewritten from scratch. |
Testing out antigravity implementation of the custom Id dropdown feature, closing issue #1216
Provides a dropdown search of available custom ids, the subjectId form input acts as the search bar.
Summary by CodeRabbit