Add aichat to allthethings by ebeastlake · Pull Request #52538 · code-dot-org/code-dot-org

ebeastlake · 2023-06-29T17:59:48Z

Adds a placeholder level for Aichat to /s/allthethings using the new Lab2 framework (I think 😄). S/o to Molly, Brendan, and Sanchit for their work there!

aichat_allthethings.mov

I welcome a close look at the boilerplate because this is my first time adding a new app type.

Links

Jira ticket: https://codedotorg.atlassian.net/browse/SL-940

Testing story

Tested locally; see Quicktime movie above.

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

breville · 2023-06-29T21:58:05Z

      elsif @type_class == Music
        @game = Game.music
+      elsif @type_class == Aichat
+        @game = Aichat.music


This should probably be @game = Game.aichat.

Good catch; I have no idea how I renamed it so badly!

breville · 2023-06-29T21:58:34Z

  end

+  def self.aichat
+    @@game_music ||= find_by_name('Aichat')


This should probably be @@game_aichat ||= find_by_name('Aichat')

breville · 2023-06-29T22:26:46Z

We should be able to simplify this a fair bit, and hopefully not depend on the legacy studioApp.

breville · 2023-06-29T22:27:12Z

We should also be able to simplify this file, and not rely on legacy components such as StudioAppWrapper.

breville · 2023-07-02T03:51:12Z

It would be great for @molly-moen and @sanchitmalhotra126 to take a look at this too, to suggest which new parts of our framework this code might be able to use.

sanchitmalhotra126 · 2023-07-03T18:22:08Z

It would be great for @molly-moen and @sanchitmalhotra126 to take a look at this too, to suggest which new parts of our framework this code might be able to use.

For sure, it would be great to use the new lab framework here! (this would also be a great test of how easy or hard it is to onboard a new lab to the new framework). I think all the dashboard changes are good as-is (adding a new game type, etc). For the front end, we should just need:

levels-aichat-main.js (JS entrypoint): this can be modeled after levels-music-main.js, which wraps the lab view in all the new lab framework components. This essentially replaces studioApp, project.js, etc. This also means we don't need the various loadAichat, Aichat.js boilerplate classes
AichatView: the React view that renders the actual AI Chat experience. It seems like currently this view is just stubbed out so this can be super small, but it shouldn't need to include StudioAppWrapper or any of the other common classes. AichatView can also be a functional component (and preferably typescript too!)

@breville actually has some cool stuff in the works here which would create a single common entrypoint for all "Lab2" (lab lab) labs, so we potentially wouldn't even need a separate levels-aichat-main.js entrypoint.

breville · 2023-07-11T18:03:00Z

+ */
+
+const Aichat: React.FunctionComponent<AichatProps> = ({children}) => {
+  // Leave a margin to the left and the right of the video, to the edges


You probably won't need this layout stuff which is designed for a video which needs to maintain its aspect ratio while also filling the screen as much as possible. A simple flex layout should be enough for your app to take up the whole page in a few panels.

…statements

ebeastlake · 2023-07-11T19:57:41Z

+import ProjectContainer from '@cdo/apps/lab2/projects/ProjectContainer';
+import ProgressContainer from '@cdo/apps/lab2/progress/ProgressContainer';
+import AichatView from '@cdo/apps/aichat/AichatView';
+import {logError} from '@cdo/apps/music/utils/MusicMetrics';


@sanchitmalhotra126 Can you confirm that the desired approach is to create an AichatMetrics version of

code-dot-org/apps/src/music/utils/MusicMetrics.ts

Line 4 in 0cda37a

/**

here?

Actually I've removed MusicMetrics in this PR: #52714, in favor of a global Lab2MetricsReporter singleton that any lab can use directly, so we shouldn't need a specific AichatMetrics.

ebeastlake · 2023-07-11T19:59:47Z

  },
+  {
+    name: 'aichat',
+    usesChannel: false,


Can someone clarify the meaning of these values? usesChannel indicates a project is channel-backed, yes? I'm not sure about backgroundMode.

usesChannel means that the level is project/channel-backed.

backgroundMode is a new concept that indicates whether the level is removed from the DOM when not active. For Music Lab, we don't yet support this removal from the DOM due to our Blockly usage not yet supporting a full teardown and subsequent recreation, and so we just make it invisible. For standalone videos, we do support removal from the DOM. You can pick based on your lab's capabilities, but it seems most likely you'd want to aim for false for this value.

ebeastlake · 2023-07-11T20:00:47Z

+#  index_levels_on_name       (name)
+#
+
+# TODO: Update this level to account for the params we actually need.


Callout: I did not do much work to customize this, but I assume that's all right to be future work.

We don't want to lock ourselves into a schema that requires a migration to address, but it's probably the contents of serialized_attrs that we might want to adjust, and those can be changed without a migration since they all serialize to the same database field.

breville · 2023-07-11T20:06:02Z

+      '@cdo/aichat/locale': path.resolve(
+        __dirname,
+        'src',
+        'aichat',
+        'locale-do-not-import.js'
+      ),


I'm actually curious whether this is needed.

I get a webpack error without it, but I am happy to dig in more.

I think so long as we have lab-specific locale files, we will need these specific aliases.

breville · 2023-07-11T20:06:36Z

      elsif @level.is_a? Blockly
        blockly_options
-      elsif @level.is_a?(Weblab) || @level.is_a?(Fish) || @level.is_a?(Ailab) || @level.is_a?(Javalab)
+      elsif @level.is_a?(Weblab) || @level.is_a?(Fish) || @level.is_a?(Ailab) || @level.is_a?(Javalab) || @level.is_a?(Aichat)


Is this needed since this is a Lab2 app?

I don't think so, thanks! I should also be able to remove the non_blockly_puzzle_level_options method from the Aichat model.

molly-moen · 2023-07-11T21:26:35Z

+    options.freeze
+  end
+
+  def uses_lab2?


since uses_lab2 is true, we don't need a bunch of the boilerplate anymore. Everything gets loaded via levels-lab2-main, so you don't need the aichat version of that file, along with a bunch of other logic, you can see where I removed some of this for music in this PR. It would also be worth merging the latest changes into this branch, it doesn't look like you have the changes from that linked pr here.

Here's what I removed so far: 90cb706

molly-moen · 2023-07-12T16:08:36Z

@@ -0,0 +1,12 @@
+import appMain from '@cdo/apps/appMain';


I don't think we need this anymore either!

molly-moen · 2023-07-12T16:15:52Z

+    },
+    {
+      "seeding_key": {
+        "level.key": "AI Chat Test Level",


are we missing this file from this PR? I tried to pull and seed locally and it can't find the level.

Added dashboard/config/scripts/levels/AI Chat Test Level.level and confirmed seeding that level succeeds locally.

sanchitmalhotra126 · 2023-07-12T18:57:05Z

@molly-moen @breville double checking - do we still need this file? I know we currently do have one for music, but it seems like since we don't call appMain(), these levels files are not referenced anywhere.

Hmm we may be able to delete it then! I may have missed that in my clean up

sanchitmalhotra126 · 2023-07-12T18:58:14Z

+      '@cdo/aichat/locale': path.resolve(
+        __dirname,
+        'src',
+        'aichat',
+        'locale-do-not-import.js'
+      ),


I think so long as we have lab-specific locale files, we will need these specific aliases.

ebeastlake · 2023-07-17T16:34:49Z

@sanchitmalhotra126 I was assuming there would be outstanding work here to include the new MetricsReporter but it looks like there isn't because the file that originally used the Music version of the reporter is no longer needed and the reporting functionality is built into the Lab2 framework (see original comment here). Does that sound right?

sanchitmalhotra126 · 2023-07-17T20:12:38Z

@sanchitmalhotra126 I was assuming there would be outstanding work here to include the new MetricsReporter but it looks like there isn't because the file that originally used the Music version of the reporter is no longer needed and the reporting functionality is built into the Lab2 framework (see original comment here). Does that sound right?

Yep correct! This was replaced with a top-level Lab2MetricsReporter whose reporting functions can be called directly from lab code when needed.

molly-moen

🎉 thanks for incorporating all the lab2 stuff as it was hot off the presses!

sanchitmalhotra126

🎉 awesome! Very cool to see a new app onboarded onto the Lab2 framework so quickly, and thanks for bearing with our updates!

sanchitmalhotra126 · 2023-07-19T22:56:31Z

+// TODO: Add AichatLevelData.
+


Note that if AichatLevelData extends ProjectLevelData, it would probably be better to define the type within the aichat directory (like MusicLevelData, defined in music/types), so that the lab-specific level data definition can be closer to the lab that's using it

Thanks for the feedback! I'll call this out in Slack for the fast-follow PRs, but I didn't move the comment for now, because I'm eager to get this into staging.

ebeastlake requested a review from a team as a code owner June 29, 2023 20:04

breville reviewed Jun 29, 2023

View reviewed changes

ebeastlake force-pushed the task/initial-chat-levels branch from fea5397 to e8e2c20 Compare July 7, 2023 06:19

ebeastlake added 8 commits July 10, 2023 10:43

add chat to allthethings

91a2c6f

convert to aichat

457ee71

add level to allthethings locally

bd9a4a2

inherit from level

240a8a4

fix misnamed variables

a59880a

support non blockly puzzle options

1647088

add default i18n file

1381b5f

modify levels-aichat-main

07e1d33

ebeastlake force-pushed the task/initial-chat-levels branch from ad086c0 to 07e1d33 Compare July 11, 2023 17:50

render using lab2 framework (#52730)

73ca46e

breville reviewed Jul 11, 2023

View reviewed changes

ebeastlake added 2 commits July 11, 2023 12:30

remove unnecessary UI and LevelData for now; remove unneeded console …

b9150b7

…statements

update comments

be3e6d8

ebeastlake changed the title ~~[DO NOT MERGE] Add chat to allthethings~~ Add aichat to allthethings Jul 11, 2023

ebeastlake commented Jul 11, 2023

View reviewed changes

breville reviewed Jul 11, 2023

View reviewed changes

breville requested review from molly-moen and sanchitmalhotra126 July 11, 2023 20:09

molly-moen reviewed Jul 11, 2023

View reviewed changes

ebeastlake added 2 commits July 11, 2023 22:12

remove unnecessary boilerplate

90cb706

merge in updated lab2 changes

fb366e4

molly-moen reviewed Jul 12, 2023

View reviewed changes

sanchitmalhotra126 reviewed Jul 12, 2023

View reviewed changes

ebeastlake added 2 commits July 12, 2023 19:57

delete more unnecessary files

0d331fa

add missing level file

a44cc27

Merge branch 'staging' into task/initial-chat-levels

a211344

ebeastlake added 11 commits July 17, 2023 20:54

Merge branch 'staging' into task/initial-chat-levels

b17978c

update to use LabViewsRenderer

365b3ff

add to types_without_ideal_level_source

07616c4

add aichat to edit.html.haml

addd1d2

add log messages for debugging

f4b3dff

simplify fields .html.haml file to pass unit test

238b094

fix path to aichat_fields haml file

b75c632

remove log messages

04b218b

remove more log messages

6ad919f

remove unnecessary white space

e0250a5

modify test level to match row

c605961

ebeastlake requested review from breville, molly-moen and sanchitmalhotra126 July 19, 2023 22:26

molly-moen approved these changes Jul 19, 2023

View reviewed changes

sanchitmalhotra126 approved these changes Jul 19, 2023

View reviewed changes

ebeastlake merged commit 0e458f8 into staging Jul 20, 2023

ebeastlake deleted the task/initial-chat-levels branch July 20, 2023 16:10

Conversation

ebeastlake commented Jun 29, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

breville commented Jul 2, 2023

Uh oh!

sanchitmalhotra126 commented Jul 3, 2023

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!

ebeastlake Jul 11, 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!

ebeastlake Jul 12, 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

Uh oh!

ebeastlake Jul 17, 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!

ebeastlake commented Jul 17, 2023

Uh oh!

sanchitmalhotra126 commented Jul 17, 2023

Uh oh!

molly-moen left a comment

Choose a reason for hiding this comment

Uh oh!

sanchitmalhotra126 left a comment

ebeastlake commented Jun 29, 2023 •

edited

Loading

ebeastlake Jul 11, 2023 •

edited

Loading

ebeastlake Jul 12, 2023 •

edited

Loading

ebeastlake Jul 17, 2023 •

edited

Loading