fix(tui): fixed link bug with glamour update#86
Merged
ontehfritz merged 3 commits intomainfrom Mar 28, 2026
Merged
Conversation
|
Caution Review failedPull request was closed or merged during review WalkthroughReworks TUI hyperlink handling to detect OSC 8 hyperlink regions, match links by URL/visible-text within those regions, and inject start/end markers accordingly; updates parsing to skip OSC (BEL/ST) and CSI escapes and adapts Bubble Tea API/viewport/mouse key changes and tests to exercise OSC 8 behavior. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Possibly related PRs
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/cmd/demarkus-tui/main.go`:
- Around line 1004-1016: The function textStartAfterOSC currently assumes end
points at the ESC and returns end+2, but the OSC-handling code returns end
pointing at the trailing '\' for ST terminators (see the earlier return of
end+1), so adjust textStartAfterOSC to advance only one rune for ST-terminated
sequences: detect BEL vs ST as before (runes[end] == '\x07' -> end+1) and for
the ST case return end+1 instead of end+2 so the visible textStart aligns with
the first visible rune; update the textStartAfterOSC function accordingly.
- Around line 937-942: The OSC URL comparison needs normalization to ignore URL
fragments: before computing urlMatch in the loop that iterates over hyperlinks
(variables: hyperlinks, used, hl.url, info.Dest, info.Text), strip or remove the
fragment portion (the part after '#') from hl.url and from info.Dest (or
normalize hl.url to the same form as LinkInfo.Dest which already has fragments
removed) and then perform the equality/suffix checks; update the urlMatch
computation to compare the fragment-stripped values (or use a parsed URL with
Fragment cleared) so links like page.md#top match LinkInfo.Dest and get markers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e4804128-b730-441b-8d3b-e91c54c02c8c
📒 Files selected for processing (2)
client/cmd/demarkus-tui/links_test.goclient/cmd/demarkus-tui/main.go
Contributor
Author
|
@coderabbitai what is left to get approval? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 9628
You're clear to merge. |
There was a problem hiding this comment.
♻️ Duplicate comments (2)
client/cmd/demarkus-tui/main.go (2)
952-957:⚠️ Potential issue | 🟠 MajorNormalize fragments before URL matching.
links.LinkInfo.Destis fragment-stripped, buthl.urlmay still contain#fragment. This misses valid matches, so markers are not inserted and those links won’t be reachable by Tab/click.Proposed fix
for j, hl := range hyperlinks { if used[j] { continue } - urlMatch := hl.url == info.Dest || strings.HasSuffix(hl.url, info.Dest) || strings.HasSuffix(info.Dest, hl.url) + hlURL := hl.url + if idx := strings.IndexByte(hlURL, '#'); idx != -1 { + hlURL = hlURL[:idx] + } + dest := info.Dest + if idx := strings.IndexByte(dest, '#'); idx != -1 { + dest = dest[:idx] + } + urlMatch := hlURL == dest || strings.HasSuffix(hlURL, dest) || strings.HasSuffix(dest, hlURL) if urlMatch && hl.text == info.Text { insertions = append(insertions, insertion{runePos: hl.textStart, marker: string(markerStart(i))},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/cmd/demarkus-tui/main.go` around lines 952 - 957, The URL matching misses cases where hl.url contains a fragment (e.g., `#anchor`) while info.Dest is already fragment-stripped; update the matching logic in the loop over hyperlinks (variables: hyperlinks, used, hl.url, info.Dest, info.Text) to normalize hl.url by removing any fragment before comparing — e.g., parse or split hl.url to strip the part after '#' and then perform the equality and HasSuffix checks against info.Dest and keep the existing hl.text == info.Text check so markers are inserted correctly for fragmented links.
1025-1032:⚠️ Potential issue | 🟠 MajorST-terminated OSC links start one rune late.
For ST (
ESC \),parseOSCreturns the trailing\index, buttextStartAfterOSCadvances by 2. The start marker shifts right by one visible rune, causing partial highlight/click regions.Proposed fix
-// textStartAfterOSC returns the rune index where visible text begins after an -// OSC terminator. end points to the BEL or first byte of ST. +// textStartAfterOSC returns the rune index where visible text begins after an +// OSC terminator. parseOSC returns the BEL index or the trailing '\' of ST. func textStartAfterOSC(runes []rune, end int) int { if end < len(runes) && runes[end] == '\x07' { return end + 1 } - return end + 2 // skip \x1b\\ + return end + 1 }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/cmd/demarkus-tui/main.go` around lines 1025 - 1032, textStartAfterOSC currently advances two runes for ST-terminated OSCs, which shifts the visible start one rune too far; change the logic in textStartAfterOSC (and adjust related handling in parseOSC if needed) so that when end points to BEL ('\x07') or the ST backslash ('\\') you return end+1, and only return end+2 for the legacy two-rune sequence case; reference the textStartAfterOSC function (and parseOSC which returns the trailing '\' index) to locate and correct the ST handling so ST-terminated OSC links start at the correct rune.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@client/cmd/demarkus-tui/main.go`:
- Around line 952-957: The URL matching misses cases where hl.url contains a
fragment (e.g., `#anchor`) while info.Dest is already fragment-stripped; update
the matching logic in the loop over hyperlinks (variables: hyperlinks, used,
hl.url, info.Dest, info.Text) to normalize hl.url by removing any fragment
before comparing — e.g., parse or split hl.url to strip the part after '#' and
then perform the equality and HasSuffix checks against info.Dest and keep the
existing hl.text == info.Text check so markers are inserted correctly for
fragmented links.
- Around line 1025-1032: textStartAfterOSC currently advances two runes for
ST-terminated OSCs, which shifts the visible start one rune too far; change the
logic in textStartAfterOSC (and adjust related handling in parseOSC if needed)
so that when end points to BEL ('\x07') or the ST backslash ('\\') you return
end+1, and only return end+2 for the legacy two-rune sequence case; reference
the textStartAfterOSC function (and parseOSC which returns the trailing '\'
index) to locate and correct the ST handling so ST-terminated OSC links start at
the correct rune.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8aef247a-c139-4dfd-b7aa-7b67ca60f533
⛔ Files ignored due to path filters (1)
client/go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
CLAUDE.mdclient/cmd/demarkus-tui/graphview.goclient/cmd/demarkus-tui/main.goclient/go.mod
ontehfritz
deleted the
85-bugtui-with-the-upgrade-of-glamour-links-broke
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation