Scroll to history 080526#2361
Closed
simo6529 wants to merge 4 commits into
Closed
Conversation
📝 WalkthroughWalkthroughThis PR implements structured failure handling for serial jump (scroll-to-target) operations in wave navigation. It introduces typed failure reasons, adds optional failure callbacks throughout the fetch and scroll pipeline, and wires error toast UI in the component layer. ChangesSerial Jump Failure Handling
Sequence Diagram(s)sequenceDiagram
participant UI as WaveDropsAll
participant Hook as useWaveDropsSerialScroll
participant Pag as useWavePagination
participant Fetch as fetchLightWaveMessages
UI->>Hook: Call with onSerialScrollFailure
Hook->>Pag: Initiate fetchNextPage
Pag->>Fetch: Call with onSerialScrollFailure callback
activate Fetch
Fetch->>Fetch: Attempt drop-ids lookup
alt Success
Fetch-->>Pag: Return merged drops
else Target Not Found
Fetch->>Fetch: Throw DropIdsSerialLookupError
Fetch->>Pag: Invoke onSerialScrollFailure
else Fetch Failed
Fetch->>Fetch: Throw DropIdsSerialLookupError
Fetch->>Pag: Invoke onSerialScrollFailure
end
deactivate Fetch
Pag->>Hook: Propagate callback result
alt Timeout or error detected
Hook->>Hook: reportActiveScrollFailure
end
Hook->>Hook: Check hasReportedScrollFailureRef
Hook->>UI: Invoke onSerialScrollFailure with failure
UI->>UI: Map reason to toast message
UI->>UI: Display error toast
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 docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
contexts/wave/hooks/useWavePagination.ts (1)
276-298: 💤 Low valueConsider simplifying the conditional fetch logic.
The current implementation has three separate branches. Since
fetchLightWaveMessagesaccepts an optionalonFailureparameter, you could simplify to two branches without the inner conditional:♻️ Optional simplification
let rawPromise: Promise<(ApiDrop | ApiDropId)[] | null>; if (props.type === DropSize.FULL) { rawPromise = fetchWaveMessages( props.waveId, oldestSerialNo, controller.signal ); - } else if (props.onSerialScrollFailure) { - rawPromise = fetchLightWaveMessages( - props.waveId, - oldestSerialNo, - props.targetSerialNo, - controller.signal, - props.onSerialScrollFailure - ); } else { rawPromise = fetchLightWaveMessages( props.waveId, oldestSerialNo, props.targetSerialNo, - controller.signal + controller.signal, + props.onSerialScrollFailure ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contexts/wave/hooks/useWavePagination.ts` around lines 276 - 298, The three-branch fetch can be collapsed into two: if props.type === DropSize.FULL call fetchWaveMessages(...) into rawPromise; otherwise call fetchLightWaveMessages(...) passing props.waveId, oldestSerialNo, props.targetSerialNo, controller.signal and pass props.onSerialScrollFailure (it’s optional) so you no longer need the inner conditional; update the assignment to rawPromise accordingly and keep the same parameter order and signals used in the existing calls to fetchLightWaveMessages and fetchWaveMessages.components/waves/drops/wave-drops-all/hooks/useWaveDropsSerialScroll.ts (1)
86-97: 💤 Low valueConsider extracting shared
getErrorDetailshelper.This function is duplicated from
wave-messages-utils.ts. While the duplication is minor, extracting it to a shared utility would improve maintainability.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/waves/drops/wave-drops-all/hooks/useWaveDropsSerialScroll.ts` around lines 86 - 97, The getErrorDetails function is duplicated; extract it into a shared utility (e.g., export getErrorDetails from a new or existing shared errors/util module) and replace the local implementation in useWaveDropsSerialScroll.ts with an import from that shared module; also update wave-messages-utils.ts to import and use the same exported getErrorDetails to remove the duplicate definition and keep a single source of truth.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@components/waves/drops/wave-drops-all/hooks/useWaveDropsSerialScroll.ts`:
- Around line 86-97: The getErrorDetails function is duplicated; extract it into
a shared utility (e.g., export getErrorDetails from a new or existing shared
errors/util module) and replace the local implementation in
useWaveDropsSerialScroll.ts with an import from that shared module; also update
wave-messages-utils.ts to import and use the same exported getErrorDetails to
remove the duplicate definition and keep a single source of truth.
In `@contexts/wave/hooks/useWavePagination.ts`:
- Around line 276-298: The three-branch fetch can be collapsed into two: if
props.type === DropSize.FULL call fetchWaveMessages(...) into rawPromise;
otherwise call fetchLightWaveMessages(...) passing props.waveId, oldestSerialNo,
props.targetSerialNo, controller.signal and pass props.onSerialScrollFailure
(it’s optional) so you no longer need the inner conditional; update the
assignment to rawPromise accordingly and keep the same parameter order and
signals used in the existing calls to fetchLightWaveMessages and
fetchWaveMessages.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7f25681b-e5af-4fff-9c92-581f65d06eed
📒 Files selected for processing (8)
__tests__/components/waves/drops/WaveDropsAll.test.tsx__tests__/contexts/wave/hooks/useWavePagination.test.ts__tests__/contexts/wave/utils/wave-messages-utils.additional.test.tscomponents/waves/drops/wave-drops-all/hooks/useWaveDropsSerialScroll.tscomponents/waves/drops/wave-drops-all/index.tsxcontexts/wave/hooks/useWavePagination.tscontexts/wave/utils/wave-messages-utils.tsdocs/waves/chat/feature-serial-jump-navigation.md
|
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
1 participant
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
Release Notes
Bug Fixes
Documentation