fix(affiliate): show 'code linked' notification on partial fills - #7906
tenderdeve wants to merge 8 commits into
Conversation
The 'code linked' toast was fired only from the fulfilled-order event (useNotifyAffiliateLinkedCode is mounted from FulfilledOrderInfo). A partially-filled order never reaches FULFILLED, and the recovery side effect already links the code silently from the partial-aware local-trade detection, pre-empting the fulfilled path via the isLinked guard — so no toast ever showed. Fire the toast from the same partial-aware local-trade linking path. The snackbar id is keyed on the ref code so it dedupes with the fulfilled-order path if both run, and the cross-device orderbook recovery stays silent.
|
@tenderdeve is attempting to deploy a commit to the cow-dev Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
WalkthroughThe affiliate linked-code snackbar now uses a reusable referral-code-keyed builder. Local-trade recovery returns creation timestamps, filters historical links, applies affiliate time-cap metadata, and dismisses active notifications when the account changes. ChangesAffiliate notification recovery
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
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
`@apps/cowswap-frontend/src/modules/affiliate/hooks/useAffiliateTraderRecoverySideEffect.ts`:
- Around line 31-33: Update the local-code recovery branch in the effect using
useAffiliateTraderInfo so it waits until the metadata request finishes before
linking or showing the recovery snackbar. Track the hook’s loading state,
include it in the effect dependencies, and preserve
PROGRAM_DEFAULTS.AFFILIATE_TIME_CAP_DAYS as the fallback only after a completed
not-found or failed response.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c658695d-c019-4cd6-a0f7-ffba059e4782
📒 Files selected for processing (2)
apps/cowswap-frontend/src/modules/affiliate/hooks/useAffiliateTraderRecoverySideEffect.tsapps/cowswap-frontend/src/modules/affiliate/hooks/useNotifyAffiliateLinkedCode.tsx
elena-zh
left a comment
There was a problem hiding this comment.
Hey @tenderdeve , thank you, your implementaiton works in terms of a partial fill.
However, there are 2 issues:
- I have some accounts where I was able to link a code to 'apready traded' account: now, when I open these accounts, I see 'code linked' message. However, I should not see it.
Example:0x9FA3c00a92Ec5f96B1Ad2527ab41B3932EFEDa58
- If I change an account, 'code linked' message does not disappear. Morevowe, the app may start showing another 'code linked' message for another account.
It would be nice to hide the message once I change an account.
Thanks
…unt switch The recovery side-effect fired the 'code linked' toast whenever it recovered a code from local trades, so opening an already-linked account re-showed it, and the sticky toast lingered when switching accounts. Record a per-account baseline and only notify for a code that appears after it (a live partial fill), keeping historical recovery silent like the orderbook path, and remove the toast when the account changes.
|
@elena-zh thanks — both were real. The toast was fired from the recovery path, which runs on every visit and can't tell a code linked right now from one recovered from past trades, so already-linked accounts kept re-showing it, and the sticky toast lingered when switching accounts. Fixed in df62d3d:
Could you re-check with those accounts when you have a moment? I can't reproduce a live partial-fill link locally, so I'd like to confirm a genuinely new link still toasts while the historical ones stay quiet. |
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
`@apps/cowswap-frontend/src/modules/affiliate/hooks/useAffiliateTraderRecoverySideEffect.ts`:
- Around line 35-53: Update the affiliate account identity handling in
baselineAccountsRef and the account-change useEffect: key baseline entries with
getAddressKey(account), compare prevAccountRef.current and account using
areAddressesEqual, and store the normalized address key in
prevAccountRef.current after changes. Initialize prevAccountRef consistently
with that same normalized address form.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a6f313af-d516-4222-bd29-8b84fe414804
📒 Files selected for processing (1)
apps/cowswap-frontend/src/modules/affiliate/hooks/useAffiliateTraderRecoverySideEffect.ts
| // Accounts whose baseline has been recorded. A code already present on the first evaluation for an | ||
| // account is a pre-existing (historical) link and must stay silent; only a code that appears | ||
| // afterwards — a partial fill linking a code live in this session — should raise the toast. | ||
| const baselineAccountsRef = useRef<Set<string>>(new Set()) | ||
| // Id of the affiliate toast currently shown, so it can be dismissed when the account changes. | ||
| const shownSnackbarIdRef = useRef<string | null>(null) | ||
| const prevAccountRef = useRef(account) | ||
|
|
||
| // Dismiss a lingering 'code linked' toast when the account changes, so it doesn't stay on screen | ||
| // for (or bleed into) a different account (#7527). | ||
| useEffect(() => { | ||
| if (prevAccountRef.current !== account) { | ||
| if (shownSnackbarIdRef.current) { | ||
| removeSnackbar(shownSnackbarIdRef.current) | ||
| shownSnackbarIdRef.current = null | ||
| } | ||
| prevAccountRef.current = account | ||
| } | ||
| }, [account, removeSnackbar]) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 2 'getAddressKey\(|areAddressesEqual\(' apps/cowswap-frontend/src -g '*.ts' -g '*.tsx'Repository: cowprotocol/cowswap
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
target='apps/cowswap-frontend/src/modules/affiliate/hooks/useAffiliateTraderRecoverySideEffect.ts'
if [ -f "$target" ]; then
wc -l "$target"
cat -n "$target"
else
echo "missing $target"
fd -a 'useAffiliateTraderRecoverySideEffect.ts' .
fi
printf '\n--- dependency/import references in target ---\n'
rg -n 'cow-sdk|getAddressKey|areAddressesEqual|removeSnackbar|baselineAccountsRef|prevAccountRef|useAccount|useDisconnectRecoverySideEffect' "$target" || true
printf '\n--- package versions mentioning cow-sdk ---\n'
rg -n '"`@cowprotocol/cow-sdk`"|`@cowprotocol/cow-sdk`' package.json pnpm-lock.yaml yarn.lock package-lock.json 2>/dev/null | head -40 || trueRepository: cowprotocol/cowswap
Length of output: 8582
🌐 Web query:
@cowprotocol/cow-sdk getAddressKey areAddressesEqual utility documentation
💡 Result:
In the @cowprotocol/cow-sdk, the functions getAddressKey and areAddressesEqual are utility functions found within the @cowprotocol/sdk-common package [1]. They are designed to simplify address handling and comparison across different blockchain formats. getAddressKey This function generates a normalized address key for any supported blockchain address type [2][3]. Signature: function getAddressKey(address: string): AddressKey Parameters: address: The address string to be converted into a key [2][4]. Returns: A normalized address key based on the detected address type [2][5]. areAddressesEqual This utility compares two addresses to determine if they are identical, specifically handling variations in blockchain address formats [2][3]. Signature: function areAddressesEqual(a: Nullish, b: Nullish): boolean Parameters: a: The first address string to compare (can be null or undefined) [2][4]. b: The second address string to compare (can be null or undefined) [2][5]. Returns: A boolean value indicating whether the two addresses are equal [2][3].
Citations:
- 1: https://github.com/cowprotocol/cow-sdk
- 2: https://cowprotocol-cow-sdk.mintlify.app/api/common
- 3: https://docs.cow.bleu.builders/cow-sdk/api/common
- 4: https://cowswap.mintlify.app/cow-sdk/api/common
- 5: https://mintlify.wiki/cowprotocol/cow-sdk/api/common
🌐 Web query:
@cowprotocol/cow-sdk exports getAddressKey areAddressesEqual
💡 Result:
The @cowprotocol/cow-sdk package includes getAddressKey and areAddressesEqual as utility functions, which are typically available through its common utilities package (often exported from @cowprotocol/sdk-common) [1][2][3]. getAddressKey This function is designed to normalize blockchain addresses [1][2]. It takes an address string as input and returns an AddressKey, which represents a normalized version based on the detected address type (supporting various blockchain formats) [1][2]. function getAddressKey(address: string): AddressKey [1][2] areAddressesEqual This function is used to compare two addresses to determine if they are the same, effectively handling different blockchain address formats [1][2]. It accepts two inputs, which can be nullable strings, and returns a boolean value [1][2]. function areAddressesEqual(a: Nullish, b: Nullish): boolean [1][2]
Citations:
- 1: https://cowprotocol-cow-sdk.mintlify.app/api/common
- 2: https://cowswap.mintlify.app/cow-sdk/api/common
- 3: https://github.com/cowprotocol/cow-sdk/
Use SDK address identity helpers for base-line storage and account change detection.
baselineAccountsRef keys and the account-change check both currently use raw address strings. Key the baseline set with getAddressKey(account), compare account changes with areAddressesEqual(prevAccountRef.current, account), and update prevAccountRef.current to the same address form stored in the baseline.
🤖 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
`@apps/cowswap-frontend/src/modules/affiliate/hooks/useAffiliateTraderRecoverySideEffect.ts`
around lines 35 - 53, Update the affiliate account identity handling in
baselineAccountsRef and the account-change useEffect: key baseline entries with
getAddressKey(account), compare prevAccountRef.current and account using
areAddressesEqual, and store the normalized address key in
prevAccountRef.current after changes. Initialize prevAccountRef consistently
with that same normalized address form.
Source: Coding guidelines
elena-zh
left a comment
There was a problem hiding this comment.
Hey @tenderdeve , the 2nd case is fixed, the 1st one is not.
…dration The recovery path recorded a per-account baseline on first evaluation and toasted any code that appeared afterwards. Because redux hydrates persisted orders asynchronously after mount, a historical code shows up after the baseline is recorded and was wrongly treated as a live link, so already-linked accounts kept re-showing the toast on open. Compare the linking order's own creationTime against a fixed session-start timestamp instead: only a code carried by an order created after the session started (a partial fill happening now) raises the toast; historical codes stay silent regardless of when redux hydrates them. Addresses @elena-zh's feedback.
|
@elena-zh fixed the 1st case in Root cause: the recovery path recorded a per-account baseline on first evaluation and toasted any code that appeared after it. But redux hydrates persisted orders asynchronously after mount, so a historical code shows up right after the baseline is recorded and got mistaken for a live link — that's why already-linked accounts like New rule: compare the linking order's own Case 2 (dismiss on account change) is unchanged. |
|
Hey @tenderdeve , thank you for the contribution. If your current setup doesn't allow you to build, review, or test the changes, I'd appreciate it if you could avoid starting work on that issue until you're able to validate the implementation.
Since you're not able to validate your changes in your current setup, I have to close this PR for now. Unfortunately, I don't have enough time to go through multiple rounds of back-and-forth to verify and test the implementation myself. Once you have a setup that allows you to build and test your changes, feel free to work on the issue again and open a new PR. Thanks. |
Closes #7527
Following up on my root-cause analysis — going with the direction proposed there.
Problem
The 'code linked' toast (
useNotifyAffiliateLinkedCode) is mounted only fromFulfilledOrderInfo, i.e. it fires on theON_FULFILLED_ORDERevent. A partially-filled order never reachesFULFILLED. MeanwhileuseAffiliateTraderRecoverySideEffectalready detects executed local trades with the partial-awareisExecutedNonIntegratorOrderand links the code — but silently, and because it links first it pre-empts the fulfilled-order toast via theisLinkedguard. So on a partial fill no toast ever shows.Fix
Fire the toast from the same partial-aware local-trade linking path:
buildAffiliateLinkedCodeSnackbar(refCode, timeCapDays); the snackbar id is now keyed on the ref code (not the order id). SinceaddSnackbarAtomdedupes by id, the fulfilled-order path and the recovery path can't double-toast.No change for full fills (still one toast, now possibly from either path — deduped). Partial fills now get the toast.
Notes
tscpasses. Local jest for this module can't run here (unrelated bridge-provider import crash), so the wiring is CI-verified.Summary by CodeRabbit