Repository navigation
fix(order-progress): show cancelling screen from unfillable step - #7889
Conversation
|
@tenderdeve is attempting to deploy a commit to the cow-dev Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThe order progress bar centralizes immediate step decisions in an exported helper, uses it during updates, and adds tests for cancellation, terminal, timestamp, and debounce behavior. ChangesOrder progress bar
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
|
@tenderdeve , please rebase the PR to develop |
6de8136 to
1cb7594
Compare
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/cowswap-frontend/src/modules/orderProgressBar/hooks/useOrderProgressBarProps.ts (1)
467-471: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression coverage for the centralized immediate-step contract.
Test that cancellation and terminal steps bypass the minimum display delay while a normal step remains debounced. This protects the cancellation-flow behavior introduced by this change.
🤖 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 `@apps/cowswap-frontend/src/modules/orderProgressBar/hooks/useOrderProgressBarProps.ts` around lines 467 - 471, Add regression tests around the immediate-step condition in useOrderProgressBarProps, covering cancellation and terminal step names bypassing MINIMUM_STEP_DISPLAY_TIME while a normal step remains debounced. Reuse the existing hook test setup and IMMEDIATE_STEP_NAMES contract, and assert the resulting transition behavior for each case.
🤖 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
`@apps/cowswap-frontend/src/modules/orderProgressBar/hooks/useOrderProgressBarProps.ts`:
- Around line 467-471: Add regression tests around the immediate-step condition
in useOrderProgressBarProps, covering cancellation and terminal step names
bypassing MINIMUM_STEP_DISPLAY_TIME while a normal step remains debounced. Reuse
the existing hook test setup and IMMEDIATE_STEP_NAMES contract, and assert the
resulting transition behavior for each case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0631633a-a3e9-4847-a7be-ab75c09c9fad
📒 Files selected for processing (1)
apps/cowswap-frontend/src/modules/orderProgressBar/hooks/useOrderProgressBarProps.ts
Head branch was pushed to by a user without write access
|
@coderabbitai updated in b7c16da — extracted the immediate-step decision into |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
git show --stat --oneline --decorate --no-renames b7c16dad9
git show --format=fuller --no-ext-diff --unified=80 --no-renames b7c16dad9 -- \
apps/cowswap-frontend/src/modules/orderProgressBar/hooks/useOrderProgressBarProps.ts \
'apps/cowswap-frontend/src/modules/orderProgressBar/**'
printf '\n--- candidate tests ---\n'
fd -a -t f . apps/cowswap-frontend/src/modules/orderProgressBar | rg '(/|^)(useOrderProgressBarProps|.*[Tt]est.*|.*[Ss]pec.*)\.(ts|tsx)$' || true
printf '\n--- hook outline ---\n'
ast-grep outline apps/cowswap-frontend/src/modules/orderProgressBar/hooks/useOrderProgressBarProps.ts --items all --view expanded || trueLength of output: 22654
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
The minimum step-display debounce delayed the CANCELLING step when the previous step (e.g. unfillable / price out of market) had been shown for less than the minimum time. The order reached the cancelled state before the delayed update fired, so the cancelling screen was skipped entirely. Show CANCELLING immediately, like the other terminal user-facing steps.
Extract the immediate-step decision into shouldUpdateStepImmediately and add regression tests asserting cancellation and terminal steps bypass the minimum display time while a normal step stays debounced. Addresses @coderabbitai's review suggestion.
53415e3 to
5fd7492
Compare
|
@elena-zh rebased onto latest |
Closes #6881
When an order is showing the unfillable / price out of market screen and the user cancels it, the cancelling screen was never shown — the progress bar jumped straight from unfillable to cancelled.
Cause: the minimum step-display debounce delays a step change if the previous step was shown for less than the minimum time.
CANCELLINGwas not in the list of steps exempt from this debounce, so the delayed update was still pending when the order reached the cancelled state, and it got cleared before firing.Fix: show
CANCELLINGimmediately, alongside the other terminal user-facing steps (finished, cancelled, expired, cancellation failed).Summary by CodeRabbit