Skip to content

Custom ID user dropdown#1310

Closed
david-roper wants to merge 8 commits intoDouglasNeuroInformatics:mainfrom
david-roper:custom-user-dropdown
Closed

Custom ID user dropdown#1310
david-roper wants to merge 8 commits intoDouglasNeuroInformatics:mainfrom
david-roper:custom-user-dropdown

Conversation

@david-roper
Copy link
Collaborator

@david-roper david-roper commented Mar 9, 2026

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

  • New Features
    • Added an interactive dropdown autocomplete for subject selection with real-time filtering.
    • Improved form submission to capture more detailed subject information.
    • Enhanced input handling with responsive dropdown positioning and keyboard navigation support.

@david-roper david-roper self-assigned this Mar 9, 2026
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 9, 2026

Walkthrough

Modified 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

Cohort / File(s) Summary
Dropdown Autocomplete for Subject Selection
apps/web/src/components/StartSessionForm/StartSessionForm.tsx
Added custom dropdown autocomplete UI with portal rendering, state management for dropdown positioning and visibility, MutationObserver-based event listener attachment to input field, debounced filtering logic via requestAnimationFrame, and dynamic position recalculation on scroll/resize. Integrated useSubjectsQuery hook for subject data fetching. Restructured form submission payload to separate subjectData (id, firstName, lastName, dateOfBirth, sex) from other form fields. Added StartSessionFormData export.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • OpenDataCapture#1164 — Modifies StartSessionForm's handling of subject identity fields (dateOfBirth, sex) which interact with the new subjectData structure in this PR.
  • OpenDataCapture#1069 — Updates StartSessionPage to handle new form submission shape (type and initialValues), directly corresponding to the restructured onSubmit payload in this PR.

Suggested reviewers

  • joshunrau
🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: implementing a custom dropdown for user ID selection in the StartSessionForm component.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@david-roper david-roper marked this pull request as ready for review March 10, 2026 14:49
@david-roper david-roper requested a review from joshunrau as a code owner March 10, 2026 14:49
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (2)
apps/web/src/components/StartSessionForm/StartSessionForm.tsx (2)

147-147: Consider scoping MutationObserver more narrowly.

Observing document.body with subtree: true triggers 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 container
  • role="option" on each button
  • aria-expanded on 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

📥 Commits

Reviewing files that changed from the base of the PR and between 4ff67ce and 02b80cc.

📒 Files selected for processing (1)
  • apps/web/src/components/StartSessionForm/StartSessionForm.tsx

@joshunrau
Copy link
Collaborator

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.

@joshunrau joshunrau closed this Mar 10, 2026
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.

2 participants