Skip to content

fix(CatalogDesignTable): pass design ID as third argument to handleCopyUrl for backward compatibility #1477

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

Merged
Rajesh-Nagarajan-11 merged 2 commits into from
May 6, 2026
+2 −2
Merged

fix(CatalogDesignTable): pass design ID as third argument to handleCopyUrl for backward compatibility #1477

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
083e481
Fix Copy Link URL parameter signature
rishiraj38 May 4, 2026
707f29b
Merge branch 'master' into fix-copy-link-url
saurabhraghuvanshii May 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/custom/CatalogDesignTable/columnConfig.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ export const colViews: ColView[] = [
interface ColumnConfigProps {
handleShowDetails: (design: Pattern) => void;
handleClone: (designId: string, name: string) => void;
handleCopyUrl: (designId: string, name: string) => void;
handleCopyUrl: (designId: string, name: string, id?: string) => void;
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot May 4, 2026

Choose a reason for hiding this comment

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

medium

The parameter naming (designId: string, name: string, id?: string) is confusing as it uses two different names for the same entity to support legacy signatures. Renaming them to reflect their actual roles (e.g., idOrSource for the first and designId for the third) would improve code readability and maintainability.

Suggested change
handleCopyUrl: (designId: string, name: string, id?: string) => void;
handleCopyUrl: (idOrSource: string, name: string, designId?: string) => void;

Copy link
Copy Markdown
Member

@saurabhraghuvanshii saurabhraghuvanshii May 4, 2026

Choose a reason for hiding this comment

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

@rishiraj38 can you check this?

handleOpenPlayground: (designId: string, name: string) => void;
handleUnpublish?: (design: Pattern) => void;
maxWidth?: boolean;
Expand Down Expand Up @@ -263,7 +263,7 @@ export const createDesignColumns = ({
},
{
title: 'Copy Link',
onClick: () => handleCopyUrl(rowData.id, rowData.name),
onClick: () => handleCopyUrl(rowData.id, rowData.name, rowData.id),
Copy link
Copy Markdown
Member

@saurabhraghuvanshii saurabhraghuvanshii May 4, 2026

Choose a reason for hiding this comment

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

@rishiraj38 why two time row.Data.id ?

Copy link
Copy Markdown
Member Author

@rishiraj38 rishiraj38 May 4, 2026

Choose a reason for hiding this comment

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

The MeshMap extension is still using an older signature that expects the ID as the 3rd argument (type, name, id). Because Sistent was only passing (id, name), that 3rd argument was ending up as undefined, which caused the bug. Passing the ID in both the 1st and 3rd spots fixes the bug for the extension while keeping it backward compatible with anything else that relies on the 1st argument.

Copy link
Copy Markdown
Member Author

@rishiraj38 rishiraj38 May 4, 2026

Choose a reason for hiding this comment

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

The ideal way to fix this would be updating the MeshMap extension directly to just read the ID from the 1st argument. But since the extension lives in a separate, private repo, doing the fix here in Sistent makes sure it works right away without waiting on another PR. Plus, if the extension ever gets updated to use the 1st argument instead of the 3rd, this change handles both cases, so it won't break again.

Copy link
Copy Markdown
Member Author

@rishiraj38 rishiraj38 May 4, 2026

Choose a reason for hiding this comment

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

https://github.com/layer5io/sistent/blob/master/src/custom/CatalogDesignTable/DesignTableColumnConfig.tsx#L29

Copy link
Copy Markdown
Member

@saurabhraghuvanshii saurabhraghuvanshii May 4, 2026

Choose a reason for hiding this comment

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

Thanks @rishiraj38 lgtm!!

icon: <ChainIcon width={'24'} height={'24'} fill={theme.palette.text.primary} />
},
{
Expand Down
Loading