From b317fd8346182ecc527bbebfe9f28a26fc695d66 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 27 Jul 2026 01:21:04 +0000 Subject: [PATCH] Fix review findings: readout occlusion, contrast, date bounds, disposal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of the planetarium surfaced several defects, verified against a production build driven in Chromium. Selected Object readout was unreadable. It anchors bottom-left, but the Location + Time panels fill the left column at common window sizes and the Time panel paints over it — only a ~14 px sliver showed at 1280x720 and 900x600. updateChromeLayout now slides the readout clear to the right of that column when they would overlap, mirroring the existing Reset All vs. Display panel avoidance. `left` is computed once and assigned once because this runs re-entrantly from selectionPanel.boundsProperty. Search results were invisible. Result rows and the "no matches" line sit on the dark panel fill but used LIGHT_SURFACE_TEXT_FILL (near-black), giving ~1.04:1 contrast; the highlighted row was white on the light accent at 1.81:1. Both now take the correct side of that pairing, and the rule is written down in CLAUDE.md so the mixup does not recur. The `date` deep link accepted any parseable timestamp while the UI is bounded to 1900-2100, so ?date=1850-01-01 silently rendered 1900 and ?date=2150-06-01 rendered 2100 — a shared link showed a different sky than it named. The parameter is now validated against CIVIL_TIME_MS_RANGE (derived from CIVIL_YEAR_RANGE) and falls back to the documented default. Civil time is held inside the same range at runtime via setCivilTimeMs(), so playback stops at the edge instead of leaving the Year spinner displaying 2101. The epoch combo always read "Custom time": the timer starts playing, so the first step advanced civil time a few milliseconds and marked the preset CUSTOM while the minute-precision readout still showed the preset epoch. It now flips only once the clock is a full minute off, matching the readout. Locale parity only asserted en<->fr, so a key missing from strings_es.json was not a build error despite CONVENTIONS.md sec. 4 requiring it. Both directions are now asserted for es as well (verified by deleting a key). Also: dispose SelectedObjectReadout's axon graph (a Multilink, ~20 DerivedProperties and their pattern Properties stayed subscribed to the model and StringManager after teardown) and call it from ZenithScreenView.dispose; drop the empty `credits` block that rendered blank rows in Help > About; replace literal rgba(0,0,0,0) hit-target fills with Color.TRANSPARENT; remove the unused ZenithPreferencesModel.reset(), which invited wiring preferences into Reset All against the documented contract; and document the "Use my location" third-party IP fallback in the README and model doc. Tests: 154 -> 163. npm run lint, check, build, and test all pass. --- CLAUDE.md | 6 +- README.md | 6 +- doc/model.md | 11 +- src/ZenithConstants.ts | 20 +- src/i18n/StringManager.ts | 7 + src/main.ts | 10 +- src/preferences/ZenithPreferencesModel.ts | 9 +- src/preferences/zenithQueryParameters.ts | 23 +- src/zenith-screen/model/ZenithModel.ts | 38 +++- src/zenith-screen/view/ObjectNameSearch.ts | 14 +- .../view/ObserverLocationNode.ts | 4 +- .../view/SelectedObjectReadout.ts | 210 ++++++++++++++---- src/zenith-screen/view/ZenithScreenView.ts | 24 +- tests/ZenithModel.test.ts | 30 +++ tests/memory-leak.test.ts | 45 ++++ tests/zenithQueryParameters.test.ts | 20 +- 16 files changed, 384 insertions(+), 93 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 89042df..e9d641a 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -37,6 +37,8 @@ SceneryStack **first-person planetarium** for the night sky. Observer location, **Gotchas** - **Ctrl-drag** and **Ctrl+arrow keys** advance **sidereal time** (stars move 1:1 with gesture); civil time advances ~1/1.0027 as fast. +- Civil time is bounded by `CIVIL_TIME_MS_RANGE` (derived from `CIVIL_YEAR_RANGE`, 1900–2100). Route every write that could leave that span through `setCivilTimeMs()`; the `date` query parameter rejects out-of-range epochs rather than clamping them. +- `epochPresetProperty` only flips to `CUSTOM` once civil time is a full minute off the preset — the timer starts playing, so a tighter test would mark it CUSTOM on the first frame and the combo could never show a preset. - Reset All restores model Properties but **not** preference-backed overlays (`showStarLabels`, `showConstellations`, `showPlanetLabels`, `deepStarCatalog`). - Planet positions use `astronomy-engine` only through `PlanetEphemeris.ts`; equatorial↔horizontal transforms are intentionally hand-rolled (see [doc/astronomy-engine.md](doc/astronomy-engine.md)). - Default sky: Boulder (40° N, 105° W), 2024-06-21 18:00 UTC, look south 30° alt, 140° FOV. @@ -48,7 +50,9 @@ Follows the shared [OpenPhysics accessibility convention](https://github.com/Ope ## Compliance carve-outs -- **Hardcoded colors:** `#ffffff` pin stroke in `ObserverLocationNode.ts` — fixed white ring for map legibility on both land and ocean fills; not a profile theme token. +- **Hardcoded colors:** `#ffffff` pin stroke in `ObserverLocationNode.ts` — fixed white ring for map legibility on both land and ocean fills; not a profile theme token. Invisible hit targets use `Color.TRANSPARENT`, not a literal `rgba(0,0,0,0)`. + +**Color pairing gotcha:** `LIGHT_SURFACE_TEXT_FILL` (`controlSurfaceText`, near-black) is only for text on the *white* control surfaces — combo items, flat-button labels, editable field values. Anything drawn on the dark `panelBackground` fill takes `ZenithColors.textColorProperty`. Mixing these up yields ~1:1 contrast and is invisible rather than merely ugly (see the `ObjectNameSearch` result rows). ## Testing diff --git a/README.md b/README.md index 42f60d3..6ce05c5 100644 --- a/README.md +++ b/README.md @@ -18,7 +18,11 @@ Vite 8, TypeScript 7, and Biome 2. - Sun, Moon, and Mercury–Neptune via `astronomy-engine` ephemerides (angularly correct Sun/Moon discs; optional true-scale planets) - Observer latitude/longitude, civil UTC time, and derived local sidereal time with play / pause / speed - English, Spanish, and French localization via `StringManager` -- Deep-link startup via query params (`lat`, `lon`, `date`, `fov`, `magLimit`) +- Deep-link startup via query params (`lat`, `lon`, `date` — 1900–2100, `fov`, `magLimit`) +- Optional "Use my location": browser geolocation, falling back to a third-party IP lookup + (`get.geojs.io`, then `ipapi.co`) when the browser cannot answer. An explicit permission + denial is honoured with no fallback, and the button is the only thing that makes either + request — nothing is sent at startup, and the rest of the sim runs fully offline. - Default (night-sky) and projector color profiles - Progressive Web App (installable, offline-capable) - Git hooks for Biome pre-commit checks diff --git a/doc/model.md b/doc/model.md index 1b624b4..adfd8db 100644 --- a/doc/model.md +++ b/doc/model.md @@ -68,7 +68,9 @@ works across many longitudes): - June / December solstices Year / month / day / hour controls jump to any UTC civil moment in range -(1900–2100). +(1900–2100). That range bounds civil time everywhere: playback stops at either +end rather than running past it, and a `date` deep link outside it falls back to +the default epoch instead of being silently clamped to the boundary. ### Default sky (reset / first load) @@ -101,7 +103,10 @@ south at a modest altitude — a natural outdoor “looking up” pose. - **Object name search** (top-center) — type-ahead over **47 named entries** (38 curated bright stars + 9 solar-system bodies). Enter selects and enables tracking. -- **"Use my location"** — browser geolocation with IP fallback. +- **"Use my location"** — browser geolocation, with a coarse third-party IP-address + lookup as a fallback when the browser cannot answer. Pressing the button is what + triggers the request; declining the browser permission prompt stops there, with no + IP fallback. Worth knowing before using it on a classroom machine. - **N / P** — cycle only among named stars and planets currently in the field of view. ### Coordinate systems (overlays) @@ -192,7 +197,7 @@ You can open the sim with a prepared observer and epoch in the URL, for example: `?lat=-33.9&lon=151.2&date=2024-12-21T10:00:00Z&fov=60&magLimit=4` -Useful parameters: `lat`, `lon`, `date` (ISO-8601 UTC), `fov`, `magLimit`, plus +Useful parameters: `lat`, `lon`, `date` (ISO-8601 UTC, 1900–2100), `fov`, `magLimit`, plus optional toggles for star names, constellation lines, planet names, and the deeper star catalog. Full list: [implementation-notes.md](./implementation-notes.md#deep-link-query-parameters). diff --git a/src/ZenithConstants.ts b/src/ZenithConstants.ts index a80f543..c8d495f 100644 --- a/src/ZenithConstants.ts +++ b/src/ZenithConstants.ts @@ -71,12 +71,6 @@ export const LONGITUDE_RANGE = new Range(-180, 180); /** Arrow-key nudge (degrees) when dragging the observer pin on the location map. */ export const LOCATION_STEP_DEGREES = 5; -/** - * Default local sidereal time is derived from civil time + longitude at reset; - * this constant is only used as a fallback before the first sync. - */ -export const DEFAULT_LOCAL_SIDEREAL_TIME_HOURS = 0; - /** * Hours of civil (and roughly sidereal) time advanced per second of simulation * clock at the base `1×` rate. Real-time: one wall-clock second advances civil @@ -114,6 +108,18 @@ export const CIVIL_DAY_RANGE = new Range(1, 31); /** UTC hour-of-day range for the civil date jump UI. */ export const CIVIL_HOUR_RANGE = new Range(0, 23); +/** + * Supported span of civil time in UTC milliseconds, derived from + * {@link CIVIL_YEAR_RANGE}. This is the authoritative bound: the date-jump + * spinners, the `date` deep-link parameter, and clock playback are all held + * inside it, so the Year spinner can never display a value outside its own + * range and a shared link can never resolve to an epoch the UI cannot express. + */ +export const CIVIL_TIME_MS_RANGE = new Range( + Date.UTC(CIVIL_YEAR_RANGE.min, 0, 1, 0, 0, 0), + Date.UTC(CIVIL_YEAR_RANGE.max, 11, 31, 23, 0, 0), +); + /** * Discrete time-rate ladder: each value multiplies the base educational rate * {@link CIVIL_HOURS_PER_SIM_SECOND}. Symmetric with no zero, so stepping the @@ -306,11 +312,11 @@ ZenithNamespace.register("ZenithConstants", { CONTROL_PANEL_WIDTH, DEFAULT_LATITUDE_DEG, DEFAULT_LONGITUDE_DEG, - DEFAULT_LOCAL_SIDEREAL_TIME_HOURS, CIVIL_HOURS_PER_SIM_SECOND, HOURS_PER_SIDEREAL_DAY, DEFAULT_CIVIL_TIME_MS, CIVIL_YEAR_RANGE, + CIVIL_TIME_MS_RANGE, CIVIL_MONTH_RANGE, CIVIL_DAY_RANGE, CIVIL_HOUR_RANGE, diff --git a/src/i18n/StringManager.ts b/src/i18n/StringManager.ts index e1a12a7..cd4c05c 100644 --- a/src/i18n/StringManager.ts +++ b/src/i18n/StringManager.ts @@ -10,10 +10,17 @@ import stringsEn from "./strings_en.json"; import stringsEs from "./strings_es.json"; import stringsFr from "./strings_fr.json"; +// Compile-time locale parity: every locale must carry exactly the same key set. +// Both directions are asserted for each pair, so a key missing from — or added to — +// any one locale is a build error (`npm run check`), not a runtime fallback. // biome-ignore lint/complexity/noVoid: intentional compile-time type assertion void (stringsEn satisfies typeof stringsFr); // biome-ignore lint/complexity/noVoid: intentional compile-time type assertion void (stringsFr satisfies typeof stringsEn); +// biome-ignore lint/complexity/noVoid: intentional compile-time type assertion +void (stringsEn satisfies typeof stringsEs); +// biome-ignore lint/complexity/noVoid: intentional compile-time type assertion +void (stringsEs satisfies typeof stringsEn); const stringProperties = LocalizedString.getNestedStringProperties({ en: stringsEn, diff --git a/src/main.ts b/src/main.ts index 774a42e..60759e9 100644 --- a/src/main.ts +++ b/src/main.ts @@ -64,13 +64,9 @@ onReadyToLaunch(() => { }, }), - // Optional: fill in credits shown in Help → About - credits: { - leadDesign: "", - softwareDevelopment: "", - team: "", - qualityAssurance: "", - }, + // `credits` is deliberately omitted: joist renders a row for every key it is + // given, so empty strings produce blank labelled rows in Help → About. Add + // the keys back only with real names. Project attribution lives in CREDITS.md. }); sim.start(); diff --git a/src/preferences/ZenithPreferencesModel.ts b/src/preferences/ZenithPreferencesModel.ts index c3c2cc9..cce7b89 100644 --- a/src/preferences/ZenithPreferencesModel.ts +++ b/src/preferences/ZenithPreferencesModel.ts @@ -42,12 +42,9 @@ export class ZenithPreferencesModel { }); } - public reset(): void { - this.showStarLabelsProperty.reset(); - this.showConstellationsProperty.reset(); - this.showPlanetLabelsProperty.reset(); - this.deepStarCatalogProperty.reset(); - } + // Deliberately no reset(): these overlay toggles are preference-backed and + // outlive Reset All (see ZenithModel.reset). An unused reset() here would + // invite wiring it into Reset All and silently breaking that contract. } ZenithNamespace.register("ZenithPreferencesModel", ZenithPreferencesModel); diff --git a/src/preferences/zenithQueryParameters.ts b/src/preferences/zenithQueryParameters.ts index dcfb479..12210fe 100644 --- a/src/preferences/zenithQueryParameters.ts +++ b/src/preferences/zenithQueryParameters.ts @@ -17,6 +17,7 @@ import { logGlobal } from "scenerystack/phet-core"; import { QueryStringMachine } from "scenerystack/query-string-machine"; import { + CIVIL_TIME_MS_RANGE, DEFAULT_CIVIL_TIME_MS, DEFAULT_DEEP_STAR_CATALOG, DEFAULT_FIELD_OF_VIEW_DEG, @@ -34,7 +35,14 @@ import { import ZenithNamespace from "../ZenithNamespace.js"; /** - * Returns true when `value` is empty/null (use sim default) or a parseable civil date. + * Returns true when `value` is empty/null (use sim default), or a parseable + * civil date that falls inside the span the simulation can actually represent + * ({@link CIVIL_TIME_MS_RANGE}, i.e. the `CIVIL_YEAR_RANGE` the date-jump + * spinners cover). Parseability alone is not enough: an out-of-range epoch would + * be silently clamped to the range boundary further down, so a shared link would + * quietly render a different sky than the one it names. Rejecting it here makes + * QueryStringMachine warn and fall back to the documented default instead. + * * Used by QueryStringMachine `isValidValue` for the `date` parameter. * `StringType` from QueryStringMachine includes `null`, so accept that here. */ @@ -42,18 +50,20 @@ export function isValidCivilDateQueryParam(value: string | null): boolean { if (value === null || value === "") { return true; } - return !Number.isNaN(Date.parse(value)); + const ms = Date.parse(value); + return !Number.isNaN(ms) && CIVIL_TIME_MS_RANGE.contains(ms); } /** - * Parses a civil-date query string to UTC ms, or null when empty / invalid. + * Parses a civil-date query string to UTC ms, or null when empty, unparseable, + * or outside {@link CIVIL_TIME_MS_RANGE}. */ export function parseCivilDateQueryParam(value: string | null): number | null { if (value === null || value === "") { return null; } const ms = Date.parse(value); - return Number.isNaN(ms) ? null : ms; + return Number.isNaN(ms) || !CIVIL_TIME_MS_RANGE.contains(ms) ? null : ms; } /** @@ -86,8 +96,9 @@ const zenithQueryParameters = QueryStringMachine.getAll({ }, /** - * Civil UTC timestamp (`Date.parse` / ISO-8601). Empty string uses the sim default. - * Example: `?date=2024-12-21T10:00:00Z`. + * Civil UTC timestamp (`Date.parse` / ISO-8601), within the supported civil + * year range (1900–2100). Empty string — or a date outside that range — uses + * the sim default. Example: `?date=2024-12-21T10:00:00Z`. */ date: { type: "string", diff --git a/src/zenith-screen/model/ZenithModel.ts b/src/zenith-screen/model/ZenithModel.ts index 38a4129..2a9186d 100644 --- a/src/zenith-screen/model/ZenithModel.ts +++ b/src/zenith-screen/model/ZenithModel.ts @@ -34,6 +34,7 @@ import type { ZenithPreferencesModel } from "../../preferences/ZenithPreferences import zenithQueryParameters, { resolveCivilTimeMsFromQuery } from "../../preferences/zenithQueryParameters.js"; import { CIVIL_HOURS_PER_SIM_SECOND, + CIVIL_TIME_MS_RANGE, DEFAULT_CIVIL_TIME_MS, DEFAULT_LATITUDE_DEG, DEFAULT_LONGITUDE_DEG, @@ -65,6 +66,7 @@ import { DEFAULT_LOCATION_PRESET, LOCATION_PRESET_COORDS, LocationPreset } from import type { SelectedSkyObject } from "./SelectedSkyObject.js"; const MS_PER_HOUR = 3600 * 1000; +const MS_PER_MINUTE = 60 * 1000; /** * Single instantaneous ephemeris for the whole solar system at one civil time / @@ -269,7 +271,7 @@ export class ZenithModel implements TModel { this.longitudeProperty = new NumberProperty(startLon, { range: LONGITUDE_RANGE, }); - this.civilTimeMsProperty = new NumberProperty(startCivilMs); + this.civilTimeMsProperty = new NumberProperty(CIVIL_TIME_MS_RANGE.constrainValue(startCivilMs)); this.localSiderealTimeHoursProperty = new NumberProperty(localSiderealTimeHours(startCivilMs, startLon)); this.lookAzimuthDegProperty = new NumberProperty(DEFAULT_LOOK_AZIMUTH_DEG); this.lookAltitudeDegProperty = new NumberProperty(DEFAULT_LOOK_ALTITUDE_DEG, { @@ -391,10 +393,24 @@ export class ZenithModel implements TModel { this.disposers.push(() => this.latitudeProperty.unlink(markLocationCustom)); this.disposers.push(() => this.longitudeProperty.unlink(markLocationCustom)); - const markEpochCustom = (): void => { - if (!this.applyingPreset) { - this.epochPresetProperty.value = EpochPreset.CUSTOM; + // Leaving a named epoch marks it CUSTOM — but only once the clock has moved + // far enough for the UTC readout (minute precision) to disagree with the + // preset. Without that tolerance the timer, which starts playing, advances + // civil time by a few milliseconds on the very first step and flips the combo + // to "Custom time" while the readout still shows the preset epoch — so the + // selector could never display a preset, not even right after one is chosen. + const markEpochCustom = (civilMs: number): void => { + if (this.applyingPreset) { + return; + } + const preset = this.epochPresetProperty.value; + if (preset !== EpochPreset.CUSTOM) { + const presetMs = EPOCH_PRESET_CIVIL_MS.get(preset); + if (presetMs !== undefined && Math.abs(civilMs - presetMs) < MS_PER_MINUTE) { + return; + } } + this.epochPresetProperty.value = EpochPreset.CUSTOM; }; this.civilTimeMsProperty.lazyLink(markEpochCustom); this.disposers.push(() => this.civilTimeMsProperty.unlink(markEpochCustom)); @@ -456,7 +472,17 @@ export class ZenithModel implements TModel { /** Jumps civil time to the observer's real-world current instant ("Now"). */ public setToNow(): void { - this.civilTimeMsProperty.value = Date.now(); + this.setCivilTimeMs(Date.now()); + } + + /** + * Sets civil time, held inside {@link CIVIL_TIME_MS_RANGE}. Every write that + * can leave the supported span (playback, scrubbing, "Now") goes through here, + * so the clock stops at the range edge rather than drifting to an epoch the + * date-jump spinners cannot represent. + */ + public setCivilTimeMs(civilTimeMs: number): void { + this.civilTimeMsProperty.value = CIVIL_TIME_MS_RANGE.constrainValue(civilTimeMs); } /** @@ -503,7 +529,7 @@ export class ZenithModel implements TModel { /** Advances civil time by `hours` (educational scrub / Ctrl-drag). */ public advanceCivilTimeHours(hours: number): void { - this.civilTimeMsProperty.value += hours * MS_PER_HOUR; + this.setCivilTimeMs(this.civilTimeMsProperty.value + hours * MS_PER_HOUR); this.syncLocalSiderealTime(); } diff --git a/src/zenith-screen/view/ObjectNameSearch.ts b/src/zenith-screen/view/ObjectNameSearch.ts index c732bb4..36fd6c5 100644 --- a/src/zenith-screen/view/ObjectNameSearch.ts +++ b/src/zenith-screen/view/ObjectNameSearch.ts @@ -14,7 +14,7 @@ import { DerivedProperty, Multilink, PatternStringProperty, Property, type TReadOnlyProperty } from "scenerystack/axon"; import type { OneKeyStroke } from "scenerystack/scenery"; -import { KeyboardListener, Node, Rectangle, Text, VBox } from "scenerystack/scenery"; +import { Color, KeyboardListener, Node, Rectangle, Text, VBox } from "scenerystack/scenery"; import { PhetFont } from "scenerystack/scenery-phet"; import { LIGHT_SURFACE_TEXT_FILL } from "../../common/ZenithButtonOptions.js"; import { ZenithPanel } from "../../common/ZenithPanel.js"; @@ -151,14 +151,19 @@ export class ObjectNameSearch extends Node { // ── Result rows ───────────────────────────────────────────────────────────── const rowFont = new PhetFont(CONTROL_FONT_SIZE); + // Result rows sit directly on the dark panel fill, not on the white field + // surface, so they take the panel text color. Using LIGHT_SURFACE_TEXT_FILL + // here (near-black on deep indigo) renders them at ~1:1 contrast. The + // highlighted row inverts: dark text on the light accent fill. const createRow = (entry: Entry, active: boolean): Node => { const background = new Rectangle(0, 0, ROW_WIDTH, ROW_HEIGHT, { cornerRadius: 3, - fill: active ? ZenithColors.accentColorProperty : "rgba(0,0,0,0)", + // Transparent (not null) so the whole row stays a pointer hit target. + fill: active ? ZenithColors.accentColorProperty : Color.TRANSPARENT, }); const label = new Text(entry.nameProperty, { font: rowFont, - fill: active ? ZenithColors.controlSurfaceColorProperty : LIGHT_SURFACE_TEXT_FILL, + fill: active ? LIGHT_SURFACE_TEXT_FILL : ZenithColors.textColorProperty, maxWidth: ROW_WIDTH - 12, }); label.left = 6; @@ -174,9 +179,10 @@ export class ObjectNameSearch extends Node { ); // ── "No matches" line + list visibility ──────────────────────────────────── + // Also on the dark panel fill, not the white field — panel text color. const statusText = new Text(controls.searchNoMatchesStringProperty, { font: rowFont, - fill: LIGHT_SURFACE_TEXT_FILL, + fill: ZenithColors.textColorProperty, maxWidth: FIELD_WIDTH, }); Multilink.multilink([focusedProperty, queryProperty, matchesProperty], (isFocused, q, matches) => { diff --git a/src/zenith-screen/view/ObserverLocationNode.ts b/src/zenith-screen/view/ObserverLocationNode.ts index 08a09df..842ee3e 100644 --- a/src/zenith-screen/view/ObserverLocationNode.ts +++ b/src/zenith-screen/view/ObserverLocationNode.ts @@ -20,6 +20,7 @@ import { Shape } from "scenerystack/kite"; import { ModelViewTransform2 } from "scenerystack/phetcommon"; import { Circle, + Color, DragListener, KeyboardListener, Line, @@ -215,7 +216,8 @@ export class ObserverLocationNode extends Node { const pin = new Node({ cursor: "grab", children: [ - new Circle(12, { fill: "rgba(0,0,0,0)" }), + // Transparent (not null) so the disk still hit-tests as a grab target. + new Circle(12, { fill: Color.TRANSPARENT }), new Line(-9, 0, 9, 0, { stroke: ZenithColors.locationPinColorProperty, lineWidth: 1.5 }), new Line(0, -9, 0, 9, { stroke: ZenithColors.locationPinColorProperty, lineWidth: 1.5 }), new Circle(3.5, { fill: ZenithColors.locationPinColorProperty, stroke: "#ffffff", lineWidth: 0.75 }), diff --git a/src/zenith-screen/view/SelectedObjectReadout.ts b/src/zenith-screen/view/SelectedObjectReadout.ts index de95243..5783193 100644 --- a/src/zenith-screen/view/SelectedObjectReadout.ts +++ b/src/zenith-screen/view/SelectedObjectReadout.ts @@ -163,6 +163,15 @@ const buildVisibility = ( }; export class SelectedObjectReadout extends Node { + /** + * Teardown steps for the axon graph this node builds, in the order they must + * run: internal links first, then the Text/Checkbox subtree, then the pattern + * Properties they read, then the DerivedProperties those patterns depend on. + * `DerivedProperty.dispose()` throws while it still has listeners, so the order + * matters — each stage releases the subscribers of the next. + */ + private readonly disposers: (() => void)[] = []; + public constructor(model: ZenithModel) { super(); @@ -176,9 +185,12 @@ export class SelectedObjectReadout extends Node { const nameProperty = new Property( objectName(model.selectedObjectProperty.value, stars, controls.selectedNoneStringProperty.value), ); - Multilink.multilink([model.selectedObjectProperty, controls.selectedNoneStringProperty], (selected, noneLabel) => { - nameProperty.value = objectName(selected, stars, noneLabel); - }); + const nameMultilink = Multilink.multilink( + [model.selectedObjectProperty, controls.selectedNoneStringProperty], + (selected, noneLabel) => { + nameProperty.value = objectName(selected, stars, noneLabel); + }, + ); const coordsProperty = new DerivedProperty( [ @@ -258,8 +270,16 @@ export class SelectedObjectReadout extends Node { const elongDegProperty = new DerivedProperty([elongationProperty], (e) => (e ? e.deg : "—")); const elongDirProperty = new DerivedProperty([elongationProperty], (e) => (e ? e.dir : "")); + // Pattern Properties are hoisted (rather than built inline in each Text) so + // dispose() can release them after the Texts that read them are torn down. + const patterns: { dispose: () => void }[] = []; + const pattern = void }>(property: T): T => { + patterns.push(property); + return property; + }; + const nameText = new Text( - new PatternStringProperty(controls.selectedObjectStringProperty, { name: nameProperty }), + pattern(new PatternStringProperty(controls.selectedObjectStringProperty, { name: nameProperty })), { font: labelFont, fill: ZenithColors.accentColorProperty, maxWidth }, ); const noneText = new Text(controls.selectedNoneStringProperty, { @@ -267,27 +287,34 @@ export class SelectedObjectReadout extends Node { fill: ZenithColors.textColorProperty, maxWidth, }); - const magText = new Text(new PatternStringProperty(controls.selectedMagStringProperty, { mag: magProperty }), { - font: labelFont, - fill: ZenithColors.textColorProperty, - maxWidth, - }); + const magText = new Text( + pattern(new PatternStringProperty(controls.selectedMagStringProperty, { mag: magProperty })), + { + font: labelFont, + fill: ZenithColors.textColorProperty, + maxWidth, + }, + ); const eqText = new Text( - new PatternStringProperty(controls.selectedEquatorialStringProperty, { - ra: raProperty, - dec: decProperty, - }), + pattern( + new PatternStringProperty(controls.selectedEquatorialStringProperty, { + ra: raProperty, + dec: decProperty, + }), + ), { font: labelFont, fill: ZenithColors.textColorProperty, maxWidth }, ); const hzText = new Text( - new PatternStringProperty(controls.selectedHorizontalStringProperty, { - alt: altProperty, - az: azProperty, - }), + pattern( + new PatternStringProperty(controls.selectedHorizontalStringProperty, { + alt: altProperty, + az: azProperty, + }), + ), { font: labelFont, fill: ZenithColors.textColorProperty, maxWidth }, ); const typeText = new Text( - new PatternStringProperty(controls.selectedTypeStringProperty, { type: typeNameProperty }), + pattern(new PatternStringProperty(controls.selectedTypeStringProperty, { type: typeNameProperty })), { font: labelFont, fill: ZenithColors.textColorProperty, @@ -295,16 +322,20 @@ export class SelectedObjectReadout extends Node { }, ); const constellationText = new Text( - new PatternStringProperty(controls.selectedConstellationStringProperty, { - constellation: constellationNameProperty, - }), + pattern( + new PatternStringProperty(controls.selectedConstellationStringProperty, { + constellation: constellationNameProperty, + }), + ), { font: labelFont, fill: ZenithColors.textColorProperty, maxWidth }, ); const elongationText = new Text( - new PatternStringProperty(controls.selectedElongationStringProperty, { - deg: elongDegProperty, - dir: elongDirProperty, - }), + pattern( + new PatternStringProperty(controls.selectedElongationStringProperty, { + deg: elongDegProperty, + dir: elongDirProperty, + }), + ), { font: labelFont, fill: ZenithColors.textColorProperty, maxWidth }, ); @@ -333,41 +364,48 @@ export class SelectedObjectReadout extends Node { const eventTextOptions = { font: labelFont, fill: ZenithColors.textColorProperty, maxWidth }; const riseText = new Text( - new PatternStringProperty(controls.selectedRiseStringProperty, { - time: riseTimeProperty, - clock: riseClockProperty, - az: riseAzProperty, - }), + pattern( + new PatternStringProperty(controls.selectedRiseStringProperty, { + time: riseTimeProperty, + clock: riseClockProperty, + az: riseAzProperty, + }), + ), eventTextOptions, ); const setText = new Text( - new PatternStringProperty(controls.selectedSetStringProperty, { - time: setTimeProperty, - clock: setClockProperty, - az: setAzProperty, - }), + pattern( + new PatternStringProperty(controls.selectedSetStringProperty, { + time: setTimeProperty, + clock: setClockProperty, + az: setAzProperty, + }), + ), eventTextOptions, ); const transitText = new Text( - new PatternStringProperty(controls.selectedTransitStringProperty, { - time: transitTimeProperty, - clock: transitClockProperty, - alt: transitAltProperty, - }), + pattern( + new PatternStringProperty(controls.selectedTransitStringProperty, { + time: transitTimeProperty, + clock: transitClockProperty, + alt: transitAltProperty, + }), + ), eventTextOptions, ); const circumpolarText = new Text(controls.selectedCircumpolarStringProperty, eventTextOptions); const neverRisesText = new Text(controls.selectedNeverRisesStringProperty, eventTextOptions); - visibilityProperty.link((v) => { + const onVisibility = (v: Visibility): void => { riseText.visible = v.kind === "risesSets"; setText.visible = v.kind === "risesSets"; transitText.visible = v.kind === "risesSets" || v.kind === "circumpolar"; circumpolarText.visible = v.kind === "circumpolar"; neverRisesText.visible = v.kind === "neverRises"; - }); + }; + visibilityProperty.link(onVisibility); - coordsProperty.link((coords) => { + const onCoords = (coords: SelectionCoords): void => { const hasSelection = coords.kind !== "none"; noneText.visible = !hasSelection; nameText.visible = hasSelection; @@ -376,14 +414,17 @@ export class SelectedObjectReadout extends Node { magText.visible = hasSelection; eqText.visible = hasSelection; hzText.visible = hasSelection; - }); + }; + coordsProperty.link(onCoords); // Elongation only applies to planets (and the Moon), never a bare star or the Sun. - elongationProperty.link((elong) => { + const onElongation = (elong: { deg: string; dir: string } | null): void => { elongationText.visible = elong !== null; - }); + }; + elongationProperty.link(onElongation); // ── Track toggle (enabled only when an object is selected) ──────────────── + const trackEnabledProperty = new DerivedProperty([model.selectedObjectProperty], (s) => s !== null); const trackCheckbox = new Checkbox( model.trackSelectedObjectProperty, new Text(controls.trackSelectedStringProperty, { @@ -393,7 +434,9 @@ export class SelectedObjectReadout extends Node { }), { ...ZENITH_CHECKBOX_OPTIONS, - enabledProperty: new DerivedProperty([model.selectedObjectProperty], (s) => s !== null), + // Not owned by the Checkbox (EnabledComponent only disposes an + // enabledProperty it created), so dispose() below releases it. + enabledProperty: trackEnabledProperty, accessibleName: a11y.controls.trackSelectedStringProperty, }, ); @@ -420,5 +463,78 @@ export class SelectedObjectReadout extends Node { ], }), ); + + // ── Teardown, in dependency order ───────────────────────────────────────── + // 1. Internal listeners on Properties we are about to dispose. + this.disposers.push(() => { + visibilityProperty.unlink(onVisibility); + coordsProperty.unlink(onCoords); + elongationProperty.unlink(onElongation); + nameMultilink.dispose(); + }); + // 2. Pattern Properties (their Text readers are disposed by dispose() first). + this.disposers.push(() => { + for (const patternProperty of patterns) { + patternProperty.dispose(); + } + }); + // 3. Derived Properties that fed those patterns, leaf-most first… + this.disposers.push(() => { + for (const property of [ + magProperty, + raProperty, + decProperty, + altProperty, + azProperty, + typeNameProperty, + constellationNameProperty, + constellationSourceProperty, + elongDegProperty, + elongDirProperty, + riseTimeProperty, + riseAzProperty, + riseClockProperty, + setTimeProperty, + setAzProperty, + setClockProperty, + transitTimeProperty, + transitAltProperty, + transitClockProperty, + ]) { + property.dispose(); + } + }); + // 4. …then the bases they derived from, and the standalone Properties. + this.disposers.push(() => { + coordsProperty.dispose(); + visibilityProperty.dispose(); + elongationProperty.dispose(); + trackEnabledProperty.dispose(); + nameProperty.dispose(); + }); + } + + /** + * Releases this panel's axon graph. `Node.dispose()` only detaches children, so + * the readout's Texts, its pattern Properties, and the ~20 DerivedProperties + * behind them would otherwise stay subscribed to the model and the shared + * StringManager after the screen is torn down. Children are disposed between + * stages 1 and 2 so no Text is still reading a pattern when it is released. + * Idempotent — the disposer list is drained on the first call. + */ + public override dispose(): void { + const [detachListeners, disposePatterns, ...rest] = this.disposers.splice(0); + detachListeners?.(); + + const children = this.children; + super.dispose(); + for (const child of children) { + child.disposeSubtree(); + } + + disposePatterns?.(); + for (const disposeStage of rest) { + disposeStage(); + } } } diff --git a/src/zenith-screen/view/ZenithScreenView.ts b/src/zenith-screen/view/ZenithScreenView.ts index afd68b9..48006b6 100644 --- a/src/zenith-screen/view/ZenithScreenView.ts +++ b/src/zenith-screen/view/ZenithScreenView.ts @@ -62,6 +62,7 @@ const SELECTION_PANEL_BOTTOM_CLEARANCE = 52; export class ZenithScreenView extends ScreenView { private readonly skyNode: PlanetariumSkyNode; + private readonly selectedReadout: SelectedObjectReadout; private readonly searchNode: ObjectNameSearch; private readonly controlPanel: AccordionBox; private readonly timePanel: TimeControlPanel; @@ -184,7 +185,7 @@ export class ZenithScreenView extends ScreenView { }, ); - const selectedReadout = new SelectedObjectReadout(model); + this.selectedReadout = new SelectedObjectReadout(model); const checkbox = ( property: typeof model.showGridProperty, @@ -419,7 +420,7 @@ export class ZenithScreenView extends ScreenView { this.addChild(this.skyNode); // Selection readout sits over the sky (bottom-left). - const selectionPanel = new ZenithPanel(selectedReadout, { + const selectionPanel = new ZenithPanel(this.selectedReadout, { xMargin: 8, yMargin: 6, }); @@ -500,8 +501,24 @@ export class ZenithScreenView extends ScreenView { this.searchNode.centerX = visibleBounds.centerX; this.searchNode.top = visibleBounds.minY + SCREEN_VIEW_MARGIN; - selectionPanel.left = visibleBounds.minX + SELECTION_PANEL_INSET; + // Selection readout: bottom-left over the sky. The left column (Location + + // Time) can reach the bottom of the window at common sizes, which would + // leave the readout painted over by the Time panel — so when they would + // overlap, slide it clear to the right of that column instead. + // + // `left` is computed once and assigned once: this runs re-entrantly from + // selectionPanel.boundsProperty, so writing it twice (default, then the + // shifted value) would notify back into here forever. Anchoring `bottom` + // first is safe — the overlap test reads `top`, which depends on `bottom` + // and the panel height, never on `left`. selectionPanel.bottom = safeBottom - SELECTION_PANEL_BOTTOM_CLEARANCE; + const selectionClearsLeftColumn = selectionPanel.top >= this.timePanel.bottom + RESET_ALL_PANEL_GAP; + selectionPanel.left = selectionClearsLeftColumn + ? visibleBounds.minX + SELECTION_PANEL_INSET + : Math.min( + this.timePanel.right + RESET_ALL_PANEL_GAP, + visibleBounds.maxX - SELECTION_PANEL_INSET - selectionPanel.width, + ); resetAllButton.right = visibleBounds.maxX - SCREEN_VIEW_MARGIN; resetAllButton.bottom = safeBottom - SCREEN_VIEW_MARGIN; @@ -577,6 +594,7 @@ export class ZenithScreenView extends ScreenView { dispose(); } this.skyNode.dispose(); + this.selectedReadout.dispose(); } public override step(_dt: number): void { diff --git a/tests/ZenithModel.test.ts b/tests/ZenithModel.test.ts index 3ed0e96..468fce6 100644 --- a/tests/ZenithModel.test.ts +++ b/tests/ZenithModel.test.ts @@ -10,6 +10,7 @@ import { equatorialToHorizontal } from "../src/common/sky/SkyCoordinates.js"; import { ZenithPreferencesModel } from "../src/preferences/ZenithPreferencesModel.js"; import { CIVIL_HOURS_PER_SIM_SECOND, + CIVIL_TIME_MS_RANGE, DEFAULT_CIVIL_TIME_MS, DEFAULT_FIELD_OF_VIEW_DEG, DEFAULT_LATITUDE_DEG, @@ -287,4 +288,33 @@ describe("ZenithModel", () => { expect(model.epochPresetProperty.value).toBe(EpochPreset.CUSTOM); expect(model.civilTimeMsProperty.value).toBe(Date.UTC(2020, 0, 1, 3, 0, 0)); }); + + it("keeps the epoch preset selected while the clock ticks within the readout's precision", () => { + // The timer starts playing, so the first step advances civil time by a few + // milliseconds. That must not flip the combo to CUSTOM while the UTC readout + // (minute precision) still shows the preset epoch. + model.epochPresetProperty.value = EpochPreset.DECEMBER_SOLSTICE; + model.step(1 / 60); + expect(model.epochPresetProperty.value).toBe(EpochPreset.DECEMBER_SOLSTICE); + }); + + it("marks epoch CUSTOM once the clock has moved a full minute off the preset", () => { + model.epochPresetProperty.value = EpochPreset.DECEMBER_SOLSTICE; + model.advanceCivilTimeHours(1 / 60); + expect(model.epochPresetProperty.value).toBe(EpochPreset.CUSTOM); + }); + + it("holds civil time inside the supported year range", () => { + model.setCivilTimeMs(CIVIL_TIME_MS_RANGE.max + 5 * 365 * 24 * 3600 * 1000); + expect(model.civilTimeMsProperty.value).toBe(CIVIL_TIME_MS_RANGE.max); + + model.setCivilTimeMs(CIVIL_TIME_MS_RANGE.min - 5 * 365 * 24 * 3600 * 1000); + expect(model.civilTimeMsProperty.value).toBe(CIVIL_TIME_MS_RANGE.min); + }); + + it("stops the clock at the range edge instead of playing past it", () => { + model.setCivilTimeMs(CIVIL_TIME_MS_RANGE.max); + model.advanceCivilTimeHours(48); + expect(model.civilTimeMsProperty.value).toBe(CIVIL_TIME_MS_RANGE.max); + }); }); diff --git a/tests/memory-leak.test.ts b/tests/memory-leak.test.ts index b4214da..5aa4a53 100644 --- a/tests/memory-leak.test.ts +++ b/tests/memory-leak.test.ts @@ -9,6 +9,7 @@ import { TimeModel } from "../src/common/TimeModel.js"; import { ZenithPreferencesModel } from "../src/preferences/ZenithPreferencesModel.js"; import { ZenithModel } from "../src/zenith-screen/model/ZenithModel.js"; import { PlanetariumSkyNode } from "../src/zenith-screen/view/PlanetariumSkyNode.js"; +import { SelectedObjectReadout } from "../src/zenith-screen/view/SelectedObjectReadout.js"; import { ZenithScreenView } from "../src/zenith-screen/view/ZenithScreenView.js"; async function forceGC(earlyExitRef?: WeakRef): Promise { @@ -156,6 +157,50 @@ describe("Memory leak regression", () => { }); }); + describe("SelectedObjectReadout", () => { + it("is collected after dispose", async () => { + const ref = (() => { + const preferences = new ZenithPreferencesModel(); + const model = new ZenithModel(preferences); + model.timer.isPlayingProperty.value = false; + const readout = new SelectedObjectReadout(model); + const weak = new WeakRef(readout); + readout.dispose(); + model.dispose(); + return weak; + })(); + await forceGC(ref); + expect(ref.deref()).toBeUndefined(); + }); + + it("stops observing the model after dispose", () => { + const preferences = new ZenithPreferencesModel(); + const model = new ZenithModel(preferences); + model.timer.isPlayingProperty.value = false; + const readout = new SelectedObjectReadout(model); + readout.dispose(); + + // The readout derives coordinates, rise/set times and the constellation + // name from these; after dispose none of that may still be listening. + expect(() => { + model.civilTimeMsProperty.value += 3_600_000; + model.latitudeProperty.value = -33.9; + model.selectedObjectProperty.value = null; + }).not.toThrow(); + + model.dispose(); + }); + + it("double dispose() does not throw", () => { + const preferences = new ZenithPreferencesModel(); + const model = new ZenithModel(preferences); + const readout = new SelectedObjectReadout(model); + readout.dispose(); + expect(() => readout.dispose()).not.toThrow(); + model.dispose(); + }); + }); + describe("ZenithScreenView", () => { // Full ScreenView GC is not asserted here: joist `ScreenView` plus sun // ComboBox / NumberControl / localized string Properties retain a graph that diff --git a/tests/zenithQueryParameters.test.ts b/tests/zenithQueryParameters.test.ts index 622d1fc..12e42df 100644 --- a/tests/zenithQueryParameters.test.ts +++ b/tests/zenithQueryParameters.test.ts @@ -10,7 +10,7 @@ import { parseCivilDateQueryParam, resolveCivilTimeMsFromQuery, } from "../src/preferences/zenithQueryParameters.js"; -import { DEFAULT_CIVIL_TIME_MS } from "../src/ZenithConstants.js"; +import { CIVIL_TIME_MS_RANGE, DEFAULT_CIVIL_TIME_MS } from "../src/ZenithConstants.js"; describe("zenithQueryParameters date helpers", () => { it("treats empty or null date as the sim default civil epoch", () => { @@ -33,4 +33,22 @@ describe("zenithQueryParameters date helpers", () => { expect(isValidCivilDateQueryParam("not-a-date")).toBe(false); expect(parseCivilDateQueryParam("not-a-date")).toBeNull(); }); + + it("rejects parseable dates outside the supported civil year range", () => { + // Accepting these would let the date-jump spinners clamp them to the range + // boundary, so a shared link would silently render a different sky. + for (const outOfRange of ["1850-01-01T00:00:00Z", "2150-06-01T00:00:00Z"]) { + expect(isValidCivilDateQueryParam(outOfRange)).toBe(false); + expect(parseCivilDateQueryParam(outOfRange)).toBeNull(); + expect(resolveCivilTimeMsFromQuery(outOfRange)).toBe(DEFAULT_CIVIL_TIME_MS); + } + }); + + it("accepts dates at both ends of the supported range", () => { + for (const ms of [CIVIL_TIME_MS_RANGE.min, CIVIL_TIME_MS_RANGE.max]) { + const iso = new Date(ms).toISOString(); + expect(isValidCivilDateQueryParam(iso)).toBe(true); + expect(resolveCivilTimeMsFromQuery(iso)).toBe(ms); + } + }); });