Skip to content

Conversation

@camielvs
Copy link
Collaborator

@camielvs camielvs commented Dec 2, 2025

Description

Move TaskDetails onto UI primitives & ContextPanel Blocks.

To simplify this and provide better code quality, the links and urls have been split out of Details into their own GithubDetails component, as has also been done with the action buttons -> Actions component.

Related Issue and Pull requests

https://github.com/Shopify/oasis-frontend/issues/401

Type of Change

  • Cleanup/Refactor

Checklist

  • I have tested this does not break current pipelines / runs functionality
  • I have tested the changes on staging

Screenshots (if applicable)

Before:

image.png

After:

image.png

Test Instructions

No change to app functionality. UI update only. Confirm that the interface works and shows info as expected.

Additional Comments

@camielvs camielvs force-pushed the 12-02-cleanup_taskdetails branch from cf86a9d to 6c22aea Compare December 2, 2025 23:54
@camielvs camielvs marked this pull request as ready for review December 2, 2025 23:56
@camielvs camielvs force-pushed the 12-02-cleanup_taskdetails branch from 6c22aea to f1fefa5 Compare December 3, 2025 20:35
@camielvs camielvs requested a review from Mbeaulne December 3, 2025 20:47
if (!hasExecutionData) {
return null;
}
} = useFetchContainerExecutionState(executionId, backendUrl);
Copy link
Collaborator

Choose a reason for hiding this comment

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

NIT: by using useSuspenseQuery and withSuspenseWrapper you can clean up skeleton and error logic.

useFetchContainerExecutionState seems to be used only in this component, so refactoring is straight forward.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The code here has changed a fair amount, so it's worth taking another look and seeing if this NIT is still relevant

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it is still relevant, not blocking though

Copy link
Collaborator

@maxy-shpfy maxy-shpfy left a comment

Choose a reason for hiding this comment

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

See some regression in Component Details Dialog.

Screen Recording 2025-12-03 at 4.52.59 PM.mov (uploaded via Graphite)

@camielvs camielvs mentioned this pull request Dec 4, 2025
3 tasks
@camielvs camielvs force-pushed the 12-02-cleanup_taskdetails branch from f1fefa5 to 17c6996 Compare December 4, 2025 21:26
@camielvs camielvs marked this pull request as draft December 4, 2025 21:44
@camielvs camielvs changed the base branch from master to graphite-base/1464 December 4, 2025 21:52
@camielvs camielvs force-pushed the 12-02-cleanup_taskdetails branch from 17c6996 to 748d25b Compare December 4, 2025 21:52
@camielvs camielvs changed the base branch from graphite-base/1464 to 12-04-add_ui_blocks_to_context_panel December 4, 2025 21:53
@camielvs camielvs mentioned this pull request Dec 4, 2025
4 tasks
@camielvs camielvs changed the base branch from 12-04-add_ui_blocks_to_context_panel to graphite-base/1464 December 4, 2025 21:59
@camielvs camielvs force-pushed the 12-02-cleanup_taskdetails branch from 748d25b to 4f787bc Compare December 4, 2025 21:59
@camielvs camielvs changed the base branch from graphite-base/1464 to 12-04-cleanup_executiondetails December 4, 2025 21:59
@camielvs camielvs mentioned this pull request Dec 4, 2025
3 tasks
@camielvs camielvs force-pushed the 12-02-cleanup_taskdetails branch from 4f787bc to ef11837 Compare December 4, 2025 22:29
@camielvs camielvs force-pushed the 12-04-cleanup_executiondetails branch from 49d4edd to 533c465 Compare December 4, 2025 22:44
@camielvs camielvs force-pushed the 12-02-cleanup_taskdetails branch from ef11837 to 04c9d4b Compare December 4, 2025 22:44
@camielvs camielvs force-pushed the 12-04-cleanup_executiondetails branch from 80a0367 to 8aed7ce Compare December 17, 2025 20:32
@camielvs camielvs force-pushed the 12-02-cleanup_taskdetails branch from 8d6e5bd to 9f12c4f Compare December 17, 2025 20:32
@camielvs camielvs force-pushed the 12-04-cleanup_executiondetails branch from 8aed7ce to da919d7 Compare December 17, 2025 20:34
@camielvs camielvs force-pushed the 12-02-cleanup_taskdetails branch from 9f12c4f to 5257308 Compare December 17, 2025 20:34
@camielvs camielvs force-pushed the 12-04-cleanup_executiondetails branch from da919d7 to d36c503 Compare December 18, 2025 16:03
@camielvs camielvs force-pushed the 12-02-cleanup_taskdetails branch from 5257308 to 5231971 Compare December 18, 2025 16:03
@camielvs camielvs force-pushed the 12-04-cleanup_executiondetails branch from d36c503 to 6630e43 Compare December 18, 2025 16:10
@camielvs camielvs force-pushed the 12-02-cleanup_taskdetails branch from 5231971 to 2a594fc Compare December 18, 2025 16:10
Copy link
Collaborator

@maxy-shpfy maxy-shpfy left a comment

Choose a reason for hiding this comment

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

Screen Recording 2025-12-18 at 8.37.56 AM.mov (uploaded via Graphite)

I see a problem with

return <div key={JSON.stringify(action)}>{action}</div>;

in ActionBlock

Comment on lines +133 to +140
const downloadYamlFromComponentText = (
componentSpec: ComponentSpec,
displayName: string,
) => {
const code = componentSpecToText(componentSpec);
downloadStringAsFile(code, `${displayName}.yaml`, "text/yaml");
};

Copy link
Collaborator

Choose a reason for hiding this comment

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

does this exist somewhere else?

Copy link
Collaborator Author

@camielvs camielvs Dec 18, 2025

Choose a reason for hiding this comment

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

I don't think so? But you may be confused because iirc one of @maxy-shpfy's open PRs also implements a similar method.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not aware of similar methods. I dont see any bad signals coming from this method

@camielvs camielvs force-pushed the 12-02-cleanup_taskdetails branch from 2a594fc to 8633795 Compare December 18, 2025 19:28
@camielvs camielvs force-pushed the 12-04-cleanup_executiondetails branch from 6630e43 to 87a8b15 Compare December 18, 2025 19:28
@camielvs camielvs force-pushed the 12-02-cleanup_taskdetails branch from 8633795 to a14d94e Compare December 18, 2025 19:34
Copy link
Collaborator Author

^ I think I have fixed this - let me know if not!

Copy link
Collaborator

@maxy-shpfy maxy-shpfy left a comment

Choose a reason for hiding this comment

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

Now it LGTM!

@camielvs camielvs dismissed Mbeaulne’s stale review December 18, 2025 20:33

agreed to proceed in dev sync

@camielvs camielvs force-pushed the 12-04-cleanup_executiondetails branch 2 times, most recently from 0bf0e98 to 9c9af85 Compare December 18, 2025 20:35
@camielvs camielvs force-pushed the 12-02-cleanup_taskdetails branch from a14d94e to 1bdd5d3 Compare December 18, 2025 20:35
@camielvs camielvs changed the base branch from 12-04-cleanup_executiondetails to graphite-base/1464 December 18, 2025 20:42
@camielvs camielvs force-pushed the 12-02-cleanup_taskdetails branch from 1bdd5d3 to 540b3d3 Compare December 18, 2025 20:42
@graphite-app graphite-app bot changed the base branch from graphite-base/1464 to master December 18, 2025 20:42
@camielvs camielvs force-pushed the 12-02-cleanup_taskdetails branch from 540b3d3 to 2d25276 Compare December 18, 2025 20:43
Copy link
Collaborator Author

camielvs commented Dec 18, 2025

Merge activity

  • Dec 18, 8:48 PM UTC: A user started a stack merge that includes this pull request via Graphite.
  • Dec 18, 8:48 PM UTC: @camielvs merged this pull request with Graphite.

@camielvs camielvs merged commit 41e0814 into master Dec 18, 2025
7 of 8 checks passed
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.

5 participants