Skip to content

fix(react/runtime): prevent multiple firstScreen events when reloadin…#377

Merged
Yradex merged 1 commit intolynx-family:mainfrom
Yradex:lifecycle/fix
Mar 31, 2025
Merged

fix(react/runtime): prevent multiple firstScreen events when reloadin…#377
Yradex merged 1 commit intolynx-family:mainfrom
Yradex:lifecycle/fix

Conversation

@Yradex
Copy link
Collaborator

@Yradex Yradex commented Mar 28, 2025

Summary

This PR fixes an issue where multiple firstScreen events were being sent to the Background Thread (BT) when reloading before the jsReady event is triggered. This could cause problems with page rendering and state management.

The changes include:

  • Centralizing JS Ready state management in a dedicated module
  • Adding proper state checks before triggering firstScreen events
  • Refactoring lifecycle event handling for better maintainability
  • Adding test coverage for the reload-before-jsReady scenario

Checklist

  • Tests updated (or not required).
  • Documentation updated (or not required).

@changeset-bot
Copy link

changeset-bot bot commented Mar 28, 2025 •
edited
Loading

🦋 Changeset detected

Latest commit: 8135e6e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@lynx-js/react Patch

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

@codecov
Copy link

codecov bot commented Mar 28, 2025 •
edited
Loading

❌ 1 Tests Failed:

Tests completed Failed Passed Skipped
1981 1 1980 60
View the top 2 failed test(s) by shortest run time 
web-elements.spec.ts web-elements test suite › x-list › initial-scroll-index
Stack Traces | 3.81s run time 
web-elements.spec.ts:1634:5 initial-scroll-index
react.spec.ts reactlynx3 tests › basic › basic-setstate-in-constructor
Stack Traces | 6.88s run time 
react.spec.ts:103:5 basic-setstate-in-constructor

To view more test analytics, go to the Test Analytics Dashboard
📋 Got 3 mins? Take this short survey to help us improve Test Analytics.

@codecov
Copy link

codecov bot commented Mar 28, 2025 •
edited
Loading

Bundle Report

Changes will increase total bundle size by 20 bytes (0.0%) ⬆️. This is within the configured threshold ✅

Detailed changes
Bundle name Size Change
@lynx-js/example-react-lynx-cjs 236.9kB 20 bytes (0.01%) ⬆️

Affected Assets, Files, and Routes:

view changes for bundle: @lynx-js/example-react-lynx-cjs

Assets Changed:

Asset Name Size Change Total Size Change (%)
main.lynx.bundle 20 bytes 87.64kB 0.02%

@codspeed-hq
Copy link

codspeed-hq bot commented Mar 28, 2025 •
edited
Loading

CodSpeed Performance Report

Merging #377 will not alter performance

Comparing Yradex:lifecycle/fix (8135e6e) with main (3ff8829)

Summary

✅ 1 untouched benchmarks

@Yradex Yradex marked this pull request as draft March 28, 2025 07:55
@Yradex Yradex marked this pull request as ready for review March 28, 2025 08:33
@Yradex Yradex requested review from colinaaa and hzy March 28, 2025 08:33
@hzy hzy requested a review from Copilot March 31, 2025 08:38
Copilot AI reviewed Mar 31, 2025
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes an issue where multiple firstScreen events are triggered when reloading before jsReady is signaled. Key changes include:

  • Centralizing JS Ready state management by removing inline jsReady logic from calledByNative.ts and creating a dedicated module (lifecycle/event/jsReady.ts).
  • Adding proper state checks and using resetJSReady in renderPage to ensure only one firstScreen event is fired.
  • Updating test cases to verify the correct behavior for reload-before-jsReady scenarios.

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file
File Description
packages/react/runtime/src/lynx/calledByNative.ts Removed inline jsReady implementation and replaced it with resetJSReady; combined conditional checks for SSR timing.
packages/react/runtime/src/lifecycle/reload.ts Added clearJSReadyEventIdSwap and conditional firstScreen event trigger based on jsReady state; adjusted import order and export order.
packages/react/runtime/src/lifecycle/event/jsReady.ts Introduced a new module for centralized jsReady state management with functions to update and reset state.
packages/react/runtime/test/lifecycle/reload.test.jsx Added test coverage for the reload-before-jsReady scenario ensuring only one firstScreen event is triggered.
.changeset/nasty-kids-matter.md Created a changeset for the fix to prevent multiple firstScreen events.
Comments suppressed due to low confidence (1)

