-
Notifications
You must be signed in to change notification settings - Fork 21
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
feat: handle alias redirection #1121
Conversation
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.
Copilot reviewed 2 out of 3 changed files in this pull request and generated no comments.
Files not reviewed (1)
- apps/ui/src/helpers/aliases.json: Language not supported
Comments suppressed due to low confidence (1)
apps/ui/src/routes/index.ts:43
- Ensure that the new redirection logic for handling aliases is covered by tests.
if (to.matched[0]?.name === 'space') {
@@ -25,6 +25,7 @@ export const spaceChildrenRoutes: RouteRecordRaw[] = [ | |||
component: SpaceEditor | |||
}, | |||
{ path: '', name: 'space-overview', component: SpaceOverview }, | |||
{ path: 'about', redirect: { name: 'space-overview' } }, |
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.
Is this needed? This already direct me to space overview: https://snapshot.org/#/fabien.eth/about
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.
This was handled in the previous code (which is changed now to make it more readable)
redirectPath = `/${metadataNetwork}:${domain}`;
if (rest && !/^\/about$/.test(rest)) {
redirectPath += rest;
}
// Match and redirect paths like "/safe.eth/settings" to "/s:safe.eth/settings" | ||
const domainMatch = to.path.match(/^\/([^:\/]+?\.[^:\/]+)(\/.*)?$/); | ||
if (domainMatch) { | ||
const domain = domainMatch[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.
Can't we just replace the domain here and not change the whole logic? There is plenty of case to test if we change the logic.
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.
Here we match only if the space doesn't have s:
for example URLs like /safe.eth/settings
,
Updated code handles both URLs with s:safe.eth
and safe.eth
and redirects to new aliased space
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'm a bit afraid of rewriting the logic like this, it might be hard to find out if we are breaking anything.
Do we need to redirect safe.eth
to s:safe.eth
? Space should have single canonical URL (aside from hardcoded redirects).
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.
Do we need to redirect safe.eth to s:safe.eth
Yes, else all existing URLs will break, users reference these URLs on their forums or blogs or Twitter, etc.
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 have tested most of the cases (also mentioned them in the description)
Also, this is not a complete rewrite but adds one more case to handle aliases and moving /about
to routes instead of handling it in this function
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'm mostly concerned by whitelist functionality as this code was used for this as well.
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.
Whitelist is broken:
WHITE_WHITE_LABEL_MAPPING='127.0.0.1;balancer.eth yarn dev
It redirects to http://127.0.0.1:8080/#/s:
Thanks for catching this, It was not handled in this guard before as well so I missed it. now we ignore this gaurd if it is a whitelabel domain |
@wa0x6e would appreciate if you could take a look to see if it doesn't break anything whitelabel related. |
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.
tACK
Looks good to me and it seems to work, but not sure if all edge cases for whitelist are handled. Would be good to get approval from @wa0x6e for this.
Summary
Closes: https://github.com/snapshot-labs/workflow/issues/369
/about
should redirect to the space overview page to handle v1's URLsHow to test