Fix/system hardening#90
Conversation
…f dangerous constructs
…OC heading ids consistent
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
(Note: All rangeIds from the provided summary appear above either in a layer or unassigned_ranges.) 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/src/components/Homepage/hero.tsx (1)
55-65:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove nested interactive elements in the GitHub CTA (and apply the same fix to “Get Started”).
The GitHub CTA currently nests a
<button>inside an<a>, which is invalid interactive element nesting and can break keyboard/screen-reader behavior. Make the<a>(and theLinkfor “Get Started”) the single interactive element and remove the nested<button>.Suggested fix (GitHub CTA)
- <a - href="https://github.com/mindfiredigital/markdown-reader" - className="no-underline" - target="_blank" - rel="noreferrer" - > - <button className="inline-flex items-center justify-center bg-background/50 hover:bg-background/80 text-foreground border border-border/80 font-medium h-11 px-6 text-sm rounded-xl shadow-sm cursor-pointer transition-colors duration-200"> - <GitBranchIcon className="mr-1.5 h-4 w-4" strokeWidth={2.5} /> - GitHub - </button> - </a> + <a + href="https://github.com/mindfiredigital/markdown-reader" + 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" + > + <GitBranchIcon className="mr-1.5 h-4 w-4" strokeWidth={2.5} /> + GitHub + </a>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/src/components/Homepage/hero.tsx` around lines 55 - 65, The GitHub CTA nests a <button> inside an <a> (and the "Get Started" uses a Link wrapping a button), which is invalid; update the Homepage hero component in docs/src/components/Homepage/hero.tsx to make the anchor (and the Link for "Get Started") the single interactive element by removing the inner <button> and moving its styling/classes to the <a> (or Link) itself, preserving the content (icon GitBranchIcon and text "GitHub") and all accessibility attributes (target, rel, role if needed) and hover/focus classes so keyboard and screen-reader behavior remains correct.
🤖 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/export/exportDocx.ts`:
- Around line 8-9: The inlineImages flow is vulnerable because
EXPORT_IMAGE_SRC_REGEX + normaliseImagePath() permit attacker-controlled local
paths which are passed to readFile; update inlineImages (and normaliseImagePath
usage) to resolve each src against a configured allowed root directory by
joining then calling fs.realpath and using isPathInside to ensure the resolved
path stays inside the allowed root, explicitly reject absolute paths or any path
containing traversal before resolving, detect and reject symlink escapes
(realpath != resolved join) and enforce an extension allowlist and maximum file
size before reading; on any validation/read failure return an error placeholder
(fail closed) and do not preserve the original src in the output so
buildDocument receives only validated/embedded image data.
In `@apps/main-processor/src/file.ts`:
- Around line 44-46: The watcher error and unlink handlers currently call void
unWatchFile(filePath).then(...), dropping any rejection from unWatchFile; change
both handlers to await or chain a promise that handles rejections so cleanup
failures are not unhandled: call unWatchFile(filePath).then(() =>
onError?.(watcherError) /* or onDeleted?.() */).catch(cleanErr => {
onError?.(cleanErr) ?? console.error(cleanErr); /* ensure original watcherError
handling still occurs (e.g., call reject(watcherError) or onDeleted) after
handling cleanup failure */ }); ensure the reject(watcherError) (in the error
handler) and onDeleted invocation (in unlink handler) happen in the then/catch
flow so neither cleanup rejections nor the original watcher events are dropped
(refer to unWatchFile, onError, onDeleted, and the watcher 'error'/'unlink'
handlers).
In `@apps/renderer/__tests__/hooks/useSettings.test.ts`:
- Around line 15-55: Add a failing-save test for the useSettings hook that mocks
window.api.saveSettings to return a rejected promise, then call the hook action
(e.g., increaseFontSize or decreaseFontSize) inside act and assert the save
promise rejects; verify that result.current.fontSize has not advanced (state
remains the previous value) and that any error is surfaced (rethrow/log
behavior). Reference the useSettings hook, window.api.saveSettings,
increaseFontSize/decreaseFontSize/resetFontSize, and result.current.fontSize
when implementing the test so you cover the rejected IPC save path and ensure
state is unchanged.
- Around line 17-18: The tests call
result.current.increaseFontSize/decreaseFontSize/resetFontSize but those helpers
return void while the real persistence is async via updateSettings which awaits
window.api.saveSettings; change the tests to either call
result.current.updateSettings(...) directly (which returns the promise) or wrap
the helper call with a waitFor that asserts the resulting state change (e.g.,
new font size) instead of relying on act to await saveSettings; specifically
update assertions after calling increaseFontSize/decreaseFontSize/resetFontSize
to use waitFor until the state reflects the persisted change, and ensure mocks
for window.api.saveSettings remain async so the test truly awaits the async
path.
In `@apps/renderer/setup.ts`:
- Around line 32-36: The saveSettings mock currently overwrites all settings
with defaults and only respects partial.fontSize; update the vi.fn mock for
saveSettings so it merges the incoming partial into the current stored settings
instead of resetting fields—read the existing settings state, shallow-merge
partial onto it (preserving readingWidth, customCss, etc.), persist the merged
object as the new state, and return that merged settings object; locate the
saveSettings mock function to implement this behavior.
In `@apps/renderer/src/components/UpdateBanner.tsx`:
- Around line 7-10: The UpdateBanner component subscribes unguarded to
window.api.onUpdateAvailable which throws if the Electron preload bridge is
absent; guard the subscription by checking that window.api and
window.api.onUpdateAvailable are defined before calling it (use a no-op fallback
for removeUpdateAvailable), call window.api.onUpdateAvailable((version: string)
=> setUpdateVersion(version)) only when available, and ensure
removeUpdateAvailable is a safe function (e.g., () => {}) so the component can
return/cleanup without errors; update references: window.api, onUpdateAvailable,
removeUpdateAvailable, and setUpdateVersion.
In `@apps/renderer/src/hooks/useSettings.ts`:
- Around line 52-72: The font-size handlers (increaseFontSize, decreaseFontSize,
resetFontSize) currently discard the promise from updateSettings using "void",
causing unhandled rejections and stale reads; fix by returning/awaiting the
updateSettings call (i.e., remove "void" and return the Promise so callers can
handle errors) or implement an optimistic/serialized update: compute newSize
from the latest settings via a functional update (or local state/ref lock),
apply the optimistic settings update locally, then await updateSettings({
fontSize: newSize }) and handle errors to rollback if needed; ensure references
to settings.fontSize, updateSettings, and the three handler functions are
updated accordingly.
---
Outside diff comments:
In `@docs/src/components/Homepage/hero.tsx`:
- Around line 55-65: The GitHub CTA nests a <button> inside an <a> (and the "Get
Started" uses a Link wrapping a button), which is invalid; update the Homepage
hero component in docs/src/components/Homepage/hero.tsx to make the anchor (and
the Link for "Get Started") the single interactive element by removing the inner
<button> and moving its styling/classes to the <a> (or Link) itself, preserving
the content (icon GitBranchIcon and text "GitHub") and all accessibility
attributes (target, rel, role if needed) and hover/focus classes so keyboard and
screen-reader behavior remains correct.
🪄 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: 4c156f0a-2156-4914-a797-eeae3b5d9a2c
📒 Files selected for processing (16)
.github/workflows/production.ymlapps/main-processor/src/export/exportDocx.tsapps/main-processor/src/export/exportPdf.tsapps/main-processor/src/export/sanitizeCss.tsapps/main-processor/src/file.tsapps/main-processor/src/ipc.tsapps/main-processor/src/utils/constants/ipc-validation.tsapps/preload/src/index.tsapps/renderer/__tests__/hooks/useSettings.test.tsapps/renderer/setup.tsapps/renderer/src/components/UpdateBanner.tsxapps/renderer/src/hooks/useFolderSearch.tsapps/renderer/src/hooks/useSettings.tsdocs/src/components/Homepage/hero.tsxpackage.jsonpackages/shared-types/src/markdown-type.ts
💤 Files with no reviewable changes (2)
- apps/main-processor/src/ipc.ts
- .github/workflows/production.yml
📜 Review details
🧰 Additional context used
📓 Path-based instructions (13)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (README.md)
Use TypeScript as the primary language for the application
Files:
apps/main-processor/src/export/exportPdf.tsapps/main-processor/src/export/exportDocx.tsapps/renderer/src/hooks/useFolderSearch.tsapps/renderer/src/components/UpdateBanner.tsxdocs/src/components/Homepage/hero.tsxapps/main-processor/src/export/sanitizeCss.tsapps/renderer/setup.tsapps/preload/src/index.tspackages/shared-types/src/markdown-type.tsapps/main-processor/src/utils/constants/ipc-validation.tsapps/renderer/__tests__/hooks/useSettings.test.tsapps/renderer/src/hooks/useSettings.tsapps/main-processor/src/file.ts
apps/main-processor/**/*.{ts,tsx}
📄 CodeRabbit inference engine (README.md)
Use Marked for parsing Markdown content
Files:
apps/main-processor/src/export/exportPdf.tsapps/main-processor/src/export/exportDocx.tsapps/main-processor/src/export/sanitizeCss.tsapps/main-processor/src/utils/constants/ipc-validation.tsapps/main-processor/src/file.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/export/exportPdf.tsapps/main-processor/src/export/exportDocx.tsapps/main-processor/src/export/sanitizeCss.tsapps/main-processor/src/utils/constants/ipc-validation.tsapps/main-processor/src/file.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.json
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/hooks/useFolderSearch.tsapps/renderer/src/components/UpdateBanner.tsxapps/renderer/setup.tsapps/renderer/__tests__/hooks/useSettings.test.tsapps/renderer/src/hooks/useSettings.ts
apps/renderer/**/*.{ts,tsx,css}
📄 CodeRabbit inference engine (README.md)
Use Tailwind CSS for styling
Files:
apps/renderer/src/hooks/useFolderSearch.tsapps/renderer/src/components/UpdateBanner.tsxapps/renderer/setup.tsapps/renderer/__tests__/hooks/useSettings.test.tsapps/renderer/src/hooks/useSettings.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/hooks/useFolderSearch.tsapps/renderer/src/components/UpdateBanner.tsxapps/renderer/src/hooks/useSettings.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/components/UpdateBanner.tsx
docs/**/*.{md,mdx,ts,tsx}
⚙️ CodeRabbit configuration file
docs/**/*.{md,mdx,ts,tsx}: Review docs.
- Docs must match current shortcuts, markdown support, export behaviour, install steps, and privacy/offline claims.
- Code blocks need language tags.
- Links and images should resolve.
- Docusaurus components must guard browser-only APIs during static build.
Files:
docs/src/components/Homepage/hero.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
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (README.md)
Use Vitest for testing
Files:
apps/renderer/__tests__/hooks/useSettings.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/renderer/__tests__/hooks/useSettings.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/renderer/__tests__/hooks/useSettings.test.ts
🔇 Additional comments (7)
package.json (1)
71-71: LGTM!apps/main-processor/src/export/exportPdf.ts (1)
11-11: LGTM!apps/main-processor/src/export/sanitizeCss.ts (1)
5-13: LGTM!packages/shared-types/src/markdown-type.ts (1)
32-32: LGTM!apps/preload/src/index.ts (1)
56-61: LGTM!apps/renderer/src/hooks/useFolderSearch.ts (1)
1-1: LGTM!Also applies to: 42-46
apps/main-processor/src/utils/constants/ipc-validation.ts (1)
17-21: ⚡ Quick winTighten Electron IPC sender validation allowlist (avoid broad
file:andhttp://localhost)In
apps/main-processor/src/utils/constants/ipc-validation.ts(lines 17-21),validateSenderreturnstruefor anyfile:URL and for anyhttp://localhostwith no port/origin pinning. Since this is an IPC trust gate, it weakens the main/preload boundary and could allow unexpected local/localhost pages to satisfy the check.Replace protocol/hostname-only matching with an explicit allowlist of the exact renderer entrypoint/origin(s) for dev and packaged builds (including port for localhost, or exact file paths for packaged
file://), and deny all other URLs; keep URL parsing strict with safe handling for missing/invalidevent.senderFrame?.url.
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)
.github/workflows/release.yml (1)
102-102:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUnify pnpm action version across jobs.
The
version_and_tagjob usespnpm/action-setup@v4(line 30) but this build job uses@v3. Different major versions can cause behavioral differences in dependency resolution. Standardize on@v4.🔧 Proposed fix
- - name: Setup pnpm - uses: pnpm/action-setup@v3 + - name: Setup pnpm + uses: pnpm/action-setup@v4🤖 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/release.yml at line 102, The workflow uses pnpm/action-setup@v3 in this job while other jobs use pnpm/action-setup@v4; update the uses entry (the step referencing "uses: pnpm/action-setup@v3") to pnpm/action-setup@v4 so all jobs standardize on the same major version and avoid behavioral differences across jobs.apps/renderer/src/components/UpdateBanner.tsx (1)
24-26:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard the
downloadUpdatecall against missing API.The
downloadUpdateinvocation assumeswindow.api.downloadUpdateexists. For consistency with the guard pattern at line 7 and to maintain resilience, add optional chaining.🛡️ Suggested guard
onClick={() => { - void window.api.downloadUpdate(); + void window.api?.downloadUpdate?.(); }}As per coding guidelines, renderer components must be "resilient to missing preload APIs".
🤖 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 currently assumes window.api.downloadUpdate exists; update the handler in the UpdateBanner component to call downloadUpdate defensively using optional chaining (e.g., invoke window.api?.downloadUpdate?.()) so the click is resilient when the preload API is missing; ensure the existing void usage/intent remains consistent (e.g., void window.api?.downloadUpdate?.()) and adjust only the onClick invocation.
♻️ Duplicate comments (1)
apps/renderer/src/hooks/useSettings.ts (1)
58-72:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftSerialize font-size updates instead of reading a stale render snapshot.
Line 60 and Line 66 still compute the next size from
settings.fontSizecaptured in the current render. Two rapid clicks before the save resolves/re-render happens will send the same value twice, so one increment/decrement is lost. Returning the Promise fixed the unhandled-rejection part, but not this stale-read race; compute from the latest local state/ref and persist that sequence instead.As per coding guidelines, "Handle loading, empty, error, stale-response, and rejected-promise states."
🤖 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 - 72, The increment/decrement handlers (increaseFontSize, decreaseFontSize) read settings.fontSize from the render snapshot and can lose rapid clicks; change them to compute the new value from the latest state by using a functional update instead of reading settings directly (e.g. call updateSettings with an updater that receives prev => ({ fontSize: Math.min(FONT_SIZE.MAX, prev.fontSize + FONT_SIZE.INCREMENT) }) and similarly for decrement), return that Promise, and keep resetFontSize as-is (or also return updateSettings({ fontSize: FONT_SIZE.DEFAULT })). If updateSettings does not support a functional updater, maintain a latestSettingsRef updated in an effect and read from that ref inside increaseFontSize/decreaseFontSize to serialize updates.
🤖 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 @.github/workflows/release.yml:
- Line 102: The workflow uses pnpm/action-setup@v3 in this job while other jobs
use pnpm/action-setup@v4; update the uses entry (the step referencing "uses:
pnpm/action-setup@v3") to pnpm/action-setup@v4 so all jobs standardize on the
same major version and avoid behavioral differences across jobs.
In `@apps/renderer/src/components/UpdateBanner.tsx`:
- Around line 24-26: The onClick handler currently assumes
window.api.downloadUpdate exists; update the handler in the UpdateBanner
component to call downloadUpdate defensively using optional chaining (e.g.,
invoke window.api?.downloadUpdate?.()) so the click is resilient when the
preload API is missing; ensure the existing void usage/intent remains consistent
(e.g., void window.api?.downloadUpdate?.()) and adjust only the onClick
invocation.
---
Duplicate comments:
In `@apps/renderer/src/hooks/useSettings.ts`:
- Around line 58-72: The increment/decrement handlers (increaseFontSize,
decreaseFontSize) read settings.fontSize from the render snapshot and can lose
rapid clicks; change them to compute the new value from the latest state by
using a functional update instead of reading settings directly (e.g. call
updateSettings with an updater that receives prev => ({ fontSize:
Math.min(FONT_SIZE.MAX, prev.fontSize + FONT_SIZE.INCREMENT) }) and similarly
for decrement), return that Promise, and keep resetFontSize as-is (or also
return updateSettings({ fontSize: FONT_SIZE.DEFAULT })). If updateSettings does
not support a functional updater, maintain a latestSettingsRef updated in an
effect and read from that ref inside increaseFontSize/decreaseFontSize to
serialize updates.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 62a8017a-9b60-47b3-8c75-1940d6cdb2ae
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!pnpm-lock.yaml
📒 Files selected for processing (5)
.github/workflows/release.ymlapps/main-processor/src/file.tsapps/renderer/setup.tsapps/renderer/src/components/UpdateBanner.tsxapps/renderer/src/hooks/useSettings.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 (8)
.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/release.yml
**/*.{ts,tsx}
📄 CodeRabbit inference engine (README.md)
Use TypeScript as the primary language for the application
Files:
apps/renderer/setup.tsapps/renderer/src/components/UpdateBanner.tsxapps/main-processor/src/file.tsapps/renderer/src/hooks/useSettings.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/setup.tsapps/renderer/src/components/UpdateBanner.tsxapps/renderer/src/hooks/useSettings.ts
apps/renderer/**/*.{ts,tsx,css}
📄 CodeRabbit inference engine (README.md)
Use Tailwind CSS for styling
Files:
apps/renderer/setup.tsapps/renderer/src/components/UpdateBanner.tsxapps/renderer/src/hooks/useSettings.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/components/UpdateBanner.tsxapps/renderer/src/hooks/useSettings.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/components/UpdateBanner.tsx
apps/main-processor/**/*.{ts,tsx}
📄 CodeRabbit inference engine (README.md)
Use Marked for parsing Markdown content
Files:
apps/main-processor/src/file.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/file.ts
🔇 Additional comments (4)
.github/workflows/release.yml (1)
11-12: LGTM!apps/renderer/src/components/UpdateBanner.tsx (1)
7-11: LGTM!apps/renderer/setup.ts (1)
25-37: ⚡ Quick winReset the
window.apisettings mock between tests
mockSettingsis module-scoped andsaveSettingsmutates it via{ ...mockSettings, ...partial }, so changes made in one test can leak into later tests. Recreate/resetmockSettingsfor each test case (e.g., viabeforeEachin the test file or a reset helper exported from the setup).apps/main-processor/src/file.ts (1)
44-46: LGTM!Also applies to: 68-70
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 (5)
apps/renderer/src/hooks/useShortcuts.ts (1)
28-38:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDuplicate
Cmd/Ctrl+Shift+Fcondition —onToggleFocusModeis dead code.Lines 28-32 and 34-38 test the identical condition (
mod && e.shiftKey && e.key.toLowerCase() === 'f'). The first branchreturns, so the focus-mode toggle never fires. Focus mode presumably wants a different chord (e.g.Cmd+Shift+Enteror a dedicated key) so it doesn't collide with folder search.🐛 Suggested fix (pick the intended chord)
if (mod && e.shiftKey && e.key.toLowerCase() === 'f') { e.preventDefault(); onOpenFolderSearch(); return; } - if (mod && e.shiftKey && e.key.toLowerCase() === 'f') { + if (mod && e.shiftKey && e.key.toLowerCase() === 'd') { e.preventDefault(); onToggleFocusMode(); return; }🤖 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 shortcut check uses the same condition for both onOpenFolderSearch and onToggleFocusMode so the second branch is unreachable; update the second condition in the key handler inside useShortcuts.ts to use a distinct chord (e.g. mod && e.shiftKey && e.key === 'Enter' for Cmd/Ctrl+Shift+Enter) instead of repeating mod && e.shiftKey && e.key.toLowerCase() === 'f', and ensure the onToggleFocusMode call is bound to that new condition so focus mode can be triggered correctly.apps/main-processor/src/ipc.ts (3)
206-217:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThis folder allow-list is still renderer-controlled.
This check is ineffective while
READ_FOLDERon Lines 118-125 still inserts any validatedfolderPathintoallowedFolderRootsbefore returning it. A compromised renderer can callREAD_FOLDERfor an arbitrary directory once, thenSEARCH_FOLDERpasses here. Registration needs to stay in user-approved flows;READ_FOLDERshould validate membership, not grant it.As per coding guidelines, Electron main-process code must not expose unrestricted filesystem access.
🤖 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 206 - 217, The SEARCH_FOLDER handler (IPC_CONSTANTS.SEARCH_FOLDER) trusts allowedFolderRoots which is currently populated by the READ_FOLDER flow, letting a renderer grant itself access; instead, stop granting filesystem access from READ_FOLDER and enforce owner-approved registration here by validating that safeFolderPath is a pre-approved root or by requiring an explicit user-approval token/flow before adding to allowedFolderRoots; specifically, remove/stop the code that inserts into allowedFolderRoots in the READ_FOLDER handler and change SEARCH_FOLDER (which uses validateSender, resolveDirectoryPath and allowedFolderRoots) to only accept paths already pre-approved by the user-approved registration mechanism or verify an approval token/flag included in the IPC call before calling searchFolder.
29-35:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't let
READ_FILEmint watch authorization.Line 34 adds every
READ_FILEtarget intoallowedMarkdownFiles, butREAD_FILEstill resolves any renderer-supplied markdown path. A compromised renderer can self-authorize an arbitrary.mdfile by reading it once and thenWATCH_FILEit. Populate this set only from user-approved open flows, or makeREAD_FILErequire prior authorization instead of granting it.As per coding guidelines, Electron main-process code must not expose unrestricted filesystem access.
🤖 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 29 - 35, The READ_FILE handler currently adds any resolved path to allowedMarkdownFiles (IPC_CONSTANTS.READ_FILE), which lets a renderer self-authorize files; remove the automatic allowedMarkdownFiles.add(safeFilePath) from the ipcMain.handle for READ_FILE and instead only populate allowedMarkdownFiles from explicit user-approved flows (e.g. the open-file dialog handler) or require an explicit prior authorization check before adding; update resolveMarkdownFilePath/validateSender/readFile usage so READ_FILE only reads the resolved path without granting watch permission and move the authorization grant into the user-confirmed open flow or a new authorizeFile API that records allowedMarkdownFiles.
211-213:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
isPathInside()instead of a hard-coded'/'prefix check.Line 212 is POSIX-only: on Windows
realpath()returnsC:\..., sosafeFolderPath.startsWith(\${root}/`)` never matches nested folders under an allowed root. Reuse the existing path helper here.Suggested fix
import { validatePath, validateSender, allowedFolderRoots, allowedMarkdownFiles, + isPathInside, } from './utils/constants/ipc-validation'; @@ - const isAllowed = Array.from(allowedFolderRoots).some( - (root) => safeFolderPath === root || safeFolderPath.startsWith(`${root}/`) - ); + const isAllowed = Array.from(allowedFolderRoots).some((root) => + isPathInside(safeFolderPath, root) + );As per coding guidelines, file/folder access must guard path traversal.
🤖 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 211 - 213, The check computing isAllowed using safeFolderPath.startsWith(`${root}/`) is POSIX-only and should be replaced with the project path helper to correctly handle Windows paths; update the isAllowed logic (where allowedFolderRoots and safeFolderPath are used) to call the existing isPathInside(root, safeFolderPath) (or isPathInside(safeFolderPath, root) depending on helper signature) instead of the string startsWith check, and add the necessary import for isPathInside from the path helper module so nested folders under a Windows drive letter are correctly recognized.apps/main-processor/src/utils/constants/ipc-validation.ts (1)
15-22:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPin IPC sender validation to exact origins.
Line 18 currently trusts any
file:URL, and Line 22 trusts anyhttp://localhost:*origin. That lets a navigated renderer loaded from an arbitrary local HTML file or a different localhost port call privileged IPC. Compare against the packaged app URL and the one dev-server origin instead of only checking protocol/hostname.As per coding guidelines, Electron main-process IPC handlers must validate renderer input.
🤖 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` around lines 15 - 22, The current validation accepts any file: URL and any http://localhost host by only checking parsedUrl.protocol and parsedUrl.hostname; instead, replace those loose checks in the function that parses the incoming URL (the parsedUrl variable usage) with exact origin comparisons against the app's known allowed origins (the packaged app origin and the dev-server origin), e.g., construct parsedUrl.origin and compare it strictly to a PACKAGED_APP_ORIGIN and DEV_SERVER_ORIGIN constants (including port) rather than protocol/hostname alone; ensure the code returns true only when parsedUrl.origin === PACKAGED_APP_ORIGIN or parsedUrl.origin === DEV_SERVER_ORIGIN, otherwise return false.
🤖 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/ipc.ts`:
- Around line 206-217: The SEARCH_FOLDER handler (IPC_CONSTANTS.SEARCH_FOLDER)
trusts allowedFolderRoots which is currently populated by the READ_FOLDER flow,
letting a renderer grant itself access; instead, stop granting filesystem access
from READ_FOLDER and enforce owner-approved registration here by validating that
safeFolderPath is a pre-approved root or by requiring an explicit user-approval
token/flow before adding to allowedFolderRoots; specifically, remove/stop the
code that inserts into allowedFolderRoots in the READ_FOLDER handler and change
SEARCH_FOLDER (which uses validateSender, resolveDirectoryPath and
allowedFolderRoots) to only accept paths already pre-approved by the
user-approved registration mechanism or verify an approval token/flag included
in the IPC call before calling searchFolder.
- Around line 29-35: The READ_FILE handler currently adds any resolved path to
allowedMarkdownFiles (IPC_CONSTANTS.READ_FILE), which lets a renderer
self-authorize files; remove the automatic
allowedMarkdownFiles.add(safeFilePath) from the ipcMain.handle for READ_FILE and
instead only populate allowedMarkdownFiles from explicit user-approved flows
(e.g. the open-file dialog handler) or require an explicit prior authorization
check before adding; update resolveMarkdownFilePath/validateSender/readFile
usage so READ_FILE only reads the resolved path without granting watch
permission and move the authorization grant into the user-confirmed open flow or
a new authorizeFile API that records allowedMarkdownFiles.
- Around line 211-213: The check computing isAllowed using
safeFolderPath.startsWith(`${root}/`) is POSIX-only and should be replaced with
the project path helper to correctly handle Windows paths; update the isAllowed
logic (where allowedFolderRoots and safeFolderPath are used) to call the
existing isPathInside(root, safeFolderPath) (or isPathInside(safeFolderPath,
root) depending on helper signature) instead of the string startsWith check, and
add the necessary import for isPathInside from the path helper module so nested
folders under a Windows drive letter are correctly recognized.
In `@apps/main-processor/src/utils/constants/ipc-validation.ts`:
- Around line 15-22: The current validation accepts any file: URL and any
http://localhost host by only checking parsedUrl.protocol and
parsedUrl.hostname; instead, replace those loose checks in the function that
parses the incoming URL (the parsedUrl variable usage) with exact origin
comparisons against the app's known allowed origins (the packaged app origin and
the dev-server origin), e.g., construct parsedUrl.origin and compare it strictly
to a PACKAGED_APP_ORIGIN and DEV_SERVER_ORIGIN constants (including port) rather
than protocol/hostname alone; ensure the code returns true only when
parsedUrl.origin === PACKAGED_APP_ORIGIN or parsedUrl.origin ===
DEV_SERVER_ORIGIN, otherwise return false.
In `@apps/renderer/src/hooks/useShortcuts.ts`:
- Around line 28-38: The duplicate shortcut check uses the same condition for
both onOpenFolderSearch and onToggleFocusMode so the second branch is
unreachable; update the second condition in the key handler inside
useShortcuts.ts to use a distinct chord (e.g. mod && e.shiftKey && e.key ===
'Enter' for Cmd/Ctrl+Shift+Enter) instead of repeating mod && e.shiftKey &&
e.key.toLowerCase() === 'f', and ensure the onToggleFocusMode call is bound to
that new condition so focus mode can be triggered correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: be8e8b73-8604-46f3-bea4-f29b7ac0198c
📒 Files selected for processing (9)
apps/main-processor/src/ipc.tsapps/main-processor/src/utils/constants/ipc-validation.tsapps/main-processor/src/utils/helper/ipc-path-resolver.tsapps/renderer/src/App.tsxapps/renderer/src/components/FileBrowser.tsxapps/renderer/src/components/TabBar.tsxapps/renderer/src/hooks/useShortcuts.tsapps/renderer/src/types/component-types.tsapps/renderer/src/utils/constants/icon-contants.tsx
💤 Files with no reviewable changes (1)
- apps/renderer/src/components/FileBrowser.tsx
📜 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 (8)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (README.md)
Use TypeScript as the primary language for the application
Files:
apps/renderer/src/App.tsxapps/renderer/src/types/component-types.tsapps/renderer/src/hooks/useShortcuts.tsapps/renderer/src/utils/constants/icon-contants.tsxapps/renderer/src/components/TabBar.tsxapps/main-processor/src/utils/helper/ipc-path-resolver.tsapps/main-processor/src/ipc.tsapps/main-processor/src/utils/constants/ipc-validation.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/App.tsxapps/renderer/src/types/component-types.tsapps/renderer/src/hooks/useShortcuts.tsapps/renderer/src/utils/constants/icon-contants.tsxapps/renderer/src/components/TabBar.tsx
apps/renderer/**/*.{ts,tsx,css}
📄 CodeRabbit inference engine (README.md)
Use Tailwind CSS for styling
Files:
apps/renderer/src/App.tsxapps/renderer/src/types/component-types.tsapps/renderer/src/hooks/useShortcuts.tsapps/renderer/src/utils/constants/icon-contants.tsxapps/renderer/src/components/TabBar.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/App.tsxapps/renderer/src/types/component-types.tsapps/renderer/src/hooks/useShortcuts.tsapps/renderer/src/utils/constants/icon-contants.tsxapps/renderer/src/components/TabBar.tsx
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.tsxapps/renderer/src/utils/constants/icon-contants.tsxapps/renderer/src/components/TabBar.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/constants/icon-contants.tsx
apps/main-processor/**/*.{ts,tsx}
📄 CodeRabbit inference engine (README.md)
Use Marked for parsing Markdown content
Files:
apps/main-processor/src/utils/helper/ipc-path-resolver.tsapps/main-processor/src/ipc.tsapps/main-processor/src/utils/constants/ipc-validation.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/ipc-path-resolver.tsapps/main-processor/src/ipc.tsapps/main-processor/src/utils/constants/ipc-validation.ts
🔇 Additional comments (5)
apps/renderer/src/components/TabBar.tsx (1)
4-4: LGTM!Also applies to: 52-61
apps/renderer/src/types/component-types.ts (1)
111-111: LGTM!apps/renderer/src/utils/constants/icon-contants.tsx (1)
158-169: LGTM!Also applies to: 172-172
apps/renderer/src/App.tsx (1)
165-165: LGTM!apps/renderer/src/hooks/useShortcuts.ts (1)
76-86: LGTM! Themodgate pluspreventDefault()correctly stops[/\from firing while the user is just typing those characters.
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 (1)
apps/main-processor/src/file.ts (1)
39-56:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRace condition: timeout can resolve the promise, then error rejects it → unhandled rejection.
If the 1000ms timeout fires (line 40), the promise resolves. If the watcher then emits
'error'(line 47), the handler callsreject(watcherError)on an already-resolved promise, creating an unhandled rejection in the main process.Additionally, if the timeout fires but the watcher is not actually ready, the
change/unlinkhandlers are attached to a watcher that hasn't completed its initial scan, potentially missing events.🔒 Proposed fix: resolve/reject only once
currentWatchers.set(filePath, watcher); await new Promise<void>((resolve, reject) => { + let settled = false; const timeout = setTimeout(() => { + if (settled) return; + settled = true; resolve(); }, 1000); watcher.once('ready', () => { + if (settled) return; + settled = true; clearTimeout(timeout); resolve(); }); watcher.once('error', (error) => { + if (settled) return; + settled = true; clearTimeout(timeout); const watcherError = error instanceof Error ? error : new Error(String(error)); void unWatchFile(filePath) .catch(() => {}) .finally(() => onError?.(watcherError)); reject(watcherError); }); });🤖 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 39 - 56, The promise can be resolved by the 1s timeout and then later rejected by the watcher's 'error' handler causing unhandled rejections and missed events; add a single "settled" boolean (e.g., let settled = false) and guard both the timeout callback and the watcher.once('ready') and watcher.once('error') handlers to only call resolve/reject when settled is false, set settled = true immediately before resolving or rejecting, and remove/cleanup the other listeners after settling; also ensure any change/unlink handlers are attached only inside the watcher.once('ready') callback (not before ready) and call unWatchFile/onError only when rejecting and after marking settled to avoid double cleanup.
🤖 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/file.ts`:
- Around line 39-56: The promise can be resolved by the 1s timeout and then
later rejected by the watcher's 'error' handler causing unhandled rejections and
missed events; add a single "settled" boolean (e.g., let settled = false) and
guard both the timeout callback and the watcher.once('ready') and
watcher.once('error') handlers to only call resolve/reject when settled is
false, set settled = true immediately before resolving or rejecting, and
remove/cleanup the other listeners after settling; also ensure any change/unlink
handlers are attached only inside the watcher.once('ready') callback (not before
ready) and call unWatchFile/onError only when rejecting and after marking
settled to avoid double cleanup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f543ae85-2383-477b-ae5c-37ca4f514ca9
📒 Files selected for processing (1)
apps/main-processor/src/file.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 (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (README.md)
Use TypeScript as the primary language for the application
Files:
apps/main-processor/src/file.ts
apps/main-processor/**/*.{ts,tsx}
📄 CodeRabbit inference engine (README.md)
Use Marked for parsing Markdown content
Files:
apps/main-processor/src/file.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/file.ts
🔇 Additional comments (1)
apps/main-processor/src/file.ts (1)
74-78: LGTM!
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 (3)
apps/renderer/src/components/TabBar.tsx (1)
56-65:⚠️ Potential issue | 🟠 Major | ⚡ Quick win"Open new tab" button lives inside
role="tablist".A
tablistmust only containrole="tab"children. Putting the+button (and arguably the per-tab close buttons) inside it confuses AT and breaks tab navigation semantics. Move the open-new-tab control outside the tablist container.♻️ Suggested structure
})} - <div className="flex shrink-0 items-center pl-1"> - <button - type="button" - onClick={plusOpen} - aria-label="Open new tab" - className="flex h-7 w-7 items-center justify-center rounded-md text-text-muted transition-all hover:bg-accent-bg hover:text-text-base" - > - <Icons.Plus size={16} /> - </button> - </div> </div> + <div className="flex shrink-0 items-center pl-1"> + <button + type="button" + onClick={plusOpen} + aria-label="Open new tab" + className="flex h-7 w-7 items-center justify-center rounded-md text-text-muted transition-all hover:bg-accent-bg hover:text-text-base" + > + <Icons.Plus size={16} /> + </button> + </div>Wrap the tablist + control in a shared flex parent.
🤖 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 56 - 65, The plus-button currently rendered inside the element using role="tablist" in the TabBar component violates ARIA tablist semantics; move the <button> that calls plusOpen (the "Open new tab" control) out of the role="tablist" container and place it as a sibling inside a shared flex parent so the tablist only contains role="tab" children; also ensure any per-tab close buttons are not direct children of the tablist (keep them either inside each tab element with proper role or moved outside similarly).apps/renderer/__tests__/components/TabBar.test.tsx (1)
47-62:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd coverage for the new "Open new tab" control.
The PR introduces the
plusOpenbutton but no test exercises it. Add a case asserting the callback fires.As per coding guidelines: "Write unit tests for all new features to maintain code quality."💚 Suggested test
+ it('invokes plusOpen when the open-new-tab button is clicked', () => { + const plusOpen = vi.fn(); + render( + <TabBar tabs={tabs} activeTabId="tab-1" onSwitch={() => {}} onClose={() => {}} plusOpen={plusOpen} /> + ); + fireEvent.click(screen.getByRole('button', { name: /open new tab/i })); + expect(plusOpen).toHaveBeenCalledTimes(1); + });🤖 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` around lines 47 - 62, Add a unit test in TabBar.test.tsx that asserts the new "Open new tab" control triggers its callback: render the TabBar component (same pattern as other tests) passing a mock callback for the new open-tab handler (e.g., onPlusOpen/onOpenNewTab whichever prop the component exposes), find the plus/open-new-tab button by role 'button' and accessible name (match /open new tab|new tab|add tab/i), simulate a click, and expect the mock to have been called once; ensure you use the same TabBar props shape (tabs, activeTabId, onSwitch, onClose) as the other tests.apps/renderer/src/components/SettingsPanel.tsx (1)
86-96:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix the Custom CSS textarea labeling structure.
The textarea uses
aria-labelledby="custom-css-title"(line 92), but this points to a screen-reader-only span (line 96) that comes after the textarea and contains different text ("Custom CSS input editor") than the visible label ("Custom CSS", line 87). The visible label on line 87 has noid, so it isn't programmatically associated with the textarea.This creates confusion: screen reader users hear "Custom CSS input editor" while sighted users see "Custom CSS", and the standard
<label>pattern is bypassed.♻️ Recommended fix: use standard label pattern
- <label className="block"> + <label htmlFor="custom-css-input" className="block"> <span className="mb-2 block text-sm font-medium text-text-base">Custom CSS</span> <textarea + id="custom-css-input" value={settings.customCss} onChange={(event) => onChange({ customCss: event.target.value })} rows={8} - aria-labelledby="custom-css-title" className="w-full resize-y rounded border border-border-theme bg-surface p-3 font-mono text-sm text-text-base outline-none focus:ring-2 focus:ring-accent" placeholder="Add custom reader CSS here " /> - <span id="custom-css-title" className="sr-only">Custom CSS input editor</span> </label>As per coding guidelines, interactive controls need semantic elements and accessible markup.
🤖 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 86 - 96, The textarea's accessible labeling is incorrect: give the visible label and the textarea a programmatic association (e.g., add an id to the visible label or add an id to the textarea and reference it from the label via htmlFor) and remove/match the separate sr-only span so screen readers hear the same text as sighted users; specifically update the <label> element and the textarea (which currently uses aria-labelledby="custom-css-title") so the textarea's label comes before it and the id used by aria-labelledby or htmlFor matches the visible label text (adjust or remove the sr-only span with id "custom-css-title" accordingly) while keeping existing props like value={settings.customCss} and onChange 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 `@apps/renderer/src/components/DragDrop.tsx`:
- Line 7: In the DragDrop component fix the broken Tailwind token by separating
the merged class string in the div with role="status" (currently containing
"bg-bg/80ntext-text-base") into two valid classes: "bg-bg/80" and
"text-text-base"; update the className on that element so the overlay opacity
and text color are applied (look for the div in the DragDrop component that
contains "pointer-events-none fixed inset-0 z-50 ...").
In `@apps/renderer/src/components/SearchBar.tsx`:
- Line 77: The status element referenced by aria-describedby (statusId, e.g.
"search-status-msg") is missing an id attribute; update the status div rendered
by the SearchBar component to include id={statusId} so the input's
aria-describedby actually points to that element (ensure the same statusId
variable used in the input is applied to the status container where match
counts/messages are rendered).
---
Outside diff comments:
In `@apps/renderer/__tests__/components/TabBar.test.tsx`:
- Around line 47-62: Add a unit test in TabBar.test.tsx that asserts the new
"Open new tab" control triggers its callback: render the TabBar component (same
pattern as other tests) passing a mock callback for the new open-tab handler
(e.g., onPlusOpen/onOpenNewTab whichever prop the component exposes), find the
plus/open-new-tab button by role 'button' and accessible name (match /open new
tab|new tab|add tab/i), simulate a click, and expect the mock to have been
called once; ensure you use the same TabBar props shape (tabs, activeTabId,
onSwitch, onClose) as the other tests.
In `@apps/renderer/src/components/SettingsPanel.tsx`:
- Around line 86-96: The textarea's accessible labeling is incorrect: give the
visible label and the textarea a programmatic association (e.g., add an id to
the visible label or add an id to the textarea and reference it from the label
via htmlFor) and remove/match the separate sr-only span so screen readers hear
the same text as sighted users; specifically update the <label> element and the
textarea (which currently uses aria-labelledby="custom-css-title") so the
textarea's label comes before it and the id used by aria-labelledby or htmlFor
matches the visible label text (adjust or remove the sr-only span with id
"custom-css-title" accordingly) while keeping existing props like
value={settings.customCss} and onChange intact.
In `@apps/renderer/src/components/TabBar.tsx`:
- Around line 56-65: The plus-button currently rendered inside the element using
role="tablist" in the TabBar component violates ARIA tablist semantics; move the
<button> that calls plusOpen (the "Open new tab" control) out of the
role="tablist" container and place it as a sibling inside a shared flex parent
so the tablist only contains role="tab" children; also ensure any per-tab close
buttons are not direct children of the tablist (keep them either inside each tab
element with proper role or moved outside similarly).
🪄 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: 6d15452e-e374-4e63-8906-38dcd71d341b
📒 Files selected for processing (13)
apps/renderer/__tests__/components/TabBar.test.tsxapps/renderer/src/components/DragDrop.tsxapps/renderer/src/components/Error.tsxapps/renderer/src/components/FileBrowser.tsxapps/renderer/src/components/FileTree.tsxapps/renderer/src/components/Loading.tsxapps/renderer/src/components/ReaderToolbar.tsxapps/renderer/src/components/SearchBar.tsxapps/renderer/src/components/SettingsPanel.tsxapps/renderer/src/components/StatusBar.tsxapps/renderer/src/components/TabBar.tsxapps/renderer/src/components/Toast.tsxapps/renderer/src/components/UpdateBanner.tsx
📜 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 (7)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (README.md)
Use TypeScript as the primary language for the application
Files:
apps/renderer/src/components/Toast.tsxapps/renderer/src/components/Loading.tsxapps/renderer/__tests__/components/TabBar.test.tsxapps/renderer/src/components/ReaderToolbar.tsxapps/renderer/src/components/Error.tsxapps/renderer/src/components/SettingsPanel.tsxapps/renderer/src/components/UpdateBanner.tsxapps/renderer/src/components/DragDrop.tsxapps/renderer/src/components/FileBrowser.tsxapps/renderer/src/components/TabBar.tsxapps/renderer/src/components/StatusBar.tsxapps/renderer/src/components/FileTree.tsxapps/renderer/src/components/SearchBar.tsx
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/components/Toast.tsxapps/renderer/src/components/Loading.tsxapps/renderer/__tests__/components/TabBar.test.tsxapps/renderer/src/components/ReaderToolbar.tsxapps/renderer/src/components/Error.tsxapps/renderer/src/components/SettingsPanel.tsxapps/renderer/src/components/UpdateBanner.tsxapps/renderer/src/components/DragDrop.tsxapps/renderer/src/components/FileBrowser.tsxapps/renderer/src/components/TabBar.tsxapps/renderer/src/components/StatusBar.tsxapps/renderer/src/components/FileTree.tsxapps/renderer/src/components/SearchBar.tsx
apps/renderer/**/*.{ts,tsx,css}
📄 CodeRabbit inference engine (README.md)
Use Tailwind CSS for styling
Files:
apps/renderer/src/components/Toast.tsxapps/renderer/src/components/Loading.tsxapps/renderer/__tests__/components/TabBar.test.tsxapps/renderer/src/components/ReaderToolbar.tsxapps/renderer/src/components/Error.tsxapps/renderer/src/components/SettingsPanel.tsxapps/renderer/src/components/UpdateBanner.tsxapps/renderer/src/components/DragDrop.tsxapps/renderer/src/components/FileBrowser.tsxapps/renderer/src/components/TabBar.tsxapps/renderer/src/components/StatusBar.tsxapps/renderer/src/components/FileTree.tsxapps/renderer/src/components/SearchBar.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/components/Toast.tsxapps/renderer/src/components/Loading.tsxapps/renderer/src/components/ReaderToolbar.tsxapps/renderer/src/components/Error.tsxapps/renderer/src/components/SettingsPanel.tsxapps/renderer/src/components/UpdateBanner.tsxapps/renderer/src/components/DragDrop.tsxapps/renderer/src/components/FileBrowser.tsxapps/renderer/src/components/TabBar.tsxapps/renderer/src/components/StatusBar.tsxapps/renderer/src/components/FileTree.tsxapps/renderer/src/components/SearchBar.tsx
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/Toast.tsxapps/renderer/src/components/Loading.tsxapps/renderer/src/components/ReaderToolbar.tsxapps/renderer/src/components/Error.tsxapps/renderer/src/components/SettingsPanel.tsxapps/renderer/src/components/UpdateBanner.tsxapps/renderer/src/components/DragDrop.tsxapps/renderer/src/components/FileBrowser.tsxapps/renderer/src/components/TabBar.tsxapps/renderer/src/components/StatusBar.tsxapps/renderer/src/components/FileTree.tsxapps/renderer/src/components/SearchBar.tsx
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (README.md)
Use Vitest for testing
Files:
apps/renderer/__tests__/components/TabBar.test.tsx
⚙️ 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/renderer/__tests__/components/TabBar.test.tsx
**/*.{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/renderer/__tests__/components/TabBar.test.tsx
🔇 Additional comments (13)
apps/renderer/src/components/UpdateBanner.tsx (1)
18-18: LGTM!Also applies to: 28-28
apps/renderer/__tests__/components/TabBar.test.tsx (1)
34-34: LGTM!Also applies to: 52-52
apps/renderer/src/components/Error.tsx (1)
6-9: LGTM!apps/renderer/src/components/Loading.tsx (1)
4-6: LGTM!apps/renderer/src/components/Toast.tsx (1)
16-16: LGTM!apps/renderer/src/components/TabBar.tsx (2)
24-40: ⚡ Quick winFix TabBar tab a11y wiring (aria-controls + arrow-key navigation).
- Ensure
aria-controls={tabpanel-${tab.id}}points to an element that actually hasid={tabpanel-${tab.id}}androle="tabpanel".- Add
ArrowLeft/ArrowRight(and ideallyHome/End) key handling on the tablist so keyboard users can move between tabs—current rovingtabIndexis set but there’s no arrow-key navigation.
4-4: EnsureplusOpenis optional (or defaults) inTabBarPropsif tests renderTabBarwithout it.
TabBarassignsonClick={plusOpen}; ifplusOpenis required inTabBarProps, any render that omits it will fail type-check in tests/TS. Checkapps/renderer/src/types/component-types.tsforTabBarPropsand makeplusOpenoptional or handle missing value inTabBar.apps/renderer/src/components/FileBrowser.tsx (1)
14-14: LGTM!apps/renderer/src/components/FileTree.tsx (1)
10-11: LGTM!Also applies to: 36-36, 46-46
apps/renderer/src/components/ReaderToolbar.tsx (1)
14-14: LGTM!Also applies to: 16-16, 24-27, 32-32, 39-39, 41-41
apps/renderer/src/components/SearchBar.tsx (1)
78-78: LGTM!Also applies to: 116-116, 126-126, 128-128, 149-149
apps/renderer/src/components/SettingsPanel.tsx (1)
57-74: LGTM!Also applies to: 99-99
apps/renderer/src/components/StatusBar.tsx (1)
10-10: LGTM!Also applies to: 12-12, 21-23, 31-31
2 participants
Description
This PR fixes ipc clean up issues and package override issues.
Type of Change
Checklist
Electron main/preload
SEARCH_FOLDERresolves the folder and rejects requests unless the path is already authorized (no longer auto-adds resolved folder paths toallowedFolderRoots).validateSendernow parsesevent.senderFrame?.urland accepts onlyfile:URLs orhttp:with hostnamelocalhost(removed startsWith whitelist).watchFilestores the watcher before awaiting readiness, centralizes error handling (normalizes errors, unwatches on error), and ensuresonDeletedruns even if unwatch fails.apiContract.onUpdateAvailableregisters a named handler and now returns an unsubscribe function for proper removal.Renderer UI
window.api.onUpdateAvailableand uses the returned unsubscribe for cleanup; adds ARIA attributes.TabBaraccepts a newplusOpenprop and renders a plus button that calls it to open files; accessibility attributes added to tabs and close buttons.useShortcutsnow requires modifier key (mod) for[and\shortcuts and prevents default for[(behavior changed).requestIdto ignore stale in-flight results.settings.fontSizeand include it in dependencies.Markdown rendering
exportDOCXinlines referenced images before building document markup.BrowserWindowuseswebPreferences.sandbox: true.sanitizeCssrepeatedly strips dangerous CSS patterns until a fixed point is reached.Shared packages
packages/shared-typesupdatesMarkdownReaderAPI.onUpdateAvailableto return an unsubscribe function() => voidinstead ofvoid.Tests
apps/renderer/setup.tsmockswindow.apiusing Vitestviwith asyncgetSettings/saveSettings.useSettingstests useawait act(async ...);TabBartests updated to match new accessibility attributes.Tooling/CI
pnpmversion from.github/workflows/production.yml(now relies on the action default for pnpm)..github/workflows/release.ymladdsenv: HUSKY=0to disable Husky during workflow runs.serialize-javascriptoverride bumped from>=7.0.3to>=7.0.5.Docs
Packaging
Breaking changes:
onUpdateAvailableAPI (shared types + preload) now returns an unsubscribe function() => void; callers expectingvoidmust handle/use the returned unsubscriber.SEARCH_FOLDERIPC behavior changed: folder paths are no longer auto-registered; requests for folder paths not already present inallowedFolderRootsare rejected.[and\now require the modifier key (mod); existing shortcut behavior for these keys has changed.