Conversation
There was a problem hiding this comment.
Pull request overview
Adds an internal ADR for deep link behavior and updates documentation/code comments to reference it instead of a private external design document.
Changes:
- Added ADR-001 documenting deep link schema, platform considerations, and handling approach.
- Updated docs to link to ADR-001 instead of a private Google Doc.
- Updated the deep link definitions file header comment to point at the ADR.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| docs/adr/001-deep-links.md | Introduces ADR-001 describing the deep link design and operational considerations. |
| docs/README.md | Replaces the private design doc link with a link to ADR-001. |
| common/deep-links.ts | Updates the header comment to reference the ADR in-repo. |
|
|
||
| ## Context | ||
|
|
||
| ToolHive Studio needs a way for external systems (browsers, CLI tools, enterprise dashboards) to open the app and navigate to a specific view — for example, opening an MCP server detail page by name. This is commonly solved with custom protocol handlers (deep links). |
There was a problem hiding this comment.
The ADR title uses “ToolHive Desktop”, but this sentence refers to “ToolHive Studio”. Please pick one product name and use it consistently throughout the ADR (and ideally align with the name used elsewhere in the repo/config) to avoid confusion about what application the decision applies to.
| ToolHive Studio needs a way for external systems (browsers, CLI tools, enterprise dashboards) to open the app and navigate to a specific view — for example, opening an MCP server detail page by name. This is commonly solved with custom protocol handlers (deep links). | |
| ToolHive Desktop needs a way for external systems (browsers, CLI tools, enterprise dashboards) to open the app and navigate to a specific view — for example, opening an MCP server detail page by name. This is commonly solved with custom protocol handlers (deep links). |
|
|
||
| Example: `toolhive-gui://v1/open-registry-server-detail?serverName=fetch` | ||
|
|
||
| The URL is parsed using the standard `URL` constructor — the version maps to `url.host`, the intent to `url.pathname`, and parameters to `url.searchParams`. |
There was a problem hiding this comment.
This description says the intent maps to url.pathname, but the implementation strips the leading slash (i.e., url.pathname.replace(/^\//, '')). Please update the ADR wording to match the actual parsing behavior so readers don’t accidentally treat the intent as starting with '/'.
| The URL is parsed using the standard `URL` constructor — the version maps to `url.host`, the intent to `url.pathname`, and parameters to `url.searchParams`. | |
| The URL is parsed using the standard `URL` constructor — the version maps to `url.host`, the intent to `url.pathname` with the leading `/` stripped (i.e. `url.pathname.replace(/^\//, '')`), and parameters to `url.searchParams`. |
| Deep link definitions are co-located as Zod schemas that declare the intent name, parameter schema (with sanitization via regex), and navigation handler. The `deepLinkSchema` is a discriminated union over all registered intents. Invalid or unknown deep links are rejected at parse time. | ||
|
|
There was a problem hiding this comment.
This claims invalid/unknown deep links are “rejected at parse time”, but the current flow parses/validates and then explicitly navigates to the not-found experience (via the show-not-found intent) when parsing/validation fails. Please clarify the behavior in the ADR (e.g., “validation fails and we route to notFound”) to keep the Decision and Error handling sections consistent with the implementation.
peppescg
left a comment
peppescg
left a comment
There was a problem hiding this comment.
Why a ADR inside the docs?
this removes references to a private document from the codebase. The full document (with all details, about 30 pages) will still probably be published later, but probably for the repo this is enough