Skip to content

Commit d91d681

Browse files
Merge pull request #7458 from Shopify/river/parseable-link-format-in-banners
Opt-in CommonMark link parsing in Banner-rendered plain strings
2 parents 4b223fe + b2bb310 commit d91d681

7 files changed

Lines changed: 386 additions & 10 deletions

File tree

packages/cli-kit/src/private/node/ui/components/Alert.test.tsx

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -142,4 +142,38 @@ describe('Alert', async () => {
142142
"
143143
`)
144144
})
145+
146+
test("footnotes a long URL written as `[label](url)` in the body so it doesn't wrap inside the banner border", async () => {
147+
// Regression: a 100-char URL embedded as plain text wraps across the
148+
// banner border at ~78 cols, splitting the URL with │ characters and
149+
// making it neither clickable nor copy-pasteable. Marking the URL up
150+
// with `[label](url)` should place the label inline (with a `[N]`
151+
// anchor) and emit the URL in the post-banner footnote block.
152+
const longUrl =
153+
'https://shopify.dev/docs/apps/build/sales-channels/channel-config-extension#specification-properties'
154+
const options = {
155+
body: `See specification requirements: [docs](${longUrl})`,
156+
}
157+
158+
const {lastFrame} = render(<Alert type="error" {...options} />)
159+
const frame = unstyled(lastFrame()!)
160+
161+
// The URL must not appear inside the bordered box.
162+
const bodyLines = frame.split('\n').filter((line) => line.startsWith('│'))
163+
bodyLines.forEach((line) => {
164+
expect(line).not.toContain(longUrl)
165+
})
166+
167+
// The footnote block (rendered after the closing `╰`) must list the
168+
// URL. Ink wraps the long URL onto its own line when it exceeds terminal
169+
// width, so we assert the `[1]` anchor and the URL show up *outside* the
170+
// bordered box rather than as a single contiguous `[1] URL` substring.
171+
const closingBorderIndex = frame.indexOf('╰')
172+
expect(closingBorderIndex).toBeGreaterThanOrEqual(0)
173+
const afterBox = frame.slice(closingBorderIndex)
174+
expect(afterBox).toContain('[1]')
175+
expect(afterBox).toContain(longUrl)
176+
// And the body must reference the footnote.
177+
expect(frame).toContain('docs [1]')
178+
})
145179
})

packages/cli-kit/src/private/node/ui/components/FatalError.test.tsx

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -245,4 +245,37 @@ describe('FatalError', async () => {
245245
"
246246
`)
247247
})
248+
249+
test('routes a plain-string error.message through TokenizedText so opt-in `[label](url)` markdown renders as a footnote-backed link', async () => {
250+
// Regression: previously a `FatalError` constructed with a plain string
251+
// message bypassed `TokenizedText` and rendered as a bare `<Text>`,
252+
// meaning any URL embedded in the message wrapped against the banner
253+
// border. Wiring `error.message` through `TokenizedText` lets servers
254+
// (or the CLI itself) opt in to the same footnote treatment via
255+
// CommonMark `[label](url)` markdown.
256+
const longUrl =
257+
'https://shopify.dev/docs/apps/build/sales-channels/channel-config-extension#specification-properties'
258+
const error = new AbortError(`See specification requirements: [docs](${longUrl})`)
259+
260+
const {lastFrame} = render(<FatalError error={error} />)
261+
const frame = unstyled(lastFrame()!)
262+
263+
// The URL must not appear inside the bordered box.
264+
const bodyLines = frame.split('\n').filter((line) => line.startsWith('│'))
265+
bodyLines.forEach((line) => {
266+
expect(line).not.toContain(longUrl)
267+
})
268+
269+
// The footnote block must list the URL outside the box. Ink wraps the
270+
// long URL onto its own line when it exceeds terminal width, so we
271+
// assert the `[1]` anchor and the URL show up *after* the closing
272+
// border rather than as a single contiguous `[1] URL` substring.
273+
const closingBorderIndex = frame.indexOf('╰')
274+
expect(closingBorderIndex).toBeGreaterThanOrEqual(0)
275+
const afterBox = frame.slice(closingBorderIndex)
276+
expect(afterBox).toContain('[1]')
277+
expect(afterBox).toContain(longUrl)
278+
// And the body must reference the footnote with the markdown label.
279+
expect(frame).toContain('docs [1]')
280+
})
248281
})

packages/cli-kit/src/private/node/ui/components/FatalError.tsx

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,15 @@ const FatalError: FunctionComponent<FatalErrorProps> = ({error}) => {
4848
</Text>
4949
) : null}
5050

51-
{error.formattedMessage ? <TokenizedText item={error.formattedMessage} /> : <Text>{error.message}</Text>}
51+
{error.formattedMessage ? (
52+
<TokenizedText item={error.formattedMessage} />
53+
) : (
54+
// Route plain-string error messages through TokenizedText so that any
55+
// opt-in `[label](url)` / `<url>` markdown the server (or the caller)
56+
// emits is rendered as a footnote-backed Link inside the FatalError
57+
// banner.
58+
<TokenizedText item={error.message} />
59+
)}
5260

5361
{error.tryMessage ? <TokenizedText item={error.tryMessage} /> : null}
5462

packages/cli-kit/src/private/node/ui/components/Link.test.tsx

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import {Link} from './Link.js'
2+
import {LinksContext} from '../contexts/LinksContext.js'
23
import {render} from '../../testing/ui.js'
34
import {describe, expect, test, vi} from 'vitest'
45
import React from 'react'
@@ -100,6 +101,68 @@ describe('Link', async () => {
100101
expect(lastFrame()).toMatchInlineSnapshot('"https://example.com"')
101102
})
102103

104+
test('renders a label-bearing link inside a LinksContext as `label [N]` and registers the URL in the footnote table', async () => {
105+
// Inside a Banner's LinksContext, the visible label stays compact
106+
// (`label [1]`) and the URL is captured for rendering outside the
107+
// bordered box, where it can wrap freely without `│` interleaving.
108+
supportHyperLinks(false)
109+
110+
const links: Record<string, {label: string | undefined; url: string}> = {}
111+
const link = {
112+
url: 'https://shopify.dev/docs/apps/build/sales-channels/channel-config-extension#specification-properties',
113+
label: 'docs',
114+
}
115+
116+
const {lastFrame} = render(
117+
<LinksContext.Provider
118+
value={{
119+
links: {current: links},
120+
addLink: (label, url) => {
121+
const id = (Object.keys(links).length + 1).toString()
122+
links[id] = {label, url}
123+
return id
124+
},
125+
}}
126+
>
127+
<Link {...link} />
128+
</LinksContext.Provider>,
129+
)
130+
131+
expect(lastFrame()).toBe('docs [1]')
132+
expect(links['1']).toEqual({label: 'docs', url: link.url})
133+
})
134+
135+
test('renders a label-less link inside a LinksContext as a bare `[N]` anchor (no inline URL)', async () => {
136+
// Regression: previously this path rendered `${url} [N]`, putting the
137+
// long URL inside the bordered box and defeating the footnote
138+
// mechanism. The footnote alone is now the source of truth for the URL.
139+
supportHyperLinks(false)
140+
141+
const links: Record<string, {label: string | undefined; url: string}> = {}
142+
const link = {
143+
url: 'https://shopify.dev/docs/apps/build/sales-channels/channel-config-extension#specification-properties',
144+
}
145+
146+
const {lastFrame} = render(
147+
<LinksContext.Provider
148+
value={{
149+
links: {current: links},
150+
addLink: (label, url) => {
151+
const id = (Object.keys(links).length + 1).toString()
152+
links[id] = {label, url}
153+
return id
154+
},
155+
}}
156+
>
157+
<Link {...link} />
158+
</LinksContext.Provider>,
159+
)
160+
161+
expect(lastFrame()).toBe('[1]')
162+
expect(lastFrame()).not.toContain(link.url)
163+
expect(links['1']).toEqual({label: undefined, url: link.url})
164+
})
165+
103166
function supportHyperLinks(isSupported: boolean) {
104167
vi.mocked(supportsHyperlinks).stdout = isSupported
105168
}

