UI: Implement toast component#4093
Conversation
|
✅ Actions performedReview triggered.
|
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits 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: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (13)
📝 WalkthroughWalkthroughThe pull request refactors the toast notification system from a prop-driven component to a context-based provider architecture. It introduces ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontends/web/src/app.tsx`:
- Around line 68-82: The current effect sets
initializedAccountsRef.current[account.code] before the seed fetch finishes,
causing accounts to be marked initialized on failure; change the flow in the
useEffect around getTransactionList so you only set
initializedAccountsRef.current[account.code] after getTransactionList succeeds
(i.e., after transactions.success is true and after you mark incoming as seen),
and do not set it on error; additionally add a guard in
detectAndToastIncomingForAccount to skip processing if
initializedAccountsRef.current[account.code] is not truthy so that in-flight or
failed seeds prevent toast detection until the initial seed completes
successfully.
In `@frontends/web/src/components/toast/Toast.module.css`:
- Around line 7-9: The toast entry animation currently always runs for
.toastItem (animation: toast-in 360ms ease-out); add a prefers-reduced-motion
override so users who request reduced motion don't see the animation: wrap a CSS
rule in `@media` (prefers-reduced-motion: reduce) that sets animation: none (or
animation-duration: 0s / transition: none) for .toastItem, and apply the same
media-query override to the other toast animation rules referenced around lines
59-66 (the exit/other animation selectors) so all toast animations are
suppressed when prefers-reduced-motion is enabled.
In `@frontends/web/src/components/toast/toast.tsx`:
- Around line 48-49: Move the React context plumbing out of the Toast UI
component: create new files under src/contexts to define ToastContext, implement
ToastProvider, and export useToast (references: ToastContext, ToastProvider,
useToast) and then compose ToastProvider into the existing
contexts/providers.tsx Providers composition; keep the visual Toast component
and its styles in frontends/web/src/components/toast/ and update its imports to
re-export the hooks/context from the new src/contexts index so existing imports
continue to work.
- Around line 60-65: The aria-label on the close button is hardcoded; update the
Toast component to import and call useTranslation() and replace
aria-label="Close toast" with aria-label={t('toast.close'/* or appropriate key
*/)} (or another suitable translation key), ensure the translation key exists in
your i18n resources and that useTranslation() is invoked (e.g., const { t } =
useTranslation()) in the component so the close button uses t(...) instead of a
literal string.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9db8a400-811a-4926-9bb0-f43393779dd8
📒 Files selected for processing (9)
frontends/web/src/app.tsxfrontends/web/src/components/toast/Toast.module.cssfrontends/web/src/components/toast/toast.test.tsxfrontends/web/src/components/toast/toast.tsxfrontends/web/src/contexts/providers.tsxfrontends/web/src/locales/en/app.jsonfrontends/web/src/routes/router.tsxfrontends/web/src/routes/settings/toast-demo.module.cssfrontends/web/src/routes/settings/toast-demo.tsx
|
@coderabbitai PTAL |
|
Tip For best results, initiate chat on the files or code changes.
[review] |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@frontends/web/src/app.tsx`:
- Around line 36-40: The IncomingTxIcon component hardcodes stroke="#5E94C0",
which prevents theme/variant color inheritance; update the SVG <path> in
IncomingTxIcon to use a themeable color such as stroke="currentColor" (or
stroke="var(--color-info)") so the icon inherits the surrounding Message/toast
text color and respects light/dark mode and CSS variables.
In `@frontends/web/src/contexts/ToastContext.tsx`:
- Around line 1-123: Split this combined context/provider into two files: move
the context/type definitions and hook (ToastContext, TShowToast, TToastContext,
useToast and the createContext call) into a new definition file (e.g.,
toast-context.tsx) and export them; move the ToastProvider implementation
(ToastProvider, internal state, timeoutIDs, showToast, hideToast, clearToasts)
into a provider file (e.g., toast-provider.tsx) that imports ToastContext and
the exported types/hooks; update any imports (including contexts/providers.tsx)
to import ToastProvider from the new provider file and import types/hooks from
the definition file; keep the existing API names (ToastContext, ToastProvider,
useToast, TShowToast, TToastContext) and ensure all exports are preserved.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 394fd0c1-09ce-417b-a9c8-91885b3c10d3
📒 Files selected for processing (12)
frontends/web/src/app.tsxfrontends/web/src/components/toast/Toast.module.cssfrontends/web/src/components/toast/toast-view.tsxfrontends/web/src/components/toast/toast.test.tsxfrontends/web/src/components/toast/toast.tsxfrontends/web/src/contexts/ToastContext.tsxfrontends/web/src/contexts/index.tsfrontends/web/src/contexts/providers.tsxfrontends/web/src/locales/en/app.jsonfrontends/web/src/routes/router.tsxfrontends/web/src/routes/settings/toast-demo.module.cssfrontends/web/src/routes/settings/toast-demo.tsx
|
@thisconnect PTAL :) |
thisconnect
left a comment
thisconnect
left a comment
There was a problem hiding this comment.
light code review with a few questions.
| const IncomingTxIcon = () => ( | ||
| <svg width="18" height="18" viewBox="0 0 18 18" fill="none" xmlns="http://www.w3.org/2000/svg"> | ||
| <path d="M1 17H17M9 11V1M9 11L13 7M9 11L5 7" stroke="currentColor" strokeWidth="2" strokeLinecap="round" strokeLinejoin="round"/> | ||
| </svg> |
There was a problem hiding this comment.
We should avoid inline svg if possible as react will have to needlessly scan all the attributes for dom-diffing and updating the dom.
Please move to .svg file and import from @/components/icons, see how the other icons are used in the app.
| const { showToast } = useToast(); | ||
| const shownIncomingTxIDsRef = useRef<Record<string, boolean>>({}); | ||
| const initializedAccountsRef = useRef<Record<string, boolean>>({}); | ||
| const seedingAccountsRef = useRef<Record<string, boolean>>({}); |
There was a problem hiding this comment.
The commit message writes
Add in-app notification using toast component for incoming TX.
So I don't understand why we need to keep a map of all transactions and fetch all transactions first, then mark them as seen (markIncomingAsSeen).
The previous notifications just subscribed to new transcations and fire and forget a notification. Which is much simpler and seems to achieve exactly the same, but I feel I am missing something.
It would help if the commit message explains what the goal is so it is clear why this code is here.
There was a problem hiding this comment.
I think we still need this extra state for the following:
- So old TXs don't trigger a toast on sync
- to distinguish between outgoing and incoming txs (we only want incoming to trigger a toast)
- So we can have the amount and units in the toast as well (the current notification doesn't know the amount)
There was a problem hiding this comment.
At the moment I think we should keep the logic in backend and the frontend only subscribes to /new-txs endpoint.
| }, [activeAccounts, detectAndToastIncomingForAccount, seedIncomingTransactionsForAccount]); | ||
|
|
||
| return null; | ||
| }; |
There was a problem hiding this comment.
This component doesn't belong here, but maybe it can be dropped completely? see my other comment
| <svg width="16" height="18" viewBox="0 0 8 9" fill="none" xmlns="http://www.w3.org/2000/svg"> | ||
| <path d="M6.61718 3.4375V0.34375C6.61718 0.153614 6.46357 0 6.27343 0H1.63281C1.44267 0 1.28906 0.153614 1.28906 0.34375V3.4375C0.577934 3.4375 0 3.99717 0 4.6836V8.50782C0 8.55508 0.038672 8.59375 0.085938 8.59375H0.687504C0.734764 8.59375 0.773434 8.55508 0.773434 8.50782V4.6836C0.773434 4.42256 1.00547 4.21094 1.29013 4.21094H6.61611C6.90078 4.21094 7.13281 4.42256 7.13281 4.6836V8.50782C7.13281 8.55508 7.17148 8.59375 7.21875 8.59375H7.82031C7.86757 8.59375 7.90625 8.55508 7.90625 8.50782V4.6836C7.90625 3.99717 7.32832 3.4375 6.61718 3.4375ZM2.0625 3.4375V0.773438H5.84375V3.4375H2.0625ZM3.35156 1.46094H2.83593C2.78867 1.46094 2.75 1.49961 2.75 1.54688V2.0625C2.75 2.10977 2.78867 2.14844 2.83593 2.14844H3.35156C3.39882 2.14844 3.4375 2.10977 3.4375 2.0625V1.54688C3.4375 1.49961 3.39882 1.46094 3.35156 1.46094ZM5.07031 1.46094H4.55468C4.50742 1.46094 4.46875 1.49961 4.46875 1.54688V2.0625C4.46875 2.10977 4.50742 2.14844 4.55468 2.14844H5.07031C5.11757 2.14844 5.15625 2.10977 5.15625 2.0625V1.54688C5.15625 1.49961 5.11757 1.46094 5.07031 1.46094Z" fill="#5E94C0"/> | ||
| </svg> | ||
| ); |
There was a problem hiding this comment.
I was going to ask to move all inlinve SVG to @/components/icon components, but if since these are only used in the demo file I guess it's ok to keep them here.
It would be nice if these dont land in the production built, no need to do anything rightnow, but I want to check if we can exclude from our vite production build.
|
|
||
| export { ToastContext, useToast } from './toast-context'; | ||
| export { ToastProvider } from './toast-provider'; | ||
| export type { TShowToast, TToastContext } from './toast-context'; |
There was a problem hiding this comment.
Either add all contexts here in this file or remove it.
But since we don't need it, please remove this file.
| <Route path="device-settings/recovery-words/:deviceID" element={RecoveryWordsEl} /> | ||
| <Route path="device-settings/bip85/:deviceID" element={Bip85El} /> | ||
| <Route path="advanced-settings" element={AdvancedSettingsEl} /> | ||
| <Route path="toast-demo" element={ToastDemoEl} /> |
There was a problem hiding this comment.
I can't find that this route is linked from anywhere.
I am not sure 100% if we should start adding demo views in to the production build.
This route could maybe be conditionally added in case isDevServers?
We could also add a link somewhere for example in sidebar conditionally if isDevServers
| "notification": { | ||
| "incomingTxToast": "Incoming {{amount}} {{unit}}", | ||
| "newTxs_one": "New transaction in: {{accountName}}", | ||
| "newTxs_other": "{{count}} new transactions in: {{accountName}}" |
There was a problem hiding this comment.
if these 2 are not used anymore in the codebase please remove.
| <ToastProvider> | ||
| {children} | ||
| </ToastProvider> | ||
| </WCWeb3WalletProvider> |
There was a problem hiding this comment.
A provider only makes its context available to components inside it in the tree.
So in case we ever want to create a toast message from WCWeb3WalletProvider the ToastProvider would have to be nested as parent.
I haven't checked, but it looks like all others should not need toast, but for WCWeb3WalletProvider it could make sense.
| useEffect(() => { | ||
| const unsubscribers = activeAccounts.map((account) => { | ||
| return syncdone(account.code, () => { | ||
| void (async () => { |
There was a problem hiding this comment.
nit: void doesn't do anything here afaik. remove?
|
|
||
| // Seed known incoming transactions for active accounts so we only toast truly new ones. | ||
| useEffect(() => { | ||
| void Promise.all(activeAccounts.map(account => seedIncomingTransactionsForAccount(account))); |
There was a problem hiding this comment.
nit: void doesn't do anything here afaik. remove?
jadzeidan
force-pushed
the
toast
branch
2 times, most recently
from
3c732f9 to
c633c00
Compare
2 participants
Before asking for reviews, here is a check list of the most common things you might need to consider: