Skip to content

Commit 25c4314

Browse files
authored
feat(userlist): click a user to open chat with @<name> prefilled (#7660)
* feat(userlist): click a user to open chat with @<name> prefilled Newcomers to a multi-user pad regularly fail to discover the chat panel and the @-mention convention. Make the user list itself the discovery affordance: clicking another user's row opens chat (if hidden) and prefills the input with "@<their_name> ", ready to send. The skin gets a small visual cue — pointer cursor on .usertdname and an underline on hover — so the affordance is visible without requiring a redesign. The color swatch keeps its own click semantics (color picker), so the swatch cell is excluded from the new handler. To let bot/AI plugins substitute their trigger string for an otherwise-useless @-mention of the bot's display name (e.g. "@ai Assistant" → "@ai"), this adds a new client-side hook, chatPrefillFromUser, that takes {authorId, name, prefill} and lets the first plugin to return a non-empty string override the default prefill. Documented in doc/api/hooks_client-side.md alongside chatSendMessage. Plugin errors in the hook are caught — a misbehaving plugin can't break the click. If chat is hidden by pad settings, chat.show() is a no-op and the click effectively does nothing, which matches the existing behavior of "no chat means no chat-related affordances". The new prefill never clobbers a real partial message in the input; if the user was mid-typing something, the @-mention is appended rather than replacing. * fix(userlist): don't steal rename focus + add Playwright coverage Two follow-ups on review of the click-to-chat handler: 1. Bug (Qodo, correctness): clicking the rename <input> on an unnamed user's row triggered the new row handler, which then focused #chatinput and made it impossible to name unnamed users from the user list. Add an early-return that skips form controls inside the row (input/textarea/select/button/a/[contenteditable=true]). The swatch was already excluded; this widens the same idea to anything that's interactive on its own merits. 2. Test coverage: add a frontend Playwright spec (userlist_click_to_chat.spec.ts) covering the supported flows and the new regression: - clicking another named user opens chat and prefills "@<name> " - clicking the swatch opens color picker, not chat - clicking the rename <input> on an unnamed user keeps focus on the input (regression test for the bug above) - partial chat message is preserved when prefilling * test: stabilise the partial-message preservation case The 'partial message in chat input is preserved when prefilling' case was flaking on CI. Three small changes: - Seed the chat input with fill() rather than click() + keyboard.type(). Earlier the test was racing chat.focus()'s own setTimeout(100) — when the keyboard.type started before that timer fired, the typing landed in whatever element had focus at the time, which wasn't always the chat input. fill() bypasses focus state entirely. - Wait for the chat box to be visible before filling, so we don't race the chaticon click handler. - Replace the two sequential expect/wait pairs after the daveRow click with one waitForFunction that asserts both 'hi there' and '@dave' are in the input together. The prefill is async (setTimeout(50) inside the click handler), so a combined wait is more reliable than checking one piece, then snapshotting and asserting the other. The other three cases in this file passed unchanged on CI; only this fourth one was racy. * fix: don't commit local .claude worktrees / var state These were accidentally added in ffe9477 by an over-broad git add -A. Both paths are workspace-local and unrelated to this PR.
1 parent e0a9890 commit 25c4314

4 files changed

Lines changed: 262 additions & 0 deletions

File tree

doc/api/hooks_client-side.md

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -354,6 +354,34 @@ Context properties:
354354

355355
* `message`: The message object that will be sent to the Etherpad server.
356356

357+
## `chatPrefillFromUser`
358+
359+
Called from: `src/static/js/pad_userlist.ts`
360+
361+
Called when the user clicks an entry in the user list. The default behavior is to
362+
open the chat panel and prefill the input with `@<name> `, where `<name>` is that
363+
user's display name (with whitespace replaced by underscores). Plugins can return
364+
a different prefill string from their callback — the first non-empty string
365+
returned wins.
366+
367+
Typical use is by AI/bot plugins whose author display name (e.g. "AI Assistant")
368+
isn't a useful @-mention; the plugin can substitute its trigger string instead.
369+
370+
Context properties:
371+
372+
* `authorId`: The clicked user's author id.
373+
* `name`: The clicked user's display name.
374+
* `prefill`: The default prefill string Etherpad would otherwise use.
375+
376+
Example:
377+
378+
```javascript
379+
exports.chatPrefillFromUser = (hookName, {authorId, name}, cb) => {
380+
if (authorId === window.clientVars.ep_my_bot.authorId) return cb('@bot ');
381+
return cb();
382+
};
383+
```
384+
357385
## collectContentPre
358386

359387
Called from: `src/static/js/contentcollector.js`

src/static/js/pad_userlist.ts

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919

2020
import padutils from './pad_utils'
2121
const hooks = require('./pluginfw/hooks');
22+
const chat = require('./chat').chat;
2223
import html10n from './vendors/html10n';
2324
let myUserInfo = {};
2425

@@ -369,6 +370,51 @@ const paduserlist = (() => {
369370
}, 0);
370371
});
371372

373+
// Click any other user's row to open chat with @<their_name> prefilled.
374+
// Helps newcomers discover the chat panel and the @-mention convention
375+
// without having to be told. Plugins can transform the prefilled text
376+
// — for example ep_ai_chat replaces "@AI Assistant" with the
377+
// configured trigger ("@ai") — via the chatPrefillFromUser client
378+
// hook (see doc/api/hooks_client-side.md).
379+
$('#otheruserstable').on('click', 'tr[data-authorId]', async function (event) {
380+
// Skip clicks on the color swatch — that has its own click handler
381+
// (color-picker semantics) and shouldn't double up as a chat trigger.
382+
if ($(event.target).closest('.usertdswatch').length) return;
383+
// Skip clicks on form controls inside the row. The most important
384+
// case is the rename <input> rendered for unnamed users — without
385+
// this guard, clicking the input would steal focus into #chatinput
386+
// and make it impossible to name an unnamed user from the list.
387+
if ($(event.target).closest('input, textarea, select, button, a, [contenteditable=true]').length) return;
388+
const tr = $(this);
389+
const authorId = tr.attr('data-authorId');
390+
if (!authorId) return;
391+
const name = (tr.find('.usertdname').text() || '').trim();
392+
let prefill = name ? `@${name.replace(/\s+/g, '_')} ` : '';
393+
try {
394+
const transforms = await hooks.aCallAll(
395+
'chatPrefillFromUser', {authorId, name, prefill});
396+
if (Array.isArray(transforms)) {
397+
for (const tr2 of transforms) {
398+
if (typeof tr2 === 'string' && tr2.length > 0) { prefill = tr2; break; }
399+
}
400+
}
401+
} catch { /* never let a misbehaving plugin break the click */ }
402+
try { chat.show(); } catch { /* */ }
403+
setTimeout(() => {
404+
const $input = $('#chatinput');
405+
if (!$input.length) return;
406+
const current = ($input.val() || '') as string;
407+
if (!current.trim() || /^@\S+\s*$/.test(current.trim())) {
408+
$input.val(prefill);
409+
} else if (!current.includes(prefill.trim())) {
410+
$input.val(`${current.trimEnd()} ${prefill}`);
411+
}
412+
$input.trigger('focus');
413+
const elem = $input[0] as HTMLTextAreaElement;
414+
try { elem.setSelectionRange(elem.value.length, elem.value.length); } catch (_e) { /* */ }
415+
}, 50);
416+
});
417+
372418
// color picker
373419
$('#myswatchbox').on('click', showColorPicker);
374420
$('#mycolorpicker .pickerswatchouter').on('click', function () {

src/static/skins/colibris/src/components/users.css

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,19 @@ table#otheruserstable {
22
margin-top: 20px;
33
}
44

5+
/*
6+
* Clicking a row in the user list opens chat and prefills "@<name> ".
7+
* The pointer cursor on the name cell makes the affordance visible —
8+
* the swatch keeps its own (color-picker) click semantics, so leave
9+
* its cursor alone.
10+
*/
11+
#otheruserstable tr[data-authorId] .usertdname {
12+
cursor: pointer;
13+
}
14+
#otheruserstable tr[data-authorId]:hover .usertdname {
15+
text-decoration: underline;
16+
}
17+
518
.popup#users.chatAndUsers > .popup-content {
619
padding: 20px 10px;
720
height: 250px;
Lines changed: 175 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,175 @@
1+
import {expect, test} from '@playwright/test';
2+
import {
3+
goToNewPad,
4+
goToPad,
5+
isChatBoxShown,
6+
setUserName,
7+
toggleUserList,
8+
} from '../helper/padHelper';
9+
10+
/**
11+
* Coverage for the click-a-user-to-prefill-@-mention UX added in #7660.
12+
*
13+
* Why a multi-context suite: the row click handler only runs against
14+
* #otheruserstable rows, so we always need a second client connected to
15+
* the same pad to populate that table. Each test opens the pad twice
16+
* with a fresh context, names the second user, then drives the click
17+
* from the first.
18+
*/
19+
20+
const setSecondUserName = async (page2: any, name: string) => {
21+
await toggleUserList(page2);
22+
await setUserName(page2, name);
23+
await page2.keyboard.press('Enter');
24+
};
25+
26+
test.describe('userlist click → chat prefill', {tag: '@feature:chat'}, () => {
27+
test('clicking another user opens chat and prefills @<name>',
28+
async ({browser}) => {
29+
const padId = await goToNewPad(await browser.newPage());
30+
// Hack: the line above used a throwaway page just to mint a padId.
31+
// Real users come below.
32+
33+
const ctx1 = await browser.newContext();
34+
const page1 = await ctx1.newPage();
35+
await goToPad(page1, padId);
36+
37+
const ctx2 = await browser.newContext();
38+
const page2 = await ctx2.newPage();
39+
await goToPad(page2, padId);
40+
41+
await setSecondUserName(page2, 'Alice');
42+
43+
// Wait for page1's user list to learn about Alice.
44+
await toggleUserList(page1);
45+
const aliceRow = page1.locator(
46+
'#otheruserstable tr[data-authorId] .usertdname:has-text("Alice")');
47+
await expect(aliceRow).toBeVisible({timeout: 10_000});
48+
49+
// Sanity: chat is hidden before the click.
50+
expect(await isChatBoxShown(page1)).toBe(false);
51+
52+
await aliceRow.click();
53+
54+
// Chat should be open, input prefilled.
55+
await page1.waitForFunction(
56+
"document.querySelector('#chatbox')?.classList.contains('visible')",
57+
null, {timeout: 5_000});
58+
await page1.waitForFunction(
59+
"document.querySelector('#chatinput')?.value?.startsWith('@Alice ')",
60+
null, {timeout: 5_000});
61+
62+
await ctx1.close();
63+
await ctx2.close();
64+
});
65+
66+
test('clicking the swatch opens the color picker, not chat',
67+
async ({browser}) => {
68+
const padId = await goToNewPad(await browser.newPage());
69+
70+
const ctx1 = await browser.newContext();
71+
const page1 = await ctx1.newPage();
72+
await goToPad(page1, padId);
73+
74+
const ctx2 = await browser.newContext();
75+
const page2 = await ctx2.newPage();
76+
await goToPad(page2, padId);
77+
await setSecondUserName(page2, 'Bob');
78+
79+
await toggleUserList(page1);
80+
const bobRow = page1.locator(
81+
'#otheruserstable tr[data-authorId] .usertdname:has-text("Bob")');
82+
await expect(bobRow).toBeVisible({timeout: 10_000});
83+
84+
const swatch = page1.locator(
85+
'#otheruserstable tr[data-authorId] .usertdswatch').first();
86+
await swatch.click();
87+
88+
// Chat should NOT be opened by a swatch click.
89+
// (We only check the box-state; we don't assert anything about
90+
// any color-picker popup since this PR doesn't change that flow.)
91+
await page1.waitForTimeout(300);
92+
expect(await isChatBoxShown(page1)).toBe(false);
93+
94+
await ctx1.close();
95+
await ctx2.close();
96+
});
97+
98+
test('clicking the rename input on an unnamed user does not steal focus',
99+
async ({browser}) => {
100+
const padId = await goToNewPad(await browser.newPage());
101+
102+
const ctx1 = await browser.newContext();
103+
const page1 = await ctx1.newPage();
104+
await goToPad(page1, padId);
105+
106+
// Second user joins but never sets a name → row renders an
107+
// <input data-l10n-id="pad.userlist.unnamed">.
108+
const ctx2 = await browser.newContext();
109+
const page2 = await ctx2.newPage();
110+
await goToPad(page2, padId);
111+
112+
await toggleUserList(page1);
113+
const unnamedInput = page1.locator(
114+
'#otheruserstable input[data-l10n-id="pad.userlist.unnamed"]')
115+
.first();
116+
await expect(unnamedInput).toBeVisible({timeout: 10_000});
117+
118+
// The act of clicking the input must NOT trigger the row handler.
119+
// Pre-fix, this opened chat and stole focus from the rename input.
120+
await unnamedInput.click();
121+
await page1.waitForTimeout(300);
122+
123+
expect(await isChatBoxShown(page1)).toBe(false);
124+
// Focus is still on the unnamed-user input — typing reaches it,
125+
// not #chatinput.
126+
await page1.keyboard.type('Carol');
127+
const value = await unnamedInput.inputValue();
128+
expect(value).toBe('Carol');
129+
130+
await ctx1.close();
131+
await ctx2.close();
132+
});
133+
134+
test('partial message in chat input is preserved when prefilling',
135+
async ({browser}) => {
136+
const padId = await goToNewPad(await browser.newPage());
137+
138+
const ctx1 = await browser.newContext();
139+
const page1 = await ctx1.newPage();
140+
await goToPad(page1, padId);
141+
142+
const ctx2 = await browser.newContext();
143+
const page2 = await ctx2.newPage();
144+
await goToPad(page2, padId);
145+
await setSecondUserName(page2, 'Dave');
146+
147+
// Open chat and seed a partial message *before* opening the user
148+
// list. fill() is deterministic — it sets the value without
149+
// racing the chaticon click handler's own focus timer that
150+
// earlier versions of this test were tripping over.
151+
await page1.locator('#chaticon').click();
152+
await page1.waitForFunction(
153+
"document.querySelector('#chatbox')?.classList.contains('visible')",
154+
null, {timeout: 5_000});
155+
await page1.locator('#chatinput').fill('hi there');
156+
157+
await toggleUserList(page1);
158+
const daveRow = page1.locator(
159+
'#otheruserstable tr[data-authorId] .usertdname:has-text("Dave")');
160+
await expect(daveRow).toBeVisible({timeout: 10_000});
161+
await daveRow.click();
162+
163+
// Wait for both pieces to land in the input — the prefill fires
164+
// from a 50ms setTimeout in the click handler, so a single wait
165+
// covering the full final state is more reliable than two
166+
// sequential checks.
167+
await page1.waitForFunction(() => {
168+
const v = (document.querySelector('#chatinput') as HTMLTextAreaElement)?.value || '';
169+
return v.includes('hi there') && v.includes('@Dave');
170+
}, null, {timeout: 5_000});
171+
172+
await ctx1.close();
173+
await ctx2.close();
174+
});
175+
});

0 commit comments

Comments
 (0)