Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 46 minutes and 17 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis pull request introduces wave curation drop reordering functionality using dnd-kit for drag-and-drop interactions. Changes include a new Changes
Sequence DiagramssequenceDiagram
actor User
participant UI as UserPageProfileWaveMasonry
participant DnD as dnd-kit
participant State as Local Reorder State
participant API as useDropCurationOrderMutation
participant Server as API Server
participant Cache as React Query Cache
User->>UI: Enable reorder mode
UI->>UI: fetchAllWaveCurationDrops()
UI->>Server: GET /waves/{id}/curations/{curationId}/drops
Server-->>UI: All drops with order IDs
UI->>State: Initialize persistedOrderIdsRef
User->>DnD: Drag drop to new position
DnD->>UI: handleDragEnd(event)
UI->>State: Reorder drops locally
UI->>UI: Render with new visual order
UI->>API: persistOrderAsync(updates)
API->>Server: POST /api/drops/{dropId}/curations
alt Success
Server-->>API: Order updated
API->>Cache: invalidateDrops()
Cache-->>UI: Refetch drop data
UI->>State: Keep local reorder state
else Failure
Server-->>API: Error response
API->>UI: setToast(error)
UI->>State: Revert to persistedOrderIdsRef
end
sequenceDiagram
participant App as App
participant Hook as useWaveCurationDrops
participant RQ as React Query
participant WS as WebSocket
participant API as API Server
App->>Hook: useWaveCurationDrops({wave, curationId})
Hook->>RQ: useInfiniteQuery(createKey)
RQ->>API: GET /waves/{id}/curations/{curationId}/drops?page=0
API-->>RQ: Page 1 drops with nextPageParam
RQ-->>Hook: drops[], hasNextPage, etc.
App->>Hook: fetchNextPage()
Hook->>RQ: fetchNextPage()
RQ->>API: GET /waves/{id}/curations/{curationId}/drops?page=1
API-->>RQ: Page 2 drops
WS->>Hook: DROP_UPDATE event (wave.id matches)
Hook->>Hook: useDebouncedQueryRefetch()
Hook->>RQ: refetch()
RQ->>API: GET /waves/{id}/curations/{curationId}/drops?page=0
API-->>RQ: Refreshed drops
RQ-->>App: Updated drops array
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openapi.yaml (1)
8634-8645:⚠️ Potential issue | 🔴 Critical
ApiDropGroupMentionis malformed and currently makes the spec invalid.Lines 8634–8645 contain a schema definition that mixes a scalar enum (
type: string,enum: [ALL]) with an object property (priority_order), violating YAML and OpenAPI syntax. The same schema name is declared twice immediately after. The YAML parser fails at line 8638 with "expected , but found '?'" becausepriority_orderis indented at the list item level rather than as a separate object field. This must be resolved to a single, valid schema shape before the spec can be parsed and used for validation or code generation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openapi.yaml` around lines 8634 - 8645, The ApiDropGroupMention schema is malformed: remove the duplicated scalar enum declaration and consolidate into a single valid schema shape (either keep it as a string enum OR make it an object with properties), e.g., if you need priority_order attach it under ApiDropGroupMention as properties.priority_order with type integer, format int64 and minimum 1 and move the enum into a valid property (or drop the enum entirely); ensure only one ApiDropGroupMention definition exists and that priority_order is indented as a properties field rather than at the enum list level.
🧹 Nitpick comments (4)
openapi.yaml (2)
6051-6067: Align the curation path parameter name with the sibling routes.This route uses
{curation_id}while the update/delete routes use{curationId}for the same resource. That inconsistency leaks into generated client signatures and makes the same identifier appear under two public names.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openapi.yaml` around lines 6051 - 6067, The path parameter name in the route "/waves/{id}/curations/{curation_id}/drops" is inconsistent with sibling routes; change the path segment and parameter definition from "{curation_id}" to "{curationId}" so the OpenAPI path and the parameters block use the same identifier, and update any references used by operationId "listWaveCurationDrops" to match the new name.
8365-8387: Tighten the ordering contract for curation members.These schemas introduce
drop_priority_orderfor reorder flows, but still allownull. ForApiCurationDropin particular, the endpoint only returns drops already in the curation, so clients cannot rely on a stable persisted position from the schema. Either make that field non-null there, or document the exact null case and keep it limited toApiDropCuration.Also applies to: 8614-8626
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openapi.yaml` around lines 8365 - 8387, ApiCurationDrop currently allows drop_priority_order to be null, but the endpoint returns only drops already in the curation so clients expect a stable position; update the OpenAPI schemas so ApiCurationDrop's drop_priority_order is non-nullable (remove or set nullable: false) and keep nullable only on the input/update schema (e.g., ApiDropCuration) if you must accept null for reorder payloads, or alternatively add explicit documentation to ApiCurationDrop describing the null case — change the schema definition for ApiCurationDrop (and mirror the same decision for the other occurrence at the referenced lines) so the contract is consistent.hooks/useWaveCurationDrops.ts (1)
122-149: No upper bound on page fetching could cause performance issues.
fetchAllWaveCurationDropsfetches all pages in an unbounded loop. For curations with a very large number of drops, this could result in excessive API calls and memory usage, potentially degrading user experience.Consider adding a maximum page limit or total items cap as a safety measure.
♻️ Proposed safety limit
+const MAX_FETCH_ALL_PAGES = 50; + export const fetchAllWaveCurationDrops = async ({ wave, curationId, pageSize = MAX_CURATION_DROPS_PAGE_SIZE, }: { readonly wave: ApiWave; readonly curationId: string; readonly pageSize?: number | undefined; }): Promise<ExtendedDrop[]> => { const pages: ApiCurationDropsPage[] = []; let nextPage = 1; let hasNextPage = true; - while (hasNextPage) { + while (hasNextPage && nextPage <= MAX_FETCH_ALL_PAGES) { const page = await fetchWaveCurationDropsPage({ waveId: wave.id, curationId, page: nextPage, pageSize, }); pages.push(page); hasNextPage = page.next; nextPage = page.page + 1; } return processDrops(pages, wave); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/useWaveCurationDrops.ts` around lines 122 - 149, fetchAllWaveCurationDrops currently loops unboundedly calling fetchWaveCurationDropsPage and can exhaust memory/API for very large curations; add a safety cap (e.g., maxPages or maxItems) to stop fetching after a reasonable limit and surface that limit via a new optional parameter (e.g., maxPages?: number or maxItems?: number) with a sensible default constant (e.g., MAX_CURATION_DROPS_MAX_PAGES), update the while loop to break when the cap is reached, and ensure processDrops is called on the pages fetched; reference fetchAllWaveCurationDrops, fetchWaveCurationDropsPage, MAX_CURATION_DROPS_PAGE_SIZE, and processDrops when making the change.components/user/waves/UserPageProfileWave.tsx (1)
296-301: Consider optimizing the permission check.
useWaveCurationDropsis called solely to determine ifcurationDrops.length > 1for thecanReordercheck. This triggers a full paginated fetch of drops when you only need to know if there's more than one drop.If the API supports it, consider a lighter endpoint or using a count-only query. Otherwise, this is acceptable for now as the data is likely cached or shared.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/user/waves/UserPageProfileWave.tsx` around lines 296 - 301, The current call to useWaveCurationDrops (used to compute canReorder) fetches full paginated drop data just to check if there are more than one item; replace this with a lightweight check by calling a count/summary endpoint or requesting only two items (e.g., limit=2 or a minimal fields projection) so you avoid full pagination. Update the logic around useWaveCurationDrops / curationDrops (and the derived canReorder) to use the new lightweight hook or query (or add a new useWaveCurationCount hook) that accepts wave and curationId (profileCuration?.id) and enabled (canClear && !!profileCuration) and returns a count or a small array; then compute canReorder from count > 1 or returned.length > 1.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/user/waves/UserPageProfileWave.tsx`:
- Around line 550-553: The confirmUnavailableClearProfileWave function currently
awaits clearSelectedProfileWave() and then unconditionally closes the modal via
setIsUnavailableClearConfirmOpen(false); wrap the await in a try/catch and only
call setIsUnavailableClearConfirmOpen(false) when clearSelectedProfileWave
resolves successfully, leaving the modal open on error so the user can see the
failure (mirror the behavior needed for confirmClearProfileWave); surface or
rethrow the caught error so existing error UI/notifications can display it.
- Around line 331-334: confirmClearProfileWave currently awaits
clearSelectedProfileWave() then unconditionally calls
setIsClearConfirmOpen(false), which will close the modal even if persistence
fails; change confirmClearProfileWave to only close the modal on success by
wrapping the await in a try/catch: call await clearSelectedProfileWave() inside
try, then call setIsClearConfirmOpen(false) after a successful await, and handle
errors in catch (e.g., surface the error via existing error handler/UI instead
of closing the modal). Ensure you reference confirmClearProfileWave,
clearSelectedProfileWave, and setIsClearConfirmOpen when making the change.
In `@hooks/drops/useDropCurationOrderMutation.ts`:
- Around line 25-37: The current batch update uses Promise.all over updates.map
calling postDropCuration, which will reject immediately on the first failure and
leave partial updates untracked; replace Promise.all with Promise.allSettled in
the function that performs the batch (the updates.map / postDropCuration block
inside useDropCurationOrderMutation) and iterate the settled results to separate
successes from failures, retry or roll back failed items as appropriate, collect
failed dropIds and error messages, and surface a consolidated error state or
toast to the UI so callers can reconcile partial failures rather than silently
failing on the first error.
In `@openapi.yaml`:
- Around line 977-992: The GET /light-drops operation currently defines the
query parameters min_serial_no and older_first twice; remove the duplicated
parameter definitions so each (name, in) pair appears only once in the
operation. Keep the canonical definitions for min_serial_no (type integer,
format int64, minimum 1) and older_first (type boolean) and, if the two blocks
differ, merge any unique description text into the remaining single definitions
for min_serial_no and older_first to preserve intent.
In `@services/api/drop-curations-api.ts`:
- Around line 6-7: The getDropCurationEndpoint helper inserts dropId directly
into the URL path which can break if dropId contains reserved characters; update
getDropCurationEndpoint (and any other uses around the other occurrence) to
encode the path segment by applying encodeURIComponent to dropId before
concatenation so the generated endpoint is safe when dropId contains "/", "?",
"#", etc.
---
Outside diff comments:
In `@openapi.yaml`:
- Around line 8634-8645: The ApiDropGroupMention schema is malformed: remove the
duplicated scalar enum declaration and consolidate into a single valid schema
shape (either keep it as a string enum OR make it an object with properties),
e.g., if you need priority_order attach it under ApiDropGroupMention as
properties.priority_order with type integer, format int64 and minimum 1 and move
the enum into a valid property (or drop the enum entirely); ensure only one
ApiDropGroupMention definition exists and that priority_order is indented as a
properties field rather than at the enum list level.
---
Nitpick comments:
In `@components/user/waves/UserPageProfileWave.tsx`:
- Around line 296-301: The current call to useWaveCurationDrops (used to compute
canReorder) fetches full paginated drop data just to check if there are more
than one item; replace this with a lightweight check by calling a count/summary
endpoint or requesting only two items (e.g., limit=2 or a minimal fields
projection) so you avoid full pagination. Update the logic around
useWaveCurationDrops / curationDrops (and the derived canReorder) to use the new
lightweight hook or query (or add a new useWaveCurationCount hook) that accepts
wave and curationId (profileCuration?.id) and enabled (canClear &&
!!profileCuration) and returns a count or a small array; then compute canReorder
from count > 1 or returned.length > 1.
In `@hooks/useWaveCurationDrops.ts`:
- Around line 122-149: fetchAllWaveCurationDrops currently loops unboundedly
calling fetchWaveCurationDropsPage and can exhaust memory/API for very large
curations; add a safety cap (e.g., maxPages or maxItems) to stop fetching after
a reasonable limit and surface that limit via a new optional parameter (e.g.,
maxPages?: number or maxItems?: number) with a sensible default constant (e.g.,
MAX_CURATION_DROPS_MAX_PAGES), update the while loop to break when the cap is
reached, and ensure processDrops is called on the pages fetched; reference
fetchAllWaveCurationDrops, fetchWaveCurationDropsPage,
MAX_CURATION_DROPS_PAGE_SIZE, and processDrops when making the change.
In `@openapi.yaml`:
- Around line 6051-6067: The path parameter name in the route
"/waves/{id}/curations/{curation_id}/drops" is inconsistent with sibling routes;
change the path segment and parameter definition from "{curation_id}" to
"{curationId}" so the OpenAPI path and the parameters block use the same
identifier, and update any references used by operationId
"listWaveCurationDrops" to match the new name.
- Around line 8365-8387: ApiCurationDrop currently allows drop_priority_order to
be null, but the endpoint returns only drops already in the curation so clients
expect a stable position; update the OpenAPI schemas so ApiCurationDrop's
drop_priority_order is non-nullable (remove or set nullable: false) and keep
nullable only on the input/update schema (e.g., ApiDropCuration) if you must
accept null for reorder payloads, or alternatively add explicit documentation to
ApiCurationDrop describing the null case — change the schema definition for
ApiCurationDrop (and mirror the same decision for the other occurrence at the
referenced lines) so the contract is consistent.
🪄 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: 1ed5878c-2b30-4e7f-a90a-7c00eba9fc86
⛔ Files ignored due to path filters (8)
generated/models/ApiCurationDrop.tsis excluded by!**/generated/**generated/models/ApiCurationDropsPage.tsis excluded by!**/generated/**generated/models/ApiDropCuration.tsis excluded by!**/generated/**generated/models/ApiDropCurationRequest.tsis excluded by!**/generated/**generated/models/ApiWaveCuration.tsis excluded by!**/generated/**generated/models/ApiWaveCurationRequest.tsis excluded by!**/generated/**generated/models/ObjectSerializer.tsis excluded by!**/generated/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (12)
components/brain/my-stream/curations/MyStreamWaveCurationContent.tsxcomponents/user/waves/UserPageProfileWave.tsxcomponents/user/waves/UserPageProfileWaveMasonry.tsxcomponents/utils/icons/MoveIcon.tsxhooks/drops/useDropCurationMembershipMutation.tshooks/drops/useDropCurationOrderMutation.tshooks/drops/useDropCurations.tshooks/useCurationManagementPermission.tshooks/useWaveCurationDrops.tsopenapi.yamlpackage.jsonservices/api/drop-curations-api.ts
| async function confirmClearProfileWave() { | ||
| await clearSelectedProfileWave(); | ||
| setIsClearConfirmOpen(false); | ||
| } |
There was a problem hiding this comment.
Modal may close prematurely on persistence failure.
confirmClearProfileWave closes the modal immediately after await clearSelectedProfileWave(), but if the operation fails, the user won't see the error in context since the modal is already closed. Consider only closing on success.
🛠️ Proposed fix
async function confirmClearProfileWave() {
- await clearSelectedProfileWave();
- setIsClearConfirmOpen(false);
+ try {
+ await clearSelectedProfileWave();
+ setIsClearConfirmOpen(false);
+ } catch {
+ // Keep modal open on failure; toast will show error
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async function confirmClearProfileWave() { | |
| await clearSelectedProfileWave(); | |
| setIsClearConfirmOpen(false); | |
| } | |
| async function confirmClearProfileWave() { | |
| try { | |
| await clearSelectedProfileWave(); | |
| setIsClearConfirmOpen(false); | |
| } catch { | |
| // Keep modal open on failure; toast will show error | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/user/waves/UserPageProfileWave.tsx` around lines 331 - 334,
confirmClearProfileWave currently awaits clearSelectedProfileWave() then
unconditionally calls setIsClearConfirmOpen(false), which will close the modal
even if persistence fails; change confirmClearProfileWave to only close the
modal on success by wrapping the await in a try/catch: call await
clearSelectedProfileWave() inside try, then call setIsClearConfirmOpen(false)
after a successful await, and handle errors in catch (e.g., surface the error
via existing error handler/UI instead of closing the modal). Ensure you
reference confirmClearProfileWave, clearSelectedProfileWave, and
setIsClearConfirmOpen when making the change.
| async function confirmUnavailableClearProfileWave() { | ||
| await clearSelectedProfileWave(); | ||
| }, [clearSelectedProfileWave]); | ||
| setIsUnavailableClearConfirmOpen(false); | ||
| } |
There was a problem hiding this comment.
Same modal close behavior issue.
Same as confirmClearProfileWave - consider keeping the modal open on failure so the user sees the error in context.
🛠️ Proposed fix
async function confirmUnavailableClearProfileWave() {
- await clearSelectedProfileWave();
- setIsUnavailableClearConfirmOpen(false);
+ try {
+ await clearSelectedProfileWave();
+ setIsUnavailableClearConfirmOpen(false);
+ } catch {
+ // Keep modal open on failure; toast will show error
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async function confirmUnavailableClearProfileWave() { | |
| await clearSelectedProfileWave(); | |
| }, [clearSelectedProfileWave]); | |
| setIsUnavailableClearConfirmOpen(false); | |
| } | |
| async function confirmUnavailableClearProfileWave() { | |
| try { | |
| await clearSelectedProfileWave(); | |
| setIsUnavailableClearConfirmOpen(false); | |
| } catch { | |
| // Keep modal open on failure; toast will show error | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/user/waves/UserPageProfileWave.tsx` around lines 550 - 553, The
confirmUnavailableClearProfileWave function currently awaits
clearSelectedProfileWave() and then unconditionally closes the modal via
setIsUnavailableClearConfirmOpen(false); wrap the await in a try/catch and only
call setIsUnavailableClearConfirmOpen(false) when clearSelectedProfileWave
resolves successfully, leaving the modal open on error so the user can see the
failure (mirror the behavior needed for confirmClearProfileWave); surface or
rethrow the caught error so existing error UI/notifications can display it.
| - name: min_serial_no | ||
| in: query | ||
| required: false | ||
| description: Oldest message if null | ||
| schema: | ||
| type: integer | ||
| format: int64 | ||
| minimum: 1 | ||
| - name: older_first | ||
| in: query | ||
| required: false | ||
| description: >- | ||
| By default this endpoint orders things newer first, but if you set | ||
| it to true then it starts from older drops. | ||
| schema: | ||
| type: boolean |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
from collections import Counter
names = []
for i, line in enumerate(open("openapi.yaml"), start=1):
if 955 <= i <= 999 and line.lstrip().startswith("- name:"):
names.append((i, line.split(":", 1)[1].strip()))
print("parameters:", names)
counts = Counter(name for _, name in names)
print("duplicates:", {k: v for k, v in counts.items() if v > 1})
PYRepository: 6529-Collections/6529seize-frontend
Length of output: 276
🏁 Script executed:
# Check the operation context and structure around lines 955-1000
sed -n '945,1000p' openapi.yaml | cat -nRepository: 6529-Collections/6529seize-frontend
Length of output: 2079
Remove the duplicated min_serial_no and older_first query parameters from GET /light-drops.
The operation declares both parameters twice (at lines 963–976 and 977–992). OpenAPI requires parameters to be unique by (name, in) within an operation; duplicates break spec validation and client generation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openapi.yaml` around lines 977 - 992, The GET /light-drops operation
currently defines the query parameters min_serial_no and older_first twice;
remove the duplicated parameter definitions so each (name, in) pair appears only
once in the operation. Keep the canonical definitions for min_serial_no (type
integer, format int64, minimum 1) and older_first (type boolean) and, if the two
blocks differ, merge any unique description text into the remaining single
definitions for min_serial_no and older_first to preserve intent.
| const getDropCurationEndpoint = (dropId: string): string => | ||
| `${publicEnv.API_ENDPOINT}/api/drops/${dropId}/curations`; |
There was a problem hiding this comment.
Encode dropId before inserting it into the path.
dropId is used as a raw path segment in both requests. If it ever contains /, ?, or #, the client will hit the wrong endpoint.
Suggested fix
const getDropCurationEndpoint = (dropId: string): string =>
- `${publicEnv.API_ENDPOINT}/api/drops/${dropId}/curations`;
+ `${publicEnv.API_ENDPOINT}/api/drops/${encodeURIComponent(dropId)}/curations`;
export const deleteDropCuration = async ({
dropId,
body,
}: {
readonly dropId: string;
readonly body: ApiDropCurationRequest;
}): Promise<void> => {
await commonApiDeleteWithBody<ApiDropCurationRequest, void>({
- endpoint: `drops/${dropId}/curations`,
+ endpoint: `drops/${encodeURIComponent(dropId)}/curations`,
body,
});
};Also applies to: 75-75
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/api/drop-curations-api.ts` around lines 6 - 7, The
getDropCurationEndpoint helper inserts dropId directly into the URL path which
can break if dropId contains reserved characters; update getDropCurationEndpoint
(and any other uses around the other occurrence) to encode the path segment by
applying encodeURIComponent to dropId before concatenation so the generated
endpoint is safe when dropId contains "/", "?", "#", etc.
|
1 participant
Summary by CodeRabbit
New Features
Improvements