Skip to content

fix: persist resolved default k/q/i-grid on unedited execution units (SOF-7990) - #161

Closed
VsevolodX wants to merge 3 commits into
mainfrom
feature/SOF-7990
Closed

fix: persist resolved default k/q/i-grid on unedited execution units (SOF-7990)#161
VsevolodX wants to merge 3 commits into
mainfrom
feature/SOF-7990

Conversation

@VsevolodX

Copy link
Copy Markdown
Member

Summary

  • ExecutionUnit.savePersistentContext only kept explicitly edited context items, so the default (unedited) path never wrote a kgrid/qgrid/igrid entry into an execution unit's persisted context. assign-precision-for-material and similar consumers reading context["kgrid"] unconditionally hit a KeyError on any workflow that never had its k-grid touched by the user.
  • PointsGridFormDataProvider (kgrid/qgrid/igrid) now opts into persisting its resolved default via a new isPersistedWhenNotEdited flag, checked alongside isEdited in savePersistentContext's filter.
  • Found in review: a persisted-but-unedited item is read back into the provider on the next construction (getContextProvidersInstances seeds providers from unit.context regardless of isEdited), so without a guard the resolved default would pin to whichever material was rendered first instead of recomputing per material. PointsGridFormDataProvider's constructor now discards inherited data when the incoming item isn't edited, so every render recomputes fresh while still persisting the result.
  • Also fixes convergence_mixin.py's bare get_context_item("kgrid").get(...), which raised AttributeError instead of the intended ValueError when no kgrid context item exists yet.

Test plan

  • npm test — 49 passing (baseline 47), no failing line
  • python -m pytest — 84 passed (baseline 81)
  • npx tsc --noEmit — clean
  • npx eslint src/js tests/js — 0 errors, 5 pre-existing warnings
  • Negative-controlled: the material-pinning regression test fails when the fix's guard is reverted, and the Workflow.test.ts invariant fails when savePersistentContext alone is reverted (flag left true) — both confirmed by deliberately reverting each fix in isolation and re-running.
  • Verified end-to-end against a real running instance of mat3ra/web-app (dependency bumped to this commit): a job created via PUT /api/2018-10-01/jobs/create on the default k-grid path now persists kgrid with a sensible resolved grid, where before it was context: [].

Notes for reviewers

  • Full review record, including a documented blocker found and fixed in review (the material-pinning issue above): see the task's review log at reviews/mat3ra/wode/working-tree-2026-08-07/comments-c0f12bc.md in the planning repo (not part of this diff).
  • End-to-end verification also surfaced a separate, pre-existing bug in web-app, unrelated to this fix: globalSettings.defaultKPPRA (injected as 10 via imports/workflows/context/registry.ts) doesn't reach the module instance of wode used during job rendering, which falls back to wode's internal default of 5. Confirmed independent of this PR by reproducing the same behavior with the unmodified, currently-published @mat3ra/wode. Flagged separately, not addressed here.

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

VsevolodX and others added 3 commits August 7, 2026 19:45
…(SOF-7990)

ExecutionUnit.savePersistentContext only kept explicitly edited context items, so
the default (unedited) path never wrote a kgrid entry into an execution unit's
persisted context. assign-precision-for-material and similar consumers reading
context["kgrid"] unconditionally then hit a KeyError on any workflow that never
had its k-grid touched by the user.

PointsGridFormDataProvider (kgrid/qgrid/igrid) now opts into persisting its
resolved default via a new isPersistedWhenNotEdited flag, checked alongside
isEdited in savePersistentContext's filter.

A persisted-but-unedited item is read back into the provider on the next
construction (getContextProvidersInstances seeds providers from unit.context
regardless of isEdited), so without guarding against it the resolved default
would pin to whichever material was rendered first instead of recomputing per
material. PointsGridFormDataProvider's constructor now discards inherited data
when the incoming item isn't edited, so every render recomputes fresh while
still persisting the result.

Also fixes convergence_mixin.py's bare get_context_item("kgrid").get(...),
which raised AttributeError instead of the intended ValueError when no kgrid
context item exists yet.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Doc comments on the new isPersistedWhenNotEdited flag and its call sites had
grown into multi-paragraph essays, well past this codebase's actual
convention of dense one-liners. Cut each to a single line.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Comments should explain the why, not point back at the ticket that motivated
the change — that belongs in the PR description and commit history, not code
that outlives it.

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

Copy link
Copy Markdown
Member Author

Closing: the premise was never verified. The KeyError at assign-precision-for-material this fix targets was never reproduced — only inferred from an empty unit.context. Reopening the question of whether the fix belongs in wode's JS at all, given (1) it required a counter-fix to stop persisted defaults from pinning the first material's grid, and (2) it drops reported precision 10 -> 5 on default-path jobs. Will re-open or supersede once the actual failure is reproduced and located.

@VsevolodX VsevolodX closed this Aug 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant