Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The critical renderer-isolation issue and moderate theme-color issue remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Re-implements Mermaid, KaTeX, and table previews as an in-editor web overlay instead of native Swift popovers.
Changes:
- Adds web renderers, sanitization, overlay UI, localization, and tests.
- Removes native Previewer implementations, resources, targets, and bridges.
- Updates shared types, project configuration, styling, and bundled-resource checks.
Review findings: Critical (3 votes) in loadModule.ts: CDN renderers run in the privileged editor context and require isolation or integrity-pinned bundling. Moderate (1 vote) in index.css: canvas colors do not follow selected editor themes.
File summaries
| File | Change |
|---|---|
MarkEditMac/Sources/Main/AppResources.swift |
Adds localized close-button text. |
MarkEditMac/Sources/Editor/Controllers/EditorViewController+Preview.swift |
Removes native preview presentation. |
MarkEditMac/Sources/Editor/Controllers/EditorViewController+Delegate.swift |
Removes preview delegate handling. |
MarkEditMac/Sources/Editor/Controllers/EditorViewController.swift |
Removes native preview setup. |
MarkEditMac/Resources/Localizable.xcstrings |
Adds Close translations. |
MarkEditMac/mul.lproj/Main.xcstrings |
Normalizes file formatting. |
MarkEditMac/Modules/Sources/SharedUI/FocusTrackingView.swift |
Removes obsolete focus view. |
MarkEditMac/Modules/Sources/Previewer/Resources/table.html |
Removes old table template. |
MarkEditMac/Modules/Sources/Previewer/Resources/mermaid.html |
Removes old Mermaid template. |
MarkEditMac/Modules/Sources/Previewer/Resources/katex.html |
Removes old KaTeX template. |
MarkEditMac/Modules/Sources/Previewer/Previewer.swift |
Removes native Previewer implementation. |
MarkEditMac/Modules/Package.swift |
Removes the Previewer target. |
MarkEditKit/Sources/Bridge/Web/Generated/WebBridgeSelection.swift |
Relocates generated WebRect. |
MarkEditKit/Sources/Bridge/Native/Modules/EditorModulePreview.swift |
Removes the native preview module. |
MarkEditKit/Sources/Bridge/Native/Generated/NativeModulePreview.swift |
Removes the generated preview bridge. |
MarkEditCore/Sources/EditorSharedTypes.swift |
Removes shared WebRect. |
MarkEditCore/Sources/EditorLocalizable.swift |
Adds close-button localization. |
MarkEdit.xcodeproj/project.pbxproj |
Removes Previewer project references. |
CoreEditor/tsconfig.json |
Enables Vite client types. |
CoreEditor/test/preview.test.ts |
Tests overlay behavior and table sanitization. |
CoreEditor/test/build.test.ts |
Updates bundled-resource assertions. |
CoreEditor/src/styling/views/index.ts |
Removes preview position data. |
CoreEditor/src/modules/preview/renderers/table.ts |
Adds sanitized table rendering. |
CoreEditor/src/modules/preview/renderers/mermaid.ts |
Adds Mermaid rendering. |
CoreEditor/src/modules/preview/renderers/loadModule.ts |
Loads renderer dependencies dynamically. |
CoreEditor/src/modules/preview/renderers/katex.ts |
Adds KaTeX rendering and stylesheet loading. |
CoreEditor/src/modules/preview/render.ts |
Coordinates preview rendering and errors. |
CoreEditor/src/modules/preview/overlay.ts |
Implements the modal preview overlay. |
CoreEditor/src/modules/preview/index.ts |
Routes preview actions to the overlay. |
CoreEditor/src/modules/preview/index.css |
Styles the preview overlay and content. |
CoreEditor/src/config.ts |
Adds close-button localization configuration. |
CoreEditor/src/bridge/native/preview.ts |
Removes native preview declarations. |
CoreEditor/src/@types/global.d.ts |
Removes preview bridge typings. |
CoreEditor/index.ts |
Removes native preview setup and adds fallback localization. |
Review details
Suppressed comments (1)
CoreEditor/src/modules/preview/index.css:29
CanvasandCanvasTextfollow the system appearance rather than the selected editor theme. For example, Solarized Light supplies a#fdf6e3editor background (AppTheme.swift:248-255), while the editor appliescolors.backgrounddirectly (builder.ts:135-138); because this dialog is a sibling of.cm-editor, it will render with a different white/black surface and text colors. Use the active editor colors for the overlay instead of system canvas keywords.
background: Canvas;
color: CanvasText;
- Files reviewed: 30/34 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
3f8f36a to
076d27b
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
Five moderate preview behavior and accessibility issues remain, with additional renderer test coverage gaps.
Review details
Suppressed comments (7)
CoreEditor/src/modules/preview/frame/index.ts:24
- This listener is one-shot, so a later reload of the iframe gets no
render-previewmessage. The oldPreviewWebViewexplicitly hid Reload because it emptied the preview; with an iframe, a WebKit/context-menu reload now leaves the new document blank. Keep this listener for subsequent loads (or resend the code after reload).
frame.addEventListener('load', () => {
frame.contentWindow?.postMessage({ type: 'render-preview', code }, '*');
}, { once: true });
CoreEditor/src/modules/preview/frame/index.ts:43
- This CSP allows images only from
data:. A valid GFM table cell such as|  |now renders without its image, whereas the removed table renderer had no such restriction. Either allow the intended image origins or explicitly remove images during sanitization so this behavior is deliberate rather than a silent rendering regression.
const policy = frameDocument.createElement('meta');
policy.httpEquiv = 'Content-Security-Policy';
policy.content = "default-src 'none'; script-src 'unsafe-inline' https://cdn.jsdelivr.net; style-src 'unsafe-inline' https://cdn.jsdelivr.net; font-src https://cdn.jsdelivr.net; img-src data:";
CoreEditor/src/modules/preview/frame/script.js:24
{ once: true }removes this listener on the firstmessageevent even when the guard rejects it. If any unrelated message reaches the sandbox before the parent’s render request, the listener is gone and the preview remains empty witharia-busyset; remove the listener only after accepting a valid render request.
}, { once: true });
CoreEditor/src/modules/preview/index.css:29
- Using the system
Canvasbackground loses the selected editor theme's actual background. The previous native preview appliedtransientTintColor, and themes such as Dracula use#282a36;Canvasonly follows the light/dark appearance, so opening a preview visibly changes the background for custom/tinted themes. Propagate the current editor background/tint to the overlay and frame instead of relying onCanvas.
background: Canvas;
color: CanvasText;
CoreEditor/src/modules/preview/index.css:64
outline: nonesuppresses the browser focus indicator, while the replacement rule only applies after aTabkeydown adds.cm-previewKeyboardFocus. Because this button isautofocus, opening the preview from the keyboard initially leaves the focused close control without a visible indicator. Use:focus-visibledirectly or apply the keyboard-focus class when focus is keyboard-originated.
outline: none;
CoreEditor/src/modules/preview/renderers/katex.ts:16
- This new renderer has no test coverage for loading the KaTeX module and stylesheet or for inserting the rendered output. The current tests cover table sanitization only, so a CDN export change or stylesheet-load failure could regress math previews without being detected. Add mocked-loader tests for successful output and the stylesheet rejection path.
export async function renderKatex(container: HTMLElement, code: string, loadModule: ModuleLoader): Promise<void> {
const [{ default: katex }] = await Promise.all([
loadModule<{ default: Katex }>('https://cdn.jsdelivr.net/npm/katex@0.18.7/dist/katex.min.mjs'),
loadMathStyles(),
]);
container.innerHTML = katex.renderToString(
code,
{ throwOnError: false, displayMode: true },
);
CoreEditor/src/modules/preview/renderers/mermaid.ts:12
- This new renderer has no test coverage for its actual Mermaid integration. The existing preview tests only mock
renderPreviewand inspect the frame/message plumbing, so a changedmermaid.rendersignature or the dark-mode branch could leave Mermaid previews broken while CI stays green. Add a mocked-loader test that exercises successful rendering and the rejection path, following the existingrenderTabletests.
export async function renderMermaid(container: HTMLElement, code: string, loadModule: ModuleLoader): Promise<void> {
const { default: mermaid } = await loadModule<{ default: Mermaid }>('https://cdn.jsdelivr.net/npm/mermaid@12.0.0/dist/mermaid.esm.min.mjs');
const theme = matchMedia('(prefers-color-scheme: dark)').matches ? 'dark' : 'default';
mermaid.initialize({ theme, startOnLoad: false });
container.innerHTML = (await mermaid.render('markedit-diagram', code, container)).svg;
- Files reviewed: 36/40 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
Unresolved moderate issues affect iframe reload recovery and initial keyboard focus; renderer test coverage also has a nit.
Review details
Suppressed comments (3)
CoreEditor/src/modules/preview/frame/index.ts:25
- This one-shot load handler does not run again if the sandboxed iframe is reloaded (for example, through WebKit's Reload context-menu item). The new srcdoc starts with an empty container and waits for this postMessage, so reloading leaves the preview blank; keep the load and completion listeners active until
preview-close, or otherwise suppress iframe reloads.
frame.addEventListener('load', () => {
frame.contentWindow?.postMessage({ type: 'render-preview', code }, '*');
}, { once: true });
CoreEditor/src/modules/preview/index.css:64
- This removes the browser's focus outline unconditionally, but the replacement outline is only enabled after a Tab keydown adds
cm-previewKeyboardFocus. Because the close button is autofocus, it starts focused immediately aftershowModal()yet has no visible focus indicator, making the primary keyboard control hard to locate; preserve the UA focus ring (or otherwise mark the initial focus) here.
outline: none;
CoreEditor/src/modules/preview/render.ts:19
- The overlay tests mock
renderPreview, and only the table helper is exercised directly; the newly added Mermaid and KaTeX paths are never executed. Add renderer tests with a fake module loader (including Mermaid's render call and KaTeX stylesheet load) so incorrect CDN module shapes or renderer behavior cannot ship unnoticed.
case 'mermaid': return renderMermaid;
case 'katex': return renderKatex;
case 'table': return renderTable;
- Files reviewed: 36/40 changed files
- Comments generated: 0 new
- Review effort level: Lite
No description provided.