fix simulation_results names#122
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughTwo unrelated changes: R/snapshot.R remaps filesystem-safe simulation names back to the original snapshot simulation names for stored results and tibbles. renv/activate.R applies multiple bootstrap/activation refinements (version/checksum bump, suppressed startup output, safer cache bootstrap copy, repo parsing refactor, cache-path inference, stricter version equality, and tryCatch-wrapped bootstrap run). ChangesSnapshot simulation-name remapping
renv activation and bootstrap changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request modifies the result naming logic in R/snapshot.R to map sanitized file names back to their original simulation names, specifically handling cases where slashes were replaced by underscores. The review identifies a critical logic error where the subsequent loop uses sanitized names to index into the result lists, which will now fail to match the updated original names and cause incorrect list assignments. Additionally, the feedback suggests that the mapping logic should be more robust to account for other characters that might be sanitized beyond just slashes.
| # sim_names correspond to the original simulations names but if it contains "/" it has been replaced by "_" | ||
| # create mapping to ensure returned list name correspond to the original simulation names | ||
| original_sim_names <- self$get_names("simulations") | ||
| names(original_sim_names) <- gsub("/", "_", original_sim_names) |
There was a problem hiding this comment.
The mapping logic specifically targets slashes. If simulation names contain other characters that are sanitized by the underlying tool (e.g., spaces, dots, or other special characters), this gsub will not account for them, leading to NA names in the results list. It would be safer to use a more comprehensive sanitization or to verify if other characters are also replaced.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
R/snapshot.R (1)
409-423:⚠️ Potential issue | 🔴 CriticalName remapping is bypassed during result assignment, creating two separate sets of keys
At Line 409, you pre-name
results_objandresults_tibblewith original simulation names (e.g., "Sim/1", "Sim/2"), but Lines 417 and 421 assign using filesystem-safesim_name(e.g., "Sim_1", "Sim_2"). In R,x[[name]] <- valuecreates a new element if the name doesn't exist in the list. This means your lists end up with both the original-named slots (empty) and sanitized-named slots (populated). Downstream code at lines 585, 597, 612 accesses by original names, which will fail.Proposed fix
results_obj <- results_tibble <- vector("list", length(sim_results_files)) names(results_obj) <- names(results_tibble) <- original_sim_names[sim_names] for (i in seq_along(sim_results_files)) { sim_name <- sim_names[i] + result_name <- unname(original_sim_names[sim_name]) + if (is.na(result_name)) { + cli::cli_abort( + "Unable to map simulation result file name {.code {sim_name}} back to an original simulation name." + ) + } simulation <- ospsuite::loadSimulation(file.path( temp_dir, glue(temp_file_name, "-", sim_name, ".pkml") )) - results_obj[[sim_name]] <- ospsuite::importResultsFromCSV( + results_obj[[result_name]] <- ospsuite::importResultsFromCSV( simulation, file.path(temp_dir, sim_results_files[i]) ) - results_tibble[[ - sim_name - ]] <- ospsuite::simulationResultsToTibble(results_obj[[sim_name]]) + results_tibble[[result_name]] <- + ospsuite::simulationResultsToTibble(results_obj[[result_name]]) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@R/snapshot.R` around lines 409 - 423, You pre-set names using original_sim_names[sim_names] but then assign using the filesystem-safe sim_name, creating duplicate keys; change the assignments to use the original name mapping instead (e.g., compute target_name <- original_sim_names[sim_name] or original_sim_names[sim_names[i]] and then do results_obj[[target_name]] <- ospsuite::importResultsFromCSV(...) and results_tibble[[target_name]] <- ospsuite::simulationResultsToTibble(...)) so the populated list slots match the pre-named keys (references: results_obj, results_tibble, original_sim_names, sim_names, sim_name, importResultsFromCSV, simulationResultsToTibble).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@R/snapshot.R`:
- Around line 406-407: The gsub("/", "_", ...) mapping for
names(original_sim_names) can produce duplicate keys; replace that single
assignment with a guarded sequence: create safe_names <- gsub("/", "_",
original_sim_names), check for duplicated(safe_names) and if any duplicates
exist call safe_names <- make.unique(safe_names, sep = "_") (or append numeric
suffixes) and optionally emit a warning, then set names(original_sim_names) <-
safe_names so the mapping is injective and collisions are avoided.
---
Outside diff comments:
In `@R/snapshot.R`:
- Around line 409-423: You pre-set names using original_sim_names[sim_names] but
then assign using the filesystem-safe sim_name, creating duplicate keys; change
the assignments to use the original name mapping instead (e.g., compute
target_name <- original_sim_names[sim_name] or original_sim_names[sim_names[i]]
and then do results_obj[[target_name]] <- ospsuite::importResultsFromCSV(...)
and results_tibble[[target_name]] <- ospsuite::simulationResultsToTibble(...))
so the populated list slots match the pre-named keys (references: results_obj,
results_tibble, original_sim_names, sim_names, sim_name, importResultsFromCSV,
simulationResultsToTibble).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
nautilus69
left a comment
nautilus69
left a comment
There was a problem hiding this comment.
additional note: RCMDcheck failure is not related to this change.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@renv/activate.R`:
- Line 1059: The comparison uses redundant c() coercion: replace the expression
that checks package versions so it directly compares the two character strings
(variables expected and version) without wrapping them in c(); update the
condition from the current is.character(expected) && identical(c(expected),
c(version)) to a simpler is.character(expected) && identical(expected, version)
(or add is.character(version) if you want explicit type-safety) in
renv/activate.R where expected is derived from description[["Version"]] and
version is the installed version.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b541929e-68a9-4de1-b420-cb0fad643823
⛔ Files ignored due to path filters (1)
renv.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
renv/activate.R
| renv_bootstrap_validate_version_release <- function(version, description) { | ||
| expected <- description[["Version"]] | ||
| is.character(expected) && identical(expected, version) | ||
| is.character(expected) && identical(c(expected), c(version)) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for related version comparison patterns and the types involved
rg -n -C3 "renv_bootstrap_validate_version_release" --type=r
# Check if there are tests or documentation about version types
rg -n "numeric_version|package_version" --type=r -g "renv/activate.R"Repository: esqLABS/cts
Length of output: 838
🏁 Script executed:
# Look at the calling context at line 1016
rg -n -B 15 "renv_bootstrap_validate_version_release" renv/activate.R | head -40
# Check where version comes from in the bootstrap flow
rg -n "function(version" renv/activate.R
# Look for how version objects are created/handled
rg -n -C 3 "as\.character.*version|c\(.*version" renv/activate.RRepository: esqLABS/cts
Length of output: 2655
🏁 Script executed:
# Find where version is passed to the bootstrap chain
rg -n "renv_bootstrap_exec\|renv_bootstrap_validate_version" renv/activate.R | head -20
# Look at the whole bootstrap flow to see where version comes from
rg -n -B 5 -A 5 "renv_bootstrap_exec.*version" renv/activate.R
# Check if numeric_version is applied to the version parameter
rg -n "numeric_version" renv/activate.RRepository: esqLABS/cts
Length of output: 804
🏁 Script executed:
# Look at lines around 1434 to find version origin
sed -n '1400,1440p' renv/activate.R | cat -n
# Also search for where version is first defined in the activate.R file
rg -n "^version <-|^ version <-" renv/activate.R | head -10Repository: esqLABS/cts
Length of output: 1440
🏁 Script executed:
# Find where version is assigned (likely in the main body)
rg -n "version.*<-.*\"" renv/activate.R
# Look for the main function body - search around line 1395
sed -n '1370,1410p' renv/activate.R | cat -n
# Also check if version is used with numeric_version anywhere in the validation path
rg -n -B 3 -A 3 "numeric_version.*version|version.*numeric_version" renv/activate.RRepository: esqLABS/cts
Length of output: 1825
🏁 Script executed:
# Check how description is created - via packageDescription
rg -n "packageDescription" renv/activate.R -B 2 -A 2
# Look for any tests or edge cases around version validation
rg -n "test.*version|describe.*version" renv/activate.R -i
# Check git log for this specific change (if available)
git log -p --all -S "identical(c(expected), c(version))" -- renv/activate.R 2>/dev/null | head -50Repository: esqLABS/cts
Length of output: 814
🏁 Script executed:
# Search for any place where version might be transformed to numeric_version
rg -n "as\..*version|numeric_version|package_version" renv/activate.R | grep -v "^943:"
# Look specifically in the validation functions
sed -n '1001,1070p' renv/activate.R | cat -nRepository: esqLABS/cts
Length of output: 2986
Simplify the version comparison to remove redundant c() coercion.
Both expected (from description[["Version"]]) and version are character strings and do not require wrapping in c() before comparison. The coercion is a no-op for character inputs and suggests uncertainty about the parameter types. Simplify to is.character(expected) && identical(expected, version) for clarity. If type safety is needed, add an explicit is.character(version) check instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@renv/activate.R` at line 1059, The comparison uses redundant c() coercion:
replace the expression that checks package versions so it directly compares the
two character strings (variables expected and version) without wrapping them in
c(); update the condition from the current is.character(expected) &&
identical(c(expected), c(version)) to a simpler is.character(expected) &&
identical(expected, version) (or add is.character(version) if you want explicit
type-safety) in renv/activate.R where expected is derived from
description[["Version"]] and version is the installed version.
2 participants
closes #121
Summary by CodeRabbit