fix: assign SystemInfo to globalThis in background thread#1599
fix: assign SystemInfo to globalThis in background thread#1599PupilTong merged 2 commits intolynx-family:mainfrom
Conversation
🦋 Changeset detectedLatest commit: da760db The changes in this PR will be included in the next version bump. This PR includes changesets to release 7 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughSystemInfo is no longer injected into generated background modules or native app contexts. Workers can receive an optional systemInfo in their start message which is assigned to globalThis.SystemInfo. bootWorkers now accepts and forwards a BrowserConfig and merges it with systemInfo for background initialization; BrowserConfig was removed from one constants type. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (9)
💤 Files with no reviewable changes (4)
🚧 Files skipped from review as they are similar to previous changes (5)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
There was a problem hiding this comment.
Pull Request Overview
This PR fixes SystemInfo assignment in web worker background threads by moving the logic from the background thread initialization to the worker message handler. The change ensures SystemInfo is properly available on globalThis in background threads.
Key changes:
- Move SystemInfo assignment from background thread creation to worker message handler
- Pass browserConfig through the worker boot chain to make it available for SystemInfo
- Remove unused browserConfig parameter from various function signatures
Reviewed Changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/web-platform/web-worker-runtime/src/index.ts | Add systemInfo to WorkerStartMessage interface and assign to globalThis in message handler |
| packages/web-platform/web-worker-runtime/src/backgroundThread/background-apis/createNativeApp.ts | Remove SystemInfo assignment and unused browserConfig parameter |
| packages/web-platform/web-mainthread-apis/src/prepareMainThreadAPIs.ts | Remove unused browserConfig parameter |
| packages/web-platform/web-core/src/uiThread/startUIThread.ts | Pass browserConfig to bootWorkers function |
| packages/web-platform/web-core/src/uiThread/bootWorkers.ts | Add browserConfig parameter and pass systemInfo through worker message |
| packages/web-platform/web-constants/src/utils/generateTemplate.ts | Remove SystemInfo from background inject vars |
| packages/web-platform/web-constants/src/types/BackThreadStartConfigs.ts | Remove browserConfig from BackMainThreadContextConfig interface |
| .changeset/hip-apples-obey.md | Add changeset documenting the SystemInfo fix |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/web-platform/web-core/src/uiThread/bootWorkers.ts (1)
109-115: Bug: truthy check on lynxGroupId breaks for 0; use explicit undefined check
if (lynxGroupId)will treat a valid group id of 0 as “no group,” creating extra workers and leaking contexts.Apply:
- if (lynxGroupId) { + if (lynxGroupId !== undefined) {Note: elsewhere in this file you already use
!== undefined, so this also improves consistency.
🧹 Nitpick comments (6)
.changeset/hip-apples-obey.md (1)
8-8: Clarify changeset message; avoid abbreviations and improve scope prefix“bts” is ambiguous. Prefer a clear, conventional subject scoped to the affected package(s).
Apply this diff for clarity:
-fix: the SystemInfo in bts should be assigned to the globalThis +fix(web-worker-runtime): assign SystemInfo to globalThis during background thread startupOptionally add a short body sentence explaining that SystemInfo is now provided via WorkerStartMessage and assigned once at worker init.
packages/web-platform/web-mainthread-apis/src/global.d.ts (1)
1-4: Tighten the global type and mark file as a module to remove TS frictionUsing
anyreduces safety, and the global may legitimately be undefined until the worker initializes. Also, addingexport {}ensures the.d.tsis treated as a module, avoiding accidental global augment bleed.Apply:
declare global { // eslint-disable-next-line no-var - var SystemInfo: Record<string, any>; + var SystemInfo: Readonly<Record<string, string | number>> | undefined; } +export {};This matches the merged shape { ...systemInfo, ...browserConfig } and allows code to guard for undefined at startup.
packages/web-platform/web-worker-runtime/src/index.ts (1)
12-13: NarrowWorkerStartMessage.systemInfotype for better safetyAlign the payload with your constants’ shape and avoid
any.- systemInfo?: Record<string, any>; + systemInfo?: Readonly<Record<string, string | number>>;packages/web-platform/web-core/src/uiThread/bootWorkers.ts (2)
105-121: Confirm the merged payload shape and precedence; consider compile-time assertionYou’re flattening
{ ...systemInfo, ...browserConfig }with browserConfig taking precedence on key collisions. If that precedence is intentional, consider asserting the final type at compile time to catch regressions.- systemInfo: { ...systemInfo, ...browserConfig }, + systemInfo: { ...systemInfo, ...browserConfig } as Readonly<Record<string, string | number>>,If collisions are not desired, consider nesting under
browser: browserConfiginstead; otherwise, document precedence.
80-100: Optional: preheated worker lifecycle edge casesReassigning
preHeatedMainWorker = createMainWorker()on every boot may accumulate workers if callers churn quickly. If that’s expected, ignore; otherwise consider a small pool or timeout-based reuse.packages/web-platform/web-core/src/uiThread/startUIThread.ts (1)
32-48: Double-check that configs.browserConfig is always present at runtimeIf
configscan come from older callers lackingbrowserConfig, add a defensive default to avoid postingundefinedand leaving SystemInfo unset in the worker.Lightweight guard:
-} = bootWorkers(lynxGroupId, allOnUI, configs.browserConfig); +} = bootWorkers( + lynxGroupId, + allOnUI, + (configs as any).browserConfig ?? { pixelRatio: devicePixelRatio, pixelWidth: innerWidth, pixelHeight: innerHeight } +);Prefer a proper typed default via StartMainThreadContextConfig if available.
📜 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 selected for processing (9)
.changeset/hip-apples-obey.md(1 hunks)packages/web-platform/web-constants/src/types/BackThreadStartConfigs.ts(0 hunks)packages/web-platform/web-constants/src/utils/generateTemplate.ts(0 hunks)packages/web-platform/web-core/src/uiThread/bootWorkers.ts(5 hunks)packages/web-platform/web-core/src/uiThread/startUIThread.ts(1 hunks)packages/web-platform/web-mainthread-apis/src/global.d.ts(1 hunks)packages/web-platform/web-mainthread-apis/src/prepareMainThreadAPIs.ts(0 hunks)packages/web-platform/web-worker-runtime/src/backgroundThread/background-apis/createNativeApp.ts(0 hunks)packages/web-platform/web-worker-runtime/src/index.ts(1 hunks)
💤 Files with no reviewable changes (4)
- packages/web-platform/web-constants/src/utils/generateTemplate.ts
- packages/web-platform/web-constants/src/types/BackThreadStartConfigs.ts
- packages/web-platform/web-mainthread-apis/src/prepareMainThreadAPIs.ts
- packages/web-platform/web-worker-runtime/src/backgroundThread/background-apis/createNativeApp.ts
🧰 Additional context used
🧬 Code graph analysis (3)
packages/web-platform/web-worker-runtime/src/index.ts (1)
packages/web-platform/web-constants/src/constants.ts (1)
systemInfo(40-43)
packages/web-platform/web-core/src/uiThread/startUIThread.ts (2)
packages/web-platform/web-core/src/uiThread/bootWorkers.ts (1)
bootWorkers(19-66)packages/web-platform/web-core/src/apis/LynxView.ts (2)
lynxGroupId(257-261)lynxGroupId(262-268)
packages/web-platform/web-core/src/uiThread/bootWorkers.ts (2)
packages/web-platform/web-constants/src/types/PageConfig.ts (1)
BrowserConfig(12-16)packages/web-platform/web-constants/src/constants.ts (1)
systemInfo(40-43)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build / Build (Ubuntu)
- GitHub Check: build / Build (Windows)
- GitHub Check: test-rust / Test (Ubuntu)
- GitHub Check: CodeQL Analyze (javascript-typescript)
🔇 Additional comments (4)
packages/web-platform/web-worker-runtime/src/index.ts (1)
15-22: No top-level SystemInfo accesses in worker runtimeThe ripgrep search did not identify any
SystemInforeads outside of theonmessagehandler inpackages/web-platform/web-worker-runtime. The only occurrence in that package is the assignment guarded by theif (!globalThis.SystemInfo)check within the handler. There are no top-level references that would run before worker startup.packages/web-platform/web-core/src/uiThread/bootWorkers.ts (2)
35-39: LGTM: passing browserConfig through to the background workerThis cleanly centralizes environment info into a single message.
21-23: AllbootWorkerscall sites now includebrowserConfig
- Verified the only TS/TSX invocation in
packages/web-platform/web-core/src/uiThread/startUIThread.ts:47
now passesconfigs.browserConfig- No JS/JSX call sites found
- Confirmed
StartMainThreadContextConfig(in
packages/web-platform/web-constants/src/types/MainThreadStartConfigs.ts:16)
declaresbrowserConfig: BrowserConfigNo further action required.
packages/web-platform/web-core/src/uiThread/startUIThread.ts (1)
47-47: LGTM: bootWorkers now receives browserConfigThe call-site update matches the new signature and keeps UI-thread concerns minimal.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/web-platform/web-worker-runtime/src/global.d.ts (3)
2-2: Tighten the type: preferunknownoveranyforSystemInfo.
anydefeats type safety.unknownforces call sites to narrow before use and still supports a flexible shape.Apply this minimal change:
- var SystemInfo: Record<string, any> | undefined; + var SystemInfo: Record<string, unknown> | undefined;Optional: add a brief JSDoc to document lifecycle and intent (read-mostly, assigned once at worker boot):
/** * Assigned once during worker boot from WorkerStartMessage and read thereafter. * Access via `globalThis.SystemInfo`. */
2-2: Prefer consistent access viaglobalThis.SystemInfo.To avoid accidental shadowing by a local
SystemInfovariable, encourage code to read viaglobalThis.SystemInfoeverywhere.
3-3: Remove unused globalmoduledeclaration in web-worker-runtimeThe
var module: { exports: any }in
packages/web-platform/web-worker-runtime/src/global.d.tsis never referenced by any code in this package (all internal uses ofmoduleare local parameters in dynamic imports). It therefore only serves to mask missing runtime errors and isn’t needed in the Web Worker environment. You can safely delete it:• File:
packages/web-platform/web-worker-runtime/src/global.d.ts
• Remove this line:- var module: { exports: any };
📜 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 selected for processing (2)
packages/web-platform/web-worker-runtime/src/global.d.ts(1 hunks)packages/web-platform/web-worker-runtime/src/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/web-platform/web-worker-runtime/src/index.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build / Build (Ubuntu)
- GitHub Check: build / Build (Windows)
- GitHub Check: test-rust / Test (Ubuntu)
🔇 Additional comments (2)
packages/web-platform/web-worker-runtime/src/global.d.ts (2)
1-6: Ambient global augmentation pattern is correct.Using
declare global { ... }with a trailingexport {}is the right way to surfaceglobalThisproperties while keeping this file a module. No runtime side effects introduced.
6-6: Global augmentation file is already included in the TS buildThe
tsconfig.jsonforpackages/web-platform/web-worker-runtimehas"include": ["src"], which coverssrc/global.d.ts. The globalSystemInfoaugmentation will therefore be picked up by the compiler—no further action needed.
React Example#4571 Bundle Size — 237.06KiB (0%).da760db(current) vs 1777bb4 main#4553(baseline) Bundle metrics
|
| Current #4571 |
Baseline #4553 |
|
|---|---|---|
 Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
| Cache Invalidation |
0% |
0% |
| Chunks |
0 |
0 |
| Assets |
4 |
4 |
| Modules |
158 |
158 |
| Duplicate Modules |
64 |
64 |
| Duplicate Code |
45.83% |
45.83% |
| Packages |
2 |
2 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #4571 |
Baseline #4553 |
|
|---|---|---|
| IMG |
145.76KiB |
145.76KiB |
| Other |
91.3KiB |
91.3KiB |
Bundle analysis report Branch PupilTong:p/hw/global-systeminfo Project dashboard
Generated by RelativeCI Documentation Report issue
Web Explorer#4564 Bundle Size — 368.21KiB (~+0.01%).da760db(current) vs 1777bb4 main#4546(baseline) Bundle metrics
|
| Current #4564 |
Baseline #4546 |
|
|---|---|---|
 Initial JS |
144.3KiB(+0.03%) |
144.25KiB |
| Initial CSS |
31.84KiB |
31.84KiB |
 Cache Invalidation |
61.85% |
51.51% |
| Chunks |
8 |
8 |
| Assets |
8 |
8 |
| Modules |
220(-0.45%) |
221 |
| Duplicate Modules |
17 |
17 |
| Duplicate Code |
3.85% |
3.85% |
| Packages |
4 |
4 |
| Duplicate Packages |
0 |
0 |
Bundle size by type 1 change 1 regression
| Current #4564 |
Baseline #4546 |
|
|---|---|---|
| JS |
236.21KiB (+0.01%) |
236.18KiB |
| Other |
100.16KiB |
100.16KiB |
| CSS |
31.84KiB |
31.84KiB |
Bundle analysis report Branch PupilTong:p/hw/global-systeminfo Project dashboard
Generated by RelativeCI Documentation Report issue
CodSpeed Performance ReportMerging #1599 will not alter performanceComparing Summary
|
PupilTong
force-pushed
the
p/hw/global-systeminfo
branch
from
e581a30 to
da760db
Compare
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @lynx-js/rspeedy@0.11.0 ### Minor Changes - Deprecate `source.alias`, use `resolve.alias` instead. ([#1610](#1610)) Note that `resolve.alias` has **lower** priority than the deprecated `source.alias`. - Bump Rsbuild v1.5.0 with Rspack v1.5.0. ([#1591](#1591)) - **BREAKING CHANGE**: Remove the `./register` exports from `@lynx-js/rspeedy`. ([#1547](#1547)) This should not affect most users. ### Patch Changes - Support `resolve.alias`. ([#1610](#1610)) - Support `rspeedy build --watch` ([#1579](#1579)) - Updated dependencies \[[`d7d0b9b`](d7d0b9b), [`1952fc1`](1952fc1)]: - @lynx-js/cache-events-webpack-plugin@0.0.2 - @lynx-js/chunk-loading-webpack-plugin@0.3.2 ## @lynx-js/web-constants@0.16.0 ### Minor Changes - refactor: provide the mts a real globalThis ([#1589](#1589)) Before this change, We create a function wrapper and a fake globalThis for Javascript code. This caused some issues. After this change, we will create an iframe for createing an isolated Javascript context. This means the globalThis will be the real one. ### Patch Changes - refactor: add `:not([l-e-name])` at the end of selector for lazy component ([#1622](#1622)) - fix: the SystemInfo in bts should be assigned to the globalThis ([#1599](#1599)) - Updated dependencies \[[`c1f8715`](c1f8715)]: - @lynx-js/web-worker-rpc@0.16.0 ## @lynx-js/web-core@0.16.0 ### Minor Changes - refactor: provide the mts a real globalThis ([#1589](#1589)) Before this change, We create a function wrapper and a fake globalThis for Javascript code. This caused some issues. After this change, we will create an iframe for createing an isolated Javascript context. This means the globalThis will be the real one. ### Patch Changes - refactor: add `:not([l-e-name])` at the end of selector for lazy component ([#1622](#1622)) - feat: remove multi-thread mts heating ([#1597](#1597)) The default rendering mode is "all-on-ui". Therefore the preheating for "multi-thread" will be removed. - fix: the SystemInfo in bts should be assigned to the globalThis ([#1599](#1599)) - Updated dependencies \[[`1a32dd8`](1a32dd8), [`bb53d9a`](bb53d9a), [`1a32dd8`](1a32dd8), [`c1f8715`](c1f8715)]: - @lynx-js/web-mainthread-apis@0.16.0 - @lynx-js/web-constants@0.16.0 - @lynx-js/web-worker-runtime@0.16.0 - @lynx-js/offscreen-document@0.1.4 - @lynx-js/web-worker-rpc@0.16.0 ## @lynx-js/web-core-server@0.16.0 ### Minor Changes - refactor: provide the mts a real globalThis ([#1589](#1589)) Before this change, We create a function wrapper and a fake globalThis for Javascript code. This caused some issues. After this change, we will create an iframe for createing an isolated Javascript context. This means the globalThis will be the real one. ## @lynx-js/web-mainthread-apis@0.16.0 ### Minor Changes - refactor: provide the mts a real globalThis ([#1589](#1589)) Before this change, We create a function wrapper and a fake globalThis for Javascript code. This caused some issues. After this change, we will create an iframe for createing an isolated Javascript context. This means the globalThis will be the real one. ### Patch Changes - refactor: add `:not([l-e-name])` at the end of selector for lazy component ([#1622](#1622)) - fix: the SystemInfo in bts should be assigned to the globalThis ([#1599](#1599)) - Updated dependencies \[[`1a32dd8`](1a32dd8), [`bb53d9a`](bb53d9a), [`c1f8715`](c1f8715)]: - @lynx-js/web-constants@0.16.0 - @lynx-js/web-style-transformer@0.16.0 ## @lynx-js/web-worker-rpc@0.16.0 ### Minor Changes - refactor: provide the mts a real globalThis ([#1589](#1589)) Before this change, We create a function wrapper and a fake globalThis for Javascript code. This caused some issues. After this change, we will create an iframe for createing an isolated Javascript context. This means the globalThis will be the real one. ## @lynx-js/web-worker-runtime@0.16.0 ### Minor Changes - refactor: provide the mts a real globalThis ([#1589](#1589)) Before this change, We create a function wrapper and a fake globalThis for Javascript code. This caused some issues. After this change, we will create an iframe for createing an isolated Javascript context. This means the globalThis will be the real one. ### Patch Changes - fix: the SystemInfo in bts should be assigned to the globalThis ([#1599](#1599)) - Updated dependencies \[[`1a32dd8`](1a32dd8), [`bb53d9a`](bb53d9a), [`1a32dd8`](1a32dd8), [`c1f8715`](c1f8715)]: - @lynx-js/web-mainthread-apis@0.16.0 - @lynx-js/web-constants@0.16.0 - @lynx-js/offscreen-document@0.1.4 - @lynx-js/web-worker-rpc@0.16.0 ## @lynx-js/react@0.112.5 ### Patch Changes - Remove the "key is not on root element of snapshot" warning. ([#1558](#1558)) ## create-rspeedy@0.11.0 ### Patch Changes - Use TypeScript [Project References](https://www.typescriptlang.org/docs/handbook/project-references.html) in templates. ([#1612](#1612)) ## @lynx-js/offscreen-document@0.1.4 ### Patch Changes - remove innerHTML, replace it by textContent ([#1622](#1622)) ## @lynx-js/cache-events-webpack-plugin@0.0.2 ### Patch Changes - Fix that `__webpack_require__.lynx_ce` is incorrectly injected when lazy bundle is enabled. ([#1616](#1616)) ## @lynx-js/chunk-loading-webpack-plugin@0.3.2 ### Patch Changes - Should not load initial CSS chunks. ([#1601](#1601)) ## upgrade-rspeedy@0.11.0 ## @lynx-js/web-style-transformer@0.16.0 Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Summary by CodeRabbit
Bug Fixes
Refactor
Chores
Checklist