From 63188477cbf6a62cadc7a26181df356f50daab94 Mon Sep 17 00:00:00 2001 From: Noah Date: Tue, 19 Dec 2023 23:58:12 -0800 Subject: [PATCH 1/6] Format with cargo fmt --- src/components/about.rs | 2 +- src/components/mod.rs | 2 +- src/components/organization_entry.rs | 4 +- src/components/repository_card.rs | 25 ++-- src/components/repository_list.rs | 34 +++--- src/components/repository_paginator.rs | 151 ++++++++++++++++--------- src/components/review_and_submit.rs | 17 ++- src/components/welcome.rs | 2 +- src/lib.rs | 4 +- src/main.rs | 28 ++--- src/page_repo_map.rs | 4 +- src/repository.rs | 37 +++--- src/services/mod.rs | 2 +- 13 files changed, 186 insertions(+), 126 deletions(-) diff --git a/src/components/about.rs b/src/components/about.rs index 8258f74..8c5936d 100644 --- a/src/components/about.rs +++ b/src/components/about.rs @@ -7,4 +7,4 @@ pub fn about() -> Html {

{ "Explain the basic idea of the app here" }

} -} \ No newline at end of file +} diff --git a/src/components/mod.rs b/src/components/mod.rs index f7ef804..806bab0 100644 --- a/src/components/mod.rs +++ b/src/components/mod.rs @@ -1,7 +1,7 @@ -pub mod welcome; pub mod about; pub mod organization_entry; pub mod repository_card; pub mod repository_list; pub mod repository_paginator; pub mod review_and_submit; +pub mod welcome; diff --git a/src/components/organization_entry.rs b/src/components/organization_entry.rs index 282c647..e580d96 100644 --- a/src/components/organization_entry.rs +++ b/src/components/organization_entry.rs @@ -33,7 +33,9 @@ pub fn organization_entry() -> Html { let field_contents = field_contents.clone(); Callback::from(move |_| { if !field_contents.is_empty() { - dispatch.set(Organization { name: Some(field_contents.deref().clone()) }); + dispatch.set(Organization { + name: Some(field_contents.deref().clone()), + }); } }) }; diff --git a/src/components/repository_card.rs b/src/components/repository_card.rs index 84e099d..e5229aa 100644 --- a/src/components/repository_card.rs +++ b/src/components/repository_card.rs @@ -1,8 +1,8 @@ -use wasm_bindgen::{UnwrapThrowExt, JsCast}; +use wasm_bindgen::{JsCast, UnwrapThrowExt}; use web_sys::HtmlInputElement; use yew::prelude::*; -use crate::repository::{Repository, DesiredArchiveState}; +use crate::repository::{DesiredArchiveState, Repository}; #[derive(Clone, PartialEq, Properties)] pub struct Props { @@ -14,13 +14,16 @@ pub struct Props { // If it's a Some variant, then the enclosed boolean should indicate the desired // state for this repository. pub desired_archive_state: Option, - pub on_checkbox_change: Callback + pub on_checkbox_change: Callback, } #[function_component(RepositoryCard)] pub fn repository_card(props: &Props) -> Html { - let Props { repository, desired_archive_state, on_checkbox_change } - = props; + let Props { + repository, + desired_archive_state, + on_checkbox_change, + } = props; // If we pass this assertion, then the desired_archive_state.unwrap() in the HTML // should be safe. @@ -37,11 +40,13 @@ pub fn repository_card(props: &Props) -> Html { let target: HtmlInputElement = event_target.dyn_into().unwrap_throw(); let desired_archive_state = target.checked(); - web_sys::console::log_1(&format!("In ME callback desired state is {desired_archive_state}.").into()); + web_sys::console::log_1( + &format!("In ME callback desired state is {desired_archive_state}.").into(), + ); on_checkbox_change.emit(DesiredArchiveState { id, - desired_archive_state + desired_archive_state, }); }) }; @@ -55,10 +60,10 @@ pub fn repository_card(props: &Props) -> Html {
diff --git a/src/components/repository_list.rs b/src/components/repository_list.rs index ec06d14..9a9ef7d 100644 --- a/src/components/repository_list.rs +++ b/src/components/repository_list.rs @@ -2,8 +2,8 @@ use gloo::console::log; use yew::prelude::*; use yewdux::prelude::use_store; -use crate::repository::{RepoId, DesiredArchiveState, DesiredStateMap}; use crate::components::repository_card::RepositoryCard; +use crate::repository::{DesiredArchiveState, DesiredStateMap, RepoId}; // TODO: Can we use `AttrValue` instead of `String` here? // `AttrValue` is supposed to be more efficient @@ -13,30 +13,36 @@ use crate::components::repository_card::RepositoryCard; pub struct Props { pub repo_ids: Option>, pub empty_repo_list_message: String, - pub on_checkbox_change: Callback + pub on_checkbox_change: Callback, } #[function_component(RepositoryList)] pub fn repository_list(props: &Props) -> Html { - let Props { repo_ids, - empty_repo_list_message, - on_checkbox_change } = props; + let Props { + repo_ids, + empty_repo_list_message, + on_checkbox_change, + } = props; let (state_map, _) = use_store::(); log!(format!("We're in repo list with repo IDs {repo_ids:?}")); - log!(format!("We're in repo list with ArchiveStateMap {state_map:?}")); + log!(format!( + "We're in repo list with ArchiveStateMap {state_map:?}" + )); #[allow(clippy::option_if_let_else)] if let Some(repo_ids) = repo_ids { - repo_ids.iter() - .map(|repo_id: &RepoId| { - html! { - - } - }).collect() + repo_ids + .iter() + .map(|repo_id: &RepoId| { + html! { + + } + }) + .collect() } else { html! {

{ empty_repo_list_message }

diff --git a/src/components/repository_paginator.rs b/src/components/repository_paginator.rs index 3d43ff6..180b8cc 100644 --- a/src/components/repository_paginator.rs +++ b/src/components/repository_paginator.rs @@ -1,37 +1,37 @@ use std::num::ParseIntError; use std::ops::Deref; -use url::{Url, ParseError}; +use url::{ParseError, Url}; use gloo::console::log; -use reqwasm::http::{Request}; +use reqwasm::http::Request; -use yew_router::prelude::*; use yew::prelude::*; +use yew_router::prelude::*; use yewdux::prelude::{use_store, Dispatch}; use yewdux::store::Store; -use crate::Route; -use crate::repository::{Repository, DesiredArchiveState, DesiredStateMap, DesiredState}; -use crate::page_repo_map::{PageRepoMap, PageNumber}; use crate::components::repository_list::RepositoryList; +use crate::page_repo_map::{PageNumber, PageRepoMap}; +use crate::repository::{DesiredArchiveState, DesiredState, DesiredStateMap, Repository}; +use crate::Route; #[derive(Debug, Clone, PartialEq, Eq, Properties)] pub struct Props { - pub organization: String + pub organization: String, } #[derive(Debug, Clone)] struct State { current_page: PageNumber, - last_page: PageNumber + last_page: PageNumber, } #[derive(Debug)] enum LinkParseError { InvalidUrl(ParseError), PageEntryMissing(Url), - InvalidPageNumber(ParseIntError) + InvalidPageNumber(ParseIntError), } impl From for LinkParseError { @@ -49,9 +49,9 @@ impl From for LinkParseError { /* * This parses the `last` component of the link field in the response header from * GitHub, which tells us how many pages there are. - * + * * The link field looks like: - * + * * ; rel="prev", ; rel="next", ; rel="last", ; rel="first" */ fn parse_last_page(link_str: &str) -> Result, LinkParseError> { @@ -63,13 +63,15 @@ fn parse_last_page(link_str: &str) -> Result, LinkParseError> // rel="last" is missing if we're on the last page let last_entry = match last_entry { None => return Ok(None), - Some(s) => s + Some(s) => s, }; // This fails and returns a LinkParseError::UrlParseError if we can't parse the URL. - let last_url = last_entry.trim_start_matches('<') + let last_url = last_entry + .trim_start_matches('<') .trim_end_matches('>') .parse::()?; - let num_pages_str = last_url.query_pairs() + let num_pages_str = last_url + .query_pairs() // This returns the None variant if there was no "page" query parameter. // This is an error on GitHub's part (or a major change to their API), // and we'll return a LinkParseError::PageEntryMissingError if it happens. @@ -101,21 +103,25 @@ fn next_button_class(loaded: bool) -> String { class } -fn make_button_callback(page_number: PageNumber, repository_paginator_state: UseStateHandle) -> Callback { +fn make_button_callback( + page_number: PageNumber, + repository_paginator_state: UseStateHandle, +) -> Callback { Callback::from(move |_| { let repo_state = State { current_page: page_number, - last_page: repository_paginator_state.last_page + last_page: repository_paginator_state.last_page, }; - web_sys::console::log_1(&format!("make_button_callback called with page number {page_number}.").into()); + web_sys::console::log_1( + &format!("make_button_callback called with page number {page_number}.").into(), + ); web_sys::console::log_1(&format!("New state is {repo_state:?}.").into()); repository_paginator_state.set(repo_state); }) } fn try_extract(link_str: &str, current_page: PageNumber) -> Result { - let parse_result = parse_last_page(link_str)? - .unwrap_or(current_page); + let parse_result = parse_last_page(link_str)?.unwrap_or(current_page); Ok(parse_result) } @@ -126,14 +132,27 @@ fn handle_parse_error(err: &LinkParseError) { .alert_with_message("There was an error contacting the GitHub server; please try again") .unwrap(); web_sys::console::error_1( - &format!("There was an error parsing the link field in the HTTP response: {:?}", err).into()); + &format!( + "There was an error parsing the link field in the HTTP response: {:?}", + err + ) + .into(), + ); } -fn load_new_page(organization: &str, desired_state_map_dispatch: Dispatch, page_map: UseStateHandle, current_page: PageNumber, state: UseStateHandle) { +fn load_new_page( + organization: &str, + desired_state_map_dispatch: Dispatch, + page_map: UseStateHandle, + current_page: PageNumber, + state: UseStateHandle, +) { let organization = organization.to_owned(); // TODO: Possibly change `spawn_local` to `use_async`. wasm_bindgen_futures::spawn_local(async move { - web_sys::console::log_1(&format!("spawn_local called with organization {organization}.").into()); + web_sys::console::log_1( + &format!("spawn_local called with organization {organization}.").into(), + ); let request_url = format!("/orgs/{organization}/repos?sort=pushed&direction=asc&per_page={REPOS_PER_PAGE}&page={current_page}"); let response = Request::get(&request_url).send().await.unwrap(); let link = response.headers().get("link"); @@ -142,8 +161,11 @@ fn load_new_page(organization: &str, desired_state_map_dispatch: Dispatch 1, Some(link_str) => match try_extract(link_str, current_page) { Ok(last_page) => last_page, - Err(err) => { handle_parse_error(&err); return } - } + Err(err) => { + handle_parse_error(&err); + return; + } + }, }; // TODO: This seems fairly slow when there are a lot of repositories. My guess // is that parsing the huge pile of JSON we get back is at least part of the @@ -152,24 +174,18 @@ fn load_new_page(organization: &str, desired_state_map_dispatch: Dispatch = response.json().await.unwrap(); - + desired_state_map_dispatch.reduce_mut(|desired_state_map| { desired_state_map.with_repos(&repos_result); }); - let mut new_page_map - = page_map - .deref() - .clone(); - new_page_map.add_page( - current_page, - repos_result.iter().map(|r| r.id).collect() - ); + let mut new_page_map = page_map.deref().clone(); + new_page_map.add_page(current_page, repos_result.iter().map(|r| r.id).collect()); page_map.set(new_page_map); let repo_state = State { current_page, - last_page + last_page, }; web_sys::console::log_1(&format!("The new repo state is <{repo_state:?}>.").into()); state.set(repo_state); @@ -184,7 +200,6 @@ fn load_new_page(organization: &str, desired_state_map_dispatch: Dispatch Html { let repository_paginator_state = use_state(|| State { current_page: 1, - last_page: 0 + last_page: 0, }); // TODO: I feel like these should actually be two separate state objects. Instead // of having one big state object, it's probably better to separate out the state @@ -204,8 +219,10 @@ pub fn repository_paginator(props: &Props) -> Html { // TODO: In doing this separation, we should change `last_page` to be `Option` // so we'd have a clean way of distinguishing between when it's been set and when // it hasn't. - let State { current_page, last_page } - = (*repository_paginator_state).clone(); + let State { + current_page, + last_page, + } = (*repository_paginator_state).clone(); log!(format!("In paginator with page_map {page_map:?}.")); @@ -220,16 +237,25 @@ pub fn repository_paginator(props: &Props) -> Html { let desired_state_map_dispatch = desired_state_map_dispatch.clone(); use_effect_with_deps( move |_| { - repository_paginator_state.set(State { current_page: 1, last_page: 0 }); + repository_paginator_state.set(State { + current_page: 1, + last_page: 0, + }); page_map.set(PageRepoMap::new()); desired_state_map_dispatch.set(DesiredStateMap::new()); || () }, - organization + organization, ) } - web_sys::console::log_1(&format!("RepositoryPaginator called with organization {:?}.", organization).into()); + web_sys::console::log_1( + &format!( + "RepositoryPaginator called with organization {:?}.", + organization + ) + .into(), + ); web_sys::console::log_1(&format!("Current StateMap is {:?}.", desired_state_map).into()); // TODO: We want to see if the current page has already been loaded, and only do @@ -245,45 +271,58 @@ pub fn repository_paginator(props: &Props) -> Html { let desired_state_map_dispatch = desired_state_map_dispatch.clone(); use_effect_with_deps( move |(page_map, current_page)| { - log!(format!("Organization = {organization} and current page = {current_page}.")); - log!(format!("Current page has loaded = {}", page_map.has_loaded_page(*current_page))); + log!(format!( + "Organization = {organization} and current page = {current_page}." + )); + log!(format!( + "Current page has loaded = {}", + page_map.has_loaded_page(*current_page) + )); let current_page = *current_page; if !page_map.has_loaded_page(current_page) { - load_new_page(&organization.clone(), - desired_state_map_dispatch, + load_new_page( + &organization.clone(), + desired_state_map_dispatch, page_map.clone(), - current_page, - repository_paginator_state); + current_page, + repository_paginator_state, + ); } || () - }, - (page_map, current_page) + }, + (page_map, current_page), ); } - + let on_checkbox_change: Callback = { Callback::from(move |desired_archive_state| { - let DesiredArchiveState { id, desired_archive_state } = desired_archive_state; + let DesiredArchiveState { + id, + desired_archive_state, + } = desired_archive_state; desired_state_map_dispatch.reduce_mut(|state_map| { - state_map.update_desired_state(id, DesiredState::from_paginator_state(desired_archive_state)); + state_map.update_desired_state( + id, + DesiredState::from_paginator_state(desired_archive_state), + ); }); }) }; let prev: Callback = { // assert!(current_page > 1); - make_button_callback(current_page-1, repository_paginator_state.clone()) + make_button_callback(current_page - 1, repository_paginator_state.clone()) }; let next_or_review: Callback = { if current_page < last_page { - make_button_callback(current_page+1, repository_paginator_state) + make_button_callback(current_page + 1, repository_paginator_state) } else { let history = use_history().unwrap(); Callback::from(move |_: MouseEvent| history.push(Route::ReviewAndSubmit)) } }; - + html! { <> Html { } -} \ No newline at end of file +} diff --git a/src/components/review_and_submit.rs b/src/components/review_and_submit.rs index a61fc42..0025ea2 100644 --- a/src/components/review_and_submit.rs +++ b/src/components/review_and_submit.rs @@ -2,22 +2,27 @@ use web_sys::MouseEvent; use yew::{function_component, html, Callback}; use yewdux::prelude::use_store; -use crate::repository::{DesiredStateMap, DesiredArchiveState, DesiredState}; use crate::components::repository_list::RepositoryList; +use crate::repository::{DesiredArchiveState, DesiredState, DesiredStateMap}; use crate::services::archive_repos::archive_repositories; /// Review selected repositories to archive and /// submit archive requests. #[function_component(ReviewAndSubmit)] pub fn review_and_submit() -> Html { - let (archive_state_map, archive_state_dispatch) - = use_store::(); + let (archive_state_map, archive_state_dispatch) = use_store::(); let on_checkbox_change: Callback = { Callback::from(move |desired_archive_state| { - let DesiredArchiveState { id, desired_archive_state } = desired_archive_state; + let DesiredArchiveState { + id, + desired_archive_state, + } = desired_archive_state; archive_state_dispatch.reduce_mut(|archive_state_map| { - archive_state_map.update_desired_state(id, DesiredState::from_review_state(desired_archive_state)); + archive_state_map.update_desired_state( + id, + DesiredState::from_review_state(desired_archive_state), + ); }); }) }; @@ -48,4 +53,4 @@ pub fn review_and_submit() -> Html { } -} \ No newline at end of file +} diff --git a/src/components/welcome.rs b/src/components/welcome.rs index 4570e8d..ac7cf46 100644 --- a/src/components/welcome.rs +++ b/src/components/welcome.rs @@ -9,4 +9,4 @@ pub fn welcome() -> Html {

{ "A tool for archiving groups of GitHub repos" }

} -} \ No newline at end of file +} diff --git a/src/lib.rs b/src/lib.rs index 9377e95..4b8d019 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -6,10 +6,10 @@ use yew_router::Routable; -pub mod services; pub mod components; -pub mod repository; pub mod page_repo_map; +pub mod repository; +pub mod services; #[derive(Clone, Routable, PartialEq, Eq)] pub enum Route { diff --git a/src/main.rs b/src/main.rs index 5ab6c27..0eaa467 100644 --- a/src/main.rs +++ b/src/main.rs @@ -4,19 +4,21 @@ #![warn(clippy::expect_used)] use yew::prelude::*; -use yew_oauth2::prelude::*; use yew_oauth2::oauth2::*; // use `openid::*` when using OpenID connect +use yew_oauth2::prelude::*; use yew_router::prelude::*; use yewdux::prelude::*; -use ice_repos::{components::{ - welcome::Welcome, - about::About, - organization_entry::OrganizationEntry, - repository_paginator::RepositoryPaginator, - review_and_submit::ReviewAndSubmit -}, repository::Organization, Route}; +use ice_repos::{ + components::{ + about::About, organization_entry::OrganizationEntry, + repository_paginator::RepositoryPaginator, review_and_submit::ReviewAndSubmit, + welcome::Welcome, + }, + repository::Organization, + Route, +}; // =================================================================================== // for {username}.github.io/{repo_name} @@ -63,7 +65,7 @@ fn home_page() -> Html { // Where the list of repositories go // TODO: Maybe move this `if` into the paginator so that `HomePage` doesn't need to ever - // access any part of the global state. + // access any part of the global state. if let Some(organization) = organization {

{ format!("The list of repositories for the organization {}", organization) }

@@ -87,7 +89,7 @@ fn app() -> Html { OAuth2Dispatcher::::new().start_login(); }); let logout = Callback::from(|_: MouseEvent| { - OAuth2Dispatcher::::new().logout(); + OAuth2Dispatcher::::new().logout(); }); let config = Config { @@ -107,15 +109,15 @@ fn app() -> Html { -

+

{ "You need to log in" }

- +

- } + }; // html! { // // ******************************************************** diff --git a/src/page_repo_map.rs b/src/page_repo_map.rs index 827a028..f94bce2 100644 --- a/src/page_repo_map.rs +++ b/src/page_repo_map.rs @@ -8,13 +8,13 @@ pub type PageNumber = usize; #[derive(Default, Debug, Store, Eq, PartialEq, Clone)] pub struct PageRepoMap { - map: HashMap> + map: HashMap>, } impl PageRepoMap { pub fn new() -> Self { Self { - map: HashMap::new() + map: HashMap::new(), } } diff --git a/src/repository.rs b/src/repository.rs index e90e8d8..72f5712 100644 --- a/src/repository.rs +++ b/src/repository.rs @@ -25,27 +25,26 @@ pub struct Repository { pub archived: bool, pub updated_at: DateTime, pub pushed_at: DateTime, - // #[serde(flatten)] // extras: HashMap, } pub struct DesiredArchiveState { pub id: RepoId, - pub desired_archive_state: bool + pub desired_archive_state: bool, } // TODO: I think we may want to ultimately get rid of // this struct. It's not needed in the Paginator anymore, // and we may not need it in the `OrganizationEntry` component, -// but I'm not 100% about that. +// but I'm not 100% about that. // TODO: Can we use `AttrValue` instead of `String` here? // `AttrValue` is supposed to be more efficient // because cloning `String`s can be expensive. // https://yew.rs/docs/concepts/components/properties#memoryspeed-overhead-of-using-properties #[derive(Debug, Default, Clone, PartialEq, Eq, Store)] pub struct Organization { - pub name: Option + pub name: Option, } // TODO: Add an `AlreadyArchived` option here and keep all the @@ -61,7 +60,7 @@ pub enum DesiredState { /// We have chosen in the pagination view to archive this repository. Archive, /// We have changed from "to archive" to "don't archive" in the review view. - KeptInReview + KeptInReview, } impl DesiredState { @@ -98,7 +97,7 @@ pub struct DesiredStateMap { // containing the Repository struct and a boolean // indicating whether we want to archive that repository // or not. - pub map: BTreeMap + pub map: BTreeMap, } impl DesiredStateMap { @@ -109,7 +108,9 @@ impl DesiredStateMap { } else { DesiredState::Archive }; - self.map.entry(repo.id).or_insert((repo.clone(), initial_state)); + self.map + .entry(repo.id) + .or_insert((repo.clone(), initial_state)); } self } @@ -123,7 +124,7 @@ impl DesiredStateMap { pub fn update_desired_state(&mut self, id: RepoId, desired_state: DesiredState) -> &mut Self { web_sys::console::log_1(&format!("Updating {id} to {desired_state:?}").into()); - self.map.entry(id).and_modify(|p| { p.1 = desired_state }); + self.map.entry(id).and_modify(|p| p.1 = desired_state); web_sys::console::log_1(&format!("The resulting map was {self:?}").into()); self } @@ -133,18 +134,20 @@ impl DesiredStateMap { /// Will panic `repo_id` isn't in the `ArchiveStateMap`. #[must_use] pub fn get_repo(&self, repo_id: RepoId) -> &Repository { - assert!(self.map.contains_key(&repo_id), "Repository key {repo_id} not found in StateMap"); + assert!( + self.map.contains_key(&repo_id), + "Repository key {repo_id} not found in StateMap" + ); #[allow(clippy::unwrap_used)] self.map.get(&repo_id).map(|p| &p.0).unwrap() } pub fn get_repos_to_review(&self) -> impl Iterator { - self.map - .values() - .filter_map(|(repo, desired_state)| { - (*desired_state != DesiredState::AlreadyArchived || *desired_state != DesiredState::Keep) - .then_some(repo) - }) + self.map.values().filter_map(|(repo, desired_state)| { + (*desired_state != DesiredState::AlreadyArchived + || *desired_state != DesiredState::Keep) + .then_some(repo) + }) } #[must_use] @@ -160,8 +163,6 @@ impl DesiredStateMap { pub fn get_repos_to_archive(&self) -> impl Iterator { self.map .values() - .filter_map(|(repo, to_archive)| { - (*to_archive == DesiredState::Archive).then_some(repo) - }) + .filter_map(|(repo, to_archive)| (*to_archive == DesiredState::Archive).then_some(repo)) } } diff --git a/src/services/mod.rs b/src/services/mod.rs index ad352a0..5356a04 100644 --- a/src/services/mod.rs +++ b/src/services/mod.rs @@ -1 +1 @@ -pub mod archive_repos; \ No newline at end of file +pub mod archive_repos; From 038af0ddd63704dd4941cdc633daec508c824b23 Mon Sep 17 00:00:00 2001 From: Noah Date: Wed, 20 Dec 2023 00:13:13 -0800 Subject: [PATCH 2/6] Updated Yew and Yewdux to latest version --- Cargo.toml | 6 +- src/components/about.rs | 2 +- src/components/repository_paginator.rs | 69 ++++++++++------------ src/components/review_and_submit.rs | 2 +- src/components/welcome.rs | 2 +- src/lib.rs | 2 +- src/main.rs | 81 ++++++++++++++------------ 7 files changed, 82 insertions(+), 82 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index b5d8743..d599ce6 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -6,17 +6,17 @@ edition = "2021" # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html [dependencies] -yew = "0.19" +yew = { version = "0.21", features = ["csr"] } serde = { version = "1.0.117", features = ["derive"] } serde_json = "1.0" reqwasm = "0.5.0" gloo = "0.6.0" -yew-router = "0.16.0" +yew-router = "0.18.0" wasm-bindgen = "0.2.81" wasm-bindgen-futures = "0.4" chrono = { version = "0.4", features = [ "serde" ] } url = "2.2.2" -yewdux = "0.8.2" +yewdux = "0.10" yew-oauth2 = "0.4.0" [dependencies.web-sys] diff --git a/src/components/about.rs b/src/components/about.rs index 8c5936d..4bb7d07 100644 --- a/src/components/about.rs +++ b/src/components/about.rs @@ -1,4 +1,4 @@ -use yew::{function_component, html}; +use yew::{function_component, html, Html}; #[function_component(About)] pub fn about() -> Html { diff --git a/src/components/repository_paginator.rs b/src/components/repository_paginator.rs index 180b8cc..ad3e6a9 100644 --- a/src/components/repository_paginator.rs +++ b/src/components/repository_paginator.rs @@ -10,7 +10,6 @@ use reqwasm::http::Request; use yew::prelude::*; use yew_router::prelude::*; use yewdux::prelude::{use_store, Dispatch}; -use yewdux::store::Store; use crate::components::repository_list::RepositoryList; use crate::page_repo_map::{PageNumber, PageRepoMap}; @@ -235,18 +234,15 @@ pub fn repository_paginator(props: &Props) -> Html { let page_map = page_map.clone(); let organization = organization.clone(); let desired_state_map_dispatch = desired_state_map_dispatch.clone(); - use_effect_with_deps( - move |_| { - repository_paginator_state.set(State { - current_page: 1, - last_page: 0, - }); - page_map.set(PageRepoMap::new()); - desired_state_map_dispatch.set(DesiredStateMap::new()); - || () - }, - organization, - ) + use_effect_with(organization, move |_| { + repository_paginator_state.set(State { + current_page: 1, + last_page: 0, + }); + page_map.set(PageRepoMap::new()); + desired_state_map_dispatch.set(DesiredStateMap::default()); + || () + }) } web_sys::console::log_1( @@ -269,29 +265,26 @@ pub fn repository_paginator(props: &Props) -> Html { let page_map = page_map.clone(); let repository_paginator_state = repository_paginator_state.clone(); let desired_state_map_dispatch = desired_state_map_dispatch.clone(); - use_effect_with_deps( - move |(page_map, current_page)| { - log!(format!( - "Organization = {organization} and current page = {current_page}." - )); - log!(format!( - "Current page has loaded = {}", - page_map.has_loaded_page(*current_page) - )); - let current_page = *current_page; - if !page_map.has_loaded_page(current_page) { - load_new_page( - &organization.clone(), - desired_state_map_dispatch, - page_map.clone(), - current_page, - repository_paginator_state, - ); - } - || () - }, - (page_map, current_page), - ); + use_effect_with((page_map, current_page), move |(page_map, current_page)| { + log!(format!( + "Organization = {organization} and current page = {current_page}." + )); + log!(format!( + "Current page has loaded = {}", + page_map.has_loaded_page(*current_page) + )); + let current_page = *current_page; + if !page_map.has_loaded_page(current_page) { + load_new_page( + &organization.clone(), + desired_state_map_dispatch, + page_map.clone(), + current_page, + repository_paginator_state, + ); + } + || () + }); } let on_checkbox_change: Callback = { @@ -314,12 +307,12 @@ pub fn repository_paginator(props: &Props) -> Html { make_button_callback(current_page - 1, repository_paginator_state.clone()) }; + let history = use_navigator().unwrap(); let next_or_review: Callback = { if current_page < last_page { make_button_callback(current_page + 1, repository_paginator_state) } else { - let history = use_history().unwrap(); - Callback::from(move |_: MouseEvent| history.push(Route::ReviewAndSubmit)) + Callback::from(move |_: MouseEvent| history.push(&Route::ReviewAndSubmit)) } }; diff --git a/src/components/review_and_submit.rs b/src/components/review_and_submit.rs index 0025ea2..e3b44c5 100644 --- a/src/components/review_and_submit.rs +++ b/src/components/review_and_submit.rs @@ -1,5 +1,5 @@ use web_sys::MouseEvent; -use yew::{function_component, html, Callback}; +use yew::{function_component, html, Callback, Html}; use yewdux::prelude::use_store; use crate::components::repository_list::RepositoryList; diff --git a/src/components/welcome.rs b/src/components/welcome.rs index ac7cf46..da16c38 100644 --- a/src/components/welcome.rs +++ b/src/components/welcome.rs @@ -1,4 +1,4 @@ -use yew::{function_component, html}; +use yew::{function_component, html, Html}; #[function_component(Welcome)] pub fn welcome() -> Html { diff --git a/src/lib.rs b/src/lib.rs index 4b8d019..eb16813 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -11,7 +11,7 @@ pub mod page_repo_map; pub mod repository; pub mod services; -#[derive(Clone, Routable, PartialEq, Eq)] +#[derive(Clone, Routable, PartialEq, Eq, Copy)] pub enum Route { #[at("/ice-repos/review-and-submit")] ReviewAndSubmit, diff --git a/src/main.rs b/src/main.rs index 0eaa467..1a65d05 100644 --- a/src/main.rs +++ b/src/main.rs @@ -4,8 +4,6 @@ #![warn(clippy::expect_used)] use yew::prelude::*; -use yew_oauth2::oauth2::*; // use `openid::*` when using OpenID connect -use yew_oauth2::prelude::*; use yew_router::prelude::*; use yewdux::prelude::*; @@ -24,7 +22,7 @@ use ice_repos::{ // for {username}.github.io/{repo_name} // replace 'yew-template-for-github.io' to your repo name -#[derive(Clone, Routable, PartialEq)] +#[derive(Clone, Routable, PartialEq, Copy)] enum RootRoute { #[at("/ice-repos/")] Home, @@ -32,16 +30,16 @@ enum RootRoute { Route, } -fn root_route(routes: &RootRoute) -> Html { +fn root_route(routes: RootRoute) -> Html { match routes { RootRoute::Home => html! { }, RootRoute::Route => html! { - render={Switch::render(switch)} /> + render={switch} /> }, } } -fn switch(routes: &Route) -> Html { +fn switch(routes: Route) -> Html { match routes { Route::ReviewAndSubmit => html! { }, Route::About => html! { }, @@ -85,39 +83,48 @@ fn home_page() -> Html { /// main root #[function_component(App)] fn app() -> Html { - let login = Callback::from(|_: MouseEvent| { - OAuth2Dispatcher::::new().start_login(); - }); - let logout = Callback::from(|_: MouseEvent| { - OAuth2Dispatcher::::new().logout(); - }); - - let config = Config { - client_id: "c5b735f256dadf835133".into(), - auth_url: "https://github.com/login/oauth/authorize".into(), - token_url: "http://0.0.0.0:8787/finalize_login".into(), - }; + // let login = Callback::from(|_: MouseEvent| { + // OAuth2Dispatcher::::new().start_login(); + // }); + // let logout = Callback::from(|_: MouseEvent| { + // OAuth2Dispatcher::::new().logout(); + // }); + + // let config = Config { + // client_id: "c5b735f256dadf835133".into(), + // auth_url: "https://github.com/login/oauth/authorize".into(), + // token_url: "http://0.0.0.0:8787/finalize_login".into(), + // }; return html! { - - - -

-

{"Authenticated!"}

- - render={Switch::render(root_route)}/> - - - -

- { "You need to log in" } -

-

- -

- - + <> + //

+ //

{"Authenticated!"}

+ + render={root_route}/> + + }; + // return html! { + // + // + // + //

+ //

{"Authenticated!"}

+ // + // render={Switch::render(root_route)}/> + // + // + // + //

+ // { "You need to log in" } + //

+ //

+ // + //

+ // + // + // }; // html! { // // ******************************************************** @@ -134,5 +141,5 @@ fn app() -> Html { /// entry point fn main() { - yew::start_app::(); + yew::Renderer::::new().render(); } From 762aed168d0092260ce1677dd1a6f88b6f83185b Mon Sep 17 00:00:00 2001 From: Noah Date: Wed, 20 Dec 2023 03:08:31 -0800 Subject: [PATCH 3/6] Refactor state management --- src/components/organization_entry.rs | 25 +- src/components/repository_card.rs | 77 +++--- src/components/repository_list.rs | 55 ++-- src/components/repository_paginator.rs | 345 ++++--------------------- src/components/review_and_submit.rs | 40 +-- src/main.rs | 4 +- src/repository.rs | 177 +++++-------- src/services/archive_repos.rs | 2 +- src/services/get_repos.rs | 50 ++++ src/services/mod.rs | 1 + 10 files changed, 271 insertions(+), 505 deletions(-) create mode 100644 src/services/get_repos.rs diff --git a/src/components/organization_entry.rs b/src/components/organization_entry.rs index e580d96..a4feb50 100644 --- a/src/components/organization_entry.rs +++ b/src/components/organization_entry.rs @@ -6,7 +6,7 @@ use web_sys::HtmlInputElement; use yew::prelude::*; use yewdux::prelude::*; -use crate::repository::Organization; +use crate::{repository::Organization, services::get_repos::load_organization}; // * Change the state when the text area loses focus instead of requiring a click on the // submit button. @@ -20,7 +20,8 @@ use crate::repository::Organization; #[function_component(OrganizationEntry)] pub fn organization_entry() -> Html { let field_contents = use_state(|| String::from("")); - let (_, dispatch) = use_store::(); + let org_dispatch = use_dispatch::(); + let pagination_dispatch = use_dispatch::(); let oninput = { let field_contents = field_contents.clone(); @@ -29,17 +30,27 @@ pub fn organization_entry() -> Html { }) }; - let onclick: Callback = { + let onclick = { let field_contents = field_contents.clone(); Callback::from(move |_| { - if !field_contents.is_empty() { - dispatch.set(Organization { - name: Some(field_contents.deref().clone()), - }); + if field_contents.is_empty() { + return; } + + org_dispatch.reduce_mut(|org| { + let name = field_contents.deref().clone().into(); + org.set_name(name); + }); + + pagination_dispatch.reduce_mut(|state| { + state.reset(); + }); + + load_organization(&field_contents, org_dispatch.clone()); }) }; + // TODO: Use a form so we can also submit on "Enter" instead of having to click on "Submit" html! {
diff --git a/src/components/repository_card.rs b/src/components/repository_card.rs index e5229aa..9b21eb3 100644 --- a/src/components/repository_card.rs +++ b/src/components/repository_card.rs @@ -1,86 +1,85 @@ use wasm_bindgen::{JsCast, UnwrapThrowExt}; use web_sys::HtmlInputElement; use yew::prelude::*; +use yewdux::use_store; -use crate::repository::{DesiredArchiveState, Repository}; +use crate::repository::{ArchiveState, Organization, RepoId}; #[derive(Clone, PartialEq, Properties)] pub struct Props { // TODO: Having to clone the repository in `RepositoryList` is annoying and it // would be cool to turn this into a reference without making a mess of the // memory management. - pub repository: Repository, + pub repo_id: RepoId, // If this is the None variant, then this repository should already be archived. // If it's a Some variant, then the enclosed boolean should indicate the desired // state for this repository. - pub desired_archive_state: Option, - pub on_checkbox_change: Callback, + pub toggle_state: ToggleState, +} + +#[derive(Clone, PartialEq, Copy)] +pub struct ToggleState { + pub on: ArchiveState, + pub off: ArchiveState, } #[function_component(RepositoryCard)] pub fn repository_card(props: &Props) -> Html { let Props { - repository, - desired_archive_state, - on_checkbox_change, - } = props; - - // If we pass this assertion, then the desired_archive_state.unwrap() in the HTML - // should be safe. - if desired_archive_state.is_none() { - assert!(repository.archived); - } + repo_id, + toggle_state, + } = *props; - let onclick: Callback = { - let id = repository.id; - let on_checkbox_change = on_checkbox_change.clone(); - - Callback::from(move |mouse_event: MouseEvent| { - let event_target = mouse_event.target().unwrap_throw(); - let target: HtmlInputElement = event_target.dyn_into().unwrap_throw(); - let desired_archive_state = target.checked(); + let (org, org_dispatch) = use_store::(); + let repo = match org.repositories.get(&repo_id) { + Some(repo) => repo, + None => return html! {}, + }; - web_sys::console::log_1( - &format!("In ME callback desired state is {desired_archive_state}.").into(), - ); + let onclick = org_dispatch.reduce_mut_callback_with(move |state, mouse_event: MouseEvent| { + let event_target = mouse_event.target().unwrap_throw(); + let target: HtmlInputElement = event_target.dyn_into().unwrap_throw(); + let checked = target.checked(); - on_checkbox_change.emit(DesiredArchiveState { - id, - desired_archive_state, - }); - }) - }; + if let Some(repo) = state.repositories.get_mut(&repo_id) { + if checked { + repo.archive_state = toggle_state.on; + } else { + repo.archive_state = toggle_state.off; + } + } + }); html! {
- if repository.archived { + if repo.info.archived {

{ "This repository is already archived" }

} else {
} - if repository.archived { -

{ &repository.name }

+ if repo.info.archived { +

{ &repo.info.name }

} else { -

{ &repository.name }

+

{ &repo.info.name }

} { - repository.description.as_ref().map_or_else( + repo.info.description.as_ref().map_or_else( || html! {

{ "There was no description for this repository "}

}, |s| html! {

{ s }

} ) } -

{ format!("Last updated on {}; ", repository.updated_at.format("%Y-%m-%d")) } - { format!("last pushed to on {}", repository.pushed_at.format("%Y-%m-%d")) }

+

{ format!("Last updated on {}; ", repo.info.updated_at.format("%Y-%m-%d")) } + { format!("last pushed to on {}", repo.info.pushed_at.format("%Y-%m-%d")) }

} diff --git a/src/components/repository_list.rs b/src/components/repository_list.rs index 9a9ef7d..f905e20 100644 --- a/src/components/repository_list.rs +++ b/src/components/repository_list.rs @@ -1,9 +1,10 @@ -use gloo::console::log; use yew::prelude::*; use yewdux::prelude::use_store; use crate::components::repository_card::RepositoryCard; -use crate::repository::{DesiredArchiveState, DesiredStateMap, RepoId}; +use crate::repository::{ArchiveState, Organization}; + +use super::repository_card::ToggleState; // TODO: Can we use `AttrValue` instead of `String` here? // `AttrValue` is supposed to be more efficient @@ -11,38 +12,44 @@ use crate::repository::{DesiredArchiveState, DesiredStateMap, RepoId}; // https://yew.rs/docs/concepts/components/properties#memoryspeed-overhead-of-using-properties #[derive(Clone, PartialEq, Properties)] pub struct Props { - pub repo_ids: Option>, + pub range: std::ops::Range, + pub filter: Vec, + pub toggle_state: ToggleState, pub empty_repo_list_message: String, - pub on_checkbox_change: Callback, } #[function_component(RepositoryList)] pub fn repository_list(props: &Props) -> Html { + let (org, _) = use_store::(); + let Props { - repo_ids, + range, + filter, empty_repo_list_message, - on_checkbox_change, + toggle_state, } = props; - let (state_map, _) = use_store::(); - - log!(format!("We're in repo list with repo IDs {repo_ids:?}")); - log!(format!( - "We're in repo list with ArchiveStateMap {state_map:?}" - )); + let mut repo_ids = org + .repositories + .iter() + .skip(range.start) + .filter(|(_, repo)| filter.contains(&repo.archive_state)) + .take(range.end - range.start) + .map(|(repo_id, _)| repo_id) + .peekable(); + // Check if we have any repos to display. + let is_empty = repo_ids.peek().is_none(); + let repos_view = repo_ids + .map(|repo_id| { + let toggle_state = *toggle_state; + html! { + + } + }) + .collect(); - #[allow(clippy::option_if_let_else)] - if let Some(repo_ids) = repo_ids { - repo_ids - .iter() - .map(|repo_id: &RepoId| { - html! { - - } - }) - .collect() + if !is_empty { + repos_view } else { html! {

{ empty_repo_list_message }

diff --git a/src/components/repository_paginator.rs b/src/components/repository_paginator.rs index ad3e6a9..a81226c 100644 --- a/src/components/repository_paginator.rs +++ b/src/components/repository_paginator.rs @@ -1,94 +1,30 @@ -use std::num::ParseIntError; -use std::ops::Deref; - -use url::{ParseError, Url}; - -use gloo::console::log; - -use reqwasm::http::Request; - use yew::prelude::*; -use yew_router::prelude::*; -use yewdux::prelude::{use_store, Dispatch}; +use yew_router::prelude::use_navigator; +use yewdux::prelude::use_store; +use yewdux::{use_store_value, Store}; +use crate::components::repository_card::ToggleState; use crate::components::repository_list::RepositoryList; -use crate::page_repo_map::{PageNumber, PageRepoMap}; -use crate::repository::{DesiredArchiveState, DesiredState, DesiredStateMap, Repository}; +use crate::page_repo_map::PageNumber; +use crate::repository::{ArchiveState, Organization}; use crate::Route; -#[derive(Debug, Clone, PartialEq, Eq, Properties)] -pub struct Props { - pub organization: String, -} -#[derive(Debug, Clone)] -struct State { - current_page: PageNumber, - last_page: PageNumber, -} - -#[derive(Debug)] -enum LinkParseError { - InvalidUrl(ParseError), - PageEntryMissing(Url), - InvalidPageNumber(ParseIntError), -} +const INCREMENT: PageNumber = 5; -impl From for LinkParseError { - fn from(e: ParseError) -> Self { - Self::InvalidUrl(e) - } +#[derive(Clone, Debug, PartialEq, Default, Store)] +pub struct State { + page: PageNumber, } -impl From for LinkParseError { - fn from(e: ParseIntError) -> Self { - Self::InvalidPageNumber(e) +impl State { + pub fn reset(&mut self) { + self.page = 0; } } -/* - * This parses the `last` component of the link field in the response header from - * GitHub, which tells us how many pages there are. - * - * The link field looks like: - * - * ; rel="prev", ; rel="next", ; rel="last", ; rel="first" - */ -fn parse_last_page(link_str: &str) -> Result, LinkParseError> { - // This split won't do the desired thing if there can ever be a comma in a - // URL, but that doesn't seem likely given the structure of these GitHub URLs. - let last_entry = link_str - .split(", ") - .find_map(|s| s.trim().strip_suffix(r#"; rel="last""#)); - // rel="last" is missing if we're on the last page - let last_entry = match last_entry { - None => return Ok(None), - Some(s) => s, - }; - // This fails and returns a LinkParseError::UrlParseError if we can't parse the URL. - let last_url = last_entry - .trim_start_matches('<') - .trim_end_matches('>') - .parse::()?; - let num_pages_str = last_url - .query_pairs() - // This returns the None variant if there was no "page" query parameter. - // This is an error on GitHub's part (or a major change to their API), - // and we'll return a LinkParseError::PageEntryMissingError if it happens. - .find(|(k, _)| k.eq("page")) - .map(|(_, v)| v) - .ok_or_else(|| LinkParseError::PageEntryMissing(last_url.clone()))?; - // This fails and returns a LinkParseError::PageNumberParseError if for some - // reason the `num_pages_str` can't be parsed to a `PageNumber`. This would also - // presumably be an error or major API change on the part of GitHub. - Ok(Some(num_pages_str.parse::()?)) -} - -// The GitHub default is 30; they allow no more than 100. -const REPOS_PER_PAGE: u8 = 30; - fn prev_button_class(current_page: PageNumber) -> String { let mut class = "btn btn-primary".to_string(); - if current_page == 1 { + if current_page <= 1 { class.push_str(" btn-disabled"); } class @@ -101,236 +37,55 @@ fn next_button_class(loaded: bool) -> String { } class } - -fn make_button_callback( - page_number: PageNumber, - repository_paginator_state: UseStateHandle, -) -> Callback { - Callback::from(move |_| { - let repo_state = State { - current_page: page_number, - last_page: repository_paginator_state.last_page, - }; - web_sys::console::log_1( - &format!("make_button_callback called with page number {page_number}.").into(), - ); - web_sys::console::log_1(&format!("New state is {repo_state:?}.").into()); - repository_paginator_state.set(repo_state); - }) -} - -fn try_extract(link_str: &str, current_page: PageNumber) -> Result { - let parse_result = parse_last_page(link_str)?.unwrap_or(current_page); - Ok(parse_result) -} - -fn handle_parse_error(err: &LinkParseError) { - #[allow(clippy::unwrap_used)] - web_sys::window() - .unwrap() - .alert_with_message("There was an error contacting the GitHub server; please try again") - .unwrap(); - web_sys::console::error_1( - &format!( - "There was an error parsing the link field in the HTTP response: {:?}", - err - ) - .into(), - ); -} - -fn load_new_page( - organization: &str, - desired_state_map_dispatch: Dispatch, - page_map: UseStateHandle, - current_page: PageNumber, - state: UseStateHandle, -) { - let organization = organization.to_owned(); - // TODO: Possibly change `spawn_local` to `use_async`. - wasm_bindgen_futures::spawn_local(async move { - web_sys::console::log_1( - &format!("spawn_local called with organization {organization}.").into(), - ); - let request_url = format!("/orgs/{organization}/repos?sort=pushed&direction=asc&per_page={REPOS_PER_PAGE}&page={current_page}"); - let response = Request::get(&request_url).send().await.unwrap(); - let link = response.headers().get("link"); - web_sys::console::log_1(&format!("The link element of the header was <{link:?}>.").into()); - let last_page = match link.as_deref() { - None => 1, - Some(link_str) => match try_extract(link_str, current_page) { - Ok(last_page) => last_page, - Err(err) => { - handle_parse_error(&err); - return; - } - }, - }; - // TODO: This seems fairly slow when there are a lot of repositories. My guess - // is that parsing the huge pile of JSON we get back is at least part of the - // problem. Switching to GraphQL would potentially help this by allowing us to - // specify the exact info we need for each repository (which is a tiny subset of - // what GitHub currently provides), which should greatly reduce the - // size of the JSON package and the cost of the parsing. - let repos_result: Vec = response.json().await.unwrap(); - - desired_state_map_dispatch.reduce_mut(|desired_state_map| { - desired_state_map.with_repos(&repos_result); - }); - - let mut new_page_map = page_map.deref().clone(); - new_page_map.add_page(current_page, repos_result.iter().map(|r| r.id).collect()); - page_map.set(new_page_map); - - let repo_state = State { - current_page, - last_page, - }; - web_sys::console::log_1(&format!("The new repo state is <{repo_state:?}>.").into()); - state.set(repo_state); - }); -} - -// * Convert the state back to &str to avoid all the copying. -// * Maybe going to leave this alone? We got into a lot of lifetime issues that I didn't -// want to deal with right now., because with the current version of Yew (v19), we can't -// add generics to function components, and we'd need a lifetime component on the -// properties, which bleeds through to the function component. -// * Generics on function components have been added in the next version of Yew, so -// we can come back to this if/when I upgrade to the newer version. - -// This component has gotten _really_ long. At a minimum it should be moved -// into its own file. It's also possible that it should be converted into -// a struct component to help avoid some of the function call/return issues -// in the error handling. #[function_component(RepositoryPaginator)] -pub fn repository_paginator(props: &Props) -> Html { - let Props { organization } = props; - let page_map = use_state(|| PageRepoMap::new()); - - let repository_paginator_state = use_state(|| State { - current_page: 1, - last_page: 0, - }); - // TODO: I feel like these should actually be two separate state objects. Instead - // of having one big state object, it's probably better to separate out the state - // elements so they can be handled separately. - // TODO: In doing this separation, we should change `last_page` to be `Option` - // so we'd have a clean way of distinguishing between when it's been set and when - // it hasn't. - let State { - current_page, - last_page, - } = (*repository_paginator_state).clone(); - - log!(format!("In paginator with page_map {page_map:?}.")); - - // TODO: Change this from being a Yewdux global to being either "internal" state for - // the paginator, or use Yew's context tools to share this with the review and submit - // component. - let (desired_state_map, desired_state_map_dispatch) = use_store::(); - { - let repository_paginator_state = repository_paginator_state.clone(); - let page_map = page_map.clone(); - let organization = organization.clone(); - let desired_state_map_dispatch = desired_state_map_dispatch.clone(); - use_effect_with(organization, move |_| { - repository_paginator_state.set(State { - current_page: 1, - last_page: 0, - }); - page_map.set(PageRepoMap::new()); - desired_state_map_dispatch.set(DesiredStateMap::default()); - || () - }) - } - - web_sys::console::log_1( - &format!( - "RepositoryPaginator called with organization {:?}.", - organization - ) - .into(), - ); - web_sys::console::log_1(&format!("Current StateMap is {:?}.", desired_state_map).into()); - - // TODO: We want to see if the current page has already been loaded, and only do - // `update_state_for_organization` if it has not been loaded yet. Might make sense - // to fix this along with switching to "Prev"/"Next" UI model. - // TODO: It's possible that this would all be easier if we used a structural component - // here with messages for the various updates instead of having multiple `use_effect_with_deps` - // calls. - { - let organization = organization.clone(); - let page_map = page_map.clone(); - let repository_paginator_state = repository_paginator_state.clone(); - let desired_state_map_dispatch = desired_state_map_dispatch.clone(); - use_effect_with((page_map, current_page), move |(page_map, current_page)| { - log!(format!( - "Organization = {organization} and current page = {current_page}." - )); - log!(format!( - "Current page has loaded = {}", - page_map.has_loaded_page(*current_page) - )); - let current_page = *current_page; - if !page_map.has_loaded_page(current_page) { - load_new_page( - &organization.clone(), - desired_state_map_dispatch, - page_map.clone(), - current_page, - repository_paginator_state, - ); - } - || () - }); - } - - let on_checkbox_change: Callback = { - Callback::from(move |desired_archive_state| { - let DesiredArchiveState { - id, - desired_archive_state, - } = desired_archive_state; - desired_state_map_dispatch.reduce_mut(|state_map| { - state_map.update_desired_state( - id, - DesiredState::from_paginator_state(desired_archive_state), - ); - }); - }) - }; - - let prev: Callback = { - // assert!(current_page > 1); - make_button_callback(current_page - 1, repository_paginator_state.clone()) +pub fn repository_paginator() -> Html { + let (state, dispatch) = use_store::(); + let org = use_store_value::(); + let total_repos = org.repositories.len(); + let max_pages = total_repos / INCREMENT; + let filter = ArchiveState::filter_select(); + let toggle_state = ToggleState { + on: ArchiveState::Archive, + off: ArchiveState::Keep, }; - + let range = state.page..state.page.saturating_add(INCREMENT); + let prev = dispatch.reduce_mut_callback(|state| { + state.page = state.page.saturating_sub(INCREMENT); + }); let history = use_navigator().unwrap(); - let next_or_review: Callback = { - if current_page < last_page { - make_button_callback(current_page + 1, repository_paginator_state) - } else { - Callback::from(move |_: MouseEvent| history.push(&Route::ReviewAndSubmit)) + let next_btn = if state.page >= max_pages * INCREMENT { + let onclick = Callback::from(move |_: MouseEvent| history.push(&Route::ReviewAndSubmit)); + html! { + + } + } else { + let onclick = dispatch.reduce_mut_callback(move |state| { + state.page = state + .page + .saturating_add(INCREMENT) + .min(max_pages * INCREMENT); + }); + html! { + } }; html! { <> - +
- - + { next_btn }
} diff --git a/src/components/review_and_submit.rs b/src/components/review_and_submit.rs index e3b44c5..d550d26 100644 --- a/src/components/review_and_submit.rs +++ b/src/components/review_and_submit.rs @@ -1,45 +1,29 @@ use web_sys::MouseEvent; use yew::{function_component, html, Callback, Html}; -use yewdux::prelude::use_store; +use yewdux::use_store_value; +use crate::components::repository_card::ToggleState; use crate::components::repository_list::RepositoryList; -use crate::repository::{DesiredArchiveState, DesiredState, DesiredStateMap}; -use crate::services::archive_repos::archive_repositories; +use crate::repository::{ArchiveState, Organization}; /// Review selected repositories to archive and /// submit archive requests. #[function_component(ReviewAndSubmit)] pub fn review_and_submit() -> Html { - let (archive_state_map, archive_state_dispatch) = use_store::(); - - let on_checkbox_change: Callback = { - Callback::from(move |desired_archive_state| { - let DesiredArchiveState { - id, - desired_archive_state, - } = desired_archive_state; - archive_state_dispatch.reduce_mut(|archive_state_map| { - archive_state_map.update_desired_state( - id, - DesiredState::from_review_state(desired_archive_state), - ); - }); - }) - }; - - let onclick: Callback = { - let archive_state_map = archive_state_map.clone(); - Callback::from(move |_| { - archive_repositories(archive_state_map.get_repos_to_archive()); - }) + let org = use_store_value::(); + let onclick: Callback = Callback::noop(); + let filter = ArchiveState::filter_review(); + let toggle_state = ToggleState { + on: ArchiveState::Archive, + off: ArchiveState::KeptInReview, }; + let range = 0..org.repositories.len(); // TODO: We need some kind of shared header that comes across to pages like this. html! {
- +

{ "Clicking the 'Archive selected repositories' button will send archive requests diff --git a/src/main.rs b/src/main.rs index 1a65d05..066107a 100644 --- a/src/main.rs +++ b/src/main.rs @@ -50,7 +50,7 @@ fn switch(routes: Route) -> Html { #[function_component(HomePage)] fn home_page() -> Html { let (organization, _) = use_store::(); - let organization = organization.name.as_ref(); + let organization = organization.name(); html! {

@@ -67,7 +67,7 @@ fn home_page() -> Html { if let Some(organization) = organization {

{ format!("The list of repositories for the organization {}", organization) }

- +
} diff --git a/src/repository.rs b/src/repository.rs index 72f5712..bc7260e 100644 --- a/src/repository.rs +++ b/src/repository.rs @@ -1,4 +1,4 @@ -use std::collections::BTreeMap; +use std::{collections::BTreeMap, rc::Rc}; use chrono::{DateTime, Local}; @@ -8,20 +8,30 @@ use yewdux::prelude::*; pub type RepoId = usize; -// TODO: Can we use `AttrValue` instead of `String` here -// and in other places where there are properties? -// I'm not sure what would be necessary here since -// serde is filling these in, but maybe for other -// properties. `AttrValue::from(string)` may be -// important in doing the necessary conversions. -// `AttrValue` is supposed to be more efficient -// because cloning `String`s can be expensive. -// https://yew.rs/docs/concepts/components/properties#memoryspeed-overhead-of-using-properties +#[derive(Debug, Default, Clone, PartialEq, Eq, Store)] +pub struct Organization { + name: Option>, + pub repositories: Repositories, +} + +impl Organization { + /// Update the organization name. + pub fn set_name(&mut self, name: Rc) { + self.name = Some(name); + self.repositories = Default::default(); + // TODO: Maybe make the request to get the repositories here? + } + + pub fn name(&self) -> Option<&Rc> { + self.name.as_ref() + } +} + #[derive(Clone, Eq, PartialEq, Deserialize, Debug)] -pub struct Repository { +pub struct RepositoryInfo { pub id: RepoId, - pub name: String, - pub description: Option, + pub name: Rc, + pub description: Option>, pub archived: bool, pub updated_at: DateTime, pub pushed_at: DateTime, @@ -29,30 +39,42 @@ pub struct Repository { // extras: HashMap, } -pub struct DesiredArchiveState { - pub id: RepoId, - pub desired_archive_state: bool, +#[derive(Clone, Eq, PartialEq, Debug)] +pub struct Repository { + pub info: RepositoryInfo, + pub archive_state: ArchiveState, } -// TODO: I think we may want to ultimately get rid of -// this struct. It's not needed in the Paginator anymore, -// and we may not need it in the `OrganizationEntry` component, -// but I'm not 100% about that. -// TODO: Can we use `AttrValue` instead of `String` here? -// `AttrValue` is supposed to be more efficient -// because cloning `String`s can be expensive. -// https://yew.rs/docs/concepts/components/properties#memoryspeed-overhead-of-using-properties -#[derive(Debug, Default, Clone, PartialEq, Eq, Store)] -pub struct Organization { - pub name: Option, +#[derive(Debug, Default, Clone, PartialEq, Eq)] +pub struct Repositories(BTreeMap); + +impl Repositories { + pub fn update(&mut self, repos: impl IntoIterator) { + for repo in repos { + self.0.insert(repo.info.id, repo); + } + } + + pub fn len(&self) -> usize { + self.0.len() + } + + pub fn get(&self, id: &RepoId) -> Option<&Repository> { + self.0.get(id) + } + + pub fn get_mut(&mut self, id: &RepoId) -> Option<&mut Repository> { + self.0.get_mut(id) + } + + pub fn iter(&self) -> impl Iterator { + self.0.iter() + } } -// TODO: Add an `AlreadyArchived` option here and keep all the -// the repositories in all the maps regardless of whether they -// were archived in advance. /// The desired state for a given repository. -#[derive(Debug, Clone, PartialEq, Eq)] -pub enum DesiredState { +#[derive(Debug, Clone, PartialEq, Eq, Copy)] +pub enum ArchiveState { /// This repository was already archived and its state can't be change. AlreadyArchived, /// We have chosen in the pagination view to _not_ archive this repository. @@ -63,7 +85,20 @@ pub enum DesiredState { KeptInReview, } -impl DesiredState { +impl ArchiveState { + pub fn filter_select() -> Vec { + vec![ + Self::AlreadyArchived, + Self::Keep, + Self::Archive, + Self::KeptInReview, + ] + } + + pub fn filter_review() -> Vec { + vec![Self::Archive, Self::KeptInReview] + } + /// Convert a boolean, essentially the toggle state of a checkbox in the /// Paginator component and convert it into an `ArchiveState`. In the /// paginator, we want to use the `Skip` state to indicate that we do not @@ -90,79 +125,3 @@ impl DesiredState { } } } - -#[derive(Debug, Default, Clone, PartialEq, Eq, Store)] -pub struct DesiredStateMap { - // Map from the repository ID as a key, to a pair - // containing the Repository struct and a boolean - // indicating whether we want to archive that repository - // or not. - pub map: BTreeMap, -} - -impl DesiredStateMap { - pub fn with_repos(&mut self, repositories: &[Repository]) -> &mut Self { - for repo in repositories { - let initial_state = if repo.archived { - DesiredState::AlreadyArchived - } else { - DesiredState::Archive - }; - self.map - .entry(repo.id) - .or_insert((repo.clone(), initial_state)); - } - self - } - - #[must_use] - pub fn get_desired_state(&self, id: RepoId) -> Option { - self.map - .get(&id) - .map(|(_, desired_state)| matches!(desired_state, DesiredState::Archive)) - } - - pub fn update_desired_state(&mut self, id: RepoId, desired_state: DesiredState) -> &mut Self { - web_sys::console::log_1(&format!("Updating {id} to {desired_state:?}").into()); - self.map.entry(id).and_modify(|p| p.1 = desired_state); - web_sys::console::log_1(&format!("The resulting map was {self:?}").into()); - self - } - - /// # Panics - /// - /// Will panic `repo_id` isn't in the `ArchiveStateMap`. - #[must_use] - pub fn get_repo(&self, repo_id: RepoId) -> &Repository { - assert!( - self.map.contains_key(&repo_id), - "Repository key {repo_id} not found in StateMap" - ); - #[allow(clippy::unwrap_used)] - self.map.get(&repo_id).map(|p| &p.0).unwrap() - } - - pub fn get_repos_to_review(&self) -> impl Iterator { - self.map.values().filter_map(|(repo, desired_state)| { - (*desired_state != DesiredState::AlreadyArchived - || *desired_state != DesiredState::Keep) - .then_some(repo) - }) - } - - #[must_use] - pub fn get_repo_ids_to_review(&self) -> Vec { - self.get_repos_to_review().map(|r| r.id).collect() - } - - #[must_use] - pub fn get_owned_repos_to_review(&self) -> Vec { - self.get_repos_to_review().cloned().collect() - } - - pub fn get_repos_to_archive(&self) -> impl Iterator { - self.map - .values() - .filter_map(|(repo, to_archive)| (*to_archive == DesiredState::Archive).then_some(repo)) - } -} diff --git a/src/services/archive_repos.rs b/src/services/archive_repos.rs index 1977f44..59b403c 100644 --- a/src/services/archive_repos.rs +++ b/src/services/archive_repos.rs @@ -7,6 +7,6 @@ pub fn archive_repositories<'a>(repos: impl Iterator) { for repo in repos { // TODO: We need to change this to actually make the REST request // to the GitHub servers. - log!(format!("We are archiving {}.", repo.name)); + log!(format!("We are archiving {}.", repo.info.name)); } } diff --git a/src/services/get_repos.rs b/src/services/get_repos.rs new file mode 100644 index 0000000..e5ec655 --- /dev/null +++ b/src/services/get_repos.rs @@ -0,0 +1,50 @@ +use std::error::Error; + +use reqwasm::http::Request; +use yewdux::Dispatch; + +use crate::{ + page_repo_map::PageNumber, + repository::{ArchiveState, Organization, Repository, RepositoryInfo}, +}; + +// The GitHub default is 30; they allow no more than 100. +const REPOS_PER_PAGE: u8 = 30; + +async fn load_page( + organization: &str, + current_page: PageNumber, +) -> Result, Box> { + let request_url = format!( + "https://api.github.com/orgs/{organization}/repos?sort=pushed&direction=asc&per_page={REPOS_PER_PAGE}&page={current_page}" + ); + let response = Request::get(&request_url).send().await?; + Ok(response.json().await?) +} + +pub fn load_organization(organization: &str, dispatch: Dispatch) { + let organization = organization.to_owned(); + yew::platform::spawn_local(async move { + let mut page = 1; + while let Ok(repos) = load_page(&organization, page).await { + if repos.is_empty() { + break; + } + + let it = repos.into_iter().map(|info| Repository { + archive_state: if info.archived { + ArchiveState::AlreadyArchived + } else { + ArchiveState::Archive + }, + info, + }); + + dispatch.reduce_mut(move |org| { + org.repositories.update(it); + }); + + page += 1; + } + }); +} diff --git a/src/services/mod.rs b/src/services/mod.rs index 5356a04..1d2d521 100644 --- a/src/services/mod.rs +++ b/src/services/mod.rs @@ -1 +1,2 @@ pub mod archive_repos; +pub mod get_repos; From a9655ba7d3eb7211ab50cbfa815d5ba77bf73747 Mon Sep 17 00:00:00 2001 From: Noah Date: Fri, 22 Dec 2023 03:16:57 -0800 Subject: [PATCH 4/6] Polish and document --- src/components/organization_entry.rs | 36 ++++++------- src/components/repository_card.rs | 44 +++++++++------- src/components/repository_list.rs | 46 ++++++---------- src/components/repository_paginator.rs | 73 +++++++++++++++----------- src/repository.rs | 37 ++++++++++--- src/services/get_repos.rs | 3 ++ 6 files changed, 132 insertions(+), 107 deletions(-) diff --git a/src/components/organization_entry.rs b/src/components/organization_entry.rs index a4feb50..9facc3a 100644 --- a/src/components/organization_entry.rs +++ b/src/components/organization_entry.rs @@ -13,9 +13,6 @@ use crate::{repository::Organization, services::get_repos::load_organization}; // * There is an `onfocusout` event that we should be able to leverage. // * This will trigger when we tab out, but I'm thinking that might be OK since there's // nowhere else to go in this simple interface. -// * There's an `onsubmit` event. Would that be potentially useful? -// * Allow the user to press "Enter" instead of having to click on "Submit" - /// Controlled Text Input Component #[function_component(OrganizationEntry)] pub fn organization_entry() -> Html { @@ -29,10 +26,11 @@ pub fn organization_entry() -> Html { field_contents.set(get_value_from_input_event(input_event)); }) }; - - let onclick = { + let onsubmit = { let field_contents = field_contents.clone(); - Callback::from(move |_| { + Callback::from(move |event: SubmitEvent| { + event.prevent_default(); + if field_contents.is_empty() { return; } @@ -50,21 +48,23 @@ pub fn organization_entry() -> Html { }) }; - // TODO: Use a form so we can also submit on "Enter" instead of having to click on "Submit" html! { -
-
-
- - -
-
- +
+
+
+
+ + +
+
+ +
-
+ } } diff --git a/src/components/repository_card.rs b/src/components/repository_card.rs index 9b21eb3..534b587 100644 --- a/src/components/repository_card.rs +++ b/src/components/repository_card.rs @@ -1,5 +1,4 @@ use wasm_bindgen::{JsCast, UnwrapThrowExt}; -use web_sys::HtmlInputElement; use yew::prelude::*; use yewdux::use_store; @@ -7,17 +6,15 @@ use crate::repository::{ArchiveState, Organization, RepoId}; #[derive(Clone, PartialEq, Properties)] pub struct Props { - // TODO: Having to clone the repository in `RepositoryList` is annoying and it - // would be cool to turn this into a reference without making a mess of the - // memory management. + /// The ID of the repository to display. pub repo_id: RepoId, - // If this is the None variant, then this repository should already be archived. - // If it's a Some variant, then the enclosed boolean should indicate the desired - // state for this repository. + /// The state to set the repository to when the checkbox is checked/unchecked. pub toggle_state: ToggleState, } -#[derive(Clone, PartialEq, Copy)] +/// The toggle state for a repository card. Decsribes what ArchiveState to set when the checkbox is +/// checked/unchecked. +#[derive(Clone, PartialEq, Eq, Copy)] pub struct ToggleState { pub on: ArchiveState, pub off: ArchiveState, @@ -25,29 +22,27 @@ pub struct ToggleState { #[function_component(RepositoryCard)] pub fn repository_card(props: &Props) -> Html { + let (org, org_dispatch) = use_store::(); let Props { repo_id, toggle_state, } = *props; - let (org, org_dispatch) = use_store::(); let repo = match org.repositories.get(&repo_id) { Some(repo) => repo, - None => return html! {}, + None => return Default::default(), }; let onclick = org_dispatch.reduce_mut_callback_with(move |state, mouse_event: MouseEvent| { - let event_target = mouse_event.target().unwrap_throw(); - let target: HtmlInputElement = event_target.dyn_into().unwrap_throw(); - let checked = target.checked(); + let Some(repo) = state.repositories.get_mut(&repo_id) else { + return; + }; - if let Some(repo) = state.repositories.get_mut(&repo_id) { - if checked { - repo.archive_state = toggle_state.on; - } else { - repo.archive_state = toggle_state.off; - } - } + repo.archive_state = if is_checked(&mouse_event) { + toggle_state.on + } else { + toggle_state.off + }; }); html! { @@ -84,3 +79,12 @@ pub fn repository_card(props: &Props) -> Html {
} } + +fn is_checked(mouse_event: &MouseEvent) -> bool { + mouse_event + .target() + .unwrap_throw() + .dyn_into::() + .unwrap_throw() + .checked() +} diff --git a/src/components/repository_list.rs b/src/components/repository_list.rs index f905e20..4e1c714 100644 --- a/src/components/repository_list.rs +++ b/src/components/repository_list.rs @@ -1,27 +1,22 @@ use yew::prelude::*; -use yewdux::prelude::use_store; +use yewdux::use_store_value; use crate::components::repository_card::RepositoryCard; use crate::repository::{ArchiveState, Organization}; use super::repository_card::ToggleState; -// TODO: Can we use `AttrValue` instead of `String` here? -// `AttrValue` is supposed to be more efficient -// because cloning `String`s can be expensive. -// https://yew.rs/docs/concepts/components/properties#memoryspeed-overhead-of-using-properties #[derive(Clone, PartialEq, Properties)] pub struct Props { pub range: std::ops::Range, pub filter: Vec, pub toggle_state: ToggleState, - pub empty_repo_list_message: String, + pub empty_repo_list_message: AttrValue, } #[function_component(RepositoryList)] pub fn repository_list(props: &Props) -> Html { - let (org, _) = use_store::(); - + let org = use_store_value::(); let Props { range, filter, @@ -29,30 +24,21 @@ pub fn repository_list(props: &Props) -> Html { toggle_state, } = props; - let mut repo_ids = org - .repositories - .iter() - .skip(range.start) - .filter(|(_, repo)| filter.contains(&repo.archive_state)) - .take(range.end - range.start) - .map(|(repo_id, _)| repo_id) - .peekable(); - // Check if we have any repos to display. - let is_empty = repo_ids.peek().is_none(); - let repos_view = repo_ids - .map(|repo_id| { + let mut repos = org.repositories.select(range.clone(), filter).peekable(); + + let is_empty = repos.peek().is_none(); + if is_empty { + return html! { +

{ empty_repo_list_message }

+ }; + } + + repos + .map(|repo| { let toggle_state = *toggle_state; html! { - + } }) - .collect(); - - if !is_empty { - repos_view - } else { - html! { -

{ empty_repo_list_message }

- } - } + .collect() } diff --git a/src/components/repository_paginator.rs b/src/components/repository_paginator.rs index a81226c..9e9aba4 100644 --- a/src/components/repository_paginator.rs +++ b/src/components/repository_paginator.rs @@ -9,52 +9,47 @@ use crate::page_repo_map::PageNumber; use crate::repository::{ArchiveState, Organization}; use crate::Route; -const INCREMENT: PageNumber = 5; +const PAGE_SIZE: PageNumber = 5; -#[derive(Clone, Debug, PartialEq, Default, Store)] +#[derive(Clone, Debug, Default, PartialEq, Eq, Store)] pub struct State { page: PageNumber, } impl State { pub fn reset(&mut self) { - self.page = 0; + *self = Self::default(); } } -fn prev_button_class(current_page: PageNumber) -> String { - let mut class = "btn btn-primary".to_string(); - if current_page <= 1 { - class.push_str(" btn-disabled"); - } - class -} - -fn next_button_class(loaded: bool) -> String { - let mut class = "btn btn-primary".to_string(); - if !loaded { - class.push_str(" btn-disabled"); - } - class -} #[function_component(RepositoryPaginator)] pub fn repository_paginator() -> Html { let (state, dispatch) = use_store::(); let org = use_store_value::(); - let total_repos = org.repositories.len(); - let max_pages = total_repos / INCREMENT; + let history = use_navigator().unwrap(); + let filter = ArchiveState::filter_select(); + let total_repos = org.repositories.len(); + let total_pages = { + let pages = total_repos / PAGE_SIZE; + if total_repos % PAGE_SIZE == 0 { + pages.saturating_sub(1) + } else { + pages + } + }; + let range = (state.page * PAGE_SIZE)..(state.page * PAGE_SIZE).saturating_add(PAGE_SIZE); let toggle_state = ToggleState { on: ArchiveState::Archive, off: ArchiveState::Keep, }; - let range = state.page..state.page.saturating_add(INCREMENT); + let prev = dispatch.reduce_mut_callback(|state| { - state.page = state.page.saturating_sub(INCREMENT); + state.page = state.page.saturating_sub(1); }); - let history = use_navigator().unwrap(); - let next_btn = if state.page >= max_pages * INCREMENT { + let next_btn = if state.page >= total_pages { let onclick = Callback::from(move |_: MouseEvent| history.push(&Route::ReviewAndSubmit)); + html! { } }; @@ -79,14 +72,30 @@ pub fn repository_paginator() -> Html { <>
- { next_btn }
} } + +fn prev_button_class(current_page: PageNumber) -> String { + let mut class = "btn btn-primary".to_string(); + if current_page <= 1 { + class.push_str(" btn-disabled"); + } + class +} + +fn next_button_class(loaded: bool) -> String { + let mut class = "btn btn-primary".to_string(); + if !loaded { + class.push_str(" btn-disabled"); + } + class +} diff --git a/src/repository.rs b/src/repository.rs index bc7260e..cf74c17 100644 --- a/src/repository.rs +++ b/src/repository.rs @@ -15,14 +15,15 @@ pub struct Organization { } impl Organization { - /// Update the organization name. + /// Update the organization name. This will also reset the repositories, because we are + /// changing organizations. A separate call to `load_organization` must be made to actually + /// load the new organization's repositories. pub fn set_name(&mut self, name: Rc) { self.name = Some(name); self.repositories = Default::default(); - // TODO: Maybe make the request to get the repositories here? } - pub fn name(&self) -> Option<&Rc> { + pub const fn name(&self) -> Option<&Rc> { self.name.as_ref() } } @@ -49,12 +50,36 @@ pub struct Repository { pub struct Repositories(BTreeMap); impl Repositories { + /// Updates the repositories with the given `repos`. This will overwrite any existing + /// repositories with the same ID. pub fn update(&mut self, repos: impl IntoIterator) { for repo in repos { self.0.insert(repo.info.id, repo); } } + /// Selects a range of repositories, filtered by the given `filter`. The range is selected + /// _after_ filtering. + /// + /// # Panics + /// Panics if `range.start > range.end`. + pub fn select<'a>( + &'a self, + range: std::ops::Range, + filter: &'a [ArchiveState], + ) -> impl Iterator + 'a { + assert!(range.start <= range.end, "range start must be <= range end"); + self.0 + .values() + .filter(|repo| filter.contains(&repo.archive_state)) + .skip(range.start) + .take(range.end - range.start) + } + + pub fn is_empty(&self) -> bool { + self.0.is_empty() + } + pub fn len(&self) -> usize { self.0.len() } @@ -66,10 +91,6 @@ impl Repositories { pub fn get_mut(&mut self, id: &RepoId) -> Option<&mut Repository> { self.0.get_mut(id) } - - pub fn iter(&self) -> impl Iterator { - self.0.iter() - } } /// The desired state for a given repository. @@ -86,6 +107,7 @@ pub enum ArchiveState { } impl ArchiveState { + /// The filter for the pagination view. Includes all variants of `ArchiveState`. pub fn filter_select() -> Vec { vec![ Self::AlreadyArchived, @@ -95,6 +117,7 @@ impl ArchiveState { ] } + /// The filter for the review page. Includes only the `Archive` and `KeptInReview` variants. pub fn filter_review() -> Vec { vec![Self::Archive, Self::KeptInReview] } diff --git a/src/services/get_repos.rs b/src/services/get_repos.rs index e5ec655..a40a1f1 100644 --- a/src/services/get_repos.rs +++ b/src/services/get_repos.rs @@ -22,6 +22,7 @@ async fn load_page( Ok(response.json().await?) } +/// Load the repositories for the given organization. pub fn load_organization(organization: &str, dispatch: Dispatch) { let organization = organization.to_owned(); yew::platform::spawn_local(async move { @@ -40,6 +41,8 @@ pub fn load_organization(organization: &str, dispatch: Dispatch) { info, }); + // This call causes the entire organization state to be cloned. This is probably fine, + // but may not scale well for a very large repo list. dispatch.reduce_mut(move |org| { org.repositories.update(it); }); From 1c2aa205251b0f23c640729741491ca8a3cd270f Mon Sep 17 00:00:00 2001 From: Noah Date: Fri, 22 Dec 2023 03:40:54 -0800 Subject: [PATCH 5/6] Refactor repository_list filter type Adds RepoFilter, which is much more flexible. --- src/components/organization_entry.rs | 2 +- src/components/repository_card.rs | 2 +- src/components/repository_list.rs | 4 +- src/components/repository_paginator.rs | 4 +- src/components/review_and_submit.rs | 4 +- src/lib.rs | 2 +- src/main.rs | 2 +- src/{repository.rs => organization.rs} | 100 +++++++++++++++++++------ src/page_repo_map.rs | 2 +- src/services/archive_repos.rs | 2 +- src/services/get_repos.rs | 2 +- 11 files changed, 90 insertions(+), 36 deletions(-) rename src/{repository.rs => organization.rs} (68%) diff --git a/src/components/organization_entry.rs b/src/components/organization_entry.rs index 9facc3a..aac77d6 100644 --- a/src/components/organization_entry.rs +++ b/src/components/organization_entry.rs @@ -6,7 +6,7 @@ use web_sys::HtmlInputElement; use yew::prelude::*; use yewdux::prelude::*; -use crate::{repository::Organization, services::get_repos::load_organization}; +use crate::{organization::Organization, services::get_repos::load_organization}; // * Change the state when the text area loses focus instead of requiring a click on the // submit button. diff --git a/src/components/repository_card.rs b/src/components/repository_card.rs index 534b587..3553178 100644 --- a/src/components/repository_card.rs +++ b/src/components/repository_card.rs @@ -2,7 +2,7 @@ use wasm_bindgen::{JsCast, UnwrapThrowExt}; use yew::prelude::*; use yewdux::use_store; -use crate::repository::{ArchiveState, Organization, RepoId}; +use crate::organization::{ArchiveState, Organization, RepoId}; #[derive(Clone, PartialEq, Properties)] pub struct Props { diff --git a/src/components/repository_list.rs b/src/components/repository_list.rs index 4e1c714..a259947 100644 --- a/src/components/repository_list.rs +++ b/src/components/repository_list.rs @@ -2,14 +2,14 @@ use yew::prelude::*; use yewdux::use_store_value; use crate::components::repository_card::RepositoryCard; -use crate::repository::{ArchiveState, Organization}; +use crate::organization::{Organization, RepoFilter}; use super::repository_card::ToggleState; #[derive(Clone, PartialEq, Properties)] pub struct Props { pub range: std::ops::Range, - pub filter: Vec, + pub filter: RepoFilter, pub toggle_state: ToggleState, pub empty_repo_list_message: AttrValue, } diff --git a/src/components/repository_paginator.rs b/src/components/repository_paginator.rs index 9e9aba4..6f68dd5 100644 --- a/src/components/repository_paginator.rs +++ b/src/components/repository_paginator.rs @@ -5,8 +5,8 @@ use yewdux::{use_store_value, Store}; use crate::components::repository_card::ToggleState; use crate::components::repository_list::RepositoryList; +use crate::organization::{ArchiveState, Organization, RepoFilter}; use crate::page_repo_map::PageNumber; -use crate::repository::{ArchiveState, Organization}; use crate::Route; const PAGE_SIZE: PageNumber = 5; @@ -28,7 +28,7 @@ pub fn repository_paginator() -> Html { let org = use_store_value::(); let history = use_navigator().unwrap(); - let filter = ArchiveState::filter_select(); + let filter = RepoFilter::all(); let total_repos = org.repositories.len(); let total_pages = { let pages = total_repos / PAGE_SIZE; diff --git a/src/components/review_and_submit.rs b/src/components/review_and_submit.rs index d550d26..e4d040d 100644 --- a/src/components/review_and_submit.rs +++ b/src/components/review_and_submit.rs @@ -4,7 +4,7 @@ use yewdux::use_store_value; use crate::components::repository_card::ToggleState; use crate::components::repository_list::RepositoryList; -use crate::repository::{ArchiveState, Organization}; +use crate::organization::{ArchiveState, Organization, RepoFilter}; /// Review selected repositories to archive and /// submit archive requests. @@ -12,7 +12,7 @@ use crate::repository::{ArchiveState, Organization}; pub fn review_and_submit() -> Html { let org = use_store_value::(); let onclick: Callback = Callback::noop(); - let filter = ArchiveState::filter_review(); + let filter = RepoFilter::review_and_submit(); let toggle_state = ToggleState { on: ArchiveState::Archive, off: ArchiveState::KeptInReview, diff --git a/src/lib.rs b/src/lib.rs index eb16813..7990069 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -7,8 +7,8 @@ use yew_router::Routable; pub mod components; +pub mod organization; pub mod page_repo_map; -pub mod repository; pub mod services; #[derive(Clone, Routable, PartialEq, Eq, Copy)] diff --git a/src/main.rs b/src/main.rs index 066107a..702ceb4 100644 --- a/src/main.rs +++ b/src/main.rs @@ -14,7 +14,7 @@ use ice_repos::{ repository_paginator::RepositoryPaginator, review_and_submit::ReviewAndSubmit, welcome::Welcome, }, - repository::Organization, + organization::Organization, Route, }; diff --git a/src/repository.rs b/src/organization.rs similarity index 68% rename from src/repository.rs rename to src/organization.rs index cf74c17..7fd7a57 100644 --- a/src/repository.rs +++ b/src/organization.rs @@ -28,6 +28,12 @@ impl Organization { } } +#[derive(Clone, Eq, PartialEq, Debug)] +pub struct Repository { + pub info: RepositoryInfo, + pub archive_state: ArchiveState, +} + #[derive(Clone, Eq, PartialEq, Deserialize, Debug)] pub struct RepositoryInfo { pub id: RepoId, @@ -40,12 +46,6 @@ pub struct RepositoryInfo { // extras: HashMap, } -#[derive(Clone, Eq, PartialEq, Debug)] -pub struct Repository { - pub info: RepositoryInfo, - pub archive_state: ArchiveState, -} - #[derive(Debug, Default, Clone, PartialEq, Eq)] pub struct Repositories(BTreeMap); @@ -66,12 +66,12 @@ impl Repositories { pub fn select<'a>( &'a self, range: std::ops::Range, - filter: &'a [ArchiveState], + filter: &'a RepoFilter, ) -> impl Iterator + 'a { assert!(range.start <= range.end, "range start must be <= range end"); self.0 .values() - .filter(|repo| filter.contains(&repo.archive_state)) + .filter(|repo| filter.call(repo)) .skip(range.start) .take(range.end - range.start) } @@ -107,21 +107,6 @@ pub enum ArchiveState { } impl ArchiveState { - /// The filter for the pagination view. Includes all variants of `ArchiveState`. - pub fn filter_select() -> Vec { - vec![ - Self::AlreadyArchived, - Self::Keep, - Self::Archive, - Self::KeptInReview, - ] - } - - /// The filter for the review page. Includes only the `Archive` and `KeptInReview` variants. - pub fn filter_review() -> Vec { - vec![Self::Archive, Self::KeptInReview] - } - /// Convert a boolean, essentially the toggle state of a checkbox in the /// Paginator component and convert it into an `ArchiveState`. In the /// paginator, we want to use the `Skip` state to indicate that we do not @@ -148,3 +133,72 @@ impl ArchiveState { } } } + +/// Filters repositories based on some criteria. +/// +/// # Example +/// ``` +/// use ice_repos::organization::RepoFilter; +/// +/// let filter = RepoFilter::review_and_submit() +/// .and(|repo| repo.info.description.is_some()) +/// .and(|repo| repo.info.name.starts_with("a")) +/// // Other filters can be unpacked to combine with the current filter. +/// .and(RepoFilter::all().unpack()); +/// ``` +#[derive(Clone)] +pub struct RepoFilter(Rc bool>); +impl RepoFilter { + pub fn new(filter: impl Fn(&Repository) -> bool + 'static) -> Self { + Self(Rc::new(filter)) + } + + /// Filter for repositories to display in the review and submit page. + pub fn review_and_submit() -> Self { + Self::new(|repo| { + [ArchiveState::Archive, ArchiveState::KeptInReview].contains(&repo.archive_state) + }) + } + + /// Let all repositories through. + pub fn all() -> Self { + Self::new(|_| true) + } + + pub fn and(self, other: F) -> Self + where + F: Fn(&Repository) -> bool + 'static, + { + Self::new(move |repo| self.call(repo) && other(repo)) + } + + pub fn or(self, other: F) -> Self + where + F: Fn(&Repository) -> bool + 'static, + { + Self::new(move |repo| self.call(repo) || other(repo)) + } + + pub fn unpack(self) -> impl Fn(&Repository) -> bool { + move |repo| self.call(repo) + } + + pub fn call(&self, repo: &Repository) -> bool { + (self.0)(repo) + } +} + +impl PartialEq for RepoFilter { + fn eq(&self, other: &Self) -> bool { + Rc::ptr_eq(&self.0, &other.0) + } +} + +impl From for RepoFilter +where + F: Fn(&Repository) -> bool + 'static, +{ + fn from(f: F) -> Self { + Self::new(f) + } +} diff --git a/src/page_repo_map.rs b/src/page_repo_map.rs index f94bce2..34e1f57 100644 --- a/src/page_repo_map.rs +++ b/src/page_repo_map.rs @@ -2,7 +2,7 @@ use std::collections::HashMap; use yewdux::store::Store; -use crate::repository::RepoId; +use crate::organization::RepoId; pub type PageNumber = usize; diff --git a/src/services/archive_repos.rs b/src/services/archive_repos.rs index 59b403c..3497112 100644 --- a/src/services/archive_repos.rs +++ b/src/services/archive_repos.rs @@ -1,7 +1,7 @@ // TODO: Clean up the logging elsewhere to use `gloo::console::log`. use gloo::console::log; -use crate::repository::Repository; +use crate::organization::Repository; pub fn archive_repositories<'a>(repos: impl Iterator) { for repo in repos { diff --git a/src/services/get_repos.rs b/src/services/get_repos.rs index a40a1f1..6f5f2d2 100644 --- a/src/services/get_repos.rs +++ b/src/services/get_repos.rs @@ -4,8 +4,8 @@ use reqwasm::http::Request; use yewdux::Dispatch; use crate::{ + organization::{ArchiveState, Organization, Repository, RepositoryInfo}, page_repo_map::PageNumber, - repository::{ArchiveState, Organization, Repository, RepositoryInfo}, }; // The GitHub default is 30; they allow no more than 100. From 7f4b8bca08fbf39ee58138e92c81ea864ea146af Mon Sep 17 00:00:00 2001 From: Noah Date: Sat, 23 Dec 2023 04:36:28 -0800 Subject: [PATCH 6/6] Re-add last page parsing Prevents the extra request that was happening at the end of loading an organization. Was initially removed for simplicity, but re-added because we waste no cycles here :) --- src/services/get_repos.rs | 154 +++++++++++++++++++++++++++++--------- 1 file changed, 119 insertions(+), 35 deletions(-) diff --git a/src/services/get_repos.rs b/src/services/get_repos.rs index 6f5f2d2..c140756 100644 --- a/src/services/get_repos.rs +++ b/src/services/get_repos.rs @@ -1,6 +1,6 @@ -use std::error::Error; +use std::{error::Error, num::ParseIntError}; -use reqwasm::http::Request; +use reqwasm::http::{Request, Response}; use yewdux::Dispatch; use crate::{ @@ -11,43 +11,127 @@ use crate::{ // The GitHub default is 30; they allow no more than 100. const REPOS_PER_PAGE: u8 = 30; -async fn load_page( - organization: &str, - current_page: PageNumber, -) -> Result, Box> { - let request_url = format!( - "https://api.github.com/orgs/{organization}/repos?sort=pushed&direction=asc&per_page={REPOS_PER_PAGE}&page={current_page}" - ); - let response = Request::get(&request_url).send().await?; - Ok(response.json().await?) -} - /// Load the repositories for the given organization. pub fn load_organization(organization: &str, dispatch: Dispatch) { let organization = organization.to_owned(); yew::platform::spawn_local(async move { - let mut page = 1; - while let Ok(repos) = load_page(&organization, page).await { - if repos.is_empty() { - break; - } - - let it = repos.into_iter().map(|info| Repository { - archive_state: if info.archived { - ArchiveState::AlreadyArchived - } else { - ArchiveState::Archive - }, - info, - }); - - // This call causes the entire organization state to be cloned. This is probably fine, - // but may not scale well for a very large repo list. - dispatch.reduce_mut(move |org| { - org.repositories.update(it); - }); - - page += 1; + if let Err(error) = load_organization_task(&organization, dispatch).await { + web_sys::console::log_1( + &format!("spawn_local called with organization {error:?}.").into(), + ); } }); } + +pub async fn load_organization_task( + organization: &str, + dispatch: Dispatch, +) -> Result<(), Box> { + let mut current_page = 1; + + let response = get_repos(organization, current_page).await?; + append_repos(parse_repos(&response).await?, &dispatch); + + let last_page = response + .headers() + .get("link") + .and_then(|link| parse_last_page(&link).transpose()) + .transpose() + .map_err(|err| format!("Error parsing last page from link: {:?}", err))? + .unwrap_or(current_page); + current_page += 1; + while current_page <= last_page { + let response = get_repos(organization, current_page).await?; + // This call causes the entire organization state to be cloned. This is probably fine, + // but may not scale well for a very large repo list. + append_repos(parse_repos(&response).await?, &dispatch); + + current_page += 1; + } + + Ok(()) +} + +fn append_repos(repos: Vec, dispatch: &Dispatch) { + let it = repos.into_iter().map(|info| Repository { + archive_state: if info.archived { + ArchiveState::AlreadyArchived + } else { + ArchiveState::Archive + }, + info, + }); + + dispatch.reduce_mut(move |org| { + org.repositories.update(it); + }); +} + +async fn get_repos(organization: &str, page: PageNumber) -> Result> { + let base = "https://api.github.com/orgs"; + let args = "sort=pushed&direction=asc"; + let request_url = + format!("{base}/{organization}/repos?{args}&per_page={REPOS_PER_PAGE}&page={page}"); + + Ok(Request::get(&request_url).send().await?) +} + +async fn parse_repos(response: &Response) -> Result, Box> { + Ok(response.json().await?) +} + +/// This parses the `last` component of the link field in the response header from +/// GitHub, which tells us how many pages there are. +/// +/// The link field looks like: +/// +/// ; rel="prev", ; rel="next", ; rel="last", ; rel="first" +/// +fn parse_last_page(link_str: &str) -> Result, LinkParseError> { + // This split won't do the desired thing if there can ever be a comma in a + // URL, but that doesn't seem likely given the structure of these GitHub URLs. + let last_entry = link_str + .split(", ") + .find_map(|s| s.trim().strip_suffix(r#"; rel="last""#)); + // rel="last" is missing if we're on the last page + let last_entry = match last_entry { + None => return Ok(None), + Some(s) => s, + }; + // This fails and returns a LinkParseError::UrlParseError if we can't parse the URL. + let last_url = last_entry + .trim_start_matches('<') + .trim_end_matches('>') + .parse::()?; + let num_pages_str = last_url + .query_pairs() + // This returns the None variant if there was no "page" query parameter. + // This is an error on GitHub's part (or a major change to their API), + // and we'll return a LinkParseError::PageEntryMissingError if it happens. + .find(|(k, _)| k.eq("page")) + .map(|(_, v)| v) + .ok_or_else(|| LinkParseError::PageEntryMissing(last_url.clone()))?; + // This fails and returns a LinkParseError::PageNumberParseError if for some + // reason the `num_pages_str` can't be parsed to a `PageNumber`. This would also + // presumably be an error or major API change on the part of GitHub. + Ok(Some(num_pages_str.parse::()?)) +} + +#[derive(Debug)] +enum LinkParseError { + InvalidUrl(url::ParseError), + PageEntryMissing(url::Url), + InvalidPageNumber(ParseIntError), +} + +impl From for LinkParseError { + fn from(e: url::ParseError) -> Self { + Self::InvalidUrl(e) + } +} + +impl From for LinkParseError { + fn from(e: ParseIntError) -> Self { + Self::InvalidPageNumber(e) + } +}