Feat: Integrate ubeswap savings widget #619

[Oluwatomilola]

Description

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

About #619

How Has This Been Tested?

Please describe the tests that you ran to verify your changes.

Checklist:

• PR title matches follow: (Feature|Bug|Chore) Task Name
• My code follows the style guidelines of this project
• I have followed all the instructions described in the initial task (check Definitions of Done)
• I have performed a self-review of my own code
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes
• I have added reference to a related issue in the repository
• I have added a detailed description of the changes proposed in the pull request. I am as descriptive as possible, assisting reviewers as much as possible.
• I have added screenshots related to my pull request (for frontend tasks)
• I have pasted a gif showing the feature.
• @mentions of the person or team responsible for reviewing proposed changes

[sourcery-ai]

Hey - I've found 2 issues, and left some high level feedback:

• 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.

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>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[sourcery-ai]

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:

    useEffect(() => {

    }, [])

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.

[sourcery-ai]

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:

// 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:

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:

useEffect(() => {
  // ...
}, [account])

to:

useEffect(() => {
  // ...
}, []) // load once per mount

3. Consolidate loading/error/success handling

Instead of two booleans and inline display logic, derive a single status:

const [status, setStatus] = useState<'loading' | 'error' | 'ready'>('loading')

useEffect(() => {
  // on success:
  setStatus('ready')
  // on error:
  setStatus('error')
}, [])

Then JSX becomes more linear:

{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.