fix(plotting): Fix groupComparisonPlots to render when viewport dims are low#206
fix(plotting): Fix groupComparisonPlots to render when viewport dims are low#206tonywu1999 merged 1 commit intodevelfrom
Conversation
📝 WalkthroughWalkthroughThe pull request excludes AI configuration directories ( Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
tonywu1999
changed the title
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.Rbuildignore (1)
1-11:⚠️ Potential issue | 🟡 MinorInconsistency between PR objectives and files shown.
The PR title indicates this is a fix for "groupComparisonPlots to render when viewport dims are low," and the AI summary mentions "conditionally restricts plot printing in comparison plot generation to non-Plotly runs." However, the files provided for review only contain changes to
.gitignoreand.Rbuildignore(excluding AI tool directories).Are the main plotting-related code changes in other files not included in this review? If so, those files should be the primary focus of review for this PR.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.Rbuildignore around lines 1 - 11, The PR claims to fix rendering in groupComparisonPlots (and conditionally restrict plot printing for non-Plotly runs), but the diff only touches .gitignore/.Rbuildignore; update the PR to include the actual plotting changes (the functions/methods that implement groupComparisonPlots and any plotly-related code paths), or if those changes exist in other commits/branches, add them to this branch/PR so reviewers can inspect functions such as groupComparisonPlots, any helper like renderComparisonPlot or isPlotlyEnabled, and the conditional print logic; ensure the commit/message clearly references the updated functions and that the diff contains the files with the plotting/rendering logic for review.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.Rbuildignore:
- Line 8: Update the .Rbuildignore entry for the appspec file to use anchored
regex like the surrounding entries: replace the unanchored literal "appspec.yml"
with a pattern such as "^appspec\\.yml$" so it matches only the root filename
and remains consistent with entries like "^\\.positai$" and "^\\.claude$"; edit
the existing appspec.yml line accordingly.
---
Outside diff comments:
In @.Rbuildignore:
- Around line 1-11: The PR claims to fix rendering in groupComparisonPlots (and
conditionally restrict plot printing for non-Plotly runs), but the diff only
touches .gitignore/.Rbuildignore; update the PR to include the actual plotting
changes (the functions/methods that implement groupComparisonPlots and any
plotly-related code paths), or if those changes exist in other commits/branches,
add them to this branch/PR so reviewers can inspect functions such as
groupComparisonPlots, any helper like renderComparisonPlot or isPlotlyEnabled,
and the conditional print logic; ensure the commit/message clearly references
the updated functions and that the diff contains the files with the
plotting/rendering logic for review.
🪄 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: bd94cdd6-2d98-435c-ab25-0fc81e2fdb6e
📒 Files selected for processing (3)
.Rbuildignore.gitignoreR/groupComparisonPlots.R
| ^docs$ | ||
| ^pkgdown$ | ||
| appspec.yml No newline at end of file | ||
| appspec.yml |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the previous format of the appspec.yml pattern
git log -p --follow -S "appspec" -- .Rbuildignore | head -30Repository: Vitek-Lab/MSstats
Length of output: 43
🏁 Script executed:
cat -n .RbuildignoreRepository: Vitek-Lab/MSstats
Length of output: 258
Ensure pattern consistency for .Rbuildignore entries.
The appspec.yml pattern on line 8 lacks regex anchors, making it inconsistent with all other entries in the file. Every other pattern uses the ^ prefix (and many use trailing $ as well), including the newly added ^\.positai$ and ^\.claude$ on lines 9-10. The unanchored pattern appspec.yml will match the filename anywhere in the path, whereas the anchored patterns match only at the root.
Consider changing it to ^appspec\.yml$ to match the format of surrounding entries.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.Rbuildignore at line 8, Update the .Rbuildignore entry for the appspec file
to use anchored regex like the surrounding entries: replace the unanchored
literal "appspec.yml" with a pattern such as "^appspec\\.yml$" so it matches
only the root filename and remains consistent with entries like "^\\.positai$"
and "^\\.claude$"; edit the existing appspec.yml line accordingly.
1 participant
PR Type
Bug fix, Other
Description
Skip printing Plotly plot objects
Prevent plot rendering viewport failures
Keep generated plots stored normally
Ignore local AI tool folders
Diagram Walkthrough
File Walkthrough
.Rbuildignore
Ignore local AI tooling directories.Rbuildignore
.positaito ignored build paths.claudeto ignored build pathsgroupComparisonPlots.R
Guard Plotly plots from direct printingR/groupComparisonPlots.R
isPlotlyis falseplots[[i]]Motivation and Context
The
groupComparisonPlotsfunction has issues rendering plots when the viewport has low dimensions, particularly when used in the MSstatsShiny application with Plotly mode enabled. WhenisPlotly = TRUE, the function is intended to generate interactive Plotly visualizations for web-based rendering. However, unconditional printing of ggplot2 plot objects during Plotly rendering can interfere with the rendering pipeline and cause viewport-related failures. The fix conditionally suppresses these print statements when in Plotly mode, allowing the plots to render correctly regardless of viewport dimensions while maintaining full functionality by returning the plots in their respective lists.Changes
.Rbuildignore: Updated to exclude hidden configuration directories.positaiand.claudefrom R package builds, while continuing to excludeappspec.yml. Added trailing newline to file..gitignore: Added.positaipattern to ignore list.R/groupComparisonPlots.R:.plotVolcano()function to conditionally print volcano plots only when!isPlotly(i.e., when not in Plotly rendering mode).plotComparison()function to conditionally print comparison plots only when!isPlotlyisPlotlyflag, preserving all functionalityisPlotlyparameterTesting
No tests were added or modified to verify these changes. The existing test suite in
inst/tinytest/test_groupComparison.Rcovers thegroupComparisonanalysis function but does not include tests for the plotting functions or theisPlotlyparameter behavior.