Skip to content

ui: invalid state support#26803

Open
harsh-vador wants to merge 2 commits intomainfrom
autocomplete-invalid-state
Open

ui: invalid state support#26803
harsh-vador wants to merge 2 commits intomainfrom
autocomplete-invalid-state

Conversation

@harsh-vador
Copy link
Contributor

Describe your changes:

Fixes

I worked on ... because ...

Type of change:

  • Bug fix
  • Improvement
  • New feature
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation

Checklist:

  • I have read the CONTRIBUTING document.
  • My PR title is Fixes <issue-number>: <short explanation>
  • I have commented on my code, particularly in hard-to-understand areas.
  • For JSON Schema changes: I updated the migration scripts or explained why it is not needed.

@harsh-vador harsh-vador self-assigned this Mar 26, 2026
@harsh-vador harsh-vador added the safe to test Add this label to run secure Github workflows on PRs label Mar 26, 2026
@github-actions
Copy link
Contributor

github-actions bot commented Mar 26, 2026

❌ Lint Check Failed — ESLint + Prettier (core-components)

The following files have style issues that need to be fixed:
src/components/base/autocomplete/autocomplete.tsx

Fix locally (fast — changed files only):

cd openmetadata-ui-core-components/src/main/resources/ui
yarn ui-checkstyle:changed

Or to fix all files: yarn lint:fix && yarn pretty

@github-actions
Copy link
Contributor

🟡 Playwright Results — all passed (16 flaky)

✅ 3400 passed · ❌ 0 failed · 🟡 16 flaky · ⏭️ 216 skipped

Shard Passed Failed Flaky Skipped
🟡 Shard 1 453 0 2 2
🟡 Shard 2 601 0 3 32
🟡 Shard 3 605 0 4 27
🟡 Shard 4 601 0 2 47
✅ Shard 5 587 0 0 67
🟡 Shard 6 553 0 5 41
🟡 16 flaky test(s) (passed on retry)
  • Features/CustomizeDetailPage.spec.ts › Glossary Term - customization should work (shard 1, 1 retry)
  • Pages/UserCreationWithPersona.spec.ts › Create user with persona and verify on profile (shard 1, 1 retry)
  • Features/BulkEditEntity.spec.ts › Glossary (shard 2, 1 retry)
  • Features/BulkImport.spec.ts › Database service (shard 2, 1 retry)
  • Features/BulkImport.spec.ts › Keyboard Delete selection (shard 2, 1 retry)
  • Features/Permissions/GlossaryPermissions.spec.ts › Team-based permissions work correctly (shard 3, 1 retry)
  • Features/Table.spec.ts › Tags term should be consistent for search (shard 3, 1 retry)
  • Features/TableSearch.spec.ts › Services page should have search functionality (shard 3, 1 retry)
  • Flow/ExploreDiscovery.spec.ts › Should display deleted assets when showDeleted is checked and deleted is not present in queryFilter (shard 3, 1 retry)
  • Pages/Customproperties-part2.spec.ts › entityReferenceList shows item count, scrollable list, no expand toggle (shard 4, 1 retry)
  • Pages/Entity.spec.ts › Set & Update table-cp, hyperlink-cp, string, integer, markdown, number, duration, email, enum, sqlQuery, timestamp, entityReference, entityReferenceList, timeInterval, time-cp, date-cp, dateTime-cp Custom Property (shard 4, 1 retry)
  • Pages/Glossary.spec.ts › Create glossary, change language to Dutch, and delete glossary (shard 6, 1 retry)
  • Pages/HyperlinkCustomProperty.spec.ts › should show No Data placeholder when hyperlink has no value (shard 6, 1 retry)
  • Pages/Users.spec.ts › Permissions for table details page for Data Consumer (shard 6, 1 retry)
  • Pages/Users.spec.ts › Check permissions for Data Steward (shard 6, 1 retry)
  • VersionPages/EntityVersionPages.spec.ts › Directory (shard 6, 1 retry)

📦 Download artifacts

How to debug locally
# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip    # view trace

inputValue={filterText}
items={visibleItems}
menuTrigger="focus"
menuTrigger="input"
Copy link

Choose a reason for hiding this comment

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

⚠️ Bug: menuTrigger change from "focus" to "input" is a behavioral regression

Changing menuTrigger from "focus" to "input" (line 452) means the dropdown no longer opens when the user clicks/tabs into the autocomplete — it now only opens when the user starts typing. This is a UX regression for all existing Autocomplete consumers (12+ story usages, plus production code), not just the new single-select mode.

The added ArrowDown handler partially compensates by letting users manually open the dropdown, but this is far less discoverable than the previous focus-to-open behavior.

If the intent was to prevent the dropdown from reopening in single-select mode after a selection is made, consider conditioning this on the multiple prop instead of changing it globally.

Suggested fix:

menuTrigger={isAtMax ? 'manual' : 'focus'}

