track number of terminal commands run #2519
Conversation
WalkthroughThis change introduces terminal command tracking functionality across the Electron application and backend telemetry system. A module-level counter is added to the main process, incremented via IPC when commands execute in the terminal view. The counter is periodically read and reset during activity logging, then propagated through TypeScript type definitions and Go structs to enable terminal command metrics in the telemetry pipeline. Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Areas requiring extra attention:
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: unknown linters: 'unusedfunc,unusedparams', run 'golangci-lint help linters' to see the list of supported linters 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
emain/emain.ts (1)
177-189: Terminal command tracking implementation looks correct.The counter is read once and conditionally included in both the activity update and event properties when non-zero, which aligns with the telemetry design.
The condition
termCmdCount > 0is checked twice (lines 178, 187). You could reduce redundancy by restructuring:const termCmdCount = getAndClearTermCommandsRun(); if (termCmdCount > 0) { activity.termcommandsrun = termCmdCount; + props["activity:termcommandsrun"] = termCmdCount; } const props: TEventProps = { "activity:activeminutes": activity.activeminutes, "activity:fgminutes": activity.fgminutes, "activity:openminutes": activity.openminutes, }; -if (termCmdCount > 0) { - props["activity:termcommandsrun"] = termCmdCount; -}Note:
propswould need to be declared before the check.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
emain/emain-activity.ts(2 hunks)emain/emain-ipc.ts(2 hunks)emain/emain.ts(2 hunks)emain/preload.ts(2 hunks)frontend/app/view/term/termwrap.ts(2 hunks)frontend/types/custom.d.ts(1 hunks)frontend/types/gotypes.d.ts(2 hunks)pkg/telemetry/telemetry.go(1 hunks)pkg/telemetry/telemetrydata/telemetrydata.go(1 hunks)pkg/wshrpc/wshrpctypes.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
emain/emain-ipc.ts (1)
emain/emain-activity.ts (1)
incrementTermCommandsRun(57-59)
emain/emain.ts (2)
emain/emain-activity.ts (1)
getAndClearTermCommandsRun(61-65)pkg/telemetry/telemetrydata/telemetrydata.go (1)
TEventProps(80-143)
frontend/app/view/term/termwrap.ts (1)
frontend/app/store/global.ts (1)
getApi(826-826)
⏰ 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: Build for TestDriver.ai
- GitHub Check: Analyze (go)
🔇 Additional comments (15)
frontend/types/custom.d.ts (1)
125-125: LGTM! API surface properly extended.The new
incrementTermCommandsmethod follows the existing pattern for fire-and-forget IPC calls and correctly complements the preload bridge exposure.emain/emain-ipc.ts (2)
21-21: LGTM! Import added correctly.The import of
incrementTermCommandsRunfromemain-activityis properly placed and used in the IPC handler below.
399-401: LGTM! IPC handler correctly implemented.The fire-and-forget pattern is appropriate for telemetry counter increments. No return value or error handling is needed for this use case.
pkg/wshrpc/wshrpctypes.go (1)
843-843: LGTM! ActivityUpdate field added correctly.The new
TermCommandsRunfield is properly positioned among other activity metrics and follows the established naming and tagging conventions.emain/preload.ts (2)
6-6: Good practice: helpful reminder comment.The comment serves as a useful reminder to keep the ElectronApi type definition in sync when modifying the exposed API surface.
64-64: LGTM! IPC bridge correctly exposed.The
incrementTermCommandsmethod is properly exposed viacontextBridge, usingipcRenderer.sendfor fire-and-forget semantics, which is appropriate for telemetry.frontend/app/view/term/termwrap.ts (2)
8-8: LGTM! Import added correctly.The
getApiimport is properly added to support the terminal command tracking functionality.
255-255: LGTM! Terminal command tracking correctly implemented.The call to
getApi().incrementTermCommands()is placed at the right location—in the OSC 16162 "C" command handler when a terminal command starts. The fire-and-forget pattern is appropriate for telemetry.frontend/types/gotypes.d.ts (2)
40-40: LGTM! ActivityUpdate type extended correctly.The
termcommandsrunfield is properly added as an optional number, matching the Go struct definition and following the established pattern for activity metrics.
1065-1065: LGTM! TEventProps type extended correctly.The
"activity:termcommandsrun"field is properly added to the telemetry event properties, matching the Go telemetry data structure.pkg/telemetry/telemetrydata/telemetrydata.go (1)
88-88: LGTM! Telemetry event property added correctly.The
TermCommandsRunfield is properly defined inTEventPropswith the correct JSON tag, matching the TypeScript type definitions and positioned appropriately among other activity metrics.pkg/telemetry/telemetry.go (1)
156-156: LGTM! Activity accumulation correctly implemented.The
TermCommandsRuncounter is properly accumulated in themergeActivityfunction, following the same pattern as other activity metrics. The placement ensures the terminal commands count is aggregated correctly during activity merges.emain/emain.ts (1)
15-15: LGTM!The import of
getAndClearTermCommandsRunis correctly placed with other activity-related imports.emain/emain-activity.ts (2)
11-11: LGTM!The module-level counter initialization is appropriate for tracking terminal commands across the application lifecycle.
57-65: LGTM! Clean implementation of the counter pattern.The increment and read-and-reset functions follow a standard metrics collection pattern. The read-and-reset approach in
getAndClearTermCommandsRun()is appropriate for periodic activity logging (called every 60s fromlogActiveState), ensuring the counter doesn't grow unbounded and that each period captures discrete command counts.No concurrency concerns since the Electron main process runs on a single-threaded event loop.
No description provided.