Open
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:
WalkthroughReplaced string casts for PluginID in portal responses; added per-transaction Progress computation; introduced per-policy Progress types and responses; added plugin endpoints and service methods to fetch policy progress (single and batch); wired progress into plugin policy service; added tx-indexer CountByPolicyID; updated CI Go version and added formatting check. Changes
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
4others
approved these changes
Feb 17, 2026
Collaborator
|
@evgensheff How we return progress from API? How UI can determine the format, is it 0/100% or just counter e.g 333 |
Co-authored-by: 4others <44651681+4others@users.noreply.github.com>
Collaborator
Author
from 0 to 100 % |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/portal/server.go (1)
729-731:⚠️ Potential issue | 🟡 MinorInconsistent:
string()cast remains here but was removed elsewhere.Lines 357, 425, 588, and 810 all removed the
string()cast onPluginID, but line 731 still hasstring(updated.PluginID). This should be updated for consistency.Proposed fix
return c.JSON(http.StatusOK, PluginApiKeyResponse{ ID: updated.ID.String(), - PluginID: string(updated.PluginID), + PluginID: updated.PluginID, ApiKey: maskApiKey(updated.Apikey),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/portal/server.go` around lines 729 - 731, The response still applies an explicit string() cast to updated.PluginID in the PluginApiKeyResponse construction; remove the string() cast so it matches the other occurrences (e.g., where PluginID is assigned without casting) and set PluginID: updated.PluginID in the return payload to maintain consistency with the other edits that removed the cast.
🧹 Nitpick comments (1)
internal/types/transaction.go (1)
65-89: Progress semantics are undocumented and the API contract is unclear for the frontend.The reviewer (webpiratt) already asked how the UI should interpret progress — is it a percentage (0–100), a step counter, or something else? The hardcoded values (20, 40, 60, 80, 100) are magic numbers without documentation. A brief doc comment on
getProgressexplaining the scale and meaning of each value would help both the frontend team and future maintainers.Suggested doc comment
+// getProgress returns a progress percentage (0-100) representing how far +// a transaction has advanced through the pipeline: +// 0 = unknown/uninitialized +// 20 = proposed +// 40 = verified +// 60 = signed (awaiting broadcast or on-chain confirmation) +// 80 = broadcasted, pending on-chain +// 100 = terminal (confirmed on-chain, success or fail) func getProgress(status storage.TxStatus, statusOnchain *rpc.TxOnChainStatus, broadcastedAt *time.Time) int {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/types/transaction.go` around lines 65 - 89, getProgress currently returns magic numbers (20,40,60,80,100) with unclear semantics; add a concise doc comment above getProgress describing that it returns a percentage (0–100) representing overall transaction progress for the UI, and enumerate the mapping used: TxProposed -> 20, TxVerified -> 40, TxSigned -> 60/80/100 depending on rpc.TxOnChainStatus and broadcastedAt, and default -> 0; mention the meaning of broadcastedAt and how rpc.TxOnChainStatus values (TxOnChainSuccess/Fail -> 100, TxOnChainPending + broadcastedAt -> 80, pending without broadcast -> 60) influence the percentage so frontend consumers and future maintainers understand the contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/types/transaction.go`:
- Around line 75-77: The switch currently returns 100 for both
rpc.TxOnChainSuccess and rpc.TxOnChainFail which makes failed transactions
appear as fully complete; update the branch handling of statusOnchain so that
rpc.TxOnChainSuccess returns 100 but rpc.TxOnChainFail returns a distinct value
(e.g., 0 or -1) or otherwise signals failure (or alternatively add an explicit
comment explaining why both map to 100 if that was intentional). Locate the
switch on the statusOnchain variable in internal/types/transaction.go and modify
the rpc.TxOnChainFail case (or add a separate status field) so the UI can
distinguish failure from true success.
---
Outside diff comments:
In `@internal/portal/server.go`:
- Around line 729-731: The response still applies an explicit string() cast to
updated.PluginID in the PluginApiKeyResponse construction; remove the string()
cast so it matches the other occurrences (e.g., where PluginID is assigned
without casting) and set PluginID: updated.PluginID in the return payload to
maintain consistency with the other edits that removed the cast.
---
Nitpick comments:
In `@internal/types/transaction.go`:
- Around line 65-89: getProgress currently returns magic numbers
(20,40,60,80,100) with unclear semantics; add a concise doc comment above
getProgress describing that it returns a percentage (0–100) representing overall
transaction progress for the UI, and enumerate the mapping used: TxProposed ->
20, TxVerified -> 40, TxSigned -> 60/80/100 depending on rpc.TxOnChainStatus and
broadcastedAt, and default -> 0; mention the meaning of broadcastedAt and how
rpc.TxOnChainStatus values (TxOnChainSuccess/Fail -> 100, TxOnChainPending +
broadcastedAt -> 80, pending without broadcast -> 60) influence the percentage
so frontend consumers and future maintainers understand the contract.
Collaborator
|
@webpiratt Could you please confirm whether we also need a progress indicator at the policy level? If yes, please ping me once it’s ready so I can update the frontend accordingly. cc: @evgensheff |
Collaborator
|
Discussing in DM |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
internal/portal/server.go (1)
749-756: Inconsistent:string()conversion not removed here.Line 751 still has
string(updated.PluginID)while all other equivalent sites in this PR had theirstring()wrappers removed. This should be updated for consistency.♻️ Proposed fix
return c.JSON(http.StatusOK, PluginApiKeyResponse{ ID: updated.ID.String(), - PluginID: string(updated.PluginID), + PluginID: updated.PluginID, ApiKey: maskApiKey(updated.Apikey), CreatedAt: updated.CreatedAt.Time.Format(time.RFC3339), ExpiresAt: expiresAt, Status: updated.Status, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/portal/server.go` around lines 749 - 756, The return value constructs a PluginApiKeyResponse but still wraps updated.PluginID with string(...) inconsistent with the rest of the PR; update the construction to use the PluginID value directly (remove the string(...) conversion) in the response creation that references PluginApiKeyResponse and updated.PluginID so it matches other sites where the string wrapper was removed.internal/types/transaction.go (1)
59-62: Use keyed fields inProgressliteral.The positional literal
Progress{ProgressPercent, getProgress(...)}will silently assign wrong values if theProgressstruct fields are reordered or a new field is inserted. Use keyed fields for resilience.♻️ Proposed fix
- Progress: Progress{ - ProgressPercent, - getProgress(tx.Status, tx.StatusOnChain, tx.BroadcastedAt), - }, + Progress: Progress{ + Kind: ProgressPercent, + Value: getProgress(tx.Status, tx.StatusOnChain, tx.BroadcastedAt), + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/types/transaction.go` around lines 59 - 62, Replace the positional composite literal for Progress with a keyed-field literal to avoid brittle assignments: in the code that constructs Progress (the Progress{ProgressPercent, getProgress(tx.Status, tx.StatusOnChain, tx.BroadcastedAt)} expression), use the actual field names from the Progress struct (e.g., Percent: and whatever the second field is) and assign ProgressPercent and the getProgress(...) result to those keys respectively so the fields remain correct if Progress is reordered or extended.internal/types/plugin.go (1)
11-14: Consider using a typed constant for progress kind.The
Kindfield onProgressis a plainstring, and these constants are untyped. A typed alias (e.g.,type ProgressKind string) would prevent accidentally assigning arbitrary strings and improve discoverability.♻️ Proposed refactor
+type ProgressKind string + const ( - ProgressPercent = "percent" - ProgressCounter = "counter" + ProgressPercent ProgressKind = "percent" + ProgressCounter ProgressKind = "counter" )Then update
Progress.Kindto useProgressKind:type Progress struct { - Kind string `json:"kind"` + Kind ProgressKind `json:"kind"` Value uint32 `json:"value"` }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/types/plugin.go` around lines 11 - 14, Introduce a new distinct type alias (e.g., ProgressKind) and make the existing constants typed (e.g., const ProgressPercent ProgressKind = "percent", ProgressCounter ProgressKind = "counter"), then update the Progress struct’s Kind field to use ProgressKind instead of string (reference: ProgressKind, ProgressPercent, ProgressCounter, Progress.Kind) so callers are constrained to the defined values and accidental arbitrary strings are prevented.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/portal/server.go`:
- Around line 749-756: The return value constructs a PluginApiKeyResponse but
still wraps updated.PluginID with string(...) inconsistent with the rest of the
PR; update the construction to use the PluginID value directly (remove the
string(...) conversion) in the response creation that references
PluginApiKeyResponse and updated.PluginID so it matches other sites where the
string wrapper was removed.
In `@internal/types/plugin.go`:
- Around line 11-14: Introduce a new distinct type alias (e.g., ProgressKind)
and make the existing constants typed (e.g., const ProgressPercent ProgressKind
= "percent", ProgressCounter ProgressKind = "counter"), then update the Progress
struct’s Kind field to use ProgressKind instead of string (reference:
ProgressKind, ProgressPercent, ProgressCounter, Progress.Kind) so callers are
constrained to the defined values and accidental arbitrary strings are
prevented.
In `@internal/types/transaction.go`:
- Around line 59-62: Replace the positional composite literal for Progress with
a keyed-field literal to avoid brittle assignments: in the code that constructs
Progress (the Progress{ProgressPercent, getProgress(tx.Status, tx.StatusOnChain,
tx.BroadcastedAt)} expression), use the actual field names from the Progress
struct (e.g., Percent: and whatever the second field is) and assign
ProgressPercent and the getProgress(...) result to those keys respectively so
the fields remain correct if Progress is reordered or extended.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
plugin/server/server.go (1)
605-623: Consider validatingPolicyIDslength in the batch endpoint.An empty or excessively large
policy_idsarray is accepted without bounds checking. A missing/empty array silently hitsGetProgressBatchwith no IDs, and an unbounded array could cause performance issues downstream.Proposed validation
func (s *Server) handleGetPluginPoliciesProgress(c echo.Context) error { var req struct { PolicyIDs []uuid.UUID `json:"policy_ids"` } if err := c.Bind(&req); err != nil { return c.JSON(http.StatusBadRequest, NewErrorResponse("invalid request")) } + if len(req.PolicyIDs) == 0 { + return c.JSON(http.StatusBadRequest, NewErrorResponse("policy_ids must not be empty")) + } + if len(req.PolicyIDs) > 100 { + return c.JSON(http.StatusBadRequest, NewErrorResponse("policy_ids exceeds maximum of 100")) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/server/server.go` around lines 605 - 623, The handler handleGetPluginPoliciesProgress accepts an unvalidated batch of req.PolicyIDs and may call s.policy.GetProgressBatch with an empty or huge slice; add validation before calling GetProgressBatch to (1) return HTTP 400 if req.PolicyIDs is nil or length == 0 with a clear error message, and (2) enforce an upper bound (e.g., maxBatch = 100) and return HTTP 400 if len(req.PolicyIDs) > maxBatch; perform these checks on req.PolicyIDs in handleGetPluginPoliciesProgress and only call s.policy.GetProgressBatch when the slice length is within bounds.internal/api/policy.go (2)
398-426: Goroutines never return an error —eg.Wait()error check is dead code.Every goroutine unconditionally returns
nil(line 420), soeg.Wait()on line 423 will never return a non-nil error, making lines 423-426 unreachable. This is misleading to future readers. Either remove the dead error check, or have the goroutines propagate errors fromCountByPolicyIDif that failure should be surfaced.Given the intent is graceful degradation (consistent with the single-policy handler), the always-return-nil approach is fine — just drop the dead branch or add a comment clarifying it's a safeguard.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/policy.go` around lines 398 - 426, The goroutines launched in the errgroup (eg.Go) always return nil, so the eg.Wait() error check is dead code; update the block to either propagate CountByPolicyID errors from the goroutines (return the error from the closure so eg.Wait() can surface it) or—preferably for graceful degradation—remove the eg.Wait() error branch and add a short clarifying comment near the errgroup/eg.Wait usage (referencing eg, eg.Go, s.txIndexerService.CountByPolicyID, responses, and itypes.PluginPolicyResponse/itypes.Progress) to explain that failures are logged per-iteration and are intentionally not fatal.
398-421: Consider batchingCountByPolicyIDcalls to reduce N+1 queries for policies without plugin progress.When
progressMapis nil or sparse, each policy without progress triggers a separateCountByPolicyIDDB call inside a goroutine, resulting in up to 100 concurrent queries. Currently, no batch variant of this method exists in the tx-indexer service. If aCountByPolicyIDsbatch query were added, it could consolidate these calls. Given the page size cap of 100 policies, individual concurrent queries are acceptable, but batching would be a worthwhile optimization if the indexer supports it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/policy.go` around lines 398 - 421, The loop over policies (policies.Policies) makes a separate txIndexerService.CountByPolicyID call per policy when progressMap lacks entries, causing N+1 DB queries; add a batch path: detect which policy IDs have no progress, call a new txIndexerService.CountByPolicyIDs(ctx, []ids) to get counts for all of them at once, then populate prog for those policies before or inside the errgroup goroutines so responses[i] uses the pre-fetched counts; update the txIndexerService interface and invocation site (where CountByPolicyID is currently used) to prefer CountByPolicyIDs when available and fall back to CountByPolicyID for backwards compatibility.internal/service/plugin.go (2)
441-480: Inconsistent HTTP call patterns between the two new methods.
GetPolicyProgressuses a manualhttp.Client+json.NewDecoder, whileGetPoliciesProgressuses the sharedlibhttp.Callhelper. Both talk to the same plugin server. Usinglibhttp.Callconsistently would reduce duplication and ensure uniform timeout, error handling, and response reading behavior.Proposed refactor for GetPolicyProgress
func (s *PluginService) GetPolicyProgress(ctx context.Context, pluginID string, policyID uuid.UUID) (*types.Progress, error) { plugin, err := s.db.FindPluginById(ctx, nil, ptypes.PluginID(pluginID)) if err != nil { return nil, fmt.Errorf("failed to find plugin: %w", err) } keyInfo, err := s.db.GetAPIKeyByPluginId(ctx, pluginID) if err != nil || keyInfo == nil { return nil, fmt.Errorf("failed to find plugin server info: %w", err) } url := fmt.Sprintf("%s/plugin/policy/%s/progress", strings.TrimSuffix(plugin.ServerEndpoint, "/"), policyID) - req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) - if err != nil { - return nil, fmt.Errorf("failed to create request: %w", err) - } - req.Header.Set("Authorization", "Bearer "+keyInfo.ApiKey) - - client := &http.Client{Timeout: 10 * time.Second} - resp, err := client.Do(req) - if err != nil { - return nil, fmt.Errorf("failed to call plugin endpoint: %w", err) - } - defer resp.Body.Close() - - if resp.StatusCode == http.StatusNotImplemented || resp.StatusCode == http.StatusNotFound { - return nil, nil - } - if resp.StatusCode != http.StatusOK { - return nil, fmt.Errorf("plugin endpoint returned status %d", resp.StatusCode) - } - - var prog types.Progress - err = json.NewDecoder(resp.Body).Decode(&prog) - if err != nil { - return nil, fmt.Errorf("failed to decode response: %w", err) - } - - return &prog, nil + result, err := libhttp.Call[types.Progress]( + ctx, + http.MethodGet, + url, + map[string]string{ + "Authorization": "Bearer " + keyInfo.ApiKey, + }, + nil, + nil, + ) + if err != nil { + var httpErr *libhttp.HTTPError + if errors.As(err, &httpErr) && (httpErr.StatusCode == http.StatusNotImplemented || httpErr.StatusCode == http.StatusNotFound) { + return nil, nil + } + return nil, fmt.Errorf("failed to get policy progress: %w", err) + } + + return &result, nil }Also applies to: 482-516
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/plugin.go` around lines 441 - 480, GetPolicyProgress duplicates the manual HTTP request/timeout/decoding logic used elsewhere; replace the manual client/decoder with the shared libhttp.Call helper (same pattern used by GetPoliciesProgress) to ensure consistent timeout, error handling and response reading. Specifically, build the same URL and Authorization header as now, call libhttp.Call(ctx, http.MethodGet, url, headers, nil), handle the returned response status: return nil for 404/501, error for non-200 statuses, and use the response body returned by libhttp.Call to json.Unmarshal into types.Progress (or decode consistently with how GetPoliciesProgress handles the body). Update GetPolicyProgress to mirror the call/response/error handling used in GetPoliciesProgress.
447-449: Misleading error message whenkeyInfois nil buterris also nil.When
GetAPIKeyByPluginIdreturns(nil, nil), the error wrappingfmt.Errorf("failed to find plugin server info: %w", err)produces"failed to find plugin server info: <nil>". This is confusing for debugging.Proposed fix
keyInfo, err := s.db.GetAPIKeyByPluginId(ctx, pluginID) - if err != nil || keyInfo == nil { - return nil, fmt.Errorf("failed to find plugin server info: %w", err) + if err != nil { + return nil, fmt.Errorf("failed to find plugin server info: %w", err) + } + if keyInfo == nil { + return nil, fmt.Errorf("no API key found for plugin %s", pluginID) }Also applies to: 487-490
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/plugin.go` around lines 447 - 449, The current check `if err != nil || keyInfo == nil` in the GetAPIKeyByPluginId call returns a wrapped error that can be "failed to find plugin server info: <nil>" when err is nil but keyInfo is nil; change the logic to return a clear, non-nil error when keyInfo == nil (separate the conditions), e.g., if err != nil return that error, else if keyInfo == nil return a new descriptive error like "failed to find plugin server info: no API key found for plugin id <id>"; apply the same fix to the other occurrence that mirrors lines 487-490 so both places return meaningful errors instead of wrapping a nil err.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugin/policy/service.go`:
- Line 9: The import of github.com/vultisig/verifier/plugin/progress and all
uses of progress.Service and progress.Progress are invalid because that package
doesn't exist; fix by either importing the correct existing package that
provides those types or by replacing them with locally defined equivalents:
remove the bad import line, add a local interface/type (e.g., type
ProgressService interface { Update(...); Get(...)} and type Progress struct { /*
fields used in service.go */ }) or reuse an existing module's types, then update
references in NewService, the Service struct, and any methods that accept/return
progress.Service or progress.Progress to use the new local names (or the correct
package path) so the symbols resolve and the file compiles.
- Around line 27-28: Create a new package "plugin/progress" exposing a Progress
struct and a Service interface so references in policy/service.go compile:
define type Progress struct { Kind string; Value uint32 } (matching
internal/types.Progress) and a Service interface with methods GetProgress(ctx
context.Context, policyID uuid.UUID) (*Progress, error) and GetProgressBatch(ctx
context.Context, policyIDs []uuid.UUID) (map[uuid.UUID]*Progress, error); place
the package at the import path used by policy/service.go and ensure the symbol
names Progress and Service exactly match the references in that file (and
elsewhere e.g., where progress.Service is injected).
---
Nitpick comments:
In `@internal/api/policy.go`:
- Around line 398-426: The goroutines launched in the errgroup (eg.Go) always
return nil, so the eg.Wait() error check is dead code; update the block to
either propagate CountByPolicyID errors from the goroutines (return the error
from the closure so eg.Wait() can surface it) or—preferably for graceful
degradation—remove the eg.Wait() error branch and add a short clarifying comment
near the errgroup/eg.Wait usage (referencing eg, eg.Go,
s.txIndexerService.CountByPolicyID, responses, and
itypes.PluginPolicyResponse/itypes.Progress) to explain that failures are logged
per-iteration and are intentionally not fatal.
- Around line 398-421: The loop over policies (policies.Policies) makes a
separate txIndexerService.CountByPolicyID call per policy when progressMap lacks
entries, causing N+1 DB queries; add a batch path: detect which policy IDs have
no progress, call a new txIndexerService.CountByPolicyIDs(ctx, []ids) to get
counts for all of them at once, then populate prog for those policies before or
inside the errgroup goroutines so responses[i] uses the pre-fetched counts;
update the txIndexerService interface and invocation site (where CountByPolicyID
is currently used) to prefer CountByPolicyIDs when available and fall back to
CountByPolicyID for backwards compatibility.
In `@internal/service/plugin.go`:
- Around line 441-480: GetPolicyProgress duplicates the manual HTTP
request/timeout/decoding logic used elsewhere; replace the manual client/decoder
with the shared libhttp.Call helper (same pattern used by GetPoliciesProgress)
to ensure consistent timeout, error handling and response reading. Specifically,
build the same URL and Authorization header as now, call libhttp.Call(ctx,
http.MethodGet, url, headers, nil), handle the returned response status: return
nil for 404/501, error for non-200 statuses, and use the response body returned
by libhttp.Call to json.Unmarshal into types.Progress (or decode consistently
with how GetPoliciesProgress handles the body). Update GetPolicyProgress to
mirror the call/response/error handling used in GetPoliciesProgress.
- Around line 447-449: The current check `if err != nil || keyInfo == nil` in
the GetAPIKeyByPluginId call returns a wrapped error that can be "failed to find
plugin server info: <nil>" when err is nil but keyInfo is nil; change the logic
to return a clear, non-nil error when keyInfo == nil (separate the conditions),
e.g., if err != nil return that error, else if keyInfo == nil return a new
descriptive error like "failed to find plugin server info: no API key found for
plugin id <id>"; apply the same fix to the other occurrence that mirrors lines
487-490 so both places return meaningful errors instead of wrapping a nil err.
In `@plugin/server/server.go`:
- Around line 605-623: The handler handleGetPluginPoliciesProgress accepts an
unvalidated batch of req.PolicyIDs and may call s.policy.GetProgressBatch with
an empty or huge slice; add validation before calling GetProgressBatch to (1)
return HTTP 400 if req.PolicyIDs is nil or length == 0 with a clear error
message, and (2) enforce an upper bound (e.g., maxBatch = 100) and return HTTP
400 if len(req.PolicyIDs) > maxBatch; perform these checks on req.PolicyIDs in
handleGetPluginPoliciesProgress and only call s.policy.GetProgressBatch when the
slice length is within bounds.
webpiratt
approved these changes
Feb 20, 2026
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
plugin/progress/service_nil.go (1)
10-10: Add a compile-time interface satisfaction check.Without
var _ Service = (*NilService)(nil), a future method added toServicewill produce a compile error only at the injection call site rather than here, obscuring the root cause.♻️ Proposed addition (after the struct declaration)
type NilService struct{} + +var _ Service = (*NilService)(nil)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/progress/service_nil.go` at line 10, Add a compile-time interface satisfaction assertion for NilService so the compiler fails at the type declaration when Service gains new methods; specifically, after the NilService struct declaration add a var _ Service = (*NilService)(nil) line to ensure NilService implements Service and surface missing method errors at the declaration site.plugin/progress/service.go (2)
14-17: Document the nil-on-success contract and reconsider pointer map values.Two related points:
GetProgressreturns*Progress, so(nil, nil)is a valid, non-error result (asNilServicedemonstrates). Callers must nil-check the pointer even whenerr == nil. This implicit contract should be documented on the interface.
GetProgressBatchreturnsmap[uuid.UUID]*Progress. A caller cannot distinguish a missing key from a key present with anilvalue. Using non-pointer values (map[uuid.UUID]Progress) eliminates one nil-dereference opportunity; absent entries would simply not appear in the map.♻️ Proposed refactor
type Service interface { + // GetProgress returns the current progress for policyID. + // Returns (nil, nil) when progress is not available (e.g. not yet tracked). GetProgress(ctx context.Context, policyID uuid.UUID) (*Progress, error) - GetProgressBatch(ctx context.Context, policyIDs []uuid.UUID) (map[uuid.UUID]*Progress, error) + // GetProgressBatch returns progress keyed by policyID for each ID in policyIDs. + // IDs with no available progress are omitted from the returned map. + GetProgressBatch(ctx context.Context, policyIDs []uuid.UUID) (map[uuid.UUID]Progress, error) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/progress/service.go` around lines 14 - 17, The Service interface's nil-on-success behavior isn't documented and the GetProgressBatch signature uses pointer values which makes missing-vs-explicit-nil ambiguous; update the Service interface comments to state that GetProgress returns (nil, nil) when no progress exists (document the nil-on-success contract for GetProgress and NilService), and change GetProgressBatch to return map[uuid.UUID]Progress (non-pointer) so absent keys mean missing entries and callers avoid nil derefs; update all implementations (including NilService) and callers of GetProgressBatch and any code that relied on pointer semantics to adapt to non-pointer Progress values.
9-12:Kindas a barestringleaves the API contract underspecified.Without a defined type and constants (e.g.,
type Kind string; const KindPercentage Kind = "percentage"), every implementor invents its own string values and callers have no compile-time safety or discoverability. GivenKindis the discriminator the UI depends on to choose its representation, this should be locked down now before multiple implementations diverge.♻️ Proposed refactor
+// Kind identifies the representation of a Progress value. +type Kind string + +const ( + KindPercentage Kind = "percentage" // 0–100 + KindCount Kind = "count" // raw transaction count +) + type Progress struct { - Kind string `json:"kind"` + Kind Kind `json:"kind"` Value uint32 `json:"value"` }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/progress/service.go` around lines 9 - 12, The Progress struct's Kind field is underspecified as a plain string; introduce a strong type (e.g., type ProgressKind string) and declare a small set of exported constants (e.g., ProgressKindPercentage, ProgressKindFraction, ProgressKindIndeterminate) that represent the allowed discriminators, then change Progress.Kind's type from string to ProgressKind and keep the json:"kind" tag; update any usages of Progress.Kind and any switch/validation logic to use the new ProgressKind constants so callers get compile-time safety and discoverability.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@plugin/progress/service_nil.go`:
- Line 10: Add a compile-time interface satisfaction assertion for NilService so
the compiler fails at the type declaration when Service gains new methods;
specifically, after the NilService struct declaration add a var _ Service =
(*NilService)(nil) line to ensure NilService implements Service and surface
missing method errors at the declaration site.
In `@plugin/progress/service.go`:
- Around line 14-17: The Service interface's nil-on-success behavior isn't
documented and the GetProgressBatch signature uses pointer values which makes
missing-vs-explicit-nil ambiguous; update the Service interface comments to
state that GetProgress returns (nil, nil) when no progress exists (document the
nil-on-success contract for GetProgress and NilService), and change
GetProgressBatch to return map[uuid.UUID]Progress (non-pointer) so absent keys
mean missing entries and callers avoid nil derefs; update all implementations
(including NilService) and callers of GetProgressBatch and any code that relied
on pointer semantics to adapt to non-pointer Progress values.
- Around line 9-12: The Progress struct's Kind field is underspecified as a
plain string; introduce a strong type (e.g., type ProgressKind string) and
declare a small set of exported constants (e.g., ProgressKindPercentage,
ProgressKindFraction, ProgressKindIndeterminate) that represent the allowed
discriminators, then change Progress.Kind's type from string to ProgressKind and
keep the json:"kind" tag; update any usages of Progress.Kind and any
switch/validation logic to use the new ProgressKind constants so callers get
compile-time safety and discoverability.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/deploy.yaml (1)
26-32: Avoid redundantgofmttraversal.The
gofmt -s -l .command runs twice (once inside$()for the wc check, then again for output). Capture the output once to avoid unnecessary directory traversal:♻️ Proposed refactor
- name: Check formatting run: | - if [ "$(gofmt -s -l . | wc -l)" -gt 0 ]; then - echo "Code is not formatted properly:" - gofmt -s -l . - exit 1 - fi + UNFORMATTED=$(gofmt -s -l .) + if [ -n "$UNFORMATTED" ]; then + echo "Code is not formatted properly:" + echo "$UNFORMATTED" + exit 1 + fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/deploy.yaml around lines 26 - 32, The Check formatting step currently runs gofmt -s -l . twice; change the shell snippet in the "Check formatting" job to capture the output of gofmt -s -l . into a variable (e.g., files=$(gofmt -s -l .)), then test the variable (if [ -n "$files" ] ...) and print $files before exiting; update the commands referenced in this step so the gofmt traversal only runs once and the same captured output is reused for both the existence check and the printed list.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/deploy.yaml:
- Around line 26-32: The Check formatting step currently runs gofmt -s -l .
twice; change the shell snippet in the "Check formatting" job to capture the
output of gofmt -s -l . into a variable (e.g., files=$(gofmt -s -l .)), then
test the variable (if [ -n "$files" ] ...) and print $files before exiting;
update the commands referenced in this step so the gofmt traversal only runs
once and the same captured output is reused for both the existence check and the
printed list.
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
4 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.
Closes vultisig/recipes#521
Summary by CodeRabbit
New Features
Refactor
Chores