Fix defects found in a full simulation review#10
Merged
Conversation
…abel
Fixes found while reviewing the simulation end to end (physics, state
handling, views, i18n, a11y), verified in a real browser build.
- Lab permalink lost the preset selection. queryStringFromState() never
emits `preset=`, but stateFromQueryParameters() only compared the state
against the preset named in the URL, so every link produced by "Copy
link" reopened with the physics right and the "Show me" menu reading
"Custom". The selection is now recovered by matching the clamped state
against every preset; preset states are mutually distinct, and a new
LabPresets test asserts that so the match stays unambiguous. The
round-trip test now covers all 20 presets and asserts the selection.
- The compile-time locale key-parity guard in StringManager compared only
English and French, so a Spanish file missing a key would have compiled
and thrown at runtime. Added the Spanish pair (the locales are in parity
today; this keeps them that way).
- The material insert checkbox announced a fixed "Insert material" while
its visible label is the panel title ("Polarizer (dichroic filter)",
"Wave plate (birefringent slab)"), failing WCAG 2.5.3 Label in Name for
speech-input users. The accessible name now embeds the visible title via
a new controls.material.insertPattern string in all three locales.
- Intro's panel stack was the only one without maxHeight, so a longer
locale could push it past the screen edge. Matched the other screens.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CumJAU8CY31XxX176jcQ2w
The remaining items from the simulation review, verified against a real
build in Chromium at 1024x700, 1280x800 and 1600x900.
Scene framing (the major one). The three.js canvas spans the whole window
and the Scenery panels are painted over it, so a screen's scene only
clears its controls if it is shifted left and zoomed out enough to fit
beside them. On the three two-column screens it was not: roughly 30% of
the play area sat behind the panels, including the right transverse frame
and the right-end E-vector — exactly the transmitted light those screens
exist to show. viewOffset alone could not fix it, so screens now declare
a panelLayout ("singleColumn" | "twoColumn") and WaveScreenView picks the
matching projection offset and starting camera range from one table;
WaveSceneCamera takes the starting range so reset() returns to it. The
widest window is the tightest fit, because ScreenView scaling grows the
panels faster than the extra width it buys, so the two-column range is
set from that case. Intro is unchanged.
Minor findings:
- WaveDisplayNode used one module-level scratch Vector3 as both the
camera position and the arrow direction, correct only because of the
order render() happens to call them in. Separate scratches now.
- The Lab's "Copy link" confirmation did not cancel its pending timeout,
so a second click had its label cleared early by the first click's
timer. Each copy now restarts the two seconds.
- "Custom" was offered in the preset menu even though selecting it does
nothing — it reports a state the model sets after a manual edit. It is
now listed only while it IS the selection, in the visible list and in
the parallel DOM alike (verified: hidden/display:none on the option
element when a preset is selected).
- The Wave Plates readouts showed a signed retardation, so "-90°"
whenever the horizontal index was the larger one. A plate is specified
by how much retardation it introduces and the Fast axis readout already
states the direction in words, so the readouts now quote the magnitude.
Δφ itself keeps its sign. The degrees rounding moved onto the model, so
the panel and the accessible summary can no longer disagree; a new
WavePlatesModel suite covers both.
- The wavelength-dependent absorption preference is global but only does
anything where a material absorbs, so its description now says which
screens that is, in all three locales.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CumJAU8CY31XxX176jcQ2w
The three useExplicitType warnings were the only noise left in `npm run lint`; annotating the constants makes the whole gate silent. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CumJAU8CY31XxX176jcQ2w
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
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
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.
Reviewed the simulation end to end — physics model, state handling, views, i18n, accessibility, tests, build — and fixed everything found. Every change was verified against a real production build in Chromium (WebGL via SWiftShader) at 1024×700, 1280×800 and 1600×900, not only in unit tests.
The physics core came out clean and was verified independently rather than taken on trust: phase is continuous at both slab interfaces for all n, w and both travel directions; the quarter-wave preset genuinely converts 45° linear light to circular (|E| after the plate is constant at 5.000 across a period, versus 0 → 7.07 before it); the half-wave plate mirrors the polarization plane (135° → 45°); B = k̂ × E is perpendicular for every polarization and flips with direction. No changes were needed there.
Scene framing behind the control panels
The three.js canvas spans the whole window and the Scenery panels are painted over it, so a screen's scene only clears its controls if it is shifted left and zoomed out enough to fit beside them. On the three two-column screens it was not: roughly 30% of the play area sat behind the panels, including the right transverse frame and the right-end E-vector arrow — exactly the transmitted light those screens exist to show. This also contradicted
viewOffset's own comment ("clearing the right-hand panel stack").viewOffsetalone could not fix it — clearing the panels at the old zoom would have needed a half-width of ~200 px against the ~420 px actually drawn. Screens now declare apanelLayout("singleColumn"|"twoColumn") andWaveScreenViewpicks the matching projection offset and starting camera range from oneSCENE_FRAMINGtable;WaveSceneCameratakes the starting range as a constructor argument so Reset All returns to it. The widest window is the tightest fit, because ScreenView scaling grows the panels faster than the extra width it buys, so the two-column range is set from that case. Intro is unchanged.Wave Plates benefits most: the whole plate, both end frames, the incoming linear light and the outgoing circular light are now visible together.
Lab permalinks lost the preset selection
queryStringFromState()never emitspreset=, butstateFromQueryParameters()only compared the state against the preset named in the URL. Every link produced by "Copy link" therefore reopened with the physics correct and the "Show me" menu reading Custom — 19 of the 20 presets affected.The selection is now recovered by matching the clamped state against every preset. The 20 preset states are mutually distinct, and a new test asserts that so the match stays unambiguous. The round-trip test previously covered 3 presets and only asserted
state, which is why this passed; it now covers all 20 and asserts the selection too.Accessibility
The material insert checkbox announced a fixed "Insert material" while its visible label is the panel title ("Polarizer (dichroic filter)", "Wave plate (birefringent slab)"), so a speech-input user saying "Polarizer" could not activate it — WCAG 2.5.3 Label in Name. The accessible name now embeds the visible title via a new
controls.material.insertPatternstring in all three locales. Confirmed in the DOM on each screen, e.g."Insert: Polarizer (dichroic filter)".Smaller fixes
StringManagerasserteden ↔ fronly, so a Spanish file missing a key would have compiled and thrown at runtime. The locales are in parity today (124 keys each); the added pair keeps them that way.WaveDisplayNodeused one module-levelVector3as both the camera position and the arrow direction — correct only because of the orderrender()happened to call them in. Separate scratches now.hidden/display: nonewhen a preset is selected).maxHeight, so a longer locale could push it past the screen edge.useExplicitTypewarnings were the only noise left innpm run lint; annotating the constants makes the whole gate silent.Verification
npm run check,npm run lintandnpm run buildare clean, with lint now completely warning-free.69 tests pass, up from 61. New coverage: all 20 presets round-tripping through the permalink mapping with their selection asserted, preset-state distinctness, and a new
tests/WavePlatesModel.test.tscovering retardation, its displayed magnitude (including the negative-Δφ case that is awkward to reach in a browser) and the exact QWP/HWP presets.Browser checks across all four screens plus the permalink, clipboard and accessible-name paths reported 0 console errors.
CLAUDE.mdanddoc/implementation-notes.mdare updated for the new test suite and the per-screen framing decision.Reviewer notes
CAMERA_RANGE_TWO_COLUMN = 29was tuned empirically against screenshots at the three window sizes above. It is a visual-design number; if you want the wave larger and are willing to accept less clearance, that is the one value to move.Generated by Claude Code