Feat: Integrate ubeswap savings widget #619#625
Conversation
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The savings widget script is reloaded on every
accountchange but only ever appended (and only to the container div), which can lead to multiple<script>tags being added over time; consider loading it once (empty dependency array) and/or guarding with anid/existence check, and appending todocument.headorbodyinstead of the container. - The
<div id="savings-widget-container">only hosts the script element and does not render a specific web component instance (e.g.,<gooddollar-savings-widget ... />), so if the widget does not self-mount into the container, it may never display; double-check whether explicitly rendering the custom element is required and add it if so.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The savings widget script is reloaded on every `account` change but only ever appended (and only to the container div), which can lead to multiple `<script>` tags being added over time; consider loading it once (empty dependency array) and/or guarding with an `id`/existence check, and appending to `document.head` or `body` instead of the container.
- The `<div id="savings-widget-container">` only hosts the script element and does not render a specific web component instance (e.g., `<gooddollar-savings-widget ... />`), so if the widget does not self-mount into the container, it may never display; double-check whether explicitly rendering the custom element is required and add it if so.
## Individual Comments
### Comment 1
<location path="src/pages/gd/Savings/index.tsx" line_range="37-46" />
<code_context>
+ useEffect(() => {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Avoid reloading the widget script on every account change unless the widget actually depends on it.
Because the effect depends on `account` but the script URL/injection don’t, every account change tears down and reinjects the script, causing extra network/CPU work and potential re‑init bugs. If the widget truly doesn’t depend on `account`, make the dependency array `[]` so it runs only on mount. If it does depend on `account`, prefer passing that via the widget’s own API (e.g., attributes/props) instead of reloading the script itself.
Suggested implementation:
```typescript
useEffect(() => {
```
```typescript
}, [])
```
If the dependency array is not exactly `[account]` (for example `[account, chainId]` or similar), you should update that full dependency array to `[]` instead, so that the widget script is only loaded once on mount and not reloaded on account changes. If the widget actually needs `account` to render correctly, prefer passing `account` into the widget via its attributes/props or configuration object inside the effect, rather than by reloading the script tag itself.
</issue_to_address>
### Comment 2
<location path="src/pages/gd/Savings/index.tsx" line_range="22" />
<code_context>
+ </VStack>
+)
+
+const SavingsWidgetContainer: React.FC = () => {
+ const containerRef = useRef<HTMLDivElement>(null)
+ const [isLoading, setIsLoading] = useState(true)
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the widget container to use a reusable script-loading hook, a single derived status, and NativeBase layout components to simplify control flow and concerns.
You can simplify this component without changing behavior by extracting the script-loading concern, tightening dependencies, and consolidating state.
### 1. Extract script loading into a small hook
This removes DOM manipulation from the page component and makes it reusable/testable:
```ts
// hooks/useExternalScript.ts
import { useEffect, useState } from 'react'
export const useExternalScript = (src: string) => {
const [status, setStatus] = useState<'idle' | 'loading' | 'ready' | 'error'>('idle')
useEffect(() => {
if (!src) return
setStatus('loading')
const script = document.createElement('script')
script.src = src
script.async = true
script.defer = true
script.onload = () => setStatus('ready')
script.onerror = () => setStatus('error')
document.body.appendChild(script)
return () => {
script.onload = null
script.onerror = null
script.remove()
}
}, [src])
return status
}
```
Then your widget container becomes focused on layout:
```tsx
const SavingsWidgetContainer: React.FC = () => {
const containerRef = useRef<HTMLDivElement>(null)
const status = useExternalScript(
'https://unpkg.com/@gooddollar/savings-widget@latest/dist/index.js'
)
const isLoading = status === 'loading'
const hasError = status === 'error'
const isReady = status === 'ready'
// ...rendering below...
}
```
### 2. Remove unnecessary `account` dependency
The script doesn’t depend on `account`, so reloading it on account change adds complexity and flicker. If you keep the current inline approach, you can change:
```ts
useEffect(() => {
// ...
}, [account])
```
to:
```ts
useEffect(() => {
// ...
}, []) // load once per mount
```
### 3. Consolidate loading/error/success handling
Instead of two booleans and inline `display` logic, derive a single `status`:
```tsx
const [status, setStatus] = useState<'loading' | 'error' | 'ready'>('loading')
useEffect(() => {
// on success:
setStatus('ready')
// on error:
setStatus('error')
}, [])
```
Then JSX becomes more linear:
```tsx
{status === 'loading' && (
<VStack alignItems="center" justifyContent="center" minH="300px" w="100%">
<Spinner color="gdPrimary" size="lg" />
<Text fontSize="sm" color="goodGrey.400" mt={4}>
Loading savings widget...
</Text>
</VStack>
)}
{status === 'error' && (
<VStack alignItems="center" justifyContent="center" minH="300px" w="100%">
<Text fontSize="sm" color="red.500" textAlign="center">
Unable to load the savings widget. Please try again later.
</Text>
</VStack>
)}
<Box
as="div"
ref={containerRef}
id="savings-widget-container"
w="100%"
minH="500px"
display={status === 'ready' ? 'flex' : 'none'}
/>
```
### 4. Keep layout consistent with NativeBase
Instead of a raw `div`, use `Box as="div"` to stay within the NativeBase layout system (as above). This keeps styling consistent and avoids mixing paradigms while still giving the web component the `div` it needs.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
1ca3f78 to
22fd77c
Compare
5fe3994 to
52390d5
Compare
Oluwatomilola
left a comment
There was a problem hiding this comment.
@L03TJ3 kindly review
|
@Oluwatomilola your build fails, can you check the build logs |
|
Alright, on it. |
6ccb13d to
0732551
Compare
There was a problem hiding this comment.
Noted. I would remove it.
| <VStack alignItems="center" justifyContent="center" minH="300px" w="100%"> | ||
| <Text fontSize="sm" color="red.500" textAlign="center" mb={4} fontWeight="bold"> | ||
| Unable to Load Savings Widget | ||
| </Text> | ||
| <Text fontSize="xs" color="goodGrey.500" textAlign="center" mb={3}> | ||
| The widget script is not available. To enable this feature: | ||
| </Text> | ||
| <VStack fontSize="xs" color="goodGrey.400" textAlign="left" alignItems="flex-start" space={1}> | ||
| <Text>1. Clone GoodSDKs/packages/savings-widget</Text> | ||
| <Text>2. Run: yarn install && yarn build</Text> | ||
| <Text>3. Copy dist/index.global.js to public/</Text> | ||
| <Text>4. Restart the development server</Text> | ||
| </VStack> | ||
| <Button | ||
| size="sm" | ||
| colorScheme="blue" | ||
| variant="ghost" | ||
| mt={3} | ||
| onPress={() => | ||
| window.open( | ||
| 'https://github.com/GoodDollar/GoodSDKs/tree/main/packages/savings-widget', |
There was a problem hiding this comment.
What is this?
why would this make any sense to show on a user-facing page?
There was a problem hiding this comment.
My bad.
Exposing build/setup instructions in a user-facing page is not appropriate. That was a misstep on my part while trying to handle the missing widget script case.
There was a problem hiding this comment.
Have you read the readme for the savings-widget?
https://github.com/GoodDollar/GoodSDKs/blob/main/packages/savings-widget/README.md
There was a problem hiding this comment.
Yes, I have.
I will read through it again to see what I'm missing.
There was a problem hiding this comment.
revert any changes that don't are not related to the requested change
There was a problem hiding this comment.
Alright, will do that.
| ) | ||
|
|
||
| const SavingsWidgetContainer: React.FC = () => { | ||
| const containerRef = useRef<HTMLDivElement>(null) |
There was a problem hiding this comment.
I had expected you followed the readme at least and how to use it.
there are ways that the iife (the web-component compiled file, not the one from dist)
could have worked.
But you might have had some blockers, and I would have expected them to be raised.
It could have been an easy fix if you would have followed the readme.
You will now have to remove and revert a lot of this.
Please, when things are unclear, ask questions, push draft PRs.
you raised a couple of blockers before but there was never code for me to review.
and 'I have some eslint errors' is really hard to help with.
However. what was also missing was a published npm package.
That is my bad.
I am going to publish it now and you can use add @goodsdks/savings-widget as package. You can look at the demo-webcomponents to see how it can be used: https://github.com/GoodDollar/GoodSDKs/blob/main/apps/demo-webcomponents/src/index.js
The demo loads it high-level in html. once you import from the package you can use the html anywhere in react. (if anything not working, please raise blockers early)
There was a problem hiding this comment.
Thanks for the detailed feedback.
My apologies for deviating from the intended integration approach.
I did follow the README. I cloned the goodSDK quite alright, but I was a bit unclear on the subsequent steps to integrating the component and I indeed had blockers. I should have stated them clearly, my bad.
Moving forward I will raise blockers early, and I’ll make sure to push draft PRs with partial progress next time so issues can be caught earlier.
Please can you add a link to the npm package if it's available.
I’ll go ahead and make the required changes.
There was a problem hiding this comment.
you can add it with yarn: yarn add @goodsdks/savings-widget
0732551 to
4706a65
Compare
|
@Oluwatomilola Hey, what is the status of this PR? |
|
Hi @L03TJ3 I’m currently blocked by a TypeScript issue from the pre-commit checks related to the I’m fixing the typing properly so it aligns with the existing feature flag structure, and I should have everything passing checks shortly. I’ll push an updated commit once that’s resolved. |
Oluwatomilola
left a comment
There was a problem hiding this comment.
@L03TJ3
Update: The savings widget is now integrated on a dedicated page and wired to the existing wallet flow.
The widget script is loaded dynamically, and provider injection is handled via the existing web3 context without modifying shared hooks or feature flags.
Would appreciate a quick review to confirm the integration approach is okay.
b3e79f5 to
8b4f4ff
Compare
|
@Oluwatomilola, are you having difficulty implementing the few changes left? |
|
@pheobeayo I'm done with implementing all the changes. |
pheobeayo
left a comment
There was a problem hiding this comment.
Please can you resolve the conflicts, then we can have a final review and merge.
6575531 to
d7d7a1b
Compare
|
@Oluwatomilola, can you stop force-pushing and closing pull-requests? It's really difficult to keep reviewing like this. |
|
My apologies. I’m running into some git trouble. My local branch and the remote have diverged, so I can’t push my latest commits. I’ve tried pulling, but I’m getting merge conflicts and want to make sure I resolve everything and reopen the PR correctly without messing up the PR history. Could you clarify the best way you’d like me to handle this? Should I merge, rebase, or is there a preferred workflow for resolving these conflicts? Thanks for your patience. |
So sorry about this, Recommended: Rebase onto master # Make sure your local master is up to date
git checkout master
git pull upstream master
# Rebase your feature branch on top of it
git checkout feat-savings-integration
git rebase masterDuring the rebase, if conflicts appear, Git will pause and show you which files conflict. For each one:
git add <resolved-file>
git rebase --continueOnce the rebase is complete, push with: git push origin feat-savings-integration --force-with-lease( Why rebase and not merge? Merging would add a merge commit to the PR history, which makes the diff harder to review and the history messier. Rebase keeps a clean, linear history — which is also easier to squash before merging. After pushing, please also squash your commits into one: git rebase -i HEAD~N # replace N with the number of your commitsIn the interactive editor, mark the first commit as Then push again: git push origin feat-savings-integration --force-with-leaseThe PR will stay open and update automatically once you push — no need to close and reopen it. Let me know if you hit any specific conflict errors and I can help you resolve them.Thanks for flagging this. Here's the cleanest way to handle it: Recommended: Rebase onto master # Make sure your local master is up to date
git checkout master
git pull upstream master
# Rebase your feature branch on top of it
git checkout feat-savings-integration
git rebase masterDuring the rebase, if conflicts appear, Git will pause and show you which files conflict. For each one:
git add <resolved-file>
git rebase --continueOnce the rebase is complete, push with: git push origin feat-savings-integration --force-with-lease( Why rebase and not merge? Merging would add a merge commit to the PR history, which makes the diff harder to review and the history messier. Rebase keeps a clean, linear history — which is also easier to squash before merging. After pushing, please also squash your commits into one: git rebase -i HEAD~N # replace N with the number of your commitsIn the interactive editor, mark the first commit as Then push again: git push origin feat-savings-integration --force-with-leaseThe PR will stay open and update automatically once you push. Or you may have to open another PR, pulling from the origin and pushing to it with your updated files |
|
Thanks a lot for the detailed steps and clear explanation! 🙏 Really appreciate you breaking down the rebase process, it makes things much clearer. I’ll follow your advice and let you know if I hit any snags during the rebase. |
|
@pheobeayo thanks, I've been able to reopen the pr, but I think I lost my previous commits. Do I need to open a new pr with all the needed fixes? |
Apologies about this, you can do that, will be awaiting your PR notification |
Description
This PR integrates the GoodDollar Savings Widget into GoodDapp as a first-class page.
Changes include:
Added a new Savings menu item to the sidebar.
Created a new /savings page using the same layout structure as Bridge/Swap/Claim.
Embedded the GooddollarSavingsWidget via a dedicated wrapper component.
Added contextual FAQs for the Savings feature.
This implements the requirements described in issue #619.
About: https://github.com/GoodDollar/GoodProtocolUI/issues/#619
fixes: #619 #619 #619
How Has This Been Tested?
Ran the app locally using yarn start.
Navigated to the new Savings route from the sidebar.
Verified that the Savings Widget loads correctly.
Confirmed layout consistency with Bridge/Swap/Claim pages.
Manually tested wallet connection and widget rendering.
Checklist:
Summary by Sourcery
Add a dedicated Savings page integrating the GoodDollar savings widget with existing app layout and wallet context.
New Features:
Enhancements: