Skip to content

Conversation

@sawka
Copy link
Member

@sawka sawka commented Dec 17, 2025

No description provided.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 17, 2025

Walkthrough

The change introduces telemetry tracking for SSH connections initiated via OSC 16162 command protocol. A single import of recordTEvent was added to termwrap.ts, and a telemetry event recording statement was inserted in the OSC 16162 case "C" handler. When a decoded command starts with "ssh ", the system now emits a telemetry event with name "conn:connect" and type "ssh-manual".

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~7 minutes

  • Verify that recordTEvent is properly exported from "@/store/global"
  • Confirm the event name "conn:connect" and type "ssh-manual" are correct and consistent with existing telemetry patterns elsewhere in the codebase
  • Ensure the string comparison (startsWith("ssh ")) accurately captures the intended command pattern
  • Check that this telemetry call has negligible performance impact in the OSC command handling path

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Description check ❓ Inconclusive No pull request description was provided by the author, making it impossible to assess relevance to the changeset. Add a description explaining the purpose of tracking SSH connection telemetry and how it improves remote connection statistics.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The title 'better remote connection stats' is related to the changeset but is somewhat vague. The actual change adds telemetry tracking for SSH manual connections, which supports better stats collection.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch sawka/r131-minor-2

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
frontend/app/view/term/termwrap.ts (1)

277-279: Telemetry logic looks good; consider edge cases as optional enhancements.

The telemetry tracking correctly identifies SSH commands in the common case and properly avoids logging sensitive information like hostnames or usernames. The implementation is straightforward and appropriate for its purpose.

For improved coverage, consider handling these edge cases:

  • Commands with leading whitespace (e.g., " ssh host")
  • Case variations (e.g., "SSH host")
  • Path-prefixed commands (e.g., "/usr/bin/ssh host")
  • Commands with prefixes (e.g., "sudo ssh host")

However, these are optional improvements—the current implementation handles the primary use case effectively.

Optional enhancement:

-                        if (decodedCmd?.startsWith("ssh ")) {
+                        const normalizedCmd = decodedCmd?.trim().toLowerCase();
+                        if (normalizedCmd?.startsWith("ssh ") || normalizedCmd?.match(/(?:^|\s|\/)(ssh)\s/)) {
                             recordTEvent("conn:connect", { "conn:conntype": "ssh-manual" });
                         }
📜 Review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5a16413 and 2580d3a.

📒 Files selected for processing (1)
  • frontend/app/view/term/termwrap.ts (2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-02-06T02:52:00.271Z
Learnt from: esimkowitz
Repo: wavetermdev/waveterm PR: 1896
File: pkg/remote/fileshare/s3fs/s3fs.go:0-0
Timestamp: 2025-02-06T02:52:00.271Z
Learning: In the Wave Terminal codebase, data must be base64 encoded before assigning it to the `Data64` field to ensure proper data transmission and decoding.

Applied to files:

  • frontend/app/view/term/termwrap.ts
🧬 Code graph analysis (1)
frontend/app/view/term/termwrap.ts (1)
frontend/app/store/global.ts (1)
  • recordTEvent (873-873)
⏰ 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: Analyze (go)
  • GitHub Check: Analyze (javascript-typescript)
  • GitHub Check: Build for TestDriver.ai
🔇 Additional comments (1)
frontend/app/view/term/termwrap.ts (1)

8-17: LGTM!

The import of recordTEvent is correctly added and properly used in the telemetry logic below.

@sawka sawka merged commit cbd979a into main Dec 17, 2025
6 of 7 checks passed
@sawka sawka deleted the sawka/r131-minor-2 branch December 17, 2025 04:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants