Reviewed GitHub PR for photon module fix

Summary

In Progress

Understand
Read PR description, comments, commits, and changed files.
Understand
Understand
Checked for linked issues and analyzed them.
Understand
Understand
Verified function usage and callers.
Understand
Verify
Validated changelog entry and attribution.
Verify

You are given one or more GitHub PR URLs: https://github.com/badlogic/pi-mono/pull/795/changes

For each PR URL, do the following in order:

Read the PR page in full. Include description, all comments, all commits, and all changed files.
Identify any linked issues referenced in the PR body, comments, commit messages, or cross links. Read each issue in full, including all comments.
Analyze the PR diff. Read all relevant code files in full with no truncation from the current main branch and compare against the diff. Do not fetch PR file blobs unless a file is missing on main or the diff context is insufficient. Include related code paths that are not in the diff but are required to validate behavior.
Check for a changelog entry in the relevant packages/*/CHANGELOG.md files. Report whether an entry exists. If missing, state that a changelog entry is required before merge and that you will add it if the user decides to merge. Follow the changelog format rules in AGENTS.md. Verify:
- Entry uses correct section (### Breaking Changes, ### Added, ### Fixed, etc.)
- External contributions include PR link and author: Fixed foo ([#123](https://github.com/badlogic/pi-mono/pull/123) by [@user](https://github.com/user))
- Breaking changes are in ### Breaking Changes, not just ### Fixed
Check if packages/coding-agent/README.md, packages/coding-agent/docs/.md, packages/coding-agent/examples/**/.md require modification. This is usually the case when existing features have been changed, or new features have been added.
Provide a structured review with these sections:
- Good: solid choices or improvements
- Bad: concrete issues, regressions, missing tests, or risks
- Ugly: subtle or high impact problems
Add Questions or Assumptions if anything is unclear.
Add Change summary and Tests.

Output format per PR: PR: Changelog:

... Good:
... Bad:
... Ugly:
... Questions or Assumptions:
... Change summary:
... Tests:
...

If no issues are found, say so under Bad and Ugly.

>gh pr view 795 --repo badlogic/pi-mono --json title,body,comments,commits,files,author,labels,state,reviews

{"author":{"id":"MDQ6VXNlcjIyMDY1ODM=","is_bot":false,"login":"dannote","name":"Danila Poyarkov"},"body":"**Problem**\n\nPhoton module fails to load with \"require is not defined\" error when running in ESM context.\n\n**Solution**\n\nReplace synchronous `require()` with async `import()` in photon.ts.\n\n**Changes**\n\n- `packages/coding-agent/src/utils/photon.ts`: Replace `getPhoton()` with async `loadPhoton()`\n- `packages/coding-agent/src/utils/image-convert.ts`: Use `await loadPhoton()`\n- `packages/coding-agent/src/utils/image-resize.ts`: Use `await loadPhoton()`","comments":[],"commits":[{"authoredDate":"2026-01-17T00:06:27Z","authors":[{"email":"[REDACTED]","id":"MDQ6VXNlcjIyMDY1ODM=","login":"dannote","name":"Danila Poyarkov"}],"committedDate":"2026-01-17T00:06:27Z","messageBody":"","messageHeadline":"fix(coding-agent): use dynamic import for photon module in ESM context","oid":"e4e26a27c5c810c44413e2b0b03abb83a96eeed3"}],"files":[{"path":"packages/coding-agent/CHANGELOG.md","additions":1,"deletions":0},{"path":"packages/coding-agent/src/utils/image-convert.ts","additions":10,"deletions":12},{"path":"packages/coding-agent/src/utils/image-resize.ts","additions":2,"deletions":2},{"path":"packages/coding-agent/src/utils/photon.ts","additions":18,"deletions":32}],"labels":[],"reviews":[],"state":"OPEN","title":"fix(coding-agent): use dynamic import for photon module in ESM context"}

>gh pr diff 795 --repo badlogic/pi-mono

diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md
index a40543c35..961be9e57 100644
--- a/packages/coding-agent/CHANGELOG.md
+++ b/packages/coding-agent/CHANGELOG.md
@@ -15,6 +15,7 @@
 
 ### Fixed
 
+- Fixed photon module failing to load in ESM context with "require is not defined" error
 - Fixed crash during auto-compaction when summarization fails (e.g., quota exceeded). Now displays error message instead of crashing ([#792](https://github.com/badlogic/pi-mono/issues/792))
 - Fixed `--no-extensions` flag not preventing extension discovery ([#776](https://github.com/badlogic/pi-mono/issues/776))
 - Fixed extension messages rendering twice on startup when `pi.sendMessage({ display: true })` is called during `session_start` ([#765](https://github.com/badlogic/pi-mono/pull/765) by [@dannote](https://github.com/dannote))
diff --git a/packages/coding-agent/src/utils/image-convert.ts b/packages/coding-agent/src/utils/image-convert.ts
index a1e5a4745..9f1e8ec18 100644
--- a/packages/coding-agent/src/utils/image-convert.ts
+++ b/packages/coding-agent/src/utils/image-convert.ts
@@ -1,26 +1,23 @@
-import { getPhoton } from "./photon.js";
+import { loadPhoton } from "./photon.js";
 
 /**
  * Convert image to PNG format for terminal display.
  * Kitty graphics protocol requires PNG format (f=100).
  */
-export async function convertToPng(
-	base64Data: string,
-	mimeType: string,
-): Promise<{ data: string; mimeType: string } | null> {
+export async function convertToPng(base64Data: string, mimeType: string): Promise<{ data: string; mimeType: string }> {
 	// Already PNG, no conversion needed
 	if (mimeType === "image/png") {
 		return { data: base64Data, mimeType };
 	}
 
-	const photon = getPhoton();
+	const photon = await loadPhoton();
 	if (!photon) {
-		// Photon not available, can't convert
-		return null;
+		throw new Error("Photon module not available");
 	}
 
 	try {
-		const image = photon.PhotonImage.new_from_byteslice(new Uint8Array(Buffer.from(base64Data, "base64")));
+		const bytes = new Uint8Array(Buffer.from(base64Data, "base64"));
+		const image = photon.PhotonImage.new_from_byteslice(bytes);
 		try {
 			const pngBuffer = image.get_bytes();
 			return {
@@ -30,8 +27,9 @@ export async function convertToPng(
 		} finally {
 			image.free();
 		}
-	} catch {
-		// Conversion failed
-		return null;
+	} catch (e) {
+		// Conversion failed - return error details
+		const msg = e instanceof Error ? e.message : String(e);
+		throw new Error(`Photon conversion failed: ${msg}`);
 	}
 }
diff --git a/packages/coding-agent/src/utils/image-resize.ts b/packages/coding-agent/src/utils/image-resize.ts
index dbaf0b4c5..91c3f7968 100644
--- a/packages/coding-agent/src/utils/image-resize.ts
+++ b/packages/coding-agent/src/utils/image-resize.ts
@@ -1,5 +1,5 @@
 import type { ImageContent } from "@mariozechner/pi-ai";
-import { getPhoton } from "./photon.js";
+import { loadPhoton } from "./photon.js";
 
 export interface ImageResizeOptions {
 	maxWidth?: number; // Default: 2000
@@ -53,7 +53,7 @@ export async function resizeImage(img: ImageContent, options?: ImageResizeOption
 	const opts = { ...DEFAULT_OPTIONS, ...options };
 	const inputBuffer = Buffer.from(img.data, "base64");
 
-	const photon = getPhoton();
+	const photon = await loadPhoton();
 	if (!photon) {
 		// Photon not available, return original image
 		return {
diff --git a/packages/coding-agent/src/utils/photon.ts b/packages/coding-agent/src/utils/photon.ts
index 0666460bc..d7a2f07fc 100644
--- a/packages/coding-agent/src/utils/photon.ts
+++ b/packages/coding-agent/src/utils/photon.ts
@@ -8,8 +8,8 @@
  * The challenge: photon-node's CJS entry uses fs.readFileSync(__dirname + '/photon_rs_bg.wasm')
  * which bakes the build machine's absolute path into Bun compiled binaries.
  *
- * Solution: Lazy-load photon and gracefully handle failures. Image processing functions
- * already have fallbacks that return original images when photon isn't available.
+ * Solution: Lazy-load photon via dynamic import and gracefully handle failures.
+ * Image processing functions have fallbacks that return original images when photon isn't available.
  */
 
 // Re-export types from the main package
@@ -17,43 +17,29 @@ export type { PhotonImage as PhotonImageType } from "@silvia-odwyer/photon-node"
 
 // Lazy-loaded photon module
 let photonModule: typeof import("@silvia-odwyer/photon-node") | null = null;
-let loadAttempted = false;
-let loadError: Error | null = null;
+let loadPromise: Promise<typeof import("@silvia-odwyer/photon-node") | null> | null = null;
 
 /**
- * Get the photon module, loading it lazily on first access.
- * Returns null if loading fails (e.g., in broken Bun binary).
+ * Load the photon module asynchronously.
+ * Returns cached module on subsequent calls.
  */
-export function getPhoton(): typeof import("@silvia-odwyer/photon-node") | null {
-	if (loadAttempted) {
+export async function loadPhoton(): Promise<typeof import("@silvia-odwyer/photon-node") | null> {
+	if (photonModule) {
 		return photonModule;
 	}
 
-	loadAttempted = true;
-
-	try {
-		// Dynamic require to defer loading until actually needed
-		// This also allows the error to be caught gracefully
-		photonModule = require("@silvia-odwyer/photon-node");
-	} catch (e) {
-		loadError = e as Error;
-		photonModule = null;
+	if (loadPromise) {
+		return loadPromise;
 	}
 
-	return photonModule;
-}
-
-/**
- * Check if photon is available and working.
- */
-export function isPhotonAvailable(): boolean {
-	return getPhoton() !== null;
-}
+	loadPromise = (async () => {
+		try {
+			photonModule = await import("@silvia-odwyer/photon-node");
+		} catch {
+			photonModule = null;
+		}
+		return photonModule;
+	})();
 
-/**
- * Get the error that occurred during photon loading, if any.
- */
-export function getPhotonLoadError(): Error | null {
-	getPhoton(); // Ensure load was attempted
-	return loadError;
+	return loadPromise;
 }

Let me read the current source files on main to compare:

Let me check for any usages of the removed functions and verify the callers of convertToPng:

>grep -r "isPhotonAvailable\|getPhotonLoadError\|getPhoton\|loadPhoton" packages/coding-agent/src --include="*.ts" | head -50

packages/coding-agent/src/utils/photon.ts:export function getPhoton(): typeof import("@silvia-odwyer/photon-node") | null {
packages/coding-agent/src/utils/photon.ts:export function isPhotonAvailable(): boolean {
packages/coding-agent/src/utils/photon.ts:	return getPhoton() !== null;
packages/coding-agent/src/utils/photon.ts:export function getPhotonLoadError(): Error | null {
packages/coding-agent/src/utils/photon.ts:	getPhoton(); // Ensure load was attempted
packages/coding-agent/src/utils/image-convert.ts:import { getPhoton } from "./photon.js";
packages/coding-agent/src/utils/image-convert.ts:	const photon = getPhoton();
packages/coding-agent/src/utils/image-resize.ts:import { getPhoton } from "./photon.js";
packages/coding-agent/src/utils/image-resize.ts:	const photon = getPhoton();

Let me check how convertToPng is called in the codebase:

>grep -B5 -A10 "convertToPng" packages/coding-agent/src/modes/interactive/components/tool-execution.ts

import stripAnsi from "strip-ansi";
import type { ToolDefinition } from "../../../core/extensions/types.js";
import { computeEditDiff, type EditDiffError, type EditDiffResult } from "../../../core/tools/edit-diff.js";
import { allTools } from "../../../core/tools/index.js";
import { DEFAULT_MAX_BYTES, DEFAULT_MAX_LINES, formatSize } from "../../../core/tools/truncate.js";
import { convertToPng } from "../../../utils/image-convert.js";
import { sanitizeBinaryOutput } from "../../../utils/shell.js";
import { getLanguageFromPath, highlightCode, theme } from "../theme/theme.js";
import { renderDiff } from "./diff.js";
import { keyHint } from "./keybinding-hints.js";
import { truncateToVisualLines } from "./visual-truncate.js";

// Preview line limit for bash when not expanded
const BASH_PREVIEW_LINES = 5;

/**
--
			if (img.mimeType === "image/png") continue;
			if (this.convertedImages.has(i)) continue;

			// Convert async
			const index = i;
			convertToPng(img.data, img.mimeType).then((converted) => {
				if (converted) {
					this.convertedImages.set(index, converted);
					this.updateDisplay();
					this.ui.requestRender();
				}
			});
		}
	}

	setExpanded(expanded: boolean): void {

>grep -B10 -A20 "convertToPng" packages/coding-agent/src/modes/interactive/components/tool-execution.ts | tail -50

import { renderDiff } from "./diff.js";
import { keyHint } from "./keybinding-hints.js";
import { truncateToVisualLines } from "./visual-truncate.js";

// Preview line limit for bash when not expanded
const BASH_PREVIEW_LINES = 5;

/**
 * Convert absolute path to tilde notation if it's in home directory
 */
function shortenPath(path: string): string {
	const home = os.homedir();
	if (path.startsWith(home)) {
		return `~${path.slice(home.length)}`;
	}
	return path;
}

--

		for (let i = 0; i < imageBlocks.length; i++) {
			const img = imageBlocks[i];
			if (!img.data || !img.mimeType) continue;
			// Skip if already PNG or already converted
			if (img.mimeType === "image/png") continue;
			if (this.convertedImages.has(i)) continue;

			// Convert async
			const index = i;
			convertToPng(img.data, img.mimeType).then((converted) => {
				if (converted) {
					this.convertedImages.set(index, converted);
					this.updateDisplay();
					this.ui.requestRender();
				}
			});
		}
	}

	setExpanded(expanded: boolean): void {
		this.expanded = expanded;
		this.updateDisplay();
	}

	setShowImages(show: boolean): void {
		this.showImages = show;
		this.updateDisplay();
	}

	override invalidate(): void {

This is a breaking change! The PR changes:

convertToPng signature from returning Promise<{ data, mimeType } | null> to Promise<{ data, mimeType }> (never null, throws instead)
But the caller in tool-execution.ts calls it with .then((converted) => { if (converted) { ... } }) and doesn't catch errors.

If photon is unavailable or conversion fails, the current code just ignores it (returns null). The PR changes this to throw an error, which would cause an unhandled promise rejection in the calling code since there's no .catch() handler.

This is a breaking change that would cause uncaught promise rejection in the tool-execution.ts component!

Let me also check if there are any linked issues referenced:

Let me check if there are any linked issues and also verify that the PR modifies the caller:

Now let me do a final check on the PR. The key issue I've identified is that the PR changes convertToPng to throw on errors, but the caller in tool-execution.ts doesn't handle errors. However, looking at the diff again, the PR doesn't modify tool-execution.ts at all!

This means the caller still does:

But convertToPng now throws instead of returning null, causing an unhandled promise rejection.

Let me also note:

The PR removes two exported functions: isPhotonAvailable() and getPhotonLoadError() - I should check if these are used anywhere else
The return type of convertToPng changed from Promise<{ data, mimeType } | null> to Promise<{ data, mimeType }> (no longer nullable)

Let me verify nothing else uses these removed functions:

Good, those functions are internal only, not exported from the package's public API.

Now let me summarize my findings:

Good:

The core issue (ESM context failing with require is not defined) is correctly addressed by switching from require() to import()
The caching mechanism using loadPromise is correct to prevent multiple concurrent loads
Changelog entry exists and is properly formatted for an external contribution

Bad:

Critical bug: convertToPng return type changed from Promise<{ data, mimeType } | null> to Promise<{ data, mimeType }> (throws on error). The caller in tool-execution.ts calls .then((converted) => { if (converted) { ... } }) without a .catch(), causing unhandled promise rejection when photon fails.
The PR doesn't update tool-execution.ts to handle the new throwing behavior

Ugly:

The behavioral change from "return null on failure" to "throw on failure" is an undocumented breaking change for the internal API

Let me verify my understanding of the return type change in the diff:

Looking at the diff for image-convert.ts:

Before: Promise<{ data: string; mimeType: string } | null>
After: Promise<{ data: string; mimeType: string }> (no null)

Before, it returned null when:

photon not available
conversion failed

After, it throws an error in both cases.

This is definitely a bug in the PR since it breaks the caller.

Let me check the CHANGELOG entry format as well:

Let me verify the changelog entry format and check for attribution:

>gh pr diff 795 --repo badlogic/pi-mono | grep "CHANGELOG" -A5

diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md
index a40543c35..961be9e57 100644
--- a/packages/coding-agent/CHANGELOG.md
+++ b/packages/coding-agent/CHANGELOG.md
@@ -15,6 +15,7 @@
 
 ### Fixed
 
+- Fixed photon module failing to load in ESM context with "require is not defined" error

The changelog entry is missing the PR attribution! Per AGENTS.md:

External contributions: Fixed foo ([#456](https://github.com/badlogic/pi-mono/pull/456) by [@username](https://github.com/username))

It should be: - Fixed photon module failing to load in ESM context with "require is not defined" error ([#795](https://github.com/badlogic/pi-mono/pull/795) by [@dannote](https://github.com/dannote))

Now I have a complete picture of this PR:

Problem it solves: Photon module fails with "require is not defined" in ESM context
Solution: Replace synchronous require() with async import()
Issues:
- Critical: The return type change on convertToPng breaks the caller in tool-execution.ts (unhandled promise rejection)
- Changelog missing PR attribution for external contribution
- Two helper functions removed (isPhotonAvailable, getPhotonLoadError) - though they're not used externally

Let me provide my full review.

PR: https://github.com/badlogic/pi-mono/pull/795

Changelog:

Entry exists in correct section (### Fixed) but missing PR attribution for external contribution
Should be: Fixed photon module failing to load in ESM context with "require is not defined" error ([#795](https://github.com/badlogic/pi-mono/pull/795) by [@dannote](https://github.com/dannote))

Good:

Core fix is correct: replacing synchronous require() with async import() solves the ESM context issue
Promise-based caching via loadPromise is a clean pattern that prevents race conditions on concurrent loads
Clean simplification of the photon module API

Bad:

Critical bug: convertToPng return type changed from Promise<{ data, mimeType } | null> to Promise<{ data, mimeType }> (throws instead of returning null). The caller in tool-execution.ts (line ~290) uses .then((converted) => { if (converted) { ... } }) without a .catch() handler, causing unhandled promise rejection when photon is unavailable or conversion fails.
The PR doesn't update tool-execution.ts to handle the new throwing behavior
Two helper functions removed (isPhotonAvailable(), getPhotonLoadError()) without deprecation, though they're internal-only and unused

Ugly:

The behavioral change from "graceful null return" to "throw on failure" is subtle but breaks the existing call site. The original design was intentionally fault-tolerant (images just wouldn't convert if photon failed), but now it causes errors.

Questions or Assumptions:

Was the behavior change in convertToPng (null → throw) intentional? If so, tool-execution.ts needs a corresponding update with .catch() handler.
Has this been tested in the actual ESM context where the error originally occurred?

Change summary:

photon.ts: Replace sync getPhoton() with async loadPhoton(), remove isPhotonAvailable() and getPhotonLoadError()
image-convert.ts: Use await loadPhoton(), change from returning null to throwing on errors
image-resize.ts: Use await loadPhoton(), preserves null-return behavior (returns original image on photon unavailable)
CHANGELOG entry added

Tests:

No tests in this PR
Manual testing needed: run pi in ESM context (e.g., when imported as a module) and verify photon loads without "require is not defined" error
Manual testing needed: verify image conversion/resize works when photon is available
Manual testing needed: verify graceful degradation when photon is unavailable (currently would cause unhandled rejection in tool-execution.ts)