Skip to content

fix(terminal): make .md path links visibly clickable + harden fileRead errors#100

Merged
InbarR merged 2 commits into
InbarR:mainfrom
yoziv:bug/yoziv/terminal-md-link-clickability
May 8, 2026
Merged

fix(terminal): make .md path links visibly clickable + harden fileRead errors#100
InbarR merged 2 commits into
InbarR:mainfrom
yoziv:bug/yoziv/terminal-md-link-clickability

Conversation

@yoziv
Copy link
Copy Markdown
Contributor

@yoziv yoziv commented May 7, 2026

Summary

The xterm link provider for markdown file paths in TerminalPanel.tsx was registering .md matches without any decorations, so matched paths in terminal output had no visual affordance — users couldn't tell they were clickable until the cursor accidentally landed on one and the tooltip appeared.

This PR makes .md paths render with an underline and switch the cursor to a pointer on hover, mirroring the existing image link provider (line 609) and URL provider in the same file. Also tightens the fileRead error handling so silent failures become loggable instead of vanishing.

Tested locally: paths in gh copilot / claude output, in build error stacks, and in dir/Get-ChildItem listings all now show as clickable links.

Changes

File What
src/renderer/components/TerminalPanel.tsx (lines ~675–728) MD link provider — see details below

Detailed changes (1 file, +12 / −5)

  1. Add visual affordancedecorations: { underline: true, pointerCursor: true } on each registered link. Mirrors the proven pattern at line 609 (image link provider) in the same file.
  2. Tooltip textCtrl+Click to preview: <path>Click to preview: <path> (matches the no-modifier activation behavior — there is no Ctrl gating in the activate handler).
  3. Error handling on fileRead — was: silent drop on null and silent drop on throw. Now:
    • content === nullconsole.warn('[md-link] fileRead returned null', { fullPath }) and return.
    • Promise rejects → .catch logs console.error('[md-link] fileRead threw', { fullPath, err }).
      This helps triage future "looks clickable but doesn't open" reports without changing the happy path.
  4. Local links array type widened to match the proven shape used by the image link provider at line 531-536: activate: (e: MouseEvent, text: string) => void and decorations?: { underline?: boolean; pointerCursor?: boolean }. The implementation activate() { ... } remains compatible (TS allows fewer params in implementations).

Out of scope

  • The diagnostic console.warn / console.error are a deliberate, narrow addition for triage — only fire on actual failures, not on every click. Happy to drop them or wire to a typed logger if you'd prefer.
  • Did not touch the URL or image link providers (already have decorations). Did not change MD_PATH_PATTERN or the fileRead IPC contract.
  • Did not add the same diagnostic counter (__tmaxLinkActivates) the URL provider keeps for TASK-104/106 — happy to add a __tmaxMdLinkActivates if useful for symmetry.

Test plan

  • Manual (verified locally): .md paths in terminal output now render with underline + pointer cursor; clicking opens the markdown preview overlay.
  • No new automated tests in this PR — the existing test suite has no coverage for the link provider's decorations field or the fileRead callback branches. Happy to add a Playwright spec asserting the activate path if you'd like a follow-up commit.

Notes

  • Branch is rebased on latest main (18251d5).
  • TypeScript strictness: this PR introduces 1 new tsc --noEmit error at line 729 that is structurally identical to the pre-existing one at line 613 (image link provider). The codebase doesn't gate on tsc --noEmit (main has 29 errors and ships), so this is consistent with house style. A separate cleanup of xterm link-provider TS strictness across all 3 providers (URL, image, md) would be a worthwhile follow-up.

yoziv added 2 commits May 7, 2026 08:00
…d errors

The xterm link provider for markdown file paths was registering matches without `decorations`, so matched .md paths in terminal output had no visual affordance - users had no way to tell they were clickable until the cursor happened to land exactly on one.

Changes:

- Add `decorations: { underline: true, pointerCursor: true }` to the md link descriptors so paths render with an underline and switch the cursor to a pointer on hover (mirrors the existing image link provider at line 609).

- Update tooltip from 'Ctrl+Click to preview' to 'Click to preview' to match the no-modifier activation behavior.

- Restructure the fileRead promise so a null return logs a warning (fileRead returned null) and a thrown error logs an error (fileRead threw), instead of silently dropping both. Helps triage 'looks clickable but doesn't open' reports.

- Widen the local links array type to match the proven shape used by the image link provider (decorations?, activate signature with MouseEvent/text).
Adds a focused Playwright spec for the changes in this PR:

- The .md link provider now emits each link with `decorations: { underline: true, pointerCursor: true }` so xterm renders an underline and switches the cursor on hover. Pre-PR the field was missing entirely.

- The tooltip text was reworded from 'Ctrl+Click to preview:' to 'Click to preview:' to match the no-modifier activation behavior.

Coverage scope is deliberately narrow - just the link provider's output shape via `provideLinks`. The fileRead null/throw branches that were also tightened in this PR are NOT covered here because the existing fileRead spy pattern (`Object.defineProperty` on `terminalAPI`) is currently broken across the whole e2e suite (task-107 fails with `Cannot redefine property: fileRead` on current main, suggesting a contextBridge tightening in a recent Electron upgrade). Once the spy infra is restored, follow-up tests for the warn/error branches should slot in here.

Verified locally: 1 passed (4.4s).
@yoziv
Copy link
Copy Markdown
Contributor Author

yoziv commented May 7, 2026

Took a stab at the test myself in commit 95cf95e (tests/e2e/pr100-md-link-decorations.spec.ts) — 1 focused spec asserting the link provider emits decorations: { underline: true, pointerCursor: true } and the new tooltip text by querying the link provider directly via provideLinks.

Verified locally: 1 passed (4.4s).

Coverage scope is deliberately narrow — just the link provider's output shape. The fileRead null/throw branches are not covered because the existing fileRead spy pattern (Object.defineProperty on terminalAPI) is currently broken across the whole e2e suite — task-107-md-path-wrap-and-spaces.spec.ts fails with the same Cannot redefine property: fileRead error on current main, suggesting a contextBridge tightening in a recent Electron upgrade. Once that's restored, follow-up tests for the warn/error branches can slot in here. Happy to take that on as a separate PR if useful.

@InbarR InbarR merged commit ce4c1cf into InbarR:main May 8, 2026
6 of 7 checks passed
InbarR pushed a commit that referenced this pull request May 8, 2026
Ink-based TUIs (Claude Code, Copilot CLI) wrap paths at their content
width without setting xterm's `isWrapped` flag, so the existing
soft-wrap walk in the .md link provider only saw the queried row. A
path like `c:\Users\...\OneDrive -` + `Microsoft\...\note.md` fell
back to the bare-filename branch on the continuation row, then opened
cwd-relative against the wrong drive.

Mirror the image-path provider's hard-newline stitch:
- PATH_BODY seam check on both sides, capped at 4 rows so screens
  full of path-shaped tokens can't glue together.
- Forward + backward walk so a click on either head or continuation
  row reconstructs the same logical string.
- Restore one seam space when the post-wrap row had leading
  whitespace (preserves the spelling of paths with embedded spaces
  like `OneDrive - Microsoft\...`).
- Update Seg shape and offsetToRowCol to track soft-vs-hard rows
  and per-row leading WS, matching the image-path provider's model.

Rebased on top of PR #100 (ce4c1cf) which added decorations and
fileRead error handling to the same provider; both fixes compose
cleanly. No automated coverage in this commit - the e2e Playwright
spec drafted for the repro hit the same launch-stage infra failure
PR #100's author also flagged.
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.

2 participants