P1: formatting sandbox — full CellFormat, layout tools, style recipes, NL coverage - #44
Conversation
Reviewer's GuideThis PR completes the formatting sandbox by extending format_cells, adding undo-aware layout tools, sharing NL phrase parsing across client and server paths, and introducing bounded style recipes whose preview and execution consume the same resolved plan. Sequence diagram for shared natural-language formatting actionssequenceDiagram
actor User
participant Parser as ClientParser
participant Templates as ActTemplates
participant Phrases as SharedPhraseTable
participant Preview as PreviewBuilder
participant Executor as ToolExecutor
participant Store as SpreadsheetStore
User->>Parser: parseMessage(request)
Parser->>Phrases: parseLayoutPhrase(request)
Parser->>Phrases: parseStyleRecipePhrase(request)
Parser-->>User: tool action
User->>Templates: resolveActTemplates(request)
Templates->>Phrases: parseLayoutPhrase(request)
Templates->>Phrases: parseStyleRecipePhrase(request)
Templates-->>User: Apply action
User->>Preview: buildActionPreview(style_recipe)
Preview->>Executor: buildRecipePlan(recipe)
Executor-->>User: preview changes
User->>Executor: executeTool(action)
Executor->>Store: setColumnWidth / setRowHeight / autoFitRows
Executor->>Store: apply recipe plan
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 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 reviewed your changes and they look great!
Sourcery assessment
Needs a human reviewer. If the table detection or total-row logic is wrong, applying the recipe can write a Total row and SUM formulas into the workbook, while styling, filters, and layout changes also persist after the code is reverted. These effects are bounded and can be undone or manually recomputed and removed, but reverting the PR alone will not repair workbooks already changed.
Review follow-up:
|
| writes | formula | reported total | |
|---|---|---|---|
| 1st run | A5 B5 C5 |
=SUM(B2:B4) |
4 |
| 2nd run | A6 B6 C6 |
=SUM(B2:B5) |
8 |
The second run's range spans the first run's total, and it compounded on every re-apply. The preview rendered a plausible-looking Total row each time, so nothing looked wrong — the number was just quietly inflated.
The recipe now recognises its own output (the Total label and at least one SUM formula on the row) and treats that row as the target, excluding it from the data, so re-running refreshes it in place. Requiring the formula as well as the label is load-bearing: a data row that merely reads "Total" is left alone and the totals row is appended below it. I got that wrong on the first attempt and a test caught it.
Detection is confined to styleRecipes.ts — generateTableTotals's only caller — so formatAsTable.ts is untouched and the helper remains a pure function of the range it receives.
On the revert-doesn't-repair-workbooks point: worth noting these writes are already undoable in-app. handleStyleRecipe calls ctx.pushHistory(...) before mutating (formatOps.ts:37), and style_recipe is in the highImpactTools set in chatSlice.ts, so it goes through the Apply/Reject preview path like the other bulk mutations. A user who applies a bad recipe has a single undo. The residual risk is narrow: a recipe applied via a saved workbook and then re-opened in a session where undo no longer spans it.
Also checked and not affected: header and table_polish are format-only and re-applying overwrites rather than accumulates, so they're naturally idempotent.
Verification
| Gate | Result |
|---|---|
npm run lint:ci |
pass |
npm run typecheck |
pass |
npm run test |
1825 / 1825 (+3 regression) |
npm run test --prefix server |
344 / 344 |
npm run test:realengine |
12 / 12 |
🤖 Generated with Claude Code
…, NL coverage The formatting half of the "useful first, unique second" strategy, stacked on p0-usefulness-first. Same executor spine as the agent tools — no new runtime. P1.1 — format_cells reaches the full CellFormat surface Underline, strikethrough, fontFamily, textAlign, verticalAlign, textWrap and borders, wired through buildFormatPatch, FormatCellsParams and the tool registry schema. src/lib/formatCellsTool.ts owns the patch construction. P1.2 — layout tools set_column_width, set_row_height and auto_fit as agent tools, with ExecutionContext hooks delegating to the existing undoable store methods (src/agent/toolHandlers/layoutOps.ts, src/store/aiExecution.ts). P1.3 — NL coverage parseLayoutPhrase routes column-width / row-height / auto-fit requests through both the client parser and the server actTemplates, sharing one phrase table (shared/spreadsheetPhrases.ts) so the two can't drift. P1.4 — style recipes Bounded one-shot macros — header, total_row, table_polish — resolved to a plan that drives both the Apply/Reject preview and execution, so what the user reviews is what runs. style_recipe joins the question-veto set, so "Should I add a total row?" no longer fires a bulk restyle. Verified: lint:ci, typecheck, 1822 unit tests, 344 server tests, 12 realengine golden-set tests.
Applying the total_row recipe twice silently doubled the reported total. detectTableRange derives endRow from every non-empty cell, so the Total row written by the first run became part of the "data". The second run then appended a *new* Total row one row lower and widened the SUM range to span the first one: 1st run A5="Total" B5==SUM(B2:B4) -> total 4 2nd run A6="Total" B6==SUM(B2:B5) -> total 8, and B5 is now double-counted It compounded on every re-apply, and the preview showed the user a plausible looking "Total" row each time, so nothing looked wrong. Now the recipe recognises its own output — the Total label *and* at least one SUM formula on the row — and treats that row as the target, excluding it from the data. Re-running refreshes the same row in place. Requiring the formula as well as the label matters: a data row that merely reads "Total" is left alone and the totals row is appended below it instead. Detection is confined to styleRecipes.ts, which is generateTableTotals' only caller, so formatAsTable.ts is untouched and the helper stays a pure function of the range it is handed. Verified: lint:ci, typecheck, 1825 unit tests, 344 server tests, 12 realengine golden-set tests. Adds 3 regression tests covering repeat application, the "Total" data row, and refresh after a value change.
7dd3a65 to
4e38414
Compare
The formatting half of the "useful first, unique second" strategy, stacked on #43. Same executor spine as the existing agent tools — no new agent runtime.
19 files, +983 / −4.
P1.1 —
format_cellsreaches the fullCellFormatsurfaceUnderline, strikethrough,
fontFamily,textAlign,verticalAlign,textWrapand borders, wired throughbuildFormatPatch,FormatCellsParamsand the tool registry schema.src/lib/formatCellsTool.tsowns patch construction.P1.2 — layout tools
set_column_width,set_row_heightandauto_fitas agent tools, withExecutionContexthooks delegating to the existing undoable store methods (src/agent/toolHandlers/layoutOps.ts,src/store/aiExecution.ts).P1.3 — NL coverage
parseLayoutPhraseroutes column-width / row-height / auto-fit requests through both the client parser and the serveractTemplates, sharing one phrase table (shared/spreadsheetPhrases.ts) so the two can't drift. Includes a false-positive corpus.P1.4 — style recipes
Bounded one-shot macros —
header,total_row,table_polish— resolved to a plan that drives both the Apply/Reject preview and execution, so what the user reviews is exactly what runs. Reuses the existingformatAsTable/generateTableTotalsbuilding blocks rather than re-implementing table styling.style_recipejoinsDESTRUCTIVE_TOOLS, so "Should I add a total row?" no longer fires a bulk restyle.Fixes included
Two pre-existing breakages that were masked by an unresolved merge on the original branch:
styleRecipes.test.ts— usedrefToCell's inverse wrongly (cellToReftakes a cell id, not row/col)styleRecipes.ts— deadtotalsRowbinding, which failedlint:ci(--max-warnings=0)Verification
npm run lint:cinpm run typechecknpm run testnpm run test --prefix servernpm run test:realengineNotes for the reviewer
parser.tsandpreviewBuilders.tsalso carry the P0: share gap-risk detection with the preview path; user-entered AI feedback #43 changes, since the P1 branches are appended to the same functions. Review the P1-only hunks (theparseLayoutPhrase/parseStyleRecipePhraseblocks and thestyle_recipepreview branch).docs/verification-backlog.mditems 1–3.🤖 Generated with Claude Code
Summary by Sourcery
Extend the formatting sandbox with comprehensive cell formatting, undoable layout controls, natural-language coverage, and previewable style recipes.
New Features:
Bug Fixes:
Enhancements:
Tests: