Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6f22b0da57
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b263ba9c7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c4d608ca85
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| "size-full [&>*:first-child]:mt-0 [&>*:last-child]:mb-0", | ||
| className, | ||
| )} | ||
| linkSafety={linkSafetyConfig} |
There was a problem hiding this comment.
Preserve local artifact link routing when enabling linkSafety
Enabling linkSafety here changes Streamdown’s link renderer to its intercepted link flow, so markdown links are no longer handled as plain anchors. useArtifactLinkHandler still only matches closest("a"), which means assistant markdown links stop going through resolveMarkdownHref/openResolvedPath; internal artifact paths are then routed through the modal’s openUrl path (or fail), breaking artifact navigation and bypassing the allowed-roots policy.
Useful? React with 👍 / 👎.
External links in chat messages were silently ignored in Tauri's
WKWebView. Add a custom MarkdownLink component that splits link
behavior by type: external links show a LinkSafetyModal confirmation
dialog before opening via @tauri-apps/plugin-opener, while internal
links render as plain <a> elements so useArtifactLinkHandler can
intercept them for artifact navigation.
This replaces Streamdown's built-in linkSafety which converts all
links to <button> elements, breaking the artifact policy layer that
relies on closest("a") to resolve internal paths.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c02f12a0c3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| @@ -0,0 +1,89 @@ | |||
| import { useCallback, useEffect, useRef, useState } from "react"; | |||
| import { openUrl } from "@tauri-apps/plugin-opener"; | |||
There was a problem hiding this comment.
Export openUrl in plugin-opener test mocks
Introducing the openUrl import here means any Vitest mock of @tauri-apps/plugin-opener must now provide that named export; existing mocks like src/features/chat/ui/__tests__/MessageBubble.test.tsx currently define only openPath, so module loading will fail with a missing-export error before assertions run. Please update affected mocks (or use a partial mock of the real module) so chat-related test suites keep running.
Useful? React with 👍 / 👎.
- Move isExternalHref to shared/lib/ to fix dependency inversion where shared/ui imported from features/chat - Reset isCopied state when LinkSafetyModal reopens via useEffect on isOpen, preventing stale 'Copied!' text on reopen - Await openUrl before closing LinkSafetyModal so the modal stays visible if the open fails - Lift modal state to MessageResponse level with a single LinkSafetyModal instance and React context, instead of mounting one Dialog per external link in the message - Update useArtifactLinkHandler doc comment to reference MarkdownLink instead of the now-disabled Streamdown linkSafety Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
- Update test description from 'Streamdown linkSafety modal' to 'MarkdownLink'\''s LinkSafetyModal' to match actual implementation - Clean up isExternalHref import/re-export in artifactPathPolicyCore: import once at top for local use, re-export the local binding instead of a separate re-export-from-source statement - Move hardcoded English strings in LinkSafetyModal to react-i18next under components.linkSafety namespace for i18n consistency - Add comment on MessageResponse memo comparator explaining that internal state (modalUrl) is intentionally outside the comparator Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
The LinkSafetyModal introduced in c02f12a imports openUrl from @tauri-apps/plugin-opener. Test files that mock this module only provided openPath, causing module loading to fail with a missing export error. Add openUrl: vi.fn() to all four affected mocks. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a8cc9f0326
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| "title": "Environment Variables", | ||
| "toggleVisibility": "Toggle value visibility" | ||
| }, | ||
| "linkSafety": { |
There was a problem hiding this comment.
Localize new link-safety keys in Spanish locale
LinkSafetyModal now reads common:components.linkSafety.*, but this commit only adds those keys to locales/en/common.json; locales/es/common.json is still missing them. In Spanish sessions the modal will fall back to English for every new label, creating a visible localization regression on a surface that was previously fully translated.
Useful? React with 👍 / 👎.
| <button | ||
| className="wrap-anywhere appearance-none text-left font-medium text-primary underline" | ||
| data-streamdown="link" | ||
| onClick={() => openModal?.(href ?? "")} | ||
| type="button" | ||
| > |
There was a problem hiding this comment.
Keep external markdown links as anchors for accessibility
Replacing external markdown links with <button> removes link semantics, so assistive tech users can no longer navigate these items via link lists and users lose standard link affordances (e.g., link context actions). This is an accessibility regression versus the previous anchor-based rendering; keep an <a href> element and intercept click behavior to open the safety modal instead of changing the role to a button.
Useful? React with 👍 / 👎.
| lower.startsWith("tel:") | ||
| ); | ||
| } | ||
| export { isExternalHref }; |
There was a problem hiding this comment.
Instead of the import-then-re-export shim, they should just update these two spots to import from @/shared/lib/isExternalHref directly and remove the re-export from artifactPathPolicyCore
| @@ -34,6 +34,7 @@ vi.mock("@tauri-apps/api/event", () => ({ | |||
|
|
|||
| vi.mock("@tauri-apps/plugin-opener", () => ({ | |||
There was a problem hiding this comment.
we can move this mock to src/test/setup.ts and can remove the duplicated mock setup in individual tests
|
Looks good! I left some comments. Before merging this PR, would you please address these including the i18n one from codex. Also one thought: would it be possible to keep external markdown links as something like That would keep the external-link flow the same, while preserving normal link semantics and accessibility. Since the modal and openUrl() handling are already in place, it seems like a pretty small change from the current implementation. |
Summary