Refuse comments whose anchor doesn't resolve — no pin, no comment - #173
Conversation
A comment thread whose anchor never resolved to source positions renders nowhere: no highlight, no popover, no way to reach it from the page. Yet create happily persisted it and told the user "Comment added." — a comment posted into the void. CommentThread now validates on create that a present anchor resolved. Resolution moves from before_create to before_validation (on: :create) so validation can see its result. Both stay create-only: a resolved thread whose content later drifts is the out_of_date flow, not a validity problem. On refusal, the selection form keeps the draft and shows the error inline (new #new-comment-form-error region, updated via turbo stream with a 422); a plain HTML post redirects with an alert. The JSON API already returned 422 for RecordInvalid, so agents were covered. The rule also surfaced a resolver gap: mermaid labels line-break on literal <br/> tags, and the browser reads the label back without them — "first<br/>fetching" is selected as "firstfetching", which never matched the source and would now be refused despite being a real, visible pin. The resolver gains a pass that drops the tags from the stripped text, carrying the position map along, so these anchors resolve (and now survive edits via OT instead of going out_of_date unconditionally). Specs that relied on silently-unresolved anchors are the proof of the hole: the factory's :with_anchor trait and several request specs anchored to text their plans never contained. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3b546d4a92
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if stripped_ranges.empty? | ||
| stripped, pos_map = remove_break_tags(stripped, pos_map) | ||
| stripped_ranges = find_all_occurrences(stripped, normalized_anchor) |
There was a problem hiding this comment.
When the selected <br>-split Mermaid label has the same visible text as ordinary prose earlier in the plan, the browser submits its DOM-wide occurrence (for example, 2), but the raw search finds only the prose occurrence. Because the new break-tag fallback runs only when ranges is empty, it is skipped, anchor_start remains unset, and this valid comment is rejected as unresolved. Build the occurrence list from a single rendered-text representation so both forms participate in occurrence selection.
AGENTS.md reference: AGENTS.md:L129-L129
Useful? React with 👍 / 👎.
| # Removes <br>/<br/> tags from stripped text, carrying the position | ||
| # map along so matches still resolve to raw source positions. | ||
| def remove_break_tags(text, pos_map) |
There was a problem hiding this comment.
When two Mermaid labels contain the same anchor split by <br/>, this resolver can persist the second label correctly, but anchor_occurrence_index still searches the original stripped text where firstfetching never occurs and falls back to occurrence 0. The frontend then highlights and opens the first matching label instead of the selected second one; apply the same break-tag normalization when calculating the occurrence returned to the view.
AGENTS.md reference: AGENTS.md:L139-L139
Useful? React with 👍 / 👎.
1 participant
The hole
A comment thread whose anchor never resolved to source positions renders nowhere — no highlight, no popover, no path to it from the page. Yet
createhappily persisted it and told the user "Comment added." The comment went into the void, and nothing anywhere said so.This is easy to hit: any anchor captured from rendered text that the resolver can't map back to the markdown source (or an
anchor_occurrencepast the ones that exist) produced an invisible thread.The rule
No pin, no comment.
CommentThreadnow validates on create that a present anchor actually resolved:before_createtobefore_validation, on: :createso validation can see its result.out_of_dateflow, not a validity problem — old threads stay updatable.On refusal:
#new-comment-form-errorregion updated via turbo stream with a 422; the form's reset already checksevent.detail.success, so nothing is lost).RecordInvalid, so programmatic clients were covered.The resolver gap the rule surfaced
Mermaid labels line-break on literal
<br/>tags, and the browser reads the rendered label back without them —first<br/>fetchinggets selected asfirstfetching. That never matched the source, so these were exactly the invisible-position threads described above (client-side highlighting made them look pinned until the next edit unconditionally expired them). The resolver gains a pass that drops the tags from the stripped text, carrying the position map along — these anchors now resolve to real positions and survive edits through OT like everything else.Tests
The specs that had to change are the proof of the hole: the factory's
:with_anchortrait and several request specs anchored to text their plans never contained, and nothing ever noticed. New coverage for the validation (model + both response shapes), the occurrence-out-of-range case, the update-doesn't-re-litigate case, and the<br/>resolution.Full suite: 1334 examples, 0 failures.
🤖 Generated with Claude Code