Add wsh wavepath command for getting Wave paths
#1545
Conversation
WalkthroughThe pull request introduces a new Changes
Sequence DiagramsequenceDiagram
participant User
participant CLI
participant RPCClient
participant WshServer
User->>CLI: Execute wavepath command
CLI->>RPCClient: Call PathCommand
RPCClient->>WshServer: Send path request
WshServer-->>RPCClient: Return path
RPCClient-->>CLI: Provide path
alt Open Internal
CLI->>WshServer: Create block with path
end
alt Open External
CLI->>System: Open path in external application
end
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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: 1
🧹 Nitpick comments (5)
cmd/wsh/cmd/wshcmd-path.go (2)
12-19: Enhance command documentation with examples and detailed descriptionConsider adding usage examples and a more detailed description to help users understand the command better.
var pathCommand = &cobra.Command{ Use: "path [flags] config|data|log", - Short: "Get paths to various waveterm files and directories", + Short: "Get paths to various Waveterm files and directories", + Long: "Get paths to various Waveterm files and directories. Use flags to open paths in a new block or external application.", + Example: ` # Get config file path + wsh path config + # Open data directory in default application + wsh path data --open-external + # Open log file in a new block + wsh path log --open`, ValidArgs: []string{"config", "data", "log"}, Args: cobra.MatchAll(cobra.OnlyValidArgs, cobra.ExactArgs(1)), Run: runPathCommand, PreRunE: preRunSetupRpcClient, }
21-25: Consider making --open and --open-external flags mutually exclusiveOpening a path both internally and externally might lead to unexpected behavior.
Add flag validation in PreRunE:
func init() { pathCommand.Flags().BoolP("open", "o", false, "Open the path in a new block") pathCommand.Flags().BoolP("open-external", "O", false, "Open the path in the default external application") + oldPreRunE := pathCommand.PreRunE + pathCommand.PreRunE = func(cmd *cobra.Command, args []string) error { + if err := oldPreRunE(cmd, args); err != nil { + return err + } + open, _ := cmd.Flags().GetBool("open") + openExternal, _ := cmd.Flags().GetBool("open-external") + if open && openExternal { + return fmt.Errorf("--open and --open-external flags cannot be used together") + } + return nil + } rootCmd.AddCommand(pathCommand) }pkg/waveobj/wtype.go (1)
211-211: Add documentation for the Ephemeral fieldThe purpose and behavior of the
Ephemeralfield should be documented.type LayoutActionData struct { ActionType string `json:"actiontype"` BlockId string `json:"blockid"` NodeSize *uint `json:"nodesize,omitempty"` IndexArr *[]int `json:"indexarr,omitempty"` Focused bool `json:"focused"` Magnified bool `json:"magnified"` - Ephemeral bool `json:"ephemeral"` + // Ephemeral indicates whether the block should be treated as temporary + // and cleaned up when the window is closed + Ephemeral bool `json:"ephemeral"` }pkg/wshrpc/wshrpctypes.go (1)
609-614: Consider adding documentation for PathType values.While the struct is well-designed, it would benefit from:
- Documentation comments describing valid values for the
PathTypefield- Constants defining the allowed path types
- Validation logic for the path type values
+// PathType values: "config", "data", "log", etc. +const ( + PathTypeConfig = "config" + PathTypeData = "data" + PathTypeLog = "log" +) + type PathCommandData struct { PathType string `json:"pathtype"` Open bool `json:"open"` OpenExternal bool `json:"openexternal"` TabId string `json:"tabid" wshcontext:"TabId"` }frontend/types/gotypes.d.ts (1)
404-404: Consider making ephemeral property optionalThe ephemeral property in LayoutActionData is marked as required, but it might be better to make it optional for backward compatibility.
- ephemeral: boolean; + ephemeral?: boolean;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
cmd/wsh/cmd/wshcmd-path.go(1 hunks)frontend/app/store/wshclientapi.ts(1 hunks)frontend/layout/lib/layoutModel.ts(1 hunks)frontend/types/gotypes.d.ts(3 hunks)go.mod(1 hunks)pkg/waveobj/wtype.go(1 hunks)pkg/wshrpc/wshclient/wshclient.go(1 hunks)pkg/wshrpc/wshrpctypes.go(3 hunks)pkg/wshrpc/wshserver/wshserver.go(3 hunks)
🔇 Additional comments (9)
go.mod (1)
23-23: Verify security status of open-golang dependency
The github.com/skratchdot/open-golang package hasn't been updated since 2020. Consider alternatives or verify its security status.
✅ Verification successful
Based on the results, I can generate the final response:
Package github.com/skratchdot/open-golang appears safe to use
While the package hasn't been updated since January 2020, the verification shows:
- No security vulnerabilities reported in GitHub's advisory database
- Simple, focused functionality with minimal attack surface
- No critical open issues (17 total issues, mostly feature requests/discussions)
The package's inactivity is not a major concern given its stable, single-purpose nature of providing cross-platform file opening capabilities.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for known vulnerabilities and activity status
# Check latest commit date and activity
gh api repos/skratchdot/open-golang/commits/master --jq '.commit.committer.date'
# Check open issues and PRs
gh api repos/skratchdot/open-golang/issues --jq 'length'
# Check for security advisories
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: GO, package: "github.com/skratchdot/open-golang") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
}
}
}'
# Look for alternative packages
rg -l 'import.*"os/exec".*xdg-open|start|open'
Length of output: 5074
frontend/app/store/wshclientapi.ts (1)
205-208: LGTM! The PathCommand implementation follows established patterns.
The implementation is consistent with other RPC commands in the class and properly handles types and error cases.
pkg/wshrpc/wshclient/wshclient.go (1)
250-254: LGTM! The PathCommand function follows established patterns.
The implementation is consistent with other RPC command functions and properly handles the RPC communication and error cases.
pkg/wshrpc/wshrpctypes.go (1)
294-294: LGTM! The Ephemeral field addition is well-structured.
The field follows Go naming conventions and is properly tagged for JSON serialization.
pkg/wshrpc/wshserver/wshserver.go (2)
188-188: LGTM: Ephemeral field added to CreateBlockCommand
The addition of the Ephemeral field to the layout action is consistent with the changes in the frontend types.
836-870:
Verify error handling and path security
The PathCommand implementation has a few concerns that need to be addressed:
- The path construction could potentially lead to path traversal if not properly sanitized
- Error handling could be more specific for different failure scenarios
- The external path opening could pose security risks if not properly validated
Consider applying these improvements:
func (ws *WshServer) PathCommand(ctx context.Context, data wshrpc.PathCommandData) (string, error) {
+ if err := validatePathType(data.PathType); err != nil {
+ return "", fmt.Errorf("invalid path type: %w", err)
+ }
+
var path string
switch pathType {
case "config":
path = wavebase.GetWaveConfigDir()
case "data":
path = wavebase.GetWaveDataDir()
case "log":
path = filepath.Join(wavebase.GetWaveDataDir(), "waveapp.log")
+ default:
+ return "", fmt.Errorf("unsupported path type: %s", pathType)
}
+ // Ensure the path exists and is accessible
+ if _, err := os.Stat(path); err != nil {
+ return "", fmt.Errorf("path access error: %w", err)
+ }
+
if openInternal && openExternal {
return "", fmt.Errorf("open and openExternal cannot both be true")
}
if openInternal {
_, err := ws.CreateBlockCommand(ctx, wshrpc.CommandCreateBlockData{BlockDef: &waveobj.BlockDef{Meta: map[string]any{
waveobj.MetaKey_View: "preview",
waveobj.MetaKey_File: path,
}}, Ephemeral: true, TabId: data.TabId})
if err != nil {
- return path, fmt.Errorf("error opening path: %w", err)
+ return "", fmt.Errorf("error opening path internally: %w", err)
}
} else if openExternal {
+ // Validate path before opening externally
+ if err := validateExternalPath(path); err != nil {
+ return "", fmt.Errorf("invalid external path: %w", err)
+ }
+
err := open.Run(path)
if err != nil {
- return path, fmt.Errorf("error opening path: %w", err)
+ return "", fmt.Errorf("error opening path externally: %w", err)
}
}
return path, nil
}Run the following script to check for similar path handling patterns in the codebase:
frontend/types/gotypes.d.ts (2)
567-573: LGTM: PathCommandData type definition
The type definition is well-structured and includes all necessary properties for path command handling.
142-142: LGTM: Optional ephemeral property in CommandCreateBlockData
The optional ephemeral property is correctly typed as boolean and aligns with the backend changes.
frontend/layout/lib/layoutModel.ts (1)
415-419: LGTM: Ephemeral node handling
The implementation correctly handles ephemeral nodes by creating them through newEphemeralNode when the action indicates it's ephemeral.
cmd/wsh/cmd/wshcmd-path.go
Outdated
| func runPathCommand(cmd *cobra.Command, args []string) { | ||
| pathType := args[0] | ||
| open, _ := cmd.Flags().GetBool("open") | ||
| openExternal, _ := cmd.Flags().GetBool("open-external") | ||
| path, err := wshclient.PathCommand(RpcClient, wshrpc.PathCommandData{ | ||
| PathType: pathType, | ||
| Open: open, | ||
| OpenExternal: openExternal, | ||
| }, nil) | ||
| if err != nil { | ||
| WriteStderr("Error getting path: %v\n", err) | ||
| return | ||
| } | ||
| WriteStdout("%s\n", path) | ||
| } |
There was a problem hiding this comment.
Improve error handling in runPathCommand
The function ignores potential flag retrieval errors and could provide more specific error messages.
func runPathCommand(cmd *cobra.Command, args []string) {
pathType := args[0]
- open, _ := cmd.Flags().GetBool("open")
- openExternal, _ := cmd.Flags().GetBool("open-external")
+ open, err := cmd.Flags().GetBool("open")
+ if err != nil {
+ WriteStderr("Error getting open flag: %v\n", err)
+ return
+ }
+ openExternal, err := cmd.Flags().GetBool("open-external")
+ if err != nil {
+ WriteStderr("Error getting open-external flag: %v\n", err)
+ return
+ }
path, err := wshclient.PathCommand(RpcClient, wshrpc.PathCommandData{
PathType: pathType,
Open: open,
OpenExternal: openExternal,
}, nil)
if err != nil {
- WriteStderr("Error getting path: %v\n", err)
+ WriteStderr("Error getting %s path: %v\n", pathType, err)
return
}
WriteStdout("%s\n", path)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func runPathCommand(cmd *cobra.Command, args []string) { | |
| pathType := args[0] | |
| open, _ := cmd.Flags().GetBool("open") | |
| openExternal, _ := cmd.Flags().GetBool("open-external") | |
| path, err := wshclient.PathCommand(RpcClient, wshrpc.PathCommandData{ | |
| PathType: pathType, | |
| Open: open, | |
| OpenExternal: openExternal, | |
| }, nil) | |
| if err != nil { | |
| WriteStderr("Error getting path: %v\n", err) | |
| return | |
| } | |
| WriteStdout("%s\n", path) | |
| } | |
| func runPathCommand(cmd *cobra.Command, args []string) { | |
| pathType := args[0] | |
| open, err := cmd.Flags().GetBool("open") | |
| if err != nil { | |
| WriteStderr("Error getting open flag: %v\n", err) | |
| return | |
| } | |
| openExternal, err := cmd.Flags().GetBool("open-external") | |
| if err != nil { | |
| WriteStderr("Error getting open-external flag: %v\n", err) | |
| return | |
| } | |
| path, err := wshclient.PathCommand(RpcClient, wshrpc.PathCommandData{ | |
| PathType: pathType, | |
| Open: open, | |
| OpenExternal: openExternal, | |
| }, nil) | |
| if err != nil { | |
| WriteStderr("Error getting %s path: %v\n", pathType, err) | |
| return | |
| } | |
| WriteStdout("%s\n", path) | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
cmd/wsh/cmd/wshcmd-wavepath.go (4)
36-43: Simplify argument validation by checking forlen(args) != 1Currently, the code separately handles the cases when no arguments or too many arguments are provided. You can simplify the argument validation by checking if the number of arguments is not equal to one, and provide a single error message.
Apply this diff to simplify the validation:
if len(args) != 1 { OutputHelpMessage(cmd) - if len(args) == 0 { - return fmt.Errorf("no arguments. wsh wavepath requires a type argument (config, data, or log)") - } - if len(args) > 1 { - return fmt.Errorf("too many arguments. wsh wavepath requires exactly one argument") - } + return fmt.Errorf("wsh wavepath requires exactly one argument (config, data, or log)") }
46-49: Consider defining valid path types as a constant or map for scalabilityTo improve maintainability and scalability, especially if more path types might be added in the future, consider defining the valid path types as a slice or map and checking against it.
Here's how you might implement it:
var validPathTypes = map[string]struct{}{ "config": {}, "data": {}, "log": {}, } if _, ok := validPathTypes[pathType]; !ok { OutputHelpMessage(cmd) return fmt.Errorf("invalid path type %q. must be one of: config, data, log", pathType) }
100-103: Useio.SeekStartconstant instead of0for file seekingFor better readability and to avoid magic numbers, use the
io.SeekStartconstant when seeking from the start of the file.Apply this diff:
-_, err = file.Seek(offset, 0) +_, err = file.Seek(offset, io.SeekStart)
106-109: Handle potential partial reads when reading the fileThe
file.Read(buf)call might read fewer bytes than requested. To ensure you read as much as possible, consider usingio.ReadAtLeastorio.ReadFull.Apply this diff to handle partial reads:
-n, err := file.Read(buf) +_, err := io.ReadFull(file, buf) if err != nil && err != io.EOF { return fmt.Errorf("reading file: %w", err) } +// Adjust buffer size if fewer bytes were read +if err == io.EOF { + buf = buf[:n] +}docs/docs/wsh-reference.mdx (4)
698-704: Enhance command syntax presentation for clarityFor better readability and consistency, add the shell prompt symbol and use syntax highlighting in the command usage example.
Apply this diff:
```bash -wsh wavepath {config|data|log} +$ +wsh wavepath {config|data|log}--- `708-711`: **Clarify the exact number of lines shown with `--tail` flag** The description uses `~100 lines`, which may imply an approximate number. Since the code displays exactly the last 100 lines, consider updating the description for accuracy. Apply this diff: ```diff - `-t, --tail` - show the last ~100 lines of the log file (only valid for log path) +- `-t, --tail` - show the last 100 lines of the log file (only valid for `log` path)
725-726: Use long-form flags in examples for clarityIn the example commands, consider using the long-form flags for better understanding, especially for users unfamiliar with the shorthand.
Apply this diff:
-wsh wavepath -o log +wsh wavepath --open log
727-729: Consistently use long-form flags in examplesContinue using long-form flags for consistency and clarity in the examples.
Apply this diff:
-wsh wavepath -O config +wsh wavepath --open-external config
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/wsh/cmd/wshcmd-wavepath.go(1 hunks)docs/docs/wsh-reference.mdx(1 hunks)
🔇 Additional comments (1)
cmd/wsh/cmd/wshcmd-wavepath.go (1)
113-118: Handle case where no newline is found when skipping partial line
When skipping the partial line at the start, if no newline character is found, the entire buffer might be skipped. Ensure that this case is handled appropriately to avoid losing data.
Run the following script to verify how the code behaves when no newline is found:
| open, _ := cmd.Flags().GetBool("open") | ||
| openExternal, _ := cmd.Flags().GetBool("open-external") | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation to prevent conflicting --open and --open-external flags
Currently, if both --open and --open-external flags are set, the behavior may be undefined or undesirable. Consider adding validation to ensure these flags are not used together.
Apply this diff to add the validation:
open, _ := cmd.Flags().GetBool("open")
openExternal, _ := cmd.Flags().GetBool("open-external")
+if open && openExternal {
+ return fmt.Errorf("--open and --open-external cannot be used together")
+}
path, err := wshclient.PathCommand(RpcClient, wshrpc.PathCommandData{
PathType: pathType,
Open: open,
OpenExternal: openExternal,Committable suggestion skipped: line range outside the PR's diff.
sawka
changed the title
wsh path command for getting Wave pathswsh wavepath command for getting Wave paths
Summary by CodeRabbit
Release Notes
New Features
wavepathto retrieve paths for configuration, data, and log files via CLI.PathCommandfunctionality to handle path-related commands in the application.Bug Fixes
wavepathcommand.Documentation
wavepathcommand.Type Enhancements