Conversation
|
Warning Rate limit exceeded
⌛ 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. WalkthroughThe PR introduces table provider infrastructure with ColumnDef/ColumnBuilder fluent API and TableProvider interface for structured data rendering, adds Bubble Tea-based task UI with taskModel replacing legacy rendering, enhances reflection-based type handling in TryTypedValue and ParseDataWithSchema, adds TextTable JSON/YAML marshalling support, and updates Charmbracelet dependencies from legacy lipgloss to bubbletea-based stack. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Mgr as Manager
participant BubbleTea as Bubble Tea Program
participant Model as taskModel
App->>Mgr: Create/Initialize
Mgr->>BubbleTea: startProgram()
BubbleTea->>Model: Create taskModel
Model->>Model: Init() - start ticker
loop Update Loop (each tick)
BubbleTea->>Model: Update(tickMsg)
Model->>Mgr: manager.Pretty()
Mgr-->>Model: render output
Model->>Model: View() - format for display
BubbleTea->>BubbleTea: Render to screen
end
App->>Mgr: WaitFor() or shutdown
Mgr->>BubbleTea: stopProgram()
BubbleTea->>Model: Update(shutdownMsg)
Model->>Model: View() - renderFinalView()
BubbleTea->>App: Program exits
sequenceDiagram
participant Caller as Caller
participant API as api.NewTableFrom
participant First as First Item (TableProvider)
participant Items as Slice Items
participant Builder as ColumnDef/Row
Caller->>API: NewTableFrom([]T items)
API->>First: Get first item
First->>Builder: Columns() []ColumnDef
Builder-->>API: Column metadata
API->>Builder: Row() map[string]any
Builder-->>API: Field values
API->>API: Build Headers and FieldNames
loop For each item
API->>Items: Get item
Items->>Builder: Row() for item
Builder-->>API: Value map
API->>API: convertToTextable(value)
API->>API: Build TypedValue row
end
API-->>Caller: TextTable with rows/columns
Possibly related PRs
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
formatters/html_formatter.go (1)
133-140: Guard against nil PrettyData before IsEmpty.
If ToPrettyData ever returns (nil, nil), Line 139 panics. Safer to handle nil explicitly.✅ Suggested fix
- if data.IsEmpty() { + if data == nil || data.IsEmpty() { return "", nil }formatters/csv_formatter.go (1)
62-83: Guard against empty headers to avoid blank CSV output.Line 72 writes headers even when none exist, producing a blank header row and potentially empty/misaligned data. Consider returning an error when headers are missing.
🐛 Proposed fix
func (f *CSVFormatter) FormatTable(table *api.TextTable) (string, error) { if table == nil { return "", nil } + if len(table.Headers) == 0 { + return "", fmt.Errorf("no headers defined") + } var output strings.Builder writer := csv.NewWriter(&output)task/manager.go (1)
277-401: Prevent races/double-close when stopping the plain render loop.Line 282 starts the goroutine before the stop channels exist, and Line 365 closes the channel while
stopProgramis called from multiple places (Lines 627/676). This can leak the loop or panic on double-close. Initialize channels before spawning the goroutine and makestopProgramidempotent by swapping channel refs once.🐛 Proposed fix (idempotent shutdown + pre-init channels)
- } else { - // Non-interactive mode: start a plain render loop - go tm.plainRenderLoop() - } + } else { + // Non-interactive mode: start a plain render loop + tm.stopPlainRender = make(chan struct{}) + tm.plainRenderDone = make(chan struct{}) + go tm.plainRenderLoop(tm.stopPlainRender, tm.plainRenderDone) + } }-func (tm *Manager) stopProgram() { - // Stop plain render loop if running - if tm.stopPlainRender != nil { - close(tm.stopPlainRender) - <-tm.plainRenderDone - tm.stopPlainRender = nil - } +func (tm *Manager) stopProgram() { + // Stop plain render loop if running (idempotent) + tm.mu.Lock() + stopCh := tm.stopPlainRender + doneCh := tm.plainRenderDone + tm.stopPlainRender = nil + tm.plainRenderDone = nil + tm.mu.Unlock() + if stopCh != nil { + close(stopCh) + if doneCh != nil { + <-doneCh + } + } if tm.program == nil { return }-func (tm *Manager) plainRenderLoop() { - tm.stopPlainRender = make(chan struct{}) - tm.plainRenderDone = make(chan struct{}) - defer close(tm.plainRenderDone) +func (tm *Manager) plainRenderLoop(stopCh <-chan struct{}, doneCh chan<- struct{}) { + defer close(doneCh) @@ - case <-tm.stopPlainRender: + case <-stopCh: tm.PlainRender() returnAlso applies to: 627-676
examples/uber_demo/main.go (1)
561-584: Fix GoDoc comment to match method name.Line 573 says “Rows implements …” but the method is
Row, which can mislead GoDoc readers.✏️ Comment fix
-// Rows implements api.TableProvider - returns raw data for this item +// Row implements api.TableProvider - returns raw data for this item
🤖 Fix all issues with AI agents
In `@api/meta.go`:
- Around line 39-45: PrettyData.IsEmpty currently only checks Schema, Table,
Tree, and TypedMap, ignoring other payload types; update the IsEmpty method
(PrettyData.IsEmpty) to also consider Textable, Slice, Map, and TypedList (and
any fields set by TryTypedValue) as non-empty when present — i.e., return false
if any of those fields are non-nil or contain data, otherwise return true;
ensure the nil/empty checks reference the exact field names Textable, Slice,
Map, TypedList and preserve the existing checks for Schema, Table, Tree, and
TypedMap.
In `@formatters/parser.go`:
- Around line 624-633: The slice handling currently falls through to
ParseStructSchema when both opt.SkipTable and opt.SkipTree are true (or when
SkipTable=true and no tree structure), which can cause confusing errors; update
the slice branch in the function using FlattenSlice, hasTreeStructure,
convertSliceToTreeData and convertSliceToPrettyData to detect when no output
path was chosen (i.e., opt.SkipTree && opt.SkipTable or opt.SkipTable &&
!hasTreeStructure(val)) and return a clear error (or an explicit fallback)
instead of falling through to ParseStructSchema—ensure the check is placed
before calling convertSliceToTreeData/convertSliceToPrettyData and reference
ParseStructSchema in the error message so callers know why parsing failed.
In `@formatters/reflect.go`:
- Around line 59-72: FlattenSlice currently returns an empty slice of the input
type when flattened is empty, causing a mismatch (returning [][]T vs []T);
change the empty-case to construct and return an empty slice of the inner
element type instead. In FlattenSlice, when len(flattened)==0, derive the inner
element type from val.Type(): if val.Type().Kind()==reflect.Slice and
val.Type().Elem().Kind()==reflect.Slice use val.Type().Elem().Elem() (otherwise
use val.Type().Elem()), then return reflect.MakeSlice(reflect.SliceOf(elemType),
0, 0) so the empty path and the normal path both return []T; update references
to flattened, val and newSlice accordingly.
In `@formatters/tests/sorting_test.go`:
- Line 11: The test was disabled by renaming the function to XTestSortRows;
restore it to a proper test name (e.g., TestSortRows) so the test runs, or if it
must remain skipped intentionally, call t.Skip("reason or ticket #") inside
TestSortRows with an explicit reason; update the function name and/or add the
t.Skip call in the TestSortRows function to re-enable correct test discovery and
document why it's skipped.
In `@formatters/tests/tree_test.go`:
- Line 128: The test was disabled by renaming it to XTestCustomRenderFunction so
it won't run; restore test execution by renaming the function back to
TestCustomRenderFunction so the test runner includes it, or if you intend to
keep it skipped, call t.Skip with a clear reason/ticket inside
XTestCustomRenderFunction (e.g., t.Skip("disabled: <reason or ticket>")) to make
the skip explicit and documented.
In `@task/model.go`:
- Around line 81-88: renderFinalView currently returns an empty string so the
last frame is lost; change it to produce the manager's Pretty() output instead:
call m.manager.Pretty(), check m.manager.noColor.Load() to decide between
rendered.String() (when noColor) and rendered.ANSI() (when colored), and return
that string—update the renderFinalView method to return the final rendered view
using these existing symbols (m.manager.Pretty, m.manager.noColor.Load,
rendered.String, rendered.ANSI).
🧹 Nitpick comments (7)
api/text.go (1)
217-241: Non-deterministic style ordering due to map iteration.The
uniqueStylesfunction builds the result string by iterating over a map (lines 232-237), which produces non-deterministic ordering in Go. While functionally correct for CSS (class order doesn't affect styling), this can cause:
- Flaky tests when comparing style strings
- Inconsistent output making debugging harder
- Snapshot test failures
Consider sorting the styles for deterministic output:
♻️ Proposed fix for deterministic ordering
func uniqueStyles(existing string, styles ...string) string { styleSet := make(map[string]struct{}) if existing != "" { for _, s := range strings.Split(existing, " ") { styleSet[s] = struct{}{} } } for _, style := range styles { for _, s := range strings.Split(style, " ") { if s != "" { styleSet[s] = struct{}{} } } } - uniq := "" - for s := range styleSet { - if uniq == "" { - uniq = s - } else { - uniq += " " + s - } - } - - return uniq + // Sort for deterministic output + result := make([]string, 0, len(styleSet)) + for s := range styleSet { + result = append(result, s) + } + sort.Strings(result) + return strings.Join(result, " ") }Note: The
sortpackage is already imported in this file (line 6).formatters/reflect.go (1)
207-214: RedundantToLowercall beforeEqualFold.
strings.EqualFoldalready performs case-insensitive comparison, so callingstrings.ToLoweron line 208 is unnecessary.♻️ Suggested fix
// Try case-insensitive match - lowerName := strings.ToLower(name) for i := 0; i < typ.NumField(); i++ { field := typ.Field(i) - if strings.EqualFold(field.Name, lowerName) { + if strings.EqualFold(field.Name, name) { return val.Field(i) } }task/render_test.go (1)
44-59: Missingr.Close()for the read end of the pipe.The read end of the pipe (
r) is never closed. While the test completes becauseio.Copyreturns when the write end is closed, explicitly closing the read end is good practice to avoid potential resource leaks.♻️ Suggested fix
// Close write end and wait for reader to finish w.Close() <-done + r.Close() // Restore stderr os.Stderr = originalStderrThis same pattern applies to
TestPlainRenderOnlyDirtyTasksat lines 289-291.api/column_test.go (1)
145-170: Test doesn’t validate hidden-column behavior.
The current block only checks mockEmployee defaults. Consider asserting that a hidden column is excluded from Headers/FieldNames/Rows.✅ Example improvement
It("excludes hidden columns", func() { - type hiddenEmployee struct { - ID int - Name string - } - - // Create a local type with hidden column - employees := []struct { - ID int - Name string - }{{ID: 1, Name: "Alice"}} - - // Use a wrapper that implements TableProvider - type wrapperEmployee struct { - ID int - Name string - } - - // Since we can't easily test hidden columns without modifying the mock, - // we verify that our mockEmployee doesn't have hidden columns - cols := mockEmployee{}.Columns() - for _, col := range cols { - Expect(col.Hidden).To(BeFalse(), "no columns should be hidden in mockEmployee") - } - - _ = employees // suppress unused warning + type hiddenEmployee struct { + ID int + Name string + } + func (hiddenEmployee) Columns() []ColumnDef { + return []ColumnDef{ + Column("id").Label("ID").Hidden().Build(), + Column("name").Label("Name").Build(), + } + } + func (e hiddenEmployee) Row() map[string]any { + return map[string]any{"id": e.ID, "name": e.Name} + } + + table := NewTableFrom([]hiddenEmployee{{ID: 1, Name: "Alice"}}) + Expect(table.Headers).To(HaveLen(1)) + Expect(table.FieldNames).To(Equal([]string{"name"})) })formatters/csv_formatter.go (1)
90-102: Clarify single-table behavior in the comment.Line 96 uses only
FirstTable, but the comment says “flattening all fields.” Consider updating the comment (or explicitly handling multiple tables) to avoid confusion.✏️ Comment fix
-// FormatPrettyData formats PrettyData as CSV, flattening all fields +// FormatPrettyData formats PrettyData as CSV using the first tableapi/column.go (1)
91-154: WireMaxWidthinto table columns (if supported) to avoid a dead option.
NewTableFrommaps mostColumnDeffields intoPrettyField, butMaxWidthis currently dropped. IfPrettyFieldsupports it, pass it through so the option has effect.♻️ Proposed wiring
table.Columns = append(table.Columns, PrettyField{ Name: col.Name, Label: col.DisplayLabel(), Style: col.Style, LabelStyle: col.HeaderStyle, Type: col.Type, Format: col.Format, FormatOptions: col.FormatOptions, + MaxWidth: col.MaxWidth, })examples/uber_demo/main.go (1)
755-867: Optional: surface task failures from the demo command.Line 781 ignores
task.Wait()’s exit code; returning an error on non‑zero would make failures visible in CLI runs.♻️ Suggested handling
- task.Wait() + if code := task.Wait(); code != 0 { + return nil, fmt.Errorf("tasks failed with exit code %d", code) + }
| // IsEmpty returns true if the PrettyData has no meaningful content | ||
| func (pd *PrettyData) IsEmpty() bool { | ||
| if pd == nil { | ||
| return true | ||
| } | ||
| return pd.Schema == nil && pd.Table == nil && pd.Tree == nil && pd.TypedMap == nil | ||
| } |
There was a problem hiding this comment.
PrettyData.IsEmpty ignores non-table content.
If PrettyData carries Textable/Slice/Map/TypedList (common via TryTypedValue), IsEmpty() currently returns true and can suppress valid output. Include those fields in the emptiness check.
✅ Proposed fix
func (pd *PrettyData) IsEmpty() bool {
if pd == nil {
return true
}
- return pd.Schema == nil && pd.Table == nil && pd.Tree == nil && pd.TypedMap == nil
+ return pd.Schema == nil &&
+ pd.Textable == nil &&
+ pd.Slice == nil &&
+ pd.Map == nil &&
+ pd.TypedMap == nil &&
+ pd.TypedList == nil &&
+ pd.Table == nil &&
+ pd.Tree == nil
}📝 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.
| // IsEmpty returns true if the PrettyData has no meaningful content | |
| func (pd *PrettyData) IsEmpty() bool { | |
| if pd == nil { | |
| return true | |
| } | |
| return pd.Schema == nil && pd.Table == nil && pd.Tree == nil && pd.TypedMap == nil | |
| } | |
| // IsEmpty returns true if the PrettyData has no meaningful content | |
| func (pd *PrettyData) IsEmpty() bool { | |
| if pd == nil { | |
| return true | |
| } | |
| return pd.Schema == nil && | |
| pd.Textable == nil && | |
| pd.Slice == nil && | |
| pd.Map == nil && | |
| pd.TypedMap == nil && | |
| pd.TypedList == nil && | |
| pd.Table == nil && | |
| pd.Tree == nil | |
| } |
🤖 Prompt for AI Agents
In `@api/meta.go` around lines 39 - 45, PrettyData.IsEmpty currently only checks
Schema, Table, Tree, and TypedMap, ignoring other payload types; update the
IsEmpty method (PrettyData.IsEmpty) to also consider Textable, Slice, Map, and
TypedList (and any fields set by TryTypedValue) as non-empty when present —
i.e., return false if any of those fields are non-nil or contain data, otherwise
return true; ensure the nil/empty checks reference the exact field names
Textable, Slice, Map, TypedList and preserve the existing checks for Schema,
Table, Tree, and TypedMap.
| val = FlattenSlice(val) | ||
| // Handle slices/arrays - default to table format unless items have tree structure | ||
| if val.Kind() == reflect.Slice || val.Kind() == reflect.Array { | ||
| if hasTreeStructure(val) { | ||
| if !opt.SkipTree && hasTreeStructure(val) { | ||
| return convertSliceToTreeData(val) | ||
| } | ||
| return convertSliceToPrettyData(val) | ||
| if !opt.SkipTable { | ||
| return convertSliceToPrettyData(val) | ||
| } | ||
| } |
There was a problem hiding this comment.
Guard slice inputs when both table and tree are skipped.
When SkipTable=true and SkipTree=true (or SkipTable=true with no tree structure), slice inputs fall through to ParseStructSchema on a slice, which is likely to error with a confusing message. Add an explicit fallback or error for this case.
🛠️ Proposed fix (explicit error)
if val.Kind() == reflect.Slice || val.Kind() == reflect.Array {
if !opt.SkipTree && hasTreeStructure(val) {
return convertSliceToTreeData(val)
}
if !opt.SkipTable {
return convertSliceToPrettyData(val)
}
+ return nil, fmt.Errorf("cannot format slice input when both SkipTable and SkipTree are set")
}📝 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.
| val = FlattenSlice(val) | |
| // Handle slices/arrays - default to table format unless items have tree structure | |
| if val.Kind() == reflect.Slice || val.Kind() == reflect.Array { | |
| if hasTreeStructure(val) { | |
| if !opt.SkipTree && hasTreeStructure(val) { | |
| return convertSliceToTreeData(val) | |
| } | |
| return convertSliceToPrettyData(val) | |
| if !opt.SkipTable { | |
| return convertSliceToPrettyData(val) | |
| } | |
| } | |
| val = FlattenSlice(val) | |
| // Handle slices/arrays - default to table format unless items have tree structure | |
| if val.Kind() == reflect.Slice || val.Kind() == reflect.Array { | |
| if !opt.SkipTree && hasTreeStructure(val) { | |
| return convertSliceToTreeData(val) | |
| } | |
| if !opt.SkipTable { | |
| return convertSliceToPrettyData(val) | |
| } | |
| return nil, fmt.Errorf("cannot format slice input when both SkipTable and SkipTree are set") | |
| } |
🤖 Prompt for AI Agents
In `@formatters/parser.go` around lines 624 - 633, The slice handling currently
falls through to ParseStructSchema when both opt.SkipTable and opt.SkipTree are
true (or when SkipTable=true and no tree structure), which can cause confusing
errors; update the slice branch in the function using FlattenSlice,
hasTreeStructure, convertSliceToTreeData and convertSliceToPrettyData to detect
when no output path was chosen (i.e., opt.SkipTree && opt.SkipTable or
opt.SkipTable && !hasTreeStructure(val)) and return a clear error (or an
explicit fallback) instead of falling through to ParseStructSchema—ensure the
check is placed before calling convertSliceToTreeData/convertSliceToPrettyData
and reference ParseStructSchema in the error message so callers know why parsing
failed.
| // If no elements were collected, return empty slice of same type as input | ||
| if len(flattened) == 0 { | ||
| return reflect.MakeSlice(val.Type(), 0, 0) | ||
| } | ||
|
|
||
| // Create a new slice with the flattened elements | ||
| // Determine the element type from the first flattened element | ||
| elemType := flattened[0].Type() | ||
| newSlice := reflect.MakeSlice(reflect.SliceOf(elemType), len(flattened), len(flattened)) | ||
| for i, elem := range flattened { | ||
| newSlice.Index(i).Set(elem) | ||
| } | ||
|
|
||
| return newSlice |
There was a problem hiding this comment.
Potential type inconsistency in FlattenSlice return value.
When the input is [][]T but all inner slices are empty, line 61 returns an empty slice of the input type ([][]T), while the normal path at line 67 returns a slice of the inner element type ([]T). This inconsistency could cause issues for callers expecting a consistent return type.
🐛 Suggested fix for consistent return type
// If no elements were collected, return empty slice of same type as input
if len(flattened) == 0 {
- return reflect.MakeSlice(val.Type(), 0, 0)
+ // Return empty slice of the inner element type for consistency
+ innerType := firstElem.Type().Elem()
+ return reflect.MakeSlice(reflect.SliceOf(innerType), 0, 0)
}🤖 Prompt for AI Agents
In `@formatters/reflect.go` around lines 59 - 72, FlattenSlice currently returns
an empty slice of the input type when flattened is empty, causing a mismatch
(returning [][]T vs []T); change the empty-case to construct and return an empty
slice of the inner element type instead. In FlattenSlice, when
len(flattened)==0, derive the inner element type from val.Type(): if
val.Type().Kind()==reflect.Slice and val.Type().Elem().Kind()==reflect.Slice use
val.Type().Elem().Elem() (otherwise use val.Type().Elem()), then return
reflect.MakeSlice(reflect.SliceOf(elemType), 0, 0) so the empty path and the
normal path both return []T; update references to flattened, val and newSlice
accordingly.
| ) | ||
|
|
||
| func TestSortRows(t *testing.T) { | ||
| func XTestSortRows(t *testing.T) { |
There was a problem hiding this comment.
Don’t disable tests via renaming.
Renaming to XTestSortRows silences this test and can mask regressions. Please re‑enable or use an explicit t.Skip with a reason/ticket.
✅ Suggested fix (re‑enable)
-func XTestSortRows(t *testing.T) {
+func TestSortRows(t *testing.T) {📝 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 XTestSortRows(t *testing.T) { | |
| func TestSortRows(t *testing.T) { |
🤖 Prompt for AI Agents
In `@formatters/tests/sorting_test.go` at line 11, The test was disabled by
renaming the function to XTestSortRows; restore it to a proper test name (e.g.,
TestSortRows) so the test runs, or if it must remain skipped intentionally, call
t.Skip("reason or ticket #") inside TestSortRows with an explicit reason; update
the function name and/or add the t.Skip call in the TestSortRows function to
re-enable correct test discovery and document why it's skipped.
| } | ||
|
|
||
| func TestCustomRenderFunction(t *testing.T) { | ||
| func XTestCustomRenderFunction(t *testing.T) { |
There was a problem hiding this comment.
Avoid disabling tests by renaming.
XTestCustomRenderFunction won’t run; this reduces coverage of the render registry. Re‑enable or explicitly t.Skip with a reason/ticket.
✅ Suggested fix (re‑enable)
-func XTestCustomRenderFunction(t *testing.T) {
+func TestCustomRenderFunction(t *testing.T) {📝 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 XTestCustomRenderFunction(t *testing.T) { | |
| func TestCustomRenderFunction(t *testing.T) { |
🤖 Prompt for AI Agents
In `@formatters/tests/tree_test.go` at line 128, The test was disabled by renaming
it to XTestCustomRenderFunction so it won't run; restore test execution by
renaming the function back to TestCustomRenderFunction so the test runner
includes it, or if you intend to keep it skipped, call t.Skip with a clear
reason/ticket inside XTestCustomRenderFunction (e.g., t.Skip("disabled: <reason
or ticket>")) to make the skip explicit and documented.
| func (m taskModel) renderFinalView() string { | ||
| return "" | ||
| // rendered := m.manager.Pretty() | ||
| // if m.manager.noColor.Load() { | ||
| // return rendered.String() | ||
| // } | ||
| // return rendered.ANSI() | ||
| } |
There was a problem hiding this comment.
Render a final view on quit to avoid blank exit.
renderFinalView() returns "", so the last frame is lost when quitting. Consider rendering the final Pretty() output here so users see the last task state.
🔧 Proposed fix
func (m taskModel) renderFinalView() string {
- return ""
- // rendered := m.manager.Pretty()
- // if m.manager.noColor.Load() {
- // return rendered.String()
- // }
- // return rendered.ANSI()
+ rendered := m.manager.Pretty()
+ if m.manager.noColor.Load() {
+ return rendered.String()
+ }
+ return rendered.ANSI()
}📝 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 (m taskModel) renderFinalView() string { | |
| return "" | |
| // rendered := m.manager.Pretty() | |
| // if m.manager.noColor.Load() { | |
| // return rendered.String() | |
| // } | |
| // return rendered.ANSI() | |
| } | |
| func (m taskModel) renderFinalView() string { | |
| rendered := m.manager.Pretty() | |
| if m.manager.noColor.Load() { | |
| return rendered.String() | |
| } | |
| return rendered.ANSI() | |
| } |
🤖 Prompt for AI Agents
In `@task/model.go` around lines 81 - 88, renderFinalView currently returns an
empty string so the last frame is lost; change it to produce the manager's
Pretty() output instead: call m.manager.Pretty(), check m.manager.noColor.Load()
to decide between rendered.String() (when noColor) and rendered.ANSI() (when
colored), and return that string—update the renderFinalView method to return the
final rendered view using these existing symbols (m.manager.Pretty,
m.manager.noColor.Load, rendered.String, rendered.ANSI).
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Fix all issues with AI agents
In `@api/column_test.go`:
- Around line 145-171: The test "excludes hidden columns" is incomplete: instead
of unused local types, create a concrete type with a hidden field and exercise
NewTableFrom (or the TableProvider implementation) to ensure hidden fields are
filtered out; implement a small TableProvider wrapper type (e.g.,
employeeWithHidden) that exposes a hidden column, call NewTableFrom or
mockEmployee.NewTableFrom equivalent to produce a table, and assert the hidden
column is not present in table.Headers and table.FieldNames while visible
columns remain. If you only want to assert mockEmployee has no hidden columns,
rename the test to reflect that.
In `@api/meta.go`:
- Around line 39-45: The IsEmpty method on PrettyData currently ignores embedded
TypedValue content; update PrettyData.IsEmpty to also check pd.TypedValue and
return false when it contains meaningful data: if pd.TypedValue is non-nil,
inspect its concrete kinds (Textable -> non-empty string, Slice -> length>0, Map
-> length>0, TypedList -> length>0) or call an existing TypedValue/IsEmpty
helper if present, and only return true when Schema, Table, Tree, TypedMap are
nil and TypedValue is either nil or empty; reference PrettyData.IsEmpty,
pd.TypedValue, and the Textable/Slice/Map/TypedList kinds when implementing the
checks.
- Around line 484-551: The slice handling in TryTypedValue currently bails out
for empty slices due to the val.Len() > 0 guard, causing empty typed slices to
be stringified; update the logic in TryTypedValue so that slice kind is handled
regardless of length: remove the val.Len() > 0 check (or branch to handle
len==0) and for each interface case (TableProvider, TableMixin, TableRowMixin2,
TreeNode, TreeMixin, Pretty, Textable) allocate zero-length target slices (e.g.,
make([]TableProvider, val.Len()), make(TextList, val.Len()), etc.), populate
them only when len>0, and return the appropriate TypedValue using NewTableFrom,
NewTable, NewTableFromMixin, NewTree or a TextList pointer so empty
[]MyTableProvider{} / []TreeNode{} produce empty tables/trees/lists instead of
being stringified.
In `@api/parser.go`:
- Around line 348-354: The short-circuit in the TryTypedValue branch skips
attaching the schema FieldMeta (e.g., CompactItems, Format/FormatTable) which
downstream renderers need; modify the code in the block that handles
TryTypedValue (where fieldVal, TryTypedValue(...) and values[field.Name] are
used) to preserve/attach the field's FieldMeta instead of simply
continuing—either by merging field.Meta into the returned TypedValue (if
TypedValue has a meta or options field) or by setting the corresponding entry in
the schema metadata map (e.g., valuesMeta[field.Name] = field.Meta) before
returning; ensure CompactItems/Format metadata from field.Meta is available to
renderers after this branch.
In `@api/text.go`:
- Around line 217-241: uniqueStyles produces a non-deterministic ordering
because it iterates a map; modify uniqueStyles to collect keys from styleSet
into a slice, sort that slice (e.g., sort.Strings), and then join the sorted
keys with spaces to build the returned string; also ensure you consistently
ignore empty strings when parsing the existing parameter and when adding keys so
no empty tokens are included in the final sorted output.
In `@formatters/options.go`:
- Around line 47-53: The current SkipTable and SkipTree logic causes both to
return true when Table and Tree are both true; either enforce mutual exclusivity
in the option-merging path or make the skip predicates reflect simple absence.
Fix by updating MergeOptions or ResolveFormat to validate that both Table and
Tree are not simultaneously true and return an error (or coerce one value), or
change SkipTable() to return !o.Table and SkipTree() to return !o.Tree so
setting both displays both outputs; locate and modify the SkipTable and SkipTree
methods (and/or add validation logic in MergeOptions / ResolveFormat)
accordingly.
In `@formatters/parser.go`:
- Around line 626-633: When val is a slice/array and both opt.SkipTree and
opt.SkipTable are true the code currently falls through to
ParseStructSchema(val) which will panic; update the slice/array branch in the
parser so that after checking hasTreeStructure and SkipTable you explicitly
handle the case where opt.SkipTree && opt.SkipTable by returning an appropriate
result (e.g. a nil/empty data value or a typed error) instead of falling through
to ParseStructSchema; reference the slice handling logic around
hasTreeStructure, convertSliceToTreeData, convertSliceToPrettyData and ensure
ParseStructSchema is only called for actual structs.
In `@formatters/reflect.go`:
- Around line 207-214: The case-insensitive match is using a pre-lowercased
variable but then calling strings.EqualFold with that lowercased value; change
the comparison to use the original name (not lowerName) and remove the
unnecessary lowercasing. Specifically, in the loop that iterates typ.NumField()
and assigns field := typ.Field(i) (returning val.Field(i) on match), replace the
strings.EqualFold(field.Name, lowerName) call with strings.EqualFold(field.Name,
name) and delete the lowerName := strings.ToLower(name) variable since EqualFold
already handles case-insensitivity.
In `@task/manager.go`:
- Around line 282-290: The plain render goroutine can start after stopProgram
runs leaving tm.stopPlainRender nil; before launching the goroutine in the block
that calls tm.plainRenderLoop(), allocate/init tm.stopPlainRender (and any other
plain-render channels) on tm (e.g., set tm.stopPlainRender = make(chan struct{})
and related channels) so they exist even if stopProgram runs immediately, and
ensure plainRenderLoop and stopProgram use those initialized channels (also
optionally check and create channels at the top of plainRenderLoop to be
defensive); modify the start sequence around tm.plainRenderLoop(), the
initialization of tm.stopPlainRender, and the stop logic in tm.stopProgram to
rely on the non-nil channel.
In `@task/render_test.go`:
- Around line 41-114: Add a cleanup handler to guarantee restoring os.Stderr and
closing the pipe/reader goroutine if the test fails early: register a t.Cleanup
(or defer) immediately after creating the pipe and redirecting os.Stderr that
resets os.Stderr to originalStderr, closes the write end (w) if still open,
closes the read end (r) and waits for the reader goroutine (done) to finish;
ensure this runs regardless of where the test returns so resources used by
newManagerWithConcurrency, testManager, the pipe (r/w) and the stderrCapture
goroutine are always cleaned up before the test exits, and keep the existing
explicit close/wait after PlainRender for the success path.
🧹 Nitpick comments (10)
formatters/tests/sorting_test.go (1)
11-11: Add tracking for disabled test.Renaming to
XTestSortRowsdisables this test. Consider adding a comment explaining why it's disabled and when it should be re-enabled, or create a tracking issue to ensure it's not forgotten.+// TODO: Re-enable once sorting API stabilizes - see issue `#XX` func XTestSortRows(t *testing.T) {formatters/tests/tree_test.go (1)
128-128: Prefert.Skip()overXTestprefix for consistency.This file already uses
t.Skip("reason")at line 156 forTestTreeWithPrettyTags. Consider using the same pattern here for better documentation of why the test is disabled:-func XTestCustomRenderFunction(t *testing.T) { +func TestCustomRenderFunction(t *testing.T) { + t.Skip("Temporarily disabled due to API changes - see issue `#XX`")formatters/tests/formatters_test.go (1)
46-51: Minor: Stray blank line inside conditional.There's an extra blank line between the error message and the closing brace. This appears accidental.
if !strings.Contains(output, "id: TEST-001") { t.Errorf("Pretty formatter should display ID field") - }formatters/tests/map_fields_test.go (1)
54-85: Consider creating a tracking issue for commented-out test.The FIXME comment documents the reason, but consider adding an issue reference to ensure this gets re-enabled:
- // FIXME: Test ParseDataWithSchema - commented out due to API changes + // FIXME: Test ParseDataWithSchema - commented out due to API changes (tracking: `#XX`)Also note that lines 164 and 265 have tests disabled via
XTestprefix without similar documentation.aliases.go (1)
57-58: Use a wrapper function instead of a mutable variable for this function re-export.
var Column = api.Columnallows downstream packages to reassignclicky.Column, which is non-idiomatic for function re-exports. Define a wrapper:func Column(name string) *ColumnBuilder { return api.Column(name) }formatters/parser.go (1)
614-622: Variable shadowing may cause confusion.Line 615 shadows the outer
val(of typereflect.Value) with a newval(of typeinterface{}). While this works correctly because the inner scope exits via return, it can be confusing for readers.Consider using a different variable name for clarity:
♻️ Suggested fix
// Check dereferenced value for Pretty interface if val.CanInterface() { - val := val.Interface() - if v := api.TryTypedValue(val); v != nil { + iface := val.Interface() + if v := api.TryTypedValue(iface); v != nil { return &api.PrettyData{ Original: data, TypedValue: *v, }, nil } }formatters/reflect.go (1)
168-190: Consider documenting intentional omissions forisEmptyValue.The function doesn't treat
falsebooleans or zero numeric values as empty, which may be intentional. A brief doc comment clarifying this design choice would help future maintainers understand the expected behavior.examples/uber_demo/main.go (1)
610-613: Remove debug output before merging.These
fmt.Fprintf(os.Stderr, "[DEBUG ...")statements appear to be development artifacts and should be removed for production code.Proposed fix
func showAll(opts AllOptions) (any, error) { demo := createDemoData() - // Debug: check if FileSystem is set - fmt.Fprintf(os.Stderr, "[DEBUG showAll] FileSystem nil? %v\n", demo.FileSystem == nil) - clicky.Infof(clicky.MustFormat(*demo.FileSystem, clicky.FormatOptions{Pretty: true, Format: "pretty"}))And remove line 631:
- fmt.Fprintf(os.Stderr, "[DEBUG showAll after filtering] FileSystem nil? %v\n", demo.FileSystem == nil) return demo, nilAlso applies to: 631-631
task/manager.go (1)
347-360: startProgram lacks synchronization for concurrent access.The
tm.program != nilcheck and subsequent assignment aren't protected by a mutex, which could lead to multiple programs being started ifstartProgramis called concurrently. While this may not occur in practice, adding synchronization would make the code more robust.Proposed fix
func (tm *Manager) startProgram() { + tm.mu.Lock() + defer tm.mu.Unlock() + if tm.program != nil { return } model := newTaskModel(tm) tm.program = tea.NewProgram(model, tea.WithOutput(os.Stderr)) go func() { if _, err := tm.program.Run(); err != nil { logger.Errorf("bubbletea program error: %v", err) } }() }api/column.go (1)
102-154: Document assumption: all items must return identical Columns().
NewTableFromusesitems[0].Columns()to define the table schema. If different items in the slice return different column definitions, only the first item's schema is used. This is likely intentional for performance, but documenting this assumption would help API consumers.Suggested doc enhancement
// NewTableFrom creates a TextTable from a slice of TableProvider items. +// The column schema is derived from the first item's Columns() method. +// All items are expected to return the same column definitions. func NewTableFrom[T TableProvider](items []T) TextTable {
| It("excludes hidden columns", func() { | ||
| type hiddenEmployee struct { | ||
| ID int | ||
| Name string | ||
| } | ||
|
|
||
| // Create a local type with hidden column | ||
| employees := []struct { | ||
| ID int | ||
| Name string | ||
| }{{ID: 1, Name: "Alice"}} | ||
|
|
||
| // Use a wrapper that implements TableProvider | ||
| type wrapperEmployee struct { | ||
| ID int | ||
| Name string | ||
| } | ||
|
|
||
| // Since we can't easily test hidden columns without modifying the mock, | ||
| // we verify that our mockEmployee doesn't have hidden columns | ||
| cols := mockEmployee{}.Columns() | ||
| for _, col := range cols { | ||
| Expect(col.Hidden).To(BeFalse(), "no columns should be hidden in mockEmployee") | ||
| } | ||
|
|
||
| _ = employees // suppress unused warning | ||
| }) |
There was a problem hiding this comment.
Incomplete test for hidden column exclusion.
This test doesn't actually verify that hidden columns are excluded from the table. It creates unused local types (hiddenEmployee, employees, wrapperEmployee) and only asserts that mockEmployee has no hidden columns, which doesn't test the hidden column filtering logic in NewTableFrom.
Consider creating a type with an actual hidden column and verifying it's excluded:
💡 Suggested approach
It("excludes hidden columns", func() {
// Create a wrapper that has a hidden column
type employeeWithHidden struct {
ID int
Name string
Internal string // This will be hidden
}
// Implement TableProvider with a hidden column
// Then verify the hidden column is not in table.Headers or table.FieldNames
})Alternatively, if the intent is just to document the current state of mockEmployee, rename the test to clarify its purpose.
🤖 Prompt for AI Agents
In `@api/column_test.go` around lines 145 - 171, The test "excludes hidden
columns" is incomplete: instead of unused local types, create a concrete type
with a hidden field and exercise NewTableFrom (or the TableProvider
implementation) to ensure hidden fields are filtered out; implement a small
TableProvider wrapper type (e.g., employeeWithHidden) that exposes a hidden
column, call NewTableFrom or mockEmployee.NewTableFrom equivalent to produce a
table, and assert the hidden column is not present in table.Headers and
table.FieldNames while visible columns remain. If you only want to assert
mockEmployee has no hidden columns, rename the test to reflect that.
| // IsEmpty returns true if the PrettyData has no meaningful content | ||
| func (pd *PrettyData) IsEmpty() bool { | ||
| if pd == nil { | ||
| return true | ||
| } | ||
| return pd.Schema == nil && pd.Table == nil && pd.Tree == nil && pd.TypedMap == nil | ||
| } |
There was a problem hiding this comment.
IsEmpty ignores embedded TypedValue content.
If PrettyData only carries Textable/Slice/Map/TypedList (no Schema/Table/Tree/TypedMap), this returns true and callers may suppress legitimate output.
🛠️ Suggested fix
func (pd *PrettyData) IsEmpty() bool {
if pd == nil {
return true
}
- return pd.Schema == nil && pd.Table == nil && pd.Tree == nil && pd.TypedMap == nil
+ if pd.Schema != nil || pd.Table != nil || pd.Tree != nil || pd.TypedMap != nil {
+ return false
+ }
+ if pd.Textable != nil || pd.Slice != nil || pd.Map != nil || pd.TypedList != nil {
+ return false
+ }
+ return true
}🤖 Prompt for AI Agents
In `@api/meta.go` around lines 39 - 45, The IsEmpty method on PrettyData currently
ignores embedded TypedValue content; update PrettyData.IsEmpty to also check
pd.TypedValue and return false when it contains meaningful data: if
pd.TypedValue is non-nil, inspect its concrete kinds (Textable -> non-empty
string, Slice -> length>0, Map -> length>0, TypedList -> length>0) or call an
existing TypedValue/IsEmpty helper if present, and only return true when Schema,
Table, Tree, TypedMap are nil and TypedValue is either nil or empty; reference
PrettyData.IsEmpty, pd.TypedValue, and the Textable/Slice/Map/TypedList kinds
when implementing the checks.
| // Use reflection to check slices of interface implementations | ||
| val := reflect.ValueOf(o) | ||
| if val.Kind() == reflect.Slice && val.Len() > 0 { | ||
| elemType := val.Type().Elem() | ||
|
|
||
| // Check TableProvider first (most specific table interface) | ||
| if elemType.Implements(tableProviderType) { | ||
| items := make([]TableProvider, val.Len()) | ||
| for i := 0; i < val.Len(); i++ { | ||
| items[i] = val.Index(i).Interface().(TableProvider) | ||
| } | ||
| return &TypedValue{Table: lo.ToPtr(NewTableFrom(items))} | ||
| } | ||
|
|
||
| // Check TableMixin | ||
| if elemType.Implements(tableMixinType) { | ||
| items := make([]TableMixin, val.Len()) | ||
| for i := 0; i < val.Len(); i++ { | ||
| items[i] = val.Index(i).Interface().(TableMixin) | ||
| } | ||
| return &TypedValue{Table: lo.ToPtr(NewTable(items))} | ||
| } | ||
|
|
||
| // Check TableRowMixin2 | ||
| if elemType.Implements(tableRowMixin2Type) { | ||
| items := make([]TableRowMixin2, val.Len()) | ||
| for i := 0; i < val.Len(); i++ { | ||
| items[i] = val.Index(i).Interface().(TableRowMixin2) | ||
| } | ||
| return &TypedValue{Table: lo.ToPtr(NewTableFromMixin(items))} | ||
| } | ||
|
|
||
| // Check TreeNode | ||
| if elemType.Implements(treeNodeType) { | ||
| items := make([]TreeNode, val.Len()) | ||
| for i := 0; i < val.Len(); i++ { | ||
| items[i] = val.Index(i).Interface().(TreeNode) | ||
| } | ||
| return &TypedValue{Tree: lo.ToPtr(NewTree(items...))} | ||
| } | ||
|
|
||
| // Check TreeMixin | ||
| if elemType.Implements(treeMixinType) { | ||
| nodes := make([]TreeNode, val.Len()) | ||
| for i := 0; i < val.Len(); i++ { | ||
| nodes[i] = val.Index(i).Interface().(TreeMixin).Tree() | ||
| } | ||
| return &TypedValue{Tree: lo.ToPtr(NewTree(nodes...))} | ||
| } | ||
|
|
||
| // Check Pretty | ||
| if elemType.Implements(prettyType) { | ||
| list := make(TextList, val.Len()) | ||
| for i := 0; i < val.Len(); i++ { | ||
| list[i] = val.Index(i).Interface().(Pretty).Pretty() | ||
| } | ||
| return &TypedValue{Slice: &list} | ||
| } | ||
|
|
||
| // Check Textable (last - most general) | ||
| if elemType.Implements(textableType) { | ||
| list := make(TextList, val.Len()) | ||
| for i := 0; i < val.Len(); i++ { | ||
| list[i] = val.Index(i).Interface().(Textable) | ||
| } | ||
| return &TypedValue{Slice: &list} | ||
| } | ||
| } |
There was a problem hiding this comment.
Handle empty slices in TryTypedValue.
The val.Len() > 0 guard skips empty slices, so []MyTableProvider{} or []TreeNode{} fall through and get stringified instead of producing an empty table/tree/list.
🛠️ Suggested fix
- if val.Kind() == reflect.Slice && val.Len() > 0 {
+ if val.Kind() == reflect.Slice {
elemType := val.Type().Elem()🤖 Prompt for AI Agents
In `@api/meta.go` around lines 484 - 551, The slice handling in TryTypedValue
currently bails out for empty slices due to the val.Len() > 0 guard, causing
empty typed slices to be stringified; update the logic in TryTypedValue so that
slice kind is handled regardless of length: remove the val.Len() > 0 check (or
branch to handle len==0) and for each interface case (TableProvider, TableMixin,
TableRowMixin2, TreeNode, TreeMixin, Pretty, Textable) allocate zero-length
target slices (e.g., make([]TableProvider, val.Len()), make(TextList,
val.Len()), etc.), populate them only when len>0, and return the appropriate
TypedValue using NewTableFrom, NewTable, NewTableFromMixin, NewTree or a
TextList pointer so empty []MyTableProvider{} / []TreeNode{} produce empty
tables/trees/lists instead of being stringified.
| // Try TryTypedValue first - handles TableProvider, TreeNode, Textable, etc. | ||
| if fieldVal.CanInterface() { | ||
| if tv := TryTypedValue(fieldVal.Interface()); tv != nil { | ||
| values[field.Name] = *tv | ||
| continue | ||
| } | ||
| } |
There was a problem hiding this comment.
Preserve schema FieldMeta when short‑circuiting via TryTypedValue.
The early continue skips the schema-driven FieldMeta attachment (e.g., CompactItems, Format), which downstream renderers (HTML table deferral/compact rendering) rely on. This can change rendering behavior for fields that specify FormatTable/CompactItems.
✅ Suggested fix
- if fieldVal.CanInterface() {
- if tv := TryTypedValue(fieldVal.Interface()); tv != nil {
- values[field.Name] = *tv
- continue
- }
- }
+ if fieldVal.CanInterface() {
+ if tv := TryTypedValue(fieldVal.Interface()); tv != nil {
+ if field.Format != "" || field.CompactItems {
+ tv.FieldMeta = &FieldMeta{
+ Name: field.Name,
+ CompactItems: field.CompactItems,
+ Format: field.Format,
+ }
+ }
+ values[field.Name] = *tv
+ continue
+ }
+ }🤖 Prompt for AI Agents
In `@api/parser.go` around lines 348 - 354, The short-circuit in the TryTypedValue
branch skips attaching the schema FieldMeta (e.g., CompactItems,
Format/FormatTable) which downstream renderers need; modify the code in the
block that handles TryTypedValue (where fieldVal, TryTypedValue(...) and
values[field.Name] are used) to preserve/attach the field's FieldMeta instead of
simply continuing—either by merging field.Meta into the returned TypedValue (if
TypedValue has a meta or options field) or by setting the corresponding entry in
the schema metadata map (e.g., valuesMeta[field.Name] = field.Meta) before
returning; ensure CompactItems/Format metadata from field.Meta is available to
renderers after this branch.
| func uniqueStyles(existing string, styles ...string) string { | ||
| styleSet := make(map[string]struct{}) | ||
| if existing != "" { | ||
| for _, s := range strings.Split(existing, " ") { | ||
| styleSet[s] = struct{}{} | ||
| } | ||
| } | ||
| for _, style := range styles { | ||
| for _, s := range strings.Split(style, " ") { | ||
| if s != "" { | ||
| styleSet[s] = struct{}{} | ||
| } | ||
| } | ||
| } | ||
| uniq := "" | ||
| for s := range styleSet { | ||
| if uniq == "" { | ||
| uniq = s | ||
| } else { | ||
| uniq += " " + s | ||
| } | ||
| } | ||
|
|
||
| return uniq | ||
| } |
There was a problem hiding this comment.
Non-deterministic style ordering may cause test flakiness.
Map iteration in Go is non-deterministic, so uniqueStyles may return styles in different orders across runs. This could cause issues if any code compares style strings for equality.
Consider sorting the output for deterministic ordering:
♻️ Suggested fix
func uniqueStyles(existing string, styles ...string) string {
styleSet := make(map[string]struct{})
if existing != "" {
for _, s := range strings.Split(existing, " ") {
- styleSet[s] = struct{}{}
+ if s != "" {
+ styleSet[s] = struct{}{}
+ }
}
}
for _, style := range styles {
for _, s := range strings.Split(style, " ") {
if s != "" {
styleSet[s] = struct{}{}
}
}
}
- uniq := ""
- for s := range styleSet {
- if uniq == "" {
- uniq = s
- } else {
- uniq += " " + s
- }
- }
-
- return uniq
+ result := make([]string, 0, len(styleSet))
+ for s := range styleSet {
+ result = append(result, s)
+ }
+ sort.Strings(result)
+ return strings.Join(result, " ")
}This also adds the empty-string filter for the existing parameter for consistency.
🤖 Prompt for AI Agents
In `@api/text.go` around lines 217 - 241, uniqueStyles produces a
non-deterministic ordering because it iterates a map; modify uniqueStyles to
collect keys from styleSet into a slice, sort that slice (e.g., sort.Strings),
and then join the sorted keys with spaces to build the returned string; also
ensure you consistently ignore empty strings when parsing the existing parameter
and when adding keys so no empty tokens are included in the final sorted output.
| func (o FormatOptions) SkipTable() bool { | ||
| return !o.Table || o.Tree | ||
| } | ||
|
|
||
| func (o FormatOptions) SkipTree() bool { | ||
| return !o.Tree || o.Table | ||
| } |
There was a problem hiding this comment.
Logic issue when both Table and Tree are true.
When both Table=true and Tree=true:
SkipTable()returnstrue(becauseo.Treeis true)SkipTree()returnstrue(becauseo.Tableis true)
This results in both being skipped, which seems unintended. If these flags are mutually exclusive, consider adding validation in MergeOptions or ResolveFormat. If they should be additive (as suggested by the comment on lines 35-37), the logic needs adjustment:
🔧 Option 1: Make flags mutually exclusive (explicit check)
+func (o FormatOptions) Validate() error {
+ if o.Table && o.Tree {
+ return fmt.Errorf("--table and --tree flags are mutually exclusive")
+ }
+ return nil
+}🔧 Option 2: If both are set, show both (change skip logic)
func (o FormatOptions) SkipTable() bool {
- return !o.Table || o.Tree
+ return !o.Table
}
func (o FormatOptions) SkipTree() bool {
- return !o.Tree || o.Table
+ return !o.Tree
}📝 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 (o FormatOptions) SkipTable() bool { | |
| return !o.Table || o.Tree | |
| } | |
| func (o FormatOptions) SkipTree() bool { | |
| return !o.Tree || o.Table | |
| } | |
| func (o FormatOptions) SkipTable() bool { | |
| return !o.Table | |
| } | |
| func (o FormatOptions) SkipTree() bool { | |
| return !o.Tree | |
| } |
🤖 Prompt for AI Agents
In `@formatters/options.go` around lines 47 - 53, The current SkipTable and
SkipTree logic causes both to return true when Table and Tree are both true;
either enforce mutual exclusivity in the option-merging path or make the skip
predicates reflect simple absence. Fix by updating MergeOptions or ResolveFormat
to validate that both Table and Tree are not simultaneously true and return an
error (or coerce one value), or change SkipTable() to return !o.Table and
SkipTree() to return !o.Tree so setting both displays both outputs; locate and
modify the SkipTable and SkipTree methods (and/or add validation logic in
MergeOptions / ResolveFormat) accordingly.
| if val.Kind() == reflect.Slice || val.Kind() == reflect.Array { | ||
| if hasTreeStructure(val) { | ||
| if !opt.SkipTree && hasTreeStructure(val) { | ||
| return convertSliceToTreeData(val) | ||
| } | ||
| return convertSliceToPrettyData(val) | ||
| if !opt.SkipTable { | ||
| return convertSliceToPrettyData(val) | ||
| } | ||
| } |
There was a problem hiding this comment.
Edge case: Slices fall through to struct parsing when both SkipTree and SkipTable are true.
When a slice/array is passed and both SkipTree and SkipTable are set, the code falls through to ParseStructSchema(val) on line 636, which will fail for slices since they're not structs.
Consider returning an appropriate result or error for this case:
💡 Suggested fix
if val.Kind() == reflect.Slice || val.Kind() == reflect.Array {
if !opt.SkipTree && hasTreeStructure(val) {
return convertSliceToTreeData(val)
}
if !opt.SkipTable {
return convertSliceToPrettyData(val)
}
+ // Both tree and table skipped - return empty PrettyData for slices
+ return &api.PrettyData{
+ Schema: &api.PrettyObject{Fields: []api.PrettyField{}},
+ Original: data,
+ }, nil
}📝 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.
| if val.Kind() == reflect.Slice || val.Kind() == reflect.Array { | |
| if hasTreeStructure(val) { | |
| if !opt.SkipTree && hasTreeStructure(val) { | |
| return convertSliceToTreeData(val) | |
| } | |
| return convertSliceToPrettyData(val) | |
| if !opt.SkipTable { | |
| return convertSliceToPrettyData(val) | |
| } | |
| } | |
| if val.Kind() == reflect.Slice || val.Kind() == reflect.Array { | |
| if !opt.SkipTree && hasTreeStructure(val) { | |
| return convertSliceToTreeData(val) | |
| } | |
| if !opt.SkipTable { | |
| return convertSliceToPrettyData(val) | |
| } | |
| // Both tree and table skipped - return empty PrettyData for slices | |
| return &api.PrettyData{ | |
| Schema: &api.PrettyObject{Fields: []api.PrettyField{}}, | |
| Original: data, | |
| }, nil | |
| } |
🤖 Prompt for AI Agents
In `@formatters/parser.go` around lines 626 - 633, When val is a slice/array and
both opt.SkipTree and opt.SkipTable are true the code currently falls through to
ParseStructSchema(val) which will panic; update the slice/array branch in the
parser so that after checking hasTreeStructure and SkipTable you explicitly
handle the case where opt.SkipTree && opt.SkipTable by returning an appropriate
result (e.g. a nil/empty data value or a typed error) instead of falling through
to ParseStructSchema; reference the slice handling logic around
hasTreeStructure, convertSliceToTreeData, convertSliceToPrettyData and ensure
ParseStructSchema is only called for actual structs.
| // Try case-insensitive match | ||
| lowerName := strings.ToLower(name) | ||
| for i := 0; i < typ.NumField(); i++ { | ||
| field := typ.Field(i) | ||
| if strings.EqualFold(field.Name, lowerName) { | ||
| return val.Field(i) | ||
| } | ||
| } |
There was a problem hiding this comment.
Bug in case-insensitive matching logic.
strings.EqualFold already performs case-insensitive comparison, so pre-lowercasing name with strings.ToLower is redundant but harmless. However, the comparison should be strings.EqualFold(field.Name, name) (original name), not lowerName.
Proposed fix
// Try case-insensitive match
- lowerName := strings.ToLower(name)
for i := 0; i < typ.NumField(); i++ {
field := typ.Field(i)
- if strings.EqualFold(field.Name, lowerName) {
+ if strings.EqualFold(field.Name, name) {
return val.Field(i)
}
}📝 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.
| // Try case-insensitive match | |
| lowerName := strings.ToLower(name) | |
| for i := 0; i < typ.NumField(); i++ { | |
| field := typ.Field(i) | |
| if strings.EqualFold(field.Name, lowerName) { | |
| return val.Field(i) | |
| } | |
| } | |
| // Try case-insensitive match | |
| for i := 0; i < typ.NumField(); i++ { | |
| field := typ.Field(i) | |
| if strings.EqualFold(field.Name, name) { | |
| return val.Field(i) | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@formatters/reflect.go` around lines 207 - 214, The case-insensitive match is
using a pre-lowercased variable but then calling strings.EqualFold with that
lowercased value; change the comparison to use the original name (not lowerName)
and remove the unnecessary lowercasing. Specifically, in the loop that iterates
typ.NumField() and assigns field := typ.Field(i) (returning val.Field(i) on
match), replace the strings.EqualFold(field.Name, lowerName) call with
strings.EqualFold(field.Name, name) and delete the lowerName :=
strings.ToLower(name) variable since EqualFold already handles
case-insensitivity.
| // Start appropriate rendering based on mode | ||
| if !tm.noProgress.Load() { | ||
| if isInteractive { | ||
| tm.startProgram() | ||
| } else { | ||
| // Non-interactive mode: start a plain render loop | ||
| go tm.plainRenderLoop() | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential race: plainRenderLoop may not initialize channels before stopProgram is called.
If stopProgram() is called very quickly after manager creation (before the goroutine at line 288 runs), tm.stopPlainRender will still be nil, and the stop logic in stopProgram will skip the plain render cleanup entirely. Consider initializing the channels before starting the goroutine.
Proposed fix
} else {
// Non-interactive mode: start a plain render loop
+ tm.stopPlainRender = make(chan struct{})
+ tm.plainRenderDone = make(chan struct{})
go tm.plainRenderLoop()
}And in plainRenderLoop:
func (tm *Manager) plainRenderLoop() {
- tm.stopPlainRender = make(chan struct{})
- tm.plainRenderDone = make(chan struct{})
defer close(tm.plainRenderDone)🤖 Prompt for AI Agents
In `@task/manager.go` around lines 282 - 290, The plain render goroutine can start
after stopProgram runs leaving tm.stopPlainRender nil; before launching the
goroutine in the block that calls tm.plainRenderLoop(), allocate/init
tm.stopPlainRender (and any other plain-render channels) on tm (e.g., set
tm.stopPlainRender = make(chan struct{}) and related channels) so they exist
even if stopProgram runs immediately, and ensure plainRenderLoop and stopProgram
use those initialized channels (also optionally check and create channels at the
top of plainRenderLoop to be defensive); modify the start sequence around
tm.plainRenderLoop(), the initialization of tm.stopPlainRender, and the stop
logic in tm.stopProgram to rely on the non-nil channel.
| // Save original stderr and global state | ||
| originalStderr := os.Stderr | ||
|
|
||
| // Create pipe for capturing stderr | ||
| r, w, err := os.Pipe() | ||
| if err != nil { | ||
| t.Fatalf("failed to create pipe: %v", err) | ||
| } | ||
|
|
||
| // Redirect stderr to our pipe | ||
| os.Stderr = w | ||
|
|
||
| // Create buffer and copy goroutine | ||
| stderrCapture := &bytes.Buffer{} | ||
| done := make(chan struct{}) | ||
| go func() { | ||
| io.Copy(stderrCapture, r) | ||
| close(done) | ||
| }() | ||
|
|
||
| // Create a fresh manager for this test | ||
| testManager := newManagerWithConcurrency(4) | ||
| testManager.noProgress.Store(true) | ||
| testManager.noColor.Store(true) | ||
|
|
||
| // Create tasks that complete immediately | ||
| for i := 0; i < tt.numTasks; i++ { | ||
| taskName := fmt.Sprintf("task-%d", i) | ||
| task := testManager.newTask(taskName) | ||
| task.runFunc = func(ctx flanksourceContext.Context, t *Task) error { | ||
| t.Success() | ||
| return nil | ||
| } | ||
| testManager.enqueue(task) | ||
| } | ||
|
|
||
| // Wait for tasks to complete | ||
| timeout := time.After(5 * time.Second) | ||
| ticker := time.NewTicker(10 * time.Millisecond) | ||
| defer ticker.Stop() | ||
|
|
||
| waitLoop: | ||
| for { | ||
| select { | ||
| case <-timeout: | ||
| t.Fatal("timeout waiting for tasks to complete") | ||
| case <-ticker.C: | ||
| if testManager.taskQueue.Empty() && testManager.workersActive.Load() == 0 { | ||
| allComplete := true | ||
| testManager.mu.RLock() | ||
| for _, task := range testManager.tasks { | ||
| if !task.completed.Load() { | ||
| allComplete = false | ||
| break | ||
| } | ||
| } | ||
| testManager.mu.RUnlock() | ||
| if allComplete { | ||
| break waitLoop | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Force render | ||
| testManager.PlainRender() | ||
|
|
||
| // Close write end and wait for reader to finish | ||
| w.Close() | ||
| <-done | ||
|
|
||
| // Restore stderr | ||
| os.Stderr = originalStderr | ||
|
|
There was a problem hiding this comment.
Ensure stderr/pipe cleanup on test failure.
If the test exits early, os.Stderr remains redirected and pipe FDs can leak, impacting subsequent tests. A cleanup handler prevents cascading failures.
🛠️ Suggested fix
// Create pipe for capturing stderr
r, w, err := os.Pipe()
if err != nil {
t.Fatalf("failed to create pipe: %v", err)
}
+ t.Cleanup(func() {
+ os.Stderr = originalStderr
+ _ = w.Close()
+ _ = r.Close()
+ })
// Redirect stderr to our pipe
os.Stderr = w🤖 Prompt for AI Agents
In `@task/render_test.go` around lines 41 - 114, Add a cleanup handler to
guarantee restoring os.Stderr and closing the pipe/reader goroutine if the test
fails early: register a t.Cleanup (or defer) immediately after creating the pipe
and redirecting os.Stderr that resets os.Stderr to originalStderr, closes the
write end (w) if still open, closes the read end (r) and waits for the reader
goroutine (done) to finish; ensure this runs regardless of where the test
returns so resources used by newManagerWithConcurrency, testManager, the pipe
(r/w) and the stderrCapture goroutine are always cleaned up before the test
exits, and keep the existing explicit close/wait after PlainRender for the
success path.
Single item returns a single row, not multiple rows.
1 participant
Description
Brief description of the changes in this PR.
Type of Change
Testing
Checklist
Breaking Changes
If this is a breaking change, please describe the impact and migration path for existing users:
Additional Notes
Add any additional notes, screenshots, or context about the changes here.
Summary by CodeRabbit
Release Notes
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.