Display terminal links in cursor #1998
Conversation
|
|
Caution Review failedThe pull request is closed. WalkthroughThis set of changes updates how terminal hyperlink support is detected and how clickable links are generated in terminal output within the CLI package. A new local implementation replaces the previous reliance on external dependencies for hyperlink detection and link formatting. New utility modules are introduced for these purposes, and related dependencies are updated in the package configuration files. The patch for "supports-hyperlinks" is removed, and the "terminal-link" dependency is eliminated from both the CLI and SDK packages. Type declarations for the previous "terminal-link" module are also deleted. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant supportsHyperlinks
participant terminalLink
User->>CLI: Runs command that outputs a link
CLI->>terminalLink: Call terminalLink(text, url, options)
terminalLink->>supportsHyperlinks: Check if hyperlinks are supported (stdout/stderr)
supportsHyperlinks-->>terminalLink: Returns boolean (supported or not)
alt Hyperlinks supported
terminalLink-->>CLI: Return clickable link (with escape sequences)
else Not supported
terminalLink-->>CLI: Return fallback text (text + URL)
end
CLI-->>User: Outputs formatted link to terminal
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/cli-v3/src/utilities/terminalLink.ts (2)
40-54: Consider adding URL validation for security.The function doesn't validate the URL parameter, which could lead to security issues if user input is directly passed without sanitization, especially since this generates terminal escape sequences.
Consider adding basic URL validation:
function terminalLink( text: string, url: string, { target = "stdout", ...options }: { target?: "stdout" | "stderr" } & TerminalLinkOptions = {} ) { + // Basic URL validation + try { + new URL(url); + } catch (error) { + console.warn(`Invalid URL passed to terminalLink: ${url}`); + } + if (!supportsHyperlinks[target]) { // If the fallback has been explicitly disabled, don't modify the text itself. if (options.fallback === false) { return text; } return typeof options.fallback === "function" ? options.fallback(text, url) : `${text} (\u200B${url}\u200B)`; }
94-94: Consider using a more direct export pattern.The current pattern of naming the function and then re-exporting it could be simplified if there's no need to reference the function internally.
If the function doesn't need to be referenced elsewhere in this file, consider simplifying to:
- function terminalLink( + export function terminalLink( text: string, url: string, { target = "stdout", ...options }: { target?: "stdout" | "stderr" } & TerminalLinkOptions = {} ) { // implementation } terminalLink.isSupported = supportsHyperlinks.stdout; terminalLink.stderr = terminalLinkStderr; // ...terminalLinkStderr implementation... - export { terminalLink };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
package.json(1 hunks)packages/cli-v3/package.json(3 hunks)packages/cli-v3/src/utilities/cliOutput.ts(1 hunks)packages/cli-v3/src/utilities/supportsHyperlinks.ts(1 hunks)packages/cli-v3/src/utilities/terminalLink.ts(1 hunks)packages/cli-v3/types.d.ts(0 hunks)packages/trigger-sdk/package.json(0 hunks)patches/supports-hyperlinks@2.3.0.patch(0 hunks)
💤 Files with no reviewable changes (3)
- packages/trigger-sdk/package.json
- patches/supports-hyperlinks@2.3.0.patch
- packages/cli-v3/types.d.ts
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: units / 🧪 Unit Tests
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (17)
package.json (1)
75-83: Dependency cleanup: Patch removal is appropriate.The removal of the "supports-hyperlinks@2.3.0" patch from patchedDependencies is correct since this functionality has been internalized with a custom implementation in
packages/cli-v3/src/utilities/supportsHyperlinks.ts. This aligns with the PR objective to directly incorporate the necessary code into the CLI.packages/cli-v3/src/utilities/cliOutput.ts (1)
3-3: Import update looks good.The import statement has been updated to use the local implementation of
terminalLinkfrom a relative path instead of the external package. This change is consistent with the PR objective to directly incorporate the necessary code into the CLI.packages/cli-v3/package.json (3)
94-94: Dependencies addition: ansi-escapes is appropriate.The addition of the
ansi-escapespackage is necessary for the new terminal link implementation to generate hyperlink escape sequences.
107-107: Dependencies addition: has-flag is appropriate.The addition of the
has-flagpackage is necessary for the new hyperlinks support detection utility to parse CLI flags.
128-128: Dependencies replacement: supports-color replaces terminal-link.The change from
terminal-linktosupports-coloris appropriate as part of the transition to an internal implementation. Thesupports-colorpackage is used by the new hyperlinks support detection utility.packages/cli-v3/src/utilities/supportsHyperlinks.ts (5)
1-8: License attribution is properly included.The MIT license attribution to the original authors (Sindre Sorhus and James Talmage) is correctly included, which is good practice when adapting open-source code.
13-30: Version parsing implementation looks good.The
parseVersionfunction correctly handles different version string formats, including the 4-digit format sometimes used in environment variables.
38-150: Comprehensive hyperlink support detection implementation.The
createSupportsHyperlinksfunction thoroughly checks various conditions to determine if hyperlinks are supported:
- Environment variables (FORCE_HYPERLINK, NETLIFY, etc.)
- CLI flags
- Color support
- TTY status
- Various terminal types and versions
The implementation properly handles edge cases and includes the
CURSOR_TRACE_IDenvironment variable check.
99-101: CURSOR_TRACE_ID support included.This check for the
CURSOR_TRACE_IDenvironment variable ensures compatibility with the cursor tracing functionality, addressing the core issue mentioned in the PR objective.
153-158: Efficient implementation with precomputed values.The exported
supportsHyperlinksobject precomputes the support status for stdout and stderr, which is an efficient approach that avoids redundant calculations.packages/cli-v3/src/utilities/terminalLink.ts (7)
1-7: Correctly attributed MIT license header.The license header properly credits Sindre Sorhus as the original author, which is important since this appears to be an adaptation of his terminal-link package now implemented locally.
9-10: Clean dependency management with local imports.Good job replacing the external terminal-link dependency with local implementation. Using ansiEscapes for the escape sequences and a dedicated local supportsHyperlinks utility follows good separation of concerns.
12-18: Well-documented type definition with flexible fallback options.The TerminalLinkOptions type provides good flexibility for handling fallback behavior with proper JSDoc documentation.
51-54: Smart use of zero-width spaces in the fallback format.Good implementation detail using zero-width spaces (\u200B) around the URL in the default fallback to prevent terminal parsing issues.
56-57: Clean implementation for supported terminals.Properly leverages the ansiEscapes.link method for terminals that support hyperlinks.
58-64: Static properties and methods nicely enhance the main function.Good pattern of extending the function with static properties and methods for checking support and providing an alternative output stream.
66-85: Well-documented stderr function with good code reuse.The terminalLinkStderr function effectively reuses the main function logic with different target settings, accompanied by thorough documentation.
Our previous patch was never applied to CLI releases. Easiest fix was to pull in the required package code directly into the CLI and add our patch there. This also removes the old patch and
terminal-linkpackage.Summary by CodeRabbit