feat/1-view-room-list #32

Merged
Sam merged 11 commits from feat/1-view-room-list into main 2026-05-01 00:54:38 +12:00
Owner

Add logic to display joined and available rooms. Room addresses and mock signals conform to API specification (with at least the minimum allowed specificity). Add display with rudimentary formatting of room objects. Simple unit tests for verification of conformity to expected JSON formats. Use signals, resources and memos to encourage decoupling, re-rendering (only as necessary) and future feature scaling.

Refs #1

Add logic to display joined and available rooms. Room addresses and mock signals conform to API specification (with at least the minimum allowed specificity). Add display with rudimentary formatting of room objects. Simple unit tests for verification of conformity to expected JSON formats. Use signals, resources and memos to encourage decoupling, re-rendering (only as necessary) and future feature scaling. Refs #1
skylar requested changes 2026-04-26 15:26:50 +12:00
Dismissed
skylar left a comment

feel free to msg me directly about any of the points so we dont have to have a discussion here, because I'm new to Rust my review points may not be that applicable

feel free to msg me directly about any of the points so we dont have to have a discussion here, because I'm new to Rust my review points may not be that applicable
Cargo.toml Outdated
@ -6,1 +6,4 @@
[dependencies]
serde = { version = "1.0", features = ["derive", "rc"] }
serde_json = "1.0"
either = "1.15.0"
Owner

was this ever used?

was this ever used?
Author
Owner

Both serde, serde_json are used but either ended up being removed. I thought I removed it merging main into my branch, it is not needed.

Both serde, serde_json are used but either ended up being removed. I thought I removed it merging main into my branch, it is not needed.
skylar marked this conversation as resolved
@ -0,0 +1,379 @@
use dioxus::prelude::*;
use serde;
Owner

It looks like all the uses of serde reference it themselves, so can this line be removed?

It looks like all the uses of serde reference it themselves, so can this line be removed?
skylar marked this conversation as resolved
@ -0,0 +56,4 @@
fn new(s: String) -> Result<Self, &'static str> {
// Empty check.
if s.is_empty() {
return Err::<T, &str>("Missing room id");
Owner

I noticed a lot of type annotations that can be inferred, can you remove these from the file? It causes a lot of clutter

I noticed a lot of type annotations that can be inferred, can you remove these from the file? It causes a lot of clutter
Author
Owner

I forget why but the compiler previously complained that they were needed. I removed all the turbofishes from the errors, let me know if you think there are other things that could be cleaned up.

I forget why but the compiler previously complained that they were needed. I removed all the turbofishes from the errors, let me know if you think there are other things that could be cleaned up.
Author
Owner

Ah it was because I wasn't explicitly returning the errors before (which was also a mistake).

Ah it was because I wasn't explicitly returning the errors before (which was also a mistake).
skylar marked this conversation as resolved
@ -0,0 +61,4 @@
// Prefix check for SPECIFIED type.
let (index, id_or_alias) = T::validate_prefix(&s);
if index == None {
if (|| id_or_alias == MxRoomEnum::MxRoomId as usize)() {
Owner

you can just put this inline to avoid the need of ||, it will keep it a bit cleaner

you can just put this inline to avoid the need of ||, it will keep it a bit cleaner
skylar marked this conversation as resolved
@ -0,0 +63,4 @@
if index == None {
if (|| id_or_alias == MxRoomEnum::MxRoomId as usize)() {
return Err::<T, &str>("Invalid room id, '!' missing.");
} else if (|| id_or_alias == MxRoomEnum::MxRoomAlias as usize)() {
Owner

same thing here, you can omit the || and put the return in line

same thing here, you can omit the || and put the return in line
skylar marked this conversation as resolved
@ -0,0 +68,4 @@
}
}
// Check that SLD has been listed.
let n_index = index.unwrap_or_else(|| 0);
Owner

Wondering here as well (referring to my comment on line 77)

Wondering here as well (referring to my comment on line 77)
Author
Owner

Technically yes, but you are right that it should be no. If the caller returns a usize that doesn't belong to the enum it would allow execution to continue, which is a flaw.

Technically yes, but you are right that it should be no. If the caller returns a usize that doesn't belong to the enum it would allow execution to continue, which is a flaw.
skylar marked this conversation as resolved
@ -0,0 +74,4 @@
return Err::<T, &str>("Invalid room, ':' missing.");
}
// Check that TLD has been listed.
let n_index = index.unwrap_or_else(|| 0);
Owner

Just wondering, what does unwrap_or_else do here? Are there conditions further than the previous index == None that can still happen?

Just wondering, what does unwrap_or_else do here? Are there conditions further than the previous index == None that can still happen?
skylar marked this conversation as resolved
@ -0,0 +81,4 @@
}
// Check legal chars for SPECIFIED type.
let mut t = String::with_capacity(s.len());
if (|| id_or_alias == MxRoomEnum::MxRoomId as usize)() {
Owner

same thing here, you can omit the || and put the replace inline

same thing here, you can omit the || and put the replace inline
skylar marked this conversation as resolved
@ -0,0 +83,4 @@
let mut t = String::with_capacity(s.len());
if (|| id_or_alias == MxRoomEnum::MxRoomId as usize)() {
t.push_str(&s.replace(&['!', ':', '.'][..], ""));
} else if (|| id_or_alias == MxRoomEnum::MxRoomAlias as usize)() {
Owner

same thing here, you can omit the || and put the replace inline

same thing here, you can omit the || and put the replace inline
skylar marked this conversation as resolved
@ -0,0 +87,4 @@
t.push_str(&s.replace(&['#', ':', '.'][..], ""));
}
if !(t.chars().all(char::is_alphanumeric)) {
Err::<T, &str>("Invalid room, illegal char(s).")
Owner

I was actually able to make a room with the following ID
#test-!a_@#$(&%($#&^%$bunchofchars:elsie.cafe

It seems like the only reserved character is the colon from what I could find (once you are already in a private/public room id from the ! or # that is)

I was actually able to make a room with the following ID #test-!a_@#$(&%(*$#&^%*$bunchofchars:elsie.cafe It seems like the _only_ reserved character is the colon from what I could find (once you are already in a private/public room id from the ! or # that is)
Author
Owner

Good catch. This was a bit trickier to sort out, but I have updated the logic to be much more lenient with room addresses. Room aliases in element disallow both '#' and '%' from the localpart which I think make sense to exclude, I've just followed that for now.

Good catch. This was a bit trickier to sort out, but I have updated the logic to be much more lenient with room addresses. Room aliases in [element](https://docs.element.io/latest/element-support/matrix-rooms/getting-started-creating-a-room/#starting-a-new-room) disallow both '#' and '%' from the localpart which I think make sense to exclude, I've just followed that for now.
skylar marked this conversation as resolved
@ -0,0 +96,4 @@
impl std::fmt::Display for MxRoomId {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
write!(f, "{:?}", self.0)
Owner

I noticed the {:?} format being used a few times, and saw that it is for debug
https://doc.rust-lang.org/std/fmt/index.html#fmtdisplay-vs-fmtdebug
Is this intentional?

I noticed the {:?} format being used a few times, and saw that it is for debug https://doc.rust-lang.org/std/fmt/index.html#fmtdisplay-vs-fmtdebug Is this intentional?
Author
Owner

It can be removed.

It can be removed.
skylar marked this conversation as resolved
@ -0,0 +121,4 @@
impl MxRoomAddressFactory for MxRoomId {
fn validate_prefix(s: &str) -> (Option<usize>, usize) {
let index: Option<usize> = s.find('!');
if index == Some(0) && s[index.unwrap_or_else(|| 0) + 1..].find('!') == None {
Owner

does the index == Some(0) followed by an unwrap_or_else make sense here?
Maybe I am misinterpreting unwrap_or_else, but I would assume it would always resolve to 0 here

does the index == Some(0) followed by an unwrap_or_else make sense here? Maybe I am misinterpreting unwrap_or_else, but I would assume it would always resolve to 0 here
Author
Owner

I'd already removed this while updating the checks for valid room addresses. But yes, it isn't needed - I forgot that && will just short circuit if index isn't Some(0). If the conditions were flipped for some reason it might make sense as index could be None in that case.

I'd already removed this while updating the checks for valid room addresses. But yes, it isn't needed - I forgot that && will just short circuit if index isn't Some(0). If the conditions were flipped for some reason it might make sense as index could be None in that case.
skylar marked this conversation as resolved
@ -0,0 +191,4 @@
impl std::fmt::Display for AvailableRoom {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
write!(f, "{:?} @ {:?}", self.name, self.room_id.0)
Owner

this one has also got the debug string formatting, is it intentional?

this one has also got the debug string formatting, is it intentional?
skylar marked this conversation as resolved
@ -0,0 +201,4 @@
}
// This is only needed for testing, we only expect to receive valid JSON objects making queries to the matrix API.
impl ParseError for AvailableRoomsList {
Owner

For these usages can default be used instead? it's used more as a default fallback than providing an error from what I can see.

For these usages can default be used instead? it's used more as a default fallback than providing an error from what I can see.
Author
Owner

Default could be used but I opted doing it this way since I think it's more idiomatic to have default return an empty or "base" object. ParseError is only a placeholder anyway, since once it's hooked up to the API we only expect to receive valid JSON - in which case the assertion-like unwrap() can be used instead. MxRoom_::new() already handles validity checks, so rooms can just be appended directly in the resource.

Default could be used but I opted doing it this way since I think it's more idiomatic to have default return an empty or "base" object. ParseError is only a placeholder anyway, since once it's hooked up to the API we only expect to receive valid JSON - in which case the assertion-like unwrap() can be used instead. MxRoom_::new() already handles validity checks, so rooms can just be appended directly in the resource.
Author
Owner

Actually it just calls default for AvailableRoomsList anyway. I was thinking of what I did for JoinedRoomsList (I wrote this code when I was quite tired). To be honest the whole thing could just be removed. There is no point since there is already a check upon serialisation of synthetic data.

Actually it just calls default for AvailableRoomsList anyway. I was thinking of what I did for JoinedRoomsList (I wrote this code when I was quite tired). To be honest the whole thing could just be removed. There is no point since there is already a check upon serialisation of synthetic data.
Author
Owner

Wow I completely forgot I did this but when attempting to deserialize using serde_json, all the necessary checks already take place (I overrode try_from so that serde calls the constructor method of MxRoom_) so yes this is not needed at all.

Wow I completely forgot I did this but when attempting to deserialize using serde_json, all the necessary checks already take place (I overrode try_from so that serde calls the constructor method of MxRoom_) so yes this is not needed at all.
Author
Owner

Also updating MxRoom_ defaults because I was treating those as fallback for some reason.

Also updating MxRoom_ defaults because I was treating those as fallback for some reason.
skylar marked this conversation as resolved
@ -0,0 +227,4 @@
// use_server_future not preferred as manual refresh expected. Make mutable on room addition.
let joined_rooms_monitor = use_resource(move || async move {
serde_json::from_str::<JoinedRoomsList>(&joined_rooms_from_api.read())
.unwrap_or_else(|_| JoinedRoomsList::default_parse_error()) // Can do assertion-like unwrap() if known conforming JSON.
Owner

can you use .unwrap_or_default here?

can you use .unwrap_or_default here?
skylar marked this conversation as resolved
@ -0,0 +301,4 @@
}
}
macro_rules! test_block {
Owner

Should this bit be out of the test block? I think it means it will get built with the application which we wouldn't want right?

Its giving me an unused macro in my ide as well when not inside of cfg(test)

Should this bit be out of the test block? I think it means it will get built with the application which we wouldn't want right? Its giving me an unused macro in my ide as well when not inside of cfg(test)
Author
Owner

Actually I don't think I need this macro at all.

Actually I don't think I need this macro at all.
skylar marked this conversation as resolved
Merge remote-tracking branch 'remotes/origin/main' into feat/1-view-room-list
Some checks failed
nix-build / build (pull_request) Failing after 2m36s
351c0c8756
Sam force-pushed feat/1-view-room-list from 5758b4240b to 7c7128adc8 2026-04-26 22:26:08 +12:00 Compare
refactor: resolved merge conflicts, rewrote sections of code to be cleaner
All checks were successful
nix-build / build (pull_request) Successful in 9m4s
aca441abe5
test: add additional unit tests for updated room address constructor logic
All checks were successful
nix-build / build (pull_request) Successful in 8m56s
2cd967b717
skylar approved these changes 2026-04-30 23:21:30 +12:00
Dismissed
chore: merge cleanup
All checks were successful
nix-build / build (pull_request) Successful in 26m10s
b8a255296f
Sam dismissed skylar's review 2026-05-01 00:02:11 +12:00
Reason:

New commits pushed, approval review dismissed automatically according to repository settings

chore: format
All checks were successful
nix-build / build (pull_request) Successful in 26m15s
1133324c7c
skylar approved these changes 2026-05-01 00:40:53 +12:00
Sam scheduled this pull request to auto merge when all checks succeed 2026-05-01 00:44:39 +12:00
Sam merged commit 0ec2f253e1 into main 2026-05-01 00:54:38 +12:00
Sam deleted branch feat/1-view-room-list 2026-05-01 00:54:38 +12:00
Sign in to join this conversation.
No reviewers
No labels
No milestone
No project
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
E91/GS3!32
No description provided.