Conversation
WalkthroughThis PR removes most Sentry logging helpers and breadcrumb calls across the exchange app and app-wide init, consolidating telemetry to a single error-focused helper ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ 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 |
Deploying x with
|
| Latest commit: |
2544471
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://786a27bc.x-e62.pages.dev |
| Branch Preview URL: | https://fix-sentry-logging.x-e62.pages.dev |
src/apps/the-exchange/components/ExchangeAction/ExchangeAction.tsx
Outdated
Show resolved
Hide resolved
github-actions
bot
temporarily deployed
to
Preview (fix/sentry-logging)
github-actions
bot
temporarily deployed
to
Preview (fix/sentry-logging)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/apps/the-exchange/components/EnterAmount/EnterAmount.tsx (1)
136-147: Align error payload with logExchangeError allowlist; current fields are droppedlogExchangeError only persists operation, swapToken, receiveToken, amount. Passing params and walletAddress here is ignored, losing useful diagnostics.
Update payload to recognized keys (and include chainIds succinctly). Example:
- logExchangeError( - e, - { - operation: 'get_best_offer', - params, - walletAddress, - }, - { - component: 'EnterAmount', - method: 'getOffer', - } - ); + logExchangeError( + e instanceof Error ? e : String(e), + { + operation: 'get_best_offer', + swapToken: swapToken?.symbol, + receiveToken: receiveToken?.symbol, + amount: amountSwap, + fromChainId: chainNameToChainIdTokensData(swapToken?.blockchain) ?? undefined, + toChainId: chainNameToChainIdTokensData(receiveToken?.blockchain) ?? undefined, + walletAddress, + }, + { component: 'EnterAmount', method: 'getOffer' } + );Also see util-side enhancement below to pick up walletAddress and chainId.
src/apps/the-exchange/components/ExchangeAction/ExchangeAction.tsx (1)
244-258: Fix payload key: use amount instead of amountSwaplogExchangeError whitelists amount; amountSwap won’t be captured.
logExchangeError( error instanceof Error ? error : String(error), { operation: 'exchange_click', swapToken: swapToken?.symbol, receiveToken: receiveToken?.symbol, - amountSwap, + amount: amountSwap, walletAddress, }, { component: 'ExchangeAction', method: 'onClickToExchange', } );src/apps/the-exchange/utils/sentry.ts (1)
22-33: Support walletAddress and additional exchange context in Sentry loggingThe current implementation discards critical data that call sites actively provide. All four call sites pass
walletAddress, one passeschainId, and one passesamountSwap—but the current whitelist drops them. Adopt the proposed changes to capture this data.export const logExchangeError = ( error: Error | string, extra?: Record<string, unknown>, tags?: Record<string, string> ) => { - const walletAddress = fallbackWalletAddressForLogging(); + const ex = (extra ?? {}) as Record<string, unknown>; + const walletAddress = + (typeof ex.walletAddress === 'string' && ex.walletAddress) || + fallbackWalletAddressForLogging(); Sentry.withScope((scope) => { scope.setLevel('error'); scope.setTag('wallet_address', walletAddress); scope.setTag('app_module', 'the-exchange'); scope.setTag('error_type', 'exchange_error'); @@ - // Only include essential error data - if (extra) { - const essentialData = { - operation: extra.operation, - swapToken: extra.swapToken, - receiveToken: extra.receiveToken, - amount: extra.amount, - }; - scope.setExtra('exchange_error_data', essentialData); - } + // Only include essential error data (support common aliases) + if (extra) { + const essentialData = { + operation: ex.operation, + swapToken: ex.swapToken ?? ex.fromTokenSymbol ?? ex.fromToken, + receiveToken: ex.receiveToken ?? ex.toTokenSymbol ?? ex.toToken, + amount: (ex.amount ?? ex.amountSwap ?? ex.fromAmount) as unknown, + chainId: ex.chainId ?? ex.fromChainId ?? ex.toChainId, + }; + scope.setExtra('exchange_error_data', essentialData); + }
♻️ Duplicate comments (1)
src/apps/the-exchange/index.tsx (1)
171-175: Same as above: extra fields are droppedlifi_config_init error passes walletAddress only; util currently discards it. Extend util to tag wallet when provided.
🧹 Nitpick comments (7)
src/components/BottomMenuModal/SendModal/SendModalBottomButtons.tsx (1)
31-33: Consider removing the unusedallowBatchingprop.The
allowBatchingprop is defined but never used. The batch button visibility is already controlled by the presence ofonAddToBatch. Unless there's a specific near-term plan to use this prop, consider removing it to reduce maintenance overhead.- // Disabled line check here as we may need this in future - // eslint-disable-next-line @typescript-eslint/no-unused-vars - allowBatching = true, + allowBatching = true,If
allowBatchingwill control independent logic in the future, document that plan in a TODO comment for clarity.src/main.tsx (3)
69-79: Always reduce extras to an allowlistCurrently extras are trimmed only when >5 keys, which can still leak non‑essential extras. Reduce to an allowlist regardless of count.
- // Remove large extra data - if (filteredEvent.extra && Object.keys(filteredEvent.extra).length > 5) { - const essentialKeys = ['error_data', 'exchange_error_data']; - const filteredExtra: Record<string, unknown> = {}; - essentialKeys.forEach((key) => { - if (filteredEvent.extra && filteredEvent.extra[key]) { - filteredExtra[key] = filteredEvent.extra[key]; - } - }); - filteredEvent.extra = filteredExtra; - } + // Keep only essential extra data + if (filteredEvent.extra) { + const essentialKeys = ['error_data', 'exchange_error_data']; + const filteredExtra: Record<string, unknown> = {}; + essentialKeys.forEach((key) => { + if (filteredEvent.extra && filteredEvent.extra[key]) { + filteredExtra[key] = filteredEvent.extra[key]; + } + }); + filteredEvent.extra = filteredExtra; + }
59-67: Preserve minimal auth context by allowlisting itYou set Sentry.setContext('authentication_state', …) elsewhere, but beforeSend removes all non‑essential contexts. Keep a tiny auth context by allowlisting it here.
- const essentialContexts = ['runtime', 'browser']; + const essentialContexts = ['runtime', 'browser', 'authentication_state'];
31-35: Gate Sentry by DSN to avoid overhead in non‑Sentry envsAvoid initializing when DSN is missing.
- enabled: true, + enabled: Boolean(import.meta.env.VITE_SENTRY_DSN),src/apps/the-exchange/components/ExchangeAction/ExchangeAction.tsx (1)
200-208: Avoid excessively long transaction names derived from calldataIncluding full data inflates memory/logs and can break UIs. Use a short suffix.
- const transactionName = `tx-${chainId}-${data}`; + const dataSuffix = (data?.toString() ?? '').slice(0, 18); + const transactionName = `tx-${chainId}-${dataSuffix}`; const batchName = `batch-${chainId}`;src/containers/Main.tsx (1)
151-160: Mask addresses in breadcrumbsYou log accountAddress verbatim. Prefer truncating to reduce PII while keeping utility.
- data: { - accountAddress: address, + data: { + accountAddress: `${address.slice(0, 6)}...${address.slice(-4)}`,src/apps/the-exchange/index.tsx (1)
43-46: Run initSentryForExchange onceTags are static; re‑running on wallet UI state changes is unnecessary.
- useEffect(() => { - initSentryForExchange(); - }, [walletAddress, isSwapOpen, isReceiveOpen]); + useEffect(() => { + initSentryForExchange(); + }, []);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
src/apps/the-exchange/SENTRY_LOGGING.md(0 hunks)src/apps/the-exchange/components/CardsSwap/CardsSwap.tsx(0 hunks)src/apps/the-exchange/components/EnterAmount/EnterAmount.tsx(1 hunks)src/apps/the-exchange/components/ExchangeAction/ExchangeAction.tsx(2 hunks)src/apps/the-exchange/components/ExchangeAction/test/ExchangeAction.test.tsx(0 hunks)src/apps/the-exchange/components/SelectToken/SelectToken.tsx(0 hunks)src/apps/the-exchange/components/SwapSummary/SwapSummary.tsx(0 hunks)src/apps/the-exchange/hooks/useOffer.tsx(0 hunks)src/apps/the-exchange/index.tsx(4 hunks)src/apps/the-exchange/utils/sentry.ts(4 hunks)src/components/BottomMenuModal/SendModal/SendModalBottomButtons.tsx(1 hunks)src/containers/Main.tsx(1 hunks)src/main.tsx(1 hunks)
💤 Files with no reviewable changes (6)
- src/apps/the-exchange/components/SelectToken/SelectToken.tsx
- src/apps/the-exchange/hooks/useOffer.tsx
- src/apps/the-exchange/components/ExchangeAction/test/ExchangeAction.test.tsx
- src/apps/the-exchange/components/SwapSummary/SwapSummary.tsx
- src/apps/the-exchange/components/CardsSwap/CardsSwap.tsx
- src/apps/the-exchange/SENTRY_LOGGING.md
🧰 Additional context used
🧬 Code graph analysis (2)
src/apps/the-exchange/index.tsx (1)
src/apps/the-exchange/utils/sentry.ts (1)
logExchangeError(17-53)
src/apps/the-exchange/components/ExchangeAction/ExchangeAction.tsx (1)
src/apps/the-exchange/utils/sentry.ts (1)
logExchangeError(17-53)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: lint
- GitHub Check: unit-tests
- GitHub Check: build
🔇 Additional comments (3)
src/components/BottomMenuModal/SendModal/SendModalBottomButtons.tsx (1)
39-53: LGTM! Sentry logging removed as intended.The removal of Sentry context and breadcrumb logging aligns with the PR objectives. All button handlers remain functionally intact, and the comments clearly document where logging was removed, which will be helpful when reintroducing logging later.
src/containers/Main.tsx (1)
78-91: authentication_state context currently gets droppedbeforeSend only keeps runtime/browser. Either remove this setContext or keep it minimal and allowlist via src/main.tsx change suggested earlier. I recommend the latter so you retain a tiny auth snapshot without quota impact.
Would you like me to wire a tiny helper to set a masked auth context only on errors?src/apps/the-exchange/index.tsx (1)
146-154: Ensure chainId and wallet are preserved by logExchangeErrorCurrent util ignores chainId and walletAddress. Either pass recognized keys (swapToken, receiveToken, amount) or extend the util to capture chainId and wallet tag (see utils/sentry.ts comment). If you keep these fields, adopt the util enhancement so they persist.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/containers/Main.tsx (1)
105-106: Comment could be more specific.The comment "Remove breadcrumb logging to save quota" documents the change but doesn't indicate what breadcrumb was removed (e.g., "User authenticated" breadcrumb, Privy authentication state, etc.). While the intent is clear, being more specific helps future maintainers understand exactly what was removed and why.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/containers/Main.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: lint
- GitHub Check: unit-tests
- GitHub Check: build
| // Minimal Sentry context for authentication state - only set on errors | ||
| useEffect(() => { | ||
| Sentry.setContext('authentication_state', { | ||
| ready, | ||
| authenticated, | ||
| hasUser: !!user, | ||
| hasWallets: wallets.length > 0, | ||
| wagmiIsConnected, | ||
| isAppReady, | ||
| isAuthenticated, | ||
| previouslyAuthenticated, | ||
| userAgent: navigator.userAgent, | ||
| timestamp: new Date().toISOString(), | ||
| }); | ||
|
|
||
| if (isAuthenticated) { | ||
| Sentry.addBreadcrumb({ | ||
| category: 'authentication', | ||
| message: 'User authenticated', | ||
| level: 'info', | ||
| data: { | ||
| authenticated, | ||
| wagmiIsConnected, | ||
| walletsCount: wallets.length, | ||
| }, | ||
| // Only set context if there's an authentication error | ||
| if (!isAuthenticated && ready && isAppReady) { | ||
| Sentry.setContext('authentication_state', { | ||
| ready, | ||
| authenticated, | ||
| isAppReady, | ||
| isAuthenticated, | ||
| timestamp: new Date().toISOString(), | ||
| wagmiIsConnected, | ||
| hasUser: !!user, | ||
| userAddress: user?.wallet?.address, | ||
| }); | ||
| } | ||
| }, [ | ||
| ready, | ||
| authenticated, | ||
| user, | ||
| wagmiIsConnected, | ||
| isAppReady, | ||
| isAuthenticated, | ||
| previouslyAuthenticated, | ||
| wallets.length, | ||
| user, | ||
| wagmiIsConnected, | ||
| ]); |
There was a problem hiding this comment.
Reconsider the "error-focused" logging approach.
The comment claims this is "error-focused" logging that only sets context "on errors", but the condition !isAuthenticated && ready && isAppReady checks for a normal "not yet authenticated" state, not an error condition. This will still execute during normal app initialization flows and consume Sentry quota.
Additionally:
- Line 90 captures
userAddresswhen!isAuthenticated, which seems contradictory—if the user isn't authenticated, why capture their address? - The dependencies (
user,wagmiIsConnected) will cause this effect to re-run frequently during authentication transitions, potentially setting context multiple times.
If the goal is truly error-focused logging to reduce quota usage, consider:
- Only setting context when specific error conditions occur (e.g., authentication failures, timeouts, exceptions).
- Removing or throttling context updates during normal state transitions.
- Reconsidering whether to capture user data when not authenticated.
🤖 Prompt for AI Agents
In src/containers/Main.tsx around lines 78-100: the useEffect currently runs for
a normal "not yet authenticated" state (condition: !isAuthenticated && ready &&
isAppReady), causing non-error Sentry context updates, capturing userAddress
despite not being authenticated, and re-running frequently due to broad
dependencies. Change the guard to only run on actual authentication error
conditions (e.g., an authError flag, timeout, or caught exception) instead of
generic !isAuthenticated; do not include userAddress when user is not
authenticated (remove or conditionally include it only when authenticated);
reduce effect dependencies to only error-related values and/or add a
one-time/throttling mechanism (ref or timestamp check) so context is set only
once per error occurrence.
| ); | ||
| }); | ||
| }; | ||
| // Remove all other logging functions to save quota |
| allowBatching, | ||
| estimatedCostFormatted, | ||
| ]); | ||
| // Remove Sentry context logging to save quota |
| }, | ||
| }); | ||
|
|
||
| // Remove breadcrumb logging to save quota |
| }, | ||
| }); | ||
|
|
||
| // Remove breadcrumb logging to save quota |
| }, | ||
| }); | ||
|
|
||
| // Remove breadcrumb logging to save quota |
src/containers/Main.tsx
Outdated
| }, | ||
| }); | ||
| }, [authenticated, user, wallets.length]); | ||
| // Remove breadcrumb logging to save quota |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/apps/the-exchange/utils/sentry.ts (1)
3-7: Fix useEffect to initialize Sentry only once on app startup.The
initSentryForExchange()call insrc/apps/the-exchange/index.tsx(line 44) runs every timewalletAddress,isSwapOpen, orisReceiveOpenchanges because of the dependency array[walletAddress, isSwapOpen, isReceiveOpen]. Change the dependency array to an empty array[]so initialization runs exactly once when the component mounts:useEffect(() => { initSentryForExchange(); }, []);
🧹 Nitpick comments (10)
src/main.tsx (5)
35-37: Replay keys are unused; consider zeroing traces and removing replay config.Replay is disabled; keep config lean and unambiguous. Also set tracesSampleRate to 0 unless you intend to re-enable tracing soon.
Apply:
- tracesSampleRate: 0.01, // Only capture 1% of performance traces - replaysSessionSampleRate: 0, - replaysOnErrorSampleRate: 0, + // No tracing or replay + tracesSampleRate: 0,
35-37: Prefer SDK-level breadcrumb cap over post-hoc slicing.Set maxBreadcrumbs to 10 to avoid collecting excess in the first place.
Apply:
environment: import.meta.env.VITE_SENTRY_ENVIRONMENT ?? 'staging', + maxBreadcrumbs: 10,
54-66: Strip PII-prone top-level fields too.Remove user and request objects to avoid accidental PII.
Apply:
filteredEvent.contexts = filteredContexts; } + // Strip PII-prone fields + delete (filteredEvent as any).user; + delete (filteredEvent as any).request;
67-77: Always whitelist extra keys; don’t rely on a length threshold.Trim extra to the allowed set regardless of count and drop undefineds.
Apply:
- if (filteredEvent.extra && Object.keys(filteredEvent.extra).length > 5) { - const essentialKeys = ['error_data', 'exchange_error_data']; - const filteredExtra: Record<string, unknown> = {}; - essentialKeys.forEach((key) => { - if (filteredEvent.extra && filteredEvent.extra[key]) { - filteredExtra[key] = filteredEvent.extra[key]; - } - }); - filteredEvent.extra = filteredExtra; - } + if (filteredEvent.extra) { + const essentialKeys = ['error_data', 'exchange_error_data'] as const; + const filteredExtra: Record<string, unknown> = {}; + for (const k of essentialKeys) { + const v = (filteredEvent.extra as any)[k]; + if (v !== undefined) filteredExtra[k] = v; + } + filteredEvent.extra = filteredExtra; + }
35-37: Optional: gate Sentry by DSN and prod.Avoid initializing when DSN is absent or in non-prod. Keeps local/dev clean.
Example:
const DSN = import.meta.env.VITE_SENTRY_DSN; const enabled = Boolean(DSN) && import.meta.env.PROD; if (enabled) { Sentry.init({ dsn: DSN, /* ... */ }); }src/apps/the-exchange/utils/sentry.ts (5)
25-29: Unify tag names; drop app_module to avoid duplicate semantics.You already set app/module globally in init. Keep a single convention.
Apply:
- scope.setTag('app_module', 'the-exchange'); scope.setTag('error_type', 'exchange_error');
36-45: Whitelist and de-undef extra; coerce amount to a safe primitive.Keeps payload tight and deterministic.
Apply:
- const essentialData = { - operation: extra.operation, - swapToken: extra.swapToken, - receiveToken: extra.receiveToken, - amount: extra.amount, - }; + const essentialData = Object.fromEntries( + Object.entries({ + operation: (extra as any).operation, + swapToken: (extra as any).swapToken, + receiveToken: (extra as any).receiveToken, + amount: + typeof (extra as any).amount === 'object' + ? String((extra as any).amount) + : (extra as any).amount, + }).filter(([, v]) => v !== undefined) + );
24-52: Return the event id for correlation.Capture the id inside withScope and return it from the function.
Apply:
export const logExchangeError = ( error: Error | string, extra?: Record<string, unknown>, tags?: Record<string, string> ) => { const walletAddress = fallbackWalletAddressForLogging(); + let eventId: string | undefined; Sentry.withScope((scope) => { @@ - if (error instanceof Error) { - Sentry.captureException(error); - } else { - Sentry.captureMessage(error, 'error'); - } + eventId = + error instanceof Error + ? Sentry.captureException(error) + : Sentry.captureMessage(error as string, 'error'); }); + + return eventId; };
9-14: Update stale comment or accept wallet address as a param.Function always returns 'unknown_wallet_address'; the “component context” note is misleading.
Apply:
-// This function should be called from within a React component context -// where the wallet address is available +// Fallback when the wallet address is not available or must not be loggedAlternative: accept an optional walletAddress in logExchangeError and set the tag only if provided.
55-59: Naming: use implies a React Hook.*Consider an alias to avoid rules-of-hooks confusion while keeping BC.
Apply:
export const useWalletAddressForLogging = () => { // This hook is kept for compatibility but returns minimal data return 'unknown_wallet_address'; }; + +// Back-compat alias with a non-hook name (preferred for new code) +export const getWalletAddressForLogging = useWalletAddressForLogging;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/apps/the-exchange/utils/sentry.ts(4 hunks)src/components/BottomMenuModal/SendModal/SendModalBottomButtons.tsx(1 hunks)src/containers/Main.tsx(1 hunks)src/main.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/BottomMenuModal/SendModal/SendModalBottomButtons.tsx
- src/containers/Main.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: lint
- GitHub Check: unit-tests
- GitHub Check: build
| // Only send error-level events to Sentry | ||
| if (event.level && event.level !== 'error' && event.level !== 'fatal') { | ||
| return null; // Drop the event | ||
| } |
There was a problem hiding this comment.
Non-error events with missing level still pass. Tighten the drop condition.
Only allow exceptions or explicit error/fatal. Otherwise drop.
Apply:
- // Only send error-level events to Sentry
- if (event.level && event.level !== 'error' && event.level !== 'fatal') {
- return null; // Drop the event
- }
+ // Only send exceptions or explicit error/fatal levels
+ const isError =
+ (event as any).exception != null ||
+ event.level === 'error' ||
+ event.level === 'fatal';
+ if (!isError) return null;🤖 Prompt for AI Agents
In src/main.tsx around lines 41 to 44, the current filter only drops events when
event.level exists and is not 'error'/'fatal', allowing events with missing
level to pass; change the condition to only allow events that are exceptions or
have an explicit 'error' or 'fatal' level. Replace the current if with a guard
that returns null unless event.exception is present OR event.level === 'error'
OR event.level === 'fatal', ensuring all other events (including those with no
level) are dropped.
2 participants
Description
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Summary by CodeRabbit