Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds PDF theming and nested-list rendering to PDF export, writes exports to the OS Downloads directory and expands Tauri download permissions, introduces a PDF themes module, implements a Share flow (ShareChip, share popover, useShareLogic, multiple export handlers including Obsidian/email/copy/pdf), and small UI/text and i18n updates; chat button temporarily commented out. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as Share UI
participant Logic as useShareLogic / exportHandlers
participant PDF as exportToPDF
participant Theme as getPDFTheme
participant FS as Tauri FS (writeFile)
participant OS as Downloads
User->>UI: open Share popover
UI->>Logic: select PDF theme
User->>UI: click Export PDF
UI->>Logic: invoke exportHandlers.pdf(session, theme)
Logic->>PDF: exportToPDF(session, theme)
PDF->>Theme: getPDFTheme(theme)
Theme-->>PDF: PDFTheme
PDF->>PDF: layout pages, render nested lists & bullets, apply theme
PDF->>FS: writeFile(downloadPath, bytes)
FS-->>PDF: saved path
PDF-->>Logic: return path
Logic-->>UI: show success + path
UI-->>User: display saved path
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ 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). (2)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
| case 0: | ||
| return `${counter}.`; | ||
| case 1: | ||
| return `${String.fromCharCode(96 + counter)}.`; |
There was a problem hiding this comment.
Alphabetic list numbering breaks after the 26th item, producing non-letter characters
Prompt for AI agents
Address the following comment on apps/desktop/src/components/toolbar/utils/pdf-export.ts at line 36:
<comment>Alphabetic list numbering breaks after the 26th item, producing non-letter characters</comment>
<file context>
@@ -11,26 +14,61 @@ export type SessionData = Session & {
interface TextSegment {
text: string;
- bold?: boolean;
- italic?: boolean;
- isHeader?: number; // 1, 2, 3 for h1, h2, h3
+ isHeader?: number;
isListItem?: boolean;
+ listType?: "ordered" | "unordered";
</file context>
| { "path": "$APPDATA/**" } | ||
| { "path": "$APPDATA/**" }, | ||
| { "path": "$DOWNLOAD/*" }, | ||
| { "path": "$DOWNLOAD/**" } |
There was a problem hiding this comment.
Allowing unrestricted write-file access to the entire Downloads directory is overly permissive and could let a compromised app modify or remove any user download
Prompt for AI agents
Address the following comment on apps/desktop/src-tauri/capabilities/default.json at line 65:
<comment>Allowing unrestricted write-file access to the entire Downloads directory is overly permissive and could let a compromised app modify or remove any user download</comment>
<file context>
@@ -60,7 +60,9 @@
"identifier": "opener:allow-open-path",
"allow": [
{ "path": "$APPDATA/*" },
- { "path": "$APPDATA/**" }
+ { "path": "$APPDATA/**" },
+ { "path": "$DOWNLOAD/*" },
+ { "path": "$DOWNLOAD/**" }
]
},
</file context>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/components/toolbar/utils/pdf-export.ts (1)
506-510: Build file paths with join() for cross-OS correctness.
Concatenating with “/” will break on Windows. Use Tauri’s join for portability.Apply this diff:
-import { downloadDir } from "@tauri-apps/api/path"; +import { downloadDir, join } from "@tauri-apps/api/path";- const filePath = downloadsPath.endsWith("/") - ? `${downloadsPath}${filename}` - : `${downloadsPath}/${filename}`; + const filePath = await join(downloadsPath, filename);Also applies to: 1-1
🧹 Nitpick comments (9)
apps/desktop/src/locales/ko/messages.po (1)
1126-1133: String update aligns with component change; consider adding a Korean translation.The updated key matches the UI change in src/components/organization-profile/recent-notes.tsx. The msgstr is empty, so it will likely fall back to English. If that’s intentional, fine; otherwise, consider providing a Korean translation to avoid mixed-language UI.
apps/desktop/src/components/toolbar/utils/pdf-themes.ts (3)
28-186: Hoist the themes map to module scope and derive the available list from it.getPDFTheme reconstructs the full themes object on every call. Hoisting it to the module level reduces allocations and lets you derive getAvailableThemes from the keys to prevent drift.
Apply this diff to simplify getPDFTheme:
-export const getPDFTheme = (themeName: ThemeName): PDFTheme => { - const themes: Record<ThemeName, PDFTheme> = { - default: { +export const getPDFTheme = (themeName: ThemeName): PDFTheme => { + return themes[themeName] || themes.default; +};And define the themes map once at module scope (outside the selected range):
// Place near the top of the file const themes = { default: { font: "helvetica", colors: { background: [255, 255, 255], mainContent: [33, 33, 33], headers: [0, 0, 0], metadata: [102, 102, 102], hyprnoteLink: [59, 130, 246], separatorLine: [229, 229, 229], bullets: [75, 85, 99], }, }, // ... existing theme entries moved here unchanged ... } as const satisfies Record<ThemeName, PDFTheme>;Then refactor getAvailableThemes:
-export const getAvailableThemes = (): ThemeName[] => { - return [ - "default", - "light", - "dark", - "corporate", - "ocean", - "sunset", - "forest", - "cyberpunk", - "retro", - "spring", - "summer", - "winter", - ]; -}; +export const getAvailableThemes = (): ThemeName[] => + Object.keys(themes) as ThemeName[];
218-235: Fix font mismatches in theme descriptions.Descriptions mention “with Courier” for themes that specify helvetica (forest, cyberpunk, summer). Update for consistency.
const descriptions: Record<ThemeName, string> = { default: "Clean charcoal text on white with Helvetica", light: "Deep blues on light blue with Helvetica", dark: "Light text on deep black with Helvetica", corporate: "Professional navy on white with Times", ocean: "Ocean blues on light cyan with Helvetica", sunset: "Warm browns and oranges on cream with Times", - forest: "Forest greens on mint background with Courier", - cyberpunk: "Matrix green on space black with Courier", + forest: "Forest greens on mint background with Helvetica", + cyberpunk: "Matrix green on space black with Helvetica", retro: "Gold text on dark brown with Courier", spring: "Fresh greens on yellow-green with Courier", - summer: "Bright reds on yellow with Courier", + summer: "Bright reds on yellow with Helvetica", winter: "Deep blues on icy background with Times", };
15-25: Optional: Introduce an RGB type alias and lock tuple literals.A small readability win and stronger typing. Also consider using as const for the color arrays to ensure literal/readonly tuples throughout.
// Near the top type RGB = readonly [number, number, number]; export interface PDFTheme { font: string; colors: { background: RGB; mainContent: RGB; headers: RGB; metadata: RGB; hyprnoteLink: RGB; separatorLine: RGB; bullets: RGB; }; }And define color arrays with as const in the themes map, e.g.:
background: [255, 255, 255] as const,apps/desktop/src/components/toolbar/buttons/share-button.tsx (2)
41-41: End-to-end theme wiring for PDF export looks solid; consider memoizing theme options.
Plumbing from state → UI Select → export call is correct. Micro-optimization: getAvailableThemes() is called on every render. Memoize once to avoid re-allocating and to keep renders lighter.Apply this diff within the selected range to reference memoized options:
- {getAvailableThemes().map((theme: ThemeName) => ( + {themeOptions.map((theme: ThemeName) => (Additionally, add these changes outside the selected ranges:
// 1) Import useMemo import { useMemo, useState } from "react"; // 2) Define memoized options near other state (e.g., after line 41) const themeOptions = useMemo(() => getAvailableThemes(), []);Also applies to: 165-166, 335-356
467-472: Avoid splitting UX between handler and onSuccess; centralize user notification.
Currently the handler shows a message, and onSuccess opens the path. Prefer a single place for the UX (either the handler handles both message + openPath, or defer both to onSuccess) to keep responsibilities clear.apps/desktop/src/components/toolbar/utils/pdf-export.ts (3)
61-69: InnerHTML on untrusted content: sanitize or use DOMParser to reduce XSS risk.
You’re assigning cleanedHtml to innerHTML. Even though it’s a detached element, static analysis flags this as risky. Prefer DOMParser, or sanitize first.Apply this diff for a safer parse:
- const tempDiv = document.createElement("div"); - tempDiv.innerHTML = cleanedHtml; + const parser = new DOMParser(); + const parsed = parser.parseFromString(cleanedHtml, "text/html"); + const tempDiv = parsed.body;Note: This keeps the element detached while avoiding direct innerHTML assignment.
31-40: Alphabetic list markers break beyond “z”.
Level-1 ordered lists use String.fromCharCode(96 + counter), which produces non-letters for counter > 26. Consider an Excel-like alpha sequence.Apply this diff:
const getOrderedListMarker = (counter: number, level: number): string => { switch (level) { case 0: return `${counter}.`; case 1: - return `${String.fromCharCode(96 + counter)}.`; + return `${toAlphaSequence(counter)}.`; default: return `${toRomanNumeral(counter)}.`; } }; + +const toAlphaSequence = (n: number): string => { + // 1 -> a, 26 -> z, 27 -> aa, etc. + let s = ""; + while (n > 0) { + n--; // 1-based + s = String.fromCharCode(97 + (n % 26)) + s; + n = Math.floor(n / 26); + } + return s; +};Also applies to: 42-54
255-263: Trim “what-style” comments to match repo guidelines.
Comments like “Save current state”, “Use metadata color”, “Accept color parameter” restate the code. Prefer removing or replacing with brief “why” context when needed.Also applies to: 369-370
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
apps/desktop/src-tauri/capabilities/default.json(2 hunks)apps/desktop/src/components/organization-profile/recent-notes.tsx(1 hunks)apps/desktop/src/components/toolbar/buttons/share-button.tsx(5 hunks)apps/desktop/src/components/toolbar/utils/pdf-export.ts(8 hunks)apps/desktop/src/components/toolbar/utils/pdf-themes.ts(1 hunks)apps/desktop/src/locales/en/messages.po(1 hunks)apps/desktop/src/locales/ko/messages.po(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,ts,tsx,rs}
⚙️ CodeRabbit Configuration File
**/*.{js,ts,tsx,rs}: 1. No error handling.
2. No unused imports, variables, or functions.
3. For comments, keep it minimal. It should be about "Why", not "What".
Files:
apps/desktop/src/components/organization-profile/recent-notes.tsxapps/desktop/src/components/toolbar/utils/pdf-themes.tsapps/desktop/src/components/toolbar/utils/pdf-export.tsapps/desktop/src/components/toolbar/buttons/share-button.tsx
🧬 Code Graph Analysis (1)
apps/desktop/src/components/toolbar/buttons/share-button.tsx (1)
apps/desktop/src/components/toolbar/utils/pdf-export.ts (3)
ThemeName(8-8)getAvailableThemes(8-8)exportToPDF(304-513)
🪛 ast-grep (0.38.6)
apps/desktop/src/components/toolbar/utils/pdf-export.ts
[warning] 67-67: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: tempDiv.innerHTML = cleanedHtml
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
[warning] 67-67: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: tempDiv.innerHTML = cleanedHtml
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
🪛 Biome (2.1.2)
apps/desktop/src/components/toolbar/utils/pdf-export.ts
[error] 276-276: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 287-287: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
⏰ 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). (2)
- GitHub Check: ci (macos, macos-latest)
- GitHub Check: ci (windows, windows-latest)
🔇 Additional comments (7)
apps/desktop/src/locales/en/messages.po (1)
1126-1133: LGTM: Wording change is correctly reflected and old entry marked obsolete.The active string now uses “with this organization,” and the previous “for this organization” entry is properly marked obsolete (#~).
apps/desktop/src/components/organization-profile/recent-notes.tsx (1)
70-71: LGTM: Updated copy matches locales.The new wording is consistent with en/ko messages.po and maintains i18n via .
apps/desktop/src-tauri/capabilities/default.json (2)
63-66: Approving Downloads directory open permissions.Adding $DOWNLOAD/* and $DOWNLOAD/** to opener:allow-open-path is appropriate for opening exported PDFs from the Downloads folder.
83-86: Approve Downloads write permissions and confirm placeholder
- In apps/desktop/src-tauri/capabilities/default.json (lines 83–86),
$DOWNLOADis the correct Tauri v2 placeholder for the Downloads directory, so the current paths ("$DOWNLOAD/*","$DOWNLOAD/**") are valid.apps/desktop/src/components/toolbar/utils/pdf-themes.ts (1)
31-41: Font identifiers are valid as-isjsPDF’s built-in standard fonts can be referenced by their family name (case-insensitive), so using
"helvetica","times", and"courier"will map correctly to Helvetica, Times-Roman, and Courier in the generated PDF. When you calldoc.setFont(theme.font)without a style argument, jsPDF defaults to"normal". No changes are required here.apps/desktop/src/components/toolbar/buttons/share-button.tsx (1)
26-26: Imports for theming are correct and aligned with the new API.
The import of exportToPDF, getAvailableThemes, and ThemeName from the utils module matches the PR’s API changes.apps/desktop/src/components/toolbar/utils/pdf-export.ts (1)
344-348: Ensure PDF_STYLES.font values are valid jsPDF fonts or registered before useWe can’t confirm from the snippet whether
PDF_STYLES.fontis one of jsPDF’s built-ins (helvetica, times, courier) or if any custom fonts have been registered viaaddFileToVFS/addFont. Please verify that:• All font names in your theme (e.g. in
pdf-themes.ts) map to jsPDF’s built-ins
• Any custom fonts are registered (withjsPDF.API.addFileToVFSandjsPDF.API.addFont) before callingsetFontWithout this,
pdf.setFont(PDF_STYLES.font, "bold")may throw at runtime.
| switch (bulletType) { | ||
| case "filled-circle": | ||
| pdf.circle(x, bulletY, size * 0.85, "F"); | ||
| break; | ||
|
|
||
| case "hollow-circle": | ||
| pdf.circle(x, bulletY, size * 0.85, "S"); | ||
| break; | ||
|
|
||
| case "square": | ||
| const squareSize = size * 1.4; | ||
| pdf.rect( | ||
| x - squareSize / 2, | ||
| bulletY - squareSize / 2, | ||
| squareSize, | ||
| squareSize, | ||
| "F", | ||
| ); | ||
| break; | ||
|
|
||
| case "triangle": | ||
| const triangleSize = size * 1.15; | ||
| pdf.triangle( | ||
| x, | ||
| bulletY - triangleSize / 2, // top point | ||
| x - triangleSize / 2, | ||
| bulletY + triangleSize / 2, // bottom left | ||
| x + triangleSize / 2, | ||
| bulletY + triangleSize / 2, // bottom right | ||
| "F", | ||
| ); | ||
| break; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix Biome “noSwitchDeclarations”: wrap case-local declarations in blocks.
Biome flags const declarations inside switch cases without blocks. Wrap those cases in braces to avoid cross-case scope bleed.
Apply this diff:
switch (bulletType) {
case "filled-circle":
pdf.circle(x, bulletY, size * 0.85, "F");
break;
case "hollow-circle":
pdf.circle(x, bulletY, size * 0.85, "S");
break;
- case "square":
- const squareSize = size * 1.4;
- pdf.rect(
- x - squareSize / 2,
- bulletY - squareSize / 2,
- squareSize,
- squareSize,
- "F",
- );
- break;
+ case "square": {
+ const squareSize = size * 1.4;
+ pdf.rect(
+ x - squareSize / 2,
+ bulletY - squareSize / 2,
+ squareSize,
+ squareSize,
+ "F",
+ );
+ break;
+ }
- case "triangle":
- const triangleSize = size * 1.15;
- pdf.triangle(
- x,
- bulletY - triangleSize / 2, // top point
- x - triangleSize / 2,
- bulletY + triangleSize / 2, // bottom left
- x + triangleSize / 2,
- bulletY + triangleSize / 2, // bottom right
- "F",
- );
- break;
+ case "triangle": {
+ const triangleSize = size * 1.15;
+ pdf.triangle(
+ x,
+ bulletY - triangleSize / 2, // top point
+ x - triangleSize / 2,
+ bulletY + triangleSize / 2, // bottom left
+ x + triangleSize / 2,
+ bulletY + triangleSize / 2, // bottom right
+ "F",
+ );
+ break;
+ }
}📝 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.
| switch (bulletType) { | |
| case "filled-circle": | |
| pdf.circle(x, bulletY, size * 0.85, "F"); | |
| break; | |
| case "hollow-circle": | |
| pdf.circle(x, bulletY, size * 0.85, "S"); | |
| break; | |
| case "square": | |
| const squareSize = size * 1.4; | |
| pdf.rect( | |
| x - squareSize / 2, | |
| bulletY - squareSize / 2, | |
| squareSize, | |
| squareSize, | |
| "F", | |
| ); | |
| break; | |
| case "triangle": | |
| const triangleSize = size * 1.15; | |
| pdf.triangle( | |
| x, | |
| bulletY - triangleSize / 2, // top point | |
| x - triangleSize / 2, | |
| bulletY + triangleSize / 2, // bottom left | |
| x + triangleSize / 2, | |
| bulletY + triangleSize / 2, // bottom right | |
| "F", | |
| ); | |
| break; | |
| } | |
| switch (bulletType) { | |
| case "filled-circle": | |
| pdf.circle(x, bulletY, size * 0.85, "F"); | |
| break; | |
| case "hollow-circle": | |
| pdf.circle(x, bulletY, size * 0.85, "S"); | |
| break; | |
| case "square": { | |
| const squareSize = size * 1.4; | |
| pdf.rect( | |
| x - squareSize / 2, | |
| bulletY - squareSize / 2, | |
| squareSize, | |
| squareSize, | |
| "F", | |
| ); | |
| break; | |
| } | |
| case "triangle": { | |
| const triangleSize = size * 1.15; | |
| pdf.triangle( | |
| x, | |
| bulletY - triangleSize / 2, // top point | |
| x - triangleSize / 2, | |
| bulletY + triangleSize / 2, // bottom left | |
| x + triangleSize / 2, | |
| bulletY + triangleSize / 2, // bottom right | |
| "F", | |
| ); | |
| break; | |
| } | |
| } |
🧰 Tools
🪛 Biome (2.1.2)
[error] 276-276: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 287-287: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🤖 Prompt for AI Agents
In apps/desktop/src/components/toolbar/utils/pdf-export.ts around lines 266 to
298, the "square" and "triangle" switch cases declare consts inside cases which
Biome flags; wrap each case body in its own block (add { ... } after the case
label and before the break) so the const declarations are block-scoped, keeping
the existing pdf.rect/pdf.triangle calls and break statements unchanged.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/components/editor-area/note-header/chips/index.tsx (1)
29-41: Remove unused propisCompact.
isCompactis accepted but unused in this component. The repo guidelines disallow unused variables. Drop it here and at call sites (if any).export default function NoteHeaderChips({ sessionId, hashtags = [], isVeryNarrow = false, isNarrow = false, - isCompact = false, }: { sessionId: string; hashtags?: string[]; isVeryNarrow?: boolean; isNarrow?: boolean; - isCompact?: boolean; }) {
🧹 Nitpick comments (9)
apps/desktop/src/components/toolbar/bars/main-toolbar.tsx (1)
6-6: Remove commented-out ShareButton (or gate via a feature flag) and add a short “why”.Commented code tends to linger and drifts from the source of truth. Since sharing moved to the header chips, either delete these lines or wrap with a feature flag if you expect a quick rollback. If you keep it, add a succinct “why” comment (e.g., “Share moved to note header while toolbar version is deprecated”).
Apply this cleanup:
-// import { ShareButton } from "@/components/toolbar/buttons/share-button"; ... - {/*isNote && <ShareButton />*/}Also applies to: 60-60
apps/desktop/src/components/editor-area/note-header/chips/index.tsx (1)
1-3: Strip commented-out chat code; prefer a feature flag and “why” note.Large commented blocks add noise. If chat is temporarily disabled, gate it with a boolean and remove dead code. If you must comment, keep one short “why” line.
Example cleanup:
-// Temporarily commented out chat functionality -// import { useRightPanel } from "@/contexts"; -// import { MessageCircleMore } from "lucide-react"; ... -// Temporarily commented out StartChatButton -// function StartChatButton({ isVeryNarrow }: { isVeryNarrow: boolean }) { -// const { togglePanel } = useRightPanel(); -// const handleChatClick = () => { -// togglePanel("chat"); -// }; -// return ( -// <button -// onClick={handleChatClick} -// className="flex flex-row items-center gap-1 rounded-md px-2 py-1.5 hover:bg-neutral-100 flex-shrink-0 text-xs transition-colors" -// > -// <MessageCircleMore size={14} className="flex-shrink-0" /> -// {!isVeryNarrow && <span className="truncate">Chat</span>} -// </button> -// ); -// } ... - {/* Temporarily commented out chat button */} - {/* <StartChatButton isVeryNarrow={isVeryNarrow} /> */}Also applies to: 10-27, 52-53
apps/desktop/src/components/editor-area/note-header/chips/share-chip.tsx (2)
16-21: Minor: memoizehandleOpenChangeto reduce re-renders.Not critical, but wrapping in
useCallbackavoids re-creating the handler on every render.- const handleOpenChange = (newOpen: boolean) => { + const handleOpenChange = useCallback((newOpen: boolean) => { setOpen(newOpen); if (hasEnhancedNote) { handleOpenStateChange(newOpen); } - }; + }, [hasEnhancedNote, handleOpenStateChange]);(Remember to import
useCallback.)
49-60: Consider routing through i18n for user-visible strings.“Share Enhanced Note” and “Enhanced Note Required” are hardcoded; other parts of the app localize strings.
apps/desktop/src/components/editor-area/note-header/share-button-header.tsx (5)
53-72: Remove internal try/catch incopyhandler; let the mutation’sonErrorhandle failures.The repo guideline says “No error handling.” Also, react-query already centralizes error handling. Streamline the logic and keep a single error path.
- copy: async (session: Session): Promise<ExportResult> => { - try { - let textToCopy = ""; - - if (session.enhanced_memo_html) { - textToCopy = html2md(session.enhanced_memo_html); - } else if (session.raw_memo_html) { - textToCopy = html2md(session.raw_memo_html); - } else { - textToCopy = session.title || "No content available"; - } - - await navigator.clipboard.writeText(textToCopy); - return { type: "copy", success: true }; - } catch (error) { - console.error("Failed to copy to clipboard:", error); - throw new Error("Failed to copy note to clipboard"); - } - }, + copy: async (session: Session): Promise<ExportResult> => { + const textToCopy = + (session.enhanced_memo_html && html2md(session.enhanced_memo_html)) ?? + (session.raw_memo_html && html2md(session.raw_memo_html)) ?? + session.title ?? + "No content available"; + await navigator.clipboard.writeText(textToCopy); + return { type: "copy", success: true }; + },
231-268: Align with “no error handling”: drop local try/catch and side-effects; let the query surface errors.Local catch logs and side-effects (opening Obsidian) complicate control flow and violate the repo rule. Return data or let errors propagate; handle UX in the caller (react-query
onError) if needed.-async function fetchObsidianFolders(): Promise<ObsidianFolder[]> { - try { - const [apiKey, baseUrl] = await Promise.all([ - obsidianCommands.getApiKey(), - obsidianCommands.getBaseUrl(), - ]); - client.setConfig({ fetch: tauriFetch, auth: apiKey!, baseUrl: baseUrl! }); - const response = await getVault({ client }); - const folders = response.data?.files - ?.filter(item => item.endsWith("/")) - ?.map(folder => ({ value: folder.slice(0, -1), label: folder.slice(0, -1) })) || []; - return [{ value: "default", label: "Default (Root)" }, ...folders]; - } catch (error) { - console.error("Failed to fetch Obsidian folders:", error); - obsidianCommands.getDeepLinkUrl("").then((url) => { openUrl(url); }).catch((error) => { - console.error("Failed to open Obsidian:", error); - }); - return [{ value: "default", label: "Default (Root)" }]; - } -} +async function fetchObsidianFolders(): Promise<ObsidianFolder[]> { + const [apiKey, baseUrl] = await Promise.all([ + obsidianCommands.getApiKey(), + obsidianCommands.getBaseUrl(), + ]); + client.setConfig({ fetch: tauriFetch, auth: apiKey!, baseUrl: baseUrl! }); + const response = await getVault({ client }); + const folders = + response.data?.files + ?.filter((item) => item.endsWith("/")) + ?.map((folder) => ({ value: folder.slice(0, -1), label: folder.slice(0, -1) })) ?? []; + return [{ value: "default", label: "Default (Root)" }, ...folders]; +}Optionally, add an
onErrorto theuseQuerythat shows a message or deep-links to Obsidian (keeps concerns separated).
421-424: Remove catch intoggleExpanded; let query error policy handle it.Local console logging and fallback here break the “no error handling” rule and split error paths between places.
- }).catch((error) => { - console.error("Error fetching Obsidian data:", error); - setSelectedObsidianFolder("default"); - }); + });
270-305: Type the transcript helpers (avoidany).You already have
Word2[](plugins/db/js/bindings.gen.ts). Use concrete types forwordsandspeakerto reduce runtime surprises.Example:
function convertWordsToTranscript(words: Word2[]): string { /* ... */ } function getSpeakerLabel(speaker: Word2["speaker"] | null | undefined): string { /* ... */ }
554-571: Avoid double-instantiatinguseShareLogicin both ShareChip and SharePopoverContent.Both components call
useShareLogic(), creating separate state instances and duplicate queries. Consider liftinguseShareLogic()intoShareChipand passing the return object intoSharePopoverContentas props.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
apps/desktop/src/components/editor-area/note-header/chips/index.tsx(2 hunks)apps/desktop/src/components/editor-area/note-header/chips/share-chip.tsx(1 hunks)apps/desktop/src/components/editor-area/note-header/share-button-header.tsx(1 hunks)apps/desktop/src/components/toolbar/bars/main-toolbar.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,ts,tsx,rs}
⚙️ CodeRabbit Configuration File
**/*.{js,ts,tsx,rs}: 1. No error handling.
2. No unused imports, variables, or functions.
3. For comments, keep it minimal. It should be about "Why", not "What".
Files:
apps/desktop/src/components/editor-area/note-header/share-button-header.tsxapps/desktop/src/components/toolbar/bars/main-toolbar.tsxapps/desktop/src/components/editor-area/note-header/chips/share-chip.tsxapps/desktop/src/components/editor-area/note-header/chips/index.tsx
🧬 Code Graph Analysis (3)
apps/desktop/src/components/editor-area/note-header/share-button-header.tsx (6)
plugins/db/js/bindings.gen.ts (2)
Session(181-181)Tag(183-183)packages/tiptap/src/shared/utils.ts (1)
html2md(48-50)apps/desktop/src/components/toolbar/utils/pdf-export.ts (1)
exportToPDF(304-513)packages/obsidian/generated/sdk.gen.ts (3)
putVaultByFilename(1141-1156)patchVaultByFilename(1092-1107)getVault(907-918)apps/desktop/src/contexts/hypr.tsx (1)
useHypr(63-69)packages/utils/src/contexts/sessions.tsx (1)
useSession(49-74)
apps/desktop/src/components/editor-area/note-header/chips/share-chip.tsx (1)
apps/desktop/src/components/editor-area/note-header/share-button-header.tsx (2)
useShareLogic(327-552)SharePopoverContent(555-721)
apps/desktop/src/components/editor-area/note-header/chips/index.tsx (1)
apps/desktop/src/components/editor-area/note-header/chips/share-chip.tsx (1)
ShareChip(12-44)
⏰ 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). (2)
- GitHub Check: ci (macos, macos-latest)
- GitHub Check: ci (windows, windows-latest)
apps/desktop/src/components/editor-area/note-header/chips/index.tsx
Outdated
Show resolved
Hide resolved
apps/desktop/src/components/editor-area/note-header/chips/share-chip.tsx
Show resolved
Hide resolved
apps/desktop/src/components/editor-area/note-header/share-button-header.tsx
Show resolved
Hide resolved
apps/desktop/src/components/editor-area/note-header/share-button-header.tsx
Show resolved
Hide resolved
apps/desktop/src/components/editor-area/note-header/share-button-header.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/components/toolbar/utils/pdf-export.ts (1)
510-514: Use cross-platform path join instead of manual slash handling.Windows uses backslashes; mixing separators can produce odd paths. Use Tauri’s
joinhelper.Apply this diff:
-import { downloadDir } from "@tauri-apps/api/path"; +import { downloadDir, join } from "@tauri-apps/api/path"; ... - const downloadsPath = await downloadDir(); - const filePath = downloadsPath.endsWith("/") - ? `${downloadsPath}${filename}` - : `${downloadsPath}/${filename}`; + const downloadsPath = await downloadDir(); + const filePath = await join(downloadsPath, filename);
♻️ Duplicate comments (2)
apps/desktop/src/components/toolbar/utils/pdf-export.ts (2)
31-41: Fix alphabetic list numbering beyond "z" (use base-26: a..z, aa, ab, ...).Current implementation uses
String.fromCharCode(96 + counter), which breaks at 27th item. Replace with a base-26 converter and use it here. This was flagged earlier and remains unresolved.Apply this diff within the marker function:
const getOrderedListMarker = (counter: number, level: number): string => { switch (level) { case 0: return `${counter}.`; case 1: - return `${String.fromCharCode(96 + counter)}.`; + return `${toAlphabeticLabel(counter)}.`; default: return `${toRomanNumeral(counter)}.`; } };And add this helper (place it near the numeral helpers):
// 1 -> a, 2 -> b, ..., 26 -> z, 27 -> aa, ... const toAlphabeticLabel = (n: number): string => { let s = ""; while (n > 0) { n--; // make 0-indexed s = String.fromCharCode(97 + (n % 26)) + s; n = Math.floor(n / 26); } return s; };
270-302: Wrap switch-case declarations in blocks to satisfy Biome and prevent cross-case scope bleed.
constdeclarations insidecase "square"andcase "triangle"must be block-scoped. This also aligns with the earlier automated suggestion and linter errors.Apply this diff:
switch (bulletType) { case "filled-circle": pdf.circle(x, bulletY, size * 0.85, "F"); break; case "hollow-circle": pdf.circle(x, bulletY, size * 0.85, "S"); break; - case "square": - const squareSize = size * 1.4; - pdf.rect( - x - squareSize / 2, - bulletY - squareSize / 2, - squareSize, - squareSize, - "F", - ); - break; + case "square": { + const squareSize = size * 1.4; + pdf.rect( + x - squareSize / 2, + bulletY - squareSize / 2, + squareSize, + squareSize, + "F", + ); + break; + } - case "triangle": - const triangleSize = size * 1.15; - pdf.triangle( - x, - bulletY - triangleSize / 2, // top point - x - triangleSize / 2, - bulletY + triangleSize / 2, // bottom left - x + triangleSize / 2, - bulletY + triangleSize / 2, // bottom right - "F", - ); - break; + case "triangle": { + const triangleSize = size * 1.15; + pdf.triangle( + x, + bulletY - triangleSize / 2, // top point + x - triangleSize / 2, + bulletY + triangleSize / 2, // bottom left + x + triangleSize / 2, + bulletY + triangleSize / 2, // bottom right + "F", + ); + break; + } }
🧹 Nitpick comments (1)
apps/desktop/src/components/toolbar/utils/pdf-export.ts (1)
156-169: Remove unused bullet type or ensure it’s reachable.
"triangle"is part ofTextSegment["bulletType"]and supported bydrawVectorBullet, but never selected inbulletTypes. Either add it for deeper levels, or drop it from the union for consistency.Example (if you want to use triangle at deeper nesting):
- const bulletTypes = ["filled-circle", "hollow-circle", "square"] as const; + const bulletTypes = ["filled-circle", "hollow-circle", "square", "triangle"] as const; ... - bulletType: type === "unordered" - ? (level <= 2 ? bulletTypes[level] : "square") + bulletType: type === "unordered" + ? (level < bulletTypes.length ? bulletTypes[level] : bulletTypes[bulletTypes.length - 1]) : undefined,
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
apps/desktop/src/components/toolbar/utils/pdf-export.ts(8 hunks)apps/desktop/src/components/toolbar/utils/pdf-themes.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/components/toolbar/utils/pdf-themes.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,ts,tsx,rs}
⚙️ CodeRabbit Configuration File
**/*.{js,ts,tsx,rs}: 1. No error handling.
2. No unused imports, variables, or functions.
3. For comments, keep it minimal. It should be about "Why", not "What".
Files:
apps/desktop/src/components/toolbar/utils/pdf-export.ts
🧬 Code Graph Analysis (1)
apps/desktop/src/components/toolbar/utils/pdf-export.ts (1)
apps/desktop/src/components/toolbar/utils/pdf-themes.ts (2)
ThemeName(1-14)getPDFTheme(29-202)
🪛 ast-grep (0.38.6)
apps/desktop/src/components/toolbar/utils/pdf-export.ts
[warning] 67-67: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: tempDiv.innerHTML = cleanedHtml
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 67-67: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: tempDiv.innerHTML = cleanedHtml
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
🪛 Biome (2.1.2)
apps/desktop/src/components/toolbar/utils/pdf-export.ts
[error] 280-280: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 291-291: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
⏰ 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). (3)
- GitHub Check: ci (windows, windows-latest)
- GitHub Check: ci (macos, macos-latest)
- GitHub Check: zizmor
🔇 Additional comments (2)
apps/desktop/src/components/toolbar/utils/pdf-export.ts (2)
67-67: Sanity check: ensure untrusted HTML can’t leak script text into the PDF.Even with DOMParser, if
script/stylenodes weren’t removed, their text could be included as plain text via the TEXT_NODE branch. The proposed removal of unsafe nodes in the earlier comment addresses this.If you want to be extra defensive, also gate in
processNode:- } else if (node.nodeType === Node.ELEMENT_NODE) { + } else if (node.nodeType === Node.ELEMENT_NODE) { const element = node as Element; const tagName = element.tagName.toLowerCase(); + if (tagName === "script" || tagName === "style") return;
290-301: No fallback needed forpdf.triangle
pdf.triangleis part of the core jsPDF API (since before v3.0.1) and is always included when you import from"jspdf". In our desktop app we use jsPDF 3.0.1, so the method will always exist—there’s no plugin registration or tree-shaking omission to guard against. (parallax.github.io)
1 participant
Summary by cubic
Upgraded PDF export with themes, better list rendering, and saving to the system Downloads folder. Added a theme picker to Share → PDF and updated desktop permissions.
New Features
Refactors