Skip to content

Copy of PR #22496: revert: "refactor: replace i18n HTTP requests with build-time bundling (#22422)"#1

Open
dakshgup wants to merge 1 commit intomainfrom
copy-pr-22496-revert-i18n-pr
Open

Copy of PR #22496: revert: "refactor: replace i18n HTTP requests with build-time bundling (#22422)"#1
dakshgup wants to merge 1 commit intomainfrom
copy-pr-22496-revert-i18n-pr

Conversation

@dakshgup
Copy link
Owner

This is a copy of calcom#22496

Originally by: hbjORbj

This reverts commit 163c7ff.

Summary by CodeRabbit

  • Refactor
    • Updated all localization logic and configuration to load translation files from the web app's public static directory instead of server-side directories.
    • Removed unused scripts and code related to previous translation bundling and copying mechanisms.
    • Adjusted build and test configurations to align with new locale file locations.
  • Style
    • Minor formatting changes in locale JSON files (e.g., removal of trailing newlines, spacing adjustments).
  • Chores
    • Updated workflow and configuration files to reflect new paths for localization resources.

…s with build-time bundling (calcom#22422)"

Originally by: hbjORbj
This reverts commit 163c7ff.

<!-- This is an auto-generated comment: release notes by coderabbit.ai -->

## Summary by CodeRabbit

* **Refactor**
  * Updated all localization logic and configuration to load translation files from the web app's public static directory instead of server-side directories.
  * Removed unused scripts and code related to previous translation bundling and copying mechanisms.
  * Adjusted build and test configurations to align with new locale file locations.
* **Style**
  * Minor formatting changes in locale JSON files (e.g., removal of trailing newlines, spacing adjustments).
* **Chores**
  * Updated workflow and configuration files to reflect new paths for localization resources.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->
@github-actions
Copy link

Hey there and thank you for opening this pull request! 👋🏼

We require pull request titles to follow the Conventional Commits specification and it looks like your proposed title needs to be adjusted.

Details:

No release type found in pull request title "Copy of PR #22496: revert: "refactor: replace i18n HTTP requests with build-time bundling (#22422)"". Add a prefix to indicate what kind of release this pull request corresponds to. For reference, see https://www.conventionalcommits.org/

Available types:
 - feat: A new feature
 - fix: A bug fix
 - docs: Documentation only changes
 - style: Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc)
 - refactor: A code change that neither fixes a bug nor adds a feature
 - perf: A code change that improves performance
 - test: Adding missing tests or correcting existing tests
 - build: Changes that affect the build system or external dependencies (example scopes: gulp, broccoli, npm)
 - ci: Changes to our CI configuration files and scripts (example scopes: Travis, Circle, BrowserStack, SauceLabs)
 - chore: Other changes that don't modify src or test files
 - revert: Reverts a previous commit

@dakshgup
Copy link
Owner Author

cursor review

Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Bug: SSR Translation Fetching Issues

Translation loading fails during server-side rendering (SSR) because the mechanism changed from local bundling to fetching translations via HTTP from WEBAPP_URL. This can lead to failures if the WEBAPP_URL is inaccessible from the server's environment or if the server attempts to fetch assets before they are fully available.

packages/lib/server/i18n.ts#L12-L27

*/
async function loadFallbackTranslations() {
const cacheKey = "en-common";
if (translationCache.has(cacheKey)) {
return translationCache.get(cacheKey);
}
try {
const res = await fetch(`${WEBAPP_URL}/static/locales/en/common.json`, {
cache: process.env.NODE_ENV === "production" ? "force-cache" : "no-store",
});
if (!res.ok) {
throw new Error(`Failed to fetch fallback translations: ${res.status}`);
}

Fix in CursorFix in Web


BugBot free trial expires on July 22, 2025
Learn more in the Cursor dashboard.

Was this report helpful? Give feedback by reacting with 👍 or 👎

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Greptile Summary

This PR is a reversion of a previous change (calcom#22422) that had moved i18n (internationalization) from HTTP requests to build-time bundling. The key changes include:

  1. Moving translation files from server-side (packages/lib/server/locales/) back to client-side (apps/web/public/static/locales/)
  2. Removing the translation bundling script and related build-time optimizations
  3. Updating all import paths and configurations to point to the new static file locations
  4. Standardizing line endings across all locale files by removing trailing newlines

The reversion suggests that the build-time bundling approach introduced issues or limitations that made the HTTP-based translation loading preferable. This change maintains more flexibility in translation updates without requiring full rebuilds.

Confidence score: 4 /5

  1. This PR is safe to merge as it reverts to a previously working implementation
  2. High confidence due to comprehensive changes across configuration, paths, and consistent formatting updates
  3. Key files to review:
    • packages/lib/server/i18n.ts: Verify translation loading logic
    • apps/web/next.config.js: Check updated translation import paths
    • i18n.json: Confirm locale configuration paths

53 files reviewed, 7 comments
Edit PR Review Bot Settings | Greptile

@@ -62,9 +62,9 @@ export default defineConfig(({ mode }) => {
"@calcom/platform-constants": path.resolve(__dirname, "../constants/index.ts"),
"@calcom/platform-types": path.resolve(__dirname, "../types/index.ts"),
"@calcom/platform-utils": path.resolve(__dirname, "../constants/index.ts"),
Copy link

Choose a reason for hiding this comment

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

logic: @calcom/platform-utils alias points to constants/index.ts - should this be utils/index.ts instead?

Comment on lines +71 to 74
let translations;
if (eventLocale) {
const ns = "common";
translations = await loadTranslations(eventLocale, ns);
Copy link

Choose a reason for hiding this comment

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

style: Redundant 'ns' declaration. Use the one defined on line 70.

Suggested change
let translations;
if (eventLocale) {
const ns = "common";
translations = await loadTranslations(eventLocale, ns);
let translations;
if (eventLocale) {
translations = await loadTranslations(eventLocale, ns);

@@ -32,7 +32,7 @@ const ServerPage = async ({ params, searchParams }: ServerPageProps) => {

const eventLocale = props.eventData?.interfaceLanguage;
const ns = "common";
Copy link

Choose a reason for hiding this comment

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

style: ns is redeclared on line 37. Use the existing declaration

Suggested change
const ns = "common";
const eventLocale = props.eventData?.interfaceLanguage;
const ns = "common";
let translations;
if (eventLocale) {
translations = await loadTranslations(eventLocale, ns);
}

Comment on lines 34 to +35
const ns = "common";
let translations: Record<string, string> = {};
let translations;
Copy link

Choose a reason for hiding this comment

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

logic: Duplicate declaration of 'ns' constant on line 37. Remove redundant declaration.

Suggested change
const ns = "common";
let translations: Record<string, string> = {};
let translations;
let translations;
if (eventLocale) {
const ns = "common";
translations = await loadTranslations(eventLocale, ns);
}

let translations: Record<string, string> = {};
let translations;
if (eventLocale) {
const ns = "common";
Copy link

Choose a reason for hiding this comment

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

logic: Duplicate declaration of 'ns' constant that was already declared on line 70

Suggested change
const ns = "common";
let translations;
if (eventLocale) {
translations = await loadTranslations(eventLocale, ns);

const config = {
// localesPath: localePath, // uncomment to run on all locales (to calculate kb savings)
localesPath: path.join("./packages/lib/server/locales", "/en"),
localesPath: path.join("./apps/website", "/public/static/locales", "/en"),
Copy link

Choose a reason for hiding this comment

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

style: Double forward slashes in path.join() are unnecessary and may cause path resolution issues

Suggested change
localesPath: path.join("./apps/website", "/public/static/locales", "/en"),
localesPath: path.join("./apps/website", "public/static/locales", "en"),

Comment on lines 7 to +8
...i18nConfig,
localePath: path.resolve("../../packages/lib/server/locales"),
localePath: path.resolve("../../web/public/static/locales"),
Copy link

Choose a reason for hiding this comment

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

style: Consider adding a comment explaining why we're using the web app's public directory for API locales

*/
async function loadFallbackTranslations(): Promise<Record<string, string>> {
const cacheKey = `en-common-${CALCOM_VERSION}`;
async function loadFallbackTranslations() {
Copy link

Choose a reason for hiding this comment

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

style: The return type Promise<Record<string, string>> is missing from the function declaration

Suggested change
async function loadFallbackTranslations() {
async function loadFallbackTranslations(): Promise<Record<string, string>> {

* @returns {Promise<Record<string, string>>} Translations object or fallback translations on failure
*/
export async function loadTranslations(_locale: string, ns: string): Promise<Record<string, string>> {
export async function loadTranslations(_locale: string, ns: string) {
Copy link

Choose a reason for hiding this comment

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

style: Return type annotation should be explicit for public exported functions

Suggested change
export async function loadTranslations(_locale: string, ns: string) {
export async function loadTranslations(_locale: string, ns: string): Promise<Record<string, string>> {

@@ -3360,4 +3360,4 @@
"license_key_saved": "লাইসেন্স কী সফলভাবে সংরক্ষিত হয়েছে",
"timezone_mismatch_tooltip": "আপনি আপনার প্রোফাইল টাইমজোন ({{userTimezone}}) অনুসারে রিপোর্ট দেখছেন, যখন আপনার ব্রাউজার টাইমজোন ({{browserTimezone}}) এ সেট করা আছে",
"ADD_NEW_STRINGS_ABOVE_THIS_LINE_TO_PREVENT_MERGE_CONFLICTS": "↑↑↑↑ুষান্ত"
Copy link

Choose a reason for hiding this comment

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

style: The 'ADD_NEW_STRINGS_ABOVE_THIS_LINE_TO_PREVENT_MERGE_CONFLICTS' key includes Bengali characters that appear to be incorrectly appended

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Greptile Summary

This PR addresses some potential issues with the i18n HTTP request handling by updating the paths and error handling in several key components. Notable changes include:

  1. Improved error handling in translation loading by adding descriptive error messages
  2. Simplified caching logic by removing version-specific cache keys
  3. Adjusted how translations are loaded in the booking page wrapper
  4. Modified the translation loading logic to be more resilient to missing translations

These changes focus on making the translation system more robust and easier to maintain by removing complexity from the caching mechanism and improving error handling.

Confidence score: 3 /5

  1. The changes require careful testing of translation loading in various scenarios
  2. Score reflects the sensitive nature of i18n changes and potential impact on user experience
  3. Files needing attention:
    • apps/web/app/(booking-page-wrapper)/org/[orgSlug]/[user]/[type]/page.tsx
    • packages/lib/server/i18n.ts
    • Any components that dynamically load translations

56 files reviewed, no comments
Edit PR Review Bot Settings | Greptile

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.

1 participant