feat(ui): update homepage hero to execution-layer framing (#100)#263
Conversation
|
@collinsezedike is attempting to deploy a commit to the ezedikeevan's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
@collinsezedike Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
ezedike-evan
left a comment
There was a problem hiding this comment.
The hero copy changes for app/page.tsx are exactly right and the four tests in HomePage.test.tsx are well written. But this PR bundles significant scope that wasn't asked for in #100, and some of that extra scope has real correctness issues. This can't merge as-is.
1. PR scope far exceeds issue #100 — blocking
Issue #100 says: "Estimated File Changes: 1 (app/page.tsx)". The PR touches 11 files and introduces:
- A new
lib/session.tsmodule (JWT nonce/sessionStorage utilities) - A full refactor of
WalletContext.tsx(WatchWalletChangeslazy-import + polling fallback) - A near-complete rewrite of
hooks/useFreighter.ts(two-phase polling + event listeners) - A mainnet network guard in
RateTable.tsxandapp/offramp/page.tsx - URL-based tracking state rehydration in
app/offramp/page.tsx
These changes are individually meaningful but they are not what #100 asks for. They belong in their own PRs, each linked to their own issues, with their own descriptions and test plans. Please split accordingly:
- Keep
app/page.tsx+tests/components/HomePage.test.tsx+ snapshot in this PR. - Open a separate PR for the session / URL-rehydration work (
lib/session.ts+ offramp page changes). - Open a separate PR for the
WalletContext/useFreighterrefactor. - Open a separate PR for the mainnet guard in
RateTable.
2. useFreighter.ts rewrite has a dangling onVisibilityDetect listener — correctness bug
function onVisibilityDetect() {
if (!cancelled) void detect()
}
// ...
earlyTimeoutId = setTimeout(() => {
// ...
window.addEventListener('focus', onVisibilityDetect)
document.addEventListener('visibilitychange', onVisibilityDetect)
}, EARLY_POLL_DURATION_MS)The cleanup function runs window.removeEventListener('focus', onVisibilityDetect) unconditionally. But the listeners are only registered inside the timeout callback, which may not have fired yet when the component unmounts (e.g. during SSR test or fast navigation). That's fine in that direction. The real bug: if the component unmounts after the timeout fires, cancelled = true is set, but onVisibilityDetect still holds a reference to the outer scope's cancelled variable and correctly checks it — so no state update occurs. This is actually OK. However, the earlyTimeoutId is cleared but if the timeout has already fired, that clearTimeout is a no-op and the event listeners remain attached until the next mount/unmount, which can't remove them because the function reference is lost (the onVisibilityDetect is a closure created in the previous effect invocation). The event listeners will leak across re-mounts unless the reference is stable. Move onVisibilityDetect outside the useEffect or capture the reference in a ref so the cleanup can remove the correct instance.
3. lib/session.ts JWT stored in sessionStorage without any expiry — security concern
The JWT written by saveJwtToSession is keyed only by nonce and has no TTL. If the anchor JWT has a 24-hour validity and the user leaves the tab open, a stale JWT could be replayed. Consider writing the expiry timestamp alongside the JWT and rejecting it in loadJwtFromSession if expired. At a minimum, document the non-expiry as a known limitation in a comment.
4. Missing CHANGELOG.md entry for the hero copy change
The checklist box for user-facing behaviour → CHANGELOG is unchecked. A hero copy update is user-visible; it needs an [Unreleased] entry.
5. Checklist mostly unchecked
Please complete the checklist for the items that are in scope for this PR.
To summarise: the app/page.tsx copy + HomePage tests are ready to merge as soon as they're in a clean, focused PR. Please split the out-of-scope changes into separate PRs and re-open this one with only the copy work.
27a69cc to
3755227
Compare
ezedike-evan
left a comment
There was a problem hiding this comment.
Sprint re-review — changes requested
1. Merge conflict — mergeable: CONFLICTING (must fix before anything else)
The branch feat/100-homepage-hero-copy is conflicting with main. This PR cannot be merged until the author rebases onto the current main and resolves all conflicts. Run:
```
git fetch origin
git rebase origin/main
resolve conflicts, then:
git push --force-with-lease
```
Note from last sprint: the previous review flagged scope concerns about the 10 extra files that were outside the 1-file scope of this issue. This PR has since been narrowed down to 4 files (app/page.tsx, CHANGELOG.md, tests/components/HomePage.test.tsx, and the snapshot) which is a clean, focused scope — that issue is resolved.
2. "Linked issue" template section left as Closes # with no issue number
The PR body contains the raw template placeholder:
Closes #
The top-of-body line correctly says Closes #100, but the "Linked issue" section inside the template body was not filled in (it still reads Closes #). Remove or complete that section to avoid ambiguity.
3. Snapshot committed to the repo
tests/components/__snapshots__/HomePage.test.tsx.snap (224 lines) is committed into version control. Vitest/Jest snapshots are typically gitignored or treated as automatically regenerated artefacts. Check .gitignore — if snapshots are not listed there, add the tests/components/__snapshots__/ directory. If the team intentionally tracks them, leave as-is, but be aware that any future page.tsx change will require a snapshot update commit.
Fix the rebase conflict first; the other two items can be addressed in the same push.
3755227 to
4ed48e4
Compare
Summary
tests/components/HomePage.test.tsxwith 4 tests (hero copy, subcopy, CTA link, snapshot)Test plan
/offrampand is the sole CTAnpx vitest run tests/components/HomePage.test.tsx— 4 tests passCloses #100