Configure editing setup matrix for dose response curves#143
Conversation
WalkthroughReplaced dynamic matrix building with a reactive Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant DT as "DT (editable table)"
participant Obs as "table_cell_edit observer"
participant Func as "update_matrix_from_edit()"
participant State as "contrast$matrix (reactive)"
participant UI as "UI renderer"
User->>DT: edit cell
DT->>Obs: emit table_cell_edit event
Obs->>State: obtain current matrix
Obs->>Func: call with matrix + edit info
Func->>Func: coerce value to column type and update cell
Func-->>Obs: return updated matrix
Obs->>State: assign updated matrix
State->>UI: trigger reactive re-render
UI->>DT: display updated matrix
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
|
Failed to generate code suggestions for PR |
![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
🧹 Nitpick comments (4)
tests/testthat/test-utils-statmodel-server.R (1)
396-404: Consider adding a test for failed type coercion.The
update_matrix_from_editfunction has atryCatchthat keeps the original value if coercion fails. This error path is not tested. Consider adding a test case where an invalid value (e.g.,"abc") is provided for a numeric column to verify the fallback behavior.test_that("update_matrix_from_edit handles invalid coercion gracefully", { mat <- matrix(c(1, 2, 3, 4), nrow = 2, ncol = 2) info <- list(row = 1, col = 2, value = "invalid_text") result <- update_matrix_from_edit(mat, info) # When coercion fails, the string value is assigned (R coerces to NA for numeric) # Verify the function doesn't crash and returns a result expect_true(!is.null(result)) })R/module-statmodel-server.R (3)
218-237: Failed coercion silently assigns potentially incompatible value.When
tryCatchcatches an error, it returns the original string valuev, which is then assigned tomat[i, j]. For a numeric column, this will cause R to coerce the string (likely producingNAwith a warning), which may not be the intended behavior. Consider either:
- Keeping the original cell value on failed coercion, or
- Returning the matrix unchanged with a message to the user.
update_matrix_from_edit = function(mat, info) { i <- info$row j <- info$col v <- info$value if (is.data.frame(mat)) { - v <- tryCatch(as(v, class(mat[[j]])), error = function(e) v) - mat[i, j] <- v + original_value <- mat[i, j] + coerced <- tryCatch(as(v, class(mat[[j]])), error = function(e) NULL) + mat[i, j] <- if (!is.null(coerced)) coerced else original_value } else { - v <- tryCatch(as(v, class(mat[1, 1])), error = function(e) v) - mat[i, j] <- v + original_value <- mat[i, j] + coerced <- tryCatch(as(v, class(mat[1, 1])), error = function(e) NULL) + mat[i, j] <- if (!is.null(coerced)) coerced else original_value } return(mat) }
640-650: Add NULL check forcurrent_matrix.If
input$table_cell_editfires beforecontrast$matrixis initialized (edge case), callingupdate_matrix_from_edit(NULL, ...)could cause unexpected behavior. Consider adding a guard clause.observeEvent(input$table_cell_edit, { current_matrix <- isolate(contrast$matrix) + if (is.null(current_matrix)) return() updated_matrix <- update_matrix_from_edit(current_matrix, input$table_cell_edit) contrast$matrix <- updated_matrix })
673-679: CSS is injected repeatedly insiderenderUI.The
tags$head(tags$style(...))is insiderenderUI, so every time the matrix UI re-renders, a duplicate<style>tag is added to the DOM. While not breaking functionality, this is suboptimal.Consider moving the CSS to the module's UI definition or using a one-time injection pattern.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
R/module-statmodel-server.R(6 hunks)tests/testthat/test-utils-statmodel-server.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 (4)
tests/testthat/test-utils-statmodel-server.R (1)
406-425: LGTM!Good test coverage for the data.frame case with mixed types. The test properly verifies both numeric and character column updates while confirming type preservation.
R/module-statmodel-server.R (3)
293-307: LGTM!The access to
contrast$matrixwithinrenderUIis correct. ReactiveValues members can be accessed directly without parentheses, and the reactive context is established byrenderUI.
618-633: LGTM!The transition from
matrix_build()tocontrast$matrixis consistent with the new editing workflow. Usingreq()ensures proper validation before proceeding.
704-729: LGTM!The usage of
contrast$matrixfor the response curve visualization is consistent with the rest of the implementation.
| editable_options <- list(target = 'cell') | ||
| # Perform a case-insensitive check for the 'GROUP' column for robustness. | ||
| if (any(toupper(colnames(mat)) == "GROUP")) { | ||
| group_col_idx <- which(toupper(colnames(mat)) == "GROUP") | ||
| editable_options$disable <- list(columns = group_col_idx) | ||
| } | ||
|
|
||
| DT::datatable(mat, editable = editable_options, options = list(scrollX = TRUE)) | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
DT::datatable editable disable columns index convention 0-based 1-based
💡 Result:
Use 0-based (JavaScript-style) column indices. The editable:disable columns vector counts from 0, so the first column is 0. Example:
editable = list(target = "cell", disable = list(columns = c(0, 1))) # disables col1 and col2. [1][2]
Sources:
[1] Stack Overflow discussion showing 0-based indices.
[2] DT datatable documentation for the editable argument.
Fix column index conversion for DT's 0-based indexing.
DT::datatable's editable$disable$columns expects 0-based column indices (JavaScript convention), but which() returns 1-based indices. The GROUP column won't be disabled correctly. Subtract 1 from the result: group_col_idx <- which(toupper(colnames(mat)) == "GROUP") - 1.
🤖 Prompt for AI Agents
In R/module-statmodel-server.R around lines 660 to 668, the code uses which() to
find the GROUP column but passes 1-based R indices to DT::datatable
editable$disable$columns which expects 0-based JS indices; change the assignment
to subtract 1 from the which() result (and coerce to integer/vector if
necessary) so editable_options$disable$columns receives 0-based indices (e.g.,
group_col_idx <- which(toupper(colnames(mat)) == "GROUP") - 1) before setting
editable_options$disable.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
R/module-statmodel-server.R (2)
293-307: Fixcontrastscoping and add safety checks in the response-curve drug selector.Inside
render_group_comparison_plot_inputs(),contrastis referenced but never passed in; because this helper is defined outsidestatmodelServer, it won’t see the module’s localcontrast <- reactiveValues(...)and will error (object 'contrast' not found) when the response-curve UI branch is hit. In addition, this block assumes (a)contrast$matrixis already built and (b) it has adrugcolumn and at least one non‑control drug, sounique_drugs_without_control[[1]]can also error.Consider:
-render_group_comparison_plot_inputs = function(output, session, rownames, get_data, input, loadpage_input, condition_list) { +render_group_comparison_plot_inputs = function(output, session, rownames, get_data, input, loadpage_input, condition_list, contrast) { @@ - output[[NAMESPACE_STATMODEL$visualization_response_curve_which_drug]] = renderUI({ - if (input[[NAMESPACE_STATMODEL$visualization_plot_type]] == - CONSTANTS_STATMODEL$plot_type_response_curve) { - response_curve_setup_matrix = contrast$matrix - unique_drugs = unique(response_curve_setup_matrix$drug) - unique_drugs_without_control = unique_drugs[unique_drugs != "DMSO"] - selectInput(session$ns(NAMESPACE_STATMODEL$visualization_response_curve_which_drug), - label = h5("Select Drug"), - unique_drugs_without_control, selected = unique_drugs_without_control[[1]]) - } else { - NULL - } - }) + output[[NAMESPACE_STATMODEL$visualization_response_curve_which_drug]] = renderUI({ + if (input[[NAMESPACE_STATMODEL$visualization_plot_type]] != + CONSTANTS_STATMODEL$plot_type_response_curve) { + return(NULL) + } + # Only valid for the response-curve comparison mode, once the matrix exists. + if (is.null(contrast$matrix) || !"drug" %in% colnames(contrast$matrix)) { + return(NULL) + } + response_curve_setup_matrix <- contrast$matrix + unique_drugs <- unique(response_curve_setup_matrix$drug) + unique_drugs_without_control <- unique_drugs[unique_drugs != "DMSO"] + if (!length(unique_drugs_without_control)) { + return(NULL) + } + selectInput( + session$ns(NAMESPACE_STATMODEL$visualization_response_curve_which_drug), + label = h5("Select Drug"), + choices = unique_drugs_without_control, + selected = unique_drugs_without_control[[1]] + ) + })and update the call site inside
statmodelServerto passcontrast:- render_group_comparison_plot_inputs(output, session, Rownames, get_data, input, loadpage_input, condition_list) + render_group_comparison_plot_inputs(output, session, Rownames, get_data, input, loadpage_input, condition_list, contrast)
705-729: Verify merge key for dose–response preparation (GROUPvsCondition).In the response-curve branch you do:
matrix = contrast$matrix protein_level_data <- merge(preprocess_data()$ProteinLevelData, matrix, by = "GROUP")For some experiment types (notably TMT),
ProteinLevelDataoften hasCondition(and notGROUP) as the grouping column, whilebuild_response_curve_matrix()labels its grouping column asGROUPfromcondition_list. IfProteinLevelDatalacksGROUP, thismerge()will error, and if bothGROUPandConditionexist, you may be merging on the wrong key.Consider selecting the merge column dynamically (and always merging to the
GROUPcolumn of the contrast matrix), e.g.:- matrix = contrast$matrix - protein_level_data <- merge(preprocess_data()$ProteinLevelData, matrix, by = "GROUP") + matrix <- contrast$matrix + protein_level_data_raw <- preprocess_data()$ProteinLevelData + group_col <- if ("GROUP" %in% colnames(protein_level_data_raw)) "GROUP" else "Condition" + protein_level_data <- merge( + protein_level_data_raw, + matrix, + by.x = group_col, + by.y = "GROUP" + )and, if response curves are only valid for certain experiment types, optionally guarding this branch accordingly.
♻️ Duplicate comments (1)
R/module-statmodel-server.R (1)
654-668: Adjust GROUP column index for DT’s 0‑basededitable$disable$columns.
group_col_idx <- which(toupper(colnames(mat)) == "GROUP")produces 1‑based indices, but DT’seditable = list(disable = list(columns = ...))expects 0‑based column indices. As a result, the intended GROUP column isn’t actually locked, and instead some other column becomes non-editable, while GROUP remains editable and can be corrupted.You should convert to 0‑based indices before assigning:
- if (any(toupper(colnames(mat)) == "GROUP")) { - group_col_idx <- which(toupper(colnames(mat)) == "GROUP") - editable_options$disable <- list(columns = group_col_idx) - } + if (any(toupper(colnames(mat)) == "GROUP")) { + group_col_idx <- which(toupper(colnames(mat)) == "GROUP") - 1L + editable_options$disable <- list(columns = as.integer(group_col_idx)) + }As noted in previous review comments, this is required for the GROUP column to be reliably protected from edits.
🧹 Nitpick comments (2)
R/module-qc-server.R (1)
31-45: Protein/peptide standard selectors now use explicit columns; consider guardingget_data().Switching to
unique(get_data()$ProteinName)/unique(get_data()$PeptideSequence)is much safer than positional indexing and aligns with the rest of the code. For robustness against transientNULL/empty states, you might wrap this renderUI with areq(get_data())(and optionallyreq("ProteinName" %in% names(get_data()))) so QC can’t break if the data isn’t fully initialized yet.R/module-statmodel-server.R (1)
670-685: Matrix UI enhancements are helpful; CSS could be scoped more narrowly if desired.The added helper text and black-text CSS override will improve usability when editing the matrix. Note that this
<style>block is injected on every render of this UI; not a functional bug, but if you ever see style collisions elsewhere, consider scoping the selector to this table’s specificidso the rule only affects this matrix.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
R/module-qc-server.R(1 hunks)R/module-statmodel-server.R(7 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 (4)
R/module-statmodel-server.R (4)
211-237:update_matrix_from_edit()cleanly encapsulates DT cell-edit application.Using the 1‑based
info$row/info$colindices directly againstmatand wrapping coercion intryCatchis a good way to keep the app from crashing on bad input while still attempting to preserve column types. The helper is well-factored and should make future DT edit paths easier to maintain.
262-266: Protein selector for visualization now correctly binds toProteinName.Referencing
unique(get_data()$ProteinName)instead of positionalget_data()[[1]]makes this selector resilient to column reordering and matches the rest of the app’s usage ofProteinName. No issues here.
618-633: Modeling now correctly depends oncontrast$matrix(including edits).Switching
data_comparison/data_comparison_codetoreq(contrast$matrix)and then usingmatrix <- contrast$matrix/comp_mat <- contrast$matrixensures the analysis and generated code reflect the latest (possibly user-edited) matrix rather than a stalematrix_build()snapshot. This is the right reactive source to use here.
640-650:table_cell_editobserver safely applies edits without reactive loops.Using
isolate(contrast$matrix)avoids self-triggering the observer whencontrast$matrixis updated, and delegating the actual mutation toupdate_matrix_from_edit()keeps the logic centralized. This wiring looks sound.
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/module-statmodel-server.R (1)
253-265: Guard response-curve drug UI and downstream usage againstcontrast$matrixbeingNULLFor the response-curve path you now rely on
contrast$matrixin multiple places:
- In the drug selector UI:
if (input[[NAMESPACE_STATMODEL$visualization_plot_type]] == CONSTANTS_STATMODEL$plot_type_response_curve) { response_curve_setup_matrix = contrast$matrix unique_drugs = unique(response_curve_setup_matrix$drug) ... }
- And later in plotting:
matrix = contrast$matrix protein_level_data <- merge(preprocess_data()$ProteinLevelData, matrix, by = "GROUP")If the user switches the plot type to Response Curve before building the contrast matrix for that mode,
contrast$matrixwill beNULLandresponse_curve_setup_matrix$drugwill error insiderenderUI. Similarly, the plotting branch assumescontrast$matrixis a non-NULLobject with adrugcolumn.I recommend:
output[[NAMESPACE_STATMODEL$visualization_response_curve_which_drug]] = renderUI({ if (input[[NAMESPACE_STATMODEL$visualization_plot_type]] == CONSTANTS_STATMODEL$plot_type_response_curve) { - response_curve_setup_matrix = contrast$matrix + req(contrast$matrix) + response_curve_setup_matrix = contrast$matrix + req("drug" %in% colnames(response_curve_setup_matrix)) unique_drugs = unique(response_curve_setup_matrix$drug) ... } else { NULL } })and optionally a similar
req(contrast$matrix)/ column check right before themerge()in the plotting branch to fail fast with a clearer error if the matrix hasn’t been configured yet.Also applies to: 294-304, 705-707
♻️ Duplicate comments (1)
R/module-statmodel-server.R (1)
654-668: Fix GROUP-column disable indexing for DT’s 0-basededitable$disable$columnsYou correctly detect the
GROUPcolumn and attempt to disable edits:editable_options <- list(target = 'cell') if (any(toupper(colnames(mat)) == "GROUP")) { group_col_idx <- which(toupper(colnames(mat)) == "GROUP") editable_options$disable <- list(columns = group_col_idx) }However,
DT::datatableexpects 0‑based column indices ineditable$disable$columns, whilewhich()returns 1‑based R indices. As a result, the wrong column is disabled andGROUPremains editable, which can break the response-curve path where you latermerge(..., by = "GROUP").You should subtract 1 from the index:
editable_options <- list(target = 'cell') # Perform a case-insensitive check for the 'GROUP' column for robustness. if (any(toupper(colnames(mat)) == "GROUP")) { - group_col_idx <- which(toupper(colnames(mat)) == "GROUP") - editable_options$disable <- list(columns = group_col_idx) + group_col_idx <- which(toupper(colnames(mat)) == "GROUP") - 1L + editable_options$disable <- list(columns = group_col_idx) }This matches DT’s JS indexing and ensures the GROUP column is actually protected.
In DT for R, what indexing convention (0-based vs 1-based) is used for `editable = list(disable = list(columns = ...))` when specifying which columns to disable?
🧹 Nitpick comments (2)
R/module-statmodel-server.R (2)
211-237: Clarify type-coercion behavior inupdate_matrix_from_editand consider extra safety checksThe abstraction is good, but a couple of details are worth tightening:
- The comment says “If coercion fails, the original value is kept”, but the implementation always writes something into
mat[i, j]. For example,as("foo", "numeric")returnsNA(not an error), so the original numeric value is replaced byNA. If you truly want to preserve the previous value on invalid input, captureold <- mat[i, j]and only overwrite when coercion is successful (and not anNAintroduced by coercion for a non‑empty string).- Similarly, in the matrix branch you may want to mirror this pattern so that a typo doesn’t silently turn into
NAin a numeric matrix.- As a small robustness improvement, you could early-return when
is.null(mat)or wheni/jfall outsidenrow(mat)/ncol(mat)to guard against any out-of-sync edit events.This is not blocking, but would make the edit behavior better match the intent expressed in the comments.
Also applies to: 640-650
673-680: Scope the CSS override to this table to avoid affecting other DT instancesThe CSS override:
tags$head(tags$style(HTML( "table.dataTable td input { color: black !important; }" )))will apply to all editable
DT::datatableinputs in the app, not just this module’s contrast matrix. If that’s intentional, it’s fine; otherwise, consider scoping it to this table’s container, for example:tags$head(tags$style(HTML( paste0("#", ns("table"), " table.dataTable td input { color: black !important; }") )))so other modules can still control their own editing styles.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
R/module-statmodel-server.R(9 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 (1)
R/module-statmodel-server.R (1)
618-627: Good:contrast$matrixas single source of truth for modeling and code generationThe changes to:
data_comparison = eventReactive(input[[...modeling_start]], { req(contrast$matrix) matrix = contrast$matrix ... }) data_comparison_code = eventReactive(input[[...modeling_start]], { req(contrast$matrix) comp_mat = contrast$matrix generate_analysis_code(...) })nicely centralize the contrast definition in
contrast$matrixand ensure modeling and code generation stay in sync with any edits made in the UI. This also avoids reusing a potentially stalematrix_build()result.No issues from my side here.
Also applies to: 629-633
| output[[NAMESPACE_STATMODEL$visualization_which_protein]] = renderUI({ | ||
| selectInput(ns(NAMESPACE_STATMODEL$visualization_which_protein), | ||
| label = h4("which protein to plot"), | ||
| unique(get_data()[[1]])) | ||
| unique(get_data()$ProteinName)) | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check ProteinName and protein_column usage
echo "=== Searching for ProteinName usage ==="
rg -n "ProteinName" -S --type r
echo ""
echo "=== Searching for protein_column configuration ==="
rg -n 'protein_column' -S --type r
echo ""
echo "=== Checking the specific lines mentioned ==="
sed -n '262,266p' R/module-statmodel-server.R
echo "---"
sed -n '695,721p' R/module-statmodel-server.R
echo ""
echo "=== Searching for Protein column usage ==="
rg -n '"Protein"' -S --type r | head -20Repository: Vitek-Lab/MSstatsShiny
Length of output: 8719
🏁 Script executed:
#!/bin/bash
# Check what columns ProteinLevelData contains
echo "=== Searching for ProteinLevelData column definitions ==="
rg -n "ProteinLevelData" -S --type r -A 3 -B 3
echo ""
echo "=== Checking preprocess_data function ==="
rg -n "def preprocess_data\|function.*preprocess" -S --type rRepository: Vitek-Lab/MSstatsShiny
Length of output: 13049
Fix protein selector to use protein-level data column name
The protein selector at lines 262-266 sources choices from get_data()$ProteinName, which is the feature-level protein identifier column. However, at lines 707-721, the response curve analysis uses preprocess_data()$ProteinLevelData with protein_column = "Protein" in MSstatsPrepareDoseResponseFit.
Since ProteinLevelData uses the "Protein" column (not "ProteinName"), the protein names selected by the UI won't match the identifiers available in the prepared data, causing downstream filtering to return empty results.
Change line 265 to:
unique(preprocess_data()$ProteinLevelData$Protein)This same issue applies at lines 695-721 where the response curve branch uses the same selector input.
🤖 Prompt for AI Agents
In R/module-statmodel-server.R around lines 262-266 (and likewise the
response-curve branch around lines 695-721), the selectInput choices are pulled
from get_data()$ProteinName (feature-level column) but the downstream
response-curve analysis and filtering use preprocess_data()$ProteinLevelData
with the "Protein" column; update the selectInput choices to source unique names
from preprocess_data()$ProteinLevelData$Protein so the UI selection matches the
prepared protein-level identifiers used later, and keep the input id/namespace
unchanged.
Co-authored-by: Tony Wu <wu.anthon@northeastern.edu>
tonywu1999
deleted the
Configure-editing-setup-matrix-for-dose-response-curves
branch
2 participants
Making the Matrix editable.
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.