Fix hyperlinks being split into multiple links when text is highlighted#4110
Open
filbranden wants to merge 4 commits intoTextualize:masterfrom
Open
Fix hyperlinks being split into multiple links when text is highlighted#4110filbranden wants to merge 4 commits intoTextualize:masterfrom
filbranden wants to merge 4 commits intoTextualize:masterfrom
Conversation
…k styles Two Style(link=...) objects with the same URL are equal by hash and comparison, but carry different _link_id values. Combining each with a base style must yield combined styles with distinct _link_ids; the lru_cache must not cause the second result to inherit the first's _link_id.
Style.__add__ was calling copy() on every combined style that contained a link, which generated a fresh link_id for each highlighted sub-span within a single link markup region. Terminals use link_id to identify hyperlink boundaries, so each sub-span appeared as a separate clickable link instead of one continuous one. The underlying root cause is that _add's lru_cache keys on __hash__/__eq__, which intentionally excludes link_id. Two Style objects with the same visual content but different link_ids therefore collide in the cache; copy() was a blunt workaround that overcorrected by regenerating the id on every hit. Fix this by having _add take the link_id as an argument so that the lru_cache will now also key by link_id, preventing the issue that required the copy() workaround altogether.
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
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.
Type of changes
AI?
I used Claude Sonnet 4.6 to work on this fix, it was fully reviewed by a human though. The process went roughly as follows:
copy()call, I told it there was probably a reason why it was there, so I asked it to figure that out and add a test that would ensure that behavior was preserved.I understand there wasn't discussion in the issue itself, I hope this PR with the code might serve as a starting point for discussion with a proposed code change that I'm willing to iterate on. Thanks!
Checklist
Description
Style.__add__was callingcopy()on every combined style that contained alink, which generated a fresh link_id for each highlighted sub-span within
a single link markup region. Terminals use link_id to identify hyperlink
boundaries, so each sub-span appeared as a separate clickable link instead
of one continuous one.
The underlying root cause is that
_add's lru_cache keys on__hash__/__eq__,which intentionally excludes link_id. Two Style objects with the same visual
content but different link_ids therefore collide in the cache; copy() was
a blunt workaround that overcorrected by regenerating the id on every hit.
Fix this by having
_addtake the link_id as an argument so that the lru_cachewill now also key by link_id, preventing the issue that required the
copy()workaround altogether.
Add test cases to confirm the issue is fixed and include regression tests to prevent it from getting broken again in the future.
Fixes #4109.