Skip to content

fix(central): no-issue: fix ESM build reference data export regression#10145

Open
tcaiger wants to merge 2 commits into
mainfrom
fix/xlsx-esm-fs-binding
Open

fix(central): no-issue: fix ESM build reference data export regression#10145
tcaiger wants to merge 2 commits into
mainfrom
fix/xlsx-esm-fs-binding

Conversation

@tcaiger

@tcaiger tcaiger commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Changes

After the build-less switch (#10097), the central server runs xlsx's ESM build directly. xlsx's ESM build does not bind Node's fs automatically, so readFile/writeFile throw cannot save file — breaking reference-data export (and any disk-backed import).

Fix: call XLSX.set_fs(fs) once at process start (app/index.js, the universal entrypoint for the server and all subcommands), plus in the export writer util (excelUtils.js) as a safety net for direct/test use. No behaviour change beyond enabling file IO.

Reproduced via GET /api/admin/export/referenceData returning a 500 (cannot save file ./export-*.xlsx); fixed after restart.

Auto-Deploy

  • Deploy
Options
  • Artillery load test
  • Seed from closest snapshot
  • Generate fake data
  • More data (20Gi)
  • No facility servers (central-only)
  • No sync (facility tasks scaled to zero)
  • AMD64 architecture (default is arm64)
  • Skip mobile build
  • Always build mobile
  • Stay up for 8 hours
  • Stay up for 24 hours
  • Stay up (no TTL)
  • Build images only (don't deploy)
  • Pause this deploy

Tests

  • Run E2E tests
  • Run DAST scan

Review Hero

  • Run Review Hero
  • Auto-fix review suggestions Wait for Review Hero to finish, resolve any comments you disagree with or want to fix manually, then check this to auto-fix the rest.
  • Auto-fix CI failures Check this to auto-fix lint errors, test failures, and other CI issues.
  • Auto-merge upstream Check this to merge the base branch into this PR, with AI conflict resolution if needed.
  • Save suppressions Check this to capture 👎 reactions on Review Hero comments as suppression rules in .github/review-hero/suppressions.yml. Also runs automatically at the end of any auto-fix run.

Remember to...

  • ...write or update tests
  • ...add UI screenshots and testing notes to the Linear issue
  • ...add any manual upgrade steps to the Linear issue
  • ...update the config reference, settings reference, or any relevant runbook(s)
  • ...call out additions or changes to config files for the deployment team to take note of

…export/import works

After the build-less switch (#10097) the central server runs xlsx's ESM build
directly, which does not bind Node's fs automatically. readFile/writeFile then
throw "cannot save file", breaking reference-data export (and disk-backed
imports). Call XLSX.set_fs(fs) once at process start, plus in the export writer
util for direct/test use.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@tcaiger tcaiger changed the title fix(central): no-issue: bind fs for xlsx ESM build so reference-data export/import works fix(central): no-issue: bind fs for xlsx ESM build for reference-data export Jun 25, 2026
@review-hero

review-hero Bot commented Jun 25, 2026

Copy link
Copy Markdown

🦸 Review Hero Summary
9 agents reviewed this PR | 0 critical | 0 suggestions | 0 nitpicks | Filtering: consensus 3 voters

No issues found. Looks good!

@tcaiger tcaiger changed the title fix(central): no-issue: bind fs for xlsx ESM build for reference-data export fix(central): no-issue: fix reference data export regression Jun 25, 2026
@tcaiger tcaiger changed the title fix(central): no-issue: fix reference data export regression fix(central): no-issue: fix ESM build reference data export regression Jun 25, 2026
@review-hero

review-hero Bot commented Jun 25, 2026

Copy link
Copy Markdown

🦸 Review Hero Summary
6 agents reviewed this PR | 0 critical | 1 suggestion | 0 nitpicks | Filtering: consensus 3 voters

Local fix prompt (copy to your coding agent)

Fix these issues identified on the pull request. One commit per issue fixed.


packages/central-server/app/utils/excelUtils.js:5: XLSX.set_fs(fs) is called here at module initialisation AND again in index.js. Due to ESM semantics, module bodies of dependencies execute before the importing module's body, so this call in excelUtils.js will always run before the one in index.js. The comment in index.js ('bind it once at process start') is therefore misleading — it actually runs last. More importantly, any module that uses xlsx directly without importing excelUtils.js won't have the binding applied until index.js's body runs. Pick one canonical place: if the intent is a guaranteed early binding, keep only the index.js call (it will run before any user-initiated xlsx operations, just not before dependency module bodies). If the intent is defensive per-module binding, remove the index.js call and add the same pattern to every file that uses xlsx. Having both creates maintenance confusion about which is the real fix.

Comment thread packages/central-server/app/utils/excelUtils.js Outdated
The jest-resolved xlsx build auto-binds fs and does not expose set_fs, so the
unconditional call threw "set_fs is not a function" at module load and failed
the suites. Guard the call so it binds fs on the ESM build (which needs it) and
is a no-op elsewhere.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
tcaiger added a commit that referenced this pull request Jun 25, 2026
Temporary copy of the xlsx ESM fs-binding fix so reference-data export/import
works locally on this branch. The real fix is in its own PR (#10145); drop this
commit once that merges to main and reaches this branch.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@tcaiger tcaiger added this pull request to the merge queue Jun 26, 2026
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jun 26, 2026
@github-actions

Copy link
Copy Markdown

E2E failure report

E2E failed for run 21597.

The report artifact contains the merged Playwright HTML report with failure videos, screenshots, and traces where available.

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