feat(api,ui,example): add time range filtering and entity detail page navigation Implement time range filtering with from/to parameters replacing the previous window-based approach. Add entity detail page with row action support and response body viewing. Update API to support both application/clicky+json and application/json+clicky content types. Add StopTask function to global task API for canceling specific tasks. Upgrade dependencies including Go 1.26.1 and charmbracelet UI libraries.#98
Open
feat(api,ui,example): add time range filtering and entity detail page navigation Implement time range filtering with from/to parameters replacing the previous window-based approach. Add entity detail page with row action support and response body viewing. Update API to support both application/clicky+json and application/json+clicky content types. Add StopTask function to global task API for canceling specific tasks. Upgrade dependencies including Go 1.26.1 and charmbracelet UI libraries.#98
Conversation
…nd completions Implement Filter interface and lookup functions to enable dynamic filter metadata exposure through the RPC executor. This allows UI components to display available and selected filter options with rich formatting (styles, tooltips) and supports dependent filter narrowing based on other filter selections. Key additions: - Filter[T] interface for defining filter behavior with Key(), Label(), Lookup(), and Options() methods - buildLookupFunc() generates lookup metadata from filter definitions - buildFilterCompletionBinder() registers flag completion functions for shell and UI - resolveEntityOpts() applies filters to transform raw flag values into backend-canonical forms - LookupFunc field on EntityOperation and BulkActionInfo for RPC exposure - BindCompletions field for registering completion functions on generated commands - New clicky_lookup.go with entityLookupResponse and clickyNode types for rich filter metadata - RPC executor support for HEAD and GET __lookup=filters requests - Comprehensive test coverage in entity_filters_test.go and rpc/filter_lookup_test.go - End-to-end example in examples/entity/ demonstrating filters, admin entities, and executor-backed UI Breaking changes: buildOpts() now returns (T, error) instead of T; callers must handle error return value.
…and text input Introduce comprehensive prompt functionality for interactive terminal UIs: - Vendor gumchoose from charmbracelet/gum for single and multi-select prompts - Add PromptSelect, PromptMultiSelect, and PromptText APIs for interactive user input - Implement terminal detection, PTY handling, and screen management - Support customizable rendering, validation, and pagination - Include extensive test coverage with PTY-based integration tests - Handle terminal state restoration and cursor positioning This enables rich interactive CLI experiences with proper terminal handling across macOS and Linux.
… ownership management Implement exclusive ANSI terminal ownership control to allow prompts to safely take over the terminal from task renderers. Add new task-prompt demo command showcasing interactive select, multi-select, and text input prompts integrated with concurrent task execution. Key changes: - Add AcquirePromptTerminal() for prompt-to-task renderer handoff - Implement ansiTerminalController to coordinate terminal access - Add SetNoRender() to suppress all task output including final summaries - Add StopTask(id) to cancel specific tasks by immutable UUID - Add Task.ID() returning stable UUID independent of task name - Update go.mod dependencies (Go 1.26.1, commons v1.50.3, gomplate v3.24.77) - Add comprehensive tests for no-render and stop-task functionality This enables building interactive CLI applications where prompts can safely interrupt task rendering and restore it afterward.
…/json+clicky Change the primary content type for clicky JSON format from application/clicky+json to application/json+clicky to follow standard IANA media type conventions. The HTTP layer now accepts both the new standard format and the legacy alias for backward compatibility. BREAKING CHANGE: The Content-Type header for clicky JSON responses is now application/json+clicky instead of application/clicky+json. Clients should update their expectations, though the legacy format is still accepted in Accept headers.
… navigation Implement time range filtering with from/to parameters replacing the previous window-based approach. Add entity detail page with row action support and response body viewing. Update API to support both application/clicky+json and application/json+clicky content types. Add StopTask function to global task API for canceling specific tasks. Upgrade dependencies including Go 1.26.1 and charmbracelet UI libraries. BREAKING CHANGE: stackWindowOpts now uses From and To time.Time fields instead of UpdatedSince and Window duration fields. Content-Type header for clicky-json format changed from application/clicky+json to application/json+clicky. Refs: entity demo tests, example app routing, API response handling
Contributor
WalkthroughThis PR introduces entity filters with dynamic lookup metadata rendering, interactive CLI prompts with terminal ownership coordination, task cancellation APIs, and a comprehensive entity demonstration example. Core additions include filter-driven UI support, lookup operation endpoints for metadata delivery, and task management enhancements. Changes
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![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: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
entity.go (2)
533-542:⚠️ Potential issue | 🟠 MajorAllow
GetWithFlagsto use--idwithout a positional arg.Line 542 rejects zero positional args before
DataFunccan readflagMap["id"], making the new--idfallback unreachable.Allow optional positional ID for flagged get commands
func generateIDCommand(parent *cobra.Command, verb, short string, op EntityOperation, validArgs func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective)) { hasFlags := op.FlagsType != nil + argsValidator := cobra.ExactArgs(1) use := fmt.Sprintf("%s <id>", verb) if hasFlags { use = fmt.Sprintf("%s <id> [flags]", verb) + } + if hasFlags && op.Verb == "get" { + use = fmt.Sprintf("%s [id] [flags]", verb) + argsValidator = cobra.MaximumNArgs(1) } cmd := &cobra.Command{ Use: use, Short: short, - Args: cobra.ExactArgs(1), + Args: argsValidator,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@entity.go` around lines 533 - 542, The command currently forces one positional arg via Args: cobra.ExactArgs(1) which prevents using --id flag fallback in generateIDCommand; change the Args to allow 0 or 1 args when hasFlags is true (e.g., use cobra.MaximumNArgs(1) or a custom validation func that returns no error for 0/1 if hasFlags, otherwise enforce exactly 1) so DataFunc and the flag lookup (flagMap["id"]) can read --id when no positional ID is provided; update the Args on the cobra.Command created in generateIDCommand accordingly.
642-653:⚠️ Potential issue | 🟠 MajorLet bulk filter mode run without dummy IDs.
execute()supports--filter, but Line 653 still requires at least one ID, so filter-mode bulk commands cannot run as intended unless users pass an irrelevant positional arg.Make IDs optional when `--filter` is set
func generateBulkActionCommand(parent *cobra.Command, ba BulkActionInfo) { execute := func(flagMap map[string]string, args []string) (any, error) { if ba.FilterFunc != nil && flagMap["filter"] != "" { return ba.FilterFunc(flagMap, args) } return ba.DataFunc(flagMap, args) } + use := fmt.Sprintf("%s <id> [id...]", ba.Name) + argsValidator := cobra.MinimumNArgs(1) + if ba.FilterFunc != nil { + use = fmt.Sprintf("%s [id...] [flags]", ba.Name) + argsValidator = func(c *cobra.Command, args []string) error { + if f := c.Flags().Lookup("filter"); f != nil && f.Value.String() != "" { + return nil + } + return cobra.MinimumNArgs(1)(c, args) + } + } + cmd := &cobra.Command{ - Use: fmt.Sprintf("%s <id> [id...]", ba.Name), + Use: use, Short: ba.Short, - Args: cobra.MinimumNArgs(1), + Args: argsValidator,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@entity.go` around lines 642 - 653, The command currently forces at least one positional ID via Args: cobra.MinimumNArgs(1) which prevents running generateBulkActionCommand in pure filter mode; change the command argument validation to a custom validator that checks the "filter" flag (accessible via cmd.Flags().GetString("filter") or similar) and allows zero positional args when ba.FilterFunc is present and the filter flag is non-empty, otherwise enforce cobra.MinimumNArgs(1). Update the cobra.Command Args from the static MinimumNArgs to this validator so execute (and ba.FilterFunc/ba.DataFunc) can run correctly without dummy IDs.task/options.go (1)
104-131:⚠️ Potential issue | 🟡 MinorExpose
NoRenderin the flag binders too.
Apply()honorsNoRender, butBindManagerFlagsandBindManagerPFlagsnever bind it, so CLIs using these helpers cannot enable the new render suppression option.🔧 Proposed fix
func BindManagerFlags(flags *flag.FlagSet, options *ManagerOptions) { flags.BoolVar(&options.NoProgress, "no-progress", options.NoProgress, "Disable progress display") + flags.BoolVar(&options.NoRender, "no-render", options.NoRender, + "Disable all task rendering") flags.IntVar(&options.MaxConcurrent, "max-concurrent", options.MaxConcurrent, "Maximum concurrent tasks (0 = unlimited)") flags.DurationVar(&options.GracefulTimeout, "graceful-timeout", options.GracefulTimeout, "Timeout for graceful shutdown on interrupt") @@ func BindManagerPFlags(flags *pflag.FlagSet, options *ManagerOptions) { flags.BoolVar(&options.NoProgress, "no-progress", options.NoProgress, "Disable progress display") + flags.BoolVar(&options.NoRender, "no-render", options.NoRender, + "Disable all task rendering") flags.IntVar(&options.MaxConcurrent, "max-concurrent", options.MaxConcurrent, "Maximum concurrent tasks (0 = unlimited)") flags.DurationVar(&options.GracefulTimeout, "graceful-timeout", options.GracefulTimeout, "Timeout for graceful shutdown on interrupt")Also applies to: 143-170
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@task/options.go` around lines 104 - 131, ManagerOptions exposes NoRender and Apply respects it, but BindManagerFlags and BindManagerPFlags never bind the corresponding CLI flag; update both BindManagerFlags and BindManagerPFlags to add a boolean flag (e.g. "--no-render") bound to opts.NoRender with the same default/description style used for NoProgress (use opts.NoRender as the target variable and a helpful help string like "Disable all task rendering"), mirroring how NoProgress is registered so CLIs can toggle render suppression.go.mod (1)
3-3:⚠️ Potential issue | 🟠 MajorUpdate to the current Go 1.26 security patch.
go 1.26.1is behindgo1.26.2, the latest available 1.26 patch with security fixes. Since this PR already upgrades the Go baseline, use the latest 1.26 patch unless there is a known incompatibility.Proposed fix
-go 1.26.1 +go 1.26.2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go.mod` at line 3, Update the Go module's go directive from 1.26.1 to the latest security patch 1.26.2: locate the go directive line (currently "go 1.26.1" in go.mod) and change it to "go 1.26.2" so the project uses the up-to-date Go 1.26 security patch.
🧹 Nitpick comments (8)
examples/enitity/webapp/tsconfig.tsbuildinfo (1)
1-1: Remove the generated TypeScript build cache.
tsconfig.tsbuildinfois compiler output, not source. Please remove it and add*.tsbuildinfoto the example webapp’s.gitignore; also double-check whether theexamples/enititydirectory name is intended.Suggested ignore entry
+*.tsbuildinfo🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/enitity/webapp/tsconfig.tsbuildinfo` at line 1, Remove the generated TypeScript build cache file tsconfig.tsbuildinfo from the commit (it is compiler output, not source), add the pattern *.tsbuildinfo to the example webapp’s .gitignore so future buildinfo files are ignored, and verify/rename the examples/enitity directory if it is a typo (likely should be examples/entity); look for references to the listed root files (./src/app.tsx, ./src/api.ts, ./src/domains.ts, ./src/main.tsx) to ensure renaming does not break paths.task/terminal_owner.go (2)
109-131: Document that the release function does not restart the displaced task renderer.
AcquirePromptTerminalstops the active task renderer viamanager.stopRender()and returns a release closure that only callsreleasePrompt()— it does not restart the renderer. The second return value hints at this (indicating displacement happened) but the contract isn't stated. Consider expanding the doc so callers know they're responsible for re-engaging task rendering if they need it after the prompt, e.g. by checking the second return value and calling the appropriate lifecycle function.// AcquirePromptTerminal waits for exclusive ANSI terminal ownership for prompt // rendering, stopping the active task renderer first when necessary. // The second return value reports whether the prompt displaced an active task renderer. +// The returned release function only yields terminal ownership; it does NOT +// restart the task renderer that was stopped. Callers that displaced an active +// renderer (second return value == true) are responsible for restarting task +// rendering afterwards if desired. func AcquirePromptTerminal() (func(), bool) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@task/terminal_owner.go` around lines 109 - 131, Update the AcquirePromptTerminal doc comment to explicitly state that the returned release closure only calls globalANSITerminal.releasePrompt() and does not restart any renderer stopped by manager.stopRender(); note that the second return bool only indicates a renderer was displaced and callers are responsible for re-starting or re-engaging task rendering (e.g., by checking the bool and invoking the appropriate lifecycle method) after calling the release function.
68-81: Minor:acquirePromptimplicitly trusts that a prompt waiter was registered.
acquirePromptdecrementspromptWaitersonlyif c.promptWaiters > 0, and the function assumes it's always called afterbeginPromptAcquire(which increments the counter). If some future refactor ever callsacquirePromptstandalone, the counter accounting will silently drift from whattryAcquireTaskRendererobserves. Consider making the pairing explicit (e.g., rename tofinishPromptAcquireor assertpromptWaiters > 0) so the invariant is self-documenting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@task/terminal_owner.go` around lines 68 - 81, acquirePrompt currently decrements promptWaiters only when >0 and assumes it was paired with beginPromptAcquire, which can silently hide mismatches; modify acquirePrompt (or rename to finishPromptAcquire) to enforce the pairing by asserting that c.promptWaiters > 0 before decrementing (e.g., if c.promptWaiters == 0 { panic("acquirePrompt called without matching beginPromptAcquire") }), or alternatively rename the method to finishPromptAcquire to document the required pairing, and update any callers (including tryAcquireTaskRenderer) accordingly so the invariant is explicit and fails fast on misuse.examples/enitity/webapp/src/App.tsx (1)
117-122: Consider an explicit catch-all route for better UX.
/:domainKeywill match any single-segment path, so e.g. navigating to/commands(without an:operationId) or any typo rendersDomainPagewith "Unknown domain: commands" instead of a proper not-found page. Adding an explicitpath="*"route after the others gives a clearer fallback and frees/:domainKeyfrom doubling as a 404 handler.<Route path="/" element={<Navigate to="/stacks" replace />} /> <Route path="/commands/:operationId" element={<CommandRoute />} /> <Route path="/entity/:domainKey/:id" element={<EntityRoute />} /> <Route path="/:domainKey" element={<DomainPage />} /> + <Route path="*" element={<div className="p-6 text-sm text-muted-foreground">Not found</div>} />Also, nit: the directory is spelled
enitity(should beentity) — not blocking since it spans many files, but worth renaming while the example is still fresh.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/enitity/webapp/src/App.tsx` around lines 117 - 122, The Routes currently place Route path="/:domainKey" (rendering DomainPage) before any catch-all, causing single-segment typos like "/commands" to resolve to DomainPage; add an explicit wildcard Route (path="*") after the existing Route entries that renders a NotFound component (or Navigate to a 404 page) so unknown or malformed paths show a proper not-found UX, and ensure the order in the Routes list keeps specific routes (e.g., Route path="/commands/:operationId" -> CommandRoute, Route path="/entity/:domainKey/:id" -> EntityRoute) before the wildcard; optionally, rename the enitity directory to entity across the example to fix the typo.examples/enitity/webapp/src/domains.ts (1)
14-64: Preserve literal domain keys fordomainOrder.
Record<string, DomainSpec>widenskeyof typeof domainstostring, sodomainOrderwon’t catch typos. Consider defining a literalDomainKeyunion first.♻️ Proposed type-safety refactor
+const domainKeys = ["stacks", "clusters", "admin-stacks", "explorer"] as const; +type DomainKey = (typeof domainKeys)[number]; + -export const domains: Record<string, DomainSpec> = { +export const domains: Record<DomainKey, DomainSpec> = { stacks: { definition: { key: "stacks", @@ -}; +}; -export const domainOrder: Array<keyof typeof domains> = [ - "stacks", - "clusters", - "admin-stacks", - "explorer", -]; +export const domainOrder: DomainKey[] = [...domainKeys];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/enitity/webapp/src/domains.ts` around lines 14 - 64, The domains object is typed as Record<string, DomainSpec>, which widens its keys and makes domainOrder: Array<keyof typeof domains> become string (losing typo-safety); fix by introducing a literal DomainKey union that enumerates "stacks" | "clusters" | "admin-stacks" | "explorer" (or derive it from a const assertion) and then type domains as Record<DomainKey, DomainSpec> and domainOrder as DomainKey[]; update any usage of domains or domainOrder accordingly (referencing the domains constant and the domainOrder variable).prompt.go (3)
221-225: Prompt errors are silently swallowed.
newPromptSessioncan fail for legitimate reasons (no TTY available,/dev/ttyopen error) but all three public entry points (PromptMultiSelect,Prompt/PromptSelect/PromptChoose,PromptText) return the same(_, false)as a user-cancellation. Callers cannot distinguish "user aborted" from "no interactive terminal" or other runtime failures, which makes automation and error reporting awkward.Consider either (a) adding an error-returning variant (
PromptSelectE, etc.) or (b) logging viaclicky's existing logger at debug level when a non-abort error is dropped, so these cases are at least observable.Also applies to: 272-275, 305-310
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@prompt.go` around lines 221 - 225, newPromptSession errors are being swallowed and treated as user cancellations; update each caller (PromptMultiSelect, Prompt/PromptSelect/PromptChoose, PromptText) to detect a non-nil err returned from newPromptSession and log the error at debug level via the package logger (clicky or existing logger) before returning the existing nil,false result so automated callers can at least observe runtime failures; apply the same change to the other two newPromptSession call sites so non-abort errors are logged consistently.
573-605:clampPromptPageSizesilently overrides an explicit smallPageSize.If a caller passes e.g.
PageSize: 3with 20 items and plenty of terminal rows, the final clampif pageSize < minPageSize { pageSize = minPageSize }bumps it up topromptMinSelectPageSize(5). The floor is useful whenpageSize <= 0(defaulting), but applying it to an explicit positive value effectively ignores caller intent. Consider only applying the floor when the caller did not supply a value, or whenmaxPageSizeforces us below it.♻️ Suggested tweak
- if pageSize <= 0 { - pageSize = maxPageSize - } else if pageSize > maxPageSize { - pageSize = maxPageSize - } - if pageSize > itemCount { - pageSize = itemCount - } - if pageSize < minPageSize { - pageSize = minPageSize - } - return pageSize + explicit := pageSize > 0 + if !explicit { + pageSize = maxPageSize + } else if pageSize > maxPageSize { + pageSize = maxPageSize + } + if pageSize > itemCount { + pageSize = itemCount + } + if !explicit && pageSize < minPageSize { + pageSize = minPageSize + } + return pageSize🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@prompt.go` around lines 573 - 605, clampPromptPageSize currently forces an explicit positive pageSize up to promptMinSelectPageSize; change it so the floor (minPageSize) is only applied when the caller did not supply a value (pageSize <= 0) or when the computed maxPageSize is smaller than the configured min (i.e., maxPageSize < minPageSize) which forces the floor down; specifically, keep computing minPageSize and maxPageSize as-is, set pageSize = maxPageSize when pageSize <= 0, clamp pageSize down to maxPageSize and itemCount when pageSize > maxPageSize or > itemCount, and only apply pageSize = minPageSize when (pageSize <= 0) OR (maxPageSize > 0 && maxPageSize < minPageSize); update the logic in clampPromptPageSize to reflect this and reference variables pageSize, minPageSize, maxPageSize, promptMinSelectPageSize, itemCount, terminalLines, chromeHeight.
127-152:cursorRowcan discard user keystrokes and busy-spins on POLLHUP.Two minor concerns with the cursor-row probe:
- While the terminal is in raw mode,
readPromptCursorResponsedrains bytes one at a time until it seesR. Anything a user happens to type between sendingESC[6nand the terminal's response is consumed and thrown away. In practice this is a very narrow window (~150ms), but worth noting since this runs on everyprepareSelectPromptcall.- In
readPromptCursorResponse, ifpollreturnsready > 0with onlyPOLLHUP/POLLERR(noPOLLIN), the loop hitscontinueand re-polls with a 1ms minimum timeout, tight-looping until the 150ms deadline. Bailing out explicitly on those revents would be cleaner:♻️ Suggested tweak
- if ready == 0 || pollfds[0].Revents&unix.POLLIN == 0 { - continue - } + if pollfds[0].Revents&(unix.POLLHUP|unix.POLLERR|unix.POLLNVAL) != 0 { + return nil, false + } + if ready == 0 || pollfds[0].Revents&unix.POLLIN == 0 { + continue + }Also applies to: 509-554
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@prompt.go` around lines 127 - 152, The probe in cursorRow consumes any user keystrokes while in raw mode and can busy-loop on POLLHUP/POLLERR; update readPromptCursorResponse to only perform reads when poll reports POLLIN and to immediately return (ok=false) when poll revents contain POLLHUP or POLLERR, and change its read loop to accumulate bytes but treat any leading bytes that are not the expected ESC '[' sequence as unconsumed input (either push them back via TIOCSTI or store them in a small unconsumed buffer on promptTerminal) so those keystrokes aren’t lost; keep references to promptCursorQuerySequence, promptCursorQueryTimeout, readPromptCursorResponse, parsePromptCursorResponse and cursorRow so the changes are made in the cursor probe code path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@entity.go`:
- Around line 871-895: describeLookupField currently detects Multi when the
field is a slice but classifies type based on the slice kind itself, so multi
filters like []int or []time.Time never get their element types mapped; update
describeLookupField to, after detecting Slice and setting meta.Multi, unwrap the
element type (e.g., fieldType = fieldType.Elem()) before the switch/type checks
so isNumericKind, the time.Time check and isRangeStartFlag/isRangeEndFlag logic
operate on the slice element type; ensure pointer-unwrapping still happens first
and keep entityLookupMetadata.Multi assignment using the original slice
detection.
- Around line 332-338: The admin EntityInfo constructed as adminInfo is missing
the Parent field, so admin commands lose their nesting; update the adminInfo
creation (EntityInfo adminInfo) to copy the parent from the original info (e.g.,
set Parent: info.Parent) so GenerateCLI sees the same entity.Parent and
registers admin commands under the correct parent group.
In `@examples/enitity/e2e/.gitignore`:
- Around line 1-4: Filename and paths contain a typo: the directory name
"enitity" should be "entity"; update the repository structure and any references
so the e2e .gitignore and its parent directory use "entity" instead of "enitity"
(rename the directory or move the file, and update any CI/README/PR references
that point to "enitity") to ensure consistency with the project naming.
In `@examples/enitity/e2e/playwright.config.ts`:
- Around line 36-43: The webServer.command string in the Playwright config
interpolates environment-derived values (HOST, PORT, GOCACHE, exampleDir,
BASE_URL) directly into a shell command which can break if those values contain
spaces or shell metacharacters; change webServer.command so those values are
properly escaped or passed as separate argv (e.g., use a shell-escape utility or
construct the command with safe-quoted/escaped values or switch to a
spawn/execFile-style invocation with arguments) ensuring the variables HOST,
PORT, GOCACHE, exampleDir and any paths are sanitized/quoted before
interpolation in the webServer.command property.
In `@examples/enitity/main_test.go`:
- Around line 8-16: Test is flaky because it computes From/To with separate
time.Now() calls; change TestMatchingStacksLockedHonorsFromAndTo to capture a
single now before building stackWindowOpts (e.g. now := time.Now().UTC()) and
use now.Add(-24*time.Hour) and now for From/To, and add a small tolerance (e.g.
+-1ms) or modify newDemoStore() to accept an injected fixed clock so
matchingStacksLocked sees deterministic timestamps; update references in the
test to use the captured now and/or the fixed clock so stackWindowOpts.From/To
cannot accidentally exclude boundary records.
In `@examples/enitity/main.go`:
- Around line 534-545: Normalize/canonicalize the filter inputs (e.g., convert
opts.Team and opts.Status to the same canonical form used when storing stacks)
before comparing in matchingStacksLocked so slug/label inputs like "platform" or
"healthy" match stored canonical values; update the comparisons in
matchingStacksLocked to compare canonicalized opts.Team and opts.Status (and
apply the same normalization to the separate filtering block referenced around
lines 616-623) so filters match stored stack values consistently.
In `@examples/enitity/webapp/src/index.css`:
- Around line 1-3: Add a local Stylelint suppression immediately before the
Tailwind `@source` directive to silence the scss/at-rule-no-unknown warning:
insert a single-line disable comment for `scss/at-rule-no-unknown` directly
above the `@source "../node_modules/@flanksource/clicky-ui/dist";` line in
index.css so the linter ignores that specific unknown at-rule without disabling
the rule globally.
In `@examples/go.mod`:
- Line 51: The go.mod entry for github.com/go-jose/go-jose/v3 is pinned to the
vulnerable v3.0.4; update that module version to the patched v3.0.5 in this file
and also update the other occurrences mentioned (root go.mod and
examples/uber_demo/go.mod) so all instances are consistent, then refresh module
metadata (e.g., run your normal dependency update/tidy step) to ensure the new
version is used; look for the exact module string
"github.com/go-jose/go-jose/v3" to locate and change the version in each file.
In `@formatters/http/http.go`:
- Around line 243-244: Change every occurrence that returns
"application/json+clicky" to the correct RFC form "application/clicky+json":
update the formatToContentType implementation(s) (there are duplicate
implementations) so they return the suffix-last form, replace the hardcoded
media type used in the lookup response handler, and update the corresponding
tests to assert "application/clicky+json" instead of the old value; while at it,
consolidate the duplicate formatToContentType functions into a single shared
helper (rename/move one implementation and point callers to that function) so
future changes affect only one place.
In `@internal/gumchoose/choose.go`:
- Around line 139-157: In Run, guard against nil/empty items before creating the
model/program: if items == nil || len(items) == 0 return a non-submitted Result
with an empty Indexes slice (and no error) instead of calling
newModel/tea.NewProgram; this prevents model methods (model.selectedIndexes,
model.submitted, submit/toggle paths) from dereferencing m.items[0] or using
index 0 when there are no options.
In `@prompt_test.go`:
- Around line 1-31: Add a Unix-only build constraint at the top of the file
(e.g., “//go:build unix” with the matching “// +build unix” comment if needed)
so the PTY test file compiles only on Unix platforms, and replace usages of
syscall.SIGKILL with the portable Process.Kill() call (update code paths that
signal a process to use proc.Kill() rather than syscall.SIGKILL) and remove any
direct syscall import-only references; keep existing runtime guard
skipPromptPTYTests() but ensure compilation no longer depends on non-Unix
symbols like syscall.SIGKILL.
In `@prompt.go`:
- Around line 1-19: This file uses Unix-only APIs (/dev/tty, unix.Poll,
unix.Read) but has no platform build constraint; add a top-level "//go:build
unix" (and matching +build comment) to prompt.go so it only compiles on Unix,
and create a Windows stub file (e.g., prompt_windows.go) that provides the same
exported functions/types used by the rest of the package (or returns appropriate
errors) to allow Windows builds; ensure the stub implements the same function
signatures referenced in this file (calls that rely on unix.Poll, unix.Read, and
opening /dev/tty) so consumers compile on Windows.
In `@rpc/executor.go`:
- Around line 99-139: FindLookupOperation currently falls back to other HTTP
verbs (GET, POST, PUT, DELETE, PATCH) which causes lookups registered for
different methods to be matched; change it to respect the incoming method only
(alias HEAD to GET) so it behaves like FindOperation: if method is HEAD set
method = http.MethodGet, then call findLookupOperationForMethod with that single
method and remove the candidate-method loop. Also simplify
findLookupOperationForMethod by removing the pointless preferredMethods
slice/loop and directly match op.Method (case-insensitive) and op.LookupFunc !=
nil, returning the op when matchTemplatePath(op.Path, path) is true. Ensure no
other cross-method scanning remains.
In `@rpc/serve_test.go`:
- Around line 442-443: The media-type suffix order is reversed: change
formatToContentType implementations in rpc/serve.go and formatters/http/http.go
to emit "application/clicky+json" (not "application/json+clicky") for the
"clicky-json" formatter, and update tests asserting formatToContentType in
rpc/serve_test.go and formatters/http/http_test.go to expect
"application/clicky+json"; locate the code paths by the formatToContentType
function and any switch/case or mapping for "clicky-json" and update the emitted
string and test assertions accordingly.
In `@rpc/serve.go`:
- Around line 622-630: The handler currently calls w.WriteHeader(http.StatusOK)
before json encoding, so a 200 is sent even if json.NewEncoder(w).Encode(data)
fails; instead, encode the response into a temporary buffer (e.g., bytes.Buffer)
first using json.NewEncoder(&buf).Encode(data), and only if that succeeds set
the Content-Type header and call w.WriteHeader(http.StatusOK) and then write the
buffer to w; for HEAD requests, still return after writing headers but do not
write the body. Update references to json.NewEncoder(w).Encode(data),
w.WriteHeader(http.StatusOK), r.Method/ http.MethodHead, and the variable data
accordingly.
In `@task/manager.go`:
- Around line 371-378: The problem: renderStarted (the sync.Once used in
Manager.enqueue) is being consumed even when startRenderLoop() returns early
(fails to acquire the interactive renderer), preventing future retries; fix it
by ensuring we only mark rendering as started when the render loop actually
began. Change enqueue to acquire the terminal/renderer (or call the portion of
startRenderLoop that can fail to obtain the TTY) before invoking
renderStarted.Do, or replace renderStarted (sync.Once) with a mutex-protected
startedSuccessfully boolean and only set startedSuccessfully=true after
startRenderLoop completes successfully; update references to renderStarted.Do,
startRenderLoop, noRender, and noProgress accordingly so the Once is not
consumed on early return.
In `@task/snapshot.go`:
- Line 32: Snapshot filtering in SnapshotAll is still using group names
(g.Name()) while snapshots are now emitted with task IDs via t.ID(), so callers
get no matches; update the SnapshotAll implementation to build a set from the
input taskIDs and change the filter to compare the snapshot's emitted ID (the
field set from t.ID(), e.g., snapshot.ID or snapshot.TaskID depending on struct)
against that set instead of g.Name(), and make the same replacement for the
other filtering code paths mentioned (the block around the other occurrences) so
all filters use the emitted task ID.
In `@task/worker.go`:
- Around line 35-45: The skip path for cancelled tasks marks them completed
which hides cancellation from dependents; remove the task.completed.Store(true)
(and any logic that treats a cancelled task as fully completed) in the
cancellation branch where task.status == StatusCancelled so that
checkDependencies can still observe dependency.status; alternatively, update
checkDependencies to examine dependency.status (StatusCancelled/StatusFailed)
regardless of dependency.completed. Adjust the cancellation branch around
task.mu, task.signalDone and w.manager.tasksByIdentity.Delete(task.identity) as
needed so cancelled tasks are removed/cleaned up without flipping completed to
true, and ensure checkDependencies is updated to check status directly rather
than relying on completed.
In `@testdata/prompt_test_helper.go`:
- Around line 81-102: The test uses a fixed sleep in runTaskTakeover
(time.Sleep(600 * time.Millisecond)) which is flaky; replace the wall-clock
delay with a synchronization strategy that waits until the renderer signals
readiness before calling clicky.PromptSelect — e.g., have the task started via
clicky.StartTask set a known description marker with t.SetDescription("before
prompt TAKEOVER-MARKER") and then in the test poll/read stderr (or poll the task
description via any available API) for that TAKEOVER-MARKER or expose a
readiness channel from the task and wait on it, then call clicky.PromptSelect;
remove the fixed sleep to ensure deterministic behavior under load.
---
Outside diff comments:
In `@entity.go`:
- Around line 533-542: The command currently forces one positional arg via Args:
cobra.ExactArgs(1) which prevents using --id flag fallback in generateIDCommand;
change the Args to allow 0 or 1 args when hasFlags is true (e.g., use
cobra.MaximumNArgs(1) or a custom validation func that returns no error for 0/1
if hasFlags, otherwise enforce exactly 1) so DataFunc and the flag lookup
(flagMap["id"]) can read --id when no positional ID is provided; update the Args
on the cobra.Command created in generateIDCommand accordingly.
- Around line 642-653: The command currently forces at least one positional ID
via Args: cobra.MinimumNArgs(1) which prevents running generateBulkActionCommand
in pure filter mode; change the command argument validation to a custom
validator that checks the "filter" flag (accessible via
cmd.Flags().GetString("filter") or similar) and allows zero positional args when
ba.FilterFunc is present and the filter flag is non-empty, otherwise enforce
cobra.MinimumNArgs(1). Update the cobra.Command Args from the static
MinimumNArgs to this validator so execute (and ba.FilterFunc/ba.DataFunc) can
run correctly without dummy IDs.
In `@go.mod`:
- Line 3: Update the Go module's go directive from 1.26.1 to the latest security
patch 1.26.2: locate the go directive line (currently "go 1.26.1" in go.mod) and
change it to "go 1.26.2" so the project uses the up-to-date Go 1.26 security
patch.
In `@task/options.go`:
- Around line 104-131: ManagerOptions exposes NoRender and Apply respects it,
but BindManagerFlags and BindManagerPFlags never bind the corresponding CLI
flag; update both BindManagerFlags and BindManagerPFlags to add a boolean flag
(e.g. "--no-render") bound to opts.NoRender with the same default/description
style used for NoProgress (use opts.NoRender as the target variable and a
helpful help string like "Disable all task rendering"), mirroring how NoProgress
is registered so CLIs can toggle render suppression.
---
Nitpick comments:
In `@examples/enitity/webapp/src/App.tsx`:
- Around line 117-122: The Routes currently place Route path="/:domainKey"
(rendering DomainPage) before any catch-all, causing single-segment typos like
"/commands" to resolve to DomainPage; add an explicit wildcard Route (path="*")
after the existing Route entries that renders a NotFound component (or Navigate
to a 404 page) so unknown or malformed paths show a proper not-found UX, and
ensure the order in the Routes list keeps specific routes (e.g., Route
path="/commands/:operationId" -> CommandRoute, Route
path="/entity/:domainKey/:id" -> EntityRoute) before the wildcard; optionally,
rename the enitity directory to entity across the example to fix the typo.
In `@examples/enitity/webapp/src/domains.ts`:
- Around line 14-64: The domains object is typed as Record<string, DomainSpec>,
which widens its keys and makes domainOrder: Array<keyof typeof domains> become
string (losing typo-safety); fix by introducing a literal DomainKey union that
enumerates "stacks" | "clusters" | "admin-stacks" | "explorer" (or derive it
from a const assertion) and then type domains as Record<DomainKey, DomainSpec>
and domainOrder as DomainKey[]; update any usage of domains or domainOrder
accordingly (referencing the domains constant and the domainOrder variable).
In `@examples/enitity/webapp/tsconfig.tsbuildinfo`:
- Line 1: Remove the generated TypeScript build cache file tsconfig.tsbuildinfo
from the commit (it is compiler output, not source), add the pattern
*.tsbuildinfo to the example webapp’s .gitignore so future buildinfo files are
ignored, and verify/rename the examples/enitity directory if it is a typo
(likely should be examples/entity); look for references to the listed root files
(./src/app.tsx, ./src/api.ts, ./src/domains.ts, ./src/main.tsx) to ensure
renaming does not break paths.
In `@prompt.go`:
- Around line 221-225: newPromptSession errors are being swallowed and treated
as user cancellations; update each caller (PromptMultiSelect,
Prompt/PromptSelect/PromptChoose, PromptText) to detect a non-nil err returned
from newPromptSession and log the error at debug level via the package logger
(clicky or existing logger) before returning the existing nil,false result so
automated callers can at least observe runtime failures; apply the same change
to the other two newPromptSession call sites so non-abort errors are logged
consistently.
- Around line 573-605: clampPromptPageSize currently forces an explicit positive
pageSize up to promptMinSelectPageSize; change it so the floor (minPageSize) is
only applied when the caller did not supply a value (pageSize <= 0) or when the
computed maxPageSize is smaller than the configured min (i.e., maxPageSize <
minPageSize) which forces the floor down; specifically, keep computing
minPageSize and maxPageSize as-is, set pageSize = maxPageSize when pageSize <=
0, clamp pageSize down to maxPageSize and itemCount when pageSize > maxPageSize
or > itemCount, and only apply pageSize = minPageSize when (pageSize <= 0) OR
(maxPageSize > 0 && maxPageSize < minPageSize); update the logic in
clampPromptPageSize to reflect this and reference variables pageSize,
minPageSize, maxPageSize, promptMinSelectPageSize, itemCount, terminalLines,
chromeHeight.
- Around line 127-152: The probe in cursorRow consumes any user keystrokes while
in raw mode and can busy-loop on POLLHUP/POLLERR; update
readPromptCursorResponse to only perform reads when poll reports POLLIN and to
immediately return (ok=false) when poll revents contain POLLHUP or POLLERR, and
change its read loop to accumulate bytes but treat any leading bytes that are
not the expected ESC '[' sequence as unconsumed input (either push them back via
TIOCSTI or store them in a small unconsumed buffer on promptTerminal) so those
keystrokes aren’t lost; keep references to promptCursorQuerySequence,
promptCursorQueryTimeout, readPromptCursorResponse, parsePromptCursorResponse
and cursorRow so the changes are made in the cursor probe code path.
In `@task/terminal_owner.go`:
- Around line 109-131: Update the AcquirePromptTerminal doc comment to
explicitly state that the returned release closure only calls
globalANSITerminal.releasePrompt() and does not restart any renderer stopped by
manager.stopRender(); note that the second return bool only indicates a renderer
was displaced and callers are responsible for re-starting or re-engaging task
rendering (e.g., by checking the bool and invoking the appropriate lifecycle
method) after calling the release function.
- Around line 68-81: acquirePrompt currently decrements promptWaiters only when
>0 and assumes it was paired with beginPromptAcquire, which can silently hide
mismatches; modify acquirePrompt (or rename to finishPromptAcquire) to enforce
the pairing by asserting that c.promptWaiters > 0 before decrementing (e.g., if
c.promptWaiters == 0 { panic("acquirePrompt called without matching
beginPromptAcquire") }), or alternatively rename the method to
finishPromptAcquire to document the required pairing, and update any callers
(including tryAcquireTaskRenderer) accordingly so the invariant is explicit and
fails fast on misuse.
🪄 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: 26ac0774-03e2-4703-975a-778ee22a7d8c
⛔ Files ignored due to path filters (3)
examples/go.sumis excluded by!**/*.sumexamples/uber_demo/go.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sum
📒 Files selected for processing (52)
clicky_lookup.gocobra_command.goentity.goentity_filters_test.goexamples/README.mdexamples/enitity/e2e/.gitignoreexamples/enitity/e2e/playwright.config.tsexamples/enitity/e2e/tests/entity-demo.spec.tsexamples/enitity/main.goexamples/enitity/main_test.goexamples/enitity/webapp/.gitignoreexamples/enitity/webapp/src/App.tsxexamples/enitity/webapp/src/api.tsexamples/enitity/webapp/src/domains.tsexamples/enitity/webapp/src/index.cssexamples/enitity/webapp/src/main.tsxexamples/enitity/webapp/tsconfig.tsbuildinfoexamples/enitity/webapp/vite.config.tsexamples/go.modexamples/uber_demo/go.modexamples/uber_demo/main.goformatters/html_react_formatter.goformatters/http/http.goformatters/http/http_test.goglobal_task_api.gogo.modinternal/gumchoose/LICENSEinternal/gumchoose/choose.gointernal/gumchoose/choose_test.goprompt.goprompt_terminal_ioctl_darwin_test.goprompt_terminal_ioctl_linux_test.goprompt_test.gorpc/converter.gorpc/executor.gorpc/filter_lookup_test.gorpc/serve.gorpc/serve_test.gorpc/types.gosub_command_test.gotask/group.gotask/manager.gotask/manager_lifecycle.gotask/manager_wait.gotask/no_render_test.gotask/options.gotask/snapshot.gotask/stop_test.gotask/task.gotask/terminal_owner.gotask/worker.gotestdata/prompt_test_helper.go
Comment on lines
332
to
338
| adminInfo := EntityInfo{ | ||
| Name: admin.Name, | ||
| Type: info.Type, | ||
| ListType: info.ListType, | ||
| IsAdmin: true, | ||
| Name: admin.Name, | ||
| Type: info.Type, | ||
| ListType: info.ListType, | ||
| ValidArgs: adminValidArgs, | ||
| IsAdmin: true, | ||
| } |
Contributor
There was a problem hiding this comment.
Preserve parent nesting for admin entities.
GenerateCLI routes admin commands using entity.Parent, but adminInfo drops the parent, so an entity registered under a parent gets its admin commands attached at the root admin group.
Preserve inherited admin parent
+ adminParent := admin.Parent
+ if adminParent == "" {
+ adminParent = e.Parent
+ }
// Store as admin sub-entity — GenerateCLI nests under "admin" parent
adminInfo := EntityInfo{
Name: admin.Name,
+ Parent: adminParent,
Type: info.Type,
ListType: info.ListType,
ValidArgs: adminValidArgs,
IsAdmin: true,📝 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
| adminInfo := EntityInfo{ | |
| Name: admin.Name, | |
| Type: info.Type, | |
| ListType: info.ListType, | |
| IsAdmin: true, | |
| Name: admin.Name, | |
| Type: info.Type, | |
| ListType: info.ListType, | |
| ValidArgs: adminValidArgs, | |
| IsAdmin: true, | |
| } | |
| adminParent := admin.Parent | |
| if adminParent == "" { | |
| adminParent = e.Parent | |
| } | |
| // Store as admin sub-entity — GenerateCLI nests under "admin" parent | |
| adminInfo := EntityInfo{ | |
| Name: admin.Name, | |
| Parent: adminParent, | |
| Type: info.Type, | |
| ListType: info.ListType, | |
| ValidArgs: adminValidArgs, | |
| IsAdmin: true, | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@entity.go` around lines 332 - 338, The admin EntityInfo constructed as
adminInfo is missing the Parent field, so admin commands lose their nesting;
update the adminInfo creation (EntityInfo adminInfo) to copy the parent from the
original info (e.g., set Parent: info.Parent) so GenerateCLI sees the same
entity.Parent and registers admin commands under the correct parent group.
Comment on lines
+871
to
+895
| func describeLookupField(field flags.FieldInfo) entityLookupMetadata { | ||
| fieldType := field.FieldType | ||
| for fieldType.Kind() == reflect.Ptr { | ||
| fieldType = fieldType.Elem() | ||
| } | ||
|
|
||
| meta := entityLookupMetadata{ | ||
| Multi: fieldType.Kind() == reflect.Slice, | ||
| } | ||
|
|
||
| return v.Interface().(T) | ||
| switch { | ||
| case fieldType.Kind() == reflect.Bool: | ||
| meta.Type = "bool" | ||
| case isNumericKind(fieldType.Kind()): | ||
| meta.Type = "number" | ||
| case fieldType.String() == "time.Time": | ||
| switch { | ||
| case isRangeStartFlag(field.FlagName): | ||
| meta.Type = "from" | ||
| case isRangeEndFlag(field.FlagName): | ||
| meta.Type = "to" | ||
| default: | ||
| meta.Type = "date" | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Infer lookup type from slice elements.
For multi filters like []int or []time.Time, fieldType.Kind() is Slice, so the current switch never sets number, date, from, or to.
Unwrap slice element before type classification
func describeLookupField(field flags.FieldInfo) entityLookupMetadata {
fieldType := field.FieldType
for fieldType.Kind() == reflect.Ptr {
fieldType = fieldType.Elem()
}
meta := entityLookupMetadata{
Multi: fieldType.Kind() == reflect.Slice,
}
+ if fieldType.Kind() == reflect.Slice {
+ fieldType = fieldType.Elem()
+ for fieldType.Kind() == reflect.Ptr {
+ fieldType = fieldType.Elem()
+ }
+ }
switch {📝 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
| func describeLookupField(field flags.FieldInfo) entityLookupMetadata { | |
| fieldType := field.FieldType | |
| for fieldType.Kind() == reflect.Ptr { | |
| fieldType = fieldType.Elem() | |
| } | |
| meta := entityLookupMetadata{ | |
| Multi: fieldType.Kind() == reflect.Slice, | |
| } | |
| return v.Interface().(T) | |
| switch { | |
| case fieldType.Kind() == reflect.Bool: | |
| meta.Type = "bool" | |
| case isNumericKind(fieldType.Kind()): | |
| meta.Type = "number" | |
| case fieldType.String() == "time.Time": | |
| switch { | |
| case isRangeStartFlag(field.FlagName): | |
| meta.Type = "from" | |
| case isRangeEndFlag(field.FlagName): | |
| meta.Type = "to" | |
| default: | |
| meta.Type = "date" | |
| } | |
| } | |
| func describeLookupField(field flags.FieldInfo) entityLookupMetadata { | |
| fieldType := field.FieldType | |
| for fieldType.Kind() == reflect.Ptr { | |
| fieldType = fieldType.Elem() | |
| } | |
| meta := entityLookupMetadata{ | |
| Multi: fieldType.Kind() == reflect.Slice, | |
| } | |
| if fieldType.Kind() == reflect.Slice { | |
| fieldType = fieldType.Elem() | |
| for fieldType.Kind() == reflect.Ptr { | |
| fieldType = fieldType.Elem() | |
| } | |
| } | |
| switch { | |
| case fieldType.Kind() == reflect.Bool: | |
| meta.Type = "bool" | |
| case isNumericKind(fieldType.Kind()): | |
| meta.Type = "number" | |
| case fieldType.String() == "time.Time": | |
| switch { | |
| case isRangeStartFlag(field.FlagName): | |
| meta.Type = "from" | |
| case isRangeEndFlag(field.FlagName): | |
| meta.Type = "to" | |
| default: | |
| meta.Type = "date" | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@entity.go` around lines 871 - 895, describeLookupField currently detects
Multi when the field is a slice but classifies type based on the slice kind
itself, so multi filters like []int or []time.Time never get their element types
mapped; update describeLookupField to, after detecting Slice and setting
meta.Multi, unwrap the element type (e.g., fieldType = fieldType.Elem()) before
the switch/type checks so isNumericKind, the time.Time check and
isRangeStartFlag/isRangeEndFlag logic operate on the slice element type; ensure
pointer-unwrapping still happens first and keep entityLookupMetadata.Multi
assignment using the original slice detection.
Comment on lines
+1
to
+4
| node_modules/ | ||
| playwright-report/ | ||
| test-results/ | ||
| .cache/ |
Contributor
There was a problem hiding this comment.
Fix typo in directory path.
The directory path contains a typo: examples/enitity/ should be examples/entity/. The PR objectives and context confirm this should be "entity", not "enitity".
The .gitignore entries themselves are standard and appropriate for a Playwright e2e test setup.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/enitity/e2e/.gitignore` around lines 1 - 4, Filename and paths
contain a typo: the directory name "enitity" should be "entity"; update the
repository structure and any references so the e2e .gitignore and its parent
directory use "entity" instead of "enitity" (rename the directory or move the
file, and update any CI/README/PR references that point to "enitity") to ensure
consistency with the project naming.
Comment on lines
+36
to
+43
| webServer: { | ||
| command: `cd "${exampleDir}" && GOCACHE="${GOCACHE}" task build && GOCACHE="${GOCACHE}" ./entity-demo serve-ui --host ${HOST} --port ${PORT}`, | ||
| url: BASE_URL, | ||
| reuseExistingServer: false, | ||
| timeout: 240_000, | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| }, |
Contributor
There was a problem hiding this comment.
Shell-escape environment-derived command values.
HOST, PORT, GOCACHE, and paths are interpolated into a shell command. A value containing spaces, quotes, or shell metacharacters can break the E2E startup command.
🛡️ Proposed fix
const GOCACHE = process.env.GOCACHE ?? "/tmp/go-build";
+const shellQuote = (value: string) => `'${value.replace(/'/g, "'\\''")}'`;
+const webServerCommand = [
+ `cd ${shellQuote(exampleDir)}`,
+ `GOCACHE=${shellQuote(GOCACHE)} task build`,
+ `GOCACHE=${shellQuote(GOCACHE)} ./entity-demo serve-ui --host ${shellQuote(HOST)} --port ${shellQuote(PORT)}`,
+].join(" && ");
export default defineConfig({
@@
webServer: {
- command: `cd "${exampleDir}" && GOCACHE="${GOCACHE}" task build && GOCACHE="${GOCACHE}" ./entity-demo serve-ui --host ${HOST} --port ${PORT}`,
+ command: webServerCommand,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/enitity/e2e/playwright.config.ts` around lines 36 - 43, The
webServer.command string in the Playwright config interpolates
environment-derived values (HOST, PORT, GOCACHE, exampleDir, BASE_URL) directly
into a shell command which can break if those values contain spaces or shell
metacharacters; change webServer.command so those values are properly escaped or
passed as separate argv (e.g., use a shell-escape utility or construct the
command with safe-quoted/escaped values or switch to a spawn/execFile-style
invocation with arguments) ensuring the variables HOST, PORT, GOCACHE,
exampleDir and any paths are sanitized/quoted before interpolation in the
webServer.command property.
Comment on lines
+8
to
+16
| func TestMatchingStacksLockedHonorsFromAndTo(t *testing.T) { | ||
| store := newDemoStore() | ||
|
|
||
| items := store.matchingStacksLocked(stackListOpts{ | ||
| stackWindowOpts: stackWindowOpts{ | ||
| From: time.Now().Add(-24 * time.Hour).UTC(), | ||
| To: time.Now().UTC(), | ||
| }, | ||
| }, false) |
Contributor
There was a problem hiding this comment.
Avoid a wall-clock boundary flake.
If newDemoStore() seeds timestamps from time.Now(), the exact now-24h lower bound can exclude boundary records by a few microseconds. Capture one clock value and add a small tolerance, or use a fixed clock in the fixture.
🧪 Proposed test hardening
func TestMatchingStacksLockedHonorsFromAndTo(t *testing.T) {
+ now := time.Now().UTC()
store := newDemoStore()
items := store.matchingStacksLocked(stackListOpts{
stackWindowOpts: stackWindowOpts{
- From: time.Now().Add(-24 * time.Hour).UTC(),
- To: time.Now().UTC(),
+ From: now.Add(-24*time.Hour - time.Minute),
+ To: now.Add(time.Minute),
},
}, false)📝 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
| func TestMatchingStacksLockedHonorsFromAndTo(t *testing.T) { | |
| store := newDemoStore() | |
| items := store.matchingStacksLocked(stackListOpts{ | |
| stackWindowOpts: stackWindowOpts{ | |
| From: time.Now().Add(-24 * time.Hour).UTC(), | |
| To: time.Now().UTC(), | |
| }, | |
| }, false) | |
| func TestMatchingStacksLockedHonorsFromAndTo(t *testing.T) { | |
| now := time.Now().UTC() | |
| store := newDemoStore() | |
| items := store.matchingStacksLocked(stackListOpts{ | |
| stackWindowOpts: stackWindowOpts{ | |
| From: now.Add(-24*time.Hour - time.Minute), | |
| To: now.Add(time.Minute), | |
| }, | |
| }, false) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/enitity/main_test.go` around lines 8 - 16, Test is flaky because it
computes From/To with separate time.Now() calls; change
TestMatchingStacksLockedHonorsFromAndTo to capture a single now before building
stackWindowOpts (e.g. now := time.Now().UTC()) and use now.Add(-24*time.Hour)
and now for From/To, and add a small tolerance (e.g. +-1ms) or modify
newDemoStore() to accept an injected fixed clock so matchingStacksLocked sees
deterministic timestamps; update references in the test to use the captured now
and/or the fixed clock so stackWindowOpts.From/To cannot accidentally exclude
boundary records.
Comment on lines
+622
to
+630
| w.Header().Set("Content-Type", "application/json+clicky") | ||
| w.WriteHeader(http.StatusOK) | ||
|
|
||
| if strings.EqualFold(r.Method, http.MethodHead) { | ||
| return | ||
| } | ||
|
|
||
| if err := json.NewEncoder(w).Encode(data); err != nil { | ||
| http.Error(w, fmt.Sprintf("failed to encode lookup response: %v", err), http.StatusInternalServerError) |
Contributor
There was a problem hiding this comment.
Encode lookup responses before committing 200 OK.
WriteHeader(http.StatusOK) is sent before JSON encoding. If data cannot be encoded, the client still receives a 200 response. Buffer the JSON first, then write headers.
🐛 Proposed fix
+ // add near the other imports
+ // "bytes"
+
w.Header().Set("Content-Type", "application/json+clicky")
- w.WriteHeader(http.StatusOK)
if strings.EqualFold(r.Method, http.MethodHead) {
+ w.WriteHeader(http.StatusOK)
return
}
- if err := json.NewEncoder(w).Encode(data); err != nil {
+ var body bytes.Buffer
+ if err := json.NewEncoder(&body).Encode(data); err != nil {
http.Error(w, fmt.Sprintf("failed to encode lookup response: %v", err), http.StatusInternalServerError)
+ return
}
+ w.WriteHeader(http.StatusOK)
+ _, _ = w.Write(body.Bytes())📝 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
| w.Header().Set("Content-Type", "application/json+clicky") | |
| w.WriteHeader(http.StatusOK) | |
| if strings.EqualFold(r.Method, http.MethodHead) { | |
| return | |
| } | |
| if err := json.NewEncoder(w).Encode(data); err != nil { | |
| http.Error(w, fmt.Sprintf("failed to encode lookup response: %v", err), http.StatusInternalServerError) | |
| w.Header().Set("Content-Type", "application/json+clicky") | |
| if strings.EqualFold(r.Method, http.MethodHead) { | |
| w.WriteHeader(http.StatusOK) | |
| return | |
| } | |
| var body bytes.Buffer | |
| if err := json.NewEncoder(&body).Encode(data); err != nil { | |
| http.Error(w, fmt.Sprintf("failed to encode lookup response: %v", err), http.StatusInternalServerError) | |
| return | |
| } | |
| w.WriteHeader(http.StatusOK) | |
| _, _ = w.Write(body.Bytes()) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rpc/serve.go` around lines 622 - 630, The handler currently calls
w.WriteHeader(http.StatusOK) before json encoding, so a 200 is sent even if
json.NewEncoder(w).Encode(data) fails; instead, encode the response into a
temporary buffer (e.g., bytes.Buffer) first using
json.NewEncoder(&buf).Encode(data), and only if that succeeds set the
Content-Type header and call w.WriteHeader(http.StatusOK) and then write the
buffer to w; for HEAD requests, still return after writing headers but do not
write the body. Update references to json.NewEncoder(w).Encode(data),
w.WriteHeader(http.StatusOK), r.Method/ http.MethodHead, and the variable data
accordingly.
Comment on lines
371
to
+378
| func (tm *Manager) enqueue(task *Task) *Task { | ||
| tm.renderStarted.Do(func() { | ||
| if !tm.noProgress.Load() { | ||
| tm.startRenderLoop() | ||
| } | ||
| }) | ||
| if !tm.noRender.Load() { | ||
| tm.renderStarted.Do(func() { | ||
| if !tm.noProgress.Load() { | ||
| tm.startRenderLoop() | ||
| } | ||
| }) | ||
| } |
Contributor
There was a problem hiding this comment.
Don’t consume renderStarted when render startup is skipped.
startRenderLoop() can now return early when the interactive renderer cannot be acquired, but this call is inside renderStarted.Do. That permanently marks rendering as started even though no render loop exists, so later tasks will not retry after the TTY becomes available.
Consider moving terminal acquisition before the sync.Once, or replacing sync.Once with a mutex-protected “started successfully” state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@task/manager.go` around lines 371 - 378, The problem: renderStarted (the
sync.Once used in Manager.enqueue) is being consumed even when startRenderLoop()
returns early (fails to acquire the interactive renderer), preventing future
retries; fix it by ensuring we only mark rendering as started when the render
loop actually began. Change enqueue to acquire the terminal/renderer (or call
the portion of startRenderLoop that can fail to obtain the TTY) before invoking
renderStarted.Do, or replace renderStarted (sync.Once) with a mutex-protected
startedSuccessfully boolean and only set startedSuccessfully=true after
startRenderLoop completes successfully; update references to renderStarted.Do,
startRenderLoop, noRender, and noProgress accordingly so the Once is not
consumed on early return.
| func SnapshotTask(t *Task, groupName string) TaskSnapshot { | ||
| snap := TaskSnapshot{ | ||
| ID: t.Name(), | ||
| ID: t.ID(), |
Contributor
There was a problem hiding this comment.
Make SnapshotAll filter by the immutable task IDs it now emits.
Line 32 changes task snapshot IDs to t.ID(), but SnapshotAll(taskIDs...) still filters only on g.Name(). A caller using a returned task id to reload the task detail will get no snapshots unless that ID happens to match a group name.
🛠️ Proposed fix
-// If taskIDs is non-empty, only groups whose name matches are included.
+// If taskIDs is non-empty, only matching groups or matching child tasks are included.
func SnapshotAll(taskIDs ...string) []TaskSnapshot {
if global == nil {
return nil
}
@@
- for _, g := range groups {
- if len(filter) > 0 && !filter[g.Name()] {
- continue
- }
- snapshots = append(snapshots, SnapshotGroup(g))
-
- g.mu.RLock()
- items := g.Items
- g.mu.RUnlock()
-
- for _, item := range items {
- snapshots = append(snapshots, SnapshotTask(item.GetTask(), g.Name()))
- }
- }
+ for _, g := range groups {
+ groupMatched := len(filter) == 0 || filter[g.Name()]
+
+ g.mu.RLock()
+ items := append([]Taskable(nil), g.Items...)
+ g.mu.RUnlock()
+
+ matchedItems := items
+ if len(filter) > 0 && !groupMatched {
+ matchedItems = nil
+ for _, item := range items {
+ if filter[item.GetTask().ID()] {
+ matchedItems = append(matchedItems, item)
+ }
+ }
+ if len(matchedItems) == 0 {
+ continue
+ }
+ }
+
+ snapshots = append(snapshots, SnapshotGroup(g))
+ for _, item := range matchedItems {
+ snapshots = append(snapshots, SnapshotTask(item.GetTask(), g.Name()))
+ }
+ }Also applies to: 87-118
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@task/snapshot.go` at line 32, Snapshot filtering in SnapshotAll is still
using group names (g.Name()) while snapshots are now emitted with task IDs via
t.ID(), so callers get no matches; update the SnapshotAll implementation to
build a set from the input taskIDs and change the filter to compare the
snapshot's emitted ID (the field set from t.ID(), e.g., snapshot.ID or
snapshot.TaskID depending on struct) against that set instead of g.Name(), and
make the same replacement for the other filtering code paths mentioned (the
block around the other occurrences) so all filters use the emitted task ID.
Comment on lines
+35
to
+45
| task.mu.Lock() | ||
| skip := task.status == StatusCancelled | ||
| task.mu.Unlock() | ||
| if skip { | ||
| task.completed.Store(true) | ||
| task.signalDone() | ||
| if task.identity != "" { | ||
| w.manager.tasksByIdentity.Delete(task.identity) | ||
| } | ||
| continue | ||
| } |
Contributor
There was a problem hiding this comment.
Don’t treat cancelled dependencies as satisfied.
The new skip path marks cancelled tasks as completed. checkDependencies only inspects dependency failure/cancellation when completed == false, so dependents can run after a dependency was cancelled.
🐛 Proposed fix
func (w *worker) checkDependencies(task *Task) bool {
for _, dep := range task.dependencies {
if dep == nil {
continue
}
- if !dep.completed.Load() {
- // Check if dependency failed
- dep.mu.Lock()
- depStatus := dep.status
- dep.mu.Unlock()
-
- if depStatus == StatusFailed || depStatus == StatusCancelled {
- // Dependency failed, mark this task as canceled
- task.mu.Lock()
- task.status = StatusCancelled
- task.endTime = time.Now()
- task.err = fmt.Errorf("dependency failed")
- task.completed.Store(true)
- task.mu.Unlock()
- return false
- }
- // Dependency not yet complete
+
+ dep.mu.Lock()
+ depStatus := dep.status
+ dep.mu.Unlock()
+
+ if depStatus == StatusFailed || depStatus == StatusCancelled {
+ // Dependency failed, mark this task as canceled
+ task.mu.Lock()
+ task.status = StatusCancelled
+ task.endTime = time.Now()
+ task.err = fmt.Errorf("dependency failed")
+ task.completed.Store(true)
+ task.mu.Unlock()
+ return false
+ }
+
+ if !dep.completed.Load() {
return false
}
}
return true
}Also applies to: 83-104
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@task/worker.go` around lines 35 - 45, The skip path for cancelled tasks marks
them completed which hides cancellation from dependents; remove the
task.completed.Store(true) (and any logic that treats a cancelled task as fully
completed) in the cancellation branch where task.status == StatusCancelled so
that checkDependencies can still observe dependency.status; alternatively,
update checkDependencies to examine dependency.status
(StatusCancelled/StatusFailed) regardless of dependency.completed. Adjust the
cancellation branch around task.mu, task.signalDone and
w.manager.tasksByIdentity.Delete(task.identity) as needed so cancelled tasks are
removed/cleaned up without flipping completed to true, and ensure
checkDependencies is updated to check status directly rather than relying on
completed.
Comment on lines
+81
to
+102
| func runTaskTakeover() { | ||
| release := make(chan struct{}) | ||
| clicky.StartTask[string]("rendering", func(ctx flanksourceContext.Context, t *clicky.Task) (string, error) { | ||
| t.SetDescription("before prompt TAKEOVER-MARKER") | ||
| select { | ||
| case <-release: | ||
| t.SetDescription("after prompt TAKEOVER-MARKER") | ||
| return "done", nil | ||
| case <-ctx.Done(): | ||
| return "", ctx.Err() | ||
| } | ||
| }) | ||
|
|
||
| time.Sleep(600 * time.Millisecond) | ||
|
|
||
| value, ok := clicky.PromptSelect([]string{"continue-marker", "stop-marker"}, clicky.PromptSelectOptions[string]{ | ||
| Title: "Take over terminal", | ||
| }) | ||
|
|
||
| close(release) | ||
| _ = clicky.WaitForGlobalCompletion() | ||
| fmt.Fprintf(os.Stderr, "\nRESULT=%s OK=%v\n", value, ok) |
Contributor
There was a problem hiding this comment.
Fixed sleep before prompt may be flaky under load.
time.Sleep(600 * time.Millisecond) is a best-effort gap to let the task renderer print its pre-takeover frame before the prompt steals the terminal. On a busy CI runner or a cold binary start this can be too short and flip the test. Consider synchronizing on an observable signal from the renderer (e.g. read stderr for TAKEOVER-MARKER in the test harness, or expose a readiness channel via t.SetDescription + a poll) rather than a wall-clock delay.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@testdata/prompt_test_helper.go` around lines 81 - 102, The test uses a fixed
sleep in runTaskTakeover (time.Sleep(600 * time.Millisecond)) which is flaky;
replace the wall-clock delay with a synchronization strategy that waits until
the renderer signals readiness before calling clicky.PromptSelect — e.g., have
the task started via clicky.StartTask set a known description marker with
t.SetDescription("before prompt TAKEOVER-MARKER") and then in the test poll/read
stderr (or poll the task description via any available API) for that
TAKEOVER-MARKER or expose a readiness channel from the task and wait on it, then
call clicky.PromptSelect; remove the fixed sleep to ensure deterministic
behavior under load.
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
1 participant
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 by CodeRabbit
Release Notes
New Features
Documentation
Chores
application/json+clicky