fix(notifications): do not automatically subscribe account #5638
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Skipped Deployments
|
WalkthroughThis change refactors the Telegram authentication and subscription logic in the frontend application. The functions handling authentication and authorization are renamed and their responsibilities swapped for clarity. A new helper function is introduced to unify checking and adding Telegram subscriptions, and a new function manages the conditional subscription flow. The effect for managing subscription status is updated to use the new helper. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ConnectTelegram
participant TelegramAPI
participant CMS
User->>ConnectTelegram: Clicks Subscribe
ConnectTelegram->>ConnectTelegram: subscribeAccount()
alt Not subscribed
ConnectTelegram->>TelegramAPI: authorize()
TelegramAPI-->>ConnectTelegram: Auth callback
ConnectTelegram->>CMS: checkOrAddTgSubscription('POST')
CMS-->>ConnectTelegram: Subscription status
else Already subscribed
ConnectTelegram->>CMS: checkOrAddTgSubscription('POST')
CMS-->>ConnectTelegram: Subscription status
end
ConnectTelegram-->>User: Update subscription state
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🔭 Outside diff range comments (1)
apps/cowswap-frontend/src/modules/notifications/containers/ConnectTelegram.tsx (1)
64-74: 🛠️ Refactor suggestionMissing fallback when the Telegram SDK is not yet injected
authorizebails out early ifwindow.Telegramis undefined. In that case the user has no way to progress because the callback is never executed and the script may still load a few milliseconds later.Consider:
- Moving
setIsLoginInProgress(true)above thewindow.Telegramguard and- Falling back to
authenticate(callback)rather than returning immediately.This keeps the UI in a predictable state and still honours the happy path once the widget finishes loading.
🧹 Nitpick comments (2)
apps/cowswap-frontend/src/modules/notifications/containers/ConnectTelegram.tsx (2)
55-60: Use optional chaining to simplify null-checksThe static-analysis hint is correct – the manual check
response && response.usercan be replaced by optional chaining to make the intent clearer and avoid the double read ofresponse.user.- if (response && response.user) { - setTgData(response.user) - } + const user = response?.user + if (user) { + setTgData(user) + }🧰 Tools
🪛 Biome (1.9.4)
[error] 56-56: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
98-108: Guard against duplicate subscription attempts
subscribeAccountalways callsaddSubscription()even ifisTgSubscribed
is already truthy afterauthorize. This is harmless server-side but still
fires an unnecessary network request (and spinner) every time.Add a check right before invoking
addSubscription():- addSubscription() + if (!isTgSubscribed) { + addSubscription() + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/cowswap-frontend/src/modules/notifications/containers/ConnectTelegram.tsx(4 hunks)apps/cowswap-frontend/src/modules/notifications/pure/TelegramConnectionStatus/index.tsx(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
apps/cowswap-frontend/src/modules/notifications/containers/ConnectTelegram.tsx
[error] 56-56: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Cypress
- GitHub Check: Setup
🔇 Additional comments (1)
apps/cowswap-frontend/src/modules/notifications/pure/TelegramConnectionStatus/index.tsx (1)
22-32: Prop rename looks good – no further action requiredThe interface rename from
authenticatetosubscribeAccountaligns the presentational component with the new logic in the container, and the callback is still passed unchanged to theToggle. No other regressions spotted in this hunk.
apps/cowswap-frontend/src/modules/notifications/containers/ConnectTelegram.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/cowswap-frontend/src/modules/notifications/containers/ConnectTelegram.tsx (1)
83-96: 🛠️ Refactor suggestionEndpoints should be consistent with leading slashes.
The
methodparameter is receiving endpoints with inconsistent leading slashes ('add-tg-subscription'in one place and'/check-tg-subscription'elsewhere). This could cause request failures if the CMS client concatenates paths naively.- const checkOrAddTgSubscription = (method: string) => { + const checkOrAddTgSubscription = (endpoint: string) => {Also ensure all calls to this function use consistent leading slashes in the endpoints.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/cowswap-frontend/src/modules/notifications/containers/ConnectTelegram.tsx(4 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
apps/cowswap-frontend/src/modules/notifications/containers/ConnectTelegram.tsx
[error] 56-56: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🪛 GitHub Actions: CI
apps/cowswap-frontend/src/modules/notifications/containers/ConnectTelegram.tsx
[error] 127-127: ESLint: React Hook useEffect has a missing dependency: 'checkOrAddTgSubscription'. Either include it or remove the dependency array (react-hooks/exhaustive-deps)
🔇 Additional comments (5)
apps/cowswap-frontend/src/modules/notifications/containers/ConnectTelegram.tsx (5)
54-62: Improved naming to match actual functionality.The function renaming from
authorizetoauthenticatebetter reflects its actual purpose - checking the authentication status usinggetTelegramAuth. This aligns with the updated flow where we don't automatically subscribe users.🧰 Tools
🪛 Biome (1.9.4)
[error] 56-56: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
64-81: Improved function naming and callback handling.The function has been correctly renamed from
authenticatetoauthorizeto better reflect its purpose of initiating the Telegram login flow. The callback is now properly called in all code paths which ensures proper flow completion.
98-108: Well-implemented conditional subscription flow.The new
subscribeAccountfunction properly implements the required flow: initiating Telegram authorization first if needed, or directly adding a subscription if already authenticated. This resolves the core issue described in the PR objectives.
118-118: Correct update to use authenticate instead of authorize.The initial authentication effect now properly calls
authenticateinstead ofauthorize, which aligns with the new flow of checking authentication status first without automatic subscription.
153-157: Correct prop update for TelegramConnectionStatus component.The component now correctly receives the
subscribeAccountfunction as a prop instead of the previousauthenticatefunction, which aligns with the updated subscription handling flow.
apps/cowswap-frontend/src/modules/notifications/containers/ConnectTelegram.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
apps/cowswap-frontend/src/modules/notifications/containers/ConnectTelegram.tsx (1)
64-73:⚠️ Potential issueGuard against
window.Telegram.Loginbeing undefined
window.Telegrammight be present whilewindow.Telegram.Loginis still unavailable (e.g. partial script load or bot being blocked by CSP).
Callingwindow.Telegram.Login.authwithout checking will throw a runtime error that cannot be caught by React’s error boundary.- if (!window.Telegram) return + if (!window.Telegram?.Login?.auth) { + // Fallback to a passive auth check + authenticate(callback) + return + }
♻️ Duplicate comments (1)
apps/cowswap-frontend/src/modules/notifications/containers/ConnectTelegram.tsx (1)
83-87: Rename misleading parametermethod→endpointThis util always issues an HTTP POST; the variable holds the CMS endpoint path, not the HTTP method. Renaming avoids confusion for future readers.
- const checkOrAddTgSubscription = useCallback( - (method: string) => { + const checkOrAddTgSubscription = useCallback( + (endpoint: string) => { … - getCmsClient().POST(method, { body: { account, data: tgData } }) + getCmsClient().POST(endpoint, { body: { account, data: tgData } })
🧹 Nitpick comments (1)
apps/cowswap-frontend/src/modules/notifications/containers/ConnectTelegram.tsx (1)
55-60: Prefer optional–chaining for terser, safer null-checks
response && response.useris functionally fine but the linter has already flagged it (lint/complexity/useOptionalChain). Using optional–chaining keeps the code concise and prevents accidental mistakes when the object shape changes.- if (response && response.user) { + if (response?.user) {🧰 Tools
🪛 Biome (1.9.4)
[error] 56-56: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/cowswap-frontend/src/modules/notifications/containers/ConnectTelegram.tsx(5 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
apps/cowswap-frontend/src/modules/notifications/containers/ConnectTelegram.tsx
[error] 56-56: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Cypress
- GitHub Check: Setup
🔇 Additional comments (1)
apps/cowswap-frontend/src/modules/notifications/containers/ConnectTelegram.tsx (1)
124-131: Possible race condition when switching accounts fast
setTgSubscribed(false)fires before launching the CMS request.
If the user changes wallets rapidly, an earlier request can finish after the newer one and overwrite state with a stale value.Consider tracking the requested
accountin a ref and discarding late responses, or use anAbortController.- .then(({ data: result }) => { - setTgSubscribed(result) - }) + .then(({ data: result }) => { + if (currentAccountRef.current === account) { + setTgSubscribed(result) + } + })(Where
currentAccountRefis updated on every account change.)
This prevents flickering “Connected” labels for the wrong account.
Summary
Needs cowprotocol/cms#60 to be merged first, before testing.
The bug was in the fact that we called
POST /add-tg-subscriptionjust if we have a Telegram auth session, which is not right.Now we do:
When we click on the toggle, we do:
To Test
Summary by CodeRabbit
Refactor
Bug Fixes