Skip to content

Conversation

mickr777
Copy link
Contributor

@mickr777 mickr777 commented Jul 13, 2023

Adds a Clear Nodes Button with Confirmation Dialog, I think I Did it right 😃

I am sure there is a way to make the Confirmation look better and have Yes/No instead of OK/Cancel

Changed Confirmation Dialog to use chakra-ui alert dialog
@blessedcoolant blessedcoolant self-requested a review July 13, 2023 08:35
@blessedcoolant blessedcoolant merged commit fde9fa2 into invoke-ai:main Jul 13, 2023
@mickr777 mickr777 deleted the clearnodes branch July 13, 2023 08:54
Copy link
Collaborator

@psychedelicious psychedelicious left a comment

Choose a reason for hiding this comment

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

Thanks @mickr777 !

I've made two small requested changes and explained the reasoning for them.

Comment on lines +136 to +139
clearNodes: (state) => {
state.nodes = [];
state.edges = [];
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you please:

  • replace the faulty logic in the nodeEditorReset reducer (just a few lines above this one) with the logic you implemented, for example:
    nodeEditorReset: () => {
      state.nodes = [];
      state.edges = [];
    },
  • dispatch nodeEditorReset in the component instead of clearNodes
  • remove the clearNodes action entirely

Reasoning:
Redux actions are more like "events" (they shouldn't have been named actions). When you dispatch a redux action, think of it as a signal that an event just occurred, and throughout the app, any number of handlers can listen for that event and respond to it.

clearNodes is imperative - it's telling the app to do something - but it doesn't make sense to respond to an imperative directive by taking different actions.

So instead, we prefer to model this like an event that happened:

  • What happened? The user reset the node editor
  • How do we respond to that event? By settings the nodes and edges to empty arrays, thus resetting the node editor

Elsewhere in the app, we may want to add more responses to this nodeEditorReset event.

For example, if we want to display a toast (as you've done), we might choose to handle this in the systemSlice, which has a toastQueue array. We could add a "listener" in that slice that triggers on the nodeEditorReset action, pushing a new toast object onto the toastQueue, which would then be displayed. Don't worry about changing that, though - just mentioned as an example of how we might want to respond to a redux action in multiple places.

Admittedly, we haven't adhered to the naming convention very well. We name many actions imperatively.


The actual logic in nodeEditorReset is flawed. It actually resets more than just the nodes and edges, I think I had that in there in the early days while I needed an "oh shit" button. The logic in clearNodes is what should happen in nodeEditorReset.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I merged the PR in already but if @mickr777 can open a new PR with these changes, I'll merge that up. If not, I'll do one myself later. Either is fine.

Comment on lines +30 to +43
const handleConfirmClear = () => {
dispatch(clearNodes());

dispatch(
addToast(
makeToast({
title: t('toast.nodesCleared'),
status: 'success',
})
)
);

onClose();
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please wrap this function in a useCallback, for example:

  const handleConfirmClear = useCallback(() => {
    dispatch(clearNodes());

    dispatch(
      addToast(
        makeToast({
          title: t('toast.nodesCleared'),
          status: 'success',
        })
      )
    );

    onClose();
  }, [dispatch, onClose, t]);

Reasoning:
Every time this component re-renders, the code in the component is run again. The handleConfirmClear function will be recreated every time.

In simpler applications (like a website), this is no big deal, but we have a ton of stuff happening, and need to be as efficient as possible. Every millisecond counts.

By wrapping the function in useCallback, we tell react to not recreate this function on every render - it's maintained with a stable reference in memory. Each render then is slightly less resource-intensive.

The dependency array [dispatch, onClose, t] tells react that whenever one of those things changes, it should recreate the function, so it doesn't get stale. It's important to ensure that array is correct. VSCode gives me red squigglies if it's not correct, hopefully it does for you too.

Copy link
Contributor Author

@mickr777 mickr777 Jul 13, 2023

Choose a reason for hiding this comment

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

Ok, how do reopen branch that was deleted or will i need to start a new pr

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.

3 participants