Sign up
View stargazersSign up

A scrolling capture that fails is stored as a static capture with no record of the fallback#1667

opened27d agobyMMatt Donovan

Defect: a scrolling capture that fails is stored as a static capture, and nothing records the change.

What: When the operator asks for a scrolling capture and the scroll step fails, the browser extension quietly takes a static screenshot instead and reports success. The signed record of the capture does not say which kind was asked for, which kind was taken, or that a fallback happened. A reader of the evidence cannot tell whether content that loads on scroll was given the chance to load. The same quiet fallback exists one level down, from a whole-page screenshot to the visible part of the page only.

Why it was not fixed where it was found: it turned up in a design session that changed no code. The design agreed in that session replaces this code, but only in its third step, so released builds keep the gap until then.

Where it is and how to reproduce it

Where: extension/src/background.ts:133 (captureScrollingPageScreenshot, fallbacks at lines 147 and 155), extension/src/background.ts:108 (captureFullPageScreenshot, fallbacks at lines 121 and 128), and extension/src/background.ts:805 (the sendMhtmlCapture payload, which carries no field for the requested mode or the fallback). Line numbers are at 30699111. No field for a scroll phase exists in src/shared/types.ts, src/main/services/manifest.ts or src/main/services/captureLifecycle.ts.

Reproduce: Not run. Established by reading the code: choose "Capture Full Page (Scrolling)" on a page where the content script's scroll phase returns an error or throws. The only trace is a console.warn in the service worker; the capture uploads and the toast reports success.

The background renderer has the matching gap: scrollToLoadLazyContent in src/main/services/backgroundRenderer.ts returns nothing, so a background Capture does not record whether its scroll phase ran or how it ended.

The agreed fix is in docs/specs/2026-09-30-capture-engine-design.md and ADR-0034 on the branch drafts/2026-09-29-capture-engine-glossary: Scroll-to-load is recorded in the Manifest Entry with its outcome, and a failed Scroll-to-load never silently yields a static Capture.

MMatt Donovan22d ago

Note

Claude responding on behalf of Matt.

This was generated by AI during triage.

Agent Brief

Category: bug, evidence-affecting Summary: When a scrolling or whole-page screenshot fails and the extension takes a lesser one instead, the investigator is told which kind they got.

Triage confirmed the defect from the code on 2026-10-04. A failed scrolling screenshot falls back to a whole-page one, and a failed whole-page one falls back to the visible part of the page. Each fallback writes a warning the investigator never sees, and the Capture reports plain success.

The maintainer's ruling: fix the investigator-facing half now. The signed record is not changed here. The capture engine design already defines how the record states the scroll outcome, and an interim field would be replaced by it.

Current and desired behavior, interfaces, acceptance criteria, verification and scope

Current behavior (read at origin/main 43906e63, not run)

  • captureScrollingPageScreenshot in the extension background script returns the result of captureFullPageScreenshot when the content script answers with an error or the message throws. captureFullPageScreenshot does the same with captureScreenshot (viewport). Each path only calls console.warn.
  • The manual capture flow builds its UPDATE_CAPTURE_TOAST outcome from the upload result alone: degraded with "Captured (screenshot too large)" when the server dropped the screenshot, otherwise success with no message.
  • A CaptureUiSuppressionError is rethrown, not downgraded. That stays.

Desired behavior

  • The screenshot helpers report which kind of screenshot was produced (scrolling, full page, viewport, or none) alongside the image, so the caller can compare it with the kind requested.
  • When the produced kind is lesser than the requested kind, the capture outcome toast uses the existing degraded status with a message that names what was requested and what was taken, in the register of the existing "Captured (screenshot too large)" text.
  • When both a fallback and a dropped screenshot apply, the dropped-screenshot message wins, because no screenshot was stored.
  • Every caller of the two helpers surfaces a fallback the way that caller already surfaces outcomes (toast or inline bar). A caller with no investigator-facing outcome is left alone and named in the PR.

Key interfaces

  • captureScrollingPageScreenshot, captureFullPageScreenshot: return type gains the produced kind. All call sites updated.
  • The UPDATE_CAPTURE_TOAST message: no new status; degraded and message already exist.
  • The upload payload and everything on the app side: unchanged.

Acceptance criteria

  • A scrolling capture whose content script returns an error completes, and the outcome is degraded with a message saying a scrolling capture was requested and a full-page screenshot was taken.
  • A scrolling capture whose message throws behaves the same.
  • A full-page capture that falls back to the viewport is degraded with a message saying only the visible part of the page was captured.
  • A scrolling capture that falls back twice reports the viewport outcome.
  • A capture with no fallback is success with no message, as today.
  • A dropped screenshot still reports "Captured (screenshot too large)".
  • A CaptureUiSuppressionError still fails the capture and is never reported as a fallback.
  • The bytes uploaded and the request body sent to the app are unchanged for every case above.

Verification

pnpm preflight on Node 20, which runs pnpm build:extension because extension/ changes. New tests go beside the existing extension background tests in tests/extension/.

Starting files

  • extension/src/background.ts (captureScrollingPageScreenshot, captureFullPageScreenshot, their call sites, sendCaptureOutcomeToast)
  • tests/extension/manualCaptureConcurrency.test.ts and tests/extension/backgroundScreenshotSuppression.test.ts for the harness

Evidence-affecting: yes, by the maintainer's call on 2026-10-04. The label is on the issue and the PR carries it.

Out of scope

  • Recording the requested kind, the produced kind or the fallback in the upload payload, the Manifest Entry or the database. This issue stays open for that half, or is closed in favor of the capture engine work, at the maintainer's choice when the PR merges; the PR says "Refs", not "Closes".
  • scrollToLoadLazyContent in the background renderer, which the capture engine's first slice covers.
  • Failing the capture outright instead of falling back.
Bbirdbrain-agent19d ago
Synced from GitHub

Cycle claim: PR #1767

Claiming the automated review of the latest commit on this pull request.

Bbirdbrain-agent19d ago
Synced from GitHub

The work this issue asked an agent to do has already merged, so I took it off the agent queue instead of starting a new run on it.

The merged change tells the investigator when a scrolling or whole-page screenshot falls back to a lesser one. I checked each acceptance criterion against the current code and its tests, and all eight are met. The signed-record half stays out of scope, as the brief says, so the issue stays open for the maintainer to keep or close in favor of the capture engine work.

I moved the issue from ready for an agent to ready for a human. The queued label is the maintainer's to remove.

What merged and what I checked
  • Merged: #1767 at 2026-10-08T19:50:19Z, body "refs #1667", so the issue did not auto-close and kept ready-for-agent and queued.
  • Checked at origin/main 7c2f755b, file extension/src/background.ts: ScreenshotKind, screenshotFallbackNote, both helpers returning { blob, kind }, the dropped-screenshot message outranking the fallback note, and CaptureUiSuppressionError rethrown on both paths.
  • Tests: tests/extension/backgroundScreenshotSuppression.test.ts, block "a screenshot fallback is reported to the operator (#1667)": no-fallback success, scrolling error and throw, full-page to viewport, double fallback, every path failed, dropped screenshot wins, suppression failure fails the capture. Each case asserts the uploaded screenshot and upload fields.
  • Labels: removed ready-for-agent, added ready-for-human. Left queued in place.
MMatt Donovan17d ago

Note

Claude responding on behalf of Matt.

How this issue closes, ruled by the maintainer on 2026-10-10.

The app-side half: a capture the app takes in the background does not record whether its scroll phase ran or how it ended. That half closes this issue when the capture engine step that records the outcome on each such capture merges. The step is planned in the capture engine's first slice, which is under review, and its pull request should say that it closes this issue.

The extension half: the signed record does not say that a scrolling capture fell back to a lesser screenshot. It will not be fixed in the extension. A later stage of the capture work moves capture out of the extension, and that design replaces this code. The warning the investigator now sees on a fallback, merged on 2026-10-08, stays until then.

The issue stays open until the app-side step merges.

Ruling and the planned step
  • Ruling 24 (2026-10-10): #1667 closes when CE-8 records its background half; its extension half closes in favor of CE-S3.
  • The step: CE-8's first part, step 6 of the first-slice plan (draft PR #1803, "Scroll-to-load and the screenshot record"). scrollToLoadLazyContent returns scrollToLoad: { requested, ran, outcome?, endHeight?, scrollLockLifted } with outcomes nothing-to-scroll, height-cap, stopped-growing and time-budget (plan D5), and ingest writes it to the Manifest entry and the row. The plan notes the background renderer already fails the Capture when a scroll step throws, so no build in the first slice writes a failed outcome.
  • The extension half moves to the stage 3 design (CE-S3 in the roadmap, draft PR #1802), which deletes the extension's screenshot code.
  • Investigator-facing half: #1767, merged 2026-10-08, refs #1667.
  • Labels unchanged: ready-for-human.