Conversation
Convert markdown task list items (- [ ] / - [x]) into interactive checkboxes in the plan viewer. Clicking a checkbox applies a replace_exact operation on the server, creating a new PlanVersion attributed to the user. - Markdown helper post-processes Commonmarker HTML to make checkboxes interactive with Stimulus data attributes - Stimulus checkbox_controller handles optimistic toggle with in-flight guard, sends PATCH, retries once on 409 conflict - PlansController#toggle_checkbox applies replace_exact with optimistic concurrency (base_revision), broadcasts header update - CSS for task lists: no bullets, muted color for checked items - Supports all list markers (-, *, +) and case-insensitive [x]/[X] - Helper, request, and system specs (13 new tests) Amp-Thread-ID: https://ampcode.com/threads/T-019d7340-20dd-74b5-bcc4-d428c23883cc Co-authored-by: Amp <amp@ampcode.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: d61a7ccbb3
ℹ️ 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".
| result = Plans::ApplyOperations.call( | ||
| content: current_content, | ||
| operations: [{ "op" => "replace_exact", "old_text" => old_text, "new_text" => new_text }] | ||
| ) |
There was a problem hiding this comment.
toggle_checkbox applies client-supplied old_text/new_text directly as a replace_exact operation, so any user who can hit this endpoint can replace arbitrary unique text in the plan by crafting the JSON payload instead of only flipping [ ]/[x] on a task item. This effectively bypasses the normal update? guard for non-checkbox content and turns a checkbox toggle API into a general edit primitive.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 26dd248 — added server-side validation that both old_text and new_text match the task list pattern (/\A\s*[*+-]\s+\[[ xX]\]\s/). Arbitrary text replacements now get a 422.
| task_lines = content.to_s.lines.map(&:rstrip).select { |l| l.match?(/^\s*[*+-]\s+\[[ xX]\]\s/) } | ||
|
|
||
| checkboxes.each_with_index do |cb, i| | ||
| line_text = task_lines[i] |
There was a problem hiding this comment.
The helper pairs rendered checkboxes to source lines purely by index (task_lines[i]), but task_lines is built from raw markdown text and includes task-like lines that do not produce checkbox nodes (for example, inside fenced code blocks). In those documents the indices drift, so clicking a visible checkbox can send old_text for a different line and mutate the wrong content.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 26dd248 — extracted extract_task_lines method that tracks fenced code blocks (```/~~~) and skips task-like lines inside them. Added a test confirming checkboxes inside code fences don't misalign the mapping.
| cb.remove_attribute("disabled") | ||
| cb["data-action"] = "coplan--checkbox#toggle" | ||
| cb["data-coplan--checkbox-target"] = "checkbox" | ||
| cb["data-line-text"] = line_text.lstrip |
There was a problem hiding this comment.
Using lstrip on the captured task line drops leading indentation, so nested task items (e.g., - [ ] subtask) are posted back as - [ ] subtask. The server then cannot find the exact text to replace and returns 422, which makes nested checklist items non-toggleable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 26dd248 — removed lstrip, so data-line-text now preserves leading whitespace (e.g. - [ ] subtask). Added a test for nested task items confirming indentation is retained.
…ve indentation - Validate old_text/new_text match task list pattern to prevent using toggle_checkbox as a general edit endpoint - Extract task lines skipping fenced code blocks to prevent checkbox-to-line misalignment - Remove lstrip to preserve indentation for nested task items - Add tests for all three fixes Amp-Thread-ID: https://ampcode.com/threads/T-019d7340-20dd-74b5-bcc4-d428c23883cc Co-authored-by: Amp <amp@ampcode.com>
- Wrap task list item contents in <label> so clicking anywhere on the text toggles the checkbox (not just the tiny input) - Remove task-list-item--checked class and all checked styling — the checkbox state itself is the only visual indicator - Add system test for label-click toggling Amp-Thread-ID: https://ampcode.com/threads/T-019d7340-20dd-74b5-bcc4-d428c23883cc Co-authored-by: Amp <amp@ampcode.com>
1 participant
Summary
Convert markdown task list items (
- [ ]/- [x]) into interactive checkboxes in the plan viewer. Clicking a checkbox toggles it on the server, creating a new PlanVersion with full provenance.How it works
MarkdownHelper#make_checkboxes_interactivepost-processes Commonmarker HTML: removesdisabled, adds Stimulus bindings, maps each checkbox to its source markdown line viadata-line-textcheckbox_controllersendsPATCH /plans/:id/toggle_checkboxwithold_text,new_text, andbase_revisionfor optimistic concurrencyPlansController#toggle_checkboxapplies areplace_exactoperation, creates a versionedPlanVersionattributed to the clicking user, broadcasts header updateDetails
-,*,+) and case-insensitive[x]/[X]Tests