Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a Task UI (embedded Preact frontend with JSON & SSE endpoints), a new html-react formatter, a Go static-analysis linter plugin, admin-entity support, task snapshot models/handlers, various HTML rendering tweaks, and CI/build updates (Go 1.26 + Node frontend builds). Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant Server
participant TaskManager
Browser->>Server: GET / (load page with embedded bundle)
Browser->>Server: GET /api/tasks/stream (open SSE)
Server->>TaskManager: SnapshotAll(ids...) (poll every ~200ms)
TaskManager-->>Server: []TaskSnapshot
Server->>Browser: SSE event: task (data: snapshots JSON)
Browser->>Browser: update UI state and render groups/tasks
alt all tracked groups complete
Server->>Browser: SSE event: done {"status":"completed"}
Browser->>Server: close SSE
end
Possibly related PRs
Suggested labelsreleased 🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ 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 |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![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: 3
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Makefile (1)
18-24:⚠️ Potential issue | 🟠 Major
buildtarget should depend ontask-ui.The
buildtarget runsgo build, buttask/ui/assets.gouses//go:embed dist/taskui.js, which requires the JavaScript bundle to exist at compile time. Ifdist/taskui.jsdoesn't exist, the Go build will fail.Proposed fix
# Build the binary -build: +build: task-ui go build -o clicky ./cmd/clicky/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 18 - 24, The Makefile's build target must run the frontend build first because task/ui/assets.go uses //go:embed dist/taskui.js at compile time; update the Makefile so the build target depends on the task-ui target (e.g., make build depend on task-ui) so that task-ui runs before running go build -o clicky ./cmd/clicky/, ensuring dist/taskui.js exists when assets.go is compiled.
🟠 Major comments (19)
entity.go-274-281 (1)
274-281:⚠️ Potential issue | 🟠 MajorMissing ID validation in admin action handler.
The admin action handler extracts the ID but doesn't validate it's non-empty before calling
a.Run(id, flagMap). Compare to the main entity's action handler (lines 205-207) which returns an error whenid == "".Proposed fix
DataFunc: func(flagMap map[string]string, args []string) (any, error) { id := flagMap["id"] if id == "" && len(args) > 0 { id = args[0] } + if id == "" { + return nil, fmt.Errorf("id is required") + } return a.Run(id, flagMap) },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@entity.go` around lines 274 - 281, The admin action handler's DataFunc extracts id but doesn't validate it before calling a.Run; modify the DataFunc (the admin action handler using DataFunc and a.Run) to check if id == "" (after checking args) and return a descriptive error (same behavior as the main entity action handler) instead of calling a.Run with an empty id so the handler fails early with a clear message.entity.go-563-579 (1)
563-579:⚠️ Potential issue | 🟠 MajorEmpty inner objects produce invalid JSON.
If
e.Innermarshals to an empty object{}, this method produces{"_id":"...",}which is invalid JSON due to the trailing comma.Proposed fix
func (e entityWithID[T]) MarshalJSON() ([]byte, error) { data, err := json.Marshal(e.Inner) if err != nil { return nil, err } - if len(data) < 2 || data[0] != '{' { + // Not a JSON object, return as-is (loses _id) + if len(data) == 0 || data[0] != '{' { + return data, nil + } + // Empty object "{}" - just inject _id + if len(data) == 2 { + idJSON, err := json.Marshal(e.ID) + if err != nil { + return nil, err + } + return append(append([]byte(`{"_id":`), idJSON...), '}'), nil + } + idJSON, err := json.Marshal(e.ID) + if err != nil { + return nil, err + } + prefix := []byte(`{"_id":`) + prefix = append(prefix, idJSON...) + prefix = append(prefix, ',') + return append(prefix, data[1:]...), nil +}Alternatively, consider using a more robust approach with map-based merging:
func (e entityWithID[T]) MarshalJSON() ([]byte, error) { data, err := json.Marshal(e.Inner) if err != nil { return nil, err } - if len(data) < 2 || data[0] != '{' { - return data, nil - } - idJSON, err := json.Marshal(e.ID) - if err != nil { - return nil, err + // If not an object, return as-is + if len(data) == 0 || data[0] != '{' { + return data, nil } - prefix := []byte(`{"_id":`) - prefix = append(prefix, idJSON...) - prefix = append(prefix, ',') - return append(prefix, data[1:]...), nil + var m map[string]json.RawMessage + if err := json.Unmarshal(data, &m); err != nil { + return data, nil // fallback to original + } + idJSON, _ := json.Marshal(e.ID) + m["_id"] = idJSON + return json.Marshal(m) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@entity.go` around lines 563 - 579, entityWithID.MarshalJSON currently builds JSON by concatenating bytes and produces invalid output for an empty inner object (e.g. {"_id":"...",}), so change the implementation to unmarshal e.Inner into a map[string]json.RawMessage (or map[string]any), set m["_id"] = json.RawMessage of e.ID (or assign the marshaled ID value), then return json.Marshal(m); this replaces the fragile byte-string prefix/append logic and correctly handles empty objects and all field merging.lint/plugin.go-20-22 (1)
20-22:⚠️ Potential issue | 🟠 Major
LoadModeSyntaxis insufficient for this analyzer.As noted in the analyzer review,
checkFuncReturnTypeandisClickyTextTypeusepass.TypesInfo.TypeOf(). This requires type information which won't be available withLoadModeSyntax. Change toLoadModeTypesInfo:🐛 Proposed fix
func (p *plugin) GetLoadMode() string { - return register.LoadModeSyntax + return register.LoadModeTypesInfo }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lint/plugin.go` around lines 20 - 22, The analyzer currently returns register.LoadModeSyntax from plugin.GetLoadMode but checkFuncReturnType and isClickyTextType rely on pass.TypesInfo.TypeOf(), so change the return value in GetLoadMode (function plugin.GetLoadMode) to register.LoadModeTypesInfo to ensure type information is populated for those helpers; locate the GetLoadMode method and replace LoadModeSyntax with LoadModeTypesInfo so pass.TypesInfo is available for checkFuncReturnType and isClickyTextType.lint/analyzer.go-17-22 (1)
17-22:⚠️ Potential issue | 🟠 MajorLoadMode mismatch: analyzer uses type information but plugin declares
LoadModeSyntax.The analyzer calls
pass.TypesInfo.TypeOf()(lines 53, 163) which requires type-checking information. However,lint/plugin.goline 21 returnsregister.LoadModeSyntax, which provides only AST data without type information. This should beregister.LoadModeTypesInfoto ensure type information is available when run via golangci-lint.The singlechecker in
cmd/clickylint/main.goworks because it defaults to full type checking, but the golangci-lint plugin will fail to resolve types correctly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lint/analyzer.go` around lines 17 - 22, The Analyzer uses type info via pass.TypesInfo.TypeOf() in run, but the plugin currently registers with register.LoadModeSyntax (in lint/plugin.go), which supplies only AST; change the plugin registration to use register.LoadModeTypesInfo so the analyzer (Analyzer, run) receives TypesInfo at runtime and type resolution calls like pass.TypesInfo.TypeOf() succeed when run under golangci-lint.mcp/install.go-38-40 (1)
38-40:⚠️ Potential issue | 🟠 MajorLookPath override may register wrong binary.
If the user invokes the CLI via an absolute path (e.g.,
/opt/myapp/bin/cli mcp install) but a different binary with the same name exists in$PATH,exec.LookPath(rootCmd.Use)will find the PATH binary and overridebinPath, registering the wrong executable.Consider only using LookPath as a fallback when
os.Executable()fails or returns an unexpected result:🐛 Suggested fix
binPath, err := os.Executable() if err != nil { - return fmt.Errorf("failed to find executable path: %w", err) + // Fallback to LookPath if Executable fails + binPath, err = exec.LookPath(rootCmd.Use) + if err != nil { + return fmt.Errorf("failed to find executable path: %w", err) + } + } else { + binPath, _ = filepath.EvalSymlinks(binPath) } - binPath, _ = filepath.EvalSymlinks(binPath) - - if p, err := exec.LookPath(rootCmd.Use); err == nil { - binPath = p - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcp/install.go` around lines 38 - 40, The current logic unconditionally uses exec.LookPath(rootCmd.Use) which can overwrite the actual invoked binary; change it to first obtain the running executable via os.Executable() (use function os.Executable and variable binPath) and validate it (ensure it's non-empty, absolute, and its basename matches rootCmd.Use); only if os.Executable() fails or returns an unexpected result should you call exec.LookPath(rootCmd.Use) as a fallback to set binPath. Update the code paths around binPath, exec.LookPath, os.Executable, and rootCmd.Use to prefer the discovered executable and fall back to LookPath only when necessary.rpc/executor.go-397-408 (1)
397-408:⚠️ Potential issue | 🟠 MajorOnly positional path params should feed
req.Args.
rpc/converter.goLines 314-323 can generate/{id}/actionsegments from an ID flag when there is no positional placeholder. This loop appends every path param toreq.Args, so a flag-backed route ends up executing with both--id=valueand a stray positional argument, which can trip Cobra arg validation or change command behavior.Proposed fix
pathParams := extractPathParams(op.Path, r.URL.Path) +positionalName := "" +if op.Command != nil { + positionalName = extractParameterName(op.Command.Use) +} +appendedPositional := false for _, param := range op.Parameters { - if param.In == "path" { - if value, ok := pathParams[param.Name]; ok { - // Split comma-delimited IDs into separate args - for _, id := range strings.Split(value, ",") { - req.Args = append(req.Args, strings.TrimSpace(id)) - } - req.Flags[param.Name] = value - } - } + if param.In != "path" { + continue + } + value, ok := pathParams[param.Name] + if !ok { + continue + } + req.Flags[param.Name] = value + if param.Name != positionalName || appendedPositional { + continue + } + for _, id := range strings.Split(value, ",") { + req.Args = append(req.Args, strings.TrimSpace(id)) + } + appendedPositional = true }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rpc/executor.go` around lines 397 - 408, The loop over op.Parameters currently appends every path param value into req.Args which adds positional args for params that are actually flag-backed (generated by rpc/converter.go); change the logic in the block that handles param.In == "path" to only append to req.Args when the path template actually contains a positional placeholder for that parameter (e.g., check that op.Path contains "{" + param.Name + "}"); always set req.Flags[param.Name] = value as before, but avoid adding comma-split values to req.Args for parameters that are only flag-backed.rpc/converter.go-303-312 (1)
303-312:⚠️ Potential issue | 🟠 MajorFix phantom path parameter from
prompt [name]command.The verification found a concrete case:
mcp/command.go:322definesUse: "prompt [name]"with noArgsfield. The new path-building logic will extract'name'and create a route template like/mcp/prompt/{name}, butConvertCommandwon't emit a matchingRPCParameter(sinceArgsis absent), leaving the executor unable to hydrate the placeholder.Either make
extractParameterNamereject Cobra metasyntax like[...]and<...>, or synthesize anRPCParameterwhen theUsestring supplies a path segment template.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rpc/converter.go` around lines 303 - 312, The code is creating a phantom path parameter when extractParameterName parses Cobra metasyntax from cmd.Use (e.g., "prompt [name]") but no Args/RPCParameter exists; update the logic in ConvertCommand/path-building to either (a) make extractParameterName ignore Cobra metasyntax forms (square or angle brackets) so it returns empty for "[name]" and "<id>", or (b) when extractParameterName returns a name but no corresponding Args/RPCParameter is present, synthesize an RPCParameter (with name, in="path", required=true and appropriate schema/type) so the generated route template and RPCParameter list stay consistent; modify the code around extractParameterName, the pathParts insertion block, and the RPCParameter construction to apply one of these fixes.task/ui/src/components/Summary.tsx-12-17 (1)
12-17:⚠️ Potential issue | 🟠 MajorCanceled/SKIP tasks are miscounted as pending.
Line 16 treats unclassified statuses as pending, so
canceledandSKIPinflate pending counts and distort progress.Suggested fix
const ok = all.filter(t => t.status === 'success' || t.status === 'PASS').length; const warn = all.filter(t => t.status === 'warning').length; const fail = all.filter(t => t.status === 'failed' || t.status === 'FAIL' || t.status === 'ERR').length; const run = all.filter(t => t.status === 'running').length; - const pending = total - ok - warn - fail - run; + const neutralDone = all.filter(t => t.status === 'canceled' || t.status === 'SKIP').length; + const pending = total - ok - warn - fail - run - neutralDone;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@task/ui/src/components/Summary.tsx` around lines 12 - 17, The pending count currently computes as pending = total - ok - warn - fail - run which incorrectly treats "canceled" and "SKIP" as pending; update Summary.tsx to explicitly compute counts for canceled and skipped (e.g., const canceled = all.filter(t => t.status === 'canceled').length and const skipped = all.filter(t => t.status === 'SKIP' || t.status === 'skip').length) and then compute pending = total - ok - warn - fail - run - canceled - skipped (or compute pending directly by filtering for explicit pending statuses); adjust any UI labels that reference pending if needed and keep existing elapsed/startTime logic unchanged.task/api.go-14-17 (1)
14-17:⚠️ Potential issue | 🟠 MajorServer-side task scoping is bypassable via query merge.
Line 14–17 appends user-supplied
tasksIDs even whentaskIDswere provided by the server, which contradicts the handler contract and can expose unintended task groups.Suggested fix
func JSONHandler(taskIDs ...string) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { ids := append([]string{}, taskIDs...) - if q := r.URL.Query().Get("tasks"); q != "" { - ids = append(ids, strings.Split(q, ",")...) + if len(ids) == 0 { + if q := r.URL.Query().Get("tasks"); q != "" { + for _, id := range strings.Split(q, ",") { + id = strings.TrimSpace(id) + if id != "" { + ids = append(ids, id) + } + } + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@task/api.go` around lines 14 - 17, The handler currently merges server-scoped taskIDs with user-supplied query IDs (ids := append([]string{}, taskIDs...) then appending strings.Split(q, ",") from r.URL.Query().Get("tasks")), which allows clients to bypass server scoping; change the logic so that the query parameter is only considered when taskIDs is empty (i.e., if len(taskIDs) == 0) — otherwise ignore the "tasks" query and use only the server-provided taskIDs; update the code paths around ids, taskIDs, and r.URL.Query().Get("tasks") accordingly and keep any splitting/validation only for the query branch.task/sse.go-18-21 (1)
18-21:⚠️ Potential issue | 🟠 MajorSSE filter scope can be widened by clients.
Line 18–21 appends query
tasksIDs even when server-providedtaskIDsexist. This weakens server-side scoping and can stream unintended groups.Suggested fix
ids := append([]string{}, taskIDs...) - if q := r.URL.Query().Get("tasks"); q != "" { - ids = append(ids, strings.Split(q, ",")...) + if len(ids) == 0 { + if q := r.URL.Query().Get("tasks"); q != "" { + for _, id := range strings.Split(q, ",") { + id = strings.TrimSpace(id) + if id != "" { + ids = append(ids, id) + } + } + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@task/sse.go` around lines 18 - 21, The current logic appends client-provided query task IDs (r.URL.Query().Get("tasks")) to the server-provided taskIDs, widening the SSE scope; change it so the query `tasks` parameter is only used when the server-provided slice is empty (i.e., use the query IDs only if len(taskIDs) == 0), or otherwise ignore the query param; update the block that constructs `ids` (the variables `ids` and `taskIDs` in sse.go) to enforce this check so the server never expands its allowed tasks by concatenating client-supplied IDs.task/ui/src/components/TaskRow.tsx-15-18 (1)
15-18:⚠️ Potential issue | 🟠 MajorClickable row is not keyboard-accessible.
Line 15–18 uses a mouse-only clickable container. Add keyboard semantics (
role,tabIndex, and Enter/Space handling) whenhasLogsis true.Suggested fix
<div class={`flex items-start gap-3 py-2 border-b border-gray-100 last:border-0${hasLogs ? ' cursor-pointer hover:bg-gray-50 rounded -mx-1 px-1' : ''}`} onClick={hasLogs ? onToggle : undefined} + role={hasLogs ? 'button' : undefined} + tabIndex={hasLogs ? 0 : undefined} + onKeyDown={hasLogs ? (e) => { + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault(); + onToggle(); + } + } : undefined} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@task/ui/src/components/TaskRow.tsx` around lines 15 - 18, The clickable container div in TaskRow.tsx uses onClick only and must be keyboard-accessible; when hasLogs is true, add role="button" and tabIndex={0} to the div and wire a keyDown handler that calls onToggle when Enter (key === 'Enter') or Space (key === ' ' or key === 'Spacebar') is pressed (prevent default for Space), preserving the existing onClick behavior; update the JSX around the div (the element using hasLogs and onToggle) to conditionally add these attributes and the handler so keyboard users can toggle logs.task/snapshot.go-73-80 (1)
73-80:⚠️ Potential issue | 🟠 MajorCount every terminal status in the group aggregates.
warning,canceled, andSKIPnever increment any bucket here, so a finished group can still reportTotalgreater thanCompleted + Failed + Running. The new demo already hits this viat.Warning(), so the aggregate stats drift immediately.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@task/snapshot.go` around lines 73 - 80, The switch over t.Status() currently ignores terminal statuses (warning, canceled, SKIP) so group totals don't sum; update the switch in snapshot.go (the block using t.Status(), snap.Completed, snap.Failed, snap.Running) to handle StatusWarning, StatusCanceled, and StatusSKIP: if the snapshot struct defines explicit counters (e.g., snap.Skipped, snap.Canceled, snap.Warning) increment those; otherwise treat them as terminal failures and increment snap.Failed so Total == Completed + Failed + Running. Ensure you add the new case labels alongside StatusSuccess/StatusFailed/StatusRunning to cover all terminal states.formatters/html_react_formatter.go-68-70 (1)
68-70:⚠️ Potential issue | 🟠 MajorDon't short-circuit empty
PrettyDatato"".This skips the HTML shell entirely for valid empty values, so the html-react formatter returns a blank page instead of a mountable document.
💡 Minimal fix
- if prettyData == nil || prettyData.IsEmpty() { + if prettyData == nil { return "", nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@formatters/html_react_formatter.go` around lines 68 - 70, The current early return when prettyData == nil || prettyData.IsEmpty() causes the html-react formatter to emit an empty string instead of the full HTML shell; instead remove or change this short-circuit so the formatter always produces the mountable HTML document: if prettyData is nil, instantiate or treat it as an empty PrettyData and let the existing rendering path continue (do not return ""), ensuring code paths that reference prettyData (methods like IsEmpty) still behave safely while the HTML wrapper is always emitted.formatters/html_react_formatter.go-82-84 (1)
82-84:⚠️ Potential issue | 🟠 MajorDon't include
pd.Originalin the default client payload.This sends the raw input object to the browser, bypassing the pretty-data projection and potentially leaking fields that were intentionally not rendered. It also makes
json.Marshalfail on otherwise renderable values with non-serializable members.🔒 Minimal fix
func convertPrettyData(pd *api.PrettyData) reactData { rd := convertTypedValue(&pd.TypedValue, pd.Schema) - rd.Original = pd.Original if pd.Schema != nil && rd.Schema == nil { rd.Schema = convertSchema(pd.Schema)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@formatters/html_react_formatter.go` around lines 82 - 84, convertPrettyData currently copies pd.Original into the client payload (rd.Original = pd.Original), which leaks raw input and can break json.Marshal; remove that assignment so convertPrettyData does not include pd.Original in the default reactData, and if the raw Original must be preserved for internal use instead add an explicit opt-in (e.g. an includeOriginal bool parameter) or mark reactData.Original with a json:"-" tag so it isn't serialized; update usages of convertPrettyData accordingly.examples/uber_demo/main.go-1119-1140 (1)
1119-1140:⚠️ Potential issue | 🟠 MajorReturn invalid
--spread/ server startup errors to Cobra.Silently treating a bad duration as zero and logging
ListenAndServefailures makes this command exit successfully on misconfiguration. Because the producer goroutine starts first, a bind failure can still kick off background task generation with no UI attached.💡 Minimal fix
if opts.Spread != "" { - if d, err := time.ParseDuration(opts.Spread); err == nil { - spread = d - } + d, err := time.ParseDuration(opts.Spread) + if err != nil { + return nil, fmt.Errorf("invalid --spread: %w", err) + } + spread = d } go func() { time.Sleep(1 * time.Second) if opts.Count > 0 { runScaleTasks(opts.Count, spread) @@ addr := fmt.Sprintf(":%d", opts.Port) clicky.Infof("Task UI at http://localhost%s", addr) - if err := http.ListenAndServe(addr, handler); err != nil { - fmt.Fprintf(os.Stderr, "Server error: %v\n", err) - } - return nil, nil + return nil, http.ListenAndServe(addr, handler)If you also want to avoid starting work on bind failures, bind with
net.Listenbefore launching the goroutine.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/uber_demo/main.go` around lines 1119 - 1140, The command currently swallows a bad --spread value and logs ListenAndServe errors, causing the command to exit successfully; change behavior so that when time.ParseDuration(opts.Spread) returns an error you return that error (propagating it to Cobra) instead of treating it as zero, and propagate any server start error by returning the error from the function rather than only logging it; also consider calling net.Listen to bind the address before launching the producer goroutine (so if net.Listen fails you return the error immediately and do not call runScaleTasks/runGroupedTasks), and update references in this function (opts.Spread parsing, runScaleTasks, runGroupedTasks, taskui.Handler, and http.ListenAndServe/net.Listen) accordingly.task/snapshot.go-31-37 (1)
31-37:⚠️ Potential issue | 🟠 MajorUse the task's stable identity for
TaskSnapshot.ID.
TaskGroupkeys rows and stores expansion state byt.id; reusing the display name here means duplicate task names collide, so expanding one row can affect another.Taskalready has a uniqueidentityfield.💡 Minimal fix
snap := TaskSnapshot{ - ID: t.Name(), + ID: t.identity, Name: t.Name(), Type: "task", Group: groupName, Status: string(t.Status()), } + if snap.ID == "" { + snap.ID = t.Name() + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@task/snapshot.go` around lines 31 - 37, The TaskSnapshot.ID is using the display name (t.Name()) which causes collisions; change it to use the task's stable unique identity field (t.identity) instead while leaving Name as t.Name(); update the code that constructs TaskSnapshot (the ID assignment) to assign t.identity rather than t.Name() so TaskGroup row keys and expansion state are stable.formatters/html/react/bootstrap.js-47-54 (1)
47-54:⚠️ Potential issue | 🟠 MajorHandle parent-directory virtual imports too.
The virtual FS only rewrites
./...imports. A nested component file importing../sharedfalls through as external, so the blob bundle fails at runtime as soon as someone organizes the component into subdirectories.💡 Suggested normalization
- if (args.namespace === "virtual" && args.path.startsWith("./")) { - const dir = args.importer.replace(/\/[^/]+$/, ""); - return { path: dir + "/" + args.path.slice(2), namespace: "virtual" }; + if (args.namespace === "virtual" && /^(?:\.{1,2})\//.test(args.path)) { + const base = args.importer.replace(/\/[^/]+$/, "/"); + const path = new URL(args.path, `https://virtual${base}`).pathname; + return { path, namespace: "virtual" }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@formatters/html/react/bootstrap.js` around lines 47 - 54, The current build.onResolve handler only rewrites virtual imports that start with "./", so imports that use parent-directory syntax like "../shared" end up marked external; update the logic in build.onResolve (the handler that checks args.namespace === "virtual") to also match "../" (e.g. /^\.{1,2}\//) and compute the resolved path by taking args.importer directory and joining the relative import, normalizing any ".." segments so the resulting path stays inside the virtual namespace; ensure the returned object still sets namespace: "virtual" for these resolved parent-directory imports.task/ui/src/components/TaskGroup.tsx-70-105 (1)
70-105:⚠️ Potential issue | 🟠 MajorUse real buttons for the expand/collapse controls.
These controls use
<div>+onClickand lack keyboard support. Keyboard and screen reader users cannot access these affordances. Three instances in TaskGroup.tsx (lines 70–77, 78–85, 99–106) need conversion to semantic<button>elements withtype="button". A similar issue exists in TaskRow.tsx (lines 15–18) for the logs expansion control.Swap
<div>to<button type="button">and addw-full text-leftclasses to preserve layout, since buttons are inline by default. Verify the controls are tabbable and activate on Enter/Space after the change.Example fix for TaskGroup expand control
{!showAll && totalHidden > 0 && ( - <div - class="text-xs text-gray-400 py-1.5 cursor-pointer hover:text-gray-600 border-b border-gray-100" - onClick={() => setShowAll(true)} - > + <button + type="button" + class="w-full text-left text-xs text-gray-400 py-1.5 cursor-pointer hover:text-gray-600 border-b border-gray-100" + onClick={() => setShowAll(true)} + > ... {totalHidden} more tasks - </div> + </button> )}Apply the same conversion to the collapse control and the pending tasks control in TaskGroup, and to the logs expansion control in TaskRow.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@task/ui/src/components/TaskGroup.tsx` around lines 70 - 105, The expand/collapse affordances in TaskGroup (the "... {totalHidden} more tasks" control, the "▲ collapse" control, and the "... {hiddenPending} more pending" control) and the logs expansion control in TaskRow are currently non-semantic <div> elements with onClick handlers; replace each with a semantic <button type="button">, keep the existing onClick handlers (e.g., the setShowAll(true)/setShowAll(false) callbacks and onToggle usage), add the classes "w-full text-left" to preserve layout and any existing tailwind classes, and ensure they remain tabbable and activate with Enter/Space; update TaskRow's logs-expansion control the same way so keyboard and screen-reader users can operate it.formatters/html_react_formatter.go-262-267 (1)
262-267:⚠️ Potential issue | 🟠 MajorEncode component source before embedding in HTML.
The
ReactComponentis user-controlled input embedded directly into<script>elements without encoding. Any</script>sequence in the JSX source closes the tag prematurely, breaking the HTML structure. For example:customJSX = "code</script><script>alert('xss')</script><script>"generates:
<script id="clicky-custom-component" type="text/plain">code</script><script>alert('xss')</script><script></script>The malicious script executes before the bootstrap code runs. Encode the source as a JSON string literal or apply HTML entity encoding, then decode in
bootstrap.js.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@formatters/html_react_formatter.go` around lines 262 - 267, The embedded React component sources (customJSX and reactAssets.DefaultComponent) are written raw into the <script id="clicky-custom-component"> and <script id="clicky-default-component"> tags (in html_react_formatter.go), allowing a user-supplied '</script>' to break out and inject JS; instead JSON-string-encode or HTML-entity-escape the component source before calling b.WriteString(customJSX) and b.WriteString(reactAssets.DefaultComponent) (so the written content is safe), and update bootstrap.js to decode/parse that encoded string back into the original JSX at runtime; locate the writes around the script tags and replace them with the encoded output from the encoding helper you add.
🟡 Minor comments (12)
entity.go-248-252 (1)
248-252:⚠️ Potential issue | 🟡 MinorAdmin list operation missing
_idinjection.The main entity's list operation wraps results with
withEntityIDs(line 123), but the admin list does not. This inconsistency means admin list responses won't have the_idfield for frontend linking.Proposed fix
DataFunc: func(flagMap map[string]string, args []string) (any, error) { opts := buildOpts[ListOpts](flagMap) - return admin.List(opts) + items, err := admin.List(opts) + if err != nil { + return nil, err + } + return withEntityIDs(items), nil },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@entity.go` around lines 248 - 252, The admin DataFunc currently calls admin.List(opts) and returns raw results without injecting the entity `_id`; wrap the returned list through withEntityIDs to match the main list behavior. Update the DataFunc in the block that builds opts via buildOpts[ListOpts] so it calls result, err := admin.List(opts); if err != nil return nil, err; return withEntityIDs(result), nil (preserving error handling and types), referencing the DataFunc, buildOpts[ListOpts], admin.List, and withEntityIDs symbols.lint/testdata/src/github.com/flanksource/clicky/api/stub.go-1-36 (1)
1-36:⚠️ Potential issue | 🟡 MinorFix gofmt formatting to pass CI.
The pipeline reports that this file is not formatted correctly. Run
go fmt ./...orgofmt -s -w lint/testdata/src/github.com/flanksource/clicky/api/stub.goto fix.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lint/testdata/src/github.com/flanksource/clicky/api/stub.go` around lines 1 - 36, The file containing the Textable/Text/TextBuilder definitions (types Textable, Text, TextBuilder and functions NewText, SuccessText, ErrorText, and the methods like Appendf, Styles, Build) is not gofmt-formatted; run gofmt (or go fmt ./...) or gofmt -s -w on this file to fix spacing and alignment so the declarations and method receivers conform to gofmt standards, then re-run CI. Ensure you format the file containing Text, its methods (String, ANSI, HTML, Markdown, Append, Appendf, Add, Space, Styles), and TextBuilder methods (Bold, Color, Build) so the file passes gofmt.mcp/install.go-75-75 (1)
75-75:⚠️ Potential issue | 🟡 MinorFlag description inconsistent with actual path.
The
--globalflag description mentions~/.claude/settings.json, butclaudeSettingsPath(true)returns~/.mcp.json.📝 Suggested fix
- cmd.Flags().BoolVar(&global, "global", false, "Install in global Claude Code settings (~/.claude/settings.json)") + cmd.Flags().BoolVar(&global, "global", false, "Install in global MCP settings (~/.mcp.json)")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcp/install.go` at line 75, The flag description for the --global flag is inconsistent with the actual path returned by claudeSettingsPath(true); update the description string passed to cmd.Flags().BoolVar (the line that defines the --global flag) to reference the correct path (~/.mcp.json) so it matches claudeSettingsPath(true), or alternatively change claudeSettingsPath(true) to return ~/.claude/settings.json if that was intended—ensure the text in the BoolVar call and the behavior of claudeSettingsPath(true) are aligned.mcp/command.go-95-108 (1)
95-108:⚠️ Potential issue | 🟡 MinorAppend-based merge may duplicate Include/Exclude patterns.
If
InitialConfig.Tools.ExcludeorInitialConfig.Tools.Includecontain patterns already present in the loaded config (e.g.,"mcp"is inDefaultConfig().Tools.Exclude), they'll be duplicated. This causes redundant regex matching and confusing output indisplayServerInfo.Consider deduplicating or using a set-based merge:
♻️ Suggested fix to deduplicate patterns
+func appendUnique(slice []string, items ...string) []string { + seen := make(map[string]bool, len(slice)) + for _, s := range slice { + seen[s] = true + } + for _, item := range items { + if !seen[item] { + slice = append(slice, item) + seen[item] = true + } + } + return slice +} // Merge initial config (from NewCommandWithConfig) if ic := opts.InitialConfig; ic != nil { if ic.Name != "" { config.Name = ic.Name } if ic.Version != "" { config.Version = ic.Version } if ic.Tools.AutoExpose { config.Tools.AutoExpose = true } - config.Tools.Exclude = append(config.Tools.Exclude, ic.Tools.Exclude...) - config.Tools.Include = append(config.Tools.Include, ic.Tools.Include...) + config.Tools.Exclude = appendUnique(config.Tools.Exclude, ic.Tools.Exclude...) + config.Tools.Include = appendUnique(config.Tools.Include, ic.Tools.Include...) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcp/command.go` around lines 95 - 108, The merge for InitialConfig (opts.InitialConfig / ic) blindly appends ic.Tools.Exclude and ic.Tools.Include into config.Tools.Exclude/Include causing duplicate patterns (e.g., from DefaultConfig()); change the merge in the block that handles ic to deduplicate before appending — e.g., build a set/map of existing entries from config.Tools.Exclude and config.Tools.Include and only add entries from ic.Tools.Exclude and ic.Tools.Include that are not already present, so displayServerInfo and regex matching do not show or run duplicates.mcp/install.go-79-85 (1)
79-85:⚠️ Potential issue | 🟡 MinorSilent error handling for UserHomeDir.
If
os.UserHomeDir()fails,homewill be empty, resulting in/.mcp.jsonas the path, which is likely not writable and will cause a confusing error later.🛡️ Suggested fix to propagate error
-func claudeSettingsPath(global bool) string { +func claudeSettingsPath(global bool) (string, error) { if global { - home, _ := os.UserHomeDir() - return filepath.Join(home, ".mcp.json") + home, err := os.UserHomeDir() + if err != nil { + return "", fmt.Errorf("failed to get home directory: %w", err) + } + return filepath.Join(home, ".mcp.json"), nil } - return ".mcp.json" + return ".mcp.json", nil }Then update the caller to handle the error.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcp/install.go` around lines 79 - 85, The claudeSettingsPath function swallows errors from os.UserHomeDir which can produce an invalid path (e.g., "/.mcp.json"); change claudeSettingsPath to return an error alongside the path (e.g., func claudeSettingsPath(global bool) (string, error)), propagate the error when os.UserHomeDir() fails instead of ignoring it, and update all callers to handle the returned error (check and surface the error to the user or fallback appropriately) so failures to determine the home directory are not silently masked.mcp/server.go-85-108 (1)
85-108:⚠️ Potential issue | 🟡 MinorUnbuffered channel may cause unprocessed lines on scan error.
If
scanner.Err()returns an error after some lines were already sent to thelineschannel but not yet processed, the main loop'sselectcould receive fromscanErrfirst (due to non-deterministic select), causing the function to return immediately and potentially losing buffered lines.Consider draining the
lineschannel before returning on scan error, or use a bufferedlineschannel if losing lines on error is acceptable.🛡️ Suggested approach to drain lines before returning on error
case err := <-scanErr: + // Drain any remaining lines before returning + for line := range lines { + if line == "" { + continue + } + response, err := s.handleJSONRPCRequest(ctx, line) + if err != nil { + log.Printf("Error handling request: %v", err) + continue + } + if response != nil { + responseJSON, _ := json.Marshal(response) + fmt.Println(string(responseJSON)) + } + } return fmt.Errorf("stdin scan error: %w", err)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcp/server.go` around lines 85 - 108, The goroutine sending to lines and scanErr can leave unread lines when the main loop receives scanErr first; change the behavior so the main loop drains remaining lines before returning on a scan error or make lines buffered: either (A) make lines a buffered channel (e.g., make(chan string, N)) large enough for expected bursts, or (B) on receiving from scanErr in the select, loop to drain remaining values from the lines channel until it's closed (for line := range lines { /* process or discard */ }) then return fmt.Errorf("stdin scan error: %w", err); additionally ensure the sender in the goroutine writes scanErr in a non-blocking way (use select { case scanErr <- err: default: }) so the goroutine cannot block when main stops reading, and keep the existing close(lines) call to signal EOF.formatters/html/react/default_component.jsx-70-79 (1)
70-79:⚠️ Potential issue | 🟡 MinorMissing dependency in
useMemo.
colNamesis used insideuseMemobut not listed in the dependency array. ESLint'sreact-hooks/exhaustive-depsrule would flag this.Proposed fix
const sortedRows = useMemo(() => { if (sortCol === null) return table.rows; const name = colNames[sortCol]; return [...table.rows].sort((a, b) => { const av = (a[name]?.text || a[name]?.html || "").toLowerCase(); const bv = (b[name]?.text || b[name]?.html || "").toLowerCase(); const cmp = av < bv ? -1 : av > bv ? 1 : 0; return sortAsc ? cmp : -cmp; }); - }, [table.rows, sortCol, sortAsc]); + }, [table.rows, sortCol, sortAsc, colNames]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@formatters/html/react/default_component.jsx` around lines 70 - 79, The useMemo block that computes sortedRows references colNames but doesn't include it in the dependency array; update the dependency array for the useMemo that defines sortedRows (the useMemo surrounding sortedRows) to include colNames along with table.rows, sortCol, and sortAsc so the memo updates when colNames changes, or alternatively ensure colNames is memoized/stable before use and then include that stable reference in the dependency list.formatters/html/react/default_component.jsx-2-2 (1)
2-2:⚠️ Potential issue | 🟡 MinorRemove unused imports.
CompactTable,Section, andBadgeare imported but never used in the component.Proposed fix
import React, { useState, useCallback, useMemo } from "react"; -import { CompactTable, Section, Badge } from "@facet";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@formatters/html/react/default_component.jsx` at line 2, The import statement in default_component.jsx includes unused symbols CompactTable, Section, and Badge; remove these unused imports from the import declaration (or replace the import with only the actually used exports) so the file no longer imports CompactTable, Section, and Badge unnecessarily.formatters/options.go-41-43 (1)
41-43:⚠️ Potential issue | 🟡 Minor
ReactComponentfield is not merged inMergeOptions.The new
ReactComponentfield is added toFormatOptions, but theMergeOptionsfunction (lines 60-133) does not include logic to merge this field. This may cause the field value to be lost when options are merged.Proposed fix to add ReactComponent merging
Add the following in
MergeOptionsaround line 98 (after the depth handling):if opt.depth > 0 { merged.depth = opt.depth } + if opt.ReactComponent != "" { + merged.ReactComponent = opt.ReactComponent + } if opt.JSON {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@formatters/options.go` around lines 41 - 43, MergeOptions currently doesn't propagate the new ReactComponent field on FormatOptions, so values can be lost; update MergeOptions to copy ReactComponent from the source options into the target when the target's ReactComponent is empty (or zero) similar to how other string fields are handled (place this near the depth handling in MergeOptions), referencing the FormatOptions.ReactComponent field and the MergeOptions function to locate and implement the check and assignment.task/ui/handler.go-21-24 (1)
21-24:⚠️ Potential issue | 🟡 MinorRoot UI route should enforce GET-only semantics.
Line 21–24 serves HTML for all methods. Since this endpoint is documented as
GET /, return405for non-GET requests.Suggested fix
mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodGet { + http.Error(w, "method not allowed", http.StatusMethodNotAllowed) + return + } w.Header().Set("Content-Type", "text/html") fmt.Fprint(w, pageHTML()) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@task/ui/handler.go` around lines 21 - 24, The root handler registered via mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { ... }) currently serves pageHTML() for all HTTP methods; change it to enforce GET-only semantics by checking r.Method at the start of that handler and, if it is not "GET", write a 405 status (and set an "Allow: GET" header) and return early; otherwise continue to set Content-Type and write pageHTML().task/ui/README.md-32-33 (1)
32-33:⚠️ Potential issue | 🟡 MinorSSE event payload shape is documented incorrectly.
Line 32 says SSE
datais an array, buttask/sse.goemits oneevent: taskperTaskSnapshotobject. Update docs to prevent client implementation mistakes.Suggested fix
-- `data: [TaskSnapshot, ...]` — array of updated task snapshots +- `event: task` + `data: { ...TaskSnapshot }` — one snapshot per SSE event - `event: done` — all tasks have completed🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@task/ui/README.md` around lines 32 - 33, The docs incorrectly state SSE `data` is an array; update the README to match task/sse.go where the server emits one `event: task` per single TaskSnapshot (use `data: TaskSnapshot`) rather than `data: [TaskSnapshot, ...]`, and clarify that `event: done` is emitted after all tasks complete (no array payload — typically no data or an empty payload). Reference the TaskSnapshot type and the `task`/`done` event names so clients implement one snapshot-per-`task` event.task/ui/README.md-7-20 (1)
7-20:⚠️ Potential issue | 🟡 MinorAdd a language tag to the fenced block.
Line 7 uses a fenced code block without a language, which is already flagged by markdownlint (MD040).
Suggested fix
-``` +```text handler.go HTTP handler: serves HTML page + JSON/SSE APIs ... -``` +```🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@task/ui/README.md` around lines 7 - 20, The README.md fenced code block (the file list starting with "handler.go HTTP handler...") lacks a language tag which fails markdownlint MD040; update that code fence in README.md by adding a language identifier (e.g., ```text) on the opening fence for the block that contains handler.go, assets.go, src/, App.tsx, etc., so the block becomes a tagged fenced code block and the linter warning is resolved.
🧹 Nitpick comments (3)
lint/analyzer.go (2)
151-170: Consider expanding the whitelist or using a pattern match for Pretty-prefixed functions.The hardcoded whitelist
Pretty,PrettyFull,PrettyRowmay need updates if new naming conventions emerge. Consider usingstrings.HasPrefix(name, "Pretty")for more flexibility.♻️ Optional: Use prefix matching instead of exact match
func checkFuncReturnType(pass *analysis.Pass, fn *ast.FuncDecl) { if fn.Type.Results == nil { return } name := fn.Name.Name - if name == "Pretty" || name == "PrettyFull" || name == "PrettyRow" { + if strings.HasPrefix(name, "Pretty") { return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lint/analyzer.go` around lines 151 - 170, The checkFuncReturnType function currently skips only exact names "Pretty", "PrettyFull", "PrettyRow" which is brittle; update the name check to allow any function with a Pretty prefix (or expand the whitelist) by replacing the exact-equals logic with a prefix check (e.g., use strings.HasPrefix(name, "Pretty")) on the local variable name in checkFuncReturnType so all Pretty* variants are exempt, and add an import for strings if needed; alternatively, document and centralize the whitelist so future additions are easy to update.
119-129:isFmtSprintfrelies on identifier name "fmt", not package path.This check matches any selector
fmt.Sprintf, including if someone shadows thefmtidentifier or imports the package under an alias. A more robust approach would usepass.TypesInfoto resolve the actual package. However, for a lint rule this is likely acceptable—false positives from shadowing are rare.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lint/analyzer.go` around lines 119 - 129, The helper isFmtSprintf should resolve the package via type info instead of relying on the identifier name: change its signature to accept the analysis pass or types.Info (e.g., isFmtSprintf(call *ast.CallExpr, pass *analysis.Pass) or isFmtSprintf(call *ast.CallExpr, info *types.Info), locate sel.X as an *ast.Ident, then use info.Uses[ident] and assert to *types.PkgName and check pkg.Imported().Path() == "fmt" while still verifying sel.Sel.Name == "Sprintf"; update all callers to pass the pass/info parameter accordingly so the check is robust against shadowing or aliasing.lint/analyzer_test.go (1)
25-29: The "good" test assertion doesn't verify absence of diagnostics.
analysistest.Runreturns[]*analysistest.Resultwith one entry per analyzed package, regardless of whether diagnostics were produced. The assertionExpect(results).NotTo(BeEmpty())only confirms a package was analyzed, not that it passed without issues.The
analysistestframework validates expected diagnostics via// wantcomments in test files. For the "good" fixture (which has no// wantcomments),analysistest.Runwill fail if any unexpected diagnostics are reported. So the test does work correctly—the framework handles it—but the explicit assertion is misleading.Consider removing or clarifying the assertion:
♻️ Clarify test intent
It("allows good api.Text usage patterns", func() { testdata := analysistest.TestData() - results := analysistest.Run(GinkgoT(), testdata, lint.Analyzer, "good") - Expect(results).NotTo(BeEmpty()) + // analysistest.Run fails if unexpected diagnostics occur (no // want comments in good.go) + analysistest.Run(GinkgoT(), testdata, lint.Analyzer, "good") })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lint/analyzer_test.go` around lines 25 - 29, The test currently calls analysistest.Run(GinkgoT(), testdata, lint.Analyzer, "good") but then has a misleading assertion Expect(results).NotTo(BeEmpty()); remove that Expect(...) line (or replace it with a short comment) because analysistest.Run already validates there are no diagnostics for the "good" fixture; update the It("allows good api.Text usage patterns"...) block to rely on analysistest.Run and not assert on results explicitly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 48698478-2be2-4a73-b4e0-6c1c52d16eaf
⛔ Files ignored due to path filters (2)
examples/uber_demo/go.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sum
📒 Files selected for processing (47)
Makefileapi/code.goapi/collapsed.goapi/table.goapi/text.goapi/tree_html.gocmd/clickylint/main.gocobra_command.goentity.goexamples/uber_demo/go.modexamples/uber_demo/main.goformatters/html/react/assets.goformatters/html/react/bootstrap.jsformatters/html/react/default_component.jsxformatters/html_react_formatter.goformatters/html_react_formatter_test.goformatters/manager.goformatters/options.gogo.modlint/analyzer.golint/analyzer_test.golint/plugin.golint/testdata/src/bad/bad.golint/testdata/src/github.com/flanksource/clicky/api/stub.golint/testdata/src/good/good.gomcp/command.gomcp/install.gomcp/registry.gomcp/server.gorpc/converter.gorpc/executor.gotask/api.gotask/snapshot.gotask/sse.gotask/ui/.gitignoretask/ui/README.mdtask/ui/assets.gotask/ui/handler.gotask/ui/src/App.tsxtask/ui/src/components/ProgressBar.tsxtask/ui/src/components/Summary.tsxtask/ui/src/components/TaskGroup.tsxtask/ui/src/components/TaskRow.tsxtask/ui/src/index.tsxtask/ui/src/types.tstask/ui/src/utils.tstask/ui/vite.config.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (6)
.github/workflows/test.yml (1)
35-41: Deduplicate repeated Node + frontend build steps to reduce drift.The same setup/build block appears twice. Consider extracting it into a reusable workflow or composite action so version/command changes stay in one place.
Also applies to: 73-79
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/test.yml around lines 35 - 41, The workflow repeats the Node setup + frontend build steps (the blocks using actions/setup-node@v4 with node-version and the "Build task-ui frontend" run: cd task/ui && npm ci && npm run build), so extract those steps into a reusable workflow or a composite action (e.g., create .github/workflows/reusable-build.yml or .github/actions/frontend-build) exposing node-version and working-directory inputs, then replace both duplicated blocks with a single uses: call to that reusable workflow/action; update callers to pass node-version and path as inputs so future changes live in one place..github/workflows/lint.yml (1)
34-40: Frontend build step correctly added before linting.The Node.js setup and frontend build are necessary because the Go embed directive requires
dist/taskui.jsto exist forgolangci-lintto pass. This aligns with the Makefile'stask-uitarget.Consider adding npm caching to speed up builds:
⚡ Optional: Add npm caching for faster builds
- name: Set up Node.js uses: actions/setup-node@v4 with: node-version: '22' + cache: 'npm' + cache-dependency-path: task/ui/package-lock.json - name: Build task-ui frontend run: cd task/ui && npm ci && npm run build🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/lint.yml around lines 34 - 40, The workflow now sets up Node.js (actions/setup-node@v4 with node-version '22') and runs the frontend build step ("Build task-ui frontend" running cd task/ui && npm ci && npm run build) so golangci-lint's Go embed of dist/taskui.js will succeed; ensure this step remains before the lint job and matches the Makefile's task-ui behavior (producing dist/taskui.js). To speed CI, add npm caching by enabling actions/cache for the task/ui node_modules and npm cache keys tied to package-lock.json (optional but recommended) so subsequent runs reuse installed packages.task/snapshot.go (1)
104-117: Redundant lock acquisition and slice copy.
SnapshotGroup(g)at line 108 already acquiresg.mu.RLock()and copiesg.Items. Then lines 110-112 acquire the same lock and copy the same slice again. This is safe but inefficient.Consider refactoring to return items from
SnapshotGroupor creating a combined helper that snapshots both group and tasks in a single lock acquisition.💡 Optional: Combine group and task snapshotting
+// snapshotGroupWithTasks returns the group snapshot and task snapshots in one lock acquisition. +func snapshotGroupWithTasks(g *Group) (TaskSnapshot, []TaskSnapshot) { + snap := TaskSnapshot{ + ID: g.Name(), + Name: g.Name(), + Type: "group", + Status: string(g.Status()), + } + g.mu.RLock() + items := g.Items + g.mu.RUnlock() + + var taskSnaps []TaskSnapshot + for _, item := range items { + t := item.GetTask() + snap.Total++ + // ... status counting ... + taskSnaps = append(taskSnaps, SnapshotTask(t, g.Name())) + } + return snap, taskSnaps +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@task/snapshot.go` around lines 104 - 117, The loop redundantly re-locks g.mu and re-copies g.Items after calling SnapshotGroup(g), which itself already takes the lock and copies items; modify SnapshotGroup (or add a new helper like SnapshotGroupWithItems) to return the copied items along with the group snapshot (or to snapshot both group and its tasks while holding g.mu), then in this loop call that function and iterate the returned items to call SnapshotTask(item.GetTask(), g.Name())—removing the separate g.mu.RLock()/RUnlock() and duplicate slice copy.task/sse.go (1)
53-56: Consider logging marshal errors for debuggability.Silently skipping snapshots that fail to marshal makes sense for stream continuity, but could hide serialization bugs. Consider logging these failures at debug level.
💡 Optional: Add debug logging
data, err := json.Marshal(snap) if err != nil { + // Consider: log.Debug("failed to marshal snapshot", "id", snap.ID, "error", err) continue }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@task/sse.go` around lines 53 - 56, The json.Marshal(snap) error is currently ignored which hides serialization failures; update the block around "data, err := json.Marshal(snap)" to log the error at debug (or trace) level including the snapshot identifier/context (e.g., snap or any available ID) and the error before continuing, so the stream still skips the bad snapshot but the failure is recorded for debugging; ensure you call the existing logger (or processLogger) used elsewhere in task/sse.go and keep the continue behavior after logging.formatters/html/react/default_component.jsx (1)
90-95: Make sortable headers keyboard-accessible.Line 91 binds sorting to mouse click on
<th>only. Wrap the label in a<button>(or add keyboard handlers/roles) so keyboard users can trigger sorting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@formatters/html/react/default_component.jsx` around lines 90 - 95, The header cell is only clickable via mouse; make it keyboard-accessible by replacing the plain label inside the <th> with a focusable control (e.g., a <button>) or adding appropriate keyboard handlers and ARIA role/ tabindex. Update the <th> where handleSort(i), sortCol, and sortAsc are used so the label is rendered inside a <button> (or element with role="button" and onKeyDown handling Enter/Space) and move the onClick to that control; ensure the visual indicator still uses sortCol === i and sortAsc to show ↑/↓ and that the control has an accessible name/aria-sort where appropriate.formatters/html_react_formatter_test.go (1)
59-99: Add focused tests forTypedValue.MapandTextTable.RowDetailconversion.Current cases miss map and row-detail payload behavior, which are high-risk paths for silent data loss in this formatter.
Also applies to: 136-206
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@formatters/html_react_formatter_test.go` around lines 59 - 99, Add focused unit tests that exercise TypedValue.Map and TextTable.RowDetail conversion paths in the formatter: create a TextTable whose Rows include a field with a TypedValue containing a Map (TypedValue.Map) and ensure formatter.Format(table, FormatOptions{}) emits the map entries in the output; also create a TextTable with RowDetail populated (TextTable.RowDetail) and assert formatter.Format returns the row-detail payload (e.g., nested detail text or structured data) rather than dropping it. Target the existing test block around Describe("Data conversion") and reuse formatter.Format and FormatOptions to validate both presence and structure of the map and row-detail contents in the formatted output.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/table.go`:
- Around line 148-151: Looping over table.Headers and indexing row by
header.String() can yield nil Textable values when rows use table.FieldNames
keys; before calling Textable.HTML() in the render loop, check for nil and fall
back to the corresponding table.FieldNames[i] key or a safe empty Textable.
Specifically, in the block iterating table.Headers (use the same index i),
retrieve cellValue := row[header.String()]; if cellValue == nil then try
cellValue = row[table.FieldNames[i]]; if still nil replace it with a non-nil
empty Textable placeholder so calling Textable.HTML() is safe.
- Around line 156-157: Several places in api/table.go use
result.WriteString(fmt.Sprintf(...)) which triggers lint QF1012; replace each
occurrence with fmt.Fprintf(&result, ...) to write formatted strings directly
into the builder. Locate uses like the one building the expandable row HTML (the
lines that reference result, fmt.Sprintf and variables such as colCount) and
update them along with the other 14 occurrences (the places that call
result.WriteString(fmt.Sprintf(...)) at the noted line numbers) so that
fmt.Fprintf takes &result as the first argument and the same format string and
args follow. Ensure you import fmt if not already imported and keep the same
format strings and arguments when converting each call.
- Around line 255-260: The detail row is wrapped in a client-side Alpine
`<template x-if="open">` which PDF/static renderers ignore; change the logic in
the block that checks `if detail != nil` (the code that currently writes
`<template x-if="open"><tr>` and `</td></tr></template>`) so that for static/PDF
output you emit a normal `<tr>`/`<td colspan="%d">` containing `detail.HTML()`
(or otherwise avoid the `<template>` wrapper) — locate the calls that use
`result.WriteString` around the `<template x-if="open"><tr>` and replace them
with plain row output (still using `colCount` and `detail.HTML()`) when
rendering static HTML/PDF.
In `@formatters/html_react_formatter_test.go`:
- Around line 103-111: The test uses ES module syntax in customJSX (`import {
Section } from '@facet'; export default function App...`) while the formatter
emits a non-module script (`<script type="text/babel">`); update the test
fixture instead of changing formatter behavior: replace the module import/export
in the customJSX string with global-scope syntax (e.g., assume Section is
available as a global like window.Section and define `function App({ data }) {
return <Section ...>{...}</Section>; }` or assign to `window.App = ...`) so the
component matches the non-module script context; locate the customJSX variable
in the test around the formatter.Format call and update the string used in
FormatOptions{ReactComponent: customJSX}.
In `@formatters/html_react_formatter.go`:
- Around line 42-45: The conversion to the React payload currently drops per-row
detail content (api.TextTable.RowDetail); update the row serialization code (the
logic that builds reactTable.Rows between lines ~138-151) to include RowDetail
for each row by adding a field (e.g., "detail" or "rowDetail") in reactCell or
as a separate key in the row map so the UI can render expandable details; locate
the code that creates reactCell and populate it from TextTable.RowDetail (or
attach RowDetail to the row map) ensuring reactTable, reactColumn, and reactCell
structures are updated/handled accordingly so the serialized JSON includes the
per-row detail data.
- Around line 93-113: convertTypedValue currently ignores api.TypedValue.Map so
map-shaped values fall through to tv.String(); add a branch checking tv.Map !=
nil and route it to the appropriate map converter (e.g., call convertMap(tv.Map,
schema) or the existing convertTypedMap variant) before the final text fallback.
Update the convertTypedValue function to include "if tv.Map != nil { return
convertMap(tv.Map, schema) }" (or call the proper map-conversion helper) placed
before the return reactData{Type: "text", Text: tv.String()} so structured maps
are preserved.
In `@formatters/html/react/default_component.jsx`:
- Around line 37-39: The component is directly injecting unsanitized HTML via
dangerouslySetInnerHTML using data.html (occurrences in default_component.jsx
around the conditional that returns <div dangerouslySetInnerHTML={{ __html:
data.html }} /> and the other similar sinks), creating an XSS vector; fix it by
sanitizing or escaping the HTML before rendering (e.g., run data.html through a
vetted sanitizer/whitelist library or use a safe HTML renderer) and replace
direct uses of dangerouslySetInnerHTML with the sanitized result (or
conditionally render plain text when content is untrusted), updating every
occurrence (lines referenced by the review) so only sanitized content is passed
to dangerouslySetInnerHTML.
In `@task/snapshot.go`:
- Around line 99-102: The code currently uses global.mu.Lock()/Unlock() to read
global.groups; change to a read lock by calling global.mu.RLock() before
accessing global.groups and global.mu.RUnlock() after copying to avoid exclusive
locking during SnapshotAll (ensure the copy logic remains the same so the slice
contents are safely duplicated under the read lock).
- Around line 70-81: The switch in the loop over items (using item.GetTask() and
updating snap.Total/snap.Completed/snap.Failed/snap.Running) omits several
statuses (StatusPending, StatusWarning, StatusCancelled, StatusSKIP) so totals
don't reconcile; update the switch in the same loop to increment explicit
counters for these statuses (e.g., Pending, Warning, Cancelled, Skipped) on the
TaskSnapshot (or add a Pending field if preferred) so
Completed+Failed+Running+Pending+Warning+Cancelled+Skipped equals Total, and
ensure TaskSnapshot has the corresponding fields initialized and used wherever
snapshots are consumed.
- Around line 44-54: Replace direct field access of t.bufferedLogger with the
accessor t.getBufferedLogger() to follow the project's pattern: call logger :=
t.getBufferedLogger(), check logger != nil, then use logger.GetLogs() and
populate snap.Message and snap.Logs as before (using LogEntry and
e.Level.String()/e.Message). Update references in this block only (the snippet
that sets snap.Message and appends to snap.Logs) so behavior remains identical
but uses the getBufferedLogger() accessor.
---
Nitpick comments:
In @.github/workflows/lint.yml:
- Around line 34-40: The workflow now sets up Node.js (actions/setup-node@v4
with node-version '22') and runs the frontend build step ("Build task-ui
frontend" running cd task/ui && npm ci && npm run build) so golangci-lint's Go
embed of dist/taskui.js will succeed; ensure this step remains before the lint
job and matches the Makefile's task-ui behavior (producing dist/taskui.js). To
speed CI, add npm caching by enabling actions/cache for the task/ui node_modules
and npm cache keys tied to package-lock.json (optional but recommended) so
subsequent runs reuse installed packages.
In @.github/workflows/test.yml:
- Around line 35-41: The workflow repeats the Node setup + frontend build steps
(the blocks using actions/setup-node@v4 with node-version and the "Build task-ui
frontend" run: cd task/ui && npm ci && npm run build), so extract those steps
into a reusable workflow or a composite action (e.g., create
.github/workflows/reusable-build.yml or .github/actions/frontend-build) exposing
node-version and working-directory inputs, then replace both duplicated blocks
with a single uses: call to that reusable workflow/action; update callers to
pass node-version and path as inputs so future changes live in one place.
In `@formatters/html_react_formatter_test.go`:
- Around line 59-99: Add focused unit tests that exercise TypedValue.Map and
TextTable.RowDetail conversion paths in the formatter: create a TextTable whose
Rows include a field with a TypedValue containing a Map (TypedValue.Map) and
ensure formatter.Format(table, FormatOptions{}) emits the map entries in the
output; also create a TextTable with RowDetail populated (TextTable.RowDetail)
and assert formatter.Format returns the row-detail payload (e.g., nested detail
text or structured data) rather than dropping it. Target the existing test block
around Describe("Data conversion") and reuse formatter.Format and FormatOptions
to validate both presence and structure of the map and row-detail contents in
the formatted output.
In `@formatters/html/react/default_component.jsx`:
- Around line 90-95: The header cell is only clickable via mouse; make it
keyboard-accessible by replacing the plain label inside the <th> with a
focusable control (e.g., a <button>) or adding appropriate keyboard handlers and
ARIA role/ tabindex. Update the <th> where handleSort(i), sortCol, and sortAsc
are used so the label is rendered inside a <button> (or element with
role="button" and onKeyDown handling Enter/Space) and move the onClick to that
control; ensure the visual indicator still uses sortCol === i and sortAsc to
show ↑/↓ and that the control has an accessible name/aria-sort where
appropriate.
In `@task/snapshot.go`:
- Around line 104-117: The loop redundantly re-locks g.mu and re-copies g.Items
after calling SnapshotGroup(g), which itself already takes the lock and copies
items; modify SnapshotGroup (or add a new helper like SnapshotGroupWithItems) to
return the copied items along with the group snapshot (or to snapshot both group
and its tasks while holding g.mu), then in this loop call that function and
iterate the returned items to call SnapshotTask(item.GetTask(),
g.Name())—removing the separate g.mu.RLock()/RUnlock() and duplicate slice copy.
In `@task/sse.go`:
- Around line 53-56: The json.Marshal(snap) error is currently ignored which
hides serialization failures; update the block around "data, err :=
json.Marshal(snap)" to log the error at debug (or trace) level including the
snapshot identifier/context (e.g., snap or any available ID) and the error
before continuing, so the stream still skips the bad snapshot but the failure is
recorded for debugging; ensure you call the existing logger (or processLogger)
used elsewhere in task/sse.go and keep the continue behavior after logging.
🪄 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: 51812ffb-3d6d-45ad-8052-fcd4b258f0f4
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumtask/ui/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (22)
.github/workflows/lint.yml.github/workflows/test.yml.golangci.yamlapi/collapsed.goapi/table.goentity.goexamples/uber_demo/main.goformatters/html/react/assets.goformatters/html/react/default_component.jsxformatters/html_react_formatter.goformatters/html_react_formatter_test.gogo.modlint/testdata/src/github.com/flanksource/clicky/api/stub.gorpc/executor.gorpc/serve.gotask/api.gotask/snapshot.gotask/sse.gotask/task_fixture_test.gotask/ui/handler.gotask/ui/package.jsontask/ui/tsconfig.json
✅ Files skipped from review due to trivial changes (7)
- rpc/serve.go
- .golangci.yaml
- task/ui/tsconfig.json
- task/task_fixture_test.go
- task/ui/package.json
- lint/testdata/src/github.com/flanksource/clicky/api/stub.go
- examples/uber_demo/main.go
🚧 Files skipped from review as they are similar to previous changes (6)
- formatters/html/react/assets.go
- task/ui/handler.go
- go.mod
- entity.go
- api/collapsed.go
- task/api.go
Comment on lines
+42
to
+45
| type reactTable struct { | ||
| Columns []reactColumn `json:"columns"` | ||
| Rows []map[string]reactCell `json:"rows"` | ||
| } |
Contributor
There was a problem hiding this comment.
TextTable.RowDetail is currently discarded in React payload conversion.
Line 138+ serializes rows but not per-row detail content. That drops api.TextTable.RowDetail entirely, so downstream UI cannot render expandable row details even if source data has them.
💡 Proposed direction
type reactTable struct {
Columns []reactColumn `json:"columns"`
Rows []map[string]reactCell `json:"rows"`
+ RowDetail []reactData `json:"rowDetail,omitempty"`
}
@@
- for _, row := range t.Rows {
+ for i, row := range t.Rows {
rowMap := make(map[string]reactCell)
@@
rt.Rows = append(rt.Rows, rowMap)
+ if i < len(t.RowDetail) && t.RowDetail[i] != nil {
+ rt.RowDetail = append(rt.RowDetail, convertTextable(t.RowDetail[i]))
+ }
}Also applies to: 138-151
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@formatters/html_react_formatter.go` around lines 42 - 45, The conversion to
the React payload currently drops per-row detail content
(api.TextTable.RowDetail); update the row serialization code (the logic that
builds reactTable.Rows between lines ~138-151) to include RowDetail for each row
by adding a field (e.g., "detail" or "rowDetail") in reactCell or as a separate
key in the row map so the UI can render expandable details; locate the code that
creates reactCell and populate it from TextTable.RowDetail (or attach RowDetail
to the row map) ensuring reactTable, reactColumn, and reactCell structures are
updated/handled accordingly so the serialized JSON includes the per-row detail
data.
Comment on lines
+70
to
+81
| for _, item := range items { | ||
| t := item.GetTask() | ||
| snap.Total++ | ||
| switch t.Status() { | ||
| case StatusSuccess, StatusPASS: | ||
| snap.Completed++ | ||
| case StatusFailed, StatusFAIL, StatusERR: | ||
| snap.Failed++ | ||
| case StatusRunning: | ||
| snap.Running++ | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Incomplete status counting — several statuses are not tracked.
The switch statement doesn't account for StatusPending, StatusWarning, StatusCancelled, or StatusSKIP. This means:
Totalcounts all items, butCompleted + Failed + Runningwon't equalTotal- Pending tasks (waiting to start) are invisible in the counters
- Cancelled/Warning/Skipped tasks are silently lost from accounting
This could confuse the frontend or cause progress calculations to be inaccurate.
🐛 Suggested fix to handle all statuses
for _, item := range items {
t := item.GetTask()
snap.Total++
switch t.Status() {
case StatusSuccess, StatusPASS:
snap.Completed++
- case StatusFailed, StatusFAIL, StatusERR:
+ case StatusFailed, StatusFAIL, StatusERR, StatusCancelled:
snap.Failed++
case StatusRunning:
snap.Running++
+ case StatusPending:
+ // Pending tasks are implicitly: Total - Completed - Failed - Running
+ // Or add a Pending field to TaskSnapshot if needed
+ case StatusWarning, StatusSKIP:
+ snap.Completed++ // Completed with warnings/skipped
}
}Alternatively, add a Pending counter field to TaskSnapshot for explicit tracking.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@task/snapshot.go` around lines 70 - 81, The switch in the loop over items
(using item.GetTask() and updating
snap.Total/snap.Completed/snap.Failed/snap.Running) omits several statuses
(StatusPending, StatusWarning, StatusCancelled, StatusSKIP) so totals don't
reconcile; update the switch in the same loop to increment explicit counters for
these statuses (e.g., Pending, Warning, Cancelled, Skipped) on the TaskSnapshot
(or add a Pending field if preferred) so
Completed+Failed+Running+Pending+Warning+Cancelled+Skipped equals Total, and
ensure TaskSnapshot has the corresponding fields initialized and used wherever
snapshots are consumed.
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 (1)
formatters/options.go (1)
60-98:⚠️ Potential issue | 🟡 MinorMissing merge logic for
ReactComponentfield.The
MergeOptionsfunction handles all existing fields but does not merge the newly addedReactComponentfield. If multipleFormatOptionsare merged whereReactComponentis set, the value will be lost.🔧 Proposed fix to add merge logic for ReactComponent
if opt.depth > 0 { merged.depth = opt.depth } + if opt.ReactComponent != "" { + merged.ReactComponent = opt.ReactComponent + } if opt.JSON { merged.JSON = true continue // Only one format can be set🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@formatters/options.go` around lines 60 - 98, The MergeOptions function ignores the new ReactComponent field causing it to be lost when merging; update MergeOptions to check opt.ReactComponent (from the incoming FormatOptions) and, if non-empty/non-nil as appropriate for its type, assign it to merged.ReactComponent so the final FormatOptions retains the ReactComponent value (i.e., add logic alongside the other fields in MergeOptions to set merged.ReactComponent = opt.ReactComponent when present).
♻️ Duplicate comments (1)
task/snapshot.go (1)
64-80:⚠️ Potential issue | 🟠 MajorHandle the full task status enum before exposing group snapshots.
Task.Status()can return more than the few cases handled here. Right nowwarning,canceled, andSKIPnever hit a counter, andGroup.Status()can still surface"success"for children that finished asFAIL/ERR. That makes the new JSON/SSE snapshot disagree with the underlying tasks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@task/snapshot.go` around lines 64 - 80, The snapshot loop currently only tallies a subset of Task.Status() values causing missed counters (warning, canceled, SKIP) and potential mismatch with Group.Status(); update the switch over t.Status() (used on item.GetTask()) to include cases for StatusWarning, StatusCanceled, StatusSkip (or whatever enum names map to warning/canceled/SKIP) and increment distinct counters (e.g., snap.Warning++, snap.Canceled++, snap.Skipped++) and keep Total/Completed/Failed/Running as-is; additionally, ensure the snapshot's top-level status uses the group's computed aggregate status (recompute from child statuses if necessary) instead of directly trusting g.Status() so the JSON/SSE reflects actual child outcomes.
🤖 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 274-279: The DataFunc currently pulls id from flagMap/args and
directly calls a.Run(id, flagMap) without validating id; update DataFunc (the
anonymous func assigned to DataFunc) to check if id == "" and return a
descriptive error (same style as the main entity actions) before calling
a.Run(id, flagMap) so empty IDs are rejected early.
- Around line 248-251: The admin list DataFunc returns items directly from
admin.List causing JSON inconsistency; modify the DataFunc (the anonymous
function assigned to DataFunc using buildOpts[ListOpts]) to call
admin.List(opts), take the returned items slice, wrap it with
withEntityIDs(items) before returning so admin.List's output matches the main
entity list's use of withEntityIDs and injects _id fields consistently.
- Around line 563-579: The MarshalJSON implementation in
entityWithID[T].MarshalJSON can produce invalid JSON when e.Inner marshals to an
empty object ("{}") because appending a comma before data[1:] yields a trailing
comma; modify entityWithID[T].MarshalJSON to detect the empty-object case (e.g.,
len(data)==2 and data[0]=='{' and data[1]=='}' or data equals "{}") and, instead
of inserting a comma+data[1:], return a byte sequence that is {"_id":<idJSON>}
(no trailing comma), otherwise keep the existing logic of prefix + ',' +
data[1:]; ensure you still handle json.Marshal errors for e.Inner and e.ID as
before.
In `@examples/uber_demo/main.go`:
- Around line 1135-1140: The server startup error is being swallowed: replace
the current http.ListenAndServe error handling (addr, handler) so that any
failure is returned to the caller instead of only being printed and allowing the
function to return nil, nil; specifically, in the block that builds addr and
calls http.ListenAndServe(addr, handler), return a wrapped or raw error (e.g.,
fmt.Errorf("listen %s: %w", addr, err) or just err) from the function so Cobra
sees the failure rather than continuing to return nil, nil.
- Around line 1119-1123: The code silently ignores time.ParseDuration errors for
opts.Spread; update the block around the time.ParseDuration call (the
opts.Spread -> spread parsing) to fail fast by checking if err != nil and
returning or propagating the parse error (include opts.Spread in the error
message) instead of silently continuing—i.e., where
time.ParseDuration(opts.Spread) is called, replace the current ignore-err branch
with logic that returns fmt.Errorf("invalid --spread %q: %w", opts.Spread, err)
(or uses the surrounding function's error-return mechanism / log.Fatalf if
appropriate) so invalid durations are reported immediately.
In `@lint/analyzer.go`:
- Around line 48-50: The current isClickyModule uses
strings.HasPrefix(pass.Pkg.Path(), clickyModulePrefix) which incorrectly matches
packages like "github.com/flanksource/clicky-foo"; update isClickyModule to only
match the exact module path or a proper module boundary by checking
pass.Pkg.Path() == clickyModulePrefix || strings.HasPrefix(pass.Pkg.Path(),
clickyModulePrefix + "/") so it only returns true for the module itself or
subpackages; change the logic in isClickyModule to use that comparison
referencing pass.Pkg.Path() and clickyModulePrefix.
- Around line 119-129: Update the isFmtSprintf logic to use type info instead of
comparing identifier text: change the call site to pass the analysis.Pass into
isFmtSprintf (call isFmtSprintf(pass, v)) and modify the function signature to
func isFmtSprintf(pass *analysis.Pass, call *ast.CallExpr) bool; inside, ensure
you extract sel := call.Fun.(*ast.SelectorExpr) and then resolve the selector
symbol via pass.TypesInfo.Uses[sel.Sel], assert it is a *types.Func, check
fnObj.Pkg() != nil, and finally compare fnObj.Pkg().Path() == "fmt" and
fnObj.Name() == "Sprintf" so aliased imports and shadowing are handled
correctly.
- Around line 60-67: The function isClickyTextTypesType currently fails to
unwrap type aliases and only checks for *types.Named; modify it to call
types.Unalias(t) (or otherwise resolve aliases) at the start so alias forms like
"type MyText = api.Text" are unwrapped before assertions; then proceed to handle
*types.Named and pointer unwrapping as before (e.g., call types.Unalias on
ptr.Elem() when recursing) so comparisons against api.Text work for both direct
and aliased types.
In `@mcp/command.go`:
- Around line 95-108: The merge currently only copies
Name/Version/Tools.Include/Exclude and AutoExpose from opts.InitialConfig to
config; extend it to also copy Description, the entire Security struct fields
(e.g., RequireConfirmation, AuditLog, TimeoutSeconds, and any other
non-zero/explicitly-set fields) and Tools.Descriptions from opts.InitialConfig
so those settings are not dropped. In practice update the block that checks
opts.InitialConfig to: set config.Description when ic.Description is non-empty,
merge or overwrite config.Security with ic.Security (or copy individual Security
fields only when ic.Security fields are explicitly set), and append/merge
ic.Tools.Descriptions into config.Tools.Descriptions (similar to how
Tools.Include/Exclude are handled), ensuring you reference opts.InitialConfig
(ic), config, config.Security and config.Tools.Descriptions during the merge.
In `@mcp/install.go`:
- Line 75: The flag help text for the --global flag is incorrect: it says
"~/.claude/settings.json" but the code path used is claudeSettingsPath which
returns "~/.mcp.json"; update the help string passed to
cmd.Flags().BoolVar(&global, "global", ...) to accurately describe the actual
settings file (or alternatively change claudeSettingsPath to return the
~/.claude/settings.json path) so the flag help and the claudeSettingsPath
behavior match; locate the cmd.Flags().BoolVar call and the claudeSettingsPath
function to make the corresponding, consistent change.
- Around line 79-85: claudeSettingsPath currently ignores os.UserHomeDir()
errors which can produce "/.mcp.json"; change claudeSettingsPath to return
(string, error), call home, err := os.UserHomeDir() when global is true, and if
err != nil return "", err (or a sensible fallback you choose), otherwise return
filepath.Join(home, ".mcp.json"); update all callers to handle the error. This
ensures the error is propagated instead of silently producing an incorrect root
path.
- Around line 38-40: The current code unconditionally replaces binPath with
exec.LookPath(rootCmd.Use), which can point to a different binary; change the
logic in the installer initialization (the block using os.Executable and
exec.LookPath and the variables binPath/rootCmd.Use) to only accept the LookPath
result if it actually refers to the same file as the running executable: obtain
the running exe path via os.Executable(), resolve/evaluate symlinks
(filepath.EvalSymlinks) for both paths, and use os.SameFile or compare fileinfos
(os.Stat on both) to verify they match; if they do, set binPath =
lookPathResult, otherwise keep the original os.Executable() result and ignore
the PATH match and any errors encountered during the validation.
In `@rpc/converter.go`:
- Around line 303-311: The code currently reparses cmd.Use via
extractParameterName to insert a path placeholder, which can diverge from the
positional argument metadata created in ConvertCommand and cause mismatches with
RPCParameter and updateParameterLocations; instead, look up the positional
argument info already produced (the RPCParameter entries created by
ConvertCommand or the positional-args collection) and only insert "/{param}"
when a matching positional RPCParameter exists and is marked positional/required
according to that metadata, using that parameter's exact name and requiredness
to build pathParts so route, parameters, and required flags remain in sync
(avoid calling extractParameterName(cmd.Use) here).
- Around line 302-327: The current guard uses cmd.Parent() != nil which still
allows root-level direct children (e.g., status <id>) to be rewritten into
/{id}/action; tighten it by requiring an actual resource segment before
inserting the ID — update the condition around the block that references
cmd.Parent(), isCRUDOperation(cmd.Name()), extractParameterName(cmd.Use) and
pathParts so it also requires either cmd.Parent().Parent() != nil or
len(pathParts) > 1 (i.e., ensure there is a resource ancestor/segment before
treating the last segment as an action) before inserting the "{...}" path
parameter.
In `@task/snapshot.go`:
- Around line 31-36: The snapshot uses t.Name() for TaskSnapshot.ID which causes
UI key collisions; replace the ID field with a stable, collision-free identifier
(e.g., a canonical Task.ID()/UniqueID()/path-based ID or a deterministic hash
derived from task metadata) while keeping Name as the display label; update the
same pattern in the group snapshot construction (the block around lines 60-63)
so both task and group snapshots use their stable ID property instead of
t.Name().
In `@task/ui/README.md`:
- Around line 7-20: The README's fenced code block listing filenames lacks a
language tag, causing markdownlint failures; update the triple-backtick fence
that wraps the file list (the block containing handler.go, assets.go,
src/index.tsx, App.tsx, types.ts, utils.ts, and components/*) to include a
language such as text (e.g., replace ``` with ```text) so the linter recognizes
it as a code block.
- Around line 28-33: The README's SSE section is incorrect: the server actually
sends one SSE event per TaskSnapshot with event name "task" (not a single data
array), and each tick sends the full snapshot set (all tasks) rather than only
dirty tasks; update the `/api/tasks/stream` documentation to describe the wire
format precisely by documenting that the stream emits repeated `event: task`
messages with a single TaskSnapshot payload (or JSON-encoded snapshot) per
event, and that it also emits `event: done` when all tasks complete—ensure
examples and text reflect "task" events, full snapshot set semantics per tick,
and the exact JSON payload shape used by the handler.
In `@task/ui/src/App.tsx`:
- Line 15: The EventSource is using an absolute path ('/api/tasks/stream') which
ignores any handler prefix; in App.tsx update the EventSource instantiation
(const es = new EventSource('/api/tasks/stream');) to use a handler-relative URL
(e.g., remove the leading slash or build the URL against
document.baseURI/window.location so the request honors the current mount prefix)
so the SSE connects correctly when the UI is served under a path prefix.
In `@task/ui/src/components/TaskGroup.tsx`:
- Around line 70-83: Replace the clickable <div>s used for expand/collapse with
semantic <button type="button"> elements (preserving the existing onClick
handlers that call setShowAll and the visible text that uses totalHidden and
showAll) so they become keyboard-focusable and expose button semantics; ensure
you move the class attributes to the button (or className if this is JSX), add
aria-expanded={showAll} and an accessible label (or visually-hidden text) as
appropriate, and make the same change for the second occurrence referenced
(lines around the 99-105 block) so both expand and collapse controls are proper
buttons.
---
Outside diff comments:
In `@formatters/options.go`:
- Around line 60-98: The MergeOptions function ignores the new ReactComponent
field causing it to be lost when merging; update MergeOptions to check
opt.ReactComponent (from the incoming FormatOptions) and, if non-empty/non-nil
as appropriate for its type, assign it to merged.ReactComponent so the final
FormatOptions retains the ReactComponent value (i.e., add logic alongside the
other fields in MergeOptions to set merged.ReactComponent = opt.ReactComponent
when present).
---
Duplicate comments:
In `@task/snapshot.go`:
- Around line 64-80: The snapshot loop currently only tallies a subset of
Task.Status() values causing missed counters (warning, canceled, SKIP) and
potential mismatch with Group.Status(); update the switch over t.Status() (used
on item.GetTask()) to include cases for StatusWarning, StatusCanceled,
StatusSkip (or whatever enum names map to warning/canceled/SKIP) and increment
distinct counters (e.g., snap.Warning++, snap.Canceled++, snap.Skipped++) and
keep Total/Completed/Failed/Running as-is; additionally, ensure the snapshot's
top-level status uses the group's computed aggregate status (recompute from
child statuses if necessary) instead of directly trusting g.Status() so the
JSON/SSE reflects actual child outcomes.
🪄 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: b2822700-c4a9-45df-a1e8-28a2abf4af96
⛔ Files ignored due to path filters (3)
examples/uber_demo/go.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sumtask/ui/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (54)
.github/workflows/lint.yml.github/workflows/test.yml.golangci.yamlMakefileapi/code.goapi/collapsed.goapi/table.goapi/text.goapi/tree_html.gocmd/clickylint/main.gocobra_command.goentity.goexamples/uber_demo/go.modexamples/uber_demo/main.goformatters/html/react/assets.goformatters/html/react/default_component.jsxformatters/html_react_formatter.goformatters/html_react_formatter_test.goformatters/manager.goformatters/options.goformatters/tree_formatter.gogo.modlint/analyzer.golint/analyzer_test.golint/plugin.golint/testdata/src/bad/bad.golint/testdata/src/github.com/flanksource/clicky/api/stub.golint/testdata/src/good/good.gomcp/command.gomcp/install.gomcp/registry.gomcp/server.gorpc/converter.gorpc/executor.gorpc/serve.gotask/api.gotask/snapshot.gotask/sse.gotask/task_fixture_test.gotask/ui/.gitignoretask/ui/README.mdtask/ui/assets.gotask/ui/handler.gotask/ui/package.jsontask/ui/src/App.tsxtask/ui/src/components/ProgressBar.tsxtask/ui/src/components/Summary.tsxtask/ui/src/components/TaskGroup.tsxtask/ui/src/components/TaskRow.tsxtask/ui/src/index.tsxtask/ui/src/types.tstask/ui/src/utils.tstask/ui/tsconfig.jsontask/ui/vite.config.ts
✅ Files skipped from review due to trivial changes (18)
- task/ui/.gitignore
- task/ui/src/index.tsx
- formatters/html/react/assets.go
- task/ui/tsconfig.json
- cobra_command.go
- task/ui/assets.go
- task/ui/package.json
- .golangci.yaml
- lint/analyzer_test.go
- rpc/serve.go
- formatters/manager.go
- task/task_fixture_test.go
- task/ui/vite.config.ts
- lint/testdata/src/bad/bad.go
- task/ui/src/types.ts
- formatters/html_react_formatter_test.go
- lint/testdata/src/good/good.go
- task/ui/src/components/ProgressBar.tsx
🚧 Files skipped from review as they are similar to previous changes (24)
- formatters/tree_formatter.go
- api/code.go
- api/text.go
- Makefile
- .github/workflows/test.yml
- cmd/clickylint/main.go
- api/tree_html.go
- task/ui/handler.go
- task/api.go
- lint/plugin.go
- api/collapsed.go
- examples/uber_demo/go.mod
- task/ui/src/utils.ts
- task/ui/src/components/TaskRow.tsx
- .github/workflows/lint.yml
- task/ui/src/components/Summary.tsx
- formatters/html/react/default_component.jsx
- task/sse.go
- api/table.go
- mcp/server.go
- formatters/html_react_formatter.go
- go.mod
- lint/testdata/src/github.com/flanksource/clicky/api/stub.go
- rpc/executor.go
Comment on lines
+248
to
+251
| DataFunc: func(flagMap map[string]string, args []string) (any, error) { | ||
| opts := buildOpts[ListOpts](flagMap) | ||
| return admin.List(opts) | ||
| }, |
Contributor
There was a problem hiding this comment.
Inconsistent: Admin list does not wrap items with withEntityIDs.
The main entity's list operation at line 123 wraps items with withEntityIDs(items) to inject _id fields, but the admin list operation returns items directly without wrapping. This could cause inconsistent JSON output between admin and non-admin list operations.
🔧 Proposed fix for consistency
DataFunc: func(flagMap map[string]string, args []string) (any, error) {
opts := buildOpts[ListOpts](flagMap)
- return admin.List(opts)
+ items, err := admin.List(opts)
+ if err != nil {
+ return nil, err
+ }
+ return withEntityIDs(items), nil
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| DataFunc: func(flagMap map[string]string, args []string) (any, error) { | |
| opts := buildOpts[ListOpts](flagMap) | |
| return admin.List(opts) | |
| }, | |
| DataFunc: func(flagMap map[string]string, args []string) (any, error) { | |
| opts := buildOpts[ListOpts](flagMap) | |
| items, err := admin.List(opts) | |
| if err != nil { | |
| return nil, err | |
| } | |
| return withEntityIDs(items), nil | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@entity.go` around lines 248 - 251, The admin list DataFunc returns items
directly from admin.List causing JSON inconsistency; modify the DataFunc (the
anonymous function assigned to DataFunc using buildOpts[ListOpts]) to call
admin.List(opts), take the returned items slice, wrap it with
withEntityIDs(items) before returning so admin.List's output matches the main
entity list's use of withEntityIDs and injects _id fields consistently.
Comment on lines
+274
to
+279
| DataFunc: func(flagMap map[string]string, args []string) (any, error) { | ||
| id := flagMap["id"] | ||
| if id == "" && len(args) > 0 { | ||
| id = args[0] | ||
| } | ||
| return a.Run(id, flagMap) |
Contributor
There was a problem hiding this comment.
Missing ID validation in admin actions.
The admin action DataFunc does not validate that id is non-empty before calling a.Run(id, flagMap). Compare with the main entity actions at lines 205-207 which return an error when id == "". This could lead to empty IDs being passed to action handlers.
🔧 Proposed fix to add ID validation
DataFunc: func(flagMap map[string]string, args []string) (any, error) {
id := flagMap["id"]
if id == "" && len(args) > 0 {
id = args[0]
}
+ if id == "" {
+ return nil, fmt.Errorf("id is required")
+ }
return a.Run(id, flagMap)
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@entity.go` around lines 274 - 279, The DataFunc currently pulls id from
flagMap/args and directly calls a.Run(id, flagMap) without validating id; update
DataFunc (the anonymous func assigned to DataFunc) to check if id == "" and
return a descriptive error (same style as the main entity actions) before
calling a.Run(id, flagMap) so empty IDs are rejected early.
Comment on lines
+563
to
+579
| func (e entityWithID[T]) MarshalJSON() ([]byte, error) { | ||
| data, err := json.Marshal(e.Inner) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if len(data) < 2 || data[0] != '{' { | ||
| return data, nil | ||
| } | ||
| idJSON, err := json.Marshal(e.ID) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| prefix := []byte(`{"_id":`) | ||
| prefix = append(prefix, idJSON...) | ||
| prefix = append(prefix, ',') | ||
| return append(prefix, data[1:]...), nil | ||
| } |
Contributor
There was a problem hiding this comment.
Potential invalid JSON for empty inner objects.
If e.Inner marshals to an empty object {}, then data[1:] would be }, resulting in {"_id":"...",} which has a trailing comma and is invalid JSON. While this edge case may be rare if entities always have fields, it could cause parsing errors downstream.
🔧 Proposed fix to handle empty objects
func (e entityWithID[T]) MarshalJSON() ([]byte, error) {
data, err := json.Marshal(e.Inner)
if err != nil {
return nil, err
}
if len(data) < 2 || data[0] != '{' {
return data, nil
}
+ // Handle empty object case: {} -> just add _id field
+ if string(data) == "{}" {
+ idJSON, err := json.Marshal(e.ID)
+ if err != nil {
+ return nil, err
+ }
+ return []byte(fmt.Sprintf(`{"_id":%s}`, idJSON)), nil
+ }
idJSON, err := json.Marshal(e.ID)
if err != nil {
return nil, err
}
prefix := []byte(`{"_id":`)
prefix = append(prefix, idJSON...)
prefix = append(prefix, ',')
return append(prefix, data[1:]...), nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func (e entityWithID[T]) MarshalJSON() ([]byte, error) { | |
| data, err := json.Marshal(e.Inner) | |
| if err != nil { | |
| return nil, err | |
| } | |
| if len(data) < 2 || data[0] != '{' { | |
| return data, nil | |
| } | |
| idJSON, err := json.Marshal(e.ID) | |
| if err != nil { | |
| return nil, err | |
| } | |
| prefix := []byte(`{"_id":`) | |
| prefix = append(prefix, idJSON...) | |
| prefix = append(prefix, ',') | |
| return append(prefix, data[1:]...), nil | |
| } | |
| func (e entityWithID[T]) MarshalJSON() ([]byte, error) { | |
| data, err := json.Marshal(e.Inner) | |
| if err != nil { | |
| return nil, err | |
| } | |
| if len(data) < 2 || data[0] != '{' { | |
| return data, nil | |
| } | |
| // Handle empty object case: {} -> just add _id field | |
| if string(data) == "{}" { | |
| idJSON, err := json.Marshal(e.ID) | |
| if err != nil { | |
| return nil, err | |
| } | |
| return []byte(fmt.Sprintf(`{"_id":%s}`, idJSON)), nil | |
| } | |
| idJSON, err := json.Marshal(e.ID) | |
| if err != nil { | |
| return nil, err | |
| } | |
| prefix := []byte(`{"_id":`) | |
| prefix = append(prefix, idJSON...) | |
| prefix = append(prefix, ',') | |
| return append(prefix, data[1:]...), nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@entity.go` around lines 563 - 579, The MarshalJSON implementation in
entityWithID[T].MarshalJSON can produce invalid JSON when e.Inner marshals to an
empty object ("{}") because appending a comma before data[1:] yields a trailing
comma; modify entityWithID[T].MarshalJSON to detect the empty-object case (e.g.,
len(data)==2 and data[0]=='{' and data[1]=='}' or data equals "{}") and, instead
of inserting a comma+data[1:], return a byte sequence that is {"_id":<idJSON>}
(no trailing comma), otherwise keep the existing logic of prefix + ',' +
data[1:]; ensure you still handle json.Marshal errors for e.Inner and e.ID as
before.
Comment on lines
+1119
to
+1123
| if opts.Spread != "" { | ||
| if d, err := time.ParseDuration(opts.Spread); err == nil { | ||
| spread = d | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Fail fast on an invalid --spread value.
Silently ignoring a bad duration makes it look like the flag worked when it did not. Returning the parse error here will save a lot of head-scratching when someone mistypes the value.
💡 Suggested fix
if opts.Spread != "" {
- if d, err := time.ParseDuration(opts.Spread); err == nil {
- spread = d
- }
+ d, err := time.ParseDuration(opts.Spread)
+ if err != nil {
+ return nil, fmt.Errorf("invalid --spread value %q: %w", opts.Spread, err)
+ }
+ spread = d
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/uber_demo/main.go` around lines 1119 - 1123, The code silently
ignores time.ParseDuration errors for opts.Spread; update the block around the
time.ParseDuration call (the opts.Spread -> spread parsing) to fail fast by
checking if err != nil and returning or propagating the parse error (include
opts.Spread in the error message) instead of silently continuing—i.e., where
time.ParseDuration(opts.Spread) is called, replace the current ignore-err branch
with logic that returns fmt.Errorf("invalid --spread %q: %w", opts.Spread, err)
(or uses the surrounding function's error-return mechanism / log.Fatalf if
appropriate) so invalid durations are reported immediately.
Comment on lines
+1135
to
+1140
| addr := fmt.Sprintf(":%d", opts.Port) | ||
| clicky.Infof("Task UI at http://localhost%s", addr) | ||
| if err := http.ListenAndServe(addr, handler); err != nil { | ||
| fmt.Fprintf(os.Stderr, "Server error: %v\n", err) | ||
| } | ||
| return nil, nil |
Contributor
There was a problem hiding this comment.
Return server startup failures instead of swallowing them.
If the port is already in use or the bind fails, this command currently logs the error and still exits successfully. That should propagate back to Cobra as a real failure.
💡 Suggested fix
addr := fmt.Sprintf(":%d", opts.Port)
clicky.Infof("Task UI at http://localhost%s", addr)
if err := http.ListenAndServe(addr, handler); err != nil {
- fmt.Fprintf(os.Stderr, "Server error: %v\n", err)
+ return nil, fmt.Errorf("start task UI server on %s: %w", addr, err)
}
return nil, nil📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| addr := fmt.Sprintf(":%d", opts.Port) | |
| clicky.Infof("Task UI at http://localhost%s", addr) | |
| if err := http.ListenAndServe(addr, handler); err != nil { | |
| fmt.Fprintf(os.Stderr, "Server error: %v\n", err) | |
| } | |
| return nil, nil | |
| addr := fmt.Sprintf(":%d", opts.Port) | |
| clicky.Infof("Task UI at http://localhost%s", addr) | |
| if err := http.ListenAndServe(addr, handler); err != nil { | |
| return nil, fmt.Errorf("start task UI server on %s: %w", addr, err) | |
| } | |
| return nil, nil |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/uber_demo/main.go` around lines 1135 - 1140, The server startup
error is being swallowed: replace the current http.ListenAndServe error handling
(addr, handler) so that any failure is returned to the caller instead of only
being printed and allowing the function to return nil, nil; specifically, in the
block that builds addr and calls http.ListenAndServe(addr, handler), return a
wrapped or raw error (e.g., fmt.Errorf("listen %s: %w", addr, err) or just err)
from the function so Cobra sees the failure rather than continuing to return
nil, nil.
Comment on lines
+31
to
+36
| snap := TaskSnapshot{ | ||
| ID: t.Name(), | ||
| Name: t.Name(), | ||
| Type: "task", | ||
| Group: groupName, | ||
| Status: string(t.Status()), |
Contributor
There was a problem hiding this comment.
Use stable IDs here instead of display names.
Both task and group snapshots use Name() as ID, but the UI keys state by id. Two groups or tasks with the same name will overwrite each other and share expansion state. Please emit a collision-free identifier here and keep Name as the display label.
Also applies to: 60-63
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@task/snapshot.go` around lines 31 - 36, The snapshot uses t.Name() for
TaskSnapshot.ID which causes UI key collisions; replace the ID field with a
stable, collision-free identifier (e.g., a canonical
Task.ID()/UniqueID()/path-based ID or a deterministic hash derived from task
metadata) while keeping Name as the display label; update the same pattern in
the group snapshot construction (the block around lines 60-63) so both task and
group snapshots use their stable ID property instead of t.Name().
Comment on lines
+7
to
+20
| ``` | ||
| handler.go HTTP handler: serves HTML page + JSON/SSE APIs | ||
| assets.go //go:embed dist/taskui.js | ||
| src/ | ||
| index.tsx Preact entry point, mounts App to #root | ||
| App.tsx Main component: fetches /api/tasks, opens SSE stream | ||
| types.ts TaskSnapshot, LogEntry interfaces | ||
| utils.ts Status → color/icon/background mappings | ||
| components/ | ||
| Summary.tsx Aggregate stats bar with progress visualization | ||
| TaskGroup.tsx Collapsible group with child task list | ||
| TaskRow.tsx Individual task: status icon, name, duration, message | ||
| ProgressBar.tsx Colored segment bar (completed/running/failed/pending) | ||
| ``` |
Contributor
There was a problem hiding this comment.
Add a language to this fenced block.
markdownlint is already flagging this code fence, so this README will keep failing docs lint until it is annotated with a language such as text.
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 7-7: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@task/ui/README.md` around lines 7 - 20, The README's fenced code block
listing filenames lacks a language tag, causing markdownlint failures; update
the triple-backtick fence that wraps the file list (the block containing
handler.go, assets.go, src/index.tsx, App.tsx, types.ts, utils.ts, and
components/*) to include a language such as text (e.g., replace ``` with
```text) so the linter recognizes it as a code block.
Comment on lines
+28
to
+33
| | `/api/tasks/stream` | GET | SSE stream, pushes dirty task updates every 200ms | | ||
|
|
||
| ### SSE Events | ||
|
|
||
| - `data: [TaskSnapshot, ...]` — array of updated task snapshots | ||
| - `event: done` — all tasks have completed |
Contributor
There was a problem hiding this comment.
Document the actual SSE wire format.
The handler emits one event: task per TaskSnapshot, not data: [TaskSnapshot, ...], and it sends the full snapshot set each tick rather than a dirty-only batch. As written, the README describes a different protocol than the code implements.
📝 Suggested doc fix
-| `/api/tasks/stream` | GET | SSE stream, pushes dirty task updates every 200ms |
+| `/api/tasks/stream` | GET | SSE stream, emits one `event: task` per snapshot every 200ms |
-- `data: [TaskSnapshot, ...]` — array of updated task snapshots
+- `event: task` — `data` contains one `TaskSnapshot`
- `event: done` — all tasks have completed📝 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
| | `/api/tasks/stream` | GET | SSE stream, pushes dirty task updates every 200ms | | |
| ### SSE Events | |
| - `data: [TaskSnapshot, ...]` — array of updated task snapshots | |
| - `event: done` — all tasks have completed | |
| | `/api/tasks/stream` | GET | SSE stream, emits one `event: task` per snapshot every 200ms | | |
| ### SSE Events | |
| - `event: task` — `data` contains one `TaskSnapshot` | |
| - `event: done` — all tasks have completed |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@task/ui/README.md` around lines 28 - 33, The README's SSE section is
incorrect: the server actually sends one SSE event per TaskSnapshot with event
name "task" (not a single data array), and each tick sends the full snapshot set
(all tasks) rather than only dirty tasks; update the `/api/tasks/stream`
documentation to describe the wire format precisely by documenting that the
stream emits repeated `event: task` messages with a single TaskSnapshot payload
(or JSON-encoded snapshot) per event, and that it also emits `event: done` when
all tasks complete—ensure examples and text reflect "task" events, full snapshot
set semantics per tick, and the exact JSON payload shape used by the handler.
| const [, forceUpdate] = useState(0); | ||
|
|
||
| useEffect(() => { | ||
| const es = new EventSource('/api/tasks/stream'); |
Contributor
There was a problem hiding this comment.
Use a handler-relative SSE URL.
Hardcoding /api/tasks/stream only works when this UI is mounted at the domain root. If taskui.Handler() is served under a prefix, the page loads but the EventSource bypasses that prefix and 404s.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@task/ui/src/App.tsx` at line 15, The EventSource is using an absolute path
('/api/tasks/stream') which ignores any handler prefix; in App.tsx update the
EventSource instantiation (const es = new EventSource('/api/tasks/stream');) to
use a handler-relative URL (e.g., remove the leading slash or build the URL
against document.baseURI/window.location so the request honors the current mount
prefix) so the SSE connects correctly when the UI is served under a path prefix.
Comment on lines
+70
to
+83
| {!showAll && totalHidden > 0 && ( | ||
| <div | ||
| class="text-xs text-gray-400 py-1.5 cursor-pointer hover:text-gray-600 border-b border-gray-100" | ||
| onClick={() => setShowAll(true)} | ||
| > | ||
| ... {totalHidden} more tasks | ||
| </div> | ||
| )} | ||
| {showAll && totalHidden > 0 && ( | ||
| <div | ||
| class="text-xs text-gray-400 py-1.5 cursor-pointer hover:text-gray-600 border-b border-gray-100" | ||
| onClick={() => setShowAll(false)} | ||
| > | ||
| ▲ collapse |
Contributor
There was a problem hiding this comment.
Use buttons for the expand/collapse controls.
These clickable <div>s are not keyboard-focusable and don't expose button semantics, so keyboard users cannot expand hidden tasks or collapse the list. Please switch them to <button type="button"> or add equivalent role/tabIndex/key handling.
Also applies to: 99-105
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@task/ui/src/components/TaskGroup.tsx` around lines 70 - 83, Replace the
clickable <div>s used for expand/collapse with semantic <button type="button">
elements (preserving the existing onClick handlers that call setShowAll and the
visible text that uses totalHidden and showAll) so they become
keyboard-focusable and expose button semantics; ensure you move the class
attributes to the button (or className if this is JSX), add
aria-expanded={showAll} and an accessible label (or visually-hidden text) as
appropriate, and make the same change for the second occurrence referenced
(lines around the 99-105 block) so both expand and collapse controls are proper
buttons.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
api/table.go (2)
148-154:⚠️ Potential issue | 🟠 MajorCompactHTML still drops cells when
FieldNamesdiffer from headers.
WithoutEmptyColumns()normalizes kept rows underFieldNames[i], so looking uprow[header.String()]here leaves those columns blank. ReusegetCellValue(row, colIdx)or the same field-name fallback before callingHTML().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/table.go` around lines 148 - 154, The CompactHTML rendering loop is using row[header.String()] which can miss cells when FieldNames differ from table.Headers; update the loop in the CompactHTML/HTML builder to obtain the cell via the same lookup used during normalization (call getCellValue(row, colIdx) or apply the FieldNames fallback before invoking HTML()) so that you compute the correct cellValue for the given column index (use the same colIdx used to iterate table.Headers) and then call cellValue.HTML() to produce cellHTML.
267-270:⚠️ Potential issue | 🟠 Major
StaticHTML()is still emitting Alpine-only detail rows.This renderer is documented for PDF/non-JS output, but the detail branch still uses
<template x-if="open">anddetail.HTML(). That keeps detail rows invisible in static output and also drops theStaticHTMLProviderfallback you added elsewhere.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/table.go` around lines 267 - 270, In the StaticHTML() rendering path replace the Alpine-only block that writes `<template x-if="open">` and calls detail.HTML() so static output actually contains the detail row: remove the `<template x-if="open">` wrapper and call the detail's static renderer (e.g. detail.StaticHTML() or the StaticHTMLProvider fallback you added elsewhere) instead of detail.HTML(), and ensure the `<tr><td colspan="%d">...</td></tr>` is emitted directly so detail rows are visible in non-JS/PDF output.formatters/html/react/default_component.jsx (1)
3-15:⚠️ Potential issue | 🟠 MajorThis sanitizer still leaves URL-based XSS vectors open.
It removes
<script>andon*handlers, but it does not scrub dangerous URL-bearing attributes likehref,src,xlink:href, orformaction. Payloads such asjavascript:links will survive and still flow intodangerouslySetInnerHTML. Please switch this to a vetted sanitizer such as DOMPurify instead of maintaining a bespoke blacklist.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@formatters/html/react/default_component.jsx` around lines 3 - 15, The current sanitizeHTML function leaves URL-based XSS vectors (href/src/formaction/xlink:href) unfiltered; replace this bespoke sanitizer by importing and using a vetted library (DOMPurify) in sanitizeHTML/SafeHTML: remove the manual DOMParser stripping, call DOMPurify.sanitize(html) in sanitizeHTML, and ensure DOMPurify is configured to disallow javascript: URIs (or provide a safe ALLOWED_URI_REGEXP) and to strip dangerous attributes; update SafeHTML to feed its dangerouslySetInnerHTML with DOMPurify output and add the DOMPurify import/initialization appropriate for the runtime (browser/server) so all href/src/formaction/xlink:href vectors are neutralized.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/table.go`:
- Around line 159-169: The StaticHTMLProvider branch currently bypasses the
Alpine template wrapper causing rows to be permanently expanded and breaking
script init; update both CompactHTML and htmlWithDetail render paths so the
outer `<template x-if="open">` (or equivalent template wrapper used for
interactive expansion) is always emitted, and only the inner cell content is
swapped: when detail implements StaticHTMLProvider call sp.StaticHTML() for the
inner content, otherwise call detail.HTML(); ensure you replace the direct row
emission in the StaticHTMLProvider branch (references: StaticHTMLProvider type,
CompactHTML, htmlWithDetail, detail.HTML(), sp.StaticHTML(), and the `<template
x-if="open">` wrapper) and apply the same change at the other occurrence around
the 450-453 area.
In `@task/snapshot.go`:
- Around line 66-68: In SnapshotGroup and SnapshotAll, the code currently copies
only the slice header with "items := g.Items" while holding g.mu.RLock(), then
unlocks and iterates—this races with concurrent appends; fix by making a real
copy of the slice contents while under the lock (e.g., allocate a new slice and
copy g.Items into it) so the iteration uses an immutable snapshot after
g.mu.RUnlock(); apply the same change for both functions referencing g.Items and
g.mu.RLock/g.mu.RUnlock.
---
Duplicate comments:
In `@api/table.go`:
- Around line 148-154: The CompactHTML rendering loop is using
row[header.String()] which can miss cells when FieldNames differ from
table.Headers; update the loop in the CompactHTML/HTML builder to obtain the
cell via the same lookup used during normalization (call getCellValue(row,
colIdx) or apply the FieldNames fallback before invoking HTML()) so that you
compute the correct cellValue for the given column index (use the same colIdx
used to iterate table.Headers) and then call cellValue.HTML() to produce
cellHTML.
- Around line 267-270: In the StaticHTML() rendering path replace the
Alpine-only block that writes `<template x-if="open">` and calls detail.HTML()
so static output actually contains the detail row: remove the `<template
x-if="open">` wrapper and call the detail's static renderer (e.g.
detail.StaticHTML() or the StaticHTMLProvider fallback you added elsewhere)
instead of detail.HTML(), and ensure the `<tr><td colspan="%d">...</td></tr>` is
emitted directly so detail rows are visible in non-JS/PDF output.
In `@formatters/html/react/default_component.jsx`:
- Around line 3-15: The current sanitizeHTML function leaves URL-based XSS
vectors (href/src/formaction/xlink:href) unfiltered; replace this bespoke
sanitizer by importing and using a vetted library (DOMPurify) in
sanitizeHTML/SafeHTML: remove the manual DOMParser stripping, call
DOMPurify.sanitize(html) in sanitizeHTML, and ensure DOMPurify is configured to
disallow javascript: URIs (or provide a safe ALLOWED_URI_REGEXP) and to strip
dangerous attributes; update SafeHTML to feed its dangerouslySetInnerHTML with
DOMPurify output and add the DOMPurify import/initialization appropriate for the
runtime (browser/server) so all href/src/formaction/xlink:href vectors are
neutralized.
🪄 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: 2645bb7b-25df-4851-8559-81a8377dd754
📒 Files selected for processing (7)
api/table.goexamples/uber_demo/go.modformatters/html/react/default_component.jsxformatters/html_react_formatter.goformatters/html_react_formatter_test.gogo.modtask/snapshot.go
🚧 Files skipped from review as they are similar to previous changes (3)
- examples/uber_demo/go.mod
- go.mod
- formatters/html_react_formatter.go
Comment on lines
+66
to
+68
| g.mu.RLock() | ||
| items := g.Items | ||
| g.mu.RUnlock() |
Contributor
There was a problem hiding this comment.
Copy g.Items under lock before iterating.
Line 67 and Line 113 only copy the slice header. After unlock, concurrent append/mutation can race with iteration and produce inconsistent snapshots.
🔧 Proposed fix
- g.mu.RLock()
- items := g.Items
- g.mu.RUnlock()
+ g.mu.RLock()
+ items := make([]Taskable, len(g.Items))
+ copy(items, g.Items)
+ g.mu.RUnlock()Apply the same change in both SnapshotGroup and SnapshotAll.
Also applies to: 112-114
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@task/snapshot.go` around lines 66 - 68, In SnapshotGroup and SnapshotAll, the
code currently copies only the slice header with "items := g.Items" while
holding g.mu.RLock(), then unlocks and iterates—this races with concurrent
appends; fix by making a real copy of the slice contents while under the lock
(e.g., allocate a new slice and copy g.Items into it) so the iteration uses an
immutable snapshot after g.mu.RUnlock(); apply the same change for both
functions referencing g.Items and g.mu.RLock/g.mu.RUnlock.
Adds task snapshot APIs (JSON and SSE), a web UI for monitoring task groups in real-time, and supporting infrastructure. The UI streams updates via Server-Sent Events with JSON polling fallback.
Implements a new html-react formatter that converts data structures to interactive React components. Includes a default Preact-based component with support for tables, trees, maps, and lists with sorting, filtering, and HTML sanitization. Supports custom JSX components via FormatOptions.
Add 'mcp install' subcommand to register CLI as MCP server in Claude Code settings. Improve stdio server stability with goroutine-based scanning and context cancellation. Auto-exclude built-in commands from tool registry.
Introduces admin entity operations nested under a shared "admin" parent command. List operations now wrap items with _id field for JSON output. Improves REST path generation for action commands and adds path parameter extraction for templated routes. Hardens server startup detection in tests.
Introduces a new static analyzer that detects bad usage patterns of the clicky text API, including direct struct literals, non-Pretty functions returning api.Text, string concatenation in Content fields, and improper Children initialization. Also simplifies command usage documentation.
Add Node.js setup and frontend build steps to CI/CD workflows. Upgrade Go version from 1.25 to 1.26 and golangci-lint from v2.4.0 to v2.11.0. Add new OpenAPI integration test job.
…ml provider Replace fmt.Sprintf with fmt.Fprintf for direct string builder writes, improving performance. Add StaticHTMLProvider interface to avoid embedding scripts in template tags. Use template x-if instead of x-show for collapsed content to prevent unnecessary DOM bloat. Add Code.Trim() method and column max-width support.
The *.json and *.yaml gitignore rules were dropping package.json, package-lock.json, tsconfig.json, and .golangci.yaml on rebase. Add negation rules after the wildcard rules so these files persist.
|
🎉 This PR is included in version 1.21.2 |
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.
Description
Brief description of the changes in this PR.
Type of Change
Testing
Checklist
Breaking Changes
If this is a breaking change, please describe the impact and migration path for existing users:
Additional Notes
Add any additional notes, screenshots, or context about the changes here.
Summary by CodeRabbit
New Features
Improvements
Developer Tools