Fix/system hardening#78
Open
Ashminita28 wants to merge 37 commits into
Open
Conversation
|
Warning Review limit reached
More reviews will be available in 18 minutes and 4 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
WalkthroughThis PR adds typed settings persistence (IPC + preload), a settings UI, rewrites TOC extraction to use marked tokens and threads TOC through store/hooks/UI, hardens file/folder watchers (debounce, depth, cycle detection), extends tests, and adds CI/packaging updates. ChangesMain Processor: File Monitoring, Settings, and IPC
Renderer: TOC Extraction, Data Threading, and UI
Shared Types and Constants
Config and CI
Sequence Diagram (high-level: Settings save/load flow) sequenceDiagram
participant Renderer
participant Preload
participant Main
participant FS
Renderer->>Preload: window.api.saveSettings(partial)
Preload->>Main: ipc.invoke('SAVE_SETTINGS', partial)
Main->>FS: saveSettings(mergedSettings)
FS-->>Main: write result
Main-->>Preload: return AppSettings
Preload-->>Renderer: resolved AppSettings
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/main-processor/src/index.ts (2)
92-92:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd error handling for the async call.
sendFilePathToRendereris now async and can throw errors during path resolution. This call site in thesecond-instancehandler lacks error handling, which could result in unhandled promise rejections. Line 76 demonstrates the correct pattern with.catch().🛡️ Proposed fix to add error handling
mainWindow.focus(); - sendFilePathToRenderer(filePath); + void sendFilePathToRenderer(filePath).catch((error) => { + console.error('Failed to open file from second instance:', error); + }); } else {🤖 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 92, The call to sendFilePathToRenderer inside the second-instance handler can reject but is invoked without handling; update the second-instance handler to handle promise rejections the same way as the pattern at the earlier handler (e.g., use .catch(...) or await inside try/catch). Specifically, locate the second-instance handler where sendFilePathToRenderer(filePath) is called and wrap it with a .catch that logs the error via the existing logger or processLogger (or convert the handler to async and use try { await sendFilePathToRenderer(filePath) } catch (err) { log the error }). Ensure you reference sendFilePathToRenderer and the second-instance handler so the call site is protected from unhandled rejections.
65-65:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd error handling for the async call.
sendFilePathToRendereris now async and can throw errors during path resolution (e.g., invalid path, disallowed extension). This call site lacks error handling, which could result in unhandled promise rejections. Line 76 demonstrates the correct pattern with.catch().🛡️ Proposed fix to add error handling
const filePathToOpen = pendingFilePath ?? cliFilePath; if (filePathToOpen) { - sendFilePathToRenderer(filePathToOpen); + void sendFilePathToRenderer(filePathToOpen).catch((error) => { + console.error('Failed to open file on startup:', 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, The call to sendFilePathToRenderer is asynchronous and can reject; wrap the call in proper error handling (either await inside a try/catch or append .catch(...)) so you do not produce unhandled promise rejections. Update the invocation of sendFilePathToRenderer(filePathToOpen) to mirror the existing pattern used at the other site (line 76) by logging the error (using the same logger or console.error) and handling any cleanup or user-notification needed when resolution fails. Ensure you reference the sendFilePathToRenderer function name so reviewers can find the change.
🧹 Nitpick comments (3)
apps/main-processor/src/file.ts (1)
59-62: ⚡ Quick winPreserve the original error as cause when normalizing non-Error values.
When converting a non-Error value to an Error instance, the original value is lost. Preserving it as
causeaids debugging.📋 Suggested improvement
- const watcherError = error instanceof Error ? error : new Error(String(error)); + const watcherError = error instanceof Error ? error : new Error(String(error), { cause: 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/file.ts` around lines 59 - 62, The error normalization in the watcher error handler currently wraps non-Error values with new Error(String(error)) which loses the original value; update the handler so when error is not an instance of Error you create the wrapper preserving the original as the cause (e.g., new Error(String(error), { cause: error }) or assign watcherError.cause = error for environments without Error cause support), then pass that watcherError into onError after unWatchFile resolves; refer to the watcher.on('error' ...) callback, the watcherError variable, unWatchFile(filePath) call, and the onError callback when making this change.apps/main-processor/tests/folder.test.ts (2)
8-21: ⚡ Quick winAdd cleanup for the temporary test directory.
The test creates a temporary directory but does not clean it up afterward, which can leave test artifacts on the file system.
🧹 Proposed fix to add cleanup using afterEach
+import { describe, expect, it, afterEach } from 'vitest'; import { mkdtemp, mkdir, writeFile } from 'node:fs/promises'; +import { rm } from 'node:fs/promises'; import { join } from 'node:path'; import { tmpdir } from 'node:os'; import { getFolder } from '../src/folder'; +let testRoot: string | null = null; + describe('folder reader testing', () => { + afterEach(async () => { + if (testRoot) { + await rm(testRoot, { recursive: true, force: true }); + testRoot = null; + } + }); + it('should enforce the recursion depth', async () => { - const root = await mkdtemp(join(tmpdir(), 'markdown-reader-folder-')); + testRoot = await mkdtemp(join(tmpdir(), 'markdown-reader-folder-')); + const root = testRoot; let current = root;🤖 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 8 - 21, The test creates a temporary directory in the variable root (via mkdtemp) and never removes it; add an afterEach cleanup that checks for the created root and removes it recursively (e.g., using fs.promises.rm or rimraf) to delete the temp tree after the test; locate the test in apps/main-processor/tests/folder.test.ts (symbols: mkdtemp, root, getFolder) and add an afterEach block that safely removes root if defined, swallowing or logging any removal errors to avoid failing the test run.
7-7: ⚡ Quick winRemove
asyncfrom thedescribecallback (unnecessary here). (apps/main-processor/tests/folder.test.ts:7)
Vitest supports asyncdescribecallbacks, but this suite-definition callback doesn’t await anything—it's only defining theitblock—so droppingasyncavoids confusion.🔧 Proposed fix
-describe('folder reader testing', async () => { +describe('folder reader testing', () => {🤖 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` at line 7, The describe callback for the suite "folder reader testing" is marked async but doesn't await anything; remove the unnecessary async keyword from the describe callback declaration (the describe('folder reader testing', async () => { ... }) should become a plain synchronous callback) so the suite-definition is not misleading—update the describe block that defines the 'folder reader testing' suite accordingly.
🤖 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 `@apps/main-processor/src/folder.ts`:
- Line 13: Wrap the realpath(folderPath) call in a try/catch to handle
filesystem errors from realpath; on failure log or surface a controlled error
(e.g., via the existing logger or by returning/skipping this folder) instead of
letting the exception propagate to the IPC handler. Locate the line with
realpath(folderPath) in folder.ts, catch errors from realpath, and decide to
either return a sensible result (skip this path) or throw a normalized error
object/message that the IPC layer expects so unhandled promise rejections are
avoided.
In `@apps/renderer/src/utils/helpers/heading-helper.ts`:
- Around line 29-31: The TOC slugging diverges because headings are slugged from
raw markdown while headingText(token) normalizes (parses inline and strips
HTML); update the code that generates heading IDs/slugs (the function/method
named heading or any slugging call that uses token.text) to call
headingText(token) first and use its returned normalized string for slugging/ID
generation so rendered heading IDs and TOC anchors match exactly.
---
Outside diff comments:
In `@apps/main-processor/src/index.ts`:
- Line 92: The call to sendFilePathToRenderer inside the second-instance handler
can reject but is invoked without handling; update the second-instance handler
to handle promise rejections the same way as the pattern at the earlier handler
(e.g., use .catch(...) or await inside try/catch). Specifically, locate the
second-instance handler where sendFilePathToRenderer(filePath) is called and
wrap it with a .catch that logs the error via the existing logger or
processLogger (or convert the handler to async and use try { await
sendFilePathToRenderer(filePath) } catch (err) { log the error }). Ensure you
reference sendFilePathToRenderer and the second-instance handler so the call
site is protected from unhandled rejections.
- Line 65: The call to sendFilePathToRenderer is asynchronous and can reject;
wrap the call in proper error handling (either await inside a try/catch or
append .catch(...)) so you do not produce unhandled promise rejections. Update
the invocation of sendFilePathToRenderer(filePathToOpen) to mirror the existing
pattern used at the other site (line 76) by logging the error (using the same
logger or console.error) and handling any cleanup or user-notification needed
when resolution fails. Ensure you reference the sendFilePathToRenderer function
name so reviewers can find the change.
---
Nitpick comments:
In `@apps/main-processor/src/file.ts`:
- Around line 59-62: The error normalization in the watcher error handler
currently wraps non-Error values with new Error(String(error)) which loses the
original value; update the handler so when error is not an instance of Error you
create the wrapper preserving the original as the cause (e.g., new
Error(String(error), { cause: error }) or assign watcherError.cause = error for
environments without Error cause support), then pass that watcherError into
onError after unWatchFile resolves; refer to the watcher.on('error' ...)
callback, the watcherError variable, unWatchFile(filePath) call, and the onError
callback when making this change.
In `@apps/main-processor/tests/folder.test.ts`:
- Around line 8-21: The test creates a temporary directory in the variable root
(via mkdtemp) and never removes it; add an afterEach cleanup that checks for the
created root and removes it recursively (e.g., using fs.promises.rm or rimraf)
to delete the temp tree after the test; locate the test in
apps/main-processor/tests/folder.test.ts (symbols: mkdtemp, root, getFolder) and
add an afterEach block that safely removes root if defined, swallowing or
logging any removal errors to avoid failing the test run.
- Line 7: The describe callback for the suite "folder reader testing" is marked
async but doesn't await anything; remove the unnecessary async keyword from the
describe callback declaration (the describe('folder reader testing', async () =>
{ ... }) should become a plain synchronous callback) so the suite-definition is
not misleading—update the describe block that defines the 'folder reader
testing' suite accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ed99aef6-0656-427c-bf59-d6bae463e38f
📒 Files selected for processing (24)
apps/main-processor/setup.tsapps/main-processor/src/file.tsapps/main-processor/src/folder.tsapps/main-processor/src/index.tsapps/main-processor/src/types/watch-file-types.tsapps/main-processor/src/utils/constants/path-constants.tsapps/main-processor/tests/file.test.tsapps/main-processor/tests/folder.test.tsapps/main-processor/tests/recent.test.tsapps/main-processor/vitest.config.tsapps/renderer/src/App.tsxapps/renderer/src/components/Sidebar.tsxapps/renderer/src/hooks/useCollapsibleToc.tsapps/renderer/src/hooks/useFile.tsapps/renderer/src/hooks/useFileActions.tsapps/renderer/src/hooks/useFilePersistence.tsapps/renderer/src/renderer/toc.tsapps/renderer/src/store/tabStore.tsapps/renderer/src/types/component-types.tsapps/renderer/src/utils/helpers/heading-helper.tsapps/renderer/src/utils/helpers/sidebar-helper.tsapps/renderer/src/utils/helpers/tab-helper.tsxapps/renderer/tests/hooks/useCollapsibleToc.test.tsapps/renderer/tests/renderer/toc.test.ts
…dden for production
There was a problem hiding this comment.
Actionable comments posted: 14
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/export/inlineImage.ts (1)
23-25: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueDead no-op in catch branch.
output.replace(fullMatch, fullMatch)rewrites the string with itself — it does nothing. Drop it (or add a real fallback / log) so the intent is clear.♻️ Suggested cleanup
- } catch { - output = output.replace(fullMatch, fullMatch); - } + } catch { + // leave original <img> untouched if the file can't be inlined + }🤖 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 - 25, The catch block contains a no-op call output.replace(fullMatch, fullMatch) which does nothing; remove that no-op and either delete the empty catch or replace it with a meaningful fallback or logging (e.g., log the error or leave the original output unchanged explicitly). Locate the try/catch in apps/main-processor/src/export/inlineImage.ts around the code using variables fullMatch and output and update the catch to either remove the redundant replace or call the logger with the caught error so intent is clear.apps/renderer/src/context/ThemeProvider.tsx (1)
11-27:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSystem theme ignored on initial load.
getSystemTheme()only runs inside thechangelistener, so on first mount with no savedapp-themethe theme staysgithub-lighteven when the OS prefers dark. Apply the system theme on mount too.🎨 Proposed fix
useEffect(()=>{ const saved=localStorage.getItem('app-theme'); if(saved && APPTHEMES.includes(saved as Theme)){ setTheme(saved as Theme); + } else { + setThemeState(getSystemTheme()); } },[])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/renderer/src/context/ThemeProvider.tsx` around lines 11 - 27, The mount logic never applies the system theme because getSystemTheme() is only called in the media change handler; update the ThemeProvider mount logic so when there is no saved 'app-theme' you call getSystemTheme() and setThemeState(...) on initial mount (e.g., inside the existing first useEffect or at the start of the second useEffect before adding the listener). Ensure you keep the APPTHEMES check for saved values and only apply the system theme when localStorage.getItem('app-theme') is falsy; reference setThemeState, getSystemTheme, APPTHEMES and the 'app-theme' key to locate the relevant code.apps/main-processor/tests/file.test.ts (1)
42-42: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winFile-watcher tests write to cwd and never clean up.
TEST_FILE = './test-file.md'lands in the working directory and is created by these tests (incl. the newdoes not call callback...case at Lines 76-84) but never removed inafterAll, unlike the temp-dir files. This leavestest-file.mdlying around after each run and can flake on parallel/repeat runs.Move it under
test_dirand unlink it during teardown.♻️ Suggested change
- const TEST_FILE = './test-file.md'; + const TEST_FILE = join(test_dir, 'test-file.md');afterAll(() => { unlinkSync(simple_file); unlinkSync(empty_file); unlinkSync(unicode_file); + try { unlinkSync(join(test_dir, 'test-file.md')); } catch { /* may not exist */ } });As per coding guidelines: "Use isolated temp directories for disk tests and clean them up."
🤖 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/file.test.ts` at line 42, Tests create './test-file.md' in the repo root and never remove it; change the TEST_FILE constant in file.test.ts to live inside the existing test_dir (e.g. use path.join(test_dir, 'test-file.md')) and update the teardown in afterAll to unlink that file as part of cleanup so the "does not call callback..." test and others no longer leave artifacts; reference the TEST_FILE constant and the afterAll teardown in your change.
🤖 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:
- Around line 52-53: The workflow uses two different pnpm action majors: update
the pnpm/action-setup usage so both jobs use the same version (either bump the
build job's pnpm/action-setup@v3 to `@v4` or downgrade security's
pnpm/action-setup@v4 to `@v3`) — locate the pnpm/action-setup entries in the build
and security jobs and make them identical to avoid setup drift.
- Around line 67-73: Replace the unpinned action reference
aquasecurity/trivy-action@master with a specific released tag you have validated
(e.g., aquasecurity/trivy-action@vX.Y.Z) and add the Trivy option exit-code: 1
inside the with: block so that CRITICAL/HIGH findings cause the job to fail;
keep existing keys (scan-type: fs, scan-ref: ., severity: CRITICAL,HIGH, format:
table) but ensure the action ref is pinned and exit-code: 1 is present.
In @.github/workflows/production.yml:
- Around line 52-53: The pnpm action versions differ between jobs: the security
job uses "pnpm/action-setup@v4" while the build job uses "`@v3`", causing pnpm
version drift; update the build job's step that uses "pnpm/action-setup" (and
any other jobs using it) so they all use the same pinned action version (e.g.,
change the build job's "uses: pnpm/action-setup@v3" to "uses:
pnpm/action-setup@v4") so both the build and security jobs resolve the same pnpm
and respect the pinned-pnpm CI expectation.
In `@apps/main-processor/src/export/inlineImage.ts`:
- Line 12: The regexp EXPORT_IMAGE_SRC_REGEX captures the src in group 2
(match[2] is the correct src value), so confirm/keep using match[2] where the
inlineImage processing logic reads the source; also remove the redundant no-op
catch that does output = output.replace(fullMatch, fullMatch) — either delete
the catch entirely or replace it with simpler error handling (e.g., log and
continue) in the function that processes matches (refer to
EXPORT_IMAGE_SRC_REGEX, match[0], match[1], match[2], and fullMatch to locate
the code).
In `@apps/main-processor/src/settings/save-settings.ts`:
- Around line 12-19: The saveSettings function currently writes directly to
settingsPath; change it to perform an atomic write: create the directory (keep
use of mkdir(dir, { recursive: true })), write JSON to a temporary file (e.g.,
settingsPath + '.tmp') using writeFile, ensure the temp file is flushed to disk
(open+fsync) then atomically rename the temp file over settingsPath with rename,
and on any error unlink the temp file; also add a simple in-memory mutex/flag
around saveSettings to serialize concurrent calls so two invocations cannot
interleave. Reference the existing symbols settingsPath, nextSettings,
saveSettings, mkdir, writeFile, and add use of rename/unlink (and fsync via
open/sync) in the implementation and ensure errors are re-thrown with the
original cause.
In `@apps/main-processor/src/utils/helper/setting-helper.ts`:
- Around line 8-14: The getSettingsPath function should not silently fall back
to process.cwd(); update getSettingsPath to stop using process.cwd() as a
fallback: either let the original error propagate (so failures in
app.getPath('userData') surface) or catch the error, log it, and use a safe
fallback such as app.getPath('temp') (or an in-memory mode) instead of
process.cwd(); modify the try/catch around app.getPath('userData') in
getSettingsPath to implement one of these approaches and ensure the error is
logged when the fallback is used.
In `@apps/main-processor/tests/export.test.ts`:
- Around line 58-66: The test creates a temporary directory with mkdtemp (using
tmpdir and join) and writes image.png but never removes the dir; update the test
for inlineImages to ensure the temp directory is always cleaned up by wrapping
the test body in a try/finally (or use afterEach/cleanup) where the finally
block removes the directory (e.g., using rm/rmdir with recursive true) so the
directory created for imagePath is deleted regardless of assertion outcome;
reference the test spec in apps/main-processor/tests/export.test.ts and the
functions mkdtemp, writeFile, inlineImages, tmpdir, and imagePath when making
the change.
In `@apps/main-processor/tests/settings.test.ts`:
- Around line 4-16: Add comprehensive tests for validateSettings covering all
configurable fields: for theme assert success with a value in THEMES and failure
with an unknown string; for readingWidth assert success with a value in
READING_WIDTHS and failure with an invalid value; for lineNumbers and
showHiddenFiles assert success with booleans and failure with non-boolean
values; for customCss assert success with a string and failure with non-string;
for zoom assert success with values in range 50–200 and failure outside that
range; for recentFilesLimit assert success with values in range 1–50 and failure
outside that range. Use the existing test pattern (expect(valid...
).toEqual(...) for success and expect(() => validateSettings(...)).toThrow(...)
for failures), referencing validateSettings, THEMES, READING_WIDTHS, and the
specific field names to implement both success and failure paths for each field.
In `@apps/preload/src/index.ts`:
- Around line 23-27: Constrain the onMenuEvent API so callers can only register
for known menu channels: change the parameter type from event: string to the
specific union derived from MENU_EVENT_LIST (e.g., event: (typeof
MENU_EVENT_LIST)[number] or a keyof/union of its entries) and add a runtime
guard that throws or returns a no-op if an event not present in MENU_EVENT_LIST
is passed; keep the unsubscribe pattern (returning () =>
ipcRenderer.removeListener(...)) and preserve the handler signature (handler:
(_event: IpcRendererEvent, payload?: unknown) => void) but only attach it after
the allowlist check to prevent registering on arbitrary IPC channels.
In `@apps/renderer/src/App.tsx`:
- Around line 54-57: The useEffect in App.tsx is missing a dependency array so
it runs on every render; change the effect that calls window.api.getAppVersion()
and setAppVersion so it only runs on mount by adding an empty dependency array
(keep the guard for window.api?.getAppVersion and the .then/.catch handling),
ensuring the call is invoked once when the component mounts.
In `@apps/renderer/src/components/SettingsPanel.tsx`:
- Around line 13-21: When SettingsPanel opens (isOpen true) ensure keyboard
accessibility by moving focus into the dialog and closing on Escape: add a ref
(e.g., dialogRef) to the <section> rendered by SettingsPanel, set tabIndex={-1}
on that section, and in a useEffect that runs when isOpen becomes true call
dialogRef.current.focus(); also attach an onKeyDown handler (on the section or
the overlay div) that listens for the Escape key and invokes the component's
close handler (e.g., onClose or setIsOpen(false)). This minimal change gives
initial focus and Escape dismissal while a full focus-trap can be added later.
In `@apps/renderer/src/config/marked.ts`:
- Line 31: Replace the magic index THEMES[1] with an explicit, named theme
constant: import either Theme (enum/value) or a dedicated CODE_BLOCK_THEME from
`@package/shared-constants` (or expose it from theme-constants) and set const
theme = CODE_BLOCK_THEME (or Theme.githubDark) instead of using THEMES[1];
update the import at the top of apps/renderer/src/config/marked.ts and remove
the positional dependency on THEMS array so the code block theme remains
explicit and type-checked.
In `@packages/shared-types/package.json`:
- Line 11: Replace the deep import of RecentFile that points into the built dist
of shared-types with a types-only package import: change the import to "import
type { RecentFile } from '`@package/shared-types`'" (instead of importing from
'`@package/shared-types/dist/`...') in the component-types module; if RecentFile
is not already exported from the shared-types package entry, re-export it from
the package's public API (e.g., add/export it from the shared-types index or
barrel) so the type-only import resolves without coupling to the build output
layout.
In `@packages/shared-types/src/settings-type.ts`:
- Line 3: The exported Theme union in packages/shared-types/src/settings-type.ts
is duplicated and can drift from the authoritative THEMES in
packages/shared-constants; replace the hard-coded export by reusing the Theme
type from shared-constants (import Theme from the shared-constants export or
directly from theme-constants type) so both packages share the same definition,
add shared-constants as a dependency in shared-types' package.json if missing,
and ensure shared-constants/src/index.ts exports the Theme type (or export it
from theme-constants) so settings-type.ts can import and re-export it.
---
Outside diff comments:
In `@apps/main-processor/src/export/inlineImage.ts`:
- Around line 23-25: The catch block contains a no-op call
output.replace(fullMatch, fullMatch) which does nothing; remove that no-op and
either delete the empty catch or replace it with a meaningful fallback or
logging (e.g., log the error or leave the original output unchanged explicitly).
Locate the try/catch in apps/main-processor/src/export/inlineImage.ts around the
code using variables fullMatch and output and update the catch to either remove
the redundant replace or call the logger with the caught error so intent is
clear.
In `@apps/main-processor/tests/file.test.ts`:
- Line 42: Tests create './test-file.md' in the repo root and never remove it;
change the TEST_FILE constant in file.test.ts to live inside the existing
test_dir (e.g. use path.join(test_dir, 'test-file.md')) and update the teardown
in afterAll to unlink that file as part of cleanup so the "does not call
callback..." test and others no longer leave artifacts; reference the TEST_FILE
constant and the afterAll teardown in your change.
In `@apps/renderer/src/context/ThemeProvider.tsx`:
- Around line 11-27: The mount logic never applies the system theme because
getSystemTheme() is only called in the media change handler; update the
ThemeProvider mount logic so when there is no saved 'app-theme' you call
getSystemTheme() and setThemeState(...) on initial mount (e.g., inside the
existing first useEffect or at the start of the second useEffect before adding
the listener). Ensure you keep the APPTHEMES check for saved values and only
apply the system theme when localStorage.getItem('app-theme') is falsy;
reference setThemeState, getSystemTheme, APPTHEMES and the 'app-theme' key to
locate the relevant code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 306ee836-a2ef-42af-89da-8767a621040a
⛔ Files ignored due to path filters (2)
apps/renderer/public/icons/app-icon.svgis excluded by!**/*.svgpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!pnpm-lock.yaml
📒 Files selected for processing (35)
.github/workflows/development.yml.github/workflows/production.yml.gitignore.husky/pre-push.prettierignoreapps/main-processor/src/export/inlineImage.tsapps/main-processor/src/ipc.tsapps/main-processor/src/menu.tsapps/main-processor/src/settings/get-settings.tsapps/main-processor/src/settings/save-settings.tsapps/main-processor/src/utils/constants/setting-constants.tsapps/main-processor/src/utils/helper/menu-helper.tsapps/main-processor/src/utils/helper/setting-helper.tsapps/main-processor/tests/export.test.tsapps/main-processor/tests/file.test.tsapps/main-processor/tests/settings.test.tsapps/preload/src/index.tsapps/renderer/index.htmlapps/renderer/src/App.tsxapps/renderer/src/components/SettingsPanel.tsxapps/renderer/src/config/marked.tsapps/renderer/src/context/ThemeProvider.tsxapps/renderer/src/hooks/useMenuEvents.tsapps/renderer/src/hooks/useSettings.tsapps/renderer/src/types/component-types.tsapps/renderer/src/types/hook-types.tsapps/renderer/src/utils/helpers/theme-helper.tspackage.jsonpackages/shared-constants/src/index.tspackages/shared-constants/src/menu-constants.tspackages/shared-constants/src/theme-constants.tspackages/shared-types/package.jsonpackages/shared-types/src/index.tspackages/shared-types/src/markdown-type.tspackages/shared-types/src/settings-type.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🧰 Additional context used
📓 Path-based instructions (14)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (README.md)
Use TypeScript as the primary language for the application
Files:
apps/main-processor/tests/settings.test.tsapps/main-processor/src/settings/get-settings.tsapps/renderer/src/utils/helpers/theme-helper.tspackages/shared-constants/src/menu-constants.tsapps/main-processor/tests/file.test.tsapps/main-processor/src/utils/helper/menu-helper.tsapps/main-processor/src/ipc.tspackages/shared-constants/src/theme-constants.tsapps/renderer/src/types/hook-types.tsapps/renderer/src/components/SettingsPanel.tsxapps/main-processor/src/settings/save-settings.tsapps/renderer/src/config/marked.tsapps/main-processor/src/utils/constants/setting-constants.tsapps/main-processor/src/export/inlineImage.tsapps/main-processor/src/utils/helper/setting-helper.tsapps/main-processor/src/menu.tsapps/preload/src/index.tspackages/shared-types/src/settings-type.tsapps/main-processor/tests/export.test.tsapps/renderer/src/hooks/useMenuEvents.tsapps/renderer/src/App.tsxapps/renderer/src/context/ThemeProvider.tsxpackages/shared-constants/src/index.tspackages/shared-types/src/index.tspackages/shared-types/src/markdown-type.tsapps/renderer/src/hooks/useSettings.tsapps/renderer/src/types/component-types.ts
apps/main-processor/**/*.{ts,tsx}
📄 CodeRabbit inference engine (README.md)
Use Marked for parsing Markdown content
Files:
apps/main-processor/tests/settings.test.tsapps/main-processor/src/settings/get-settings.tsapps/main-processor/tests/file.test.tsapps/main-processor/src/utils/helper/menu-helper.tsapps/main-processor/src/ipc.tsapps/main-processor/src/settings/save-settings.tsapps/main-processor/src/utils/constants/setting-constants.tsapps/main-processor/src/export/inlineImage.tsapps/main-processor/src/utils/helper/setting-helper.tsapps/main-processor/src/menu.tsapps/main-processor/tests/export.test.ts
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (README.md)
Use Vitest for testing
Files:
apps/main-processor/tests/settings.test.tsapps/main-processor/tests/file.test.tsapps/main-processor/tests/export.test.ts
⚙️ CodeRabbit configuration file
**/*.{test,spec}.{ts,tsx}: Review tests.
- Cover success and failure paths, especially IPC, filesystem, markdown rendering, search, settings, tabs, and exports.
- Use isolated temp directories for disk tests and clean them up.
- Mock Electron/preload APIs explicitly.
- Prefer Testing Library user-event and getByRole for UI tests.
Files:
apps/main-processor/tests/settings.test.tsapps/main-processor/tests/file.test.tsapps/main-processor/tests/export.test.ts
**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Write unit tests for all new features to maintain code quality, adding test cases alongside component source code and ensuring all tests pass via
pnpm vitest
Files:
apps/main-processor/tests/settings.test.tsapps/main-processor/tests/file.test.tsapps/main-processor/tests/export.test.ts
apps/main-processor/src/**/*.ts
⚙️ CodeRabbit configuration file
apps/main-processor/src/**/*.ts: Review as Electron main-process code.
- IPC handlers must use shared constants and validate renderer input.
- File/folder access must guard path traversal, missing files, permissions, symlinks, and deleted watched files.
- Watchers, menus, dialogs, and IPC listeners must be cleaned up.
- Do not expose Node/Electron internals or unrestricted filesystem access.
- Export/update/download flows must sanitize content, close resources, and avoid executing embedded scripts.
Files:
apps/main-processor/src/settings/get-settings.tsapps/main-processor/src/utils/helper/menu-helper.tsapps/main-processor/src/ipc.tsapps/main-processor/src/settings/save-settings.tsapps/main-processor/src/utils/constants/setting-constants.tsapps/main-processor/src/export/inlineImage.tsapps/main-processor/src/utils/helper/setting-helper.tsapps/main-processor/src/menu.ts
apps/renderer/**/*.{ts,tsx}
📄 CodeRabbit inference engine (README.md)
apps/renderer/**/*.{ts,tsx}: Use React for frontend UI components
Use Shiki for syntax highlighting in code blocks
Use KaTeX for mathematical equation rendering
Use Mermaid for diagram rendering
Files:
apps/renderer/src/utils/helpers/theme-helper.tsapps/renderer/src/types/hook-types.tsapps/renderer/src/components/SettingsPanel.tsxapps/renderer/src/config/marked.tsapps/renderer/src/hooks/useMenuEvents.tsapps/renderer/src/App.tsxapps/renderer/src/context/ThemeProvider.tsxapps/renderer/src/hooks/useSettings.tsapps/renderer/src/types/component-types.ts
apps/renderer/**/*.{ts,tsx,css}
📄 CodeRabbit inference engine (README.md)
Use Tailwind CSS for styling
Files:
apps/renderer/src/utils/helpers/theme-helper.tsapps/renderer/src/types/hook-types.tsapps/renderer/src/components/SettingsPanel.tsxapps/renderer/src/config/marked.tsapps/renderer/src/hooks/useMenuEvents.tsapps/renderer/src/App.tsxapps/renderer/src/context/ThemeProvider.tsxapps/renderer/src/hooks/useSettings.tsapps/renderer/src/types/component-types.ts
apps/renderer/src/**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
apps/renderer/src/**/*.{ts,tsx}: Review as React renderer code.
- Keep components typed, accessible, keyboard-friendly, and resilient to missing preload APIs.
- Effects must have correct dependencies and cleanup.
- Handle loading, empty, error, stale-response, and rejected-promise states.
- Do not import Node-only modules into renderer code.
- Avoid unnecessary derived state, unsafe globals, and broad any types.
Files:
apps/renderer/src/utils/helpers/theme-helper.tsapps/renderer/src/types/hook-types.tsapps/renderer/src/components/SettingsPanel.tsxapps/renderer/src/config/marked.tsapps/renderer/src/hooks/useMenuEvents.tsapps/renderer/src/App.tsxapps/renderer/src/context/ThemeProvider.tsxapps/renderer/src/hooks/useSettings.tsapps/renderer/src/types/component-types.ts
apps/renderer/src/**/{renderer,markdown,utils}/**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
apps/renderer/src/**/{renderer,markdown,utils}/**/*.{ts,tsx}: Review markdown rendering carefully.
- Sanitize raw HTML, links, images, Mermaid, KaTeX, anchors, and exported content.
- Block script execution, javascript: URLs, unsafe inline handlers, and unsafe local file references.
- Heading IDs and TOC entries must be stable and collision-safe.
- Mermaid/KaTeX/code highlighting failures should not break the whole document.
- Add tests for unsafe HTML, malformed markdown, links, images, code blocks, Mermaid, and KaTeX when changed.
Files:
apps/renderer/src/utils/helpers/theme-helper.ts
{package.json,package-lock.json,yarn.lock,pnpm-lock.yaml,.github/dependabot.yml}
📄 CodeRabbit inference engine (SECURITY.md)
Keep dependencies updated and monitor GitHub Dependabot alerts for security vulnerabilities
Files:
package.json
**/package.json
⚙️ CodeRabbit configuration file
**/package.json: Review scripts and dependencies.
- Scripts for lint, typecheck, test, coverage, build, and dist must fail on errors.
- Dependencies should live in the package that imports them.
- Runtime imports must not be placed only in devDependencies.
- Electron, Vite, React, TypeScript, Tailwind, and testing upgrades need compatibility attention.
Files:
package.jsonpackages/shared-types/package.json
apps/renderer/src/**/*.{css,tsx}
⚙️ CodeRabbit configuration file
apps/renderer/src/**/*.{css,tsx}: Review UI, theme, and accessibility.
- Interactive controls need semantic elements, visible focus, and keyboard access.
- Theme changes must preserve readable contrast in light and dark modes.
- Markdown prose must remain readable for tables, code, blockquotes, links, lists, and images.
- Prefer existing tokens/classes over ad hoc inline styling.
Files:
apps/renderer/src/components/SettingsPanel.tsxapps/renderer/src/App.tsxapps/renderer/src/context/ThemeProvider.tsx
apps/preload/src/**/*.ts
⚙️ CodeRabbit configuration file
apps/preload/src/**/*.ts: Review as a strict preload boundary.
- Expose only typed contextBridge APIs, never raw ipcRenderer.
- Use shared IPC constants and shared payload/result types.
- Listener methods must return unsubscribe functions.
- Reject broad channel names, arbitrary invoke/send wrappers, and any-typed payloads.
Files:
apps/preload/src/index.ts
.github/workflows/**/*.yml
⚙️ CodeRabbit configuration file
.github/workflows/**/*.yml: Review CI/CD.
- Actions should use version tags, not
@main.- Secrets must use ${{ secrets.* }} and never be hardcoded.
- CI should install with pinned pnpm, then run lint, typecheck, tests, build, and package checks.
- Security scans should fail for high/critical findings unless justified.
Files:
.github/workflows/development.yml.github/workflows/production.yml
🪛 zizmor (1.25.2)
.github/workflows/development.yml
[warning] 49-50: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 50-50: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 53-53: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 56-56: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 68-68: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
.github/workflows/production.yml
[warning] 49-50: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 50-50: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 53-53: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 56-56: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 68-68: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🔇 Additional comments (24)
.husky/pre-push (1)
1-1: LGTM!apps/renderer/index.html (1)
6-7: LGTM!.prettierignore (1)
4-9: LGTM!apps/main-processor/src/utils/constants/setting-constants.ts (1)
3-14: LGTM!apps/main-processor/src/utils/helper/setting-helper.ts (1)
17-70: LGTM!apps/main-processor/src/settings/get-settings.ts (1)
7-19: LGTM!packages/shared-constants/src/menu-constants.ts (1)
23-46: LGTM!.gitignore (1)
1-41: LGTM!apps/renderer/src/utils/helpers/theme-helper.ts (1)
4-6: LGTM!apps/renderer/src/types/hook-types.ts (1)
45-46: LGTM!Also applies to: 61-61
apps/renderer/src/hooks/useMenuEvents.ts (1)
44-49: LGTM!apps/renderer/src/App.tsx (1)
184-184: LGTM!apps/renderer/src/hooks/useSettings.ts (1)
10-45: LGTM!Also applies to: 59-75
apps/renderer/src/types/component-types.ts (1)
79-90: LGTM!Also applies to: 104-132, 169-175
apps/main-processor/src/utils/helper/menu-helper.ts (1)
3-9: LGTM!apps/main-processor/src/ipc.ts (2)
119-124: LGTM!Also applies to: 133-138
126-131: ⚡ Quick winRenderer-supplied settings payload is already runtime-validated before persisting.
saveSettings()callsvalidateSettings(settings), which checks the payload is a plain object, rejects unknown keys viaSETTINGS_KEYS, and validates each supported field at runtime (theme/readingWidth enums, numeric ranges, and boolean/string types) before writingsettings.json.apps/main-processor/src/menu.ts (1)
98-98: LGTM!packages/shared-types/src/index.ts (1)
1-8: LGTM!.github/workflows/production.yml (1)
67-73: ⚡ Quick winFix Trivy CI gate + pin action version (File:
.github/workflows/production.yml, lines 67-73)
aquasecurity/trivy-action@masteris unpinned and can change over time—pin toaquasecurity/trivy-action@v0.36.0.- Ensure the scan step actually fails the workflow on CRITICAL/HIGH by setting an explicit non-zero
exit-code(not justformat: table).🔧 Suggested change
- name: Run Trivy vulnerability scanner uses: aquasecurity/trivy-action@v0.36.0 with: scan-type: fs scan-ref: . severity: CRITICAL,HIGH format: table exit-code: '1'packages/shared-constants/src/theme-constants.ts (1)
1-19: LGTM!package.json (1)
28-28: LGTM!Also applies to: 69-75
packages/shared-constants/src/index.ts (1)
6-15: LGTM!packages/shared-types/src/markdown-type.ts (1)
16-21: LGTM!
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/main-processor/src/folder.ts (1)
28-50:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUnguarded
readdircan sink the whole tree on one bad child.The cycle/depth hardening is solid, but
readdirat Line 28 isn't guarded. A single child directory that's permission-denied, deleted mid-walk, or otherwise unreadable will reject the recursivegetFolderat Line 39 and fail the entire folder load. Skip the offending entry instead of crashing the traversal.As per coding guidelines: "File/folder access must guard path traversal, missing files, permissions, symlinks, and deleted watched files."🛡️ Proposed fix
- seenPaths.add(realFolderPath); - const entries = (await readdir(folderPath, { withFileTypes: true })).sort((a, b) => - a.name.localeCompare(b.name) - ); + seenPaths.add(realFolderPath); + let entries; + try { + entries = (await readdir(folderPath, { withFileTypes: true })).sort((a, b) => + a.name.localeCompare(b.name) + ); + } catch { + // Unreadable/deleted directory: return it as an empty node rather than failing the walk. + return { name: basename(folderPath), path: folderPath, isDir: true, children: [] }; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/main-processor/src/folder.ts` around lines 28 - 50, The unguarded filesystem calls can make the entire traversal fail; wrap the initial readdir(folderPath, { withFileTypes: true }) in a try/catch and return an empty children array (or skip) on error, and also wrap the recursive call to getFolder(fullPath, maxDepth, currentDepth + 1, seenPaths) in its own try/catch so a single unreadable/removed child is skipped instead of rejecting the whole function; use the existing symbols readdir, join, isMarkdownFile and getFolder and optionally log the caught error before continuing.electron-builder.ts (1)
60-67: 🧹 Nitpick | 🔵 Trivial | 💤 Low value
nsis.licenseturns the notice into a forced "accept" gate.NSIS renders
licenseas a mandatory License Agreement page the user must accept to proceed. Repurposing the unsigned-build notice as a license is functional (.txtis valid) but semantically misleading. Since it's already shipped viaextraResources, consider dropping it fromnsis.license(or authoring a dedicated license/EULA) to avoid surprising the installer flow.🤖 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 `@electron-builder.ts` around lines 60 - 67, The NSIS installer config is using nsis.license which forces a mandatory License Agreement UI; remove the license entry from the nsis block (or replace it with a proper LICENSE/EULA file path if you intend a true license) so the unsigned-build notice isn't presented as an accept gate, and ensure the existing notice remains shipped via extraResources (or add a dedicated license asset) so packaging still includes the notice without triggering the NSIS accept dialog.
🤖 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.
Outside diff comments:
In `@apps/main-processor/src/folder.ts`:
- Around line 28-50: The unguarded filesystem calls can make the entire
traversal fail; wrap the initial readdir(folderPath, { withFileTypes: true }) in
a try/catch and return an empty children array (or skip) on error, and also wrap
the recursive call to getFolder(fullPath, maxDepth, currentDepth + 1, seenPaths)
in its own try/catch so a single unreadable/removed child is skipped instead of
rejecting the whole function; use the existing symbols readdir, join,
isMarkdownFile and getFolder and optionally log the caught error before
continuing.
In `@electron-builder.ts`:
- Around line 60-67: The NSIS installer config is using nsis.license which
forces a mandatory License Agreement UI; remove the license entry from the nsis
block (or replace it with a proper LICENSE/EULA file path if you intend a true
license) so the unsigned-build notice isn't presented as an accept gate, and
ensure the existing notice remains shipped via extraResources (or add a
dedicated license asset) so packaging still includes the notice without
triggering the NSIS accept dialog.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b0f5e127-ab36-417b-8999-ce091237e67c
📒 Files selected for processing (7)
.github/workflows/development.ymlapps/main-processor/src/folder.tsapps/renderer/src/App.tsxapps/renderer/src/utils/helpers/heading-helper.tsassets/unsigned-install-notice.txtelectron-builder.tselectron.vite.config.ts
💤 Files with no reviewable changes (1)
- electron.vite.config.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🧰 Additional context used
📓 Path-based instructions (10)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (README.md)
Use TypeScript as the primary language for the application
Files:
electron-builder.tsapps/renderer/src/utils/helpers/heading-helper.tsapps/main-processor/src/folder.tsapps/renderer/src/App.tsx
electron-builder.ts
⚙️ CodeRabbit configuration file
electron-builder.ts: Review packaging.
- Check appId, productName, files, asar, icons, targets, file associations, artifact names, and publish settings.
- Exclude source-only, test, coverage, cache, and map files from releases.
- Signing/notarisation/update config must not hardcode secrets.
- Platform targets should match expected Windows, macOS, and Linux release formats.
Files:
electron-builder.ts
apps/renderer/**/*.{ts,tsx}
📄 CodeRabbit inference engine (README.md)
apps/renderer/**/*.{ts,tsx}: Use React for frontend UI components
Use Shiki for syntax highlighting in code blocks
Use KaTeX for mathematical equation rendering
Use Mermaid for diagram rendering
Files:
apps/renderer/src/utils/helpers/heading-helper.tsapps/renderer/src/App.tsx
apps/renderer/**/*.{ts,tsx,css}
📄 CodeRabbit inference engine (README.md)
Use Tailwind CSS for styling
Files:
apps/renderer/src/utils/helpers/heading-helper.tsapps/renderer/src/App.tsx
apps/renderer/src/**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
apps/renderer/src/**/*.{ts,tsx}: Review as React renderer code.
- Keep components typed, accessible, keyboard-friendly, and resilient to missing preload APIs.
- Effects must have correct dependencies and cleanup.
- Handle loading, empty, error, stale-response, and rejected-promise states.
- Do not import Node-only modules into renderer code.
- Avoid unnecessary derived state, unsafe globals, and broad any types.
Files:
apps/renderer/src/utils/helpers/heading-helper.tsapps/renderer/src/App.tsx
apps/renderer/src/**/{renderer,markdown,utils}/**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
apps/renderer/src/**/{renderer,markdown,utils}/**/*.{ts,tsx}: Review markdown rendering carefully.
- Sanitize raw HTML, links, images, Mermaid, KaTeX, anchors, and exported content.
- Block script execution, javascript: URLs, unsafe inline handlers, and unsafe local file references.
- Heading IDs and TOC entries must be stable and collision-safe.
- Mermaid/KaTeX/code highlighting failures should not break the whole document.
- Add tests for unsafe HTML, malformed markdown, links, images, code blocks, Mermaid, and KaTeX when changed.
Files:
apps/renderer/src/utils/helpers/heading-helper.ts
.github/workflows/**/*.yml
⚙️ CodeRabbit configuration file
.github/workflows/**/*.yml: Review CI/CD.
- Actions should use version tags, not
@main.- Secrets must use ${{ secrets.* }} and never be hardcoded.
- CI should install with pinned pnpm, then run lint, typecheck, tests, build, and package checks.
- Security scans should fail for high/critical findings unless justified.
Files:
.github/workflows/development.yml
apps/main-processor/**/*.{ts,tsx}
📄 CodeRabbit inference engine (README.md)
Use Marked for parsing Markdown content
Files:
apps/main-processor/src/folder.ts
apps/main-processor/src/**/*.ts
⚙️ CodeRabbit configuration file
apps/main-processor/src/**/*.ts: Review as Electron main-process code.
- IPC handlers must use shared constants and validate renderer input.
- File/folder access must guard path traversal, missing files, permissions, symlinks, and deleted watched files.
- Watchers, menus, dialogs, and IPC listeners must be cleaned up.
- Do not expose Node/Electron internals or unrestricted filesystem access.
- Export/update/download flows must sanitize content, close resources, and avoid executing embedded scripts.
Files:
apps/main-processor/src/folder.ts
apps/renderer/src/**/*.{css,tsx}
⚙️ CodeRabbit configuration file
apps/renderer/src/**/*.{css,tsx}: Review UI, theme, and accessibility.
- Interactive controls need semantic elements, visible focus, and keyboard access.
- Theme changes must preserve readable contrast in light and dark modes.
- Markdown prose must remain readable for tables, code, blockquotes, links, lists, and images.
- Prefer existing tokens/classes over ad hoc inline styling.
Files:
apps/renderer/src/App.tsx
🪛 zizmor (1.25.2)
.github/workflows/development.yml
[error] 68-68: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🔇 Additional comments (7)
.github/workflows/development.yml (2)
52-53: ⚡ Quick win
pnpm/action-setupversions still diverge across jobs.Build (Line 23) pins
@v3while security (Line 53) uses@v4. Standardise on one to avoid setup drift.
67-74: LGTM!apps/main-processor/src/folder.ts (1)
13-18: LGTM!apps/renderer/src/App.tsx (1)
54-57: LGTM!apps/renderer/src/utils/helpers/heading-helper.ts (2)
18-18: LGTM!
29-36: ⚡ Quick winFix: confirm de-duplication between rendered heading
ids and TOC anchor ids
toc.tsalready de-dupes TOC entries by counting repeatedgetHeadingId(text)values and appending-<count>(so TOC anchors won’t always target the first heading). Butheading()inapps/renderer/src/utils/helpers/heading-helper.tscurrently assignsid="${getHeadingId(plainText)}"with no suffixing, which could leave duplicate DOM IDs and cause anchor mismatches (or non-deterministic navigation) when identical headings exist. Update/render-time heading IDs to use the same de-dup scheme (or ensure the renderer de-dupes identically totoc.ts).assets/unsigned-install-notice.txt (1)
1-24: LGTM!
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/production.yml (1)
52-53:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPin pnpm version in the security job too.
The
buildjob pinsversion: 10(Line 25) but thesecurityjob omits it, so the two jobs can resolve different pnpm versions for the same--frozen-lockfileinstall.🔧 Suggested fix
- name: Setup pnpm uses: pnpm/action-setup@v4 + with: + version: 10As per coding guidelines: "CI should install with pinned pnpm".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/production.yml around lines 52 - 53, The security job's "Setup pnpm" step omits a pinned version causing inconsistent pnpm between jobs; update the security job's Setup pnpm step (the pnpm/action-setup@v4 usage) to include the same pinned version as the build job (e.g., add version: 10) so both jobs install the identical pnpm when running --frozen-lockfile.
🤖 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:
- Around line 22-23: The pnpm setup steps in the workflow use
pnpm/action-setup@v4 without a pinned version; update the pnpm setup steps for
both the build and security jobs to include an explicit version key (e.g.,
version: 10) so they match production's pinned pnpm major and avoid differing
majors when resolving from packageManager; locate the uses: pnpm/action-setup@v4
entries in the workflow and add the version field to each.
---
Outside diff comments:
In @.github/workflows/production.yml:
- Around line 52-53: The security job's "Setup pnpm" step omits a pinned version
causing inconsistent pnpm between jobs; update the security job's Setup pnpm
step (the pnpm/action-setup@v4 usage) to include the same pinned version as the
build job (e.g., add version: 10) so both jobs install the identical pnpm when
running --frozen-lockfile.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e1fe3fcd-1b8f-4132-864a-5d2db08c4c93
📒 Files selected for processing (9)
.github/workflows/development.yml.github/workflows/production.ymlapps/main-processor/src/utils/helper/setting-helper.tsapps/main-processor/tests/export.test.tsapps/main-processor/tests/settings.test.tsapps/renderer/src/types/component-types.tspackages/shared-types/src/index.tspackages/shared-types/src/settings-default.tspackages/shared-types/src/settings-type.ts
💤 Files with no reviewable changes (1)
- packages/shared-types/src/settings-type.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🧰 Additional context used
📓 Path-based instructions (9)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (README.md)
Use TypeScript as the primary language for the application
Files:
packages/shared-types/src/settings-default.tspackages/shared-types/src/index.tsapps/main-processor/tests/export.test.tsapps/main-processor/src/utils/helper/setting-helper.tsapps/main-processor/tests/settings.test.tsapps/renderer/src/types/component-types.ts
.github/workflows/**/*.yml
⚙️ CodeRabbit configuration file
.github/workflows/**/*.yml: Review CI/CD.
- Actions should use version tags, not
@main.- Secrets must use ${{ secrets.* }} and never be hardcoded.
- CI should install with pinned pnpm, then run lint, typecheck, tests, build, and package checks.
- Security scans should fail for high/critical findings unless justified.
Files:
.github/workflows/development.yml.github/workflows/production.yml
apps/main-processor/**/*.{ts,tsx}
📄 CodeRabbit inference engine (README.md)
Use Marked for parsing Markdown content
Files:
apps/main-processor/tests/export.test.tsapps/main-processor/src/utils/helper/setting-helper.tsapps/main-processor/tests/settings.test.ts
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (README.md)
Use Vitest for testing
Files:
apps/main-processor/tests/export.test.tsapps/main-processor/tests/settings.test.ts
⚙️ CodeRabbit configuration file
**/*.{test,spec}.{ts,tsx}: Review tests.
- Cover success and failure paths, especially IPC, filesystem, markdown rendering, search, settings, tabs, and exports.
- Use isolated temp directories for disk tests and clean them up.
- Mock Electron/preload APIs explicitly.
- Prefer Testing Library user-event and getByRole for UI tests.
Files:
apps/main-processor/tests/export.test.tsapps/main-processor/tests/settings.test.ts
**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Write unit tests for all new features to maintain code quality, adding test cases alongside component source code and ensuring all tests pass via
pnpm vitest
Files:
apps/main-processor/tests/export.test.tsapps/main-processor/tests/settings.test.ts
apps/main-processor/src/**/*.ts
⚙️ CodeRabbit configuration file
apps/main-processor/src/**/*.ts: Review as Electron main-process code.
- IPC handlers must use shared constants and validate renderer input.
- File/folder access must guard path traversal, missing files, permissions, symlinks, and deleted watched files.
- Watchers, menus, dialogs, and IPC listeners must be cleaned up.
- Do not expose Node/Electron internals or unrestricted filesystem access.
- Export/update/download flows must sanitize content, close resources, and avoid executing embedded scripts.
Files:
apps/main-processor/src/utils/helper/setting-helper.ts
apps/renderer/**/*.{ts,tsx}
📄 CodeRabbit inference engine (README.md)
apps/renderer/**/*.{ts,tsx}: Use React for frontend UI components
Use Shiki for syntax highlighting in code blocks
Use KaTeX for mathematical equation rendering
Use Mermaid for diagram rendering
Files:
apps/renderer/src/types/component-types.ts
apps/renderer/**/*.{ts,tsx,css}
📄 CodeRabbit inference engine (README.md)
Use Tailwind CSS for styling
Files:
apps/renderer/src/types/component-types.ts
apps/renderer/src/**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
apps/renderer/src/**/*.{ts,tsx}: Review as React renderer code.
- Keep components typed, accessible, keyboard-friendly, and resilient to missing preload APIs.
- Effects must have correct dependencies and cleanup.
- Handle loading, empty, error, stale-response, and rejected-promise states.
- Do not import Node-only modules into renderer code.
- Avoid unnecessary derived state, unsafe globals, and broad any types.
Files:
apps/renderer/src/types/component-types.ts
🪛 zizmor (1.25.2)
.github/workflows/development.yml
[error] 23-23: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 66-66: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
.github/workflows/production.yml
[error] 23-23: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 68-68: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🔇 Additional comments (8)
packages/shared-types/src/settings-default.ts (1)
3-12: LGTM!packages/shared-types/src/index.ts (1)
1-7: LGTM!.github/workflows/development.yml (1)
66-72: LGTM!apps/main-processor/tests/export.test.ts (1)
60-69: LGTM!apps/main-processor/src/utils/helper/setting-helper.ts (1)
8-66: LGTM!apps/main-processor/tests/settings.test.ts (1)
18-59: LGTM!.github/workflows/production.yml (1)
67-74: LGTM!apps/renderer/src/types/component-types.ts (1)
79-90: LGTM!Also applies to: 104-132, 169-175
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 fixe several critical, high and medium level issues related to security and UI.
Type of Change
Related Issue
Closes #42 ,#76,#77, #82, #83
Checklist
Electron main/preload
Renderer UI
Markdown rendering
Shared packages
Tests
Tooling / CI
Docs
Packaging
Breaking changes: