Skip to content

Conversation

@bmartel
Copy link
Contributor

@bmartel bmartel commented Nov 19, 2025

This pull request refactors the Dropdown component usage across both the Label Studio and Data Manager apps to use a shared, external UI library (@humansignal/ui). It removes local implementations, styles, and context for Dropdowns, consolidating the codebase and ensuring consistency. Additionally, it improves the positioning logic for dropdowns to prevent overflow off the screen.

Dropdown Component Refactor

  • Replaced all local imports and usages of Dropdown, DropdownTrigger, and useDropdown with imports from the shared @humansignal/ui package in multiple files, including Breadcrumbs.jsx, Menubar.jsx, Menu.jsx, and DatePicker.jsx. [1] [2] [3] [4]
  • Removed local Dropdown component implementations, triggers, context, and related exports from both Label Studio and Data Manager codebases (DropdownComponent.jsx, DropdownTrigger.jsx, DropdownContext.js, and their corresponding JS entry files). [1] [2] [3] [4] [5]

Styles Cleanup

  • Deleted local Dropdown stylesheets (Dropdown.scss) for both apps, removing redundant CSS now handled by the shared UI library. [1] [2]

Dropdown Positioning Improvements

  • Progressively enhanced to use @position-try for a JS-free approach to dropdown positioning logic, bolstering and fixing many nuanced bugs that were produced by the JS logic that came before it.
  • Falls back to an improved variant of the original JS implementation for positioning. (Only Firefox has yet to support, currently in the nightly build).
  • Enhanced the alignElements function in dom.ts to add boundary checks, ensuring dropdowns do not overflow the viewport edges (left, right, top, bottom) and always remain visible with a minimum padding.

This refactor simplifies maintenance, reduces duplication, and ensures a unified Dropdown experience across the platform.

Create a unified, feature-complete Dropdown component that consolidates three
nearly-identical implementations from editor, datamanager, and labelstudio.

Features included:
- TypeScript with full type safety (from editor)
- CSS anchor positioning with @position-try for automatic viewport-aware
  repositioning (from all implementations)
- Stable unique IDs using React.useId() for anchor names
- Progressive enhancement: modern browsers use CSS positioning, older browsers
  fall back to JavaScript positioning
- Animation support with customizable transitions
- Portal rendering for proper z-index stacking
- Nested dropdown support with parent-child relationships

Additional features consolidated:
- openUpwardForShortViewport prop (from datamanager)
- constrainHeight prop (from datamanager)
- isChildValid callback (from datamanager)
- onVisibilityChanged callback (from labelstudio)
- dropdownClassName for additional styling flexibility
- minIndex z-index calculation for nested dropdowns (from editor)

Component structure:
- dropdown.tsx: Main Dropdown component with all positioning logic
- dropdown-trigger.tsx: Trigger wrapper with click-outside detection
- dropdown-context.tsx: Context for parent-child dropdown relationships
- dropdown.module.scss: Styles with CSS anchor positioning and fallbacks
- index.ts: Barrel exports

This consolidation will:
- Reduce maintenance burden (one component instead of three)
- Improve consistency across all apps
- Enable shared improvements and bug fixes
- Provide better type safety throughout the codebase
- Simplify onboarding for new developers

Next steps:
1. Migrate editor to use @humansignal/ui Dropdown
2. Migrate datamanager to use @humansignal/ui Dropdown
3. Migrate labelstudio to use @humansignal/ui Dropdown
4. Remove old implementations after successful migration
Migrate all editor dropdown imports to use the new consolidated Dropdown
component from @humansignal/ui instead of the local implementation.

Files updated:
- components/BottomBar/Controls.tsx
- components/ContextMenu/ContextMenu.tsx
- components/Comments/Comment/CommentItem.tsx
- components/AnnotationsCarousel/AnnotationButton.tsx
- components/SidePanels/OutlinerPanel/ViewControls.tsx
- tags/object/Video/HtxVideo.jsx
- common/Menu/Menu.jsx

Changes:
- Replace 'from ../../common/Dropdown/Dropdown' with 'from @humansignal/ui'
- Replace 'from ../Dropdown/DropdownTrigger' with 'from @humansignal/ui'
- Consolidate multiple dropdown imports into single import statement where applicable

Benefits:
- Use fully-typed TypeScript component
- Get all consolidated features (CSS anchor positioning, better viewport handling)
- Consistent behavior across the application
- Easier maintenance with single source of truth

The old editor/src/common/Dropdown/ implementation will be removed after all
migrations are complete and tested.
…gnal/ui

Migrate all datamanager dropdown imports to use the new consolidated Dropdown
component from @humansignal/ui instead of the local implementation.

