test: affiliate#220
Merged
Merged
Conversation
…w trader data structure
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds an affiliate e2e test suite: test constants and a mocked config, three snapshot specs (partner overview, trader overview, trader activity), a sanitize helper, and Nx/TypeScript changes to run e2e tests separately. ChangesAffiliate E2E Test Suite
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
|
All alerts resolved. Learn more about Socket for GitHub. This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
libs/services/e2e/affiliate/AffiliatePartnerOverview.spec.ts (1)
11-13: ⚡ Quick winParallelize the API calls for better performance.
Both test loops fetch partner data sequentially, which could be slow with 28 total calls. Consider using
Promise.allto fetch all partners concurrently, similar to the refactor suggested inAffiliateTraderActivity.spec.ts.Also applies to: 22-24
🤖 Prompt for 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. In `@libs/services/e2e/affiliate/AffiliatePartnerOverview.spec.ts` around lines 11 - 13, The loops calling service.getAffiliateStats for each PROD_PARTNERS member are sequential and should be parallelized: replace the for-loops that populate partnerOverview (keyed by partnerAddress) with a Promise.all over PROD_PARTNERS.map(...) to concurrently call service.getAffiliateStats for each partnerAddress, then assign results back into partnerOverview (or build an object from the array of [partnerAddress, stats] pairs); apply the same refactor to both occurrences mentioned so PROD_PARTNERS, partnerOverview, and service.getAffiliateStats are used with Promise.all for concurrent fetching.libs/services/e2e/affiliate/AffiliateTraderActivity.spec.ts (1)
18-20: ⚡ Quick winConsider parallelizing API calls for better test performance.
The sequential
forloop processes ~24 trader addresses one-by-one. With network latency, this could approach the 30s timeout. Parallelizing withPromise.allwould significantly improve execution speed.⚡ Proposed refactor to parallelize calls
- for (const traderAddress of [...BUGGY_TRADERS, ...PROD_TRADERS]) { - traderActivity[traderAddress] = await service.getTraderActivity(traderAddress) - } + const traders = [...BUGGY_TRADERS, ...PROD_TRADERS] + const results = await Promise.all( + traders.map(async (traderAddress) => ({ + address: traderAddress, + activity: await service.getTraderActivity(traderAddress) + })) + ) + results.forEach(({ address, activity }) => { + traderActivity[address] = activity + })🤖 Prompt for 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. In `@libs/services/e2e/affiliate/AffiliateTraderActivity.spec.ts` around lines 18 - 20, The test currently awaits service.getTraderActivity sequentially in the for loop over [...BUGGY_TRADERS, ...PROD_TRADERS], causing slow execution; replace the loop with a parallel fetch using Promise.all over the combined address array (map each traderAddress to service.getTraderActivity) and then populate traderActivity with the resolved results (preserve keys from traderAddress), ensuring any per-request errors are handled or allowed to fail the test as appropriate; references: traderActivity, service.getTraderActivity, BUGGY_TRADERS, PROD_TRADERS.libs/services/e2e/affiliate/AffiliateTraderOverview.staging.spec.ts (1)
11-13: ⚡ Quick winParallelize the API calls for better performance.
With 37 sequential API calls across both tests, execution time could approach the 30s timeout. Refactor to use
Promise.allfor concurrent fetching, as suggested in the other e2e test files.Also applies to: 22-24
🤖 Prompt for 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. In `@libs/services/e2e/affiliate/AffiliateTraderOverview.staging.spec.ts` around lines 11 - 13, The loop that fills traderOverview by awaiting service.getTraderStats sequentially for each address (using PROD_TRADERS and traderOverview) should be refactored to run concurrently with Promise.all: map PROD_TRADERS to an array of service.getTraderStats(traderAddress) promises, await Promise.all to get results, and then assign results back into traderOverview (preserving traderAddress keys). Apply the same Promise.all refactor to the other similar block that populates traderOverview (the second occurrence using PROD_TRADERS and service.getTraderStats).
🤖 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.
Nitpick comments:
In `@libs/services/e2e/affiliate/AffiliatePartnerOverview.spec.ts`:
- Around line 11-13: The loops calling service.getAffiliateStats for each
PROD_PARTNERS member are sequential and should be parallelized: replace the
for-loops that populate partnerOverview (keyed by partnerAddress) with a
Promise.all over PROD_PARTNERS.map(...) to concurrently call
service.getAffiliateStats for each partnerAddress, then assign results back into
partnerOverview (or build an object from the array of [partnerAddress, stats]
pairs); apply the same refactor to both occurrences mentioned so PROD_PARTNERS,
partnerOverview, and service.getAffiliateStats are used with Promise.all for
concurrent fetching.
In `@libs/services/e2e/affiliate/AffiliateTraderActivity.spec.ts`:
- Around line 18-20: The test currently awaits service.getTraderActivity
sequentially in the for loop over [...BUGGY_TRADERS, ...PROD_TRADERS], causing
slow execution; replace the loop with a parallel fetch using Promise.all over
the combined address array (map each traderAddress to service.getTraderActivity)
and then populate traderActivity with the resolved results (preserve keys from
traderAddress), ensuring any per-request errors are handled or allowed to fail
the test as appropriate; references: traderActivity, service.getTraderActivity,
BUGGY_TRADERS, PROD_TRADERS.
In `@libs/services/e2e/affiliate/AffiliateTraderOverview.staging.spec.ts`:
- Around line 11-13: The loop that fills traderOverview by awaiting
service.getTraderStats sequentially for each address (using PROD_TRADERS and
traderOverview) should be refactored to run concurrently with Promise.all: map
PROD_TRADERS to an array of service.getTraderStats(traderAddress) promises,
await Promise.all to get results, and then assign results back into
traderOverview (preserving traderAddress keys). Apply the same Promise.all
refactor to the other similar block that populates traderOverview (the second
occurrence using PROD_TRADERS and service.getTraderStats).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d3e10419-7880-44bd-91a0-1c83f4360388
⛔ Files ignored due to path filters (3)
libs/services/e2e/affiliate/__snapshots__/AffiliatePartnerOverview.spec.ts.snapis excluded by!**/*.snaplibs/services/e2e/affiliate/__snapshots__/AffiliateTraderActivity.spec.ts.snapis excluded by!**/*.snaplibs/services/e2e/affiliate/__snapshots__/AffiliateTraderOverview.staging.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (7)
libs/services/e2e/affiliate/AffiliatePartnerOverview.spec.tslibs/services/e2e/affiliate/AffiliateTests.const.tslibs/services/e2e/affiliate/AffiliateTraderActivity.spec.tslibs/services/e2e/affiliate/AffiliateTraderOverview.staging.spec.tslibs/services/project.jsonlibs/services/tsconfig.spec.jsonpackage.json
shoom3301
approved these changes
May 13, 2026
shoom3301
left a comment
shoom3301
left a comment
Contributor
There was a problem hiding this comment.
There are lint issues to address
Contributor
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 `@libs/services/e2e/affiliate/snapshot.utils.ts`:
- Around line 3-22: The sanitize function's signature incorrectly claims to
always return T while it can return undefined (e.g., when
isAfterMaxSnapshotDate(...) is true); update the return type of sanitize from T
to T | undefined and adjust internal handling: ensure the Array branch returns
(value.map(sanitize).filter((e) => e !== undefined) as unknown) as (T |
undefined)[] or equivalent to preserve types, and keep the object branch
returning Object.fromEntries(...) as T | undefined; update any callers that
assume a guaranteed T to handle the undefined case. Reference: function
sanitize, predicate isAfterMaxSnapshotDate, and filtered key 'lastUpdatedAt'.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 69a866fd-66f9-4c7b-8442-65b3b046c513
⛔ Files ignored due to path filters (3)
libs/services/e2e/affiliate/__snapshots__/AffiliatePartnerOverview.spec.ts.snapis excluded by!**/*.snaplibs/services/e2e/affiliate/__snapshots__/AffiliateTraderActivity.spec.ts.snapis excluded by!**/*.snaplibs/services/e2e/affiliate/__snapshots__/AffiliateTraderOverview.staging.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (4)
libs/services/e2e/affiliate/AffiliatePartnerOverview.spec.tslibs/services/e2e/affiliate/AffiliateTraderActivity.spec.tslibs/services/e2e/affiliate/AffiliateTraderOverview.staging.spec.tslibs/services/e2e/affiliate/snapshot.utils.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- libs/services/e2e/affiliate/AffiliateTraderActivity.spec.ts
- libs/services/e2e/affiliate/AffiliateTraderOverview.staging.spec.ts
- libs/services/e2e/affiliate/AffiliatePartnerOverview.spec.ts
shoom3301
approved these changes
May 13, 2026
Contributor
Author
I'm not sure but they should be fixed in the newest commits. Which lint issues do you mean specifically? |
limitofzero
approved these changes
May 18, 2026
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
3 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.
I snapshotted parts of the dune dashboards in my quest to debug https://www.notion.so/cownation/Orders-are-not-included-into-volume-when-protocol-fee-is-missing-3298da5f04ca800f94efe1f3a64a01f4?v=25a8da5f04ca81d1b904000c7c87b11c&source=copy_link
This will also be useful when we migrate to the new fac_trades dashboards from Matias, making sure nothing breaks. https://github.com/cowprotocol/cow-dagster/pull/608
Summary by CodeRabbit