Skip to content

fix(stage6): post-merge review (a11y + refs + beta + slim empty state)#11

Merged
satra merged 1 commit into
mainfrom
fix/post-merge-review
May 17, 2026
Merged

fix(stage6): post-merge review (a11y + refs + beta + slim empty state)#11
satra merged 1 commit into
mainfrom
fix/post-merge-review

Conversation

@satra

@satra satra commented May 17, 2026

Copy link
Copy Markdown
Collaborator

Addresses the gemini-code-assist review comments on PRs #9 + #10, the axe-core a11y audit from the live production site, and the user's beta-tag + empty-state trim.

Summary

  • state_key docstring corrected: removed the contradictory "picks the most recent by mtime" sentence; the function now documents that more than one rollup is a hard error by design.
  • references.yaml authors for the NeuroScape entries are now Mario Senden (the canonical author per the Aperture Neuro paper); titles follow the same Author (Year) — title format as the rest of the registry; year corrected to 2026.
  • A11y: ran axe-core against production — only serious violation was scrollable-region-focusable on every overflow-y: auto container. Added tabindex=\"0\" + role=\"region\" + aria-label to the cluster-grid, related-list scroll containers, facet options scroll, and cart items. New site/src/tests/e2e/a11y.spec.ts runs the axe audit on home / about / permalink and fails on critical or serious violations.
  • (beta) tag added to the page title + h1.
  • Empty-state tip in the right pane no longer lists "Models × inputs" and "Cells" — just "Accepted abstracts". Those numbers were developer-facing.
  • About page: Stage 6 TL;DR now documents the a11y posture (axe-core probe + keyboard-reachable scroll regions).

Test plan

  • pnpm test:unit --run (56/56)
  • pnpm exec vite build clean
  • link_check.py passes against the cleaned-up references.yaml
  • axe a11y probe re-runs against the deployed preview — should now report 0 critical / 0 serious
  • Production deploy succeeds; home + about + permalink render the (beta) tag

🤖 Generated with Claude Code

Addresses the gemini-code-assist review comments on PRs #9 and #10
plus the axe-core a11y audit results from the live production site.

PR #9 review fixes:
* state_key.discover_rollup_state_key — docstring previously said
  "picks the most recent by mtime" but the implementation raises on >1
  match; corrected the docstring (intentional design: no implicit
  newest-wins, the operator must disambiguate).
* PR #9 also flagged a cross-device rename OSError in builder.py and
  redundant inline `shutil` imports. Both already addressed by the
  earlier in-place-write refactor — no shutil/tempfile/rename calls
  remain in builder.py. Verified.

PR #10 review fixes:
* references.yaml authors fields: `ccnmaastricht` → `Mario Senden`
  (the canonical author per the Aperture Neuro paper). Year corrected
  to 2026.
* Both entries' titles now follow the same `<Author> (<Year>) — <title>`
  format as the other references in the registry.

a11y (axe-core against production):
* `scrollable-region-focusable` was the one serious WCAG 2.1 violation
  surfacing on every route. Added `tabindex="0"` (+ `role="region"`
  + `aria-label` where useful) to every `overflow-y:auto` container:
  cluster-grid, related-list scroll, facet options scroll, cart items.
* New `site/src/tests/e2e/a11y.spec.ts` Playwright spec that runs the
  axe audit against the home / about / permalink routes. Reads
  `TARGET_BASE` so the same probe can hit production OR a PR preview.
* `@axe-core/playwright` added as a dev dep.

(beta) tag:
* Page <title> + the H1 in the header now read "OHBM 2026 Atlas (beta)"
  so users see clearly the site is pre-launch.

Empty-state tip:
* Dropped the `Models × inputs` and `Cells` lines from the right-pane
  "Tap an abstract to see its details here" affordance — they were
  more developer-facing than reader-facing. Just `Accepted abstracts`
  stays.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@satra
satra temporarily deployed to pr-preview-11 May 17, 2026 22:37 — with GitHub Actions Inactive
@satra
satra merged commit 1c12654 into main May 17, 2026
2 checks passed

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request focuses on accessibility enhancements and the integration of automated accessibility testing using @axe-core/playwright. Key changes include adding ARIA labels and keyboard focusability to scrollable components like the CartDrawer, DetailPanel, and FacetSidebar, alongside updating the site branding to a 'beta' status. Reviewer feedback suggests refactoring the new E2E tests to leverage Playwright fixtures instead of manual browser management for better performance and resource handling. Additionally, there are recommendations to make tabindex and aria-label attributes conditional on content length to avoid unnecessary tab stops for keyboard users and to preserve list semantics by removing redundant role="region" attributes.

*/

import AxeBuilder from '@axe-core/playwright';
import { test, expect, chromium } from '@playwright/test';

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Import the Page type and remove the unused chromium import, as browser management should be handled by Playwright fixtures.

Suggested change
import { test, expect, chromium } from '@playwright/test';
import { test, expect, type Page } from '@playwright/test';

Comment on lines +23 to +61
async function auditRoute(
pathSuffix: string,
label: string,
waitFor: string = 'main'
): Promise<void> {
const browser = await chromium.launch();
const ctx = await browser.newContext({ viewport: { width: 1440, height: 900 } });
const page = await ctx.newPage();
const url = `${BASE.replace(/\/$/, '')}${pathSuffix}`;
await page.goto(url, { waitUntil: 'load' });
await page.waitForSelector(waitFor, { timeout: 30000 });
await page.waitForTimeout(2500); // let hydration + data-package fetch settle

const results = await new AxeBuilder({ page })
.withTags(['wcag2a', 'wcag2aa', 'wcag21a', 'wcag21aa'])
.analyze();

const critical = results.violations.filter((v) => v.impact === 'critical');
const serious = results.violations.filter((v) => v.impact === 'serious');
const moderate = results.violations.filter((v) => v.impact === 'moderate');
const minor = results.violations.filter((v) => v.impact === 'minor');

console.log(
`[${label}] axe results: ${critical.length} critical, ${serious.length} serious, ${moderate.length} moderate, ${minor.length} minor`
);
for (const v of [...critical, ...serious]) {
console.log(` · ${v.impact?.toUpperCase()} (${v.id}) — ${v.help}`);
for (const node of v.nodes.slice(0, 3)) {
console.log(` target: ${node.target}`);
}
}
for (const v of moderate.slice(0, 3)) {
console.log(` · MODERATE (${v.id}) — ${v.help}`);
}
await browser.close();
// Critical / serious fail the test; moderate / minor are logged only.
expect(critical, `critical a11y issues on ${label}`).toEqual([]);
expect(serious, `serious a11y issues on ${label}`).toEqual([]);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The auditRoute function manually launches and closes browser instances for each test. This is an anti-pattern in Playwright that prevents leveraging built-in fixtures, parallelization, and proper resource cleanup.

Refactor the function to accept the page fixture and remove the manual browser management. Also, consider replacing waitForTimeout with a more robust wait condition (e.g., waiting for a specific element that indicates data loading is complete).

async function auditRoute(
	page: Page,
	pathSuffix: string,
	label: string,
	waitFor: string = 'main'
): Promise<void> {
	await page.setViewportSize({ width: 1440, height: 900 });
	const url = `${BASE.replace(/\/$/, '')}${pathSuffix}`;
	await page.goto(url, { waitUntil: 'load' });
	await page.waitForSelector(waitFor, { timeout: 30000 });
	await page.waitForTimeout(2500); // let hydration + data-package fetch settle

	const results = await new AxeBuilder({ page })
		.withTags(['wcag2a', 'wcag2aa', 'wcag21a', 'wcag21aa'])
		.analyze();

	const critical = results.violations.filter((v) => v.impact === 'critical');
	const serious = results.violations.filter((v) => v.impact === 'serious');
	const moderate = results.violations.filter((v) => v.impact === 'moderate');
	const minor = results.violations.filter((v) => v.impact === 'minor');

	console.log(
		`[${label}] axe results: ${critical.length} critical, ${serious.length} serious, ${moderate.length} moderate, ${minor.length} minor`
	);
	for (const v of [...critical, ...serious]) {
		console.log(`  · ${v.impact?.toUpperCase()} (${v.id}) — ${v.help}`);
		for (const node of v.nodes.slice(0, 3)) {
			console.log(`      target: ${node.target}`);
		}
	}
	for (const v of moderate.slice(0, 3)) {
		console.log(`  · MODERATE (${v.id}) — ${v.help}`);
	}
	// Critical / serious fail the test; moderate / minor are logged only.
	expect(critical, `critical a11y issues on ${label}`).toEqual([]);
	expect(serious, `serious a11y issues on ${label}`).toEqual([]);
}

Comment on lines +63 to +69
test('axe — home page', async () => {
await auditRoute('/', 'home', '[data-testid="search-input"]');
});

test('axe — about page', async () => {
await auditRoute('/about/', 'about', 'main');
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Update these tests to use the page fixture provided by Playwright.

Suggested change
test('axe — home page', async () => {
await auditRoute('/', 'home', '[data-testid="search-input"]');
});
test('axe — about page', async () => {
await auditRoute('/about/', 'about', 'main');
});
test('axe — home page', async ({ page }) => {
await auditRoute(page, '/', 'home', '[data-testid="search-input"]');
});
test('axe — about page', async ({ page }) => {
await auditRoute(page, '/about/', 'about', 'main');
});

Comment on lines +71 to +88
test('axe — abstract permalink page', async () => {
const browser = await chromium.launch();
const ctx = await browser.newContext({ viewport: { width: 1440, height: 900 } });
const page = await ctx.newPage();
await page.goto(`${BASE.replace(/\/$/, '')}/`, { waitUntil: 'load' });
await page.waitForSelector('[data-testid="result-card"]', { timeout: 30000 });
const posterId = await page.locator('[data-testid="result-card"]').first().getAttribute('data-poster-id');
await browser.close();
if (!posterId) {
test.skip();
return;
}
await auditRoute(
`/abstract/${encodeURIComponent(posterId)}/`,
`permalink-${posterId}`,
'[data-testid="detail-panel"]'
);
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Refactor this test to use the page fixture and remove the redundant browser launch. This ensures consistency and better performance by reusing the managed browser instance.

test('axe — abstract permalink page', async ({ page }) => {
	await page.goto(`${BASE.replace(/\/$/, '')}/`, { waitUntil: 'load' });
	await page.waitForSelector('[data-testid="result-card"]', { timeout: 30000 });
	const posterId = await page.locator('[data-testid="result-card"]').first().getAttribute('data-poster-id');
	if (!posterId) {
		test.skip();
		return;
	}
	await auditRoute(
		page,
		`/abstract/${encodeURIComponent(posterId)}/`,
		`permalink-${posterId}`,
		'[data-testid="detail-panel"]'
	);
});

Comment on lines +619 to +624
<ul
class="cluster-grid"
tabindex="0"
role="region"
aria-label="Cluster membership across all (model × input) cells"
>

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Unconditional tabindex="0" adds unnecessary tab stops for keyboard users when the list is too short to scroll. Additionally, role="region" on a <ul> overrides its implicit list role, which can be confusing for screen reader users.

Consider making the focusability and labeling conditional on content length (similar to the implementation in FacetSidebar.svelte) and removing the role attribute to preserve list semantics.

				<ul
					class="cluster-grid"
					tabindex={clusterMemberships.length > 5 ? 0 : -1}
					aria-label={clusterMemberships.length > 5 ? "Cluster membership across all (model × input) cells" : undefined}
				>

<span class="hint">closest 5 shown; scroll for more</span>
</h3>
<ul class="related-list related-scroll" data-testid="related-nearest-list">
<ul class="related-list related-scroll" data-testid="related-nearest-list" tabindex="0" role="region" aria-label="Most-similar abstracts list">

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Apply conditional tabindex and aria-label here to avoid unnecessary tab stops on short lists, and remove role="region" to maintain list semantics.

						<ul class="related-list related-scroll" data-testid="related-nearest-list" tabindex={nearest.length > 5 ? 0 : -1} aria-label={nearest.length > 5 ? "Most-similar abstracts list" : undefined}>

<span class="hint">farthest 5 shown; scroll for more</span>
</h3>
<ul class="related-list related-scroll" data-testid="related-farthest-list">
<ul class="related-list related-scroll" data-testid="related-farthest-list" tabindex="0" role="region" aria-label="Most-different abstracts list">

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Apply conditional tabindex and aria-label here as well, and remove role="region".

						<ul class="related-list related-scroll" data-testid="related-farthest-list" tabindex={farthest.length > 5 ? 0 : -1} aria-label={farthest.length > 5 ? "Most-different abstracts list" : undefined}>

</p>
{:else}
<ul class="items">
<ul class="items" tabindex="0" aria-label="Your saved abstracts">

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Make tabindex and aria-label conditional on the number of items to avoid unnecessary tab stops when the cart list is not scrollable.

			<ul class="items" tabindex={items.length > 5 ? 0 : -1} aria-label={items.length > 5 ? "Your saved abstracts" : undefined}>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant