fix: remove dead ternary in WORKSPACE_ROOT resolution by hobostay · Pull Request #487 · nexu-io/open-design

hobostay · 2026-05-05T00:42:55Z

Summary

Simplifies WORKSPACE_ROOT in tools/dev/src/config.ts and tools/pack/src/config.ts from a ternary with identical branches to a single "../.."
Removes unused ENTRY_DIR_NAME constant from both files

Problem

Both files contained:

ENTRY_DIR_NAME === "dist" ? "../../.." : "../../.."

The ternary condition checks whether the entry directory is dist (built) or src (source), but since src/ and dist/ are siblings at the same depth under tools/{dev,pack}/, both branches resolve to the same path — three levels up reaches the repo root from either. The conditional and the ENTRY_DIR_NAME constant were dead code that suggested a distinction where none existed.

Test plan

pnpm tools-dev start still resolves workspace root correctly
pnpm tools-pack still resolves workspace root correctly

🤖 Generated with Claude Code

Both tools/dev and tools/pack config files had: ENTRY_DIR_NAME === "dist" ? "../../.." : "../../.." with identical branches. Since `src/` and `dist/` are siblings under `tools/{dev,pack}/`, both resolve to the same path. The ternary and ENTRY_DIR_NAME constant were dead code — simplify to "../..". Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

lefarcen · 2026-05-05T00:44:51Z

Hi @hobostay! 👋

Thanks for the contribution — removing this dead ternary and unused constant is a nice cleanup.

I will run a deep review and get back to you within 24h.

Thanks for making open-design better!
— open-design team

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e256dd9f12

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-05-05T00:44:54Z

-const ENTRY_DIR_NAME = path.basename(__dirname);

-export const WORKSPACE_ROOT = path.resolve(__dirname, ENTRY_DIR_NAME === "dist" ? "../../.." : "../../..");
+export const WORKSPACE_ROOT = path.resolve(__dirname, "../..");


Resolve tools-dev workspace root at repo level

Update this constant to keep climbing three levels from config.ts; with "../..", WORKSPACE_ROOT becomes <repo>/tools (for both src and bundled dist execution), not the repository root. In resolveToolDevConfig, that immediately builds invalid paths like <repo>/tools/apps/daemon/sidecar/index.ts and <repo>/tools/apps/desktop/package.json, so tools-dev cannot locate app entrypoints in normal runs.

Useful? React with 👍 / 👎.

chatgpt-codex-connector · 2026-05-05T00:44:54Z

-const ENTRY_DIR_NAME = path.basename(__dirname);

-export const WORKSPACE_ROOT = resolve(__dirname, ENTRY_DIR_NAME === "dist" ? "../../.." : "../../..");
+export const WORKSPACE_ROOT = resolve(__dirname, "../..");


Resolve tools-pack workspace root at repo level

This should also resolve three levels up; "../.." resolves to <repo>/tools, so resolveToolPackConfig points to <repo>/tools/apps/desktop/package.json instead of <repo>/apps/desktop/package.json. That breaks electron version/dist resolution used by build commands, causing tools-pack to fail when it tries to load desktop package metadata.

Useful? React with 👍 / 👎.

mrcfps

@hobostay thanks for the focused cleanup here. I found one path-depth regression that affects both tool configs: the simplified relative path now stops at <repo>/tools instead of the workspace root, so the tool entrypoint paths are built under the wrong directory.

_{Generated by Looper 0.5.4 · runner=reviewer · agent=opencode}

mrcfps · 2026-05-05T00:46:14Z

-const ENTRY_DIR_NAME = path.basename(__dirname);

-export const WORKSPACE_ROOT = path.resolve(__dirname, ENTRY_DIR_NAME === "dist" ? "../../.." : "../../..");
+export const WORKSPACE_ROOT = path.resolve(__dirname, "../..");


This needs to keep resolving to the repository root. From tools/dev/src/config.ts (and similarly from a built tools/dev/dist/config.js), path.resolve(__dirname, "../..") lands at <repo>/tools, not <repo>. That makes the changed config later construct paths such as <repo>/tools/apps/daemon/sidecar/index.ts, <repo>/tools/apps/web/sidecar/index.ts, and <repo>/tools/apps/desktop/package.json, so normal tools-dev startup cannot find the app entrypoints. Please keep the three-level climb here, e.g. path.resolve(__dirname, "../../.."), and ideally add/adjust a config test that asserts workspaceRoot contains apps/daemon, apps/web, and apps/desktop. 🙂

mrcfps · 2026-05-05T00:46:14Z

-const ENTRY_DIR_NAME = path.basename(__dirname);

-export const WORKSPACE_ROOT = resolve(__dirname, ENTRY_DIR_NAME === "dist" ? "../../.." : "../../..");
+export const WORKSPACE_ROOT = resolve(__dirname, "../..");


