fix(#540): remove dead sidebarMarkup/badgeFor and the setBadge chain it left orphaned

[richard-devbot]

Summary

sidebarMarkup() and badgeFor() in ui/pages/index.js had no caller anywhere in src/ — the live navigation has been the #278 destination shell (desktopNavigationMarkup/navigationGroups in ui/navigation.js) for a while. Deleted both.

Following the issue's own "check before deleting" instruction surfaced a larger, connected chain of dead code from the same superseded design:

• tests/dashboard-e2e-96.test.js's nav/container-parity test called sidebarMarkup() directly — it was silently validating dead code the whole time, giving false confidence that nav/container parity and landmarks held against the UI users actually see. Retargeted at the live desktopNavigationMarkup()/pageMarkup() pairing instead of dropping the coverage outright — this is a genuine, previously-missing regression test for the real [P1] Replace the 21-item primary navigation with a six-destination responsive shell #278 shell. Mutation-tested against two independent real regressions (a nav-id typo, a page container silently omitted) — both correctly caught.
• client.js's setBadge('badge-approvals', ...) / setBadge('badge-alerts', ...) calls target ids that only sidebarMarkup() ever rendered — nothing in the live nav does. These were themselves silently dead in production, not just in tests (setBadge no-ops when the element is missing). Removed both calls; the real topbar pending/alert counts already render via separate, live approval-count/alert-count elements.
• With those two calls gone, setBadge() in lib.js became the only definition with zero remaining callers anywhere in the concatenated client bundle — removed it too.
• tests/dashboard-client-modules.test.js asserted the literal setBadge('badge-approvals', ...) call text existed in the bundle — retargeted to the real live topbar approval-count markup instead of deleting the check outright.

Verification

• 1950/1950 core tests, 43/43 browser tests (including the new IA batch: Approvals in nav, Run Workspace run picker, long-path truncation with copy (#501 children 2-4) #537 IA-batch suites), typecheck/lint/security-audit/validate all clean.
• The retargeted nav-parity test mutation-tested against two independent real regressions (a nav-id typo, a silently-omitted page container) — both correctly caught.

Test plan

• npm test — 1950/1950
• npm run test:browser — 43/43
• npm run typecheck / npm run lint — 0 errors
• node scripts/security-audit.mjs / npm run validate — clean
• Retargeted test mutation-tested against 2 real regressions

🤖 Generated with Claude Code

[strix-security]

Strix is installed on this repository, but we couldn't run this PR security review because this workspace's trial has ended. Add a card to resume code reviews here.

[coderabbitai]

Warning

Review limit reached

@richard-devbot, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 49 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 389f8b19-3e5c-42c7-bea5-5cadb836423f

📥 Commits

Reviewing files that changed from the base of the PR and between 91fb336 and c748320.

📒 Files selected for processing (5)

• src/observability/dashboard/ui/client.js
• src/observability/dashboard/ui/lib.js
• src/observability/dashboard/ui/pages/index.js
• tests/dashboard-client-modules.test.js
• tests/dashboard-e2e-96.test.js

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[qodo-code-review]

PR Summary by Qodo

Remove dead dashboard sidebar badge code and retarget nav parity tests

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

AI Description

• Delete unused sidebar navigation markup and badge rendering helpers from dashboard UI.
• Remove orphaned setBadge() calls/utility; rely on live topbar alert/approval counts.
• Retarget regression and bundle tests to validate the live #278 navigation shell.

Diagram

graph TD
  E2E["test: nav parity (#96)"] --> NAV["ui/navigation.js\ndesktopNavigationMarkup()"] --> IDS["data-page ids"] --> PAGES["ui/pages/index.js\npageMarkup()"] --> SECTS["#page-* sections"]
  CLIENT["ui/client.js\nrenderFrame()"] --> LIB["ui/lib.js\nDOM helpers"]
  BTEST["test: bundle/ARIA contract"] --> HTML["dashboard HTML snapshot"] --> NAV
  HTML --> CLIENT

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Test via full `dashboardHtml()` only (no direct markup imports)

• ➕ Even closer to what users receive over the wire
• ➕ Catches wiring/assembly regressions (e.g., nav omitted from HTML) without additional tests
• ➖ Harder to isolate failures to nav vs pages vs assembly
• ➖ May require more fixtures/HTML parsing to keep assertions robust

2. Use DOM parsing (JSDOM) instead of regex-based string matching

• ➕ Less brittle than regex for attribute ordering/whitespace changes
• ➕ More natural assertions for landmark structure and ids
• ➖ Adds heavier test dependency/runtime cost
• ➖ May be at odds with current lightweight string-snapshot testing style

Recommendation: Keep the PR’s approach: remove the dead badge/sidebar chain and retarget tests directly to the live desktopNavigationMarkup() + pageMarkup() outputs. It preserves fast, dependency-free tests while ensuring coverage is aligned with the UI users actually see; if brittleness becomes an issue, consider DOM parsing as a follow-up.

Files changed (5) +30 / -51

Bug fix (1) +0 / -2

client.jsRemove dead 'setBadge()' updates for approvals/alerts +0/-2

Remove dead 'setBadge()' updates for approvals/alerts

• Drops 'setBadge('badge-approvals'| 'badge-alerts', ...)' calls that targeted elements no longer rendered by the live navigation shell. The client continues to update the live topbar text counters ('approval-count', 'alert-count') and chip classes.

src/observability/dashboard/ui/client.js

Refactor (2) +0 / -33

lib.jsDelete unused 'setBadge()' DOM helper +0/-8

Delete unused 'setBadge()' DOM helper

• Removes the 'setBadge()' helper function after its last callers were deleted. This trims dead bundle code and avoids implying badge elements are part of the supported UI contract.

src/observability/dashboard/ui/lib.js

index.jsRemove unused sidebar navigation renderer and badge markup +0/-25

Remove unused sidebar navigation renderer and badge markup

• Deletes 'sidebarMarkup()' and its internal 'badgeFor()' helper, which were not referenced by the live navigation implementation. Keeps 'pageMarkup()' unchanged as the source of page container sections.

src/observability/dashboard/ui/pages/index.js

Tests (2) +30 / -16

dashboard-client-modules.test.jsRetarget ARIA/bundle assertion from dead badges to live topbar counts +5/-2

Retarget ARIA/bundle assertion from dead badges to live topbar counts

• Replaces the bundle string assertion that looked for 'setBadge('badge-approvals', ...)' with a check for the real approvals button/count markup. Updates comments to clarify the prior assertion was validating dead code.

tests/dashboard-client-modules.test.js

dashboard-e2e-96.test.jsRetarget nav/container parity test to 'desktopNavigationMarkup()' +25/-14

Retarget nav/container parity test to 'desktopNavigationMarkup()'

• Stops importing/calling 'sidebarMarkup()' and instead validates the live 'desktopNavigationMarkup()' output against 'pageMarkup()' containers. Adjusts expectations to be one-directional (visible nav entries must have containers; hidden pages need not appear in nav) and removes badge-id assertions that were tied to the dead sidebar design.

tests/dashboard-e2e-96.test.js

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (2)

Context used

✅ Compliance rules (platform): 300 rules

✅ Skills: 17 invoked

  code-review-pr

  claude-api

  documentation-writing

  pptx

  docx

  performance-monitoring

  security-compliance

  cso

  plan-eng-review

  design-review

  prompt-engineering

  mcp-builder

  qa-testing

  code-patterns

  xlsx

  security-owasp

  testing-qa

1. Shallow navIds.length assertion ✓ Resolved 📜 Skill insight ▣ Testability

Description

The new test only asserts navIds.length > 0, which checks mere existence rather than verifying the
live nav contains the expected destinations. This can pass even if most navigation destinations
regress or disappear, weakening the regression coverage.

Code

tests/dashboard-e2e-96.test.js[372]

+  assert.ok(navIds.length > 0, 'the live nav renders destination targets');

Relevance

●● Moderate

Mixed history: strengthening weak truthy assertions accepted in some tests, but similar “vague
assertion” feedback also rejected.

PR-#536
PR-#503
PR-#520

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 1399609 requires assertions to verify actual behavior rather than mere
existence/truthiness. The assertion at tests/dashboard-e2e-96.test.js:372 only checks that
navIds is non-empty, without validating which destinations are present.

tests/dashboard-e2e-96.test.js[372-372]
Skill: qa-testing

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The test uses an existence-only assertion (`navIds.length > 0`), which does not validate meaningful navigation behavior.

## Issue Context
Compliance requires test assertions to verify actual behavior, not just that something exists/truthy. This regression test should confirm at least one known expected destination is present (or that the set/count matches a stable expected baseline for visible destinations).

## Fix Focus Areas
- tests/dashboard-e2e-96.test.js[369-380]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Brittle HTML assertion ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

tests/dashboard-client-modules.test.js now matches an exact serialized substring for the Approvals
topbar button, making the test sensitive to benign attribute reordering/whitespace changes in
dashboardHtml(). This can cause false-negative test failures even when the accessibility/behavioral
contract is unchanged.

Code

tests/dashboard-client-modules.test.js[R79-83]

+  // Topbar alert/approval counts are visible text, not an icon-only badge
+  // needing a separate spoken label (#540: the older aria-labeled
+  // badge-approvals/badge-alerts elements this used to check were dead code
+  // — nothing in the live nav ever rendered them).
+  assert.match(html, /id="btn-approvals" onclick="showPage\('approvals'\)">Approvals <span id="approval-count">/);

Relevance

●● Moderate

No clear historical evidence on avoiding exact HTML serialization/attribute-order regexes in tests;
pattern appears common.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The test currently hard-codes the exact id+onclick sequence and spacing, while the actual button
markup is generated in dashboardHtml(); changing serialization without changing meaning would
break the test.

tests/dashboard-client-modules.test.js[62-84]
src/observability/dashboard/ui/index.js[56-60]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
A unit test asserts the exact HTML serialization of the Approvals button (including attribute order and spacing). This is unnecessarily brittle and can fail on non-semantic markup changes.

### Issue Context
The markup comes from `dashboardHtml()` and is string-assembled; minor template edits (adding a class, reordering attributes, inserting new attributes) should not break the accessibility contract test.

### Fix Focus Areas
- tests/dashboard-client-modules.test.js[79-84]
- src/observability/dashboard/ui/index.js[56-60]

### Suggested change
Replace the single strict regex with one of:
- Two or three independent assertions (e.g., `id="btn-approvals"`, `onclick="showPage('approvals')"`, and `id="approval-count"`), or
- A more tolerant regex such as:
 - `/id="btn-approvals"[^>]*>\s*Approvals\s*<span[^>]*id="approval-count"/`
 so attribute order/whitespace do not matter.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Regression added to existing test 📜 Skill insight ⚙ Maintainability

Description

A regression check was added by modifying existing test files instead of creating a new dedicated
regression test file. This reduces traceability/auditability of regressions and violates the
required regression-test isolation convention.

Code

tests/dashboard-e2e-96.test.js[R365-390]

+test('dashboard regression (#96): every live nav destination has a container and correct landmarks', () => {
+  const nav = desktopNavigationMarkup();
  const pages = pageMarkup();

-  const navIds = [...sidebar.matchAll(/data-page="([^"]+)"/g)].map((match) => match[1]);
+  const navIds = [...nav.matchAll(/data-page="([^"]+)"/g)].map((match) => match[1]);
  const sectionIds = [...pages.matchAll(/<section class="page[^"]*" id="page-([^"]+)"/g)].map((match) => match[1]);

-  assert.ok(navIds.length > 0, 'sidebar renders nav targets');
+  assert.ok(navIds.length > 0, 'the live nav renders destination targets');
+  // One direction only: every VISIBLE nav link must resolve to a real page
+  // container. The reverse does not hold by design — pageMarkup() still
+  // renders every page unconditionally, but several of those are
+  // intentionally demoted `hidden: true` children (#278/#284), reachable by
+  // deep link but deliberately absent from navigationGroups()'s
+  // visibleChildren filter.
  for (const id of navIds) {
    assert.ok(sectionIds.includes(id), `nav target "${id}" has a matching #page-${id} container`);
  }
-  for (const id of sectionIds) {
-    assert.ok(navIds.includes(id), `page container "${id}" is reachable from the nav`);
-  }

  // Landmarks: one h1 per destination, aria-current marks the active entry.
  for (const id of sectionIds) {
    const section = pages.slice(pages.indexOf(`id="page-${id}"`));
    assert.match(section.slice(0, 600), /<h1 class="page-title">/, `destination "${id}" leads with an h1`);
  }
-  assert.match(sidebar, /aria-current="page"/, 'the active nav entry carries aria-current');
-  assert.match(sidebar, /aria-hidden="true"/, 'nav icons are hidden from the accessibility tree');
-
-  // Actionable badges exist for the two attention surfaces.
-  assert.match(sidebar, /id="badge-approvals"/);
-  assert.match(sidebar, /id="badge-alerts"/);
+  assert.match(nav, /aria-current="page"/, 'the active nav entry carries aria-current');
+  assert.match(nav, /aria-hidden="true"/, 'nav icons are hidden from the accessibility tree');

Relevance

● Weak

Requests to isolate regressions into new dedicated test files (not editing existing suites) were
consistently rejected.

PR-#503
PR-#513
PR-#488

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 1400748 requires regression tests be introduced via new test files and prohibits
modifying existing test files for regression additions. The diff shows regression-focused
assertions/comments being added by editing existing files tests/dashboard-e2e-96.test.js and
tests/dashboard-client-modules.test.js.

tests/dashboard-e2e-96.test.js[365-390]
tests/dashboard-client-modules.test.js[79-84]
Skill: qa-testing

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The PR adds/retargets regression coverage by modifying existing test files, but the compliance rule requires regression tests to be added as new test files (e.g., `*.regression-NNN.test.*`) rather than editing existing ones.

## Issue Context
This PR retargets a regression test to the live navigation shell and updates bundle assertions, which constitutes adding regression coverage tied to `#540`.

## Fix Focus Areas
- tests/dashboard-e2e-96.test.js[365-390]
- tests/dashboard-client-modules.test.js[79-84]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

4. Regression test lacks attribution metadata 📜 Skill insight ⚙ Maintainability

Description

The updated regression test comment block does not include the required attribution metadata (date
found and QA report path), making the regression harder to audit and trace back to its origin.

Code

tests/dashboard-e2e-96.test.js[R354-365]

+//
+// #540: this test used to validate sidebarMarkup() — a flat one-nav-link-
+// per-page renderer with an approvals/alerts badge via badgeFor() — but
+// neither function had a caller anywhere in src/: the real navigation has
+// been the #278 destination shell (desktopNavigationMarkup +
+// navigationGroups) for a while. This test was silently exercising dead
+// code, giving false confidence that nav/container parity held against the
+// UI users actually see. Retargeted at the live nav function instead of
+// dropping the coverage; the badge-id assertions are gone with it since the
+// live nav never renders those ids (client.js's matching setBadge calls
+// were dead too — removed in the same cleanup, see #540).
+test('dashboard regression (#96): every live nav destination has a container and correct landmarks', () => {

Relevance

● Weak

Similar requests to add regression attribution metadata (found date/QA report) were repeatedly
rejected.

PR-#536
PR-#509
PR-#508

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 1400067 requires regression tests to include attribution comments containing issue
ID, what broke, date found, and QA report path. The new/updated comment block above the regression
test includes issue references and narrative, but does not include a found date or QA report path.

tests/dashboard-e2e-96.test.js[353-365]
Skill: qa-testing

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A regression test was modified/retargeted but the adjacent attribution comment block is missing required metadata (at minimum: date found and QA report path), which is required for traceability.

## Issue Context
The file contains a regression test and already has a narrative comment (`#540: ...`) explaining why the test was retargeted, but it does not include the standardized attribution fields required by the compliance checklist.

## Fix Focus Areas
- tests/dashboard-e2e-96.test.js[354-365]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

_{To customize comments, go to the Qodo configuration screen, or learn more in the docs.}

[richard-devbot]

Addressed both findings:

1. Shallow navIds.length > 0 — now asserts each of the six primary destinations is actually present. Mutation-tested against a real regression (a destination's only child demoted to hidden) — correctly caught.
2. Brittle HTML substring match — split into three independent, order-tolerant assertions.