Skip to content

feat(cli): improve UX when throw error#431

Merged
bzp2010 merged 2 commits intomainfrom
bzp/feat-improve-source-map
Apr 16, 2026
Merged

feat(cli): improve UX when throw error#431
bzp2010 merged 2 commits intomainfrom
bzp/feat-improve-source-map

Conversation

@bzp2010
Copy link
Copy Markdown
Collaborator

@bzp2010 bzp2010 commented Apr 16, 2026

Description

By providing out-of-the-box source map remapping, error logs in single binary builds can display the original code paths and line and column numbers.

Checklist

  • I have explained the need for this PR and the problem it solves
  • I have explained the changes or the new features added to this PR
  • I have added tests corresponding to this change
  • I have updated the documentation to reflect this change
  • I have verified that this change is backward compatible

Summary by CodeRabbit

  • Chores

    • Improved source map support for better error reporting and resilient map loading (falls back to a warning instead of failing)
    • Updated dependencies and build/workspace configuration to enable linked source maps
  • Refactor

    • Shifted environment and initialization to occur during command setup to reduce import-time side effects
  • Other

    • No public API changes

Copilot AI review requested due to automatic review settings April 16, 2026 10:56
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 16, 2026

📝 Walkthrough

Walkthrough

Added CLI source-map support: SEA asset mapping for main.js.map, dependency and build config updates, runtime logic to install/initialize source-map handling (including SEA-aware retrieval), and moved some CLI initialization from module load to explicit setup calls.

Changes

Cohort / File(s) Summary
SEA & CLI build config
apps/cli/node-sea.json, apps/cli/package.json
Added SEA assets entry mapping main.js.map; changed esbuild sourcemap from "inline" to "linked" and removed explicit minify settings from build configs.
CLI runtime & source-map logic
apps/cli/src/main.ts
Added setupSourceMap() and call from bootstrap() to conditionally install source-map-support; for SEA runtimes attempts to load main.js.map via node:sea and provide a custom retrieveSourceMap.
CLI command initialization
apps/cli/src/command/index.ts
Moved top-level side-effect init into a new setupInternal() function and call it from setupCommands() / setupIngressCommands() (shifted init from import-time to invocation-time).
Dependencies & workspace
apps/cli/package.json, pnpm-workspace.yaml, package.json
Added source-map-support and related types; moved/updated several deps (e.g., semver moved to dependencies); added autoInstallPeers: true and updated pnpm workspace ordering; removed @nx/web from root devDependencies.

Sequence Diagram(s)

sequenceDiagram
  participant Bootstrap as Bootstrap
  participant SourceMapSetup as setupSourceMap()
  participant SEA as node:sea
  participant SMS as source-map-support
  Bootstrap->>SourceMapSetup: call setupSourceMap()
  alt runtime is SEA
    SourceMapSetup->>SEA: getAsset("main.js.map")
    SEA-->>SourceMapSetup: return asset content (or error)
    alt asset returned & parsed
      SourceMapSetup->>SMS: install with retrieveSourceMap(using preloaded map)
      SMS-->>SourceMapSetup: installed
    else parse/load fails
      SourceMapSetup-->>Bootstrap: warn and continue
    end
  else non-SEA runtime
    SourceMapSetup->>SMS: require/install default source-map-support
    SMS-->>SourceMapSetup: installed
  end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested reviewers

  • juzhiyuan
  • LiteSun
  • guoqqqi

Important

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

❌ Failed checks (1 error, 1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Security Check ❌ Error PR fails security check due to unaddressed dotenv.config() error handling where parse errors are silently swallowed with quiet:true flag. Capture dotenv.config() return value, check .error property, allow ENOENT errors, but throw on parse errors to prevent silent configuration failures.
E2e Test Quality Review ⚠️ Warning PR implements critical error-reporting infrastructure (source maps) without E2E test coverage and with incomplete error handling in dotenv.config(). Add comprehensive E2E tests for SEA/non-SEA runtimes and fix dotenv.config() error handling by capturing return value and throwing on parse errors.
Title check ❓ Inconclusive The title partially describes the PR's changes but misses the main focus. The PR primarily implements source map support for better error stack traces in single-binary builds, but the title only vaguely mentions 'improve UX when throw error' without specifying the actual implementation (source maps). Consider a more specific title like 'feat(cli): add source map support for improved error traces in SEA builds' to clearly communicate the key technical change.
✅ Passed checks (1 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch bzp/feat-improve-source-map

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

@bzp2010 bzp2010 added test/api7 Trigger the API7 test on the PR test/apisix-standalone Trigger the APISIX standalone test on the PR labels Apr 16, 2026
Copy link
Copy Markdown

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 improves ADC CLI error-stack UX by enabling source-map remapping (including in Node SEA single-binary builds), so stack traces can point to original source locations.

Changes:

  • Add and install source-map-support, with custom source-map retrieval when running under SEA.
  • Switch CLI esbuild sourcemaps from inline to linked and embed the .map as a SEA asset.
  • Update workspace deps/lockfile (including removing @nx/web from root devDependencies).

Reviewed changes

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

Show a summary per file
File Description
pnpm-workspace.yaml Adds source-map-support to the catalog and adjusts pnpm workspace settings.
pnpm-lock.yaml Lockfile updates reflecting new dependency and removed @nx/web.
package.json Removes @nx/web from root devDependencies.
apps/cli/src/main.ts Installs source-map-support and adds SEA-specific source map retrieval.
apps/cli/package.json Adds deps/types for source-map support and switches esbuild sourcemap mode.
apps/cli/node-sea.json Embeds the generated sourcemap as a SEA asset.
Files not reviewed (1)
  • pnpm-lock.yaml: Language not supported

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread apps/cli/src/main.ts
Comment thread apps/cli/src/main.ts
Comment thread apps/cli/package.json
Comment thread package.json
Copy link
Copy Markdown

@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

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

Inline comments:
In `@apps/cli/src/main.ts`:
- Around line 30-31: The static import of ./command should be changed to a
dynamic import inside the async bootstrap() so setupSourceMap() runs before any
top-level code in command executes; inside bootstrap(), after calling
setupSourceMap(), perform await import('./command') (or const { default: command
} = await import('./command')) and then continue using the imported module,
removing or replacing the top-level static import; update any references to the
formerly statically imported symbols to use the dynamically imported module
within bootstrap().
🪄 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: 7f073c7b-aaf3-4674-ae49-48c55de6223c

📥 Commits

Reviewing files that changed from the base of the PR and between d13f53a and b2b659c.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (5)
  • apps/cli/node-sea.json
  • apps/cli/package.json
  • apps/cli/src/main.ts
  • package.json
  • pnpm-workspace.yaml
💤 Files with no reviewable changes (1)
  • package.json

Comment thread apps/cli/src/main.ts
Copy link
Copy Markdown

@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

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

Inline comments:
In `@apps/cli/src/command/index.ts`:
- Around line 18-24: In setupInternal, capture the result of dotenv.config() and
handle errors explicitly: call dotenv.config(), inspect the returned object for
error, allow an ENOENT (missing .env) to pass silently, but for other errors
(parse failures) either log a clear warning or throw; update the setupInternal
function to check the config result and act accordingly so that malformed .env
files are not silently swallowed.
🪄 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: cc48d27a-309e-4841-b6f3-156a7420f7b4

📥 Commits

Reviewing files that changed from the base of the PR and between b2b659c and e786ebc.

📒 Files selected for processing (1)
  • apps/cli/src/command/index.ts

Comment thread apps/cli/src/command/index.ts
@bzp2010 bzp2010 merged commit 7cf7dfa into main Apr 16, 2026
55 of 56 checks passed
@bzp2010 bzp2010 deleted the bzp/feat-improve-source-map branch April 16, 2026 11:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

test/api7 Trigger the API7 test on the PR test/apisix-standalone Trigger the APISIX standalone test on the PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants