Skip to content

Commit 5808d70

Browse files
refactor(register): improve code quality across registration flow
- Validate URL scheme in normalize_custom_homeserver (reject ftp://, file://, etc.) - Add RegisterStatusModal helper methods (set_title, set_status, button_ref) following the LoginStatusModal pattern, replacing direct widget tree access - Extract update_homeserver_option_buttons() to consolidate radio button state - Initialize selected_homeserver via LiveHook::after_new_from_doc instead of lazy init in handle_actions - Suppress redundant popup for TokenRequired errors (token input field is sufficient feedback) - Stop storing username/password in Cli struct; use RegisterRequest directly - Improve token error detection with structured UIAA error checking before falling back to string matching - Reduce spawn_sso_server complexity by extracting post_pending/post_status/ post_failure closures, eliminating 6 duplicated if-else branches
1 parent 5c835fe commit 5808d70

4 files changed

Lines changed: 158 additions & 102 deletions

File tree

src/register/register_screen.rs

Lines changed: 35 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@ use url::Url;
5656
use crate::sliding_sync::{submit_async_request, MatrixRequest, RegisterRequest};
5757
use crate::login::login_screen::LoginAction;
5858
use super::{
59-
register_status_modal::RegisterStatusModalAction,
59+
register_status_modal::{RegisterStatusModalAction, RegisterStatusModalWidgetExt},
6060
validation::{
6161
MIN_PASSWORD_CHARS, needs_custom_homeserver_input, normalize_custom_homeserver,
6262
normalize_username, password_has_min_chars,
@@ -409,7 +409,7 @@ live_design! {
409409
}
410410
}
411411

412-
#[derive(Live, LiveHook, Widget)]
412+
#[derive(Live, Widget)]
413413
pub struct RegisterScreen {
414414
#[deref]
415415
view: View,
@@ -427,6 +427,12 @@ pub struct RegisterScreen {
427427
registration_token_required: bool,
428428
}
429429

430+
impl LiveHook for RegisterScreen {
431+
fn after_new_from_doc(&mut self, _cx: &mut Cx) {
432+
self.selected_homeserver = "matrix.org".to_string();
433+
}
434+
}
435+
430436
impl RegisterScreen {
431437
fn toggle_homeserver_options(&mut self, cx: &mut Cx) {
432438
self.is_homeserver_editing = !self.is_homeserver_editing;
@@ -436,6 +442,20 @@ impl RegisterScreen {
436442
self.redraw(cx);
437443
}
438444

445+
/// Updates the radio-style homeserver option buttons to reflect the current selection.
446+
fn update_homeserver_option_buttons(&self, cx: &mut Cx) {
447+
let is_matrix_org = !self.pending_custom_homeserver;
448+
let matrix_btn = self.view.button(ids!(matrix_option));
449+
let other_btn = self.view.button(ids!(other_option));
450+
if is_matrix_org {
451+
matrix_btn.set_text(cx, "● matrix.org");
452+
other_btn.set_text(cx, "○ Other homeserver");
453+
} else {
454+
matrix_btn.set_text(cx, "○ matrix.org");
455+
other_btn.set_text(cx, "● Other homeserver");
456+
}
457+
}
458+
439459
fn show_warning(&self, message: &str) {
440460
use crate::shared::popup_list::{enqueue_popup_notification, PopupItem, PopupKind};
441461
enqueue_popup_notification(PopupItem {
@@ -521,10 +541,7 @@ impl RegisterScreen {
521541
.set_visible(cx, false);
522542

523543
// Reset homeserver option buttons
524-
let matrix_option_button = self.view.button(ids!(matrix_option));
525-
let other_option_button = self.view.button(ids!(other_option));
526-
matrix_option_button.set_text(cx, "● matrix.org");
527-
other_option_button.set_text(cx, "○ Other homeserver");
544+
self.update_homeserver_option_buttons(cx);
528545

529546
// Clear input fields
530547
self.view.text_input(ids!(username_input)).set_text(cx, "");
@@ -574,12 +591,6 @@ impl MatchEvent for RegisterScreen {
574591
let edit_button = self.view.button(ids!(edit_button));
575592
let sso_button = self.view.button(ids!(sso_button));
576593

577-
// Initialize selected_homeserver if empty
578-
if self.selected_homeserver.is_empty() {
579-
self.selected_homeserver = "matrix.org".to_string();
580-
self.pending_custom_homeserver = false;
581-
}
582-
583594
if login_button.clicked(actions) {
584595
cx.action(RegisterAction::NavigateToLogin);
585596
}
@@ -596,10 +607,9 @@ impl MatchEvent for RegisterScreen {
596607
self.update_button_mask(&sso_button, cx, 1.0);
597608

598609
// Show SSO registration modal immediately
599-
let status_label = self.view.label(ids!(status_modal_inner.status));
600-
status_label.set_text(cx, "Opening your browser...\n\nPlease complete registration in your browser, then return to Robrix.");
601-
let cancel_button = self.view.button(ids!(status_modal_inner.cancel_button));
602-
cancel_button.set_text(cx, "Cancel");
610+
let status_modal_inner = self.view.register_status_modal(ids!(status_modal_inner));
611+
status_modal_inner.set_status(cx, "Opening your browser...\n\nPlease complete registration in your browser, then return to Robrix.");
612+
status_modal_inner.button_ref().set_text(cx, "Cancel");
603613
self.view.modal(ids!(status_modal)).open(cx);
604614
self.redraw(cx);
605615

@@ -629,8 +639,7 @@ impl MatchEvent for RegisterScreen {
629639
.set_visible(cx, false);
630640

631641
// Update button styles to show selection
632-
matrix_option_button.set_text(cx, "● matrix.org");
633-
other_option_button.set_text(cx, "○ Other homeserver");
642+
self.update_homeserver_option_buttons(cx);
634643

635644
self.is_homeserver_editing = false;
636645
self.view
@@ -650,8 +659,7 @@ impl MatchEvent for RegisterScreen {
650659
.set_text(cx, "Custom homeserver (required)");
651660

652661
// Update button styles to show selection
653-
matrix_option_button.set_text(cx, "○ matrix.org");
654-
other_option_button.set_text(cx, "● Other homeserver");
662+
self.update_homeserver_option_buttons(cx);
655663
self.update_registration_mode(cx);
656664
}
657665

@@ -748,12 +756,10 @@ impl MatchEvent for RegisterScreen {
748756
self.update_button_mask(&register_button, cx, 1.0);
749757

750758
// Show registration status modal with appropriate text for password registration
751-
let status_label = self.view.label(ids!(status_modal_inner.status));
752-
status_label.set_text(cx, "Registering account, please wait...");
753-
let title_label = self.view.label(ids!(status_modal_inner.title));
754-
title_label.set_text(cx, "Registration Status");
755-
let cancel_button = self.view.button(ids!(status_modal_inner.cancel_button));
756-
cancel_button.set_text(cx, "Cancel");
759+
let status_modal_inner = self.view.register_status_modal(ids!(status_modal_inner));
760+
status_modal_inner.set_status(cx, "Registering account, please wait...");
761+
status_modal_inner.set_title(cx, "Registration Status");
762+
status_modal_inner.button_ref().set_text(cx, "Cancel");
757763
self.view.modal(ids!(status_modal)).open(cx);
758764
self.redraw(cx);
759765

@@ -827,11 +833,9 @@ impl MatchEvent for RegisterScreen {
827833
RegisterAction::SsoRegistrationStatus { status } => {
828834
// Update SSO status in modal (only if our modal is already open)
829835
if self.sso_pending {
830-
let status_label = self.view.label(ids!(status_modal_inner.status));
831-
status_label.set_text(cx, status);
832-
let cancel_button =
833-
self.view.button(ids!(status_modal_inner.cancel_button));
834-
cancel_button.set_text(cx, "Cancel");
836+
let status_modal_inner = self.view.register_status_modal(ids!(status_modal_inner));
837+
status_modal_inner.set_status(cx, status);
838+
status_modal_inner.button_ref().set_text(cx, "Cancel");
835839
self.redraw(cx);
836840
}
837841
}

src/register/register_status_modal.rs

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -120,6 +120,46 @@ impl MatchEvent for RegisterStatusModal {
120120
}
121121
}
122122

123+
impl RegisterStatusModal {
124+
/// Sets the title text displayed in the modal.
125+
fn set_title(&mut self, cx: &mut Cx, title: &str) {
126+
self.label(ids!(title)).set_text(cx, title);
127+
}
128+
129+
/// Sets the status text displayed in the body of the modal.
130+
fn set_status(&mut self, cx: &mut Cx, status: &str) {
131+
self.label(ids!(status)).set_text(cx, status);
132+
}
133+
134+
/// Returns a reference to the modal's button, enabling you to set its properties.
135+
fn button_ref(&self) -> ButtonRef {
136+
self.button(ids!(cancel_button))
137+
}
138+
}
139+
140+
impl RegisterStatusModalRef {
141+
/// See [`RegisterStatusModal::set_title()`].
142+
pub fn set_title(&self, cx: &mut Cx, title: &str) {
143+
if let Some(mut inner) = self.borrow_mut() {
144+
inner.set_title(cx, title);
145+
}
146+
}
147+
148+
/// See [`RegisterStatusModal::set_status()`].
149+
pub fn set_status(&self, cx: &mut Cx, status: &str) {
150+
if let Some(mut inner) = self.borrow_mut() {
151+
inner.set_status(cx, status);
152+
}
153+
}
154+
155+
/// See [`RegisterStatusModal::button_ref()`].
156+
pub fn button_ref(&self) -> ButtonRef {
157+
self.borrow()
158+
.map(|inner| inner.button_ref())
159+
.unwrap_or_default()
160+
}
161+
}
162+
123163
#[derive(Clone, DefaultNone, Debug)]
124164
pub enum RegisterStatusModalAction {
125165
/// The modal requested to be closed

src/register/validation.rs

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,8 @@ pub(super) fn normalize_custom_homeserver(raw: &str) -> Option<String> {
4242
format!("https://{trimmed}")
4343
};
4444

45-
Url::parse(&normalized).ok().map(|_| normalized)
45+
let url = Url::parse(&normalized).ok()?;
46+
matches!(url.scheme(), "http" | "https").then_some(normalized)
4647
}
4748

4849
pub(super) fn needs_custom_homeserver_input(
@@ -88,6 +89,16 @@ mod tests {
8889
);
8990
}
9091

92+
#[test]
93+
fn normalize_custom_homeserver_rejects_non_http_schemes() {
94+
assert_eq!(normalize_custom_homeserver("ftp://evil.com"), None);
95+
assert_eq!(normalize_custom_homeserver("file:///etc/passwd"), None);
96+
assert_eq!(normalize_custom_homeserver("javascript://alert(1)"), None);
97+
// http and https are accepted
98+
assert!(normalize_custom_homeserver("http://my-server.example").is_some());
99+
assert!(normalize_custom_homeserver("https://my-server.example").is_some());
100+
}
101+
91102
#[test]
92103
fn choosing_other_requires_explicit_custom_value_before_submit() {
93104
assert!(needs_custom_homeserver_input(true, "matrix.org"));

0 commit comments

Comments
 (0)