-
Notifications
You must be signed in to change notification settings - Fork 464
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: redirect to old deployment for legacy Safes #1096
Conversation
ESLint Summary View Full Report
Report generated by eslint-plus-action |
Deploying with Cloudflare Pages
|
@@ -37,6 +39,7 @@ describe('useSafeNotifications', () => { | |||
implementation: { value: '0x123' }, | |||
implementationVersionState: 'OUTDATED', | |||
version: '1.1.1', | |||
chainId: '1', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's a leftover
src/hooks/useSafeNotifications.ts
Outdated
href: { | ||
link: { | ||
href: isOldSafe | ||
? `https://gnosis-safe.io/app/${chain?.shortName}:${safeAddress}/settings/details?no-redirect=true` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make the old URL a constant. The safe query param can be taken from the router instead of concatenating chainId and safeAddress.
src/hooks/useSafeNotifications.ts
Outdated
}), | ||
) | ||
|
||
return () => { | ||
dispatch(closeNotification({ id })) | ||
} | ||
}, [dispatch, implementationVersionState, version, query.safe]) | ||
}, [dispatch, implementationVersionState, version, query.safe, safeAddress]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
safeAddress can be removed now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Tested, and it works. ✅ |
What it solves
Notification link for legacy Safes
How this PR fixes it
Instead of linking to the CLI, we link to the upgrade page of the
safe-react
deployment for older Safes not supported byweb-core
.How to test it
Open a <1.1.1 Safe and observe the new link in the notification.
Note: redirection on
safe-react
will still happen until we merge this.