Skip to content

ci(benchmark/react): add LoadScript to benchmark#1699

Merged
colinaaa merged 1 commit intolynx-family:mainfrom
hzy:p/hzy/bench_10
Sep 9, 2025
Merged

ci(benchmark/react): add LoadScript to benchmark#1699
colinaaa merged 1 commit intolynx-family:mainfrom
hzy:p/hzy/bench_10

Conversation

@hzy
Copy link
Copy Markdown
Collaborator

@hzy hzy commented Sep 9, 2025

Summary by CodeRabbit

  • New Features

    • Deferred rendering and benchmark execution until after a load script (post-load callbacks).
    • Added a build plugin that instruments benchmark start/stop and flushes post-load callbacks.
    • Added a new "load-script" benchmark case.
    • Public API extended with a post-load callback registration.
  • Chores

    • Updated build configuration (disabled filename hashing; added entries).
    • Added npm scripts to run the new benchmark case.
  • Notes

    • Existing benchmark logic and public signatures unchanged.

Checklist

  • Tests updated (or not required).
  • Documentation updated (or not required).
  • Changeset added, and when a BREAKING CHANGE occurs, it needs to be clearly marked (or not required).

@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Sep 9, 2025

⚠️ No Changeset found

Latest commit: 6ddfd5e

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Sep 9, 2025

📝 Walkthrough

Walkthrough

Adds a post-load hook API (runAfterLoadScript), defers execution/rendering in multiple React benchmark entrypoints and the dummy root, introduces a build plugin that injects start/stop instrumentation and flush behavior, updates build config and npm scripts, adds typings, and adds a new load-script benchmark case.

Changes

Cohort / File(s) Summary
Deferred execution in benchmark entries
benchmark/react/cases/001-fib/index.ts, benchmark/react/cases/002-hello-reactLynx/index.tsx, benchmark/react/cases/003-hello-list/index.tsx, benchmark/react/cases/004-various-update/index.tsx, benchmark/react/cases/005-load-script/index.tsx, benchmark/react/src/dummyRoot.tsx
Top-level renders/execution moved into runAfterLoadScript(() => { ... }) callbacks. 001-fib removed @lynx-js/react import, preserved memoized fib2, relocated bench setup into the deferred wrapper. New 005-load-script registers the post-load hook and renders after load.
Build plugin and config
benchmark/react/plugins/pluginScriptLoad.mjs, benchmark/react/lynx.config.js
New pluginScriptLoad Rsbuild plugin injects startup instrumentation (Codspeed.startBenchmark() and a queue-backed Codspeed.runAfterLoadScript) and per-asset teardown (Codspeed.stopBenchmark() plus .flush()). lynx.config.js wires the plugin, disables output.filenameHash, updates source.entry['001-fib'] to ./src/dummyRoot.tsx, and adds 005-load-script entry.
Type declarations
benchmark/react/rspeedy-env.d.ts
Adds ambient declaration: declare function runAfterLoadScript(cb: () => void): void with JSDoc describing post-load invocation.
NPM scripts
benchmark/react/package.json
Adds bench:005-load-script and perfetto:005-load-script scripts to run the new 005-load-script bundle.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Suggested reviewers

  • upupming
  • colinaaa

Pre-merge checks (1 passed, 2 warnings)

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Description Check ⚠️ Warning The current description consists solely of a generic contributor template and checklist without any mention of the actual changes, making it unrelated to the modifications introduced in this pull request. Please replace the placeholder text with a summary of the updates—such as the addition of the runAfterLoadScript API, the new LoadScript benchmark case, the pluginScriptLoad implementation, and configuration changes—so reviewers can understand the purpose and scope of the PR.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (1 passed)
Check name Status Explanation
Title Check ✅ Passed The title “ci(benchmark/react): add LoadScript to benchmark” succinctly captures the primary change of introducing the LoadScript hook into the React benchmark suite and does so in a concise, single sentence that aligns with the conventional commit style and clearly indicates the main intent of the PR.

Poem

I sat in a little queue of hops,
Waiting gentle fluffs when the loading stops.
Start the bench, then stop and flush,
Callbacks leap in a jittery rush.
A rabbit claps — the bundles hush. 🐇


📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8108611 and 6ddfd5e.

📒 Files selected for processing (10)
  • benchmark/react/cases/001-fib/index.ts (1 hunks)
  • benchmark/react/cases/002-hello-reactLynx/index.tsx (1 hunks)
  • benchmark/react/cases/003-hello-list/index.tsx (1 hunks)
  • benchmark/react/cases/004-various-update/index.tsx (1 hunks)
  • benchmark/react/cases/005-load-script/index.tsx (1 hunks)
  • benchmark/react/lynx.config.js (4 hunks)
  • benchmark/react/package.json (1 hunks)
  • benchmark/react/plugins/pluginScriptLoad.mjs (1 hunks)
  • benchmark/react/rspeedy-env.d.ts (1 hunks)
  • benchmark/react/src/dummyRoot.tsx (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
  • benchmark/react/rspeedy-env.d.ts
  • benchmark/react/cases/004-various-update/index.tsx
  • benchmark/react/src/dummyRoot.tsx
  • benchmark/react/package.json
  • benchmark/react/lynx.config.js
  • benchmark/react/cases/005-load-script/index.tsx
  • benchmark/react/cases/002-hello-reactLynx/index.tsx
  • benchmark/react/cases/003-hello-list/index.tsx
  • benchmark/react/cases/001-fib/index.ts
  • benchmark/react/plugins/pluginScriptLoad.mjs
⏰ 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: zizmor
✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov
Copy link
Copy Markdown

codecov bot commented Sep 9, 2025

⚠️ File not in storage

No result to display due to the CLI not being able to find the file.
Please ensure the file contains junit in the name and automated file search is enabled,
or the desired file specified by the file and search_dir arguments of the CLI.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🧹 Nitpick comments (11)
benchmark/react/package.json (1)

16-16: Align repeat behavior across perfetto scripts (and avoid potential single-run overwrite).

If benchx_cli writes one ptrace per invocation, --repeat=50 may overwrite or aggregate implicitly. For apples-to-apples runs, either propagate --repeat=50 to the other perfetto scripts or keep all single-run; also confirm how benchx_cli handles repeated -o output.

Proposed alignment:

 "perfetto:001-fib": "benchx_cli -o dist/001-fib.ptrace run dist/001-fib.lynx.bundle",
-"perfetto:002-hello-reactLynx": "benchx_cli -o dist/002-hello-reactLynx.ptrace run dist/002-hello-reactLynx.lynx.bundle --repeat=50 --wait-for-id=stop-benchmark-true",
+"perfetto:002-hello-reactLynx": "benchx_cli -o dist/002-hello-reactLynx.ptrace run dist/002-hello-reactLynx.lynx.bundle --repeat=50 --wait-for-id=stop-benchmark-true",
-"perfetto:003-hello-list": "benchx_cli -o dist/003-hello-list.ptrace run dist/003-hello-list.lynx.bundle --wait-for-id=stop-benchmark-true",
+"perfetto:003-hello-list": "benchx_cli -o dist/003-hello-list.ptrace run dist/003-hello-list.lynx.bundle --repeat=50 --wait-for-id=stop-benchmark-true",
-"perfetto:004-various-update": "benchx_cli -o dist/004-various-update.ptrace run dist/004-various-update.lynx.bundle --wait-for-id=stop-benchmark-true",
+"perfetto:004-various-update": "benchx_cli -o dist/004-various-update.ptrace run dist/004-various-update.lynx.bundle --repeat=50 --wait-for-id=stop-benchmark-true",

If repeats produce multiple traces, consider suffixing with run index or switching to an aggregated output flag if available.

benchmark/react/rspeedy-env.d.ts (1)

13-20: Tighten docs: grammar + clarify semantics of the callback.

Small grammar fix and clarity that the callback runs after the load-time benchmark has stopped.

   /**
-   * This function will be called after the script is loaded and executed.
-   * Codspeed don't allow stacked benchmark, but sadly we have some cases
-   * benchmark is executed while loading script, we need to move those cases
-   * to after script load using this API.
-   * @param cb The callback to run after script loaded.
+   * Register a callback to run after the bundle has loaded and executed.
+   * Codspeed doesn't allow stacked benchmarks; some cases currently start
+   * during script load. Use this API to defer those to after script load.
+   * Note: callbacks typically run after the load-time benchmark has stopped.
+   * @param cb The callback to run after the script has loaded.
    */
benchmark/react/plugins/pluginScriptLoad.mjs (3)

43-46: Consider setting the benchmark name before stopping to ensure attribution.

Safer ordering: name → stop → flush.

-Codspeed.stopBenchmark();
-Codspeed.setExecutedBenchmark(\`${caseDir}::\${${JSON.stringify(chunkName)}}_LoadScript\`);
+Codspeed.setExecutedBenchmark(\`${caseDir}::\${${JSON.stringify(chunkName)}}_LoadScript\`);
+Codspeed.stopBenchmark();
 Codspeed.runAfterLoadScript.flush();

21-28: Make runAfterLoadScript idempotent and resilient (drain queue, ignore non-fns, guard double flush).

Prevents double-execution, unbounded queue growth, and improves error isolation.

 Codspeed.runAfterLoadScript = (() => {
-  const q = [];
-  const r = (cb) => { q.push(cb); };
-  r.flush = () => { for (const cb of q) { cb(); } };
+  /** @type {Array<() => void>} */
+  const q = [];
+  let flushed = false;
+  const r = (cb) => {
+    if (typeof cb !== 'function') return;
+    if (flushed) {
+      queueMicrotask(cb);
+    } else {
+      q.push(cb);
+    }
+  };
+  r.flush = () => {
+    if (flushed) return;
+    flushed = true;
+    const cbs = q.splice(0, q.length);
+    for (const cb of cbs) {
+      try { cb(); } catch (e) { console.error('[Codspeed.runAfterLoadScript]', e); }
+    }
+  };
   return r;
 })();

39-40: Avoid hardcoding caseDir.

Derive from a build param or from a compile-time constant to keep this plugin reusable across packages.

I can wire this to accept a caseDir option (defaulting to current) and update lynx.config.js accordingly.

benchmark/react/lynx.config.js (1)

14-14: Disabling filename hashing risks stale assets behind a CDN.

If this is only for local/CI benchmarks, OK. If it’s ever served via assetPrefix, ensure strong no-cache headers or a versioned assetPrefix to avoid cache poisoning.

benchmark/react/cases/004-various-update/index.tsx (1)

230-234: Prefer a safe fallback if Codspeed isn’t injected.

Avoid a hard dependency on the plugin for local/dev runs.

Apply this diff:

-Codspeed.runAfterLoadScript(() => {
-  root.render(
-    <F />,
-  );
-});
+if (typeof Codspeed !== 'undefined') {
+  Codspeed.runAfterLoadScript(() => {
+    root.render(<F />);
+  });
+} else {
+  root.render(<F />);
+}
benchmark/react/cases/003-hello-list/index.tsx (1)

71-75: Add non-Codspeed fallback for resilience.

Same rationale as other cases; keep dev runs working without the banner plugin.

Apply this diff:

-Codspeed.runAfterLoadScript(() => {
-  root.render(
-    <BenchmarkList />,
-  );
-});
+if (typeof Codspeed !== 'undefined') {
+  Codspeed.runAfterLoadScript(() => {
+    root.render(<BenchmarkList />);
+  });
+} else {
+  root.render(<BenchmarkList />);
+}
benchmark/react/cases/005-load-script/index.tsx (1)

8-12: Guard against missing Codspeed in non-benchmark environments.

Apply this diff:

-Codspeed.runAfterLoadScript(() => {
-  root.render(
-    <></>,
-  );
-});
+if (typeof Codspeed !== 'undefined') {
+  Codspeed.runAfterLoadScript(() => {
+    root.render(<></>);
+  });
+} else {
+  root.render(<></>);
+}
benchmark/react/src/dummyRoot.tsx (1)

7-9: Minor: add fallback to keep this entry usable without the plugin.

Apply this diff:

-Codspeed.runAfterLoadScript(() => {
-  root.render(<></>);
-});
+if (typeof Codspeed !== 'undefined') {
+  Codspeed.runAfterLoadScript(() => {
+    root.render(<></>);
+  });
+} else {
+  root.render(<></>);
+}
benchmark/react/cases/001-fib/index.ts (1)

9-63: Two nits: safer memo check; optional Codspeed fallback wrapper.

  • Use explicit !== undefined for memoization checks to avoid truthiness pitfalls.
  • Optional: guard Codspeed to allow running this case without the plugin.

Apply this diff:

-Codspeed.runAfterLoadScript(() => {
+const run = () => {
   if (__MAIN_THREAD__) {
@@
-    function fib2(n: number): number {
-      const memo: number[] = Array.from({ length: n + 1 });
-      const dp = (n: number): number => {
-        if (n <= 1) return n;
-        if (memo[n]) return memo[n];
-        memo[n] = dp(n - 1) + dp(n - 2);
-        return memo[n];
-      };
-      return dp(n);
-    }
+    function fib2(n: number): number {
+      const memo: (number | undefined)[] = new Array(n + 1);
+      const dp = (k: number): number => {
+        if (k <= 1) return k;
+        const cached = memo[k];
+        if (cached !== undefined) return cached;
+        const val = dp(k - 1) + dp(k - 2);
+        memo[k] = val;
+        return val;
+      };
+      return dp(n);
+    }
@@
-});
+};
+
+if (typeof Codspeed !== 'undefined') {
+  Codspeed.runAfterLoadScript(run);
+} else {
+  run();
+}
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ffc1cd5 and 14b15de.

📒 Files selected for processing (10)
  • benchmark/react/cases/001-fib/index.ts (1 hunks)
  • benchmark/react/cases/002-hello-reactLynx/index.tsx (1 hunks)
  • benchmark/react/cases/003-hello-list/index.tsx (1 hunks)
  • benchmark/react/cases/004-various-update/index.tsx (1 hunks)
  • benchmark/react/cases/005-load-script/index.tsx (1 hunks)
  • benchmark/react/lynx.config.js (3 hunks)
  • benchmark/react/package.json (1 hunks)
  • benchmark/react/plugins/pluginScriptLoad.mjs (1 hunks)
  • benchmark/react/rspeedy-env.d.ts (1 hunks)
  • benchmark/react/src/dummyRoot.tsx (1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-08-12T16:09:32.413Z
Learnt from: colinaaa
PR: lynx-family/lynx-stack#1497
File: packages/react/transform/tests/__swc_snapshots__/src/swc_plugin_snapshot/mod.rs/basic_full_static.js:9-10
Timestamp: 2025-08-12T16:09:32.413Z
Learning: In the Lynx stack, functions prefixed with `__` that are called in transformed code may be injected globally by the Lynx Engine at runtime rather than exported from the React runtime package. For example, `__CreateFrame` is injected globally by the Lynx Engine, not exported from lynx-js/react.

Applied to files:

  • benchmark/react/cases/002-hello-reactLynx/index.tsx
  • benchmark/react/cases/005-load-script/index.tsx
📚 Learning: 2025-08-25T09:16:24.277Z
Learnt from: CR
PR: lynx-family/lynx-stack#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-25T09:16:24.277Z
Learning: Always run a full build with `pnpm turbo build` before running any tests

Applied to files:

  • benchmark/react/package.json
📚 Learning: 2025-08-27T12:42:01.095Z
Learnt from: upupming
PR: lynx-family/lynx-stack#1616
File: packages/webpack/cache-events-webpack-plugin/test/cases/not-cache-events/lazy-bundle/index.js:3-3
Timestamp: 2025-08-27T12:42:01.095Z
Learning: In webpack, properties like __webpack_require__.lynx_ce are injected during compilation/build time when webpack processes modules and generates bundles, not at runtime when dynamic imports execute. Tests for such properties don't need to wait for dynamic imports to complete.

Applied to files:

  • benchmark/react/lynx.config.js
  • benchmark/react/cases/005-load-script/index.tsx
📚 Learning: 2025-08-06T13:28:57.182Z
Learnt from: colinaaa
PR: lynx-family/lynx-stack#1453
File: vitest.config.ts:49-61
Timestamp: 2025-08-06T13:28:57.182Z
Learning: In the lynx-family/lynx-stack repository, the file `packages/react/testing-library/src/vitest.config.js` is source code for the testing library that gets exported for users, not a test configuration that should be included in the main vitest projects array.

Applied to files:

  • benchmark/react/lynx.config.js
🧬 Code graph analysis (2)
benchmark/react/cases/002-hello-reactLynx/index.tsx (1)
benchmark/react/src/RunBenchmarkUntil.tsx (1)
  • RunBenchmarkUntilHydrate (7-9)
benchmark/react/lynx.config.js (1)
benchmark/react/plugins/pluginScriptLoad.mjs (2)
  • pluginScriptLoad (8-52)
  • pluginScriptLoad (8-52)
⏰ 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 / clippy
  • GitHub Check: test-rust / Test (Ubuntu)
🔇 Additional comments (3)
benchmark/react/lynx.config.js (2)

10-10: Import looks correct; ensure plugin file is included in build graph.

No issues; just confirm ./plugins/pluginScriptLoad.mjs is part of the package published/consumed by CI, otherwise config will fail at runtime.


30-30: TSX entry is supported—no changes needed. benchmark/react/tsconfig.json sets "jsx": "react-jsx" and includes "src", so .tsx files (e.g. ./src/dummyRoot.tsx) are picked up and compiled.

benchmark/react/cases/002-hello-reactLynx/index.tsx (1)

10-17: LGTM: render deferral via Codspeed wrapper is consistent with the suite.

@relativeci
Copy link
Copy Markdown

relativeci bot commented Sep 9, 2025

Web Explorer

#4999 Bundle Size — 366.49KiB (0%).

6ddfd5e(current) vs de8a1c7 main#4998(baseline)

Bundle metrics  Change 1 change
                 Current
#4999
     Baseline
#4998
No change  Initial JS 144.22KiB 144.22KiB
No change  Initial CSS 31.84KiB 31.84KiB
No change  Cache Invalidation 0% 0%
No change  Chunks 8 8
No change  Assets 8 8
Change  Modules 219(-0.45%) 220
No change  Duplicate Modules 16 16
No change  Duplicate Code 3.33% 3.33%
No change  Packages 4 4
No change  Duplicate Packages 0 0
Bundle size by type  no changes
                 Current
#4999
     Baseline
#4998
No change  JS 235.59KiB 235.59KiB
No change  Other 99.06KiB 99.06KiB
No change  CSS 31.84KiB 31.84KiB

Bundle analysis reportBranch hzy:p/hzy/bench_10Project dashboard


Generated by RelativeCIDocumentationReport issue

@relativeci
Copy link
Copy Markdown

relativeci bot commented Sep 9, 2025

React Example

#5006 Bundle Size — 238.2KiB (0%).

6ddfd5e(current) vs de8a1c7 main#5005(baseline)

Bundle metrics  no changes
                 Current
#5006
     Baseline
#5005
No change  Initial JS 0B 0B
No change  Initial CSS 0B 0B
No change  Cache Invalidation 0% 0%
No change  Chunks 0 0
No change  Assets 4 4
No change  Modules 163 163
No change  Duplicate Modules 67 67
No change  Duplicate Code 46.88% 46.88%
No change  Packages 2 2
No change  Duplicate Packages 0 0
Bundle size by type  no changes
                 Current
#5006
     Baseline
#5005
No change  IMG 145.76KiB 145.76KiB
No change  Other 92.45KiB 92.45KiB

Bundle analysis reportBranch hzy:p/hzy/bench_10Project dashboard


Generated by RelativeCIDocumentationReport issue

@codspeed-hq
Copy link
Copy Markdown

codspeed-hq bot commented Sep 9, 2025

CodSpeed Performance Report

Merging #1699 will degrade performances by 11.64%

Comparing hzy:p/hzy/bench_10 (6ddfd5e) with main (de8a1c7)

🎉 Hooray! codspeed-node just leveled up to 4.0.1!

A heads-up, this is a breaking change and it might affect your current performance baseline a bit. But here's the exciting part - it's packed with new, cool features and promises improved result stability 🥳!
Curious about what's new? Visit our releases page to delve into all the awesome details about this new version.

Summary

❌ 2 regressions
✅ 41 untouched benchmarks
🆕 10 new benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark BASE HEAD Change
🆕 001-fib/background.js_LoadScript N/A 546.8 µs N/A
🆕 001-fib/main-thread.js_LoadScript N/A 1.1 ms N/A
002-hello-reactLynx-destroyBackground 625 µs 707.3 µs -11.64%
🆕 002-hello-reactLynx/background.js_LoadScript N/A 475.6 µs N/A
🆕 002-hello-reactLynx/main-thread.js_LoadScript N/A 388.8 µs N/A
🆕 003-hello-list/background.js_LoadScript N/A 1.5 ms N/A
🆕 003-hello-list/main-thread.js_LoadScript N/A 1.5 ms N/A
🆕 004-various-update/background.js_LoadScript N/A 383.3 µs N/A
🆕 004-various-update/main-thread.js_LoadScript N/A 643.4 µs N/A
🆕 005-load-script/background.js_LoadScript N/A 1.2 ms N/A
🆕 005-load-script/main-thread.js_LoadScript N/A 1.1 ms N/A
basic-performance-nest-level-100 5.9 ms 6.6 ms -11.1%

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

♻️ Duplicate comments (1)
benchmark/react/lynx.config.js (1)

57-57: Guard against Codspeed absence and make flush idempotent (duplicate of prior feedback).

As previously noted, pluginScriptLoad() injects unguarded Codspeed.* calls, a non-idempotent flush(), and a buggy regex for chunkName. These can crash dev builds and re-run callbacks across assets. Please adopt the earlier fix in benchmark/react/plugins/pluginScriptLoad.mjs, or gate this plugin to CI-only in this config.

Optionally gate the plugin here to avoid local/dev crashes:

   plugins: [
     pluginRepoFilePath(),
     pluginReactLynx({
       enableParallelElement: false,
       pipelineSchedulerConfig: 0,
       debugInfoOutside: false,
     }),
-    pluginScriptLoad(),
+    ...(process.env.CODSPEED_BENCH === '1' ? [pluginScriptLoad()] : []),
     pluginQRCode({}),
   ],
🧹 Nitpick comments (1)
benchmark/react/lynx.config.js (1)

14-14: Disabling filename hashing: confirm cache-busting strategy.

With filenameHash: false and a static assetPrefix, CDN/browser caches may serve stale bundles across runs. Either ensure no-store headers on the CDN or scope the prefix by commit/CI run.

Apply this to make the prefix versioned by CI/commit (fallback to dev):

-    assetPrefix: 'https://example.com/benchmark/react',
+    assetPrefix: `${process.env.BENCH_ASSET_PREFIX ?? 'https://example.com/benchmark/react'}/${process.env.GIT_SHA ?? 'dev'}`,
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 14b15de and c4308d7.

📒 Files selected for processing (10)
  • benchmark/react/cases/001-fib/index.ts (1 hunks)
  • benchmark/react/cases/002-hello-reactLynx/index.tsx (1 hunks)
  • benchmark/react/cases/003-hello-list/index.tsx (1 hunks)
  • benchmark/react/cases/004-various-update/index.tsx (1 hunks)
  • benchmark/react/cases/005-load-script/index.tsx (1 hunks)
  • benchmark/react/lynx.config.js (4 hunks)
  • benchmark/react/package.json (1 hunks)
  • benchmark/react/plugins/pluginScriptLoad.mjs (1 hunks)
  • benchmark/react/rspeedy-env.d.ts (1 hunks)
  • benchmark/react/src/dummyRoot.tsx (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
  • benchmark/react/cases/002-hello-reactLynx/index.tsx
  • benchmark/react/cases/003-hello-list/index.tsx
  • benchmark/react/rspeedy-env.d.ts
  • benchmark/react/package.json
  • benchmark/react/src/dummyRoot.tsx
  • benchmark/react/cases/005-load-script/index.tsx
  • benchmark/react/plugins/pluginScriptLoad.mjs
  • benchmark/react/cases/001-fib/index.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-27T12:42:01.095Z
Learnt from: upupming
PR: lynx-family/lynx-stack#1616
File: packages/webpack/cache-events-webpack-plugin/test/cases/not-cache-events/lazy-bundle/index.js:3-3
Timestamp: 2025-08-27T12:42:01.095Z
Learning: In webpack, properties like __webpack_require__.lynx_ce are injected during compilation/build time when webpack processes modules and generates bundles, not at runtime when dynamic imports execute. Tests for such properties don't need to wait for dynamic imports to complete.

Applied to files:

  • benchmark/react/lynx.config.js
📚 Learning: 2025-08-06T13:28:57.182Z
Learnt from: colinaaa
PR: lynx-family/lynx-stack#1453
File: vitest.config.ts:49-61
Timestamp: 2025-08-06T13:28:57.182Z
Learning: In the lynx-family/lynx-stack repository, the file `packages/react/testing-library/src/vitest.config.js` is source code for the testing library that gets exported for users, not a test configuration that should be included in the main vitest projects array.

Applied to files:

  • benchmark/react/lynx.config.js
🧬 Code graph analysis (1)
benchmark/react/lynx.config.js (1)
benchmark/react/plugins/pluginScriptLoad.mjs (2)
  • pluginScriptLoad (8-52)
  • pluginScriptLoad (8-52)
⏰ 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 (Windows)
  • GitHub Check: build / Build (Ubuntu)
  • GitHub Check: test-rust / Test (Ubuntu)
🔇 Additional comments (3)
benchmark/react/lynx.config.js (3)

10-10: Import path and symbol look correct.

The local import resolves to the expected plugin module and matches the exported name.


45-47: 005-load-script case validated: uses Codspeed.runAfterLoadScript post-load hook and contains no dynamic imports (single JS chunk).


30-30: Resolved: TSX pipeline and ambient types are correctly configured
The local tsconfig.json in benchmark/react declares "jsx": "react-jsx" with the appropriate jsxImportSource, and the ambient declarations in benchmark/react/rspeedy-env.d.ts are included by the default "include": ["**/*"] behavior—no further changes needed.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (4)
benchmark/react/cases/001-fib/index.ts (4)

16-25: Memoization: make presence checks explicit and avoid param shadowing

Truthiness on memo[n] works here but is brittle. Seed base cases, check for undefined explicitly, avoid reusing the name n in dp, and avoid any[] from Array.from.

-    function fib2(n: number): number {
-      const memo: number[] = Array.from({ length: n + 1 });
-      const dp = (n: number): number => {
-        if (n <= 1) return n;
-        if (memo[n]) return memo[n];
-        memo[n] = dp(n - 1) + dp(n - 2);
-        return memo[n];
-      };
-      return dp(n);
-    }
+    function fib2(n: number): number {
+      const memo: (number | undefined)[] = new Array(n + 1);
+      memo[0] = 0;
+      memo[1] = 1;
+      const dp = (k: number): number => {
+        const v = memo[k];
+        if (v !== undefined) return v;
+        const res = dp(k - 1) + dp(k - 2);
+        memo[k] = res;
+        return res;
+      };
+      return dp(n);
+    }

27-36: Nit: collapse base cases

Slightly tighter and consistent with fib().

-    function fib3(n: number): number {
-      if (n === 0) return 0;
-      if (n === 1) return 1;
+    function fib3(n: number): number {
+      if (n < 2) return n;
       let prev = 0, curr = 1;
       for (let i = 2; i <= n; i++) {
         const next = prev + curr;
         prev = curr;
         curr = next;
       }
       return curr;
     }

39-45: tinybench time is milliseconds—make intent explicit

If you meant 5 seconds, set 5_000 and consider letting time govern instead of mixing iterations; tinybench treats time in ms.

-      new Bench({
-        name: 'simple benchmark',
-        time: 5,
-        iterations: 20,
-      }),
+      new Bench({
+        name: 'simple benchmark',
+        // time is in milliseconds; use 5s
+        time: 5_000,
+        iterations: 20,
+      }),

58-63: Ensure tinybench version supports runSync; otherwise switch to async run

Some versions only expose run(). This tweak keeps the surrounding code non-async.

-    bench.runSync();
-
-    console.log(bench.name);
-    console.log(JSON.stringify(bench.table(), undefined, 2));
+    void (async () => {
+      await bench.run();
+      console.log(bench.name);
+      console.log(JSON.stringify(bench.table(), undefined, 2));
+    })();
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c4308d7 and 8108611.

📒 Files selected for processing (10)
  • benchmark/react/cases/001-fib/index.ts (1 hunks)
  • benchmark/react/cases/002-hello-reactLynx/index.tsx (1 hunks)
  • benchmark/react/cases/003-hello-list/index.tsx (1 hunks)
  • benchmark/react/cases/004-various-update/index.tsx (1 hunks)
  • benchmark/react/cases/005-load-script/index.tsx (1 hunks)
  • benchmark/react/lynx.config.js (4 hunks)
  • benchmark/react/package.json (1 hunks)
  • benchmark/react/plugins/pluginScriptLoad.mjs (1 hunks)
  • benchmark/react/rspeedy-env.d.ts (1 hunks)
  • benchmark/react/src/dummyRoot.tsx (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
  • benchmark/react/cases/004-various-update/index.tsx
  • benchmark/react/src/dummyRoot.tsx
  • benchmark/react/cases/002-hello-reactLynx/index.tsx
  • benchmark/react/cases/003-hello-list/index.tsx
  • benchmark/react/cases/005-load-script/index.tsx
  • benchmark/react/lynx.config.js
  • benchmark/react/rspeedy-env.d.ts
  • benchmark/react/plugins/pluginScriptLoad.mjs
  • benchmark/react/package.json
⏰ 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 (Windows)
  • GitHub Check: build / Build (Ubuntu)
  • GitHub Check: test-rust / clippy
  • GitHub Check: test-rust / Test (Ubuntu)
🔇 Additional comments (1)
benchmark/react/cases/001-fib/index.ts (1)

9-15: Load-after-script deferral looks correct

Keeping the MAIN_THREAD guard inside the runAfterLoadScript callback is the right placement for avoiding worker-side execution. Please confirm pluginScriptLoad flush order guarantees the callback runs exactly once after injection.

@hzy hzy requested review from colinaaa and upupming September 9, 2025 13:29
colinaaa
colinaaa previously approved these changes Sep 9, 2025
@colinaaa colinaaa merged commit 2158db2 into lynx-family:main Sep 9, 2025
42 of 43 checks passed
@coderabbitai coderabbitai bot mentioned this pull request Oct 29, 2025
3 tasks
@coderabbitai coderabbitai bot mentioned this pull request Mar 27, 2026
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants