Repository navigation
fix(connectors): a list card is as tall as its rows (TASK-156) - #1869
Conversation
#1547 stretched the channel list to fill the page; #1669 then put Tools under it and the stretch carried on, so at 1200 the channel card was 564px around 262px of rows, and with the GitHub grant open the Tools card was 718px around one 86px row — 630px of empty card. The page scrolls; a card does not hold empty space. - .v2-connectors: drop min-height: calc(100vh - 86px) and its comment. - .v2-connectors__content: drop flex: 1 / min-height: 0 and switch stretch to start; .v2-connectors__rows: drop flex: 1 / min-height: 0 (keep overflow: hidden, it clips the row corners). - The now-dead opt-outs go with them: .v2-tools { flex: none }, and the 1010px and 760px undo rules. - The Slack row names its workspace: a linked Slack stores teamName, never chatTitle, so the row read "Slack · linked to Ops" beside the word Slack. chatTitle still wins when both are set. Guards invert with the decision: v2-layout-invariants asserted the old stretch (and its undo rules); it now asserts no min-height on .v2-connectors, align-items: start with no flex: 1 on the content grid, no flex: 1 on the rows, and that no .v2-tools rule opts out. The Tools opt-out is matched as a regex on the Connectors section rather than through ruleBody, because with the rule gone ruleBody returns '' and the assertion would have passed without reading anything. Three page cases cover the Slack title precedence.
lilyshen0722
left a comment
There was a problem hiding this comment.
GATE: PASS at 9e64a125 — measured. One non-blocking note.
BASE 163/163, RESTORED 163/163, tree clean after every mutation. v2.css braces balance 2007/2007. Anchor count 1 each.
| mutation | result |
|---|---|
align-items: start → stretch |
1 red |
the dead .v2-tools { flex: none } restored |
1 red |
teamName dropped from the title chain |
2 red |
teamName moved ahead of chatTitle |
1 red |
flex: 1 restored on .v2-connectors__rows |
1 red |
Your trap fix works, and it is the important one. Re-adding .v2-tools { flex: none } reds. That is the proof the regex reads the section — with ruleBody returning '', not.toContain would have passed on nothing at all, which is the standard way this class of assertion dies. Catching it before shipping is the right instinct; the regex-over-the-section is the right repair.
The title chain is pinned for order, not just presence: flipping teamName ahead of chatTitle reds on its own. A chain asserted only by membership would have accepted that.
Note: the min-height guard reads one rule, and the cascade uses the last
expect(connectors).not.toContain('min-height') runs on ruleBody(v2, '.v2-connectors'), which takes the first line-start match — v2.css:9685. I added a second .v2-connectors { min-height: calc(100vh - 86px); } at :9706, leaving the original untouched: 163/163 green. In the browser the later rule wins and the stretch is back.
Not a defect, and not blocking: the natural way to undo this change is to edit the rule at 9685, which the guard catches. The duplicate-rule route is the one it cannot see, in an 11k-line sheet where the same selector reappearing in another section is plausible.
If you want it closed, the cheap version is a count rather than a body read:
expect(v2.match(/^\.v2-connectors \{/gm) || []).toHaveLength(1);Same shape as #1855's order guard: a rule body answers "what does this rule say", never "what does the cascade do".
…e first (TASK-156) sprint-review's gate passed #1869 at 9e64a12 and named one route it does not cover: `not.toContain('min-height')` reads ruleBody's FIRST line-start `.v2-connectors` match, so a second `.v2-connectors { min-height: … }` written later in the sheet passes every assertion while the browser paints the stretch again — equal specificity, later rule wins. layoutBlocks() instead scans the whole sheet for every declaration block whose last compound carries the class, and the three selectors that hold the decision (`.v2-connectors`, `__content`, `__rows`) are asserted empty of the declarations that would stretch them. A non-vacuity check per selector keeps the filters from passing on an empty scan. Additive only: every assertion sprint-review gated is byte-identical, so their PASS still describes this tree; what is new is a stronger instrument beside it. A rule scoped another way than the last compound (e.g. `.v2-connectors > div`) is still not covered — said in the comment.
lilyshen0722
left a comment
There was a problem hiding this comment.
Gap closed. PASS stands at 8fc119b3.
I re-ran rather than carried, because the new filter is the object in question.
- Additive only, confirmed: the diff against the head I gated is
+23, -0in one test file. No assertion I measured changed, so the earlier five mutations still describe this tree. - BASE 163/163, RESTORED 163/163, tree clean.
The route I reported is shut, and so are the two neighbours it implies:
| appended at EOF | result |
|---|---|
a second .v2-connectors { min-height: … } (my route) |
1 red |
.v2-root .v2-connectors { min-height: … } (descendant-scoped) |
1 red |
.v2-connectors__rows { flex: 1 } |
1 red |
base __rows rule deleted, so the scan finds nothing |
1 red — the non-vacuity check |
Matching on the last compound rather than the literal selector string is what makes the descendant-scoped case catchable, and it is the version I would have missed if I had only asked for a selector count: .v2-root .v2-connectors never matches ^\.v2-connectors \{ and would have slipped straight through the cheap fix I proposed.
The non-vacuity check at :1828 is the part worth keeping deliberately. A filter of the shape "every matching block is empty of X" passes loudest when nothing matches — that is the same failure that made the original .v2-tools assertion pass on an empty ruleBody. Deleting the base rule reds it, so the scan cannot silently stop scanning.
lilyshen0722
left a comment
There was a problem hiding this comment.
UX-GATE: PASS @ 8fc119b — both list cards are exactly as tall as their rows at 1200 and 1440, with the grant closed and open; the GitHub row sits fully above the 1440×900 fold; 390 is unchanged.
Measured in a real browser against 9e64a125. The main build (9a32fca5) and this branch's build were both served from disk with the same sample data. 8fc119b3 adds only test lines (+23 in v2-layout-invariants.test.ts). The CSS and component diff has the same patch-id at both heads (833432e8), so these numbers still describe the current head.
Dead space is the card height minus its rows minus the 2 px border.
| main | this PR | |
|---|---|---|
| 1200 · Channels / Tools | 300 / 0 | 0 / 0 |
| 1200 grant open · Channels / Tools | 122 / 630 | 0 / 0 |
| 1440 · Channels / Tools | 304 / 0 | 0 / 0 |
| 1440 grant open · Channels / Tools | 126 / 630 | 0 / 0 |
- At 1440×900 the GitHub row ends at 678, so the whole row is above the fold. On main it ends at 856.
- 390 matches main with the grant closed and open. Cards are 334 and 106 tall with no dead space. The GitHub row sits at 1023–1127 closed and 697–801 open. Nothing scrolls sideways.
- The Slack row now reads "Acme · linked to Support". On main it reads "Slack · linked to Support".
Not blocking, and filed as its own board row. Without the stretch, the Connectors page is shorter than the screen, so the inset card stops short of the bottom. At 1200×900 it ends 178 px above the edge, where the gutter should be 14. This PR only exposes the problem; the cause is in the shell (#1510). .v2-authenticated-shell uses grid-template-rows: auto minmax(0, 1fr). A verified user has no banner, so the content auto-places into the auto row, and any page shorter than the viewport shrinks the card. Agents already does this on main (195 px). Adding .v2-authenticated-shell__content { grid-row: 2; } on this branch moved the gap from 178 to 14 at 1200 and 1440. The banner case, Settings and 390 did not change. Please don't restore the page min-height here — the shell fix covers every short page at once.
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.
…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.
TASK-156. #1547 made the channel list fill the page ("the channel list is the page's content card"); #1669 then put Tools under it and the stretch carried on. ux-lead measured on
9a32fca5with a sample-data fixture: at 1200 the channel card is 564px around 262px of rows, and with the GitHub grant open the Tools card is 718px around one 86px row — 630px of empty card. 390 was clean only because the ≤1010 block undid the stretch.The rule
A list card is as tall as its rows. The page scrolls; a card never holds empty space. An aside taller than its list runs on past it on the white.
Changes
.v2-connectors: dropmin-height: calc(100vh - 86px)and its comment..v2-connectors__content: dropflex: 1; min-height: 0;,align-items: stretch→start..v2-connectors__rows: dropflex: 1; min-height: 0;(keepoverflow: hidden— it clips the row corners)..v2-tools { flex: none }, and the 1010px (min-height: 0,__rows { flex: none }) and 760px (min-height: 0) undo rules.teamName(slackOAuthService.ts:87) and neverchatTitle, so the row read "Slack · linked to Ops" beside the word Slack.chatTitlestill wins when both are set; the not-linked variant reads "Acme · not linked to a pod"; the non-owner projection carries neither and keeps today's fallback.Guards, inverted with the decision
v2-layout-invariants.test.tspinned the old stretch and each undo rule. It now asserts: nomin-heighton.v2-connectors;align-items: startand noflex: 1on the content grid; noflex: 1on the rows; the 760 rule matchesgap: 24px; margin: -12px -18px 0without amin-height.One thing worth flagging for review: the Tools opt-out is asserted as a regex over the Connectors section, not through
ruleBody. With the rule deleted,ruleBodyreturns''andexpect(ruleBody(v2, '.v2-tools')).not.toContain('flex: none')would have passed without reading anything — a vacuous assertion. Mutation m4 (re-adding the rule) is what proves the regex actually reads the section.Verification
teamNameis not read at all (m6, 2 tests);teamNameoutrankschatTitle(m7). Base green before and after (163/163).tsc --noEmitclean; eslint 0 errors (the 8 warnings on these files are pre-existing and outside the diff — verified by line).vite build9.35s, and the emitted CSS confirms the deletions survive minification:.v2-connectorshas nomin-height, the content grid isalign-items:start, the rows have noflex, no.v2-tools{flex:none}exists, and the 760 rule is{gap:24px;margin:-12px -18px 0}. jsdom parses no CSS, so the build is the check that the sheet is even valid.Limits
jsdom has no layout engine, so the pixel claims are ux-lead's to re-measure — requested gate: 1200 and 390, default and with the GitHub grant open, with the same fixture: each
.v2-connectors__rowscard equals its rows plus the 2px border in both states, the GitHub row sits fully above the fold at 1440×900, and 390 is unchanged.Out of scope, as the row says: the Discord · WhatsApp wrap (#1799), the hardcoded "linked to" string (#1717 — whichever lands second rebases), and two asides open at once (intended).