Cap note thread dialog height (task #417)
User reported (in Arabic, with screenshot) that the note Replies dialog was
covering the entire page when a thread had multiple grouped conversations.
The owner/admin grouped-replies view (`GroupedReplies`) had a `max-h-72`
internal scroll, but the outer `DialogContent` had no overall height cap, so
on tall content the dialog grew to fill the viewport.
Changes
- artifacts/tx-os/src/pages/notes.tsx
- `DialogContent` for the thread dialog: add `max-h-[85vh] flex flex-col`.
- Wrap the middle (sender info + recipients chips + replies block) in a
new `flex-1 min-h-0 overflow-y-auto` scroller with
`data-testid="note-thread-scroll"`.
- Pin the reply composer with `shrink-0` so it stays visible while the
middle scrolls.
- Drop now-redundant `max-h-48` on the recipient ReplyBubble list and
`max-h-72` on `GroupedReplies` — the outer scroller handles overflow,
avoiding nested double scrollbars.
Tests
- New artifacts/tx-os/tests/notes-thread-dialog.spec.mjs:
- Seeds an owner note with 2 recipients and 30 alternating replies via
SQL, deep-links into `/notes?thread=ID`, asserts the dialog's bounding
box height ≤ 0.9 × viewport height, asserts the inner scroll region
actually overflows (scrollHeight > clientHeight), and asserts the
composer remains visible.
- Verified the existing notes-folders.spec.mjs (2 tests) still pass.
- `tsc --noEmit` clean.
Out of scope (left as-is): visual redesign, reply send/archive logic,
Inbox/Sent thread list.
This commit is contained in:
@@ -1167,7 +1167,7 @@ function ThreadDialog({
|
||||
return (
|
||||
<Dialog open onOpenChange={(o) => !o && onClose()}>
|
||||
<DialogContent
|
||||
className={`max-w-lg ${thread ? colorBg(thread.color) : ""}`}
|
||||
className={`max-w-lg max-h-[85vh] flex flex-col ${thread ? colorBg(thread.color) : ""}`}
|
||||
data-testid="note-thread-dialog"
|
||||
>
|
||||
<DialogHeader>
|
||||
@@ -1179,66 +1179,75 @@ function ThreadDialog({
|
||||
<Loading />
|
||||
) : (
|
||||
<>
|
||||
<div className="text-[11px] text-muted-foreground flex items-center gap-1">
|
||||
<Mail size={12} />
|
||||
{t("notes.from", "From:")} {userDisplayName(thread.sender, lang)}
|
||||
</div>
|
||||
{thread.content && (
|
||||
<div className="text-sm text-foreground/85 whitespace-pre-wrap mt-1">
|
||||
{thread.content}
|
||||
{/* Scrollable middle: header info + recipients + replies. The
|
||||
outer DialogContent caps height at 85vh and the composer
|
||||
below stays pinned, so long threads scroll inside this
|
||||
region instead of pushing the dialog past the viewport. */}
|
||||
<div
|
||||
className="flex-1 min-h-0 overflow-y-auto -mx-1 px-1 flex flex-col gap-3"
|
||||
data-testid="note-thread-scroll"
|
||||
>
|
||||
<div className="text-[11px] text-muted-foreground flex items-center gap-1">
|
||||
<Mail size={12} />
|
||||
{t("notes.from", "From:")} {userDisplayName(thread.sender, lang)}
|
||||
</div>
|
||||
)}
|
||||
{(thread.isOwner || thread.isAdmin) && thread.recipients.length > 0 && (
|
||||
<div className="border-t pt-3">
|
||||
<div className="text-xs text-muted-foreground mb-1">
|
||||
{t("notes.to", "To:")}
|
||||
</div>
|
||||
<div className="flex flex-wrap gap-1.5">
|
||||
{thread.recipients.map((r) => (
|
||||
<span
|
||||
key={r.id}
|
||||
className="inline-flex items-center gap-1 text-[11px] px-2 py-0.5 rounded-full bg-white/70 border border-black/5"
|
||||
data-testid={`thread-recipient-${r.recipientUserId}`}
|
||||
>
|
||||
<span>{userDisplayName(r.recipient, lang)}</span>
|
||||
<StatusDot status={r.status} />
|
||||
<span className="text-muted-foreground">
|
||||
{t(`notes.status.${r.status}`, r.status)}
|
||||
</span>
|
||||
</span>
|
||||
))}
|
||||
</div>
|
||||
</div>
|
||||
)}
|
||||
|
||||
<div className="border-t pt-3">
|
||||
<div className="text-xs text-muted-foreground mb-1.5 flex items-center gap-1">
|
||||
<MessageCircle size={12} />
|
||||
{t("notes.replies", "Replies")}
|
||||
{thread.replies.length > 0 && (
|
||||
<span className="text-foreground/70">
|
||||
({thread.replies.length})
|
||||
</span>
|
||||
)}
|
||||
</div>
|
||||
{thread.replies.length === 0 ? (
|
||||
<div className="text-xs text-muted-foreground italic">
|
||||
{t("notes.noRepliesYet", "No replies yet")}
|
||||
</div>
|
||||
) : thread.isOwner || thread.isAdmin ? (
|
||||
<GroupedReplies thread={thread} lang={lang} />
|
||||
) : (
|
||||
<div className="space-y-2 max-h-48 overflow-y-auto">
|
||||
{thread.replies.map((r) => (
|
||||
<ReplyBubble key={r.id} reply={r} lang={lang} />
|
||||
))}
|
||||
{thread.content && (
|
||||
<div className="text-sm text-foreground/85 whitespace-pre-wrap">
|
||||
{thread.content}
|
||||
</div>
|
||||
)}
|
||||
{(thread.isOwner || thread.isAdmin) && thread.recipients.length > 0 && (
|
||||
<div className="border-t pt-3">
|
||||
<div className="text-xs text-muted-foreground mb-1">
|
||||
{t("notes.to", "To:")}
|
||||
</div>
|
||||
<div className="flex flex-wrap gap-1.5">
|
||||
{thread.recipients.map((r) => (
|
||||
<span
|
||||
key={r.id}
|
||||
className="inline-flex items-center gap-1 text-[11px] px-2 py-0.5 rounded-full bg-white/70 border border-black/5"
|
||||
data-testid={`thread-recipient-${r.recipientUserId}`}
|
||||
>
|
||||
<span>{userDisplayName(r.recipient, lang)}</span>
|
||||
<StatusDot status={r.status} />
|
||||
<span className="text-muted-foreground">
|
||||
{t(`notes.status.${r.status}`, r.status)}
|
||||
</span>
|
||||
</span>
|
||||
))}
|
||||
</div>
|
||||
</div>
|
||||
)}
|
||||
|
||||
<div className="border-t pt-3">
|
||||
<div className="text-xs text-muted-foreground mb-1.5 flex items-center gap-1">
|
||||
<MessageCircle size={12} />
|
||||
{t("notes.replies", "Replies")}
|
||||
{thread.replies.length > 0 && (
|
||||
<span className="text-foreground/70">
|
||||
({thread.replies.length})
|
||||
</span>
|
||||
)}
|
||||
</div>
|
||||
{thread.replies.length === 0 ? (
|
||||
<div className="text-xs text-muted-foreground italic">
|
||||
{t("notes.noRepliesYet", "No replies yet")}
|
||||
</div>
|
||||
) : thread.isOwner || thread.isAdmin ? (
|
||||
<GroupedReplies thread={thread} lang={lang} />
|
||||
) : (
|
||||
<div className="space-y-2">
|
||||
{thread.replies.map((r) => (
|
||||
<ReplyBubble key={r.id} reply={r} lang={lang} />
|
||||
))}
|
||||
</div>
|
||||
)}
|
||||
</div>
|
||||
</div>
|
||||
|
||||
{(thread.isOwner ||
|
||||
(thread.myStatus !== null && thread.myStatus !== "archived")) && (
|
||||
<div className="border-t pt-3 flex flex-col gap-2">
|
||||
<div className="border-t pt-3 flex flex-col gap-2 shrink-0">
|
||||
{thread.isOwner && thread.recipients.length > 1 && (
|
||||
<select
|
||||
value={ownerReplyTarget ?? ""}
|
||||
@@ -1376,7 +1385,7 @@ function GroupedReplies({
|
||||
);
|
||||
}
|
||||
return (
|
||||
<div className="space-y-3 max-h-72 overflow-y-auto">
|
||||
<div className="space-y-3">
|
||||
{Array.from(groups.entries()).map(([otherId, replies]) => (
|
||||
<div
|
||||
key={otherId}
|
||||
|
||||
@@ -0,0 +1,155 @@
|
||||
// E2E test: the note thread dialog must cap its height instead of growing
|
||||
// to fill the entire viewport when the conversation is long. Regression
|
||||
// guard for task #417 (user reported the dialog covering the whole page).
|
||||
import { test, expect } from "@playwright/test";
|
||||
import pg from "pg";
|
||||
|
||||
const DATABASE_URL = process.env.DATABASE_URL;
|
||||
if (!DATABASE_URL) {
|
||||
throw new Error("DATABASE_URL must be set to run this UI test");
|
||||
}
|
||||
|
||||
const TEST_PASSWORD = "TestPass123!";
|
||||
const TEST_PASSWORD_HASH =
|
||||
"$2b$10$Bs636ukPMyz01nKrsi.5m.JlDXSN22AVCvn8cgPWWDbo5yJRQX2vu";
|
||||
|
||||
const pool = new pg.Pool({ connectionString: DATABASE_URL });
|
||||
|
||||
const createdUserIds = [];
|
||||
const createdNoteIds = [];
|
||||
|
||||
function uniq() {
|
||||
return `${Date.now().toString(36)}_${Math.random().toString(36).slice(2, 8)}`;
|
||||
}
|
||||
|
||||
async function createUser(prefix) {
|
||||
const username = `${prefix}_${uniq()}`;
|
||||
const { rows } = await pool.query(
|
||||
`INSERT INTO users (username, email, password_hash, display_name_en, preferred_language, is_active)
|
||||
VALUES ($1, $2, $3, $4, 'en', true) RETURNING id`,
|
||||
[username, `${username}@example.com`, TEST_PASSWORD_HASH, prefix],
|
||||
);
|
||||
const id = rows[0].id;
|
||||
createdUserIds.push(id);
|
||||
await pool.query(
|
||||
`INSERT INTO user_roles (user_id, role_id)
|
||||
SELECT $1, id FROM roles WHERE name = 'user'`,
|
||||
[id],
|
||||
);
|
||||
return { id, username };
|
||||
}
|
||||
|
||||
test.afterAll(async () => {
|
||||
if (createdNoteIds.length > 0) {
|
||||
await pool.query(
|
||||
`DELETE FROM note_replies WHERE note_id = ANY($1::int[])`,
|
||||
[createdNoteIds],
|
||||
);
|
||||
await pool.query(
|
||||
`DELETE FROM note_recipients WHERE note_id = ANY($1::int[])`,
|
||||
[createdNoteIds],
|
||||
);
|
||||
await pool.query(`DELETE FROM notes WHERE id = ANY($1::int[])`, [
|
||||
createdNoteIds,
|
||||
]);
|
||||
}
|
||||
if (createdUserIds.length > 0) {
|
||||
await pool.query(`DELETE FROM user_roles WHERE user_id = ANY($1::int[])`, [
|
||||
createdUserIds,
|
||||
]);
|
||||
await pool.query(`DELETE FROM users WHERE id = ANY($1::int[])`, [
|
||||
createdUserIds,
|
||||
]);
|
||||
}
|
||||
await pool.end();
|
||||
});
|
||||
|
||||
async function loginViaUi(page, username) {
|
||||
await page.goto("/login");
|
||||
await page.locator("#username").fill(username);
|
||||
await page.locator("#password").fill(TEST_PASSWORD);
|
||||
await Promise.all([
|
||||
page.waitForURL((url) => !url.pathname.endsWith("/login"), {
|
||||
timeout: 15_000,
|
||||
}),
|
||||
page.locator('form button[type="submit"]').click(),
|
||||
]);
|
||||
}
|
||||
|
||||
test("thread dialog does not exceed viewport height when conversation is long", async ({
|
||||
page,
|
||||
}) => {
|
||||
const VIEWPORT_HEIGHT = 720;
|
||||
await page.setViewportSize({ width: 1280, height: VIEWPORT_HEIGHT });
|
||||
|
||||
const owner = await createUser("thread_dialog_owner");
|
||||
const r1 = await createUser("thread_dialog_recip1");
|
||||
const r2 = await createUser("thread_dialog_recip2");
|
||||
|
||||
await loginViaUi(page, owner.username);
|
||||
|
||||
// Create the note as the owner.
|
||||
const noteResp = await page.request.post("/api/notes", {
|
||||
data: { title: `Long Thread ${uniq()}`, content: "kickoff message" },
|
||||
});
|
||||
expect(noteResp.ok()).toBeTruthy();
|
||||
const note = await noteResp.json();
|
||||
createdNoteIds.push(note.id);
|
||||
|
||||
// Snapshot-deliver to two recipients via SQL (mirrors what /send does).
|
||||
for (const rcp of [r1, r2]) {
|
||||
await pool.query(
|
||||
`INSERT INTO note_recipients
|
||||
(note_id, sender_user_id, recipient_user_id, title, content, color, status)
|
||||
VALUES ($1, $2, $3, $4, $5, 'default', 'replied')`,
|
||||
[note.id, owner.id, rcp.id, note.title, note.content],
|
||||
);
|
||||
}
|
||||
|
||||
// Insert a long conversation alternating between owner and each recipient
|
||||
// (~15 replies per recipient → 30 total) so the OWNER thread view renders
|
||||
// many GroupedReplies bubbles.
|
||||
for (const rcp of [r1, r2]) {
|
||||
for (let i = 0; i < 15; i++) {
|
||||
const fromOwner = i % 2 === 0;
|
||||
const sender = fromOwner ? owner.id : rcp.id;
|
||||
const recipient = fromOwner ? rcp.id : owner.id;
|
||||
await pool.query(
|
||||
`INSERT INTO note_replies (note_id, sender_user_id, recipient_user_id, content)
|
||||
VALUES ($1, $2, $3, $4)`,
|
||||
[note.id, sender, recipient, `Reply #${i + 1} in this conversation`],
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
// Deep-link opens the thread dialog regardless of which tab is active.
|
||||
await page.goto(`/notes?thread=${note.id}`);
|
||||
const dialog = page.getByTestId("note-thread-dialog");
|
||||
await expect(dialog).toBeVisible();
|
||||
|
||||
// Wait for the thread payload to land so the GroupedReplies block renders.
|
||||
await expect(page.getByTestId("note-thread-scroll")).toBeVisible();
|
||||
await expect(
|
||||
dialog.getByTestId(`thread-conversation-${r1.id}`),
|
||||
).toBeVisible();
|
||||
|
||||
// The dialog must stay capped at ~85vh and never exceed 90vh, regardless
|
||||
// of how many replies it contains. The composer + header should still be
|
||||
// reachable (composer stays pinned outside the scroll region).
|
||||
const box = await dialog.boundingBox();
|
||||
expect(box).not.toBeNull();
|
||||
expect(box.height).toBeLessThanOrEqual(VIEWPORT_HEIGHT * 0.9);
|
||||
|
||||
// The internal scroll container should actually overflow (proving content
|
||||
// is being scrolled, not truncated).
|
||||
const scrollInfo = await page
|
||||
.getByTestId("note-thread-scroll")
|
||||
.evaluate((el) => ({
|
||||
scrollHeight: el.scrollHeight,
|
||||
clientHeight: el.clientHeight,
|
||||
}));
|
||||
expect(scrollInfo.scrollHeight).toBeGreaterThan(scrollInfo.clientHeight);
|
||||
|
||||
// Composer remains visible and usable.
|
||||
await expect(page.getByTestId("thread-reply-input")).toBeVisible();
|
||||
});
|
||||
Reference in New Issue
Block a user