feature(diann): Enable support for DIANN 2.0 and parquet files#114
feature(diann): Enable support for DIANN 2.0 and parquet files#114tonywu1999 merged 6 commits intodevelfrom
Conversation
|
Warning Rate limit exceeded@tonywu1999 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 14 minutes and 10 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (8)
WalkthroughAdds Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as loadpageUI
participant Server as getData()
participant Arrow as arrow::read_parquet
participant CSV as read.csv
participant DIANN as DIANNtoMSstatsFormat
User->>UI: choose DIANN file + options
UI->>Server: submit file path + diann_2plus + intensity_column
Server->>Server: ext = getFileExtension(path)
alt ext ∈ {.parquet, .pq}
Server->>Arrow: read_parquet(path)
Arrow-->>Server: table
else
Server->>CSV: read.csv(path, ...)
CSV-->>Server: table
end
Server->>Server: set quantificationColumn ("auto" or intensity_column)
Server->>DIANN: DIANNtoMSstatsFormat(table, quantificationColumn=...)
DIANN-->>Server: MSstats-format data
Server-->>UI: data ready
UI-->>User: proceed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
man/MSstatsShiny.Rd (1)
7-8: Fix spelling: “differencially” → “differentially”User-facing title/description contain a typo.
Apply this diff:
-\\title{MSstatsShiny: An R-shiny based package for detecting differencially abundant +\\title{MSstatsShiny: An R-shiny based package for detecting differentially abundantman/loadpageUI.Rd (1)
12-18: Fix return type and tighten description.Docs claim "returns nothing," but loadpageUI returns a shiny.tag.list (confirmed by tests). Update value and polish description.
Apply this diff (regenerate via roxygen in R/module-loadpage-ui.R, do not hand-edit Rd):
-\value{ -This function returns nothing, as it sets up the loadpage UI -} +\value{ +A \code{shiny.tag.list} that defines the Load page UI. +} @@ -\description{ -This function sets up the loadpage UI where it consists of several -options for users to select and upload files. -} +\description{ +Sets up the Load page UI with options for selecting workflows and uploading files. +}R/utils.R (1)
788-797: Update code export (getDataCode) to reflect Parquet and quantificationColumn.The generated “How to” code still assumes CSV-only and lacks quantificationColumn. Align with new DIANN behavior.
- else if(input$filetype == 'diann') { - - codes = paste(codes, "data = read.csv(\"insert your MSstats scheme output from DIANN filepath\", header = TRUE, sep = '\\t')\nannot_file = read.csv(\"insert your annotation filepath\")#Optional\n" - , sep = "") - - codes = paste(codes, "data = DIANNtoMSstatsFormat(data, - annotation = annot_file, #Optional - qvalue_cutoff = 0.01, ## same as default - removeProtein_with1Feature = TRUE, - use_log_file = FALSE)\n", sep = "") - } + else if(input$filetype == 'diann') { + codes = paste0(codes, + "diann_path <- \"insert your DIANN file path (.csv/.tsv/.parquet)\"\n", + "annot_file <- read.csv(\"insert your annotation filepath\") # Optional\n", + "if (grepl(\"\\\\.(parquet|pq)$\", tolower(diann_path))) {\n", + " library(arrow)\n", + " data <- as.data.frame(arrow::read_parquet(diann_path))\n", + "} else {\n", + " data <- read.csv(diann_path, header = TRUE, sep = '\\t')\n", + "}\n", + "data <- DIANNtoMSstatsFormat(data,\n", + " annotation = annot_file, # Optional\n", + " qvalue_cutoff = 0.01,\n", + " removeProtein_with1Feature = TRUE,\n", + " use_log_file = FALSE,\n", + " quantificationColumn = \"auto\")\n" + ) + }
🧹 Nitpick comments (11)
R/MSstatsShiny.R (1)
47-48: Complete the roxygen “internal package” patternYou added
@keywords internaland"_PACKAGE", but@name MSstatsShinyremains. Prefer the canonical pattern without@nameto avoid duplicate/legacy Rd scaffolding.Apply this diff:
-#' @name MSstatsShiny #' @keywords internal "_PACKAGE"R/module-loadpage-ui.R (5)
272-277: Allow Parquet uploads for DIANNExpose Parquet extensions in the file chooser to match backend support.
Apply this diff:
- fileInput(ns('dianndata'), "", multiple = FALSE, accept = NULL), + fileInput(ns('dianndata'), "", multiple = FALSE, accept = c(".csv", ".tsv", ".txt", ".parquet", ".pq")),
114-117: Stale CSS selector for#loadpage-reset1No
reset1control is defined here. Either add the control or drop the style to avoid dead code.
212-214: Condition lists an unreachable filetype'10col'
'10col'is not among the filetype choices; keep conditions aligned with available options.Apply this diff:
- condition = "(input['loadpage-filetype'] =='10col' || input['loadpage-filetype'] =='prog' || input['loadpage-filetype'] =='PD' || input['loadpage-filetype'] =='open'|| + condition = "(input['loadpage-filetype'] =='prog' || input['loadpage-filetype'] =='PD' || input['loadpage-filetype'] =='open'||
486-489: Remove legacyMRFguard or add it to choices
'MRF'is not a selectable filetype; the guard adds noise.Apply this diff:
- condition = "input['loadpage-filetype'] && input['loadpage-DDA_DIA'] == 'LType' && input['loadpage-filetype'] !== 'sample' && input['loadpage-filetype'] !== 'MRF'", + condition = "input['loadpage-filetype'] && input['loadpage-DDA_DIA'] == 'LType' && input['loadpage-filetype'] !== 'sample'",
238-246: Redundant condition duplication
(input['loadpage-BIO'] != 'PTM' && (input['loadpage-BIO'] != 'PTM' ...))repeats the same predicate.Apply this diff:
- condition = "input['loadpage-filetype'] == 'msstats' && (input['loadpage-BIO'] != 'PTM' && (input['loadpage-BIO'] != 'PTM' && input['loadpage-DDA_DIA'] != 'TMT'))", + condition = "input['loadpage-filetype'] == 'msstats' && input['loadpage-BIO'] != 'PTM' && input['loadpage-DDA_DIA'] != 'TMT'",man/loadpageUI.Rd (1)
5-5: Title wording nit.Avoid repeating “UI.” Suggest “Load page UI module for data selection and upload.”
-\title{Loadpage UI module for data selection and upload UI.} +\title{Load page UI module for data selection and upload.}R/utils.R (2)
527-531: Coerce Parquet read to data.frame to match downstream expectations.arrow::read_parquet may not return a base data.frame; DIANNtoMSstatsFormat typically expects a data.frame.
- if (getFileExtension(input$dianndata$name) %in% c("parquet", "pq")) { - data = read_parquet(input$dianndata$datapath) + if (getFileExtension(input$dianndata$name) %in% c("parquet", "pq")) { + data = as.data.frame(read_parquet(input$dianndata$datapath)) } else { data = read.csv(input$dianndata$datapath, sep=input$sep_dianndata) }
539-543: Guard quantificationColumn when DIANN 2+ toggle is off.Avoid passing NULL/empty; fall back to "auto". Also be robust to NULL checkbox.
- quantificationColumn = "auto" - if (!input$diann_2plus) { - quantificationColumn = input$intensity_column - } + quantificationColumn <- "auto" + if (!isTRUE(input$diann_2plus)) { + quantificationColumn <- if (!is.null(input$intensity_column) && nzchar(input$intensity_column)) { + input$intensity_column + } else "auto" + }tests/testthat/test-module-loadpage-ui.R (2)
8-11: Avoid brittle deep indexing into tag tree.Indexing via result[[1]][[4]][[1]] is fragile. Match class/name via HTML string or tagQuery.
- # Should contain a fluidPage as the main structure - expect_equal(result[[1]][[4]][[1]]$name, "div") - expect_true("container-fluid" %in% result[[1]][[4]][[1]]$attribs$class) + # Should contain a fluidPage/container-fluid somewhere in the structure + html_output <- as.character(result) + expect_true(grepl("container-fluid", html_output, fixed = TRUE))
85-109: Minor: clarify what’s asserted for the upload button.The comment says “disabled initially,” but the test only checks presence. Either assert disabled attribute or adjust the comment.
- # Upload button should be disabled initially (using shinyjs) + # Upload button should exist (state handled dynamically by shinyjs)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
DESCRIPTION(1 hunks)NAMESPACE(1 hunks)R/MSstatsShiny.R(1 hunks)R/module-loadpage-ui.R(2 hunks)R/utils.R(2 hunks)man/MSstatsShiny.Rd(1 hunks)man/loadpageUI.Rd(1 hunks)tests/testthat/test-module-loadpage-ui.R(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: Run pr agent on every pull request
🔇 Additional comments (6)
man/MSstatsShiny.Rd (1)
59-59: Docs correctly marked as internalAdding
\keyword{internal}aligns the man page with the internal package pattern.DESCRIPTION (1)
30-30: Arrow import looks goodAdding
arrowto Imports matches the new Parquet support.R/module-loadpage-ui.R (2)
486-492: Verify and update server-side references for the renamed inputs
I wasn’t able to locate any occurrences ofinput$remove,input$remove_1pep_charge, orinput$remove_1featurein the server code. Please manually confirm and update any reactive expressions, observers, or render functions that depend on the “remove” checkbox so they reference the new IDs (remove_1pep_chargeandremove_1feature). This will ensure you don’t introduce undefined behavior or break existing functionality.
489-489: Ignore theuniqe_peptidestypo warning
The only occurrence ofuniqe_peptidesis the UI declaration—there are no references to eitheruniqe_peptidesorunique_peptidesin your server code. If you intended this checkbox to control a “unique peptides” filter, you’ll need to implement the corresponding server-side logic; otherwise you can remove this unused UI element.Likely an incorrect or invalid review comment.
NAMESPACE (1)
51-51: Importingarrow::read_parquetis correctMatches DESCRIPTION and enables Parquet ingestion code paths.
tests/testthat/test-module-loadpage-ui.R (1)
16-35: Namespacing check looks good.Verifies key IDs carry the module prefix. Keep.
| test_that("loadpageUI includes required conditional panels for different workflows", { | ||
| # Test that key conditional panels exist for different analysis types | ||
| result <- loadpageUI("test") | ||
| html_output <- as.character(result) | ||
|
|
||
| # Check for conditional panel conditions that handle different workflows | ||
| # Note: HTML entities encode single quotes as ' | ||
| expected_conditions <- c( | ||
| "input['loadpage-filetype'] == 'sample'", # Sample data panels | ||
| "input['loadpage-BIO'] != 'PTM'", # Non-PTM workflows | ||
| "input['loadpage-filetype'] == 'maxq'", # MaxQuant workflow | ||
| "input['loadpage-DDA_DIA'] == 'TMT'", # TMT labeling | ||
| "input['loadpage-filetype'] == 'sky'" # Skyline workflow | ||
| ) | ||
|
|
||
| for(condition in expected_conditions) { | ||
| expect_true(grepl(condition, html_output, fixed = TRUE), | ||
| info = paste("Missing conditional panel for:", condition)) | ||
| } | ||
| }) |
There was a problem hiding this comment.
Do not hardcode 'loadpage-' in conditionalPanel conditions.
If the module id is "test", conditions should reference test-, not loadpage-. This test may mask real namespacing issues.
- expected_conditions <- c(
- "input['loadpage-filetype'] == 'sample'", # Sample data panels
- "input['loadpage-BIO'] != 'PTM'", # Non-PTM workflows
- "input['loadpage-filetype'] == 'maxq'", # MaxQuant workflow
- "input['loadpage-DDA_DIA'] == 'TMT'", # TMT labeling
- "input['loadpage-filetype'] == 'sky'" # Skyline workflow
- )
+ ns <- shiny::NS("test")
+ expected_conditions <- c(
+ paste0("input['", ns(\"filetype\"), "'] == 'sample'"),
+ paste0("input['", ns(\"BIO\"), "'] != 'PTM'"),
+ paste0("input['", ns(\"filetype\"), "'] == 'maxq'"),
+ paste0("input['", ns(\"DDA_DIA\"), "'] == 'TMT'"),
+ paste0("input['", ns(\"filetype\"), "'] == 'sky'")
+ )Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In tests/testthat/test-module-loadpage-ui.R around lines 64 to 83, the test
hardcodes conditionalPanel strings with the prefix 'loadpage-' which will fail
when the module id is different; update the test to build expected_conditions
dynamically from the module id used in the call (e.g., use the id variable
"test" to prefix each control name or use Shiny::NS to generate namespaced IDs),
ensuring you still match the HTML-encoded single quotes (') in the expected
strings before running grepl.
There was a problem hiding this comment.
I'll deal with this in another PR, namespace is chaos here.
There was a problem hiding this comment.
@tonywu1999 Understood! Since you'll be addressing this in a separate PR, would you like me to create a GitHub issue to track this testing improvement? It would help ensure the hardcoded 'loadpage-' conditional panel conditions get properly updated to use dynamic namespacing.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
R/utils.R (1)
198-198: Confirm DESCRIPTION/NAMESPACE consistency for arrow.Since read_parquet is imported, ensure arrow is listed in DESCRIPTION Imports to satisfy R CMD check.
Run:
#!/bin/bash rg -nP '^(Imports|Suggests):' DESCRIPTION rg -nP '^\s*arrow\b' DESCRIPTION rg -nP 'importFrom\(\s*arrow,\s*read_parquet\s*\)' NAMESPACE
🧹 Nitpick comments (1)
R/utils.R (1)
518-523: Simplify DIANN parquet detection and standardize output
input$dianndatais single-file (multiple=FALSE), sogetFileExtension(input$dianndata$name)always returns length 1—drop the extra length guard. Wrapread_parquet()inas.data.frame()to guarantee a data.frame across arrow versions. Consider updating the DIANN example ingetDataCode()to mention parquet support.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
DESCRIPTION(1 hunks)NAMESPACE(2 hunks)R/module-loadpage-ui.R(2 hunks)R/utils.R(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- NAMESPACE
- R/module-loadpage-ui.R
- DESCRIPTION
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (1)
R/utils.R (1)
192-196: NA-safe, normalized extension extraction — LGTM.Handles NULL/NA/empty and normalizes via tools::file_ext; good improvement.
| mydata = DIANNtoMSstatsFormat(data, | ||
| annotation = getAnnot(input), | ||
| qvalue_cutoff = qvalue_cutoff, | ||
| MBR = MBR, | ||
| removeProtein_with1Feature = TRUE, | ||
| removeFewMeasurements = FALSE, | ||
| use_log_file = FALSE | ||
| use_log_file = FALSE, | ||
| quantificationColumn = quantificationColumn | ||
| ) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Guard for MSstats versions lacking quantificationColumn; pass conditionally.
Older DIANNtoMSstatsFormat may not accept quantificationColumn. Prevent runtime error by building args dynamically (or bump MSstats minimal version).
Apply:
- mydata = DIANNtoMSstatsFormat(data,
- annotation = getAnnot(input),
- qvalue_cutoff = qvalue_cutoff,
- MBR = MBR,
- removeProtein_with1Feature = TRUE,
- removeFewMeasurements = FALSE,
- use_log_file = FALSE,
- quantificationColumn = quantificationColumn
- )
+ diann_args = list(
+ data,
+ annotation = getAnnot(input),
+ qvalue_cutoff = qvalue_cutoff,
+ MBR = MBR,
+ removeProtein_with1Feature = TRUE,
+ removeFewMeasurements = FALSE,
+ use_log_file = FALSE
+ )
+ if ("quantificationColumn" %in% names(formals(DIANNtoMSstatsFormat))) {
+ diann_args$quantificationColumn = quantificationColumn
+ }
+ mydata = do.call(DIANNtoMSstatsFormat, diann_args)Alternatively, ensure DESCRIPTION pins MSstats to a version that supports quantificationColumn; verify with:
🏁 Script executed:
#!/bin/bash
rg -nP '^Imports:' DESCRIPTION
rg -nP '^\s*MSstats\b' DESCRIPTIONLength of output: 242
Guard the quantificationColumn argument in DIANNtoMSstatsFormat or pin MSstats’s minimal version
Build the call arguments dynamically and only add quantificationColumn if it’s in the function formals (see diff below), or update DESCRIPTION to import MSstats at a version that supports this parameter.
- mydata = DIANNtoMSstatsFormat(data,
- annotation = getAnnot(input),
- qvalue_cutoff = qvalue_cutoff,
- MBR = MBR,
- removeProtein_with1Feature = TRUE,
- removeFewMeasurements = FALSE,
- use_log_file = FALSE,
- quantificationColumn = quantificationColumn
- )
+ diann_args = list(
+ data,
+ annotation = getAnnot(input),
+ qvalue_cutoff = qvalue_cutoff,
+ MBR = MBR,
+ removeProtein_with1Feature = TRUE,
+ removeFewMeasurements = FALSE,
+ use_log_file = FALSE
+ )
+ if ("quantificationColumn" %in% names(formals(DIANNtoMSstatsFormat))) {
+ diann_args$quantificationColumn = quantificationColumn
+ }
+ mydata = do.call(DIANNtoMSstatsFormat, diann_args)Alternatively, in DESCRIPTION:
- Imports: …, MSstats,
+ Imports: …, MSstats (>= 4.0.0),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| mydata = DIANNtoMSstatsFormat(data, | |
| annotation = getAnnot(input), | |
| qvalue_cutoff = qvalue_cutoff, | |
| MBR = MBR, | |
| removeProtein_with1Feature = TRUE, | |
| removeFewMeasurements = FALSE, | |
| use_log_file = FALSE | |
| use_log_file = FALSE, | |
| quantificationColumn = quantificationColumn | |
| ) | |
| # Build DIANNtoMSstatsFormat args and include quantificationColumn only if supported | |
| diann_args = list( | |
| data, | |
| annotation = getAnnot(input), | |
| qvalue_cutoff = qvalue_cutoff, | |
| MBR = MBR, | |
| removeProtein_with1Feature = TRUE, | |
| removeFewMeasurements = FALSE, | |
| use_log_file = FALSE | |
| ) | |
| if ("quantificationColumn" %in% names(formals(DIANNtoMSstatsFormat))) { | |
| diann_args$quantificationColumn = quantificationColumn | |
| } | |
| mydata = do.call(DIANNtoMSstatsFormat, diann_args) |
🤖 Prompt for AI Agents
In R/utils.R around lines 535 to 543, the call to DIANNtoMSstatsFormat
unconditionally passes quantificationColumn which breaks on older MSstats;
either guard this argument or pin MSstats. Modify the call to build arguments
dynamically (e.g., create a list of args and only add quantificationColumn if
"quantificationColumn" is present in names(formals(DIANNtoMSstatsFormat)), then
call DIANNtoMSstatsFormat via do.call with that args list), or alternatively
update DESCRIPTION to import MSstats at a minimal version that supports
quantificationColumn (bump Imports/Depends with >= the required MSstats
version).
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
tests/testthat/test-module-loadpage-ui.R (1)
64-83: Do not hardcode 'loadpage-' in conditionalPanel conditions; derive from NS(id)Use the module id used in this test ("test") to build expected conditions so the test actually validates namespacing.
- expected_conditions <- c( - "input['loadpage-filetype'] == 'sample'", # Sample data panels - "input['loadpage-BIO'] != 'PTM'", # Non-PTM workflows - "input['loadpage-filetype'] == 'maxq'", # MaxQuant workflow - "input['loadpage-DDA_DIA'] == 'TMT'", # TMT labeling - "input['loadpage-filetype'] == 'sky'" # Skyline workflow - ) + ns <- shiny::NS("test") + expected_conditions <- c( + sprintf("input['%s'] == 'sample'", ns("filetype")), + sprintf("input['%s'] != 'PTM'", ns("BIO")), + sprintf("input['%s'] == 'maxq'", ns("filetype")), + sprintf("input['%s'] == 'TMT'", ns("DDA_DIA")), + sprintf("input['%s'] == 'sky'", ns("filetype")) + )
🧹 Nitpick comments (1)
tests/testthat/test-module-loadpage-ui.R (1)
201-201: Use shiny::NS consistentlyBe explicit to avoid accidental masking and to make intent clear.
- controls <- create_main_selection_controls(NS("test")) + controls <- create_main_selection_controls(shiny::NS("test")) @@ - selection <- create_label_free_type_selection(NS("test")) + selection <- create_label_free_type_selection(shiny::NS("test")) @@ - uploads <- create_standard_uploads(NS("test")) + uploads <- create_standard_uploads(shiny::NS("test")) @@ - uploads <- create_msstats_uploads(NS("test")) + uploads <- create_msstats_uploads(shiny::NS("test")) @@ - uploads <- create_ptm_fragpipe_uploads(NS("test")) + uploads <- create_ptm_fragpipe_uploads(shiny::NS("test")) @@ - uploads <- create_maxquant_uploads(NS("test")) + uploads <- create_maxquant_uploads(shiny::NS("test")) @@ - uploads <- create_ptm_uploads(NS("test")) + uploads <- create_ptm_uploads(shiny::NS("test")) @@ - options <- create_processing_options(NS("test")) + options <- create_processing_options(shiny::NS("test")) @@ - options <- create_quality_filtering_options(NS("test")) + options <- create_quality_filtering_options(shiny::NS("test")) @@ - buttons <- create_separator_buttons(NS("test"), "sep_test") + buttons <- create_separator_buttons(shiny::NS("test"), "sep_test")Also applies to: 231-231, 247-247, 262-262, 279-279, 300-300, 314-314, 332-332, 343-343, 355-355
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
tests/testthat/test-module-loadpage-ui.R(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (2)
tests/testthat/test-module-loadpage-ui.R (2)
16-35: LGTM: namespaced ID presence checks are solid
134-151: Tests call internal helpers; confirm export or use :::/getFromNamespaceIf helpers like create_header_content() are internal (@nord, not exported), direct calls will fail under R CMD check. Options:
- Use MSstatsShiny:::create_header_content(), etc.
- Or define a local helper once:
ns_get <- function(nm) getFromNamespace(nm, "MSstatsShiny")Then call ns_get("create_header_content")(). Let me know if you want a patch to update all invocations.
Also applies to: 153-170, 171-197, 199-227, 229-244, 245-260, 261-276, 277-297, 298-311, 312-329, 330-340, 341-352, 353-367, 368-386, 388-402
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (11)
tests/testthat/test-module-loadpage-ui.R (11)
248-261: Add explicit Shiny namespace prefix.Apply this diff for consistency:
- uploads <- create_msstats_uploads(NS("test")) + uploads <- create_msstats_uploads(shiny::NS("test"))
264-282: Add explicit Shiny namespace prefix.Apply this diff for consistency:
- uploads <- create_ptm_fragpipe_uploads(NS("test")) + uploads <- create_ptm_fragpipe_uploads(shiny::NS("test"))
285-296: Add explicit Shiny namespace prefix.Apply this diff for consistency:
- uploads <- create_maxquant_uploads(NS("test")) + uploads <- create_maxquant_uploads(shiny::NS("test"))
299-314: Add explicit Shiny namespace prefix.Apply this diff for consistency:
- uploads <- create_ptm_uploads(NS("test")) + uploads <- create_ptm_uploads(shiny::NS("test"))
317-325: Add explicit Shiny namespace prefix throughout.Multiple test functions use unqualified
NS()calls.Apply this diff to add consistent namespacing:
- options <- create_processing_options(NS("test")) + options <- create_processing_options(shiny::NS("test"))- options <- create_quality_filtering_options(NS("test")) + options <- create_quality_filtering_options(shiny::NS("test"))- buttons <- create_separator_buttons(NS("test"), "sep_test") + buttons <- create_separator_buttons(shiny::NS("test"), "sep_test")- controls <- create_main_selection_controls(NS("test")) + controls <- create_main_selection_controls(shiny::NS("test"))- controls <- create_main_selection_controls(NS("test")) + controls <- create_main_selection_controls(shiny::NS("test"))Also applies to: 328-337, 340-352, 355-372, 375-387
110-118: Prefix Shiny functions to avoid relying on search path.The code calls
fluidPageand potentially other Shiny functions without explicit namespace qualification, but there's no evidence oflibrary(shiny)being called in the test environment.Apply this diff to add explicit namespacing:
- ui_output <- loadpageUI("test") + ui_output <- loadpageUI("test") + # Note: Verify fluidPage calls in helper functions use shiny:: prefixSince this affects the create_test_app helper (which isn't shown in this file), ensure all Shiny function calls use
shiny::prefix or addlibrary(shiny)to a test helper file.
169-175: Fix list-to-string coercion in CSS content validation.The
paste()function on a list of tags doesn't reliably render HTML content. The children should be properly converted to character strings first.Apply this diff to fix the coercion:
- children_html <- paste(css_children, collapse = " ") - expect_true(grepl("background-color:orange", children_html)) - expect_true(grepl("proceed1", children_html)) - expect_true(grepl("reset1", children_html)) - expect_true(grepl("style.css", children_html)) + children_html <- paste(vapply(css_children, as.character, character(1)), collapse = " ") + expect_true(grepl("background-color:orange", children_html)) + expect_true(grepl("proceed1", children_html)) + expect_true(grepl("reset1", children_html)) + expect_true(grepl("style\\.css", children_html))
186-212: Use explicit shiny:: namespace prefix consistently.The
NS("test")call should be prefixed withshiny::to avoid dependency on the search path.Apply this diff:
- controls <- create_main_selection_controls(NS("test")) + controls <- create_main_selection_controls(shiny::NS("test"))
217-229: Fix hardcoded namespace in conditional logic assertions.The test checks for "loadpage-BIO" and "loadpage-filetype" without considering the actual namespace used in the test.
Apply this diff to use proper namespacing:
- selection <- create_label_free_type_selection(NS("test")) + selection <- create_label_free_type_selection(shiny::NS("test")) selection_html <- as.character(selection) expect_true(grepl("shiny-panel-conditional", selection_html)) @@ -225,8 +225,9 @@ expect_true(grepl("SRM/PRM", selection_html)) # Check conditional logic - expect_true(grepl("loadpage-BIO", selection_html)) - expect_true(grepl("loadpage-filetype", selection_html)) + ns <- shiny::NS("test") + expect_true(grepl(ns("BIO"), selection_html, fixed = TRUE)) + expect_true(grepl(ns("filetype"), selection_html, fixed = TRUE))
232-245: Fix hardcoded namespace and improve ID matching precision.The test uses hardcoded "loadpage-filetype" and very loose token matching that may match unrelated content.
Apply this diff to improve precision:
- uploads <- create_standard_uploads(NS("test")) + uploads <- create_standard_uploads(shiny::NS("test")) uploads_html <- as.character(uploads) expect_true(grepl("Upload quantification dataset", uploads_html)) expect_true(grepl("shiny-input-file", uploads_html)) expect_true(grepl("test-data", uploads_html)) - # Check conditional logic for multiple file types - expect_true(grepl("loadpage-filetype", uploads_html)) - expect_true(grepl("prog", uploads_html)) - expect_true(grepl("PD", uploads_html)) - expect_true(grepl("phil", uploads_html)) + # Check for namespaced filetype reference and specific file type tokens + expect_true(grepl(shiny::NS("test")("filetype"), uploads_html, fixed = TRUE)) + expect_true(grepl("id=\\\"test-(prog|PD|phil)\\\"", uploads_html))
390-395: Update test expectations for DIANN Parquet support.Based on the PR objectives, DIANN 2.0 adds support for parquet files, which may legitimately include accept attributes. The current test may fail if parquet support is properly implemented.
Apply this diff to accommodate parquet support:
- # Test CSV-only inputs - annot_input <- create_standard_annotation_uploads(NS("test")) + # Test CSV-only inputs + annot_input <- create_standard_annotation_uploads(shiny::NS("test")) annot_html <- as.character(annot_input) - expect_true(grepl('accept=.*csv', annot_html)) + # Annotation files should still be CSV-only + expect_true(grepl('accept=.*csv', annot_html)) + # But standard uploads may now include parquet for DIANN + standard_input <- create_standard_uploads(shiny::NS("test")) + standard_html <- as.character(standard_input) + expect_true(grepl("\\.parquet", standard_html) || grepl("\\.csv", standard_html))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
tests/testthat/test-module-loadpage-ui.R(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
| test_that("loadpageUI returns a valid tagList with fluidPage structure", { | ||
| # Test basic function execution and structure | ||
| result <- loadpageUI("test") | ||
|
|
||
| # Should return a tagList | ||
| expect_s3_class(result, "shiny.tag.list") | ||
|
|
||
| result_html = as.character(result) | ||
| expect_true(grepl("div", result_html)) | ||
|
|
||
| # Should not be NULL or empty | ||
| expect_true(length(result) > 0) | ||
| }) | ||
|
|
||
| test_that("loadpageUI generates correct namespaced input IDs", { | ||
| # Test that all input elements use proper namespacing | ||
| result <- loadpageUI("testmodule") | ||
| html_output <- as.character(result) | ||
|
|
||
| # Check for key namespaced input IDs that should be present | ||
| expected_ids <- c( | ||
| "testmodule-BIO", # Biological question radio buttons | ||
| "testmodule-DDA_DIA", # Label type radio buttons | ||
| "testmodule-filetype", # File type radio buttons | ||
| "testmodule-proceed1" # Upload button | ||
| ) | ||
|
|
||
| for(id in expected_ids) { | ||
| expect_true( | ||
| grepl(id, html_output, fixed = TRUE), | ||
| info = paste("Missing namespaced ID:", id) | ||
| ) | ||
| } | ||
| }) | ||
|
|
||
| test_that("loadpageUI contains all required radio button choices", { | ||
| # Test that essential radio button options are present | ||
| result <- loadpageUI("test") | ||
| html_output <- as.character(result) | ||
|
|
||
| # Check biological question options | ||
| bio_options <- c("Protein", "Peptide", "PTM") | ||
| for(option in bio_options) { | ||
| expect_true(grepl(option, html_output), | ||
| info = paste("Missing biological option:", option)) | ||
| } | ||
|
|
||
| # Check label type options | ||
| label_options <- c("Label-Free", "TMT") | ||
| for(option in label_options) { | ||
| expect_true(grepl(option, html_output), | ||
| info = paste("Missing label option:", option)) | ||
| } | ||
|
|
||
| # Check file type options (sample a few key ones) | ||
| file_options <- c("MaxQuant", "Skyline", "MSstats Format") | ||
| for(option in file_options) { | ||
| expect_true(grepl(option, html_output), | ||
| info = paste("Missing file type option:", option)) | ||
| } | ||
| }) | ||
|
|
||
| test_that("loadpageUI includes required conditional panels for different workflows", { | ||
| # Test that key conditional panels exist for different analysis types | ||
| result <- loadpageUI("test") | ||
| html_output <- as.character(result) | ||
|
|
||
| # Check for conditional panel conditions that handle different workflows | ||
| # Note: HTML entities encode single quotes as ' | ||
| expected_conditions <- c( | ||
| "input['loadpage-filetype'] == 'sample'", # Sample data panels | ||
| "input['loadpage-BIO'] != 'PTM'", # Non-PTM workflows | ||
| "input['loadpage-filetype'] == 'maxq'", # MaxQuant workflow | ||
| "input['loadpage-DDA_DIA'] == 'TMT'", # TMT labeling | ||
| "input['loadpage-filetype'] == 'sky'" # Skyline workflow | ||
| ) | ||
|
|
||
| for(condition in expected_conditions) { | ||
| expect_true(grepl(condition, html_output, fixed = TRUE), | ||
| info = paste("Missing conditional panel for:", condition)) | ||
| } | ||
| }) | ||
|
|
||
| test_that("loadpageUI properly handles file input elements and validation", { | ||
| # Test that file inputs are properly configured | ||
| result <- loadpageUI("test") | ||
| html_output <- as.character(result) | ||
|
|
||
| # Should contain file input elements | ||
| expect_true(grepl('type="file"', html_output), | ||
| "No file input elements found") | ||
|
|
||
| # Upload button should be disabled initially (using shinyjs) | ||
| expect_true(grepl("proceed1", html_output), | ||
| "Upload button not found") | ||
|
|
||
| # Check for separator radio buttons for file parsing | ||
| expect_true(grepl("sep_data", html_output) || grepl("Column separator", html_output), | ||
| "File separator options not found") | ||
|
|
||
| # Should include help text and external links | ||
| expect_true(grepl("User Guide", html_output), | ||
| "Help documentation links not found") | ||
|
|
||
| # Should include file size warnings | ||
| expect_true(grepl("250 MB", html_output), | ||
| "File size limit warning not found") | ||
| }) | ||
|
|
||
| # Test suite for loadpage UI module | ||
| test_that("loadpageUI creates proper structure", { | ||
| # Test the main UI structure | ||
| ui_output <- loadpageUI("test") | ||
|
|
||
| expect_s3_class(ui_output, "shiny.tag.list") | ||
| expect_true(any(grepl("container-fluid", as.character(ui_output)))) | ||
| expect_true(any(grepl("Upload data", as.character(ui_output)))) | ||
| }) | ||
|
|
||
| # Tests for create_header_content() | ||
| test_that("create_header_content includes required elements", { | ||
| header <- create_header_content() | ||
| header_html <- as.character(header) | ||
|
|
||
| # Check for key text content | ||
| expect_true(grepl("MSstats Pipeline", header_html)) | ||
| expect_true(grepl("User Guide", header_html)) | ||
| expect_true(grepl("MSstatsPTM", header_html)) | ||
| expect_true(grepl("CSV/TSV format", header_html)) | ||
| expect_true(grepl("250 MB", header_html)) | ||
|
|
||
| # Check for external links | ||
| expect_true(grepl("msstats.org", header_html)) | ||
| expect_true(grepl("bioconductor.org", header_html)) | ||
| expect_true(grepl('target="_blank"', header_html)) | ||
| }) | ||
|
|
||
| # Tests for create_sample_dataset_descriptions() | ||
| test_that("create_sample_dataset_descriptions creates conditional panels", { | ||
| descriptions <- create_sample_dataset_descriptions() | ||
| descriptions_html <- as.character(descriptions) | ||
|
|
||
| # Check for conditional panels | ||
| expect_true(grepl("shiny-panel-conditional", descriptions_html)) | ||
|
|
||
| # Check for specific dataset references | ||
| expect_true(grepl("DDA acquisition", descriptions_html)) | ||
| expect_true(grepl("DIA acquisition", descriptions_html)) | ||
| expect_true(grepl("SRM/PRM acquisition", descriptions_html)) | ||
|
|
||
| # Check for publication links | ||
| expect_true(grepl("Choi, M. et al", descriptions_html)) | ||
| expect_true(grepl("Selevsek, N. et al", descriptions_html)) | ||
| expect_true(grepl("Picotti, P. et al", descriptions_html)) | ||
| }) | ||
|
|
||
| # Tests for create_css_styling() | ||
| test_that("create_css_styling includes required CSS", { | ||
| css <- create_css_styling() | ||
|
|
||
| # Check that it's a proper tag structure (not character conversion) | ||
| expect_s3_class(css, "shiny.tag") | ||
| expect_equal(css$name, "head") | ||
|
|
||
| # Check the children elements contain the expected content | ||
| css_children <- css$children | ||
| expect_true(length(css_children) >= 3) # Should have 3 children (2 styles + 1 link) | ||
|
|
||
| # Convert children to string to check content | ||
| children_html <- paste(css_children, collapse = " ") | ||
| expect_true(grepl("background-color:orange", children_html)) | ||
| expect_true(grepl("proceed1", children_html)) | ||
| expect_true(grepl("reset1", children_html)) | ||
| expect_true(grepl("style.css", children_html)) | ||
|
|
||
| # Check that it's a head tag with proper structure | ||
| expect_true(any(sapply(css_children, function(x) { | ||
| if (is.list(x) && !is.null(x$name)) { | ||
| return(x$name == "style" || x$name == "link") | ||
| } | ||
| FALSE | ||
| }))) | ||
| }) | ||
|
|
||
| # Tests for create_main_selection_controls() | ||
| test_that("create_main_selection_controls creates proper radio buttons", { | ||
| controls <- create_main_selection_controls(NS("test")) | ||
| controls_html <- as.character(controls) | ||
|
|
||
| # Check for biological question options | ||
| expect_true(grepl("Biological Question", controls_html)) | ||
| expect_true(grepl("Protein", controls_html)) | ||
| expect_true(grepl("Peptide", controls_html)) | ||
| expect_true(grepl("PTM", controls_html)) | ||
|
|
||
| # Check for label type options | ||
| expect_true(grepl("Label Type", controls_html)) | ||
| expect_true(grepl("Label-Free", controls_html)) | ||
| expect_true(grepl("TMT", controls_html)) | ||
|
|
||
| # Check for file type options | ||
| expect_true(grepl("Type of File", controls_html)) | ||
| expect_true(grepl("MSstats Format", controls_html)) | ||
| expect_true(grepl("Skyline", controls_html)) | ||
| expect_true(grepl("MaxQuant", controls_html)) | ||
| expect_true(grepl("FragPipe", controls_html)) | ||
| expect_true(grepl("DIANN", controls_html)) | ||
|
|
||
| # Check for proper namespace | ||
| expect_true(grepl("test-BIO", controls_html)) | ||
| expect_true(grepl("test-DDA_DIA", controls_html)) | ||
| expect_true(grepl("test-filetype", controls_html)) | ||
| }) | ||
|
|
||
| # Tests for create_label_free_type_selection() | ||
| test_that("create_label_free_type_selection creates conditional panel", { | ||
| selection <- create_label_free_type_selection(NS("test")) | ||
| selection_html <- as.character(selection) | ||
|
|
||
| expect_true(grepl("shiny-panel-conditional", selection_html)) | ||
| expect_true(grepl("Type of Label-Free type", selection_html)) | ||
| expect_true(grepl("DDA", selection_html)) | ||
| expect_true(grepl("DIA", selection_html)) | ||
| expect_true(grepl("SRM/PRM", selection_html)) | ||
|
|
||
| # Check conditional logic | ||
| expect_true(grepl("loadpage-BIO", selection_html)) | ||
| expect_true(grepl("loadpage-filetype", selection_html)) | ||
| }) | ||
|
|
||
| # Tests for create_standard_uploads() | ||
| test_that("create_standard_uploads creates file input with conditions", { | ||
| uploads <- create_standard_uploads(NS("test")) | ||
| uploads_html <- as.character(uploads) | ||
|
|
||
| expect_true(grepl("Upload quantification dataset", uploads_html)) | ||
| expect_true(grepl("shiny-input-file", uploads_html)) | ||
| expect_true(grepl("test-data", uploads_html)) | ||
|
|
||
| # Check conditional logic for multiple file types | ||
| expect_true(grepl("loadpage-filetype", uploads_html)) | ||
| expect_true(grepl("prog", uploads_html)) | ||
| expect_true(grepl("PD", uploads_html)) | ||
| expect_true(grepl("phil", uploads_html)) | ||
| }) | ||
|
|
||
| # Tests for create_msstats_uploads() | ||
| test_that("create_msstats_uploads creates different inputs for regular and PTM", { | ||
| uploads <- create_msstats_uploads(NS("test")) | ||
| uploads_html <- as.character(uploads) | ||
|
|
||
| # Check for regular MSstats format | ||
| expect_true(grepl("Upload data in MSstats Format", uploads_html)) | ||
| expect_true(grepl("test-msstatsdata", uploads_html)) | ||
|
|
||
| # Check for PTM MSstats format | ||
| expect_true(grepl("Upload PTM data in MSstats Format", uploads_html)) | ||
| expect_true(grepl("test-msstatsptmdata", uploads_html)) | ||
| expect_true(grepl("Upload unmodified data in MSstats Format", uploads_html)) | ||
| expect_true(grepl("test-unmod", uploads_html)) | ||
| }) | ||
|
|
||
| # Tests for create_ptm_fragpipe_uploads() | ||
| test_that("create_ptm_fragpipe_uploads creates comprehensive PTM options", { | ||
| uploads <- create_ptm_fragpipe_uploads(NS("test")) | ||
| uploads_html <- as.character(uploads) | ||
|
|
||
| # Check for required uploads | ||
| expect_true(grepl("Upload PTM msstats dataset", uploads_html)) | ||
| expect_true(grepl("Upload PTM annotation file", uploads_html)) | ||
| expect_true(grepl("Upload global profiling msstats dataset", uploads_html)) | ||
| expect_true(grepl("Upload global profiling annotation file", uploads_html)) | ||
|
|
||
| # Check for processing options | ||
| expect_true(grepl("modification id column", uploads_html)) | ||
| expect_true(grepl("localization_cutoff", uploads_html)) | ||
| expect_true(grepl("Remove unlocalized peptides", uploads_html)) | ||
|
|
||
| # Check default values | ||
| expect_true(grepl("STY", uploads_html)) | ||
| expect_true(grepl("\\.75", uploads_html)) | ||
| }) | ||
|
|
||
| # Tests for create_maxquant_uploads() | ||
| test_that("create_maxquant_uploads creates proper file inputs", { | ||
| uploads <- create_maxquant_uploads(NS("test")) | ||
| uploads_html <- as.character(uploads) | ||
|
|
||
| expect_true(grepl("Upload evidence.txt File", uploads_html)) | ||
| expect_true(grepl("Upload proteinGroups.txt File", uploads_html)) | ||
| expect_true(grepl("Upload annotation File", uploads_html)) | ||
|
|
||
| expect_true(grepl("test-evidence", uploads_html)) | ||
| expect_true(grepl("test-pGroup", uploads_html)) | ||
| expect_true(grepl("test-annot1", uploads_html)) | ||
| }) | ||
|
|
||
| # Tests for create_ptm_uploads() | ||
| test_that("create_ptm_uploads creates PTM-specific inputs", { | ||
| uploads <- create_ptm_uploads(NS("test")) | ||
| uploads_html <- as.character(uploads) | ||
|
|
||
| # Check for PTM specific uploads | ||
| expect_true(grepl("Upload PTM input.txt File", uploads_html)) | ||
| expect_true(grepl("Upload fasta File", uploads_html)) | ||
| expect_true(grepl("Upload Unmodified Protein input.txt File", uploads_html)) | ||
|
|
||
| # Check for modification labels | ||
| expect_true(grepl("Modification Label", uploads_html)) | ||
| expect_true(grepl("FASTA file column name", uploads_html)) | ||
|
|
||
| # Check default values | ||
| expect_true(grepl("uniprot_iso", uploads_html)) | ||
| }) | ||
|
|
||
| # Tests for create_processing_options() | ||
| test_that("create_processing_options creates TMT and label-free options", { | ||
| options <- create_processing_options(NS("test")) | ||
| options_html <- as.character(options) | ||
|
|
||
| # Check for processing options structure | ||
| expect_true(grepl("Select the options for pre-processing", options_html)) | ||
| expect_true(grepl("Use unique peptides", options_html)) | ||
| expect_true(grepl("Remove proteins with 1", options_html)) | ||
| }) | ||
|
|
||
| # Tests for create_quality_filtering_options() | ||
| test_that("create_quality_filtering_options creates filtering controls", { | ||
| options <- create_quality_filtering_options(NS("test")) | ||
| options_html <- as.character(options) | ||
|
|
||
| expect_true(grepl("Filter with Q-value", options_html)) | ||
| expect_true(grepl("Filter with M-score", options_html)) | ||
| expect_true(grepl("Q-value cutoff", options_html)) | ||
| expect_true(grepl("M-score cutoff", options_html)) | ||
| expect_true(grepl("MBR Enabled", options_html)) | ||
| }) | ||
|
|
||
| # Tests for create_separator_buttons() | ||
| test_that("create_separator_buttons creates proper radio buttons", { | ||
| buttons <- create_separator_buttons(NS("test"), "sep_test") | ||
| buttons_html <- as.character(buttons) | ||
|
|
||
| expect_true(grepl("Column separator", buttons_html)) | ||
| expect_true(grepl("Comma", buttons_html)) | ||
| expect_true(grepl("Semicolon", buttons_html)) | ||
| expect_true(grepl("Tab", buttons_html)) | ||
| expect_true(grepl("Pipe", buttons_html)) | ||
|
|
||
| # Check for inline display | ||
| expect_true(grepl("radio-inline", buttons_html)) | ||
| }) | ||
|
|
||
| # Test order preservation in main selection controls | ||
| test_that("main selection controls maintain proper order", { | ||
| controls <- create_main_selection_controls(NS("test")) | ||
| controls_html <- as.character(controls) | ||
|
|
||
| # Find positions of each section | ||
| bio_pos <- regexpr("Biological Question", controls_html) | ||
| label_pos <- regexpr("Label Type", controls_html) | ||
| file_pos <- regexpr("Type of File", controls_html) | ||
|
|
||
| # Verify correct order | ||
| expect_true(bio_pos < label_pos) | ||
| expect_true(label_pos < file_pos) | ||
|
|
||
| # Verify numbered headings are in order | ||
| expect_true(grepl("1\\. Biological Question", controls_html)) | ||
| expect_true(grepl("2\\. Label Type", controls_html)) | ||
| expect_true(grepl("3\\. Type of File", controls_html)) | ||
| }) | ||
|
|
||
| # Test tooltip content is preserved | ||
| test_that("tooltips contain proper explanatory text", { | ||
| controls <- create_main_selection_controls(NS("test")) | ||
| controls_html <- as.character(controls) | ||
|
|
||
| # Check for tooltip explanations | ||
| expect_true(grepl("Select the biological question of interest", controls_html)) | ||
| expect_true(grepl("Label-free will process all label-free acquisitions", controls_html)) | ||
| expect_true(grepl("Choose the spectral processing tool used", controls_html)) | ||
|
|
||
| # Check for icon-wrapper and icon-tooltip classes | ||
| expect_true(grepl("icon-wrapper", controls_html)) | ||
| expect_true(grepl("icon-tooltip", controls_html)) | ||
| }) | ||
|
|
||
| # Test file input configurations | ||
| test_that("file inputs have proper accept attributes", { | ||
| # Test CSV-only inputs | ||
| annot_input <- create_standard_annotation_uploads(NS("test")) | ||
| annot_html <- as.character(annot_input) | ||
| expect_true(grepl('accept=.*csv', annot_html)) | ||
| }) No newline at end of file |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Missing verification scripts for namespace consistency.
The tests have several namespace-related issues that should be verified across the codebase to ensure the UI functions properly handle different namespace scenarios.
Run the following script to verify namespace consistency:
🏁 Script executed:
#!/bin/bash
# Description: Verify that loadpageUI and helper functions handle namespacing consistently
echo "=== Checking for loadpage UI functions and namespace usage ==="
ast-grep --pattern $'function($id) {
ns <- NS($id)
$$$
}'
echo "=== Finding conditional panel conditions that reference input names ==="
rg -nP 'input\[.*?\].*?==' --type=r -A2 -B2
echo "=== Checking for hardcoded loadpage- references in UI code ==="
rg -n 'loadpage-' --type=r -g '!test*'
echo "=== Verifying shiny:: namespace usage in test files ==="
rg -n 'NS\(' tests/testthat/ -A1 -B1Length of output: 43822
Replace hardcoded “loadpage-” prefixes in all conditionalPanel conditions with dynamic ns() calls
In module-loadpage-ui.R, update every condition = "input['loadpage-XXX'] …" to build the ID via ns("XXX") (e.g. paste0("input['", ns("filetype"), "'] == 'sample'")), and adjust the corresponding tests in test-module-loadpage-ui.R to assert against the actual namespaced IDs returned by loadpageUI("test").
🤖 Prompt for AI Agents
In tests/testthat/test-module-loadpage-ui.R lines 1-395, the tests expect
conditionalPanel conditions and namespaced IDs to use hardcoded "loadpage-*"
prefixes; update the UI module (module-loadpage-ui.R) to build conditionalPanel
condition strings with ns(), e.g. paste0("input['", ns("filetype"), "'] ==
'sample'"), and then change the tests to assert against the runtime namespaced
IDs produced by calling loadpageUI("test") (or using NS("test") where
appropriate) instead of checking for "loadpage-*" literals so tests validate the
actual namespaced IDs and condition strings.
refactor(loadpage): Refactor loadpage UI to modular subcomponents
1 participant
PR Type
Enhancement, Tests, Documentation
Description
Add DIANN 2.0 and parquet support
Refactor loadpage UI into subcomponents
Introduce DIANN-specific UI options
Add tests for loadpage UI structure
Diagram Walkthrough
File Walkthrough
2 files
Add arrow to Imports for parquet supportImport arrow::read_parquet symbol3 files
Package doc roxygen updated to internalMark package docs as internalAdjust description punctuation2 files
Modularize loadpage UI and add DIANN optionsSupport DIANN parquet and v2.0 quant column1 files
Add tests for loadpage UI structure and IDsSummary by CodeRabbit
New Features
Refactor
Documentation
Tests
Chores