feat: refine CLI workflows for datasets, queries, auth, and help#100
Merged
Conversation
📝 WalkthroughWalkthroughThis PR renames and reorganizes query commands under ChangesDatasets, Picker, and Dataset Commands
SQL CLI, Saving, Quoting, Limit, and Fetch
TUI: SQL and PromQL Controls, Time Editing, and Display Modes
CLI UX, Help, Analytics, and List Output
Documentation
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
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 unit tests (beta)
Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/query.go (1)
271-285:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDon't rewrite every dotted identifier into a quoted field.
The
hasDotbranch now turns anya.btoken into"a.b". That breaks valid qualified references likeSELECT a.status FROM logs aand schema-qualified names, because those stop being parsed astable_or_alias.column.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/query.go` around lines 271 - 285, The current loop sets hasDot and then quotes the whole dotted token (identifier) when hasDot is true, which turns qualified names like a.b into a single quoted field; change the logic so dotted identifiers are NOT automatically quoted. Concretely, remove hasDot as a trigger for quoting and only call the quoting branch when shouldQuoteIdentifier(identifier, query, k) returns true (or when an individual segment requires quoting); keep the existing loop that builds identifier but ensure you do not wrap the entire dotted string in quotes — instead leave a.b unchanged (or quote individual segments if you detect they need quoting) while preserving i = k and existing tokenization.
🧹 Nitpick comments (2)
cmd/promql.go (1)
142-161: ⚡ Quick winDocument the hardcoded command order or consider dynamic ordering.
The
orderslice hardcodes the list of subcommands for help rendering. If a new subcommand is added toPromqlCmdbut not included in this slice, it will be silently omitted from help output, which could confuse users and developers.📝 Options to improve maintainability
Option 1: Add a comment documenting the maintenance requirement:
+ // IMPORTANT: When adding new subcommands to PromqlCmd, update this order slice + // to ensure they appear in help output. order := []string{"run", "labels", "label-values", "series", "cardinality", "tsdb", "active-queries"}Option 2: Generate the order dynamically from the actual commands (falling back to default order for known commands, appending others):
// Known command order for help display knownOrder := []string{"run", "labels", "label-values", "series", "cardinality", "tsdb", "active-queries"} order := make([]string, 0, len(cmd.Commands())) seen := make(map[string]bool) // Add known commands in preferred order for _, name := range knownOrder { if _, exists := commandsByName[name]; exists { order = append(order, name) seen[name] = true } } // Append any additional commands not in knownOrder for name := range commandsByName { if !seen[name] { order = append(order, name) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/promql.go` around lines 142 - 161, The help display currently uses a hardcoded order slice named order in the PromqlCmd help rendering (the loop over order and the commandsByName map), which will omit new subcommands; replace the hardcoded order with a dynamically built slice: define knownOrder (the preferred sequence: "run","labels","label-values","series","cardinality","tsdb","active-queries"), build order by appending existing names from knownOrder found in commandsByName (marking them in a seen map), then append any remaining names from commandsByName not in seen; use that order in the existing fmt.Fprintf loop so new subcommands are included automatically, or alternatively add a clear comment above the current order variable explaining the maintenance requirement if you prefer to keep the static list.cmd/queryList_test.go (1)
12-23: ⚡ Quick winRestore
os.Stdoutwitht.Cleanup.This test mutates a process-global. If anything fails before Line 22, later tests inherit the pipe and start failing for unrelated reasons. Register the restore/close logic before swapping stdout.
Suggested cleanup pattern
oldStdout := os.Stdout r, w, err := os.Pipe() if err != nil { t.Fatalf("pipe failed: %v", err) } + t.Cleanup(func() { + os.Stdout = oldStdout + _ = w.Close() + _ = r.Close() + }) os.Stdout = w @@ - w.Close() - os.Stdout = oldStdout + _ = w.Close() io.Copy(&output, r)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/queryList_test.go` around lines 12 - 23, The test mutates os.Stdout and currently restores it after the call, risking leaks if the test fails; register cleanup handlers with t.Cleanup before swapping so restoration/closing always runs: create the pipe (r,w), save oldStdout, then call t.Cleanup to close w, r and restore os.Stdout, then assign os.Stdout = w and call savedQueryToPbQuery(...). Reference the existing symbols oldStdout, r, w and the function savedQueryToPbQuery to locate where to add the t.Cleanup registrations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/logout.go`:
- Around line 120-139: confirmLogout currently always reads stdin which breaks
non-interactive/scripted use; change its signature to accept a force (or yes)
bool (e.g., confirmLogout(profileName, profileURL string, force bool)),
short-circuit and return true immediately when force is true, and before reading
stdin detect whether stdin is a terminal (use os.Stdin.Fd() with
golang.org/x/term.IsTerminal); if stdin is not interactive and force is false,
print a clear message to stderr advising to use --yes/--force and return false
(do not block reading), otherwise proceed with the existing interactive prompt
and parsing logic.
In `@cmd/query.go`:
- Around line 522-555: Replace the full-buffering read of resp.Body (io.ReadAll
+ json.Unmarshal/string(respBody)) with a streaming approach: use a bufio.Reader
(wrapping resp.Body) and Peek/ReadByte to detect empty/whitespace-only responses
and whether the body starts with '[' for the "[]"/no-rows check, and for
non-JSON output simply stream the body to stdout with io.Copy to avoid
buffering; for JSON output use json.Decoder to decode and stream/pretty-print
entries as they arrive (or write using json.Encoder/Indent per item) instead of
json.Unmarshal; for non-200 responses capture only a small preview (io.ReadAll
of io.LimitReader(resp.Body, N)) for the error message rather than reading the
entire body; update code around startSpinner/stopSpinner, the resp variable
handling, and the blocks that reference outputFormat and json.Unmarshal to use
the streaming logic.
In `@cmd/role.go`:
- Around line 248-252: The current RunE prints the role table via
printRoleTable(roles, roleResponses) but still returns success even when some
roleResponses[idx].err are non-nil; collect all per-role errors from
roleResponses (referencing roles and roleResponses) into a single aggregated
error string (you can continue appending to cmd.Annotations["errors"] or build a
local buffer), and after rendering the table return a non-nil error (e.g.,
errors.New or fmt.Errorf) that includes the aggregated message so callers and
scripts receive a failing exit code; adjust RunE to check the aggregated error
and return it instead of nil.
In `@cmd/status.go`:
- Around line 51-55: When the status check fails the code currently prints a
friendly message then returns nil; change that to return a non-nil error so the
process exits with a non-zero code. In the error branch that prints "✗ Not
connected" and the output from statusErrorMessage(err), return the original err
(or wrap it with fmt.Errorf("status check failed: %w", err)) instead of nil so
callers get a non-zero exit status; reference the existing err variable and the
statusErrorMessage helper when making the change.
In `@cmd/user.go`:
- Around line 351-359: The code prints user role rows but always returns nil and
sets cmd.Annotations["error"]="none" even if some per-user fetchUserRoles calls
failed; change the control flow to detect failures in the roleResponses slice
after calling printUserRoleTable(users, roleResponses), aggregate any row-level
errors (e.g. count failures and join their messages), set
cmd.Annotations["error"] to a non-success value and return a non-nil error
(fmt.Errorf with summary) instead of nil; apply this logic in both the
outputFormat == "text" branch and the later branch so pb user ls fails when any
fetchUserRoles failed.
In `@main.go`:
- Around line 65-72: The analytics call in postRunAnalytics currently starts
analytics.PostRunAnalytics in a detached goroutine which can be killed when main
returns (after cli.Execute()), causing dropped events; change postRunAnalytics
to ensure the analytics work completes before process exit—either call
analytics.PostRunAnalytics synchronously (remove the leading "go") or coordinate
with main/cli.Execute() using a shared sync mechanism (e.g., waitgroup or
channel) so main waits for the PostRunAnalytics call to finish; update the
postRunAnalytics function and the main/cli.Execute() exit path to use that
synchronization, referencing postRunAnalytics and analytics.PostRunAnalytics
(and main/cli.Execute()) so callers wait for completion.
In `@pkg/model/query.go`:
- Around line 1466-1507: The hasTopLevelSQLLimit function currently skips quotes
and parentheses but treats text inside SQL comments as code; update
hasTopLevelSQLLimit to ignore SQL comments by either pre-stripping comments or
adding comment-skip logic inside the scanner: when you see "--" skip until
end-of-line and when you see "/*" skip until the closing "*/" (respecting EOF)
so that isSQLLimitTokenAt is never invoked for tokens inside comments; keep
existing handling for quotes and parentheses and call isSQLLimitTokenAt only
when not inside a comment.
In `@pkg/model/timeinput.go`:
- Around line 337-363: renderTimeDisplayMode currently builds localLabel from
time.Now(), which can mismatch displayed timestamps across DST boundaries;
change it to derive the offset from the active/selected timestamp instead.
Update renderTimeDisplayMode to accept (or read) the currently selected time
(e.g., selectedTime time.Time or m.SelectedTime) and replace
time.Now().Format("UTC-07:00") with selectedTime.Format("UTC-07:00") when
composing localLabel; keep the rest of the logic (labelStyle, active/inactive
styles, TimeDisplayUTC handling) unchanged so the label reflects the same
instant shown in the results.
- Around line 286-297: The end/evaluation time pane is always passing a fixed
“show now” flag, so the [NOW] badge appears even for historical values. Update
the time input rendering around renderTimePickerInputBox in timeinput.go to
derive the badge from the actual selected end time (for both regular range and
instant evaluation modes) instead of hard-coding it, and make the same change
anywhere else the end box is built in the referenced render block.
In `@README.md`:
- Around line 85-92: The README's SQL examples block omits the new interactive
save command; add a concise example demonstrating the new top-level command
syntax "pb sql save <query>" (and show optional flags like --from, --to,
--save-as) alongside the existing "pb sql run" examples so users can discover
the "pb sql save" flow; update the SQL examples block to include at least one
line such as: pb sql save "SELECT * FROM backend WHERE status = 500" --from=1h
--save-as=server-errors and mention that it creates a saved query in the same
namespace as the shown --save-as usage.
---
Outside diff comments:
In `@cmd/query.go`:
- Around line 271-285: The current loop sets hasDot and then quotes the whole
dotted token (identifier) when hasDot is true, which turns qualified names like
a.b into a single quoted field; change the logic so dotted identifiers are NOT
automatically quoted. Concretely, remove hasDot as a trigger for quoting and
only call the quoting branch when shouldQuoteIdentifier(identifier, query, k)
returns true (or when an individual segment requires quoting); keep the existing
loop that builds identifier but ensure you do not wrap the entire dotted string
in quotes — instead leave a.b unchanged (or quote individual segments if you
detect they need quoting) while preserving i = k and existing tokenization.
---
Nitpick comments:
In `@cmd/promql.go`:
- Around line 142-161: The help display currently uses a hardcoded order slice
named order in the PromqlCmd help rendering (the loop over order and the
commandsByName map), which will omit new subcommands; replace the hardcoded
order with a dynamically built slice: define knownOrder (the preferred sequence:
"run","labels","label-values","series","cardinality","tsdb","active-queries"),
build order by appending existing names from knownOrder found in commandsByName
(marking them in a seen map), then append any remaining names from
commandsByName not in seen; use that order in the existing fmt.Fprintf loop so
new subcommands are included automatically, or alternatively add a clear comment
above the current order variable explaining the maintenance requirement if you
prefer to keep the static list.
In `@cmd/queryList_test.go`:
- Around line 12-23: The test mutates os.Stdout and currently restores it after
the call, risking leaks if the test fails; register cleanup handlers with
t.Cleanup before swapping so restoration/closing always runs: create the pipe
(r,w), save oldStdout, then call t.Cleanup to close w, r and restore os.Stdout,
then assign os.Stdout = w and call savedQueryToPbQuery(...). Reference the
existing symbols oldStdout, r, w and the function savedQueryToPbQuery to locate
where to add the t.Cleanup registrations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9d37e24b-e04f-427e-9512-c364f4ce4bc9
📒 Files selected for processing (27)
README.mdcmd/dataset.gocmd/dataset_test.gocmd/logout.gocmd/profile.gocmd/promql.gocmd/promql_test.gocmd/query.gocmd/queryList.gocmd/queryList_test.gocmd/query_test.gocmd/role.gocmd/status.gocmd/status_test.gocmd/style.gocmd/user.gomain.gopkg/datasets/datasets.gopkg/datasets/datasets_test.gopkg/model/datetime/datetime.gopkg/model/promql.gopkg/model/promql_test.gopkg/model/query.gopkg/model/query_test.gopkg/model/timeinput.gopkg/model/timeinput_test.gopkg/ui/views/picker.go
This was referenced Jun 4, 2026
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/model/query.go (1)
1699-1744: 🏗️ Heavy liftSQL normalization helpers are duplicated between
cmd/query.goandpkg/model/query.go.
quoteUnsafeSQLTableNameshere mirrorsquoteStreamNamesincmd/query.go. Similarly,quoteUnsafeSQLFieldNamesmirrorsquoteFieldsWithDots, and theensureDefaultSQLLimit/hasTopLevelSQLLimit/endsInSQLLineCommentfunctions are near-identical copies.Consider extracting these into a shared
pkg/sqlpackage to avoid maintenance divergence.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3468afeb-f196-4c2a-b2db-45e6f18d4c1b
📒 Files selected for processing (12)
README.mdcmd/query.gocmd/queryList_test.gocmd/query_test.gocmd/role.gocmd/status.gocmd/user.gopkg/analytics/analytics.gopkg/model/query.gopkg/model/query_test.gopkg/model/timeinput.gopkg/model/timeinput_test.go
✅ Files skipped from review due to trivial changes (2)
- pkg/analytics/analytics.go
- README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/model/timeinput_test.go
- cmd/role.go
Contributor
Author
--This is manjor refactor so will need to do in next version drop or cleanUp! |
nitisht
approved these changes
Jun 4, 2026
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
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
This PR improves the CLI experience across dataset management, SQL/PromQL workflows, auth/profile handling, and help.
Dataset management
pb dataset add <name>opens a themed picker forlogs,metrics, ortracespb dataset add <name> --type logs|metrics|tracessupports scriptspb dataset remove <name> --type logs|metrics|tracesdataset info.pb dataset lspb dataset statSQL workflow
pb sqlflow.pb sql save <query>for saving queries directly.LIMIT 500when a SQL query has no top-level limit.pb sql lsalias.SQL interactive UI
PromQL workflow
pb promql runflow.pb promql --helpcommands by priority, withrunfirst.pb promql psalias foractive-queries.PromQL interactive UI
Auth and profiles
pb logincredentials before saving a profile.pb logoutconfirmation messaging.Users and roles
pb user listandpb role listreadability with aligned table output.pb user lspb role lsCommand help and aliases
pb profile lspb dataset lspb dataset statpb user lspb role lspb sql lspb promql psSummary by CodeRabbit
New Features
lsaliases added for profile, user, role, and saved-query listing.Improvements
pb sqland updated CLI examples.Tests