Skip to content

fix: preserve user-provided id on Autocomplete input - #4682

Merged
pomfrida merged 4 commits into
mainfrom
fix/autocomplete-preserve-id
Apr 1, 2026
Merged

fix: preserve user-provided id on Autocomplete input#4682
pomfrida merged 4 commits into
mainfrom
fix/autocomplete-preserve-id

Conversation

@pomfrida

Copy link
Copy Markdown
Collaborator

Summary

  • Autocomplete was overwriting user-provided id prop with downshift's auto-generated ID
  • Pass user id as inputId to downshift's useCombobox, ensuring it's used consistently for the input element, label htmlFor, and all related aria attributes
  • Adds two tests verifying id preservation and label-input association

Closes #4588

Test plan

  • Existing 32 Autocomplete tests pass
  • New test: user-provided id is preserved on the input element
  • New test: label for attribute matches custom id

🤖 Generated with Claude Code

Pass user-provided id as inputId to downshift's useCombobox so it is
used consistently for the input element, label htmlFor, and all
related aria attributes. Closes #4588
…lass'

Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
@pomfrida

Copy link
Copy Markdown
Collaborator Author

@claude review

@claude

claude Bot commented Mar 23, 2026

Copy link
Copy Markdown

Claude finished @pomfrida's task in 1m 4s —— View job


Code Review

  • Gather context
  • Read changed files
  • Provide review feedback

Summary

The fix is correct and minimal. One test improvement needed (also flagged by the code quality bot).


useAutocomplete.ts — ✅ Looks good

Line 299:

...(id !== undefined && { inputId: id }),

This is the right approach. Passing inputId to downshift's useCombobox ensures:

  • The input element gets id="<your-id>"
  • The label's for attribute is automatically set to match
  • All aria relationships managed by downshift remain consistent

The conditional spread correctly avoids overriding downshift's auto-generated ID when no id prop is passed. Clean, minimal fix.


Autocomplete.test.tsx⚠️ Minor issue

The second new test has a weak assertion (also flagged by the code quality bot):

// line 151
const input = screen.getAllByLabelText(labelText)[0]
// ...
expect(input).toBeInTheDocument() // ← this adds no value

getAllByLabelText already throws if the element isn't found, so toBeInTheDocument() is redundant. The input variable is effectively unused in a meaningful way.

Suggested fix — replace with a stronger assertion:

it('Maintains label-input association when custom id is provided', () => {
  const { container } = render(
    <Autocomplete id="my-custom-id" label={labelText} options={items} />,
  )
  const label = container.querySelector('label')
  expect(label).toHaveAttribute('for', 'my-custom-id')
  // Verify the label actually points to the input element
  const input = screen.getAllByLabelText(labelText)[0]
  expect(input.nodeName).toBe('INPUT')
})

Or simply drop the input variable entirely if the test is purely about the label's for attribute.

Fix this →


Overall

The core fix is solid. Just the minor unused variable / weak assertion in the second test to clean up before merge.

@pomfrida pomfrida self-assigned this Mar 23, 2026
Remove unused variable and use getByRole instead of container
querySelector to satisfy testing-library lint rules.
Re-apply inputId fix in useAutocomplete and test cases that were
lost during rebase. Restore snapshot from main to match CI environment.

@millus millus 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.

LGTM 🚀

@pomfrida
pomfrida merged commit 85a4301 into main Apr 1, 2026
12 checks passed
@pomfrida
pomfrida deleted the fix/autocomplete-preserve-id branch April 1, 2026 07:13
@github-actions github-actions Bot mentioned this pull request Apr 1, 2026
@github-actions github-actions Bot mentioned this pull request May 20, 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.

Autocomplete-component in eds-core-react overwrites the id-attribute

2 participants