Conversation
WalkthroughThis pull request introduces telemetry enhancements, debug configuration infrastructure, and directory search functionality across backend and frontend. On the backend, startup telemetry now captures cohort information (month and ISO week) derived from TosAgreedTs or current time, and the pprof server startup is refactored from environment-variable-based to configuration-based with improved error handling. On the frontend, a new directory search feature adds keyboard-driven search activation (Cmd+F) with state management, and type definitions are expanded to support debug settings and cohort telemetry fields. Supporting schema and configuration constants are added across the Go and TypeScript layers. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (2 inconclusive)
✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/app/view/term/term-model.ts (1)
532-537: Verify the keyDownHandler invocation flow.The terminal now calls
appHandleKeyDown(waveEvent)after processing terminal-specific keys. However,appHandleKeyDowndispatches back to the focused block'sviewModel.keyDownHandler(see frontend/app/store/keymodel.ts lines 437-446), which was already invoked directly on line 492.This creates a flow where
this.keyDownHandleris called twice for the same event:
- Direct call on line 492
- Indirect call via
appHandleKeyDown→ global dispatcher →viewModel.keyDownHandlerWhile the terminal's
keyDownHandler(lines 467-485) only handles Cmd:Escape and vdom keys, this double invocation is inefficient. Please verify:
- This pattern is intentional and doesn't cause unexpected side effects
- The app-level global keys (Cmd+N, Cmd+W, etc.) are properly handled without conflicts
The intent appears to be allowing global keys to work even when the terminal is focused, which is good. Consider whether
appHandleKeyDownshould skip dispatching back to the calling view model's handler to avoid the redundant call, or document this behavior if intentional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
cmd/server/main-server.go(5 hunks)frontend/app/store/keymodel.ts(2 hunks)frontend/app/view/preview/preview-directory.tsx(6 hunks)frontend/app/view/preview/preview-model.tsx(2 hunks)frontend/app/view/term/term-model.ts(2 hunks)frontend/types/gotypes.d.ts(3 hunks)pkg/telemetry/telemetry.go(1 hunks)pkg/telemetry/telemetrydata/telemetrydata.go(1 hunks)pkg/wconfig/metaconsts.go(1 hunks)pkg/wconfig/settingsconfig.go(4 hunks)schema/settings.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
frontend/app/store/keymodel.ts (2)
pkg/waveobj/wtype.go (2)
Block(282-290)Block(292-294)frontend/layout/lib/layoutModelHooks.ts (1)
getLayoutModelForStaticTab(45-48)
cmd/server/main-server.go (4)
pkg/telemetry/telemetrydata/telemetrydata.go (1)
TEventUserProps(55-78)pkg/wavebase/wavebase.go (1)
WaveVersion(25-25)pkg/telemetry/telemetry.go (1)
GetTosAgreedTs(33-48)pkg/wconfig/filewatcher.go (1)
GetWatcher(33-57)
⏰ 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). (3)
- GitHub Check: Build for TestDriver.ai
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (5)
frontend/app/store/keymodel.ts (1)
155-215: LGTM! Whitespace formatting improvements.The changes in this file are whitespace-only formatting adjustments around the
uxCloseBlockandgenericClosefunctions. The functional behavior remains unchanged, and the existing export ofappHandleKeyDownenables the integration in term-model.ts.frontend/app/view/preview/preview-model.tsx (1)
163-177: LGTM! Clean state management addition.The new
directorySearchActiveatom is properly declared and initialized, providing a clear mechanism to track directory search state. The implementation follows the established jotai pattern used throughout the codebase.frontend/app/view/preview/preview-directory.tsx (2)
452-471: LGTM! Search banner implementation is well-designed.The search banner now:
- Displays when search is activated (via Cmd+F) even before typing, providing immediate user feedback
- Shows contextual messaging: "Type to search (Esc to cancel)" or current search query
- Includes
text-blackfor proper contrast on thebg-warningbackground- Properly resets both search text and activation state when cleared
The conditional rendering
(searchActive || search !== "")ensures the banner appears as soon as the user activates search mode, which is good UX.
557-557: LGTM! Keyboard shortcuts properly integrated.The directory search activation and deactivation logic is well-implemented:
Activation (lines 659-662):
- Cmd+F activates search mode by setting
directorySearchActiveto true- Integrates correctly with the global key handling in keymodel.ts: the global Cmd+F handler checks for
searchAtoms, returns false for PreviewModel, allowing the event to dispatch to this view-specific handlerDeactivation:
- Escape key (line 665): Explicitly cancels search
- Enter key (line 690): Deactivates after navigation
- Double-click (line 557): Deactivates after navigation
The search state is properly cleared on all navigation actions, preventing stale search state when browsing directories.
Also applies to: 659-662, 665-665, 690-690
pkg/telemetry/telemetry.go (1)
244-264: The review comment is incorrect—epoch units are consistent.The code correctly uses millisecond timestamps throughout:
waveobj.Client.TosAgreedis explicitly documented as// unix milli(line 132 in wtype.go)- The value is written using
time.Now().UnixMilli()(line 60 in clientservice.go)GetTosAgreedTs()returns it without conversion (telemetry.go)- The 24-hour comparison in line 244 correctly treats it as milliseconds
There is no epoch unit mismatch, no data corruption risk, and no conversion needed. The code is safe as-is.
Likely an incorrect or invalid review comment.
No description provided.