Skip to content

Comments

Code Review Bench PR #26629 - refactor: move WebWrapper files from packages/platform to apps/web/modules#9

Open
ketkarameya wants to merge 2 commits intobase_pr_26629_20260125_6392from
corrupted_pr_26629_20260125_6392
Open

Code Review Bench PR #26629 - refactor: move WebWrapper files from packages/platform to apps/web/modules#9
ketkarameya wants to merge 2 commits intobase_pr_26629_20260125_6392from
corrupted_pr_26629_20260125_6392

Conversation

@ketkarameya
Copy link

@ketkarameya ketkarameya commented Feb 10, 2026

Benchmark PR for Gitar evaluation

Original PR: agentic-review-benchmarks#9

refactor: move WebWrapper files from packages/platform to apps/web/modules


Summary by Gitar

  • Architecture refactor:
    • Relocated 8+ WebWrapper components from packages/platform/atoms to apps/web/modules for better code locality
  • Fixed critical bug:
    • Corrected host priority sorting in packages/lib/bookings/hostGroupUtils.ts:56 (reversed comparison operator)
  • Import path updates:
    • Updated ~30 files to reference new component locations under apps/web/modules/*

This will update automatically on new commits.


Comment on lines +52 to +53
if (weightA === weightB) {
return priorityA - priorityB;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Bug: sortHosts equal-weights case reverses priority sort direction

The sortHosts function was changed from priorityB - priorityA to priorityA - priorityB in the equal-weights branch (when isRRWeightsEnabled is true and weights are equal). This reverses the sort order from descending (highest priority first) to ascending (lowest priority first).

In this codebase, priority values are: 0=lowest, 1=low, 2=medium, 3=high, 4=highest (see HostEditDialogs.tsx priorityOptions). The backend's getUsersWithHighestPriority in getLuckyUser.ts uses Math.max(...), confirming higher numbers = higher priority.

Old behavior (equal weights): priorityB - priorityA → hosts with priority 4 ("highest") sort first — correct
New behavior (equal weights): priorityA - priorityB → hosts with priority 0 ("lowest") sort first — incorrect

Note that the !isRRWeightsEnabled branch still uses priorityB - priorityA (descending), making the two code paths inconsistent. This will cause hosts with the lowest priority to appear first in the UI when round-robin weights are enabled and weights happen to be equal.

The PR context claims this is a bug fix, but the original code was correct for both branches. This change introduces a regression in host ordering.

Was this helpful? React with 👍 / 👎

Suggested change
if (weightA === weightB) {
return priorityA - priorityB;
return priorityB - priorityA;
  • Apply suggested fix

"./add-members-switch/AddMembersWithSwitchPlatformWrapper": "./add-members-switch/AddMembersWithSwitchPlatformWrapper.tsx",
"./add-members-switch/AddMembersWithSwitchWebWrapper": "./add-members-switch/AddMembersWithSwitchWebWrapper.tsx",
"./availability/AvailabilitySettings": "./availability/AvailabilitySettings.tsx",
"./booker": "./booker/index.ts",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Bug: package.json "booker" export points to deleted index.ts file

The packages/platform/atoms/package.json still declares "./booker": "./booker/index.ts" in its exports, but booker/index.ts was deleted in this PR. Any external consumer (or future internal code) importing @calcom/atoms/booker will encounter a module resolution error at build time.

The export entry should either be removed (since BookerWebWrapper has moved to apps/web) or updated to point to the remaining export.ts in the booker directory if that's the intended entry point.

Was this helpful? React with 👍 / 👎

  • Apply suggested fix

Copy link

@gitar-bot-staging gitar-bot-staging bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ This PR is blocked due to unresolved code review findings.

Configure merge blocking · Maintainers can dismiss this review. Gitar never approves changes.

@gitar-bot-staging
Copy link

gitar-bot-staging bot commented Feb 10, 2026

Code Review ⚠️ Changes requested 0 resolved / 3 findings

Migration refactoring is well-structured, but the sortHosts priority comparison change reverses the correct sort order for equal-weight hosts, and the @calcom/atoms/booker export points to a deleted file.

⚠️ Bug: sortHosts equal-weights case reverses priority sort direction

📄 packages/lib/bookings/hostGroupUtils.ts:52-53

The sortHosts function was changed from priorityB - priorityA to priorityA - priorityB in the equal-weights branch (when isRRWeightsEnabled is true and weights are equal). This reverses the sort order from descending (highest priority first) to ascending (lowest priority first).

In this codebase, priority values are: 0=lowest, 1=low, 2=medium, 3=high, 4=highest (see HostEditDialogs.tsx priorityOptions). The backend's getUsersWithHighestPriority in getLuckyUser.ts uses Math.max(...), confirming higher numbers = higher priority.

Old behavior (equal weights): priorityB - priorityA → hosts with priority 4 ("highest") sort first — correct
New behavior (equal weights): priorityA - priorityB → hosts with priority 0 ("lowest") sort first — incorrect

Note that the !isRRWeightsEnabled branch still uses priorityB - priorityA (descending), making the two code paths inconsistent. This will cause hosts with the lowest priority to appear first in the UI when round-robin weights are enabled and weights happen to be equal.

The PR context claims this is a bug fix, but the original code was correct for both branches. This change introduces a regression in host ordering.

Suggested fix
      return priorityB - priorityA;
⚠️ Bug: package.json "booker" export points to deleted index.ts file

📄 packages/platform/atoms/package.json:58

The packages/platform/atoms/package.json still declares "./booker": "./booker/index.ts" in its exports, but booker/index.ts was deleted in this PR. Any external consumer (or future internal code) importing @calcom/atoms/booker will encounter a module resolution error at build time.

The export entry should either be removed (since BookerWebWrapper has moved to apps/web) or updated to point to the remaining export.ts in the booker directory if that's the intended entry point.

💡 Quality: Deep relative paths reaching into packages/ from apps/web

📄 apps/web/components/apps/DestinationCalendarSettingsWebWrapper.tsx:3-4

After moving DestinationCalendarSettingsWebWrapper to apps/web/components/apps/, it now uses fragile deep relative paths to import from the platform package:

import { AtomsWrapper } from "../../../../packages/platform/atoms/src/components/atoms-wrapper";
import { DestinationCalendarSettings } from "../../../../packages/platform/atoms/destination-calendar/DestinationCalendar";

These paths bypass the package alias system (@calcom/atoms/...) and are fragile — any directory restructuring will break them. While there is some pre-existing usage of this pattern elsewhere in the codebase, the refactoring goal of reducing coupling with the platform package would be better served by adding proper package exports for AtomsWrapper and DestinationCalendarSettings and using the @calcom/atoms/... alias.

Rules 🎸 1 action taken

Gitar Rules

🎸 Summary Enhancement: Appended technical summary with 3 bullets covering architecture refactor, critical bug fix, and import updates

5 rules not applicable. Show all rules by commenting gitar display:verbose.

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change:

Auto-apply Compact
gitar auto-apply:on         
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants