Skip to content

TypeScript migration: src/ is now 100% TypeScript (Phases 0–6, #924)#925

Open
haslinghuis wants to merge 58 commits into
betaflight:masterfrom
haslinghuis:ts-migration-phase0-tooling
Open

TypeScript migration: src/ is now 100% TypeScript (Phases 0–6, #924)#925
haslinghuis wants to merge 58 commits into
betaflight:masterfrom
haslinghuis:ts-migration-phase0-tooling

Conversation

@haslinghuis

@haslinghuis haslinghuis commented May 31, 2026

Copy link
Copy Markdown
Member

Summary

Completes the TypeScript migration of the blackbox-log-viewer (#924). src/ is now 100% TypeScript — every module converted .js.ts and all 33 Vue components moved to <script setup lang="ts">, type-checked clean by vue-tsc. Delivered as a single PR (mirroring the original Vue/Pinia migration), covering Phases 0–6.

The conversion is behaviour-preserving: no runtime logic was changed (Option-A conversions — typed constructor + instance interface — that git detects as renames; only type-level casts/assertions added), and parser equivalence is proven byte-for-byte by a golden-file characterization test.

Phase Scope
0 Tooling — tsconfig, ESLint (TS scoped to src/**/*.ts), type-check script, CI step
2 Framework-agnostic core — flightlog_parser, flightlog, datastream/decoders, fft_complex, flightlog_fielddefs, presenters, core utils
1 Pinia state architecture — extracted useBlackboxViewer() composable, collapsed the 40-callback store bridge
3 Pinia stores → TS (typed flightLog, refs, action params)
4 All 33 Vue components → <script setup lang="ts">
5 Canvas-renderer + Vue/DOM-glue layer → TS
6 Retire the intentional Loose = any data surfaces with real types (graph-config model, FlightLog returns, frame data)

Guarantees

  • Parser, FlightLog & exports proven equivalentnpm test (in CI) runs golden-file characterization suites: the parser (sysConfig/frame output), FlightLog (the computed-field pipeline — attitude/IMU, PID sum/error, RC scaling, GPS coord/distance/azimuth, exercising imu.ts + gps_transform.ts), the CSV/GPX/spectrum exporters (worker output + nested-vs-flat postMessage protocol equivalence), and ExpoCurve. All pass unchanged across the migration.
  • Gates greentype-check (vue-tsc, 0 errors) · lint · npm test (parser + flightlog + export + expo goldens) · build.

Review feedback addressed

  • CodeRabbit — all inline findings fixed (hardened the latent behaviours the type-only casts had masked: A/motor divide-by-zero, altitude conversion factor, parser byte type, getMin/MaxTime false sentinels, null canvas context, comma/time parsing), plus the round-2 simple-stats === false guard.
  • SonarCloud — quality gate passes; ~155 safe new-code smells cleared (mechanical autofix + per-site fixes). Remaining smells are the deliberate Loose = any marker (Sonar wants raw any, which ESLint no-explicit-any forbids) and structural refactors of untested renderer code — recommended as a separate post-merge issue.
  • Cleanup — dropped allowJs/checkJs now that no .js is part of the program.

TypeScript coverage

The codebase is TypeScript end-to-end — the only non-TS file is the vendored public/js/three.min.js. src/ (incl. the export Web Workers, now TS Vite module workers in src/workers/), the build/tooling config (eslint.config.ts via jiti, vite.config.ts), and the whole test suite (test/*.ts) are TypeScript. The untyped THREE / L (<script>-loaded globals) are declared any in env.d.ts.

On upgrading three.js: the modern three npm package ships built-in TS types — but the vendored copy is r70 (2015), and craft_3d uses APIs removed since (THREE.Geometry, Path, Shape, ExtrudeGeometryGeometry was dropped in r125). Adopting the typed package is a ~90-revision upgrade that would require rewriting the 3D craft renderer for the modern BufferGeometry API, not a typing change — so three.min.js stays vendored and <script>-loaded for now (see follow-up).

Remaining Loose = any

The three deferred data surfaces are now typed: SysConfig for the header bag (HeaderDialog), VideoOptions/VideoLogParameters/VideoExportEvents for video export, and PenGraph/PenField/PenDefault for pen adjustments (the legacy default array-as-object typed as an array carrying optional smoothing/power, so it serializes unchanged). The Loose markers that remain are genuinely dynamic and stay by design: the <script>-loaded THREE/L globals, the untyped webm-writer, frame data indexed dynamically by field index, and the grapher's runtime-adapted graphs (whose curve becomes an ExpoCurve instance at render time).

Runtime verification

A manual app pass was done (load log → play → zoom → sync → export → workspaces). Gates prove the build + parser but not the running UI, so this surfaced 4 issues — all fixed:

  • Video export crashed on click (FlightLogAnalyser.resize dereferenced a null parentElement in the off-DOM export grapher) → guarded (b5c69b7).
  • CSV/GPX export froze the main thread ~10–24 s (pre-existing — postMessage structure-cloned the nested frames). Now flattened to a transferable Float64Array; output verified byte-identical (07782f6).
  • PSD spectrum parameters didn't hide when the chart was hidden via a legend-line click (hasAnalyserFullscreen wasn't reset) → mirrored the toolbar toggle (39a19c8).
  • Earlier Phase-1 regression (analyser-canvas init timing) was already found and fixed.

Follow-up work (post-merge)

Deliberately out of scope for this behaviour-preserving migration; tracked here so nothing is lost. Recommend separate issues:

  • SonarCloud structural refactors (left untouched — risky in untested renderer/parser code): cognitive-complexity reductions (S3776, up to 129), switchif / case-count restructures (S1301/S1479), parameter-count reductions (S107), duplicate-branch merges (S1871). The Loose = any marker itself (S6564) needs a non-any representation if Sonar is to be satisfied, since ESLint no-explicit-any forbids the fix Sonar suggests.
  • Behaviour-risk style items — defer until there is test coverage to back them: negated-condition flips (S7735), throw new Error(...) instead of throwing values (S3696), Math.trunc/Math.hypot swaps (S7767/S7769, numeric edge cases), FileReaderBlob.text()/arrayBuffer() (S7756).
  • Upgrade three.js r70 → modern three (npm, typed) — drops the last vendored JS and types craft_3d, but requires rewriting it for the modern BufferGeometry API (the r70 Geometry/Path/Shape/ExtrudeGeometry calls were removed upstream). A real upgrade, not a typing pass.
  • Renderer golden coverageFlightLog's computed fields are now golden-covered, but the Canvas renderers (grapher/sticks/craft/spectrum-plot) still aren't: they couple to Pinia stores + ThemeColors (getComputedStyle/DOM) + a canvas context, so a headless golden needs a mock 2D context plus store/DOM stubs (or light DI to decouple them). Worth doing to lock the draw logic.

Done in this PR (were follow-ups): a CSV/GPX/spectrum export golden test (test/export.golden.test.ts) that locks the output format and proves the nested-vs-flat protocol equivalence; the export Web Workers migrated to TypeScript Vite module workers (src/workers/, byte-identical output); vite.config.ts, eslint.config.ts (via jiti) and the golden-test harness converted to TypeScript; the dead browser smoke test replaced by test/expo.test.ts; the three intentional Loose = any data surfaces (sysConfig header bag, video-export options, pen-adjustment default) retired with real types; and a FlightLog computed-field golden (test/flightlog.golden.test.ts) covering the attitude/PID/RC/GPS pipeline (imu + gps_transform).

📖 Full phase-by-phase log (original running description)

Per-phase running log, kept for history. Superseded note: the later "vendor.js kept .js" mentions no longer apply — vendor.js was converted to vendor.ts in the final tooling commit, so src/ is now 100% TypeScript with no .js files remaining.

Phase 0 — Tooling foundation

  • tsconfig.jsonallowJs: true + checkJs: false (existing JS consumed but not yet type-checked), strict: true for new TS, noEmit (Vite/esbuild transpiles; tsc & vue-tsc only type-check), moduleResolution: bundler, @/*./src/* alias mirroring Vite.
  • src/env.d.ts*.vue shims, the __APP_VERSION__ Vite define, and <script>-loaded globals (THREE, L, chrome).
  • eslint.config.mjs — added typescript-eslint, scoped to src/**/*.ts only so the JS ruleset is unchanged; shared stylistic rules factored into one const.
  • package.json — new type-check script (vue-tsc --noEmit) + devDeps typescript, vue-tsc, typescript-eslint.
  • .github/workflows/build.yml — run type-check in CI before the build.

Phase 2 — framework-agnostic core (bottom-up)

flightlog_fielddefs.js.ts

Pure type annotations, no logic change (97% rename similarity): makeReadOnly<T>, live-binding mutable exports typed string[], adjustFieldDefsList(firmwareType: number, firmwareVersion: string). Added @types/semver.

datastream.js + decoders.js.ts

  • ArrayDataStream: prototype-constructor → class. EOF stays on the prototype (declared via declare).
  • decoders: separate side-effect module; methods added via module augmentation so import "./decoders" is unchanged.
  • Runtime-verified (esbuild+node, 19 assertions): codecs, VB/ZigZag, nextOffsetOf, prototype.EOF, decoder methods — all pass.

fft_complex.js.ts

  • FFTComplex: prototype-constructor → class (state typed via FFTState); defensive undefined ctor guard preserved via optional params.
  • butterfly2/3/4, generic butterfly, and work() typed; FFTArray = Float64Array | number[], FFTBuffer for the {data,offset,stride} structs.
  • No logic change. Sole consumer (graph_spectrum_calc) needs no changes.
  • Runtime-verified against a naive DFT (esbuild+node) across n=5 (generic butterfly), n=6 (butterfly3), n=8 (butterfly4), complex + real modes — max error ~1e-14, guard throws as before.

Verification

Check Result
npm run type-check ✅ pass
npm run lint ✅ pass (JS rules unchanged)
npm run build ✅ pass

Part of

Refs #924 — Phase 0 (tooling) + Phase 2 (flightlog_fielddefs, datastream/decoders, fft_complex). Next: the large parsers — flightlog_parser, then flightlog.

Pre-work for the parser conversion — golden-file test

flightlog_parser (1900 lines) has no test suite, so before converting it a golden-file characterization test was added: deterministic synthetic logs are parsed and their sysConfig/frame-defs/stats/onFrameReady output is compared to committed snapshots (npm run test:parser, wired into CI). Two fixtures: a simple one (I/P frames, INC/PREVIOUS predictors, VB encodings) and a complex one covering GPS home/GPS frames (HOME_COORD predictor pair), slow + event frames (incl. End-of-log), and the TAG2_3S32 / TAG8_8SVB / NEG_14BIT encodings. After the parser is converted to TS these must pass unchanged, proving byte-for-byte equivalence. esbuild promoted to an explicit devDependency (used by the bundled test).

flightlog_parser.js.ts (the big one — 1900 lines)

Converted via Option A (typed constructor function + instance interface), no logic change — git detects it as a rename. New shared flightlog_types.ts (SysConfig with index signature + typed hot fields, FrameDef/FrameArray/Stats, event + callback types) reused by flightlog.js next. this: FlightLogParser on the constructor (inner arrows inherit it); header handlers typed via a HeaderHandler alias; dynamic sysConfig/event-data access kept as any (faithful to the original) behind localized eslint-disable; non-null assertions only where the algorithm guarantees presence. Consumers use extensionless/.js imports → no changes.

Proven equivalent: both parser golden snapshots pass unchanged after the conversion — byte-for-byte identical behavior across every frame type and encoding.

flightlog.js.ts (1844 lines — completes Phase 2 core)

Converted via Option A, no logic change (rename). Declared the FlightLog instance interface (~43 methods, this-typed constructor), added shared FlightLogChunk and local ComputeCtx/IframeDirectory/LogStats types for the computed-field pipeline, chunk caching, and smoothing. OnFrameReady's frame loosened to any (genuinely polymorphic). Only non-behavioural casts/assertions used (incl. as unknown as to construct the typed-this parser). Sole importer is main.js (extensionless) → no changes. Diff verified type-only; type-check/lint/build + the parser golden test all pass.

Phase 2 framework-agnostic core is now fully TypeScript: flightlog_fielddefs, datastream/decoders, fft_complex, flightlog_parser, flightlog.

Core dependency utilities → .ts

Converted the pure-logic files the parser/flightlog depend on, so the algorithmic core's import graph is fully typed:

  • tools.ts (sign-extend/VB/binary-search/formatting/firmware helpers + canvas/DOM utils),
  • cache.ts (FIFOCache), gps_transform.ts, imu.ts (now returns a typed Attitude), flightlog_index.ts — all Option A, no logic change.
  • shared flightlog_types.ts gained LogStats + IntraIndex, now used by both flightlog_index (producer) and flightlog (consumer).

Type-only / runtime-equivalent (verified by diff); type-check, lint, build, and the parser golden test all pass.

Small pure-logic utils → .ts

expo (ExpoCurve), configuration, simple-stats, csv-exporter, gpx-exporter, spectrum-exporter, export_utils, workspace_io, user_settings_data — Option A / annotation, no logic change. All gates green.

graph_spectrum_calc (929L) + flightlog_fields_presenter (3130L) → .ts

The last two large pure-logic files. Both are namespaces/singletons of static methods, typed via an interface + cast (methods get contextual this/param/return typing); large lookup tables typed as Record. No logic change. The entire framework-agnostic / pure-logic core is now TypeScript. Remaining .js is the canvas-renderer + Vue/DOM-glue layer (grapher, graph_config, sticks, main.js, …), which converts alongside the Pinia/component work (Phases 1/3/4).

Phase 1 — finish the Pinia migration (state architecture)

Extracted a useBlackboxViewer() composable (singleton) that owns the renderer instances (graph/seekBar/mapGrapher/video) and all imperative operations; main.js is now a 5-line bootstrap. Collapsed the legacy callback bridge: 40 component-facing shallowRef(null) callbacks (spectrum, app-level loadFiles/exports/refreshGraph/settings, graph zoom/pen/field ops, playback play/jump/sync/rate, workspace switch/save/default/bookmark) moved out of the stores into the composable's returned ops. Consumers (App.vue, LegendPanel, SeekBarToolbar, SpectrumAnalyser, dark_theme) now call the composable directly; the stores are now (almost) pure state. Residual: store-internal invalidateGraph/updateCanvasSize (graph-store legend actions) and renderer-instance refs (legitimate reactive state).

Runtime verification pending: this rewires the app's state/bootstrap. type-check/lint/build/parser-golden all pass, but a green build does not prove the running app — it needs an app run (load log → play → zoom → sync → export → workspaces). Per the agreed implement-then-fix-later approach.

Phase 3 — Pinia stores → .ts

All 6 stores + pinia_instance converted. The log store's flightLog is now typed FlightLog | null, so getMinTime/hasGpsData/getSysConfig etc. flow typed into consumers; playback/app/workspace/settings stores have typed refs + action params. Renderer/config instance refs in the graph store use a Loose alias until grapher/graph_config convert. Consumers use .js store specifiers → no changes. No logic change.

Phase 1 runtime regression (analyserCanvas init timing) was found and fixed — confirmed working.

Phase 4 — Vue components → <script setup lang="ts">

All 33 .vue components now use <script setup lang="ts"> and the project type-checks cleanly (vue-tsc 0 errors). Converted in waves (small → medium → the large dialogs last):

  • Type-based defineProps<…>() / withDefaults and defineModel<T>(); typed event handlers (Event/MouseEvent/DragEvent/KeyboardEvent, e.target as HTMLInputElement) and template refs (ref<HTMLElement | null>, typed Set<string>).
  • Interfaces for component-local view-models (PID rows, feature/config lines, legend fields, graph/field config in GraphConfigDialog, HeaderParam in HeaderDialog).
  • Where a component still talks to the JS renderer layer (grapher / graph_config / analyser instances) or to a genuinely-dynamic map (the sysConfig flight-log header, hundreds of optional fields), access is kept behind a localized Loose = any alias — these tighten automatically once the renderer layer is converted.

No behavioural change (template markup untouched; only the <script> block typed). Each wave verified with type-check + lint + build.

Status: Phases 0–4 are now complete. The remaining .js is the canvas-renderer + DOM-glue layer (grapher, graph_config, graph_spectrum_plot, graph_map, sticks, craft_2d/3d, seekbar, video renderer, and the use_blackbox_viewer composable + small glue modules) — a natural follow-up that would let the Loose aliases above be dropped.

Check Result
npm run type-check (vue-tsc) ✅ 0 errors
npm run lint ✅ pass
npm run test:parser (golden) ✅ both snapshots pass
npm run build ✅ pass

Phase 5 — renderer + DOM-glue layer (in progress)

The final zone: the framework-agnostic Canvas renderers and the Vue/DOM glue. Converting it is what lets the Loose = any renderer-instance aliases in the stores/components be dropped.

graph_config.js.ts (1692L) — Option A (rename, no logic change). export interface GraphConfig types the instance (the constructor's this); the value GraphConfig carries the static helpers (PALETTE, load, getDefault*ForField, the getMinMax* statics, getExampleGraphConfigs). Added local Curve/MinMax/ExampleGraph types; the free-form, user-editable graph/field config stays behind a localized Loose = any (it's shared with the still-JS grapher). ConvertFieldValue results cast to number where the min/max is numeric (type-only); notifyListeners became a lexical arrow to drop the this-alias (runtime identical). The sole new GraphConfig() lives in still-JS use_blackbox_viewer.js, so the typed-this construct-signature caveat doesn't surface; TS consumers (the GraphConfigDialog.vue statics, graph-store instance access) are unchanged. Type-check 0 / lint / build / parser-golden all pass.

grapher.js.ts (1282L) — the main trace renderer (FlightLogGrapher). Option A (rename, no logic change); export interface FlightLogGrapher types the instance, the value is the constructor. graphConfig is now typed against the converted GraphConfig interface (a first concrete cross-module type in the renderer layer); flightLog, the renderer sub-objects (sticks/craft/analyser/lapTimer), the per-field config entries and the options bag stay behind a localized Loose = any. Type-only/behaviour-preserving touch-ups for JS-isms strict TS flags: getContext("2d")!; regex-group/toFixed() string→number made explicit via Number(...) (same coercion as before); number|false in/out markers cast to number where already guarded; ConvertFieldValue min/max cast to number; new ExpoCurve(...) via a construct cast (Option-A typed-this fn) and FlightLogSticks cast to Loose to keep its long-standing dead 4th arg; the bookmark loop reads its element into a local for null-narrowing. FlightLogGrapher is only new'd in still-JS files. Type-check 0 / lint / build / parser-golden all pass.

graph_spectrum_plot.js + graph_spectrum.js.ts (1925L + 370L) — the FFT/PSD plotting layer, consuming the already-typed graph_spectrum_calc.ts.

  • graph_spectrum_plot.ts: the GraphSpectrumPlot singleton, typed via the same idiom as graph_spectrum_calc — a GraphSpectrumPlotType interface plus the object literal cast as unknown as GraphSpectrumPlotType, so the ~50 methods assigned afterward get contextual this/param/return typing with no body annotations. Canvas contexts / fftData / sysConfig / imported-curve helpers stay Loose; scalars typed precisely. Touch-ups: heatmap getContext(...)!; the two cache-canvas fields kept Loose (passed straight to canvas APIs); two genuinely-unused grid-line params underscored.
  • graph_spectrum.ts: the FlightLogAnalyser constructor (Option A — interface + typed-this). The debounce-wrapped setters annotate their inner value: number (debounce doesn't contextually type the callback); parseInt(value) on numeric inputs made explicit via parseInt(String(value), 10) (behaviour-identical to the JS coercion); parentElement! assumed present; the const that = this alias (needed by the debounced callbacks) kept behind a localized eslint-disable.
  • grapher.ts: new FlightLogAnalyser(...) via a construct cast (now an Option-A typed-this fn). env.d.ts: declare the untyped throttle-debounce module.

Type-check 0 / lint / build / parser-golden all pass.

graph_map.js + seekbar.js.ts (426L + 382L) — the GPS map renderer and the seek bar. Both Option A (rename, no logic change): an instance interface (MapGrapher / SeekBar) types the constructor's this. Leaflet objects, flightLog, frame coordinates and the activity arrays stay behind a localized Loose = any; scalars and DOM handlers (MouseEvent/TouchEvent, width/height) typed precisely.

  • env.d.ts: L (Leaflet) retyped from unknown to any (eslint-disabled) — it's a rich untyped global used throughout graph_map.
  • seekbar: canvas + background getContext("2d")!; dirtyRegion typed false | DirtyRegion so the cursor dirty-rect narrows in repaint; in/out times number | false; the that = this alias (used by the drag/seek callbacks) kept behind a localized eslint-disable.
  • user_settings_data: added the missing mapTrailAltitudeColored: false default — a real setting bound by a switch in UserSettingsDialog.vue but absent from the defaults object; false is behaviour-identical to the prior undefined (both falsy) and gives graph_map a typed read.

Both constructors are only new'd in still-JS files. Type-check 0 / lint / build / parser-golden all pass.

sticks.js + craft_2d.js + craft_3d.js.ts (351L + 285L + 331L) — the stick-position overlay and the 2D/3D craft overlays. All Option A (rename, no logic change): an instance interface (FlightLogSticks / Craft2D / Craft3D) types each constructor's this. flightLog, frame data, prop colours, the craft-parameter objects and the three.js scene graph stay behind a localized Loose = any; scalar args (width/height/indices) typed precisely.

  • env.d.ts: THREE (three.js) retyped unknownany (eslint-disabled), like Leaflet L — a rich untyped global used throughout craft_3d.
  • sticks: getContext("2d")!; new ExpoCurve(...) via a construct cast (ExpoCurve is an Option-A typed-this fn).
  • craft_2d/craft_3d: getContext("2d")!; customMix typed Loose — the settings store types its default as null, which narrows the user-mix branch to never, so Loose restores the runtime shape.
  • grapher.ts: new Craft2D(...) / new Craft3D(...) via construct casts (both now Option-A typed-this fns; FlightLogSticks was already cast).

Type-check 0 / lint / build / parser-golden all pass.

flightlog_video_renderer.js + the small renderer helpers → .ts — completes the renderer layer.

  • flightlog_video_renderer.ts (306L): Option A + a static isSupported; new FlightLogGrapher(...) via a construct cast; the vendor-prefixed document.{moz,ms,webkit}Hidden props and the dynamic document[hidden] index cast to Loose; getContext("2d")!; toDataURL's pre-existing object quality arg cast. env.d.ts: declare the untyped webm-writer module.
  • theme_colors.ts (52L): singleton object literal; colorCache typed Record<string, string> so keyed access type-checks.
  • values_display.ts (107L): free-form stores/log/frame as Loose; decodeFieldToFriendly's null flightLog/flightMode args cast (the presenter types them non-null).
  • laptimer.ts (179L): Option A; lapTime fields number | null; formatTime tolerates null at runtime (Math.abs coerces) so the number | null args are cast as number (type-only); getContext("2d")!.
  • pen_adjustment.ts (148L): pure functions; graphs/group/field/delta Loose, returns string | null.
  • graph_imported_curves.ts (164L): Option A; that-alias kept (used by FileReader callbacks); catch (e)(e as Error).message.

Construct casts added in already-TS callers: grapher.ts (new LapTimer), graph_spectrum_plot.ts (new ImportedCurves ×2), VideoExportDialog.vue (new FlightLogVideoRenderer).

The entire Canvas-renderer layer is now TypeScript. Type-check 0 / lint / build / parser-golden all pass.

Vue/DOM-glue layer → .tsmain, vue_init, nuxt_ui_router, screenshot, pref_storage, video_handler, log_lifecycle, dark_theme, keyboard_handler, playback_controls.

  • pref_storage.ts: Option A + interface; chrome retyped any (env.d.ts); keyPrefix defaults ""; catch (e)(e as Error).message.
  • video_handler/log_lifecycle/playback_controls: plain function modules. The now-typed store fields — logStore.flightLog (FlightLog | null) and playbackStore.videoElement (HTMLVideoElement | null) — are dereferenced via (x as Loose) / x! at the call sites (they run only when a log/video is loaded); flightlog.ts itself uses the same as number pattern for getMinTime/getMaxTime (number | false).
  • dark_theme.ts: singleton literal; prefs/mediaQuery cast Loose; typed method params.
  • keyboard_handler.ts: ctx Loose; letterKeyHandlers typed Record<string, (e, shifted) => void> so the inline handlers get contextual params; returned listener (e: Loose).
  • screenshot/nuxt_ui_router/main/vue_init: trivial.
  • vendor.js kept .js — it's 3 side-effect Leaflet imports, and the Leaflet.MultiOptionsPolyline plugin's bundled .d.ts globally augments L; keeping vendor as JS avoids that narrowing the ambient L across the program.

Ripple construct casts in already-TS callers (PrefStorage is now an Option-A typed-this fn): graph_spectrum.ts, stores/graph.ts, stores/settings.ts. graph_map.ts: the ambient global L (narrowed by the plugin .d.ts) cast Loose at its use sites, kept lazy.

Renderer + glue layers are now TypeScript. Only the use_blackbox_viewer composable remains (plus the trivial vendor.js). Type-check 0 / lint / build / parser-golden all pass.

composables/use_blackbox_viewer.js.ts (727L) — the orchestrator that owns the renderer instances and populates the imperative-ops bridge. Typing const ops = {} as BlackboxViewerOps makes each ops.X = (...) => assignment checked against the interface, and the arrow params pick up contextual types for free.

  • All renderer/util constructors it news (FlightLogGrapher, MapGrapher, SeekBar, GraphConfig, FlightLog, PrefStorage, Configuration, ConfigurationDefaults) are Option-A typed-this fns → constructed via casts.
  • DOM lookups cast to element types; flightLog (FlightLog | null) dereferenced via (x as Loose) at call sites; graph / userSettings kept Loose; boundary casts where the typed stores/helpers don't structurally match the hand-written param types; parseFloat/parseInt args String()-wrapped (ToString-identical); launchQueue via (globalThis as Loose).
  • Honest signature widenings the composable's real call shapes required: setGraphZoom gains an optional (ignored) 2nd arg; setVideoInTime/OutTime accept the false sentinel and the playback store's videoExportInTime/OutTime widen to number | false | null; mouseNotification.show x/y accept null (screen-relative positioning).

src/ is now TypeScript end-to-end — the only remaining .js is the trivial side-effect vendor.js (3 Leaflet imports, kept JS so its plugin .d.ts doesn't narrow the ambient L). Type-check 0 / lint / build / parser-golden all pass.

Phase 5 cleanup — drop the Loose renderer-instance aliases

Now that every renderer/config module is TypeScript, the Loose = any placeholders standing in for renderer instances are replaced with their real interfaces (the remaining Loose aliases — free-form graph/field config arrays, the sysConfig firmware header map, frame data — are intentionally dynamic and stay).

  • Graph store: graphFlightLogGrapher | null, mapGrapherMapGrapher | null, seekBarSeekBar | null, activeGraphConfigGraphConfig | null (via import type, so runtime stays the one-way grapher→store dep).
  • GraphConfigDialog: flightLog/graphConfig/grapher props move to type-based defineProps (FlightLog/GraphConfig/FlightLogGrapher); App.vue now passes the store values straight through and the Phase-4 ?? undefined workarounds are gone.
  • Ripple: consumers that dereference these post-load (store legend/analyser actions, the composable, log_lifecycle, playback_controls) use lifecycle non-null assertions — the renderer instances are guaranteed present once a log is loaded (same spirit as flightlog.ts's own as number casts).
  • blackbox_viewer_ops.ts is kept — still the single source of the BlackboxViewerOps interface the composable's ops is typed against.

Type-check 0 / lint / build / parser-golden all pass.

Phase 6 — replace the intentional Loose data surfaces with real types

Phase 5 typed every module + the renderer instances; Phase 6 retires the genuinely-dynamic Loose = any placeholders that were left to keep the conversion behaviour-preserving.

  • Graph-config model — new src/graph_types.ts (MinMax/Curve/GraphField/GraphConfigEntry/GraphConfigList), threaded through graph_config (shared MinMax/Curve, typed getGraphs/setGraphs), GraphConfigDialog (replaced its ad-hoc local types), and the graph store (graphConfig/lastGraphConfig). The grapher's runtime-adapted graphs stay loose (curve → ExpoCurve instance there).
  • FlightLog returns / glue de-LoosegetActivitySummary() returns a typed ActivitySummary; getCurrentFrameAtTime() returns {previous,current,next: FrameArray|null} | false. Every wholesale (logStore.flightLog as Loose) cast is gone from log_lifecycle/video_handler/playback_controls/the composable — flightLog is accessed typed, with only the real union returns narrowed at the use site (getMinTime/getMaxTime as number, etc). No flightLog as Loose cast remains in src/.
  • Frame data — all renderers (grapher/sticks/craft_2d/craft_3d/graph_spectrum/graph_map) now take a typed FlightLog, so chunk/frame data flows from FlightLogChunk (frames: number[][]) / FrameArray from the source. Deep per-helper frame vars (indexed dynamically by field index) stay loose by nature.

Deferred (low ROI, documented on #924): the SysConfig firmware-header bag (parser fills it by key, HeaderDialog reads it via a Loose prop — the index-signature model already fits); the video-export options bags (consumed through unknown/Loose boundaries); pen_adjustment's default field (a legacy array-used-as-object that serializes differently from a plain object, so not safely retypeable). blackbox_viewer_ops.ts is kept — still the single source of BlackboxViewerOps.

Each increment kept type-check 0 / lint / build / parser-golden green.

Summary by CodeRabbit

  • Tests

    • New golden-file and unit tests for parser, CSV/GPX and spectrum export to ensure consistent outputs.
  • New Features

    • Improved export pipeline with an efficient flattened transferable format for large CSV/GPX exports and dedicated spectrum CSV generation.
    • New viewer/composable initialization for app-wide controls.
  • Bug Fixes

    • Export handling fixes for empty/missing values and multiple input shapes; CI now includes type-checking.
  • Chores

    • Broad TypeScript migration and dev-dependency upgrades.

Lay the incremental TS migration foundation so .ts and .js coexist:

- tsconfig.json: allowJs + checkJs:false (existing JS untouched), strict
  for new TS, noEmit (Vite transpiles), bundler resolution, @/* alias
- src/env.d.ts: *.vue shims, __APP_VERSION__ define, script-loaded globals
- eslint.config.mjs: typescript-eslint scoped to src/**/*.ts only; JS
  ruleset unchanged; shared stylistic rules factored out
- package.json: add type-check (vue-tsc --noEmit) + typescript, vue-tsc,
  typescript-eslint devDeps
- build.yml: run type-check in CI before build

A file opts into type-checking by being renamed .js -> .ts; no big-bang.
type-check, lint, and build all pass.

Refs betaflight#924
@coderabbitai

coderabbitai Bot commented May 31, 2026

Copy link
Copy Markdown

Need the big picture first? Review this PR in Change Stack to see what changed before going file by file.

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: c10f010c-1f52-46b2-8527-23362b0599ae

📥 Commits

Reviewing files that changed from the base of the PR and between 87ee540 and b422c32.

📒 Files selected for processing (1)
  • src/stores/graph.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/stores/graph.ts

Walkthrough

Adds TypeScript across the codebase, centralizes imperative UI control in a new useBlackboxViewer composable, rewrites exporters/workers for transferable payloads, adds CI type-check and ESLint changes, and introduces golden/unit tests and fixtures.

Changes

TypeScript migration and viewer ops refactor

Layer / File(s) Summary
Core TS migration, viewer ops, exporters/workers, and tests
eslint.config.*, tsconfig.json, package.json, src/**/*.ts, src/**/*.vue, src/composables/*, src/workers/*, public/js/webworkers/*, test/*
Adds CI type-check, centralizes ESLint rules, enables TypeScript across SFCs and core modules, introduces useBlackboxViewer + BlackboxViewerOps, converts parser/datastream/flightlog and related modules to typed TS, rewrites exporters/workers (CSV/GPX/spectrum) with transferable flattening, adjusts Pinia stores to remove exported callback refs, and adds golden/unit tests and fixtures.

Sequence Diagram(s)

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related issues

Possibly related PRs

Suggested reviewers

  • nerdCopter
  • ctzsnooze
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
eslint.config.mjs (1)

33-37: ⚡ Quick win

Include .ts in lint-staged patterns to enforce these new TS rules locally.

The TS lint config is in place, but .lintstagedrc currently targets **/*.{tsx,js} only, so staged .ts files can bypass pre-commit linting.

🤖 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 `@eslint.config.mjs` around lines 33 - 37, The lint-staged config currently
omits .ts files so the new typescript-eslint rules (see the tseslint.config
block with files: ["src/**/*.ts"] in eslint.config.mjs) aren't enforced on
commit; update your lint-staged patterns to include .ts (for example add ts to
the existing glob so it becomes **/*.{ts,tsx,js} or add a separate /**/*.ts
entry) so staged .ts files run the same eslint --fix/precommit tasks as other
sources.
🤖 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.

Nitpick comments:
In `@eslint.config.mjs`:
- Around line 33-37: The lint-staged config currently omits .ts files so the new
typescript-eslint rules (see the tseslint.config block with files:
["src/**/*.ts"] in eslint.config.mjs) aren't enforced on commit; update your
lint-staged patterns to include .ts (for example add ts to the existing glob so
it becomes **/*.{ts,tsx,js} or add a separate /**/*.ts entry) so staged .ts
files run the same eslint --fix/precommit tasks as other sources.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 053156e6-53ab-409b-9a90-d8c452e33d37

📥 Commits

Reviewing files that changed from the base of the PR and between d350ef4 and 4e8d10e.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (5)
  • .github/workflows/build.yml
  • eslint.config.mjs
  • package.json
  • src/env.d.ts
  • tsconfig.json

coderabbitai[bot]
coderabbitai Bot previously approved these changes May 31, 2026
…hase 2)

First framework-agnostic core module converted to TS. Pure type
annotations, no logic change (97% rename similarity):

- makeReadOnly<T>(x: T): T
- live-binding mutable exports typed string[]: FLIGHT_LOG_FLIGHT_MODE_NAME,
  DEBUG_MODE, ACC_HARDWARE, MAG_HARDWARE
- adjustFieldDefsList(firmwareType: number, firmwareVersion: string): void
- add @types/semver devDep

All 10 importers (incl. HeaderDialog.vue) need no changes: Vite +
moduleResolution bundler resolve the .js specifiers to the .ts file.
type-check, lint, and build all pass.

Refs betaflight#924
coderabbitai[bot]
coderabbitai Bot previously approved these changes May 31, 2026
@haslinghuis haslinghuis changed the title chore: TypeScript tooling foundation (#924 Phase 0) TypeScript migration: tooling foundation + flightlog_fielddefs (#924) May 31, 2026
…Phase 2)

Convert the binary stream reader and advanced-format decoders to TS.

- ArrayDataStream: prototype-constructor -> idiomatic class. EOF kept on
  the prototype (ArrayDataStream.prototype.EOF) since the parser reads it
  there; declared on the instance type via `declare`.
- decoders: keep the separate side-effect module; add the decoder methods
  to ArrayDataStream via module augmentation (declaration merging) so the
  two-file structure and `import "./decoders"` contract are unchanged.
- typed reads (number / string | number), needle as ArrayLike<number>,
  data as Uint8Array | number[].

No logic change. Both consumers (flightlog_parser, flightlog_index) use
extensionless imports and need no changes. Verified with a temporary
esbuild+node runtime harness (19 assertions: U16/U32, VB/ZigZag,
readString, nextOffsetOf, prototype.EOF, decoder methods) — all pass.
type-check, lint, and build pass.

Refs betaflight#924
@haslinghuis haslinghuis changed the title TypeScript migration: tooling foundation + flightlog_fielddefs (#924) TypeScript migration: tooling + core modules (fielddefs, datastream, decoders) (#924) May 31, 2026
@haslinghuis haslinghuis marked this pull request as draft May 31, 2026 20:38
Convert the mixed-radix complex FFT to TS.

- FFTComplex: prototype-constructor -> idiomatic class (state typed via
  FFTState). Defensive undefined guard preserved via optional ctor params.
- butterfly2/3/4, generic butterfly, and work() typed; FFTArray =
  Float64Array | number[], FFTBuffer for the {data,offset,stride} structs.
- factors.shift() non-null asserted (the radix decomposition guarantees
  pairs are present).

No logic change. Sole consumer (graph_spectrum_calc) uses a `.js`
specifier which Vite resolves to the .ts. Verified against a naive DFT
with a temporary esbuild+node harness across n=5 (generic butterfly),
n=6 (butterfly3), n=8 (butterfly4), complex + real modes — max error
~1e-14, guard throws as before. type-check, lint, build pass.

Refs betaflight#924
@haslinghuis haslinghuis changed the title TypeScript migration: tooling + core modules (fielddefs, datastream, decoders) (#924) TypeScript migration: tooling + core modules (fielddefs, datastream, decoders, fft_complex) (#924) May 31, 2026
…aflight#924)

Lock in current parser behavior before the JS -> TS conversion (the
parser has no test suite and is the riskiest Phase 2 module).

- test/fixtures/synthetic_log.mjs: deterministic hand-encoded blackbox log
  (header + 1 I-frame + 3 P-frames) exercising header parsing, frame defs,
  intra/inter-frame decode, and the INC/PREVIOUS predictors.
- test/parser.golden.test.mjs: parses the fixture and compares sysConfig,
  frame defs, stats, and every onFrameReady frame against a committed
  snapshot; `-- --update` regenerates it.
- test/fixtures/parser.golden.json: the committed snapshot (from the
  current JS parser — the oracle).
- npm run test:parser (esbuild-bundled, since the parser is TS with
  extensionless imports); wired into CI before build.
- esbuild promoted to an explicit devDependency (used by the test script).

After the parser is converted to TS, this test must still pass unchanged,
proving byte-for-byte behavioral equivalence.

Refs betaflight#924
…dings (betaflight#924)

Extend coverage before the flightlog_parser TS conversion so the
previously-untested paths are locked in:

- new buildComplexLog() fixture: GPS home (H) + GPS (G) frames exercising
  the HOME_COORD / HOME_COORD_1 predictor pair, slow (S) frames, event (E)
  frames (SYNC_BEEP + End-of-log terminator), and the TAG2_3S32 /
  TAG8_8SVB / NEG_14BIT field encodings inside main frames
- snapshot now also captures H/G/S frame defs and serialises event-object
  frames; runs both the simple and complex fixtures
- committed test/fixtures/parser.golden.complex.json

Verified decoded values are exactly as designed, incl. home-coord
prediction (123456+10, 654321+20) and every tag decode. All frame types
report validCount>=1 with zero corrupt/desync. test:parser, type-check,
lint, build all pass.

Refs betaflight#924
…e 2)

Convert the 1900-line blackbox parser via Option A (typed constructor
function + instance interface), no logic change.

- new flightlog_types.ts: shared SysConfig (index signature + typed hot
  fields), FrameDef/FrameDefs/FrameArray, Stats/FrameTypeStats, event +
  callback types — reused by flightlog.js next.
- FlightLogParser: `this: FlightLogParser` on the constructor (inner
  arrows inherit it), declared instance interface, typed all inner
  functions, header handlers (HeaderHandler), predictors, and frame defs.
- dynamic sysConfig / event-data access kept as `any` (faithful to the
  original) behind localized eslint-disable; non-null assertions only
  where the algorithm guarantees presence (mainHistory[0], previous2);
  `as Uint8Array` for the two header subarray() reads.
- consumers (flightlog, grapher, sticks, playback_controls,
  flightlog_index) are extensionless/.js imports → no changes.

Proven equivalent: both parser golden snapshots (simple + complex:
GPS/slow/event frames, HOME_COORD predictor pair, TAG/NEG_14BIT
encodings) pass unchanged. type-check, lint, build all green.

Refs betaflight#924
@haslinghuis haslinghuis changed the title TypeScript migration: tooling + core modules (fielddefs, datastream, decoders, fft_complex) (#924) TypeScript migration: tooling + core modules incl. flightlog_parser (#924) May 31, 2026
Convert the on-demand log reader/cacher via Option A (typed constructor
function + instance interface), no logic change — git detects a rename.
Completes the Phase 2 framework-agnostic core.

- declared FlightLog instance interface (~43 methods); `this: FlightLog`
  on the constructor (inner closures inherit it).
- new shared types in flightlog_types.ts: FlightLogChunk; OnFrameReady
  `frame` loosened to any (genuinely polymorphic: numeric frame, event
  object, or null — matches the dynamic JS callers).
- local types: ComputeCtx (resolved field indices, FieldIndexList =
  number[] | false), IframeDirectory, LogStats (both originate from the
  still-JS flightlog_index, kept loose).
- typed the computed-field pipeline (attitude/PID/RC/GPS helpers), chunk
  caching, and smoothing.
- non-behavioural casts/assertions only: `as unknown as` to construct the
  typed-`this` FlightLogParser, gpsTransform!/stats.field! where the
  algorithm guarantees presence, and string field-index -> number at the
  smoothing array reads (array indexing already coerces).

No consumer changes (main.js is the only importer, extensionless). Sole
remaining JS-dynamic surfaces (sysConfig, event data, stats) kept loose
behind localized eslint-disable. type-check, lint, parser golden test,
and build all pass; diff is type-only (verified).

Refs betaflight#924
@haslinghuis haslinghuis changed the title TypeScript migration: tooling + core modules incl. flightlog_parser (#924) TypeScript migration: tooling + Phase 2 core (parser, flightlog, fft, decoders, fielddefs) (#924) May 31, 2026
@nerdCopter

nerdCopter commented Jun 5, 2026

Copy link
Copy Markdown
Member

video export does nothing (c7fc102a , http://localhost:5173/)

https://www.chromestatus.com/feature/5745543795965952
(anonymous) @ grapher.ts:1339
(anonymous) @ flightlog_video_renderer.ts:271
onStartExport @ VideoExportDialog.vue:258
(anonymous) @ Button.vue:84
onClickWrapper @ Button.vue:84
onClickWrapper @ LinkBase.vue:29
callWithErrorHandling @ chunk-GSIWT6FD.js?v=f77b4435:2609
callWithAsyncErrorHandling @ chunk-GSIWT6FD.js?v=f77b4435:2616
invoker @ chunk-GSIWT6FD.js?v=f77b4435:11800
graph_spectrum.ts:160 Uncaught (in promise) TypeError: Cannot read properties of null (reading 'style')
    at FlightLogAnalyser.resize (graph_spectrum.ts:160:18)
    at FlightLogGrapher.resize (grapher.ts:919:36)
    at new FlightLogGrapher (grapher.ts:1349:8)
    at new FlightLogVideoRenderer (flightlog_video_renderer.ts:271:17)
    at onStartExport (VideoExportDialog.vue:258:19)
    at Button.vue:84:49
    at Array.map (<anonymous>)
    at onClickWrapper (Button.vue:84:33)
    at onClickWrapper (LinkBase.vue:29:7)
    at callWithErrorHandling (chunk-GSIWT6FD.js?v=f77b4435:2609:19)
(anonymous) @ graph_spectrum.ts:160
(anonymous) @ grapher.ts:919
(anonymous) @ grapher.ts:1349
(anonymous) @ flightlog_video_renderer.ts:271
onStartExport @ VideoExportDialog.vue:258
(anonymous) @ Button.vue:84
onClickWrapper @ Button.vue:84
onClickWrapper @ LinkBase.vue:29
callWithErrorHandling @ chunk-GSIWT6FD.js?v=f77b4435:2609
callWithAsyncErrorHandling @ chunk-GSIWT6FD.js?v=f77b4435:2616
invoker @ chunk-GSIWT6FD.js?v=f77b4435:11800

@nerdCopter

Copy link
Copy Markdown
Member

GPX button was delayed, then worked.

[Violation] 'click' handler took 9452ms
export_utils.ts:14 GPX export finished in 15.3395 secs

CSV button was delayed, then worked.

THREE.WebGLRenderer 70
chunk-GSIWT6FD.js?v=f77b4435:11794 [Violation] 'click' handler took 10062ms
chunk-GSIWT6FD.js?v=f77b4435:11794 [Violation] 'click' handler took 5579ms
export_utils.ts:14 CSV export finished in 23.661700000047684 secs
csv-exporter.ts:29 [Violation] 'message' handler took 1440ms
export_utils.ts:14 CSV export finished in 18.875699999928475 secs

@demvlad

demvlad commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

The issue at the all PSD spectrum charts.

1

When PSD spectrum chart hides by legends line mouse click, then spectrums parameters does not hide. The spectrum buttons show/hide works properly.
2

Video export built an off-DOM FlightLogGrapher, whose ctor calls resize() →
analyser.resize(), which dereferenced analyserCanvas.parentElement! to
position the on-screen analyser container. Off-DOM that parent is null, so
the non-null assertion masked a `Cannot read properties of null (reading
'style')` TypeError that aborted the export on click.

Guard on the parent: when there is none, skip the DOM positioning and the
graphStore.analyserLayout push (both only meaningful for the on-screen
analyser). On-screen behaviour is unchanged.

Fixes R1 (manual runtime finding). Gates green.
…gend

Hiding the spectrum by clicking its legend line (selectLegendField) cleared
hasAnalyser but left hasAnalyserFullscreen set, so the fullscreen spectrum
parameter controls (zoom sliders, PSD min/max/segment inputs — gated on
hasAnalyserFullscreen) stayed on screen after the chart was gone. The
toolbar show/hide button (toggleAnalyser) already resets fullscreen on hide;
mirror that here.

Fixes R4 (manual runtime finding). Gates green.
CSV/GPX export froze the main thread for ~10-24 s on large logs: the click
handler structure-cloned a deeply-nested number[][][] frames array into the
export worker via postMessage (the serialization itself was already
off-thread). Flatten the frames into a single Float64Array and transfer it
zero-copy instead; the workers reconstruct rows as subarray views.

null/undefined cells (from the slow/GPS frame merge) are mapped to NaN,
which both workers already render identically to the original null
(CSV -> "NaN", GPX -> falsy -> skipped). Empty/ragged frames fall back to
the original nested postMessage, and the workers accept either shape.

Verified byte-identical CSV and GPX output between the old and new
protocols over representative data (nulls, NaN, valid/zero/null GPS coords,
ints, floats) — exports have no golden test, so this was checked by running
both worker sources in a sandbox. Also replaced the `getMin/MaxTime() as
number` casts here with the same `=== false` guard used in simple-stats.

Addresses R2/R3 (pre-existing perf, surfaced during manual testing).
coderabbitai[bot]
coderabbitai Bot previously approved these changes Jun 5, 2026
- export_frames.ts (introduced by the export perf fix): split the measure
  pass out of flattenFrames to drop its cognitive complexity under the
  threshold (S3776); use `?? Number.NaN` instead of the `== null` ternary
  (S6606, S7773).
- Flatten three `else { if … }` lonely-ifs into `else if` (S6660) in
  use_blackbox_viewer, graph_config and graph_spectrum_plot.

Behaviour-preserving; gates green (type-check 0, lint, golden, build).
coderabbitai[bot]
coderabbitai Bot previously approved these changes Jun 5, 2026
…to TS

Closes two follow-up items:

- Export output coverage: new test/export.golden.test.ts locks the CSV/GPX
  worker output against committed snapshots (test/fixtures/export.golden.{csv,gpx})
  and asserts the nested-frames and flattened-Float64Array postMessage
  protocols produce identical output — a regression guard for the export
  perf change. Also unit-checks the real flattenFrames (null/undefined → NaN,
  value preservation, empty/ragged → null). The classic Web Workers can't be
  imported in Node, so their real source runs in a small sandbox. Wired into
  `npm test` and CI.

- Tooling to TS: vite.config.js → vite.config.ts; the golden-test harness
  (parser.golden.test, synthetic_log fixture) → .ts with typed signatures.

Gates green: type-check 0, lint, npm test (parser + export goldens), build.
Move the 3 export workers out of public/js/webworkers/*.js into src/workers/
as Vite module workers, loaded via
`new Worker(new URL("./workers/x-export-worker.ts", import.meta.url), { type: "module" })`.

Each worker is split into a pure, typed, testable serializer
(csv_export/gpx_export/spectrum_export → buildCsv/buildGpx/buildSpectrumCsv)
and a thin worker entry. The export golden test now imports those real
serializers directly instead of running the worker source in a `new Function`
sandbox — so it tests the shipped code and drops the sandbox hotspot.

Behaviour-preserving: the CSV and GPX goldens (incl. the GPX header's
trailing whitespace) are unchanged — byte-for-byte proof the TS port matches
the old JS workers. Added a spectrum-CSV golden too. Vite bundles the three
workers into hashed chunks; no first-party `.js` remains in the app.

Gates green: type-check 0, lint, npm test (parser + export goldens), build.
…gacy

- eslint.config.mjs → eslint.config.ts (ESLint 10 loads it via jiti, added as
  a devDependency).
- Replace the dead browser test (test/index.js + test/index.html, which loaded
  a `../js/expo.js` global that no longer exists) with test/expo.test.ts — the
  same ExpoCurve lookup assertions, run in Node via esbuild and wired into
  `npm test` (test:expo).

First-party code is now fully TypeScript; the only remaining non-TS file is
the vendored public/js/three.min.js (third-party r70, loaded via <script>).

Gates green: type-check 0, lint (TS config), npm test (parser + export + expo),
build.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
src/workers/spectrum-export-worker.ts (1)

1-7: 💤 Low value

Consider using DedicatedWorkerGlobalScope type instead of Worker.

The type assertion self as unknown as Worker is semantically incorrect for a Web Worker context. Inside a worker, self refers to a DedicatedWorkerGlobalScope, not a Worker (which is the main-thread handle type). While the code functions correctly at runtime since both types share .onmessage and .postMessage, using the correct type improves clarity.

♻️ Proposed refinement
-const ctx = self as unknown as Worker;
+const ctx: DedicatedWorkerGlobalScope = self;
 ctx.onmessage = (event: MessageEvent) => {
   ctx.postMessage(buildSpectrumCsv(event.data));
 };
🤖 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 `@src/workers/spectrum-export-worker.ts` around lines 1 - 7, The file uses an
incorrect type assertion for the worker global: replace the `ctx` declaration
`const ctx = self as unknown as Worker;` with the correct worker-global type
(e.g. `const ctx = self as unknown as DedicatedWorkerGlobalScope;`) so that
`ctx.onmessage` / `ctx.postMessage(buildSpectrumCsv(event.data))` are typed
against `DedicatedWorkerGlobalScope` while leaving `buildSpectrumCsv` usage
unchanged.
🤖 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 `@src/workers/gpx_export.ts`:
- Around line 30-31: The current falsy check in gpx export skips valid zero
coordinates; update the condition in the loop that references frame, latIndex
and lngIndex to explicitly detect missing coordinates (NaN or non-finite) rather
than falsy values: replace `if (!frame[latIndex] || !frame[lngIndex])` with a
check using Number.isFinite (e.g. `if (!Number.isFinite(frame[latIndex]) ||
!Number.isFinite(frame[lngIndex])) continue;`) so zeros are preserved and
Float64Array-mapped NaN/null values are skipped; ensure this change covers
frames produced by flattenFrames.

---

Nitpick comments:
In `@src/workers/spectrum-export-worker.ts`:
- Around line 1-7: The file uses an incorrect type assertion for the worker
global: replace the `ctx` declaration `const ctx = self as unknown as Worker;`
with the correct worker-global type (e.g. `const ctx = self as unknown as
DedicatedWorkerGlobalScope;`) so that `ctx.onmessage` /
`ctx.postMessage(buildSpectrumCsv(event.data))` are typed against
`DedicatedWorkerGlobalScope` while leaving `buildSpectrumCsv` usage unchanged.
🪄 Autofix (Beta)

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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 2979ee74-be5a-49d3-9102-de58b33ebc7e

📥 Commits

Reviewing files that changed from the base of the PR and between be81075 and 5b730fa.

⛔ Files ignored due to path filters (2)
  • test/fixtures/export.golden.csv is excluded by !**/*.csv
  • test/fixtures/export.golden.spectrum.csv is excluded by !**/*.csv
📒 Files selected for processing (20)
  • .github/workflows/build.yml
  • package.json
  • public/js/webworkers/csv-export-worker.js
  • public/js/webworkers/gpx-export-worker.js
  • public/js/webworkers/spectrum-export-worker.js
  • src/csv-exporter.ts
  • src/gpx-exporter.ts
  • src/spectrum-exporter.ts
  • src/workers/csv-export-worker.ts
  • src/workers/csv_export.ts
  • src/workers/export_rows.ts
  • src/workers/gpx-export-worker.ts
  • src/workers/gpx_export.ts
  • src/workers/spectrum-export-worker.ts
  • src/workers/spectrum_export.ts
  • test/export.golden.test.ts
  • test/fixtures/export.golden.gpx
  • test/fixtures/synthetic_log.ts
  • test/parser.golden.test.ts
  • vite.config.ts
💤 Files with no reviewable changes (3)
  • public/js/webworkers/spectrum-export-worker.js
  • public/js/webworkers/csv-export-worker.js
  • public/js/webworkers/gpx-export-worker.js
✅ Files skipped from review due to trivial changes (1)
  • src/workers/gpx-export-worker.ts
🚧 Files skipped from review as they are similar to previous changes (5)
  • .github/workflows/build.yml
  • src/spectrum-exporter.ts
  • src/csv-exporter.ts
  • package.json
  • src/gpx-exporter.ts

Comment thread src/workers/gpx_export.ts
…taflight#924 Phase 6)

Replaces the three deferred dynamic data bags with real types:

- HeaderDialog `sysConfig` → `SysConfig` (already has the index signature);
  the lookup-list / group-map helpers typed too, removing the Loose alias.
- Video export → `VideoOptions` / `VideoLogParameters` / `VideoExportEvents`
  (and `flightLog: FlightLog`, `isSupported(): boolean`). The false time
  sentinels are guarded with `|| 0` and the optional flight video captured to
  a narrowed local; only the webm-writer, vendor-prefixed `document.hidden`
  and the grapher construct-cast stay Loose.
- pen_adjustment → `PenGraph`/`PenField`/`PenCurve`/`PenDefault`; the legacy
  `default` array-as-object is typed as an array carrying optional
  smoothing/power (so it serializes unchanged), and callers assert the
  stored→runtime-adapted shape at the two getGraphs() sites.

Behaviour-preserving (type-only, plus the `|| 0` sentinel guards that match
the prior coercion and a string→number array-index normalization in
changePenZoom). Gates green: type-check 0, lint, npm test, build.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/flightlog_video_renderer.ts (1)

350-355: ⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Fix the toDataURL quality parameter.

The quality parameter should be a number, not an object. Line 352 passes { quality: 0.9 } as Loose, but HTMLCanvasElement.toDataURL(type?, quality?) expects the second argument to be a number in the range 0.0–1.0. Passing an object will cause the browser to ignore or misinterpret the quality setting, resulting in incorrect WebP encoding quality.

🐛 Proposed fix
-  const encoded = canvas.toDataURL("image/webp", { quality: 0.9 } as Loose);
+  const encoded = canvas.toDataURL("image/webp", 0.9);
🤖 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 `@src/flightlog_video_renderer.ts` around lines 350 - 355, The call to
canvas.toDataURL is passing an object ({ quality: 0.9 } as Loose) as the second
argument which is invalid; change the call so the second argument is a numeric
quality value (0.9) rather than an object so
HTMLCanvasElement.toDataURL("image/webp", 0.9) is used; update the expression
that sets encoded (the canvas.toDataURL invocation) and remove the object cast
while keeping the subsequent data URL check intact.
🤖 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.

Outside diff comments:
In `@src/flightlog_video_renderer.ts`:
- Around line 350-355: The call to canvas.toDataURL is passing an object ({
quality: 0.9 } as Loose) as the second argument which is invalid; change the
call so the second argument is a numeric quality value (0.9) rather than an
object so HTMLCanvasElement.toDataURL("image/webp", 0.9) is used; update the
expression that sets encoded (the canvas.toDataURL invocation) and remove the
object cast while keeping the subsequent data URL check intact.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: a92a7ee9-86f9-4e31-a281-e54bc721f494

📥 Commits

Reviewing files that changed from the base of the PR and between 5b730fa and 3ed386d.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (9)
  • eslint.config.ts
  • package.json
  • src/components/HeaderDialog.vue
  • src/composables/use_blackbox_viewer.ts
  • src/flightlog_video_renderer.ts
  • src/pen_adjustment.ts
  • test/expo.test.ts
  • test/index.html
  • test/index.js
💤 Files with no reviewable changes (3)
  • eslint.config.ts
  • test/index.js
  • test/index.html
🚧 Files skipped from review as they are similar to previous changes (3)
  • package.json
  • src/components/HeaderDialog.vue
  • src/composables/use_blackbox_viewer.ts

Behaviour-preserving cleanup of the smells introduced by the latest commits:
- worker entries: `globalThis` instead of `self` (S7764)
- pen_adjustment: drop the redundant `default!` assertions — TS already
  narrows via the preceding `== null` guard (S4325)
- test/expo: drop zero fractions in the ExpoCurve assertions (S7748)
- test/export: `Number.NaN` over `NaN` in the fixture/expected arrays (S7773)

Gates green: type-check 0, lint, npm test, build.
Provably-equivalent inversions of negated conditions where the change is
clearly behaviour-preserving and/or test-verifiable:
- flightlog_parser `translateFieldName` → single ternary (golden-covered).
- flightlog_index / craft_2d: flip small if/else.
- flightlog (rcCommand/gyro computed-field ternaries), flightlog stats,
  graph_spectrum_plot catmull-rom p3: invert `x !== undefined ? a : b`
  ternaries to `x === undefined ? b : a`.
- export.golden.test: collapse the nested exit-code ternary (S3358).

Left intentionally: the negated-condition flips with large branches,
big-template ternaries, nested ternaries, and the NaN-sensitive
Math.min/max rewrites — style-only nits in code paths without test
coverage, where a flip carries transcription risk for no functional gain.

Gates green: type-check 0, lint, npm test (incl. parser golden), build.
Adds test/flightlog.golden.test.ts: builds a FlightLog from the synthetic
logs, opens it, and snapshots the chunk frames (rounded to 4 decimals) plus
fieldNames/min-max/numMotors/hasGpsData. This locks FlightLog's computed
fields — attitude (imu.ts), PID sum/error, RC-command scaling, and GPS
coord/distance/azimuth (gps_transform.ts via the complex GPS fixture) —
which the parser golden (FlightLogParser only) did not cover. Wired into
`npm test` and CI.

Covers the previously-untested core paths touched by this PR (the
injectComputedFields rcCommand/gyro ternaries, the Phase-6 frame typing).
Renderers (grapher/sticks/craft) remain uncovered — they couple to Pinia
stores + ThemeColors (getComputedStyle/DOM) + a canvas context, so a
headless golden there needs heavy mocking; tracked as a follow-up.

Gates green: type-check 0, lint, npm test (9 checks), build.
coderabbitai[bot]
coderabbitai Bot previously approved these changes Jun 5, 2026
@demvlad

demvlad commented Jun 6, 2026

Copy link
Copy Markdown
Contributor

The issue at the all PSD spectrum charts.

A small notice.
The spectrums full screen mode does not save when we hide spectrum chart and show it again.

Hiding the analyser collapsed `hasAnalyserFullscreen` (so the chart, params
and fullscreen layout don't linger — R4), but that meant re-showing always
came back in normal mode; fullscreen was never restored.

Remember the fullscreen preference when hiding and restore it (incl. the
grapher's setAnalyser state) when the analyser is shown again, via a shared
applyAnalyserFullscreenOnToggle() used by both toggleAnalyser (toolbar) and
selectLegendField (legend click). Selecting a different field while shown
leaves fullscreen untouched.

Addresses R5 (manual runtime finding). Gates green; UI behaviour — manual
re-test recommended.
@sonarqubecloud

sonarqubecloud Bot commented Jun 6, 2026

Copy link
Copy Markdown

@github-actions

github-actions Bot commented Jun 6, 2026

Copy link
Copy Markdown

Preview URL: https://pr925.betaflight-blackbox.pages.dev

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants