fix: resolve 4 bugs in Draftdeckai - #1466
Conversation
|
Someone is attempting to deploy a commit to the muneeralimca2025-5238's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThe changes add export rejection logging and replace several JavaScript patterns for DOM clearing, decimal parsing, and final-path access. ChangesRuntime cleanup updates
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Biome (2.5.6)app/dashboard/export/page.tsxFile contains syntax errors that prevent linting: Line 406: Expected a statement but instead found '.catch(err => console.error("Promise.all failed:", err))'. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/diagram/diagram-preview.tsx (1)
193-204: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winFinish the XSS surface cleanup.
textContentremoves the stale diagram, but Mermaid still writes SVG into a DOM node withinnerHTML. WithsecurityLevel: "loose", this path can render HTML labels generated fromvisualContent/Mermaid code. Sanitize the rendered SVG before insertion, or render only trusted Mermaid sources withstrict/sandboxand replace the looseinnerHTMLwith a safe DOM parse path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/diagram/diagram-preview.tsx` around lines 193 - 204, Sanitize the SVG returned by mermaid.render before inserting it in the diagramContainer created by the render flow, eliminating the direct unsafe innerHTML assignment while preserving diagram rendering. Use the project’s established sanitization utility, or configure Mermaid to strict/sandbox and parse the sanitized SVG through a safe DOM path; ensure content derived from visualContent/code cannot introduce executable HTML.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/dashboard/export/page.tsx`:
- Around line 405-406: Remove the detached .catch expression after the
ExportPage component; it is not attached to a Promise and causes invalid
TypeScript syntax. Keep fetchStats’s existing surrounding try/catch as the sole
Promise.all failure handler, unless logging is explicitly moved onto the
Promise.all expression itself.
In `@components/presentation/mobile-presentation-generator.tsx`:
- Line 540: Update the pageCount input handler to clamp the parsed value to a
minimum of 3 before applying the MAX_FREE_PAGES upper bound, ensuring negative
or otherwise below-minimum input never reaches the API. Preserve the existing
fallback behavior for invalid input and use the surrounding page-count update
logic in the mobile presentation generator.
---
Outside diff comments:
In `@components/diagram/diagram-preview.tsx`:
- Around line 193-204: Sanitize the SVG returned by mermaid.render before
inserting it in the diagramContainer created by the render flow, eliminating the
direct unsafe innerHTML assignment while preserving diagram rendering. Use the
project’s established sanitization utility, or configure Mermaid to
strict/sandbox and parse the sanitized SVG through a safe DOM path; ensure
content derived from visualContent/code cannot introduce executable HTML.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fa4febc9-f803-4e40-b5fd-18e1bfc64f43
📒 Files selected for processing (4)
app/dashboard/export/page.tsxcomponents/diagram/diagram-preview.tsxcomponents/presentation/mobile-presentation-generator.tsxcomponents/resume/resume-preview.tsx
|
|
||
| .catch(err => console.error("Promise.all failed:", err)); No newline at end of file |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Remove the detached .catch before merging.
Line 406 is invalid TypeScript syntax because it follows the closing brace of ExportPage instead of a Promise expression. The file cannot compile.
fetchStats already catches Promise.all failures through its surrounding try/catch at Lines 92-155. Remove Lines 405-406, or attach the handler directly to the Promise.all expression if separate logging is required.
Proposed fix
-}
-
-.catch(err => console.error("Promise.all failed:", err));
+}📝 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.
| .catch(err => console.error("Promise.all failed:", err)); | |
| } |
🧰 Tools
🪛 Biome (2.5.6)
[error] 406-406: Expected a statement but instead found '.catch(err => console.error("Promise.all failed:", err))'.
(parse)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/dashboard/export/page.tsx` around lines 405 - 406, Remove the detached
.catch expression after the ExportPage component; it is not attached to a
Promise and causes invalid TypeScript syntax. Keep fetchStats’s existing
surrounding try/catch as the sole Promise.all failure handler, unless logging is
explicitly moved onto the Promise.all expression itself.
Source: Linters/SAST tools
| onChange={(e) => | ||
| setPageCount( | ||
| Math.min(parseInt(e.target.value) || 3, MAX_FREE_PAGES), | ||
| Math.min(parseInt(e.target.value, 10) || 3, MAX_FREE_PAGES), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Clamp pageCount to the lower bound.
For input -1, parseInt(e.target.value, 10) returns -1, and Math.min stores -1. The HTML min="3" attribute does not prevent this state update. Clamp the value with Math.max(3, ...) before sending pageCount to the API.
Proposed fix
- Math.min(parseInt(e.target.value, 10) || 3, MAX_FREE_PAGES),
+ Math.max(
+ 3,
+ Math.min(
+ parseInt(e.target.value, 10) || 3,
+ MAX_FREE_PAGES,
+ ),
+ ),📝 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.
| Math.min(parseInt(e.target.value, 10) || 3, MAX_FREE_PAGES), | |
| Math.max( | |
| 3, | |
| Math.min( | |
| parseInt(e.target.value, 10) || 3, | |
| MAX_FREE_PAGES, | |
| ), | |
| ), |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@components/presentation/mobile-presentation-generator.tsx` at line 540,
Update the pageCount input handler to clamp the parsed value to a minimum of 3
before applying the MAX_FREE_PAGES upper bound, ensuring negative or otherwise
below-minimum input never reaches the API. Preserve the existing fallback
behavior for invalid input and use the surrounding page-count update logic in
the mobile presentation generator.
👷 Deploy request for docmagic-muneer pending review.Visit the deploys page to approve it
|
👷 Deploy request for docmagic1 pending review.Visit the deploys page to approve it
|
1 participant
Description
This PR fixes real bugs found in the codebase:
parseInt: without10, strings like'0x1F'or'08'parse in unintended bases.innerHTMLassignment withtextContent: prevents HTML injection / XSS and is faster since it does not parse markup.arr[arr.length - 1]with.at(-1): cleaner access to the last element.Promise.all: an unhandled rejection in any input promise previously crashed silently.Type of Change
How Has This Been Tested?
Checklist
Related Issue
Ref: #1465
Summary by CodeRabbit