fix(plugin): make docker builder work in forked worktrees#1899
fix(plugin): make docker builder work in forked worktrees#1899Ajit-Mehrotra wants to merge 1 commit into
Conversation
- Purpose: make the plugin docker builder work from a forked Codex worktree without mutating protected mount targets. - Before: docker run mounted UI build outputs directly into nested paths under plugin/source and relied on the local checkout .git file. - Problem: the forked worktree was detached, plugin/.git was invalid for git lookups, and Docker failed to create nested mountpoints under the mounted source tree. - Change: move the UI asset mounts to a neutral container path, read git metadata from the repo worktree, and stage a temporary writable copy of plugin/source before packaging. - How: dc.sh now resolves git state through the parent repo and exports compose env vars, docker-compose mounts assets under /app/.mounted-assets, and build-txz syncs those assets into a temp build tree before makepkg runs.
WalkthroughThe pull request restructures the TXZ build workflow to use a temporary build sandbox with separate source and plugin directories, introduces asset synchronization from mounted directories, and updates Docker configuration to redirect volume mounts to a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9136bdaf18
ℹ️ 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".
| - ../unraid-ui/dist-wc:/app/.mounted-assets/uui | ||
| - ../web/dist:/app/.mounted-assets/standalone |
There was a problem hiding this comment.
In the build:watch workflow, nodemon only watches source/**/* (see plugin/scripts/build-watcher.sh), but this change remounts ../unraid-ui/dist-wc and ../web/dist to /app/.mounted-assets/*, which is outside the watched tree. As a result, when those frontend artifacts are rebuilt on the host, the watcher no longer triggers a plugin rebuild, so local watch runs can keep producing packages with stale UI assets until a manual rebuild/restart.
Useful? React with 👍 / 👎.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1899 +/- ##
==========================================
- Coverage 50.63% 50.62% -0.01%
==========================================
Files 1021 1021
Lines 70262 70262
Branches 7565 7561 -4
==========================================
- Hits 35575 35571 -4
- Misses 34564 34568 +4
Partials 123 123 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
plugin/builder/build-txz.ts (3)
34-38: Consider unique build directory for concurrent builds.Using a fixed path
tmpdir()/connect-plugin-buildmeans concurrent builds would interfere with each other. If concurrent builds are a possibility (e.g., multiple CI jobs on same runner), consider appending a unique suffix.♻️ Suggested change for unique build directory
-const buildRootDir = join(tmpdir(), "connect-plugin-build"); +const buildRootDir = join(tmpdir(), `connect-plugin-build-${process.pid}`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/builder/build-txz.ts` around lines 34 - 38, The prepareBuildSourceDir routine currently uses a fixed buildRootDir (tmpdir()/connect-plugin-build) which causes interference for concurrent builds; update the code that defines buildRootDir/buildSourceDir to append a unique suffix (e.g., timestamp + process.pid or crypto.randomUUID()) so each invocation gets its own temp directory, then have prepareBuildSourceDir continue to rm/mkdir/cp against that unique path (references: prepareBuildSourceDir, buildRootDir, buildSourceDir, hostSourceDir).
48-51: Silent early return may hinder debugging.When
sourceDirdoesn't exist, the function returns without any indication. If assets were expected but the mount failed, this would be difficult to diagnose.♻️ Suggested logging for visibility
if (!existsSync(sourceDir)) { + console.log(`Skipping ${label}: source directory ${sourceDir} does not exist`); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/builder/build-txz.ts` around lines 48 - 51, The function currently does a silent early return when !existsSync(sourceDir); before returning, add an explicit warning log (e.g., using the project's logger or console.warn) that includes the value of sourceDir and what was expected (e.g., "sourceDir missing, skipping asset packaging"), so callers can diagnose mount/fetch issues; update the anonymous arrow function that performs the existsSync check to log this diagnostic message prior to the return.
97-103: Add type guard for error handling.The
error.codeaccess assumes the error has acodeproperty, but in TypeScript's strict mode, caught errors are typed asunknown. Consider adding a type guard.♻️ Suggested type-safe error handling
} catch (error) { - if (error.code === 'ENOENT') { + if (error instanceof Error && 'code' in error && error.code === 'ENOENT') { console.warn(`Directory does not exist: ${dir}`); return []; } throw error; // Re-throw other errors }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/builder/build-txz.ts` around lines 97 - 103, The catch block assumes the caught value has a code property; add a type guard (e.g., an isErrnoException or inline check like typeof error === 'object' && error !== null && 'code' in error) and narrow error to NodeJS.ErrnoException before reading error.code so TypeScript strict checks pass. Update the catch in build-txz.ts to use that guard: if it confirms an ENOENT, log the directory and return []; otherwise rethrow the original error (or the narrowed error) to preserve typing and stack information.plugin/docker-compose.yml (1)
14-15: Mount path change correctly resolves nested mount issues.Moving mounts to
/app/.mounted-assets/*avoids the permission failures from mounting into nested destinations under/app/source. The paths correctly align withbuild-txz.ts(lines 15-17) wheremountedAssetsDir = join(startingDir, ".mounted-assets").Consider extracting the
.mounted-assetspath to a shared constant or documented convention to prevent future drift between this file andbuild-txz.ts.,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/docker-compose.yml` around lines 14 - 15, The docker-compose mount paths were changed to /app/.mounted-assets/* to match build-txz.ts's mountedAssetsDir; to prevent future drift, extract the ".mounted-assets" string into a single shared constant or clearly documented convention used by both docker-compose and build-txz.ts: introduce a named constant (e.g., MOUNTED_ASSETS_DIR or mountedAssetsDirExport) in build-txz.ts and reference/export it for consumers, or add a short comment/docblock in both docker-compose.yml and build-txz.ts that points to the single source of truth (mountedAssetsDir) so both remain aligned.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@plugin/builder/build-txz.ts`:
- Around line 34-38: The prepareBuildSourceDir routine currently uses a fixed
buildRootDir (tmpdir()/connect-plugin-build) which causes interference for
concurrent builds; update the code that defines buildRootDir/buildSourceDir to
append a unique suffix (e.g., timestamp + process.pid or crypto.randomUUID()) so
each invocation gets its own temp directory, then have prepareBuildSourceDir
continue to rm/mkdir/cp against that unique path (references:
prepareBuildSourceDir, buildRootDir, buildSourceDir, hostSourceDir).
- Around line 48-51: The function currently does a silent early return when
!existsSync(sourceDir); before returning, add an explicit warning log (e.g.,
using the project's logger or console.warn) that includes the value of sourceDir
and what was expected (e.g., "sourceDir missing, skipping asset packaging"), so
callers can diagnose mount/fetch issues; update the anonymous arrow function
that performs the existsSync check to log this diagnostic message prior to the
return.
- Around line 97-103: The catch block assumes the caught value has a code
property; add a type guard (e.g., an isErrnoException or inline check like
typeof error === 'object' && error !== null && 'code' in error) and narrow error
to NodeJS.ErrnoException before reading error.code so TypeScript strict checks
pass. Update the catch in build-txz.ts to use that guard: if it confirms an
ENOENT, log the directory and return []; otherwise rethrow the original error
(or the narrowed error) to preserve typing and stack information.
In `@plugin/docker-compose.yml`:
- Around line 14-15: The docker-compose mount paths were changed to
/app/.mounted-assets/* to match build-txz.ts's mountedAssetsDir; to prevent
future drift, extract the ".mounted-assets" string into a single shared constant
or clearly documented convention used by both docker-compose and build-txz.ts:
introduce a named constant (e.g., MOUNTED_ASSETS_DIR or mountedAssetsDirExport)
in build-txz.ts and reference/export it for consumers, or add a short
comment/docblock in both docker-compose.yml and build-txz.ts that points to the
single source of truth (mountedAssetsDir) so both remain aligned.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 30c5c305-43d7-416d-8e57-b7a09c78c3b9
📒 Files selected for processing (3)
plugin/builder/build-txz.tsplugin/docker-compose.ymlplugin/scripts/dc.sh
|
closed - not a problem. local issue |
1 participant
Summary
plugin/sourceplugin/sourcebefore packaging the txz.gitfileProblem
pnpm run docker:build-and-runfailed after forking this work into a new Codex worktree.The forked worktree was detached at the same commit as
main, and the host-sideplugin/scripts/dc.shlogic was still reading git state from the local checkout. In that environment the localplugin/.gitfile was not usable, so the script was already operating on shaky ground before Docker started.After the dependency build completed, Docker then failed to start the builder container because it tried to bind mount
../unraid-ui/dist-wcand../web/distdirectly into nested destinations under/app/source/.../unraid-components/*. Docker could not create those nested mountpoints inside the mounted source tree, which led to permission and mountpoint creation failures.That meant the plugin build flow broke precisely in the case where we wanted to split unrelated work into a separate worktree and PR.
Why this is needed
We use worktrees to separate unrelated changes into isolated branches and pull requests. The plugin builder should keep working in that setup.
Without this change, forking plugin work into a separate Codex worktree can leave
docker:build-and-rununusable, forcing manual cleanup or avoiding worktrees entirely.Fix
plugin/scripts/dc.shto resolve git metadata through the parent repo worktree and export the compose environment before launchplugin/docker-compose.ymlto mount the built UI artifacts under/app/.mounted-assets/*instead of nested destinations inside/app/sourceplugin/builder/build-txz.tsto create a writable temp copy ofplugin/source, sync the mounted assets into that temp tree, and package from that staged treeVerification
SKIP_HOST_BUILD=true ./scripts/dc.sh bash -lc 'echo container-ok'SKIP_HOST_BUILD=true ./scripts/dc.sh bash -lc 'pnpm run build:txz'Summary by CodeRabbit