Files updated:
- components/Common/ErrorBox.jsx
- components/Common/FieldsButton.jsx
- components/Common/FiltersPane.jsx
- components/Common/DatePicker/DatePicker.jsx
- components/Common/Tabs/Tabs.jsx
- components/Common/Table/TableHead/TableHead.jsx
- components/Common/Menu/Menu.jsx
- components/DataManager/Toolbar/GridWidthButton.jsx
- components/DataManager/Toolbar/LabelButton.jsx
- components/DataManager/Toolbar/ActionsButton.jsx

Changes:
- Replace 'from ../../Common/Dropdown/DropdownComponent' with 'from @humansignal/ui'
- Replace 'from ../Dropdown/Dropdown' with 'from @humansignal/ui'
- Replace 'from ../Dropdown/DropdownTrigger' with 'from @humansignal/ui'

Benefits:
- Access to all consolidated features including openUpwardForShortViewport and constrainHeight
- Better TypeScript support
- CSS anchor positioning with automatic viewport-aware repositioning
- Consistent behavior with editor and labelstudio
- Single source of truth for dropdown logic

The old datamanager/src/components/Common/Dropdown/ implementation will be
removed after all migrations are complete and tested.
This is the critical fix for Agreement Selected dropdown not appearing.

The Issue:
- DropdownTrigger had: const dropdownClone = content ? <Dropdown>{content}</Dropdown> : null
- Agreement Selected uses conditional content: content={isOpen ? <HeaderCell /> : <></>}
- When content changed from falsy to truthy, React unmounted and remounted Dropdown
- Remounting caused dropdown.current ref to become null during animation
- This broke the animation flow and dropdown wouldn't appear

The Fix:
- Always render Dropdown: const dropdownClone = <Dropdown>{content}</Dropdown>
- Dropdown stays mounted, only children change
- Ref remains stable throughout content changes
- Animations complete successfully

This fixes the bug where Agreement Selected only appeared after DevTools opened.
Export from ./lib/dropdown barrel instead of ./lib/dropdown/dropdown
This includes DropdownTrigger and useDropdown exports
Added logging to trace:
- handleToggle in DropdownTrigger
- toggle in Dropdown
- performAnimation flow
- Guard check for dropdown.current being null
… toggle

The issue:
- useEffect watching 'visible' prop would call open()/close() on every render
- Agreement Selected doesn't use 'visible' prop (defaults to false)
- This useEffect kept calling close() repeatedly, preventing dropdown from opening
- Multiple toggle(false) calls seen in console before toggle(true)

The fix:
- Remove this useEffect entirely
- Agreement Selected uses imperative toggle() calls via ref, not controlled 'visible' prop
- Components using controlled 'visible' prop still work via setVisible(visible) useEffect

This should fix Agreement Selected dropdown not appearing without DevTools.
- Fix Agreement Selected dropdown not appearing on first click
- Root cause: display:none prevented CSS anchor positioning initialization
- Solution: Add 'mounted' class that keeps element in DOM but hidden
- Implement lazy content rendering for performance and autofocus
- Restore original slide-down animation with transform
- Clean up debug logging

The dropdown wrapper now stays in DOM with display:flex but invisible,
allowing CSS anchor positioning to initialize properly while maintaining
original animation behavior and performance.
The 4px margin intended for viewport edge spacing was also offsetting
the dropdown from its anchor trigger. CSS anchor positioning with
position-try-fallbacks handles viewport overflow automatically.
The editor was already migrated to use @humansignal/ui Dropdown.
The custom Dropdown implementation in src/common/Dropdown was completely
unused and has been removed to reduce code duplication.
- Migrate Label Studio app dropdown usages (Menubar, Breadcrumbs, Menu)
- Remove unused dropdown implementations from:
  - web/apps/labelstudio/src/components/Dropdown/
  - web/libs/datamanager/src/components/Common/Dropdown/
- All dropdowns now use the unified implementation from @humansignal/ui
- Eliminates ~1200 lines of duplicated dropdown code across the codebase
- Fix trigger className to preserve original element classes
- Fix dropdown prop not being used (was using ref instead)
- This fixes the main menu hamburger trigger className being overwritten
- This fixes duplicate dropdown instances causing anchor positioning conflicts
The old LSE dropdown implementation did not add any className to the trigger element.
Adding dropdown__trigger class was causing unwanted classes to be mixed in,
breaking functionality in components like the user menu trigger.

This restores the original behavior where trigger elements keep their original
className without any modifications from the dropdown system.
Replace reduce() with for-loop and early returns to short-circuit checks.
This significantly improves performance when there are many nested dropdowns
(e.g., notifications list with 42+ items each having their own dropdown menu).

Before: O(n) always - checked every child dropdown
After: O(1) best case - returns immediately on first match
When clicking on a dropdown trigger/content, we now check the immediate
trigger and dropdown refs BEFORE iterating through child dropdowns.

