diff --git a/chat-frontend/scripts/asyncJob.smoke.mjs b/chat-frontend/scripts/asyncJob.smoke.mjs new file mode 100644 index 000000000..b18592929 --- /dev/null +++ b/chat-frontend/scripts/asyncJob.smoke.mjs @@ -0,0 +1,256 @@ +// Wire-level smoke test for src/lib/asyncJob.js against a real NATS server. +// Not part of the standing test suite — invoke ad-hoc when verifying that +// the helper survives contact with the actual nats.ws WebSocket transport, +// header serialization, and response-subject delivery. +// +// Prereqs: a NATS server with WebSocket on ws://localhost:8443. +// /root/go/bin/nats-server -c /tmp/nats.conf +// +// Usage: node scripts/asyncJob.smoke.mjs + +import { connect, StringCodec } from 'nats.ws' +import { requestWithAsyncResult, ASYNC_JOB_ERROR_KINDS } from '../src/lib/asyncJob.js' +import { isDMExistsReply } from '../src/lib/constants.js' + +const WS_URL = process.env.NATS_WS_URL || 'ws://localhost:8443' +const sc = StringCodec() + +let pass = 0 +let fail = 0 + +function check(label, ok, detail = '') { + const tag = ok ? 'PASS' : 'FAIL' + console.log(` [${tag}] ${label}${detail ? ' — ' + detail : ''}`) + if (ok) pass++ + else fail++ +} + +async function withResponder(nc, subject, handler) { + const sub = nc.subscribe(subject) + ;(async () => { + for await (const msg of sub) { + try { + await handler(msg) + } catch (err) { + console.error('responder error:', err) + } + } + })() + return sub +} + +async function main() { + console.log(`Connecting to ${WS_URL}…`) + const nc = await connect({ servers: WS_URL }) + + // ── case 1: full happy path ──────────────────────────────────────────────── + console.log('\n[1] happy-path: sync accept + async ok') + { + let observedHeader = null + let observedSubject = null + const responder = await withResponder( + nc, + 'chat.user.*.request.room.*.create', + async (msg) => { + observedSubject = msg.subject + observedHeader = msg.headers?.get('X-Request-ID') ?? null + // sync reply: accepted + msg.respond(sc.encode(JSON.stringify({ + status: 'accepted', + roomId: 'r-new', + roomType: 'channel', + }))) + // async reply: publish to response subject — derive account from the + // request subject (chat.user.{a}.request.room.{site}.create). + const acct = msg.subject.split('.')[2] + const responseSubject = `chat.user.${acct}.response.${observedHeader}` + // Tiny gap so the client's async-result wait actually waits. + setTimeout(() => { + nc.publish(responseSubject, sc.encode(JSON.stringify({ + requestId: observedHeader, + operation: 'room.create', + status: 'ok', + roomId: 'r-new', + timestamp: Date.now(), + }))) + }, 50) + } + ) + + const result = await requestWithAsyncResult( + nc, + 'alice', + 'chat.user.alice.request.room.site-A.create', + { name: 'frontend', users: [], orgs: [], channels: [] } + ) + check('request reached responder', !!observedSubject, observedSubject) + check('X-Request-ID header survived the wire', !!observedHeader && observedHeader.length >= 30, + observedHeader) + check('sync reply parsed', result.sync.status === 'accepted', + `roomId=${result.sync.roomId} roomType=${result.sync.roomType}`) + check('async result delivered', result.async?.status === 'ok', + `operation=${result.async?.operation} roomId=${result.async?.roomId}`) + check('requestId on AsyncJobResult matches header', + result.async?.requestId === observedHeader, + `${result.async?.requestId} vs ${observedHeader}`) + + await responder.unsubscribe() + } + + // ── case 2: DM-exists treatAsSuccess branch ──────────────────────────────── + console.log('\n[2] DM-exists: {error, roomId} reply is treated as success') + { + const responder = await withResponder( + nc, + 'chat.user.*.request.room.*.create', + async (msg) => { + msg.respond(sc.encode(JSON.stringify({ + error: 'dm already exists', + roomId: 'r-existing', + }))) + } + ) + + let err + let result + try { + result = await requestWithAsyncResult( + nc, + 'alice', + 'chat.user.alice.request.room.site-A.create', + { name: '', users: ['bob'], orgs: [], channels: [] }, + { treatAsSuccess: isDMExistsReply } + ) + } catch (e) { + err = e + } + check('did not throw', !err, err?.message) + check('sync.error preserved', result?.sync?.error === 'dm already exists') + check('sync.roomId preserved', result?.sync?.roomId === 'r-existing') + check('async is null (no follow-up)', result?.async === null) + await responder.unsubscribe() + } + + // ── case 3: async error ──────────────────────────────────────────────────── + console.log('\n[3] async error: AsyncJobResult.status=error bubbles up') + { + const responder = await withResponder( + nc, + 'chat.user.*.request.room.*.*.member.add', + async (msg) => { + const reqId = msg.headers?.get('X-Request-ID') + msg.respond(sc.encode(JSON.stringify({ status: 'accepted' }))) + const acct = msg.subject.split('.')[2] + setTimeout(() => { + nc.publish(`chat.user.${acct}.response.${reqId}`, sc.encode(JSON.stringify({ + requestId: reqId, + operation: 'room.member.add', + status: 'error', + error: 'only owners can add members', + timestamp: Date.now(), + }))) + }, 25) + } + ) + + let err + try { + await requestWithAsyncResult( + nc, + 'alice', + 'chat.user.alice.request.room.r1.site-A.member.add', + { roomId: 'r1', users: ['bob'], orgs: [], channels: [], history: { mode: 'all' } } + ) + } catch (e) { + err = e + } + check('threw with server error message', /only owners/.test(err?.message ?? ''), err?.message) + check('err.kind = async-error', err?.kind === ASYNC_JOB_ERROR_KINDS.AsyncError, err?.kind) + await responder.unsubscribe() + } + + // ── case 4: async timeout (no responder publishes AsyncJobResult) ────────── + console.log('\n[4] async timeout: helper rejects + cleans up subscription') + { + const responder = await withResponder( + nc, + 'chat.user.*.request.room.*.*.member.remove', + async (msg) => { + msg.respond(sc.encode(JSON.stringify({ status: 'accepted' }))) + // deliberately do NOT publish an AsyncJobResult + } + ) + + const t0 = Date.now() + let err + try { + await requestWithAsyncResult( + nc, + 'alice', + 'chat.user.alice.request.room.r1.site-A.member.remove', + { roomId: 'r1', account: 'bob' }, + { asyncTimeout: 400 } + ) + } catch (e) { + err = e + } + const dt = Date.now() - t0 + check('err.kind = async-timeout', err?.kind === ASYNC_JOB_ERROR_KINDS.AsyncTimeout, err?.kind) + check('rejection happened ~asyncTimeout (≥350ms, ≤2000ms)', dt >= 350 && dt <= 2000, `${dt}ms`) + await responder.unsubscribe() + } + + // ── case 5: subscribe-before-publish ordering ────────────────────────────── + console.log('\n[5] subscribe-before-publish: AsyncJobResult published immediately is still received') + { + // Race condition probe: responder publishes the async result the same tick + // it sends the sync reply. If the helper subscribes AFTER the request, the + // async result is lost. If it subscribes BEFORE (as designed), it lands. + const responder = await withResponder( + nc, + 'chat.user.*.request.room.*.create', + async (msg) => { + const reqId = msg.headers?.get('X-Request-ID') + const acct = msg.subject.split('.')[2] + // Publish async result FIRST, then sync reply. The helper must have + // already subscribed to the response subject for this to work. + nc.publish(`chat.user.${acct}.response.${reqId}`, sc.encode(JSON.stringify({ + requestId: reqId, + operation: 'room.create', + status: 'ok', + roomId: 'r-fast', + timestamp: Date.now(), + }))) + msg.respond(sc.encode(JSON.stringify({ + status: 'accepted', + roomId: 'r-fast', + roomType: 'channel', + }))) + } + ) + + let result, err + try { + result = await requestWithAsyncResult( + nc, + 'alice', + 'chat.user.alice.request.room.site-A.create', + { name: 'fast', users: [], orgs: [], channels: [] }, + { asyncTimeout: 1000 } + ) + } catch (e) { err = e } + check('completed without timeout (sub was open in time)', !err, err?.message) + check('async.status=ok', result?.async?.status === 'ok') + await responder.unsubscribe() + } + + await nc.drain() + + console.log(`\n=== ${pass} passed, ${fail} failed ===`) + process.exit(fail === 0 ? 0 : 1) +} + +main().catch((err) => { + console.error('FATAL:', err) + process.exit(2) +}) diff --git a/chat-frontend/scripts/liveStack.smoke.mjs b/chat-frontend/scripts/liveStack.smoke.mjs new file mode 100644 index 000000000..ca3da707a --- /dev/null +++ b/chat-frontend/scripts/liveStack.smoke.mjs @@ -0,0 +1,173 @@ +// End-to-end smoke against the LIVE stack: auth-service + room-service + room-worker. +// Verifies that what the unit suite mocks actually happens on the wire: +// - POST /auth (dev mode) returns a NATS JWT +// - the frontend's requestWithAsyncResult creates a real channel via +// chat.user.{a}.request.room.{site}.create +// - room-worker materializes the room and emits AsyncJobResult +// - chat.user.{a}.request.room.{r}.{s}.member.list?enrich=true returns +// the seeded individuals + the requester as owner +// +// Prereqs: +// * NATS running with WebSocket on ws://localhost:9222 +// * auth-service in DEV_MODE on http://localhost:8080 +// * room-service + room-worker connected to NATS + Mongo +// * Users alice/bob/charlie seeded in Mongo +// +// Usage: npx vite-node scripts/liveStack.smoke.mjs + +import { connect, StringCodec, jwtAuthenticator } from 'nats.ws' +import { createUser } from 'nkeys.js' +import { requestWithAsyncResult } from '../src/lib/asyncJob.js' +import { roomCreate, memberList } from '../src/lib/subjects.js' +import { isDMExistsReply } from '../src/lib/constants.js' + +const AUTH_URL = process.env.AUTH_URL || 'http://localhost:8080' +const NATS_WS = process.env.NATS_WS_URL || 'ws://localhost:9222' +const sc = StringCodec() + +let pass = 0 +let fail = 0 +function check(label, ok, detail = '') { + const tag = ok ? 'PASS' : 'FAIL' + console.log(` [${tag}] ${label}${detail ? ' — ' + detail : ''}`) + if (ok) pass++; else fail++ +} + +async function devLogin(account) { + const nkey = createUser() + const natsPublicKey = nkey.getPublicKey() + const resp = await fetch(`${AUTH_URL}/auth`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ account, natsPublicKey }), + }) + if (!resp.ok) throw new Error(`auth failed: ${resp.status} ${await resp.text()}`) + const { natsJwt, user } = await resp.json() + return { natsJwt, user, seed: nkey.getSeed() } +} + +async function main() { + console.log(`Auth: ${AUTH_URL} | NATS-ws: ${NATS_WS}`) + + // ── 1. dev-login as alice ────────────────────────────────────────────────── + console.log('\n[1] dev-login as alice') + const { natsJwt, user, seed } = await devLogin('alice') + check('auth returned a JWT', !!natsJwt && natsJwt.length > 50) + check('auth returned user.account=alice', user?.account === 'alice', `id=${user?.id}`) + + // ── 2. open a NATS WS connection authenticated by the JWT ────────────────── + console.log('\n[2] connect NATS WebSocket with returned JWT') + const nc = await connect({ + servers: NATS_WS, + authenticator: jwtAuthenticator(natsJwt, seed), + }) + check('connected', !!nc, nc.getServer()) + + // ── 3. create a channel — real room-service + room-worker ────────────────── + console.log('\n[3] create a channel via the frontend helper, live stack') + const channelName = `e2e-${Date.now()}` + let createResult, createErr + try { + createResult = await requestWithAsyncResult( + nc, + 'alice', + roomCreate('alice', 'site-local'), + { name: channelName, users: ['bob', 'charlie'], orgs: [], channels: [] }, + { asyncTimeout: 10000 } + ) + } catch (e) { createErr = e } + if (createErr) { + check('create succeeded', false, createErr.message) + } else { + check('sync.status = accepted', createResult.sync?.status === 'accepted', `roomId=${createResult.sync?.roomId}`) + check('sync.roomType = channel', createResult.sync?.roomType === 'channel') + check('async result is ok', createResult.async?.status === 'ok', + `op=${createResult.async?.operation} requestId=${createResult.async?.requestId?.slice(0, 8)}…`) + check('async.roomId matches sync.roomId', createResult.async?.roomId === createResult.sync?.roomId) + } + + // ── 4. member.list?enrich on the new room ────────────────────────────────── + console.log('\n[4] member.list?enrich on the new room') + if (createResult?.sync?.roomId) { + const roomId = createResult.sync.roomId + const listSubject = memberList('alice', roomId, 'site-local') + let listResult + try { + const resp = await nc.request(listSubject, sc.encode(JSON.stringify({ enrich: true })), { timeout: 5000 }) + listResult = JSON.parse(sc.decode(resp.data)) + } catch (e) { + listResult = { error: e.message } + } + + check('member.list responded', !listResult.error, listResult.error) + if (!listResult.error) { + const members = listResult.members ?? [] + const individuals = members.filter((m) => m.member?.type === 'individual') + const accounts = individuals.map((m) => m.member?.account).sort() + check('individuals include alice + bob + charlie', + accounts.join(',') === 'alice,bob,charlie', + accounts.join(',')) + const alice = individuals.find((m) => m.member?.account === 'alice') + check('alice is owner', alice?.member?.isOwner === true) + const bob = individuals.find((m) => m.member?.account === 'bob') + check('bob is member (not owner)', bob?.member?.isOwner !== true) + check('display names enriched (engName populated)', + !!alice?.member?.engName && !!bob?.member?.engName, + `${alice?.member?.engName}, ${bob?.member?.engName}`) + } + } else { + check('member.list', false, 'skipped: no roomId from create') + } + + // ── 5. DM-exists dedup: create the same DM twice ─────────────────────────── + console.log('\n[5] DM dedup: second create with same counterpart returns existing roomId') + let dm1Result, dm2Result + try { + dm1Result = await requestWithAsyncResult( + nc, 'alice', + roomCreate('alice', 'site-local'), + { name: '', users: ['bob'], orgs: [], channels: [] }, + { asyncTimeout: 10000 } + ) + } catch (e) { dm1Result = { error: e.message } } + check('first DM accepted', dm1Result?.sync?.status === 'accepted', `roomId=${dm1Result?.sync?.roomId}`) + + try { + dm2Result = await requestWithAsyncResult( + nc, 'alice', + roomCreate('alice', 'site-local'), + { name: '', users: ['bob'], orgs: [], channels: [] }, + { asyncTimeout: 10000, treatAsSuccess: isDMExistsReply } + ) + } catch (e) { dm2Result = { error: e.message } } + const sameRoomId = dm2Result?.sync?.roomId === dm1Result?.sync?.roomId + check('second DM reply carries existing roomId', + sameRoomId, + `first=${dm1Result?.sync?.roomId} second=${dm2Result?.sync?.roomId}`) + check('second DM reply is the "dm already exists" shape', + dm2Result?.sync?.error === 'dm already exists', + dm2Result?.sync?.error) + check('second DM async is null (no follow-up)', dm2Result?.async === null) + + // ── 6. validation: empty create rejected ────────────────────────────────── + console.log('\n[6] empty payload rejected with classification error') + let empty + try { + await requestWithAsyncResult( + nc, 'alice', + roomCreate('alice', 'site-local'), + { name: '', users: [], orgs: [], channels: [] }, + { asyncTimeout: 3000 } + ) + } catch (e) { empty = e } + check('threw with empty-create error', !!empty, empty?.message) + + await nc.drain() + console.log(`\n=== ${pass} passed, ${fail} failed ===`) + process.exit(fail === 0 ? 0 : 1) +} + +main().catch((err) => { + console.error('FATAL:', err) + process.exit(2) +}) diff --git a/chat-frontend/src/components/CreateRoomDialog.jsx b/chat-frontend/src/components/CreateRoomDialog.jsx index db71bf895..678537ec2 100644 --- a/chat-frontend/src/components/CreateRoomDialog.jsx +++ b/chat-frontend/src/components/CreateRoomDialog.jsx @@ -1,52 +1,132 @@ -import { useState } from 'react' +import { useEffect, useRef, useState } from 'react' import { useNats } from '../context/NatsContext' -import { roomsCreate } from '../lib/subjects' -import { parseList } from '../lib/parseList' +import { useRoomSummaries } from '../context/RoomEventsContext' +import { roomCreate } from '../lib/subjects' +import { isDMExistsReply } from '../lib/constants' +import { formatAsyncJobError } from '../lib/asyncJob' +import MemberPicker from './MemberPicker' +// How long to wait for the server's subscription.update event before +// giving up and closing the dialog anyway. With the event the user lands +// in a room that already has its channel subscription opened (so messages +// echo back immediately); without it, the message stream is racy for the +// first second or so, but at least the dialog doesn't hang forever. +const SUBSCRIPTION_WAIT_TIMEOUT_MS = 3000 + +// Type inferred server-side from payload shape (name vs. members). export default function CreateRoomDialog({ onClose, onCreated }) { - const { user, request } = useNats() + const { user, requestWithAsyncResult } = useNats() + const { summaries } = useRoomSummaries() const [name, setName] = useState('') - const [roomType, setRoomType] = useState('channel') - const [members, setMembers] = useState('') + const [users, setUsers] = useState([]) + const [orgs, setOrgs] = useState([]) + const [channels, setChannels] = useState([]) const [loading, setLoading] = useState(false) const [error, setError] = useState(null) + // The just-created room we're waiting for confirmation on. Holds the + // {id, type, siteId, name} payload destined for onCreated. While set, + // the dialog stays open with the "Waiting for server confirmation…" + // label; once `summaries` contains a row matching `id` we fire the + // callback. ROOM_ADDED is only dispatched via the subscription.update + // event handler, which also calls openChannelSub() for channels — so by + // the time we close, the channel subscription is already open and the + // user's first message will echo back live. + const [pendingRoom, setPendingRoom] = useState(null) + const pickerRef = useRef(null) + + // Close when the room shows up in summaries (subscription.update arrived + // → ROOM_ADDED dispatched → channel sub opened). Idempotent: stays open + // until match. + useEffect(() => { + if (!pendingRoom) return + if (summaries.some((r) => r.id === pendingRoom.id)) { + onCreated(pendingRoom) + onClose() + } + }, [summaries, pendingRoom, onCreated, onClose]) + + // Safety net: if subscription.update never arrives (server bug, NATS + // outage), don't auto-select the would-be room. Calling onCreated here + // would set selectedRoom on ChatPage, but summaries doesn't contain the + // room yet so the page's auto-deselect effect would bounce the user + // back to the empty state; even if it didn't, the channel subscription + // for the new room hasn't been opened (also gated on + // subscription.update), so any message the user tried to send would + // not echo back live. Surface an error inside the dialog instead, + // clear pendingRoom so the wait state unwinds (re-enabling Cancel), + // and let the user dismiss. If the room actually was created, it'll + // appear in their sidebar shortly when subscription.update finally + // lands — they can click it there. + useEffect(() => { + if (!pendingRoom) return + const t = setTimeout(() => { + setError( + 'Room creation is taking longer than expected. If it succeeds, the room will appear in your sidebar shortly — you can dismiss this dialog.' + ) + setPendingRoom(null) + }, SUBSCRIPTION_WAIT_TIMEOUT_MS) + return () => clearTimeout(t) + }, [pendingRoom]) + + const trimmedName = name.trim() const handleSubmit = async (e) => { e.preventDefault() - if (!name.trim() || !user) return - + if (!user) return + // Auto-commit any typed-but-not-Entered text in the picker so users + // don't have to remember to press Enter before clicking Create. Use the + // returned values for this submit because the state updates queued by + // flushAndGetEntries don't land until the next render. + const { users: finalUsers, orgs: finalOrgs, channels: finalChannels } = + pickerRef.current?.flushAndGetEntries() ?? { users, orgs, channels } + if (!trimmedName && finalUsers.length + finalOrgs.length + finalChannels.length === 0) return setLoading(true) setError(null) - - const account = user.account - const siteId = user.siteId - - const memberList = parseList(members) - try { - const room = await request(roomsCreate(account), { - name: name.trim(), - type: roomType, - createdBy: account, - createdByAccount: account, - siteId: siteId, - members: memberList, - }) - onCreated(room) - onClose() + const { sync } = await requestWithAsyncResult( + roomCreate(user.account, user.siteId), + { name: trimmedName, users: finalUsers, orgs: finalOrgs, channels: finalChannels }, + { treatAsSuccess: isDMExistsReply } + ) + const roomId = sync.roomId + // On the dedup branch the server only tells us the existing roomId, not + // its type — could be either dm or botDM. Default to 'dm'; the canonical + // type arrives shortly via subscription.update. + const roomType = sync.roomType || (isDMExistsReply(sync) ? 'dm' : undefined) + // For DM/BotDM the name is empty; fall back to the counterpart account + // so the sidebar + header have something to show until the canonical + // name arrives via subscription.update. + const displayName = trimmedName || finalUsers[0] || '' + // Request is done; flip loading off so Cancel becomes re-enabled + // during the subscription.update wait. The pendingRoom state marks + // the wait phase — see the two useEffects above for the resolution + // paths (summaries-match success vs timeout error). + setPendingRoom({ id: roomId, type: roomType, siteId: user.siteId, name: displayName }) } catch (err) { - setError(err.message) + setError(formatAsyncJobError(err)) } finally { setLoading(false) } } + const buttonLabel = pendingRoom + ? 'Waiting for server confirmation…' + : loading + ? 'Creating…' + : 'Create' + return ( -
-
e.stopPropagation()}> +
{ + if (loading) return + onClose() + }} + > +
e.stopPropagation()}>

Create Room

- + - - - - - setMembers(e.target.value)} - placeholder="e.g. bob, charlie" + {error &&
{error}
}
+ {/* Cancel is enabled while waiting so users can back out of a + slow subscription.update without having to wait for the + timeout. It stays disabled only while the request itself + is in flight. */} -
diff --git a/chat-frontend/src/components/CreateRoomDialog.test.jsx b/chat-frontend/src/components/CreateRoomDialog.test.jsx new file mode 100644 index 000000000..335e465e6 --- /dev/null +++ b/chat-frontend/src/components/CreateRoomDialog.test.jsx @@ -0,0 +1,268 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest' +import { act, render, screen, fireEvent, waitFor } from '@testing-library/react' +import CreateRoomDialog from './CreateRoomDialog' + +vi.mock('../context/NatsContext', () => ({ + useNats: vi.fn(), +})) +// useRoomSummaries is consumed by the dialog so it can wait for the +// just-created room to appear in summaries (i.e. for the server's +// subscription.update event to arrive) before closing. Tests default the +// mock to summaries already containing every roomId the various success +// fixtures return; tests that exercise the wait path override this. +vi.mock('../context/RoomEventsContext', () => ({ + useRoomSummaries: vi.fn(), +})) + +import { useNats } from '../context/NatsContext' +import { useRoomSummaries } from '../context/RoomEventsContext' + +// Pre-populate summaries with the roomIds the success fixtures return so +// the dialog's "wait for subscription.update" useEffect resolves on the +// first render after submit and the happy-path tests pass synchronously. +const DEFAULT_SUMMARIES = [ + { id: 'r-new', name: 'frontend', type: 'channel', siteId: 'site-A' }, + { id: 'r-dm', name: '', type: 'dm', siteId: 'site-A' }, + { id: 'r-existing', name: '', type: 'dm', siteId: 'site-A' }, +] + +function setup(overrides = {}) { + const requestWithAsyncResult = vi.fn().mockResolvedValue({ + sync: { status: 'accepted', roomId: 'r-new', roomType: 'channel' }, + async: { status: 'ok', roomId: 'r-new', operation: 'room.create' }, + }) + const request = vi.fn() + useNats.mockReturnValue({ + user: { account: 'alice', siteId: 'site-A' }, + request, + requestWithAsyncResult, + ...overrides, + }) + useRoomSummaries.mockReturnValue({ + summaries: overrides.summaries ?? DEFAULT_SUMMARIES, + }) + const onClose = vi.fn() + const onCreated = vi.fn() + render() + return { requestWithAsyncResult, request, onClose, onCreated } +} + +describe('CreateRoomDialog', () => { + beforeEach(() => { + useNats.mockReset() + useRoomSummaries.mockReset() + }) + + it('does not show a room-type dropdown — type is inferred from inputs', () => { + setup() + expect(screen.queryByLabelText(/^Type$/i)).not.toBeInTheDocument() + }) + + it('submit is always clickable; clicking on a fully-empty form is a no-op', async () => { + // Submit-button gating used to be `name || chip-count > 0`, but that meant + // a user who typed "alice" in Users (without pressing Enter) saw a + // disabled Create and wondered why. Drop the visual gate; the submit + // handler does the actual empty-check after flushing pending text. + const { requestWithAsyncResult } = setup() + const submit = screen.getByRole('button', { name: /Create/i }) + expect(submit).not.toBeDisabled() + fireEvent.click(submit) + await new Promise((r) => setTimeout(r, 30)) + expect(requestWithAsyncResult).not.toHaveBeenCalled() + }) + + it('submits a channel-shaped payload to room.{siteId}.create when a name is given', async () => { + const { requestWithAsyncResult, onCreated, onClose } = setup() + fireEvent.change(screen.getByLabelText(/Name/i), { target: { value: 'frontend' } }) + fireEvent.change(screen.getByLabelText(/Users/i), { target: { value: 'bob' } }) + fireEvent.keyDown(screen.getByLabelText(/Users/i), { key: 'Enter' }) + fireEvent.click(screen.getByRole('button', { name: /Create/i })) + await waitFor(() => expect(requestWithAsyncResult).toHaveBeenCalledTimes(1)) + expect(requestWithAsyncResult).toHaveBeenCalledWith( + 'chat.user.alice.request.room.site-A.create', + { name: 'frontend', users: ['bob'], orgs: [], channels: [] }, + expect.objectContaining({ treatAsSuccess: expect.any(Function) }) + ) + await waitFor(() => expect(onCreated).toHaveBeenCalledWith( + expect.objectContaining({ id: 'r-new', type: 'channel' }) + )) + await waitFor(() => expect(onClose).toHaveBeenCalled()) + }) + + it('submits a DM-shaped payload (empty name, one user) and surfaces roomType=dm', async () => { + const requestWithAsyncResult = vi.fn().mockResolvedValue({ + sync: { status: 'accepted', roomId: 'r-dm', roomType: 'dm' }, + async: { status: 'ok', roomId: 'r-dm', operation: 'room.create' }, + }) + useNats.mockReturnValue({ + user: { account: 'alice', siteId: 'site-A' }, + request: vi.fn(), + requestWithAsyncResult, + }) + useRoomSummaries.mockReturnValue({ summaries: DEFAULT_SUMMARIES }) + const onCreated = vi.fn() + render() + fireEvent.change(screen.getByLabelText(/Users/i), { target: { value: 'bob' } }) + fireEvent.keyDown(screen.getByLabelText(/Users/i), { key: 'Enter' }) + fireEvent.click(screen.getByRole('button', { name: /Create/i })) + await waitFor(() => + expect(requestWithAsyncResult).toHaveBeenCalledWith( + 'chat.user.alice.request.room.site-A.create', + { name: '', users: ['bob'], orgs: [], channels: [] }, + expect.objectContaining({ treatAsSuccess: expect.any(Function) }) + ) + ) + await waitFor(() => expect(onCreated).toHaveBeenCalledWith( + expect.objectContaining({ id: 'r-dm', type: 'dm' }) + )) + }) + + it('uses the counterpart account as the optimistic DM display name', async () => { + // The server will send the canonical name via subscription.update, + // but until then the room header would render blank if we passed + // the empty `name` field through. Fall back to the counterpart so + // the sidebar and header have something to show. + const { onCreated } = setup({ + requestWithAsyncResult: vi.fn().mockResolvedValue({ + sync: { status: 'accepted', roomId: 'r-dm', roomType: 'dm' }, + async: { status: 'ok', roomId: 'r-dm', operation: 'room.create' }, + }), + }) + fireEvent.change(screen.getByLabelText(/Users/i), { target: { value: 'bob' } }) + fireEvent.keyDown(screen.getByLabelText(/Users/i), { key: 'Enter' }) + fireEvent.click(screen.getByRole('button', { name: /Create/i })) + await waitFor(() => + expect(onCreated).toHaveBeenCalledWith(expect.objectContaining({ name: 'bob' })) + ) + }) + + it('treats a "dm already exists" reply as success and navigates to the existing room', async () => { + const requestWithAsyncResult = vi.fn().mockResolvedValue({ + sync: { error: 'dm already exists', roomId: 'r-existing' }, + async: null, + }) + useNats.mockReturnValue({ + user: { account: 'alice', siteId: 'site-A' }, + request: vi.fn(), + requestWithAsyncResult, + }) + useRoomSummaries.mockReturnValue({ summaries: DEFAULT_SUMMARIES }) + const onCreated = vi.fn() + const onClose = vi.fn() + render() + fireEvent.change(screen.getByLabelText(/Users/i), { target: { value: 'bob' } }) + fireEvent.keyDown(screen.getByLabelText(/Users/i), { key: 'Enter' }) + fireEvent.click(screen.getByRole('button', { name: /Create/i })) + await waitFor(() => expect(onCreated).toHaveBeenCalledWith( + expect.objectContaining({ id: 'r-existing', type: 'dm' }) + )) + await waitFor(() => expect(onClose).toHaveBeenCalled()) + expect(requestWithAsyncResult.mock.calls[0][2]).toMatchObject({ + treatAsSuccess: expect.any(Function), + }) + }) + + it('auto-flushes typed-but-not-Entered text into the request payload', async () => { + const { requestWithAsyncResult } = setup() + fireEvent.change(screen.getByLabelText(/Name/i), { target: { value: 'team' } }) + // User types but does NOT press Enter on any picker field. + fireEvent.change(screen.getByLabelText(/Users/i), { target: { value: 'alice, bob' } }) + fireEvent.change(screen.getByLabelText(/Orgs/i), { target: { value: 'eng-org' } }) + fireEvent.change(screen.getByLabelText(/Channels/i), { target: { value: 'r-x, r-y' } }) + fireEvent.click(screen.getByRole('button', { name: /Create/i })) + await waitFor(() => expect(requestWithAsyncResult).toHaveBeenCalledTimes(1)) + expect(requestWithAsyncResult.mock.calls[0][1]).toEqual({ + name: 'team', + users: ['alice', 'bob'], + orgs: ['eng-org'], + channels: [ + { roomId: 'r-x', siteId: 'site-A' }, + { roomId: 'r-y', siteId: 'site-A' }, + ], + }) + }) + + describe('subscription.update wait', () => { + it('shows "Waiting for server confirmation…" and holds the dialog open after sync ack until summaries contains the room', async () => { + // Start with the new room absent from summaries — emulates the + // window between the synchronous create ack and the asynchronous + // subscription.update event landing. + const { requestWithAsyncResult, onCreated, onClose } = setup({ summaries: [] }) + fireEvent.change(screen.getByLabelText(/Name/i), { target: { value: 'frontend' } }) + fireEvent.click(screen.getByRole('button', { name: /Create/i })) + // Wait for the sync ack to be processed (request fires + we set + // pendingRoom). + await waitFor(() => expect(requestWithAsyncResult).toHaveBeenCalledTimes(1)) + // After sync ack the button label flips to "Waiting…" and neither + // callback has fired yet — the dialog is parked, watching summaries. + expect(await screen.findByRole('button', { name: /Waiting for server confirmation/i })).toBeInTheDocument() + expect(onCreated).not.toHaveBeenCalled() + expect(onClose).not.toHaveBeenCalled() + }) + + it('after the 3-second timeout, surfaces an error in the dialog and does NOT auto-select the room', async () => { + // Calling onCreated on timeout used to bounce the user back to the + // empty state — ChatPage's auto-deselect effect kicks in because + // summaries still doesn't contain the new roomId, and the channel + // subscription wasn't opened either (so messages wouldn't echo + // back). The dialog now surfaces the slow-server condition inside + // itself; the user dismisses with Cancel, and if the room was + // actually created they pick it up from the sidebar when + // subscription.update finally lands. + vi.useFakeTimers({ shouldAdvanceTime: true }) + try { + const { onCreated, onClose } = setup({ summaries: [] }) + fireEvent.change(screen.getByLabelText(/Name/i), { target: { value: 'frontend' } }) + fireEvent.click(screen.getByRole('button', { name: /Create/i })) + await screen.findByRole('button', { name: /Waiting/i }) + expect(onCreated).not.toHaveBeenCalled() + await act(async () => { + await vi.advanceTimersByTimeAsync(3001) + }) + // Error appears, dialog stays open, neither callback fired. + expect(await screen.findByText(/taking longer than expected/i)).toBeInTheDocument() + expect(onCreated).not.toHaveBeenCalled() + expect(onClose).not.toHaveBeenCalled() + // Submit button is back to "Create" — pendingRoom has been cleared + // so the wait state is over. User can dismiss with Cancel. + expect(screen.getByRole('button', { name: /^Create$/ })).toBeInTheDocument() + expect(screen.getByRole('button', { name: /^Cancel$/ })).not.toBeDisabled() + } finally { + vi.useRealTimers() + } + }) + + it('Cancel is re-enabled during the "Waiting…" state so the user can back out without waiting for the timeout', async () => { + // While the request was in flight (loading=true) Cancel stays + // disabled — we can't safely abort a published NATS request. Once + // the sync ack lands and we're just waiting for subscription.update + // (pendingRoom set, loading=false), Cancel should re-enable. + const { onClose } = setup({ summaries: [] }) + fireEvent.change(screen.getByLabelText(/Name/i), { target: { value: 'frontend' } }) + fireEvent.click(screen.getByRole('button', { name: /Create/i })) + // Wait for the request to settle and the dialog to enter the + // pending state. + await screen.findByRole('button', { name: /Waiting/i }) + const cancel = screen.getByRole('button', { name: /^Cancel$/ }) + expect(cancel).not.toBeDisabled() + fireEvent.click(cancel) + expect(onClose).toHaveBeenCalled() + }) + }) + + it('shows the server error on a failed create and does not close', async () => { + const requestWithAsyncResult = vi.fn().mockRejectedValue(new Error('exceeds maximum capacity (50)')) + useNats.mockReturnValue({ + user: { account: 'alice', siteId: 'site-A' }, + request: vi.fn(), + requestWithAsyncResult, + }) + useRoomSummaries.mockReturnValue({ summaries: DEFAULT_SUMMARIES }) + const onClose = vi.fn() + render() + fireEvent.change(screen.getByLabelText(/Name/i), { target: { value: 'huge' } }) + fireEvent.click(screen.getByRole('button', { name: /Create/i })) + expect(await screen.findByText(/exceeds maximum capacity/i)).toBeInTheDocument() + expect(onClose).not.toHaveBeenCalled() + }) +}) diff --git a/chat-frontend/src/components/ManageMembersDialog.jsx b/chat-frontend/src/components/ManageMembersDialog.jsx index fd3abb5bc..7f9432f5c 100644 --- a/chat-frontend/src/components/ManageMembersDialog.jsx +++ b/chat-frontend/src/components/ManageMembersDialog.jsx @@ -1,20 +1,14 @@ import { useState } from 'react' import AddMembersForm from './manageMembers/AddMembersForm' -import RemoveMemberForm from './manageMembers/RemoveMemberForm' -import RoleUpdateForm from './manageMembers/RoleUpdateForm' -import RemoveOrgForm from './manageMembers/RemoveOrgForm' +import MemberRoster from './manageMembers/MemberRoster' const TABS = [ - { id: 'add', label: 'Add', Form: AddMembersForm }, - { id: 'remove', label: 'Remove', Form: RemoveMemberForm }, - { id: 'role', label: 'Role', Form: RoleUpdateForm }, - { id: 'removeOrg', label: 'Remove Org', Form: RemoveOrgForm }, + { id: 'roster', label: 'Members' }, + { id: 'add', label: 'Add' }, ] export default function ManageMembersDialog({ room, onClose }) { - const [mode, setMode] = useState('add') - const active = TABS.find((t) => t.id === mode) - const ActiveForm = active.Form + const [mode, setMode] = useState('roster') return (
@@ -36,7 +30,7 @@ export default function ManageMembersDialog({ room, onClose }) { ))}
- + {mode === 'roster' ? : }
+ + ))} +
+ )} + { + setQuery(e.target.value) + setActiveIdx(0) + }} + onKeyDown={handleKeyDown} + placeholder={placeholder} + disabled={disabled} + /> + {hasDropdown && ( +
+ {results.map((r, idx) => ( +
{ + if (disabled) return + commitSingle(entryFromResult(r)) + }} + > + {renderResult(r)} +
+ ))} +
+ )} +
+ ) +}) diff --git a/chat-frontend/src/components/MemberPicker.test.jsx b/chat-frontend/src/components/MemberPicker.test.jsx new file mode 100644 index 000000000..4a10e9889 --- /dev/null +++ b/chat-frontend/src/components/MemberPicker.test.jsx @@ -0,0 +1,365 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest' +import { createRef } from 'react' +import { render, screen, fireEvent, waitFor, act } from '@testing-library/react' +import MemberPicker from './MemberPicker' + +vi.mock('../context/NatsContext', () => ({ + useNats: vi.fn(), +})) + +import { useNats } from '../context/NatsContext' + +function setup(overrides = {}) { + const request = vi.fn().mockResolvedValue({ results: [] }) + useNats.mockReturnValue({ + user: { account: 'alice', siteId: 'site-A' }, + request, + ...overrides, + }) + const onUsersChange = vi.fn() + const onOrgsChange = vi.fn() + const onChannelsChange = vi.fn() + const utils = render( + + ) + return { request, onUsersChange, onOrgsChange, onChannelsChange, ...utils } +} + +describe('MemberPicker', () => { + beforeEach(() => { + vi.clearAllMocks() + vi.useFakeTimers({ shouldAdvanceTime: true }) + }) + afterEach(() => { + vi.useRealTimers() + }) + + it('renders three labeled inputs', () => { + setup() + expect(screen.getByLabelText(/Users/i)).toBeInTheDocument() + expect(screen.getByLabelText(/Orgs/i)).toBeInTheDocument() + expect(screen.getByLabelText(/Channels/i)).toBeInTheDocument() + }) + + it('renders existing chips from props', () => { + useNats.mockReturnValue({ user: { account: 'alice', siteId: 'site-A' }, request: vi.fn() }) + render( + + ) + expect(screen.getByText('bob')).toBeInTheDocument() + expect(screen.getByText('charlie')).toBeInTheDocument() + expect(screen.getByText('eng')).toBeInTheDocument() + expect(screen.getByText(/r-x/)).toBeInTheDocument() + }) + + it('Enter on Users input commits typed value as a chip and clears the input', () => { + const { onUsersChange } = setup() + const input = screen.getByLabelText(/Users/i) + fireEvent.change(input, { target: { value: 'bob' } }) + fireEvent.keyDown(input, { key: 'Enter' }) + expect(onUsersChange).toHaveBeenCalledWith(['bob']) + expect(input.value).toBe('') + }) + + it('Enter on Users input with comma-separated text commits each segment as its own chip', () => { + const { onUsersChange } = setup() + const input = screen.getByLabelText(/Users/i) + fireEvent.change(input, { target: { value: 'alice, bob , charlie' } }) + fireEvent.keyDown(input, { key: 'Enter' }) + expect(onUsersChange).toHaveBeenCalledWith(['alice', 'bob', 'charlie']) + expect(input.value).toBe('') + }) + + it('comma-separated parsing drops empty segments and trims whitespace', () => { + const { onOrgsChange } = setup() + const input = screen.getByLabelText(/Orgs/i) + fireEvent.change(input, { target: { value: ' eng,, ops , ' } }) + fireEvent.keyDown(input, { key: 'Enter' }) + expect(onOrgsChange).toHaveBeenCalledWith(['eng', 'ops']) + }) + + it('comma-separated parsing for Channels turns each id into a local-site ChannelRef', () => { + const { onChannelsChange } = setup() + const input = screen.getByLabelText(/Channels/i) + fireEvent.change(input, { target: { value: 'r1, r2, r3' } }) + fireEvent.keyDown(input, { key: 'Enter' }) + expect(onChannelsChange).toHaveBeenCalledWith([ + { roomId: 'r1', siteId: 'site-A' }, + { roomId: 'r2', siteId: 'site-A' }, + { roomId: 'r3', siteId: 'site-A' }, + ]) + }) + + it('partial dedup on multi-add: existing chips are skipped, new ones land', () => { + const onUsersChange = vi.fn() + useNats.mockReturnValue({ user: { account: 'alice', siteId: 'site-A' }, request: vi.fn() }) + render( + + ) + fireEvent.change(screen.getByLabelText(/Users/i), { target: { value: 'alice, bob, charlie' } }) + fireEvent.keyDown(screen.getByLabelText(/Users/i), { key: 'Enter' }) + expect(onUsersChange).toHaveBeenCalledWith(['bob', 'alice', 'charlie']) + }) + + it('flushAndGetEntries also splits comma-separated pending text', () => { + const onUsersChange = vi.fn() + useNats.mockReturnValue({ user: { account: 'alice', siteId: 'site-A' }, request: vi.fn() }) + const ref = createRef() + render( + + ) + fireEvent.change(screen.getByLabelText(/Users/i), { target: { value: 'alice, bob, charlie' } }) + let merged + act(() => { + merged = ref.current.flushAndGetEntries() + }) + expect(merged.users).toEqual(['alice', 'bob', 'charlie']) + expect(onUsersChange).toHaveBeenCalledWith(['alice', 'bob', 'charlie']) + }) + + it('does not call search.users — endpoint does not exist server-side', () => { + const { request } = setup() + fireEvent.change(screen.getByLabelText(/Users/i), { target: { value: 'bo' } }) + vi.advanceTimersByTime(500) + expect(request).not.toHaveBeenCalled() + }) + + it('Enter on Channels input commits a ChannelRef using the current user siteId', () => { + const { onChannelsChange } = setup() + const input = screen.getByLabelText(/Channels/i) + fireEvent.change(input, { target: { value: 'r-x' } }) + fireEvent.keyDown(input, { key: 'Enter' }) + expect(onChannelsChange).toHaveBeenCalledWith([{ roomId: 'r-x', siteId: 'site-A' }]) + }) + + it('Enter on an empty input does nothing', () => { + const { onUsersChange } = setup() + const input = screen.getByLabelText(/Users/i) + fireEvent.keyDown(input, { key: 'Enter' }) + expect(onUsersChange).not.toHaveBeenCalled() + }) + + it('does not duplicate an already-selected value on Enter', () => { + const onUsersChange = vi.fn() + useNats.mockReturnValue({ user: { account: 'alice', siteId: 'site-A' }, request: vi.fn() }) + render( + + ) + const input = screen.getByLabelText(/Users/i) + fireEvent.change(input, { target: { value: 'bob' } }) + fireEvent.keyDown(input, { key: 'Enter' }) + expect(onUsersChange).not.toHaveBeenCalled() + }) + + it('clicking the × on a chip removes that entry', () => { + const onUsersChange = vi.fn() + useNats.mockReturnValue({ user: { account: 'alice', siteId: 'site-A' }, request: vi.fn() }) + render( + + ) + fireEvent.click(screen.getByRole('button', { name: /Remove bob/i })) + expect(onUsersChange).toHaveBeenCalledWith(['charlie']) + }) + + it('debounces search.rooms (channels) and adds a ChannelRef when a result is clicked', async () => { + const request = vi.fn().mockResolvedValue({ + results: [{ roomId: 'r-x', roomName: 'project-x', siteId: 'site-B', roomType: 'c' }], + }) + const onChannelsChange = vi.fn() + useNats.mockReturnValue({ user: { account: 'alice', siteId: 'site-A' }, request }) + render( + + ) + fireEvent.change(screen.getByLabelText(/Channels/i), { target: { value: 'pro' } }) + vi.advanceTimersByTime(250) + await waitFor(() => { + expect(request).toHaveBeenCalledWith( + 'chat.user.alice.request.search.rooms', + expect.objectContaining({ searchText: 'pro' }) + ) + }) + await waitFor(() => expect(screen.getByText('project-x')).toBeInTheDocument()) + fireEvent.click(screen.getByText('project-x')) + expect(onChannelsChange).toHaveBeenCalledWith([{ roomId: 'r-x', siteId: 'site-B' }]) + }) + + it('survives a failing search.rooms request without breaking the input', async () => { + const request = vi.fn().mockRejectedValue(new Error('no responders available')) + useNats.mockReturnValue({ user: { account: 'alice', siteId: 'site-A' }, request }) + render( + + ) + fireEvent.change(screen.getByLabelText(/Channels/i), { target: { value: 'pro' } }) + vi.advanceTimersByTime(250) + await waitFor(() => expect(request).toHaveBeenCalled()) + expect(screen.getByLabelText(/Channels/i)).not.toBeDisabled() + }) + + it('forwarded ref exposes flushAndGetEntries that captures typed-but-uncommitted text', () => { + const onUsersChange = vi.fn() + const onOrgsChange = vi.fn() + const onChannelsChange = vi.fn() + useNats.mockReturnValue({ user: { account: 'alice', siteId: 'site-A' }, request: vi.fn() }) + const ref = createRef() + render( + + ) + // User types into each field without ever pressing Enter: + fireEvent.change(screen.getByLabelText(/Users/i), { target: { value: 'bob' } }) + fireEvent.change(screen.getByLabelText(/Orgs/i), { target: { value: 'eng-org' } }) + fireEvent.change(screen.getByLabelText(/Channels/i), { target: { value: 'r-typed' } }) + + // Parent calls flush at submit time. Wrap in act so the reset() state + // updates inside the imperative call flush before the next render-state + // assertion. Without act, the input value still shows the typed text. + let merged + act(() => { + merged = ref.current.flushAndGetEntries() + }) + + expect(merged).toEqual({ + users: ['existing', 'bob'], + orgs: ['eng-org'], + channels: [{ roomId: 'r-typed', siteId: 'site-A' }], + }) + // Each field's pending text is also propagated as a chip via onChange. + expect(onUsersChange).toHaveBeenCalledWith(['existing', 'bob']) + expect(onOrgsChange).toHaveBeenCalledWith(['eng-org']) + expect(onChannelsChange).toHaveBeenCalledWith([{ roomId: 'r-typed', siteId: 'site-A' }]) + // Inputs are cleared after flush. + expect(screen.getByLabelText(/Users/i).value).toBe('') + expect(screen.getByLabelText(/Orgs/i).value).toBe('') + expect(screen.getByLabelText(/Channels/i).value).toBe('') + }) + + it('flushAndGetEntries returns current entries unchanged when no pending text', () => { + useNats.mockReturnValue({ user: { account: 'alice', siteId: 'site-A' }, request: vi.fn() }) + const ref = createRef() + render( + + ) + const merged = ref.current.flushAndGetEntries() + expect(merged).toEqual({ + users: ['bob'], + orgs: ['eng'], + channels: [{ roomId: 'r-x', siteId: 'site-A' }], + }) + }) + + it('flushAndGetEntries skips pending text that duplicates an existing chip', () => { + const onUsersChange = vi.fn() + useNats.mockReturnValue({ user: { account: 'alice', siteId: 'site-A' }, request: vi.fn() }) + const ref = createRef() + render( + + ) + fireEvent.change(screen.getByLabelText(/Users/i), { target: { value: 'bob' } }) + let merged + act(() => { + merged = ref.current.flushAndGetEntries() + }) + expect(merged.users).toEqual(['bob']) + expect(onUsersChange).not.toHaveBeenCalled() + // Input still clears even on dedup so the next submit isn't sticky. + expect(screen.getByLabelText(/Users/i).value).toBe('') + }) + + it('disables all inputs when disabled prop is set', () => { + useNats.mockReturnValue({ user: { account: 'alice', siteId: 'site-A' }, request: vi.fn() }) + render( + + ) + expect(screen.getByLabelText(/Users/i)).toBeDisabled() + expect(screen.getByLabelText(/Orgs/i)).toBeDisabled() + expect(screen.getByLabelText(/Channels/i)).toBeDisabled() + }) +}) diff --git a/chat-frontend/src/components/MessageArea.jsx b/chat-frontend/src/components/MessageArea.jsx index ea09fb6f5..c7b54b409 100644 --- a/chat-frontend/src/components/MessageArea.jsx +++ b/chat-frontend/src/components/MessageArea.jsx @@ -1,8 +1,9 @@ import { useEffect, useRef } from 'react' import { useRoomEvents } from '../context/RoomEventsContext' import { BUFFER_MODE } from '../lib/roomEventsReducer' -import { roomPrefix } from '../lib/roomFormat' +import { roomPrefix, roomDisplayName } from '../lib/roomFormat' import MessageActionMenu from './MessageActionMenu' +import RoomMembersBadge from './RoomMembersBadge' function formatTime(dateStr) { const d = new Date(dateStr) @@ -20,7 +21,7 @@ function messageContent(msg) { return msg.content || msg.msg || '' } -export default function MessageArea({ room }) { +export default function MessageArea({ room, onOpenMembers, membersRefreshKey }) { const { messages, hasLoadedHistory, @@ -80,9 +81,13 @@ export default function MessageArea({ room }) {
- {roomPrefix(room.type)}{room.name} + {roomPrefix(room.type)}{roomDisplayName(room)} - {room.userCount} members +
{!hasLoadedHistory && !historyError &&
Loading messages...
} diff --git a/chat-frontend/src/components/MessageArea.test.jsx b/chat-frontend/src/components/MessageArea.test.jsx index 8948fffac..ccd31a7a2 100644 --- a/chat-frontend/src/components/MessageArea.test.jsx +++ b/chat-frontend/src/components/MessageArea.test.jsx @@ -14,6 +14,16 @@ beforeEach(() => { vi.mock('../context/RoomEventsContext', () => ({ useRoomEvents: vi.fn(), })) +// Badge lives inside MessageArea's header now. Stub it so MessageArea +// tests don't have to set up NatsContext + a member.list responder; the +// badge's own test file (RoomMembersBadge.test.jsx) covers the fetch + +// click in isolation. +vi.mock('./RoomMembersBadge', () => ({ + default: ({ room, onOpen }) => + room?.type === 'channel' ? ( + + ) : null, +})) vi.mock('../context/NatsContext', () => ({ useNats: vi.fn(), @@ -65,6 +75,30 @@ describe('MessageArea', () => { expect(loadHistory).toHaveBeenCalled() }) + it('renders the members badge in the header and forwards clicks to onOpenMembers', () => { + useRoomEvents.mockReturnValue({ + messages: [], hasLoadedHistory: true, historyError: null, loadHistory: vi.fn().mockResolvedValue(), + }) + const onOpenMembers = vi.fn() + render( + + ) + const btn = screen.getByRole('button', { name: /3 members/ }) + fireEvent.click(btn) + expect(onOpenMembers).toHaveBeenCalledTimes(1) + }) + + it('does not render the members badge for DM rooms (membership is fixed)', () => { + useRoomEvents.mockReturnValue({ + messages: [], hasLoadedHistory: true, historyError: null, loadHistory: vi.fn().mockResolvedValue(), + }) + render() + expect(screen.queryByRole('button', { name: /members$/ })).not.toBeInTheDocument() + }) + it('shows the jump-to-latest pill in historical mode and resets to live on click', () => { const resetToLiveTail = vi.fn() useRoomEvents.mockReturnValue({ diff --git a/chat-frontend/src/components/MessageInput.jsx b/chat-frontend/src/components/MessageInput.jsx index b976515ab..3b5415541 100644 --- a/chat-frontend/src/components/MessageInput.jsx +++ b/chat-frontend/src/components/MessageInput.jsx @@ -3,6 +3,7 @@ import { v4 as uuidv4 } from 'uuid' import { useNats } from '../context/NatsContext' import { msgSend } from '../lib/subjects' import { generateMessageID } from '../lib/idgen' +import { roomPrefix, roomDisplayName } from '../lib/roomFormat' export default function MessageInput({ room }) { const { user, publish } = useNats() @@ -41,7 +42,7 @@ export default function MessageInput({ room }) { value={text} onChange={(e) => setText(e.target.value)} onKeyDown={handleKeyDown} - placeholder={room ? `Message #${room.name}` : 'Select a room...'} + placeholder={room ? `Message ${roomPrefix(room.type)}${roomDisplayName(room)}` : 'Select a room...'} disabled={!room} /> + ) +} diff --git a/chat-frontend/src/components/RoomMembersBadge.test.jsx b/chat-frontend/src/components/RoomMembersBadge.test.jsx new file mode 100644 index 000000000..1bdc6128c --- /dev/null +++ b/chat-frontend/src/components/RoomMembersBadge.test.jsx @@ -0,0 +1,94 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest' +import { render, screen, fireEvent, waitFor } from '@testing-library/react' +import RoomMembersBadge from './RoomMembersBadge' + +vi.mock('../context/NatsContext', () => ({ + useNats: vi.fn(), +})) + +import { useNats } from '../context/NatsContext' + +function setupNats(overrides = {}) { + const request = vi.fn().mockResolvedValue({ members: [{}, {}, {}] }) + useNats.mockReturnValue({ + user: { account: 'alice', siteId: 'site-A' }, + request, + ...overrides, + }) + return { request } +} + +const channel = { id: 'r1', siteId: 'site-A', name: 'general', type: 'channel' } +const dm = { id: 'r-dm', siteId: 'site-A', name: '', type: 'dm' } + +describe('RoomMembersBadge', () => { + beforeEach(() => useNats.mockReset()) + + it('renders nothing when room is null', () => { + setupNats() + const { container } = render() + expect(container.firstChild).toBeNull() + }) + + it('renders nothing for DM-type rooms (membership is fixed at create)', () => { + setupNats() + const { container } = render() + expect(container.firstChild).toBeNull() + }) + + it('fetches member.list on mount and shows "N members"', async () => { + const { request } = setupNats() + render() + await waitFor(() => + expect(request).toHaveBeenCalledWith( + 'chat.user.alice.request.room.r1.site-A.member.list', + expect.anything() + ) + ) + expect(await screen.findByRole('button', { name: /3 members/i })).toBeInTheDocument() + }) + + it('renders singular "1 member" when the room has exactly one member', async () => { + setupNats({ request: vi.fn().mockResolvedValue({ members: [{}] }) }) + render() + expect(await screen.findByRole('button', { name: /^1 member$/i })).toBeInTheDocument() + }) + + it('clicking the badge invokes onOpen so the parent can open ManageMembersDialog', async () => { + setupNats() + const onOpen = vi.fn() + render() + const btn = await screen.findByRole('button', { name: /3 members/i }) + fireEvent.click(btn) + expect(onOpen).toHaveBeenCalledTimes(1) + }) + + it('refetches when the room changes', async () => { + const request = vi.fn() + .mockResolvedValueOnce({ members: [{}, {}] }) + .mockResolvedValueOnce({ members: [{}, {}, {}, {}] }) + setupNats({ request }) + const { rerender } = render() + await screen.findByRole('button', { name: /2 members/i }) + rerender() + await screen.findByRole('button', { name: /4 members/i }) + expect(request).toHaveBeenCalledTimes(2) + }) + + it('refetches when refreshKey prop bumps (e.g. after the dialog closes)', async () => { + const request = vi.fn() + .mockResolvedValueOnce({ members: [{}, {}] }) + .mockResolvedValueOnce({ members: [{}, {}, {}] }) + setupNats({ request }) + const { rerender } = render() + await screen.findByRole('button', { name: /2 members/i }) + rerender() + await screen.findByRole('button', { name: /3 members/i }) + }) + + it('falls back to a generic label on fetch failure rather than disappearing', async () => { + setupNats({ request: vi.fn().mockRejectedValue(new Error('not subscribed')) }) + render() + expect(await screen.findByRole('button', { name: /members/i })).toBeInTheDocument() + }) +}) diff --git a/chat-frontend/src/components/manageMembers/AddMembersForm.jsx b/chat-frontend/src/components/manageMembers/AddMembersForm.jsx index 930701014..5214a5064 100644 --- a/chat-frontend/src/components/manageMembers/AddMembersForm.jsx +++ b/chat-frontend/src/components/manageMembers/AddMembersForm.jsx @@ -1,19 +1,23 @@ import { useEffect, useRef, useState } from 'react' import { useNats } from '../../context/NatsContext' import { memberAdd } from '../../lib/subjects' -import { parseList } from '../../lib/parseList' import { HISTORY_MODE_ALL, HISTORY_MODE_NONE } from '../../lib/constants' +import { formatAsyncJobError } from '../../lib/asyncJob' +import MemberPicker from '../MemberPicker' + +const SUCCESS_BANNER_MS = 3000 export default function AddMembersForm({ room }) { - const { user, request } = useNats() - const [accounts, setAccounts] = useState('') - const [orgs, setOrgs] = useState('') - const [channels, setChannels] = useState('') + const { user, requestWithAsyncResult } = useNats() + const [users, setUsers] = useState([]) + const [orgs, setOrgs] = useState([]) + const [channels, setChannels] = useState([]) const [shareHistory, setShareHistory] = useState(true) const [loading, setLoading] = useState(false) const [error, setError] = useState(null) const [success, setSuccess] = useState(false) const successTimer = useRef(null) + const pickerRef = useRef(null) useEffect(() => { return () => { @@ -21,67 +25,50 @@ export default function AddMembersForm({ room }) { } }, []) - const users = parseList(accounts) - const orgList = parseList(orgs) - const channelList = parseList(channels) - const canSubmit = users.length + orgList.length + channelList.length > 0 - const handleSubmit = async (e) => { e.preventDefault() - if (!canSubmit || !user) return + if (!user) return + // Flush any typed-but-uncommitted text in the picker (comma-split into + // entries) so users can paste "alice, bob" and click Add without first + // pressing Enter. + const { users: finalUsers, orgs: finalOrgs, channels: finalChannels } = + pickerRef.current?.flushAndGetEntries() ?? { users, orgs, channels } + if (finalUsers.length + finalOrgs.length + finalChannels.length === 0) return setLoading(true) setError(null) setSuccess(false) try { - await request(memberAdd(user.account, room.id, room.siteId), { + await requestWithAsyncResult(memberAdd(user.account, room.id, room.siteId), { roomId: room.id, - users, - orgs: orgList, - channels: channelList, + users: finalUsers, + orgs: finalOrgs, + channels: finalChannels, history: { mode: shareHistory ? HISTORY_MODE_ALL : HISTORY_MODE_NONE }, }) - setAccounts('') - setOrgs('') - setChannels('') + setUsers([]) + setOrgs([]) + setChannels([]) setSuccess(true) if (successTimer.current) clearTimeout(successTimer.current) - successTimer.current = setTimeout(() => setSuccess(false), 3000) + successTimer.current = setTimeout(() => setSuccess(false), SUCCESS_BANNER_MS) } catch (err) { - setError(err.message) + setError(formatAsyncJobError(err)) } finally { setLoading(false) } } + return (
- - setAccounts(e.target.value)} - placeholder="e.g. bob, charlie" - disabled={loading} - /> - - - setOrgs(e.target.value)} - placeholder="e.g. eng-frontend" - disabled={loading} - /> - - - setChannels(e.target.value)} - placeholder="e.g. r-existing" + @@ -96,11 +83,11 @@ export default function AddMembersForm({ room }) { {error &&
{error}
} - {success &&
Accepted
} + {success &&
Added
}
-
diff --git a/chat-frontend/src/components/manageMembers/AddMembersForm.test.jsx b/chat-frontend/src/components/manageMembers/AddMembersForm.test.jsx index 2383e00e6..1daa38a21 100644 --- a/chat-frontend/src/components/manageMembers/AddMembersForm.test.jsx +++ b/chat-frontend/src/components/manageMembers/AddMembersForm.test.jsx @@ -11,83 +11,171 @@ import { useNats } from '../../context/NatsContext' const room = { id: 'r1', siteId: 'site-A', name: 'general' } function setup(overrides = {}) { - const request = vi.fn().mockResolvedValue({ status: 'accepted' }) + const requestWithAsyncResult = vi.fn().mockResolvedValue({ + sync: { status: 'accepted' }, + async: { status: 'ok', operation: 'room.member.add' }, + }) useNats.mockReturnValue({ - user: { account: 'alice' }, - request, + user: { account: 'alice', siteId: 'site-A' }, + request: vi.fn(), + requestWithAsyncResult, ...overrides, }) render() - return { request } + return { requestWithAsyncResult } } describe('AddMembersForm', () => { - beforeEach(() => { - useNats.mockReset() - }) + beforeEach(() => useNats.mockReset()) - it('disables submit when all inputs are empty', () => { - setup() - expect(screen.getByRole('button', { name: /^Add$/ })).toBeDisabled() + it('submit is always clickable; clicking on an empty form is a no-op', () => { + // Previously the button was disabled until a chip existed, which meant + // typing "alice" in Users without pressing Enter left submit greyed out. + // The handler now flushes pending text first and early-returns on empty. + const { requestWithAsyncResult } = setup() + const submit = screen.getByRole('button', { name: /^Add$/ }) + expect(submit).not.toBeDisabled() + fireEvent.click(submit) + expect(requestWithAsyncResult).not.toHaveBeenCalled() }) - it('submits parsed lists with mode=all by default', async () => { - const { request } = setup() - fireEvent.change(screen.getByLabelText(/Accounts/i), { target: { value: 'bob, charlie' } }) + it('sends ChannelRef-shaped channels (not strings) with mode=all by default', async () => { + const { requestWithAsyncResult } = setup() + fireEvent.change(screen.getByLabelText(/Users/i), { target: { value: 'bob' } }) + fireEvent.keyDown(screen.getByLabelText(/Users/i), { key: 'Enter' }) fireEvent.change(screen.getByLabelText(/Orgs/i), { target: { value: 'eng' } }) + fireEvent.keyDown(screen.getByLabelText(/Orgs/i), { key: 'Enter' }) fireEvent.change(screen.getByLabelText(/Channels/i), { target: { value: 'r-x' } }) + fireEvent.keyDown(screen.getByLabelText(/Channels/i), { key: 'Enter' }) fireEvent.click(screen.getByRole('button', { name: /^Add$/ })) - await waitFor(() => expect(request).toHaveBeenCalledTimes(1)) - expect(request).toHaveBeenCalledWith( + await waitFor(() => expect(requestWithAsyncResult).toHaveBeenCalledTimes(1)) + expect(requestWithAsyncResult).toHaveBeenCalledWith( 'chat.user.alice.request.room.r1.site-A.member.add', { roomId: 'r1', - users: ['bob', 'charlie'], + users: ['bob'], orgs: ['eng'], - channels: ['r-x'], + channels: [{ roomId: 'r-x', siteId: 'site-A' }], history: { mode: 'all' }, } ) }) it('sends mode=none when share-history is unchecked', async () => { - const { request } = setup() - fireEvent.change(screen.getByLabelText(/Accounts/i), { target: { value: 'bob' } }) + const { requestWithAsyncResult } = setup() + fireEvent.change(screen.getByLabelText(/Users/i), { target: { value: 'bob' } }) + fireEvent.keyDown(screen.getByLabelText(/Users/i), { key: 'Enter' }) fireEvent.click(screen.getByLabelText(/Share room history/i)) fireEvent.click(screen.getByRole('button', { name: /^Add$/ })) - await waitFor(() => expect(request).toHaveBeenCalledTimes(1)) - expect(request.mock.calls[0][1].history).toEqual({ mode: 'none' }) + await waitFor(() => expect(requestWithAsyncResult).toHaveBeenCalledTimes(1)) + expect(requestWithAsyncResult.mock.calls[0][1].history).toEqual({ mode: 'none' }) + }) + + it('auto-flushes typed-but-not-Entered text in all three fields on submit', async () => { + const { requestWithAsyncResult } = setup() + // User types comma-separated values in each field but does NOT press Enter. + fireEvent.change(screen.getByLabelText(/Users/i), { target: { value: 'alice, bob' } }) + fireEvent.change(screen.getByLabelText(/Orgs/i), { target: { value: 'eng-org, ops-org' } }) + fireEvent.change(screen.getByLabelText(/Channels/i), { target: { value: 'r-x, r-y' } }) + fireEvent.click(screen.getByRole('button', { name: /^Add$/ })) + await waitFor(() => expect(requestWithAsyncResult).toHaveBeenCalledTimes(1)) + expect(requestWithAsyncResult.mock.calls[0][1]).toMatchObject({ + users: ['alice', 'bob'], + orgs: ['eng-org', 'ops-org'], + channels: [ + { roomId: 'r-x', siteId: 'site-A' }, + { roomId: 'r-y', siteId: 'site-A' }, + ], + }) }) - it('shows error banner on request failure', async () => { - const request = vi.fn().mockRejectedValue(new Error('only owners can add members')) - setup({ request }) - fireEvent.change(screen.getByLabelText(/Accounts/i), { target: { value: 'bob' } }) + it('shows error banner when the async result is an error', async () => { + const requestWithAsyncResult = vi.fn().mockRejectedValue(new Error('only owners can add members')) + setup({ requestWithAsyncResult }) + fireEvent.change(screen.getByLabelText(/Users/i), { target: { value: 'bob' } }) + fireEvent.keyDown(screen.getByLabelText(/Users/i), { key: 'Enter' }) fireEvent.click(screen.getByRole('button', { name: /^Add$/ })) expect(await screen.findByText(/only owners/)).toBeInTheDocument() }) - it('clears inputs and shows Accepted on success', async () => { + it('clears chips and shows Added on async success', async () => { setup() - const accounts = screen.getByLabelText(/Accounts/i) - fireEvent.change(accounts, { target: { value: 'bob' } }) + fireEvent.change(screen.getByLabelText(/Users/i), { target: { value: 'bob' } }) + fireEvent.keyDown(screen.getByLabelText(/Users/i), { key: 'Enter' }) + expect(screen.getByText('bob')).toBeInTheDocument() fireEvent.click(screen.getByRole('button', { name: /^Add$/ })) - expect(await screen.findByText('Accepted')).toBeInTheDocument() - expect(accounts.value).toBe('') + expect(await screen.findByText('Added')).toBeInTheDocument() + expect(screen.queryByText('bob')).not.toBeInTheDocument() + }) + + it('shows "Adding..." on the submit button while waiting for the async result', async () => { + let resolveIt + const requestWithAsyncResult = vi.fn( + () => new Promise((r) => { resolveIt = () => r({ sync: { status: 'accepted' }, async: { status: 'ok' } }) }) + ) + setup({ requestWithAsyncResult }) + fireEvent.change(screen.getByLabelText(/Users/i), { target: { value: 'bob' } }) + fireEvent.keyDown(screen.getByLabelText(/Users/i), { key: 'Enter' }) + fireEvent.click(screen.getByRole('button', { name: /^Add$/ })) + expect(await screen.findByRole('button', { name: /Adding/i })).toBeInTheDocument() + await act(async () => { resolveIt() }) + await waitFor(() => expect(screen.getByRole('button', { name: /^Add$/ })).toBeInTheDocument()) }) it('auto-dismisses the success banner after 3 seconds', async () => { vi.useFakeTimers({ shouldAdvanceTime: true }) try { setup() - fireEvent.change(screen.getByLabelText(/Accounts/i), { target: { value: 'bob' } }) + fireEvent.change(screen.getByLabelText(/Users/i), { target: { value: 'bob' } }) + fireEvent.keyDown(screen.getByLabelText(/Users/i), { key: 'Enter' }) fireEvent.click(screen.getByRole('button', { name: /^Add$/ })) - expect(await screen.findByText('Accepted')).toBeInTheDocument() - await act(async () => { - vi.advanceTimersByTime(3000) + expect(await screen.findByText('Added')).toBeInTheDocument() + await act(async () => { vi.advanceTimersByTime(3000) }) + await waitFor(() => expect(screen.queryByText('Added')).not.toBeInTheDocument()) + } finally { + vi.useRealTimers() + } + }) + + it('cleanup effect calls clearTimeout with the pending timer id on unmount', async () => { + // The React 18+ "setState after unmount" warning was removed in React 19, + // so checking for that warning would be vacuously true. Spy on + // clearTimeout directly to verify the cleanup effect actually disarms + // the pending success-banner timer. + vi.useFakeTimers({ shouldAdvanceTime: true }) + const setTimeoutSpy = vi.spyOn(globalThis, 'setTimeout') + const clearSpy = vi.spyOn(globalThis, 'clearTimeout') + try { + const requestWithAsyncResult = vi.fn().mockResolvedValue({ + sync: { status: 'accepted' }, + async: { status: 'ok', operation: 'room.member.add' }, + }) + useNats.mockReturnValue({ + user: { account: 'alice', siteId: 'site-A' }, + request: vi.fn(), + requestWithAsyncResult, }) - await waitFor(() => expect(screen.queryByText('Accepted')).not.toBeInTheDocument()) + const { unmount } = render() + fireEvent.change(screen.getByLabelText(/Users/i), { target: { value: 'bob' } }) + fireEvent.keyDown(screen.getByLabelText(/Users/i), { key: 'Enter' }) + fireEvent.click(screen.getByRole('button', { name: /^Add$/ })) + await screen.findByText('Added') + + // The 3000ms success-banner timer should be the most recent + // setTimeout call with delay=3000. Capture its returned id. + const successTimerCall = setTimeoutSpy.mock.results.find( + (r, i) => setTimeoutSpy.mock.calls[i][1] === 3000 + ) + expect(successTimerCall).toBeDefined() + const successTimerId = successTimerCall.value + + clearSpy.mockClear() + unmount() + // Cleanup effect ran; clearTimeout must have been called with that id. + expect(clearSpy).toHaveBeenCalledWith(successTimerId) } finally { + setTimeoutSpy.mockRestore() + clearSpy.mockRestore() vi.useRealTimers() } }) diff --git a/chat-frontend/src/components/manageMembers/MemberRoster.jsx b/chat-frontend/src/components/manageMembers/MemberRoster.jsx new file mode 100644 index 000000000..fa6c7c4cd --- /dev/null +++ b/chat-frontend/src/components/manageMembers/MemberRoster.jsx @@ -0,0 +1,358 @@ +import { useCallback, useEffect, useMemo, useRef, useState } from 'react' +import { useNats } from '../../context/NatsContext' +import { memberList, memberRemove, memberRoleUpdate, orgMembers } from '../../lib/subjects' +import { ROLE_OWNER, ROLE_MEMBER } from '../../lib/constants' +import { formatAsyncJobError } from '../../lib/asyncJob' + +export default function MemberRoster({ room }) { + const { user, request, requestWithAsyncResult } = useNats() + const account = user?.account + const [members, setMembers] = useState([]) + const [loading, setLoading] = useState(true) + const [error, setError] = useState(null) + const [busyKey, setBusyKey] = useState(null) + const [actionError, setActionError] = useState(null) + // Org-expansion state. Each map is keyed by orgId. + // expandedOrgs — UI state: which orgs are currently open + // orgChildren — cached responses from orgs.{id}.members; survives a + // collapse/re-expand cycle so we don't refetch. + // orgFetchState — 'loading' | 'error' while a fetch is in flight or + // after it failed (so the user can see a banner). On + // success the key is removed. + const [expandedOrgs, setExpandedOrgs] = useState({}) + const [orgChildren, setOrgChildren] = useState({}) + const [orgFetchState, setOrgFetchState] = useState({}) + + // Generation refs guard against stale async writes when the request + // identity changes mid-fetch. memberListGenRef bumps every time + // fetchMembers is invoked (room change or post-action refetch). Each + // org has its own generation key in orgFetchGenRef.current[orgId] so a + // user can rapidly expand/collapse the same org without the previous + // fetch's resolution stomping on the current state. + const memberListGenRef = useRef(0) + const orgFetchGenRef = useRef({}) + + // Depend on the primitive identity fields, not the room object reference — + // a parent re-render that hands us an equivalent-but-new room object + // shouldn't fire a redundant member.list. + const fetchMembers = useCallback(async () => { + if (!account) return + // Bump the generation before we await so a later invocation (room + // switch, post-action refetch) marks this one stale by the time its + // promise resolves. Without this guard, a slow first request that + // resolves after a second request finishes could overwrite the + // newer state. + const gen = ++memberListGenRef.current + setError(null) + setLoading(true) + try { + const resp = await request(memberList(account, room.id, room.siteId), { enrich: true }) + if (gen !== memberListGenRef.current) return + setMembers(resp.members ?? []) + } catch (err) { + if (gen !== memberListGenRef.current) return + setError(err.message) + } finally { + if (gen === memberListGenRef.current) setLoading(false) + } + }, [request, account, room.id, room.siteId]) + + useEffect(() => { + fetchMembers() + }, [fetchMembers]) + + // Owner-status is derived from the just-fetched roster: find the row that + // matches the current user's account and read isOwner. Memoised so the + // per-row gating doesn't re-derive on every render. + const isCurrentUserOwner = useMemo(() => { + const self = members.find((m) => m.member?.type === 'individual' && m.member?.account === account) + return !!self?.member?.isOwner + }, [members, account]) + + // Single ordered list: orgs first, individuals second. Within each group + // the server's enriched-list order is preserved (today: arbitrary; if the + // server later sorts by sectName / engName the UI inherits that for free). + const ordered = useMemo(() => { + const orgs = members.filter((m) => m.member?.type === 'org') + const individuals = members.filter((m) => m.member?.type === 'individual') + return [...orgs, ...individuals] + }, [members]) + + /** + * Run a member.{remove|role-update} action through requestWithAsyncResult, + * surface failures as a banner, and refetch the roster on success. + */ + const runAction = useCallback( + async (key, subject, payload) => { + setBusyKey(key) + setActionError(null) + try { + await requestWithAsyncResult(subject, payload) + await fetchMembers() + return true + } catch (err) { + setActionError(formatAsyncJobError(err)) + return false + } finally { + setBusyKey(null) + } + }, + [requestWithAsyncResult, fetchMembers] + ) + + /** + * Leave the room (member.remove on self). Distinct from the generic + * runAction path because (a) we don't refetch — we're no longer a member + * and the call would fail or return an empty roster, and (b) the dialog + * will dismiss on its own via ChatPage's selectedRoom/summaries effect + * once subscription.update lands. + */ + const handleLeave = useCallback(async () => { + if (!account) return + if (!window.confirm(`Leave "${room.name}"?`)) return + setBusyKey(`leave:${account}`) + setActionError(null) + try { + await requestWithAsyncResult(memberRemove(account, room.id, room.siteId), { + roomId: room.id, + account, + }) + } catch (err) { + setActionError(formatAsyncJobError(err)) + } finally { + setBusyKey(null) + } + }, [room.id, room.siteId, room.name, account, requestWithAsyncResult]) + + /** + * Toggle an org row open/closed. On the first open we fetch the org's + * members via the chat.user.{a}.request.orgs.{orgId}.members RPC and + * cache the result in orgChildren — collapsing then re-expanding doesn't + * refetch. Loading + error states live in orgFetchState so the row can + * render a "Loading…" / "Failed to load members" hint without conflating + * with the top-level dialog error. + */ + const toggleOrg = useCallback( + async (orgId) => { + const wasOpen = !!expandedOrgs[orgId] + setExpandedOrgs((s) => ({ ...s, [orgId]: !wasOpen })) + // Collapsing or already cached — nothing else to do. + if (wasOpen) return + if (orgChildren[orgId]) return + if (!account) return + // Per-org generation guard. If the user expands → collapses → + // re-expands rapidly, only the latest fetch's resolution writes to + // state; earlier in-flight responses are dropped on the floor. + const gen = (orgFetchGenRef.current[orgId] ?? 0) + 1 + orgFetchGenRef.current[orgId] = gen + setOrgFetchState((s) => ({ ...s, [orgId]: 'loading' })) + try { + const resp = await request(orgMembers(account, orgId), {}) + if (orgFetchGenRef.current[orgId] !== gen) return + setOrgChildren((s) => ({ ...s, [orgId]: resp?.members ?? [] })) + setOrgFetchState((s) => { + const { [orgId]: _drop, ...rest } = s + return rest + }) + } catch { + if (orgFetchGenRef.current[orgId] !== gen) return + setOrgFetchState((s) => ({ ...s, [orgId]: 'error' })) + } + }, + [expandedOrgs, orgChildren, request, account] + ) + + if (loading) return
Loading members…
+ if (error) return
{error}
+ + return ( +
+ {actionError &&
{actionError}
} + + {ordered.length === 0 ? ( +
No members.
+ ) : ( +
    + {ordered.map((m) => + m.member?.type === 'org' + ? renderOrgRow(m, { + busyKey, + isCurrentUserOwner, + room, + user, + runAction, + expanded: !!expandedOrgs[m.member.id], + children: orgChildren[m.member.id], + fetchState: orgFetchState[m.member.id], + toggleOrg, + }) + : renderIndividualRow(m, { + busyKey, + isCurrentUserOwner, + room, + user, + runAction, + handleLeave, + }) + )} +
+ )} +
+ ) +} + +function renderOrgRow(m, { busyKey, isCurrentUserOwner, room, user, runAction, expanded, children, fetchState, toggleOrg }) { + const entry = m.member + const orgId = entry.id + const display = entry.sectName || orgId + const removeKey = `removeOrg:${orgId}` + return ( +
  • +
    + +
    + {isCurrentUserOwner && ( + + )} +
    +
    + {expanded && ( +
      + {fetchState === 'loading' && ( +
    • Loading members…
    • + )} + {fetchState === 'error' && ( +
    • Failed to load members.
    • + )} + {!fetchState && (children?.length ?? 0) === 0 && ( +
    • No members.
    • + )} + {children?.map((c) => { + // Org-children rows show engName + chineseName only — the parent + // org row already owns the Remove control (server treats the org + // as a single membership unit; removing it removes all members + // server-side). + const primary = c.engName || c.account + const secondary = c.chineseName || '' + return ( +
    • + {primary} + {secondary && {secondary}} +
    • + ) + })} +
    + )} +
  • + ) +} + +function renderIndividualRow(m, { busyKey, isCurrentUserOwner, room, user, runAction, handleLeave }) { + const entry = m.member + const isOwner = !!entry.isOwner + const isSelf = entry.account === user.account + const primary = entry.engName || entry.account + const secondary = entry.chineseName || '' + const promoteKey = `promote:${entry.account}` + const demoteKey = `demote:${entry.account}` + const removeKey = `remove:${entry.account}` + const leaveKey = `leave:${entry.account}` + + return ( +
  • +
    + {primary} + {secondary && {secondary}} + {isOwner && owner} +
    +
    + {isSelf ? ( + // Self row: only Leave. Remove/Promote/Demote on self would let + // the user kick themselves or lock themselves out of admin + // mid-flow — Leave is the one self-applicable action and it + // confirms first. + + ) : isCurrentUserOwner ? ( + <> + {isOwner ? ( + + ) : ( + + )} + + + ) : null /* non-owner viewing someone else's row: no controls */} +
    +
  • + ) +} diff --git a/chat-frontend/src/components/manageMembers/MemberRoster.test.jsx b/chat-frontend/src/components/manageMembers/MemberRoster.test.jsx new file mode 100644 index 000000000..825541101 --- /dev/null +++ b/chat-frontend/src/components/manageMembers/MemberRoster.test.jsx @@ -0,0 +1,517 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest' +import { render, screen, fireEvent, waitFor, act } from '@testing-library/react' +import MemberRoster from './MemberRoster' + +vi.mock('../../context/NatsContext', () => ({ + useNats: vi.fn(), +})) + +import { useNats } from '../../context/NatsContext' + +const room = { id: 'r1', siteId: 'site-A', name: 'general' } + +const baseMembers = [ + { id: 'rm1', rid: 'r1', member: { id: 'u-alice', type: 'individual', account: 'alice', engName: 'Alice A', chineseName: '王明', isOwner: true } }, + { id: 'rm2', rid: 'r1', member: { id: 'u-bob', type: 'individual', account: 'bob', engName: 'Bob B', chineseName: '陳大文', isOwner: false } }, + { id: 'rm3', rid: 'r1', member: { id: 'u-carol', type: 'individual', account: 'carol', engName: 'Carol C', chineseName: '李美', isOwner: true } }, + { id: 'rm4', rid: 'r1', member: { id: 'org-eng', type: 'org', sectName: 'Engineering', memberCount: 42 } }, +] + +function setupContext(overrides = {}) { + const request = vi.fn().mockResolvedValue({ members: baseMembers }) + const requestWithAsyncResult = vi.fn().mockResolvedValue({ sync: { status: 'accepted' }, async: { status: 'ok' } }) + useNats.mockReturnValue({ + user: { account: 'alice', siteId: 'site-A' }, + request, + requestWithAsyncResult, + ...overrides, + }) + return { request, requestWithAsyncResult } +} + +describe('MemberRoster', () => { + beforeEach(() => useNats.mockReset()) + + it('does not refetch when the room prop is a new object reference but same id+siteId', async () => { + const { request } = setupContext() + const { rerender } = render() + await waitFor(() => expect(request).toHaveBeenCalledTimes(1)) + rerender() + // Empty act flushes any pending effects + microtasks deterministically. + // If the rerender mistakenly re-fires fetchMembers, it would queue a new + // call inside this flush; since deps are now `room.id` + `room.siteId` + // (not the room object), the count must stay at 1. + await act(async () => {}) + expect(request).toHaveBeenCalledTimes(1) + }) + + it('drops a stale member.list response if the room changed mid-fetch', async () => { + // Slow network: switching from room A to room B before A's + // member.list resolves used to let A's members overwrite B's. The + // generation guard now drops the stale write. + let resolveA + const aResp = new Promise((r) => { resolveA = r }) + const request = vi.fn().mockImplementation((subject) => { + if (subject.startsWith('chat.user.alice.request.room.r1.')) return aResp + return Promise.resolve({ + members: [ + { id: 'rm-b', rid: 'r2', member: { id: 'u-newroom', type: 'individual', account: 'newroom', engName: 'New Room User', chineseName: '', isOwner: false } }, + ], + }) + }) + useNats.mockReturnValue({ + user: { account: 'alice', siteId: 'site-A' }, + request, + requestWithAsyncResult: vi.fn(), + }) + const { rerender } = render() + // Switch rooms while r1's fetch is still in flight. + rerender() + await screen.findByText('New Room User') + // Now the stale r1 response lands with what would have overwritten + // the r2 list. The generation guard drops it. + await act(async () => { + resolveA({ + members: [ + { id: 'rm-a', rid: 'r1', member: { id: 'u-oldroom', type: 'individual', account: 'oldroom', engName: 'Old Room User', chineseName: '', isOwner: false } }, + ], + }) + await Promise.resolve() + }) + expect(screen.queryByText('Old Room User')).not.toBeInTheDocument() + expect(screen.getByText('New Room User')).toBeInTheDocument() + }) + + it('calls member.list with enrich on mount', async () => { + const { request } = setupContext() + render() + await waitFor(() => + expect(request).toHaveBeenCalledWith( + 'chat.user.alice.request.room.r1.site-A.member.list', + { enrich: true } + ) + ) + }) + + it('renders orgs first then individuals in a single list, with EngName + ChineseName for individuals', async () => { + setupContext() + render() + await screen.findByText('Alice A') + expect(screen.getByText('Engineering')).toBeInTheDocument() + expect(screen.getByText('Bob B')).toBeInTheDocument() + // Individuals show chineseName as the secondary text (replacing the + // old engName + accountName combo). + expect(screen.getByText('王明')).toBeInTheDocument() + expect(screen.getByText('陳大文')).toBeInTheDocument() + // Single roster-list, orgs ordered before individuals. + const listItems = screen.getAllByRole('listitem') + expect(listItems[0]).toHaveTextContent('Engineering') + expect(listItems[1]).toHaveTextContent('Alice A') + expect(listItems[2]).toHaveTextContent('Bob B') + expect(listItems[3]).toHaveTextContent('Carol C') + // Owner badge still appears on owners; not on regular members. + expect(listItems[1]).toHaveTextContent(/owner/i) + expect(listItems[2]).not.toHaveTextContent(/owner/i) + expect(listItems[3]).toHaveTextContent(/owner/i) + }) + + it('shows a loading indicator before the list resolves', async () => { + let resolveIt + const request = vi.fn().mockImplementation(() => new Promise((r) => { resolveIt = r })) + useNats.mockReturnValue({ + user: { account: 'alice', siteId: 'site-A' }, + request, + requestWithAsyncResult: vi.fn(), + }) + render() + expect(screen.getByText(/loading members/i)).toBeInTheDocument() + resolveIt({ members: [] }) + await waitFor(() => expect(screen.queryByText(/loading members/i)).not.toBeInTheDocument()) + }) + + it('renders an error banner when member.list fails', async () => { + const request = vi.fn().mockRejectedValue(new Error('not a room member')) + useNats.mockReturnValue({ + user: { account: 'alice', siteId: 'site-A' }, + request, + requestWithAsyncResult: vi.fn(), + }) + render() + expect(await screen.findByText(/not a room member/i)).toBeInTheDocument() + }) + + it('Promote on a non-owner member sends member.role-update newRole=owner', async () => { + const { requestWithAsyncResult } = setupContext() + render() + await screen.findByText('Bob B') + fireEvent.click(screen.getByRole('button', { name: /Promote bob/i })) + await waitFor(() => + expect(requestWithAsyncResult).toHaveBeenCalledWith( + 'chat.user.alice.request.room.r1.site-A.member.role-update', + { roomId: 'r1', account: 'bob', newRole: 'owner' } + ) + ) + }) + + it('Demote on another owner sends member.role-update newRole=member', async () => { + // Run as alice (owner); demote carol (also owner). Owner-on-self is a + // Leave button in the new design, not a Demote, so the Demote-action + // test must target a peer owner. + const { requestWithAsyncResult } = setupContext() + render() + await screen.findByText('Carol C') + fireEvent.click(screen.getByRole('button', { name: /Demote carol/i })) + await waitFor(() => + expect(requestWithAsyncResult).toHaveBeenCalledWith( + 'chat.user.alice.request.room.r1.site-A.member.role-update', + { roomId: 'r1', account: 'carol', newRole: 'member' } + ) + ) + }) + + it('Remove on an individual sends member.remove with account', async () => { + const { requestWithAsyncResult } = setupContext() + render() + await screen.findByText('Bob B') + fireEvent.click(screen.getByRole('button', { name: /Remove bob/i })) + await waitFor(() => + expect(requestWithAsyncResult).toHaveBeenCalledWith( + 'chat.user.alice.request.room.r1.site-A.member.remove', + { roomId: 'r1', account: 'bob' } + ) + ) + }) + + it('Remove on an org sends member.remove with orgId', async () => { + const { requestWithAsyncResult } = setupContext() + render() + await screen.findByText('Engineering') + fireEvent.click(screen.getByRole('button', { name: /Remove org-eng/i })) + await waitFor(() => + expect(requestWithAsyncResult).toHaveBeenCalledWith( + 'chat.user.alice.request.room.r1.site-A.member.remove', + { roomId: 'r1', orgId: 'org-eng' } + ) + ) + }) + + it('refetches the roster after a successful action', async () => { + const { request } = setupContext() + render() + await screen.findByText('Bob B') + expect(request).toHaveBeenCalledTimes(1) + fireEvent.click(screen.getByRole('button', { name: /Remove bob/i })) + await waitFor(() => expect(request).toHaveBeenCalledTimes(2)) + }) + + it('isolates the busy state to the row whose action is in flight (no suffix collisions)', async () => { + // Regression: previous predicate was `busyKey?.endsWith(`:${account}`)` + // which (a) disabled all three buttons on the active row at once and + // (b) cross-disabled `bob` and `dynamicbob` because both end in `:bob`. + let resolveAction + const requestWithAsyncResult = vi.fn( + () => new Promise((r) => { resolveAction = () => r({ sync: { status: 'accepted' }, async: { status: 'ok' } }) }) + ) + useNats.mockReturnValue({ + user: { account: 'alice', siteId: 'site-A' }, + request: vi.fn().mockResolvedValue({ + members: [ + // Alice (current user) must be in the roster as owner for the + // owner-gated Promote/Remove buttons to render. Without her, + // isCurrentUserOwner falls through to false and the buttons + // this test is exercising don't exist. + { id: 'rm0', rid: 'r1', member: { id: 'u-alice', type: 'individual', account: 'alice', engName: 'Alice', isOwner: true } }, + { id: 'rm1', rid: 'r1', member: { id: 'u-bob', type: 'individual', account: 'bob', engName: 'Bob', isOwner: false } }, + { id: 'rm2', rid: 'r1', member: { id: 'u-dyn', type: 'individual', account: 'dynamicbob', engName: 'DBob', isOwner: false } }, + ], + }), + requestWithAsyncResult, + }) + render() + await screen.findByText('DBob') + fireEvent.click(screen.getByRole('button', { name: /Promote bob/i })) + // Promote on bob is in flight — bob's Remove and dynamicbob's buttons must remain enabled. + expect(screen.getByRole('button', { name: /Promote bob/i })).toBeDisabled() + expect(screen.getByRole('button', { name: /Remove bob/i })).not.toBeDisabled() + expect(screen.getByRole('button', { name: /Promote dynamicbob/i })).not.toBeDisabled() + expect(screen.getByRole('button', { name: /Remove dynamicbob/i })).not.toBeDisabled() + resolveAction() + }) + + it('does not render Remove-by-ID inputs (every row already has its own Remove button)', async () => { + setupContext() + render() + await screen.findByText('Bob B') + expect(screen.queryByLabelText(/Remove individual by account/i)).toBeNull() + expect(screen.queryByLabelText(/Remove org by id/i)).toBeNull() + }) + + it('inline-button callers safely ignore runAction return value on both success and failure', async () => { + // The Promote/Demote/Remove buttons fire-and-forget; they don't branch on + // runAction's boolean return. Lock in that ignoring the return value is + // safe (no unhandled rejection, no post-action side effects fired wrongly). + const requestWithAsyncResult = vi.fn() + .mockRejectedValueOnce(new Error('only owners can remove members')) + .mockResolvedValueOnce({ sync: { status: 'accepted' }, async: { status: 'ok' } }) + useNats.mockReturnValue({ + user: { account: 'alice', siteId: 'site-A' }, + request: vi.fn().mockResolvedValue({ members: baseMembers }), + requestWithAsyncResult, + }) + render() + await screen.findByText('Bob B') + + // Failure path: error banner appears, button re-enables, no crash. + fireEvent.click(screen.getByRole('button', { name: /Remove bob/i })) + expect(await screen.findByText(/only owners/i)).toBeInTheDocument() + expect(screen.getByRole('button', { name: /Remove bob/i })).not.toBeDisabled() + + // Success path right after: no error banner, action proceeds normally. + fireEvent.click(screen.getByRole('button', { name: /Promote bob/i })) + await waitFor(() => expect(requestWithAsyncResult).toHaveBeenCalledTimes(2)) + }) + + it('shows a Leave button on the current user\'s own row (not Remove / Promote / Demote)', async () => { + setupContext() // current user = alice (owner) + render() + await screen.findByText('Alice A') + const aliceRow = screen.getByText('Alice A').closest('li') + // The self row never offers Remove or role-change controls — leaving is + // the only self-applicable action. Demote/Remove on self would be a UX + // trap: a user could lock themselves out of admin or kick themselves + // and lose the dialog mid-action. + expect(aliceRow).toHaveTextContent(/Leave/i) + expect(aliceRow.querySelector('button[aria-label^="Remove alice"]')).toBeNull() + expect(aliceRow.querySelector('button[aria-label^="Promote alice"]')).toBeNull() + expect(aliceRow.querySelector('button[aria-label^="Demote alice"]')).toBeNull() + }) + + it('clicking Leave on own row sends member.remove with the user\'s own account after confirm', async () => { + const { requestWithAsyncResult } = setupContext() + const confirmSpy = vi.spyOn(window, 'confirm').mockReturnValue(true) + try { + render() + await screen.findByText('Alice A') + fireEvent.click(screen.getByRole('button', { name: /^Leave$/i })) + expect(confirmSpy).toHaveBeenCalledWith(expect.stringMatching(/Leave .*general/i)) + await waitFor(() => + expect(requestWithAsyncResult).toHaveBeenCalledWith( + 'chat.user.alice.request.room.r1.site-A.member.remove', + { roomId: 'r1', account: 'alice' } + ) + ) + } finally { + confirmSpy.mockRestore() + } + }) + + it('cancelling the Leave confirm dialog does not send any request', async () => { + const { requestWithAsyncResult } = setupContext() + const confirmSpy = vi.spyOn(window, 'confirm').mockReturnValue(false) + try { + render() + await screen.findByText('Alice A') + fireEvent.click(screen.getByRole('button', { name: /^Leave$/i })) + expect(confirmSpy).toHaveBeenCalled() + // Give a microtask to confirm no request fires. + await new Promise((r) => setTimeout(r, 30)) + expect(requestWithAsyncResult).not.toHaveBeenCalled() + } finally { + confirmSpy.mockRestore() + } + }) + + it('hides Promote / Demote / Remove on every row when the current user is not an owner', async () => { + // Bob is a regular member; he must see only Leave (on his own row) and + // no admin controls anywhere else. + useNats.mockReturnValue({ + user: { account: 'bob', siteId: 'site-A' }, + request: vi.fn().mockResolvedValue({ members: baseMembers }), + requestWithAsyncResult: vi.fn(), + }) + render() + await screen.findByText('Alice A') + // No Promote/Demote/Remove anywhere (across individuals and orgs). + expect(screen.queryByRole('button', { name: /Promote/i })).toBeNull() + expect(screen.queryByRole('button', { name: /Demote/i })).toBeNull() + expect(screen.queryByRole('button', { name: /^Remove/i })).toBeNull() + // Bob still sees a Leave button on his own row. + expect(screen.getByRole('button', { name: /^Leave$/i })).toBeInTheDocument() + }) + + it('surfaces a server error from an action as a banner', async () => { + const requestWithAsyncResult = vi.fn().mockRejectedValue(new Error('only owners can remove members')) + useNats.mockReturnValue({ + user: { account: 'alice', siteId: 'site-A' }, + request: vi.fn().mockResolvedValue({ members: baseMembers }), + requestWithAsyncResult, + }) + render() + await screen.findByText('Bob B') + fireEvent.click(screen.getByRole('button', { name: /Remove bob/i })) + expect(await screen.findByText(/only owners/i)).toBeInTheDocument() + }) + + describe('org expansion', () => { + function setupWithOrgFetch(orgMembersResponse) { + // Reuse the base roster mock, then layer the orgs.{id}.members + // response for the second request call (the first call is + // member.list?enrich at mount; the second is orgMembers). + const request = vi.fn() + request.mockImplementation((subject) => { + if (subject.includes('.orgs.') && subject.endsWith('.members')) { + return orgMembersResponse instanceof Error + ? Promise.reject(orgMembersResponse) + : Promise.resolve(orgMembersResponse) + } + return Promise.resolve({ members: baseMembers }) + }) + useNats.mockReturnValue({ + user: { account: 'alice', siteId: 'site-A' }, + request, + requestWithAsyncResult: vi.fn().mockResolvedValue({ sync: { status: 'accepted' }, async: { status: 'ok' } }), + }) + return { request } + } + + const orgMembersPayload = { + members: [ + { id: 'u-dave', account: 'dave', engName: 'Dave Davies', chineseName: '戴文偉', siteId: 'site-A' }, + { id: 'u-erin', account: 'erin', engName: 'Erin Evans', chineseName: '葉伊蓮', siteId: 'site-A' }, + { id: 'u-frank', account: 'frank', engName: 'Frank Fischer', chineseName: '費法蘭', siteId: 'site-A' }, + ], + } + + it('clicking the org name fires chat.user.{a}.request.orgs.{id}.members and renders the expanded children', async () => { + const { request } = setupWithOrgFetch(orgMembersPayload) + render() + await screen.findByText('Engineering') + fireEvent.click(screen.getByRole('button', { name: /Expand Engineering/i })) + await waitFor(() => + expect(request).toHaveBeenCalledWith('chat.user.alice.request.orgs.org-eng.members', {}) + ) + // Each child shows engName + chineseName. + expect(await screen.findByText('Dave Davies')).toBeInTheDocument() + expect(screen.getByText('戴文偉')).toBeInTheDocument() + expect(screen.getByText('Erin Evans')).toBeInTheDocument() + expect(screen.getByText('Frank Fischer')).toBeInTheDocument() + }) + + it('child rows have no Promote / Demote / Remove buttons (org row owns the lifecycle)', async () => { + setupWithOrgFetch(orgMembersPayload) + render() + await screen.findByText('Engineering') + fireEvent.click(screen.getByRole('button', { name: /Expand Engineering/i })) + await screen.findByText('Dave Davies') + // No buttons targeting an org child. + expect(screen.queryByRole('button', { name: /Remove dave/i })).toBeNull() + expect(screen.queryByRole('button', { name: /Promote dave/i })).toBeNull() + expect(screen.queryByRole('button', { name: /Demote dave/i })).toBeNull() + expect(screen.queryByRole('button', { name: /Remove erin/i })).toBeNull() + }) + + it('clicking the org name a second time collapses without refetching (cached response is reused)', async () => { + const { request } = setupWithOrgFetch(orgMembersPayload) + render() + await screen.findByText('Engineering') + const toggle = screen.getByRole('button', { name: /Expand Engineering/i }) + + // First expand → fetches. + fireEvent.click(toggle) + await screen.findByText('Dave Davies') + const fetchCallsAfterOpen = request.mock.calls.filter( + (call) => call[0] === 'chat.user.alice.request.orgs.org-eng.members' + ).length + expect(fetchCallsAfterOpen).toBe(1) + + // Collapse → no children visible, no new fetch. + fireEvent.click(screen.getByRole('button', { name: /Collapse Engineering/i })) + expect(screen.queryByText('Dave Davies')).not.toBeInTheDocument() + + // Re-expand → still cached, no second fetch. + fireEvent.click(screen.getByRole('button', { name: /Expand Engineering/i })) + await screen.findByText('Dave Davies') + const fetchCallsAfterReopen = request.mock.calls.filter( + (call) => call[0] === 'chat.user.alice.request.orgs.org-eng.members' + ).length + expect(fetchCallsAfterReopen).toBe(1) + }) + + it('shows a "Failed to load members" fallback when the org-members RPC errors', async () => { + setupWithOrgFetch(new Error('not subscribed')) + render() + await screen.findByText('Engineering') + fireEvent.click(screen.getByRole('button', { name: /Expand Engineering/i })) + expect(await screen.findByText(/Failed to load members/i)).toBeInTheDocument() + }) + + it('a stale toggle (rapid expand → collapse → re-expand) only writes the latest fetch result to state', async () => { + // A user rapidly expanding then collapsing the same org would + // otherwise let the first fetch's response overwrite the cached + // state set by the second fetch. The orgFetchGenRef guard drops + // stale resolutions. + let resolveFirst + const firstFetch = new Promise((r) => { resolveFirst = r }) + const secondPayload = { + members: [{ id: 'u-late', account: 'late', engName: 'Late Arrival', chineseName: '', siteId: 'site-A' }], + } + let orgFetchCount = 0 + const request = vi.fn().mockImplementation((subject) => { + if (subject.includes('.orgs.') && subject.endsWith('.members')) { + orgFetchCount += 1 + return orgFetchCount === 1 ? firstFetch : Promise.resolve(secondPayload) + } + return Promise.resolve({ members: baseMembers }) + }) + useNats.mockReturnValue({ + user: { account: 'alice', siteId: 'site-A' }, + request, + requestWithAsyncResult: vi.fn(), + }) + render() + await screen.findByText('Engineering') + + // First expand → first fetch in flight (not yet resolved). + fireEvent.click(screen.getByRole('button', { name: /Expand Engineering/i })) + // Collapse — clears expanded state. + fireEvent.click(screen.getByRole('button', { name: /Collapse Engineering/i })) + // Re-expand → second fetch fires (and resolves immediately). + fireEvent.click(screen.getByRole('button', { name: /Expand Engineering/i })) + await screen.findByText('Late Arrival') + + // NOW the first (stale) fetch resolves with a stale payload that + // would, without the gen guard, clobber 'Late Arrival'. + await act(async () => { + resolveFirst({ + members: [{ id: 'u-stale', account: 'stale', engName: 'Stale Result', chineseName: '', siteId: 'site-A' }], + }) + await Promise.resolve() + }) + // The stale write is dropped — the latest fetch's result wins. + expect(screen.queryByText('Stale Result')).not.toBeInTheDocument() + expect(screen.getByText('Late Arrival')).toBeInTheDocument() + }) + + it('non-owner viewer can still expand orgs even though no Remove button is shown', async () => { + const request = vi.fn() + request.mockImplementation((subject) => { + if (subject.includes('.orgs.') && subject.endsWith('.members')) { + return Promise.resolve(orgMembersPayload) + } + return Promise.resolve({ members: baseMembers }) + }) + useNats.mockReturnValue({ + user: { account: 'bob', siteId: 'site-A' }, // bob is a regular member + request, + requestWithAsyncResult: vi.fn(), + }) + render() + await screen.findByText('Engineering') + // No org-level Remove button for bob. + expect(screen.queryByRole('button', { name: /Remove org-eng/i })).toBeNull() + // But Expand still works. + fireEvent.click(screen.getByRole('button', { name: /Expand Engineering/i })) + expect(await screen.findByText('Dave Davies')).toBeInTheDocument() + }) + }) +}) diff --git a/chat-frontend/src/components/manageMembers/RemoveMemberForm.jsx b/chat-frontend/src/components/manageMembers/RemoveMemberForm.jsx deleted file mode 100644 index 2cedfff51..000000000 --- a/chat-frontend/src/components/manageMembers/RemoveMemberForm.jsx +++ /dev/null @@ -1,66 +0,0 @@ -import { useEffect, useRef, useState } from 'react' -import { useNats } from '../../context/NatsContext' -import { memberRemove } from '../../lib/subjects' - -export default function RemoveMemberForm({ room }) { - const { user, request } = useNats() - const [account, setAccount] = useState('') - const [loading, setLoading] = useState(false) - const [error, setError] = useState(null) - const [success, setSuccess] = useState(false) - const successTimer = useRef(null) - - useEffect(() => { - return () => { - if (successTimer.current) clearTimeout(successTimer.current) - } - }, []) - - const trimmed = account.trim() - const canSubmit = trimmed.length > 0 - - const handleSubmit = async (e) => { - e.preventDefault() - if (!canSubmit || !user) return - setLoading(true) - setError(null) - setSuccess(false) - try { - await request(memberRemove(user.account, room.id, room.siteId), { - roomId: room.id, - account: trimmed, - }) - setAccount('') - setSuccess(true) - if (successTimer.current) clearTimeout(successTimer.current) - successTimer.current = setTimeout(() => setSuccess(false), 3000) - } catch (err) { - setError(err.message) - } finally { - setLoading(false) - } - } - - return ( -
    - - setAccount(e.target.value)} - placeholder="e.g. bob" - disabled={loading} - /> - - {error &&
    {error}
    } - {success &&
    Accepted
    } - -
    - -
    -
    - ) -} diff --git a/chat-frontend/src/components/manageMembers/RemoveMemberForm.test.jsx b/chat-frontend/src/components/manageMembers/RemoveMemberForm.test.jsx deleted file mode 100644 index 46d6b03c0..000000000 --- a/chat-frontend/src/components/manageMembers/RemoveMemberForm.test.jsx +++ /dev/null @@ -1,61 +0,0 @@ -import { describe, it, expect, vi, beforeEach } from 'vitest' -import { render, screen, fireEvent, waitFor } from '@testing-library/react' -import RemoveMemberForm from './RemoveMemberForm' - -vi.mock('../../context/NatsContext', () => ({ - useNats: vi.fn(), -})) - -import { useNats } from '../../context/NatsContext' - -const room = { id: 'r1', siteId: 'site-A', name: 'general' } - -function setup(overrides = {}) { - const request = vi.fn().mockResolvedValue({ status: 'accepted' }) - useNats.mockReturnValue({ - user: { account: 'alice' }, - request, - ...overrides, - }) - render() - return { request } -} - -describe('RemoveMemberForm', () => { - beforeEach(() => { - useNats.mockReset() - }) - - it('disables submit when account input is empty', () => { - setup() - expect(screen.getByRole('button', { name: /^Remove$/ })).toBeDisabled() - }) - - it('submits {roomId, account} with the correct subject', async () => { - const { request } = setup() - fireEvent.change(screen.getByLabelText(/Account/i), { target: { value: ' bob ' } }) - fireEvent.click(screen.getByRole('button', { name: /^Remove$/ })) - await waitFor(() => expect(request).toHaveBeenCalledTimes(1)) - expect(request).toHaveBeenCalledWith( - 'chat.user.alice.request.room.r1.site-A.member.remove', - { roomId: 'r1', account: 'bob' } - ) - }) - - it('shows the server error in a banner on rejection', async () => { - const request = vi.fn().mockRejectedValue(new Error('only owners can remove members')) - setup({ request }) - fireEvent.change(screen.getByLabelText(/Account/i), { target: { value: 'bob' } }) - fireEvent.click(screen.getByRole('button', { name: /^Remove$/ })) - expect(await screen.findByText(/only owners/)).toBeInTheDocument() - }) - - it('clears the input and shows Accepted on success', async () => { - setup() - const input = screen.getByLabelText(/Account/i) - fireEvent.change(input, { target: { value: 'bob' } }) - fireEvent.click(screen.getByRole('button', { name: /^Remove$/ })) - expect(await screen.findByText('Accepted')).toBeInTheDocument() - expect(input.value).toBe('') - }) -}) diff --git a/chat-frontend/src/components/manageMembers/RemoveOrgForm.jsx b/chat-frontend/src/components/manageMembers/RemoveOrgForm.jsx deleted file mode 100644 index 82fe606f2..000000000 --- a/chat-frontend/src/components/manageMembers/RemoveOrgForm.jsx +++ /dev/null @@ -1,66 +0,0 @@ -import { useEffect, useRef, useState } from 'react' -import { useNats } from '../../context/NatsContext' -import { memberRemove } from '../../lib/subjects' - -export default function RemoveOrgForm({ room }) { - const { user, request } = useNats() - const [orgId, setOrgId] = useState('') - const [loading, setLoading] = useState(false) - const [error, setError] = useState(null) - const [success, setSuccess] = useState(false) - const successTimer = useRef(null) - - useEffect(() => { - return () => { - if (successTimer.current) clearTimeout(successTimer.current) - } - }, []) - - const trimmed = orgId.trim() - const canSubmit = trimmed.length > 0 - - const handleSubmit = async (e) => { - e.preventDefault() - if (!canSubmit || !user) return - setLoading(true) - setError(null) - setSuccess(false) - try { - await request(memberRemove(user.account, room.id, room.siteId), { - roomId: room.id, - orgId: trimmed, - }) - setOrgId('') - setSuccess(true) - if (successTimer.current) clearTimeout(successTimer.current) - successTimer.current = setTimeout(() => setSuccess(false), 3000) - } catch (err) { - setError(err.message) - } finally { - setLoading(false) - } - } - - return ( -
    - - setOrgId(e.target.value)} - placeholder="e.g. eng-frontend" - disabled={loading} - /> - - {error &&
    {error}
    } - {success &&
    Accepted
    } - -
    - -
    -
    - ) -} diff --git a/chat-frontend/src/components/manageMembers/RemoveOrgForm.test.jsx b/chat-frontend/src/components/manageMembers/RemoveOrgForm.test.jsx deleted file mode 100644 index 160646169..000000000 --- a/chat-frontend/src/components/manageMembers/RemoveOrgForm.test.jsx +++ /dev/null @@ -1,61 +0,0 @@ -import { describe, it, expect, vi, beforeEach } from 'vitest' -import { render, screen, fireEvent, waitFor } from '@testing-library/react' -import RemoveOrgForm from './RemoveOrgForm' - -vi.mock('../../context/NatsContext', () => ({ - useNats: vi.fn(), -})) - -import { useNats } from '../../context/NatsContext' - -const room = { id: 'r1', siteId: 'site-A', name: 'general' } - -function setup(overrides = {}) { - const request = vi.fn().mockResolvedValue({ status: 'accepted' }) - useNats.mockReturnValue({ - user: { account: 'alice' }, - request, - ...overrides, - }) - render() - return { request } -} - -describe('RemoveOrgForm', () => { - beforeEach(() => { - useNats.mockReset() - }) - - it('disables submit when org id is empty', () => { - setup() - expect(screen.getByRole('button', { name: /Remove Org/i })).toBeDisabled() - }) - - it('submits {roomId, orgId} with the correct subject', async () => { - const { request } = setup() - fireEvent.change(screen.getByLabelText(/Org ID/i), { target: { value: ' eng-frontend ' } }) - fireEvent.click(screen.getByRole('button', { name: /Remove Org/i })) - await waitFor(() => expect(request).toHaveBeenCalledTimes(1)) - expect(request).toHaveBeenCalledWith( - 'chat.user.alice.request.room.r1.site-A.member.remove', - { roomId: 'r1', orgId: 'eng-frontend' } - ) - }) - - it('shows the server error in a banner on rejection', async () => { - const request = vi.fn().mockRejectedValue(new Error('only owners can remove orgs')) - setup({ request }) - fireEvent.change(screen.getByLabelText(/Org ID/i), { target: { value: 'eng' } }) - fireEvent.click(screen.getByRole('button', { name: /Remove Org/i })) - expect(await screen.findByText(/only owners/)).toBeInTheDocument() - }) - - it('clears the input and shows Accepted on success', async () => { - setup() - const input = screen.getByLabelText(/Org ID/i) - fireEvent.change(input, { target: { value: 'eng' } }) - fireEvent.click(screen.getByRole('button', { name: /Remove Org/i })) - expect(await screen.findByText('Accepted')).toBeInTheDocument() - expect(input.value).toBe('') - }) -}) diff --git a/chat-frontend/src/components/manageMembers/RoleUpdateForm.jsx b/chat-frontend/src/components/manageMembers/RoleUpdateForm.jsx deleted file mode 100644 index 6822bce26..000000000 --- a/chat-frontend/src/components/manageMembers/RoleUpdateForm.jsx +++ /dev/null @@ -1,80 +0,0 @@ -import { useEffect, useRef, useState } from 'react' -import { useNats } from '../../context/NatsContext' -import { memberRoleUpdate } from '../../lib/subjects' -import { ROLE_OWNER, ROLE_MEMBER } from '../../lib/constants' - -export default function RoleUpdateForm({ room }) { - const { user, request } = useNats() - const [account, setAccount] = useState('') - const [newRole, setNewRole] = useState(ROLE_OWNER) - const [loading, setLoading] = useState(false) - const [error, setError] = useState(null) - const [success, setSuccess] = useState(false) - const successTimer = useRef(null) - - useEffect(() => { - return () => { - if (successTimer.current) clearTimeout(successTimer.current) - } - }, []) - - const trimmed = account.trim() - const canSubmit = trimmed.length > 0 - - const handleSubmit = async (e) => { - e.preventDefault() - if (!canSubmit || !user) return - setLoading(true) - setError(null) - setSuccess(false) - try { - await request(memberRoleUpdate(user.account, room.id, room.siteId), { - roomId: room.id, - account: trimmed, - newRole, - }) - setAccount('') - setSuccess(true) - if (successTimer.current) clearTimeout(successTimer.current) - successTimer.current = setTimeout(() => setSuccess(false), 3000) - } catch (err) { - setError(err.message) - } finally { - setLoading(false) - } - } - - return ( -
    - - setAccount(e.target.value)} - placeholder="e.g. bob" - disabled={loading} - /> - - - - - {error &&
    {error}
    } - {success &&
    Accepted
    } - -
    - -
    -
    - ) -} diff --git a/chat-frontend/src/components/manageMembers/RoleUpdateForm.test.jsx b/chat-frontend/src/components/manageMembers/RoleUpdateForm.test.jsx deleted file mode 100644 index d8833e3a8..000000000 --- a/chat-frontend/src/components/manageMembers/RoleUpdateForm.test.jsx +++ /dev/null @@ -1,70 +0,0 @@ -import { describe, it, expect, vi, beforeEach } from 'vitest' -import { render, screen, fireEvent, waitFor } from '@testing-library/react' -import RoleUpdateForm from './RoleUpdateForm' - -vi.mock('../../context/NatsContext', () => ({ - useNats: vi.fn(), -})) - -import { useNats } from '../../context/NatsContext' - -const room = { id: 'r1', siteId: 'site-A', name: 'general' } - -function setup(overrides = {}) { - const request = vi.fn().mockResolvedValue({ status: 'accepted' }) - useNats.mockReturnValue({ - user: { account: 'alice' }, - request, - ...overrides, - }) - render() - return { request } -} - -describe('RoleUpdateForm', () => { - beforeEach(() => { - useNats.mockReset() - }) - - it('disables submit when account input is empty', () => { - setup() - expect(screen.getByRole('button', { name: /Update Role/i })).toBeDisabled() - }) - - it('submits newRole=owner by default', async () => { - const { request } = setup() - fireEvent.change(screen.getByLabelText(/Account/i), { target: { value: 'bob' } }) - fireEvent.click(screen.getByRole('button', { name: /Update Role/i })) - await waitFor(() => expect(request).toHaveBeenCalledTimes(1)) - expect(request).toHaveBeenCalledWith( - 'chat.user.alice.request.room.r1.site-A.member.role-update', - { roomId: 'r1', account: 'bob', newRole: 'owner' } - ) - }) - - it('submits newRole=member when the select is changed', async () => { - const { request } = setup() - fireEvent.change(screen.getByLabelText(/Account/i), { target: { value: 'bob' } }) - fireEvent.change(screen.getByLabelText(/Role/i), { target: { value: 'member' } }) - fireEvent.click(screen.getByRole('button', { name: /Update Role/i })) - await waitFor(() => expect(request).toHaveBeenCalledTimes(1)) - expect(request.mock.calls[0][1]).toEqual({ roomId: 'r1', account: 'bob', newRole: 'member' }) - }) - - it('shows the server error in a banner on rejection', async () => { - const request = vi.fn().mockRejectedValue(new Error('cannot demote: you are the last owner')) - setup({ request }) - fireEvent.change(screen.getByLabelText(/Account/i), { target: { value: 'bob' } }) - fireEvent.click(screen.getByRole('button', { name: /Update Role/i })) - expect(await screen.findByText(/last owner/)).toBeInTheDocument() - }) - - it('clears the account input and shows Accepted on success', async () => { - setup() - const input = screen.getByLabelText(/Account/i) - fireEvent.change(input, { target: { value: 'bob' } }) - fireEvent.click(screen.getByRole('button', { name: /Update Role/i })) - expect(await screen.findByText('Accepted')).toBeInTheDocument() - expect(input.value).toBe('') - }) -}) diff --git a/chat-frontend/src/context/NatsContext.jsx b/chat-frontend/src/context/NatsContext.jsx index 83a31374a..aa8d3b020 100644 --- a/chat-frontend/src/context/NatsContext.jsx +++ b/chat-frontend/src/context/NatsContext.jsx @@ -1,7 +1,8 @@ -import { createContext, useContext, useRef, useState, useCallback } from 'react' +import { createContext, useContext, useRef, useState, useCallback, useMemo } from 'react' import { connect as natsConnect, StringCodec, jwtAuthenticator } from 'nats.ws' import { createUser } from 'nkeys.js' import { AUTH_URL, NATS_URL } from '../lib/runtimeConfig' +import { requestWithAsyncResult as asyncJobRequest } from '../lib/asyncJob' export const NatsContext = createContext(null) @@ -16,9 +17,18 @@ export function NatsProvider({ children }) { const authUrl = AUTH_URL const natsUrl = NATS_URL - // connectToNats accepts an opts object with one of two shapes: - // { mode: 'dev', account, siteId } — dev login by account name - // { mode: 'sso', ssoToken, siteId } — production login via OIDC access token + /** + * Authenticate against auth-service and open the NATS WebSocket + * connection. On success, `user`/`connected` flip true and any + * subsequent server-initiated close updates `error`. + * + * @param {Object} opts + * @param {'dev'|'sso'} opts.mode + * @param {string} [opts.account] Dev mode: account name to log in as. + * @param {string} [opts.ssoToken] Production mode: OIDC access token. + * @param {string} opts.siteId + * @throws if auth-service rejects or the NATS handshake fails. + */ const connectToNats = useCallback(async (opts) => { setError(null) @@ -62,6 +72,18 @@ export function NatsProvider({ children }) { }) }, [authUrl, natsUrl]) + /** + * Send a synchronous NATS request/reply. Use this for handlers that + * return their full result inline (e.g. `member.list`, `search.rooms`). + * For deferred-result operations use `requestWithAsyncResult` instead. + * + * @param {string} subject + * @param {unknown} [data={}] JSON-serialisable payload. + * @returns {Promise} Parsed JSON reply. + * @throws if not connected, the request times out (5s), or the reply + * carries `{error}` — in the last case the thrown Error's message + * is the server's user-safe error string. + */ const request = useCallback(async (subject, data = {}) => { if (!ncRef.current) throw new Error('Not connected') const payload = sc.encode(JSON.stringify(data)) @@ -71,12 +93,57 @@ export function NatsProvider({ children }) { return parsed }, []) + /** + * Two-phase request/reply for operations whose sync reply is just + * "accepted" — the real outcome arrives later on the per-request + * response subject as an AsyncJobResult. Components await this and + * get the final ok/error from the worker, not the optimistic accept. + * + * Injects the current `user.account` and the live `nc`; for the full + * contract see {@link asyncJobRequest} in `lib/asyncJob.js`. + * + * @param {string} subject + * @param {unknown} [data={}] + * @param {Object} [opts] Forwarded to the helper (`treatAsSuccess`, + * `requestId`, `syncTimeout`, `asyncTimeout`). + * @returns {Promise<{requestId: string, sync: unknown, async: unknown}>} + * @throws Tagged Error with `.kind` from ASYNC_JOB_ERROR_KINDS on every + * failure path; use `formatAsyncJobError` for user-facing text. + */ + const requestWithAsyncResult = useCallback(async (subject, data = {}, opts = {}) => { + if (!ncRef.current) throw new Error('Not connected') + const account = user?.account + if (!account) throw new Error('Not authenticated') + return asyncJobRequest(ncRef.current, account, subject, data, opts) + }, [user]) + + /** + * Fire-and-forget JSON publish. Use for events the server consumes + * via QueueSubscribe (no reply expected); for request/reply use + * `request` or `requestWithAsyncResult`. + * + * @param {string} subject + * @param {unknown} [data={}] + * @throws if not connected. + */ const publish = useCallback((subject, data = {}) => { if (!ncRef.current) throw new Error('Not connected') const payload = sc.encode(JSON.stringify(data)) ncRef.current.publish(subject, payload) }, []) + /** + * Subscribe to a subject pattern and dispatch parsed JSON messages + * to `callback`. Malformed JSON is silently skipped (server + * canonical events are always JSON). + * + * @param {string} subject + * @param {(data: unknown) => void} callback + * @returns {{unsubscribe: () => void}} The underlying NATS + * subscription. Callers MUST call `.unsubscribe()` on unmount / + * cleanup to avoid leaking the iterator and the server-side sid. + * @throws if not connected. + */ const subscribe = useCallback((subject, callback) => { if (!ncRef.current) throw new Error('Not connected') const sub = ncRef.current.subscribe(subject) @@ -93,6 +160,11 @@ export function NatsProvider({ children }) { return sub }, []) + /** + * Drain the NATS connection (flushes pending publishes, then closes) + * and reset `user`/`connected`. Idempotent: calling on a disconnected + * provider is a no-op. + */ const disconnect = useCallback(async () => { if (ncRef.current) { await ncRef.current.drain() @@ -102,14 +174,18 @@ export function NatsProvider({ children }) { setUser(null) }, []) - return ( - ({ connected, user, error, - connect: connectToNats, request, publish, subscribe, disconnect, - }}> - {children} - + connect: connectToNats, request, requestWithAsyncResult, publish, subscribe, disconnect, + }), + [connected, user, error, connectToNats, request, requestWithAsyncResult, publish, subscribe, disconnect] ) + + return {children} } export function useNats() { diff --git a/chat-frontend/src/context/RoomEventsContext.jsx b/chat-frontend/src/context/RoomEventsContext.jsx index 899b9e830..9d02a663e 100644 --- a/chat-frontend/src/context/RoomEventsContext.jsx +++ b/chat-frontend/src/context/RoomEventsContext.jsx @@ -43,7 +43,8 @@ export function RoomEventsProvider({ children }) { const openChannelSub = (roomId) => { if (channelSubs.current.has(roomId)) return - const sub = subscribe(roomEvent(roomId), (evt) => { + const subj = roomEvent(roomId) + const sub = subscribe(subj, (evt) => { if (evt?.type === 'new_message') { const hasMention = (evt.mentions ?? []).some( (p) => p.account === user.account @@ -68,7 +69,13 @@ export function RoomEventsProvider({ children }) { request(roomsGet(user.account, evt.subscription.roomId), {}) .then((room) => { if (cancelledRef.current || !room) return - safeDispatch({ type: 'ROOM_ADDED', room }) + // DM rooms have no canonical Room.Name server-side — the friendly + // text lives on the user's Subscription. Stash that here so the + // sidebar + header can fall back to it via roomDisplayName(room). + const merged = evt.subscription?.name + ? { ...room, subscriptionName: evt.subscription.name } + : room + safeDispatch({ type: 'ROOM_ADDED', room: merged }) if (room.type === 'channel') openChannelSub(room.id) }) .catch(() => {}) diff --git a/chat-frontend/src/lib/asyncJob.js b/chat-frontend/src/lib/asyncJob.js new file mode 100644 index 000000000..7594051af --- /dev/null +++ b/chat-frontend/src/lib/asyncJob.js @@ -0,0 +1,175 @@ +// requestWithAsyncResult bridges the room-service two-phase reply contract: +// 1. sync NATS reply → typically {status:"accepted", …} (or {error}) +// 2. async result → AsyncJobResult on chat.user.{account}.response.{requestID} +// +// The response subscription is opened BEFORE the request is published so a +// fast worker can't beat us to the punch. The X-Request-ID header on the +// request is what tells room-worker which response subject to publish on — +// without it the async result is never emitted. + +import { StringCodec, headers as natsHeaders } from 'nats.ws' +import { v7 as uuidv7 } from 'uuid' +import { userResponse } from './subjects' + +const sc = StringCodec() + +const DEFAULT_SYNC_TIMEOUT = 5000 +const DEFAULT_ASYNC_TIMEOUT = 30000 + +// Error kinds attached to every Error thrown from this helper so callers +// can distinguish wire-level failures from server-reported ones without +// string-matching the message. +export const ASYNC_JOB_ERROR_KINDS = Object.freeze({ + SyncError: 'sync-error', + AsyncError: 'async-error', + AsyncTimeout: 'async-timeout', + SubscriptionClosed: 'subscription-closed', +}) + +function taggedError(message, kind, cause) { + const err = new Error(message) + err.kind = kind + if (cause) err.cause = cause + return err +} + +/** + * User-facing message for an error thrown by `requestWithAsyncResult`. + * + * Server-side errors (`SyncError`, `AsyncError`) already carry a sanitised, + * user-safe message and are returned as-is. Wire-level failures + * (`AsyncTimeout`, `SubscriptionClosed`) get a friendlier hint that says what + * happened and what the user can do about it — the raw "async result timeout" + * isn't actionable. + * + * @param {Error & {kind?: string}} err + * @returns {string} + */ +export function formatAsyncJobError(err) { + if (!err) return '' + switch (err.kind) { + case ASYNC_JOB_ERROR_KINDS.AsyncTimeout: + return "The server didn't respond in time. The action may still complete — refresh to check." + case ASYNC_JOB_ERROR_KINDS.SubscriptionClosed: + return 'Connection interrupted before the server confirmed. Refresh to check the result.' + default: + return err.message ?? '' + } +} + +/** + * Issues a NATS request whose handler responds in two phases: a sync reply + * (typically `{status:"accepted"}` or `{error}`) followed by an + * `AsyncJobResult` published to `chat.user.{account}.response.{requestID}` + * once the underlying worker finishes. + * + * Subscribes to the response subject BEFORE publishing so a fast worker + * can't beat the client to the punch. Sets the `X-Request-ID` NATS header + * so the worker knows where to publish the async result. + * + * @param {object} nc nats.ws connection + * @param {string} account requester's account (used to build the response subject) + * @param {string} subject request subject (e.g. result of `roomCreate(...)`) + * @param {object} payload JSON-serialisable request body + * @param {object} [opts] + * @param {string} [opts.requestId] defaults to a fresh UUIDv7 + * @param {number} [opts.syncTimeout=5000] ms to wait for sync reply + * @param {number} [opts.asyncTimeout=30000] ms to wait for AsyncJobResult after sync accept + * @param {(reply: object) => boolean} [opts.treatAsSuccess] + * Predicate over the sync reply. Returning true short-circuits: caller gets + * `{requestId, sync, async: null}` instead of throwing on `sync.error`. + * Use this for "200-with-error-but-actually-success" replies (e.g. DM-exists + * dedup — see `isDMExistsReply`). + * + * @returns {Promise<{requestId: string, sync: object, async: object|null}>} + * @throws {Error & {kind: string}} On any failure, with `err.kind` set to one + * of {@link ASYNC_JOB_ERROR_KINDS}. Pass to `formatAsyncJobError(err)` for + * user-facing text. + */ +export async function requestWithAsyncResult(nc, account, subject, payload, opts = {}) { + const { + requestId = uuidv7(), + syncTimeout = DEFAULT_SYNC_TIMEOUT, + asyncTimeout = DEFAULT_ASYNC_TIMEOUT, + treatAsSuccess, + } = opts + + const sub = nc.subscribe(userResponse(account, requestId), { max: 1 }) + + // Register before request resolves so a result that arrives during the + // sync window is buffered, not dropped. Tagged-envelope resolves never + // reject so late cleanup signals can't surface as unhandled rejections. + let resolveAsync + const asyncPromise = new Promise((res) => { resolveAsync = res }) + ;(async () => { + try { + for await (const msg of sub) { + resolveAsync({ kind: 'data', data: JSON.parse(sc.decode(msg.data)) }) + return + } + resolveAsync({ kind: 'closed' }) + } catch (err) { + resolveAsync({ kind: 'error', error: err }) + } + })() + + const cleanupSub = () => { + try { sub.unsubscribe() } catch { /* already closed */ } + } + + let sync + try { + const h = natsHeaders() + h.set('X-Request-ID', requestId) + const resp = await nc.request(subject, sc.encode(JSON.stringify(payload)), { + timeout: syncTimeout, + headers: h, + }) + sync = JSON.parse(sc.decode(resp.data)) + } catch (err) { + cleanupSub() + throw taggedError(err.message, ASYNC_JOB_ERROR_KINDS.SyncError, err) + } + + if (sync.error) { + // DM-exists and similar "200 with error+roomId" replies are success cases + // for the caller. The caller opts into this with treatAsSuccess(reply). + if (treatAsSuccess && treatAsSuccess(sync)) { + cleanupSub() + return { requestId, sync, async: null } + } + cleanupSub() + throw taggedError(sync.error, ASYNC_JOB_ERROR_KINDS.SyncError) + } + + let timer + try { + const timeoutPromise = new Promise((resolve) => { + timer = setTimeout(() => resolve({ kind: 'timeout' }), asyncTimeout) + }) + const envelope = await Promise.race([asyncPromise, timeoutPromise]) + clearTimeout(timer) + if (envelope.kind === 'timeout') { + throw taggedError('async result timeout', ASYNC_JOB_ERROR_KINDS.AsyncTimeout) + } + if (envelope.kind === 'error') { + throw taggedError(envelope.error?.message ?? 'subscription error', + ASYNC_JOB_ERROR_KINDS.SubscriptionClosed, envelope.error) + } + if (envelope.kind === 'closed') { + throw taggedError('subscription closed before result arrived', + ASYNC_JOB_ERROR_KINDS.SubscriptionClosed) + } + const asyncResult = envelope.data + if (asyncResult.status === 'error') { + throw taggedError(asyncResult.error || 'operation failed', + ASYNC_JOB_ERROR_KINDS.AsyncError) + } + cleanupSub() + return { requestId, sync, async: asyncResult } + } catch (err) { + clearTimeout(timer) + cleanupSub() + throw err + } +} diff --git a/chat-frontend/src/lib/asyncJob.test.js b/chat-frontend/src/lib/asyncJob.test.js new file mode 100644 index 000000000..e36fc8892 --- /dev/null +++ b/chat-frontend/src/lib/asyncJob.test.js @@ -0,0 +1,259 @@ +import { describe, it, expect, vi } from 'vitest' +import { StringCodec } from 'nats.ws' +import { requestWithAsyncResult, ASYNC_JOB_ERROR_KINDS, formatAsyncJobError } from './asyncJob' + +const sc = StringCodec() + +// makeFakeSub builds a minimal duck-typed nats subscription whose +// async-iterator yields once we call push(). unsubscribe() ends iteration. +function makeFakeSub() { + const buf = [] + let resolveWait + let closed = false + return { + push(msg) { + buf.push(msg) + if (resolveWait) { + const r = resolveWait + resolveWait = null + r() + } + }, + unsubscribe() { + closed = true + if (resolveWait) { + const r = resolveWait + resolveWait = null + r() + } + }, + [Symbol.asyncIterator]() { + return { + async next() { + while (buf.length === 0 && !closed) { + await new Promise((r) => { resolveWait = r }) + } + if (buf.length === 0 && closed) return { value: undefined, done: true } + return { value: buf.shift(), done: false } + }, + } + }, + } +} + +function encode(obj) { + return { data: sc.encode(JSON.stringify(obj)) } +} + +function makeNc({ syncReply, subFactory } = {}) { + const sub = subFactory ? subFactory() : makeFakeSub() + return { + sub, + request: vi.fn(async (subject, _data, opts) => { + if (typeof syncReply === 'function') return encode(await syncReply(subject, opts)) + return encode(syncReply ?? { status: 'accepted' }) + }), + subscribe: vi.fn(() => sub), + } +} + +describe('requestWithAsyncResult', () => { + it('subscribes to the per-request response subject before publishing', async () => { + const nc = makeNc() + const p = requestWithAsyncResult(nc, 'alice', 'chat.user.alice.request.room.s1.create', { name: 'x' }, { + requestId: 'req-1', + }) + expect(nc.subscribe).toHaveBeenCalledWith('chat.user.alice.response.req-1', { max: 1 }) + // subscribe must have been called BEFORE request was awaited + expect(nc.subscribe.mock.invocationCallOrder[0]) + .toBeLessThan(nc.request.mock.invocationCallOrder[0]) + nc.sub.push(encode({ requestId: 'req-1', operation: 'room.create', status: 'ok', roomId: 'r1', timestamp: 1 })) + await p + }) + + it('sends X-Request-ID header on the underlying request', async () => { + const nc = makeNc() + const p = requestWithAsyncResult(nc, 'alice', 'subj', {}, { requestId: 'req-1' }) + nc.sub.push(encode({ requestId: 'req-1', operation: 'room.create', status: 'ok', timestamp: 1 })) + await p + const opts = nc.request.mock.calls[0][2] + expect(opts.headers).toBeDefined() + expect(opts.headers.get('X-Request-ID')).toBe('req-1') + }) + + it('resolves with sync + async results when async status is ok', async () => { + const nc = makeNc({ syncReply: { status: 'accepted', roomId: 'r1', roomType: 'channel' } }) + const p = requestWithAsyncResult(nc, 'alice', 'subj', {}, { requestId: 'req-1' }) + nc.sub.push(encode({ requestId: 'req-1', operation: 'room.create', status: 'ok', roomId: 'r1', timestamp: 1 })) + const result = await p + expect(result.sync).toEqual({ status: 'accepted', roomId: 'r1', roomType: 'channel' }) + expect(result.async).toMatchObject({ status: 'ok', roomId: 'r1' }) + }) + + it('throws with the sync error message and kind=sync-error when the sync reply carries an error', async () => { + const nc = makeNc({ syncReply: { error: 'only owners can add members' } }) + const err = await requestWithAsyncResult(nc, 'alice', 'subj', {}, { requestId: 'req-1' }) + .catch((e) => e) + expect(err.message).toBe('only owners can add members') + expect(err.kind).toBe(ASYNC_JOB_ERROR_KINDS.SyncError) + }) + + it('returns the DM-exists payload (error + roomId) without throwing', async () => { + // Server replies with {error, roomId} for the dedup case — caller must be + // able to distinguish this from a true failure and navigate to roomId. + const nc = makeNc({ syncReply: { error: 'dm already exists', roomId: 'r-existing' } }) + const result = await requestWithAsyncResult(nc, 'alice', 'subj', {}, { + requestId: 'req-1', + treatAsSuccess: (reply) => reply.error === 'dm already exists' && !!reply.roomId, + }) + expect(result.sync).toEqual({ error: 'dm already exists', roomId: 'r-existing' }) + expect(result.async).toBeNull() + }) + + it('throws with kind=async-error when async status is error', async () => { + const nc = makeNc() + const p = requestWithAsyncResult(nc, 'alice', 'subj', {}, { requestId: 'req-1' }) + nc.sub.push(encode({ requestId: 'req-1', operation: 'room.member.add', status: 'error', error: 'remote site unreachable', timestamp: 1 })) + const err = await p.catch((e) => e) + expect(err.message).toBe('remote site unreachable') + expect(err.kind).toBe(ASYNC_JOB_ERROR_KINDS.AsyncError) + }) + + it('rejects with kind=async-timeout if no async result arrives in time', async () => { + vi.useFakeTimers({ shouldAdvanceTime: true }) + try { + const nc = makeNc() + const p = requestWithAsyncResult(nc, 'alice', 'subj', {}, { + requestId: 'req-1', + asyncTimeout: 500, + }) + await Promise.resolve() + await Promise.resolve() + vi.advanceTimersByTime(600) + const err = await p.catch((e) => e) + expect(err.kind).toBe(ASYNC_JOB_ERROR_KINDS.AsyncTimeout) + expect(err.message).toMatch(/timeout/i) + } finally { + vi.useRealTimers() + } + }) + + it('unsubscribes the response subscription on async timeout', async () => { + vi.useFakeTimers({ shouldAdvanceTime: true }) + try { + const nc = makeNc() + const unsubSpy = vi.spyOn(nc.sub, 'unsubscribe') + const p = requestWithAsyncResult(nc, 'alice', 'subj', {}, { + requestId: 'req-1', + asyncTimeout: 100, + }) + await Promise.resolve(); await Promise.resolve() + vi.advanceTimersByTime(200) + await expect(p).rejects.toThrow() + expect(unsubSpy).toHaveBeenCalled() + } finally { + vi.useRealTimers() + } + }) + + it('unsubscribes when the sync reply itself is an error', async () => { + const nc = makeNc({ syncReply: { error: 'invalid request' } }) + const unsubSpy = vi.spyOn(nc.sub, 'unsubscribe') + await expect( + requestWithAsyncResult(nc, 'alice', 'subj', {}, { requestId: 'req-1' }) + ).rejects.toThrow('invalid request') + expect(unsubSpy).toHaveBeenCalled() + }) + + it('rejects with kind=subscription-closed when the sub closes before a result arrives', async () => { + const nc = makeNc() + const p = requestWithAsyncResult(nc, 'alice', 'subj', {}, { + requestId: 'req-1', + asyncTimeout: 10000, + }) + // Let the sync request resolve, then close the subscription without pushing. + await Promise.resolve(); await Promise.resolve() + nc.sub.unsubscribe() + const err = await p.catch((e) => e) + expect(err.kind).toBe(ASYNC_JOB_ERROR_KINDS.SubscriptionClosed) + expect(err.message).toMatch(/subscription closed/i) + }) + + it('unsubscribes on the success path too (defensive — does not rely on max:1 alone)', async () => { + const nc = makeNc() + const unsubSpy = vi.spyOn(nc.sub, 'unsubscribe') + const p = requestWithAsyncResult(nc, 'alice', 'subj', {}, { requestId: 'req-1' }) + nc.sub.push(encode({ requestId: 'req-1', operation: 'room.create', status: 'ok', timestamp: 1 })) + await p + expect(unsubSpy).toHaveBeenCalled() + }) + + it('auto-generates a request id when caller does not supply one', async () => { + const nc = makeNc() + const p = requestWithAsyncResult(nc, 'alice', 'subj', {}) + const subCall = nc.subscribe.mock.calls[0][0] + // UUIDv7: 8-4-4-4-12 hex with the version nibble '7' at the start of the + // third group, and IETF variant nibble (8/9/a/b) at the start of the fourth. + const m = subCall.match(/^chat\.user\.alice\.response\.([0-9a-f]{8}-[0-9a-f]{4}-7[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12})$/i) + expect(m).not.toBeNull() + const generatedId = m[1] + nc.sub.push(encode({ requestId: generatedId, operation: 'room.create', status: 'ok', timestamp: 1 })) + const result = await p + expect(result.requestId).toBe(generatedId) + }) +}) + +describe('formatAsyncJobError', () => { + it('returns a friendly retry hint for AsyncTimeout', () => { + const err = new Error('async result timeout') + err.kind = ASYNC_JOB_ERROR_KINDS.AsyncTimeout + const msg = formatAsyncJobError(err) + expect(msg).toMatch(/server didn.t respond/i) + expect(msg).toMatch(/may still complete|refresh/i) + }) + + it('returns a friendly hint for SubscriptionClosed', () => { + const err = new Error('subscription closed before result arrived') + err.kind = ASYNC_JOB_ERROR_KINDS.SubscriptionClosed + expect(formatAsyncJobError(err)).toMatch(/connection|disconnected|interrupted/i) + }) + + it('returns the raw message for SyncError (server-supplied user-safe text)', () => { + const err = new Error('only owners can add members') + err.kind = ASYNC_JOB_ERROR_KINDS.SyncError + expect(formatAsyncJobError(err)).toBe('only owners can add members') + }) + + it('returns the raw message for AsyncError', () => { + const err = new Error('exceeds maximum capacity') + err.kind = ASYNC_JOB_ERROR_KINDS.AsyncError + expect(formatAsyncJobError(err)).toBe('exceeds maximum capacity') + }) + + it('falls back to message for unknown / untagged errors', () => { + expect(formatAsyncJobError(new Error('random'))).toBe('random') + expect(formatAsyncJobError(null)).toBe('') + expect(formatAsyncJobError(undefined)).toBe('') + }) + + it('handles every ASYNC_JOB_ERROR_KINDS value (no silent drift when a new kind is added)', () => { + // Kinds that intentionally pass the raw server-supplied message through + // (already user-safe via room-service's sanitizeError). All other kinds + // must produce a friendlier client-side hint. + const RAW_MESSAGE_KINDS = new Set([ + ASYNC_JOB_ERROR_KINDS.SyncError, + ASYNC_JOB_ERROR_KINDS.AsyncError, + ]) + for (const kind of Object.values(ASYNC_JOB_ERROR_KINDS)) { + const err = new Error('original-message') + err.kind = kind + const formatted = formatAsyncJobError(err) + if (RAW_MESSAGE_KINDS.has(kind)) { + expect(formatted, `kind=${kind} should pass server message through`).toBe('original-message') + } else { + expect(formatted, `kind=${kind} should produce a branded client-side hint`).not.toBe('original-message') + expect(formatted, `kind=${kind} should produce non-empty hint`).toBeTruthy() + } + } + }) +}) diff --git a/chat-frontend/src/lib/constants.js b/chat-frontend/src/lib/constants.js index 57c66f632..416b55bf3 100644 --- a/chat-frontend/src/lib/constants.js +++ b/chat-frontend/src/lib/constants.js @@ -1,9 +1,23 @@ -// Frontend mirror of Go constants. Keep in sync with: +// Frontend mirror of the server contract: constants the wire uses and a few +// tightly-coupled predicates that test them. Keep in sync with: // pkg/model/subscription.go (Role) // pkg/model/member.go (HistoryMode) +// room-service/helper.go (dmExistsError.Error()) export const ROLE_OWNER = 'owner' export const ROLE_MEMBER = 'member' export const HISTORY_MODE_ALL = 'all' export const HISTORY_MODE_NONE = 'none' + +// Server's "DM already exists" sync-reply error string. The dedup reply is +// shape {error: this, roomId: existingId} — a 200-with-error that callers +// treat as success (open the existing room). +export const ERR_DM_ALREADY_EXISTS = 'dm already exists' + +// Predicate for the DM-exists sync reply. Co-located with the constant +// because they encode the same contract; any caller that wants to treat +// dedup as success should use this rather than re-deriving the check. +export function isDMExistsReply(reply) { + return reply?.error === ERR_DM_ALREADY_EXISTS && !!reply.roomId +} diff --git a/chat-frontend/src/lib/parseList.js b/chat-frontend/src/lib/parseList.js deleted file mode 100644 index 18b8d2d82..000000000 --- a/chat-frontend/src/lib/parseList.js +++ /dev/null @@ -1,6 +0,0 @@ -export function parseList(input) { - return input - .split(',') - .map((s) => s.trim()) - .filter(Boolean) -} diff --git a/chat-frontend/src/lib/parseList.test.js b/chat-frontend/src/lib/parseList.test.js deleted file mode 100644 index 79ae62795..000000000 --- a/chat-frontend/src/lib/parseList.test.js +++ /dev/null @@ -1,20 +0,0 @@ -import { describe, it, expect } from 'vitest' -import { parseList } from './parseList' - -describe('parseList', () => { - it('splits on commas and trims whitespace', () => { - expect(parseList('alice, bob,charlie ')).toEqual(['alice', 'bob', 'charlie']) - }) - - it('filters out empty entries', () => { - expect(parseList('alice,, ,bob')).toEqual(['alice', 'bob']) - }) - - it('returns an empty array for an empty string', () => { - expect(parseList('')).toEqual([]) - }) - - it('returns an empty array for whitespace-only input', () => { - expect(parseList(' , , ')).toEqual([]) - }) -}) diff --git a/chat-frontend/src/lib/roomEventsReducer.js b/chat-frontend/src/lib/roomEventsReducer.js index a987ada64..2e0435ba0 100644 --- a/chat-frontend/src/lib/roomEventsReducer.js +++ b/chat-frontend/src/lib/roomEventsReducer.js @@ -24,6 +24,12 @@ function toSummary(room) { return { id: room.id, name: room.name, + // Per-user friendly name (DM display fallback). RoomEventsContext sets + // this from the inbound subscription.update event; rooms loaded via the + // initial rooms.list don't carry it today (server returns Room, not + // Subscription), so it'll be undefined on first paint — roomDisplayName + // falls back to a placeholder until subscription.update lands. + subscriptionName: room.subscriptionName, type: room.type, siteId: room.siteId, userCount: room.userCount, @@ -96,30 +102,43 @@ export function roomEventsReducer(state, action) { } case 'MESSAGE_RECEIVED': { const evt = action.event - // Channel rooms broadcast encrypted-only events (Message zeroed, - // EncryptedMessage populated). Without client-side crypto we can't - // render those, so just skip them rather than crash on the missing - // .id below. DM rooms always carry a populated .message and proceed - // normally. The DEV_MODE plaintext fallback in broadcast-worker - // populates .message for channels too, so dev sees them. - if (!evt.message || !evt.message.id) { - return state + // Normalize the message payload across the two possible broadcast-worker + // modes: plaintext (evt.message populated) and encrypted-only (only + // evt.encryptedMessage populated; .message is dropped via Go's + // json:omitempty). Until client-side crypto lands we can't decrypt, + // but silently swallowing the event leaves the room visually frozen — + // synthesize a "[encrypted message]" placeholder from the top-level + // lastMsgId/lastMsgAt instead so the user sees something happened. + // The `encrypted: true` marker lets the UI render it differently if + // it wants to (italics, lock icon, etc.); the default message renderer + // just shows the placeholder text. + let msg = evt.message + if ((!msg || !msg.id) && evt.encryptedMessage) { + if (!evt.lastMsgId) return state + msg = { + id: evt.lastMsgId, + roomId: evt.roomId, + content: '[encrypted message]', + createdAt: evt.lastMsgAt ?? new Date(evt.timestamp ?? Date.now()).toISOString(), + encrypted: true, + } } + if (!msg || !msg.id) return state const roomId = evt.roomId const prev = state.roomState[roomId] ?? emptyRoomState() const isActive = state.activeRoomId === roomId if (prev.bufferMode === BUFFER_MODE.HISTORICAL) { if ( - prev.messages.some((m) => m.id === evt.message.id) || - prev.pendingLiveMessages.some((m) => m.id === evt.message.id) + prev.messages.some((m) => m.id === msg.id) || + prev.pendingLiveMessages.some((m) => m.id === msg.id) ) { return state } - const pendingLiveMessages = [...prev.pendingLiveMessages, evt.message] + const pendingLiveMessages = [...prev.pendingLiveMessages, msg] const nextRoomState = { ...prev, pendingLiveMessages, - lastMsgAt: evt.lastMsgAt ?? prev.lastMsgAt, + lastMsgAt: evt.lastMsgAt ?? msg.createdAt ?? prev.lastMsgAt, lastMsgId: evt.lastMsgId ?? prev.lastMsgId, unreadCount: isActive ? prev.unreadCount : prev.unreadCount + 1, hasMention: isActive ? false : prev.hasMention || !!evt.hasMention, @@ -146,12 +165,12 @@ export function roomEventsReducer(state, action) { roomState: { ...state.roomState, [roomId]: nextRoomState }, } } - if (prev.messages.some((m) => m.id === evt.message.id)) return state - const messages = appendBounded(prev.messages, evt.message) + if (prev.messages.some((m) => m.id === msg.id)) return state + const messages = appendBounded(prev.messages, msg) const nextRoomState = { ...prev, messages, - lastMsgAt: evt.lastMsgAt ?? prev.lastMsgAt, + lastMsgAt: evt.lastMsgAt ?? msg.createdAt ?? prev.lastMsgAt, lastMsgId: evt.lastMsgId ?? prev.lastMsgId, unreadCount: isActive ? prev.unreadCount : prev.unreadCount + 1, hasMention: isActive ? false : prev.hasMention || !!evt.hasMention, diff --git a/chat-frontend/src/lib/roomEventsReducer.test.js b/chat-frontend/src/lib/roomEventsReducer.test.js index 8dfc44bdc..76fb30351 100644 --- a/chat-frontend/src/lib/roomEventsReducer.test.js +++ b/chat-frontend/src/lib/roomEventsReducer.test.js @@ -187,6 +187,45 @@ describe('roomEventsReducer: MESSAGE_RECEIVED', () => { expect(next.summaries[0].unreadCount).toBe(1) }) + it('renders a placeholder when only encryptedMessage is present (no plaintext .message)', () => { + // broadcast-worker with ENCRYPTION_ENABLED=true emits events where + // ClientMessage is encrypted into evt.encryptedMessage and evt.message + // is dropped via json:omitempty. Until client-side crypto lands we + // can't decrypt — but silently swallowing the event makes the room + // look frozen. Synthesize a "[encrypted message]" placeholder from + // the top-level lastMsgId/lastMsgAt so the user at least sees that + // a message arrived (and can tell their broadcast-worker is encrypting). + const next = roomEventsReducer(initialState, { + type: 'MESSAGE_RECEIVED', + event: { + type: 'new_message', + roomId: 'a', + lastMsgAt: '2026-04-17T12:00:00Z', + lastMsgId: 'm-enc', + encryptedMessage: { v: 1, ciphertext: 'AAA' }, + // no .message field — the omitempty drop + timestamp: 1, + }, + }) + expect(next.roomState.a.messages).toHaveLength(1) + expect(next.roomState.a.messages[0]).toMatchObject({ + id: 'm-enc', + content: '[encrypted message]', + encrypted: true, + }) + }) + + it('does not drop an event that has both message and encryptedMessage — plaintext wins', () => { + // Forward-compatible: if a future broadcaster sends both lanes (e.g. + // during a rollout), the plaintext path is authoritative. + const next = roomEventsReducer(initialState, { + type: 'MESSAGE_RECEIVED', + event: newMessageEvent({ encryptedMessage: { v: 1, ciphertext: 'XX' } }), + }) + expect(next.roomState.a.messages[0].content).toBe('hi') + expect(next.roomState.a.messages[0].encrypted).not.toBe(true) + }) + it('caps the cached messages at MAX_CACHED, dropping oldest', async () => { const { MAX_CACHED } = await import('./roomEventsReducer') let state = initialState diff --git a/chat-frontend/src/lib/roomFormat.js b/chat-frontend/src/lib/roomFormat.js index 65cd5e405..0ae44729c 100644 --- a/chat-frontend/src/lib/roomFormat.js +++ b/chat-frontend/src/lib/roomFormat.js @@ -1,5 +1,20 @@ export function roomPrefix(type) { - return type === 'dm' ? '@ ' : '# ' + return type === 'dm' || type === 'botDM' ? '@ ' : '# ' +} + +// Resolves the text label for a room in the sidebar / chat header. DM rooms +// have no canonical Room.Name on the server (the server only stores +// subscription-side names for them — each user has their own friendly name +// per DM, typically the counterpart account). When the user-scoped +// subscription event flowed into the reducer it brought that subscription +// name with it; otherwise we fall back to "(DM)" rather than rendering an +// empty string that the user can't act on. +export function roomDisplayName(room) { + if (!room) return '' + if (room.name) return room.name + if (room.subscriptionName) return room.subscriptionName + if (room.type === 'dm' || room.type === 'botDM') return '(DM)' + return room.id ?? '' } export function roomFromSearchHit(hit) { @@ -15,5 +30,5 @@ export function roomFromSearchHit(hit) { // roomPrefix above but without the trailing space — search rows place the // prefix in its own span/div so spacing is handled by CSS, not the string. export function searchRoomPrefix(roomType) { - return roomType === 'dm' ? '@' : '#' + return roomType === 'dm' || roomType === 'botDM' ? '@' : '#' } diff --git a/chat-frontend/src/lib/roomFormat.test.js b/chat-frontend/src/lib/roomFormat.test.js new file mode 100644 index 000000000..dfcd1bb74 --- /dev/null +++ b/chat-frontend/src/lib/roomFormat.test.js @@ -0,0 +1,60 @@ +import { describe, it, expect } from 'vitest' +import { roomPrefix, roomDisplayName, roomFromSearchHit, searchRoomPrefix } from './roomFormat' + +describe('roomPrefix', () => { + it('uses "@ " for dm and botDM rooms, "# " for everything else', () => { + expect(roomPrefix('dm')).toBe('@ ') + expect(roomPrefix('botDM')).toBe('@ ') + expect(roomPrefix('channel')).toBe('# ') + expect(roomPrefix('unknown')).toBe('# ') + }) +}) + +describe('searchRoomPrefix', () => { + it('mirrors roomPrefix without the trailing space', () => { + expect(searchRoomPrefix('dm')).toBe('@') + expect(searchRoomPrefix('channel')).toBe('#') + }) +}) + +describe('roomDisplayName', () => { + it('returns "" for null / undefined', () => { + expect(roomDisplayName(null)).toBe('') + expect(roomDisplayName(undefined)).toBe('') + }) + + it('prefers room.name when set (channels)', () => { + expect(roomDisplayName({ name: 'frontend', type: 'channel', id: 'r1' })).toBe('frontend') + }) + + it('falls back to room.subscriptionName when room.name is empty (DM with subscription event)', () => { + expect( + roomDisplayName({ name: '', subscriptionName: 'bob', type: 'dm', id: 'r-dm' }) + ).toBe('bob') + }) + + it('renders a "(DM)" placeholder for dm / botDM rooms with no name and no subscription name', () => { + // The initial roomsList payload returns Room not Subscription, so DMs + // arrive with empty name and no subscriptionName until a subscription.update + // event lands. Show a placeholder rather than the empty string so the + // sidebar row remains clickable + identifiable. + expect(roomDisplayName({ name: '', type: 'dm', id: 'r-dm' })).toBe('(DM)') + expect(roomDisplayName({ name: '', type: 'botDM', id: 'r-bot' })).toBe('(DM)') + }) + + it('falls back to room.id for an unnamed non-DM room (defensive)', () => { + expect(roomDisplayName({ name: '', type: 'channel', id: 'r-orphan' })).toBe('r-orphan') + }) +}) + +describe('roomFromSearchHit', () => { + it('maps search-hit field names onto the room shape', () => { + const hit = { roomId: 'r1', roomName: 'frontend', roomType: 'channel', siteId: 'site-A' } + expect(roomFromSearchHit(hit)).toEqual({ + id: 'r1', + name: 'frontend', + type: 'channel', + siteId: 'site-A', + }) + }) +}) diff --git a/chat-frontend/src/lib/subjects.js b/chat-frontend/src/lib/subjects.js index f54caa738..cc45167db 100644 --- a/chat-frontend/src/lib/subjects.js +++ b/chat-frontend/src/lib/subjects.js @@ -25,8 +25,11 @@ export function roomsGet(account, roomId) { return `chat.user.${account}.request.rooms.get.${roomId}` } -export function roomsCreate(account) { - return `chat.user.${account}.request.rooms.create` +// roomCreate is the room-service create subject. The site segment is the +// requester's site — room-service queue-subscribes on its own siteID, so a +// caller from site-A always lands its create on the site-A room-service. +export function roomCreate(account, siteId) { + return `chat.user.${account}.request.room.${siteId}.create` } export function subscriptionUpdate(account) { @@ -57,6 +60,17 @@ export function readReceipt(account, roomId, siteId) { return `chat.user.${account}.request.room.${roomId}.${siteId}.message.read-receipt` } +export function memberList(account, roomId, siteId) { + return `chat.user.${account}.request.room.${roomId}.${siteId}.member.list` +} + +// userResponse is where room-worker publishes AsyncJobResult after finishing +// a deferred operation. The client subscribes here before publishing the +// request and X-Request-ID header so it can match the result back. +export function userResponse(account, requestId) { + return `chat.user.${account}.response.${requestId}` +} + export function searchRooms(account) { return `chat.user.${account}.request.search.rooms` } @@ -64,3 +78,11 @@ export function searchRooms(account) { export function searchMessages(account) { return `chat.user.${account}.request.search.messages` } + +// orgMembers requests the enriched member list of a single org (sect). +// Used by MemberRoster to expand an org row into its individual members. +// Response shape: { members: [{ id, account, engName, chineseName, siteId }] }. +// Mirrors pkg/subject/subject.go::OrgMembers. +export function orgMembers(account, orgId) { + return `chat.user.${account}.request.orgs.${orgId}.members` +} diff --git a/chat-frontend/src/lib/subjects.test.js b/chat-frontend/src/lib/subjects.test.js index 218337205..15588b0cf 100644 --- a/chat-frontend/src/lib/subjects.test.js +++ b/chat-frontend/src/lib/subjects.test.js @@ -5,10 +5,14 @@ import { memberAdd, memberRemove, memberRoleUpdate, + memberList, searchRooms, searchMessages, msgSurrounding, readReceipt, + roomCreate, + userResponse, + orgMembers, } from './subjects' describe('subjects', () => { @@ -57,4 +61,22 @@ describe('subjects', () => { 'chat.user.alice.request.room.room1.site1.message.read-receipt' ) }) + + it('roomCreate builds the create-room request subject scoped to the requester site', () => { + expect(roomCreate('alice', 'site-A')).toBe('chat.user.alice.request.room.site-A.create') + }) + + it('memberList builds the list-members request subject', () => { + expect(memberList('alice', 'r1', 'site-A')).toBe( + 'chat.user.alice.request.room.r1.site-A.member.list' + ) + }) + + it('userResponse builds the per-request async-result subject', () => { + expect(userResponse('alice', 'req-123')).toBe('chat.user.alice.response.req-123') + }) + + it('orgMembers builds the list-org-members request subject', () => { + expect(orgMembers('alice', 'sect-eng')).toBe('chat.user.alice.request.orgs.sect-eng.members') + }) }) diff --git a/chat-frontend/src/lib/useDebouncedSearch.js b/chat-frontend/src/lib/useDebouncedSearch.js index d310a03d5..1d79eef3f 100644 --- a/chat-frontend/src/lib/useDebouncedSearch.js +++ b/chat-frontend/src/lib/useDebouncedSearch.js @@ -1,40 +1,58 @@ import { useState, useRef, useEffect } from 'react' +/** + * Controlled debounced-search hook. + * + * @param {Object} opts + * @param {number} [opts.delay=250] Debounce delay in ms. + * @param {number} [opts.minLen=2] Minimum query length before fetcher is invoked. + * @param {Function} [opts.fetcher] `(q) => Promise`. Optional — when + * omitted/null, the hook is a controlled input + * tracker only: `query` updates, but no timer is + * scheduled and `results` stays `[]`. Use this + * mode for chip-input fields that don't have a + * server-side search endpoint yet. + */ export function useDebouncedSearch({ delay = 250, minLen = 2, fetcher }) { const [query, setQuery] = useState('') const [results, setResults] = useState([]) - const [loading, setLoading] = useState(false) const debounceRef = useRef(null) + // Sequence guard: each scheduled fetch captures the seq counter; an + // older fetch resolving after a newer one (or after reset) is dropped + // so suggestions don't flicker to a stale list. + const seqRef = useRef(0) useEffect(() => () => clearTimeout(debounceRef.current), []) const onChange = (q) => { + const seq = ++seqRef.current setQuery(q) clearTimeout(debounceRef.current) - if (q.length < minLen) { - setResults([]) - setLoading(false) + if (q.length < minLen || !fetcher) { + // Avoid no-op renders: `setState` to the same primitive bails out, but + // `setResults([])` allocates a fresh array every call and would otherwise + // re-render once per keystroke on fields with no fetcher. + setResults((prev) => (prev.length === 0 ? prev : [])) return } debounceRef.current = setTimeout(async () => { - setLoading(true) try { const resp = await fetcher(q) + if (seq !== seqRef.current) return setResults(resp ?? []) } catch { + if (seq !== seqRef.current) return setResults([]) - } finally { - setLoading(false) } }, delay) } const reset = () => { - setQuery('') - setResults([]) - setLoading(false) + seqRef.current += 1 + setQuery((prev) => (prev === '' ? prev : '')) + setResults((prev) => (prev.length === 0 ? prev : [])) clearTimeout(debounceRef.current) } - return { query, results, loading, onChange, reset } + return { query, results, onChange, reset } } diff --git a/chat-frontend/src/lib/useDebouncedSearch.test.js b/chat-frontend/src/lib/useDebouncedSearch.test.js new file mode 100644 index 000000000..85b99d0dd --- /dev/null +++ b/chat-frontend/src/lib/useDebouncedSearch.test.js @@ -0,0 +1,50 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest' +import { renderHook, act } from '@testing-library/react' +import { useDebouncedSearch } from './useDebouncedSearch' + +describe('useDebouncedSearch', () => { + beforeEach(() => { + vi.useFakeTimers({ shouldAdvanceTime: true }) + }) + afterEach(() => { + vi.useRealTimers() + }) + + it('debounces and calls the fetcher after delay when q ≥ minLen', async () => { + const fetcher = vi.fn().mockResolvedValue([{ id: 1 }]) + const { result } = renderHook(() => + useDebouncedSearch({ delay: 250, minLen: 2, fetcher }) + ) + act(() => { result.current.onChange('ab') }) + expect(fetcher).not.toHaveBeenCalled() + await act(async () => { vi.advanceTimersByTime(250) }) + expect(fetcher).toHaveBeenCalledWith('ab') + }) + + it('does not schedule any timer when fetcher is null, even with long input', async () => { + const setTimeoutSpy = vi.spyOn(global, 'setTimeout') + const { result } = renderHook(() => + useDebouncedSearch({ delay: 250, minLen: 2, fetcher: null }) + ) + setTimeoutSpy.mockClear() + act(() => { result.current.onChange('long-query') }) + // No debounce timer should have been scheduled. + const debounceCall = setTimeoutSpy.mock.calls.find(([, d]) => d === 250) + expect(debounceCall).toBeUndefined() + expect(result.current.results).toEqual([]) + expect(result.current.query).toBe('long-query') + setTimeoutSpy.mockRestore() + }) + + it('resets state and clears any pending timer', async () => { + const fetcher = vi.fn().mockResolvedValue([{ id: 1 }]) + const { result } = renderHook(() => + useDebouncedSearch({ delay: 250, minLen: 2, fetcher }) + ) + act(() => { result.current.onChange('ab') }) + act(() => { result.current.reset() }) + await act(async () => { vi.advanceTimersByTime(500) }) + expect(fetcher).not.toHaveBeenCalled() + expect(result.current.query).toBe('') + }) +}) diff --git a/chat-frontend/src/pages/ChatPage.jsx b/chat-frontend/src/pages/ChatPage.jsx index 1ff3ad616..d72d43abc 100644 --- a/chat-frontend/src/pages/ChatPage.jsx +++ b/chat-frontend/src/pages/ChatPage.jsx @@ -6,7 +6,6 @@ import MessageArea from '../components/MessageArea' import MessageInput from '../components/MessageInput' import CreateRoomDialog from '../components/CreateRoomDialog' import ManageMembersDialog from '../components/ManageMembersDialog' -import LeaveRoomButton from '../components/LeaveRoomButton' import SearchBar from '../components/SearchBar' import SearchResultsPane from './SearchResultsPane' import InRoomSearch from '../components/InRoomSearch' @@ -18,6 +17,11 @@ export default function ChatPage() { const [selectedRoom, setSelectedRoom] = useState(null) const [showCreateRoom, setShowCreateRoom] = useState(false) const [showMembers, setShowMembers] = useState(false) + // Bumped each time ManageMembersDialog closes so RoomMembersBadge refetches + // its count — a member-add/remove inside the dialog should be reflected in + // the badge immediately when the dialog dismisses, without waiting for the + // next room switch. + const [membersRefreshKey, setMembersRefreshKey] = useState(0) const [searchQuery, setSearchQuery] = useState(null) const [inRoomSearchOpen, setInRoomSearchOpen] = useState(false) @@ -91,18 +95,6 @@ export default function ChatPage() { }} />
    - {isChannel && ( - <> - - - - )} {user?.account} · {user?.siteId} @@ -135,7 +127,11 @@ export default function ChatPage() { ) : (
    - + setShowMembers(true)} + membersRefreshKey={membersRefreshKey} + />
    {inRoomSearchOpen && selectedRoom && ( @@ -158,7 +154,10 @@ export default function ChatPage() { {showMembers && selectedRoom && ( setShowMembers(false)} + onClose={() => { + setShowMembers(false) + setMembersRefreshKey((k) => k + 1) + }} /> )}
    diff --git a/chat-frontend/src/pages/ChatPage.test.jsx b/chat-frontend/src/pages/ChatPage.test.jsx index 09f5ed178..9958fccdb 100644 --- a/chat-frontend/src/pages/ChatPage.test.jsx +++ b/chat-frontend/src/pages/ChatPage.test.jsx @@ -21,7 +21,19 @@ vi.mock('../components/RoomList', () => ({
    ), })) -vi.mock('../components/MessageArea', () => ({ default: () =>
    })) +// MessageArea now owns the per-room "N members" badge in its header — the +// mock surfaces a clickable members button when a channel is passed in so +// ChatPage's wiring tests (open dialog on click, refresh after close) keep +// working without rendering the real MessageArea + its NATS deps. +vi.mock('../components/MessageArea', () => ({ + default: ({ room, onOpenMembers }) => ( +
    + {room?.type === 'channel' && ( + + )} +
    + ), +})) vi.mock('../components/MessageInput', () => ({ default: () =>
    })) vi.mock('../components/CreateRoomDialog', () => ({ default: () => null })) vi.mock('../components/SearchBar', () => ({ @@ -50,7 +62,6 @@ vi.mock('../components/InRoomSearch', () => ({ vi.mock('../components/ThemeToggle', () => ({ default: () =>