From 5ce93a012175c8544db9512dc8bbd078173e418a Mon Sep 17 00:00:00 2001 From: Claude Code Date: Wed, 11 Feb 2026 16:40:05 +0100 Subject: [PATCH 1/4] docs(review): add comprehensive code review report 59 findings across 8 dimensions (code quality, performance, accessibility, security, SEO, test coverage, UX, maintainability) with prioritized backlog. Co-Authored-By: Claude Signed-off-by: Ben Peter --- .../2026-02-11-comprehensive-code-review.md | 2192 +++++++++++++++++ 1 file changed, 2192 insertions(+) create mode 100644 docs/reviews/2026-02-11-comprehensive-code-review.md diff --git a/docs/reviews/2026-02-11-comprehensive-code-review.md b/docs/reviews/2026-02-11-comprehensive-code-review.md new file mode 100644 index 0000000..ab60b7b --- /dev/null +++ b/docs/reviews/2026-02-11-comprehensive-code-review.md @@ -0,0 +1,2192 @@ +# Comprehensive Code Review: schamdan.de + +| Attribute | Value | +|-----------------|--------------------------------------------| +| Review Date | 2026-02-11 | +| Codebase Snapshot | `929bb9120b1d7e2f1313e3f53d16db4babdb042b` | + +--- + +## Executive Summary + +### Finding Summary + +| Severity | Count | +|----------|-------| +| Critical | 3 | +| High | 12 | +| Medium | 19 | +| Low | 25 | +| **Total** | **59** | + +4 additional informational/positive findings (SEC-03, SEC-05, SEC-06, SEO-07) are documented in their category sections but excluded from the backlog. + +### Top 3 Priority Areas + +1. **Accessibility -- header navigation (A11Y-01, A11Y-02, A11Y-04, A11Y-05)**: All three Critical findings are accessibility issues in the header. The hamburger menu and dropdown sections are invisible to screen readers and inoperable via keyboard, violating WCAG Level A requirements. These are quick fixes (S effort) that unblock basic assistive technology access. + +2. **Speisen block quality (A11Y-03, TEST-02, UX-02, UX-03)**: The custom menu block concentrates findings across 5 review categories. It lacks ARIA table semantics (Critical), has zero test coverage (High), no responsive breakpoints (High), and missing CSS styling for header rows (Medium). As the most complex custom code and the highest-value content for a restaurant website, this block should be the primary focus for custom code improvements. + +3. **Performance critical path (PERF-01, UX-01)**: The unconditional `dapreview.js` import delays rendering for every visitor to benefit zero production users. The undefined `--overlay-color` variable leaves the mobile nav background transparent. Both are High severity, S effort fixes with immediate visible impact. + +### Overall Assessment + +The schamdan.de codebase is a lean AEM Edge Delivery Services site with zero runtime dependencies and clean linting baselines. The EDS boilerplate provides a solid foundation, and the site correctly leverages the platform's CDN-served content model. However, the project inherits accessibility gaps from the unmodified boilerplate header code, and the custom speisen block -- the site's most important feature -- was built without responsive design, semantic markup, or test coverage. The 3 Critical and 12 High findings are concentrated in accessibility and the speisen block, both addressable with moderate effort. The remaining Medium and Low findings represent standard technical debt for a site at this maturity level. + +**Recommended next step**: Fix the 3 Critical accessibility findings first (all S effort, approximately 2-3 hours total), then address the speisen block's ARIA semantics and responsive layout. + +--- + +## Severity Definitions + +| Severity | Description | Backlog Priority | +|----------|-------------|------------------| +| Critical | Must fix -- security vulnerabilities, broken functionality, accessibility barriers blocking users. | P0 | +| High | Should fix soon -- performance regressions, significant quality gaps, accumulating tech debt. | P1 | +| Medium | Should fix -- best practice violations, maintainability concerns, moderate risk. | P2 | +| Low | Consider fixing -- suggestions, minor improvements, polish. | P3 | + +## Effort Definitions + +| Effort | Time Estimate | +|--------|---------------| +| S | < 1 hour | +| M | 1-4 hours | +| L | 4-16 hours | +| XL | > 16 hours | + +--- + +## Methodology + +### Scope + +All website source code (HTML, CSS, JS, templates, configuration, CI/CD). Excludes content accuracy and third-party service configurations. + +### Tools + +- ESLint (airbnb-base) +- Stylelint (standard) +- Manual code review +- Static analysis + +### Baseline + +- ESLint passes clean (0 violations). +- Stylelint passes clean (0 violations). +- Test suite: 13 tests, all passing, 77.13% overall coverage. + +### Boilerplate Policy + +Unmodified AEM boilerplate files are reviewed but labeled as boilerplate in findings. Modifications to boilerplate files are reviewed as custom code. + +### Exclusions + +- `scripts/aem.js` internals (AEM SDK, not project-owned code) +- `.skills/` and `.agents/` directories (tooling configuration, not website code) +- DA content (authored content, not source code) + +### Finding Format + +Each finding follows this template: + +``` +#### [ID] Short descriptive title + +| Attribute | Value | +|-----------|-------| +| Severity | Critical / High / Medium / Low | +| Category | Code Quality / Performance / etc. | +| Files | `path/to/file.js:line` | +| Effort | S / M / L / XL | + +**Problem**: One paragraph describing what is wrong and why it matters. + +**Evidence**: Code snippet or reference showing the issue. + +**Recommendation**: Specific, actionable fix. +``` + +--- + +## Findings by Category + +### Code Quality (CODE) + +#### CODE-01 eslint-disable audit: eight suppressions in project-owned files + +| Attribute | Value | +|-----------|-------| +| Severity | Low | +| Category | Code Quality | +| Files | `scripts/scripts.js:20,48,57,73,119`, `scripts/delayed.js:1`, `scripts/dapreview.js:2`, `blocks/fragment/fragment.js:7` | +| Effort | S | + +**Problem**: There are 8 `eslint-disable` comments across 4 project-owned files (excluding `aem.js` which is AEM SDK code). While most are justified in the EDS boilerplate context, one is unnecessary, and none are documented for future contributors. + +**Evidence**: +- `scripts/scripts.js:20` -- `no-bitwise`: Required for `compareDocumentPosition()` bitmask. Valid. +- `scripts/scripts.js:48` -- `no-console`: `console.error` in auto-blocking catch. Valid for error reporting. +- `scripts/scripts.js:57` -- `import/prefer-default-export`: `decorateMain` is a named export consumed by `fragment.js`. Valid. +- `scripts/scripts.js:73` -- `no-await-in-loop`: This `await` (line 74) is **not inside a loop**. The suppression is unnecessary dead configuration. +- `scripts/scripts.js:119` -- `import/no-cycle`: Circular dependency between `scripts.js` and `delayed.js`. Valid (EDS pattern). +- `scripts/delayed.js:1` -- `import/no-cycle`: Counterpart of the above. Valid. +- `scripts/dapreview.js:2` -- `import/no-unresolved`: External CDN import (`da.live`). Valid. +- `blocks/fragment/fragment.js:7` -- `import/no-cycle`: Fragment imports `decorateMain` from `scripts.js`. Valid (EDS pattern). + +Note: All files except `speisen.js` and `dapreview.js` are AEM boilerplate code. + +**Recommendation**: Remove the unnecessary `no-await-in-loop` suppression at `scripts/scripts.js:73`. The remaining suppressions are valid. Consider adding a brief inline comment next to the `import/no-cycle` suppressions explaining the EDS circular loading pattern for future contributors. + +--- + +#### CODE-02 Silent catch blocks swallow errors without logging + +| Attribute | Value | +|-----------|-------| +| Severity | Medium | +| Category | Code Quality | +| Files | `scripts/scripts.js:33-37`, `scripts/scripts.js:84-90` | +| Effort | S | + +**Problem**: Two `try/catch` blocks in `scripts.js` catch exceptions and do nothing (`// do nothing`). The first guards `sessionStorage.setItem` in `loadFonts()`, the second guards `sessionStorage.getItem` and `loadFonts()` in `loadEager()`. While `sessionStorage` access can throw in private browsing mode, silently swallowing all errors makes debugging difficult. A thrown error inside `loadFonts()` (line 86) would also be silently lost. + +**Evidence**: +```js +// scripts/scripts.js:33-37 +try { + if (!window.location.hostname.includes('localhost')) sessionStorage.setItem('fonts-loaded', 'true'); +} catch (e) { + // do nothing +} + +// scripts/scripts.js:84-90 +try { + if (window.innerWidth >= 900 || sessionStorage.getItem('fonts-loaded')) { + loadFonts(); + } +} catch (e) { + // do nothing +} +``` + +Note: This is AEM boilerplate code. + +**Recommendation**: At minimum, add `console.debug` in these catch blocks so errors are visible during development but do not disrupt production. Better yet, narrow the try/catch to only the `sessionStorage` calls and let `loadFonts()` errors propagate normally. + +--- + +#### CODE-03 innerHTML from fetch responses (trusted content injection) + +| Attribute | Value | +|-----------|-------| +| Severity | Low | +| Category | Code Quality | +| Files | `blocks/header/header.js:31`, `blocks/footer/footer.js:17`, `blocks/fragment/fragment.js:26`, `blocks/cards/cards.js:8` | +| Effort | M | + +**Problem**: Four blocks set `innerHTML` from `fetch()` responses. In the EDS architecture, these responses come from the same-origin AEM content delivery pipeline (`.plain.html` fragments), so the XSS risk is low in practice. However, `innerHTML` bypasses any sanitization and would become a vulnerability if the content source ever changed to accept user-generated content. + +**Evidence**: +```js +// blocks/header/header.js:31 +nav.innerHTML = html; + +// blocks/footer/footer.js:17 +footer.innerHTML = html; + +// blocks/fragment/fragment.js:26 +main.innerHTML = await resp.text(); + +// blocks/cards/cards.js:8 +li.innerHTML = row.innerHTML; +``` + +Note: header.js, footer.js, fragment.js, and cards.js are AEM boilerplate code. + +**Recommendation**: This is an accepted EDS pattern since the content is author-controlled. No immediate action required. If the content model ever changes to accept third-party input, introduce a sanitization step (e.g., DOMPurify or the Sanitizer API). + +--- + +#### CODE-04 speisen.js innerHTML with regex-extracted content + +| Attribute | Value | +|-----------|-------| +| Severity | Low | +| Category | Code Quality | +| Files | `blocks/speisen/speisen.js:80-81` | +| Effort | S | + +**Problem**: The `splitPrices()` function builds HTML strings from regex capture groups and assigns them via `innerHTML`. The source data comes from author-controlled AEM content (menu item labels and prices), not user input. However, the regex capture group `m[1]` (the label portion, e.g., "0,3l") is injected into HTML without escaping. If a label ever contained characters like `<` or `&`, it would be interpreted as HTML. + +**Evidence**: +```js +// blocks/speisen/speisen.js:80-81 +sizesEl.innerHTML = matches.map((m) => `

${m[1]}

`).join(''); +pricesEl.innerHTML = matches.map((m) => `

${m[2]}

`).join(''); +``` + +**Recommendation**: Use `textContent` with DOM creation instead of string interpolation into `innerHTML`: +```js +matches.forEach((m) => { + const sp = document.createElement('p'); + sp.textContent = m[1]; + sizesEl.append(sp); + const pp = document.createElement('p'); + pp.textContent = m[2]; + pricesEl.append(pp); +}); +``` + +--- + +#### CODE-05 Inconsistent error handling across blocks + +| Attribute | Value | +|-----------|-------| +| Severity | Medium | +| Category | Code Quality | +| Files | `blocks/header/header.js`, `blocks/footer/footer.js`, `blocks/fragment/fragment.js`, `blocks/speisen/speisen.js`, `blocks/cards/cards.js`, `blocks/columns/columns.js` | +| Effort | M | + +**Problem**: Error handling is inconsistent across blocks. `scripts.js` wraps `buildAutoBlocks` in a try/catch, but none of the block `decorate()` functions have any error handling. If `header.js` or `footer.js` fetch calls fail (network error, not just non-200 response), the entire block decoration throws an unhandled rejection. The `resp.ok` check only handles HTTP error codes, not network failures or CORS errors. + +**Evidence**: +- `header.js:25`: `fetch()` can throw on network error; no try/catch. +- `footer.js:13`: Same pattern. +- `fragment.js:23`: Same pattern. +- `columns.js:2`: Assumes `block.firstElementChild` is non-null with no guard. + +Note: header.js, footer.js, fragment.js, cards.js, and columns.js are AEM boilerplate code. + +**Recommendation**: The AEM block loading framework (`aem.js`) wraps block decoration in a try/catch at the framework level (`aem.js:576-583`), so unhandled exceptions in block decorate functions are caught and logged. This mitigates the risk. For the custom `speisen.js` block, consider adding a guard at the top: `if (!block.children.length) return;` to handle empty blocks gracefully. + +--- + +#### CODE-06 CSS class naming follows different conventions across blocks + +| Attribute | Value | +|-----------|-------| +| Severity | Low | +| Category | Code Quality | +| Files | `blocks/speisen/speisen.css`, `blocks/cards/cards.css`, `blocks/header/header.css` | +| Effort | S | + +**Problem**: CSS class naming follows different conventions across blocks. The speisen block uses `item-nr`, `item-info`, `item-desc`, `item-sizes`, `item-prices` (component-prefix pattern). The cards block uses `cards-card-image`, `cards-card-body` (block-element pattern). The header uses `nav-brand`, `nav-sections`, `nav-tools`, `nav-hamburger` (component-prefix pattern). While each block is internally consistent, there is no project-wide convention. + +**Evidence**: +- speisen: `item-nr`, `item-info`, `item-desc`, `item-sizes`, `item-prices` +- cards: `cards-card-image`, `cards-card-body` +- header: `nav-brand`, `nav-sections`, `nav-tools`, `nav-hamburger`, `nav-hamburger-icon` + +Note: cards.js and header.js are AEM boilerplate code. speisen.js is custom code. + +**Recommendation**: Cosmetic and low priority. The EDS block scoping model (each block's CSS is scoped by the block class name in the parent) means collision risk is minimal. No action required unless the project adopts a formal CSS naming convention. + +--- + +#### CODE-07 Magic number 900 used as breakpoint in JavaScript + +| Attribute | Value | +|-----------|-------| +| Severity | Low | +| Category | Code Quality | +| Files | `scripts/scripts.js:85` | +| Effort | S | + +**Problem**: The number `900` is hardcoded in `scripts.js` as a viewport width threshold for font loading. This value corresponds to the 900px breakpoint used in `styles.css` but is not derived from a shared source. If the breakpoint changes in CSS, the JavaScript threshold would need to be updated independently. + +**Evidence**: +```js +// scripts/scripts.js:85 +if (window.innerWidth >= 900 || sessionStorage.getItem('fonts-loaded')) { +``` + +Note: This is AEM boilerplate code. + +**Recommendation**: Low-risk since the 900px breakpoint is an EDS convention unlikely to change. For documentation, add a comment: `// matches @media (width >= 900px) breakpoint in styles.css`. + +### Maintainability (MAINT) + +#### MAINT-01 ESLint 8.x is end-of-life + +| Attribute | Value | +|-----------|-------| +| Severity | High | +| Category | Maintainability | +| Files | `package.json:30` | +| Effort | L | + +**Problem**: The project uses ESLint 8.57.1, which reached end-of-life on 2024-10-05. ESLint 8.x no longer receives bug fixes, security patches, or compatibility updates. Additionally, `@babel/eslint-parser` is unnecessary for this project since all code uses standard ES module syntax that ESLint's default parser handles natively. + +**Evidence**: +```json +"eslint": "8.57.1", +"eslint-config-airbnb-base": "15.0.0", +"eslint-plugin-import": "2.32.0", +"@babel/eslint-parser": "7.28.6", +``` + +**Recommendation**: Migrate to ESLint 9.x with the new flat config format (`eslint.config.js`). This requires: (1) Replace `.eslintrc.js` with `eslint.config.js` using flat config syntax. (2) Replace `eslint-config-airbnb-base` with `@stylistic/eslint-plugin` or equivalent (airbnb-base has limited ESLint 9 support). (3) Remove `@babel/eslint-parser` (unnecessary for ES module syntax). (4) Update `eslint-plugin-import` to a version supporting ESLint 9 or switch to `eslint-plugin-import-x`. (5) Update CI workflow to verify the migration. + +--- + +#### MAINT-02 Breakpoint inconsistency: header uses 1000px, rest uses 900px + +| Attribute | Value | +|-----------|-------| +| Severity | Medium | +| Category | Maintainability | +| Files | `blocks/header/header.css:170`, `styles/styles.css:212,218`, `blocks/columns/columns.css:10` | +| Effort | S | + +**Problem**: The header block switches to desktop layout at 1000px while all other components switch at 900px. This creates a 100px window (900-999px) where the page layout is desktop but the navigation is still in mobile hamburger mode. + +**Evidence**: +All breakpoints in the codebase: +- `styles/styles.css:212`: `@media (width >= 600px)` -- section padding adjustment +- `styles/styles.css:218`: `@media (width >= 900px)` -- heading size scale-up, max-width constraint +- `blocks/columns/columns.css:10`: `@media (width >= 900px)` -- horizontal column layout +- `blocks/header/header.css:170`: `@media (width >= 1000px)` -- desktop navigation + +Note: header.css is AEM boilerplate code. + +**Recommendation**: Change the header breakpoint from 1000px to 900px to match the rest of the site. Verify the navigation layout works at 900px viewport width. If the navigation genuinely needs more space, document why 1000px is intentional. + +--- + +#### MAINT-03 Dead code: empty fonts.css and lazy-styles.css with active loadFonts() + +| Attribute | Value | +|-----------|-------| +| Severity | Low | +| Category | Maintainability | +| Files | `styles/fonts.css`, `styles/lazy-styles.css`, `scripts/scripts.js:31-38,86,110` | +| Effort | S | + +**Problem**: Both `fonts.css` and `lazy-styles.css` contain only placeholder comments and no actual CSS rules. The `loadFonts()` function in `scripts.js` fetches `fonts.css` and sets a sessionStorage flag, but the file is empty -- the font stack relies on system fonts (`Palatino`, `Times`, `Times New Roman`) declared in `styles.css:23`. This means `loadFonts()` makes unnecessary network requests and the sessionStorage logic serves no purpose. + +**Evidence**: +```css +/* styles/fonts.css -- entire file */ +/* load fonts */ + +/* styles/lazy-styles.css -- entire file */ +/* below the fold CSS goes here */ +``` + +`loadFonts()` is called twice: once in `loadEager()` (line 86, conditionally) and once in `loadLazy()` (line 110, unconditionally). + +Note: These are AEM boilerplate placeholder files. + +**Recommendation**: Two options: (1) If custom web fonts will be added later, keep the files as placeholders but add a comment in `loadFonts()` explaining they are intentionally empty. (2) If the site will continue using system fonts, remove the `loadFonts()` function, its two call sites, and the empty `fonts.css`. Keep `lazy-styles.css` as an EDS convention placeholder. + +--- + +#### MAINT-04 loadFonts() called twice: eager and lazy phases + +| Attribute | Value | +|-----------|-------| +| Severity | Low | +| Category | Maintainability | +| Files | `scripts/scripts.js:86,110` | +| Effort | S | + +**Problem**: `loadFonts()` is called in both `loadEager()` (line 86, conditionally on viewport width or sessionStorage) and `loadLazy()` (line 110, unconditionally). Even if `fonts.css` had content, the duplicate call in `loadLazy` would be redundant on desktop since `loadEager` already loaded it. + +**Evidence**: +```js +// loadEager (line 84-90) -- conditional +if (window.innerWidth >= 900 || sessionStorage.getItem('fonts-loaded')) { + loadFonts(); +} + +// loadLazy (line 110) -- unconditional +loadFonts(); +``` + +Note: This is AEM boilerplate code. The `loadCSS` function in `aem.js` already de-duplicates by checking for existing `` elements, so the double call is not harmful -- just unnecessary dead code. + +**Recommendation**: Since `fonts.css` is empty and the site uses system fonts, remove both `loadFonts()` calls and the function itself. If web fonts are added later, restore the EDS pattern. + +--- + +#### MAINT-05 EDS boilerplate drift inventory + +| Attribute | Value | +|-----------|-------| +| Severity | Low | +| Category | Maintainability | +| Files | Multiple | +| Effort | S | + +**Problem**: There is no documented inventory of which files are unmodified EDS boilerplate vs. project-customized. Without this, future EDS SDK upgrades risk overwriting customizations or missing improvements. + +**Evidence**: + +**Unmodified boilerplate** (safe to overwrite on EDS upgrade): +- `blocks/cards/cards.js`, `blocks/cards/cards.css` +- `blocks/columns/columns.js`, `blocks/columns/columns.css` +- `blocks/fragment/fragment.js` +- `blocks/hero/hero.css` +- `styles/fonts.css`, `styles/lazy-styles.css` +- `404.html` + +**Lightly customized boilerplate** (merge carefully): +- `scripts/scripts.js` -- `document.documentElement.lang = 'de-DE'` (line 72), DA preview import (lines 73-75) +- `blocks/header/header.js`, `blocks/header/header.css` -- Unchanged JS logic, CSS may have minor styling tweaks +- `blocks/footer/footer.js`, `blocks/footer/footer.css` -- Unchanged JS, CSS has site-specific styling +- `head.html` -- Standard EDS head +- `.eslintrc.js` -- Standard EDS lint config +- `scripts/delayed.js` -- Custom external link handling added (lines 9-19) + +**Custom (project-specific)**: +- `blocks/speisen/speisen.js`, `blocks/speisen/speisen.css` -- Entirely custom menu block +- `scripts/dapreview.js` -- DA preview integration +- `styles/styles.css` -- Custom color palette, typography, layout (lines 13-42) + +**Recommendation**: Add a `# Boilerplate Status` section to the project README or a dedicated `BOILERPLATE.md` documenting which files are boilerplate vs. customized, and the EDS boilerplate version they were forked from. This enables safe future upgrades. + +--- + +#### MAINT-06 CSS custom properties incomplete: hardcoded max-width and spacing values + +| Attribute | Value | +|-----------|-------| +| Severity | Low | +| Category | Maintainability | +| Files | `blocks/speisen/speisen.css:8,34`, `blocks/hero/hero.css:13-15,19`, `blocks/header/header.css:8`, `blocks/footer/footer.css:8`, `styles/styles.css:147` | +| Effort | S | + +**Problem**: The `:root` custom properties in `styles.css` define colors, fonts, and heading sizes, but several layout values are hardcoded across multiple files. The `max-width: 900px` value appears in four separate files. Spacing values in `speisen.css` are hardcoded rather than using a shared scale. + +**Evidence**: +- `max-width: 900px` repeated in: `styles/styles.css:147`, `blocks/hero/hero.css:19`, `blocks/footer/footer.css:8`, `blocks/header/header.css:8` +- `blocks/speisen/speisen.css:8`: `margin-bottom: 8px` (hardcoded spacing) +- `blocks/speisen/speisen.css:34`: `padding-left: 8px` (hardcoded spacing) +- `blocks/hero/hero.css:15`: `margin-top: 80px` (hardcoded, likely related to `--nav-height: 64px` plus padding) +- `blocks/hero/hero.css:14`: `min-height: 300px` (hardcoded) + +**Recommendation**: Extract `max-width: 900px` into a custom property `--content-max-width: 900px` in `:root` and reference it across all files. For the speisen block spacing, the values are small and scoped -- custom properties are optional but would improve consistency if a design token system is adopted. + +--- + +#### MAINT-07 CI/CD workflow only triggers on pull_request + +| Attribute | Value | +|-----------|-------| +| Severity | Medium | +| Category | Maintainability | +| Files | `.github/workflows/run-tests.yaml` | +| Effort | S | + +**Problem**: The GitHub Actions workflow runs `npm run lint` and `npm test` on pull requests only, not on pushes to `main`. Code merged directly to `main` (including admin merges or direct pushes) bypasses CI entirely. + +**Evidence**: +```yaml +name: Tests and Linting +on: [pull_request] +``` + +**Recommendation**: Add `push` trigger for the `main` branch: `on: { pull_request: {}, push: { branches: [main] } }`. Consider enabling branch protection rules requiring the CI check to pass before merging. + +--- + +#### MAINT-08 Outdated testing dependencies: sinon 14.x and chai 4.x + +| Attribute | Value | +|-----------|-------| +| Severity | Low | +| Category | Maintainability | +| Files | `package.json:26-29` | +| Effort | M | + +**Problem**: Several testing dependencies are significantly outdated. `sinon` is at 14.0.1 (current major: 19.x), and `chai` at 4.3.6 with the `@esm-bundle/chai` wrapper at 4.3.4-fix.0. Chai 5.x has been available since early 2024 with native ESM support, which would eliminate the need for the `@esm-bundle/chai` wrapper. + +**Evidence**: +```json +"@esm-bundle/chai": "4.3.4-fix.0", +"chai": "4.3.6", +"sinon": "14.0.1", +``` + +**Recommendation**: Update `sinon` to the latest 19.x and evaluate migrating from `chai` 4.x + `@esm-bundle/chai` to `chai` 5.x with native ESM. This is a test-only change with no production impact, but the `@esm-bundle/chai` wrapper may have compatibility constraints with `@web/test-runner`. Test thoroughly after upgrading. + +--- + +#### MAINT-09 No CODEOWNERS file or branch protection + +| Attribute | Value | +|-----------|-------| +| Severity | Low | +| Category | Maintainability | +| Files | `.github/` | +| Effort | S | + +**Problem**: The repository has no `CODEOWNERS` file and no documented branch protection rules. For a single-contributor project this is acceptable, but the lack of required reviewers for critical files (e.g., `scripts/scripts.js`, CI configuration) increases risk as contributors are added. + +**Evidence**: No `.github/CODEOWNERS` file exists. The CI workflow only triggers on `pull_request`, and there is no evidence of branch protection rules. + +**Recommendation**: Low priority for a single-contributor project. When additional contributors are added, create a `.github/CODEOWNERS` file and enable branch protection on `main` requiring at least one approval and passing CI checks. + +### Performance (PERF) + +#### PERF-01 dapreview.js imported unconditionally on the critical path + +| Attribute | Value | +|-----------|-------| +| Severity | High | +| Category | Performance | +| Files | `scripts/scripts.js:74-75` | +| Effort | S | + +**Problem**: `loadEager()` unconditionally executes `await import('./dapreview.js')` before any rendering begins. Every visitor -- not just DA preview users -- pays the cost of importing and evaluating this module. The dynamic `import()` inside `dapreview.js` (line 3) is itself conditional on the `?dapreview` query parameter, but the outer import in `scripts.js` is not. This adds an unnecessary network request and JavaScript evaluation to the critical rendering path, delaying LCP for all visitors. + +**Evidence**: +```js +// scripts/scripts.js:74-75 +const daPreview = (await import(`${import.meta.url.replace('scripts.js', 'dapreview.js')}`)).default; +if (daPreview) await daPreview; +``` +The `await import(...)` call runs before `decorateTemplateAndTheme()`, `decorateMain()`, and `document.body.classList.add('appear')`. This means the browser cannot begin rendering until `dapreview.js` has been fetched, parsed, and executed. + +**Recommendation**: Guard the import with a query parameter check so non-preview visitors skip it entirely: +```js +if (new URLSearchParams(window.location.search).has('dapreview')) { + const { default: daPreview } = await import('./dapreview.js'); + if (daPreview) await daPreview; +} +``` +This removes the module from the critical path for all production visitors (100% of real users). + +--- + +#### PERF-02 Empty CSS files fetched on every page load + +| Attribute | Value | +|-----------|-------| +| Severity | Medium | +| Category | Performance | +| Files | `styles/fonts.css:1`, `styles/lazy-styles.css:1`, `scripts/scripts.js:32`, `scripts/scripts.js:109` | +| Effort | S | + +**Problem**: Both `fonts.css` (contains only a comment `/* load fonts */`) and `lazy-styles.css` (contains only `/* below the fold CSS goes here */`) are fetched on every page load despite containing no useful declarations. Each file triggers an HTTP request (DNS lookup if not cached, TCP connection, TLS negotiation on first visit) with zero benefit. `fonts.css` is loaded in `loadEager()` (line 85-87) on desktop or when `sessionStorage` has the fonts-loaded flag, and again in `loadLazy()` (line 110). `lazy-styles.css` is loaded in `loadLazy()` (line 109). + +**Evidence**: +```css +/* styles/fonts.css — entire file: */ +/* load fonts */ +``` +```css +/* styles/lazy-styles.css — entire file: */ +/* below the fold CSS goes here */ +``` + +**Recommendation**: Either populate these files with actual content (e.g., `@font-face` declarations in `fonts.css` if web fonts are needed, below-fold styles in `lazy-styles.css`) or remove the `loadCSS()` calls from `scripts.js` until they contain real declarations. Removing two wasted HTTP requests improves FCP and reduces overall page load time. Note: These are AEM EDS boilerplate placeholder files. If no custom web fonts or lazy styles are needed, the safest approach is to remove the `loadCSS()` calls rather than deleting the files (to avoid confusion with the boilerplate convention). + +--- + +#### PERF-03 Hero LCP image missing fetchpriority="high" + +| Attribute | Value | +|-----------|-------| +| Severity | Medium | +| Category | Performance | +| Files | `scripts/aem.js:654-665` (upstream boilerplate) | +| Effort | S | + +**Problem**: The `waitForFirstImage()` function in `aem.js` sets `loading="eager"` on the LCP candidate image but does not set `fetchpriority="high"`. Without `fetchpriority="high"`, the browser may still deprioritize the hero image fetch behind other resources (stylesheets, scripts) even though it has `loading="eager"`. This is a significant LCP optimization opportunity -- Chrome and other browsers use `fetchpriority` to boost the image in the network priority queue, which can improve LCP by 100-400ms depending on network conditions. + +**Evidence**: +```js +// scripts/aem.js:654-665 +async function waitForFirstImage(section) { + const lcpCandidate = section.querySelector('img'); + await new Promise((resolve) => { + if (lcpCandidate && !lcpCandidate.complete) { + lcpCandidate.setAttribute('loading', 'eager'); + // Missing: lcpCandidate.setAttribute('fetchpriority', 'high'); + lcpCandidate.addEventListener('load', resolve); + lcpCandidate.addEventListener('error', resolve); + } else { + resolve(); + } + }); +} +``` + +**Recommendation**: This involves `aem.js` (upstream boilerplate). Consider adding `fetchpriority="high"` in block-specific code. In `scripts.js`, after `waitForFirstImage` resolves, or by overriding the LCP image handling in the hero block decorator. Alternatively, add `fetchpriority="high"` directly in the hero block's `decorate()` function: +```js +const heroImg = block.querySelector('img'); +if (heroImg) heroImg.fetchPriority = 'high'; +``` + +--- + +#### PERF-04 Hero margin-top vs --nav-height mismatch causes layout gap + +| Attribute | Value | +|-----------|-------| +| Severity | Medium | +| Category | Performance | +| Files | `blocks/hero/hero.css:15`, `styles/styles.css:41` | +| Effort | S | + +**Problem**: The hero block uses `margin-top: 80px` (hero.css line 15) to account for the fixed navigation bar, but `--nav-height` is defined as `64px` (styles.css line 41). This 16px discrepancy creates an unnecessary visual gap between the nav and hero. More importantly from a performance perspective, the hero image is pushed 16px further down the viewport, which can delay when the browser considers it as the LCP element and triggers the LCP paint. Any additional space above the LCP element increases the risk of it being partially or fully below the initial viewport on smaller screens, degrading the LCP measurement. + +**Evidence**: +```css +/* blocks/hero/hero.css:15 */ +main .hero { + margin-top: 80px; +} + +/* styles/styles.css:41 */ +:root { + --nav-height: 64px; +} +``` + +**Recommendation**: Replace the hardcoded `80px` with `var(--nav-height)` to keep the hero block consistent with the navigation height. If intentional padding is desired below the nav, use `calc(var(--nav-height) + 16px)` and add a comment explaining the extra spacing. + +--- + +#### PERF-05 No AVIF format in createOptimizedPicture + +| Attribute | Value | +|-----------|-------| +| Severity | Low | +| Category | Performance | +| Files | `scripts/aem.js:315-361` (upstream boilerplate) | +| Effort | M | + +**Problem**: The `createOptimizedPicture()` function generates `` elements for WebP and falls back to the original format, but does not include AVIF sources. AVIF offers ~50% better compression than JPEG and ~20% better than WebP at equivalent quality. All modern browsers (Chrome, Firefox, Safari 16+, Edge) support AVIF as of 2024. For a restaurant website where hero images and food photography are central, AVIF could significantly reduce image payload and improve LCP. + +**Evidence**: +```js +// scripts/aem.js:326-336 — only WebP sources generated +breakpoints.forEach((br) => { + const source = document.createElement('source'); + if (br.media) source.setAttribute('media', br.media); + source.setAttribute('type', 'image/webp'); + source.setAttribute('srcset', + `${origin}${pathname}?width=${br.width}&format=webply&optimize=medium`); + picture.appendChild(source); +}); +``` + +**Recommendation**: This involves `aem.js` (upstream boilerplate). The AEM image optimization CDN supports AVIF via `format=avif`. Consider overriding `createOptimizedPicture` in project code or creating a project-level wrapper that adds AVIF `` elements before WebP sources in the `` element. This is dependent on the AEM CDN supporting the `format=avif` parameter -- verify before implementing. + +--- + +#### PERF-06 Oversized image breakpoints (2000px / 750px) + +| Attribute | Value | +|-----------|-------| +| Severity | Low | +| Category | Performance | +| Files | `scripts/aem.js:319` (upstream boilerplate) | +| Effort | M | + +**Problem**: `createOptimizedPicture()` uses default breakpoints of `2000px` (desktop) and `750px` (mobile). The site's `main` content area has `max-width: 900px` (styles.css line 147), meaning a 2000px-wide image is more than 2x wider than necessary for content images. For the hero block, which is full-width (`max-width: unset` in hero.css), the 2000px breakpoint is more appropriate but still potentially oversized for most viewports. The gap between 750px and 2000px means mid-range devices (tablets at 768-1024px) receive either an undersized or oversized image. + +**Evidence**: +```js +// scripts/aem.js:319 +breakpoints = [{ media: '(min-width: 600px)', width: '2000' }, { width: '750' }] +``` +```css +/* styles/styles.css:147 */ +main { max-width: 900px; } +``` + +**Recommendation**: This involves `aem.js` (upstream boilerplate). Consider passing custom breakpoints when calling `createOptimizedPicture` in block-level code. For content images, breakpoints like `[{ media: '(min-width: 600px)', width: '900' }, { width: '750' }]` would better match the layout. For the full-width hero, a three-tier approach could reduce payload: `[{ media: '(min-width: 1200px)', width: '1600' }, { media: '(min-width: 600px)', width: '1000' }, { width: '750' }]`. + +--- + +#### PERF-07 No performance budget defined + +| Attribute | Value | +|-----------|-------| +| Severity | Low | +| Category | Performance | +| Files | (project-wide) | +| Effort | M | + +**Problem**: The project has no defined performance budget -- no Lighthouse CI configuration, no bundle size limits, and no Core Web Vitals thresholds. Without a budget, there is no automated mechanism to detect performance regressions before they reach production. Given that this is a restaurant website where mobile performance and fast LCP directly impact user experience (potential customers looking up the menu), establishing performance guardrails is important. + +**Evidence**: No `lighthouserc.js`, `.lighthouserc.json`, `budget.json`, or equivalent configuration file exists in the repository. No CI/CD performance checks were found. + +**Recommendation**: Define a performance budget with three tiers: +1. **Metric-based**: LCP < 2.5s, CLS < 0.1, INP < 200ms, FCP < 1.8s, TBT < 200ms +2. **Resource-based**: Total page weight < 500 KB (compressed), JavaScript < 100 KB, CSS < 30 KB, images < 300 KB +3. **Lighthouse score**: Performance score >= 90 + +Implement using Lighthouse CI in the CI/CD pipeline with a `lighthouserc.js` configuration that fails builds on regression. Start with the current baseline and tighten over time. + +--- + +#### PERF-08 body { display: none } without JavaScript failure fallback + +| Attribute | Value | +|-----------|-------| +| Severity | Low | +| Category | Performance | +| Files | `styles/styles.css:52-57`, `scripts/scripts.js:80` | +| Effort | S | + +**Problem**: The body starts hidden (`display: none` in styles.css line 52) and is revealed by JavaScript adding `body.appear` (scripts.js line 80). This is a standard AEM EDS pattern to prevent FOUC (flash of unstyled content), but if JavaScript fails to load or execute (network error, CSP violation, runtime exception before line 80), the page remains permanently invisible. Users see a blank page with no content whatsoever. + +**Evidence**: +```css +/* styles/styles.css:52-57 */ +body { + display: none; +} +body.appear { + display: unset; +} +``` +```js +// scripts/scripts.js:80 +document.body.classList.add('appear'); +``` + +**Recommendation**: Add a `