Skip to content
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

Draft
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

pythcon
Copy link

@pythcon pythcon commented Dec 8, 2024

Please make sure to check the following tasks before opening and submitting a PR

  • I understand and have followed the contribution guide
  • I have tested my changes locally and they are working as intended

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.

Official Server
Custom Server

@Zomatree
Copy link
Member

Zomatree commented Dec 8, 2024

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()
)

Copy link
Author

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.

@pythcon pythcon marked this pull request as draft December 9, 2024 04:55
@pythcon
Copy link
Author

pythcon commented Dec 9, 2024

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.

@Zomatree
Copy link
Member

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.

@pythcon
Copy link
Author

pythcon commented Dec 10, 2024

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?
Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🏗 In Progress
Development

Successfully merging this pull request may close these issues.

2 participants