feat: Production readiness & security hardening#15
Merged
Conversation
Contributor
|
Warning Review limit reached
More reviews will be available in 11 minutes and 10 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (20)
📝 WalkthroughWalkthroughThis PR migrates OpenAI API key storage from the renderer (localStorage/safeStorage) to the Electron main process via new IPC handlers, extracts the CodeMirror editor into a standalone ChangesApp feature changes
CI/CD and repository docs
Sequence Diagram(s)sequenceDiagram
rect rgba(173, 216, 230, 0.5)
Note over Renderer,configEnc: API Key Setup Flow
Renderer->>Preload: setApiKey(key)
Preload->>MainProcess: IPC set-api-key
MainProcess->>MainProcess: safeStorage.encryptString(key)
MainProcess->>configEnc: fs.writeFileSync config.enc
MainProcess-->>Renderer: true
end
rect rgba(144, 238, 144, 0.5)
Note over Editor,ElectronAPI: /ai Command Flow
Editor->>useEditorExtensions: Enter keypress on /ai line
useEditorExtensions->>ElectronAPI: getApiKeyStatus()
ElectronAPI-->>useEditorExtensions: true
useEditorExtensions->>ElectronAPI: openAIChat(model, messages, baseURL)
ElectronAPI->>MainProcess: IPC openai-chat (uses memoryApiKey)
MainProcess-->>ElectronAPI: completion text
ElectronAPI-->>useEditorExtensions: response
useEditorExtensions->>Editor: replace thinking marker with completion
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
Contributor
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 (2)
src/App.tsx (2)
69-78:⚠️ Potential issue | 🔴 CriticalAdd missing
useAIStoreimport.Line 70 calls
useAIStore.setState(...)butuseAIStoreis not imported. This will cause aReferenceErrorat runtime. Add the import:import { useAIStore } from './store/useAIStore'🤖 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 `@src/App.tsx` around lines 69 - 78, The useAIStore store is being accessed via useAIStore.setState() in the App.tsx file but the import statement is missing, which will cause a ReferenceError at runtime. Add the missing import for useAIStore from the store directory at the top of the App.tsx file so that the useAIStore reference is properly available when the setState call is executed.
107-107:⚠️ Potential issue | 🔴 CriticalAdd missing variables to
useSettingsStoredestructuring at line 32.
themePreset,showRulings,textColor, andnumColorare used at lines 107, 116, 153, and 155, but are not destructured fromuseSettingsStore. Update the destructuring:Current code (line 32)
const { fontFamily, bgType, bgColor, bgImage } = useSettingsStore()Should include:
themePreset,showRulings,textColor,numColor.🤖 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 `@src/App.tsx` at line 107, The variables themePreset, showRulings, textColor, and numColor are being used throughout the component but are not being destructured from the useSettingsStore hook call at line 32. Update the destructuring assignment from useSettingsStore to include these four missing variables alongside the existing fontFamily, bgType, bgColor, and bgImage variables. This will ensure these variables are properly defined and available for use in the className attribute and other locations where they appear.
🧹 Nitpick comments (6)
SECURITY.md (1)
9-12: ⚡ Quick winUse a role-based security contact instead of a personal inbox.
Publishing a personal Gmail address makes the reporting channel harder to rotate and exposes an individual inbox unnecessarily. A dedicated
security@alias (or similar intake) fits the private-reporting policy better.🤖 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 `@SECURITY.md` around lines 9 - 12, Replace the personal email address adityasharma.variable@gmail.com in the SECURITY.md file with a dedicated role-based security contact email address such as security@yourdomain.com or a similar alias. This provides better security practices by creating a team-managed inbox rather than exposing an individual's personal email, and allows for easier rotation and management of the security reporting channel.src/components/Editor.tsx (2)
10-10: 💤 Low valueConsider typing
dispatchparameter andeditorRefmore strictly.Using
anyfor the transaction parameter and editor ref loses type safety. The CodeMirror types could provide better autocomplete and catch errors.♻️ Optional type improvements
+import type { TransactionSpec } from '`@codemirror/state`' +import type { ReactCodeMirrorRef } from '`@uiw/react-codemirror`' export interface EditorRef { - dispatch: (tx: any) => void + dispatch: (tx: TransactionSpec) => void focus: () => void } -const editorRef = useRef<any>(null) +const editorRef = useRef<ReactCodeMirrorRef>(null)Also applies to: 22-22
🤖 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 `@src/components/Editor.tsx` at line 10, The dispatch function parameter uses any for the transaction type and the editorRef also lacks proper typing, which reduces type safety and IDE autocomplete support. Replace the any type in the dispatch function signature with the appropriate CodeMirror transaction type, and similarly update the editorRef type annotation to use the proper CodeMirror editor reference type instead of any to ensure better type safety and development experience.
37-53: ⚡ Quick winStale closure risk:
notesarray captured in callback may be outdated during async operations.The
handleEditorChangecallback closes overnotes, but between the callback creation and execution, the notes array could change (e.g., from external updates). Since the callback spreads and mutates based onnotes[currentNoteIndex], you may overwrite changes made by other sources.Consider using the functional update pattern with
setNotesor reading fresh state viauseAppStore.getState()inside the callback, similar to howextensions.tsdoes it.♻️ Suggested approach
const handleEditorChange = useCallback( (val: string, viewUpdate?: ViewUpdate) => { - const updatedNotes = [...notes] - if (updatedNotes[currentNoteIndex]) { - updatedNotes[currentNoteIndex].content = val - setNotes(updatedNotes) - window.electronAPI.saveNote(activeNote.id, val) + const currentNotes = useAppStore.getState().notes + const currentIndex = useAppStore.getState().currentNoteIndex + const note = currentNotes[currentIndex] + if (note) { + setNotes((prev) => { + const updated = [...prev] + if (updated[currentIndex]) { + updated[currentIndex] = { ...updated[currentIndex], content: val } + } + return updated + }) + window.electronAPI.saveNote(note.id, val) }🤖 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 `@src/components/Editor.tsx` around lines 37 - 53, The handleEditorChange callback captures the notes array from its closure, which can become stale during async operations or external state updates, causing potential loss of concurrent changes. Instead of spreading the notes array directly in the callback, refactor it to use a functional update pattern by passing a callback function to setNotes that receives the current state as a parameter, ensuring you always work with the most up-to-date notes array. This way, you read fresh state inside the callback execution rather than relying on the captured notes value, and you can remove notes and currentNoteIndex from the dependency array since the updater function will have access to current values.src/lib/editor/extensions.ts (1)
56-99: ⚡ Quick winDuplicate code:
Mod-BackspaceandMod-Deletehandlers are identical.Both handlers perform the exact same logic. Extract to a shared helper function to reduce duplication and maintenance burden.
♻️ Extract shared delete handler
+const deleteCurrentNote = (setNotes: typeof useAppStore.getState().setNotes, setCurrentNoteIndex: typeof useAppStore.getState().setCurrentNoteIndex) => { + const note = useAppStore.getState().notes[useAppStore.getState().currentNoteIndex] + if (!note) return true + if (note.id.startsWith('commands/')) { + alert('Files in the commands folder cannot be deleted.') + return true + } + if (confirm('Delete this note?')) { + window.electronAPI.deleteNote(note.id) + setNotes((prev) => prev.filter((n) => n.id !== note.id)) + if (useAppStore.getState().currentNoteIndex >= useAppStore.getState().notes.length - 1) { + setCurrentNoteIndex(Math.max(0, useAppStore.getState().notes.length - 2)) + } + } + return true +} + // Then in the keymap: { key: 'Mod-Backspace', run: () => deleteCurrentNote(setNotes, setCurrentNoteIndex) }, { key: 'Mod-Delete', run: () => deleteCurrentNote(setNotes, setCurrentNoteIndex) },🤖 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 `@src/lib/editor/extensions.ts` around lines 56 - 99, Extract the duplicate note deletion logic from both the Mod-Backspace and Mod-Delete keyboard event handlers into a single shared helper function. The helper function should encapsulate all the logic that currently appears in both run functions including the note retrieval, the commands folder check with alert, the deletion confirmation dialog, the electronAPI.deleteNote call, the setNotes state update to filter the deleted note, and the setCurrentNoteIndex logic to adjust the index if needed. Then replace the identical code blocks in both the Mod-Backspace and Mod-Delete handlers with calls to this new shared helper function.src/lib/editor/MathEvaluator.ts (1)
71-82: ⚡ Quick winPotential use-after-destroy: accessing
view.statein async callback after view may be unmounted.If the editor component unmounts between scheduling the timeout and it firing,
view.state.doc.toString()could throw or access stale state.🛡️ Add a guard for destroyed view
static triggerMathEvaluation(view: EditorView) { if (this.evalTimeout) window.clearTimeout(this.evalTimeout) this.evalTimeout = window.setTimeout(async () => { + // Guard against destroyed view + if (view.destroyed) return const docStr = view.state.doc.toString() const scope = VariableScope.getScope() const changes = await this.evaluateMathChanges(docStr, scope)🤖 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 `@src/lib/editor/MathEvaluator.ts` around lines 71 - 82, The triggerMathEvaluation method has a potential use-after-destroy issue where the async callback accesses view.state without verifying the view is still valid. If the editor component unmounts during the 300ms timeout period, accessing view.state.doc.toString() could fail. Add a guard check at the beginning of the setTimeout callback (after the arrow function opens) to verify the view is still mounted and valid before attempting to access view.state. Check if the view has a valid state property or use an appropriate method to determine if the EditorView is still active before proceeding with the docStr and scope initialization.src/Settings.tsx (1)
75-77: 💤 Low valueMissing error handling for
setApiKeyfailure and no way to clear the key.Two concerns:
- If
setApiKeyreturnsfalse, the user receives no feedback that the save failed.- Users cannot clear an existing API key - they can only replace it. If
apiKeyis empty, the key persists unchanged.Consider handling the failure case and optionally allowing key removal:
Proposed improvement
if (apiKey) { - await window.electronAPI.setApiKey(apiKey) + const success = await window.electronAPI.setApiKey(apiKey) + if (!success) { + console.error('Failed to save API key') + // Optionally show user feedback here + 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 `@src/Settings.tsx` around lines 75 - 77, The setApiKey call in the Settings component is missing error handling and cannot clear existing API keys. Modify the logic to handle both cases: when apiKey has a value, call setApiKey and check if it returns false, then show error feedback to the user; when apiKey is empty, still call setApiKey with the empty value to clear the stored key instead of skipping the operation entirely. This ensures users get feedback on save failures and can remove API keys when needed.
🤖 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/ci.yml:
- Around line 14-17: The GitHub Actions workflow references use moving version
tags (`@v4`) which poses a supply-chain security risk. Update the
actions/checkout@v4 reference to pin it to a specific commit SHA instead of the
moving tag, and similarly update the actions/setup-node@v4 reference to use a
pinned commit SHA. Additionally, add persist-credentials: false to the checkout
action configuration to prevent GitHub from persisting credentials in the
workflow, reducing credential exposure risk.
In `@CONTRIBUTING.md`:
- Line 26: The Pre-PR Checks section in the CONTRIBUTING.md file is incomplete
and only mentions npm run lint and npm run test, but CI also enforces npm run
typecheck and npm run format:check. Update the Pre-PR Checks bullet point to
include all four commands that contributors must run before opening a PR: lint,
test, typecheck, and format:check, so that contributors are aware of all
required checks and can avoid opening PRs with failing required checks.
In `@electron/main.ts`:
- Around line 656-667: In the set-api-key IPC handler, add error logging to the
catch block before returning false. Instead of silently catching and returning
false when an exception occurs during encryption or file writing, log the actual
error details from the caught exception using an appropriate logger so that disk
write failures, permission issues, or encryption problems become visible for
troubleshooting and debugging purposes.
- Around line 646-692: Remove the unused error variables from the catch blocks
in this section. In the initial try-catch block that reads from the config.enc
file and in the set-api-key IPC handler's try-catch block, change the catch
clauses from catching an unused error variable to using empty catch blocks
(remove the parameter). Additionally, review the code for any formatting issues
flagged by prettier and adjust indentation, spacing, or line breaks accordingly
to match the project's formatting standards.
- Around line 358-375: The session.defaultSession.webRequest.onHeadersReceived
callback and its CSP configuration have formatting inconsistencies that need to
be addressed. Reformat the onHeadersReceived callback to improve readability,
particularly the long CSP policy strings in the ternary operator. Break the
Content-Security-Policy header value across multiple lines if needed, ensure
consistent indentation throughout the block, and verify that spacing around
operators and function parameters follows ESLint/Prettier standards. Apply
automatic formatting tools to align the entire callback structure with the
project's linting configuration.
In `@src/lib/editor/extensions.ts`:
- Around line 120-206: The code inserts a thinking marker
(`\n\u200B...\u200C\n`) using `view.dispatch()` and later replaces it via
`docStr.replace()`, but this is fragile because if the user edits the document
while the async openAIChat call is pending, the marker position shifts and the
replacement fails silently. Instead of relying on string replacement, capture
the exact position where the marker was inserted (the `line.to` value), then use
`view.dispatch()` with a targeted change that replaces the specific range
containing the marker rather than searching for it in the modified document
string. Apply this approach consistently in all three locations where the marker
replacement currently occurs: in the response handler, the catch handler, and
the outer try-catch block.
In `@src/main.tsx`:
- Around line 36-44: The safeStorageDecrypt method returns the original
base64-encoded value on decryption failure rather than throwing or returning
empty, so the if (decrypted) check will pass even with invalid data. Add
validation logic after the safeStorageDecrypt call to ensure the decrypted value
looks like a valid API key before attempting to set it with setApiKey. Validate
characteristics such as expected prefix or reasonable length to distinguish
between actual decrypted API keys and garbage base64 data that would indicate
decryption failure.
---
Outside diff comments:
In `@src/App.tsx`:
- Around line 69-78: The useAIStore store is being accessed via
useAIStore.setState() in the App.tsx file but the import statement is missing,
which will cause a ReferenceError at runtime. Add the missing import for
useAIStore from the store directory at the top of the App.tsx file so that the
useAIStore reference is properly available when the setState call is executed.
- Line 107: The variables themePreset, showRulings, textColor, and numColor are
being used throughout the component but are not being destructured from the
useSettingsStore hook call at line 32. Update the destructuring assignment from
useSettingsStore to include these four missing variables alongside the existing
fontFamily, bgType, bgColor, and bgImage variables. This will ensure these
variables are properly defined and available for use in the className attribute
and other locations where they appear.
---
Nitpick comments:
In `@SECURITY.md`:
- Around line 9-12: Replace the personal email address
adityasharma.variable@gmail.com in the SECURITY.md file with a dedicated
role-based security contact email address such as security@yourdomain.com or a
similar alias. This provides better security practices by creating a
team-managed inbox rather than exposing an individual's personal email, and
allows for easier rotation and management of the security reporting channel.
In `@src/components/Editor.tsx`:
- Line 10: The dispatch function parameter uses any for the transaction type and
the editorRef also lacks proper typing, which reduces type safety and IDE
autocomplete support. Replace the any type in the dispatch function signature
with the appropriate CodeMirror transaction type, and similarly update the
editorRef type annotation to use the proper CodeMirror editor reference type
instead of any to ensure better type safety and development experience.
- Around line 37-53: The handleEditorChange callback captures the notes array
from its closure, which can become stale during async operations or external
state updates, causing potential loss of concurrent changes. Instead of
spreading the notes array directly in the callback, refactor it to use a
functional update pattern by passing a callback function to setNotes that
receives the current state as a parameter, ensuring you always work with the
most up-to-date notes array. This way, you read fresh state inside the callback
execution rather than relying on the captured notes value, and you can remove
notes and currentNoteIndex from the dependency array since the updater function
will have access to current values.
In `@src/lib/editor/extensions.ts`:
- Around line 56-99: Extract the duplicate note deletion logic from both the
Mod-Backspace and Mod-Delete keyboard event handlers into a single shared helper
function. The helper function should encapsulate all the logic that currently
appears in both run functions including the note retrieval, the commands folder
check with alert, the deletion confirmation dialog, the electronAPI.deleteNote
call, the setNotes state update to filter the deleted note, and the
setCurrentNoteIndex logic to adjust the index if needed. Then replace the
identical code blocks in both the Mod-Backspace and Mod-Delete handlers with
calls to this new shared helper function.
In `@src/lib/editor/MathEvaluator.ts`:
- Around line 71-82: The triggerMathEvaluation method has a potential
use-after-destroy issue where the async callback accesses view.state without
verifying the view is still valid. If the editor component unmounts during the
300ms timeout period, accessing view.state.doc.toString() could fail. Add a
guard check at the beginning of the setTimeout callback (after the arrow
function opens) to verify the view is still mounted and valid before attempting
to access view.state. Check if the view has a valid state property or use an
appropriate method to determine if the EditorView is still active before
proceeding with the docStr and scope initialization.
In `@src/Settings.tsx`:
- Around line 75-77: The setApiKey call in the Settings component is missing
error handling and cannot clear existing API keys. Modify the logic to handle
both cases: when apiKey has a value, call setApiKey and check if it returns
false, then show error feedback to the user; when apiKey is empty, still call
setApiKey with the empty value to clear the stored key instead of skipping the
operation entirely. This ensures users get feedback on save failures and can
remove API keys when needed.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0121f43a-08d6-4c04-9b0f-bc88790b9fd2
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (20)
.github/workflows/ci.yml.github/workflows/release.ymlCONTRIBUTING.mdSECURITY.mdelectron/main.tselectron/preload.tspackage.jsonsrc/App.tsxsrc/Settings.test.tsxsrc/Settings.tsxsrc/components/Editor.tsxsrc/components/ErrorBoundary.tsxsrc/lib/editor/MathEvaluator.tssrc/lib/editor/extensions.tssrc/main.tsxsrc/setupTests.tssrc/store/useAIStore.tssrc/types.d.tstests/MathEvaluator.test.tstests/safeStorage.test.ts
💤 Files with no reviewable changes (1)
- src/store/useAIStore.ts
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 5 file(s) based on 7 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken:
Lines 11–18 os: [macos-latest, ubuntu-latest, windows-latest]
runs-on: ${{ matrix.os }}
steps:
- - uses: actions/checkout@v4
- - uses: actions/setup-node@v4
+ - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
+ with:
+ persist-credentials: false
+ - uses: actions/setup-node@39370e3970a6d050c480ffad4ff0ed4d3fdee5af # v4.1.0
with:
node-version: '22'
- run: npm ci |
Fixed 5 file(s) based on 7 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
This was referenced Jun 24, 2026
Merged
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.
This PR completes a comprehensive security and architecture audit to harden PaperCache for production.
1. Build Pipeline & Repository Health
npm cifor deterministic builds.workflow_dispatch.package.jsonwith proper node engines andprivateflags.marked).SECURITY.mdandCONTRIBUTING.md.2. API Key Security & Hardening
localStorage. It's encrypted at rest via Electron'ssafeStorageand held in the main process memory.3. Main Process Hardening
unsafe-evalspecifically for mathjs, and tightening all other sources.fs.existsSync.electron-updaterwith MacOS tray checking integration.4. UI/UX Improvements & Code Organization
<ErrorBoundary>to prevent white-screen crashes.Editor.tsxandextensions.tsout ofApp.tsxfor a massively cleaner entry point.5. Automated Tests
MathEvaluatorand wrote targeted tests bypassing the DOM.safeStorageIPC bindings.Summary by CodeRabbit
npm ci, and adjusted release workflow/assets.