[liviaUeno]

Context

The Tests workflow was failing on develop in the Vitest packages/shared-components job: 2 outdated snapshots. 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: the creation actions are now hidden for users who can't create rooms.

• RoomListHeaderView > renders without compose menu: the compose button no longer renders when displayComposeMenu is false (in the fork it follows isAdmin), so the snapshot was regenerated. The NoComposeMenu story is unchanged: it sets only that flag and inherits canCreateRoom: true from the default snapshot, which models a non-admin on Home. (An earlier revision of this PR changed the story args based on a mistaken premise that this state cannot occur; that was reverted.)
• RoomListView > renders EmptyWithoutCreatePermission story: the "Start chat" button no longer renders in the empty state when snapshot.canCreateRoom is false.

Changes made in response to review:

• The empty-state copy for users without create rights said "Get started by messaging someone", while the only way to do that was hidden. The copy is now "Rooms you join will show up here". It deliberately does not prescribe an action, because the Explore button only renders on the Home space.
• Since the meaning of the string changed, its key was renamed from no_chats_description_no_room_rights to no_chats_description_joined_rooms, and the old key was removed from the other locales (they fall back to English until translated).
• The canCreateRoom guard now wraps the whole Flex, so no empty container is rendered.
• Removed the commented-out button and its stale comment in RoomListHeaderView.tsx; the ternary is now {displayComposeMenu && <ComposeMenuView vm={vm} />}. The comment on displayComposeMenu in RoomListHeaderViewModel.ts now explains why it is gated on isAdmin rather than canCreateRoom.
• Added an explicit assertion that "Start chat" and "New room" are hidden without create rights, plus a control test that they are shown with it, so the permission gating no longer depends on a snapshot alone.
• Applied Prettier to the files touched by this PR so it adds no new lint:prettier noise.
• Updated the stale displayComposeMenu docs (property comment and component docstring) in RoomListHeaderView.tsx to say it is gated on global admins in this fork.
• The fallback (default:) branch of the empty-state filter switch now reuses the same canCreateRoom-dependent description, so it can no longer invite users to create a room when the create actions are hidden.

A note on "Start chat" disappearing

A user without room-creation permission can still technically start a DM through other entry points in the app, so this change doesn't close every path. The intent is narrower: removing the prominent "Start chat" button from the empty state removes the invitation to start a private conversation. My first version of this PR left the copy in place, which contradicted the hidden button; that is fixed (see above). If the requirement is a hard block on DMs from every entry point, that is a separate, broader piece of work (auditing every place a DM can be started), not something this PR attempts.

Follow-up (not part of this PR)

The fork has two different canCreateRoom rules (isGlobalAdmin() for the empty state, hasCreateRoomRights() for the header), which can disagree for a global admin in a space without the power level. Tracked in Basecamp.

How this was verified

• pnpm test:unit: 91 test files passed, 739 tests passed, 6 skipped.
• Reviewed each snapshot diff before regenerating with -u.
• Checked that the new assertion catches a regression: temporarily making the create actions render regardless of canCreateRoom made it fail with expected <button> to be null (the "Start chat" button). Reverted afterwards.

Note

This PR only fixes the Vitest packages/shared-components job, so the Tests aggregate stays red after merge. Per @zZMathSP's review (he checked their logs; nothing there points at these snapshots), these checks are already failing on develop for unrelated reasons:

• Analyse Dead Code
• ESLint
• Prettier
• TypeScript Syntax Check
• Jest (Element Web) (1)
• Jest (Element Web) (2)

Build and Static Analysis / i18n Check are also red on develop and are tracked separately.

Done when: Vitest packages/shared-components is green.

[zZMathSP]

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.

[zZMathSP]

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:

1. 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
2. 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.

[liviaUeno]

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

[zZMathSP]

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.

[liviaUeno]

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

[zZMathSP]

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.

[liviaUeno]

Done: 382c123

[zZMathSP]

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.

[liviaUeno]

Okay! Done: 860d644

[zZMathSP]

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.

[liviaUeno]

Done: 382c123

[zZMathSP]

Thanks for the follow-through here. All five points from the last round are addressed, and the control test (proving the button names are real before asserting they're absent) goes a step beyond what I asked for. That's the right instinct: a queryByRole with a typo in the name would pass silently, and you closed that door.

One correction on my side first. In my comment on the NoComposeMenu story I said "can create rooms, but has no compose menu" isn't a state a real user can be in. That was wrong for this fork; see the inline note on the story. Your change is harmless, but I don't want a wrong premise of mine to live on in the PR description as fact.

Then two things this PR itself introduces that I'd like fixed before merge: the new empty-state copy promises an Explore button that isn't always there (same class of problem we fixed in round one), and the other locales still carry the old meaning of the string. Plus a Prettier nit, since the point of this PR is a greener CI. Details inline, along with a couple of follow-ups that don't belong in this PR.

CI on the head commit: Vitest packages/shared-components is green, and the remaining red jobs are the same ones already red on develop.

[zZMathSP]

Same question as last round, applied to the new string: what does the surrounding UI now promise? "Explore public rooms to find one to join" points the user at the Explore button, but that button only exists under two conditions, see apps/web/src/viewmodels/room-list/RoomListSearchViewModel.ts:51:

const displayExploreButton = activeSpace === MetaSpace.Home && shouldShowComponent(UIComponent.ExploreRooms);

So a non-admin whose active space is anything other than Home is told to explore public rooms with no Explore button in sight. Different text, same bug shape as the one we just fixed.

Two ways out, both fine:

1. Render the action right here instead of describing it. This file already has ActionPlaceholder for exactly this pattern (title + one button). The view model would need an onExplore-style callback, so it's a bit more work.
2. Pick copy that doesn't prescribe an action the screen can't offer, e.g. "You haven't joined any rooms yet" / "Rooms you join will show up here". Smallest change, no promise to break.

I'd go with 2 for this PR. The habit to take away: when copy names a UI element, grep for the condition that renders that element before committing the string.

[liviaUeno]

Done: 626964d

[zZMathSP]

Changing the English value in place changed the meaning of no_chats_description_no_room_rights, but the key stayed the same, so every other locale kept its old translation. Right now de_DE, fr, ru, pl, hu, cs, et, cy, ko, nb_NO and zh_Hans still say "start by messaging someone" for this key. A German user gets Lege los, indem du jemandem schreibst under a screen with no way to message anyone. The i18n check passed because it validates structure, not meaning.

Upstream's convention for this case: when a string's meaning changes, rename the key (e.g. no_chats_description_no_room_rights → no_chats_description_explore, or whatever fits the final copy). The other locales then fall back to English until someone translates them, which is the honest state. We don't have Localazy in this fork, so stale translations would otherwise stay forever.

Practical steps: rename the key here and in the _t() call, delete the old key from the locale files that have it (grep -rn no_chats_description_no_room_rights src/i18n/strings lists them), run pnpm i18n in packages/shared-components to re-sort and lint, and regenerate the snapshot.

[liviaUeno]

Done: 626964d

[zZMathSP]

Nit, but relevant for a PR whose job is a greener CI: there's a trailing space after isAdmin; on this line, and the new it("hides the create actions...") block in RoomListView.test.tsx is indented 8 spaces instead of 4. The Prettier job already fails on develop for other reasons, but its log for this PR's head lists exactly these two files, so we'd be adding new noise to the pile we're trying to shrink. pnpm exec prettier --write on the touched files (or format-on-save in your editor) sorts both.

Tip: pnpm lint:prettier at the repo root is the same check CI runs. Worth running before pushing, so any new red is always yours to see first.

[liviaUeno]

Done: 96dbe6b. pnpm lint:prettier is clean on the files touched by this PR (checked with prettier --check on the PR’s changed files). Will run it before pushing from now on.

[zZMathSP]

Good comment, it explains the why. Writing it down also surfaces something worth a follow-up (not this PR): the fork now has two different canCreateRooms.

• RoomListViewModel.ts:190: canCreateRoom = await isGlobalAdmin(), drives the empty state.
• Here: canCreateRoom = hasCreateRoomRights(matrixClient, activeSpace), the Matrix power level, drives the items inside ComposeMenuView, with displayComposeMenu = isAdmin layered on top.

Concrete case where they disagree: a global admin inside a space where they lack the power level for m.room.avatar. The empty state shows "New room" (admin ⇒ true) while the compose menu in the header opens but hides "New room" (power level ⇒ false). Same panel, two answers.

The fix is to define the rule once, probably isAdmin && hasCreateRoomRights(...) in room-list/utils.ts, and have both view models derive from it. Could you open an issue so it isn't lost? General lesson: when a comment has to say "we use X here instead of Y", check whether Y is still deciding the same thing somewhere else.

[liviaUeno]

Good catch, thanks. I have registered it as a follow-up on Basecamp (“[Refactor] Unificar a regra de “pode criar sala” (canCreateRoom)”), proposing to define the rule once (isAdmin && hasCreateRoomRights(...) in room-list/utils.ts) and derive both view models from it.

[zZMathSP]

Correction on my side. I said "can create rooms, but has no compose menu" isn't a state a real user can be in. In this fork it is, and it's the common one: displayComposeMenu comes from isGlobalAdmin(), while canCreateRoom in this snapshot comes from hasCreateRoomRights(), which returns true for anyone on Home with no active space (utils.ts:58-60). So the original story (displayComposeMenu: false only) was already a real state: a non-admin on Home.

Your change is harmless. The two extra args are never read when the menu isn't mounted, and "non-admin in a space without power level" is also a real state. Keep it or revert it, your call. What I'd fix is the PR description, which now presents my wrong premise as the reason for the change. "Reviewer was mistaken; the story args are a no-op either way" is a perfectly fine thing to write there. The lesson from last round still stands (a story is a claim about a state of the world), it just cut the other way this time.

[liviaUeno]

Done: fe1ad4e. Since the extra args were a no-op, I reverted the story to its original form (it already represented a real state: a non-admin on Home) and removed the story change from the PR description.

[zZMathSP]

You updated the comment in the view model, but the shared-components side still tells the old story. Line 36 of this file documents displayComposeMenu as "True if the user can create rooms", and the component docstring at line 144 says "a compose menu is shown if the user can create rooms". Both are now wrong, and this interface is what Storybook autodocs shows to the next person writing a view model or a story.

Suggestion for both places: "True if the user may use the compose menu (global admins only in this fork)", or similar. Rule of thumb from last round, extended: when you fix a stale comment, grep for the same claim elsewhere. grep -rn "can create rooms" packages/shared-components/src/room-list finds both.

[liviaUeno]

Done: 2ca927c. Updated both places to “True if the user may use the compose menu (global admins only in this fork)” / “…if the user may use it (global admins only in this fork)”.

[zZMathSP]

Minor, and not a bug today. The default: branch of the filter switch further down still renders no_chats_description ("Get started by messaging someone or by creating a room") regardless of canCreateRoom. No current filter reaches it, so nothing is wrong now, but the next FilterId someone adds without a case here would invite non-admins to create rooms with no button. Cheapest fix is to reuse the same ternary you have at lines 40-43. Fine to fold into this PR or leave a short comment explaining the gap; just don't let it be a surprise later.

[liviaUeno]

Done: 1602982