Protein-turnover followups: MSstats-format CSV download, imputation help text, template-gated contrast UI#215
Conversation
- Load page: add "Download MSstats-format CSV" button per converter, enabled after conversion, written via data.table::fwrite. - QC/help text: state both imputation preconditions across the QC description, MBi tooltip, and help page. - Statmodel: hide contrast-matrix header and workflow bullet for the turnover and chemoproteomics templates, gated by app_template().
|
Warning Review limit reached
More reviews will be available in 46 minutes and 47 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR enhances MSstatsShiny with three functional improvements: clarified help documentation for data imputation and censoring assumptions, a new download feature to export processed data in MSstats CSV format, and template-driven conditional UI visibility for statistical model configuration workflows. ChangesUI Enhancements and Data Export
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@DESCRIPTION`:
- Around line 46-47: The two roxygen2 version fields are inconsistent: update
both the Config/roxygen2/version and the RoxygenNote entries in DESCRIPTION to
the same intended roxygen2 version (pick either 8.0.0 or 7.3.3 consistently),
save the file, and then regenerate the package documentation (run
roxygen2/devtools::document or equivalent) so the Rd/NAMESPACE artifacts match
the chosen version and avoid spurious diffs.
In `@R/module-loadpage-server.R`:
- Around line 571-578: The download handler output$download_msstats_format
currently always calls data.table::fwrite(get_data(), file) but get_data() can
return a list with PTM and PROTEIN tables; update the handler to detect that
case and branch: if get_data() is a single data.frame write it as before, but if
it's a list containing elements like PTM and PROTEIN then either disable/hide
this download (e.g., return or show a message) or export the appropriate
MSstats-formatted table(s) separately (e.g., write PTM and PROTEIN to separate
CSVs or combine into a single export), ensuring you reference get_data(),
output$download_msstats_format and use data.table::fwrite for each table
written.
- Around line 566-568: The export button is only enabled in the
observeEvent(get_data()) handler so it can remain enabled when upstream inputs
change or a later conversion fails; add logic to disable the
"download_msstats_format" control whenever upstream inputs that affect
conversion change (e.g., file selection inputs and any controls used by the
eventReactive triggered by input$proceed1) and ensure the button is only
re-enabled inside the successful conversion observer (observeEvent(get_data()))
after req(get_data()) succeeds; also explicitly disable the button in the
conversion error path so stale exports cannot be downloaded (reference
observeEvent(get_data()), get_data(), eventReactive(input$proceed1, ...), and
shinyjs::enable/"download_msstats_format").
In `@R/module-qc-ui.R`:
- Line 25: Summary: The UI copy in the p(...) call promises CSV downloads for
"all outputs" but plots are not downloadable. Update the paragraph string in
R/module-qc-ui.R (the p(...) element that currently mentions "All outputs are
available to download in 'csv' format") to accurately state that only summarized
tables and processed datasets are available as CSV downloads and that plot
export is not currently provided (or will be provided separately), so users
aren't misled by the claim referencing plot export at lines ~217-218.
In `@R/statmodel-ui-headers.R`:
- Around line 44-45: Update the default workflow bullet text in the UI by
removing any mention of "response curve" from the p element with id
"statmodel_workflow_bullet_default" so it only references creating a contrast
matrix/group comparison; leave any response-curve wording confined to the p
element with id "statmodel_workflow_bullet_response_curve" and ensure this
change aligns with the behavior in statmodelServer() which removes
response-curve mode for non-turnover/non-chemoproteomics templates.
🪄 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: 77ea17a6-6b49-483b-9ade-1ddf23f466d2
📒 Files selected for processing (10)
DESCRIPTIONPREDESIGN.mdR/module-help-ui.RR/module-loadpage-server.RR/module-loadpage-ui.RR/module-qc-ui.RR/module-statmodel-server.RR/statmodel-ui-comparisons.RR/statmodel-ui-headers.Rman/MSstatsShiny.Rd
💤 Files with no reviewable changes (1)
- man/MSstatsShiny.Rd
swaraj-neu
changed the title
| if (inherits(data, "data.frame")) { | ||
| paste0("MSstats_format-", Sys.Date(), ".csv") | ||
| } else { | ||
| paste0("MSstats_format-", Sys.Date(), ".zip") | ||
| } |
There was a problem hiding this comment.
I think it'll always inherit data frame right?
| tmp_dir <- tempfile("msstats_format_") | ||
| dir.create(tmp_dir) | ||
| on.exit(unlink(tmp_dir, recursive = TRUE), add = TRUE) | ||
| tmp_files <- character() | ||
| for (nm in names(data)) { | ||
| tbl <- data[[nm]] | ||
| if (is.null(tbl)) next | ||
| if (NROW(tbl) == 0L) next | ||
| tmp_path <- file.path(tmp_dir, paste0(nm, ".csv")) | ||
| data.table::fwrite(tbl, tmp_path) | ||
| tmp_files <- c(tmp_files, tmp_path) | ||
| } | ||
| utils::zip(zipfile = file, files = tmp_files, flags = "-j") |
There was a problem hiding this comment.
Simiilarly here, it should always inherit a data.frame. Or at least convertible into a data.table?
There was a problem hiding this comment.
@tonywu1999 Replying for both comment 1 and comment 2:
get_data() is a single data.frame for every converter except PTM. For PTM it's a list of two tables, PTM and PROTEIN, which this module already reads as get_data()$PTM and get_data()$PROTEIN (a data.frame's $ returns a column, not a full table). That list is what broke the original handler: fwrite produced a PTM,PROTEIN-headed CSV with columns collapsed by |. Since inherits(data, "data.frame") is FALSE for it, PTM now routes to the zip branch. Verified on a Metamorpheus PTM dataset, the download produces a valid zip with PTM.csv and PROTEIN.csv. The two are separate MSstats inputs (modified peptides vs unmodified proteins), so they can't be flattened into one CSV. Happy to just disable the button for PTM instead if you'd prefer the smaller surface.
There was a problem hiding this comment.
Oh that's a great callout, in this case, that should be fine.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@R/module-loadpage-server.R`:
- Around line 593-601: The tmp_files vector may be empty if every element in
data is NULL or has zero rows, causing utils::zip(zipfile = file, files =
tmp_files, ...) to fail; before calling utils::zip in the block that builds
tmp_files (the for loop over names(data) and the subsequent utils::zip call),
check if length(tmp_files) == 0 (or identical(tmp_files, character(0))) and if
so call stop() with a clear message (e.g. "No tables available to write to ZIP")
so the error is handled instead of invoking utils::zip with no files; ensure
this check is placed just before the utils::zip(...) invocation that uses the
variable file and tmp_files.
- Around line 584-603: The downloadHandler content function should be wrapped in
tryCatch to prevent unhandled errors from crashing the Shiny session: wrap the
entire body of the existing content = function(file) { ... } in tryCatch({...},
error = function(e) { write a short error message containing conditionMessage(e)
to the target file (use writeLines or writeBin depending on zip vs csv) instead
of calling stop(), and return silently }); ensure the existing cleanup
(on.exit(unlink(...))) and tmp_files handling remain intact and that failures in
data.table::fwrite, dir.create, or utils::zip are caught and result in an error
file for the user rather than rethrowing.
🪄 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: 458ad809-a11c-481b-b0c2-f595bc3853b1
📒 Files selected for processing (3)
R/module-loadpage-server.RR/module-qc-ui.RR/statmodel-ui-headers.R
✅ Files skipped from review due to trivial changes (1)
- R/module-qc-ui.R
🚧 Files skipped from review as they are similar to previous changes (1)
- R/statmodel-ui-headers.R
…ing in a trycatch block
tonywu1999
deleted the
MSstatsShiny/work/20260528_protein_turnover_easy_followups
branch
2 participants
Motivation and Context
This PR implements three key enhancements to the protein-turnover analysis workflow in MSstatsShiny:
MSstats-format CSV Download: Adds a convenient download button on the load page to export processed data in MSstats format after data conversion completes.
Imputation Help Text Clarification: Improves user guidance by explicitly documenting both preconditions for running feature imputation (protein must have at least one observed feature in the run AND the missing feature must be observed in at least one other run) across the QC description, model-based imputation tooltip, and help page.
Template-Gated Contrast UI: Hides the contrast matrix definition section and the default workflow step for protein-turnover and chemoproteomics templates, since these templates use response-curve analysis instead of traditional contrasts. This simplifies the UI for template-specific workflows.
Detailed Changes
Data Export Functionality
R/module-loadpage-server.R (+40 lines): Implemented download handler for MSstats-formatted data
get_data()becomes availableMSstats_format-<date>.csv)data.table::fwrite()for efficient writingR/module-loadpage-ui.R (+9/-2 lines): Extended action button section
downloadButton()andshinyjs::disabled()Help Text and Documentation
R/module-qc-ui.R (+2/-2 lines): Enhanced imputation documentation
R/module-help-ui.R (+6/-1 lines): Reformatted imputation help text
Template-Specific UI Visibility Control
R/module-statmodel-server.R (+9 lines): Template-aware UI management via
observeEvent(app_template())protein_turnovertemplate: hides contrast-matrix header and default workflow bullet, shows response-curve workflow bulletchemoproteomicstemplate: applies same hide/show behaviorshinyjs::hide()andshinyjs::show()withasis = TRUEfor DOM targetingR/statmodel-ui-comparisons.R (+1 line): Added explicit DOM targeting
id = "statmodel_contrast_header"to contrast-matrix header h4 elementR/statmodel-ui-headers.R (+6/-1 lines): Restructured workflow bullets for conditional display
id = "statmodel_workflow_bullet_default"to default workflow stepid = "statmodel_workflow_bullet_response_curve"to response-curve stepshinyjs::hidden()for initial visibility controlUnit Tests
No new unit tests were added or modified in this PR. The following test coverage gaps exist:
download_msstats_formatdownload handler functionality (single data.frame vs. multiple tables, ZIP creation, file naming)tests/testthat/test-module-loadpage-ui.R,tests/testthat/test-module-statmodel-ui.R, andtests/testthat/test-module-statmodel-server.Rwere not extendedCoding Guidelines
No violations of standard R or Shiny coding practices are evident in this PR. The implementation:
download_msstats_format,statmodel_contrast_header,statmodel_workflow_bullet_*)observeEvent()andreactive()on.exit()cleanup for ZIP creationns()andNS()data.table::fwrite(),utils::zip())Recommendation: While not a violation per se, best practice suggests adding unit tests for the new download handler and template visibility logic to ensure maintainability and prevent regressions.