Merged
Conversation
The existing tests reached removeSignifAndCharData directly and called CChart on a fixture that was already one column wide, so nothing exercised the step that reduces a table to a single column in the first place - which is how the reported document gets there. Drive tb.2d.colcmp through PrepareData with select.columns instead, and assert the column name survives into ChartData. Also pins the neighbouring branch: with significance appended, two numeric statistics survive removal, the data stays 3-dimensional, and the column name is kept that way instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The guard added by RS-14165 keeps a single-row table 2-dimensional by transposing the dropped vector, but t() names only the columns - the row is left unnamed. Q labels the series "[1,]" whenever it plots series in rows, which is the natural layout for a one-row table, so the same defect this ticket fixes for columns was still live on the other axis. Selecting one row from a banner table, with or without transposing first, hits it. Restore the row name under the same non-blank guard used for the column name, and extend the tests: the shape matrix now asserts both dimnames for single-row data (and that a row with no name stays unnamed), plus a PrepareData(select.rows) case alongside the select.columns one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Preserves named single-column table dimensions so exported chart legends use the column name rather than the statistic.
Changes:
- Retains row and column names when statistic removal collapses dimensions.
- Adds comprehensive table-shape regression coverage.
- Bumps the package patch version.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
R/cchart.R |
Preserves named singleton dimensions in ChartData. |
tests/testthat/test-stattesting.R |
Adds end-to-end and shape regression tests. |
DESCRIPTION |
Updates version to 1.12.15. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Both branches were doing the same thing - the drop collapsed a length-1 dimension, so rebuild the 2-dimensional shape and keep the names - but they said it in different ways: one transposed then named the row, the other carried its guard in the branch condition and built a matrix. Rebuilding from dim(x)[1:2] and dimnames(x)[1:2] covers both axes at once and subsumes the transpose from RS-14165. The condition reads directly now, so the commentary is down to the part that isn't visible in the code: what Q does when a name is missing, and why an unnamed single column is deliberately left alone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The shape matrix only fed in tables carrying two or more statistics, so three branches went untested. One of them matters: a 3-dimensional table with a single statistic and character data reaches the rebuild this ticket added, and nothing was holding it there. The other two are the early returns - numeric data with nothing to remove, and a single statistic on a 2-dimensional table, which is where one sits once there is no significance data to append. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
JustinCCYap
reviewed
Aug 18, 2026
Review catch from Justin. The guard only asked whether the column had a usable name, which left two holes. A 1x1 table whose column is unnamed dropped to a bare scalar: unlike a single column of many rows, whose row labels survive as the vector's own names, there is nowhere for the row name to go, so it was lost too. A 1-D Q table reduced to one row by select.rows lands exactly here. The other hole was mine: rebuilding from dimnames(x)[1:2] copied a blank or NA row name straight through, where the previous t() would have left the row unnamed and where the same value is rejected on the column side. Filter both dimensions through the same predicate, and rebuild when the vector has no names to carry rather than only when the column is named. The test block claims to cover every table shape, so the unnamed 1x1, the wholly unnamed 1x1, and blank and NA labels on each dimension are now in it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
JustinCCYap
reviewed
Aug 18, 2026
JustinCCYap
approved these changes
Aug 18, 2026
JustinCCYap
left a comment
JustinCCYap
left a comment
Contributor
There was a problem hiding this comment.
LGTM, 2 minor comments
Second review catch from Justin, and he is right about the R semantics: when the
row and column dimnames are both NULL, drop = TRUE takes its names from the only
non-NULL component, which is the statistic's. A 1x1 table with no labels therefore
came back as c("Column %" = value), moving the statistic from the series name to
the category name rather than removing it. Confirmed on R 4.5.1.
A vector result stands for the table's rows, so its names can only be the row
labels. Setting them explicitly clears the inherited statistic name and is a no-op
for the single-column case, where they already are the row labels.
The test for this shape asserted only that the result stays a vector, so it passed
without noticing, and its comment claimed nothing was named. Both corrected.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Third review point from Justin, and Carmen's call. Q fills an absent row label with its "[1,]" placeholder (IndexedLabels.Fill only replaces nulls, so a blank survives as a real label), and "[1,]" in a legend is more conspicuous than an empty entry. So a blank row label is now kept, with NA emptied rather than exported as the text "NA". The two dimensions are treated differently on purpose: an unlabelled column falls back to the statistic name, which is what Q should show for one (RS-3402), so a blank column label there is still worth no more than none. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
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.
Problem
Exporting a page to PowerPoint labels the chart legend
Column %instead of the banner column shown in Displayr (AMA Memberin the ticket,Lapsed 1-6 yearsin the customer's data).Root cause
removeSignifAndCharDatastrips the character statistics and then drops the statistic dimension withx[,,1, drop = TRUE]. When the table has exactly one column, that call drops the column dimension too, taking its name with it and leaving a bare named vector.Q reads a
ChartDatawith no column labels as a genuinely 1-dimensional table and puts the statistic into the single column header (RS-3402,RTable1Or2DAdapter). That header becomes the exported chart's series name, and hence the legend entry.It needs two conditions at once — one column and significance data appended (column comparisons or arrows). Without the significance statistic the data is 2-dimensional and returns early, which is why most charts are unaffected.
Longstanding rather than a regression: the
drop = TRUEdates to RS-12989 (Feb 2023), and RS-14165 added the mirror guard for single-row tables without covering single-column ones.Fix
Keep the data 2-dimensional when the column that was dropped had a name. A table that never had column names is left as a vector, so Q still shows the statistic as its column header (RS-3402) —
test-stattesting.R:684covers that path and catches the over-broad version of this fix.Tests
tb.1col.colcmpnow asserts the"Male"column name survives intoChartData, end to end throughCChart(append.data = TRUE)."ChartData keeps row and column names for every table shape", covering nine shapes so a change to the customer's table can't silently reintroduce this: single column with column comparisons / with arrows, single row, 1x1, multiple columns, multiple numeric statistics retained, NULL column names, blank column name, and no statistic dimension at all.Full suite green (16 files).
Notes
ChartDatais stored with the R output, so this only reaches an existing document once the visualization recalculates.master, which requiresflipTime (>= 2.10.4)— that version is still on the unmerged flipTimeRS-23170branch, so CI here needs it released first.🤖 Generated with Claude Code