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. 📝 WalkthroughWalkthroughNew annotation functionality is introduced to the editor area. This includes the addition of two new React components— Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant EditorArea
participant TextSelectionPopover
participant AnnotationBox
participant LLM/Backend
User->>EditorArea: Selects text in enhanced note
EditorArea->>TextSelectionPopover: Renders popover if selection
User->>TextSelectionPopover: Clicks "Annotate"
TextSelectionPopover->>EditorArea: handleAnnotate(selectedText, rect)
EditorArea->>AnnotationBox: Renders with selectedText, rect, sessionId
AnnotationBox->>LLM/Backend: Streams explanation for selectedText
LLM/Backend-->>AnnotationBox: Streamed explanation chunks
AnnotationBox->>User: Displays streamed explanation in popover
User->>AnnotationBox: Clicks close/cancel
AnnotationBox->>EditorArea: onCancel()
EditorArea->>AnnotationBox: Unmounts popover
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. ✨ 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 comments)
Other keywords and placeholders
Documentation and Community
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
cubic analysis
2 issues found across 3 files • Review in cubic
React with 👍 or 👎 to teach cubic. You can also tag @cubic-dev-ai to give feedback, ask questions, or re-run the review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (7)
apps/desktop/src/components/editor-area/text-selection-popover.tsx (2)
57-57: UX nit: reduce debounce to feel snappier600ms feels sluggish. 200–300ms is typical for selection popovers.
- }, 600); + }, 250);
9-10: Comments should explain “why”, not “what”Trim or reword these to convey intent (“why we hide when box is open”), not the obvious behavior.
Also applies to: 30-35, 88-91
apps/desktop/src/components/editor-area/index.tsx (3)
186-192: Consider storing a rect snapshot instead of DOMRectDOMRect isn’t serializable and can be misleading in logs. A plain snapshot is simpler to reason about.
Example shape:
type Rect = Pick<DOMRect, "left" | "top" | "right" | "bottom" | "width" | "height">; const [annotationBox, setAnnotationBox] = useState<{ selectedText: string; selectedRect: Rect } | null>(null);…and construct the snapshot in handleAnnotate.
243-251: Comment style + potential scoping
- The comment is “what”, not “why”. Suggest: “Why: keep onboarding UI minimal; avoid extra UI during first-run.”
- Optional: if you ever support multiple editors on a page, pass a container/ref to TextSelectionPopover to scope selection to that editor.
252-259: Position won’t update on scroll/resizeAnnotationBox uses a static DOMRect. If the user scrolls or resizes, it can drift. Consider re-positioning on scroll/resize with a throttled effect.
apps/desktop/src/components/editor-area/annotation-box.tsx (2)
107-117: Guideline check: error handlingCoding guidelines for ts/tsx say “No error handling.” This block shows a toast and rethrows. Confirm if this is an approved exception for user feedback, or if error handling should be centralized elsewhere.
26-37: Comments are “what”, not “why”Trim “CRITICAL/Cleanup/Start analysis/Cancel function/Header with close button” style comments. Keep only rationale where needed (e.g., “why fixed height”).
Also applies to: 159-165, 166-193, 253-269, 271-280
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/desktop/src/components/editor-area/annotation-box.tsx(1 hunks)apps/desktop/src/components/editor-area/index.tsx(3 hunks)apps/desktop/src/components/editor-area/text-selection-popover.tsx(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/editor-area/text-selection-popover.tsxapps/desktop/src/components/editor-area/annotation-box.tsxapps/desktop/src/components/editor-area/index.tsx
🪛 Biome (2.1.2)
apps/desktop/src/components/editor-area/annotation-box.tsx
[error] 310-310: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
🪛 ast-grep (0.38.6)
apps/desktop/src/components/editor-area/annotation-box.tsx
[warning] 309-309: Usage of dangerouslySetInnerHTML detected. This bypasses React's built-in XSS protection. Always sanitize HTML content using libraries like DOMPurify before injecting it into the DOM to prevent XSS attacks.
Context: dangerouslySetInnerHTML
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://reactjs.org/docs/dom-elements.html#dangerouslysetinnerhtml
- https://cwe.mitre.org/data/definitions/79.html
(react-unsafe-html-injection)
🔇 Additional comments (2)
apps/desktop/src/components/editor-area/index.tsx (2)
193-200: LGTM on handlersSimple and clear. No unnecessary complexity.
201-202: Good gating for enhanced contentThis prevents the popover on raw/empty notes.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
apps/desktop/src/components/editor-area/annotation-box.tsx (3)
309-316: Eliminate dangerouslySetInnerHTML; render safely to prevent XSSLLM output is untrusted. Injecting via dangerouslySetInnerHTML is an XSS risk and violates lint/security guidance. Render as React nodes or sanitize first.
Apply in-place diff:
- <div - className="prose prose-xs max-w-none" - dangerouslySetInnerHTML={{ - __html: streamedContent - .replace(/\*\*(.*?)\*\*/g, "<strong>$1</strong>") - .replace(/\n/g, "<br>"), - }} - /> + <div className="prose prose-xs max-w-none"> + {renderStreamedContent(streamedContent)} + </div>Add helper in this file (outside the selected range):
function renderStreamedContent(text: string) { if (!text) return null; const escapeHtml = (s: string) => s.replace(/&/g, "&").replace(/</g, "<").replace(/>/g, ">"); const renderBold = (line: string, keyPrefix: string) => { const out: (string | JSX.Element)[] = []; const re = /\*\*(.+?)\*\*/g; let last = 0, m: RegExpExecArray | null; while ((m = re.exec(line))) { if (m.index > last) out.push(line.slice(last, m.index)); out.push(<strong key={`${keyPrefix}-b-${out.length}`}>{m[1]}</strong>); last = m.index + m[0].length; } if (last < line.length) out.push(line.slice(last)); return out; }; return escapeHtml(text).split("\n").map((ln, i, arr) => ( <span key={`ln-${i}`}> {renderBold(ln, `ln-${i}`)} {i < arr.length - 1 ? <br /> : null} </span> )); }
38-43: Prevent duplicate analysis on window focus; trigger mutate once via query onSuccessEffect depends on llmConnectionQuery.data while query refetches on focus, causing repeated mutate calls.
Apply diffs:
- Disable refetch-on-focus and run mutate once in onSuccess:
const llmConnectionQuery = useQuery({ queryKey: ["llm-connection"], queryFn: () => connectorCommands.getLlmConnection(), - refetchOnWindowFocus: true, + refetchOnWindowFocus: false, + onSuccess: () => { + sourceAnalysisMutation.mutate(); + }, });
- Remove the effect that re-triggers mutate:
- // Start analysis when component mounts - useEffect(() => { - if (llmConnectionQuery.data) { - sourceAnalysisMutation.mutate(); - } - }, [llmConnectionQuery.data]);Also applies to: 160-166
212-218: Fix top clamping; incorrect Math.min pins popover to marginClamp “below” to bottom margin and use Math.max for “above” to enforce a minimum top margin.
- if (canFitBelow) { - // Prefer below if there's space for max height - top = selectedRect.bottom + minDistanceFromText; - } else if (canFitAbove) { - // Use above if below doesn't work but above does - top = selectedRect.top - maxPopoverHeight - minDistanceFromText; + if (canFitBelow) { + // Prefer below if there's space; clamp to bottom margin + top = Math.min( + selectedRect.bottom + minDistanceFromText, + window.innerHeight - margin - maxPopoverHeight, + ); + } else if (canFitAbove) { + // Use above; clamp to top margin + top = Math.max( + margin, + selectedRect.top - maxPopoverHeight - minDistanceFromText, + ); } else { // Neither fits perfectly - choose the side with more space if (spaceBelow >= spaceAbove) { // More space below - position to fit in available space top = Math.max( selectedRect.bottom + minDistanceFromText, window.innerHeight - Math.min(maxPopoverHeight, spaceBelow) - margin, ); } else { // More space above - position to fit in available space - top = Math.min( - selectedRect.top - minDistanceFromText - maxPopoverHeight, - margin, - ); + top = Math.max( + margin, + selectedRect.top - minDistanceFromText - maxPopoverHeight, + ); } }Also applies to: 227-231
🧹 Nitpick comments (3)
apps/desktop/src/components/editor-area/annotation-box.tsx (3)
53-60: Avoid redundant LLM connection fetch; reuse query data with fallbackYou already fetch the connection via useQuery; calling connectorCommands.getLlmConnection() again is unnecessary.
- const [{ type }, words] = await Promise.all([ - connectorCommands.getLlmConnection(), - getWordsFunc(sessionId), - ]); + const [words] = await Promise.all([getWordsFunc(sessionId)]); + const type = + llmConnectionQuery.data?.type ?? + (await connectorCommands.getLlmConnection()).type;
26-36: Trim comments; keep only “why”, not “what”Several comments narrate implementation details (“CRITICAL”, “Create NEW abort controller”, header labels). Per guidelines, keep comments minimal and focused on rationale.
Suggestion:
- Remove descriptive/obvious comments.
- Keep a single brief comment explaining the rationale (e.g., “Why we combine abort + timeout”).
Also applies to: 49-52, 87-93, 98-106, 296-305
61-63: Bound prompt size; consider contextualization for long transcriptsConcatenating full transcript can exceed model/context limits and increase latency/cost.
Options (incremental):
- Hard cap characters/tokens sent to the model.
- Prefer contextualization: locate occurrences of selectedText (case-insensitive), send ±N words around top matches, plus a brief preface, instead of full transcript.
- If needed, chunk transcript and stream retrieval progressively.
Example simple cap:
const MAX_CHARS = 12000; const transcriptText = words.map(w => w.text).join(" "); const promptTranscript = transcriptText.length > MAX_CHARS ? `${transcriptText.slice(0, MAX_CHARS)}…` : transcriptText;Then use promptTranscript in userMessage.
Also applies to: 80-86
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/desktop/src/components/editor-area/annotation-box.tsx(1 hunks)apps/desktop/src/components/editor-area/index.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/components/editor-area/index.tsx
🧰 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/annotation-box.tsx
🪛 Biome (2.1.2)
apps/desktop/src/components/editor-area/annotation-box.tsx
[error] 311-311: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
🪛 ast-grep (0.38.6)
apps/desktop/src/components/editor-area/annotation-box.tsx
[warning] 310-310: Usage of dangerouslySetInnerHTML detected. This bypasses React's built-in XSS protection. Always sanitize HTML content using libraries like DOMPurify before injecting it into the DOM to prevent XSS attacks.
Context: dangerouslySetInnerHTML
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://reactjs.org/docs/dom-elements.html#dangerouslysetinnerhtml
- https://cwe.mitre.org/data/definitions/79.html
(react-unsafe-html-injection)
⏰ 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 (1)
apps/desktop/src/components/editor-area/annotation-box.tsx (1)
107-116: Guideline check: "No error handling" policy for ts/tsxCurrent code displays toasts and logs in onError paths. This appears to conflict with the repo guideline “No error handling” for ts/tsx.
Please confirm whether UI toasts/logging are permitted here. If not, remove the onError hooks and rely on upstream error boundaries/state (e.g., render a neutral fallback when streamedContent is empty). I can provide a diff once confirmed.
Also applies to: 144-157
1 participant
Summary by cubic
Added a text selection popover in enhanced notes that lets users analyze the source of selected text in the transcript.