refactor(#292 review): extract NAVBAR_COLLAPSE_BREAKPOINT constant (F2)

The AppShell navbar breakpoint and both burger toggles' hiddenFrom/visibleFrom
must be equal, or the sidebar becomes unreachable on tablet widths (the round-1
regression). A comment guarded that before; now a shared const does. Add
NAVBAR_COLLAPSE_BREAKPOINT='md' to sidebar-atom.ts and reference it from the navbar
breakpoint (global-app-shell) + both toggles (app-header). aside.breakpoint and the
sm brand/search gates are intentionally separate contracts, left untouched.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

apps/client/src/components/layouts/global/app-header.tsx

@@ -12,6 +12,7 @@ import TopMenu from "@/components/layouts/global/top-menu.tsx";
 import { Link } from "react-router-dom";
 import { useAtom } from "jotai";
 import {
+  NAVBAR_COLLAPSE_BREAKPOINT,
   desktopSidebarAtom,
   mobileSidebarAtom,
 } from "@/components/layouts/global/hooks/atoms/sidebar-atom.ts";
@@ -53,7 +54,13 @@ export function AppHeader() {
               aria-label={t("Sidebar toggle")}
               opened={mobileOpened}
               onClick={toggleMobile}
-              hiddenFrom="sm"
+              // Must match the AppShell navbar breakpoint (md). The navbar
+              // collapses to the MOBILE drawer below md, so the mobile toggle
+              // (which flips mobileOpened) must be the one visible across the
+              // whole <md band — otherwise at 768-991 the desktop toggle showed
+              // but flipped the wrong atom, leaving the drawer unopenable (the
+              // regression from the initial sm->md navbar change).
+              hiddenFrom={NAVBAR_COLLAPSE_BREAKPOINT}
               size="sm"
             />
           </Tooltip>
@@ -63,7 +70,7 @@ export function AppHeader() {
               aria-label={t("Sidebar toggle")}
               opened={desktopOpened}
               onClick={toggleDesktop}
-              visibleFrom="sm"
+              visibleFrom={NAVBAR_COLLAPSE_BREAKPOINT}
               size="sm"
             />
           </Tooltip>

apps/client/src/components/layouts/global/global-app-shell.tsx

@@ -6,6 +6,7 @@ import SettingsSidebar from "@/components/settings/settings-sidebar.tsx";
 import { useAtom } from "jotai";
 import {
   APP_NAVBAR_ID,
+  NAVBAR_COLLAPSE_BREAKPOINT,
   asideStateAtom,
   desktopSidebarAtom,
   mobileSidebarAtom,
@@ -88,7 +89,13 @@ export default function GlobalAppShell({
       header={{ height: 45 }}
       navbar={{
         width: isSpaceRoute ? sidebarWidth : 300,
-        breakpoint: "sm",
+        // `md` (not `sm`): below 992px the fixed ~300px sidebar leaves too little
+        // room for content — the settings tables (Members/…) overflow the offset
+        // content area on tablet (~768px) and clip the Role/actions columns
+        // off-screen with no horizontal scroll. Collapsing the navbar to a toggle
+        // drawer across the whole tablet band frees the full width for content
+        // (the mobile drawer is closed by default, so nothing overlaps on load).
+        breakpoint: NAVBAR_COLLAPSE_BREAKPOINT,
         collapsed: {
           mobile: !mobileOpened,
           desktop: !desktopOpened,
@@ -97,7 +104,7 @@ export default function GlobalAppShell({
       aside={
         isPageRoute && {
           width: 420,
-          breakpoint: "sm",
+          breakpoint: "md",
           collapsed: { mobile: !isAsideOpen, desktop: !isAsideOpen },
         }
       }

apps/client/src/components/layouts/global/hooks/atoms/sidebar-atom.ts

@@ -7,6 +7,13 @@ import { atom } from "jotai";
 // would create a shell -> chat-window -> shell import cycle).
 export const APP_NAVBAR_ID = "app-shell-navbar";
 
+// Single source of truth for the navbar collapse breakpoint. The AppShell navbar
+// `breakpoint` and BOTH burger toggles' `hiddenFrom`/`visibleFrom` MUST use this
+// exact value: if they drift, the sidebar becomes unreachable on tablet widths
+// (the round-1 regression of #292). Kept here so the shell and the header share
+// one constant the compiler enforces, instead of three hand-synced string literals.
+export const NAVBAR_COLLAPSE_BREAKPOINT = "md";
+
 export const mobileSidebarAtom = atom<boolean>(false);
 
 export const desktopSidebarAtom = atomWithWebStorage<boolean>(

apps/server/src/core/ai-chat/ai-chat.prompt.spec.ts

@@ -303,11 +303,6 @@ describe('buildSystemPrompt page-changed note (#274)', () => {
     expect(prompt).toContain(NOTE_MARKER);
     expect(prompt).toContain('-old line');
     expect(prompt).toContain('+new line');
-    // Strengthened note (#274): instructs a fresh re-read via getPage and steers
-    // the agent toward small, targeted edits instead of a full-page overwrite.
-    expect(prompt).toContain('getPage');
-    expect(prompt.toLowerCase()).toContain('targeted');
-    expect(prompt).toContain('editPageText');
     // Inside the safety sandwich: the trailing SAFETY block follows the note.
     expect(prompt.lastIndexOf(SAFETY_MARKER)).toBeGreaterThan(
       prompt.indexOf(NOTE_MARKER),

apps/server/src/core/ai-chat/ai-chat.prompt.ts

@@ -85,17 +85,11 @@ const INTERRUPT_NOTE =
 const PAGE_CHANGED_NOTE =
   'NOTE: The user edited the open page AFTER your last response in this ' +
   'conversation, so any copy of that page you produced or remember from earlier ' +
-  'is now STALE and must not be reused. Before you edit the page, you MUST first ' +
-  're-read its current content with the getPage tool and base your work on that ' +
-  'live version — never on your earlier copy or on the transcript. The unified ' +
-  'diff below shows exactly what the user changed since you last spoke (lines ' +
-  'starting with "-" were removed, "+" were added) and is the source of truth. ' +
-  'Preserve every one of the user\'s edits: make the smallest change that ' +
-  'satisfies the request using the targeted edit tools (editPageText, patchNode, ' +
-  'insertNode, deleteNode) rather than replacing the whole page, and do not ' +
-  'revert, drop, or overwrite anything the user changed. If a full rewrite is ' +
-  'truly unavoidable, start from the current getPage content and carry over all ' +
-  'of the user\'s edits.';
+  'is now STALE. The unified diff below shows exactly what changed since you last ' +
+  'spoke (lines starting with "-" were removed, "+" were added) and is the source ' +
+  'of truth. Preserve the user\'s edits: build on the current page, do not revert ' +
+  'or overwrite their changes. If you need the full up-to-date page, re-read it ' +
+  'with the getPage tool before editing.';
 
 /**
  * Sanitize a value interpolated into a prompt XML-ish attribute (e.g.

apps/server/src/core/ai-chat/ai-chat.service.spec.ts

@@ -356,32 +356,6 @@ describe('flushAssistant', () => {
     expect(flushed.toolCalls).not.toBeNull();
     expect(flushed.metadata.error).toBe('boom');
   });
-
-  // #274 observability: the page-change diff the agent saw this turn is persisted
-  // to metadata.pageChanged when a non-empty diff was injected, and omitted when
-  // the diff is empty/whitespace or the arg is not supplied.
-  it('persists metadata.pageChanged when a non-empty diff was injected', () => {
-    const f = flushAssistant([], '', 'completed', {
-      pageChanged: { title: 'Doc', diff: '@@ -1 +1 @@\n-old\n+new' },
-    });
-    expect(f.metadata.pageChanged).toEqual({
-      title: 'Doc',
-      diff: '@@ -1 +1 @@\n-old\n+new',
-    });
-  });
-
-  it('omits metadata.pageChanged for an empty/whitespace diff or a missing arg', () => {
-    const whitespace = flushAssistant([], '', 'completed', {
-      pageChanged: { title: 'Doc', diff: '   \n  ' },
-    });
-    expect('pageChanged' in whitespace.metadata).toBe(false);
-
-    const nullArg = flushAssistant([], '', 'completed', { pageChanged: null });
-    expect('pageChanged' in nullArg.metadata).toBe(false);
-
-    const omitted = flushAssistant([], '', 'streaming');
-    expect('pageChanged' in omitted.metadata).toBe(false);
-  });
 });
 
 /**

apps/server/src/core/ai-chat/ai-chat.service.ts

@@ -685,7 +685,7 @@ export class AiChatService implements OnModuleInit {
     // no-op (guarded below) so the turn still streams to the user.
     let assistantId: string | undefined;
     try {
-      const seed = flushAssistant([], '', 'streaming', { pageChanged });
+      const seed = flushAssistant([], '', 'streaming');
       const seeded = await this.aiChatMessageRepo.insert({
         chatId,
         workspaceId: workspace.id,
@@ -720,7 +720,7 @@ export class AiChatService implements OnModuleInit {
         await this.aiChatMessageRepo.update(
           assistantId,
           workspace.id,
-          flushAssistant(capturedSteps, '', 'streaming', { pageChanged }),
+          flushAssistant(capturedSteps, '', 'streaming'),
           { onlyIfStreaming: true },
         );
       } catch (err) {
@@ -860,7 +860,6 @@ export class AiChatService implements OnModuleInit {
               // resolved from the admin-configured provider settings (in
               // closure scope here). Omitted/0 = no limit.
               maxContextTokens: resolved?.chatContextWindow,
-              pageChanged,
             }),
           );
           // Lifecycle: release the external MCP clients leased for this turn.
@@ -912,7 +911,6 @@ export class AiChatService implements OnModuleInit {
           await finalizeAssistant(
             flushAssistant(capturedSteps, inProgressText, 'error', {
               error: errorText,
-              pageChanged,
             }),
           );
           await closeExternalClients();
@@ -942,9 +940,7 @@ export class AiChatService implements OnModuleInit {
               `steps=${steps.length}`,
           );
           await finalizeAssistant(
-            flushAssistant(capturedSteps, inProgressText, 'aborted', {
-              pageChanged,
-            }),
+            flushAssistant(capturedSteps, inProgressText, 'aborted'),
           );
           await closeExternalClients();
           // Advance the page snapshot even on abort (#274): an agent edit that
@@ -1510,7 +1506,6 @@ export function flushAssistant(
     contextTokens?: number;
     maxContextTokens?: number;
     error?: string;
-    pageChanged?: { title: string; diff: string } | null;
   },
 ): AssistantFlush {
   const finished = capturedSteps ?? [];
@@ -1543,15 +1538,6 @@ export function flushAssistant(
   if (extra?.maxContextTokens)
     metadata.maxContextTokens = extra.maxContextTokens;
   if (extra?.error) metadata.error = extra.error;
-  // Persist the page-change diff the agent saw this turn (#274 observability),
-  // so history / the Markdown export can show what the user changed. Only when
-  // a non-empty diff was actually injected into the prompt this turn.
-  if (extra?.pageChanged && extra.pageChanged.diff?.trim().length) {
-    metadata.pageChanged = {
-      title: extra.pageChanged.title,
-      diff: extra.pageChanged.diff,
-    };
-  }
 
   return {
     content: stepsText + trailing,

apps/server/src/core/ai-chat/chat-markdown.util.spec.ts

@@ -269,168 +269,6 @@ describe('buildChatMarkdown (server) — structure', () => {
     expect(md).toContain('**⚠️ Error:** 401: Unauthorized');
   });
 
-  // #274 observability: an assistant row whose turn started with a user edit to
-  // the open page carries metadata.pageChanged = { title, diff }; the export
-  // renders the diff the agent saw, before the message body.
-  it('renders the persisted page-change diff block for an assistant row', () => {
-    const md = buildChatMarkdown({
-      title: 'T',
-      chatId: 'c',
-      rows: [
-        row({
-          role: 'assistant',
-          content: 'answer',
-          metadata: {
-            pageChanged: { title: 'Doc', diff: '@@ -1 +1 @@\n-old\n+new' },
-          } as never,
-        }),
-      ],
-    });
-    expect(md).toContain(
-      'The user edited this page before this turn; the diff the agent saw:',
-    );
-    expect(md).toContain('("Doc")');
-    expect(md).toContain('-old');
-    expect(md).toContain('+new');
-    // The diff sits before the message body (chronological: change, then reply).
-    expect(md.indexOf('-old')).toBeLessThan(md.indexOf('answer'));
-  });
-
-  it('does not render the page-change block when metadata.pageChanged is absent', () => {
-    const md = buildChatMarkdown({
-      title: 'T',
-      chatId: 'c',
-      rows: [row({ role: 'assistant', content: 'answer' })],
-    });
-    expect(md).not.toContain(
-      'The user edited this page before this turn; the diff the agent saw:',
-    );
-  });
-
-  // #288 F1/F2: an empty page title must render the BARE heading with no
-  // `("…")` suffix (the `pc.title ? … : …` false branch).
-  it('renders the page-change heading with no title suffix when title is empty', () => {
-    const md = buildChatMarkdown({
-      title: 'T',
-      chatId: 'c',
-      rows: [
-        row({
-          role: 'assistant',
-          content: 'answer',
-          metadata: {
-            pageChanged: { title: '', diff: '@@ -1 +1 @@\n-old\n+new' },
-          } as never,
-        }),
-      ],
-    });
-    // Bare heading, single line, no parenthesized title.
-    expect(md).toContain(
-      '> **📝 The user edited this page before this turn; the diff the agent saw:**',
-    );
-    expect(md).not.toContain('("');
-    expect(md).toContain('-old');
-  });
-
-  // #288 F1: the page title is UNTRUSTED cross-user data, so a title carrying a
-  // newline / backtick / `"` / `<`/`>` must be neutralized by escapeAttr before
-  // it is interpolated into the `> **…**` blockquote heading — otherwise it
-  // could break the blockquote onto multiple lines or inject markup/HTML into
-  // the downloaded .md. escapeAttr strips `<>"` and collapses whitespace runs to
-  // a single space, so `Ev"il\n> `x` <b>` becomes ``Evil `x` b``.
-  it('escapes an untrusted page title in the page-change heading', () => {
-    const md = buildChatMarkdown({
-      title: 'T',
-      chatId: 'c',
-      rows: [
-        row({
-          role: 'assistant',
-          content: 'answer',
-          metadata: {
-            pageChanged: {
-              title: 'Ev"il\n> `x` <b>',
-              diff: '@@ -1 +1 @@\n-old\n+new',
-            },
-          } as never,
-        }),
-      ],
-    });
-    // The heading stays a single blockquote line with the escaped title.
-    expect(md).toContain(
-      '> **📝 The user edited this page before this turn; the diff the agent saw: ("Evil `x` b")**',
-    );
-    // No raw attribute/markup breakers survived from the title.
-    expect(md).not.toContain('Ev"il');
-    expect(md).not.toContain('<b>');
-  });
-
-  // #288 review F1: escapeAttr ALONE is insufficient for this MARKDOWN sink —
-  // link/image syntax survives it. A cross-user title with `![x](url)` /
-  // `[phish](url)` must NOT become a working remote image or clickable link in
-  // the downloaded .md; markdownHeadingSafe backslash-escapes `[`/`]` so both are
-  // inert. (Non-vacuous: fails against the escapeAttr-only version, which left
-  // `](https://` intact.)
-  it('neutralizes markdown link/image syntax in an untrusted page title', () => {
-    const md = buildChatMarkdown({
-      title: 'T',
-      chatId: 'c',
-      rows: [
-        row({
-          role: 'assistant',
-          content: 'answer',
-          metadata: {
-            pageChanged: {
-              title:
-                '![x](https://attacker.example/t.png) and [click](https://phish.example)',
-              diff: '@@ -1 +1 @@\n-old\n+new',
-            },
-          } as never,
-        }),
-      ],
-    });
-    // No WORKING image/link syntax survives — the `[…]` sits escaped as `\[…\]`,
-    // so the unescaped `![x](` image and `[click](` link markers are gone. (We
-    // deliberately do NOT assert `not.toContain('](https://')`: after escaping the
-    // literal `\](https://` still contains `](https://` as a raw substring — that
-    // check would false-fail even though the link is inert.)
-    expect(md).not.toContain('![x](');
-    expect(md).not.toContain('[click](');
-    // The brackets are backslash-escaped, so `[text](url)`/`![text](url)` are inert.
-    expect(md).toContain('\\[');
-    expect(md).toContain('\\]');
-    // The heading stays a SINGLE blockquote line (no newline injected).
-    const headingLine = md
-      .split('\n')
-      .find((l) => l.includes('the diff the agent saw:'));
-    expect(headingLine).toBeDefined();
-    expect(headingLine).toContain('\\[x\\]');
-    expect(headingLine).toContain('\\[click\\]');
-  });
-
-  // #288 internal review Finding 2: a NON-empty title made up entirely of
-  // escapeAttr breakers (`<>"`) escapes to '' — the ternary must then fall to the
-  // BARE heading with NO `("…")` suffix. Locks the ternary-on-escaped-value
-  // behavior (distinct from the empty-string input test above).
-  it('renders the bare heading for a title that escapes to empty', () => {
-    const md = buildChatMarkdown({
-      title: 'T',
-      chatId: 'c',
-      rows: [
-        row({
-          role: 'assistant',
-          content: 'answer',
-          metadata: {
-            pageChanged: { title: '<>"', diff: '@@ -1 +1 @@\n-old\n+new' },
-          } as never,
-        }),
-      ],
-    });
-    expect(md).toContain(
-      '> **📝 The user edited this page before this turn; the diff the agent saw:**',
-    );
-    expect(md).not.toContain('("');
-    expect(md).toContain('-old');
-  });
-
   it('escapes embedded triple-backtick fences with a longer delimiter', () => {
     const md = buildChatMarkdown({
       title: 'T',

apps/server/src/core/ai-chat/chat-markdown.util.ts

@@ -15,7 +15,6 @@
  */
 
 import type { AiChatMessage } from '@docmost/db/types/entity.types';
-import { escapeAttr } from './ai-chat.prompt';
 
 /** Supported export label languages. Defaults to English. */
 export type ExportLang = 'en' | 'ru';
@@ -64,7 +63,6 @@ const LABELS: Record<
     tools: Record<string, string>;
     ranTool: (name: string) => string;
     stillGenerating: string;
-    pageEditedByUser: string;
   }
 > = {
   en: {
@@ -85,8 +83,6 @@ const LABELS: Record<
     ranTool: (name) => `Ran tool ${name}`,
     stillGenerating:
       'This message is still being generated — the export captured a partial, in-progress response.',
-    pageEditedByUser:
-      'The user edited this page before this turn; the diff the agent saw:',
   },
   ru: {
     untitled: 'Без названия',
@@ -106,29 +102,9 @@ const LABELS: Record<
     ranTool: (name) => `Выполнил инструмент ${name}`,
     stillGenerating:
       'Это сообщение всё ещё генерируется — экспорт захватил частичный, незавершённый ответ.',
-    pageEditedByUser:
-      'Пользователь изменил страницу перед этим ходом; дифф, который видел агент:',
   },
 };
 
-/**
- * Make an untrusted title safe to interpolate into a Markdown blockquote
- * HEADING. escapeAttr() neutralizes the XML/HTML breakers (`<` `>` `"`) and
- * collapses whitespace for the PROMPT sink (`page="…"`), but this export sink is
- * MARKDOWN — link/image syntax survives escapeAttr. So additionally backslash-
- * escape `[` and `]`: that disables both `[text](url)` links and `![text](url)`
- * images, so a cross-user title like `![x](http://evil)` or `[phish](http://evil)`
- * cannot inject a remote (auto-loading) image or a clickable link into the
- * downloaded .md disguised as a trusted system annotation. A bare `(url)` with no
- * preceding `[]` is inert Markdown, so brackets are the only security-critical
- * characters here. (We leave backticks to escapeAttr's whitespace pass — a title
- * shown as inline code cannot escape the blockquote line or load a resource, so
- * it is not a security concern for this sink.)
- */
-function markdownHeadingSafe(title: string): string {
-  return escapeAttr(title).replace(/[[\]]/g, (m) => `\\${m}`);
-}
-
 /** True for AI SDK tool parts (static `tool-*` or `dynamic-tool`). */
 function isToolPart(type: string): boolean {
   return type.startsWith('tool-') || type === 'dynamic-tool';
@@ -232,23 +208,6 @@ function rowParts(row: AiChatMessage): ExportPart[] {
     : [{ type: 'text', text: row.content ?? '' }];
 }
 
-/** The persisted page-change diff the agent saw this turn (#274), when any. */
-function pageChangedOf(
-  row: AiChatMessage,
-): { title: string; diff: string } | undefined {
-  const meta = (row.metadata ?? {}) as {
-    pageChanged?: { title?: string; diff?: string };
-  };
-  const pc = meta.pageChanged;
-  if (pc && typeof pc.diff === 'string' && pc.diff.trim().length > 0) {
-    return {
-      title: typeof pc.title === 'string' ? pc.title : '',
-      diff: pc.diff,
-    };
-  }
-  return undefined;
-}
-
 /**
  * Serialize a chat to a Markdown string from its persisted rows. Source = DB
  * ONLY (no live client state). A row whose `status` is still 'streaming' is an
@@ -307,26 +266,6 @@ export function buildChatMarkdown(args: {
       blocks.push(`<!-- ${iso} -->`);
     }
 
-    // Page-change observability (#274): show the diff the agent saw at the start
-    // of this turn, before its response, so the export reflects the stale-page
-    // warning the model received.
-    const pc = pageChangedOf(row);
-    if (pc) {
-      // The page title is UNTRUSTED cross-user data (a collaborative page's title
-      // controllable by another user). escapeAttr() alone (the prompt sink) is
-      // INSUFFICIENT here: this is a MARKDOWN sink, so we neutralize link/image
-      // syntax too (backslash-escaping `[`/`]`) before interpolating it into this
-      // `> **…**` blockquote heading — otherwise `![x](url)` / `[phish](url)` would
-      // inject a remote image or clickable link into the downloaded .md. An
-      // all-`<>"` title escapes to empty and correctly falls to the bare heading.
-      // The diff body is already safe via fence(). (#288 review F1.)
-      const safeTitle = markdownHeadingSafe(pc.title);
-      const heading = safeTitle
-        ? `${L.pageEditedByUser} ("${safeTitle}")`
-        : L.pageEditedByUser;
-      blocks.push(`> **📝 ${heading}**\n\n${fence(pc.diff, 'diff')}`);
-    }
-
     blocks.push(...renderMessageParts(rowParts(row), lang));
 
     // A still-'streaming' row is an interrupted/in-progress turn captured by the