Skip to content

repeater card UI for Create/Update/Index properties#1227

Draft
jingcheng16 wants to merge 17 commits intojc/save-to-case-basefrom
jc/new-repeater-pattern
Draft

repeater card UI for Create/Update/Index properties#1227
jingcheng16 wants to merge 17 commits intojc/save-to-case-basefrom
jc/new-repeater-pattern

Conversation

@jingcheng16
Copy link
Copy Markdown
Contributor

@jingcheng16 jingcheng16 commented Apr 23, 2026

Product Description

Vellum has a recurring widget pattern for editing a list of structured items. Every time it shows up, it's rendered as a gray background card containing three or four rows, each a full-width whose only clue about what it represents is the placeholder text. No labels. Little visual grouping that says "these three fields belong together". No indication and no validation of which fields are required vs. optional.

Before
Screenshot 2026-04-23 at 12 12 02 AM

This PR is to replace the old gray card with a new card:

  • Field labe is now on the left instead of as placeholder
  • Calculation and Condition are now both fully-supported Xpath field
  • Errors are now attached to individual fields inside the card with a summary message at the end of the list

Create Section

Screenshot 2026-04-23 at 12 00 38 AM

Update Section

Screenshot 2026-04-23 at 12 11 17 AM

Index Section

Screenshot 2026-04-23 at 12 03 43 AM

Inline Validation + Widget-level message at the end of the list
Screenshot 2026-04-23 at 12 12 43 AM

Technical Summary

Ticket: https://dimagi.atlassian.net/browse/SAAS-19666
The ticket doesn't has any useful content. It is just created to track my work.

Design Spec: https://docs.google.com/document/d/1a2S1b1spUumzGV82FrrloZ01GER5qkAnwtjg12JhAto/edit?tab=t.2g3s56t3ix7z#bookmark=id.kiknyxtcwm8j

Renaming mentioned in the tech spec will be done in a follow up PR

Review by commit

I tried my best to improve the readability of the code and admit there are still parts that even me find not very intuitive. I had to use a lot of real form to test the feature, come up with edge case to test the feature to make sure it won't break anything.

Feature Flag

VELLUM_SAVE_TO_CASE

Safety Assurance

Safety story

  • Data model is unchanged. mug.p.createProperty / updateProperty / indexProperty JSON shape is identical; all existing XML fixture round-trips (loadXML(UPDATE_PROPERTY_XML)assertXmlEqual(call("createXML"), UPDATE_PROPERTY_XML)) pass untouched.
  • Tested using real form on prod env.

Automated test coverage

new tests across tests/saveToCase.js and tests/widgets.js

QA Plan

Manual spot-checks worth doing in a dev env:

  1. Load an existing form with SaveToCase mugs (Create + Update + Index populated). Verify UI renders cards identically to before — same fields, same values — and a round-trip save produces identical XML.
  2. Click "Add property" in each section. Type a name, type an xpath. Verify inline validation fires for bad names/expressions and clears once fixed. Save; reload; verify the row persists.
  3. Drag a question from the tree onto an xpath field inside a card. Verify it inserts as a bubble (rich-text) or string (plain) as appropriate.
  4. Remove a card that has validation errors. Verify the widget-level alert + tree-triangle + save-tooltip clear when the errored card is gone.
  5. Try the example form pattern from a customer: one SaveToCase creates a case, another indexes it via /data/.../case/@case_id. Verify no spurious "Unknown question" warning appears.

Rollback instructions

  • This PR can be reverted after deploy with no further considerations

Data shapes are unchanged; a revert brings back the old UI without any migration. Saved XMLs produced on the new UI are indistinguishable from ones produced on the old.

Labels & Review

  • Risk label is set correctly
  • The set of people pinged as reviewers is appropriate for the level of risk of the change

@jingcheng16 jingcheng16 changed the title SaveToCase: card-based repeater UI for Create/Update/Index properties repeater card UI for Create/Update/Index properties Apr 23, 2026
@jingcheng16 jingcheng16 force-pushed the jc/new-repeater-pattern branch 3 times, most recently from 7d64fcf to 2465778 Compare April 23, 2026 17:43
@jingcheng16 jingcheng16 marked this pull request as ready for review April 23, 2026 17:46
jingcheng16 and others added 9 commits April 23, 2026 16:43
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The established pattern (itext) defines `mapLogicExpressions(fn)` and
`updateLogicExpressions(fn)` on the stored value object itself. That
doesn't work for widgets whose value is a plain map, eg:
  mug.p.createProperty === {
      age:  { calculate: "/data/actual_age" },
      name: { calculate: "/data/first_name" },
  }
since you can't attach methods to plain data without polluting it. A later commit will allow the spec to carry the method instead. This commit is to prepare for that.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Outer template that renders a config-driven card list: one card per
record, iterating cardConfig.fields per card and branching per field's
`widget` type (input / xpath / dropdown) — xpath and dropdown delegate
to the template partials added in the previous two commits. Outer
`.fd-repeater-list` wrapper, per-card panel, and the Add button all
live in this one template.

No consumer yet — widgets.repeaterCard and the SaveToCase port land
in subsequent commits. Old widget_update_case.html /
widget_index_case.html still exist and are what current
saveCasePropWidget / indexCaseWidget render through.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is intentionally core-rendering only. The inline-driven
validation architecture (touched-state tracking, validateField,
syncMugMessages, getMessages override, save-button hover hook)
lands in a dedicated follow-up so it can be reviewed on its own.

No consumer wired yet — the SaveToCase port is the next commit.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@jingcheng16 jingcheng16 force-pushed the jc/new-repeater-pattern branch 2 times, most recently from 51a4546 to a5dbdbb Compare April 24, 2026 22:09
jingcheng16 and others added 8 commits April 24, 2026 23:21
Swap the three compound-list properties (createProperty, updateProperty,
indexProperty) to the new
widgets.repeaterCard with declarative cardConfig.

Validation at this commit follows the old pattern — validationFunc
walks mug.p via hasNestedCardError and returns errorSummary. A
follow-up commit replaces that with an inline-driven architecture;
this commit lands the port without changing validation semantics.

Known regression at this commit: drag-drop onto a nested xpath field
inside a card misroutes because the old handleDropFinish checks the
enclosing widget's options.richText (which is false on the repeater
widget). Fixed in the next commit.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The nested xpath fields just introduced by widgets.repeaterCard are
the first place a rich-text target element lives inside a widget whose
options.richText = false. The old handleDropFinish check
`widgets.util.getWidget(target, this).options.richText` asks the
enclosing widget "should I use rich-text?" which misses that case
(always picks the val()-concat branch, which doesn't work on a
contenteditable div). It also crashes when getWidget returns null
(e.g. xpath-editor modal fields that aren't inside a .widget).

Inspect the target element directly: `target.data("editorWrapper")`.
richText.editor() sets this data property on every element it attaches
to, so it's the direct source of truth for "is this a rich-text
target." Superset of the old behavior for every existing case, plus
handles nested rich-text fields and the null-widget edge.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Single source of truth for "this repeater card has an error": the
inline per-field validator running on a touched-state gate. The widget
pushes one mug-<path>-inline-error message (content:
cardConfig.errorSummary) from the DOM's .has-error count. Widget-level
alert, tree triangle, and save tooltip all read the same mug.messages
entry — they can't disagree.

widgets.repeaterCard additions:
- validateField helper — Required (touched + empty), XPath syntax,
  unknown-reference (via new findUnknownReferences), and a per-field
  extraValidator hook for call-site-specific checks.
- widget.validateCard — walks every [data-widget] element in a card,
  calls validateField, then syncMugMessages.
- widget.syncMugMessages — inspects DOM .has-error count and writes or
  drops mug-<path>-inline-error.
- widget.getMessages override — prefers validationFunc text under
  mug-<path>-error (empty-state, list-level messages); falls back to
  mug-<path>-inline-error text.
- Touched seeding on render: cards loaded from saved data start with
  every field touched; fresh blank cards (added via Add button) stay
  untouched until the user engages a specific field.
- Touched marking on input/change/keyup and rich-text editor change.
- Save-button mouseenter hook force-touches every field and re-
  validates. Namespaced listener cleaned up on teardown.
- messages-changed listener re-runs validateCard so inline "Unknown
  question" text stays current after external delete/rename.

SaveToCase simplifications:
- Add inline validator functions: validatePropertyNameChars,
  validateCreatePropertyName (reserved-name + chars),
  validateRelationshipChoice. Wire as `extraValidator` on the
  corresponding cardConfig fields.
- Delete hasNestedCardError helper — inline handles it now.
- Simplify the three validationFuncs to only list-level gates:
  createProperty becomes an empty block (fully inline);
  updateProperty / indexProperty keep only their requiresAtLeastOne
  empty-state check.

Result: one place says "what's wrong" (inline under the specific
field), one place says "there's something above" (widget-level alert
driven by the DOM count), and the global error signals are aligned.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The inline validator's `findUnknownReferences` in `widgets.repeaterCard`
asked `form.getMugByPath(pathString)` and flagged anything that didn't
resolve. But references into a mug's generated XML subtree — notably
`/data/<stc>/case/@case_id` — don't resolve to a standalone mug and
were incorrectly flagged as "Unknown question" in the repeater-card
fields.

This pattern is standard in CommCare: one SaveToCase creates a case
(its `case/@case_id` set via `<setvalue value="uuid()">`), and a
sibling SaveToCase references that attribute as the parent id in its
index section. Valid XPath, valid reference — but our inline validator
was stricter than it should be.

Introduce `isValidOutputSubPath(mug, suffix)` as a per-mug-type spec
hook that declares which sub-paths of its absolute path are valid
references. When a path doesn't resolve directly and isn't a hashtag,
walk up to the first ancestor mug and defer to that mug's type hook
to decide whether the remaining suffix is accepted. Types without the
hook keep strict pre-walk-up behavior — no sub-path tolerance.

For SaveToCase the whitelist is intentionally narrow: just the three
case-identifying attributes (`@case_id`, `@date_modified`, `@user_id`).
`dataChildFilter` emits a larger tree (`/case/create/*`, `/case/update
/<k>`, `/case/index/<k>`, `/case/close`, bare `/case`), but those are
*output bind targets*, not paths that anything in a real form
references back. If a use case emerges for the other sub-paths, adding
them is cheap; a too-broad whitelist hides typos.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This PR wires up spec-level `mapLogicExpressions` for SaveToCase's
repeater-card properties (createProperty / updateProperty /
indexProperty). Before this PR, logic.js never walked those nested
xpaths — so the reference tracker never saw paths like
`/data/stc/case/@case_id` and never had reason to evaluate them. With
the spec hook now feeding them in, logic.js's existing strict
`form.getMugByPath(pathString)` check flags them as "Unknown question"
on the form-tree triangle.

These attribute-path references are a standard CommCare idiom: one
SaveToCase creates a case (its `case/@case_id` set via
`<setvalue value="uuid()">`), and a sibling SaveToCase references that
attribute as the parent id in its index section. They were never
broken in prod — they were invisible to the reference tracker
entirely.

Apply the same `isValidOutputSubPath` check introduced in the
widgets.js walk-up: walk up to the first ancestor mug, defer to its
type's hook to decide whether the suffix is valid. Same rule in both
call sites, so the fix covers every surface that displays "Unknown
question" text — tree triangle, regular xpath widget messages, and
the repeater-card inline validator.

Tests cover: valid `@case_id` references pass, typo attribute names
flag, non-whitelisted sub-paths (`/case/update/<k>`) flag, and
non-SaveToCase mugs still reject arbitrary sub-paths.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The repeater-card widget's inline validator is DOM-driven and gated
on each field's touched state. That's correct for interactive
feedback (don't nag about empty required fields before the user has
engaged), but it means any code path that calls mug.validate() without
going through the widget — most notably `isFormValid()` / the Save
button's pre-submit check — never sees per-field errors. Forms with
broken, untouched cards would round-trip through save cleanly.

Add a data-side mirror (`hasCardFieldError`) that applies the same
rules as `validateField` — Required, XPath syntax, extraValidator —
but reads mug.p directly, no touched gating. Wire it into the
createProperty / updateProperty / indexProperty validationFuncs so
`mug.validate()` picks up the errors and the widget-level message +
tree triangle + save tooltip all fire.

Skip entirely-empty placeholder rows (blank identifier, every valueKey
empty) to match the repeater card's add-blank-card UX — same
heuristic the pre-PR hasNestedCardError used.

Updated the "should only allow extension and child as relationship"
test: widget-level message is now `cardConfig.errorSummary` instead
of the specific per-field validator text (per-field text shows inline
now; covered by dedicated inline-validator tests).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
repeaterCard assigned `id = options.id` to widget.input's `name` attr,
but for spec-defined widgets `options.id` is undefined — the base
`normal()` widget defaults to `'property-' + options.path` via its own
setup, but `repeaterCard` builds its own `widget.input` DOM element and
skipped that fallback.

Result: `<div class="control-row" name="undefined" />` — so selectors
like `$("[name='property-updateProperty']")` in both tests and other
callers couldn't find the widget. Add, remove, typing, and inline-
validator flows all rely on that selector path, which is why four tests
in `repeater card DOM interactions` and `inline validators` were
failing even though the widget rendered correctly.

Apply the same default as normal(): `options.id || 'property-' + path`.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
jQuery's $(string) parses as HTML, which uppercases tagName on the
resulting nodes. $(call("createXML")) returns the child element as
<NAME>, so `.prop('tagName')` returns "NAME" and the assertion
`assert.equal(..., 'name')` failed. Lowercase the actual before
comparing.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@jingcheng16 jingcheng16 marked this pull request as draft April 25, 2026 03:24
@jingcheng16 jingcheng16 force-pushed the jc/new-repeater-pattern branch from a5dbdbb to 3aecf09 Compare April 25, 2026 03:24
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