-
-
Notifications
You must be signed in to change notification settings - Fork 1
fix(hyperlinks): check if OSC-8 is supported, fallback to just the link #92
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request enhances terminal hyperlink support by adding OSC 8 hyperlink detection and providing graceful fallbacks for terminals that don't support the feature. The implementation checks various environment variables (TERM, TERM_PROGRAM, VTE_VERSION, etc.) to determine terminal capabilities and adjusts output formatting accordingly.
- Added
hyperlinkSupported()function to detect OSC 8 hyperlink support across multiple terminal emulators - Modified
hyperlink()function to use OSC 8 sequences when supported or fall back to plain text with URL - Updated image rendering fallback to provide descriptive text when hyperlinks aren't supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| for _, supported := range supportedPrograms { | ||
| if strings.Contains(termProgram, supported) { |
Copilot
AI
Jan 4, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same strings.Contains() issue applies here for TERM_PROGRAM detection. A value like "my-vscode-fork" or "hyperion" would incorrectly match "vscode" or "hyper" respectively.
Consider using exact matching or prefix matching for more reliable detection, especially since TERM_PROGRAM values are typically well-defined (e.g., "iTerm.app", "vscode", "Hyper").
| if strings.Contains(termProgram, supported) { | |
| if strings.HasPrefix(termProgram, supported) { |
| func hyperlinkSupported() bool { | ||
| term := strings.ToLower(os.Getenv("TERM")) | ||
|
|
||
| // Terminals known to support OSC 8 hyperlinks | ||
| supportedTerms := []string{ | ||
| "kitty", | ||
| "ghostty", | ||
| "wezterm", | ||
| "alacritty", | ||
| "foot", | ||
| "tmux", | ||
| "screen", | ||
| } | ||
|
|
||
| for _, supported := range supportedTerms { | ||
| if strings.Contains(term, supported) { | ||
| return true | ||
| } | ||
| } | ||
|
|
||
| // Check for specific terminal programs | ||
| termProgram := strings.ToLower(os.Getenv("TERM_PROGRAM")) | ||
| supportedPrograms := []string{ | ||
| "iterm.app", | ||
| "hyper", | ||
| "vscode", | ||
| "ghostty", | ||
| "wezterm", | ||
| } | ||
|
|
||
| for _, supported := range supportedPrograms { | ||
| if strings.Contains(termProgram, supported) { | ||
| return true | ||
| } | ||
| } | ||
|
|
||
| // Check for VTE-based terminals (GNOME Terminal, etc.) | ||
| if os.Getenv("VTE_VERSION") != "" { | ||
| return true | ||
| } | ||
|
|
||
| // Check for specific environment variables that indicate hyperlink support | ||
| if os.Getenv("KITTY_WINDOW_ID") != "" || | ||
| os.Getenv("GHOSTTY_RESOURCES_DIR") != "" || | ||
| os.Getenv("WEZTERM_EXECUTABLE") != "" { | ||
| return true | ||
| } | ||
|
|
||
| return false | ||
| } | ||
|
|
||
| // hyperlink formats a string as either a terminal-clickable hyperlink or plain text with URL. | ||
| func hyperlink(url, text string) string { | ||
| if text == "" { | ||
| text = url | ||
| } | ||
| return fmt.Sprintf("\x1b]8;;%s\x07%s\x1b]8;;\x07", url, text) | ||
|
|
||
| supported := hyperlinkSupported() | ||
|
|
||
| if supported { | ||
| // Use OSC 8 hyperlink sequence for supported terminals | ||
| return fmt.Sprintf("\x1b]8;;%s\x07%s\x1b]8;;\x07", url, text) | ||
| } else { | ||
| // Fallback to plain text format for unsupported terminals | ||
| if text == url { | ||
| return fmt.Sprintf("<%s>", url) | ||
| } | ||
| return fmt.Sprintf("%s <%s>", text, url) | ||
| } | ||
| } |
Copilot
AI
Jan 4, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new hyperlinkSupported() function and the modified hyperlink() function lack test coverage. Given that the repository has comprehensive test coverage (as seen in view/html_test.go with TestProcessBody, TestMarkdownToHTML, etc.), tests should be added for:
hyperlinkSupported()with various combinations of environment variables (TERM, TERM_PROGRAM, VTE_VERSION, etc.)hyperlink()behavior in both supported and unsupported terminal scenarios- Edge cases like empty text, matching text and URL, etc.
These tests are important to ensure the terminal detection logic works correctly and the fallback behavior is as expected.
| } else { | ||
| // Fallback to plain text format for unsupported terminals | ||
| if text == url { | ||
| return fmt.Sprintf("<%s>", url) | ||
| } | ||
| return fmt.Sprintf("%s <%s>", text, url) | ||
| } | ||
| } | ||
|
|
Copilot
AI
Jan 4, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The else keyword is unnecessary here since the if block returns. Removing it would improve code clarity and reduce nesting, following Go best practices.
The code can be simplified to:
if supported {
// Use OSC 8 hyperlink sequence for supported terminals
return fmt.Sprintf("\x1b]8;;%s\x07%s\x1b]8;;\x07", url, text)
}
// Fallback to plain text format for unsupported terminals
if text == url {
return fmt.Sprintf("<%s>", url)
}
return fmt.Sprintf("%s <%s>", text, url)| } else { | |
| // Fallback to plain text format for unsupported terminals | |
| if text == url { | |
| return fmt.Sprintf("<%s>", url) | |
| } | |
| return fmt.Sprintf("%s <%s>", text, url) | |
| } | |
| } | |
| } | |
| // Fallback to plain text format for unsupported terminals | |
| if text == url { | |
| return fmt.Sprintf("<%s>", url) | |
| } | |
| return fmt.Sprintf("%s <%s>", text, url) | |
| } |
| return false | ||
| } | ||
|
|
||
| // hyperlink formats a string as either a terminal-clickable hyperlink or plain text with URL. |
Copilot
AI
Jan 4, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment should be updated to reflect that the function now does more than just format - it also performs terminal capability detection. Consider updating to: "hyperlink formats a string as either a terminal-clickable hyperlink (OSC 8) or plain text with URL, based on terminal support."
| // hyperlink formats a string as either a terminal-clickable hyperlink or plain text with URL. | |
| // hyperlink formats a string as either a terminal-clickable hyperlink (OSC 8) or plain text with URL, based on terminal support. |
| func hyperlinkSupported() bool { | ||
| term := strings.ToLower(os.Getenv("TERM")) | ||
|
|
||
| // Terminals known to support OSC 8 hyperlinks | ||
| supportedTerms := []string{ | ||
| "kitty", | ||
| "ghostty", | ||
| "wezterm", | ||
| "alacritty", | ||
| "foot", | ||
| "tmux", | ||
| "screen", | ||
| } | ||
|
|
||
| for _, supported := range supportedTerms { | ||
| if strings.Contains(term, supported) { | ||
| return true | ||
| } | ||
| } | ||
|
|
||
| // Check for specific terminal programs | ||
| termProgram := strings.ToLower(os.Getenv("TERM_PROGRAM")) | ||
| supportedPrograms := []string{ | ||
| "iterm.app", | ||
| "hyper", | ||
| "vscode", | ||
| "ghostty", | ||
| "wezterm", | ||
| } | ||
|
|
||
| for _, supported := range supportedPrograms { | ||
| if strings.Contains(termProgram, supported) { | ||
| return true | ||
| } | ||
| } | ||
|
|
||
| // Check for VTE-based terminals (GNOME Terminal, etc.) | ||
| if os.Getenv("VTE_VERSION") != "" { | ||
| return true | ||
| } | ||
|
|
||
| // Check for specific environment variables that indicate hyperlink support | ||
| if os.Getenv("KITTY_WINDOW_ID") != "" || | ||
| os.Getenv("GHOSTTY_RESOURCES_DIR") != "" || | ||
| os.Getenv("WEZTERM_EXECUTABLE") != "" { | ||
| return true | ||
| } | ||
|
|
||
| return false | ||
| } |
Copilot
AI
Jan 4, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hyperlinkSupported() function is called multiple times during email rendering (once per hyperlink and once for image fallback), but its result never changes during a single run. Each call performs multiple string operations and environment variable lookups, which is inefficient.
Consider using sync.OnceValue (available since Go 1.21) or a package-level cached variable to memoize the result. For example:
var hyperlinkSupportedOnce = sync.OnceValue(func() bool {
// detection logic here
})This pattern is similar to how environment checks are typically optimized in similar codebases.
| if hyperlinkSupported() { | ||
| s.ReplaceWithHtml(hyperlink(src, fmt.Sprintf("\n [Click here to view image: %s] \n", alt))) | ||
| } else { | ||
| s.ReplaceWithHtml(fmt.Sprintf("\n [Image: %s, %s] \n", alt, src)) | ||
| } |
Copilot
AI
Jan 4, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hyperlinkSupported() check is redundant here because the hyperlink() function already performs this check internally. When hyperlink support is detected, hyperlink(src, ...) will return the OSC 8 sequence; when not supported, it will return plain text with the URL.
This creates an inconsistency: regular links (line 386) use hyperlink() without an additional check, but image fallbacks check support explicitly and format differently. The image fallback should either use hyperlink() consistently or have a clear reason for the different behavior.
If the intention is to have different fallback text for images versus regular links, consider adding a parameter to the hyperlink() function to customize the fallback format.
| for _, supported := range supportedTerms { | ||
| if strings.Contains(term, supported) { | ||
| return true | ||
| } | ||
| } |
Copilot
AI
Jan 4, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using strings.Contains() for terminal detection can lead to false positives. For example, TERM=xterm-kitty-custom would match "kitty", but so would TERM=my-kitty-terminal or even TERM=kitten.
While most of these cases would still support OSC 8 hyperlinks (being kitty-derivatives), this could cause issues with terminals that have these strings in their name but don't support the feature.
Consider using more precise matching patterns, such as checking if the term string equals specific values or starts with known prefixes (e.g., strings.HasPrefix(term, "kitty") or term == "alacritty").
This pull request enhances terminal hyperlink support in the
view/html.gofile by introducing robust detection of terminal capabilities, ensuring hyperlinks are formatted appropriately based on the environment. The main changes include adding a new function to check for OSC 8 hyperlink support and updating hyperlink formatting logic to provide graceful fallbacks for unsupported terminals.Terminal hyperlink support improvements:
hyperlinkSupportedfunction to detect if the current terminal supports OSC 8 hyperlinks by checking terminal type, program, and relevant environment variables.hyperlinkfunction to use OSC 8 hyperlink sequences when supported, and fall back to plain text with the URL for unsupported terminals.Image rendering enhancements:
processBodyto use clickable hyperlinks for images when supported, and provide a descriptive fallback when not supported.