Skip to content

Conversation

@tzador
Copy link
Collaborator

@tzador tzador commented Oct 16, 2025

No description provided.

@tzador tzador mentioned this pull request Oct 16, 2025
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Addresses bug/816 focused on improving handling of long names and UX/navigation, while introducing global Snapshots and Deployments views and unifying timestamp parsing.

  • Adds global Snapshots and Deployments routes/pages and top-level project tab navigation
  • Centralizes timestamp parsing via prepareBackendTimestamp and refactors components to use it
  • Refactors list tables (Deployments/Snapshots) to accept column definitions and introduces shared column factories; updates destructive action dropdowns to use a unified Delete dialog; adds a header Back button

Reviewed Changes

Copilot reviewed 54 out of 59 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
src/router/routes.tsx Adds overview routes for snapshots and deployments under projects.
src/router/Router.tsx Registers new lazy-loaded routes for global snapshots/deployments pages.
src/lib/timeline/mapping.ts Uses prepareBackendTimestamp for robust date parsing in timeline.
src/lib/dates.ts Introduces prepareBackendTimestamp to normalize backend timestamps.
src/lib/bulk-delete.tsx Improves bulk delete error toasts to show specific failures per item.
src/layouts/project-tabs/tabs.tsx Adds Snapshots and Deployments tabs and navigation handlers.
src/layouts/AuthenticatedLayout/back-button.tsx Adds a back button to the authenticated header.
src/layouts/AuthenticatedLayout/AuthenticatedHeader.tsx Integrates BackButton and reorders breadcrumb imports.
src/components/service-connectors/expiry.tsx Uses prepareBackendTimestamp for expiry calculations.
src/components/pipeline-snapshots/table-actions.tsx Renames action component to PipelineSnapshotActions.
src/components/pipeline-snapshots/list/use-queryparams.ts Renames hook to useSnapshotListQueryParams.
src/components/pipeline-snapshots/list/table.tsx Table now accepts external columns; pagination and selection unchanged.
src/components/pipeline-snapshots/list/column-definitions.tsx New shared snapshot column factories used across pages.
src/components/logs/log-line.tsx Normalizes timestamp parsing and switches display formatting.
src/components/deployments/list/use-deployment-queryparams.ts Renames hook to useDeploymentQueryParams.
src/components/deployments/list/table.tsx Table now accepts external columns.
src/components/deployments/list/column-definitions.tsx New shared deployment column factories.
src/components/breadcrumbs/library.ts Enables breadcrumb links for Deployments/Snapshots.
src/components/DisplayDate.tsx Uses prepareBackendTimestamp for consistent date handling.
src/components/DeleteAlertDialog.tsx Allows disabling Delete during pending state.
src/components/dialog/DialogItem.tsx Removes obsolete dialog helper.
src/components/AlertDialogDropdownItem.tsx Removes obsolete alert dialog menu item helper.
src/app/snapshots/** Adds global snapshots page and columns.
src/app/pipelines/[pipelineId]/snapshots/** Adopts shared list components and columns.
src/app/pipelines/[pipelineId]/runs/columns.tsx Adjusts run name cell layout for long names.
src/app/pipelines/_components/columns.tsx Improves layout and widths to handle long pipeline names.
src/app/pipelines/[pipelineId]/deployments/** Adopts shared deployment list components/columns.
src/app/deployments/** Adds global deployments page and columns.
src/app/settings/** Unifies delete/edit dropdowns to use shared DeleteAlertContent with proper dialogs; uses prepareBackendTimestamp for age checks.
package.json Updates vite version.
CLAUDE.md, AGENTS.md, .coderabbit.yaml Adds tooling guidance and repository agent configs (docs only).
Files not reviewed (1)
  • pnpm-lock.yaml: Language not supported

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines 61 to 69
export function prepareBackendTimestamp(dateString: string | number) {
if (typeof dateString === "number") {
return new Date(dateString);
}
if (!dateString.endsWith("Z")) {
return new Date(dateString + "Z");
}
return new Date(dateString);
}
Copy link

Copilot AI Oct 16, 2025

Choose a reason for hiding this comment

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

Appending 'Z' whenever the string doesn't end with 'Z' breaks timestamps that already include an explicit timezone offset (e.g. '+01:00'), producing invalid strings like '...+01:00Z'. Only append 'Z' when the input has no timezone information.

Copilot uses AI. Check for mistakes.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is intended. The backend is controlled by us, and retrieves the dates in UTC, but sometimes the Z is missing. We know for sure though that its utc

@Cahllagerfeld Cahllagerfeld changed the base branch from main to staging October 16, 2025 07:09
Copy link
Contributor

@Cahllagerfeld Cahllagerfeld left a comment

Choose a reason for hiding this comment

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

Bildschirmfoto 2025-10-16 um 10 22 23

Nice 👍 The truncate works better now

Bildschirmfoto 2025-10-16 um 10 21 58

There still seems to be something off. For smaller sizes the columns (here the actions) overflow with other columns.

I wonder if we should use width, with a min-width or so, and apply this to all tables.
Another option I would see is using a fixed table layout

Wdyt? It may also be needed for other tables I assume

Copy link
Contributor

@Cahllagerfeld Cahllagerfeld left a comment

Choose a reason for hiding this comment

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

There are still tables missing:

  • Deployments
  • users
  • secrets
  • ...

just to name some

Q: Right now the max-w is not applied to all columns, but most of the time only to the name column. Should we add these max-widths to all columns or is it fine if we just add it to some?

id: "select",
meta: {
width: "1%"
width: "2rem"
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not consistent with the, rest there are still occurencies with 1%

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

actually it depends on how many other columns are there. we dont want max-w too small when there is few other columsn

Copy link
Contributor

Choose a reason for hiding this comment

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

also notice that these columns are likely to be resizable in a -hopefully- near future, so it would be nice to have this in mind.

id: "name",
header: "Run",
meta: {
className: "max-w-[100ch]"
Copy link
Contributor

Choose a reason for hiding this comment

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

Sometimes its 20rem, here its 100ch, it should be consistent imo.

@htahir1 htahir1 linked an issue Oct 20, 2025 that may be closed by this pull request
Copy link
Contributor

@Cahllagerfeld Cahllagerfeld left a comment

Choose a reason for hiding this comment

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

I think there are still tables missing, see my comment earlier

@tzador
Copy link
Collaborator Author

tzador commented Oct 20, 2025

I think there are still tables missing, see my comment earlier

we add it to the name because name is truncatable and for that to work it need max width

@Cahllagerfeld
Copy link
Contributor

we add it to the name because name is truncatable and for that to work it need max width

Im not sure if I got it right, but I was referring to other tables, for example the users-table, or the secrets table etc... Where the truncate behavior also needs to be added to the name column.

@tzador
Copy link
Collaborator Author

tzador commented Oct 20, 2025

we add it to the name because name is truncatable and for that to work it need max width

Im not sure if I got it right, but I was referring to other tables, for example the users-table, or the secrets table etc... Where the truncate behavior also needs to be added to the name column.

applied the truncating component to the rest of places

@Cahllagerfeld
Copy link
Contributor

Did you check all the places? I think there are still some missing, e.g members, service-accounts, api-keys 🤔

Copy link
Contributor

@Cahllagerfeld Cahllagerfeld left a comment

Choose a reason for hiding this comment

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

  • src/app/members/columns.tsx
  • src/app/settings/secrets/[id]/columns.tsx - this needs to be tested
  • src/app/settings/service-accounts/columns.tsx
  • src/app/settings/service-accounts/[service-account-id]/columns.tsx

these are the ones I just found

@tzador tzador requested a review from Cahllagerfeld October 24, 2025 12:40
Copy link
Contributor

@Cahllagerfeld Cahllagerfeld left a comment

Choose a reason for hiding this comment

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

There are still some small inconsistencies, but I think the crucial part with adjusting the Component, to make things optional is fine 👍

}
topCopy={name}
bottomLink={routes.components.detail(id)}
bottomName={getFirstUuidSegment(id)}
Copy link
Contributor

Choose a reason for hiding this comment

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

The component sheet also needs to be added to the bottom line like it was originally

className="shrink-0"
/>
}
topLink={routes.components.detail(id)}
Copy link
Contributor

Choose a reason for hiding this comment

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

In order to make it work with the new component, you will need to remove the link here, otherwise it will still render as a link

@tzador tzador requested a review from Cahllagerfeld October 27, 2025 09:18
Copy link
Contributor

@Cahllagerfeld Cahllagerfeld left a comment

Choose a reason for hiding this comment

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

I hope the last ones

bottomLink={routes.components.detail(id)}
bottomName={getFirstUuidSegment(id)}
bottomName={
<ComponentSheet componentId={id}>
Copy link
Contributor

Choose a reason for hiding this comment

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

grafik

There is an issue now with the icons, that are looking squeezed

topName={
<StackSheet stackName={name} stackId={id}>
<h2 className="text-text-md font-semibold">{name}</h2>
<h2 className="truncate text-text-md font-semibold">{name}</h2>
Copy link
Contributor

@Cahllagerfeld Cahllagerfeld Oct 27, 2025

Choose a reason for hiding this comment

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

There seems to be an error now logged in this list

validateDOMNesting(...): <h2> cannot appear as a descendant of <p>. Component Stack: 
    h2 unknown:0

Could oyu please check for these errors. I havent checked all the places for this error though

@tzador tzador requested a review from Cahllagerfeld October 27, 2025 15:40
@tzador tzador force-pushed the bug/816-long-names branch from c021e67 to 3976eb2 Compare October 31, 2025 08:15
Copy link
Contributor

@Cahllagerfeld Cahllagerfeld left a comment

Choose a reason for hiding this comment

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

The new approach works really well 👍 👍

I think one last request:
We should maybe add a min-width so it doesnt truncate to just one letter, but besides that it looks good 🎉

grafik

@Cahllagerfeld Cahllagerfeld merged commit a8d1652 into staging Nov 5, 2025
4 checks passed
@Cahllagerfeld Cahllagerfeld deleted the bug/816-long-names branch November 5, 2025 09:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Issues with long name strings

4 participants