-
Notifications
You must be signed in to change notification settings - Fork 17
Truncate long names #862
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.
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
base: staging
Are you sure you want to change the base?
Truncate long names #862
Conversation
WalkthroughAdds meta.className "max-w-[30ch]" to multiple "name"/key columns and refactors their cell DOM to flex-based layouts with min-w-0, flex-1, and truncate classes. CopyButton and tooltips retained, with minor spacing fix. No data logic changes; updates are presentational across pipelines, runs, secrets, service accounts, and stack components. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10–15 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/app/settings/service-accounts/columns.tsx (1)
54-63
: LGTM: Truncation layout correctly implemented.The flex layout with
min-w-0
andflex-1
on the Link container correctly enables text truncation. Thetruncate
class on the Link will apply ellipsis when the service account name exceeds the column width.Optional: Consider adding a
title
attribute to the Link for better UX:<Link className="truncate text-text-md font-semibold text-theme-text-primary" to={routes.settings.service_accounts.detail(row.original.id)} + title={row.original.name} > {row.original.name} </Link>
This would show the full name in a native browser tooltip on hover when truncated.
src/components/stack-components/component-sheet/runs-tab/columns.tsx (1)
37-46
: Truncation implementation is correct. Consider adding a tooltip for truncated names.The flex layout with
min-w-0
on the Link (lines 39-42) andtruncate
on the span (line 43-45) correctly enables text truncation. The structure properly propagates width constraints from the table cell through the flex containers to the text element.However, when names exceed 30 characters, users have no visual way to view the full text. Consider wrapping the name Link with a Tooltip component (similar to the status tooltip at lines 47-54) to show the full name on hover, improving discoverability compared to relying solely on the CopyButton.
Example implementation:
<div className="flex flex-1 items-center gap-1"> + <TooltipProvider> + <Tooltip> + <TooltipTrigger asChild> <Link to={routes.projects.runs.detail(id)} className="flex min-w-0 flex-1 items-center gap-1" > <span className="truncate text-text-md font-semibold text-theme-text-primary"> {name} </span> </Link> + </TooltipTrigger> + <TooltipContent>{name}</TooltipContent> + </Tooltip> + </TooltipProvider> <TooltipProvider>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/app/pipelines/[pipelineId]/deployments/colunns/index.tsx
(1 hunks)src/app/pipelines/[pipelineId]/snapshots/columns/index.tsx
(1 hunks)src/app/pipelines/_components/columns.tsx
(1 hunks)src/app/runs/columns.tsx
(2 hunks)src/app/settings/secrets/[id]/columns.tsx
(3 hunks)src/app/settings/secrets/columns.tsx
(1 hunks)src/app/settings/service-accounts/[service-account-id]/columns.tsx
(1 hunks)src/app/settings/service-accounts/columns.tsx
(2 hunks)src/components/stack-components/component-sheet/runs-tab/columns.tsx
(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
src/app/pipelines/[pipelineId]/deployments/colunns/index.tsx (1)
src/router/routes.tsx (1)
routes
(1-86)
src/components/stack-components/component-sheet/runs-tab/columns.tsx (1)
src/router/routes.tsx (1)
routes
(1-86)
src/app/pipelines/_components/columns.tsx (2)
src/components/ExecutionStatus.tsx (1)
getExecutionStatusColor
(13-34)src/router/routes.tsx (1)
routes
(1-86)
src/app/settings/secrets/columns.tsx (1)
src/lib/code-snippets.ts (1)
getSecretSnippet
(22-27)
src/app/pipelines/[pipelineId]/snapshots/columns/index.tsx (1)
src/router/routes.tsx (1)
routes
(1-86)
src/app/runs/columns.tsx (1)
src/router/routes.tsx (1)
routes
(1-86)
🔇 Additional comments (17)
src/app/settings/service-accounts/columns.tsx (1)
42-44
: LGTM: Consistent width constraint applied.The
max-w-[30ch]
constraint appropriately limits column width to prevent layout issues with long names.src/app/pipelines/[pipelineId]/snapshots/columns/index.tsx (2)
55-57
: LGTM: Width constraint applied.
64-86
: LGTM: Proper truncation structure.The nested flex layout correctly implements truncation with
min-w-0
on the Link andtruncate
on the name span.src/app/pipelines/_components/columns.tsx (2)
55-57
: LGTM: Consistent width constraint.
64-96
: LGTM: Truncation implemented correctly with preserved functionality.The layout correctly implements truncation while maintaining the ExecutionStatusIcon tooltip and CopyButton functionality.
src/app/runs/columns.tsx (2)
57-59
: LGTM: Width constraint applied.
69-94
: LGTM: Correct truncation with preserved interactivity.The flex layout properly enables truncation while maintaining the tooltip and copy functionality.
src/app/settings/service-accounts/[service-account-id]/columns.tsx (2)
44-46
: LGTM: Width constraint applied.
51-60
: LGTM: Truncation correctly applied.The flex layout with
min-w-0
andtruncate
on the name container correctly implements text truncation for API key names.src/app/pipelines/[pipelineId]/deployments/colunns/index.tsx (2)
21-23
: LGTM: Width constraint applied.
30-52
: LGTM: Correct truncation implementation.The flex layout with
min-w-0
andtruncate
properly implements text truncation for deployment names.src/app/settings/secrets/columns.tsx (2)
18-20
: LGTM: Width constraint applied.
27-49
: LGTM: Truncation correctly implemented with preserved tooltip.The flex layout properly enables truncation while maintaining the SecretTooltip functionality.
src/app/settings/secrets/[id]/columns.tsx (3)
14-14
: LGTM: Formatting fix.Removed extra space in className.
30-32
: LGTM: Width constraint applied.
43-52
: LGTM: Truncation correctly implemented.The flex layout with
min-w-0
andtruncate
properly implements text truncation for secret keys while preserving the SecretTooltip functionality.src/components/stack-components/component-sheet/runs-tab/columns.tsx (1)
25-27
: LGTM! Max-width constraint applied correctly.The
max-w-[30ch]
constraint is a reasonable approach to prevent long names from breaking the table layout. The constraint is applied consistently across similar columns per the PR context.
/> | ||
<div> | ||
<div className="flex items-center gap-1"> | ||
<div className="flex w-full flex-col"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question for @znegrin: with this change, all the icons are pushed ot the very right, which on one hand aligns them, but on the other makes them a bit floating around. Wdyt?

There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Cahllagerfeld, I would keep them aligned with the pipeline name, as showing them there would miss the context and consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I think we need to add some shrink-prevention to the icons, as they seem to disappear if there is a truncate now. (See Screenshot)

I dont know so much about the the ch
unit in css to clearly say if its a good practice for truncates or not, but I think you know 👍
Do you have all the cases from the ticket covered with the current changes? It always tackles the first column of the tables
I think the icons should just be fixed with the same size always, as in the designs, and not change or disappear if the text varies, as it happens now. I also imagine it is even easier to have a fixed-size icon that always uses the |
Summary by CodeRabbit