Skip to content

fix(common-utils): forward original arguments in debounce - #7920

Open
Kropiunig wants to merge 2 commits into
cowprotocol:developfrom
Kropiunig:fix/debounce-forward-args
Open

fix(common-utils): forward original arguments in debounce#7920
Kropiunig wants to merge 2 commits into
cowprotocol:developfrom
Kropiunig:fix/debounce-forward-args

Conversation

@Kropiunig

@Kropiunig Kropiunig commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Summary

debounce in libs/common-utils/src/misc.ts invokes the wrapped function with the collected arguments array as a single first parameter (func(args), line 48) instead of spreading it (func(...args)). Every argument-taking callback wrapped by this util receives an array as its first parameter and undefined for the rest:

const debounced = debounce((path?: string, params?: string[], title?: string) => { ... }, 1000)
debounced('/swap', ['param'], 'CoW Swap')
// after 1s the callback receives:
//   path   = ['/swap', ['param'], 'CoW Swap']   <- the whole args array
//   params = undefined
//   title  = undefined

The production consumer, debouncedPageView in libs/analytics/src/gtm/CowAnalyticsGtm.ts:109, declares exactly this 3-parameter callback. Consequences for every page_view pushed to the GTM data layer:

  • page_path is an array (e.g. ['/swap']) instead of a string, so any GTM variable/trigger doing type-sensitive matching on it misbehaves
  • any params/title passed to the public sendPageView(path, params, title) API are silently dropped: they end up folded into the page_path array while page_title/page_params become undefined and are stripped by sanitizeRecord

The fix spreads the arguments (func(...args)), which matches the declared generic type F extends (...args: any) => any and standard debounce semantics.

Regression tests added in libs/common-utils/src/misc.test.ts:

  • "forwards the original arguments to the wrapped function" — fails on develop (received [['/swap?chain=mainnet', ['param'], 'CoW Swap']]), passes with the fix
  • "invokes the wrapped function only once with the latest arguments" — also fails on develop (received [['second']]), passes with the fix
  • "does not invoke the wrapped function before the wait time elapses"

To Test

  1. Run npx nx test common-utils
  • All suites pass; reverting the one-line change in misc.ts makes the two new debounce assertions fail
  1. Run npx nx test analytics
  • Passes
  1. In the app, set window.enableGaLogging = true and navigate between pages
  • The logged page_view data-layer push contains page_path as a string (e.g. "/swap"), not an array

Background

usePageViewTracking currently passes only path, so the most visible damage today is the wrong type for page_path in the data layer. But sendPageView accepts params and title as part of the CowAnalytics interface, and debounce is exported from @cowprotocol/common-utils, so any consumer passing arguments hits the same mangling.

Summary by CodeRabbit

  • Bug Fixes
    • Fixed debounced callbacks to forward multiple arguments correctly (instead of passing a single argument array), ensuring analytics calls receive the expected (field, amount) values.
    • Updated advanced and limit order input handlers to invoke debounced trade amount analytics with separate parameters.
  • Tests
    • Added Jest coverage for debounce argument forwarding, ensuring execution is delayed appropriately and that rapid repeated calls use only the latest arguments.

@vercel

vercel Bot commented Jul 28, 2026

Copy link
Copy Markdown

@Kropiunig is attempting to deploy a commit to the cow-dev Team on Vercel.

A member of the Team first needs to authorize it.

@coderabbitai

coderabbitai Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: c53059f2-d32d-48a9-a305-c20bdb99bd7d

📥 Commits

Reviewing files that changed from the base of the PR and between cbf8e4f and d35db40.

📒 Files selected for processing (3)
  • apps/cowswap-frontend/src/modules/advancedOrders/hooks/useAdvancedOrdersActions.ts
  • apps/cowswap-frontend/src/modules/limitOrders/containers/LimitOrdersWidget/hooks/useLimitOrdersWidgetActions.ts
  • apps/cowswap-frontend/src/modules/trade/utils/analytics.ts

Walkthrough

The debounce utility now forwards arguments individually. Trade analytics callers use the corrected convention, and Jest tests cover forwarding, latest-argument behavior, and delayed invocation.

Changes

Debounce argument convention

Layer / File(s) Summary
Forward debounced arguments and validate timing
libs/common-utils/src/misc.ts, libs/common-utils/src/misc.test.ts
debounce invokes callbacks with spread arguments, with tests covering forwarding, latest arguments, and delay timing.
Update trade analytics callers
apps/cowswap-frontend/src/modules/trade/utils/analytics.ts, apps/cowswap-frontend/src/modules/advancedOrders/hooks/useAdvancedOrdersActions.ts, apps/cowswap-frontend/src/modules/limitOrders/containers/LimitOrdersWidget/hooks/useLimitOrdersWidgetActions.ts
Trade analytics callbacks and their advanced- and limit-order callers now pass field and amount as separate arguments.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested reviewers: kernelwhisperer

Poem

A rabbit tapped the timer’s drum,
“Send each carrot one by one!”
The latest bunch now leads the way,
No nibble comes before delay.
Hop, debounce, the fix is done!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: fixing debounce to forward arguments correctly.
Description check ✅ Passed The description covers Summary, To Test, and Background with concrete behavior and verification steps.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Fix failing CI checks
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@shoom3301 shoom3301 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!
The change looks valid. I would kindly ask to check the places where the function is in use. I see two places:

  • debouncedTradeAmountAnalytics([field, Number(typedValue)])
  • this.debouncedPageView(path, params, title)

It looks like with this PR changes debouncedTradeAmountAnalytics call can be simplified.

@Kropiunig

Copy link
Copy Markdown
Contributor Author

Good catch, thanks! Simplified the debouncedTradeAmountAnalytics call sites to pass (field, value) directly and changed the handler to take the two arguments naturally instead of a tuple. Verified debouncedPageView(path, params, title) already forwards correctly with the fixed debounce, so it needed no change. Updated in d35db40.

@vercel

vercel Bot commented Aug 5, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
swap-dev Ready Ready Preview Aug 5, 2026 2:08pm

Request Review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants