Conversation
sanchitmalhotra126
left a comment
There was a problem hiding this comment.
So cool! The future is coming 😎
This will be a huge speedup levelbuilders and I'm excited to see us already taking advantage of the AI Gateway for internal use. Left comments as I saw them but didn't pore too deep into some of the longer files. Broadly I'd say my feedback for the next round of iteration is 1) encourage reuse of existing types and frameworks where possible (Lab2 types, HttpClient, etc) and 2) break up larger files into smaller modules.
|
|
||
| import {uploadLevelAsset} from './levelApi'; | ||
|
|
||
| // We don't import getModel from aichat/api/client/helpers/modelHelpers because |
There was a problem hiding this comment.
Huh a bit confused by this - doesn't seem like getModel should have Lab2Registry in the import path, unless I'm missing something? And even then seems like simply importing Lab2Registry shouldn't be an issue if we're not actually calling it?
If it doesn't cause issue, I would prefer to use the existing helpers in aichat/api/client since this gives us a good use case for vetting the AI Gateway client code against internal tool usage (cc @edcodedotorg you might be interested in this use case too).
There was a problem hiding this comment.
From Claude:
You're right, I was wrong about that.
aichat/api/client/helpers/modelHelpers.tsitself doesn't importLab2Registry— onlygenerateChatResponse.tsandperformClientApiChatCompletion.tsdo, and we don't need either. Switched togetModelfrommodelHelpersdirectly in 7a5a47b. Treating this as the validation case for AI Gateway internal-tool reuse you mentioned.
| ), | ||
| layout: z | ||
| .enum([ | ||
| 'text-top-left', |
There was a problem hiding this comment.
Can we consolidate/share with the PanelLayout type?
There was a problem hiding this comment.
From Claude:
Done in 5a3ab24 —
ai/panels.tsnow importsPanelLayoutfrom@cdo/apps/panels/types. The literal list inside the zod schema stays (zod's.enum()needs a tuple of literals, not a TS type), but the runtime fallback inplanPanelsand thePanelPlantype are bothPanelLayout-typed, so a drift between schema and panel type would now fail typecheck.
| const prompt = [ | ||
| 'You are helping a curriculum author build a "Panels" level: a short,', | ||
| 'comic-strip-style sequence of full-width panels with overlay text.', | ||
| 'The level description follows. Plan a sequence of 3 to 6 panels that,', |
There was a problem hiding this comment.
Should the "3 to 6" be configurable?
There was a problem hiding this comment.
I think 3 is 6 is a reasonable default but the prompt writer should be able to specify their own count (or at least range) which overrides this.
There was a problem hiding this comment.
From Claude:
Per Brendan's reply: kept 3-6 as the default and rephrased the prompt to "Plan a sequence of panels (3 to 6 by default; if the description names a specific count or range, honor that)". Schema max bumped from 8 to 12 for headroom. Landed in 21ba1e2.
There was a problem hiding this comment.
Can we break up this file 🙂 would be great to at least split the panels and weblab2 specific generation pieces into their own files, with any shared utils in a common location
There was a problem hiding this comment.
From Claude:
Done in 5a3ab24. Layout:
ai/ shared.ts PROMPT_TAGS, log helpers, getTextModel, getImageModel panels.ts plan + image generation pipeline weblab2.ts schema, prompt, MultiFileSource synthesis outline.ts lesson outline generatoraiGeneration.ts is gone; nothing imports the barrel name.
| }); | ||
|
|
||
| export interface Weblab2Generation { | ||
| startSources: object; |
There was a problem hiding this comment.
Any reason this can't be MultiFileSource?
There was a problem hiding this comment.
From Claude:
Done in 7a5a47b.
Weblab2Generation.startSourcesisMultiFileSource,updateStartSources's parameter isMultiFileSource, andpriorOutputFromLevelProperties's weblab2 branch reads throughMultiFileSource. Files useProjectFileType.STARTERinstead of the literal'starter'.
| // handle_json_params. | ||
| export async function updatePanelsLevel( | ||
| levelId: number, | ||
| panels: object[] |
There was a problem hiding this comment.
Can this be Panel[]?
There was a problem hiding this comment.
From Claude:
updatePanelsLevel(levelId: number, panels: Panel[])in 7a5a47b.
| // so this is the same payload shape used by the codebridge save button. | ||
| export async function updateStartSources( | ||
| levelId: number, | ||
| startSources: object |
There was a problem hiding this comment.
Can this be MultiFileSource (presumably we do something similar when we save start sources in labs)? @molly-moen @fisher-alice might know better
There was a problem hiding this comment.
From Claude:
Done in 7a5a47b —
updateStartSources(levelId: number, startSources: MultiFileSource). Same payload shape the codebridge save button posts to/levels/:id/update_start_code.
| export async function updateLongInstructions( | ||
| levelId: number, | ||
| longInstructions: string | ||
| ): Promise<void> { | ||
| const form = new FormData(); | ||
| form.append('level[long_instructions]', longInstructions); |
There was a problem hiding this comment.
Wonder if there's a case for a generic updateLevelProperty API
There was a problem hiding this comment.
From Claude:
Done in d804df9.
updateLongInstructionsandupdateGeneratePromptwere both thin wrappers; replaced withupdateLevelProperty(id, property, value). Call sites pass'long_instructions'/'generate_prompt'explicitly. Works for any string-valued serialized attribute the level subclass exposes viaLevel.permitted_params.
| const blob = new Blob([data.buffer as ArrayBuffer], {type: mediaType}); | ||
| const form = new FormData(); | ||
| form.append('file', blob, filename); | ||
| const response = await fetch('/level_assets/upload', { |
There was a problem hiding this comment.
Potential future refactor - share this upload code with UploadImageDialog
There was a problem hiding this comment.
From Claude (who can also file issues):
Parking this one as you suggested — we'd want to extract a small
uploadLevelAsset(file: File | Blob, opts)fromUploadImageDialog'shandleChangeand have both call it. I'd rather do that as a focused follow-up that touches both call sites; happy to file an issue if useful.
| export async function loadLessonLevelProperties( | ||
| lessonId: number | ||
| ): Promise<Record<string, Record<string, unknown>>> { | ||
| const response = await fetch(`/lessons/${lessonId}/level_properties`, { |
There was a problem hiding this comment.
Potentially shareable with useLoadLevelProperties though it is just an API call. If not shared, I'd at least say let's use HttpClient and LevelPropertiesValidator.
Return type can also be LevelPropertiesMap.
There was a problem hiding this comment.
From Claude:
loadLessonLevelPropertiesnow usesHttpClient.fetchJson<LevelPropertiesMap>(path, undefined, LevelPropertiesMapValidator)and returnsLevelPropertiesMap, mirroringuseLoadLevelProperties(7a5a47b). Didn't fully merge with the hook because the hook reads its lesson id from redux +useAppSelector, which we don't have on this page. If you'd rather see them factored into a shared util, happy to open a follow-up.
PR feedback from Sanchit on #72537. Mechanical, no behavior change. - levelApi.ts: replace hand-rolled fetch + csrfToken/jsonHeaders with HttpClient.{post,put,fetchJson}. PATCH calls now go via PUT (Rails routes both to :update). createOrFindLevel catches NetworkError on the POST 406 path so the "name has already been taken" fallback still works under HttpClient's throw-on-not-ok behavior. - levelApi.ts: panels parameter typed as Panel[]; startSources as MultiFileSource; loadLessonLevelProperties returns LevelPropertiesMap via LevelPropertiesMapValidator (mirroring useLoadLevelProperties). - aiGeneration.ts: drop the local createGoogleGenerativeAI provider in favour of getModel from aichat/api/client/helpers/modelHelpers. The Lab2Registry concern was unfounded: that file imports only model SDK glue and has no Lab2Registry transitively. - aiGeneration.ts: Weblab2Generation.startSources is now MultiFileSource; ProjectFileType.STARTER replaces the literal 'starter'. - LessonGenerator.tsx: priorOutputFromLevelProperties takes LevelProperties; levelPropertiesById typed as LevelPropertiesMap. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PR feedback from Sanchit on #72537. Internal lab identifier moves from Rails STI class names ('Panels', 'Weblab2') to Lab2 AppName values ('panels', 'weblab2'), with conversion helpers at the API and page boundaries. - types.ts: SUPPORTED_LAB_TYPES constant satisfies readonly AppName[], so adding a lab is a single-line entry. LabType is derived from it. RAILS_TYPE_BY_LAB and labTypeFromRailsType handle conversion to/from the Rails STI class names the levels controller speaks. - aiGeneration.ts: outline schema's labType enum is built from SUPPORTED_LAB_TYPES via z.enum, so a new lab automatically becomes a valid outline output. OutlineLevel.labType is now LabType. - LessonGenerator.tsx: drop the literal SUPPORTED_TYPES set; replace the type/SUPPORTED_TYPES check with labTypeFromRailsType. LAB_OPTIONS is generated from SUPPORTED_LAB_TYPES with a separate LAB_LABELS map for display strings. Generator dispatch compares against 'panels' / 'weblab2'. priorOutputFromLevelProperties matches the new identifiers. - levelApi.ts: createOrFindLevel and findLevelByName translate LabType to the Rails STI name via RAILS_TYPE_BY_LAB before sending. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PR feedback from Sanchit on #72537. Use the design system icon component (matching EditPanels.tsx) instead of unicode arrows and the trash emoji on the per-level cards. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PR feedback from Sanchit on #72537. updateLongInstructions and updateGeneratePrompt were thin wrappers that differed only in the property name; replace them with a single updateLevelProperty(id, key, value) helper that takes the property name as an argument. Call sites in LessonGenerator pass 'long_instructions' and 'generate_prompt' explicitly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PR feedback from Sanchit on #72537. Defaults stay where they were, but the per-level description can now drive the shape of the output. - aiGeneration.ts: panels prompt rephrased from "Plan a sequence of 3 to 6 panels" to "Plan a sequence of panels (3 to 6 by default; if the description names a specific count or range, honor that)". Schema max bumped from 8 to 12 to give the override path headroom. - aiGeneration.ts: weblab2 prompt allows file names with `/` to express subfolders (e.g. "css/style.css"); flat layout stays the default and is only deepened when the description asks. Schema max bumped from 6 to 20. - aiGeneration.ts: file synthesis splits the model's filename on `/`, walks the path, and creates folder entries on demand. Each file's folderId points to its leaf folder ("0" for files at the root). The earlier "files must live in folder 0" comment was wrong; existing hand-built weblab2 levels nest folders fine. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PR feedback from Sanchit on #72537. The single-file generator was 521 lines; split it into focused modules under ai/. ai/shared.ts PROMPT_TAGS, log helpers, getTextModel, getImageModel ai/panels.ts panels schema, plan, image generation, full pipeline ai/weblab2.ts weblab schema, prompt, MultiFileSource synthesis ai/outline.ts lesson outline schema and generator LessonGenerator.tsx imports each entry point directly. The old aiGeneration.ts is removed; nothing imports the barrel name. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PR feedback from Sanchit on #72537. The page component was 1179 lines. Pull out the four inline subcomponents and the three helper modules into their own files. handleGenerate stays inline; it touches enough local state that hoisting it into a hook would just thread props. components/LevelCard.tsx per-level card UI components/OutlineBlock.tsx <details> outline section components/ProgressDialog.tsx in-progress modal components/SummaryDialog.tsx post-run modal helpers/buildInitialState.ts listLessonLevels, inferPrefix, buildInitialState, newLevelSpec helpers/precedingLevels.ts PriorOutput/PriorEntry types, priorOutputFromLevelProperties, formatPrecedingLevels helpers/rebuildActivities.ts Placement type, blank* constructors, rebuildActivities LessonGenerator.tsx drops to ~540 lines: one component, four memoised callbacks, the long generation loop, and the page render that wires the extracted components together. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PR feedback from Sanchit on #72537. The lesson generator's findLevelByName previously hit /levels/get_filtered_levels — a paged LIKE %name% search — and filtered the result client-side for an exact match. Replace with a dedicated endpoint backed by the unique levels.name index. - routes.rb: collection route `get :by_name` under :levels. - levels_controller.rb: by_name action narrows by optional :type and finds by exact :name on the existing @levels CanCanCan scope. Returns the standard summarize_for_edit hash, or 404 if missing. - levelApi.ts: findLevelByName posts to /levels/by_name, treats 404 as "not found" via isNetworkError. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds coverage for the Ruby changes that landed earlier in this PR. LessonsController: - generate (id form): auth matrix + happy path asserting generateOutline flows through and editLessonUrl is the id-form edit URL. - generate_with_lesson_position: auth matrix + happy path asserting the edit URL stays in the lesson-position family. Plus the legacy-script- levels guard. - update: pin generate_outline round-trip via lesson_params, and that sending an empty value clears the saved property. LevelsController#by_name: - Happy path returns the summary; type filter narrows; missing or type-excluded names return 404; non-levelbuilders are forbidden; signed-out users are redirected to sign in. ScriptLevel#summarize_for_lesson_edit: - Pin that each level summary now carries `type` (the Rails STI class name) and `generatePrompt` (or nil when unset). The /generate page reads these to decide which lab types it can populate. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This proposes a new levelbuilder feature to generate a lesson's levels using AI.
The goal is to make it faster and easier to create a lesson's levels. It's influenced by @jamjamgobambam's Levels and Slides Drafter demo.
This feature is accessed by visiting
/s/[script]/lessons/[lesson]/generate.As working right now, a levelbuilder can generate an entire lesson of panels & weblab2 levels from a single description. It's also easy to adjust the set of levels & their individual descriptions before generating them.
From a single lesson prompt
When starting, it's optional to generate a set of levels from a single lesson prompt:
If provided, this prompt is also fed to all subsequent generation operations to maintain some common context across all levels.
A set of level descriptions
Here's an example set of level descriptions generated from this prompt:
If the single lesson prompt was not used, this can all be set up manually. And even if the prompt was used, this can all be adjusted before the levels generation takes place.
Levels generation
When ready, levels generation can be initiated. A popup shows progress, with even more detail emitted to the console. The feature creates levels and their content: images & text for panels levels; instructions, HTML/CSS/JS for weblab2 levels.
The generated content of all preceding levels is provided for each new level's generation, helping it to maintain consistency.
A lesson
Once generation is complete, we have a lesson with levels. They'll include panels:
And weblab2:
Revisions
It's important to note that this is designed to be run repeatedly for the same lesson when desired. On the generate page, each level's
Generatecheckbox is checked if the description is updated, and can also be checked manually, so that the level will be updated on the next initiation of levels generation.Other labs
This feature enumerates but leaves unsupported levels intact, so that a wider variety of level types can be included in a lesson. Of course, we can add generation of additional level types in the future.