Skip to content

Refactor of inputs app#624

Open
tomjemmett wants to merge 27 commits into
mainfrom
refactor_mitigators_server
Open

Refactor of inputs app#624
tomjemmett wants to merge 27 commits into
mainfrom
refactor_mitigators_server

Conversation

@tomjemmett
Copy link
Copy Markdown
Member

@tomjemmett tomjemmett commented Feb 16, 2026

Various refactors to improve the codebase and remove some of the historic code that doesn't serve a purpose any longer, and to make some strides to improve the apps responsiveness.

Remove the start() reactive

No longer serving any purpose, removed this in favour of a shiny::req(params$dataset) instead.

Altered handling of data files

Previously, we loaded all of the data files from azure inside of the server function, caching the reactive results. But, caching wasn't enabled for dev workflows, so our dev workflow was always slowed down by needing to re-download files.

Instead, we now have a function get_all_data_files() which is called in run_app if the folder inst/app/data does not exist.

This has necessitated renaming the existing data folder to inst/app/reference.

Used the same pattern for the json schema, this is downloaded by the get_all_data_files() function.

If the data files change, for dev purposes you can simply re-run the function. It will re-download and overwrite all the files.

Move all lookup files into a lookups environment

Rather than loading the lookups into reactives, then evaluating these and passing them to the modules, we now have a centralised lookups singleton.

All of the cases where we use the lookups are moved into the modules themselves, making the main server module much cleaner.

Removed redundant config values

Some options in the golem-config.yml no longer server any purpose. Removed these.

Removed caching code

No longer need to rely on shiny for caching, we persist the data on initial load. During shiny deployments, the inst/app/data folder will not be present. It will be created the first time the app is run after any redeployment.

Improvements to schema validation

There was a potential issue in mod_run_model_remove_invalid_mitigators - if the json provided was valid, then the errors attribute can be NULL, so the subsequent dplyr code could error out.

Loading the schema can take some time, moved this into a singleton pattern so the schema only needs to be loaded once.

Switch from download.file to using httr2 to load the params file - this gets rid of some janky code to append missing final newlines

@tomjemmett tomjemmett force-pushed the refactor_mitigators_server branch 2 times, most recently from 80061da to fc134f6 Compare February 16, 2026 21:03
@tomjemmett tomjemmett changed the title Refactor mitigators server Refactor of inputs app Feb 16, 2026
@tomjemmett tomjemmett marked this pull request as ready for review February 18, 2026 16:01
Copilot AI review requested due to automatic review settings February 18, 2026 16:01
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Refactors the Shiny “inputs” app to reduce startup latency in dev, centralize lookup loading, and simplify module wiring by removing legacy caching/reactives and moving some data/schema loading to singleton helpers.

Changes:

  • Adds ADLS download helpers (download_provider_data(), get_all_data_files()) and switches runtime loading to local parquet reads via load_provider_data().
  • Introduces a singleton get_lookups() environment and updates modules to consume lookups directly (instead of passing reactives around).
  • Refactors schema handling to a singleton (get_params_schema()), and removes legacy caching/startup mechanisms (e.g., create_data_cache(), start() reactive, waiting list tab).

Reviewed changes

Copilot reviewed 27 out of 38 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
man/load_provider_data.Rd Updates generated docs for load_provider_data() signature.
man/get_all_data_files.Rd Adds generated docs for get_all_data_files().
man/download_provider_data.Rd Adds generated docs for download_provider_data().
inst/golem-config.yml Removes redundant slider config values from mitigator config.
inst/app/reference/waiting_list_params.csv Adds reference CSV for waiting list multipliers.
inst/app/reference/rtt_specialties.csv Adds RTT specialties reference data.
inst/app/reference/providers.csv Adds providers lookup reference data.
inst/app/reference/procedures.csv Adds procedures lookup reference data.
inst/app/reference/peers.csv Adds peers lookup reference data.
inst/app/reference/nee_table.csv Adds NEE lookup reference data.
inst/app/reference/ndg_variants.json Adds NDG variants lookup reference data.
inst/app/reference/mitigator-codes.csv Adds mitigator codes lookup reference data.
R/run_app.R Ensures data directory exists at app startup; triggers initial download.
R/mod_waiting_list_imbalances_utils.R Switches RTT specialties lookup access to get_lookups().
R/mod_waiting_list_imbalances_server.R Switches waiting list multipliers to get_lookups() output.
R/mod_run_model_server.R Removes schema text parameter and uses schema singleton.
R/mod_run_model_remove_invalid_mitigators.R Fixes NULL errors edge case by early-return on valid JSON.
R/mod_run_model_fix_params.R Updates calls to new schema/migitator-cleanup signatures.
R/mod_non_demographic_adjustment_server.R Switches NDG variants loading to get_lookups().
R/mod_mitigators_summary_server.R Switches mitigator lookup usage to get_lookups() and relies on filtered reactive data inputs.
R/mod_mitigators_server.R Refactors module init, replaces passed-in lookups with get_lookups(), and reorganizes observers/outputs.
R/mod_inequalities_server.R Refactors to load inequalities data internally via get_inequalities_data().
R/mod_home_server.R Simplifies signature (providers no longer passed in).
R/mod_expat_repat_ui.R Adds spinners around plots/map outputs.
R/mod_expat_repat_server.R Refactors to load data internally and uses lookup singleton for providers/specialties/boundaries.
R/mod_baseline_adjustment_ui.R Switches RTT specialties loading to get_lookups().
R/mod_baseline_adjustment_server.R Refactors to load baseline data internally and uses lookup singleton.
R/fct_utils_lookups.R Introduces singleton lookup loader for CSV/JSON/SF reference data.
R/fct_reduce_values.R Removes reduce_values() helper.
R/fct_load_provider_data.R Adds in-memory cache and helpers for reading provider parquet datasets.
R/fct_create_data_cache.R Removes disk cache helper that configured shinyOptions(cache=...).
R/fct_azure_storage.R Replaces load_provider_data() ADLS read with download helpers and adds bulk download function.
R/app_ui.R Removes waiting list imbalances tab from UI.
R/app_server.R Removes legacy caching/start reactive; rewires module initialization and data loading to new helpers.
R/ZZZ.R Adds schema download via httr2 and a cached schema loader; removes old rtt_specialties() helper.
.gitignore Ignores inst/app/data/ (downloaded parquet + schema).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread R/mod_mitigators_server.R Outdated
Comment thread R/fct_load_provider_data.R Outdated
Comment thread R/fct_load_provider_data.R
Comment thread R/ZZZ.R Outdated
Comment thread R/fct_azure_storage.R Outdated
@StatsRhian StatsRhian removed their request for review February 27, 2026 16:46
@tomjemmett tomjemmett requested a review from a team as a code owner May 13, 2026 11:56
@matt-dray
Copy link
Copy Markdown
Contributor

matt-dray commented May 13, 2026

Thanks Tom! As discussed: we'll release v5.0 first, then attend to this before it gets stale.

tomjemmett added 16 commits May 21, 2026 09:18
removes code that is no longer needed. the config$params_items is now all just values and no longer functions that need to be evaluated

switches from a function to get the default interval to hardcoded values - the function ignored the argument and always returned c(0.95, 1.0)
use once=TRUE instead of init$destroy
prevents lookups having to be loaded multiple times for each different connection to the server
* no longer tries to download files in the shiny app
* adds dev/get_data_files.R to download the files to inst/app/data
* removes caching as no longer needed
* moves logic for reshaping the required data from the app_server function to their own functions in fct_load_provider_data.R
@tomjemmett tomjemmett force-pushed the refactor_mitigators_server branch from 5a81404 to c093082 Compare May 21, 2026 08:22
@tomjemmett tomjemmett force-pushed the refactor_mitigators_server branch from c093082 to f958388 Compare May 21, 2026 08:40
reverted to what we had before, except without the handling of function calls
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants