Merged
Conversation
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughReplaces a global fade-edge CSS utility with a hook and two new client components (useOverflowFade, OverflowFadeContainer, OverflowFadeText) and updates several UI components to use them for overflow detection and conditional fade effects. Changes
Sequence Diagram(s)sequenceDiagram
participant Component as "Component\n(TabBar / TabItem / Sidebar)"
participant Container as "OverflowFadeContainer"
participant Hook as "useOverflowFade"
participant Env as "ResizeObserver / scroll / window"
Component->>Container: render with props (fadeEdges, observeChildren, onOverflowChange)
Container->>Hook: attach ref, observe parent/children based on props
Env-->>Hook: emit resize / mutation / scroll events
Hook-->>Container: update measurements (hasOverflowX/Y, canScroll*)
Container-->>Component: call onOverflowChange(state)
Component->>Component: update UI (show/hide fades, adjust layout)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Greptile SummaryThis PR promotes the Confidence Score: 5/5Safe to merge — all findings are P2 style/cleanup suggestions with no correctness or data-integrity impact The refactoring correctly extracts shared utilities, the observer teardown is complete, the equality guard prevents cascading re-renders, and existing consumers are migrated without regressions. The three P2 comments (unconditional DOM read per render, redundant re-export files, un-memoized callback footgun) are all non-blocking improvements. packages/ui/src/hooks/use-overflow-fade.ts and packages/ui/src/components/overflow-fade/overflow-fade-container.tsx warrant a quick look for the cleanup items noted above
|
| Filename | Overview |
|---|---|
| packages/ui/src/hooks/use-overflow-fade.ts | New hook measuring scroll overflow via ResizeObserver, MutationObserver, and scroll events; minor concern with unconditional layout effect reading DOM on every render |
| packages/ui/src/components/overflow-fade/OverflowFadeContainer/OverflowFadeContainer.tsx | New forwardRef component applying conditional CSS fade classes; onOverflowChange can be called spuriously if consumer doesn't memoize the callback |
| packages/ui/src/components/overflow-fade/OverflowFadeText/OverflowFadeText.tsx | New span-based component applying right-edge fade when text overflows; observes parent for resize changes — clean implementation |
| packages/ui/src/components/overflow-fade/overflow-fade-container.tsx | Flat re-export file that is never reached through package.json exports — redundant alongside OverflowFadeContainer/index.ts |
| packages/ui/package.json | Added three new export paths for overflow-fade-container, overflow-fade-text, and overflow-fade.css — correctly wired to subdirectory index files |
| packages/panes/src/react/components/Workspace/components/TabBar/TabBar.tsx | Replaces manual ResizeObserver + scroll listener with OverflowFadeContainer; removes scrollContainerRef and inlines overflow detection via onOverflowChange callback (correctly memoized) |
| packages/panes/src/react/components/Workspace/components/TabBar/components/TabItem/TabItem.tsx | Replaces static truncate span with OverflowFadeText for conditional right-fade on tab labels; min-w-0 is now provided by OverflowFadeText's base classes |
| apps/desktop/src/renderer/globals.css | Removes the .fade-edge-r definition and the fade-edge.css import now that those utilities live in packages/ui |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[useOverflowFade hook] -->|ref + state| B[OverflowFadeContainer]
A -->|ref + hasOverflowX| C[OverflowFadeText]
B -->|onOverflowChange callback| D[TabBar\nhasHorizontalOverflow state]
B -->|observeChildren=true| E[DashboardSidebarPortGroup\nport badge row]
C -->|conditional fade-edge-r| F[TabItem\ntab label span]
subgraph useOverflowFade internals
G[ResizeObserver\nnode + parent/children] --> H[updateOverflow]
I[MutationObserver\nchildList changes] --> H
J[scroll listener] --> H
K[layout effect\nevery render] --> H
H --> L[setState with equality guard]
end
Prompt To Fix All With AI
This is a comment left during a code review.
Path: packages/ui/src/hooks/use-overflow-fade.ts
Line: 84-86
Comment:
**Unconditional layout effect re-reads DOM on every render**
The `useIsomorphicLayoutEffect(() => { updateOverflow(); })` on line 84 has no dependency array, so it fires after every single render of any component using this hook. Each call reads `scrollWidth`, `clientWidth`, `scrollLeft`, etc. from the DOM — which forces a layout/reflow. For a component like `TabBar` that can re-render frequently (e.g., during drag-and-drop or tab hover), this adds an unnecessary DOM-read per render. The state equality check prevents extra re-renders, but it does not avoid the DOM read itself. Consider gating this on an `enabled` flag or relying solely on the ResizeObserver + scroll listener set up in the second effect.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: packages/ui/src/components/overflow-fade/overflow-fade-container.tsx
Line: 1
Comment:
**Redundant flat re-export files are dead code**
`overflow-fade-container.tsx` and `overflow-fade-text.tsx` at the `overflow-fade/` root both re-export from the respective subdirectory, but the `package.json` exports map directly to the subdirectory `index.ts` files — so these flat files are never reached through any public import path. They should either be removed to avoid confusion, or wired into `package.json` as the canonical entry point (and the subdirectory index files removed).
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: packages/ui/src/components/overflow-fade/OverflowFadeContainer/OverflowFadeContainer.tsx
Line: 81-98
Comment:
**`onOverflowChange` fires spuriously when callback identity changes**
`onOverflowChange` is listed in the `useLayoutEffect` dependency array (line 97). If a consumer passes an un-memoized inline function, the effect re-runs on every parent render and calls `onOverflowChange` even though the overflow state has not changed. The current `TabBar` consumer correctly wraps `handleOverflowChange` in `useCallback`, but future consumers may not. Consider holding a stable ref to `onOverflowChange` so the effect only runs when the overflow *state* changes:
```tsx
const onOverflowChangeRef = useRef(onOverflowChange);
useLayoutEffect(() => { onOverflowChangeRef.current = onOverflowChange; });
useLayoutEffect(() => {
onOverflowChangeRef.current?.({ hasOverflowX, hasOverflowY, ... });
}, [canScrollBottom, canScrollLeft, canScrollRight, canScrollTop, hasOverflowX, hasOverflowY]);
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "Add shared overflow fade utilities" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+84
to
+86
| useIsomorphicLayoutEffect(() => { | ||
| updateOverflow(); | ||
| }); |
There was a problem hiding this comment.
The useIsomorphicLayoutEffect(() => { updateOverflow(); }) on line 84 has no dependency array, so it fires after every single render of any component using this hook. Each call reads scrollWidth, clientWidth, scrollLeft, etc. from the DOM — which forces a layout/reflow. For a component like TabBar that can re-render frequently (e.g., during drag-and-drop or tab hover), this adds an unnecessary DOM-read per render. The state equality check prevents extra re-renders, but it does not avoid the DOM read itself. Consider gating this on an enabled flag or relying solely on the ResizeObserver + scroll listener set up in the second effect.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/ui/src/hooks/use-overflow-fade.ts
Line: 84-86
Comment:
**Unconditional layout effect re-reads DOM on every render**
The `useIsomorphicLayoutEffect(() => { updateOverflow(); })` on line 84 has no dependency array, so it fires after every single render of any component using this hook. Each call reads `scrollWidth`, `clientWidth`, `scrollLeft`, etc. from the DOM — which forces a layout/reflow. For a component like `TabBar` that can re-render frequently (e.g., during drag-and-drop or tab hover), this adds an unnecessary DOM-read per render. The state equality check prevents extra re-renders, but it does not avoid the DOM read itself. Consider gating this on an `enabled` flag or relying solely on the ResizeObserver + scroll listener set up in the second effect.
How can I resolve this? If you propose a fix, please make it concise.| @@ -0,0 +1 @@ | |||
| export { OverflowFadeContainer } from "./OverflowFadeContainer"; | |||
There was a problem hiding this comment.
overflow-fade-container.tsx and overflow-fade-text.tsx at the overflow-fade/ root both re-export from the respective subdirectory, but the package.json exports map directly to the subdirectory index.ts files — so these flat files are never reached through any public import path. They should either be removed to avoid confusion, or wired into package.json as the canonical entry point (and the subdirectory index files removed).
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/ui/src/components/overflow-fade/overflow-fade-container.tsx
Line: 1
Comment:
**Redundant flat re-export files are dead code**
`overflow-fade-container.tsx` and `overflow-fade-text.tsx` at the `overflow-fade/` root both re-export from the respective subdirectory, but the `package.json` exports map directly to the subdirectory `index.ts` files — so these flat files are never reached through any public import path. They should either be removed to avoid confusion, or wired into `package.json` as the canonical entry point (and the subdirectory index files removed).
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+81
to
+98
| useLayoutEffect(() => { | ||
| onOverflowChange?.({ | ||
| hasOverflowX, | ||
| hasOverflowY, | ||
| canScrollLeft, | ||
| canScrollRight, | ||
| canScrollTop, | ||
| canScrollBottom, | ||
| }); | ||
| }, [ | ||
| canScrollBottom, | ||
| canScrollLeft, | ||
| canScrollRight, | ||
| canScrollTop, | ||
| hasOverflowX, | ||
| hasOverflowY, | ||
| onOverflowChange, | ||
| ]); |
There was a problem hiding this comment.
onOverflowChange fires spuriously when callback identity changes
onOverflowChange is listed in the useLayoutEffect dependency array (line 97). If a consumer passes an un-memoized inline function, the effect re-runs on every parent render and calls onOverflowChange even though the overflow state has not changed. The current TabBar consumer correctly wraps handleOverflowChange in useCallback, but future consumers may not. Consider holding a stable ref to onOverflowChange so the effect only runs when the overflow state changes:
const onOverflowChangeRef = useRef(onOverflowChange);
useLayoutEffect(() => { onOverflowChangeRef.current = onOverflowChange; });
useLayoutEffect(() => {
onOverflowChangeRef.current?.({ hasOverflowX, hasOverflowY, ... });
}, [canScrollBottom, canScrollLeft, canScrollRight, canScrollTop, hasOverflowX, hasOverflowY]);Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/ui/src/components/overflow-fade/OverflowFadeContainer/OverflowFadeContainer.tsx
Line: 81-98
Comment:
**`onOverflowChange` fires spuriously when callback identity changes**
`onOverflowChange` is listed in the `useLayoutEffect` dependency array (line 97). If a consumer passes an un-memoized inline function, the effect re-runs on every parent render and calls `onOverflowChange` even though the overflow state has not changed. The current `TabBar` consumer correctly wraps `handleOverflowChange` in `useCallback`, but future consumers may not. Consider holding a stable ref to `onOverflowChange` so the effect only runs when the overflow *state* changes:
```tsx
const onOverflowChangeRef = useRef(onOverflowChange);
useLayoutEffect(() => { onOverflowChangeRef.current = onOverflowChange; });
useLayoutEffect(() => {
onOverflowChangeRef.current?.({ hasOverflowX, hasOverflowY, ... });
}, [canScrollBottom, canScrollLeft, canScrollRight, canScrollTop, hasOverflowX, hasOverflowY]);
```
How can I resolve this? If you propose a fix, please make it concise.![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
packages/ui/src/hooks/use-overflow-fade.ts (2)
65-68: Missing explicit return type on the hook.
useOverflowFadereturns{ ref, updateOverflow, ...state }and is part of the package's public API (consumed across packages and apps). Adding an explicit return type prevents accidental signature drift and improves IDE introspection for downstream consumers.♻️ Suggested signature
-export function useOverflowFade<TElement extends HTMLElement>({ +interface UseOverflowFadeReturn<TElement extends HTMLElement> + extends OverflowFadeState { + ref: React.RefObject<TElement | null>; + updateOverflow: () => void; +} + +export function useOverflowFade<TElement extends HTMLElement>({ observeChildren = false, observeParent = false, -}: UseOverflowFadeOptions = {}) { +}: UseOverflowFadeOptions = {}): UseOverflowFadeReturn<TElement> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/hooks/use-overflow-fade.ts` around lines 65 - 68, The hook useOverflowFade currently lacks an explicit return type which can lead to signature drift; update its declaration to include a concrete exported return interface/type (e.g., an exported UseOverflowFadeReturn<TElement> or similar) that matches the actual returned shape ({ ref, updateOverflow, ...state }) and reference existing types like UseOverflowFadeOptions and the updateOverflow function signature, then use that return type on the function declaration to stabilize the public API and improve IDE typings.
84-86: Dep-less layout effect causes a forced reflow on every render.This
useIsomorphicLayoutEffecthas no dependency array, so it runs after every render and synchronously readsscrollWidth/scrollHeight/scrollLeft/scrollTop(all layout-thrashing reads). ThesetStateis guarded byareOverflowStatesEqual, so it won't loop, but the DOM read itself is unconditional and will be paid by every consumer on every parent re-render — even when nothing layout-relevant changed.The second effect already calls
updateOverflow()once duringobserveResizeTargets(), and any subsequent layout change is covered by theResizeObserver/MutationObserver/scroll/resizelisteners. This block looks redundant.♻️ Proposed simplification
- useIsomorphicLayoutEffect(() => { - updateOverflow(); - }); - useIsomorphicLayoutEffect(() => {If you want to keep it for "external prop changes that affect layout but don't fire ResizeObserver", consider exposing
updateOverflow(already returned) so consumers can trigger it explicitly when needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/hooks/use-overflow-fade.ts` around lines 84 - 86, The layout effect calling updateOverflow with no dependency array (useIsomorphicLayoutEffect(() => { updateOverflow(); })) causes unconditional layout reads on every render; remove this effect block entirely and rely on the existing observeResizeTargets() flow (which already calls updateOverflow once and updates via ResizeObserver/MutationObserver/scroll/resize listeners), and if you want to keep an escape hatch, expose the updateOverflow function (already returned) so consumers can call it manually for external prop-driven layout changes; reference useIsomorphicLayoutEffect, updateOverflow, observeResizeTargets, areOverflowStatesEqual, ResizeObserver and MutationObserver when making the change.packages/ui/src/components/overflow-fade/OverflowFadeContainer/OverflowFadeContainer.tsx (1)
50-62: Optional: acceptrefdirectly as a prop instead of usingforwardRef.React 19 allows functional components to receive
refas a regular prop, making theforwardRefwrapper unnecessary. UpdateOverflowFadeContainerPropsto includeref(or useComponentPropsWithRef<"div">), remove theforwardRefwrapper andsetForwardedRefhelper, and merge the hook's internal ref with the prop ref directly in a callback ref.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/overflow-fade/OverflowFadeContainer/OverflowFadeContainer.tsx` around lines 50 - 62, The component currently uses forwardRef and setForwardedRef; update OverflowFadeContainer to accept ref as a normal prop by extending OverflowFadeContainerProps (e.g., use ComponentPropsWithRef<"div"> or add ref?: Ref<HTMLDivElement>), remove the forwardRef wrapper and the setForwardedRef helper, and replace the forwardedRef logic by merging the hook’s internal ref with the incoming ref via a callback ref inside the OverflowFadeContainer function so the component uses the provided ref directly while retaining internal observation behavior (reference symbols: OverflowFadeContainer, OverflowFadeContainerProps, setForwardedRef, forwardedRef, observeChildren).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/ui/src/components/overflow-fade/overflow-fade-text.tsx`:
- Line 1: Remove the dead barrel file that re-exports OverflowFadeText: delete
the top-level file that contains `export { OverflowFadeText } from
"./OverflowFadeText";` because the component is already exported via the folder
index at `OverflowFadeText/index.ts`; ensure no imports reference the deleted
file and keep the canonical export in `OverflowFadeText/index.ts` to conform to
the one-folder-per-component convention.
In
`@packages/ui/src/components/overflow-fade/OverflowFadeContainer/OverflowFadeContainer.tsx`:
- Around line 81-98: The effect in OverflowFadeContainer uses onOverflowChange
in the useLayoutEffect dependency list which causes the callback to fire
whenever its identity changes; change the implementation to track the latest
onOverflowChange via a ref (e.g., latestOnOverflowChangeRef) and make the
useLayoutEffect depend only on the six measured flags (hasOverflowX,
hasOverflowY, canScrollLeft, canScrollRight, canScrollTop, canScrollBottom),
then call latestOnOverflowChangeRef.current(...) inside the effect so consumers
can pass non-memoized callbacks without triggering extra runs; alternatively,
add a short note in the component docs advising consumers to memoize
onOverflowChange if they update state in it.
In `@packages/ui/src/hooks/use-overflow-fade.ts`:
- Around line 101-103: The code captures observeParent once at mount so
observeResizeTargets can keep observing the original parent after reparenting;
update observeResizeTargets (and the ResizeObserver bookkeeping) to re-read
node.parentElement each time it runs, compare it to the currently-observed
parent, unobserve the old parent and observe the new one (i.e., track an
observedParent variable alongside resizeObserver.observe/unobserve), and/or add
a MutationObserver on the node itself to detect parent changes and call
observeResizeTargets when reparenting occurs; also document the caveat on
observeParent/observeChildren for OverflowFadeText if you prefer not to add the
parent-change handling.
---
Nitpick comments:
In
`@packages/ui/src/components/overflow-fade/OverflowFadeContainer/OverflowFadeContainer.tsx`:
- Around line 50-62: The component currently uses forwardRef and
setForwardedRef; update OverflowFadeContainer to accept ref as a normal prop by
extending OverflowFadeContainerProps (e.g., use ComponentPropsWithRef<"div"> or
add ref?: Ref<HTMLDivElement>), remove the forwardRef wrapper and the
setForwardedRef helper, and replace the forwardedRef logic by merging the hook’s
internal ref with the incoming ref via a callback ref inside the
OverflowFadeContainer function so the component uses the provided ref directly
while retaining internal observation behavior (reference symbols:
OverflowFadeContainer, OverflowFadeContainerProps, setForwardedRef,
forwardedRef, observeChildren).
In `@packages/ui/src/hooks/use-overflow-fade.ts`:
- Around line 65-68: The hook useOverflowFade currently lacks an explicit return
type which can lead to signature drift; update its declaration to include a
concrete exported return interface/type (e.g., an exported
UseOverflowFadeReturn<TElement> or similar) that matches the actual returned
shape ({ ref, updateOverflow, ...state }) and reference existing types like
UseOverflowFadeOptions and the updateOverflow function signature, then use that
return type on the function declaration to stabilize the public API and improve
IDE typings.
- Around line 84-86: The layout effect calling updateOverflow with no dependency
array (useIsomorphicLayoutEffect(() => { updateOverflow(); })) causes
unconditional layout reads on every render; remove this effect block entirely
and rely on the existing observeResizeTargets() flow (which already calls
updateOverflow once and updates via
ResizeObserver/MutationObserver/scroll/resize listeners), and if you want to
keep an escape hatch, expose the updateOverflow function (already returned) so
consumers can call it manually for external prop-driven layout changes;
reference useIsomorphicLayoutEffect, updateOverflow, observeResizeTargets,
areOverflowStatesEqual, ResizeObserver and MutationObserver when making the
change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b3020118-43c9-4668-b967-de9598490a73
📒 Files selected for processing (14)
apps/desktop/src/renderer/globals.cssapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarPortsList/components/DashboardSidebarPortGroup/DashboardSidebarPortGroup.tsxpackages/panes/src/react/components/Workspace/components/TabBar/TabBar.tsxpackages/panes/src/react/components/Workspace/components/TabBar/components/TabItem/TabItem.tsxpackages/ui/package.jsonpackages/ui/src/components/overflow-fade/OverflowFadeContainer/OverflowFadeContainer.tsxpackages/ui/src/components/overflow-fade/OverflowFadeContainer/index.tspackages/ui/src/components/overflow-fade/OverflowFadeText/OverflowFadeText.tsxpackages/ui/src/components/overflow-fade/OverflowFadeText/index.tspackages/ui/src/components/overflow-fade/README.mdpackages/ui/src/components/overflow-fade/fade-edge.csspackages/ui/src/components/overflow-fade/overflow-fade-container.tsxpackages/ui/src/components/overflow-fade/overflow-fade-text.tsxpackages/ui/src/hooks/use-overflow-fade.ts
💤 Files with no reviewable changes (2)
- packages/ui/src/components/overflow-fade/fade-edge.css
- apps/desktop/src/renderer/globals.css
Contributor
🚀 Preview Deployment🔗 Preview Links
Preview updates automatically with new commits |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
2 issues found across 14 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/ui/src/hooks/use-overflow-fade.ts">
<violation number="1" location="packages/ui/src/hooks/use-overflow-fade.ts:84">
P2: This layout effect has no dependency array, so it forces a synchronous DOM geometry read (`scrollWidth`, `clientWidth`, `scrollLeft`, etc.) on every render. For frequently-rendering consumers like `TabBar` (drag-and-drop, hover), this triggers an unnecessary layout/reflow per render. The state equality check avoids extra re-renders but not the DOM read itself. The `ResizeObserver` + scroll listener in the second effect already cover resize- and scroll-driven changes. Consider removing this unconditional effect or gating it behind a dependency array so it only runs when the measured node changes.</violation>
</file>
<file name="packages/ui/src/components/overflow-fade/OverflowFadeContainer/OverflowFadeContainer.tsx">
<violation number="1" location="packages/ui/src/components/overflow-fade/OverflowFadeContainer/OverflowFadeContainer.tsx:57">
P2: Including `onOverflowChange` in the dependency array means this effect re-fires whenever the callback identity changes. An un-memoized inline callback from a consumer will cause the effect to run on every render, and if that callback performs a `setState`, it creates a re-render loop. Use the "latest ref" pattern to decouple callback identity from effect execution:
```tsx
const onOverflowChangeRef = useRef(onOverflowChange);
useLayoutEffect(() => {
onOverflowChangeRef.current = onOverflowChange;
});
useLayoutEffect(() => {
onOverflowChangeRef.current?.({ ... });
}, [canScrollBottom, canScrollLeft, canScrollRight, canScrollTop, hasOverflowX, hasOverflowY]);
```</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Remove duplicate barrel shims, inline forwardRef via React 19 ref-as-prop, drop redundant layout effect and SSR guards in use-overflow-fade, and collapse the on-hover tab scrollbar back to hide-scrollbar.
Stash the callback in a ref so the effect only fires when measured overflow flags change, not on every consumer render with an unmemoized callback.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
@superset/ui.OverflowFadeContainer,OverflowFadeText, anduseOverflowFadefor measured, conditional fades.Validation
bunx biome check --write --unsafe ...bun run --cwd packages/ui typecheckbun run --cwd packages/panes typecheckbun run --cwd apps/desktop typecheckgit diff --checkNotes
bun.lockchanges were left out of this PR.Summary by cubic
Moves overflow fade into
@superset/uiwith measured, edge-aware fades and stabilized overflow-change callbacks. Adopts it in TabBar and the dashboard ports list, simplifies overflow logic, and removes custom CSS; tabs now always hide scrollbars.New Features
OverflowFadeContainer,OverflowFadeText, anduseOverflowFadefor conditional fades.@superset/ui/overflow-fade-container,@superset/ui/overflow-fade-text, and@superset/ui/overflow-fade.css.Refactors
OverflowFadeContainer(withobserveChildren) andOverflowFadeTextfor tab titles; replaced manual observers withonOverflowChange; tab scrollbar always hidden.fade-edge.cssand old.fade-edge-rusage; simplified internals and stabilizedOverflowFadeContaineronOverflowChangeto fire only when overflow flags change.Written for commit 2749d87. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
New Features
Refactor