fix(api): prevent excessive padding from blank lines and large lists - #113
Merged
Conversation
Add a LiveRenderer interface and SetLiveRenderer hook so a caller can render its own block (e.g. a status table) through clicky's task manager instead of hand-rolling an ANSI redraw. The manager keeps owning the TTY, ClearLines line accounting, and the logger serializer that prevents concurrent log lines from corrupting the in-place frame; only the rendered api.Text content is swapped. Wired into the three render entry points: interactiveRender (live tick), PlainRender (non-interactive flush, whole block), and renderFinal (post summary). When no renderer is installed the default task-tree formatting is unchanged.
…sts from causing excessive padding Add normalizeTreeLabel() to strip blank lines and trailing whitespace from multi-line tree node labels. This prevents lipgloss from rendering stray gutter-only lines and right-padding all sibling lines with thousands of trailing spaces. Modify DescriptionList.String() and ANSI() to stack large lists (>3 items or >terminal width) one pair per line instead of joining inline. This prevents single multi-thousand-character lines from forcing excessive padding in tree output. Add TypedFilter interface to allow filters to override their lookup type independently of the flag type, enabling date-math and similar flags to advertise appropriate UI controls. Includes comprehensive test coverage for tree label normalization, description list stacking behavior, and filter type overrides.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
🚧 Files skipped from review as they are similar to previous changes (10)
WalkthroughNormalizes multi-line tree labels to drop blank lines, makes DescriptionList render stacked when too wide, adds TypedFilter to override lookup types, introduces a LiveRenderer hook for task rendering, and adds a docs generation subsystem with model, renderers, scaffold, CLI command, and tests. ChangesRendering and Filter Enhancements
Possibly related PRs:
🚥 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 docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
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 |
Contributor
Gavel summary
Totals: 1889 passed · 1 failed · 63 skipped · 53.4s Failing testsginkgo Execution |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
entity_filters_test.go (1)
350-352: ⚡ Quick winAdd a regression test for typed overrides through
LiftFilters.Current assertion validates the direct-filter path only. Please add a case where an inner filter implementing
LookupType()is wrapped withLiftFilters[...], then assertlookup.Filters[...].Typestill reflects the override.🤖 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 `@entity_filters_test.go` around lines 350 - 352, Add a regression case that exercises typed overrides via LiftFilters: create or reuse an inner filter type that implements LookupType() returning "number", wrap that instance with LiftFilters[...] and use it in the same lookup construction path being tested, then assert that lookup.Filters["amount"].Type == "number". Specifically, ensure the test wraps the filter with LiftFilters, invokes whatever function builds the lookup (as in the surrounding test), and replaces or adds the assertion following the existing direct-filter check so it verifies the wrapped-filter path (referencing symbols LiftFilters, LookupType, and lookup.Filters["amount"]).formatters/tree_formatter.go (1)
138-156: 💤 Low valueConsider consolidating duplicate normalization logic.
This function duplicates
normalizeTreeLabelfromapi/meta.gowith a subtle difference: this version doesn't strip ANSI codes before checking for blank lines, while theapiversion does. Both implementations are correct for their contexts (this one normalizes before styling, the api one after), but maintaining two similar functions increases the risk of inconsistent bug fixes or divergent behavior over time.Potential consolidation approach
You could extract the common logic to a shared location with an optional parameter:
// In a shared utility package or the api package: func normalizeTreeLabel(s string, stripANSI bool) string { if !strings.Contains(s, "\n") { return s } lines := strings.Split(s, "\n") out := make([]string, 0, len(lines)) for _, line := range lines { toCheck := line if stripANSI { toCheck = ansi.Strip(toCheck) } if strings.TrimSpace(toCheck) == "" { continue } out = append(out, strings.TrimRight(line, " \t")) } return strings.Join(out, "\n") }Then both packages could call it with the appropriate flag.
🤖 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 `@formatters/tree_formatter.go` around lines 138 - 156, The normalizeTreeLabel implementation in formatters/tree_formatter.go duplicates logic found in api/meta.go but differs on ANSI stripping; extract the common logic into a shared utility function (e.g., normalizeTreeLabel(s string, stripANSI bool) or similar) and update both callers to call that shared function with stripANSI=false from formatters/tree_formatter.go's usage and stripANSI=true from api/meta.go's usage; ensure the new helper preserves the current behavior (trim trailing spaces, drop blank lines after optional ANSI strip) and replace the local normalizeTreeLabel with calls to the shared helper.task/manager_lifecycle.go (1)
156-189: 💤 Low valueConsider using
snapshotTasks()for consistency.Lines 165-167 manually create the task snapshot with the same logic that
snapshotTasks()(inrender_hook.go) encapsulates. Usingtm.snapshotTasks()would eliminate the duplication and ensure consistent snapshot behavior across all rendering paths.♻️ Proposed refactor
func (tm *Manager) renderFinal() { if tm.noRender.Load() { return } - tm.mu.RLock() - if len(tm.tasks) == 0 { - tm.mu.RUnlock() + taskSnapshot := tm.snapshotTasks() + if len(taskSnapshot) == 0 { return } - taskSnapshot := make([]*Task, len(tm.tasks)) - copy(taskSnapshot, tm.tasks) - tm.mu.RUnlock() var rendered api.Text🤖 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 `@task/manager_lifecycle.go` around lines 156 - 189, The renderFinal method duplicates task snapshot logic; replace the manual snapshot creation in renderFinal with a call to tm.snapshotTasks() (the helper in render_hook.go) to avoid duplication and ensure consistent snapshot semantics across render paths — locate renderFinal and swap the block that creates taskSnapshot (the make/copy and mutex calls around tm.tasks) to use taskSnapshot := tm.snapshotTasks(), preserving subsequent use of taskSnapshot and existing bufferMutex/renderer logic.
🤖 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 `@entity.go`:
- Around line 1514-1516: The TypedFilter[T] type assertion is currently done
against the outer filter value, so when filters have been wrapped by LiftFilters
(i.e., values of type liftedFilter[Outer, Inner]) the inner filter's
LookupType() is ignored and meta.Type falls back to reflection; update the logic
around the TypedFilter[T] check to detect a liftedFilter wrapper
(liftedFilter[Outer, Inner]) and, if present, extract its inner filter and, when
that inner implements TypedFilter[T], call inner.LookupType() and assign
meta.Type accordingly (or alternatively implement LookupType() on liftedFilter
to forward to its inner filter) so typed lookup overrides are preserved.
---
Nitpick comments:
In `@entity_filters_test.go`:
- Around line 350-352: Add a regression case that exercises typed overrides via
LiftFilters: create or reuse an inner filter type that implements LookupType()
returning "number", wrap that instance with LiftFilters[...] and use it in the
same lookup construction path being tested, then assert that
lookup.Filters["amount"].Type == "number". Specifically, ensure the test wraps
the filter with LiftFilters, invokes whatever function builds the lookup (as in
the surrounding test), and replaces or adds the assertion following the existing
direct-filter check so it verifies the wrapped-filter path (referencing symbols
LiftFilters, LookupType, and lookup.Filters["amount"]).
In `@formatters/tree_formatter.go`:
- Around line 138-156: The normalizeTreeLabel implementation in
formatters/tree_formatter.go duplicates logic found in api/meta.go but differs
on ANSI stripping; extract the common logic into a shared utility function
(e.g., normalizeTreeLabel(s string, stripANSI bool) or similar) and update both
callers to call that shared function with stripANSI=false from
formatters/tree_formatter.go's usage and stripANSI=true from api/meta.go's
usage; ensure the new helper preserves the current behavior (trim trailing
spaces, drop blank lines after optional ANSI strip) and replace the local
normalizeTreeLabel with calls to the shared helper.
In `@task/manager_lifecycle.go`:
- Around line 156-189: The renderFinal method duplicates task snapshot logic;
replace the manual snapshot creation in renderFinal with a call to
tm.snapshotTasks() (the helper in render_hook.go) to avoid duplication and
ensure consistent snapshot semantics across render paths — locate renderFinal
and swap the block that creates taskSnapshot (the make/copy and mutex calls
around tm.tasks) to use taskSnapshot := tm.snapshotTasks(), preserving
subsequent use of taskSnapshot and existing bufferMutex/renderer logic.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 12939824-ea51-48c4-910c-f43d7768cad5
📒 Files selected for processing (13)
api/keyvalue_test.goapi/meta.goapi/meta_tree_normalize_test.goapi/text.goentity.goentity_filters_test.goformatters/tree_formatter.goformatters/tree_formatter_test.gotask/manager.gotask/manager_lifecycle.gotask/render.gotask/render_hook.gotask/render_hook_test.go
…alog Introduce a comprehensive docs generation system that creates markdown CLI references and clicky-ui surface catalogs from a CLI's command tree. Features include: - New `docs` command group with `generate` subcommand supporting multiple output formats (markdown, json, yaml) - Configurable docs site scaffolding for Astro Starlight with write-once starter pages and regenerated reference pages - Per-controller documentation pages with depth-limiting to control command hierarchy inclusion - UI surface catalog generation from clicky metadata with parameter role classification - Integration with CobraExtensions for easy wiring into host CLIs - Comprehensive test coverage for model building, rendering, and scaffolding Expose ParamRole function in rpc package for reuse by docs generator. Update example to demonstrate docs configuration. Refs: docs generation feature
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@docs/cobra_walk.go`:
- Around line 65-69: nonEmptyDefault currently treats any DefValue == "false" as
empty, which drops string defaults that are literally "false"; change
nonEmptyDefault to only treat "false" as empty when the flag is a boolean flag.
Update the function nonEmptyDefault(flag *pflag.Flag) to check the flag's type
via flag.Value.Type() or equivalent (or use flag.Value.IsBoolFlag() if
available) and only return nil for DefValue == "false" when the flag is boolean;
otherwise return flag.DefValue unchanged.
In `@docs/command.go`:
- Around line 64-73: Add an explicit conflict check that returns an error if
both outputDir and outputFile are set: before choosing between scaffolding and
single-file flows (the block that calls runScaffold and the other branch around
RenderSingleFile/writeSingle), detect if outputDir != "" && outputFile != "" and
return a clear error (e.g., fmt.Errorf("flags --output and --output-dir cannot
be used together")). Update the logic around runScaffold, RenderSingleFile, and
writeSingle so the conflict is handled early and both locations perform the same
validation.
In `@docs/config.go`:
- Around line 39-44: The depth() logic contradicts the docs by treating c.Depth
== 0 as default; change it so depth() returns defaultDepth only when c is nil,
and treat both c.Depth == 0 and c.Depth < 0 as unlimited by returning
unlimitedDepth; update the conditional order in depth() to check c == nil first,
then c.Depth == 0 || c.Depth < 0 -> unlimitedDepth, otherwise return the
explicit c.Depth (use the existing symbols c.Depth, defaultDepth,
unlimitedDepth, and the depth() function).
In `@docs/provider.go`:
- Around line 54-57: The cleanBasePath function currently normalizes separators
but does not prevent path traversal; update cleanBasePath to split the
normalized path (e.g., strings.Split(p, "/")) and reject or return an
error/empty string if any segment equals "." or "..", and ensure callers that
use basePath for output path construction (where basePath is passed into
scaffolding/writes) check the cleaned result and refuse to use paths containing
those invalid segments; reference the cleanBasePath function and the code paths
where basePath is used for writing output so you validate and fail early when
traversal segments are present.
In `@docs/scaffold.go`:
- Around line 29-31: The code trusts provider.RelPath(page) and uses rel to
build abs (filepath.Join(dir, rel)) which allows absolute paths or “..” to
escape dir; before calling os.MkdirAll or os.WriteFile validate and canonicalize
the target: compute abs via filepath.Join(dir, rel), then call filepath.Clean
and filepath.Abs (or filepath.Join + filepath.Clean + filepath.Rel) and verify
that the resulting path has dir as its prefix (e.g., filepath.Rel(dir, abs) does
not start with ".." and is not an absolute escape); if the check fails, return
an error and skip MkdirAll/WriteFile. Apply the same containment check where
rel/abs are used around the MkdirAll/WriteFile blocks (lines referenced around
provider.RelPath, rel, abs, MkdirAll, WriteFile).
In `@extensions/cobra.go`:
- Around line 62-63: AllWithConfig() currently diverges from All() by not
attaching the DocsCommand, causing configured setups to miss docs registration;
update the fluent chain in AllWithConfig() (the method that composes
OpenAPICommand(), MCPCommand(), etc.) to also call DocsCommand() before
returning so its behavior matches All()—i.e., ensure the chain includes
OpenAPICommand().MCPCommand().DocsCommand() (or equivalent ordering) using the
same command-building helpers.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ef969187-9372-47ea-927c-58bd937fbaa3
📒 Files selected for processing (15)
docs/cobra_walk.godocs/command.godocs/config.godocs/docs_test.godocs/markdown.godocs/model.godocs/pages.godocs/provider.godocs/reference.godocs/render.godocs/scaffold.godocs/surfaces.goexamples/enitity/main.goextensions/cobra.gorpc/openapi.go
Comment on lines
+39
to
+44
| if c == nil || c.Depth == 0 { | ||
| return defaultDepth | ||
| } | ||
| if c.Depth < 0 { | ||
| return unlimitedDepth | ||
| } |
Contributor
There was a problem hiding this comment.
Depth == 0 behavior contradicts documented semantics.
The struct docs state “Depth 0 means unlimited,” but depth() maps 0 to defaultDepth (Line 39–40). This makes unlimited mode unreachable via the documented config value.
Suggested patch
func (c *DocsConfig) depth() int {
- if c == nil || c.Depth == 0 {
+ if c == nil {
return defaultDepth
}
- if c.Depth < 0 {
+ if c.Depth == 0 || c.Depth < 0 {
return unlimitedDepth
}
return c.Depth
}📝 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.
Suggested change
| if c == nil || c.Depth == 0 { | |
| return defaultDepth | |
| } | |
| if c.Depth < 0 { | |
| return unlimitedDepth | |
| } | |
| if c == nil { | |
| return defaultDepth | |
| } | |
| if c.Depth == 0 || c.Depth < 0 { | |
| return unlimitedDepth | |
| } |
🤖 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 `@docs/config.go` around lines 39 - 44, The depth() logic contradicts the docs
by treating c.Depth == 0 as default; change it so depth() returns defaultDepth
only when c is nil, and treat both c.Depth == 0 and c.Depth < 0 as unlimited by
returning unlimitedDepth; update the conditional order in depth() to check c ==
nil first, then c.Depth == 0 || c.Depth < 0 -> unlimitedDepth, otherwise return
the explicit c.Depth (use the existing symbols c.Depth, defaultDepth,
unlimitedDepth, and the depth() function).
Contributor
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 `@docs/scaffold.go`:
- Around line 36-37: Validate the page key before joining paths: ensure page.Key
(used by pageRelPath) does not contain path separators or traversal segments
(e.g., '/', '\\', or "..") and that the cleaned relative path equals the
original relative path; if validation fails, return an error or skip the page
instead of calling filepath.Join(dir, rel) to build abs. Target the code around
pageRelPath, rel and the filepath.Join(dir, rel) call and add the check there
(or inside pageRelPath) to defensively reject unsafe keys.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 98b88709-8e4b-4af4-975b-ae3b8e22ad72
📒 Files selected for processing (7)
.gitignoredocs/command.godocs/config.godocs/docs_test.godocs/pages.godocs/provider.godocs/scaffold.go
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/pages.go
… instead of scaffolding provider-based sites Remove the provider abstraction (astro, base-path, etc.) and simplify docs generation to write one markdown file per controller directly into the output directory as plain markdown without frontmatter. Changes: - Remove Provider interface and astroProvider implementation - Remove --provider and --base-path CLI flags - Remove DefaultProvider from DocsConfig - Simplify Scaffold() to write flat files with pageRelPath() instead of provider.RelPath() - Update command help text and examples to reflect new flat output structure - Add assertNoPathCollisions() to prevent filename collisions between controllers and starter pages - Update tests to verify flat file output without frontmatter BREAKING CHANGE: --output-dir now writes one markdown file per controller directly into the directory instead of scaffolding a provider-based docs site. The --provider and --base-path flags are removed. Generated files no longer include YAML frontmatter.
moshloop
force-pushed
the
feat/pluggable-live-renderer
branch
from
June 9, 2026 16:48
959e6cf to
dc3a3ee
Compare
|
🎉 This PR is included in version 1.21.17 |
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What
normalizeTreeLabel()TypedFilterinterface to allow filters to override their lookup type independently of flag typeWhy
Notes
Summary by CodeRabbit
New Features
Bug Fixes
Tests