Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions workbench/_web/bun.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion workbench/_web/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@
"d3-delaunay": "^6.0.4",
"dotenv": "^17.2.1",
"drizzle-orm": "^0.44.4",
"edulogitlens": "github:jon-bell/edulogitlens#fa59004",
"edulogitlens": "github:jon-bell/edulogitlens#82a7327",
"framer-motion": "^12.23.22",
"html-to-image": "^1.11.13",
"lexical": "^0.34.0",
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
"use client";

import { useEffect, useId, useRef, useState } from "react";
import { useEffect, useId, useMemo, useRef, useState } from "react";
import { createPortal } from "react-dom";
import { motion, useDragControls } from "motion/react";
import {
Expand All @@ -22,7 +22,7 @@ import { Input } from "@/components/ui/input";
import { useCapture } from "@/lib/analytics";
import { useProlificTutorial, HINT_AUTO_OFFER_AT } from "@/stores/useProlificTutorial";
import type { GlossaryEntry, HintRung, SpotlightTarget, UnitCheck } from "@/types/tutorial-content";
import { resolveCheckKey } from "@/types/tutorial-content";
import { resolveCheckKey, resolveUnitSpotlights } from "@/types/tutorial-content";
import { DEFAULT_GLOSSARY } from "@/tutorials/glossary";
import { CompletionCta } from "./CompletionCta";
import { TutorialGlossary } from "./TutorialGlossary";
Expand Down Expand Up @@ -141,56 +141,49 @@ export function TutorialActivityPanel({
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [runNonce]);

// Read before the mount/active guard below, so the patch-result effect can
// Read before the mount/active guard below, so the spotlight derivation can
// depend on them (a conditional hook isn't an option).
const isPatchUnit = unit?.progression.on === "patch";
const patchToken = store.patchTokenByUnit[store.unitIdx] ?? null;
const hintStage = store.hintStageByUnit[store.unitIdx] ?? 0;

// On arriving at a unit, ring the cells that unit asks about — and clear
// whatever the previous unit lit. Declared before the patch-result effect so
// that on arriving back at a patched step, this runs and that one re-lights,
// in the same commit.
// Everything this step spotlights, in one derived value: the layers it forces
// on screen, the cells it rings on arrival, the cells of whatever hint rung has
// been revealed, and — once a patch is filed — the result cell.
//
// Unit-level spotlights are not a nicety on the patch step. Its task says
// "drag this ringed cell onto that one", which was a lie while spotlights only
// fired on a revealed hint; and the ring is also what forces the widget to
// render that layer at all, since auto-fit downsamples layers to the column
// width and a narrow display can drop the layer the step is about.
const unitSpotlights = unit?.spotlights;
const spotlitPatch = useRef<string | null>(null);
useEffect(() => {
spotlitPatch.current = null;
onSpotlight?.(unitSpotlights?.length ? unitSpotlights : null);
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [store.unitIdx, store.active, unitSpotlights]);

// Point at the target's post-patch output the moment a patch lands. On the
// step carrying the whole point of the tool, participants performed the
// intervention successfully and then could not find its result — the panel
// now says what changed (below) and rings the cell it changed in.
// This used to be three effects writing the same channel, each overwriting the
// others. The one that hurt was the hint reveal: it was imperative, so nothing
// re-applied it, and any remount (a reload, or collapsing and re-expanding the
// dock, which unmounts the column the panel portals into) dropped the rings
// while the hint on screen still read as revealed. The revealed stage is
// persisted, so the rings can be derived from it instead of remembered.
//
// The result cell is ADDED to the step's own spotlights, never substituted for
// them. `patchToken` is not evidence that a result is on screen: a patch
// restored from an earlier session is re-filed on arrival at this step
// (PatchLensDisplay's "restored patch" effect) even when no result grid is
// rendered. Replacing here meant that token silently deleted the two cells the
// step's task names — leaving the drag it asks for pointed at nothing, and
// (because a spotlight is also what forces a downsampled layer to render) the
// patch layer missing from the grid entirely. Lighting both is safe: an
// unrendered result grid resolves to no cell, so the extra target is inert.
// Two things the effects taught us, kept here:
// - Ringing a cell is also what forces the widget to render its layer —
// auto-fit downsamples layers to the column width, so the layer a step is
// about can be missing from a narrow grid. `forceLayers` does that job
// alone, for a step that must not ring anything (see resolveUnitSpotlights).
// - The patch result is ADDED to the step's own cells, never substituted for
// them (commit f3d193a). `patchToken` is not evidence a result grid is on
// screen: a patch restored from an earlier session is re-filed on arrival
// (PatchLensDisplay's "restored patch" effect) with nothing rendered, and
// substituting there deleted the two cells the step's task names — and with
// them the patch layer itself. Lighting both is safe: an unrendered result
// grid resolves to no cell, so the extra target is inert.
//
// Nothing is spotlit while the tutorial is off screen. The guard lives here
// rather than being inherited from the render, because these hooks sit above
// the `active` early-return (hooks can't be conditional).
const spotlights = useMemo(
() => (store.active ? resolveUnitSpotlights(unit, hintStage, patchToken != null) : null),
[store.active, unit, hintStage, patchToken],
);
useEffect(() => {
// Nothing is spotlit while the tutorial is off screen: these effects sit
// above the `active` guard (hooks can't be conditional), so the invariant
// has to be stated here rather than inherited from the render.
if (!store.active || !isPatchUnit || patchToken == null) return;
if (spotlitPatch.current === patchToken) return;
spotlitPatch.current = patchToken;
onSpotlight?.([
...(unitSpotlights ?? []),
{ grid: "result", layer: "last", position: "last" },
]);
onSpotlight?.(spotlights);
// onSpotlight is a prop the host redeclares every render; re-pushing on the
// payload alone is what keeps this from thrashing the widget.
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [store.active, isPatchUnit, patchToken, unitSpotlights]);
}, [spotlights]);

// Back to the top of the step on arrival. The steps are long enough to scroll,
// and the container keeps its offset across a unit change — so advancing from
Expand Down Expand Up @@ -228,7 +221,6 @@ export function TutorialActivityPanel({

const total = units.length;
const attempts = store.attemptsByUnit[store.unitIdx] ?? 0;
const hintStage = store.hintStageByUnit[store.unitIdx] ?? 0;
const completed = store.completedUnits.includes(store.unitIdx);
const isLast = store.unitIdx === total - 1;

Expand Down Expand Up @@ -348,10 +340,10 @@ export function TutorialActivityPanel({
const stage = store.revealHint();
const rung = unit.hints.find((h) => h.stage === stage);
if (rung?.insertPrompt) onInsertPrompt(rung.insertPrompt);
// A rung may light several cells — both ends of a
// drag, say. `spotlights` wins over `spotlight`.
const cells = rung?.spotlights?.length ? rung.spotlights : rung?.spotlight;
if (cells) onSpotlight?.(cells);
// The rung's cells are NOT lit from here. `revealHint` persists
// the stage, and the spotlight payload above is derived from it
// — so the rings survive a reload and a dock collapse, which an
// imperative call from this handler did not.
}}
/>

Expand Down
45 changes: 45 additions & 0 deletions workbench/_web/src/db/__tests__/tutorials.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,8 @@ describe("tutorial content", () => {
kind: "patch",
progression: { on: "patch" },
patchPair: { source: "The Eiffel Tower is in", target: "The Colosseum is in" },
spotlights: [{ grid: "source", layer: 20, position: 5 }],
forceLayers: [{ grid: "target", layer: 20 }],
answerPlaceholder: "e.g. Paris",
observationPlaceholder: "What changed?",
faqs: [{ q: "What is a patch?", a: "Copying one cell into the other prompt." }],
Expand All @@ -220,6 +222,49 @@ describe("tutorial content", () => {
expect(() => validateTutorialContent(content)).not.toThrow();
});

// A spotlight the widget can't resolve silently highlights nothing — for a hint,
// exactly the rung a stuck participant reached for. `forceLayers` fails more
// quietly still: the step goes back to being about a column that auto-fit has
// dropped, with nothing on screen to say so.
it("rejects malformed spotlights and forceLayers, and accepts a position-less one", () => {
const base = tinyContent().units[0];
const withUnit = (overrides: Partial<TutorialUnit>) =>
validateTutorialContent({ version: 1, units: [{ ...base, ...overrides }] });

expect(() => withUnit({ spotlights: [] })).toThrow();
expect(() =>
withUnit({ spotlights: [{ grid: "middle", layer: 1, position: 1 }] as never }),
).toThrow();
expect(() =>
withUnit({ spotlights: [{ grid: "source", layer: -1, position: 1 }] }),
).toThrow();
// Present-but-unresolvable position still fails; absent is now legal (it
// renders the layer and rings nothing).
expect(() =>
withUnit({ spotlights: [{ grid: "source", layer: 1, position: 1.5 }] }),
).toThrow();
expect(() => withUnit({ spotlights: [{ grid: "source", layer: 20 }] })).not.toThrow();

expect(() => withUnit({ forceLayers: [] })).toThrow();
expect(() =>
withUnit({ forceLayers: [{ grid: "result", layer: "middle" }] as never }),
).toThrow();
expect(() => withUnit({ forceLayers: [{ grid: "nope", layer: 20 }] as never })).toThrow();
// A position here would be dropped on the way to the widget, so an author
// who wrote one is waiting for a ring that never comes.
expect(() =>
withUnit({ forceLayers: [{ grid: "source", layer: 20, position: 5 }] as never }),
).toThrow();
expect(() =>
withUnit({
forceLayers: [
{ grid: "source", layer: 20 },
{ grid: "target", layer: "last" },
],
}),
).not.toThrow();
});

// The welcome slideshow is modal and it is the first thing a participant sees,
// so a slide that renders blank blocks the tutorial behind it rather than
// degrading into something they can work around.
Expand Down
25 changes: 24 additions & 1 deletion workbench/_web/src/lib/queries/tutorialContentDb.ts
Original file line number Diff line number Diff line change
Expand Up @@ -117,12 +117,17 @@ export const validateTutorialContent = (content: TutorialContent): TutorialConte
// A spotlight the widget can't resolve silently highlights nothing — for a
// hint, exactly the rung a stuck participant reached for; for a unit, the
// cells its instructions tell them to drag between.
//
// `position` is optional: an entry with a grid and a layer but no position
// forces that layer's column to render and rings nothing (how `forceLayers`
// reaches the widget). A position that IS present still has to resolve, or
// it rings a cell nobody meant.
const checkSpotlights = (cells: unknown[], where: string) => {
for (const s of cells as { grid?: unknown; layer?: unknown; position?: unknown }[]) {
if (
!validGrids.has(s?.grid) ||
!isCellIndex(s?.layer) ||
!isCellIndex(s?.position)
(s?.position !== undefined && !isCellIndex(s?.position))
) {
throw new Error(
`Unit "${u.id}" ${where} has a malformed spotlight (needs grid source|target|result and a non-negative integer or "last" layer/position)`,
Expand All @@ -136,6 +141,24 @@ export const validateTutorialContent = (content: TutorialContent): TutorialConte
}
checkSpotlights(u.spotlights, "spotlights");
}
// Same shape minus the position, and the same failure when it's wrong: a
// layer the widget can't resolve is silently dropped, and the step that
// asked for it goes back to being about a column that isn't on screen.
if (u.forceLayers !== undefined) {
if (!Array.isArray(u.forceLayers) || u.forceLayers.length === 0) {
throw new Error(`Unit "${u.id}" forceLayers must be a non-empty array`);
}
checkSpotlights(u.forceLayers, "forceLayers");
// A position here is dropped on the way to the widget, so an author who
// wrote one is expecting a ring they will never get. Say so instead.
for (const f of u.forceLayers as { position?: unknown }[]) {
if (f?.position !== undefined) {
throw new Error(
`Unit "${u.id}" forceLayers entries take no position (they render a layer without ringing a cell) — use spotlights to ring one`,
);
}
}
}
for (const h of u.hints) {
if (typeof h?.stage !== "number" || typeof h?.text !== "string") {
throw new Error(`Unit "${u.id}" has a malformed hint rung`);
Expand Down
16 changes: 16 additions & 0 deletions workbench/_web/src/tutorials/__tests__/prolificSeed.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,22 @@ describe("prolific tutorial seed", () => {
expect(unit("u4a-compare").spotlights).toBeUndefined();
});

// The two halves of what a spotlight does, split. The compare step needs layer
// 20 rendered — auto-fit drops it from two heatmaps in one column, and then the
// column the next step drags across only appears once a hint rings it — but it
// must not ring anything, because finding those rows is the step's own task.
it("the compare step shows the patch layer without ringing its cells", () => {
const compare = unit("u4a-compare");
const forced = compare.forceLayers ?? [];
expect(forced.map((f) => f.grid).sort()).toEqual(["source", "target"]);
// Position-less, or it would ring a cell like any other spotlight.
expect(forced.every((f) => !("position" in f))).toBe(true);
// The same layer the next step drags across, so the participant has already
// looked at the column they are about to patch.
const patched = unit("u4-patching").spotlights ?? [];
expect(forced.map((f) => f.layer)).toEqual(patched.map((c) => c.layer));
Comment on lines +136 to +139

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Assert the required layer number.

The test checks parity with u4-patching, but it does not enforce the tutorial requirement that both compare grids render layer 20. If both fixtures change to another layer, this test still passes. Add an explicit f.layer === 20 assertion.

This follows the PR objective that u4a-compare must force layer 20 in both grids.

Proposed assertion
         const patched = unit("u4-patching").spotlights ?? [];
+        expect(forced.every((f) => f.layer === 20)).toBe(true);
         expect(forced.map((f) => f.layer)).toEqual(patched.map((c) => c.layer));
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// The same layer the next step drags across, so the participant has already
// looked at the column they are about to patch.
const patched = unit("u4-patching").spotlights ?? [];
expect(forced.map((f) => f.layer)).toEqual(patched.map((c) => c.layer));
// The same layer the next step drags across, so the participant has already
// looked at the column they are about to patch.
const patched = unit("u4-patching").spotlights ?? [];
expect(forced.every((f) => f.layer === 20)).toBe(true);
expect(forced.map((f) => f.layer)).toEqual(patched.map((c) => c.layer));
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@workbench/_web/src/tutorials/__tests__/prolificSeed.test.ts` around lines 136
- 139, Update the test around the forced spotlight comparison in
prolificSeed.test.ts to explicitly assert that every layer in forced has value
20, while preserving the existing parity check against u4-patching.

});

// Every hint that names a cell in prose also rings it. A hint that has to give
// coordinates ("the 'um' cell at the end of 'Colosseum'") is a hint about a
// missing affordance, and the drag is the one interaction prose can't convey.
Expand Down
22 changes: 22 additions & 0 deletions workbench/_web/src/tutorials/prolificSeed.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,13 @@ import type { TutorialContent } from "@/types/tutorial-content";
* ("the 'um' cell at the end of 'Colosseum'") is a sign the affordance isn't
* discoverable. Verified against the Llama-3.1 tokenizer: with BOS at index 0,
* position 5 is " Tower" in the source and "um" in the target.
* - **Showing a layer and ringing a cell are separated.** A spotlight does both
* at once — it rings the cell, and it keeps the widget from downsampling that
* layer away — which is right for the patch step (`spotlights`, both ends of
* the drag) and wrong for the compare step before it, whose task is to find
* those same rows. Compare uses `forceLayers` for the column and leaves the
* rings to its stage-2 hint, so layer 20 is on screen from arrival but nothing
* answers the question the step is asking.
*/

export const PROLIFIC_TUTORIAL_SLUG = "prolific-patch-lens-demo";
Expand All @@ -64,6 +71,14 @@ const PATCH_DRAG = [
{ grid: "target" as const, layer: 20, position: 5 },
];

/**
* The same two columns, with no cell ringed — for the compare step, which needs
* layer 20 on screen but must not point at the rows its task asks the participant
* to find. Derived from PATCH_DRAG so the layer cannot drift between the step that
* shows the column and the step that drags across it.
*/
const PATCH_COLUMNS = PATCH_DRAG.map(({ grid, layer }) => ({ grid, layer }));

export const PROLIFIC_TUTORIAL_SEED: TutorialContent = {
version: 1,
welcome: {
Expand Down Expand Up @@ -454,6 +469,13 @@ export const PROLIFIC_TUTORIAL_SEED: TutorialContent = {
'Write your own pair — two sentences worded the same way with different answers, like "The opposite of hot is" and "The opposite of tall is". Run them and find where each answer settles.',
prompts: [EIFFEL, COLOSSEUM],
patchPair: { source: EIFFEL, target: COLOSSEUM },
// Layer 20 on screen in both grids, and nothing ringed. The step's task
// is to find the landmark's row, so a ring would do it for them — but
// without the column rendered at all (auto-fit downsamples layers to the
// width, and two heatmaps in one column is the narrow case), the layer
// the next step drags across isn't there to be looked at, and only
// appears once the stage-2 hint rings it.
forceLayers: PATCH_COLUMNS,
hints: [
{
stage: 1,
Expand Down
Loading
Loading