Was this helpful? React with 👍 / 👎 | Reply gitar fix to apply this suggestion

)}>
<AriaInput
className="tw:w-full tw:flex-[1_0_0] tw:appearance-none tw:bg-transparent tw:text-md tw:text-ellipsis tw:text-primary tw:caret-alpha-black/90 tw:outline-none tw:placeholder:text-placeholder tw:focus:outline-hidden tw:disabled:cursor-not-allowed tw:disabled:text-disabled tw:disabled:placeholder:text-disabled"
className="tw:w-full tw:flex-[1_0_0] tw:appearance-none tw:bg-transparent tw:text-sm tw:text-ellipsis tw:text-primary tw:caret-alpha-black/90 tw:outline-none tw:placeholder:text-placeholder tw:focus:outline-hidden tw:disabled:cursor-not-allowed tw:disabled:text-disabled tw:disabled:placeholder:text-disabled"
Copy link

Choose a reason for hiding this comment

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

💡 Bug: Input text size changed from text-md to text-sm for all instances

Line 257 changes the input class from tw:text-md to tw:text-sm, which reduces the font size of the autocomplete input for all consumers — not just the new single-select or invalid-state functionality. This appears to be an unintentional side-effect unrelated to the PR's purpose (invalid state support). If intentional, it should be called out in the PR description as a visual change.

Was this helpful? React with 👍 / 👎 | Reply gitar fix to apply this suggestion

@gitar-bot
Copy link

gitar-bot bot commented Mar 26, 2026

Code Review ⚠️ Changes requested 0 resolved / 2 findings

Adds error handling and invalid state support to the autocomplete component but introduces a behavioral regression by changing menuTrigger from "focus" to "input", preventing dropdown opening on click/tab, and reduces input font size from text-md to text-sm across all instances.

⚠️ Bug: menuTrigger change from "focus" to "input" is a behavioral regression

📄 openmetadata-ui-core-components/src/main/resources/ui/src/components/base/autocomplete/autocomplete.tsx:452

Changing menuTrigger from "focus" to "input" (line 452) means the dropdown no longer opens when the user clicks/tabs into the autocomplete — it now only opens when the user starts typing. This is a UX regression for all existing Autocomplete consumers (12+ story usages, plus production code), not just the new single-select mode.

The added ArrowDown handler partially compensates by letting users manually open the dropdown, but this is far less discoverable than the previous focus-to-open behavior.

If the intent was to prevent the dropdown from reopening in single-select mode after a selection is made, consider conditioning this on the multiple prop instead of changing it globally.

Suggested fix
menuTrigger={isAtMax ? 'manual' : 'focus'}
💡 Bug: Input text size changed from text-md to text-sm for all instances

📄 openmetadata-ui-core-components/src/main/resources/ui/src/components/base/autocomplete/autocomplete.tsx:257

Line 257 changes the input class from tw:text-md to tw:text-sm, which reduces the font size of the autocomplete input for all consumers — not just the new single-select or invalid-state functionality. This appears to be an unintentional side-effect unrelated to the PR's purpose (invalid state support). If intentional, it should be called out in the PR description as a visual change.

🤖 Prompt for agents
Code Review: Adds error handling and invalid state support to the autocomplete component but introduces a behavioral regression by changing menuTrigger from "focus" to "input", preventing dropdown opening on click/tab, and reduces input font size from text-md to text-sm across all instances.

1. ⚠️ Bug: menuTrigger change from "focus" to "input" is a behavioral regression
   Files: openmetadata-ui-core-components/src/main/resources/ui/src/components/base/autocomplete/autocomplete.tsx:452

   Changing `menuTrigger` from `"focus"` to `"input"` (line 452) means the dropdown no longer opens when the user clicks/tabs into the autocomplete — it now only opens when the user starts typing. This is a UX regression for all existing Autocomplete consumers (12+ story usages, plus production code), not just the new single-select mode.
   
   The added `ArrowDown` handler partially compensates by letting users manually open the dropdown, but this is far less discoverable than the previous focus-to-open behavior.
   
   If the intent was to prevent the dropdown from reopening in single-select mode after a selection is made, consider conditioning this on the `multiple` prop instead of changing it globally.

   Suggested fix:
   menuTrigger={isAtMax ? 'manual' : 'focus'}

2. 💡 Bug: Input text size changed from text-md to text-sm for all instances
   Files: openmetadata-ui-core-components/src/main/resources/ui/src/components/base/autocomplete/autocomplete.tsx:257

   Line 257 changes the input class from `tw:text-md` to `tw:text-sm`, which reduces the font size of the autocomplete input for all consumers — not just the new single-select or invalid-state functionality. This appears to be an unintentional side-effect unrelated to the PR's purpose (invalid state support). If intentional, it should be called out in the PR description as a visual change.

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change:

Auto-apply Compact
gitar auto-apply:on         
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

safe to test Add this label to run secure Github workflows on PRs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant