Switched file reads to data.table::fread#208
Merged
Merged
Conversation
* Replaced read.csv / read.delim / read.table calls in R/utils.R, R/module-visualize-network-server.R, and R/plotting_funtions.R with data.table::fread, which auto-detects CSV vs TSV * Removed the create_separator_buttons UI helper and all sep_* radio inputs and validation, since users only upload CSV or TSV * Updated getDataCode and the network code generator so the downloadable R script also uses data.table::fread * Adjusted test stubs (read.csv -> data.table::fread) and dropped the obsolete sep_* mock_input fields and the create_separator_buttons UI test * Regenerated NAMESPACE and man/MSstatsShiny.Rd via roxygen2 Co-Authored-By: Claude <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR updates roxygen to 8.0.0, removes column-separator UI and related validations, and migrates CSV/TSV reading from base R readers to data.table::fread across utilities, modules, generated code, and tests. ChangesData Loading Modernization
Sequence Diagram(s) sequenceDiagram
participant User
participant UI
participant Server
participant getData
participant fread
User->>UI: upload file via fileInput
UI->>Server: input$data present
Server->>getData: request parse
getData->>fread: call data.table::fread(input$data$datapath)
fread-->>getData: parsed data.frame/data.table
getData-->>Server: parsed dataset
Server-->>UI: enable proceed1
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
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)
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 |
![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)
tests/testthat/test-utils.R (1)
462-468:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove hardcoded
sepfrom these migrated tests.Line 468, Line 487, and Line 596 still force
mock_input$sep = ","even after separator UI removal. This can mask regressions by keeping legacy input state alive in tests that should validatefreadauto-detection withoutsep.Suggested test update
test_that("dda pd", { suppressWarnings({ mock_input$BIO <- "Protein" mock_input$DDA_DIA <- "LType" mock_input$filetype = "PD" - mock_input$sep = "," + mock_input$sep = NULL @@ test_that("dda prog", { suppressWarnings({ @@ - mock_input$sep = "," + mock_input$sep = NULL @@ test_that("dda openms", { suppressWarnings({ @@ - mock_input$sep = "," + mock_input$sep = NULLAlso applies to: 481-487, 590-596
🤖 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 `@tests/testthat/test-utils.R` around lines 462 - 468, Remove the hardcoded separator assignments in the migrated tests so they no longer force legacy UI state: in the test block named "dda pd" (and the other affected test_that blocks where mock_input$sep = "," appears) delete the lines that set mock_input$sep = "," so the tests rely on fread auto-detection; ensure you only remove the sep assignments and leave mock_input$BIO, mock_input$DDA_DIA, mock_input$filetype etc. intact (look for usages of mock_input and the test_that titles to locate each occurrence).
🧹 Nitpick comments (3)
R/plotting_funtions.R (1)
138-139: ⚡ Quick winMake the
.logparser explicit here.This file is reading MSstatsShiny's own log artifact, not one of the CSV/TSV uploads the PR is targeting.
fread(finalfile)leaves delimiter/header detection to auto-guessing, soprocessoutcan change shape as the log contents evolve. Please pass the expected layout explicitly so this stays stable across runs.🤖 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 `@R/plotting_funtions.R` around lines 138 - 139, The code currently uses data.table::fread(finalfile) which auto-detects delimiter/header and can produce unstable shapes; change the call that populates processout (the fread invoked on finalfile in plotting_funtions.R) to parse the expected .log layout explicitly — e.g., call data.table::fread(finalfile, sep="\t", header=FALSE, fill=TRUE, quote="") (or the exact delimiter/header settings for your log format) before converting to matrix so processout remains stable as the log content evolves.R/module-loadpage-ui.R (1)
218-218: ⚡ Quick winConstrain these uploaders to the formats the new readers actually support.
After removing the separator controls, these paths now assume
fread-compatible inputs, butaccept = NULLstill lets users pick arbitrary files and only fail later in the server path. Please set per-input filters such as.csv/.tsv/.txt, and include.parquet/.pqfor DIANN.Possible direction
- fileInput(ns('skylinedata'), "", multiple = FALSE, accept = NULL) + fileInput(ns('skylinedata'), "", multiple = FALSE, + accept = c(".csv", ".tsv", ".txt")) - fileInput(ns('dianndata'), "", multiple = FALSE, accept = NULL) + fileInput(ns('dianndata'), "", multiple = FALSE, + accept = c(".csv", ".tsv", ".parquet", ".pq"))Also applies to: 242-242, 264-264, 274-274
🤖 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 `@R/module-loadpage-ui.R` at line 218, Change the fileInput controls so they only allow the reader-supported extensions instead of accept = NULL: for uploaders that feed the fread-compatible readers (e.g. the fileInput(ns('data') ...) and the other fileInput controls referenced at the same spots) set accept to CSV/TSV/text extensions like ".csv", ".tsv", ".txt"; for the DIANN-specific uploader(s) also include Parquet extensions ".parquet" and ".pq". Update each fileInput(...) accept parameter accordingly so users can only pick supported file types.tests/testthat/test-utils.R (1)
795-807: ⚡ Quick winThe second Sky
getDataCode()assertion is redundant.Line 804–806 repeats the same input and expectation as Line 800–803, so it doesn’t add coverage anymore.
Suggested cleanup
test_that("get data code filetype sky", { suppressWarnings({ @@ mock_input$DDA_DIA <- "LType" output <- getDataCode(mock_input) expect_type(output,"character") - - mock_input$DDA_DIA <- "LType" - output <- getDataCode(mock_input) - expect_type(output,"character") }) })🤖 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 `@tests/testthat/test-utils.R` around lines 795 - 807, The test contains a redundant duplicate assertion for getDataCode: remove the repeated block that sets mock_input$DDA_DIA <- "LType", calls getDataCode(mock_input), and expect_type(output, "character") so only the first invocation/assertion remains; if you intended to exercise a different branch, replace the duplicate with a different DDA_DIA value or an additional expectation instead. Ensure the unique symbol getDataCode and the mock_input setup remain unchanged.
🤖 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/utils.R`:
- Around line 865-867: The generated PTM MSstats snippet sets ptm_data and
global_data but returns a list using PROTEIN = unmod which is undefined; update
the emitted list in the branch where input$BIO == "PTM" (the code that appends
to variable codes) to reference the correct variable name (global_data or unmod
as appropriate) — e.g., replace PROTEIN = unmod with PROTEIN = global_data (or
rename global_data to unmod in the snippet) so the list uses a defined symbol
(refer to input$BIO, codes, ptm_data, global_data, unmod, and PROTEIN).
---
Outside diff comments:
In `@tests/testthat/test-utils.R`:
- Around line 462-468: Remove the hardcoded separator assignments in the
migrated tests so they no longer force legacy UI state: in the test block named
"dda pd" (and the other affected test_that blocks where mock_input$sep = ","
appears) delete the lines that set mock_input$sep = "," so the tests rely on
fread auto-detection; ensure you only remove the sep assignments and leave
mock_input$BIO, mock_input$DDA_DIA, mock_input$filetype etc. intact (look for
usages of mock_input and the test_that titles to locate each occurrence).
---
Nitpick comments:
In `@R/module-loadpage-ui.R`:
- Line 218: Change the fileInput controls so they only allow the
reader-supported extensions instead of accept = NULL: for uploaders that feed
the fread-compatible readers (e.g. the fileInput(ns('data') ...) and the other
fileInput controls referenced at the same spots) set accept to CSV/TSV/text
extensions like ".csv", ".tsv", ".txt"; for the DIANN-specific uploader(s) also
include Parquet extensions ".parquet" and ".pq". Update each fileInput(...)
accept parameter accordingly so users can only pick supported file types.
In `@R/plotting_funtions.R`:
- Around line 138-139: The code currently uses data.table::fread(finalfile)
which auto-detects delimiter/header and can produce unstable shapes; change the
call that populates processout (the fread invoked on finalfile in
plotting_funtions.R) to parse the expected .log layout explicitly — e.g., call
data.table::fread(finalfile, sep="\t", header=FALSE, fill=TRUE, quote="") (or
the exact delimiter/header settings for your log format) before converting to
matrix so processout remains stable as the log content evolves.
In `@tests/testthat/test-utils.R`:
- Around line 795-807: The test contains a redundant duplicate assertion for
getDataCode: remove the repeated block that sets mock_input$DDA_DIA <- "LType",
calls getDataCode(mock_input), and expect_type(output, "character") so only the
first invocation/assertion remains; if you intended to exercise a different
branch, replace the duplicate with a different DDA_DIA value or an additional
expectation instead. Ensure the unique symbol getDataCode and the mock_input
setup remain unchanged.
🪄 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: 7905482c-ed26-44b0-9359-b21b20120a60
📒 Files selected for processing (12)
DESCRIPTIONNAMESPACER/MSstatsShiny.RR/module-loadpage-server.RR/module-loadpage-ui.RR/module-visualize-network-server.RR/plotting_funtions.RR/utils.Rman/MSstatsShiny.Rdtests/testthat/ex.Rtests/testthat/test-module-loadpage-ui.Rtests/testthat/test-utils.R
💤 Files with no reviewable changes (2)
- NAMESPACE
- tests/testthat/test-module-loadpage-ui.R
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation and Context
The PR standardizes file-reading across MSstatsShiny by switching from base R readers (read.csv/read.delim/read.table) and manual separator UI controls to data.table::fread, which auto-detects delimiters for CSV/TSV uploads. This simplifies the UI and validation logic, reduces duplication, and makes generated reproducible code and tests consistent with the runtime behavior.
Short Summary of the Solution
Replace base file readers with data.table::fread across server loaders, plotting and network code; remove separator-selection UI and related validation/tests; update code generators, roxygen metadata, NAMESPACE, package DESCRIPTION, and tests to reflect the new approach.
Detailed Changes (bullet list)
Core file-reading changes
UI and server validation
Tests and test stubs
Package metadata and docs
Misc
Unit Tests Added or Modified
tests/testthat/ex.R
tests/testthat/test-module-loadpage-ui.R
tests/testthat/test-utils.R
Coding Guidelines Violations