fix(dashboard): put plan-review Add comment above actions on tablet

Tablet (≤1024px) now uses the action-rail Add-comment control as a full-width row above Refine/Proceed instead of the document-end trigger. Phone keeps the fixed bar above the mobile nav; desktop keeps the in-document control.
2026-07-23 17:36:10 -07:00
parent c1d37cb137
commit 28e2cd587c
5 changed files with 84 additions and 63 deletions
--- a/packages/dashboard/app/components/PlanningModeModal.css
+++ b/packages/dashboard/app/components/PlanningModeModal.css
@@ -1796,9 +1796,12 @@ its established Refine and Proceed action hierarchy.

 /*
 FNXC:PlanningComments 2026-07-23-17:05:
-Desktop (≥769px) keeps the in-document trigger under the plan markdown. Mobile (≤768px) lifts
-the counterpart into a fixed bottom bar in the visual viewport so it never requires scrolling
-the plan document or action rail after a touch selection.
+Desktop (≥1025px) keeps the in-document trigger under the plan markdown. Tablet and phone
+(≤1024px) use the action-rail counterpart so selection comments are not lost under the plan fold.
+
+FNXC:PlanningComments 2026-07-24-05:35:
+Tablet (769–1024) places that rail control as a full-width row above Refine/Proceed. Phone
+(≤768) still lifts it into a fixed bottom bar above the mobile nav.
 */

 .planning-comment-quote,
@@ -1874,6 +1877,24 @@ the plan document or action rail after a touch selection.
    grid-column: 1 / -1;
  }

+  /*
+  FNXC:PlanningComments 2026-07-24-05:35:
+  Tablet and phone hide the document-adjacent trigger (it sits at the end of a long plan and
+  requires scrolling). The action-rail variant becomes the sole control and spans the full
+  footer width above Refine/Proceed so a selection always exposes Add comment without leaving
+  the plan action baseline.
+  */
+  .planning-add-comment--document {
+    display: none;
+  }
+
+  .planning-plan-actions .btn.planning-add-comment--mobile {
+    display: flex;
+    grid-column: 1 / -1;
+    margin-top: 0;
+    justify-content: center;
+  }
+
  .planning-refine-menu {
    right: var(--space-lg);
    left: var(--space-lg);
@@ -1892,34 +1913,17 @@ plan actions, and a token-sized bottom inset keep all three controls inline with
 */
@media (max-width: 768px) {
  /*
-  FNXC:PlanningComments 2026-07-31-00:00:
-  A selected quote needs a reachable mobile action, not a control lost in the scrollable
-  document. At 769px and above the selection-adjacent document trigger stays canonical.
-
  FNXC:PlanningComments 2026-07-23-17:05:
-  The mobile trigger is position:fixed to the selection midpoint and clamped into the visual
-  viewport (including safe-area + mobile-nav clearance) so it always appears after a selection
-  and can be dismissed by selectionchange when the selection collapses — without scrolling the
-  screen. These established 768px/1024px media boundaries are permitted; new dimensions use only
-  existing design tokens.
-  */
-  .planning-add-comment--document {
-    display: none;
-  }
+  On phone, pin the rail Add-comment control to the visual viewport above the mobile nav so a
+  touch selection never requires scrolling the plan document or footer. Tablet (769–1024) keeps
+  the in-flow full-width rail row from the 1024px block instead.

-  /*
-  FNXC:PlanningComments 2026-07-23-17:05:
-  Pin the mobile Add-comment control to the visual viewport above the mobile nav/safe-area.
-  A fixed bar appears immediately after selectionchange and dismisses when the selection
-  collapses — no document or action-rail scroll required.
-
-  The mobile plan-actions button rule forces width 100 percent. With position fixed that 100
-  percent is the viewport width, and combined with left/right insets the control overflowed past
-  the right edge (measured 780px wide in a 768px viewport). Use a higher-specificity selector and
-  width auto so left+right define the used width and the bar stays fully on-screen.
+  The plan-actions button rule forces width 100 percent. With position fixed that 100 percent is
+  the viewport width, and combined with left/right insets the control overflowed past the right
+  edge (measured 780px wide in a 768px viewport). Higher specificity + width auto lets left/right
+  define the used width so the bar stays fully on-screen.
  */
  .planning-plan-actions .btn.planning-add-comment--mobile {
-    display: flex;
    position: fixed;
    left: var(--space-md);
    right: var(--space-md);
@@ -1931,8 +1935,6 @@ plan actions, and a token-sized bottom inset keep all three controls inline with
    z-index: var(--z-popover);
    width: auto;
    max-width: none;
-    margin-top: 0;
-    justify-content: center;
    box-shadow: var(--shadow-md);
  }

--- a/packages/dashboard/app/components/PlanningModeModal.tsx
+++ b/packages/dashboard/app/components/PlanningModeModal.tsx
@@ -692,8 +692,13 @@ export function PlanningModeModal({ isOpen, onClose, onTaskCreated, onTasksCreat
  }, []);

  const focusAddCommentTrigger = useCallback(() => {
-    const isMobileViewport = window.matchMedia?.("(max-width: 768px)").matches ?? false;
-    (isMobileViewport ? mobileAddCommentTriggerRef : addCommentTriggerRef).current?.focus();
+    /*
+    FNXC:PlanningComments 2026-07-24-05:35:
+    Tablet and phone both expose the action-rail trigger; only wide desktop uses the document
+    variant. Match the 1024px CSS gate so focus restore lands on the visible control.
+    */
+    const usesRailTrigger = window.matchMedia?.("(max-width: 1024px)").matches ?? false;
+    (usesRailTrigger ? mobileAddCommentTriggerRef : addCommentTriggerRef).current?.focus();
  }, []);

  /*
@@ -3053,15 +3058,19 @@ export function PlanningModeModal({ isOpen, onClose, onTaskCreated, onTasksCreat
      <div className="planning-actions planning-summary-actions planning-plan-actions" data-testid="planning-plan-actions">
        {/*
        FNXC:PlanningComments 2026-07-31-00:00:
-        FN-8533 keeps the selection-adjacent control at 769px and wider, but mobile needs a
+        FN-8533 keeps the selection-adjacent control on wide desktop, but compact shells need a
        counterpart that cannot be lost under the document fold.

        FNXC:PlanningComments 2026-07-23-17:05:
-        On ≤768px the mobile trigger is position:fixed to the visual viewport above the mobile
-        nav so it appears immediately after a selection without scrolling, and document-level
-        selectionchange dismisses it when the selection collapses. CSS still shows exactly one
-        of the two variants; only established 768px/1024px breakpoint literals are allowed here,
-        while all other dimensions remain design-token based.
+        On ≤768px the rail trigger is position:fixed above the mobile nav so a selection never
+        requires scrolling.
+
+        FNXC:PlanningComments 2026-07-24-05:35:
+        On tablet (769–1024) the same rail control stays in the plan action footer as a full-width
+        row above Refine/Proceed. Document-level selectionchange still dismisses it when the
+        selection collapses. CSS shows exactly one of the two variants; only established
+        768px/1024px breakpoint literals are allowed here, while all other dimensions remain
+        design-token based.
        */}
        {selectedPlanQuote && !isCommentEditorOpen && (
          <button
--- a/packages/dashboard/app/components/tests/PlanningModeModal.css.test.ts
+++ b/packages/dashboard/app/components/tests/PlanningModeModal.css.test.ts
@@ -166,19 +166,22 @@ describe("PlanningModeModal CSS responsive action contract", () => {
    expectSomeRule(tabletCss, ".planning-plan-actions", /flex-wrap\s*:\s*nowrap\s*;/);
  });

-  it("shows exactly one contextual comment trigger fixed to the mobile selection viewport", () => {
+  it("shows exactly one contextual comment trigger in the tablet/phone plan action rail", () => {
    const css = loadPlanningCss();
+    const compactCss = getMediaBlocks(css, "@media (max-width: 1024px)").join("\n");
    const mobileCss = getMediaBlocks(css, MOBILE_ACTIONS_QUERY).join("\n");
+    const tabletRailTriggerRule = findRule(compactCss, ".planning-plan-actions .btn.planning-add-comment--mobile");
    const mobileTriggerRule = findRule(mobileCss, ".planning-plan-actions .btn.planning-add-comment--mobile");
    const mobileEditorRule = findRule(mobileCss, ".planning-comment-editor");

    expect(findRule(css, ".planning-add-comment--mobile")).toMatch(/display\s*:\s*none\s*;/);
-    expect(findRule(mobileCss, ".planning-add-comment--document")).toMatch(/display\s*:\s*none\s*;/);
-    expect(mobileTriggerRule).toMatch(/display\s*:\s*flex\s*;/);
+    expect(findRule(compactCss, ".planning-add-comment--document")).toMatch(/display\s*:\s*none\s*;/);
+    expect(tabletRailTriggerRule).toMatch(/display\s*:\s*flex\s*;/);
+    expect(tabletRailTriggerRule).toMatch(/grid-column\s*:\s*1\s*\/\s*-1\s*;/);
+    expect(tabletRailTriggerRule).toMatch(/margin-top\s*:\s*0\s*;/);
    expect(mobileTriggerRule).toMatch(/position\s*:\s*fixed\s*;/);
    expect(mobileTriggerRule).toMatch(/width\s*:\s*auto\s*;/);
    expect(mobileTriggerRule).toMatch(/var\(--mobile-nav-height/);
-    expect(mobileTriggerRule).toMatch(/margin-top\s*:\s*0\s*;/);
    expect(mobileEditorRule).toMatch(/position\s*:\s*fixed\s*;/);
    expect(mobileEditorRule).toMatch(/var\(--mobile-nav-height/);
  });
--- a/packages/dashboard/app/components/tests/PlanningModeModal.ui-interactions.test.tsx
+++ b/packages/dashboard/app/components/tests/PlanningModeModal.ui-interactions.test.tsx
@@ -34,8 +34,9 @@ describe("PlanningModeModal sequential layout", () => {
    expect(css).toMatch(/@media \(max-width: 1024px\)[\s\S]*?\.planning-plan-actions \.btn\s*\{[^}]*width\s*:\s*100%\s*;/);
    expect(css).toMatch(/@media \(max-width: 768px\)[\s\S]*?\.planning-plan-actions\s*\{[^}]*gap\s*:\s*var\(--space-md\)\s*;[^}]*calc\(var\(--space-sm\) \+ env\(safe-area-inset-bottom\)\)/);
    expect(css).toMatch(/\.planning-add-comment--mobile\s*\{[^}]*display\s*:\s*none\s*;/);
-    expect(css).toMatch(/@media \(max-width: 768px\)[\s\S]*?\.planning-add-comment--document\s*\{[^}]*display\s*:\s*none\s*;/);
-    expect(css).toMatch(/@media \(max-width: 768px\)[\s\S]*?\.planning-plan-actions \.btn\.planning-add-comment--mobile\s*\{[^}]*display\s*:\s*flex\s*;[^}]*position\s*:\s*fixed\s*;[^}]*width\s*:\s*auto\s*;/);
+    expect(css).toMatch(/@media \(max-width: 1024px\)[\s\S]*?\.planning-add-comment--document\s*\{[^}]*display\s*:\s*none\s*;/);
+    expect(css).toMatch(/@media \(max-width: 1024px\)[\s\S]*?\.planning-plan-actions \.btn\.planning-add-comment--mobile\s*\{[^}]*display\s*:\s*flex\s*;[^}]*grid-column\s*:\s*1\s*\/\s*-1\s*;/);
+    expect(css).toMatch(/@media \(max-width: 768px\)[\s\S]*?\.planning-plan-actions \.btn\.planning-add-comment--mobile\s*\{[^}]*position\s*:\s*fixed\s*;[^}]*width\s*:\s*auto\s*;/);
    expect(css).toMatch(/@media \(max-width: 768px\)[\s\S]*?\.planning-comment-editor\s*\{[^}]*position\s*:\s*fixed\s*;/);
  });
 });
--- a/packages/dashboard/src/tests/planning-browser-e2e.test.ts
+++ b/packages/dashboard/src/tests/planning-browser-e2e.test.ts
@@ -209,9 +209,10 @@ describe.runIf(executablePath)("Planning Mode browser E2E", () => {

  async function verifyContextualCommentPlacement(
    viewport: { width: number; height: number },
-    inFooter: boolean,
+    options: { inFooter: boolean; positionFixed: boolean },
    presentation: "embedded" | "modal",
  ): Promise<void> {
+    const { inFooter, positionFixed } = options;
    const page = await browser.newPage({ viewport });
    await page.goto(`${baseUrl}app/planning-browser-e2e-fixture.html?surface=plan-review&presentation=${presentation}&reset=1`);
    if (viewport.width <= 768) await page.getByRole("tab", { name: "Plan preview" }).click();
@@ -235,9 +236,9 @@ describe.runIf(executablePath)("Planning Mode browser E2E", () => {
      Start at the preceding tab stop, then send actual Tab keys. Programmatic focus alone would
      incorrectly accept a tabIndex=-1 contextual-comment trigger as keyboard reachable.

-      FNXC:PlanningComments 2026-07-23-17:05:
-      Mobile keeps the trigger in the actions DOM for focus order, but CSS fixes it to the
-      selection so it is inside the visual viewport without scrolling the plan document.
+      FNXC:PlanningComments 2026-07-24-05:35:
+      Phone keeps the trigger position:fixed above the nav; tablet keeps it in the action rail as
+      a full-width row above Refine/Proceed. Both stay in the actions DOM for focus order.
      */
      focusable[triggerIndex - 1]?.focus();
      return {
@@ -266,20 +267,21 @@ describe.runIf(executablePath)("Planning Mode browser E2E", () => {
      totalButtons: 2,
      visibleButtons: 1,
      visibleInActions: inFooter,
-      positionFixed: inFooter,
-      // Mobile fixed-to-selection control must be in the visual viewport without scrolling.
+      positionFixed,
+      // Compact shells keep the control in the visual viewport without scrolling the plan.
      triggerInsideViewport: inFooter ? true : expect.any(Boolean),
      triggerHasPreviousTabStop: true,
      hiddenButtonsTabbable: 0,
    });
-    /*
-    FNXC:PlanningComments 2026-07-23-17:05:
-    The mobile trigger is position:fixed out of the action-grid flow, so it no longer appears in
-    the action-rail label list — only Refine/Proceed remain there while the floating control is
-    separately asserted as the sole visible Add-comment button.
-    */
-    if (inFooter) expect(placement.actionLabels).toEqual(expect.arrayContaining(["Refine", "Proceed with plan"]));
-    else expect(placement.actionLabels).not.toContain("Add comment to selection");
+    if (inFooter) {
+      expect(placement.actionLabels).toEqual(expect.arrayContaining(["Refine", "Proceed with plan"]));
+      if (!positionFixed) {
+        // Tablet in-flow rail row: Add comment sits above Refine/Proceed in the same footer.
+        expect(placement.actionLabels).toEqual(expect.arrayContaining(["Add comment to selection"]));
+      }
+    } else {
+      expect(placement.actionLabels).not.toContain("Add comment to selection");
+    }

    let reachedTriggerByTab = false;
    for (let tabCount = 0; tabCount < 8; tabCount += 1) {
@@ -312,9 +314,9 @@ describe.runIf(executablePath)("Planning Mode browser E2E", () => {
    expect(afterOpen).toMatchObject({
      addCommentButtons: 0,
      actionChildren: 0,
-      editorPosition: inFooter ? "fixed" : "static",
-      // Mobile pins the composer into the viewport; desktop keeps the in-document editor.
-      editorInsideViewport: inFooter ? true : expect.any(Boolean),
+      // Phone pins the composer; tablet/desktop keep the in-document editor.
+      editorPosition: positionFixed ? "fixed" : "static",
+      editorInsideViewport: positionFixed ? true : expect.any(Boolean),
    });

    await page.evaluate(() => {
@@ -332,9 +334,13 @@ describe.runIf(executablePath)("Planning Mode browser E2E", () => {

  it("places the sole contextual comment trigger by viewport in embedded and modal Planning", async () => {
    for (const presentation of ["embedded", "modal"] as const) {
-      await verifyContextualCommentPlacement({ width: 768, height: 900 }, true, presentation);
-      await verifyContextualCommentPlacement({ width: 769, height: 900 }, false, presentation);
-      await verifyContextualCommentPlacement({ width: 1280, height: 900 }, false, presentation);
+      // Phone: fixed bar above nav.
+      await verifyContextualCommentPlacement({ width: 768, height: 900 }, { inFooter: true, positionFixed: true }, presentation);
+      // Tablet: full-width action-rail row above Refine/Proceed.
+      await verifyContextualCommentPlacement({ width: 769, height: 900 }, { inFooter: true, positionFixed: false }, presentation);
+      await verifyContextualCommentPlacement({ width: 1024, height: 900 }, { inFooter: true, positionFixed: false }, presentation);
+      // Desktop: document-adjacent trigger only.
+      await verifyContextualCommentPlacement({ width: 1280, height: 900 }, { inFooter: false, positionFixed: false }, presentation);
    }
  }, 30_000);
 });