82 pdf preview viewer instead of new tab#89
Conversation
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
corates | d00275b | Commit Preview URL | Dec 18 2025, 10:04 PM |
WalkthroughAdds a Drawer UI component and integrates a PDF preview drawer: new Drawer export, a pdfPreviewStore, a PdfPreviewPanel rendered in ProjectView, wheel and gesture pinch-zoom in the PDF viewer, handler changes to load/cache PDF data, and new slide-in animations. Changes
sequenceDiagram
participant User
participant Handler as useProjectPdfHandlers
participant Store as pdfPreviewStore
participant Cache as Cache/Download
participant Panel as PdfPreviewPanel (Drawer)
participant Viewer as PdfViewer / usePdfJs
User->>Handler: Click "Preview PDF"
Handler->>Store: openPreview(projectId, studyId, pdfInfo)
Store-->>Panel: isOpen=true, loading=true
par Load PDF Data
Handler->>Cache: getCachedPdf(pdfInfo)
alt cached
Cache-->>Handler: cached data
else not cached
Handler->>Cache: downloadPdf(pdfInfo)
Cache-->>Handler: downloaded data
Handler->>Cache: cachePdf(data) (fire-and-forget)
end
end
Handler->>Store: setData(pdfData)
Store-->>Panel: pdfData assigned, loading=false
Panel->>Viewer: Render(pdfData)
User->>Viewer: Ctrl + Wheel / Gesture
Viewer->>usePdfJs: handleWheel / gesture handlers -> setScale(clamped)
Viewer-->>User: Re-render at new zoom
User->>Panel: Close
Panel->>Store: closePreview()
Store-->>Panel: isOpen=false (cleared after delay)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/web/src/components/checklist-ui/pdf/usePdfJs.js (1)
80-86: Clean up listeners from the old ref before setting a new one.If
setScrollContainerRefis called multiple times with different refs (e.g., during component re-renders or ref changes), the event listeners on the previousscrollContainerRefare never removed. This creates a memory leak and leaves listeners attached to stale DOM nodes.🔎 Proposed fix to clean up old listeners
// Set scroll container ref and setup scroll listener function setScrollContainerRef(ref) { + // Clean up listeners from old ref + if (scrollContainerRef) { + scrollContainerRef.removeEventListener('scroll', handleScroll); + scrollContainerRef.removeEventListener('wheel', handleWheel); + } + scrollContainerRef = ref; if (ref) { ref.addEventListener('scroll', handleScroll); ref.addEventListener('wheel', handleWheel, { passive: false }); } }
🧹 Nitpick comments (3)
packages/web/src/components/project-ui/PdfPreviewPanel.jsx (1)
37-67: Consider usingSwitch/Matchfor mutually exclusive states.The three
Showblocks represent mutually exclusive states (loading, error, success). Using SolidJS'sSwitch/Matchwould make the exclusivity explicit and prevent potential overlap if store state becomes inconsistent.🔎 Optional refactor
-import { Show } from 'solid-js'; +import { Show, Switch, Match } from 'solid-js'; ... <div class='h-full flex flex-col'> - {/* Loading state */} - <Show when={pdfPreviewStore.loading()}> - <div class='flex-1 flex items-center justify-center bg-gray-100'> - <div class='flex items-center gap-3 text-gray-500'> - <div class='animate-spin rounded-full h-6 w-6 border-b-2 border-blue-600' /> - Loading PDF... - </div> - </div> - </Show> - - {/* Error state */} - <Show when={pdfPreviewStore.error()}> - <div class='flex-1 flex items-center justify-center bg-gray-100'> - <div class='text-center'> - <p class='text-red-600 mb-2'>Failed to load PDF</p> - <p class='text-sm text-gray-500'>{pdfPreviewStore.error()}</p> - </div> - </div> - </Show> - - {/* PDF Viewer */} - <Show - when={!pdfPreviewStore.loading() && !pdfPreviewStore.error() && pdfPreviewStore.pdfData()} - > - <PdfViewer - pdfData={pdfPreviewStore.pdfData()} - pdfFileName={pdfPreviewStore.pdf()?.fileName} - readOnly={true} - /> - </Show> + <Switch> + <Match when={pdfPreviewStore.loading()}> + <div class='flex-1 flex items-center justify-center bg-gray-100'> + <div class='flex items-center gap-3 text-gray-500'> + <div class='animate-spin rounded-full h-6 w-6 border-b-2 border-blue-600' /> + Loading PDF... + </div> + </div> + </Match> + <Match when={pdfPreviewStore.error()}> + <div class='flex-1 flex items-center justify-center bg-gray-100'> + <div class='text-center'> + <p class='text-red-600 mb-2'>Failed to load PDF</p> + <p class='text-sm text-gray-500'>{pdfPreviewStore.error()}</p> + </div> + </div> + </Match> + <Match when={pdfPreviewStore.pdfData()}> + <PdfViewer + pdfData={pdfPreviewStore.pdfData()} + pdfFileName={pdfPreviewStore.pdf()?.fileName} + readOnly={true} + /> + </Match> + </Switch> </div>packages/ui/src/zag/Drawer.jsx (2)
23-30: Avoid wrapping props in derived functions—access props directly or usecreateMemofor computed values.In SolidJS, props are already reactive getters. Creating wrapper functions like
const open = () => props.openadds unnecessary overhead and is not idiomatic. Access props directly (e.g.,props.open) or usecreateMemowhen you need to derive a value.🔎 Refactor to use props directly
- const open = () => props.open; - const side = () => props.side || 'right'; - const size = () => props.size || 'md'; - const title = () => props.title; - const description = () => props.description; - const children = () => props.children; - const showHeader = () => props.showHeader ?? true; - const showBackdrop = () => props.showBackdrop ?? true; + // Access props directly in JSX and pass to helpers as neededThen update references throughout the component to use
props.open,props.side || 'right', etc., or define helpers that accept props as arguments.
43-73: Memoize helper functions to avoid unnecessary recalculation.The helper functions
getSizeClass,getPositionClasses, andgetAnimationClassesare recalculated on every render. Wrap them increateMemoso they only recompute when their dependencies change.🔎 Memoize helpers for better performance
- const getSizeClass = () => { - switch (size()) { + const sizeClass = createMemo(() => { + switch (props.size || 'md') { case 'sm': return 'w-80'; // 320px case 'md': @@ -54,19 +54,19 @@ default: return 'w-[480px]'; } - }; + }); - const getPositionClasses = () => { - if (side() === 'left') { + const positionClasses = createMemo(() => { + if ((props.side || 'right') === 'left') { return 'left-0 top-0 bottom-0'; } return 'right-0 top-0 bottom-0'; - }; + }); - const getAnimationClasses = () => { + const animationClasses = createMemo(() => { // CSS animations for slide in/out - if (side() === 'left') { + if ((props.side || 'right') === 'left') { return 'animate-slide-in-left'; } return 'animate-slide-in-right'; - }; + });Then update references to use
sizeClass(),positionClasses(), andanimationClasses().
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/ui/src/zag/Drawer.jsx(1 hunks)packages/ui/src/zag/index.js(1 hunks)packages/web/src/components/checklist-ui/pdf/usePdfJs.js(2 hunks)packages/web/src/components/project-ui/PdfPreviewPanel.jsx(1 hunks)packages/web/src/components/project-ui/ProjectView.jsx(2 hunks)packages/web/src/global.css(1 hunks)packages/web/src/primitives/useProjectPdfHandlers.js(2 hunks)packages/web/src/stores/pdfPreviewStore.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{jsx,tsx,js,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
For UI icons, use the
solid-iconslibrary or SVGs only. Do not use emojis
Files:
packages/ui/src/zag/index.jspackages/ui/src/zag/Drawer.jsxpackages/web/src/primitives/useProjectPdfHandlers.jspackages/web/src/components/project-ui/PdfPreviewPanel.jsxpackages/web/src/components/project-ui/ProjectView.jsxpackages/web/src/components/checklist-ui/pdf/usePdfJs.jspackages/web/src/stores/pdfPreviewStore.js
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,jsx,ts,tsx}: Prefer modern ES6+ syntax and features
Use aliases for imports when appropriate to improve readability
Use Zod for schema and input validation
**/*.{js,jsx,ts,tsx}: Follow standard JavaScript/SolidJS/Cloudflare best practices
Prefer modern ES6+ syntax and features
Use aliases for imports when appropriate to improve readability
Keep files small, focused, and modular. If a file exceeds a high number of lines, consider refactoring by extracting sub-modules into a folder, moving complex logic into separate utility files or primitives, or splitting large forms into section components
Each file should handle one coherent responsibility
Use Zod for schema and input validation
Files:
packages/ui/src/zag/index.jspackages/ui/src/zag/Drawer.jsxpackages/web/src/primitives/useProjectPdfHandlers.jspackages/web/src/components/project-ui/PdfPreviewPanel.jsxpackages/web/src/components/project-ui/ProjectView.jsxpackages/web/src/components/checklist-ui/pdf/usePdfJs.jspackages/web/src/stores/pdfPreviewStore.js
**/*.{js,jsx,ts,tsx,md,json,sql}
📄 CodeRabbit inference engine (.cursorrules)
Do not use emojis in code, comments, documentation, or commit messages
Files:
packages/ui/src/zag/index.jspackages/ui/src/zag/Drawer.jsxpackages/web/src/primitives/useProjectPdfHandlers.jspackages/web/src/components/project-ui/PdfPreviewPanel.jsxpackages/web/src/components/project-ui/ProjectView.jsxpackages/web/src/components/checklist-ui/pdf/usePdfJs.jspackages/web/src/stores/pdfPreviewStore.js
**/*.{jsx,tsx}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{jsx,tsx}: For UI icons, use thesolid-iconslibrary or SVGs only. Do not use emojis
Use responsive design principles for UI components
Files:
packages/ui/src/zag/Drawer.jsxpackages/web/src/components/project-ui/PdfPreviewPanel.jsxpackages/web/src/components/project-ui/ProjectView.jsx
packages/web/src/**/*.{jsx,tsx,js,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Create reusable logic in primitives (hooks) that can be shared across components
Files:
packages/web/src/primitives/useProjectPdfHandlers.jspackages/web/src/components/project-ui/PdfPreviewPanel.jsxpackages/web/src/components/project-ui/ProjectView.jsxpackages/web/src/components/checklist-ui/pdf/usePdfJs.jspackages/web/src/stores/pdfPreviewStore.js
packages/web/src/components/**/*.{jsx,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
packages/web/src/components/**/*.{jsx,tsx}: Use responsive design principles for UI components
Use Zag.js for UI components and design system
Reuse existing Zag components from packages/web/src/components/zag/* and check the README.md list before adding new components
Components should receive at most 1–5 props for local configuration only, not shared state
Components should be lean and focused on rendering, not implement business logic. Move business logic into stores, utilities, or primitives
Never have a component act as a God component coordinating multiple large concerns
packages/web/src/components/**/*.{jsx,tsx}: Group related components in subdirectories with anindex.jsbarrel export
Use Zag.js for UI components and design system
Zag components exist inpackages/web/src/components/zag/*and should be reused. Check the README.md in that folder for a list of existing components before adding new components and when debugging
Do NOT prop-drill application state. Shared or cross-feature state must live in external stores under packages/web/src/stores/ or relative to the component file. Import stores directly where needed instead of passing values through multiple components
Components should receive at most 1–5 props, and only for local configuration, not shared state. If a component would need more than 5 props, move the shared data into an external store, a primitive, or Solid context (when scoped to a feature)
With SolidJS, do not destructure props as it breaks reactivity. Instead, access props directly from the props object or wrap them in a function to ensure they are always up-to-date
Components should be lean and focused. They should not implement business logic; move that into stores, utilities, or primitives. Never have a component act as a 'God component' coordinating multiple large concerns
Files:
packages/web/src/components/project-ui/PdfPreviewPanel.jsxpackages/web/src/components/project-ui/ProjectView.jsx
packages/web/src/components/**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Group related components in subdirectories with an index.js barrel export
Files:
packages/web/src/components/project-ui/PdfPreviewPanel.jsxpackages/web/src/components/project-ui/ProjectView.jsxpackages/web/src/components/checklist-ui/pdf/usePdfJs.js
packages/web/src/**/*.{jsx,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
packages/web/src/**/*.{jsx,tsx}: Do NOT prop-drill application state in SolidJS. Use external stores under packages/web/src/stores/ or component-relative stores instead
Do not destructure props in SolidJS components; access props directly or wrap them in a function to maintain reactivity
Use createMemo for derived values and createStore for complex state in SolidJS
packages/web/src/**/*.{jsx,tsx}: When you need to compute a value based on props or state in SolidJS, usecreateMemoto ensure it updates reactively
When you have complex state or state objects, use SolidJS'screateStorefor better performance and reactivity
You may create reusable logic in 'primitives' (hooks) that can be shared across components. This keeps components clean and focused on rendering
Files:
packages/web/src/components/project-ui/PdfPreviewPanel.jsxpackages/web/src/components/project-ui/ProjectView.jsx
🧠 Learnings (9)
📚 Learning: 2025-12-18T05:00:29.870Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-18T05:00:29.870Z
Learning: Applies to packages/web/src/components/**/*.{jsx,tsx} : Reuse existing Zag components from packages/web/src/components/zag/* and check the README.md list before adding new components
Applied to files:
packages/ui/src/zag/index.jspackages/ui/src/zag/Drawer.jsx
📚 Learning: 2025-12-18T05:00:29.870Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-18T05:00:29.870Z
Learning: Applies to packages/web/src/components/**/*.{jsx,tsx} : Use Zag.js for UI components and design system
Applied to files:
packages/ui/src/zag/index.jspackages/ui/src/zag/Drawer.jsx
📚 Learning: 2025-12-18T17:05:47.242Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-18T17:05:47.242Z
Learning: Applies to packages/web/src/components/**/*.{jsx,tsx} : Zag components exist in `packages/web/src/components/zag/*` and should be reused. Check the README.md in that folder for a list of existing components before adding new components and when debugging
Applied to files:
packages/ui/src/zag/index.jspackages/ui/src/zag/Drawer.jsx
📚 Learning: 2025-12-18T05:00:29.870Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-18T05:00:29.870Z
Learning: Applies to packages/web/src/**/*.{jsx,tsx,js,ts} : Create reusable logic in primitives (hooks) that can be shared across components
Applied to files:
packages/ui/src/zag/index.js
📚 Learning: 2025-12-18T17:05:47.242Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-18T17:05:47.242Z
Learning: Applies to packages/web/src/components/**/*.{jsx,tsx} : Group related components in subdirectories with an `index.js` barrel export
Applied to files:
packages/ui/src/zag/index.js
📚 Learning: 2025-12-18T17:05:47.242Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-18T17:05:47.242Z
Learning: Applies to packages/web/src/**/*.{jsx,tsx} : You may create reusable logic in 'primitives' (hooks) that can be shared across components. This keeps components clean and focused on rendering
Applied to files:
packages/ui/src/zag/index.js
📚 Learning: 2025-12-18T05:00:29.870Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-18T05:00:29.870Z
Learning: Applies to packages/web/src/components/**/*.{jsx,tsx} : Use responsive design principles for UI components
Applied to files:
packages/ui/src/zag/index.js
📚 Learning: 2025-12-18T05:00:29.870Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-18T05:00:29.870Z
Learning: Applies to packages/web/src/components/**/*.{js,jsx,ts,tsx} : Group related components in subdirectories with an index.js barrel export
Applied to files:
packages/ui/src/zag/index.js
📚 Learning: 2025-12-18T05:00:29.870Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-18T05:00:29.870Z
Learning: Applies to packages/web/src/**/*.{jsx,tsx} : Do NOT prop-drill application state in SolidJS. Use external stores under packages/web/src/stores/ or component-relative stores instead
Applied to files:
packages/web/src/stores/pdfPreviewStore.js
🧬 Code graph analysis (4)
packages/web/src/primitives/useProjectPdfHandlers.js (3)
packages/web/src/stores/pdfPreviewStore.js (4)
studyId(13-13)pdfPreviewStore(69-84)projectId(12-12)packages/web/src/primitives/pdfCache.js (2)
getCachedPdf(82-102)cachePdf(111-138)packages/web/src/api/pdf-api.js (1)
downloadPdf(79-93)
packages/web/src/components/project-ui/PdfPreviewPanel.jsx (3)
packages/ui/src/zag/Drawer.jsx (3)
open(23-23)title(26-26)Drawer(22-128)packages/web/src/stores/pdfPreviewStore.js (2)
pdfPreviewStore(69-84)packages/web/src/components/checklist-ui/pdf/PdfViewer.jsx (1)
PdfViewer(17-116)
packages/web/src/components/project-ui/ProjectView.jsx (1)
packages/web/src/components/project-ui/PdfPreviewPanel.jsx (1)
PdfPreviewPanel(13-71)
packages/web/src/stores/pdfPreviewStore.js (1)
packages/web/src/lib/referenceLookup.js (1)
pdfInfo(406-406)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Workers Builds: corates
🔇 Additional comments (9)
packages/web/src/components/checklist-ui/pdf/usePdfJs.js (1)
643-643: LGTM: Cleanup properly removes the wheel listener.The wheel event listener is correctly removed in the cleanup handler, matching the setup in
setScrollContainerRef.packages/web/src/global.css (1)
94-119: LGTM - Slide-in animations are well-defined.The keyframes and utility classes follow the existing pattern established by toast animations. The cubic-bezier timing function provides a smooth deceleration effect.
Consider adding corresponding
slide-out-rightandslide-out-leftanimations for a smoother close transition if the Drawer component supports exit animations in the future.packages/ui/src/zag/index.js (1)
11-11: LGTM - Drawer export follows existing conventions.The named export for
Draweris consistent with howDialogand other components are exported.packages/web/src/components/project-ui/ProjectView.jsx (2)
30-30: LGTM - Import follows project component conventions.
295-296: LGTM - Placement is appropriate for a drawer component.The
PdfPreviewPanelis correctly rendered at the root level alongside other modal-like components (AddMemberModal,ConfirmDialogComponent). Reading from the store directly instead of prop-drilling aligns with the coding guidelines.packages/web/src/primitives/useProjectPdfHandlers.js (1)
14-14: LGTM - Store import for PDF preview state management.packages/web/src/components/project-ui/PdfPreviewPanel.jsx (1)
13-25: LGTM - Clean handler and title derivation.The
handleOpenChangecorrectly syncs drawer state with the store. Thetitle()function provides sensible defaults.packages/ui/src/zag/Drawer.jsx (1)
75-127: Well-structured Drawer implementation with proper Zag.js integration.The component correctly uses Zag's dialog machine, spreads API props for accessibility, and follows the retrieved learnings for using Zag.js. The Portal, Show conditionals, and icon usage (solid-icons) all align with the coding guidelines.
Based on learnings, Zag.js should be used for UI components in this codebase, and this implementation follows that pattern correctly.
packages/web/src/stores/pdfPreviewStore.js (1)
69-84: Good use of external store pattern.The store correctly exposes signals as read-only accessors and provides action methods for state updates. This aligns with the retrieved learning that external stores should be used instead of prop-drilling in SolidJS.
Based on learnings, this pattern avoids prop-drilling and follows SolidJS best practices for global state management.
| // Handle pinch-to-zoom via wheel event with ctrlKey (trackpad gesture) | ||
| function handleWheel(e) { | ||
| // Only handle pinch-to-zoom (ctrlKey is set for trackpad pinch gestures) | ||
| if (!e.ctrlKey) return; | ||
|
|
||
| e.preventDefault(); | ||
|
|
||
| // Use deltaY directly for smooth, proportional zooming | ||
| // Smaller divisor = more sensitive, larger = less sensitive | ||
| const zoomDelta = -e.deltaY * 0.01; | ||
| const newScale = Math.min(Math.max(scale() + zoomDelta, 0.5), 3.0); | ||
| setScale(newScale); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, examine the actual file to see the handleWheel function and its context
cat -n packages/web/src/components/checklist-ui/pdf/usePdfJs.js | head -130Repository: InfinityBowman/corates
Length of output: 5592
🏁 Script executed:
# Search for existing wheel event handling patterns in the codebase
rg -n 'wheel|deltaY|ctrlKey' packages/web/src/components/checklist-ui/ --type=js --type=ts -A 2 -B 2Repository: InfinityBowman/corates
Length of output: 2271
🌐 Web query:
ctrlKey trackpad pinch gesture detection cross-browser compatibility
💡 Result:
Short answer
- The most cross-browser way to detect a trackpad pinch (pinch-to-zoom) is to listen for the wheel event and check the ctrlKey (or metaKey on macOS where appropriate) plus the wheel deltas — browsers map pinch trackpad gestures to wheel events with ctrlKey set. Use that for Chrome, Firefox and most Edge builds. [1][2]
What to watch for
- Safari (WebKit) also exposes proprietary GestureEvent events (gesturestart/gesturechange/gestureend with .scale/.rotation) but they are non‑standard and not supported in other browsers. Don’t rely on them alone. [2][3]
- Behaviour differences:
- delta units and sign/scale vary by device and platform; you must normalize deltas and/or accumulate them. [1]
- On macOS you may see metaKey vs ctrlKey semantics in some contexts — detect both if you need macOS support.
- Some Windows touchpad drivers or precision vs non-precision devices may behave differently. [1]
Minimal robust pattern (example)
- Listen for wheel, require ctrlKey (or metaKey), preventDefault if you want to block browser zoom, and compute a scale factor from deltaY (or deltaZ where present). Example (concise):
document.addEventListener('wheel', e => {
// treat pinch-to-zoom when modifier is set
if (e.ctrlKey || e.metaKey) {
e.preventDefault(); // optional: stop native zoom
const zoomDelta = -e.deltaY; // sign/scale may need tuning
// convert zoomDelta to a sensible scale change (tweak factor)
const scaleChange = Math.exp(zoomDelta * 0.001);
// apply scaleChange to your content...
}
}, { passive: false });
Practical tips
- Use passive: false so you can call preventDefault().
- Throttle/debounce updates and accumulate deltas for smooth scaling.
- Fall back to WebKit GestureEvent only as an enhancement (feature-detect before using). Example: if ('ongesturechange' in window) { window.addEventListener('gesturechange', ...); } [2][3]
- Test across Chrome, Firefox, Safari and Windows precision-touchpad setups and macOS trackpads.
Key references
- Kenneth Auchenberg — detecting pinch via wheel + ctrlKey (works in Chrome/Firefox/Edge). [1]
- MDN GestureEvent (non‑standard WebKit gesture events: gesturestart/gesturechange/gestureend). [2]
- Compatibility notes for gesturechange (WebKit-only, non-standard). [3]
Sources
[1] Kenneth Auchenberg — Detecting multi-touch trackpad gestures in JavaScript.
[2] MDN Web/API GestureEvent (gesturestart/gesturechange/gestureend, non‑standard).
[3] Element: gesturechange event — Web APIs (compatibility notes).
Add metaKey check and consider Safari GestureEvent fallback for complete cross-platform pinch-to-zoom support.
The wheel-based zoom works correctly for Chrome, Firefox, and Edge. However, verification reveals two cross-platform gaps:
-
Missing
metaKeycheck for macOS contexts — some macOS scenarios emitmetaKeyinstead ofctrlKey. Update the condition toif (!(e.ctrlKey || e.metaKey)) return; -
Safari's proprietary
GestureEvent(with.scaleproperty) provides native trackpad pinch support on macOS/iOS but is non-standard and not emitted in other browsers. Consider adding a feature-detect fallback:
if ('ongesturechange' in window) {
ref.addEventListener('gesturechange', (e) => {
const newScale = Math.min(Math.max(scale() * e.scale, 0.5), 3.0);
setScale(newScale);
});
}Additionally, add throttling to prevent rapid re-renders during fast wheel/pinch events — batch or debounce setScale calls for better performance.
🤖 Prompt for AI Agents
In packages/web/src/components/checklist-ui/pdf/usePdfJs.js around lines 88-100,
the wheel handler only checks e.ctrlKey and doesn't support macOS metaKey or
Safari GestureEvent and can cause rapid re-renders; update the condition to
check both e.ctrlKey || e.metaKey, add a feature-detect fallback that listens
for window gesture events (e.g., ongsturechange/gesturechange) and applies
e.scale to compute and clamp the new scale, and introduce throttling/debouncing
(or requestAnimationFrame batching) around setScale to avoid rapid state updates
during fast wheel/gesture events; attach the gesture listener to the same ref
cleanup and ensure all listeners are removed in cleanup.
| const handleViewPdf = async (studyId, pdf) => { | ||
| if (!pdf || !pdf.fileName) return; | ||
|
|
||
| const cachedData = await getCachedPdf(projectId, studyId, pdf.fileName); | ||
| if (cachedData) { | ||
| const blob = new Blob([cachedData], { type: 'application/pdf' }); | ||
| const blobUrl = URL.createObjectURL(blob); | ||
| window.open(blobUrl, '_blank'); | ||
| setTimeout(() => URL.revokeObjectURL(blobUrl), 60000); | ||
| return; | ||
| } | ||
| // Open the preview panel immediately | ||
| pdfPreviewStore.openPreview(projectId, studyId, pdf); | ||
|
|
||
| try { | ||
| // Try cache first | ||
| let data = await getCachedPdf(projectId, studyId, pdf.fileName); | ||
|
|
||
| if (!data) { | ||
| // Fetch from server | ||
| data = await downloadPdf(projectId, studyId, pdf.fileName); | ||
| // Cache for future use | ||
| await cachePdf(projectId, studyId, pdf.fileName, data).catch(console.warn); | ||
| } | ||
|
|
||
| const url = getPdfUrl(projectId, studyId, pdf.fileName); | ||
| window.open(url, '_blank'); | ||
| pdfPreviewStore.setData(data); | ||
| } catch (err) { | ||
| console.error('Error loading PDF:', err); | ||
| pdfPreviewStore.setError(err.message || 'Failed to load PDF'); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Consider guarding against race conditions when loading PDFs.
If a user quickly opens multiple PDFs in succession, the async setData call could update the panel with stale data from an earlier request. Consider tracking the current request (e.g., by studyId or a request counter) and discarding results that don't match.
🔎 Suggested approach
const handleViewPdf = async (studyId, pdf) => {
if (!pdf || !pdf.fileName) return;
// Open the preview panel immediately
pdfPreviewStore.openPreview(projectId, studyId, pdf);
+ const requestedStudyId = studyId;
+ const requestedFileName = pdf.fileName;
try {
// Try cache first
let data = await getCachedPdf(projectId, studyId, pdf.fileName);
if (!data) {
// Fetch from server
data = await downloadPdf(projectId, studyId, pdf.fileName);
// Cache for future use
await cachePdf(projectId, studyId, pdf.fileName, data).catch(console.warn);
}
+ // Ensure we're still showing the same PDF
+ if (pdfPreviewStore.studyId() !== requestedStudyId ||
+ pdfPreviewStore.pdf()?.fileName !== requestedFileName) {
+ return; // User switched to a different PDF
+ }
+
pdfPreviewStore.setData(data);
} catch (err) {
console.error('Error loading PDF:', err);
+ // Only set error if still on the same PDF
+ if (pdfPreviewStore.studyId() === requestedStudyId) {
pdfPreviewStore.setError(err.message || 'Failed to load PDF');
+ }
}
};🤖 Prompt for AI Agents
In packages/web/src/primitives/useProjectPdfHandlers.js around lines 39 to 61,
the async PDF load can race when users open multiple PDFs quickly; to fix, add a
per-handler request identifier (e.g., incrementing counter or generate a token
and store it as currentRequest for the preview) before awaiting cache/download,
pass that token into the async flow, and before calling pdfPreviewStore.setData
or setError verify the token still matches the stored currentRequest (discard
results if it doesn’t); also ensure pdfPreviewStore.openPreview sets the current
request token so stale responses don’t overwrite the active preview.
| function openPreview(projId, stId, pdfInfo) { | ||
| setProjectId(projId); | ||
| setStudyId(stId); | ||
| setPdf(pdfInfo); | ||
| setPdfData(null); | ||
| setError(null); | ||
| setLoading(true); | ||
| setIsOpen(true); | ||
| } |
There was a problem hiding this comment.
Add input validation using Zod.
The openPreview function doesn't validate its inputs. Per coding guidelines, use Zod for schema and input validation to ensure type safety and prevent runtime errors from invalid data.
🔎 Add Zod schema validation
+import { z } from 'zod';
+
+// Define the PDF info schema
+const PdfInfoSchema = z.object({
+ id: z.string(),
+ fileName: z.string(),
+ tag: z.string().optional(),
+ // Add other expected fields
+});
+
/**
* Open the PDF preview drawer
* @param {string} projId - Project ID
* @param {string} stId - Study ID
* @param {Object} pdfInfo - PDF object { id, fileName, tag, ... }
*/
function openPreview(projId, stId, pdfInfo) {
+ // Validate inputs
+ z.string().parse(projId);
+ z.string().parse(stId);
+ PdfInfoSchema.parse(pdfInfo);
+
setProjectId(projId);
setStudyId(stId);
setPdf(pdfInfo);
setPdfData(null);
setError(null);
setLoading(true);
setIsOpen(true);
}As per coding guidelines, Zod should be used for schema and input validation across the codebase.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/web/src/stores/pdfPreviewStore.js around lines 25 to 33, the
openPreview function lacks input validation; add a Zod schema that validates
projId and stId (e.g., nonempty strings) and pdfInfo (object with required
fields you expect, like url string) then parse/validate the inputs at the start
of openPreview; if validation fails, call setError with the validation message
and do not mutate state, otherwise proceed to setProjectId, setStudyId, setPdf,
setPdfData(null), setError(null), setLoading(true), setIsOpen(true). Ensure you
import Zod, keep validation synchronous with z.parse or try/catch z.safeParse,
and avoid changing existing success-path behavior.
| function closePreview() { | ||
| setIsOpen(false); | ||
| // Clear state after animation completes | ||
| setTimeout(() => { | ||
| setProjectId(null); | ||
| setStudyId(null); | ||
| setPdf(null); | ||
| setPdfData(null); | ||
| setLoading(false); | ||
| setError(null); | ||
| }, 300); | ||
| } |
There was a problem hiding this comment.
Fix race condition when drawer is reopened before timeout completes.
The setTimeout in closePreview will clear state even if the drawer is reopened before the 300ms delay elapses. If a user closes the drawer and quickly opens a new PDF, the timeout from the first close will clear the state of the newly opened PDF.
Scenario:
- Open PDF A
- Close drawer (timeout queued for 300ms)
- After 100ms, open PDF B
- After 200ms more, the timeout fires and clears PDF B's state
🔎 Recommended fix using a timeout reference
+// Timeout reference to prevent race conditions
+let closeTimeoutId = null;
+
/**
* Open the PDF preview drawer
* @param {string} projId - Project ID
* @param {string} stId - Study ID
* @param {Object} pdfInfo - PDF object { id, fileName, tag, ... }
*/
function openPreview(projId, stId, pdfInfo) {
+ // Cancel any pending close timeout
+ if (closeTimeoutId) {
+ clearTimeout(closeTimeoutId);
+ closeTimeoutId = null;
+ }
+
setProjectId(projId);
setStudyId(stId);
setPdf(pdfInfo);
setPdfData(null);
setError(null);
setLoading(true);
setIsOpen(true);
}
/**
* Close the PDF preview drawer
*/
function closePreview() {
setIsOpen(false);
// Clear state after animation completes
- setTimeout(() => {
+ closeTimeoutId = setTimeout(() => {
setProjectId(null);
setStudyId(null);
setPdf(null);
setPdfData(null);
setLoading(false);
setError(null);
+ closeTimeoutId = null;
}, 300);
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/web/src/stores/pdfPreviewStore.js around lines 38 to 49, the
setTimeout in closePreview can clear state for a newly opened PDF if the drawer
is reopened before the 300ms delay elapses; fix by tracking the timeout id in a
module/store-scoped variable (e.g., closeTimeoutId), clear any existing timeout
when opening or before scheduling a new one (clearTimeout(closeTimeoutId)),
assign the returned id when calling setTimeout, and clear the stored id after
the timeout handler runs so a stale timer cannot wipe out newly set state.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/web/src/components/checklist-ui/pdf/usePdfJs.js (2)
100-105: Consider throttling scale updates to improve zoom performance.Both
handleGestureChangeandhandleWheelcallsetScaleon every event, which can trigger rapid re-renders of all PDF pages during fast pinch or wheel gestures. Throttling or batching updates viarequestAnimationFramewould improve performance.💡 Example throttling approach
// Add at hook level (near other local variables around line 44) let scaleUpdateFrame = null; function throttledSetScale(newScale) { if (scaleUpdateFrame !== null) { cancelAnimationFrame(scaleUpdateFrame); } scaleUpdateFrame = requestAnimationFrame(() => { setScale(newScale); scaleUpdateFrame = null; }); } // Then use throttledSetScale in both handlers: function handleGestureChange(e) { e.preventDefault(); const newScale = Math.min(Math.max(gestureStartScale * e.scale, 0.5), 3.0); throttledSetScale(newScale); // ← use throttled version } function handleWheel(e) { if (!(e.ctrlKey || e.metaKey)) return; e.preventDefault(); const zoomDelta = -e.deltaY * 0.01; const newScale = Math.min(Math.max(scale() + zoomDelta, 0.5), 3.0); throttledSetScale(newScale); // ← use throttled version }Don't forget to cancel the pending frame in
onCleanup(around line 627):if (scaleUpdateFrame !== null) { cancelAnimationFrame(scaleUpdateFrame); }Based on learnings from the past review comment.
Also applies to: 112-123
111-123: AddmetaKeycheck for complete macOS pinch-zoom support.The wheel handler only checks
e.ctrlKey, which may not work in all macOS contexts wheremetaKeyis set instead.🔎 Proposed fix
function handleWheel(e) { // Only handle pinch-to-zoom (ctrlKey is set for trackpad pinch gestures) - if (!e.ctrlKey) return; + if (!(e.ctrlKey || e.metaKey)) return; e.preventDefault();Based on learnings from the past review comment.
🧹 Nitpick comments (2)
.cursorrules (1)
23-23: Consider making the browser compatibility guideline more actionable.The guideline appropriately raises awareness about browser compatibility, especially relevant for the PDF preview feature in this PR. However, it could be more helpful by specifying:
- Concrete testing strategies (e.g., "Test in latest Safari on macOS/iOS")
- Known Safari issues to watch for (e.g., PDF rendering, touch gestures, CSS features)
- Resources or tools for compatibility checking
Example enhancement
-- Ensure browser compatibility for all frontend code (Safari is usually problematic). +- Ensure browser compatibility for all frontend code, especially Safari: + - Test in latest Safari on macOS and iOS + - Watch for issues with: PDF rendering, touch/gesture events, CSS Grid, flexbox gaps + - Use caniuse.com or MDN browser compatibility tables for reference.clauderc (1)
23-23: Good addition for cross-browser awareness.This guideline is relevant and well-placed, especially given the Drawer and PDF preview features in this PR that can have Safari-specific issues.
Optional: Make the guideline more actionable
Consider expanding this guideline with more specific guidance to help developers:
-- Ensure browser compatibility for all frontend code (Safari is usually problematic). +- Ensure browser compatibility for all frontend code across Chrome, Firefox, Safari, and Edge (latest 2 versions). + - Safari often requires specific testing for: CSS features (backdrop-filter, container queries), modern JS APIs, touch/gesture events, and PDF rendering. + - Test interactive UI components (modals, drawers, dropdowns) in Safari before merging.This provides clearer testing expectations and highlights common Safari pain points.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.clauderc(1 hunks).cursorrules(1 hunks).github/copilot-instructions.md(1 hunks)packages/web/src/components/checklist-ui/pdf/usePdfJs.js(2 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{jsx,tsx,js,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
For UI icons, use the
solid-iconslibrary or SVGs only. Do not use emojis
Files:
packages/web/src/components/checklist-ui/pdf/usePdfJs.js
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,jsx,ts,tsx}: Prefer modern ES6+ syntax and features
Use aliases for imports when appropriate to improve readability
Use Zod for schema and input validation
**/*.{js,jsx,ts,tsx}: Follow standard JavaScript/SolidJS/Cloudflare best practices
Prefer modern ES6+ syntax and features
Use aliases for imports when appropriate to improve readability
Keep files small, focused, and modular. If a file exceeds a high number of lines, consider refactoring by extracting sub-modules into a folder, moving complex logic into separate utility files or primitives, or splitting large forms into section components
Each file should handle one coherent responsibility
Use Zod for schema and input validation
Files:
packages/web/src/components/checklist-ui/pdf/usePdfJs.js
packages/web/src/components/**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Group related components in subdirectories with an index.js barrel export
Files:
packages/web/src/components/checklist-ui/pdf/usePdfJs.js
packages/web/src/**/*.{jsx,tsx,js,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Create reusable logic in primitives (hooks) that can be shared across components
Files:
packages/web/src/components/checklist-ui/pdf/usePdfJs.js
**/*.{js,jsx,ts,tsx,md,json,sql}
📄 CodeRabbit inference engine (.cursorrules)
Do not use emojis in code, comments, documentation, or commit messages
Files:
packages/web/src/components/checklist-ui/pdf/usePdfJs.js
🧠 Learnings (8)
📚 Learning: 2025-12-18T17:05:47.242Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-18T17:05:47.242Z
Learning: Applies to **/*.{js,jsx,ts,tsx} : Follow standard JavaScript/SolidJS/Cloudflare best practices
Applied to files:
.clauderc.cursorrules.github/copilot-instructions.md
📚 Learning: 2025-12-18T05:00:29.870Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-18T05:00:29.870Z
Learning: Applies to packages/web/src/components/**/*.{jsx,tsx} : Use responsive design principles for UI components
Applied to files:
.clauderc.cursorrules.github/copilot-instructions.md
📚 Learning: 2025-12-18T17:05:47.242Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-18T17:05:47.242Z
Learning: Applies to **/*.{jsx,tsx} : Use responsive design principles for UI components
Applied to files:
.clauderc.cursorrules.github/copilot-instructions.md
📚 Learning: 2025-12-18T17:05:47.242Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-18T17:05:47.242Z
Learning: Applies to **/*.{js,jsx,ts,tsx} : Keep files small, focused, and modular. If a file exceeds a high number of lines, consider refactoring by extracting sub-modules into a folder, moving complex logic into separate utility files or primitives, or splitting large forms into section components
Applied to files:
.clauderc.cursorrules.github/copilot-instructions.md
📚 Learning: 2025-12-18T05:00:29.870Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-18T05:00:29.870Z
Learning: Keep files small, focused, and modular. Extract sub-modules into folders with index.jsx and helper components when a file exceeds a high number of lines
Applied to files:
.clauderc.cursorrules.github/copilot-instructions.md
📚 Learning: 2025-12-18T17:05:47.243Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-18T17:05:47.243Z
Learning: Place plans in the docs/plans/ directory
Applied to files:
.clauderc.cursorrules.github/copilot-instructions.md
📚 Learning: 2025-12-18T17:05:47.243Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-18T17:05:47.243Z
Learning: Reference the architecture-goals.md file for relevant architecture and design patterns to follow
Applied to files:
.clauderc.cursorrules.github/copilot-instructions.md
📚 Learning: 2025-12-18T17:05:47.243Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-18T17:05:47.243Z
Learning: Reference the style-guide.md file for additional style and formatting guidelines
Applied to files:
.clauderc.cursorrules.github/copilot-instructions.md
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Workers Builds: corates
🔇 Additional comments (1)
.github/copilot-instructions.md (1)
23-23: Well-aligned addition to coding standards.The new browser compatibility guideline is a timely and appropriate addition, especially given this PR introduces new UI components (Drawer, PDF preview panel, animations) that require cross-browser testing and Safari-specific workarounds. This strengthens the project's documentation for handling frontend compatibility concerns.
1 participant
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.