fix(popover): keep the visibility dropdown on screen (#72)

[k5s-bot]

Summary

Fixes #72 — "Pop overs should respect screen boundaries".

The visibility picker dropdown (the Private / org / "Share with people…" menu used in the sidebar and on note/task pages) only clamped itself horizontally. When it was opened near the bottom of the viewport — e.g. a page low in the sidebar tree, or on a short/mobile viewport — it opened straight down past the bottom edge with no scroll and no flip, so its options were off-screen and unreachable. This matches the report: "popping up off screen with no way to get that content."

The DatePicker and the sidebar context menu already clamp both axes; this brings the visibility picker in line.

Changes

• New shared helper src/lib/utils/popoverPosition.ts — clampPopoverToViewport(trigger, size, viewport) computes a placement that stays fully on screen on both axes:
  • horizontally: align to the trigger's left edge, flip to its right edge if it would overflow, then clamp within the viewport margins;
  • vertically: open below the trigger, or flip above when it would run off the bottom and there's more room up top, returning a maxHeight so tall content scrolls internally instead of spilling off-screen.
• VisibilityPicker.svelte now uses that helper for positioning and sets overflow-y: auto + the computed max-height on the dropdown.
• Unit tests src/lib/utils/popoverPosition.test.ts cover left-align, right-edge flip, left-edge clamp, bottom-edge flip-up, and height-capping.

Testing

• pnpm test (vitest): 910 passed (68 files, including the new test file).
• svelte-check: 0 errors, 0 warnings.

Scope note

I limited the change to the one component that actually overflowed (the visibility picker). DatePicker and TreeNodeContextMenu already clamp both axes, so I left them untouched; the new helper is available to consolidate them later if desired.

---

Maintainers: apply the ai-review label to this PR to request an automated code review, or the ai-triage label to have the review feedback addressed and the branch brought back in sync with its base. (The automation can't apply either label to its own PR, so a human needs to add it.)

[k5s-bot]

🤖 Starting work on this now.

[k5s-bot]

🤖 Watch live or take over the session here: https://claude.ai/code/session_0146PGYa9JBFjt8ZxxVDKBLf

[k5s-bot]

Reviewed the diff and ran the suite locally: the new clampPopoverToViewport math is correct on both axes, the global box-sizing: border-box (src/app.css:88) means the always-applied max-height won't overflow by padding/border, and src/lib/utils/popoverPosition.test.ts passes (7/7). The scope note holds — DatePicker (src/lib/components/shared/DatePicker.svelte:49-52) already flips vertically, so leaving it is reasonable. This correctly fixes #72 for the visibility picker.

One minor robustness note inline. Heads-up unrelated to this diff: the frontend CI job is failing, but on a pre-existing pnpm audit advisory for the transitive devalue dependency (via svelte-tiptap/svelte-dnd-action) — not caused by this PR.

[k5s-bot]

Review posted successfully.

Summary

I reviewed PR #73 (imaustink/glyph) — "fix(popover): keep the visibility dropdown on screen (#72)". I checked out the branch, examined the diff, installed deps, and ran the new test file locally (7/7 pass).

Verdict: approve-with-a-minor-note. The change is well-scoped and correct:

• clampPopoverToViewport math is sound on both axes (horizontal left-align → right-edge flip → clamp; vertical open-below → flip-above with a scrollable maxHeight). I traced the edge cases and the branch conditions hold.
• The always-applied max-height is safe because the repo sets a global box-sizing: border-box (src/app.css:88), so padding+border don't push it past the viewport.
• The scope decision to leave DatePicker/TreeNodeContextMenu untouched checks out — DatePicker already flips vertically (DatePicker.svelte:49-52).

One inline comment (with a one-click suggestion): on scroll/resize re-measurement, offsetHeight reads the already-capped box rather than the content's natural height; using scrollHeight makes the flip decision use the true content height. Low impact and self-correcting, but a concrete improvement.

CI note I flagged in the review: the frontend check is failing, but on a pre-existing pnpm audit advisory for the transitive devalue dependency (pulled in via svelte-tiptap/svelte-dnd-action) — not caused by this PR's diff. The other jobs (api, collab, e2e, helm) are green.

No code was changed…

---

⚠️ Work is on imaustink/glyph branch pr73, but no open pull request was found.

[imaustink]

Make sure to sync with origin main

[k5s-bot]

🤖 Starting work on this now.

[k5s-bot]

🤖 Watch live or take over the session here: https://claude.ai/code/session_01BZvrVWXhk7cKW5rBFawLWL

[k5s-bot]

Picked this PR back up (ai-triage)

Addressed the review feedback, fixed the failing check, and synced the branch with main.

Review feedback addressed

• scrollHeight vs offsetHeight (review comment): updateDropdownPosition now measures the dropdown's natural content height via dropdownRef.scrollHeight instead of offsetHeight. Once a max-height is applied, offsetHeight reports the clamped box, so the flip/placement re-measurement on scroll/resize was running against a capped value. Reading scrollHeight uses the true content height. Commit 91fb194; thread resolved.

Failing check fixed

• The frontend job was red on the pnpm audit --prod step — the transitive devalue advisory (via svelte-tiptap / svelte-dnd-action → svelte). As the review noted, this was pre-existing and not caused by this PR: main had already fixed it by bumping the pnpm-workspace.yaml override to devalue@<5.9.3 → >=5.9.3 <6. This branch was simply stale (it still carried the old devalue@>=5.6.3 <=5.8.0 → >=5.8.1 override). Merging main in brought the fix, and pnpm audit --prod now reports No known vulnerabilities found.

Branch synced with base

• Merged origin/main into the branch (6163330), matching this repo's existing sync convention (Merge remote-tracking branch 'origin/main' into <branch>). The merge was conflict-free — main's changes (the "Shared with me" sidebar, multi-arch build CI, dependency overrides) don't touch the popover files.

Verification (local)

• pnpm test (vitest): 910 passed (68 files, incl. popoverPosition.test.ts 7/7).
• pnpm check (svelte-check): 0 errors, 0 warnings (888 files).
• pnpm audit --prod: clean.

Deliberately not done

• Left DatePicker and TreeNodeContextMenu untouched — per the PR's original scope note they already clamp both axes, and consolidating them onto the shared helper is out of scope for this fix.
• Did not modify the devalue/yjs overrides myself — the correct values came in from main; no independent dependency change was needed.

CI is re-running on the new push; the previously-red frontend audit step should now pass given the local audit is clean.

[k5s-bot]

Done. The PR is MERGEABLE and the new CI run is in progress. Here's the summary.

What I did on PR #73

Reviewed all feedback:

• One inline review comment from k5s-bot (use scrollHeight not offsetHeight).
• The review body noting the frontend CI job was failing on a pre-existing devalue audit advisory.

Changes pushed to fix/popover-screen-boundaries-72:

1. Addressed the review comment (91fb194) — updateDropdownPosition now measures the dropdown's natural content height via dropdownRef.scrollHeight instead of offsetHeight. Once a max-height is applied, offsetHeight reports the clamped box, so the flip/placement re-measurement on scroll/resize was running against a capped value. Replied to the comment and resolved the thread.

2. Fixed the failing frontend check by syncing with base. The red step was pnpm audit --prod on the transitive devalue advisory — pre-existing and already fixed on main (which bumped the pnpm-workspace.yaml override to devalue@<5.9.3 → >=5.9.3 <6). This branch was stale and still carried the old override. Merging main in (6163330, conflict-free, matching the repo's Merge remote-tracking branch 'origin/main' convention) pulled the fix. pnpm audit --prod now reports No known vulnerabilities found.

Local verification: pnpm test 910 passed (68 files), pnpm check 0 errors/0 warnings, pnpm audit --prod clean.

Deliberately not done: left DatePicker/`Tree…

---

✅ Opened pull request: imaustink/glyph#73