fix(client): interpolate %{field} in nested-remove confirm on client-added rows (#222) - #223
Conversation
…added rows (#222) ## Summary A row added client-side via reactive_nested_add is a cloneNode of the <template>, and the clone path (#renumberNestedRow / #seedNestedRow) never rewrote data-reactive-confirm[-when]-param — so a per-row confirm like "Delete '%{name}'?" froze to the template's value-less string on every added row (the exact rows the primitive exists for). nestedRemove now resolves %{field} placeholders in the confirm message from THAT row's live field values (via #nestedRowObject, keyed by the same trailing-bracket inference as: :json uses) at click time — so a later edit is reflected too. An unresolved %{key} is left as its literal text (debuggable, never a silent blank); the confirm-when Hash message interpolates as well. Server-rendered rows already interpolate server-side, so their finished strings (no %{}) are a no-op. Superset of the issue's proposal 3: confirmResolver now receives an optional 2nd arg — { el } on every confirm path (dispatch/runOps/nestedRemove), plus { row, fields } on nestedRemove — so a themed-dialog override can build row-specific messages. Additive: a one-parameter resolver (and window.confirm) is unchanged. ## Test Coverage - spec/javascript/reactive_nested.test.js: 6 new — interpolation from live values, literal on missing key, client-added row reflects its own value, later-edit (click-time not clone-time), confirm-when message interpolation, resolver ctx { el, row, fields } - spec/system/draft_order_confirm_interpolate_spec.rb: 2 new — the ADDED row's own typed quantity interpolated into its confirm (Puma + Falcon), later edit - Existing #218 confirm-remove + #52/#55/#178/#179 confirm specs unchanged ## Verification - [x] bundle exec rubocop (288 files, clean) - [x] bundle exec rspec spec/phlex spec/requests (1357 examples) - [x] confirm + draft system specs green under Puma AND Falcon - [x] bun test spec/javascript (533) - [x] client rebuilt (rake build:js) + vendored copy re-synced (sync spec green)
📝 WalkthroughWalkthrough
ChangesNested-remove confirmation flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
coderabbitai
Bot
added
documentation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/app/views/docs/pages/draft_rows_new_parent.rb (1)
306-314: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a
DocsUI::Callout(:note)for the unresolved-placeholder caveat.This caveat text (unresolved
%{key}behavior, no-escape gotcha) is the same category of content as theCallout(:note)blocks used elsewhere in this file (json_mode,fill_then_add). As per coding guidelines: "Use the dedicated reference helpers (DocsUI::PropTable,DocsUI::FieldTable,DocsUI::RequestExample,DocsUI::Callout(:note | :tip | :warning)) before writing equivalent prose manually."♻️ Proposed refactor
- A `%{key}` with no matching row field is left as its literal text - (visible and debuggable, never a silent blank). The placeholder - works in the conditional Hash's `message:` too. Interpolation - applies to `reactive_nested_remove` confirms only — `on`/`on_client` - confirms never substitute — so if a remove confirm needs a - **literal** `%{word}` where `word` also happens to be a field on the - row, reword it (there is no escape); this only bites text that - deliberately contains a field name in braces. + The placeholder works in the conditional Hash's `message:` too. + Interpolation applies to `reactive_nested_remove` confirms only — + `on`/`on_client` confirms never substitute. MD + end + + DocsUI::Callout(:note) do + md <<~'MD' + A `%{key}` with no matching row field is left as its literal text + (visible and debuggable, never a silent blank). If a remove confirm + needs a **literal** `%{word}` where `word` also happens to be a + field on the row, reword it (there is no escape); this only bites + text that deliberately contains a field name in braces. + MD end🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/app/views/docs/pages/draft_rows_new_parent.rb` around lines 306 - 314, Replace the manual unresolved-placeholder caveat prose in the reactive_nested_remove documentation with a DocsUI::Callout(:note), following the existing json_mode and fill_then_add callout patterns in the same file; preserve all details about literal unresolved %{key} text, message interpolation, non-interpolated on/on_client confirms, and the lack of an escape mechanism.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@spec/javascript/reactive_nested.test.js`:
- Around line 47-51: Update the cleanup around stubConfirm to capture the
original window.confirm before stubbing, then in afterAll restore window.confirm
to that original implementation and reset confirmModule’s resolver to delegate
to the restored original (while retaining the non-browser fallback). Ensure
later tests do not inherit the stubbed confirmation behavior.
---
Nitpick comments:
In `@docs/app/views/docs/pages/draft_rows_new_parent.rb`:
- Around line 306-314: Replace the manual unresolved-placeholder caveat prose in
the reactive_nested_remove documentation with a DocsUI::Callout(:note),
following the existing json_mode and fill_then_add callout patterns in the same
file; preserve all details about literal unresolved %{key} text, message
interpolation, non-interpolated on/on_client confirms, and the lack of an escape
mechanism.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 83f0fe56-acc6-49b6-8ea6-f9872bdd99da
⛔ Files ignored due to path filters (3)
app/javascript/phlex/reactive/confirm.min.js.mapis excluded by!**/*.map,!**/*.min.js.mapapp/javascript/phlex/reactive/reactive_controller.min.jsis excluded by!**/*.min.jsapp/javascript/phlex/reactive/reactive_controller.min.js.mapis excluded by!**/*.map,!**/*.min.js.map
📒 Files selected for processing (11)
CHANGELOG.mdREADME.mdapp/javascript/phlex/reactive/confirm.jsapp/javascript/phlex/reactive/reactive_controller.jsdocs/app/views/docs/pages/draft_rows_new_parent.rbspec/dummy/app/components/draft_order_confirm_interpolate_component.rbspec/dummy/app/controllers/demos_controller.rbspec/dummy/config/routes.rbspec/dummy/public/vendor/reactive_controller.jsspec/javascript/reactive_nested.test.jsspec/system/draft_order_confirm_interpolate_spec.rb
|
Thanks — I looked at this closely and it's a push-back: the described leak can't occur, and the suggested change would actually make it less correct. The
Capturing/restoring the "original" Verified empirically — both cross-file orderings and the full suite are green: Leaving the |
1 participant
Closes #222.
The bug
reactive_nested_remove(confirm:)(#218) renders the confirm message per row, which is correct for rows the server rendered. But a row added client-side viareactive_nested_addis acloneNode(true)of the<template>, and the clone path (#renumberNestedRow/#seedNestedRow) only rewritesname/id/for— it never touchesdata-reactive-confirm[-when]-param. So every client-added row carried the template's confirm string (a value-less"Delete '%{name}'?"or a blank-interpolated string), not one that reflected the row the user actually built — on exactly the rows the primitive exists for.The fix (issue proposal 2 + a superset of proposal 3)
Click-time
%{field}interpolation. On remove,nestedRemoveresolves%{field}placeholders in the confirm message from that row's live field values — read from the row's own inputs via#nestedRowObject(keyed by the same trailing-bracket inferenceas: :jsonuses, so the two can't drift). Because it resolves at click time (not clone time), a later edit to the row is reflected too. This fixes the bug for every app with zero app JS, on the defaultwindow.confirm.%{key}(no matching row field) is left as its literal text — visible and debuggable, never a silent blank.{ when:, message: }Hash'smessage:is interpolated too.%{}), so this is a no-op for them.Optional resolver context (proposal 3, as a superset).
confirmResolvernow receives an optional 2nd argument:{ el }(the trigger) on every confirm path —dispatch,runOps,nestedRemove— plus{ row, fields }onnestedRemove. A themed-dialog override can build row-specific messages programmatically. The arg is purely additive: a one-parameter resolver (andwindow.confirm) is unchanged.Test coverage
reactive_nested.test.js, 6 new){ el, row, fields }draft_order_confirm_interpolate_spec.rb, 2 new)All gates green:
rubocop(288 files),rspec spec/phlex spec/requests(1357), the confirm + draft system suite under Puma and Falcon,bun test spec/javascript(533). Client rebuilt (rake build:js) and the vendored copy re-synced (byte-identical, sync spec green). An adversarial multi-agent review found no blocking bugs — it empirically verified (Node) that$-replacement patterns in a field value are inserted literally (function callback, not string replacement), that the substitution is non-recursive (a field value of%{x}does not then resolve fieldx— no injection, no infinite loop), and that the field-map keys match the placeholder in both wire modes.Docs & changelog
setConfirmResolversection documents the ctx 2nd arg +%{field}on client-added rows.docs/…/draft_rows_new_parent.rb"Confirm before removing a row" now distinguishes server-rendered (interpolate at render) from client-added (%{field}placeholder).CHANGELOG.mdunder Unreleased → Fixed.Deviations & judgment calls
confirmResolvercall sites, not justnestedRemove.dispatch/runOpsnow pass{ el }so an app's resolver override has a uniform(message, ctx)signature everywhere (one function covers every confirm path);nestedRemovepasses the fuller{ el, row, fields }. Interpolation itself is nested-only (onlynestedRemovehas a row). Judgment: a uniform signature beats a nested-only 2nd arg that would makectxsometimes-present.runOpscaptured no trigger element before this change — addedconst trigger = event.currentTarget ?? event.target(the idiomdispatchalready uses) purely to populatectx.el. Inert otherwise.confirmResolveris module-global mutable state and bun runs test files in one worker. The new ctx test installs a(message, ctx) => falseresolver, which leaked intoreactive_confirm.test.js(relies on the defaultwindow.confirm, no reset) and zeroed its fetches. Fixed with anafterAllinreactive_nested.test.jsthat restores the shipped default — mirroringreactive_confirm_resolver.test.js. Not a product bug; a gap the new test surfaced.reactive_nested_removeconfirm containing a literal%{word}is now interpolated if that row has a field keyedword(e.g. a message"…20%{discount} rule…"on a row with adiscountfield →"…2010 rule…"). Blast radius: onlyreactive_nested_remove(neveron/on_client), only a persisted row whose message literally contains%{fieldkey}. There is no%%escape (adding one is more surface than this contrived case warrants).%{}was never a documented-safe literal in a confirm; the docs now explicitly flag this collision as a footgun and the CHANGELOG names%{field}as the syntax.%{key}visible rather than empty-substitute.%{field}(Ruby-style, matches the issue). Required one scopedrubocop:disable Style/FormatStringTokenon the dummy component + docs page (these are client-interpolation templates, not Ruby format strings), each with a rationale comment.Summary by CodeRabbit
Bug Fixes
Enhancements
Documentation