Chore/health score improvements#74
Merged
Merged
Conversation
Move 138-line createMockProvider() from provider.ts into new mock-provider.ts. provider.ts drops from 324 to ~190 LOC with a single responsibility: provider factory and detection.
Delete server/ai/summarizer.ts (9-line re-export indirection) and update ai-routes.ts to import from task modules directly.
Split 498-line BranchSwitcher.tsx into CompareControls, BranchSelector, TagSelector, CommitShaInput, and dropdown-common (shared LoadingRow, ErrorRow, EmptyRow). BranchSwitcher.tsx becomes a thin orchestrator.
Split 455-line HeaderActionMenu.tsx into ViewModeActions (view tabs + workspace layout sections) and MobileBranchPanel (branch switch/compare sub-panel). HeaderActionMenu becomes the menu shell.
Add a step to the build job that computes total gzipped size of dist/assets/*.js and fails if > 500 KB. Upload bundle stats artifact.
- Add 5 smoke tests (home, repo, branch, AI explain, search pages) - Add playwright.config.ts with testMatch for *.e2e.ts files - Rename to *.e2e.ts to avoid bun test picking them up - Add test:e2e npm script - Add @playwright/test devDependency - Add e2e CI job with chromium install - Exclude e2e/ from bun test runner
- Add @axe-core/playwright + axe-core devDependencies - Create e2e/accessibility.e2e.ts — audits homepage and viz page for critical/serious violations - Add test:a11y and a11y:report scripts - Add a11y CI job (build → server → playwright a11y scan) - Fix playwright.config.ts webServer env for cross-platform (use env prop instead of inline KEY=VALUE)
Dependency ReviewThe following issues were found:
License Issues.github/workflows/ci.yml
OpenSSF Scorecard
Scanned Files
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
22 issues found across 25 files
Confidence score: 3/5
- The highest risk is in
e2e/repo-page.e2e.ts,e2e/ai-explain.e2e.ts,e2e/search.e2e.ts, ande2e/branch-switch.e2e.ts:if (count() > 0)guards skip assertions when elements are missing, so CI can pass even when key UI is broken. Merging as-is weakens regression detection for core repo-page flows — make these assertions fail explicitly when expected elements are absent before merging. e2e/ai-explain.e2e.tsalso usesbutton[title*="Sidebar"i], which can target a non-AI sidebar toggle and validate the wrong behavior path, creating false confidence in AI explain coverage. Tighten the locator to AI-specific attributes/text so the test verifies the intended feature.- In
src/components/viz/top-nav/ViewModeActions.tsx, duplicating mode definitions fromWorkspaceModeSelector.tsxand typing mode props as genericstringincreases drift risk and weakens compile-time checks, which can lead to inconsistent mode behavior after future edits. Consolidate a shared mode constant/type and remove casts (e.g., inHeaderActionMenu.tsx) before merging. - Accessibility gaps in
src/components/viz/top-nav/CommitShaInput.tsxandsrc/components/viz/top-nav/MobileBranchPanel.tsx(missing accessible names) may make branch/SHA inputs hard to discover for assistive-tech users. Addaria-labelor linked<label>now to avoid shipping avoidable usability regressions.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/components/viz/top-nav/CommitShaInput.tsx">
<violation number="1" location="src/components/viz/top-nav/CommitShaInput.tsx:19">
P2: The commit SHA field can be hard to identify for screen-reader users because the input has no accessible name. Adding an explicit `aria-label` (or a linked `<label>`) would keep this control discoverable without relying on placeholder text.</violation>
<violation number="2" location="src/components/viz/top-nav/CommitShaInput.tsx:36">
P2: Validation errors for invalid SHA may not be announced when they appear, which weakens keyboard/screen-reader feedback. Marking the error message as a live alert improves immediate error announcement.</violation>
</file>
<file name="src/components/viz/top-nav/MobileBranchPanel.tsx">
<violation number="1" location="src/components/viz/top-nav/MobileBranchPanel.tsx:41">
P2: Keyboard users may lose visible focus indication on the back control because the outline is removed without a custom focus style. Adding `focus-visible` ring/border styles would preserve navigability.</violation>
<violation number="2" location="src/components/viz/top-nav/MobileBranchPanel.tsx:53">
P2: Branch search can be hard to use with assistive tech because the input has no programmatic label. Adding an explicit accessible name (for example `aria-label`) would make the field discoverable and consistently announced.</violation>
</file>
<file name="e2e/repo-page.e2e.ts">
<violation number="1" location="e2e/repo-page.e2e.ts:14">
P2: The `expect(title).toBeTruthy()` assertion on line 14 only checks that the page title is not an empty string. It would pass for `"Error"`, `"404"`, or `"Loading…"`. For stronger regression coverage, follow the pattern already used in `home.e2e.ts` line 10 and assert specific title content with `await expect(page).toHaveTitle(/gitSdm/i)` or `expect(title).toMatch(/gitSdm/i)`.</violation>
<violation number="2" location="e2e/repo-page.e2e.ts:25">
P1: Test "file explorer toggle button exists" silently passes when no toggle button is found. The `if (count() > 0)` guard skips all assertions when zero matching elements exist, so a regression that removes or renames the explorer toggle would go undetected. Either assert existence unconditionally (`await expect(toggleButtons.first()).toBeVisible()`) or use a more resilient locator that always matches and drop the conditional.</violation>
</file>
<file name="src/components/viz/top-nav/ViewModeActions.tsx">
<violation number="1" location="src/components/viz/top-nav/ViewModeActions.tsx:5">
P2: `LAYOUT_MODES` duplicates the same workspace-mode definitions already defined in `WorkspaceModeSelector.tsx` (the `MODES` array). If a mode is added/renamed/removed, both files must be updated. Extract the shared config into `viewTabs.ts` (or a dedicated `workspaceModes.ts`) and import it in both components.</violation>
<violation number="2" location="src/components/viz/top-nav/ViewModeActions.tsx:13">
P2: `activeView` and `workspaceMode` props are typed as generic `string` instead of their known union of literal values. This forces callers to cast (see `HeaderActionMenu.tsx` lines 195-196) and loses compile-time safety — passing an invalid id won't error.</violation>
</file>
<file name="e2e/search.e2e.ts">
<violation number="1" location="e2e/search.e2e.ts:11">
P2: Conditional assertion pattern makes the test unreliable. The `if (await searchInput.count() > 0)` guard means the test silently passes when the search input is missing — a broken search page or rendering failure would go undetected. The `SearchBar` component renders its input unconditionally, so the conditional adds nothing while removing all regression-catching value. Apply the same direct-assertion pattern used in `home.e2e.ts` (e.g., `await expect(searchInput.first()).toBeVisible({ timeout: 15000 })`) to guarantee meaningful coverage.</violation>
</file>
<file name="bunfig.toml">
<violation number="1" location="bunfig.toml:2">
P2: The `[test] exclude` key is not a valid bunfig option — it's silently ignored. Use `pathIgnorePatterns` instead to actually exclude the e2e directory from test discovery. Currently doesn't cause failures since e2e files use `.e2e.ts` extensions (not in Bun's default discovery patterns), but the protection is absent for any future `.test.ts` files added there.</violation>
</file>
<file name="e2e/ai-explain.e2e.ts">
<violation number="1" location="e2e/ai-explain.e2e.ts:11">
P2: The locator's `button[title*="Sidebar"i]` term can match non-AI sidebar-toggle buttons on the repo page. If a generic sidebar toggle is found first, the test clicks it and looks for AI-related text in the resulting panel, producing a false positive or a confusing failure. Remove the `[title*="Sidebar"i]` term unless the AI button is specifically identified by that title.</violation>
<violation number="2" location="e2e/ai-explain.e2e.ts:12">
P1: Both tests can silently pass without asserting anything. In test 1: if `aiButton.count()` is 0, the `toBeVisible` assertion is skipped and the test passes. In test 2: the same happens at two levels — skipped button click and skipped panel assertion. This gives false confidence that AI-sidebar behavior is covered. Either make the locator unconditional with a longer timeout (Playwright auto-retries), or gate the entire describe block with a preliminary check so the test either runs fully or skips explicitly with `test.skip`.</violation>
<violation number="3" location="e2e/ai-explain.e2e.ts:26">
P3: Hardcoded `waitForTimeout(500)` is brittle — rendering can take >500ms on slower CI runners or <500ms, adding unnecessary delay. Replace with `waitForSelector` or a `toBeVisible` assertion on the AI panel to wait for exactly the right condition.</violation>
</file>
<file name="src/components/viz/top-nav/dropdown-common.tsx">
<violation number="1" location="src/components/viz/top-nav/dropdown-common.tsx:5">
P2: Add `role="status"` to the root `<div>` so screen readers announce the loading state when it appears dynamically. The spinner animation is purely visual without this.</violation>
<violation number="2" location="src/components/viz/top-nav/dropdown-common.tsx:14">
P2: Add `role="alert"` to the root `<div>` so screen readers announce the error message when it appears. This is a standard pattern for dynamic error states.</violation>
</file>
<file name=".github/workflows/ci.yml">
<violation number="1" location=".github/workflows/ci.yml:75">
P3: The bundle stats upload step references `path: stats.html`, but the visualizer outputs to `dist/stats.html` (as configured in `vite.config.ts`). This path mismatch means the upload either silently produces an empty artifact or logs a warning, depending on the upload-artifact version. Update the path to `dist/stats.html` to actually capture the generated stats file.</violation>
</file>
<file name="playwright.config.ts">
<violation number="1" location="playwright.config.ts:14">
P2: The E2E and A11y CI jobs build the app twice — once in explicit CI steps and once again when Playwright's webServer starts. The playwright.config.ts webServer command runs `bun run build && bun run build:server && bun start`, and since `reuseExistingServer` is `false` in CI, this full rebuild runs on every test invocation despite the CI steps already having built both bundles. Consider splitting the webServer command so that CI reuses the pre-built artifacts while local development keeps the full build chain. For example, use a conditional like `if [ "$CI" = "true" ]; then bun start; else bun run build && bun run build:server && bun start; fi`, or restructure the webServer command to start without rebuilding.</violation>
</file>
<file name="e2e/branch-switch.e2e.ts">
<violation number="1" location="e2e/branch-switch.e2e.ts:13">
P2: These conditional `if (await x.count() > 0)` wrappers around assertions make the smoke tests unable to detect regressions. When `BranchSwitcher`, `AISidebar`, search, or file-explorer UI elements are missing from the page, the tests run zero assertions and pass silently — the opposite of what a regression-detection suite should do. The test names clearly promise verification (e.g., "BranchSwitcher button is visible"), but the code skips all checks whenever the target element is absent. If the intent is to probe for optional elements, the test names and descriptions should reflect that behavior, and the tests should still document the absence so it's detectable. For features that are expected to be present (the branch switcher and AI sidebar appear to be core UI), use direct unconditional assertions so regressions can be caught.</violation>
<violation number="2" location="e2e/branch-switch.e2e.ts:30">
P2: These `waitForTimeout(500)` calls are timing-dependent waits that can cause flaky tests — too short on slow CI runners, unnecessarily long on fast machines. Prefer waiting for a concrete UI condition (e.g., dropdown visibility or panel render) with an `expect(locator).toBeVisible()` or `waitForSelector` assertion instead of a fixed delay.</violation>
</file>
<file name="src/components/viz/top-nav/HeaderActionMenu.tsx">
<violation number="1" location="src/components/viz/top-nav/HeaderActionMenu.tsx:110">
P2: The Search and Share buttons lost the `gap-1.5` spacing between their icon and label text. The old `desktopButtonClass` provided this spacing, but the replacement `mobileButtonClass` doesn't include it and no inline gap class is added at the usage sites. As a result the icon and text will render touching each other.</violation>
<violation number="2" location="src/components/viz/top-nav/HeaderActionMenu.tsx:129">
P3: The copied-state Check icon lost its entrance animation (`animate-in fade-in-0 scale-in-95 duration-100`). This animation provided a subtle scale-up + fade-in visual feedback when the link was copied. The PR description says these are behavior-preserving changes, but this is a visible UI difference.</violation>
<violation number="3" location="src/components/viz/top-nav/HeaderActionMenu.tsx:252">
P2: The new inline `HistoryIcon` and `StarIcon` SVG components are missing `aria-hidden="true"`. These icons are decorative (they accompany text like "commits" and "stars") so they should be hidden from assistive technology. The original lucide-react icons had this built-in, so this is a regression in accessibility.</violation>
</file>
Architecture diagram
sequenceDiagram
participant CI as CI Pipeline
participant Build as Build Step
participant BundleCheck as Bundle Size Check
participant Artifact as Upload Artifact
participant E2E as E2E Job
participant A11y as Accessibility Job
participant PW as Playwright Runner
participant Server as Dev Server (AI_PROVIDER=mock)
participant Browser as Headless Chromium
participant Axe as axe-core
participant AI as AI Provider (mock)
Note over CI,AI: NEW: CI pipeline additions
CI->>Build: Build app (all targets)
Build->>BundleCheck: Compute total gzipped JS size
BundleCheck->>BundleCheck: Sum dist/assets/*.js gzip sizes
alt Total > 500 KB
BundleCheck-->>CI: ❌ Fail build
else Total ≤ 500 KB
BundleCheck->>Artifact: Upload stats.html
BundleCheck-->>CI: ✅ Pass
end
CI->>E2E: Run E2E job
E2E->>PW: playwright test
PW->>Server: Start web server (AI_PROVIDER=mock)
Server->>AI: NEW: Load mock provider (via task imports, no summarizer shim)
PW->>Browser: Launch chromium
Browser->>Server: GET /, /mbayue/gitSdm, etc.
Server->>AI: Complete(messages)
AI-->>Server: Mock response
Server-->>Browser: HTML/JS
Browser-->>PW: Test results (5 smoke tests)
PW-->>E2E: Pass / Fail
CI->>A11y: Run Accessibility job
A11y->>PW: playwright test --grep "Accessibility audit"
PW->>Server: Start web server (AI_PROVIDER=mock)
PW->>Browser: Launch chromium
Browser->>Server: GET / (homepage)
Server-->>Browser: Page
Browser->>Axe: Analyze page
Axe-->>Browser: Violation list
Browser->>Server: GET /viz?url=… (viz page)
Server-->>Browser: Page
Browser->>Axe: Analyze page
Axe-->>Browser: Violation list
alt Critical or serious violations found
PW-->>A11y: ❌ Fail
else No critical/serious violations
PW-->>A11y: ✅ Pass
end
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
- Wrap ArchitectureView (Mermaid, ~600 KB gzipped) in React.lazy + Suspense - Wrap GraphCanvas (react-force-graph-2d, ~188 KB gzipped) in React.lazy + Suspense - Initial load dropped from 1.89 MB to 164 KB - Lazy chunks load on demand when user switches to graph/architecture tabs - Landing page HeroSection also lazy-loads GraphCanvas for the preview
Contributor
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/components/home/Trending.tsx">
<violation number="1" location="src/components/home/Trending.tsx:46">
P2: Each repository row now contains two focusable controls that perform the same action: the outer `<div>` (made interactive with `role="button"` and `tabIndex={0}`) and the inner `Button` component (which renders a native `<button>` and is focusable by default). This creates duplicate interactive elements per row, leading to confusing tab order and ambiguous controls for keyboard and screen-reader users. Consider making the inner `Button` non-focusable (e.g., `tabIndex={-1}`) if the outer row is meant to be the primary interactive surface, or restructure so only one element in the row is focusable.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| {data?.slice(0, 5).map((repo) => ( | ||
| <div | ||
| key={repo.fullName} | ||
| role="button" |
Contributor
There was a problem hiding this comment.
P2: Each repository row now contains two focusable controls that perform the same action: the outer <div> (made interactive with role="button" and tabIndex={0}) and the inner Button component (which renders a native <button> and is focusable by default). This creates duplicate interactive elements per row, leading to confusing tab order and ambiguous controls for keyboard and screen-reader users. Consider making the inner Button non-focusable (e.g., tabIndex={-1}) if the outer row is meant to be the primary interactive surface, or restructure so only one element in the row is focusable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/components/home/Trending.tsx, line 46:
<comment>Each repository row now contains two focusable controls that perform the same action: the outer `<div>` (made interactive with `role="button"` and `tabIndex={0}`) and the inner `Button` component (which renders a native `<button>` and is focusable by default). This creates duplicate interactive elements per row, leading to confusing tab order and ambiguous controls for keyboard and screen-reader users. Consider making the inner `Button` non-focusable (e.g., `tabIndex={-1}`) if the outer row is meant to be the primary interactive surface, or restructure so only one element in the row is focusable.</comment>
<file context>
@@ -43,13 +43,22 @@ export function Trending({ onSelect }: TrendingProps) {
{data?.slice(0, 5).map((repo) => (
<div
key={repo.fullName}
+ role="button"
+ tabIndex={0}
className="group flex flex-col sm:flex-row sm:items-center justify-between gap-4 sm:gap-6 p-4 bg-[#161b22] hover:bg-[#1c2128] transition-colors cursor-pointer"
</file context>
mbayue
force-pushed
the
chore/health-score-improvements
branch
from
edf2c12 to
15047b4
Compare
P2/P3 fixes: - CommitShaInput.tsx: add aria-label to input, role=alert to error - MobileBranchPanel.tsx: add focus-visible ring to back button, aria-label to search - dropdown-common.tsx: add role=status (loading), role=alert (error) - HeaderActionMenu.tsx: restore gap-1.5 spacing, add Check icon animation, aria-hidden on SVGs - ViewModeActions.tsx: extract shared WORKSPACE_MODES config, use literal union types - WorkspaceModeSelector.tsx: consume shared config, use WorkspaceModeId type - workspaceModes.ts: new shared config with deduplicated layout modes Config: - bunfig.toml: fix invalid exclude key -> pathIgnorePatterns - ci.yml: fix stats.html artifact path -> dist/stats.html - playwright.config.ts: skip build in CI webServer (prevents double build) E2E tests: - repo-page.e2e.ts: use toHaveTitle assertion, remove conditional guard - search.e2e.ts: remove both conditional guards - ai-explain.e2e.ts: remove generic Sidebar locator, remove conditional guards, replace waitForTimeout with proper assertions - branch-switch.e2e.ts: remove conditional guards, replace waitForTimeout
mbayue
force-pushed
the
chore/health-score-improvements
branch
from
24632fa to
974377d
Compare
Contributor
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="scripts/a11y.ts">
<violation number="1" location="scripts/a11y.ts:59">
P1: When Lighthouse exits non-zero, the script can still resolve successfully if a stale output file exists from a previous run. Because `outputPath` is deterministic and no cleanup happens before each run, a prior `test-results/lighthouse-${name}.json` will satisfy `existsSync(outputPath)` and bypass the error. The script will then read and report stale results, masking real accessibility failures in CI.
Consider deleting the output file before spawning Lighthouse, or tightening the exit-code check so a non-zero exit always rejects regardless of file presence.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| return new Promise((resolve, reject) => { | ||
| const proc = spawnCmd(cmd[0], [...cmd.slice(1), ...args], { stdio: 'pipe', env: { ...process.env, CHROME_PATH } }); | ||
| proc.on('close', (code) => { | ||
| if (code !== 0 && code !== null && !existsSync(outputPath)) { |
Contributor
There was a problem hiding this comment.
P1: When Lighthouse exits non-zero, the script can still resolve successfully if a stale output file exists from a previous run. Because outputPath is deterministic and no cleanup happens before each run, a prior test-results/lighthouse-${name}.json will satisfy existsSync(outputPath) and bypass the error. The script will then read and report stale results, masking real accessibility failures in CI.
Consider deleting the output file before spawning Lighthouse, or tightening the exit-code check so a non-zero exit always rejects regardless of file presence.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/a11y.ts, line 59:
<comment>When Lighthouse exits non-zero, the script can still resolve successfully if a stale output file exists from a previous run. Because `outputPath` is deterministic and no cleanup happens before each run, a prior `test-results/lighthouse-${name}.json` will satisfy `existsSync(outputPath)` and bypass the error. The script will then read and report stale results, masking real accessibility failures in CI.
Consider deleting the output file before spawning Lighthouse, or tightening the exit-code check so a non-zero exit always rejects regardless of file presence.</comment>
<file context>
@@ -0,0 +1,96 @@
+ return new Promise((resolve, reject) => {
+ const proc = spawnCmd(cmd[0], [...cmd.slice(1), ...args], { stdio: 'pipe', env: { ...process.env, CHROME_PATH } });
+ proc.on('close', (code) => {
+ if (code !== 0 && code !== null && !existsSync(outputPath)) {
+ reject(new Error(`Lighthouse exited with code ${code}`));
+ return;
</file context>
6e1c5b3 to
974377d
Compare
Contributor
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="scripts/a11y.ts">
<violation number="1" location="scripts/a11y.ts:15">
P1: The bundled Chrome discovery logic is Windows-only and will fail on the Linux CI runners configured for the accessibility job. Playwright stores browsers under `~/.cache/ms-playwright` on Linux and `~/Library/Caches/ms-playwright` on macOS, but `findBundledChrome()` only checks `~/AppData/Local/ms-playwright`. Because the CI workflow does not set `CHROME_PATH`, the `a11y` job on `ubuntu-latest` will throw `No bundled Chrome found.` and exit before running any audits. Consider expanding `findBundledChrome()` to check platform-appropriate cache directories (e.g. via `process.platform`), or alternatively use the `chromium` package from Playwright to resolve the executable path directly.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| } | ||
|
|
||
| function findBundledChrome(): string | undefined { | ||
| const browsersDir = join(homedir(), 'AppData', 'Local', 'ms-playwright'); |
Contributor
There was a problem hiding this comment.
P1: The bundled Chrome discovery logic is Windows-only and will fail on the Linux CI runners configured for the accessibility job. Playwright stores browsers under ~/.cache/ms-playwright on Linux and ~/Library/Caches/ms-playwright on macOS, but findBundledChrome() only checks ~/AppData/Local/ms-playwright. Because the CI workflow does not set CHROME_PATH, the a11y job on ubuntu-latest will throw No bundled Chrome found. and exit before running any audits. Consider expanding findBundledChrome() to check platform-appropriate cache directories (e.g. via process.platform), or alternatively use the chromium package from Playwright to resolve the executable path directly.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/a11y.ts, line 15:
<comment>The bundled Chrome discovery logic is Windows-only and will fail on the Linux CI runners configured for the accessibility job. Playwright stores browsers under `~/.cache/ms-playwright` on Linux and `~/Library/Caches/ms-playwright` on macOS, but `findBundledChrome()` only checks `~/AppData/Local/ms-playwright`. Because the CI workflow does not set `CHROME_PATH`, the `a11y` job on `ubuntu-latest` will throw `No bundled Chrome found.` and exit before running any audits. Consider expanding `findBundledChrome()` to check platform-appropriate cache directories (e.g. via `process.platform`), or alternatively use the `chromium` package from Playwright to resolve the executable path directly.</comment>
<file context>
@@ -0,0 +1,87 @@
+}
+
+function findBundledChrome(): string | undefined {
+ const browsersDir = join(homedir(), 'AppData', 'Local', 'ms-playwright');
+ if (!existsSync(browsersDir)) return undefined;
+
</file context>
739eaa1 to
974377d
Compare
Contributor
There was a problem hiding this comment.
1 issue found across 5 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/components/home/Trending.tsx">
<violation number="1" location="src/components/home/Trending.tsx:46">
P2: Each repository row now contains two focusable controls that perform the same action: the outer `<div>` (made interactive with `role="button"` and `tabIndex={0}`) and the inner `Button` component (which renders a native `<button>` and is focusable by default). This creates duplicate interactive elements per row, leading to confusing tab order and ambiguous controls for keyboard and screen-reader users. Consider making the inner `Button` non-focusable (e.g., `tabIndex={-1}`) if the outer row is meant to be the primary interactive surface, or restructure so only one element in the row is focusable.</violation>
</file>
<file name="scripts/a11y.ts">
<violation number="1" location="scripts/a11y.ts:15">
P1: The bundled Chrome discovery logic is Windows-only and will fail on the Linux CI runners configured for the accessibility job. Playwright stores browsers under `~/.cache/ms-playwright` on Linux and `~/Library/Caches/ms-playwright` on macOS, but `findBundledChrome()` only checks `~/AppData/Local/ms-playwright`. Because the CI workflow does not set `CHROME_PATH`, the `a11y` job on `ubuntu-latest` will throw `No bundled Chrome found.` and exit before running any audits. Consider expanding `findBundledChrome()` to check platform-appropriate cache directories (e.g. via `process.platform`), or alternatively use the `chromium` package from Playwright to resolve the executable path directly.</violation>
<violation number="2" location="scripts/a11y.ts:59">
P1: When Lighthouse exits non-zero, the script can still resolve successfully if a stale output file exists from a previous run. Because `outputPath` is deterministic and no cleanup happens before each run, a prior `test-results/lighthouse-${name}.json` will satisfy `existsSync(outputPath)` and bypass the error. The script will then read and report stale results, masking real accessibility failures in CI.
Consider deleting the output file before spawning Lighthouse, or tightening the exit-code check so a non-zero exit always rejects regardless of file presence.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR improves gitSdm's internal code health score from ~8.5 to 10/10 through a set of behavior-preserving structural improvements across five areas:
Types of Changes
Checklist
Summary
Raises the internal code health score to 10/10, cuts initial JS load by ~91% via lazy-loading heavy viz components, and tightens CI with a bundle-size gate. Behavior is unchanged; the app loads faster and CI is more stable.