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 structured API error handling and end-to-end monitoring for drop reactions: new monitoring module, hook and component integrations that record mutation lifecycle and reconcile websocket updates, API changes to surface structured errors, OpenAPI/helpers threshold shape updates, and extensive tests exercising these flows. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Client UI
participant Hook as useDropReaction
participant Mon as dropReactionMonitoring
participant API as commonApiPost/Delete
participant WS as WebSocketStatus
participant Sentry as Sentry
UI->>Hook: click reaction (intendedReaction)
Hook->>WS: read websocket status
Hook->>Mon: beginReactionMutation(params)
Mon-->>Hook: mutationContext
Hook->>Mon: recordReactionOptimisticApplied
Hook->>API: POST/DELETE (errorMode: "structured")
Note over API,Mon: request in-flight
API-->>Hook: success
Hook->>Mon: recordReactionRequestSucceeded
Hook->>UI: finalize / call onSuccess
API-->>Hook: OR structured error
Hook->>Mon: recordReactionRequestFailed(error)
Hook->>Mon: recordReactionRollbackApplied
Mon->>Sentry: addBreadcrumb / maybe captureException
WS->>Mon: realtime update arrives
Mon->>Mon: reconcile server state vs latest mutation
Mon->>Sentry: emit reconciliation breadcrumb / capture if anomaly
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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: 807ffc7aba
ℹ️ 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
__tests__/components/waves/drops/WaveDropReactions.test.tsx (1)
280-292:⚠️ Potential issue | 🟡 MinorAssert that chip reactions request structured errors.
The new failure test would still pass if the component stopped passing
errorMode: "structured". Update the API-call assertions so the structured-error contract is covered.💚 Proposed test update
- expect(commonApi.commonApiPost).toHaveBeenCalledWith({ - endpoint: "drops/test-drop/reaction", - body: { reaction: ":gm:" }, - }); + expect(commonApi.commonApiPost).toHaveBeenCalledWith( + expect.objectContaining({ + endpoint: "drops/test-drop/reaction", + body: { reaction: ":gm:" }, + errorMode: "structured", + }) + ); @@ - expect(commonApi.commonApiDelete).toHaveBeenCalledWith({ - endpoint: "drops/test-drop/reaction", - }); + expect(commonApi.commonApiDelete).toHaveBeenCalledWith( + expect.objectContaining({ + endpoint: "drops/test-drop/reaction", + errorMode: "structured", + }) + );Also applies to: 295-335
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/components/waves/drops/WaveDropReactions.test.tsx` around lines 280 - 292, Update the test assertions to verify the structured-error contract by expecting the API calls include errorMode: "structured"; specifically, modify the expect for commonApi.commonApiPost (the call that posts to endpoint "drops/test-drop/reaction" with body { reaction: ":gm:" }) to include errorMode: "structured" in the argument object, and likewise update the expect for commonApi.commonApiDelete (the call that deletes at endpoint "drops/test-drop/reaction") to assert it was called with an object containing errorMode: "structured".
🧹 Nitpick comments (1)
__tests__/hooks/useDropReaction.test.ts (1)
65-118: Consider broadening test coverage foruseDropReaction.The suite currently asserts a single error-path scenario. Given the hook orchestrates optimistic updates, monitoring calls, and two API endpoints, consider also covering:
- The remove path (
commonApiDeletecalled witherrorMode: "structured"whenreactionCode === contextProfileContext.reaction).- Successful execution:
onSuccessis invoked andrecordReactionRequestSucceededis called with the mutation context frombeginReactionMutation.- Short-circuit when
drop.idstarts with"temp-"(canReact === false).- Fallback error message when the rejection does not carry a usable
message.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/hooks/useDropReaction.test.ts` around lines 65 - 118, Add additional tests for useDropReaction: (1) test the remove path by calling result.current.react with a reaction equal to contextProfileContext.reaction and assert commonApi.commonApiDelete is called with errorMode: "structured"; (2) test successful execution by mocking commonApi.commonApiPost to resolve, invoking react, and asserting onSuccess path behavior including that recordReactionRequestSucceeded is called with the mutation context returned from beginReactionMutation and that optimistic update/monitoring hooks are invoked; (3) test the short-circuit behavior when drop.id starts with "temp-" by setting mockDrop.id = "temp-..." and asserting canReact is false and no API call is made; (4) test the fallback error message by rejecting the API promise with a value that has no usable message and asserting setToast is called with a generic error message—use the existing symbols useDropReaction, result.current.react, commonApi.commonApiPost, commonApi.commonApiDelete, beginReactionMutation, recordReactionRequestSucceeded, canReact, and setToastMock to locate and assert the behaviors.
🤖 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/drops/useDropReaction.ts`:
- Around line 183-202: The current try block calls onSuccess?.() so if the
consumer throws, our catch treats a successful API call as a failure; move the
onSuccess call out of the try so consumer errors don't trigger
recordReactionRequestFailed/setToast/recordReactionRollbackApplied. Concretely,
in useDropReaction.ts around the code using recordReactionRequestSucceeded,
recordReactionRequestFailed, rollbackRef.current and
recordReactionRollbackApplied: set a local success flag (e.g., let succeeded =
false), set succeeded = true immediately after
recordReactionRequestSucceeded(...) and clearing rollbackRef, then after the
try/catch (only when succeeded) call onSuccess?.() inside its own try/catch if
you want to swallow consumer errors; ensure rollbackRef.current is only
read/cleared inside the catch as before so rollbacks remain correct.
In `@utils/monitoring/dropReactionMonitoring.ts`:
- Around line 581-583: The current guard uses strict null checks so undefined
apiFailedAt causes an unintended early return; change the checks to
null-coalescing equality checks that treat undefined the same as null (e.g.,
replace "context.apiFailedAt !== null || context.apiSucceededAt === null" with
"context.apiFailedAt != null || context.apiSucceededAt == null") so that
recordReactionRequestSucceeded() setting apiSucceededAt lets reconciliation
mismatches be reported; update this condition in dropReactionMonitoring.ts
around the code that references context.apiFailedAt and context.apiSucceededAt
(the block that runs after recordReactionRequestSucceeded()).
---
Outside diff comments:
In `@__tests__/components/waves/drops/WaveDropReactions.test.tsx`:
- Around line 280-292: Update the test assertions to verify the structured-error
contract by expecting the API calls include errorMode: "structured";
specifically, modify the expect for commonApi.commonApiPost (the call that posts
to endpoint "drops/test-drop/reaction" with body { reaction: ":gm:" }) to
include errorMode: "structured" in the argument object, and likewise update the
expect for commonApi.commonApiDelete (the call that deletes at endpoint
"drops/test-drop/reaction") to assert it was called with an object containing
errorMode: "structured".
---
Nitpick comments:
In `@__tests__/hooks/useDropReaction.test.ts`:
- Around line 65-118: Add additional tests for useDropReaction: (1) test the
remove path by calling result.current.react with a reaction equal to
contextProfileContext.reaction and assert commonApi.commonApiDelete is called
with errorMode: "structured"; (2) test successful execution by mocking
commonApi.commonApiPost to resolve, invoking react, and asserting onSuccess path
behavior including that recordReactionRequestSucceeded is called with the
mutation context returned from beginReactionMutation and that optimistic
update/monitoring hooks are invoked; (3) test the short-circuit behavior when
drop.id starts with "temp-" by setting mockDrop.id = "temp-..." and asserting
canReact is false and no API call is made; (4) test the fallback error message
by rejecting the API promise with a value that has no usable message and
asserting setToast is called with a generic error message—use the existing
symbols useDropReaction, result.current.react, commonApi.commonApiPost,
commonApi.commonApiDelete, beginReactionMutation,
recordReactionRequestSucceeded, canReact, and setToastMock to locate and assert
the behaviors.
🪄 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: 10b0d955-c90c-4a95-b7d7-9959613e720b
📒 Files selected for processing (12)
__tests__/components/waves/drops/WaveDropActionsAddReaction.test.tsx__tests__/components/waves/drops/WaveDropReactions.test.tsx__tests__/hooks/useDropReaction.test.ts__tests__/utils/monitoring/dropReactionMonitoring.test.tscomponents/waves/drops/WaveDropActionsAddReaction.tsxcomponents/waves/drops/WaveDropActionsQuickReact.tsxcomponents/waves/drops/WaveDropReactions.tsxcomponents/waves/drops/reaction-utils.tscontexts/wave/hooks/useWaveRealtimeUpdater.tshooks/drops/useDropReaction.tsservices/api/common-api.tsutils/monitoring/dropReactionMonitoring.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
components/waves/drops/reaction-utils.ts (1)
193-206: Minor: redundantcase nullin switch.
case nullanddefaultboth returnnull; the explicitnullcase is dead given thedefault. Also, a lookup table tends to read a bit better here:Proposed simplification
-const getEmptyStructuredReactionStatusMessage = ( - status: number | null -): string | null => { - switch (status) { - case 401: - return "Unauthorized"; - case 429: - return "Too Many Requests"; - case null: - return null; - default: - return null; - } -}; +const EMPTY_BODY_STATUS_MESSAGES: Record<number, string> = { + 401: "Unauthorized", + 429: "Too Many Requests", +}; + +const getEmptyStructuredReactionStatusMessage = ( + status: number | null +): string | null => (status !== null ? EMPTY_BODY_STATUS_MESSAGES[status] ?? null : null);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/waves/drops/reaction-utils.ts` around lines 193 - 206, The switch in getEmptyStructuredReactionStatusMessage contains a redundant case null; replace the switch with a small lookup (e.g., a statusMap object keyed by numeric codes) that maps 401 -> "Unauthorized" and 429 -> "Too Many Requests" and returns null for anything else, and delete the explicit case null branch so the function returns map[status] ?? null (ensure the function signature still accepts number | null and returns null for null).__tests__/components/waves/drops/WaveDropReactions.test.tsx (3)
31-48: Sentry/websocket mocks look good; considerjest.Mockedoverany.The
withScopecallback argument is typedany— prefer a minimal scope type (orPartial<import("@sentry/nextjs").Scope>) to keep lint noise down and catch drift if the scope surface being exercised changes. Non-blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/components/waves/drops/WaveDropReactions.test.tsx` around lines 31 - 48, The withScope mock currently types its callback parameter as any; change that to a minimal typed scope to reduce lint noise and catch API drift by using Partial<import("@sentry/nextjs").Scope> (or jest.Mocked<Scope>) for the callback argument in the jest.mock of "@sentry/nextjs" (the withScope mock) so the mocked scope object keys (setLevel, setFingerprint, setTag, setExtras) match the expected Scope shape; no changes needed to the useWebsocketStatus mock.
319-409: Good coverage for structured error → toast mapping.Both the JSON-body (
Unauthorized) and empty-body-with-statusText (Not Found) paths are exercised end-to-end through the UI. One small suggestion: add an assertion that the optimistic update was rolled back (rollbackwas called) on failure, since that's a likely regression surface when the error-handling path changes. Not essential for this PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/components/waves/drops/WaveDropReactions.test.tsx` around lines 319 - 409, Add an assertion after the waitFor in each failing-chip test to verify the optimistic rollback was invoked: locate the test cases in WaveDropReactions.test.tsx (the two it(...) blocks that mock commonApi.commonApiPost rejection) and add expect(rollback).toHaveBeenCalled(); (or the actual mocked rollback function name used in the test harness) after the existing setToastMock assertion so the test ensures the optimistic update is rolled back on error.
70-92: Module-scopedsetToastMock— confirm isolation across tests.
setToastMockis declared at module scope butjest.clearAllMocks()inbeforeEach(line 137) clears its call history, so assertions remain isolated. 👍However,
createStructuredReactionErroris duplicated verbatim with__tests__/components/waves/drops/reaction-utils.test.ts. Extract to a shared test helper to avoid drift when the structured-error shape evolves (e.g., ifresponse.headersis added).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/components/waves/drops/WaveDropReactions.test.tsx` around lines 70 - 92, The helper createStructuredReactionError is duplicated; extract it (and the module-scoped setToastMock if needed) into a shared test helper module (e.g., tests/helpers/reactionTestUtils.ts), export createStructuredReactionError and setToastMock, and then replace the inline definitions in WaveDropReactions.test.tsx and reaction-utils.test.ts with imports from that helper; update the tests to import { createStructuredReactionError, setToastMock } and remove the duplicate declarations so any future shape changes (e.g., response.headers) are made in one place.__tests__/components/waves/drops/reaction-utils.test.ts (1)
3-24: DuplicatecreateStructuredReactionErrorhelper.This exact helper is also defined in
__tests__/components/waves/drops/WaveDropReactions.test.tsx(and likely in the reaction hook/component tests referenced by the summary). Consider extracting it to a shared test utility (e.g.,__tests__/test-utils/structured-errors.ts) to keep the structured-error shape in one place as the error contract evolves.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/components/waves/drops/reaction-utils.test.ts` around lines 3 - 24, Duplicate helper createStructuredReactionError is defined in multiple tests; extract it into a single shared test utility (e.g., structured-errors.ts) and replace the inline definitions with imports. Move the factory function signature and implementation exactly as-is into the shared module, export it (named export createStructuredReactionError), then update tests (those containing createStructuredReactionError) to import { createStructuredReactionError } from that utility and remove the duplicated definitions so the structured-error shape is centralized.
🤖 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/waves/drops/reaction-utils.ts`:
- Around line 142-167: getStructuredReactionBodyMessage currently returns
whatever getDetailsFirstMessage(bodyRecord["details"]) yields, which can leave a
parseable-but-unknown-shaped object treated as a “structured body” and prevent
status/statusText fallbacks; change getStructuredReactionBodyMessage so that
after checking "error" and "message" it calls getDetailsFirstMessage and only
returns that value if it is a non-empty string, otherwise explicitly return null
— this makes parseable-but-no-known-fields be treated the same as no structured
body and aligns with how hasNoStructuredReactionBody/commonApiPost expect empty
results.
---
Nitpick comments:
In `@__tests__/components/waves/drops/reaction-utils.test.ts`:
- Around line 3-24: Duplicate helper createStructuredReactionError is defined in
multiple tests; extract it into a single shared test utility (e.g.,
structured-errors.ts) and replace the inline definitions with imports. Move the
factory function signature and implementation exactly as-is into the shared
module, export it (named export createStructuredReactionError), then update
tests (those containing createStructuredReactionError) to import {
createStructuredReactionError } from that utility and remove the duplicated
definitions so the structured-error shape is centralized.
In `@__tests__/components/waves/drops/WaveDropReactions.test.tsx`:
- Around line 31-48: The withScope mock currently types its callback parameter
as any; change that to a minimal typed scope to reduce lint noise and catch API
drift by using Partial<import("@sentry/nextjs").Scope> (or jest.Mocked<Scope>)
for the callback argument in the jest.mock of "@sentry/nextjs" (the withScope
mock) so the mocked scope object keys (setLevel, setFingerprint, setTag,
setExtras) match the expected Scope shape; no changes needed to the
useWebsocketStatus mock.
- Around line 319-409: Add an assertion after the waitFor in each failing-chip
test to verify the optimistic rollback was invoked: locate the test cases in
WaveDropReactions.test.tsx (the two it(...) blocks that mock
commonApi.commonApiPost rejection) and add expect(rollback).toHaveBeenCalled();
(or the actual mocked rollback function name used in the test harness) after the
existing setToastMock assertion so the test ensures the optimistic update is
rolled back on error.
- Around line 70-92: The helper createStructuredReactionError is duplicated;
extract it (and the module-scoped setToastMock if needed) into a shared test
helper module (e.g., tests/helpers/reactionTestUtils.ts), export
createStructuredReactionError and setToastMock, and then replace the inline
definitions in WaveDropReactions.test.tsx and reaction-utils.test.ts with
imports from that helper; update the tests to import {
createStructuredReactionError, setToastMock } and remove the duplicate
declarations so any future shape changes (e.g., response.headers) are made in
one place.
In `@components/waves/drops/reaction-utils.ts`:
- Around line 193-206: The switch in getEmptyStructuredReactionStatusMessage
contains a redundant case null; replace the switch with a small lookup (e.g., a
statusMap object keyed by numeric codes) that maps 401 -> "Unauthorized" and 429
-> "Too Many Requests" and returns null for anything else, and delete the
explicit case null branch so the function returns map[status] ?? null (ensure
the function signature still accepts number | null and returns null for null).
🪄 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: ac2d2e26-1a4a-4198-a6f4-a5e4ba897aec
📒 Files selected for processing (6)
__tests__/components/waves/drops/WaveDropReactions.test.tsx__tests__/components/waves/drops/reaction-utils.test.ts__tests__/hooks/useDropReaction.test.ts__tests__/services/common-api.test.tscomponents/waves/drops/reaction-utils.tsservices/api/common-api.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- services/api/common-api.ts
- tests/hooks/useDropReaction.test.ts
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
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)
11638-11709:⚠️ Potential issue | 🔴 CriticalMissing required ApiWaveConfig fields in wave response mappers and request builders.
The helper functions constructing
ApiWaveConfigobjects are missing several required fields now mandated by the schema:
waves.helpers.ts(line 63–73): Response mapping omitstotal_no_of_decisions,no_of_decisions_done,no_of_decisions_left,authenticated_user_eligible_for_admin, andnext_decision_timecreate-wave.helpers.ts(line 401–423): Request builder omits the same fieldsThese fields are marked as required in
openapi.yaml(lines 11641–11652) and included in the generatedApiWaveConfigmodel, but absence from these mappers will cause serialization gaps and violate the schema contract expected by generated clients.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openapi.yaml` around lines 11638 - 11709, The ApiWaveConfig mapping functions in waves.helpers.ts (response mapper) and create-wave.helpers.ts (request builder) are missing required fields from the OpenAPI schema; update the functions that build ApiWaveConfig objects to include total_no_of_decisions, no_of_decisions_done, no_of_decisions_left, authenticated_user_eligible_for_admin, and next_decision_time (use the corresponding source properties from the domain object or set explicit null/defaults if not present), ensuring the object returned by the response mapper (where ApiWaveConfig is assembled) and the request builder (where the payload is assembled) include these exact property names so the generated ApiWaveConfig model matches the schema.
🧹 Nitpick comments (2)
__tests__/services/common-api.test.ts (1)
119-366: LGTM — thorough coverage of the whitespace/precedence edge cases.The new tests exercise all the interesting branches of
handleApiError: whitespace-only raw body (structured + legacy), whitespace-onlyerrorfield falling back tostatusText,messagepreferred whenerroris whitespace, anddetails[0].messagepreferred when both higher-priority fields are whitespace. Assertions onresponse.statusText,response.body, anderror.messagetogether pin down the contract well.Optional nit: there's noticeable duplication in the
errortype-cast shape and the try/catch-and-rethrow-into-let errorpattern across the five structured-error tests. Extracting a small helper likecatchStructuredError(fn)returning the typed error would shrink each test substantially — worth considering as a follow-up, not a blocker.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/services/common-api.test.ts` around lines 119 - 366, The tests duplicate the same try/catch-and-typed-error pattern across multiple cases; extract a helper (e.g., catchStructuredError) that accepts a call (or the arguments for commonApiPost) and returns the caught error typed with { message: string; response: { status: number; headers: Headers; statusText?: string; body?: unknown } }; replace each repeated try/catch block with awaiting catchStructuredError(() => commonApiPost({...})) (or catchStructuredError({endpoint:"e",body:{},errorMode:"structured"}) if you prefer passing args) and update assertions to use the returned error; keep the helper local to the test file so other tests are unchanged.components/waves/drops/reaction-utils.ts (1)
244-282: Optional: flattengetReactionErrorMessageto address SonarCloud cognitive-complexity warning.SonarCloud flags cognitive complexity 22 (limit 15) at line 247. Extracting the "empty-body fallback chain" into a helper and using early returns brings this well under the threshold without behavior change.
♻️ Proposed refactor
+const getEmptyBodyFallback = ( + error: StructuredReactionError +): string | null => + getEmptyStructuredReactionStatusMessage(getStructuredReactionStatus(error)) ?? + getEmptyStructuredReactionStatusText(error) ?? + getEmptyStructuredReactionFallbackMessage(error); + export const getReactionErrorMessage = ( error: unknown, fallback: string ): string => { - if (error !== null && typeof error === "object") { - const structuredError = error as StructuredReactionError; - const structuredResponse = structuredError.response; - if (structuredResponse !== undefined) { - const structuredBody = structuredResponse.body; - const safeMessage = getStructuredReactionBodyMessage(structuredBody); - if (safeMessage) { - return safeMessage; - } - - if (hasNoStructuredReactionBody(structuredBody)) { - const statusMessage = getEmptyStructuredReactionStatusMessage( - getStructuredReactionStatus(structuredError) - ); - if (statusMessage) { - return statusMessage; - } - - const statusText = - getEmptyStructuredReactionStatusText(structuredError); - if (statusText) { - return statusText; - } - - const fallbackMessage = - getEmptyStructuredReactionFallbackMessage(structuredError); - if (fallbackMessage) { - return fallbackMessage; - } - } - } - } - - return fallback; + if (error === null || typeof error !== "object") { + return fallback; + } + const structuredError = error as StructuredReactionError; + const structuredResponse = structuredError.response; + if (structuredResponse === undefined) { + return fallback; + } + const safeMessage = getStructuredReactionBodyMessage(structuredResponse.body); + if (safeMessage) { + return safeMessage; + } + if (!hasNoStructuredReactionBody(structuredResponse.body)) { + return fallback; + } + return getEmptyBodyFallback(structuredError) ?? fallback; };Also minor: in
getEmptyStructuredReactionStatusMessage(Lines 198-208) the explicitcase nullis redundant givendefaultalso returnsnull— you can drop it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/waves/drops/reaction-utils.ts` around lines 244 - 282, getReactionErrorMessage has high cognitive complexity due to nested checks and an "empty-body fallback chain"; extract the block that handles hasNoStructuredReactionBody(structuredBody) (the calls to getEmptyStructuredReactionStatusMessage, getEmptyStructuredReactionStatusText, and getEmptyStructuredReactionFallbackMessage) into a small helper (e.g., resolveEmptyStructuredReactionFallback(structuredError): string | null) and refactor getReactionErrorMessage to use early returns for the structuredResponse/structuredBody/safeMessage checks, delegating the fallback chain to that helper to reduce nesting and complexity; optionally remove the redundant explicit "case null" in getEmptyStructuredReactionStatusMessage since default already returns null.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@helpers/waves/create-wave.helpers.ts`:
- Around line 401-405: The current wave creation sets wave.winning_threshold to
config.approval.thresholdTimeMs unconditionally; change this so that
winning_threshold is null unless the wave type (config.overview.type or the
created wave.type) equals 'APPROVE' — i.e., set wave.winning_threshold =
config.approval.thresholdTimeMs only when config.overview.type === 'APPROVE',
otherwise set it to null to match the schema for non-APPROVE waves.
In `@openapi.yaml`:
- Around line 8293-8297: The description for the max_winners property is
ambiguous about null semantics; update the OpenAPI description for max_winners
to explicitly state when null is allowed and what it means: for waves with type
== APPROVE, max_winners may be null to indicate unlimited or a non-null integer
to limit winners, and for non-APPROVE waves max_winners must be null/omitted.
Reference the max_winners property and the type value APPROVE when making this
change.
---
Outside diff comments:
In `@openapi.yaml`:
- Around line 11638-11709: The ApiWaveConfig mapping functions in
waves.helpers.ts (response mapper) and create-wave.helpers.ts (request builder)
are missing required fields from the OpenAPI schema; update the functions that
build ApiWaveConfig objects to include total_no_of_decisions,
no_of_decisions_done, no_of_decisions_left,
authenticated_user_eligible_for_admin, and next_decision_time (use the
corresponding source properties from the domain object or set explicit
null/defaults if not present), ensuring the object returned by the response
mapper (where ApiWaveConfig is assembled) and the request builder (where the
payload is assembled) include these exact property names so the generated
ApiWaveConfig model matches the schema.
---
Nitpick comments:
In `@__tests__/services/common-api.test.ts`:
- Around line 119-366: The tests duplicate the same try/catch-and-typed-error
pattern across multiple cases; extract a helper (e.g., catchStructuredError)
that accepts a call (or the arguments for commonApiPost) and returns the caught
error typed with { message: string; response: { status: number; headers:
Headers; statusText?: string; body?: unknown } }; replace each repeated
try/catch block with awaiting catchStructuredError(() => commonApiPost({...}))
(or catchStructuredError({endpoint:"e",body:{},errorMode:"structured"}) if you
prefer passing args) and update assertions to use the returned error; keep the
helper local to the test file so other tests are unchanged.
In `@components/waves/drops/reaction-utils.ts`:
- Around line 244-282: getReactionErrorMessage has high cognitive complexity due
to nested checks and an "empty-body fallback chain"; extract the block that
handles hasNoStructuredReactionBody(structuredBody) (the calls to
getEmptyStructuredReactionStatusMessage, getEmptyStructuredReactionStatusText,
and getEmptyStructuredReactionFallbackMessage) into a small helper (e.g.,
resolveEmptyStructuredReactionFallback(structuredError): string | null) and
refactor getReactionErrorMessage to use early returns for the
structuredResponse/structuredBody/safeMessage checks, delegating the fallback
chain to that helper to reduce nesting and complexity; optionally remove the
redundant explicit "case null" in getEmptyStructuredReactionStatusMessage since
default already returns null.
🪄 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: 753697d4-f4cf-42fb-ac9d-14fdca6635a0
⛔ Files ignored due to path filters (3)
generated/models/ApiCreateWaveConfig.tsis excluded by!**/generated/**generated/models/ApiWaveConfig.tsis excluded by!**/generated/**generated/models/ObjectSerializer.tsis excluded by!**/generated/**
📒 Files selected for processing (6)
__tests__/services/common-api.test.tscomponents/waves/drops/reaction-utils.tshelpers/waves/create-wave.helpers.tshelpers/waves/waves.helpers.tsopenapi.yamlservices/api/common-api.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
__tests__/helpers/waves/create-wave.helpers.extra.test.ts (1)
160-260: LGTM — coverage for both Approve and non-Approvewinning_thresholdpaths.The updated Approve assertion on the scalar
body.wave.winning_threshold === 60000and the new Rank test assertingtoBeNull()directly exercise the conditional added increate-wave.helpers.tsL406-409. This prevents regressions of the past review finding.Nit (pre-existing, not introduced here): Lines 109, 160, and 211 define
it(...)blocks at the top level, outside thedescribe("create-wave.helpers extra", ...)that closes at Line 108. They still execute, but grouping them under thedescribewould improve report readability. Feel free to defer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/helpers/waves/create-wave.helpers.extra.test.ts` around lines 160 - 260, Three test cases (the two new it(...) blocks checking winning_threshold and another earlier it at line 109) are declared at the top-level instead of inside the existing describe("create-wave.helpers extra", ...) which harms test grouping; move the two new it(...) blocks into that describe (or wrap them and the top-level it at line 109 in a new describe) so all tests for create-wave.helpers.extra are grouped together, keeping the assertions using getCreateNewWaveBody and ApiWaveType unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@utils/monitoring/dropReactionMonitoring.ts`:
- Around line 56-73: pruneState currently evicts mutationContextById and
latestMutationIdByDrop but never removes dropMutationSeqByDrop, causing
unbounded growth; update pruneState so that when you delete the
latestMutationIdByDrop entry for a drop (i.e., inside the branch where
latestMutationIdByDrop.get(context.dropId) === mutationId and you call
latestMutationIdByDrop.delete(context.dropId)), also call
dropMutationSeqByDrop.delete(context.dropId) so the per-drop sequence state is
pruned only when the last tracked mutation for that drop ages out (preserving
sequences while in-flight mutations exist).
- Around line 613-625: The fingerprint in captureReactionEvent currently uses
ANOMALY_REVERTED_AFTER_SUCCESS while the dedupe key, anomaly_kind tag, and
anomaly_kind extra use ANOMALY_OPTIMISTIC_REVERTED; update
captureReactionEvent's fingerprint to use ANOMALY_OPTIMISTIC_REVERTED (alongside
REACTION_FEATURE) so the Sentry grouping matches the anomaly_kind tags/extra,
and then remove the now-unused ANOMALY_REVERTED_AFTER_SUCCESS constant from the
module to avoid confusion.
---
Nitpick comments:
In `@__tests__/helpers/waves/create-wave.helpers.extra.test.ts`:
- Around line 160-260: Three test cases (the two new it(...) blocks checking
winning_threshold and another earlier it at line 109) are declared at the
top-level instead of inside the existing describe("create-wave.helpers extra",
...) which harms test grouping; move the two new it(...) blocks into that
describe (or wrap them and the top-level it at line 109 in a new describe) so
all tests for create-wave.helpers.extra are grouped together, keeping the
assertions using getCreateNewWaveBody and ApiWaveType unchanged.
🪄 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: 93a3c003-34de-4301-b40b-0bf42dc1e121
📒 Files selected for processing (7)
__tests__/helpers/waves/create-wave.helpers.extra.test.ts__tests__/helpers/waves/create-wave.helpers.test.ts__tests__/hooks/useDropReaction.test.ts__tests__/utils/monitoring/dropReactionMonitoring.test.tshelpers/waves/create-wave.helpers.tshooks/drops/useDropReaction.tsutils/monitoring/dropReactionMonitoring.ts
✅ Files skipped from review due to trivial changes (1)
- tests/utils/monitoring/dropReactionMonitoring.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/hooks/useDropReaction.test.ts
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
utils/monitoring/dropReactionMonitoring.ts (2)
552-586: Reconciliation window is measured fromstartedAt, not from API completion — slow requests can silently miss mismatch detection.At line 568 the 15 s window starts at
context.startedAt. If the API takes ~12–14 s (slow network, cold backend), a websocket reconciliation arriving even a second after the server ACK will be dropped becausenow - startedAt > RECONCILIATION_WINDOW_MS, so theoptimistic_revertedanomaly will never be reported for exactly the class of user where it matters most.Consider anchoring the window on
apiSucceededAt ?? requestSentAt ?? startedAtso the window bounds "time since we expected the server's canonical state", not "time since the user clicked". Not critical, but worth reconsidering.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@utils/monitoring/dropReactionMonitoring.ts` around lines 552 - 586, The current time-window check in recordReactionRealtimeReconciliation uses context.startedAt to decide if a websocket reconciliation is stale; change it to anchor the reconciliation window to the point we expect the server's canonical state (e.g., use const anchor = context.apiSucceededAt ?? context.requestSentAt ?? context.startedAt) and replace the now - context.startedAt > RECONCILIATION_WINDOW_MS check with now - anchor > RECONCILIATION_WINDOW_MS so slow API responses don't cause missed anomaly detection; keep the other exits (apiFailedAt/apiSucceededAt checks) intact.
46-47: RemovefailureCapturedandsupersededByMutationIdfromReactionMutationContext— they are written but never read.These fields are mutated at lines 319, 506, and 543 but never inspected anywhere in the codebase. A full repository search confirms no caller reads them. If they're not needed for future logging or analytics, remove them to reduce dead state in the context object.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@utils/monitoring/dropReactionMonitoring.ts` around lines 46 - 47, Remove the dead state fields failureCaptured and supersededByMutationId from the ReactionMutationContext type and delete all assignments that set those properties (the writes at the three mutation sites that set context.failureCaptured and context.supersededByMutationId). Update any constructors or initializers that add these keys to the context so they are not created, and run tests / a repo-wide search to ensure no reads remain; if any future logging/analytics code needs similar data, add it back with a clear usage.
🤖 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/waves/drops/reaction-utils.ts`:
- Around line 125-140: The function getDetailsFirstMessage currently only
inspects details[0] and returns null if that item is blank or malformed; update
getDetailsFirstMessage to iterate over the details array (use
isNonEmptyUnknownArray(details) as the guard), examine each entry as an object
with an optional message property, and return the first non-empty trimmed string
found; if no entries yield a usable message return null. Ensure references to
getDetailsFirstMessage and isNonEmptyUnknownArray are preserved and remove the
single-index assumption (currently using firstDetail) in favor of a loop or
array.find approach.
- Around line 260-290: The code dereferences structuredResponse.body without
guarding for response === null and also skips status-only errors; update the
block that casts error to StructuredReactionError and replace the current
structuredResponse checks so you first test whether structuredError.response is
non-null (use structuredError.response != null) before accessing .body and
calling getStructuredReactionBodyMessage/hasNoStructuredReactionBody, and if
response is null/undefined fall back to treating the error as a status-only case
by calling getStructuredReactionStatus(structuredError) and then running
getEmptyStructuredReactionStatusMessage, getEmptyStructuredReactionStatusText,
and getEmptyStructuredReactionFallbackMessage in the same order; ensure you
reference StructuredReactionError, structuredResponse,
getStructuredReactionBodyMessage, hasNoStructuredReactionBody,
getStructuredReactionStatus, getEmptyStructuredReactionStatusMessage,
getEmptyStructuredReactionStatusText, and
getEmptyStructuredReactionFallbackMessage when making these guards and fallback
calls.
In `@helpers/waves/create-wave.helpers.ts`:
- Around line 324-334: The current check in the isRolling path only uses typeof
to validate dates.endDate, which allows NaN/Infinity; update the validation in
create-wave.helpers.ts to require Number.isFinite(dates.endDate) (instead of or
in addition to typeof) and throw the same error message if it fails, so that
calculateLastDecisionTime(dates.firstDecisionTime, dates.subsequentDecisions,
dates.endDate) always receives a finite millisecond timestamp; locate the check
around the isRolling logic and the `if (typeof dates.endDate !== "number")`
branch and replace it with a Number.isFinite validation.
---
Nitpick comments:
In `@utils/monitoring/dropReactionMonitoring.ts`:
- Around line 552-586: The current time-window check in
recordReactionRealtimeReconciliation uses context.startedAt to decide if a
websocket reconciliation is stale; change it to anchor the reconciliation window
to the point we expect the server's canonical state (e.g., use const anchor =
context.apiSucceededAt ?? context.requestSentAt ?? context.startedAt) and
replace the now - context.startedAt > RECONCILIATION_WINDOW_MS check with now -
anchor > RECONCILIATION_WINDOW_MS so slow API responses don't cause missed
anomaly detection; keep the other exits (apiFailedAt/apiSucceededAt checks)
intact.
- Around line 46-47: Remove the dead state fields failureCaptured and
supersededByMutationId from the ReactionMutationContext type and delete all
assignments that set those properties (the writes at the three mutation sites
that set context.failureCaptured and context.supersededByMutationId). Update any
constructors or initializers that add these keys to the context so they are not
created, and run tests / a repo-wide search to ensure no reads remain; if any
future logging/analytics code needs similar data, add it back with a clear
usage.
🪄 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: 01cb9ee8-12e4-46bf-a90e-83560625f407
📒 Files selected for processing (6)
__tests__/components/waves/drops/reaction-utils.test.ts__tests__/helpers/waves/create-wave.helpers.extra.test.ts__tests__/utils/monitoring/dropReactionMonitoring.test.tscomponents/waves/drops/reaction-utils.tshelpers/waves/create-wave.helpers.tsutils/monitoring/dropReactionMonitoring.ts
✅ Files skipped from review due to trivial changes (1)
- tests/components/waves/drops/reaction-utils.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/helpers/waves/create-wave.helpers.extra.test.ts
- tests/utils/monitoring/dropReactionMonitoring.test.ts
|
ragnep
approved these changes
Apr 22, 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
Chores