Same path-depth issue here: tools/pack/src plus "../.." resolves to <repo>/tools, while resolveElectronVersion() and resolveElectronDistPath() immediately read join(workspaceRoot, "apps/desktop/package.json"). With this change that becomes <repo>/tools/apps/desktop/package.json, so packaging commands will fail before they can resolve Electron metadata. Please keep this at resolve(__dirname, "../../..") (or otherwise derive the actual repo root) and cover the expected desktop package path in the tools-pack config tests.

lefarcen

Hi @hobostay! 👋

Thanks for catching the dead ternary — you're absolutely right that both branches were identical. However, the proposed simplification has an off-by-one error that would break path resolution.

The Issue

Both files currently use "../../.." (three levels up) in the ternary, but your PR changes it to "../.." (two levels up). This breaks the path resolution:

Current (correct): From tools/dev/src/ → ../../../ reaches workspace root
After PR: From tools/dev/src/ → ../../ reaches only <repo>/tools/

This would cause every caller that joins WORKSPACE_ROOT with app paths (like apps/desktop/package.json) to look in the wrong location (tools/apps/... instead of <repo>/apps/...).

Concrete Fix

The correct simplification keeps three levels:

export const WORKSPACE_ROOT = path.resolve(__dirname, "../../..");

This is equivalent to the old ternary and resolves correctly from both src/ and dist/ subdirectories.

See inline comments for the specific lines. Thanks for the cleanup effort — with this path fix, it'll be a great improvement!

lefarcen · 2026-05-05T00:47:14Z

-const ENTRY_DIR_NAME = path.basename(__dirname);

-export const WORKSPACE_ROOT = path.resolve(__dirname, ENTRY_DIR_NAME === "dist" ? "../../.." : "../../..");
+export const WORKSPACE_ROOT = path.resolve(__dirname, "../..");


P1 Path resolution off by one level.

From tools/dev/src/, "../.." resolves to <repo>/tools, not the workspace root. The old ternary used "../../.." which correctly reaches workspace root.

Why it matters: Callers join WORKSPACE_ROOT with app paths like apps/desktop/package.json. With "../..", these paths become tools/apps/... (doesn't exist).

Concrete fix:

export const WORKSPACE_ROOT = path.resolve(__dirname, "../../..");

lefarcen · 2026-05-05T00:47:14Z

-const ENTRY_DIR_NAME = path.basename(__dirname);

-export const WORKSPACE_ROOT = resolve(__dirname, ENTRY_DIR_NAME === "dist" ? "../../.." : "../../..");
+export const WORKSPACE_ROOT = resolve(__dirname, "../..");


P1 Same path resolution issue.

From tools/pack/src/, "../.." resolves to <repo>/tools instead of workspace root. This breaks resolveToolPackConfig() (output paths), resolveElectronVersion(), and resolveElectronDistPath() which all expect workspace root.

Concrete fix:

export const WORKSPACE_ROOT = resolve(__dirname, "../../..");

lefarcen self-requested a review May 5, 2026 00:44

chatgpt-codex-connector Bot reviewed May 5, 2026

View reviewed changes

mrcfps requested changes May 5, 2026

View reviewed changes

lefarcen requested changes May 5, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: remove dead ternary in WORKSPACE_ROOT resolution#487

fix: remove dead ternary in WORKSPACE_ROOT resolution#487
hobostay wants to merge 1 commit intonexu-io:mainfrom
hobostay:fix/workspace-root-ternary

hobostay commented May 5, 2026

Uh oh!

lefarcen commented May 5, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

chatgpt-codex-connector Bot May 5, 2026

Uh oh!

chatgpt-codex-connector Bot May 5, 2026

Uh oh!

mrcfps left a comment

Uh oh!

mrcfps May 5, 2026

Uh oh!

mrcfps May 5, 2026

Uh oh!

lefarcen left a comment

Uh oh!

lefarcen May 5, 2026

Uh oh!

lefarcen May 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

hobostay commented May 5, 2026

Summary

Problem

Test plan

Uh oh!

lefarcen commented May 5, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot May 5, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot May 5, 2026

Choose a reason for hiding this comment

Uh oh!

mrcfps left a comment

Choose a reason for hiding this comment

Uh oh!

mrcfps May 5, 2026

Choose a reason for hiding this comment

Uh oh!

mrcfps May 5, 2026

Choose a reason for hiding this comment

Uh oh!

lefarcen left a comment

Choose a reason for hiding this comment

The Issue

Concrete Fix

Uh oh!

lefarcen May 5, 2026

Choose a reason for hiding this comment

Uh oh!

lefarcen May 5, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants