Dev#89
Conversation
Feature/drag drop
docs: add ipc api and architecture docs
Fix/system hardening
Feature/folder search
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds export (HTML/PDF/DOCX) modules and wiring, centralized IPC/path validation and resolvers, folder search and watcher improvements, settings persistence and updater wiring, major renderer refactor into hooks/components, tests, CI security jobs, and documentation/config updates. ChangesEnd-to-end export, IPC hardening, renderer refactor, and CI/docs
Sequence Diagram(s)sequenceDiagram
participant Renderer
participant Preload
participant Main
participant Exporter
participant FS
Renderer->>Preload: invoke EXPORT_{HTML,PDF,DOCX}(html, css)
Preload->>Main: invoke IPC handler with payload
Main->>Exporter: buildDocument + sanitizeCss + inlineImages
Exporter->>Main: generated Buffer
Main->>FS: writeFile(outPath, buffer)
FS-->>Main: ok
Main-->>Preload: resolve
Preload-->>Renderer: success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/main-processor/src/index.ts (1)
61-68:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle rejection at the remaining
sendFilePathToRenderercall sites.
sendFilePathToRendereris now async andresolveMarkdownFilePathcan reject (e.g., path validation/escape failure). Theopen-filehandler (Line 76) was updated to.catch, but thedid-finish-load(Line 65) andsecond-instance(Line 92) call sites invoke it unguarded, leaving an unhandled promise rejection and a silently failing open.🛡️ Proposed fix to guard both call sites
if (filePathToOpen) { - sendFilePathToRenderer(filePathToOpen); + void sendFilePathToRenderer(filePathToOpen).catch((error) => { + console.error('Failed to open file:', error); + }); }mainWindow.focus(); - sendFilePathToRenderer(filePath); + void sendFilePathToRenderer(filePath).catch((error) => { + console.error('Failed to open file:', error); + });Also applies to: 84-96
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/main-processor/src/index.ts` around lines 61 - 68, The unguarded calls to the now-async sendFilePathToRenderer in the mainWindow.webContents.once('did-finish-load') handler and the 'second-instance' handler can produce unhandled rejections from resolveMarkdownFilePath; update both sites to handle promise rejections by either awaiting sendFilePathToRenderer inside an async callback with try/catch or appending .catch to the returned promise, and ensure pendingFilePath is cleared only after successful handling (or still cleared on error if intended) so failures are logged/handled rather than silently dropped; locate and update the callers referenced as sendFilePathToRenderer in the did-finish-load block and the second-instance handler.apps/renderer/src/hooks/useShortcuts.ts (1)
28-38:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUnreachable duplicate shortcut: focus-mode toggle is now dead.
Lines 28-32 and 34-38 guard the identical combo (
mod + Shift + F). The first branchreturns, so the second block (onToggleFocusMode) can never run — the focus-mode keyboard shortcut is silently disabled. Either drop the stale block, or assign focus mode to a distinct key if it should remain reachable.🐛 Proposed fix (remove dead block)
if (mod && e.shiftKey && e.key.toLowerCase() === 'f') { e.preventDefault(); onOpenFolderSearch(); return; } - - if (mod && e.shiftKey && e.key.toLowerCase() === 'f') { - e.preventDefault(); - onToggleFocusMode(); - return; - }If focus mode should still be triggerable, remap it to an unused combination instead of removing it. Note that
onToggleFocusModethen becomes an unused parameter and should be cleaned up accordingly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/renderer/src/hooks/useShortcuts.ts` around lines 28 - 38, The duplicate keyboard handler uses the same condition (mod + Shift + F) twice so the first branch calling onOpenFolderSearch() always returns and onToggleFocusMode() is unreachable; either remove the stale second block (dropping onToggleFocusMode from the hook signature if unused) or change its shortcut condition to a distinct key combination (e.g., mod + Shift + G) and update the condition that checks e.key and/or modifier bits (refer to the mod variable and the two handlers onOpenFolderSearch and onToggleFocusMode) so both shortcuts are uniquely reachable.
🟡 Minor comments (13)
.coderabbit.yaml-116-123 (1)
116-123:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix the shared package path glob.
'packages/shared-* /src/**/*.ts'has an extra space before/src, so these instructions will not match files underpackages/shared-*at all. That silently disables the intended contract checks for shared packages. Use'packages/shared-*/src/**/*.ts'instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.coderabbit.yaml around lines 116 - 123, The glob in .coderabbit.yaml is wrong: update the path string currently written as 'packages/shared-* /src/**/*.ts' to remove the stray space so it reads 'packages/shared-*/src/**/*.ts' so the shared package contract rules (the instructions block that checks IPC constants, menu constants, shortcuts, and shared types) actually match files under packages/shared-*; locate and correct the path value in the instructions block to ensure contract checks run.packages/shared-constants/src/menu-constants.ts-11-11 (1)
11-11:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInconsistent label casing.
'Export as Docx'differs from the sibling labels'Export as HTML'and'Export as PDF'. Use uppercase for consistency in the menu UI.- EXPORT_DOCX: 'Export as Docx', + EXPORT_DOCX: 'Export as DOCX',🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/shared-constants/src/menu-constants.ts` at line 11, The menu label for EXPORT_DOCX is using inconsistent casing; update the value of the EXPORT_DOCX constant to match siblings by using 'Export as DOCX' (same capitalization style as 'Export as HTML' and 'Export as PDF') so the menu UI is consistent; locate EXPORT_DOCX in menu-constants.ts and change its string value accordingly.apps/renderer/src/hooks/useMenuEvents.ts-47-51 (1)
47-51:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
SET_THEMEforwards any string as aTheme.The
typeof theme === 'string'guard confirms the type but not that the value is a valid theme; an unexpected string is cast withas Themeand passed downstream, bypassing type safety. Validate against the known theme set before forwarding.🛡️ Suggested guard
- window.api.onMenuEvent(MENU_EVENTS.SET_THEME, (theme) => { - if (typeof theme === 'string') { - onSetTheme(theme as Theme); - } - }); + window.api.onMenuEvent(MENU_EVENTS.SET_THEME, (theme) => { + if (typeof theme === 'string' && (THEMES as readonly string[]).includes(theme)) { + onSetTheme(theme as Theme); + } + });(Import
THEMESfrom@package/shared-constants.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/renderer/src/hooks/useMenuEvents.ts` around lines 47 - 51, The handler registered with window.api.onMenuEvent for MENU_EVENTS.SET_THEME currently casts any string to Theme and forwards it; instead validate the incoming theme value against the known THEMES set (import THEMES from `@package/shared-constants`) and only call onSetTheme(theme) when the value is a member of THEMES (otherwise ignore or log/handle invalid values); update the listener in useMenuEvents to perform this membership check before forwarding.apps/renderer/src/context/ThemeProvider.tsx-18-27 (1)
18-27:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSystem theme not applied on initial mount. This effect only reacts to subsequent
changeevents, so a user whose OS is already in dark mode (and has no storedapp-theme) still gets the defaultgithub-lightuntil they toggle their OS setting. Initialize fromgetSystemTheme()on mount when no saved theme exists.Proposed fix
useEffect(()=>{ const saved=localStorage.getItem('app-theme'); if(saved && APPTHEMES.includes(saved as Theme)){ setTheme(saved as Theme); + } else { + setThemeState(getSystemTheme()); } },[])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/renderer/src/context/ThemeProvider.tsx` around lines 18 - 27, The effect currently only handles subsequent media 'change' events and doesn't set the theme on initial mount; update the useEffect so that on mount it checks localStorage.getItem('app-theme') and if missing calls setThemeState(getSystemTheme()) before adding the media.addEventListener('change', onChange), keeping the existing onChange handler and cleanup (media.removeEventListener) unchanged; reference the existing symbols useEffect, media, onChange, setThemeState, and getSystemTheme to locate where to add the initial check.apps/main-processor/package.json-29-29 (1)
29-29:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove/justify unused direct dependency
core-util-isinapps/main-processor/package.json(line 29).There are no direct
require('core-util-is')/from "core-util-is"usages found withinapps/main-processor; if it isn’t needed via dynamic/runtime loading, drop it from direct dependencies.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/main-processor/package.json` at line 29, The package.json lists a direct dependency "core-util-is" but there are no direct imports/require usages; remove the unnecessary direct dependency by deleting "core-util-is" from apps/main-processor/package.json dependencies and run your package manager (npm/yarn/pnpm install) to update lockfile, then run tests/build to ensure nothing breaks; if the package is actually needed at runtime via dynamic require or implicit use by code not caught by static search, instead add a short justification in the PR description and retain it, or move it to devDependencies only if used for tooling.apps/main-processor/src/settings/get-settings.ts-9-18 (1)
9-18:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAny single invalid field silently resets all persisted settings.
validateSettings(parsed)throws on the first unknown key or out-of-range value, and the barecatchthen returnsDEFAULT_SETTINGS. So a settings file with one stale/unknown key (e.g. after a schema change) or one corrupted value discards every other valid preference the user had. The emptycatchalso hides genuine read/parse failures, making such issues hard to diagnose.Consider validating field-by-field and keeping the valid subset, and logging unexpected (non-ENOENT) errors.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/main-processor/src/settings/get-settings.ts` around lines 9 - 18, The current get-settings logic uses validateSettings(parsed) which throws on any unknown or invalid field causing the catch to return DEFAULT_SETTINGS and hide errors; change get-settings to parse the file then iterate fields of the parsed object, validate each field individually (using validateSettings or a new per-field validator), merge only the valid fields into DEFAULT_SETTINGS so valid preferences are preserved, and handle errors more granularly: if readFile throws ENOENT return DEFAULT_SETTINGS silently, but for other read/parse/validation errors log the error (include settingsPath and error details) before returning DEFAULT_SETTINGS so corrupt files or unexpected issues are visible.apps/renderer/src/renderer/shiki.ts-16-22 (1)
16-22:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCaching a rejected promise permanently breaks highlighting.
Storing the in-flight promise avoids duplicate initialization, but if
createHighlighterCorerejects, the rejected promise stays inhighlighterand every subsequentshikiHighlighter()call re-throws the same error with no retry. Clear the cache on failure so a transient error can recover.🔧 Proposed change
if (!highlighter) { highlighter = createHighlighterCore({ themes, langs, engine: createJavaScriptRegexEngine(), - }); + }).catch((error) => { + highlighter = null; + throw error; + }); } return highlighter;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/renderer/src/renderer/shiki.ts` around lines 16 - 22, The current pattern assigns the in-flight promise from createHighlighterCore directly to the module-level highlighter so a rejection is cached forever; change initialization in shikiHighlighter to assign the promise to a local variable first (e.g., const init = createHighlighterCore(...)), set highlighter = init, then await/init inside a try/catch, and on catch reset highlighter = undefined before rethrowing the error so transient failures can be retried; reference the highlighter variable, createHighlighterCore call, and shikiHighlighter function to locate where to apply this change.apps/preload/src/index.ts-56-60 (1)
56-60:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
onUpdateAvailablehas no unsubscribe path.Unlike
onMenuEvent, this registers anipcRenderer.onlistener and returns nothing. If a component re-subscribes (e.g., remount), listeners accumulate and the callback fires multiple times. Consider returning an unsubscribe likeonMenuEventdoes.🔧 Proposed change
- onUpdateAvailable: (callback: (version: string) => void) => { - ipcRenderer.on(IPC_CONSTANTS.UPDATE_AVAILABLE, (_event, version: string) => { - callback(version); - }); - }, + onUpdateAvailable: (callback: (version: string) => void) => { + const handler = (_event: IpcRendererEvent, version: string) => callback(version); + ipcRenderer.on(IPC_CONSTANTS.UPDATE_AVAILABLE, handler); + return () => ipcRenderer.removeListener(IPC_CONSTANTS.UPDATE_AVAILABLE, handler); + },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/preload/src/index.ts` around lines 56 - 60, The onUpdateAvailable export registers an ipcRenderer.on listener but never returns an unsubscribe, causing duplicate callbacks on re-subscribe; change onUpdateAvailable to mirror onMenuEvent by creating a named listener (e.g., const listener = (_event, version: string) => callback(version)) passed to ipcRenderer.on(IPC_CONSTANTS.UPDATE_AVAILABLE, listener) and return an unsubscribe function that calls ipcRenderer.removeListener(IPC_CONSTANTS.UPDATE_AVAILABLE, listener) so callers can detach the handler when unmounting.docs/versioned_docs/version-1.0.0/architecture.md-5-7 (1)
5-7:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove the duplicate top-level heading.
With the front-matter
title, lines 5 and 7 produce two H1s (# Architectureand# Architecture Overview). Drop the redundant one and keep a single H1 for proper document structure.📝 Proposed fix
-# Architecture - # Architecture Overview🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/versioned_docs/version-1.0.0/architecture.md` around lines 5 - 7, The document contains duplicate top-level headings ("# Architecture" and "# Architecture Overview"); remove one so there is only a single H1 to match the front-matter title. Edit the file and delete either the line "# Architecture" or "# Architecture Overview" (whichever duplicates the front-matter title) and leave the other as the sole top-level heading so document structure is correct.apps/renderer/src/utils/helpers/heading-helper.ts-29-32 (1)
29-32:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDecode HTML entities in
headingText.
parseInlineescapes characters (e.g.&→&), andstripHtmlonly removes tags, so a heading like## Tips & Tricksyields TOC textTips & Tricks(and a slug containingamp). Decode after stripping.🛠️ Proposed fix
export function headingText(token: Tokens.Heading): string { const inlineHtml = parseInline(token.text) as string; - return stripHtml(inlineHtml).trim(); + return decodeHtml(stripHtml(inlineHtml)).trim(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/renderer/src/utils/helpers/heading-helper.ts` around lines 29 - 32, The headingText function returns HTML-escaped entities because parseInline escapes chars and stripHtml only removes tags; update headingText to decode HTML entities after calling stripHtml and before trimming (i.e., call an HTML-entity decoder on the result of stripHtml(inlineHtml)), referencing the existing headingText, parseInline and stripHtml symbols; import/use a safe decoder (e.g., a small utility or a library like `he.decode` or DOMParser decoding) and ensure the decoded string is then trimmed and returned so TOC text and any derived slugs use the decoded characters.apps/renderer/src/components/DragDrop.tsx-7-7 (1)
7-7:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winBroken Tailwind classes:
bg-bg/80ntext-text-baseis a single invalid token.A separator was lost, merging
bg-bg/80andtext-text-baseinto one class that Tailwind cannot resolve. As a result the dimmed backdrop and base text color never apply, leaving only the dashed border on the overlay.🎨 Proposed fix
- <div className='pointer-events-none fixed inset-0 z-50 flex items-center justify-center border-2 border-dashed border-accent bg-bg/80ntext-text-base'> + <div className='pointer-events-none fixed inset-0 z-50 flex items-center justify-center border-2 border-dashed border-accent bg-bg/80 text-text-base'>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/renderer/src/components/DragDrop.tsx` at line 7, In DragDrop.tsx the overlay div's Tailwind classes were accidentally merged into a single invalid token (`bg-bg/80ntext-text-base`); update the className on the overlay div in the DragDrop component to split that token into two valid classes (e.g., `bg-bg/80` and `text-text-base`) so the dimmed backdrop and base text color apply correctly.apps/renderer/src/hooks/useFileActions.ts-34-40 (1)
34-40:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard
window.apiinopenFileDialogfor consistency.
openFolderbails out whenwindow.apiis undefined, butopenFileDialogdereferences it directly, which will throw aTypeErrorif the preload bridge isn't available.🛡️ Proposed fix
const openFileDialog = useCallback(() => { + if (!window.api) return; void window.api.openFileDialog().then((chosenPath) => { if (chosenPath) { void loadFileInTab(chosenPath); } }); }, [loadFileInTab]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/renderer/src/hooks/useFileActions.ts` around lines 34 - 40, openFileDialog currently dereferences window.api directly which can throw when the preload bridge is absent; update the openFileDialog callback to first check for window.api (similar to openFolder) and return early if undefined, then call window.api.openFileDialog().then(...) and proceed to call loadFileInTab(chosenPath) only when chosenPath is truthy. Ensure you reference the openFileDialog function and loadFileInTab to locate and modify the code.apps/renderer/src/hooks/useFilePersistence.ts-13-40 (1)
13-40:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClear pending timers on unmount.
debounceTimerandscrollTimerare never cleared when the component unmounts. A pending callback can fire after unmount and calldispatch/setShowToaston a stale instance. Add a cleanup effect.🧹 Proposed cleanup
const debounceTimer = useRef<number | undefined>(undefined); const scrollTimer = useRef<number | undefined>(undefined); + + useEffect(() => { + return () => { + if (debounceTimer.current) clearTimeout(debounceTimer.current); + if (scrollTimer.current) clearTimeout(scrollTimer.current); + }; + }, []);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/renderer/src/hooks/useFilePersistence.ts` around lines 13 - 40, The component never clears pending timers on unmount, so add a cleanup effect that clears any pending debounceTimer and scrollTimer to prevent handleFileChange from running after unmount: create a useEffect with an empty dependency array that on cleanup checks debounceTimer.current and scrollTimer.current and calls clearTimeout for each (and if you start using requestAnimationFrame elsewhere, store its id and cancelAnimationFrame on cleanup), then reset the refs to undefined; this ensures handleFileChange, dispatch, and setShowToast won't run on a stale component.
🧹 Nitpick comments (12)
.github/workflows/development.yml (2)
47-72: ⚖️ Poor tradeoffHarden the new
securityjob: pin actions and disable credential persistence.Static analysis flags the new steps as security-posture gaps:
- Third-party/marketplace actions are not pinned to a commit SHA (lines 48, 51, 54, 66), leaving the workflow exposed if a tag is moved to malicious code.
actions/checkoutpersists credentials by default; since this job only scans the tree, setpersist-credentials: false.🔒️ Suggested hardening
- name: Checkout - uses: actions/checkout@v4 + uses: actions/checkout@<commit-sha> # v4 + with: + persist-credentials: falseApply the same SHA-pinning to
pnpm/action-setup,actions/setup-node, andaquasecurity/trivy-action(and the bump on line 23). Keep the version as a trailing comment for readability.As per coding guidelines: "Keep dependencies updated and monitor GitHub Dependabot alerts for security vulnerabilities."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/development.yml around lines 47 - 72, The security job uses marketplace actions without SHA pins and leaves checkout credentials persisted; update the steps named "Checkout", "Setup pnpm", "Setup Node", and "Run Trivy vulnerability scanner" to reference pinned commit SHAs for their respective actions (replace tags like actions/checkout@v4, pnpm/action-setup@v4, actions/setup-node@v4, aquasecurity/trivy-action@v0.36.0 with the corresponding full commit SHAs while optionally keeping the tag as a trailing comment) and set persist-credentials: false on the "Checkout" step so the job does not retain repository credentials when only scanning the tree.
62-63: 💤 Low value
pnpm audit --audit-level=highmay block builds on unfixable transitive advisories.Since this gates the workflow, a high-severity advisory with no available patch will fail every run. Confirm this is intended, or scope it (e.g.,
--prod) to avoid noisy failures on dev-only dependencies.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/development.yml around lines 62 - 63, The workflow step named "pnpm audit" currently runs pnpm audit --audit-level=high which will fail the run on high-severity transitive advisories even if they are unfixable; change the step to either scope the audit to production deps (e.g., add --prod) or relax/remove the audit-level gating (or run audit but do not fail the job) so unfixable dev-only advisories do not block CI; update the "pnpm audit" step command accordingly to use the chosen option.apps/main-processor/src/export/getImage.ts (1)
3-12: 💤 Low valueConsider renaming for clarity.
getImagereturns a MIME type string, not an image. A name likegetImageMimeTypewould better convey intent at call sites. Logic is otherwise correct.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/main-processor/src/export/getImage.ts` around lines 3 - 12, Rename the exported function getImage to getImageMimeType to reflect that it returns a MIME type string; update the function declaration/export (getImage → getImageMimeType) and change all call sites/imports that refer to getImage to the new name, and keep the internal logic (ext detection and returned MIME strings) unchanged so behavior is preserved.docs/docs/architecture.md (1)
5-7: 💤 Low valueRedundant top-level headings. The frontmatter
title: Architecturealready renders a page title, and lines 5 and 7 add two more#headings ("Architecture" then "Architecture Overview"). Drop the duplicate H1 to avoid repetition.Proposed fix
-# Architecture - -# Architecture Overview +# Architecture Overview🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/docs/architecture.md` around lines 5 - 7, Remove the redundant top-level heading by deleting the standalone "# Architecture" heading introduced in the file (the duplicate H1) so the frontmatter title remains the page title and the remaining "Architecture Overview" heading is the sole visible section heading; ensure only one H1 is rendered and that "Architecture Overview" stays as the section header.apps/main-processor/src/folder-search.ts (1)
28-37: 💤 Low valueRedundant string lowercasing impacts performance.
Line 30 checks
lineText.toLowerCase().includes(normalizedQuery), then line 36 callslineText.toLowerCase().indexOf(normalizedQuery)again. For large files, this doubles the lowercasing work for every matching line.♻️ Cache the lowercased line text
for (let index = 0; index < lines.length; index += 1) { const lineText = lines[index] ?? ''; - if (!lineText.toLowerCase().includes(normalizedQuery)) continue; + const lowerLine = lineText.toLowerCase(); + if (!lowerLine.includes(normalizedQuery)) continue; results.push({ filePath, fileName: basename(filePath), line: index + 1, - column: lineText.toLowerCase().indexOf(normalizedQuery) + 1, + column: lowerLine.indexOf(normalizedQuery) + 1, preview: lineText.trim().slice(0, 240), });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/main-processor/src/folder-search.ts` around lines 28 - 37, The loop currently calls lineText.toLowerCase() twice per matching line; to fix this, compute a cached lowercased version once (e.g., const lowerLine = lineText.toLowerCase()) immediately after const lineText, then use lowerLine.includes(normalizedQuery) and lowerLine.indexOf(normalizedQuery) when building the result (keep preview using the original lineText to preserve casing). Update references in the block that pushes into results (column calculation and include check) to use the cached lowercased variable.apps/main-processor/__tests__/folder-search.test.ts (1)
26-29: ⚡ Quick winTest logic flaw: assertion passes vacuously.
The query "not a markdown" only exists in
notes.txt, sosearchFoldershould return an empty array (since .txt files are excluded). The assertion.every((r) => r.filePath.endsWith('.md'))passes vacuously when results is empty, but doesn't actually verify that .txt files were excluded—it would also pass ifsearchFolderreturned no results for any other reason.✅ Strengthen the test assertion
it('does not search non .md files', async () => { - const results = await searchFolder(DIR, 'not a markdown'); - expect(results.every((r) => r.filePath.endsWith('.md'))).toBe(true); + const results = await searchFolder(DIR, 'not a markdown'); + expect(results).toHaveLength(0); + // Also verify that .md files ARE searched + const mdResults = await searchFolder(DIR, 'Version'); + expect(mdResults.length).toBeGreaterThan(0); + expect(mdResults.every((r) => r.filePath.endsWith('.md'))).toBe(true); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/main-processor/__tests__/folder-search.test.ts` around lines 26 - 29, The test currently vacuously passes because results is empty; update the assertion in the test for searchFolder to explicitly assert that no matches are returned for a query present only in a .txt file by checking results.length === 0 (or expect(results).toEqual([])), rather than using .every(...). Reference the test in apps/main-processor/__tests__/folder-search.test.ts and the searchFolder call to ensure the query "not a markdown" yields an empty array, proving .txt files are excluded.apps/main-processor/__tests__/docx.test.ts (1)
18-32: ⚡ Quick winTest order dependency creates fragile coupling.
The second test (lines 28-32) depends on the first test (lines 18-26) having already created the DOCX file. This implicit ordering makes the suite brittle; running tests in isolation or in different order will cause failures.
♻️ Refactor to remove test order dependency
it('it should create a DOCX file at the specified output path', async () => { await exportDOCX( '<h1>Test DOCX</h1><p>Hello DOCX export.</p>', 'body { color: black; }', docxPath ); expect(existsSync(docxPath)).toBe(true); + const stats = statSync(docxPath); + expect(stats.size).toBeGreaterThan(100); }); - it('it should create a non-empty DOCX file', () => { - const stats = statSync(docxPath); - - expect(stats.size).toBeGreaterThan(100); - });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/main-processor/__tests__/docx.test.ts` around lines 18 - 32, The second test relies on the DOCX being created by the first test; remove that ordering dependency by ensuring the DOCX is created as part of the test setup: call exportDOCX(...) (using the same arguments and docxPath) inside a beforeEach or beforeAll hook, or call exportDOCX at the start of the second test before statSync; reference the exportDOCX function and the docxPath variable and use existsSync/statSync only after exportDOCX completes to guarantee the file exists for the size assertion.apps/renderer/__tests__/renderer/markdown.test.ts (2)
52-57: ⚡ Quick winClarify test description.
The test name uses a double negative ("should not be able to remove") which makes the intent unclear. Rephrase to clearly state that dangerous handlers are preserved at this stage:
- it('it should not be able to remove dangerous inline event handlers before dom purification', async () => { + it('should preserve inline event handlers before DOM purification', async () => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/renderer/__tests__/renderer/markdown.test.ts` around lines 52 - 57, Rename the confusing test description string in the it(...) call to a clear positive statement—e.g., change "it should not be able to remove dangerous inline event handlers before dom purification" to "should preserve dangerous inline event handlers before DOM purification"—so the test intent is unambiguous; update the it(...) description where renderMarkdown is invoked in the test case.
47-50: ⚡ Quick winClarify test description.
The test name "should not be able to remove iframe tags before sanitization" uses a double negative and is confusing. Consider renaming to describe what the test actually verifies:
- it('should not be able to remove iframe tags before sanitization', async () => { + it('should preserve iframe tags before sanitization', async () => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/renderer/__tests__/renderer/markdown.test.ts` around lines 47 - 50, Rename the confusing test case 'it("should not be able to remove iframe tags before sanitization", ...)' to a clear positive description such as 'it("should retain iframe tags in rendered HTML before sanitization", ...)' and update any related comments/messages to match; the assertion using renderMarkdown and expect(html).toContain('<iframe') should remain unchanged so the test behavior (calling renderMarkdown and checking for '<iframe') is clear and matches the new name.apps/renderer/src/App.tsx (1)
39-41: 💤 Low valueMinor inconsistency in TOC fallback source.
activeToc(Line 39, fed touseTocfor active-heading tracking) falls back to theuseFiletoc, whileSidebar(Line 199) falls back toextractTOC(activeTab.html). WhenactiveTab.tocis absent these two diverge, so the highlighted/scroll-target headings fromuseTocmay not match the items actually rendered in the sidebar. Consider sourcing both from the same value.Also applies to: 199-199
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/renderer/src/App.tsx` around lines 39 - 41, activeToc currently uses activeTab?.toc ?? toc while Sidebar uses extractTOC(activeTab.html), causing diverging TOC sources; make both consumers use the same canonical TOC value. Fix by computing a single tocSource (e.g., const resolvedToc = activeTab?.toc ?? extractTOC(activeTab?.html ?? '') or a helper getToc(activeTab, toc)) and pass resolvedToc into useToc (replace activeToc) and into Sidebar so highlight/scrolling (useToc, activeToc) always match the rendered items (Sidebar, extractTOC).apps/renderer/src/hooks/useExport.ts (1)
6-31: ⚡ Quick winWrap export IPC calls in error handling.
window.api.exportHTML/PDF/DOCXare external (main-process) calls that can reject (write failures, permission errors, etc.). Currently a rejection surfaces as an unhandled promise rejection with no user-visible feedback. Consider try/catch around each export with a toast/error notification.♻️ Example for exportHtml
const exportHtml = useCallback(async () => { if (!activeTab) return; const outPath = await window.api.showSaveDialog({ defaultExt: 'html' }); if (!outPath) return; - const css = exportCss; - await window.api.exportHTML(activeTab.html, css, outPath); + try { + await window.api.exportHTML(activeTab.html, exportCss, outPath); + } catch (err) { + // surface failure to the user (toast/log) + } }, [activeTab]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/renderer/src/hooks/useExport.ts` around lines 6 - 31, Wrap each export function (exportHtml, exportPdf, exportDocx) body in try/catch to handle rejections from window.api.exportHTML/exportPDF/exportDOCX; on error, log or surface the error to the user (e.g., call your toast/error notification helper or window.api.showError) including the error message and context (which export and outPath) and ensure the hook still returns/cleans up gracefully. Keep the existing early returns for missing activeTab/outPath and only wrap the async IPC call(s) and related logic inside the try block so any thrown exceptions are caught and reported.apps/renderer/src/components/SettingsPanel.tsx (1)
16-44: ⚡ Quick winModal lacks a focus trap.
The dialog sets
aria-modal="true"and moves focus on open, but nothing keepsTab/Shift+Tabfocus inside the panel. Keyboard users can tab into the obscured background content, which breaks the modal contract.Consider trapping focus within the dialog (cycle focus between the first/last focusable elements) while
isOpen.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/renderer/src/components/SettingsPanel.tsx` around lines 16 - 44, The modal currently moves focus to dialogRef when isOpen but doesn’t trap Tab navigation; add a focus-trap inside the existing effect that runs when isOpen (or create a new useEffect keyed on [isOpen, dialogRef, onClose]) that: queries focusable elements within dialogRef (first and last), listens for keydown and when e.key === 'Tab' prevents default and cycles focus to first when Shift not pressed and to last when Shift pressed (or vice versa), ensures the existing Escape handler and previouslyFocused restore logic are preserved, and cleans up the keydown listener on unmount/close; reference dialogRef, isOpen, and the existing onClose handler when implementing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2659b106-3dbd-4732-919b-c535c625267c
⛔ Files ignored due to path filters (5)
apps/renderer/public/icons/app-icon.svgis excluded by!**/*.svgdocs/static/img/docusaurus-social-card.jpgis excluded by!**/*.jpgdocs/static/img/markdown-social-card.pngis excluded by!**/*.pngdocs/static/video/hero.mp4is excluded by!**/*.mp4pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (128)
.coderabbit.yaml.github/workflows/development.yml.github/workflows/production.yml.gitignore.husky/pre-push.prettierignoreREADME.mdapps/main-processor/__tests__/cli.test.tsapps/main-processor/__tests__/docx.test.tsapps/main-processor/__tests__/export.test.tsapps/main-processor/__tests__/file.test.tsapps/main-processor/__tests__/folder-search.test.tsapps/main-processor/__tests__/folder.test.tsapps/main-processor/__tests__/ipc.test.tsapps/main-processor/__tests__/menu.test.tsapps/main-processor/__tests__/recent.test.tsapps/main-processor/__tests__/settings.test.tsapps/main-processor/package.jsonapps/main-processor/setup.tsapps/main-processor/src/cli.tsapps/main-processor/src/export/buildDocument.tsapps/main-processor/src/export/exportDocx.tsapps/main-processor/src/export/exportHtml.tsapps/main-processor/src/export/exportPdf.tsapps/main-processor/src/export/getImage.tsapps/main-processor/src/export/inlineImage.tsapps/main-processor/src/export/sanitizeCss.tsapps/main-processor/src/file.tsapps/main-processor/src/folder-search.tsapps/main-processor/src/folder.tsapps/main-processor/src/index.tsapps/main-processor/src/ipc.tsapps/main-processor/src/menu.tsapps/main-processor/src/recent/getUniqueRecentFile.tsapps/main-processor/src/settings/get-settings.tsapps/main-processor/src/settings/save-settings.tsapps/main-processor/src/types/watch-file-types.tsapps/main-processor/src/updater.tsapps/main-processor/src/utils/constants/export-constants.tsapps/main-processor/src/utils/constants/folder-constants.tsapps/main-processor/src/utils/constants/ipc-validation.tsapps/main-processor/src/utils/constants/path-constants.tsapps/main-processor/src/utils/constants/setting-constants.tsapps/main-processor/src/utils/constants/window-constants.tsapps/main-processor/src/utils/helper/folder-search-helper.tsapps/main-processor/src/utils/helper/html-template.tsapps/main-processor/src/utils/helper/ipc-path-resolver.tsapps/main-processor/src/utils/helper/menu-helper.tsapps/main-processor/src/utils/helper/path-helper.tsapps/main-processor/src/utils/helper/setting-helper.tsapps/main-processor/src/utils/ipc-validation.tsapps/main-processor/tests/ipc.test.tsapps/main-processor/vitest.config.tsapps/preload/src/index.tsapps/preload/src/utils/menu-event-helper.tsapps/renderer/__tests__/components/Sidebar.test.tsxapps/renderer/__tests__/components/StatusBar.test.tsxapps/renderer/__tests__/components/TabBar.test.tsxapps/renderer/__tests__/components/Welcome.test.tsxapps/renderer/__tests__/hooks/useCollapsibleToc.test.tsapps/renderer/__tests__/hooks/useSearch.test.tsapps/renderer/__tests__/hooks/useSettings.test.tsapps/renderer/__tests__/renderer/callout.test.tsapps/renderer/__tests__/renderer/drag-drop.test.tsapps/renderer/__tests__/renderer/katex.test.tsapps/renderer/__tests__/renderer/markdown.test.tsapps/renderer/__tests__/renderer/mermaid.test.tsapps/renderer/__tests__/renderer/sanitize.test.tsapps/renderer/__tests__/renderer/sikhi.test.tsapps/renderer/__tests__/renderer/toc.test.tsapps/renderer/__tests__/store/tabStore.test.tsapps/renderer/index.htmlapps/renderer/src/App.tsxapps/renderer/src/components/DragDrop.tsxapps/renderer/src/components/ReaderToolbar.tsxapps/renderer/src/components/SearchBar.tsxapps/renderer/src/components/SettingsPanel.tsxapps/renderer/src/components/Sidebar.tsxapps/renderer/src/components/UpdateBanner.tsxapps/renderer/src/config/marked.tsapps/renderer/src/context/ThemeProvider.tsxapps/renderer/src/hooks/useCollapsibleToc.tsapps/renderer/src/hooks/useDragDrop.tsapps/renderer/src/hooks/useExport.tsapps/renderer/src/hooks/useFile.tsapps/renderer/src/hooks/useFileActions.tsapps/renderer/src/hooks/useFilePersistence.tsapps/renderer/src/hooks/useFolderSearch.tsapps/renderer/src/hooks/useLayout.tsapps/renderer/src/hooks/useMenuEvents.tsapps/renderer/src/hooks/useOpenFilePath.tsapps/renderer/src/hooks/useSearch.tsapps/renderer/src/hooks/useSettings.tsapps/renderer/src/hooks/useShortcuts.tsapps/renderer/src/hooks/useTabNavigation.tsapps/renderer/src/renderer/drag-drop.tsapps/renderer/src/renderer/shiki.tsapps/renderer/src/renderer/toc.tsapps/renderer/src/store/tabStore.tsapps/renderer/src/styles/export.cssapps/renderer/src/types/component-types.tsapps/renderer/src/types/hook-types.tsapps/renderer/src/utils/helpers/heading-helper.tsapps/renderer/src/utils/helpers/sidebar-helper.tsapps/renderer/src/utils/helpers/tab-helper.tsxapps/renderer/src/utils/helpers/theme-helper.tsassets/unsigned-install-notice.txtdocs/docs/architecture.mddocs/docs/ipc-api.mddocs/docusaurus.config.tsdocs/sidebars.tsdocs/src/components/Homepage/hero.tsxdocs/versioned_docs/version-1.0.0/architecture.mddocs/versioned_docs/version-1.0.0/ipc-api.mddocs/versioned_sidebars/version-1.0.0-sidebars.jsonelectron-builder.tspackage.jsonpackages/shared-constants/src/index.tspackages/shared-constants/src/ipc-constants.tspackages/shared-constants/src/menu-constants.tspackages/shared-constants/src/path-constants.tspackages/shared-constants/src/theme-constants.tspackages/shared-types/package.jsonpackages/shared-types/src/index.tspackages/shared-types/src/markdown-type.tspackages/shared-types/src/search-type.tspackages/shared-types/src/settings-default.tspackages/shared-types/src/settings-type.ts
💤 Files with no reviewable changes (2)
- apps/main-processor/tests/ipc.test.ts
- apps/main-processor/src/utils/ipc-validation.ts
| uses: pnpm/action-setup@v3 | ||
| with: | ||
| version: 10 | ||
| uses: pnpm/action-setup@v4 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP 'uses:\s+\S+@(?![0-9a-fA-F]{40}\b)\S+' .github/workflows/production.ymlRepository: mindfiredigital/markdown-reader
Length of output: 354
Pin GitHub Actions uses: refs to commit SHAs in .github/workflows/production.yml.
uses: actions/checkout@v4(15, 48),uses: actions/setup-node@v4(18, 54),uses: pnpm/action-setup@v4(23, 51), anduses: aquasecurity/trivy-action@v0.36.0(66) are on mutable tags; pin each to the exact 40-hex commit SHA for reproducible, supply-chain-safe runs.
🧰 Tools
🪛 zizmor (1.25.2)
[error] 23-23: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/production.yml at line 23, The workflow is using mutable
action tags (e.g., uses: pnpm/action-setup@v4, uses: actions/checkout@v4, uses:
actions/setup-node@v4, uses: aquasecurity/trivy-action@v0.36.0); replace each
mutable tag with the corresponding immutable 40-character commit SHA for the
action (lookup the action repo on GitHub, copy the full commit SHA for the
tag/version, and substitute the tag with @<40-hex-sha>) so that entries like the
lines containing "uses: pnpm/action-setup@v4", "uses: actions/checkout@v4",
"uses: actions/setup-node@v4", and "uses: aquasecurity/trivy-action@v0.36.0" are
pinned to exact commit SHAs.
| - name: Checkout | ||
| uses: actions/checkout@v4 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant section around the cited lines
echo "=== production.yml lines 35-80 ==="
nl -ba .github/workflows/production.yml | sed -n '35,80p'
echo
echo "=== All checkout steps in production.yml (with context) ==="
rg -n --context 2 "uses:\s*actions/checkout@" .github/workflows/production.yml
echo
echo "=== Any persist-credentials occurrences in production.yml ==="
rg -n "persist-credentials" .github/workflows/production.yml || trueRepository: mindfiredigital/markdown-reader
Length of output: 171
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== production.yml lines 35-80 (cat -n) ==="
cat -n .github/workflows/production.yml | sed -n '35,80p'
echo
echo "=== All checkout steps in production.yml (with context) ==="
rg -n --context 2 "uses:\s*actions/checkout@" .github/workflows/production.yml
echo
echo "=== Any persist-credentials occurrences in production.yml ==="
rg -n "persist-credentials" .github/workflows/production.yml || trueRepository: mindfiredigital/markdown-reader
Length of output: 1573
Disable persisted checkout credentials in the Security Audit job.
The “Security Audit” job checks out the repo and runs scanners; its actions/checkout@v4 step doesn’t set persist-credentials: false, so the token can be written to .git/config for no benefit. Add it to that checkout step.
Suggested change
- name: Checkout
uses: actions/checkout@v4
+ with:
+ persist-credentials: false📝 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.
| - name: Checkout | |
| uses: actions/checkout@v4 | |
| - name: Checkout | |
| uses: actions/checkout@v4 | |
| with: | |
| persist-credentials: false |
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 47-48: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 48-48: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/production.yml around lines 47 - 48, The Security Audit
job's checkout step uses actions/checkout@v4 without disabling persisted
credentials; update the checkout step in the "Security Audit" job (the
actions/checkout@v4 invocation) to include persist-credentials: false so the
runner's token is not written to .git/config during that job.
| const html = buildDocument(bodyHtml, sanitizeCss(css)); | ||
|
|
||
| const result = await HTMLtoDOCX(html, null, { | ||
| table: { row: { cantSplit: true } }, | ||
| footer: true, | ||
| pageNumber: true, | ||
| }); |
There was a problem hiding this comment.
Inline images before DOCX conversion.
This path skips inlineImages, so DOCX exports will miss local images while the HTML and PDF exporters embed them.
Suggested fix
import HTMLtoDOCX from 'html-to-docx';
import { writeFile } from 'node:fs/promises';
import { buildDocument } from './buildDocument';
+import { inlineImages } from './inlineImage';
import { sanitizeCss } from './sanitizeCss';
export async function exportDOCX(bodyHtml: string, css: string, outputPath: string): Promise<void> {
- const html = buildDocument(bodyHtml, sanitizeCss(css));
+ const htmlWithInlineImages = await inlineImages(bodyHtml);
+ const html = buildDocument(htmlWithInlineImages, sanitizeCss(css));📝 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.
| const html = buildDocument(bodyHtml, sanitizeCss(css)); | |
| const result = await HTMLtoDOCX(html, null, { | |
| table: { row: { cantSplit: true } }, | |
| footer: true, | |
| pageNumber: true, | |
| }); | |
| import HTMLtoDOCX from 'html-to-docx'; | |
| import { writeFile } from 'node:fs/promises'; | |
| import { buildDocument } from './buildDocument'; | |
| import { inlineImages } from './inlineImage'; | |
| import { sanitizeCss } from './sanitizeCss'; | |
| export async function exportDOCX(bodyHtml: string, css: string, outputPath: string): Promise<void> { | |
| const htmlWithInlineImages = await inlineImages(bodyHtml); | |
| const html = buildDocument(htmlWithInlineImages, sanitizeCss(css)); | |
| const result = await HTMLtoDOCX(html, null, { | |
| table: { row: { cantSplit: true } }, | |
| footer: true, | |
| pageNumber: true, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/main-processor/src/export/exportDocx.ts` around lines 7 - 13, The DOCX
export currently passes HTML from buildDocument(sanitizeCss(css)) straight into
HTMLtoDOCX, skipping image embedding; call the existing inlineImages helper to
process the HTML (resolving and embedding local images) before passing it into
HTMLtoDOCX so DOCX exports include local images—i.e., process the HTML produced
by buildDocument (using bodyHtml and sanitizeCss(css)) through inlineImages,
then await HTMLtoDOCX on that inlined HTML.
| const pdfWindow = new BrowserWindow({ | ||
| show: false, | ||
| webPreferences: { | ||
| sandbox: false, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the target file
FILE="apps/main-processor/src/export/exportPdf.ts"
if [ ! -f "$FILE" ]; then
echo "Missing: $FILE" >&2
exit 1
fi
echo "=== File: $FILE (context around BrowserWindow) ==="
# Print lines 1-120 with numbers (should be manageable)
nl -ba "$FILE" | sed -n '1,160p'
echo
echo "=== Search for BrowserWindow usage in this file ==="
rg -n "new BrowserWindow|webPreferences" "$FILE" || true
echo
echo "=== Search for sandbox/webPreferences related helpers across main-processor ==="
rg -n "sandbox\s*:\s*(false|true)" apps/main-processor/src/export apps/main-processor/src || true
echo
echo "=== Search for how pdfWindow loads content (data URL / HTML) ==="
rg -n "loadURL|loadFile|data:|bodyHtml" "$FILE" || trueRepository: mindfiredigital/markdown-reader
Length of output: 226
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="apps/main-processor/src/export/exportPdf.ts"
if [ ! -f "$FILE" ]; then
echo "Missing: $FILE" >&2
exit 1
fi
echo "=== File: $FILE (lines 1-200) ==="
cat -n "$FILE" | sed -n '1,220p'
echo
echo "=== Search for BrowserWindow usage in this file ==="
rg -n "new BrowserWindow|webPreferences" "$FILE" || true
echo
echo "=== Search for sandbox/webPreferences related settings in main-processor ==="
rg -n "sandbox\s*:\s*(false|true)" apps/main-processor/src || true
echo
echo "=== Search for how pdfWindow loads content (data URL / HTML) ==="
rg -n "loadURL|loadFile|data:|bodyHtml|setContent" "$FILE" || trueRepository: mindfiredigital/markdown-reader
Length of output: 2318
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "webPreferences\s*:\s*\{|nodeIntegration|contextIsolation|enableRemoteModule|sandbox\s*:" apps/main-processor/src/export apps/main-processor/src || true
echo
echo "=== BrowserWindow configs under apps/main-processor/src/export ==="
rg -n "new BrowserWindow\s*\(" apps/main-processor/src/export || true
echo
echo "=== show full webPreferences object for exportPdf.ts ==="
awk 'NR>=1 && NR<=80 {print}' apps/main-processor/src/export/exportPdf.tsRepository: mindfiredigital/markdown-reader
Length of output: 2009
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="apps/main-processor/src/index.ts"
if [ -f "$FILE" ]; then
echo "=== File: $FILE (lines 1-140) ==="
cat -n "$FILE" | sed -n '1,180p'
else
echo "Missing: $FILE" >&2
exit 1
fi
echo
echo "=== Search for createPreload / preload usage in main-processor ==="
rg -n "preload\s*:|contextIsolation\s*:|nodeIntegration\s*:|sandbox\s*:" apps/main-processor/src || true
echo
echo "=== Search for electron sandbox requirements / uses in codebase ==="
rg -n "sandbox" apps/main-processor/src || trueRepository: mindfiredigital/markdown-reader
Length of output: 4773
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Inspect buildDocument.ts ==="
FILE="apps/main-processor/src/export/buildDocument.ts"
if [ -f "$FILE" ]; then cat -n "$FILE" | sed -n '1,220p'; else echo "Missing $FILE"; fi
echo
echo "=== Inspect sanitizeCss.ts ==="
FILE="apps/main-processor/src/export/sanitizeCss.ts"
if [ -f "$FILE" ]; then cat -n "$FILE" | sed -n '1,220p'; else echo "Missing $FILE"; fi
echo
echo "=== Inspect inlineImage.ts (inlineImages) ==="
FILE="apps/main-processor/src/export/inlineImage.ts"
if [ -f "$FILE" ]; then cat -n "$FILE" | sed -n '1,260p'; else echo "Missing $FILE"; fi
echo
echo "=== Search for sanitization/DOMPurify/script stripping in export pipeline ==="
rg -n "DOMPurify|sanitize|script|onerror|onload|<script|Content-Security-Policy|csp" apps/main-processor/src/export || trueRepository: mindfiredigital/markdown-reader
Length of output: 2920
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Search for other BrowserWindow usage in export folder ==="
rg -n "new BrowserWindow\s*\(" apps/main-processor/src/export || true
echo
echo "=== Inspect exportHtml.ts ==="
FILE="apps/main-processor/src/export/exportHtml.ts"
if [ -f "$FILE" ]; then cat -n "$FILE" | sed -n '1,200p'; else echo "Missing $FILE"; fi
echo
echo "=== Inspect exportDocx.ts ==="
FILE="apps/main-processor/src/export/exportDocx.ts"
if [ -f "$FILE" ]; then cat -n "$FILE" | sed -n '1,240p'; else echo "Missing $FILE"; fiRepository: mindfiredigital/markdown-reader
Length of output: 1957
🏁 Script executed:
rg -n "exportPDF\s*\(" apps/main-processor/src apps/main-processor || true
rg -n "exportPDF\s*:" apps/main-processor/src || trueRepository: mindfiredigital/markdown-reader
Length of output: 523
🌐 Web query:
Electron BrowserWindow webPreferences sandbox option default requirements nodeIntegration contextIsolation executeJavaScript printToPDF compatibility
💡 Result:
Electron BrowserWindow webPreferences sandbox option default requirements nodeIntegration contextIsolation executeJavaScript printToPDF compatibility 1) sandbox webPreferences option (BrowserWindow → webPreferences) - Type: boolean. - Default: true “since Electron 20.” [1] - Meaning: enables renderer sandboxing compatible with the Chromium OS-level sandbox; it also disables the Node.js engine in the renderer. [1] - Relationship to nodeIntegration: “The sandbox will automatically be disabled when nodeIntegration is set to true.” [1] Also, enabling Node.js integration disables sandboxing. [2] 2) nodeIntegration requirements (and its interaction with sandbox) - nodeIntegration controls whether Node.js integration is enabled. Its default is false. [1] - If you set nodeIntegration: true for a renderer, sandboxing is disabled for that renderer (sandbox=false implied). [1][2] 3) contextIsolation requirements and defaults - contextIsolation controls whether Electron APIs and the preload script run in a separate JavaScript context from the loaded page. [1] - Default: true. [1] - Security note: Electron documentation recommends context isolation to prevent the loaded page from accessing Electron internals/APIs exposed by preload. [3] 4) executeJavaScript compatibility with sandbox + contextIsolation - Electron’s webContents.executeJavaScript runs JavaScript in the page context (after the page can execute JS) and returns a Promise that resolves to the result. [4] - Context isolation affects what preload scripts can see vs what the web page can see; Electron APIs exposed via preload are meant to be accessed through a safe bridge. [3][5] - For compatibility specifically with isolated contexts: Electron also provides webContents.executeJavaScriptInIsolatedWorld, which lets you run code in a chosen isolated “worldId” (Electron’s contextIsolation world is 999). [6] Recommendation: if you need to interact with the DOM/Electron-exposed globals that live in the preload-isolated world, prefer executeJavaScriptInIsolatedWorld with the appropriate worldId; otherwise executeJavaScript targets the main page context. [6] 5) printToPDF compatibility with sandbox (and common edge cases) - webContents.printToPDF exists as an API on the WebContents for printing the current page to PDF. [4] - Sandboxed rendering: Electron release notes state “Printing now works when the sandbox is enabled.” [7] So, sandbox=true (Electron 20+) should not inherently break printToPDF. [7] - Cross-origin iframe issues (rendering correctness / resolution problems): there are known historical bugs where cross-origin iframe content can be blank in the PDF, and in older versions printToPDF might never resolve. [8] More recent work includes a fix for printToPDF with cross-process subframes / iframes. [9] Practical takeaways - If your goal is security defaults: omit sandbox/nodeIntegration and rely on Electron defaults: nodeIntegration defaults to false and sandbox defaults to true (Electron 20+). [1] - Keep contextIsolation enabled (default true) and use contextBridge for exposing safe APIs to the page. [1][3] - For executeJavaScript: use executeJavaScript for page context; use executeJavaScriptInIsolatedWorld if you must target the contextIsolation/preload isolated world (worldId 999 for Electron’s isolation). [6] - For printToPDF: sandbox=true should be compatible (printing works with sandbox enabled), but still test pages with cross-origin iframes because rendering behavior has had version-specific issues and fixes. [7][8][9]
Citations:
- 1: https://github.com/electron/electron/blob/main/docs/api/structures/web-preferences.md
- 2: https://github.com/electron/electron/blob/main/docs/tutorial/sandbox.md
- 3: https://electronjs.org/docs/latest/tutorial/context-isolation
- 4: https://github.com/electron/electron/blob/da3dbdcb9bb98471cede64ddd311dc8b4c378e62/docs/api/web-contents.md
- 5: https://github.com/electron/electron/blob/main/docs/api/context-bridge.md
- 6: https://github.com/electron/electron/blob/11-x-y/docs/api/web-contents.md
- 7: https://zeke.github.io/electron.atom.io/releases/
- 8: [Bug]: printToPDF has blank iframes from different origin electron/electron#36588
- 9: fix:
webContents.printToPDF()with cross-process subframes electron/electron#46218
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Inspect ipc.ts around exportPDF call (lines ~150-220) ==="
FILE="apps/main-processor/src/ipc.ts"
if [ -f "$FILE" ]; then cat -n "$FILE" | sed -n '140,240p'; else echo "Missing $FILE"; fi
echo
echo "=== Find htmlTemplate implementation ==="
rg -n "htmlTemplate" apps/main-processor/src/utils/helper apps/main-processor/src || true
echo
echo "=== Inspect htmlTemplate file (show key parts) ==="
# Try common location based on buildDocument.ts import: ../utils/helper/html-template
HFILE="apps/main-processor/src/utils/helper/html-template.ts"
if [ -f "$HFILE" ]; then cat -n "$HFILE" | sed -n '1,220p'; else echo "Missing $HFILE"; fiRepository: mindfiredigital/markdown-reader
Length of output: 3919
Security: keep Electron renderer sandbox enabled for PDF export
apps/main-processor/src/export/exportPdf.ts creates a hidden BrowserWindow and explicitly sets webPreferences.sandbox: false. This window loads a data:text/html document where bodyHtml is injected directly into the template (${htmlBody}) with only CSS sanitization—so untrusted markup is rendered. Disabling the Chromium renderer sandbox weakens isolation on this sensitive path without a clear need.
Suggested fix
const pdfWindow = new BrowserWindow({
show: false,
webPreferences: {
- sandbox: false,
+ sandbox: true,
},
});📝 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.
| const pdfWindow = new BrowserWindow({ | |
| show: false, | |
| webPreferences: { | |
| sandbox: false, | |
| }, | |
| }); | |
| const pdfWindow = new BrowserWindow({ | |
| show: false, | |
| webPreferences: { | |
| sandbox: true, | |
| }, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/main-processor/src/export/exportPdf.ts` around lines 8 - 13, The code
creates pdfWindow via new BrowserWindow in exportPdf.ts with
webPreferences.sandbox set to false; re-enable the Chromium renderer sandbox by
setting webPreferences.sandbox: true (and also enforce renderer isolation by
setting contextIsolation: true and nodeIntegration: false) on the pdfWindow
creation, and avoid directly injecting untrusted HTML into a data:text/html
string — instead load sanitized content via a safer channel (e.g., loadURL with
an encoded/escaped data URL or load a temporary local file) or pass the HTML to
a preload script through IPC so the renderer never gets unsafe globals; update
the BrowserWindow creation (pdfWindow, webPreferences) accordingly.
| const sanitized = EXPORT_CONST.DANGEROUS_CSS_PATTERNS.reduce( | ||
| (safeCss, pattern) => safeCss.replace(pattern, ''), | ||
| css | ||
| ); | ||
| return sanitized; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -t f 'export-constants.ts' --full-path 'apps/main-processor' --exec cat {}
echo "----"
rg -nP 'DANGEROUS_CSS_PATTERNS' -C2Repository: mindfiredigital/markdown-reader
Length of output: 1311
Make sanitizeCss fixed-point to prevent reconstitution of dangerous constructs.
DANGEROUS_CSS_PATTERNS entries are all declared with /gi (global), so replace(pattern, '') won’t “miss” additional matches for the same pattern. The remaining issue is that sanitizeCss applies the patterns only once in order; removing a later fragment (e.g., javascript:) can concatenate surrounding text into an earlier dangerous construct (e.g., expression(), which won’t be re-checked in the same call. Use an until-stable loop (or a vetted CSS sanitizer).
🛡️ Fixed-point sanitization
-export function sanitizeCss(css: string): string {
- if (!css || typeof css !== 'string') return '';
- const sanitized = EXPORT_CONST.DANGEROUS_CSS_PATTERNS.reduce(
- (safeCss, pattern) => safeCss.replace(pattern, ''),
- css
- );
- return sanitized;
-}
+export function sanitizeCss(css: string): string {
+ if (!css || typeof css !== 'string') return '';
+ let sanitized = css;
+ let previous: string;
+ do {
+ previous = sanitized;
+ sanitized = EXPORT_CONST.DANGEROUS_CSS_PATTERNS.reduce(
+ (safeCss, pattern) => safeCss.replace(pattern, ''),
+ sanitized
+ );
+ } while (sanitized !== previous);
+ return sanitized;
+}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/main-processor/src/export/sanitizeCss.ts` around lines 5 - 9,
sanitizeCss currently applies EXPORT_CONST.DANGEROUS_CSS_PATTERNS once in order,
which can leave reconstituted dangerous constructs; change sanitizeCss to be
fixed-point by repeatedly applying the replace pass until the CSS string stops
changing (e.g., loop applying EXPORT_CONST.DANGEROUS_CSS_PATTERNS.reduce over
the current css and break when result === previous) so any newly formed
dangerous fragments (like expression( or javascript:) are removed; alternatively
call a vetted CSS sanitizer instead of the single-pass reduce.
| try { | ||
| const next = await window.api.saveSettings(partial); | ||
| setSettings(next); | ||
| } catch { | ||
| setSettings((current) => ({ ...current, ...partial })); | ||
| } |
There was a problem hiding this comment.
Don't merge settings into local state after a failed save.
If saveSettings rejects, this still applies partial locally, so the UI looks saved even though disk state was not updated. That creates a misleading success path and guarantees settings will "revert" on the next restart. Keep the previous state on failure and surface the error instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/renderer/src/hooks/useSettings.ts` around lines 67 - 72, The catch block
in useSettings.ts currently merges `partial` into local state even when
`window.api.saveSettings(partial)` fails, which misleads the UI; change the
catch to leave state unchanged (do not call `setSettings` with merged `partial`)
and instead surface or rethrow the error (e.g., return or propagate the caught
error from the `saveSettings` call) so callers can display an error;
specifically modify the try/catch around `window.api.saveSettings(partial)` /
`setSettings(next)` to avoid applying `partial` on failure and ensure the error
is reported to the caller or UI.
| export function heading({ text, depth }: HeadingProps) { | ||
| const id = getHeadingId(text); | ||
| return `<h${depth} id="${id}">${text}</h${depth}>\n`; | ||
| const plainText = stripHtml(text); | ||
| const id = getHeadingId(plainText); | ||
| const safeText = escapeHtml(plainText); | ||
| return `<h${depth} id="${id}">${safeText}</h${depth}>\n`; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect how `heading` is registered with marked and the HeadingProps shape + marked version
fd -t f 'marked.ts' apps/renderer/src/config --exec cat {}
echo '--- HeadingProps ---'
rg -nP --type=ts -C3 '\bHeadingProps\b'
echo '--- marked version ---'
fd -t f 'package.json' apps/renderer --exec sh -c 'jq -r ".dependencies.marked // .devDependencies.marked" "$1"' _ {}Repository: mindfiredigital/markdown-reader
Length of output: 2961
🏁 Script executed:
#!/bin/bash
# Inspect heading-helper implementation and any usage of parseInline/headingText
fd -t f 'heading-helper.ts' . --exec sh -c 'echo "--- $1 ---"; sed -n "1,200p" "$1"' _ {}
echo '--- Any other renderer overrides for heading ---'
fd -t f 'marked*.ts' apps/renderer/src --exec rg -n "renderer:\s*\{\s*heading\b" --hidden -S {} \;
echo '--- Where headingText() is used ---'
fd -t f 'heading-helper.ts' apps/renderer/src --exec rg -n "headingText" {} \;
echo '--- Marked renderer typing: heading() signature expected by marked ---'
fd -t f 'package.json' apps/renderer --exec sh -c 'echo "marked version in $1:"; jq -r ".dependencies.marked // .devDependencies.marked" "$1"' _ {}Repository: mindfiredigital/markdown-reader
Length of output: 2105
🏁 Script executed:
#!/bin/bash
# Search for any usage of headingText (TOC, etc.)
rg -n "headingText\s*\(" . --hidden
# Check for TOC / slug / heading id generation usage
rg -n "getHeadingId\s*\(" apps/renderer/src --hidden
rg -n "toc|table of contents|Table of Contents" apps/renderer/src --hidden
# Inspect for decodeHtml usage
rg -n "decodeHtml\s*\(" apps/renderer/src --hidden
# Find the HeadingProps type usage
rg -n "HeadingProps" apps/renderer/src --hiddenRepository: mindfiredigital/markdown-reader
Length of output: 4822
🏁 Script executed:
#!/bin/bash
# Show toc extraction code and any marked lexer usage
sed -n '1,200p' apps/renderer/src/renderer/toc.ts
echo '--- extractTOC usage ---'
sed -n '1,120p' apps/renderer/src/hooks/useFile.ts
echo '--- App usage ---'
sed -n '150,260p' apps/renderer/src/App.tsxRepository: mindfiredigital/markdown-reader
Length of output: 5773
🏁 Script executed:
#!/bin/bash
sed -n '1,200p' apps/renderer/src/renderer/markdown.ts
echo '--- any marked usage ---'
rg -n "new Marked|marked\." apps/renderer/src/renderer -S --hidden
echo '--- custom renderer.heading signature callers (if any) ---'
rg -n "\brenderMarkdown\b" apps/renderer/src/renderer -S --hiddenRepository: mindfiredigital/markdown-reader
Length of output: 1079
🌐 Web query:
marked v18 renderer.heading text parameter raw inline markdown or html
💡 Result:
In Marked v18, the renderer methods—including renderer.heading—receive a single token object as an argument, rather than individual parameters like text or level [1]. For the heading method, the token object (of type Tokens.Heading) contains the following relevant properties: - token.tokens: An array of child tokens representing the inline content [2][3]. To render this into HTML (the processed inline markdown/HTML), you should pass this property to this.parser.parseInline(token.tokens) [2][3][1]. - token.raw: The raw, original markdown string that created the heading [1]. - token.depth: The heading level (e.g., 1, 2, 3) [2][1]. If you are overriding the renderer, your implementation should look like this [1]: renderer: { heading(token) { // 'token.tokens' contains the parsed inline structure. // Use 'this.parser.parseInline(token.tokens)' to get the rendered HTML. const text = this.parser.parseInline(token.tokens); return <h${token.depth}>${text}</h${token.depth}>\n; } } Do not rely on a simple text parameter, as the older function signature (e.g., heading(text, level, raw)) is deprecated and no longer used in current versions [1]. Using this.parser.parseInline(token.tokens) ensures that any inline markdown (like bold, italics, or links) or inline HTML within the heading is correctly processed into HTML [2][3].
Citations:
- 1: Linking back to the source markedjs/marked#3591
- 2: https://github.com/markedjs/marked/blob/0ccf4769/src/Renderer.ts
- 3: https://marked.js.org/using_pro
🌐 Web query:
Marked v18 Tokens.Heading properties token.text token.raw token.tokens
💡 Result:
In Marked v18, the Heading token is defined within the library's token system [1][2]. It represents a Markdown heading (e.g., #, ##, etc.) and contains the following specific properties [1][2]: - type: Always set to the string 'heading' [1][2]. - raw: A string containing the exact text from the source markdown that generated this token [1][2][3]. - depth: A number (typically 1–6) representing the level of the heading [1][2]. - text: A string containing the raw text content of the heading [1][2]. - tokens: An array of child tokens (e.g., Text, Em, Strong, Link) representing the inline content within the heading [1][2][3]. The tokens array allows Marked to process and render inline-level elements found inside the heading's text [4][3]. You can access or manipulate these properties, for instance, when writing custom renderers or using the walkTokens feature to process the token tree [5][3][6].
Citations:
- 1: https://github.com/markedjs/marked/blob/0ccf4769/src/Tokens.ts
- 2: https://github.com/markedjs/marked/blob/master/src/Tokens.ts
- 3: https://marked.js.org/using_pro
- 4: https://github.com/markedjs/marked/blob/v18.0.0/docs/USING_PRO.md
- 5: Heading IDs? markedjs/marked#3436
- 6: https://deepwiki.com/markedjs/marked/3.3-hooks-and-token-processing
🌐 Web query:
marked v18 src Tokens.ts Heading text tokens how heading token.text is set
💡 Result:
In marked v18, the heading token’s text field is set when the block tokenizer creates a heading token. 1) Token type definition A heading token includes: - type: 'heading' - raw - depth - text - tokens (inline child tokens) This is defined in src/Tokens.ts as the Heading interface. [1] 2) Where token.text is assigned In src/Tokenizer.ts, the heading(src: string) rule matches a heading line and then computes text like this: - Start with let text = cap[2].trim() (captured heading text). - If trailing # characters exist, it trims them off; behavior depends on options.pedantic: - If endingHash matches: trimmed = rtrim(text, '#'). - If pedantic, it uses text = trimmed.trim(). - Otherwise, it applies CommonMark-like rules for when to accept/remove the trailing hashes (it may fall back to trimmed.trim() or keep spaces trimmed based on endingSpaceChar). - Finally it returns the heading token object with text and tokens: this.lexer.inline(text). [2] So, in code terms, token.text is the processed/trailing-#-adjusted “heading content string” derived from the heading regex capture (cap[2]), not the original raw heading line. [2] 3) Example of resulting token shape The v18 documentation example shows a heading token where text: "heading" (and raw includes the #). [3] If you want, I can also trace how the regex groups cap[1] (depth) and cap[2] (initial heading text) are defined in marked’s heading rule so you can see exactly what part of the source ends up in cap[2].
Citations:
- 1: https://github.com/markedjs/marked/blob/0ccf4769/src/Tokens.ts
- 2: https://github.com/markedjs/marked/blob/0ccf4769/src/Tokenizer.ts
- 3: https://github.com/markedjs/marked/blob/v18.0.0/docs/USING_PRO.md
🏁 Script executed:
rg -n "SLUG_PATTERNS|HTML_PATTERNS" -S apps/renderer/src/utils/constants/regex-constants.ts
sed -n '1,200p' apps/renderer/src/utils/constants/regex-constants.tsRepository: mindfiredigital/markdown-reader
Length of output: 1040
🏁 Script executed:
#!/bin/bash
# Inspect how TOC item text/id are used in rendering and scrolling
fd -t f 'Sidebar.tsx|useCollapsibleToc.ts|scrollToHeading*' apps/renderer/src --exec sh -c 'echo "--- $1 ---"; sed -n "1,220p" "$1"' _
# Also inspect scrollToHeading implementation
rg -n "scrollToHeading" apps/renderer/src --hidden
rg -n "id ===|getElementById|querySelector|scrollIntoView" apps/renderer/src --hiddenRepository: mindfiredigital/markdown-reader
Length of output: 7375
🏁 Script executed:
sed -n '1,120p' apps/renderer/src/hooks/useTOC.tsRepository: mindfiredigital/markdown-reader
Length of output: 1026
Fix marked heading() to render inline markdown and keep TOC heading ids consistent
apps/renderer/src/utils/helpers/heading-helper.tsheading({ text, depth })only doesstripHtml(text)+escapeHtml(plainText)and never renders Marked’s heading inline children (token.tokens), so inline markdown in headings can render literally (e.g.,## **Bold**).headingText(token)usesparseInline(token.text)thenstripHtml(...), leaving HTML entities (e.g.&)—extractTOC()feeds that intogetHeadingId(), which can produce ids that don’t matchheading()’s ids, breaking TOC scroll/active state; decode entities (usedecodeHtml) before computing ids.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/renderer/src/utils/helpers/heading-helper.ts` around lines 16 - 21, The
heading helper must render inline markdown and compute TOC-safe ids from decoded
text: inside heading({ text, depth }) render the inline children (use the same
inline parser used elsewhere — e.g., call parseInline(text) or use token.tokens
to produce an inline HTML string), then compute the id by decoding HTML entities
and stripping tags (call decodeHtml(stripHtml(parsedInline))) and pass that to
getHeadingId; finally escape the rendered inline HTML for safe output
(escapeHtml on the rendered inline HTML where needed) and return the <h${depth}>
with that escaped inline HTML. Also update headingText/token id generation to
decode HTML entities before calling getHeadingId (use
decodeHtml(stripHtml(parseInline(token.text))) or decodeHtml on the existing
stripHtml result) so TOC ids match the heading() output.
| <a | ||
| href="/docs/introduction" | ||
| className="no-underline" | ||
| target="_blank" | ||
| rel="noreferrer" | ||
| > | ||
| <button className="inline-flex items-center justify-center bg-background/50 hover:bg-background/80 text-foreground border border-border/80 font-medium h-11 px-6 text-sm rounded-xl shadow-sm cursor-pointer transition-colors duration-200"> | ||
| <GitBranchIcon className="mr-1.5 h-4 w-4" strokeWidth={2.5} /> | ||
| GitHub | ||
| </button> | ||
| </a> |
There was a problem hiding this comment.
"GitHub" button links to internal docs and ignores baseUrl.
This button shows a Git branch icon with the label "GitHub", but href="/docs/introduction" points to an internal docs page (opened in a new tab). Additionally, the raw <a href="/docs/..."> is not baseUrl-aware, so under the /markdown-reader/ base it resolves to the wrong path on GitHub Pages — unlike the "Get Started" button which uses Docusaurus <Link>.
Either point it to the actual GitHub repo, or fix the label/icon and use a baseUrl-aware link for the docs target.
🔧 Option A — make it actually link to GitHub
- <a
- href="/docs/introduction"
- className="no-underline"
- target="_blank"
- rel="noreferrer"
- >
+ <a
+ href="https://github.com/mindfiredigital/markdown-reader"
+ className="no-underline"
+ target="_blank"
+ rel="noreferrer"
+ >
<button className="inline-flex items-center justify-center bg-background/50 hover:bg-background/80 text-foreground border border-border/80 font-medium h-11 px-6 text-sm rounded-xl shadow-sm cursor-pointer transition-colors duration-200">
<GitBranchIcon className="mr-1.5 h-4 w-4" strokeWidth={2.5} />
GitHub
</button>
</a>If the intent was an internal docs link instead, use <Link to="/docs/introduction"> (baseUrl-aware) and update the label/icon accordingly.
📝 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.
| <a | |
| href="/docs/introduction" | |
| className="no-underline" | |
| target="_blank" | |
| rel="noreferrer" | |
| > | |
| <button className="inline-flex items-center justify-center bg-background/50 hover:bg-background/80 text-foreground border border-border/80 font-medium h-11 px-6 text-sm rounded-xl shadow-sm cursor-pointer transition-colors duration-200"> | |
| <GitBranchIcon className="mr-1.5 h-4 w-4" strokeWidth={2.5} /> | |
| GitHub | |
| </button> | |
| </a> | |
| <a | |
| href="https://github.com/mindfiredigital/markdown-reader" | |
| className="no-underline" | |
| target="_blank" | |
| rel="noreferrer" | |
| > | |
| <button className="inline-flex items-center justify-center bg-background/50 hover:bg-background/80 text-foreground border border-border/80 font-medium h-11 px-6 text-sm rounded-xl shadow-sm cursor-pointer transition-colors duration-200"> | |
| <GitBranchIcon className="mr-1.5 h-4 w-4" strokeWidth={2.5} /> | |
| GitHub | |
| </button> | |
| </a> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/src/components/Homepage/hero.tsx` around lines 55 - 65, The "GitHub"
button currently links to an internal docs path and ignores baseUrl; change the
anchor to point to the real GitHub repo URL (e.g. replace
href="/docs/introduction" with the repo HTTPS URL), keep target="_blank" and
rel="noreferrer", and keep the GitBranchIcon and label "GitHub" so the label,
icon and link are consistent; alternatively, if the intent was a docs link
instead, replace the <a href=...> wrapper with Docusaurus' <Link
to="/docs/introduction"> (baseUrl-aware) and change the icon/label to reflect
"Get Started" or docs. Ensure the unique elements GitBranchIcon and the
surrounding anchor/button are updated together so icon, text and destination
match.
| "pnpm": { | ||
| "overrides": { | ||
| "serialize-javascript": ">=7.0.3", | ||
| "fast-uri": ">=3.1.2", | ||
| "tmp": ">=0.2.6" | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check published versions for the override targets
for pkg in serialize-javascript fast-uri tmp; do
echo "== $pkg =="
curl -s "https://registry.npmjs.org/$pkg" | jq -r '.versions | keys[]' | tail -n 8
done
# Cross-check known advisories
gh api graphql -f query='
{
serialize: securityVulnerabilities(first: 5, ecosystem: NPM, package: "serialize-javascript") { nodes { advisory { severity summary } vulnerableVersionRange firstPatchedVersion { identifier } } }
fastUri: securityVulnerabilities(first: 5, ecosystem: NPM, package: "fast-uri") { nodes { advisory { severity summary } vulnerableVersionRange firstPatchedVersion { identifier } } }
tmp: securityVulnerabilities(first: 5, ecosystem: NPM, package: "tmp") { nodes { advisory { severity summary } vulnerableVersionRange firstPatchedVersion { identifier } } }
}'Repository: mindfiredigital/markdown-reader
Length of output: 2166
Adjust serialize-javascript override floor to cover remaining advisory
All three override minima resolve to published versions (serialize-javascript includes 7.0.3/7.0.4/7.0.5, fast-uri includes 3.1.2, tmp includes 0.2.6), so pnpm install shouldn’t fail resolution. However, serialize-javascript’s CPU exhaustion advisory is patched at 7.0.5 (>=5.0.0, <7.0.5), so >=7.0.3 can still permit vulnerable 7.0.3/7.0.4—bump it to >=7.0.5. fast-uri >=3.1.2 and tmp >=0.2.6 align with the first patched versions for the returned HIGH advisories.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@package.json` around lines 69 - 75, The pnpm override for
"serialize-javascript" currently uses ">=7.0.3" which can still resolve to
vulnerable 7.0.3/7.0.4; update the pnpm.overrides entry for
"serialize-javascript" in package.json to use ">=7.0.5" so the resolved version
excludes the CPU exhaustion advisory, leaving "fast-uri" and "tmp" overrides
unchanged.
| onUpdateAvailable: (callback: (version: string) => void) => void; | ||
| downloadUpdate: () => void; |
There was a problem hiding this comment.
Return an unsubscribe handle for update notifications.
onUpdateAvailable introduces another listener-style API, but unlike onMenuEvent it gives callers no cleanup path. In the renderer that makes effect cleanup impossible and leads to duplicate handlers after remounts. This should either return () => void or have a paired remove method in the public contract.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/shared-types/src/markdown-type.ts` around lines 32 - 33, The
onUpdateAvailable listener API lacks an unsubscribe handle which causes
duplicate handlers on remounts; update the onUpdateAvailable signature in the
markdown-type definitions to return an unsubscribe function (() => void) instead
of void and update any implementing/consuming code (e.g., places that call
onUpdateAvailable and expect no return) to capture and call that returned
function during teardown (or alternatively add a paired removeUpdateAvailable
method across the same contract if you prefer explicit add/remove), ensuring the
unique symbol onUpdateAvailable is updated everywhere to return () => void and
that effects call the unsubscribe on cleanup.
This reverts commit 9e38862.
2 participants
Description
This PR raised to merge dev into main
Type of Change
Checklist
Summary by CodeRabbit
New Features
Improvements
Security
Documentation