Skip to content

Delete FullReportOnly: let selfContainedReportStrip alone control what self-contained reports render #143

Description

@Romex91

FullReportOnly is a second, parallel mechanism for the exact thing selfContainedReportStrip already does: decide what a self-contained report may show. The strip removes artifact-path fields from the measurement; FullReportOnly then separately hides the UI that would have rendered them. Two sources of truth for one decision, and they must be kept in sync by hand — a stage that adds a stripped field but forgets the <FullReportOnly> wrapper (or vice versa) silently renders a dead link or drops content that was still present.

Proposal: delete FullReportOnly and let presence of the field in the measurement be the sole control. Stripped ⇒ absent ⇒ not rendered. The strip dictionary on the stage stays the single declaration of what self-contained reports carry.

Why this works today

Every current FullReportOnly site gates UI whose data is already in that stage's strip dictionary, and most sites already do the presence check — the wrapper is pure redundancy:

Site Gated field(s) Stripped by
src/compare/stages/shared/perf-report.tsx:191 benchReportHref, control/experimentLighthouseHref, timelineHref, diffHrefs PerfEngineStage (src/compare/stages/perf/stage.ts:53)
src/audit/stages/audit/report.tsx:101 lighthouseHref AuditStage (src/audit/stages/audit/stage.ts:53)
src/audit/stages/accessibility/report-preview.tsx:101 rawArtifactHref AccessibilityStage (src/audit/stages/accessibility/stage.ts:34)
src/compare/stages/accessibility/report.tsx:550, 726 comparisonArtifactHref, control/experiment.rawArtifactHref (via RawLinks) AccessibilityCompareStage (src/compare/stages/accessibility/stage.ts:34)
src/compare/stages/accessibility/report.tsx:1034 sideRawArtifactHref(...) same

ArtifactLinks and RawLinks both already return null when their link list is empty; the audit/a11y sites already sit behind a href ? … : null. Dropping the wrapper changes nothing about the rendered output.

Scope

  1. Remove the 6 <FullReportOnly> wrappers above, keeping/adding the plain presence check where it isn't already there.
  2. src/audit/stages/build_annotated_timeline/report.tsx:39,64 — same class of thing via the hook: screencastHref={mode === 'self-contained' ? undefined : row.timeline.screencastHref}. screencastHref is already in that stage's strip (stage.ts:129), so this becomes screencastHref={row.timeline.screencastHref} and useReportMode() goes away.
  3. Delete FullReportOnly from src/pipeline/report-mode.tsx. With (1)+(2) done, useReportMode / ReportModeContext / ReportModeProvider have no consumers either — delete them and drop the provider from report-shell/src/main.tsx:30. Keep the ReportMode type and meta.reportMode (used by reportDataForMode and asserted in the bisect tests) as metadata about which file this is.
  4. Type check: each stripped key must be optional on its measurement interface so the presence check compiles and so applySelfContainedReportStrip removing it isn't a lie about the type. Verify for all of: lighthouseHref, lighthouseThumbHref, coverageStatementIdsHref, controlLighthouseHref, experimentLighthouseHref, timelineHref, benchReportHref, diffHrefs, rawArtifactHref, comparisonArtifactHref, screencastHref.
  5. Update the review-architecture skill (.claude/skills/review-architecture/SKILL.md:186,256) so the checklist says the strip dictionary is the only control and there is no render-mode gate to keep in sync.

Risks / things to check

  • reportDataForMode no-ops the strip when stages is empty (src/pipeline/report.ts:326shouldStrip requires selfContainedStrips.size > 0), and writeReport's stages param defaults to []. Today FullReportOnly is a backstop for that path; after this change a self-contained report built without stages would render live artifact links. Both real callers pass stages (src/pipeline/runner.ts:789, src/compare/bisect/report.ts:44), so the fix is to make stages required on writeReport (or fail loudly when it's empty) as part of this change.
  • Sites that consume a stripped field outside a FullReportOnly need a look while we're in there — e.g. src/compare/stages/accessibility/report.tsx:~554 passes href={result.comparisonArtifactHref ?? '#'} to DetailedArtifactDialog, i.e. a self-contained report gets an '#' href. Not a regression from this change, but presence-based rendering is the moment to decide what that dialog should do when the artifact isn't there.
  • Tests referencing the removed component / mode gate: src/pipeline/__tests__/report.test.ts, test/audit/accessibility-report-ui_spec.ts, test/compare/bisect-report-ui_spec.ts. The UI specs should keep asserting "no raw-artifact links in self-contained output" — that assertion is the real regression guard once the wrapper is gone.

Acceptance

  • No FullReportOnly / useReportMode left in the tree.
  • Self-contained and full report HTML byte-compare equivalent to before the change for a fixture run (same links present/absent).
  • A stage can now control self-contained rendering by editing only its selfContainedReportStrip.

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions