fix: keep multi-choice visibleJs expressions attribute-safe and id-typed#178
Merged
Merged
Conversation
v3.5.2 evaluates multi-choice contains conditions as exact option membership,
but the generated client (visibleJs) expression broke in the browser so the
dependent field never showed when the trigger option was selected:
- Option ids arrive from Livewire state as strings while the resolved condition
ids are integers, so strict `includes()` never matched. Both sides are now
stringified.
- When the controlling field's options were not eager-loaded, the expression
fell back to comparing option names and emitted json_encode double quotes,
which truncated Filament's `x-bind:class="{ 'fi-hidden': !(…) }"` attribute
and threw an Alpine parse error. Options are now loaded via loadMissing(), JS
strings are single-quoted (never double), and expressions are single-line.
Adds a frontend-expression regression test (attribute-safe, id-based,
string-normalized) covering what the prior server-only tests could not catch.
There was a problem hiding this comment.
Pull request overview
This PR fixes Filament/Alpine visibleJs generation for option-backed multi-choice conditional visibility so client-side rendering matches server-side evaluation and remains safe to embed inside Filament’s double-quoted x-bind:class attribute.
Changes:
- Normalize multi-choice membership comparisons by stringifying both selected ids and condition ids to avoid strict
includes()mismatches ("204"vs204). - Replace JSON/double-quoted JS string emission with single-quoted, single-line expressions intended to be attribute-safe.
- Add/adjust feature tests to assert attribute-safety (no
"/ no newlines) and that multi-choicecontainsresolves option names to numeric ids.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/Services/Visibility/FrontendVisibilityService.php | Updates JS expression generation: option loading, single-line expressions, string normalization, and new single-quoted string literal formatter. |
| tests/Feature/UnifiedVisibilityConsistencyTest.php | Strengthens frontend-expression assertions (no double quotes/newlines) and adds a regression test for multi-choice contains behavior. |
| tests/Feature/SectionVisibilityIntegrationTest.php | Updates expectations to match single-quoted string output and ensure no double quotes are emitted. |
Comment on lines
+629
to
+633
| $escaped = str_replace( | ||
| ['\\', "'", "\r", "\n", "\t"], | ||
| ['\\\\', "\\'", '', ' ', ' '], | ||
| $value, | ||
| ); |
Comment on lines
+444
to
+446
| return is_array($resolvedValue) | ||
| ? "(() => { | ||
| const fieldVal = Array.isArray({$fieldValue}) ? {$fieldValue} : []; | ||
| const conditionVal = {$jsValue}; | ||
| return conditionVal.some(id => fieldVal.includes(id)); | ||
| })()" | ||
| : "(() => { | ||
| const fieldVal = Array.isArray({$fieldValue}) ? {$fieldValue} : []; | ||
| return fieldVal.includes({$jsValue}); | ||
| })()"; | ||
| ? "({$jsValue}.map(v => String(v)).some(id => {$selected}.includes(id)))" | ||
| : "({$selected}.includes(String({$jsValue})))"; |
Comment on lines
+227
to
+231
| // Option resolution (name -> id) and the option-backed choice branch both depend on the | ||
| // target field's options being loaded. Callers do not always eager-load them, so guarantee | ||
| // it here — otherwise a choice condition falls back to comparing option names against the | ||
| // selected ids, which never matches and emits quoted strings that break the class binding. | ||
| $targetField?->loadMissing('options'); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #177 (v3.5.2). That PR made multi-choice
containsevaluate as exact option membership on the server, but the client (visibleJs) expression broke in the browser, so a dependent field never appeared when its trigger option was selected (reported on Abode's multi-select forms).Root causes (browser-only — the v3.5.2 tests were server-side and missed them)
"204"), but the resolved condition id was an integer ([204]).[204].some(id => ["204"].includes(id))isfalseunder strictincludes(), so the field never showed. (v3.5.1's substring path accidentally stringified, which is why the older behaviour "worked".)optionsweren't eager-loaded, the expression fell back to comparing option names andjson_encode'd them to"Other"— the double quote truncates Filament'sx-bind:class="{ 'fi-hidden': !(…) }"attribute, throwingAlpine Expression Error: Unexpected token '}'and dropping the field from the DOM.Fix
.map(v => String(v))).loadMissing('options')on the trigger field so choice conditions always resolve names → numeric ids.Verified end-to-end in a real browser (Filament v5 create form)
contains "Other"condition (substring would have).Adds a frontend-expression regression test (attribute-safe, id-based, string-normalized) that the prior server-only tests could not catch. Full package suite green, Pint + PHPStan clean.