feat(charts/vscode): Added charting capabilities to kilo-ui storybook and VS Code extension - #12525
Conversation
Code Review SummaryStatus: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Notes
Files Reviewed (incremental diff, 1 file)
Fix these issues in Kilo Cloud Previous Review Summaries (6 snapshots, latest commit 4623b03)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 4623b03)Status: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Resolved since last review
Notes
Files Reviewed (incremental diff, 1 file)
Fix these issues in Kilo Cloud Previous review (commit c94d49c)Status: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Notes
Files Reviewed (incremental diff, 1 file)
Fix these issues in Kilo Cloud Previous review (commit 100f8c6)Status: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Notes
Files Reviewed (incremental diff, 1 file)
The rest of the diff since the last review is a Fix these issues in Kilo Cloud Previous review (commit edc6976)Status: No Issues Found | Recommendation: Merge All previously flagged issues have been resolved in this update:
The Files Reviewed (incremental diff, 10 files)
Previous review (commit 53179eb)Status: No Issues Found | Recommendation: Merge All previously flagged issues have been resolved in this update:
The Files Reviewed (incremental diff, 10 files)
Previous review (commit 7f1d780)Status: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (16 files)
Reviewed by claude-opus-5 · Input: 34 · Output: 8.7K · Cached: 822K Review guidance: REVIEW.md from base branch |
…pt; Add unit tests for vs code only feature.
|
Addressing comments and will be pushing updates soon. |
| @@ -0,0 +1,137 @@ | |||
| /** @jsxImportSource solid-js */ | |||
| import { createEffect, createSignal, onCleanup } from "solid-js" | |||
| import { Chart, registerables } from "chart.js" | |||
There was a problem hiding this comment.
SUGGESTION: the static import puts all of chart.js in the main webview chunk
message-part.tsx registers ChartTool via a static import, so hoisting chart.js to a top-level import means the whole library (~200KB minified) is now loaded for every chat session, even when no chart is ever rendered. The previous dynamic import("chart.js") kept it in its own chunk.
If the eager cost is acceptable, this is fine as-is. If you'd rather keep the split chunk, a module-level memoized loader gives you both (no repeated register, no lost reactive owner):
const load = import("chart.js").then((m) => (m.Chart.register(...m.registerables), m.Chart))and then register the cleanup synchronously in the effect while creating the instance in the .then().
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
@marius-kilocode this is why I had it inline before, but I don't know if you have strong feelings about it. This could make it load faster initially but also means it could be loaded but never needed.
Thank you. Would you please share your profiling steps? I have addressed the lifecycle code, so the only outstanding update would be addressing applying theme colors for accessibility. |
|
@cosi-conda you can profile with https://github.com/Kilo-Org/vscode-extension-self-test if you like. |
Thank you for that and also the review. I am just learning more about kilo and solidjs, although I have a lot of TS, React and Angular experience. Hopefully my next PR will be faster. :) |
feat(charts/vscode): Added charting capabilities to kilo-ui storybook and VS Code extension
Context
Follow up features
Implementation
Various charting libraries were considered, but this was ultimately implemented with chart.js for the following reasons:
Screenshots / Video
How to Test
Manual/local verification
bun run extensionChecklist