Skip to content

Build: NX resource-class eval — linux-browsers-js = extra_large+#34572

Closed
kasperpeulen wants to merge 30 commits into
kasper/nx-aifrom
kasper/nx-rc-xlarge-plus
Closed

Build: NX resource-class eval — linux-browsers-js = extra_large+#34572
kasperpeulen wants to merge 30 commits into
kasper/nx-aifrom
kasper/nx-rc-xlarge-plus

Conversation

@kasperpeulen
Copy link
Copy Markdown
Member

@kasperpeulen kasperpeulen commented Apr 17, 2026

Closes #

What I did

NX Cloud resource-class sweep — branch 1 of 6. Do not merge.

Changes exactly one line: .nx/workflows/agents.yamllinux-browsers-js.resource-class = docker_linux_amd64/extra_large+ (60 credits/min).

Part of measuring the real cost / duration / flake curve across linux-browsers-js resource classes on real ci:normal traffic. Replaces the single linearly-extrapolated "medium+ projected" column in the NX-vs-CircleCI findings canvas with a 6-point data-backed curve. linux-js stays at extra_large+ across all branches. Parent branch kasper/nx-ai (#34282) strips CircleCI + NX_EXPERIMENT so these 6 PRs only exercise NX Cloud.

Sweep matrix:

Branch linux-browsers-js credits/min
kasper/nx-rc-xlarge-plus extra_large+ 60
kasper/nx-rc-xlarge extra_large 40
kasper/nx-rc-large-plus large+ 30
kasper/nx-rc-large large 20
kasper/nx-rc-medium-plus medium+ 15
kasper/nx-rc-medium medium 10

Traffic plan: cache-bust every ~10 min per branch until 30 runs/branch or Enterprise trial expiry (2026-04-19). Data → scripts/ci-eval.db → new "Resource class sweep" section in the findings canvas with cost + duration bar charts and a measured recommendation.

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

  • stories
  • unit tests
  • integration tests
  • end-to-end tests

Manual testing

Caution

This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!

No manual test necessary — this PR exists only to exercise CI traffic. Verify by opening the PR's NX Cloud CIPE (GH check nx: normal) and confirming the linux-browsers-js agents are running on docker_linux_amd64/extra_large+ (visible in the agent metadata on cloud.nx.app). The sync script scripts/evaluate-ci.ts records credit_multiplier = 60 per CIPE in scripts/ci-eval.db.

Documentation

  • Add or update documentation reflecting your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Checklist for Maintainers

  • When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli-storybook/src/sandbox-templates.ts

  • Make sure this PR contains one of the labels below:

    Available labels
    • bug: Internal changes that fixes incorrect behavior.
    • maintenance: User-facing maintenance tasks.
    • dependencies: Upgrading (sometimes downgrading) dependencies.
    • build: Internal-facing build tooling & test updates. Will not show up in release changelog.
    • cleanup: Minor cleanup style change. Will not show up in release changelog.
    • documentation: Documentation only changes. Will not show up in release changelog.
    • feature request: Introducing a new feature.
    • BREAKING CHANGE: Changes that break compatibility in some way with current major version.
    • other: Changes that don't fit in the above categories.

🦋 Canary release

This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the @storybookjs/core team here.

core team members can create a canary release here or locally with gh workflow run --repo storybookjs/storybook publish.yml --field pr=<PR_NUMBER>

@nx-cloud
Copy link
Copy Markdown

nx-cloud Bot commented Apr 17, 2026

View your CI Pipeline Execution ↗ for commit 4065d56

Command Status Duration Result
nx run-many -t compile,check,knip,test,lint,fmt... ✅ Succeeded 11m 6s View ↗

☁️ Nx Cloud last updated this comment at 2026-04-17 20:17:49 UTC

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 17, 2026

📝 Walkthrough

Walkthrough

This PR transitions CI infrastructure by removing CircleCI's dynamic configuration entrypoint and trigger workflow, introducing an NX experiment flag to conditionally exclude certain jobs, and adding a comprehensive CI evaluation system with SQLite-based metrics database and analytics scripts for comparing CircleCI vs NX Cloud performance across runs.

Changes

Cohort / File(s) Summary
CI Infrastructure Removal
.circleci/config.yml, .github/workflows/trigger-circle-ci-workflow.yml
Removed CircleCI dynamic config setup (setup: true, pipeline parameters, generation job, and setup workflow) and deleted GitHub Actions workflow that triggered CircleCI pipelines via API, including branch computation and label-based parameter selection logic.
CI Workflow Configuration
.github/workflows/nx.yml, nx.json
Added concurrency group configuration to prevent cancellation of in-progress runs and updated codexCacheBust timestamp for cache invalidation.
CI Job Generation with NX Experiment Flag
scripts/ci/init-empty.ts, scripts/ci/sandboxes.ts
Updated function signatures to accept optional { nxExperiment?: boolean } parameter, conditionally excluding Windows-related jobs and chromatic task when experiment flag is enabled.
CI Evaluation & Analytics System
scripts/evaluate-ci.ts, scripts/generate-canvas-data.ts, scripts/investigate-nx-cache.ts, scripts/run-backfill-only.ts, scripts/ci-eval.db.README.md
Added comprehensive SQLite-based CI metrics database (ci-eval.db) infrastructure with evaluation script that syncs CircleCI and NX Cloud run data via paginated API calls, canvas data generation script for inline TypeScript constant updates, investigation utility for NX Cloud cache analysis, backfill-only sync script, and detailed schema documentation including tables for runs, failed tasks, credits, retry stats, cache stats, and flaky task analytics.
Build System & Configuration
package.json, .cursor/mcp.json, .gitignore
Bumped @nx/workspace and nx from ^22.6.1 to ^22.6.5, replaced Wallaby MCP server with NX MCP server, and added scripts/ci-eval.db to ignore patterns.
Project Metadata
test-storybooks/portable-stories-kitchen-sink/react-vitest-3/project.json, test-storybooks/yarn-pnp/project.json
Updated Nx project tags arrays to remove or reduce CI-related labels: first removed "ci:normal" leaving only "ci:merged" and "ci:daily"; second reduced to only "ci:daily".

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

  • #32925 — Directly removes CircleCI dynamic config entrypoint and associated infrastructure that were previously added
  • #33194 — Overlaps on CI infrastructure changes including .circleci/config.yml and .github/workflows/nx.yml modifications
  • #33720 — Modifies the same scripts/ci/sandboxes.ts file with executor-size changes alongside experiment flag updates
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch

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

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: 4

🧹 Nitpick comments (7)
scripts/ci/sandboxes.ts (1)

142-143: Module-level mutable state creates implicit coupling.

Using a module-level nxExperiment variable that's mutated by getSandboxes and read by defineSandboxFlow creates hidden dependencies. If getSandboxes is called multiple times with different options, the state persists incorrectly.

Consider threading nxExperiment through the function parameters instead:

♻️ Proposed refactor to pass flag through parameters
-let nxExperiment = false;
-
-export function defineSandboxFlow<Key extends string>(key: Key) {
+export function defineSandboxFlow<Key extends string>(key: Key, nxExperiment = false) {

Then update the call in getSandboxes:

-  const sandboxes = getListOfSandboxes(workflow).map(defineSandboxFlow);
+  const sandboxes = getListOfSandboxes(workflow).map((key) => defineSandboxFlow(key, nxExperiment));
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@scripts/ci/sandboxes.ts` around lines 142 - 143, The module-level mutable
variable nxExperiment introduces hidden coupling between getSandboxes and
defineSandboxFlow; remove nxExperiment from module scope and thread it as an
explicit boolean parameter instead: compute the flag locally inside getSandboxes
(or its callers) and pass it into defineSandboxFlow (and any other functions
that currently read nxExperiment), update the defineSandboxFlow signature to
accept the nxExperiment flag, and update all call sites of
defineSandboxFlow/getSandboxes to supply the flag so state is not shared
implicitly.
.github/workflows/nx.yml (1)

13-16: Consider cancel-in-progress: true for normal development.

Setting cancel-in-progress: false makes sense for the evaluation sweep (collecting 30 runs per branch), but for typical development workflows, canceling superseded runs saves CI resources. Consider reverting this to true after the evaluation or making it conditional.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.github/workflows/nx.yml around lines 13 - 16, The concurrency block
currently sets cancel-in-progress: false which keeps superseded workflow runs
running; update the concurrency configuration in the .github/workflows/nx.yml
file by setting cancel-in-progress: true for normal development (or make it
conditional using an environment variable or workflow input/evaluation flag so
it remains false only during the evaluation sweep). Locate the concurrency block
(symbols: concurrency, group, cancel-in-progress) and either change
cancel-in-progress to true or add a conditional that flips it based on a flag
(e.g., EVALUATION_SWEEP or a workflow input) so CI runs are canceled for typical
PR updates but retained during the evaluation period.
scripts/run-backfill-only.ts (2)

101-104: Duplicated inferAgentTemplate function.

This function and LINUX_JS_TARGETS set are duplicated from scripts/evaluate-ci.ts (lines 1128-1132). This should be extracted to the same shared module suggested for getNxCloudHeaders.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@scripts/run-backfill-only.ts` around lines 101 - 104, The inferAgentTemplate
function and LINUX_JS_TARGETS set are duplicated; extract them into the shared
module you created/reused for getNxCloudHeaders so both scripts import the same
implementation. Move the const LINUX_JS_TARGETS and function inferAgentTemplate
to that shared module, export them, then replace the local definitions in
scripts/run-backfill-only.ts and scripts/evaluate-ci.ts with imports of the
shared inferAgentTemplate and LINUX_JS_TARGETS to eliminate duplication and keep
behavior identical.

155-157: Empty catch blocks discard error details.

When API calls fail, the empty catch blocks at lines 155-157 and 256-258 only increment a counter. Logging the error message would help diagnose issues during backfill runs.

🔧 Proposed improvement
-    } catch {
+    } catch (e: any) {
+      if (process.env.DEBUG) console.error(`  API error for ${id}: ${e.message}`);
       apiFailures++;
     }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@scripts/run-backfill-only.ts` around lines 155 - 157, The catch blocks in
scripts/run-backfill-only.ts currently only do "apiFailures++" which discards
error details; change both catches to catch the error (e.g., "catch (err)") and
log the error before incrementing apiFailures — include err.message and
err.stack (or JSON.stringify(err) if needed) and a short contextual message
identifying the API call or loop iteration so failures can be diagnosed; keep
the existing apiFailures++ behavior after logging.
scripts/investigate-nx-cache.ts (1)

19-42: Duplicated getNxCloudHeaders function across multiple scripts.

This function is duplicated in scripts/run-backfill-only.ts (lines 22-45) and scripts/evaluate-ci.ts (lines 389-413). Consider extracting to a shared utility module.

♻️ Proposed: Extract to shared module

Create scripts/lib/nx-cloud-auth.ts:

import { readFileSync } from 'node:fs';
import { join } from 'node:path';
import { parse as parseIni } from 'ini';

const NX_CLOUD_URL = 'https://cloud.nx.app';
const NX_CLOUD_ID = '6929fbef73e98d8094d2a343';

export function getNxCloudHeaders(): Record<string, string> {
  // ... shared implementation
}

Then import in each script:

import { getNxCloudHeaders } from './lib/nx-cloud-auth.ts';
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@scripts/investigate-nx-cache.ts` around lines 19 - 42, Extract the duplicated
getNxCloudHeaders function into a single shared module (e.g., create
scripts/lib/nx-cloud-auth.ts) that exports getNxCloudHeaders and the constants
NX_CLOUD_URL and NX_CLOUD_ID, copy the existing implementation (including
reading env vars, fallback ini parsing, and header building) into that module,
then replace the duplicated definitions in scripts/investigate-nx-cache.ts,
scripts/run-backfill-only.ts, and scripts/evaluate-ci.ts by importing
getNxCloudHeaders from the new module and removing the local duplicate
functions.
scripts/evaluate-ci.ts (1)

168-182: Silent migration failures could mask schema issues.

The migration try/catch blocks silently swallow all errors. While this is intentional for "column already exists" errors, it also hides actual failures (e.g., disk full, permissions). Consider logging in debug mode.

🔧 Optional: Log migration errors in debug mode
   // Migration: add commit_sha column to existing databases
   try {
     db.exec(`ALTER TABLE runs ADD COLUMN commit_sha TEXT`);
-  } catch {}
+  } catch (e: any) {
+    // Expected to fail if column exists; log only if DEBUG for other errors
+    if (process.env.DEBUG && !e.message?.includes('duplicate column')) {
+      console.warn(`Migration warning: ${e.message}`);
+    }
+  }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@scripts/evaluate-ci.ts` around lines 168 - 182, The empty catch blocks around
the migration db.exec calls swallow all errors; update the two migration blocks
(the ALTER TABLE runs ADD COLUMN commit_sha and ALTER TABLE nx_cipe_retry_stats
ADD COLUMN hypothetical_no_cache_ms) to handle errors more safely by inspecting
the thrown error: if it indicates the column already exists, ignore it,
otherwise log the full error and SQL (using the project's logger or
console.debug) or rethrow; include the SQL text and error details in the log so
real migration failures (disk/permission/etc.) are visible.
scripts/generate-canvas-data.ts (1)

563-577: Template literals with embedded expressions may cause incorrect parsing.

The brace-matching logic tracks backtick strings but doesn't handle ${...} expressions inside template literals. If a replaced constant contains a template literal like `value: ${obj.prop}`, the { inside ${...} would incorrectly increment depth.

This is likely acceptable for the current use case if the canvas constants don't contain such patterns, but worth noting for future robustness.

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@scripts/ci/sandboxes.ts`:
- Around line 521-522: The getSandboxes(workflow, options) function declares the
nxExperiment flag but the caller that invokes getSandboxes(...) never passes
options so nxExperiment is always false; fix by either (A) updating the caller
that invokes getSandboxes(...) to pass { nxExperiment: true } or a boolean
derived from CI config when you want experimental NX jobs enabled, ensuring the
call supplies the options object, or (B) if the option is unused project-wide,
remove the options parameter and the nxExperiment references inside getSandboxes
to eliminate dead code; locate the flag usage in getSandboxes and the single
caller site that invokes getSandboxes(...) to apply one of these two fixes.

In `@scripts/evaluate-ci.ts`:
- Around line 729-743: resolvePRNumber currently calls GitHub API even when no
token is set (it passes an empty Authorization header), risking unauthenticated
rate limits; update the function to first check process.env.GITHUB_TOKEN and
skip or short-circuit the network request when it's missing (e.g., return branch
or cached value immediately) or explicitly fail with a clear error, and only
call fetchJSON with the Authorization header when a non-empty token exists;
reference symbols: resolvePRNumber, prNumberCache, fetchJSON.

In `@scripts/generate-canvas-data.ts`:
- Around line 26-29: CANVAS_PATH is hardcoded to a user-specific absolute path
causing failures for other devs; update the scripts/generate-canvas-data.ts
logic that defines CANVAS_PATH to use a portable default (e.g., resolve relative
to the script using import.meta.dirname or __dirname) and allow override via a
CLI flag or environment variable (check process.env.CANVAS_PATH or parse an
--canvas argument and use its value if present), keeping the symbol name
CANVAS_PATH and falling back to the relative path when no override is provided.

In `@scripts/investigate-nx-cache.ts`:
- Around line 75-78: The code calls fetch(...).then(r => r.json()) without
checking HTTP status, which can throw on error responses; change the fetch usage
in investigate-nx-cache.ts (the call that uses NX_CLOUD_URL, getNxCloudHeaders,
and cipeId to produce details and rg) to first await the Response, verify
response.ok (or throw a new Error including response.status and statusText or
response body), and only then parse response.json() into details so any HTTP
errors produce a clear, descriptive error instead of a JSON parse failure.

---

Nitpick comments:
In @.github/workflows/nx.yml:
- Around line 13-16: The concurrency block currently sets cancel-in-progress:
false which keeps superseded workflow runs running; update the concurrency
configuration in the .github/workflows/nx.yml file by setting
cancel-in-progress: true for normal development (or make it conditional using an
environment variable or workflow input/evaluation flag so it remains false only
during the evaluation sweep). Locate the concurrency block (symbols:
concurrency, group, cancel-in-progress) and either change cancel-in-progress to
true or add a conditional that flips it based on a flag (e.g., EVALUATION_SWEEP
or a workflow input) so CI runs are canceled for typical PR updates but retained
during the evaluation period.

In `@scripts/ci/sandboxes.ts`:
- Around line 142-143: The module-level mutable variable nxExperiment introduces
hidden coupling between getSandboxes and defineSandboxFlow; remove nxExperiment
from module scope and thread it as an explicit boolean parameter instead:
compute the flag locally inside getSandboxes (or its callers) and pass it into
defineSandboxFlow (and any other functions that currently read nxExperiment),
update the defineSandboxFlow signature to accept the nxExperiment flag, and
update all call sites of defineSandboxFlow/getSandboxes to supply the flag so
state is not shared implicitly.

In `@scripts/evaluate-ci.ts`:
- Around line 168-182: The empty catch blocks around the migration db.exec calls
swallow all errors; update the two migration blocks (the ALTER TABLE runs ADD
COLUMN commit_sha and ALTER TABLE nx_cipe_retry_stats ADD COLUMN
hypothetical_no_cache_ms) to handle errors more safely by inspecting the thrown
error: if it indicates the column already exists, ignore it, otherwise log the
full error and SQL (using the project's logger or console.debug) or rethrow;
include the SQL text and error details in the log so real migration failures
(disk/permission/etc.) are visible.

In `@scripts/investigate-nx-cache.ts`:
- Around line 19-42: Extract the duplicated getNxCloudHeaders function into a
single shared module (e.g., create scripts/lib/nx-cloud-auth.ts) that exports
getNxCloudHeaders and the constants NX_CLOUD_URL and NX_CLOUD_ID, copy the
existing implementation (including reading env vars, fallback ini parsing, and
header building) into that module, then replace the duplicated definitions in
scripts/investigate-nx-cache.ts, scripts/run-backfill-only.ts, and
scripts/evaluate-ci.ts by importing getNxCloudHeaders from the new module and
removing the local duplicate functions.

In `@scripts/run-backfill-only.ts`:
- Around line 101-104: The inferAgentTemplate function and LINUX_JS_TARGETS set
are duplicated; extract them into the shared module you created/reused for
getNxCloudHeaders so both scripts import the same implementation. Move the const
LINUX_JS_TARGETS and function inferAgentTemplate to that shared module, export
them, then replace the local definitions in scripts/run-backfill-only.ts and
scripts/evaluate-ci.ts with imports of the shared inferAgentTemplate and
LINUX_JS_TARGETS to eliminate duplication and keep behavior identical.
- Around line 155-157: The catch blocks in scripts/run-backfill-only.ts
currently only do "apiFailures++" which discards error details; change both
catches to catch the error (e.g., "catch (err)") and log the error before
incrementing apiFailures — include err.message and err.stack (or
JSON.stringify(err) if needed) and a short contextual message identifying the
API call or loop iteration so failures can be diagnosed; keep the existing
apiFailures++ behavior after logging.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: c9c2b57f-9c5f-4e91-9104-2c5fdbac33ed

📥 Commits

Reviewing files that changed from the base of the PR and between 09fb0ba and c79f14b.

⛔ Files ignored due to path filters (1)
  • yarn.lock is excluded by !**/yarn.lock, !**/*.lock
📒 Files selected for processing (16)
  • .circleci/config.yml
  • .cursor/mcp.json
  • .github/workflows/nx.yml
  • .github/workflows/trigger-circle-ci-workflow.yml
  • .gitignore
  • nx.json
  • package.json
  • scripts/ci-eval.db.README.md
  • scripts/ci/init-empty.ts
  • scripts/ci/sandboxes.ts
  • scripts/evaluate-ci.ts
  • scripts/generate-canvas-data.ts
  • scripts/investigate-nx-cache.ts
  • scripts/run-backfill-only.ts
  • test-storybooks/portable-stories-kitchen-sink/react-vitest-3/project.json
  • test-storybooks/yarn-pnp/project.json
💤 Files with no reviewable changes (2)
  • .circleci/config.yml
  • .github/workflows/trigger-circle-ci-workflow.yml

Comment thread scripts/ci/sandboxes.ts
Comment on lines +521 to +522
export function getSandboxes(workflow: Workflow, options: { nxExperiment?: boolean } = {}) {
nxExperiment = options.nxExperiment ?? false;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Find all call sites of getSandboxes to verify they're updated appropriately
rg -n "getSandboxes\(" --type ts -A2

Repository: storybookjs/storybook

Length of output: 579


🏁 Script executed:

#!/bin/bash
# Check for all call sites of getSandboxes in the entire codebase
echo "=== All getSandboxes call sites ==="
rg -n "getSandboxes\(" --type ts

echo -e "\n=== Check if nxExperiment is used in the function body ==="
# Get the function body from sandboxes.ts starting at line 521
sed -n '521,600p' scripts/ci/sandboxes.ts | head -100

Repository: storybookjs/storybook

Length of output: 1006


The nxExperiment option is not being utilized at the call site.

The getSandboxes() function defines an options parameter to control whether experimental NX jobs are added (via the !nxExperiment condition on line 530), but the call in scripts/ci/main.ts:48 does not pass this option. This means nxExperiment will always be false at this call site, rendering the parameter ineffective. Either pass the option from the call site or remove the unused parameter.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@scripts/ci/sandboxes.ts` around lines 521 - 522, The getSandboxes(workflow,
options) function declares the nxExperiment flag but the caller that invokes
getSandboxes(...) never passes options so nxExperiment is always false; fix by
either (A) updating the caller that invokes getSandboxes(...) to pass {
nxExperiment: true } or a boolean derived from CI config when you want
experimental NX jobs enabled, ensuring the call supplies the options object, or
(B) if the option is unused project-wide, remove the options parameter and the
nxExperiment references inside getSandboxes to eliminate dead code; locate the
flag usage in getSandboxes and the single caller site that invokes
getSandboxes(...) to apply one of these two fixes.

Comment thread scripts/evaluate-ci.ts
Comment on lines +729 to +743
async function resolvePRNumber(branch: string): Promise<string> {
if (prNumberCache.has(branch)) return prNumberCache.get(branch)!;

try {
const data = await fetchJSON<{ number: number }[]>(
`https://api.github.com/repos/storybookjs/storybook/pulls?head=storybookjs:${branch}&state=open&per_page=1`,
{ headers: { Authorization: `token ${process.env.GITHUB_TOKEN ?? ''}` } }
);
const num = data[0]?.number?.toString() ?? branch;
prNumberCache.set(branch, num);
return num;
} catch {
return branch;
}
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

GitHub API calls may hit rate limits without authentication.

resolvePRNumber makes GitHub API calls with an optional token (falling back to empty string). Unauthenticated GitHub API requests are limited to 60/hour, which could be exhausted quickly when syncing many branches.

🛡️ Suggested improvement
 async function resolvePRNumber(branch: string): Promise<string> {
   if (prNumberCache.has(branch)) return prNumberCache.get(branch)!;

+  const token = process.env.GITHUB_TOKEN;
+  if (!token) {
+    // Skip API call if no token - just use branch name
+    prNumberCache.set(branch, branch);
+    return branch;
+  }
+
   try {
     const data = await fetchJSON<{ number: number }[]>(
       `https://api.github.com/repos/storybookjs/storybook/pulls?head=storybookjs:${branch}&state=open&per_page=1`,
-      { headers: { Authorization: `token ${process.env.GITHUB_TOKEN ?? ''}` } }
+      { headers: { Authorization: `token ${token}` } }
     );
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@scripts/evaluate-ci.ts` around lines 729 - 743, resolvePRNumber currently
calls GitHub API even when no token is set (it passes an empty Authorization
header), risking unauthenticated rate limits; update the function to first check
process.env.GITHUB_TOKEN and skip or short-circuit the network request when it's
missing (e.g., return branch or cached value immediately) or explicitly fail
with a clear error, and only call fetchJSON with the Authorization header when a
non-empty token exists; reference symbols: resolvePRNumber, prNumberCache,
fetchJSON.

Comment on lines +26 to +29
const CANVAS_PATH = join(
process.env.HOME!,
'.cursor/projects/Users-kasperpeulen-cursor-worktrees-storybook-jafl/canvases/nx-vs-circleci-findings.canvas.tsx'
);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Hardcoded user-specific path will break for other developers.

CANVAS_PATH is hardcoded to a specific user's local path (/Users-kasperpeulen-cursor-worktrees-storybook-jafl/...). This script will fail for anyone else trying to run it.

🔧 Proposed fix: Use a relative or configurable path
 const CANVAS_PATH = join(
-  process.env.HOME!,
-  '.cursor/projects/Users-kasperpeulen-cursor-worktrees-storybook-jafl/canvases/nx-vs-circleci-findings.canvas.tsx'
+  import.meta.dirname,
+  '../canvases/nx-vs-circleci-findings.canvas.tsx'
 );

Alternatively, accept it as a CLI argument:

const CANVAS_PATH = args.includes('--canvas')
  ? args[args.indexOf('--canvas') + 1]
  : join(import.meta.dirname, '../canvases/nx-vs-circleci-findings.canvas.tsx');
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const CANVAS_PATH = join(
process.env.HOME!,
'.cursor/projects/Users-kasperpeulen-cursor-worktrees-storybook-jafl/canvases/nx-vs-circleci-findings.canvas.tsx'
);
const CANVAS_PATH = process.env.CANVAS_PATH || join(
process.cwd(),
'canvases/nx-vs-circleci-findings.canvas.tsx'
);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@scripts/generate-canvas-data.ts` around lines 26 - 29, CANVAS_PATH is
hardcoded to a user-specific absolute path causing failures for other devs;
update the scripts/generate-canvas-data.ts logic that defines CANVAS_PATH to use
a portable default (e.g., resolve relative to the script using
import.meta.dirname or __dirname) and allow override via a CLI flag or
environment variable (check process.env.CANVAS_PATH or parse an --canvas
argument and use its value if present), keeping the symbol name CANVAS_PATH and
falling back to the relative path when no override is provided.

Comment thread scripts/investigate-nx-cache.ts Outdated
Comment on lines +75 to +78
const details = await fetch(`${NX_CLOUD_URL}/nx-cloud/mcp-context/pipeline-executions/${cipeId}`, {
headers: getNxCloudHeaders(),
}).then((r) => r.json());
const rg = details?.runGroups?.[0]?.runGroupName;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Missing response status check before parsing JSON.

The fetch at line 75-77 calls .json() without first checking if the response was successful. If the API returns an error status, this will likely throw a confusing JSON parse error instead of a meaningful HTTP error.

🛡️ Proposed fix
-  const details = await fetch(`${NX_CLOUD_URL}/nx-cloud/mcp-context/pipeline-executions/${cipeId}`, {
-    headers: getNxCloudHeaders(),
-  }).then((r) => r.json());
+  const detailsRes = await fetch(`${NX_CLOUD_URL}/nx-cloud/mcp-context/pipeline-executions/${cipeId}`, {
+    headers: getNxCloudHeaders(),
+  });
+  if (!detailsRes.ok) {
+    console.error(`Failed to fetch pipeline execution: HTTP ${detailsRes.status}`);
+    process.exit(1);
+  }
+  const details = await detailsRes.json();
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@scripts/investigate-nx-cache.ts` around lines 75 - 78, The code calls
fetch(...).then(r => r.json()) without checking HTTP status, which can throw on
error responses; change the fetch usage in investigate-nx-cache.ts (the call
that uses NX_CLOUD_URL, getNxCloudHeaders, and cipeId to produce details and rg)
to first await the Response, verify response.ok (or throw a new Error including
response.status and statusText or response body), and only then parse
response.json() into details so any HTTP errors produce a clear, descriptive
error instead of a JSON parse failure.

@kasperpeulen kasperpeulen added the build Internal-facing build tooling & test updates label Apr 17, 2026
@kasperpeulen kasperpeulen changed the title [eval][do-not-merge] linux-browsers-js = extra_large+ Build: NX resource-class eval — linux-browsers-js = extra_large+ Apr 17, 2026
@kasperpeulen kasperpeulen changed the base branch from next to kasper/nx-ai April 17, 2026 07:35
@kasperpeulen kasperpeulen marked this pull request as draft April 17, 2026 07:35
@kasperpeulen kasperpeulen force-pushed the kasper/nx-rc-xlarge-plus branch 2 times, most recently from b934e89 to e774fab Compare April 17, 2026 08:09
@kasperpeulen kasperpeulen force-pushed the kasper/nx-rc-xlarge-plus branch from b4763fc to 9ba80b8 Compare April 17, 2026 08:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

build Internal-facing build tooling & test updates ci:normal

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants