Repository navigation
Conversation
XLSX export kept raw numbers, so Table/Pivot downloads did not match the chart. Map d3 specifiers to Excel formats after the workbook is written. Co-authored-by: Cursor <cursoragent@cursor.com>
Sheet-wide color scales paint labels and blanks. Fill only numeric cells that satisfy the Explore comparator, including a value-based gradient. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| result = polish_explore_xlsx( | ||
| result, df, form_data, include_index=include_index | ||
| ) |
There was a problem hiding this comment.
Suggestion: Pivot XLSX exports using showValuesAs percentages skip display formatting here, leaving fractional values with General formatting instead of showing percentages.
Assessment: 🟠 Major · 🔁 Occurrence: Sometimes · 🏷️ Api mismatch
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/common/query_context_processor.py
**Line:** 575:577
**Comment:**
*Api Mismatch: Pivot XLSX exports using `showValuesAs` percentages skip display formatting here, leaving fractional values with General formatting instead of showing percentages.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of c91cbe4.
The XLSX export now passes the generated workbook, dataframe, and form data to polish_explore_xlsx, which applies Explore display formats and conditional formatting after serialization.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of b811d26.
Pivot XLSX exports in percentage modes now pass number_format="0.0%" to excel.df_to_excel, keeping fractional values numeric while displaying them as percentages.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of efb539a.
Pivot XLSX exports in percentage modes now pass number_format="0.0%" to excel.df_to_excel, keeping fractional values numeric while displaying them as percentages.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of 1f72f99.
Pivot XLSX exports now pass number_format="0.0%" when showValuesAs is a percentage mode, preserving numeric fractions while displaying them as percentages.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of e7def26.
Percentage-mode pivot XLSX exports now pass the 0.0% number format to the Excel writer, keeping fractional values numeric while displaying them as percentages.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of 1899f3f.
Pivot-table XLSX exports with a percentage showValuesAs mode now pass 0.0% as the Excel number format, keeping the values numeric while displaying them as percentages.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of ab40a53.
The XLSX export now sets number_format to 0.0% for pivot-table percentage modes and passes it to df_to_excel.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of 10d5a50.
The XLSX export now sets number_format to 0.0% for pivot-table percentage modes and passes it to df_to_excel.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of 6f44786.
Pivot XLSX exports now pass 0.0% as the number format when showValuesAs is a percentage mode, keeping fractional values numeric while displaying them as percentages.
If that's not right, unresolve this thread and CodeAnt will leave it open.
|
The flagged issue is correct. In To resolve this, you can modify Would you like me to fetch all other comments on this PR to validate and implement fixes for them as well? superset/utils/excel_conditional.py |
| query["data"] = _apply_excel_explore_formats( | ||
| query["data"], | ||
| processed_df, | ||
| form_data, | ||
| viz_type, | ||
| include_index, | ||
| ) |
There was a problem hiding this comment.
Suggestion: processed_df uses verbose column labels while conditional-formatting rules use raw column names, so _column_index cannot find many configured columns and emits no rules.
Assessment: 🟠 Major · 🔁 Occurrence: Sometimes · 🏷️ Api mismatch
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/charts/client_processing.py
**Line:** 1479:1485
**Comment:**
*Api Mismatch: `processed_df` uses verbose column labels while conditional-formatting rules use raw column names, so `_column_index` cannot find many configured columns and emits no rules.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of b811d26.
Excel formatting now passes the datasource verbose_map alongside processed_df to _apply_excel_explore_formats, allowing configured raw column names to resolve against verbose headers.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of efb539a.
Excel formatting now passes the datasource verbose_map alongside processed_df to _apply_excel_explore_formats, allowing configured raw column names to resolve against verbose headers.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of 1f72f99.
Excel formatting now passes verbose_map to _apply_excel_explore_formats, allowing conditional-formatting rules to resolve configured raw column names against verbose export headers.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of fa97cb6.
The Excel formatting call now passes the datasource verbose_map alongside processed_df, allowing configured raw column names to resolve against verbose headers.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of 6cf5cf0.
The XLSX formatting path now passes the datasource verbose_map into _apply_excel_explore_formats, allowing configured raw column names to be resolved against verbose export headers.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of 6775ff6.
The XLSX formatting call now passes the datasource verbose_map to _apply_excel_explore_formats, allowing formatting rules to resolve verbose column labels.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of e7def26.
The Excel formatting pipeline now receives the datasource verbose_map, allowing configured raw column names to be matched against exported headers.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of 1899f3f.
The XLSX formatting call now forwards the datasource verbose_map to _apply_excel_explore_formats so configured columns can be matched against displayed headers.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of 6f44786.
The Excel formatting helper now receives the datasource verbose_map and datasource data, allowing formatting rules to resolve verbose export headers.
If that's not right, unresolve this thread and CodeAnt will leave it open.
| if object_fmt == _OBJECT_CELL_BAR: | ||
| _add_data_bar(sheet, cell_range, rgb) | ||
| return |
There was a problem hiding this comment.
Suggestion: A cell-bar rule's comparator is discarded, so a rule such as > 10 displays bars for every value instead of only matching values.
Assessment: 🟠 Major · 🔁 Occurrence: Sometimes · 🏷️ Logic error
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/utils/excel_conditional.py
**Line:** 237:239
**Comment:**
*Logic Error: A cell-bar rule's comparator is discarded, so a rule such as `>` 10 displays bars for every value instead of only matching values.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of b811d26.
Cell-bar rules now derive a matching range via _matching_cell_range(sheet, col_idx, header_row, rule) before adding the data bar, so the rule comparator is applied.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of efb539a.
Cell-bar rules now derive a matching range via _matching_cell_range(sheet, col_idx, header_row, rule) before adding the data bar, so the rule comparator is applied.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of 1f72f99.
Cell-bar rules now derive bar_range through _matching_cell_range(..., rule), so the rule metadata, including its comparator and threshold, is applied when selecting cells for the bars.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of fa97cb6.
Cell-bar rules now derive their bar range through _matching_cell_range(..., rule), so the rule’s comparator and threshold are applied instead of formatting the entire column.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of 6775ff6.
Cell-bar rules now compute a matching cell range from the rule and add data bars only to that range.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of e7def26.
Cell-bar rules now pass the rule to _matching_cell_range and apply bars only to the resulting matching range.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of 1899f3f.
Cell-bar rules now pass the full rule to _matching_cell_range and apply bars only to the range returned for that rule.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of ab40a53.
Cell-bar rules now pass through _matching_cell_range with the rule, and the data bar is applied only to the returned range.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of 10d5a50.
Cell-bar rules now pass through _matching_cell_range with the rule, and the data bar is applied only to the returned range.
If that's not right, unresolve this thread and CodeAnt will leave it open.
| sheet.conditional_formatting.add( | ||
| cell_range, | ||
| ColorScaleRule( | ||
| start_type="min", | ||
| start_color=_SCALE_LOW, | ||
| end_type="max", | ||
| end_color=rgb, | ||
| ), |
There was a problem hiding this comment.
Suggestion: Color-scale rules discard configured minBound and maxBound, causing Excel to scale against the data range instead of the bounds used by Explore.
Assessment: 🟠 Major · 🔁 Occurrence: Sometimes · 🏷️ Incomplete implementation
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/utils/excel_conditional.py
**Line:** 279:286
**Comment:**
*Incomplete Implementation: Color-scale rules discard configured `minBound` and `maxBound`, causing Excel to scale against the data range instead of the bounds used by Explore.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of b811d26.
Color-scale rules now convert minBound and maxBound to numeric values and pass them as start_value and end_value, using min/max only when bounds are absent.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of efb539a.
Color-scale rules now convert minBound and maxBound to numeric values and pass them as start_value and end_value, using min/max only when bounds are absent.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of 1f72f99.
Color-scale rules now convert minBound and maxBound to numeric values and pass them as start_value and end_value with matching num bound types.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of fa97cb6.
Color-scale rules now parse minBound and maxBound and pass them as numeric start and end values to ColorScaleRule.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of e7def26.
Color-scale rules now parse minBound and maxBound and use the configured values as numeric start and end bounds.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of 1899f3f.
Color-scale rules now use configured minBound and maxBound as numeric start and end values, falling back to Excel’s minimum and maximum when bounds are absent.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of ab40a53.
Color-scale rules now use configured minBound and maxBound as numeric start and end values when present.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of 10d5a50.
Color-scale rules now use configured minBound and maxBound as numeric start and end values when present.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of 6f44786.
Color-scale rules now use configured maxBound for greater-than rules and minBound for less-than rules; rules without an operator are also passed to the color-scale helper.
If that's not right, unresolve this thread and CodeAnt will leave it open.
| currency = config.get("currencyFormat") | ||
| currency = currency if isinstance(currency, dict) else None | ||
| number_format = d3_number_to_excel( | ||
| config.get("d3NumberFormat"), currency | ||
| ) or d3_time_to_excel(config.get("d3TimeFormat")) |
There was a problem hiding this comment.
Suggestion: Table exports ignore datasource-level columnFormats and currencyFormats, so columns without explicit column_config lose formats that the chart still displays.
Assessment: 🟠 Major · 🔁 Occurrence: Sometimes · 🏷️ Api mismatch
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/utils/excel_display.py
**Line:** 171:175
**Comment:**
*Api Mismatch: Table exports ignore datasource-level `columnFormats` and `currencyFormats`, so columns without explicit `column_config` lose formats that the chart still displays.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of e7def26.
When explicit column configuration lacks formats, datasource currencyFormats and columnFormats now supply the currency and number formats.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of 1899f3f.
Datasource currencyFormats and columnFormats are now used as fallbacks when building each column’s Excel number format.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of ab40a53.
Datasource currencyFormats and columnFormats are now used as fallbacks when column-level currency or number formats are missing.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of 10d5a50.
Datasource currencyFormats and columnFormats are now used as fallbacks when column-level currency or number formats are missing.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of 6f44786.
Datasource currencyFormats and columnFormats are now used as fallbacks when column-level configuration does not provide those formats.
If that's not right, unresolve this thread and CodeAnt will leave it open.
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #3b06a2
Actionable Suggestions - 1
-
superset/utils/excel_conditional.py - 1
- AttributeError on plain Index · Line 351-351
Additional Suggestions - 6
-
tests/unit_tests/utils/test_excel_conditional.py - 2
-
Missing helper docstring · Line 27-27The new helper `_cf_rules` (line 27) has type hints but no docstring. BITO.md rules [12147]/[12490] require every new helper function to document its purpose and behavior.
-
Missing test docstrings · Line 35-113The four new test functions (`test_apply_conditional_formatting_writes_cell_is_rule`, `test_apply_cell_bars_when_show_cell_bars_and_no_custom_rules`, `test_cell_bar_object_writes_data_bar_rule`, `test_color_scale_when_operator_is_none`) have no docstrings. Repo rule [12148]/[12490] requires every new test function to document its purpose and expected outcome.
-
-
superset/utils/excel_conditional.py - 4
-
Duplicated theme color literal · Line 125-125`_rule_color` hardcodes the fallback literal `"52C41A"`, duplicating `_NAMED_COLORS["success"]` (line 57). If the theme green changes in the map, this default silently keeps the old color and downloads diverge from Explore. Return `_NAMED_COLORS["success"]` or a named `_DEFAULT_COLOR` constant instead of the bare hex string.
-
Magic 20-row sampling window · Line 291-291`_is_numeric_header` samples only `min(sheet.max_row, header_row + 20)` rows; the literal 20 is an undocumented business threshold. A text column whose first 20 data cells are blank is misclassified as numeric and gets a data bar. Name the constant and document the tradeoff, or scan all data rows.
-
Inconsistent nlevels guarding · Line 353-353Line 353 guards the same hazard with `getattr(df.index, "nlevels", 1)` while line 351 accesses `df.columns.nlevels` unguarded — inconsistent handling of the identical pandas-attribute risk within one function. Unifying both on the `getattr(..., 1)` form removes the crash and prevents the next contributor from copying the unguarded pattern.
-
Silent header_rows coercion · Line 328-328`max(header_rows, 1)` silently coerces `header_rows=0` to 1. No current caller passes 0, so this only masks future caller bugs; raising `ValueError` for non-positive `header_rows` would surface mistakes at the call site. Low priority.
-
Review Details
-
Files reviewed - 7 · Commit Range:
467c610..f71ab3b- superset/charts/client_processing.py
- superset/common/query_context_processor.py
- superset/utils/excel_conditional.py
- superset/utils/excel_display.py
- tests/unit_tests/charts/test_client_processing.py
- tests/unit_tests/utils/test_excel_conditional.py
- tests/unit_tests/utils/test_excel_display.py
-
Files skipped - 0
-
Tools
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers an incremental AI Review. -
/review full- Manually triggers a full AI Review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
| if viz_type not in ("table", "pivot_table_v2"): | ||
| return workbook_bytes | ||
|
|
||
| header_rows = df.columns.nlevels if isinstance(df.columns, pd.MultiIndex) else 1 |
There was a problem hiding this comment.
polish_explore_xlsx evaluates df.columns.nlevels whenever df.columns is not a pd.MultiIndex, but a plain pandas Index (built when include_index adds index columns, as from query_context_processor.py and client_processing.py) has no nlevels attribute, so XLSX downloads raise AttributeError instead of returning a file. Use getattr(df.columns, "nlevels", 1) as line 353 already does for df.index.
Code Review Run #3b06a2
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
…ting-export' into feature/excel-conditional-formatting-export
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #44664 +/- ##
==========================================
+ Coverage 84.91% 84.95% +0.03%
==========================================
Files 3075 3080 +5
Lines 207844 209635 +1791
Branches 48995 49318 +323
==========================================
+ Hits 176494 178094 +1600
- Misses 27989 28099 +110
- Partials 3361 3442 +81
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Code Review Agent Run #6fb400
Actionable Suggestions - 2
-
tests/unit_tests/utils/test_excel_conditional.py - 1
- Weak substring range assertions · Line 102-104
-
superset/utils/excel_conditional.py - 1
- ColorScale num with None val · Line 399-410
Additional Suggestions - 4
-
tests/unit_tests/utils/test_excel_conditional.py - 1
-
New helper missing docstring · Line 36-36`_workbook_with_sales` is a new helper but lacks a docstring, while `_cf_rules` and every test function in this file have one; BITO rule 12147 requires docstrings on all new Python functions including test-file helpers. Add a one-line docstring describing the workbook shape it builds.
-
-
superset/utils/excel_conditional.py - 3
-
Numeric string equality mismatch · Line 316-316In `_value_matches_rule`, the `str(value) == str(target)` fallback re-parses nothing but discards the already-computed `number`/`target_number`: for numeric-looking strings like "1,234" vs "1234" the string compare fails even though `_as_number` parsed both successfully earlier. Comparing the parsed floats (falling back to string equality only when parsing failed) matches the comparator's numeric intent.
-
Per-rule reverse map scan · Line 213-215`_rule_column_aliases` scans every entry of `verbose_map` for a reverse (verbose→raw) match on each `_apply_rule` call. With large datasource column counts this is O(columns) per rule per export; a forward lookup plus one reverse scan per export (or a prebuilt reverse map) would keep aliasing cheap. Minor, but it runs inside the per-rule loop.
-
getattr weakens type check · Line 480-480Replacing the `isinstance(df.columns, pd.MultiIndex)` check with `getattr(df.columns, "nlevels", 1)` changes behavior for non-MultiIndex columns: a flat `Index` also exposes `.nlevels` (always 1, so benign), but any object with an unrelated `nlevels` attribute would now be trusted. The explicit type check was stricter and self-documenting; the getattr form silently accepts duck-typed impostors.
-
Filtered by Review Rules
Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
-
superset/common/query_context_processor.py - 1
- Duplicated percent format logic · Line 572-578
Review Details
-
Files reviewed - 5 · Commit Range:
f71ab3b..efb539a- superset/charts/client_processing.py
- superset/common/query_context_processor.py
- superset/utils/excel_conditional.py
- tests/unit_tests/common/test_query_context_processor.py
- tests/unit_tests/utils/test_excel_conditional.py
-
Files skipped - 0
-
Tools
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers an incremental AI Review. -
/review full- Manually triggers a full AI Review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
| assert "A2" in cf_range | ||
| assert "A4" in cf_range | ||
| assert "A3" not in cf_range |
There was a problem hiding this comment.
These substring assertions cannot detect the regression this test exists for: a fallback to the full column range A2:A4 (e.g. _matching_cell_range returning _data_range) passes all three checks, since "A3" is not a substring of "A2:A4", while "A2" also matches "A20:A25". Verified with an openpyxl 3.1.5 round-trip. Compare exact range tokens instead: assert cf_range.split() == ["A2", "A4"].
Code suggestion
Check the AI-generated fix before applying
| assert "A2" in cf_range | |
| assert "A4" in cf_range | |
| assert "A3" not in cf_range | |
| assert cf_range.split() == ["A2", "A4"] |
Code Review Run #6fb400
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| min_bound = _as_number(rule.get("minBound")) | ||
| max_bound = _as_number(rule.get("maxBound")) | ||
| start_type = "num" if min_bound is not None else "min" | ||
| end_type = "num" if max_bound is not None else "max" | ||
| sheet.conditional_formatting.add( | ||
| cell_range, | ||
| ColorScaleRule( | ||
| start_type=start_type, | ||
| start_value=min_bound, | ||
| start_color=_SCALE_LOW, | ||
| end_type=end_type, | ||
| end_value=max_bound, |
There was a problem hiding this comment.
start_value=min_bound / end_value=max_bound are passed even when the bound is None and the type falls back to "min"/"max". openpyxl 3.1.5's ColorScaleRule wraps every non-None type in a FormatObject(type=..., val=...) with no validation, so a type="num" cfvo with val=None can be emitted (e.g. minBound absent but maxBound="abc"), producing a color scale Excel may reject or misread. Pass the value only when the type is "num".
Code Review Run #6fb400
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
There was a problem hiding this comment.
Code Review Agent Run #214642
Actionable Suggestions - 1
-
tests/unit_tests/utils/test_excel_conditional.py - 1
- Missing test docstrings · Line 209-399
Additional Suggestions - 1
-
tests/unit_tests/utils/test_excel_conditional.py - 1
-
Duplicated data-bar filter · Line 302-307The `data_bars = [rng for rng, rule in _cf_rules(...) if rule.type == "dataBar"]` comprehension is written out three times in this diff (lines 302-306, 322-326, 395) plus a tuple variant at 108. Extract a `_data_bar_ranges(sheet)` helper next to `_cf_rules` so the data-bar expectation stays consistent if `_cf_rules` output shape changes again.
-
Review Details
-
Files reviewed - 5 · Commit Range:
efb539a..6cf5cf0- superset/common/query_context_processor.py
- superset/utils/excel_conditional.py
- superset/utils/excel_display.py
- tests/unit_tests/utils/test_excel_conditional.py
- tests/unit_tests/utils/test_excel_display.py
-
Files skipped - 0
-
Tools
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers an incremental AI Review. -
/review full- Manually triggers a full AI Review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
| def test_returns_original_bytes_when_nothing_to_apply() -> None: | ||
| raw = _workbook_with_sales(10) | ||
| assert apply_conditional_formatting(raw, []) is raw | ||
|
|
||
|
|
||
| def test_missing_column_does_not_add_rules() -> None: | ||
| styled = apply_conditional_formatting( | ||
| _workbook_with_sales(10), | ||
| [{"column": "missing", "operator": ">", "targetValue": 1}], | ||
| ) | ||
| assert _cf_rules(load_workbook(io.BytesIO(styled)).active) == [] | ||
|
|
||
|
|
||
| def test_range_operator_writes_formula_rule() -> None: | ||
| styled = apply_conditional_formatting( | ||
| _workbook_with_sales(5), | ||
| [ | ||
| { | ||
| "column": "sales", | ||
| "operator": "≤ x ≤", | ||
| "targetValueLeft": 0, | ||
| "targetValueRight": 10, | ||
| "colorScheme": "#00FF00", | ||
| } | ||
| ], | ||
| ) | ||
| cf_range, rule = _cf_rules(load_workbook(io.BytesIO(styled)).active)[0] | ||
| assert "A2" in cf_range | ||
| assert rule.type == "expression" | ||
| formula = "".join(str(part) for part in (rule.formula or [])) | ||
| assert "AND(" in formula | ||
| assert "A2>=0" in formula | ||
| assert "A2<=10" in formula | ||
|
|
||
|
|
||
| def test_text_color_cell_is_uses_font_not_fill() -> None: | ||
| styled = apply_conditional_formatting( | ||
| _workbook_with_sales(12), | ||
| [ | ||
| { | ||
| "column": "sales", | ||
| "operator": ">", | ||
| "targetValue": 8, | ||
| "colorScheme": "#FF0000", | ||
| "objectFormatting": "TEXT_COLOR", | ||
| } | ||
| ], | ||
| ) | ||
| _, rule = _cf_rules(load_workbook(io.BytesIO(styled)).active)[0] | ||
| assert rule.type == "cellIs" | ||
| assert rule.dxf is not None | ||
| assert rule.dxf.font is not None | ||
| assert str(rule.dxf.font.color.rgb).endswith("FF0000") | ||
| assert rule.dxf.fill is None | ||
|
|
||
|
|
||
| def test_pivot_sum_header_matches_rule_column() -> None: | ||
| workbook = Workbook() | ||
| sheet = workbook.active | ||
| sheet["A1"] = "SUM(sales)" | ||
| sheet["A2"] = 12 | ||
| raw = io.BytesIO() | ||
| workbook.save(raw) | ||
| styled = apply_conditional_formatting( | ||
| raw.getvalue(), | ||
| [ | ||
| { | ||
| "column": "sales", | ||
| "operator": ">", | ||
| "targetValue": 8, | ||
| "colorScheme": "#FF0000", | ||
| } | ||
| ], | ||
| ) | ||
| rules = _cf_rules(load_workbook(io.BytesIO(styled)).active) | ||
| assert len(rules) == 1 | ||
| assert "A2" in rules[0][0] | ||
|
|
||
|
|
||
| def test_cell_bar_range_operator_covers_contiguous_matches() -> None: | ||
| styled = apply_conditional_formatting( | ||
| _workbook_with_sales(1, 5, 15), | ||
| [ | ||
| { | ||
| "column": "sales", | ||
| "operator": "< x <", | ||
| "targetValueLeft": 0, | ||
| "targetValueRight": 10, | ||
| "objectFormatting": "CELL_BAR", | ||
| "colorScheme": "#5AC189", | ||
| } | ||
| ], | ||
| ) | ||
| data_bars = [ | ||
| rng | ||
| for rng, rule in _cf_rules(load_workbook(io.BytesIO(styled)).active) | ||
| if rule.type == "dataBar" | ||
| ] | ||
| assert data_bars == ["A2:A3"] | ||
|
|
||
|
|
||
| def test_cell_bar_without_operator_covers_full_column() -> None: | ||
| styled = apply_conditional_formatting( | ||
| _workbook_with_sales(12.5, 5, 20), | ||
| [ | ||
| { | ||
| "column": "sales", | ||
| "operator": None, | ||
| "objectFormatting": "CELL_BAR", | ||
| "colorScheme": "#5AC189", | ||
| } | ||
| ], | ||
| ) | ||
| data_bars = [ | ||
| rng | ||
| for rng, rule in _cf_rules(load_workbook(io.BytesIO(styled)).active) | ||
| if rule.type == "dataBar" | ||
| ] | ||
| assert data_bars == ["A2:A4"] | ||
|
|
||
|
|
||
| def test_color_scale_without_bounds_uses_min_max() -> None: | ||
| styled = apply_conditional_formatting( | ||
| _workbook_with_sales(10), | ||
| [{"column": "sales", "operator": None, "colorScheme": "success"}], | ||
| ) | ||
| scale = next( | ||
| rule | ||
| for _, rule in _cf_rules(load_workbook(io.BytesIO(styled)).active) | ||
| if rule.type == "colorScale" | ||
| ) | ||
| assert scale.colorScale.cfvo[0].type == "min" | ||
| assert scale.colorScale.cfvo[1].type == "max" | ||
|
|
||
|
|
||
| def test_polish_explore_xlsx_table_cf_on_xlsxwriter_workbook() -> None: | ||
| """Explore Table download writes xlsxwriter; polish must still see data rows.""" | ||
| df = pd.DataFrame({"sales": [10, 50, 5], "region": ["west", "east", "north"]}) | ||
| polished = polish_explore_xlsx( | ||
| df_to_excel(df, index=False), | ||
| df, | ||
| { | ||
| "viz_type": "table", | ||
| "conditionalFormatting": [ | ||
| { | ||
| "column": "sales", | ||
| "operator": ">", | ||
| "targetValue": 8, | ||
| "colorScheme": "#FF0000", | ||
| } | ||
| ], | ||
| "show_cell_bars": False, | ||
| "column_config": { | ||
| "sales": { | ||
| "d3NumberFormat": ",.1f", | ||
| "horizontalAlign": "right", | ||
| } | ||
| }, | ||
| }, | ||
| ) | ||
| sheet = load_workbook(io.BytesIO(polished)).active | ||
| assert sheet.max_row == 4 | ||
| assert sheet["A2"].value == 10 | ||
| assert sheet["A3"].value == 50 | ||
| rules = _cf_rules(sheet) | ||
| assert len(rules) == 1 | ||
| assert rules[0][1].type == "cellIs" | ||
| assert rules[0][1].operator == "greaterThan" | ||
| assert "A2" in rules[0][0] | ||
| assert sheet["A2"].number_format == "#,##0.0" | ||
| assert sheet["A2"].alignment.horizontal == "right" | ||
| assert sheet["B2"].alignment.horizontal is None | ||
|
|
||
|
|
||
| def test_polish_explore_xlsx_default_cell_bars_on_xlsxwriter() -> None: | ||
| df = pd.DataFrame({"sales": [10, 20], "region": ["west", "east"]}) | ||
| polished = polish_explore_xlsx( | ||
| df_to_excel(df, index=False), | ||
| df, | ||
| { | ||
| "viz_type": "table", | ||
| "show_cell_bars": True, | ||
| "conditional_formatting": [], | ||
| }, | ||
| ) | ||
| sheet = load_workbook(io.BytesIO(polished)).active | ||
| data_bars = [rng for rng, rule in _cf_rules(sheet) if rule.type == "dataBar"] | ||
| assert data_bars == ["A2:A3"] | ||
|
|
||
|
|
||
| def test_polish_explore_xlsx_skips_unrelated_viz() -> None: |
There was a problem hiding this comment.
Ten new test functions (lines 209, 214, 222, 244, 265, 288, 310, 330, 383, 399) have no docstring, while sibling tests in this same file document intent (e.g. test_value_matches_rule_comparators, test_polish_explore_xlsx_table_cf_on_xlsxwriter_workbook). BITO rule 12148 requires docstrings on all new test functions. Add one-line docstrings stating scenario and expected outcome. (BITO.md)
Code Review Run #214642
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
|
…ting-export' into feature/excel-conditional-formatting-export
github-actions
Bot
added
the
requires:rebase
… export Honor datasource number/currency formats when Table column_config is empty, and omit ColorScale cfvo values unless the bound type is numeric.
github-actions
Bot
removed
the
requires:rebase
TypedDict from datasource.data is not dict[str, Any]; Mapping unblocks mypy on pre-commit.
…/excel-conditional-formatting-export Keep Mapping types for ExplorableData so mypy still accepts datasource.data on the Excel XLSX path.
Use the default export and children render function instead of a named AutoSizer/renderProp, which crashed Explore and dashboard chart lists. Skip the purge-audit coordination seed when the sentinel already exists so db upgrade can run.
The default/children import crashed Explore on react-virtualized-auto-sizer v2. Also drop the unrelated purge_audit migration guard from this branch.
SUMMARY
Table chart XLSX download did not include Explore adaptive formatting. The export writes the workbook in
QueryContextProcessor.get_databefore client post-processing, soconditional_formattingnever reached the file.For Table charts, the export now writes native Excel conditional formatting: comparator rules as CellIs (fill or font), operator
Noneas a color scale, range operators as formulas, and cell bar as Excel data bars. If there are no custom rules andshow_cell_barsis on, numeric columns get data bars like the default Table gradient. Sheet bounds are refreshed so rules cover all data rows, not only the header.BEFORE/AFTER SCREENSHOTS
Before. Explore Table shows cell bars / color formatters; the XLSX has values only.
After. Matching columns in the XLSX have Excel data bars or CellIs/color-scale rules. Screenshots:
screenshots/after/.TESTING INSTRUCTIONS
Manual:
show_cell_barson, no custom formatters. Download XLSX: numeric columns have data bars.sales > 10with background color. Download: Excel CellIs greater-than onsales.region) do not get data bars fromshow_cell_bars.ADDITIONAL INFORMATION