Skip to content

fix(shell): bound the authenticated content to the shell's second row, and its panes to that box (TASK-157) - #1871

Merged
lilyshen0722 merged 2 commits into
mainfrom
fix/task-157-shell-height
Sep 25, 2026
Merged

lilyshen0722 merged 2 commits into
mainfrom
fix/task-157-shell-height

Conversation

@lilyshen0722

Copy link
Copy Markdown
Contributor

Two rules, from ux-lead's browser measurements at 390 and 1200 on main (9a32fca5). Both are cascade outcomes, so both are pinned by a cascade-aware guard rather than by presence.

Desktop — the content div never took the 1fr row. .v2-authenticated-shell is grid-template-rows: auto minmax(0, 1fr). Row 1 is the verify-email banner's auto row, and a verified user has no banner, so .v2-authenticated-shell__content was the first grid child and auto-placement put it in that row. Any page shorter than the viewport then sized the inset card to its own content: 195px above the bottom gutter on Agents, 178px on Connectors (ux-lead, 1200x900), against the ruled 14. Fixed by grid-row: 2 on the content div — row 1 collapses to 0 with nothing in it, which is what the banner variant already produced.

Phones — the pane was 100vh inside a box that had already subtracted the banner. The TASK-129 block sets .v2-shell .v2-pane { height: 100vh } at the end of the sheet, and it beat #1510's .v2-authenticated-shell .v2-pane--main { height: 100% } on file order at equal (0,2,0) specificity. With the banner up (111px) the pane ran 111-955 in a 844 viewport; the pane's own overflow: hidden then parked the pod header under the banner — back button, title and inspector toggle all untappable, and nothing to scroll back — while Connectors lost its last 54px and Settings its last 48px. Fixed with a <=760 rule scoped .v2-authenticated-shell__content .v2-shell .v2-pane { height: 100% } at (0,3,0), so it wins on specificity rather than on being written later. ux-lead offered deleting the 100vh rule as an equivalent fix; I kept it, because it is the right height outside this shell and the narrower change is the safer one.

Why the guard is shaped the way it is. v2-layout-invariants.test.ts gains a small cascade parser: rules with their at-rule context, specificity and document order, plus a max-width/min-width applicability test. The four new cases then assert on the winner of a scoped chain —

  • .v2-authenticated-shell__content declares grid-row: 2 as the winner;
  • at 390, every pane selector in the main pane's family that states a viewport height is outranked by a height: 100% rule, and the winner is the bounded rule;
  • that bounded rule applies at 390 and not at 1200 (where the pane must keep its 16px gutter);
  • the selector chain it walks is the one the components actually render (V2App -> V2Layout/V2FeaturePage -> V2Thread).

Presence cannot see either defect: not.toContain('100vh') passes while a later duplicate repaints the phone, and toContain('grid-row: 2') passes while a later rule resets it to auto. Non-vacuity is asserted in both directions — at least one viewport-height pane rule must still be found, and at least one bounding rule — so the filters cannot pass on an empty scan, which is the .v2-tools/ruleBody trap from #1869.

Mutations — seven, each applied alone, each red on the expected assertion, every restore verified at 0 differing hunk-lines against the commit:

# mutation result
m1 bounded pane rule deleted 2 red (phone winner, phone-scope)
m2 it states 100vh 1 red (phone)
m3 a later (0,3,0) 100vh added at EOF 1 red (phone)
m4 grid-row: 2 removed 1 red (grid row)
m5 a later duplicate resets grid-row: auto 1 red (grid row)
m6 bounded rule hoisted out of the media block 1 red (phone-scope)
m7 bounded rule loses its shell scope ((0,2,0), wins on order) 2 red — deliberate, see below

Stated limits. jsdom has no layout engine, so what is pinned here is the cascade, never the rendered box; ux-lead's 390/1200 read is the other half. The parser models simple selectors, max-width/min-width, and the height/grid-row declarations asserted on; it does not resolve percentages against containing blocks, does not model !important, and treats unmodelled media features (hover, prefers-reduced-motion) as applying — the conservative direction for a guard whose job is to catch a rule that wins. m7 is the one intentional over-strictness: the guard requires the specificity-scoped form, not merely a rule that happens to win, because a same-specificity rule winning on order is the exact shape that produced this bug (and #1868). That is written into the test comment.

Verification: full frontend suite 109 suites / 930 tests (main is 926; the four new cases are the delta), tsc --noEmit clean, eslint 0 errors (the 6 warnings on that file are pre-existing by line), and a production build in a scratch worktree — 9.79s — with the emitted index-*.css carrying grid-row:2 and .v2-shell .v2-pane{height:100vh}.v2-authenticated-shell__content .v2-shell .v2-pane{height:100%} inside the same max-width: 760px block. jsdom parses no CSS, so the build is the only check that the sheet is valid.

Gate: ux-lead at 1200 and 390, banner on and off, on a pod (header tappable), Connectors, Agents and Settings. Press is lily-shen's.

…, and its panes to that box (TASK-157)

Two rules, both measured by ux-lead in a browser at 390 and 1200.

`.v2-authenticated-shell__content` now takes `grid-row: 2` explicitly. Row 1 is
the banner's `auto` row; a verified user has no banner, so the content div was
the first grid child and auto-placement put it in that row. Any page shorter
than the viewport then shrank the inset card to its own content and ended
178-195px above the bottom gutter instead of 14.

At <=760 a pane inside the authenticated shell is `height: 100%` of the content
box, not 100vh. That box has already had the verification banner subtracted, so
a 100vh pane overflowed by the banner's height and its own `overflow: hidden`
left the pod header parked under the banner: back button, title and inspector
toggle untappable, and nothing to scroll back. The rule is scoped (0,3,0) so it
wins on specificity rather than file order (#1868's lesson), and the 100vh rule
it corrects stays in place for the routes outside this shell.

Guarded by a cascade-aware block in v2-layout-invariants.test.ts: it parses the
sheet into rules carrying at-rule context, specificity and document order, then
asserts on the WINNER of the height chain at 390. Presence cannot see either
defect — `not.toContain('100vh')` passes while a later duplicate repaints the
phone, and `toContain('grid-row: 2')` passes while a later rule resets it.

@lilyshen0722 lilyshen0722 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

UX-GATE: FAIL @ d627a1d — the pod header under the banner and the 1200 short-page gap are both fixed, but on the phone pods list a verified user loses the tab bar: 0 of 4 tabs visible or tappable, short list or long.

I built d627a1dd and its merge-base 9a32fca5 (main), served both port-free against fixture APIs, and measured them in Chromium at 1200 and 390, with the verify-email banner off (a verified user) and on. I checked the pod, /v2, Connectors, Agents and Settings, plus Board and Activity at 1200. Hit tests are elementFromPoint at the centre of each control.

What passes:

  • 390 pod, banner on. The header starts at 111. Back, title, inspector and composer all hit-test true. The inspector sheet opens at 185.7–844 and closes, back returns to the list, and there's no scroll trap.
  • 390 Connectors / Agents / Settings, banner on. Each is bounded to 111–844 with 0 px unreachable. On main the bottom 54, 52 and 48 px can't be reached.
  • 1200, banner off. The short-page gap closes: Board and Activity go from 465 to 14 px below the content, Agents from 195 to 14. The rail runs the full height.
  • 1200, banner on, and the pod and home at both states. Identical to main.

What fails, /v2 at 390, the phone pods list. "Tabs" counts the four tab buttons (Pods, Needs you, Team, Settings) that hit-test as themselves. "Long" is the chat group open with 20 pods, scrolled to the end. The document is 390×844 with no scroll trap in every cell.

build verified, short banner, short verified, long banner, long
main 9a32fca5 4/4 · aside 0–788 · z 60 0/4 · aside 111–844 4/4 · last row 746–780 0/4 · last row 802–836
#1871 d627a1dd 0/4 · aside 0–844 · z 60 0/4 · aside 111–844 0/4 · last row 802–836 0/4 · last row 802–836
d627a1dd + the fix below 4/4 · aside 0–788 · z auto 4/4 · aside 111–788 4/4 · last row 746–780 4/4 · last row 746–780

A returning phone user lands on this page. Anyone who has opened a pod on the device stays on the list, because readLastPodId() is set (V2Layout.tsx:164). On this page the tab bar is the only way to reach Needs you, Team and Settings (V2Layout.tsx:205–219 renders the list, the tabs and the first-run hero, nothing else).

Why:

  1. The PR's ≤760 rule .v2-authenticated-shell__content .v2-shell .v2-pane { height: 100% } is (0,3,0), and it also matches the list's aside, .v2-pane.v2-pods-aside.v2-pods-aside--page. It outranks the list's own .v2-shell--list .v2-pods-aside--page { height: calc(100vh - 56px) } at (0,2,0), so the aside grows from 0–788 to 0–844 and runs under the tab bar.
  2. The ≤760 drawer rule .v2-pods-aside { position: fixed; …; z-index: 60 } (v2.css:5538–5550) is only partly undone by the page variant. Position, transform, shadow and border are reset; z-index isn't. .v2-shell--list is display: flex, and a flex item with a z-index other than auto forms a stacking context even while it's static. So the aside paints at 60 over .v2-mobile-tabs (fixed, z 40, 788–844), and the bar can be neither seen nor tapped.
  3. Main already fails this way whenever the banner is up: the aside starts at 111, the flex column shrinks it to 111–844, and z 60 covers the bar. The PR extends that failure to every verified user, which is the common case.

The fix is two changes to the ≤760 list rules at v2.css:10996–11012. I verified it in a build of d627a1dd carrying only these changes (bottom row of the table):

  .v2-shell.v2-shell--list {
    display: flex;
    flex-direction: column;
    gap: 0;
    padding: 0 0 56px;
  }

  .v2-shell--list .v2-pods-aside--page {
    position: static;
    z-index: auto;
    width: 100%;
    transform: none;
    box-shadow: none;
    border: 0;
  }
  • padding-bottom: 56px reserves the tab bar inside the list shell. The aside's height: 100%, which comes from your (0,3,0) rule, resolves against the shell's content box, so the aside ends at 788 with the banner or without it. The selector goes up to .v2-shell.v2-shell--list (0,2,0) so the padding beats the (0,1,0) .v2-shell padding rules on specificity rather than file order. That's the tie your m7 already treats as red.
  • z-index: auto at (0,2,0) completes the drawer reset, so the aside stops stacking over the fixed bar.
  • height: calc(100vh - 56px) is dropped because it's dead. Every V2Layout render sits inside .v2-authenticated-shell__content (V2App.tsx:135, and the V2RequireAuth branch at :231 via :117), so your (0,3,0) rule always outranks it.
  • This also closes main's banner case.

The test: your height-chain assertion stays true under the fix. The list aside's height is still your (0,3,0) 100%; the change is the box it resolves against. That's why the guard can't see this, and it needs two winner assertions at ≤760:

  • The winning z-index on .v2-authenticated-shell__content .v2-shell.v2-shell--list > .v2-pane.v2-pods-aside.v2-pods-aside--page is auto, or at least below the tab bar's 40.
  • The winning padding-bottom on .v2-shell.v2-shell--list equals the winning height of .v2-mobile-tabs (56px).
  • Mutations that must red: drop z-index: auto; padding back to 0; change the tab bar's height without the padding.
  • Worth a comment in the test: the (0,3,0) pane rule reaches .v2-pods-aside--page, so the list's height is now set by the shell's box.

sprint-review's !important gap is TASK-158 and doesn't bear on this verdict.

Re-gate target: the next head.

ux-lead's design gate failed `d627a1dd`: at 390 on /v2 a verified user got 0 of
4 tabs hit-testable, short list or long. Two causes, both from the TASK-157
bounding rule reaching a pane it was not aimed at, and one latent one it exposed.

The list's aside carries `.v2-pane`, so the (0,3,0) rule that bounds panes inside
the authenticated shell also replaced its `calc(100vh - 56px)` with 100% of the
shell's content box — a box nothing reserved the tab bar inside. The aside grew
the full 844 and ran under the fixed bar. Reserve the bar's own height as padding
on the list shell. The two-class selector is required so the padding beats the
(0,1,0) `.v2-shell` phone rules on specificity rather than on file order.

The drawer reset left `z-index: 60` behind. `.v2-shell--list` is a flex
container, and a flex item with a non-auto z-index forms a stacking context even
while static, so the aside painted over the bar (fixed, z 40): not visible, not
tappable. Complete the reset with `z-index: auto`.

`calc(100vh - 56px)` is dropped rather than kept as a fallback — the bounding
rule outranks it in every render of this shell, so it was already inert. Main
failed this same way whenever the banner was up; the padding fixes that too.

Guard: two winner assertions in the TASK-157 block, on the same parser. The
reserved space must equal the tab bar's own winning height (derived from the
sheet, so a bar that grows reds without anyone editing two numbers) and must win
on specificity; the aside's winning z-index must be `auto` or below the bar's.
Eight mutations, one red each — including the one that caught a bug in my own
shorthand expansion (3 values are top|horizontal|bottom), which is why the
padding is read through a resolver instead of by property name.

Measured by ux-lead at 390 on a build carrying only these changes: 4/4 tabs,
short and long, banner up and down. jsdom has no layout engine, so the suite
pins the cascade and never the rendered box.

@lilyshen0722 lilyshen0722 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

UX-GATE: PASS @ 02c1b7e — the phone pods list keeps its tab bar. In every cell I measured, the aside ends at the bar's top edge and the last row scrolls clear of it. Nothing outside the list moved.

I measured this head's own build, served through Playwright routing with the same fixture as the FAIL at d627a1d. I reached the list the way a person does: tapping the pod header's back arrow at 390×844.

list at 390 aside z-index tabs that hit-test last row
verified, short list 0–788 auto 4 of 4 358–392
banner, short list 111–788 auto 4 of 4 469–503
verified, long list, scrolled to the end 0–788 auto 4 of 4 746–780, hit-tests as the row
banner, long list, scrolled to the end 111–788 auto 4 of 4 746–780, hit-tests as the row

The tab bar sits at 788–844 in every cell. The same run reproduces d627a1d's failure on its build: the aside runs to 844 at z-index 60, 0 of 4 tabs hit-test, and the last row sits at 802–836, 48 px under the bar.

Nothing else changed. I measured seven routes on d627a1d and on this head: home, pod, board, Connectors, Agents, Settings and Activity, at 390 and 1200, banner on and off. All 28 cells are identical. That covers:

  • geometry;
  • the hit tests on back, title, inspector toggle, search and composer;
  • the tab bar;
  • the scroll trap;
  • the inspector sheet opening and closing;
  • back to the list.

So d627a1d's two fixes carry: the pod header under the banner, and the 1200 gap.

The CSS delta against d627a1d is the spec: .v2-shell.v2-shell--list gets padding: 0 0 56px, the list's aside gets z-index: auto, and the inert calc(100vh - 56px) is gone. The rest is comment. The v2.css patch-id at merge-base 9a32fca is 3d4e957c.

Press target: 02c1b7e.

@lilyshen0722
lilyshen0722 added this pull request to the merge queue Sep 25, 2026
samxu01 pushed a commit that referenced this pull request Sep 25, 2026
Frame 4 from ux-lead's placement spec on the row: 1440x900 at 2x
(2880x1800, sha256 2b6e1ec2…, 199832 B), copied into docs/assets/readme/
and placed as its own block after the closing paragraph of "Connectors and
grants" and before the `---` above "## First-party apps" — the row's
position, between :63 and :64 at the parent commit.

Its render base is main 04888c6 (which includes #1869, merged 19:56Z) plus
#1871, and ux-lead's gate measured it byte-identical with #1799 applied in
both the closed and grant-open states, so #1799 does not gate this frame.
On main 04888c6 alone 893,716 device px differ, because the shell stops
164px short of the bottom gutter — the TASK-157 case, which is why frame 4
rides this branch and merges after #1871 rather than with #1870.

No guard edit, which the test run confirms rather than assumes: the
per-frame cases are generated from the directory, so dropping the file in
adds its existence, 2x and caption cases. Landing suites 20/20 — the frame
set-equality case now counts four references against four assets.

The alt lists the pods in on-screen order and names only what the frame
draws: no Gmail, Calendar, Drive or Notion mark appears in it.
Merged via the queue into main with commit 24f224e Sep 25, 2026
15 checks passed
lilyshen0722 added a commit to thantiklermcirony/commonly that referenced this pull request Oct 9, 2026
…m-Commonly#1905)

* docs(readme): place the hero frame, and guard the shape every frame keeps (TASK-155)

Frame 1 of 5: ux-lead's 2x capture of the live demo's first state, placed per
the artboard (docs/design/landing-demo/Readme.dc.html) — a centered block,
880 wide, italic caption under it with commonly.me linked, between the header
block and "## What is Commonly?". Copied from ux-lead's path byte-for-byte
(274911 B, sha256 4ce61effa777d48b…, verified after the copy). The asset is
2628x1444 so it renders at 2x once GitHub scales it to 880.

Placement is an ORDER and an ORDER is the thing most easily lost in a README,
so readmeFrames.test.ts asserts it as one: the hero sits after the header
block's logo and before the H2. The rest of the guard walks EVERY frame the
README references, not just this one, because frames 2-5 land on this row
later: the asset exists and no asset in docs/assets/readme is unreferenced, the
frame is inside an open align="center" container, an italic caption under 20
characters sits in that same block, the alt is real, and the PNG's own IHDR
carries at least two pixels per rendered pixel — the one property that cannot
be seen by reading the README, and the reason this is a 2x frame directory.

* test(readme): count caption prose with a scan, not a tag-stripping regex (TASK-155)

CodeQL failed PR Team-Commonly#1870 on the first push: js/incomplete-multi-character-
sanitization, high, at readmeFrames.test.ts:70 — `.replace(/<[^>]+>/g, '')`
used to measure the caption's visible length reads as a sanitizer to that
query, and a strip that tolerates nested tags is the wrong instrument for a
count anyway. Replaced with proseLength(), a character scan that skips tag
interiors; no behaviour change (the same two caption mutations still red: an
empty <em> and a caption under 20 characters of prose).

Caught at the PR check, before either gate ran. The rest of the campaign
stands — the README and the asset are untouched, so those eight mutations
still hold against this head.

* docs(readme): size the hero caption with <sub>, and pin the caption shape per frame

ux-lead's design gate on Team-Commonly#1870 measured the caption on github.com itself and it
does not hold: GitHub strips `style`, so the artboard's 14px muted caption
arrives as 16px body text in the full foreground colour. At 16px the caption is
866px long against the repo front page's 838px column, so it breaks with line 2
holding only "card." at every desktop width from 1280 to 1920.

Wrap it in `<sub>` — on GitHub's supported formatting list, rendered at 75%
(12px). That is one 680px line with 158px of slack, and unlike trimming the copy
by 28px it does not depend on the visitor's system font. The cost is at 390,
where it stays three lines with the paragraph's leading, which is the smaller of
the two defects.

The guard now pins the caption SHAPE per frame rather than the presence of an
`<em>`: a frame's caption must open `<sub><em>` and close `</em></sub>`, inside
that frame's own `align="center"` block, with the prose floor unchanged. Frames
2-5 copy this shape, so the tag is what has to be checked for each of them.

Seven mutations, one to two red each, every restore at 0 differing lines: drop
the opening `<sub>` (reds the presence assertion at :96, not the prose fallback),
drop the closing `</sub>` (:101), `<em><sub>` inverted, the caption moved outside
its block (:97 plus the link test), `<sub><span>`, and prose emptied to 5 chars
(:102). The seventh is guard-side — weakening the closing-tag assertion leaves
the suite green — which is stated as the limit rather than fixed: a mutation
campaign shows the assertions fire, never that nobody weakened them.

Verified: full frontend 110 suites / 931 tests, tsc clean, eslint 0 errors. No
production build: the diff is README.md and a test file, neither in the bundle.
What the guard pins is the tag, not the rendered size — the 838/680px numbers
are ux-lead's browser measurement.

* docs(readme): place the Activity and Your team frames (TASK-161)

Frames 2 and 3 from ux-lead's placement spec on the row: both are 1440x900
at 2x (2880x1800, sha256 91b77a57… and 51da4e69…), copied into
docs/assets/readme/, and placed as ONE block after the closing paragraph of
"What is Commonly?" and before "### Pods".

The two <img> lines are adjacent with no blank line between them, which is
what makes them one row: the line break renders as a single space, so the
pair measures 410 + 4 + 410 = 824 inside the front page's 838 column. A
blank line would break the paragraph and stack them. Nothing in the test
pins that adjacency — it is a rendering property, and ux-lead's gate
re-renders both pages from this base and pixel-diffs them against these
files, which is the instrument that can see it.

Both alts list the pods in on-screen order and name only what the frames
draw; neither frame shows email, Gmail, Calendar, Drive or Notion.

Also folds in the defect sprint-review found on Team-Commonly#1870 and this row recorded
as the :136 item: the test named "links commonly.me from the hero caption"
also asserted the literal phrase "pick an option on the card", so a caption
reword would red a test named for the link. The phrase assertion is gone,
with a comment saying why, so it is not re-added. Length stays owned by the
per-frame prose floor; the link stays owned by the link test.

Guard: 9/9 green with three frames on disk and three references. Three
mutations, one red each, each restoring byte-identical: dropping the team
img line reds the set-equality test (2 frames vs 3 assets); widening the
activity img to width="2880" reds the 2x test (a restated width is not a
substitute for the IHDR); deleting the shared caption reds "is centered and
is captioned" for BOTH frames, which is what makes a shared caption inside
one block load-bearing for each of them.

* docs(readme): place the Connectors frame (TASK-161)

Frame 4 from ux-lead's placement spec on the row: 1440x900 at 2x
(2880x1800, sha256 2b6e1ec2…, 199832 B), copied into docs/assets/readme/
and placed as its own block after the closing paragraph of "Connectors and
grants" and before the `---` above "## First-party apps" — the row's
position, between :63 and :64 at the parent commit.

Its render base is main 04888c6 (which includes Team-Commonly#1869, merged 19:56Z) plus
Team-Commonly#1871, and ux-lead's gate measured it byte-identical with Team-Commonly#1799 applied in
both the closed and grant-open states, so Team-Commonly#1799 does not gate this frame.
On main 04888c6 alone 893,716 device px differ, because the shell stops
164px short of the bottom gutter — the TASK-157 case, which is why frame 4
rides this branch and merges after Team-Commonly#1871 rather than with Team-Commonly#1870.

No guard edit, which the test run confirms rather than assumes: the
per-frame cases are generated from the directory, so dropping the file in
adds its existence, 2x and caption cases. Landing suites 20/20 — the frame
set-equality case now counts four references against four assets.

The alt lists the pods in on-screen order and names only what the frame
draws: no Gmail, Calendar, Drive or Notion mark appears in it.
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