Move the Tutor+ teacher gallery into the lesson-deep-dive package - #75105
Draft
stephenliang wants to merge 13 commits into
Draft
Move the Tutor+ teacher gallery into the lesson-deep-dive package#75105stephenliang wants to merge 13 commits into
stephenliang wants to merge 13 commits into
Conversation
Adds gallery.html/src/dev/gallery.tsx as a second dev-shell entry rendering apps' Tutor+ project gallery (ChallengeGallery), mirroring how main.tsx renders the student lesson deep dive. Extracts the shared fonts/theme/Font-Awesome/50px-band chrome both entries need into devPageChrome.tsx, with no change to main.tsx's rendered output. Adds galleryFixtures.ts (tutorGalleryData bootstrap plus a plausible class section's worth of video/whiteboard challenge responses, wire-typed from ChallengeResponsesController/ChallengeResponse#summarize) and galleryMocks.ts (msw GET handlers for the gallery's three endpoints, gated behind a ?viewerRole= knob for the teacher/owner/peer project page layouts), plus tiny real video/PNG fixtures in public/gallery/ for the asset download_urls.
… them ChallengeResponse#summarize adds student_feedback/evaluated_at/ evaluation_result/rubric as conditional hash keys (include_feedback/ include_evaluation), not nulled fields — Rails never sends them at all to a peer, and never sends evaluation_result/rubric to an owner. The GET /challenge_responses/:id mock was sending null/[] instead, which a client distinguishing absent-key from null would not catch against the shell. Build the response with conditional spreads instead.
…he package challenge_responses and challenges share their wire types and validators between the student ChallengeActivities flow and the teacher gallery, both still partly in apps. Moving the types first, ahead of the gallery components themselves, lets ChallengeActivities and the gallery views staying behind in apps pull from one definition instead of two. Moved: the ChallengeResponse/Challenge family and their validators from lessonDeepDive/types.ts (ExplanationTypes, EvaluationStatus stay with them); all of gallery/types.ts; assetUtils.ts, the one gallery leaf whose runtime imports and scss are self-contained. Deferred: GallerySidebar, ProjectCard, and ReactionChips share challenge-gallery.module.scss with ChallengeGallery; ProjectDetailsCard and ProjectStage share project-view.module.scss with ProjectView and AssessmentPanel. Since ChallengeGallery, ProjectView, and AssessmentPanel are out of scope for this slice and an scss module cannot split across the apps/package boundary, none of those five move yet. ReflectionValue, ReflectionData, LessonDeepDiveData, and the practice problem types stay in apps/types.ts: they depend on LessonObjectiveReflectionValues from generated-scripts, which is Ruby codegen the package does not have a story for yet, and none of them are on the gallery's dependency path.
…y_data endpoint gallery.tsx now always fetches GET /api/v1/tutor_gallery_data (msw mocks it; dashboard mode proxies it) before mounting ChallengeGallery, with ?script/?lessonPosition knobs, falling back to the checked-in fixture and one console.info on any fetch failure so the page never renders blank ahead of the Rails endpoint landing.
…gallery_data The gallery page embeds its bootstrap payload in a data attribute, which nothing but that page can read. A standalone dev shell, and later the page itself, need the same payload over HTTP. New Lessons::TutorGalleryDataController#show, routed as a sibling of tutor/gallery inside the unit_routes block so both the /s/ and /courses/ URL grammars work. Same lesson resolution and availability 404 as the page; class-level authorize! to grant exactly what the page grants. The payload build is duplicated from LessonsController#tutor_gallery on purpose and guarded by a test until the page stops embedding it. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The CJS output compiled `import ky from 'ky'` to `require('ky').create`.
ky is ESM-only, so a CJS consumer such as the apps webpack bundle gets the
module namespace and `create` is undefined. Studio never saw it because
Vite takes the ESM output. Rollup's `interop: 'auto'` picks `.default`
when the required module carries __esModule, the same fix the
lesson-deep-dive package already carries.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…pClient The Tutor+ gallery's four GETs went through apps' HttpClient, the last host import standing between the components and the package. They now go through a core API client that the host injects via ApiClientProvider, with the same URLs, the same validators, and the same effect structure. The client is built per host with a same-origin base URL, not the singleton DashboardApiClient: the singleton's base URL is absolute and would bypass the webpack and Vite dev proxies. ky's default 10 s timeout is turned off because HttpClient had none. apps' Jest cannot load the real @code-dot-org/core/api (ky is ESM-only under its CJS transform), so the two fetching suites stand in a context-only mock, the same shape as the HttpClient module mock they replace. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…package git mv of the eight components, two SCSS modules and the tutor avatar from apps/src/aiTutor/views/gallery to the package's src/gallery. Content is unchanged except what the new location forces: barrel imports become relative, the avatar import becomes relative (Vite inlines it), and the unused default React import goes because the package compiles JSX with the automatic runtime under noUnusedLocals. The barrel exports the components; the webpack entry and the five apps Jest suites import them from there. A type-only reference to the component library's themes module makes its MUI variant augmentation visible to the dts build, which excludes the dev shell that used to carry it. Runtime deps the moved code needs (core, MUI, classnames) become peer deps. The three presentational Jest suites stub @code-dot-org/core/api with an empty module: the package is one chunk, so importing any component loads ChallengeGallery's core import, which Jest cannot parse. They move to Vitest next. Package lint and prettier fail on this commit by design; the next commit reformats the moved files to the package's conventions so this one stays a pure rename. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ntions Output of the package's eslint --fix and prettier --write on the files the previous commit moved: import grouping and order, trailing commas, and one named MouseEvent type import in place of the default React import that import-x/default rejects. No JSX, identifier, comment or logic changes. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…s Vitest The five suites move next to the components they test. Runner syntax and import paths change; assertions and test names do not. The two fetching suites render inside the real ApiClientProvider with a stub transport, which Vitest can do because it loads core's ESM dependencies; the Jest-only module mocks go away with them. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The gallery's move put an @code-dot-org/core/api import into the lesson-deep-dive package's single-chunk build, so every apps Jest suite that imports anything from that barrel now loads ky, camelcase-keys, its nested camelcase and quick-lru, and map-obj. All five ship ESM only and Jest's CJS transform refused them. Allowing them through babel-jest fixes the five lesson deep dive suites this broke; it works now that core's CJS output carries default-import interop. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…in the page The Rails page no longer embeds the gallery's bootstrap in a data attribute. A new TutorGalleryPage in the package fetches it from the tutor/gallery_data route beside the page, shows the gallery's own loading and error lines meanwhile, then renders ChallengeGallery. The webpack entry derives the lesson path from the URL and mounts that page; the Rails action shrinks to authorization, the availability 404, and the mount div. The dev shell mounts the same page component, so its live mode is the production code path; its fixture fallback and the drift-guard test that compared the JSON to the embedded payload go away with the payload. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The README opened by saying the package ships nothing and the feature's source still lives in apps. The gallery lives here now and the student flow's leaf views have for a while. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The Tutor+ teacher gallery lived in
apps/with a webpack entry, anHttpClientdependency and a Rails-embedded bootstrap payload; it now lives infrontend/packages/lesson-deep-diveas a host-agnostic component, the page fetches its bootstrap from a JSON route beside the page, and the Rails action does nothing but authorize and render a mount div.This is one draft PR carrying the whole grow-in as an ordered commit series so each step can be read on its own. Apps stays the production host; a Studio route is a later, additive step that needs no edits under
src/gallery/.feat(dashboard): serve the Tutor+ gallery bootstrap as JSON at tutor/gallery_dataLessons::TutorGalleryDataController#show, routed besidetutor/galleryin the unit routes so both URL grammars work.fix(core): emit default-import interop in the CJS buildrequire('ky').create; ky is ESM-only. First apps consumer of@code-dot-org/core/apihiti.create is not a function. One-line rollupinterop: 'auto'.refactor(gallery): call Rails through core's transport instead of HttpClientApiClientProviderwith a same-origin base URL (the singleton's absolute base URL bypasses the dev proxies).refactor(gallery): move the Tutor+ gallery into the lesson-deep-dive packagegit mvof 8 components, 2 SCSS modules and the avatar. Content differs only by forced import paths and the unused React default import. Package lint fails on this commit by design.style(gallery): format the moved gallery files to the package's conventionseslint --fixandprettier --write, plus one namedMouseEventtype import.test(gallery): port the gallery suites from apps Jest to the package's VitestApiClientProviderwith a stub transport.test(apps): transform core's ESM-only dependencies under Jestfeat(gallery): fetch the bootstrap over JSON instead of embedding it in the pageTutorGalleryPage; the entry derives the lesson path from the URL; the Haml data attribute and the controller's payload build are gone.docs(lesson-deep-dive): describe what the package now shipsThe branch is based on
stephen/ldd-shell-live-bootstrap(staging plus the teacher-gallery dev shell from #74914, the wire-types move, and the shell's live bootstrap fetch), so those commits appear here too.Deviations from the written plan, each forced by a measurement: the "style prep in apps before the move" step was infeasible (apps compiles JSX with the classic runtime, pins prettier 2, and orders imports differently), so the move is two commits, a pure rename and a formatting pass. The core interop fix and the Jest transform allowlist were not planned; both are prerequisites the swap and the move exposed. The bootstrap endpoint is page-adjacent (
tutor/gallery_data, precedentlevel_properties) rather than under/api/v1, because the page answers to two URL grammars and the entry can only derive its own path.Links
PLAN-ldd-teacher-gallery-growin.md(scratchpad, ask if you want it attached)Testing story
Parity was measured, not eyeballed. A DOM and computed-style fingerprint (tag, own text, alt/href/role, bounding rect, 27 computed properties for every element under the mount) was captured on the pre-change code and again after commits 3, 5 and 8, for seven states: section list, three teacher project views (scored video, unscored video, whiteboard deep link), the owner and peer project views as students, and the "My projects" empty state. All states compare IDENTICAL to the pre-change baseline at every step, ignoring only a 15 px scrollbar width between the headless baseline browser and the headed one. Class names are excluded by construction; they are Vite-hashed after the move.
Request sequence on the final page:
GET .../tutor/gallery_data200, thenGET /challenge_responses?unit_id=§ion_id=andGET /challenge_responses/unit_counts?section_id=. The page HTML has nodata-tutorgallerydataattribute. One request more than before, a loading line before the sidebar, otherwise unchanged.Mocked mode (
VITE_API_MODE=msw yarn devin the package,/gallery.html) covers the edge cases: owner and peer layouts via?viewerRole=, a missing project via?project=999, and the empty unit state.Automated: package
release:dryrun(build, lint, prettier, stylelint, typecheck, 7 Vitest files / 27 tests); appstypecheck; full apps Jest suite 1107 suites / 10504 tests after commit 7;Lessons::TutorGalleryDataControllerTest4 tests.LessonsControllerTest'stutor_gallerycases that render the HTML layout needRAILS_ENV=test rake assets:precompilelocally and are verified by CI.To try it locally: run
bin/dashboard-server, build apps (cd apps && yarn build), sign in as a teacher whose section has final challenge submissions, and open/s/<script>/lessons/<n>/tutor/gallery?enableExperiments=lesson-tutor-challenge. For the shell:cd frontend/packages/lesson-deep-dive && VITE_API_MODE=msw yarn devthenhttp://localhost:5173/gallery.html, or withoutVITE_API_MODEathttp://localhost-studio.code.org:5173/gallery.htmlagainst a running Rails.Deployment notes
The page now depends on
GET .../tutor/gallery_dataat runtime. Both ship in this PR; nothing to coordinate. Thelesson-tutor-challengeexperiment gate is unchanged and still in the webpack entry.🤖 Generated with Claude Code