Auto-detect and linkify URLs in plain-string TokenizedText items#7384
Auto-detect and linkify URLs in plain-string TokenizedText items#7384ryancbahan wants to merge 3 commits into
Conversation
isaacroldan
left a comment
There was a problem hiding this comment.
Review assisted by pair-review
| * links, and commands. | ||
| */ | ||
| const URL_REGEX = /https?:\/\/\S+/g | ||
| const URL_TRAILING_PUNCTUATION = /[.,;:!?)\]}>'"]+$/ |
There was a problem hiding this comment.
🐛 Bug: URL_TRAILING_PUNCTUATION = /[.,;:!?)\]}>'"]+$/ unconditionally strips ), ], }, and > from the end of a matched URL, but these characters are legal URL suffixes. Real-world examples that would break: Wikipedia disambiguation links like https://en.wikipedia.org/wiki/Ruby_(programming_language), MDN Function() pages, Microsoft Learn URLs using parens in paths, and query strings ending in JSON-array-style values. Because this helper is now the chokepoint for every plain string passed through TokenizedText, any such URL in a server-returned error message will have its closing bracket stripped, and the <Link> component will emit an OSC 8 escape pointing at a broken target (with the trailing ) rendered outside the link). None of the new tests exercise URLs containing (), so this silent corruption is uncovered.
Suggestion: Narrow the character class to unambiguous sentence punctuation, or balance-count parens before stripping. Minimal fix:
| const URL_TRAILING_PUNCTUATION = /[.,;:!?)\]}>'"]+$/ | |
| const URL_TRAILING_PUNCTUATION = /[.,;:!?'"]+$/ |
And add a test for https://en.wikipedia.org/wiki/Foo_(bar) to lock the behavior in.
| const TokenizedText: FunctionComponent<TokenizedTextProps> = ({item}) => { | ||
| if (typeof item === 'string') { | ||
| return <Text>{item}</Text> | ||
| return renderStringWithLinks(item) |
There was a problem hiding this comment.
📐 Design: The PR is framed as a Banner/alert fix, but renderStringWithLinks runs on every plain-string token passing through TokenizedText. That includes Alert, FatalError, List, TabularData, DangerousConfirmationPrompt, Prompts/PromptLayout, Prompts/InfoMessage, and TextPrompt. In non-Banner contexts there's no LinksContext, so the Link component falls back to raw-URL output for non-OSC-8 terminals (preserved) but emits OSC 8 escapes on OSC-8-capable terminals (behavior change). A concrete example: the placeholder URL in packages/app/src/cli/services/dev/urls.ts:86 (Valid format: "https://my-tunnel-url:port") will now be rendered as a clickable escape to a non-existent host. Reviewers of downstream packages won't catch this from the PR description alone.
Suggestion: Either (a) scope the auto-detection to contexts that own a LinksContext by threading an explicit flag through (e.g. an autoLinkify prop or opt-in from Banner/Alert/FatalError), or (b) keep the current broad behavior but update the PR description/changeset and add snapshot coverage for non-Banner callers (TabularData, List, prompts) so the behavior change is visible at review time.
There was a problem hiding this comment.
You can trigger this one by running:
pnpm shopify app dev --tunnel-url https://wrong
| expect(lastFrame()).toContain(`]8;;${url}${url}]8;;`) | ||
| }) | ||
|
|
||
| test('detects multiple URLs in the same string', async () => { |
There was a problem hiding this comment.
Might be worth adding edge test cases where there's back to back URLs
| * `TokenizedText` renders a text string with tokens that can be either strings, | ||
| * links, and commands. | ||
| */ | ||
| const URL_REGEX = /https?:\/\/\S+/g |
There was a problem hiding this comment.
Are there any existing libraries we can use to validate URLs?
There was a problem hiding this comment.
we're actively trying to reduce lib usage given the amount of supply chain attacks.
- Narrow URL_TRAILING_PUNCTUATION so legal URL chars `)`, `]`, `}`, `>` are no longer stripped (Wikipedia/MDN/Microsoft Learn URLs). - Gate renderStringWithLinks on a non-null LinksContext so non-Banner consumers (TabularData, prompts, InfoMessage) keep their original rendering unchanged. - Validate detected URLs via the WHATWG URL constructor and require the hostname to contain a dot. Rejects user typos and placeholder strings like https://asda and https://my-tunnel-url:port that should not appear as clickable hyperlinks (the case Isaac screenshotted). Tests: - TokenizedText: parens-in-URL, back-to-back URLs, no-context passthrough, no-dot hostname, non-numeric port. - Link: label-less `[N]` rendering and always-footnote registration inside a LinksContext. - FatalError: snapshot reproducing the AbortError tunnel-url case so the regression is locked in. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- isLikelyRealUrl now narrows to TypeError (URL constructor's failure type) and rethrows anything else (no-catch-all/no-catch-all). - Reformat WithLinksContext prop type onto multiple lines (prettier). - Drop unnecessary backslash escapes from a template-literal inline snapshot in FatalError.test.tsx (no-useless-escape). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Differences in type declarationsWe detected differences in the type declarations generated by Typescript for this branch compared to the baseline ('main' branch). Please, review them to ensure they are backward-compatible. Here are some important things to keep in mind:
New type declarationsWe found no new type declarations in this PR Existing type declarationspackages/cli-kit/dist/private/node/ui/components/TokenizedText.d.ts@@ -40,9 +40,5 @@ export declare function appendToTokenItem(token: TokenItem, suffix: string): Tok
interface TokenizedTextProps {
item: TokenItem;
}
-/**
- * renders a text string with tokens that can be either strings,
- * links, and commands.
- */
declare const TokenizedText: FunctionComponent<TokenizedTextProps>;
export { TokenizedText };
\ No newline at end of file
|
Catherine-Fu
left a comment
There was a problem hiding this comment.
The expected behaviour is that the link in CLI validation messages should be easily clickable or copyable as a complete, valid URL.
I don’t have much context on the CLI library, so I’d prefer to have another reviewer take a look and approve this.
Merge activity
|
|
This PR seems inactive. If it's still relevant, please add a comment saying so. Otherwise, take no action. |


WHY are these changes introduced?
Long URLs embedded as plain strings in CLI error messages wrap across multiple lines inside the fixed-width error banner (~53 chars on an 80-col terminal). The wrap splits the URL with the border's
│characters, producing output that is neither clickable nor copy-pasteable as a single URL.WHAT is this pull request doing?
Three small changes in
packages/cli-kit:TokenizedText.tsx— NewrenderStringWithLinkshelper auto-detectshttp(s)://…URLs in plain-string tokens, strips common trailing sentence punctuation, and routes each URL through the existing<Link>component. This is the chokepoint through which all banner/alert bodies render, so server-returned error strings benefit automatically.FatalError.tsx— Theerror.messagefallback (used when aFatalErroris constructed with a plainOutputMessageinstead of aTokenItem) now flows throughTokenizedTextinstead of a bare<Text>, picking up the same URL treatment.Link.tsx— Two fixes to the non-OSC-8 fallback path, both limited to behavior inside aLinksContext(i.e. inside a Banner):[N]as the in-body anchor instead ofurl [N](prevents the URL from being visible in-body where it would wrap).LinksContextis present, regardless of whether a label was provided (previously the method short-circuited for label-less links and emitted the raw URL inline).Net effect
Before:
After:
On OSC 8 terminals (iTerm2, modern Terminal.app, VS Code, Warp, Kitty, WezTerm, Ghostty) the
[1]anchor is a clickable hyperlink to the full URL. On terminals without OSC 8 support, the URL appears outside the bordered box in the footnote block, where it can still wrap at terminal width but is not interleaved with│characters, so copy-paste recovers the full URL.How to test your changes?
Unit coverage added
TokenizedText.test.tsx: 5 tests — no-URL passthrough, URL preserved without OSC 8, URL wrapped in OSC 8 when supported, multiple URLs, trailing punctuation stripped.Alert.test.tsxandFatalError.test.tsx: inline snapshot integration tests that render a long-URL message inside a realBanner/FatalErrorand lock in the[1]in-body + URL-in-footnote layout.Visual QA in a real terminal
pnpm --filter @shopify/cli-kit buildrenderError/renderFatalErrorwith a plain-string body containing a long URL, then run it withnode.│, the[1]anchor is clickable (OSC 8 terminals), and copy-pasting the footnote URL yields a single clean URL.FORCE_HYPERLINK=0and confirm the[N]+ footnote layout still renders cleanly.Post-release steps
None.
Checklist
supports-hyperlinksdependency.patchchangeset (bug fix).