-
Notifications
You must be signed in to change notification settings - Fork 0
refactor: move WebWrapper files from packages/platform to apps/web/modules #25
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: devin_pr_code_review_bench_100_devin2_base_refactor_move_webwrapper_files_from_packages_platform_to_apps_web_modules_pr708
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 Undefined router variable causes runtime errors in BookerWebWrapper
The router variable is used in multiple places but is never defined in the component.
Click to expand
Problem:
The component uses router at lines 193, 195, 228, 232, and 238, but there is no const router = useRouter() or similar declaration.
Looking at the imports at line 4:
import { useSearchParams, usePathname } from "next/navigation";The useRouter import was removed during the refactor, but the router usage remained.
Affected code:
// Line 193
router.push(`${url.pathname}${url.search}`);
// Line 195
[router]
// Line 228
router.push(newPath);
// Line 232
router.push("/apps/categories/calendar");
// Line 238
router.push(newUrl.toString());Impact:
This will cause a ReferenceError: router is not defined at runtime when users interact with:
- The overlay calendar switch
- The instant meeting "Connect Now" button
- The "No Calendar" overlay click handler
- The overlay continue button
(Refers to line 193)
Recommendation: Add import { useRouter } from "next/navigation"; and define const router = useRouter(); inside the BookerWebWrapperComponent function.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
|
||
| if (isRRWeightsEnabled) { | ||
| if (weightA === weightB) { | ||
| return priorityA - priorityB; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 sortHosts function has reversed priority sorting order when weights are equal
The sortHosts function was moved from HostEditDialogs.tsx to hostGroupUtils.ts, but the sorting logic for priority was changed incorrectly.
Click to expand
Original code (correct):
if (weightA === weightB) {
return priorityB - priorityA; // Descending - higher priority first
}New code (incorrect):
if (weightA === weightB) {
return priorityA - priorityB; // Ascending - lower priority first
}Impact:
When round-robin weights are enabled and two hosts have equal weights, the hosts will now be sorted with lower priority hosts appearing first instead of higher priority hosts. This affects:
PriorityDialoginHostEditDialogs.tsx:82WeightDialoginHostEditDialogs.tsx:178useEventTypeFormhook inpackages/platform/atoms/event-types/hooks/useEventTypeForm.ts:103EventTeamAssignmentTabinapps/web/modules/event-types/components/tabs/assignment/EventTeamAssignmentTab.tsx
This reverses the expected behavior where hosts with higher priority (e.g., 4 = "highest") should appear before hosts with lower priority (e.g., 0 = "lowest").
Recommendation: Change line 53 from return priorityA - priorityB; to return priorityB - priorityA; to maintain the original descending sort order for priority.
Was this helpful? React with 👍 or 👎 to provide feedback.
Benchmark PR from qodo-benchmark#708