0d7938aff5
Post-commit review follow-ups for the batch-3 feed work.
Frontend — VirtualFeed: the guarded setOptions keyed only on (count, scrollMargin),
so a rotation (or the first-paint 0->real container width) changed colWidth / card
heights without invalidating the cached measurements, leaving getTotalSize and the
scrollbar stale until each row scrolled back through the window. Track appliedWidth
and call virtualizer.measure() on a width delta so heights re-estimate at the new
width (rendered rows re-measure immediately via their ResizeObserver). Covers list +
grid and the first-paint 120px-estimate case.
Backend — social.rs: the fresh like/comment count SELECT used `.await?`, so a
transient DB failure would 500 the request after the like/comment had already
committed. Make the count + broadcast best-effort (if let Ok { ... }); the mutation
succeeds regardless and the next event / pull-to-refresh reconciles the count.
Docs — FOLLOWUPS: record the review findings left as-is (grid-prepend tile reflow,
filtered-grid auto-load which is pre-existing, comment-delete stale count, and the
last-write-wins count ordering note).
Verified: svelte-check 0 errors, cargo build clean.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
138 lines
9.1 KiB
Markdown
138 lines
9.1 KiB
Markdown
# Follow-ups
|
|
|
|
Tracked work that was deferred during the multi-round UI/UX review pass.
|
|
Each item has a clear acceptance criterion so a future pass can land it
|
|
without re-deriving the context.
|
|
|
|
## A11y — assistive-tech containment inside modals
|
|
|
|
**Problem.** Open modals (LightboxModal, ConfirmSheet, Modal, OnboardingGuide,
|
|
join PIN modal, account data-mode sheet, export HTML guide) trap keyboard Tab
|
|
via `focusTrap`, but VoiceOver rotor / TalkBack arrow-key navigation can still
|
|
escape into the page content behind the dialog. Screen-reader users hear the
|
|
wrong context.
|
|
|
|
**Why deferred.** A naive sibling-walk that sets `inert` on direct children of
|
|
the modal's parent silences the global `<Toaster>` (`aria-live="polite"` region
|
|
mounted in `+layout.svelte`) and the `<BottomNav>` while a modal is open —
|
|
breaking toast announcements and the visible nav state. SvelteKit has no
|
|
built-in portal mechanism, so dialogs render inside the route tree alongside
|
|
the Toaster.
|
|
|
|
**Acceptance criterion.** With any modal open:
|
|
- VoiceOver rotor (iOS Safari) and TalkBack swipe navigation (Android Chrome)
|
|
cannot leave the dialog subtree.
|
|
- Toasts that fire while a modal is open are still announced.
|
|
- Nested modals (e.g. ConfirmSheet opened from inside ContextSheet) maintain
|
|
correct containment when the inner closes.
|
|
|
|
**Sketch of an approach.** One of:
|
|
1. **Portal pattern.** Render dialogs into a dedicated `<div id="modal-root">`
|
|
that's a sibling of the main app root in `app.html`. `focusTrap` then sets
|
|
`inert` on the main root, leaving the modal root and the toast region (also
|
|
moved to its own portal root) untouched.
|
|
2. **Opt-out marker.** Walk siblings and inert them, but skip any node carrying
|
|
a `data-modal-passthrough` attribute. Mark `<Toaster>` with it. Document
|
|
the contract.
|
|
3. **Stack-aware containment.** Maintain a module-level stack of open dialog
|
|
nodes; the topmost owns the inert state, popped dialogs restore the
|
|
previous layer. Avoids the nested-modal restoration bug.
|
|
|
|
Approach 1 is the cleanest long-term but the highest blast radius. Approach 2
|
|
is the smallest patch.
|
|
|
|
**Files to touch.**
|
|
- [frontend/src/lib/actions/focus-trap.ts](frontend/src/lib/actions/focus-trap.ts) — add inert logic
|
|
- [frontend/src/lib/components/Toaster.svelte](frontend/src/lib/components/Toaster.svelte) — add passthrough marker (if approach 2) or move to a portal (if approach 1)
|
|
- [frontend/src/app.html](frontend/src/app.html) — add `<div id="modal-root">` (if approach 1)
|
|
|
|
## Feed — DOM-windowing virtualization (IMPLEMENTED — residual validation owed)
|
|
|
|
**Status.** Implemented in [frontend/src/lib/components/VirtualFeed.svelte](frontend/src/lib/components/VirtualFeed.svelte)
|
|
using `@tanstack/svelte-virtual`'s `createWindowVirtualizer`. Both the **list**
|
|
view (dynamic `measureElement` heights, keyed by upload id with `anchorTo:'start'`
|
|
so an SSE prepend doesn't yank a scrolled-down reader) and the **grid** view
|
|
(three measured square tiles per row) now keep only the on-screen window (+overscan)
|
|
in the DOM instead of one node per upload. The window virtualizer scrolls the
|
|
document, so the sticky header, pull-to-refresh, the infinite-scroll sentinel and
|
|
the bottom nav are untouched. The earlier `content-visibility` band-aid was removed
|
|
from `FeedListCard` (it interferes with real-height measurement), and the old
|
|
`FeedGrid.svelte` was deleted (its sole consumer migrated to `VirtualFeed`).
|
|
|
|
**Verified.** `svelte-check` 0 errors, production build clean. The integration
|
|
follows the library's documented window-virtualizer contract (confirmed against
|
|
`virtual-core` source: item `start` includes `scrollMargin`, `getTotalSize()`
|
|
excludes it; the SSR path is guarded by `getScrollElement()` returning null).
|
|
|
|
**Residual validation owed (needs the running app — could not be done headless).**
|
|
- Manual scroll testing on a ~1000-item event: confirm no jank, correct scrollbar
|
|
size, and that an SSE `new-upload` / `feed-delta` prepend while scrolled down does
|
|
not jump the viewport (the `anchorTo:'start'` + id-key path).
|
|
- `scrollMargin` re-measure when grid filter chips change the header height (handled
|
|
reactively via the `uploads`-length-driven effect, but unverified visually).
|
|
- A new e2e spec that scrolls far down, likes an item via SSE, and asserts scroll
|
|
position is retained — the existing suite only asserts a single card is visible,
|
|
so it cannot catch a scroll regression.
|
|
|
|
**Files.**
|
|
- [frontend/src/lib/components/VirtualFeed.svelte](frontend/src/lib/components/VirtualFeed.svelte) — new windowing component (list + grid)
|
|
- [frontend/src/routes/feed/+page.svelte](frontend/src/routes/feed/+page.svelte) — renders `VirtualFeed` for both views
|
|
- [frontend/src/lib/components/FeedListCard.svelte](frontend/src/lib/components/FeedListCard.svelte) — `content-visibility` removed
|
|
|
|
**Known limitations (surfaced in the post-commit review, left as-is — low impact).**
|
|
- **Grid prepend reflows tiles.** Grid rows are index-keyed and pack 3 tiles each, so
|
|
an SSE `new-upload` shifts every tile by one position; `anchorTo:'start'` can only
|
|
anchor a scrolled grid reader when the prepend crosses a 3-item boundary (it adds a
|
|
*row*). List view is unaffected (one id-keyed row per upload). A fix would key rows
|
|
by the first tile's id and accept partial-row churn; not worth it for the rarer
|
|
"new upload while browsing the grid" case.
|
|
- **Filtered grid can auto-load the whole feed.** When a grid filter matches few items
|
|
the `VirtualFeed` is short, so the infinite-scroll sentinel sits in-viewport and
|
|
`loadMore()` fires until `nextCursor` is null — pulling all pages to widen the
|
|
client-side search. This is pre-existing (the old `FeedGrid` had the same shape, and
|
|
the empty-filter copy even says "scrolle weiter"), not a virtualization regression.
|
|
If undesired, gate auto-load to list view or to actual user scroll.
|
|
|
|
## Feed — comment deletion leaves a stale live count
|
|
|
|
**Problem.** `add_comment` now broadcasts a fresh `comment_count` so feed clients patch
|
|
the card in place, but `delete_comment` and `host_delete_comment`
|
|
([backend/src/handlers/social.rs](backend/src/handlers/social.rs),
|
|
[host.rs](backend/src/handlers/host.rs)) soft-delete without broadcasting any
|
|
count/event. So a deletion leaves the count too high on every client until a full
|
|
refetch (pull-to-refresh or an unrelated `upload-processed` merge). `toggle_like`
|
|
already broadcasts on both add and remove, so likes are fine — the gap is
|
|
comments-on-delete. Pre-existing, but the in-place-patch scheme makes it observable.
|
|
|
|
**Fix.** Emit a `new-comment` (or a `comment-deleted`) event carrying the refreshed
|
|
`comment_count` from both delete paths, the same best-effort way `add_comment` does;
|
|
the frontend `patchCount(..., 'comment_count')` handler already consumes it.
|
|
|
|
**Note — count ordering.** The broadcast count is read just after the (auto-committed)
|
|
mutation, not inside it, so under concurrent likes/comments on one upload the SSE
|
|
messages are last-write-wins. The frontend *replaces* (not increments) the count, so
|
|
steady state is correct and self-healing; only document this if strict per-event
|
|
ordering is ever required (then compute the count in-tx with a monotonic sequence).
|
|
|
|
## Feed — per-image exact CLS reservation
|
|
|
|
**Problem.** `FeedListCard` now reserves a default `aspect-[4/5]` box for photos so
|
|
the card doesn't collapse to height 0 and reflow as images stream in (matching
|
|
`Skeleton`). But no image dimensions are stored anywhere (not on `FeedUpload`, the
|
|
`upload` table, or any migration), so the box is a uniform guess that crops to fit —
|
|
the original is one tap away in the lightbox.
|
|
|
|
**Acceptance criterion.** Extract image width/height during the compression worker,
|
|
store them on `upload`, expose them on `FeedUpload`, and have the card reserve the
|
|
*true* aspect ratio (no crop, zero shift).
|
|
|
|
**Files to touch.**
|
|
- backend: `services/compression.rs`, `models/upload.rs`, `handlers/feed.rs`, a migration
|
|
- [frontend/src/lib/types.ts](frontend/src/lib/types.ts), [FeedListCard.svelte](frontend/src/lib/components/FeedListCard.svelte)
|
|
|
|
## Smaller nits, optional
|
|
|
|
- **Auto-submit on retried 4th digit.** [recover/+page.svelte](frontend/src/routes/recover/+page.svelte), [join/+page.svelte](frontend/src/routes/join/+page.svelte) — after a wrong PIN, deleting one digit and retyping triggers an immediate submit. Backend's 3-attempts/15-min lockout makes this safe; could feel hair-trigger after a typo. Consider gating the second auto-submit per input session behind an explicit button press.
|
|
- **Onboarding pip tap target on the vertical axis.** [OnboardingGuide.svelte](frontend/src/lib/components/OnboardingGuide.svelte) — current `p-2.5` yields ~26 px height, meets WCAG 2.2 AA (≥24 px) but below iOS HIG / Material's 44 / 48 dp recommendation. Bumping to `p-3` is the easy improvement; further increases start crowding the row.
|
|
- **Migrate bespoke focus-trapped dialogs to `<Modal>`.** Join PIN modal, OnboardingGuide, LightboxModal, HTML guide, account data-mode sheet — all currently roll their own shell with `focusTrap`. They're correct, just not using the canonical primitive. Migrate when `<Modal>` gains features (e.g. the inert work above) you'd want everywhere.
|