simplifies app_server by moving data items into modules#91
Conversation
not required currently, but could be useful in future if we wanted to make the year selectable
There was a problem hiding this comment.
Pull request overview
This PR refactors the Shiny app architecture by moving data loading and processing logic from app_server into individual modules. The changes simplify the main server function and improve encapsulation by keeping related functionality together within modules.
Changes:
- Renamed
baseline_yeartoselected_yearthroughout the codebase and made it a reactive expression that defaults to the maximum year in the data ifBASELINE_YEARenvironment variable is not set - Moved static data loading (lookup tables, configurations) from
app_serverinto individual modules that use them - Simplified module interfaces by passing
inputs_dataas a reactive instead of individual preprocessed data frames
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| R/app_server.R | Simplified by removing static data loading and preprocessing; now passes raw inputs_data to modules and creates selected_year as a reactive |
| R/mod_table_procedures.R | Now loads procedures_lookup internally and extracts procedures data from inputs_data |
| R/mod_table_diagnoses.R | Now loads diagnoses_lookup internally and extracts diagnoses data from inputs_data |
| R/mod_show_strategy_text.R | Now loads descriptions_lookup internally |
| R/mod_select_strategy.R | Now loads strategies data internally, but has unused parameter issue |
| R/mod_select_provider.R | Now loads provider data reactively based on selected geography |
| R/mod_plot_rates_trend.R | Updated to use selected_year parameter with minor documentation issue |
| R/mod_plot_rates.R | Now loads configuration and peers data internally; extracts and processes rates data from inputs_data |
| R/mod_plot_nee.R | Now loads NEE data internally |
| R/mod_plot_age_sex_pyramid.R | Extracts and filters age-sex data from inputs_data with parameter shadowing issue |
| R/utils_table.R | Updated function signatures to use selected_year instead of baseline_year |
| R/utils_plot.R | Updated function signatures to use selected_year instead of baseline_year |
| R/fct_plots.R | Updated function signatures to use selected_year instead of baseline_year |
| man/*.Rd | Updated documentation files to reflect parameter name changes, with some inconsistencies in descriptions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
* no longer loops both provider/la data * simplifies caching used in app to use shiny options
matt-dray
left a comment
There was a problem hiding this comment.
Excellent, this looks more sensible and makes the app server much more readable and better suited to testing.
One thing: in mod_select_geography_server can you remove the geographies argument and its appearance in @params? No longer needed.
each module now takes
inputs_data, and does the processing to that data inside the module. Any static data items required by the module are moved inside the module.This simplifies the logic in
app_server, and keeps functions logically closer together, which should make it easier to add unit tests to in the future.Makes baseline_year a reactive, called
selected_year. Adds logic so if the value is not set, it reads the maximum year frominputs_data