packages/cli-kit/src/private/node/ui/components/Link.tsx

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -12,16 +12,19 @@ interface LinkProps {
1212

1313
function link(label: string | undefined, url: string, linksContext: LinksContextValue | null) {
1414
if (!supportsHyperlinks.stdout) {
15-
if (url === (label ?? url)) {
16-
return url
17-
}
18-
1915
if (linksContext === null) {
16+
if (url === (label ?? url)) {
17+
return url
18+
}
2019
return label ? `${label} ${chalk.dim(`( ${url} )`)}` : url
2120
}
2221

22+
// Inside a LinksContext, register every link in the footnote table — even
23+
// ones whose label equals the URL — so the visible label stays compact and
24+
// the URL is rendered outside the bordered box where it can wrap without
25+
// being interleaved with `│` characters.
2326
const linkId = linksContext.addLink(label, url)
24-
return `${label ?? url} [${linkId}]`
27+
return label ? `${label} [${linkId}]` : `[${linkId}]`
2528
}
2629

2730
return ansiEscapes.link(label ?? url, url)

packages/cli-kit/src/private/node/ui/components/TokenizedText.test.tsx

Lines changed: 175 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,40 @@
11
import {tokenItemToString, TokenizedText} from './TokenizedText.js'
2+
import {LinksContext, Link} from '../contexts/LinksContext.js'
23
import {unstyled} from '../../../../public/node/output.js'
34
import {render} from '../../testing/ui.js'
4-
import {describe, expect, test} from 'vitest'
5+
import {describe, expect, test, vi} from 'vitest'
6+
import supportsHyperlinks from 'supports-hyperlinks'
57

6-
import React from 'react'
8+
import React, {FunctionComponent, useRef} from 'react'
9+
10+
vi.mock('supports-hyperlinks')
11+
12+
// Matches the on-the-wire OSC 8 sequence emitted by `ansiEscapes.link`,
13+
// which is what `<Link>` ultimately renders when the terminal supports
14+
// hyperlinks. Format: `ESC ] 8 ; ; URL BEL TEXT ESC ] 8 ; ; BEL`.
15+
function asOsc8Link(url: string, label?: string) {
16+
return `\u001b]8;;${url}\u0007${label ?? url}\u001b]8;;\u0007`
17+
}
18+
19+
// Mirrors the LinksContext that <Banner> sets up at runtime, without pulling
20+
// the whole Banner border into these tests.
21+
const WithLinksContext: FunctionComponent<{children: React.ReactNode}> = ({children}) => {
22+
const links = useRef<Record<string, Link>>({})
23+
return (
24+
<LinksContext.Provider
25+
value={{
26+
links,
27+
addLink: (label, url) => {
28+
const newId = (Object.keys(links.current).length + 1).toString()
29+
links.current = {...links.current, [newId]: {label, url}}
30+
return newId
31+
},
32+
}}
33+
>
34+
{children}
35+
</LinksContext.Provider>
36+
)
37+
}
738

839
describe('TokenizedText', async () => {
940
test('renders arrays of items separated by spaces', async () => {
@@ -57,6 +88,148 @@ describe('TokenizedText', async () => {
5788
`)
5889
})
5990

91+
describe('markdown-link parsing in plain strings', async () => {
92+
test('renders strings without a markdown link unchanged', async () => {
93+
vi.mocked(supportsHyperlinks).stdout = false
94+
95+
const {lastFrame} = render(
96+
<WithLinksContext>
97+
<TokenizedText item="no link here, just text" />
98+
</WithLinksContext>,
99+
)
100+
101+
expect(lastFrame()).toBe('no link here, just text')
102+
})
103+
104+
test('does not linkify a bare URL — callers must opt in via `[label](url)` or `<url>`', async () => {
105+
vi.mocked(supportsHyperlinks).stdout = true
106+
const url = 'https://example.com/docs'
107+
108+
const {lastFrame} = render(
109+
<WithLinksContext>
110+
<TokenizedText item={`visit ${url} now`} />
111+
</WithLinksContext>,
112+
)
113+
114+
expect(lastFrame()).toBe(`visit ${url} now`)
115+
expect(lastFrame()).not.toContain(']8;;')
116+
})
117+
118+
test('replaces an opt-in `[label](url)` with the label and a `[N]` footnote anchor when the terminal does not support hyperlinks', async () => {
119+
vi.mocked(supportsHyperlinks).stdout = false
120+
const url = 'https://shopify.dev/docs/apps/build/sales-channels/channel-config-extension#specification-properties'
121+
122+
const {lastFrame} = render(
123+
<WithLinksContext>
124+
<TokenizedText item={`Reference: [See specification requirements](${url})`} />
125+
</WithLinksContext>,
126+
)
127+
128+
expect(lastFrame()).toBe('Reference: See specification requirements [1]')
129+
expect(lastFrame()).not.toContain(url)
130+
})
131+
132+
test('wraps the label of a `[label](url)` in OSC 8 escapes when the terminal supports hyperlinks', async () => {
133+
vi.mocked(supportsHyperlinks).stdout = true
134+
const url = 'https://example.com/docs'
135+
136+
const {lastFrame} = render(
137+
<WithLinksContext>
138+
<TokenizedText item={`Reference: [docs page](${url})`} />
139+
</WithLinksContext>,
140+
)
141+
142+
expect(lastFrame()).toContain(asOsc8Link(url, 'docs page'))
143+
})
144+
145+
test('renders a label-less `<url>` autolink as a `[N]` anchor and registers the URL in the footnote table', async () => {
146+
vi.mocked(supportsHyperlinks).stdout = false
147+
const url = 'https://shopify.dev/docs'
148+
149+
const {lastFrame} = render(
150+
<WithLinksContext>
151+
<TokenizedText item={`See specification requirements: <${url}>`} />
152+
</WithLinksContext>,
153+
)
154+
155+
expect(lastFrame()).toBe('See specification requirements: [1]')
156+
expect(lastFrame()).not.toContain(url)
157+
})
158+
159+
test('parses multiple opt-in links in the same string', async () => {
160+
vi.mocked(supportsHyperlinks).stdout = false
161+
const first = 'https://example.com/a'
162+
const second = 'https://example.com/b'
163+
164+
const {lastFrame} = render(
165+
<WithLinksContext>
166+
<TokenizedText item={`see [a](${first}) and <${second}>`} />
167+
</WithLinksContext>,
168+
)
169+
170+
expect(lastFrame()).toBe('see a [1] and [2]')
171+
})
172+
173+
test('parses back-to-back opt-in links separated only by whitespace', async () => {
174+
vi.mocked(supportsHyperlinks).stdout = false
175+
const first = 'https://example.com/a'
176+
const second = 'https://example.com/b'
177+
178+
const {lastFrame} = render(
179+
<WithLinksContext>
180+
<TokenizedText item={`<${first}> <${second}>`} />
181+
</WithLinksContext>,
182+
)
183+
184+
expect(lastFrame()).toBe('[1] [2]')
185+
})
186+
187+
test('does not parse markdown links that omit the http(s) scheme', async () => {
188+
vi.mocked(supportsHyperlinks).stdout = true
189+
190+
const {lastFrame} = render(
191+
<WithLinksContext>
192+
<TokenizedText item="see [the section](#anchor) for more" />
193+
</WithLinksContext>,
194+
)
195+
196+
expect(lastFrame()).toBe('see [the section](#anchor) for more')
197+
expect(lastFrame()).not.toContain(']8;;')
198+
})
199+
200+
test('does not parse opt-in markdown when no LinksContext is present (e.g. outside a Banner)', async () => {
201+
vi.mocked(supportsHyperlinks).stdout = true
202+
const url = 'https://example.com/docs'
203+
204+
const {lastFrame} = render(<TokenizedText item={`see [docs](${url}) now`} />)
205+
206+
expect(lastFrame()).toBe(`see [docs](${url}) now`)
207+
expect(lastFrame()).not.toContain(']8;;')
208+
})
209+
210+
test('echoing back a user-supplied URL inside an error message is left as plain text', async () => {
211+
// Regression: an earlier auto-detection approach would turn the
212+
// user's bad `--tunnel-url` value into a clickable OSC-8 link,
213+
// which is misleading. With opt-in markdown the bare URL stays
214+
// as-is and only the doc reference — which the server marks up —
215+
// becomes clickable.
216+
vi.mocked(supportsHyperlinks).stdout = true
217+
const tunnelUrl = 'https://wrong'
218+
const docUrl = 'https://shopify.dev/docs/tunnels'
219+
220+
const {lastFrame} = render(
221+
<WithLinksContext>
222+
<TokenizedText item={`Invalid tunnel URL: ${tunnelUrl}. See [tunnel docs](${docUrl}).`} />
223+
</WithLinksContext>,
224+
)
225+
226+
expect(lastFrame()).toContain(`Invalid tunnel URL: ${tunnelUrl}.`)
227+
expect(lastFrame()).toContain(asOsc8Link(docUrl, 'tunnel docs'))
228+
// The user-supplied URL must not be wrapped in an OSC 8 escape.
229+
expect(lastFrame()).not.toContain(`\u001b]8;;${tunnelUrl}`)
230+
})
231+
})
232+
60233
describe('tokenItemToString', async () => {
61234
test("doesn't add a space before char", async () => {
62235
expect(tokenItemToString(['Run', {char: '!'}])).toBe('Run!')

0 commit comments

Comments
 (0)