Dev#6
Merged
ElizabethSobiya merged 28 commits intomainfrom Apr 28, 2026
Merged
Conversation
…d update project configuration
…alog and EditorToolbar components, and improve auto-save functionality
[Feature] - Add image upload functionality with multer and integrate in editor
Add table blocks to the editor using Lexical's official @lexical/table package. Tables are insertable via the slash command menu (/table) and start as 3x3 with a styled header row. Features: - Tab/Shift+Tab and arrow key navigation between cells - Rich text formatting inside cells (bold, italic, etc.) - Add/remove rows and columns via floating action menu - Notion-inspired styling with header row highlight - Full serialization support (auto-save, restore) This contribution was developed with AI assistance (Claude Code). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Move TablePlugin.tsx from src/plugins/ to src/components/ to match CodeBlockPlugin.tsx convention - Import INSERT_TABLE_COMMAND directly from @lexical/table in SlashCommandMenu.tsx instead of re-exporting through TablePlugin - Update Editor.tsx import path accordingly
feat: add table block support via @lexical/table
[Feature] - Added page break, cleanup the folder structure for constants and interfaces
…ions for each page in the document.
…move unused insertPageBreakWithParagraph function
…d update DocumentPDF import
[Feature] - Page Break
📝 WalkthroughWalkthroughAdds backend image upload handling and static serving, introduces Lexical Image and PageBreak nodes plus table support and toolbar upload flow, centralizes editor constants, updates PDF rendering/pagination, and adds dev tooling (husky, lint-staged, ESLint flat configs, CI tweak, gitignore). Changes
Sequence Diagram(s)sequenceDiagram
participant User as User (Browser)
participant Toolbar as Editor Toolbar
participant API as API Server
participant FileSystem as File Storage
participant Editor as Lexical Editor
User->>Toolbar: Click image upload button
Toolbar->>Toolbar: Show file input
User->>Toolbar: Select image file
Toolbar->>API: POST /api/uploads (multipart/form-data)
API->>API: Validate size ≤ 10MB and mimetype image/*
API->>FileSystem: Write file as UUID + ext
API->>Toolbar: Respond { url: "/uploads/<filename>" }
Toolbar->>Editor: Insert ImageNode with url
Editor->>User: Render image in document
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 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 docstrings
🧪 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: 6
🧹 Nitpick comments (7)
apps/web/src/context/DocumentContext.ts (1)
8-11: Consider a nullable context + guard hook to catch missing provider wiring.With empty-string defaults on Line 8, consumers outside the provider fail silently instead of failing fast.
♻️ Recommended refactor
import { createContext } from 'react'; +import { useContext } from 'react'; interface DocumentContextValue { header: string; footer: string; } -export const DocumentContext = createContext<DocumentContextValue>({ - header: '', - footer: '', -}); +export const DocumentContext = createContext<DocumentContextValue | null>(null); + +export function useDocumentContext(): DocumentContextValue { + const ctx = useContext(DocumentContext); + if (!ctx) throw new Error('useDocumentContext must be used within DocumentContext.Provider'); + return ctx; +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/context/DocumentContext.ts` around lines 8 - 11, The DocumentContext currently uses empty-string defaults which masks missing provider wiring; change the context to be nullable by creating it as createContext<DocumentContextValue | null>(null) and add a guard hook (e.g., useDocumentContext or useDocument) that calls useContext(DocumentContext) and throws a clear error if the value is null, then update consumers to import and use that guard hook instead of directly accessing DocumentContext to ensure failures surface fast when the provider is not wired.apps/api/src/index.ts (1)
52-58: Consider sanitizing error messages in production.The global error handler returns
err.messagedirectly in the response. This could leak internal implementation details (e.g., database errors, file paths) to clients.🛡️ Proposed fix
app.use((err: Error, _req: Request, res: Response, _next: NextFunction) => { setCors(res); console.error(err.message); - res.status(500).json({ error: err.message }); + const isProduction = env.NODE_ENV === 'production'; + res.status(500).json({ error: isProduction ? 'Internal server error' : err.message }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/index.ts` around lines 52 - 58, The global error handler registered with app.use currently returns err.message directly to clients (and logs only err.message), which can leak internal details; change the handler in the app.use(...) block to sanitize responses in production by returning a generic message (e.g., "Internal server error") when process.env.NODE_ENV === "production" while still logging the full error internally (use console.error(err) or the existing logger) and keep setCors(res) behavior; ensure you reference the same middleware function signature and only replace the response body content based on the environment.apps/api/src/routes/uploads.ts (1)
36-43: Handle multer errors gracefully.When the file exceeds
MAX_UPLOAD_SIZE_BYTESor fails thefileFilter, multer throws an error that propagates to the global error handler. Consider adding explicit error handling to return user-friendly messages (e.g., "File too large" with status 413).🛡️ Proposed error handling
+import type { NextFunction } from 'express'; +import multer from 'multer'; export const uploadsRouter = Router(); // TODO: add requireAuth once auth UI is built -uploadsRouter.post('/', upload.single('image'), (req: Request, res: Response) => { +uploadsRouter.post('/', (req: Request, res: Response, next: NextFunction) => { + upload.single('image')(req, res, (err) => { + if (err instanceof multer.MulterError) { + if (err.code === 'LIMIT_FILE_SIZE') { + res.status(413).json({ error: 'File too large' }); + return; + } + res.status(400).json({ error: err.message }); + return; + } + if (err) { + res.status(400).json({ error: err.message }); + return; + } if (!req.file) { res.status(400).json({ error: 'No file uploaded' }); return; } res.json({ url: `/uploads/${req.file.filename}` }); + }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/routes/uploads.ts` around lines 36 - 43, Wrap the current uploadsRouter.post handler to catch Multer errors from upload.single('image') by invoking upload.single('image')(req, res, (err) => { ... }) instead of using it directly as middleware; inside the callback check for err and if err is an instance of multer.MulterError handle err.code === 'LIMIT_FILE_SIZE' by sending status 413 with message "File too large", handle other MulterError codes with 400 and a friendly message, and handle non-Multer errors by calling next(err); if no error proceed with the existing logic that checks req.file and returns the uploaded file URL. Ensure you reference the existing upload variable and uploadsRouter.post route and import/use multer.MulterError for the instanceof check.apps/web/src/components/Editor.tsx (1)
203-213: Inline styles for paper dimensions.The inline styles define precise A4 paper dimensions (794px width ≈ 210mm at 96dpi, 1123px height ≈ 297mm). While coding guidelines discourage inline styles, these specific print-layout values could be extracted to CSS classes or Tailwind arbitrary values for consistency.
♻️ Optional: Extract to Tailwind arbitrary values
-<div style={{ width: 794, margin: '0 auto' }}> +<div className="w-[794px] mx-auto"> -<div - className="bg-white" - style={{ - padding: 72, - minHeight: 1123, - boxShadow: '0 1px 3px rgba(0,0,0,0.12), 0 4px 16px rgba(0,0,0,0.10)', - }} -> +<div className="bg-white p-[72px] min-h-[1123px] shadow-[0_1px_3px_rgba(0,0,0,0.12),0_4px_16px_rgba(0,0,0,0.10)]">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/Editor.tsx` around lines 203 - 213, The inline fixed paper sizing and layout styles in the Editor component (the outer div with style {{ width: 794, margin: '0 auto' }} and the inner div with className="bg-white" and style {{ padding: 72, minHeight: 1123, boxShadow: '...'}}) should be extracted to CSS/Tailwind to follow project conventions; replace those inline values with either a named CSS class (e.g., .paper, .paper-container) or Tailwind arbitrary utilities (e.g., w-[794px] mx-auto p-[72px] min-h-[1123px] shadow-[0_1px_3px_rgba(0,0,0,0.12),0_4px_16px_rgba(0,0,0,0.10)]) and move the box-shadow and sizing into the stylesheet so the component uses className references instead of inline style objects.apps/web/src/hooks/useAutoSave.ts (1)
6-14: Consider validating parsed localStorage data.The
as AutoSaveDatacast assumes the parsed JSON matches the expected shape. Corrupted or tampered localStorage could cause runtime errors. Per coding guidelines, Zod should validate external data.🛡️ Proposed fix with Zod validation
import { STORAGE_KEY, DEBOUNCE_MS } from '../constants/autosave'; import type { AutoSaveData } from '../types/editor'; +import { z } from 'zod'; + +const AutoSaveSchema = z.object({ + title: z.string(), + content: z.string(), +}); export function loadAutoSave(): AutoSaveData | null { try { const raw = localStorage.getItem(STORAGE_KEY); if (!raw) return null; - return JSON.parse(raw) as AutoSaveData; + const parsed = JSON.parse(raw); + return AutoSaveSchema.parse(parsed); } catch { return null; } }As per coding guidelines: "Use Zod for all external data validation including API inputs and environment variables".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/hooks/useAutoSave.ts` around lines 6 - 14, The loadAutoSave function currently casts parsed JSON to AutoSaveData without validation; update loadAutoSave to parse localStorage.getItem(STORAGE_KEY) and validate the result with a Zod schema for AutoSaveData (create or reuse a zod schema named something like AutoSaveDataSchema), returning the parsed value only if AutoSaveDataSchema.parse or safeParse succeeds, otherwise return null; keep the try/catch but replace the direct "as AutoSaveData" cast with Zod validation using AutoSaveDataSchema and reference STORAGE_KEY and AutoSaveData in the fix.apps/web/src/nodes/PageBreakNode.tsx (1)
26-51: Consider extracting shared delete command registration.Both
ImageNodeandPageBreakNodehave identical delete/backspace command registration logic. This could be extracted to a shared hook to reduce duplication.♻️ Proposed shared hook
Create
apps/web/src/hooks/useDeleteOnSelection.ts:import { useEffect } from 'react'; import { $getNodeByKey, KEY_BACKSPACE_COMMAND, KEY_DELETE_COMMAND, COMMAND_PRIORITY_LOW } from 'lexical'; import { useLexicalComposerContext } from '@lexical/react/LexicalComposerContext'; import { mergeRegister } from '@lexical/utils'; export function useDeleteOnSelection(nodeKey: string, isSelected: boolean) { const [editor] = useLexicalComposerContext(); useEffect(() => { if (!isSelected) return; return mergeRegister( editor.registerCommand( KEY_BACKSPACE_COMMAND, () => { editor.update(() => { $getNodeByKey(nodeKey)?.remove(); }); return true; }, COMMAND_PRIORITY_LOW, ), editor.registerCommand( KEY_DELETE_COMMAND, () => { editor.update(() => { $getNodeByKey(nodeKey)?.remove(); }); return true; }, COMMAND_PRIORITY_LOW, ), ); }, [editor, isSelected, nodeKey]); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/nodes/PageBreakNode.tsx` around lines 26 - 51, The delete/backspace command registration logic in PageBreakNode (useEffect registering KEY_BACKSPACE_COMMAND and KEY_DELETE_COMMAND via editor.registerCommand and using $getNodeByKey(nodeKey)?.remove()) is duplicated with ImageNode; extract it into a shared hook (e.g., useDeleteOnSelection) that accepts nodeKey and isSelected, uses useLexicalComposerContext to get editor, and returns the mergeRegister of the two command registrations (COMMAND_PRIORITY_LOW) so both PageBreakNode and ImageNode call the hook instead of duplicating the useEffect logic.apps/web/src/components/TablePlugin.tsx (1)
39-41: Consider viewport boundary checks for menu positioning.The menu is positioned at
rect.top - 36, which could place it off-screen if the cell is near the top of the viewport. A simple clamp would improve UX:🔧 Proposed fix to clamp menu position
const rect = domElement.getBoundingClientRect(); - setMenuPos({ top: rect.top - 36, left: rect.right + 4 }); + const top = Math.max(8, rect.top - 36); + const left = Math.min(rect.right + 4, window.innerWidth - 200); + setMenuPos({ top, left }); setShowMenu(true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/TablePlugin.tsx` around lines 39 - 41, The menu position calculation in TablePlugin (where domElement.getBoundingClientRect() is used and setMenuPos({ top: rect.top - 36, left: rect.right + 4 }) is called before setShowMenu) can place the menu off-screen; update this logic to clamp the computed top and left to the viewport: compute tentativeTop = rect.top - 36 and tentativeLeft = rect.right + 4, then clamp top to be at least 0 and at most window.innerHeight - menuHeight, and clamp left to be at least 0 and at most window.innerWidth - menuWidth (use estimated menuWidth/menuHeight constants or measure the menu element if available) before calling setMenuPos and setShowMenu so the menu never renders outside the viewport.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 18-20: Replace the non-deterministic install command in the
workflow step that currently runs "npm install" with "npm ci" so CI performs a
clean, lockfile-driven install; update the step that contains the "run: npm
install" entry to "run: npm ci" and ensure the repository's package-lock.json is
committed to the repo so the action can succeed deterministically.
In `@apps/api/src/constants/upload.ts`:
- Line 3: UPLOADS_DIR currently resolves relative to process.cwd(), causing
uploads to land in the repo root; change it to resolve relative to this module
so uploads go to apps/api/uploads. Replace the declaration of UPLOADS_DIR
(symbol: UPLOADS_DIR) to use __dirname with path.resolve or path.join, e.g.
path.resolve(__dirname, '../../uploads') (adjusting the relative segments to
reach the apps/api/uploads directory from apps/api/src/constants/upload.ts), and
keep the existing path import.
In `@apps/api/tsconfig.json`:
- Around line 10-12: Remove the conflicting setting "noImplicitAny": false from
tsconfig.json so the existing "strict": true (enabled on line 9) can take
effect; locate the tsconfig.json entry with the "noImplicitAny" key and delete
that property (leave "types" and "typeRoots" intact) so implicit any checking is
enforced by the "strict" compiler option.
In `@apps/web/src/components/EditorToolbar.tsx`:
- Around line 218-235: The handleImageUpload function needs to check the HTTP
response status before parsing JSON: after awaiting
fetch(`${apiUrl}/api/uploads`, ...) verify res.ok (or inspect res.status) and
handle non-2xx responses by reading the error body and throwing or logging a
clear error instead of trying to destructure { url }; only when res.ok should
you parse JSON and call editor.update with
$insertNodeToNearestRoot($createImageNode(`${apiUrl}${url}`, file.name)). Ensure
any thrown error is caught by the existing catch block so upload failures (e.g.,
413/401) are handled gracefully.
In `@apps/web/src/index.css`:
- Around line 118-119: Remove the empty CSS rule for .editor-table-row to
eliminate lint noise: locate the .editor-table-row selector in
apps/web/src/index.css and delete the empty block (or add needed styles if it
was intended to be used) so the rule is no longer an empty block that triggers
the block-no-empty style lint rule.
In `@apps/web/tsconfig.json`:
- Line 15: Remove the explicit "noImplicitAny": false override in the tsconfig
(the line containing "noImplicitAny": false) so the compiler inherits the
project-wide "strict": true behavior; if you prefer to be explicit instead of
deleting the line, change the value to true to enforce implicit-any errors
across the codebase.
---
Nitpick comments:
In `@apps/api/src/index.ts`:
- Around line 52-58: The global error handler registered with app.use currently
returns err.message directly to clients (and logs only err.message), which can
leak internal details; change the handler in the app.use(...) block to sanitize
responses in production by returning a generic message (e.g., "Internal server
error") when process.env.NODE_ENV === "production" while still logging the full
error internally (use console.error(err) or the existing logger) and keep
setCors(res) behavior; ensure you reference the same middleware function
signature and only replace the response body content based on the environment.
In `@apps/api/src/routes/uploads.ts`:
- Around line 36-43: Wrap the current uploadsRouter.post handler to catch Multer
errors from upload.single('image') by invoking upload.single('image')(req, res,
(err) => { ... }) instead of using it directly as middleware; inside the
callback check for err and if err is an instance of multer.MulterError handle
err.code === 'LIMIT_FILE_SIZE' by sending status 413 with message "File too
large", handle other MulterError codes with 400 and a friendly message, and
handle non-Multer errors by calling next(err); if no error proceed with the
existing logic that checks req.file and returns the uploaded file URL. Ensure
you reference the existing upload variable and uploadsRouter.post route and
import/use multer.MulterError for the instanceof check.
In `@apps/web/src/components/Editor.tsx`:
- Around line 203-213: The inline fixed paper sizing and layout styles in the
Editor component (the outer div with style {{ width: 794, margin: '0 auto' }}
and the inner div with className="bg-white" and style {{ padding: 72, minHeight:
1123, boxShadow: '...'}}) should be extracted to CSS/Tailwind to follow project
conventions; replace those inline values with either a named CSS class (e.g.,
.paper, .paper-container) or Tailwind arbitrary utilities (e.g., w-[794px]
mx-auto p-[72px] min-h-[1123px]
shadow-[0_1px_3px_rgba(0,0,0,0.12),0_4px_16px_rgba(0,0,0,0.10)]) and move the
box-shadow and sizing into the stylesheet so the component uses className
references instead of inline style objects.
In `@apps/web/src/components/TablePlugin.tsx`:
- Around line 39-41: The menu position calculation in TablePlugin (where
domElement.getBoundingClientRect() is used and setMenuPos({ top: rect.top - 36,
left: rect.right + 4 }) is called before setShowMenu) can place the menu
off-screen; update this logic to clamp the computed top and left to the
viewport: compute tentativeTop = rect.top - 36 and tentativeLeft = rect.right +
4, then clamp top to be at least 0 and at most window.innerHeight - menuHeight,
and clamp left to be at least 0 and at most window.innerWidth - menuWidth (use
estimated menuWidth/menuHeight constants or measure the menu element if
available) before calling setMenuPos and setShowMenu so the menu never renders
outside the viewport.
In `@apps/web/src/context/DocumentContext.ts`:
- Around line 8-11: The DocumentContext currently uses empty-string defaults
which masks missing provider wiring; change the context to be nullable by
creating it as createContext<DocumentContextValue | null>(null) and add a guard
hook (e.g., useDocumentContext or useDocument) that calls
useContext(DocumentContext) and throws a clear error if the value is null, then
update consumers to import and use that guard hook instead of directly accessing
DocumentContext to ensure failures surface fast when the provider is not wired.
In `@apps/web/src/hooks/useAutoSave.ts`:
- Around line 6-14: The loadAutoSave function currently casts parsed JSON to
AutoSaveData without validation; update loadAutoSave to parse
localStorage.getItem(STORAGE_KEY) and validate the result with a Zod schema for
AutoSaveData (create or reuse a zod schema named something like
AutoSaveDataSchema), returning the parsed value only if AutoSaveDataSchema.parse
or safeParse succeeds, otherwise return null; keep the try/catch but replace the
direct "as AutoSaveData" cast with Zod validation using AutoSaveDataSchema and
reference STORAGE_KEY and AutoSaveData in the fix.
In `@apps/web/src/nodes/PageBreakNode.tsx`:
- Around line 26-51: The delete/backspace command registration logic in
PageBreakNode (useEffect registering KEY_BACKSPACE_COMMAND and
KEY_DELETE_COMMAND via editor.registerCommand and using
$getNodeByKey(nodeKey)?.remove()) is duplicated with ImageNode; extract it into
a shared hook (e.g., useDeleteOnSelection) that accepts nodeKey and isSelected,
uses useLexicalComposerContext to get editor, and returns the mergeRegister of
the two command registrations (COMMAND_PRIORITY_LOW) so both PageBreakNode and
ImageNode call the hook instead of duplicating the useEffect logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0e547a88-f6fb-4534-898d-e90a4f731a9a
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (36)
.coderabbit.yaml.github/workflows/ci.yml.gitignore.husky/pre-commitCLAUDE.mdCONTRIBUTING.mdREADME.mdapps/api/.env.exampleapps/api/package.jsonapps/api/src/constants/upload.tsapps/api/src/index.tsapps/api/src/middleware/auth.tsapps/api/src/routes/uploads.tsapps/api/src/types/auth.tsapps/api/tsconfig.jsonapps/web/eslint.config.jsapps/web/package.jsonapps/web/src/components/DocumentPDF.tsxapps/web/src/components/Editor.tsxapps/web/src/components/EditorToolbar.tsxapps/web/src/components/SlashCommandMenu.tsxapps/web/src/components/TablePlugin.tsxapps/web/src/constants/autosave.tsapps/web/src/constants/editor.tsapps/web/src/context/DocumentContext.tsapps/web/src/hooks/useAutoSave.tsapps/web/src/index.cssapps/web/src/nodes/ImageNode.tsxapps/web/src/nodes/PageBreakNode.tsxapps/web/src/types/editor.tsapps/web/src/vite-env.d.tsapps/web/src/workers/pdfExport.worker.tsapps/web/tsconfig.jsonapps/web/vite.config.tseslint.config.mjspackage.json
💤 Files with no reviewable changes (1)
- apps/web/src/workers/pdfExport.worker.ts
Comment on lines
+18
to
+20
| # npm ci fails cross-platform when lock file was generated on a different | ||
| # OS (macOS lock file missing Linux rollup binary). Use npm install instead. | ||
| - run: npm install |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Quick lockfile sanity checks for platform-specific rollup optional packages
rg -n '"@rollup/rollup-(linux|darwin|win32)' package-lock.json
rg -n '"optionalDependencies"|\"os\"|\"cpu\"' package-lock.jsonRepository: hawk-doc/hawkdoc
Length of output: 1483
🏁 Script executed:
# Check package.json for how rollup dependencies are declared
cat -n package.json | head -50Repository: hawk-doc/hawkdoc
Length of output: 1026
🏁 Script executed:
# Search for rollup in package.json to understand dependency structure
rg -A 5 -B 5 '"rollup"' package.jsonRepository: hawk-doc/hawkdoc
Length of output: 42
🏁 Script executed:
# Check if there are optional dependencies or optionalDependencies sections in package.json
rg -n 'optionalDependencies' package.jsonRepository: hawk-doc/hawkdoc
Length of output: 42
🏁 Script executed:
# Look at how the platform-specific rollup packages are marked in package-lock.json
# Search for the rollup-linux entry to see its structure (optional: true/false)
rg -A 10 '"@rollup/rollup-linux-x64-gnu"' package-lock.json | head -30Repository: hawk-doc/hawkdoc
Length of output: 945
🏁 Script executed:
# Check workspace package.json files for rollup dependencies
find apps packages -name package.json -type f | head -10Repository: hawk-doc/hawkdoc
Length of output: 132
🏁 Script executed:
# Check if rollup is in apps/web/package.json
cat apps/web/package.json | grep -A 10 -B 2 'rollup\|vite'Repository: hawk-doc/hawkdoc
Length of output: 1207
🏁 Script executed:
# Check the full structure of a platform-specific rollup package in lockfile to see if it's marked optional
rg -B 5 '"@rollup/rollup-linux-x64-gnu": "4.59.0"' package-lock.json | head -20Repository: hawk-doc/hawkdoc
Length of output: 375
🏁 Script executed:
# Check git log for this specific file to see when npm install was introduced and why
git log --oneline .github/workflows/ci.yml | head -10Repository: hawk-doc/hawkdoc
Length of output: 122
🏁 Script executed:
# Look for recent commits that might explain the npm install change
git log -p --all -S 'npm install' -- .github/workflows/ci.yml | head -100Repository: hawk-doc/hawkdoc
Length of output: 42
🏁 Script executed:
# Check the actual .github/workflows/ci.yml to see full context
cat .github/workflows/ci.ymlRepository: hawk-doc/hawkdoc
Length of output: 774
🏁 Script executed:
# Check if there are any issues or PRs mentioning npm ci cross-platform problems
git log --all --grep="npm ci" --oneline | head -20Repository: hawk-doc/hawkdoc
Length of output: 42
🏁 Script executed:
# Check package.json version constraint for npm (if specified)
rg -n 'engines' package.jsonRepository: hawk-doc/hawkdoc
Length of output: 42
🏁 Script executed:
# Search for any npm-related configuration that might affect optional deps
cat .npmrc 2>/dev/null || echo "No .npmrc found"Repository: hawk-doc/hawkdoc
Length of output: 75
🏁 Script executed:
# Check if vite (the build tool) is what requires these optional rollup deps
rg -A 5 'vite' apps/web/package.json | grep -A 5 devDependenciesRepository: hawk-doc/hawkdoc
Length of output: 42
Switch back to npm ci for deterministic CI installs.
The lockfile already contains all platform-specific @rollup binaries. Since the prerequisite—a complete lockfile with all optional dependencies for every platform—is now met, npm ci will work correctly and provide deterministic installations in CI. Using npm install risks mutating the lockfile across runs and masking dependency issues.
🔧 Proposed fix
- # npm ci fails cross-platform when lock file was generated on a different
- # OS (macOS lock file missing Linux rollup binary). Use npm install instead.
- - run: npm install
+ - run: npm ci📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # npm ci fails cross-platform when lock file was generated on a different | |
| # OS (macOS lock file missing Linux rollup binary). Use npm install instead. | |
| - run: npm install | |
| - run: npm ci |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yml around lines 18 - 20, Replace the non-deterministic
install command in the workflow step that currently runs "npm install" with "npm
ci" so CI performs a clean, lockfile-driven install; update the step that
contains the "run: npm install" entry to "run: npm ci" and ensure the
repository's package-lock.json is committed to the repo so the action can
succeed deterministically.
Comment on lines
+10
to
+12
| "noImplicitAny": false, | ||
| "types": ["node"], | ||
| "typeRoots": ["../../node_modules/@types"], |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect API compiler options that affect typing and resolution
rg -n '"noImplicitAny"|\"typeRoots\"|\"types\"' apps/api/tsconfig.jsonRepository: hawk-doc/hawkdoc
Length of output: 181
🏁 Script executed:
cat -n apps/api/tsconfig.jsonRepository: hawk-doc/hawkdoc
Length of output: 759
Remove noImplicitAny: false which contradicts the enabled strict mode on line 9.
Line 10 explicitly disables implicit any checking, which overrides the strict mode already enabled on line 9 and violates coding guidelines. Simply removing this line lets strict mode enforce type safety as intended.
🔧 Proposed fix
"strict": true,
- "noImplicitAny": false,
"types": ["node"],📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "noImplicitAny": false, | |
| "types": ["node"], | |
| "typeRoots": ["../../node_modules/@types"], | |
| "types": ["node"], | |
| "typeRoots": ["../../node_modules/@types"], |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/tsconfig.json` around lines 10 - 12, Remove the conflicting setting
"noImplicitAny": false from tsconfig.json so the existing "strict": true
(enabled on line 9) can take effect; locate the tsconfig.json entry with the
"noImplicitAny" key and delete that property (leave "types" and "typeRoots"
intact) so implicit any checking is enforced by the "strict" compiler option.
| "noEmit": true, | ||
| "jsx": "react-jsx", | ||
| "strict": true, | ||
| "noImplicitAny": false, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify strict/implicit-any settings in tsconfig files
rg -n '"strict"\s*:\s*true|"noImplicitAny"\s*:\s*false|"noImplicitAny"\s*:\s*true' apps/web/tsconfig.json apps/api/tsconfig.jsonRepository: hawk-doc/hawkdoc
Length of output: 258
Remove the noImplicitAny: false override to maintain strict type checking.
Setting "noImplicitAny": false contradicts "strict": true and allows implicit any types, weakening type safety across the application.
Proposed fix
- "noImplicitAny": false,
+ "noImplicitAny": true,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "noImplicitAny": false, | |
| "noImplicitAny": true, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/tsconfig.json` at line 15, Remove the explicit "noImplicitAny":
false override in the tsconfig (the line containing "noImplicitAny": false) so
the compiler inherits the project-wide "strict": true behavior; if you prefer to
be explicit instead of deleting the line, change the value to true to enforce
implicit-any errors across the codebase.
…nd add delete on selection hook
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (8)
apps/web/src/index.css (2)
157-169: Prefer theme tokens over hardcoded hex for toolbar colors.The rest of this file consistently uses
notion-*Tailwind tokens (text-notion-text,bg-notion-hover,border-notion-border, etc.), but the toolbar changes switch to raw hex values (#444746,#f1f3f4,#d3e3fd,#1a73e8,#dadce0). This fragments the design tokens, makes future theming (e.g., dark mode) harder, and creates two sources of truth for "hover background" and "border" shades.If these specific Google-Docs-like shades are intentional, consider adding them to the Tailwind theme (e.g.,
toolbar-text,toolbar-hover,toolbar-active-bg,toolbar-active-text,toolbar-divider) and referencing them via@apply, so.toolbar-btn,.toolbar-btn.active, and.toolbar-dividerstay consistent with the rest of the editor styling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/index.css` around lines 157 - 169, The toolbar CSS uses hardcoded hex colors in the .toolbar-btn, .toolbar-btn.active, and .toolbar-divider rules instead of the project's Tailwind tokens; replace those hex values by referencing existing notion-* tokens (e.g., text-notion-text, bg-notion-hover, border-notion-border) or add new theme tokens (e.g., toolbar-text, toolbar-hover, toolbar-active-bg, toolbar-active-text, toolbar-divider) in the Tailwind config and then update .toolbar-btn, .toolbar-btn.active, and .toolbar-divider to `@apply` those tokens so the toolbar matches the app's design system and supports theming.
113-130: Small cleanup: duplicated cell base styles and an extra blank line.
.editor-table-celland.editor-table-cell-headerrepeat the same base declarations (border border-notion-border px-3 py-2 text-sm align-top min-w-[80px]). Applying the shared rules via a grouped selector keeps them in sync if the base changes later.- Line 118 is a double blank line between
.editor-tableand.editor-table-cell; trimming it matches the rest of the file.Also worth noting:
EDITOR_THEME.tableRowinapps/web/src/constants/editor.tsmaps toeditor-table-row, which has no CSS rule here. That's fine if rows genuinely need no styling (and avoids the prior empty-block lint finding), but confirm it's intentional rather than accidentally dropped.♻️ Proposed refactor
.editor-table { `@apply` w-full border-collapse my-4; } - - .editor-table-cell { - `@apply` border border-notion-border px-3 py-2 text-sm align-top min-w-[80px]; - } - - .editor-table-cell-header { - `@apply` border border-notion-border px-3 py-2 text-sm align-top min-w-[80px] - bg-notion-hover font-semibold; + .editor-table-cell, + .editor-table-cell-header { + `@apply` border border-notion-border px-3 py-2 text-sm align-top min-w-[80px]; + } + + .editor-table-cell-header { + `@apply` bg-notion-hover font-semibold; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/index.css` around lines 113 - 130, Group the duplicated base rules into a shared selector and remove the extra blank line: replace the repeated declarations in .editor-table-cell and .editor-table-cell-header with a combined selector (.editor-table-cell, .editor-table-cell-header) that contains the shared rules (border border-notion-border px-3 py-2 text-sm align-top min-w-[80px]), then keep .editor-table-cell-header only for its unique rules (bg-notion-hover font-semibold); also delete the double blank line between .editor-table and .editor-table-cell to match file spacing, and confirm whether EDITOR_THEME.tableRow -> "editor-table-row" intentionally has no CSS rule or needs styling added.apps/web/src/hooks/useDeleteOnSelection.ts (3)
19-34: Re-registering commands on everyisSelectedtoggle is avoidable.Because
isSelectedis in the dependency array, both commands are unregistered and re-registered every time the decorator node gains/loses selection. CapturingisSelectedin a ref lets you register the commands once pereditor/nodeKeychange and avoids the churn — useful given the project's "zero lag" design principle for editor interactions.♻️ Proposed refactor using a ref
-import { useEffect } from 'react'; +import { useEffect, useRef } from 'react'; ... export function useDeleteOnSelection(nodeKey: string, isSelected: boolean): void { const [editor] = useLexicalComposerContext(); + const isSelectedRef = useRef(isSelected); + isSelectedRef.current = isSelected; useEffect(() => { + const handleRemove = () => { + if (!isSelectedRef.current) return false; + editor.update(() => { + $getNodeByKey(nodeKey)?.remove(); + }); + return true; + }; return mergeRegister( - editor.registerCommand(KEY_BACKSPACE_COMMAND, () => { ... }, COMMAND_PRIORITY_LOW), - editor.registerCommand(KEY_DELETE_COMMAND, () => { ... }, COMMAND_PRIORITY_LOW), + editor.registerCommand(KEY_BACKSPACE_COMMAND, handleRemove, COMMAND_PRIORITY_LOW), + editor.registerCommand(KEY_DELETE_COMMAND, handleRemove, COMMAND_PRIORITY_LOW), ); - }, [editor, isSelected, nodeKey]); + }, [editor, nodeKey]); }Based on learnings: "The core design principle is zero lag — on typing, scrolling, cursor movement, and export. Optimize all editor interactions for performance."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/hooks/useDeleteOnSelection.ts` around lines 19 - 34, The commands are being unregistered/re-registered whenever isSelected changes; instead, create a mutable ref (e.g., isSelectedRef) updated inside the selection effect and register the handlers for KEY_BACKSPACE_COMMAND and KEY_DELETE_COMMAND once per editor/nodeKey (use editor and nodeKey in the registration dependency) so the handlers read isSelectedRef.current when invoked; update the ref whenever isSelected toggles so editor.update(() => $getNodeByKey(nodeKey)?.remove()) only runs when the ref is true, avoiding re-registration churn in useDeleteOnSelection.ts (refer to registerCommand, KEY_BACKSPACE_COMMAND, KEY_DELETE_COMMAND, COMMAND_PRIORITY_LOW, and nodeKey).
14-39: Consider DRYing the two identical command handlers.Both
KEY_BACKSPACE_COMMANDandKEY_DELETE_COMMANDrun the same logic. Extracting a single handler keeps behavior in lock-step and makes future changes (e.g., clearing selection after removal) a one-line edit.♻️ Proposed refactor
useEffect(() => { + const handleRemove = () => { + if (!isSelected) return false; + editor.update(() => { + $getNodeByKey(nodeKey)?.remove(); + }); + return true; + }; + return mergeRegister( - editor.registerCommand( - KEY_BACKSPACE_COMMAND, - () => { - if (isSelected) { - editor.update(() => { $getNodeByKey(nodeKey)?.remove(); }); - return true; - } - return false; - }, - COMMAND_PRIORITY_LOW, - ), - editor.registerCommand( - KEY_DELETE_COMMAND, - () => { - if (isSelected) { - editor.update(() => { $getNodeByKey(nodeKey)?.remove(); }); - return true; - } - return false; - }, - COMMAND_PRIORITY_LOW, - ), + editor.registerCommand(KEY_BACKSPACE_COMMAND, handleRemove, COMMAND_PRIORITY_LOW), + editor.registerCommand(KEY_DELETE_COMMAND, handleRemove, COMMAND_PRIORITY_LOW), ); }, [editor, isSelected, nodeKey]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/hooks/useDeleteOnSelection.ts` around lines 14 - 39, Both KEY_BACKSPACE_COMMAND and KEY_DELETE_COMMAND register identical handlers in useDeleteOnSelection.ts; extract the shared logic into a single function (e.g., handleDeleteOnSelection) and pass that function to editor.registerCommand for both KEY_BACKSPACE_COMMAND and KEY_DELETE_COMMAND so behavior is centralized; update the existing editor.registerCommand calls to reference handleDeleteOnSelection (keep COMMAND_PRIORITY_LOW) and ensure the new function uses editor.update and $getNodeByKey(nodeKey)?.remove() and returns true/false as before.
20-20: Stale node-selection state after keyboard deletion.After
$getNodeByKey(nodeKey)?.remove()in the DELETE/BACKSPACE command handlers, the nodeKey entry can linger in the LexicalNodeSelectionset. WhileclearSelection()is already available and used in click handlers, it's not called when deletion occurs via keyboard commands, potentially leaving stale selection state. To fix this defensively, passclearSelectionas a parameter touseDeleteOnSelectionand call it after the node is removed, or callclearSelection()at the call sites afteruseDeleteOnSelectiontriggers deletion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/hooks/useDeleteOnSelection.ts` at line 20, The deletion handler using editor.update currently calls $getNodeByKey(nodeKey)?.remove() but can leave a stale NodeSelection entry; update useDeleteOnSelection to accept a clearSelection callback (or call clearSelection at the delete callsites) and invoke clearSelection() immediately after the node is removed so the Lexical NodeSelection set is cleared; reference the editor.update block, $getNodeByKey(nodeKey), nodeKey and clearSelection when making the change.apps/api/src/db.ts (1)
8-13: Prefer a named export over default forquery.Switching
queryfrom a named to a default export is a stylistic regression: default exports lose rename-safety on import, break tree-shaking ergonomics, and makegrep/refactor tooling less reliable. The previous named export was fine and all four call sites (index.ts,hocuspocus.ts,routes/auth.ts,routes/documents.ts) have no technical reason to require default import.If the motivation was just a shorter import syntax, consider keeping the named export and optionally re-exporting
poolfrom the same module. This also keeps the public API ofdb.tsconsistent (poolis already a named export).♻️ Suggested revert to named export
-export default async function query<T extends pg.QueryResultRow>( +export async function query<T extends pg.QueryResultRow>( text: string, params?: unknown[], ): Promise<pg.QueryResult<T>> { return pool.query<T>(text, params); }And update the four import sites back to
import { query } from '...'.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/db.ts` around lines 8 - 13, Replace the default export of the database query function with a named export: change the exported symbol from default to a named export "query" (retain the generic signature query<T extends pg.QueryResultRow>(text: string, params?: unknown[]): Promise<pg.QueryResult<T>>), keep the existing named export "pool" as-is, and update all import sites that currently do default-imports to use named imports (import { query } ...) in the four call sites referencing this module (index.ts, hocuspocus.ts, routes/auth.ts, routes/documents.ts) so consumers use the named export consistently and retain tree-shaking/rename-safety.apps/api/src/constants/upload.ts (1)
1-7: LGTM —UPLOADS_DIRcorrectly anchored to the module path.
__dirnamederived viafileURLToPath(import.meta.url)andpath.resolve(__dirname, '../../uploads')correctly resolves toapps/api/uploadsregardless of the process cwd, addressing the earlier concern.One small follow-up to consider (non-blocking):
MAX_UPLOAD_SIZE_BYTESis hardcoded to 10 MB butindex.tsusesenv.MAX_FILE_SIZE_MBforexpress.jsonbody limits. If the intent is a single source of truth for upload size, deriving this constant fromenv.MAX_FILE_SIZE_MB * 1024 * 1024would avoid drift between JSON body limits and multer limits.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/constants/upload.ts` around lines 1 - 7, Replace the hardcoded MAX_UPLOAD_SIZE_BYTES with a value derived from the existing env.MAX_FILE_SIZE_MB so both multer and express limits share a single source of truth: import the env symbol used by index.ts and compute MAX_UPLOAD_SIZE_BYTES as (Number(env.MAX_FILE_SIZE_MB) with a sensible fallback of 10) multiplied by 1024 * 1024; keep the __dirname and UPLOADS_DIR logic unchanged and ensure the fallback is numeric to avoid NaN.apps/web/src/components/EditorToolbar.tsx (1)
230-233: Storing the API host in the image URL couples documents to a specific deployment.
${apiUrl}${url}bakes the absolute origin (e.g.,http://localhost:3001/uploads/...) into the serialized editor state viaImageNode. IfVITE_API_URLchanges between environments (dev/staging/prod) or across deployments, previously saved documents will render broken images. Consider storing the relative path (/uploads/...) inImageNodeand prepending the API base at render time inImageNode.tsx/ PDF export, or exposing a static‑assets CDN host that's stable across environments.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/EditorToolbar.tsx` around lines 230 - 233, The uploaded image URL is being saved with the API origin (`${apiUrl}${url}`) which couples saved documents to a deployment; change the call in the upload flow so $createImageNode is given the relative path (just `url`) instead of `${apiUrl}${url}` (update the code around editor.update / $insertNodeToNearestRoot / $createImageNode). Then modify ImageNode rendering and any PDF export logic in ImageNode.tsx (or its export helpers) to prepend the runtime API base or CDN host (e.g., from VITE_API_URL or a stable asset host) when displaying or exporting images so stored document state remains deployment‑agnostic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/index.ts`:
- Around line 54-59: The error handler currently reads process.env.NODE_ENV
directly; update the env validation to include NODE_ENV (e.g., add
z.enum(['development','test','production']).default('development') to the
exported Zod schema in env.ts) and then replace process.env.NODE_ENV with the
validated env value (env.NODE_ENV) inside the Express error middleware (the
app.use(...) error handler) so the handler uses the Zod-validated environment
variable consistently; ensure the env module is imported where the middleware is
defined and that the application crashes at startup if NODE_ENV is
missing/invalid per the schema.
- Around line 32-33: The /uploads route is currently served publicly via
app.use('/uploads', express.static(UPLOADS_DIR)), exposing capability URLs;
update this to either (a) protect the route with the existing JWT middleware
used elsewhere (wrap or replace the express.static mount with the same
middleware, e.g., app.use('/uploads', authenticateJWT,
express.static(UPLOADS_DIR))) or (b) switch to presigned URLs when you migrate
to multer-s3; additionally ensure uploaded filenames are generated using a
cryptographically secure method (crypto.randomUUID or similar) and document the
capability-URL model in the repo README.
- Around line 14-27: Replace the permissive wildcard CORS logic in
setCors/app.use by reading an allowed origin list from your validated env
(env.ts via Zod), check req.headers.origin and if it matches the allowlist echo
that origin in Access-Control-Allow-Origin (do not use "*"), set the Vary:
Origin header, and keep the other CORS headers; for OPTIONS requests still
return 204. Update any error handler invocation mentioned in the review to
accept and forward req as an argument where required. Use the existing setCors,
app.use middleware, and env.ts symbols to locate and implement these changes.
- Line 5: The import of UPLOADS_DIR should come directly from its source to
avoid implicit coupling: replace the combined import that pulls UPLOADS_DIR from
'./routes/uploads.js' with two imports — keep uploadsRouter from
'./routes/uploads.js' and import UPLOADS_DIR from the constants module (the
module that defines it, e.g., './constants/upload.js'); update the import
statement(s) in index.ts to reference uploadsRouter and UPLOADS_DIR from their
respective modules so app.use('/uploads', express.static(UPLOADS_DIR)) uses the
constant's canonical export.
In `@apps/web/src/components/EditorToolbar.tsx`:
- Around line 303-334: The dropdown currently uses inline style for dynamic
fonts (style={{ fontFamily }} and style={{ fontFamily: css }}) which violates
the "no inline styles" rule; update the implementation in EditorToolbar.tsx to
remove inline styles by either (A) adding a small safelist of Tailwind font-*
classes in FONT_FAMILIES and render className based on a lookup (e.g.,
className={`font-${fontKey} ...`} using the font identifier), or (B) set a CSS
custom property on the parent (e.g., style-wise via Tailwind arbitrary property
like className={`[--font:${css}]`} on the button container) and use a Tailwind
utility that maps to that variable (e.g., font-[var(--font)]), then replace the
style usages in the trigger button (where fontFamily is referenced) and in the
option buttons inside the FONT_FAMILIES.map to use the chosen approach; adjust
applyFontFamily usage and any state (fontFamily, fontFamilyOpen, fontFamilyRef,
FONT_FAMILIES) accordingly so visuals remain identical without inline styles.
- Around line 218-239: In handleImageUpload, validate the parsed response before
using it: after parsing JSON from the upload response, ensure the object has a
non-empty string url (e.g., check typeof resJson.url === 'string' &&
resJson.url.trim() !== '') and throw a descriptive error if it doesn't; this
prevents inserting a broken ImageNode via $createImageNode. Also replace the
silent console.error in the catch with user-visible feedback by invoking the
app's toast/dialog utility (or set an error state that triggers an InputDialog)
with the error message/status so 413/401/network errors are surfaced to the
user; keep editor.update($insertNodeToNearestRoot($createImageNode(...))) only
when validation succeeds.
In `@apps/web/src/hooks/useAutoSave.ts`:
- Around line 44-50: The deferred setTimeout that calls setIsSaving(false) is
not stored and therefore not cleared on unmount; create a ref (e.g.,
savingFlipRef) to hold the inner timeout id, assign the result of the inner
setTimeout to savingFlipRef.current, clear any existing savingFlipRef.current
before scheduling a new one, and ensure the effect cleanup also clears
savingFlipRef.current (alongside timerRef.current) so the pending
setIsSaving(false) never runs after teardown; update usages around setIsSaving,
timerRef, and the debounce logic accordingly.
---
Nitpick comments:
In `@apps/api/src/constants/upload.ts`:
- Around line 1-7: Replace the hardcoded MAX_UPLOAD_SIZE_BYTES with a value
derived from the existing env.MAX_FILE_SIZE_MB so both multer and express limits
share a single source of truth: import the env symbol used by index.ts and
compute MAX_UPLOAD_SIZE_BYTES as (Number(env.MAX_FILE_SIZE_MB) with a sensible
fallback of 10) multiplied by 1024 * 1024; keep the __dirname and UPLOADS_DIR
logic unchanged and ensure the fallback is numeric to avoid NaN.
In `@apps/api/src/db.ts`:
- Around line 8-13: Replace the default export of the database query function
with a named export: change the exported symbol from default to a named export
"query" (retain the generic signature query<T extends pg.QueryResultRow>(text:
string, params?: unknown[]): Promise<pg.QueryResult<T>>), keep the existing
named export "pool" as-is, and update all import sites that currently do
default-imports to use named imports (import { query } ...) in the four call
sites referencing this module (index.ts, hocuspocus.ts, routes/auth.ts,
routes/documents.ts) so consumers use the named export consistently and retain
tree-shaking/rename-safety.
In `@apps/web/src/components/EditorToolbar.tsx`:
- Around line 230-233: The uploaded image URL is being saved with the API origin
(`${apiUrl}${url}`) which couples saved documents to a deployment; change the
call in the upload flow so $createImageNode is given the relative path (just
`url`) instead of `${apiUrl}${url}` (update the code around editor.update /
$insertNodeToNearestRoot / $createImageNode). Then modify ImageNode rendering
and any PDF export logic in ImageNode.tsx (or its export helpers) to prepend the
runtime API base or CDN host (e.g., from VITE_API_URL or a stable asset host)
when displaying or exporting images so stored document state remains
deployment‑agnostic.
In `@apps/web/src/hooks/useDeleteOnSelection.ts`:
- Around line 19-34: The commands are being unregistered/re-registered whenever
isSelected changes; instead, create a mutable ref (e.g., isSelectedRef) updated
inside the selection effect and register the handlers for KEY_BACKSPACE_COMMAND
and KEY_DELETE_COMMAND once per editor/nodeKey (use editor and nodeKey in the
registration dependency) so the handlers read isSelectedRef.current when
invoked; update the ref whenever isSelected toggles so editor.update(() =>
$getNodeByKey(nodeKey)?.remove()) only runs when the ref is true, avoiding
re-registration churn in useDeleteOnSelection.ts (refer to registerCommand,
KEY_BACKSPACE_COMMAND, KEY_DELETE_COMMAND, COMMAND_PRIORITY_LOW, and nodeKey).
- Around line 14-39: Both KEY_BACKSPACE_COMMAND and KEY_DELETE_COMMAND register
identical handlers in useDeleteOnSelection.ts; extract the shared logic into a
single function (e.g., handleDeleteOnSelection) and pass that function to
editor.registerCommand for both KEY_BACKSPACE_COMMAND and KEY_DELETE_COMMAND so
behavior is centralized; update the existing editor.registerCommand calls to
reference handleDeleteOnSelection (keep COMMAND_PRIORITY_LOW) and ensure the new
function uses editor.update and $getNodeByKey(nodeKey)?.remove() and returns
true/false as before.
- Line 20: The deletion handler using editor.update currently calls
$getNodeByKey(nodeKey)?.remove() but can leave a stale NodeSelection entry;
update useDeleteOnSelection to accept a clearSelection callback (or call
clearSelection at the delete callsites) and invoke clearSelection() immediately
after the node is removed so the Lexical NodeSelection set is cleared; reference
the editor.update block, $getNodeByKey(nodeKey), nodeKey and clearSelection when
making the change.
In `@apps/web/src/index.css`:
- Around line 157-169: The toolbar CSS uses hardcoded hex colors in the
.toolbar-btn, .toolbar-btn.active, and .toolbar-divider rules instead of the
project's Tailwind tokens; replace those hex values by referencing existing
notion-* tokens (e.g., text-notion-text, bg-notion-hover, border-notion-border)
or add new theme tokens (e.g., toolbar-text, toolbar-hover, toolbar-active-bg,
toolbar-active-text, toolbar-divider) in the Tailwind config and then update
.toolbar-btn, .toolbar-btn.active, and .toolbar-divider to `@apply` those tokens
so the toolbar matches the app's design system and supports theming.
- Around line 113-130: Group the duplicated base rules into a shared selector
and remove the extra blank line: replace the repeated declarations in
.editor-table-cell and .editor-table-cell-header with a combined selector
(.editor-table-cell, .editor-table-cell-header) that contains the shared rules
(border border-notion-border px-3 py-2 text-sm align-top min-w-[80px]), then
keep .editor-table-cell-header only for its unique rules (bg-notion-hover
font-semibold); also delete the double blank line between .editor-table and
.editor-table-cell to match file spacing, and confirm whether
EDITOR_THEME.tableRow -> "editor-table-row" intentionally has no CSS rule or
needs styling added.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2da5bb03-22fc-48e3-b0a3-d879ea56b279
📒 Files selected for processing (19)
.github/ISSUE_TEMPLATE/good_first_issue.mdapps/api/package.jsonapps/api/src/constants/upload.tsapps/api/src/db.tsapps/api/src/hocuspocus.tsapps/api/src/index.tsapps/api/src/routes/auth.tsapps/api/src/routes/documents.tsapps/api/src/routes/uploads.tsapps/web/src/components/Editor.tsxapps/web/src/components/EditorToolbar.tsxapps/web/src/components/TablePlugin.tsxapps/web/src/context/DocumentContext.tsapps/web/src/hooks/useAutoSave.tsapps/web/src/hooks/useDeleteOnSelection.tsapps/web/src/index.cssapps/web/src/nodes/ImageNode.tsxapps/web/src/nodes/PageBreakNode.tsxapps/web/tsconfig.node.json
✅ Files skipped from review due to trivial changes (5)
- apps/api/src/hocuspocus.ts
- apps/api/src/routes/documents.ts
- apps/api/src/routes/auth.ts
- apps/web/tsconfig.node.json
- .github/ISSUE_TEMPLATE/good_first_issue.md
🚧 Files skipped from review as they are similar to previous changes (7)
- apps/web/src/components/TablePlugin.tsx
- apps/web/src/nodes/ImageNode.tsx
- apps/web/src/context/DocumentContext.ts
- apps/api/src/routes/uploads.ts
- apps/api/package.json
- apps/web/src/components/Editor.tsx
- apps/web/src/nodes/PageBreakNode.tsx
| import { env } from './env.js'; | ||
| import { authRouter } from './routes/auth.js'; | ||
| import { documentsRouter } from './routes/documents.js'; | ||
| import { uploadsRouter, UPLOADS_DIR } from './routes/uploads.js'; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm where UPLOADS_DIR is defined/re-exported
rg -nP '\bUPLOADS_DIR\b' apps/api/srcRepository: hawk-doc/hawkdoc
Length of output: 711
Import UPLOADS_DIR directly from the constants module.
UPLOADS_DIR is defined in apps/api/src/constants/upload.ts and re-exported from ./routes/uploads.ts. Importing it through the routes module creates implicit coupling — if the re-export is removed during refactoring, the static serving in app.use('/uploads', express.static(UPLOADS_DIR)) will break silently. Import directly from the source module.
♻️ Suggested fix
-import { uploadsRouter, UPLOADS_DIR } from './routes/uploads.js';
+import { uploadsRouter } from './routes/uploads.js';
+import { UPLOADS_DIR } from './constants/upload.js';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| import { uploadsRouter, UPLOADS_DIR } from './routes/uploads.js'; | |
| import { uploadsRouter } from './routes/uploads.js'; | |
| import { UPLOADS_DIR } from './constants/upload.js'; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/index.ts` at line 5, The import of UPLOADS_DIR should come
directly from its source to avoid implicit coupling: replace the combined import
that pulls UPLOADS_DIR from './routes/uploads.js' with two imports — keep
uploadsRouter from './routes/uploads.js' and import UPLOADS_DIR from the
constants module (the module that defines it, e.g., './constants/upload.js');
update the import statement(s) in index.ts to reference uploadsRouter and
UPLOADS_DIR from their respective modules so app.use('/uploads',
express.static(UPLOADS_DIR)) uses the constant's canonical export.
Comment on lines
+14
to
27
| function setCors(res: Response): void { | ||
| res.header('Access-Control-Allow-Origin', '*'); | ||
| res.header('Access-Control-Allow-Methods', 'GET,POST,PATCH,DELETE,OPTIONS'); | ||
| res.header('Access-Control-Allow-Headers', 'Content-Type,Authorization'); | ||
| } | ||
|
|
||
| app.use((req, res, next) => { | ||
| setCors(res); | ||
| if (req.method === 'OPTIONS') { | ||
| res.sendStatus(204); | ||
| return; | ||
| } | ||
| next(); | ||
| }); |
There was a problem hiding this comment.
Wildcard CORS origin is too permissive for an authenticated API.
Access-Control-Allow-Origin: * allows any site to issue cross-origin requests to this API. Combined with Access-Control-Allow-Headers: Authorization, this means any webpage a user visits can attempt to hit your endpoints with a stolen/bearer token if one is leaked into JS-accessible storage. Even without credentials, it exposes all read endpoints (documents, uploads listing, etc.) cross-origin.
Drive the allowlist from an env var (validated via Zod in env.ts) and only echo the request Origin back when it matches. Also Vary: Origin should be set so caches don't serve the wrong header.
🛡️ Suggested change
-function setCors(res: Response): void {
- res.header('Access-Control-Allow-Origin', '*');
- res.header('Access-Control-Allow-Methods', 'GET,POST,PATCH,DELETE,OPTIONS');
- res.header('Access-Control-Allow-Headers', 'Content-Type,Authorization');
-}
-
-app.use((req, res, next) => {
- setCors(res);
+const ALLOWED_ORIGINS = new Set(env.ALLOWED_ORIGINS); // e.g. ['http://localhost:5173', 'https://app.example.com']
+
+function setCors(req: Request, res: Response): void {
+ const origin = req.headers.origin;
+ if (origin && ALLOWED_ORIGINS.has(origin)) {
+ res.header('Access-Control-Allow-Origin', origin);
+ res.header('Vary', 'Origin');
+ }
+ res.header('Access-Control-Allow-Methods', 'GET,POST,PATCH,DELETE,OPTIONS');
+ res.header('Access-Control-Allow-Headers', 'Content-Type,Authorization');
+}
+
+app.use((req, res, next) => {
+ setCors(req, res);
if (req.method === 'OPTIONS') {
res.sendStatus(204);
return;
}
next();
});Remember to update the error handler call site to pass req too.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/index.ts` around lines 14 - 27, Replace the permissive wildcard
CORS logic in setCors/app.use by reading an allowed origin list from your
validated env (env.ts via Zod), check req.headers.origin and if it matches the
allowlist echo that origin in Access-Control-Allow-Origin (do not use "*"), set
the Vary: Origin header, and keep the other CORS headers; for OPTIONS requests
still return 204. Update any error handler invocation mentioned in the review to
accept and forward req as an argument where required. Use the existing setCors,
app.use middleware, and env.ts symbols to locate and implement these changes.
Comment on lines
+32
to
+33
| // Static file serving for uploaded images | ||
| app.use('/uploads', express.static(UPLOADS_DIR)); |
There was a problem hiding this comment.
/uploads is served publicly with no auth.
express.static(UPLOADS_DIR) exposes every uploaded asset under a guessable URL with no access control. For an editor handling potentially private user documents, anyone who obtains or guesses a filename can retrieve the image. If filenames are generated via crypto.randomUUID() (as hinted by the imports) this is effectively a capability URL — acceptable for MVP but worth making explicit.
For production you should either (a) gate /uploads behind the same JWT middleware used elsewhere, or (b) rely on presigned URLs once the multer-s3 migration mentioned in the coding guidelines lands. At minimum, document the capability-URL model and ensure filenames are cryptographically random, not user-controlled.
As per coding guidelines: "For MVP, use multer with local disk storage. For production, replace with multer-s3 ... while keeping frontend URL the same."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/index.ts` around lines 32 - 33, The /uploads route is currently
served publicly via app.use('/uploads', express.static(UPLOADS_DIR)), exposing
capability URLs; update this to either (a) protect the route with the existing
JWT middleware used elsewhere (wrap or replace the express.static mount with the
same middleware, e.g., app.use('/uploads', authenticateJWT,
express.static(UPLOADS_DIR))) or (b) switch to presigned URLs when you migrate
to multer-s3; additionally ensure uploaded filenames are generated using a
cryptographically secure method (crypto.randomUUID or similar) and document the
capability-URL model in the repo README.
Comment on lines
+54
to
+59
| app.use((err: Error, _req: Request, res: Response, _next: NextFunction) => { | ||
| setCors(res); | ||
| console.error(err); | ||
| const message = process.env.NODE_ENV === 'production' ? 'Internal server error' : err.message; | ||
| res.status(500).json({ error: message }); | ||
| }); |
There was a problem hiding this comment.
Use the validated env instead of process.env.NODE_ENV.
process.env.NODE_ENV is read directly here, bypassing the Zod-validated env module used elsewhere in the file. If NODE_ENV is unset or typo'd (production vs prod), this silently leaks raw error messages in what should be a production deployment.
As per coding guidelines: "Validate environment variables at startup with Zod — crash fast if missing." Add NODE_ENV (e.g. z.enum(['development','test','production']).default('development')) to env.ts and read env.NODE_ENV here.
♻️ Suggested change
- const message = process.env.NODE_ENV === 'production' ? 'Internal server error' : err.message;
+ const message = env.NODE_ENV === 'production' ? 'Internal server error' : err.message;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| app.use((err: Error, _req: Request, res: Response, _next: NextFunction) => { | |
| setCors(res); | |
| console.error(err); | |
| const message = process.env.NODE_ENV === 'production' ? 'Internal server error' : err.message; | |
| res.status(500).json({ error: message }); | |
| }); | |
| app.use((err: Error, _req: Request, res: Response, _next: NextFunction) => { | |
| setCors(res); | |
| console.error(err); | |
| const message = env.NODE_ENV === 'production' ? 'Internal server error' : err.message; | |
| res.status(500).json({ error: message }); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/index.ts` around lines 54 - 59, The error handler currently
reads process.env.NODE_ENV directly; update the env validation to include
NODE_ENV (e.g., add
z.enum(['development','test','production']).default('development') to the
exported Zod schema in env.ts) and then replace process.env.NODE_ENV with the
validated env value (env.NODE_ENV) inside the Express error middleware (the
app.use(...) error handler) so the handler uses the Zod-validated environment
variable consistently; ensure the env module is imported where the middleware is
defined and that the application crashes at startup if NODE_ENV is
missing/invalid per the schema.
Comment on lines
+218
to
+239
| const handleImageUpload = useCallback( | ||
| async (file: File) => { | ||
| const apiUrl = | ||
| (import.meta.env.VITE_API_URL as string | undefined) ?? 'http://localhost:3001'; | ||
| const formData = new FormData(); | ||
| formData.append('image', file); | ||
| try { | ||
| const res = await fetch(`${apiUrl}/api/uploads`, { method: 'POST', body: formData }); | ||
| if (!res.ok) { | ||
| const body = await res.text(); | ||
| throw new Error(`Upload failed (${res.status}): ${body}`); | ||
| } | ||
| const { url } = (await res.json()) as { url: string }; | ||
| editor.update(() => { | ||
| $insertNodeToNearestRoot($createImageNode(`${apiUrl}${url}`, file.name)); | ||
| }); | ||
| } catch (err) { | ||
| console.error('Image upload failed:', err); | ||
| } | ||
| }, | ||
| [editor], | ||
| ); |
There was a problem hiding this comment.
Validate response shape and surface upload errors to the user.
Two small gaps in handleImageUpload:
- Line 230 casts the parsed JSON to
{ url: string }without a runtime check. If the backend ever returns a different payload,urlwill beundefinedand you’ll insert anImageNodewithsrc = "${apiUrl}undefined". - Lines 234–236 swallow failures into
console.erroronly. The user clicks upload, nothing happens, and there’s no visible feedback for 413/401/network errors. Consider surfacing via a toast/InputDialog-style notification so the failure is observable.
🛡️ Suggested tightening of the response parse
- const { url } = (await res.json()) as { url: string };
- editor.update(() => {
- $insertNodeToNearestRoot($createImageNode(`${apiUrl}${url}`, file.name));
- });
+ const body = (await res.json()) as unknown;
+ if (
+ typeof body !== 'object' || body === null ||
+ typeof (body as { url?: unknown }).url !== 'string'
+ ) {
+ throw new Error('Upload response missing url');
+ }
+ const { url } = body as { url: string };
+ editor.update(() => {
+ $insertNodeToNearestRoot($createImageNode(`${apiUrl}${url}`, file.name));
+ });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/components/EditorToolbar.tsx` around lines 218 - 239, In
handleImageUpload, validate the parsed response before using it: after parsing
JSON from the upload response, ensure the object has a non-empty string url
(e.g., check typeof resJson.url === 'string' && resJson.url.trim() !== '') and
throw a descriptive error if it doesn't; this prevents inserting a broken
ImageNode via $createImageNode. Also replace the silent console.error in the
catch with user-visible feedback by invoking the app's toast/dialog utility (or
set an error state that triggers an InputDialog) with the error message/status
so 413/401/network errors are surfaced to the user; keep
editor.update($insertNodeToNearestRoot($createImageNode(...))) only when
validation succeeds.
Comment on lines
+303
to
+334
| {/* Font family dropdown */} | ||
| <div className="relative" ref={fontFamilyRef}> | ||
| <button | ||
| type="button" | ||
| className="flex items-center gap-1.5 h-7 px-2.5 rounded-md text-sm text-[#444746] hover:bg-[#f1f3f4] transition-colors" | ||
| onClick={() => setFontFamilyOpen((v) => !v)} | ||
| style={{ fontFamily }} | ||
| > | ||
| <span className="min-w-[110px] text-left truncate">{fontFamily}</span> | ||
| <ChevronDown size={12} className="text-notion-muted flex-shrink-0" /> | ||
| </button> | ||
|
|
||
| {fontFamilyOpen && ( | ||
| <div className="absolute top-full left-0 mt-1 w-52 bg-white rounded-xl shadow-xl border border-notion-border py-1 z-50"> | ||
| {FONT_FAMILIES.map(({ label, css }) => ( | ||
| <button | ||
| key={label} | ||
| type="button" | ||
| className="w-full flex items-center justify-between px-3 py-1.5 text-sm text-notion-text hover:bg-notion-hover transition-colors" | ||
| style={{ fontFamily: css }} | ||
| onMouseDown={(e) => { | ||
| e.preventDefault(); | ||
| applyFontFamily(label); | ||
| }} | ||
| > | ||
| <span>{label}</span> | ||
| {fontFamily === label && <Check size={12} className="text-notion-accent" />} | ||
| </button> | ||
| ))} | ||
| </div> | ||
| )} | ||
| </div> |
There was a problem hiding this comment.
Inline style on font‑family dropdown violates the "never write inline styles" rule.
Lines 309 and 322 use style={{ fontFamily }} / style={{ fontFamily: css }} to preview the font in the trigger button and options. Per coding guidelines, styling in .tsx must go through Tailwind utility classes. Because the font value is dynamic, the idiomatic workarounds are (a) a small safelist of font-* classes and a lookup from FONT_FAMILIES, or (b) a CSS custom property applied via a Tailwind arbitrary property utility and set through the variable (e.g., add the variable on a parent or use Tailwind's [--font:...] + font-[var(--font)] utilities in your setup).
As per coding guidelines: "Use Tailwind CSS utility classes for styling — never write inline styles."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/components/EditorToolbar.tsx` around lines 303 - 334, The
dropdown currently uses inline style for dynamic fonts (style={{ fontFamily }}
and style={{ fontFamily: css }}) which violates the "no inline styles" rule;
update the implementation in EditorToolbar.tsx to remove inline styles by either
(A) adding a small safelist of Tailwind font-* classes in FONT_FAMILIES and
render className based on a lookup (e.g., className={`font-${fontKey} ...`}
using the font identifier), or (B) set a CSS custom property on the parent
(e.g., style-wise via Tailwind arbitrary property like
className={`[--font:${css}]`} on the button container) and use a Tailwind
utility that maps to that variable (e.g., font-[var(--font)]), then replace the
style usages in the trigger button (where fontFamily is referenced) and in the
option buttons inside the FONT_FAMILIES.map to use the chosen approach; adjust
applyFontFamily usage and any state (fontFamily, fontFamilyOpen, fontFamilyRef,
FONT_FAMILIES) accordingly so visuals remain identical without inline styles.
Comment on lines
+44
to
50
| // localStorage is synchronous — delay the flip so React renders "Saving…" first | ||
| setTimeout(() => setIsSaving(false), 600); | ||
| }, DEBOUNCE_MS); | ||
|
|
||
| return () => { | ||
| if (timerRef.current !== null) clearTimeout(timerRef.current); | ||
| if (timerRef.current) clearTimeout(timerRef.current); | ||
| }; |
There was a problem hiding this comment.
Untracked inner setTimeout is not cleaned up.
The deferred setIsSaving(false) timer at line 45 is not stored in a ref, so the cleanup at line 49 only clears the outer debounce timer. If the component unmounts (or the effect re-runs) during the 600 ms window, the pending timer will still call setIsSaving(false) after teardown. Track it in a ref and clear it alongside timerRef.
♻️ Suggested fix
const [isSaving, setIsSaving] = useState(false);
const timerRef = useRef<ReturnType<typeof setTimeout> | null>(null);
+ const resetTimerRef = useRef<ReturnType<typeof setTimeout> | null>(null);
useEffect(() => {
if (!editorState) return;
if (timerRef.current) clearTimeout(timerRef.current);
timerRef.current = setTimeout(() => {
setIsSaving(true);
try {
const data: AutoSaveData = {
title,
content: JSON.stringify(editorState.toJSON()),
};
localStorage.setItem(STORAGE_KEY, JSON.stringify(data));
} catch {
// ignore storage errors
}
// localStorage is synchronous — delay the flip so React renders "Saving…" first
- setTimeout(() => setIsSaving(false), 600);
+ resetTimerRef.current = setTimeout(() => setIsSaving(false), 600);
}, DEBOUNCE_MS);
return () => {
if (timerRef.current) clearTimeout(timerRef.current);
+ if (resetTimerRef.current) clearTimeout(resetTimerRef.current);
};
}, [editorState, title]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // localStorage is synchronous — delay the flip so React renders "Saving…" first | |
| setTimeout(() => setIsSaving(false), 600); | |
| }, DEBOUNCE_MS); | |
| return () => { | |
| if (timerRef.current !== null) clearTimeout(timerRef.current); | |
| if (timerRef.current) clearTimeout(timerRef.current); | |
| }; | |
| const [isSaving, setIsSaving] = useState(false); | |
| const timerRef = useRef<ReturnType<typeof setTimeout> | null>(null); | |
| const resetTimerRef = useRef<ReturnType<typeof setTimeout> | null>(null); | |
| useEffect(() => { | |
| if (!editorState) return; | |
| if (timerRef.current) clearTimeout(timerRef.current); | |
| timerRef.current = setTimeout(() => { | |
| setIsSaving(true); | |
| try { | |
| const data: AutoSaveData = { | |
| title, | |
| content: JSON.stringify(editorState.toJSON()), | |
| }; | |
| localStorage.setItem(STORAGE_KEY, JSON.stringify(data)); | |
| } catch { | |
| // ignore storage errors | |
| } | |
| // localStorage is synchronous — delay the flip so React renders "Saving…" first | |
| resetTimerRef.current = setTimeout(() => setIsSaving(false), 600); | |
| }, DEBOUNCE_MS); | |
| return () => { | |
| if (timerRef.current) clearTimeout(timerRef.current); | |
| if (resetTimerRef.current) clearTimeout(resetTimerRef.current); | |
| }; | |
| }, [editorState, title]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/hooks/useAutoSave.ts` around lines 44 - 50, The deferred
setTimeout that calls setIsSaving(false) is not stored and therefore not cleared
on unmount; create a ref (e.g., savingFlipRef) to hold the inner timeout id,
assign the result of the inner setTimeout to savingFlipRef.current, clear any
existing savingFlipRef.current before scheduling a new one, and ensure the
effect cleanup also clears savingFlipRef.current (alongside timerRef.current) so
the pending setIsSaving(false) never runs after teardown; update usages around
setIsSaving, timerRef, and the debounce logic accordingly.
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
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Type of change
How to test
Screenshots
Checklist
npm run typecheck)npm run lint)devbranch (notmain)Summary by CodeRabbit
New Features
Improvements
Documentation
Developer Experience