Fix profile side-panel close behavior for activate and purchase flows#2016
Merged
Fix profile side-panel close behavior for activate and purchase flows#2016
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughComponents now read typed route params ( Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
cursor
bot
changed the title
alexprate
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/epics/src/people/components/activate-spaces-form.tsx`:
- Around line 126-128: Replace the router.push call inside the timeout with
router.replace to avoid adding a new history entry and prevent the browser Back
button from reopening the completed form; specifically update the setTimeout
callback that currently calls router.push(closePanelUrl) (refer to
closePanelTimeoutRef and closePanelUrl) to call router.replace(closePanelUrl)
instead while leaving the timeout logic intact.
In `@packages/epics/src/people/components/people-purchase-hypha-tokens.tsx`:
- Around line 141-151: The try block currently mixes the successful purchase
flow (investInHypha -> setShowSuccessMessage -> arming closePanelTimeoutRef and
router.push) with the post-purchase refresh (manualUpdate), so any error in
manualUpdate surfaces as a purchase failure; to fix, move the manualUpdate call
out of the purchase try/catch into its own try/catch after the success path so
that investInHypha, setShowSuccessMessage, form.reset, and the timer
(closePanelTimeoutRef + router.push) always run on success, and handle/log
errors from manualUpdate separately (use manualUpdate in a separate try/catch
and do not rethrow to avoid changing the user-facing success state).
- Around line 147-149: The timeout handler currently uses router.push to
navigate (closePanelTimeoutRef.current setTimeout calling
router.push(closePanelUrl)), which creates a new history entry; change that call
to router.replace(closePanelUrl) so the navigation replaces the current history
entry and prevents the Back button from reopening the purchase panel (update the
setTimeout callback where closePanelTimeoutRef.current is assigned to use
router.replace instead of router.push).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9fee8f46-3a6c-4c30-ae16-77639609ff52
📒 Files selected for processing (2)
packages/epics/src/people/components/activate-spaces-form.tsxpackages/epics/src/people/components/people-purchase-hypha-tokens.tsx
packages/epics/src/people/components/people-purchase-hypha-tokens.tsx
Outdated
Show resolved
Hide resolved
cursor
bot
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/epics/src/people/components/people-purchase-hypha-tokens.tsx (1)
54-77: 🧹 Nitpick | 🔵 TrivialExtract the delayed side-panel close flow into a shared hook.
This is now the second copy of the same route construction, timeout cleanup, and delayed
replace()scheduling added in this PR. The earlier follow-up fixes already had to touch both copies, so this will drift again the next time the close behavior changes.♻️ Refactor sketch
- const router = useRouter(); - const { lang } = useParams<{ lang: string }>(); - const closePanelTimeoutRef = useRef<ReturnType<typeof setTimeout> | null>( - null, - ); - const closePanelUrl = useMemo( - () => `/${lang}/profile/${personSlug}`, - [lang, personSlug], - ); + const { scheduleClosePanel } = useDelayedProfilePanelClose({ personSlug }); ... - if (closePanelTimeoutRef.current) { - clearTimeout(closePanelTimeoutRef.current); - } - closePanelTimeoutRef.current = setTimeout(() => { - router.replace(closePanelUrl); - }, 3000); + scheduleClosePanel();function useDelayedProfilePanelClose({ personSlug, delayMs = 3000, }: { personSlug: string; delayMs?: number; }) { const router = useRouter(); const { lang } = useParams<{ lang: string }>(); const timeoutRef = useRef<ReturnType<typeof setTimeout> | null>(null); useEffect(() => { return () => { if (timeoutRef.current) clearTimeout(timeoutRef.current); }; }, []); return { scheduleClosePanel() { if (timeoutRef.current) clearTimeout(timeoutRef.current); timeoutRef.current = setTimeout(() => { router.replace(`/${lang}/profile/${personSlug}`); }, delayMs); }, }; }As per coding guidelines "Components are composable and follow single-responsibility principle".
Also applies to: 182-188, 221-225
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/epics/src/people/components/people-purchase-hypha-tokens.tsx` around lines 54 - 77, Extract the delayed side-panel close logic (the timeout ref closePanelTimeoutRef, closePanelUrl construction, and the delayed router.replace scheduling currently in this component) into a shared hook named useDelayedProfilePanelClose that accepts personSlug and optional delayMs; inside the hook use useRouter and useParams to build the route and keep a timeoutRef with useEffect cleanup, and expose a scheduleClosePanel() method that clears any existing timeout and sets a new setTimeout to call router.replace(`/${lang}/profile/${personSlug}`) after delayMs; then replace usages of closePanelTimeoutRef and direct router.replace calls in this file (and the other occurrences noted) to call useDelayedProfilePanelClose(...).scheduleClosePanel() and remove duplicated URL/timeout code from the component.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/epics/src/people/components/people-purchase-hypha-tokens.tsx`:
- Around line 224-225: The router.replace call currently used inside the
setTimeout (referenced by closePanelTimeoutRef.current and closePanelUrl)
scrolls the page to top; modify the router.replace invocation in
people-purchase-hypha-tokens (inside the setTimeout callback) to pass { scroll:
false } as the second argument (i.e., router.replace(closePanelUrl, undefined, {
scroll: false })) so the page preserves scroll position when the panel closes,
and apply the same change to the analogous router.replace call in
activate-spaces-form (the closePanelTimeoutRef/current setTimeout callback
referencing closePanelUrl).
---
Outside diff comments:
In `@packages/epics/src/people/components/people-purchase-hypha-tokens.tsx`:
- Around line 54-77: Extract the delayed side-panel close logic (the timeout ref
closePanelTimeoutRef, closePanelUrl construction, and the delayed router.replace
scheduling currently in this component) into a shared hook named
useDelayedProfilePanelClose that accepts personSlug and optional delayMs; inside
the hook use useRouter and useParams to build the route and keep a timeoutRef
with useEffect cleanup, and expose a scheduleClosePanel() method that clears any
existing timeout and sets a new setTimeout to call
router.replace(`/${lang}/profile/${personSlug}`) after delayMs; then replace
usages of closePanelTimeoutRef and direct router.replace calls in this file (and
the other occurrences noted) to call
useDelayedProfilePanelClose(...).scheduleClosePanel() and remove duplicated
URL/timeout code from the component.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b83ef887-3b34-4cd8-9952-91d64d902907
📒 Files selected for processing (2)
packages/epics/src/people/components/activate-spaces-form.tsxpackages/epics/src/people/components/people-purchase-hypha-tokens.tsx
packages/epics/src/people/components/people-purchase-hypha-tokens.tsx
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/epics/src/people/components/activate-spaces-form.tsx (1)
151-158: 🧹 Nitpick | 🔵 TrivialConsider adding
formto the dependency array for completeness.The effect uses
form.getValuesandform.setValue, but the dependency array only includesperson?.address. While theformobject from react-hook-form is stable in practice, ESLint's exhaustive-deps rule would flag this.♻️ Proposed fix
useEffect(() => { if (person?.address) { const currentBuyer = form.getValues('buyer'); if (currentBuyer !== person.address) { form.setValue('buyer', person.address); } } - }, [person?.address]); + }, [person?.address, form]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/epics/src/people/components/activate-spaces-form.tsx` around lines 151 - 158, The useEffect reads and writes via form.getValues and form.setValue while only depending on person?.address; add the form reference to the dependency array (i.e. change dependencies from [person?.address] to [person?.address, form]) so the effect includes the react-hook-form instance used inside useEffect; if form is not stable, wrap or memoize the form instance where it is created (or otherwise ensure stability) before adding it to the dependency list.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/epics/src/people/components/activate-spaces-form.tsx`:
- Around line 151-158: The useEffect reads and writes via form.getValues and
form.setValue while only depending on person?.address; add the form reference to
the dependency array (i.e. change dependencies from [person?.address] to
[person?.address, form]) so the effect includes the react-hook-form instance
used inside useEffect; if form is not stable, wrap or memoize the form instance
where it is created (or otherwise ensure stability) before adding it to the
dependency list.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 94736afd-43c5-434f-ad06-c8e20b76c886
📒 Files selected for processing (2)
packages/epics/src/people/components/activate-spaces-form.tsxpackages/epics/src/people/components/people-purchase-hypha-tokens.tsx
Co-authored-by: webguru-hypha <webguru-hypha@users.noreply.github.com>
Co-authored-by: webguru-hypha <webguru-hypha@users.noreply.github.com>
Co-authored-by: webguru-hypha <webguru-hypha@users.noreply.github.com>
Co-authored-by: webguru-hypha <webguru-hypha@users.noreply.github.com>
Co-authored-by: webguru-hypha <webguru-hypha@users.noreply.github.com>
Co-authored-by: webguru-hypha <webguru-hypha@users.noreply.github.com>
plitzenberger
force-pushed
the
cursor/issue-1938-investigation-0146
branch
from
4281164 to
9298d01
Compare
Member
Autofix / review thread resolvedThe CodeRabbit thread about passing Both call sites use The review thread has been resolved in the GitHub UI. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
router.replace(...)for delayed panel close navigation so browser Back does not reopen completed formsrouter.replace(..., { scroll: false })manualUpdate()refresh errors from purchase transaction errors to avoid showing false purchase failuresformat:checkmainwhile preserving both:Commits addressing review comments
da3e288f5—fix(profile): replace history entry after activate spaces success978255b9e—fix(profile): isolate post-purchase asset refresh errors4a390dd76—fix(profile): replace history entry after token purchase success7deb23035—style(profile): apply prettier formatting for CI2ba6131b5—merge: resolve main branch conflicts in profile activation/purchase flows428116476—fix(profile): preserve scroll when auto-closing side panelsValidation
pnpm run format:check✅Issues
Summary by CodeRabbit
Improvements
Bug Fixes