Skip to content

Keep Content image uploads alive through native file selection - #2820

Open
3mdistal wants to merge 5 commits into
BuilderIO:mainfrom
3mdistal:t3code/investigate-content-editor-failures
Open

Keep Content image uploads alive through native file selection#2820
3mdistal wants to merge 5 commits into
BuilderIO:mainfrom
3mdistal:t3code/investigate-content-editor-failures

Conversation

@3mdistal

@3mdistal 3mdistal commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Problem

Content's /image slash 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

  • Moved native picker lifecycle ownership into VisualEditor and preserved recovery when collaboration removes the placeholder.
  • Kept the upload marker on the staged image until the real editor element passes load validation.
  • Restored the original empty attributes on failure and guarded late completion after editor teardown.
  • Added focused coverage for existing-node staging, final marker clearing, rollback without duplicates, placeholder reconstruction, and render ordering/failure.

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

  • 79 focused editor/image lifecycle tests pass, demonstrating staged identity, render-before-commit ordering, rollback, and persistence guards.
  • Content typecheck passes; local execution emits the expected diagnostics for absent production auth/database environment values.
  • All 51 repository guards pass.
  • On exact head e0790e553, real Chrome plus the macOS picker uploaded an SVG, displayed “Image added” only with the SVG visibly rendered, and preserved it after reload.
  • The ordinary /code block sentinel const acceptance = true; remained highlighted and persisted after reload on the same document.
  • One bounded independent review found and then verified the core staged-marker repair through f9d6d13ef; the final picker-marker follow-up on e0790e553 has local and real-interface verification but has not received another independent review.

Review focus

  • Does the pending upload ID remain stable across collaborative node movement until actual render validation finishes?
  • Does every failure path restore exactly one usable empty image block without persisting the staged source?
  • Are teardown and timeout behavior acceptably bounded without allowing a late editor dispatch?
content_product_impact:
  lane: contract_repair
  features:
    - content.feature.read-and-annotate-anything
  capabilities:
    - content.author.media
  record_change: none
  proof:
    - 79 focused editor and image lifecycle tests pass on e0790e553
    - Content typecheck passes
    - All 51 repository guards pass
    - Real SVG upload and reload persistence pass through Chrome and the macOS picker
    - Ordinary code block insertion and reload persistence pass
  rationale: This restores the approved image insertion contract at the editor/native-picker boundary without combining Mermaid or optimistic-preview work.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@github-actions

github-actions Bot commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Visual recap — screenshot failed

A 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"

@3mdistal
3mdistal marked this pull request as ready for review August 12, 2026 15:30
@3mdistal
3mdistal requested a review from steve8708 August 12, 2026 15:30
builder-io-integration[bot]

This comment was marked as outdated.

@3mdistal 3mdistal changed the title Report Content image upload success only after rendering Keep Content image uploads alive through native file selection Aug 12, 2026

@builder-io-integration builder-io-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Comment on lines +1260 to +1267
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 }),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 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.

Fix in Builder

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

Comment on lines +1298 to +1299
if (updatePendingMediaNode(view, "image", uploadId, attrs)) return true;
return insertImageNodeAtPendingPosition(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 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.

Fix in Builder

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

Comment thread templates/content/app/components/editor/extensions/ImageBlock.tsx Outdated
Comment thread templates/content/app/components/editor/extensions/ImageBlock.tsx
Comment thread templates/content/app/components/editor/VisualEditor.tsx

@builder-io-integration builder-io-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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).

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