⚡ Bolt: [Performance] Eliminate O(N) re-renders in ChatHistory via CSS group-hover & memoization#69
Conversation
…S group-hover & memoization - Refactored `ChatHistory` list to use Tailwind CSS `group-hover` utility classes instead of React state for hover transitions, eliminating O(N) re-renders on hover interactions. - Wrapped `ChatHistory` component with `React.memo` to prevent cascading updates from frequent parent re-renders. - Fixed a type error in `ext/src/providers/chat-view-provider.ts` where marked `.parse` returning a union was incorrectly called with `.catch()`.
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
Hey @iotserver24! 👋 I'll go through the changes and help you out with an automated review! 🔍 Starting the review now... |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR addresses O(N) re-render cascades in chat lists by documenting the pattern, wrapping ChangesChat List Performance Optimization
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Review rate limit: 9/10 reviews remaining, refill in 6 minutes. Comment |
There was a problem hiding this comment.
Pull request overview
Refactors UI hover behavior in the Desktop ChatHistory session list to reduce render churn by moving hover-driven UI toggles from React state to CSS (group-hover) and memoizing the component; also hardens VS Code extension history HTML parsing around marked.parse() return types.
Changes:
- Replaced
hoveredIdReact state with Tailwindgroup-hoverto toggle timestamp vs delete affordance inChatHistory. - Wrapped
ChatHistoryinReact.memo()to avoid unnecessary re-renders when props are stable. - Updated extension
getParsedHistory()to safely handle both sync/asyncmarked.parse()results. - Documented the performance learning in
.jules/bolt.md.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/ext/src/providers/chat-view-provider.ts | Makes history markdown parsing robust when marked.parse() is sync or async. |
| packages/desktop/src/renderer/components/ChatHistory.tsx | Removes hover state from list rendering and memoizes the component to reduce re-renders. |
| .jules/bolt.md | Records the “CSS hover over React hover state in lists” performance guideline. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <span | ||
| onClick={(e) => handleDelete(e, s.id)} | ||
| className="shrink-0 rounded p-1 text-xibe-text-dim/50 hover:text-xibe-error hover:bg-xibe-error/10 transition-colors animate-fade-in hidden group-hover:block" | ||
| title="Delete chat" | ||
| > | ||
| <Trash2 className="h-3.5 w-3.5" /> | ||
| </span> |
| const parsed = marked.parse(m.content); | ||
| html = parsed instanceof Promise ? await parsed : parsed; |
|
Xibe review this |
|
Hey @iotserver24! 👋 I'll go through the changes and help you out! 🔍 Starting the review now... |
🔍 Final Review for @iotserver24PR: ⚡ Bolt: [Performance] Eliminate O(N) re-renders in ChatHistory via CSS group-hover & memoization 🔴 Security Concerns (Top Priority)1. Cross-Site Scripting (XSS) via Unsanitized Markdown OutputFile: const parsed = marked.parse(m.content);
html = parsed instanceof Promise ? await parsed : parsed;Why it’s critical:
Fix: import { JSDOM } from 'jsdom';
import DOMPurify from 'dompurify';
const window = new JSDOM('').window;
const purify = DOMPurify(window);
try {
const parsed = marked.parse(m.content);
const rawHtml = await parsed;
html = purify.sanitize(rawHtml);
} catch (e) {
// fallback to escaped content as plain text?
html = m.content;
}2. Potential Stored XSS via Chat Description TitleFile: <span>{s.title}</span>Why it’s a concern:
<span>{String(s.title)}</span>Additionally, ensure the backend sanitizes or rejects malicious content in session titles. 🔵 User-Specific Analysis: “Xibe review this”Your PR focuses on performance – eliminating O(N) re-renders by moving hover state to CSS and using However, reviewing the whole PR reveals three categories of issues that need attention before merging:
The performance gain is real, but the accessibility regression is a breaking change for keyboard/screen-reader users. The XSS vulnerability is a hard blocker. 🛠️ Recommended Changes (Actionable Fixes)🔴 Critical: Add HTML Sanitization (chat-view-provider.ts)Replace the current 🟡 High Priority: Restore Keyboard-Accessible Delete Button (ChatHistory.tsx)Lines 108–110 – Change <button
type="button"
onClick={(e) => {
e.stopPropagation();
handleDelete(e, s.id);
}}
className="shrink-0 rounded p-1 text-xibe-text-dim/50 hover:text-xibe-error hover:bg-xibe-error/10 transition-colors animate-fade-in invisible group-hover:visible"
title="Delete chat"
aria-label={`Delete session ${s.title}`}
>
<Trash2 className="h-3.5 w-3.5" />
</button>Why the changes?
🟡 Medium Priority: Prevent Layout Shift (ChatHistory.tsx)Apply the same <span className="invisible group-hover:visible ml-auto text-xs text-xibe-text-dim/20 pointer-events-none">
{formatRelativeTime(s.updatedAt)}
</span>🔵 Low Priority: Code Cleanup
📊 Comprehensive Analysis✅ What’s Great
|
| Area | Current State | Impact | Fix Priority |
|---|---|---|---|
| Security | Unsanitized markdown → stored XSS | Critical – potential script injection | 🔴 Highest |
| Accessibility | Delete button not keyboard-accessible | Screen reader & keyboard users blocked | 🟡 High |
| User Experience | Layout shift on hover | Jarring visual reflow | 🟡 Medium |
| Prop drift risk | Memoized parent list with non-primitive props | May cause stale UI if props change but reference stays same | 🔵 Low |
| Code clarity | instanceof Promise and unused catch |
Minor lint noise | 🔵 Low |
🧪 Testing Recommendation
After applying fixes, re-run both tests and manual verification:
- Keyboard navigation – Tab to a chat item, then Tab to the delete button; press Enter/Space to delete. Verify no accidental navigation.
- Visual regression – Playwright screenshot comparison to ensure no layout shift on hover.
- Security test – Inject
<img src=x onerror=alert(1)>as a chat message title and confirm no dialog appears.
📎 Summary
| Metric | Value |
|---|---|
| Files Reviewed | 3 |
| Issues Found | 2 🔴 Critical, 3 🟡 Warnings, 2 🔵 Suggestions |
| Security Blocker | Yes – XSS in chat-view-provider.ts |
| Recommendation |
Your performance refactor is clean and valuable. Once the XSS sanitization and accessibility regression are addressed, this PR will be a model of modern React optimization. Great work on the core logic – just need to button up the edges.
🤖 Powered by Xibe AI
📊 Analysis: 5595 characters analyzed across 3 files
💙 Real-time Analytics • 📚 Documentation
💡 What
Refactored the
ChatHistoryReact component to drop the use ofhoveredIdvia ReactuseState. The interactive visibility of timestamps vs delete buttons is now exclusively managed via Tailwind CSS's nativegroup-hoverutility classes. The list component itself was also wrapped inReact.memo().🎯 Why
Prop-drilling or managing local interactive state (like
hoveredIdonmouseenter/mouseleave) within a list component mapping an array of items creates O(N) re-renders when interacting with any single item. By deferring visual changes on hover to native CSS rules, we entirely sidestep React's rendering lifecycle for this interaction.📊 Impact
Reduces list re-renders to O(1) on mount, entirely eliminating the O(N) overhead during user hover interactions or when the parent
Appis updating streaming state.🔬 Measurement
Run
pnpm testandpnpm lintto verify code validity. Visual verifications via Playwright assert the frontend styling remains completely unchanged.PR created automatically by Jules for task 14370465157369467408 started by @iotserver24
Summary by CodeRabbit
Performance
Bug Fixes
Documentation