diff --git a/apps/web/src/chat/chat.tsx b/apps/web/src/chat/chat.tsx index 11913c5d3..945df5645 100644 --- a/apps/web/src/chat/chat.tsx +++ b/apps/web/src/chat/chat.tsx @@ -10,14 +10,16 @@ import { useEffect, useId, useLayoutEffect, useReducer, useRef, useState } from "react"; import { + ChopinMark, CONNECTION_GRACE, MAX_RESEARCH_BRIEF, SendAction, useConnectionNotice, usePopoverDismissal, } from "@chopin/editor"; +import { usePhone } from "@chopin/editor/pointer"; import { MENTION } from "@chopin/protocol/address"; -import { ArchiveIcon, InfoIcon, LockIcon, PlusIcon, WarningIcon } from "@chopin/icons"; +import { ArchiveIcon, CloseIcon, InfoIcon, LockIcon, PlusIcon, WarningIcon } from "@chopin/icons"; import { DraftInput } from "./draft-input"; import type { DraftInputHandle } from "./draft-input"; import { ModeSwitch } from "./mode-switch"; @@ -240,6 +242,7 @@ export function Chat( let textarea = useRef(null); let composerRoot = useRef(null); let [mode, setMode] = useState(false); + let phone = usePhone(); let [historyKey, setHistoryKey] = useState(0); let pendingCaret = useRef(undefined); let submission = useRef(undefined); @@ -562,10 +565,15 @@ export function Chat( let end = selection.end; if (effectiveMode) { let removals = [...current.text.matchAll(/(^|[^\w@])@chopin\b/gi)] - .map(match => ({ - start: match.index + match[1]!.length, - end: match.index + match[0].length, - })) + .map(match => { + let start = match.index + match[1]!.length; + let end = match.index + match[0].length; + // Take one following space too where the mention began the text or followed one, + // so removing it leaves no doubled or leading space. + let before = current.text[start - 1]; + if (current.text[end] === " " && (start === 0 || before === " ")) end += 1; + return { start, end }; + }) .filter(edit => !current.references.some(reference => edit.start < reference.end && edit.end > reference.start @@ -798,6 +806,23 @@ export function Chat( ) : ( <> + {/* A phone's switch shows only an icon; the chip says where the message goes. */} + {phone && effectiveMode && ( + + + + )} void; + onComments?: () => void; onEditingChange: (editing?: TitleEdit) => void; onRenamed: (channel: DocumentMetadata) => void; presentation: WorkspacePresentation; @@ -123,6 +131,19 @@ export function PhoneHeader( )}
+ {comments > 0 && onComments && ( + + )} {canManage && ( item.thread.status === "accepted" && !item.applied).length; + // The threads a phone's list of comments can show: open, with a passage left to stand on. + let openComments = + threadState.threads.filter(item => + item.thread.status === "open" && !item.orphaned && !item.drifted + ).length; let statusNotices = ( <> @@ -1479,10 +1485,16 @@ export function RoomWorkspace( onDocumentAction(room, action)} + onComments={() => { + // The list stands over the document, so it opens from any tab onto it. + selectDestination("plan"); + openCommentList(room); + }} onEditingChange={setTitleEdit} onRenamed={updateMetadata} presentation={presentation} diff --git a/e2e/chat-phone.e2e.ts b/e2e/chat-phone.e2e.ts index 112310207..66d80eb65 100644 --- a/e2e/chat-phone.e2e.ts +++ b/e2e/chat-phone.e2e.ts @@ -218,3 +218,60 @@ test("the desktop composer keeps its labelled mode switch, footer and dropdown p ]); expect(list!.width).toBeLessThan(pill!.width / 2); }); + +test("a phone composer shows a removable Chopin chip while a message addresses Chopin", async ({ join }) => { + let page = await join("ana", PHONE); + // The Planner is off in this suite; the session says it is on, as the composer reads it. + await page.context().route("**/api/session", async route => { + let response = await route.fetch(); + await route.fulfill({ response, json: { ...await response.json(), agent: true } }); + }); + await page.reload(); + await openPhoneChat(page); + let chat = chatPane(page); + let draft = chatInput(chat); + let chip = chat.getByRole("button", { name: "Remove Chopin", exact: true }); + // Hidden while the chip shows, but its pressed state still says where the message goes. + let mode = chat.getByRole("button", { includeHidden: true, name: "Talk to Chopin", exact: true }); + await expect(chip).toHaveCount(0); + + // A mention addresses Chopin; the chip says so above the draft, inside the pill. + await draft.click(); + await page.keyboard.type("@chopin what is left? "); + await expect(chip).toBeVisible(); + await expect(mode).toHaveAttribute("aria-pressed", "true"); + // The chip stands in for the switch while it shows. + await expect(mode).toBeHidden(); + let [chipBox, draftBox, pill] = await Promise.all([ + chip.boundingBox(), + draft.boundingBox(), + chat.locator(".composer-surface").boundingBox(), + ]); + expect(chipBox!.y + chipBox!.height).toBeLessThanOrEqual(draftBox!.y + 1); + expect(chipBox!.y).toBeGreaterThanOrEqual(pill!.y); + // Drawn small, but a full touch target. + let reach = await chip.evaluate(element => { + let box = element.getBoundingClientRect(); + let x = box.left + box.width / 2; + let y = box.top + box.height / 2; + return [y - 21, y + 21].every(at => + document.elementFromPoint(x, at)?.closest("button") === element + ); + }); + expect(reach).toBe(true); + + // Removing the chip removes the address, as switching to Chat does. + await chip.tap(); + await expect(chip).toHaveCount(0); + await expect(mode).toHaveAttribute("aria-pressed", "false"); + // No leading space is left where the mention was. + expect(await draft.textContent()).toBe("what is left? "); + + // The switch turns it back on without a mention. + await expect(mode).toBeVisible(); + await mode.tap(); + await expect(chip).toBeVisible(); + await chip.tap(); + await expect(mode).toHaveAttribute("aria-pressed", "false"); + await expectNoHorizontalOverflow(page); +}); diff --git a/e2e/comment-sheet.e2e.ts b/e2e/comment-sheet.e2e.ts index 5fd35e49b..4e7956e59 100644 --- a/e2e/comment-sheet.e2e.ts +++ b/e2e/comment-sheet.e2e.ts @@ -584,3 +584,82 @@ test("a phone marker reaches 44px into the empty margin without covering the nex }, { chip, paragraph }); expect(hits).toEqual([true, true, false]); }); + +test("the phone top bar lists every open thread, leads into each one and back", async ({ join, seed }) => { + await seed(SOURCE, { + threads: [ + await open("review the pilot", ["Is two weeks enough?"], 1), + await open("starting with the docs team", ["Which team is next?", "Support."], 0), + ], + }); + let page = await join("ana", IPHONE); + let comments = page.locator(".phone-top-bar").getByRole("button", { name: "2 open comments" }); + let commentsBox = await size(comments); + expect(commentsBox.width).toBeGreaterThanOrEqual(44); + expect(commentsBox.height).toBeGreaterThanOrEqual(44); + // The title stays centred with a second button on the right. + let [documents, actions] = await Promise.all([ + size(page.locator(".phone-top-bar").getByRole("button", { name: "Documents", exact: true })), + size(page.locator(".phone-top-bar").getByRole("button", { name: /^Actions for/ })), + ]); + expect(documents.x).toBeLessThan(commentsBox.x); + expect(commentsBox.x + commentsBox.width).toBeLessThanOrEqual(actions.x + 1); + let title = await size(page.locator(".phone-top-bar-center")); + expect(Math.abs(title.x + title.width / 2 - 390 / 2)).toBeLessThanOrEqual(1); + + await comments.tap(); + let list = page.getByRole("dialog", { name: "Comments" }); + await settled(list); + await expect(list.getByText("2 comments", { exact: true })).toHaveCount(1); + // In reading order, each item saying which passage it is about. + let items = list.locator("[data-plan-comment-group-item]"); + await expect(items).toHaveCount(2); + await expect(items.nth(0)).toContainText("starting with the docs team"); + await expect(items.nth(0)).toContainText("Which team is next?"); + await expect(items.nth(0)).toContainText("1 reply"); + await expect(items.nth(1)).toContainText("review the pilot"); + // The list opens at the medium detent, leaving the document in view above it. + expect((await size(list)).y).toBeGreaterThan(844 * 0.3); + expect((await size(items.nth(1))).height).toBeGreaterThanOrEqual(44); + + await items.nth(1).click(); + let thread = page.getByRole("dialog", { name: "Comment thread" }); + await expect(thread).toContainText("Is two weeks enough?"); + let back = thread.getByRole("button", { name: "All 2 comments" }); + await expect(back).toBeFocused(); + // The passage stays in view beneath the back row. + await expect(thread.locator("[data-plan-comment-sheet-quote]")).toHaveText("review the pilot"); + await back.click(); + let again = page.getByRole("dialog", { name: "Comments" }); + await expect(again.getByRole("button", { name: /Is two weeks enough\?/ })).toBeFocused(); + + await again.getByRole("button", { name: "Close comment" }).click(); + await expect(again).toHaveCount(0); + await expect(comments).toBeFocused(); +}); + +test("a phone thread's position in the pager opens the list of every thread", async ({ join, seed }) => { + await seed(SOURCE, { + threads: [ + await open("starting with the docs team", ["Which team is next?"], 0), + await open("review the pilot", ["Is two weeks enough?"], 1), + ], + }); + let page = await join("ana", IPHONE); + await page.getByRole("button", { name: /^Comment on “review the pilot”/ }).tap(); + let sheet = page.getByRole("dialog", { name: "Comment thread" }); + let position = sheet.getByRole("button", { name: "2 of 2, show all comments" }); + await expect(position).toHaveText("2 of 2"); + expect((await size(position)).height).toBeGreaterThanOrEqual(44); + await position.click(); + let list = page.getByRole("dialog", { name: "Comments" }); + await expect(list.locator("[data-plan-comment-group-item]")).toHaveCount(2); + await expect(list.getByRole("button", { name: /Is two weeks enough\?/ })).toBeFocused(); +}); + +test("the desktop header has no comments list button", async ({ join, seed }) => { + await seed(SOURCE, { threads: [await open("review the pilot", ["Is two weeks enough?"])] }); + let page = await join("ana", { viewport: { width: 1440, height: 900 } }); + await expect(page.getByRole("button", { name: /^Comment on “review the pilot”/ })).toBeVisible(); + await expect(page.getByRole("button", { name: /open comments?$/ })).toHaveCount(0); +}); diff --git a/packages/editor/src/comment-layer.tsx b/packages/editor/src/comment-layer.tsx index 8c71195f8..bf4cbb472 100644 --- a/packages/editor/src/comment-layer.tsx +++ b/packages/editor/src/comment-layer.tsx @@ -3,7 +3,7 @@ import { useCallback, useEffect, useLayoutEffect, useMemo, useRef, useState } from "react"; import { createPortal } from "react-dom"; import { useLexicalComposerContext } from "@lexical/react/LexicalComposerContext"; -import { CheckIcon, MessageIcon } from "@chopin/icons"; +import { CheckIcon, CommentIcon, MessageIcon } from "@chopin/icons"; import { useCellValue } from "@mdxeditor/gurx"; import { $getNodeByKey } from "lexical"; @@ -22,7 +22,14 @@ import { useCommentSheetReveal } from "./comment-sheet-reveal"; import { $rangeOf } from "./marks"; import { blockElement } from "./scroll"; import { COARSE_POINTER_QUERY, PRIMARY_COARSE_POINTER_QUERY, usePhone } from "./pointer"; -import { handlesOpenComment, OPEN_COMMENT, openOutcome, openRequest, useThreads } from "./threads"; +import { + handlesOpenComment, + OPEN_COMMENT, + OPEN_COMMENTS, + openOutcome, + openRequest, + useThreads, +} from "./threads"; import type { OpenRequest } from "./threads"; import { useTransitionPresence } from "./transition-presence"; import { widgets$ } from "./widget-options"; @@ -72,6 +79,8 @@ type MeasuredThread = { /** A pinned list of one line's threads, as opposed to a pinned thread id. */ const LIST = "list:"; +/** A phone sheet listing every open thread in the document, asked for from outside it. */ +const ALL = "all"; /** How long a resolve can be undone from its toast, while nobody is pointing at it. */ const UNDO_WINDOW = 6_000; @@ -377,6 +386,8 @@ export function CommentLayer({ store }: { store: ThreadStore }) { let [previewMeasurement, setPreviewMeasurement] = useState(); let [pinned, setPinned] = useState(); let [returnTo, setReturnTo] = useState(); + /** The thread shown was chosen from the list of every thread, so its header leads back there. */ + let [fromAll, setFromAll] = useState(false); let [notice, setNotice] = useState(); let [noticeHeld, setNoticeHeld] = useState(false); let [linked, setLinked] = useState(() => @@ -410,6 +421,8 @@ export function CommentLayer({ store }: { store: ThreadStore }) { // The comment sheet belongs to the phone class, portrait or landscape. let compact = options.commentPresentation === "sheet" && phone; let previousCompact = useRef(compact); + let compactRef = useRef(compact); + compactRef.current = compact; let immediately = options.motionImmediately?.() ?? false; let draft = canEdit ? state.draft : undefined; let measurePreview = useCallback((request: PreviewRequest, height: number) => { @@ -461,6 +474,12 @@ export function CommentLayer({ store }: { store: ThreadStore }) { dialog?.querySelector("[data-plan-comment-close], button")?.focus(); }, [compact, pinned]); + useEffect(() => { + if (!pinned || pinned === ALL || pinned === "orphans" || pinned.startsWith(LIST)) { + setFromAll(false); + } + }, [pinned]); + // A preview belongs to the thread it was raised for; another pinned thread clears it. useEffect(() => { if (pinned) setPreview(current => current === pinned ? current : undefined); @@ -623,7 +642,7 @@ export function CommentLayer({ store }: { store: ThreadStore }) { let openIds = pinned?.startsWith(LIST) ? markers.find(marker => `${LIST}${marker.key}` === pinned)?.views.map(view => view.thread.id) ?? [] - : pinned && pinned !== "orphans" + : pinned && pinned !== "orphans" && pinned !== ALL ? [pinned] : []; let openKey = openIds.join(" "); @@ -846,11 +865,23 @@ export function CommentLayer({ store }: { store: ThreadStore }) { waiting.current = undefined; settle(pending.request, true); }); + // Only a phone's sheet has a list of every thread; elsewhere each thread keeps its margin. + let list = (event: Event) => { + let detail = (event as CustomEvent<{ channel?: string } | undefined>).detail; + if (!compactRef.current || !handlesOpenComment(store.channel, detail?.channel)) return; + let active = document.activeElement; + origin.current = active instanceof HTMLElement ? active : undefined; + setReturnTo(undefined); + setPreview(undefined); + setPinned(ALL); + }; addEventListener("hashchange", follow); addEventListener(OPEN_COMMENT, open); + addEventListener(OPEN_COMMENTS, list); return () => { removeEventListener("hashchange", follow); removeEventListener(OPEN_COMMENT, open); + removeEventListener(OPEN_COMMENTS, list); arrived(); clearTimeout(waiting.current?.timer); waiting.current = undefined; @@ -879,6 +910,8 @@ export function CommentLayer({ store }: { store: ThreadStore }) { if (!pinned) return; let available = pinned === "orphans" ? state.threads.some(view => view.thread.status === "open" && view.orphaned) + : pinned === ALL + ? state.threads.some(view => view.thread.status === "open" && !view.orphaned && !view.drifted) : pinned.startsWith(LIST) ? markersRef.current.some(marker => `${LIST}${marker.key}` === pinned) : state.threads.some(view => @@ -917,7 +950,7 @@ export function CommentLayer({ store }: { store: ThreadStore }) { ?.focus(); }, [pinned]); - let sheetId = compact && pinned !== "orphans" ? pinned : undefined; + let sheetId = compact && pinned !== "orphans" && pinned !== ALL ? pinned : undefined; let sheetPassages = useMemo(() => { if (!sheetId) return undefined; let list = sheetId.startsWith(LIST) ? sheetId.slice(LIST.length) : undefined; @@ -979,7 +1012,26 @@ export function CommentLayer({ store }: { store: ThreadStore }) { selected.current = id; setPinned(id); }; + // The document's open threads in reading order, for the list of them all and to step through. + let order = [ + ...new Set( + [...placed].sort((a, b) => passageTop(a) - passageTop(b)).map(entry => entry.view.thread.id), + ), + ]; + let allViews = order.flatMap(id => { + let view = placed.find(entry => entry.view.thread.id === id)?.view; + return view ? [view] : []; + }); let backTo = (view: ThreadView) => { + if (fromAll && allViews.length > 1) { + return { + count: allViews.length, + go: () => { + setReturnTo(view.thread.id); + setPinned(ALL); + }, + }; + } let group = markerOf(view.thread.id); let back = group && group.views.length > 1 ? group : undefined; return back @@ -1092,8 +1144,10 @@ export function CommentLayer({ store }: { store: ThreadStore }) { }); compactClose = dismiss; compactContent = orphaned.map(view => card(view)); - } else if (compact && pinned && (pinnedView || pinnedList)) { - // One sheet serves a line's list and every thread, so moving between them does not reopen it. + } else if ( + compact && pinned && (pinnedView || pinnedList || (pinned === ALL && allViews.length > 0)) + ) { + // One sheet serves the lists and every thread, so moving between them does not reopen it. compactKey = "pinned"; compactId = dialogId(pinned); compactLabel = pinnedView ? "Comment thread" : "Comments"; @@ -1104,13 +1158,12 @@ export function CommentLayer({ store }: { store: ThreadStore }) { quote: pinnedView.quote, siblings: back?.count ?? 1, }) - : commentSheetHeading({ kind: "list", count: pinnedList!.views.length }); + : commentSheetHeading({ + kind: "list", + count: pinnedList ? pinnedList.views.length : allViews.length, + }); compactBack = back?.go; compactClose = dismiss; - // The document's open threads in reading order, so the sheet can step through them. - let order = [...placed] - .sort((a, b) => passageTop(a) - passageTop(b)) - .map(entry => entry.view.thread.id); let at = pinnedView ? order.indexOf(pinnedView.thread.id) : -1; // Paging moves the way back too: closing returns focus to the marker of the thread shown. let step = (by: number) => { @@ -1128,6 +1181,10 @@ export function CommentLayer({ store }: { store: ThreadStore }) { ? { count: order.length, index: at, + onList: () => { + setReturnTo(pinnedView!.thread.id); + setPinned(ALL); + }, onNext: at < order.length - 1 ? () => step(1) : undefined, onPrevious: at > 0 ? () => step(-1) : undefined, } @@ -1136,9 +1193,13 @@ export function CommentLayer({ store }: { store: ThreadStore }) { { + setFromAll(!pinnedList); + selectThread(id); + }} + quotes={!pinnedList} returnTo={returnTo} - views={pinnedList!.views} + views={pinnedList ? pinnedList.views : allViews} /> ); } @@ -1297,7 +1358,10 @@ export function CommentLayer({ store }: { store: ThreadStore }) { style={{ left: button.left - left, top: (height - CHIP) / 2, width: button.width }} > {(single || !button.slim || coarse) && ( -