Skip to content

jsonforms-renderers(spreadsheet)!: require id on formula config/result entries, persist derivations as an id-keyed map - #32

Open
sthanikan2000 wants to merge 2 commits into
feat/spreadsheet-parse-process-splitfrom
feat/derivation-id-required
Open

jsonforms-renderers(spreadsheet)!: require id on formula config/result entries, persist derivations as an id-keyed map#32
sthanikan2000 wants to merge 2 commits into
feat/spreadsheet-parse-process-splitfrom
feat/derivation-id-required

Conversation

@sthanikan2000

Copy link
Copy Markdown
Contributor

Summary

Breaking change to x-evaluate/derivation results, done as one cohesive change now that the parse/process split (#31) has landed underneath it:

  1. id is now a required string on both FormulaConfigEntry (the x-evaluate config) and FormulaResult (what evaluateExpressions returns per entry) — previously entries were matched only by array index/label.
    • evaluateExpressions itself (in expression.ts) is untouched in behavior — it still returns an array, in authored order; only its per-entry shape now guarantees an id.
    • All 19 x-evaluate entries in the spreadsheet dev fixture now carry an id.
    • Extracted describeFormulaError as a reusable export (pure refactor, no behavior change) — used by later PRs in this stack.
  2. SpreadsheetControl's persisted derivations changes from FormulaResult[] to Record<id, DerivationResult>, written directly into processMatrix (the seam jsonforms-renderers(spreadsheet): separate spreadsheet matrix processing from parsing #31 introduced). This is the actual purpose id-required serves: a sibling field can address one specific derivation's value directly via a plain data-tree path (e.g. sales.derivations.total_sales.value), with no "find by id in an array" step needed anywhere.
    • The "Computed values" panel now iterates Object.entries(derivations) instead of the array, keyed by id.

These two changes are combined into a single PR (rather than split, as in an earlier iteration of this stack) because requiring id has no purpose in this codebase other than serving as this map's key — splitting them meant a followup PR would delete/replace code the first PR had just introduced. Combining them, on top of the already-extracted processMatrix, means the map-building logic is written exactly once, directly in its final home.

Test plan

  • pnpm type-check and pnpm test pass
  • pnpm run format:check passes
  • Manually verified in the dev playground: re-uploaded the sample spreadsheet, confirmed "Computed values" still lists every entry correctly

🤖 Generated with Claude Code

sthanikan2000 and others added 2 commits August 31, 2026 12:27
Every x-evaluate FormulaConfigEntry and the FormulaResult it produces
must now carry an id, so consumers can key off a stable identifier
instead of array index. This is a breaking change: entries without an
id are rejected the same way a missing label/expression is today.

evaluateExpressions now threads id through on both the success and
error/catch paths, and the catch-block error-code lookup is extracted
into an exported describeFormulaError() helper. Updates the dev
fixture, docs example, and SpreadsheetControl's list key accordingly.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Derivations were an array (FormulaResult[]); now they're persisted as
Record<id, {label, value, error?}>, keyed by each x-evaluate entry's
mandatory id. This is the actual purpose id-required serves: a sibling
field can now address one specific derivation's value directly via a
plain data-tree path (e.g. sales.derivations.total_sales.value), with
no "find by id in an array" step needed anywhere.

Written once, directly in processMatrix (introduced by the previous
PR) — SpreadsheetControl.tsx's processFile calls processMatrix
opaquely and needs no changes; only its "Computed values" panel
rendering (Object.entries/Object.keys instead of array methods) is
touched.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Aug 31, 2026

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 59 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Team

Run ID: 97f88684-2fea-4c2f-8926-37c8fa6ed30e

📥 Commits

Reviewing files that changed from the base of the PR and between 9853559 and bc05d26.

📒 Files selected for processing (9)
  • packages/jsonforms-renderers/dev/fixtures.ts
  • packages/jsonforms-renderers/docs/spreadsheet-formulas.md
  • packages/jsonforms-renderers/src/renderers/SpreadsheetControl.tsx
  • packages/jsonforms-renderers/src/utils/spreadsheet/expression.test.ts
  • packages/jsonforms-renderers/src/utils/spreadsheet/expression.ts
  • packages/jsonforms-renderers/src/utils/spreadsheet/index.ts
  • packages/jsonforms-renderers/src/utils/spreadsheet/process.test.ts
  • packages/jsonforms-renderers/src/utils/spreadsheet/process.ts
  • packages/jsonforms-renderers/src/utils/spreadsheet/types.ts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@sthanikan2000
sthanikan2000 force-pushed the feat/derivation-id-required branch from bc05d26 to 38eda65 Compare August 31, 2026 07:06
@sthanikan2000
sthanikan2000 force-pushed the feat/derivation-id-required branch from 38eda65 to bc05d26 Compare August 31, 2026 07:23
@sthanikan2000 sthanikan2000 self-assigned this Aug 31, 2026
@sthanikan2000 sthanikan2000 changed the title jsonforms-renderers: require id on formula config/result entries, persist derivations as an id-keyed map jsonforms-renderers(spreadsheet)!: require id on formula config/result entries, persist derivations as an id-keyed map Sep 2, 2026
@sthanikan2000
sthanikan2000 marked this pull request as ready for review September 2, 2026 01:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant