Skip to content

fix(image-paste): preserve attachment identity across draft restoration - #688

Open
ooiuuii wants to merge 1 commit into
openpi-dev:mainfrom
ooiuuii:fix/image-paste-roundtrip-identity
Open

ooiuuii wants to merge 1 commit into
openpi-dev:mainfrom
ooiuuii:fix/image-paste-roundtrip-identity

Conversation

@ooiuuii

@ooiuuii ooiuuii commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

Problem

Two reversible image-paste editor transitions lose the pasted image reference:

  • Type literal [Image #1] , paste an image, then queue and restore the input. Restoration reuses #1, leaving two ambiguous tokens; Enter submits no image path.
  • Paste an image, press Ctrl+U, then native Undo. The visible token returns without its mapping, so Enter submits the token instead of the image path.

Both reproduce on main 3cb2ecf through the actual extension and locked Pi 0.99.1 editor/queue hooks. These are distinct from the repeated-path adoption fixed in #418.

Value

Preserve literal user text and attachment identity across queue restoration and native Undo, without changing persisted sessions or introducing another editor state machine.

Approach

  • Reserve literal token IDs when adopting expanded image paths.
  • Keep IDs stable within a draft and recover recently removed mappings only during explicit native Undo.
  • Revoke recovery when users type or duplicate matching tokens; clear mappings at submission, explicit reset and session boundaries.
  • Bound removed-mapping retention to 128 entries. Older evicted tokens remain literal on Undo.
  • Add native-editor regressions and preserve feat(tui): add compact image paste placeholders #418 repeated-path controls.

Visible numbering intentionally changes: deleting #2 and pasting again produces #3 rather than reusing #2. The existing submitted-payload assertion remains unchanged.

Validation

  • Baseline: two failing regression witnesses and two passing controls.
  • Candidate: 42 focused tests passed; original witnesses 4/4 passed.
  • Product-only revert with regression tests retained failed again; restoring the fix passed.
  • Final bun run check and git diff --check passed.
  • Separate UI lane: 82 files / 1,158 tests passed.
  • Full Node lane: 2,300 passed, 8 skipped, 3 background-terminal process-tree cleanup failures also present in saved base evidence. This run preceded the final small Undo-insertion guard; final focused tests and checks cover that refinement. The combined command stopped before UI, which was run separately. No full-suite-green claim.
  • One bounded independent source review found no actionable in-scope issue.

The harness uses real native editor input/Undo and queue/restore hooks; rendering and session transport are local stubs. Installed TUI loading, OS clipboard interaction and live provider behavior were not tested.

Impact

Scoped to image-paste bookkeeping and its tests. Clipboard files are not deleted. Literal/ambiguous tokens remain literal, and reset boundaries cannot revive cleared attachments. The 128-entry recovery window is deliberately bounded, not unlimited native-equivalent image Undo.

@tt-a1i tt-a1i left a comment •

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed head: 1291d0baa621323c10cc3bba8a498c4301d21e03 against main a18f457687b45c877bc8a15d689a7580c4f0659a.

Standards: no confirmed code-level blocker in the scoped change.

Spec: the inspected change matches the stated behavior in this PR. #743 and #688 overlap in Session draft cleanup; retain one integrated behavior and both relevant regressions.

Validation: node --test --experimental-strip-types tests/extensions/image-paste/native-editor.test.ts tests/extensions/image-paste/index.test.ts: exit=0; ℹ tests 42; ℹ pass 42; ℹ fail 0; ℹ skipped 0

The unchanged-head focused receipts are reused after a live head/main refresh; #718 uses this round's latest-head tests. This code review does not claim installed-runtime, live-provider, full merged-queue or visual acceptance. No merge is authorized by this review.

Integration follow-up: this head does not contain current main a18f457687b45c877bc8a15d689a7580c4f0659a. Please synchronize the branch and rerun the required checks before merging; a changed head requires fresh review. The current source approval does not override strict up-to-date protection.

Current-main integration

The original head remains 1291d0baa621323c10cc3bba8a498c4301d21e03. Because its fork does not permit maintainer edits, separate draft #761 preserves this contributor commit and merges frozen main a18f457687b45c877bc8a15d689a7580c4f0659a; prepared head b3590807932338e0ca36ceb90ea290bf476806b0 has the byte-identical original source/test patch. The original source APPROVE does not establish the new draft’s integration gate. Current-head local full-test receipt: bun run test exits 0 at b3590807: Node 2,321 passed / 9 skipped / 0 failed; UI 1,158 passed across 82 files.. New-draft remote CI receipt, independently checked: CI run 38353110690 is terminal SUCCESS on checkout merge 4f11d0b3; 17/17 CI jobs succeeded. E2E and Plan-provider fixture checks pass. The integration remains draft and needs eligible maintainer review.. Protected main merge and closing this original PR remain pending human selection; this update changes neither authorization.

@ooiuuii

ooiuuii commented Oct 15, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for integrating this in #761 and handling the overlap with #743. Maintainer edits are now enabled here.

I'm happy to continue through #761, keeping the integrated cleanup behavior and the relevant regressions together. Its current head (4388482) has 19 successful checks and is no longer a draft. I haven't refreshed this conflicting original branch independently, since that would duplicate your integration work. I understand that automated/in-process coverage is not a claim of full interactive clipboard/TUI acceptance; please let me know if there is a specific remaining scenario you would like me to validate.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants