chore: neutral simplification markers and #5707 review follow-ups - #5712
Merged
norman-abramovitz merged 3 commits intoJul 29, 2026
Conversation
Six comments flagged intentional shortcuts or do-not-refactor notes behind an ad-hoc prefix rather than the marker already used elsewhere in the tree. The notes themselves are worth keeping — the snackbar one guards against a theming sweep flipping an inverse overlay to a light-on-light surface — so rename the prefix rather than drop the comments. Four become `simplification:`, matching the marker already in scripts/lint-e2e-data-test.mjs. Two become `NOTE:`, since they are not simplifications at all: the recursion guard for self-referential $ref schemas, and the snackbar inverse-overlay warning. Comment text only; no behaviour change.
Three loose ends raised in review that merged before they were picked up. No behaviour change. - onNext coalesced the endpoint guid at the save site with `?? ''`. SourceType.endpointGuid is optional, so that operator was carrying the type rather than ever changing a value: the public branch is already guarded non-empty and the other is a literal ''. Narrow at the declaration instead and drop it from the object literal. - The two setGitMode specs asserted with toHaveBeenCalledWith, which matches any call, while their comment described the last one. Use toHaveBeenLastCalledWith so the assertion states what the comment claims. - The deferral comment in setActive said a synchronous pOnEnter would run "before the entering step is marked active", but the step is marked active a few lines above it. The actual constraint is that change detection has not yet re-rendered the content outlet for the new currentIndex.
The existing suite builds StepComponent instances by hand and assigns allSteps directly. That covers setActive's bookkeeping but never renders a stepper, so nothing in it can observe how projected step content is actually instantiated — which is how an incorrect claim about lazy instantiation sat in setActive's comment unchallenged. Add a host that mounts a real <app-steppers> with projected content and pin two behaviours: - A step that is never activated still has its content constructed AND change-detected on first render. Ivy does not defer <ng-content> inside an <ng-template>, so a step's directives and their ngOnChanges run up-front. The probe puts its directive behind nested @if blocks to mirror deploy-application-step2, where exactly this caused a regression. - onEnter still arrives only after the activating render, asserted through a rendered stepper rather than hand-built objects. Both were checked against mutations: disabling the probe's outer @if fails the first, and calling pOnEnter synchronously fails the second. Closes cloudfoundry#5710
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.
Three tidy-ups off the back of the #5707 review. No behaviour change; the only production-code edit is a type narrowing that removes a redundant operator.
1. Neutral markers for deliberate simplifications
Six comments flagged deliberate shortcuts or do-not-refactor notes behind an ad-hoc prefix instead of the marker already used elsewhere in the tree. This renames the prefix; the notes themselves are unchanged in substance and worth keeping — one of them is load-bearing.
plan-parameters-preview.util.ts:19simplification:$schema, names the upgrade pathschema-resolve.util.ts:168simplification:tailwind-dialog.service.ts:412simplification:cdkDrag, with the reasontools/stb/src/main.ts:132simplification:schema-resolve.util.ts:99NOTE:tailwind-snackbar.service.ts:207NOTE:simplification:is not a new convention —scripts/lint-e2e-data-test.mjs:68already uses it. The two that becameNOTE:were never simplifications: one is a correctness guard against infinite recursion on self-referential$refschemas, the other warns that the snackbar'sbg-gray-800/text-whiteare a deliberate inverse overlay kept raw so a theming sweep doesn't convert them tocontent-*tokens and produce light-on-light text.2. Follow-ups from the #5707 review
Three loose ends raised in review on #5707 that merged before they were picked up.
onNextcoalesced the endpoint guid at thesaveAppDetailscall with?? ''.SourceType.endpointGuidis optional, so that operator was carrying the type rather than ever changing a value — the public branch is already guarded non-empty above it and the other branch is a literal''. Narrowed at the declaration instead and dropped from the object literal.The two
setGitModespecs asserted withtoHaveBeenCalledWith, which matches any call, while their comment described the last one. Switched totoHaveBeenLastCalledWithso the assertion states what the comment claims — a strictly stronger check, still green.The deferral comment in
setActivesaid a synchronouspOnEnterwould run "before the entering step is marked active". The step is marked active a few lines above it. The real constraint is that change detection has not yet re-rendered the content outlet for the newcurrentIndex, so the callback would run against a view still showing the outgoing step. Reworded to say that.3. Real coverage for projected step content
Closes #5710.
The existing stepper suite builds
StepComponentinstances by hand and assignsallStepsdirectly. That coverssetActive's bookkeeping but never renders a stepper, so nothing in it can observe how projected step content is actually instantiated — which is how the incorrect claim in section 2 went unchallenged in the first place.Adds a host that mounts a real
<app-steppers>with projected content and pins two behaviours:<ng-content>inside an<ng-template>, so a step's directives and theirngOnChangesrun up-front. The probe places its directive behind nested@ifblocks to mirrordeploy-application-step2, where precisely this caused the regression fixed in fix(deploy): make GitHub/GitLab app deploy work (Source Config, private repos, token re-validate) #5707.onEnterstill arrives only after the activating render, asserted through a rendered stepper rather than hand-built objects.Both were checked against mutations rather than assumed meaningful: disabling the probe's outer
@iffails the first (while construction is still logged, so it distinguishes template execution from construction), and callingpOnEntersynchronously fails the second — identically to the pre-existing hand-built test.The older hand-built spec is left as-is; it remains a reasonable unit-level check of
setActive. The point is that it is no longer the only coverage.make check gategreen.