Fix donations page layout shell

[schjonhaug]

Summary

• wrap the donations page in the shared app shell used by other routes
• restore the expected horizontal padding for the shared header and footer on /donations

Testing

• not run (layout-only change)

[github-actions]

🤖 Codex Code Review

Findings

1. Low - Missing regression test coverage for the new route layout composition (header/footer/banner moved from page to donations/layout.tsx).
   • Changed code moves structural UI responsibility from page to layout, but no corresponding test updates were included.
   • Risk: silent regressions like duplicate/missing header/footer/banner or spacing/layout regressions on /donations.
   • References: layout.tsx:5, page.tsx:17

What I checked in changed code

• No obvious new security issues in the modified lines (target="_blank" correctly uses rel="noopener noreferrer").
• No performance concerns beyond normal static UI rendering.
• Code quality is generally clean for this refactor.

Coverage assessment

• Test coverage appears incomplete for this PR’s behavior change (layout extraction + banner/header/footer placement). A route render test for /donations would close that gap.

[github-actions]

🤖 Gemini Code Review

Here is a review of the provided pull request changes:

🌟 Code Quality & Best Practices

• Excellent Next.js App Router Usage: Moving the AppHeader and AppFooter (along with the new DemoBanner) out of the page component and into layout.tsx is a great architectural move. This leverages Next.js's layout system effectively.
• Separation of Concerns: The page.tsx is now purely focused on its primary responsibility—rendering the donations content—making it cleaner and easier to read.
• Smart Tailwind Adjustments: You correctly removed the horizontal padding (px-4) from the inner container in page.tsx since the new layout wrapper (<div className="max-w-6xl mx-auto px-4 py-8">) already provides the necessary horizontal padding, avoiding double-padding issues.
• Suggestion (Semantic HTML): Consider wrapping the {children} prop inside layout.tsx with a <main> tag to improve accessibility and semantic structure (assuming your header and footer use <header> and <footer> elements).

  return (
    <div className="max-w-6xl mx-auto px-4 py-8">
      <DemoBanner />
      <AppHeader />
      <main className="flex-1">{children}</main>
      <AppFooter />
    </div>
  )

🐛 Potential Bugs or Issues

• No apparent bugs: The types, imports, and component structures look correct and align with modern Next.js/React standards. The transition from fragments (<>...</>) to a clean DOM tree is handled well.
• Architecture Check: Just ensure that AppHeader and AppFooter aren't already being rendered in a parent layout (like src/app/layout.tsx). If they are, you might end up with duplicate headers/footers. If they are meant to be injected purely on a per-route group basis, this code is perfectly fine.

⚡ Performance Considerations

• Improved Client-Side Nav: By utilizing Next.js Layouts, the DemoBanner, AppHeader, and AppFooter will not unmount and remount when users navigate between sub-routes within /donations (if you add any in the future). This leads to slightly better performance and preserves state within those components during navigation.

🔒 Security Concerns

• None. This is purely a presentational and structural UI refactor with no security implications.

🧪 Test Coverage

• Component Tests: If you have unit/component tests (e.g., Jest + React Testing Library) that mount DonationsPage and assert the presence of the AppHeader or AppFooter, those tests will now fail. You will need to either test the DonationsLayout component directly or update the page tests to no longer expect the header and footer.
• E2E Tests: End-to-end tests (like Cypress or Playwright) should remain completely unaffected since they assert against the fully rendered page in the browser.

[github-actions]

🤖 Claude Code Review

Here is my review of this pull request, which covers two distinct areas: dev script modularization and frontend error handling/i18n improvements.

---

PR Review

Summary of Changes

1. scripts/dev.sh — Refactored a 2500-line monolithic script into a clean ~120-line dispatcher backed by 7 library modules in scripts/lib/
2. Frontend error handling — Replaced hardcoded English error strings with ApiError-aware, translated error messages across several components
3. Donations layout — Extracted AppHeader/AppFooter into a dedicated layout.tsx
4. i18n — Added missing translation keys for failure states across all 9 locale files

---

Code Quality & Best Practices

scripts/dev.sh modularization — Excellent

The refactor is well-executed. The dispatch table in dev.sh is now readable at a glance, and the library sourcing loop with a guard is a good pattern:

for lib in "${required_libs[@]}"; do
    if [ ! -f "$lib_path" ]; then
        echo "Missing required library: $lib_path" >&2
        exit 1
    fi
    source "$lib_path"
done

The old code had a subtle bug where cpfp_for_wallet redefined btc_wallet inside its own body, shadowing the outer function. This is cleanly resolved by the refactor.

balance-alerts-list.tsx — submitAlert extraction

The submitAlert helper correctly eliminates the duplicated post-creation logic (setLocalAlerts, setShowCreateForm, setThresholdInput). One minor note: submitAlert is defined after the two handler functions that call it. While JavaScript hoisting covers this for function declarations, this is an arrow function assigned to a const, so the ordering only works because it's called inside async handlers that execute later — not at definition time. Consider moving submitAlert above its callers for clarity.

donations/layout.tsx — Alignment concern

The layout wrapper uses max-w-6xl, while the page content uses max-w-2xl. This is intentional (layout owns the chrome, page controls its own content width), but it means the AppHeader/AppFooter span up to 6xl while the donations content is constrained to 2xl. This is consistent with other pages and fine as-is, just worth confirming it matches the design intent.

---

Potential Bugs or Issues

inline-wallet-name-edit.test.tsx — Fallback path only

The updated test "shows translated error message when update fails" throws a plain new Error('Update failed'), which hits the else branch (not ApiError), and expects t('edit.failed') = "Failed to update wallet name". The test name says "translated" but it's actually testing the fallback path. There's no test covering the ApiError path for wallet name updates. Not a blocking issue, but worth adding a case.

contact-modal.tsx — Non-API error loses detail

// Before
setError(err instanceof Error ? err.message : `Failed to ${isEditMode ? 'update' : 'create'} contact`)

// After
setError(t('form.saveFailed'))

The else branch here handles non-ApiError throws (e.g., unexpected JS errors). The old code surfaced the actual error message; the new code always shows a generic string. For development visibility this could hide useful context. Low severity for production UX, but consider whether real errors from the network layer (which should be ApiError) could ever reach this branch.

---

Performance Considerations

No performance concerns introduced. The submitAlert extraction is a minor but correct deduplication. The layout changes are cosmetic.

---

Security Concerns

The shift from err.message → getTranslatedApiError is a security improvement: it prevents raw backend error messages (which could contain internal details) from being shown directly to users. The getTranslatedApiError function properly falls back through a defined hierarchy rather than leaking stack traces or server internals.

The getTranslatedApiError fallback detection:

if (translated !== `errors.api.${err.errorCode}` && translated !== err.errorCode)

This relies on next-intl returning the key path string when a key is missing. This is a known fragility (behavior could differ between next-intl versions), but it's pre-existing and not introduced by this PR.

---

Test Coverage

Good:

• contact-modal.test.tsx correctly updated to throw ApiError (with error_code: 'duplicate_notification_targets') and assert translated text (/already in use/) rather than the raw backend message
• inline-wallet-name-edit.test.tsx mock correctly preserves ApiError from the real module via jest.requireActual

Gap:

• No test added for delete-wallet-modal.tsx or delete-contact-modal.tsx error handling changes
• submitAlert extraction has no direct unit test (covered indirectly through existing handler tests)

---

Minor Nits

• scripts/lib/helpers.sh: The is_wallet_name() function maintains a static hardcoded list of wallet names. If new wallets are added in init.sh, this list must be updated in sync — easy to forget. A comment noting this dependency would help.
• The # shellcheck disable=SC1090 comment in dev.sh is appropriate for the dynamic source, good call.
• Translation keys were added consistently across all 9 locale files — well done.

---

Overall Assessment

This is a solid, well-scoped PR. The dev.sh modularization is the most impactful change and is cleanly done. The frontend changes are consistent incremental improvements. The one item worth addressing before merge is the test gap on the delete-*-modal components — those changes are real but untested.