Repository navigation
Conversation
…Mermaid, JoyPixels
|
@itznan is attempting to deploy a commit to the Developer Labs Team on Vercel. A member of the Team first needs to authorize it. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
ThisIs-Developer
left a comment
ThisIs-Developer
left a comment
There was a problem hiding this comment.
Hi @itznan,
First of all, this is really great work. I honestly never dared to touch this issue because of how many rendering libraries are involved, so I really appreciate the effort you’ve put into it.
I haven’t done a full review yet. I only tested the PR briefly from a user-experience perspective, but I noticed a few concerns that I want to share before going deeper.
-
MathJax style changes don’t seem to apply correctly.
When I select another MathJax style, the rendered math often doesn’t change. I need to refresh, and even then it doesn’t always seem to apply. -
LaTeX rendering feels quite slow.
LaTeX rendering time is noticeably higher. -
The Visual Settings panel becomes laggy with large LaTeX documents.
I imported a Markdown file containing many different LaTeX expressions, then opened the Visual Settings menu. The application became quite unresponsive, and even scrolling inside the settings panel was hanging. -
Emoji skin tone didn’t work for me.
I changed the skin tone and also tried refreshing, but I couldn’t see the expected change.
I also ran an additional code review (Codex) and found a few technical concerns:
Codex Review — 4 P2 Issues Found in PR #278
1. Math font selection does not change rendered math.
Selecting STIX Two updates the configuration, but the real MathJax renderer remains
MathJaxModern. Rebuilding the output jax retains the existing font data.Reference: [visual-settings-adapters.js:209](https://github.com/ThisIs-Developer/Markdown-Viewer/pull/278/files)
2. Dark syntax themes become difficult to read in light mode.
With Monokai selected, both
<pre>and<code>have transparent backgrounds, leaving light text (#ddd) on the application's pale background (#f6f8fa). The selected theme's background needs to override the existing preview styles.Reference: [visual-settings-adapters.js:109](https://github.com/ThisIs-Developer/Markdown-Viewer/pull/278/files)
3. Three syntax themes request nonexistent stylesheets.
Dracula, Solarized Dark, and Solarized Light all return HTTP 404. Their files are under
styles/base16/, while the adapter constructs URLs directly understyles/.Reference: [visual-settings-adapters.js:131](https://github.com/ThisIs-Developer/Markdown-Viewer/pull/278/files)
4. Settings do not refresh the secondary split preview.
With
:wave:in both documents, selectingtone3changes the primary preview to 👋🏽 while the secondary remains 👋. The settings handler only rerenders the primary preview; it also needs to invalidate and refresh the secondary.Reference: [script.js:24174](https://github.com/ThisIs-Developer/Markdown-Viewer/pull/278/files)
If you would like to continue improving this PR, let me know. I can do a much deeper review, and we can work through these issues together. This is only my initial review.
I also want to ask your opinion about the feature itself. From my perspective, I’m not completely sure how many users actually need things like changing MathJax fonts or switching between many rendering themes. I understand that different users have different preferences, so I’d genuinely like to know your perspective as well. Do you think this level of visual customization is something Markdown Viewer users would actively use?
Thanks again for taking on such a complex feature.
ThisIs-Developer
added
accessibility
enhancement
ChatGPT Codex Connector
ThisIs-Developer
left a comment
ThisIs-Developer
left a comment
There was a problem hiding this comment.
Codex review
Found five P2 issues that should be addressed before merging: math font changes do not reach the renderer, Mermaid ignores the export theme, Docker omits the new scripts, three highlight.js themes use nonexistent URLs, and the secondary document preview retains the previous settings. Details and reproduction evidence are attached inline.
Reviewed commit 2296a16f4bdb81666d307e3648b44914d0a20a40.
Validation:
- All 32 unit tests passed.
- All three new visual-settings Chromium tests passed.
- The existing
dark vector PDF keeps tables and diagrams dark in print mediatest failed on this PR and passed when using the base commit's application files (9ce2de9). - Browser checks with the real libraries reproduced the MathJax font and secondary-preview issues. The Docker asset omission was verified against its COPY list and reproduced by serving the missing scripts as 404s.
- The build passed after temporarily normalizing Windows line endings in
sitemap.xml; its content already matches the base branch. No source changes were retained.
| const newOutputJax = window.MathJax.startup.getOutputJax(); | ||
| if (newOutputJax) { | ||
| window.MathJax.startup.output = newOutputJax; | ||
| if (typeof window.MathJax.startup.getDocument === 'function') { | ||
| window.MathJax.startup.document = window.MathJax.startup.getDocument(); |
There was a problem hiding this comment.
[P2] Activate the selected font data before rebuilding MathJax
After math has rendered, selecting STIX Two or TeX (Classic) updates the setting and downloads the component, but getOutputJax() still uses the existing MathJax.config.chtml.fontData. Browser verification with the real MathJax 4 beta showed both the font constructor and configured font data remaining MathJaxModern, with the same MJX-MM glyph fonts after either selection. Activate the newly loaded font data before recreating the output renderer so the setting changes the rendered math.
| getMermaidConfig: (themeOverride) => ({ | ||
| theme: resolveMermaidThemeName(_activeMermaidTheme), | ||
| look: _activeMermaidLook === 'handDrawn' ? 'handDrawn' : 'classic' |
There was a problem hiding this comment.
[P2] Preserve the export theme when resolving Auto
getMermaidConfig() ignores themeOverride, so with Mermaid set to Auto, exporting a dark PDF from a light workspace produces light Mermaid diagrams. The existing tests/e2e/export.spec.js test dark vector PDF keeps tables and diagrams dark in print media fails on this PR: node/text colors remain the light palette. The same test passes with the base commit's application files. Resolve Auto using the supplied export theme when present, falling back to the workspace theme otherwise.
| <script src="visual-settings-adapters.js" defer></script> | ||
| <script src="visual-settings-store.js" defer></script> |
There was a problem hiding this comment.
[P2] Include both new scripts in the Docker image
Dockerfile copies an explicit list of files and was not updated to include either of these scripts. Docker deployments therefore return 404 for both URLs, leaving the store and adapters undefined. Reproducing those missing assets produces a Visual Library Settings panel with five empty dropdowns, so the new feature cannot be used in the Docker distribution. Add both files to the Docker build.
| const effectiveName = resolveHljsThemeName(_activeHljsTheme); | ||
| const href = `${HLJS_CDN_BASE}${effectiveName}.min.css`; |
There was a problem hiding this comment.
[P2] Map the Base16 themes to their actual stylesheet paths
The generated URLs for dracula, solarized-dark, and solarized-light return 404 in the pinned highlight.js 11.9.0 release. These stylesheets are under styles/base16/, where all three URLs return 200. Selecting any of these options saves the preference but does not load the requested palette. Map these options to the correct asset paths instead of treating every theme as a file directly under styles/.
| console.warn('Error applying visual setting via adapter:', err); | ||
| } | ||
| } | ||
| renderMarkdown({ force: true, forceAdvancedPostProcess: true }); |
There was a problem hiding this comment.
[P2] Refresh the secondary document preview after settings changes
This rerenders only the primary preview. Open two documents side by side in preview mode, include :wave: in each, and select skin tone tone3: the primary wave changes to the selected tone while the secondary remains yellow. The secondary preview also caches by document content, so merely calling its render function without invalidating that cache will retain the old appearance. Invalidate and refresh the secondary preview when rendering settings change.
… Docker and SW (P3)
…ebounce renders (P6, P8)
…yles, theme contrast, and emoji tones (P1, P2, P4, P5, P7, P8)
|
Thank you for the thorough initial review and feedback! I've addressed all the UX concerns and Codex issues, and pushed the updates to the PR ( Here is a summary of what was improved and how each issue was resolved: 1. MathJax Font Switching & Live Application (P1)
2. Dark Vector PDF Mermaid Regression (P2)
3. Docker & Distribution Asset Completeness (P3)
4. Highlight.js Base16 Stylesheet 404s (P4)
5. Dark Syntax Theme Contrast in Light Mode (P5)
6. Secondary Split Document Preview Synchronization (P6)
7. Emoji Skin Tone Application (P7)
8. LaTeX Performance & Settings Panel Responsiveness (P8)
9. Build Housekeeping (P9)
10. DeepScan Static Analysis
Regarding Your Question on Customization ValueRegarding your point about whether this level of customization is something users actively need: I think you raise a valid consideration. My perspective:
Could you please check out the latest changes and let me know if this is what you were looking for? Happy to make any further adjustments! |
|
Hi @itznan, First of all, thank you for resolving all the issues. That’s really great, and I appreciate the amount of work you’ve put into this. Coming to the question of whether we should continue with this feature—yes, I’d like to continue with it. The perspective you shared also makes sense to me. This does not seem like a heavy feature that would significantly affect the overall performance of the application, especially since the additional themes and fonts are loaded only when needed. Many other editors and applications also provide this kind of customization, so I think it makes sense for Markdown Viewer to support it as well. From a UX and user perspective, I tested the latest version again and everything looks much better now. I’ve also given the updated code to Codex for another code review, and I’ll review those results and share the findings with you as soon as possible. Thanks again for picking up this issue, continuing with the feature, and actually making it real. I really appreciate the effort you’ve put into it. |
ThisIs-Developer
left a comment
ThisIs-Developer
left a comment
There was a problem hiding this comment.
Codex review — six confirmed issues
Reviewed PR #278 at e5a4a6104022cf0aeafccd52819fa6bf44f6627f against base 9ce2de94c167deb76ccadbb976654791a059d5e9. The PR head was rechecked before posting and is unchanged. The six current findings are attached inline with reproduction details, observed behavior, and suggested fixes.
| # | Priority | Finding | Code reference |
|---|---|---|---|
| 1 | P1 | Keep the desktop MathJax bundle compatible with the new configuration | desktop-app/resources/js/script.js:4144–4145 |
| 2 | P2 | Switch math font resources along with the font class | visual-settings-adapters.js:325–326 |
| 3 | P2 | Resolve automatic code colors against the PDF export theme | visual-settings-adapters.js:190–195 |
| 4 | P2 | Remove the built-in token palette when applying a syntax theme | visual-settings-adapters.js:217–223 |
| 5 | P2 | Canonicalize emoji aliases before adding a skin-tone suffix | visual-settings-adapters.js:444–450 |
| 6 | P2 | Load Modern when resetting after an alternate-font reload | visual-settings-adapters.js:292–296 |
Validation and coverage
npm run build: passed.npm run test:unit: 32/32 passed.- Chromium/Chrome Playwright coverage across
visual-settings.spec.js,export.spec.js,rich-content.spec.js,startup-loading.spec.js, andreview-mode.spec.js: 34/34 passed. - Additional browser checks used the real rendering libraries and inspected network responses, computed token colors, font classes/resources, and rendered output. These checks exposed failures that the existing passing tests do not assert.
| Feature | Additional checks | Result |
|---|---|---|
| Syntax highlighting | All 15 choices in light and dark app modes; identical highlighted JavaScript compared with the selected stylesheet in isolation | All stylesheets loaded; 10 choices had token-color conflicts in both modes (finding 4) |
| MathJax | All 10 font choices; live switching, reload, representative equations, and reset | Live resource switching and reset failures confirmed (findings 2 and 6) |
| Desktop LaTeX compatibility | Base versus PR configuration using the actual bundled MathJax 3.2.2 dependency | Default math startup regression confirmed (finding 1) |
| PDF export | Auto syntax highlighting with light app/dark export and dark app/light export; base comparison | Code-block export theme regression confirmed (finding 3) |
| Mermaid | 6 themes × 2 looks × 2 app modes × 4 diagram types (flowchart, sequence, class, pie): 96 rendered SVGs; representative reload/reset | All tested SVGs rendered with valid dimensions and no console/page errors; no additional Mermaid defect confirmed |
| Emoji | Default plus 5 skin tones in both app modes; canonical names, aliases, explicit tones, unsupported names, native Unicode, and inline code | Canonical examples followed the setting; aliases failed (finding 5); explicit tones and protected/unsupported content were preserved |
Mermaid's HandDrawn appearance varies by supported diagram type; an unchanged sequence/pie appearance alone was not treated as a PR regression.
Assessment: the feature is not ready to be described as working for every option. These are all six confirmed findings from this review, not a guarantee that no other defects exist. Browser verification was performed in Chromium/Chrome with representative fixtures; Firefox, WebKit, every document/option combination, and a packaged native desktop run were outside the verified coverage.
| chtml: { | ||
| font: initialFont |
There was a problem hiding this comment.
[P1] Keep the desktop MathJax bundle compatible with the new configuration
The desktop configuration now passes a font name string to chtml.font, but desktop-app/prepare.js:274–275 still downloads MathJax 3.2.2 and the desktop loader uses that local bundle. MathJax 3 expects a font object here. This breaks ordinary desktop LaTeX rendering even when the user leaves the default font selected.
Reproduction: render $$x^2$$ using the desktop dependency version with this configuration. In an isolated real-library comparison, the base configuration resolves startup and renders one equation; the PR configuration reports MathJax(?): t.font.adaptiveCSS is not a function, leaves startup pending, never exposes typesetPromise, and renders zero equations. The application harness with the same dependency substitution also leaves raw LaTeX visible.
Suggested fix: update the desktop bundle and configuration together, or use version-compatible configuration for the existing desktop dependency. This was reproduced against the actual 3.2.2 library in a browser harness; a packaged native desktop application was not run.
| if (fontClass) { | ||
| window.MathJax.config.chtml.fontData = fontClass; |
There was a problem hiding this comment.
[P2] Switch math font resources along with the font class
Changing chtml.fontData and rebuilding the output jax retains font resource configuration from the previous font. The selected font class changes, but its glyphs are requested from the wrong package, so the live preview does not reliably render the selected mathematical font.
Reproduction: start with Modern and a rendered equation, then select STIX Two without reloading. The renderer becomes MathJaxStix2, but its fontURL remains https://cdn.jsdelivr.net/npm/mathjax-modern-font/chtml/woff; requests such as .../mathjax-modern-font/chtml/woff/mjx-stx-zero.woff return 404. All nine alternate fonts showed failed live font loads in the Modern-boot matrix. A further case—reload with STIX, then switch to Asana in a document containing \mathbb{R}—reduced five rendered equations to zero and rejected startup with Error: dynamic file 'double-struck' failed to load.
Suggested fix: initialize the selected font's complete resource configuration, including its WOFF URL and dynamic font resources, before rebuilding/typesetting. Verify loaded glyph resources and actual rendered equations; checking only the selected setting or font class misses this failure.
| @media print { | ||
| .markdown-body pre:has(> code.hljs), | ||
| .markdown-body pre > code.hljs, | ||
| .markdown-body code.hljs { | ||
| background-color: ${bg} !important; | ||
| color: ${fg} !important; |
There was a problem hiding this comment.
[P2] Resolve automatic code colors against the PDF export theme
These global !important code-block rules use colors resolved from the application theme and also apply to the export document. With the syntax theme set to Auto, they override the selected PDF theme's code-block styling.
Reproduction: use the light application theme and Auto syntax highlighting, then choose a dark PDF export: the page is dark but code blocks retain a white background. Reversing the themes produces dark code blocks on the light export. The same fixture using the base commit's application files produced the expected code-block colors in both directions.
Suggested fix: resolve Auto against the requested export theme for export rendering, or scope preview-only helper rules so they do not override the export styles.
| let link = document.getElementById('mdv-hljs-theme-link'); | ||
| if (!link) { | ||
| link = document.createElement('link'); | ||
| link.id = 'mdv-hljs-theme-link'; | ||
| link.rel = 'stylesheet'; | ||
| link.crossOrigin = 'anonymous'; | ||
| if (document.head) document.head.appendChild(link); |
There was a problem hiding this comment.
[P2] Remove the built-in token palette when applying a syntax theme
Appending the selected theme stylesheet leaves the existing token colors in styles.css:4115–4204 active. More-specific selectors such as .hljs-title.function_ win over the new theme, and tokens without an explicit rule in the selected theme can retain the old palette. The stylesheet loads and the background changes, but the resulting code colors only partially match the selection.
Reproduction: select Monokai and render a JavaScript function such as function hello() { return 42; }. The function name keeps the legacy purple rgb(111, 66, 193) instead of Monokai's green rgb(166, 226, 46). Comparing the same highlighted HTML against an isolated iframe containing only the selected stylesheet confirmed mismatches in 10 of 15 choices, in both light and dark app modes: Monokai, Dracula, Atom One Dark, Atom One Light, Solarized Dark, Solarized Light, VS, VS2015, Nord, and Default. All stylesheet URLs loaded successfully; this is a cascade conflict.
Suggested fix: scope or disable the built-in token palette while an external theme is active, and compare actual token colors in regression coverage rather than asserting only that the link URL changed. The other five choices matched the representative JavaScript fixture; that does not cover every language/token.
| const toneCandidate = `${shortcode}_${_activeEmojiSkinTone}`; | ||
| const joy = typeof joypixels !== 'undefined' ? joypixels : (typeof window !== 'undefined' ? window.joypixels : undefined); | ||
| if (joy && joy.emojiList) { | ||
| if (joy.emojiList[`:${toneCandidate}:`]) { | ||
| return toneCandidate; | ||
| } | ||
| return shortcode; |
There was a problem hiding this comment.
[P2] Canonicalize emoji aliases before adding a skin-tone suffix
This lookup adds _toneN to the original shortcode without first resolving JoyPixels aliases. A supported emoji can therefore remain at its default tone when its alias is used, even though the equivalent canonical shortcode follows the selected setting.
Reproduction: render :thumbsup: :+1: :runner: and select any of tone1 through tone5. :thumbsup: follows the selected tone, while :+1: stays yellow; :runner: also remains at its default tone. This reproduced for all five non-default tone choices in both light and dark mode. Canonical shortcodes such as :wave: and :thumbsup: worked, and explicitly toned shortcodes were preserved.
Suggested fix: resolve the input to JoyPixels' canonical shortcode before checking for a toned variant, while preserving explicit tones and leaving genuinely unsupported emojis unchanged.
| // Load external font script if needed (Modern is bundled by default) | ||
| if (_activeMathFont !== 'mathjax-modern') { | ||
| const fontUrl = `${MATHJAX_FONT_CDN_BASE}${_activeMathFont}-font/chtml.js`; | ||
| try { | ||
| await loadScriptOnce(fontUrl); |
There was a problem hiding this comment.
[P2] Load Modern when resetting after an alternate-font reload
The assumption that Modern is always bundled is false after startup with a persisted alternate font. In that case only the alternate font package is registered. Skipping Modern's loader means there is no Modern class to install, and the later fontData assignment leaves the previous renderer active.
Reproduction: select STIX Two, reload, then click Reset to defaults. The store, selector, and adapter report mathjax-modern, but the output remains MathJaxStix2 with prefix STX and fontData: MathJaxStix2; the registered font packages still contain only mathjax-stix2. A minimal $$x^2$$ document reproduces this, independently of the missing-glyph-resource problem in finding 2. Reset after a Termes reload likewise retained Termes.
Suggested fix: load the Modern package whenever it is absent, including reset/fallback paths, and verify that the active renderer class and font resources match the restored setting.
|
Hi @itznan, I know I’ve shared multiple rounds of feedback. I’m only doing this because I don’t want the feature to become slow or unstable — I want it to be as good as possible before merging. Thank you again for all the work you’re putting into it. Really appreciate it. |
Summary
Implements a configurable visual library settings panel allowing users to customize rendering appearances for highlight.js, MathJax, Mermaid, and JoyPixels emojis. Settings update live without full page reload, persist across reloads via
localStorage(mdv.visualSettings.v1), and support light/dark modes and mobile viewports.Closes #129
Configurable Settings & Options
hljsTheme):auto(follows app light/dark mode) + 14 themes (github,github-dark,monokai,dracula,atom-one-dark,atom-one-light,solarized-dark,solarized-light,vs,vs2015,nord,tokyo-night-dark,tokyo-night-light,default).mathFont): 10 mathematical fonts (mathjax-modern,mathjax-tex,mathjax-stix2,mathjax-asana,mathjax-bonum,mathjax-dejavu,mathjax-fira,mathjax-pagella,mathjax-schola,mathjax-termes).mermaidTheme):auto,default,neutral,dark,forest,base.mermaidLook):classic,handDrawn.emojiSkinTone):default(yellow),tone1-tone5(Fitzpatrick modifiers). Preserves explicit tones and leaves unsupported emojis untouched.Library & Architecture Updates
4.0.0-beta.7) with SRI verification (sha384-2f5bAKjuFIbbQ+0O9aSu5W1OyLA4q80QrDEvjXDwJexjU9VqoJhMgw19pYVpSJab) to enable dynamic on-demand mathematical font switching.visual-settings-store.js) from library adapters (visual-settings-adapters.js), with schema validation and subscriber events.Local Testing & Validation
Run locally:
npm install && npm startValidation results:
npm run build: Static build smoke check and SEO sitemap check passed.npm run test:unit: 32/32 unit tests passed (including store and adapter tests).PLAYWRIGHT_CHANNEL=chrome npx playwright test tests/e2e/visual-settings.spec.js: 3/3 passed (dialog lifecycle, live preview updates/persistence/reset, dark mode/mobile).Known Limitations
4.0.0-beta.7.