Skip to content

feat(export): wire History + Logs export JSON handlers (#1354)#1367

Merged
cliffhall merged 3 commits into
v2/mainfrom
v2/issue-1354-export-json
May 27, 2026
Merged

feat(export): wire History + Logs export JSON handlers (#1354)#1367
cliffhall merged 3 commits into
v2/mainfrom
v2/issue-1354-export-json

Conversation

@cliffhall
Copy link
Copy Markdown
Member

Summary

Closes #1354.

Replaces the todoNoop placeholders for History / Logs export with real handlers that serialize the current in-memory entries to JSON and trigger a browser download via the existing downloadJsonFile helper. Also routes the Network export through the same filename helper for consistency.

Filename

New buildExportFilename(kind, serverId, now?) helper next to downloadJsonFile. Produces sortable, filesystem-safe names like:

```
inspector-history-alpha-2026-03-17T10-00-42.123Z.json
inspector-logs-alpha-2026-03-17T10-00-42.123Z.json
inspector-network-alpha-2026-03-17T10-00-42.123Z.json
```

  • URL-encodes server ids that contain /, spaces, : etc. (since ids are user-supplied)
  • Swaps : for - in the ISO timestamp so the filename is safe on Windows
  • Omits the server-id segment when no active server is set (defensive — buttons only render when connected)

Export shapes

  • History → array of MessageEntry objects (direction, timestamp, message, response, duration). Full structured data for offline review.
  • Logs → array of LogEntryData objects (the notifications/message entries the screen consumes). Same shape the UI renders.
  • Network → array of FetchRequestEntry objects (unchanged from Surface backend-side HTTP fetch log in a Network screen #1355).

All three skip the download when the source list is empty (matches the disabled-button state in the panels).

Test plan

  • npm run validate — 1811 unit + integration tests pass
  • npm run test:storybook — 320 stories pass
  • New buildExportFilename tests cover: kind + server id + ISO swap, undefined server id fallback, special-character encoding, default-now shape
  • Manual: connect to a server, exercise a few tool calls + logging notifications, click Export on each screen, verify a file downloads and the JSON re-opens with the expected entries

Out of scope (per the issue)

  • Re-importing exported files (load-from-file) — separate feature
  • Filtering exports by method / level — current screen filters are display-only

Replaces the `todoNoop` placeholders in App.tsx with real handlers that
serialize the current entries and trigger a browser download via the
existing `downloadJsonFile` helper.

- Adds `buildExportFilename(kind, serverId, now?)` next to the download
  helper. Produces sortable, filesystem-safe names like
  `inspector-history-alpha-2026-03-17T10-00-42.123Z.json` (URL-encodes
  server ids that contain slashes / spaces / colons, swaps `:` for `-`
  in the ISO timestamp so the file is safe on Windows).
- Wires `onExportHistory` (full MessageEntry list) and `onExportLogs`
  (the LogEntryData list the screen renders).
- Routes the existing Network export through the same filename helper
  for consistency.

Unit tests cover the filename builder, including the no-server-id
fallback, special-character encoding, and the default-`now` shape.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@cliffhall cliffhall added the v2 Issues and PRs for v2 label May 27, 2026
@cliffhall
Copy link
Copy Markdown
Member Author

@claude review

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 27, 2026

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


Code Review

  • Read PR diff and changed files
  • Review filename helper (buildExportFilename)
  • Review App.tsx wiring for History / Logs / Network export
  • Review test coverage
  • Post review findings

Overall this is a clean, tightly-scoped PR. The three handlers mirror each other, the helper is well-documented, and the test coverage is solid for the new surface. The Network handler being switched over to the shared helper is a nice consistency win on top of the stated scope. A few notes below — none are blockers.

Findings

1. kind parameter could be a literal union — clients/web/src/lib/downloadFile.ts:46

kind: string is the only knob on the helper that isn't already type-safe. Today there are exactly three call sites ("history" | "logs" | "network"), all in App.tsx. Tightening the signature gives:

  • Compile-time guard against typos at call sites (e.g. "hisotry" would be caught)
  • Inline documentation of the supported export kinds without a reader having to grep
  • Stable on-disk format — callers can't accidentally invent a new prefix
export type ExportKind = "history" | "logs" | "network";
export function buildExportFilename(
  kind: ExportKind,
  serverId: string | undefined,
  now: Date = new Date(),
): string { ... }

Genuinely minor — drop if you prefer to keep the helper as a generic primitive. Fix this →

2. Date fields silently become ISO strings in the exported JSON — clients/web/src/App.tsx:692

MessageEntry.timestamp and the per-fetch entry timestamps are Date instances at runtime. JSON.stringify(messages, null, 2) will turn them into ISO strings via Date.prototype.toJSON. That's the right behavior, and it's lossless enough for human inspection. Worth being aware of for the future load-from-file feature (out of scope here per the PR description) — the import path will need to re-new Date(...) the timestamp fields rather than assume the in-memory shape round-trips identity. No change requested; flagging for the next ticket.

3. Empty-string serverId is treated as "no server" — clients/web/src/lib/downloadFile.ts:51

const id = serverId ? encodeURIComponent(serverId) : undefined; falls into the "omit segment" branch for both undefined and "". In practice empty strings shouldn't appear (active server ids come from mcp.json), so this is fine — but the JSDoc says "When serverId is undefined the segment is omitted," which is slightly under-specified. Consider documenting the falsy fallback, or being explicit (serverId === undefined) to match the docstring exactly. Cosmetic.

4. No handler-level tests — clients/web/src/App.tsx:688-702

buildExportFilename is well tested in isolation, and downloadJsonFile has its own suite. The three useCallback handlers (onExportHistory, onExportLogs, onExportNetwork) don't have an explicit integration test — but this is consistent with how the pre-existing onExportNetwork and onServerExport callbacks are covered (i.e., implicitly), and the per-file coverage gate is per-source-file, so App.tsx's percentage is what counts. No change requested.

Things I checked and was happy with

  • Filename is sortable (kind → server → ISO time) and the :- swap is necessary for Windows
  • Empty-list guards mirror the disabled-button state — handlers are safe against programmatic callers
  • encodeURIComponent correctly handles /, space, :, and other unsafe characters; the test asserts this
  • Network export now uses the same helper — good cleanup
  • All four buildExportFilename tests cover meaningful branches (default now, undefined id, encoded id, fixed id)
  • useCallback dependency arrays correctly include both the data array and activeServerId

Verdict

LGTM with the optional ExportKind tightening. The manual smoke test in the PR's checklist (connect, generate traffic, click Export, re-open the file) is the right last step before merging — that's the one thing the unit suite can't verify.
· branch: v2/issue-1354-export-json

- Introduce an `ExportKind = "history" | "logs" | "network"` union and
  use it as the type of the `kind` parameter. Typos like "hisotry" now
  fail at compile time and the supported export kinds are documented
  inline.
- Clarify the JSDoc: the segment is omitted when `serverId` is falsy
  (undefined OR empty string), matching the existing `serverId ? ... :
  undefined` check. New test pins the empty-string branch.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@cliffhall
Copy link
Copy Markdown
Member Author

Thanks for the review! Responses inline, addressed in 49c1150.

#1 kind could be a literal union — Fixed. Added export type ExportKind = "history" | "logs" | "network" in downloadFile.ts and used it as the parameter type. All three call sites already pass matching literals so no further changes needed.

#2 Date fields become ISO strings via toJSON — Acknowledged, no change. The export is meant to be human-/tooling-readable JSON, and ISO strings are the right wire form. Filed in my head for the future load-from-file feature, which will need to new Date(...) the timestamp fields on import.

#3 Empty-string serverId is treated as "no server" — Documentation fixed. The check serverId ? ... : undefined does fall through for both undefined and "", which is actually the desired behavior (we never want inspector-history--<iso>.json with a double dash). JSDoc now reads "When serverId is falsy (undefined or empty)…" to match. New regression test pins the empty-string branch.

#4 No handler-level App.tsx tests — Acknowledged, no change. The new useCallback handlers are thin wrappers around buildExportFilename (well-covered) + downloadJsonFile (well-covered) + the in-memory arrays. Adding an integration test for App.tsx itself would be a meaningful new test surface that this PR isn't taking on. Consistent with how onExportNetwork and onServerExport are covered today.

@cliffhall
Copy link
Copy Markdown
Member Author

@claude review

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 27, 2026

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


Code Review — follow-up to 49c1150

  • Read PR diff and changed files
  • Review buildExportFilename updates (ExportKind type + docs)
  • Review handler wiring in App.tsx
  • Review test coverage for empty-string regression
  • Post review findings

Tight, focused follow-up commit. Both of the actionable items from the prior review are addressed cleanly; the two acknowledged-only items are reasonable to leave for later. No blockers, no new findings.

Verification of prior-review responses

#1ExportKind union
clients/web/src/lib/downloadFile.ts:39 introduces export type ExportKind = "history" | "logs" | "network" and buildExportFilename now takes it as kind's type. All three call sites in App.tsx (onExportHistory, onExportLogs, onExportNetwork at lines 683, 691, 699) pass matching literals — typechecks cleanly with no other call-site changes. Typos at future call sites will now be caught at compile time.

#2Date → ISO via toJSON ✅ Acknowledged-only, no change. Reasonable: the export is for human/tooling consumption, and a future load-from-file feature can new Date(...) on import. Filed correctly.

#3 — Empty-string serverId
The JSDoc at downloadFile.ts:49 is now explicit: "When serverId is falsy (undefined or empty) the segment is omitted." The implementation at line 59 (serverId ? encodeURIComponent(serverId) : undefined) genuinely is the desired behavior — without this we'd produce names like inspector-history--<iso>.json with a double-dash, which is the ugly path. A regression test pins it at downloadFile.test.ts:111-115. Doc + behavior + test now agree.

#4 — Handler-level App.tsx tests ✅ Acknowledged-only, no change. Consistent with the existing coverage approach for onExportNetwork and onServerExport. Reasonable.

Spot-checks on this commit

  • activeServerId is typed string | undefined at App.tsx:183 — matches buildExportFilename's serverId: string | undefined parameter exactly. No as casts at the call sites, which is the right outcome.
  • The new empty-string test (downloadFile.test.ts:111) asserts the same output as the undefined test (line 105) — both produce inspector-logs-2026-03-17T10-00-42.123Z.json. Good — it codifies the "falsy" contract rather than implementation-detail behavior.
  • ExportKind is exported, which is right — call sites in App.tsx don't currently need it (they pass literals), but exporting the type means future helpers (e.g. an importer) can share the vocabulary without re-declaring.
  • Diff is 90/-6 across 3 files. No drive-by changes; scope matches the PR title.

Verdict

LGTM, ready to merge once the manual smoke test (last unchecked box in the PR description: connect → exercise calls + logging → click Export on each screen → verify JSON re-opens) is done. Nothing in the review-feedback round needs another pass.
· branch: v2/issue-1354-export-json

Export now produces the same JSON the Copy All button used to dump to
the clipboard, so the button is redundant. History and Network panels
have no equivalent — dropping Copy All also brings Logs into parity.

Removes the prop from LogStreamPanel + LoggingScreen + InspectorView +
App.tsx, and updates the matching tests/stories. Tests now assert the
button is *not* in the document so a future re-introduction would have
to update the assertion deliberately.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@cliffhall cliffhall merged commit a717828 into v2/main May 27, 2026
1 check passed
@cliffhall cliffhall deleted the v2/issue-1354-export-json branch May 27, 2026 23:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

v2 Issues and PRs for v2

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant