Viewer: detail-pane UX parity with deployed ara-hub (#55-#59)

[EYH0602]

Summary

• Detail-pane UX parity work matching the deployed ara-hub viewer: node id + isolated pill in the header (Viewer: show node id and isolated-subtree pill in the detail header #56), exhibit captions (Viewer: render exhibit captions (description) in the result block #55), clickable DEPENDS ON / DEPENDED ON BY jump chips (Viewer: cross-node navigation from the detail pane (depends-on jump chips, parent/child links) #57), collapsible content blocks with item counts (Viewer: collapsible detail blocks with item counts #58), and replay-key focus guarding (Viewer: global left/right arrow keys for replay stepping #59).
• Follow-up fix (found during a live design review of this branch): the new DEPENDS ON jump chips inherited display: inline-flex from the base .chip class, on which text-overflow: ellipsis silently has no effect — long labels were hard-clipped mid-word instead of showing an ellipsis. Fixed by overriding chip-jump to display: inline-block, and regenerated the embedded viewer bundle via scripts/embed-viewer.sh.

Test plan

• cargo test -p ara-viewer — 170 passed
• scripts/embed-viewer.sh --check — embedded bundle up to date
• Live-verified in browser (trunk build): node id/isolated pill render correctly (synthetic isolated-node test), DEPENDS ON / DEPENDED ON BY chips resolve and jump-select correctly, collapsible blocks expand/collapse with correct counts, and the chip-truncation ellipsis fix renders correctly before/after

[fenfenai]

Review Summary

Reviewed the functional diff in crates/ara-viewer/src/detail.rs, crates/ara-viewer/src/replay.rs, and crates/ara-viewer/public/styles.css across bug detection, error handling, type design, test coverage, comment quality, and guidelines-compliance dimensions (parallel specialized reviewers, findings filtered to confidence ≥75 and cross-checked against the actual PR source).

Solid PR overall: the pure detail_model() transform is thoroughly unit-tested (7 new cases covering isolated-subtree inheritance, cycle guards, and depends-on resolution in both directions), docs/CHANGELOG/version bump all follow this repo's CLAUDE.md conventions, and the plans/ → docs/ workflow was followed correctly. Error handling, comment accuracy, and guidelines compliance reviewers found nothing to report.

One likely UI-state bug and a few smaller design/coverage gaps below.

Important Issues (confidence 75-89)

crates/ara-viewer/src/detail.rs:511 — Collapsed block state may leak across node selection (confidence: 78)

CollapsibleBlock sets <details open=true> as a static (non-reactive) attribute. DetailPane's single top-level move || {...} re-invokes render_detail(model, selected).into_any() on every selected change, but since every Some(node) branch produces the same concrete view type, Leptos's AnyView reconciliation patches the existing <details> elements in place rather than remounting them — static attributes set once at initial build() aren't reapplied on rebuild(). If a user manually collapses e.g. the "evidence" block (native <details> toggle) and then selects a different node, that block stays collapsed for the new node's data with no visual cue it's hidden.
Suggestion: make open reactive per node id (e.g. open=move || ... keyed off m.id), or force a full remount on node change (<Show>/keyed wrapper) so collapse state doesn't persist across node switches.

crates/ara-viewer/src/detail.rs:68-74 — DepView.known: bool + label: String can drift out of sync (confidence: 78)

known is meant to indicate whether label came from a resolved node vs. a raw-id fallback, but nothing in the type ties the two together — both fields are pub and nothing prevents constructing an inconsistent DepView (e.g. known: true with a raw-id label). Today dep_view() is the only constructor so it's not exploited, but the render layer trusts known alone to decide whether to emit a clickable jump chip.
Suggestion: enum DepTarget { Resolved { id: NodeId, label: String }, Unresolved(NodeId) }, or keep the struct but make fields private behind DepView::resolved()/DepView::unresolved() constructors so known is derived, not independently settable.

crates/ara-viewer/src/detail.rs:93 — DetailModel.id: String duplicates the NodeId newtype (confidence: 76)

DepView.id (added in this same PR, line 70) correctly uses NodeId, but DetailModel.id is typed String and populated via node.id.as_str().to_string() — discarding the type-level guarantee NodeId already provides, inconsistent with the sibling field added in the same diff.
Suggestion: type as pub id: NodeId and convert to a display string at the Leptos render call site instead of at model-construction time.

Test gap: multi-parent Child links to the same node are untested (confidence: 82)

is_in_isolated_subtree's upward walk picks the first matching Child link via .find() over manifest.links (i.e., insertion/iteration order) when a node has more than one incoming Child link. The viewer deserializes Manifest directly from JSON without routing through ara-core's parse.rs tree-building validation (which would normally enforce single-parent trees), so a hand-edited or malformed manifest reaching the viewer is a real, reachable input — the isolated pill could flip depending on link order.
Suggestion: add a test with two Child links pointing at the same child from an isolated and a non-isolated root, and assert/pin the "first parent wins" contract explicitly (or reject/warn on the ambiguity if that's preferred).

Test gap: duplicate DependsOn links are untested (confidence: 76)

ara-core::parse.rs::dedupe_links normally strips duplicate links at compile time, but the viewer bypasses that path (see above) — a duplicate DependsOn link between the same pair would render as two identical duplicate chips with no dedup in detail_model.
Suggestion: add a test asserting the current (duplicate-preserving) behavior is intentional, or dedupe in detail_model if not.

Positive Observations

• is_in_isolated_subtree's cycle guard (HashSet-based, matches tree_model's existing guard pattern) and the depends_on/depended_on_by link-direction filtering are correct and well covered by tests.
• The CSS fix comment on button.chip-jump (why inline-flex breaks text-overflow: ellipsis) is technically accurate and a good "why" comment.
• docs/hub-parity.md and docs/stage-3-viewer.md updates accurately describe the shipped behavior, not just touched files.
• Version bump (0.1.11 → 0.1.12) and CHANGELOG.md entries correctly follow this repo's CLAUDE.md conventions, including the plans/ → docs/ rewrite workflow (plans/viewer-detail-pane-ux.md added then folded into docs within this PR's commit lineage).

[EYH0602]

Thanks for the thorough review — all five findings addressed in fc91ee7. Replies inline below.

1. Collapsed block state leaking across node selection (detail.rs:511) — Confirmed and fixed, with one correction to the suggested approach: a reactive open=move || { selected.track(); true } does NOT work — Leptos diffs attribute values and skips the DOM write when the value is unchanged (true → true), even though the user mutated the live DOM via native <details> toggle. Fixed instead with a NodeRef + Effect that watches selected and forcibly calls el.set_open(true) on the patched-in-place element. Covered by a new browser test collapsible_block_reopens_on_node_switch (collapse EVIDENCE on N01 → switch to N02 → assert open re-applied), which fails against the old code and passes now.

2. DepView.known/label drift (detail.rs:68-74) — Fields are now private; construction goes through DepView::resolved(id, label) / DepView::unresolved(id) only, so known can no longer be set independently of how label was derived.

3. DetailModel.id: String duplicating the NodeId newtype (detail.rs:93) — Retyped to pub id: NodeId; the display string conversion moved to the render call site ({m.id.to_string()}).

4. Multi-parent Child links untested — Added isolated_multi_parent_first_child_link_wins: two Child links into the same child from an isolated and a non-isolated root, asserted in both link orders, pinning the "first parent wins" contract explicitly (including its order-dependence).

5. Duplicate DependsOn links untested — Added duplicate_depends_on_links_rendered_as_is, pinning the current duplicate-preserving behavior as intentional: the viewer renders the manifest faithfully rather than silently editing it.

Failing viewer-web-test check — Root cause was in the PR itself: #58 changed the EVIDENCE/BUILT ON/DEPENDS ON/RESULT/SOURCES blocks from <div> to <details>, but the pre-existing browser tests still queried div.built-on-block / div.result-block. Selectors updated to the class-only form; the full 54-test browser suite passes locally (wasm-pack test --headless --chrome), plus clippy, fmt, native tests (172), and scripts/embed-viewer.sh --check (embedded bundle regenerated).

[fenfenai]

Re-Review: Comment Resolution

Resolved (5 threads)

• detail.rs:511 — Collapsed block state leak ✓ Fixed. CollapsibleBlock now takes selected and forces set_open(true) on the live <details> element via a NodeRef + Effect keyed on selected, working around Leptos's in-place patching. Verified by a new wasm_bindgen_test (collapsible_block_reopens_on_node_switch) that collapses a block on N01, switches to N02, and asserts the block re-opened — this directly exercises the failure mode I flagged.
• detail.rs:68-74 — DepView.known/label drift ✓ Fixed. Fields are now private; construction goes through DepView::resolved() / DepView::unresolved(), so the invariant is enforced by the type, not just by convention.
• detail.rs:93 — DetailModel.id: String ✓ Fixed. Now NodeId, matching DepView.id; converted to a display string only at the render call site (m.id.to_string()).
• detail.rs:280 (multi-parent Child links) ✓ Fixed. New test isolated_multi_parent_first_child_link_wins pins the "first Child link wins" contract in both link orderings.
• detail.rs:239 (duplicate DependsOn links) ✓ Fixed. New test duplicate_depends_on_links_rendered_as_is pins the current preserve-duplicates behavior as intentional.

Verification

• cargo test -p ara-viewer --lib: 172 passed (up from 170; +2 native tests for the two pinned contracts above).
• Viewer bundle (index.html, hashed .js/.wasm) was correctly regenerated to match the new source.
• No regressions spotted in the rest of the diff; the tests/web.rs selector fixes (div.built-on-block → .built-on-block etc.) are a correct follow-on from blocks becoming <details> in #58, not new scope creep.

All prior findings addressed. No new issues found.