Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion engine/app/controllers/coplan/plans_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,17 @@ def toggle_checkbox
return
end

# Optional source line number scoping the replacement to a single-line
# box, so identical task lines elsewhere in the document can't collide.
line = nil
if params[:line].present?
line = Integer(params[:line], exception: false)
if line.nil? || line < 1
render json: { error: "line must be a positive integer" }, status: :unprocessable_content
return
end
end

ActiveRecord::Base.transaction do
@plan.lock!
@plan.reload
Expand All @@ -148,9 +159,11 @@ def toggle_checkbox
end

current_content = @plan.current_content || ""
operation = { "op" => "replace_exact", "old_text" => old_text, "new_text" => new_text }
operation["lines"] = line if line
result = Plans::ApplyOperations.call(
content: current_content,
operations: [{ "op" => "replace_exact", "old_text" => old_text, "new_text" => new_text }]
operations: [operation]
)

new_revision = @plan.current_revision + 1
Expand Down
12 changes: 8 additions & 4 deletions engine/app/helpers/coplan/markdown_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ module MarkdownHelper
details summary
].freeze

ALLOWED_ATTRIBUTES = %w[id class href src alt title type checked disabled data-line-text data-action data-coplan--checkbox-target data-mention-username].freeze
ALLOWED_ATTRIBUTES = %w[id class href src alt title type checked disabled data-line data-line-text data-action data-coplan--checkbox-target data-mention-username].freeze

# Matches `[@username](mention:username)` where the bracket text and link
# target encode the same username. Username allows letters, digits, dots,
Expand Down Expand Up @@ -78,13 +78,14 @@ def make_checkboxes_interactive(html, content)
task_lines = extract_task_lines(content)

checkboxes.each_with_index do |cb, i|
line_text = task_lines[i]
line_number, line_text = task_lines[i]
next unless line_text

cb.remove_attribute("disabled")
cb["data-action"] = "coplan--checkbox#toggle"
cb["data-coplan--checkbox-target"] = "checkbox"
cb["data-line-text"] = line_text
cb["data-line"] = line_number.to_s

li = cb.parent
next unless li&.name == "li"
Expand All @@ -102,17 +103,20 @@ def make_checkboxes_interactive(html, content)
doc.to_html
end

# Returns [1-based line number, rstripped line text] pairs for each task
# line, in document order. Fenced lines are counted (so numbering stays
# accurate) but never treated as task lines.
def extract_task_lines(content)
lines = []
in_fence = false
content.to_s.each_line do |line|
content.to_s.each_line.with_index(1) do |line, line_number|
stripped = line.rstrip
if stripped.match?(/\A(`{3,}|~{3,})/)
in_fence = !in_fence
next
end
next if in_fence
lines << stripped if stripped.match?(/^\s*[*+-]\s+\[[ xX]\]\s/)
lines << [line_number, stripped] if stripped.match?(/^\s*[*+-]\s+\[[ xX]\]\s/)
end
lines
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,17 @@ export default class extends Controller {
? lineText.replace(/([*+-]\s+)\[[ ]\]/, "$1[x]")
: lineText.replace(/([*+-]\s+)\[[xX]\]/, "$1[ ]")

// Source line number disambiguates duplicate task-line text server-side.
const line = parseInt(checkbox.dataset.line, 10)

// Optimistic UI: update immediately
checkbox.dataset.lineText = newText

this.inflight = true
this.#sendToggle({ checkbox, oldText, newText, nowChecked, retried: false })
this.#sendToggle({ checkbox, oldText, newText, line, nowChecked, retried: false })
}

#sendToggle({ checkbox, oldText, newText, nowChecked, retried }) {
#sendToggle({ checkbox, oldText, newText, line, nowChecked, retried }) {
const token = document.querySelector('meta[name="csrf-token"]')?.content

fetch(this.toggleUrlValue, {
Expand All @@ -36,7 +39,8 @@ export default class extends Controller {
body: JSON.stringify({
old_text: oldText,
new_text: newText,
base_revision: this.revisionValue
base_revision: this.revisionValue,
...(Number.isInteger(line) && line >= 1 ? { line } : {})
})
}).then(response => {
if (response.ok) {
Expand All @@ -50,7 +54,7 @@ export default class extends Controller {
if (data.current_revision) {
this.revisionValue = data.current_revision
}
this.#sendToggle({ checkbox, oldText, newText, nowChecked, retried: true })
this.#sendToggle({ checkbox, oldText, newText, line, nowChecked, retried: true })
})
} else {
this.#revert(checkbox, oldText, nowChecked)
Expand Down
2 changes: 2 additions & 0 deletions engine/app/services/coplan/plans/commit_session.rb
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,8 @@ def call
rebased_ops = []
@session.operations_json.each do |op_data|
op_data = op_data.transform_keys(&:to_s)
# "lines" is intentionally absent: this fallback re-resolves against
# *current* content, where a base-revision line box would be wrong.
semantic_keys = %w[op old_text new_text heading content needle occurrence replace_all count new_content include_heading]
semantic_op = op_data.slice(*semantic_keys)

Expand Down
56 changes: 56 additions & 0 deletions engine/app/services/coplan/plans/position_resolver.rb
Original file line number Diff line number Diff line change
Expand Up @@ -38,12 +38,25 @@ def resolve_replace_exact
occurrence = (@op["occurrence"] || 1).to_i
raise OperationError, "replace_exact: occurrence must be >= 1, got #{occurrence}" if occurrence < 1

line_box = parse_line_box

ranges = find_all_occurrences(old_text)

if ranges.empty?
raise OperationError, "replace_exact found 0 occurrences of the specified text"
end

if line_box
box = char_range_for_lines(*line_box)
total = ranges.length
# Only occurrences fully contained in the box survive; the
# occurrence/replace_all/count rules below then index within it.
ranges = ranges.select { |s, e| s >= box[0] && e <= box[1] }
if ranges.empty?
raise OperationError, "replace_exact found #{total} occurrence(s) of the text, but none within lines #{line_box[0]}..#{line_box[1]}"
end
end

if replace_all
Resolution.new(op: "replace_exact", ranges: ranges)
else
Expand Down Expand Up @@ -108,6 +121,49 @@ def resolve_delete_paragraph_containing
Resolution.new(op: "delete_paragraph_containing", ranges: ranges)
end

# Optional "lines" qualifier: a 1-based inclusive line range ("boxed
# context") that scopes replace_exact matching. Accepts an Integer
# (single-line box) or a two-element [start, end] pair. Multi-line
# old_text is fine as long as the match fits inside the box.
def parse_line_box
raw = @op["lines"]
return nil if raw.nil?

bounds = raw.is_a?(Array) ? raw : [raw, raw]
unless bounds.length == 2 && bounds.all?(Integer)
raise OperationError, "replace_exact: 'lines' must be an integer line number or a [start, end] pair of integers"
end

start_line, end_line = bounds
if start_line < 1 || end_line < start_line
raise OperationError, "replace_exact: 'lines' must satisfy 1 <= start <= end, got #{start_line}..#{end_line}"
end

[start_line, end_line]
end

# Character range [start, end) covering the given 1-based inclusive
# line range, including the end line's terminator so a match may end
# at the end of that line.
def char_range_for_lines(start_line, end_line)
total = 0
box_start = nil
box_end = nil
pos = 0
@content.each_line do |line|
total += 1
box_start = pos if total == start_line
pos += line.length
box_end = pos if total == end_line
end

if box_start.nil? || box_end.nil?
raise OperationError, "replace_exact: lines #{start_line}..#{end_line} out of range (document has #{total} lines)"
end

[box_start, box_end]
end

def find_all_occurrences(text)
ranges = []
start_pos = 0
Expand Down
16 changes: 16 additions & 0 deletions engine/app/views/coplan/agent_instructions/show.text.erb
Original file line number Diff line number Diff line change
Expand Up @@ -290,6 +290,22 @@ Find and replace exact text. Fails if text not found or count exceeded.
}
```

Optional `lines` qualifier: a 1-based inclusive line range scoping the match — an
integer (single line) or a `[start, end]` pair. Use it to disambiguate text that
appears more than once (only occurrences fully inside the range are considered;
`occurrence`/`replace_all`/`count` then apply within it). Multi-line `old_text`
is allowed as long as the match fits inside the range. Fails if the text isn't
found within the range.

```json
{
"op": "replace_exact",
"old_text": "- [ ] Write tests",
"new_text": "- [x] Write tests",
"lines": 12
}
```

### insert_under_heading

Insert content after a markdown heading. Fails if heading not found or ambiguous.
Expand Down
36 changes: 36 additions & 0 deletions spec/helpers/coplan/markdown_helper_checkbox_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -96,4 +96,40 @@
expect(nested["data-line-text"]).to eq(" - [ ] Nested child")
end
end

describe "#render_markdown data-line attribute" do
it "sets data-line to the 1-based source line number" do
md = "# Heading\n\n- [ ] First\n- [x] Second"
html = helper.render_markdown(md)
doc = Nokogiri::HTML::DocumentFragment.parse(html)
checkboxes = doc.css('input[type="checkbox"]')
expect(checkboxes.map { |cb| cb["data-line"] }).to eq(%w[3 4])
end

it "keeps line numbers accurate across fenced code blocks" do
md = "```\n- [ ] Fake checkbox\n```\n- [ ] Real checkbox"
html = helper.render_markdown(md)
doc = Nokogiri::HTML::DocumentFragment.parse(html)
checkboxes = doc.css('input[type="checkbox"]')
expect(checkboxes.length).to eq(1)
expect(checkboxes[0]["data-line"]).to eq("4")
end

it "keeps line numbers accurate across multiple lists and duplicates" do
md = "- [ ] TODO\n\nSome text\n\n- [ ] TODO\n- [ ] Other"
html = helper.render_markdown(md)
doc = Nokogiri::HTML::DocumentFragment.parse(html)
checkboxes = doc.css('input[type="checkbox"]')
expect(checkboxes.map { |cb| cb["data-line"] }).to eq(%w[1 5 6])
expect(checkboxes[0]["data-line-text"]).to eq(checkboxes[1]["data-line-text"])
end

it "numbers nested tasks by their own source lines" do
md = "- [ ] Parent\n - [ ] Nested child"
html = helper.render_markdown(md)
doc = Nokogiri::HTML::DocumentFragment.parse(html)
nested = doc.css('input[type="checkbox"]').find { |cb| cb["data-line-text"]&.include?("Nested") }
expect(nested["data-line"]).to eq("2")
end
end
end
83 changes: 83 additions & 0 deletions spec/requests/toggle_checkbox_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -111,4 +111,87 @@
expect(response).to have_http_status(:ok)
end
end

describe "PATCH toggle_checkbox with duplicate task lines" do
before do
plan.current_plan_version.update!(
content_markdown: "# Tasks\n\n- [ ] TODO\n- [ ] TODO\n- [ ] Deploy to staging\n- [ ] Deploy"
)
end

it "toggles the checkbox on the given line, leaving its duplicate untouched" do
patch toggle_checkbox_plan_path(plan), params: {
old_text: "- [ ] TODO",
new_text: "- [x] TODO",
base_revision: plan.current_revision,
line: 4
}, as: :json

expect(response).to have_http_status(:ok)
plan.reload
expect(plan.current_content).to eq("# Tasks\n\n- [ ] TODO\n- [x] TODO\n- [ ] Deploy to staging\n- [ ] Deploy")
end

it "toggles a line whose text is a prefix of another task line" do
patch toggle_checkbox_plan_path(plan), params: {
old_text: "- [ ] Deploy",
new_text: "- [x] Deploy",
base_revision: plan.current_revision,
line: 6
}, as: :json

expect(response).to have_http_status(:ok)
plan.reload
expect(plan.current_content).to eq("# Tasks\n\n- [ ] TODO\n- [ ] TODO\n- [ ] Deploy to staging\n- [x] Deploy")
end

it "returns 422 when the text is not on the given line" do
patch toggle_checkbox_plan_path(plan), params: {
old_text: "- [ ] TODO",
new_text: "- [x] TODO",
base_revision: plan.current_revision,
line: 5
}, as: :json

expect(response).to have_http_status(:unprocessable_content)
plan.reload
expect(plan.current_content).not_to include("- [x] TODO")
end

it "returns 422 without a line param when duplicates exist (regression guard)" do
patch toggle_checkbox_plan_path(plan), params: {
old_text: "- [ ] TODO",
new_text: "- [x] TODO",
base_revision: plan.current_revision
}, as: :json

expect(response).to have_http_status(:unprocessable_content)
end

it "returns 422 for a non-integer line param" do
patch toggle_checkbox_plan_path(plan), params: {
old_text: "- [ ] TODO",
new_text: "- [x] TODO",
base_revision: plan.current_revision,
line: "abc"
}, as: :json

expect(response).to have_http_status(:unprocessable_content)
expect(response.parsed_body["error"]).to include("line must be a positive integer")
end

it "records the lines qualifier in the version's operations_json" do
patch toggle_checkbox_plan_path(plan), params: {
old_text: "- [ ] TODO",
new_text: "- [x] TODO",
base_revision: plan.current_revision,
line: 3
}, as: :json

expect(response).to have_http_status(:ok)
op = plan.reload.current_plan_version.operations_json.first
expect(op["lines"]).to eq(3)
expect(op["resolved_range"]).to be_present
end
end
end
24 changes: 24 additions & 0 deletions spec/services/plans/apply_operations_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,30 @@
)
}.to raise_error(CoPlan::Plans::OperationError, /requires 'old_text'/)
end

it "scopes replacement with a lines qualifier and records it in applied data" do
content = "- [ ] TODO\n- [ ] TODO\n- [ ] Other"
result = CoPlan::Plans::ApplyOperations.call(
content: content,
operations: [{ "op" => "replace_exact", "old_text" => "- [ ] TODO", "new_text" => "- [x] TODO", "lines" => 2 }]
)
expect(result[:content]).to eq("- [ ] TODO\n- [x] TODO\n- [ ] Other")

applied = result[:applied].first
expect(applied["lines"]).to eq(2)
expect(applied["resolved_range"]).to eq([11, 21])
expect(applied["new_range"]).to eq([11, 21])
expect(applied["delta"]).to eq(0)
end

it "replaces multi-line old_text within a [start, end] lines box" do
content = "intro\nalpha\nbeta\nalpha\nbeta"
result = CoPlan::Plans::ApplyOperations.call(
content: content,
operations: [{ "op" => "replace_exact", "old_text" => "alpha\nbeta", "new_text" => "gamma", "lines" => [4, 5] }]
)
expect(result[:content]).to eq("intro\nalpha\nbeta\ngamma")
end
end

describe "insert_under_heading" do
Expand Down
Loading
Loading