Skip to content

Conversation

@maxy-shpfy
Copy link
Collaborator

@maxy-shpfy maxy-shpfy commented Aug 16, 2025

Description

Closes https://github.com/Shopify/oasis-frontend/issues/202

This PR improves performance and code quality through several optimizations:

  1. Removed unnecessary isDragging check in TaskNodeCard that was causing performance issues
  2. Optimized the useToastNotification hook with useCallback to prevent unnecessary re-renders
  3. Fixed a memory leak in ContextPanelProvider by using a ref for defaultContent
  4. Refactored TaskNodeProvider to extract callbacks from data with a fallback to default no-op functions

Type of Change

  • Improvement
  • Cleanup/Refactor
  • Bug fix

Checklist

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

Test Instructions

  1. Verify task nodes can still be selected and manipulated without issues
  2. Check that toast notifications work properly
  3. Confirm context panel content updates correctly when selecting nodes
  4. Verify task node callbacks (delete, duplicate, upgrade) function as expected

Screen Recording 2025-12-04 at 3.33.30 PM.mov (uploaded via Graphite)

Copy link
Collaborator Author

maxy-shpfy commented Aug 16, 2025

This stack of pull requests is managed by Graphite. Learn more about stacking.

children: ReactNode;
}) => {
const { setNodes } = useReactFlow();
const defaultContentRef = useRef<ReactNode>(defaultContent);
Copy link
Collaborator Author

@maxy-shpfy maxy-shpfy Aug 16, 2025

Choose a reason for hiding this comment

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

This ref is needed to make "clearContent" stable. Another approach to try - is to extract the piece of JSX in PipelineEditor and other places, where we using <ContextPanelProvider>.

const pipelineDetails = useMemo(() => <PipelineDetails />, []);

...

<ContextPanelProvider defaultContent={pipelineDetails}>

@maxy-shpfy maxy-shpfy force-pushed the 08-16-fix_context_panel_on_node_dragging branch 3 times, most recently from a23bf03 to 39e01e3 Compare December 4, 2025 23:36
@maxy-shpfy maxy-shpfy marked this pull request as ready for review December 4, 2025 23:37
setArguments,
setAnnotations,
setCacheStaleness,
} = data.callbacks ?? DEFAULT_TASK_NODE_CALLBACKS;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We need stable references

type,
});
};
const notify = useCallback(
Copy link
Collaborator

Choose a reason for hiding this comment

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

iirc we're running the latest React and we should no longer need to explicitly call useCallback?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In case if we use Compiler. But I dont think we enabled it yet

Copy link
Collaborator

@camielvs camielvs left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator Author

maxy-shpfy commented Dec 5, 2025

Merge activity

  • Dec 5, 1:19 AM UTC: A user started a stack merge that includes this pull request via Graphite.
  • Dec 5, 1:19 AM UTC: Graphite rebased this pull request as part of a merge.
  • Dec 5, 1:21 AM UTC: @maxy-shpfy merged this pull request with Graphite.

@maxy-shpfy maxy-shpfy force-pushed the 08-16-fix_context_panel_on_node_dragging branch from 39e01e3 to fe5234a Compare December 5, 2025 01:19
@maxy-shpfy maxy-shpfy merged commit c424d9a into master Dec 5, 2025
5 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.

2 participants