feat(web): expandable resource_link with read-on-demand (#1449)

[cliffhall]

Closes #1449

Summary

Replaces V2's placeholder resource_link rendering with a dedicated, Mantine-native ResourceLink element that brings V1's interactive, expandable, click-to-read experience to V2.

Behavior

• Shows the URI (monospace, link-styled), optional name and description, and a mimeType badge.
• Expand/collapse affordance (chevron + loading spinner) that is keyboard accessible — a native button gives role, aria-expanded, aria-label, and Enter/Space handling.
• On first expand, reads the linked resource on demand and renders the full read result inline as formatted JSON under a green "Resource:" label, capped to a max height (400px) with scroll for large results. The fetched result is cached across collapse/expand.

The matching V1 layout (uri / name / description / mime badge / green "Resource:" label / scrollable JSON):

Collapsed cards + one expanded showing the read result	✅ ported

Wiring

Follows the V2 "dumb component + callbacks as props" pattern. The read handler is threaded App → InspectorView → ToolsScreen → ToolResultPanel → ContentViewer → ResourceLink. App supplies a handler returning the ReadResourceResult from inspectorClient.readResource(uri).

Refactor

Extracts the shared resourceContentsToBlock helper into utils/resourceContents.ts (previously private to ResourcePreviewPanel).

Testing

• New unit tests: ResourceLink (collapsed/expanded/error/cache/non-interactive), resourceContents, and the ContentViewer resource_link wiring.
• New Storybook stories with play functions for ResourceLink and ContentViewer.
• Verified the expanded layout visually in Storybook against the target design.
• npm run validate, npm run test:integration (522), and npm run test:storybook (345) all pass.

🤖 Generated with Claude Code

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 2m 6s —— View job

---

Claude Code Review

Nice work — this is a clean, well-tested port. The dumb-component + callbacks-as-props pattern is followed correctly, the prop is threaded properly through App → InspectorView → ToolsScreen → ToolResultPanel → ContentViewer → ResourceLink, and App.onReadResourceContents correctly unwraps invocation.result from inspectorClient.readResource(uri) (ResourceReadInvocation). Mantine usage adheres to the repo conventions (.withProps() subcomponents, no inline styles, monoBreak variant exists in the theme). Tests and stories cover the key states.

A few things worth considering before merge — none are blockers.

1. A failed read is cached forever — no retry path

ResourceLink.tsx:146 returns early when error !== null, so once a read fails the user can never retry by collapse/re-expand; the error sticks until the component unmounts. A transient network/server hiccup becomes permanent. Consider clearing error on collapse (or not treating an error as a cached terminal state) so re-expand re-attempts the read.

// toggle(): re-read on re-expand if the prior attempt errored
if (result !== null) return;   // only the successful result is cached
setError(null);
setLoading(true);

Fix this →

2. Possible double-fetch if the user toggles mid-load

In toggle() (ResourceLink.tsx:138) there's no loading guard. Sequence: click expand (read in flight) → click collapse → click expand again. On the third click result/error are still null, so a second onReadResource(uri) fires concurrently. Harmless to correctness, but it's a redundant network call. A if (loading) return; at the top of the expand path closes it.

3. Circular import between ContentViewer and ResourceLink

ResourceLink.tsx:15 imports ContentViewer, and ContentViewer.tsx:9 imports ResourceLink. It works today because both are hoisted function declarations only referenced at render time (not module-eval time), but circular deps are fragile under some bundler/HMR conditions. Not urgent, just flagging — if it ever bites, the JSON-rendering in ResourceLink could inline a <Code> block instead of round-tripping through ContentViewer.

4. The extracted resourceContentsToBlock helper has no new consumer

The refactor moves resourceContentsToBlock into utils/resourceContents.ts (good, and well-tested), but ResourceLink doesn't use it — it JSON.stringifys the entire ReadResourceResult and renders it as one text block (ResourceLink.tsx:202-209). So:

• The "shared" helper is still only consumed by ResourcePreviewPanel; the extraction isn't strictly needed by this PR (harmless, just noting).
• More notably, this means an expanded resource_link pointing at an image/audio resource shows base64 JSON rather than the rendered media — inconsistent with ResourcePreviewPanel, which uses the helper to render rich content. If matching V1's "dump JSON" behavior is intentional (the PR description says it is), no change needed — but it'd be a natural follow-up to map each contents[] item through resourceContentsToBlock for richer inline previews.

5. Minor: color tokens

c="blue"/c="green"/color="red" are Mantine named colors (not hex/rgba), so they don't violate the literal CLAUDE.md rule, and c="blue" is already used in ResourcePreviewPanel. Just note the repo leans toward semantic --inspector-* tokens; if a green "Resource:" label recurs it may be worth a token.

Tests

Good coverage of collapsed/expanded/error/cache/non-interactive. Two gaps you may want to add given the above: a test asserting a failed read is retryable (if you adopt #1), and one asserting no double-fetch on rapid toggle (if you adopt #2). The transient "Loading resource…" state isn't asserted but that's fine.

Overall: solid, ready to merge once you decide on the error-retry behavior (#1), which is the only one with real UX impact.
· v2/resource-link-expandable-1449

[cliffhall]

Thanks for the thorough review! Addressed in d25c938:

#1 — Failed read cached forever (fixed). toggle() now caches only a successful result; an error is no longer treated as a terminal cached state. setError(null) runs before each read, so collapsing and re-expanding after a transient failure retries the read. Added a test (retries the read on re-expand after a failure).

#2 — Double-fetch on rapid toggle (fixed). Added an if (loading) return; guard on the expand path so toggling while a read is in flight no longer fires a second onReadResource. Added a test (does not fire a second read when toggled while a read is in flight) using a deferred promise to hold the read open across the toggle.

#3 — Circular import (already resolved). This was fixed by the component reorg: resource_link metadata now lives in the ResourceLinkInfo element, and the interactive card moved to the groups/ResourceLink group. ContentViewer now imports ResourceLinkInfo (not ResourceLink), and ResourceLink imports ContentViewer + ResourceLinkInfo — no cycle remains.

#4 — Extracted helper / rich media previews (intentional, follow-up noted). Dumping the full ReadResourceResult as JSON is the intended V1-matching behavior for this PR. Mapping each contents[] item through resourceContentsToBlock for rich image/audio previews is a reasonable follow-up; I'll file it rather than expand this PR's scope.

#5 — Color tokens (noted). Agreed these are Mantine named colors (no literal-hex violation) and c="blue" already matches ResourcePreviewPanel. If the green "Resource:" label recurs elsewhere I'll promote it to an --inspector-* token.

npm run validate and the ResourceLink suite (7 tests) pass.

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 1m 17s —— View job

---

Claude Code Review (round 2)

I re-reviewed against origin/v2/main, focusing on the fixes from the last round. The source changes are solid and the prior feedback is properly resolved — but there's one thing that needs to be fixed before merge.

• Read the changed source files
• Verify the fixes for prior review items (Progress notifications #1 retry, Request logging #2 double-fetch, Completion requests #3 circular import)
• Check build artifacts inclusion
• Post review

---

🔴 Blocker: committed build artifacts

8 compiled/build files were accidentally added in d25c9382 and account for ~16,500 of the PR's ~16,543 additions:

clients/cli/build/index.js            (+4096)
clients/cli/build/index.js.map
clients/tui/build/index.js            (+8985)
clients/tui/build/index.js.map
clients/web/build/index.js            (+2653)
clients/web/build/index.js.map
clients/launcher/build/index.js
clients/launcher/build/parse-launcher-argv.js

These don't exist on origin/v2/main, and clients/*/build/ is not in .gitignore (only test-servers/build is), so a stray npm run build got swept into the commit. They shouldn't be in source control — they bloat the diff, will churn/conflict constantly, and obscure the ~30 lines of real change. Please git rm them and add clients/*/build/ (or the specific paths) to .gitignore.

Fix this →

---

✅ Prior review items — all resolved

#1 Retryable failed read — toggle() now caches only a successful result (ResourceLink.tsx:102) and clears error before each read (:105). Covered by "retries the read on re-expand after a failure" (ResourceLink.test.tsx:105). 👍

#2 Double-fetch guard — if (loading) return; added at ResourceLink.tsx:104. Note the ordering: the cached-result short-circuit (:102) runs first, then the loading guard — correct, since a re-expand mid-flight has result === null and falls through to the loading check. Covered by the deferred-promise test (:140). 👍

#3 Circular import — resolved by the reorg. ContentViewer now imports the pure-display ResourceLinkInfo element (ContentViewer.tsx:6), and the interactive card lives in groups/ResourceLink, which imports ContentViewer + ResourceLinkInfo. No cycle. The split (element vs. group) is a clean improvement. 👍

Minor (non-blocking)

• ContentViewer inside ResultScroll (ResourceLink.tsx:159-166): the inner Code block p={36} adds 36px padding inside the 400px-capped scroll area, so the JSON gets a fairly large inset. Cosmetic, and consistent with how ContentViewer renders text elsewhere — fine to leave.
• Render-time JSON.stringify of the full result (:163): re-stringifies on every render while expanded. Results are typically small, so not worth memoizing, but worth a useMemo if large results ever become common.
• c="green" Resource label (:64) and the other Mantine named colors are fine per the literal-hex rule, as the previous round noted; promote to an --inspector-* token only if it recurs.

Verdict

Source code, tests, and the element/group refactor are all in good shape and ready. Just strip the build artifacts (and ideally gitignore them) so the diff reflects the actual change, and this is good to merge.
· v2/resource-link-expandable-1449

[cliffhall]

Good catch on the blocker — fixed in 799637c.

🔴 Build artifacts (fixed). A stray git add -A on my earlier commit swept in 8 compiled build files (clients/{cli,tui,web,launcher}/build/*). I git rm --cached'd all 8 (they're leftover output from the reverted "first pass" client import, not part of this change) and added clients/*/build to .gitignore so client build output can't be committed again. The diff now contains only the real ResourceLink/ContentViewer change plus the .gitignore line — confirmed no build/ paths remain in git diff v2/main.

The minor notes (Code padding inside the scroll area, render-time JSON.stringify, c="green" token) I'm leaving as-is per your "fine to leave" assessment and the prior round's decision to keep the JSON-dump behavior.

@claude review

[claude]

Claude finished @cliffhall's task in 34s —— View job

---

Claude Code Review (round 3)

Re-reviewed the build-artifact fix in 799637ce against origin/v2/main.

• Confirm all 8 build artifacts removed from the diff
• Confirm .gitignore covers clients/*/build
• Confirm only the real source change remains
• Post verdict

---

✅ Blocker resolved

Build artifacts stripped. git diff origin/v2/main...HEAD --name-only | grep build/ now returns nothing — all 8 of clients/{cli,tui,web,launcher}/build/* are gone from the diff, and git ls-files confirms none remain tracked.

.gitignore hardened. The new line lands right where it belongs, next to the existing test-servers/build:

 dist
+clients/*/build
 test-servers/build

So a stray npm run build can't sweep client output back into a commit. 👍

Diff is now honest. The PR is back to 728 insertions / 17 deletions across 15 files — all of it the real ResourceLink / ResourceLinkInfo / ContentViewer change plus the one .gitignore line. Working tree is clean.

---

Verdict

🟢 Ready to merge. All prior items are resolved:

• Progress notifications #1 retryable failed read ✅ (round 1)
• Request logging #2 double-fetch guard ✅ (round 1)
• Completion requests #3 circular import — fixed by the element/group split ✅ (round 1)
• 🔴 committed build artifacts ✅ (this round)

The remaining minor notes (Code padding inside the scroll area, render-time JSON.stringify, c="green" token, image/audio rich-preview follow-up via resourceContentsToBlock) are all non-blocking and were already agreed as leave-as-is / follow-up. Nice clean PR now.
· v2/resource-link-expandable-1449