Repository navigation
docs: spreadsheet styling standard (homemade -> pro spec) - #59
Merged
Merged
Conversation
House styling standard the AI follows for style/cleanup/professionalize requests. Written in our own words (no third-party text/code copied); the source example material stays gitignored under temp/. Defines the spec for two deferred features: the one-shot professionalize-my-sheet makeover (an execute_macro chaining existing style_recipe/format_cells/auto_fit tools) and the Q4 styling-skills catalog (kind:recipe capabilities). Every rule maps to a tool that already exists. Records that column-role inference already exists (sheetProfile.inferRole) so no new inference helper is needed; includes the role->numberFormat mapping and the amount-vs-currency gap resolved via detectedPurpose.
Reviewer's GuideAdds a design-only spreadsheet styling standard that turns “make it look professional” into an implementable, existing-tool-based specification. It defines the visual rules, a preview-first value-safe makeover plan, preservation of existing styles, future recipe intents, and the build order for the deferred makeover and styling catalog features. Sequence diagram for the planned professionalize-sheet makeoversequenceDiagram
actor User
participant Router
participant execute_macro
participant style_recipe
participant format_cells
participant auto_fit
User->>Router: Request professionalize sheet
Router->>execute_macro: Build preview-first makeover plan
execute_macro->>style_recipe: Apply header
execute_macro->>format_cells: Apply role-based numberFormat
execute_macro->>style_recipe: Apply total_row when applicable
execute_macro->>auto_fit: Fit column widths
execute_macro->>format_cells: Apply one conditional emphasis
execute_macro-->>User: Show full diff for confirmation
User->>execute_macro: Confirm
execute_macro-->>User: Apply as one undo entry
Flow diagram for role-based spreadsheet number formattingflowchart LR
Profile["buildSheetProfile"] --> Role["ColumnProfile.role"]
Role --> Purpose["detectedPurpose"]
Purpose --> Mapping["Role-to-numberFormat mapping"]
Mapping --> Format["format_cells"]
Format --> Values["Values unchanged"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configuration
Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 9 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="docs/spreadsheet-styling-standard.md" line_range="120" />
<code_context>
+and the user hasn't overridden these:
+
+- Zeros shown as `-` (dash), not `0`.
+- Negatives red and in parentheses: `(500)`, not `-500` (`accounting-neg`).
+- Units in the header: "Revenue ($mm)", not a bare "Revenue".
+- Totals sum the contiguous range directly above, with a border above the total.
</code_context>
<issue_to_address>
**Styling adds workbook data**
When the makeover applies or refreshes a totals row, `style_recipe: total_row` writes a Total label and SUM formulas or values, so the totals step changes cell contents despite the makeover’s value-safe boundary; users see new or changed workbook data after a styling request.
Keep the data-writing totals recipe out of the value-safe makeover, or change it to format existing totals without writing cell values.
Also at `docs/spreadsheet-styling-standard.md:149-150`, `docs/spreadsheet-styling-standard.md:161-163`.
</issue_to_address>
### Comment 2
<location path="docs/spreadsheet-styling-standard.md" line_range="164" />
<code_context>
+- Every step is a format/layout op — no data mutation, no row deletion. The
+ makeover must never change a cell VALUE (that keeps it safely reversible and
+ honest).
+- One `execute_macro` → one undo entry.
+
+---
</code_context>
<issue_to_address>
**Concurrent edits disappear on rollback**
When a user edits the workbook while a multi-step makeover is running and a later step fails or is cancelled, `executeMacro` can await between makeover steps, and a later failure or cancellation calls `rollbackGroup`; the shared-snapshot undo manager restores the entire workbook snapshot, erasing any user edits made after the makeover began.
Prevent concurrent workbook edits for the duration of the macro, or make rollback restore only the macro's own changes.
</issue_to_address>
### Comment 3
<location path="docs/spreadsheet-styling-standard.md" line_range="159" />
<code_context>
+7. **(Finance sheets only)** apply §1.7 extras.
+
+Boundaries:
+- Reuse `table_polish` where it already does banded-table styling rather than
+ hand-rolling bands.
+- Every step is a format/layout op — no data mutation, no row deletion. The
</code_context>
<issue_to_address>
**Makeover loses existing filters**
When the makeover uses `table_polish` on a sheet with existing filter conditions, `table_polish` replaces the active filter configuration with unconditioned filters for each table column. The macro undo snapshot covers only the workbook, so the user's existing filter conditions disappear and cannot be restored by the makeover's single undo entry.
Preserve existing filter conditions when applying table styling and include filter state in the macro's rollback and undo history.
</issue_to_address>
### Comment 4
<location path="docs/spreadsheet-styling-standard.md" line_range="150" />
<code_context>
+ date / count) from its values and apply the matching `numberFormat` via
+ `format_cells` on the column range. This is the highest-impact single step.
+4. **Totals** — `style_recipe: total_row` where a totals row exists or is wanted
+ (idempotent — must not stack a second total row).
+5. **Column widths** — `auto_fit` (or `set_column_width` with label-wider-than-
+ figure heuristics) so nothing is clipped or swimming in space.
</code_context>
<issue_to_address>
**Existing totals get counted twice**
When the sheet already has a hardcoded totals row, the existing recipe recognizes only a labeled totals row containing a formula; a hardcoded totals row is treated as data. `generateTableTotals` then appends another row whose SUM includes that existing total, so the displayed result double-counts the values.
Detect and exclude existing totals rows whether their totals are formulas or hardcoded values before generating another row.
</issue_to_address>
### Comment 5
<location path="docs/spreadsheet-styling-standard.md" line_range="58" />
<code_context>
+- Plain counts → `number-int`; measured quantities → `number` with a sensible
+ decimal count.
+- Infer the format from the column's role and content, not from the header word
+ alone (a column of `0.08`, `0.2` is almost certainly percent even if unlabeled).
+
+### 1.2 Hierarchy: headers, data, and totals must look different
</code_context>
<issue_to_address>
**Unlabeled ratios stay decimals**
When a generic-purpose sheet has a numeric ratio column without a percentage-related header, `inferRole` classifies dense numeric columns without percentage-like headers as `amount`, and the generic-purpose mapping formats them as `number`; ratios such as 0.08 therefore display as decimals instead of percentages.
Use the column values as well as header keywords to identify ratio columns as percentages before applying the role-to-format mapping.
Also at `docs/spreadsheet-styling-standard.md:59`, `docs/spreadsheet-styling-standard.md:230`, `docs/spreadsheet-styling-standard.md:237-240`, `docs/spreadsheet-styling-standard.md:242-243`.
</issue_to_address>
### Comment 6
<location path="docs/spreadsheet-styling-standard.md" line_range="234" />
<code_context>
+ column (`ColumnProfile.role`) alongside `dtype` and `detectedPurpose`
+ (`budget | invoice | inventory | sales | generic`). The makeover's step 3 maps
+ roles → formats:
+ - `date` → a date format
+ - `percentage` → `percent`
+ - `quantity` → `number-int`
</code_context>
<issue_to_address>
**Years display as dates**
When a column named `Year` contains numeric calendar years, `inferRole` classifies the column as `date`, and the role mapping applies a date format; values such as 2024 then display as Excel-serial dates instead of years.
Distinguish calendar-year columns from date columns in `inferRole` or the role-to-format mapping.
</issue_to_address>
### Comment 7
<location path="docs/spreadsheet-styling-standard.md" line_range="97" />
<code_context>
+
+When highlighting by rule (negatives red, numeric cells, values over a
+threshold), prefer a live `condition` on `format_cells` (`negative`, `is_number`,
+`gt`, etc.) so it keeps holding as the data changes — not a one-time fill painted
+onto whatever cells happen to match today.
+
</code_context>
<issue_to_address>
**Highlights become stale**
When cell values change after a condition-based format is applied, `format_cells` evaluates the condition once and writes ordinary cell formatting; it does not retain a live rule. Old matches stay highlighted and new matches are not highlighted, so users see stale conditional formatting.
Use a persistent conditional-formatting rule that reevaluates the condition when cell values change.
Also at `docs/spreadsheet-styling-standard.md:200-201`.
</issue_to_address>
### Comment 8
<location path="docs/spreadsheet-styling-standard.md" line_range="119" />
<code_context>
+When the sheet is clearly a financial model (P&L, budget, valuation, 3-statement)
+and the user hasn't overridden these:
+
+- Zeros shown as `-` (dash), not `0`.
+- Negatives red and in parentheses: `(500)`, not `-500` (`accounting-neg`).
+- Units in the header: "Revenue ($mm)", not a bare "Revenue".
</code_context>
<issue_to_address>
**Zero-as-dash has no format**
When the finance-style makeover applies the zero-display rule, the finance-only rule asks for zeros to display as a dash, but the documented number-format vocabulary has no zero-as-dash format and `format_cells` cannot define a custom one. Applying the listed formats leaves zeros displayed as zero, so the finance makeover cannot satisfy this rule using the claimed existing tools.
Add a zero-as-dash number format or mark this rule as requiring new formatting support.
</issue_to_address>
### Comment 9
<location path="docs/spreadsheet-styling-standard.md" line_range="162" />
<code_context>
+- Reuse `table_polish` where it already does banded-table styling rather than
+ hand-rolling bands.
+- Every step is a format/layout op — no data mutation, no row deletion. The
+ makeover must never change a cell VALUE (that keeps it safely reversible and
+ honest).
+- One `execute_macro` → one undo entry.
</code_context>
<issue_to_address>
**Makeover changes cell values**
When the makeover applies `total_row` because a totals row is missing or wanted, `style_recipe: total_row` writes a “Total” label and SUM formulas, so a makeover that includes this step changes workbook data despite the value-safe promise; users see new values and formulas.
Exclude `total_row` from the value-safe makeover or explicitly make its data writes a separately previewed and confirmed exception.
</issue_to_address>Sourcery assessment
Approval pending. 9 findings to address first.
Blocking findings: docs/spreadsheet-styling-standard.md:120, docs/spreadsheet-styling-standard.md:164, docs/spreadsheet-styling-standard.md:159, docs/spreadsheet-styling-standard.md:150, docs/spreadsheet-styling-standard.md:58, and 4 more
| - Every step is a format/layout op — no data mutation, no row deletion. The | ||
| makeover must never change a cell VALUE (that keeps it safely reversible and | ||
| honest). | ||
| - One `execute_macro` → one undo entry. |
There was a problem hiding this comment.
🟡 Medium · Concurrent edits disappear on rollback
When a user edits the workbook while a multi-step makeover is running and a later step fails or is cancelled, executeMacro can await between makeover steps, and a later failure or cancellation calls rollbackGroup; the shared-snapshot undo manager restores the entire workbook snapshot, erasing any user edits made after the makeover began.
Prevent concurrent workbook edits for the duration of the macro, or make rollback restore only the macro's own changes.
Prompt for AI agents
In `docs/spreadsheet-styling-standard.md` at line 164:
**Concurrent edits disappear on rollback**
When a user edits the workbook while a multi-step makeover is running and a later step fails or is cancelled, `executeMacro` can await between makeover steps, and a later failure or cancellation calls `rollbackGroup`; the shared-snapshot undo manager restores the entire workbook snapshot, erasing any user edits made after the makeover began.
Prevent concurrent workbook edits for the duration of the macro, or make rollback restore only the macro's own changes.| 7. **(Finance sheets only)** apply §1.7 extras. | ||
|
|
||
| Boundaries: | ||
| - Reuse `table_polish` where it already does banded-table styling rather than |
There was a problem hiding this comment.
🟡 Medium · Makeover loses existing filters
When the makeover uses table_polish on a sheet with existing filter conditions, table_polish replaces the active filter configuration with unconditioned filters for each table column. The macro undo snapshot covers only the workbook, so the user's existing filter conditions disappear and cannot be restored by the makeover's single undo entry.
Preserve existing filter conditions when applying table styling and include filter state in the macro's rollback and undo history.
Prompt for AI agents
In `docs/spreadsheet-styling-standard.md` at line 159:
**Makeover loses existing filters**
When the makeover uses `table_polish` on a sheet with existing filter conditions, `table_polish` replaces the active filter configuration with unconditioned filters for each table column. The macro undo snapshot covers only the workbook, so the user's existing filter conditions disappear and cannot be restored by the makeover's single undo entry.
Preserve existing filter conditions when applying table styling and include filter state in the macro's rollback and undo history.| date / count) from its values and apply the matching `numberFormat` via | ||
| `format_cells` on the column range. This is the highest-impact single step. | ||
| 4. **Totals** — `style_recipe: total_row` where a totals row exists or is wanted | ||
| (idempotent — must not stack a second total row). |
There was a problem hiding this comment.
🟠 High · Existing totals get counted twice
When the sheet already has a hardcoded totals row, the existing recipe recognizes only a labeled totals row containing a formula; a hardcoded totals row is treated as data. generateTableTotals then appends another row whose SUM includes that existing total, so the displayed result double-counts the values.
Detect and exclude existing totals rows whether their totals are formulas or hardcoded values before generating another row.
Prompt for AI agents
In `docs/spreadsheet-styling-standard.md` at line 150:
**Existing totals get counted twice**
When the sheet already has a hardcoded totals row, the existing recipe recognizes only a labeled totals row containing a formula; a hardcoded totals row is treated as data. `generateTableTotals` then appends another row whose SUM includes that existing total, so the displayed result double-counts the values.
Detect and exclude existing totals rows whether their totals are formulas or hardcoded values before generating another row.| - Plain counts → `number-int`; measured quantities → `number` with a sensible | ||
| decimal count. | ||
| - Infer the format from the column's role and content, not from the header word | ||
| alone (a column of `0.08`, `0.2` is almost certainly percent even if unlabeled). |
There was a problem hiding this comment.
🟡 Medium · Unlabeled ratios stay decimals
When a generic-purpose sheet has a numeric ratio column without a percentage-related header, inferRole classifies dense numeric columns without percentage-like headers as amount, and the generic-purpose mapping formats them as number; ratios such as 0.08 therefore display as decimals instead of percentages.
Use the column values as well as header keywords to identify ratio columns as percentages before applying the role-to-format mapping.
Also at docs/spreadsheet-styling-standard.md:59, docs/spreadsheet-styling-standard.md:230, docs/spreadsheet-styling-standard.md:237-240, docs/spreadsheet-styling-standard.md:242-243.
Prompt for AI agents
In `docs/spreadsheet-styling-standard.md` at line 58:
**Unlabeled ratios stay decimals**
When a generic-purpose sheet has a numeric ratio column without a percentage-related header, `inferRole` classifies dense numeric columns without percentage-like headers as `amount`, and the generic-purpose mapping formats them as `number`; ratios such as 0.08 therefore display as decimals instead of percentages.
Use the column values as well as header keywords to identify ratio columns as percentages before applying the role-to-format mapping.
Also at `docs/spreadsheet-styling-standard.md:59`, `docs/spreadsheet-styling-standard.md:230`, `docs/spreadsheet-styling-standard.md:237-240`, `docs/spreadsheet-styling-standard.md:242-243`.| column (`ColumnProfile.role`) alongside `dtype` and `detectedPurpose` | ||
| (`budget | invoice | inventory | sales | generic`). The makeover's step 3 maps | ||
| roles → formats: | ||
| - `date` → a date format |
There was a problem hiding this comment.
🟡 Medium · Years display as dates
When a column named Year contains numeric calendar years, inferRole classifies the column as date, and the role mapping applies a date format; values such as 2024 then display as Excel-serial dates instead of years.
Distinguish calendar-year columns from date columns in inferRole or the role-to-format mapping.
Prompt for AI agents
In `docs/spreadsheet-styling-standard.md` at line 234:
**Years display as dates**
When a column named `Year` contains numeric calendar years, `inferRole` classifies the column as `date`, and the role mapping applies a date format; values such as 2024 then display as Excel-serial dates instead of years.
Distinguish calendar-year columns from date columns in `inferRole` or the role-to-format mapping.|
|
||
| When highlighting by rule (negatives red, numeric cells, values over a | ||
| threshold), prefer a live `condition` on `format_cells` (`negative`, `is_number`, | ||
| `gt`, etc.) so it keeps holding as the data changes — not a one-time fill painted |
There was a problem hiding this comment.
🟡 Medium · Highlights become stale
When cell values change after a condition-based format is applied, format_cells evaluates the condition once and writes ordinary cell formatting; it does not retain a live rule. Old matches stay highlighted and new matches are not highlighted, so users see stale conditional formatting.
Use a persistent conditional-formatting rule that reevaluates the condition when cell values change.
Also at docs/spreadsheet-styling-standard.md:200-201.
Prompt for AI agents
In `docs/spreadsheet-styling-standard.md` at line 97:
**Highlights become stale**
When cell values change after a condition-based format is applied, `format_cells` evaluates the condition once and writes ordinary cell formatting; it does not retain a live rule. Old matches stay highlighted and new matches are not highlighted, so users see stale conditional formatting.
Use a persistent conditional-formatting rule that reevaluates the condition when cell values change.
Also at `docs/spreadsheet-styling-standard.md:200-201`.| When the sheet is clearly a financial model (P&L, budget, valuation, 3-statement) | ||
| and the user hasn't overridden these: | ||
|
|
||
| - Zeros shown as `-` (dash), not `0`. |
There was a problem hiding this comment.
🟡 Medium · Zero-as-dash has no format
When the finance-style makeover applies the zero-display rule, the finance-only rule asks for zeros to display as a dash, but the documented number-format vocabulary has no zero-as-dash format and format_cells cannot define a custom one. Applying the listed formats leaves zeros displayed as zero, so the finance makeover cannot satisfy this rule using the claimed existing tools.
Add a zero-as-dash number format or mark this rule as requiring new formatting support.
Prompt for AI agents
In `docs/spreadsheet-styling-standard.md` at line 119:
**Zero-as-dash has no format**
When the finance-style makeover applies the zero-display rule, the finance-only rule asks for zeros to display as a dash, but the documented number-format vocabulary has no zero-as-dash format and `format_cells` cannot define a custom one. Applying the listed formats leaves zeros displayed as zero, so the finance makeover cannot satisfy this rule using the claimed existing tools.
Add a zero-as-dash number format or mark this rule as requiring new formatting support.The makeover was described as value-safe (never changes a cell value) while including style_recipe: total_row and table_polish, both of which WRITE cell contents (Total label + SUM formulas; table_polish calls generateTableTotals) and carry persistenceCaveat. Split the makeover into a value-safe formatting pass (header, number formats, widths, conditional emphasis) and a separate opt-in totals step that writes data, is preview-only, caveat-carrying, and never auto-applied. Banded-table look prefers format-only banding over table_polish unless the user wants its totals. Clarified the 1.7 totals line and added a Writes-data column to the Q4 catalog. Addresses a CodeRabbit finding.
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.
Adds
docs/spreadsheet-styling-standard.md— the house standard the chat AI follows when a user asks to style, clean up, or "make this look professional." This is the design spec that unblocks two deferred features:execute_macrochaining existingstyle_recipe/format_cells/auto_fittools).kind: 'recipe'capabilities whose trigger phrases map to these outcomes).What's in it
kind: 'recipe'capability mechanism.Verified during authoring
Column-role inference already exists (
sheetProfile.inferRole→ColumnRole), so no new inference helper is needed for the makeover — only a role→numberFormat mapping (included), with theamount-vs-currency ambiguity resolved viadetectedPurpose.Provenance / licensing
Written entirely in our own words — distilled industry spreadsheet-design conventions, no third-party text or code copied. The example material that informed it stays gitignored under
temp/and is not part of the repo, so nothing third-party is shared or committed.Notes
Doc-only change — no code, no deploy impact. It's the spec for later work, not an implementation.
Summary by Sourcery
Establish the design specification for professional spreadsheet styling and the deferred makeover and recipe-catalog features.
Enhancements:
Documentation: