Conversation
zZMathSP
left a comment
There was a problem hiding this comment.
Thanks for this, and especially for the PR description. Reviewing the snapshot diff before running -u, and writing down your reasoning about the "Start chat" button, is exactly the right instinct. Most snapshot-update PRs don't get that care, and it made this review much easier.
That said, I don't think we should merge it as is. The short version: the snapshots do match what the components render, but what they render has a couple of problems that this PR would be formalising rather than fixing. I've left inline comments explaining each one and what I'd change. They're all small, and I think they fit in this PR.
One thing about the "Done when: Tests green on develop" goal: this change only fixes the Vitest packages/shared-components job. Jest, ESLint, Prettier, the TS check, Build and dead-code analysis are already red on develop at 324eff9 for unrelated reasons (I checked their logs; nothing there points at these snapshots). So the Tests aggregate will still be red after merge. Please scope the claim in the description to the Vitest job, and if you can, link the remaining failures so they're tracked separately rather than assumed fixed.
Happy to pair on any of this if something isn't clear.
| Start chat | ||
| </button> | ||
| </div> | ||
| /> |
There was a problem hiding this comment.
This is the most important thing I'd like you to look at, and it's a good example of why "the snapshot matches what the component renders" is not the same as "the component renders the right thing".
Look at what this empty state now says to a non-admin user. A few lines above, GenericPlaceholder receives the description room_list|empty|no_chats_description_no_room_rights, which in en_EN.json reads "Get started by messaging someone". And right below it, the only affordance to message someone (the "Start chat" button) is gone. So the screen tells the user to do something and then gives them no way to do it here.
Your PR description actually reasons about this very well (DMs are still possible through other entry points, this is about removing the invitation). But the copy is still an invitation. So we need to pick one:
- keep the string and bring back the DM button for this case (if a non-admin may start a DM, the empty state is a fine place to offer it), or
- keep the button hidden and change the string so it no longer asks the user to message someone.
Either is defensible; what we shouldn't do is lock in the contradiction with -u. A useful habit: when a snapshot diff removes an interactive element, ask "what does the surrounding text now promise?" before accepting it.
There was a problem hiding this comment.
Good catch on the contradiction. I hadn't connected the button change back to what the copy was still promising.
I went with option 2, keeping the button hidden and updating the string to point users toward exploring public rooms instead, so it's not asking them to do something they no longer have a way to do here.
Done: 4acdd21
| @@ -3397,29 +3397,7 @@ exports[`<RoomListView /> > renders EmptyWithoutCreatePermission story 1`] = ` | |||
| <div | |||
| class="Flex-module_flex RoomListEmptyStateView-module_defaultPlaceholder" | |||
| style="--mx-flex-display: flex; --mx-flex-direction: column; --mx-flex-align: center; --mx-flex-justify: center; --mx-flex-gap: var(--cpd-space-4x); --mx-flex-wrap: nowrap;" | |||
There was a problem hiding this comment.
Small one, but worth understanding. Notice the <div class="... defaultPlaceholder" ... /> here is now self-closing: it renders with nothing inside. That's because in RoomListEmptyStateView.tsx both buttons are wrapped in their own snapshot.canCreateRoom && (...) guard, so when the flag is false the Flex still mounts, just empty. An empty flex container isn't free: it still occupies its slot in the layout and carries its gap and margins.
Since both children share the exact same condition, the cleaner shape is to hoist it once around the whole Flex:
{snapshot.canCreateRoom && (
<Flex className={styles.defaultPlaceholder} align="center" justify="center" direction="column" gap="var(--cpd-space-4x)">
<Button size="md" kind="secondary" Icon={ChatIcon} onClick={vm.createChatRoom}>
{_t("action|start_chat")}
</Button>
<Button size="md" kind="secondary" Icon={RoomIcon} onClick={vm.createRoom}>
{_t("action|new_room")}
</Button>
</Flex>
)}Two wins: no duplicated check, and no empty element in the DOM (this snapshot gets shorter too). Rule of thumb: when two siblings have the same guard, the guard usually belongs on the parent.
There was a problem hiding this comment.
Thank you for such a thorough explanation, I really appreciate it. This helped me understand not just the fix but the reasoning behind it.
Done: b595961
| @@ -3397,29 +3397,7 @@ exports[`<RoomListView /> > renders EmptyWithoutCreatePermission story 1`] = ` | |||
| <div | |||
There was a problem hiding this comment.
A thought about test design, prompted by how this PR came to exist. The permission gating you describe ("Start chat" / "New room" hidden when canCreateRoom is false) is currently protected only by this snapshot. Snapshots are great at catching "something changed", but terrible at explaining what was supposed to be true. When one fails, the path of least resistance is vitest -u, which is exactly what happened here. It also means a real regression (someone accidentally re-adding the button) would fail the same way and could be "fixed" the same way.
Next to the snapshot in RoomListView.test.tsx, add an assertion that says the intent out loud:
it("hides the create actions when the user cannot create rooms", () => {
renderWithMockContext(<EmptyWithoutCreatePermission />);
expect(screen.queryByRole("button", { name: "Start chat" })).toBeNull();
expect(screen.queryByRole("button", { name: "New room" })).toBeNull();
});Now a future failure reads as "the Start chat button is back for users without permission", and nobody will reach for -u to silence it. Snapshots for shape, explicit assertions for behaviour.
| </svg> | ||
| </div> | ||
| </button> | ||
| <button |
There was a problem hiding this comment.
The button that disappeared from this snapshot didn't disappear from the source. Open RoomListHeaderView.tsx around line 191 and you'll find it commented out inside the else branch of the ternary, right under a comment that says "If we don't display the compose menu, it means that the user can only send DM". That sentence is no longer true: the user gets nothing.
Two reasons to clean this up as part of this PR, since you're the one formalising the new behaviour:
- Commented-out code is a lie waiting to happen. Git already remembers the old button; the file doesn't need to.
- A stale comment is worse than no comment, because the next reader will trust it.
The ternary collapses to a single line:
{displayComposeMenu && <ComposeMenuView vm={vm} />}While you're there, RoomListHeaderViewModel.ts line 321 has the same smell: const displayComposeMenu = isAdmin; // canCreateRoom;. Either the trailing comment explains why it's isAdmin and not canCreateRoom, or it should go.
| </svg> | ||
| </div> | ||
| </button> | ||
| </div> |
There was a problem hiding this comment.
Let's check what this snapshot actually exercises versus what the PR description says it does. The description says the create button no longer renders "when the user lacks permission to create rooms/spaces". But the NoComposeMenu story in RoomListHeaderView.stories.tsx only sets displayComposeMenu: false and inherits everything else from default-snapshot.ts, where canCreateRoom is true. So the state rendered here is "can create rooms, but has no compose menu", which isn't a state a real user can be in.
The snapshot is still correct (that's what the component renders for those props), but the story doesn't test the scenario you're describing, so a reader who trusts the PR text gets the wrong idea about what's covered.
Either add a story that models the real case (canCreateRoom: false, displayComposeMenu: false) and snapshot that, or adjust the description so it says what the story really is. Also, the description mentions the "AO VIVO" live chip as a cause of the staleness, but nothing in this diff touches it. Worth double-checking before merge so the history stays accurate.
General lesson: a story is a claim about a state of the world. Make sure the args describe a state that can actually happen.
test: update stale snapshots in RoomListView / RoomListHeaderView
Context
The
Testsworkflow was failing ondevelopin theVitest packages/shared-componentsjob — 2 outdated snapshots, out of 737 passingtests. Last broken run:
https://github.com/Buzzlabs/element-web/actions/runs/34639681717
What changed
Two snapshots were stale after fork-specific changes to the room list
components (room/space creation permission blocking, and the "AO VIVO" live
chip on room list items):
RoomListHeaderView > renders without compose menu— the create button nolonger renders when the user lacks permission to create rooms/spaces.
RoomListView > renders EmptyWithoutCreatePermission story— the "Startchat" button no longer renders in the empty state when
snapshot.canCreateRoomis false.A note on "Start chat" disappearing
Worth calling out explicitly, since it wasn't immediately obvious this was
intended: a user without room-creation permission can still technically
start a DM through other entry points in the app — this change doesn't
close every path. The intent here is narrower: removing the prominent
"Start chat" button from the empty state removes the invitation to start
a private conversation when the user isn't meant to be creating
rooms/spaces, rather than attempting to block DMs outright everywhere. If
the requirement is a hard block on DMs regardless of entry point, that's a
separate, broader piece of work (auditing every place a DM can be started),
not something this PR attempts.
How this was verified
Ran the tests locally (
pnpm test:unit), reviewed the diff for bothsnapshots before updating (not just accepting blindly), confirmed both
matched the expected behaviour, then regenerated with
-u.Note
This PR only fixes the
Vitest packages/shared-componentsjob. Thefollowing checks are already failing on
developfor unrelated reasons:Done when: Vitest packages/shared-components is green.