Skip to content

Conversation

@maryhipp
Copy link
Contributor

@maryhipp maryhipp commented Aug 3, 2023

What type of PR is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix
  • Optimization
  • Documentation Update
  • Community Node Submission

Have you discussed this change with the InvokeAI team?

  • Yes
  • No, because:

Have you updated all relevant documentation?

  • Yes
  • No

Description

I'm not positive that I did this right - but I was running into issues locally where autoAddBoardId was null, which it seems is an unexpected state. It was getting sent as a board_id param when creating an upload in the "uncategorized" view. It seems that autoAddBoardId should always be defined, and that if a board is not selected, it should always be "none" - never null or undefined.

Related Tickets & Documents

  • Related Issue #
  • Closes #

QA Instructions, Screenshots, Recordings

Added/updated tests?

  • Yes
  • No : please replace this line with details on why tests
    have not been included

[optional] Are there any post deployment tasks we need to perform?

@maryhipp
Copy link
Contributor Author

maryhipp commented Aug 3, 2023

@KevinBrack - in case you want to take a look, may overlap with your recent work on the checkbox setting

@maryhipp
Copy link
Contributor Author

maryhipp commented Aug 3, 2023

This begs the bigger question - should autoAddToBoardId always be assigned? If you have this "off", isn't that the same as having it on and set to "none"?

@psychedelicious
Copy link
Contributor

You're right, it should never be null or undefined. Maybe you had leftover state from a prev version? I've also noticed some weirdness with hot module reloading and the RTK cache.

Originally I used undefined to mean "put the image in Uncategorized", but was getting mixed up in the board logic. Making it either "none" or the actual board ID made more sense to me, but if this is confusing we can change it back. Just need to be careful in the query definitions.

@psychedelicious
Copy link
Contributor

The remaining places where we are checking if the board id is falsy are from me missing those spots when rebasing the multi select pr. I'll do another pass and update this pr.

Copy link
Contributor

@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 for catching this andd hopefully autoAddBoardId is never not a string again

@psychedelicious psychedelicious enabled auto-merge (rebase) August 4, 2023 12:17
@psychedelicious psychedelicious merged commit cfc3a20 into main Aug 4, 2023
@psychedelicious psychedelicious deleted the maryhipp/autoAddBoardId-fix branch August 4, 2023 12:19
@maryhipp
Copy link
Contributor Author

maryhipp commented Aug 4, 2023

Definitely possible probable that my local storage could have something to do with it

@KevinBrack
Copy link
Contributor

NoBoardBoard.tsx was using board undefined here. When I set up auto add board on click I used the same syntax that was there. Is this supposed to be 'none'?

const NoBoardBoard = memo(({ isSelected }: Props) => {
  const dispatch = useAppDispatch();
  const { autoAddBoardId } = useAppSelector(selector);
  const { autoAddBoardId, autoAssignBoardOnClick, isProcessing } =
    useAppSelector(selector);
  const boardName = useBoardName(undefined);
  const handleSelectBoard = useCallback(() => {
    dispatch(boardIdSelected(undefined));
  }, [dispatch]);
    if (autoAssignBoardOnClick && !isProcessing) {
      dispatch(autoAddBoardIdChanged(undefined));
    }
  }, [dispatch, autoAssignBoardOnClick, isProcessing]);

@psychedelicious
Copy link
Contributor

@KevinBrack No, you're good - it was supposed to be undefined in your PR. I updated your code in NoBoardBoard's to use 'none' in the multi-select PR, which changed it to 'none', so that wasn't an issue

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.

4 participants