Keep Content image uploads alive through native file selection - #2820
Keep Content image uploads alive through native file selection#28203mdistal wants to merge 5 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Visual recap — screenshot failedA recap was published, but the PR-comment screenshot could not be captured or uploaded. Open the interactive recap directly: Open the full interactive recap Diagnostic: light: page.goto: Timeout 45000ms exceeded. Call log: - navigating to "https://plan.agent-native.com/recaps/recap-10c9aa913b1e4fe3?recapScreenshot=1&recapScreenshotTheme=light", waiting until "domcontentloaded" |
There was a problem hiding this comment.
Builder reviewed your changes and found 5 potential issues 🟡
Review Details
Incremental Code Review Summary
The latest revision substantially reworks image-file selection: picker ownership now lives in VisualEditor, operation IDs distinguish the native-picker and upload phases, missing placeholders can be reconstructed, and existing-image replacement controls are disabled during active uploads. The previous overlapping replacement-upload issue is fixed, so its review thread was resolved. Render validation and marker clearing remain correctly ordered on the normal success path.
Risk assessment: Standard. The remaining concerns are lifecycle and collaborative-editor edge cases in the new recovery boundary.
Key Findings
- 🟡 MEDIUM — Reconstruction uses an unmapped absolute position, so concurrent edits can place the image in the wrong block.
- 🟡 MEDIUM — A pending image can be resurrected after the user deletes it.
- 🟡 MEDIUM — Outside-click and assets-dialog close paths can clear or retain picker state incorrectly.
- 🟡 MEDIUM — Completion can dispatch against a destroyed editor after unmount.
🧪 Browser testing: Will run after this review (PR touches UI code). The dev server is healthy; the previous browser run had no per-test results, so this is a FULL run.
| const position = Math.min( | ||
| Math.max(request.position, 0), | ||
| view.state.doc.content.size, | ||
| ); | ||
| try { | ||
| let tr = view.state.tr.insert( | ||
| position, | ||
| imageType.create({ ...request.attrs, ...attrs }), |
There was a problem hiding this comment.
🟡 Map the saved picker position through collaborative edits
The fallback reconstruction uses the raw request.position captured before the native picker opened and only clamps it to the current document size. Inserts or deletes before that position while the picker is open can recreate the placeholder or completed image in a different paragraph; preserve a mapped/bookmarked or collaboration-aware anchor for recovery.
Additional Info
Reported by all three review agents; the existing tests cover removal without intervening document edits.
There was a problem hiding this comment.
Not changing this in this repair. The saved position is only a fallback when reconciliation removed the marked placeholder; preserving a collaboration-aware relative anchor requires a separate editor-position design and is outside the approved image completion acceptance. The primary path continues to locate the live node by upload ID.
| if (updatePendingMediaNode(view, "image", uploadId, attrs)) return true; | ||
| return insertImageNodeAtPendingPosition( |
There was a problem hiding this comment.
🟡 Do not resurrect a deliberately deleted pending image
If a user deletes the pending image node while its upload is awaiting storage or render validation, updatePendingMediaNode returns false and this fallback inserts the completed image at the saved position. The error path similarly restores an empty placeholder, so deletion is undone asynchronously; distinguish local deletion/cancellation from collaborative reconciliation before reconstructing.
Additional Info
Reported by one review agent; confirmed from the missing-node fallback and the fact that pending controls do not prevent editor deletion.
There was a problem hiding this comment.
Not changing this in this repair. Reconstructing a missing placeholder is the deliberate behavior required to survive collaborative reconciliation while the native picker owns focus. Distinguishing an intentional local deletion from remote reconciliation needs an explicit cancellation signal that this editor boundary does not currently expose; that is a separate product decision.
There was a problem hiding this comment.
Builder reviewed your changes — no new findings
Review Details
Incremental Code Review Summary
The latest revision adds a staged image-upload lifecycle: the uploaded URL is first applied with the upload marker retained, the actual editor-owned <img> is observed for successful rendering, and only then is the marker cleared and the image considered complete. The rollback tests now cover staged identity, render failure, and avoiding duplicate placeholders. The teardown concern from the previous review is fixed: staging/commit callbacks and the error path check editor.isDestroyed, and that review thread was resolved.
The four other previously reported issues remain open and unchanged, so they were not reposted: raw picker-position reconstruction under collaboration, resurrection after deliberate deletion, outside-click clearing active upload markers, and Assets-dialog close marker leakage. No genuinely new issue was identified in this incremental diff.
Risk assessment: Standard. The implementation direction is sound, with the remaining concerns confined to the already-open collaborative/lifecycle edge cases.
🧪 Browser testing: Will run after this review (PR touches UI code).
Problem
Content's
/imageslash command crosses a native file-picker boundary. While that dialog owns focus, the collaborative editor can reconcile the transient empty image placeholder away. The picker can then return to a node-view-owned input and handler whose node no longer exists, so no upload request is made and the image block disappears.The image completion path also reported success after storage returned a URL but before the editor proved its rendered
<img>could use that URL. This was especially confusing for SVG uploads: the toast could say “Image added” while the document still showed no image.Ordinary code-block insertion is covered here only as a regression story. Mermaid rendering and optimistic local image previews remain separate repair surfaces.
Approach
Move native image selection ownership from the transient image node view to the durable
VisualEditor. The editor records the operation's marker, intended position, and image attributes before opening the dialog, so it can promote or reconstruct the pending node after selection.After upload, stage the returned URL on that marked editor node, wait for the actual rendered
<img>to load successfully, and only then clear the marker, persist the completed image, and report success. Render failure restores one selected, retryable empty image block with no broken source.What changed
VisualEditorand preserved recovery when collaboration removes the placeholder.Safety and operations
There are no schema, migration, credential, permission, or storage-format changes. Uploaded bytes may remain in provider storage when render validation fails; this PR does not guess whether a provider-owned object is safe to delete. Audio/video completion, Mermaid rendering, and the separately tracked optimistic loading preview are unchanged.
Verification
e0790e553, real Chrome plus the macOS picker uploaded an SVG, displayed “Image added” only with the SVG visibly rendered, and preserved it after reload./code blocksentinelconst acceptance = true;remained highlighted and persisted after reload on the same document.f9d6d13ef; the final picker-marker follow-up one0790e553has local and real-interface verification but has not received another independent review.Review focus