Repository navigation
fix: nine small defects from the v2.13 review (#2827) - #2856
Merged
Merged
Conversation
1. Self-host the Fredoka and Roboto Mono woff2 files (clients/web/src/theme/fonts) instead of loading them from fonts.googleapis.com at runtime, in the app and in Storybook. 2. The sandbox listener answers HEAD, ignores a query string on /sandbox, and applies the main server's Host allow-list (plus the host of MCP_SANDBOX_FULL_ADDRESS). 3. The prod SPA fallback excludes only /api and /api/..., so /apixyz gets the SPA. 4. One ad-hoc server naming rule (adHocServerName in core/mcp/serverList.ts): URL host or command basename, used by web and by the shared CLI/TUI/mcpdo resolution, which used to call it "default". 5. The default seed scopes filesystem-server-default to os.tmpdir(), passed in by the Node backend (defaultSeedConfig(tempDir)) since serverList.ts is browser-consumed. 6. Every web screen has a visually hidden h1; connected-screen panel headings move to h2/h3 (visual sizes unchanged) so axe heading-order stays clean. 7. The TUI lists tools alphabetically, as the web sidebar does since #2782, via a shared core/mcp/toolOrder.ts. 8. TUI test modals no longer print their title twice; the roots-editor comment in App.tsx reflects #2432. 9. A custom MCP_INSPECTOR_API_TOKEN shorter than 32 characters prints a startup warning. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: cliffhall <cliff@futurescale.com>
2 tasks done
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved issues include missing font redistribution license materials, a heading-order violation in maximized apps with messages, and tool selection and scroll resets on unrelated server updates.
3 open issues
What changed in this PR
Addresses nine defects from #2827 across the web client, TUI, and shared MCP configuration, without adding dependencies.
Changes:
- Self-hosts web fonts, improves heading structure, and removes duplicate TUI form titles.
- Hardens sandbox request handling, corrects SPA routing, and warns about short custom tokens.
- Unifies ad-hoc server naming and tool sorting, uses platform-specific seed paths, and updates tests and documentation.
| File | Description |
|---|---|
| docs/mcp-server-configuration.md | Documents platform-specific seed paths. |
| docs/environment-variables.md | Documents short-token warnings. |
| core/mcp/toolOrder.ts | Adds shared tool comparators. |
| core/mcp/serverList.ts | Centralizes ad-hoc naming and parameterizes seed paths. |
| core/mcp/remote/node/server.ts | Seeds catalogs using os.tmpdir(). |
| core/mcp/node/servers.ts | Applies shared ad-hoc naming. |
| clients/web/src/utils/toolUtils.ts | Re-exports shared tool comparators. |
| clients/web/src/theme/fonts/fonts.test.ts | Checks local font references. |
| clients/web/src/theme/fonts/fonts.css | Defines self-hosted font faces. |
| clients/web/src/test/integration/server/web-server-config.test.ts | Tests custom-token warnings. |
| clients/web/src/test/integration/server/server-token-injection.test.ts | Tests non-API SPA fallback. |
| clients/web/src/test/integration/server/sandbox-controller.test.ts | Tests HEAD, query strings, and Host filtering. |
| clients/web/src/test/integration/mcp/remote/servers-route.test.ts | Updates seed-route expectations. |
| clients/web/src/test/core/react/useServers.test.tsx | Updates seed-key assertions. |
| clients/web/src/test/core/mcp/toolOrder.test.ts | Tests shared sorting behavior. |
| clients/web/src/test/core/mcp/serverList.test.ts | Tests naming and platform seed paths. |
| clients/web/src/test/core/mcp/node/servers.test.ts | Updates ad-hoc server expectations. |
| clients/web/src/main.tsx | Loads bundled fonts. |
| clients/web/src/components/views/InspectorView/InspectorView.tsx | Adds a screen-specific hidden h1. |
| clients/web/src/components/views/InspectorView/InspectorView.test.tsx | Tests page headings. |
| clients/web/src/components/screens/ServerListScreen/ServerListScreen.tsx | Makes the panel title h2. |
| clients/web/src/components/screens/ConsoleScreen/ConsoleScreen.tsx | Makes console headings h2. |
| clients/web/src/components/screens/AppsScreen/AppsScreen.tsx | Makes app messages a subsection heading. |
| clients/web/src/components/groups/ToolResultPanel/ToolResultPanel.tsx | Adjusts result heading hierarchy. |
| clients/web/src/components/groups/ToolResultPanel/ToolCallErrorPanel.tsx | Makes the error panel heading h2. |
| clients/web/src/components/groups/ToolControls/ToolControls.tsx | Makes the tools heading h2. |
| clients/web/src/components/groups/TaskListPanel/TaskListPanel.tsx | Adjusts task heading hierarchy. |
| clients/web/src/components/groups/TaskControls/TaskControls.tsx | Adjusts task control headings. |
| clients/web/src/components/groups/StructuredOutputPanel/StructuredOutputPanel.tsx | Makes structured output a subsection heading. |
| clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.tsx | Makes the template heading h2. |
| clients/web/src/components/groups/ResourcePreviewPanel/ResourcePreviewPanel.tsx | Makes the preview heading h2. |
| clients/web/src/components/groups/ResourceControls/ResourceControls.tsx | Makes the resources heading h2. |
| clients/web/src/components/groups/ProtocolListPanel/ProtocolListPanel.tsx | Adjusts protocol heading hierarchy. |
| clients/web/src/components/groups/ProtocolControls/ProtocolControls.tsx | Adjusts protocol control headings. |
| clients/web/src/components/groups/PromptMessagesDisplay/PromptMessagesDisplay.tsx | Makes the messages heading h2. |
| clients/web/src/components/groups/PromptControls/PromptControls.tsx | Makes the prompts heading h2. |
| clients/web/src/components/groups/PromptArgumentsForm/PromptArgumentsForm.tsx | Makes the arguments heading h2. |
| clients/web/src/components/groups/NetworkStreamPanel/NetworkStreamPanel.tsx | Makes the network panel heading h2. |
| clients/web/src/components/groups/NetworkControls/NetworkControls.tsx | Adjusts network control headings. |
| clients/web/src/components/groups/MessageDirectionFilter/MessageDirectionFilter.tsx | Makes the filter heading h3. |
| clients/web/src/components/groups/LogStreamPanel/LogStreamPanel.tsx | Makes the log stream heading h2. |
| clients/web/src/components/groups/LogControls/LogControls.tsx | Adjusts logging control headings. |
| clients/web/src/components/groups/AppControls/AppControls.tsx | Makes the apps heading h2. |
| clients/web/server/web-server-config.ts | Warns about short custom tokens. |
| clients/web/server/vite-hono-plugin.ts | Passes allowed hosts to the dev sandbox. |
| clients/web/server/server.ts | Corrects SPA routing and passes sandbox hosts. |
| clients/web/server/sandbox-controller.ts | Adds Host filtering, HEAD, and query support. |
| clients/web/server/run-web.ts | Uses shared ad-hoc naming. |
| clients/web/README.md | Documents sandbox Host checks. |
| clients/web/index.html | Removes Google Fonts links. |
| clients/web/.storybook/preview.tsx | Loads bundled fonts in Storybook. |
| clients/web/.storybook/main.ts | Removes external font injection. |
| clients/tui/src/utils/formHeader.ts | Adds an untitled form-copy helper. |
| clients/tui/src/components/ToolTestModal.tsx | Removes the duplicate form title. |
| clients/tui/src/components/ResourceTestModal.tsx | Removes the duplicate form title. |
| clients/tui/src/components/PromptTestModal.tsx | Removes the duplicate form title. |
| clients/tui/src/App.tsx | Sorts tools and corrects the roots-editor comment. |
| clients/tui/__tests__/tui-servers.test.ts | Updates ad-hoc naming expectations. |
| clients/tui/__tests__/formHeader.test.ts | Tests title removal without mutation. |
| clients/tui/__tests__/App.test.tsx | Tests alphabetical tool display. |
| clients/launcher/README.md | Documents platform-specific seed paths. |
Review effort: Balanced
Comment on lines
+10
to
+12
| * - Fredoka (wght 300–700) — SIL Open Font License 1.1 | ||
| * - Roboto Mono (wght 100–700, normal + italic) — Apache License 2.0 | ||
| * Both licenses permit redistribution. To refresh them, request the same css2 |
Member
Author
There was a problem hiding this comment.
Fixed in 6f63891: the full upstream texts from google/fonts (ofl/fredoka/OFL.txt, ofl/robotomono/OFL.txt) are now in clients/web/public/fonts/, copyright notices included. Vite copies that folder verbatim to dist/fonts/, so the licenses ship in every build and in the npm tarball (clients/web/dist is in files). I also corrected the header: Roboto Mono is OFL 1.1 too, not Apache. A test pins that both files exist and carry their notice.
…led font licenses (#2827) - TUI: memoize the sorted tools on the store snapshot so an unrelated update (a stderr line) no longer resets the Tools selection; regression test. - Web: the Apps 'Messages from app' heading is an h2, valid with the sidebar hidden while maximized; test. - Ship the full upstream OFL 1.1 texts for Fredoka and Roboto Mono in public/fonts (copied to dist/fonts, so they are in the tarball); test. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: cliffhall <cliff@futurescale.com>
Member
Author
|
Copilot round 1, all three findings fixed in 6f63891 (replies are in each thread):
|
Member
Author
|
Copilot review loop closed. Round 2 recommends approval, with no inline comments, nothing in the headline and nothing suppressed. The font-license item it still lists as "open" is the round-1 thread, which is fixed in 6f63891 and answered in place; the thread just has not been resolved. |
2 tasks done
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #2827
All nine items from the v2.13 review, one commit each concern, no new dependencies.
clients/web/src/theme/fonts/with afonts.cssof matching@font-facerules (split byunicode-range), imported bymain.tsxand the Storybook preview. Thefonts.googleapis.comlinks are gone fromindex.htmland.storybook/main.ts. Vite bundles and hashes the files. Licenses: OFL 1.1 (Fredoka), Apache 2.0 (Roboto Mono); both allow redistribution. A test pins "nohttp(s)://in index.html" and "everyurl()in fonts.css is a local file that exists".clients/web/index.html,src/theme/fonts/*HEADlikeGETminus the body (Content-Lengthincluded), matches/sandbox//sandbox/ignoring a query string, and applies the main server'sHostallow-list (createHostAllowListfromhost-allow-list.ts: loopback names, IP literals, bind host,ALLOWED_ORIGINShosts,MCP_INSPECTOR_ALLOWED_HOSTS, plus the host ofMCP_SANDBOX_FULL_ADDRESS). A refused Host gets the same 403 text as the main server. Applies in prod and--dev.server/sandbox-controller.ts(+allowedHostspassed fromserver.tsandvite-hono-plugin.ts)/apiand/api/…(isApiPath), so/apixyzgets the SPA. That is the only change inserver.tsbesides threadingallowedHoststo the sandbox.server/server.tsserver) and moved it toadHocServerNameincore/mcp/serverList.ts. The sharedloadServerEntries(CLI, TUI, mcpdo) now uses it too, sonode server.jsisnodeeverywhere. Why this one:defaultdoesn't tell you which server it is. Behavior change: CLI, TUI and mcpdo no longer key an ad-hoc server asdefault. Selection is unaffected, since an ad-hoc launch has one entry and no--server.core/mcp/serverList.ts,core/mcp/node/servers.ts,server/run-web.tsserverList.tsis browser-consumed, so it can't importnode:os.DEFAULT_SEED_CONFIGis nowdefaultSeedConfig(tempDir), and the only caller that writes a seed (the Node backend's first-launch branch) passesos.tmpdir().core/mcp/serverList.ts,core/mcp/remote/node/server.ts(3 lines)<h1>. InspectorView renders<VisuallyHidden component="h1">MCP Inspector: <tab></VisuallyHidden>first in the header. Design note: an h1 alone would have broken axeheading-orderon every connected screen, because their panel headings start at h3/h4 and h1→h4 skips. So the connected screens' panel headings move to h2 and their sections to h3, throughPanelHeading/SubHeadingTitle.withPropsconstants that keep the old visual size (size: "h4"/"h5"). Nothing looks different. The Servers panel title also moves h3→h2, because that screen shows while connected too, when the header has no h2. Modal-hosted panels (ConnectionInfoContent,SamplingRequestPanel,ImportServerJsonPanel) are unchanged: they already sit under the modal's h2.InspectorView.tsx+ ~20components/groups/*tools/listorder isn't something servers promise. So the TUI now sorts too. The collator moved tocore/mcp/toolOrder.ts, and webtoolUtilsre-exports it. Each surface sorts by the label its row shows: web rows showtitle ?? name, TUI rows showname(same as the web's excluded-tool rows). The two orders are identical for tools without atitle. Prompts and resources were unsorted in both clients and stay that way.core/mcp/toolOrder.ts,clients/tui/src/App.tsxink-formprintsform.titlein its own header and the modal prints it again, so the form now gets an untitled copy (untitledForm). I applied it to the prompt and resource-template modals too, since they had the same duplication (Get Prompt:/Read Resource:). Theroots: cleanRoots(...)comment now describes the #2432 roots editor (session-only, never writesmcp.json).clients/tui/src/utils/formHeader.ts, the three*TestModal.tsx,App.tsxMCP_INSPECTOR_API_TOKEN(or the legacyMCP_PROXY_AUTH_TOKEN) shorter than 32 characters prints a startup warning. It is still accepted. The generated token is 64 hex characters, so only an operator-chosen one can trigger it.server/web-server-config.tsDocs:
docs/mcp-server-configuration.md(seed path, ad-hoc naming),docs/environment-variables.md(warning),clients/web/README.md(sandbox Host check),clients/launcher/README.md(seed).Verification
npm run local:gategreen.page-has-heading-one,heading-order,regionandlandmark-one-mainon Servers, Tools (incl. an executed result), Apps, Prompts and Resources (incl. a read resource), plus Protocol and Network in the monitor. 0 violations. Before the heading shift, the same scan showedheading-orderfailing on every connected tab once the h1 was in.Screenshots
Web Tools tab on the prod build. Fredoka and Roboto Mono render from the bundled files, with no request to Google. The h1 is visually hidden, so the look is unchanged:
TUI tool form, before (title twice) and after. These are ANSI-stripped Ink frames rendered to PNG, so terminal colors are dropped:
🤖 Generated with Claude Code