Guided tutorial: make the spotlit layer actually visible, and separate forcing from ringing - #140
Merged
Merged
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughTutorial spotlight handling now derives a single payload from unit targets, persisted hints, patch results, and forced layers. Tutorial validation and Prolific fixtures support layer-only targets. The ChangesTutorial spotlight behavior
Dependency reference update
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with 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.
Inline comments:
In `@workbench/_web/src/tutorials/__tests__/prolificSeed.test.ts`:
- Around line 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.
In `@workbench/_web/src/types/tutorial-content.ts`:
- Around line 294-300: Update the spotlight selection in the revealed hints
reduce logic to prioritize the plural spotlights field whenever it is present,
including when it is an empty array, instead of checking its length before
falling back to spotlight. Add a test covering both fields with spotlights: []
and confirming the plural field wins.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f0c94526-f534-4f3f-8b69-e795eac88250
⛔ Files ignored due to path filters (1)
workbench/_web/bun.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
workbench/_web/package.jsonworkbench/_web/src/app/workbench/[workspaceId]/patch-lens/[chartId]/components/tutorial/TutorialActivityPanel.tsxworkbench/_web/src/db/__tests__/tutorials.test.tsworkbench/_web/src/lib/queries/tutorialContentDb.tsworkbench/_web/src/tutorials/__tests__/prolificSeed.test.tsworkbench/_web/src/tutorials/prolificSeed.tsworkbench/_web/src/types/__tests__/unitSpotlights.test.tsworkbench/_web/src/types/tutorial-content.ts
Comment on lines
+136
to
+139
| // 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)); |
There was a problem hiding this comment.
🎯 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.
Comment on lines
+294
to
+300
| const revealed = unit.hints | ||
| .filter((h) => h.stage <= hintStage) | ||
| .sort((a, b) => a.stage - b.stage) | ||
| .reduce<SpotlightTarget[]>((acc, h) => { | ||
| const rung = h.spotlights?.length ? h.spotlights : h.spotlight ? [h.spotlight] : []; | ||
| return rung.length ? rung : acc; | ||
| }, []); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Preserve plural spotlight precedence for empty arrays.
Line 298 treats spotlights: [] as absent. It then uses spotlight when both fields exist. This conflicts with the HintRung contract that spotlights wins over spotlight.
Select spotlights based on field presence, not array length. Add a test for an empty spotlights array with a singular spotlight.
Proposed fix
- const rung = h.spotlights?.length ? h.spotlights : h.spotlight ? [h.spotlight] : [];
+ const rung = h.spotlights !== undefined ? h.spotlights : h.spotlight ? [h.spotlight] : [];📝 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
| const revealed = unit.hints | |
| .filter((h) => h.stage <= hintStage) | |
| .sort((a, b) => a.stage - b.stage) | |
| .reduce<SpotlightTarget[]>((acc, h) => { | |
| const rung = h.spotlights?.length ? h.spotlights : h.spotlight ? [h.spotlight] : []; | |
| return rung.length ? rung : acc; | |
| }, []); | |
| const revealed = unit.hints | |
| .filter((h) => h.stage <= hintStage) | |
| .sort((a, b) => a.stage - b.stage) | |
| .reduce<SpotlightTarget[]>((acc, h) => { | |
| const rung = h.spotlights !== undefined ? h.spotlights : h.spotlight ? [h.spotlight] : []; | |
| return rung.length ? rung : acc; | |
| }, []); |
🤖 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/types/tutorial-content.ts` around lines 294 - 300, Update
the spotlight selection in the revealed hints reduce logic to prioritize the
plural spotlights field whenever it is present, including when it is an empty
array, instead of checking its length before falling back to spotlight. Add a
test covering both fields with spotlights: [] and confirming the plural field
wins.
… from ringing Steps 7 and 8 both need layer 20 on screen in each heatmap. Step 8 asked for it and appeared not to get it; step 7 never asked at all. Neither was the mechanism failing — the emission side is sound, `setTarget` takes arrays, and against the real Llama-3.1-8B tokenization position 5 is " Tower" / "um" as the content claims. The layer WAS being forced into the grid. It was then clipped out of view. Auto-fit sizes its column budget as if no spotlight existed, and the widget renders the spotlit layer on top of that set — so with the tutorial docked at 1366x768 each grid gets a one-column budget, shows [0, 31], and the forced layer 20 lands past the scroll container's right edge. Measured at 69% visible, and 0% at a 420px panel. That is width-dependent, which is why it read as intermittent and why an 80-layer harness (f3d193a) showed it passing. The widget half is edulogitlens 82a7327, pinned here: auto-fit now budgets in rendered columns, and a grid scrolls its own container to bring a spotlit column into view. Ring visibility goes to 1.00 at every tested width. Three changes on this side: - `forceLayers` on a unit. A spotlight does two jobs — it rings a cell and it keeps auto-fit from downsampling that layer away — and step 7's task is to FIND the landmark row, so it needs the second job without the first. Ringing the cells on arrival would do the step's work for the participant. u4a-compare now forces layer 20 in both grids and still rings nothing until its stage-2 hint; the assertion that it has no unit `spotlights` stays true on purpose. - One derived spotlight payload instead of three effects overwriting each other. The hint-reveal write was imperative and nothing re-applied it, so 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 still read as revealed. The revealed stage is persisted, so `resolveUnitSpotlights` derives the whole payload from it and is unit-tested directly. - The arrival effect had no `active` guard, so closing the tutorial on a patch step re-lit its rings on a closed panel. Validation stays looser than the code, so `position` is now optional there too and `forceLayers` rejects an entry carrying one — a dropped position would have the author waiting for a ring that never comes. Verified: 194 tests pass; tsc (34 errors) and lint (35/25) unchanged from main. Ring visibility confirmed in Chromium against the real widget at 32 and 80 layers across 420/528/538/801/809px panels. Known, not addressed: once a patch lands, three grids share the vertical space and the source/target rings fall below their fold — pre-existing, and the result ring is now fully visible where it was 0%. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jon-bell
force-pushed
the
fix/tutorial-spotlight-visibility
branch
from
August 12, 2026 16:38
f4d5179 to
c1cd525
Compare
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
Contributor
|
🧹 Preview for PR #140 torn down. |
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.
Steps 7 (
u4a-compare) and 8 (u4-patching) both need layer 20 on screen in each heatmap. Step 8 asked for it and appeared not to get it; step 7 never asked at all.What was actually wrong
Not the spotlight mechanism.
setTargetaccepts arrays, per-grid resolution is correct,allLayers.indexOf(20)hits on the 32-layer model, and against the real Llama-3.1-8B tokenization position 5 really is" Tower"/"um"as the content claims. All verified.The layer was being forced into the grid and then clipped out of view. Auto-fit sizes its column budget as if no spotlight existed, and the widget renders the spotlit layer on top of that set. With the tutorial docked at 1366×768 each grid gets a one-column budget, renders
[0, 31], and the forced layer 20 lands past the scroll container's right edge — 380px of content in a 252px port.Measured in Chromium against the real widget: 69% visible at the fielded 528px panel, 0% at 420px and on the result grid.
That it depends on panel width is why this read as intermittent, and why the previous fix (
f3d193a) verified green — its harness used 80 layers at a width where three columns happened to fit.The widget half is edulogitlens
82a7327(jon-bell/edulogitlens#3), pinned here: auto-fit now budgets in rendered columns, and a grid scrolls its own container to bring a spotlit column into view. Ring visibility goes to 1.00 at every tested width.Changes on this side
forceLayerson a unit. A spotlight does two jobs — it rings a cell, and it keeps auto-fit from downsampling that layer away. Step 7's task is to find the landmark row, so it needs the second job without the first; ringing on arrival would do the step's work for the participant.u4a-comparenow forces layer 20 in both grids and still rings nothing until its stage-2 hint. The existing assertion that it has no unit-levelspotlightsstays true on purpose, with a sibling assertion for the new behavior.One derived spotlight payload instead of three effects overwriting each other. This fixes two real bugs:
resolveUnitSpotlightsderives the whole payload from it. Extracted as a pure function and unit-tested directly (there's no React harness in this repo).store.activeguard, so closing the tutorial on a patch step re-lit its rings on a closed panel.Validation.
positionis now optional, andforceLayersrejects an entry that carries one — a silently dropped position would leave the author waiting for a ring that never comes.Verification
scripts/test.shtsc --noEmitbun run lintRing/column visibility confirmed in real Chromium at 32 and 80 layers across 420 / 528 / 538 / 801 / 809px panels, including the fielded 1366×768 docked layout. The no-spotlight control is byte-identical before and after.
Notes for review
82a7327onspotlight-layer-reachable-v1.tutorial.jsonat the repo root is untracked and has the matchingforceLayersblock; it still needs loading through the workshop admin UI.ensureSeedTutorialonly inserts when absent and never updates, so any environment seeded before60eea6ais still serving pre-unit-spotlight content regardless of this PR.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests