conn updates 4 #1726
conn updates 4 #1726
Conversation
…h combo to save an additional session.
…sistent -- requires an absolute path. hardquote path on go side (for posix and pwsh), so no quotes in the actual files. remote quoting for tilde paths
…te setup. rename some functions to make it more clear they are for local use only
|
Warning Rate limit exceeded@sawka has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 20 minutes and 57 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe pull request introduces a comprehensive set of modifications across multiple files in the Wave Terminal project. The changes primarily focus on enhancing shell integration, improving connection management, and refining error handling and logging mechanisms. Key modifications include updating function signatures to incorporate context management, adding new shell type constants, introducing more robust path resolution for shell binaries, and expanding connection configuration options. The modifications span various components of the application, including remote connection handling, shell execution, utility functions, and frontend components. Notable additions include new constants for shell types, methods for detecting shell paths, and expanded connection keyword structures. The changes aim to improve the flexibility, maintainability, and robustness of the Wave Terminal's shell interaction and connection management capabilities. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🔭 Outside diff range comments (1)
pkg/shellexec/shellexec.go (1)
Line range hint
272-314: Utilize context parameter withinStartRemoteShellProcNoWshfor cancellationThe
ctx context.Contextparameter is introduced toStartRemoteShellProcNoWshbut isn't used within the function body. To fully leverage context benefits, consider checkingctx.Done()at appropriate points and passingctxto dependent functions to handle cancellations and timeouts during SSH session creation and command execution.
🧹 Nitpick comments (8)
pkg/shellexec/shellexec.go (3)
161-176: Ensure robust shell detection with case-insensitive matchingThe
getShellTypeFromShellPathfunction usesstrings.Containsto detect shell types, which may fail for shell paths with different casing (e.g., "Bash", "ZSH"). Consider normalizingshellBaseto lower case to handle case variations.Proposed change:
func getShellTypeFromShellPath(shellPath string) string { shellBase := filepath.Base(shellPath) + shellBase = strings.ToLower(shellBase) if strings.Contains(shellBase, "bash") { return ShellType_bash } if strings.Contains(shellBase, "zsh") { return ShellType_zsh } // Apply similar changes for other shell types
Line range hint
315-393: Incorporate context intoStartRemoteShellProcfor better control flowSimilar to
StartRemoteShellProcNoWsh,StartRemoteShellProcacceptsctx context.Contextbut doesn't utilize it. Integratectxinto the function logic to manage execution flow, especially during network operations that might benefit from context-based cancellation and timeout handling.
466-500: Refactor shell option construction to reduce code duplicationThe conditional logic for constructing
shellOptsbased onshellTypehas repetitive patterns across different shells. Consider refactoring this logic into a separate helper function to improve code maintainability and readability.pkg/genconn/ssh-impl.go (1)
45-45: Consider using structured logging.While adding logging for SSH session creation is valuable, consider using structured logging with additional context (e.g., client ID or connection details) to make debugging easier.
-log.Printf("SSH-NEWSESSION (cmdclient)\n") +log.Printf("SSH-NEWSESSION (cmdclient) client=%v\n", client.RemoteAddr())pkg/remote/connutil.go (1)
90-93: Consider using && for fail-fast behavior.While adding semicolons improves shell compatibility, using
&&would ensure the script stops on the first failure.-mkdir -p {{.installDir}} || exit 1; -cat > {{.tempPath}} || exit 1; -mv {{.tempPath}} {{.installPath}} || exit 1; -chmod a+x {{.installPath}} || exit 1; +mkdir -p {{.installDir}} || exit 1 && \ +cat > {{.tempPath}} || exit 1 && \ +mv {{.tempPath}} {{.installPath}} || exit 1 && \ +chmod a+x {{.installPath}} || exit 1pkg/blockcontroller/blockcontroller.go (1)
372-386: Consider enhancing error handling in WSH fallback scenario.While the addition of context support is good, the error handling in the WSH fallback scenario could be improved. When WSH fails and falls back to no-WSH mode, we should consider:
- Logging the original WSH error for debugging
- Adding more detailed error information in the connection status
Consider updating the error handling:
} else { shellProc, err = shellexec.StartRemoteShellProc(ctx, rc.TermSize, cmdStr, cmdOpts, conn) if err != nil { conn.SetWshError(err) conn.WshEnabled.Store(false) - log.Printf("error starting remote shell proc with wsh: %v", err) - log.Print("attempting install without wsh") + log.Printf("error starting remote shell proc with wsh (falling back to no-wsh mode): %v", err) shellProc, err = shellexec.StartRemoteShellProcNoWsh(ctx, rc.TermSize, cmdStr, cmdOpts, conn) if err != nil { + // Combine both errors for better debugging + return nil, fmt.Errorf("shell process start failed: with-wsh: %v, no-wsh: %v", conn.GetWshError(), err) } } }pkg/wshrpc/wshserver/wshserver.go (1)
713-716: Consider utilizing the discarded architecture information.The additional return value (architecture information) is being discarded. This information could be useful for logging or debugging when WSH is not installed.
Consider logging the architecture information when WSH is not up to date:
- upToDate, _, _, err := conncontroller.IsWshVersionUpToDate(remoteInfo.ClientVersion) + upToDate, _, osArchStr, err := conncontroller.IsWshVersionUpToDate(remoteInfo.ClientVersion) if err != nil { return false, fmt.Errorf("unable to compare wsh version: %w", err) } if upToDate { // no need to update log.Printf("wsh is already up to date for connection %s", connName) return false, nil } + if !upToDate { + log.Printf("wsh needs update for connection %s (arch: %s)", connName, osArchStr) + }frontend/app/view/preview/preview.tsx (1)
41-42: Address the TODO comment for configuration-driven bookmarks.The bookmarks should be driven by configuration to allow users to customize their navigation shortcuts.
Would you like me to help implement a configuration-driven solution? I can:
- Add a new setting type for bookmarks in the configuration.
- Create a function to load bookmarks from the configuration.
- Add a UI for managing bookmarks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
cmd/wsh/cmd/wshcmd-ai.go(1 hunks)cmd/wsh/cmd/wshcmd-rcfiles.go(1 hunks)cmd/wsh/cmd/wshcmd-setbg.go(1 hunks)frontend/app/view/preview/preview.tsx(2 hunks)frontend/app/view/waveai/waveai.tsx(0 hunks)frontend/types/gotypes.d.ts(1 hunks)pkg/blockcontroller/blockcontroller.go(2 hunks)pkg/genconn/quote.go(2 hunks)pkg/genconn/ssh-impl.go(2 hunks)pkg/remote/conncontroller/conncontroller.go(8 hunks)pkg/remote/connutil.go(4 hunks)pkg/shellexec/shellexec.go(7 hunks)pkg/util/shellutil/shellutil.go(10 hunks)pkg/wshrpc/wshrpctypes.go(1 hunks)pkg/wshrpc/wshserver/wshserver.go(2 hunks)pkg/wshutil/wshutil.go(1 hunks)pkg/wsl/wsl.go(1 hunks)
💤 Files with no reviewable changes (1)
- frontend/app/view/waveai/waveai.tsx
✅ Files skipped from review due to trivial changes (2)
- cmd/wsh/cmd/wshcmd-setbg.go
- pkg/wshutil/wshutil.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build for TestDriver.ai
🔇 Additional comments (23)
pkg/shellexec/shellexec.go (1)
38-44: Introduce shell type constants for clarity and maintainabilityDefining shell type constants enhances code readability and maintainability by avoiding magic strings. It also simplifies future updates when supporting additional shells.
pkg/remote/conncontroller/conncontroller.go (6)
72-77: Enhance readability by usingstrings.Joinin command templateThe restructured
ConnServerCmdTemplateusingstrings.Joinimproves code clarity, making the multi-line command easier to read and maintain.
232-246: UpdateIsWshVersionUpToDateto provide detailed version infoModifying
IsWshVersionUpToDateto returnosArchStralong with the version enhances error handling by supplying additional context whenwshis not installed, facilitating better decision-making downstream.
249-255: Encapsulate WSH path logic ingetWshPathmethodIntroducing the
getWshPathfunction centralizes WSH path determination logic, improving code modularity and simplifying maintenance by avoiding repeated code.
Line range hint
268-379: AdjustStartConnServerto returnosArchStrfor installation flowBy updating
StartConnServerto returnosArchStr, the code can better handle WSH installation scenarios when the client version is not up to date. This change enhances error handling and allows for prompt installation of the correct WSH version.
462-475: ModifyInstallWshto useosArchStrfor accurate installationUpdating
InstallWshto acceptosArchStrallows for precise detection of the client's OS and architecture, ensuring that the correct version of WSH is installed. This change improves the reliability of the installation process.
576-579:⚠️ Potential issueFix uninitialized
errvariable in error handlingThe
errvariable is checked but hasn't been assigned a value, which will cause a compilation error. Ensureerris properly assigned before usage.Proposed fix:
existingConnection, ok := conn.getConnectionConfig() -if err != nil { - // i do not consider this a critical failure - log.Printf("config read error: unable to save connection %s: %v", conn.GetName(), err) -} +if !ok { + // i do not consider this a critical failure + log.Printf("config read error: unable to save connection %s", conn.GetName()) +}Alternatively, remove the unused error handling if
erris not applicable.Likely invalid or redundant comment.
cmd/wsh/cmd/wshcmd-rcfiles.go (1)
22-22: CorrectWriteStderrfunction usage by adding format specifierAdding the format string to
WriteStderrensures proper formatting of the error message and prevents runtime errors due to missing format specifiers.pkg/genconn/quote.go (3)
9-10: LGTM! Clear distinction between Unix and PowerShell patterns.The
psSafePatterncorrectly excludes forward slash since PowerShell uses backslash as path separator.
26-38: Improved escape handling with better structure.The switch-case structure makes the code more maintainable and the newline handling is now explicit.
Line range hint
40-65: Consider adding unit tests for PowerShell quoting.The PowerShell-specific quoting implementation looks correct, using backtick as escape character and handling special cases appropriately. However, this new functionality should be thoroughly tested.
Would you like me to help generate comprehensive unit tests for the PowerShell quoting functionality?
cmd/wsh/cmd/wshcmd-ai.go (1)
145-146: Verify memory impact of increased message size.The message size limit has been increased from 10KB to 50KB. While this provides more flexibility for users, we should verify that this doesn't cause memory pressure, especially in concurrent scenarios.
✅ Verification successful
Message size increase to 50KB is safe
The increase from 10KB to 50KB is safe as messages are processed synchronously and the limit is well within the global 64MB threshold. There's no evidence of concurrent processing or complex buffering that could amplify memory impact.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any memory-related configurations or limitations in the codebase rg -A 3 'memory|heap|buffer.*size|maxSize' # Look for similar size limits in other parts of the code rg '(\d+)\s*\*\s*1024'Length of output: 119535
Script:
#!/bin/bash # Check message handling patterns and buffering rg -A 5 'message.*process|buffer.*message|message.*buffer|message.*pool|message.*queue' # Look for concurrent message handling rg -A 5 'go\s+func.*message|chan.*message|mutex.*message' # Check for memory management patterns rg -A 5 'sync\.Pool|buffer\.Pool|memory.*limit|memory.*manage'Length of output: 117083
Script:
#!/bin/bash # Check message processing implementation in wshcmd-ai.go rg -A 10 'func.*message|process.*message' cmd/wsh/cmd/wshcmd-ai.go # Look for goroutine patterns and channel usage rg -A 5 'go\s+func|chan\s+\w+|make\(chan' cmd/wsh/cmd/wshcmd-ai.go # Check buffer management rg -A 5 'buffer.*size|maxBuffer|MaxBuffer' cmd/wsh/cmd/Length of output: 642
pkg/remote/connutil.go (2)
77-87: LGTM! Well-structured platform parsing.The function properly normalizes and validates the OS/architecture string input.
98-98: LGTM! Improved path handling and logging.The changes simplify path handling and add helpful debug logging. The switch to
GetLocalWshBinaryPathaligns with the broader changes in the codebase.Also applies to: 109-110, 116-116
pkg/util/shellutil/shellutil.go (4)
50-53: LGTM! Improved Zsh shell integration with proper ZDOTDIR handling.The changes correctly handle the ZDOTDIR environment variable across Zsh startup files, ensuring proper sourcing of user configuration files while maintaining shell environment consistency.
Also applies to: 65-69, 73-84
41-41: LGTM! Added comprehensive Fish shell support.The changes introduce proper Fish shell integration with:
- Integration directory setup
- PATH management
- Wave completions support
Also applies to: 109-116, 247-249, 291-295
119-123: LGTM! Enhanced PowerShell integration with proper path handling.The changes improve PowerShell support with:
- OS-aware path separator handling
- PowerShell-specific environment setup
- Proper quoting for PowerShell paths
Also applies to: 302-312
277-279: LGTM! Improved path safety in InitRcFiles.The function signature now explicitly requires an absolute, expanded path for
absWshBinDir, preventing potential path resolution issues.pkg/wsl/wsl.go (1)
340-340: LGTM! Updated function call to match new naming convention.The change correctly uses the renamed
GetLocalWshBinaryPathfunction, maintaining consistency with the changes in shellutil.go.pkg/wshrpc/wshrpctypes.go (1)
478-478: LGTM! Added shell path configuration support.The new
ConnShellPathfield properly extends connection configuration options, following the existing field naming convention and JSON tag patterns.frontend/types/gotypes.d.ts (1)
307-307: LGTM! The new property follows the established conventions.The addition of the optional
"conn:shellpath"property toConnKeywordstype is well-structured and maintains backward compatibility.pkg/wshrpc/wshserver/wshserver.go (1)
699-699: Verify the empty string parameter usage.The empty string parameter passed to
InstallWshmight indicate a missing value. Consider whether a meaningful value should be provided instead.Run this script to check if there are any references to this parameter in the codebase:
frontend/app/view/preview/preview.tsx (1)
197-201: LGTM! The context menu implementation is clean and maintainable.The dynamic generation of menu items from the
BOOKMARKSarray:
- Improves code maintainability by centralizing the bookmark definitions
- Enhances user experience by showing both the label and path
- Makes the code more DRY by eliminating repetitive menu item definitions
No description provided.