perf: remove city-timezones dep in favor of CDN#23430
Conversation
Walkthrough
Possibly related PRs
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
dosubot
bot
added
the
performance
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/v2/src/modules/timezones/services/timezones.service.ts (1)
1-10: Remove unused RedisService dependencyNo longer used; keep DI clean.
-import { RedisService } from "@/modules/redis/redis.service"; import { Injectable } from "@nestjs/common"; import { cityTimezonesHandler } from "@calcom/platform-libraries"; import type { CityTimezones } from "@calcom/platform-libraries"; @Injectable() export class TimezonesService { - constructor(private readonly redisService: RedisService) {} + constructor() {}
🧹 Nitpick comments (4)
packages/features/cityTimezones/cityTimezonesHandler.ts (1)
36-37: Return stable, locale-aware orderingProvide deterministic ordering for UX/search.
- const uniqueCities = Object.values(topByName); - return uniqueCities; + const uniqueCities = Object.values(uniqueByKey).sort((a, b) => + a.city.localeCompare(b.city, undefined, { sensitivity: "base" }) + ); + return uniqueCities;apps/api/v2/src/modules/timezones/services/timezones.service.ts (1)
11-13: Use RedisService for caching city time zones
The service already injects RedisService but never uses it. Replace the per-request CDN call with a Redis-backed cache to avoid in-memory state and support multi-instance setups:async getCityTimeZones(): Promise<CityTimezones> { const key = 'city-timezones'; const cached = await this.redisService.get<CityTimezones>(key); if (cached) return cached; const data = await cityTimezonesHandler(); await this.redisService.set(key, data, { ttl: 60 * 60 * 1000 }); // 1h TTL return data; }This centralizes caching, leverages the existing RedisService, and makes TTL configurable via your config service.
packages/features/components/timezone-select/TimezoneSelect.tsx (2)
112-114: Localize aria-labelPer guideline, use t() for user-visible strings.
Replace with your project’s i18n helper:
- aria-label="Timezone Select" + aria-label={t("timezone_select")}Please confirm the correct import for t() in this package (e.g., next-intl or your i18n util).
58-60: Consider SSR/server fetch to avoid client CDN dependencyClient-side fetching adds latency and leaks CDN calls to browsers. Prefer server-side fetch (API or RSC) and pass data as props, with cache and revalidate.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (5)
apps/api/v2/src/modules/timezones/services/timezones.service.ts(1 hunks)package.json(0 hunks)packages/features/cityTimezones/cityTimezonesHandler.ts(1 hunks)packages/features/components/timezone-select/TimezoneSelect.tsx(2 hunks)packages/lib/package.json(0 hunks)
💤 Files with no reviewable changes (2)
- packages/lib/package.json
- package.json
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{service,repository}.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Avoid dot-suffixes like
.service.tsor.repository.tsfor new files; reserve.test.ts,.spec.ts,.types.tsfor their specific purposes
Files:
apps/api/v2/src/modules/timezones/services/timezones.service.ts
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
**/*.ts: For Prisma queries, only select data you need; never useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
apps/api/v2/src/modules/timezones/services/timezones.service.tspackages/features/cityTimezones/cityTimezonesHandler.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
apps/api/v2/src/modules/timezones/services/timezones.service.tspackages/features/components/timezone-select/TimezoneSelect.tsxpackages/features/cityTimezones/cityTimezonesHandler.ts
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.
Files:
apps/api/v2/src/modules/timezones/services/timezones.service.tspackages/features/components/timezone-select/TimezoneSelect.tsxpackages/features/cityTimezones/cityTimezonesHandler.ts
**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Always use
t()for text localization in frontend code; direct text embedding should trigger a warning
Files:
packages/features/components/timezone-select/TimezoneSelect.tsx
🧠 Learnings (1)
📓 Common learnings
Learnt from: mdm317
PR: calcom/cal.com#23221
File: packages/features/schedules/components/NewScheduleButton.tsx:35-35
Timestamp: 2025-08-20T17:36:46.521Z
Learning: PR #20804 "perf: server-side fetching for /availability" moved availability data fetching from TRPC to server-side in the Cal.com codebase. This means TRPC cache updates like `utils.viewer.availability.list.setData()` have no effect on the availability page, making server actions like `revalidateAvailabilityList()` necessary for proper cache invalidation.
🧬 Code graph analysis (2)
apps/api/v2/src/modules/timezones/services/timezones.service.ts (1)
packages/features/cityTimezones/cityTimezonesHandler.ts (1)
cityTimezonesHandler(9-38)
packages/features/components/timezone-select/TimezoneSelect.tsx (1)
packages/features/cityTimezones/cityTimezonesHandler.ts (1)
cityTimezonesHandler(9-38)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Install dependencies / Yarn install & cache
packages/features/components/timezone-select/TimezoneSelect.tsx
Outdated
Show resolved
Hide resolved
packages/features/components/timezone-select/TimezoneSelect.tsx
Outdated
Show resolved
Hide resolved
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
- Replace outdated tRPC mock with proper cityTimezonesHandler mock - Update test expectations to match component's actual rendering behavior - Fix openMenu function to wait for component loading and use correct option text - All timezone tests now pass with proper mocking approach Co-Authored-By: Volnei Munhoz <volnei.munhoz@gmail.com>
volnei
changed the title
E2E results are ready! |
| const uniqueCities = Object.values(topPopulatedCities); | ||
| /** Add specific overrides in here */ | ||
| uniqueCities.forEach((city) => { | ||
| if (city.city === "London") city.timezone = "Europe/London"; |
There was a problem hiding this comment.
don't we need this condition in the new refactor?
Udit-takkar
left a comment
Udit-takkar
left a comment
There was a problem hiding this comment.
Locally it keeps showing loading(...) next to the timezone
|
I see the PR is in Draft mode and has updated platform libraries - in the meanwhile I needed to update platform libraries for another PR so once PR #23514 is merged then it is needed to merge main into this branch and release platform libraries again. |
|
This PR is being marked as stale due to inactivity. |
volnei
force-pushed
the
perf/remove-city-timezones
branch
from
230615c to
b0e70a3
Compare
Co-Authored-By: Volnei Munhoz <volnei.munhoz@gmail.com>
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
8 issues found across 48 files
Prompt for AI agents (all 8 issues)
Understand the root cause of the following 8 issues and fix them.
<file name="apps/api/v2/src/modules/timezones/services/timezones.service.ts">
<violation number="1" location="apps/api/v2/src/modules/timezones/services/timezones.service.ts:9">
Calling cityTimezonesHandler() directly here removes the Redis cache, so every request now re-fetches the full timezone list from the CDN. Because cityTimezonesHandler performs an uncached remote fetch, this change regresses latency and risks rate-limiting under load—please keep or replace the caching layer.</violation>
</file>
<file name="packages/features/components/timezone-select/TimezoneSelect.tsx">
<violation number="1" location="packages/features/components/timezone-select/TimezoneSelect.tsx:48">
If cityTimezonesHandler rejects (e.g., CDN outage), this await never settles successfully, leaving the select stuck in a loading state and emitting an unhandled rejection. Please handle errors and clear isPending in a finally/catch path.</violation>
</file>
<file name="apps/web/components/apps/paypal/Setup.tsx">
<violation number="1" location="apps/web/components/apps/paypal/Setup.tsx:104">
When opening a link in a new tab, include `noopener` along with `noreferrer` so the external page cannot reach back via `window.opener`.</violation>
<violation number="2" location="apps/web/components/apps/paypal/Setup.tsx:123">
Add `noopener` alongside `noreferrer` on new-tab links to block `window.opener` access and avoid reverse tabnabbing.</violation>
</file>
<file name="packages/app-store-cli/src/build.ts">
<violation number="1" location="packages/app-store-cli/src/build.ts:3">
Dropping this eslint-disable reactivates our no-restricted-imports rule against the lodash import, so linting will now fail unless the directive (or the import) is restored.</violation>
</file>
<file name="apps/web/components/team/screens/Team.tsx">
<violation number="1" location="apps/web/components/team/screens/Team.tsx:42">
Removing the lint suppression here re-enables the `react/no-danger` error for the existing `dangerouslySetInnerHTML` call, so linting will now fail. Please restore the guard (or refactor to avoid `dangerouslySetInnerHTML`) before merging.</violation>
</file>
<file name="packages/features/ee/managed-event-types/lib/handleChildrenEventTypes.ts">
<violation number="1" location="packages/features/ee/managed-event-types/lib/handleChildrenEventTypes.ts:1">
Removing the eslint-disable for `no-restricted-imports` while still importing from "vitest-mock-extended" will cause the linter to fail. Please restore the directive so this import continues to pass lint.</violation>
</file>
<file name="packages/features/cityTimezones/cityTimezonesHandler.test.ts">
<violation number="1" location="packages/features/cityTimezones/cityTimezonesHandler.test.ts:15">
Assigning `global.fetch` directly in the test without restoring it pollutes the global environment, so later suites will still see the mocked fetch and can break. Use a spy/mock that is restored after the test (or restore `global.fetch` in teardown) instead of overwriting it permanently.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
| } else { | ||
| return JSON.parse(cachedTimezones) as CityTimezones; | ||
| } | ||
| return await cityTimezonesHandler(); |
There was a problem hiding this comment.
Calling cityTimezonesHandler() directly here removes the Redis cache, so every request now re-fetches the full timezone list from the CDN. Because cityTimezonesHandler performs an uncached remote fetch, this change regresses latency and risks rate-limiting under load—please keep or replace the caching layer.
Prompt for AI agents
Address the following comment on apps/api/v2/src/modules/timezones/services/timezones.service.ts at line 9:
<comment>Calling cityTimezonesHandler() directly here removes the Redis cache, so every request now re-fetches the full timezone list from the CDN. Because cityTimezonesHandler performs an uncached remote fetch, this change regresses latency and risks rate-limiting under load—please keep or replace the caching layer.</comment>
<file context>
@@ -1,24 +1,11 @@
- } else {
- return JSON.parse(cachedTimezones) as CityTimezones;
- }
+ return await cityTimezonesHandler();
}
}
</file context>
|
|
||
| useEffect(() => { | ||
| (async () => { | ||
| const data = await cityTimezonesHandler(); |
There was a problem hiding this comment.
If cityTimezonesHandler rejects (e.g., CDN outage), this await never settles successfully, leaving the select stuck in a loading state and emitting an unhandled rejection. Please handle errors and clear isPending in a finally/catch path.
Prompt for AI agents
Address the following comment on packages/features/components/timezone-select/TimezoneSelect.tsx at line 48:
<comment>If cityTimezonesHandler rejects (e.g., CDN outage), this await never settles successfully, leaving the select stuck in a loading state and emitting an unhandled rejection. Please handle errors and clear isPending in a finally/catch path.</comment>
<file context>
@@ -40,14 +40,17 @@ export type TimezoneSelectProps = SelectProps & {
+
+ useEffect(() => {
+ (async () => {
+ const data = await cityTimezonesHandler();
+ setData(data);
+ setIsPending(false);
</file context>
| target="_blank" | ||
| href="https://developer.paypal.com/dashboard/applications/live" | ||
| className="text-orange-600 underline"> | ||
| className="text-orange-600 underline" rel="noreferrer"> |
There was a problem hiding this comment.
Add noopener alongside noreferrer on new-tab links to block window.opener access and avoid reverse tabnabbing.
Prompt for AI agents
Address the following comment on apps/web/components/apps/paypal/Setup.tsx at line 123:
<comment>Add `noopener` alongside `noreferrer` on new-tab links to block `window.opener` access and avoid reverse tabnabbing.</comment>
<file context>
@@ -120,7 +120,7 @@ export default function PayPalSetup() {
target="_blank"
href="https://developer.paypal.com/dashboard/applications/live"
- className="text-orange-600 underline">
+ className="text-orange-600 underline" rel="noreferrer">
{t("here")}
</a>
</file context>
| className="text-orange-600 underline" rel="noreferrer"> | |
| className="text-orange-600 underline" rel="noopener noreferrer"> |
| className="text-orange-600 underline" | ||
| target="_blank" | ||
| href="https://developer.paypal.com/api/rest/#link-getclientidandclientsecret"> | ||
| href="https://developer.paypal.com/api/rest/#link-getclientidandclientsecret" rel="noreferrer"> |
There was a problem hiding this comment.
When opening a link in a new tab, include noopener along with noreferrer so the external page cannot reach back via window.opener.
Prompt for AI agents
Address the following comment on apps/web/components/apps/paypal/Setup.tsx at line 104:
<comment>When opening a link in a new tab, include `noopener` along with `noreferrer` so the external page cannot reach back via `window.opener`.</comment>
<file context>
@@ -101,7 +101,7 @@ export default function PayPalSetup() {
className="text-orange-600 underline"
target="_blank"
- href="https://developer.paypal.com/api/rest/#link-getclientidandclientsecret">
+ href="https://developer.paypal.com/api/rest/#link-getclientidandclientsecret" rel="noreferrer">
Link to Paypal developer API REST Setup Guide:
https://developer.paypal.com/api/rest/#link-getclientidandclientsecret
</file context>
| href="https://developer.paypal.com/api/rest/#link-getclientidandclientsecret" rel="noreferrer"> | |
| href="https://developer.paypal.com/api/rest/#link-getclientidandclientsecret" rel="noopener noreferrer"> |
| import chokidar from "chokidar"; | ||
| import fs from "fs"; | ||
| // eslint-disable-next-line no-restricted-imports | ||
There was a problem hiding this comment.
Dropping this eslint-disable reactivates our no-restricted-imports rule against the lodash import, so linting will now fail unless the directive (or the import) is restored.
Prompt for AI agents
Address the following comment on packages/app-store-cli/src/build.ts at line 3:
<comment>Dropping this eslint-disable reactivates our no-restricted-imports rule against the lodash import, so linting will now fail unless the directive (or the import) is restored.</comment>
<file context>
@@ -1,6 +1,6 @@
import chokidar from "chokidar";
import fs from "fs";
-// eslint-disable-next-line no-restricted-imports
+
import { debounce } from "lodash";
import path from "path";
</file context>
| // eslint-disable-next-line no-restricted-imports |
| <div | ||
| className=" text-subtle break-words text-sm [&_a]:text-blue-500 [&_a]:underline [&_a]:hover:text-blue-600" | ||
| // eslint-disable-next-line react/no-danger | ||
There was a problem hiding this comment.
Removing the lint suppression here re-enables the react/no-danger error for the existing dangerouslySetInnerHTML call, so linting will now fail. Please restore the guard (or refactor to avoid dangerouslySetInnerHTML) before merging.
Prompt for AI agents
Address the following comment on apps/web/components/team/screens/Team.tsx at line 42:
<comment>Removing the lint suppression here re-enables the `react/no-danger` error for the existing `dangerouslySetInnerHTML` call, so linting will now fail. Please restore the guard (or refactor to avoid `dangerouslySetInnerHTML`) before merging.</comment>
<file context>
@@ -39,7 +39,7 @@ const Member = ({ member, teamName }: { member: MemberType; teamName: string | n
<div
className=" text-subtle break-words text-sm [&_a]:text-blue-500 [&_a]:underline [&_a]:hover:text-blue-600"
- // eslint-disable-next-line react/no-danger
+
dangerouslySetInnerHTML={{ __html: markdownToSafeHTML(member.bio) }}
/>
</file context>
| // eslint-disable-next-line react/no-danger |
| @@ -1,4 +1,4 @@ | |||
| // eslint-disable-next-line no-restricted-imports | |||
There was a problem hiding this comment.
Removing the eslint-disable for no-restricted-imports while still importing from "vitest-mock-extended" will cause the linter to fail. Please restore the directive so this import continues to pass lint.
Prompt for AI agents
Address the following comment on packages/features/ee/managed-event-types/lib/handleChildrenEventTypes.ts at line 1:
<comment>Removing the eslint-disable for `no-restricted-imports` while still importing from "vitest-mock-extended" will cause the linter to fail. Please restore the directive so this import continues to pass lint.</comment>
<file context>
@@ -1,4 +1,4 @@
-// eslint-disable-next-line no-restricted-imports
+
import type { DeepMockProxy } from "vitest-mock-extended";
</file context>
| // eslint-disable-next-line no-restricted-imports |
| ], | ||
| }); | ||
|
|
||
| global.fetch = mockFetch; |
There was a problem hiding this comment.
Assigning global.fetch directly in the test without restoring it pollutes the global environment, so later suites will still see the mocked fetch and can break. Use a spy/mock that is restored after the test (or restore global.fetch in teardown) instead of overwriting it permanently.
Prompt for AI agents
Address the following comment on packages/features/cityTimezones/cityTimezonesHandler.test.ts at line 15:
<comment>Assigning `global.fetch` directly in the test without restoring it pollutes the global environment, so later suites will still see the mocked fetch and can break. Use a spy/mock that is restored after the test (or restore `global.fetch` in teardown) instead of overwriting it permanently.</comment>
<file context>
@@ -0,0 +1,76 @@
+ ],
+ });
+
+ global.fetch = mockFetch;
+
+ const result = await cityTimezonesHandler();
</file context>
5 participants
What does this PR do?
Replace city-timezones dependency with CDN version to reduce bundle size.