Tighten code comments in src/versions (#63462)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 7c52b57f-2c29-4aa1-8233-99d0d6e13571

Author: heiskr

src/versions/components/DeprecationBanner.tsx

@@ -16,10 +16,7 @@ export const DeprecationBanner = () => {
     return null
   }
 
-  // Have to "trick" TypeScript here because by default, this is an
-  // optional key. But because we're confident with the JS business
-  // logic in MainContext.tsx, we can safely assume that this key
-  // is present.
+  // MainContext supplies enterprise_deprecation before React renders this banner.
   const enterpriseDeprecation = data.reusables.enterprise_deprecation as EnterpriseDeprecation
   const message = enterpriseServerReleases.isOldestReleaseDeprecated
     ? enterpriseDeprecation.version_was_deprecated

src/versions/components/VersionPicker.module.scss

@@ -1,8 +1,5 @@
-/*
- * The header variant's styling is shared with the language picker and lives in
- * @/frame/components/page-header/HeaderPicker.module.scss. Only the default
- * (non-header) variant is styled here.
- */
+// The header variant shares HeaderPicker.module.scss with the language picker.
+// This file styles the default variant.
 
 .itemsWidth {
   width: 14rem;

src/versions/components/VersionPicker.tsx

@@ -14,8 +14,8 @@ import { DEFAULT_VERSION, useVersion } from '@/versions/components/useVersion'
 import { useTranslation } from '@/languages/components/useTranslation'
 
 import styles from './VersionPicker.module.scss'
-// The header variant's trigger, menu surface and rows are shared with the language
-// picker so the two dropdowns cannot drift apart.
+// The header variant shares HeaderPicker.module.scss with the language picker,
+// so the two dropdowns stay in sync.
 import headerStyles from '@/frame/components/page-header/HeaderPicker.module.scss'
 
 type Props = {
@@ -27,8 +27,8 @@ type VersionPickerLink = {
   text: string
   selected: boolean
   href: string
-  // Brand's ActionMenu identifies the chosen row by string value, so every row needs
-  // one. Versions use their own version name; the two extra rows use sentinels.
+  // Brand's ActionMenu identifies rows by string value. Versions use their version
+  // name; extra rows use sentinels.
   value: string
   extra: {
     arrow: boolean
@@ -41,82 +41,69 @@ type VersionPickerLink = {
 const ALL_RELEASES_VALUE = 'all-enterprise-releases'
 const ABOUT_VERSIONS_VALUE = 'about-versions'
 
-// Brand clones ActionMenu.Button with its own ref, so the trigger cannot be reached
-// through a React ref. A stable test id keeps both the Escape handler and the tests
-// off Brand's hashed CSS class names.
+// Brand clones ActionMenu.Button with its own ref, so React refs cannot reach the
+// trigger. A stable test id keeps the Escape handler and tests off hashed CSS classes.
 const HEADER_TRIGGER_TESTID = 'version-picker-button'
 
 type PlanMenuItemProps = {
   item: VersionPickerLink
-  // Injected by ActionMenu.Overlay, which clones each of its direct children with the
-  // select handler and the selection type derived from `selectionVariant`.
+  // ActionMenu.Overlay injects handler and type into each direct child.
   handler?: (value: string) => void
   type?: 'none' | 'single' | 'link'
 }
 
+// Extra rows opt out of Brand selection semantics because axe rejects aria-checked
+// on menuitem, and Brand derives both role and aria-checked from type.
 const PlanMenuItem = ({ item, handler, type }: PlanMenuItemProps) => {
   const isExtra = Boolean(item.extra.arrow || item.extra.info)
 
   return (
     <BrandActionMenu.Item
       handler={handler}
-      // Brand derives both `role` and `aria-checked` from `type`. The two extra rows
-      // navigate elsewhere instead of choosing a version, so under the injected
-      // 'single' they would render role="menuitem" *plus* aria-checked — which axe
-      // rejects, since aria-checked is not an allowed attribute on menuitem. Overlay
-      // injects `type` into its direct children only, so this wrapper is the seam
-      // where a single row can opt out of selection semantics.
       type={isExtra ? 'none' : type}
       value={item.value}
       selected={item.selected}
       className={cx(
         headerStyles.headerMenuItem,
         item.selected && headerStyles.headerMenuItemSelected,
       )}
-      // Only spread `role` for the extras: passing `role={undefined}` would override
-      // the role Brand computes and leave the version rows with no role at all.
+      // Only spread role for extras; role undefined overrides Brand's computed role.
       {...(isExtra ? { role: 'menuitem' } : {})}
     >
       <span data-testid="version-picker-item" className={headerStyles.headerMenuItemLabel}>
         {item.text}
         {item.extra.arrow && <ArrowRightIcon verticalAlign="middle" size={15} className="ml-1" />}
         {item.extra.info && <InfoIcon verticalAlign="middle" size={15} className="ml-1" />}
       </span>
-      {/* The design marks the current plan with a trailing green dot instead of
-          Brand's leading check icon, which the stylesheet hides. */}
+      {/* HeaderPicker.module.scss hides Brand's leading check icon; design uses a trailing green dot. */}
       {item.selected && <DotFillIcon size={16} className={headerStyles.headerMenuItemDot} />}
     </BrandActionMenu.Item>
   )
 }
 
-// The rule between the version rows and the two navigation rows. Brand has no divider
-// child, and ActionMenu.Overlay clones every direct child with `handler` and `type`,
-// so this wrapper takes no props at all: the injected ones are swallowed here instead
-// of landing on the DOM node. The <li> carries no tabIndex and no `data-value`, so
-// Brand's focus zone and its Enter handler both skip it — and it is never the menu's
-// first or last <li>, which are the two rows Brand wires its arrow-key wrap-around to.
+// Brand lacks a divider child, and ActionMenu.Overlay injects handler and type into
+// every direct child. This wrapper swallows those props so they do not reach the li.
+// Without tabIndex or data-value, Brand's focus zone and Enter handler skip the
+// separator. The caller keeps it away from the first and last li, which Brand uses
+// for arrow-key wrap-around.
 const PlanMenuSeparator = () => <li role="separator" className={headerStyles.headerMenuSeparator} />
 
+// VersionPicker uses startsWith to identify Enterprise Server because VersionItem
+// omits hasNumberedReleases. The label says "version" for Enterprise Server because
+// versionTitle includes the numbered release; a "plan" label would make screen
+// readers announce "Select your plan: Enterprise Server 3.19".
 export const VersionPicker = ({ variant = 'default', onNavigate }: Props) => {
   const router = useRouter()
   const { currentVersion } = useVersion()
   const mainContext = useMainContext()
   const [open, setOpen] = useState(false)
   const pickerId = useId()
   const isHeader = variant === 'header'
-  // Use TypeScript's "not null assertion" because mainContext.page should
-  // be present in mainContext if it's gotten to the stage of React
-  // rendering.
+  // React rendering only starts after MainContext adds page.
   const page = mainContext.page!
   const { allVersions, enterpriseServerVersions } = mainContext
   const { t } = useTranslation(['pages', 'picker'])
 
-  // The same control chooses a plan on dotcom and Enterprise Cloud but a numbered
-  // release on Enterprise Server, where `versionTitle` is `${planTitle} ${release}`.
-  // A single "Select your plan:" would announce "Select your plan: Enterprise
-  // Server 3.19" to screen readers. Uses the same `startsWith` predicate as
-  // `hasEnterpriseVersions` below: `hasNumberedReleases` is set on the runtime
-  // version object but is not declared on the `VersionItem` type.
   const pickerLabel = currentVersion.startsWith('enterprise-server')
     ? t('version_picker_label')
     : t('plan_picker_label')
@@ -195,7 +182,7 @@ export const VersionPicker = ({ variant = 'default', onNavigate }: Props) => {
   const selectedOption = allLinks.find((item) => item.selected)
 
   const handleVersionSelect = (item: VersionPickerLink) => {
-    // Save the user's version preference when they actively select one
+    // Navigation rows leave the existing version preference alone.
     if (item.extra?.version) {
       try {
         Cookies.set(USER_VERSION_COOKIE_NAME, item.extra.version)
@@ -204,26 +191,20 @@ export const VersionPicker = ({ variant = 'default', onNavigate }: Props) => {
       }
     }
     setOpen(false)
-    // Navigate after setting cookie
+    // Set the cookie before navigation so the next page can read the preference.
     if (item.href) {
       onNavigate?.()
       router.push(item.href)
     }
   }
 
   if (isHeader) {
-    // The Figma dropdown node draws no divider, but the rule that separated the
-    // versions from the two navigation rows is kept from the @primer/react menu this
-    // replaced. The filter keeps it from ever becoming the menu's first or last row:
-    // Brand focuses the first <li> and binds its arrow-key wrap-around to the first
-    // and the last, and neither should land on a separator.
+    // Keep the separator from the default picker, but not where Brand focuses or wraps rows.
     const headerLinks = allLinks.filter(
       (item, index) => !item.divider || (index > 0 && index < allLinks.length - 1),
     )
 
-    // Brand reports the chosen row by value. Routing every row — the two extras
-    // included — back through handleVersionSelect keeps navigation client-side
-    // instead of letting the extras become anchors that reload the page.
+    // Route extra rows through handleVersionSelect so they stay client-side.
     const handleHeaderSelect = (value: string) => {
       const item = headerLinks.find((link) => link.value === value)
       if (item) {
@@ -239,15 +220,10 @@ export const VersionPicker = ({ variant = 'default', onNavigate }: Props) => {
       )
       if (trigger?.getAttribute('aria-expanded') !== 'true') return
 
-      // Brand's ActionMenu and SubdomainNavBar both listen for Escape on `document`
-      // and neither honours defaultPrevented, so a single Escape would close this
-      // picker *and* the surrounding narrow menu. Stopping the event here — while it
-      // is still in its capture phase, before it reaches either listener — leaves the
-      // outer menu open. Brand has no controlled `open` prop, so the picker is closed
-      // through its own trigger: focus it first so focus stays put, then click it to
-      // let ActionMenu toggle itself shut.
+      // Stop Escape in capture so SubdomainNavBar's document listener leaves the narrow menu open.
       event.preventDefault()
       event.stopPropagation()
+      // Brand has no controlled open prop, so click its focused trigger to close it.
       trigger.focus()
       trigger.click()
     }

src/versions/lib/all-versions.ts

@@ -2,9 +2,7 @@ import fs from 'fs'
 import type { AllVersions, Version } from '@/types'
 import enterpriseServerReleases from './enterprise-server-releases'
 
-// version = "plan"@"release"
-// example: enterprise-server@2.21
-// where "enterprise-server" is the plan and "2.21" is the release
+// Version keys combine plan and release, for example enterprise-server@2.21.
 const versionDelimiter = '@'
 const latestNonNumberedRelease = 'latest'
 const REST_DATA_META_FILE = 'src/rest/lib/config.json'
@@ -27,26 +25,23 @@ interface RestApiConfig {
   }
 }
 
-// !Explanation of versionless redirect fallbacks!
-// This array is **in order** of the versions the site should try to fall back to if
-// no version is provided in a URL. For example, if /foo refers to a page that is available
-// in all versions, we should not redirect it (because /foo is the correct FPT versioned URL).
-// But if /foo refers to a page that is only available in GHEC and GHES, we should redirect it
-// to /enterprise-cloud@latest/foo (since GHEC comes first in the hierarchy of version fallbacks).
-// The implementation lives in lib/redirects/permalinks.ts.
+// Versionless redirects try these plans in order. If /foo supports every plan, it
+// stays the Free, Pro, and Team URL. If it supports only Enterprise Cloud and
+// Enterprise Server, src/redirects/lib/permalinks.ts redirects it to
+// /enterprise-cloud@latest/foo.
 const plans: PlanConfig[] = [
   {
-    // free-pro-team is **not** a user-facing version and is stripped from URLs.
-    // See lib/remove-fpt-from-path.ts for details.
+    // free-pro-team is not user-facing.
+    // src/versions/lib/remove-fpt-from-path.ts strips it from URLs.
     plan: 'free-pro-team',
     planTitle: 'Free, Pro, & Team',
     shortName: 'fpt',
     releases: [latestNonNumberedRelease],
     latestRelease: latestNonNumberedRelease,
-    nonEnterpriseDefault: true, // permanent way to refer to this plan if the name changes
+    nonEnterpriseDefault: true, // Marks the non-enterprise default independently of the plan name.
     hasNumberedReleases: false,
-    openApiBaseName: 'fpt', // used for REST
-    miscBaseName: 'dotcom', // used for GraphQL and webhooks
+    openApiBaseName: 'fpt', // REST base name.
+    miscBaseName: 'dotcom', // Search index version map base name.
   },
   {
     plan: 'enterprise-cloud',
@@ -72,8 +67,6 @@ const plans: PlanConfig[] = [
 
 const allVersions: AllVersions = {}
 
-// combine the plans and releases to get allVersions object
-// e.g. free-pro-team@latest, enterprise-server@2.21, enterprise-server@2.20, etc.
 for (const planObj of plans) {
   for (const release of planObj.releases) {
     const version = `${planObj.plan}${versionDelimiter}${release}`
@@ -91,8 +84,10 @@ for (const planObj of plans) {
       miscVersionName: planObj.hasNumberedReleases
         ? `${planObj.miscBaseName}${release}`
         : planObj.miscBaseName,
-      apiVersions: [], // REST Calendar Date Versions, this may be empty for non calendar date versioned products
-      latestApiVersion: '', // Latest REST Calendar Date Version, this may be empty for non calendar date versioned products
+      // REST calendar date versions; empty for products without calendar date API versions.
+      apiVersions: [],
+      // Latest REST calendar date version; empty for products without calendar date API versions.
+      latestApiVersion: '',
       plan: planObj.plan,
       planTitle: planObj.planTitle,
       shortName: planObj.shortName,
@@ -108,15 +103,15 @@ for (const planObj of plans) {
   }
 }
 
-// Adds the calendar date (or api versions) to the allVersions object
+// REST config adds calendar date API versions after the version objects exist.
 const apiVersions: RestApiConfig['api-versions'] = JSON.parse(
   fs.readFileSync(REST_DATA_META_FILE, 'utf8'),
 )['api-versions']
 
 for (const key of Object.keys(apiVersions)) {
   const docsVersion = getDocsVersion(key)
   allVersions[docsVersion].apiVersions.push(...apiVersions[key].sort().reverse())
-  // Create a copy of the array to avoid mutating the original when using pop()
+  // Copy before pop so latestApiVersion does not remove a version from apiVersions.
   const sortedVersions = [...apiVersions[key].sort()]
   allVersions[docsVersion].latestApiVersion = sortedVersions.pop() || ''
 }
@@ -130,9 +125,7 @@ export function isApiVersioned(version: string): boolean {
   return allVersions[version] && allVersions[version].apiVersions.length > 0
 }
 
-// Currently the versions from the OpenAPI do not match the versions on Docs.
-// There is a mapping between the version names. This gets the Docs version from
-// the OpenAPI version name.
+// OpenAPI names do not match Docs version names, so this maps one to its Docs version.
 export function getDocsVersion(openApiVersion: string): string {
   const matchingVersion = Object.values(allVersions).find((version) =>
     openApiVersion.startsWith(version.openApiVersionName),

src/versions/lib/enterprise-server-releases.d.ts

@@ -1,11 +1,13 @@
 type Dates = {
   [key: string]: {
-    releaseDate: string // For backward compatibility - will be RC date initially, then GA date once available
+    // Templates read releaseDate as the display date: RC date until the GA date exists.
+    releaseDate: string
     deprecationDate: string
-    releaseCandidateDate?: string // Release Candidate date
-    generalAvailabilityDate?: string // General Availability date
-    displayCandidateDate?: string | null // Computed: RC date if in past, null if future
-    displayReleaseDate?: string | null // Computed: GA date if in past, null if future
+    releaseCandidateDate?: string
+    generalAvailabilityDate?: string
+    // Templates hide release dates until each date has passed.
+    displayCandidateDate?: string | null
+    displayReleaseDate?: string | null
   }
 }
 

src/versions/lib/enterprise-server-releases.ts

@@ -24,18 +24,18 @@ const rawDates: RawDatesData = JSON.parse(
   fs.readFileSync('src/ghes-releases/lib/enterprise-dates.json', 'utf8'),
 )
 
-// Upcoming GHES release numbers (used in frontmatter and release planning)
+// Frontmatter and release planning use the next two GHES release numbers.
 export const next = '3.23'
 export const nextNext = '3.24'
 
-// Currently supported GHES versions (in descending order, latest first)
+// Keep supported GHES versions in descending order, latest first.
 export const supported = ['3.22', '3.21', '3.20', '3.19', '3.18', '3.17']
 
-// Set to version number when in RC phase, null when no RC is active
+// Use the release number during an active RC; use null outside RC.
 export const releaseCandidate = null
 
-// Deprecated versions with functional redirect handling (3.0+)
-// When archiving a new version, add it here and update the archival process
+// Deprecated releases from 3.0 onward use functional redirects.
+// Add a newly archived release here and update the archival process.
 export const deprecatedWithFunctionalRedirects = [
   '3.16',
   '3.15',
@@ -56,7 +56,7 @@ export const deprecatedWithFunctionalRedirects = [
   '3.0',
 ]
 
-// All deprecated versions (combines functional + legacy redirect handling)
+// The deprecated list combines functional redirects with legacy redirect handling.
 export const deprecated = [
   ...deprecatedWithFunctionalRedirects,
   '2.22',
@@ -85,13 +85,13 @@ export const deprecated = [
   '11.10.340',
 ]
 
-// Versions with legacy asset handling (stored in separate repos before blob storage)
+// Legacy asset releases store assets in separate repos instead of blob storage.
 export const legacyAssetVersions = ['3.0', '2.22', '2.21']
 
 export const firstReleaseStoredInBlobStorage = '3.2'
 export const firstVersionDeprecatedOnNewSite = '2.13'
 export const lastVersionWithoutArchivedRedirectsFile = '2.17'
-export const lastReleaseWithLegacyFormat = '2.18' // Last to use /enterprise/<release>/... paths
+export const lastReleaseWithLegacyFormat = '2.18' // Last release with /enterprise/<release>/... paths.
 export const firstReleaseNote = '2.20'
 export const firstRestoredAdminGuides = '2.21'
 
@@ -101,7 +101,7 @@ export const latest = supported[0]
 export const latestStable = releaseCandidate ? supported[1] : latest
 export const oldestSupported = supported[supported.length - 1]
 
-// Enhanced dates object with computed display values for templates
+// Templates read these computed display dates to hide future release dates.
 export const dates: Record<string, EnhancedVersionDateData> = Object.fromEntries(
   Object.entries(rawDates).map(([version, versionData]) => [
     version,
@@ -118,8 +118,7 @@ export const isOldestReleaseDeprecated = nextDeprecationDate
   ? new Date() > new Date(nextDeprecationDate)
   : false
 
-// Find any other releases that may share the oldest deprecation date
-// We'll want to display the deprecation banner on all of these releases (not just oldest)
+// Show the deprecation banner on every release that shares the oldest deprecation date.
 export const releasesWithOldestDeprecationDate = Object.entries(dates)
   .filter(([, versionData]) => versionData.deprecationDate === nextDeprecationDate)
   .map(([version]) => version)
@@ -140,8 +139,8 @@ export const deprecatedReleasesOnDeveloperSite = deprecated.filter((version) =>
   versionSatisfiesRange(version, '<=2.16'),
 )
 
-// Returns the date only once it has passed, so we never advertise a future
-// release date. An unparseable date gives NaN, which also returns null.
+// Return a date only after it has passed, so templates never advertise future releases.
+// Unparseable dates produce NaN, which also returns null.
 function processDateForDisplay(date: string | undefined): string | null {
   if (!date) return null
   const currentTimestamp = Math.floor(Date.now() / 1000)