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: ssconf:// deeplinking on ios #1524
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.
LGTM, i wonder shall we put these constants (ss://
, ssconf://
) in a centralized place?
That would be great, but would require a lot of build work - these values need to be interpolated into several plists/config files |
I believe I am not making myself well understood, let's discuss this
offline.
…On Wed, Jan 4, 2023, 18:49 Vinicius Fortuna ***@***.***> wrote:
***@***.**** requested changes on this pull request.
------------------------------
In src/www/app/app.ts
<#1524 (comment)>
:
> @@ -591,7 +591,7 @@ export class App {
private registerUrlInterceptionListener(urlInterceptor: UrlInterceptor) {
urlInterceptor.registerListener(url => {
- if (!url || !unwrapInvite(url).startsWith('ss://')) {
+ if (!url || !(unwrapInvite(url).startsWith('ss://') || unwrapInvite(url).startsWith('ssconf://'))) {
I believe my suggestions are straightforward, and they fix bugs that your
change is not fixing. I already did the work of finding the places to fix
for you.
At a minimum, the two lines I mention above must be fixed. That will be
cleaner if you define IsOutlineAccessKey(). The tests should be
straightforward too, we already have them set up.
We also need to make sure the fixed unwrap is not breaking names for
dynamic keys.
—
Reply to this email directly, view it on GitHub
<#1524 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA4V5VEUSHIPR56WVTZGASLWQYEBHANCNFSM6AAAAAATFYUQSY>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
(just reconfirmed it still works for ssconf:// after the refactor) |
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.
Looks good. Some minor details. You can submit after fixing those.
Co-authored-by: Vinicius Fortuna <fortuna@users.noreply.github.com>
Co-authored-by: Vinicius Fortuna <fortuna@users.noreply.github.com>
Co-authored-by: Vinicius Fortuna <fortuna@users.noreply.github.com>
No description provided.