app-16292: refactor Details panel into focused subcomponents by DTCurrie · Pull Request #736 · viamrobotics/visualization

DTCurrie · 2026-06-01T21:02:28Z

Splits the monolithic Details.svelte overlay into focused subcomponents under src/lib/components/overlay/details/. Behavior is unchanged for the refactor itself.

This is preparation for the per-entity panels that the Gizmos plugin mounts in #738.

Note

Use this PR preview to view all of the changes from this stack: https://viamrobotics.github.io/visualization/pr-preview/pr-739/snapshot

Stack

Shared ECS traits and helpers (app-16292: shared ECS traits and helpers for entity pose plugins #735)
This PR: Details refactor
Component tweaks (app-16292: small component tweaks to support per-entity pose mutators #737)
Gizmos plugin (app-16292: add Gizmos plugin #738)
Gizmos plugin docs (app-16292: add docs page for the Gizmos plugin #739)

Frontend

Added src/lib/components/overlay/details/ColorDetails.svelte, GeometryDetails.svelte, MatrixDetails.svelte, LineDetails/LineDetails.svelte, OpacityDetails.svelte, and AxesHelperDetails.svelte.
src/lib/components/overlay/details/LineDetails/linePositions.ts: extracts the line-position derivation with its own spec so it can be tested without rendering Svelte.
src/lib/components/overlay/Details.svelte: mounts a PortalTarget named details-extensions so plugin-supplied editors (e.g. the gizmo Details in app-16292: add Gizmos plugin #738) can render inline, and replaces the inline opacity slider and show-axes-helper switch with <OpacityDetails> and <AxesHelperDetails>. Plugin-supplied panels mount these conditionally per entity kind.
MatrixDetails.svelte: takes required onPoseChange(patch) and onParentChange(next) props so the caller decides whether mutations commit immediately (writeMatrix / hierarchy.setParent) or buffer through an edit session (e.g. a future frame edit plugin writing to EditedMatrix). Required rather than defaulted to avoid silently committing edits that should have been buffered.
OpacityDetails.svelte: owns the opacity slider. Always writes traits.Opacity(value) instead of removing the trait at 1, and falls back to 0.7 for display to match the renderer's historical default. Previously, the slider would remove the trait at 1, after which Line / Mesh would fall back to their own 0.7 default and render translucent instead of opaque.
AxesHelperDetails.svelte: owns the show-axes-helper switch, adding/removing traits.ShowAxesHelper directly.
vitest-setup-client.ts: stubs @threlte/extras Portal, PortalTarget, and HTML so the refactored panel can render in unit tests without a <Canvas> parent.

Testing

The existing src/lib/components/overlay/__tests__/Details.svelte.spec.ts continues to pass against the refactored component with the new mocks. New MatrixDetails.svelte.spec.ts, OpacityDetails.svelte.spec.ts, and AxesHelperDetails.svelte.spec.ts cover the structural rendering branches of each extracted component, and linePositions.spec.ts covers the extracted line-position derivation. Ran pnpm check and pnpm test. Manually verified the opacity slider now produces a fully opaque mesh at 1 and stays at 0.7 for a trait-less entity.

changeset-bot · 2026-06-01T21:02:35Z

🦋 Changeset detected

Latest commit: 99d1188

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@viamrobotics/motion-tools	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Foundation for the upcoming Gizmos plugin. No callers yet; these symbols are consumed by follow-up PRs in the stack. * writeMatrix(entity, patch): patch a Pose onto an entity's Matrix trait via createPose/matrixToPose/poseToMatrix, then dirty the trait. Used by the refactored Details panel and the Gizmos plugin. * CustomDetails trait: marker so the Details panel suppresses its default frame/pose UI for entities that render their own. * useMouseRaycaster firstHitOnly option (forwarded to three-mesh-bvh). * 'gizmo' value added to Settings.interactionMode. * Re-exports of DEFAULT_LINE_WIDTH and ARROW_LENGTH for downstream consumers.

The Details overlay component had grown to handle four entirely different entity shapes (color, geometry, line, matrix) inline. Split each into its own component under details/ so callers and reviewers can reason about one entity type at a time. * ColorDetails.svelte, GeometryDetails.svelte, MatrixDetails.svelte and LineDetails/LineDetails.svelte are now individual focused components. * linePositions.ts extracts the line-position derivation with its own spec. * MatrixDetails uses writeMatrix() added in the prior PR. * vitest-setup-client.ts stubs @threlte/extras Portal / PortalTarget / HTML so the refactored panel can render in unit tests. Behavior should be unchanged; existing Details.svelte.spec.ts continues to pass with the new mocks.

- Move writeMatrix below updateGeometryTrait for consistent file layout - Add JSDoc to writeMatrix documenting round-trip semantics and no-op behavior - Filter undefined values from patch before Object.assign to prevent NaN in matrix - Add comment clarifying firstHitOnly affects both hover and click handlers - Strengthen orientation test with oX/oY/oZ assertions Co-authored-by: Devin T. Currie <DTCurrie@users.noreply.github.com>

- linePositions.ts: add bounds check to writeLinePosition; return copy unchanged on out-of-bounds index - Details.svelte: await navigator.clipboard.writeText() and wrap in try/catch to handle permission denial - MatrixDetails.svelte: add comments explaining useQuery vs configFrames and unconditional rendering - linePositions.spec.ts: add edge cases for out-of-bounds write and single-point removal guard - Add component tests for ColorDetails, GeometryDetails, MatrixDetails, and LineDetails Co-authored-by: Devin T. Currie <DTCurrie@users.noreply.github.com>

…stHitOnly effect split - Add early-exit guard in writeMatrix when filtered patch is empty to avoid spurious entity.changed(Matrix) notifications - Add tests for undefined field filtering and empty/all-undefined patch paths - Split firstHitOnly assignment into its own $effect so toggling it doesn't tear down and re-attach event listeners Co-authored-by: Devin T. Currie <DTCurrie@users.noreply.github.com>

Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>

- vitest-setup-client.ts: `invalidate` was nested inside `scene` in the useThrelte mock instead of at the top level; `const { invalidate } = useThrelte()` returned undefined in all tests. - linePositions.ts: `removeLinePosition` had a duplicate `if (source.length <= 6) return source` guard (dead code after the bounds-check fix in a prior commit). Co-authored-by: Devin T. Currie <DTCurrie@users.noreply.github.com>

Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>

DTCurrie · 2026-06-03T14:43:58Z

@claude final review

claude · 2026-06-03T14:44:56Z

Claude finished @DTCurrie's task in 2m 40s —— View job

Final Review

Read all changed files
Traced prior review rounds — all previous issues addressed
Fixed duplicate test case in linePositions.spec.ts (committed directly)

This PR is in good shape. All substantive issues from previous rounds have been resolved:

Issue	Status
`writeLinePosition` out-of-bounds guard	✅ Added (`index * 3 + 2 >= source.length` early return)
`removeLinePosition` out-of-bounds guard	✅ Added (symmetric guard, returns `source` unchanged)
`invalidate()` missing in `ColorDetails`, `GeometryDetails`, `LineDetails`	✅ Fixed — all handlers call `invalidate()`
`invalidate()` missing in `handleParentChange` for non-frame entities	✅ Fixed — called before the `isFrameNode` branch
`navigator.clipboard.writeText()` unawaited	✅ Fixed — `try/await/catch` with proper error handling
`useThrelte` mock missing `invalidate`	✅ Present at line 14, no duplicate
`removeLinePosition` out-of-bounds test	✅ Added

One fix committed: duplicate 'keeps original array intact' test case in linePositions.spec.ts (copy-paste artifact — two identical it blocks). Removed the second copy.

Pre-existing CI noise (unrelated to this PR): pnpm check errors are missing generated protobuf files ($lib/buf/**), pnpm lint flags .ci-prompts/prompts/ci-common.md formatting, and pnpm test fails due to Playwright browser not installed in this runner.

Architecture is sound. The onPoseChange/onParentChange prop pattern in MatrixDetails is a clean inversion that lets callers decide whether mutations are immediate or buffered — exactly right for the upcoming gizmo edit sessions in #738. The details-extensions portal target is minimal and correct.

Ready to merge after #735 lands.
| Branch

Co-authored-by: Devin T. Currie <DTCurrie@users.noreply.github.com>

micheal-parks

should we postfix all of these new files with "Details" if they're in the details folder? seems potentially noisy like the situation with "BaseView", "CameraView" etc in test-cards, but I could also understand not wanting to pollute ctrl+p search

DTCurrie · 2026-06-03T16:57:13Z

I could also understand not wanting to pollute ctrl+p search

@micheal-parks that is the exact reason. I don't feel strongly about it though so I am open to removing them

micheal-parks · 2026-06-03T18:43:20Z

@@ -0,0 +1,189 @@
+<script


Can we call this PoseDetails? I feel like this should reflect what the user sees, matrices are an internal design detail that the user never sees

Ha, I actually originally named it that then swapped to MatrixDetails. I'll swap it back

This was referenced Jun 1, 2026

app-16292: small component tweaks to support per-entity pose mutators #737

Open

app-16292: add Gizmos plugin #738

Open

app-16292: add docs page for the Gizmos plugin #739

Open

DTCurrie added 2 commits June 1, 2026 17:11

Add changeset

9e637c6

DTCurrie force-pushed the app-16292/1-ecs-utils branch from 681bd0b to 9e637c6 Compare June 1, 2026 21:12

DTCurrie mentioned this pull request Jun 1, 2026

app-16292: shared ECS traits and helpers for entity pose plugins #735

Merged

DTCurrie added 2 commits June 1, 2026 17:13

Add changeset

bf49747

DTCurrie force-pushed the app-16292/2-details-refactor branch from a9faa2f to bf49747 Compare June 1, 2026 21:14

This comment was marked as outdated.

Sign in to view

claude Bot reviewed Jun 1, 2026

View reviewed changes

Comment thread src/lib/components/overlay/details/LineDetails/linePositions.ts

claude Bot reviewed Jun 1, 2026

View reviewed changes

Comment thread src/lib/components/overlay/details/PoseDetails.svelte

claude Bot reviewed Jun 1, 2026

View reviewed changes

Comment thread src/lib/components/overlay/details/MatrixDetails.svelte Outdated

claude Bot reviewed Jun 1, 2026

View reviewed changes

Comment thread src/lib/components/overlay/Details.svelte

claude Bot reviewed Jun 1, 2026

View reviewed changes

Comment thread src/lib/components/overlay/details/LineDetails/__tests__/linePositions.spec.ts

github-actions Bot and others added 2 commits June 1, 2026 21:27

Merge branch 'main' into app-16292/1-ecs-utils

c18c0e7

This comment was marked as outdated.

Sign in to view

github-actions Bot and others added 5 commits June 2, 2026 14:16

Merge branch 'app-16292/1-ecs-utils' into app-16292/2-details-refactor

4b68aae

Update src/lib/ecs/traits.ts

c00b48a

Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>

Merge branch 'app-16292/1-ecs-utils' into app-16292/2-details-refactor

2bcdba2

This comment was marked as outdated.

Sign in to view

claude Bot reviewed Jun 2, 2026

View reviewed changes

Comment thread src/lib/components/overlay/Details.svelte

This comment was marked as outdated.

Sign in to view

claude Bot reviewed Jun 2, 2026

View reviewed changes

Comment thread vitest-setup-client.ts Outdated

claude Bot reviewed Jun 2, 2026

View reviewed changes

Comment thread src/lib/components/overlay/details/LineDetails/linePositions.ts

Apply suggestions from code review

156feed

Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>

This comment was marked as outdated.

Sign in to view

claude Bot reviewed Jun 3, 2026

View reviewed changes

Comment thread src/lib/components/overlay/details/LineDetails/__tests__/linePositions.spec.ts

DTCurrie and others added 4 commits June 3, 2026 10:17

Apply suggestion from @claude[bot]

2cdca10

Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>

Apply suggestion from @claude[bot]

fb7f589

Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>

claude is bad at suggesting code edits

7d48588

remove outdated mock

5ccbd2b

test: remove duplicate 'keeps original array intact' test case

84b0156

Co-authored-by: Devin T. Currie <DTCurrie@users.noreply.github.com>

claude Bot reviewed Jun 3, 2026

View reviewed changes

Comment thread src/lib/components/overlay/details/LineDetails/__tests__/linePositions.spec.ts

micheal-parks reviewed Jun 3, 2026

View reviewed changes

Comment thread src/lib/components/overlay/Details.svelte Outdated

micheal-parks reviewed Jun 3, 2026

View reviewed changes

Comment thread src/lib/components/overlay/Details.svelte Outdated

DTCurrie added 2 commits June 3, 2026 13:30

remove old local matrix code

8c7540a

cleanup

470ae90

micheal-parks reviewed Jun 3, 2026

View reviewed changes

micheal-parks approved these changes Jun 3, 2026

View reviewed changes

DTCurrie and others added 2 commits June 3, 2026 16:57

MatrixDetails -> PoseDetails

ac26da9

Merge branch 'main' into app-16292/2-details-refactor

99d1188

DTCurrie merged commit a0cd017 into main Jun 3, 2026
7 checks passed

DTCurrie deleted the app-16292/2-details-refactor branch June 3, 2026 21:07

claude Bot mentioned this pull request Jun 3, 2026

Version Packages #748

Open

Conversation

DTCurrie commented Jun 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Stack

Frontend

Testing

Uh oh!

changeset-bot Bot commented Jun 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

Uh oh!

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

Uh oh!

DTCurrie commented Jun 3, 2026

Uh oh!

claude Bot commented Jun 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Final Review

Uh oh!

Uh oh!

micheal-parks left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DTCurrie commented Jun 3, 2026

Uh oh!

Uh oh!

micheal-parks Jun 3, 2026

Choose a reason for hiding this comment

Uh oh!

DTCurrie Jun 3, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

DTCurrie commented Jun 1, 2026 •

edited

Loading

changeset-bot Bot commented Jun 1, 2026 •

edited

Loading

claude Bot commented Jun 3, 2026 •

edited

Loading