Skip to content

fix(ppt): dep injection#3732

Merged
icecrasher321 merged 1 commit intostagingfrom
fix/pptjsx-image
Mar 24, 2026
Merged

fix(ppt): dep injection#3732
icecrasher321 merged 1 commit intostagingfrom
fix/pptjsx-image

Conversation

@icecrasher321
Copy link
Collaborator

Summary

Correctly inject deps for pptx worker in docker image.

Type of Change

  • Bug fix

Testing

Will test in staging env

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

@vercel
Copy link

vercel bot commented Mar 24, 2026

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

Project Deployment Actions Updated (UTC)
docs Ready Ready Preview, Comment Mar 24, 2026 4:18am

Request Review

@cursor
Copy link

cursor bot commented Mar 24, 2026

PR Summary

Medium Risk
Moderate risk because it changes build and Docker packaging/tracing for the PPTX subprocess worker; misconfiguration could break PPTX generation in production deployments.

Overview
Fixes PPTX worker dependency injection by bundling the pptx-worker.cjs into apps/sim/dist and treating that artifact as the runtime source of truth.

Build/runtime wiring is updated accordingly: package.json now builds the worker before next build, next.config.ts traces the bundled worker file, pptx-vm.ts searches additional dist locations when spawning the worker, and the Docker image copies the bundled worker instead of the source worker (and stops copying pptxgenjs explicitly).

Written by Cursor Bugbot for commit e7f5621. Configure here.

@icecrasher321 icecrasher321 merged commit 8eb45e3 into staging Mar 24, 2026
12 checks passed
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 24, 2026

Greptile Summary

This PR fixes the PPTX worker dependency injection in the Docker image by switching from copying node_modules/pptxgenjs as a raw dependency to bundling it into a self-contained dist/pptx-worker.cjs artifact using bun build.

Key changes:

  • A new build:pptx-worker script runs before next build and uses bun build to inline pptxgenjs (and all its transitive deps) into a single CJS file at dist/pptx-worker.cjs.
  • next.config.ts outputFileTracingIncludes is updated to reference the bundled artifact instead of the full node_modules/pptxgenjs/**/* tree.
  • The Dockerfile drops the explicit node_modules/pptxgenjs copy and instead copies only the bundled worker from the builder stage.
  • pptx-vm.ts expands its worker path discovery to check dist/ locations first, with broad fallbacks covering Docker, monorepo-root, and local dev environments.
  • One fallback path candidate (path.join(process.cwd(), 'dist', 'pptx-worker.cjs')) is potentially redundant with other candidates but is harmless.

Confidence Score: 4/5

  • This PR is safe to merge pending a staging validation of full PPTX generation (especially image insertion and custom font paths that may exercise optional pptxgenjs modules).
  • The approach is architecturally sound: bundling pptxgenjs into the worker eliminates the need to carry the raw node_modules directory into the Docker runner image. The build pipeline order (bundle first, then Next.js build) is correct, the Dockerfile changes are consistent, and the fallback path resolution is thorough. The only risk is dynamic requires inside pptxgenjs that bun cannot statically capture, which staging testing will surface. One harmless redundant fallback path candidate is the only style concern.
  • No files require special attention; verify staging PPTX generation end-to-end to confirm no dynamic require misses in the bundled worker.

Important Files Changed

Filename Overview
apps/sim/package.json Adds build:pptx-worker script that bundles pptx-worker.cjs (with pptxgenjs inlined) to dist/ before the Next.js build; risk is dynamic requires not captured by bun's static bundler.
docker/app.Dockerfile Removes explicit pptxgenjs node_modules copy and replaces it with the self-contained bundled worker artifact from the builder stage; Dockerfile changes are correct and consistent with the new build approach.
apps/sim/next.config.ts Updates outputFileTracingIncludes to reference the bundled ./dist/pptx-worker.cjs instead of the raw pptxgenjs node_modules tree; generated before next build runs so the path will exist at trace time.
apps/sim/lib/execution/pptx-vm.ts Expands the worker path discovery candidates to prioritize the new dist/ location; fallback chain is thorough and covers Docker, monorepo-root, and local dev scenarios, though one candidate is redundant.

Sequence Diagram

sequenceDiagram
    participant Dockerfile as Docker Build
    participant BunBuild as bun build
    participant NextBuild as next build
    participant Runner as Docker Runner
    participant PptxVM as pptx-vm.ts
    participant Worker as dist/pptx-worker.cjs

    Dockerfile->>BunBuild: bun run build:pptx-worker
    Note over BunBuild: Bundles pptx-worker.cjs + pptxgenjs into dist/
    BunBuild-->>Dockerfile: dist/pptx-worker.cjs created

    Dockerfile->>NextBuild: next build
    Note over NextBuild: outputFileTracingIncludes picks up dist/pptx-worker.cjs
    NextBuild-->>Dockerfile: .next/standalone output

    Dockerfile->>Runner: COPY dist/pptx-worker.cjs to apps/sim/dist/
    Note over Runner: No node_modules/pptxgenjs needed

    Runner->>PptxVM: spawn node worker
    PptxVM->>Worker: resolves via process.cwd()/apps/sim/dist/pptx-worker.cjs
    Worker-->>PptxVM: IPC ready
    PptxVM->>Worker: IPC generate code
    Worker-->>PptxVM: IPC result base64
Loading

Comments Outside Diff (1)

  1. apps/sim/lib/execution/pptx-vm.ts, line 33-47 (link)

    P2 Unreachable fallback path candidate

    Candidate 5 — path.join(process.cwd(), 'dist', 'pptx-worker.cjs') — is intended for the case where process.cwd() is the apps/sim/ directory (local dev after build:pptx-worker). However, candidate 3 (process.cwd() + 'apps/sim/dist/pptx-worker.cjs') covers the Docker/monorepo-root case. In the only other scenario where process.cwd() resolves to apps/sim/ itself, candidate 1 (currentDir + '../../dist/pptx-worker.cjs') and candidate 5 would refer to the same file.

    This is not a bug since the list uses .find() and stops at the first match, but the redundancy may cause confusion. Consider removing candidate 5 or adding a comment that documents which environment each candidate targets.

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Reviews (1): Last reviewed commit: "fix(ppt): dep injection" | Re-trigger Greptile

"dev:full": "bunx concurrently -n \"App,Realtime\" -c \"cyan,magenta\" \"bun run dev\" \"bun run dev:sockets\"",
"build": "next build",
"build": "bun run build:pptx-worker && next build",
"build:pptx-worker": "bun build ./lib/execution/pptx-worker.cjs --target=node --format=cjs --outfile ./dist/pptx-worker.cjs",
Copy link
Contributor

Choose a reason for hiding this comment

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

P2 Bundling a pre-compiled .cjs as entry point

The build:pptx-worker script takes ./lib/execution/pptx-worker.cjs as its entry point. This works, but if pptxgenjs or any of its transitive dependencies use dynamic require() with non-literal arguments (e.g. require(someVar)), bun's static bundler won't capture those modules and the worker will throw MODULE_NOT_FOUND at runtime for those paths.

pptxgenjs itself is a pure-JS library and is unlikely to have this issue, but it's worth verifying in the staging test that all PPTX generation features (including image insertion, custom fonts, etc.) work end-to-end, as those code paths in pptxgenjs may pull in optional dependencies at runtime.

@waleedlatif1 waleedlatif1 deleted the fix/pptjsx-image branch March 24, 2026 06:12
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