Repository navigation
Selected words can be made into a link, and stay words - #38
Merged
Merged
Conversation
Select some text while editing, press "Link" on the bar above it, and give an address: the words point there. A label and an address are two things and the field they live in is one string, so the string holds `[our roadmap](https://…)` — markdown's spelling, because it is the one people already know and already type. The store is unchanged: still a plain string, still no markup, still a recogniser on the way out. `core/links.js` knows one more shape of link. **That costs the one rule everything here has had for free: what is on screen is what is in the document.** Every edit reads a field back through `innerText`, and `innerText` over a labelled link is its label — so a single keystroke anywhere in a card would write the labels down and throw away every address in it. Focusing a field therefore puts its own characters back in it, and leaving it draws the links again. Editing shows the source, reading shows the words, and the round trip is exact at every moment in between. The caret is placed *before* the focus lands, for the same reason: revealing a source moves every character after it along the line, so a position taken afterwards is a position in a line nobody saw — which is how a double-click after a link ends up putting the caret inside its address. **Where the selection is in the string is measured with the browser's own answer.** `platform/text-link.js` puts a marker at each end of the selection, reads `innerText` once with them in and takes them out again. The alternative is a DOM walk that re-implements `innerText` — a second opinion about the same string, which would disagree with the first one the day somebody pressed Enter. A test drives exactly that: a second line made with the Enter key, living in a `<div>` the store knows nothing about. What comes back is two offsets into the string the store holds, so the change itself is arithmetic in `core/` applied as one recorded `set` — undo, autosave and the other people on the board all follow from that and none of them had to be arranged. An answer that arrives for a string the field no longer holds is dropped rather than cutting somebody else's sentence in half. The control is on the format bar rather than in a bar of its own, since that bar is already what appears above the thing you are working on. It is not offered for a selection that is already a link — the answer to "make this a link" is that it is one — nor for one holding a `]`, which is where a label ends and therefore the one character a label cannot hold. The address is asked for in the page's own dialog, where a bare `example.com` counts and in prose deliberately does not; one that cannot be opened is asked again, keeping what was typed. Two things found by building it. A double-click inside the field being edited was collapsing the browser's own word selection, so there was no way to select a word on a card at all — that one is left to the caret now. And the PNG export drew the brackets, because it wraps the raw string; it goes through `displayText`, so a picture of a board says what the board says. 915 tests, 96.6% lines. `platform/text-link.js` at 100%. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PTizw7U35rC2MPquwr4Fv5
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughThe application now supports labelled Markdown links. It parses and renders labels, restores source text during editing, creates links from selections through the format bar, validates destinations, preserves caret offsets, updates the store, and exports display text. ChangesLabelled link workflow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Editor
participant FormatBar
participant createTextLinks
participant useAsk
participant store
Editor->>FormatBar: expose eligible text selection
FormatBar->>createTextLinks: capture selection
FormatBar->>useAsk: prompt for link address
useAsk-->>FormatBar: return typed address
FormatBar->>createTextLinks: insert validated href
createTextLinks->>store: apply one undoable set update
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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: 1
🧹 Nitpick comments (2)
src/core/links.js (1)
204-205: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueConsider encoding an unbalanced
)in the href.
linkSourcewrites the href verbatim.LABELLEDaccepts only one level of balanced parentheses in the address, so an href holding an unbalanced)— for examplehttps://a.test/x)— reparses to a shorter address and leaves a stray)as prose.linkFromcan return such an href becauseURLkeeps)in a path.Encoding just the closing parenthesis keeps the round trip exact and stays readable.
♻️ Proposed change
-const linkSource = (label, href) => `[${label}](${href})`; +const linkSource = (label, href) => `[${label}](${href.replace(/\)/g, '%29')})`;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/links.js` around lines 204 - 205, Update linkSource to encode closing parentheses in href values before interpolating them into the Markdown link, while preserving other URL characters and readable output. Ensure hrefs such as URLs with an unbalanced “)” round-trip through LABELLED without leaving trailing prose; linkFrom and label handling should remain unchanged.src/platform/text-link.js (1)
72-101: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winRemove the marks in a
finallyblock.
measureinserts two private-use characters into the live field. IfinsertNodeor theinnerTextread throws, the marks stay in the DOM. The nextinputevent reads the field throughinnerText, so a\uE000would be written into the store and broadcast to other editors.A
finallyblock makes the cleanup unconditional.♻️ Proposed change
const measure = (field, range) => { const text = field.innerText; if (text.includes(MARK)) return null; const head = document.createTextNode(MARK); const tail = document.createTextNode(MARK); - const to = range.cloneRange(); - to.collapse(false); - to.insertNode(tail); - - const from = range.cloneRange(); - from.collapse(true); - from.insertNode(head); - - const marked = field.innerText; - - head.remove(); - tail.remove(); - // The marks left split text nodes either side of where they were. - field.normalize(); + let marked; + try { + const to = range.cloneRange(); + to.collapse(false); + to.insertNode(tail); + + const from = range.cloneRange(); + from.collapse(true); + from.insertNode(head); + + marked = field.innerText; + } finally { + head.remove(); + tail.remove(); + // The marks left split text nodes either side of where they were. + field.normalize(); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/platform/text-link.js` around lines 72 - 101, Update measure so removal of the head and tail marker nodes, followed by field.normalize(), occurs in a finally block surrounding the range insertion and marked innerText read. Preserve the existing validation and return logic while ensuring cleanup runs if either insertNode or innerText throws.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/FormatBar.jsx`:
- Around line 183-187: Handle the boolean result from chosen.insert(href) in the
addLink flow: when insertion returns false, keep the dialog open and surface an
appropriate failure message instead of silently returning; only proceed with
closing or success behavior when insertion succeeds, consistent with the
existing commands.exportPng outcome handling.
---
Nitpick comments:
In `@src/core/links.js`:
- Around line 204-205: Update linkSource to encode closing parentheses in href
values before interpolating them into the Markdown link, while preserving other
URL characters and readable output. Ensure hrefs such as URLs with an unbalanced
“)” round-trip through LABELLED without leaving trailing prose; linkFrom and
label handling should remain unchanged.
In `@src/platform/text-link.js`:
- Around line 72-101: Update measure so removal of the head and tail marker
nodes, followed by field.normalize(), occurs in a finally block surrounding the
range insertion and marked innerText read. Preserve the existing validation and
return logic while ensuring cleanup runs if either insertNode or innerText
throws.
🪄 Autofix
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: CHILL
Plan: Pro Plus
Run ID: fe2f11ef-c1e8-432a-a18e-2a5a178bf073
📒 Files selected for processing (11)
README.mdsrc/app.jssrc/components/FormatBar.jsxsrc/core/links.jssrc/platform/export-png.jssrc/platform/input.jssrc/platform/text-link.jssrc/platform/views.jssrc/styles/canvas.csstest/browser/text-link.test.jstest/node/links.test.js
… bracket - **An insertion that came to nothing said nothing.** `insert` answers false when the field no longer holds the string the words were measured in — another editor's change landing while the question is open — and the answer was dropped on the floor, so somebody typed an address and watched the dialog close. Asking again would be no use, since those offsets are gone whatever is typed next, so it goes out as a problem code and the shell writes the sentence, exactly as a refused image paste does. - **The marks come out in a `finally`.** They are two private-use characters in a live field, and the next thing typed reads that field back through `innerText` — so a mark left behind by a throw would have been written into the store and broadcast to everyone else on the board. `remove()` on a node that was never inserted does nothing, which covers the case where the first `insertNode` is what threw. - **An address whose brackets cannot be read back has them encoded.** The recogniser follows one level of balanced brackets, which is what keeps `Ruby_(gem)` legible — but `https://a.test/p)` was being written down as `https://a.test/p` with a stray bracket left as prose, which is a link pointing somewhere nobody chose. `linkSource` asks the recogniser now and percent-encodes only what it cannot read back, so the readable case stays readable and the other one stays right. 918 tests, 96.6% lines. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PTizw7U35rC2MPquwr4Fv5
Owner
Author
|
Both nitpicks are in too, in the same commit. The marks come out in a The bracket one is real and got a slightly different fix. Encoding |
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.
Select some text while editing — double-click a word, or drag across it — and a Link control appears on the bar above. Press it, give an address, and the words point there.
Where a labelled link lives
A label and an address are two things, and the field they live in is one string — so the string holds
[our roadmap](https://…), in markdown's spelling because that is the one people already know and already type. The store is unchanged: still a plain string, still no markup, still a recogniser on the way out.core/links.jsknows one more shape of link.The one rule it costs
What is on screen is no longer what is in the document. Every edit reads a field back through
innerText, andinnerTextover a labelled link is its label — so one keystroke anywhere in a card would write the labels down and throw away every address in it.So focusing a field puts its own characters back in it, and leaving it draws the links again. Editing shows the source, reading shows the words, and the round trip is exact at every moment in between.
The caret is placed before the focus lands, for the same reason: revealing a source moves every character after it along the line, so a position taken afterwards is a position in a line nobody saw — which is how a double-click after a link ends up putting the caret inside its address.
Measuring the selection
platform/text-link.jsputs a marker at each end of the selection, readsinnerTextonce with them in, and takes them out again. The alternative is a DOM walk that re-implementsinnerText— a second opinion about the same string, which would disagree with the first one the day somebody pressed Enter. A test drives exactly that: a second line made with the Enter key, living in a<div>the store knows nothing about.What comes back is two offsets into the string the store holds, so the change itself is arithmetic in
core/applied as one recordedset— undo, autosave and the other people on the board all follow from that and none of them had to be arranged. An answer arriving for a string the field no longer holds is dropped rather than cutting somebody else's sentence in half.The control
On the format bar rather than in a bar of its own, since that bar is already what appears above the thing you are working on. Not offered for a selection that is already a link — the answer to "make this a link" is that it is one — nor for one holding a
], which is where a label ends and so the one character a label cannot hold. The address is asked for in the page's own dialog, where a bareexample.comcounts and in prose deliberately does not; one that cannot be opened is asked again, keeping what was typed.Found while building it
displayText, so a picture of a board says what the board says.Not in this change
⌘K. Keys pressed inside a field are that field's, deliberately — it is what lets a board be named "My board" — so a shortcut means a listener on the editing layer and a route from there to the question, which is the format bar's to ask. Worth doing; written down as a limit.]is not offered a link, since those words cannot be written down as a label at all. Selecting round it works.Checks
915 tests, all passing, 96.6% line coverage.
src/platform/text-link.jsat 100%. 18 new browser tests over the real gestures, plus core unit tests for the recogniser, the address parsing and the string edit.🤖 Generated with Claude Code
https://claude.ai/code/session_01PTizw7U35rC2MPquwr4Fv5
Summary by CodeRabbit