feat: SQL dataset/column spotlight and UX polish#97
Merged
Conversation
|
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughAdds a UI theming system and chrome primitives, a root App model and view interface, new picker/placeholder views, chart/highlight/icons/logo helpers, palette-driven styling across many models, large SQL and PromQL TUI refactors with spotlights and responsive sizing, saved-queries CLI/UI simplifications, and related tests and dependency updates. ChangesTUI Theme System and Architecture Refactor
🎯 4 (Complex) | ⏱️ ~60 minutes
🚥 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)
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: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
pkg/model/promql.go (2)
399-407:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDefer the initial fetch until a dataset is known.
Init()can dispatchNewPromqlFetchTaskwith an emptym.dataset. When the model starts with an expression but no dataset, the first request fails beforefetchMetricDatasetsauto-selects one.🤖 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 `@pkg/model/promql.go` around lines 399 - 407, The initial PromQL fetch can run with an empty m.dataset; change the startup ordering so dataset discovery happens before any query fetch and only enqueue NewPromqlFetchTask when a dataset is known: call fetchMetricDatasets(m.profile) first, then (if m.dataset != "") append fetchCacheMetrics(m.profile, m.dataset) and only then append NewPromqlFetchTask(m.profile, m.query.Value(), m.dataset, ...); ensure Init() no longer dispatches NewPromqlFetchTask when m.dataset is empty.
1926-1929:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEscape matcher values before formatting the PromQL expression.
pkg/model/promql.go(around lines 1926-1929) interpolatesvaluedirectly into a double-quoted PromQL label matcher ({label="%s"}) without escaping. In PromQL string literals,\starts an escape sequence and"must be escaped, so embedded\or"can break the query or change semantics. Escapevalueusing PromQL string-literal rules (at minimum:\→\\and"→\"; also escape control chars like newline as\n).🤖 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 `@pkg/model/promql.go` around lines 1926 - 1929, The code directly interpolates the label `value` into a PromQL quoted matcher; modify the logic that builds `{label="%s"}` to first escape `value` according to PromQL string-literal rules (at minimum replace `\`→`\\`, `"`→`\"`, and control characters like newline/tab → `\n`/`\t`), then use the escaped string in fmt.Sprintf; keep the existing special-case branch for empty or "(any)" values that returns `%s{%s!=""}` and only apply escaping for the quoted matcher path (the variables to change are `value`, and the fmt.Sprintf call that currently produces `%s{%s="%s"}`).pkg/model/timerange.go (1)
33-49:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winExpose
FiveMinutein the selectabletimeDurationslist.
FiveMinuteis defined but never surfaced in the list model, so users can’t pick it from the time-range UI.Suggested patch
var ( timeDurations = []list.Item{ + timeDurationItem{duration: FiveMinute, repr: "5 Minutes"}, timeDurationItem{duration: TenMinute, repr: "10 Minutes"}, timeDurationItem{duration: OneHour, repr: "1 Hour"}, timeDurationItem{duration: FiveHour, repr: "5 Hours"}, timeDurationItem{duration: OneDay, repr: "1 Day"}, timeDurationItem{duration: ThreeDay, repr: "3 Days"}, } )🤖 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 `@pkg/model/timerange.go` around lines 33 - 49, The selectable durations slice timeDurations omits the FiveMinute constant, so add a timeDurationItem with duration: FiveMinute and an appropriate repr (e.g., "5 Minutes") to the timeDurations slice so users can select it; update the slice near the timeDurationItem entries (reference symbols: FiveMinute, timeDurations, timeDurationItem) to include this new item in the desired order.
🧹 Nitpick comments (4)
pkg/ui/app.go (1)
158-164: ⚡ Quick winEscape key comment is incomplete.
The comment on lines 159-160 states "Esc on Help / Time / Picker returns to whichever view the user came from," but the implementation only handles
ViewHelp. Either extend the handler to coverViewTimeandViewPickeror update the comment to reflect current behavior.🤖 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 `@pkg/ui/app.go` around lines 158 - 164, The comment for the "esc" key handler is incorrect: it claims Esc on Help / Time / Picker returns to the previous view but the code only handles ViewHelp (in the switch case "esc" using a.current and ViewHelp). Either update the comment to state Esc currently only returns to ViewQuery from ViewHelp, or implement the missing branches: detect a.current == ViewTime and a.current == ViewPicker and set a.current = ViewQuery (or restore the actual previous view if that logic exists elsewhere). Modify the case "esc" block around the a.current checks (ViewHelp, ViewTime, ViewPicker) and ensure the behavior and comment are consistent.pkg/ui/views/picker.go (2)
99-109: "p" key serves dual purpose (acknowledged TODO).The TODO comment on lines 100-103 correctly identifies that "p" acts as both a filter character and a pin command, which can be confusing. Consider tracking this as a follow-up task to implement
ctrl+pfor pinning.🤖 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 `@pkg/ui/views/picker.go` around lines 99 - 109, Update the existing TODO about dual-purpose "p" key into a concrete follow-up task: create an issue or task in the project tracker to implement a dedicated pin hotkey (e.g., Ctrl+P) and reference that issue ID in the inline comment above the pin handling block (the code manipulating v.cursor, v.filtered, v.pinned and calling v.filter). In the comment replace the vague TODO with a short note like "TODO: replace literal 'p' command with Ctrl+P (see ISSUE-###)" so maintainers can find the planned change and so reviewers know the plan.
113-116: ⚡ Quick winFilter input doesn't handle non-ASCII characters.
The printable character check only accepts ASCII range
0x20-0x7F, preventing users from filtering dataset names containing accented letters or other Unicode characters. If your dataset names include non-ASCII characters, consider usingunicode.IsPrint(rune)or widening the accepted range.🤖 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 `@pkg/ui/views/picker.go` around lines 113 - 116, The current printable check only allows ASCII bytes (0x20-0x7F) and rejects multi-byte UTF-8 input; change the input handling in the picker input branch so it validates runes using unicode.IsPrint before appending to v.query and calling v.filter(), e.g., iterate runes of the incoming string s, ensure each rune passes unicode.IsPrint (or otherwise accept the whole string if all runes printable), then append s to v.query and call v.filter(); also add the required import for the unicode package. This touches the logic around v.query and v.filter() in the picker input handler.pkg/ui/theme.go (1)
222-225: ⚡ Quick winInvalidate cached syntax style when changing the active palette.
SetActiverefreshesActiveandActiveType, but cached highlighter style (pbStyle) remains from the previous palette, so highlight colors can drift from the selected theme.♻️ Proposed fix
func SetActive(p Palette) { Active = p ActiveType = NewTypography(p) + pbStyle = nil // force rebuild with the new palette on next highlight call }🤖 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 `@pkg/ui/theme.go` around lines 222 - 225, SetActive currently updates Active and ActiveType (via NewTypography) but doesn't clear the cached syntax highlighter style pbStyle, causing stale colors; update SetActive to invalidate the cached style by resetting pbStyle (e.g., set pbStyle to nil or call whatever reset function exists) whenever Active is changed so subsequent calls recreate the style for the new palette.
🤖 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/queryList.go`:
- Around line 154-160: The current code calls fetchData(...) and only logs
errors, causing the command to exit 0 on failure; modify the caller to propagate
the error instead of swallowing it — either return the error from the enclosing
RunE handler or call os.Exit(1) after printing the error. Specifically update
the block that calls fetchData (and uses startSpinner/stopSpinner) so that on
non-nil err you propagate it (return err from the function that implements RunE
for the `pb query list` command) or explicitly os.Exit(1) if returning is not
applicable, ensuring failures surface as non-zero exits.
In `@pkg/model/promql.go`:
- Around line 789-803: Currently selecting a dataset always clears m.query and
cache fields even if the chosen dataset equals the currently active one; update
the selection handler (the block using m.filteredDatasets[m.datasetSelectedIdx]
and assigning to m.dataset) to first compare newDS with m.dataset (or their
unique id) and if they are equal, skip clearing m.query via
m.query.SetValue(""), skip resetting
m.cacheDataset/m.cacheMetrics/m.cacheLabels/m.cacheValues, and avoid calling
fetchCacheMetrics; instead just close the picker (m.overlay = overlayNone),
clear/blur spotlightFilter and call m.focusSelected() and return m, nil (or
no-op result) so no editor state or cache is lost on a no-op re-selection.
In `@pkg/model/query.go`:
- Around line 439-449: The code only calls fetchStreamSchema when selected !=
m.dataset, leaving the initially preselected dataset without columns; update the
logic in the block that reads m.filteredDatasets and selected (around
m.datasetSelectedIdx) so that if selected == m.dataset but columns are not
populated (e.g., m.allColumns is empty) you still set m.selectedColumn = "";
m.allColumns = []string{}; m.filteredColumns = []string{}; m.columnsLoading =
true and append fetchStreamSchema(m.profile, selected) to cmds; keep the
existing behavior for when the dataset actually changes (assign m.dataset =
selected) and still fetch in both cases when columns are missing.
- Around line 1292-1296: resolveColumnPlaceholder currently uses
strings.ReplaceAll which replaces every substring "column" (breaking
identifiers/literals); change it to only replace the standalone placeholder
token by using a word-boundary regex, e.g. compile
regexp.MustCompile(`\bcolumn\b`) and call ReplaceAllString(query,
`"`+column+`"`), keeping the early return for empty column and leaving function
name resolveColumnPlaceholder unchanged.
In `@pkg/model/savedQueries.go`:
- Around line 333-340: The code treats filter.Query.FilterQuery as raw SQL in
pkg/model/savedQueries.go (used to build Item with desc:
*filter.Query.FilterQuery) but the CLI path in cmd/queryList.go still
JSON-marshals that field, causing quoted/"null" output; make the two paths
consistent by either (A) updating cmd/queryList.go to use the same raw-string
conversion as pkg/model/savedQueries.go (read filter.Query.FilterQuery as a
plain string, emit empty/skip when nil) or (B) extract a shared helper (e.g.,
ConvertFilterQueryToString) and call it from both locations; ensure you
reference and handle FilterQuery, Item, and filter.Query.FilterQuery so output
for `pb query list -o` emits unquoted SQL and no `"null"` values.
- Around line 251-269: The card widths are using m.width (and m.width-2) but not
accounting for the border and padding chrome; change mainCard's Width call to
subtract the total chrome (use Width(m.width-6) for mainCard) and change
footerInner's Width to m.width-4 (currently m.width-2) so the inner content plus
padding and border equal the intended overall m.width; update the Width
arguments on mainCard, footerInner (and any similar lipgloss NewStyle blocks in
this file) to subtract the correct chrome amounts.
In `@pkg/model/status.go`:
- Around line 105-123: Before composing row, ensure the right segment is trimmed
or dropped to avoid overflow: after computing innerW and contentW, compute
allowedRight = contentW - lipgloss.Width(left) - 1 (minimum 0); if
lipgloss.Width(right) > allowedRight then replace right with a truncated/dropped
version (empty string when allowedRight == 0 or a truncated string clipped to
allowedRight characters using rune-aware truncation or lipgloss.Truncate); then
recompute gap = contentW - lipgloss.Width(left) - lipgloss.Width(right) (ensure
gap >= 1) and build row = left + strings.Repeat(" ", gap) + right before
rendering into inner. This uses the existing symbols innerW, contentW, left,
right, gap, row, inner and preserves the existing lipgloss.Width checks.
In `@pkg/ui/app.go`:
- Line 89: The comment for SetError claims statusError is "cleared on next view
switch" but the view-switching code (the block that switches views around the
code that currently toggles/sets the active view at the location corresponding
to lines 174-177) does not clear statusError; either remove/update the comment
or implement clearing: locate the statusError variable and SetError function
and, in the routine that performs view switches (the function/method that
changes the active view around the 174-177 region), reset statusError (e.g., set
it to empty/zero value) and trigger the UI status redraw/update so the status
bar is actually cleared on view change; ensure tests or callers of SetError
behave consistently after this change.
In `@pkg/ui/chrome.go`:
- Around line 404-415: The truncate function uses byte slicing which can split
multibyte runes and miscount visible cell width; update truncate(s string, n
int) to iterate runes (or use a rune-aware builder) and accumulate visible cell
width (via lipgloss.Width or github.com/mattn/go-runewidth) to produce a
substring that fits within n (returning "…" when n<=1) and then append the
ellipsis without cutting a rune; also align the similar logic in
pkg/ui/views/picker.go (replace uses of len(s) and s[:n]) with the same
rune/cell-width-aware truncation routine (reuse truncate or a shared helper) so
all truncation is UTF‑8 and cell‑width safe.
In `@pkg/ui/icons.go`:
- Line 77: activeIcons is always set to asciiIcons, so PB_ICONS env is ignored;
change the package initialization to read os.Getenv("PB_ICONS")
(case-insensitive) in the init function and set activeIcons accordingly (e.g.,
if value == "nerd" set activeIcons = nerdIcons, else default to asciiIcons).
Locate the assignment to activeIcons and replace it with a small switch/if that
checks the PB_ICONS value and assigns the matching icon slice (using the
existing identifiers activeIcons, asciiIcons, nerdIcons) while keeping
asciiIcons as the fallback.
In `@pkg/ui/views/picker.go`:
- Line 370: Replace the unbounded io.ReadAll(resp.Body) call by reading from
io.LimitReader(resp.Body, <MAX_BYTES>) to cap the response size (e.g. 10<<20 for
10MiB), update the `body` variable assignment to read from that limited reader,
and handle the case where the response is larger than the limit (return an error
or a clear log) instead of silently truncating; make changes where
io.ReadAll(resp.Body) is used in pkg/ui/views/picker.go so that resp.Body is
wrapped with io.LimitReader and errors from reading are properly checked and
propagated.
---
Outside diff comments:
In `@pkg/model/promql.go`:
- Around line 399-407: The initial PromQL fetch can run with an empty m.dataset;
change the startup ordering so dataset discovery happens before any query fetch
and only enqueue NewPromqlFetchTask when a dataset is known: call
fetchMetricDatasets(m.profile) first, then (if m.dataset != "") append
fetchCacheMetrics(m.profile, m.dataset) and only then append
NewPromqlFetchTask(m.profile, m.query.Value(), m.dataset, ...); ensure Init() no
longer dispatches NewPromqlFetchTask when m.dataset is empty.
- Around line 1926-1929: The code directly interpolates the label `value` into a
PromQL quoted matcher; modify the logic that builds `{label="%s"}` to first
escape `value` according to PromQL string-literal rules (at minimum replace
`\`→`\\`, `"`→`\"`, and control characters like newline/tab → `\n`/`\t`), then
use the escaped string in fmt.Sprintf; keep the existing special-case branch for
empty or "(any)" values that returns `%s{%s!=""}` and only apply escaping for
the quoted matcher path (the variables to change are `value`, and the
fmt.Sprintf call that currently produces `%s{%s="%s"}`).
In `@pkg/model/timerange.go`:
- Around line 33-49: The selectable durations slice timeDurations omits the
FiveMinute constant, so add a timeDurationItem with duration: FiveMinute and an
appropriate repr (e.g., "5 Minutes") to the timeDurations slice so users can
select it; update the slice near the timeDurationItem entries (reference
symbols: FiveMinute, timeDurations, timeDurationItem) to include this new item
in the desired order.
---
Nitpick comments:
In `@pkg/ui/app.go`:
- Around line 158-164: The comment for the "esc" key handler is incorrect: it
claims Esc on Help / Time / Picker returns to the previous view but the code
only handles ViewHelp (in the switch case "esc" using a.current and ViewHelp).
Either update the comment to state Esc currently only returns to ViewQuery from
ViewHelp, or implement the missing branches: detect a.current == ViewTime and
a.current == ViewPicker and set a.current = ViewQuery (or restore the actual
previous view if that logic exists elsewhere). Modify the case "esc" block
around the a.current checks (ViewHelp, ViewTime, ViewPicker) and ensure the
behavior and comment are consistent.
In `@pkg/ui/theme.go`:
- Around line 222-225: SetActive currently updates Active and ActiveType (via
NewTypography) but doesn't clear the cached syntax highlighter style pbStyle,
causing stale colors; update SetActive to invalidate the cached style by
resetting pbStyle (e.g., set pbStyle to nil or call whatever reset function
exists) whenever Active is changed so subsequent calls recreate the style for
the new palette.
In `@pkg/ui/views/picker.go`:
- Around line 99-109: Update the existing TODO about dual-purpose "p" key into a
concrete follow-up task: create an issue or task in the project tracker to
implement a dedicated pin hotkey (e.g., Ctrl+P) and reference that issue ID in
the inline comment above the pin handling block (the code manipulating v.cursor,
v.filtered, v.pinned and calling v.filter). In the comment replace the vague
TODO with a short note like "TODO: replace literal 'p' command with Ctrl+P (see
ISSUE-###)" so maintainers can find the planned change and so reviewers know the
plan.
- Around line 113-116: The current printable check only allows ASCII bytes
(0x20-0x7F) and rejects multi-byte UTF-8 input; change the input handling in the
picker input branch so it validates runes using unicode.IsPrint before appending
to v.query and calling v.filter(), e.g., iterate runes of the incoming string s,
ensure each rune passes unicode.IsPrint (or otherwise accept the whole string if
all runes printable), then append s to v.query and call v.filter(); also add the
required import for the unicode package. This touches the logic around v.query
and v.filter() in the picker input handler.
🪄 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: 1a4e6473-b299-4ad9-a6aa-4e8ece919d99
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (24)
.gitignorecmd/queryList.gocmd/style.gogo.modpkg/model/credential/credential.gopkg/model/defaultprofile/profile.gopkg/model/login/login.gopkg/model/promql.gopkg/model/query.gopkg/model/role/role.gopkg/model/savedQueries.gopkg/model/status.gopkg/model/timeinput.gopkg/model/timerange.gopkg/ui/app.gopkg/ui/chart.gopkg/ui/chrome.gopkg/ui/highlight.gopkg/ui/icons.gopkg/ui/logo.gopkg/ui/theme.gopkg/ui/views/empty.gopkg/ui/views/picker.gopkg/ui/views/views.go
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/model/promql.go (1)
397-405:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid the infinite loading state when an expression starts without a dataset.
NewPromqlModelstill treats a non-empty expression as an in-flight query, butInit()now skipsNewPromqlFetchTaskuntilm.datasetis set. When the dataset list later auto-selects the first dataset, this branch only warms the cache, so the view can sit onfetching...forever. Either keeploading/hasQueriedfalse until a dataset exists, or batch the initial fetch when the auto-selection happens.Also applies to: 452-456
🤖 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 `@pkg/model/promql.go` around lines 397 - 405, Init currently starts a fetch cycle when a query exists even if m.dataset is empty, causing a permanent "fetching..." state; update PromqlModel.Init to NOT mark loading/hasQueried or enqueue NewPromqlFetchTask unless m.dataset is non-empty, and likewise change the logic around the auto-selection branch (the block around lines 452-456) so that when the dataset is later auto-selected it explicitly enqueues NewPromqlFetchTask (or sets loading/hasQueried and then enqueues) instead of only warming cache; reference PromqlModel.Init, NewPromqlFetchTask, fetchCacheMetrics, and the loading/hasQueried fields when making the change.
♻️ Duplicate comments (3)
pkg/ui/app.go (1)
125-177:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClear
statusErroron direct view switches too.Line 175 only clears the banner when a view changes via
ctx.nextView. The1-7shortcuts and theeschelp exit still switcha.currentdirectly, so stale errors survive those view changes even thoughSetErrorsays they clear on the next switch.🛠️ Proposed fix
func (a App) Update(msg tea.Msg) (tea.Model, tea.Cmd) { + switchView := func(v ViewID) (tea.Model, tea.Cmd) { + a.current = v + a.ctx.statusError = "" + return a, nil + } + switch m := msg.(type) { case tea.WindowSizeMsg: a.width = m.Width @@ switch m.String() { case "ctrl+c": return a, tea.Quit case "1": - a.current = ViewQuery - return a, nil + return switchView(ViewQuery) case "2": - a.current = ViewResults - return a, nil + return switchView(ViewResults) @@ case "?", "7": - a.current = ViewHelp - return a, nil + return switchView(ViewHelp) case "esc": if a.current == ViewHelp { - a.current = ViewQuery - return a, nil + return switchView(ViewQuery) } } } @@ if a.ctx.nextView != nil { - a.current = *a.ctx.nextView + a.current = *a.ctx.nextView a.ctx.nextView = nil a.ctx.statusError = "" }🤖 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 `@pkg/ui/app.go` around lines 125 - 177, The Update method sets a.current directly for key shortcuts (cases "1".."6", "7"/"?", and the "esc" help-exit branch) but only clears a.ctx.statusError when a view change comes from ctx.nextView; modify the key-handling branches in App.Update so that whenever you assign a.current (e.g., in the cases for "1".."7", "?", and the esc branch that sets a.current = ViewQuery) you also clear a.ctx.statusError (set to "") so stale errors are removed on direct switches; keep the existing ctx.nextView clearing logic unchanged.pkg/ui/views/picker.go (1)
361-374:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle failed and oversized dataset reads explicitly.
Line 366 caps memory, but the read error is still discarded and an oversized body is still silently truncated. That turns transport failures or over-limit responses into misleading unmarshal/HTTP errors instead of a clear fetch failure.
In Go, does io.ReadAll(io.LimitReader(r, n)) return an error when the source exceeds n bytes, or does it stop cleanly at n bytes?🛠️ Proposed fix
- body, _ := io.ReadAll(io.LimitReader(resp.Body, 10*1024*1024)) + const maxBody = 10 << 20 + lr := &io.LimitedReader{R: resp.Body, N: maxBody + 1} + body, err := io.ReadAll(lr) + if err != nil { + return datasetListMsg{err: err.Error()} + } + if len(body) > maxBody { + return datasetListMsg{err: "dataset list exceeds 10 MiB"} + }🤖 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 `@pkg/ui/views/picker.go` around lines 361 - 374, The code currently discards the error from io.ReadAll(io.LimitReader(resp.Body, 10*1024*1024)) and silently truncates oversized responses which masks transport/read failures and produces misleading unmarshal/HTTP errors; update the HTTP fetch block around client.Do(req) and the subsequent read so that you (1) check and return the error from ReadAll instead of ignoring it, (2) detect when the response was truncated by comparing the number of bytes read to the 10*1024*1024 limit and return a clear datasetListMsg error like "response too large" (or include the full Content-Length/limit), and (3) preserve the existing HTTP status handling and json.Unmarshal error behavior (references: client.Do(req), resp.Body, io.LimitReader(..., 10*1024*1024), truncate, and datasetListMsg) so callers get an explicit fetch/read error rather than an ambiguous unmarshal/HTTP error.pkg/model/savedQueries.go (1)
251-269:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse inner widths for the bordered cards.
Line 255 still overshoots the available body width.
m.listis sized tomsg.Width - 6, butmainCard.Width(m.width - 2)adds 4 cells of horizontal padding and 2 cells of border on top of that, so the right edge can still clip. The footer has the same problem withfooterInner.Width(m.width-2).In lipgloss v0.12.1, does Style.Width(n) set the inner content width before horizontal padding and borders are added to the final rendered width?🛠️ Proposed fix
mainCard := lipgloss.NewStyle(). Border(lipgloss.NormalBorder()). BorderForeground(p.Border). Padding(1, 2). - Width(m.width - 2). + Width(m.width - 6). Render(mainInner) @@ footerInner := lipgloss.NewStyle(). - Width(m.width-2). + Width(m.width-4). Padding(0, 1). Render(hintRow)🤖 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 `@pkg/model/savedQueries.go` around lines 251 - 269, The bordered card widths are using outer widths so their rendered boxes overflow; adjust mainCard and footerInner to use the inner content width by subtracting the horizontal padding (2 left + 2 right = 4) and the border characters (1 left + 1 right = 2) from m.width, e.g. replace mainCard.Width(m.width - 2) and footerInner.Width(m.width - 2) with mainCard.Width(m.width - 8) and footerInner.Width(m.width - 8) (so the inner width matches m.list which is sized to msg.Width - 6), ensuring the final rendered width doesn't clip.
🤖 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 `@pkg/model/query.go`:
- Around line 1306-1315: The current resolveColumnPlaceholder rewrites any
standalone "column" token; change it to match a dedicated sentinel like
"__pb_selected_column__" instead: update the regexp variable
(columnPlaceholderRE) to match that exact token and update
resolveColumnPlaceholder to only replace that sentinel with the quoted, escaped
column name (use the same escaping logic). Also update the column picker
insertion site to insert the same sentinel token (not the plain "column") so
editor text and executed SQL remain identical; reference columnPlaceholderRE and
resolveColumnPlaceholder when making these edits.
---
Outside diff comments:
In `@pkg/model/promql.go`:
- Around line 397-405: Init currently starts a fetch cycle when a query exists
even if m.dataset is empty, causing a permanent "fetching..." state; update
PromqlModel.Init to NOT mark loading/hasQueried or enqueue NewPromqlFetchTask
unless m.dataset is non-empty, and likewise change the logic around the
auto-selection branch (the block around lines 452-456) so that when the dataset
is later auto-selected it explicitly enqueues NewPromqlFetchTask (or sets
loading/hasQueried and then enqueues) instead of only warming cache; reference
PromqlModel.Init, NewPromqlFetchTask, fetchCacheMetrics, and the
loading/hasQueried fields when making the change.
---
Duplicate comments:
In `@pkg/model/savedQueries.go`:
- Around line 251-269: The bordered card widths are using outer widths so their
rendered boxes overflow; adjust mainCard and footerInner to use the inner
content width by subtracting the horizontal padding (2 left + 2 right = 4) and
the border characters (1 left + 1 right = 2) from m.width, e.g. replace
mainCard.Width(m.width - 2) and footerInner.Width(m.width - 2) with
mainCard.Width(m.width - 8) and footerInner.Width(m.width - 8) (so the inner
width matches m.list which is sized to msg.Width - 6), ensuring the final
rendered width doesn't clip.
In `@pkg/ui/app.go`:
- Around line 125-177: The Update method sets a.current directly for key
shortcuts (cases "1".."6", "7"/"?", and the "esc" help-exit branch) but only
clears a.ctx.statusError when a view change comes from ctx.nextView; modify the
key-handling branches in App.Update so that whenever you assign a.current (e.g.,
in the cases for "1".."7", "?", and the esc branch that sets a.current =
ViewQuery) you also clear a.ctx.statusError (set to "") so stale errors are
removed on direct switches; keep the existing ctx.nextView clearing logic
unchanged.
In `@pkg/ui/views/picker.go`:
- Around line 361-374: The code currently discards the error from
io.ReadAll(io.LimitReader(resp.Body, 10*1024*1024)) and silently truncates
oversized responses which masks transport/read failures and produces misleading
unmarshal/HTTP errors; update the HTTP fetch block around client.Do(req) and the
subsequent read so that you (1) check and return the error from ReadAll instead
of ignoring it, (2) detect when the response was truncated by comparing the
number of bytes read to the 10*1024*1024 limit and return a clear datasetListMsg
error like "response too large" (or include the full Content-Length/limit), and
(3) preserve the existing HTTP status handling and json.Unmarshal error behavior
(references: client.Do(req), resp.Body, io.LimitReader(..., 10*1024*1024),
truncate, and datasetListMsg) so callers get an explicit fetch/read error rather
than an ambiguous unmarshal/HTTP error.
🪄 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: 49d46a1e-0345-4301-9af8-0ef5aed315f2
📒 Files selected for processing (12)
cmd/queryList.gopkg/model/promql.gopkg/model/promql_test.gopkg/model/query.gopkg/model/query_test.gopkg/model/savedQueries.gopkg/model/timerange.gopkg/ui/app.gopkg/ui/chrome.gopkg/ui/icons.gopkg/ui/theme.gopkg/ui/views/picker.go
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/ui/app.go (1)
195-225:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winNil pointer dereference if view not in map.
Update()(line 176) checks for nil view before use, butView()does not. Ifa.views[a.current]returns nil (missing entry),view.HeaderKeys()at line 203 andview.Render()at line 225 will panic.Proposed fix to add nil guard
// ── Top chrome: HeaderStrip with context KVs + view-aware keybinds + logo view := a.views[a.current] + if view == nil { + return lipgloss.NewStyle().Foreground(Active.Err).Render( + fmt.Sprintf("view %q not registered", a.current.Label())) + } ctxKVs := []KV{🤖 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 `@pkg/ui/app.go` around lines 195 - 225, a.views[a.current] can be nil causing panics at view.HeaderKeys() and view.Render(); after fetching view := a.views[a.current] add a nil-guard and use a safe fallback (either a known default view from a.views like a.views[ViewQuery] if present, or a lightweight no-op view that implements HeaderKeys() returning nil/defaultKeyHints() and Render() returning an empty body) so HeaderKeys() and Render() are never called on nil; update the same guard pattern used in Update()/View() to protect HeaderStrip(a.width,...), Breadcrumbs/status calculation, and body := view.Render(...) by ensuring view is non-nil before those calls.pkg/model/status.go (1)
121-124:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRefine gap calculation to avoid forcing spacing when content overflows.
After clearing
right(line 119), ifleftalone exceedscontentW, the gap becomes negative and is clamped to 1. This forces an extra space that doesn't fit, wasting a character whenlipgloss.Width(innerW)truncates the row. The 1-character minimum should only apply when both segments are present and fit.📏 Proposed fix to adjust gap logic
+ // Calculate gap; only enforce spacing when both segments are present gap := contentW - lipgloss.Width(left) - lipgloss.Width(right) - if gap < 1 { + if right != "" && gap < 1 { gap = 1 + } else if gap < 0 { + gap = 0 }🤖 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 `@pkg/model/status.go` around lines 121 - 124, The gap calculation currently clamps negative gaps to 1 unconditionally, causing an extra space when only left exists or content overflows; update the logic around variables gap, left, right, and contentW in pkg/model/status.go so that you only enforce a minimum gap (1) when both left and right are non-empty and their combined widths fit within contentW (use lipgloss.Width(left) + lipgloss.Width(right) <= contentW), otherwise allow gap to be zero or negative (so truncation via lipgloss.Width(innerW) can occur) and avoid inserting the extra space.
🧹 Nitpick comments (2)
pkg/model/status.go (1)
39-47: 💤 Low valueConsider extracting the default mode label as a named constant.
The default title
"parseable"is hardcoded here. Extracting it as a package-level constant (e.g.,defaultModeLabel) would improve maintainability if this value is referenced elsewhere or needs to change.♻️ Optional refactor
+const defaultModeLabel = "parseable" + func NewStatusBar(host string, width int) StatusBar { return StatusBar{ - title: "parseable", + title: defaultModeLabel, host: host, Info: "", Error: "",🤖 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 `@pkg/model/status.go` around lines 39 - 47, Extract the hardcoded default title "parseable" into a package-level constant (e.g., defaultModeLabel) and use that constant in NewStatusBar when initializing the StatusBar.title field; update the declaration near the top of the pkg/model package so NewStatusBar, the StatusBar struct initialization, and any future references use defaultModeLabel instead of the literal string.pkg/model/promql.go (1)
456-466: 💤 Low valueVerify this auto-execution behavior is intentional.
When no dataset was previously selected (or placeholder was set), and the first available dataset is auto-selected, this code auto-executes any pre-existing query. While this could streamline the UX, it might also surprise users who typed a query intending to pick a specific dataset first.
If this is intentional UX behavior, consider adding a brief comment explaining why auto-execution on initial dataset selection is desirable.
🤖 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 `@pkg/model/promql.go` around lines 456 - 466, The code in promql.go currently auto-executes a pre-existing query when a dataset becomes populated (see the block using m.query.Value(), m.loading, m.hasQueried, fetchCacheMetrics and NewPromqlFetchTask), which can be surprising; either document this intentional UX by adding a concise comment above that block explaining why auto-execution on initial dataset selection is desired, or change the guard so auto-execution only happens when the dataset was explicitly chosen (e.g., add/inspect a flag like m.datasetWasSelected or require !m.placeholderDataset before setting m.loading/m.hasQueried and returning the Batch with NewPromqlFetchTask) so that typing a query without choosing a dataset does not trigger fetches.
🤖 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.
Outside diff comments:
In `@pkg/model/status.go`:
- Around line 121-124: The gap calculation currently clamps negative gaps to 1
unconditionally, causing an extra space when only left exists or content
overflows; update the logic around variables gap, left, right, and contentW in
pkg/model/status.go so that you only enforce a minimum gap (1) when both left
and right are non-empty and their combined widths fit within contentW (use
lipgloss.Width(left) + lipgloss.Width(right) <= contentW), otherwise allow gap
to be zero or negative (so truncation via lipgloss.Width(innerW) can occur) and
avoid inserting the extra space.
In `@pkg/ui/app.go`:
- Around line 195-225: a.views[a.current] can be nil causing panics at
view.HeaderKeys() and view.Render(); after fetching view := a.views[a.current]
add a nil-guard and use a safe fallback (either a known default view from
a.views like a.views[ViewQuery] if present, or a lightweight no-op view that
implements HeaderKeys() returning nil/defaultKeyHints() and Render() returning
an empty body) so HeaderKeys() and Render() are never called on nil; update the
same guard pattern used in Update()/View() to protect HeaderStrip(a.width,...),
Breadcrumbs/status calculation, and body := view.Render(...) by ensuring view is
non-nil before those calls.
---
Nitpick comments:
In `@pkg/model/promql.go`:
- Around line 456-466: The code in promql.go currently auto-executes a
pre-existing query when a dataset becomes populated (see the block using
m.query.Value(), m.loading, m.hasQueried, fetchCacheMetrics and
NewPromqlFetchTask), which can be surprising; either document this intentional
UX by adding a concise comment above that block explaining why auto-execution on
initial dataset selection is desired, or change the guard so auto-execution only
happens when the dataset was explicitly chosen (e.g., add/inspect a flag like
m.datasetWasSelected or require !m.placeholderDataset before setting
m.loading/m.hasQueried and returning the Batch with NewPromqlFetchTask) so that
typing a query without choosing a dataset does not trigger fetches.
In `@pkg/model/status.go`:
- Around line 39-47: Extract the hardcoded default title "parseable" into a
package-level constant (e.g., defaultModeLabel) and use that constant in
NewStatusBar when initializing the StatusBar.title field; update the declaration
near the top of the pkg/model package so NewStatusBar, the StatusBar struct
initialization, and any future references use defaultModeLabel instead of the
literal string.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0283945a-b50c-4902-8035-485335d6f45d
📒 Files selected for processing (5)
pkg/model/promql.gopkg/model/query.gopkg/model/status.gopkg/ui/app.gopkg/ui/views/picker.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@pkg/ui/app.go`:
- Around line 195-198: The fallback to a.views[ViewQuery] in the render path can
still be nil and cause a panic; update either NewApp to validate and initialize
required views (ensuring a.views[ViewQuery] and a.views entries referenced by
a.current are non-nil) or add a second nil-guard where view is selected in the
rendering code (check view after assigning a.views[ViewQuery] and return an
error or use a safe default) so that view is never dereferenced when nil (refer
to a.views, a.current, ViewQuery and NewApp to locate the logic to change).
🪄 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: b99a365c-7bc2-4a2a-9242-cb7514d79f13
📒 Files selected for processing (2)
pkg/model/status.gopkg/ui/app.go
nitisht
approved these changes
May 25, 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
pkg/ui/package to centralize all shared UI components and styling.HeaderStrip,Card,Breadcrumbs,Pill,SelectionRail, charts, icons, and logo support.SQL Query TUI (
pkg/model/query.go)Ctrl+D/Enter) for quick dataset selection.SELECT * FROM dataset LIMIT 100when the editor is empty.columnplaceholder, resolved to the actual column name during query execution.PromQL TUI (
pkg/model/promql.go)pkg/uicomponents and theme system.Refactor
lipglossstyles from model files and replaced them with shared UI primitives.Other Changes
.gitignoreto ignorevendor/directory.Test Plan
make buildpassesgo test ./...passesmake lint/make verifierspassSummary by CodeRabbit
New Features
Style
Behavior
Tests