-
-
Notifications
You must be signed in to change notification settings - Fork 10
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
Feature: 3rd-Party Server URLs #7
base: main
Are you sure you want to change the base?
Conversation
Hi thanks for the PR, most of this looks good although please try refrain from editing code which is not relevant to the feature your adding. The logic for the API URL doesn't work in practice as not all instances have the API at /api, even the official instance has it under a subdomain currently, although it's being migrated to /api, the field is going to need to take the API URL itself unless we add a mechanism to link to the API from the root, maybe an RFC could be created for a special meta tag or a file under .well-known. In the future we want to be able to login to multiple account simultaneously and have all the servers show up together so how this code will stand going forward is unknown however I'm fine with having it in the codebase ATM. |
@@ -338,9 +347,14 @@ class UserSettingsData { | |||
mfaStatus: try await state.http.fetchMFAStatus().get() | |||
) | |||
|
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 change was added due to an infinite loop in waiting for notifications. When logging into the official server or a 3rd party, the request would make it to the API, but the user would never get the "Accept Notifications" prompt.
The default behavior has been changed to handle the custom url/domain directly. If that fails, it will also check the /api endpint as well to adhere to the self-hosted guide configuration. This no longer hardcodes that /api endpoint, but adds a check that is easy for the end-user. If the domain fails, and the domain + /api, succeeds, it will auto-update for the user. API validation was also implemented. |
This PR is looking great however this won't get merged currently due to the fact that official branding will still be used for 3rd party instances, the revolt team as a whole is still undecided on the best way to deal with this as this requires a way for the api to serve the assets and for them to be loaded dynamically into the app or an alternative solution. I'm happy to leave this PR open until then if you want to work on it in the future or I can put this into a branch so I can finish this instead. |
Hi @Zomatree! Thank you for the review. Please keep me updated on what the Revolt team decides on this issue. If we need to remove Revolt branding from 3rd-party instances, I would be happy to add those changes to this PR. You can leave this PR open for now, I will continue to make refinements. I will also be happy to contribute to the macOS version of the application to add this new feature. Any ETA on when the Revolt team will make a decision on this? |
Please make sure to check the following tasks before opening and submitting a PR
3rd-Party Server URLs are now supported on this version of the application. The default is set to the official Revolt servers, but the user can press a new button to change it to their own. A new field was added to the Account Settings page to list the server that the user is currently connected to. This prevents user confusion of where they are signed into.
3rd-Party Server URLs are validated upon the user un-focusing from the text box as well as when the button is pressed to the right of the textbox. Server URLs are now stored in persistent storage. The default behavior of a 3rd-Party server url is to take the user's entry and add /api at the end to adhere to the self-hosted default configuration.