Skip to content

[codex] Fix v2 terminal OSC links#3736

Merged
Kitenite merged 1 commit into
mainfrom
xterm-link-detector-fix
Apr 25, 2026
Merged

[codex] Fix v2 terminal OSC links#3736
Kitenite merged 1 commit into
mainfrom
xterm-link-detector-fix

Conversation

@Kitenite
Copy link
Copy Markdown
Collaborator

@Kitenite Kitenite commented Apr 25, 2026

Summary

Fixes OSC 8 hyperlinks in the desktop terminal by wiring xterm's built-in linkHandler into Superset's existing URL link action path.

Root Cause

xterm 6 always registers its own OscLinkProvider before custom providers. When options.linkHandler is unset, OSC 8 links fall back to xterm's default confirm(...) + window.open() behavior. In Electron this shows the warning dialog, but allowing it does not open the link reliably and bypasses Superset's terminal link preferences.

What Changed

  • Keep Superset's existing URL provider for visible/plain/wrapped URLs.
  • Add an OSC 8 linkHandler in TerminalLinkManager that delegates activation and hover events to the same URL callbacks.
  • Preserve the existing modifier behavior and internal-vs-external link preference because the consumer callback still owns activation decisions.
  • Clean up only the handler installed by TerminalLinkManager, so another owner is not clobbered if it replaces terminal.options.linkHandler.
  • Add focused tests for OSC 8 routing and link-handler lifecycle cleanup.

Design Notes

I checked the vendored/upstream behavior again:

  • xterm's built-in OSC 8 provider is the source of the warning/no-op path.
  • VS Code handles this by setting xterm.options.linkHandler and routing to its terminal URL opener.
  • Tabby also sets xterm.options.linkHandler for OSC links and uses web-link detection separately for printed URLs.

This PR follows that split: use our detector for printed URLs, use xterm's handler hook for OSC 8 links.

Validation

  • bun test apps/desktop/src/renderer/lib/terminal/terminal-link-manager.test.ts apps/desktop/src/renderer/lib/terminal/links/link-detector-adapter.test.ts apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/link-providers/url-link-provider.test.ts
  • bunx biome check apps/desktop/src/renderer/lib/terminal/terminal-link-manager.ts apps/desktop/src/renderer/lib/terminal/terminal-link-manager.test.ts
  • bun run --cwd apps/desktop typecheck

Summary by cubic

Fixes OSC 8 hyperlinks in the desktop terminal by handling them via @xterm/xterm’s linkHandler and routing to our URL opener. Links now open reliably in Electron and respect modifier keys and internal/external link preferences.

  • Bug Fixes
    • Use terminal.options.linkHandler for OSC 8 and keep existing detector for printed URLs.
    • Delegate activate/hover/leave to existing URL callbacks and disallow non-HTTP protocols.
    • Only clear the handler installed by TerminalLinkManager on dispose to avoid clobbering others.
    • Add focused tests for OSC 8 routing and handler lifecycle.

Written for commit 7609a7d. Summary will update on new commits.

Summary by CodeRabbit

  • Bug Fixes

    • Improved terminal hyperlink handling to ensure consistent routing through the application's link handler.
    • Fixed handler cleanup to prevent stale or duplicate handlers in the terminal.
  • Tests

    • Added comprehensive test coverage for terminal link management.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 25, 2026

Caution

Review failed

Pull request was closed or merged during review

📝 Walkthrough

Walkthrough

A new test suite validates TerminalLinkManager behavior, and the implementation adds a custom OSC 8 hyperlink handler that routes activations through onUrlClick while managing proper handler lifecycle and cleanup.

Changes

Cohort / File(s) Summary
Terminal Link Handler
apps/desktop/src/renderer/lib/terminal/terminal-link-manager.ts, apps/desktop/src/renderer/lib/terminal/terminal-link-manager.test.ts
Implements custom OSC 8 linkHandler with state tracking in _oscLinkHandler field, routing hyperlink activations through onUrlClick callback. Adds comprehensive test coverage validating handler installation, event routing for activation/hover/leave, and proper cleanup on disposal while preserving externally-set handlers.

Sequence Diagram(s)

sequenceDiagram
    participant App as Application
    participant Manager as TerminalLinkManager
    participant Terminal as xterm Terminal
    participant Provider as LinkProvider

    App->>Manager: setHandlers(onUrlClick, onHover, onLeave)
    Manager->>Manager: Create OSC 8 linkHandler
    Manager->>Terminal: Install linkHandler in options
    Terminal->>Manager: Handler registered

    User->>Terminal: Click OSC 8 hyperlink
    Terminal->>Manager: Invoke linkHandler with URL
    Manager->>App: Call onUrlClick(URL)

    User->>Terminal: Hover over hyperlink
    Terminal->>Manager: Invoke linkHandler (hover event)
    Manager->>App: Call onHover(URL)

    User->>Terminal: Leave hyperlink
    Terminal->>Manager: Invoke linkHandler (leave event)
    Manager->>App: Call onLeave()

    App->>Manager: dispose()
    Manager->>Terminal: Clear linkHandler from options
    Manager->>Manager: Clean up _oscLinkHandler
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 Hops with delight

