Tighten code comments in frame article and layout components (#63436)

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

Author: heiskr

src/frame/components/ClientSideHashFocus.tsx

@@ -1,10 +1,7 @@
 import { useEffect } from 'react'
 
-// When an in-page anchor link is clicked (e.g. href="#section-id"),
-// browsers scroll to the target but may not move keyboard focus to it.
-// This component listens for hash changes and programmatically focuses
-// the target element so screen reader and keyboard users land at the
-// correct position.
+// Hash links such as href="#section-id" can scroll without moving keyboard focus.
+// Focusing the target keeps screen reader and keyboard users at the anchor.
 export function ClientSideHashFocus() {
   useEffect(() => {
     const handleHashChange = () => {
@@ -17,8 +14,7 @@ export function ClientSideHashFocus() {
       }
     }
 
-    // Handle initial page load with a hash (e.g. direct link to
-    // docs.github.com/en/discussions#guides-2)
+    // Direct links such as /en/discussions#guides-2 need this before hashchange fires.
     handleHashChange()
 
     window.addEventListener('hashchange', handleHashChange)

src/frame/components/ClientSideRefresh.tsx

@@ -1,34 +1,20 @@
 import { useRouter } from 'next/router'
 import useSWR from 'swr'
 
-// This component is never mounted when you're in production mode.
-// Only when running in `NODE_ENV==='development'`.
-// It will reload the content every time the current page is focussed
-// (from being not focussed).
+// NODE_ENV === 'development' mounts this to reload the page when the tab regains focus.
+// SWR focus revalidation follows the Page Visibility API:
+// https://developer.mozilla.org/en-US/docs/Web/API/Page_Visibility_API
 export default function ClientSideRefresh() {
   const router = useRouter()
 
   useSWR(
     router.asPath,
     () => {
-      // Remember, in NextJS, the `router.locale` is never including the
-      // `router.asPath`. So we have to make sure it's always there
-      // otherwise, after this hook runs, you lose that `/en` prefix
-      // in the URL on the address bar.
+      // Prefix router.asPath with router.locale so refreshes keep the localized URL.
       router.replace(`/${router.locale}${router.asPath}`, undefined, { scroll: false })
     },
     {
-      // Implied here is that `revalidateOnFocus: true` which the default
-      // and it means that the `useSWR` hook will make a listener on the
-      // the Page Visibility API.
-      // https://developer.mozilla.org/en-US/docs/Web/API/Page_Visibility_API
-      // It effectively means that the callback of this hook will run every
-      // time the browser window is put back to being visible.
-
-      // The `revalidateOnMount` is crucial because it means that we don't
-      // bother executing the hook callback when it was first mounted
-      // because, naturally, the first time you mount it, it will not
-      // need to refresh because it's as fresh as it gets already.
+      // Skip mount revalidation because initial content is current.
       revalidateOnMount: false,
     },
   )

src/frame/components/CodeTabsGroup.tsx

@@ -24,18 +24,12 @@ import { sendEvent } from '@/events/components/events'
 import { EventType } from '@/events/types'
 import { useTranslation } from '@/languages/components/useTranslation'
 
-// React-native replacement for the imperative CodeTabs enhancer (#6619). The old
-// component scanned `#article-contents` for `.ghd-codetabs`, inserted a foreign
-// `.ghd-codetabs-nav` mountPoint as the container's first child, portaled a nav
-// into it, and toggled panel attributes. Those mutations are destructive surgery
-// on React-owned nodes and break on client-side navigation teardown. Instead, the
-// article body hast maps each `.ghd-codetabs` container to <CodeTabsGroup>, which
-// reads its `.ghd-codetab` panel children straight from props and renders the nav
-// and panels itself. No DOM scanning, no portal, no foreign nodes.
-//
-// The selected language lives in CodeLanguageContext so multiple code-tab groups
-// on one page stay in sync and share the language cookie, matching the previous
-// single-component behavior.
+// CodeTabsGroup avoids DOM scanning, portals, and panel-attribute mutations so
+// React owns the code-tab nodes through client-side navigation.
+// The article body hast maps each .ghd-codetabs container to CodeTabsGroup, which
+// reads .ghd-codetab panel children from props and renders the nav and panels.
+// CodeLanguageContext keeps code-tab groups on a page in sync and shares the
+// language cookie.
 
 type CodeLanguageContextT = {
   language: string
@@ -47,10 +41,9 @@ const CodeLanguageContext = createContext<CodeLanguageContextT>({
   setLanguage: () => {},
 })
 
+// CodeTabsProvider starts empty so SSR and first hydration select each group's first tab.
+// It applies the cookie preference after hydration, avoiding an SSR/client mismatch.
 export function CodeTabsProvider({ children }: { children: ReactNode }) {
-  // Start empty so server + first client render select each group's first tab
-  // (deterministic, hydration-safe). The cookie preference is applied after
-  // hydration, the same moment the old imperative enhancer used to run.
   const [language, setLanguageState] = useState('')
 
   useEffect(() => {
@@ -104,8 +97,7 @@ export function CodeTabsGroup({ className, children, ...rest }: CodeTabsGroupPro
   const { language, setLanguage } = useContext(CodeLanguageContext)
   const baseId = useId()
 
-  // Pull the `.ghd-codetab` panel children straight from the converted hast. Fail
-  // open (render the original markup) if the expected metadata isn't present.
+  // Fail open to original markup if converted hast lacks code-tab metadata.
   const tabs: PanelTab[] = Children.toArray(children)
     .filter((child): child is ReactElement<{ className?: string }> => isValidElement(child))
     .filter((child) => hasClass(child.props.className, 'ghd-codetab'))

src/frame/components/CopyButton.tsx

@@ -6,15 +6,13 @@ type CopyButtonProps = ComponentPropsWithoutRef<'button'> & {
   'data-clipboard'?: string
 }
 
-// React replacement for the imperative `copy-code.ts` enhancer. The code-block
-// header (`content-render/unified/code-header.ts`) emits this button into the
-// HTML AST next to a hidden `<pre data-clipboard="<id>">` holding the raw code.
-// When the article body is rendered from hast (instead of dangerouslySetInnerHTML),
-// `MarkdownContent` maps that `<button class="js-btn-copy">` to this component so
-// React owns the node rather than a post-hydration `document.querySelectorAll`.
-//
-// Analytics is intentionally NOT sent here: a global delegated click listener in
-// `events/components/events.ts` already records `.js-btn-copy` clicks.
+// React replacement for the imperative copy-code.ts enhancer. The code-block
+// header in content-render/unified/code-header.ts emits this button into the HTML
+// AST next to a hidden pre[data-clipboard] element that holds the raw code.
+// MarkdownContent maps button.js-btn-copy to CopyButton so React owns the node
+// instead of a post-hydration document.querySelectorAll pass.
+// Do not send analytics here: events/components/events.ts already records
+// .js-btn-copy clicks through its delegated click listener.
 export function CopyButton({ className, children, ...props }: CopyButtonProps) {
   const [copied, setCopied] = useState(false)
   const buttonRef = useRef<HTMLButtonElement>(null)
@@ -31,9 +29,7 @@ export function CopyButton({ className, children, ...props }: CopyButtonProps) {
   const handleClick = useCallback(async () => {
     if (!clipboardId) return
 
-    // The hidden <pre> is a sibling of this button inside the code-block header,
-    // so look it up locally to avoid copying a different block that happens to
-    // share the same content hash.
+    // Look up the sibling hidden pre locally so a reused content hash cannot copy another block.
     const scope: Element | Document = buttonRef.current?.parentElement ?? document
     const pre = scope.querySelector<HTMLElement>(`pre[data-clipboard="${CSS.escape(clipboardId)}"]`)
     const text = pre?.innerText
@@ -42,8 +38,7 @@ export function CopyButton({ className, children, ...props }: CopyButtonProps) {
     try {
       await navigator.clipboard.writeText(text)
     } catch {
-      // Clipboard write can be blocked (permissions, insecure context, etc.).
-      // Don't show a false "Copied!" state.
+      // A blocked clipboard write must not show a false Copied state.
       return
     }
 

src/frame/components/DefaultLayout.module.scss

@@ -5,32 +5,26 @@
 
 .mainContent {
   scroll-margin-top: 5rem;
-  // Keep anything that overflows the main column from scrolling the page
-  // sideways. This backstopped the article section rules while they overshot
-  // ±1500px to reach the rails; those now span the body column and no further,
-  // so it is only a guard for wide content (tables, code) today. `clip` (not
-  // hidden/auto) creates no scroll container, so the sticky rails and secondary
-  // bar are unaffected.
+  // Clip wide content such as tables and code without creating a scroll container.
+  // Sticky rails and the secondary bar still resolve against the viewport.
   overflow-x: clip;
 }
 
-// The search results page splits into rail + results at brand's `medium`
-// breakpoint rather than waiting for `d-lg-flex` at 1012px, so the facet rail is
-// available on tablets. Applied only on /search.
+// Search results split into facet rail and results at Brand's medium breakpoint,
+// before d-lg-flex at 1012px, so filters are available on tablets. Applied only
+// on search results pages, versioned or not.
 .searchColumns {
   @media (min-width: $brand-breakpoint-medium) {
     display: flex;
   }
 }
 
-// Sticky descendants share the header/bar offset; the sub-bar adds 40px.
-// These modifiers follow the same visibility state as OverviewSubBar.
+// Sticky descendants share the header and secondary-bar offset.
+// The sub-bar adds 40px, matching OverviewSubBar visibility.
 //
-// The header half of the stack comes from --docs-header-height, not a literal
-// 65px: the Brand header is 3.5rem below 48rem and 4rem above, so a fixed px
-// stack would be wrong at one of the two sizes. The 45px is the secondary bar,
-// which is a fixed height; keep it in step with DocsSecondaryBar.module.scss
-// and SidebarNav.module.scss, which compose the same two parts.
+// The header height comes from --docs-header-height because the Brand header is
+// 3.5rem below 48rem and 4rem above. Keep the fixed 45px secondary-bar height
+// in step with DocsSecondaryBar.module.scss and SidebarNav.module.scss.
 .stickyStack {
   --docs-sticky-stack: calc(var(--docs-header-height) + 45px);
 }
@@ -62,11 +56,8 @@
   }
 }
 
-// The a11y skip link is a filled accent chip. Was primer/css
-// `color-bg-accent-emphasis color-fg-on-emphasis`, which paints the chip from
-// the PRC palette on a Brand surface. The Brand accent fill inverts between
-// modes (dark green in light, light green in dark), so onEmphasis text is
-// correct here.
+// The skip link uses the Brand accent fill so its onEmphasis text inverts with
+// the page's light and dark Brand surfaces.
 .skipButton {
   background-color: var(--brand-color-accent-primary);
   color: var(--brand-color-text-onEmphasis);

src/frame/components/DefaultLayout.tsx

@@ -28,11 +28,12 @@ const MINIMAL_RENDER = Boolean(JSON.parse(process.env.MINIMAL_RENDER || 'false')
 
 type Props = {
   children?: React.ReactNode
-  // Whether this page renders the right-rail "In this article" drawer (article +
-  // automated pages do; REST reference pages do not). Controls whether the
-  // secondary bar's collapsed Overview menu yields to the drawer at xxl.
+  // Article and automated pages render the right-rail drawer; REST pages do not.
+  // The secondary bar's collapsed Overview menu yields to that drawer at xxl.
   hasDrawer?: boolean
 }
+// The non-homepage branch wraps the secondary bar and article content in SelectionProvider
+// so the collapsed Overview menu and article body share platform/tool selection.
 export const DefaultLayout = (props: Props) => {
   const mainContext = useMainContext()
   const {
@@ -52,17 +53,14 @@ export const DefaultLayout = (props: Props) => {
   const { languages } = useLanguages()
   const [isNarrowMenuOpen, setIsNarrowMenuOpen] = useState(false)
 
-  // This is only true when we do search indexing which renders every page
-  // just to be able to `cheerio` load the main body (and the meta
-  // keywords tag).
+  // Search indexing renders every page so Cheerio can read the body and meta keywords.
   if (MINIMAL_RENDER) {
     return (
       <div>
         <Head>
           <title>{page.fullTitle}</title>
         </Head>
 
-        {/* For local site search indexing */}
         <div className="d-none d-xl-block" data-search="breadcrumbs">
           <Breadcrumbs />
         </div>
@@ -143,7 +141,6 @@ export const DefaultLayout = (props: Props) => {
           <title>{page.fullTitle}</title>
         ) : null}
 
-        {/* For Google and Bots */}
         <meta name="description" content={metaDescription} />
         {page.hidden && <meta name="robots" content="noindex" />}
         {Object.values(languages)
@@ -161,7 +158,6 @@ export const DefaultLayout = (props: Props) => {
             )
           })}
 
-        {/* For analytics events */}
         {router.locale && <meta name="path-language" content={router.locale} />}
         {currentVersion && <meta name="path-version" content={currentVersion} />}
         {currentProduct && <meta name="path-product" content={currentProduct.id} />}
@@ -178,7 +174,6 @@ export const DefaultLayout = (props: Props) => {
         )}
         {status && <meta name="status" content={status.toString()} />}
 
-        {/* OpenGraph data */}
         {page.fullTitle && (
           <>
             <meta property="og:site_name" content="GitHub Docs" />
@@ -188,15 +183,13 @@ export const DefaultLayout = (props: Props) => {
             <meta property="og:image" content={getSocialCardImage()} />
           </>
         )}
-        {/* Twitter Meta Tags */}
         <meta name="twitter:card" content="summary" />
         <meta property="twitter:domain" content={new URL(fullUrl).hostname} />
         <meta property="twitter:url" content={fullUrl} />
         <meta name="twitter:title" content={page.fullTitle} />
         {page.introPlainText && <meta name="twitter:description" content={page.introPlainText} />}
         <meta name="twitter:image" content={getSocialCardImage()} />
 
-        {/* LLM-friendly alternate formats */}
         <link
           rel="alternate"
           type="text/markdown"
@@ -220,7 +213,6 @@ export const DefaultLayout = (props: Props) => {
         />
       </Head>
 
-      {/* a11y */}
       <a
         href="#main-content"
         className={cx('visually-hidden skip-button', styles.skipButton)}
@@ -246,10 +238,6 @@ export const DefaultLayout = (props: Props) => {
               </div>
             </div>
           ) : (
-            // SelectionProvider wraps both the secondary bar and the content so the
-            // bar's collapsed "In this article" menu (OverviewMenu) sees the same
-            // platform/tool selection as the article body and filters its headings
-            // accordingly.
             <SelectionProvider>
               <ActiveSectionProvider>
                 <DocsSecondaryBar />
@@ -263,52 +251,45 @@ export const DefaultLayout = (props: Props) => {
   )
 }
 
-// The doc-tree rail + content column, split out so it can read the collapse
-// context that DefaultLayout provides. On desktop the rail shows unless
-// collapsed; on mobile it shows inline (in the page flow, like desktop) only
-// when the nav is opened from the secondary bar. The content column (flex-1)
-// fills the row when the rail is absent.
+// LayoutBody reads SidebarCollapseContext after DefaultLayout provides it.
+// On mobile, the inline rail shows only when opened from the secondary bar.
+// LayoutBody matches SidebarNav's search-page gate instead of router.route
+// because src/pages/search.tsx and src/pages/[versionId]/search.tsx must agree
+// on the facet rail.
+// It mirrors OverviewSubBar's render gate so sticky-stack classes describe a bar
+// that renders.
+// Search results split the facet rail beside results at Brand's medium
+// breakpoint; route gating keeps other pages on d-lg-flex at 1012px.
+// The desktop rail-collapse cookie does not hide the open mobile nav. Otherwise
+// the content column hides with no drawer visible and shows a blank area instead
+// of the doc tree.
+// Search ignores the collapse cookie because it has no DocsSecondaryBar toggle
+// to restore filters.
+// Sticky elements need an explicit height because their scroll container sets
+// overflow:auto.
+// The sticky-stack class publishes the header and bar offset for descendants
+// such as article table headers.
+// Keeping OverviewSubBar inside main lets it start at the doc-tree drawer's
+// right edge and share that band with the drawer; mainContent uses overflow-x:clip,
+// so sticky still resolves against the viewport.
 type LayoutBodyProps = {
   children?: React.ReactNode
   hasDrawer?: boolean
 }
 const LayoutBody = ({ children, hasDrawer }: LayoutBodyProps) => {
   const { collapsed, mobileNavOpen } = useSidebarCollapsed()
   const { currentProduct } = useMainContext()
-  // Matches SidebarNav's own gate rather than testing router.route. There are two search
-  // pages, src/pages/search.tsx and src/pages/[versionId]/search.tsx, so a route test
-  // for '/search' misses every versioned search URL, and this check would then disagree
-  // with SidebarNav about whether the rail is a facet rail.
   const isSearchResultsPage = currentProduct?.id === 'search'
-  // Mirrors OverviewSubBar's own render gate (it returns null at <= 1 item), so
-  // the sticky-stack classes below describe the bar that actually renders.
   const miniTocItems = useMiniTocItems()
   const hasSubBar = miniTocItems.length > 1
   return (
-    // `d-lg-flex` only goes side-by-side at 1012px. The search page's facet rail
-    // is meant to sit beside the results from brand's `medium` breakpoint, so it
-    // gets an earlier split of its own. Route-gated, so no other page moves.
     <div className={cx('d-lg-flex', isSearchResultsPage && styles.searchColumns)}>
-      {/* `collapsed` is the desktop rail-collapse state (persisted). The inline
-        mobile nav is independent, so still render the sidebar when it's open.
-        Otherwise opening the mobile nav while the desktop rail is collapsed
-        hides the content column (contentHiddenForNav) with no drawer to show,
-        so the open nav displays a blank area instead of the doc tree.
-
-        Search is exempt: the cookie is shared with the doc-tree rail, but the
-        search page has no toggle to undo it (DocsSecondaryBar returns null
-        there), so honouring it would strand the filters with no way back. */}
       {collapsed && !mobileNavOpen && !isSearchResultsPage ? null : (
         <SidebarNav mobileOpen={mobileNavOpen} />
       )}
-      {/* Need to set an explicit height for sticky elements since we also
-        set overflow to auto */}
       <div
         className={cx(
           'flex-column flex-1 min-width-0',
-          // Publish the sticky-stack height to everything in the column (article
-          // table headers read it). Driven by the same values as OverviewSubBar's
-          // visibility modifier just below, so the offset and the bar agree.
           styles.stickyStack,
           hasSubBar && styles.stickyStackWithSubBar,
           hasSubBar &&
@@ -318,14 +299,6 @@ const LayoutBody = ({ children, hasDrawer }: LayoutBodyProps) => {
         )}
       >
         <main id="main-content" className={styles.mainContent}>
-          {/* Inside <main>, not before it: as a preceding sibling the "Skip to
-              main content" link jumped the reader straight past the page's only
-              in-article navigation. Still within the content column, so on
-              desktop it starts at the doc-tree drawer's right edge and runs to
-              the screen edge, sharing that band with the drawer rather than
-              cutting across above it. (.mainContent uses `overflow-x: clip`,
-              which creates no scroll container, so sticky still resolves against
-              the viewport.) */}
           <OverviewSubBar hasDrawer={hasDrawer} />
           <DeprecationBanner />
           <RestBanner />