Fix/system hardening#96
Closed
Ashminita28 wants to merge 11 commits into
Closed
Conversation
…nown renderer locations
WalkthroughAdds export features (HTML/PDF/DOCX), secure IPC path resolution, settings persistence with atomic writes, folder search, auto-update events, and a renderer UI refactor with new hooks/components. Updates shared contracts, tests, docs, CI security jobs, and repo/tooling configs. ChangesApp features and platform updates
Docs, CI, and repository configuration
Sequence Diagram(s)sequenceDiagram
participant Renderer
participant Preload
participant Main
participant FS
Renderer->>Preload: exportDOCX(html, css, outPath)
Preload->>Main: IPC EXPORT_DOCX
Main->>Main: sanitizeCss + inlineImages + buildDocument
Main->>FS: html-to-docx() + write(outPath)
Main-->>Preload: done
Preload-->>Renderer: success
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 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: 20
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 (3)
apps/main-processor/src/index.ts (1)
65-65:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle rejected promises from
sendFilePathToRendererin the startup and second-instance flows
sendFilePathToRendererisasyncand can reject (e.g., resolver/validation failures), but the call sites at lines 65 and 92 don’tawaitorcatch, so failures become unhandled promise rejections.Suggested fix
- if (filePathToOpen) { - sendFilePathToRenderer(filePathToOpen); - } + if (filePathToOpen) { + void sendFilePathToRenderer(filePathToOpen).catch((error) => { + console.error('Failed to open file from startup flow:', error); + }); + } @@ - sendFilePathToRenderer(filePath); + void sendFilePathToRenderer(filePath).catch((error) => { + console.error('Failed to open file from second-instance flow:', error); + });🤖 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` at line 65, sendFilePathToRenderer is async and its calls in the startup and second-instance flows are not awaited or error-handled, which leads to unhandled promise rejections; fix by awaiting or explicitly handling the promise returned by sendFilePathToRenderer (e.g., await sendFilePathToRenderer(...) inside the async startup handler or append .catch(err => processLogger.error(...)) in the second-instance event handler), ensuring any thrown errors are logged and do not become unhandled rejections; update both call sites that invoke sendFilePathToRenderer to use the same error-handling pattern.apps/renderer/src/hooks/useShortcuts.ts (1)
28-38:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove the duplicated
mod + Shift + Fcondition; the focus-mode branch is unreachable.The first
mod + Shift + Fblock always returns, so the second block with the same condition can never execute. This breaks keyboard access toonToggleFocusModethrough this handler.Based on learnings: Implement keyboard shortcut
Ctrl/Cmd + Shift + Fto search in folder or focus mode.🤖 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 mod+Shift+F blocks in useShortcuts make the second branch unreachable; remove the second block and consolidate into a single if (mod && e.shiftKey && e.key.toLowerCase() === 'f') handler that chooses which action to invoke (onOpenFolderSearch or onToggleFocusMode) based on the current focus state (e.g., a focusMode/isFocusMode boolean from your hook or context), call e.preventDefault() once, then call the appropriate callback and return; update references to onOpenFolderSearch and onToggleFocusMode inside that single conditional so both keyboard paths are reachable.apps/renderer/src/context/ThemeProvider.tsx (1)
9-16:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSystem theme is not applied on first load when no saved preference exists.
themestarts as'github-light'and only updates from system preference on later media-change events. Users withoutapp-themesaved won’t get the correct initial theme.Suggested fix
- const [theme, setThemeState] = useState<Theme>('github-light'); + const [theme, setThemeState] = useState<Theme>(() => { + const saved = localStorage.getItem('app-theme'); + if (saved && APPTHEMES.includes(saved as Theme)) return saved as Theme; + return getSystemTheme(); + }); - useEffect(()=>{ - const saved=localStorage.getItem('app-theme'); - if(saved && APPTHEMES.includes(saved as Theme)){ - setTheme(saved as Theme); - } - },[]) + // initial theme now handled in lazy initializerAlso applies to: 18-27
🤖 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 9 - 16, The initial state uses 'github-light' and the effect incorrectly calls setTheme (typo) and doesn't apply system preference when no saved value exists; update the useEffect in ThemeProvider to (1) read localStorage('app-theme'), (2) if exists and is in APPTHEMES call setThemeState(saved as Theme), (3) else detect system preference via window.matchMedia('(prefers-color-scheme: dark)').matches and call setThemeState to the corresponding theme (e.g. 'github-dark' or 'github-light'), and (4) keep or add the media query listener for changes; also fix the typo by using the actual state setter setThemeState and validate values against APPTHEMES before setting.
🟡 Minor comments (12)
packages/shared-constants/src/menu-constants.ts-11-11 (1)
11-11:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNormalize DOCX label casing in user-facing menu text.
Use
Export as DOCXto keep acronym formatting consistent in the UI.Suggested change
- 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, Update the EXPORT_DOCX constant value so the UI uses the acronym form; locate the EXPORT_DOCX export in menu-constants.ts and change its string from 'Export as Docx' to 'Export as DOCX' so the menu label displays consistent acronym casing.docs/versioned_docs/version-1.0.0/ipc-api.md-7-24 (1)
7-24:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winIPC docs are missing exported channels from the shared contract.
The table is presented as consolidated IPC coverage, but it omits channels already exported in shared constants (for example
CLEAR_RECENT_FILES,SHOW_SAVE_DIALOG, and update-related channels). Please either add the missing entries or explicitly label this table as a subset.🤖 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/ipc-api.md` around lines 7 - 24, The IPC docs table omits channels exported in the shared contract (e.g., CLEAR_RECENT_FILES, SHOW_SAVE_DIALOG and the update-related channels), so update the table in ipc-api.md to include those missing constants with their Direction, Parameters and Returns or, if intentional, add a clear note/header that this table is only a subset of the full shared contract; specifically locate the existing constants like READ_FILE, GET_RECENT_FILES and ADD_RECENT_FILES in the table and add matching rows for CLEAR_RECENT_FILES, SHOW_SAVE_DIALOG and the update channels (or add the explicit “subset” label) so the documentation matches the shared exported constants.apps/main-processor/__tests__/docx.test.ts-8-35 (1)
8-35:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse isolated temp paths per test run to avoid flaky DOCX tests.
Line 8 and Line 9 use stable paths under
tmpdir(), and Line 28 depends on a file created by an earlier test. This can make failures order/environment-dependent (especially for the “invalid path” assertion).Suggested fix
-import { existsSync, unlinkSync, statSync } from 'node:fs'; +import { existsSync, statSync, mkdtempSync, rmSync } from 'node:fs'; import { join } from 'node:path'; import { tmpdir } from 'node:os'; -const docxPath = join(tmpdir(), 'markdown-export.docx'); -const invalidPath = join(tmpdir(), 'missing-folder-test', 'out.docx'); +const tempDir = mkdtempSync(join(tmpdir(), 'markdown-export-')); +const docxPath = join(tempDir, 'markdown-export.docx'); +const invalidPath = join(tempDir, 'missing-folder-test', 'out.docx'); describe('export docx', () => { afterAll(() => { - try { - unlinkSync(docxPath); - } catch {} + rmSync(tempDir, { recursive: true, force: true }); }); @@ - it('it should create a non-empty DOCX file', () => { + it('it should create a non-empty DOCX file', async () => { + await exportDOCX('<h1>Test DOCX</h1><p>Hello DOCX export.</p>', 'body { color: black; }', docxPath); 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 8 - 35, Tests use shared stable paths (docxPath, invalidPath) which makes them order-dependent and flaky; modify the test suite to create isolated temp paths per test (e.g. in beforeEach create a unique tmp dir/name for docxPath and invalidPath using mkdtemp or a timestamp/uuid), call exportDOCX against that per-test docxPath, assert with existsSync/statSync, and clean up the specific file/dir in afterEach (using unlinkSync if exists) so the "invalid path" test always targets a guaranteed-nonexistent directory; update references to docxPath/invalidPath and the cleanup block around unlinkSync/afterAll to use the per-test variables.apps/main-processor/__tests__/folder-search.test.ts-7-17 (1)
7-17:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse a unique temp directory to avoid flaky parallel runs.
Line 7 uses a fixed folder name under OS temp; concurrent test workers/processes can interfere with each other and cause nondeterministic failures.
Suggested fix
-import { writeFileSync, mkdirSync, rmSync } from 'node:fs'; +import { writeFileSync, mkdtempSync, rmSync } from 'node:fs'; import { join } from 'node:path'; import { tmpdir } from 'node:os'; import { searchFolder } from '../src/folder-search'; -const DIR = join(tmpdir(), 'md-search-test'); +const DIR = mkdtempSync(join(tmpdir(), 'md-search-test-')); beforeAll(() => { - mkdirSync(DIR, { recursive: true }); writeFileSync(join(DIR, 'README.md'), '# Project\nThis project uses React.\nReact is great.'); writeFileSync(join(DIR, 'CHANGELOG.md'), '# Changes\nVersion 1.0 released.'); writeFileSync(join(DIR, 'notes.txt'), 'not a markdown file'); });Based on learnings: "Ensure all tests pass before submitting changes by running
pnpm vitest."🤖 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 7 - 17, The temporary directory DIR created in the test setup is a fixed path (join(tmpdir(), 'md-search-test')) which can cause flakes in parallel runs; change the beforeAll/afterAll setup to create a unique temp directory (use mkdtempSync or append a unique suffix like process.pid or crypto.randomUUID) when assigning DIR and use that same DIR in the writeFileSync/remove calls so beforeAll creates the unique folder and afterAll removes it; update the DIR constant initialization and the beforeAll/afterAll blocks (references: DIR, beforeAll, afterAll) accordingly.apps/renderer/__tests__/components/TabBar.test.tsx-52-52 (1)
52-52:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid index-based close-button selection in this test.
Using
getAllByRole(...)[0]couples this test to DOM order and can make it flaky after harmless markup changes. Scope the query to theREADME.mdtab container (or target by tab id) before clicking close.🤖 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__/components/TabBar.test.tsx` at line 52, The test is selecting the first close button by index which couples it to DOM order; update the test in TabBar.test.tsx to scope the query to the specific README.md tab container (or target the tab by its id) before clicking the close button: locate the README tab element (e.g., via getByText / getByRole for the tab label "README.md" or by its test id), use within(readmeTabElement) to call getByRole('button', { name: /close .* tab/i }) and then fireEvent.click that scoped button so the test no longer depends on array indexing.apps/renderer/src/hooks/useOpenFilePath.ts-3-12 (1)
3-12:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd unit tests for this new hook behavior.
This introduces new listener lifecycle behavior but no dedicated test coverage is included in the provided changes (subscribe, callback invocation, cleanup).
As per coding guidelines,
**/*.{test,spec}.{ts,tsx,js,jsx}requires unit tests for all new features.🤖 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/useOpenFilePath.ts` around lines 3 - 12, The hook useOpenFilePath lacks unit tests for its listener lifecycle; add tests that mock window.api and verify subscription, invocation and cleanup. Write tests (matching **/*.{test,spec}.{ts,tsx,js,jsx}) that: render the hook (e.g., with renderHook or mounting a test component) passing a jest.fn() for loadFileInTab, assert window.api.onOpenFilePath is called on mount, simulate invoking the provided callback and assert loadFileInTab was called with the path, and confirm window.api.removeOpenFilePathListener is called on unmount; reference the functions useOpenFilePath, window.api.onOpenFilePath, window.api.removeOpenFilePathListener, and loadFileInTab when implementing mocks and assertions.apps/renderer/src/App.tsx-1-1 (1)
1-1:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMemoize
onOpenSettingsto avoid listener churn across renders.Line 95 and Line 112 create a new function each render, which causes avoidable re-subscription work in downstream hooks that depend on handler identity.
Proposed fix
-import { useEffect, useState, useRef } from 'react'; +import { useCallback, useEffect, useState, useRef } from 'react'; @@ const [settingsOpen, setSettingsOpen] = useState(false); const [appVersion, setAppVersion] = useState(''); + const openSettings = useCallback(() => setSettingsOpen(true), []); @@ - onOpenSettings:()=>setSettingsOpen(true), + onOpenSettings: openSettings, @@ - onOpenSettings:()=>setSettingsOpen(true) + onOpenSettings: openSettings });Also applies to: 95-96, 112-113
🤖 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` at line 1, The onOpenSettings handler is re-created each render causing listener churn; memoize it using React's useCallback and replace the inline functions referenced at the onOpenSettings definition (and the other identical handler instance) so their identity is stable across renders; update any usages that pass this handler (e.g., where onOpenSettings is supplied to downstream hooks or listeners) to use the memoized onOpenSettings returned by useCallback with the correct dependency array (include only values that truly change).apps/renderer/src/hooks/useDragDrop.ts-47-49 (1)
47-49:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCatch dropped-file load failures to avoid unhandled rejections.
If
loadFileInTabrejects, the promise is currently unhandled.Proposed fix
const droppedPath = extractDroppedMdpath(e.dataTransfer); if (droppedPath) { - void loadFileInTab(droppedPath); + void loadFileInTab(droppedPath).catch(() => { + // optionally notify user that file open failed + }); }🤖 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/useDragDrop.ts` around lines 47 - 49, The call to loadFileInTab inside useDragDrop uses void loadFileInTab(droppedPath) which can produce unhandled promise rejections if it rejects; update the droppedPath handling to catch errors from loadFileInTab (either by awaiting it in an async handler or appending .catch) and handle/log the error appropriately (e.g., console.error or the app logger) so failures from loadFileInTab(droppedPath) are not left unhandled.apps/renderer/src/components/UpdateBanner.tsx-24-26 (1)
24-26:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHandle update download failures explicitly.
Line 25 fire-and-forgets
downloadUpdate; if it rejects, this becomes an unhandled promise rejection.Proposed fix
<button type="button" onClick={() => { - void window.api.downloadUpdate(); + if (!window.api?.downloadUpdate) return; + void window.api.downloadUpdate().catch(() => { + // optionally surface a toast/error state + }); }}🤖 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/UpdateBanner.tsx` around lines 24 - 26, The onClick handler in UpdateBanner currently fire-and-forgets window.api.downloadUpdate(), which can cause unhandled promise rejections; change the handler to handle promise failures from downloadUpdate by awaiting it or attaching .catch and surface the error (e.g., call a UI error handler or processLogger/window.api.notify) so failures are logged/shown and not left unhandled—update the onClick closure that calls window.api.downloadUpdate() to catch and handle errors from that call.apps/renderer/src/utils/helpers/sidebar-helper.ts-16-18 (1)
16-18:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winActive level-1 TOC items lose emphasis due to class override.
When an item is level 1 and active,
font-mediumappended at the end overrides the earlierfont-semibold, so active top-level entries become less prominent than intended.Proposed fix
- if (item.id === activeId) { - baseStyle += ' font-medium'; - } + if (item.id === activeId) { + baseStyle += item.level === 1 ? ' font-semibold' : ' font-medium'; + }🤖 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/sidebar-helper.ts` around lines 16 - 18, Active top-level TOC items lose emphasis because when item.id === activeId the code appends 'font-medium' to baseStyle, overriding an earlier 'font-semibold'; update the conditional to preserve stronger weight for level-1 items by checking item.level (e.g., item.level === 1) and appending ' font-semibold' for those, otherwise append ' font-medium' (use the existing variables item.id, activeId and baseStyle to locate and change the logic).apps/renderer/src/components/TabBar.tsx-8-12 (1)
8-12:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep non-tab actions outside the
tablist.The “Open new tab” button is inside
role="tablist", which should contain only tab elements. Move this button outside the tablist container to keep screen-reader tab semantics correct.Also applies to: 56-65
🤖 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/TabBar.tsx` around lines 8 - 12, The Open New Tab button(s) are currently rendered inside the div with role="tablist" in the TabBar component, which must contain only tab elements; move the button markup(s) (the element(s) with labels like "Open new tab" / the renderOpenNewTabButton) out of the role="tablist" container and render them as siblings (e.g., immediately after the tablist div) so they are not children of the element with role="tablist" (also update any styling/container wrappers as needed to preserve layout for the buttons that were at lines ~56-65).apps/renderer/src/utils/helpers/theme-helper.ts-4-6 (1)
4-6:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard
matchMediabefore calling it.This can throw when
matchMediais unavailable (e.g., some test/runtime contexts). Add a safe fallback.Suggested patch
export function getSystemTheme(): Theme { - return window.matchMedia('(prefers-color-scheme: dark)').matches ? 'github-dark' : 'github-light'; + const prefersDark = + typeof window !== 'undefined' && + typeof window.matchMedia === 'function' && + window.matchMedia('(prefers-color-scheme: dark)').matches; + return prefersDark ? 'github-dark' : 'github-light'; }🤖 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/theme-helper.ts` around lines 4 - 6, Guard the call to window.matchMedia in getSystemTheme so it won't throw in environments where matchMedia is undefined; check that typeof window !== 'undefined' and typeof window.matchMedia === 'function' (or just window?.matchMedia) before calling .matches and return a safe default like 'github-light' (or the Theme fallback) when unavailable; update getSystemTheme to perform this existence check and return the fallback Theme if matchMedia cannot be used.
🧹 Nitpick comments (5)
apps/main-processor/__tests__/folder.test.ts (1)
2-21: ⚡ Quick winClean up temporary directories in the test.
At Line 9, the test creates a temp tree but never deletes it. This can slowly pollute CI/local temp storage across runs.
Proposed patch
-import { mkdtemp, mkdir, writeFile } from 'node:fs/promises'; +import { mkdtemp, mkdir, rm, writeFile } from 'node:fs/promises'; @@ it('should enforce the recursion depth', async () => { const root = await mkdtemp(join(tmpdir(), 'markdown-reader-folder-')); - let current = root; - for (let index = 0; index < 12; index += 1) { - current = join(current, `level-${index}`); - await mkdir(current); - await writeFile(join(current, `file-${index}.md`), '# test', 'utf-8'); - } - - const tree = await getFolder(root, 2); - const first = tree.children?.find((child) => child.isDir); - const second = first?.children?.find((child) => child.isDir); - expect(second?.children).toEqual([]); + try { + let current = root; + for (let index = 0; index < 12; index += 1) { + current = join(current, `level-${index}`); + await mkdir(current); + await writeFile(join(current, `file-${index}.md`), '# test', 'utf-8'); + } + + const tree = await getFolder(root, 2); + const first = tree.children?.find((child) => child.isDir); + const second = first?.children?.find((child) => child.isDir); + expect(second?.children).toEqual([]); + } finally { + await rm(root, { recursive: true, force: 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.test.ts` around lines 2 - 21, The test creates a temporary directory via mkdtemp (root) but never removes it; update the test to clean up the temp tree after it runs—either add an afterEach/afterAll hook or wrap the test body in try/finally to call fs.rm or fs.rmdir recursively on the root (the variable produced by mkdtemp) so the directory created for the 'should enforce the recursion depth' test is removed; reference mkdtemp, root, and the getFolder invocation when locating where to add the cleanup.apps/main-processor/src/export/exportPdf.ts (1)
13-15: ⚡ Quick winPin critical
BrowserWindowsecurity flags explicitly.
exportPDFcurrently relies on Electron defaults for isolation/integration. Please setcontextIsolationandnodeIntegrationexplicitly here to avoid regression if defaults/config drift.Suggested hardening diff
const pdfWindow = new BrowserWindow({ show: false, webPreferences: { sandbox: true, + contextIsolation: true, + nodeIntegration: false, + webSecurity: 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 13 - 15, In the BrowserWindow creation inside exportPDF (the exportPDF function), explicitly set webPreferences.contextIsolation = true and webPreferences.nodeIntegration = false (in addition to the existing sandbox: true) so the window does not inherit risky defaults; update the webPreferences object where the BrowserWindow is constructed to include these flags (reference the BrowserWindow constructor in exportPDF) to harden the renderer process against node access and ensure context isolation.apps/renderer/src/styles/export.css (1)
25-31: ⚡ Quick winAvoid globally hiding scrollbars in exported documents.
Applying scrollbar suppression to
*can hurt readability/navigation for overflow-heavy blocks (code, diagrams, math). Prefer scoping this behavior or removing it for exports.🤖 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/styles/export.css` around lines 25 - 31, The export.css currently hides scrollbars globally using the selectors "*::-webkit-scrollbar" and "*" with "scrollbar-width: none", which removes scroll affordances for overflow-heavy content; update the stylesheet to stop applying this globally—either remove these rules entirely or restrict them to a specific export-safe class (e.g., a .hide-scrollbar-export container) or to non-content elements only (e.g., UI wrappers), and ensure code blocks, diagrams, and math containers retain default scrollbars by excluding selectors like pre, code, .diagram, .math from the hide rule so exported documents remain navigable.apps/renderer/src/components/SettingsPanel.tsx (1)
35-43: ⚡ Quick winTrap focus inside the settings dialog while it is open.
aria-modal="true"is set, but focus can still escape to background controls via Tab navigation. Add focus-trap behavior (or make the background inert) so keyboard users stay within the dialog until it closes.🤖 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 35 - 43, The settings dialog (SettingsPanel) currently sets aria-modal="true" but allows Tab to move focus out; implement a focus-trap around the dialogRef (or make the backdrop inert) so keyboard users cannot tab into background controls while open. Specifically, when the panel opens register focus management: save the previously focused element, move focus into the dialog (to an element with id "settings-title" or the first tabbable), intercept Tab/Shift+Tab to cycle between first and last tabbable elements inside the dialog, and restore focus on close (onClose). You can either integrate a tested library (e.g., focus-trap or `@headlessui/focus-trap`) keyed to dialogRef or implement the first/last tabbable logic in useEffect; ensure backdrop elements are inert/aria-hidden while open and keep dialog role="dialog" and aria-modal unchanged.apps/renderer/src/renderer/toc.ts (1)
10-15: ⚡ Quick winRename the
extractTOCinput contract to markdown.The function now tokenizes markdown, but the parameter name/comment still describe HTML. That mismatch can cause incorrect future call sites.
Suggested patch
-// extracts table of content from HTML string -export function extractTOC(html: string): TOCType[] { +// extracts table of content from Markdown text +export function extractTOC(markdown: string): TOCType[] { const items: TOCType[] = []; const registry = createHeadingRegistry(); - const tokens = lexer(html); + const tokens = lexer(markdown);🤖 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/toc.ts` around lines 10 - 15, The function extractTOC currently accepts a parameter named html and has a comment saying it extracts a table of content from an HTML string, but it actually tokenizes markdown using lexer and createHeadingRegistry; rename the parameter from html to markdown and update the function comment to reflect that it extracts a TOC from a markdown string so callers and future maintainers have the correct contract (update any internal references to the parameter and keep extractTOC, lexer, and createHeadingRegistry usages intact).
🤖 Prompt for all review comments with 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.
Inline comments:
In @.github/workflows/development.yml:
- Line 23: Update the workflow to harden action usage: replace loose tags like
actions/checkout@v4, pnpm/action-setup@v4, actions/setup-node@v4 and
aquasecurity/trivy-action@v0.36.0 with their corresponding full commit SHAs, and
add with: persist-credentials: false to every actions/checkout step (both
occurrences) so credentials are not persisted; keep the same step names and
behavior but use the SHA-pinned references and the persist-credentials: false
setting.
In @.github/workflows/production.yml:
- Line 23: Replace mutable action tags with immutable refs and disable checkout
credentials: update the workflow entries for actions/checkout,
actions/setup-node, and pnpm/action-setup to use a pinned commit SHA (or other
immutable ref) instead of the floating `@v4` tag, and add with:
persist-credentials: false to both uses: actions/checkout occurrences so the job
does not leave credentials in the local git config; ensure both checkout steps
are changed and the other actions (actions/setup-node and pnpm/action-setup) use
committed SHAs.
In `@apps/main-processor/__tests__/settings.test.ts`:
- Around line 5-61: Add unit tests covering the new persistence flows: (1)
getSettings: test read fallback by mocking fs read for the primary path to throw
and the legacy path to return known JSON and assert returned settings come from
legacy; (2) writeSettingsAtomically: simulate a failure during the atomic write
(e.g., write to temp throws or rename throws) and assert cleanup occurs (temp
file removed and original file unchanged) and that saveSettings surfaces a
proper error; (3) runExclusive / saveSettings concurrency: run multiple
concurrent saveSettings calls (mocking underlying write to record order and
delay) and assert runExclusive serializes them (writes occur one after another,
not concurrently); use vi.spyOn or mocked fs/promises functions to control
success/failure and cleanup assertions and call validateSettings where
appropriate to ensure saved payloads are valid.
In `@apps/main-processor/src/export/inlineImage.ts`:
- Around line 23-40: The code resolves src immediately with normaliseImagePath
and realpath (creating resolveImagePath) which treats relative image paths as
relative to the process cwd; instead, detect when src is a relative path and,
before calling realpath, resolve it against the markdown file's directory (use
dirname(file) for each allowedMarkdownFiles entry or the specific markdown file
context you have) so that ./ and ../ image references are resolved relative to
the markdown parent; then call realpath on that joined path and continue the
existing allowedFolderRoots / allowedMarkdownFiles checks using
resolveImagePath, ensuring local markdown images are correctly recognized for
inlining (update references in normaliseImagePath/realpath usage and any place
that sets resolveImagePath).
In `@apps/main-processor/src/file.ts`:
- Around line 47-77: The error is that error/change/unlink callbacks use only
filePath and may act on stale watchers; fix by capturing the local watcher
instance (or a unique token) when registering handlers and, inside each callback
(the 'error' handler that calls unWatchFile(filePath), the 'change' timer
callback that calls onChange(), and the 'unlink' handler that calls
unWatchFile(filePath) / onDeleted()), first verify that
currentWatchers.get(filePath) === watcher (or token) before proceeding; if it
does not match, bail out (clear any local timers as needed) so you never close
or signal for a newer watcher instance. Ensure you reference the existing
symbols watcher, unWatchFile, currentWatchers, debounceTimers, onChange, and
onDeleted when implementing the guard.
In `@apps/main-processor/src/ipc.ts`:
- Around line 124-126: The READ_FOLDER handler currently resolves the incoming
folderPath with resolveDirectoryPath and unconditionally adds it to
allowedFolderRoots then returns getFolder(safeFolderPath); remove the
self-authorization step: do not call allowedFolderRoots.add(safeFolderPath) in
the READ_FOLDER flow; instead validate that safeFolderPath is already in
allowedFolderRoots (or perform an explicit authorization check) before calling
getFolder(safeFolderPath), and ensure only authorized paths (per SEARCH_FOLDER
rules) are returned—update the READ_FOLDER handler to check allowedFolderRoots
and deny/throw if the path is not allowed.
In `@apps/main-processor/src/settings/get-settings.ts`:
- Around line 12-17: The current code returns DEFAULT_SETTINGS if
validateSettings(parsed) throws for unknown keys, discarding all other valid
persisted preferences; fix by catching validation errors for unknown keys and
instead merge defaults with only the known/allowed keys from parsed. Concretely:
in the get-settings logic around parsed/validateSettings, replace the
unconditional catch-return DEFAULT_SETTINGS with code that builds a sanitized
object by picking keys from parsed that exist in DEFAULT_SETTINGS (or run
validateSettings on that filtered object), then return { ...DEFAULT_SETTINGS,
...sanitizedValidatedSettings }; reference validateSettings, parsed, and
DEFAULT_SETTINGS to locate where to change.
In `@apps/main-processor/src/updater.ts`:
- Around line 13-19: autoUpdater callbacks currently call
window.webContents.send(...) unguarded which can throw if the BrowserWindow or
its webContents are destroyed; update the 'update-available' and
'update-downloaded' handlers to first verify the window and webContents are not
destroyed (e.g. check window && !window.isDestroyed() and window.webContents &&
!window.webContents.isDestroyed()) before calling window.webContents.send with
IPC_CONSTANTS.UPDATE_AVAILABLE / IPC_CONSTANTS.UPDATE_DOWNLOADED (or bail out if
destroyed); ensure you reference the same autoUpdater, UpdateInfo, window, and
IPC_CONSTANTS symbols used in the diff.
In `@apps/main-processor/src/utils/constants/ipc-validation.ts`:
- Line 28: The current validateSender logic trusts any http://localhost origin;
update validateSender to require an exact match against the configured renderer
URL: if process.env.ELECTRON_RENDERER_URL is missing return false (fail closed),
otherwise compute the origin of process.env.ELECTRON_RENDERER_URL (via new
URL(...).origin) and only return true when the parsed sender origin equals that
renderer origin; reference the validateSender function and
process.env.ELECTRON_RENDERER_URL when making this change.
In `@apps/renderer/__tests__/renderer/markdown.test.ts`:
- Around line 47-57: The tests in
apps/renderer/__tests__/renderer/markdown.test.ts currently assert that
dangerous HTML survives rendering (using renderMarkdown), which is inverted;
update the two assertions for the iframe and inline event-handler cases to
assert absence instead of presence (use expect(html).not.toContain('<iframe')
and expect(html).not.toContain('onerror') or equivalent) so the tests verify
that renderMarkdown sanitizes/removes dangerous elements/attributes (the two
affected specs reference the iframe case and the onerror img case).
In `@apps/renderer/setup.ts`:
- Around line 25-37: The global mutable mockSettings is shared across tests
causing leaks; update the window.api mock so getSettings returns a fresh copy
(e.g., deep/shallow clone) instead of the original object and ensure
saveSettings updates an internal immutable state correctly; additionally expose
or implement a reset function (or reinitialize the internal state in a
beforeEach) so mockSettings is reset to the initial defaults between
tests—locate the mock in setup.ts where mockSettings, getSettings, saveSettings
and window.api are defined and change their implementations to return/cloned
objects and support resetting.
In `@apps/renderer/src/config/marked.ts`:
- Around line 9-24: The module-level mutable currentRegistry causes a race when
reusing the singleton Marked instance; change getMarkdown so the registry is
stored per-renderer instead of a module global: either create a new Marked each
call (avoid sharing the singleton instance) or attach the provided registry to
the Marked object returned (e.g., instance.registry = registry) and update the
renderer.heading closure to read the registry from that instance (use this or
instance reference) rather than from currentRegistry; update references to
currentRegistry and remove the module-level variable currentRegistry to
eliminate the race between getMarkdown, Marked, and the heading renderer.
In `@apps/renderer/src/hooks/useExport.ts`:
- Around line 6-31: The export handlers exportHtml, exportPdf, and exportDocx
currently call window.api.showSaveDialog and
window.api.exportHTML/exportPDF/exportDOCX without error handling; wrap the body
of each useCallback in a try/catch that awaits the IPC calls and catches any
rejection, handle errors by logging via console.error (or processLogger) and
optionally showing a user-facing alert/dialog, and ensure the catch
returns/cleans up to avoid unhandled promise rejections when showSaveDialog or
the export IPC rejects.
In `@apps/renderer/src/hooks/useFileActions.ts`:
- Around line 9-41: Wrap all IPC and file-load async calls in explicit try/catch
blocks to prevent unhandled rejections: in openFolder, catch errors from
window.api.openFolderDialog() and window.api.readFolder() and handle/log them
(and avoid setting state on failure); in openFileDialog, catch errors from
window.api.openFileDialog() and from downstream loadFileInTab invocation; and
inside loadFileInTab wrap the await loadFile(path) call in try/catch and
handle/log loadFile errors before dispatching the OPEN_TAB action. Use the
existing function names openFolder, openFileDialog, loadFileInTab and the IPC
calls window.api.openFolderDialog, window.api.readFolder,
window.api.openFileDialog and loadFile to locate where to add the try/catch and
error handling.
In `@apps/renderer/src/hooks/useFilePersistence.ts`:
- Around line 37-55: The debounced async callback in useFilePersistence calls
await loadFile(activeTab.filePath) without error handling, risking unhandled
rejections and partial UI updates; wrap the loadFile call and subsequent logic
in a try/catch inside the debounceTimer.current callback (catching errors from
loadFile and any downstream awaits), on error abort the update flow (don’t call
dispatch/scroll/ setShowToast) and surface the failure (e.g., log or set an
error state/toast) so the watcher reload failure is safely handled; update the
async timeout callback that references loadFile, contentRef, dispatch, and
setShowToast accordingly.
In `@apps/renderer/src/hooks/useOpenFilePath.ts`:
- Around line 5-7: The onOpenFilePath callback currently calls void
loadFileInTab(path) which silences Promise rejections; change the event handler
to handle errors from loadFileInTab (the handler passed to
window.api.onOpenFilePath). Wrap the call in an async function and await
loadFileInTab(path) with try/catch, or call loadFileInTab(path).catch(err => {
/* log or handle error */ }), so any rejection is caught and logged/handled
rather than becoming an unhandled promise rejection.
In `@apps/renderer/src/hooks/useSettings.ts`:
- Around line 58-68: increaseFontSize and decreaseFontSize compute new fontSize
from the captured settings.fontSize, causing races on rapid repeated calls;
change each to call updateSettings with a functional updater that receives the
previous settings and computes fontSize as Math.min(FONT_SIZE.MAX, prev.fontSize
+ FONT_SIZE.INCREMENT) (and Math.max for decrease) so updates are based on the
latest state, and then remove settings.fontSize from the useCallback dependency
arrays; target the increaseFontSize and decreaseFontSize functions and the
updateSettings call-site to implement this functional update pattern.
In `@apps/renderer/src/styles/export.css`:
- Line 7: The export CSS currently hardcodes light-mode colors (background:
`#ffffff` and color: `#111827`) which forces light styling in exported HTML; update
the export flow so export.css does not use fixed hex values but instead uses
theme-aware CSS variables (e.g., background: var(--bg); color: var(--text)) or,
better, during the export process compute and inline the document's current root
variables into the exported HTML (replace the background and color declarations
in export.css with the active theme's computed values) so the exported
self-contained HTML preserves the active theme, typography, code blocks, math,
diagrams, and images.
In `@docs/src/components/Homepage/hero.tsx`:
- Around line 49-53: The CTA currently nests a <button> inside a <Link>, which
is invalid and harms accessibility; replace the nested button with the Link
itself as the interactive element by moving the button's classes and attributes
onto the <Link> (e.g., set className="inline-flex items-center justify-center
bg-primary ..." and include aria-label or role="button" if desired) and render
the text and <ArrowRight/> directly inside the <Link>; make the same change for
the other CTA instance that wraps lines 55-65 so no <button> elements are nested
inside <Link>.
In `@package.json`:
- Around line 69-74: Update the pnpm.overrides entry in package.json to pin
exact patched versions instead of open-ended ranges: replace the
"serialize-javascript": ">=7.0.5", "fast-uri": ">=3.1.2", and "tmp": ">=0.2.6"
entries under pnpm.overrides with the exact versions found in the lockfile
(e.g., "serialize-javascript": "7.0.5", "fast-uri": "3.1.2", "tmp": "0.2.7") so
that pnpm.overrides uses fixed versions; modify the package.json pnpm.overrides
block accordingly and then regenerate/verify the lockfile to ensure resolutions
match.
---
Outside diff comments:
In `@apps/main-processor/src/index.ts`:
- Line 65: sendFilePathToRenderer is async and its calls in the startup and
second-instance flows are not awaited or error-handled, which leads to unhandled
promise rejections; fix by awaiting or explicitly handling the promise returned
by sendFilePathToRenderer (e.g., await sendFilePathToRenderer(...) inside the
async startup handler or append .catch(err => processLogger.error(...)) in the
second-instance event handler), ensuring any thrown errors are logged and do not
become unhandled rejections; update both call sites that invoke
sendFilePathToRenderer to use the same error-handling pattern.
In `@apps/renderer/src/context/ThemeProvider.tsx`:
- Around line 9-16: The initial state uses 'github-light' and the effect
incorrectly calls setTheme (typo) and doesn't apply system preference when no
saved value exists; update the useEffect in ThemeProvider to (1) read
localStorage('app-theme'), (2) if exists and is in APPTHEMES call
setThemeState(saved as Theme), (3) else detect system preference via
window.matchMedia('(prefers-color-scheme: dark)').matches and call setThemeState
to the corresponding theme (e.g. 'github-dark' or 'github-light'), and (4) keep
or add the media query listener for changes; also fix the typo by using the
actual state setter setThemeState and validate values against APPTHEMES before
setting.
In `@apps/renderer/src/hooks/useShortcuts.ts`:
- Around line 28-38: The duplicate mod+Shift+F blocks in useShortcuts make the
second branch unreachable; remove the second block and consolidate into a single
if (mod && e.shiftKey && e.key.toLowerCase() === 'f') handler that chooses which
action to invoke (onOpenFolderSearch or onToggleFocusMode) based on the current
focus state (e.g., a focusMode/isFocusMode boolean from your hook or context),
call e.preventDefault() once, then call the appropriate callback and return;
update references to onOpenFolderSearch and onToggleFocusMode inside that single
conditional so both keyboard paths are reachable.
---
Minor comments:
In `@apps/main-processor/__tests__/docx.test.ts`:
- Around line 8-35: Tests use shared stable paths (docxPath, invalidPath) which
makes them order-dependent and flaky; modify the test suite to create isolated
temp paths per test (e.g. in beforeEach create a unique tmp dir/name for
docxPath and invalidPath using mkdtemp or a timestamp/uuid), call exportDOCX
against that per-test docxPath, assert with existsSync/statSync, and clean up
the specific file/dir in afterEach (using unlinkSync if exists) so the "invalid
path" test always targets a guaranteed-nonexistent directory; update references
to docxPath/invalidPath and the cleanup block around unlinkSync/afterAll to use
the per-test variables.
In `@apps/main-processor/__tests__/folder-search.test.ts`:
- Around line 7-17: The temporary directory DIR created in the test setup is a
fixed path (join(tmpdir(), 'md-search-test')) which can cause flakes in parallel
runs; change the beforeAll/afterAll setup to create a unique temp directory (use
mkdtempSync or append a unique suffix like process.pid or crypto.randomUUID)
when assigning DIR and use that same DIR in the writeFileSync/remove calls so
beforeAll creates the unique folder and afterAll removes it; update the DIR
constant initialization and the beforeAll/afterAll blocks (references: DIR,
beforeAll, afterAll) accordingly.
In `@apps/renderer/__tests__/components/TabBar.test.tsx`:
- Line 52: The test is selecting the first close button by index which couples
it to DOM order; update the test in TabBar.test.tsx to scope the query to the
specific README.md tab container (or target the tab by its id) before clicking
the close button: locate the README tab element (e.g., via getByText / getByRole
for the tab label "README.md" or by its test id), use within(readmeTabElement)
to call getByRole('button', { name: /close .* tab/i }) and then fireEvent.click
that scoped button so the test no longer depends on array indexing.
In `@apps/renderer/src/App.tsx`:
- Line 1: The onOpenSettings handler is re-created each render causing listener
churn; memoize it using React's useCallback and replace the inline functions
referenced at the onOpenSettings definition (and the other identical handler
instance) so their identity is stable across renders; update any usages that
pass this handler (e.g., where onOpenSettings is supplied to downstream hooks or
listeners) to use the memoized onOpenSettings returned by useCallback with the
correct dependency array (include only values that truly change).
In `@apps/renderer/src/components/TabBar.tsx`:
- Around line 8-12: The Open New Tab button(s) are currently rendered inside the
div with role="tablist" in the TabBar component, which must contain only tab
elements; move the button markup(s) (the element(s) with labels like "Open new
tab" / the renderOpenNewTabButton) out of the role="tablist" container and
render them as siblings (e.g., immediately after the tablist div) so they are
not children of the element with role="tablist" (also update any
styling/container wrappers as needed to preserve layout for the buttons that
were at lines ~56-65).
In `@apps/renderer/src/components/UpdateBanner.tsx`:
- Around line 24-26: The onClick handler in UpdateBanner currently
fire-and-forgets window.api.downloadUpdate(), which can cause unhandled promise
rejections; change the handler to handle promise failures from downloadUpdate by
awaiting it or attaching .catch and surface the error (e.g., call a UI error
handler or processLogger/window.api.notify) so failures are logged/shown and not
left unhandled—update the onClick closure that calls window.api.downloadUpdate()
to catch and handle errors from that call.
In `@apps/renderer/src/hooks/useDragDrop.ts`:
- Around line 47-49: The call to loadFileInTab inside useDragDrop uses void
loadFileInTab(droppedPath) which can produce unhandled promise rejections if it
rejects; update the droppedPath handling to catch errors from loadFileInTab
(either by awaiting it in an async handler or appending .catch) and handle/log
the error appropriately (e.g., console.error or the app logger) so failures from
loadFileInTab(droppedPath) are not left unhandled.
In `@apps/renderer/src/hooks/useOpenFilePath.ts`:
- Around line 3-12: The hook useOpenFilePath lacks unit tests for its listener
lifecycle; add tests that mock window.api and verify subscription, invocation
and cleanup. Write tests (matching **/*.{test,spec}.{ts,tsx,js,jsx}) that:
render the hook (e.g., with renderHook or mounting a test component) passing a
jest.fn() for loadFileInTab, assert window.api.onOpenFilePath is called on
mount, simulate invoking the provided callback and assert loadFileInTab was
called with the path, and confirm window.api.removeOpenFilePathListener is
called on unmount; reference the functions useOpenFilePath,
window.api.onOpenFilePath, window.api.removeOpenFilePathListener, and
loadFileInTab when implementing mocks and assertions.
In `@apps/renderer/src/utils/helpers/sidebar-helper.ts`:
- Around line 16-18: Active top-level TOC items lose emphasis because when
item.id === activeId the code appends 'font-medium' to baseStyle, overriding an
earlier 'font-semibold'; update the conditional to preserve stronger weight for
level-1 items by checking item.level (e.g., item.level === 1) and appending '
font-semibold' for those, otherwise append ' font-medium' (use the existing
variables item.id, activeId and baseStyle to locate and change the logic).
In `@apps/renderer/src/utils/helpers/theme-helper.ts`:
- Around line 4-6: Guard the call to window.matchMedia in getSystemTheme so it
won't throw in environments where matchMedia is undefined; check that typeof
window !== 'undefined' and typeof window.matchMedia === 'function' (or just
window?.matchMedia) before calling .matches and return a safe default like
'github-light' (or the Theme fallback) when unavailable; update getSystemTheme
to perform this existence check and return the fallback Theme if matchMedia
cannot be used.
In `@docs/versioned_docs/version-1.0.0/ipc-api.md`:
- Around line 7-24: The IPC docs table omits channels exported in the shared
contract (e.g., CLEAR_RECENT_FILES, SHOW_SAVE_DIALOG and the update-related
channels), so update the table in ipc-api.md to include those missing constants
with their Direction, Parameters and Returns or, if intentional, add a clear
note/header that this table is only a subset of the full shared contract;
specifically locate the existing constants like READ_FILE, GET_RECENT_FILES and
ADD_RECENT_FILES in the table and add matching rows for CLEAR_RECENT_FILES,
SHOW_SAVE_DIALOG and the update channels (or add the explicit “subset” label) so
the documentation matches the shared exported constants.
In `@packages/shared-constants/src/menu-constants.ts`:
- Line 11: Update the EXPORT_DOCX constant value so the UI uses the acronym
form; locate the EXPORT_DOCX export in menu-constants.ts and change its string
from 'Export as Docx' to 'Export as DOCX' so the menu label displays consistent
acronym casing.
---
Nitpick comments:
In `@apps/main-processor/__tests__/folder.test.ts`:
- Around line 2-21: The test creates a temporary directory via mkdtemp (root)
but never removes it; update the test to clean up the temp tree after it
runs—either add an afterEach/afterAll hook or wrap the test body in try/finally
to call fs.rm or fs.rmdir recursively on the root (the variable produced by
mkdtemp) so the directory created for the 'should enforce the recursion depth'
test is removed; reference mkdtemp, root, and the getFolder invocation when
locating where to add the cleanup.
In `@apps/main-processor/src/export/exportPdf.ts`:
- Around line 13-15: In the BrowserWindow creation inside exportPDF (the
exportPDF function), explicitly set webPreferences.contextIsolation = true and
webPreferences.nodeIntegration = false (in addition to the existing sandbox:
true) so the window does not inherit risky defaults; update the webPreferences
object where the BrowserWindow is constructed to include these flags (reference
the BrowserWindow constructor in exportPDF) to harden the renderer process
against node access and ensure context isolation.
In `@apps/renderer/src/components/SettingsPanel.tsx`:
- Around line 35-43: The settings dialog (SettingsPanel) currently sets
aria-modal="true" but allows Tab to move focus out; implement a focus-trap
around the dialogRef (or make the backdrop inert) so keyboard users cannot tab
into background controls while open. Specifically, when the panel opens register
focus management: save the previously focused element, move focus into the
dialog (to an element with id "settings-title" or the first tabbable), intercept
Tab/Shift+Tab to cycle between first and last tabbable elements inside the
dialog, and restore focus on close (onClose). You can either integrate a tested
library (e.g., focus-trap or `@headlessui/focus-trap`) keyed to dialogRef or
implement the first/last tabbable logic in useEffect; ensure backdrop elements
are inert/aria-hidden while open and keep dialog role="dialog" and aria-modal
unchanged.
In `@apps/renderer/src/renderer/toc.ts`:
- Around line 10-15: The function extractTOC currently accepts a parameter named
html and has a comment saying it extracts a table of content from an HTML
string, but it actually tokenizes markdown using lexer and
createHeadingRegistry; rename the parameter from html to markdown and update the
function comment to reflect that it extracts a TOC from a markdown string so
callers and future maintainers have the correct contract (update any internal
references to the parameter and keep extractTOC, lexer, and
createHeadingRegistry usages intact).
In `@apps/renderer/src/styles/export.css`:
- Around line 25-31: The export.css currently hides scrollbars globally using
the selectors "*::-webkit-scrollbar" and "*" with "scrollbar-width: none", which
removes scroll affordances for overflow-heavy content; update the stylesheet to
stop applying this globally—either remove these rules entirely or restrict them
to a specific export-safe class (e.g., a .hide-scrollbar-export container) or to
non-content elements only (e.g., UI wrappers), and ensure code blocks, diagrams,
and math containers retain default scrollbars by excluding selectors like pre,
code, .diagram, .math from the hide rule so exported documents remain navigable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
🛑 Comments failed to post (20)
.github/workflows/development.yml (1)
23-23:
⚠️ Potential issue | 🟠 Major | ⚡ Quick win🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail ls -la .github/workflows || true sed -n '1,120p' .github/workflows/development.yml | nl -ba | sed -n '1,120p'Repository: mindfiredigital/markdown-reader
Length of output: 504
🏁 Script executed:
#!/bin/bash set -euo pipefail sed -n '1,140p' .github/workflows/development.yml | nl -baRepository: mindfiredigital/markdown-reader
Length of output: 118
🏁 Script executed:
#!/bin/bash set -euo pipefail sed -n '1,200p' .github/workflows/development.yml | nl -baRepository: mindfiredigital/markdown-reader
Length of output: 118
🏁 Script executed:
#!/bin/bash set -euo pipefail sed -n '1,200p' .github/workflows/development.yml | nl -baRepository: mindfiredigital/markdown-reader
Length of output: 118
🏁 Script executed:
#!/bin/bash set -euo pipefail sed -n '1,240p' .github/workflows/development.yml | nl -baRepository: mindfiredigital/markdown-reader
Length of output: 118
🏁 Script executed:
#!/bin/bash set -euo pipefail # Show only the relevant parts around reported lines (23, 47-48, 51, 54, 66) for L in 15 20 23 30 40 47 48 51 54 60 66 80; do echo "---- around $L ----" sed -n "$((L-5)),$((L+5))p" .github/workflows/development.yml | nl -ba doneRepository: mindfiredigital/markdown-reader
Length of output: 156
🏁 Script executed:
#!/bin/bash set -euo pipefail for L in 23 47 48 51 54 66; do echo "=== around line $L ===" start=$((L-6)); end=$((L+8)) [ $start -lt 1 ] && start=1 sed -n "${start},${end}p" .github/workflows/development.yml | nl -ba doneRepository: mindfiredigital/markdown-reader
Length of output: 159
🏁 Script executed:
#!/bin/bash set -euo pipefail FILE=".github/workflows/development.yml" echo "=== File: $FILE (first 140 lines) ===" awk '{printf "%4d\t%s\n", NR, $0}' "$FILE" | sed -n '1,140p' echo "=== Around key lines (18-30) ===" awk '{printf "%4d\t%s\n", NR, $0}' "$FILE" | sed -n '18,30p' echo "=== Around key lines (42-60) ===" awk '{printf "%4d\t%s\n", NR, $0}' "$FILE" | sed -n '42,60p' echo "=== Around key lines (60-72) ===" awk '{printf "%4d\t%s\n", NR, $0}' "$FILE" | sed -n '60,72p'Repository: mindfiredigital/markdown-reader
Length of output: 3282
🌐 Web query:
GitHub actions/checkout persist-credentials default value💡 Result:
The default value for the persist-credentials input in the GitHub actions/checkout action is true [1][2][3]. When this option is set to its default (true), the action configures the provided token or SSH key with the local git configuration [1][3]. This allows your workflow scripts to run authenticated git commands (such as git fetch or git push) without requiring additional setup [1][2]. The credentials are removed during the post-job cleanup phase [1][2]. You can set persist-credentials to false if you wish to opt out of this behavior [1][4].
Citations:
- 1: https://github.com/actions/checkout/?tab=readme-ov-file
- 2: https://github.com/actions/checkout/tree/v6.0.2?tab=readme-ov-file
- 3: https://github.com/actions/checkout/blob/72f2cec99f417b1a1c5e2e88945068983b7965f9/action.yml
- 4: https://github.com/actions/checkout/tree/refs/heads/main
Pin action versions to full commit SHAs and set
persist-credentials: falsefor checkout (development.yml).
actions/checkout@v4is used (lines 15 and 48) withoutwith: persist-credentials: false; default istrue, so credentials are configured/persisted longer than needed.pnpm/action-setup@v4(lines 23, 51),actions/setup-node@v4(lines 18, 54), andaquasecurity/trivy-action@v0.36.0(line 66) are not pinned to full commit SHAs.🔒 Suggested hardening
- - name: Checkout - uses: actions/checkout@v4 + - name: Checkout + uses: actions/checkout@<full-commit-sha> + with: + persist-credentials: false - - name: Setup pnpm - uses: pnpm/action-setup@v4 + - name: Setup pnpm + uses: pnpm/action-setup@<full-commit-sha> - - name: Setup Node - uses: actions/setup-node@v4 + - name: Setup Node + uses: actions/setup-node@<full-commit-sha> - - name: Run Trivy vulnerability scanner - uses: aquasecurity/trivy-action@v0.36.0 + - name: Run Trivy vulnerability scanner + uses: aquasecurity/trivy-action@<full-commit-sha>🧰 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/development.yml at line 23, Update the workflow to harden action usage: replace loose tags like actions/checkout@v4, pnpm/action-setup@v4, actions/setup-node@v4 and aquasecurity/trivy-action@v0.36.0 with their corresponding full commit SHAs, and add with: persist-credentials: false to every actions/checkout step (both occurrences) so credentials are not persisted; keep the same step names and behavior but use the SHA-pinned references and the persist-credentials: false setting..github/workflows/production.yml (1)
23-23:
⚠️ Potential issue | 🟠 Major | ⚡ Quick win🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail file=".github/workflows/production.yml" echo "== File: $file ==" if [ ! -f "$file" ]; then echo "Missing file: $file" exit 1 fi echo echo "== Lines 1-120 (for context) ==" nl -ba "$file" | sed -n '1,120p' echo echo "== Lines around 15-90 (pin/checkout/cred-related) ==" nl -ba "$file" | sed -n '10,90p' echo echo "== Action usage summary ==" rg -n --hidden --no-ignore-vcs "uses:\s*([a-zA-Z0-9_.-]+/[a-zA-Z0-9_.-]+@.+)" "$file" || true echo echo "== Specifically search for checkout/persist-credentials ==" rg -n --hidden --no-ignore-vcs "actions/checkout@|persist-credentials|checkout" "$file" || true echo echo "== Specifically search for pnpm/action-setup@ ==" rg -n --hidden --no-ignore-vcs "pnpm/action-setup@" "$file" || true echo echo "== Show any other credential persistence indicators ==" rg -n --hidden --no-ignore-vcs "(persist-credentials|GITHUB_TOKEN|token|secrets\.)" "$file" || trueRepository: mindfiredigital/markdown-reader
Length of output: 215
🏁 Script executed:
#!/bin/bash set -euo pipefail file=".github/workflows/production.yml" echo "== Check file ==" ls -la "$file" || exit 1 echo echo "== Context lines 1-120 ==" cat -n "$file" | sed -n '1,120p' echo echo "== Context lines 15-90 ==" cat -n "$file" | sed -n '15,90p' echo echo "== All action uses with @... found in production.yml ==" rg -n --no-ignore-vcs "uses:\s*[\"']?[^\"'\n]+\@[^\"'\n]+" "$file" || true echo echo "== Any mutable action tags (uses: owner/repo@v*) ==" rg -n --no-ignore-vcs "uses:\s*[^\"'\n]+\`@v`[0-9]+(\.[0-9]+)*" "$file" || true echo echo "== Specifically search for checkout + persist-credentials ==" rg -n --no-ignore-vcs "actions/checkout@|persist-credentials" "$file" || true echo echo "== Specifically search for pnpm/action-setup@ ==" rg -n --no-ignore-vcs "pnpm/action-setup@" "$file" || true echo echo "== Search for token/secret credential persistence indicators ==" rg -n --no-ignore-vcs "(persist-credentials|GITHUB_TOKEN|actions/checkout\.|secrets\.|token:)" "$file" || trueRepository: mindfiredigital/markdown-reader
Length of output: 4521
🌐 Web query:
actions/checkout@v4 persist-credentials default value💡 Result:
The default value for the persist-credentials input in actions/checkout@v4 is true [1][2][3]. When set to true (the default), the action configures the authentication token or SSH key with the local git configuration [2][3]. This allows subsequent steps in your workflow to execute authenticated git commands [3][4]. The token is automatically removed during the post-job cleanup process [3][5]. If you wish to disable this behavior, you must explicitly set persist-credentials to false [3][4].
Citations:
- 1: https://github.com/actions/checkout/blob/v4.1.1/action.yml
- 2: https://github.com/actions/checkout/blob/v4/action.yml
- 3: https://github.com/actions/checkout/blob/v4/README.md
- 4: https://github.com/actions/checkout
- 5: https://github.com/actions/checkout/?tab=readme-ov-file
Pin actions to immutable references and harden checkout credential handling in production CI.
actions/checkout@v4(lines 15, 48),actions/setup-node@v4(lines 18, 54), andpnpm/action-setup@v4(lines 23, 51) use mutable@v4tags—pin to a commit SHA (or otherwise immutable ref).actions/checkout@v4defaultspersist-credentials: true, so credentials are left in the local git config during the job—setwith: persist-credentials: falseon both checkout steps.[
aquasecurity/trivy-action@v0.36.0at line 66 is already version-pinned.]🧰 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, Replace mutable action tags with immutable refs and disable checkout credentials: update the workflow entries for actions/checkout, actions/setup-node, and pnpm/action-setup to use a pinned commit SHA (or other immutable ref) instead of the floating `@v4` tag, and add with: persist-credentials: false to both uses: actions/checkout occurrences so the job does not leave credentials in the local git config; ensure both checkout steps are changed and the other actions (actions/setup-node and pnpm/action-setup) use committed SHAs.apps/main-processor/__tests__/settings.test.ts (1)
5-61: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Add tests for the new settings persistence paths introduced in this PR.
This suite validates
validateSettingswell, but the newgetSettings,saveSettings,writeSettingsAtomically, andrunExclusiveflows are not covered here. Please add targeted tests for read fallback behavior, atomic write failure cleanup, and serialized concurrent saves.As per coding guidelines:
**/*.{test,spec}.{ts,tsx,js,jsx}requires unit tests for all new features and validation viapnpm vitest.🤖 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__/settings.test.ts` around lines 5 - 61, Add unit tests covering the new persistence flows: (1) getSettings: test read fallback by mocking fs read for the primary path to throw and the legacy path to return known JSON and assert returned settings come from legacy; (2) writeSettingsAtomically: simulate a failure during the atomic write (e.g., write to temp throws or rename throws) and assert cleanup occurs (temp file removed and original file unchanged) and that saveSettings surfaces a proper error; (3) runExclusive / saveSettings concurrency: run multiple concurrent saveSettings calls (mocking underlying write to record order and delay) and assert runExclusive serializes them (writes occur one after another, not concurrently); use vi.spyOn or mocked fs/promises functions to control success/failure and cleanup assertions and call validateSettings where appropriate to ensure saved payloads are valid.apps/main-processor/src/export/inlineImage.ts (1)
23-40:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winResolve relative image paths against markdown parents before
realpath.Line 24 resolves
srcimmediately; relative image paths (./img.png,../assets/a.png) are treated relative to process cwd, so valid markdown-local images can fail to inline and exports may stay non-self-contained.Suggested fix
-import { dirname } from 'node:path'; +import { dirname, isAbsolute, resolve } from 'node:path'; @@ - const imagePath = normaliseImagePath(src); - const resolveImagePath = await realpath(imagePath); - let isAllowed = false; - for (const root of allowedFolderRoots) { - if (isPathInside(resolveImagePath, root)) { - isAllowed = true; - break; - } - } - if (!isAllowed) { - for (const file of allowedMarkdownFiles) { - const parentDir = dirname(file); - if (isPathInside(resolveImagePath, parentDir)) { - isAllowed = true; - break; - } - } - } + const normalizedSrc = normaliseImagePath(src); + const candidates = new Set<string>(); + if (normalizedSrc.startsWith('file://') || isAbsolute(normalizedSrc)) { + candidates.add(normalizedSrc); + } else { + for (const file of allowedMarkdownFiles) { + candidates.add(resolve(dirname(file), normalizedSrc)); + } + } + + let resolveImagePath: string | null = null; + for (const candidate of candidates) { + try { + const resolved = await realpath(candidate); + const allowedByRoot = allowedFolderRoots.some((root) => isPathInside(resolved, root)); + const allowedByMdParent = allowedMarkdownFiles.some((file) => + isPathInside(resolved, dirname(file)) + ); + if (allowedByRoot || allowedByMdParent) { + resolveImagePath = resolved; + break; + } + } catch {} + } - if (!isAllowed) continue; - const data = await readFile(resolveImagePath); - const mimeType = getImage(resolveImagePath); + if (!resolveImagePath) continue; + const data = await readFile(resolveImagePath); + const mimeType = getImage(resolveImagePath);Based on learnings: "HTML export should embed local images where possible".
🤖 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/inlineImage.ts` around lines 23 - 40, The code resolves src immediately with normaliseImagePath and realpath (creating resolveImagePath) which treats relative image paths as relative to the process cwd; instead, detect when src is a relative path and, before calling realpath, resolve it against the markdown file's directory (use dirname(file) for each allowedMarkdownFiles entry or the specific markdown file context you have) so that ./ and ../ image references are resolved relative to the markdown parent; then call realpath on that joined path and continue the existing allowedFolderRoots / allowedMarkdownFiles checks using resolveImagePath, ensuring local markdown images are correctly recognized for inlining (update references in normaliseImagePath/realpath usage and any place that sets resolveImagePath).apps/main-processor/src/file.ts (1)
47-77:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard callbacks against stale watcher instances.
At Line 51 and Line 75, callbacks call
unWatchFile(filePath)using only the path key. If a newer watcher has already replaced that key, a stale callback can close the newer watcher or emit staleonChange.Proposed patch
const watcher = chokidar.watch(filePath, { @@ currentWatchers.set(filePath, watcher); + const isCurrentWatcher = (): boolean => currentWatchers.get(filePath) === watcher; @@ watcher.once('error', (error) => { + if (!isCurrentWatcher()) return; clearTimeout(timeout); const watcherError = error instanceof Error ? error : new Error(String(error)); @@ watcher.on('change', () => { + if (!isCurrentWatcher()) return; const existingTimer = debounceTimers.get(filePath); @@ const timer = setTimeout(() => { - if (!currentWatchers.has(filePath)) { + if (!isCurrentWatcher()) { debounceTimers.delete(filePath); return; } @@ watcher.on('unlink', () => { + if (!isCurrentWatcher()) return; void unWatchFile(filePath) .catch(() => {}) .finally(() => onDeleted?.()); });🤖 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/file.ts` around lines 47 - 77, The error is that error/change/unlink callbacks use only filePath and may act on stale watchers; fix by capturing the local watcher instance (or a unique token) when registering handlers and, inside each callback (the 'error' handler that calls unWatchFile(filePath), the 'change' timer callback that calls onChange(), and the 'unlink' handler that calls unWatchFile(filePath) / onDeleted()), first verify that currentWatchers.get(filePath) === watcher (or token) before proceeding; if it does not match, bail out (clear any local timers as needed) so you never close or signal for a newer watcher instance. Ensure you reference the existing symbols watcher, unWatchFile, currentWatchers, debounceTimers, onChange, and onDeleted when implementing the guard.apps/main-processor/src/ipc.ts (1)
124-126:
⚠️ Potential issue | 🟠 Major | ⚡ Quick win
READ_FOLDERcurrently self-authorizes arbitrary roots.At Line 124-Line 126, the handler resolves any provided folder and immediately adds it to
allowedFolderRoots. That makes theSEARCH_FOLDERauthorization gate ineffective after oneREAD_FOLDERcall.Proposed patch
ipcMain.handle(IPC_CONSTANTS.READ_FOLDER, async (event, folderPath: string) => { @@ const safeFolderPath = await resolveDirectoryPath(folderPath); - allowedFolderRoots.add(safeFolderPath); + const isAllowed = Array.from(allowedFolderRoots).some( + (root) => safeFolderPath === root || safeFolderPath.startsWith(`${root}${sep}`) + ); + if (!isAllowed) { + throw new Error('Folder path is not authorized'); + } return await getFolder(safeFolderPath); });Based on learnings: Limit IPC exposure to only necessary communication channels.
🤖 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/ipc.ts` around lines 124 - 126, The READ_FOLDER handler currently resolves the incoming folderPath with resolveDirectoryPath and unconditionally adds it to allowedFolderRoots then returns getFolder(safeFolderPath); remove the self-authorization step: do not call allowedFolderRoots.add(safeFolderPath) in the READ_FOLDER flow; instead validate that safeFolderPath is already in allowedFolderRoots (or perform an explicit authorization check) before calling getFolder(safeFolderPath), and ensure only authorized paths (per SEARCH_FOLDER rules) are returned—update the READ_FOLDER handler to check allowedFolderRoots and deny/throw if the path is not allowed.apps/main-processor/src/settings/get-settings.ts (1)
12-17:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid dropping all persisted settings when one unknown key is present.
At Line [13],
validateSettings(parsed)throws on unknown keys; then Line [16] catches and returnsDEFAULT_SETTINGS, which discards otherwise valid persisted preferences.Suggested fix
import { validateSettings, getSettingsPath } from '../utils/helper/setting-helper'; +import { SETTINGS_KEYS } from '../utils/constants/setting-constants'; ... const parsed = JSON.parse(data); if (parsed && typeof parsed === 'object' && !Array.isArray(parsed)) { - return { ...DEFAULT_SETTINGS, ...validateSettings(parsed) }; + const filtered = Object.fromEntries( + Object.entries(parsed).filter(([key]) => SETTINGS_KEYS.has(key as keyof AppSettings)) + ) as Partial<AppSettings>; + return { ...DEFAULT_SETTINGS, ...validateSettings(filtered) }; }🤖 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 12 - 17, The current code returns DEFAULT_SETTINGS if validateSettings(parsed) throws for unknown keys, discarding all other valid persisted preferences; fix by catching validation errors for unknown keys and instead merge defaults with only the known/allowed keys from parsed. Concretely: in the get-settings logic around parsed/validateSettings, replace the unconditional catch-return DEFAULT_SETTINGS with code that builds a sanitized object by picking keys from parsed that exist in DEFAULT_SETTINGS (or run validateSettings on that filtered object), then return { ...DEFAULT_SETTINGS, ...sanitizedValidatedSettings }; reference validateSettings, parsed, and DEFAULT_SETTINGS to locate where to change.apps/main-processor/src/updater.ts (1)
13-19:
⚠️ Potential issue | 🟠 Major | ⚡ Quick win🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail # List the file if present ls -la apps/main-processor/src/updater.ts 2>/dev/null || true # Print the relevant parts of the file with line numbers sed -n '1,220p' apps/main-processor/src/updater.ts | cat -n # Search for setupAutoUpdater usage and any safe send helper rg -n "setupAutoUpdater|autoUpdater\.on\('update-(available|downloaded)'" apps/main-processor/src/updater.ts apps/main-processor/src -S || true # Search for any existing window lifecycle guards / safe send patterns rg -n "webContents\.isDestroyed\(\)|window\.isDestroyed\(\)|safeSend\(|isDestroyed\(\)\s*\|\|" apps/main-processor/src -S || true # Show IPC_CONSTANTS definition if referenced rg -n "IPC_CONSTANTS" apps/main-processor/src -S || trueRepository: mindfiredigital/markdown-reader
Length of output: 4233
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "---- index.ts (relevant lines) ----" sed -n '1,120p' apps/main-processor/src/index.ts | cat -n echo echo "---- ipc.ts (top + any send helpers) ----" sed -n '1,240p' apps/main-processor/src/ipc.ts | cat -n echo echo "---- search for safe send / destroyed guards in main-processor ----" rg -n "safeSend|isDestroyed\(|webContents\.isDestroyed\(|try\s*\{[^}]*send|catch\s*\([^)]*\)\s*\{\s*//" apps/main-processor/src -S || trueRepository: mindfiredigital/markdown-reader
Length of output: 13012
Guard auto-updater IPC sends against destroyed BrowserWindow/webContents.
apps/main-processor/src/updater.ts sends towindow.webContentsdirectly fromautoUpdatercallbacks with nowindow.isDestroyed()/webContents.isDestroyed()check; since the app stays alive on macOS after window close, these events can fire after destruction and cause runtime failures.Suggested fix
export function setupAutoUpdater(window: BrowserWindow): void { + const safeSend = (channel: string, ...args: unknown[]) => { + if (window.isDestroyed() || window.webContents.isDestroyed()) return; + window.webContents.send(channel, ...args); + }; + autoUpdater.autoDownload = false; autoUpdater.autoInstallOnAppQuit = true; autoUpdater.removeAllListeners('update-available'); autoUpdater.removeAllListeners('update-downloaded'); autoUpdater.removeAllListeners('error'); autoUpdater.on('update-available', (info: UpdateInfo) => { - window.webContents.send(IPC_CONSTANTS.UPDATE_AVAILABLE, info.version); + safeSend(IPC_CONSTANTS.UPDATE_AVAILABLE, info.version); }); autoUpdater.on('update-downloaded', () => { - window.webContents.send(IPC_CONSTANTS.UPDATE_DOWNLOADED); + safeSend(IPC_CONSTANTS.UPDATE_DOWNLOADED); });📝 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.export function setupAutoUpdater(window: BrowserWindow): void { const safeSend = (channel: string, ...args: unknown[]) => { if (window.isDestroyed() || window.webContents.isDestroyed()) return; window.webContents.send(channel, ...args); }; autoUpdater.autoDownload = false; autoUpdater.autoInstallOnAppQuit = true; autoUpdater.removeAllListeners('update-available'); autoUpdater.removeAllListeners('update-downloaded'); autoUpdater.removeAllListeners('error'); autoUpdater.on('update-available', (info: UpdateInfo) => { safeSend(IPC_CONSTANTS.UPDATE_AVAILABLE, info.version); }); autoUpdater.on('update-downloaded', () => { safeSend(IPC_CONSTANTS.UPDATE_DOWNLOADED); }); }🤖 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/updater.ts` around lines 13 - 19, autoUpdater callbacks currently call window.webContents.send(...) unguarded which can throw if the BrowserWindow or its webContents are destroyed; update the 'update-available' and 'update-downloaded' handlers to first verify the window and webContents are not destroyed (e.g. check window && !window.isDestroyed() and window.webContents && !window.webContents.isDestroyed()) before calling window.webContents.send with IPC_CONSTANTS.UPDATE_AVAILABLE / IPC_CONSTANTS.UPDATE_DOWNLOADED (or bail out if destroyed); ensure you reference the same autoUpdater, UpdateInfo, window, and IPC_CONSTANTS symbols used in the diff.apps/main-processor/src/utils/constants/ipc-validation.ts (1)
28-28:
⚠️ Potential issue | 🟠 Major | ⚡ Quick win🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail # Locate and inspect the IPC validation file fd -t f "ipc-validation.ts" -i . || true FILE="apps/main-processor/src/utils/constants/ipc-validation.ts" if [ -f "$FILE" ]; then echo "=== $FILE (head) ===" sed -n '1,120p' "$FILE" | nl -ba else echo "Missing $FILE" fi # Find usages of validateSender echo "=== Usages of validateSender ===" rg -n "validateSender" -S apps/main-processor || true # Inspect any Electron renderer URL usage in index/dev path echo "=== Search ELECTRON_RENDERER_URL ===" rg -n "ELECTRON_RENDERER_URL" -S apps/main-processor || true # Inspect apps/main-processor/src/index.ts if it exists IDX="apps/main-processor/src/index.ts" if [ -f "$IDX" ]; then echo "=== $IDX (head/tail) ===" sed -n '1,220p' "$IDX" | nl -ba else echo "Missing $IDX" fi # Search for IPC message handling that uses this validation echo "=== Search for ipc / sender validation patterns ===" rg -n "(ipc|sender|webContents|origin|parsedUrl|validate)" -S apps/main-processor/src || trueRepository: mindfiredigital/markdown-reader
Length of output: 269
🏁 Script executed:
#!/bin/bash set -euo pipefail FILE="apps/main-processor/src/utils/constants/ipc-validation.ts" if [ -f "$FILE" ]; then echo "=== $FILE ===" sed -n '1,200p' "$FILE" | cat -n else echo "Missing $FILE" fi echo "=== Usages of validateSender ===" rg -n "validateSender" -S apps/main-processor || true echo "=== Search ELECTRON_RENDERER_URL ===" rg -n "ELECTRON_RENDERER_URL" -S apps/main-processor || true IDX="apps/main-processor/src/index.ts" if [ -f "$IDX" ]; then echo "=== $IDX (head/tail) ===" { sed -n '1,260p' "$IDX" | cat -n; } || true else echo "Missing $IDX" fi echo "=== Search ipc message handling that references sender/origin ===" rg -n "(sender|origin|parsedUrl|webContents|ipcMain|ipcRenderer|MessagePort|validate)" -S apps/main-processor/src || trueRepository: mindfiredigital/markdown-reader
Length of output: 18170
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect ipc handlers to understand threat model and usage expectations echo "=== apps/main-processor/src/ipc.ts ===" sed -n '1,260p' apps/main-processor/src/ipc.ts | cat -n # Inspect tests for validateSender echo "=== apps/main-processor/__tests__/ipc.test.ts ===" sed -n '1,120p' apps/main-processor/__tests__/ipc.test.ts | cat -n # Inspect PATHS.RENDERER_HTML definition echo "=== apps/main-processor/src/utils/constants/path-constants.ts ===" # file name might differ; locate path-constants fd -t f "path-constants.ts" apps/main-processor/src/utils/constants || true for f in $(fd -t f "path-constants.ts" apps/main-processor/src/utils/constants); do echo "--- $f ---" sed -n '1,200p' "$f" | cat -n doneRepository: mindfiredigital/markdown-reader
Length of output: 12688
Restrict
validateSenderto the configured Electron renderer URL (dev), not anyhttp://localhost
apps/main-processor/src/utils/constants/ipc-validation.tscurrently treats anyhttp://localhostsender as trusted (line 28). In dev, the renderer is loaded fromprocess.env.ELECTRON_RENDERER_URL(apps/main-processor/src/index.ts), so IPC trust should be limited to that exact origin/path (and fail closed when it’s missing) rather than trusting arbitrary localhost pages.Suggested fix
- return parsedUrl.protocol === 'http:' && parsedUrl.hostname === 'localhost'; + if (parsedUrl.protocol === 'http:' || parsedUrl.protocol === 'https:') { + const configured = process.env.ELECTRON_RENDERER_URL; + if (!configured) return false; + const expected = new URL(configured); + return ( + parsedUrl.origin === expected.origin && + parsedUrl.pathname === expected.pathname + ); + } + return 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.if (parsedUrl.protocol === 'http:' || parsedUrl.protocol === 'https:') { const configured = process.env.ELECTRON_RENDERER_URL; if (!configured) return false; const expected = new URL(configured); return ( parsedUrl.origin === expected.origin && parsedUrl.pathname === expected.pathname ); } return false;🤖 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/utils/constants/ipc-validation.ts` at line 28, The current validateSender logic trusts any http://localhost origin; update validateSender to require an exact match against the configured renderer URL: if process.env.ELECTRON_RENDERER_URL is missing return false (fail closed), otherwise compute the origin of process.env.ELECTRON_RENDERER_URL (via new URL(...).origin) and only return true when the parsed sender origin equals that renderer origin; reference the validateSender function and process.env.ELECTRON_RENDERER_URL when making this change.apps/renderer/__tests__/renderer/markdown.test.ts (1)
47-57:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winSecurity assertions are inverted for dangerous HTML/event-handler cases.
These tests currently require
<iframe>tags and inlineonerrorhandlers to survive output, which locks in XSS-unsafe behavior and can block sanitization hardening.Suggested test expectation fix
- it('should not be able to remove iframe tags before sanitization', async () => { + it('removes iframe tags from rendered output', async () => { const html = await renderMarkdown(`<iframe src="https://example.com"></iframe>`); - expect(html).toContain('<iframe'); + expect(html).not.toContain('<iframe'); }); - it('it should not be able to remove dangerous inline event handlers before dom purification', async () => { + it('removes dangerous inline event handlers from rendered output', async () => { const html = await renderMarkdown(` <img src="x" onerror="alert('xss')" /> `); - expect(html).toContain('onerror'); + expect(html).not.toContain('onerror'); });Based on learnings: "Sanitize user-rendered Markdown content before rendering using DOMPurify" and "Filter dangerous HTML elements and event handlers during Markdown sanitization".
🤖 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 - 57, The tests in apps/renderer/__tests__/renderer/markdown.test.ts currently assert that dangerous HTML survives rendering (using renderMarkdown), which is inverted; update the two assertions for the iframe and inline event-handler cases to assert absence instead of presence (use expect(html).not.toContain('<iframe') and expect(html).not.toContain('onerror') or equivalent) so the tests verify that renderMarkdown sanitizes/removes dangerous elements/attributes (the two affected specs reference the iframe case and the onerror img case).apps/renderer/setup.ts (1)
25-37:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winGlobal mock settings state can leak across tests and cause flakiness.
mockSettingsis shared for the entire run, andgetSettingsreturns the same object reference. A single test can mutate state and affect unrelated tests.Suggested deterministic mock setup
-import { vi } from 'vitest'; +import { vi, beforeEach } from 'vitest'; @@ -let mockSettings = { +const DEFAULT_SETTINGS = { fontSize: 16, readingWidth: 'default', customCss: '', }; +let mockSettings = { ...DEFAULT_SETTINGS }; + +beforeEach(() => { + mockSettings = { ...DEFAULT_SETTINGS }; +}); @@ - getSettings: vi.fn(async () => mockSettings), + getSettings: vi.fn(async () => ({ ...mockSettings })), saveSettings: vi.fn(async (partial) => { mockSettings = { ...mockSettings, ...partial }; - return mockSettings; + return { ...mockSettings }; }),As per coding guidelines: "Write unit tests for all new features to maintain code quality ... ensuring all tests pass via
pnpm vitest."🤖 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/setup.ts` around lines 25 - 37, The global mutable mockSettings is shared across tests causing leaks; update the window.api mock so getSettings returns a fresh copy (e.g., deep/shallow clone) instead of the original object and ensure saveSettings updates an internal immutable state correctly; additionally expose or implement a reset function (or reinitialize the internal state in a beforeEach) so mockSettings is reset to the initial defaults between tests—locate the mock in setup.ts where mockSettings, getSettings, saveSettings and window.api are defined and change their implementations to return/cloned objects and support resetting.apps/renderer/src/config/marked.ts (1)
9-24:
⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Verify the shared-state pattern behind the risk. rg -n "let currentRegistry|let instance|getMarkdown\\(|currentRegistry = registry|if \\(instance\\) return instance|heading\\(props\\)" apps/renderer/src/config/marked.ts rg -n "await marked\\.parse\\(" apps/renderer/src/renderer/markdown.tsRepository: mindfiredigital/markdown-reader
Length of output: 376
Fix race condition risk from singleton
Marked+ module-level heading registry.
apps/renderer/src/config/marked.tsoverwrites module-levelcurrentRegistrywhile reusing a shared singletoninstance, andrenderer.heading()readscurrentRegistry; withawait marked.parse(...)inapps/renderer/src/renderer/markdown.ts, overlapping parses can generate heading IDs/anchors against the wrong registry. Make the registry part of the per-parse renderer (no module-level mutable state) or avoid sharing a singletonMarkedacross calls.🤖 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/config/marked.ts` around lines 9 - 24, The module-level mutable currentRegistry causes a race when reusing the singleton Marked instance; change getMarkdown so the registry is stored per-renderer instead of a module global: either create a new Marked each call (avoid sharing the singleton instance) or attach the provided registry to the Marked object returned (e.g., instance.registry = registry) and update the renderer.heading closure to read the registry from that instance (use this or instance reference) rather than from currentRegistry; update references to currentRegistry and remove the module-level variable currentRegistry to eliminate the race between getMarkdown, Marked, and the heading renderer.apps/renderer/src/hooks/useExport.ts (1)
6-31:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle export IPC failures to avoid unhandled rejections.
The async export flows call external IPC/file operations without error handling. A rejected dialog/export call will bubble as an unhandled promise rejection on user action.
🤖 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, The export handlers exportHtml, exportPdf, and exportDocx currently call window.api.showSaveDialog and window.api.exportHTML/exportPDF/exportDOCX without error handling; wrap the body of each useCallback in a try/catch that awaits the IPC calls and catches any rejection, handle errors by logging via console.error (or processLogger) and optionally showing a user-facing alert/dialog, and ensure the catch returns/cleans up to avoid unhandled promise rejections when showSaveDialog or the export IPC rejects.apps/renderer/src/hooks/useFileActions.ts (1)
9-41:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard IPC file/folder actions with explicit error handling.
openFolder,openFileDialog, and downstreamloadFileInTabcalls currently rely on promise chains without catch/try handling. Any IPC/read failure can become an unhandled rejection.🤖 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 9 - 41, Wrap all IPC and file-load async calls in explicit try/catch blocks to prevent unhandled rejections: in openFolder, catch errors from window.api.openFolderDialog() and window.api.readFolder() and handle/log them (and avoid setting state on failure); in openFileDialog, catch errors from window.api.openFileDialog() and from downstream loadFileInTab invocation; and inside loadFileInTab wrap the await loadFile(path) call in try/catch and handle/log loadFile errors before dispatching the OPEN_TAB action. Use the existing function names openFolder, openFileDialog, loadFileInTab and the IPC calls window.api.openFolderDialog, window.api.readFolder, window.api.openFileDialog and loadFile to locate where to add the try/catch and error handling.apps/renderer/src/hooks/useFilePersistence.ts (1)
37-55:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle watcher reload failures inside the debounced callback.
loadFile(...)is awaited inside an async timeout callback withouttry/catch. A rejection here can surface as an unhandled promise rejection and leave refresh flow partially applied.Suggested fix
debounceTimer.current = window.setTimeout(async () => { const currentScroll = contentRef.current?.scrollTop ?? 0; - const result = await loadFile(activeTab.filePath); + let result; + try { + result = await loadFile(activeTab.filePath); + } catch { + return; + } if (!result || !isMounted.current) return; dispatch({ type: 'UPDATE_TAB_STATE',🤖 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 37 - 55, The debounced async callback in useFilePersistence calls await loadFile(activeTab.filePath) without error handling, risking unhandled rejections and partial UI updates; wrap the loadFile call and subsequent logic in a try/catch inside the debounceTimer.current callback (catching errors from loadFile and any downstream awaits), on error abort the update flow (don’t call dispatch/scroll/ setShowToast) and surface the failure (e.g., log or set an error state/toast) so the watcher reload failure is safely handled; update the async timeout callback that references loadFile, contentRef, dispatch, and setShowToast accordingly.apps/renderer/src/hooks/useOpenFilePath.ts (1)
5-7:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle
loadFileInTabrejections in the event callback.
void loadFileInTab(path)drops Promise errors; a failed load can surface as an unhandled rejection.Suggested fix
window.api.onOpenFilePath((path) => { - void loadFileInTab(path); + void loadFileInTab(path).catch((error) => { + console.error('Failed to open file path event:', error); + }); });🤖 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/useOpenFilePath.ts` around lines 5 - 7, The onOpenFilePath callback currently calls void loadFileInTab(path) which silences Promise rejections; change the event handler to handle errors from loadFileInTab (the handler passed to window.api.onOpenFilePath). Wrap the call in an async function and await loadFileInTab(path) with try/catch, or call loadFileInTab(path).catch(err => { /* log or handle error */ }), so any rejection is caught and logged/handled rather than becoming an unhandled promise rejection.apps/renderer/src/hooks/useSettings.ts (1)
58-68:
⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftFont-size updates are race-prone under rapid repeated actions.
Line 60 and Line 66 derive new values from captured
settings.fontSize; back-to-back calls can overwrite each other and persist an incorrect value.Suggested direction
- const updateSettings = useCallback(async (partial: Partial<AppSettings>) => { + const updateSettings = useCallback( + async (partialOrUpdater: Partial<AppSettings> | ((current: AppSettings) => Partial<AppSettings>)) => { + const partial = + typeof partialOrUpdater === 'function' + ? partialOrUpdater(settings) + : partialOrUpdater; // existing save logic... - }, []); + }, + [settings] + ); - const increaseFontSize = useCallback(() => { - return updateSettings({ - fontSize: Math.min(FONT_SIZE.MAX, settings.fontSize + FONT_SIZE.INCREMENT), - }); - }, [settings.fontSize, updateSettings]); + const increaseFontSize = useCallback(() => { + return updateSettings((current) => ({ + fontSize: Math.min(FONT_SIZE.MAX, current.fontSize + FONT_SIZE.INCREMENT), + })); + }, [updateSettings]); - const decreaseFontSize = useCallback(() => { - return updateSettings({ - fontSize: Math.max(FONT_SIZE.MIN, settings.fontSize - FONT_SIZE.INCREMENT), - }); - }, [settings.fontSize, updateSettings]); + const decreaseFontSize = useCallback(() => { + return updateSettings((current) => ({ + fontSize: Math.max(FONT_SIZE.MIN, current.fontSize - FONT_SIZE.INCREMENT), + })); + }, [updateSettings]);🤖 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 58 - 68, increaseFontSize and decreaseFontSize compute new fontSize from the captured settings.fontSize, causing races on rapid repeated calls; change each to call updateSettings with a functional updater that receives the previous settings and computes fontSize as Math.min(FONT_SIZE.MAX, prev.fontSize + FONT_SIZE.INCREMENT) (and Math.max for decrease) so updates are based on the latest state, and then remove settings.fontSize from the useCallback dependency arrays; target the increaseFontSize and decreaseFontSize functions and the updateSettings call-site to implement this functional update pattern.apps/renderer/src/styles/export.css (1)
7-7:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winExport CSS hardcodes light-mode colors, which can break active-theme export fidelity.
Using fixed
background:#ffffffand `color: `#111827forces light styling in exported HTML instead of preserving the active theme.Based on learnings: "HTML export should inline the active styles" and "Implement export to self-contained HTML preserving theme, typography, code blocks, math, diagrams, and images."
Also applies to: 14-14
🤖 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/styles/export.css` at line 7, The export CSS currently hardcodes light-mode colors (background: `#ffffff` and color: `#111827`) which forces light styling in exported HTML; update the export flow so export.css does not use fixed hex values but instead uses theme-aware CSS variables (e.g., background: var(--bg); color: var(--text)) or, better, during the export process compute and inline the document's current root variables into the exported HTML (replace the background and color declarations in export.css with the active theme's computed values) so the exported self-contained HTML preserves the active theme, typography, code blocks, math, diagrams, and images.docs/src/components/Homepage/hero.tsx (1)
49-53:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid nested interactive elements in CTA controls.
Line 49 and Line 55 nest
<button>inside link elements, which is invalid and can break accessibility/keyboard behavior. Use the link elements themselves as the styled interactive controls.Proposed fix
- <Link to="/docs/installation" className="no-underline"> - <button className="inline-flex items-center justify-center bg-primary hover:bg-primary/90 text-white font-medium h-11 px-6 text-sm rounded-xl shadow-sm cursor-pointer transition-colors duration-200 border-0"> + <Link + to="/docs/installation" + className="no-underline inline-flex items-center justify-center bg-primary hover:bg-primary/90 text-white font-medium h-11 px-6 text-sm rounded-xl shadow-sm cursor-pointer transition-colors duration-200 border-0" + > Get Started <ArrowRight className="ml-1.5 h-4 w-4" strokeWidth={2.5} /> - </button> </Link> @@ - <a + <a href="https://github.com/mindfiredigital/markdown-reader" - className="no-underline" + className="no-underline 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" 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> + <GitBranchIcon className="mr-1.5 h-4 w-4" strokeWidth={2.5} /> + GitHub </a>Also applies to: 55-65
🤖 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 49 - 53, The CTA currently nests a <button> inside a <Link>, which is invalid and harms accessibility; replace the nested button with the Link itself as the interactive element by moving the button's classes and attributes onto the <Link> (e.g., set className="inline-flex items-center justify-center bg-primary ..." and include aria-label or role="button" if desired) and render the text and <ArrowRight/> directly inside the <Link>; make the same change for the other CTA instance that wraps lines 55-65 so no <button> elements are nested inside <Link>.package.json (1)
69-74:
⚠️ Potential issue | 🟠 Major | ⚡ Quick win🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "Checking override declarations in package.json..." rg -n '"pnpm"|"overrides"|"(serialize-javascript|fast-uri|tmp)"' package.json -C1 LOCKFILE="$(fd -a '^pnpm-lock\.yaml$' | head -n1 || true)" if [[ -z "${LOCKFILE}" ]]; then echo "pnpm-lock.yaml not found; cannot verify resolved transitive versions." exit 0 fi echo "" echo "Inspecting resolved versions for overridden packages in ${LOCKFILE}..." rg -n '\/(serialize-javascript|fast-uri|tmp)@' "${LOCKFILE}" -A2Repository: mindfiredigital/markdown-reader
Length of output: 498
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "Re-checking pnpm.overrides in package.json..." rg -n '"pnpm"\s*:\s*\{' -n package.json -C1 || true rg -n '"overrides"\s*:' package.json -C2 || true rg -n '"(serialize-javascript|fast-uri|tmp)"\s*:\s*"' package.json -C0 LOCKFILE="$(fd -a '^pnpm-lock\.yaml$' | head -n1 || true)" echo "LOCKFILE=$LOCKFILE" if [[ -z "${LOCKFILE}" ]]; then echo "pnpm-lock.yaml not found; cannot verify resolved transitive versions." exit 0 fi echo "" echo "Searching pnpm-lock.yaml for resolved versions..." rg -n '(serialize-javascript|fast-uri|tmp)@' "${LOCKFILE}" -n -C1 | head -n 200 echo "" echo "Searching pnpm-lock.yaml for any override-specific blocks (optional)..." rg -n 'overrides:' "${LOCKFILE}" -C1 | head -n 50Repository: mindfiredigital/markdown-reader
Length of output: 1237
Pin
pnpm.overridesto exact patched versions inpackage.json
package.jsonuses open-ended>=ranges forserialize-javascript,fast-uri, andtmp—this can drift when the lockfile is regenerated.- Current
pnpm-lock.yamlresolutions areserialize-javascript@7.0.5,fast-uri@3.1.2, andtmp@0.2.7; setpnpm.overridesto those exact versions instead of the>=ranges.- Keep monitoring Dependabot/security alerts for these 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 `@package.json` around lines 69 - 74, Update the pnpm.overrides entry in package.json to pin exact patched versions instead of open-ended ranges: replace the "serialize-javascript": ">=7.0.5", "fast-uri": ">=3.1.2", and "tmp": ">=0.2.6" entries under pnpm.overrides with the exact versions found in the lockfile (e.g., "serialize-javascript": "7.0.5", "fast-uri": "3.1.2", "tmp": "0.2.7") so that pnpm.overrides uses fixed versions; modify the package.json pnpm.overrides block accordingly and then regenerate/verify the lockfile to ensure resolutions match.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR fixes arbitary file read in export , adds css sanitization tests, and fixes the marked instance recreation.
Type of Change
Related Issue
Closes #93, #94, #95
Checklist
Summary by CodeRabbit
New Features
Improvements