Skip to content

Comments

Surface scanner version on security tab#18

Open
mgoldsborough wants to merge 1 commit intomainfrom
feat/surface-scanner-version
Open

Surface scanner version on security tab#18
mgoldsborough wants to merge 1 commit intomainfrom
feat/surface-scanner-version

Conversation

@mgoldsborough
Copy link
Contributor

Summary

  • Extract scanner_version from the scan report's scan metadata in the API transform layer
  • Add scanner_version field to all SecurityScan Zod schemas (source + generated copies)
  • Display scanner version in the security report subtitle line (e.g. "Scanner v0.4.2")
  • Only renders when present, so older scans without it display unchanged

Test plan

  • Verify typecheck/lint pass (pnpm typecheck && pnpm lint)
  • Check a bundle with a completed scan — scanner version should appear after the scan date
  • Check a bundle scanned before this field existed — should render without the scanner version gracefully

Extract scanner_version from the scan report metadata in the API
response and display it on the security tab next to the scan date.
@mgoldsborough mgoldsborough requested review from a team and JoeCardoso13 February 21, 2026 02:33
Copy link
Collaborator

@JoeCardoso13 JoeCardoso13 left a comment

Choose a reason for hiding this comment

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

Findings

All low-severity — none are blocking. Most are pre-existing patterns rather than problems introduced by this PR.

1. Duplicated "generated" schemas are manually maintained (Low / Observation)

The schemas in apps/registry/src/schemas/generated/api-responses.ts and apps/web/src/schemas/generated/api-responses.ts appear to be copies of packages/schemas/src/api-responses.ts. The directory name generated/ implies these should be auto-generated, but this PR updates them by hand. If there is a codegen step, it should be run instead; if there isn't, the generated/ naming is misleading and risks drift over time.

Suggested improvement (optional): If these are auto-generated, both the registry and web apps should import from or generate against the single source of truth in packages/schemas/. If they're not auto-generated, rename the generated/ directories to something like shared/ or copied/ to avoid the misleading implication and reduce surprise for future contributors.

2. Type assertion on report.scan — no runtime validation (Low)

const scanMeta = report?.['scan'] as Record<string, unknown> | undefined;
const scannerVersion = (scanMeta?.['scanner_version'] as string) ?? null;

This uses as casts to extract the value. If scanner_version is present but not a string (e.g. a number), it would silently pass through and could produce unexpected UI output like "Scanner v[object Object]". Consistent with the existing pattern used for other fields in this file, but a typeof guard would be more robust.

Suggested hardening (optional):

const raw = scanMeta?.['scanner_version'];
const scannerVersion = typeof raw === 'string' ? raw : null;

3. Manual SecurityScan interface in the component duplicates the Zod schema type (Low / Observation)

SecurityReportSection.tsx defines its own SecurityScan TypeScript interface rather than deriving it from the Zod schema via z.infer<typeof SecurityScanSchema>. This means every schema change requires a parallel manual edit to the interface. Consistent with existing code, but worth noting.

Suggested improvement (optional): Replace the hand-written interface with z.infer<typeof SecurityScanSchema> (or a pick/partial of it) so the component stays in sync with the schema automatically. That would have made this PR one fewer file to touch.

4. No unit test for the new field extraction (Low)

There's no test covering the new extraction logic (what happens when report.scan.scanner_version is present, missing, or a non-string value). The existing test suite passes, but there's no coverage of this specific path.

Suggested improvement (optional): Add a small test case in the packages route tests that asserts scanner_version is surfaced when present in the report metadata and falls back to null when absent. This would also serve as a safety net for the type-assertion concern in item #2.

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.

2 participants