chore: trim useless files for client#14488
Merged
darkskygit merged 3 commits intocanaryfrom Feb 22, 2026
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds Electron build-time locale trimming (removes unneeded .lproj and .pak locale files) integrated into the Forge afterCopy hook; also reformats a TypeScript union type for brevity. Changes
Sequence Diagram(s)sequenceDiagram
participant Forge as Electron Forge Packager
participant Hook as afterCopy Hook
participant TrimmerFW as trimElectronFrameworkLocales
participant TrimmerPAK as trimElectronPakLocales
participant FS as File System
Forge->>Hook: invoke afterCopy(filesPath, platform)
alt platform is darwin or mas
Hook->>TrimmerFW: pass frameworkPath & keep list
TrimmerFW->>FS: read framework resources directories
TrimmerFW->>FS: remove non-kept `.lproj` directories
TrimmerFW-->>Hook: done
else platform is win or linux
Hook->>TrimmerPAK: pass localesPath & keep list
TrimmerPAK->>FS: read locales directory
TrimmerPAK->>FS: remove non-kept `.pak` files
TrimmerPAK-->>Hook: done
end
Hook-->>Forge: post-copy cleanup complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/frontend/apps/electron/forge.config.mjs (1)
55-74: Env-var locale names are not case-normalised — mismatches silently skip deletion on Linux.
raw.split(',').map(s => s.trim())preserves the caller's casing. For the macOS path, the case-insensitive filesystem hides the problem. For Linux.paktrimming,'zh_cn'converts to'zh-cn'(not'zh-CN'), sopakKeep.has('zh-cn')won't match Chromium's actualzh-CN.pak, and that file will be skipped silently.✨ Suggested improvement
const keep = new Set( raw .split(',') .map(s => s.trim()) .filter(Boolean) ); +// Normalise locale names that users may supply in wrong case. +// The DEFAULT set uses the canonical Chromium casing (e.g. 'zh_CN', 'en_GB'). +// We can't do a full case-fold because locale codes are case-significant +// (upper-case region suffix is the convention), so at minimum warn: +if (process.env.NODE_ENV !== 'production') { + for (const l of keep) { + if (l !== l.replace(/[a-z]{2}$/, s => s.toUpperCase())) { + console.warn(`[forge] ELECTRON_LOCALES_KEEP: "${l}" may need a capitalised region suffix (e.g. zh_CN)`); + } + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/frontend/apps/electron/forge.config.mjs` around lines 55 - 74, The getElectronLocalesKeep function builds the keep Set from ELECTRON_LOCALES_KEEP but preserves caller casing, causing mismatches (e.g., 'zh_cn' vs 'zh-CN'); update the mapping in getElectronLocalesKeep to normalize each entry before adding to the Set by applying .toLowerCase() and normalizing separators (e.g., replace '_' with '-') so comparisons against Chromium .pak names succeed, and ensure the forced fallbacks ('en', 'en_US', 'en_GB') are added in the same normalized form; leave DEFAULT_ELECTRON_LOCALES_KEEP and the null/"all" handling unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/frontend/apps/electron/forge.config.mjs`:
- Around line 29-53: DEFAULT_ELECTRON_LOCALES_KEEP omits 'en_US' while
getElectronLocalesKeep force-adds 'en_US' when an env-var override is present;
update DEFAULT_ELECTRON_LOCALES_KEEP to include 'en_US' so default (no env-var)
builds behave consistently and won't remove an en_US.lproj if Electron ever
ships it, ensuring both the constant and getElectronLocalesKeep logic align.
---
Nitpick comments:
In `@packages/frontend/apps/electron/forge.config.mjs`:
- Around line 55-74: The getElectronLocalesKeep function builds the keep Set
from ELECTRON_LOCALES_KEEP but preserves caller casing, causing mismatches
(e.g., 'zh_cn' vs 'zh-CN'); update the mapping in getElectronLocalesKeep to
normalize each entry before adding to the Set by applying .toLowerCase() and
normalizing separators (e.g., replace '_' with '-') so comparisons against
Chromium .pak names succeed, and ensure the forced fallbacks ('en', 'en_US',
'en_GB') are added in the same normalized form; leave
DEFAULT_ELECTRON_LOCALES_KEEP and the null/"all" handling unchanged.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## canary #14488 +/- ##
==========================================
- Coverage 57.41% 56.93% -0.48%
==========================================
Files 2860 2860
Lines 154542 154542
Branches 23351 23222 -129
==========================================
- Hits 88724 87988 -736
- Misses 62901 63629 +728
- Partials 2917 2925 +8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/frontend/apps/electron/forge.config.mjs (1)
345-354: Consider anchoring (or documenting) the broad directory-name ignore patterns.Electron Packager's
ignorepatterns are matched against the absolute path of every file/directory being copied — including paths insidenode_modules. The three unanchored patterns:/\/src($|\/)/ /\/test($|\/)/ /\/scripts($|\/)/will silently exclude any
src/,test/, orscripts/subdirectory found anywhere undernode_modules, not just those at the project root. This is harmless for the vast majority of npm packages, but if any production dependency ever ships runtime assets under one of those directory names it would be stripped without warning.Anchoring them to the project root (
^prefix) would make the intent explicit and avoid the ambiguity:♻️ Suggested anchoring
- /\/src($|\/)/, - /\/test($|\/)/, - /\/scripts($|\/)/, + /^\/src($|\/)/, + /^\/test($|\/)/, + /^\/scripts($|\/)/,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/frontend/apps/electron/forge.config.mjs` around lines 345 - 354, The ignore array in forge.config.mjs currently uses unanchored patterns (/\/src($|\/)/, /\/test($|\/)/, /\/scripts($|\/)/) which will match those directory names anywhere in absolute paths (including node_modules); update the ignore entries in the ignore: [...] block to anchor them to the project root (prefix with ^) so they only match root-level directories, or alternatively add a clear comment above the ignore array documenting that these patterns are intentionally global and may strip matching dirs inside node_modules; target the ignore array and the specific regex entries to make this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/frontend/apps/electron/forge.config.mjs`:
- Around line 345-354: The ignore array in forge.config.mjs currently uses
unanchored patterns (/\/src($|\/)/, /\/test($|\/)/, /\/scripts($|\/)/) which
will match those directory names anywhere in absolute paths (including
node_modules); update the ignore entries in the ignore: [...] block to anchor
them to the project root (prefix with ^) so they only match root-level
directories, or alternatively add a clear comment above the ignore array
documenting that these patterns are intentionally global and may strip matching
dirs inside node_modules; target the ignore array and the specific regex entries
to make this change.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/frontend/apps/electron/forge.config.mjs (1)
114-119: Silent catch swallows non-ENOENTerrors with no build signal.Both
readdircatch blocksreturnunconditionally, meaning a permission error or unexpected FS issue silently disables locale trimming and the build succeeds with the full locale set included. Adding a warning would surface these cases.♻️ Proposed improvement for both catch blocks
- } catch { - return; - } + } catch (err) { + if (err?.code !== 'ENOENT') { + console.warn('[locale-trim] readdir failed, skipping trim:', err); + } + return; + }Also applies to: 146-151
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/frontend/apps/electron/forge.config.mjs` around lines 114 - 119, The try/catch around the readdir calls (when reading frameworkResourcesDir and the other locale directory) currently swallows all errors by returning silently; update both catch blocks to capture the thrown error (e.g., catch (err)) and log a warning that includes the error details and context (which directory failed) instead of unconditionally returning, and only return early when the error is ENOENT; reference the readdir calls and the entries variable and include the directory name (frameworkResourcesDir / the other locale dir) in the log so permission or unexpected FS errors are visible during build.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/frontend/apps/electron/forge.config.mjs`:
- Around line 114-119: The try/catch around the readdir calls (when reading
frameworkResourcesDir and the other locale directory) currently swallows all
errors by returning silently; update both catch blocks to capture the thrown
error (e.g., catch (err)) and log a warning that includes the error details and
context (which directory failed) instead of unconditionally returning, and only
return early when the error is ENOENT; reference the readdir calls and the
entries variable and include the directory name (frameworkResourcesDir / the
other locale dir) in the log so permission or unexpected FS errors are visible
during build.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR Dependency Tree
This tree was auto-generated by Charcoal
Summary by CodeRabbit