fix: data items nested templates

[chrismclarke]

PR Checklist

• PR title descriptive (can be used in release notes)

Description

Fix an issue where updates to local variables would not inherit through to child templates within data_items components

Author Notes

There is a bit of potential for knock-on effects of the changes so visual tests have been added to the PR, it would be good to double check against anything flagged by the tests or any particularly complex nested templating examples that you can think of

As a small bonus the fix should have also removed the need to aggressively remove and recreate all templates when using the emit: force_reload action, which previously resulted in a flicker of empty page and then recreated content on emit.

Review Notes

Should fix test case from debug_data_items_local

Would also be good to confirm if it fixes any similar issues seen elsewhere

Dev Notes

Looking through the code and example it was clear that the changes to local variables were correctly being passed through, but the final level of nested templates were not re-processing. I think this makes sense given that the nested templates are essentially detached from the main chain when rendered as data_items. In addition the main template-container component was only set to reprocess a template when it's input templatename changed (and not input row which could include inherited properties)

I'm not actually sure if this is an issue specific only to data_items or could also appear elsewhere with similar nesting structures. In any case the fix is a somewhat hacky workaround to force reprocessing of nested templates when the parent input row changes.

As a side-effect the template-row renderedRows output has been converted to a signal (to fix a change detection issue), and a previous workaround to set the output first to an empty array before reprocessing (to force change detection) removed. This should mean now that the emit: force_reload action should no longer have a flicker of empty content between renders.

I haven't attempted to benchmark whether the overall changes result in any performance gains or losses (assume it should mostly effect a minimal number of templates)

Git Issues

Closes #2636
Closes #2637

Screenshots/Videos

Screenity.video.-.Dec.17.2024.2.webm

[github-actions]

Visual Test Summary
new : 0
different : 12
same : 304

Largest Differences
1 | 5.5 % | feat_animated_slides
2 | 2.9 % | example_lang_template_pop_2
3 | 0.4 % | example_lang_template_pop_1
4 | 0.4 % | user_actions
5 | 0.3 % | debug_data_items_condition
6 | 0.2 % | debug_advanced_dashed_box_1
7 | 0.2 % | example_calc_date
8 | 0.1 % | debug_set_field_2
9 | 0.1 % | example_calc
10 | 0.1 % | example_parent_point_box_info

Download Link
https://nightly.link/IDEMSInternational/parenting-app-ui/actions/runs/12392323903

Run Details
https://github.com/IDEMSInternational/parenting-app-ui/actions/runs/12392323903

[jfmcquade]

Thanks, @chrismclarke. I can see how this directly addresses the issue.

I'm happy with the code and my functional testing of the debug template shows it working as expected.

[esmeetewinkel]

Would also be good to confirm if it fixes any similar issues seen elsewhere

I reviewed this on the plh_kids_kw deployment, where the issue first showed up.

On this PR branch it seems that this condition somehow broke. On initial load the screen shows correctly, but when I'm trying to "navigate" (that is, updating a local variable) to reveal the next subtemplate and hide the current, instead all subtemplates are revealed.

Screenity.video.-.Dec.18.2024.2.mp4

[chrismclarke]

@jfmcquade @esmeetewinkel
I believe the issues seen in the plh_kids_kw app should be fixed with the commit
6ff0da8

This takes a slightly more heavy-handed approach to re-processing child rows if a parent template changes the value of a child template row. The template container no longer reprocessed rows, but instead is fully destroyed and recreated. I think this makes more sense given that it needs to load an entirely different template, which would likely destroy existing child rows and recreate new ones in the process anyways.

@jfmcquade
The code now more selectively forces the container recreation only when the template value changes (fixing the issue for plh_kids_kw), in theory I thought this would mean the original debug case would stop working (as the template name doesn't change, just one of the parent variables), however the debug case is still working so I'm guessing that was more related to change detection rather than re-processing.

Also, despite all the troubleshooting I failed to add any new spec tests or code to try and help troubleshoot if similar issues come up in the future (really hope it just doesn't! 😅 ) , but did add a few extra code comments here and there.

[jfmcquade]

Thanks, @chrismclarke.

I can't say I fully understand the issues with change detection and reprocessing, but your high-level description makes sense, and is reflected in the code.

I'm happy to approve if the test cases and production use-cases are working as expected.

I'm not sure why the visual tests are failing, but probably fine to disable them on this PR for now, I don't think it's fundamentally an issue with anything changed here.

[jfmcquade]

This use of the comma operator isn't clear to me.

I'm supposing that the signal value is being accessed here to ensure that any reactive side-effects (including ultimately updating this._row.value) have been triggered before accessing the value, is that right? Or else, to achieve the same result of not accessing the value until any signal-based changes have propagated, I can see how this line could be establishing a reactive dependency between this function and the source signal that wouldn't otherwise be there.

If that's right, might it be clearer to access the signal outside of the if statement and add a comment to explain the dependency being established? I think my confusion is mostly down to unfamiliarity with the comma operator, I can see how this could be an established pattern for making sure the first expression has evaluated and any side-effects propagated before evaluating the first expression, but I wouldn't have expected this to be required when using signals.

[chrismclarke]

Whoops, that was meant to be a !== operator, although when I change it the debug cases fails to update when parent local variable (as was actually expected, and confusing why it was passing)

So I think for now we'll just have to stick with the heavier condition to always fully recreate when the parent of a child template changes (first part of componentRef.instance.templateName being truthy), and try to improve a more finegrained reprocessing when refactoring the templating system

[chrismclarke]

Updated in #2647

[chrismclarke]

As for the visual-tests failing, I see this happening generally example failed action

[1220/175851.640539:FATAL:zygote_host_impl_linux.cc(117)] No usable sandbox! Update your kernel or see 
https://chromium.googlesource.com/chromium/src/+/main/docs/linux/suid_sandbox_development.md for more information on developing with the SUID sandbox. If you want to live dangerously and need an immediate workaround, you can try using --no-sandbox.

so I think I'll give it a couple days to see if it's an issue resolved within github actions (could be an issue related to internal action runner updates), before looking more closely and trying to update the action/dependencies

[jfmcquade]

Might benefit from an explanatory comment on why data_items and items rows are excluded

[chrismclarke]

Updated in #2647

[esmeetewinkel]

Functional test passed