Skip to content

fix(discord-embed-ext): cap inline enlarged media to the VIEWPORT, declaratively - #1290

Open
ZacxDev wants to merge 2 commits into
mainfrom
feat/discord-embed-viewport-cap
Open

fix(discord-embed-ext): cap inline enlarged media to the VIEWPORT, declaratively#1290
ZacxDev wants to merge 2 commits into
mainfrom
feat/discord-embed-viewport-cap

Conversation

@ZacxDev

@ZacxDev ZacxDev commented Sep 4, 2026

Copy link
Copy Markdown
Member

The defect

applyOverride() removed Discord's 400×300 cap and left the media unbounded by two independent routes:

route mechanism
vertical max-height: none on the element. Unbounded by construction — a tall image ran off the bottom of the window on every enlarge, no resize required.
horizontal max-width: 100%, which resolves against the container whose own max-width this same function sets to none just above the write site. The effective bound was an ancestor chain that need not be viewport-bounded at all.

unclipAncestors() sets ancestors' overflow to visible, which is what lets the media spill rather than be clipped.

The fix

max-width: min(100%, 96vw) and max-height: 92vh, built by concatenation from two exported constants so the number appears exactly once.

No resize listener, deliberately. The engine re-evaluates vw/vh on resize itself — nothing to leak, no cached px value to go stale. It is also the only assertable form here: tests/fake_discord_dom.mjs is a hand-written fake DOM that models no viewport (no innerWidth/innerHeight, no getBoundingClientRect, no layout), so 92vh is directly assertable where window.innerHeight * 0.92 would not be.

Both halves of the width rule are load-bearing. 100% keeps media inside Discord's message column, which is far narrower than the window; 96vw keeps it inside the window when the column is not. Dropping either is a bug, in opposite directions — pinned separately.

Under 100 on purpose. vw includes the vertical scrollbar gutter. Measured in Brave/Chromium 144, headless over CDP, 1000px window with a scrolling document: innerWidth 1000 vs documentElement.clientWidth 985 — a 15px gutter. A 100vw box rendered 1000.00px and pushed scrollWidth past clientWidth, i.e. grew a horizontal scrollbar; 96vw rendered 960.00px and did not. A test named for the trap fails if anyone rounds it up.

vh, not dvh — a commented decision, not an omission. dvh tracks a retracting mobile toolbar and would relayout mid-scroll; identical on desktop Brave, the only place this runs.

Constant naming. ENLARGED_MAX_V{W,H} are deliberately not WIDTH_THRESHOLD/HEIGHT_THRESHOLD. Those are px numbers findContainer reads off an ancestor to recognise Discord's cap; these are viewport units this code writes. Same shape, opposite direction — a test pins that applyOverride never reaches for a detection threshold as a size.

Container vs element — element only

The container keeps max-width/max-height: none; width/height: auto and gets no cap of its own. With auto sizing it tracks its content, so bounding the media bounds the wrapper.

Measured, not reasoned: the exact four-declaration set around a 92vh-capped 200×2000 image gave wrapper 592.80px against image 588.80px — a 4px inline baseline gap, not an open box — inside a 640px viewport. Capping the container too would be redundant at best, and at worst re-imposes a constraint on Discord's own layout box that the uncapping just removed.

Accepted tradeoff

🔴 Enlarge now does less for portrait media: a tall image is capped to 92vh and letterboxed rather than rendering full height and running off screen. object-fit: contain was already set and the ratio is preserved (the 200×2000 image measured 58.88 × 588.80). This is the operator's explicit decision, documented in the README.

Test matrix — red at base, green at HEAD

tree sha result
base 2882d2c743069a28a2b9b58119bab97c2bb392fa 143 tests, 132 pass, 11 FAIL
HEAD e77cbcbd 143 tests, 143 pass, 0 fail

All 11 failed at base with their own assertion, not a neighbour's. 9 are the new guards; 2 are pre-existing tests that pinned the old 100%/none values, updated to read the exported constants rather than new hardcoded literals.

Coverage: both axes pinned separately · the width rule's 100% half pinned independently of its 96vw half · the cap surviving unclipAncestors() · the undo path restoring the exact prior overflow (value and priority, in both spellings) with no cap leaking onto an ancestor · idempotence proven by poisoning the declaration first so a missing early-return is observable rather than merely harmless · a reachable negative guard (an avatar on the same CDN, rejected only by the path prefix, with a positive control proving the harness can see a cap at all) · the 100vw trap · constants pinned two-way, behind a validated comment-stripper and a positive control.

Mutation sweep — 14 mutants

Each edit verified to have landed; a no-mutation control run confirming the harness can report GREEN; source restored by copy and checksum-verified after every run.

  • 13 KILLED by their own expected assertion.
  • 1 (hardcoding the literal at the write site) died on an earlier assertion in the same test — the no-second-literal scanner saw 96vw first — which left the structural "writes the constant" guard unreachable and therefore unproven. Closed with two behaviour-preserving mutants that inline the expression instead of the literal: byte-identical output, no literal for the scanner to find, so only the structural guard can see them. Both KILLED by it.

Gate — base 2882d2c743069a28a2b9b58119bab97c2bb392fa

Run on the final committed tree, both sandbox tiers one at a time (never concurrently):

tier command result
dev-host node node --test 'scripts/discord-embed-ext/tests/*.test.mjs' 143/143 pass
sandbox nodetests nix build .#checks.x86_64-linux.nodetests RESULT: PASS (exit=0) — discord-embed-ext 143/143 (floor 128); TOTAL 1458 pass / 0 fail across 5 suites
sandbox pytests nix build .#checks.x86_64-linux.pytests rc=0

The nodetests derivation hash for this tree (r8l8n530…) differs from the earlier run's (rpc2nd0x…), so its log is genuinely this tree's and not a stale re-read; the log contains the new test names, so the tier really ran them. dl-router's 537 tests also pass — its source_hygiene.test.mjs reads only scripts/dl-router/extension, so it cannot reach this extension's emoji.

⚠️ Not verified in the live browser

The rendering measurements above come from headless Brave/Chromium 144 driven over CDP against a synthetic page carrying the exact declaration setnot from the extension loaded in the operator's Brave against real Discord. The fake-DOM tests pin declarations only and observe no layout. manifest 0.3.00.3.1 so the reload can be confirmed; nothing in the repo asserted 0.3.0 (embed_enlarge.test.mjs:305 reads the manifest but asserts only matches, permissions, host_permissions).

🤖 Generated with Claude Code

https://claude.ai/code/session_01BePLrAzQrg983BX2FSdEjR

ZacxDev and others added 2 commits September 4, 2026 01:22
…claratively

applyOverride() removed Discord's 400x300 cap and left the media unbounded by
two independent routes:

  vertical    `max-height: none` on the element. Unbounded BY CONSTRUCTION — a
              tall image ran off the bottom of the window on every enlarge, no
              resize required.
  horizontal  `max-width: 100%`, which resolves against the container whose own
              max-width applyOverride sets to `none` just above the write site.
              The effective bound was an ancestor chain that need not be
              viewport-bounded at all.

Replaced with `max-width: min(100%, 96vw)` and `max-height: 92vh`, built by
concatenation from two exported constants so the number appears exactly once.

WHY CSS UNITS AND NO resize LISTENER
The engine re-evaluates vw/vh on resize itself: no listener to leak, no cached
px value to go stale, no teardown. It is also the only assertable form here —
tests/fake_discord_dom.mjs is a hand-written fake DOM that models NO viewport
(no innerWidth/innerHeight, no getBoundingClientRect, no layout), so `92vh` is
directly assertable where `window.innerHeight * 0.92` would not be.

BOTH HALVES OF THE WIDTH RULE ARE LOAD-BEARING
`100%` keeps media inside Discord's message column, which is far narrower than
the window; `96vw` keeps it inside the window when the column is not. Dropping
either is a bug, in opposite directions, and both are pinned separately.

UNDER 100 ON PURPOSE — `vw` INCLUDES THE SCROLLBAR GUTTER
Measured in Brave/Chromium 144, headless over CDP, 1000px window with a
scrolling document: innerWidth 1000 vs documentElement.clientWidth 985, a 15px
gutter. A `100vw` box rendered 1000.00px and pushed scrollWidth past
clientWidth — it grew a horizontal scrollbar; `96vw` rendered 960.00px and did
not. A test named for the trap fails if anyone rounds it up.
`vh` not `dvh` is a deliberate, commented decision: `dvh` tracks a retracting
mobile toolbar and would relayout mid-scroll; they are identical on desktop.

ELEMENT ONLY, NOT THE CONTAINER
The container keeps `max-width/max-height: none; width/height: auto` and gets no
cap of its own. With `auto` sizing it tracks its content, so bounding the media
bounds the wrapper. MEASURED rather than reasoned: the exact four-declaration
set around a 92vh-capped 200x2000 image gave wrapper 592.80px vs image 588.80px
— a 4px inline baseline gap, not an open box — inside a 640px viewport. Capping
it too would be redundant at best and re-constrain Discord's own layout box at
worst.

ACCEPTED TRADEOFF
Enlarge now does LESS for portrait media: a tall image is capped to 92vh and
letterboxed rather than rendering full height and running off screen.
object-fit: contain was already set and the ratio is preserved — the 200x2000
image measured 58.88 x 588.80. This is intended, per the operator's decision.

CONSTANT NAMING
ENLARGED_MAX_V{W,H} are deliberately NOT WIDTH_THRESHOLD/HEIGHT_THRESHOLD. Those
are px numbers findContainer READS off an ancestor to recognise Discord's cap;
these are viewport units this code WRITES. Same shape, opposite direction — a
test pins that applyOverride never reaches for a detection threshold as a size.

TEST MATRIX — red at base, green at HEAD
  base  2882d2c   143 tests, 132 pass, 11 FAIL
  HEAD  this commit                                143 tests, 143 pass,  0 fail
All 11 failed at base with THEIR OWN assertion, not a neighbour's. 9 are the new
guards; 2 are pre-existing tests that pinned the old `100%`/`none` values and
were updated to read the exported constants instead of new hardcoded literals.

Coverage: both axes pinned separately; the width rule's `100%` half pinned
independently of its `96vw` half; the cap surviving unclipAncestors; the undo
path restoring the exact prior overflow (value AND priority, in both spellings)
with no cap leaking onto an ancestor; idempotence proven by POISONING the
declaration first so a missing early-return is observable rather than merely
harmless; a reachable negative guard (an avatar on the same CDN, rejected only
by the path prefix, with a positive control proving the harness can see a cap at
all); the 100vw trap; and the constants pinned two-way — derived strings, the
write site reading the constant, and no second hardcoded viewport literal in
code, behind a validated comment-stripper and a positive control.

MUTATION SWEEP — 14 mutants, each edit verified to have landed, with a
no-mutation control run confirming the harness can report GREEN, and the source
restored by copy and checksum-verified after every run.
  13 KILLED by their own expected assertion.
  1 (hardcoding the literal at the write site) died on an EARLIER assertion in
  the same test — the no-second-literal scanner saw `96vw` first — which left
  the structural "writes the constant" guard unreachable and therefore unproven.
  Closed with two behaviour-preserving mutants that inline the EXPRESSION
  instead of the literal: byte-identical output, no literal for the scanner to
  find, so only the structural guard can see them. Both KILLED by it.

GATE (base 2882d2c)
  dev-host node tier   143/143 pass in scripts/discord-embed-ext/tests
  sandbox nodetests    nix build .#checks.x86_64-linux.nodetests -> RESULT: PASS
                       (discord-embed-ext 143/143, floor 128; TOTAL 1458 pass,
                       0 fail across 5 suites) — the log shows the new test
                       names, so the tier genuinely ran them
  sandbox pytests      built separately, never concurrently (a combined
                       invocation produces false failures from store contention)

NOT VERIFIED IN THE LIVE BROWSER. The rendering claims above come from headless
Brave/Chromium 144 driven over CDP against a synthetic page carrying the exact
declaration set — not from the extension loaded in the operator's Brave against
real Discord. manifest 0.3.0 -> 0.3.1 so the reload can be confirmed; nothing in
the repo asserted 0.3.0 (embed_enlarge.test.mjs:305 reads the manifest but
asserts only `matches`, `permissions` and `host_permissions`).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BePLrAzQrg983BX2FSdEjR
Claude-Session-Id: e13f042e-3be9-449d-a597-4b9a7ea7d259
…ves the ratio, and "viewport" is not the visible area

Adversarial audit returned zero 🔴 and safe-to-merge: the defect is real, the
fix is correct on both axes, the red/green matrix reproduced exactly (132/11 at
base, 143/143 at head, every failure on its own assertion), and an independent
19-mutant sweep killed all 19. Four prose defects, all corrected here and swept
at BOTH sites (the source comment and the README carried each claim).

F1. "object-fit: contain is what makes a capped portrait image letterbox" is
false in the case it cites. MEASURED in the same engine: a 200x2000 image under
these declarations renders 58.88 x 588.80 with `contain`, with `fill`, and with
the declaration deleted outright -- identical, because the box ratio already
equals the intrinsic ratio and the replaced-element auto/max-constraint
algorithm does the work. The declaration is still load-bearing where the box
ratio DIFFERS: in `display:flex; flex-direction:column` the box is
614.39 x 588.80 and `fill` squashes where `contain` letterboxes. So: keep it,
stop crediting it for the block case, and say which case needs it. Left as the
only thing standing between the declaration and a future "provably inert,
delete it" -- the structural declaration-set assertion.

F2. The "wrapper 592.80 vs image 588.80, a 4px inline baseline gap" figure does
not reproduce: re-measured at 588.80 vs 588.80, a 0px gap, in three
line-height contexts. The decision it supports -- the container tracks its
content, so no container cap is needed -- is unchanged and stronger. Corrected
rather than quietly dropped, since the old number is in the PR body too.

F3. "cap to the VIEWPORT" reads as "now fits on screen" and does not: `vh` is
the window, and Discord insets the scroller by a header and a composer.
MEASURED on a synthetic Discord-shaped layout (1200x800, 48px header, 68px
composer): visible scroller 684px, a 1000x3000 image capped to 736px -- still
scrolls. Enormously better than 2000px in a 640px viewport, but not the same
claim. Named the constant as the lever, and marked the measurement synthetic:
it is NOT from the live client.

F4. The README's "The last one is a heuristic" lost its referent when this PR
inserted a section between it and the list it pointed at, so it now reads as
naming the container-vs-element decision -- mis-identifying which mechanism is
the fragile one. Names the message-boundary walk explicitly.

node: 143 pass / 0 fail (unchanged -- comments and prose only).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BePLrAzQrg983BX2FSdEjR
Claude-Session-Id: e13f042e-3be9-449d-a597-4b9a7ea7d259
@ZacxDev

ZacxDev commented Sep 4, 2026

Copy link
Copy Markdown
Member Author

Adversarial audit round 1 — safe to merge, zero 🔴 — plus two corrections to the description above

Fixes landed in 4ba529b1. Both sandbox tiers green after them: nodetests RESULT: PASS (discord-embed-ext 143/143, total 1458/0 fail), pytests RESULT: PASS (21073 passed / 0 failed).

🔴 Two measurements in the PR body above are wrong. Correcting them here rather than silently editing the body, since anyone who has already read it took away the wrong numbers.

1. "wrapper 592.80px against image 588.80px — a 4px inline baseline gap." Does not reproduce. Re-measured at 588.80 vs 588.80 — a 0px gap, in three line-height contexts. The container tracks the media exactly. The decision that figure supports (no container cap needed) is unchanged and in fact stronger than the body claims.

2. "object-fit: contain was already set and the ratio is preserved." True as two facts, false as a causal claim — and the causal reading is what the sentence invites. Measured in the same engine, same image, same declarations: 58.88 × 588.80 with contain, with fill, and with the declaration deleted outright. Identical. In the block case the box ratio already equals the intrinsic ratio, so the replaced-element auto/max-constraint algorithm preserves it; object-fit is inert there.

Do not conclude the declaration is useless and delete it. It becomes load-bearing where the box ratio differs from the content's: display:flex; flex-direction:column gives a 614.39 × 588.80 box for the same image, and there fill squashes while contain letterboxes. No behavioural test in this suite can observe that — only the structural declaration-set assertion holds it.

Two further corrections, in the code and README

3. "Cap to the VIEWPORT" reads as "now fits on screen", and it does not. vh is the window; Discord insets the scroller by a channel header and a composer. Measured on a synthetic Discord-shaped layout (1200×800, 48px header, 68px composer): visible scroller 684px, a 1000×3000 image capped to 736px — still scrolls. Far better than the shipped max-height: none (2000px in a 640px viewport), but not the same claim. ENLARGED_MAX_VH is the single lever if fitting matters more than size. ⚠ Synthetic layout, not the live client.

4. The README's "The last one is a heuristic" lost its referent when this PR inserted a section between it and the list it pointed at, so it read as naming the container-vs-element decision — mis-identifying which mechanism is fragile. Now names the message-boundary walk.

Each claim was swept at both sites (source comment and README), not only where reported.

What the audit confirmed rather than took on trust

  • Red/green matrix exact: 143 tests, 132 pass / 11 fail at base 2882d2c7, 143/143 at head, and it extracted every failure's own error: line to confirm all 11 fail on an assertion inside their own body.
  • Mutation sweep re-run independently: 19 mutants, 19 killed, 0 survived, with a no-mutation control green and each mutation re-read from disk to prove it landed. The unreachable-guard gap the implementing agent self-reported was real, correctly diagnosed, and genuinely closed — the expression-inlining mutants die on the structural guard's own assertion.
  • Both of the interactions I was worried about do not exist. lightbox.js does not cloneNode — it builds a fresh element and copies only src, so the new inline !important caps cannot reach the lightbox. And the min() cliff is unreachable: manifest_version: 3 implies Chrome ≥88, min() shipped in Chrome 79. (The cliff's shape was confirmed — an unparseable setProperty is a silent no-op — it just cannot be reached here.)
  • Gated on the MERGED tree, not the branch: main had moved 11 commits past this PR's base but touched nothing under scripts/discord-embed-ext/ (verified by tree OID identity, with a positive control proving the pathspec works).

🔴 No round 2, and this is the escape hatch, not convergence

All four findings are false sentences: no 🔴, nothing touches an executable line, and the blast radius is bounded by "the document contains a false sentence". The recurring shape was swept at every site rather than only where reported. A round 2 would be a prose round auditing a prose fix to a prose defect — the non-terminating loop. Recorded so a later reader knows the new sentences are unaudited, not cleared.

⚠ Still true and unchanged: nothing here has been tested against the real Discord client with the extension loaded. Every rendering number, mine and the audit's, comes from a synthetic page carrying the declaration set.

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.

1 participant