Fix paste availability across editor tabs - #1101
Conversation
Use sessionStorage as the clipboard source of truth. Notify each editor when clipboard data changes and keep paste enabled after the temporary localStorage transport value is removed. Avoid broadcasting empty or invalid clipboard data.
Reviewer's GuideCentralizes clipboard availability logic in SvgCanvas using sessionStorage as the source of truth, emits a clipboardChanged event to keep the editor UI in sync across tabs, and adds tests to ensure cross-tab copy/paste keeps Paste enabled even after localStorage transport cleanup. Sequence diagram for cross-tab clipboardChanged and paste availabilitysequenceDiagram
actor User
participant Tab1_Editor as Tab1_Editor
participant Tab1_SvgCanvas as Tab1_SvgCanvas
participant SessionStorage as SessionStorage
participant LocalStorage as LocalStorage
participant Tab2_SvgCanvas as Tab2_SvgCanvas
participant Tab2_Editor as Tab2_Editor
User->>Tab1_Editor: copySelectedElements
Tab1_Editor->>Tab1_SvgCanvas: copySelectedElements
Tab1_SvgCanvas->>SessionStorage: setItem(CLIPBOARD_ID, data)
Tab1_SvgCanvas->>Tab1_SvgCanvas: call(clipboardChanged)
Tab1_SvgCanvas->>Tab1_SvgCanvas: flashStorage
Tab1_SvgCanvas->>LocalStorage: setItem(CLIPBOARD_ID, data)
LocalStorage-->>Tab2_SvgCanvas: storage event (key=CLIPBOARD_ID, newValue)
Tab2_SvgCanvas->>SessionStorage: setItem(CLIPBOARD_ID, newValue)
Tab2_SvgCanvas->>Tab2_SvgCanvas: call(clipboardChanged)
Tab2_SvgCanvas->>Tab2_Editor: clipboardChanged handler
Tab2_Editor->>Tab2_SvgCanvas: hasClipboardData
Tab2_SvgCanvas-->>Tab2_Editor: boolean
Tab2_Editor->>Tab2_Editor: enableOrDisableClipboard
Tab2_Editor->>Tab2_Editor: canvMenu.setAttribute(enablemenuitems|disablemenuitems)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
SvgCanvas.hasClipboardDataandflashStorage, consider wrappingsessionStorage/localStorageaccess in a try/catch similar to the previousenableOrDisableClipboardlogic so that environments with restricted storage (e.g., private mode) don’t throw and break clipboard enablement. - You fire
clipboardChangedboth when copying locally and when receiving storage events regardless of whetherhasClipboardData()is true; you might want to gate or coalesce these emissions so the editor UI only reacts when pasteability actually changes, reducing redundant updates on invalid/empty payloads.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `SvgCanvas.hasClipboardData` and `flashStorage`, consider wrapping `sessionStorage`/`localStorage` access in a try/catch similar to the previous `enableOrDisableClipboard` logic so that environments with restricted storage (e.g., private mode) don’t throw and break clipboard enablement.
- You fire `clipboardChanged` both when copying locally and when receiving storage events regardless of whether `hasClipboardData()` is true; you might want to gate or coalesce these emissions so the editor UI only reacts when pasteability actually changes, reducing redundant updates on invalid/empty payloads.
## Individual Comments
### Comment 1
<location path="packages/svgcanvas/svgcanvas.js" line_range="297-301" />
<code_context>
} else if (ev.key === CLIPBOARD_ID) {
// Another tab sent data.
sessionStorage.setItem(CLIPBOARD_ID, ev.newValue)
+ this.call('clipboardChanged')
}
}
</code_context>
<issue_to_address>
**suggestion:** Avoid storing a literal 'null' string in sessionStorage when clipboard is cleared
When `ev.newValue` is `null`, this call stores the string `'null'` under `CLIPBOARD_ID`. Since we treat clipboard absence specially, consider calling `sessionStorage.removeItem(CLIPBOARD_ID)` instead in that case to keep storage semantics clear and avoid surprises for code that reads the raw storage value.
```suggestion
} else if (ev.key === CLIPBOARD_ID) {
// Another tab sent data.
if (ev.newValue === null) {
sessionStorage.removeItem(CLIPBOARD_ID)
} else {
sessionStorage.setItem(CLIPBOARD_ID, ev.newValue)
}
this.call('clipboardChanged')
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Thanks for the suggestion. The storageChange handler returns before this |
Use sessionStorage as the clipboard source of truth. Notify each editor when clipboard data changes and keep paste enabled after the temporary localStorage transport value is removed. Avoid broadcasting empty or invalid clipboard data.
PR description
Summary
This PR fixes copy and paste between SVG-Edit instances running in separate same-origin browser tabs.
Clipboard data was already transferred between tabs through a temporary
localStoragevalue and stored in each tab'ssessionStorage. However, the Paste context-menu actions were disabled again when the temporarylocalStoragevalue was removed, even though valid clipboard data remained available insessionStorage.Changes
sessionStorageas the source of truth for clipboard availability.hasClipboardData()to validate that the clipboard contains pasteable elements.clipboardChangedevent after:localStoragetransport value when determining Paste availability.Verification
npm testpassed.localStoragevalue is removed;Checklist
Note that we require UI tests to ensure that the added feature will not be
nixed by some future fix and that there is at least some test-as-documentation
to indicate how the fix or enhancement is expected to behave.
npm test, ensuring linting passes and that Cypress UI tests keepcoverage to at least the same percent (reflected in the coverage badge
that should be updated after the tests run)
help both for future users and for the PR reviewer.
Summary by Sourcery
Ensure clipboard availability is driven by sessionStorage-backed events so paste remains enabled across same-origin editor tabs.
New Features:
Bug Fixes:
Enhancements:
Tests: