WIP Address growing architectural problems #123
Description
Problem Statement
The codebase currently has many architectural problems related to navigation and state management that limit tests and modularity (which would allow for reusability). Even though we're not focusing on adding more features right now, we should still aim to maintain a codebase that is easy to work with and extend later on. This means extensive test coverage, clearer control flow and more extensible functions/systems.
To highlight the need for this refactor, here are a few concerns I had while going through the codebase:
- The Model (
App) is overloaded
App is responsible for state management, navigation, and UI control. All fields in App are exposed because at some point, some page might need them. This makes for a simple solution to state management (global state/object), but an ugly solution that can lead to high mental overhead when working with the codebase.
pub struct App {
pub current_screen: CurrentScreen, // UI state
pub mailing_list_selection: MailingListSelection, // Business logic
pub config: Config, // Infrastructure
pub lore_api_client: BlockingLoreAPIClient, // Infrastructure
pub latest_patchsets: Option<LatestPatchsets>, // Holds latest patchsets even when not in use
pub bookmarked_patchsets: BookmarkedPatchsets, // Holds bookmarks even if we are not in that screen
}Aditionally, every screen has dedicated fields inside App, making it hard to track dependencies and to extend. Every new page would result in a new arm for every match app.current_screen, something that is used often.
Terminaldependency is hard to test
The Terminal instance, an external dependency, is passed deeply into functions which makes testing difficult.
- Key Handling mixes logic & UI
Right now, key handlers update state and trigger rendering (by modifying App). Additionally, loading_screen and popup is called primarily in handler, which is quite obscure.
- Navigation logic is scattered across functions
While this makes it modular in some sense, it also obscures a key part of the codebase deep within different functions. Extending and/or modifying this can be risky unless you read through the different possible chains of navigation to ensure robustness. This problem appears in multiple parts of the codebase. It can be difficult to pinpoint the exact place where something happens.
Navigation logic is also tightly coupled with business logic.
pub fn handle_mailing_list_selection<B>(
app: &mut App,
key: KeyEvent,
mut terminal: Terminal<B>,
) -> color_eyre::Result<ControlFlow<(), Terminal<B>>>
where
B: Backend + Send + 'static,
{
match key.code {
KeyCode::Enter => {
if app.mailing_list_selection.has_valid_target_list() {
app.init_latest_patchsets(); // Business logic
app.set_current_screen(CurrentScreen::LatestPatchsets); // Navigation
}
}
}
Ok(ControlFlow::Continue(terminal))
}If we want to change navigation behaviour (e.g., add a confirmation popup before switching screens), we must modify multiple places in the code.
These are just primary concerns, smaller issues such as screens not being modular, popup-logic being hard to scale or confusing order of operations at the start of the app (logger runs after a decent chunk of code is executed, args. are resolved after app is setup etc) are also reasons for a refactor.
Proposed Refactor
It's more practical and efficient to start with small concerns, with a rough plan, and refine it as we go, rather than trying to find all problems upfront and make one solution to fix them all. I also aim to ensure that no refactor breaks the build and not to start from scratch, which makes it all the more difficult to map out exactly how and what to refactor.
MVVM
Having said that, the first major refactor I'd like to propose is to move to MVVM architecture instead of MVC. I believe this would address a lot of the problems with modularity in the app as well as lead to a simpler control flow.
The primary reasons for the switch are:
- MVVM scales better for multiple screens
Adding a new screen means:
- Updating the Controller
- Possibly modifying the Model
- Updating how the View fetches the state
Over time, the controller and the model becomes bloated due to having to handle the data and logic for many views. The idea behind MVVM is for one screen to only require a ViewModel and minimal modifications elsewhere:
struct App {
current_screen: Screen,
current_viewmodel: Box<dyn ScreenViewModel>,
}
Each screen will have all it's data and logic in it's ViewModel. This also enables better testing as ViewModels are independent of Views and the primary Model. Screens will not need to know about each other and thus increase modularity.
- View pulls directly from the model
Since our pages need data to be shown, the UI often requires data directly from App. This means the View is tied to data structures that are not optimized for UI's needs. By introducing a ViewModel, it ensures the View is dumb and simply displays what data it is given instead of accessing App and keeps UI separate from logic.
- Screens can be made more modular
Since we can safely assume we'll have all required data within it's ViewModel, screens made be to consist of just components constructing UI.
Next Steps
As stated earlier, in the process of refactoring one part, I might have to refactor another. So this would be a more comprehensive refactor than expected, and would lead to more similar proposals down the road. If these changes seem to be worth the shot, I can proceed implementing a small part and showing proof of concept. I think the path ahead would become more clearer as we start refactoring.