Conversation
tomjemmett
commented
Jan 21, 2026
- uses new rates data format
- reduces calls to reactives, instead saving values as variables
- modifies get container and enables caching
- moves get container into the get_all_geo_data function, rather than passing as an argument - changes logic in get_container to use the canonical microsoft cli app for authentication - changes logic in get_container to try to get a managed token, then fall back to using oauth - caches the data we load for performance - also caches the strategy text
There was a problem hiding this comment.
Pull request overview
This pull request updates the data handling architecture to use a new rates data format and implements caching strategies to improve performance. The changes consolidate Azure container access, modify the data structure for rates processing, and introduce memoization for data loading.
Changes:
- Refactored Azure authentication to use try-catch fallback from managed identity to interactive authentication
- Moved container initialization inside
get_all_geo_dataand updated geography-to-folder mapping logic - Implemented caching with memoise for data loading and bindCache for strategy text rendering
- Transformed rates data structure to separate national rates and join them with provider-level data
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| R/utils_server.R | Removed inputs_container parameter, internalized container access, updated geography folder mapping from conditional to switch statement, changed column rename from "resladst_ons" to "lad23cd" |
| R/mod_show_strategy_text.R | Added disk cache initialization and attempted to apply bindCache to module output |
| R/fct_azure.R | Removed tenant and app_id parameters, replaced conditional auth with try-catch pattern using hardcoded fallback credentials |
| R/app_server.R | Added caching infrastructure with memoise, reduced reactive calls by saving values to variables, added rates data transformation to extract and join national rates |
| .gitignore | Added .cache/ directory to ignore list for caching infrastructure |
Comments suppressed due to low confidence (1)
R/utils_server.R:6
- The documentation still references
inputs_containerwhich is no longer a parameter of this function. The text should be updated to reflect that the container is now obtained internally using the AZ_CONTAINER_INPUTS environment variable.
#' @param data_types Character. A vector of filenames (without filetypes) for
#' each of the inputs data files that you want to read from
#' the `inputs_container`.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
matt-dray
left a comment
There was a problem hiding this comment.
Thanks for putting this together, this solves a few issues.
One problem: the rates trend chart was erroring for me when running the app. Does this happen for you?
I think I have a fix. In mod_plot_rates_server() we need a y_labels reactive like:
# This reactive is missing
y_labels <- shiny::reactive({
shiny::req(strategy_config())
strategy_config()[["number_type"]] # returns a function
})And this can be passed to the (missing) y_labels argument in the subsequent module call:
mod_plot_rates_trend_server(
"mod_plot_rates_trend",
rates_trend_data,
y_axis_limits,
y_axis_title,
y_labels, # this was missing
baseline_year
)For me, this makes the trend plot appear in the app. Except the labels aren't there! Seems to work when forcing the number of breaks inside the plot_rates_trend() function though:
ggplot2::scale_y_continuous(
name = y_axis_title,
labels = y_labels,
breaks = scales::pretty_breaks(n = 5) # added this
)Happy to chat. Maybe there's a better way.
matt-dray
left a comment
There was a problem hiding this comment.
You screenshared this with me and it works locally as expected. Cheers!