WIP: Pilot safe OpenCode pull request reviews

lambadalambda commented

2026-08-03 08:27:02 +00:00

Owner

Summary

track the isolated OpenCode review bot pilot
use this pull request for the first live requestable review

Testing

pending bot deployment and live review

## Summary - track the isolated OpenCode review bot pilot - use this pull request for the first live requestable review ## Testing - pending bot deployment and live review

lambadalambda added 20 commits

2026-08-03 08:27:02 +00:00

fix: improve large thread navigation 09ef3e9f99

fix: remove thread ending seam c3fbbb6339

test: stabilize stale thread navigation 1b27e047ef

fix: render emoji in reply previews da499fc9bd

fix: reliably catch up timeline streams a8895869f0

docs: record custom emoji audit 0e5de617a0

fix: reconcile replies into open threads 5c47301425

fix: reveal thread reply composers 1287c77363

feat: add compact media previews 780a80759f

fix: render custom emoji across app surfaces ebc0e7751a

feat: expand compact image previews 1c8b63993e

feat: add fit-images timeline setting bee9b01388

fix: insert replies to thread ancestors e287f6e5d9

feat: share fit-images setting with threads 1901a5780d

fix: reconcile post counts from notifications 922ec5aff6

feat: show and inherit post visibility 651e256edd

fix: align visibility with post timestamp 00b40b67f8

feat: align notifications page with handoff 2f886e44f0

fix: use available lightbox height 87167d89dc

docs: track safe OpenCode review pilot

opencode/review OpenCode execution completed

Details

93280f3bc3

lambadalambda commented

2026-08-03 11:03:52 +00:00

Author

Owner

/oc review

opencode-bot commented

2026-08-03 11:04:06 +00:00

Collaborator

OpenCode Review

Execution completed for lambadalambda/pleromanet at 1a043729493a (job #6).

Findings

Medium

Unbounded growth of status-engagement tracking maps — src/routes/app/[...path]/+page.svelte
The new revision-tracking structures (statusEngagementSnapshots, statusEngagementNotificationRevisions, statusEngagementNotificationArrivals, notificationEngagementSignatures, seenNotificationEngagementSignatures) are keyed by status/notification IDs and never pruned. invalidateStatusActionRequests resets them only on session/route invalidation, so a long-running session that views many posts will accumulate entries indefinitely. Consider evicting entries for status IDs no longer present in any loaded surface, or limiting to the most-recent N targets. This is the most concrete memory-growth concern introduced by the count-reconciliation feature.

Floating image preview is repositioned on every scroll/resize without throttling — src/lib/rebuild/CompactMediaPreview.svelte
positionFullPhoto is bound to window.scroll (capture), window.resize, visualViewport.resize, and visualViewport.scroll, plus image onload. Each invocation performs getBoundingClientRect() reads, multiple await tick() calls, and updates fullPhotoStyle. There is no throttle, debounce, or requestAnimationFrame coalescing. On lower-end devices and long hover sessions this can introduce jank, especially because the effect tears down and re-creates listeners on every fullPhoto/fullPhotoAnchor change. Consider an rAF-batched reposition and keeping listeners attached for the lifetime of the open preview (they are already gated by the open state).

Low

Misleading time-bucket labels in NotificationsPage — src/lib/rebuild/NotificationsPage.svelte
bucketFor maps ages 1–6 h to "Today", 6–48 h to "Yesterday", and 2–7 days to "This week". The labels do not match the implied semantics and will be confusing for users (e.g., a 7-hour-old notification shows under "Yesterday"). Either rename the buckets (e.g., "Recent", "Today", "Earlier this week") or compute them against calendar days.

Redundant condition in WebSocket onerror handler — src/lib/pleroma/streaming.ts

socket.onerror = (event) => {
    if (closed) return;
    if (!closed) onError?.(event);
};

The inner if (!closed) is unreachable because closed cannot change synchronously between the two checks. Collapse to a single onError?.(event); after the early return.

role="note" is non-standard — src/lib/rebuild/CompactMediaPreview.svelte

<div class="compact-media-preview compact-media-hidden" role="note">Sensitive media</div>

note is not in the WAI-ARIA 1.2 vocabulary; some screen readers will ignore it. Use a standard role such as status, or omit role and rely on the visible text.

Notification actionable row loses pointer affordance — src/app.css, src/lib/rebuild/NotifRow.svelte
The old rule .notif-row.actionable { cursor: pointer; } was removed and the actionable surface is now a real <button class="notif-row-text">. Browser default cursor for <button> is default, so the row no longer signals clickability. Add cursor: pointer to .notif-row-text:is(button) (or rely on the existing .notif-quote-open:is(button) pattern).

Two distinct buttons open the same notification — src/lib/rebuild/NotifRow.svelte
For actionable rows with a post, both .notif-row-text (the whole row) and .notif-quote-open (the excerpt) call onOpen?.(n). This creates redundant focusable targets and a duplicated "open" action in the a11y tree. Either expose a single activator or mark the secondary one tabindex="-1"/aria-hidden.

Double notification load after clear — src/routes/app/[...path]/+page.svelte
clearNotifications first runs loadNotifications(session, { replace: true }) and then, in finally, unconditionally calls loadNotifications(session, { background: true }). The replace load already fetches authoritative state right after the clear; the second call is a duplicate network request in the common success path. Gate the background follow-up on a real signal (e.g., the replacement stream has opened with no fresh data) or skip it when the replace succeeded.

CompactMediaPreview portal lifecycle is fragile — src/lib/rebuild/CompactMediaPreview.svelte
The portal action appends node to document.body and removes it on destroy. Svelte also tries to remove the node from its (now-moved) parent during {#if fullPhoto} teardown. The current ordering works today, but it depends on Svelte calling the action destroy before attempting DOM removal. Adding a comment or guarding the destroy to only remove when still attached would future-proof this against Svelte internals changes.

aria-label on visibility span may double-read — src/lib/rebuild/PostVisibility.svelte
The outer <span> carries aria-label while wrapping an Icon plus a visible <span>{details.label}</span>. If Icon exposes its own accessible name, AT users may hear the label twice. Ensure the inner Icon is aria-hidden="true" (the component is not shown in the patch).

Informational

timeline-catch-up.ts and reconcileTimelineCatchUp are intricate but accompanied by a substantial, focused Playwright suite covering multi-page catch-up, concurrent streamed posts, empty-timeline recovery, route retention, and stale local vs. federated races. The behavior matches the tests; future maintainers will benefit from inline comments explaining the initialDataIds/initialQueuedIds overlap algorithm.
Notification clearing correctly bumps notificationStreamGeneration, gates applyStreamedNotification on both stream generation and notificationClearState === 'clearing', and re-checks isCurrentSessionRequest after each await. The in-flight-notification and delayed-stream tests exercise the important races.
statusReplyTargetId/matchesStatusReplyTarget correctly route boosted-source identity through thread reconciliation, ancestor replies, and reply-target expansion. Indexing byId by both id and statusReplyTargetId is safe under the assumption that source IDs are unique within a thread context.
compactExcerpt shortcode-boundary logic is exported and unit-tested, and is reused for chat excerpts after send — a nice consolidation of the prior chatExcerpt helper.
Custom-emoji propagation is consistently applied to the audit-identified surfaces (notifications, CW summaries, pinned posts, chat rows, mention suggestions, lightbox attribution, profile context, settings preview), and hidden-emoji/no-request assertions back the privacy claims.
openPleromaTimelineStream changes (open-timeout, detach-on-close, idempotent close) are well-covered by the rewritten client tests, including the new "socket never opens" replacement case.

Overall

A large, well-structured patch that delivers a coherent set of features (fit-images, compact media previews with floating full-image view, notifications-page redesign, visibility badges, timeline stream catch-up, thread loading/scroll/restoration, count reconciliation, custom-emoji audit fixes, clear-all with generation-safe catch-up). Test coverage is thorough and traces cleanly to the per-issue acceptance criteria. No security, data-integrity, or correctness defects were identified. The most actionable items are the unbounded engagement-snapshot maps (Medium), the unthrottled floating-preview positioning (Medium), and a handful of low-severity UX/a11y/style nits listed above.

## OpenCode Review _Execution completed for `lambadalambda/pleromanet` at `1a043729493a` (job #6)._ ## Findings ### Medium **Unbounded growth of status-engagement tracking maps** — `src/routes/app/[...path]/+page.svelte` The new revision-tracking structures (`statusEngagementSnapshots`, `statusEngagementNotificationRevisions`, `statusEngagementNotificationArrivals`, `notificationEngagementSignatures`, `seenNotificationEngagementSignatures`) are keyed by status/notification IDs and never pruned. `invalidateStatusActionRequests` resets them only on session/route invalidation, so a long-running session that views many posts will accumulate entries indefinitely. Consider evicting entries for status IDs no longer present in any loaded surface, or limiting to the most-recent N targets. This is the most concrete memory-growth concern introduced by the count-reconciliation feature. **Floating image preview is repositioned on every scroll/resize without throttling** — `src/lib/rebuild/CompactMediaPreview.svelte` `positionFullPhoto` is bound to `window.scroll` (capture), `window.resize`, `visualViewport.resize`, and `visualViewport.scroll`, plus image `onload`. Each invocation performs `getBoundingClientRect()` reads, multiple `await tick()` calls, and updates `fullPhotoStyle`. There is no throttle, debounce, or `requestAnimationFrame` coalescing. On lower-end devices and long hover sessions this can introduce jank, especially because the effect tears down and re-creates listeners on every `fullPhoto`/`fullPhotoAnchor` change. Consider an `rAF`-batched reposition and keeping listeners attached for the lifetime of the open preview (they are already gated by the open state). ### Low **Misleading time-bucket labels in NotificationsPage** — `src/lib/rebuild/NotificationsPage.svelte` `bucketFor` maps ages 1–6 h to "Today", 6–48 h to "Yesterday", and 2–7 days to "This week". The labels do not match the implied semantics and will be confusing for users (e.g., a 7-hour-old notification shows under "Yesterday"). Either rename the buckets (e.g., "Recent", "Today", "Earlier this week") or compute them against calendar days. **Redundant condition in WebSocket onerror handler** — `src/lib/pleroma/streaming.ts` ```js socket.onerror = (event) => { if (closed) return; if (!closed) onError?.(event); }; ``` The inner `if (!closed)` is unreachable because `closed` cannot change synchronously between the two checks. Collapse to a single `onError?.(event);` after the early return. **`role="note"` is non-standard** — `src/lib/rebuild/CompactMediaPreview.svelte` ```svelte <div class="compact-media-preview compact-media-hidden" role="note">Sensitive media</div> ``` `note` is not in the WAI-ARIA 1.2 vocabulary; some screen readers will ignore it. Use a standard role such as `status`, or omit `role` and rely on the visible text. **Notification actionable row loses pointer affordance** — `src/app.css`, `src/lib/rebuild/NotifRow.svelte` The old rule `.notif-row.actionable { cursor: pointer; }` was removed and the actionable surface is now a real `<button class="notif-row-text">`. Browser default cursor for `<button>` is `default`, so the row no longer signals clickability. Add `cursor: pointer` to `.notif-row-text:is(button)` (or rely on the existing `.notif-quote-open:is(button)` pattern). **Two distinct buttons open the same notification** — `src/lib/rebuild/NotifRow.svelte` For actionable rows with a post, both `.notif-row-text` (the whole row) and `.notif-quote-open` (the excerpt) call `onOpen?.(n)`. This creates redundant focusable targets and a duplicated "open" action in the a11y tree. Either expose a single activator or mark the secondary one `tabindex="-1"`/`aria-hidden`. **Double notification load after clear** — `src/routes/app/[...path]/+page.svelte` `clearNotifications` first runs `loadNotifications(session, { replace: true })` and then, in `finally`, unconditionally calls `loadNotifications(session, { background: true })`. The replace load already fetches authoritative state right after the clear; the second call is a duplicate network request in the common success path. Gate the background follow-up on a real signal (e.g., the replacement stream has opened with no fresh data) or skip it when the replace succeeded. **`CompactMediaPreview` portal lifecycle is fragile** — `src/lib/rebuild/CompactMediaPreview.svelte` The `portal` action appends `node` to `document.body` and removes it on destroy. Svelte also tries to remove the node from its (now-moved) parent during `{#if fullPhoto}` teardown. The current ordering works today, but it depends on Svelte calling the action `destroy` before attempting DOM removal. Adding a comment or guarding the destroy to only remove when still attached would future-proof this against Svelte internals changes. **`aria-label` on visibility span may double-read** — `src/lib/rebuild/PostVisibility.svelte` The outer `<span>` carries `aria-label` while wrapping an `Icon` plus a visible `<span>{details.label}</span>`. If `Icon` exposes its own accessible name, AT users may hear the label twice. Ensure the inner `Icon` is `aria-hidden="true"` (the component is not shown in the patch). ### Informational - **`timeline-catch-up.ts` and `reconcileTimelineCatchUp`** are intricate but accompanied by a substantial, focused Playwright suite covering multi-page catch-up, concurrent streamed posts, empty-timeline recovery, route retention, and stale local vs. federated races. The behavior matches the tests; future maintainers will benefit from inline comments explaining the `initialDataIds`/`initialQueuedIds` overlap algorithm. - **Notification clearing** correctly bumps `notificationStreamGeneration`, gates `applyStreamedNotification` on both stream generation and `notificationClearState === 'clearing'`, and re-checks `isCurrentSessionRequest` after each `await`. The in-flight-notification and delayed-stream tests exercise the important races. - **`statusReplyTargetId`/`matchesStatusReplyTarget`** correctly route boosted-source identity through thread reconciliation, ancestor replies, and reply-target expansion. Indexing `byId` by both `id` and `statusReplyTargetId` is safe under the assumption that source IDs are unique within a thread context. - **`compactExcerpt` shortcode-boundary logic** is exported and unit-tested, and is reused for chat excerpts after send — a nice consolidation of the prior `chatExcerpt` helper. - **Custom-emoji propagation** is consistently applied to the audit-identified surfaces (notifications, CW summaries, pinned posts, chat rows, mention suggestions, lightbox attribution, profile context, settings preview), and hidden-emoji/no-request assertions back the privacy claims. - **`openPleromaTimelineStream` changes** (open-timeout, detach-on-close, idempotent close) are well-covered by the rewritten client tests, including the new "socket never opens" replacement case. ## Overall A large, well-structured patch that delivers a coherent set of features (fit-images, compact media previews with floating full-image view, notifications-page redesign, visibility badges, timeline stream catch-up, thread loading/scroll/restoration, count reconciliation, custom-emoji audit fixes, clear-all with generation-safe catch-up). Test coverage is thorough and traces cleanly to the per-issue acceptance criteria. No security, data-integrity, or correctness defects were identified. The most actionable items are the unbounded engagement-snapshot maps (Medium), the unthrottled floating-preview positioning (Medium), and a handful of low-severity UX/a11y/style nits listed above.

lambadalambda commented

2026-08-03 11:09:26 +00:00

Author

Owner

/oc rerun

lambadalambda commented

2026-08-03 11:11:57 +00:00

Author

Owner

/oc rerun

lambadalambda commented

2026-08-03 11:15:25 +00:00

Author

Owner

/oc rerun

lambadalambda commented

2026-08-03 11:17:54 +00:00

Author

Owner

/oc rerun

lambadalambda commented

2026-08-03 11:35:09 +00:00

Author

Owner

/oc rerun

lambadalambda added 1 commit

2026-08-03 11:49:45 +00:00

docs: archive OpenCode review pilot

opencode/review OpenCode execution completed

Details

1a04372949

lambadalambda commented

2026-08-03 11:49:53 +00:00

Author

Owner

/oc rerun

opencode/review OpenCode execution completed

Details

This pull request is marked as a work in progress.

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin issue/109-opencode-review-bot-pilot:issue/109-opencode-review-bot-pilot

git switch issue/109-opencode-review-bot-pilot

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch master

git merge --no-ff issue/109-opencode-review-bot-pilot

git switch issue/109-opencode-review-bot-pilot

git rebase master

git switch master

git merge --ff-only issue/109-opencode-review-bot-pilot

git switch issue/109-opencode-review-bot-pilot

git rebase master

git switch master

git merge --no-ff issue/109-opencode-review-bot-pilot

git switch master

git merge --squash issue/109-opencode-review-bot-pilot

git switch master

git merge --ff-only issue/109-opencode-review-bot-pilot

git switch master

git merge issue/109-opencode-review-bot-pilot

git push origin master

Rows
Columns

WIP: Pilot safe OpenCode pull request reviews #1

Summary

Testing

OpenCode Review

Findings

Medium

Low

Informational

Overall

Checkout

Merge