Delete the sumupApi and squareApi alias wrapper exports - #2061
Conversation
The eleven one-line wrappers (four on sumup.ts, seven on square.ts) only renamed members of the stubbable api objects. Every caller now uses the shared mechanism itself — sumupApi.X(...) / squareApi.X(...) — which late-binds through test stubs exactly like the wrappers did. The two functions captured at module load (the SumUp checkout builder handed to makeCreateCheckoutSession, and each settings page's testFn) become lambdas over the api member so stubbing keeps working. Migrated callers: sumup-provider, square-provider, settings-sumup, settings-square, and the seven direct test importers. Stripe already had no wrapper layer. Closes the TODO entry recorded for this sweep, and marks M3 complete in the plan now that #2060 is merged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AqLNrpvmNdGSSqUtSafiMd
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 46 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: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe change removes Square and SumUp alias wrapper exports. Production callers and tests now access methods through ChangesPayment API migration
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6f09d0788f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| }, | ||
| async ({ paymentsGet }) => { | ||
| const result = await retrievePayment("pay_wrapper"); | ||
| const result = await squareApi.retrievePayment("pay_wrapper"); |
There was a problem hiding this comment.
After deleting the wrapper export, this changed call invokes squareApi.retrievePayment directly, so the suite can no longer verify the delegation named by its title and merely repeats the retrievePayment coverage immediately above. Delete this wrapper-only case rather than retaining a misleading dead test; the repository specifically requires removing a deleted export's now-pointless test.
AGENTS.md reference: AGENTS.md:L170-L177
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9039160 — the whole "retrievePayment wrapper export" describe block is deleted, exactly per the dead-code rule you cite: with the wrapper gone the test asserted nothing the direct squareApi.retrievePayment coverage above it doesn't already pin. The same commit also fixes the two type-position leftovers (typeof getSquareClient / typeof testSquareConnection) that failed CI's typecheck.
Generated by Claude Code
The sweep's call-site migration missed two references in type position (typeof getSquareClient / typeof testSquareConnection in the client suite), which failed CI's typecheck; they now name the api members. The "retrievePayment wrapper export delegates" test asserted the very hop the sweep deleted, so it duplicated the direct coverage above it — removed, as the dead-code rule requires for a deleted export's test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AqLNrpvmNdGSSqUtSafiMd
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 90391606cc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -846,16 +846,6 @@ export const squareApi: { | |||
| }, | |||
| }; | |||
|
|
|||
There was a problem hiding this comment.
Because this deletion removes the top-level resetSquareClient export, the ALLOWED_TEST_HOOKS entry shared/square.ts:resetSquareClient in test/integration/code-quality.test.ts no longer matches any export. Leaving that dead exemption misdocuments the API and would silently exempt the same test-only alias if it were reintroduced later, so remove its entry and comment with the wrapper.
AGENTS.md reference: AGENTS.md:L170-L177
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 366a499 — the shared/square.ts:resetSquareClient entry and its comment are removed from ALLOWED_TEST_HOOKS. Tests reach the reset through squareApi.resetSquareClient(), and squareApi itself has production callers, so the export scan needs no exemption; the code-quality suite passes with the entry gone (19/19, including the test-only-exports scan). I also checked the rest of the list for entries orphaned by this sweep — constructTestWebhookEvent is still a live top-level export, and no sumup.ts entries exist — so this was the only stale one.
Generated by Claude Code
The wrapper sweep deleted the top-level resetSquareClient export, so the ALLOWED_TEST_HOOKS entry for it no longer matches anything — tests reach the member through squareApi, which the export scan already credits. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AqLNrpvmNdGSSqUtSafiMd
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/features/admin/settings-square.ts`:
- Around line 45-47: Annotate the public testFn callback in settings-square.ts
(lines 45-47) with ReturnType<typeof squareApi.testSquareConnection>, preserving
its late-bound squareApi.testSquareConnection() call. Apply the equivalent
explicit return type in settings-sumup.ts (lines 33-35) using ReturnType<typeof
sumupApi.testSumupConnection>.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 95b5a21c-56f3-46bc-9691-6069d561b0e4
📒 Files selected for processing (17)
PLAN.mdTODO.mdsrc/features/admin/settings-square.tssrc/features/admin/settings-sumup.tssrc/shared/square-provider.tssrc/shared/square.tssrc/shared/sumup-provider.tssrc/shared/sumup.tstest/integration/code-quality.test.tstest/shared/square/client.test.tstest/shared/square/refund-transport.test.tstest/shared/square/rest-transport.test.tstest/shared/square/retrieve-refund.test.tstest/shared/sumup/connection.test.tstest/shared/sumup/create-checkout.test.tstest/shared/sumup/transactions.test.tstest/test-utils/square/harness.ts
💤 Files with no reviewable changes (4)
- src/shared/sumup.ts
- test/integration/code-quality.test.ts
- src/shared/square.ts
- TODO.md
The square mutation run reports the testSquareConnection.locations ?? -> || entry as redundant: a test kills that mutant, so the registry entry no longer describes a real survivor. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AqLNrpvmNdGSSqUtSafiMd
|
Gate results, as promised in the description — all green:
One piece of fallout the square run surfaced: the recorded equivalent-mutant entry for All three review threads on this PR are answered: two fixed (9039160, 366a499), one declined with reasons and withdrawn by CodeRabbit. Generated by Claude Code |
2 participants
What this does
Removes the eleven one-line wrapper exports that only renamed members of the stubbable api objects — four on
sumup.ts(createCheckout,refundTransaction,getTransactionStatus,testSumupConnection) and seven onsquare.ts(getSquareClient,resetSquareClient,testSquareConnection,createPaymentLink,retrieveOrder,retrievePayment,refundPayment). This is the sweep recorded in TODO.md during #2060's review: AGENTS.md's "No alias exports" rule says to expose the shared mechanism itself, so every caller now writessumupApi.X(...)/squareApi.X(...)directly, which late-binds through test stubs exactly like the wrappers did.The two places that capture a function at module load — the SumUp checkout builder handed to
makeCreateCheckoutSession, and each settings page'stestFn— become lambdas over the api member, so test stubbing keeps working. Stripe already had no wrapper layer.Migrated callers:
sumup-provider.ts,square-provider.ts,settings-sumup.ts,settings-square.ts, and the seven direct test importers (test/shared/sumup/*,test/shared/square/*,test/test-utils/square/harness.ts).Also in this PR
Budgets
65 changed
src/lines. No behaviour change: every call resolves to the same api member it did before, one hop earlier. No query or provider-call counts change.Tests and gates
deno task lintclean, jscpd 0%,deno checkclean on every touched file.deno task precommitand targeted mutation on the four touched payment modules are running now; results will be confirmed on this PR.🤖 Generated with Claude Code
https://claude.ai/code/session_01AqLNrpvmNdGSSqUtSafiMd
Generated by Claude Code
Summary by CodeRabbit
Refactor
Tests
Documentation