OSC 8 links now hop through the wire,
A custom handler fuels the fire—
Click, hover, leave, each event so fine,
The manager cleans up, keeping handlers in line! 🔗✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title "[codex] Fix v2 terminal OSC links" clearly and specifically identifies the main change: fixing OSC 8 hyperlink handling in the terminal, summarized concisely without unnecessary detail.
Description check ✅ Passed The description comprehensively covers all template sections: a detailed summary explaining the bug and solution, related issues context, bug fix classification, validation steps, and additional design notes. All required information is present and well-organized.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch xterm-link-detector-fix

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented Apr 25, 2026

Greptile Summary

This PR fixes OSC 8 hyperlinks in the Electron terminal by setting xterm's options.linkHandler to route activation, hover, and leave events through the existing onUrlClick/onLinkHover/onLinkLeave callbacks, bypassing xterm's default confirm() + window.open() path. The handler lifecycle is well-managed: _clearOscLinkHandler uses an identity check to ensure only the handler installed by TerminalLinkManager is ever cleared.

Confidence Score: 5/5

Safe to merge — the change is well-scoped, all edge cases are covered by tests, and the lifecycle management is correct.

No P0 or P1 issues found. The OSC handler installation and cleanup logic are correct, the identity-check guard prevents clobbering external handlers, and allowNonHttpProtocols: false is an appropriate conservative default. Three focused tests validate the new behavior.

No files require special attention.

Important Files Changed

Filename Overview
apps/desktop/src/renderer/lib/terminal/terminal-link-manager.ts Adds OSC 8 link handler via xterm's linkHandler option, routing activation/hover/leave to existing URL callbacks. Lifecycle is clean: handler is only cleared if it was the one installed by this manager, preventing clobbering external owners.
apps/desktop/src/renderer/lib/terminal/terminal-link-manager.test.ts New test file covering three scenarios: OSC 8 callback routing, handler cleanup on dispose, and non-clobbering of externally-installed handlers. Coverage is focused and correct.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[OSC 8 hyperlink in terminal] -->|xterm intercepts| B{options.linkHandler set?}
    B -->|No - old behavior| C[xterm default confirm + window.open blocked in Electron]
    B -->|Yes - new behavior| D[TerminalLinkManager oscLinkHandler]
    D -->|activate| E[onUrlClick callback]
    D -->|hover| F[onLinkHover kind url]
    D -->|leave| G[onLinkLeave callback]

    H[Plain or wrapped URL in terminal] -->|xterm link provider| I[UrlLinkProvider]
    I -->|activate| E
    I -->|hover| F
    I -->|leave| G

    J[dispose or re-register] --> K{linkHandler === oscLinkHandler?}
    K -->|Yes own handler| L[Set to null]
    K -->|No external handler| M[Leave untouched]
Loading

Reviews (1): Last reviewed commit: "fix v2 terminal osc links" | Re-trigger Greptile

@Kitenite Kitenite merged commit 682d07c into main Apr 25, 2026
5 of 7 checks passed
@Kitenite Kitenite deleted the xterm-link-detector-fix branch April 25, 2026 20:17
@github-actions
Copy link
Copy Markdown
Contributor

🧹 Preview Cleanup Complete

The following preview resources have been cleaned up:

  • ⚠️ Neon database branch

Thank you for your contribution! 🎉

MocA-Love added a commit to MocA-Love/superset that referenced this pull request Apr 26, 2026
PR1〜PR5 (#435 #436 #437 #438 #440) で 13 commits 全件 cherry-pick + 手動 conflict 解消で取り込み済み。
本コミットは git 履歴上 behind=0 とするための ours マージ記録。

取り込み済み 13 commits:
- 1f55c62 Fix host service restart adoption (superset-sh#3732)
- 0fe65d2 test(desktop): remove host-service-coordinator test (superset-sh#3734)
- 3012b5a Add optimistic Electric collection updates (superset-sh#3722)
- c272a51 fix(desktop): drop branch row from v2 sidebar workspace item (superset-sh#3733)
- c2f3fdc feat(desktop): add fade-edge mask utilities (superset-sh#3735)
- 682d07c fix v2 terminal osc links (superset-sh#3736)
- 7c0d22b feat(ports): surface remote host-service ports in the sidebar (superset-sh#3676)
- 6a3be2d [codex] Stabilize v2 terminal resize (superset-sh#3739)
- 8928ac6 [codex] Improve v2 pane header responsiveness (superset-sh#3737)
- 5fe3d22 refactor(desktop): tidy v2 terminal session dropdown (superset-sh#3743)
- 66c23d6 Fix automation timezone scheduling (superset-sh#3738)
- 16e270c [codex] Add terminal session titles (superset-sh#3740)
- 583fa5d fix(desktop): refit v2 terminal after font settle (superset-sh#3742)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant