Preserve wave context (scroll position + chat draft) across new-version reloads#3129
Preserve wave context (scroll position + chat draft) across new-version reloads#3129brookr wants to merge 16 commits into
Conversation
Every release shows the new-version toast; clicking it reloads and, for wave readers who had scrolled up, dumped them at the bottom. The toast now pins the drop nearest viewport center into the URL as ?serialNo=<n> (rows already carry id="drop-<serial>") before reloading, so the existing deep-link restore path — fetch around the serial, scroll it to center, clean the param — puts them back where they were. Readers at the bottom stay in live mode: nothing is pinned within 48px of newest. No new state or persistence: one data attribute on the wave scroll container, a DOM-reading helper, and one call in the toast handler. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: brookr <11095+brookr@users.noreply.github.com>
📝 WalkthroughWalkthroughAdds reload-time wave scroll-position URL pinning, active subwave parent recovery through ChangesWave reload and draft persistence
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant NewVersionToast
participant ScrollPositionHelper
participant WaveDropRows
participant BrowserHistory
NewVersionToast->>ScrollPositionHelper: preserveWaveScrollPositionForReload()
ScrollPositionHelper->>WaveDropRows: locate container and visible serial
WaveDropRows-->>ScrollPositionHelper: return serialNo
ScrollPositionHelper->>BrowserHistory: replaceState with serialNo
NewVersionToast->>BrowserHistory: reload()
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
6529bot general PR review - 587d867Verdict: Needs changes Important
Nice-to-have
Suggested next steps
Prompt for all review comments with AI agents |
6529bot WCAG 2.2 AA analysis - 587d867Verdict: No WCAG findings This PR adds a DOM-reading helper that captures the reader's scroll position (nearest drop serial) and pins it into the URL before a version-toast reload. The change is behavioral/navigational and does not introduce interactive UI, alter accessible names, focus behavior, or semantics. The only markup change is a non-visual From an accessibility standpoint, preserving reading position across reloads is a mild positive for users of screen magnification and cognitive-load-sensitive users, since it avoids disorienting relocation to the feed bottom. No color, contrast, target-size, motion, or live-region concerns are introduced. Note: the correctness concerns already raised in the general lane review (selector scoping, |
6529bot i18n analysis - 587d867Verdict: No i18n findings The changed surface is scroll-position-preservation plumbing: a DOM-reading helper, a data attribute on the scroll container, and one call in the toast handler. No new user-facing copy, accessible names, dates, numbers, or locale-sensitive formatting are introduced.
No i18n regressions in this PR. |
6529bot general PR review - 587d867Verdict: Needs changes The prior 6529bot review at this same head already covered the main issues (id-prefix selector matching nested/non-numeric ids, Important
Nice-to-have
Suggested next steps
Note: coverage here is limited to the included diff/context; not exhaustive. Prompt for all review comments with AI agents |
6529bot crypto security analysis - 587d867Verdict: No security findings This PR reads a numeric drop serial from the DOM and writes it as a Security-relevant checks I verified in the changed code:
The correctness concerns already raised in the general lane (selector scoping to genuine drop rows, |
6529bot GLM Swarm ReviewVerdict: Advisory only This GLM swarm is advisory and complements, not replaces, existing tests and existing reviewbots. Important
Nice-to-have
Testing feedback loop
Partial reviewer outputOne or more internal advisory reviewer slices were unavailable; the synthesis used the remaining reviewer output.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
components/waves/drops/WaveDropsReverseContainer.tsx (1)
75-75: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAttribute is a hardcoded string duplicate of
WAVE_DROPS_SCROLL_CONTAINER_ATTRIBUTE.Value currently matches the helper's constant, but nothing ties them together — renaming the constant in
wave-visible-serial.helpers.tswon't update this JSX literal, silently breaking capture. Sincedata-mobile-bottom-nav-scroll-targeton the same line follows the same hardcoded pattern, this is consistent with existing style, but worth tightening for the new coupling this PR introduces.♻️ Suggested fix using computed attribute spread
+import { WAVE_DROPS_SCROLL_CONTAINER_ATTRIBUTE } from "`@/helpers/waves/wave-visible-serial.helpers`"; + ... <div ref={handleScrollContainerRef} data-mobile-bottom-nav-scroll-target="true" - data-wave-drops-scroll-container="true" + {...{ [WAVE_DROPS_SCROLL_CONTAINER_ATTRIBUTE]: "true" }}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/waves/drops/WaveDropsReverseContainer.tsx` at line 75, The `WaveDropsReverseContainer` JSX is hardcoding a `data-wave-drops-scroll-container` attribute that duplicates `WAVE_DROPS_SCROLL_CONTAINER_ATTRIBUTE`, so it can drift if the helper constant changes. Update the container markup to use the shared constant from `wave-visible-serial.helpers.ts` instead of a literal string, and apply the same pattern to the adjacent `data-mobile-bottom-nav-scroll-target` attribute if it is intended to stay coupled to a shared symbol. Use the existing `WaveDropsReverseContainer` render path to keep the attribute source of truth centralized.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@components/waves/drops/WaveDropsReverseContainer.tsx`:
- Line 75: The `WaveDropsReverseContainer` JSX is hardcoding a
`data-wave-drops-scroll-container` attribute that duplicates
`WAVE_DROPS_SCROLL_CONTAINER_ATTRIBUTE`, so it can drift if the helper constant
changes. Update the container markup to use the shared constant from
`wave-visible-serial.helpers.ts` instead of a literal string, and apply the same
pattern to the adjacent `data-mobile-bottom-nav-scroll-target` attribute if it
is intended to stay coupled to a shared symbol. Use the existing
`WaveDropsReverseContainer` render path to keep the attribute source of truth
centralized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 411d80e3-2ced-49f8-b161-1bb967dbfe65
📒 Files selected for processing (5)
__tests__/components/utils/NewVersionToast.test.tsx__tests__/helpers/wave-visible-serial.helpers.test.tscomponents/utils/NewVersionToast.tsxcomponents/waves/drops/WaveDropsReverseContainer.tsxhelpers/waves/wave-visible-serial.helpers.ts
6529bot responsiveness review - 587d867Verdict: Needs changes Root Cause / Summary The web-desktop, web-mobile, and electron-desktop layouts render cleanly across all routes; the only genuine layout/branch failure is in the native-mobile Capacitor simulation. On Blocking Findings
Non-Blocking Findings
Platform Coverage
Bottom line: Fix/investigate the intermittent Screenshot evidence: native-mobile /meme-lab, native-mobile /rememes, native-mobile /the-memes, native-mobile /, native-mobile /waves, native-mobile /discover, native-mobile /meme-calendar, electron-desktop /the-memes, web-mobile /rememes, electron-desktop /rememes, web-desktop /the-memes, electron-desktop /meme-lab and 19 more in the details. Deterministic responsiveness detailsVerdict: Needs changes
Responsiveness runner summary6529bot Responsiveness SummaryStatus: fail Run Errors
Platform Context
Platform Matrix
Failures
Next.js Overlay Diagnostics
Warnings
Slowest Checks
Prompt for all review comments with AI agents |
Rows now carry data-drop-serial-no (HighlightDropWrapper prop from DropsList) so capture no longer id-prefix-matches descendants; the container attribute is documented as a column-reverse contract, removing the inline-style/getComputedStyle sniffing whose reverse branch never ran in production; the largest-container pick uses an explicit loop (fixes the Sonar reduce-without-initial-value bug); preserveWaveScrollPositionForReload never throws so the reload always proceeds; the JSX attribute references the exported constant. Tests: malformed-nearest-row ordering, nested containers, at-bottom epsilon, DOM-throw safety, and pin-runs-after-toast-param-strip ordering. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: brookr <11095+brookr@users.noreply.github.com>
|
Round 1 pushed, addressing every thread:
Validation: 21/21 unit tests, lint clean, and the full Playwright loop re-run against local dev — scrolled-up reader restored to the same drop (83px from center, param auto-cleaned), at-bottom reader stays in live mode. |
|
Re the responsiveness verdict: the one blocking finding ( Worth noting for the lane's signal quality: the desktop |
6529bot follow-up commit review - 219a8d8Verdict: No new findings The follow-up commit resolves the substantive concerns from the prior reviews. New findings are absent; one minor non-blocking observation below. Resolved since last review
Nice-to-have
Coverage note: assessment is limited to the included diff/context and is not exhaustive. |
Companion to the scroll-position fix: a forced reload (new-version toast) also wiped whatever the user was typing. The primary wave chat composer now autosaves its serialized Lexical editor state to sessionStorage (debounced, per wave), restores it at editor-creation time on the next mount, and clears it once the editor is emptied or the drop is submitted. - Restore via initialConfig.editorState (Lexical reads it once), guarded to the first mount so the post-submit remount can't re-restore. - Scoped to the primary composer (not reply/quote/edit) so a keyed-by-wave draft can't bleed across composers. - Media-safe: drafts containing an image node are skipped (blob URLs die on reload); uploaded attachments live outside the editor state and are intentionally not persisted. - sessionStorage (per-tab, cleared on close) is the right lifetime for a transient draft. Verified live: type -> reload -> exact text restored; empty editor and submit both drop the key. Helper is unit-tested (empty/whitespace/ image/malformed/oversized/round-trip/per-wave isolation). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: brookr <11095+brookr@users.noreply.github.com>
brookr
changed the title
Extended: also persists the in-progress chat messagePer follow-up: a forced reload was still wiping whatever the user had typed. Added draft persistence alongside the scroll-position restore, so the update reload preserves both where you were reading and what you were writing. How (commit 9173b67):
Verified live: typed a message → reloaded → exact text restored; emptying the editor and submitting both drop the key. New helper unit-tested (empty/whitespace/image/malformed/oversized/round-trip/per-wave isolation) — 12 cases. Full composer suite green (the one unrelated Scope note: this is FE-only, no deploy ordering; the benefit (like the scroll fix) starts with the release after the one that ships it, since the running bundle owns the reload. |
6529bot follow-up commit review - 9173b67Verdict: Needs changes The prior follow-up review covered the wave-visible-serial changes. This commit set adds a new, previously-unreviewed feature: draft persistence ( Important
Nice-to-have
Suggested next steps
Coverage note: assessment is limited to the included diff/context and is not exhaustive. Prompt for all review comments with AI agents |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
- Parse the restored draft inside a try/catch at editor creation (function-form initialConfig.editorState). onError re-throws, so an older-version or schema-incompatible draft would otherwise crash the composer mount during the very update-reload this feature serves; it now degrades to an empty editor. Verified live with a poisoned draft: composer mounts empty, zero page errors. - Document the draft lifecycle: autosave derives draftWaveId live so a reply/quote/edit on the same stream-composer instance never writes under the primary wave key (no cross-mode bleed); restore is mount-only. Confirmed the stream composer is one instance whose activeDrop flips (MyStreamWaveChat), which is why the live guard, not a frozen value, is correct. - Drop the dead Number.isFinite check after the digits-only regex (repeat reviewer nit). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: brookr <11095+brookr@users.noreply.github.com>
|
Addressed the draft-persistence review (63096a5):
23/23 helper tests, lint, typecheck clean. |
6529bot follow-up commit review - 63096a5Verdict: No new findings The final commit (63096a5) resolves both Important findings from the prior follow-up review, and the changes are sound. Resolved since last review
Nice-to-have
Coverage note: assessment is limited to the included diff/context and is not exhaustive. |
Third piece of the reload-context work: after the new-version toast reload, the sidebar left the active subwave's parent collapsed for a beat (or longer on slower/larger accounts) because a cold load must await the active-wave -> parent_wave -> subwaves fetch waterfall before it can expand; live SPA nav is instant because that data is cached. The active subwave's parent is now persisted (keyed to the active wave id) and used as an immediate fallback for the sidebar's activeParentWaveId until the live value resolves — so the tree expands and highlights the current subwave right away on reload, then the live value takes over seamlessly. Purely additive: when the live parent is known it is used unchanged, so no path regresses. Wired into both the web and mobile sidebar consumers. Helper logic unit-tested (round-trip, malformed guards, live-vs-hint precedence, wave-id keying so a stale hint never expands the wrong parent). Existing sidebar/consumer suites remain green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: brookr <11095+brookr@users.noreply.github.com>
Also: expand & highlight the active subwave after reloadAdded the subwave piece (fb4300c). After the toast reload the sidebar left the active subwave's parent collapsed — because a cold load must await the Fix: persist the active subwave's parent (keyed to the active wave id) and use it as an immediate fallback for the sidebar's Verification note (honest): the hint-write and resolve logic are unit-tested (8 cases: round-trip, malformed guards, live-vs-hint precedence, wave-id keying so a stale hint never expands the wrong parent), all existing sidebar/consumer suites stay green (64 tests), and I confirmed live that the hint is written to sessionStorage as expected. I could not cleanly A/B the flicker-elimination end-to-end locally today: my local stack runs on custom ports which trips a This is the third context-preservation piece on this PR: scroll position + chat draft + active subwave. |
6529bot follow-up commit review - fb4300cVerdict: No new findings The final commit (fb4300c) adds the active-subwave-parent hint feature — a new, previously-unreviewed surface. It's well-scoped, defensively coded, and the prior draft-persistence findings remain resolved. I reviewed the new code against the storage/lifecycle hazards raised earlier in this PR and found no distinct issues worth blocking. Notes on the new surface (
Nice-to-have
Coverage note: assessment is limited to the included diff/context and is not exhaustive. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
__tests__/helpers/wave-visible-serial.helpers.test.ts (1)
135-147: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider adding a non-tied nested-container case.
This test only covers a tie between the outer and nested rows (identical rects). Adding a variant where the nested row is strictly closer to the container center than the outer's own row would surface the scoping gap noted in
wave-visible-serial.helpers.ts(querySelectorAll matching nested-container rows).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@__tests__/helpers/wave-visible-serial.helpers.test.ts` around lines 135 - 147, Add a non-tied nested-container test in wave-visible-serial.helpers.test.ts to cover the case where a row inside the nested container is strictly closer to the container center than the outer container’s row. Use buildContainer, outer, nested, and captureVisibleWaveDropSerial to verify the helper still scopes to the correct container and does not let querySelectorAll pick the nested-container row.helpers/waves/wave-visible-serial.helpers.ts (1)
85-101: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winRow scan doesn't scope to the container's own rows when containers can nest.
container.querySelectorAll([data-drop-serial-no])matches all descendants, including rows belonging to a nestedWAVE_DROPS_SCROLL_CONTAINER_ATTRIBUTEelement (if one is ever mounted inside the dominant container). The added test ("prefers the outer container when a smaller one is nested inside it") only exercises a tie (identical rects for both rows), which happens to resolve correctly via document-order tie-breaking — it doesn't cover the case where the nested container's row is strictly closer to center than the outer container's own row, which would incorrectly select a serial from a different chat feed.♻️ Suggested scoping fix
for (const row of container.querySelectorAll( `[${DROP_SERIAL_ATTRIBUTE}]` )) { + // Skip rows that belong to a nested scroll container (e.g. an + // embedded/side-panel chat) rather than this one. + if (row.closest(`[${WAVE_DROPS_SCROLL_CONTAINER_ATTRIBUTE}]`) !== container) { + continue; + } const serial = parseRowSerial(row);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@helpers/waves/wave-visible-serial.helpers.ts` around lines 85 - 101, The row selection logic in the visible-serial helper is too broad because `container.querySelectorAll` inside the scan loop can pick up rows from nested scroll containers instead of only the current container’s own rows. Update the scan in the helper that computes the best serial (the loop using `parseRowSerial`, `getBoundingClientRect`, and `bestSerial`) so it filters out descendants belonging to a nested `WAVE_DROPS_SCROLL_CONTAINER_ATTRIBUTE` container and only considers rows directly owned by the active container. Make the scope explicit in this selector/iteration path so the closest row is chosen from the correct chat feed even when containers are nested.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@components/waves/CreateDropContent.tsx`:
- Around line 572-609: Clear the composer state when canceling reply/quote or
edit mode, because only the mode flag is being reset and the existing
editorState can still be autosaved into the primary draft via
CreateDropContent’s draftWaveId effect. Update the cancel handlers
(onCancelReplyQuote and the edit-cancel path) to also reset/clear the editor
state or otherwise prevent stale content from surviving after exit. Make sure
the fix works with CreateDropInput’s mount-only initialEditorStateJson behavior
so reply/edit text is not persisted under the primary wave key.
In `@components/waves/CreateDropInput.tsx`:
- Around line 174-191: The draft restore logic in CreateDropInput’s editorState
initializer currently swallows parseEditorState failures but leaves the bad
stored JSON in place, causing the same broken draft to be retried on every
mount. Update the catch path inside the initialEditorStateJson restore callback
to also clear/remove the persisted draft when parsing fails, alongside the
existing fallback to an empty editor, so the composer stops reloading the
invalid state on subsequent mounts.
---
Nitpick comments:
In `@__tests__/helpers/wave-visible-serial.helpers.test.ts`:
- Around line 135-147: Add a non-tied nested-container test in
wave-visible-serial.helpers.test.ts to cover the case where a row inside the
nested container is strictly closer to the container center than the outer
container’s row. Use buildContainer, outer, nested, and
captureVisibleWaveDropSerial to verify the helper still scopes to the correct
container and does not let querySelectorAll pick the nested-container row.
In `@helpers/waves/wave-visible-serial.helpers.ts`:
- Around line 85-101: The row selection logic in the visible-serial helper is
too broad because `container.querySelectorAll` inside the scan loop can pick up
rows from nested scroll containers instead of only the current container’s own
rows. Update the scan in the helper that computes the best serial (the loop
using `parseRowSerial`, `getBoundingClientRect`, and `bestSerial`) so it filters
out descendants belonging to a nested `WAVE_DROPS_SCROLL_CONTAINER_ATTRIBUTE`
container and only considers rows directly owned by the active container. Make
the scope explicit in this selector/iteration path so the closest row is chosen
from the correct chat feed even when containers are nested.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 826e576f-c31a-4df1-8d0a-72225b97a80f
📒 Files selected for processing (15)
__tests__/components/utils/NewVersionToast.test.tsx__tests__/helpers/active-subwave-hint.helpers.test.ts__tests__/helpers/wave-draft.helpers.test.ts__tests__/helpers/wave-visible-serial.helpers.test.tscomponents/brain/left-sidebar/waves/UnifiedWavesListWaves.tsxcomponents/brain/left-sidebar/web/WebUnifiedWavesListWaves.tsxcomponents/drops/view/DropsList.tsxcomponents/drops/view/HighlightDropWrapper.tsxcomponents/waves/CreateDropContent.tsxcomponents/waves/CreateDropInput.tsxcomponents/waves/drops/WaveDropsReverseContainer.tsxhelpers/waves/active-subwave-hint.helpers.tshelpers/waves/wave-draft.helpers.tshelpers/waves/wave-visible-serial.helpers.tshooks/useActiveSubwaveParentHint.ts
✅ Files skipped from review due to trivial changes (2)
- tests/helpers/wave-draft.helpers.test.ts
- components/drops/view/DropsList.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- components/waves/drops/WaveDropsReverseContainer.tsx
- tests/components/utils/NewVersionToast.test.tsx
Main rewrote CreateDropContent into create-drop-content/ modules, so the draft-persistence feature is re-ported onto the new structure: autosave effect and restore seed live in CreateDropContent, the initial JSON threads through CreateDropLayout, and CreateDropInput parses the restored draft at editor creation. Also addresses CodeRabbit: a draft that fails to parse on restore is now removed from storage instead of being retried on every mount. Signed-off-by: brookr <11095+brookr@users.noreply.github.com>
6529bot follow-up commit review - a668091Verdict: No new findings This is the same head (fb4300c) as the prior follow-up review marker, so the diff since the marker is empty — no new commits to evaluate. The active-subwave-hint feature was already reviewed at this head and found sound; the draft-persistence and scroll-serial findings from earlier commits remain resolved. One note surfaced by re-reading the full context (not a new finding, and not blocking): the The prior nice-to-have (stale top-level hint never cleared, |
6529bot follow-up commit review - 57b5afaVerdict: No new findings This diff since the prior same-lane marker ( All previously-raised Important findings remain resolved:
The prior nice-to-have (stale subwave hint never cleared on top-level nav, Coverage note: assessment is limited to the included diff/context and is not exhaustive. |
6529bot follow-up commit review - c236ac2Verdict: No new findings This is the same head (57b5afa) as the prior follow-up marker, and the diff since the last same-lane marker (587d868) recaps the full PR arc already reviewed commit-by-commit. I re-checked the current state of each surface and the previously-raised findings remain resolved; the only new code relative to 587d868 (draft persistence, subwave hint) was already reviewed in the 9173b67 / 63096a5 / fb4300c follow-ups. One observation worth noting, not previously raised in this exact form: Nice-to-have
Resolved since last review
Coverage note: assessment is limited to the included diff/context since 587d868 and is not exhaustive. |
Main's refactor left CreateDropContent.tsx at 796 lines, four under the 800-line ceiling, so the re-ported draft logic tripped oversized_files. The persistence now lives in create-drop-content/useWaveDraftPersistence and is wired inside CreateDropLayout (which already receives activeDrop, editorState and the refresh key); CreateDropContent matches main exactly. Submit-time draft clearing now keys off the editor refresh-key bump instead of wrapping refreshState. Signed-off-by: brookr <11095+brookr@users.noreply.github.com>
6529bot follow-up commit review - c094f01Verdict: No new findings The final commit (c094f01) is a pure refactor: it lifts the draft-persistence logic out of Resolved / preserved since last review
Nice-to-have
No behavioral regressions introduced by the move; the extracted hook matches the prior inline implementation. |
6529bot follow-up commit review - 06d8353Verdict: No new findings The newest commit (c094f01, "Move draft persistence into a hook") is a pure extraction refactor to satisfy the debt ratchet. It moves the draft-persistence logic out of Notes on the refactor:
The remaining lanes' concerns (wave-visible-serial, active-subwave-hint) are untouched by this commit and their prior resolutions stand. |
6529bot follow-up commit review - f522d6dVerdict: No new findings This commit set (through f522d6d) is dominated by the PATCH 6 refactor that moves draft persistence out of Resolved since last review
Nice-to-have
Coverage note: assessment is limited to the included diff/context and is not exhaustive; the extraction is behavior-preserving as far as the included excerpts show. |
6529bot follow-up commit review - c974fe3Verdict: No new findings This commit set (through the prior marker to Resolved / verified since last review
One minor observation, not blocking: Coverage note: assessment is limited to the included diff/context and is not exhaustive; the omitted 17th file was not reviewable here. |
6529bot follow-up commit review - 49fcd52Verdict: No new findings The primary change since the last same-lane marker ( Resolved since last review
Nice-to-have
Coverage note: assessment is limited to the included diff/context and is not exhaustive; the extraction is behavior-preserving on the evidence here. |
6529bot follow-up commit review - 9f3eb3bVerdict: No new findings This commit set (through the prior same-lane marker at 49fcd52) reorganizes the draft-persistence logic into Resolved / verified since last review
Nice-to-have
Coverage note: assessment is limited to the included diff/context and is not exhaustive; the extraction is behavior-preserving relative to the previously-reviewed logic. |
6529bot follow-up commit review - ab6f95eVerdict: No new findings This commit set is the debt-ratchet refactor (patch 6, Resolved / carried-forward correctly
Nice-to-have
Coverage note: assessment is limited to the included diff/context and is not exhaustive. The moved logic has no direct unit test at the hook level (the helper-level tests in |
The sidebar already remembered which subwave menus were expanded (sessionStorage), but after a full reload only the active wave's parent had its subwaves re-fetched, so manually expanded menus stayed collapsed. A new useLoadPersistedExpandedSubwaves hook eagerly re-fetches subwaves for every remembered parent in both sidebar variants. The new reload e2e also exposed a real gap: a draft restored at editor creation never fires OnChangePlugin, leaving the parent convinced the composer is empty and the Post button disabled until the user types. NotifyInitialEditorStatePlugin pushes the seeded state up once on mount. New coverage: sandbox e2e for type -> reload -> restored-and-sendable; unit tests for useWaveDraftPersistence (debounced save, reply/edit gating, submit-time clear, serialization-failure clear), the failed draft-restore cleanup in CreateDropInput, and the persisted-expansion eager load. Signed-off-by: brookr <11095+brookr@users.noreply.github.com>
|
Pushed b79ad6c expanding this PR's scope per Brook: Subwave menu expansion now survives the new-version reload. The sidebar already persisted which menus were expanded, but after a reload only the active wave's parent had its subwaves re-fetched — manually expanded menus stayed collapsed. Real bug found by the new reload e2e: a restored draft never fired OnChangePlugin, so the Post button stayed disabled until the user typed. New coverage: sandbox e2e (type → reload → restored and sendable, mutation-guarded), unit tests for Note: |
6529bot follow-up commit review - b79ad6cVerdict: No new findings The commit set since the prior same-lane marker ( Resolved / verified since last review
Nice-to-have
Coverage note: assessment is limited to the included diff/context and is not exhaustive; 8 relevant files were omitted after context limits. |
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
__tests__/components/waves/CreateDropInput.draftRestore.test.tsx (1)
20-26: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify plugin mocks by removing
forwardRef.In React 19,
refis treated as a regular prop, soforwardRefis no longer required for components to accept it. You can safely remove thejest.requireActualcall and simplify the mock to return a standard functional component.♻️ Proposed refactor
function createNoopPluginMock() { - const { forwardRef } = jest.requireActual<typeof React>("react"); return { __esModule: true, - default: forwardRef(() => null), + default: () => null, }; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@__tests__/components/waves/CreateDropInput.draftRestore.test.tsx` around lines 20 - 26, Update createNoopPluginMock to remove the jest.requireActual React import and forwardRef usage. Return the default plugin mock as a standard functional component while preserving the existing no-op behavior and module shape.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@__tests__/components/waves/CreateDropInput.draftRestore.test.tsx`:
- Around line 20-26: Update createNoopPluginMock to remove the
jest.requireActual React import and forwardRef usage. Return the default plugin
mock as a standard functional component while preserving the existing no-op
behavior and module shape.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 12898ff2-7e8e-4065-acd8-ab4f9c430368
📒 Files selected for processing (3)
__tests__/components/waves/CreateDropInput.draftRestore.test.tsx__tests__/components/waves/create-drop-content/useWaveDraftPersistence.test.tsx__tests__/hooks/useLoadPersistedExpandedSubwaves.test.tsx
1 participant
Issue
location.reload(). Wave chats boot at the newest message, so anyone reading back in history lands at the bottom and loses their place — on every release, and we release a lot.Fix
id="drop-<serial_no>"(DropsList) — the helper reads the row nearest the scroll container's vertical center.?serialNo=<n>is the long-standing copy-link/notification contract: on load the app fetches around that serial, scrolls it to viewport center, and cleans the param from the URL (MyStreamWaveChat + useWaveDropsSerialScroll). Capture-to-center and restore-to-center are symmetric.Changes
helpers/waves/wave-visible-serial.helpers.ts(new): capture + URL-pinning helpers, documented.components/utils/NewVersionToast.tsx: one call in the refresh handler.components/waves/drops/WaveDropsReverseContainer.tsx: one data attribute marking the wave scroll container.Validation
?showNewVersionToast=trueoverride to render the real toast and click the real handler):serialNo=6826→ after reload the same drop was visible 83px from container center and the param was cleaned from the URL.Risk
Deployment
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes