fix(pm): serialize inline checklist description updates#1339
Conversation
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
Requesting changes for two inline checklist detail-line regressions introduced by the broader section scanning.
Code Issues
Should Fix
- src/pm/_shared/inline-checklist.ts:203 -
addItemToChecklist()still anchors insertion to the last checkbox line. With### AC\n- [ ] First\n Detail line, addingSecondreturns### AC\n- [ ] First\n- [ ] Second\n Detail line, moving the existing detail under the new item. Since detail/prose lines are now intentionally part of the checklist section, adding should preserve trailing detail with the item it already describes. - src/pm/_shared/inline-checklist.ts:314 -
scanSection()now treats non-checkbox lines as section content until the next heading, but last-item deletion still removes only through the checkbox line. Deleting the only item in### AC\n- [ ] Only\nDetail line\n\n### Next...leavesDetail lineorphaned before the next section.
I verified both scenarios locally with direct npx tsx imports of the changed helper.
🕵️ codex · gpt-5.5 · run details
| if (CHECKBOX_REGEX.test(lines[i])) { | ||
| insertIdx = i; | ||
| } else if (lines[i].trim() !== '') { | ||
| } else if (HEADING_REGEX.test(lines[i])) { |
There was a problem hiding this comment.
This still anchors insertion to the last checkbox only. After this parser change, trailing detail lines belong to the current section/item; adding to ### AC\n- [ ] First\n Detail produces the new checkbox before Detail, which reattaches the detail to the new item. Track the insertion point after the trailing detail block, or at the next heading, so add preserves existing item detail.
There was a problem hiding this comment.
Fixed. now advances for non-empty, non-heading lines within the section, not just checkbox lines. So when the loop finishes, insertIdx points to the last non-empty content line (whether that's a checkbox or a trailing detail line), and the new item is spliced in after it.
Added a targeted regression test:
it('adds after trailing detail lines that follow the last checkbox', () => {
const desc = '### AC\n- [ ] First\n Detail line';
const result = addItemToChecklist(desc, 'AC', 'Second');
expect(result).toBe('### AC\n- [ ] First\n Detail line\n- [ ] Second');
});| continue; | ||
| } | ||
| if (!inSection) continue; | ||
| if (HEADING_REGEX.test(lines[i])) break; |
There was a problem hiding this comment.
scanSection now includes non-checkbox detail lines until the next heading, but the last-item path still calls removeSectionBlock(..., targetLineIdx). Deleting the only item in a section like ### AC\n- [ ] Only\nDetail line\n\n### Next leaves Detail line orphaned. Return the section end/last content index and remove the whole section block when itemCount === 1.
There was a problem hiding this comment.
Fixed. scanSection now tracks lastContentIdx — the index of the last non-empty line in the section (which may be a detail/prose line after the final checkbox). The SectionScan interface gains this field. For the single-item deletion path, removeChecklistItem now passes lastContentIdx (not targetLineIdx) to removeSectionBlock, so the entire section block including trailing detail is removed in one splice.
Added a targeted regression test:
it('removes the whole section including trailing detail when deleting the only item', () => {
const desc = '### AC\n- [ ] Only\nDetail line\n\n### Next\n- [ ] Other';
const checklists = parseInlineChecklists(desc);
const result = removeChecklistItem(desc, checklists[0].items[0].id, checklists);
expect(result).toBe('### Next\n- [ ] Other');
});…oveChecklistItem addItemToChecklist() anchored insertion at the last checkbox line, so trailing detail/prose lines that belong to that item got displaced under the newly-inserted checkbox. Fix: advance insertIdx past non-empty non-heading lines so the new item always lands after any trailing detail. removeChecklistItem() called removeSectionBlock with targetLineIdx (the checkbox line) even when the section held detail lines after the only checkbox, leaving those lines orphaned before the next heading. Fix: scanSection now tracks lastContentIdx (last non-empty line in section) and the single-item deletion path uses it as the section end. Adds regression tests for both scenarios per reviewer feedback. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
Requesting changes for one remaining inline checklist delete regression with detail lines in multi-item sections.
Code Issues
Should Fix
- src/pm/_shared/inline-checklist.ts:257 —
removeChecklistItem()now removes trailing detail when deleting the only checkbox in a section, but multi-item sections still remove only the checkbox line. Verified locally with### AC\n- [ ] Remove\n Detail for remove\n- [ ] Keep: deletingRemovereturns### AC\n Detail for remove\n- [ ] Keep. The same happens for a last item with detail, where the detail becomes attached to the previous item. Because this PR intentionally treats prose/detail lines as part of the checklist section andaddItemToChecklist()places new items after detail belonging to the previous row, deletion should remove the deleted item’s following detail block up to the next checkbox or heading.
🕵️ codex · gpt-5.5 · run details
| removeSectionBlock(lines, scan.headingIdx, scan.targetLineIdx); | ||
| // Remove the entire section: use lastContentIdx so trailing detail lines | ||
| // after the only checkbox are included and not left orphaned. | ||
| const sectionEnd = scan.lastContentIdx !== -1 ? scan.lastContentIdx : scan.targetLineIdx; |
There was a problem hiding this comment.
This only expands deletion through trailing detail when the section has exactly one checkbox. Multi-item sections still take the else path and splice only the checkbox line, so deleting Remove from `### AC
- Remove
Detail for remove - Keep
leavesDetail for removeorphaned aboveKeep`; deleting a last item with detail leaves that detail attached to the previous item. Since the parser/add path now treats these detail/prose lines as section content belonging to the preceding checklist item, the delete path needs to remove the target item’s following detail block up to the next checkbox or heading as well.
There was a problem hiding this comment.
Fixed. The else branch in removeChecklistItem() now scans forward from targetLineIdx + 1 and extends the splice through any consecutive non-checkbox, non-heading, non-blank detail lines before the next item boundary.
For ### AC\n- [ ] Remove\n Detail for remove\n- [ ] Keep:
- Loop sees
Detail for remove→ non-blank, non-checkbox, non-heading →deleteEnd = 2 - Loop sees
- [ ] Keep→ checkbox → break lines.splice(1, 2)removes both- [ ] RemoveandDetail for remove- Result:
### AC\n- [ ] Keep✓
Same fix handles the last-item-with-detail case: trailing detail stops at end-of-section rather than spilling onto the previous item.
Added two regression tests covering both scenarios.
…ions When deleting a checklist item in a multi-item section, also remove any non-checkbox, non-heading, non-blank detail/prose lines that immediately follow the deleted checkbox line. Previously only the checkbox line was spliced out, leaving trailing detail orphaned (e.g. ` Detail for remove` appearing above the next item or attached to the previous one). The single-item section path already used `lastContentIdx` to cover this case; the multi-item `else` branch now scans forward from the target line and removes the whole item block in a single splice. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
nhopeatall
left a comment
There was a problem hiding this comment.
LGTM - the latest diff addresses the prior inline checklist add/delete regressions, wraps all Linear/JIRA whole-description checklist mutations in the shared lock, and keeps local parser errors out of the provider retry path. Focused unit coverage passes locally: npx vitest run --project unit-core tests/unit/pm/_shared/inline-checklist.test.ts tests/unit/pm/_shared/description-mutation-lock.test.ts tests/unit/pm/linear/adapter.test.ts tests/unit/pm/jira/adapter.test.ts.
🕵️ codex · gpt-5.5 · run details
Summary
Fixes https://linear.app/issue/MNG-656
Linear and JIRA inline checklist mutations rewrite the entire work-item description, so concurrent checklist item updates could race and overwrite sibling rows. This PR serializes those description mutations per provider/work item and hardens the shared inline checklist scanner so prose or detail lines between checkbox rows do not hide later checklist IDs.
Changes
withDescriptionMutationLock(provider, workItemId, fn)for stale-safe temp-file locking with timeout, polling, and token-checked release.getIssue/updateIssuefailures so local parser errors such as missing checklist IDs are not mislabeled as provider conflicts.Testing
npx vitest run --project unit-core tests/unit/pm/_shared/inline-checklist.test.ts tests/unit/pm/_shared/description-mutation-lock.test.ts tests/unit/pm/linear/adapter.test.ts tests/unit/pm/jira/adapter.test.tsnpm run typechecknpm run lintnpm testNote:
npm run lintexits 0 but still reports existing warning-level complexity/non-null assertions outside this change set.🕵️ codex · gpt-5.5 · run details