Skip to content
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

fix(dashboard): nested parents #1317

Merged
merged 7 commits into from
Feb 19, 2025
Merged

fix(dashboard): nested parents #1317

merged 7 commits into from
Feb 19, 2025

Conversation

bryson-g
Copy link
Contributor

No description provided.

@bryson-g bryson-g added the bug An issue with correctness, stability, performance, or API conformance. label Feb 19, 2025
@bryson-g bryson-g requested a review from mijailr February 19, 2025 18:40
@bryson-g bryson-g self-assigned this Feb 19, 2025
id: ids[0],
}
}
const wfRunId: WfRunId = ids.reduce((wfRunId, id, i) => (i === 0 ? { id } : { id, parentWfRunId: wfRunId }), {} as WfRunId);
Copy link
Contributor

Choose a reason for hiding this comment

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

The list must be reversed, please use the previous implementation.

Suggested change
const wfRunId: WfRunId = ids.reduce((wfRunId, id, i) => (i === 0 ? { id } : { id, parentWfRunId: wfRunId }), {} as WfRunId);
const wfRunId = ids
.reverse()
.reduceRight<WfRunId | undefined>((parentWfRunId, id) => ({ id, parentWfRunId }), undefined)

Also, this should be done in the calling function. you can pass the string array from this page, so the receiver has the logic to generate the valid WfRunId.

Copy link
Contributor Author

@bryson-g bryson-g Feb 19, 2025

Choose a reason for hiding this comment

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

The previous implementation negates the reverse by immediately after doing reduceRight.
reverse().reduceRight() is the same as reduce.
https://stackoverflow.com/a/20753610

Additionally, the old implementation doesn't work in this scenario because the initial value for the reduce function becomes improperly inserted in the first index. This is why the new implementation uses the i === 0 conditional.

Since this is a server component, I do agree that it should be ran in the client component. I'll make that change.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, this should be done in the calling function. you can pass the string array from this page, so the receiver has the logic to generate the valid WfRunId.

This is not mandatory, having it here is fine as well. It's just a preference. If we use the WfRunId in the component for multiple purposes, then we should have a utility function.

Copy link
Contributor Author

@bryson-g bryson-g Feb 19, 2025

Choose a reason for hiding this comment

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

Also, this should be done in the calling function. you can pass the string array from this page, so the receiver has the logic to generate the valid WfRunId.

This is not mandatory, having it here is fine as well. It's just a preference. If we use the WfRunId in the component for multiple purposes, then we should have a utility function.

I think it's better practice to include it in the client component as you suggest, so we avoid potentially slowing down the server with any unnecessary operations (even though this operation is so simple that it would never slow anything down here, it's just good practice I suppose 😂 ).

@bryson-g bryson-g requested a review from mijailr February 19, 2025 19:18
@bryson-g bryson-g merged commit 1f4129d into master Feb 19, 2025
24 checks passed
@bryson-g bryson-g deleted the fix-nested-parents branch February 19, 2025 20:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An issue with correctness, stability, performance, or API conformance.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants