fix: forward cookies on local-discovery path (dropped for auth-walled URLs)#526
Open
shrinishLT wants to merge 1 commit into
Open
fix: forward cookies on local-discovery path (dropped for auth-walled URLs)#526shrinishLT wants to merge 1 commit into
shrinishLT wants to merge 1 commit into
Conversation
processSnapshot.ts has two parallel functions producing processedSnapshot
payloads. prepareSnapshot (used for remote discovery) includes the
cookies field; processSnapshot (the default export, used for local
discovery — the most common path) silently drops it.
Symptom for customers whose snapshot URL is auth-walled (e.g. Veeva
Vault): the CLI's discovery Playwright navigates to the snapshot URL
unauthenticated, fails to fetch resources behind the customer's session
(page images, CDN-hosted CSS/JS), and dotui receives a payload with no
styling/scripts/images cached. The rendered screenshot ends up blank or
unstyled.
Reproduction with two captured Vault DOMs (same customer, similar test):
Old (had cookies):
'cookies' field present, 384 chars
11 makepageimage PNGs cached → page rendered correctly
New (cookies dropped on local discovery):
'cookies' field absent
0 makepageimage PNGs cached → page renders blank in dotui
Fix: the default-export processSnapshot's return now mirrors
prepareSnapshot's — emits `cookies: Buffer.from(snapshot.dom.cookies
?? '').toString('base64')` alongside the existing payload fields.
This is the third instance of the dual-function divergence in this
file; worth a refactor to share the payload-construction code, but
out of scope for a one-line correctness fix.
Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
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
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.
Summary
processSnapshot.tshas two parallel functions emitting theprocessedSnapshotpayload:cookiesfieldprepareSnapshot(line 44)USE_REMOTE_DISCOVERY/config.useRemoteDiscovery)processSnapshot(default export, line 288)This PR adds the missing
cookiesfield to the local-discovery return so the two paths emit the same payload shape.Why this matters
The CLI's discovery flow navigates a separate Playwright instance to
snapshot.urlto fetch and cache page resources. For customers whose URL is behind authentication (Veeva Vault, internal apps with SSO), the discovery browser needs the session cookies captured by the SDK to authenticate.With local discovery, those cookies are silently dropped from the payload. The discovery Playwright opens the URL unauthenticated, fails to fetch:
/ui/annotate/makepageimage?n=NPDF page images)static-assets.veevavault.comCSS/JS bundles)…and the snapshot uploaded to dotui has zero useful CSS/JS/images. The rendered screenshot is blank or grossly mis-styled.
Reproduction with real customer DOMs
Two captured Veeva Vault payloads from the same customer's test suite:
[..., 'cookies', ...]['name','url','dom','resources','options']cookiesfield lengthIS_SELENIUM=true; veevaUserSsoSettings=…; VAULT_DATE_FORMAT_TYPE=…)makepageimagePNGs inresourcesSame customer, same test pattern — the only thing that changed in the payload shape was the disappearance of cookies. Traced back to this dual-function divergence.
The fix
return { processedSnapshot: { name: snapshot.name, url: snapshot.url, dom: Buffer.from(snapshot.dom.html).toString('base64'), resources: cache, - options: processedOptions + options: processedOptions, + cookies: Buffer.from(snapshot.dom.cookies ?? '').toString('base64'), }, warnings: [...optionWarnings, ...snapshot.dom.warnings], discoveryErrors: discoveryErrors }Identical pattern to the existing
prepareSnapshotreturn at line 281 — just brings the local-discovery path to parity.Scope / risk
cookiesfield on the payload (the remote-discovery path has been emitting it). For customers whosesnapshot.dom.cookieswas empty/undefined before,Buffer.from('').toString('base64')='', so they get an emptycookiesfield — same effective behavior as before.Test plan
pnpm run build).cookiesfield and the page renders with proper styling in dotui.Note on the dual-function divergence
This is the third bug caused by
prepareSnapshotandprocessSnapshotdiverging in this file. The first two (custom-scroll flag forwarding + the user-facing warning) were addressed in PR #522. Worth a follow-up refactor to share the payload-construction code between the two paths, but kept out of this PR for tightness.🤖 Generated with Claude Code