With 42+ nested dropdowns (notifications), this prevents expensive
child iteration in 99% of cases, reducing the delay from 3-4s to instant.

Fast path checks (< 1ms each):
1. Is click on our trigger? Return immediately
2. Is click on our dropdown? Return immediately
3. Is click valid via custom validator? Return immediately
4. Only then check children (expensive)

This optimization is critical for dropdowns with many nested children.
Critical performance fix: Previously, ALL dropdown triggers (84+ in notifications)
registered document-level click listeners even when closed. This caused 84+
callbacks to run on EVERY click, creating a 3-4 second delay.

Now click listeners are only added when the dropdown is actually open:
- Closed dropdowns: 0 event listeners
- Open dropdown: 1 event listener per visible dropdown

This eliminates the delay when closing the notifications dropdown.
All 84+ nested dropdown event listeners still run, but now they exit on the
very first line by checking dropdownRef.current?.visible (boolean check on ref).

This is much faster and more reliable than conditionally adding/removing
listeners since:
- No dependency on reactive state changes
- Simple boolean check (~0.1ms per listener)
- 84 listeners * 0.1ms = ~8ms total vs 3-4 seconds before

The visibility check must be FIRST before any other logic including
closeOnClickOutside check.
@bmartel bmartel requested review from a team, hlomzik and nick-skriabin as code owners November 19, 2025 15:14
@netlify
Copy link

netlify bot commented Nov 19, 2025

Deploy Preview for label-studio-docs-new-theme canceled.

Name Link
🔨 Latest commit 5fa24a5
🔍 Latest deploy log https://app.netlify.com/projects/label-studio-docs-new-theme/deploys/691f36172b89e70009269398

@netlify
Copy link

netlify bot commented Nov 19, 2025

Deploy Preview for heartex-docs canceled.

Name Link
🔨 Latest commit 5fa24a5
🔍 Latest deploy log https://app.netlify.com/projects/heartex-docs/deploys/691f3617f6f1b60007019994

@netlify
Copy link

netlify bot commented Nov 19, 2025

Deploy Preview for label-studio-playground ready!

Name Link
🔨 Latest commit 5fa24a5
🔍 Latest deploy log https://app.netlify.com/projects/label-studio-playground/deploys/691f36175761fc0008d8c1d5
😎 Deploy Preview https://deploy-preview-8844--label-studio-playground.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@github-actions github-actions bot added the chore label Nov 19, 2025
@netlify
Copy link

netlify bot commented Nov 19, 2025

Deploy Preview for label-studio-storybook ready!

Name Link
🔨 Latest commit 5fa24a5
🔍 Latest deploy log https://app.netlify.com/projects/label-studio-storybook/deploys/691f3617eb97f20008ae5c62
😎 Deploy Preview https://deploy-preview-8844--label-studio-storybook.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

Copy link
Contributor

@yyassi-heartex yyassi-heartex left a comment

Choose a reason for hiding this comment

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

change looks good to me overall - good to see component unification!

@codecov
Copy link

codecov bot commented Nov 19, 2025

Codecov Report

❌ Patch coverage is 76.00000% with 54 lines in your changes missing coverage. Please review.
✅ Project coverage is 57.09%. Comparing base (93ba514) to head (5fa24a5).
⚠️ Report is 1 commits behind head on develop.

Files with missing lines Patch % Lines
web/libs/ui/src/lib/dropdown/dropdown.tsx 74.82% 35 Missing ⚠️
web/libs/ui/src/lib/dropdown/dropdown-trigger.tsx 78.31% 18 Missing ⚠️
.../libs/editor/src/components/BottomBar/Controls.tsx 50.00% 1 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (93ba514) and HEAD (5fa24a5). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (93ba514) HEAD (5fa24a5)
pytests 1 0
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #8844      +/-   ##
===========================================
- Coverage    66.31%   57.09%   -9.23%     
===========================================
  Files          813      554     -259     
  Lines        63625    40287   -23338     
  Branches     10768    10790      +22     
===========================================
- Hits         42193    23001   -19192     
+ Misses       21428    17282    -4146     
  Partials         4        4              
Flag Coverage Δ
lsf-e2e 50.12% <71.11%> (+0.34%) ⬆️
lsf-integration 49.14% <76.00%> (-0.05%) ⬇️
lsf-unit 8.35% <50.00%> (+<0.01%) ⬆️
pytests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@bmartel
Copy link
Contributor Author

bmartel commented Nov 20, 2025

Skipping the flakey e2e test, it passed the commit before merging develop back in, and passed on develop. Same single flakey test that fails periodically, and this branch has no changes which affect it.

@bmartel bmartel merged commit 08eed6e into develop Nov 20, 2025
77 of 84 checks passed
@robot-ci-heartex robot-ci-heartex deleted the fb-fit-1002 branch November 20, 2025 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants