filament icon indicating copy to clipboard operation
filament copied to clipboard

fix(repeater): bubble afterStateUpdated to parents

Open Domianidze opened this issue 8 months ago • 6 comments

Description

This PR addresses an issue where afterStateUpdated callbacks on parent components were not being triggered after running repeater-related actions such as add, delete, reorder, etc.

In these cases, the component would call afterStateUpdated on itself at the end of the action, but unlike normal state updates, the update did not bubble up to its parent components. This meant any logic relying on afterStateUpdated at the parent level would silently fail to run.

To fix this, a new helper was introduced that first checks if the component is live, and then recursively walks up the component tree, calling afterStateUpdated on each parent. This mirrors the behavior of actual state updates, ensuring that all necessary callbacks are triggered.

This approach has worked well in our testing and reflects the expected behavior of how state updates should propagate. Let me know if there’s a more appropriate way to handle this logic, or if there are any edge cases we might’ve missed.

Visual changes

No visual changes.

Functional changes

  • [x] Code style has been fixed by running the composer cs command.
  • [x] Changes have been tested to not break existing functionality.
  • [x] Documentation is up-to-date.

Domianidze avatar Apr 21 '25 16:04 Domianidze

When you mention parent components, can you provide an example of some code that is currently broken? I'm not even sure that running the hook on parents is desired behaviour but I would like to hear what you are using this for first?

danharrin avatar Apr 26 '25 15:04 danharrin

For example, in our case we have a Section component that contains various fields, some of which are Repeaters. On the side, we have a live preview that should update anytime any field changes. On the section we have an afterStateUpdated function set that refreshes the preview.

Example:

Section::make()
    ->schema([
        TextInput::make('heading'),
        Repeater::make('blocks')
            ->schema([
                TextInput::make('image'),
            ]),
    ])
    ->afterStateUpdated(fn () => $this->updatePreview())
    ->live(),

Normally, if a user edits the heading field, the afterStateUpdated hook runs, and the preview refreshes.

Problem:

If a user adds, deletes, reorders or does any of the repeater actions inside the blocks Repeater, the Repeater would call its own afterStateUpdated, but the Section’s afterStateUpdated doesn’t get triggered. As a result, the preview doesn’t update.

This is why we made the change to bubble afterStateUpdated up through the parent components to match how normal state updates behave.

Domianidze avatar Apr 26 '25 23:04 Domianidze

Does the section have a statePath() defined or anything in your example?

danharrin avatar Apr 27 '25 11:04 danharrin

Only thing relevant it has is live(), which is what causes the normal field changes to bubble up the afterStateUpdated to the Section. That’s why I’m checking if the Repeater is live before bubbling it up to mirror the normal state updates.

(Sorry, I forgot to include that in the example.)

Domianidze avatar Apr 27 '25 12:04 Domianidze

Honestly that is super weird behaviour, I never knew it did that. So I will need to look into the underlying cause and decide whether or not that is desired behaviour. Sorry, that will result in a delay to this PR.

danharrin avatar Apr 27 '25 12:04 danharrin

No worries, I’ll be here to help if I can.

Domianidze avatar Apr 27 '25 12:04 Domianidze

Definitely a great improvement, and it behaves as it should. It was just missing the check for whether the initial component was live, so I added that :)

Domianidze avatar Jun 03 '25 20:06 Domianidze

Yeah, but without checking if it’s live, it happens instantly in this case—rather than during a Livewire request or when another input is updated. I added it because the goal of this PR was to mimic how normal state updates behave, which wouldn’t reflect the change immediately if the component wasn’t live. But if you think it’s unnecessary, I’m happy to remove it.

Domianidze avatar Jun 03 '25 20:06 Domianidze

Even if it isn't live though, it still happens at some point. Currently, the way it is it means that it never gets executed if its not live, right?

Since adding items to a repeater sends a Livewire request, the repeater doesn't need to be live for that to happen either

Are there any other disadvantages to removing it? If not, please proceed

danharrin avatar Jun 03 '25 21:06 danharrin

Well, after actually checking, it turns out that with normal state updates, afterStateUpdated doesn’t bubble if the component isn’t live—even after a Livewire request or when another input is updated. So the current version, with the live check, does actually mimic normal state update behavior. Do you still want to remove it?

Domianidze avatar Jun 03 '25 21:06 Domianidze

I can't reproduce that behaviour, and update hooks should run whenever a component is updated even if its not live, so you can keep the rest of the form's state up to date. Its a more consistent behaviour this way as well

danharrin avatar Jun 10 '25 15:06 danharrin

I was talking about the update hook bubbling and not just running, but sure, maybe I missed something. Thanks, Dan.

Domianidze avatar Jun 10 '25 18:06 Domianidze