Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds announcement-wave support: new settings field and helper, announcement resolution in waves hook (with conditional fetch), UI sections for announcement waves with adjusted pin visibility, pin-server eligibility updates, OpenAPI additions for wave selections, and corresponding tests and mocks. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Sidebar UI
participant Hook as useWavesList
participant Settings as SeizeSettingsContext
participant API as useWaveById
UI->>Settings: read announcements_wave_id & isAnnouncementsWave(waveId)
UI->>Hook: request combined waves
Hook->>Settings: normalize announcements_wave_id
Hook->>Hook: filter main/pinned lists (exclude announcements)
alt announcement id present and missing from lists
Hook->>API: conditional fetch announcement (enabled)
API-->>Hook: returns announcement wave
end
Hook-->>UI: combined waves (announcement inserted + sorted others)
UI->>UI: render Announcement / Pinned / Regular sections with adjusted showPin
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 101e3a8fa6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
contexts/SeizeSettingsContext.tsx (1)
21-23: Addannouncements_wave_idtoTempApiSeizeSettingstype.The
announcements_wave_idfield is initialized in state (line 58) and used throughout, but it's not declared in theTempApiSeizeSettingstype definition (lines 21-23). TypeScript may allow this due to the spread fromApiSeizeSettings, but for type safety and documentation, explicitly add it.♻️ Proposed fix
type TempApiSeizeSettings = ApiSeizeSettings & { curation_wave_id: string | null; + announcements_wave_id: string | null; };Also applies to: 58-58
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contexts/SeizeSettingsContext.tsx` around lines 21 - 23, The TempApiSeizeSettings type is missing announcements_wave_id despite that field being initialized and used; update the TempApiSeizeSettings type declaration (which currently adds curation_wave_id) to also include announcements_wave_id: string | null so the state shape matches the type and TypeScript enforces it alongside the existing curation_wave_id declaration.components/brain/left-sidebar/waves/UnifiedWavesListWaves.tsx (1)
215-215: Redundant check, but harmless and self-documenting.Since
pinnedWavesis already filtered to exclude announcement waves (line 117-119),wave.isAnnouncementwill always befalsehere. The explicit!wave.isAnnouncementcheck is technically redundant but clearly communicates intent.🔧 Optional simplification
If you prefer brevity over explicitness:
- showPin={!hidePin && !wave.isAnnouncement} + showPin={!hidePin}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/brain/left-sidebar/waves/UnifiedWavesListWaves.tsx` at line 215, The prop expression showPin={!hidePin && !wave.isAnnouncement} is redundant because pinnedWaves is already filtered to exclude announcement waves; remove the needless !wave.isAnnouncement check and set showPin based only on hidePin (i.e., showPin={!hidePin}) to simplify code while preserving behavior—update the component rendering that uses showPin and ensure no other logic depends on wave.isAnnouncement at that spot (reference: showPin, hidePin, wave.isAnnouncement, pinnedWaves).components/brain/left-sidebar/web/WebUnifiedWavesListWaves.tsx (2)
227-229: Redundant check (harmless).Since waves in
pinnedWavesare already guaranteed to be non-announcements (filtered at line 90), the!wave.isAnnouncementcheck is always true here. The check is defensive and doesn't cause issues, so this is a minor observation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/brain/left-sidebar/web/WebUnifiedWavesListWaves.tsx` around lines 227 - 229, The showPin expression in WebUnifiedWavesListWaves currently includes a redundant !wave.isAnnouncement check because pinnedWaves are already filtered to exclude announcements; simplify the prop to showPin={!hidePin && !isCollapsed} by removing the unnecessary !wave.isAnnouncement clause (affecting the showPin prop where pinnedWaves, hidePin, isCollapsed, and wave.isAnnouncement are used).
284-284: Same redundant check as noted above.Waves in
regularWavesare non-announcements by construction. The!wave.isAnnouncementcheck is always true here. Consistent with line 228 but similarly redundant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/brain/left-sidebar/web/WebUnifiedWavesListWaves.tsx` at line 284, Redundant boolean check: when rendering items from regularWaves (which are guaranteed non-announcements), remove the unnecessary "!wave.isAnnouncement" check and set showPin based only on hidePin (e.g., change showPin={!hidePin && !wave.isAnnouncement} to showPin={!hidePin}) in the component where regularWaves are mapped (reference: showPin prop in WebUnifiedWavesListWaves render).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contexts/wave/hooks/useEnhancedWavesListCore.ts`:
- Around line 27-30: The EnhancedApiWave.isAnnouncement flag is populated
explicitly by useWavesList() but left unset in useDmWavesList(), relying on a
fallback; update useDmWavesList() to explicitly set isAnnouncement: false on
each wave it returns to match the pattern in useWavesList() and keep the data
shape consistent across sources (reference EnhancedApiWave, useWavesList,
useDmWavesList).
---
Nitpick comments:
In `@components/brain/left-sidebar/waves/UnifiedWavesListWaves.tsx`:
- Line 215: The prop expression showPin={!hidePin && !wave.isAnnouncement} is
redundant because pinnedWaves is already filtered to exclude announcement waves;
remove the needless !wave.isAnnouncement check and set showPin based only on
hidePin (i.e., showPin={!hidePin}) to simplify code while preserving
behavior—update the component rendering that uses showPin and ensure no other
logic depends on wave.isAnnouncement at that spot (reference: showPin, hidePin,
wave.isAnnouncement, pinnedWaves).
In `@components/brain/left-sidebar/web/WebUnifiedWavesListWaves.tsx`:
- Around line 227-229: The showPin expression in WebUnifiedWavesListWaves
currently includes a redundant !wave.isAnnouncement check because pinnedWaves
are already filtered to exclude announcements; simplify the prop to
showPin={!hidePin && !isCollapsed} by removing the unnecessary
!wave.isAnnouncement clause (affecting the showPin prop where pinnedWaves,
hidePin, isCollapsed, and wave.isAnnouncement are used).
- Line 284: Redundant boolean check: when rendering items from regularWaves
(which are guaranteed non-announcements), remove the unnecessary
"!wave.isAnnouncement" check and set showPin based only on hidePin (e.g., change
showPin={!hidePin && !wave.isAnnouncement} to showPin={!hidePin}) in the
component where regularWaves are mapped (reference: showPin prop in
WebUnifiedWavesListWaves render).
In `@contexts/SeizeSettingsContext.tsx`:
- Around line 21-23: The TempApiSeizeSettings type is missing
announcements_wave_id despite that field being initialized and used; update the
TempApiSeizeSettings type declaration (which currently adds curation_wave_id) to
also include announcements_wave_id: string | null so the state shape matches the
type and TypeScript enforces it alongside the existing curation_wave_id
declaration.
🪄 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: 3e649c59-a63d-4e24-95b4-e4ef2c083306
⛔ Files ignored due to path filters (10)
generated/models/ApiDrop.tsis excluded by!**/generated/**generated/models/ApiDropWithoutWave.tsis excluded by!**/generated/**generated/models/ApiSeizeSettings.tsis excluded by!**/generated/**generated/models/ApiWave.tsis excluded by!**/generated/**generated/models/ApiWaveMin.tsis excluded by!**/generated/**generated/models/ApiWaveSelection.tsis excluded by!**/generated/**generated/models/ApiWaveSelectionDropRequest.tsis excluded by!**/generated/**generated/models/ApiWaveSelectionRequest.tsis excluded by!**/generated/**generated/models/ObjectSerializer.tsis excluded by!**/generated/**package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (21)
__tests__/components/brain/left-sidebar/waves/BrainLeftSidebarWave.test.tsx__tests__/components/brain/left-sidebar/waves/UnifiedWavesListWaves.test.tsx__tests__/components/brain/left-sidebar/web/WebUnifiedWavesListWaves.test.tsx__tests__/components/waves/header/WaveHeaderPinButton.test.tsx__tests__/contexts/SeizeSettingsContext.test.tsx__tests__/contexts/wave/MyStreamContext.test.tsx__tests__/hooks/useWavesList.test.tsx__tests__/utils/mockFactories.tscomponents/brain/left-sidebar/waves/BrainLeftSidebarWave.tsxcomponents/brain/left-sidebar/waves/UnifiedWavesListWaves.tsxcomponents/brain/left-sidebar/web/WebUnifiedWavesListWaves.tsxcomponents/user/layout/userPageVisibility.tscomponents/waves/header/WaveHeaderPinButton.tsxcomponents/waves/memes/submission/utils/buildPreviewDrop.tscomponents/waves/utils/getOptimisticDrop.tscontexts/SeizeSettingsContext.tsxcontexts/wave/hooks/useEnhancedWavesListCore.tshelpers/waves/wave.helpers.tshooks/useWaveDropsSearch.tshooks/useWavesList.tsopenapi.yaml
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
hooks/useWavesList.ts (1)
146-147: Make the new pin-state split explicit.A legacy pinned announcement can now appear in
waveswithisPinned: truewhile being deliberately omitted frompinnedWaves. That asymmetry is easy for callers to read as a full mirror ofpinnedIds. Consider renaming or documenting this return contract before more consumers depend on it.Also applies to: 179-183
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/useWavesList.ts` around lines 146 - 147, The function currently injects a pin flag that can diverge from pinnedWaves/pinnedIds (via waveIsDm/isAnnouncementsWave), so make that split explicit: replace the ambiguous isPinned set in result.push with a clearly named property (e.g., isPinnedInResult or isLegacyAnnouncementPinned) and update the function's return contract and any callers to consume the new field; alternatively add a short JSDoc on the hook (useWavesList) explaining that this flag is not necessarily derived from pinnedIds/pinnedWaves. Target symbols to change/inspect: the result.push call that currently spreads wave and sets isPinned, the pinnedWaves/pinnedIds usage, and callers of the hook that read wave.isPinned.__tests__/hooks/useWavesList.test.tsx (1)
143-148: Avoid asserting a private data shape here.This
hasOwnProperty("isAnnouncement")check is testing an implementation detail rather than behavior. It makes it harder to move announcement classification into the returned wave data if we later want to remove the sidebars’ dependency onuseSeizeSettingsOptional(). The ordering and pin assertions already cover the observable behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/hooks/useWavesList.test.tsx` around lines 143 - 148, Remove the implementation-detail assertion that checks for the presence of "isAnnouncement" on a wave (the Object.prototype.hasOwnProperty.call(...) assertion around result.current.waves.find(wave => wave.id === "4")). Instead, delete that assertion entirely and rely on the existing observable-behavior checks (ordering and pin assertions) in the test; do not replace it with another shape-specific check so the test doesn't depend on internal data shapes like isAnnouncement.components/brain/left-sidebar/waves/UnifiedWavesListWaves.tsx (1)
114-136: Verify this component never depends on a missing settings provider.Announcement bucketing now depends on
useSeizeSettingsOptional(). If that hook can returnundefined—or even lag the provider update by a render—the announcement wave falls back into the regular/pinned buckets, which also re-enables the normal pin control for an unpinned announcement. Please verify every call site guarantees the provider, or pass explicit announcement metadata on thewavesitems instead. The same pattern is duplicated inWebUnifiedWavesListWaves.tsx.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/brain/left-sidebar/waves/UnifiedWavesListWaves.tsx` around lines 114 - 136, The current bucketing uses useSeizeSettingsOptional() so when the provider is missing or lags the optional hook returns undefined and announcements silently fall into pinned/regular (see UnifiedWavesListWaves.tsx: announcementWaves/pinnedWaves/regularWaves and the same pattern in WebUnifiedWavesListWaves.tsx). Fix by either (A) requiring the provider: replace useSeizeSettingsOptional() with the non-optional useSeizeSettings() (or assert/throw if undefined) so isAnnouncementsWave is always callable, or (B) make the buckets resilient by using explicit announcement metadata on each wave (e.g., wave.isAnnouncement or wave.announcementFlag) and fall back to that when seizeSettings is undefined; apply the same change to WebUnifiedWavesListWaves.tsx so both components use the same provider-or-metadata strategy.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hooks/useWavesList.ts`:
- Around line 86-96: The hook currently chooses announcementWave from
trackedAnnouncementWave or fetchedAnnouncementWave but doesn't surface the
fetchedAnnouncementWave's query state; update the hook to expose the fallback
query's loading and error (and refetch) so consumers can distinguish "no
announcement" from a loading/error state. Concretely, keep using
useWaveById(announcementsWaveId, { enabled: shouldFetchAnnouncementWave })
(fetchedAnnouncementWave, refetchAnnouncementWave) and add properties such as
announcementQueryLoading: fetchedAnnouncementWave.isLoading,
announcementQueryError: fetchedAnnouncementWave.error, and announcementRefetch:
refetchAnnouncementWave (or similar names used by your hook return object)
alongside the existing announcementWave, trackedAnnouncementWave and other flags
so callers can read loading/error for the fallback query.
---
Nitpick comments:
In `@__tests__/hooks/useWavesList.test.tsx`:
- Around line 143-148: Remove the implementation-detail assertion that checks
for the presence of "isAnnouncement" on a wave (the
Object.prototype.hasOwnProperty.call(...) assertion around
result.current.waves.find(wave => wave.id === "4")). Instead, delete that
assertion entirely and rely on the existing observable-behavior checks (ordering
and pin assertions) in the test; do not replace it with another shape-specific
check so the test doesn't depend on internal data shapes like isAnnouncement.
In `@components/brain/left-sidebar/waves/UnifiedWavesListWaves.tsx`:
- Around line 114-136: The current bucketing uses useSeizeSettingsOptional() so
when the provider is missing or lags the optional hook returns undefined and
announcements silently fall into pinned/regular (see UnifiedWavesListWaves.tsx:
announcementWaves/pinnedWaves/regularWaves and the same pattern in
WebUnifiedWavesListWaves.tsx). Fix by either (A) requiring the provider: replace
useSeizeSettingsOptional() with the non-optional useSeizeSettings() (or
assert/throw if undefined) so isAnnouncementsWave is always callable, or (B)
make the buckets resilient by using explicit announcement metadata on each wave
(e.g., wave.isAnnouncement or wave.announcementFlag) and fall back to that when
seizeSettings is undefined; apply the same change to
WebUnifiedWavesListWaves.tsx so both components use the same
provider-or-metadata strategy.
In `@hooks/useWavesList.ts`:
- Around line 146-147: The function currently injects a pin flag that can
diverge from pinnedWaves/pinnedIds (via waveIsDm/isAnnouncementsWave), so make
that split explicit: replace the ambiguous isPinned set in result.push with a
clearly named property (e.g., isPinnedInResult or isLegacyAnnouncementPinned)
and update the function's return contract and any callers to consume the new
field; alternatively add a short JSDoc on the hook (useWavesList) explaining
that this flag is not necessarily derived from pinnedIds/pinnedWaves. Target
symbols to change/inspect: the result.push call that currently spreads wave and
sets isPinned, the pinnedWaves/pinnedIds usage, and callers of the hook that
read wave.isPinned.
🪄 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: 58f6eb9d-1023-4d3f-bdc7-c1a3b54a4cad
📒 Files selected for processing (8)
__tests__/components/brain/left-sidebar/waves/BrainLeftSidebarWave.test.tsx__tests__/components/brain/left-sidebar/waves/UnifiedWavesListWaves.test.tsx__tests__/components/brain/left-sidebar/web/WebUnifiedWavesListWaves.test.tsx__tests__/hooks/useWavesList.test.tsx__tests__/utils/mockFactories.tscomponents/brain/left-sidebar/waves/UnifiedWavesListWaves.tsxcomponents/brain/left-sidebar/web/WebUnifiedWavesListWaves.tsxhooks/useWavesList.ts
✅ Files skipped from review due to trivial changes (1)
- tests/utils/mockFactories.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/components/brain/left-sidebar/waves/BrainLeftSidebarWave.test.tsx
- tests/components/brain/left-sidebar/waves/UnifiedWavesListWaves.test.tsx
There was a problem hiding this comment.
🧹 Nitpick comments (3)
__tests__/hooks/useWavesList.test.tsx (2)
65-107: Consider adding a test for DM announcement wave filtering.The
waveIsDmcheck inannouncementWaveresolution (line 96 of the hook) filters out DM waves even if configured as the announcement. There's no test covering this edge case.🧪 Suggested test case
test("filters out DM waves from being treated as announcements", () => { const dmAnnouncementWave = { id: "5", created_at: 4, metrics: { latest_drop_timestamp: 400 }, wave: { type: ApiWaveType.Chat }, chat: { scope: { group: { is_direct_message: true } } }, } as any; useSeizeSettingsMock.mockReturnValue({ seizeSettings: { announcements_wave_id: "5", }, isAnnouncementsWave: (waveId: string | null | undefined) => waveId === "5", }); useWaveByIdMock.mockReturnValue({ wave: dmAnnouncementWave, isLoading: false, isError: false, error: null, refetch: jest.fn(), isFetching: false, }); const { result } = renderHook(() => useWavesList(), { wrapper }); expect(result.current.announcementWave).toBeNull(); expect(result.current.waves.find((w: any) => w.id === "5")).toBeUndefined(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/hooks/useWavesList.test.tsx` around lines 65 - 107, The test suite lacks coverage for the case where the configured announcement wave is a DM and should be filtered out by the waveIsDm logic inside useWavesList; add a unit test that mocks useSeizeSettingsMock to set announcements_wave_id to the DM wave id, mocks useWaveByIdMock to return a wave with wave.type = ApiWaveType.Chat and chat.scope.group.is_direct_message = true, then render useWavesList and assert that announcementWave is null and that the DM wave is not present in the returned waves array (reference identifiers: useWavesList, announcementWave, waveIsDm, useSeizeSettingsMock, useWaveByIdMock).
198-228: Validates whitespace normalization in announcement ID.The test uses
announcements_wave_id: " 4 "(with surrounding spaces) butisAnnouncementsWavechecks for"4". This implicitly tests thatnormalizeOptionalWaveIdtrims the ID before comparison.However, the mock
isAnnouncementsWavereturnswaveId === "4", which doesn't account for normalization. The test passes because the actual wave IDs in the fixtures are already trimmed ("4"), so the comparison works. Consider making the mock more realistic by having it call the actual normalization logic or documenting that the mock is simplified.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/hooks/useWavesList.test.tsx` around lines 198 - 228, The test is intended to validate whitespace normalization of announcements_wave_id but the mock isAnnouncementsWave simply compares waveId === "4" and thus doesn't reflect trimming; update the useSeizeSettingsMock so its isAnnouncementsWave uses the same normalization as production (call normalizeOptionalWaveId or trim the incoming waveId) or change announcements_wave_id to a trimmed value and/or document the simplification; reference the mock provider (useSeizeSettingsMock), the predicate isAnnouncementsWave, the input field announcements_wave_id, and the hook under test useWavesList so the mock behavior matches normalizeOptionalWaveId's contract.hooks/useWavesList.ts (1)
167-176: RedundantisAnnouncementsWavecheck incombinedWaves.The input arrays (
mainWavesandseparatelyFetchedPinnedWaves) are already filtered to exclude announcement waves in their respectiveuseMemohooks (lines 105-109 and 139-141). The check at line 168 is therefore always false for waves in these arrays.While harmless, this adds unnecessary computation. Consider removing the redundant check for clarity.
♻️ Suggested simplification
[...mainWaves, ...separatelyFetchedPinnedWaves].forEach((wave) => { - if (waveIsDm(wave) || isAnnouncementsWave(wave.id)) { + if (waveIsDm(wave)) { return; } - allWavesMap.set(wave.id, { ...wave, isPinned: pinnedWavesSet.has(wave.id), }); });Then remove
isAnnouncementsWavefrom the dependency array at line 198.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/useWavesList.ts` around lines 167 - 176, The loop merging mainWaves and separatelyFetchedPinnedWaves into allWavesMap contains a redundant isAnnouncementsWave(wave.id) check because both source arrays are already filtered to exclude announcement waves; remove the isAnnouncementsWave condition from that forEach (keep waveIsDm check) and ensure you also remove isAnnouncementsWave from the dependency array of the useMemo that builds this combinedWaves so dependencies reflect only the actually used values (e.g., mainWaves, separatelyFetchedPinnedWaves, pinnedWavesSet, waveIsDm if applicable).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@__tests__/hooks/useWavesList.test.tsx`:
- Around line 65-107: The test suite lacks coverage for the case where the
configured announcement wave is a DM and should be filtered out by the waveIsDm
logic inside useWavesList; add a unit test that mocks useSeizeSettingsMock to
set announcements_wave_id to the DM wave id, mocks useWaveByIdMock to return a
wave with wave.type = ApiWaveType.Chat and chat.scope.group.is_direct_message =
true, then render useWavesList and assert that announcementWave is null and that
the DM wave is not present in the returned waves array (reference identifiers:
useWavesList, announcementWave, waveIsDm, useSeizeSettingsMock,
useWaveByIdMock).
- Around line 198-228: The test is intended to validate whitespace normalization
of announcements_wave_id but the mock isAnnouncementsWave simply compares waveId
=== "4" and thus doesn't reflect trimming; update the useSeizeSettingsMock so
its isAnnouncementsWave uses the same normalization as production (call
normalizeOptionalWaveId or trim the incoming waveId) or change
announcements_wave_id to a trimmed value and/or document the simplification;
reference the mock provider (useSeizeSettingsMock), the predicate
isAnnouncementsWave, the input field announcements_wave_id, and the hook under
test useWavesList so the mock behavior matches normalizeOptionalWaveId's
contract.
In `@hooks/useWavesList.ts`:
- Around line 167-176: The loop merging mainWaves and
separatelyFetchedPinnedWaves into allWavesMap contains a redundant
isAnnouncementsWave(wave.id) check because both source arrays are already
filtered to exclude announcement waves; remove the isAnnouncementsWave condition
from that forEach (keep waveIsDm check) and ensure you also remove
isAnnouncementsWave from the dependency array of the useMemo that builds this
combinedWaves so dependencies reflect only the actually used values (e.g.,
mainWaves, separatelyFetchedPinnedWaves, pinnedWavesSet, waveIsDm if
applicable).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c5f1d4f8-93d1-4e28-824d-7c0a02bffb50
📒 Files selected for processing (2)
__tests__/hooks/useWavesList.test.tsxhooks/useWavesList.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
__tests__/hooks/usePinnedWavesServer.test.tsx (1)
82-85: Add the unresolved-settings regression case here.All of the new tests stub
useSeizeSettingsOptional()as already resolved, so they won't catch the bootstrap path where the hook still seesnullannouncement settings and miscounts the budget. A case withuseSeizeSettingsOptionalMock.mockReturnValue(null)would fail against the current hook logic and protect the fix.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/hooks/usePinnedWavesServer.test.tsx` around lines 82 - 85, Add a test in usePinnedWavesServer.test.tsx that covers the unresolved-settings regression by setting useSeizeSettingsOptionalMock.mockReturnValue(null) (instead of the resolved object used in other tests) and asserting the hook's bootstrap path behaves correctly (e.g., budget/count is correct and no miscount occurs); target the existing test harness that uses useSeizeSettingsOptionalMock and the isAnnouncementsWave behavior to reproduce the failing path so the fix is protected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hooks/usePinnedWavesServer.ts`:
- Around line 99-133: The budget calculation counts announcement pins before
seize settings (and announcements_wave_id) are loaded; update the hook so budget
logic waits for settings readiness: add or derive a readiness flag from
useSeizeSettingsOptional (e.g. isSeizeSettingsReady or check
announcements_wave_id presence on seizeSettings) and use it in
countsTowardPinBudget/pinnedBudgetCount/getOngoingPinCount/canPinWave so that
until settings are ready you do not treat legacy announcement ids as counting
against the MAX_PINNED_WAVES (or alternatively return a readiness boolean from
the hook so callers can defer pin decisions until settings are loaded).
---
Nitpick comments:
In `@__tests__/hooks/usePinnedWavesServer.test.tsx`:
- Around line 82-85: Add a test in usePinnedWavesServer.test.tsx that covers the
unresolved-settings regression by setting
useSeizeSettingsOptionalMock.mockReturnValue(null) (instead of the resolved
object used in other tests) and asserting the hook's bootstrap path behaves
correctly (e.g., budget/count is correct and no miscount occurs); target the
existing test harness that uses useSeizeSettingsOptionalMock and the
isAnnouncementsWave behavior to reproduce the failing path so the fix is
protected.
🪄 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: 7c455d17-0495-4b22-8455-70c6d9b1d803
📒 Files selected for processing (8)
__tests__/components/brain/left-sidebar/waves/BrainLeftSidebarWavePin.test.tsx__tests__/components/waves/header/WaveHeaderPinButton.test.tsx__tests__/hooks/usePinnedWavesServer.test.tsx__tests__/hooks/useWavesList.test.tsxcomponents/brain/left-sidebar/waves/BrainLeftSidebarWavePin.tsxcomponents/waves/header/WaveHeaderPinButton.tsxhooks/usePinnedWavesServer.tshooks/useWavesList.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/hooks/useWavesList.test.tsx
- tests/components/waves/header/WaveHeaderPinButton.test.tsx
- hooks/useWavesList.ts
|
ragnep
approved these changes
Apr 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Tests