Conversation
…le-drag-support
|
@sushilbang is attempting to deploy a commit to the sz47's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughIntroduces an event-driven sync pipeline with timestamped change events and queued persistence; replaces plain-text paste handling with a comprehensive WiskPasteHandler; adds paste-link UI, new link/embed components and page-creation flow; expands plugin UI, touch/drag and virtual-keyboard handling; adds templates and editor font-size theming. Changes
Sequence DiagramsequenceDiagram
participant UI as Editor UI / Plugins
participant Editor as js/editor.js
participant Sync as wisk.sync
participant Doc as In-memory Document
participant DB as wisk.db
participant Ext as External metadata service
UI->>Editor: user action (paste / create / edit / delete / paste-link)
Editor->>Sync: wisk.sync.newChange(event{path, value, agent, timestamp})
Sync->>Sync: validate & populate metadata
Sync->>Sync: append event to eventLog
alt apply immediately / enqueue save
Sync->>Doc: applyEvent(document, event) — mutate target paths
Doc-->>Sync: document updated (lastUpdated, syncLogs)
Sync->>DB: enqueueSave / wisk.db.setPage(document)
DB-->>Sync: persist OK / error
Sync-->>Editor: persistence result
end
opt external metadata needed (link-preview/embed)
UI->>Ext: fetch metadata
Ext-->>UI: metadata
UI->>Editor: create newChange for metadata
Editor->>Sync: wisk.sync.newChange(metadataEvent)
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
js/plugins/code/link-preview-element.js (2)
193-258: Dead code referencing undefinedthis.editable.These methods (
handleSpecialKeys,handleBackspace,onValueUpdated,bindEvents) referencethis.editable, but:
- The editable element is commented out in
render()(lines 407-413)- The
this.editableassignment is commented out inconnectedCallback()(line 17)bindEvents()is never calledIf this component is now purely a display/preview element (non-editable), remove these dead methods to avoid confusion. If editing functionality is planned, the editable element and bindings need to be restored.
#!/bin/bash # Check if these methods are referenced anywhere rg -n "handleSpecialKeys|onValueUpdated" --type js -g "!link-preview-element.js"
421-434:bindEventsreferences non-existent elements.This method references
.openbutton andthis.editable, both of which are commented out or undefined. SincebindEvents()is not called inconnectedCallback, this is dead code that should be removed.js/plugins/code/base-text-element.js (1)
878-889: Missing paste-link-menu element in render template.The code references
this.pasteLinkMenuinconnectedCallback(line 352) and uses it throughout the paste link methods, but the render template doesn't include the HTML for.paste-link-menu. This will cause the paste link menu feature to silently fail.Add the paste link menu markup to the render template:
<div class="emoji-suggestions"></div> + <div class="paste-link-menu" style="display: none; position: absolute; z-index: 1000;"> + <div class="paste-link-option selected" data-type="url"> + <span>🔗</span> Paste as link + </div> + <div class="paste-link-option" data-type="bookmark"> + <span>📑</span> Create bookmark + </div> + <div class="paste-link-option" data-type="embed"> + <span>📦</span> Create embed + </div> + </div> `;Also add corresponding styles for
.paste-link-menuand.paste-link-optionin the<style>section.
🧹 Nitpick comments (12)
style.css (1)
24-24: Consider using CSS variable instead of hardcoded value.Changing from
var(--gap-2)to a hardcoded2pxbreaks consistency with the design token system. If this specific value is intentional, consider defining a new CSS variable or documenting why the deviation is needed.- gap: 2px; + gap: var(--gap-2);js/sync/sync.js (2)
180-190: Redundant path length check.The condition
pathParts.length === 2at line 180 is redundant sinceevent.path === 'data.elements'already guarantees exactly 2 path parts. This could be simplified.- if (event.path === 'data.elements' && pathParts.length === 2) { + if (event.path === 'data.elements') {
276-280: Consider bounding syncLogs growth.Events are appended to
syncLogsindefinitely. For long-lived documents with frequent edits, this array could grow very large, impacting IndexedDB storage and document load times. Consider implementing a retention policy or compaction strategy.js/plugins/plugins.js (1)
38-38: Consider expanding the comment for clarity.The comment "page-element is handled specially in selector" is helpful, but could be more descriptive about what "specially" means. This would help future maintainers understand why it's pre-populated.
- loadedPlugins: ['page-element'], // page-element is handled specially in selector + loadedPlugins: ['page-element'], // Pre-loaded: page-element uses link-element component but bypasses normal plugin loading for selector page creation workflowjs/elements/selector-element.js (2)
63-70: Magic string check for "Page" type.The condition
newDetail.title === 'Page'relies on a string literal. Consider using a dedicated property (e.g.,newDetail.isPageTypeor checkingnewDetail.component === 'page-element') to make this more robust against title changes in the plugin data.- if (newDetail.title === 'Page') { + if (newDetail.component === 'page-element' || newDetail.title === 'Page') {
125-137: Remove or track commented-out code.The commented
createChildPagemethod appears to be dead code. Consider either removing it or creating an issue to track if this alternative implementation is needed later.js/plugins/code/link-element.js (2)
57-69: Consider extracting hardcoded domain to a constant.The
https://app.wisk.ccstring is hardcoded. If this domain could change or differs between environments (staging/production), consider extracting it to a configuration constant.The empty catch block on line 66 is acceptable here since invalid URLs should simply return
falseforisInternal.
466-470: Auto-refresh timing may need adjustment.The 500ms delay for refreshing internal links on
DOMContentLoadedis arbitrary. If pages load slowly or IndexedDB access is delayed, this might execute before all elements are ready. Consider using a more robust trigger (e.g., waiting forwisk.dbto be ready, or listening for a custom "editor-ready" event).js/editor.js (3)
345-348: Add error handling for async operation in setTimeout.The
await wisk.sync.saveModification()inside setTimeout can throw, but errors won't be caught or logged.// Save immediately (critical event) setTimeout(async () => { - await wisk.sync.saveModification(); + try { + await wisk.sync.saveModification(); + } catch (err) { + console.error('Failed to save block creation:', err); + } }, 0);The same pattern applies to lines 730-732 and 1072-1073.
186-212: Consider adding email validation for addUserAccess.The function accepts any string as email without validation. While server-side validation should exist, client-side validation improves UX by providing immediate feedback.
wisk.editor.addUserAccess = async function (email) { const timestamp = Date.now(); + + // Basic email format validation + const emailRegex = /^[^\s@]+@[^\s@]+\.[^\s@]+$/; + if (!emailRegex.test(email)) { + console.warn('Invalid email format:', email); + return; + } // Initialize access array if it doesn't exist
2646-2651: Address TODO: Consider flushing pending updates on page unload.The comment correctly identifies that pending updates could be lost if the user closes the tab before the debounce triggers. Consider adding a
beforeunloadhandler.// Add near initEditor or initializeRectangleSelection window.addEventListener('beforeunload', async (e) => { if (elementUpdatesNeeded.size > 0) { clearTimeout(debounceTimer); // Synchronously flush updates - use sendBeacon or synchronous approach // Note: async operations may not complete during unload } });Would you like me to provide a more complete implementation using
navigator.sendBeaconfor reliable unload persistence?js/plugins/code/base-text-element.js (1)
1033-1044: Consider guarding against component unmount during timeout.If the element is removed from DOM between
showPasteLinkMenucall and the setTimeout callback, the listener may be added to a detached element or cause issues.// Close on outside click setTimeout(() => { + // Guard against component being unmounted + if (!this.isConnected) return; + this._closePasteMenuHandler = (e) => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
js/plugins/icons/page.svgis excluded by!**/*.svg
📒 Files selected for processing (13)
js/editor.js(18 hunks)js/elements/selector-element.js(1 hunks)js/plugins/code/base-text-element.js(17 hunks)js/plugins/code/embed-element.js(1 hunks)js/plugins/code/link-element.js(1 hunks)js/plugins/code/link-preview-element.js(7 hunks)js/plugins/code/list-element.js(1 hunks)js/plugins/code/numbered-list-element.js(2 hunks)js/plugins/code/text-element.js(4 hunks)js/plugins/plugin-data.json(1 hunks)js/plugins/plugins.js(2 hunks)js/sync/sync.js(2 hunks)style.css(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
js/plugins/code/link-element.js (2)
js/wisk.js (1)
wisk(3-3)js/editor.js (3)
index(1008-1008)index(1018-1018)wrapper(85-85)
js/elements/selector-element.js (1)
js/wisk.js (1)
wisk(3-3)
js/sync/sync.js (2)
js/wisk.js (1)
wisk(3-3)js/editor.js (7)
event(1731-1731)event(1801-1801)pathParts(518-518)pathParts(605-605)i(767-767)i(901-901)i(1273-1273)
js/editor.js (1)
js/sync/sync.js (3)
elementId(199-199)pathParts(154-154)element(200-200)
js/plugins/code/link-preview-element.js (1)
js/wisk.js (1)
wisk(3-3)
js/plugins/code/numbered-list-element.js (1)
js/wisk.js (1)
wisk(3-3)
js/plugins/code/embed-element.js (2)
js/wisk.js (1)
wisk(3-3)js/editor.js (2)
prevElement(308-308)nextElement(1459-1459)
🪛 Biome (2.1.2)
js/editor.js
[error] 968-968: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
js/plugins/code/base-text-element.js
[error] 1153-1153: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🔇 Additional comments (22)
style.css (1)
198-201: LGTM!The touch and selection disabling properties are appropriate for interactive hover elements, preventing unwanted text selection and touch callouts during drag/interaction operations.
js/plugins/code/list-element.js (1)
145-152: LGTM!The
top: 0.6empositioning aligns the bullet point better with the text baseline, which is a cleaner approach than the previoustranslateY(-50%)centering. This works well with theline-height: 1.5on the editable element.js/plugins/plugin-data.json (1)
170-213: Verify inconsistenttextualproperty between page-element and link-element.Both plugins use the same
component: "link-element", butpage-elementhas"textual": false(line 185) whilelink-elementhas"textual": true(line 208). This could affect how the editor handles text-related operations (e.g., backspace merging, content appending) differently for what appears to be the same underlying component.Please verify this is intentional behavior.
js/plugins/plugins.js (1)
7-7: LGTM!Adding
link-elementto the default plugins list ensures it's available for the new page/link creation workflows introduced in this PR.js/plugins/code/text-element.js (2)
15-31: Style improvements look good.The increased
min-heightand padding for the editable area, along with the accent color for links (including visited state), improve visual consistency.
114-157: Paste-link menu UI integration is properly implemented.The styling and HTML structure for the paste-link menu are well-defined with the menu options (URL, Bookmark, Embed) correctly wired to the base-text-element's event handlers. All three required handlers (
showPasteLinkMenu,hidePasteLinkMenu,handlePasteLinkChoice) are properly implemented in BaseTextElement and fully functional.js/plugins/code/numbered-list-element.js (2)
119-157: Improved emptiness detection logic.The new combined check (
innerText.trim().length === 0 && this.editable.children.length === 0) is more robust than a simple text check, properly handling cases where the element might contain non-text children like inline elements.Note: The indentation in this method appears inconsistent (the function body starts at column 0 instead of being indented within the class). This might be a formatting artifact, but verify it matches the project's code style.
211-216: Number positioning adjustment is appropriate.Switching from vertical centering to top alignment with matching
line-height: 1.5ensures the list number aligns with the first line of text content, which is the expected behavior for numbered lists.js/plugins/code/link-preview-element.js (1)
16-32: Click handler implementation is good.The click handler properly normalizes URLs without protocol prefix before opening. Making the entire preview clickable provides good UX.
js/plugins/code/link-element.js (2)
1-39: Well-structured custom element implementation.The component follows good patterns:
- Proper use of
observedAttributesandattributeChangedCallback- Clean separation of
connectedCallbackanddisconnectedCallback- Conditional re-render only when connected and value changed
261-278: This pattern is consistent with existing codebase architecture.The
updateLinkedPageTitlemethod correctly useswisk.db.setPage()directly because it updates a different page, not the currently-edited document. This matches the pattern used throughout the codebase for updating other pages (seedatabase-element.js,selector-element.js, andsync.jsitself).The sync system's
newChange()is intentionally limited to the current document (wisk.editor.document), while directsetPage()calls handle updates to other pages. Unless linked pages should participate in the event-driven sync system—which the current architecture doesn't support—no changes are needed.js/editor.js (3)
546-565: Potential double mutation in deletion handler.The code manually filters
wisk.editor.document.data.elementson lines 559-560, then callswisk.sync.applyEventon line 562 which likely also modifies the document state. This could cause inconsistencies.Consider whether the manual array mutation is needed if
applyEventalready handles it, or move theapplyEventcall before the manual mutation:if (!deletedElements.includes(deletedId)) { deletedElements.push(deletedId); const element = document.getElementById(`div-${deletedId}`); if (element) { document.getElementById('editor').removeChild(element); } + wisk.sync.applyEvent(wisk.editor.document, event); wisk.editor.document.data.elements = wisk.editor.document.data.elements.filter(e => e.id !== deletedId); - - wisk.sync.applyEvent(wisk.editor.document, event); }
2268-2299: Good defensive sanitization for clipboard HTML.The
sanitizeHtmlForClipboardfunction properly cleans anchor attributes to prevent JSON corruption. The approach of removing all attributes then re-adding only clean ones is robust.
304-306: Good addition of lastUpdated timestamp for conflict resolution.Adding
lastUpdatedto block objects enables proper last-writer-wins conflict resolution in the event-driven sync model.js/plugins/code/base-text-element.js (5)
1066-1133: Paste link choice handling is well-structured.Each case properly handles URL insertion/conversion with appropriate block wrapping. The inline link-element insertion correctly adds a trailing space and updates cursor position.
1806-1838: Link conversion sanitization looks correct.The
convertLinksToElementshelper properly sanitizes hrefs and converts anchors to atomiclink-elementcomponents with appropriate attributes.
970-992: URL validation utilities are adequate for the use case.The
isUrlfunction correctly rejects strings with spaces/newlines and uses URL constructor validation.isInternalUrlproperly identifies internal wisk pages. The empty catch blocks are acceptable here since invalid URLs should simply return false.
1599-1608: Improved placeholder detection for child elements.The updated
updatePlaceholdercorrectly considers both text content and child elements (likelink-element), preventing the placeholder from showing when inline elements are present.
1893-1957: Well-designed recursive list processing with proper indentation.The
processListRecursivelyfunction correctly:
- Maintains separate number counters per indent level
- Extracts block elements (tables, code) embedded within list items
- Marks processed nodes to prevent duplicates
js/plugins/code/embed-element.js (3)
64-111: URL conversion logic is sound for common embed services.The
convertToEmbedUrlfunction correctly handles:
- YouTube watch and short URLs → embed format
- Google Drive file view → preview format
- Protocol normalization for all URLs
The iframe sandbox attribute (line 339) provides appropriate security restrictions.
31-56: setValue correctly extracts URLs from iframe embed code.The
extractSrcFromIframehelper properly parses iframe HTML and extracts the src attribute. ThesetValuemethod handles both direct URLs and pasted iframe embed codes.
339-339: Appropriate iframe sandbox configuration.The sandbox attribute permits necessary capabilities (scripts, forms, popups) while maintaining isolation. The
allow-presentationenables fullscreen video playback which is important for YouTube embeds.
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (10)
js/sync/sync.js (1)
131-151: Avoid mutating caller-ownedevent+ trim noisy logging
newChangecurrently mutatesevent.value(timestamp/agent) and logs fulleventLogevery time, which can be expensive and leak document data into consoles. Consider cloning and logging only counts/ids.wisk.sync.newChange = function(event) { @@ - if(!event.value.timestamp) { - event.value.timestamp = Date.now(); - } - if(!event.value.agent) { - event.value.agent = wisk.sync.agent; - } + event = { + ...event, + value: { + ...event.value, + timestamp: event.value.timestamp ?? Date.now(), + agent: event.value.agent ?? wisk.sync.agent, + }, + }; // add to event log wisk.sync.eventLog.push(event); - console.log('New change event logged:', event); - console.log('Current event log:', wisk.sync.eventLog); + console.log('New change event logged:', event.path); + console.log('Current event log size:', wisk.sync.eventLog.length); }js/elements/selector-element.js (2)
56-74: Consider awaitingchangeBlockType()for non-Page selections too
selectButtonis nowasync, but the non-Page path doesn’tawait wisk.editor.changeBlockType(...), so failures/async saves can be dropped.- wisk.editor.changeBlockType(this.elementId, element.getValue(), newDetail.component); + await wisk.editor.changeBlockType(this.elementId, element.getValue(), newDetail.component); this.hide();
76-123: Page ID generation: prefercryptooverMath.random()+ minor log text
Math.random()IDs can collide; usingcrypto.randomUUID()(orcrypto.getRandomValues) is safer. Also the catch message says “child page” but the function iscreatePageAndLink.- const randomId = Math.random().toString(36).substring(2, 12).toUpperCase(); + const randomId = (crypto.randomUUID?.() ?? Math.random().toString(36).slice(2)).replace(/-/g, '').slice(0, 10).toUpperCase(); @@ - console.error('Error creating child page:', error); + console.error('Error creating page:', error);js/plugins/code/embed-element.js (1)
167-174: Consider tightening iframe sandbox / adding referrer policy
Current sandbox allows scripts + same-origin + popups; for arbitrary embeds this is a high-trust posture. Consider least-privilege per supported providers, and addreferrerpolicy.- <iframe sandbox="allow-scripts allow-same-origin allow-forms allow-popups allow-presentation" allowfullscreen></iframe> + <iframe + sandbox="allow-scripts allow-same-origin allow-presentation" + referrerpolicy="strict-origin-when-cross-origin" + allowfullscreen + ></iframe>js/plugins/code/base-text-element.js (1)
1032-1056: Ensure outside-click handler is removed on disconnect
If the component is removed while the paste menu is open,_closePasteMenuHandlerstays ondocument. Add cleanup in your disconnect path.this.disconnectedCallback = () => { observer.disconnect(); this.shadowRoot.removeEventListener('selectionchange', handleSelectionChange); + if (this._closePasteMenuHandler) { + document.removeEventListener('mousedown', this._closePasteMenuHandler); + this._closePasteMenuHandler = null; + } };Also applies to: 527-531
js/editor.js (5)
345-348: Unhandled promise rejection risk in setTimeout.The
setTimeout(async () => { await wisk.sync.saveModification(); }, 0)pattern wraps an async function but doesn't handle potential rejections. IfsaveModificationthrows, the error is silently swallowed.Consider using
.catch()or a try-catch block:- setTimeout(async () => { - await wisk.sync.saveModification(); - }, 0); + setTimeout(() => { + wisk.sync.saveModification().catch(err => { + console.error('Failed to save block creation:', err); + }); + }, 0);
546-565: Element deletion order inconsistency: array filtered beforeapplyEvent.At lines 559-560, the element is filtered out of the data array, then
applyEventis called at line 562. IfapplyEventexpects the element to exist in the array (e.g., for logging or validation), this ordering causes issues.Consider calling
applyEventbefore modifying the local state:if (element) { document.getElementById('editor').removeChild(element); } + wisk.sync.applyEvent(wisk.editor.document, event); wisk.editor.document.data.elements = wisk.editor.document.data.elements.filter(e => e.id !== deletedId); - - wisk.sync.applyEvent(wisk.editor.document, event); }
1130-1155: Consider consolidating multiple events for block type change.Three separate events are emitted for a single logical operation (component, value, lastUpdated). This increases event log size and could cause partial application issues if events are processed individually.
Consider emitting a single compound event or ensuring the sync layer treats these as atomic:
+ // Single event for all changes + wisk.sync.newChange({ + path: `data.elements.${elementId}`, + value: { + data: { + component: newType, + value: value, + lastUpdated: timestamp + }, + timestamp: timestamp, + agent: wisk.sync.agent + } + }); - wisk.sync.newChange({ - path: `data.elements.${elementId}.component`, - ... - }); - wisk.sync.newChange({ - path: `data.elements.${elementId}.value`, - ... - }); - wisk.sync.newChange({ - path: `data.elements.${elementId}.lastUpdated`, - ... - });
2647-2651: Data loss risk if user closes tab during debounce window.The comment at line 2647 correctly identifies that changes may be lost if the user closes the tab before the 1000ms debounce triggers. Consider adding a
beforeunloadevent listener to flush pending updates:window.addEventListener('beforeunload', async (event) => { if (elementUpdatesNeeded.size > 0) { // Trigger immediate save clearTimeout(debounceTimer); // Note: async operations in beforeunload are unreliable // but we can at least try await wisk.sync.saveModification(); } });Alternatively, reduce the debounce timer or use
sendBeaconAPI for more reliable unload persistence.
186-212: Direct state mutation before event emission may cause inconsistency.Lines 190-192, 217-219, and 254-256 directly mutate
wisk.editor.document.data.configbefore emitting the change event. Ifwisk.sync.applyEvent(called elsewhere for incoming events) also attempts to set this value, the state is already modified. This could lead to subtle bugs with conflict resolution or event replay.Consider deferring the initialization to the sync layer or ensuring
applyEventhandles the case where the property already exists.#!/bin/bash # Check how applyEvent handles these paths in sync.js rg -nA 10 'applyEvent' --type=js | head -60
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
js/editor.js(18 hunks)js/elements/selector-element.js(1 hunks)js/plugins/code/base-text-element.js(17 hunks)js/plugins/code/embed-element.js(1 hunks)js/plugins/code/link-element.js(1 hunks)js/sync/sync.js(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
js/plugins/code/base-text-element.js (1)
js/wisk.js (1)
wisk(3-3)
js/plugins/code/link-element.js (2)
js/wisk.js (1)
wisk(3-3)js/editor.js (1)
wrapper(85-85)
js/elements/selector-element.js (2)
js/editor.js (12)
element(522-522)element(555-555)element(768-768)element(902-902)element(1056-1056)element(1086-1086)element(1099-1099)element(1119-1119)element(1189-1189)element(1277-1277)element(1551-1551)element(1696-1696)js/wisk.js (1)
wisk(3-3)
js/sync/sync.js (1)
js/wisk.js (1)
wisk(3-3)
js/editor.js (2)
js/sync/sync.js (3)
elementId(199-199)pathParts(154-154)element(200-200)js/wisk.js (1)
wisk(3-3)
🔇 Additional comments (7)
js/sync/sync.js (1)
298-300: Public API exposure looks consistent
ExportingsaveModification+applyEventonwisk.syncmatches the intended event-driven flow.js/plugins/code/link-element.js (1)
1-48: Static instance registry is a solid fix for shadow-DOM visibility
LinkElement.instances+refreshAllInternalLinks()addresses the “document.querySelectorAll can’t see into shadow roots” limitation.Also applies to: 139-147
js/plugins/code/base-text-element.js (1)
1151-1156: Good: switch-case scoping for Enter
Wrapping theEntercase in a block avoids switch-clause scope leakage.js/editor.js (4)
54-64: LGTM! Touch event handling improvements.The
passive: falseoption is correctly added to allowpreventDefault(), and the guard against triggering select during drag prevents unintended behavior.
123-184: LGTM! Event-driven config changes are well-structured.Each switch case is properly scoped with blocks, the event payload structure is consistent, and persistence is correctly awaited.
270-284: LGTM! Plugin data persistence follows the critical event pattern.Correctly marks plugin data changes as critical and saves immediately.
2269-2300: LGTM! Clipboard sanitization properly handles link security.The function removes all attributes and re-adds only safe ones (
href,contenteditable,target), preventing potential XSS from malformed clipboard data.
| if(elementId === wisk.editor.document.data.elements[0].id) { | ||
| const textContent = domElement.getTextContent().text || ''; | ||
|
|
||
| if(textContent) { | ||
| document.title = textContent; | ||
| wisk.sync.newChange({ | ||
| path: 'data.config.name', | ||
| value: { | ||
| data: textContent, | ||
| timestamp: timestamp, | ||
| agent: wisk.sync.agent | ||
| } | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential null reference when accessing getTextContent().
At line 2698, domElement.getTextContent().text is accessed without checking if getTextContent() returns a valid object. If getTextContent() is undefined or returns an object without text, this will throw.
if(elementId === wisk.editor.document.data.elements[0].id) {
- const textContent = domElement.getTextContent().text || '';
+ const textContentObj = domElement.getTextContent?.();
+ const textContent = textContentObj?.text || '';
if(textContent) {🤖 Prompt for AI Agents
In js/editor.js around lines 2697 to 2711, the call to
domElement.getTextContent().text may throw if getTextContent() returns undefined
or lacks a text property; guard that access by first retrieving the result into
a variable, check that it is an object and has a non-empty text string (or use
optional chaining and a default), then proceed to set document.title and call
wisk.sync.newChange only when text is present; ensure you replace direct chained
access with a safe check (e.g., const tc = domElement.getTextContent(); const
textContent = tc && tc.text ? tc.text : '') and keep the existing behavior when
textContent is falsy.
js/sync/sync.js
Outdated
| function applyEvent(document, event) { | ||
| const pathParts = event.path.split('.'); | ||
| let current = document; | ||
| let i = 0; | ||
|
|
||
| if (event.path === 'data.elementOrder') { | ||
| const newOrder = event.value.data; | ||
|
|
||
| // Store the order | ||
| if (!document.data) document.data = {}; | ||
| document.data.elementOrder = newOrder; | ||
|
|
||
| // Reorder elements array to match | ||
| if (document.data.elements && Array.isArray(document.data.elements)) { | ||
| const orderedElements = []; | ||
| newOrder.forEach(id => { | ||
| const elem = document.data.elements.find(e => e.id === id); | ||
| if (elem) orderedElements.push(elem); | ||
| }); | ||
| document.data.elements = orderedElements; | ||
| } | ||
|
|
||
| document.lastUpdated = event.value.timestamp; | ||
| console.log('Applied element order event'); | ||
| return; | ||
| } |
There was a problem hiding this comment.
data.elementOrder reordering can drop elements (data loss risk)
If newOrder is missing ids, document.data.elements becomes “only the found ones”. Safer to append leftovers (or at least warn) to avoid silent loss.
if (document.data.elements && Array.isArray(document.data.elements)) {
const orderedElements = [];
+ const seen = new Set();
newOrder.forEach(id => {
const elem = document.data.elements.find(e => e.id === id);
- if (elem) orderedElements.push(elem);
+ if (elem) {
+ orderedElements.push(elem);
+ seen.add(id);
+ }
});
+ // Keep any elements not present in newOrder (avoid accidental drops)
+ document.data.elements.forEach(e => {
+ if (e?.id && !seen.has(e.id)) orderedElements.push(e);
+ });
document.data.elements = orderedElements;
}
> Committable suggestion skipped: line range outside the PR's diff.
<details>
<summary>🤖 Prompt for AI Agents</summary>
In js/sync/sync.js around lines 153 to 178, the handler for event.path ===
'data.elementOrder' builds orderedElements only from IDs present in newOrder
which will drop any existing elements whose IDs are missing from newOrder;
change this to track which IDs were added (use a Set), push matching elements
into orderedElements while adding their IDs to the Set, then iterate the
original document.data.elements and append any elements whose id is not in the
Set to preserve leftovers; finally assign document.data.elements =
orderedElements and optionally log a warning if newOrder omitted any IDs so the
operation does not silently lose data.
</details>
<!-- fingerprinting:phantom:medusa:falcon -->
<!-- This is an auto-generated comment by CodeRabbit -->
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
js/editor.js (4)
534-542: Component change still doesn't reapply width classes.This is the same issue flagged in previous reviews. When changing a block's component type, the new DOM element is created without checking if the plugin's
fullWidthproperty requires adding/removing therndr-full-widthclass on the container.Apply this fix to handle width classes on component change:
} else if (property === 'component') { const newType = event.value.data; const newDomElement = document.createElement(newType); newDomElement.id = elementId; domElement.replaceWith(newDomElement); + + // Update container width class based on new plugin type + const container = document.getElementById(`div-${elementId}`); + if (container) { + const pluginDetail = wisk.plugins.getPluginDetail(newType); + if (pluginDetail.width === 'max') { + container.classList.add('rndr-full-width'); + } else { + container.classList.remove('rndr-full-width'); + } + } + setTimeout(() => { newDomElement.setValue('', element.value); }, 0); }
1063-1063: Still updating unuseddeletedElementsLeftarray.As noted in previous reviews, this array is declared (line 4) and updated but never read in active code. It should be removed.
Remove the declaration at line 4 and this push statement to eliminate dead code.
2526-2593: Remove commented-out legacy code.As flagged in previous reviews, this commented-out
justUpdatesimplementation should be removed now that the new event-driven version is in place.Delete lines 2526-2593 to clean up the codebase.
2698-2698: Potential null reference accessinggetTextContent().As noted in previous reviews, this line accesses
getTextContent().textwithout checking ifgetTextContent()returns a valid object.Apply this fix:
- const textContent = domElement.getTextContent().text || ''; + const textContentObj = domElement.getTextContent?.(); + const textContent = textContentObj?.text || '';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
js/editor.js(18 hunks)
🔇 Additional comments (8)
js/editor.js (8)
123-184: LGTM! Event-driven config changes are well-structured.The function correctly wraps each switch case in blocks (addressing scoping), creates timestamped events, and immediately persists changes via
saveModification. The async/await pattern is properly applied.
186-268: LGTM! New public APIs follow consistent event-driven pattern.The four new APIs (
addUserAccess,removeUserAccess,setPublicStatus,setDatabaseProp) consistently create events with timestamps/agent metadata and immediately persist viasaveModification. Defensive initialization and duplicate-checking are appropriate.
412-487: LGTM! Event processing logic is well-structured.The priority-ordered processing (deletions → creations → updates → config/plugins → order) prevents common edge cases. Grouping events by type before processing is efficient and clear.
1429-1456: LGTM! Inline markdown processing order is well-documented.The explicit ordering (citations → code → formatting → links → references) with inline comments prevents conflicts between different markdown patterns.
54-64: LGTM! Touch event handling improved.The
touchendhandler properly distinguishes between tap (select) and hold-drag interactions. Setting{ passive: false }ontouchstartis necessary forpreventDefault()to work.
964-974: LGTM! Switch case properly wrapped in block.The
supcase is now wrapped in a block to prevent scope leakage ofconstdeclarations to other cases, addressing the previous review comment.
2269-2300: The aggressive attribute removal is appropriate for this clipboard context.The function removes all attributes before re-adding only
href,contenteditable, andtarget. This is correct behavior for clipboard operations because:
- Clipboard data should not include application-internal state (
data-*attributes)- Security attributes like
rel="noopener noreferrer"are UI concerns, not clipboard data- Custom attributes would corrupt the clipboard JSON (which is why Base64 encoding is used elsewhere in this change)
No custom attributes need to be preserved in clipboard text.
345-347: Review the setTimeout with async callback pattern.Using
setTimeoutwith an async callback for critical persistence operations can lead to race conditions or lost updates if the user navigates away before the timeout fires. Consider either:
- Awaiting
saveModificationdirectly (blocking)- Using a non-blocking queue that guarantees eventual persistence
- Documenting why the delay is necessary
js/editor.js
Outdated
| // debounce: creating events when typing stops. (what if the user closes the tab before debounce triggers???) | ||
|
|
||
| clearTimeout(debounceTimer); | ||
|
|
||
| debounceTimer = setTimeout(async () => { | ||
| const changed = elementUpdatesLeft | ||
| .map(elementId => { | ||
| const element = wisk.editor.getElement(elementId); | ||
| if (element) { | ||
| return { | ||
| path: 'document.elements', | ||
| values: { | ||
| id: element.id, | ||
| value: element.value, | ||
| lastEdited: element.lastEdited, | ||
| component: element.component, | ||
| }, | ||
| }; | ||
| console.log('typing stopped, creating events for: ', elementUpdatesNeeded.size, 'elements'); | ||
|
|
||
| const timestamp = Date.now(); | ||
|
|
||
| // create events for all elements needing update | ||
| elementUpdatesNeeded.forEach(elementId => { | ||
| const domElement = document.getElementById(elementId); | ||
| if(!domElement) { | ||
| return; | ||
| } | ||
|
|
||
| const elementValue = domElement.getValue(); | ||
| const componentName = domElement.tagName.toLowerCase(); | ||
|
|
||
| // create event | ||
| wisk.sync.newChange({ | ||
| path: `data.elements.${elementId}.value`, | ||
| value: { | ||
| data: elementValue, | ||
| timestamp: timestamp, | ||
| agent: wisk.sync.agent | ||
| } | ||
| return null; | ||
| }) | ||
| .filter(Boolean); | ||
| }); | ||
|
|
||
| const elementIds = wisk.editor.document.data.elements.map(e => e.id); | ||
| // component because they can be changed (changeBlockType) | ||
|
|
||
| await wisk.sync.saveUpdates(); | ||
| wisk.sync.newChange({ | ||
| path: `data.elements.${elementId}.component`, | ||
| value: { | ||
| data: componentName, | ||
| timestamp: timestamp, | ||
| agent: wisk.sync.agent | ||
| } | ||
| }); | ||
|
|
||
| wisk.sync.newChange({ | ||
| path: `data.elements.${elementId}.lastUpdated`, | ||
| value: { | ||
| data: timestamp, | ||
| timestamp: timestamp, | ||
| agent: wisk.sync.agent | ||
| } | ||
| }); | ||
|
|
||
| elementUpdatesLeft = []; | ||
| deletedElementsLeft = []; | ||
| }, elementSyncTimer); // should it be less? to voice your opinion, join our discord server: https://discord.gg/D8tQCvgDhu | ||
| // special case: first element | ||
| if(elementId === wisk.editor.document.data.elements[0].id) { | ||
| const textContent = domElement.getTextContent().text || ''; | ||
|
|
||
| if(textContent) { | ||
| document.title = textContent; | ||
| wisk.sync.newChange({ | ||
| path: 'data.config.name', | ||
| value: { | ||
| data: textContent, | ||
| timestamp: timestamp, | ||
| agent: wisk.sync.agent | ||
| } | ||
| }); | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| // save to indexedDB locally | ||
| await wisk.sync.saveModification(); | ||
|
|
||
| // TODO: Send only the events to server (future step) | ||
| // await sendEventsToServer(wisk.sync.eventLog); | ||
|
|
||
| // Clear the set | ||
| elementUpdatesNeeded.clear(); | ||
| }, 300); |
There was a problem hiding this comment.
Debounce timing creates data loss risk.
The 300ms debounce means changes aren't persisted until typing stops. If a user closes the tab or loses connection before the debounce fires, updates are lost. The inline comment (line 2647) acknowledges this concern.
Consider:
- Reducing debounce to 100-150ms
- Using
beforeunloadto flush pending updates - Implementing a visibility change handler to persist on tab switch
Would you like me to generate a visibility change handler to address this risk?
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
js/editor.js (1)
1414-1441: Critical XSS/URL-injection risk inconvertInlineMarkdown()
You’re constructing HTML strings with user-controlled content (<a href="$2">,<code>$1</code>,<cite-element reference-id="...">) without escaping/sanitizing. This enablesjavascript:URLs and attribute-breaking payloads.function convertInlineMarkdown(text) { + const escapeHtml = (s) => + String(s).replace(/[&<>"']/g, (c) => ({'&':'&','<':'<','>':'>','"':'"',"'":'''}[c])); + const escapeAttr = escapeHtml; + const sanitizeHref = (raw) => { + try { + const u = new URL(raw, window.location.origin); + if (u.protocol === 'http:' || u.protocol === 'https:' || u.protocol === 'mailto:') return u.href; + } catch {} + return ''; + }; // Citation elements (must be before links to avoid conflicts) - text = text.replace(/--citation-element--([^-]+)--/g, (match, referenceId) => { - return `<cite-element reference-id="${referenceId}"></cite-element>`; - }); + text = text.replace(/--citation-element--([^-]+)--/g, (_m, referenceId) => { + const safeId = String(referenceId).replace(/[^a-zA-Z0-9_-]/g, ''); + return safeId ? `<cite-element reference-id="${escapeAttr(safeId)}"></cite-element>` : ''; + }); // Inline code (must be before other formatting to preserve code content) - text = text.replace(/`([^`]+)`/g, '<code>$1</code>'); + text = text.replace(/`([^`]+)`/g, (_m, code) => `<code>${escapeHtml(code)}</code>`); ... // Links (must be before reference numbers) - text = text.replace(/\[([^\]]+)\]\(([^)]+)\)/g, '<a href="$2" contenteditable="false" target="_blank">$1</a>'); + text = text.replace(/\[([^\]]+)\]\(([^)]+)\)/g, (_m, label, href) => { + const safeHref = sanitizeHref(href); + const safeLabel = escapeHtml(label); + if (!safeHref) return safeLabel; // fallback to text if URL is unsafe + return `<a href="${escapeAttr(safeHref)}" contenteditable="false" target="_blank" rel="noopener noreferrer">${safeLabel}</a>`; + });
♻️ Duplicate comments (3)
js/editor.js (3)
1013-1056:deletedElementsLeftstill looks unused
This variable is written indeleteBlock()(deletedElementsLeft.push(...)) but I don’t see a corresponding read in the provided context; if it’s truly unused, remove it to avoid misleading state.
2511-2637: GuardgetTextContent()access insidejustUpdates()
domElement.getTextContent().textwill throw ifgetTextContentis missing or returns nullish; a defensive read avoids breaking event generation for the “main element” title.- const textContent = domElement.getTextContent().text || ''; + const tc = domElement.getTextContent?.(); + const textContent = tc?.text || '';
501-528: Remotecomponentupdates must also updaterndr-full-widthclass
changeBlockType()updates the container width class, buthandleElementUpdate()(event-driven path) does not—so remote type changes can leave stale layout.} else if (property === 'component') { const newType = event.value.data; const newDomElement = document.createElement(newType); newDomElement.id = elementId; domElement.replaceWith(newDomElement); + + // Keep width class consistent with new plugin type + const container = document.getElementById(`div-${elementId}`); + if (container) { + const pluginDetail = wisk.plugins.getPluginDetail(newType); + if (pluginDetail?.width === 'max') container.classList.add('rndr-full-width'); + else container.classList.remove('rndr-full-width'); + } + setTimeout(() => { newDomElement.setValue('', element.value); }, 0); }
🧹 Nitpick comments (5)
js/plugins/code/embed-element.js (2)
16-23: Avoid accumulating duplicate listeners on re-connect
bindEvents()usesaddEventListenereachconnectedCallback(); if the element is detached/reattached, listeners can stack. Consider guarding with a flag or removing indisconnectedCallback().
65-90: Remove or gate debug logging
Theconsole.logcalls insetValue()will be noisy in production. Consider awisk.debugflag or removing them before merge.js/plugins/code/link-element.js (2)
31-37: Prevent render storms fromsetValue()/ attribute changes
setValue()calls multiplesetAttribute()s, andattributeChangedCallback()re-renders each time → up to 4 renders per update. Consider batching via a microtask (queueMicrotask) or a_suspendRendersflag while applying attributes.Also applies to: 138-148
1-21: Check consistency with theBaseTextElementcontract used acrossjs/plugins/code/*
This is a standaloneHTMLElementwith its own shadow DOM and event wiring; if it’s intended to behave like other editor “text-ish” plugins, consider whether any shared focus/value/render contract belongs in a base class. Based on learnings, keep base-vs-derived responsibilities consistent across plugin elements.Also applies to: 193-275
js/editor.js (1)
2224-2227: Consider replacingescape/unescapebase64 UTF-8 conversions
decodeURIComponent(escape(atob(...)))andbtoa(unescape(encodeURIComponent(...)))are legacy patterns and can be brittle with Unicode. ATextEncoder/TextDecoderpath would be more robust.Also applies to: 2335-2338
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
js/editor.js(18 hunks)js/plugins/code/embed-element.js(1 hunks)js/plugins/code/link-element.js(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-12T14:14:36.148Z
Learnt from: sushilbang
Repo: sohzm/wisk PR: 54
File: js/plugins/code/base-text-element.js:58-62
Timestamp: 2025-12-12T14:14:36.148Z
Learning: In the js/plugins/code directory, ensure that BaseTextElement serves as the shared base class with common state/logic, while concrete elements (e.g., TextElement) extend it and override render() to provide UI markup. The base class's connectedCallback should query for child elements that are injected by their render methods. This inheritance pattern should be consistently applied across the plugin system; review new or existing elements to maintain a clean base-vs-extension separation, avoid tight coupling, and verify that the render contracts and DOM querying align between base and derived classes.
Applied to files:
js/plugins/code/embed-element.jsjs/plugins/code/link-element.js
🧬 Code graph analysis (1)
js/plugins/code/link-element.js (2)
js/wisk.js (1)
wisk(3-3)js/editor.js (6)
wrapper(85-85)prevElement(308-308)nextElement(1445-1445)event(1717-1717)event(1787-1787)img(75-75)
🪛 Biome (2.1.2)
js/plugins/code/embed-element.js
[error] 39-39: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 47-47: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
js/plugins/code/link-element.js
[error] 291-291: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 299-299: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🔇 Additional comments (4)
js/plugins/code/embed-element.js (1)
176-225: Sandbox is a good baseline, but double-check required permissions
sandbox="allow-scripts allow-same-origin allow-forms allow-popups allow-presentation"is fairly permissive (especiallyallow-same-origin). Make sure each capability is actually needed, otherwise tighten to reduce embed risk.js/plugins/code/link-element.js (1)
347-510: Icon rendering looks safe (noinnerHTMLinjection)
Building emoji viatextContentand images viaimg.srcproperties is the right direction for avoiding XSS.js/editor.js (2)
412-472: Event grouping inhandleChanges()looks sensible
Clear separation of creations/updates/deletions/order/config/pluginData improves determinism and keeps DOM work localized.
1092-1165: Nice:changeBlockType()now updates width class and persists via events
The DOM swap +rndr-full-widthclass sync makes component changes behave consistently.
| handleBlockKeyDown(event) { | ||
| switch (event.key) { | ||
| case 'Backspace': | ||
| case 'Delete': | ||
| event.preventDefault(); | ||
| wisk.editor.deleteBlock(this.id); | ||
| break; | ||
| case 'Enter': | ||
| event.preventDefault(); | ||
| wisk.editor.createNewBlock(this.id, 'text-element', { textContent: '' }, { x: 0 }); | ||
| break; | ||
| case 'ArrowUp': | ||
| case 'ArrowLeft': | ||
| event.preventDefault(); | ||
| const prevElement = wisk.editor.prevElement(this.id); | ||
| if (prevElement) { | ||
| wisk.editor.focusBlock(prevElement.id, { x: prevElement.value?.textContent?.length || 0 }); | ||
| } | ||
| break; | ||
| case 'ArrowDown': | ||
| case 'ArrowRight': | ||
| event.preventDefault(); | ||
| const nextElement = wisk.editor.nextElement(this.id); | ||
| if (nextElement) { | ||
| wisk.editor.focusBlock(nextElement.id, { x: 0 }); | ||
| } | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix switch case declarations (noSwitchDeclarations)
Same Biome issue as embed-element: wrap the declaration cases in {} blocks.
handleBlockKeyDown(event) {
switch (event.key) {
...
case 'ArrowUp':
case 'ArrowLeft':
- event.preventDefault();
- const prevElement = wisk.editor.prevElement(this.id);
- if (prevElement) {
- wisk.editor.focusBlock(prevElement.id, { x: prevElement.value?.textContent?.length || 0 });
- }
- break;
+ {
+ event.preventDefault();
+ const prevElement = wisk.editor.prevElement(this.id);
+ if (prevElement) {
+ wisk.editor.focusBlock(prevElement.id, { x: prevElement.value?.textContent?.length || 0 });
+ }
+ break;
+ }
case 'ArrowDown':
case 'ArrowRight':
- event.preventDefault();
- const nextElement = wisk.editor.nextElement(this.id);
- if (nextElement) {
- wisk.editor.focusBlock(nextElement.id, { x: 0 });
- }
- break;
+ {
+ event.preventDefault();
+ const nextElement = wisk.editor.nextElement(this.id);
+ if (nextElement) {
+ wisk.editor.focusBlock(nextElement.id, { x: 0 });
+ }
+ break;
+ }
}
}🧰 Tools
🪛 Biome (2.1.2)
[error] 291-291: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 299-299: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🤖 Prompt for AI Agents
In js/plugins/code/link-element.js around lines 277 to 305, the switch has const
declarations inside case clauses which violates the noSwitchDeclarations rule;
wrap the ArrowUp/ArrowLeft case body and the ArrowDown/ArrowRight case body in
their own block braces ({ ... }) so the const prevElement and const nextElement
are block-scoped, keeping event.preventDefault() and the existing logic inside
those new blocks.
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (3)
js/plugins/code/link-element.js (1)
277-305: Fix BiomenoSwitchDeclarationsinhandleBlockKeyDown(wrap cases in blocks).
This is the same issue previously flagged for switch-case scoping; it still exists here and will fail Biome.handleBlockKeyDown(event) { switch (event.key) { @@ case 'ArrowUp': case 'ArrowLeft': - event.preventDefault(); - const prevElement = wisk.editor.prevElement(this.id); - if (prevElement) { - wisk.editor.focusBlock(prevElement.id, { x: prevElement.value?.textContent?.length || 0 }); - } - break; + { + event.preventDefault(); + const prevElement = wisk.editor.prevElement(this.id); + if (prevElement) { + wisk.editor.focusBlock(prevElement.id, { x: prevElement.value?.textContent?.length || 0 }); + } + break; + } case 'ArrowDown': case 'ArrowRight': - event.preventDefault(); - const nextElement = wisk.editor.nextElement(this.id); - if (nextElement) { - wisk.editor.focusBlock(nextElement.id, { x: 0 }); - } - break; + { + event.preventDefault(); + const nextElement = wisk.editor.nextElement(this.id); + if (nextElement) { + wisk.editor.focusBlock(nextElement.id, { x: 0 }); + } + break; + } } }js/sync/sync.js (1)
153-169: Data-loss risk:data.elementOrdercan drop elements not listed innewOrder.
IfnewOrderis partial (or stale),document.data.elementsbecomes only “found ones”. Append leftovers (or at least warn) to avoid silent deletion.js/editor.js (1)
2498-2622: GuardgetTextContent()access injustUpdates(can throw) + consider unload flush for debounce.
This line can throw if a component doesn’t implementgetTextContent()or returns nullish:
domElement.getTextContent().textSuggested minimal hardening:
- const textContent = domElement.getTextContent().text || ''; + const tc = domElement.getTextContent?.(); + const textContent = tc?.text || '';Also: the comment about “flush pending updates on unload” is real—worth implementing
visibilitychange/pagehideflushing so debounced updates aren’t lost.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
js/editor.js(18 hunks)js/plugins/code/embed-element.js(1 hunks)js/plugins/code/link-element.js(1 hunks)js/plugins/code/link-preview-element.js(4 hunks)js/plugins/plugin-data.json(2 hunks)js/sync/sync.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- js/plugins/plugin-data.json
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-12T14:14:36.148Z
Learnt from: sushilbang
Repo: sohzm/wisk PR: 54
File: js/plugins/code/base-text-element.js:58-62
Timestamp: 2025-12-12T14:14:36.148Z
Learning: In the js/plugins/code directory, ensure that BaseTextElement serves as the shared base class with common state/logic, while concrete elements (e.g., TextElement) extend it and override render() to provide UI markup. The base class's connectedCallback should query for child elements that are injected by their render methods. This inheritance pattern should be consistently applied across the plugin system; review new or existing elements to maintain a clean base-vs-extension separation, avoid tight coupling, and verify that the render contracts and DOM querying align between base and derived classes.
Applied to files:
js/plugins/code/link-element.jsjs/plugins/code/link-preview-element.jsjs/plugins/code/embed-element.js
🧬 Code graph analysis (5)
js/sync/sync.js (2)
js/wisk.js (1)
wisk(3-3)js/editor.js (22)
pathParts(502-502)pathParts(589-589)i(723-723)i(854-854)i(1225-1225)elementId(364-364)elementId(378-378)elementId(398-398)elementId(503-503)elementId(2512-2512)element(506-506)element(539-539)element(724-724)element(855-855)element(1007-1007)element(1035-1035)element(1048-1048)element(1068-1068)element(1140-1140)element(1228-1228)element(1502-1502)element(1647-1647)
js/plugins/code/link-element.js (2)
js/wisk.js (1)
wisk(3-3)js/editor.js (2)
prevElement(308-308)nextElement(1411-1411)
js/editor.js (1)
js/sync/sync.js (4)
elementId(189-189)key(184-184)pathParts(149-149)element(190-190)
js/plugins/code/link-preview-element.js (2)
js/editor.js (2)
prevElement(308-308)nextElement(1411-1411)js/wisk.js (1)
wisk(3-3)
js/plugins/code/embed-element.js (2)
js/editor.js (4)
event(1683-1683)event(1753-1753)prevElement(308-308)nextElement(1411-1411)js/wisk.js (1)
wisk(3-3)
🪛 Biome (2.1.2)
js/plugins/code/link-element.js
[error] 291-291: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 299-299: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🔇 Additional comments (3)
js/plugins/code/link-element.js (1)
1-29: Good: static instance registry fixes shadow-DOM discovery for refreshAllInternalLinks.
This resolves the “document.querySelectorAll('link-element')misses shadow DOM” class of bugs by tracking instances inconnectedCallback/disconnectedCallback.js/plugins/code/embed-element.js (1)
143-205: Embed URL normalization + view toggling looks consistent.
convertToEmbedUrl()+updateView()is straightforward and avoids showing the iframe for empty input.js/editor.js (1)
55-64: Good hardening: sanitizing link protocols in clipboard HTML.
The safe-protocol allowlist + removal of dangeroushrefvalues addresses thejavascript:/data:footgun for pasted/copied HTML.Also applies to: 2220-2270
| wisk.editor.handleChanges = async function (eventPackage) { | ||
| if (!eventPackage) return; | ||
|
|
||
| const events = Array.isArray(eventPackage.events) ? eventPackage.events : []; | ||
|
|
||
| // Group events by type for efficient processing | ||
| const elementValueUpdates = []; | ||
| const elementComponentUpdates = []; | ||
| const elementCreations = []; | ||
| const elementDeletions = []; | ||
| const elementOrderChanges = []; | ||
| const configChanges = []; | ||
| const pluginDataChanges = []; | ||
|
|
||
| events.forEach(event => { | ||
| if (event.path.startsWith('data.elements.')) { | ||
| if (event.path.includes('.value')) { | ||
| elementValueUpdates.push(event); | ||
| } else if (event.path.includes('.component')) { | ||
| elementComponentUpdates.push(event); | ||
| } | ||
| } | ||
| if (change.path.startsWith('document.config.plugins')) { | ||
| if (change.path.includes('add')) { | ||
| wisk.plugins.loadPlugin(change.values.plugin); | ||
| } | ||
| if (change.path.includes('remove')) { | ||
| window.location.reload(); | ||
| } | ||
| else if (event.path === 'data.elements') { | ||
| elementCreations.push(event); | ||
| } | ||
| if (change.path.startsWith('document.config.theme')) { | ||
| wisk.theme.setTheme(change.values.theme); | ||
| else if (event.path === 'data.deletedElements') { | ||
| elementDeletions.push(event); | ||
| } | ||
| if (change.path.startsWith('document.plugin')) { | ||
| if (change.values.data) { | ||
| document.getElementById(change.path.replace('document.plugin.', '')).loadData(change.values.data); | ||
| } | ||
| else if (event.path === 'data.elementOrder') { | ||
| elementOrderChanges.push(event); | ||
| } | ||
| } | ||
| else if (event.path.startsWith('data.config')) { | ||
| configChanges.push(event); | ||
| } | ||
| else if (event.path.startsWith('data.pluginData')) { | ||
| pluginDataChanges.push(event); | ||
| } | ||
| }); | ||
|
|
||
| // Handle reordering only if necessary | ||
| if (allElements.length > 0) { | ||
| smartReorderElements(allElements); | ||
| await handleElementDeletions(elementDeletions); | ||
| for (const event of elementCreations) { | ||
| await handleElementCreation(event); | ||
| } | ||
| for (const event of elementValueUpdates) { | ||
| await handleElementUpdate(event); | ||
| } | ||
| for (const event of elementComponentUpdates) { | ||
| await handleElementUpdate(event); | ||
| } | ||
| for (const event of configChanges) { | ||
| handleConfigChange(event); | ||
| } | ||
| for (const event of pluginDataChanges) { | ||
| handlePluginDataChange(event); | ||
| } | ||
| if (elementOrderChanges.length > 0) { | ||
| const lastOrderEvent = elementOrderChanges[elementOrderChanges.length - 1]; | ||
| smartReorderElements(lastOrderEvent.value.data); | ||
| wisk.sync.applyEvent(wisk.editor.document, lastOrderEvent); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Remote apply path misses data.elements.<id>.lastUpdated events.
handleChanges() only routes .value and .component under data.elements.*; lastUpdated events are currently ignored. If those timestamps matter for conflict resolution or UI, they should either be applied via wisk.sync.applyEvent(...) in this path or explicitly handled.
🤖 Prompt for AI Agents
In js/editor.js around lines 411 to 471, the event routing ignores
data.elements.<id>.lastUpdated events so those timestamp updates are never
applied; detect events whose path startsWith 'data.elements.' and includes
'.lastUpdated' and ensure they are applied to the local document (either route
them to the same handler as value/component updates or call
wisk.sync.applyEvent(wisk.editor.document, event) directly). Update the forEach
grouping logic to push '.lastUpdated' events to an appropriate list (or call
applyEvent immediately) and then apply them in the processing phase so
lastUpdated timestamps are reflected locally.
| get isInternal() { | ||
| if (!this.url) return false; | ||
| if (this.url.startsWith('https://app.wisk.cc')) return true; | ||
|
|
||
| try { | ||
| const urlObj = new URL(this.url); | ||
| const currentOrigin = window.location.origin; | ||
|
|
||
| if (urlObj.origin === currentOrigin && urlObj.searchParams.has('id')) return true; | ||
| } catch {} | ||
|
|
||
| return false; | ||
| } |
There was a problem hiding this comment.
isInternal / URL parsing likely breaks for relative internal URLs; also fix window.open tabnabbing.
new URL(this.url)throws for relative URLs (e.g.?id=...//page?...), soisInternalbecomes false andopenLink()will incorrectly prefixhttps://and open an invalid external URL.window.open(url, '_blank')should includenoopener(and ideallynoreferrer) to preventwindow.openerattacks.
Proposed diff (use a base URL + noopener):
get isInternal() {
if (!this.url) return false;
if (this.url.startsWith('https://app.wisk.cc')) return true;
try {
- const urlObj = new URL(this.url);
+ const urlObj = new URL(this.url, window.location.origin);
const currentOrigin = window.location.origin;
if (urlObj.origin === currentOrigin && urlObj.searchParams.has('id')) return true;
} catch {}
return false;
}
async fetchInternalPageTitle() {
@@
try {
- const urlObj = new URL(this.url);
+ const urlObj = new URL(this.url, window.location.origin);
const pageId = urlObj.searchParams.get('id');
@@
async updateLinkedPageTitle(newTitle) {
try {
- const urlObj = new URL(this.url);
+ const urlObj = new URL(this.url, window.location.origin);
const pageId = urlObj.searchParams.get('id');
@@
openLink() {
@@
} else {
let url = this.url;
if (!url.startsWith('http://') && !url.startsWith('https://')) {
url = 'https://' + url;
}
- window.open(url, '_blank');
+ window.open(url, '_blank', 'noopener,noreferrer');
}
}Also applies to: 69-112, 326-339
There was a problem hiding this comment.
@sushilbang, understood! I'll leave this as-is. Let me know if you'd like me to look at anything else in the PR.
(\(\
( -.-)
o_(")(")
🧠 Learnings used
Learnt from: sushilbang
Repo: sohzm/wisk PR: 54
File: js/plugins/code/base-text-element.js:58-62
Timestamp: 2025-12-12T14:14:36.148Z
Learning: In js/plugins/code/, BaseTextElement provides shared state and logic while concrete elements like TextElement extend it and override render() to add UI markup. The base class's connectedCallback queries for elements that child classes inject via their render methods. This is an intentional inheritance pattern used throughout the plugin system.
| this.handleClick = (event) => { | ||
| if (this.link && !event.target.closest('.input-dialog')) { | ||
| let url = this.link; | ||
| if (!url.startsWith('http://') && !url.startsWith('https://')) { | ||
| url = 'https://' + url; | ||
| } | ||
| window.open(url, '_blank'); | ||
| } |
There was a problem hiding this comment.
Harden external navigation + fetch reliability (noopener + timeout).
- Add
noopener,noreferrertowindow.open. - Consider an AbortController timeout for
fetch()so “loading” can’t hang forever.
- window.open(url, '_blank');
+ window.open(url, '_blank', 'noopener,noreferrer');(And add an abort timeout around the metadata fetch.)
Also applies to: 290-331
🤖 Prompt for AI Agents
In js/plugins/code/link-preview-element.js around lines 39-46 (and similarly
around 290-331), external navigation uses window.open without rel flags and the
metadata fetch can hang; update the window.open call to include the feature
string 'noopener,noreferrer' to prevent the new window from accessing the
opener, and wrap metadata fetches in an AbortController with a short timeout:
create an AbortController, pass controller.signal to fetch, start a setTimeout
that calls controller.abort() after the timeout, handle the abort (ignore or
surface a timeout error) and clear the timer on success or component teardown to
avoid leaks; apply these changes to both occurrences noted.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
js/plugins/code/link-preview-element.js (1)
301-342: Add timeout to metadata fetch to prevent indefinite loading state.The
fetch()call lacks anAbortControllertimeout, so if the request torender.wisk.cchangs, the preview remains in the "Loading preview..." state indefinitely. Consider adding a 5-10 second timeout.async updateLinkPreview() { if (!this.link || !this.link.trim() || this.metadata) return; if (this.status === 'loading' || this.status === 'ok' || this.status === 'error') return; this.status = 'loading'; this.showLoadingState(); this.sendUpdates(); + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), 8000); + try { let url = this.link; if (!url.startsWith('http://') && !url.startsWith('https://')) { url = 'https://' + url; } const response = await fetch('https://render.wisk.cc/fetch-metadata', { method: 'POST', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({ url }), + signal: controller.signal, }); + clearTimeout(timeoutId); + if (!response.ok) { throw new Error('Failed to fetch metadata'); } const metadata = await response.json(); if (metadata.error) { throw new Error(metadata.error); } this.metadata = metadata; this.status = 'ok'; this.updatePreviewWithMetadata(metadata); this.sendUpdates(); } catch (error) { - console.error('Error fetching metadata:', error); + clearTimeout(timeoutId); + console.error('Error fetching metadata:', error); this.metadata = null; this.status = 'error'; this.showErrorState(); this.sendUpdates(); } }js/sync/sync.js (1)
176-192: Element reordering can still drop elements not in newOrder.The current logic only includes elements whose IDs appear in
newOrder(lines 182-186). Any existing element missing fromnewOrderwill be silently dropped fromdocument.data.elements, causing data loss.Apply this fix to preserve elements not in newOrder:
if (event.path === 'data.elementOrder') { const newOrder = event.value.data; if (!document.data) document.data = {}; document.data.elementOrder = newOrder; if (document.data.elements && Array.isArray(document.data.elements)) { const orderedElements = []; + const seen = new Set(); newOrder.forEach(id => { const elem = document.data.elements.find(e => e.id === id); - if (elem) orderedElements.push(elem); + if (elem) { + orderedElements.push(elem); + seen.add(id); + } }); + // Preserve elements not in newOrder to avoid data loss + document.data.elements.forEach(e => { + if (e?.id && !seen.has(e.id)) { + orderedElements.push(e); + console.warn(`Element ${e.id} not in newOrder, appending to end`); + } + }); document.data.elements = orderedElements; } document.lastUpdated = event.value.timestamp; console.log('Applied element order event'); return; }js/editor.js (2)
417-602: Remote lastUpdated events are not applied locally.The event routing in
handleChanges(lines 431-454) only groups events matching.valueor.component. Events with pathdata.elements.<id>.lastUpdateddon't match either condition and are ignored. Similarly,handleElementUpdate(lines 507-533) only handles'value'and'component'properties.To fix, add lastUpdated handling:
events.forEach(event => { if (event.path.startsWith('data.elements.')) { if (event.path.includes('.value')) { elementValueUpdates.push(event); } else if (event.path.includes('.component')) { elementComponentUpdates.push(event); + } else if (event.path.includes('.lastUpdated')) { + // Apply timestamp updates via applyEvent + wisk.sync.applyEvent(wisk.editor.document, event); } } // ... });
2596-2611: Guard getTextContent() call against undefined return value.At line 2597,
domElement.getTextContent().textassumesgetTextContent()returns an object with atextproperty. If the method returnsundefinedor an object withouttext, this will throw a runtime error.Apply safe access:
// special case: first element if(elementId === wisk.editor.document.data.elements[0].id) { - const textContent = domElement.getTextContent().text || ''; + const textContentObj = domElement.getTextContent?.(); + const textContent = textContentObj?.text || ''; if(textContent) { document.title = textContent; wisk.sync.newChange({ path: 'data.config.name', value: { data: textContent, timestamp: timestamp, agent: wisk.sync.agent } }); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
js/editor.js(18 hunks)js/plugins/code/embed-element.js(1 hunks)js/plugins/code/link-preview-element.js(4 hunks)js/sync/sync.js(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-12T14:14:36.148Z
Learnt from: sushilbang
Repo: sohzm/wisk PR: 54
File: js/plugins/code/base-text-element.js:58-62
Timestamp: 2025-12-12T14:14:36.148Z
Learning: In the js/plugins/code directory, ensure that BaseTextElement serves as the shared base class with common state/logic, while concrete elements (e.g., TextElement) extend it and override render() to provide UI markup. The base class's connectedCallback should query for child elements that are injected by their render methods. This inheritance pattern should be consistently applied across the plugin system; review new or existing elements to maintain a clean base-vs-extension separation, avoid tight coupling, and verify that the render contracts and DOM querying align between base and derived classes.
Applied to files:
js/plugins/code/link-preview-element.jsjs/plugins/code/embed-element.js
🧬 Code graph analysis (2)
js/sync/sync.js (2)
js/wisk.js (1)
wisk(3-3)js/editor.js (7)
event(1683-1683)event(1753-1753)pathParts(508-508)pathParts(595-595)i(723-723)i(854-854)i(1225-1225)
js/editor.js (2)
js/sync/sync.js (4)
elementId(212-212)key(207-207)pathParts(172-172)element(213-213)js/wisk.js (1)
wisk(3-3)
🔇 Additional comments (12)
js/plugins/code/link-preview-element.js (3)
27-108: LGTM! Event listener leak issue resolved.The refactored event binding now stores handlers as properties (
this._onOuterClick,this._onUrlInputKeyDown, etc.) and properly removes them in_removeAllListeners()anddisconnectedCallback(). This addresses the previous concern about anonymous handlers stacking on detach/reattach.
177-217: LGTM! setValue logic correctly handles append and full updates.The implementation properly:
- Concatenates text for
value.appendpath- Resets metadata/status when the link changes (old metadata becomes invalid)
- Guards fetch until
_hasConnectedis true (avoids premature network calls)- Syncs status to
'ok'when metadata is present
252-299: LGTM! Metadata rendering is robust with proper guards.The function safely handles partial or missing metadata:
- All DOM elements are checked before updating
- Missing description/meta information is hidden (
display: 'none')- Favicon load failures are caught with
onerrorhandler- Date formatting uses standard
Dateconstructorjs/plugins/code/embed-element.js (4)
9-83: LGTM! Lifecycle and event cleanup properly implemented.The component now:
- Queries DOM elements in
connectedCallback- Stores event handlers as properties for later removal
- Implements symmetric add/remove via
_removeAllListeners- Calls cleanup in
disconnectedCallbackThis resolves the previously flagged event listener leak issue.
94-124: LGTM! Switch case declarations now properly scoped.The block scoping for
prevElementandnextElement(lines 106-113 and 115-122) addresses the previousnoSwitchDeclarationslint error. Variables are now isolated to their respective cases.
126-206: LGTM! URL extraction and transformation logic is sound.The implementation correctly:
- Parses iframe HTML with
DOMParser(safe)- Extracts video IDs from YouTube watch and short URLs
- Transforms Google Drive file URLs to preview format
- Adds protocol prefix where missing
- Provides safe fallback for unknown URL types
208-229: LGTM! View toggle logic correctly manages two-pane UI.The function:
- Guards against missing DOM elements
- Properly toggles between input dialog and iframe display
- Strips protocol and transforms URL before setting iframe.src
- Prevents setting invalid URLs (line 219 guard)
js/sync/sync.js (2)
8-29: LGTM! Save queue correctly serializes persistence operations.The
enqueueSaveimplementation:
- Uses a
saveScheduledflag to coalesce rapid requests- Chains saves via
Promiseto ensure sequential execution- Catches errors without breaking the queue
This addresses the previously flagged risk of concurrent
saveModification()calls overlapping and losing events.
260-306: LGTM! Snapshot-and-drain pattern prevents event loss during persistence.The implementation correctly:
- Snapshots
eventLogand clears it upfront (lines 276-277)- Applies only the snapshot, avoiding interference from new events during
await- Restores events to the queue on persistence failure (line 303)
- Wraps persistence in try/catch to handle errors gracefully
This addresses the previously flagged risk of concurrent
saveModification()calls losing events.js/editor.js (3)
250-274: LGTM! Key sanitization prevents path injection.The validation at line 252 correctly rejects keys containing
.,[, or], preventing malicious keys from mutating unintended nested paths. This addresses the previously flagged path injection risk.
302-361: LGTM! Block creation now uses event system and queued persistence.The refactored implementation:
- Adds
lastUpdatedtimestamp to new blocks (line 318)- Emits structured
newChangeevents fordata.elementsanddata.elementOrder- Uses
enqueueSave('block-creation')instead of ad-hocsetTimeout(line 352)This addresses the previously flagged persistence serialization risk.
2220-2270: LGTM! Protocol sanitization prevents malicious link injection.The
sanitizeHtmlForClipboardfunction now:
- Allowlists safe protocols (
http:,https:,mailto:,tel:)- Explicitly rejects dangerous protocols (
javascript:,data:,vbscript:)- Strips all attributes and re-adds only clean ones
This addresses the previously flagged security risk when pasting HTML with links.
js/editor.js
Outdated
| // debounce: creating events when typing stops. (what if the user closes the tab before debounce triggers???) -> flush pending updates using `unload` | ||
|
|
||
| clearTimeout(debounceTimer); | ||
|
|
||
| debounceTimer = setTimeout(async () => { | ||
| const changed = elementUpdatesLeft | ||
| .map(elementId => { | ||
| const element = wisk.editor.getElement(elementId); | ||
| if (element) { | ||
| return { | ||
| path: 'document.elements', | ||
| values: { | ||
| id: element.id, | ||
| value: element.value, | ||
| lastEdited: element.lastEdited, | ||
| component: element.component, | ||
| }, | ||
| }; | ||
| console.log('typing stopped, creating events for: ', elementUpdatesNeeded.size, 'elements'); | ||
|
|
||
| const timestamp = Date.now(); | ||
|
|
||
| // create events for all elements needing update | ||
| elementUpdatesNeeded.forEach(elementId => { | ||
| const domElement = document.getElementById(elementId); | ||
| if(!domElement) { | ||
| return; | ||
| } | ||
|
|
||
| const elementValue = domElement.getValue(); | ||
| const componentName = domElement.tagName.toLowerCase(); | ||
|
|
||
| // create event | ||
| wisk.sync.newChange({ | ||
| path: `data.elements.${elementId}.value`, | ||
| value: { | ||
| data: elementValue, | ||
| timestamp: timestamp, | ||
| agent: wisk.sync.agent | ||
| } | ||
| return null; | ||
| }) | ||
| .filter(Boolean); | ||
| }); | ||
|
|
||
| // component because they can be changed (changeBlockType) | ||
|
|
||
| wisk.sync.newChange({ | ||
| path: `data.elements.${elementId}.component`, | ||
| value: { | ||
| data: componentName, | ||
| timestamp: timestamp, | ||
| agent: wisk.sync.agent | ||
| } | ||
| }); | ||
|
|
||
| wisk.sync.newChange({ | ||
| path: `data.elements.${elementId}.lastUpdated`, | ||
| value: { | ||
| data: timestamp, | ||
| timestamp: timestamp, | ||
| agent: wisk.sync.agent | ||
| } | ||
| }); | ||
|
|
||
| // special case: first element | ||
| if(elementId === wisk.editor.document.data.elements[0].id) { | ||
| const textContent = domElement.getTextContent().text || ''; | ||
|
|
||
| if(textContent) { | ||
| document.title = textContent; | ||
| wisk.sync.newChange({ | ||
| path: 'data.config.name', | ||
| value: { | ||
| data: textContent, | ||
| timestamp: timestamp, | ||
| agent: wisk.sync.agent | ||
| } | ||
| }); | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| const elementIds = wisk.editor.document.data.elements.map(e => e.id); | ||
| // save to indexedDB locally | ||
| await wisk.sync.saveModification(); | ||
|
|
||
| await wisk.sync.saveUpdates(); | ||
| // TODO: Send only the events to server (future step) | ||
| // await sendEventsToServer(wisk.sync.eventLog); | ||
|
|
||
| elementUpdatesLeft = []; | ||
| deletedElementsLeft = []; | ||
| }, elementSyncTimer); // should it be less? to voice your opinion, join our discord server: https://discord.gg/D8tQCvgDhu | ||
| // Clear the set | ||
| elementUpdatesNeeded.clear(); | ||
| }, 300); |
There was a problem hiding this comment.
Consider reducing debounce window or adding beforeunload flush.
The 300ms debounce (line 2550) means changes aren't persisted until typing stops. As noted in the inline comment (line 2546), if a user closes the tab within this window, updates are lost.
Consider these mitigations:
- Reduce debounce to 100-150ms to narrow the loss window
- Add a
beforeunloadhandler to flush pending updates:
window.addEventListener('beforeunload', (e) => {
if (elementUpdatesNeeded.size > 0) {
// Synchronously create events before unload
const timestamp = Date.now();
elementUpdatesNeeded.forEach(elementId => {
const domElement = document.getElementById(elementId);
if (domElement) {
const elementValue = domElement.getValue();
wisk.sync.newChange({
path: `data.elements.${elementId}.value`,
value: { data: elementValue, timestamp, agent: wisk.sync.agent }
});
}
});
// Note: Can't reliably await async saves in beforeunload
// Consider using sendBeacon or synchronous storage API
}
});- Flush on
visibilitychangewhen the tab becomes hidden
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
js/plugins/code/tier-list.js (1)
181-184: Event listener removal is broken due to.bind()creating a new function reference.
this.handleDragEnd.bind(this)returns a new function each time it's called, so theremoveEventListenercall will not remove the listener added insetupDragAndDrop. The listener will persist even after the element is disconnected.🔎 Proposed fix: Store bound references
setupDragAndDrop() { - window.addEventListener('dragend', this.handleDragEnd.bind(this)); - this.addEventListener('dragstart', this.handleDragStart.bind(this)); - this.addEventListener('dragover', this.handleDragOver.bind(this)); - this.addEventListener('dragleave', this.handleDragLeave.bind(this)); - this.addEventListener('drop', this.handleDrop.bind(this)); - this.addEventListener('touchstart', this.handleTouchStart.bind(this)); + this._boundHandleDragEnd = this.handleDragEnd.bind(this); + window.addEventListener('dragend', this._boundHandleDragEnd); + this.addEventListener('dragstart', this.handleDragStart.bind(this)); + this.addEventListener('dragover', this.handleDragOver.bind(this)); + this.addEventListener('dragleave', this.handleDragLeave.bind(this)); + this.addEventListener('drop', this.handleDrop.bind(this)); + this.addEventListener('touchstart', this.handleTouchStart.bind(this)); } disconnectedCallback() { super.disconnectedCallback(); - window.removeEventListener('dragend', this.handleDragEnd.bind(this)); + window.removeEventListener('dragend', this._boundHandleDragEnd); + // Clean up document listeners if touch was in progress + if (this._boundTouchMove) { + document.removeEventListener('touchmove', this._boundTouchMove); + document.removeEventListener('touchend', this._boundTouchEnd); + document.removeEventListener('touchcancel', this._boundTouchEnd); + } }js/plugins/code/code-element.js (1)
147-180: Fix the setCursor line index calculation to uselineCount() - 1At line 247,
setCursor(this.editor.lineCount(), 0)attempts to set the cursor at an invalid line index. SincelineCount()returns the total count and lines are 0-indexed, usethis.editor.setCursor(this.editor.lineCount() - 1, 0)to position the cursor at the end of the last line.The
text/typescriptMIME type is correctly registered by CodeMirror's built-in JavaScript mode and does not require a separate TypeScript mode file or verification.
♻️ Duplicate comments (4)
js/plugins/code/base-text-element.js (1)
1613-1623: Placeholder check still treats structural children as “non‑empty”
updatePlaceholder()now distinguishes text vs child elements, buthasChildElements = this.editable.children.length > 0;will often betrueeven when the editor is visually empty (browsers insert<br>or similar structural nodes into contenteditables), preventing the placeholder from showing.Consider filtering out purely structural children such as
<br>:const hasChildElements = Array.from(this.editable.children).some( el => el.tagName !== 'BR' );This matches the earlier suggestion you already received here.
js/editor.js (3)
417-477:.lastUpdatedelement events are still not applied in remote handler
handleChanges()groupsdata.elements.*events into value vs component updates only:if (event.path.startsWith('data.elements.')) { if (event.path.includes('.value')) { … } else if (event.path.includes('.component')) { … } }Events on
data.elements.<id>.lastUpdatedare ignored here (never pushed to any list, and never passed towisk.sync.applyEvent), so remote.lastUpdatedupdates won’t be reflected in the local document, even though you now emit them inchangeBlockTypeandjustUpdates.You should either:
- Treat
.lastUpdatedas a third branch routed tohandleElementUpdate, or- At least call
wisk.sync.applyEvent(wisk.editor.document, event)for those events so the in‑memory document’s timestamps stay correct.
506-533: Remote component changes still don’t update full‑width container classIn
handleElementUpdate, whenproperty === 'component'you replace the DOM node with a new custom element and then set its value, but you don’t update the surrounding container’srndr-full-widthclass based on the new plugin’s width:} else if (property === 'component') { const newType = event.value.data; const newDomElement = document.createElement(newType); newDomElement.id = elementId; domElement.replaceWith(newDomElement); setTimeout(() => { newDomElement.setValue('', element.value); }, 0); }Local
changeBlockType()does adjust the container class usingwisk.plugins.getPluginDetail(newType), so remote component changes will visually diverge (full‑width vs normal).You can mirror the same container‑class update here after
replaceWith(newDomElement).
2506-2631:justUpdatesdebounce is cleaner but still risks losing edits on abrupt tab closeThe new
justUpdates:
- Queues element IDs in
elementUpdatesNeeded.- Debounces for 300ms, then batches events for
value,component, andlastUpdatedfor each element.- Special‑cases the first element to update
data.config.namefromdomElement.getTextContent().text.- Persists via a single
saveModification()and clears the set.This is a solid event‑creation pipeline, but the existing concern remains:
- The 300ms debounce window still means typed changes can be lost if the user closes the tab or navigates away before the timer fires.
domElement.getTextContent().textis accessed without guarding againstgetTextContent()being missing or returning an object withouttext, which can throw for non‑textual plugins.Consider:
- Adding a
beforeunload/visibilitychangehandler that flushes pendingelementUpdatesNeededsynchronously (creating events without waiting on async I/O).- Safely reading the text with optional chaining:
const tc = domElement.getTextContent?.(); const textContent = tc?.text || '';
🧹 Nitpick comments (2)
js/plugins/code/tier-list.js (1)
296-341: Consider guarding against emptychangedTouches.The implementation correctly mirrors the mouse drop logic and properly cleans up styles and event listeners. However,
e.changedTouches[0]could be undefined in edge cases (though rare). Consider a guard clause for defensive coding.🔎 Optional defensive check
handleTouchEndEvent(e) { if (!this.touchDraggedImage) return; const touch = e.changedTouches[0]; + if (!touch) { + this.cleanupTouchState(); + return; + } this.touchDraggedImage.style.display = 'none';You could extract the cleanup to a helper method to avoid duplication.
js/plugins/code/base-text-element.js (1)
1788-1993: Variable redeclarations in Markdown paste path (Biome errors)In the markdown paste branch of
handlePaste(),inx,lastId, andiare redeclared withvarafter having already been declared earlier in the function, which Biome flags as suspicious and can be confusing to readers.You can switch these to
letand avoid redeclaration by reusing the outer variables, or scope them in a small block:- if (parsedElements.length > 0) { - var inx = 0; - var lastId = this.id; + if (parsedElements.length > 0) { + let inx = 0; + let lastId = this.id; @@ - for (var i = inx; i < parsedElements.length; i++) { + for (let i = inx; i < parsedElements.length; i++) {This keeps the control flow clearer and appeases the linter.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
index.htmljs/editor.jsjs/paste-handler.jsjs/plugins/code/base-text-element.jsjs/plugins/code/code-element.jsjs/plugins/code/tier-list.js
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-12T14:14:36.148Z
Learnt from: sushilbang
Repo: sohzm/wisk PR: 54
File: js/plugins/code/base-text-element.js:58-62
Timestamp: 2025-12-12T14:14:36.148Z
Learning: In the js/plugins/code directory, ensure that BaseTextElement serves as the shared base class with common state/logic, while concrete elements (e.g., TextElement) extend it and override render() to provide UI markup. The base class's connectedCallback should query for child elements that are injected by their render methods. This inheritance pattern should be consistently applied across the plugin system; review new or existing elements to maintain a clean base-vs-extension separation, avoid tight coupling, and verify that the render contracts and DOM querying align between base and derived classes.
Applied to files:
js/plugins/code/tier-list.jsjs/plugins/code/code-element.jsjs/plugins/code/base-text-element.js
🧬 Code graph analysis (3)
js/plugins/code/base-text-element.js (1)
js/wisk.js (1)
wisk(3-3)
js/editor.js (3)
js/sync/sync.js (4)
elementId(212-212)key(207-207)pathParts(172-172)element(213-213)js/wisk.js (1)
wisk(3-3)js/plugins/plugins.js (1)
nav(167-167)
js/paste-handler.js (2)
js/editor.js (20)
element(512-512)element(545-545)element(724-724)element(855-855)element(1015-1015)element(1043-1043)element(1056-1056)element(1076-1076)element(1148-1148)element(1236-1236)element(1510-1510)element(1655-1655)elementId(370-370)elementId(384-384)elementId(404-404)elementId(509-509)elementId(2522-2522)i(723-723)i(854-854)i(1233-1233)js/wisk.js (1)
wisk(3-3)
🪛 Biome (2.1.2)
js/plugins/code/base-text-element.js
[error] 2646-2646: Shouldn't redeclare 'inx'. Consider to delete it or rename it.
'inx' is defined here:
(lint/suspicious/noRedeclare)
[error] 2647-2647: Shouldn't redeclare 'lastId'. Consider to delete it or rename it.
'lastId' is defined here:
(lint/suspicious/noRedeclare)
[error] 2658-2658: Shouldn't redeclare 'i'. Consider to delete it or rename it.
'i' is defined here:
(lint/suspicious/noRedeclare)
🔇 Additional comments (15)
js/plugins/code/tier-list.js (3)
156-159: LGTM!Touch state fields are properly initialized in the constructor, consistent with their usage in the touch event handlers.
256-273: LGTM!Touch start handler correctly initializes state, applies visual feedback with opacity, and stores bound references for proper cleanup. The pattern of adding document-level listeners for move/end events is appropriate for touch drag operations.
275-294: LGTM!Movement detection with a 10px threshold prevents accidental drags. The
e.preventDefault()call correctly prevents page scrolling during touch drag. Fixed positioning with high z-index ensures the dragged image stays visible above other content.js/plugins/code/base-text-element.js (2)
614-699: Numbered‑list Markdown detection and numbering logic are solidThe extended
handleMarkdown()handling for###–#####,*/>/``` ````, dividers, checkbox patterns, and the new^(\d+)[.)]$detection for numbered lists is coherent. Carrying the parsed number into `val.number` and then into `changeBlockType` aligns with the list element model.
976-1143: Paste‑link menu UX wiring looks correctThe URL detection (
isUrl/isInternalUrl), menu positioning near the cursor, outside‑click closing withcomposedPath(), and keyboard handling insidehandlePasteLinkChoice/handleKeyDownform a coherent flow. The three options (inline URL link, bookmark/link‑preview block, embed block) are clearly separated and dispatch the expectedchangeBlockType/inline‑link behaviors.index.html (1)
269-276: Paste handler script ordering is appropriateLoading
/js/paste-handler.jsbeforeeditor.jsensuresWiskPasteHandleris available forhandlePasteEventand other editor code, and the placement alongside other core scripts keeps bootstrapping straightforward.js/editor.js (9)
123-185: Config mutation helpers correctly emit events and persist
addConfigChange,addUserAccess,removeUserAccess,setPublicStatus, andsetDatabasePropall:
- Build a timestamped
valueenvelope{ data, timestamp, agent }.- Use safe, dotted paths (
data.config.*,data.config.databaseProps.${key}) with input sanitization for keys.- Call
await wisk.sync.saveModification()to persist.This aligns well with the event‑driven model and keeps config mutations centralized.
250-274: Good defensive checks onsetDatabaseProp/savePluginDataidentifiersBoth
setDatabasePropandsavePluginDatanow validate their identifiers against[.\[\]]before embedding them into dotted paths, which prevents accidental path injection via.or bracket syntax. That’s exactly the right direction for the new event paths.
316-353: Element creation events integrate cleanly with event log
createBlockBase()now generates a timestamped element object (includinglastUpdated) and, for local creations, emits:
data.elementswith the new object, anddata.elementOrderwith the updated ID list,then enqueues a
block-creationsave. This keeps document state, order, and persistence in sync with the event log model without touching the remote path (isRemoteguard).
989-1034: Delete‑block flow correctly emits events and updates DOM/docThe new async
deleteBlock:
- Guards the special root element (
abcdxyz).- Emits a
data.deletedElementsevent with full element data, timestamp, and agent.- Removes the corresponding
div-<id>from the DOM.- Filters the element out of
wisk.editor.document.data.elements.- Fires a
block-deletedcustom event.- Calls
saveModification()whenrecis undefined.This is a clean, consistent implementation for element deletion in the event‑driven model.
1068-1139:changeBlockTypeevent emission and DOM update path look solid
changeBlockTypenow:
- Updates the in‑memory element’s
component,value, andlastUpdated.- Emits three events for
component,value, andlastUpdatedwith consistent timestamps.- Replaces the DOM node, updates the container’s
rndr-full-widthclass via plugin metadata, and re‑applies the value.- Dispatches a
block-changedevent and persists whenrecis undefined.This aligns well with the event‑driven design and keeps DOM, document model, and persisted log in sync.
1660-1717: Drag‑and‑drop cancelation and cleanup are robustThe added
handleDragKeydown/cleanupDragwiring:
- Cancels drags on
Escapeor windowblur.- Clears the auto‑scroll timer, hides the drop indicator, removes the clone node, and unregisters all drag listeners.
This significantly tightens drag lifecycle handling and prevents stuck drag state or stray clones.
2001-2155: Rectangle selection implementation is thoughtful and well‑boundedThe rectangle selection flow:
- Starts only in the editor area and skips UI chrome /
disableSelectionplugins.- Uses editor‑relative coordinates and throttles updates with
requestAnimationFrame.- Tracks intersecting
.rndrcontainers, adds overlays, and falls back to native selection when a single element is hit.Combined with overlay updates on scroll/resize and a mutation observer, this is a solid, user‑friendly implementation.
2201-2213: WISK clipboard intercept usingWiskPasteHandleris correctly scoped
handlePasteEventdelegates toWiskPasteHandler.handleWiskClipboardPaste(event)and, when WISK data is present, prevents default, stops propagation, restoreselementClipboard, and callspasteElements().This cleanly separates internal clipboard (multi‑element) paste from standard browser paste behavior and leverages the new centralized paste handler.
2230-2280: Clipboard HTML sanitization and base64 wrapper for WISK format are well‑designed
sanitizeHtmlForClipboard()strips dangerous elements, removes all attributes from<a>, and then re‑applies only a cleanedhref(allowlistinghttp/https/mailto/teland rejectingjavascript:/data:etc.), plus safecontenteditable=false/target=_blank. This is a good defense‑in‑depth measure.copySelectedElements()base64‑encodes the JSON clipboard payload (__WISK_CLIPBOARD__...__WISK_CLIPBOARD_END__) to avoid HTML entity corruption, while still providing a plain‑text representation for interoperability.Together, these changes strongly improve clipboard robustness and safety.
Also applies to: 2288-2334
| static parseWiskClipboard(htmlData) { | ||
| if(!htmlData) return null; | ||
|
|
||
| const match = htmlData.match(/__WISK_CLIPBOARD__(.+?)__WISK_CLIPBOARD_END__/); | ||
| if(!match) return null; | ||
|
|
||
| try { | ||
| // Try base64 decoding first (new format) | ||
| const jsonString = decodeURIComponent(escape(atob(match[1]))); | ||
| const wiskData = JSON.parse(jsonString); | ||
| if(wiskData.__wisk_elements__ && wiskData.elements) { | ||
| return wiskData.elements; | ||
| } | ||
| } catch (error) { | ||
| // Fallback: try direct JSON parse (old format) | ||
| try { | ||
| const wiskData = JSON.parse(match[1]); | ||
| if(wiskData.__wisk_elements__ && wiskData.elements) { | ||
| return wiskData.elements; | ||
| } | ||
| } catch (fallbackError) { | ||
| console.error('Failed to parse wisk clipboard data: ', error); | ||
| } | ||
| } | ||
|
|
||
| return null; | ||
| } |
There was a problem hiding this comment.
Clarify error logging in parseWiskClipboard’s fallback path
In the inner catch (fallbackError), the log still prints the outer error rather than fallbackError, which will make diagnosing JSON‑vs‑base64 issues harder:
} catch (fallbackError) {
console.error('Failed to parse wisk clipboard data: ', error);
}You likely want to log fallbackError (and optionally the original) so you can distinguish which stage failed.
🤖 Prompt for AI Agents
In js/paste-handler.js around lines 11 to 37, the inner catch block logs the
outer `error` instead of the `fallbackError`, obscuring which decode attempt
failed; update the inner catch to log the correct error (`fallbackError`) and
include contextual information (and optionally include the original `error` as
well) so the log distinguishes base64/decode failure from raw-JSON parse
failure.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (9)
js/editor.js (7)
1-6: Remove unuseddeletedElementsLefttracking.
deletedElementsLeftis declared and updated indeleteBlockbut never read anywhere in the current implementation, so it’s dead state.Suggested cleanup
-var deletedElementsLeft = []; ... - deletedElements.push(elementId); - deletedElementsLeft.push(elementId); + deletedElements.push(elementId);
989-1034: Delete-block event emission is correct, but the extradeletedElementsLeftpush is noise.The async
deleteBlocknow:
- Emits a
data.deletedElementsevent with the full element and timestamp.- Removes the DOM node and updates
wisk.editor.document.data.elements.- Pushes to both
deletedElementsanddeletedElementsLeft, but onlydeletedElementsis used.- Saves immediately when
recis undefined.Functionally fine; just drop
deletedElementsLeftas noted earlier.
2233-2283:sanitizeHtmlForClipboardrobustly cleans linkhrefprotocols.The sanitizer:
- Normalizes escaped characters in
href.- Validates protocol against a whitelist (
http:,https:,mailto:,tel:), falling back to manual checks on parse failure.- Strips all attributes and reapplies only safe ones, plus
contenteditable="false"/target="_blank".This closes the earlier
javascript:/data:risk in clipboard HTML.
417-477:handleChangesstill ignores.lastUpdatedelement events.In the
data.elements.*branch you only dispatch to:
elementValueUpdates(paths containing.value)elementComponentUpdates(paths containing.component)Events for
data.elements.<id>.lastUpdatedare neither grouped nor passed towisk.sync.applyEvent, so remote timestamp updates are effectively dropped on the client.Suggested adjustment
- events.forEach(event => { - if (event.path.startsWith('data.elements.')) { - if (event.path.includes('.value')) { - elementValueUpdates.push(event); - } else if (event.path.includes('.component')) { - elementComponentUpdates.push(event); - } - } + events.forEach(event => { + if (event.path.startsWith('data.elements.')) { + if (event.path.includes('.value')) { + elementValueUpdates.push(event); + } else if (event.path.includes('.component')) { + elementComponentUpdates.push(event); + } else if (event.path.includes('.lastUpdated')) { + // Apply directly; no DOM changes needed + wisk.sync.applyEvent(wisk.editor.document, event); + } + }
480-533: Remote component changes don’t update container width class.
handleElementUpdate’scomponentbranch replaces the inner element but doesn’t update the containingdiv-<id>to add/removerndr-full-widthbased on the new plugin’s width, unlikechangeBlockType.Blocks that change from full-width to regular (or vice versa) via remote events will leave the container with stale width styling.
Suggested fix
} else if (property === 'component') { const newType = event.value.data; const newDomElement = document.createElement(newType); newDomElement.id = elementId; domElement.replaceWith(newDomElement); - setTimeout(() => { - newDomElement.setValue('', element.value); - }, 0); + const container = document.getElementById(`div-${elementId}`); + if (container) { + const pluginDetail = wisk.plugins.getPluginDetail(newType); + if (pluginDetail.width === 'max') { + container.classList.add('rndr-full-width'); + } else { + container.classList.remove('rndr-full-width'); + } + } + setTimeout(() => { + newDomElement.setValue('', element.value); + }, 0); }
2608-2623: Still fragile: assumesgetTextContent().textalways exists on first element.In the first-element special case you still do:
const textContent = domElement.getTextContent().text || '';If
getTextContentis missing or returns something without.text, this will throw and abort the whole debounce batch.Suggested guard
- if(elementId === wisk.editor.document.data.elements[0].id) { - const textContent = domElement.getTextContent().text || ''; + if (elementId === wisk.editor.document.data.elements[0].id) { + const textContentObj = domElement.getTextContent?.(); + const textContent = textContentObj?.text || '';
2509-2635: Debounced persistence gap remains; consider a flush on unload/visibilitychange.The 300 ms debounce in
justUpdatesstill means recent edits can be lost if the tab is closed or hidden before the timeout fires, as acknowledged in the inline comment.Adding a
beforeunloadand/orvisibilitychangehandler to synchronously flushelementUpdatesNeeded(emit events and callsaveModificationonce) would mitigate this without changing the steady-state debounce.js/paste-handler.js (1)
10-31: Log the correct error in the fallback JSON parse catch.The inner
catch (fallbackError)still logs the outererror, which makes it hard to see whether base64 decode or raw JSON parse failed:} catch (fallbackError) { console.error('Failed to parse wisk clipboard data: ', error); }Log
fallbackError(and optionally both errors) so you can distinguish the failure stage.Suggested fix
- } catch (error) { - try { - const wiskData = JSON.parse(match[1]); - if (wiskData.__wisk_elements__ && wiskData.elements) { - return wiskData.elements; - } - } catch (fallbackError) { - console.error('Failed to parse wisk clipboard data: ', error); - } - } + } catch (error) { + try { + const wiskData = JSON.parse(match[1]); + if (wiskData.__wisk_elements__ && wiskData.elements) { + return wiskData.elements; + } + } catch (fallbackError) { + console.error('Failed to parse wisk clipboard data (fallback JSON parse): ', fallbackError, { + originalError: error, + }); + } + }js/plugins/code/base-text-element.js (1)
1598-1603: Placeholder empty-check still treats structural<br>as content.
hasChildElementsusesthis.editable.childElementCount > 0, so an auto-inserted<br>keepsisEmptyfalse and the placeholder hidden for visually empty blocks:const hasText = this.editable.innerText.trim() !== ''; const hasChildElements = this.editable.childElementCount > 0; const isEmpty = !hasText && !hasChildElements;To avoid this, filter out purely structural children like
<br>when computinghasChildElements.Suggested tweak
- if (this.editable) { - const hasText = this.editable.innerText.trim() !== ''; - const hasChildElements = this.editable.childElementCount > 0; - const isEmpty = !hasText && !hasChildElements; + if (this.editable) { + const hasText = this.editable.innerText.trim() !== ''; + const hasChildElements = Array.from(this.editable.children).some( + el => el.tagName !== 'BR' + ); + const isEmpty = !hasText && !hasChildElements;
🧹 Nitpick comments (5)
js/paste-handler.js (1)
536-564: Simplify or fixisMarkdownTextthreshold logic.The final return condition:
return markdownPatterns > 0 && (markdownPatterns >= 1 || markdownPatterns / lines.length >= 0.2);reduces to
markdownPatterns > 0— the ratio check is never used. Either simplify tomarkdownPatterns > 0or restore the intended threshold (e.g.,markdownPatterns / lines.length >= 0.2) so detection behavior is explicit.js/plugins/code/base-text-element.js (2)
972-1054: Paste-link menu insertion and teardown look correct.The paste-link workflow (insert temporary
<link-element>, position.paste-link-menurelative to the link, wire option clicks, and clean up with_closePasteMenuHandler) is internally consistent:
- Selection/range handling gracefully falls back to end-of-block.
- Menu is closed on outside click and when a choice is made.
- Bookmark/embed choices remove the inline link and transform the block via
changeBlockType.Just ensure that
text-element’s CSS keeps.paste-link-menuabsolutely positioned relative to the same coordinate space asthis.editableso the computedleft/topoffsets line up visually.
1156-1173: Guard suggestion debounce when paste menu is open.
handleKeyDowncorrectly routes ArrowUp/Down/Enter/Escape to the paste-link menu whenshowingPasteLinkMenuis true, but the rest of the method (emoji handling, suggestion discard, navigation handlers) still runs for other keys.If you ever see interactions between the paste-link menu and emoji/autocomplete flows, consider short‑circuiting the rest of
handleKeyDownwhenevershowingPasteLinkMenuis true (except for keys you explicitly want to bubble).js/plugins/code/code-element.js (1)
245-249: Auto-focus on initialization is generally helpful but might surprise in some contexts.Calling
this.editor.focus()and moving the cursor to the end right after initialization makes sense when a new code block is created interactively. If this element ever appears off-screen or inside a non-focused panel, you may want a guard flag to skip autofocus in those contexts.js/editor.js (1)
1781-1799: Drag-drop reinsert deletes + recreates blocks; consider event-level moves later.
handleDropcurrently implements reordering by deleting the original block and creating a new one below the target, using the capturedoriginalValueandoriginalComponent. This is consistent with your event model (a delete + create sequence), though a future enhancement could use a pureelementOrderchange for more compact histories.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
js/editor.jsjs/paste-handler.jsjs/plugins/code/base-text-element.jsjs/plugins/code/code-element.jsjs/plugins/code/numbered-list-element.js
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-12T14:14:36.148Z
Learnt from: sushilbang
Repo: sohzm/wisk PR: 54
File: js/plugins/code/base-text-element.js:58-62
Timestamp: 2025-12-12T14:14:36.148Z
Learning: In the js/plugins/code directory, ensure that BaseTextElement serves as the shared base class with common state/logic, while concrete elements (e.g., TextElement) extend it and override render() to provide UI markup. The base class's connectedCallback should query for child elements that are injected by their render methods. This inheritance pattern should be consistently applied across the plugin system; review new or existing elements to maintain a clean base-vs-extension separation, avoid tight coupling, and verify that the render contracts and DOM querying align between base and derived classes.
Applied to files:
js/plugins/code/base-text-element.jsjs/plugins/code/numbered-list-element.jsjs/plugins/code/code-element.js
🧬 Code graph analysis (3)
js/plugins/code/base-text-element.js (2)
js/editor.js (8)
newType(525-525)el(1484-1484)i(723-723)i(854-854)i(1236-1236)event(1725-1725)event(1782-1782)tempDiv(2238-2238)js/wisk.js (1)
wisk(3-3)
js/plugins/code/numbered-list-element.js (2)
js/editor.js (2)
event(1725-1725)event(1782-1782)js/wisk.js (1)
wisk(3-3)
js/paste-handler.js (1)
js/wisk.js (1)
wisk(3-3)
🔇 Additional comments (25)
js/plugins/code/base-text-element.js (4)
348-355: Paste-link menu DOM lookup matches inheritance pattern.Querying
.paste-link-menuinconnectedCallbackand storing it onthis.pasteLinkMenuis consistent with the pattern where concrete elements (e.g.,text-element) render the menu HTML whileBaseTextElementowns the behavior/state. No issues here.Based on learnings, this aligns with the intended base/derived separation.
1353-1377: Backspace removal of adjacentlink-elementis well-scoped.The new
handleBackspacelogic around selection/range checks cleanly removes a precedingLINK-ELEMENTwhen backspacing at its boundary (both for text and element containers) and falls through to existing block-merge behavior otherwise. No correctness issues spotted.
1760-1796: Copy handler forlink-element→<a>looks good.
handleCopyclones the current selection, rewrites eachlink-elementto a plain<a>withdata-wisk-*metadata, and writes both HTML and text variants to the clipboard. That should preserve Wisk semantics while keeping external pastes interoperable. No issues seen.
1798-1834: Text-element paste integration withWiskPasteHandleris consistent.The paste handler correctly:
- Special-cases standalone URLs into block‑level link elements / paste-link menu when the block is empty.
- Defers Wisk clipboard format to the document-level handler (
handlePasteEvent) by early-returning.- Uses
WiskPasteHandler.handleTextElementPastefor HTML/Markdown/plain-text, and callssendUpdates()when that handler reportshandled.This wiring matches the new unified paste pipeline; no additional issues.
js/plugins/code/numbered-list-element.js (4)
20-35: Revised numbering styles per indent level are coherent.Mapping:
- Level 0 →
1, 2, 3...- Level 1 →
a, b, c...- Level 2 →
i, ii, iii...- Level 3 →
1, 2, 3...- Level 4 →
a, b, c...via
styles[level % styles.length]is straightforward and keeps nested numbering predictable.
107-117: Indent + number reset on leading space/Tab is reasonable.On
beforeinputspace at the start and onTabat focus 0 you now:
- Increment
this.indent- Reset
this.number = 1- Update indent and send updates
That matches the behavior of starting a new nested ordered list whose first item should be
1.. No functional issues.
120-158: Enter behavior correctly handles “split vs exit list”.The new
handleEnterKey:
- Splits content at the caret into
beforeContainerandafterContainer.- Leaves current item with
beforeContainer.innerHTML.- If the current item is empty (no text and no children), converts this block to a
text-elementwith the trailing content.- Otherwise, creates a new
numbered-list-elementwith sameindentandnumber + 1, populated with the trailing content.This mirrors typical OL behavior (“Enter on empty item exits the list”) and keeps numbering consistent.
187-197: Tab-at-start resetting number to 1 is consistent with space-at-start.In
handleTab, when at focus 0 you incrementindent, resetnumberto 1, and update. That keeps behavior consistent with the space-at-start handler and avoids surprising carryover numbers when promoting an item into a deeper level.js/plugins/code/code-element.js (4)
4-138: CodeMirror styling and selection/cursor tweaks look safe.The CSS changes (host margin, select sizing/opacity behavior, CodeMirror padding/line-height, selection colors, cursor width, token colors) are purely visual and don’t affect behavior. They should improve readability without functional impact.
148-181: ExpandedsupportedLanguagesis consistent and backward compatible.New entries (Rust, Scala, SCSS, XML, JSON, YAML, Bash/Shell/PowerShell, Dockerfile, Plain Text) extend the dropdown without changing existing keys. Since
selectedLangstill defaults to'javascript', existing documents remain unaffected.
274-305:getModeForLanguagemapping covers the new languages and has a safe fallback.Mappings for new languages (e.g.,
typescript → 'text/typescript',rust → 'rust',scala → 'text/x-scala',scss → 'text/x-scss',json → 'application/json',yaml → 'yaml',bash/shell → 'shell',powershell → 'powershell',dockerfile → 'dockerfile',plaintext → 'text/plain') look correct, with an explicit'text/plain'fallback for unknown keys.
188-208: Incomplete language support: code does not load modes for all languages insupportedLanguages.The code loads only 16 language modes (javascript, xml, css, python, clike, markdown, go, sql, php, ruby, swift, rust, yaml, shell, powershell, dockerfile, sass), but
supportedLanguagesexposes at least 25 languages including java, kotlin, scala, html, json, csharp, and scss. Users can select these languages from the dropdown but will receive no syntax highlighting. Additionally, SCSS is listed insupportedLanguagesbut the code loads Sass instead. TypeScript is also insupportedLanguagesbut relies on the javascript mode variant, requiring explicit MIME type configuration (e.g.,mode: "text/typescript") rather than a dedicated mode import.Likely an incorrect or invalid review comment.
js/editor.js (13)
123-184: Config change → event emission looks correct and centralized.
addConfigChangenow consistently emits timestamped events for name, theme, and plugin add/remove, then callswisk.sync.saveModification(). Name and theme side effects (document.title,wisk.theme.setTheme) remain in place. This aligns config updates with the new event-driven sync path.
186-296: Access/public/database/pluginData setters align with event-driven config/plugin updates.
addUserAccess,removeUserAccess,setPublicStatus,setDatabaseProp, andsavePluginData:
- Sanitize keys/identifiers to avoid path injection.
- Build new arrays/objects rather than mutating in place.
- Emit
data.config.*/data.pluginData.*events with timestamps and agents.- Immediately
await wisk.sync.saveModification().This is a solid pattern for config/plugin mutations.
302-353: Element creation events and order updates are correctly emitted for local blocks.
createBlockBasenow:
- Adds
lastUpdatedto the element object.- For non-remote blocks, emits
data.elements(new element object) anddata.elementOrder(current ID list) events with timestamps/agent.- Uses
wisk.sync.enqueueSave('block-creation')instead of ad-hocsetTimeout(saveModification).This matches the intent of the new event-driven architecture.
536-555: Element deletion handler is consistent with the event log.
handleElementDeletionsapplies eachdata.deletedElementsevent by:
- Avoiding double-processing via the
deletedElementsarray.- Removing the corresponding DOM container and updating
wisk.editor.document.data.elements.- Calling
wisk.sync.applyEventto keep the document model aligned.This is a good fit for the new event package format.
557-589: Config and pluginData change handlers correctly bridge events to UI.
handleConfigChangeandhandlePluginDataChange:
- Call
wisk.sync.applyEventto update the document.- Apply side effects (theme set, document title, reload on plugins change, dispatch of custom visibility/access/database-props events, and plugin
loadDatacalls).This wiring makes config/plugin data reactive without duplicating state.
683-693: Block move now emitsdata.elementOrderand enqueues a save.
moveBlockupdateswisk.editor.document.data.elements, reorders DOM nodes, then:
- Emits a
data.elementOrderevent with the new ID order and timestamp.- Calls
wisk.sync.enqueueSave('block-move').This is consistent with creation/deletion flows and avoids fire‑and‑forget
setTimeout(saveModification).
875-949:htmlToMarkdown/convertInlineMarkdowncitation and reference handling looks correct.The updated conversion logic:
- Encodes
<cite-element>as--citation-element--id--during markdown generation and decodes it back when parsing inline markdown.- Handles reference-number links both in
<sup><a class="reference-number">and<span .reference-number>cases.- Keeps other inline formatting (bold/italic/strike/code/links) intact.
No obvious edge-case regressions.
1068-1142:changeBlockTypeevent emission and DOM replacement are solid.The updated
changeBlockType:
- Updates
element.component,element.value, andelement.lastUpdated.- Emits separate events for
.component,.value, and.lastUpdated.- Replaces the DOM element with a new instance of
newType, updates the container’s width class, and sets its value, focusing it afterward.- Persists via
saveModification()whenrecis undefined.This fits well with the event-driven model and fixes the width class issue for local type changes.
1209-1378: Markdown-to-elements conversion is well-structured and consistent with new code-element behavior.
convertMarkdownToElementshandles:
- Headings, fenced code blocks (with language), blockquotes, checkboxes, numbered/unordered lists (with indentation), horizontal rules, and plain text.
- Uses
convertInlineMarkdownfor inline formatting, which now also supports citations and reference numbers.No correctness issues observed; behavior appears compatible with the new
code-elementlanguage support.
1763-1775: Auto-scroll during drag is properly cleaned up viacleanupDrag.
cleanupDragnow stopsautoScroll, hides the drop indicator, removes the clone, and unregisters all drag-related listeners (including the new blur/keydown handlers). This prevents stuck scroll timers and lingering listeners after a drag ends or is canceled.
2205-2215: Wisk clipboard paste handler is correctly isolated.
handlePasteEventintercepts document-level pastes, callsWiskPasteHandler.handleWiskClipboardPaste, and when it returns elements:
- Prevents default and stops propagation.
- Restores
elementClipboardand callspasteElements().This keeps Wisk’s internal multi-block clipboard separate from normal paste flows.
2288-2337: Wisk clipboard serialization (base64__WISK_CLIPBOARD__marker) is sound.
copySelectedElementsnow:
- Deep-clones each element’s
valueand sanitizestextContentviasanitizeHtmlForClipboard.- Builds a
wiskClipboardDataobject with a__wisk_elements__marker andelementsarray.- Base64‑encodes the JSON and wraps it in
__WISK_CLIPBOARD__...__WISK_CLIPBOARD_END__.- Writes both plain text and HTML (with a
<meta name="wisk-clipboard">marker) to the system clipboard.This scheme is compatible with
parseWiskClipboardinpaste-handler.js.
2637-2668: Virtual keyboard detection helper is straightforward and self-contained.
initKeyboardDetectionwires bothvisualViewportandnavigator.virtualKeyboard(where available) to emit avirtual-keyboard-visiblecustom event withisVisibleandheight. No external dependencies; behavior looks correct.
| wisk.editor.justUpdates = async function (elementIdOrEvent) { | ||
| console.log('justUpdates called with:', elementIdOrEvent); | ||
|
|
||
| // handle event objects passed directly | ||
| if(typeof elementIdOrEvent === 'object') { | ||
| wisk.sync.newChange(elementIdOrEvent); | ||
|
|
||
| console.log('critical event, saving immediately'); | ||
| await wisk.sync.saveModification(); | ||
|
|
||
| // TODO: Send event to server here | ||
| return; | ||
| } | ||
|
|
||
| // Handle nested elements with IDs containing hyphens | ||
| if (elementId && elementId.includes('-')) { | ||
| const elementId = elementIdOrEvent; | ||
| // handle nested elements | ||
| if(elementId && elementId.includes('-')) { | ||
| const eid = elementId.split('-')[0]; | ||
| document.getElementById(eid).editor.justUpdates(elementId); | ||
| const parentElement = document.getElementById(eid); | ||
| if(parentElement && parentElement.editor) { | ||
| parentElement.editor.justUpdates(elementId); | ||
| } | ||
| return; | ||
| } | ||
|
|
||
| window.dispatchEvent(new CustomEvent('something-updated', { detail: { id: elementId } })); | ||
| if(!elementId) { | ||
| return; | ||
| } | ||
|
|
||
| if (elementId) { | ||
| if (elementId === wisk.editor.document.data.elements[0].id) { | ||
| document.title = byQuery('#' + elementId).getTextContent().text; | ||
| wisk.editor.document.data.config.name = document.title; | ||
| wisk.sync.newChange({ | ||
| action: 'config', | ||
| key: 'name', | ||
| value: document.title, | ||
| }); | ||
| } | ||
| // mark element as needing update (not creating events yet) | ||
| elementUpdatesNeeded.add(elementId); | ||
|
|
||
| const element = wisk.editor.getElement(elementId); | ||
| if (element) { | ||
| const domElement = document.getElementById(elementId); | ||
| if (domElement) { | ||
| element.value = domElement.getValue(); | ||
| element.lastEdited = Math.floor(Date.now() / 1000); | ||
| element.component = domElement.tagName.toLowerCase(); | ||
| document.getElementById('nav').classList.add('nav-disappear'); | ||
| document.getElementById('getting-started').style.display = 'none'; | ||
|
|
||
| if (!elementUpdatesLeft.includes(elementId)) { | ||
| elementUpdatesLeft.push(elementId); | ||
| } | ||
| } | ||
| } | ||
| // UI updates | ||
| const nav = document.getElementById('nav'); | ||
| if(nav) { | ||
| nav.classList.add('nav-disappear'); | ||
| } | ||
|
|
||
| const gettingStarted = document.getElementById('getting-started'); | ||
| if(gettingStarted) { | ||
| gettingStarted.style.display = 'none'; | ||
| } | ||
|
|
||
| // dispatch event | ||
| window.dispatchEvent(new CustomEvent('something-updates', { | ||
| detail: { id: elementId } | ||
| })); | ||
|
|
||
| // debounce: creating events when typing stops. (what if the user closes the tab before debounce triggers???) -> flush pending updates using `unload` | ||
|
|
||
| clearTimeout(debounceTimer); | ||
|
|
||
| debounceTimer = setTimeout(async () => { | ||
| const changed = elementUpdatesLeft | ||
| .map(elementId => { | ||
| const element = wisk.editor.getElement(elementId); | ||
| if (element) { | ||
| return { | ||
| path: 'document.elements', | ||
| values: { | ||
| id: element.id, | ||
| value: element.value, | ||
| lastEdited: element.lastEdited, | ||
| component: element.component, | ||
| }, | ||
| }; | ||
| console.log('typing stopped, creating events for: ', elementUpdatesNeeded.size, 'elements'); | ||
|
|
||
| const timestamp = Date.now(); | ||
|
|
||
| // create events for all elements needing update | ||
| elementUpdatesNeeded.forEach(elementId => { | ||
| const domElement = document.getElementById(elementId); | ||
| if(!domElement) { | ||
| return; | ||
| } | ||
|
|
||
| const elementValue = domElement.getValue(); | ||
| const componentName = domElement.tagName.toLowerCase(); | ||
|
|
||
| // create event | ||
| wisk.sync.newChange({ | ||
| path: `data.elements.${elementId}.value`, | ||
| value: { | ||
| data: elementValue, | ||
| timestamp: timestamp, | ||
| agent: wisk.sync.agent | ||
| } | ||
| return null; | ||
| }) | ||
| .filter(Boolean); | ||
| }); | ||
|
|
||
| // component because they can be changed (changeBlockType) | ||
|
|
||
| wisk.sync.newChange({ | ||
| path: `data.elements.${elementId}.component`, | ||
| value: { | ||
| data: componentName, | ||
| timestamp: timestamp, | ||
| agent: wisk.sync.agent | ||
| } | ||
| }); | ||
|
|
||
| wisk.sync.newChange({ | ||
| path: `data.elements.${elementId}.lastUpdated`, | ||
| value: { | ||
| data: timestamp, | ||
| timestamp: timestamp, | ||
| agent: wisk.sync.agent | ||
| } | ||
| }); | ||
|
|
||
| // special case: first element | ||
| if(elementId === wisk.editor.document.data.elements[0].id) { | ||
| const textContent = domElement.getTextContent().text || ''; | ||
|
|
||
| if(textContent) { | ||
| document.title = textContent; | ||
| wisk.sync.newChange({ | ||
| path: 'data.config.name', | ||
| value: { | ||
| data: textContent, | ||
| timestamp: timestamp, | ||
| agent: wisk.sync.agent | ||
| } | ||
| }); | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| const elementIds = wisk.editor.document.data.elements.map(e => e.id); | ||
| // save to indexedDB locally | ||
| await wisk.sync.saveModification(); | ||
|
|
||
| await wisk.sync.saveUpdates(); | ||
| // TODO: Send only the events to server (future step) | ||
| // await sendEventsToServer(wisk.sync.eventLog); | ||
|
|
||
| elementUpdatesLeft = []; | ||
| deletedElementsLeft = []; | ||
| }, elementSyncTimer); // should it be less? to voice your opinion, join our discord server: https://discord.gg/D8tQCvgDhu | ||
| // Clear the set | ||
| elementUpdatesNeeded.clear(); | ||
| }, 300); |
There was a problem hiding this comment.
justUpdates() with no argument no longer persists template or global changes.
The new justUpdates implementation treats only:
- Event objects (immediately sent via
wisk.sync.newChange+saveModification), or - A specific
elementId(debounced aggregation viaelementUpdatesNeeded).
A call like wisk.editor.justUpdates() with no arguments now returns immediately:
if (!elementId) {
return;
}But useTemplate() still calls wisk.editor.justUpdates(); after replacing wisk.editor.document.data.elements and setting values, so those changes will not emit any events or be saved.
Possible fixes
- Teach
justUpdates()to handle “all elements” when no ID is provided:
- if(!elementId) {
- return;
- }
+ if (!elementId) {
+ // enqueue all elements for update
+ wisk.editor.document.data.elements.forEach(e => {
+ if (e.id) elementUpdatesNeeded.add(e.id);
+ });
+ } else {
+ elementUpdatesNeeded.add(elementId);
+ }- Or: change
useTemplate()to explicitly emit events (e.g., loop overtemplate.elementsand calljustUpdates(element.id)or a dedicated helper).
| static parseInlineMarkdown(text) { | ||
| if (!text) return ''; | ||
|
|
||
| return active; | ||
| let result = text; | ||
| // Escape HTML entities first (but preserve existing HTML tags we want to keep) | ||
| result = result.replace(/&/g, '&'); | ||
| // Don't escape < and > for HTML tags we want to preserve | ||
| result = result.replace(/<(?!(b|i|u|strike|code|a|br|span|strong|em)\b)/g, '<'); | ||
| result = result.replace(/(?<!\b(b|i|u|strike|code|a|br|span|strong|em))>/g, '>'); | ||
| // Bold and italic combined (***text*** or ___text___) | ||
| result = result.replace(/\*\*\*(.+?)\*\*\*/g, '<b><i>$1</i></b>'); | ||
| result = result.replace(/___(.+?)___/g, '<b><i>$1</i></b>'); | ||
| // Bold (**text** or __text__) | ||
| result = result.replace(/\*\*(.+?)\*\*/g, '<b>$1</b>'); | ||
| result = result.replace(/__(.+?)__/g, '<b>$1</b>'); | ||
| // Italic (*text* or _text_) - be careful not to match inside words | ||
| result = result.replace(/\*([^*]+)\*/g, '<i>$1</i>'); | ||
| result = result.replace(/(?<![a-zA-Z])_([^_]+)_(?![a-zA-Z])/g, '<i>$1</i>'); | ||
| // Strikethrough (~~text~~) | ||
| result = result.replace(/~~(.+?)~~/g, '<strike>$1</strike>'); | ||
| // Inline code (`code`) | ||
| result = result.replace(/`([^`]+)`/g, '<code style="background: var(--bg-2); padding: 2px 4px; border-radius: 3px; font-family: var(--font-mono);">$1</code>'); | ||
| // Links [text](url) - convert to link-element | ||
| result = result.replace(/\[([^\]]+)\]\(([^)]+)\)/g, '<link-element url="$2" display="inline" title="$1" contenteditable="false"></link-element>'); | ||
| // Images  - convert to text representation | ||
| result = result.replace(/!\[([^\]]*)\]\(([^)]+)\)/g, '[Image: $1]'); | ||
|
|
||
| return result; |
There was a problem hiding this comment.
Sanitize protocols for Markdown links ([text](url)) as well.
parseInlineMarkdown converts [text](url) into <link-element url="..."> without checking the protocol, so javascript: / data: URLs can slip through this path, unlike HTML paste which is sanitized by sanitizeAndConvertLinks.
Reusing the same safe-protocol logic (or calling a shared helper) for Markdown URLs would align behavior and close a potential injection vector.
One possible approach
- // Links [text](url) - convert to link-element
- result = result.replace(/\[([^\]]+)\]\(([^)]+)\)/g, '<link-element url="$2" display="inline" title="$1" contenteditable="false"></link-element>');
+ // Links [text](url) - convert to link-element, but only for safe protocols
+ result = result.replace(/\[([^\]]+)\]\(([^)]+)\)/g, (match, text, url) => {
+ const safe = WiskPasteHandler.normalizeUrl
+ ? WiskPasteHandler.normalizeUrl(url)
+ : url.trim();
+ // Optionally reuse the same safeProtocols check as sanitizeAndConvertLinks
+ const safeProtocols = /^(https?:|mailto:|tel:|\/|#)/i;
+ if (!safeProtocols.test(safe)) return text; // degrade to plain text
+ return `<link-element url="${safe}" display="inline" title="${text}" contenteditable="false"></link-element>`;
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| static parseInlineMarkdown(text) { | |
| if (!text) return ''; | |
| return active; | |
| let result = text; | |
| // Escape HTML entities first (but preserve existing HTML tags we want to keep) | |
| result = result.replace(/&/g, '&'); | |
| // Don't escape < and > for HTML tags we want to preserve | |
| result = result.replace(/<(?!(b|i|u|strike|code|a|br|span|strong|em)\b)/g, '<'); | |
| result = result.replace(/(?<!\b(b|i|u|strike|code|a|br|span|strong|em))>/g, '>'); | |
| // Bold and italic combined (***text*** or ___text___) | |
| result = result.replace(/\*\*\*(.+?)\*\*\*/g, '<b><i>$1</i></b>'); | |
| result = result.replace(/___(.+?)___/g, '<b><i>$1</i></b>'); | |
| // Bold (**text** or __text__) | |
| result = result.replace(/\*\*(.+?)\*\*/g, '<b>$1</b>'); | |
| result = result.replace(/__(.+?)__/g, '<b>$1</b>'); | |
| // Italic (*text* or _text_) - be careful not to match inside words | |
| result = result.replace(/\*([^*]+)\*/g, '<i>$1</i>'); | |
| result = result.replace(/(?<![a-zA-Z])_([^_]+)_(?![a-zA-Z])/g, '<i>$1</i>'); | |
| // Strikethrough (~~text~~) | |
| result = result.replace(/~~(.+?)~~/g, '<strike>$1</strike>'); | |
| // Inline code (`code`) | |
| result = result.replace(/`([^`]+)`/g, '<code style="background: var(--bg-2); padding: 2px 4px; border-radius: 3px; font-family: var(--font-mono);">$1</code>'); | |
| // Links [text](url) - convert to link-element | |
| result = result.replace(/\[([^\]]+)\]\(([^)]+)\)/g, '<link-element url="$2" display="inline" title="$1" contenteditable="false"></link-element>'); | |
| // Images  - convert to text representation | |
| result = result.replace(/!\[([^\]]*)\]\(([^)]+)\)/g, '[Image: $1]'); | |
| return result; | |
| static parseInlineMarkdown(text) { | |
| if (!text) return ''; | |
| let result = text; | |
| // Escape HTML entities first (but preserve existing HTML tags we want to keep) | |
| result = result.replace(/&/g, '&'); | |
| // Don't escape < and > for HTML tags we want to preserve | |
| result = result.replace(/<(?!(b|i|u|strike|code|a|br|span|strong|em)\b)/g, '<'); | |
| result = result.replace(/(?<!\b(b|i|u|strike|code|a|br|span|strong|em))>/g, '>'); | |
| // Bold and italic combined (***text*** or ___text___) | |
| result = result.replace(/\*\*\*(.+?)\*\*\*/g, '<b><i>$1</i></b>'); | |
| result = result.replace(/___(.+?)___/g, '<b><i>$1</i></b>'); | |
| // Bold (**text** or __text__) | |
| result = result.replace(/\*\*(.+?)\*\*/g, '<b>$1</b>'); | |
| result = result.replace(/__(.+?)__/g, '<b>$1</b>'); | |
| // Italic (*text* or _text_) - be careful not to match inside words | |
| result = result.replace(/\*([^*]+)\*/g, '<i>$1</i>'); | |
| result = result.replace(/(?<![a-zA-Z])_([^_]+)_(?![a-zA-Z])/g, '<i>$1</i>'); | |
| // Strikethrough (~~text~~) | |
| result = result.replace(/~~(.+?)~~/g, '<strike>$1</strike>'); | |
| // Inline code (`code`) | |
| result = result.replace(/`([^`]+)`/g, '<code style="background: var(--bg-2); padding: 2px 4px; border-radius: 3px; font-family: var(--font-mono);">$1</code>'); | |
| // Links [text](url) - convert to link-element, but only for safe protocols | |
| result = result.replace(/\[([^\]]+)\]\(([^)]+)\)/g, (match, text, url) => { | |
| const safe = WiskPasteHandler.normalizeUrl | |
| ? WiskPasteHandler.normalizeUrl(url) | |
| : url.trim(); | |
| // Optionally reuse the same safeProtocols check as sanitizeAndConvertLinks | |
| const safeProtocols = /^(https?:|mailto:|tel:|\/|#)/i; | |
| if (!safeProtocols.test(safe)) return text; // degrade to plain text | |
| return `<link-element url="${safe}" display="inline" title="${text}" contenteditable="false"></link-element>`; | |
| }); | |
| // Images  - convert to text representation | |
| result = result.replace(/!\[([^\]]*)\]\(([^)]+)\)/g, '[Image: $1]'); | |
| return result; |
🤖 Prompt for AI Agents
In js/paste-handler.js around lines 566 to 593, parseInlineMarkdown currently
converts Markdown links ([text](url)) straight to a <link-element> without
validating the URL scheme, allowing javascript:/data: injection; update the
function to validate/sanitize link protocols before creating the link-element —
either call the existing sanitizeAndConvertLinks/shared helper that enforces an
allowed-schemes whitelist (http, https, mailto, maybe ftp) or implement the same
safe-protocol check: parse the URL, normalize/trim it, allow only whitelisted
protocols (treat protocol-less/relative URLs as safe or resolve them to https),
and if the protocol is not allowed, do not emit a link-element (render as
escaped text or a safe placeholder) so untrusted protocols are never injected
into the url attribute of the link-element.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
js/plugins/code/main-element.js (1)
1300-1306: Remove duplicategetTextContent()method definition.The
getTextContent()method is defined twice (lines 1300-1306 and 1332-1338) with different implementations. JavaScript will use the second definition, which means the emoji is not included in the exported text content.Please clarify which implementation is intended and remove the duplicate.
🔎 Proposed resolution
If the emoji should be included in text exports, keep the first implementation and remove the second:
- getTextContent() { - return { - html: this.editable.innerHTML, - text: this.editable.innerText, - markdown: '# ' + wisk.editor.htmlToMarkdown(this.editable.innerHTML), - }; - }If the emoji should NOT be included, remove the first implementation:
- getTextContent() { - return { - html: `<h1>${this.emoji} ${this.editable.innerHTML}</h1>`, - text: `${this.emoji} ${this.editable.innerText}`, - markdown: `# ${this.emoji} ${this.editable.innerText}`, - }; - } -Also applies to: 1332-1338
js/plugins/code/numbered-list-element.js (1)
142-157:isEmptycheck should ignore structural children like<br>Using
this.editable.children.length === 0inisEmptymeans a visually empty list item that contains only a browser‑inserted<br>will be treated as non‑empty, so Enter won’t convert it back to a text block. Consider aligning with the placeholder logic pattern by treating only non‑BRchildren as content, e.g.:const hasNonBrChildren = Array.from(this.editable.children).some( el => el.tagName !== 'BR' ); const isEmpty = this.editable.innerText.trim().length === 0 && !hasNonBrChildren;This keeps behavior consistent even when the browser injects structural nodes.
js/plugins/code/base-text-element.js (1)
1394-1418:convertInlineMarkdownshould share the same safe-URL rules as paste handlers
convertInlineMarkdownnow maps[text](url)into raw<a href="$2" ...>without validating protocols:text = text.replace(/\[([^\]]+)\]\(([^)]+)\)/g, '<a href="$2" contenteditable="false" target="_blank">$1</a>');For parity with
sanitizeHtmlForClipboardandWiskPasteHandler.sanitizeAndConvertLinks, this path should also enforce an allowed-schemes whitelist (http/https/mailto/tel/relative) and drop or neutralize anything else. That keeps markdown imports from reintroducingjavascript:/data:links even when they bypass the HTML sanitizer.You can either call into the shared sanitizer or run the same protocol check before emitting
<a>.js/plugins/code/link-preview-element.js (1)
316-357: Add timeout to metadata fetch to prevent indefinite loading state.While the
noopener,noreferrersecurity issue (Line 39) has been addressed from previous reviews, the fetch operation still lacks a timeout. If the external API atrender.wisk.ccis slow or unresponsive, the component remains in "loading" state indefinitely with no user recovery option.🔎 Recommended fix with AbortController timeout
async updateLinkPreview() { if (!this.link || !this.link.trim() || this.metadata) return; if (this.status === 'loading' || this.status === 'ok' || this.status === 'error') return; this.status = 'loading'; this.showLoadingState(); this.sendUpdates(); + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), 10000); // 10 second timeout + try { let url = this.link; if (!url.startsWith('http://') && !url.startsWith('https://')) { url = 'https://' + url; } const response = await fetch('https://render.wisk.cc/fetch-metadata', { method: 'POST', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({ url }), + signal: controller.signal, }); if (!response.ok) { throw new Error('Failed to fetch metadata'); } const metadata = await response.json(); if (metadata.error) { throw new Error(metadata.error); } + clearTimeout(timeoutId); this.metadata = metadata; this.status = 'ok'; this.updatePreviewWithMetadata(metadata); this.sendUpdates(); } catch (error) { + clearTimeout(timeoutId); - console.error('Error fetching metadata:', error); + const message = error.name === 'AbortError' ? 'Request timed out' : error.message; + console.error('Error fetching metadata:', message); this.metadata = null; this.status = 'error'; this.showErrorState(); this.sendUpdates(); } }
♻️ Duplicate comments (6)
js/editor.js (4)
991-1034:deletedElementsLeftwrite is dead code
deleteBlocknow pushes into bothdeletedElementsanddeletedElementsLeft:deletedElements.push(elementId); deletedElementsLeft.push(elementId);But
deletedElementsLeftis never read anywhere in the current file; its previous uses were tied to the old batch-sync path.You can safely drop this array and the associated push to reduce noise.
419-479:handleChangesstill ignoresdata.elements.*.lastUpdatedeventsThe new
handleChangesgroups only.valueand.componentunderdata.elements.*:if (event.path.startsWith('data.elements.')) { if (event.path.includes('.value')) { ... } else if (event.path.includes('.component')) { ... } }Events on
data.elements.<id>.lastUpdatedare neither added to any list nor passed towisk.sync.applyEvent, so remotelastUpdatedtimestamps are dropped on the floor and the local document never reflects them.At minimum, those events should still be applied to
wisk.editor.document(even if the DOM doesn’t care), e.g. by:
- Detecting
.lastUpdatedin the same branch and callingwisk.sync.applyEvent, or- Falling back to a generic
applyEventfor any unclassifieddata.elements.*paths.
508-535: Remote component changes don’t update container width classIn
handleElementUpdate, a remotedata.elements.<id>.componentevent replaces the inner custom element but doesn’t touch the.rndr-full-widthclass on its container:} else if (property === 'component') { const newType = event.value.data; const newDomElement = document.createElement(newType); newDomElement.id = elementId; domElement.replaceWith(newDomElement); setTimeout(() => { newDomElement.setValue('', element.value); }, 0); }Local
changeBlockTypedoes update the container based onpluginDetail.width, but remote viewers of that change will keep whatever width class the old component had.Consider mirroring the
changeBlockTypecontainer-class logic here by:
- Looking up
wisk.plugins.getPluginDetail(newType)and- Adding/removing
rndr-full-widthon#div-${elementId}accordingly.
2582-2707: GuardgetTextContent()when updating title/name from the first elementIn
justUpdates, the “first element” special case still does:if (elementId === wisk.editor.document.data.elements[0].id) { const textContent = domElement.getTextContent().text || ''; ... }If
domElement.getTextContentis undefined or returns a non-object, this will throw and break the debounced save for all pending elements.Use a defensive read instead:
const tc = domElement.getTextContent?.(); const textContent = tc?.text || '';and only update
document.title/data.config.nameiftextContentis non-empty.js/paste-handler.js (2)
16-31: Log the correct error in the WISK clipboard fallback catchIn
parseWiskClipboard, the innercatch (fallbackError)still logs the outererror, which hides whether the base64 decode or the raw‑JSON fallback failed:} catch (fallbackError) { console.error('Failed to parse wisk clipboard data: ', error); }This should log
fallbackError(and optionally the originalerror) to distinguish which stage broke and simplify debugging.
566-593: Markdown links still bypass protocol sanitization
parseInlineMarkdownconverts[text](url)directly into<link-element url="...">without validating the URL scheme:result = result.replace(/\[([^\]]+)\]\(([^)]+)\)/g, '<link-element url="$2" display="inline" title="$1" contenteditable="false"></link-element>');This reopens the same
javascript:/data:injection vector thatsanitizeAndConvertLinksexplicitly guards against for HTML anchors. Please reuse the same safe‑protocol logic here (or call a shared helper), e.g.:result = result.replace(/\[([^\]]+)\]\(([^)]+)\)/g, (match, text, url) => { const raw = url.trim(); const safe = WiskPasteHandler.normalizeUrl ? WiskPasteHandler.normalizeUrl(raw) : raw; const safeProtocols = /^(https?:|mailto:|tel:|\/|#)/i; if (!safeProtocols.test(safe)) return text; // degrade to plain text return `<link-element url="${safe}" display="inline" title="${text}" contenteditable="false"></link-element>`; });
🧹 Nitpick comments (3)
js/plugins/code/main-element.js (1)
1253-1253: Simplify the event name selection logic.The array
.find()pattern on line 1253 is unnecessarily complex. This can be simplified to a direct ternary expression for better readability.🔎 Proposed simplification
- const eventName = ['input', 'change'].find(e => e === (/^(text|number|url|email|phone)$/.test(prop.type) ? 'input' : 'change')); + const eventName = /^(text|number|url|email|phone)$/.test(prop.type) ? 'input' : 'change';js/plugins/code/link-preview-element.js (2)
48-79: Consider simplifying nested conditions in arrow-key navigation.The keyboard navigation logic is functionally correct, but lines 63 and 71 contain redundant nested checks (the outer
ifalready ensuresselectionStart === 0for ArrowLeft at line 62, andselectionStart === value.lengthfor ArrowRight at line 70).🔎 Proposed simplification
} else if (event.key === 'ArrowUp' || (event.key === 'ArrowLeft' && this.urlInput.selectionStart === 0)) { - if (this.urlInput.selectionStart === 0) { - event.preventDefault(); - const prevElement = wisk.editor.prevElement(this.id); - if (prevElement) { - wisk.editor.focusBlock(prevElement.id, { x: prevElement.value?.textContent?.length || 0 }); - } + event.preventDefault(); + const prevElement = wisk.editor.prevElement(this.id); + if (prevElement) { + wisk.editor.focusBlock(prevElement.id, { x: prevElement.value?.textContent?.length || 0 }); } } else if (event.key === 'ArrowDown' || (event.key === 'ArrowRight' && this.urlInput.selectionStart === this.urlInput.value.length)) { - if (this.urlInput.selectionStart === this.urlInput.value.length) { - event.preventDefault(); - const nextElement = wisk.editor.nextElement(this.id); - if (nextElement) { - wisk.editor.focusBlock(nextElement.id, { x: 0 }); - } + event.preventDefault(); + const nextElement = wisk.editor.nextElement(this.id); + if (nextElement) { + wisk.editor.focusBlock(nextElement.id, { x: 0 }); } }
117-138: Consider renamingnormalizedand adding URL validation.Line 121 assigns the trimmed URL to a variable named
normalized, but no actual normalization occurs. This is misleading.Additionally, there's no URL format validation before accepting the input. While invalid URLs are handled gracefully during fetch (lines 336-356), early validation could improve UX.
🔎 Proposed improvements
submitUrl() { const url = this.urlInput.value.trim(); if (!url) return; - const normalized = url; + const newUrl = url; - if (normalized !== this.link) { - this.link = normalized; + if (newUrl !== this.link) { + this.link = newUrl; this.metadata = null; this.status = 'idle'; } else { if (!this.metadata) this.status = 'idle'; }For optional URL validation, you could add a helper method:
_isValidUrl(str) { try { const url = new URL(str.startsWith('http') ? str : 'https://' + str); return url.protocol === 'http:' || url.protocol === 'https:'; } catch { return false; } }Then validate before assignment.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
js/editor.jsjs/paste-handler.jsjs/plugins/code/base-text-element.jsjs/plugins/code/embed-element.jsjs/plugins/code/link-preview-element.jsjs/plugins/code/list-element.jsjs/plugins/code/main-element.jsjs/plugins/code/numbered-list-element.jsjs/plugins/code/text-element.jsstyle.css
🚧 Files skipped from review as they are similar to previous changes (1)
- js/plugins/code/text-element.js
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-12T14:14:36.148Z
Learnt from: sushilbang
Repo: sohzm/wisk PR: 54
File: js/plugins/code/base-text-element.js:58-62
Timestamp: 2025-12-12T14:14:36.148Z
Learning: In the js/plugins/code directory, ensure that BaseTextElement serves as the shared base class with common state/logic, while concrete elements (e.g., TextElement) extend it and override render() to provide UI markup. The base class's connectedCallback should query for child elements that are injected by their render methods. This inheritance pattern should be consistently applied across the plugin system; review new or existing elements to maintain a clean base-vs-extension separation, avoid tight coupling, and verify that the render contracts and DOM querying align between base and derived classes.
Applied to files:
js/plugins/code/list-element.jsjs/plugins/code/base-text-element.jsjs/plugins/code/embed-element.jsjs/plugins/code/link-preview-element.jsjs/plugins/code/numbered-list-element.jsjs/plugins/code/main-element.js
🧬 Code graph analysis (6)
js/plugins/code/list-element.js (2)
js/wisk.js (1)
wisk(3-3)js/editor.js (2)
prevElement(322-322)prevElement(2277-2277)
js/plugins/code/base-text-element.js (1)
js/wisk.js (1)
wisk(3-3)
js/plugins/code/link-preview-element.js (2)
js/editor.js (7)
prevElement(322-322)prevElement(2277-2277)nextElement(1424-1424)nextElement(2279-2279)elementToFocus(637-637)elementToFocus(747-747)elementToFocus(2275-2275)js/wisk.js (1)
wisk(3-3)
js/editor.js (3)
js/sync/sync.js (5)
elementId(212-212)key(207-207)pathParts(172-172)element(213-213)i(174-174)js/wisk.js (1)
wisk(3-3)js/storage/db.js (1)
data(272-272)
js/paste-handler.js (1)
js/wisk.js (1)
wisk(3-3)
js/plugins/code/numbered-list-element.js (1)
js/editor.js (2)
prevElement(322-322)prevElement(2277-2277)
🔇 Additional comments (13)
js/plugins/code/list-element.js (1)
105-117: Scoped prev-element lookup and dot alignment look correctMoving
prevDomElementinside theprevElementguard avoids accidental use when no previous block exists, and the dot’s vertical offset change is purely visual with no behavioral impact. Nothing blocking here.Also applies to: 145-152
style.css (1)
24-24: Drag affordances and touch tweaks are consistent with JS drag stateThe added
.drag-handle/body.is-draggingcursors and touch-related flags on.hover-imageline up with the drag state management injs/editor.jsand should improve DnD UX on both mouse and touch. No functional issues spotted.Also applies to: 190-202, 208-223
js/plugins/code/base-text-element.js (1)
973-1107: Paste-link menu, copy, and paste integration look consistentThe new paste-link flow (URL detection in
handlePaste, inlinelink-elementplaceholder +.paste-link-menu, and conversion tolink-preview-element/embed-element) hangs together well:
- Menu state is localized (
showingPasteLinkMenu,pendingPasteUrl,_pendingLinkElement) and cleaned up viahidePasteLinkMenuand a scopedmousedownlistener.- Keyboard handling (arrows/Enter/Escape) is blocked early in
handleKeyDownwhile the menu is open, which prevents interference with normal editing.handleCopy’s translation of<link-element>to<a>withdata-wisk-*attributes provides good interop with external apps while keeping round‑trip metadata.- Delegating rich paste flows to
WiskPasteHandler.handleTextElementPastekeeps the base element lean.No blocking issues here.
Also applies to: 1763-1837
js/editor.js (1)
125-186: Config, access, public status, and plugin-data mutators align with event-driven syncThe refactored
addConfigChangeplus the new helpers (addUserAccess,removeUserAccess,setPublicStatus,setDatabaseProp,savePluginData) consistently:
- Emit timestamped
wisk.sync.newChangeevents on the appropriatedata.config.*/data.pluginData.*paths.- Guard keys/identifiers against
./[]to avoid path-injection.- Call
wisk.sync.saveModification()immediately for config and plugin data, which makes these mutations durable.This matches the new event-log architecture and looks solid.
Also applies to: 188-276, 278-298
js/plugins/code/embed-element.js (1)
9-190: EmbedElement lifecycle and URL handling look solidThe revised
EmbedElement:
- Properly caches shadow DOM elements in
connectedCallback, binds/unbinds listeners, and callsupdateView()so earlysetValuecalls aren’t lost.- Normalizes user input and pasted values via
extractSrcFromIframeandconvertToEmbedUrl, forcinghttps://where needed and handling common providers (YouTube, Maps, Drive, Gist).- Integrates cleanly with the editor: Backspace/Delete remove the block and move focus, Enter inserts a text block, and Arrow navigation respects prev/next blocks.
- Uses
focus()to route focus either to the container (when embedded) or to the URL input (when showing the dialog), which matches expected UX.No blocking issues here.
Also applies to: 223-259, 260-369
js/plugins/code/link-preview-element.js (8)
1-25: LGTM! Constructor and lifecycle setup is well-structured.The initialization sequence is sound: the constructor sets up initial state and renders, while
connectedCallbackproperly queries DOM elements, binds events, and conditionally triggers metadata fetch. The_hasConnectedflag is a good guard for deferred initialization.
27-115: LGTM! Event listener lifecycle is now correctly managed.The refactor to store handlers as properties (
_onOuterClick,_onUrlInputKeyDown, etc.) and systematically remove them in_removeAllListenerssuccessfully addresses the previous event leak concern. The pattern is solid: remove old listeners before rebinding (line 28), guard against missing elements, and clean up on disconnect.
33-41: LGTM! External navigation is now properly secured.The addition of
'noopener,noreferrer'on Line 39 correctly addresses the previous security concern. URL normalization with protocol prepending is appropriate for this use case.
140-178: LGTM! Keyboard navigation for the preview state is well-implemented.The
handleKeyDownmethod correctly implements block-level navigation (arrows), deletion (Backspace/Delete), and new block creation (Enter). The logic properly prevents defaults, focuses appropriate blocks with correct cursor positions, and follows the editor's navigation patterns.
267-414: LGTM! Metadata rendering and state management are robust.The
updatePreviewWithMetadata,showLoadingState, andshowErrorStatemethods handle edge cases well:
- Null checks prevent errors on missing DOM elements (lines 273, 277, 286, 298, etc.)
- Favicon
onerrorhandler (lines 290-292) prevents broken images- Error state displays the URL (lines 399-401) as a helpful fallback
- Loading and error states correctly toggle display properties
243-265: LGTM! State machine logic in updateView is well-structured.The
updateViewmethod correctly implements the two-pane toggle and status-based rendering:
- Empty link → input dialog (lines 261-264)
- Link with metadata → preview with metadata (lines 250-253)
- Link with error status → error state (lines 255-258)
- Link without metadata → loading state (line 260)
The state machine transitions are clear and complete.
422-564: LGTM! Rendering and registration are correctly implemented.The
rendermethod properly sets up the shadow DOM with:
- Well-structured CSS using CSS custom properties for theming consistency
- Two-pane HTML structure (input-dialog and preview-content) matching the architectural design
- Appropriate use of semantic elements and CSS classes
Custom element registration (line 564) is correct.
192-233: The guards inupdateLinkPrevieware sufficient; no race condition exists.Lines 230-232 and the guard at line 318 prevent multiple fetches. When
updateLinkPreviewis called, it immediately setsstatus = 'loading'(line 320) before any async operations begin. Any subsequentsetValuecall, even if made in rapid succession, will fail the guard check at line 318 (if (this.status === 'loading'...) return;) and exit early. Since the status transition happens synchronously with no gap between the guard check and the assignment, multiple fetch requests cannot be queued.Likely an incorrect or invalid review comment.
| wisk.editor.renumberNumberedLists = function() { | ||
| const elements = wisk.editor.document.data.elements; | ||
| const counters = [0]; // Stack for tracking numbers at each indent level | ||
|
|
||
| for (let i = 0; i < elements.length; i++) { | ||
| const el = elements[i]; | ||
| if (el.component === 'numbered-list-element') { | ||
| const domEl = document.getElementById(el.id); | ||
| if (!domEl) continue; | ||
|
|
||
| const indent = domEl.indent || 0; | ||
|
|
||
| // Adjust counter stack for current indent level | ||
| while (counters.length <= indent) { | ||
| counters.push(0); | ||
| } | ||
| while (counters.length > indent + 1) { | ||
| counters.pop(); | ||
| } | ||
|
|
||
| // Increment counter for current level | ||
| counters[indent]++; | ||
|
|
||
| // Update the element's number | ||
| const newNumber = counters[indent]; | ||
| if (domEl.number !== newNumber) { | ||
| domEl.number = newNumber; | ||
| domEl.updateNumber(); | ||
| el.value.number = newNumber; | ||
| } | ||
| } else { | ||
| // Reset counters when encountering non-numbered-list element | ||
| counters.length = 1; | ||
| counters[0] = 0; | ||
| } | ||
| } | ||
|
|
||
| // Save modifications | ||
| wisk.editor.justUpdates(elements[0]?.id); | ||
| } |
There was a problem hiding this comment.
renumberNumberedLists doesn’t persist updated numbers for all items
renumberNumberedLists correctly recomputes number based on indentation and updates both domEl.number and el.value.number for each numbered-list-element, but at the end it only calls:
wisk.editor.justUpdates(elements[0]?.id);justUpdates creates events by reading domElement.getValue() for each element in elementUpdatesNeeded. Since only the first element ID is enqueued here, renumbered items at other positions will have their updated number reflected in the DOM but not in the persisted document or remote copies.
A safer pattern is to enqueue every list element whose number actually changed, for example:
const changedIds = new Set();
// inside the loop when domEl.number !== newNumber:
changedIds.add(el.id);
// after the loop:
changedIds.forEach(id => wisk.editor.justUpdates(id));This ensures all renumbered bullets persist and synchronize correctly.
🤖 Prompt for AI Agents
In js/editor.js around lines 1810 to 1849, renumberNumberedLists updates
domEl.number and el.value.number but only calls
wisk.editor.justUpdates(elements[0]?.id) so only the first changed element is
persisted; collect the IDs of every element whose number actually changed (e.g.
use a Set changedIds and add el.id when domEl.number !== newNumber), and after
the loop iterate changedIds calling wisk.editor.justUpdates(id) for each so all
renumbered items are enqueued for persistence/sync.
| } else if (element.elementName === 'image-element') { | ||
| flattenedElements.push({ | ||
| elementName: element.elementName, | ||
| value: { | ||
| imageUrl: element.value, | ||
| textContent: '', | ||
| }, | ||
| }); | ||
| } else if (element.elementName === 'table-element') { | ||
| flattenedElements.push({ | ||
| elementName: element.elementName, | ||
| value: element.value, | ||
| }); | ||
| } else if (element.elementName === 'code-element') { |
There was a problem hiding this comment.
flattenElements mishandles image-element values
processNode('img') and other flows set element.value to an object { imageUrl, textContent }, but flattenElements treats it as a scalar:
} else if (element.elementName === 'image-element') {
flattenedElements.push({
elementName: element.elementName,
value: {
imageUrl: element.value,
textContent: '',
},
});
}This produces imageUrl: [object Object] and drops any existing textContent. You likely want to preserve the object:
} else if (element.elementName === 'image-element') {
const val = element.value || {};
flattenedElements.push({
elementName: element.elementName,
value: {
imageUrl: val.imageUrl || '',
textContent: val.textContent || '',
},
});
}🤖 Prompt for AI Agents
In js/paste-handler.js around lines 493 to 506, flattenElements treats
element.value for 'image-element' as a scalar and wraps it into imageUrl,
causing imageUrl to become "[object Object]" and dropping any existing
textContent; update the branch to read element.value as an object (guarding for
null/undefined), extract imageUrl and textContent from it (defaulting to empty
strings), and push an object with those two fields so existing imageUrl and
textContent are preserved.
| updatePlaceholder() { | ||
| if (this.editable) { | ||
| const isEmpty = this.editable.innerText.trim() === ''; | ||
| const hasText = this.editable.innerText.trim() !== ''; | ||
| const hasChildElements = this.editable.childElementCount > 0; | ||
| const isEmpty = !hasText && !hasChildElements; | ||
| this.editable.classList.toggle('empty', isEmpty); |
There was a problem hiding this comment.
Placeholder detection will treat structural children as non-empty
updatePlaceholder now uses:
const hasText = this.editable.innerText.trim() !== '';
const hasChildElements = this.editable.childElementCount > 0;
const isEmpty = !hasText && !hasChildElements;Browsers often insert <br> or similar structural nodes into an otherwise empty contenteditable, so childElementCount > 0 can make isEmpty false even when the block looks blank. That can prevent the placeholder from ever appearing.
Consider following the earlier pattern of ignoring “structural only” children, e.g.:
const hasChildElements = Array.from(this.editable.children).some(
el => el.tagName !== 'BR'
);
const isEmpty = !hasText && !hasChildElements;This keeps placeholders aligned with what users see.
🤖 Prompt for AI Agents
In js/plugins/code/base-text-element.js around lines 1601 to 1606, the
placeholder detection currently treats any child element (e.g. browser-inserted
<br>) as non-empty which prevents the placeholder from showing; change the child
check to ignore structural-only children by checking this.editable.children and
treating elements like BR (and other known structural-only tags if needed) as
non-counting, then compute isEmpty = !hasText && !hasChildElements so that only
meaningful child elements mark the node as non-empty.
| .empty-emoji { | ||
| background: var(--bg-2) !important; | ||
| opacity: 0; | ||
| padding: 8px 12px; | ||
| } | ||
| .header-container:hover .empty-emoji { | ||
| opacity: 1; | ||
| background: var(--bg-2) | ||
| } |
There was a problem hiding this comment.
Fix missing semicolon in CSS.
Line 675 is missing a semicolon after var(--bg-2), which is a CSS syntax error.
🔎 Proposed fix
.empty-emoji {
opacity: 0;
padding: 8px 12px;
}
.header-container:hover .empty-emoji {
opacity: 1;
- background: var(--bg-2)
+ background: var(--bg-2);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .empty-emoji { | |
| background: var(--bg-2) !important; | |
| opacity: 0; | |
| padding: 8px 12px; | |
| } | |
| .header-container:hover .empty-emoji { | |
| opacity: 1; | |
| background: var(--bg-2) | |
| } | |
| .empty-emoji { | |
| opacity: 0; | |
| padding: 8px 12px; | |
| } | |
| .header-container:hover .empty-emoji { | |
| opacity: 1; | |
| background: var(--bg-2); | |
| } |
🤖 Prompt for AI Agents
In js/plugins/code/main-element.js around lines 669 to 676, the CSS rule
".header-container:hover .empty-emoji { background: var(--bg-2) }" is missing a
trailing semicolon; add a semicolon after var(--bg-2) (i.e., "background:
var(--bg-2);") to fix the syntax error and ensure proper CSS parsing.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
js/plugins/code/table-element.js (1)
133-139: Remove duplicate CSS declarations.Lines 133-135 declare
max-width,min-width, andwidthfortd, but lines 137-139 immediately redeclare the same properties with different values. The second declaration overrides the first, making lines 133-135 ineffective and confusing.🔎 Proposed fix
td { height: 40px; - max-width: 400px; - min-width: 100px; - width: auto; - min-width: 200px; max-width: 400px; width: auto; }
♻️ Duplicate comments (5)
js/editor.js (5)
992-1037: Remove unuseddeletedElementsLeftvariable.The
deleteBlockfunction correctly emits deletion events and persists changes, but line 1024 still pushes todeletedElementsLeft, which was flagged in a previous review as unused. This variable (declared at line 4) should be removed along with all references to clean up dead code.
1204-1204: Template application won't persist without element IDs.At line 1204,
useTemplatecallswisk.editor.justUpdates()with no arguments after loading template elements. The newjustUpdatesimplementation (line 2620-2622) returns early whenelementIdis undefined, so template changes won't be persisted.Consider either:
- Looping over template elements and calling
justUpdates(element.id)for each, or- Detecting the no-argument case and enqueuing all elements:
if (!elementId) { // enqueue all elements for update wisk.editor.document.data.elements.forEach(e => { if (e.id) elementUpdatesNeeded.add(e.id); }); return; }
1821-1860: Only first renumbered element is persisted.The
renumberNumberedListsfunction updates all list elements' numbers correctly, but line 1859 only callswisk.editor.justUpdates(elements[0]?.id). This means only the first element's changes are enqueued for persistence. As noted in a previous review, you should collect all changed element IDs and calljustUpdatesfor each:const changedIds = new Set(); // Inside the loop when updating: if (domEl.number !== newNumber) { domEl.number = newNumber; domEl.updateNumber(); el.value.number = newNumber; changedIds.add(el.id); } // After the loop: changedIds.forEach(id => wisk.editor.justUpdates(id));
2647-2719: Consider beforeunload handler to prevent data loss.The 300ms debounce (line 2647) creates a window where pending updates in
elementUpdatesNeededcould be lost if the user closes the tab or navigates away. While the inline comment (line 2643) acknowledges this, adding abeforeunloadhandler to flush pending updates would reduce the risk:window.addEventListener('beforeunload', (e) => { if (elementUpdatesNeeded.size > 0) { clearTimeout(debounceTimer); const timestamp = Date.now(); elementUpdatesNeeded.forEach(elementId => { const domElement = document.getElementById(elementId); if (domElement) { wisk.sync.newChange({ path: `data.elements.${elementId}.value`, value: { data: domElement.getValue(), timestamp, agent: wisk.sync.agent } }); } }); // Note: Can't reliably await in beforeunload } });Alternatively, consider reducing the debounce to 100-150ms to narrow the loss window.
2693-2707: Guard against null getTextContent() for first element.Line 2694 accesses
domElement.getTextContent().textwithout checking ifgetTextContent()returns a valid object. If the method is undefined or returns null, this will throw. Use optional chaining:const textContentObj = domElement.getTextContent?.(); const textContent = textContentObj?.text || ''; if (textContent) { document.title = textContent; wisk.sync.newChange({ path: 'data.config.name', value: { data: textContent, timestamp: timestamp, agent: wisk.sync.agent } }); }
🧹 Nitpick comments (1)
js/theme/theme.js (1)
54-62: Consider revoking old favicon blob URLs to prevent memory leaks.Each theme change creates a new blob URL without revoking the previous one. While theme changes are infrequent, storing the previous URL and calling
URL.revokeObjectURL()before creating a new one would prevent gradual memory accumulation in long-running sessions.🔎 Proposed enhancement
Track and revoke the previous blob URL:
+let currentFaviconBlobUrl = null; + wisk.theme.setTheme = async function (themeName) { // ... existing code ... let favicon = document.querySelector('link[rel="icon"]'); if (!favicon) { favicon = document.createElement('link'); favicon.rel = 'icon'; favicon.type = 'image/svg+xml'; document.head.appendChild(favicon); } + if (currentFaviconBlobUrl) { + URL.revokeObjectURL(currentFaviconBlobUrl); + } - favicon.href = createThemedFaviconSVG(textColor, bgColor); + currentFaviconBlobUrl = createThemedFaviconSVG(textColor, bgColor); + favicon.href = currentFaviconBlobUrl; // ... rest of function ... };
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
css/pages.cssjs/editor.jsjs/elements/command-palette.jsjs/left-sidebar.jsjs/plugins/code/super-checkbox.jsjs/plugins/code/table-element.jsjs/theme/theme.jsstyle.css
🚧 Files skipped from review as they are similar to previous changes (1)
- style.css
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-12T14:14:36.148Z
Learnt from: sushilbang
Repo: sohzm/wisk PR: 54
File: js/plugins/code/base-text-element.js:58-62
Timestamp: 2025-12-12T14:14:36.148Z
Learning: In the js/plugins/code directory, ensure that BaseTextElement serves as the shared base class with common state/logic, while concrete elements (e.g., TextElement) extend it and override render() to provide UI markup. The base class's connectedCallback should query for child elements that are injected by their render methods. This inheritance pattern should be consistently applied across the plugin system; review new or existing elements to maintain a clean base-vs-extension separation, avoid tight coupling, and verify that the render contracts and DOM querying align between base and derived classes.
Applied to files:
js/plugins/code/table-element.jsjs/plugins/code/super-checkbox.js
🧬 Code graph analysis (3)
js/left-sidebar.js (2)
js/right-sidebar.js (7)
startX(28-28)startWidth(29-29)sidebar(15-15)sidebar(64-64)sidebar(83-83)sidebar(92-92)sidebar(164-164)script.js (1)
sidebar(10-10)
js/theme/theme.js (1)
js/wisk.js (1)
wisk(3-3)
js/editor.js (2)
js/sync/sync.js (4)
elementId(212-212)pathParts(172-172)element(213-213)i(174-174)js/left-sidebar.js (1)
latestClientX(31-31)
🔇 Additional comments (27)
js/plugins/code/table-element.js (2)
199-199: LGTM! Good refactoring to eliminate code duplication.Extracting the inline Markdown generation logic into the
getMarkdownText()method improves maintainability and follows the DRY principle.
203-248: LGTM! Well-implemented Markdown table generation.The
getMarkdownText()method correctly handles:
- Edge cases with empty headers/rows (lines 206-208)
- Null/undefined cell values with the
??operator (lines 214, 224, 238)- Proper escaping of Markdown special characters
- Column width calculation and alignment
css/pages.css (1)
12-13: LGTM! CSS variable migration enables theming.The migration to CSS variables with fallback values is well-executed and aligns with the broader theme customization work in this PR.
js/left-sidebar.js (2)
30-31: LGTM! RAF throttling pattern aligns with project-wide improvements.The introduction of
resizeScheduledandlatestClientXimplements proper requestAnimationFrame throttling to prevent excessive reflows during resize operations, consistent with similar patterns in js/editor.js.
47-60: LGTM! RAF-throttled resize correctly batches DOM updates.The resize function properly accumulates mouse position and applies width updates in a single requestAnimationFrame callback. The calculation from the original
startXthroughout the drag session is correct—it ensures the width reflects the total displacement from the drag start point.js/elements/command-palette.js (1)
13-13: LGTM! Migration to modern key property improves cross-browser compatibility.The replacement of deprecated
keyCodechecks withe.keystring comparisons is correct and improves code readability. All key mappings are accurate (80→'p', 27→'Escape', 13→'Enter', 38→'ArrowUp', 40→'ArrowDown').Also applies to: 49-49, 54-54, 58-58, 68-68, 76-76
js/plugins/code/super-checkbox.js (1)
14-14: LGTM! Bound handler pattern ensures proper lifecycle management.Pre-binding
onCheckboxChangein the constructor and using the same reference for addEventListener/removeEventListener ensures proper cleanup. The guard check indisconnectedCallbackprevents errors if the checkbox wasn't initialized.Also applies to: 36-36, 43-45
js/editor.js (17)
24-24: LGTM! Drag state management and touch handling improvements.The addition of
dragUpdateScheduledfor RAF throttling and enhanced touch event handling with proper passive settings and drag state guards are well-implemented. The!dragStatecheck on touchend correctly prevents accidental clicks when a drag operation is in progress.Also applies to: 41-41, 58-67
126-187: LGTM! Config mutations now emit timestamped events.The refactored
addConfigChangeproperly wraps each case in a block (preventing scope leakage), generates timestamps, and emits events through the new event-driven architecture before persisting. The switch cases now correctly handle config.name, config.theme, and config.plugins mutations.
189-277: LGTM! New config mutators with proper validation and event emission.The new functions (
addUserAccess,removeUserAccess,setPublicStatus,setDatabaseProp) follow a consistent pattern: validate input, initialize data structures if needed, emit timestamped events, and persist immediately. The key validation insetDatabaseProp(line 255) correctly prevents path injection by rejecting keys containing.,[, or].
279-299: LGTM! Plugin data mutations sanitized and event-driven.
savePluginDatacorrectly validates the identifier (line 281-284) to prevent path injection, emits timestamped events, and persists immediately. The validation pattern matchessetDatabasePropand effectively blocks malicious path traversal.
320-356: LGTM! Block creation now emits events and uses queued persistence.The addition of
lastUpdatedtimestamp (line 321) and the event emission for element creation and order changes (lines 337-356) integrate well with the event-driven architecture. The use ofwisk.sync.enqueueSave('block-creation')instead of directsaveModificationaddresses the previous concern about overlapping saves by centralizing persistence through a queue.
420-480: LGTM! Event-driven architecture with specialized handlers.The refactored
handleChangesproperly groups events by type and delegates to specialized handlers (handleElementCreation,handleElementUpdate,handleElementDeletions, etc.). This provides clear separation of concerns and makes the event processing flow easier to follow.
483-508: LGTM! Specialized event handlers properly apply mutations.
handleElementCreation,handleElementDeletions,handleConfigChange, andhandlePluginDataChangecorrectly apply events to the document and perform corresponding DOM/UI updates. The duplication check inhandleElementCreation(line 486) prevents double-creation.Also applies to: 539-605
686-695: LGTM! Block moves now emit order events and use queued persistence.The moveBlock function correctly emits a
data.elementOrderevent with timestamps and usesenqueueSave('block-move')to queue the persistence, avoiding overlapping saves.
1071-1145: LGTM! Block type changes emit events and update width classes.The refactored
changeBlockTypefunction now:
- Emits three timestamped events (component, value, lastUpdated)
- Correctly updates the container's
rndr-full-widthclass based on the new plugin's width property (lines 1120-1128)- Persists changes asynchronously
This addresses the previous review concern about missing width class updates on component changes.
1669-1729: LGTM! Enhanced drag lifecycle with proper cleanup.The drag handling now includes:
- Escape key to cancel drag (lines 1700-1704)
- Comprehensive cleanup in
cleanupDragthat removes listeners, clears intervals, and resets state- Proper event listener management for blur/keydown/mouse/touch events
This ensures drag operations can be cleanly cancelled and don't leak resources.
1731-1757: LGTM! RAF-throttled drag updates prevent performance issues.The
handleDragfunction correctly implements requestAnimationFrame throttling: it accumulates the latest mouse position indragState, guards against multiple RAF calls withdragUpdateScheduled, and performs all DOM updates in a single RAF callback. This pattern matches the improvements in left-sidebar.js and reduces reflows during drag operations.
2075-2097: LGTM! Shadow root handling correctly isolates editable content.The
getTextNodeshelper (lines 2084-2089) properly queries for the#editableelement within shadow roots, ensuring selection overlays only highlight actual editable content rather than the entire shadow tree (e.g., buttons, controls). This aligns with the component structure described in the retrieved learnings about BaseTextElement.
2317-2367: LGTM! HTML sanitization blocks dangerous protocols.The
sanitizeHtmlForClipboardfunction correctly:
- Strips all link attributes and rebuilds them safely
- Validates
hrefprotocols against a whitelist (http:, https:, mailto:, tel:)- Rejects dangerous protocols (javascript:, data:, vbscript:) that could enable XSS
- Handles URL parsing errors gracefully
This addresses the previous security concern about unsanitized clipboard HTML.
2382-2420: LGTM! Clipboard serialization prevents corruption.The use of sanitizeHtmlForClipboard (lines 2385-2387) and Base64 encoding (lines 2417-2420) prevents both XSS attacks and JSON corruption from HTML entities. The encoding pattern
btoa(unescape(encodeURIComponent(jsonString)))correctly handles UTF-8 characters.
901-948: LGTM! Switch case blocks prevent variable scope leakage.All switch cases that declare variables (cite-element, a, sup, span, etc.) are now properly wrapped in block scopes. This prevents const/let declarations from leaking to other cases, addressing the previous review concern about the
supcase (lines 928-936).
1395-1421: LGTM! Markdown conversion order prevents conflicts.The reordering of inline markdown transformations is well-documented and correct:
- Citations first (to avoid link regex interference)
- Code spans second (to preserve literal content)
- Formatting (bold/italic/strike)
- Links (before reference markers)
- Reference numbers last
This sequence prevents regex conflicts and ensures proper nesting of elements.
2593-2622: LGTM! Event routing and nested element handling.The new
justUpdatessignature correctly handles:
- Event objects passed directly (critical path with immediate save)
- Nested element IDs (delegates to parent editor)
- Invalid inputs (early return)
The pattern of adding to
elementUpdatesNeededSet and deferring event creation to the debounce callback is a clean separation.js/theme/theme.js (3)
35-40: Good improvement: Prevents duplicate style elements.Reusing the existing style element avoids accumulating multiple identical
<style>tags in the DOM on repeated theme changes.
54-60: Good improvement: Reuses existing favicon element.This prevents duplicate
<link rel="icon">elements from being appended on repeated theme changes.
136-142: LGTM: Proper async handling in theme initialization.The loop correctly uses a local
themeNamevariable to avoid closure issues, and the registered callbacks properly await bothsetThemeandaddConfigChange.
| const handleElementUpdate = async (event) => { | ||
| const pathParts = event.path.split('.'); | ||
| const elementId = pathParts[2]; | ||
| const property = pathParts[3]; // 'value', 'component', 'lastUpdated' | ||
|
|
||
| if (domElement.tagName.toLowerCase() !== updatedElement.component) { | ||
| const prevElement = wisk.editor.prevElement(updatedElement.id); | ||
| if (prevElement) { | ||
| wisk.editor.changeBlockType(updatedElement.id, updatedElement.value, updatedElement.component, rec); | ||
| } | ||
| } else { | ||
| const element = wisk.editor.getElement(elementId); | ||
| const domElement = document.getElementById(elementId); | ||
|
|
||
| if (!element || !domElement) { | ||
| console.warn(`Element ${elementId} not found for update`); | ||
| return; | ||
| } | ||
| wisk.sync.applyEvent(wisk.editor.document, event); | ||
| if (property === 'value') { | ||
| setTimeout(() => { | ||
| domElement.setValue('', event.value.data); | ||
| }, 0); | ||
| } else if (property === 'component') { | ||
| const newType = event.value.data; | ||
| const newDomElement = document.createElement(newType); | ||
| newDomElement.id = elementId; | ||
| domElement.replaceWith(newDomElement); | ||
| setTimeout(() => { | ||
| domElement.setValue('', updatedElement.value); | ||
| newDomElement.setValue('', element.value); | ||
| }, 0); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Missing width class update on component change.
When handleElementUpdate processes a component change (line 527-535), it replaces the DOM element but doesn't update the container's rndr-full-width class based on the new plugin's width property. This is the same issue flagged in a previous review for changeBlockType (lines 1114-1137), which was reportedly fixed.
A past review comment indicated this should be addressed, but the current code still lacks the width class logic here. Consider adding:
} else if (property === 'component') {
const newType = event.value.data;
const newDomElement = document.createElement(newType);
newDomElement.id = elementId;
domElement.replaceWith(newDomElement);
// Update container width class
const container = document.getElementById(`div-${elementId}`);
if (container) {
const pluginDetail = wisk.plugins.getPluginDetail(newType);
if (pluginDetail.width === 'max') {
container.classList.add('rndr-full-width');
} else {
container.classList.remove('rndr-full-width');
}
}
setTimeout(() => {
newDomElement.setValue('', element.value);
}, 0);
}🤖 Prompt for AI Agents
In js/editor.js around lines 510 to 536, when handling a component change the
code replaces the DOM element but omits updating the container's rndr-full-width
class based on the new plugin's width; add logic after creating/replacing the
new element to find the container (id `div-<elementId>`), fetch plugin details
via wisk.plugins.getPluginDetail(newType), and add or remove the rndr-full-width
class depending on whether pluginDetail.width === 'max' before calling setValue
so the container width reflects the new component type.
| const themeName = themeData.name; | ||
| wisk.editor.registerCommand(themeName, '', 'Theme', async () => { | ||
| wisk.theme.setTheme(themeName); | ||
| await wisk.editor.addConfigChange('document.config.theme', themeName); | ||
| }, ''); | ||
|
|
||
| console.log('Theme "' + themeData.name + '" added successfully'); | ||
| wisk.theme.setTheme(themeData.name); | ||
| console.log('Theme "' + themeName + '" added successfully'); | ||
| wisk.theme.setTheme(themeName); | ||
| wisk.editor.addConfigChange('document.config.theme', themeName); |
There was a problem hiding this comment.
Missing await on async operations causes race condition.
Lines 112-113 call setTheme and addConfigChange without awaiting them, even though:
setThemeis declaredasync(line 3)addConfigChangeis awaited in the registered command callbacks (lines 108, 113, 140)initThemeproperly awaitssetTheme(line 143)
This inconsistency can cause the config change to be lost or not persisted before addTheme returns, leading to data inconsistency.
🔎 Proposed fix
-wisk.theme.addTheme = function (themeData) {
+wisk.theme.addTheme = async function (themeData) {
try {
themeData = JSON.parse(themeData);
} catch (error) {
@@ -109,8 +109,8 @@
}, '');
console.log('Theme "' + themeName + '" added successfully');
- wisk.theme.setTheme(themeName);
- wisk.editor.addConfigChange('document.config.theme', themeName);
+ await wisk.theme.setTheme(themeName);
+ await wisk.editor.addConfigChange('document.config.theme', themeName);
return true;
};🤖 Prompt for AI Agents
In js/theme/theme.js around lines 105 to 113, the calls to
wisk.theme.setTheme(themeName) and
wisk.editor.addConfigChange('document.config.theme', themeName) are invoked
without awaiting them, which can cause a race where the config change is not
persisted; change these calls to await wisk.theme.setTheme(themeName) and await
wisk.editor.addConfigChange('document.config.theme', themeName), ensure the
enclosing function is declared async if it isn’t already, and keep the
console.log after the awaited calls so it only runs once the operations
complete.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
js/editor.js (1)
1206-1206: Template changes won't persist due to empty justUpdates() call.Line 1206 calls
wisk.editor.justUpdates()with no arguments after replacing document elements with template data. With the new implementation, this returns early (line 2625 in justUpdates), so template changes are never persisted to the event log or saved.This was flagged in a previous review but remains unresolved.
🔎 Possible fixes
Option 1: Loop through template elements
wisk.theme.setTheme(template.theme); wisk.editor.document.data.elements = template.elements; await wisk.editor.addConfigChange('document.config.theme', template.theme); - wisk.editor.justUpdates(); + // Persist all template elements + template.elements.forEach(element => { + if (element.id) { + wisk.editor.justUpdates(element.id); + } + });Option 2: Teach justUpdates to handle "all elements"
if (!elementId) { - return; + // Enqueue all elements for update + wisk.editor.document.data.elements.forEach(e => { + if (e.id) elementUpdatesNeeded.add(e.id); + }); + // Don't return, let debounce handle it }
♻️ Duplicate comments (4)
js/plugins/code/base-text-element.js (1)
1615-1622: The placeholder detection still treats structural children as non-empty.The
updatePlaceholder()logic useschildElementCount > 0to detect whether the contenteditable element has child elements. However, browsers insert structural nodes like<br>into emptycontenteditableelements, which causes this check to incorrectly mark the element as non-empty.Commit 85360e2 refactored when
updatePlaceholder()is called, but did not address the underlying logic. The method still lacks filtering for structural children.Consider filtering out structural elements:
Suggested fix
-const hasChildElements = this.editable.childElementCount > 0; +const hasChildElements = Array.from(this.editable.children).some( + el => el.tagName !== 'BR' +);js/editor.js (3)
2697-2711: Potential null reference accessing getTextContent().Line 2698 calls
domElement.getTextContent().textwithout checking ifgetTextContent()returns null or an object without thetextproperty. This was flagged in a previous review but remains unaddressed.🔎 Proposed fix
// special case: first element if(elementId === wisk.editor.document.data.elements[0].id) { - const textContent = domElement.getTextContent().text || ''; + const textContentObj = domElement.getTextContent?.(); + const textContent = textContentObj?.text || ''; if(textContent) {
1825-1864: Still only persisting first element in renumberNumberedLists.The function correctly renumbers all list elements (lines 1850-1853), but line 1863 only calls
wisk.editor.justUpdates(elements[0]?.id). This means only the first element's updated number is persisted, while other renumbered items won't sync to the document or remote copies.This issue was previously flagged but remains unresolved.
🔎 Recommended fix
+ const changedIds = new Set(); for (let i = 0; i < elements.length; i++) { const el = elements[i]; if (el.component === 'numbered-list-element') { const domEl = document.getElementById(el.id); if (!domEl) continue; const indent = domEl.indent || 0; // ... counter logic ... // Update the element's number const newNumber = counters[indent]; if (domEl.number !== newNumber) { + changedIds.add(el.id); domEl.number = newNumber; domEl.updateNumber(); el.value.number = newNumber; } } else { // Reset counters when encountering non-numbered-list element counters.length = 1; counters[0] = 0; } } // Save modifications - wisk.editor.justUpdates(elements[0]?.id); + changedIds.forEach(id => wisk.editor.justUpdates(id)); }
512-538: Missing container width class update on component change.When handling a component change (lines 529-537), the code replaces the DOM element but doesn't update the parent container's
rndr-full-widthclass based on the new plugin's width property. This was previously flagged and reportedly fixed forchangeBlockType(lines 1116-1139), but the same logic is missing here inhandleElementUpdate.🔎 Proposed fix
} else if (property === 'component') { const newType = event.value.data; const newDomElement = document.createElement(newType); newDomElement.id = elementId; domElement.replaceWith(newDomElement); + + // Update container width class + const container = document.getElementById(`div-${elementId}`); + if (container) { + const pluginDetail = wisk.plugins.getPluginDetail(newType); + if (pluginDetail.width === 'max') { + container.classList.add('rndr-full-width'); + } else { + container.classList.remove('rndr-full-width'); + } + } + setTimeout(() => { newDomElement.setValue('', element.value); }, 0); }
🧹 Nitpick comments (4)
js/plugins/code/options-component.js (1)
1131-1136: Consider adding defensive error handling.If an invalid font-size value is stored in localStorage (e.g., from manual editing or a future code change),
optionwill be undefined and the CSS variable won't be set. Consider adding a fallback:🔎 Optional: Add fallback for invalid values
applyFontSize(sizeValue) { const option = this.fontSizeOptions.find(opt => opt.value === sizeValue); if (option) { document.documentElement.style.setProperty('--editor-font-size', option.size); + } else { + // Fallback to default if invalid value + const defaultOption = this.fontSizeOptions.find(opt => opt.value === 'default'); + if (defaultOption) { + document.documentElement.style.setProperty('--editor-font-size', defaultOption.size); + } } }js/editor.js (3)
128-189: Consider adding error handling for saveModification.The
await wisk.sync.saveModification()at line 188 could throw an error if the save operation fails. While the function is properly awaited, there's no try-catch to handle potential failures gracefully.🔎 Optional improvement
} } - await wisk.sync.saveModification(); + try { + await wisk.sync.saveModification(); + } catch (error) { + console.error('Failed to save config change:', error); + wisk.utils.showToast?.('Failed to save changes', 2000); + } };
191-217: Consider validating email format in addUserAccess.The function accepts any string as an email without format validation. While this may be intentional for flexibility, invalid email formats could lead to issues downstream.
🔎 Optional email validation
wisk.editor.addUserAccess = async function (email) { + // Basic email format validation + if (!email || typeof email !== 'string' || !email.includes('@')) { + console.error('Invalid email format:', email); + return; + } + const timestamp = Date.now(); // Initialize access array if it doesn't exist
1396-1421: Consider link protocol validation in markdown conversion.Line 1417 creates links with
href="$2"without protocol validation. While markdown input is typically trusted, if user-provided markdown is ever processed, this could allowjavascript:ordata:URLs.🔎 Optional safety improvement
// Links (must be before reference numbers) - text = text.replace(/\[([^\]]+)\]\(([^)]+)\)/g, '<a href="$2" contenteditable="false" target="_blank">$1</a>'); + text = text.replace(/\[([^\]]+)\]\(([^)]+)\)/g, (match, text, href) => { + // Only allow safe protocols + if (href && !/^(https?|mailto|tel):/i.test(href) && !href.startsWith('/') && !href.startsWith('#')) { + // Treat as relative URL + } + return `<a href="${href}" contenteditable="false" target="_blank">${text}</a>`; + });
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
css/pages.cssjs/editor.jsjs/elements/home-element.jsjs/plugins/code/accordion-element.jsjs/plugins/code/base-layout-element.jsjs/plugins/code/base-text-element.jsjs/plugins/code/callout-element.jsjs/plugins/code/checkbox-element.jsjs/plugins/code/columns-element.jsjs/plugins/code/heading1-element.jsjs/plugins/code/heading2-element.jsjs/plugins/code/heading3-element.jsjs/plugins/code/heading4-element.jsjs/plugins/code/heading5-element.jsjs/plugins/code/image-element.jsjs/plugins/code/list-element.jsjs/plugins/code/main-element.jsjs/plugins/code/numbered-list-element.jsjs/plugins/code/options-component.jsjs/plugins/code/quote-element.jsjs/plugins/code/super-checkbox.jsjs/plugins/code/text-element.jsjs/plugins/code/video-element.jsjs/theme/variables.cssjs/wisk.jsstyle.css
✅ Files skipped from review due to trivial changes (4)
- js/plugins/code/accordion-element.js
- js/plugins/code/heading5-element.js
- js/plugins/code/video-element.js
- js/plugins/code/base-layout-element.js
🚧 Files skipped from review as they are similar to previous changes (5)
- js/plugins/code/main-element.js
- js/plugins/code/numbered-list-element.js
- style.css
- css/pages.css
- js/plugins/code/super-checkbox.js
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-12T14:14:36.148Z
Learnt from: sushilbang
Repo: sohzm/wisk PR: 54
File: js/plugins/code/base-text-element.js:58-62
Timestamp: 2025-12-12T14:14:36.148Z
Learning: In the js/plugins/code directory, ensure that BaseTextElement serves as the shared base class with common state/logic, while concrete elements (e.g., TextElement) extend it and override render() to provide UI markup. The base class's connectedCallback should query for child elements that are injected by their render methods. This inheritance pattern should be consistently applied across the plugin system; review new or existing elements to maintain a clean base-vs-extension separation, avoid tight coupling, and verify that the render contracts and DOM querying align between base and derived classes.
Applied to files:
js/plugins/code/columns-element.jsjs/plugins/code/checkbox-element.jsjs/plugins/code/callout-element.jsjs/plugins/code/heading4-element.jsjs/plugins/code/image-element.jsjs/plugins/code/heading3-element.jsjs/plugins/code/heading1-element.jsjs/plugins/code/options-component.jsjs/plugins/code/base-text-element.jsjs/plugins/code/list-element.jsjs/plugins/code/quote-element.jsjs/plugins/code/heading2-element.jsjs/plugins/code/text-element.js
🧬 Code graph analysis (2)
js/plugins/code/list-element.js (2)
js/wisk.js (1)
wisk(3-3)js/editor.js (2)
prevElement(325-325)prevElement(2292-2292)
js/editor.js (4)
js/sync/sync.js (5)
elementId(212-212)key(207-207)pathParts(172-172)element(213-213)i(174-174)js/storage/db.js (1)
data(272-272)js/left-sidebar.js (1)
latestClientX(31-31)script.js (1)
editor(76-76)
🔇 Additional comments (47)
js/elements/home-element.js (2)
529-532: LGTM! Improved hover behavior for tree icons.The addition of
:has(.arrow)properly constrains the hover-hide behavior to only items with children, ensuring that leaf nodes keep their emoji/icon visible on hover since they have no arrow to display.
1295-1295: LGTM! Fixes template duplication.Using
slice(3)correctly shows only the remaining templates in the expanded section, avoiding duplication of the first 3 templates already displayed above.js/plugins/code/options-component.js (4)
526-532: LGTM: Styling accommodates the new font-size control.The CSS adjustments ensure the new jalebi-select component fits properly within the options layout and labels don't wrap awkwardly.
718-718: LGTM: Property correctly declared.The editorFontSize property is properly typed and will trigger re-renders when updated.
737-747: LGTM: Font-size initialization is well-structured.The font-size options provide a good range, and the initialization properly reads from localStorage with a sensible default. Applying the font size immediately on construction ensures consistency on page load.
1589-1594: LGTM: UI control is properly wired.The font-size selector is correctly integrated into the main options view with proper event binding and state management.
js/plugins/code/heading4-element.js (1)
20-20: LGTM: Font-size scales correctly with editor font-size.The heading4 element now responds to user font-size preferences with a 10% increase over the base editor font size. The fallback value matches the default in variables.css.
js/plugins/code/heading1-element.js (1)
20-20: LGTM: Font-size scaling maintains heading hierarchy.The heading1 element uses a 2x multiplier, which maintains proper visual hierarchy with other headings (heading2 at 1.5x, heading3 at 1.25x, heading4 at 1.1x). The implementation correctly responds to user font-size preferences.
js/theme/variables.css (1)
49-49: LGTM: CSS variable provides global font-size baseline.The --editor-font-size variable is correctly defined in :root with a sensible default of 17px, which matches the 'default' option in the UI controls.
js/plugins/code/columns-element.js (1)
821-821: LGTM: Padding now uses design token.The context menu item padding has been updated to use the CSS variable
var(--padding-3). Note that--padding-3is defined as8pxin variables.css, which means both vertical and horizontal padding are now 8px (previously the horizontal padding was 10px). This improves consistency across the codebase by using design tokens.js/plugins/code/heading3-element.js (1)
20-20: LGTM! Font-size now scales with the editor configuration.The change to use
calc(var(--editor-font-size, 17px) * 1.25)aligns heading3 with the global font-size system introduced in this PR, enabling users to customize typography.js/plugins/code/checkbox-element.js (1)
397-397: LGTM! Font-size now uses the configurable editor font.The editable area now references
var(--editor-font-size, 17px), allowing checkbox text to scale with user preferences.js/plugins/code/callout-element.js (1)
108-108: LGTM! Callout font-size now matches the editor standard.The change to
var(--editor-font-size, 17px)integrates with the global font-size system and updates the fallback from 16px to 17px.js/plugins/code/heading2-element.js (1)
20-20: LGTM! Heading2 font-size now scales proportionally.The
calc(var(--editor-font-size, 17px) * 1.5)change establishes a proper heading hierarchy (h2 at 1.5x, h3 at 1.25x) while supporting user font customization.js/plugins/code/image-element.js (1)
649-649: LGTM! Image caption font-size now scales with editor settings.The change to
calc(var(--editor-font-size, 17px) * 0.85)makes the caption proportional to the base font while maintaining appropriate sizing (slightly smaller than body text).js/plugins/code/quote-element.js (1)
13-13: LGTM! Dynamic font sizing enhances consistency.The shift from fixed
1.5remtocalc(var(--editor-font-size, 17px) * 1.2)ensures quotes scale with the editor-wide font size setting while maintaining appropriate visual hierarchy.js/wisk.js (1)
86-93: LGTM! Font size initialization is properly defensive.The self-invoking function safely initializes
--editor-font-sizefrom localStorage with appropriate fallbacks. The semantic-to-pixel mapping is clear and aligns with the default of 17px used throughout the codebase.js/plugins/code/list-element.js (2)
105-107: LGTM! Defensive access prevents potential null reference error.Moving
prevDomElementretrieval inside theif (prevElement)block ensures it's only accessed whenprevElementexists, preventing a possible runtime exception.
147-147: LGTM! Styling updates align with editor-wide font sizing.The dot positioning change (from centering transform to
top: 0.6em) simplifies alignment, and the dynamic font-size adoption ensures consistency with the centralized--editor-font-sizevariable.Also applies to: 158-158
js/plugins/code/text-element.js (3)
15-17: LGTM! Editable area refinements improve usability.The increased min-height (1em → 1.5em), added vertical padding, and dynamic font sizing enhance both clickability and visual consistency with the editor-wide font system.
27-32: LGTM! Link styling maintains accent color consistency.The shift from
--fg-blueto--fg-accentaligns with the theme system, and the visited link rule ensures consistent accent coloring across link states.
115-167: LGTM! Paste-link menu UI integrates cleanly with base class logic.The CSS provides proper layering, smooth transitions, and interactive states, while the markup (URL/Bookmark/Embed options) aligns with the base-text-element methods that query and control this menu. The initial hidden state (opacity: 0, pointer-events: none) prevents premature interaction.
Based on learnings, this inheritance pattern is intentional:
BaseTextElementprovides state management and logic, whileTextElementoverridesrender()to inject the UI markup.js/plugins/code/base-text-element.js (10)
58-62: LGTM! Paste-link state properly initialized and bound.The paste-link menu state fields and DOM binding in
connectedCallbackfollow the established inheritance pattern where the base class queries for elements rendered by child classes.Based on learnings, this is the expected pattern for this plugin system.
Also applies to: 351-351
454-460: LGTM! Mobile UX enhancement with scroll-into-view.The delayed
scrollIntoViewcalls on virtual keyboard devices improve mobile usability by ensuring focused elements remain visible above the keyboard. The 150ms delay allows the keyboard animation to settle.Also applies to: 1560-1565
628-674: LGTM! Switch cases properly scoped and numbered list support added.The block wrapping for switch cases (lines 629-674) addresses the scope leakage concern from past reviews. The numbered list pattern matching (
/^(\d+)[.)]$/) is a nice markdown shortcut enhancement.Also applies to: 690-693
984-1052: LGTM! Paste-link menu flow is well-structured.The
showPasteLinkMenu()implementation:
- Creates a placeholder
link-elementat the cursor- Positions the menu below the link with proper transitions
- Wires up click handlers and outside-click cleanup
- Properly tracks and cleans up
_closePasteMenuHandlerinhidePasteLinkMenu()The logic handles selection edge cases and maintains proper state throughout the flow.
1074-1114: LGTM! Paste-link choices properly transform blocks.The
handlePasteLinkChoice()logic correctly handles the three cases:
- URL: Keeps inline link, focuses editor
- Bookmark: Removes placeholder, transforms to
link-preview-element- Embed: Removes placeholder, transforms to
embed-elementTrailing space cleanup prevents orphaned text nodes.
1117-1140: LGTM! Paste-link keyboard navigation is intuitive.Arrow keys navigate options, Enter selects, Escape cancels—standard and accessible. Early return when the menu is showing prevents key event conflicts with other handlers.
1362-1388: LGTM! Backspace correctly detects and removes link-elements.The logic handles both cases where the cursor precedes a
link-element:
- When cursor is at offset 0 in a text node with a
LINK-ELEMENTprevious sibling- When cursor is in an element node with a
LINK-ELEMENTprevious childThis ensures clean deletion of inline link components.
1393-1393: LGTM! Defensive access consistent with list-element fix.Moving
prevDomElementretrieval inside the conditional (after checkingprevElementexists) prevents potential null reference errors, matching the fix inlist-element.jsline 107.
1777-1813: LGTM! Copy handler serializes link-elements for interoperability.The
handleCopy()implementation:
- Clones the selection
- Converts
link-elementcustom elements to standard<a>tags with data attributes- Sets both HTML and plain text formats on the clipboard
This enables pasting into external applications while preserving wisk-specific metadata for internal pastes.
1815-1851: Verify WiskPasteHandler is defined and accessible.The
handlePaste()implementation delegates URL detection and paste handling toWiskPasteHandlerstatic methods (isURL(),normalizeUrl(),isInternalUrl(),isWiskClipboardFormat(),handleTextElementPaste()), but this handler is referenced without import or definition check.Run the following script to verify
WiskPasteHandleris defined before this file loads:#!/bin/bash # Check if WiskPasteHandler is defined in the codebase rg -n "class WiskPasteHandler" --type=js # Check if paste-handler.js or similar is loaded before base-text-element.js rg -n "paste-handler" index.html editor.html --type=htmljs/editor.js (15)
281-301: Good sanitization added for plugin data.The identifier validation at lines 283-286 properly prevents path injection attacks by rejecting keys containing
.,[, or]. This addresses the security concern raised in previous reviews.
307-366: Excellent improvements to block creation.The addition of
lastUpdatedtimestamp (line 323) and the use ofwisk.sync.enqueueSave('block-creation')(line 357) properly address previous concerns about data tracking and save serialization. The conditional event creation for non-remote blocks (line 340) prevents duplicate events during synchronization.
688-697: Good use of enqueueSave for block moves.The move operation correctly uses
wisk.sync.enqueueSave('block-move')instead of the oldsetTimeoutpattern, ensuring proper save serialization.
903-950: Scope leakage properly addressed.All switch cases now use block scope (curly braces), including the previously flagged
'sup'case (lines 930-938). This prevents variable declarations from leaking to other cases.
1073-1147: Width class update properly implemented.The container width class is now correctly updated (lines 1122-1130) when changing block types, addressing the previous review concern. The implementation properly checks
pluginDetail.width === 'max'and adds or removes therndr-full-widthclass accordingly.
1698-1731: Excellent drag state management.The addition of Escape key handling (lines 1702-1706) and window blur cleanup (line 1698) prevents stuck drag states. The comprehensive
cleanupDragfunction properly removes all listeners, clears intervals, removes DOM elements, and resets state.
1733-1759: Good drag performance optimization.The drag throttling using
requestAnimationFrame(lines 1743-1758) with thedragUpdateScheduledflag properly limits updates to one per frame, preventing performance issues during rapid mouse movements.
1761-1799: Smart keyboard-aware drag scrolling.The scroll handling correctly accounts for virtual keyboard height (lines 1766-1767, 1781, 1783), preventing unwanted autoscroll when dragging near the keyboard on mobile devices.
2079-2101: Proper shadow DOM handling for selection.The text node traversal correctly handles shadow DOM by specifically targeting the
#editableelement (lines 2088-2092), preventing selection of non-editable UI elements within custom components.
2272-2282: Clean integration with WiskPasteHandler.The paste event handling properly delegates to
WiskPasteHandlerfor detecting and parsing Wisk's custom clipboard format, then restores the internal clipboard and pastes elements.
2321-2371: Excellent link protocol sanitization.The href sanitization (lines 2335-2356) properly implements a safe protocol allowlist and blocks dangerous protocols like
javascript:,data:, andvbscript:. The approach of removing all attributes first (lines 2358-2361) and then setting only clean ones provides defense-in-depth. This addresses the security concern from previous reviews.
2386-2391: Good sanitization in copy operation.The clipboard copy correctly sanitizes
textContent(line 2390) before adding to the clipboard, preventing malicious link protocols from being copied and potentially pasted elsewhere.
2421-2424: Proper UTF-8 safe base64 encoding.The base64 encoding pattern
btoa(unescape(encodeURIComponent(jsonString)))at line 2423 correctly handles UTF-8 characters that would otherwise break standardbtoa(). This prevents data corruption when clipboard content passes through HTML.
2725-2789: Comprehensive virtual keyboard handling.The implementation properly handles virtual keyboards using both the Visual Viewport API (better browser support) and the newer Virtual Keyboard API (lines 2725-2755). The integration with drag scrolling via
currentKeyboardHeight(line 2763) and the smart shadow DOM focus detection (lines 2774-2776) provide excellent mobile UX.
2311-2318: Potential null reference when focusing after deletion.Lines 2312-2317 access
elementToFocus.componentwithout checking ifelementToFocusis null. If all non-abcdxyz elements are deleted,elementToFocuswill be null and this will throw.🔎 Proposed fix
clearSelection(); - if (elementToFocus) { + if (elementToFocus && elementToFocus.id) { const componentDetail = wisk.plugins.getPluginDetail(elementToFocus.component); if (componentDetail && componentDetail.textual) { wisk.editor.focusBlock(elementToFocus.id, { x: elementToFocus.value?.textContent?.length || 0 });Likely an incorrect or invalid review comment.
| wisk.editor.justUpdates(); | ||
| wisk.editor.document.data.elements = wisk.editor.document.data.elements.filter(e => e.id !== elementId); | ||
| deletedElements.push(elementId); | ||
| deletedElementsLeft.push(elementId); |
There was a problem hiding this comment.
deletedElementsLeft still present despite past review.
Line 1026 still updates deletedElementsLeft.push(elementId), even though a previous review comment (marked as addressed in commit 4631d8c) indicated this unused array should be removed. The variable is declared at line 4 but never read in active code.
Based on learnings, this was flagged for removal but remains in the codebase.
🤖 Prompt for AI Agents
In js/editor.js around line 1026 (declaration at line 4), the code still calls
deletedElementsLeft.push(elementId) even though deletedElementsLeft is unused;
remove the push call at line 1026 and also remove the deletedElementsLeft
variable declaration at line 4 (or delete the entire unused variable if not
referenced elsewhere), search the file for any remaining references to
deletedElementsLeft to ensure no other usages exist, and run linters/tests to
confirm no regressions.
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
js/scripts/home-sidebar.js (1)
66-77: Remove the redundantsetAttribute('value', currentTheme)call on line 77.The code sets
option.selected = trueon line 72 and then callssetAttribute('value', currentTheme)on line 77. However, other usage ofjalebi-selectin the codebase (specificallymain-element.js) demonstrates that settingoption.selectedalone is sufficient—nosetAttributecall is made there. This indicates thesetAttributecall on line 77 is redundant and can be safely removed.
🤖 Fix all issues with AI agents
In @js/paste-handler.js:
- Around line 460-527: flattenElements() mishandles image-element by assigning
imageUrl: element.value (the whole object) instead of the URL string created in
processNode(); update the image-element branch in flattenElements() to pull the
URL string (e.g., element.value.imageUrl) and preserve textContent (e.g.,
element.value.textContent) so flattened image elements match the shape produced
by processNode().
- Around line 44-91: sanitizeAndConvertLinks leaves dangerous href attributes in
place because when an href fails the safeProtocols test you only set href = null
but don’t remove or overwrite the original attribute; also treating "/" as safe
allows protocol-relative URLs. Fix sanitizeAndConvertLinks by: after computing
href and validating against a stricter safeProtocols (remove leading "/" and
allow only explicit https?:, mailto:, tel:, and # anchors), if the href is
invalid explicitly remove the href attribute from the original <a> element (or
set it to a safe empty value like ""), and ensure you also remove
protocol-relative URLs (those starting with "//"); then proceed with replacement
or fallback attributes on the element as currently done (refer to
sanitizeAndConvertLinks, tempDiv, links, and the safeProtocols check).
- Around line 254-458: The img handling in processNode accepts node.src without
protocol validation—mirror the href safeProtocols check used elsewhere and only
accept image URLs matching safe protocols (e.g., http(s):, root-relative "/",
fragment "#", or data:image/*) before creating an image-element; also adjust
TEXT_NODE handling in processNode to not blindly use node.textContent.trim()
(which drops spaces between inline elements) — instead check for any
non-whitespace and preserve/normalize internal spacing (e.g., collapse
consecutive whitespace to single spaces) when building the text-element so
spaces between inline siblings are kept.
- Around line 573-601: parseInlineMarkdown emits unescaped capture groups into a
link-element tag and uses lookbehinds that break older browsers; this lets
crafted markdown like [text"onclick="x](url) inject attributes when
base-text-element.js setValue() inserts innerHTML. Fix by avoiding raw string
interpolation into HTML: construct links using DOM APIs
(createElement('link-element') + setAttribute('url','...') +
setAttribute('title','...')) or explicitly escape attribute values (e.g.,
replace quotes and angle brackets in $1/$2) before embedding, and replace
unsupported lookbehind patterns (used in the italic regexes) with compatible
alternatives (e.g., boundary-aware regex without (?<!) or manual checks) so
parsing works in older runtimes.
In @js/scripts/home-sidebar.js:
- Around line 79-82: Replace the unnecessary defensive extraction in the change
handler for themeSelect: inside the themeSelect.addEventListener('change', ...)
callback stop using the conditional e.detail?.value ?? e.target.value and
instead pass e.target.value directly to wisk.theme.setTheme, so the handler
always uses the event target's value (i.e., call wisk.theme.setTheme with
e.target.value).
🧹 Nitpick comments (2)
js/paste-handler.js (2)
235-253: Default code language shouldn’t bejavascript.Pasted code blocks without an explicit language will be mislabeled.
Proposed tweak
- let language = 'javascript'; + let language = 'plaintext';
937-943:document.execCommand(...)is deprecated; double-check target browser support and editor behavior.This is used for both plain text insertion and HTML insertion. If you support modern contenteditable flows, consider migrating to
Selection/RangeAPIs (or ensure a centralized editor insertion API is used consistently).Also applies to: 1058-1074
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
js/paste-handler.jsjs/scripts/home-sidebar.js
🧰 Additional context used
🧬 Code graph analysis (1)
js/scripts/home-sidebar.js (2)
js/editor.js (1)
e(2762-2762)js/wisk.js (1)
wisk(3-3)
🔇 Additional comments (2)
js/scripts/home-sidebar.js (1)
29-31: No action needed. The<jalebi-select>custom element is properly loaded before this code executes via synchronous script inclusion in index.html (line 316), which runs before home-sidebar.js (line 324). No registration timing issue exists.Likely an incorrect or invalid review comment.
js/paste-handler.js (1)
1049-1124: Paste flow return flags look good; validate sanitization invariants end-to-end.The “handoff” flags (
isWiskFormat,isStandaloneUrl) are a clean separation. Main concern is ensuring any HTML inserted (insertHTML,textContentfields that render as HTML) is sanitized consistently for both HTML and Markdown paths.
| static isWiskClipboardFormat(htmlData) { | ||
| return htmlData && htmlData.includes('__WISK_CLIPBOARD__'); | ||
| } | ||
|
|
||
| static parseWiskClipboard(htmlData) { | ||
| if (!htmlData) return null; | ||
|
|
||
| const match = htmlData.match(/__WISK_CLIPBOARD__(.+?)__WISK_CLIPBOARD_END__/); | ||
| if (!match) return null; | ||
|
|
||
| try { | ||
| const jsonString = decodeURIComponent(escape(atob(match[1]))); | ||
| const wiskData = JSON.parse(jsonString); | ||
| if (wiskData.__wisk_elements__ && wiskData.elements) { | ||
| return wiskData.elements; | ||
| } | ||
| } catch (error) { | ||
| try { | ||
| const wiskData = JSON.parse(match[1]); | ||
| if (wiskData.__wisk_elements__ && wiskData.elements) { | ||
| return wiskData.elements; | ||
| } | ||
| } catch (fallbackError) { | ||
| console.error('Failed to parse wisk clipboard data: ', error); | ||
| } | ||
| } | ||
|
|
||
| return null; | ||
| } | ||
|
|
||
| static handleWiskClipboardPaste(event) { | ||
| const clipboardData = event.clipboardData; | ||
| if (!clipboardData) return null; | ||
|
|
||
| const html = clipboardData.getData('text/html'); | ||
| return WiskPasteHandler.parseWiskClipboard(html); | ||
| } | ||
|
|
There was a problem hiding this comment.
Harden WISK clipboard parsing (regex + UTF-8 decode).
__WISK_CLIPBOARD__(.+?)__WISK_CLIPBOARD_END__won’t match newlines; clipboard payloads can contain them.decodeURIComponent(escape(atob(...)))uses deprecatedescape()and is brittle for UTF-8.
Proposed fix
static parseWiskClipboard(htmlData) {
if (!htmlData) return null;
- const match = htmlData.match(/__WISK_CLIPBOARD__(.+?)__WISK_CLIPBOARD_END__/);
+ const match = htmlData.match(/__WISK_CLIPBOARD__([\s\S]+?)__WISK_CLIPBOARD_END__/);
if (!match) return null;
try {
- const jsonString = decodeURIComponent(escape(atob(match[1])));
+ const bytes = Uint8Array.from(atob(match[1].trim()), c => c.charCodeAt(0));
+ const jsonString = new TextDecoder('utf-8').decode(bytes);
const wiskData = JSON.parse(jsonString);
if (wiskData.__wisk_elements__ && wiskData.elements) {
return wiskData.elements;
}| static sanitizeAndConvertLinks(htmlString) { | ||
| const tempDiv = document.createElement('div'); | ||
| tempDiv.innerHTML = htmlString; | ||
|
|
||
| tempDiv.querySelectorAll('script, style, iframe, object, embed, form, input, button, meta, link, base').forEach(el => el.remove()); | ||
|
|
||
| tempDiv.querySelectorAll('*').forEach(el => { | ||
| Array.from(el.attributes).forEach(attr => { | ||
| if (attr.name.startsWith('on')) { | ||
| el.removeAttribute(attr.name); | ||
| } | ||
| }); | ||
| }); | ||
|
|
||
| const links = tempDiv.querySelectorAll('a'); | ||
| links.forEach(link => { | ||
| let href = link.getAttribute('href'); | ||
|
|
||
| if (href) { | ||
| href = href.replace(/\\/g, '').replace(/"/g, '').replace(/&/g, '&').trim(); | ||
| const safeProtocols = /^(https?:|mailto:|tel:|\/|#)/i; | ||
| if (!safeProtocols.test(href)) { | ||
| href = null; | ||
| } | ||
| } | ||
|
|
||
| if (href) { | ||
| const linkText = link.textContent || ''; | ||
| const wiskDisplay = link.getAttribute('data-wisk-display') || 'inline'; | ||
| const linkElement = document.createElement('link-element'); | ||
| linkElement.setAttribute('url', href); | ||
| linkElement.setAttribute('display', wiskDisplay); | ||
| linkElement.setAttribute('contenteditable', 'false'); | ||
|
|
||
| if (linkText) { | ||
| linkElement.setAttribute('title', linkText); | ||
| } | ||
|
|
||
| link.parentNode.replaceChild(linkElement, link); | ||
| } else { | ||
| link.setAttribute('contenteditable', 'false'); | ||
| link.setAttribute('target', '_blank'); | ||
| link.setAttribute('rel', 'noopener noreferrer'); | ||
| } | ||
| }); | ||
|
|
||
| return tempDiv.innerHTML; | ||
| } |
There was a problem hiding this comment.
Critical: unsafe <a href> is not neutralized (XSS / “javascript:” clickthrough).
When href is deemed unsafe you set href = null, but you never remove/overwrite the element’s existing href attribute—so the original dangerous URL remains clickable.
Also, allowing / as “safe” permits protocol-relative URLs like //evil.com.
Proposed fix
const links = tempDiv.querySelectorAll('a');
links.forEach(link => {
let href = link.getAttribute('href');
if (href) {
href = href.replace(/\\/g, '').replace(/"/g, '').replace(/&/g, '&').trim();
const safeProtocols = /^(https?:|mailto:|tel:|\/|#)/i;
- if (!safeProtocols.test(href)) {
+ if (!safeProtocols.test(href) || href.startsWith('//')) {
href = null;
}
}
if (href) {
const linkText = link.textContent || '';
const wiskDisplay = link.getAttribute('data-wisk-display') || 'inline';
const linkElement = document.createElement('link-element');
linkElement.setAttribute('url', href);
linkElement.setAttribute('display', wiskDisplay);
linkElement.setAttribute('contenteditable', 'false');
@@
link.parentNode.replaceChild(linkElement, link);
} else {
+ // Ensure any existing href is removed/neutralized
+ link.removeAttribute('href');
+ link.setAttribute('href', '#');
link.setAttribute('contenteditable', 'false');
link.setAttribute('target', '_blank');
link.setAttribute('rel', 'noopener noreferrer');
}
});🤖 Prompt for AI Agents
In @js/paste-handler.js around lines 44 - 91, sanitizeAndConvertLinks leaves
dangerous href attributes in place because when an href fails the safeProtocols
test you only set href = null but don’t remove or overwrite the original
attribute; also treating "/" as safe allows protocol-relative URLs. Fix
sanitizeAndConvertLinks by: after computing href and validating against a
stricter safeProtocols (remove leading "/" and allow only explicit https?:,
mailto:, tel:, and # anchors), if the href is invalid explicitly remove the href
attribute from the original <a> element (or set it to a safe empty value like
""), and ensure you also remove protocol-relative URLs (those starting with
"//"); then proceed with replacement or fallback attributes on the element as
currently done (refer to sanitizeAndConvertLinks, tempDiv, links, and the
safeProtocols check).
| static getIndentLevel(element) { | ||
| let indent = 0; | ||
| let parent = element.parentElement; | ||
| while (parent) { | ||
| if (parent.tagName === 'UL' || parent.tagName === 'OL') { | ||
| indent++; | ||
| } | ||
| parent = parent.parentElement; | ||
| } | ||
| return Math.max(0, indent - 1); | ||
| } | ||
|
|
||
| static isPartOfProcessedList(node) { | ||
| let parent = node.parentElement; | ||
| while (parent) { | ||
| if (parent._processed) return true; | ||
| parent = parent.parentElement; | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| static getDirectChildrenLi(listNode) { | ||
| return Array.from(listNode.children).filter(child => child.tagName === 'LI'); | ||
| } | ||
|
|
||
| static extractContentFromLi(li) { | ||
| const clone = li.cloneNode(true); | ||
| clone.querySelectorAll('ul, ol').forEach(list => list.remove()); | ||
| clone.querySelectorAll('input').forEach(input => input.remove()); | ||
| const blockElements = []; | ||
| clone.querySelectorAll('table').forEach(table => { | ||
| blockElements.push({ type: 'table', node: table.cloneNode(true) }); | ||
| table.remove(); | ||
| }); | ||
| clone.querySelectorAll('pre').forEach(pre => { | ||
| blockElements.push({ type: 'code', node: pre.cloneNode(true) }); | ||
| pre.remove(); | ||
| }); | ||
| const paragraphs = clone.querySelectorAll('p'); | ||
| let text = ''; | ||
|
|
||
| if (paragraphs.length > 0) { | ||
| text = Array.from(paragraphs).map(p => p.innerHTML.trim()).filter(t => t).join('<br>'); | ||
| } else { | ||
| text = clone.innerHTML.trim(); | ||
| } | ||
| text = text.replace(/^\[[\sx]\]\s*/, ''); | ||
|
|
||
| return { text, blockElements }; | ||
| } | ||
|
|
||
| static processListRecursively(listNode, baseIndent = 0, numberCounters = {}, isCheckboxList = false) { | ||
| const results = []; | ||
| const directChildren = WiskPasteHandler.getDirectChildrenLi(listNode); | ||
| const isNumbered = listNode.tagName === 'OL'; | ||
|
|
||
| // Clean the text: | ||
| // 1. Replace all whitespace characters (including newlines, tabs) with single spaces | ||
| // 2. Trim any leading/trailing whitespace | ||
| text = text.replace(/\s+/g, ' ').trim(); | ||
| if (text) { | ||
| document.execCommand('insertText', false, text); | ||
| if (!numberCounters[baseIndent]) { | ||
| numberCounters[baseIndent] = 1; | ||
| } | ||
|
|
||
| directChildren.forEach((li) => { | ||
| li._processed = true; | ||
| li.querySelectorAll('*').forEach(child => { | ||
| child._processed = true; | ||
| }); | ||
| const { text, blockElements } = WiskPasteHandler.extractContentFromLi(li); | ||
| const item = { | ||
| type: 'list-item', | ||
| text: WiskPasteHandler.sanitizeAndConvertLinks(text), | ||
| indent: baseIndent, | ||
| }; | ||
|
|
||
| if (isNumbered) { | ||
| item.number = numberCounters[baseIndent]; | ||
| numberCounters[baseIndent]++; | ||
| } | ||
|
|
||
| if (isCheckboxList) { | ||
| item.checked = li.textContent.startsWith('[x]') || li.querySelector('input[type="checkbox"]')?.checked || false; | ||
| } | ||
|
|
||
| results.push(item); | ||
| blockElements.forEach(block => { | ||
| results.push({ | ||
| type: 'block-element', | ||
| blockType: block.type, | ||
| node: block.node, | ||
| indent: baseIndent, | ||
| }); | ||
| }); | ||
| const nestedList = Array.from(li.children).find(child => | ||
| child.tagName === 'UL' || child.tagName === 'OL' | ||
| ); | ||
|
|
||
| if (nestedList) { | ||
| nestedList._processed = true; | ||
| numberCounters[baseIndent + 1] = 1; | ||
| const nestedDirectChildren = WiskPasteHandler.getDirectChildrenLi(nestedList); | ||
| const isNestedCheckboxList = nestedDirectChildren.some( | ||
| li => li.textContent.startsWith('[ ]') || li.textContent.startsWith('[x]') || li.querySelector('input[type="checkbox"]') | ||
| ); | ||
|
|
||
| const nestedResults = WiskPasteHandler.processListRecursively(nestedList, baseIndent + 1, numberCounters, isNestedCheckboxList); | ||
| results.push(...nestedResults); | ||
| } | ||
| }); | ||
|
|
||
| return results; | ||
| } |
There was a problem hiding this comment.
List parsing: checkbox detection should be case-insensitive + marker stripping inconsistent.
startsWith('[x]')misses[X](and stripping uses/^\[[\sx]\]\s*/which doesn’t matchXeither).- Indent computation assumes list nesting only; pasted markdown lists often use spaces/tabs, so HTML vs markdown indents will differ—worth validating UX.
Targeted tweak
- item.checked = li.textContent.startsWith('[x]') || li.querySelector('input[type="checkbox"]')?.checked || false;
+ item.checked = /^\s*\[[xX]\]/.test(li.textContent) || li.querySelector('input[type="checkbox"]')?.checked || false;- text = text.replace(/^\[[\sx]\]\s*/, '');
+ text = text.replace(/^\s*\[[\s xX]\]\s*/, '');| static processNode(node, structuredElements) { | ||
| if (node.nodeType === Node.TEXT_NODE) { | ||
| const text = node.textContent.trim(); | ||
| if (text) { | ||
| structuredElements.push({ | ||
| elementName: 'text-element', | ||
| value: text, | ||
| }); | ||
| } | ||
| return; | ||
| } | ||
|
|
||
| if (node.nodeType !== Node.ELEMENT_NODE) return; | ||
|
|
||
| let element = null; | ||
| let skipChildren = false; | ||
|
|
||
| switch (node.tagName.toLowerCase()) { | ||
| case 'h1': { | ||
| if (node.textContent.trim()) { | ||
| element = { elementName: 'heading1-element', value: WiskPasteHandler.sanitizeAndConvertLinks(node.innerHTML.trim()) }; | ||
| } | ||
| skipChildren = true; | ||
| break; | ||
| } | ||
| case 'h2': { | ||
| if (node.textContent.trim()) { | ||
| element = { elementName: 'heading2-element', value: WiskPasteHandler.sanitizeAndConvertLinks(node.innerHTML.trim()) }; | ||
| } | ||
| skipChildren = true; | ||
| break; | ||
| } | ||
| case 'h3': { | ||
| if (node.textContent.trim()) { | ||
| element = { elementName: 'heading3-element', value: WiskPasteHandler.sanitizeAndConvertLinks(node.innerHTML.trim()) }; | ||
| } | ||
| skipChildren = true; | ||
| break; | ||
| } | ||
| case 'h4': { | ||
| if (node.textContent.trim()) { | ||
| element = { elementName: 'heading4-element', value: WiskPasteHandler.sanitizeAndConvertLinks(node.innerHTML.trim()) }; | ||
| } | ||
| skipChildren = true; | ||
| break; | ||
| } | ||
| case 'h5': { | ||
| if (node.textContent.trim()) { | ||
| element = { elementName: 'heading5-element', value: WiskPasteHandler.sanitizeAndConvertLinks(node.innerHTML.trim()) }; | ||
| } | ||
| skipChildren = true; | ||
| break; | ||
| } | ||
| case 'ul': | ||
| case 'ol': { | ||
| if (!node._processed) { | ||
| node._processed = true; | ||
| const directLiChildren = WiskPasteHandler.getDirectChildrenLi(node); | ||
| const isCheckboxList = directLiChildren.some( | ||
| li => li.textContent.startsWith('[ ]') || li.textContent.startsWith('[x]') || li.querySelector('input[type="checkbox"]') | ||
| ); | ||
| let elementName; | ||
| if (isCheckboxList && node.tagName.toLowerCase() === 'ul') { | ||
| elementName = 'checkbox-element'; | ||
| } else { | ||
| elementName = node.tagName.toLowerCase() === 'ul' ? 'list-element' : 'numbered-list-element'; | ||
| } | ||
| const results = WiskPasteHandler.processListRecursively(node, 0, {}, isCheckboxList); | ||
| const listItems = results.filter(r => r.type === 'list-item'); | ||
| const blockElements = results.filter(r => r.type === 'block-element'); | ||
| element = { | ||
| elementName: elementName, | ||
| value: listItems.map(item => { | ||
| const itemValue = { text: item.text, indent: item.indent }; | ||
| if (item.number !== undefined) { | ||
| itemValue.number = item.number; | ||
| } | ||
| if (item.checked !== undefined) { | ||
| itemValue.checked = item.checked; | ||
| } | ||
| return itemValue; | ||
| }), | ||
| }; | ||
| blockElements.forEach(blockEl => { | ||
| if (blockEl.blockType === 'table') { | ||
| const { headers, rows } = WiskPasteHandler.parseTableNode(blockEl.node); | ||
| if (headers.length > 0 || rows.length > 0) { | ||
| structuredElements.push({ | ||
| elementName: 'table-element', | ||
| value: { | ||
| tableContent: { | ||
| headers: headers.length > 0 ? headers : ['Column 1'], | ||
| rows: rows.length > 0 ? rows : [['']], | ||
| } | ||
| } | ||
| }); | ||
| } | ||
| } else if (blockEl.blockType === 'code') { | ||
| const codeData = WiskPasteHandler.parseCodeNode(blockEl.node); | ||
| structuredElements.push({ | ||
| elementName: 'code-element', | ||
| value: codeData | ||
| }); | ||
| } | ||
| }); | ||
|
|
||
| skipChildren = true; | ||
| } | ||
| break; | ||
| } | ||
| case 'li': { | ||
| if (!WiskPasteHandler.isPartOfProcessedList(node) && !node._processed) { | ||
| const isCheckbox = | ||
| node.textContent.startsWith('[ ]') || | ||
| node.textContent.startsWith('[x]') || | ||
| node.querySelector('input[type="checkbox"]'); | ||
|
|
||
| if (isCheckbox) { | ||
| element = { | ||
| elementName: 'checkbox-element', | ||
| value: [ | ||
| { | ||
| text: WiskPasteHandler.sanitizeAndConvertLinks(node.innerHTML.replace(/^\[[\sx]\]\s*/, '').trim()), | ||
| checked: node.textContent.startsWith('[x]') || node.querySelector('input[type="checkbox"]')?.checked, | ||
| indent: WiskPasteHandler.getIndentLevel(node), | ||
| }, | ||
| ], | ||
| }; | ||
| } else { | ||
| element = { | ||
| elementName: 'list-element', | ||
| value: [ | ||
| { | ||
| text: WiskPasteHandler.sanitizeAndConvertLinks(node.innerHTML.trim()), | ||
| indent: WiskPasteHandler.getIndentLevel(node), | ||
| }, | ||
| ], | ||
| }; | ||
| } | ||
| skipChildren = true; | ||
| } | ||
| }, | ||
| true | ||
| ); | ||
| break; | ||
| } | ||
| case 'blockquote': | ||
| element = { elementName: 'quote-element', value: WiskPasteHandler.sanitizeAndConvertLinks(node.innerHTML.trim()) }; | ||
| skipChildren = true; | ||
| break; | ||
| case 'pre': | ||
| case 'code': | ||
| element = { | ||
| elementName: 'code-element', | ||
| value: WiskPasteHandler.parseCodeNode(node) | ||
| }; | ||
| skipChildren = true; | ||
| break; | ||
| case 'hr': { | ||
| element = { elementName: 'divider-element', value: '' }; | ||
| skipChildren = true; | ||
| break; | ||
| } | ||
| case 'img': { | ||
| if (node.src) { | ||
| element = { | ||
| elementName: 'image-element', | ||
| value: { imageUrl: node.src, textContent: '' }, | ||
| }; | ||
| } | ||
| skipChildren = true; | ||
| break; | ||
| } | ||
| case 'p': { | ||
| if (node.textContent.trim()) { | ||
| element = { elementName: 'text-element', value: WiskPasteHandler.sanitizeAndConvertLinks(node.innerHTML.trim()) }; | ||
| } | ||
| skipChildren = true; | ||
| break; | ||
| } | ||
| case 'table': { | ||
| const { headers, rows } = WiskPasteHandler.parseTableNode(node); | ||
| if (headers.length > 0 || rows.length > 0) { | ||
| element = { | ||
| elementName: 'table-element', | ||
| value: { | ||
| tableContent: { | ||
| headers: headers.length > 0 ? headers : ['Column 1'], | ||
| rows: rows.length > 0 ? rows : [['']], | ||
| } | ||
| } | ||
| }; | ||
| } | ||
| skipChildren = true; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| if (element) { | ||
| structuredElements.push(element); | ||
| } | ||
|
|
||
| if (!skipChildren) { | ||
| node.childNodes.forEach(childNode => { | ||
| WiskPasteHandler.processNode(childNode, structuredElements); | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the complete file to understand context and find tests
wc -l js/paste-handler.jsRepository: sohzm/wisk
Length of output: 78
🏁 Script executed:
# Search for how structuredElements is used and what happens downstream
rg "structuredElements" js/paste-handler.js -B 2 -A 2Repository: sohzm/wisk
Length of output: 1844
🏁 Script executed:
# Look for image src validation or protocol checking
rg "img.*src|protocol|http" js/paste-handler.js -iRepository: sohzm/wisk
Length of output: 433
🏁 Script executed:
# Search for any tests related to paste handler
fd -t f ".*test.*" -path "*/paste*"Repository: sohzm/wisk
Length of output: 222
🏁 Script executed:
# Look for how span and other inline elements are handled
rg "<span>|inline|whitespace" js/paste-handler.js -iRepository: sohzm/wisk
Length of output: 1118
🏁 Script executed:
# Look for sanitizeAndConvertLinks implementation
rg "sanitizeAndConvertLinks" js/paste-handler.js -A 15Repository: sohzm/wisk
Length of output: 5806
🏁 Script executed:
# Check the img element handling more carefully - does it validate src?
rg "case 'img':" js/paste-handler.js -A 10Repository: sohzm/wisk
Length of output: 408
🏁 Script executed:
# Search for any src validation logic
rg "\.src\s*=" js/paste-handler.js -B 2 -A 2Repository: sohzm/wisk
Length of output: 36
🏁 Script executed:
# Look for test files
fd "test\|spec" --type fRepository: sohzm/wisk
Length of output: 36
🏁 Script executed:
# Check how inline elements like span, strong, em are handled
rg "case 'span':|case 'strong':|case 'em':|case 'b':|case 'i':" js/paste-handler.js -A 3Repository: sohzm/wisk
Length of output: 36
🏁 Script executed:
# Check the full sanitizeAndConvertLinks to see if it handles img elements
rg "sanitizeAndConvertLinks" js/paste-handler.js -A 30 | head -50Repository: sohzm/wisk
Length of output: 1990
🏁 Script executed:
# Verify if img elements get processed through sanitizeAndConvertLinks anywhere
rg "img" js/paste-handler.js -B 3 -A 3Repository: sohzm/wisk
Length of output: 560
🏁 Script executed:
# Check if there's any downstream validation of imageUrl
rg "imageUrl|image-element" js/paste-handler.js -B 2 -A 2Repository: sohzm/wisk
Length of output: 1334
Validate image URL protocol to prevent XSS with javascript: or data: URIs.
The img case accepts node.src without any protocol validation, while <a> href uses safeProtocols check (/^(https?:|mailto:|tel:|\/|#)/i). Apply the same validation to image URLs:
case 'img': {
if (node.src) {
const safeProtocols = /^(https?:|\/|#|data:image\/)/i;
if (safeProtocols.test(node.src)) {
element = {
elementName: 'image-element',
value: { imageUrl: node.src, textContent: '' },
};
}
}
skipChildren = true;
break;
}Also, node.textContent.trim() in TEXT_NODE handling drops whitespace between unblocked inline elements (e.g., <span>Hello</span> <span>world</span> becomes two separate elements with no space), though this is less common with Google Docs/Office HTML which typically wraps in <p> tags.
🤖 Prompt for AI Agents
In @js/paste-handler.js around lines 254 - 458, The img handling in processNode
accepts node.src without protocol validation—mirror the href safeProtocols check
used elsewhere and only accept image URLs matching safe protocols (e.g.,
http(s):, root-relative "/", fragment "#", or data:image/*) before creating an
image-element; also adjust TEXT_NODE handling in processNode to not blindly use
node.textContent.trim() (which drops spaces between inline elements) — instead
check for any non-whitespace and preserve/normalize internal spacing (e.g.,
collapse consecutive whitespace to single spaces) when building the text-element
so spaces between inline siblings are kept.
| static flattenElements(structuredElements) { | ||
| const flattenedElements = []; | ||
|
|
||
| structuredElements.forEach((element, idx) => { | ||
| if (Array.isArray(element.value)) { | ||
| element.value.forEach((item, index) => { | ||
| if (typeof item === 'object') { | ||
| const newElement = { | ||
| elementName: element.elementName, | ||
| value: { | ||
| textContent: item.text || '', | ||
| indent: typeof item.indent === 'number' ? item.indent : 0, | ||
| }, | ||
| }; | ||
|
|
||
| while (active?.shadowRoot?.activeElement) { | ||
| active = active.shadowRoot.activeElement; | ||
| if (element.elementName === 'checkbox-element') { | ||
| newElement.value.checked = !!item.checked; | ||
| } | ||
|
|
||
| if (element.elementName === 'numbered-list-element') { | ||
| newElement.value.number = item.number !== undefined ? item.number : index + 1; | ||
| } | ||
|
|
||
| flattenedElements.push(newElement); | ||
| } else { | ||
| flattenedElements.push({ | ||
| elementName: element.elementName, | ||
| value: { | ||
| textContent: item, | ||
| }, | ||
| }); | ||
| } | ||
| }); | ||
| } else if (element.elementName === 'image-element') { | ||
| flattenedElements.push({ | ||
| elementName: element.elementName, | ||
| value: { | ||
| imageUrl: element.value, | ||
| textContent: '', | ||
| }, | ||
| }); | ||
| } else if (element.elementName === 'table-element') { | ||
| flattenedElements.push({ | ||
| elementName: element.elementName, | ||
| value: element.value, | ||
| }); | ||
| } else if (element.elementName === 'code-element') { | ||
| flattenedElements.push({ | ||
| elementName: element.elementName, | ||
| value: element.value, | ||
| }); | ||
| } else if (element.elementName === 'latex-element') { | ||
| flattenedElements.push({ | ||
| elementName: element.elementName, | ||
| value: element.value, | ||
| }); | ||
| } else { | ||
| flattenedElements.push({ | ||
| elementName: element.elementName, | ||
| value: { | ||
| textContent: element.value, | ||
| }, | ||
| }); | ||
| } | ||
| }); | ||
|
|
||
| return flattenedElements; | ||
| } |
There was a problem hiding this comment.
Critical: flattenElements() breaks image-element values.
processNode() creates image-element with value: { imageUrl, textContent }, but flattenElements() wraps it as imageUrl: element.value (object), not the URL string.
Proposed fix
} else if (element.elementName === 'image-element') {
+ const imageUrl =
+ typeof element.value === 'string'
+ ? element.value
+ : (element.value && element.value.imageUrl) || '';
flattenedElements.push({
elementName: element.elementName,
value: {
- imageUrl: element.value,
- textContent: '',
+ imageUrl,
+ textContent: (element.value && element.value.textContent) || '',
},
});🤖 Prompt for AI Agents
In @js/paste-handler.js around lines 460 - 527, flattenElements() mishandles
image-element by assigning imageUrl: element.value (the whole object) instead of
the URL string created in processNode(); update the image-element branch in
flattenElements() to pull the URL string (e.g., element.value.imageUrl) and
preserve textContent (e.g., element.value.textContent) so flattened image
elements match the shape produced by processNode().
| static parseInlineMarkdown(text) { | ||
| if (!text) return ''; | ||
|
|
||
| let result = text; | ||
| // Escape HTML entities first (but preserve existing HTML tags we want to keep) | ||
| result = result.replace(/&/g, '&'); | ||
| // Don't escape < and > for HTML tags we want to preserve | ||
| result = result.replace(/<(?!(b|i|u|strike|code|a|br|span|strong|em)\b)/g, '<'); | ||
| result = result.replace(/(?<!\b(b|i|u|strike|code|a|br|span|strong|em))>/g, '>'); | ||
| // Bold and italic combined (***text*** or ___text___) | ||
| result = result.replace(/\*\*\*(.+?)\*\*\*/g, '<b><i>$1</i></b>'); | ||
| result = result.replace(/___(.+?)___/g, '<b><i>$1</i></b>'); | ||
| // Bold (**text** or __text__) | ||
| result = result.replace(/\*\*(.+?)\*\*/g, '<b>$1</b>'); | ||
| result = result.replace(/__(.+?)__/g, '<b>$1</b>'); | ||
| // Italic (*text* or _text_) - be careful not to match inside words | ||
| result = result.replace(/\*([^*]+)\*/g, '<i>$1</i>'); | ||
| result = result.replace(/(?<![a-zA-Z])_([^_]+)_(?![a-zA-Z])/g, '<i>$1</i>'); | ||
| // Strikethrough (~~text~~) | ||
| result = result.replace(/~~(.+?)~~/g, '<strike>$1</strike>'); | ||
| // Inline code (`code`) | ||
| result = result.replace(/`([^`]+)`/g, '<code style="background: var(--bg-2); padding: 2px 4px; border-radius: 3px; font-family: var(--font-mono);">$1</code>'); | ||
| // Links [text](url) - convert to link-element | ||
| result = result.replace(/\[([^\]]+)\]\(([^)]+)\)/g, '<link-element url="$2" display="inline" title="$1" contenteditable="false"></link-element>'); | ||
| // Images  - convert to text representation | ||
| result = result.replace(/!\[([^\]]*)\]\(([^)]+)\)/g, '[Image: $1]'); | ||
|
|
||
| return active; | ||
| return result; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check the file size and then examine the relevant sections
wc -l js/paste-handler.jsRepository: sohzm/wisk
Length of output: 78
🏁 Script executed:
# Read the parseInlineMarkdown function and surrounding context
sed -n '570,610p' js/paste-handler.jsRepository: sohzm/wisk
Length of output: 2190
🏁 Script executed:
# Also check the lines mentioned in "Also applies to: 620-930"
sed -n '620,650p' js/paste-handler.jsRepository: sohzm/wisk
Length of output: 1156
🏁 Script executed:
# Search for sanitizeAndConvertLinks function
rg "sanitizeAndConvertLinks" js/paste-handler.jsRepository: sohzm/wisk
Length of output: 1377
🏁 Script executed:
# Search for any HTML sanitization or escaping functions
rg "sanitize|escape|DOMPurify" .Repository: sohzm/wisk
Length of output: 50370
🏁 Script executed:
# Find and read the sanitizeAndConvertLinks function
rg -A 30 "static sanitizeAndConvertLinks" js/paste-handler.jsRepository: sohzm/wisk
Length of output: 1344
🏁 Script executed:
# Check the lines 620-930 that the review mentions
sed -n '620,700p' js/paste-handler.jsRepository: sohzm/wisk
Length of output: 3004
🏁 Script executed:
# Check if there's any HTML escaping happening for attributes
rg "escapeHtml|escapeAttribute|htmlEscape" .Repository: sohzm/wisk
Length of output: 509
🏁 Script executed:
# Check how parseInlineMarkdown is used and if its output is passed through sanitizeAndConvertLinks
rg "parseInlineMarkdown" js/paste-handler.js -B 2 -A 2Repository: sohzm/wisk
Length of output: 2047
🏁 Script executed:
# Check browser support for lookbehind assertions
rg "browserslist|target|es20" package.json .babelrc .browserslistrc 2>/dev/null | head -20Repository: sohzm/wisk
Length of output: 36
🏁 Script executed:
# See the complete sanitizeAndConvertLinks function
rg -A 50 "static sanitizeAndConvertLinks" js/paste-handler.jsRepository: sohzm/wisk
Length of output: 1995
🏁 Script executed:
# Check if link-element component has any sanitization
rg -A 20 "class.*link-element" js/plugins/code/link-element.js | head -40Repository: sohzm/wisk
Length of output: 36
🏁 Script executed:
# Verify how the textContent from parseInlineMarkdown is used in elements
rg "textContent.*parseInlineMarkdown|parseInlineMarkdown.*textContent" js/paste-handler.js -A 1 -B 1 | head -30Repository: sohzm/wisk
Length of output: 1428
🏁 Script executed:
# Find the link-element implementation
fd "link-element" --type fRepository: sohzm/wisk
Length of output: 85
🏁 Script executed:
# Check how textContent is rendered in the UI (check the element processor)
rg "link-element|textContent" js/plugins/code/ -lRepository: sohzm/wisk
Length of output: 1144
🏁 Script executed:
# Look for where these elements are rendered (innerHTML vs textContent vs setAttribute)
rg "innerHTML.*textContent|textContent.*innerHTML|\.textContent\s*=" js/ -B 2 -A 2 | head -40Repository: sohzm/wisk
Length of output: 1707
🏁 Script executed:
# Check how text-element renders its textContent
rg -A 30 "class.*text-element|customElements.define.*text-element" js/plugins/code/ | head -60Repository: sohzm/wisk
Length of output: 238
🏁 Script executed:
# Check base-text-element which might handle rendering
cat js/plugins/code/base-text-element.js | head -100Repository: sohzm/wisk
Length of output: 3836
🏁 Script executed:
# Check the render method in BaseTextElement
rg -A 50 "render\(\)" js/plugins/code/base-text-element.js | head -70Repository: sohzm/wisk
Length of output: 2634
🏁 Script executed:
# Check text-element specifically
cat js/plugins/code/text-element.js | head -150Repository: sohzm/wisk
Length of output: 5170
🏁 Script executed:
# Find where element values are actually rendered/loaded into the DOM
rg "value\.textContent|\.textContent\s*=" js/plugins/code/text-element.js -B 3 -A 3Repository: sohzm/wisk
Length of output: 36
🏁 Script executed:
# Check how elements are loaded/hydrated from saved data
rg "loadValue|setValue|hydrate|connectedCallback" js/plugins/code/base-text-element.js -A 10 | head -50Repository: sohzm/wisk
Length of output: 1157
🏁 Script executed:
# Search for where innerHTML is set with element content
rg "innerHTML\s*=" js/plugins/code/text-element.js -B 2 -A 2Repository: sohzm/wisk
Length of output: 143
🏁 Script executed:
# Confirm the issue by checking setValue is called with parseInlineMarkdown output
rg "setValue|value\.textContent" js/editor.js -B 5 -A 5 | head -40Repository: sohzm/wisk
Length of output: 1309
🏁 Script executed:
# Final confirmation: see complete flow from parseMarkdownText to setValue
rg "parseMarkdownText|parseInlineMarkdown" js/editor.js -B 2 -A 5 | head -30Repository: sohzm/wisk
Length of output: 36
Markdown link handling creates injection vulnerability; regex lookbehind breaks in older browsers.
parseInlineMarkdown()emits<link-element url="$2" ... title="$1" ...>without escaping regex captures. A markdown URL like[text"onclick="alert(1)](x)breaks attribute boundaries and allows HTML injection, since the output is later set viainnerHTMLinbase-text-element.jssetValue()without sanitization.- Regexes use lookbehind assertions (
(?<!\b...),(?<![a-zA-Z])) which are ES2018+ and will fail silently in older browsers, breaking markdown parsing.
Recommend: escape attribute values (e.g., $1.replace(/"/g, '"')) before insertion, or build link-element via DOM API (createElement + setAttribute). Alternatively, run sanitizeAndConvertLinks() on the textContent before setValue() processes it.
🤖 Prompt for AI Agents
In @js/paste-handler.js around lines 573 - 601, parseInlineMarkdown emits
unescaped capture groups into a link-element tag and uses lookbehinds that break
older browsers; this lets crafted markdown like [text"onclick="x](url) inject
attributes when base-text-element.js setValue() inserts innerHTML. Fix by
avoiding raw string interpolation into HTML: construct links using DOM APIs
(createElement('link-element') + setAttribute('url','...') +
setAttribute('title','...')) or explicitly escape attribute values (e.g.,
replace quotes and angle brackets in $1/$2) before embedding, and replace
unsupported lookbehind patterns (used in the italic regexes) with compatible
alternatives (e.g., boundary-aware regex without (?<!) or manual checks) so
parsing works in older runtimes.
| static async handleImagePaste(event) { | ||
| const clipboardData = event.clipboardData; | ||
| if (!clipboardData || !clipboardData.items) return null; | ||
|
|
||
| for (const item of clipboardData.items) { | ||
| if (item.type.startsWith('image/')) { | ||
| event.preventDefault(); | ||
| const file = item.getAsFile(); | ||
| if (!file) return null; | ||
|
|
||
| return new Promise((resolve) => { | ||
| const reader = new FileReader(); | ||
| reader.onload = (e) => { | ||
| resolve({ | ||
| elementName: 'image-element', | ||
| value: { | ||
| imageUrl: e.target.result, | ||
| textContent: '', | ||
| }, | ||
| }); | ||
| }; | ||
| reader.onerror = () => resolve(null); | ||
| reader.readAsDataURL(file); | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| return null; | ||
| } |
There was a problem hiding this comment.
Image paste paths are inconsistent (data URL vs asset storage) + sanitize file extension.
handleImagePaste()returns a data URL block, buthandleTextElementPaste()stores towisk.db.setAsset()and inserts animage-elementwith a synthetic filename—these likely produce different downstream rendering.file.type.split('/')[1]can includesvg+xml; better to normalize to[a-z0-9]to avoid odd filenames.
Proposed tweak
- const extension = file.type.split('/')[1] || 'png';
+ const extensionRaw = file.type.split('/')[1] || 'png';
+ const extension = extensionRaw.replace(/[^a-z0-9]/gi, '').toLowerCase() || 'png';Also applies to: 1023-1047
| themeSelect.addEventListener('change', e => { | ||
| wisk.theme.setTheme(e.target.value); | ||
| const value = e.detail?.value ?? e.target.value; | ||
| wisk.theme.setTheme(value); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find where jalebi-select is defined and used
find . -type f -name "*.js" -o -name "*.html" -o -name "*.ts" | head -20Repository: sohzm/wisk
Length of output: 593
🏁 Script executed:
# Search for jalebi-select definition and event patterns
rg -i "jalebi-select|themeSelect" --type js --type html -C 3Repository: sohzm/wisk
Length of output: 7270
🏁 Script executed:
# Look for CustomEvent dispatch patterns and change events
rg "dispatchEvent|new CustomEvent|new Event" --type js -C 4 | head -100Repository: sohzm/wisk
Length of output: 4825
🏁 Script executed:
# Check the actual element definition in home-sidebar.js
rg -n "themeSelect" js/scripts/home-sidebar.js -B 5 -A 10Repository: sohzm/wisk
Length of output: 1471
Remove unnecessary defensive pattern; use e.target.value consistently.
The defensive pattern e.detail?.value ?? e.target.value is unnecessary. Other parts of the codebase using jalebi-select (e.g., database-element.js) use e.target.value directly without checking detail.value. Standard web component behavior dispatches change events with the value on target, not detail. Simplify to:
themeSelect.addEventListener('change', e => {
wisk.theme.setTheme(e.target.value);
});🤖 Prompt for AI Agents
In @js/scripts/home-sidebar.js around lines 79 - 82, Replace the unnecessary
defensive extraction in the change handler for themeSelect: inside the
themeSelect.addEventListener('change', ...) callback stop using the conditional
e.detail?.value ?? e.target.value and instead pass e.target.value directly to
wisk.theme.setTheme, so the handler always uses the event target's value (i.e.,
call wisk.theme.setTheme with e.target.value).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
js/sync/sync.js (2)
22-95: Path-based mutation logic is sound, but note subtle loop counter modification.The applyEvent function handles restore, reordering, element creation, and generic path traversal correctly. However, Line 65 uses
const elementId = pathParts[++i], which increments the loop counter inline to consume two path segments ('elements' + ID). While correct, this pattern is subtle and could confuse future maintainers.💡 Optional: Add clarifying comment
// 'elements' array uses ID lookup, not index if (key === 'elements' && Array.isArray(current.elements)) { + // Consume next path part as element ID (incrementing i) const elementId = pathParts[++i];
98-127: Redundant initialization on Line 116.Line 115 already initializes
wisk.editor.document.data.syncwithsyncLogs: [], making line 116's nullish coalescing operator redundant.♻️ Remove redundant line
// Store in syncLogs for future server sync wisk.editor.document.data.sync ??= { syncLogs: [], isPushed: false, lastSync: 0 }; - wisk.editor.document.data.sync.syncLogs ??= []; wisk.editor.document.data.sync.syncLogs.push(...eventsToSave);js/plugins/code/options-component.js (1)
1138-1143: Add fallback for invalid font size values.If
localStoragecontains an invalid value (e.g., manually edited), thefind()returnsundefinedand the CSS variable won't be set. Consider adding a fallback to the default size.🔧 Proposed fix with fallback
applyFontSize(sizeValue) { const option = this.fontSizeOptions.find(opt => opt.value === sizeValue); if (option) { document.documentElement.style.setProperty('--editor-font-size', option.size); + } else { + // Fallback to default if invalid value + const defaultOption = this.fontSizeOptions.find(opt => opt.value === 'default'); + document.documentElement.style.setProperty('--editor-font-size', defaultOption.size); } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
js/plugins/code/options-component.jsjs/sync/sync.jsjs/wisk.js
🚧 Files skipped from review as they are similar to previous changes (1)
- js/wisk.js
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-12T14:14:36.148Z
Learnt from: sushilbang
Repo: sohzm/wisk PR: 54
File: js/plugins/code/base-text-element.js:58-62
Timestamp: 2025-12-12T14:14:36.148Z
Learning: In the js/plugins/code directory, ensure that BaseTextElement serves as the shared base class with common state/logic, while concrete elements (e.g., TextElement) extend it and override render() to provide UI markup. The base class's connectedCallback should query for child elements that are injected by their render methods. This inheritance pattern should be consistently applied across the plugin system; review new or existing elements to maintain a clean base-vs-extension separation, avoid tight coupling, and verify that the render contracts and DOM querying align between base and derived classes.
Applied to files:
js/plugins/code/options-component.js
🧬 Code graph analysis (1)
js/plugins/code/options-component.js (1)
js/wisk.js (1)
wisk(3-3)
🔇 Additional comments (12)
js/sync/sync.js (6)
1-7: LGTM: Clean state initialization.The Promise-based queue and scheduling flag provide a solid foundation for sequential, debounced saves. The eventLog initialization is straightforward.
10-20: LGTM: Proper event validation and enqueueing.The validation ensures required fields exist, and the nullish coalescing operators provide sensible defaults for timestamp and agent metadata.
129-145: LGTM: Debounced async queue implementation.The promise-chaining pattern with the
saveScheduledflag correctly ensures sequential, non-overlapping saves. Note that errors are logged but not re-thrown, so the returned promise always resolves successfully. This appears intentional for fire-and-forget semantics.
148-209: LGTM: WebSocket cleanup and modernization.The changes improve code clarity with arrow functions, add safety with optional chaining, and enhance debugging with better logging. No functional issues identified.
212-261: LGTM: Page initialization with noted TODOs.The sync and live functions are logically sound. Note the TODO on Line 232 regarding wisk.site integration cleanup, and the commented WebSocket initialization on lines 252-253.
260-264: LGTM: Network status listeners and API exports.The online/offline event listeners provide basic network awareness, and exporting
saveModificationandapplyEventtowisk.synccorrectly exposes the event-driven API surface to the rest of the application.js/plugins/code/options-component.js (6)
526-532: LGTM: Appropriate styling for font size selector.The min/max width constraints on
jalebi-selectand the nowrap styling on labels ensure the font size UI renders consistently across different viewport sizes.
718-718: LGTM: Property declaration follows Lit conventions.The
editorFontSizeproperty with typeStringis correctly declared for reactive updates.
738-747: LGTM: Font size options and initialization.The font size options span a reasonable range (14px to 20px), and the initialization correctly reads from localStorage with a sensible default and applies the setting immediately.
1145-1151: LGTM: Font size change handler.The event handler correctly updates state, persists to localStorage, applies the CSS variable, and triggers a re-render in the proper sequence.
596-601: LGTM: Font size UI integration.The markup correctly binds the select element to the
changeFontSizehandler, maps options from thefontSizeOptionsarray, and manages the selected state.
1091-1108: LGTM: Snapshot restoration now uses event-driven system.The restore flow correctly uses
wisk.sync.newChangewith a restore event andenqueueSaveto persist the change before reloading. Note thatsnapshotIdin the event value is not currently consumed byapplyEventbut could be useful for future logging or audit trails.Based on learnings: The event-driven approach aligns with the PR-wide architectural changes to centralize document mutations through the sync system.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@js/editor.js`:
- Around line 560-567: When replacing the DOM node for a component change inside
handleElementPatch, also update the container's rndr-full-width class just like
handleElementUpdate does: locate the container (use the same lookup used in
handleElementUpdate — e.g., the element's parent container or the id-based
container derived from elementId), then add or remove the 'rndr-full-width'
class according to the patch/fullWidth value or existing element state so the
container width state stays in sync after domElement is replaced.
In `@js/sync/sync.js`:
- Around line 209-215: sendAuth currently assumes
document.querySelector('auth-component') returns an element and calling
getUserInfo() will succeed; if the element is missing the await will throw.
Update sendAuth to first query for the element (auth-component), check for null,
and handle that case (either return early or retry/wait until present), then
call getUserInfo() on the found element and only then call sendMessage with
wisk.editor.pageId and the retrieved token; ensure any errors from getUserInfo()
are caught and logged/handled so sendMessage is not invoked with undefined data.
- Around line 189-197: The error/close handlers on socket currently call alert()
then location.reload(), which discards unsaved entries in wisk.sync.eventLog;
update both socket.addEventListener('error') and 'close' handlers to first flush
pending changes by invoking (or implementing) an async flush/save function on
the sync module (e.g. wisk.sync.flushPending() or wisk.sync.savePendingEvents()
that iterates wisk.sync.eventLog and persists entries and returns a Promise),
await that operation (with a timeout/fallback), then show a non-blocking user
notice and finally call location.reload() only after the flush completes or the
fallback triggers. Ensure you reference socket, wisk.sync.eventLog, and the
new/used flush function (wisk.sync.flushPending or wisk.sync.savePendingEvents)
so reviewers can locate the change.
♻️ Duplicate comments (6)
js/sync/sync.js (1)
46-58: Element reordering may still drop elements not innewOrder.A past review flagged that if
newOrderis missing element IDs, those elements are silently dropped fromdocument.data.elements. The current implementation at line 53 uses.filter(Boolean)which removesundefinedentries (elements not innewOrder). This could cause data loss if the order array is incomplete.#!/bin/bash # Check how elementOrder events are generated to ensure they always include all element IDs rg -n "data.elementOrder" --type js -B 2 -A 5 | head -60js/editor.js (5)
2-4: Unused variabledeletedElementsLeftstill present.A past review noted this variable is declared but never read. It's still being pushed to at line 1065 but never used elsewhere.
#!/bin/bash # Verify deletedElementsLeft is never read rg -n "deletedElementsLeft" --type js -B 1 -A 1
533-541: Missing width class update on component change inhandleElementUpdate.When the component type changes via remote event, the container's
rndr-full-widthclass is not updated based on the new plugin's width property. This was flagged in a past review and the fix was reportedly applied tochangeBlockType, buthandleElementUpdatestill lacks this logic.🐛 Proposed fix
} else if (property === 'component') { const newType = event.value.data; const newDomElement = document.createElement(newType); newDomElement.id = elementId; domElement.replaceWith(newDomElement); + + // Update container width class + const container = document.getElementById(`div-${elementId}`); + if (container) { + const pluginDetail = wisk.plugins.getPluginDetail(newType); + if (pluginDetail?.width === 'max') { + container.classList.add('rndr-full-width'); + } else { + container.classList.remove('rndr-full-width'); + } + } + setTimeout(() => { newDomElement.setValue('', element.value); }, 0); }
1870-1885: Only first element persisted after renumbering.A past review flagged that
renumberNumberedListsonly callsjustUpdatesfor the first element (line 1884), so renumbered items at other positions won't have their updatednumberpersisted.🐛 Proposed fix
+ const changedIds = new Set(); + for (let i = 0; i < elements.length; i++) { const el = elements[i]; if (el.component === 'numbered-list-element') { // ... existing logic ... // Update the element's number const newNumber = counters[indent]; if (domEl.number !== newNumber) { domEl.number = newNumber; domEl.updateNumber(); el.value.number = newNumber; + changedIds.add(el.id); } } else { // Reset counters when encountering non-numbered-list element counters.length = 1; counters[0] = 0; } } // Save modifications - wisk.editor.justUpdates(elements[0]?.id); + changedIds.forEach(id => wisk.editor.justUpdates(id)); }
2644-2646:justUpdates()with no argument is a no-op.Calling
justUpdates()without anelementIdreturns immediately (line 2645). This meansuseTemplate()at line 1227 which callswisk.editor.justUpdates()won't persist the template changes.This was flagged in a past review. The template elements are set directly into
wisk.editor.document.data.elementsat line 1223 but never persisted through the event system.
2693-2708: Potential null reference accessinggetTextContent().text.At line 2695,
domElement.getTextContent().textis accessed without checking ifgetTextContent()returns a valid object. This was flagged in a past review.🐛 Proposed fix
// special case: first element if(elementId === wisk.editor.document.data.elements[0].id) { - const textContent = domElement.getTextContent().text || ''; + const textContentObj = domElement.getTextContent?.(); + const textContent = textContentObj?.text || ''; if(textContent) {
🧹 Nitpick comments (4)
js/elements/selector-element.js (2)
120-122: Silent error handling may confuse users.When page creation fails, the error is only logged to console. The user won't know why their action didn't complete. Consider showing a toast notification.
💡 Proposed improvement
} catch (error) { console.error('Error creating child page:', error); + wisk.utils.showToast?.('Failed to create page. Please try again.', 3000); }
259-263: Fragile selector for first visible button.The selector
:not([style*="display: none"])depends on the exact style string format. Ifdisplay:none(without space) is ever set, this won't match.💡 Suggested improvement
- // Set focus on first visible button - const firstVisible = this.shadowRoot.querySelector('.selector-button:not([style*="display: none"])'); - if (firstVisible) { - firstVisible.classList.add('selector-button-focused'); - } + // Set focus on first visible button + const firstVisible = Array.from(this.shadowRoot.querySelectorAll('.selector-button')) + .find(btn => btn.style.display !== 'none'); + if (firstVisible) { + firstVisible.classList.add('selector-button-focused'); + }js/sync/sync.js (1)
103-106: Minor: Inconsistent indentation.Line 105 has extra leading whitespace compared to surrounding code.
🧹 Fix indentation
// check if the next object is present so a deep path traversal wont crash if(current[key] === null || typeof current[key] !== "object") current[key] = {}; - current = current[key]; + current = current[key]; }js/editor.js (1)
1063-1066: Dead code:deletedElementsLeft.push(elementId).This line adds to
deletedElementsLeftwhich is never read anywhere in the codebase. It should be removed along with the variable declaration at line 4.🧹 Remove dead code
wisk.editor.document.data.elements = wisk.editor.document.data.elements.filter(e => e.id !== elementId); deletedElements.push(elementId); - deletedElementsLeft.push(elementId);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
js/editor.jsjs/elements/selector-element.jsjs/sync/sync.js
🧰 Additional context used
🧬 Code graph analysis (3)
js/elements/selector-element.js (3)
js/editor.js (14)
element(521-521)element(549-549)element(591-591)element(768-768)element(899-899)element(1059-1059)element(1087-1087)element(1100-1100)element(1119-1119)element(1177-1177)element(1265-1265)element(1540-1540)element(1687-1687)e(2758-2758)js/sync/sync.js (2)
element(66-66)element(95-95)js/wisk.js (1)
wisk(3-3)
js/sync/sync.js (3)
js/wisk.js (1)
wisk(3-3)js/editor.js (11)
event(1757-1757)event(1826-1826)pathParts(437-437)pathParts(517-517)pathParts(545-545)pathParts(639-639)e(2758-2758)i(767-767)i(898-898)i(1262-1262)i(1850-1850)js/storage/db.js (1)
data(272-272)
js/editor.js (2)
js/sync/sync.js (6)
elementId(62-62)elementId(94-94)pathParts(26-26)element(66-66)element(95-95)i(89-89)script.js (1)
editor(76-76)
🔇 Additional comments (13)
js/elements/selector-element.js (3)
56-74: LGTM on async selectButton implementation.The method correctly handles the "Page" item by awaiting
createPageAndLink()and returning early, while preserving the original flow for non-Page items. The commented-out code (lines 67-70) should be cleaned up eventually.
159-191: LGTM on visible-button navigation.The navigation logic correctly filters hidden buttons and handles wrap-around at list boundaries. Focus management and smooth scrolling are properly implemented.
274-282: LGTM on show() implementation.Recreating buttons on each show ensures plugin state changes are reflected. The flow of
createAllButtons()followed byrenderButtons('')is correct.js/sync/sync.js (4)
1-8: LGTM on state initialization.The save serialization state (
saveQueue,saveScheduled) and event log are properly initialized. The promise-based queue pattern is a good approach for serializing saves.
120-138: LGTM on save queue serialization.The promise-chaining pattern correctly serializes saves and handles events arriving during persistence by recursively enqueueing follow-up saves.
150-163:syncLogsentries not rolled back on persistence failure.When
wisk.db.setPagefails,wisk.editor.documentis restored fromdocBackup(line 161). However,eventsToSavewere already pushed todocument.data.sync.syncLogsat line 153. Since the backup was taken before applying events, restoring it correctly reverts syncLogs too.Wait - actually the backup is taken at line 145 before events are applied, so the restore at line 161 does revert the syncLogs changes. This is correct.
11-15: Validation order may throw on missingevent.value.If
event.valueisundefined, the checkevent.value.data === undefinedwill throw a TypeError before the validation can reject the event. The!event.valuecheck should short-circuit, but due to operator precedence,event.value.data === undefinedis evaluated whenevent.valueis falsy-but-defined (e.g.,null).🐛 Proposed fix
wisk.sync.newChange = function (event) { - if(!event || !event.path || !event.value || event.value.data === undefined) { + if(!event || !event.path || !event.value || !('data' in event.value)) { console.error("Invalid event: ", event); return; }Likely an incorrect or invalid review comment.
js/editor.js (6)
255-279: LGTM onsetDatabasePropkey sanitization.The key validation at line 257 correctly rejects keys containing
.,[, or]to prevent path injection attacks. This addresses the past review concern.
338-357: LGTM on block creation event emission.The block creation now properly emits
data.elementsanddata.elementOrderevents with timestamps and usesenqueueSavefor serialized persistence. This addresses past concerns about overlapping saves.
1112-1168: LGTM onchangeBlockTypeimplementation.The function correctly emits a patch event, updates the element state, replaces the DOM element, and properly handles the container's
rndr-full-widthclass based on the new plugin's width property. Focus restoration is a good UX touch.
2342-2392: LGTM on clipboard HTML sanitization.The function properly sanitizes link protocols, only allowing
http:,https:,mailto:, andtel:. Dangerous protocols likejavascript:anddata:are correctly blocked. The approach of removing all attributes then adding back only clean ones is secure.
2755-2785: LGTM on virtual keyboard handling.The implementation correctly adjusts editor padding when the keyboard is visible and scrolls the focused element into view. The shadow DOM focus detection (lines 2771-2772) properly handles custom elements.
1719-1752: LGTM on drag cleanup implementation.The
cleanupDragfunction properly removes all event listeners and resets state. The Escape key handler and blur event listener provide good UX for canceling drags.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| // Handle component change (must be done before value update) | ||
| if (patch.component && patch.component !== domElement.tagName.toLowerCase()) { | ||
| const newType = patch.component; | ||
| const newDomElement = document.createElement(newType); | ||
| newDomElement.id = elementId; | ||
| domElement.replaceWith(newDomElement); | ||
| domElement = newDomElement; // update reference for value setting | ||
| } |
There was a problem hiding this comment.
Missing width class update in handleElementPatch for component changes.
Same issue as handleElementUpdate: when a patch changes the component type, the container's rndr-full-width class should be updated.
🐛 Proposed fix
// Handle component change (must be done before value update)
if (patch.component && patch.component !== domElement.tagName.toLowerCase()) {
const newType = patch.component;
const newDomElement = document.createElement(newType);
newDomElement.id = elementId;
domElement.replaceWith(newDomElement);
domElement = newDomElement; // update reference for value setting
+
+ // Update container width class
+ const container = document.getElementById(`div-${elementId}`);
+ if (container) {
+ const pluginDetail = wisk.plugins.getPluginDetail(newType);
+ if (pluginDetail?.width === 'max') {
+ container.classList.add('rndr-full-width');
+ } else {
+ container.classList.remove('rndr-full-width');
+ }
+ }
}🤖 Prompt for AI Agents
In `@js/editor.js` around lines 560 - 567, When replacing the DOM node for a
component change inside handleElementPatch, also update the container's
rndr-full-width class just like handleElementUpdate does: locate the container
(use the same lookup used in handleElementUpdate — e.g., the element's parent
container or the id-based container derived from elementId), then add or remove
the 'rndr-full-width' class according to the patch/fullWidth value or existing
element state so the container width state stays in sync after domElement is
replaced.
| socket.addEventListener('error', () => { | ||
| alert('Connection with server failed. Click OK to reload the page.'); | ||
| location.reload(); | ||
| }); | ||
|
|
||
| socket.addEventListener('close', event => { | ||
| socket.addEventListener('close', () => { | ||
| alert('Connection with server closed. Click OK to reload the page.'); | ||
| location.reload(); | ||
| }); |
There was a problem hiding this comment.
WebSocket error/close handlers force reload without save.
If the WebSocket closes unexpectedly, pending changes in wisk.sync.eventLog may be lost since alert() blocks and then location.reload() discards unsaved state. Consider flushing pending saves before reload.
💡 Suggested improvement
- socket.addEventListener('error', () => {
- alert('Connection with server failed. Click OK to reload the page.');
- location.reload();
+ socket.addEventListener('error', async () => {
+ try {
+ if (wisk.sync.eventLog?.length > 0) {
+ await saveModification();
+ }
+ } catch (e) {
+ console.error('Failed to save before reload:', e);
+ }
+ alert('Connection with server failed. Click OK to reload the page.');
+ location.reload();
});🤖 Prompt for AI Agents
In `@js/sync/sync.js` around lines 189 - 197, The error/close handlers on socket
currently call alert() then location.reload(), which discards unsaved entries in
wisk.sync.eventLog; update both socket.addEventListener('error') and 'close'
handlers to first flush pending changes by invoking (or implementing) an async
flush/save function on the sync module (e.g. wisk.sync.flushPending() or
wisk.sync.savePendingEvents() that iterates wisk.sync.eventLog and persists
entries and returns a Promise), await that operation (with a timeout/fallback),
then show a non-blocking user notice and finally call location.reload() only
after the flush completes or the fallback triggers. Ensure you reference socket,
wisk.sync.eventLog, and the new/used flush function (wisk.sync.flushPending or
wisk.sync.savePendingEvents) so reviewers can locate the change.
| async function sendAuth() { | ||
| const user = await document.querySelector('auth-component').getUserInfo(); | ||
| sendMessage(JSON.stringify({ | ||
| id: wisk.editor.pageId, | ||
| token: user.token, | ||
| })); | ||
| } |
There was a problem hiding this comment.
sendAuth may send message before user info is ready.
The function awaits getUserInfo() but if auth-component doesn't exist, querySelector returns null and the await will throw.
🐛 Proposed fix
async function sendAuth() {
- const user = await document.querySelector('auth-component').getUserInfo();
+ const authComponent = document.querySelector('auth-component');
+ if (!authComponent) {
+ console.warn('Auth component not found, skipping auth');
+ return;
+ }
+ const user = await authComponent.getUserInfo();
sendMessage(JSON.stringify({
id: wisk.editor.pageId,
token: user.token,
}));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async function sendAuth() { | |
| const user = await document.querySelector('auth-component').getUserInfo(); | |
| sendMessage(JSON.stringify({ | |
| id: wisk.editor.pageId, | |
| token: user.token, | |
| })); | |
| } | |
| async function sendAuth() { | |
| const authComponent = document.querySelector('auth-component'); | |
| if (!authComponent) { | |
| console.warn('Auth component not found, skipping auth'); | |
| return; | |
| } | |
| const user = await authComponent.getUserInfo(); | |
| sendMessage(JSON.stringify({ | |
| id: wisk.editor.pageId, | |
| token: user.token, | |
| })); | |
| } |
🤖 Prompt for AI Agents
In `@js/sync/sync.js` around lines 209 - 215, sendAuth currently assumes
document.querySelector('auth-component') returns an element and calling
getUserInfo() will succeed; if the element is missing the await will throw.
Update sendAuth to first query for the element (auth-component), check for null,
and handle that case (either return early or retry/wait until present), then
call getUserInfo() on the found element and only then call sendMessage with
wisk.editor.pageId and the retrieved token; ensure any errors from getUserInfo()
are caught and logged/handled so sendMessage is not invoked with undefined data.
1 participant
Summary by CodeRabbit
New Features
Improvements
Style
✏️ Tip: You can customize this high-level summary in your review settings.