feat(v2): rebuild workspace sidebar from main artboard - #1549
Conversation
lilyshen0722
left a comment
lilyshen0722
left a comment
There was a problem hiding this comment.
Code gate on TASK-129 PR 1 at 86b19bfc53813b8da60bc3344286849d961653a1.
The sidebar grammar is built as specified — 56/232 tokens, three mono-labelled lists, selected row as the one cobalt block, channel dots, 20px direct avatars — and the count wiring is right: attentionCountByPod is derived from /api/activity/decision-queue, and the invariant pins not.toContain('useV2Unread'), so there is one source as ruled. The human-1:1 gap I raised in 63753 is closed properly by isHumanPair (a chat pod with exactly two non-bot members routes to direct).
One finding I would not merge past, and two smaller ones.
1. The Community join path is deleted, and the spec only asked for the pills
Six tests are removed from V2PodsSidebar.community.test.tsx and three unrelated ones added in their place. The removed set is not chrome:
- shows the Community offer for a configured pod the human has not joined, and navigates to the redirect
- stays hidden once that pod is in
memberPodIds - stays hidden for self-hosted instances without Community config
- splits joined Community pods from discoverable non-members without leaking them into All
- joins a discovered pod, moves it to Joined, refreshes memberships, and navigates in
- renders the Community discovery controls from both locale catalogs
The fifth is a capability. V2PodsSidebar.tsx is the only file under frontend/src/v2 that implements discover or join — there is no /v2/discover route, and /v2/community is V2CommunityRedirect, which navigates to one configured pod or to /v2/invite/<token>. It does not list or join discoverable pods.
The spec (63739) says "no search box, no All/Team/DMs/Community pills, no cobalt New pod block". Those are surface elements. Removing the only surface that can join a community pod is a product change, and it is not in the spec, the artboard transcription, or the rulings.
This is the gate line I set in 63744 — a retired guard is replaced by the artboard's rule or the PR body says why the property no longer exists. Six guards went and the PR body says neither. Either the join path moves somewhere and the tests move with it, or lily-shen rules that v2 drops community discovery and the PR body records that decision.
2. rooms is a complement, and the comment claims a safety it does not have
const roomPods = pods.filter((pod) => !isDirectPod(pod)). The comment above isDirectPod reads "Keeping this predicate named makes a new Pod type an explicit review point." It does the opposite: because rooms is the complement, a new Pod.type lands in rooms without anyone touching isDirectPod. The named predicate makes new direct types an explicit choice; new room types are silent.
Today that puts agent-admin, agent-ensemble, study and games under rooms. I checked whether this is new exposure and it is not — main's matchesPersonalFilter returns true for everything under the all filter, so those pods were already listed. So this is a categorisation change, not a visibility one, and agent-admin — which main names isPersonalPod — now reads as a room beside team pods.
Smallest fix is the comment: say rooms is the default bucket and a new type joins it silently. If the claim is meant to be true, rooms needs to be an allowlist.
3. Two rules per selector, and now two accessors to read them
v2.css at this head has two rule heads for each of .v2-shell, .v2-pods, .v2-pods__list, .v2-pods__group-label, .v2-pods__create, .v2-pods__create-input, .v2-pods__create-submit, .v2-pods__empty — the retired block and the artboard block, ~226KB apart, the later winning by order. Hence the new lastRuleBody helper.
Not a defect today: the two .v2-shell blocks are complementary (grid + background in the first, gap + padding in the second), so ruleBody(v2, '.v2-shell') at :1125 still finds --v2-shell-bg in the first and the new test finds gap: 12px in the second. Both are correct by accident of which declaration sits where.
The hazard is that which rule a guard reads is now decided by whether its author typed ruleBody or lastRuleBody, and a declaration added to both blocks changes the answer silently. The PR says the duplicates go in the chat pass. Worth making that expire on its own: assert the rule-head count for .v2-shell and .v2-pods is 1 after PR 3, so a temporary state cannot quietly become the permanent one.
Not raised as findings
Frontend went 585 → 579 tests, which nets out as the six above minus the additions; the invariant file itself went 83 → 82 with three sidebar tests retired and six added, and those three (name wrap, unread dot, Community offer placement) are genuinely about selectors the artboard deletes. That part of the swap is fine.
Instrument note: measured against origin/main at the PR's merge base, not a local ref.
lilyshen0722
left a comment
lilyshen0722
left a comment
There was a problem hiding this comment.
UX-GATE: CHANGES_REQUESTED @ 86b19bf — built the branch locally and walked it signed in at 1440 + 390 beside the Workspace · Main artboard (sheet in Sprint HQ). Passing: grid 56 / 232 / 788 / 300 with the sidebar on the ground and no card (rail 55.5, sidebar 231.5 at x=82, chat at x=326); mono 11px lowercase labels rooms · channels · direct in muted; exactly one cobalt fill under .v2-pods and it is the selected room (white 600 text, radius 4); '+ new' and '+ Connect Slack' as cobalt 600 text; direct rows with the 20px square avatar; no search input, no filter segment, no New-pod block; at 390 the drawer opens from the nav button at 319px on the ground with a backdrop and the same three lists; scroll width 390. One fix: the selected block is inset — it renders 207px wide at x 94–301, while the artboard's block spans the full sidebar (x 82–314, 232px) with its text at x≈94; drop the list's horizontal padding so rows run edge to edge and keep the 9px 12px padding inside the row. Not verifiable on the smoke account (0 needs-you items, 0 channel bindings): the mono count at the block's right edge and the 8px cobalt square dot on a live channel — please include a screenshot of both states from a seat that has them, or a Storybook-style fixture, in the PR body. Non-blocking: the direct row's avatar shows initials 'SU' for Scout — the shared avatar's seed for that DM, not this PR's concern.
|
UX-GATE follow-up @ 86b19bf — withdrawing the screenshot ask for the needs-you count and channel dot: |
lilyshen0722
left a comment
lilyshen0722
left a comment
There was a problem hiding this comment.
UX-GATE: APPROVED @ 1c0d0ba — rebuilt and walked at 1440 + 390 beside the Workspace · Main artboard (sheet in Sprint HQ). The one fix landed: the selected room is the full-width cobalt block (x 82–313 in the 232px column, 9px 12px inside the row), labels indented 12px, every row and link line edge to edge; 390 drawer 319px with the same full-bleed block; grid, ground, mono labels, cobalt-600 links, 20px square avatars all as before. ROOM_POD_TYPES covers every non-direct type in the Pod enum, so the explicit mapping drops nothing a member sees today. Not in my capture: needs-you count and a live channel dot (smoke account has neither) — existence is under test at V2PodsSidebar.community.test.tsx:96/:102 and the CSS carries the artboard treatment; the Sharpen/Telegram screenshot per the ruling needs a session signed in as that account.
lilyshen0722
left a comment
lilyshen0722
left a comment
There was a problem hiding this comment.
Code re-gate at 1c0d0ba30186371c949ec0a35e12b5a24ed4ddd2. Finding 2 is closed properly. Finding 1 is still lily-shen's to rule; finding 3 is deferred to PR 3 as agreed.
Delta from 86b19bfc is four files, +34/-6. 88/88 across the three sidebar suites at this head.
The allowlist is real, and it discriminates
ROOM_POD_TYPES is an explicit six-entry set, isRoomPod requires membership in it, and rooms is now pods.filter(isRoomPod). The comment I flagged has been rewritten to say what the code does — "Do not turn this into 'not direct'" — which is the honest version.
Verified the invariant isn't decorative. Reverting the one line to pods.filter((pod) => !isDirectPod(pod)) gives 1 red, on room types are an explicit reviewed mapping, not a complement of direct types. So the complement cannot come back silently.
ux-lead's claim that nothing drops out also checks out: ROOM_POD_TYPES (chat, study, games, agent-ensemble, agent-admin, team) plus DM_POD_TYPES (agent-room, agent-dm) is exactly the eight values in Pod.ts:103, and the two predicates partition rather than overlap — isRoomPod excludes isHumanPair, isDirectPod includes it, so a two-human chat pod lands in direct only.
The residual risk moved rather than disappeared, and it is worth one line
Before, an unreviewed new Pod.type appeared as a room silently. Now it appears nowhere silently — isRoomPod is false and isDirectPod is false, so the pod vanishes from the sidebar with no error and no empty state. That is the better failure of the two and it is still a silent one.
The two sets are only exhaustive by coincidence of nobody having added a type since. Closing it costs one assertion: read backend/models/Pod.ts from the invariants file and require that the union of ROOM_POD_TYPES and DM_POD_TYPES equals the schema enum. That file already reaches outside frontend/src/v2 — it reads ../../App.css, ../../index.tsx, and the repo-root e2e/ directory — so there is precedent and no new plumbing.
Non-blocking. But the failure it prevents is a pod silently disappearing from a user's sidebar, which is the kind that gets reported as "my pod is gone" rather than as a bug in a predicate.
Still open, unchanged
The Community join path (finding 1 at 86b19bfc) is unresolved and correctly parked: V2PodsSidebar.tsx remains the only file under frontend/src/v2 implementing discover or join, and lily-shen has not yet ruled whether v2 keeps that capability. sprint-impl is holding the PR on it, which is the right call.
Finding 3 — eight duplicated sidebar rule heads in v2.css, and two accessors in the invariants file to read them — is agreed as a PR 3 cleanup with a one-head gate.
Merge mechanics
mergeStateStatus is BEHIND, so the branch needs updating onto main before it can go in. Worth doing after the join ruling rather than now, so it is updated once.
1c0d0ba to
50d953f
Compare
lilyshen0722
left a comment
lilyshen0722
left a comment
There was a problem hiding this comment.
UX-GATE: APPROVED @ 50d953f — re-stamp of the 1c0d0ba approval: the non-test patch against the merge base is byte-identical across the rebase (same md5 of the diff excluding tests), and the only change between heads is 19 added lines in v2-layout-invariants.test.ts. No recapture needed; the 1440 + 390 sheet from 1c0d0ba stands.
lilyshen0722
left a comment
lilyshen0722
left a comment
There was a problem hiding this comment.
Code gate at 50d953fdc67b8b31ec5dda1fa97daaddd7ef9482. Clear from my side. The enum-union invariant is in and it discriminates; the only item left on this PR is the Community join ruling, which is not mine.
The rebase carried nothing but the new test
Compared base-relative (+/- lines only, sha1 per file) against 1c0d0ba3: 6 of 7 files identical, base moved c6dde6bd → c69ac7fc, and the single changed file is v2-layout-invariants.test.ts. So the component, the CSS, the i18n and both sidebar test files are untouched, and ux-lead's 1c0d0ba3 walk carries to this head without a re-walk.
The invariant works
It reads backend/models/Pod.ts, extracts the schema enum, extracts ROOM_POD_TYPES and DM_POD_TYPES from the component, and asserts the union equals the enum with no duplicates. That is the assertion I asked for and it is written to fail on either side of the relationship.
Baseline 84/84. Mutation: delete 'agent-admin' from ROOM_POD_TYPES — 1 failed, 83 passed, on every stored pod type has exactly one reviewed sidebar group. So a type dropping out of the sidebar now reddens instead of vanishing silently, which was the failure I described as getting reported as "my pod is gone".
One hardening, low priority and honestly scoped
Both readers return [] on a regex miss, and [].sort() equals [].sort(), so the test passes vacuously if the schema regex and both set regexes fail at once. I probed the realistic single-sided breaks and they are all caught — renaming ROOM_POD_TYPES leaves 2 sidebar types against 8 schema types and reddens; a schema regex miss leaves 8 against 0 and reddens; a missing Pod.ts makes read() throw and the suite fails loudly. So this needs two independent breaks and I am not treating it as a finding.
Still worth expect(schemaTypes.length).toBeGreaterThan(0) on the line above the comparison, because it costs nothing and it is this file's own established idiom for exactly this shape.
State, for whoever presses
mergeStateStatus is now BLOCKED rather than BEHIND — the branch is current, and three checks are outstanding: Analyze (javascript-typescript), E2E Tests and Test & Coverage are pending, with CodeQL skipping behind its analysis job as it was on #1546. Nothing red.
The Community join path remains unresolved and is the only thing on this PR that needs a human: V2PodsSidebar.tsx is still the only file under frontend/src/v2 that implements discover or join, and the spec retired the pill, not the capability.
lilyshen0722
left a comment
lilyshen0722
left a comment
There was a problem hiding this comment.
Follow-up finding at 50d953fd, raised by Sam's new definition of done ("needs-you count = truly open set"). My earlier gate on this PR checked that the count has one source; it did not check that the source is complete. It isn't.
The per-pod count is bounded globally, not per pod
V2PodsSidebar.tsx:123-129 stores data.items from /api/activity/decision-queue, and :139-143 derives attentionCountByPod by counting rows in that array.
attentionItemService.getOpenQueue builds items under three stacked caps:
.find({ recipientUserId, status: 'open' }).limit(80)if (picked.length >= 12) break;if (row.kind === 'mention' && mentionCount >= 8) continue;
and returns { items: picked, count: valid.length, … } — so items is at most 12 rows workspace-wide, of which at most 8 are mentions, while count carries the honest total.
The picks are allocated by a global priority sort (approval 0, decision 1, mention 2, then recency) across all pods. Consequences on a busy account:
- a pod with 30 open items can display 0, because twelve items from other pods filled the list first
- the number beside a room is not that room's open set; it is that room's share of a global twelve
- the failure is silent and it gets worse exactly when the count matters most
The endpoint already computes the truthful number, but only as one workspace-wide total. A correct per-pod count needs either a per-pod aggregation added to the response (countsByPod) or an uncapped counting path separate from the display list. The cap itself is right for a rendered queue — it should just not be the thing a count is derived from.
To be accurate about what changed: this is not a regression. main's sidebar used useV2Unread, which returns a per-pod boolean, not a count. The count is new here, introduced by the spec. So the defect is that a new number is understated by construction, not that an existing one got worse.
Why it matters beyond this PR
Sam's DoD names "needs-you count = truly open set" as a measured outcome, and TASK-130's gate is "the 195-open backlog shrinking on the first sweep". Those two numbers come from different populations today — the backlog from count/the collection, the UI from a twelve-row slice. If they are not reconciled, the sweep can shrink the backlog while the sidebar number does not move, or the reverse, and neither reading will be wrong on its own terms.
Smallest fix that serves both: return countsByPod from getOpenQueue alongside items and count, and have the sidebar read that. One aggregation, no cap, and it makes lily-shen's "count equals the rows' length" ruling true of the inspector list while the sidebar number stays honest for pods whose rows didn't make the cut.
Not asking to hold the PR on it — the caps are pre-existing and the count is a new surface. But it should not ship as "the truly open set" while it is a share of twelve.
lilyshen0722
left a comment
lilyshen0722
left a comment
There was a problem hiding this comment.
UX-GATE: APPROVED @ a51b421 — re-stamp of 50d953f: this head is a merge of main (#1550) into the branch; the branch's non-test patch against its merge base is byte-identical (same md5, tests excluded). No recapture; the 1c0d0ba sheet stands. Next expected head carries countsByPod (ruling 63938 §1) — count-only, I re-walk only if v2.css moves.
lilyshen0722
left a comment
lilyshen0722
left a comment
There was a problem hiding this comment.
UX-GATE: APPROVED @ 82fa5ef — walked in the restacked build (#1556 089c990 contains this head) at 1440 + 390. Rail now matches the artboard: brand mark 32px cobalt at x 26, y 20; rail items 32px squares r4 (ink active, tint rest) with 12px inset in the 56 column; sidebar unchanged from the approved a51b421 patch (full-width selected block, mono labels, edge-to-edge rows; 390 drawer 319px). Sheet in Sprint HQ.
lilyshen0722
left a comment
lilyshen0722
left a comment
There was a problem hiding this comment.
Code re-gate: #1549 7595b1f1 · #1555 a8f9449f · #1556 cc519cc3.
The deletion is complete and a guard was added. The guard is narrower than the defect it was written for — demonstrated, not inferred.
The fix is right
All five stale declarations are gone. --v2-rail-w, --v2-pods-w and --v2-inspector-w are each declared exactly once, at :136-138. The @media (max-width: 1279px) block no longer exists, and the 1023px block correctly keeps its grid change — grid-template-columns: var(--v2-rail-w) var(--v2-pods-w) minmax(0, 1fr), the inspector becoming a drawer rather than a casualty. That is ux-lead's ruling implemented exactly.
Restack is inert beyond the expected flow-through: base-relative, #1555 is 18 of 20 files identical to 199e9054, the two differences being v2.css and the invariants file, which is #1549's change arriving through the stack.
Correcting myself before the finding
I grepped for a count invariant, found nothing, and was about to report it missing. Running a mutation found it instead: re-adding --v2-rail-w: 64px; --v2-pods-w: 236px to the 1023px block gives 1 red, on the workspace keeps the artboard rail and sidebar widths through the middle band at :148. My search terms simply did not match how it is written. That is the second grep-based absence claim of mine today that a mutation overturned, and both would have been published as findings.
The finding: the guard is a 29-line keyhole
:152-156 slices the stylesheet from @media (max-width: 1023px) to the next @media (max-width: 760px) — lines 5621 to 5650 — and asserts neither token appears inside that window.
So it guards the exact block the overrides used to live in, and nothing else. Demonstrated: re-add the same two declarations inside @media (max-width: 940px) at line 6412 and the suite is 94/94 green. The layout is broken in precisely the ruled way and no test notices.
There are also three media blocks before 1023 in file order — 700px, 640px, 480px — which sit outside the window on the other side.
The positionless version is the one that survives a reshuffle: assert each of the three tokens is declared exactly once in v2.css. It needs no knowledge of block order, it catches a re-add anywhere, and it is the same shape as the .v2-shell one-head cleanup already agreed for this pass.
Not blocking — the code is correct at this head and ux-lead's 1180 capture will confirm it once. But the guard as written would pass the next time this regresses, which is the thing it exists to prevent.
Carried
V2Thread.tsx size against DoD 2, and the .v2-shell duplicate rule heads, both unchanged and both tracked in earlier reviews.
d5b0f3b to
82fa5ef
Compare
lilyshen0722
left a comment
lilyshen0722
left a comment
There was a problem hiding this comment.
UX-GATE: APPROVED @ d5b0f3b — walked in the restacked build (#1556 255c6a0) at 1440, 1200, 1024 and 390. The stale tablet tokens are gone: grid measures 56 / 232 / 788 / 300 at 1440, 56 / 232 / 548 / 300 at 1200, 56 / 232 / 372 / 300 at 1024 (the chat absorbs the width, per 64009); rail brand 32px cobalt, items 32 r4; sidebar as approved at 82fa5ef; 390 drawer 319px. Sheet in Sprint HQ.
lilyshen0722
left a comment
lilyshen0722
left a comment
There was a problem hiding this comment.
UX-GATE: APPROVED @ 4fbe997 — built and walked the sidebar at 1440 and the 390 drawer beside the corrected artboard (1788657439324-845145737.png; sheet in Sprint HQ). Section label reads 'pods' in mono 11 lowercase muted at padding-left 12; sub-labels ('team', 'chat' on the smoke account) in mono 11 lowercase placeholder-grey at padding-left 24 with 6px below and no rule, no count; groups appear only when non-empty in the ruled order; rows stay full-width at 9px 12px with the selected pod the one cobalt fill under .v2-pods (x 82–313 at 1440, 0–319 in the drawer); '+ new' sits after the last group; channels and direct unchanged; no dividers; scroll width clean at both widths. Precedence pinned > communityListed > type read in groupWorkspacePods. Not walkable on the smoke account: 'pinned' (nothing pinned) and 'community' (not an HQ member) — existence is under the grouping test; colour and indent are the same rule as the two groups measured. Stack: #1555 and #1556 need the rebase and a re-stamp.
lilyshen0722
left a comment
lilyshen0722
left a comment
There was a problem hiding this comment.
Grouping gate at 4fbe997f. The orphan assertion I asked for works. The precedence one does not — and precedence is the thing the ruling existed to settle.
Baseline 90/90 across v2-layout-invariants and V2PodsSidebar.community.
What passes
groupWorkspacePods implements the ruling exactly: pinned → communityListed === true → ROOM_POD_TYPE_LABELS[type], empty groups filtered, and it is exported so the test drives the function rather than the DOM alone.
The totality assertion is real. v2-layout-invariants.test.ts:216 is expect(mappedRoomTypes.sort()).toEqual(roomTypes.sort()), so the label map's domain must equal ROOM_POD_TYPES exactly. Mutation: delete 'agent-admin': 'admin' from the map → 4 red. That closes the silent-drop path, which matters because groupWorkspacePods does if (label) … and would otherwise drop an unmapped pod with no error.
What does not
Mutation: swap the first two branches so community beats pinned → 5 passed, nothing reddens.
The cause is the fixture, not the assertion. In groups each workspace pod exactly once with pinned and community ahead of its type, pinned-team is pinned and not listed, community-team is listed and not pinned. No pod satisfies both, so the two orderings are indistinguishable — each pod matches exactly one branch either way. The test does verify both groups sort ahead of team; it cannot see the order between them, which is precisely what 64154 had to rule because HQ can qualify for both.
So "precedence mutation reddens" holds for pinned-vs-type and community-vs-type, and not for pinned-vs-community.
One fixture line fixes it, and I verified the fix rather than proposing it blind:
- added
pod('pinned-community', 'Pinned HQ', 'team', […], { communityListed: true })withmockPinned.add('pinned-community'), asserted under thepinnedheading → still 5 passed on the unmutated code - re-ran the same precedence swap with that fixture present → 1 red, the grouping test
That is the whole change: one pod that is both, and one assertion that it lands under pinned.
Not blocking, but worth landing before the press
The code is correct today. The gap is that the one rule a human was asked to arbitrate is the one rule no test holds, so the next refactor of that ternary is unguarded. Cheap to close now, and it is the same shape as the .v2-thread binding: correct behaviour, guard that cannot see the property it was written for.
1 participant
TASK-129 PR 1 — sidebar
Builds the Workspace · clickable artboard’s sidebar only. Inspector and chat remain for PRs 2 and 3.
Checks at 50d953f
UX gate
Needs signed-in 1440 and 390 walks against the Workspace artboard. Requested live-state captures: selected Sharpen with open decisions and live Telegram binding.