test(api): inject resolveAccount into buildReshapeHandler (closes #192)#862
test(api): inject resolveAccount into buildReshapeHandler (closes #192)#862cristim wants to merge 1 commit into
Conversation
cristim
added
triaged
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 29 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai full review Generated by Claude Code |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Fetch the PR diff
gh pr view 862 --repo LeanerCloud/CUDly --json title,body,filesLength of output: 1178 🏁 Script executed: #!/bin/bash
# Get the diff of the PR
gh pr diff 862 --repo LeanerCloud/CUDlyLength of output: 4595 PR Review:
|
| What | Before | After |
|---|---|---|
buildReshapeHandler signature |
3 params | 4th param resolveAccount func(context.Context) (string, error) |
| Nil safety | N/A | nil → unscoped ("", nil) resolver |
| Unscoped callers (×3) | relied on the hardcoded default inside the helper | pass nil explicitly |
| Scoped test | post-construction field override h.reshapeAccountResolver = … |
inline constructor argument |
Detailed observations
buildReshapeHandler signature & nil-guard (Lines 64–70)
The nil-guard idiom is idiomatic Go for optional function parameters. The defaulted closure exactly replicates what the old hardcoded field set, so there is no behavioural change for existing callers.
Doc comment
The new comment block clearly documents the nil-vs-concrete-resolver contract so future test authors don't need to read the implementation to understand what to pass.
Removal of post-construction override in TestReshapeRecommendations_Integration_ScopedAccount_FiltersToAccount (Lines 342–352)
This is the core improvement. The old pattern required a reader to notice the h.reshapeAccountResolver = … reassignment several lines after construction; the new pattern makes the scoped intent impossible to accidentally drop in a copy-paste. No logic change.
Three nil-passing callers (Lines 146, 208, 244)
All mechanical — correct and consistent.
Minor observations (non-blocking)
- Test plan checkboxes are unchecked — presumably intentional (run locally before merge), no action needed.
- The PR closes issue
#192; if there are downstream tests that relied on the old comment explaining the STS bypass, they benefit from the new, more precise doc comment.
No issues to flag. The change is ready to merge.
🧠 Learnings used
Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-05-05T15:44:54.463Z
Learning: In the LeanerCloud/CUDly repository, when a CodeRabbit review flags something as "outside-diff" that was already implemented in an earlier commit of the same PR, it is likely a stale-snapshot artifact where the review diff was generated against a pre-fix view. The inline comment convention `(CR pass on PR #<number> Finding N)` is used by the maintainer (cristim) to mark lines that were changed in direct response to a prior CodeRabbit review pass.
Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-05-03T16:11:08.349Z
Learning: In the LeanerCloud/CUDly repository, `MockConfigStore` uses function-hook fields (e.g., `SetPlanAccountsFn`, `SaveAccountServiceOverrideFn`) to allow individual tests to wire in spy/stub behaviour. When adding new store methods to the mock, follow this same hook-field pattern rather than embedding logic directly in the mock struct.
Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-08T08:41:47.426Z
Learning: In LeanerCloud/CUDly, `store_postgres.go::CreatePurchasePlan` only inserts into the `purchase_plans` table. The `plan_accounts` insert happens in `handler_plans.go::createPlan` (the HTTP handler layer), which calls `h.config.SetPlanAccounts(ctx, plan.ID, req.TargetAccounts)` after `CreatePurchasePlan`, guarded by a `rollbackPlan` closure that calls `DeletePurchasePlan` on failure. `PlanRequest` includes a `TargetAccounts []string` field and `validateTargetAccounts` enforces a non-empty UUID list. The `updatePlan` handler does NOT call `SetPlanAccounts`, so the frontend must call `api.setPlanAccounts` explicitly on the update path but not on the create path (as of PR `#861`).
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
Accept resolveAccount as a 4th parameter (nil = unscoped default). Removes the post-construction override in the scoped-account test, making the "I want the scoped branch" intent a constructor param rather than an easy-to-forget mutation after construction. Closes #192
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
Summary
resolveAccount func(context.Context) (string, error)as a 4th parameter tobuildReshapeHandler(test helper only), withnildefaulting to the unscoped("", nil)resolver.nil.TestReshapeRecommendations_Integration_ScopedAccount_FiltersToAccountwith an inline constructor argument, making the scoped intent explicit and impossible to accidentally forget in future copy-pastes.Test plan
go build ./...passes (verified locally)grep -n "h.reshapeAccountResolver = " internal/api/handler_ri_exchange_integration_test.goreturns zero resultsgo test -tags=integration ./internal/api/...(requires Docker / testcontainers)