packages/react/runtime/src/lifecycle/event/jsReady.ts:5

  • Consider initializing isJSReady to false upon declaration (e.g., 'let isJSReady: boolean = false;') to ensure predictable behavior before it is explicitly set via resetJSReady or jsReady().
let isJSReady: boolean;

@Yradex Yradex added this pull request to the merge queue Mar 31, 2025
Merged via the queue into lynx-family:main with commit 44dfa9c Mar 31, 2025
21 checks passed
@Yradex Yradex deleted the lifecycle/fix branch March 31, 2025 09:05
@github-actions github-actions bot mentioned this pull request Mar 31, 2025
Merged
colinaaa pushed a commit that referenced this pull request Mar 31, 2025
@github-actions
chore: Release 2025-03-31 09:13:17 (#357)
5f3c12b 
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/web-constants@0.10.0

### Minor Changes

- feat: rewrite the main thread Element PAPIs
([#343](#343))

    In this commit we've rewritten the main thread apis.

    The most highlighted change is that

    -   Before this commit we send events directly to bts
- After this change, we send events to mts then send them to bts with
some data combined.

### Patch Changes

- feat: The onNapiModulesCall function of lynx-view provides the fourth
parameter: `lynxView`, which is the actual lynx-view DOM.
([#350](#350))

- fix: publicComponentEvent args order
([#401](#401))

- Updated dependencies
\[[`a521759`](a521759)]:
    -   @lynx-js/web-worker-rpc@0.10.0

## @lynx-js/web-core@0.10.0

### Minor Changes

- feat: rewrite the main thread Element PAPIs
([#343](#343))

    In this commit we've rewritten the main thread apis.

    The most highlighted change is that

    -   Before this commit we send events directly to bts
- After this change, we send events to mts then send them to bts with
some data combined.

### Patch Changes

- refactor: timing system
([#378](#378))

    Now we moved the timing system to the background thread.

- feat: support `defaultOverflowVisible` config
([#406](#406))

- fix(web): rsbuild will bundle 2 exactly same chunk for two same `new
Worker` stmt
([#372](#372))

    the bundle size will be optimized about 28.2KB

- fix: inline style will be removed for value number `0`
([#368](#368))

the inline style value could be incorrectly removed for number value
`0`;

    For example, `flex-shrink:0` may be ignored.

- feat: The onNapiModulesCall function of lynx-view provides the fourth
parameter: `lynxView`, which is the actual lynx-view DOM.
([#350](#350))

- fix: publicComponentEvent args order
([#401](#401))

- Updated dependencies
\[[`3a8dabd`](3a8dabd),
[`a521759`](a521759),
[`890c6c5`](890c6c5)]:
    -   @lynx-js/web-worker-runtime@0.10.0
    -   @lynx-js/web-constants@0.10.0
    -   @lynx-js/web-worker-rpc@0.10.0

## @lynx-js/web-mainthread-apis@0.10.0

### Minor Changes

- feat: rewrite the main thread Element PAPIs
([#343](#343))

    In this commit we've rewritten the main thread apis.

    The most highlighted change is that

    -   Before this commit we send events directly to bts
- After this change, we send events to mts then send them to bts with
some data combined.

### Patch Changes

- feat(web): use pure DOM API to implement Element PAPIs
([#334](#334))

    1.  rewrite all element PAPIs impl. Now we use DOM.
2. use our new package `@lynx-js/offscreen-document` to support the new
Element PAPI implementation in a worker

- fix: inline style will be removed for value number `0`
([#368](#368))

the inline style value could be incorrectly removed for number value
`0`;

    For example, `flex-shrink:0` may be ignored.

- fix: publicComponentEvent args order
([#401](#401))

- Updated dependencies
\[[`3a8dabd`](3a8dabd),
[`a521759`](a521759),
[`890c6c5`](890c6c5)]:
    -   @lynx-js/web-constants@0.10.0

## @lynx-js/web-worker-runtime@0.10.0

### Minor Changes

- feat: rewrite the main thread Element PAPIs
([#343](#343))

    In this commit we've rewritten the main thread apis.

    The most highlighted change is that

    -   Before this commit we send events directly to bts
- After this change, we send events to mts then send them to bts with
some data combined.

### Patch Changes

- Updated dependencies
\[[`2a8ddf3`](2a8ddf3),
[`3a8dabd`](3a8dabd),
[`878050a`](878050a),
[`a521759`](a521759),
[`890c6c5`](890c6c5)]:
    -   @lynx-js/web-mainthread-apis@0.10.0
    -   @lynx-js/web-constants@0.10.0
    -   @lynx-js/web-worker-rpc@0.10.0

## @lynx-js/react@0.106.2

### Patch Changes

- fix: prevent multiple firstScreen events when reloading before
`jsReady` ([#377](#377))

- Optimize the bundle size by eliminating unnecessary code when the lazy
bundle is not utilized.
([#284](#284))

## @lynx-js/rspeedy@0.8.7

### Patch Changes

- Support using `-debugids` in `output.sourceMap.js`.
([#342](#342))

See [Source Map Debug ID
Proposal](https://github.com/tc39/ecma426/blob/main/proposals/debug-id.md)
for more details.

- Use `chunkLoading: 'import-scripts'` for Web platform
([#352](#352))

- Support `output.distPath.*`.
([#366](#366))

See [Rsbuild - distPath](https://rsbuild.dev/config/output/dist-path)
for all available options.

- Support `performance.printFileSize`
([#336](#336))

    Whether to print the file sizes after production build.

## @lynx-js/react-rsbuild-plugin@0.9.4

### Patch Changes

- feat: add extractStr option to pluginReactLynx
([#391](#391))

- Convert background-only files from js to ts
([#346](#346))

- Updated dependencies
\[[`f849117`](f849117),
[`d730101`](d730101),
[`42217c2`](42217c2),
[`f03bd4a`](f03bd4a)]:
    -   @lynx-js/react-webpack-plugin@0.6.10
    -   @lynx-js/template-webpack-plugin@0.6.7
    -   @lynx-js/react-alias-rsbuild-plugin@0.9.4
    -   @lynx-js/react-refresh-webpack-plugin@0.3.2
    -   @lynx-js/css-extract-webpack-plugin@0.5.2
    -   @lynx-js/web-webpack-plugin@0.6.3

## @lynx-js/web-elements@0.5.2

### Patch Changes

- fix: When list with list-type: flow, scrolltoupper, scrolltolower were
specified, there was a blank block.
([#379](#379))

- fix: do not show scroll bar
([#406](#406))

## @lynx-js/web-worker-rpc@0.10.0

### Patch Changes

- feat: The onNapiModulesCall function of lynx-view provides the fourth
parameter: `lynxView`, which is the actual lynx-view DOM.
([#350](#350))

## @lynx-js/react-webpack-plugin@0.6.10

### Patch Changes

- feat: add extractStr option to pluginReactLynx
([#391](#391))

- Fix issue with lazy loading of bundles when source maps are enabled.
([#380](#380))

- Fix issue where loading a lazy bundle fails if it does not return a
webpack chunk.
([#365](#365))

## @lynx-js/template-webpack-plugin@0.6.7

### Patch Changes

- fix: merge different chunk groups for same output filename
([#371](#371))

## create-rspeedy@0.8.7



## @lynx-js/react-alias-rsbuild-plugin@0.9.4



## upgrade-rspeedy@0.8.7

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@hzy hzy hzy approved these changes

@colinaaa colinaaa colinaaa approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Yradex @hzy @colinaaa