Fix wording on patch-family upgrade pages (4.2.2.x, 4.3.1.x)#428
Fix wording on patch-family upgrade pages (4.2.2.x, 4.3.1.x)#428trikimiki wants to merge 4 commits into
Conversation
fix misleading patch upgrade (no script run) notes
vvlladd28
left a comment
There was a problem hiding this comment.
Review summary
Reviewed 7 changed files in Fix wording on patch-family upgrade pages (4.2.2.x, 4.3.1.x). Clean, minimal, and backward-compatible: patchableFrom is an optional override that falls back to baseVersion, and LATEST_PATCH_VERSIONS is derived so no per-entry flag needs maintaining. The page-template heading condition was also brought in line with the component condition (v.patch && v.baseVersion !== v.displayVersion && …), which incidentally fixes a prior TOC/heading mismatch for patch entries where baseVersion === displayVersion. Anchors are unaffected since headings render with an explicit id={v.anchor}. Left 1 minor comment inline.
Additional findings
These observations are about existing code outside the PR's diff — spotted while reading surrounding context.
- src/components/upgrade-instructions/LinuxUpgradeSteps.astro:64 (and the identical line in
DockerComposeUpgradeSteps.astro,DockerUpgradeSteps.astro,WindowsUpgradeSteps.astro) —const prevLabel = v.x ? v.upgradableFrom : v.upgradableFrom;is a no-op ternary: both branches return the same value, sov.xis never actually consulted. It can be simplified toconst prevLabel = v.upgradableFrom;, or the intended branch behavior restored if one side was meant to differ.
This is an auto-generated review. Findings may contain errors — please verify before applying changes.
| export const LATEST_PATCH_VERSIONS: Set<string> = (() => { | ||
| const seen = new Set<string>(); | ||
| const latest = new Set<string>(); | ||
| for (const v of UPGRADE_VERSIONS) { |
There was a problem hiding this comment.
This silently depends on UPGRADE_VERSIONS being ordered newest-patch-first within each baseVersion group — the first entry seen for a given baseVersion is taken as "latest." That invariant holds today (and UPGRADE_FAMILIES above relies on the same ordering), but nothing enforces it: if a future edit inserts an older patch above a newer one for the same base, the wrong version silently gets the "(latest X patch)" label with no error. Worth a one-line comment noting the newest-first requirement, or deriving "latest" via an actual version comparison rather than positional order.
vvlladd28
left a comment
There was a problem hiding this comment.
Follow-up review
Two deeper findings after cross-checking against the documented release policy (src/content/_includes/docs/releases/release-policy.mdx). The first is a possible correctness issue in the upgrade guidance itself; the second is a concrete fix for the ordering fragility raised earlier.
This is an auto-generated review. Findings may contain errors — please verify before applying changes.
| baseVersion: '4.3.1', | ||
| releaseDate: 'May 28 2026', | ||
| upgradableFrom: '4.2.1.x', | ||
| patchableFrom: '4.3.x', |
There was a problem hiding this comment.
Worth confirming this against the release policy before it ships. Per src/content/_includes/docs/releases/release-policy.mdx, the scheme is MAJOR.MINOR.MAINTENANCE.PATCH and the "no upgrade script" guarantee is PATCH-level only — i.e. within the same MAINTENANCE number. Here baseVersion is exactly the MAINTENANCE level, so 4.3.0 and 4.3.1 are two distinct maintenance releases.
Setting patchableFrom: '4.3.x' broadens the caution note ("if you are upgrading from version 4.3.x, DO NOT run the upgrade script") to cover users on 4.3.0.x moving to 4.3.1.x — which crosses a maintenance boundary. The policy says maintenance bumps "may require upgrade scripts" and should be done in a maintenance window (its example: 4.2.0.x → 4.2.1.0). The original 4.3.1.x scope (the baseVersion fallback) matched the policy exactly.
Caveat that cuts the other way: 4.3.1 is the Angular-20 maintenance release (release notes #14944), i.e. the 4.3 analog of 4.2.2.0, which the policy explicitly calls out as "no DB scripts required." So the broadening might be intentional and correct. But then it's inconsistent — this same PR leaves 4.2.2.x at 4.2.2.x scope (no patchableFrom). Either both Angular-maintenance lines should be broadened, or neither.
Could you confirm whether upgrade.sh has an incremental 4.3.0 → 4.3.1 step, or only the step from the 4.2.1.x LTS base? If the former, this should stay 4.3.1.x. (Same patchableFrom: '4.3.x' is set on all three 4.3.1.x entries — lines 52, 69, 87.)
There was a problem hiding this comment.
Could you confirm whether upgrade.sh has an incremental 4.3.0 → 4.3.1 step, or only the step from the 4.2.1.x LTS base? If the former, this should stay 4.3.1.x. (Same patchableFrom: '4.3.x' is set on all three 4.3.1.x entries — lines 52, 69, 87.)
yep, upgrade script is not needed, and that was actually the main focus of these changes.
current upgrade notes say "do not run starting from 4.3.1.x" - user upgrading from 4.3.0.x may misinterpret that script is required, run upgrade.sh, see "cannot upgrade this version" and wonder how to proceed.
actually - there were real customer portal tickets with questions on such confusions.
Caveat that cuts the other way: 4.3.1 is the Angular-20 maintenance release (release notes #14944), i.e. the 4.3 analog of 4.2.2.0, which the policy explicitly calls out as "no DB scripts required."
...
But then it's inconsistent — this same PR leaves 4.2.2.x at 4.2.2.x scope (no patchableFrom). Either both Angular-maintenance lines should be broadened, or neither.
a) separate Angular note exists, warning the user about this breaking change - also, 'patchableFrom' is strictly about "should you run upgrade.sh or not"
b) 4.2.x upgrades were consciously untouched since current cautions/notes are valid; additionally, these changes to upgrade pages were done with defaults and fallbacks to ensure they work in the same way as before in case 'patchableFrom' wasnt provided
| export const LATEST_PATCH_VERSIONS: Set<string> = (() => { | ||
| const seen = new Set<string>(); | ||
| const latest = new Set<string>(); | ||
| for (const v of UPGRADE_VERSIONS) { |
There was a problem hiding this comment.
Concrete suggestion for the ordering dependency flagged earlier — derive "latest" by comparing version components rather than relying on array position, so an out-of-order insert can't silently mislabel the newest patch:
function cmpVersion(a: string, b: string): number {
const pa = a.split('.').map(Number);
const pb = b.split('.').map(Number);
for (let i = 0; i < Math.max(pa.length, pb.length); i++) {
const d = (pa[i] ?? 0) - (pb[i] ?? 0);
if (d) return d;
}
return 0;
}
export const LATEST_PATCH_VERSIONS: Set<string> = (() => {
const byBase = new Map<string, string>(); // baseVersion → newest version
for (const v of UPGRADE_VERSIONS) {
if (!v.patch || !v.baseVersion) continue;
const cur = byBase.get(v.baseVersion);
if (!cur || cmpVersion(v.version, cur) > 0) byBase.set(v.baseVersion, v.version);
}
return new Set(byBase.values());
})();If you'd rather keep the positional approach, at least add a one-line comment stating the "newest patch listed first within each baseVersion" invariant, since nothing currently enforces it.
There was a problem hiding this comment.
imho, the concern is overblown: the upgrade instructions should be already ordered, since adding out-of-order version on top of the file is not something that could happen (new release = version number increments) or should happen (you logically never "upgrade" to older version, and the document is also always logically read bottom-to-top).
apart from that the "silent error" is factually incorrect: even if an out-of-order entry did slip in, UPGRADE_FAMILIES dedups by position, while table components render UPGRADE_VERSIONS in array order - so misorder would surface directly in the rendered table and get caught immediately, not silently.
i had Claude trace the consumers to confirm - the newest-first order is already relied on by UPGRADE_FAMILIES and every table/step component; so ordering is already load-bearing across all the UPGRADE_VERSIONS consumers, not just this Set.
since that is such a corner case and ordering - although already logical - is not enforced, i think adding comment will suffice - will commit it asap
… ordering invariant
Description
Summary
a2373cdcb77510b24a4de27b744e1f394e623f7d:patchableFromfield onUpgradeVersion, set on the three 4.3.1.x entries. Templates fall back tobaseVersionwhen unset, so every other upgrade page renders byte-identical to before.112b33c207786ee4d3927bc160d1e6848c1acd5b:LATEST_PATCH_VERSIONSset in the model (no per-entry flag to maintain).Type of change
src/content/docs/**)src/content/_includes/**)src/components/**,src/styles/**)src/pages/**,src/data/**)src/data/redirects.ts)releaseskill)Upgrade-instruction page templates (src/pages/docs/.../upgrade-instructions/[platform]/[familySlug].astro, CE + PE) and the upgrade-version data model (src/models/upgrade-instructions.ts).
Affected products
PE + CE upgrade docs
Related issues
Checklist
pnpm checkpasses (Astro / TypeScript)pnpm lint:eslintpassespnpm lint:slugcheckpasses (required if pages were added/renamed/moved across languages)pnpm lint:linkcheckpasses locally — required to merge; run it before requesting review (usepnpm lint:linkcheck:nobuildif you already ran a build)src/data/redirects.ts, andpnpm generate:redirectswas runsrc/data/versions.ts