Skip to content

Conversation

gabrielmfern
Copy link
Member

@gabrielmfern gabrielmfern commented Sep 17, 2025

Summary by cubic

Switch benchmarks to native ESM and run them with tsx instead of precompiling. This simplifies running benchmarks and aligns configs across packages.

  • Refactors
    • Enable ESM: set "type": "module" and update tsconfig (module: esnext, moduleResolution: bundler).
    • Replace tsup + node with tsx; add/adjust scripts (0.0.17-vs-local, with-vs-without, flamegraph) and update README.
    • Use top-level await in benchmark files; remove wrappers; ensure server cleanup and write results/print tables.

@gabrielmfern gabrielmfern self-assigned this Sep 17, 2025
@gabrielmfern gabrielmfern requested a review from a team as a code owner September 17, 2025 20:18
@gabrielmfern gabrielmfern requested review from joaopcm and removed request for a team September 17, 2025 20:18
Copy link

alwaysmeticulous bot commented Sep 17, 2025

🤖 Meticulous was unable to start a test run because no sessions have been recorded yet. Click here to setup session recording.

Last updated for commit 654fc93. This comment will update as new commits are pushed.

Copy link

changeset-bot bot commented Sep 17, 2025

⚠️ No Changeset found

Latest commit: 79fdf6a

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

Copy link

vercel bot commented Sep 17, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
react-email Ready Ready Preview Comment Oct 6, 2025 5:41pm
react-email-demo Error Error Oct 6, 2025 5:41pm

Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

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

7 issues found across 10 files

Prompt for AI agents (all 7 issues)

Understand the root cause of the following 7 issues and fix them.


<file name="benchmarks/preview-server/package.json">

<violation number="1" location="benchmarks/preview-server/package.json:5">
Setting type: &quot;module&quot; makes this package ESM, but several scripts use __dirname, which is undefined in ESM. This risks runtime failures when running the benchmark scripts.</violation>
</file>

<file name="benchmarks/preview-server/src/local-vs-2.1.7-canary.2.ts">

<violation number="1" location="benchmarks/preview-server/src/local-vs-2.1.7-canary.2.ts:35">
Ensure server subprocesses are terminated via try/finally so they’re killed even if an await (fetch/run/writeFile) throws.</violation>
</file>

<file name="benchmarks/preview-server/src/local-vs-2.1.7-canary.2-on-startup.ts">

<violation number="1" location="benchmarks/preview-server/src/local-vs-2.1.7-canary.2-on-startup.ts:24">
Awaiting runServer without a timeout can hang indefinitely if the child process fails to emit the expected stdout.</violation>

<violation number="2" location="benchmarks/preview-server/src/local-vs-2.1.7-canary.2-on-startup.ts:24">
Subprocess may be left running if fetch throws; wrap server lifecycle in a try/finally so kill() always runs for cleanup.</violation>

<violation number="3" location="benchmarks/preview-server/src/local-vs-2.1.7-canary.2-on-startup.ts:26">
kill() is asynchronous; await the child process &#39;close&#39; event after sending the signal to ensure the server fully exits before the next iteration.</violation>

<violation number="4" location="benchmarks/preview-server/src/local-vs-2.1.7-canary.2-on-startup.ts:29">
Subprocess may be left running if fetch throws; wrap server lifecycle in a try/finally so kill() always runs for cleanup.</violation>
</file>

<file name="benchmarks/tailwind-component/README.md">

<violation number="1" location="benchmarks/tailwind-component/README.md:13">
README structure lists wrong extension; the file is .tsx, not .ts.</violation>
</file>

React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.

"name": "@benchmarks/preview-server",
"private": true,
"version": "0.0.0",
"type": "module",
Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot Sep 17, 2025

Choose a reason for hiding this comment

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

Setting type: "module" makes this package ESM, but several scripts use __dirname, which is undefined in ESM. This risks runtime failures when running the benchmark scripts.

Prompt for AI agents
Address the following comment on benchmarks/preview-server/package.json at line 5:

<comment>Setting type: &quot;module&quot; makes this package ESM, but several scripts use __dirname, which is undefined in ESM. This risks runtime failures when running the benchmark scripts.</comment>

<file context>
@@ -2,6 +2,7 @@
   &quot;name&quot;: &quot;@benchmarks/preview-server&quot;,
   &quot;private&quot;: true,
   &quot;version&quot;: &quot;0.0.0&quot;,
+  &quot;type&quot;: &quot;module&quot;,
   &quot;scripts&quot;: {
     &quot;local-vs-2.1.7-canary.2&quot;: &quot;tsx ./src/local-vs-2.1.7-canary.2.ts&quot;,
</file context>
Fix with Cubic

await fetch(`${localServer.url}/preview/magic-links/notion-magic-link`);
await fetch(`${canaryServer.url}/preview/magic-links/notion-magic-link`);

await bench.run();
Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot Sep 17, 2025

Choose a reason for hiding this comment

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

Ensure server subprocesses are terminated via try/finally so they’re killed even if an await (fetch/run/writeFile) throws.

Prompt for AI agents
Address the following comment on benchmarks/preview-server/src/local-vs-2.1.7-canary.2.ts at line 35:

<comment>Ensure server subprocesses are terminated via try/finally so they’re killed even if an await (fetch/run/writeFile) throws.</comment>

<file context>
@@ -15,32 +15,30 @@ const pathToLocalCliScript = path.resolve(
+await fetch(`${localServer.url}/preview/magic-links/notion-magic-link`);
+await fetch(`${canaryServer.url}/preview/magic-links/notion-magic-link`);
+
+await bench.run();
+
+localServer.subprocess.kill();
</file context>
Fix with Cubic

});
bench
.add('startup on local', async () => {
const server = await runServer(pathToLocalCliScript);
Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot Sep 17, 2025

Choose a reason for hiding this comment

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

Awaiting runServer without a timeout can hang indefinitely if the child process fails to emit the expected stdout.

Prompt for AI agents
Address the following comment on benchmarks/preview-server/src/local-vs-2.1.7-canary.2-on-startup.ts at line 24:

<comment>Awaiting runServer without a timeout can hang indefinitely if the child process fails to emit the expected stdout.</comment>

<file context>
@@ -15,28 +15,26 @@ const pathToLocalCliScript = path.resolve(
-    });
+bench
+  .add(&#39;startup on local&#39;, async () =&gt; {
+    const server = await runServer(pathToLocalCliScript);
+    await fetch(`${server.url}/preview/magic-links/notion-magic-link`);
+    server.subprocess.kill();
</file context>
Fix with Cubic

.add('startup on local', async () => {
const server = await runServer(pathToLocalCliScript);
await fetch(`${server.url}/preview/magic-links/notion-magic-link`);
server.subprocess.kill();
Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot Sep 17, 2025

Choose a reason for hiding this comment

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

kill() is asynchronous; await the child process 'close' event after sending the signal to ensure the server fully exits before the next iteration.

Prompt for AI agents
Address the following comment on benchmarks/preview-server/src/local-vs-2.1.7-canary.2-on-startup.ts at line 26:

<comment>kill() is asynchronous; await the child process &#39;close&#39; event after sending the signal to ensure the server fully exits before the next iteration.</comment>

<file context>
@@ -15,28 +15,26 @@ const pathToLocalCliScript = path.resolve(
+  .add(&#39;startup on local&#39;, async () =&gt; {
+    const server = await runServer(pathToLocalCliScript);
+    await fetch(`${server.url}/preview/magic-links/notion-magic-link`);
+    server.subprocess.kill();
+  })
+  .add(&#39;startup on 2.1.7-canary.2&#39;, async () =&gt; {
</file context>
Fix with Cubic

server.subprocess.kill();
})
.add('startup on 2.1.7-canary.2', async () => {
const server = await runServer(pathToCanaryCliScript);
Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot Sep 17, 2025

Choose a reason for hiding this comment

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

Subprocess may be left running if fetch throws; wrap server lifecycle in a try/finally so kill() always runs for cleanup.

Prompt for AI agents
Address the following comment on benchmarks/preview-server/src/local-vs-2.1.7-canary.2-on-startup.ts at line 29:

<comment>Subprocess may be left running if fetch throws; wrap server lifecycle in a try/finally so kill() always runs for cleanup.</comment>

<file context>
@@ -15,28 +15,26 @@ const pathToLocalCliScript = path.resolve(
+    server.subprocess.kill();
+  })
+  .add(&#39;startup on 2.1.7-canary.2&#39;, async () =&gt; {
+    const server = await runServer(pathToCanaryCliScript);
+    await fetch(`${server.url}/preview/magic-links/notion-magic-link`);
+    server.subprocess.kill();
</file context>
Fix with Cubic

});
bench
.add('startup on local', async () => {
const server = await runServer(pathToLocalCliScript);
Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot Sep 17, 2025

Choose a reason for hiding this comment

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

Subprocess may be left running if fetch throws; wrap server lifecycle in a try/finally so kill() always runs for cleanup.

Prompt for AI agents
Address the following comment on benchmarks/preview-server/src/local-vs-2.1.7-canary.2-on-startup.ts at line 24:

<comment>Subprocess may be left running if fetch throws; wrap server lifecycle in a try/finally so kill() always runs for cleanup.</comment>

<file context>
@@ -15,28 +15,26 @@ const pathToLocalCliScript = path.resolve(
-    });
+bench
+  .add(&#39;startup on local&#39;, async () =&gt; {
+    const server = await runServer(pathToLocalCliScript);
+    await fetch(`${server.url}/preview/magic-links/notion-magic-link`);
+    server.subprocess.kill();
</file context>
Fix with Cubic

@gabrielmfern gabrielmfern marked this pull request as draft September 17, 2025 20:50
Copy link

Copy link

Caution

Review the following alerts detected in dependencies.

According to your organization's Security Policy, you must resolve all "Block" alerts before proceeding. Learn more about Socket for GitHub.

Action Severity Alert  (click "▶" to expand/collapse)
Block High
[email protected] has Telemetry.

Note: Can be disabled by setting the environment variable NEXT_TELEMETRY_DISABLED=1 . See https://nextjs.org/telemetry for more information

From: examples/resend/package.jsonnpm/[email protected]

ℹ Read more on: This package | This alert | What is telemetry?

Next steps: Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at [email protected].

Suggestion: Most telemetry comes with settings to disable it. Consider disabling telemetry if you do not want to be tracked.

Mark the package as acceptable risk. To ignore this alert only in this pull request, reply with the comment @SocketSecurity ignore npm/[email protected]. You can also ignore all packages with @SocketSecurity ignore-all. To ignore an alert for all future pull requests, use Socket's Dashboard to change the triage state of this alert.

Block Medium
[email protected] has Install scripts.

Install script: install

Source: node install/check.js

From: ?npm/[email protected]npm/[email protected]

ℹ Read more on: This package | This alert | What is an install script?

Next steps: Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at [email protected].

Suggestion: Packages should not be running non-essential scripts during install and there are often solutions to problems people solve with install scripts that can be run at publish time instead.

Mark the package as acceptable risk. To ignore this alert only in this pull request, reply with the comment @SocketSecurity ignore npm/[email protected]. You can also ignore all packages with @SocketSecurity ignore-all. To ignore an alert for all future pull requests, use Socket's Dashboard to change the triage state of this alert.

View full report

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