Skip to content

fix(fe): beautify tabs in actor details #3309

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 4 commits into from
Nov 6, 2025
Merged

fix(fe): beautify tabs in actor details #3309

merged 4 commits into from
Nov 6, 2025
+164 −125

Conversation

@jog1t
Copy link
Contributor

@jog1t jog1t commented Nov 3, 2025 •
edited
Loading

Closes FRONT-874

@vercel
Copy link

vercel bot commented Nov 3, 2025 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
rivet-inspector Error Error Nov 3, 2025 8:17pm
rivet-site Building Building Preview Comment Nov 3, 2025 8:17pm
2 Skipped Deployments
Project Deployment Preview Comments Updated (UTC)
rivet-cloud Ignored Ignored Nov 3, 2025 8:17pm
rivetkit-serverless Skipped Skipped Nov 3, 2025 8:17pm

@vercel vercel bot temporarily deployed to Preview – rivetkit-serverless November 3, 2025 20:15 Inactive
@linear
Copy link

linear bot commented Nov 3, 2025

FRONT-874 Layout inspector bug

This was referenced Nov 3, 2025
Closed
Closed
Closed
Closed
@jog1t jog1t marked this pull request as ready for review November 3, 2025 20:16
This was referenced Nov 3, 2025
Closed
Closed
Closed
Closed
This was referenced Nov 3, 2025
Merged
Merged
@jog1t Graphite App
Copy link
Contributor Author

jog1t commented Nov 3, 2025 •
edited
Loading

How to use the Graphite Merge Queue

Add the label merge-queue to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@jog1t jog1t mentioned this pull request Nov 3, 2025
Merged
@claude
Copy link

claude bot commented Nov 3, 2025

Code Review for PR #3309: fix(fe): beautify tabs in actor details

Overview

This PR improves the UI/UX of the actor details tabs by making them horizontally scrollable and adding responsive behavior for the status label. The changes are focused on the frontend presentation layer.

✅ Positive Observations

  1. Good use of container queries: The @container/actor and @lg: responsive utilities show thoughtful consideration for component-level responsive design.

  2. Accessibility consideration: Using items-safe-end is a good practice to prevent content overflow in flex containers.

  3. Visual polish: The gradient fade effect (bg-gradient-to-r from-transparent to-card) on the sticky status section is a nice touch for indicating scrollable content.

  4. Proper use of data attributes: Adding data-slot="status-label" follows a clean pattern for targeted styling.

🔍 Code Quality & Best Practices

1. ScrollArea Usage Pattern

The implementation wraps the tab list in a ScrollArea component:

<ScrollArea
  className="h-full items-end"
  viewportProps={{
    className: "h-full [&>div]:h-full",
  }}
>

Observation: The items-end class on ScrollArea itself may not have the intended effect since ScrollArea is a wrapper. The alignment is correctly applied to TabsList with items-safe-end. Consider removing items-end from ScrollArea's className for clarity.

2. Layout Structure Complexity

The new structure introduces additional nesting:

  • ScrollArea → div (relative) → TabsList

Consideration: While this works, it adds layout complexity. Ensure this nesting is necessary for the scrolling behavior.

3. Sticky Positioning

<Flex className="h-full pb-3 pt-2 pr-4 sticky right-0 bg-card">

Potential Issue: The sticky positioning with right-0 might not work as expected inside a ScrollArea, since sticky elements need a scrolling ancestor. Verify this behaves correctly when tabs overflow - the status section should remain visible while tabs scroll.

4. Tailwind Custom Utilities

The addition of items-safe-start and items-safe-end to tailwind-base.ts is good, but:

  • Only items-safe-end is used in this PR
  • Consider if items-safe-start is needed now or if it should be added when actually used

🐛 Potential Issues

1. Horizontal Scroll Behavior

The ScrollArea component renders both horizontal and vertical scrollbars by default (see scroll-area.tsx:35-36). For this use case:

  • Only horizontal scrolling is needed
  • Vertical scrollbar may appear unnecessarily

Recommendation: Pass orientation="horizontal" or hide the vertical scrollbar via additional props/styling.

2. Fixed Height Constraint

The parent container has h-[45px], and internal elements use h-full:

<div className="flex justify-between items-center border-b h-[45px]">
  <ScrollArea className="h-full items-end" ...>

Question: Does this 45px constraint work well across all viewport sizes and zoom levels? Consider testing at different zoom levels and with browser font size adjustments for accessibility.

3. Margin on TabsList

<TabsList className="... mr-2.5">

With the sticky status section already having padding, this margin might create inconsistent spacing. Verify the visual alignment between the tabs and the sticky section edge.

🎯 Performance Considerations

  • ✅ No performance concerns identified
  • ✅ No unnecessary re-renders introduced
  • ✅ Component memoization is already in place (memo on ActorsActorDetails)

🔒 Security Concerns

  • ✅ No security issues identified
  • ✅ No user input handling or XSS vectors in changed code

🧪 Test Coverage

Observation: This PR doesn't include tests for:

  • Responsive behavior of status label visibility
  • Horizontal scroll functionality
  • Sticky positioning behavior

Recommendation: Consider adding visual regression tests or E2E tests for:

  1. Tab overflow scrolling behavior
  2. Status label visibility at different container widths
  3. Sticky positioning of the status/stop button section

📋 Suggestions

  1. Test the sticky behavior: Ensure the status section stays visible when scrolling tabs
  2. Hide vertical scrollbar: Only horizontal scrolling should be enabled
  3. Verify spacing: Check that mr-2.5 on TabsList and pr-4 on sticky section create the intended visual result
  4. Accessibility testing: Test with keyboard navigation, screen readers, and browser zoom
  5. Remove unused utility: Consider removing items-safe-start from tailwind-base.ts or add a comment explaining its future use

✨ Overall Assessment

This is a solid UI improvement that enhances the user experience when dealing with multiple actor tabs. The responsive design and scrolling behavior are thoughtfully implemented. The main concerns are around verifying the sticky positioning works correctly within the ScrollArea and ensuring only horizontal scrolling is active.

Recommendation: Approve with minor verification of the sticky behavior and scroll direction.

@graphite-app
Copy link
Contributor

graphite-app bot commented Nov 3, 2025

Graphite Automations

"Test" took an action on this PR • (11/03/25)

1 assignee was added to this PR based on Kacper Wojciechowski's automation.

Base automatically changed from 11-03-fix_fe_make_changelog_non_blocking_resource to graphite-base/3306 November 6, 2025 17:53
@jog1t jog1t merged commit a28b82b into graphite-base/3306 Nov 6, 2025
6 of 10 checks passed
@jog1t jog1t deleted the 11-03-fix_fe_beautify_tabs_in_actor_details branch November 6, 2025 17:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@jog1t jog1t

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@jog1t