Repository navigation
Add Performance section to the Dashboard - #1
Conversation
cckiro
left a comment
There was a problem hiding this comment.
Review — PR #1: Performance Analysis
Thanks for the contribution @nataizya! The concept is solid — configuration health analysis with actionable recommendations is genuinely useful. The code follows the project's handler/service pattern correctly and integrates cleanly with the frontend stack.
However, there are several issues that need to be addressed before this can be merged.
🔴 Must Fix
1. Use strconv.Itoa() instead of custom itoa() (service.go lines 720-762)
There's no reason to hand-roll integer-to-string conversion when the stdlib provides strconv.Itoa(). Please replace all itoa() calls with strconv.Itoa() and remove the custom function.
2. Path validation missing — security requirement (fixes.go, sessions.go, stats.go)
All user-derived file paths must go through utils.ValidatePath() before any read/write operation. This is a hard security requirement for this project. Specifically:
fixRemoveDisabledServers()— validateconfigPathbefore read/writeAnalyzeSessions()— validatesessionsPathgetAgentFileID()— validate constructed pathscomputeAgentBudgets()— validateagentsPathcomputeUnusedTools()— validateagentsPath
3. fixRemoveDisabledServers() bypasses the MCP service layer (fixes.go)
This function directly reads/writes mcp.json with os.ReadFile/os.WriteFile. All MCP config mutations should go through mcpService to maintain consistency and avoid race conditions. Please refactor to use the existing service methods.
4. Handler test is not meaningful (handler_test.go)
TestGetScoreHandler_ReturnsJSON passes a nil service and catches the resulting panic with defer recover(). This doesn't test anything useful. Please write tests that:
- Use
t.TempDir()for the config path (never hit real filesystem) - Test the actual HTTP response status and body
- Cover error cases (invalid JSON body, missing ID, agent not found)
5. TestSessionAnalysis hits real filesystem (service_test.go)
This test reads ~/.kiro/sessions/ on whatever machine runs it. Per project standards:
- Tests must use
t.TempDir()for all file operations - Tests must be reproducible — no dependency on real user data
- Create synthetic
.jsonltest fixtures in the temp dir
6. Errors swallowed in Analyze() (service.go)
agents, _ := s.agentService.GetAllAgents()
mcpServers, _ := s.mcpService.GetAllMCPServers()If these fail, the analysis proceeds with nil data which could cause panics downstream. At minimum, log the error with s.logger.Error(). Ideally, return a partial report with a warning.
7. Dead code (fixes.go, api.js)
Per project standards: "No dead code — every function must be called, every file must have a purpose."
findAgentByName()infixes.gois defined but never called — remove itgetPerformanceScoreinapi.jsis exported but never imported anywhere — remove it
🟡 Should Fix
8. containsError() is too broad (sessions.go)
Matching "error" anywhere in tool result text will flag false positives like "0 errors found" or "error_handling_enabled". Consider checking for error indicators more precisely — e.g., checking the tool result's isError field if present, or requiring the text to start with an error pattern.
9. No session count limit (sessions.go)
AnalyzeSessions() reads every .jsonl file in the sessions directory with no cap. On machines with hundreds of sessions, this could be very slow. Consider limiting to the most recent N sessions (e.g., 50) or adding a size/time cutoff.
10. Single 477-line component (Performance.jsx)
The sub-components (StatCard, ScoreRing, ToolUsageSection, ContextBudgetSection, UnusedToolsSection, RecommendationItem, RecommendationsSection) should be extracted into separate files under src/pages/Performance/. Other pages in this project keep their page file focused on layout and state, with sub-components in their own files.
11. Test coverage is thin
Current tests cover:
itoa()(which should be removed anyway)calculateScore()✅ good- JSON round-trip of
Recommendationstruct ✅ okay - Nil-service panic catch ❌ useless
Missing test coverage:
checkAgentToolCount()with agents at various tool countscheckDuplicateTools()with actual duplicate entriesApplyFix()for each fix type (using temp dirs with fixture data)FixAll()end-to-end- Handler endpoints with proper HTTP request/response assertions
- Edge cases: empty config dir, malformed JSON files, missing directories
✅ What's Good
- Follows handler/service pattern correctly
- Constructor signatures match project conventions
- Wired into
main.goproperly (service → handler → setupRouter) - Godoc on all exported types and functions
- Package-level comment present
- API calls go through
api.jsaxios instance (not raw fetch) - Uses
useNotification,useWorkspace,react-query— reuses existing infrastructure - Route added in
App.jsx, menu item inLayout.jsxwith icon/color/path/description @fileoverviewJSDoc header on the React component- No
console.log, no commented-out code - No new dependencies added (Go or npm)
filepath.Join()used consistently for path construction- Uses
kiroService.GetConfigPath()— no hardcoded user paths
💡 Suggestion
Per your note in the PR description — yes, splitting this into 2-3 smaller PRs would make review much easier:
- Backend analysis engine (
service.go,sessions.go,stats.go,handler.go+ proper tests) - Auto-fix functionality (
fixes.go+ tests) - Frontend page + wiring
That said, if you'd prefer to address the above in this PR, that works too.
Must-fix items: - Replace custom itoa() with strconv.Itoa() in service.go - Add utils.ValidatePath() to user-derived paths in fixes.go, sessions.go, stats.go (fixRemoveDisabledServers, AnalyzeSessions, computeAgentBudgets, computeUnusedTools, getAgentFileID) - Refactor fixRemoveDisabledServers() to use mcpService.GetMCPConfig and SaveMCPConfig instead of raw os.ReadFile/os.WriteFile - Rewrite handler_test.go with real HTTP tests (GetScore, GetAnalysis, GetRecommendations, ApplyFix incl. error cases, FixAll) using t.TempDir - Rewrite TestSessionAnalysis to use t.TempDir + synthetic .jsonl fixtures instead of hitting real ~/.kiro/sessions/ - Log errors from GetAllAgents() and GetAllMCPServers() in Analyze() via s.logger.Error(); continue with partial data per review intent - Remove dead code: findAgentByName() in fixes.go and getPerformanceScore export in api.js Should-fix items: - Tighten containsError() in sessions.go: check JSONL isError field first, then strict prefix patterns (Error:, error:, failed to, etc.) instead of substring match for 'error' - Cap AnalyzeSessions() to MaxSessionFiles (50) most recent .jsonl files sorted by mtime desc to bound I/O on busy machines - Extract Performance.jsx (was 477 LOC) into 7 sub-components under client/src/pages/Performance/: StatCard, ScoreRing, ToolUsageSection, ContextBudgetSection, UnusedToolsSection, RecommendationItem, RecommendationsSection - Expand test coverage: TestCheckAgentToolCount, TestCheckDuplicateTools, new fixes_test.go (TestApplyFix_DuplicateTools, _SetModel, _RemoveDisabledServers, _UnknownID, TestFixAll) Verified: go test ./internal/... passes, go vet clean, go build and vite build succeed, api.js compliance grep returns zero matches.
Fold plan usage, session-derived counts, and model routing share
into a 'Performance' section on the Dashboard tab.
Backend (internal/performance/):
- Streams ~/.kiro/sessions/**/*.jsonl line by line to aggregate
prompt / assistant-message / tool-call counts and per-model share.
- Continuation AssistantMessages inherit the last-seen modelId within
a session so tool-use follow-ups are credited to the starting model.
- Shells out to 'kiro-cli chat --no-interactive "/usage"' for the
plan-and-credits card; parses the human output, caches 60s in
memory, fails gracefully when kiro-cli is unauthenticated
(KIRO_USAGE_UNAVAILABLE 502).
- Endpoints: GET /api/performance/{summary,recent,kiro-usage}.
- 15 tests covering session parsing, model carryforward, window
filtering, ANSI stripping, and /usage output for enterprise / free /
derived-percent cases.
Frontend (client/src/components/Performance/):
- PerformanceSection composed of PlanUsageCard, SummaryCards, and
ModelUsageChart (recharts horizontal BarChart). Time-range toggle
(24h/7d/30d/all) persisted to localStorage. React Query with 30s /
5min refetch intervals; usage query has retry:false so a broken
kiro-cli auth is not retried on every poll.
- Wired into Dashboard.jsx under a new 'Performance' heading matching
the existing 'Details' section pattern; no standalone route or nav
entry.
Follows the existing internal/steering/ and internal/skills/ package
layout and the pages/Dashboard integration pattern.
e179c6c to
e4ba8ed
Compare
Plan usage now fetches live from kiro-cli on request, with clear guidance to log in when it is unavailable. A single Refresh in the section header reloads both the session summary and plan usage, and the header shows when the data was last updated, so nothing polls in the background. - Remove the in-memory and on-disk usage caches in favour of live reads - Add a Refresh control and last-updated time to the section header - Share the rainbow palette via colors.js for the chart, usage bar, spinner - Give every model a distinct colour in the model usage chart - Move the Performance heading inline with the section controls
Regenerate client/package-lock.json to pick up security fixes across axios, js-yaml, brace-expansion, postcss, form-data, @babel/core, browserslist, nanoid, and the vite build toolchain. All updates fall within the existing semver ranges, so client/package.json is unchanged. Supersedes dependency bumps kirodotdev-labs#6, kirodotdev-labs#7, kirodotdev-labs#9, kirodotdev-labs#10, kirodotdev-labs#11, kirodotdev-labs#12, kirodotdev-labs#13.
|
Merged, and shipped in v1.2.0. Thank you, @nataizya — this is a genuinely useful addition and the quality of the contribution made it easy to take. A few things I appreciated while reviewing: the handler/service split is exactly right, the session-log parsing is careful about partial and missing data instead of assuming a shape, the I made some polish changes on top of your commits before merging, all under this PR:
One thing worth knowing that came out of testing: Also folded in a dependency refresh that picked up security fixes across axios, js-yaml, postcss and the Vite toolchain, which superseded #6, #7, #9, #10, #11, #12 and #13. Thanks again — the release notes credit you for the feature. Happy to have more contributions from you any time. |
Summary
Adds a Performance section to the Dashboard that shows how you are using Kiro CLI: your plan and credit position, activity totals from local session logs, and which models your responses came from.
What's new
Backend —
internal/performance/service.go— reads Kiro CLI session logs from~/.kiro/sessionsand aggregates sessions, user prompts, tool calls, first/last activity, and per-model response counts for a selected time window (24h / 7d / 30d / all time).kiro_cli.go— fetches plan and credit usage by runningkiro-cli chat --no-interactive "/usage"and parsing the result.handler.go— thin HTTP handlers wired intomain.go.service_test.go,handler_test.go,kiro_cli_test.go.Frontend —
client/src/components/Performance/PerformanceSection.jsx— renders inside the Dashboard, with a time-range selector, a last-updated time, and a single Refresh control.PlanUsageCard.jsx— plan, credits used against the allowance, and reset date. If usage cannot be read it explains how to fix it (runkiro-cli login) instead of failing silently.SummaryCards.jsx— activity totals for the selected window.ModelUsageChart.jsx— responses per model, each model in its own colour.colors.js— shared palette so the chart, usage bar, and spinner match the rest of the Dashboard.client/src/services/api.js:getPerformanceSummary,getPerformanceRecent,getKiroUsage.Behaviour
Nothing polls in the background. Data loads when you open the Dashboard and when you change the time range, and Refresh reloads both the activity summary and plan usage on request. The header shows when the data was last updated. Plan usage is read live each time it is requested, and a failure there is contained to its own card so the rest of the section still works.
Plan usage and session activity are account-wide, so this section is not workspace-specific.
Also included
client/package-lock.jsonregenerated to pick up security fixes for axios, js-yaml, brace-expansion, postcss, form-data,@babel/core, browserslist, nanoid, and the vite build toolchain. All within existing semver ranges, soclient/package.jsonis unchanged. This supersedes Bump esbuild, @vitejs/plugin-react and vite in /client #6, Bump form-data from 4.0.5 to 4.0.6 in /client #7, Bump @babel/core from 7.28.6 to 7.29.7 in /client #9, Bump axios from 1.16.0 to 1.18.0 in /client #10, Bump postcss from 8.5.12 to 8.5.25 in /client #11, Bump brace-expansion from 1.1.13 to 1.1.18 in /client #12 and Bump js-yaml from 4.1.1 to 4.3.1 in /client #13.Testing
make test— all Go packages pass.make build— frontend and Go binary build clean.