-
-
Notifications
You must be signed in to change notification settings - Fork 3k
feat(userlist): click a user to open chat with @<name> prefilled #7660
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
504b05b
a494307
ffe9477
f82a615
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,7 @@ | |
|
|
||
| import padutils from './pad_utils' | ||
| const hooks = require('./pluginfw/hooks'); | ||
| const chat = require('./chat').chat; | ||
| import html10n from './vendors/html10n'; | ||
| let myUserInfo = {}; | ||
|
|
||
|
|
@@ -369,6 +370,51 @@ const paduserlist = (() => { | |
| }, 0); | ||
| }); | ||
|
|
||
| // Click any other user's row to open chat with @<their_name> prefilled. | ||
| // Helps newcomers discover the chat panel and the @-mention convention | ||
| // without having to be told. Plugins can transform the prefilled text | ||
| // — for example ep_ai_chat replaces "@AI Assistant" with the | ||
| // configured trigger ("@ai") — via the chatPrefillFromUser client | ||
| // hook (see doc/api/hooks_client-side.md). | ||
| $('#otheruserstable').on('click', 'tr[data-authorId]', async function (event) { | ||
| // Skip clicks on the color swatch — that has its own click handler | ||
| // (color-picker semantics) and shouldn't double up as a chat trigger. | ||
| if ($(event.target).closest('.usertdswatch').length) return; | ||
| // Skip clicks on form controls inside the row. The most important | ||
| // case is the rename <input> rendered for unnamed users — without | ||
| // this guard, clicking the input would steal focus into #chatinput | ||
| // and make it impossible to name an unnamed user from the list. | ||
| if ($(event.target).closest('input, textarea, select, button, a, [contenteditable=true]').length) return; | ||
| const tr = $(this); | ||
| const authorId = tr.attr('data-authorId'); | ||
| if (!authorId) return; | ||
| const name = (tr.find('.usertdname').text() || '').trim(); | ||
| let prefill = name ? `@${name.replace(/\s+/g, '_')} ` : ''; | ||
| try { | ||
| const transforms = await hooks.aCallAll( | ||
| 'chatPrefillFromUser', {authorId, name, prefill}); | ||
| if (Array.isArray(transforms)) { | ||
| for (const tr2 of transforms) { | ||
| if (typeof tr2 === 'string' && tr2.length > 0) { prefill = tr2; break; } | ||
| } | ||
| } | ||
| } catch { /* never let a misbehaving plugin break the click */ } | ||
| try { chat.show(); } catch { /* */ } | ||
| setTimeout(() => { | ||
| const $input = $('#chatinput'); | ||
| if (!$input.length) return; | ||
| const current = ($input.val() || '') as string; | ||
| if (!current.trim() || /^@\S+\s*$/.test(current.trim())) { | ||
| $input.val(prefill); | ||
| } else if (!current.includes(prefill.trim())) { | ||
| $input.val(`${current.trimEnd()} ${prefill}`); | ||
| } | ||
| $input.trigger('focus'); | ||
| const elem = $input[0] as HTMLTextAreaElement; | ||
| try { elem.setSelectionRange(elem.value.length, elem.value.length); } catch (_e) { /* */ } | ||
| }, 50); | ||
| }); | ||
|
Comment on lines
+379
to
+416
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 1. Click-to-chat lacks feature flag The new click handler on user list rows changes default behavior by opening chat and prefilling an @name mention without any feature flag gating. This violates the requirement that new features be disabled by default unless explicitly enabled. Agent Prompt
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Going with a reasoned decline on the feature-flag part of this comment. Replying so the rationale is on the record:
The other comment on this review (rename-input click steals focus) was a real bug and is fixed in |
||
|
|
||
| // color picker | ||
| $('#myswatchbox').on('click', showColorPicker); | ||
| $('#mycolorpicker .pickerswatchouter').on('click', function () { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,175 @@ | ||
| import {expect, test} from '@playwright/test'; | ||
| import { | ||
| goToNewPad, | ||
| goToPad, | ||
| isChatBoxShown, | ||
| setUserName, | ||
| toggleUserList, | ||
| } from '../helper/padHelper'; | ||
|
|
||
| /** | ||
| * Coverage for the click-a-user-to-prefill-@-mention UX added in #7660. | ||
| * | ||
| * Why a multi-context suite: the row click handler only runs against | ||
| * #otheruserstable rows, so we always need a second client connected to | ||
| * the same pad to populate that table. Each test opens the pad twice | ||
| * with a fresh context, names the second user, then drives the click | ||
| * from the first. | ||
| */ | ||
|
|
||
| const setSecondUserName = async (page2: any, name: string) => { | ||
| await toggleUserList(page2); | ||
| await setUserName(page2, name); | ||
| await page2.keyboard.press('Enter'); | ||
| }; | ||
|
|
||
| test.describe('userlist click → chat prefill', {tag: '@feature:chat'}, () => { | ||
| test('clicking another user opens chat and prefills @<name>', | ||
| async ({browser}) => { | ||
| const padId = await goToNewPad(await browser.newPage()); | ||
| // Hack: the line above used a throwaway page just to mint a padId. | ||
| // Real users come below. | ||
|
|
||
| const ctx1 = await browser.newContext(); | ||
| const page1 = await ctx1.newPage(); | ||
| await goToPad(page1, padId); | ||
|
|
||
| const ctx2 = await browser.newContext(); | ||
| const page2 = await ctx2.newPage(); | ||
| await goToPad(page2, padId); | ||
|
|
||
| await setSecondUserName(page2, 'Alice'); | ||
|
|
||
| // Wait for page1's user list to learn about Alice. | ||
| await toggleUserList(page1); | ||
| const aliceRow = page1.locator( | ||
| '#otheruserstable tr[data-authorId] .usertdname:has-text("Alice")'); | ||
| await expect(aliceRow).toBeVisible({timeout: 10_000}); | ||
|
|
||
| // Sanity: chat is hidden before the click. | ||
| expect(await isChatBoxShown(page1)).toBe(false); | ||
|
|
||
| await aliceRow.click(); | ||
|
|
||
| // Chat should be open, input prefilled. | ||
| await page1.waitForFunction( | ||
| "document.querySelector('#chatbox')?.classList.contains('visible')", | ||
| null, {timeout: 5_000}); | ||
| await page1.waitForFunction( | ||
| "document.querySelector('#chatinput')?.value?.startsWith('@Alice ')", | ||
| null, {timeout: 5_000}); | ||
|
|
||
| await ctx1.close(); | ||
| await ctx2.close(); | ||
| }); | ||
|
|
||
| test('clicking the swatch opens the color picker, not chat', | ||
| async ({browser}) => { | ||
| const padId = await goToNewPad(await browser.newPage()); | ||
|
|
||
| const ctx1 = await browser.newContext(); | ||
| const page1 = await ctx1.newPage(); | ||
| await goToPad(page1, padId); | ||
|
|
||
| const ctx2 = await browser.newContext(); | ||
| const page2 = await ctx2.newPage(); | ||
| await goToPad(page2, padId); | ||
| await setSecondUserName(page2, 'Bob'); | ||
|
|
||
| await toggleUserList(page1); | ||
| const bobRow = page1.locator( | ||
| '#otheruserstable tr[data-authorId] .usertdname:has-text("Bob")'); | ||
| await expect(bobRow).toBeVisible({timeout: 10_000}); | ||
|
|
||
| const swatch = page1.locator( | ||
| '#otheruserstable tr[data-authorId] .usertdswatch').first(); | ||
| await swatch.click(); | ||
|
|
||
| // Chat should NOT be opened by a swatch click. | ||
| // (We only check the box-state; we don't assert anything about | ||
| // any color-picker popup since this PR doesn't change that flow.) | ||
| await page1.waitForTimeout(300); | ||
| expect(await isChatBoxShown(page1)).toBe(false); | ||
|
|
||
| await ctx1.close(); | ||
| await ctx2.close(); | ||
| }); | ||
|
|
||
| test('clicking the rename input on an unnamed user does not steal focus', | ||
| async ({browser}) => { | ||
| const padId = await goToNewPad(await browser.newPage()); | ||
|
|
||
| const ctx1 = await browser.newContext(); | ||
| const page1 = await ctx1.newPage(); | ||
| await goToPad(page1, padId); | ||
|
|
||
| // Second user joins but never sets a name → row renders an | ||
| // <input data-l10n-id="pad.userlist.unnamed">. | ||
| const ctx2 = await browser.newContext(); | ||
| const page2 = await ctx2.newPage(); | ||
| await goToPad(page2, padId); | ||
|
|
||
| await toggleUserList(page1); | ||
| const unnamedInput = page1.locator( | ||
| '#otheruserstable input[data-l10n-id="pad.userlist.unnamed"]') | ||
| .first(); | ||
| await expect(unnamedInput).toBeVisible({timeout: 10_000}); | ||
|
|
||
| // The act of clicking the input must NOT trigger the row handler. | ||
| // Pre-fix, this opened chat and stole focus from the rename input. | ||
| await unnamedInput.click(); | ||
| await page1.waitForTimeout(300); | ||
|
|
||
| expect(await isChatBoxShown(page1)).toBe(false); | ||
| // Focus is still on the unnamed-user input — typing reaches it, | ||
| // not #chatinput. | ||
| await page1.keyboard.type('Carol'); | ||
| const value = await unnamedInput.inputValue(); | ||
| expect(value).toBe('Carol'); | ||
|
|
||
| await ctx1.close(); | ||
| await ctx2.close(); | ||
| }); | ||
|
|
||
| test('partial message in chat input is preserved when prefilling', | ||
| async ({browser}) => { | ||
| const padId = await goToNewPad(await browser.newPage()); | ||
|
|
||
| const ctx1 = await browser.newContext(); | ||
| const page1 = await ctx1.newPage(); | ||
| await goToPad(page1, padId); | ||
|
|
||
| const ctx2 = await browser.newContext(); | ||
| const page2 = await ctx2.newPage(); | ||
| await goToPad(page2, padId); | ||
| await setSecondUserName(page2, 'Dave'); | ||
|
|
||
| // Open chat and seed a partial message *before* opening the user | ||
| // list. fill() is deterministic — it sets the value without | ||
| // racing the chaticon click handler's own focus timer that | ||
| // earlier versions of this test were tripping over. | ||
| await page1.locator('#chaticon').click(); | ||
| await page1.waitForFunction( | ||
| "document.querySelector('#chatbox')?.classList.contains('visible')", | ||
| null, {timeout: 5_000}); | ||
| await page1.locator('#chatinput').fill('hi there'); | ||
|
|
||
| await toggleUserList(page1); | ||
| const daveRow = page1.locator( | ||
| '#otheruserstable tr[data-authorId] .usertdname:has-text("Dave")'); | ||
| await expect(daveRow).toBeVisible({timeout: 10_000}); | ||
| await daveRow.click(); | ||
|
|
||
| // Wait for both pieces to land in the input — the prefill fires | ||
| // from a 50ms setTimeout in the click handler, so a single wait | ||
| // covering the full final state is more reliable than two | ||
| // sequential checks. | ||
| await page1.waitForFunction(() => { | ||
| const v = (document.querySelector('#chatinput') as HTMLTextAreaElement)?.value || ''; | ||
| return v.includes('hi there') && v.includes('@Dave'); | ||
| }, null, {timeout: 5_000}); | ||
|
|
||
| await ctx1.close(); | ||
| await ctx2.close(); | ||
| }); | ||
| }); |
Uh oh!
There was an error while loading. Please reload this page.