Add form field input on task creation (if present). #127
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (2)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughCreateGrantTask gains an optional requestData parameter and a new UpdateTaskRequestData method is added to update task request payloads. A new pkg/logging package provides a singleton Zap-based logger with levels, idempotent Init, Get(), and convenience helpers. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Comment |
- Added UpdateTaskRequestData client method to update task after collection
- Change --form-data flag from comma-separated to JSON format
Properly handles values containing commas or special characters
- Improve required field validation using IgnoreEmpty and MinLen rules
- Fix Int64FieldValidator to be consistent with StringFieldValidator
Default values now passed as initial values, not returned from IsValid
- Add context cancellation checks to collectBoolField/collectStringSliceField
- Add logging package (pkg/logging) with zap and --log-level flag
Supports debug, info, warn, error levels; silent by default
…enter 1 value at a time for form fields which take a list
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
pkg/client/task.go (1)
23-47:requestDatapropagation looks correct; consider normalizing empty maps at the API boundary.You already guard
nil/empty for create; that’s good. For consistency withUpdateTaskRequestData, consider documenting whether empty means “no change” vs “clear”.pkg/logging/logging.go (1)
43-78: Zap config: consider disabling color levels when stderr isn’t a TTY (CI/log aggregation).
zapcore.CapitalColorLevelEncodercan pollute non-interactive logs; a non-color encoder (or env/TTY check) is often friendlier for downstream parsing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
cmd/cone/flags.gois excluded by none and included by nonecmd/cone/form_fields.gois excluded by none and included by nonecmd/cone/get_drop_task.gois excluded by none and included by nonecmd/cone/main.gois excluded by none and included by none
📒 Files selected for processing (3)
pkg/client/client.go(2 hunks)pkg/client/task.go(3 hunks)pkg/logging/logging.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/client/task.go (1)
pkg/client/error.go (1)
NewHTTPError(26-47)
🪛 GitHub Check: go-lint
pkg/logging/logging.go
[failure] 115-115:
Comment should end in a period (godot)
[failure] 110-110:
Comment should end in a period (godot)
[failure] 105-105:
Comment should end in a period (godot)
[failure] 100-100:
Comment should end in a period (godot)
[failure] 95-95:
Comment should end in a period (godot)
[failure] 90-90:
Comment should end in a period (godot)
[failure] 85-85:
Comment should end in a period (godot)
[failure] 80-80:
Comment should end in a period (godot)
[failure] 15-15:
Comment should end in a period (godot)
🔇 Additional comments (2)
pkg/client/client.go (2)
94-95: The newUpdateTaskRequestDatainterface method is properly implemented in the concrete client and does not break any existing code.This codebase has no mocks or wrapper implementations of C1Client. The only implementation of the C1Client interface is the concrete
clientstruct in pkg/client/client.go, which includes the complete implementation of UpdateTaskRequestData in pkg/client/task.go. No test doubles, mocks, or wrapper types exist that would need updating.Likely an incorrect or invalid review comment.
70-80: Interface signature updated consistently:CreateGrantTasknow acceptsrequestDataparameter.The
C1Clientinterface (line 70-80) has been updated to include therequestData map[string]anyparameter, and the implementation inpkg/client/task.go(line 23-33) matches this signature. No external call sites or separate implementations of this interface exist in the codebase that require updating.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pkg/logging/logging.go (1)
10-41: Data race: unsynchronized read ofloggerinGet().The
sync.Onceprotects the write insideonce.Do, butGet()readsloggerwithout synchronization. IfInit()andGet()are called concurrently from different goroutines, this is a data race under the Go memory model.Additionally, line 38 allocates a new no-op logger on every call to
Get()before initialization, which is wasteful.Use
atomic.Valueorsync.RWMutexfor thread-safe access:import ( - "sync" + "sync/atomic" "go.uber.org/zap" "go.uber.org/zap/zapcore" ) var ( - logger *zap.SugaredLogger - once sync.Once + logger atomic.Pointer[zap.SugaredLogger] + once sync.Once + nop = zap.NewNop().Sugar() )func Init(level Level) { once.Do(func() { - logger = newLogger(level) + logger.Store(newLogger(level)) }) } func Get() *zap.SugaredLogger { - if logger == nil { - // Return a no-op logger if not initialized - return zap.NewNop().Sugar() - } - return logger + if l := logger.Load(); l != nil { + return l + } + return nop }
🧹 Nitpick comments (2)
pkg/client/task.go (2)
45-47: Simplify: omit redundant nil check.The
nilcheck is unnecessary sincelen(nil)returns0for maps in Go. The static analyzer flagged this (S1009).- if requestData != nil && len(requestData) > 0 { + if len(requestData) > 0 { req.RequestData = requestData }
166-183: Simplify nil check and consider early return for empty data.Two observations:
The
nilcheck at line 168 is redundant (S1009) sincelen(nil)returns0.Unlike
CreateGrantTaskwhererequestDatais optional, callingUpdateTaskRequestDatawith empty data still makes an API call with an emptyDatafield. Consider returning early to avoid unnecessary network calls.func (c *client) UpdateTaskRequestData(ctx context.Context, taskID string, requestData map[string]any) (*shared.TaskServiceActionResponse, error) { + if len(requestData) == 0 { + return nil, fmt.Errorf("requestData must be non-empty") + } req := shared.TaskActionsServiceUpdateRequestDataRequest{} - if requestData != nil && len(requestData) > 0 { - req.Data = requestData - } + req.Data = requestData resp, err := c.sdk.TaskActions.UpdateRequestData(ctx, operations.C1APITaskV1TaskActionsServiceUpdateRequestDataRequest{ - TaskActionsServiceUpdateRequestDataRequest: &req, - TaskID: taskID, + TaskActionsServiceUpdateRequestDataRequest: &req, + TaskID: taskID, })Note: Add
"fmt"to imports if adopting the error return.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/client/task.go(3 hunks)pkg/logging/logging.go(1 hunks)
🧰 Additional context used
🪛 GitHub Check: go-lint
pkg/client/task.go
[failure] 45-45:
S1009: should omit nil check; len() for nil maps is defined as zero (gosimple)
[failure] 168-168:
S1009: should omit nil check; len() for nil maps is defined as zero (gosimple)
[failure] 173-173:
File is not properly formatted (goimports)
🔇 Additional comments (2)
pkg/logging/logging.go (2)
43-78: LGTM!The
newLoggerfunction correctly maps log levels, configures console encoding with ISO8601 timestamps, and falls back gracefully to a no-op logger on build failure.
80-118: LGTM!The convenience helper functions correctly delegate to
Get(), ensuring consistent logger access throughout the codebase.
Release Notes
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.