refactor(frontend): shared UI primitives — PanelSection, split pane, context menu, composed tooltip - #5868
refactor(frontend): shared UI primitives — PanelSection, split pane, context menu, composed tooltip#5868ardaerzin wants to merge 5 commits into
Conversation
|
@coderabbitai review |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
✅ Action performedReview finished.
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR adds UI primitives, chat presentation components, package exports, layout fallbacks, and overflow updates. It also changes ChangesUI component additions
Shared time utility
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Pointer
participant SplitPane
participant ResizeCallbacks
Pointer->>SplitPane: pointerdown on separator
SplitPane->>ResizeCallbacks: onResizeStart()
Pointer->>SplitPane: pointermove with captured pointer
SplitPane->>SplitPane: clamp driven pane size
SplitPane->>ResizeCallbacks: onResize(nextSize)
Pointer->>SplitPane: pointerup
SplitPane->>ResizeCallbacks: onResizeEnd()
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
web/packages/agenta-ui/src/components/presentational/chat/index.tsx (1)
1-7: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueShorten non-contract comments.
These multi-line comments describe ordinary component purpose and visual details. Replace them with one short comment, or remove them when the component name and types already describe the behavior.
As per coding guidelines, “Keep in-code comments to at most one short line; use longer comments only for genuinely surprising constraints such as bugs, races, or ordering requirements.”
Also applies to: 41-45, 150-154
Source: Coding guidelines
web/packages/agenta-ui/src/components/ui/context-menu.tsx (1)
7-17: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReduce multi-line implementation comments.
Keep one short in-code comment only when it documents a surprising constraint. Move migration guidance and component rationale to external documentation or a story.
web/packages/agenta-ui/src/components/ui/context-menu.tsx#L7-L17: reduce the migration and styling rationale to a short constraint comment.web/packages/agenta-ui/src/components/ui/tooltip-composed.tsx#L14-L18: reduce the usage guidance to a short component comment.web/packages/agenta-ui/src/components/ui/split-pane.tsx#L5-L17: reduce the implementation history and visual rationale to a short constraint comment.Source: Coding guidelines
web/packages/agenta-ui/src/components/ui/split-pane.tsx (1)
127-127: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a supported semantic theme color token.
bg-[var(--ag-surface-gutter)]bypasses the approved theme color forms. Replace it with a semantic Tailwind color utility or a supportedvar(--ag-color*)token.Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 91c27599-33f3-4fd3-9bee-d0590051d2a5
📒 Files selected for processing (10)
web/packages/agenta-shared/src/utils/timeAgo.tsweb/packages/agenta-ui/package.jsonweb/packages/agenta-ui/src/components/PageLayout.tsxweb/packages/agenta-ui/src/components/presentational/chat/index.tsxweb/packages/agenta-ui/src/components/presentational/index.tsweb/packages/agenta-ui/src/components/presentational/layout/PanelSection.tsxweb/packages/agenta-ui/src/components/ui/context-menu.tsxweb/packages/agenta-ui/src/components/ui/index.tsweb/packages/agenta-ui/src/components/ui/split-pane.tsxweb/packages/agenta-ui/src/components/ui/tooltip-composed.tsx
| */ | ||
| export const timeAgo = (ts?: number): string => { | ||
| if (ts == null || !Number.isFinite(ts)) return "" | ||
| if (!ts) return "" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Preserve finite timestamp validation.
!ts treats the valid Unix timestamp 0 as missing and allows Infinity through. This can hide epoch timestamps and produce invalid output such as Infinityd ago. Keep the explicit null and Number.isFinite checks.
Proposed fix
- if (!ts) return ""
+ if (ts == null || !Number.isFinite(ts)) return ""📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!ts) return "" | |
| if (ts == null || !Number.isFinite(ts)) return "" |
| "./surfaces.css": "./src/styles/surfaces.css", | ||
| "./code-editor-styles.css": "./src/styles/code-editor-styles.css", | ||
| "./editor-theme.css": "./src/styles/editor-theme.css", | ||
| "./custom-resize-handle.css": "./src/styles/custom-resize-handle.css", | ||
| "./theme-variables.css": "./src/styles/theme-variables.css", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Package file excerpt:\n'
cat -n web/packages/agenta-ui/package.json | sed -n '1,90p'
printf '\nRelevant files/exports locations:\n'
for f in \
web/packages/agenta-ui/src/styles/surfaces.css \
web/packages/agenta-ui/src/styles/code-editor-styles.css \
web/packages/agenta-ui/src/styles/editor-theme.css \
web/packages/agenta-ui/src/styles/custom-resize-handle.css \
web/packages/agenta-ui/src/styles/theme-variables.css
do
if [ -f "$f" ]; then
printf '%s: %s\n' "$f" "$(wc -l < "$f") lines"
else
printf '%s: missing\n' "$f"
fi
done
printf '\nSearch for sideEffects and CSS package imports:\n'
rg -n '"sideEffects"|"./.*\.css"' web/packages/agenta-ui/package.json web/packages/agenta-ui -g '*.json' -g '*.ts' -g '*.tsx' -g '*.js' -g '*.jsx' | head -n 120Repository: Agenta-AI/agenta
Length of output: 6381
🌐 Web query:
npm package.json sideEffects false CSS imports tree shaking documentation
💡 Result:
Setting "sideEffects": false in your package.json enables aggressive tree shaking by telling the bundler (like Webpack) that your package contains no side effects and can safely prune unused exports [1][2][3]. However, this often causes CSS imports to be dropped from your production bundle [4][3][5]. Because CSS imports (e.g., import './styles.css') do not export anything that the JavaScript code consumes, the bundler treats them as "unused" and removes them when "sideEffects": false is declared [4][6][7]. To fix this, you must explicitly mark your CSS files as having side effects so the bundler preserves them [1][2][3]. Do not set "sideEffects" to false globally if your package includes CSS; instead, use an array to specify which files have side effects [4][2][3]. Recommended Configuration: Use the following format in your package.json to preserve CSS while still enabling tree shaking for your JavaScript files [4][2][8]: { "sideEffects": [ ".css", ".scss", ".sass", ".less" ] } By providing this array, you instruct the bundler to skip tree shaking only for the specified file patterns, ensuring your styles are included in the final build while keeping your JavaScript code optimized [4][3].
Citations:
- 1: https://webpack.js.org/guides/tree-shaking
- 2: https://docs.webpack.js.org/guides/tree-shaking
- 3: https://www.code-splitting.com/advanced-tree-shaking-dependency-optimization/configuring-sideeffects-for-optimal-tree-shaking/
- 4: https://www.code-splitting.com/advanced-tree-shaking-dependency-optimization/configuring-sideeffects-for-optimal-tree-shaking/fixing-css-imports-stripped-by-tree-shaking/
- 5: https://webpack.js.org/guides/tree-shaking/
- 6: https://stackoverflow.com/questions/49160752/what-does-webpack-4-expect-from-a-package-with-sideeffects-false
- 7: Imported CSS from packages with sideEffects set to false is omitted from bundle when using
importsyntax. gatsbyjs/gatsby#31388 - 8: optimization.sideEffects = true causes issues with local CSS webpack/webpack#6741
Preserve public CSS imports during tree-shaking.
"sideEffects": false can remove CSS package entry points from consuming production builds. Mark CSS files as side effects.
Proposed fix
- "sideEffects": false,
+ "sideEffects": ["**/*.css"],| if (kind === "video") { | ||
| return ( | ||
| <video | ||
| src={src} | ||
| controls | ||
| onError={onVideoError} | ||
| className={cn(CARD_WIDTH, "rounded-md", className)} | ||
| /> | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Render a placeholder for unloaded video attachments.
src is optional and loading represents media fetching. The video branch renders an empty <video> control when loading is true or src is absent. Return the same loading placeholder before rendering the video element.
Proposed fix
if (kind === "video") {
+ if (loading || !src) {
+ return (
+ <div
+ className={cn(
+ CARD_WIDTH,
+ "h-[140px] animate-pulse rounded-md bg-colorFillTertiary",
+ className,
+ )}
+ aria-label={`Loading ${name}`}
+ />
+ )
+ }
return (
<video📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (kind === "video") { | |
| return ( | |
| <video | |
| src={src} | |
| controls | |
| onError={onVideoError} | |
| className={cn(CARD_WIDTH, "rounded-md", className)} | |
| /> | |
| ) | |
| if (kind === "video") { | |
| if (loading || !src) { | |
| return ( | |
| <div | |
| className={cn( | |
| CARD_WIDTH, | |
| "h-[140px] animate-pulse rounded-md bg-colorFillTertiary", | |
| className, | |
| )} | |
| aria-label={`Loading ${name}`} | |
| /> | |
| ) | |
| } | |
| return ( | |
| <video | |
| src={src} | |
| controls | |
| onError={onVideoError} | |
| className={cn(CARD_WIDTH, "rounded-md", className)} | |
| /> | |
| ) |
| // `overflow-clip`, not `-hidden`: hidden makes this the sticky header's | ||
| // scroll ancestor, so it never pins against PanelScroll. | ||
| "shrink-0 overflow-clip rounded-xl border border-solid border-colorBorderSecondary bg-colorBgElevated" | ||
| "shrink-0 overflow-hidden rounded-xl border border-solid border-colorBorderSecondary bg-colorBgElevated" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate PanelSection and consumers"
fd -a 'PanelSection\.tsx$|PanelSurface\.tsx$|PanelScroll\.tsx$' web || true
echo
echo "File outline"
ast-grep outline web/packages/agenta-ui/src/components/presentational/layout/PanelSection.tsx --view expanded || true
echo
echo "Relevant PanelSection source"
sed -n '1,180p' web/packages/agenta-ui/src/components/presentational/layout/PanelSection.tsx
echo
echo "Search PanelSection usages"
rg -n "PanelSection|panel" web/packages -g '*.{tsx,ts,jsx,js}' | head -n 200Repository: Agenta-AI/agenta
Length of output: 37922
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Focused PanelSection imports/usages"
rg -n "from [\"']`@agenta/ui/components/presentational`[\"']|import \\{[^}]*PanelSection|<PanelSection" web/packages web/apps || true
echo
echo "Tailwind config overflows/packages"
fd -a 'tailwind|postcss|package\.json' web | while read -r f; do
case "$f" in
*tailwind*|*postcss*|*package.json*)
echo "--- $f"
rg -n "overflowHidden|overflowClip|container-type|overflow-clip|overflow-hidden|plugins|tailwindcss|`@tailwind`" "$f" || true
;;
esac
done
echo
echo "Existing overflow-clip usage"
rg -n "overflow-clip|overflowClip" web/packages web/apps || true
echo
echo "PanelSection export barrel"
rg -n "PanelSection|presentational" web/packages/agenta-ui/src -g '*.{ts,tsx}' | head -n 120Repository: Agenta-AI/agenta
Length of output: 17510
🌐 Web query:
MDN CSS sticky position element nearest ancestor with overflow clipping behavior overflow-clip overflow-hidden
💡 Result:
In CSS, a position: sticky element is anchored to its nearest scrolling ancestor [1][2]. A "scrolling ancestor" is defined as any ancestor element with an overflow value other than visible (e.g., hidden, scroll, or auto) [1][3][2]. When you set overflow: hidden on an ancestor, it becomes a scroll container—even if it has no visible scrollbars and cannot be scrolled by the user [4][3][2]. Because of this, the sticky element calculates its position relative to that ancestor rather than the viewport (or a parent that is actually intended to be the scroll container), often causing the sticky behavior to fail or appear broken [1][3][2]. The overflow: clip property provides a solution to this issue [4][3][2]. Like overflow: hidden, it clips content that extends beyond the element's box [5][4]. However, unlike hidden, overflow: clip specifically forbids all scrolling (including programmatic scrolling) and does not establish a scroll container [5][4][3]. Consequently, it does not become a sticky anchor, allowing the sticky element to "look past it" and anchor to the next available scrolling ancestor (typically the viewport) [6][4][3]. Key takeaway: If you need to hide overflow on an ancestor but want to maintain position: sticky behavior for a descendant, use overflow: clip instead of overflow: hidden [4][2].
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/CSS/Reference/Properties/position
- 2: https://polypane.app/blog/getting-stuck-all-the-ways-position-sticky-can-fail/
- 3: https://stackoverflow.com/questions/43909940/why-does-overflowhidden-prevent-positionsticky-from-working
- 4: https://gomakethings.com/articles/the-overflow-hidden-property-and-sticky-headers/
- 5: https://developer.mozilla.org/en-US/docs/Web/CSS/Reference/Properties/overflow-y
- 6: [css3 positioning] support position:sticky inside an overflow:hidden|auto on general parents w3c/csswg-drafts#865
🌐 Web query:
Tailwind CSS overflow-clip utility class overflow clip
💡 Result:
The Tailwind CSS utility class overflow-clip applies the CSS property overflow: clip [1][2]. This utility is part of Tailwind's layout utilities for controlling how an element handles content that exceeds its container's bounds [1][3]. When applied, the element's content is clipped to the element's border box, and no scrollbars are provided [1]. Historical Note: In earlier versions of Tailwind CSS, the class name overflow-clip was used to represent text-overflow: clip [4]. To resolve a naming collision as the CSS overflow: clip property gained wider browser support, Tailwind renamed the text-related utility to text-clip and introduced the new overflow-clip utility for the overflow: clip property [4].
Citations:
- 1: https://tailwindcss.com/docs/overflow
- 2: https://tailscan.com/tailwind/layout/overflow-clip
- 3: https://docs.w3cub.com/tailwindcss/overflow
- 4: Resolve
overflow-clipnaming collision issue tailwindlabs/tailwindcss#5630
Preserve sticky headers when clipping rail sections.
When isRail and sticky are true, the header uses sticky top-0. The rail section’s overflow-hidden makes it the nearest scrolling ancestor, so the header pins to the section instead of the intended outer scroll container. Use overflow-clip for sticky rail sections or move clipping below the sticky header.
| const lastRef = React.useRef<{size: number; total: number}>({size: paneSize, total: 0}) | ||
|
|
||
| const clamp = React.useCallback( | ||
| (raw: number, total: number) => | ||
| Math.max(paneMin, Math.min(raw, paneMax, Math.max(paneMin, total - fillMin))), | ||
| [fillMin, paneMax, paneMin], | ||
| ) | ||
|
|
||
| const handlePointerDown = (e: React.PointerEvent<HTMLDivElement>) => { | ||
| if (!resizable) return | ||
| e.preventDefault() | ||
| e.currentTarget.setPointerCapture(e.pointerId) | ||
| setDragging(true) | ||
| onResizeStart?.() | ||
| } | ||
| const handlePointerMove = (e: React.PointerEvent<HTMLDivElement>) => { | ||
| if (!dragging) return | ||
| const rect = rootRef.current?.getBoundingClientRect() | ||
| if (!rect) return | ||
| const total = rect.width - BAR_WIDTH | ||
| const raw = | ||
| paneSide === "start" | ||
| ? e.clientX - rect.left - BAR_WIDTH / 2 | ||
| : rect.right - e.clientX - BAR_WIDTH / 2 | ||
| const size = clamp(Math.round(raw), total) | ||
| lastRef.current = {size, total} | ||
| onResize?.(size, total) | ||
| } | ||
| const handlePointerUp = (e: React.PointerEvent<HTMLDivElement>) => { | ||
| if (!dragging) return | ||
| e.currentTarget.releasePointerCapture(e.pointerId) | ||
| setDragging(false) | ||
| onResizeEnd?.(lastRef.current.size, lastRef.current.total) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Complete the resize lifecycle for clicks and cancelled pointers.
lastRef.current.total remains 0 until handlePointerMove runs. A press and release without movement calls onResizeEnd with an invalid total. A pointercancel also leaves dragging set and skips onResizeEnd.
Initialize lastRef from the root width in handlePointerDown. Handle pointer cancellation with the same completion path.
| <div | ||
| data-slot="split-pane-bar" | ||
| role="separator" | ||
| aria-orientation="vertical" | ||
| onPointerDown={handlePointerDown} | ||
| onPointerMove={handlePointerMove} | ||
| onPointerUp={handlePointerUp} | ||
| className={cn( | ||
| "group relative z-[5] box-border h-full shrink-0 touch-none select-none bg-[var(--ag-surface-gutter)]", | ||
| resizable && "cursor-col-resize", | ||
| barClassName, | ||
| )} | ||
| style={{flexBasis: BAR_WIDTH, width: BAR_WIDTH}} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Add keyboard resizing for the splitter control.
This separator changes pane width but has no tabIndex or keyboard handler. Keyboard users cannot resize the pane.
Make the resizable separator focusable. Support ArrowLeft and ArrowRight. Set aria-valuenow, aria-valuemin, and aria-valuemax from the active pane constraints.
…context menu, composed tooltip
…tful The manifest claimed "sideEffects": false while exports publishes five stylesheets (surfaces, editor-theme, code-editor-styles, custom-resize-handle, theme-variables). That licenses a bundler to drop a bare `import "@agenta/ui/surfaces.css"` as a no-op. Consumers reach these through a CSS @import today, so nothing is broken yet — the declaration is simply untrue, and tree-shaking still applies to every JS module.
…an empty player
ChatAttachmentCard guarded only the image branch with `loading || !src`; video fell
straight through to <video src={undefined} controls />, which paints a dead player.
Both media kinds now share one guard ahead of the split, so there is one placeholder
rather than two copies of it.
…rollport A rail PanelSection carried `overflow-hidden`, which makes the section a scroll container — so its `sticky top-0` header pinned to a box that never scrolls and the `sticky` prop did nothing (live on the agents strip and the agent list). `overflow-clip` rounds the same corners but establishes no scrollport, so the header pins to PanelScroll as intended. No call site relied on the section scrolling.
…he keyboard Three gaps in one lifecycle. A press-and-release with no movement never reached handlePointerMove, so onResizeEnd fired with the seed total of 0 and any ratio the caller derived from it was wrong; the root is now measured on pointerdown. A cancelled pointer (scroll takeover, browser gesture) fired nothing at all and left `dragging` stuck true; pointercancel and pointerup now share one exit. And the separator had no tabIndex, no key handler and no aria-value*, so it could not be resized by keyboard: it is focusable while resizable, Arrow keys step it (Shift for a coarse step), Home/End jump to the bounds, and it publishes valuenow/min/max from the same clamp the pointer path uses. Pointer and keyboard both run onResizeStart -> onResize -> onResizeEnd, so there is one callback contract, not two.
6d24e20 to
b904ca7
Compare
aa9863a to
d71bf16
Compare
1 participant
Four primitives the extracted surfaces all need, hoisted into
@agenta/ui:PanelSection, thesplit pane, the context menu, and the composed tooltip that carries its own provider.
They were duplicated across the OSS components that later lanes move into packages; pulling them
up first is what lets those lanes be pure moves.
Not run in a browser — static gates only (
pnpm lint-fix24/24,tsc --noEmitcleanfor
@agenta/shared,ui,entities,entity-ui,settings-ui,oss,ee,mobile).Stacked on
pkg/navigation-ui; review only this lane's diff.