refactor(desktop): downgrade terminal.renderer routine logs to breadcrumb-only#395
Merged
refactor(desktop): downgrade terminal.renderer routine logs to breadcrumb-only#395
Conversation
…rumb-only Normal terminal lifecycle events (mount, webgl-load, cache-attach, runtime-attach, flush-pending-events, aggregate metrics) now only add breadcrumbs instead of creating Sentry Issues. This cuts Sentry quota usage by ~90% for active terminal sessions while preserving full debug context — breadcrumbs are still attached to any error that occurs. Error/warning events (webgl-context-lost, terminal-write-mutate-failed, ws-reconnect-scheduled) remain as captureMessage. A new `captureMessageByDefault` option on DebugChannel controls aggregate flush behavior per-channel, defaulting to true for backward compatibility. 💘 Generated with Crush Assisted-by: GLM-5 via Crush <crush@charm.land>
📝 WalkthroughWalkthroughデバッグチャネル設定に Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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.
🧹 Nitpick comments (1)
apps/desktop/src/shared/debug-channel.ts (1)
36-36: LGTM — 後方互換を保った既定値スイッチ
captureMessageByDefault?: boolean(既定true)の追加と、observe()でのoptions?.captureMessage ?? this.captureMessageByDefaultによる per-call オーバーライド優先は適切な設計です。既存のチャンネル/呼び出し側に影響を与えずに、terminal.rendererのような新規チャンネルだけを breadcrumb 既定に切り替えられます。1点だけ補足として、
flushAll()(Line 267)はチャンネル既定値のみを使用し、observe()呼び出しごとに指定されたoptions.captureMessageオーバーライドは握りつぶします(オーバーライドはタイマーのクロージャ内にしか保持されていないため)。この挙動は本 PR 以前から同等(以前はハードコードtrue)なので回帰ではありませんが、将来 per-aggregate に override を尊重させたい場合はAggregateStateにcaptureMessageフィールドを持たせる小リファクタが候補です。現状の PR スコープでは対応不要と考えます。Also applies to: 129-129, 138-138, 258-258, 267-267
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/shared/debug-channel.ts` at line 36, The current implementation adds captureMessageByDefault and lets observe() prefer options?.captureMessage ?? this.captureMessageByDefault, but flushAll() (and other aggregate-level logic) only uses the channel default and thus ignores per-observe overrides because those overrides live only in the timer closure; to fix this, add a captureMessage boolean field to the AggregateState (or equivalent aggregate object used by observe()/flushAll()), set that field from the per-call resolved value inside observe() when creating/updating the aggregate, and update flushAll() and any aggregate-level code to read aggregate.captureMessage instead of the channel default so per-observe overrides are honored across timer/flush boundaries.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/desktop/src/shared/debug-channel.ts`:
- Line 36: The current implementation adds captureMessageByDefault and lets
observe() prefer options?.captureMessage ?? this.captureMessageByDefault, but
flushAll() (and other aggregate-level logic) only uses the channel default and
thus ignores per-observe overrides because those overrides live only in the
timer closure; to fix this, add a captureMessage boolean field to the
AggregateState (or equivalent aggregate object used by observe()/flushAll()),
set that field from the per-call resolved value inside observe() when
creating/updating the aggregate, and update flushAll() and any aggregate-level
code to read aggregate.captureMessage instead of the channel default so
per-observe overrides are honored across timer/flush boundaries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 22c1d379-a896-4430-a442-303c5ef60dc5
📒 Files selected for processing (8)
apps/desktop/src/renderer/lib/terminal/debug.tsapps/desktop/src/renderer/lib/terminal/terminal-runtime-registry.tsapps/desktop/src/renderer/lib/terminal/terminal-runtime.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalLifecycle.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalRestore.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/v1-terminal-cache.tsapps/desktop/src/shared/debug-channel.ts
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
captureMessageから Breadcrumb-only に格下げcaptureMessageを維持DebugChannelにcaptureMessageByDefaultオプションを追加し、集計 flush のデフォルト挙動をチャンネル単位で制御可能に期待効果: アクティブなターミナルセッション中の Sentry Issue 生成量が ~90% 減少。Breadcrumb は全イベントを記録し続けるため、エラー発生時の調査力は変わらない。
変更分類
cache-attach-to-containerwebgl-addon-loadedwebgl-context-lostmountterminal-write-mutate-failedflush-pending-eventsws-reconnect-scheduledruntime-attach-to-containerruntime-attachTest plan
bun run lint— 通過bunx turbo typecheck --filter=@superset/desktop— 通過Full Changelog: main...feat/terminal-breadcrumb-only
💘 Generated with Crush
Assisted-by: GLM-5 via Crush crush@charm.land
Summary by CodeRabbit
リリースノート