Skip to content

chore: neutral simplification markers and #5707 review follow-ups - #5711

Closed
nabramovitz wants to merge 1 commit into
cloudfoundry:developfrom
nabramovitz:norm/chore/neutral-code-markers
Closed

chore: neutral simplification markers and #5707 review follow-ups#5711
nabramovitz wants to merge 1 commit into
cloudfoundry:developfrom
nabramovitz:norm/chore/neutral-code-markers

Conversation

@nabramovitz

@nabramovitz nabramovitz commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Two unrelated tidy-ups, both comment- and type-level only, no behaviour change.

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.

File New marker Why
plan-parameters-preview.util.ts:19 simplification: strips only top-level $schema, names the upgrade path
schema-resolve.util.ts:168 simplification: empty strings treated as unset, deliberately scoped out
tailwind-dialog.service.ts:412 simplification: hand-rolled drag instead of cdkDrag, with the reason
tools/stb/src/main.ts:132 simplification: naming-convention detection, names the upgrade path
schema-resolve.util.ts:99 NOTE: recursion guard, not a shortcut
tailwind-snackbar.service.ts:207 NOTE: inverse-overlay warning, not a shortcut

simplification: is not a new convention — scripts/lint-e2e-data-test.mjs:68 already uses it. The two that became NOTE: were never simplifications: one is a correctness guard against infinite recursion on self-referential $ref schemas, the other warns that the snackbar's bg-gray-800/text-white are a deliberate inverse overlay kept raw so a theming sweep doesn't convert them to content-* 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.

onNext coalesced the endpoint guid at the saveAppDetails call 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 above it and the other branch is a literal ''. Narrowed at the declaration instead and dropped from the object literal.

The two setGitMode specs asserted with toHaveBeenCalledWith, which matches any call, while their comment described the last one. Switched to toHaveBeenLastCalledWith so the assertion states what the comment claims — a strictly stronger check, still green.

The deferral comment in setActive said a synchronous pOnEnter would 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 new currentIndex, so the callback would run against a view still showing the outgoing step. Reworded to say that.

Related: #5710 covers the missing test coverage that let the earlier version of that comment go unchallenged.


make check gate green.

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.
@nabramovitz nabramovitz changed the title chore: use neutral markers for deliberate simplifications chore: neutral simplification markers and #5707 review follow-ups Jul 29, 2026
@nabramovitz

Copy link
Copy Markdown
Contributor Author

Caution

Superseded — do not merge, reuse, or branch from this PR.

The branch behind this PR is stale and carries only half of the intended change. It has been replaced by #5712.

Closing in favour of #5712.

The single commit here (861369a0ed) was pushed before #5707 merged, so it sits on develop at 3476691ca2 and contains only the marker rename. The three follow-ups from the #5707 review — the endpoint-guid coalesce, the toHaveBeenLastCalledWith assertions, and the corrected setActive comment — touch files that exist only in their post-#5707 form, so they could not be added here without moving this branch onto a newer develop.

Rather than rewrite already-pushed history, the work was redone on a branch cut from current develop. The replacement was verified to match, line for line, what this branch would have become.

The branch norm/chore/markers-and-review-followups supersedes norm/chore/neutral-code-markers. The old branch is being left in place rather than deleted so this PR's commit stays reachable, but nothing should be built on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant