Skip to content

Hardware exploration#421

Draft
tonyalaribe wants to merge 2 commits into
masterfrom
hardware-exploration
Draft

Hardware exploration#421
tonyalaribe wants to merge 2 commits into
masterfrom
hardware-exploration

Conversation

@tonyalaribe

Copy link
Copy Markdown
Contributor

Closes #

How to test

Checklist

  • Make sure you have described your changes and added all relevant screenshots or data.
  • Make sure your changes are tested (stories and/or unit, integration, or end-to-end tests).
  • Make sure to add/update documentation regarding your changes (or request one from the team).
  • You are NOT deprecating/removing a feature.

@claude

claude Bot commented Jun 9, 2026

Copy link
Copy Markdown

Code Review — Hardware Exploration (#421)

Overview: Adds a /hardware page backed by a <hardware-monitor> Lit web component — a digital-twin POC with a Three.js 3D arm view, SVG 2D fallback, ECharts timeline/sparklines, and synthetic telemetry. Clean concept; several polish items before it's production-ready.


Haskell (Pages/Hardware.hs, BodyWrapper.hs, Routes.hs)

  • Missing i18n on nav item. Every other nav entry uses I18n.t lang "nav.x"; the Hardware entry hardcodes the string:

    , ("Hardware", "/p/" <> pid.toText <> "/hardware", "objects-column")

    Should follow the same pattern as its neighbours.

  • Unrelated rtsopts change. -Iw60 (idle GC wakeup every 60 s) is added in the same diff line as the GHC options change. If intentional, it deserves its own commit/explanation; if accidental, drop it.

  • The Haskell page itself is fine — idiomatic term/data_ use, clean handler.


TypeScript — dead code / unused exports

  • binding.ts is entirely dead code. Neither KqlSource nor SyntheticSource is imported anywhere; the component calls generateTelemetry() directly. If the intent is a future contract sketch, a code comment in data.ts would convey that without shipping ~80 lines that can't be tested or tree-shaken. Consider removing the file and adding a // TODO: swap generateTelemetry() for a KqlSource comment at the call-site.

  • aggregateSeverity (data.ts) is exported but never imported. refreshAll() in the component re-implements the same worst-severity fold by hand. Either use the function or don't export it:

    - if (sev === 'crit') worst = 'crit';
    - else if (sev === 'warn' && worst === 'ok') worst = 'warn';
    + // use aggregateSeverity(tel, this.activeMetric, this.currentTime) instead

Duplicate time-formatter

fmtSec (inside updateTimelineSeries) and formatT (inside render) are byte-for-byte identical. Extract once at class or module level:

private static fmtSec(s: number) {
  return `${Math.floor(s / 60).toString().padStart(2,'0')}:${Math.floor(s % 60).toString().padStart(2,'0')}`;
}

JOINT_IDS constant ignored in scene.ts / schematic2d.ts

The JOINT_IDS constant is exported from data.ts specifically to avoid magic arrays, but both visual files hardcode [1, 2, 3, 4, 5, 6] as JointId[] five or more times (e.g. setJointAngles, setJointStates, setJointValues, setHighlight, the render loop). Import and use the constant:

-for (const j of [1, 2, 3, 4, 5, 6] as JointId[]) {
+for (const j of JOINT_IDS) {

Timeline double-fire on click

mountTimeline() registers both an ECharts 'click' handler and a raw 'click' DOM listener on this.timelineEl. Clicking a data point fires both: ECharts emits a precise p.value[0] time, then the raw handler computes a pixel-based time from clientX that will be slightly different. The raw listener should guard with e.stopPropagation() inside the ECharts handler, or the raw listener should only fire when ECharts didn't handle the click (e.g. check p.componentType).


any types despite @types/three being present

@types/three was added as a dependency, but JointHandle still types everything as any:

pivot: any;    // Object3D
halo: any;     // Mesh
haloMat: any;  // MeshBasicMaterial

With the types package present, these should be THREE.Object3D, THREE.Mesh, THREE.MeshBasicMaterial. Same for timelineChart: any / sparkCharts: Record<MetricKind, any> — if ECharts doesn't ship types, a minimal interface ECharts { setOption(o: object): void; on(...): void; dispose(): void; } would still be better than any.


Minor

  • setCursor series offset is fragile. The cursor update uses new Array(JOINT_IDS.length + 1).fill({}) to skip to the right series index. If the scatter/event series moves, this silently breaks. Address by series name: setOption({ series: [{ name: '__cursor', markLine: { data: [{ xAxis: this.currentTime }] } }] }) — ECharts merges by name when notMerge is omitted.

  • @types/three listed as a production dep. It's a types-only package and belongs in devDependencies.

  • sCurve and smooth in data.ts do the same thing. smooth and sCurve are both t*t*(3-2*t) smoothstep. One of them can be removed.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant