fix(connections): flush card rows and open the approval tab - #7750
Conversation
github-actions
Bot
added
the
readiness: checking
Design Review (Fable 5) — 🟡 CONCERNSDesign-level review of Design-Verdict: CONCERNS Sound shape — sync blank tab plus one reclaim invariant — but delivery is keyed to URL identity, not the attempt, and the browser-dependent core is self-declared unverified. Watch
[DESIGN-REVIEWED] 5d0a0a6 |
First Principles Review (Fable 5) — 🟡 CONCERNSPremise-level review of First-Principles-Verdict: CONCERNS Both declared fixes earn their place, but over half the diff is other work — three slices verifiably already on main, plus two undeclared riders. What this change shipsIntent: make the Connections gallery's card rows sit flush and make a Connect click actually open the provider's approval page — a FIX.
Watch
Subtractions
[FIRST-PRINCIPLES-REVIEWED] 5d0a0a6 |
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsNo findings. False positive or not applicable? A repository writer can comment: |
UX Review (Fable 5) — ✅ PASSUX-level review of UX-Verdict: PASS The connect click now opens a real, labeled tab and the heading tells the truth per outcome — the gallery renders flush with an honest recovery path. Suggestions
[UX-REVIEWED] 5d0a0a6 |
Opus 4.8 Review — ✅ no blocking findingsReviewed Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
2a7423a to
4fe1ed3
Compare
Disposition — Design 🟡 and UX 🟡 on
|
4fe1ed3 to
fae2119
Compare
Disposition — round 2 on
|
| provider | English | longest | locale |
|---|---|---|---|
| gitlab | 83 | 97 | it |
| notion | 58 | 78 | ru |
| linear | 58 | 66 | de |
| atlassian | 45 | 56 | fr |
| stripe | 49 | 53 | de |
| vercel | 45 | 51 | es |
GitLab's is the one that carries "this grant can also write", and it is also the longest and grows the most in absolute terms. I cannot prove how 97 characters wrap at the three-column width — jsdom has no layout engine — so I did not want a green test standing in for that.
The box is therefore a two-line floor with a three-line ceiling (min-h-[34px] line-clamp-3) instead of a fixed two-line height. Rows stay flush for every English value prop, since two lines is what the longest of them needs; a locale that needs a third line grows that one card rather than hiding a permission scope behind a hover-only title that touch and keyboard users cannot reach. Growing one card by 17px is the cheaper failure. The agents gallery card pins an exact height because no permission scope rides in its text — that difference is now stated in the code.
The durable fix is that a scope disclosure should not be the truncatable tail of a marketing sentence at all. That needs a registry field and a catalog pass, so it does not belong in this PR.
4. Refused-state copy duplicates the status chip (UX, Suggestions). Deferred on the reviewer's own terms — it is scoped to "when the connections-copy slice lands", and a new "Open the approval page" string is a 14-locale change this PR deliberately avoids. Noted, not actioned.
Structure: the two commits became one. Commit 1's subject and body described "a pinned two-line box", which finding 3 made false, and a message that misdescribes its own diff is worse than a squash. One commit now, within the hygiene ceiling.
github-actions
Bot
added
readiness: action required
fae2119 to
c67efb4
Compare
github-actions
Bot
added
readiness: checking
…ery mark Three findings from a manual re-test of the Connections gallery. ONE: cards in a row were not flush. Provider value props differ in length -- GitLab's wraps to two lines where Notion's takes one -- and a description with no reserved height makes each card as tall as its own copy, so the row renders ragged. The description now reserves a two-line FLOOR with a three-line ceiling. Two lines is what every English value prop needs at most (the longest, GitLab's, is 83 characters and takes exactly two), so reserving them makes the row flush. The ceiling is three rather than a fixed two because pinning it would CLIP, and the clipped tail is the consequential part: GitLab's copy ends in "this grant can also write" and translations run materially longer (Italian 97 characters, Russian Notion 78 against English 58), so at the three-column width a verbose locale could hide a write-scope disclosure behind a hover-only `title` that touch and keyboard users cannot reach. Growing one card by a line is the cheaper failure. The agents gallery card pins an exact height because no permission scope rides in its text. The grid keeps `items-start`: a card expanded into the approval flow must not drag its row partner up to its height, which is why this is fixed on the content rather than by stretching the cards. TWO: no mint-starting click opened the provider's approval page. The minted URL only ever surfaced as a link labelled "Re-open approval" -- wording for a window that had never been opened -- so the first step of connecting was a link the user had to notice, and the card's own comment recorded the gap rather than closing it. The ordering is what makes this more than a window.open() call. POST /api/connections/mint answers as soon as the mint is SCHEDULED, before the URL exists, and the card polls for it; a window.open() awaiting that URL fires outside the click's user-activation window and is blocked as a popup. That holds on the warm path too, where an adopted premint still arrives on the next poll rather than inside the click. So the click opens a blank tab synchronously and a ref holds it until the URL lands -- the one shape that serves both an adopted premint and a cold spawn. The tab belongs to the moment a mint STARTS, not to one button: Authorize and Reconnect run the identical mint-and-poll path, so all three go through the same entry point. Its lifetime is one invariant rather than a branch per dead end -- while a tab is held, a URL can still arrive only during the in-flight click and the waiting state, so every other resting place (Cancel, a mint reporting failed or expired, a rejected mint, an unmount) reclaims it. Four further points are deliberate: - `noopener` is not passed to window.open, because it makes the call return null and the handle is the feature; the reverse-tabnabbing reference is severed by assigning `opener = null` while the tab is still the same-origin blank document. - The granted tab gets a one-line holding page instead of a bare about:blank, since the mint poll leaves it on screen for seconds on the page's core action. It is written as text from an existing catalog key -- no new key, no locale pass, and no translated string can become nodes -- and sets `color-scheme` rather than colours, so it cannot clash with the active theme. - The waiting heading is driven by the click's own outcome, not by `oauth.minted`. `minted` stays false for the whole poll window, so gating on it told a user whose popup was blocked to finish in a browser page they never got, which is the claim this change exists to remove. A flow the card did not start keeps the original rule, and the outcome is reset when its attempt ends so a later mint cannot inherit it. - A refused tab is not an error. The fallback link is unchanged and is now reached only when the popup was blocked or delivery failed, which is the case its "Re-open" wording describes. THREE: a provider that ships no brand mark rendered an empty gap where its mark belongs -- GitLab today, since the art inventory carries GitHub, which the launch set holds back, and not GitLab, which it includes. The card intended a lettered tile for that case and could never reach it: it built `<ProviderLogo …/>` unconditionally, and a JSX element is truthy even for a slug with no mark, so the `??` beside it never evaluated. The component returns null when RENDERED, which `??` cannot observe. The roster is now asked of the module instead, which makes the fallback reachable and the ProviderLogo docstring's claim about it true. The mark itself is not added here: brand art is a deliberate choice, and the tile is the honest interim. Tests, red-first where red-first is meaningful: the description reserves a floor and clamps at three; a tab is opened by the click before any URL exists; the URL reaches that tab; Reconnect opens one too; a failed attempt and a cancelled one each reclaim the blank tab; the holding page is written as themeless text; the heading never claims an open browser page while the tab stands refused, checked inside the poll window where the first predicate was wrong; and a provider with no mark renders its initial while one with a mark renders no letter. Two guards pass in both directions by construction, since a blocked-popup path and a user-closed tab do not exist before this change. One path is deliberately left unpinned and says so in the test file: a granted tab whose attempt ended is only observable through a second waiting entry with no click behind it, which this harness cannot produce in one mount. One pre-existing test asserted the old heading copy. jsdom grants no window, so its click lands on the refused-tab path where the neutral wording is correct; it asserts the absence of the browser-page claim instead, because the neutral string is shared with the state badge and a positive match cannot tell them apart.
c67efb4 to
b0e40a3
Compare
github-actions
Bot
added
readiness: checking
github-actions
Bot
added
readiness: checking
github-actions
Bot
added
the
readiness: passed
github-actions
Bot
removed
the
readiness: passed
2 participants
Problem / Motivation
Two findings from a manual re-test of the Connections gallery, both user-visible:
Cards in a row are not flush. Provider value props differ in length — GitLab's wraps to two lines where Notion's takes one — and the description is free-flowing text with no reserved height, so each card is as tall as its own copy. A row of cards renders visibly ragged.
No mint-starting click opens the provider's approval page. The intended flow is: click Connect → land on the provider's consent page, with the card's "Re-open approval" link as the recovery path for a popup the browser refused. What shipped has no
window.openanywhere on the page: the minted URL only ever appears as that link, so the first open never happens and the user has to notice a link labelled as if a window had already been opened and closed.Why it matters
The first is the launch surface's most visible polish defect — the gallery is the page the feature is named after.
The second is the connect flow's first step. A user who does not notice the link concludes the click did nothing, and the label actively misleads: "Re-open approval" describes reopening something that was never opened. The card's own source comment recorded the gap ("A minted URL opened no tab…") rather than closing it.
What changed (motivation → approach → change)
Card height. Ragged rows → each card sizes to its own description → the description reserves a two-line floor with a three-line ceiling (
min-h-[34px] line-clamp-3).Two lines is what every English value prop needs at most — the longest, GitLab's, is 83 characters and takes exactly two — so reserving them makes the row flush. The ceiling is three rather than a fixed two because pinning it would clip, and the clipped tail is the consequential part: GitLab's copy ends in "this grant can also write", and translations run materially longer (Italian 97 characters; Russian Notion 78 against English 58). At the three-column width a verbose locale could therefore hide a write-scope disclosure behind a hover-only
titlethat touch and keyboard users cannot reach. Growing one card by a line is the cheaper failure. The agents gallery card pins an exact height because no permission scope rides in its text.The grid keeps
items-start: a card expanded into the approval flow must not drag its row partner up to its height, which is why this is fixed on the content rather than by stretching the cards.The approval tab. The naive fix (
window.open(url)after the mint) does not work, and the reason is the endpoint contract:POST /api/connections/mintanswers as soon as the mint is scheduled — the URL does not exist yet and the card polls for it. Awindow.openawaiting that URL fires outside the click's user-activation window and is blocked as a popup. That holds on the warm path too: an adopted premint still surfaces on the next poll, not inside the click.So the click opens a blank tab synchronously and a ref holds it until the URL lands — the one shape that serves both an adopted premint and a cold spawn.
The tab belongs to the moment a mint starts, not to one button: Authorize and Reconnect run the identical mint-and-poll path through
onReconnect, so all three go through the same entry point. Its lifetime is one invariant rather than a branch per dead end — while a tab is held, a URL can still arrive only during the in-flight click and the waiting state, so every other resting place (Cancel, a mint reporting failed or expired, a rejected mint, an unmount) reclaims it, and reclaiming resets the click's outcome so it cannot outlive its attempt.Four further points are deliberate:
noopeneris not passed towindow.open, because that option makes the call returnnulland the handle is the feature here. The reverse-tabnabbing reference it would have removed is severed by assigningopener = nullwhile the tab is still the same-origin blank document the call just created.about:blank, because the mint poll leaves it on screen for seconds on the page's core action. It is written astextContentfrom an existing catalog key — so no translated string can become nodes, and no new key or locale pass is needed — and it setscolor-scheme: light darkwith no colours of its own, so it cannot clash with the active theme.oauth.minted.mintedstays false for the whole poll window, so gating on it told a user whose popup was blocked to finish in a browser page they never got — the claim this PR exists to remove. A flow the card did not start keeps the original rule.Tests
Red-first where red-first is meaningful, and this PR says which tests discriminate rather than implying all of them do.
Pins (fail without the production change, verified by stashing it):
Guards (pass in both directions by construction, because the state they cover does not exist before this change): a blocked popup still leaves the link as the way in; a tab the user closed is dropped rather than reopened.
One path is deliberately unpinned and the test file says so. A granted tab whose attempt has ended is only observable through a second waiting entry with no click behind it, since a second click sets the outcome explicitly and masks it. Driving that needs a seeded gateway-published banner arriving after a completed click attempt: once a URL is published the card holds the waiting state through
expired, and a fresh mount resets the state under test. The fix is a two-line reordering, reviewed against the trace that found it; a contorted fixture would have bought a green tick and no confidence.One pre-existing test asserted the old heading copy. jsdom grants no window, so its click lands on the refused-tab path where the neutral wording is now correct; it asserts the absence of the browser-page claim instead, because the neutral string is shared with the state badge and a positive match cannot tell the two apart.
Full frontend suite green.
tsc -bandeslintclean.Manual verification
The card-height change was applied to a live pod serving this page and eyeballed against the ragged before-state that prompted the fix.
The tab behaviour needs a real browser — jsdom has no popup blocker and no window manager, so the unit tests pin the call, the delivery and the reclaim, not the browser's own decision. Still to do in a browser: confirm the tab opens on the click and is filled when the URL arrives, and confirm a browser configured to block popups falls back to the link with the neutral heading.
Screenshots / video
Pending — the layout change is a real visual delta and will be attached before this is considered ready, rather than waived. The before-state (ragged rows) is what motivated the change.
Related Issues
no linked issue: both findings came out of a manual re-test of the gallery, not a tracked issue.
This branch is rebased onto main including #7568 (warm handoff), which merged while review round 2 was in flight, so the diff sits on top of that engine rather than beside it.
Note on CI:
Backend Tests (Windows) (3)was inherited rather than from this diff — this PR contains zero Python files, andtest_security_posture.py::TestGateSideLogRedactorSpellingfailed identically at the earlier base commit, whereslack/gateway.pycarried 7 gate-side log sites against a census of 6.Pattern harvest
Rule candidate: when a card grid must render flush rows, reserve a floor for the variable-length content rather than pinning it — stretching couples a row's height to its tallest member and breaks when one card expands, while a fixed height silently clips. A floor gets flush rows in the common case and lets the exception grow.
Rule candidate: never let a clamp decide whether a permission or scope disclosure is visible. If a truncatable string ends in what the user is actually granting, either the box must be tall enough for every locale or the disclosure does not belong in that string — and "hover shows the full text" is not a recovery path for touch or keyboard users.
Rule candidate: before adding a
window.openfor a URL that arrives asynchronously, read the endpoint's own contract for when the URL exists. An endpoint documented as "returns once scheduled; poll for the result" cannot be awaited inside a user gesture, so the tab must be opened by the click and filled later. A code comment that explains why a capability is missing is a defect record, not a design decision.Rule candidate: attach a fix to the EVENT, not to the button that happens to raise it. Three controls started the same mint here; wiring the tab to one left the other two carrying the original defect, and the predicate meant to describe "a flow this card did not start" quietly described two of its own buttons.
Rule candidate: a resource acquired in a user gesture needs its release expressed as an invariant over the states in which it can still be consumed, not as a branch per failure — and the reset of any flag describing the attempt must sit OUTSIDE the guard that tests whether the resource is still held, or a successful path leaves the flag set forever.
Screenshot evidence
After — the connections gallery at this PR's head, rendered from the built bundle with fixture API state (six providers, first-run "Not connected"). Both rows flush: every description sits in the same reserved two-line floor (GitLab's two-line copy vs the one-liners), and GitLab shows the lettered fallback tile this PR makes reachable.
Evidence committed at temp-screenshots/connections-flush-rows/connections-gallery-after.png (sha256 ef3febdc61a3193dca91e7967418cce27a140388b7ae7fdc264535d63b2fc286). Captured via network-intercepted Playwright against the built dist — real render, fixture /api/connections/* responses — because a live pod's Disconnect/status reads would touch real grants.