refactor: effect signal writes

[chrismclarke]

PR Checklist

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

Description

Following discussion in #3304, remove legacy allowSignalWrites param and improve styleguide for derived state

Dev Notes

Hopefully this should make automated reviews a little better informed, I think one thing possibly lacking in some of our own dev previously is the use of LinkedSignals (which moved into stable in angular 20) that could solve some of our use cases (but notably not all where async operations are involved)

Git Issues

Closes #

Screenshots/Videos

If useful, provide screenshot or capture to highlight main changes

[chrismclarke]

/gemini review

[gemini-code-assist]

Code Review

This pull request refactors several effect calls by removing the allowSignalWrites: true option. While this is a positive step towards cleaner signal usage, it's important to note that in all instances, the effect still performs writes to other signals, either directly or indirectly through method calls. The repository's style guide (Repository Style Guide, lines 68-69) explicitly advises: "Avoid using effects to update other signals." This practice can lead to complex reactivity graphs and make state management harder to reason about. Each instance where an effect updates a signal should be re-evaluated to determine if the update can be handled by a computed signal, an event handler, or if it represents an acceptable exception to the style guide with clear justification.

[gemini-code-assist]

This effect indirectly updates a signal (_expression via setChildExpression and setExpression). The repository style guide (Repository Style Guide, lines 68-69) states: "Avoid using effects to update other signals." This pattern can lead to complex reactivity graphs. Re-evaluate if this signal update can be handled outside of an effect.

        this.onTemplateInitialised();
        this.templateInitialised = true;

[gemini-code-assist]

This effect indirectly updates the marginTop signal via setHeaderCollapse and removeScrollEventListeners. The repository style guide (Repository Style Guide, lines 68-69) states: "Avoid using effects to update other signals." Consider refactoring this to update the signal directly in response to an event or as a computed property if possible.

      if (collapse !== undefined) {
        this.setHeaderCollapse(collapse);
      }

[gemini-code-assist]

This effect updates the shouldShow signal. The repository style guide (Repository Style Guide, lines 68-69) states: "Avoid using effects to update other signals." Consider if shouldShow could be a computed signal if its value is purely derived from rowSignal().rows?.length and itemRows()?.length.

      const shouldHide = !this.rowSignal().rows?.length || !this.itemRows()?.length;
      this.shouldShow.set(!shouldHide);

[gemini-code-assist]

This effect indirectly updates svgPath, svgViewBox, and contentHeight signals via generateSVGPath. The repository style guide (Repository Style Guide, lines 68-69) states: "Avoid using effects to update other signals." These updates should ideally be handled outside of an effect or be part of computed signals if they are synchronous derivations.

      const { variant, verticalSpacing } = this.params();
      this.generateSVGPath(variant, verticalSpacing);

[gemini-code-assist]

This effect updates the templateAssetService.assetsContentsList signal. The repository style guide (Repository Style Guide, lines 68-69) states: "Avoid using effects to update other signals." Consider if assetsContentsList could be a computed signal if its value is purely derived from coreAssetContentsData().

            const assetContentsHashmap = arrayToHashmap(dataRows, "id") as IAssetContents;
            this.templateAssetService.assetsContentsList.set(assetContentsHashmap);

[jfmcquade]

Looks good.

For this review, Gemini is still using the style guide from master, so it remains to be seen how it will interpret the instructions in cases like this, but the wording looks good to me.

And good for me to have a look into linkedSignal, I remember seeing it a while ago when I was less familiar with signals in general, I think I can see the use for it now. My understanding is that it is effectively a computed property that can have it's value overridden explicitly.

[chrismclarke]

Looks good.

For this review, Gemini is still using the style guide from master, so it remains to be seen how it will interpret the instructions in cases like this, but the wording looks good to me.

Also the changes are focused on the files that used the previous allowSignalWrites param so we know they are candidates for refactor anyway, but yeah beyond scope of this PR to address.

And good for me to have a look into linkedSignal, I remember seeing it a while ago when I was less familiar with signals in general, I think I can see the use for it now. My understanding is that it is effectively a computed property that can have it's value overridden explicitly.

Yeah pretty much, computed is readonly, linkedSignal can be rewritten but resets when underlying signals change. I'm not sure if we really have many use cases for it as most of our components tend to derive state from top-down templates and actions, but good to be aware of nonetheless.