[JIMT] dance ai modal refactoring part 2 by thomasoniii · Pull Request #55192 · code-dot-org/code-dot-org

thomasoniii · 2023-11-28T15:46:09Z

This is about a day's worth of work, so seems like a good place to stop and get it in.

Re-arranges a bunch of stuff, in a lot of interlocking ways that makes it difficult to tease apart into multiple PRs. I'd rather just send in a single big one now which has most/all of the teasing apart done in place and then we're in better shape for smaller ones going forward. Otherwise this'll be a rapid fire dozen or so.

Anyway, this includes, at a minimum:

broke apart DanceAiClient into separate utils/- files/functions
moved DanceAiModal/getPreviewCode into separate file
moved EmojiIcon into separate component
created DanceAiEmojiIcon wrapper component
created DanceAiModalHeader wrapper component
created DanceAiModalFlipCard wrapper component
created new DanceAiModalEmojiInputGrid component
DanceAiModal/getItem is a memoized function using an itemsById cached lookup
changed useInputs variable to myInputs
simplified return logic in calculateMinMax
retooled handleItemClick to remove currentInputSlot dependency - big render perf boost
added selectedEmojis helper data (inputs.map(getItem))
moved getItem to a separate utils/file
removed superfluous keys`
added lots of memoization
added lots of callbacks

Notes in the code. Let's meet about this one on a call.

Links

Testing story

Deployment strategy

Follow-up work

Privacy

Security

Caching

PR Checklist:

Tests provide adequate coverage
Privacy and Security impacts have been assessed
Code is well-commented
New features are translatable or updates will not break translations
Relevant documentation has been added or updated
User impact is well-understood and desirable
Pull Request is labeled appropriately
Follow-up work items (including potential tech debt) are tracked and linked

…callbacks

…own file

…seMemos

… made headerValue a memo

…dded memoization

* broke apart DanceAiClient into separate utils/* files/functions * moved DanceAiModal/getPreviewCode into separate file * moved EmojiIcon into separate component * created DanceAiEmojiIcon wrapper component * created DanceAiModalHeader wrapper component * created DanceAiModalFlipCard wrapper component * DanceAiModal/getItem is a memoized function using an itemsById cached lookup * changed `useInputs` variable to `myInputs` * simplified return logic in calculateMinMax * retooled handleItemClick to remove currentInputSlot dependency - big render perf boost * allItems list is now emojiInputGrid; contains all output needed (NOTE - may go into separate component) * added selectedEmojis helper data (inputs.map(getItem))

…utGrid component

…e file

thomasoniii · 2023-11-28T15:46:29Z

+    <div
+      id={id}
+      style={{width: size, height: size}}
+      className={moduleStyles.previewVisualization}
+      ref={containerRef}
+    />


This is just an extra <div> wrapper, which we don't need AFAIK.

thomasoniii · 2023-11-28T15:48:53Z

+  );
+};
+
+export default React.memo(DanceAiEmojiIcon);


this component is a wrapper around the more generic EmojiIcon.

Each DanceAiEmojiIcon gets a custom click handler which is passed the item's id and its availability.

This way we can memoize the component and render properly. Otherwise we'd be creating a new click handler every render, and need to re-draw.

mid-long term, this could be replaced by a new click handler which doesn't pass along the isItemAvailable flag (it can be looked up from the selectedEmojis and then gets the ID from a data-tag. But we can circle back to that.

This is super cool. And thanks for the tip about using the Profiler and checking the 'Highlight updates when components render' box!

thomasoniii · 2023-11-28T15:50:02Z

  generateAiEffectBlocksFromResult,
-  generatePreviewCode,
-  getEmojiImageUrl,
+  getItem,


This should likewise be renamed. getEmoji? getEmojiItem?

I agree that getItem is vague. getEmoji seems clear and concise.

This comment was out of date - it was already moved over to getDanceAiModalItem

In general, I'm curious why we're using 'item' vs 'emoji' or 'input'.

We had a bunch of terms in the code initially, and they haven't been totally cleaned up and standardized. But, what I've been roughly working towards:

inputs are emoji ids

DanceAiModelItem are the values loaded out of the ai-inputs.json file (previously just item)

emoji is overloaded in that it could be thought of as the human readable string, the actual image, or a DanceAiModelItem.

Thanks! This is helpful and makes sense.
I see we are using selectedEmojis. Would it make sense to call these selectedDanceAiModalItems?

thomasoniii · 2023-11-28T15:50:31Z


  const [mode, setMode] = useState(Mode.INITIAL);
-  const [currentInputSlot, setCurrentInputSlot] = useState(0);
+


this is now just looked up from the inputs.length down below.

thomasoniii · 2023-11-28T15:50:55Z

+    () => inputs.map(getItem).filter(item => item !== undefined),
+    [inputs]
+  );
+


by memoizing selectedEmojis we can save a few render steps + not need to look up the item every time.

thomasoniii · 2023-11-28T15:51:20Z

-    (useInputs: string[], step: number) => {
+    (myInputs: string[], step: number) => {


useInputs as a variable looks like a hook, which it isn't.

changed to myInputs to still be local, but not conflicting.

I don't think we talk about my in most variables.. how about currentInputs?

I want that variable to emphasize that it's local to those functions and not state in the modal. currentInputs still feels ambiguous (especially with the other discussion about previousInputs vs currentInputs). A my prefix emphasizes it nicely.

What about givenInputs?

thomasoniii · 2023-11-28T15:51:58Z

+      ).reduce((accumulator: number, step: number) => {
+        const scores = getScores(myInputs, step);
+        return Math.min(...scores, accumulator);


This is just a logic simplification. It's returning the minimum of the previous value + all of the scores.

We could even remove the scores variable, but I felt it was more clear to keep.

thomasoniii · 2023-11-28T15:52:25Z

+      const previousInputString = currentAiModalField?.getValue();
+      if (previousInputString) {
+        const previousInput: AiFieldValue = JSON.parse(previousInputString);
+


currentValues is pretty vague, so I changed it to the more descriptive seeming previousInput.

It's still the current value unless it's overwritten.

+1 Brendan's comment.

Yeah, but it's also not actually a current value in the modal. It's the value that the modal reads from the blockly block when it's opened up, and that is then used to populate the inputs field of the modal, and then that becomes the actual current inputs value.

It's used to create inputs which are the current ones, but they haven't become current yet.

What about savedValueString?

how about initialValueString/ initialInput? Emphasizes more that it's the value that's going into the modal.

thomasoniii · 2023-11-28T15:53:37Z

  }, [currentAiModalField, mode, calculateMinMax]);

-  const getLabels = () => {
+  const labels = useMemo(() => {


The getLabels function was only called once. This is better written as a useMemo so we can calculate the value once and leave it alone w/o changing via re-render or a single function call later.

Also a good candidate to re-write into a separate hook in the future.

@bencodeorg I remember when you initially added this, you had run into some weird side effects from using useMemo, right? It sounds like those dependency issues may have gotten already solved along the way, but just tagging you in case there was something else we're missing here.

Ahh yes, was it that issue where the preview wouldn't show up blank intermittently if you re-opened the modal after generating an effect? That might have even been before we switched to live preview though...

thomasoniii · 2023-11-28T15:54:16Z

-  const getItem = (id: string) =>
-    inputLibraryJson.items.find(item => item.id === id);


This function was moved to utils/getDanceAiModalItem and optimized.

Notes are in that function.

thomasoniii · 2023-11-28T15:56:56Z

+    (id: string, available: boolean) => {
+      setInputs(inputs => {
+        // if we've clicked on an emoji that's not available, remove it from our inputs
+        if (!available) {
+          playSound('ai-deselect-emoji');
+          return inputs.filter(input => input !== id);
+          // if we have more input slots, we can add the emoji
+        } else if (inputs.length < SLOT_COUNT) {
+          playSound('ai-select-emoji');
+          setInputAddCount(inputAddCount => inputAddCount + 1);
+          return [...inputs, id];
+          // otherwise, it's not a remove and we don't have space - do nothing.
+        } else {
+          return inputs;
+        }
+      });
+    },
+    [playSound]


This needed a logic change to prevent a re-render.

handleItemClick was being recreated every render, which was causing all components that depended upon it to re-render (every emoji in this case). It meant that if you clicked on an emoji, then all of the emoji would redraw.

First step was to make it a callback, but it still would've had a dependency on the currentInputSlot, which still changed on every click and would trigger all the re-renders again.

Instead, it relies upon logic inside the setInputs mutator. This way, it can read the inputs state from within the mutator and behave as appropriate. currentInputSlot is not used as state any more (it's calculated from the inputs.length) so we don't need to worry about it. It only removes the item if necessary or adds it, and plays the sound.

Finally, if we're not adding or removing (meaning all slots are filled and we clicked on a new emoji), then just return the existing list of inputs for no change and no render.

thomasoniii · 2023-11-28T15:58:52Z

-  const handleGenerateClick = () => {
+  const handleGenerateClick = useCallback(() => {


All functions should be in useCallback, especially event handlers, so as to prevent needless re-renders.

thomasoniii · 2023-11-28T15:59:11Z

-      // Remove item from inputs.
-      setInputs(inputs.filter(input => input !== id));
-      setCurrentInputSlot(currentInputSlot - 1);
+  const startAi = useCallback(() => {


github got really confused here. But this function is the same as before, just in a callback.

thomasoniii · 2023-11-28T16:01:44Z

    setGeneratedProgress(0);
    setMode(Mode.SELECT_INPUTS);
-  };
+  }, [inputs]);


These extra handlers here would trigger re-renders every time the inputs change, but fortunately they're only attached to buttons that appear after all inputs have come into place. So we can get away with leaving them as is.

This could actually be dealt with via the same trick of hopping into the setInputs mutator as such:

setInputs(inputs => analyticsReporter.sendEvent(EVENTS.DANCE_PARTY_AI_BACKGROUND_RESTARTED, { emojis: inputs, }); return inputs )

And then no dependency is required and the callback persists forever. But it's unclear why it's doing that, and since the button it's attached to wouldn't exist until inputs are set, I left it as is. If I think of a clever way to pull out the analytics reporting from the handler to make this truly survive forever, I will.

thomasoniii · 2023-11-28T16:03:43Z

-    tempWorkspace.dispose();
-    return previewCode;
-  };
-


This was moved to utils/getPreviewCode and is a candidate for further refactoring since it's always called with the result of getGeneratedEffect and maybe those should be combined.

thomasoniii · 2023-11-28T16:04:02Z

-  ].map((part: string, index: number) => {
-    return <span key={index}>{part}</span>;
-  });
-


This was pulled out into the separate DanceAiModalHeader component

thomasoniii · 2023-11-28T16:04:41Z

-              <button key={0} type="button" value={Toggle.EFFECT}>
+              <button type="button" value={Toggle.EFFECT}>


keys are only meaningful in a loop. They're not needed inside of static content like this, so I dropped them.

thomasoniii · 2023-11-28T16:05:35Z

+            currentGeneratedEffect={currentGeneratedEffect}
+            getPreviewCode={getPreviewCode}
+            getGeneratedEffect={getGeneratedEffect}
+          />


This component is a candidate for further refactoring, since it's pulling in a lot of state from the Modal and I think several of these things can be crushed together into better structures. But that's a future item.

thomasoniii · 2023-11-28T16:06:50Z

+                  <div
+                    className={classNames(
+                      moduleStyles.dot,
+                      explanationKeyDotColor[index]


As a general rule, keying off of indexes is frowned upon and it's better to key with a unique value, such as the item's id in this case.

Further, since we now have the selectedEmojis list, just iterates on that instead.

… and generic input grid

fisher-alice · 2024-01-02T18:15:03Z

+};
+
+export type AIEmojiItem = EmojiItem & {
+  modelDescriptiveName: string;


Should we keep the first 'i' in 'AIEmojiItem' lowercase for consistency, e.g., 'AiEmojiItem'?

💯 Just a typo oversight. Fixed!

sanchitmalhotra126

Looks good! Just had one follow-up on only exporting a subset of the AI utils functions (instead of all of them). Thanks for all the back and forth on this!

thomasoniii and others added 10 commits November 27, 2023 12:30

jimt/dance-modal-refactoring-part-2 - made all the inlined functions …

550b6e9

…callbacks

jimt/dance-modal-refactoring-part-2 - brought EmojiIcon out into its …

fc57419

…own file

jimt/dance-modal-refactoring-part-2 - changed a few useCallbacks to u…

2ca1983

…seMemos

set alt text to empty string for blockly tracking pixel

ffd41d6

jimt/dance-modal-refactoring-part-2 - removed currentInputSlot state;…

38d3a45

… made headerValue a memo

jimt/dance-modal-refactoring-part-2 - removed unneeded div wrapper; a…

eeef5ab

…dded memoization

jimt/dance-modal-refactoring-part-2 removed superfluous keys

7322b84

jimt/dance-modal-refactoring-part-2 - pulled out DanceAiModalEmojiInp…

116b08d

…utGrid component

jimt/dance-modal-refactoring-part-2 - pulled getItem out into separat…

48b15f9

…e file

thomasoniii commented Nov 28, 2023

View reviewed changes

Comment thread apps/src/dance/ai/DanceAiModal.tsx Outdated

thomasoniii commented Nov 28, 2023

View reviewed changes

thomasoniii added 2 commits December 13, 2023 11:16

jimt/dance-modal-refactoring-part-2 - added docs and comments

3abce53

Merge branch 'staging-next' into jimt/dance-modal-refactoring-part-2

5fd0afc

thomasoniii requested review from bencodeorg, breville, fisher-alice and sanchitmalhotra126 December 13, 2023 17:29

Merge branch 'staging' into jimt/dance-modal-refactoring-part-2

c7492eb

thomasoniii changed the base branch from staging-next to staging December 13, 2023 19:37

jimt/dance-modal-refactoring-part-2 - unstaged unneeded files from merge

d0926a2

thomasoniii requested a review from a team as a code owner December 13, 2023 19:40

thomasoniii added 2 commits December 13, 2023 13:43

jimt/dance-modal-refactoring-part-2 - unwound more unchanged files

f902b3f

jimt/dance-modal-refactoring-part-2 - unwound more files

a7d43d4

breville reviewed Dec 14, 2023

View reviewed changes

Comment thread apps/src/dance/ai/DanceAiModalFlipCard.tsx Outdated

breville reviewed Dec 14, 2023

View reviewed changes

Comment thread apps/src/dance/ai/utils/calculateOutputSummedWeights.ts Outdated

thomasoniii added 4 commits December 14, 2023 09:23

jimt/dance-modal-refactoring-part-2 - formatting tweaks

19c0ed6

Merge branch 'staging' into jimt/dance-modal-refactoring-part-2

a97029f

jimtdance-modal-refactoring-part-2 - cleaned up merge detritus

fd9055e

jimt/dance-modal-refactoring-part-2 - renamed types

b813dd1

fisher-alice reviewed Dec 14, 2023

View reviewed changes

Comment thread apps/src/dance/ai/DanceAiModalFlipCard.tsx Outdated

thomasoniii added 6 commits December 14, 2023 11:38

jimt/dance-modal-refactoring-part-2 - AiOutput -> DanceAiModalOutputType

739ddc5

jimt/dance-modal-refactoring-part-2 - added missing type

11c35ee

jimt/dance-modal-refactoring-part-2 - givenInputs/initialValue

56ef511

jimt/dance-modal-refactoring-part-2 - DanceAiModelItem -> EmojiItem

116b6e3

jimt/dance-modal-refactoring-part-2 - retooled towards EmojiItem type…

2e1d258

… and generic input grid

Merge branch 'staging' into jimt/dance-modal-refactoring-part-2

f7dbd94

fisher-alice reviewed Jan 2, 2024

View reviewed changes

jimt/dance-modal-refactoring-part-2 AIEmojiItem -> AiEmojiItem

e925f56

sanchitmalhotra126 approved these changes Jan 2, 2024

View reviewed changes

thomasoniii merged commit 1011d3d into staging Jan 3, 2024

thomasoniii deleted the jimt/dance-modal-refactoring-part-2 branch January 3, 2024 15:18


		const [mode, setMode] = useState(Mode.INITIAL);
		const [currentInputSlot, setCurrentInputSlot] = useState(0);

		(useInputs: string[], step: number) => {
		(myInputs: string[], step: number) => {

		const getItem = (id: string) =>
		inputLibraryJson.items.find(item => item.id === id);

		const handleGenerateClick = () => {
		const handleGenerateClick = useCallback(() => {

		<button key={0} type="button" value={Toggle.EFFECT}>
		<button type="button" value={Toggle.EFFECT}>

Conversation

thomasoniii commented Nov 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Links

Testing story

Deployment strategy

Follow-up work

Privacy

Security

Caching

PR Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thomasoniii Nov 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

thomasoniii commented Nov 28, 2023 •

edited

Loading

thomasoniii Nov 28, 2023 •

edited

Loading

fisher-alice Jan 2, 2024 •

edited

Loading