Conversation
EhabY
force-pushed
the
feat/speedtest-visualization-888
branch
from
b4cb6cc to
a37b1ac
Compare
EhabY
force-pushed
the
feat/speedtest-visualization-888
branch
5 times, most recently
from
781bd7a to
8c6204a
Compare
EhabY
force-pushed
the
feat/speedtest-visualization-888
branch
5 times, most recently
from
83a1ca1 to
1ce0401
Compare
EhabY
force-pushed
the
feat/speedtest-visualization-888
branch
5 times, most recently
from
04e4c55 to
13ee0b6
Compare
|
/coder-agents-review |
![coder-agents-review[bot]](https://avatars.githubusercontent.com/in/3454270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Well-structured PR. The separation into @repo/shared, vanilla-JS speedtest webview, and typed IPC protocol is clean. The canvas chart avoids the React framework overhead, chartUtils.ts is well-extracted for testability, and the disposal/cleanup chains are thorough. Zoro nailed it: "Vanilla JS webview is the correct choice; avoids ~200KB of React/framework deps for a static canvas chart."
3 P2, 9 P3, 4 Nit, 6 Note.
The P2s are: (1) a division-by-zero when all samples have x=0, producing a silently blank chart, (2) Zod validation errors surfaced raw as a JSON array in the user dialog, and (3) canvas draw errors inside requestAnimationFrame escaping the top-level try/catch with no user feedback. The P3s cluster around the gradient hex-color assumption (7 reviewers independently flagged it), stale canvasRect on scroll, and missing error context in the message handler.
Netero found 2 P3 coverage gaps (reportElapsedProgress and parseSpeedtestResult untested) and a spurious blank line. Those travel with the panel findings below.
Process note: the PR description says "createBaseWebviewConfig split into createWebviewConfig and createReactWebviewConfig." The base code had createWebviewConfig (no "Base" prefix). Minor but worth correcting for review accuracy.
src/webviews/util.ts:35
P3 [DEREM-15] <title>${title}</title> interpolates without escaping. The title comes from Speed Test: ${workspaceName} where workspaceName is user-derived. A workspace name containing </title> breaks the HTML structure. CSP prevents script execution, but HTML injection can still cause rendering artifacts. Escape <, >, & in the title.
(Pariston P3, Chopper P4)
🤖
🤖 This review was automatically generated with Coder Agents.
EhabY
force-pushed
the
feat/speedtest-visualization-888
branch
from
13ee0b6 to
9fbd977
Compare
After running a speed test, results are now displayed in a lightweight Canvas-based chart (4.5KB JS) instead of raw JSON. The webview shows throughput over time with hover tooltips, a summary header, and a "View JSON" button for the raw data. - Add `packages/speedtest/` webview (vanilla TS, no framework) - Extract `createBaseWebviewConfig` from the React-specific variant so lightweight webviews can reuse the shared Vite config - Add typed IPC via `SpeedtestApi` in `@repo/shared` - Accept duration as seconds, show real-time progress bar - Expose `extensionUri` through `ServiceContainer`
- Time-proportional x-axis with dashed leader line from t=0 - Uniform tick labels with smart unit selection (s/m/h) that scale from seconds to hours - Dynamic y-axis padding based on measured label width - Binary search hit-test with crosshair-snap for dense data - ResizeObserver debounced via requestAnimationFrame - Tooltip clamped to container bounds - General cleanup: named constants, single-pass data prep, responsive layout, safer error handling, input validation, missing dependency
- Plumb workspaceName from the command through to the panel title and the in-chart heading; drop the generic "Speed Test Results" label. - Read chart accent from the theme's button color so it tracks the active theme instead of being stuck on blue. - Re-send data on active-theme change so canvas pixels repaint against the new theme (DOM CSS vars update live, canvas doesn't). - Restructure chart.ts into readTheme/layoutChart/drawAxes/drawSeries, export niceStep/formatTick for testing. - Rename the panel viewType to coder.speedtestPanel to match the tasks/chat conventions. - Clean up the duration prompt and tests.
Validate CLI output with Zod on the extension side so the webview trusts typed data and stops hand-parsing JSON. Share SpeedtestResult types via @repo/shared and flatten the message payload. Webview cleanup: newspaper-ordered index.ts with a main() entrypoint, em-scaled chart layout, named constants in place of magic numbers, empty samples handled with a message, and a subscribeNotification helper that useIpc now delegates to. Pure helpers (niceStep, formatTick, findNearest*, toChartSamples) live in chartUtils.ts for easy unit testing. Panel: extract webview panel logic into SpeedtestPanelFactory, a ServiceContainer-owned class that takes extensionUri + logger in its constructor and exposes show(payload). Surfaces webview handler errors through the logger and tracks every subscription in a disposables array. Drops the now-unused isGoDuration helper. Tests: cover SpeedtestPanelFactory end-to-end with a reusable createMockWebviewPanel harness in testHelpers, plus chartUtils and renderLineChart unit tests. Canvas 2D is stubbed globally in test/webview/setup.ts so chart tests don't need per-test plumbing. Protocol: buildApiHook and useIpc now take RequestDef, CommandDef, and NotificationDef directly in both overloads, no casts needed. Platform: move toError into shared with a serialize hook so the extension keeps util.inspect output, rename createBaseWebviewConfig to createWebviewConfig, add createReactWebviewConfig, extract reportElapsedProgress for reuse by other long running commands, and drop the createWebviewConfig eslint ignore.
EhabY
force-pushed
the
feat/speedtest-visualization-888
branch
from
9fbd977 to
bfb8ce3
Compare
EhabY
force-pushed
the
feat/speedtest-visualization-888
branch
from
4886505 to
9003aa1
Compare
2 participants
Renders speed test results in a Canvas line chart with throughput over time, a summary header, hover tooltips, and a View JSON action. Duration input drives a real-time progress bar while the CLI runs.
Extension
SpeedtestResult/SpeedtestData/SpeedtestIntervalin@repo/sharedSpeedtestPanelFactoryowned byServiceContainerencapsulates panel creation, message handling, and error surfacing via the loggerreportElapsedProgressextracted intoprogress.tsfor reuse by other long-running commandsWebview
chart.tsdoes canvas drawing; pure helpers (niceStep,formatTick,findNearest*,toChartSamples) live inchartUtils.tsfor direct unit testingsubscribeNotificationhelper in@repo/webview-shared;useIpcdelegates to itProtocol
buildApiHookanduseIpctakeRequestDef/CommandDef/NotificationDefdirectly in both overloads, no castsShared
toErrormoved into@repo/sharedwith a serialize hook so the extension keepsutil.inspectformattingcreateBaseWebviewConfigsplit intocreateWebviewConfigandcreateReactWebviewConfigCloses #888