Conversation
WalkthroughCentralizes frontend Docker builds into a shared Dockerfile.node with APP_SCOPE, relocates API Dockerfile to repo root, and adds an AIO assembler Dockerfile.aio with docker-bake targets. Updates CI to matrix-based smoke tests and AIO smoke testing. Removes per-app Dockerfiles, revises compose files to API-centric local dev, adds smoke scripts, and pins tools via mise. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer/CI
participant GH as GitHub Actions
participant Bake as Buildx Bake
participant NodeImg as Dockerfile.node (APP_SCOPE)
participant ApiImg as Dockerfile.api
participant LiveImg as apps/live/Dockerfile.live
participant ProxyImg as apps/proxy/Dockerfile.ce
participant AIO as Dockerfile.aio
rect rgb(245,248,255)
Note over GH: Matrix smoke (per app)
Dev->>GH: Push/PR
GH->>GH: paths-filter → determine matrix
loop For each app in matrix
GH->>NodeImg: Build (target=runtime, APP_SCOPE)
alt live app
GH->>LiveImg: Build live
end
GH->>GH: Run container
GH->>GH: Probe HTTP path until 200
end
end
rect rgb(245,255,245)
Note over GH: AIO smoke (conditional)
GH->>GH: paths-filter → AIO needed?
alt needed
GH->>Bake: bake targets (web, space, admin, live, api, proxy)
Bake->>NodeImg: Build web/space/admin (runtime)
Bake->>LiveImg: Build live
Bake->>ApiImg: Build api (runtime)
Bake->>ProxyImg: Build proxy
Bake->>AIO: Build with contexts (web_ctx...proxy_ctx)
GH->>AIO: Run container (port 80)
GH->>AIO: Probe /, /spaces, /god-mode, /live/health
end
end
sequenceDiagram
autonumber
participant WF as build-branch.yml
participant Setup as branch_build_setup
participant Admin as branch_build_push_admin
participant Web as branch_build_push_web
participant Space as branch_build_push_space
participant API as branch_build_push_api
participant Live as branch_build_push_live
participant Proxy as branch_build_push_proxy
participant AIOFlow as branch_build_push_aio
WF->>Setup: Initialize Buildx (outputs endpoint)
Setup-->>Admin: gh_buildx_endpoint
Setup-->>Web: gh_buildx_endpoint
Setup-->>Space: gh_buildx_endpoint
Setup-->>API: gh_buildx_endpoint
Setup-->>Live: gh_buildx_endpoint
Setup-->>Proxy: gh_buildx_endpoint
Admin->>Admin: Build Dockerfile.node (APP_SCOPE=admin)
Web->>Web: Build Dockerfile.node (APP_SCOPE=web)
Space->>Space: Build Dockerfile.node (APP_SCOPE=space)
API->>API: Build Dockerfile.api (context=.)
Live->>Live: Build live Dockerfile
Proxy->>Proxy: Build proxy Dockerfile
par AIO path (needs multiple)
AIOFlow->>AIOFlow: Aggregate images/assets
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (34)
apps/live/package.json (1)
29-31: Move client-only packages into devDependenciesI searched all TypeScript and JavaScript files under
apps/live/srcfor both static (from '…') and CommonJS (require('…')) imports of@plane/hooks,@plane/ui, and@plane/utilsand found no occurrences. This confirms they aren’t needed at runtime and can safely live indevDependenciesto slim the server image and avoid pulling in React/DOM code.Proposed change in
apps/live/package.json:--- apps/live/package.json @@ - "dependencies": { + "dependencies": { // other runtime deps… - "@plane/hooks": "workspace:*", - "@plane/ui": "workspace:*", - "@plane/utils": "workspace:*" }, + "devDependencies": { + "@plane/hooks": "workspace:*", + "@plane/ui": "workspace:*", + "@plane/utils": "workspace:*" + }apps/api/plane/settings/local.py (2)
9-18: Broaden env toggle semantics and narrow the exception type
- Accept common truthy values for
DJANGO_DEBUG_TOOLBARinstead of only"1".- Catch
ImportErrorspecifically to avoid masking unrelated errors.Apply:
-# Debug Toolbar settings (optional via DJANGO_DEBUG_TOOLBAR=1) +# Debug Toolbar settings (optional via DJANGO_DEBUG_TOOLBAR=1/true/on) DEBUG_TOOLBAR_PATCH_SETTINGS = False -if os.environ.get("DJANGO_DEBUG_TOOLBAR", "0") == "1": - try: - import debug_toolbar # noqa: F401 - INSTALLED_APPS += ("debug_toolbar",) # noqa - MIDDLEWARE += ("debug_toolbar.middleware.DebugToolbarMiddleware",) # noqa - except Exception: - # If debug_toolbar isn't installed, skip enabling it - pass +_dt_flag = os.environ.get("DJANGO_DEBUG_TOOLBAR", "0").lower() +if _dt_flag in {"1", "true", "yes", "on"}: + try: + import debug_toolbar # noqa: F401 + INSTALLED_APPS += ("debug_toolbar",) # noqa + MIDDLEWARE += ("debug_toolbar.middleware.DebugToolbarMiddleware",) # noqa + except ImportError: + # If debug_toolbar isn't installed, skip enabling it + pass
9-18: Ensure Debug Toolbar Visibility in Containerized EnvironmentsIn
apps/api/plane/settings/local.py, you currently have:DEBUG = True # … DEBUG_TOOLBAR_PATCH_SETTINGS = False if os.environ.get("DJANGO_DEBUG_TOOLBAR", "0") == "1": try: import debug_toolbar # noqa: F401 INSTALLED_APPS += ("debug_toolbar",) # noqa MIDDLEWARE += ("debug_toolbar.middleware.DebugToolbarMiddleware",) # noqa except Exception: # If debug_toolbar isn't installed, skip enabling it pass # … INTERNAL_IPS = ("127.0.0.1",)Behind Docker or other proxy networks, requests rarely originate from
127.0.0.1, so the toolbar will not render even when enabled. To force it on wheneverDJANGO_DEBUG_TOOLBAR=1is set, consider appending one of the following after adding the middleware:• Always show the toolbar (ignores
INTERNAL_IPS):INSTALLED_APPS += ("debug_toolbar",) MIDDLEWARE += ("debug_toolbar.middleware.DebugToolbarMiddleware",) + # Force toolbar visibility in containerized/local proxied environments + DEBUG_TOOLBAR_CONFIG = { + "SHOW_TOOLBAR_CALLBACK": lambda _request: True # noqa: E731 + }• Alternatively, extend
INTERNAL_IPSto include your Docker network’s gateway (e.g.,172.17.0.1/16) or read from an env var:-INTERNAL_IPS = ("127.0.0.1",) +INTERNAL_IPS = ( + "127.0.0.1", + os.environ.get("DOCKER_INTERNAL_IP", "172.17.0.1"), +)These changes are optional but will greatly improve local debugging when using containers.
packages/types/package.json (1)
19-24: typesVersions mapping is too broad; it forces all subpath imports to index.d.ts.
"typesVersions": { "*": { "*": ["dist/index.d.ts"] } }will map any subpath (e.g.,@plane/types/anything) to the root declaration, which is misleading and can mask type errors. If only the root import is supported (exports has only"."), this block is unnecessary. Recommend removing it or, if you truly need it, mirroring the built layout ("dist/*").Apply this diff to remove the block:
"exports": { ".": { "types": "./dist/index.d.ts", "require": "./dist/index.js", "import": "./dist/index.mjs" } - }, - "typesVersions": { - "*": { - "*": [ - "dist/index.d.ts" - ] - } }, +apps/live/tsup.config.ts (1)
7-12: noExternal entry for subpath is redundant; alias already handles it.
noExternal: ["@plane/editor", "@plane/editor/lib"]is over-specified. In tsup,["@plane/editor"]suffices to cover its subpaths, and your alias resolves both@plane/editorand@plane/editor/libto source anyway.Apply this diff:
- noExternal: ["@plane/editor", "@plane/editor/lib"], + noExternal: ["@plane/editor"],Dockerfile.node (2)
31-38: Fail fast if APP_SCOPE is missing or invalid.
turbo prune --scope=${APP_SCOPE}will behave badly if unset. Add a guard to fail early and reduce CI flakiness.Apply this diff:
FROM base AS builder ARG APP_SCOPE +RUN test -n "${APP_SCOPE}" || (echo "APP_SCOPE is required (web|space|admin)"; exit 1) WORKDIR /repo
130-135: Optionally set ownership of copied assets to the runtime user.Files copied from
installerdefault to root ownership. Reading usually works, but explicit ownership avoids edge cases.Example:
-COPY --from=installer /repo/apps/${APP_SCOPE}/.next/standalone ./ -COPY --from=installer /repo/apps/${APP_SCOPE}/.next/static ./apps/${APP_SCOPE}/.next/static -COPY --from=installer /repo/apps/${APP_SCOPE}/public ./apps/${APP_SCOPE}/public +COPY --chown=node:node --from=installer /repo/apps/${APP_SCOPE}/.next/standalone ./ +COPY --chown=node:node --from=installer /repo/apps/${APP_SCOPE}/.next/static ./apps/${APP_SCOPE}/.next/static +COPY --chown=node:node --from=installer /repo/apps/${APP_SCOPE}/public ./apps/${APP_SCOPE}/publicscripts/smoke.sh (2)
221-226: Tighten curl timeouts to avoid long hangs per attempt.Each probe can hang without connect/read timeouts. Add conservative limits to make retries predictable.
Apply this diff:
- status="$(curl -sS -o /dev/null -w "%{http_code}" -L "$url" 2>"$errfile" || true)" + status="$(curl -sS --connect-timeout 2 -m 5 -o /dev/null -w "%{http_code}" -L "$url" 2>"$errfile" || true)"
189-207: Optional: remove unused cid or log it for debugging (SC2034).
cidis assigned but never used outside the success check. Either drop the assignment or log it to aid debugging.Two options:
- Simplify the run check:
- if ! cid=$(docker run -d --name "$container" -p "${port}:3000" $envflags $DOCKER_FLAGS "$image" 2>"$TMP_DIR/${container}.err"); then + if ! docker run -d --name "$container" -p "${port}:3000" $envflags $DOCKER_FLAGS "$image" 2>"$TMP_DIR/${container}.err"; then
- Or keep and log:
- if ! cid=$(docker run -d --name "$container" -p "${port}:3000" $envflags $DOCKER_FLAGS "$image" 2>"$TMP_DIR/${container}.err"); then + if ! cid=$(docker run -d --name "$container" -p "${port}:3000" $envflags $DOCKER_FLAGS "$image" 2>"$TMP_DIR/${container}.err"); then ... fi + log "Started ${name} container=${container} cid=${cid}"deployments/cli/community/build.yml (1)
6-8: Add explicit multi-stage target to avoid building non-runtime stagesIf Dockerfile.node defines multiple stages, Compose will default to the last stage. Be explicit and set target: runtime for web/space/admin to prevent accidental builder-stage images.
build: context: ../../ dockerfile: Dockerfile.node + target: runtime args: APP_SCOPE: webApply the same for space and admin:
build: context: ../../ dockerfile: Dockerfile.node + target: runtime args: APP_SCOPE: spacebuild: context: ../../ dockerfile: Dockerfile.node + target: runtime args: APP_SCOPE: adminAlso applies to: 14-16, 22-24
docker-compose.yml (3)
6-6: Be explicit about build target for frontend servicesMirror bake config by setting target: runtime for web/admin/space.
build: context: . - dockerfile: ./Dockerfile.node + dockerfile: ./Dockerfile.node + target: runtimeApply to admin and space similarly.
Also applies to: 18-18, 31-31
9-9: DOCKER_BUILDKIT as a build-arg has no effectDOCKER_BUILDKIT is a CLI/daemon feature flag, not a build ARG used in Dockerfiles. Keeping it in build.args is a no-op. Prefer setting it in the environment for the CI/compose invocations (e.g., export DOCKER_BUILDKIT=1 before docker compose build) and remove from args to reduce noise.
args: - DOCKER_BUILDKIT: 1 APP_SCOPE: web(Repeat for admin/space/services where applicable.)
Also applies to: 21-21, 34-34
126-156: Consider pinning images and resources for deterministic local devValkey, MinIO, and RabbitMQ tags are reasonable, but minio/minio latest can shift. Pinning a known-good MinIO tag avoids surprise breakage in local dev.
docker-bake.hcl (3)
46-49: Support multi-arch by default or document PLATFORMS overrideIf you regularly publish arm64 images, consider default = ["linux/amd64","linux/arm64"] or add a comment warning that Apple Silicon developers should pass --set PLATFORMS='["linux/amd64","linux/arm64"]'.
-variable "PLATFORMS" { - # List of platforms (e.g., ["linux/amd64", "linux/arm64"]) - default = ["linux/amd64"] -} +variable "PLATFORMS" { + # List of platforms (e.g., ["linux/amd64", "linux/arm64"]) + # Note: Override via --set PLATFORMS='["linux/amd64","linux/arm64"]' for multi-arch. + default = ["linux/amd64"] +}
97-105: Tag naming differs from deployments (plane-api vs plane-backend)Bake uses plane-api while CLI build.yml tags plane-backend. Not a blocker, but consider harmonizing to reduce cognitive load.
116-132: AIO target leverages BuildKit contexts; document the correct build invocationThe comments at the top of Dockerfile.aio (and elsewhere) mention build-arg-based composition, but aio actually expects build contexts (as wired here). Suggest adding a quick “How to build” note in this file header or README to point to:
docker buildx bake -f docker-bake.hcl aioscripts/smoke-aio.sh (2)
215-224: Helpful: print image digest and platform after pull/runFor debugging multi-arch issues, echo docker image inspect details (RepoDigests, Architecture, Os) after pull or before probing.
if [[ $PULL -eq 1 ]]; then log "Pulling ${IMAGE} ..." docker pull "$IMAGE" >/dev/null + docker image inspect "$IMAGE" --format 'Digest: {{index .RepoDigests 0}}{{println}}{{println}}Platform: {{.Os}}/{{.Architecture}}' || true fi
83-87: Endpoint selection is solid; small UX enhancementExpose a --only flag that takes a comma-separated list (web,space,admin,live) and toggles RUN_* accordingly. Not required for MVP.
Also applies to: 254-259
Dockerfile.aio (3)
67-71: Minimize Node/Python overlap to reduce image size and risk of overwritesCopying entire /usr/local/lib and /usr/local/bin from node can clobber Python-installed files. Since you later copy API site-packages and /usr/local/bin from api_ctx, it likely works, but it's heavy. Prefer copying only what you need from Node (node binary and core dirs).
-# Copy Node runtime into the AIO container for Next standalone apps -COPY --from=node /usr/lib /usr/lib -COPY --from=node /usr/local/lib /usr/local/lib -COPY --from=node /usr/local/include /usr/local/include -COPY --from=node /usr/local/bin /usr/local/bin +# Copy Node runtime minimally (binary and node_modules if required) +COPY --from=node /usr/local/bin/node /usr/local/bin/node +COPY --from=node /usr/local/lib/node_modules /usr/local/lib/node_modulesIf Next standalone doesn’t require global node_modules, copying just node is often sufficient.
Also applies to: 84-88
103-111: Caddyfile rewrites look correct; make sed resilient to upstream format changesIf Caddyfile.ce changes service names/ports, these sed lines can silently fail. Consider templating (envsubst) or using placeholders in Caddyfile.ce to reduce fragility. Not blocking.
118-120: Consider adding a basic HEALTHCHECKA simple HTTP GET to /live/health (via localhost) would allow orchestration to detect readiness in single-container runs.
EXPOSE 80 443 - -CMD ["/app/start.sh"] +HEALTHCHECK --interval=10s --timeout=3s --retries=10 CMD wget -qO- http://127.0.0.1/live/health || exit 1 +CMD ["/app/start.sh"].github/workflows/build-images.yml (3)
58-65: Quote $GITHUB_ENV redirection (SC2086) and harden the shell snippet.Shellcheck is right: the file redirection should quote $GITHUB_ENV. Also minor robustness: add set -euo pipefail.
Apply this diff:
- - name: Set bake group based on include_aio - run: | - if [ "${{ inputs.include_aio }}" = "true" ]; then - echo "BAKE_GROUP=all" >> $GITHUB_ENV - else - echo "BAKE_GROUP=default" >> $GITHUB_ENV - fi + - name: Set bake group based on include_aio + shell: bash + run: | + set -euo pipefail + if [ "${{ inputs.include_aio }}" = "true" ]; then + echo "BAKE_GROUP=all" >> "$GITHUB_ENV" + else + echo "BAKE_GROUP=default" >> "$GITHUB_ENV" + fi
66-69: Use expression syntax consistently in if: conditions.Minor consistency nit: wrap contains() in ${{ }} like elsewhere. Current usage works but is visually inconsistent.
Apply this diff:
- - name: Set up QEMU (for multi-arch) - if: contains(inputs.platforms, 'linux/arm64') + - name: Set up QEMU (for multi-arch) + if: ${{ contains(inputs.platforms, 'linux/arm64') }}
95-113: Standardize if: expression style across build steps.You use bare booleans and ${{ }} forms interchangeably. Recommend sticking to one style for readability.
Apply this diff:
- - name: Build (no push) - if: ${{ inputs.push == false }} + - name: Build (no push) + if: ${{ inputs.push == false }} - - name: Build and Push - if: ${{ inputs.push == true }} + - name: Build and Push + if: ${{ inputs.push == true }}No functional change—just confirms consistent expression usage everywhere.
docker-compose-local.yml (3)
118-123: Quote the Postgres command to avoid YAML parsing pitfalls.Unquoted single quotes inside the command can be fragile. Quote the whole command.
Apply this diff:
- command: postgres -c 'max_connections=1000' + command: "postgres -c 'max_connections=1000'"
29-36: Consider healthchecks and conditional startup for dependent services.depends_on controls start order, not readiness. Workers/migrator can fail if DB/Redis/RabbitMQ/MinIO aren’t ready.
If helpful, I can add minimal healthchecks (pg_isready, redis-cli ping, curl 9000) and depends_on: condition: service_healthy for api/worker/beat/migrator.
Also applies to: 57-62, 83-88, 109-113
172-180: Remove stale comment or add the actual cache volumes.The “Node/pnpm caches” comment has no corresponding volumes defined.
Option A: remove the comment. Option B: define volumes like node_cache or pnpm_store and mount them where needed.
.github/workflows/build-branch.yml (2)
207-228: Consider pinning makeplane/actions/build-push version consistently.AIO uses v1.1.0; others use v1.0.0. If v1.1.0 is needed for additional-assets, keep it for AIO, but consider upgrading others for consistency (or add a brief comment why versions differ).
75-86: Guard against empty BUILDX_ENDPOINT when driver is docker-container.You already output an empty BUILDX_ENDPOINT. Ensure the downstream action tolerates empty value (looks OK). Optional: only set endpoint output when using cloud to reduce confusion.
Also applies to: 126-130
Dockerfile.api (3)
8-11: Update usage examples to match the file’s actual location.Comments show plane/Dockerfile.api, but this file appears at repo root as Dockerfile.api. Adjust examples to avoid confusion (and align all workflows to the same path).
Apply this diff:
-# docker build -f plane/Dockerfile.api --target dev -t plane-api:dev . -# docker build -f plane/Dockerfile.api --target runtime -t plane-api:latest . +# docker build -f Dockerfile.api --target dev -t plane-api:dev . +# docker build -f Dockerfile.api --target runtime -t plane-api:latest .
72-88: Alpine nodejs-current availability may vary; consider pinning or using nodejs + npm.nodejs-current comes from community and tracks latest; availability can drift between Alpine versions. If reproducibility matters, pin Alpine version or switch to nodejs + npm with explicit versions.
133-136: Runtime runs as root with chmod -R 777; consider a non-root user.For production, 777 on /code is over-permissive. Create an app user, chown the working dir, and run as that user.
Example outside selected lines:
# after copying files RUN addgroup -S app && adduser -S app -G app && \ mkdir -p /code/plane/logs && chown -R app:app /code USER appAlso applies to: 137-144
.github/workflows/docker-smoke.yml (2)
136-141: Consider passing a build context that matches Dockerfile expectations.You build with
-f plane/Dockerfile.node ... planeas context. Ensure Dockerfile.node’s COPY paths are relative to the plane/ directory. If the Dockerfile was moved to repo root, switch to-f Dockerfile.node ..
142-146: Quote matrix.env_flags expansion to avoid accidental word splitting.Currently env_flags is injected raw. It’s fine because you control it, but quoting is safer. You can transform env_flags into an array via bash eval—optional.
Example:
- docker run -d --name ${{ matrix.container }} -p ${{ matrix.host_port }}:3000 ${{ matrix.env_flags }} ${{ matrix.image }} + docker run -d --name "${{ matrix.container }}" -p "${{ matrix.host_port }}:3000" ${{ matrix.env_flags }} "${{ matrix.image }}"Also applies to: 168-172
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (28)
.github/workflows/build-branch.yml(7 hunks).github/workflows/build-images.yml(1 hunks).github/workflows/docker-smoke.yml(1 hunks)CONTRIBUTING.md(1 hunks)Dockerfile.aio(1 hunks)Dockerfile.api(1 hunks)Dockerfile.node(1 hunks)apps/admin/Dockerfile.admin(0 hunks)apps/admin/Dockerfile.dev(0 hunks)apps/api/Dockerfile.api(0 hunks)apps/api/Dockerfile.dev(0 hunks)apps/api/plane/settings/local.py(1 hunks)apps/live/package.json(1 hunks)apps/live/tsup.config.ts(1 hunks)apps/space/Dockerfile.dev(0 hunks)apps/space/Dockerfile.space(0 hunks)apps/web/Dockerfile.dev(0 hunks)apps/web/Dockerfile.web(0 hunks)deployments/aio/community/supervisor.conf(3 hunks)deployments/cli/community/build.yml(2 hunks)docker-bake.hcl(1 hunks)docker-compose-local.yml(1 hunks)docker-compose.yml(7 hunks)packages/editor/tsup.config.ts(1 hunks)packages/types/package.json(1 hunks)packages/ui/tsup.config.ts(1 hunks)scripts/smoke-aio.sh(1 hunks)scripts/smoke.sh(1 hunks)
💤 Files with no reviewable changes (8)
- apps/space/Dockerfile.dev
- apps/admin/Dockerfile.admin
- apps/space/Dockerfile.space
- apps/api/Dockerfile.dev
- apps/web/Dockerfile.dev
- apps/api/Dockerfile.api
- apps/admin/Dockerfile.dev
- apps/web/Dockerfile.web
🧰 Additional context used
🪛 GitHub Actions: Docker build and smoke test for apps
docker-bake.hcl
[error] 1-1: Command 'docker buildx bake -f plane/docker-bake.hcl aio' failed: open plane/docker-bake.hcl: no such file or directory.
apps/api/plane/settings/local.py
[error] 1-1: Docker build failed: unable to prepare context: path "plane" not found.
🪛 actionlint (1.7.7)
.github/workflows/build-images.yml
59-59: shellcheck reported issue in this script: SC2086:info:2:28: Double quote to prevent globbing and word splitting
(shellcheck)
59-59: shellcheck reported issue in this script: SC2086:info:4:32: Double quote to prevent globbing and word splitting
(shellcheck)
🪛 Shellcheck (0.10.0)
scripts/smoke.sh
[warning] 200-200: cid appears unused. Verify use (or export if used externally).
(SC2034)
[error] 297-297: 'local' is only valid in functions.
(SC2168)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build and lint web apps
- GitHub Check: Build and lint web apps
- GitHub Check: Build and lint web apps
- GitHub Check: Analyze (javascript)
🔇 Additional comments (15)
CONTRIBUTING.md (1)
245-245: LGTM: minor formatting cleanupConverting the Discord line from a list item to a paragraph reads better in this section. No issues.
deployments/aio/community/supervisor.conf (3)
109-110: Verify LIVE_BASE_PATH routing and proxy configurationI’ve confirmed that the application code correctly reads and applies the
LIVE_BASE_PATHenvironment variable:
- In
apps/live/src/server.ts, the live‐server router is mounted using
this.app.use(process.env.LIVE_BASE_PATH || "/live", this.router)
so the base path is driven by the env var.- Frontend code imports and uses
LIVE_BASE_PATH(fromprocess.env.NEXT_PUBLIC_LIVE_BASE_PATH) when constructing URLs and WebSocket paths inapps/web/.../editor-body.tsx.- The constant is also defined in
packages/constants/src/endpoints.tsand used to buildLIVE_URL.However, I did not locate any Caddyfile (or equivalent proxy config) routing
/liveto port 3005:
- No
Caddyfileexists in the repo to verify a/live→localhost:3005route.- The only reference to port 3005 is in the Supervisor config (
deployments/aio/community/supervisor.conf), but nothing in a reverse‐proxy config.Please confirm that your reverse proxy (Caddy, Nginx, ingress, etc.) has a rule along the lines of:
handle_path /live* { reverse_proxy localhost:3005 }or otherwise ensures requests under
/liveare forwarded (without stripping) to the live server.
100-100: No action needed: Live artifact path is correctThe
plane-liveimage placesdist/server.jsat/app/apps/live/dist/server.js, and in the AIO multi-stage build you copy the entire/appdirectory fromlive-imginto/app/live. That results in the file existing at/app/live/apps/live/dist/server.jsas referenced insupervisor.conf. All paths line up, so Supervisor will correctly launch the built artifact.
100-100: Please verify your CI workflow’s build context and working-directoryI wasn’t able to locate your GitHub Actions workflow file in the repository to confirm how the Bake/Buildx or Build-Push steps are configured. To resolve the “path 'plane' not found” error, double-check that:
- You’re setting the correct working directory for any
docker/bake-actionsteps (e.g.
working-directory: ./so thatdocker-bake.hclis found at the root of the checkout).- You’re using the right
context: .for anydocker/build-push-actionsteps (and not a literalplanepath).- If you’re using the
files:input indocker/bake-action, it points to thedocker-bake.hclin the repo root (e.g.files: ['docker-bake.hcl']).Bullet-point your workflow steps under
.github/workflows/...and ensure they reference.(the repo root) rather thanplane. Once confirmed or corrected, rerun the pipeline to see if the error is resolved.packages/ui/tsup.config.ts (2)
7-7: Good call ondts: { respectExternal: true }Prevents emitting types for externals and avoids leaking their declarations into the bundle. Looks correct.
12-12: ✅ External dependencies correctly declared
- Verified that
packages/ui/tsup.config.tsmarks as external:
react,react-dom,@plane/types,@plane/utils,@plane/constants- Checked
packages/ui/package.json:
- Peer dependencies include
reactandreact-dom- Regular dependencies include
@plane/types,@plane/utils, and@plane/constantsAll entries in the
externallist are matched appropriately inpeerDependenciesordependencies. No further changes are needed.apps/live/tsup.config.ts (1)
7-12: Monorepo-only alias: confirm external/publish builds don’t rely on this.The alias points to workspace sources (
../../packages/editor/src/...). That’s fine for local builds but will fail ifapps/liveis built outside the monorepo or in a publish scenario. Ensure CI paths always build in-repo, or gate the alias via env to fall back to the package import.packages/editor/tsup.config.ts (1)
3-4: Good change: watch-aware clean prevents churn in dev.Switching to
defineConfig((options) => ...)andclean: !options.watchis the right tradeoff for faster iterations.deployments/cli/community/build.yml (2)
6-8: APP_SCOPE contract verified and correctly implementedThe
Dockerfile.nodeexplicitly documents the allowedAPP_SCOPEvalues (“web”, “space”, “admin”) and consistently usesARG APP_SCOPEin all build and copy steps:
- Lines 9–13 confirm the contract in comments: “APP_SCOPE must be one of: web, space, admin”.
- Build steps use
--scope=${APP_SCOPE}forturbo pruneand--filter=${APP_SCOPE}forpnpm turbo run build.- Runtime and final image stages copy from
apps/${APP_SCOPE}and include a runtime check that errors (and lists known apps) if the directory doesn’t exist.No further branching logic is required—passing an unsupported value will fail fast, alerting to a misconfiguration.
35-35: Invalidruntimetarget in build.yml – please correct or define in docker-bake.hclOur inspection shows that adding
target: runtimewill reference a non-existent stage, since no
runtimetarget exists in docker-bake.hcl. You have two options:
- Update the
target:value to match an existing build stage in docker-bake.hcl.- Or add a
runtimetarget definition in docker-bake.hcl before referencing it here.Regarding keeping the build context lean, your root
.dockerignorealready excludes large paths (node_modules/,.git, build artifacts, etc.), so no additional ignores are strictly necessary.Likely an incorrect or invalid review comment.
docker-compose.yml (1)
43-45: Docker Compose configuration is already aligned with theruntimestage and entrypoints resolve correctly
- The Dockerfile.api’s last stage is named
runtime, so builds without an explicittargetalready default to that stage.- In the
runtimestage,WORKDIR /codeis set, and the linesplace allCOPY apps/api/bin ./bindocker-entrypoint-*.shscripts under/code/bin.- In
docker-compose.yml, commands likerun from the container’scommand: ./bin/docker-entrypoint-api.shWORKDIR (/code), so the entrypoint scripts resolve correctly.Adding
target: runtimeto each service is purely optional for clarity and does not change behavior.
scripts/smoke-aio.sh (1)
134-143: Trap handler exits with last code — goodCleanup is robust (conditional removal, tmpdir cleanup). No changes needed.
Dockerfile.aio (2)
93-98: AIO runtime assets present
Confirmed thatdeployments/aio/community/start.sh,supervisor.conf, andvariables.envall exist in the repository. No further action required.
84-86: Verified:/codeis the correct app root in Dockerfile.apiI’ve confirmed that in Dockerfile.api the base, dev, and runtime stages all set
WORKDIR /codeand place the API sources under that directory. Consequently,COPY --from=api_ctx /code /app/backendin Dockerfile.aio will pull in the full API code as intended. No changes needed here..github/workflows/docker-smoke.yml (1)
16-20: The shell script to locate the Dockerfile and bake-file paths plus inspect the workflow file has been queued. I’ll review the results and then either confirm or update the review comment accordingly.
lifeiscontent
force-pushed
the
feat/fix-local-docker-dev
branch
2 times, most recently
from
149aca6 to
b24ecf9
Compare
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/build-images.yml (1)
98-113: Action Required: Correct thedocker-bake.hclfile path in workflowsThe repository’s
docker-bake.hcllives at the project root, not underplane/. All existing workflows reference-f plane/docker-bake.hcl, which will fail because that file does not exist inplane/.Affected workflows:
.github/workflows/build-images.yml(lines 98–113).github/workflows/docker-smoke.yml(multiple bake invocations)Please update each invocation to point at the root file (or relocate
docker-bake.hclintoplane/if that’s the intended structure). For example, in build-images.yml:- docker buildx bake -f plane/docker-bake.hcl "${BAKE_GROUP}" \ + docker buildx bake -f docker-bake.hcl "${BAKE_GROUP}" \Ensure that all other workflows referencing
plane/docker-bake.hclare likewise updated so builds no longer fail.
♻️ Duplicate comments (7)
packages/types/package.json (1)
11-11: ESM entry change is fine; verify dist emits index.mjsYou switched "module" to "./dist/index.mjs". Node will prefer "exports" anyway, but bundlers read "module". Please confirm tsup actually emits index.mjs to avoid runtime resolution failures.
Run:
#!/bin/bash pnpm -w --filter @plane/types build ls -lah packages/types/dist || true for f in index.js index.mjs index.d.ts; do test -f packages/types/dist/$f && echo "$f OK" || echo "$f MISSING" donescripts/smoke-aio.sh (1)
270-276: Add curl timeouts to prevent hangs in CIWithout timeouts, a stalled connection can hang until TCP timeouts. Add conservative connect and total timeouts.
- status="$(curl -sS -o /dev/null -w "%{http_code}" -L "$url" 2>"$errfile" || true)" + status="$(curl -sS --connect-timeout 2 --max-time 5 -o /dev/null -w "%{http_code}" -L "$url" 2>"$errfile" || true)"Dockerfile.aio (1)
6-21: Header instructions are incorrect: this Dockerfile consumes BuildKit build-contexts, not --build-arg image tagsThe example still shows --build-arg WEB_IMG/SPACE_IMG/etc., but the Dockerfile copies from named contexts (web_ctx, api_ctx, etc.). Update docs to show buildx bake or docker build with --build-context so users don’t get a broken build.
-# Build requirements: -# - Build and tag the per-app images first (or override args below): -# plane-web:latest, plane-space:latest, plane-admin:latest, plane-live:latest, -# plane-api:latest, plane-proxy:latest -# -# Example: -# docker build \ -# -f plane/Dockerfile.aio \ -# --build-arg WEB_IMG=plane-web:latest \ -# --build-arg SPACE_IMG=plane-space:latest \ -# --build-arg ADMIN_IMG=plane-admin:latest \ -# --build-arg LIVE_IMG=plane-live:latest \ -# --build-arg API_IMG=plane-api:latest \ -# --build-arg PROXY_IMG=plane-proxy:latest \ -# -t plane-aio:latest . +# Build requirements: +# - Buildx/BuildKit with support for external contexts. +# - Either: +# a) docker buildx bake -f docker-bake.hcl aio +# b) docker build \ +# -f Dockerfile.aio \ +# --build-context web_ctx=target:web \ +# --build-context space_ctx=target:space \ +# --build-context admin_ctx=target:admin \ +# --build-context live_ctx=target:live \ +# --build-context api_ctx=target:api \ +# --build-context proxy_ctx=target:proxy \ +# -t plane-aio:latest .scripts/smoke.sh (1)
296-303: Bash error: ‘local’ at top-level breaks the script (SC2168)This case block isn’t inside a function; local is invalid here. Replace with a plain assignment.
- --image-prefix) - local prefix="$2" + --image-prefix) + prefix="$2" WEB_IMAGE="${prefix}plane-web:latest" SPACE_IMAGE="${prefix}plane-space:latest" ADMIN_IMAGE="${prefix}plane-admin:latest" LIVE_IMAGE="${prefix}plane-live:latest" shift 2 ;;docker-compose-local.yml (1)
149-166: MinIO bootstrap uses ‘mc’ inside minio/minio — client isn’t present; init will failThe minio/minio image doesn’t ship the mc client. Move bootstrap to a short-lived sidecar and restore the server-only command on the main container.
plane-minio: image: minio/minio restart: unless-stopped - # Bootstrap bucket on first run; suitable for local smoke/dev only - entrypoint: > - /bin/sh -c " - set -e - mkdir -p /export/${AWS_S3_BUCKET_NAME:-uploads}; - minio server /export --console-address ':9090' & pid=$!; - sleep 5; - mc alias set myminio http://plane-minio:9000 ${AWS_ACCESS_KEY_ID:-minioadmin} ${AWS_SECRET_ACCESS_KEY:-minioadmin}; - mc mb myminio/${AWS_S3_BUCKET_NAME:-uploads} -p || true; - wait $pid - " + command: server /export --console-address ':9090' environment: MINIO_ROOT_USER: ${AWS_ACCESS_KEY_ID:-minioadmin} MINIO_ROOT_PASSWORD: ${AWS_SECRET_ACCESS_KEY:-minioadmin} volumes: - uploads:/export ports: - "9000:9000" - "9090:9090" + + plane-minio-init: + image: minio/mc + depends_on: + - plane-minio + entrypoint: > + /bin/sh -c " + set -e; + until (mc alias set myminio http://plane-minio:9000 ${AWS_ACCESS_KEY_ID:-minioadmin} ${AWS_SECRET_ACCESS_KEY:-minioadmin}); do echo 'waiting for minio...'; sleep 2; done; + mc mb myminio/${AWS_S3_BUCKET_NAME:-uploads} -p || true; + " + restart: "no".github/workflows/build-images.yml (2)
75-84: Step if: uses step-local env; evaluate secrets directly in the condition.
if:currently references env.* that’s defined inside the same step; those vars aren’t available duringifevaluation. Evaluatesecrets.*directly.- - name: Log in to Docker Hub (optional) - if: inputs.push && env.DOCKERHUB_USERNAME != '' && env.DOCKERHUB_TOKEN != '' + - name: Log in to Docker Hub (optional) + if: ${{ inputs.push && secrets.DOCKERHUB_USERNAME != '' && secrets.DOCKERHUB_TOKEN != '' }} env: DOCKERHUB_USERNAME: ${{ secrets.DOCKERHUB_USERNAME }} DOCKERHUB_TOKEN: ${{ secrets.DOCKERHUB_TOKEN }} uses: docker/login-action@v3 with: username: ${{ env.DOCKERHUB_USERNAME }} password: ${{ env.DOCKERHUB_TOKEN }}
85-94: Same issue for GHCR login: evaluate secret in the if condition.Use
secrets.GHCR_TOKENdirectly in theif:.- - name: Log in to GHCR (optional) - if: inputs.push && env.GHCR_TOKEN != '' + - name: Log in to GHCR (optional) + if: ${{ inputs.push && secrets.GHCR_TOKEN != '' }} env: GHCR_TOKEN: ${{ secrets.GHCR_TOKEN }} uses: docker/login-action@v3 with: registry: ghcr.io username: ${{ github.actor }} password: ${{ env.GHCR_TOKEN }}
🧹 Nitpick comments (17)
packages/types/package.json (2)
16-18: Conditional exports look correct; consider adding "default" for broader tooling compatibilitySome bundlers/tools fall back to "exports.default". Optional but low-risk improvement.
"exports": { ".": { "types": "./dist/index.d.ts", "require": "./dist/index.js", - "import": "./dist/index.mjs" + "import": "./dist/index.mjs", + "default": "./dist/index.mjs" } },
19-24: typesVersions may be redundant if you only target TS ≥ 4.7With top-level "types" and export "types", modern TS resolves fine. Keep this only if you must support older TS; otherwise simplify by dropping it.
- "typesVersions": { - "*": { - "*": [ - "dist/index.d.ts" - ] - } - },deployments/cli/community/build.yml (2)
6-8: Shared Dockerfile.node: consider selecting the intended stageIf
Dockerfile.nodeis multi-stage (dev/build/runtime), Compose defaults to the last stage. To ensure you ship the correct image locally and in CI, set thetargetexplicitly.build: context: ../../ dockerfile: Dockerfile.node + target: runtime args: APP_SCOPE: webApply the same for
spaceandadminblocks.Also applies to: 14-16, 22-24
27-31: Confirm whether live should also use the shared Node image
livestill builds fromapps/live/Dockerfile.livewhileweb/space/adminmoved toDockerfile.nodewithAPP_SCOPE. If the unified Dockerfile supportslive, consider switching for consistency; otherwise, a quick comment explaining the exception helps future contributors.apps/api/plane/settings/local.py (2)
11-18: Narrow exception handling and guard toolbar behind DEBUGCatching a broad
Exceptioncan mask unrelated errors. Also ensure the toolbar only enables whenDEBUGis true to avoid accidental activation in non-debug environments.-if os.environ.get("DJANGO_DEBUG_TOOLBAR", "0") == "1": - try: - import debug_toolbar # noqa: F401 - INSTALLED_APPS += ("debug_toolbar",) # noqa - MIDDLEWARE += ("debug_toolbar.middleware.DebugToolbarMiddleware",) # noqa - except Exception: - # If debug_toolbar isn't installed, skip enabling it - pass +if DEBUG and os.environ.get("DJANGO_DEBUG_TOOLBAR", "0") == "1": # noqa: F405 + try: + import debug_toolbar # noqa: F401 + INSTALLED_APPS += ("debug_toolbar",) # noqa + MIDDLEWARE += ("debug_toolbar.middleware.DebugToolbarMiddleware",) # noqa + except ImportError: + # Debug toolbar not installed; skip enabling it + pass
33-33: Docker local dev: INTERNAL_IPS usually needs container-aware addressesWhen running via Docker, requests often originate from bridge IPs (e.g., 172.17.0.1/24), so
("127.0.0.1",)may not show the toolbar. Consider adding a container-friendly helper.Example:
# Append common Docker bridge gateway IPs when DEBUG toolbar is enabled if DEBUG and os.environ.get("DJANGO_DEBUG_TOOLBAR", "0") == "1": INTERNAL_IPS += ("172.17.0.1", "172.18.0.1")Or implement the dynamic pattern recommended in the debug-toolbar docs for Docker networks.
.github/workflows/pull-request-build-lint-api.yml (1)
32-37: Fix step naming and avoid auto-fixing code in CI
- The step label says “Install Pylint” but installs
ruff.ruff check --fixmutates the workspace; CI should lint, not change code. Prefer--diffor fail on issues.- - name: Install Pylint + - name: Install Ruff run: python -m pip install ruff @@ - - name: Lint apps/api - run: ruff check --fix apps/api + - name: Lint apps/api + run: ruff check --diff apps/apiscripts/smoke-aio.sh (2)
155-156: Nit: add connect-timeout in the port-probe curl fallbackYou already cap total time with
-m 1. Also add a connect timeout to fail fast on non-routable hosts.- code="$(curl -sS -m 1 -o /dev/null -w "%{http_code}" "http://${host}:${port}/" || true)" + code="$(curl -sS --connect-timeout 1 -m 1 -o /dev/null -w "%{http_code}" "http://${host}:${port}/" || true)"
21-22: Bash 4+ required (associative arrays)You use
declare -A, which requires Bash 4+. On macOS older shells are 3.2. Consider a version guard to emit a clear error instead of a cryptic failure.set -euo pipefail + +# Require Bash 4+ (associative arrays) +if [[ -z "${BASH_VERSINFO:-}" || "${BASH_VERSINFO[0]}" -lt 4 ]]; then + echo "This script requires Bash 4+. Please run in CI/Linux or install a newer bash." >&2 + exit 1 +fiDockerfile.aio (2)
78-82: Nit: also purge Next.js cache for ‘live’ appYou clean caches for web/space/admin but not live; add it for consistency and smaller layers.
RUN rm -rf /app/web/apps/web/.next/cache || true \ && rm -rf /app/space/apps/space/.next/cache || true \ - && rm -rf /app/admin/apps/admin/.next/cache || true + && rm -rf /app/admin/apps/admin/.next/cache || true \ + && rm -rf /app/live/apps/live/.next/cache || true
37-38: Hard-coded CPython 3.12 site-packages path makes PY_VERSION overrides fragileIf PY_VERSION is changed (e.g., 3.13), COPY will miss the correct site-packages path. Parameterize the major.minor.
-ARG NODE_VERSION=22-alpine -ARG PY_VERSION=3.12.10-alpine +ARG NODE_VERSION=22-alpine +ARG PY_VERSION=3.12.10-alpine +# Keep this in sync with PY_VERSION major.minor +ARG PY_MM=3.12 @@ -COPY --from=api_ctx /usr/local/lib/python3.12/site-packages/ /usr/local/lib/python3.12/site-packages/ +COPY --from=api_ctx /usr/local/lib/python${PY_MM}/site-packages/ /usr/local/lib/python${PY_MM}/site-packages/If api_ctx may vary independently, consider discovering the path at build-time via a small helper (python -c 'import site,sys; print(site.getsitepackages()[0])') and copying from that path.
Also applies to: 84-88
scripts/smoke.sh (1)
188-206: Unused variable ‘cid’ (SC2034) — simplify start logicYou never use $cid; avoid capturing it to quiet shellcheck and reduce noise.
- if ! cid=$(docker run -d --name "$container" -p "${port}:3000" $envflags $DOCKER_FLAGS "$image" 2>"$TMP_DIR/${container}.err"); then + if ! docker run -d --name "$container" -p "${port}:3000" $envflags $DOCKER_FLAGS "$image" 2>"$TMP_DIR/${container}.err"; then @@ - if ! cid=$(docker run -d --name "$container" -p "${port}:3000" $envflags $DOCKER_FLAGS "$alt_image" 2>"$TMP_DIR/${container}.err"); then + if ! docker run -d --name "$container" -p "${port}:3000" $envflags $DOCKER_FLAGS "$alt_image" 2>"$TMP_DIR/${container}.err"; thendocker-compose-local.yml (1)
114-128: Optional: pin images consistently and align health/ports for smoother DX
- You pinned Postgres/Valkey/RabbitMQ but not MinIO; consider pinning minio/minio to a known-good tag for reproducible dev.
- Consider adding simple healthchecks for plane-db and plane-redis so api/worker waits reliably.
Also applies to: 129-148, 149-171
docker-compose.yml (1)
146-156: MinIO bucket bootstrap is missing in prod-ish composeIf API requires a pre-existing bucket, either add a one-shot init sidecar (minio/mc) here too or ensure the API creates buckets on first use. Otherwise first-run may fail with NoSuchBucket.
.github/workflows/build-images.yml (2)
58-65: Quote $GITHUB_ENV redirections to satisfy shellcheck SC2086 and avoid path splitting.Use
>> "$GITHUB_ENV"in both branches.- if [ "${{ inputs.include_aio }}" = "true" ]; then - echo "BAKE_GROUP=all" >> $GITHUB_ENV - else - echo "BAKE_GROUP=default" >> $GITHUB_ENV - fi + if [ "${{ inputs.include_aio }}" = "true" ]; then + echo "BAKE_GROUP=all" >> "$GITHUB_ENV" + else + echo "BAKE_GROUP=default" >> "$GITHUB_ENV" + fi
66-69: Prefer explicit expression syntax for step condition.Use
${{ ... }}inif:for clarity and to avoid accidental string parsing.- - name: Set up QEMU (for multi-arch) - if: contains(inputs.platforms, 'linux/arm64') + - name: Set up QEMU (for multi-arch) + if: ${{ contains(inputs.platforms, 'linux/arm64') }} uses: docker/setup-qemu-action@v3.github/workflows/build-branch.yml (1)
296-301: Minor: Use double quotes for variable assignment to protect spaces.Not strictly required given sanitized values, but quoting
aio_versionassignment is safer.- aio_version=${{ needs.branch_build_setup.outputs.release_version }} + aio_version="${{ needs.branch_build_setup.outputs.release_version }}" @@ - aio_version=${{ needs.branch_build_setup.outputs.gh_branch_name }} + aio_version="${{ needs.branch_build_setup.outputs.gh_branch_name }}"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (30)
.github/workflows/build-branch.yml(7 hunks).github/workflows/build-images.yml(1 hunks).github/workflows/docker-smoke.yml(1 hunks).github/workflows/pull-request-build-lint-api.yml(2 hunks)CONTRIBUTING.md(1 hunks)Dockerfile.aio(1 hunks)Dockerfile.api(1 hunks)Dockerfile.node(1 hunks)apps/admin/Dockerfile.admin(0 hunks)apps/admin/Dockerfile.dev(0 hunks)apps/api/Dockerfile.api(0 hunks)apps/api/Dockerfile.dev(0 hunks)apps/api/plane/settings/local.py(1 hunks)apps/api/requirements/base.txt(2 hunks)apps/live/package.json(1 hunks)apps/live/tsup.config.ts(1 hunks)apps/space/Dockerfile.dev(0 hunks)apps/space/Dockerfile.space(0 hunks)apps/web/Dockerfile.dev(0 hunks)apps/web/Dockerfile.web(0 hunks)deployments/aio/community/supervisor.conf(3 hunks)deployments/cli/community/build.yml(2 hunks)docker-bake.hcl(1 hunks)docker-compose-local.yml(1 hunks)docker-compose.yml(7 hunks)packages/editor/tsup.config.ts(1 hunks)packages/types/package.json(1 hunks)packages/ui/tsup.config.ts(1 hunks)scripts/smoke-aio.sh(1 hunks)scripts/smoke.sh(1 hunks)
💤 Files with no reviewable changes (8)
- apps/admin/Dockerfile.dev
- apps/web/Dockerfile.web
- apps/admin/Dockerfile.admin
- apps/api/Dockerfile.api
- apps/space/Dockerfile.dev
- apps/space/Dockerfile.space
- apps/web/Dockerfile.dev
- apps/api/Dockerfile.dev
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/ui/tsup.config.ts
- docker-bake.hcl
- deployments/aio/community/supervisor.conf
- apps/live/tsup.config.ts
- packages/editor/tsup.config.ts
- Dockerfile.node
- apps/live/package.json
- CONTRIBUTING.md
- Dockerfile.api
🧰 Additional context used
🪛 GitHub Actions: Docker build and smoke test for apps
apps/api/plane/settings/local.py
[error] 1-1: Docker build failed: unable to prepare context: path "plane" not found. Build command attempted: docker build -f plane/Dockerfile.node --target runtime --build-arg APP_SCOPE="$name" -t "$tag" plane
🪛 actionlint (1.7.7)
.github/workflows/build-images.yml
59-59: shellcheck reported issue in this script: SC2086:info:2:28: Double quote to prevent globbing and word splitting
(shellcheck)
59-59: shellcheck reported issue in this script: SC2086:info:4:32: Double quote to prevent globbing and word splitting
(shellcheck)
🪛 Shellcheck (0.10.0)
scripts/smoke.sh
[warning] 200-200: cid appears unused. Verify use (or export if used externally).
(SC2034)
[error] 297-297: 'local' is only valid in functions.
(SC2168)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: Build and lint web apps
🔇 Additional comments (9)
packages/types/package.json (2)
36-47: Peer dependencies on React and React-DOM are requiredI searched the entire
packages/types/srcdirectory and found multiple usages of React types (e.g.React.FC,React.ReactNode,React.ReactElement,React.CSSProperties) across files likeview-props.ts,instance/auth.ts,common.ts,issues.ts,analytics.ts, andcharts/index.ts. Since the generated declaration files import from'react'and'react-dom', consumers must have those packages installed. Keeping them inpeerDependenciesis correct; no change needed here.
26-35: No changes needed—formats and types are already configuredThe local tsup.config.ts in packages/types explicitly sets
• entry: ["src/index.ts"]
• format: ["esm","cjs"]
• dts: true
• clean: true
• minify: trueTherefore, running
tsup --watchandtsup --minifyautomatically emits CJS, ESM, and.d.tsin dist, satisfying yourmain/module/typesfields.If you’d like to simplify or make flags explicit, you can optionally:
• Change dev to
tsup -w(uses the config)
• Change build totsup --minify(uses the config)Or, to embed flags in scripts:
"scripts": { - "dev": "tsup --watch", - "build": "tsc --noEmit && tsup --minify", + "dev": "tsup src/index.ts --watch --format cjs,esm --dts", + "build": "tsc --noEmit && tsup src/index.ts --format cjs,esm --dts --minify", … }—but this is purely optional, not required for correctness.
apps/api/requirements/base.txt (1)
69-69: LGTM on drf-spectacular pinKeeping
drf-spectacular==0.28.0is consistent with Django 4.2.22 and avoids drift in your OpenAPI generation. No action needed..github/workflows/pull-request-build-lint-api.yml (2)
31-31: Upgrade to Python 3.12: looks goodAligns with Dockerfile.api and modern dependency pins. Nice.
35-35: No changes needed for the requirements pathThe
apps/api/requirements.txtfile is present in the repository, so the existing step:run: cd apps/api && pip install -r requirements.txtwill correctly install your dependencies. No update to
requirements/base.txtis required.docker-compose.yml (1)
40-47: Build contexts moved to repo root — good consolidationUsing Dockerfile.api/node at repo root with context . aligns with the PR’s unified build strategy. No issues spotted.
Also applies to: 55-62, 71-78, 87-94
.github/workflows/build-branch.yml (3)
199-201: Same path convention for Space — good.
244-246: Confirmed presence of root-level Dockerfile.api
Verified thatDockerfile.apiexists at the repository root, so the reference in.github/workflows/build-branch.ymlis valid.
175-177: Align smoke tests with the updated Dockerfile pathThe build-branch workflow now uses
dockerfile-path: ./Dockerfile.node(root) andAPP_SCOPE=web, but your smoke tests in .github/workflows/docker-smoke.yml still reference the oldplane/Dockerfile.node. These must be updated to match the new conventions:• In the workspace files section (lines 16, 32, 69), replace
- "plane/Dockerfile.node" + "./Dockerfile.node"• In the Docker build invocation (around line 139), update
- docker build -f plane/Dockerfile.node --target runtime --build-arg APP_SCOPE="$name" -t "$tag" plane + docker build -f Dockerfile.node --target runtime --build-arg APP_SCOPE="$name" -t "$tag" .This ensures the smoke tests reference the same Dockerfile and context as
build-branch.yml.
lifeiscontent
force-pushed
the
feat/fix-local-docker-dev
branch
3 times, most recently
from
74509ae to
3444bb9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
.github/workflows/build-branch.yml (1)
372-385: And again: trailing comma in needs list for publish_release.Mirror the prior correction to keep YAML valid.
- needs: - [ - branch_build_setup, - branch_build_push_admin, - branch_build_push_web, - branch_build_push_space, - branch_build_push_live, - branch_build_push_api, - branch_build_push_proxy, - ] + needs: + - branch_build_setup + - branch_build_push_admin + - branch_build_push_web + - branch_build_push_space + - branch_build_push_live + - branch_build_push_api + - branch_build_push_proxyapps/api/plane/api/views/cycle.py (4)
804-811: Fix TypeError when archiving cycles with no end_dateIf end_date is None, comparing it to timezone.now() raises a TypeError. Also, draft/no-end cycles should not be archivable.
- if cycle.end_date >= timezone.now(): + if not cycle.end_date or cycle.end_date >= timezone.now(): return Response( {"error": "Only completed cycles can be archived"}, status=status.HTTP_400_BAD_REQUEST, )
981-984: Constrain cross-project/workspace updates when moving existing CycleIssue rowsThis query may pull CycleIssue rows from other projects/workspaces and then reassign their cycle_id, producing inconsistent rows (project_id/workspace_id no longer match the new cycle). Restrict by project and workspace.
- cycle_issues = list( - CycleIssue.objects.filter(~Q(cycle_id=cycle_id), issue_id__in=issues) - ) + cycle_issues = list( + CycleIssue.objects.filter( + ~Q(cycle_id=cycle_id), + issue_id__in=issues, + project_id=project_id, + workspace__slug=slug, + ) + )
1228-1231: Validate target cycle existence before dereferencingnew_cycle can be None; dereferencing end_date will raise. Return a 400 with a clear error.
new_cycle = Cycle.objects.filter( workspace__slug=slug, project_id=project_id, pk=new_cycle_id ).first() + if not new_cycle: + return Response( + {"error": "New cycle not found"}, + status=status.HTTP_400_BAD_REQUEST, + )
1304-1306: Validate current cycle existence before proceedingold_cycle may be None, leading to attribute access errors later.
- old_cycle = old_cycle.first() + old_cycle = old_cycle.first() + if not old_cycle: + return Response( + {"error": "Current cycle not found"}, + status=status.HTTP_400_BAD_REQUEST, + )
♻️ Duplicate comments (7)
packages/types/package.json (1)
11-11: ESM entry added—ensuredist/index.mjsand.d.tsare actually emitted by the build.Good move pointing
"module"to ESM. However, the current build script (tsc --noEmit && tsup --minify) won’t emit type declarations unless--dts(ordts: truein tsup config) is set. Also verify tsup outputs both CJS and ESM so"main"and"module"resolve at runtime.
- Action: make tsup emit both formats and declarations (either via CLI or tsup.config.ts).
- Example (scripts entry):
"build": "tsc --noEmit && tsup src/index.ts --format cjs,esm --dts --minify".Run this to confirm artifacts exist after the build:
#!/bin/bash set -euo pipefail pnpm -w install pnpm -w --filter @plane/types build echo "Listing dist:" ls -la packages/types/dist || true # Expect these three files to exist: test -f packages/types/dist/index.js && echo "index.js OK" || (echo "index.js MISSING"; exit 1) test -f packages/types/dist/index.mjs && echo "index.mjs OK" || (echo "index.mjs MISSING"; exit 1) test -f packages/types/dist/index.d.ts && echo "index.d.ts OK" || (echo "index.d.ts MISSING"; exit 1)docker-compose-local.yml (1)
149-166: MinIO bootstrap uses ‘mc’, but minio/minio image doesn’t include the client. Startup will fail.Move mc usage to a short-lived sidecar and run the MinIO container as a pure server. This also removes the brittle sleep.
plane-minio: image: minio/minio restart: unless-stopped - # Bootstrap bucket on first run; suitable for local smoke/dev only - entrypoint: > - /bin/sh -c " - set -e - mkdir -p /export/${AWS_S3_BUCKET_NAME:-uploads}; - minio server /export --console-address ':9090' & pid=$!; - sleep 5; - mc alias set myminio http://plane-minio:9000 ${AWS_ACCESS_KEY_ID:-minioadmin} ${AWS_SECRET_ACCESS_KEY:-minioadmin}; - mc mb myminio/${AWS_S3_BUCKET_NAME:-uploads} -p || true; - wait $pid - " + command: server /export --console-address ':9090' environment: MINIO_ROOT_USER: ${AWS_ACCESS_KEY_ID:-minioadmin} MINIO_ROOT_PASSWORD: ${AWS_SECRET_ACCESS_KEY:-minioadmin} volumes: - uploads:/export ports: - "9000:9000" - "9090:9090" + +plane-minio-init: + image: minio/mc + depends_on: + plane-minio: + condition: service_started + entrypoint: > + /bin/sh -c " + set -e; + until (mc alias set myminio http://plane-minio:9000 ${AWS_ACCESS_KEY_ID:-minioadmin} ${AWS_SECRET_ACCESS_KEY:-minioadmin}); do + echo 'waiting for minio...'; sleep 2; + done; + mc mb myminio/${AWS_S3_BUCKET_NAME:-uploads} -p || true; + " + restart: "no"scripts/smoke.sh (2)
296-303: Bash error: 'local' is only valid in functions (breaks arg parsing).The case arm for --image-prefix is at the script’s top level, so
local prefix="$2"is a hard error in Bash (SC2168). Use a normal assignment.Apply this diff:
--image-prefix) - local prefix="$2" + prefix="$2" WEB_IMAGE="${prefix}plane-web:latest" SPACE_IMAGE="${prefix}plane-space:latest" ADMIN_IMAGE="${prefix}plane-admin:latest" LIVE_IMAGE="${prefix}plane-live:latest" shift 2 ;;
153-162: Parallel mode leaks containers; cleanup won’t see subshell mutations.
CONTAINERS+=("$container")happens inside backgrounded subshells (via service_job → run_and_probe), so the parent’s array remains empty andcleanupwon’t remove containers in--parallelruns. Track containers via a shared file and merge/dedupe incleanup.Apply this minimal, robust refactor:
@@ TMP_DIR="$(mktemp -d -t plane-smoke.XXXXXX)" CONTAINERS=() PIDS=() FAILURES=0 +CONTAINERS_FILE="$TMP_DIR/containers.list" @@ cleanup() { local code=$? - if [[ $KEEP -eq 0 && ${#CONTAINERS[@]} -gt 0 ]]; then - for c in "${CONTAINERS[@]}"; do - docker rm -f "$c" >/dev/null 2>&1 || true - done - fi + if [[ $KEEP -eq 0 ]]; then + # Merge in-memory and file-tracked container names, then de-dupe + local containers=("${CONTAINERS[@]}") + if [[ -f "$CONTAINERS_FILE" ]]; then + while IFS= read -r c; do containers+=("$c"); done < "$CONTAINERS_FILE" + fi + if [[ ${#containers[@]} -gt 0 ]]; then + printf "%s\n" "${containers[@]}" | awk 'NF' | sort -u | while read -r c; do + docker rm -f "$c" >/dev/null 2>&1 || true + done + fi + fi rm -rf "$TMP_DIR" >/dev/null 2>&1 || true exit $code } @@ - CONTAINERS+=("$container") + CONTAINERS+=("$container") + echo "$container" >> "$CONTAINERS_FILE"Also applies to: 213-214, 343-359, 79-83
.github/workflows/build-images.yml (1)
75-94: Step if: use secrets directly; step-local env is unavailable during if evaluation.
if: inputs.push && env.*references env vars defined within the same step, which are not available at if-eval time. Evaluatesecrets.*directly inif:.Apply this diff:
- - name: Log in to Docker Hub (optional) - if: inputs.push && env.DOCKERHUB_USERNAME != '' && env.DOCKERHUB_TOKEN != '' + - name: Log in to Docker Hub (optional) + if: ${{ inputs.push && secrets.DOCKERHUB_USERNAME != '' && secrets.DOCKERHUB_TOKEN != '' }} env: DOCKERHUB_USERNAME: ${{ secrets.DOCKERHUB_USERNAME }} DOCKERHUB_TOKEN: ${{ secrets.DOCKERHUB_TOKEN }} uses: docker/login-action@v3 with: username: ${{ env.DOCKERHUB_USERNAME }} password: ${{ env.DOCKERHUB_TOKEN }} - - name: Log in to GHCR (optional) - if: inputs.push && env.GHCR_TOKEN != '' + - name: Log in to GHCR (optional) + if: ${{ inputs.push && secrets.GHCR_TOKEN != '' }} env: GHCR_TOKEN: ${{ secrets.GHCR_TOKEN }} uses: docker/login-action@v3 with: registry: ghcr.io username: ${{ github.actor }} password: ${{ env.GHCR_TOKEN }}.github/workflows/build-branch.yml (2)
277-286: YAML flow sequence has a trailing comma; may break workflow parsing.The
needs: [ … ]list ends with a trailing comma afterbranch_build_push_proxy. Remove it or convert to block list syntax.Apply this diff:
- needs: - [ - branch_build_setup, - branch_build_push_admin, - branch_build_push_web, - branch_build_push_space, - branch_build_push_live, - branch_build_push_api, - branch_build_push_proxy, - ] + needs: + - branch_build_setup + - branch_build_push_admin + - branch_build_push_web + - branch_build_push_space + - branch_build_push_live + - branch_build_push_api + - branch_build_push_proxy
335-343: Same trailing-comma issue in another needs list.Apply the same fix here to avoid YAML parser errors.
- needs: - [ - branch_build_setup, - branch_build_push_admin, - branch_build_push_web, - branch_build_push_space, - branch_build_push_live, - branch_build_push_api, - branch_build_push_proxy, - ] + needs: + - branch_build_setup + - branch_build_push_admin + - branch_build_push_web + - branch_build_push_space + - branch_build_push_live + - branch_build_push_api + - branch_build_push_proxy
🧹 Nitpick comments (21)
packages/types/package.json (2)
16-18: Exports map looks good; consider adding"default"for wider bundler compatibility.Some bundlers and older tooling prefer/consult
"default"in conditional exports. Adding it keeps ESM-first while preserving existingrequire/importbehavior."exports": { ".": { "types": "./dist/index.d.ts", "require": "./dist/index.js", - "import": "./dist/index.mjs" + "import": "./dist/index.mjs", + "default": "./dist/index.mjs" } },
19-24:typesVersionswildcard is unnecessary and may cause odd TS resolution; remove it.You already declare
"types"at the root and in the export for".". The wildcardtypesVersionsthat maps* -> * -> dist/index.d.tscan shadow future subpath exports and confuse editors. Recommend dropping it unless you introduce multiple subpath exports.- "typesVersions": { - "*": { - "*": [ - "dist/index.d.ts" - ] - } - },docker-compose-local.yml (6)
1-3: Header comment is stale (mentions web/space/admin/live which are no longer defined).Update the header to reflect the current scope (API + worker/beat/migrator + infra) to avoid confusion.
-# Local development stack with hot reload for web/space/admin/live and Django API. -# Uses unified dev targets in plane/Dockerfile.node and plane/Dockerfile.api. +# Local development stack for Django API (plus worker, beat, migrator) with hot reload. +# Uses unified dev targets in Dockerfile.api.
29-35: Add healthchecks and gate startup on service readiness to remove brittle sleeps/races.Today, services only wait for container start, not readiness; this routinely races DB/Rabbit/Redis availability and leads to flaky boots and migrations. Add healthchecks to infra and use depends_on: condition: service_healthy for api/worker/beat/migrator.
services: api: - depends_on: - - plane-db - - plane-redis - - plane-mq - - plane-minio + depends_on: + plane-db: + condition: service_healthy + plane-redis: + condition: service_healthy + plane-mq: + condition: service_healthy + plane-minio: + condition: service_started # or service_healthy if you add a minio healthcheck worker: - depends_on: - - api - - plane-db - - plane-redis - - plane-mq + depends_on: + api: + condition: service_started + plane-db: + condition: service_healthy + plane-redis: + condition: service_healthy + plane-mq: + condition: service_healthy beat-worker: - depends_on: - - api - - plane-db - - plane-redis - - plane-mq + depends_on: + api: + condition: service_started + plane-db: + condition: service_healthy + plane-redis: + condition: service_healthy + plane-mq: + condition: service_healthy migrator: - depends_on: - - plane-db - - plane-redis - - plane-minio + depends_on: + plane-db: + condition: service_healthy + plane-redis: + condition: service_healthy + plane-minio: + condition: service_started plane-db: image: postgres:15.7-alpine restart: unless-stopped command: postgres -c 'max_connections=1000' + healthcheck: + test: ["CMD-SHELL", "pg_isready -U ${POSTGRES_USER:-plane} -d ${POSTGRES_DB:-plane}"] + interval: 2s + timeout: 5s + retries: 30 + start_period: 5s plane-redis: image: valkey/valkey:7.2.5-alpine restart: unless-stopped + healthcheck: + test: ["CMD-SHELL", "valkey-cli -h 127.0.0.1 ping | grep PONG"] + interval: 2s + timeout: 5s + retries: 30 plane-mq: image: rabbitmq:3.13.6-management-alpine restart: unless-stopped + healthcheck: + test: ["CMD", "rabbitmq-diagnostics", "-q", "ping"] + interval: 2s + timeout: 5s + retries: 30Note: If you want a MinIO healthcheck, confirm curl/wget availability in the image first; then we can add one to /minio/health/ready and flip condition to service_healthy.
Also applies to: 57-62, 83-88, 109-113, 114-128, 129-136, 137-148
17-26: DRY repeated S3/MinIO environment with YAML anchors.Four identical env blocks are easy to drift. Use an extension field and merge key to keep them consistent.
+x-minio-env: &minio_env + USE_MINIO: "1" + AWS_S3_ENDPOINT_URL: http://plane-minio:9000 + MINIO_ENDPOINT_URL: http://plane-minio:9000 + AWS_ACCESS_KEY_ID: ${AWS_ACCESS_KEY_ID:-minioadmin} + AWS_SECRET_ACCESS_KEY: ${AWS_SECRET_ACCESS_KEY:-minioadmin} + AWS_S3_BUCKET_NAME: ${AWS_S3_BUCKET_NAME:-uploads} + AWS_REGION: ${AWS_REGION:-us-east-1} + services: api: environment: - USE_MINIO: "1" - AWS_S3_ENDPOINT_URL: http://plane-minio:9000 - MINIO_ENDPOINT_URL: http://plane-minio:9000 - AWS_ACCESS_KEY_ID: ${AWS_ACCESS_KEY_ID:-minioadmin} - AWS_SECRET_ACCESS_KEY: ${AWS_SECRET_ACCESS_KEY:-minioadmin} - AWS_S3_BUCKET_NAME: ${AWS_S3_BUCKET_NAME:-uploads} - AWS_REGION: ${AWS_REGION:-us-east-1} + <<: *minio_env WEB_URL: ${WEB_URL:-http://localhost:3001} DJANGO_DEBUG_TOOLBAR: ${DJANGO_DEBUG_TOOLBAR:-0} worker: environment: - USE_MINIO: "1" - AWS_S3_ENDPOINT_URL: http://plane-minio:9000 - MINIO_ENDPOINT_URL: http://plane-minio:9000 - AWS_ACCESS_KEY_ID: ${AWS_ACCESS_KEY_ID:-minioadmin} - AWS_SECRET_ACCESS_KEY: ${AWS_SECRET_ACCESS_KEY:-minioadmin} - AWS_S3_BUCKET_NAME: ${AWS_S3_BUCKET_NAME:-uploads} - AWS_REGION: ${AWS_REGION:-us-east-1} + <<: *minio_env beat-worker: environment: - USE_MINIO: "1" - AWS_S3_ENDPOINT_URL: http://plane-minio:9000 - MINIO_ENDPOINT_URL: http://plane-minio:9000 - AWS_ACCESS_KEY_ID: ${AWS_ACCESS_KEY_ID:-minioadmin} - AWS_SECRET_ACCESS_KEY: ${AWS_SECRET_ACCESS_KEY:-minioadmin} - AWS_S3_BUCKET_NAME: ${AWS_S3_BUCKET_NAME:-uploads} - AWS_REGION: ${AWS_REGION:-us-east-1} + <<: *minio_env migrator: environment: - USE_MINIO: "1" - AWS_S3_ENDPOINT_URL: http://plane-minio:9000 - MINIO_ENDPOINT_URL: http://plane-minio:9000 - AWS_ACCESS_KEY_ID: ${AWS_ACCESS_KEY_ID:-minioadmin} - AWS_SECRET_ACCESS_KEY: ${AWS_SECRET_ACCESS_KEY:-minioadmin} - AWS_S3_BUCKET_NAME: ${AWS_S3_BUCKET_NAME:-uploads} - AWS_REGION: ${AWS_REGION:-us-east-1} + <<: *minio_envAlso applies to: 48-55, 74-81, 100-107
137-148: Expose AMQP port 5672 only if you need host access (CLI/tools); otherwise current mapping is OK.Right now only the management UI (15672) is mapped. If local host clients need to connect (not just other containers), add 5672.
plane-mq: image: rabbitmq:3.13.6-management-alpine restart: unless-stopped environment: RABBITMQ_DEFAULT_USER: ${RABBITMQ_USER:-plane} RABBITMQ_DEFAULT_PASS: ${RABBITMQ_PASSWORD:-plane} RABBITMQ_DEFAULT_VHOST: ${RABBITMQ_VHOST:-plane} volumes: - rabbitmq_data:/var/lib/rabbitmq ports: - "15672:15672" + - "5672:5672" # optional: expose AMQP for host tools
118-123: max_connections=1000 is unnecessary for local dev and can slow Postgres.Default shared buffers/sem settings won't suit 1000 connections. Either drop the override or set to a modest value (e.g., 200).
- command: postgres -c 'max_connections=1000' + # For local dev defaults are fine; raise only if truly needed + # command: postgres -c 'max_connections=200'
58-59: Workers depending on “api” is optional.If workers don’t require the API process to be up (only DB/Redis/Rabbit), you can drop that dependency to speed boot. Keep if you intentionally share code reload signaling.
Also applies to: 84-85
scripts/smoke.sh (2)
189-201: Minor: captured container ID is unused (SC2034).
cid=$(docker run …)assigns but you never usecid. Either drop the assignment or rename to_cidto signal intentional discard.- if ! cid=$(docker run -d --name "$container" -p "${port}:3000" $envflags $DOCKER_FLAGS "$image" 2>"$TMP_DIR/${container}.err"); then + if ! _cid=$(docker run -d --name "$container" -p "${port}:3000" $envflags $DOCKER_FLAGS "$image" 2>"$TMP_DIR/${container}.err"); then @@ - if ! cid=$(docker run -d --name "$container" -p "${port}:3000" $envflags $DOCKER_FLAGS "$alt_image" 2>"$TMP_DIR/${container}.err"); then + if ! _cid=$(docker run -d --name "$container" -p "${port}:3000" $envflags $DOCKER_FLAGS "$alt_image" 2>"$TMP_DIR/${container}.err"); thenAlternatively, consider
--cidfile "$TMP_DIR/${container}.cid"if you’d like to persist the ID for tooling.
135-151: Host argument isn’t applied for ss/lsof/netstat checks.
is_port_in_useacceptshostbut only uses it in the curl fallback. On multi-homed hosts this can yield false positives if some other interface is bound. Optional: constrain checks to the specific address when possible.Example tweak (ss path):
- ss -ltn 2>/dev/null | awk '{print $4}' | grep -q ":${port}\\>" && return 0 + ss -ltn 2>/dev/null | awk '{print $4}' | grep -q -E "(${host}|\\*):${port}\\>" && return 0Or switch to
ss -ltn "sport == :$port"and then filter by local addr viass -Hn '( sport = :PORT )'andgrep -F "$host:$port"..github/workflows/build-images.yml (1)
58-65: Quote $GITHUB_ENV to satisfy shellcheck (SC2086) and avoid word splitting.In the step that sets BAKE_GROUP, redirect to
"$GITHUB_ENV"with quotes.- if [ "${{ inputs.include_aio }}" = "true" ]; then - echo "BAKE_GROUP=all" >> $GITHUB_ENV - else - echo "BAKE_GROUP=default" >> $GITHUB_ENV - fi + if [ "${{ inputs.include_aio }}" = "true" ]; then + echo "BAKE_GROUP=all" >> "$GITHUB_ENV" + else + echo "BAKE_GROUP=default" >> "$GITHUB_ENV" + fi.github/workflows/build-branch.yml (2)
71-88: Quote $GITHUB_OUTPUT when appending to avoid word splitting.Multiple echos redirect to
$GITHUB_OUTPUTwithout quotes. While usually harmless, actionlint recommends quoting to avoid edge cases.Example for this block (apply similarly throughout the step):
- echo "BUILDX_DRIVER=cloud" >> $GITHUB_OUTPUT + echo "BUILDX_DRIVER=cloud" >> "$GITHUB_OUTPUT" @@ - echo "TARGET_BRANCH=$BR_NAME" >> $GITHUB_OUTPUT + echo "TARGET_BRANCH=$BR_NAME" >> "$GITHUB_OUTPUT" @@ - echo "DH_IMG_WEB=plane-frontend" >> $GITHUB_OUTPUT + echo "DH_IMG_WEB=plane-frontend" >> "$GITHUB_OUTPUT" @@ - echo "BUILD_RELEASE=${BUILD_RELEASE}" >> $GITHUB_OUTPUT + echo "BUILD_RELEASE=${BUILD_RELEASE}" >> "$GITHUB_OUTPUT"Also quote the append in “Prepare AIO Assets”:
- echo "AIO_BUILD_VERSION=${aio_version}" >> $GITHUB_OUTPUT + echo "AIO_BUILD_VERSION=${aio_version}" >> "$GITHUB_OUTPUT"Also applies to: 97-130
310-326: Version skew of reusable action is likely intentional; confirm.AIO uses
makeplane/actions/build-push@v1.1.0while others use@v1.0.0. If this is deliberate due to new features (additional-assets, etc.), ignore. Otherwise, unify to one version to reduce drift.Would you like me to submit a follow-up patch to standardize the version (or annotate why AIO uses v1.1.0)?
apps/api/plane/api/views/cycle.py (8)
174-177: OpenAPI: include the 'incomplete' view and name the query parameterThe endpoint supports an additional view ("incomplete") via the cycle_view query param but the description omits it.
- description=( - "Retrieve all cycles in a project. Supports filtering by cycle status like " - "current, upcoming, completed, or draft." - ), + description=( + "Retrieve all cycles in a project. Supports filtering via the 'cycle_view' " + "query parameter: all, current, upcoming, completed, draft, or incomplete." + ),
295-298: OpenAPI: document date validation and 409 conflict behaviorCreation enforces “both start_date and end_date provided together or both null” and returns 409 on external_id/external_source conflicts. Make that explicit.
- description=( - "Create a new development cycle with specified name, description, and date range. " - "Supports external ID tracking for integration purposes." - ), + description=( + "Create a new development cycle with specified name, description, and optional date range. " + "Both start_date and end_date must be provided together or both null. " + "Supports external ID tracking; returns 409 if a cycle with the same external_source " + "and external_id already exists." + ),
496-499: OpenAPI: mention archived cycles cannot be editedPATCH forbids editing archived cycles; only sort_order is allowed for completed ones. Reflect that.
- description=( - "Modify an existing cycle's properties like name, description, or date range. " - "Completed cycles can only have their sort order changed." - ), + description=( + "Modify an existing cycle's properties like name, description, or date range. " + "Completed cycles can only have their sort order changed. " + "Archived cycles cannot be edited." + ),
785-788: OpenAPI: clarify what “completed” means for archivingMinor doc clarity: define “completed” as end_date < now to match logic.
- description=( - "Move a completed cycle to archived status for historical tracking. " - "Only cycles that have ended can be archived." - ), + description=( + "Move a completed cycle to archived status for historical tracking. " + "Only cycles that have ended (end_date < now) can be archived." + ),
946-949: OpenAPI: call out required 'issues' fieldPOST requires a non-empty issues array; reflect that in the description.
- description=( - "Assign multiple work items to a cycle. " - "Automatically handles bulk creation and updates with activity tracking." - ), + description=( + "Assign multiple work items to a cycle. " + "Automatically handles bulk creation and updates with activity tracking. " + "Requires 'issues' (array of issue IDs) in the request body." + ),
1173-1175: OpenAPI: state target-cycle prerequisitesThe handler rejects transfers into completed cycles and implicitly assumes the target exists. Make both explicit.
- description=( - "Move incomplete work items from the current cycle to a new target cycle. " - "Captures progress snapshot and transfers only unfinished work items." - ), + description=( + "Move incomplete work items from the current cycle to a new target cycle. " + "Captures progress snapshot and transfers only unfinished work items. " + "The target cycle must exist and must not be completed." + ),
478-482: Docstring: this endpoint only retrieves a specific cycleThe docstring still says “List or retrieve cycles.” This handler only retrieves a single cycle by ID.
- """List or retrieve cycles - - Retrieve all cycles in a project or get details of a specific cycle. - Supports filtering by cycle status like current, upcoming, completed, or draft. - """ + """Retrieve cycle + + Retrieve details of a specific cycle by its ID. + """
1557-1557: Typo: serilization → serializationNot user-facing but worth correcting for consistency.
- # Label distribution serilization + # Label distribution serialization
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (62)
.github/workflows/build-branch.yml(7 hunks).github/workflows/build-images.yml(1 hunks).github/workflows/docker-smoke.yml(1 hunks).github/workflows/pull-request-build-lint-api.yml(2 hunks)CONTRIBUTING.md(1 hunks)Dockerfile.aio(1 hunks)Dockerfile.api(1 hunks)Dockerfile.node(1 hunks)apps/admin/Dockerfile.admin(0 hunks)apps/admin/Dockerfile.dev(0 hunks)apps/api/Dockerfile.api(0 hunks)apps/api/Dockerfile.dev(0 hunks)apps/api/plane/api/views/asset.py(3 hunks)apps/api/plane/api/views/cycle.py(6 hunks)apps/api/plane/api/views/intake.py(4 hunks)apps/api/plane/api/views/issue.py(4 hunks)apps/api/plane/api/views/module.py(2 hunks)apps/api/plane/api/views/project.py(1 hunks)apps/api/plane/api/views/state.py(2 hunks)apps/api/plane/app/serializers/draft.py(0 hunks)apps/api/plane/app/serializers/issue.py(0 hunks)apps/api/plane/app/serializers/project.py(0 hunks)apps/api/plane/app/views/cycle/base.py(2 hunks)apps/api/plane/app/views/page/base.py(0 hunks)apps/api/plane/app/views/project/base.py(1 hunks)apps/api/plane/app/views/project/invite.py(1 hunks)apps/api/plane/app/views/project/member.py(1 hunks)apps/api/plane/app/views/workspace/base.py(0 hunks)apps/api/plane/app/views/workspace/invite.py(1 hunks)apps/api/plane/app/views/workspace/member.py(3 hunks)apps/api/plane/authentication/views/space/password_management.py(3 hunks)apps/api/plane/authentication/views/space/signout.py(1 hunks)apps/api/plane/bgtasks/email_notification_task.py(1 hunks)apps/api/plane/bgtasks/export_task.py(1 hunks)apps/api/plane/bgtasks/notification_task.py(5 hunks)apps/api/plane/bgtasks/project_invitation_task.py(1 hunks)apps/api/plane/db/models/asset.py(1 hunks)apps/api/plane/settings/local.py(1 hunks)apps/api/plane/tests/contract/api/test_labels.py(0 hunks)apps/api/plane/tests/unit/bg_tasks/test_copy_s3_objects.py(1 hunks)apps/api/plane/tests/unit/utils/test_url.py(0 hunks)apps/api/plane/utils/content_validator.py(1 hunks)apps/api/plane/utils/url.py(1 hunks)apps/api/pyproject.toml(2 hunks)apps/api/requirements/base.txt(2 hunks)apps/live/package.json(1 hunks)apps/live/tsup.config.ts(1 hunks)apps/space/Dockerfile.dev(0 hunks)apps/space/Dockerfile.space(0 hunks)apps/web/Dockerfile.dev(0 hunks)apps/web/Dockerfile.web(0 hunks)deployments/aio/community/supervisor.conf(3 hunks)deployments/cli/community/build.yml(2 hunks)docker-bake.hcl(1 hunks)docker-compose-local.yml(1 hunks)docker-compose.yml(7 hunks)mise.toml(1 hunks)packages/editor/tsup.config.ts(1 hunks)packages/types/package.json(1 hunks)packages/ui/tsup.config.ts(1 hunks)scripts/smoke-aio.sh(1 hunks)scripts/smoke.sh(1 hunks)
💤 Files with no reviewable changes (15)
- apps/api/plane/tests/contract/api/test_labels.py
- apps/api/plane/app/views/page/base.py
- apps/api/plane/app/serializers/project.py
- apps/admin/Dockerfile.admin
- apps/api/Dockerfile.dev
- apps/api/plane/tests/unit/utils/test_url.py
- apps/space/Dockerfile.space
- apps/api/plane/app/serializers/issue.py
- apps/api/plane/app/serializers/draft.py
- apps/api/plane/app/views/workspace/base.py
- apps/admin/Dockerfile.dev
- apps/api/Dockerfile.api
- apps/web/Dockerfile.dev
- apps/web/Dockerfile.web
- apps/space/Dockerfile.dev
✅ Files skipped from review due to trivial changes (1)
- apps/api/plane/bgtasks/notification_task.py
🚧 Files skipped from review as they are similar to previous changes (40)
- apps/api/plane/bgtasks/project_invitation_task.py
- apps/api/plane/api/views/intake.py
- CONTRIBUTING.md
- apps/api/pyproject.toml
- apps/api/plane/app/views/project/base.py
- apps/api/plane/app/views/cycle/base.py
- apps/api/plane/db/models/asset.py
- .github/workflows/pull-request-build-lint-api.yml
- apps/api/plane/utils/content_validator.py
- deployments/aio/community/supervisor.conf
- apps/api/plane/app/views/workspace/member.py
- apps/api/requirements/base.txt
- apps/api/plane/app/views/project/invite.py
- apps/api/plane/api/views/module.py
- apps/api/plane/app/views/project/member.py
- apps/api/plane/authentication/views/space/signout.py
- apps/api/plane/app/views/workspace/invite.py
- Dockerfile.node
- apps/api/plane/tests/unit/bg_tasks/test_copy_s3_objects.py
- mise.toml
- apps/live/tsup.config.ts
- apps/api/plane/authentication/views/space/password_management.py
- Dockerfile.api
- apps/api/plane/api/views/issue.py
- packages/editor/tsup.config.ts
- apps/live/package.json
- apps/api/plane/settings/local.py
- packages/ui/tsup.config.ts
- apps/api/plane/api/views/state.py
- apps/api/plane/bgtasks/email_notification_task.py
- apps/api/plane/utils/url.py
- scripts/smoke-aio.sh
- apps/api/plane/api/views/project.py
- docker-bake.hcl
- Dockerfile.aio
- apps/api/plane/api/views/asset.py
- .github/workflows/docker-smoke.yml
- deployments/cli/community/build.yml
- apps/api/plane/bgtasks/export_task.py
- docker-compose.yml
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/build-images.yml
59-59: shellcheck reported issue in this script: SC2086:info:2:28: Double quote to prevent globbing and word splitting
(shellcheck)
59-59: shellcheck reported issue in this script: SC2086:info:4:32: Double quote to prevent globbing and word splitting
(shellcheck)
🪛 Shellcheck (0.10.0)
scripts/smoke.sh
[warning] 200-200: cid appears unused. Verify use (or export if used externally).
(SC2034)
[error] 297-297: 'local' is only valid in functions.
(SC2168)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build and smoke test admin
- GitHub Check: Build and smoke test web
- GitHub Check: Build and smoke test space
- GitHub Check: Build and smoke test AIO
- GitHub Check: Build and lint web apps
🔇 Additional comments (5)
docker-compose-local.yml (1)
129-135: Confirm Valkey compatibility with your Python stack and prod parity.If prod is Redis, using Valkey locally can mask behavior differences (modules, RESP3 features). Celery/redis-py generally work, but verify feature use.
Would you like a quick script to grep for Redis module usage (streams, JSON, etc.) in apps/api and flag risky areas?
scripts/smoke.sh (1)
1-16: Solid, portable smoke harness.Nice job on the ergonomics (colored logs, retries, early-log surfacing, parallel mode) and the safety rails (pre-flight checks, port checks, cleanup on traps). This will be very useful both locally and in CI.
.github/workflows/build-branch.yml (2)
151-201: Shared Dockerfile.node usage looks correct and consistent across admin/web/space.Using
dockerfile-path: ./Dockerfile.nodewithAPP_SCOPEbuild-arg for admin/web/space matches the consolidated Dockerfile approach and root build context.
244-246: Root-level Dockerfile.api adoption looks good.Adopting
dockerfile-path: ./Dockerfile.apiwith root context aligns with the bake targets and unifies the build context.apps/api/plane/api/views/cycle.py (1)
1423-1430: No issues found: burndown_plot correctly expects a model instance
The definition of burndown_plot in apps/api/plane/utils/analytics_plot.py uses the passed-in object’s attributes (e.g.queryset.total_issues), confirming it operates on a model instance rather than iterating a Django QuerySet. In the reviewed code,old_cycleis obtained via.first(), so it is indeed a model instance. All current call sites supply instances, so no change is required.
lifeiscontent
force-pushed
the
feat/fix-local-docker-dev
branch
3 times, most recently
from
92a6c70 to
1e44fbc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/plane/api/views/module.py (1)
694-733: Bug: incorrect membership check (str(UUID) vs UUID) causes missed updates and potential duplicatesInside add_module_work_items, existing ModuleIssue detection compares str(module_issue.issue_id) to a queryset of UUIDs, which never matches, so moves are skipped and creates are attempted (later ignored due to ignore_conflicts). Fix by normalizing types and using an O(1) lookup.
Apply this diff:
- issues = Issue.objects.filter( + issues = Issue.objects.filter( workspace__slug=slug, project_id=project_id, pk__in=issues - ).values_list("id", flat=True) + ).values_list("id", flat=True) + + # Normalize to a set of UUIDs for O(1) membership checks + issue_ids = set(issues) - module_issues = list(ModuleIssue.objects.filter(issue_id__in=issues)) + module_issues = list(ModuleIssue.objects.filter(issue_id__in=issue_ids)) + module_issue_by_issue_id = {mi.issue_id: mi for mi in module_issues} update_module_issue_activity = [] records_to_update = [] record_to_create = [] - for issue in issues: - module_issue = [ - module_issue - for module_issue in module_issues - if str(module_issue.issue_id) in issues - ] - - if len(module_issue): - if module_issue[0].module_id != module_id: - update_module_issue_activity.append( - { - "old_module_id": str(module_issue[0].module_id), - "new_module_id": str(module_id), - "issue_id": str(module_issue[0].issue_id), - } - ) - module_issue[0].module_id = module_id - records_to_update.append(module_issue[0]) - else: - record_to_create.append( - ModuleIssue( - module=module, - issue_id=issue, - project_id=project_id, - workspace=module.workspace, - created_by=request.user, - updated_by=request.user, - ) - ) + for issue_id in issue_ids: + existing = module_issue_by_issue_id.get(issue_id) + if existing: + if existing.module_id != module_id: + update_module_issue_activity.append( + { + "old_module_id": str(existing.module_id), + "new_module_id": str(module_id), + "issue_id": str(existing.issue_id), + } + ) + existing.module_id = module_id + records_to_update.append(existing) + else: + record_to_create.append( + ModuleIssue( + module=module, + issue_id=issue_id, + project_id=project_id, + workspace=module.workspace, + created_by=request.user, + updated_by=request.user, + ) + )
♻️ Duplicate comments (3)
packages/types/package.json (1)
11-11: ESM entrypoint switched to .mjs — verify dist emits both ESM and CJS (plus .d.ts).The change is correct for dual export. Please confirm the build actually outputs dist/index.mjs, dist/index.js, and dist/index.d.ts. If not already handled via tsup.config, consider making formats and dts explicit in scripts.
Apply this diff (only if you’re not already setting these in tsup.config):
- "dev": "tsup --watch", - "build": "tsc --noEmit && tsup --minify", + "dev": "tsup src/index.ts --format cjs,esm --dts --watch", + "build": "tsc --noEmit && tsup src/index.ts --format cjs,esm --dts --minify",Run to verify artifacts:
#!/bin/bash pnpm -w --filter @plane/types build ls -l packages/types/dist/ test -f packages/types/dist/index.js && echo "index.js OK" || echo "index.js MISSING" test -f packages/types/dist/index.mjs && echo "index.mjs OK" || echo "index.mjs MISSING" test -f packages/types/dist/index.d.ts && echo "index.d.ts OK" || echo "index.d.ts MISSING"scripts/smoke.sh (1)
301-308: Fix: remove invalidlocalat top level in case branch.
localis only valid inside functions; using it here breaks the script (ShellCheck SC2168). The earlier bot comment flagged this already.Apply this diff:
--image-prefix) - local prefix="$2" + prefix="$2" WEB_IMAGE="${prefix}plane-web:latest" SPACE_IMAGE="${prefix}plane-space:latest" ADMIN_IMAGE="${prefix}plane-admin:latest" LIVE_IMAGE="${prefix}plane-live:latest" shift 2 ;;docker-compose-local.yml (1)
149-171: Fix: MinIO bootstrap usesmc, but theminio/minioimage doesn’t ship the client.The entrypoint will fail on
mc alias setandmc mb. Use a short-lived sidecar with minio/mc (or awscli) to initialize the bucket; keep the MinIO container running the server only. This was flagged earlier but still present here.Apply this refactor:
plane-minio: image: minio/minio restart: unless-stopped - # Bootstrap bucket on first run; suitable for local smoke/dev only - entrypoint: > - /bin/sh -c " - set -e - mkdir -p /export/${AWS_S3_BUCKET_NAME:-uploads}; - minio server /export --console-address ':9090' & pid=$!; - sleep 5; - mc alias set myminio http://plane-minio:9000 ${AWS_ACCESS_KEY_ID:-minioadmin} ${AWS_SECRET_ACCESS_KEY:-minioadmin}; - mc mb myminio/${AWS_S3_BUCKET_NAME:-uploads} -p || true; - wait $pid - " + command: server /export --console-address ':9090' environment: MINIO_ROOT_USER: ${AWS_ACCESS_KEY_ID:-minioadmin} MINIO_ROOT_PASSWORD: ${AWS_SECRET_ACCESS_KEY:-minioadmin} volumes: - uploads:/export ports: - "9000:9000" - "9090:9090" + + plane-minio-init: + image: minio/mc + depends_on: + - plane-minio + restart: "no" + entrypoint: > + /bin/sh -c " + set -e; + until (mc alias set myminio http://plane-minio:9000 ${AWS_ACCESS_KEY_ID:-minioadmin} ${AWS_SECRET_ACCESS_KEY:-minioadmin}); do + echo 'waiting for minio...'; sleep 2; + done; + mc mb myminio/${AWS_S3_BUCKET_NAME:-uploads} -p || true; + "
🧹 Nitpick comments (11)
packages/types/package.json (2)
16-18: Add "default" in conditional exports for broader tooling compatibility.Some bundlers/resolvers prefer the "default" condition alongside "import"/"require". Safe and non-breaking.
"types": "./dist/index.d.ts", "require": "./dist/index.js", - "import": "./dist/index.mjs" + "import": "./dist/index.mjs", + "default": "./dist/index.mjs"
19-23: Overbroad typesVersions may mask invalid subpath imports; recommend removing.Mapping "*" → "dist/index.d.ts" can make TS accept subpath imports that will fail at runtime (not exported). Since you already set "types": "./dist/index.d.ts", this block is unnecessary and risky.
- "typesVersions": { - "*": { - "*": [ - "dist/index.d.ts" - ] - } - },If you do intend to expose subpaths, add matching runtime exports for them instead of funneling all types to index.d.ts.
scripts/smoke.sh (4)
195-217: Tidy: drop unusedcidcapture to silence SC2034 and simplify.
cidisn’t used afterdocker run. Capture only stderr to the error file and avoid assigning to an unused var.- if ! cid=$(docker run -d --name "$container" -p "${port}:3000" $envflags $DOCKER_FLAGS "$image" 2>"$TMP_DIR/${container}.err"); then + if ! docker run -d --name "$container" -p "${port}:3000" $envflags $DOCKER_FLAGS "$image" 2>"$TMP_DIR/${container}.err"; then @@ - if ! cid=$(docker run -d --name "$container" -p "${port}:3000" $envflags $DOCKER_FLAGS "$alt_image" 2>"$TMP_DIR/${container}.err"); then + if ! docker run -d --name "$container" -p "${port}:3000" $envflags $DOCKER_FLAGS "$alt_image" 2>"$TMP_DIR/${container}.err"; then
82-84: Tidy: remove unused array to address SC2034.
CONTAINERSis no longer used after switching to a file-based tracker. Drop the declaration.-CONTAINERS=() PIDS=() FAILURES=0
137-153: Optional: respect host when checking port occupancy.
is_port_in_useignores thehostargument for ss/lsof/netstat and may false-positive when another interface is listening. Consider probing the specific host (e.g., 127.0.0.1 vs ::) or fall back to a host-targeted TCP connect.Minimal change example:
- if exists_cmd ss; then - ss -ltn 2>/dev/null | awk '{print $4}' | grep -q ":${port}\\>" && return 0 + if exists_cmd ss; then + ss -H -ltn 2>/dev/null | awk '{print $4}' | grep -Eq "(^|[[:space:]])(${host//./\\.}|\\*|0\\.0\\.0\\.0|\\[::\\]|::):${port}\$" && return 0Or simply keep the current checks and rely solely on the curl probe with the provided host (cleaner, fewer platform quirks).
225-231: Optional: accept any 2xx status, not only 200.Some services may return 204/206 on health paths. If that’s acceptable, broaden the check.
- if [[ "$status" == "200" ]]; then + if [[ "$status" =~ ^2[0-9][0-9]$ ]]; then.github/workflows/docker-smoke.yml (1)
141-160: Optional: treat any 2xx as success and shorten failure loops with logs-on-first-error.Minor UX improvement; current logic is fine as-is.
- if [ "${STATUS}" = "200" ]; then + if [[ "${STATUS}" =~ ^2[0-9][0-9]$ ]]; thendocker-bake.hcl (1)
4-5: Update comment to reflect new file layout.The Dockerfiles now live at repo root (e.g., Dockerfile.node, Dockerfile.api). The comment still references “under plane/”.
-# Uses unified Dockerfiles under plane/ for Node (Next.js) apps and API. +# Uses unified Dockerfiles at the repo root (Dockerfile.node, Dockerfile.api).docker-compose-local.yml (1)
29-35: Optional: add healthchecks and depend-on health to avoid race during first run.Compose’s
depends_ondoesn’t wait for readiness. Healthchecks for Postgres/Redis/RabbitMQ/MinIO can reduce flaky first boots; API/worker can then depend oncondition: service_healthy.If you want, I can add minimal healthchecks for these services tailored to the images in use.
Also applies to: 57-62, 83-88, 109-113
apps/api/plane/api/views/module.py (2)
742-744: Make activity payload machine-friendlySerializing str(issues) yields an unhelpful QuerySet representation. Emit concrete IDs.
- requested_data=json.dumps({"modules_list": str(issues)}), + requested_data=json.dumps({"modules_list": [str(i) for i in issue_ids]}),
738-739: Nit: bulk_update field nameYou mutate module_id on the instances; using ["module_id"] is clearer (functionally equivalent).
- ModuleIssue.objects.bulk_update(records_to_update, ["module"], batch_size=10) + ModuleIssue.objects.bulk_update(records_to_update, ["module_id"], batch_size=10)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (62)
.github/workflows/build-branch.yml(7 hunks).github/workflows/docker-smoke-aio.yml(1 hunks).github/workflows/docker-smoke.yml(1 hunks).github/workflows/pull-request-build-lint-api.yml(2 hunks)CONTRIBUTING.md(1 hunks)Dockerfile.aio(1 hunks)Dockerfile.api(1 hunks)Dockerfile.node(1 hunks)apps/admin/Dockerfile.admin(0 hunks)apps/admin/Dockerfile.dev(0 hunks)apps/api/Dockerfile.api(0 hunks)apps/api/Dockerfile.dev(0 hunks)apps/api/plane/api/views/asset.py(3 hunks)apps/api/plane/api/views/cycle.py(6 hunks)apps/api/plane/api/views/intake.py(4 hunks)apps/api/plane/api/views/issue.py(4 hunks)apps/api/plane/api/views/module.py(2 hunks)apps/api/plane/api/views/project.py(1 hunks)apps/api/plane/api/views/state.py(2 hunks)apps/api/plane/app/serializers/draft.py(0 hunks)apps/api/plane/app/serializers/issue.py(0 hunks)apps/api/plane/app/serializers/project.py(0 hunks)apps/api/plane/app/views/cycle/base.py(2 hunks)apps/api/plane/app/views/page/base.py(0 hunks)apps/api/plane/app/views/project/base.py(1 hunks)apps/api/plane/app/views/project/invite.py(1 hunks)apps/api/plane/app/views/project/member.py(1 hunks)apps/api/plane/app/views/workspace/base.py(0 hunks)apps/api/plane/app/views/workspace/invite.py(1 hunks)apps/api/plane/app/views/workspace/member.py(3 hunks)apps/api/plane/authentication/views/space/password_management.py(3 hunks)apps/api/plane/authentication/views/space/signout.py(1 hunks)apps/api/plane/bgtasks/email_notification_task.py(1 hunks)apps/api/plane/bgtasks/export_task.py(1 hunks)apps/api/plane/bgtasks/notification_task.py(5 hunks)apps/api/plane/bgtasks/project_invitation_task.py(1 hunks)apps/api/plane/db/models/asset.py(1 hunks)apps/api/plane/settings/local.py(1 hunks)apps/api/plane/tests/contract/api/test_labels.py(0 hunks)apps/api/plane/tests/unit/bg_tasks/test_copy_s3_objects.py(1 hunks)apps/api/plane/tests/unit/utils/test_url.py(0 hunks)apps/api/plane/utils/content_validator.py(1 hunks)apps/api/plane/utils/url.py(1 hunks)apps/api/pyproject.toml(2 hunks)apps/api/requirements/base.txt(2 hunks)apps/live/package.json(1 hunks)apps/live/tsup.config.ts(1 hunks)apps/space/Dockerfile.dev(0 hunks)apps/space/Dockerfile.space(0 hunks)apps/web/Dockerfile.dev(0 hunks)apps/web/Dockerfile.web(0 hunks)deployments/aio/community/supervisor.conf(3 hunks)deployments/cli/community/build.yml(2 hunks)docker-bake.hcl(1 hunks)docker-compose-local.yml(1 hunks)docker-compose.yml(7 hunks)mise.toml(1 hunks)packages/editor/tsup.config.ts(1 hunks)packages/types/package.json(1 hunks)packages/ui/tsup.config.ts(1 hunks)scripts/smoke-aio.sh(1 hunks)scripts/smoke.sh(1 hunks)
💤 Files with no reviewable changes (15)
- apps/api/plane/app/views/page/base.py
- apps/api/plane/app/serializers/issue.py
- apps/web/Dockerfile.dev
- apps/admin/Dockerfile.admin
- apps/api/Dockerfile.dev
- apps/web/Dockerfile.web
- apps/api/plane/app/views/workspace/base.py
- apps/api/plane/tests/contract/api/test_labels.py
- apps/admin/Dockerfile.dev
- apps/api/Dockerfile.api
- apps/api/plane/tests/unit/utils/test_url.py
- apps/space/Dockerfile.space
- apps/space/Dockerfile.dev
- apps/api/plane/app/serializers/draft.py
- apps/api/plane/app/serializers/project.py
✅ Files skipped from review due to trivial changes (9)
- apps/api/plane/api/views/cycle.py
- apps/api/plane/tests/unit/bg_tasks/test_copy_s3_objects.py
- apps/api/plane/authentication/views/space/password_management.py
- apps/api/plane/utils/url.py
- CONTRIBUTING.md
- apps/api/plane/app/views/project/member.py
- apps/api/plane/authentication/views/space/signout.py
- apps/api/plane/app/views/project/invite.py
- apps/api/plane/api/views/project.py
🚧 Files skipped from review as they are similar to previous changes (31)
- apps/live/tsup.config.ts
- apps/api/plane/app/views/workspace/invite.py
- apps/api/pyproject.toml
- Dockerfile.aio
- packages/ui/tsup.config.ts
- apps/api/plane/api/views/intake.py
- apps/api/plane/db/models/asset.py
- apps/api/plane/bgtasks/project_invitation_task.py
- apps/api/plane/api/views/asset.py
- apps/api/plane/app/views/cycle/base.py
- Dockerfile.api
- apps/api/plane/app/views/project/base.py
- apps/api/plane/settings/local.py
- apps/api/plane/bgtasks/export_task.py
- .github/workflows/pull-request-build-lint-api.yml
- apps/live/package.json
- deployments/aio/community/supervisor.conf
- Dockerfile.node
- apps/api/plane/utils/content_validator.py
- apps/api/plane/bgtasks/email_notification_task.py
- apps/api/plane/app/views/workspace/member.py
- apps/api/plane/api/views/state.py
- apps/api/plane/bgtasks/notification_task.py
- apps/api/requirements/base.txt
- packages/editor/tsup.config.ts
- mise.toml
- docker-compose.yml
- apps/api/plane/api/views/issue.py
- deployments/cli/community/build.yml
- scripts/smoke-aio.sh
- .github/workflows/build-branch.yml
🧰 Additional context used
🪛 Shellcheck (0.10.0)
scripts/smoke.sh
[warning] 82-82: CONTAINERS appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 205-205: cid appears unused. Verify use (or export if used externally).
(SC2034)
[error] 302-302: 'local' is only valid in functions.
(SC2168)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Build and smoke test AIO
- GitHub Check: Build and smoke test web
- GitHub Check: Build and smoke test space
- GitHub Check: Build and smoke test live
- GitHub Check: Build and smoke test admin
- GitHub Check: Analyze (javascript)
- GitHub Check: Build and lint web apps
🔇 Additional comments (10)
.github/workflows/docker-smoke-aio.yml (1)
112-115: Good: bake load into daemon for local smoke.Using
--loadensures the AIO image is available todocker runwithout needing a registry push. LGTM..github/workflows/docker-smoke.yml (2)
72-99: Matrix construction looks robust and readable.Selective builds keyed off changed paths with a sane default to web keeps CI fast. The env-flags mechanism for live is a nice touch.
124-135: Build contexts and Dockerfile paths look correct post-move.Root-level context plus Dockerfile.node for Next.js apps and apps/live/Dockerfile.live for live is appropriate.
docker-bake.hcl (2)
107-114: Proxy context and dockerfile path look reasonable.Assuming apps/proxy/Dockerfile.ce exists, this target is consistent with the rest of the bake.
Would you like me to scan the repo to confirm the presence of apps/proxy/Dockerfile.ce and surface its last change for traceability?
116-132: Nice: AIO composes via target contexts.Using
contexts = { web_ctx = "target:web", ... }is clean and keeps AIO aligned with per-target outputs.docker-compose-local.yml (2)
8-35: Good: dev target for API with bind mount and explicit entrypoint.This setup should give tight feedback loops for local work. Env wiring to MinIO/RabbitMQ/Redis looks correct.
114-148: Infra versions and volumes look sane for local.Pinned images with persistent volumes and exposed ports are appropriate for a local stack.
apps/api/plane/api/views/module.py (3)
13-13: Minimal drf-spectacular imports look goodKeeping only OpenApiRequest/OpenApiResponse is correct here; both are used below and there are no unused imports.
41-71: Confirm path params are still surfaced in the OpenAPI schemaThe per-endpoint parameter lists don’t include path params like slug/project_id. That’s fine if your URL patterns define them and drf-spectacular auto-infers path parameters. Please verify the generated schema still documents these; otherwise, reintroduce explicit OpenApiParameter entries for them.
658-661: Multi-line description via parentheses: LGTMThis keeps the description readable without changing behavior.
lifeiscontent
force-pushed
the
feat/fix-local-docker-dev
branch
from
1e44fbc to
0dee2b5
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (7)
scripts/smoke-aio.sh (1)
215-219: Nice: robust docker flag handling and curl timeouts are in placeParsing DOCKER_FLAGS into an array and adding conservative curl timeouts address common CI flakiness. This aligns with earlier feedback.
Also applies to: 278-279
Dockerfile.api (1)
39-55: Good catch: builder stage now includes headers for lxml/cryptography/xmlsecThese dev packages are required on Alpine for many Python wheels; this avoids brittle source builds during CI.
.github/workflows/build-branch.yml (1)
373-381: Fix trailing comma in needs: flow sequence (GitHub YAML may reject it)The inline list includes a trailing comma. Convert to block list to be safe.
- needs: - [ - branch_build_setup, - branch_build_push_admin, - branch_build_push_web, - branch_build_push_space, - branch_build_push_live, - branch_build_push_api, - branch_build_push_proxy, - ] + needs: + - branch_build_setup + - branch_build_push_admin + - branch_build_push_web + - branch_build_push_space + - branch_build_push_live + - branch_build_push_api + - branch_build_push_proxyscripts/smoke.sh (1)
301-308: Bash error: ‘local’ at top-level breaks the script (SC2168)local is only valid within functions. This will exit early on many shells.
- --image-prefix) - local prefix="$2" + --image-prefix) + prefix="$2" WEB_IMAGE="${prefix}plane-web:latest" SPACE_IMAGE="${prefix}plane-space:latest" ADMIN_IMAGE="${prefix}plane-admin:latest" LIVE_IMAGE="${prefix}plane-live:latest" shift 2 ;;.github/workflows/docker-smoke.yml (1)
16-19: Add missing path triggers for Dockerfile.aio and docker-bake.hclThe workflow’s
on.pull_request.pathsandon.push.pathsblocks currently include onlyDockerfile.nodeandDockerfile.api, but omitDockerfile.aioanddocker-bake.hcl. Since yourdorny/paths-filter“common” filter already includes them, changes to those files won’t trigger this workflow as intended. Please update both path lists.• In the
pull_request.pathssection (around lines 16–19):- "Dockerfile.node" - "Dockerfile.api" + - "Dockerfile.aio" + - "docker-bake.hcl"• In the
push.pathssection (around lines 31–34):- "Dockerfile.node" - "Dockerfile.api" + - "Dockerfile.aio" + - "docker-bake.hcl"Dockerfile.aio (1)
6-21: Header instructions are incorrect: this Dockerfile uses BuildKit contexts, not build-argsThe example shows --build-arg WEB_IMG=... and plane/Dockerfile.aio path, which doesn’t match the actual implementation (COPY --from= and root-level Dockerfile.aio).
Apply this doc fix:
-# Build requirements: -# - Build and tag the per-app images first (or override args below): -# plane-web:latest, plane-space:latest, plane-admin:latest, plane-live:latest, -# plane-api:latest, plane-proxy:latest -# -# Example: -# docker build \ -# -f plane/Dockerfile.aio \ -# --build-arg WEB_IMG=plane-web:latest \ -# --build-arg SPACE_IMG=plane-space:latest \ -# --build-arg ADMIN_IMG=plane-admin:latest \ -# --build-arg LIVE_IMG=plane-live:latest \ -# --build-arg API_IMG=plane-api:latest \ -# --build-arg PROXY_IMG=plane-proxy:latest \ -# -t plane-aio:latest . +# Build requirements: +# - Buildx/BuildKit with support for external contexts. +# - Preferred: docker buildx bake -f docker-bake.hcl aio +# - Or direct build with explicit contexts: +# docker build \ +# -f Dockerfile.aio \ +# --build-context web_ctx=target:web \ +# --build-context space_ctx=target:space \ +# --build-context admin_ctx=target:admin \ +# --build-context live_ctx=target:live \ +# --build-context api_ctx=target:api \ +# --build-context proxy_ctx=target:proxy \ +# -t plane-aio:latest .docker-compose-local.yml (1)
149-171: MinIO bootstrap uses ‘mc’ inside minio/minio image — mc isn’t available; bootstrap will failThe minio/minio image doesn’t ship the mc client. Move bootstrap to a short-lived sidecar and keep minio running the server.
Apply this refactor:
plane-minio: image: minio/minio restart: unless-stopped - # Bootstrap bucket on first run; suitable for local smoke/dev only - entrypoint: > - /bin/sh -c " - set -e - mkdir -p /export/${AWS_S3_BUCKET_NAME:-uploads}; - minio server /export --console-address ':9090' & pid=$!; - sleep 5; - mc alias set myminio http://plane-minio:9000 ${AWS_ACCESS_KEY_ID:-minioadmin} ${AWS_SECRET_ACCESS_KEY:-minioadmin}; - mc mb myminio/${AWS_S3_BUCKET_NAME:-uploads} -p || true; - wait $pid - " + command: server /export --console-address ':9090' environment: MINIO_ROOT_USER: ${AWS_ACCESS_KEY_ID:-minioadmin} MINIO_ROOT_PASSWORD: ${AWS_SECRET_ACCESS_KEY:-minioadmin} volumes: - uploads:/export ports: - "9000:9000" - "9090:9090" + + plane-minio-init: + image: minio/mc + depends_on: + - plane-minio + entrypoint: > + /bin/sh -c " + set -e; + until (mc alias set myminio http://plane-minio:9000 ${AWS_ACCESS_KEY_ID:-minioadmin} ${AWS_SECRET_ACCESS_KEY:-minioadmin}); do echo 'waiting for minio...'; sleep 2; done; + mc mb myminio/${AWS_S3_BUCKET_NAME:-uploads} -p || true; + " + restart: "no"
🧹 Nitpick comments (17)
apps/live/tsup.config.ts (3)
6-6: Do you need dts for a server app?Generating .d.ts for a Node server entry usually adds build time and artifacts you won’t publish. If Live isn’t shipped as a library, consider disabling it:
- dts: true, + dts: false,
7-9: Tighten noExternal matchingUsing both "@plane/editor" and "@plane/editor/lib" is brittle. A single regex covers the base package and subpaths and avoids misses if more subpath imports appear later:
- noExternal: ["@plane/editor", "@plane/editor/lib"], + noExternal: [/^@plane\/editor(\/.*)?$/],
7-9: Bundle Editor Without React Peer DependenciesWe’ve confirmed via
packages/editor/package.jsonthatreactandreact-domare listed as peerDependencies of@plane/editor. Leaving@plane/editorundernoExternalwill cause those peers to be bundled into the server output. To prevent this, update your bundler config inapps/live/tsup.config.ts(around lines 7–9) as follows:- noExternal: ["@plane/editor", "@plane/editor/lib"], - external: ["@plane/ui", "@plane/utils", "@plane/hooks"], + // Bundle @plane/editor (but exclude its peer deps) + noExternal: [/^@plane\/editor(\/.*)?$/], + external: ["react", "react-dom", "@plane/ui", "@plane/utils", "@plane/hooks"],• File: apps/live/tsup.config.ts
• Lines: ~7–9This ensures
@plane/editoritself is bundled while keepingreactandreact-domexternal.apps/api/plane/settings/local.py (2)
9-18: Narrow the exception and expose an explicit “enabled” flag for URLs to consumeCatching ImportError is safer and avoids masking unrelated config errors. Also, surfacing a boolean helps urls.py toggle debug routes cleanly.
Apply:
# Debug Toolbar settings (optional via DJANGO_DEBUG_TOOLBAR=1) DEBUG_TOOLBAR_PATCH_SETTINGS = False +DEBUG_TOOLBAR_ENABLED = False if os.environ.get("DJANGO_DEBUG_TOOLBAR", "0") == "1": try: import debug_toolbar # noqa: F401 INSTALLED_APPS += ("debug_toolbar",) # noqa - MIDDLEWARE += ("debug_toolbar.middleware.DebugToolbarMiddleware",) # noqa - except Exception: + MIDDLEWARE += ("debug_toolbar.middleware.DebugToolbarMiddleware",) # noqa + DEBUG_TOOLBAR_ENABLED = True + except ImportError: # If debug_toolbar isn't installed, skip enabling it passFollow-up:
- In apps/api/plane/urls.py, guard the toolbar URLs with this flag:
# apps/api/plane/urls.py from django.conf import settings from django.urls import include, path if getattr(settings, "DEBUG_TOOLBAR_ENABLED", False): import debug_toolbar urlpatterns = [path("__debug__/", include(debug_toolbar.urls))] + urlpatterns
15-15: Consider placing Debug Toolbar middleware at the start of MIDDLEWAREDocs recommend it be as early as possible to reliably intercept responses. If MIDDLEWARE is a tuple (as implied), prepend instead of appending.
- MIDDLEWARE += ("debug_toolbar.middleware.DebugToolbarMiddleware",) # noqa + MIDDLEWARE = ("debug_toolbar.middleware.DebugToolbarMiddleware",) + MIDDLEWARE # noqaNote: If MIDDLEWARE is a list in .common, use insert(0, "...") instead.
scripts/smoke-aio.sh (1)
202-204: Add a daemon readiness check to catch Docker socket issues earlyexists_cmd docker is necessary but not sufficient; docker can be installed yet the daemon unavailable. A quick docker info improves diagnostics.
exists_cmd docker || { log_err "docker is required"; exit 1; } exists_cmd curl || { log_err "curl is required"; exit 1; } +if ! docker info >/dev/null 2>&1; then + log_err "docker daemon is not available (is it running, and do we have permissions?)" + exit 1 +fiDockerfile.api (1)
9-10: Fix header examples to reflect root-level Dockerfile pathComments still reference plane/Dockerfile.api which can confuse contributors.
-# docker build -f plane/Dockerfile.api --target dev -t plane-api:dev . -# docker build -f plane/Dockerfile.api --target runtime -t plane-api:latest . +# docker build -f Dockerfile.api --target dev -t plane-api:dev . +# docker build -f Dockerfile.api --target runtime -t plane-api:latest .scripts/smoke.sh (4)
226-228: Add curl timeouts to avoid indefinite hangsA single endpoint can stall the entire smoke; add conservative connect and total timeouts.
- status="$(curl -sS -o /dev/null -w "%{http_code}" -L "$url" 2>"$errfile" || true)" + status="$(curl --connect-timeout 5 --max-time 10 -sS -o /dev/null -w "%{http_code}" -L "$url" 2>"$errfile" || true)"
333-334: Parse DOCKER_FLAGS into an array for safe passing to docker runString expansion can mis-handle quoting; array preserves argv fidelity.
log " Docker flags: ${DOCKER_FLAGS:-<none>}" +# Parse DOCKER_FLAGS string into an array for safe docker invocation +if [[ -n "${DOCKER_FLAGS:-}" ]]; then + read -r -a DOCKER_FLAGS_ARRAY <<< "$DOCKER_FLAGS" +else + DOCKER_FLAGS_ARRAY=() +fi
194-206: Use array expansion when passing extra docker flags (mirrors smoke-aio.sh)Prevents flags from being concatenated into a single argv.
- if ! cid=$(docker run -d --name "$container" -p "${port}:3000" $envflags $DOCKER_FLAGS "$image" 2>"$TMP_DIR/${container}.err"); then + if ! cid=$(docker run -d --name "$container" -p "${port}:3000" $envflags "${DOCKER_FLAGS_ARRAY[@]}" "$image" 2>"$TMP_DIR/${container}.err"); then @@ - if ! cid=$(docker run -d --name "$container" -p "${port}:3000" $envflags $DOCKER_FLAGS "$alt_image" 2>"$TMP_DIR/${container}.err"); then + if ! cid=$(docker run -d --name "$container" -p "${port}:3000" $envflags "${DOCKER_FLAGS_ARRAY[@]}" "$alt_image" 2>"$TMP_DIR/${container}.err"); then
82-83: Remove unused variable or add a noteCONTAINERS is no longer used after switching to file-based tracking. Trim it to avoid confusion.
-CONTAINERS=() PIDS=().github/workflows/docker-smoke.yml (1)
85-93: Matrix field bake_target is never used; either wire it up, or drop itYou compute bake_target but only use docker build. This is confusing and can drift. Either add a buildx bake step keyed to bake_target or remove the field.
Option A (use bake):
- const add = (name, bake_target, image, container, port, path, env_flags = "") => - include.push({ name, bake_target, image, container, host_port: port, path, env_flags }); + const add = (name, bake_target, image, container, port, path, env_flags = "") => + include.push({ name, bake_target, image, container, host_port: port, path, env_flags }); @@ - name: Prepare build environment - run: echo "Using docker build (no buildx bake)" + run: | + echo "Using docker buildx bake when bake_target is set; otherwise docker build." + - name: Build image with bake (${{ matrix.name }}) + if: ${{ matrix.bake_target != '' }} + run: docker buildx bake -f docker-bake.hcl ${{ matrix.bake_target }} --set *.platform=linux/amd64 + + - name: Build image with docker build (${{ matrix.name }}) + if: ${{ matrix.bake_target == '' }} shell: bash run: | set -euo pipefail name="${{ matrix.name }}" tag="${{ matrix.image }}" if [ "$name" = "live" ]; then docker build -f "apps/live/Dockerfile.live" -t "$tag" "." else docker build -f "Dockerfile.node" --target runtime --build-arg APP_SCOPE="$name" -t "$tag" "." fiOption B (drop bake_target):
- const add = (name, bake_target, image, container, port, path, env_flags = "") => - include.push({ name, bake_target, image, container, host_port: port, path, env_flags }); + const add = (name, image, container, port, path, env_flags = "") => + include.push({ name, image, container, host_port: port, path, env_flags }); - if (buildAll || changed.web) add('web', 'web', 'plane-web:ci-smoke', 'plane-web-ci', 3001, '/'); - if (buildAll || changed.space) add('space', 'space', 'plane-space:ci-smoke', 'plane-space-ci', 3002, '/spaces'); - if (buildAll || changed.admin) add('admin', 'admin', 'plane-admin:ci-smoke', 'plane-admin-ci', 3003, '/god-mode'); - if (buildAll || changed.live) add('live', 'live', 'plane-live:ci-smoke', 'plane-live-ci', 3005, '/live/health', '-e NODE_ENV=production -e LIVE_BASE_PATH=/live'); + if (buildAll || changed.web) add('web', 'plane-web:ci-smoke', 'plane-web-ci', 3001, '/'); + if (buildAll || changed.space) add('space', 'plane-space:ci-smoke', 'plane-space-ci', 3002, '/spaces'); + if (buildAll || changed.admin) add('admin', 'plane-admin:ci-smoke', 'plane-admin-ci', 3003, '/god-mode'); + if (buildAll || changed.live) add('live', 'plane-live:ci-smoke', 'plane-live-ci', 3005, '/live/health', '-e NODE_ENV=production -e LIVE_BASE_PATH=/live'); - add('web', 'web', 'plane-web:ci-smoke', 'plane-web-ci', 3001, '/'); + add('web', 'plane-web:ci-smoke', 'plane-web-ci', 3001, '/');Also applies to: 88-97, 124-134
Dockerfile.aio (1)
66-71: Copying entire /usr/lib and /usr/local from node into a Python base is risky and bloats the imageThis can clobber libraries from the Python image and inflate size. For Next.js standalone you primarily need the node runtime.
Safer alternatives:
- Install a pinned Node via apk (if version acceptable):
-# Copy Node runtime into the AIO container for Next standalone apps -COPY --from=node /usr/lib /usr/lib -COPY --from=node /usr/local/lib /usr/local/lib -COPY --from=node /usr/local/include /usr/local/include -COPY --from=node /usr/local/bin /usr/local/bin +# Install Node runtime (sufficient to run Next standalone) +RUN apk add --no-cache nodejs-current
- Or copy only the node binary (verify deps with ldd):
-# Copy Node runtime into the AIO container for Next standalone apps -COPY --from=node /usr/lib /usr/lib -COPY --from=node /usr/local/lib /usr/local/lib -COPY --from=node /usr/local/include /usr/local/include -COPY --from=node /usr/local/bin /usr/local/bin +# Minimal Node copy (ensure required libs exist) +COPY --from=node /usr/local/bin/node /usr/local/bin/nodeIf you keep the current approach, please confirm no critical libs are overridden at runtime.
docker-bake.hcl (2)
4-5: Update comment: Dockerfiles are at repo root, not under plane/Minor doc drift that could confuse maintainers skimming the file.
-# Uses unified Dockerfiles under plane/ for Node (Next.js) apps and API. +# Uses unified Dockerfiles at the repository root for Node (Next.js) apps and API.
46-49: Consider building both amd64 and arm64 by defaultMany contributors are on Apple Silicon; adding linux/arm64 improves local bake parity.
variable "PLATFORMS" { # List of platforms (e.g., ["linux/amd64", "linux/arm64"]) - default = ["linux/amd64"] + default = ["linux/amd64", "linux/arm64"] }docker-compose-local.yml (2)
17-25: DRY up duplicated S3/MinIO env across services with anchorsYou repeat the same env block in api, worker, beat-worker, and migrator. Use YAML anchors to reduce drift.
Introduce an anchor near the top and reference it in each service:
services: + x-s3-env: &s3_env + USE_MINIO: "1" + AWS_S3_ENDPOINT_URL: http://plane-minio:9000 + MINIO_ENDPOINT_URL: http://plane-minio:9000 + AWS_ACCESS_KEY_ID: ${AWS_ACCESS_KEY_ID:-minioadmin} + AWS_SECRET_ACCESS_KEY: ${AWS_SECRET_ACCESS_KEY:-minioadmin} + AWS_S3_BUCKET_NAME: ${AWS_S3_BUCKET_NAME:-uploads} + AWS_REGION: ${AWS_REGION:-us-east-1} @@ api: @@ - environment: - USE_MINIO: "1" - AWS_S3_ENDPOINT_URL: http://plane-minio:9000 - MINIO_ENDPOINT_URL: http://plane-minio:9000 - AWS_ACCESS_KEY_ID: ${AWS_ACCESS_KEY_ID:-minioadmin} - AWS_SECRET_ACCESS_KEY: ${AWS_SECRET_ACCESS_KEY:-minioadmin} - AWS_S3_BUCKET_NAME: ${AWS_S3_BUCKET_NAME:-uploads} - AWS_REGION: ${AWS_REGION:-us-east-1} + environment: *s3_env @@ worker: - environment: - USE_MINIO: "1" - AWS_S3_ENDPOINT_URL: http://plane-minio:9000 - MINIO_ENDPOINT_URL: http://plane-minio:9000 - AWS_ACCESS_KEY_ID: ${AWS_ACCESS_KEY_ID:-minioadmin} - AWS_SECRET_ACCESS_KEY: ${AWS_SECRET_ACCESS_KEY:-minioadmin} - AWS_S3_BUCKET_NAME: ${AWS_S3_BUCKET_NAME:-uploads} - AWS_REGION: ${AWS_REGION:-us-east-1} + environment: *s3_env @@ beat-worker: - environment: - USE_MINIO: "1" - AWS_S3_ENDPOINT_URL: http://plane-minio:9000 - MINIO_ENDPOINT_URL: http://plane-minio:9000 - AWS_ACCESS_KEY_ID: ${AWS_ACCESS_KEY_ID:-minioadmin} - AWS_SECRET_ACCESS_KEY: ${AWS_SECRET_ACCESS_KEY:-minioadmin} - AWS_S3_BUCKET_NAME: ${AWS_S3_BUCKET_NAME:-uploads} - AWS_REGION: ${AWS_REGION:-us-east-1} + environment: *s3_env @@ migrator: - environment: - USE_MINIO: "1" - AWS_S3_ENDPOINT_URL: http://plane-minio:9000 - MINIO_ENDPOINT_URL: http://plane-minio:9000 - AWS_ACCESS_KEY_ID: ${AWS_ACCESS_KEY_ID:-minioadmin} - AWS_SECRET_ACCESS_KEY: ${AWS_SECRET_ACCESS_KEY:-minioadmin} - AWS_S3_BUCKET_NAME: ${AWS_S3_BUCKET_NAME:-uploads} - AWS_REGION: ${AWS_REGION:-us-east-1} + environment: *s3_envAlso applies to: 47-55, 74-81, 100-107
5-35: Optional: add healthchecks and depends_on conditions to reduce flakinessWithout healthchecks, api/worker may start before DB/Redis/MQ/MinIO are ready. Adding simple healthchecks and using depends_on: condition: service_healthy improves local DX.
Example (apply similarly to other infra):
plane-db: healthcheck: test: ["CMD", "pg_isready", "-U", "${POSTGRES_USER:-plane}"] interval: 5s timeout: 3s retries: 10 api: depends_on: plane-db: condition: service_healthy plane-redis: condition: service_started plane-mq: condition: service_started plane-minio: condition: service_startedAlso applies to: 114-171
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (62)
.github/workflows/build-branch.yml(7 hunks).github/workflows/docker-smoke-aio.yml(1 hunks).github/workflows/docker-smoke.yml(1 hunks).github/workflows/pull-request-build-lint-api.yml(2 hunks)CONTRIBUTING.md(1 hunks)Dockerfile.aio(1 hunks)Dockerfile.api(1 hunks)Dockerfile.node(1 hunks)apps/admin/Dockerfile.admin(0 hunks)apps/admin/Dockerfile.dev(0 hunks)apps/api/Dockerfile.api(0 hunks)apps/api/Dockerfile.dev(0 hunks)apps/api/plane/api/views/asset.py(3 hunks)apps/api/plane/api/views/cycle.py(6 hunks)apps/api/plane/api/views/intake.py(4 hunks)apps/api/plane/api/views/issue.py(4 hunks)apps/api/plane/api/views/module.py(2 hunks)apps/api/plane/api/views/project.py(1 hunks)apps/api/plane/api/views/state.py(2 hunks)apps/api/plane/app/serializers/draft.py(0 hunks)apps/api/plane/app/serializers/issue.py(0 hunks)apps/api/plane/app/serializers/project.py(0 hunks)apps/api/plane/app/views/cycle/base.py(2 hunks)apps/api/plane/app/views/page/base.py(0 hunks)apps/api/plane/app/views/project/base.py(1 hunks)apps/api/plane/app/views/project/invite.py(1 hunks)apps/api/plane/app/views/project/member.py(1 hunks)apps/api/plane/app/views/workspace/base.py(0 hunks)apps/api/plane/app/views/workspace/invite.py(1 hunks)apps/api/plane/app/views/workspace/member.py(3 hunks)apps/api/plane/authentication/views/space/password_management.py(3 hunks)apps/api/plane/authentication/views/space/signout.py(1 hunks)apps/api/plane/bgtasks/email_notification_task.py(1 hunks)apps/api/plane/bgtasks/export_task.py(1 hunks)apps/api/plane/bgtasks/notification_task.py(5 hunks)apps/api/plane/bgtasks/project_invitation_task.py(1 hunks)apps/api/plane/db/models/asset.py(1 hunks)apps/api/plane/settings/local.py(1 hunks)apps/api/plane/tests/contract/api/test_labels.py(0 hunks)apps/api/plane/tests/unit/bg_tasks/test_copy_s3_objects.py(1 hunks)apps/api/plane/tests/unit/utils/test_url.py(0 hunks)apps/api/plane/utils/content_validator.py(1 hunks)apps/api/plane/utils/url.py(1 hunks)apps/api/pyproject.toml(2 hunks)apps/api/requirements/base.txt(2 hunks)apps/live/package.json(1 hunks)apps/live/tsup.config.ts(1 hunks)apps/space/Dockerfile.dev(0 hunks)apps/space/Dockerfile.space(0 hunks)apps/web/Dockerfile.dev(0 hunks)apps/web/Dockerfile.web(0 hunks)deployments/aio/community/supervisor.conf(3 hunks)deployments/cli/community/build.yml(2 hunks)docker-bake.hcl(1 hunks)docker-compose-local.yml(1 hunks)docker-compose.yml(7 hunks)mise.toml(1 hunks)packages/editor/tsup.config.ts(1 hunks)packages/types/package.json(1 hunks)packages/ui/tsup.config.ts(1 hunks)scripts/smoke-aio.sh(1 hunks)scripts/smoke.sh(1 hunks)
💤 Files with no reviewable changes (15)
- apps/api/plane/app/views/page/base.py
- apps/api/plane/app/serializers/draft.py
- apps/api/plane/app/serializers/issue.py
- apps/api/plane/tests/contract/api/test_labels.py
- apps/api/plane/tests/unit/utils/test_url.py
- apps/space/Dockerfile.space
- apps/web/Dockerfile.web
- apps/admin/Dockerfile.dev
- apps/api/plane/app/views/workspace/base.py
- apps/api/Dockerfile.dev
- apps/api/plane/app/serializers/project.py
- apps/web/Dockerfile.dev
- apps/space/Dockerfile.dev
- apps/admin/Dockerfile.admin
- apps/api/Dockerfile.api
✅ Files skipped from review due to trivial changes (10)
- apps/api/plane/bgtasks/project_invitation_task.py
- CONTRIBUTING.md
- apps/api/plane/authentication/views/space/signout.py
- apps/api/plane/tests/unit/bg_tasks/test_copy_s3_objects.py
- apps/api/plane/bgtasks/export_task.py
- apps/api/plane/app/views/project/member.py
- apps/api/plane/authentication/views/space/password_management.py
- apps/api/plane/app/views/cycle/base.py
- apps/api/plane/app/views/project/invite.py
- apps/api/plane/api/views/intake.py
🚧 Files skipped from review as they are similar to previous changes (26)
- apps/api/plane/utils/content_validator.py
- apps/api/plane/app/views/workspace/invite.py
- apps/api/plane/api/views/project.py
- apps/live/package.json
- apps/api/plane/db/models/asset.py
- apps/api/plane/api/views/cycle.py
- packages/editor/tsup.config.ts
- mise.toml
- Dockerfile.node
- apps/api/requirements/base.txt
- apps/api/plane/api/views/state.py
- docker-compose.yml
- deployments/cli/community/build.yml
- apps/api/plane/bgtasks/email_notification_task.py
- apps/api/plane/app/views/workspace/member.py
- apps/api/pyproject.toml
- apps/api/plane/api/views/module.py
- apps/api/plane/app/views/project/base.py
- apps/api/plane/bgtasks/notification_task.py
- .github/workflows/pull-request-build-lint-api.yml
- apps/api/plane/api/views/asset.py
- apps/api/plane/api/views/issue.py
- packages/types/package.json
- deployments/aio/community/supervisor.conf
- .github/workflows/docker-smoke-aio.yml
- apps/api/plane/utils/url.py
🧰 Additional context used
🪛 Shellcheck (0.10.0)
scripts/smoke.sh
[warning] 82-82: CONTAINERS appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 205-205: cid appears unused. Verify use (or export if used externally).
(SC2034)
[error] 302-302: 'local' is only valid in functions.
(SC2168)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Build and smoke test admin
- GitHub Check: Build and smoke test live
- GitHub Check: Build and smoke test web
- GitHub Check: Build and smoke test space
- GitHub Check: Build and smoke test AIO
- GitHub Check: Analyze (javascript)
- GitHub Check: Build and lint web apps
🔇 Additional comments (8)
packages/ui/tsup.config.ts (1)
7-7: Verifydts.respectExternalbehavior in tsup v8.4.0Based on your script, only
packages/ui/tsup.config.tsis using a custom DTS option:
- packages/ui/tsup.config.ts (line 7):
dts: { respectExternal: true },Rollup-plugin-dts does support
respectExternalin its options (npmjs.com), and tsup v8.4.0 delegates itsdtsobject directly to that plugin. However, tsup’s documentation doesn’t explicitly listrespectExternal, so please manually verify that a build actually inlines (or excludes) external.d.tsimports as expected. For example:
- Add a known external type (e.g. a type from
@plane/utils) in your UI package entry.- Run
tsupand inspectdist/index.d.tsto confirm the external type is (or isn’t) bundled perrespectExternal: true.If you find it’s not honored:
- Consider using tsup’s built-in resolve flag (
--dts-resolveordts: { resolve: true }) to bundle external types instead.- Or fall back to a custom Rollup config with
rollup-plugin-dtsdirectly.Dockerfile.api (1)
76-83: Confirm apk package availability for nodejs-current on your Alpine basenodejs-current may be repo-specific on certain Alpine versions; if unavailable, fallback to nodejs + npm or install via a tarball.
If needed, switch to:
- apk add --no-cache nodejs npm
- or install a pinned Node via official tarball (adds a few lines but is repo-agnostic).
.github/workflows/build-branch.yml (2)
151-153: Consolidation to root-level Dockerfiles looks consistentUsing ./Dockerfile.node for admin/web/space and ./Dockerfile.api for API aligns paths across workflows and avoids the earlier “plane/” drift.
Also applies to: 175-177, 199-201, 244-245
277-285: Good switch to block-style needsThis avoids YAML flow list foot-guns and improves readability.
.github/workflows/docker-smoke.yml (1)
141-161: Smoke probe loop is robust and well-instrumentedGood use of curl with -L, retry loop, and grouped container logs on failure. Port isolation per app avoids collisions during parallel matrix runs.
Dockerfile.aio (2)
84-88: Verify Python base image compatibilityThe script did not find any
FROM python:reference in Dockerfile.api, yet the runner image is based onpython:3.12.10-alpine. Copying site-packages and binaries from an unknown or different Python base risks ABI incompatibility. Please confirm and align the Python environments:• Check or add the exact Python base in Dockerfile.api (e.g.
FROM python:3.12.10-alpine) so both stages share the same interpreter and C library (Musl vs. glibc).
• If you need a different base in api_ctx, consider building wheels or a virtualenv there and copying only that into the runner (e.g. into/opt/venv), then symlink intoPATH, rather than copying raw site-packages and binaries.
90-91: Caddy install path confirmed
Thecaddy:2.10.0-builder-alpineimage builds and outputs the Caddy binary at/usr/bin/caddy, so theCOPY --from=caddy-builder /usr/bin/caddy /usr/bin/caddystep will succeed as written.docker-bake.hcl (1)
108-114: Dockerfile.ce Caddy binary path verified
Theapps/proxy/Dockerfile.cecorrectly installs and copies the Caddy binary to/usr/bin/caddy, and the Dockerfile path and context are accurate. No changes are required.
packages/ui/tsup.config.ts
Outdated
| splitting: true, | ||
| treeshake: true, | ||
| external: ["react", "@plane/types"], | ||
| external: ["react", "react-dom", "@plane/types", "@plane/utils", "@plane/constants"], |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Externalizing react, react-dom, and internal workspace deps looks right
Marking react, react-dom, @plane/types, @plane/utils, and @plane/constants as externals is a good call for a UI library to avoid duplicate copies and keep bundle size small. This aligns with common tsup guidance to externalize peer deps. (blog.logrocket.com)
Optionally, verify that these are declared as peerDependencies in packages/ui/package.json to ensure consumers provide them:
🏁 Script executed:
#!/bin/bash
jq '.peerDependencies' packages/ui/package.jsonLength of output: 98
To ensure that your internal workspace packages are properly declared (so consumers won’t hit missing-module errors when you externalize them), let’s check your dependencies block:
🏁 Script executed:
#!/bin/bash
jq '.dependencies' packages/ui/package.jsonLength of output: 754
Ensure all externals are declared as peerDependencies
The TSUP config correctly externals react, react-dom, @plane/types, @plane/utils, and @plane/constants to avoid bundling duplicates. However, in packages/ui/package.json only react and react-dom are listed under peerDependencies; the @plane/* packages remain in dependencies. Consumers won’t get those modules unless they’re also peer deps.
• In packages/ui/package.json, move the following from dependencies to peerDependencies:
– @plane/types
– @plane/utils
– @plane/constants
• Alternatively, if you intend to bundle any of these, remove them from the TSUP external list to match your dependency setup.
🤖 Prompt for AI Agents
In packages/ui/tsup.config.ts around line 12, the config externals list includes
"@plane/types", "@plane/utils", and "@plane/constants" but
packages/ui/package.json still lists those under dependencies; move
@plane/types, @plane/utils, and @plane/constants from dependencies to
peerDependencies in packages/ui/package.json so package consumers provide the
same external modules, or if you prefer to bundle them instead, remove those
three entries from the tsup externals array to keep package.json and bundling
behavior consistent.
0dee2b5 to
b53fd08
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/build-branch.yml (1)
373-381: YAML parse risk: trailing comma in flow-sequence for needs.One remaining flow-sequence still has a trailing comma; GitHub Actions may reject it.
- needs: - [ - branch_build_setup, - branch_build_push_admin, - branch_build_push_web, - branch_build_push_space, - branch_build_push_live, - branch_build_push_api, - branch_build_push_proxy, - ] + needs: + - branch_build_setup + - branch_build_push_admin + - branch_build_push_web + - branch_build_push_space + - branch_build_push_live + - branch_build_push_api + - branch_build_push_proxy
♻️ Duplicate comments (3)
scripts/smoke.sh (1)
301-308: Fix: 'local' not allowed at top-level in case parser (SC2168).This breaks the script on Bash. Assign without
localsince we’re at top-level.- --image-prefix) - local prefix="$2" + --image-prefix) + prefix="$2" WEB_IMAGE="${prefix}plane-web:latest" SPACE_IMAGE="${prefix}plane-space:latest" ADMIN_IMAGE="${prefix}plane-admin:latest" LIVE_IMAGE="${prefix}plane-live:latest" shift 2 ;;docker-compose-local.yml (1)
149-171: MinIO bootstrap uses 'mc' inside minio/minio — that image doesn’t ship the client.This init will fail at runtime. Run the server normally and bootstrap via a short-lived minio/mc sidecar.
plane-minio: image: minio/minio restart: unless-stopped - # Bootstrap bucket on first run; suitable for local smoke/dev only - entrypoint: > - /bin/sh -c " - set -e - mkdir -p /export/${AWS_S3_BUCKET_NAME:-uploads}; - minio server /export --console-address ':9090' & pid=$!; - sleep 5; - mc alias set myminio http://plane-minio:9000 ${AWS_ACCESS_KEY_ID:-minioadmin} ${AWS_SECRET_ACCESS_KEY:-minioadmin}; - mc mb myminio/${AWS_S3_BUCKET_NAME:-uploads} -p || true; - wait $pid - " + command: server /export --console-address ':9090' environment: MINIO_ROOT_USER: ${AWS_ACCESS_KEY_ID:-minioadmin} MINIO_ROOT_PASSWORD: ${AWS_SECRET_ACCESS_KEY:-minioadmin} volumes: - uploads:/export ports: - "9000:9000" - "9090:9090" + + plane-minio-init: + image: minio/mc + depends_on: + - plane-minio + restart: "no" + entrypoint: > + /bin/sh -c " + set -e; + until (mc alias set myminio http://plane-minio:9000 ${AWS_ACCESS_KEY_ID:-minioadmin} ${AWS_SECRET_ACCESS_KEY:-minioadmin}); do + echo 'waiting for minio...'; sleep 2; + done; + mc mb myminio/${AWS_S3_BUCKET_NAME:-uploads} -p || true; + "packages/ui/tsup.config.ts (1)
7-7: dts: { respectExternal: true } is the right callPrevents bundling peer types and keeps type boundaries clean.
🧹 Nitpick comments (9)
scripts/smoke.sh (3)
79-85: Tidy: remove unused in-memory CONTAINERS array (SC2034).All container tracking is file-backed via CONTAINERS_FILE; the array isn’t read.
TMP_DIR="$(mktemp -d -t plane-smoke.XXXXXX)" CONTAINERS_FILE="$TMP_DIR/containers.txt" : > "$CONTAINERS_FILE" -CONTAINERS=() PIDS=() FAILURES=0
194-217: Silence SC2034: 'cid' assigned but never used.You gate on docker run’s exit code only; the variable isn’t referenced.
- if ! cid=$(docker run -d --name "$container" -p "${port}:3000" $envflags $DOCKER_FLAGS "$image" 2>"$TMP_DIR/${container}.err"); then + if ! docker run -d --name "$container" -p "${port}:3000" $envflags $DOCKER_FLAGS "$image" 2>"$TMP_DIR/${container}.err"; then @@ - if ! cid=$(docker run -d --name "$container" -p "${port}:3000" $envflags $DOCKER_FLAGS "$alt_image" 2>"$TMP_DIR/${container}.err"); then + if ! docker run -d --name "$container" -p "${port}:3000" $envflags $DOCKER_FLAGS "$alt_image" 2>"$TMP_DIR/${container}.err"; thenAlso applies to: 205-211
137-153: Port-check fallback is pragmatic; consider host-aware lsof for precision.Optional: when
lsofexists, use-iTCP@${host}:${port}to reduce false positives on multi-IP hosts.- elif exists_cmd lsof; then - lsof -iTCP:"$port" -sTCP:LISTEN >/dev/null 2>&1 && return 0 + elif exists_cmd lsof; then + lsof -iTCP@"$host":"$port" -sTCP:LISTEN >/dev/null 2>&1 && return 0.github/workflows/docker-smoke.yml (1)
72-99: Nit: drop unused bake_target from matrix to avoid confusion.You’re not calling bake; keeping
bake_targetcan mislead maintainers.-const add = (name, bake_target, image, container, port, path, env_flags = "") => - include.push({ name, bake_target, image, container, host_port: port, path, env_flags }); +const add = (name, image, container, port, path, env_flags = "") => + include.push({ name, image, container, host_port: port, path, env_flags }); @@ -if (buildAll || changed.web) add('web', 'web', 'plane-web:ci-smoke', 'plane-web-ci', 3001, '/'); -if (buildAll || changed.space) add('space', 'space', 'plane-space:ci-smoke', 'plane-space-ci', 3002, '/spaces'); -if (buildAll || changed.admin) add('admin', 'admin', 'plane-admin:ci-smoke', 'plane-admin-ci', 3003, '/god-mode'); -if (buildAll || changed.live) add('live', 'live', 'plane-live:ci-smoke', 'plane-live-ci', 3005, '/live/health', '-e NODE_ENV=production -e LIVE_BASE_PATH=/live'); +if (buildAll || changed.web) add('web', 'plane-web:ci-smoke', 'plane-web-ci', 3001, '/'); +if (buildAll || changed.space) add('space', 'plane-space:ci-smoke', 'plane-space-ci', 3002, '/spaces'); +if (buildAll || changed.admin) add('admin', 'plane-admin:ci-smoke', 'plane-admin-ci', 3003, '/god-mode'); +if (buildAll || changed.live) add('live', 'plane-live:ci-smoke', 'plane-live-ci', 3005, '/live/health', '-e NODE_ENV=production -e LIVE_BASE_PATH=/live'); @@ - add('web', 'web', 'plane-web:ci-smoke', 'plane-web-ci', 3001, '/'); + add('web', 'plane-web:ci-smoke', 'plane-web-ci', 3001, '/');apps/live/package.json (1)
29-31: Remove unneeded runtime dependencies in apps/liveOur search shows no direct imports of
@plane/ui,@plane/utilsor@plane/hooksin the runtime code—these packages only appear inapps/live/tsup.config.tsas externals (line 8). To keep the Docker image lean:• Remove these from the
"dependencies"section ofapps/live/package.json.
• If they’re only needed at build time (for bundling), move them into"devDependencies".
• Updateapps/live/tsup.config.tsto drop any unused externals.packages/ui/package.json (3)
27-33: If you ever publish @plane/ui, replace workspace: in peers*Not required now since "private": true, but if publishing later, convert workspace:* ranges in peerDependencies to proper semver ranges to avoid invalid manifests on npm.
27-33: Externalize @plane/hooks in packages/ui to avoid duplicate singletonsVerified that
@plane/hooksis imported extensively withinpackages/ui/srcbut is not currently listed as an external in the UI build (packages/ui/tsup.config.ts) nor as a peer dependency. To prevent multiple instances of React contexts or singletons at runtime, please:• In packages/ui/package.json
– Move@plane/hooksfrom"dependencies"into"peerDependencies".
– Add it alongside@plane/utils:"peerDependencies": { "react": "^18.3.1", "react-dom": "^18.3.1", "@plane/constants": "workspace:*", "@plane/types": "workspace:*", "@plane/utils": "workspace:*", + "@plane/hooks": "workspace:*" }, "dependencies": { - "@plane/hooks": "workspace:*",• In packages/ui/tsup.config.ts
– Include@plane/hooksin theexternalarray:external: [ "react", "react-dom", "@plane/types", "@plane/utils", "@plane/constants", + "@plane/hooks", ],This ensures consuming apps provide a single shared instance of
@plane/hooks.
27-33: React versions are aligned—consider bumping @plane/typesWe ran a workspace-wide check and observed the following React versions:
- apps/admin, apps/space, apps/web →
^18.3.1- @plane/editor, @plane/hooks, @plane/propel, @plane/ui, @plane/utils →
^18.3.1- @plane/types →
^18.0.0Since
^18.3.1satisfies^18.0.0, this won’t break the hoisted install, but it can cause confusion or unexpected resolutions:
- packages/types/package.json lists React as a regular dependency at
^18.0.0. Consider bumping it to^18.3.1(or converting it to apeerDependency/devDependencyif it only provides type definitions) to keep all ranges in lockstep.No additional misalignments were found.
packages/ui/tsup.config.ts (1)
12-12: Also externalize @plane/hooks (and react/jsx-runtime) to avoid duplicationTo keep a single module instance across app and library, add @plane/hooks to externals. Adding react/jsx-runtime is a minor hardening for automatic JSX runtimes.
Apply:
- external: ["react", "react-dom", "@plane/types", "@plane/utils", "@plane/constants"], + external: ["react", "react-dom", "react/jsx-runtime", "@plane/types", "@plane/utils", "@plane/constants", "@plane/hooks"],
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (63)
.github/workflows/build-branch.yml(7 hunks).github/workflows/docker-smoke-aio.yml(1 hunks).github/workflows/docker-smoke.yml(1 hunks).github/workflows/pull-request-build-lint-api.yml(2 hunks)CONTRIBUTING.md(1 hunks)Dockerfile.aio(1 hunks)Dockerfile.api(1 hunks)Dockerfile.node(1 hunks)apps/admin/Dockerfile.admin(0 hunks)apps/admin/Dockerfile.dev(0 hunks)apps/api/Dockerfile.api(0 hunks)apps/api/Dockerfile.dev(0 hunks)apps/api/plane/api/views/asset.py(3 hunks)apps/api/plane/api/views/cycle.py(6 hunks)apps/api/plane/api/views/intake.py(4 hunks)apps/api/plane/api/views/issue.py(4 hunks)apps/api/plane/api/views/module.py(2 hunks)apps/api/plane/api/views/project.py(1 hunks)apps/api/plane/api/views/state.py(2 hunks)apps/api/plane/app/serializers/draft.py(0 hunks)apps/api/plane/app/serializers/issue.py(0 hunks)apps/api/plane/app/serializers/project.py(0 hunks)apps/api/plane/app/views/cycle/base.py(2 hunks)apps/api/plane/app/views/page/base.py(0 hunks)apps/api/plane/app/views/project/base.py(1 hunks)apps/api/plane/app/views/project/invite.py(1 hunks)apps/api/plane/app/views/project/member.py(1 hunks)apps/api/plane/app/views/workspace/base.py(0 hunks)apps/api/plane/app/views/workspace/invite.py(1 hunks)apps/api/plane/app/views/workspace/member.py(3 hunks)apps/api/plane/authentication/views/space/password_management.py(3 hunks)apps/api/plane/authentication/views/space/signout.py(1 hunks)apps/api/plane/bgtasks/email_notification_task.py(1 hunks)apps/api/plane/bgtasks/export_task.py(1 hunks)apps/api/plane/bgtasks/notification_task.py(5 hunks)apps/api/plane/bgtasks/project_invitation_task.py(1 hunks)apps/api/plane/db/models/asset.py(1 hunks)apps/api/plane/settings/local.py(1 hunks)apps/api/plane/tests/contract/api/test_labels.py(0 hunks)apps/api/plane/tests/unit/bg_tasks/test_copy_s3_objects.py(1 hunks)apps/api/plane/tests/unit/utils/test_url.py(0 hunks)apps/api/plane/utils/content_validator.py(1 hunks)apps/api/plane/utils/url.py(1 hunks)apps/api/pyproject.toml(2 hunks)apps/api/requirements/base.txt(2 hunks)apps/live/package.json(1 hunks)apps/live/tsup.config.ts(1 hunks)apps/space/Dockerfile.dev(0 hunks)apps/space/Dockerfile.space(0 hunks)apps/web/Dockerfile.dev(0 hunks)apps/web/Dockerfile.web(0 hunks)deployments/aio/community/supervisor.conf(3 hunks)deployments/cli/community/build.yml(2 hunks)docker-bake.hcl(1 hunks)docker-compose-local.yml(1 hunks)docker-compose.yml(7 hunks)mise.toml(1 hunks)packages/editor/tsup.config.ts(1 hunks)packages/types/package.json(1 hunks)packages/ui/package.json(1 hunks)packages/ui/tsup.config.ts(1 hunks)scripts/smoke-aio.sh(1 hunks)scripts/smoke.sh(1 hunks)
💤 Files with no reviewable changes (15)
- apps/api/plane/app/serializers/issue.py
- apps/api/plane/app/serializers/project.py
- apps/api/plane/tests/contract/api/test_labels.py
- apps/api/plane/app/views/page/base.py
- apps/web/Dockerfile.dev
- apps/api/plane/app/serializers/draft.py
- apps/api/plane/tests/unit/utils/test_url.py
- apps/web/Dockerfile.web
- apps/space/Dockerfile.dev
- apps/admin/Dockerfile.admin
- apps/admin/Dockerfile.dev
- apps/api/Dockerfile.api
- apps/space/Dockerfile.space
- apps/api/Dockerfile.dev
- apps/api/plane/app/views/workspace/base.py
✅ Files skipped from review due to trivial changes (8)
- apps/api/plane/app/views/project/member.py
- apps/api/plane/bgtasks/export_task.py
- apps/api/plane/bgtasks/project_invitation_task.py
- apps/api/plane/authentication/views/space/signout.py
- apps/api/requirements/base.txt
- apps/api/plane/db/models/asset.py
- apps/api/plane/api/views/cycle.py
- apps/api/plane/app/views/project/invite.py
🚧 Files skipped from review as they are similar to previous changes (33)
- apps/api/plane/tests/unit/bg_tasks/test_copy_s3_objects.py
- apps/api/pyproject.toml
- apps/api/plane/api/views/intake.py
- apps/api/plane/app/views/workspace/invite.py
- apps/live/tsup.config.ts
- apps/api/plane/utils/content_validator.py
- apps/api/plane/bgtasks/email_notification_task.py
- mise.toml
- CONTRIBUTING.md
- apps/api/plane/api/views/project.py
- packages/types/package.json
- .github/workflows/pull-request-build-lint-api.yml
- apps/api/plane/app/views/cycle/base.py
- apps/api/plane/app/views/project/base.py
- Dockerfile.aio
- docker-compose.yml
- Dockerfile.api
- packages/editor/tsup.config.ts
- apps/api/plane/settings/local.py
- apps/api/plane/api/views/issue.py
- apps/api/plane/authentication/views/space/password_management.py
- deployments/aio/community/supervisor.conf
- scripts/smoke-aio.sh
- apps/api/plane/utils/url.py
- deployments/cli/community/build.yml
- apps/api/plane/api/views/state.py
- apps/api/plane/bgtasks/notification_task.py
- apps/api/plane/api/views/asset.py
- .github/workflows/docker-smoke-aio.yml
- apps/api/plane/api/views/module.py
- apps/api/plane/app/views/workspace/member.py
- docker-bake.hcl
- Dockerfile.node
🧰 Additional context used
🧬 Code graph analysis (1)
scripts/smoke.sh (1)
scripts/smoke-aio.sh (8)
exists_cmd(127-127)log(129-129)is_port_in_use(145-159)log_err(132-132)log_warn(131-131)log_ok(130-130)usage(91-125)cleanup(134-143)
🪛 Shellcheck (0.10.0)
scripts/smoke.sh
[warning] 82-82: CONTAINERS appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 205-205: cid appears unused. Verify use (or export if used externally).
(SC2034)
[error] 302-302: 'local' is only valid in functions.
(SC2168)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build and smoke test AIO
- GitHub Check: Build and lint web apps
- GitHub Check: Analyze (javascript)
🔇 Additional comments (11)
scripts/smoke.sh (1)
155-167: Cleanup is robust; nice file-backed de-dup and trap usage.File-based tracking and
sort -uprevent leaks even in parallel. Good job..github/workflows/docker-smoke.yml (1)
124-135: LGTM: root Dockerfile paths and HTTP probe flow look correct.Build contexts and Dockerfile paths align with repo layout; the probe prints logs on failure and cleans up containers.
Also applies to: 141-161
.github/workflows/build-branch.yml (5)
151-153: LGTM: Node apps now build from the shared root Dockerfile.node.Using APP_SCOPE per app is consistent with the consolidated Dockerfile.
175-177: LGTM: Consistent Dockerfile.node usage for Web.
199-201: LGTM: Consistent Dockerfile.node usage for Space.
244-246: LGTM: API build uses root Dockerfile.api with root context.
277-285: Good fix: converted needs flow-sequences to block lists.Removes trailing-comma YAML pitfalls and improves readability.
Also applies to: 332-339
docker-compose-local.yml (2)
118-118: LGTM: Postgres command quoting fixed for YAML.Double-quoted scalar avoids YAML parsing issues.
8-14: Good dev ergonomics: dev targets + bind-mounts for instant reload.Target=dev with ./apps/api bind mounts and per-role entrypoints looks solid for local DX.
Also applies to: 37-44, 65-71, 91-97
apps/live/package.json (1)
29-31: Additions align with monorepo externals; good moveDeclaring @plane/hooks, @plane/ui, and @plane/utils as workspace dependencies for live matches the externalization approach used elsewhere and should keep local dev consistent.
packages/ui/package.json (1)
27-33: Peer-deping internals + react-dom is correct for a UI libMoving @plane/constants, @plane/types, and @plane/utils to peerDependencies (and listing react-dom) aligns with the tsup externals and prevents duplicate bundles in consumers.
lifeiscontent
force-pushed
the
feat/fix-local-docker-dev
branch
2 times, most recently
from
9fbf7bc to
d8eaf20
Compare
lifeiscontent
force-pushed
the
feat/fix-local-docker-dev
branch
from
d8eaf20 to
9aa92da
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR consolidates Docker development infrastructure by creating unified Dockerfiles and improving the local development experience. The main goal is to reduce duplication across frontend apps while introducing a new all-in-one Docker image for simplified deployments.
- Unified Dockerfile.node for all Next.js apps (web, space, admin) replacing individual app Dockerfiles
- Consolidated Dockerfile.api for the Python backend with optimized multi-stage builds
- New all-in-one (AIO) Docker image combining all services in a single container
- Improved local docker-compose setup with dedicated infrastructure services
Reviewed Changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/smoke.sh | Smoke testing script for individual frontend app images |
| scripts/smoke-aio.sh | Smoke testing script for the all-in-one image |
| mise.toml | Tool version management configuration |
| docker-compose.yml | Updated to use unified Dockerfiles |
| docker-compose-local.yml | Rewritten for API-centric local development |
| docker-bake.hcl | Build configuration for all Docker images |
| Dockerfile.node | Unified multi-stage Dockerfile for Next.js apps |
| Dockerfile.api | Unified multi-stage Dockerfile for Python API |
| Dockerfile.aio | All-in-one image assembler |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| --redis-url <val> (default: ${REDIS_URL_DEFAULT}) | ||
| --amqp-url <val> (default: ${AMQP_URL_DEFAULT}) | ||
| --aws-region <val> (default: ${AWS_REGION_DEFAULT}) | ||
| --aws-access-key-id <val> (default: ${AWS_ACCESS_KEY_ID_DEFAULT}) |
There was a problem hiding this comment.
The help text references undefined variable 'AWS_ACCESS_KEY_ID_DEFAULT'. This variable is not defined in the script and should be 'AWS_ACCESS_KEY_ID_DEFAULT="minioadmin"' to match line 41.
| --amqp-url <val> (default: ${AMQP_URL_DEFAULT}) | ||
| --aws-region <val> (default: ${AWS_REGION_DEFAULT}) | ||
| --aws-access-key-id <val> (default: ${AWS_ACCESS_KEY_ID_DEFAULT}) | ||
| --aws-secret-access-key <val> (default: ${AWS_SECRET_ACCESS_KEY_DEFAULT}) |
There was a problem hiding this comment.
The help text references undefined variable 'AWS_SECRET_ACCESS_KEY_DEFAULT'. This variable is not defined in the script and should be 'AWS_SECRET_ACCESS_KEY_DEFAULT="minioadmin"' to match line 42.
| --aws-region <val> (default: ${AWS_REGION_DEFAULT}) | ||
| --aws-access-key-id <val> (default: ${AWS_ACCESS_KEY_ID_DEFAULT}) | ||
| --aws-secret-access-key <val> (default: ${AWS_SECRET_ACCESS_KEY_DEFAULT}) | ||
| --aws-s3-bucket-name <val> (default: ${AWS_S3_BUCKET_NAME_DEFAULT}) |
There was a problem hiding this comment.
The help text references undefined variable 'AWS_S3_BUCKET_NAME_DEFAULT'. This variable is not defined in the script and should be 'AWS_S3_BUCKET_NAME_DEFAULT="smoke-bucket"' to match line 43.
| --aws-access-key-id <val> (default: ${AWS_ACCESS_KEY_ID_DEFAULT}) | ||
| --aws-secret-access-key <val> (default: ${AWS_SECRET_ACCESS_KEY_DEFAULT}) | ||
| --aws-s3-bucket-name <val> (default: ${AWS_S3_BUCKET_NAME_DEFAULT}) | ||
| --aws-s3-endpoint-url <val> (default: ${AWS_S3_ENDPOINT_URL_DEFAULT}) |
There was a problem hiding this comment.
The help text references undefined variable 'AWS_S3_ENDPOINT_URL_DEFAULT'. This variable is not defined in the script and should be 'AWS_S3_ENDPOINT_URL_DEFAULT="http://127.0.0.1:19000"' to match line 44.
| --aws-secret-access-key <val> (default: ${AWS_SECRET_ACCESS_KEY_DEFAULT}) | ||
| --aws-s3-bucket-name <val> (default: ${AWS_S3_BUCKET_NAME_DEFAULT}) | ||
| --aws-s3-endpoint-url <val> (default: ${AWS_S3_ENDPOINT_URL_DEFAULT}) | ||
| --site-address <val> (default: ${SITE_ADDRESS_DEFAULT}) |
There was a problem hiding this comment.
The help text references undefined variable 'SITE_ADDRESS_DEFAULT'. This variable is not defined in the script and should be 'SITE_ADDRESS_DEFAULT=":80"' to match line 45.
| --aws-s3-bucket-name <val> (default: ${AWS_S3_BUCKET_NAME_DEFAULT}) | ||
| --aws-s3-endpoint-url <val> (default: ${AWS_S3_ENDPOINT_URL_DEFAULT}) | ||
| --site-address <val> (default: ${SITE_ADDRESS_DEFAULT}) | ||
| --file-size-limit <bytes> (default: ${FILE_SIZE_LIMIT_DEFAULT}) |
There was a problem hiding this comment.
The help text references undefined variable 'FILE_SIZE_LIMIT_DEFAULT'. This variable is not defined in the script and should be 'FILE_SIZE_LIMIT_DEFAULT="5242880"' to match line 46.
| mkdir -p /export/${AWS_S3_BUCKET_NAME:-uploads}; | ||
| minio server /export --console-address ':9090' & pid=$!; | ||
| sleep 5; | ||
| mc alias set myminio http://plane-minio:9000 ${AWS_ACCESS_KEY_ID:-minioadmin} ${AWS_SECRET_ACCESS_KEY:-minioadmin}; |
There was a problem hiding this comment.
The MinIO client alias setup uses 'http://plane-minio:9000' which refers to the Docker service name, but this command runs inside the MinIO container where the service would be accessible via localhost. It should be 'http://localhost:9000' instead.
| mc alias set myminio http://plane-minio:9000 ${AWS_ACCESS_KEY_ID:-minioadmin} ${AWS_SECRET_ACCESS_KEY:-minioadmin}; | |
| mc alias set myminio http://localhost:9000 ${AWS_ACCESS_KEY_ID:-minioadmin} ${AWS_SECRET_ACCESS_KEY:-minioadmin}; |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
scripts/smoke.sh (1)
301-308: Fix: ‘local’ used at top level breaks script (SC2168)This causes an immediate error. Remove local.
- --image-prefix) - local prefix="$2" + --image-prefix) + prefix="$2" WEB_IMAGE="${prefix}plane-web:latest" SPACE_IMAGE="${prefix}plane-space:latest" ADMIN_IMAGE="${prefix}plane-admin:latest" LIVE_IMAGE="${prefix}plane-live:latest" shift 2 ;;docker-compose-local.yml (1)
149-171: MinIO bootstrap uses ‘mc’ inside minio/minio image; it will fail — switch to mc sidecarThis was flagged earlier but the current file still embeds mc in MinIO’s entrypoint. The minio/minio image doesn’t ship mc. Move bootstrap to a short-lived minio/mc sidecar and keep MinIO running server-only.
plane-minio: image: minio/minio restart: unless-stopped - # Bootstrap bucket on first run; suitable for local smoke/dev only - entrypoint: > - /bin/sh -c " - set -e - mkdir -p /export/${AWS_S3_BUCKET_NAME:-uploads}; - minio server /export --console-address ':9090' & pid=$!; - sleep 5; - mc alias set myminio http://plane-minio:9000 ${AWS_ACCESS_KEY_ID:-minioadmin} ${AWS_SECRET_ACCESS_KEY:-minioadmin}; - mc mb myminio/${AWS_S3_BUCKET_NAME:-uploads} -p || true; - wait $pid - " + command: server /export --console-address ':9090' environment: MINIO_ROOT_USER: ${AWS_ACCESS_KEY_ID:-minioadmin} MINIO_ROOT_PASSWORD: ${AWS_SECRET_ACCESS_KEY:-minioadmin} volumes: - uploads:/export ports: - "9000:9000" - "9090:9090" + + plane-minio-init: + image: minio/mc + depends_on: + - plane-minio + entrypoint: > + /bin/sh -c " + set -e; + until (mc alias set myminio http://plane-minio:9000 ${AWS_ACCESS_KEY_ID:-minioadmin} ${AWS_SECRET_ACCESS_KEY:-minioadmin}); do echo 'waiting for minio...'; sleep 2; done; + mc mb myminio/${AWS_S3_BUCKET_NAME:-uploads} -p || true; + " + restart: "no"
🧹 Nitpick comments (14)
docker-compose.yml (3)
6-9: Specify build target=runtime for Next.js imagesIf Dockerfile.node is multi-stage, not setting target may produce the wrong stage. Safer to pin runtime.
Apply:
services: web: build: context: . dockerfile: ./Dockerfile.node + target: runtime args: DOCKER_BUILDKIT: 1 APP_SCOPE: web @@ admin: build: context: . dockerfile: ./Dockerfile.node + target: runtime args: DOCKER_BUILDKIT: 1 APP_SCOPE: admin @@ space: build: context: . dockerfile: ./Dockerfile.node + target: runtime args: DOCKER_BUILDKIT: 1 APP_SCOPE: spaceAlso applies to: 18-21, 31-34
43-47: De-duplicate API image builds across servicesapi/worker/beat-worker/migrator rebuild the same image. Build once and reuse via image: tag to speed local dev.
Example:
api: build: context: . dockerfile: ./Dockerfile.api + target: runtime + image: plane-api:dev @@ worker: - build: - context: . - dockerfile: ./Dockerfile.api + image: plane-api:dev @@ beat-worker: - build: - context: . - dockerfile: ./Dockerfile.api + image: plane-api:dev @@ migrator: - build: - context: . - dockerfile: ./Dockerfile.api + image: plane-api:devAlso applies to: 58-62, 74-78, 90-94
40-54: Add healthchecks and depends_on conditions for reliable startupHelps frontends wait for api readiness.
api: ... + healthcheck: + test: ["CMD-SHELL", "curl -fsS http://localhost:8000/health || exit 1"] + interval: 10s + timeout: 3s + retries: 10 web: depends_on: - - api + api: + condition: service_healthyConfirm actual API health endpoint path/port before applying.
scripts/smoke-aio.sh (1)
236-249: Handle --network host by skipping -p mappingWhen users pass --network host, -p is ignored; optionally adapt to avoid confusion.
-if ! docker run -d --name "$CONTAINER" \ - -p "${PORT}:80" \ +host_net=0 +for f in "${DOCKER_FLAGS_ARRAY[@]}"; do [[ "$f" == "--network" ]] && host_net=2; [[ $host_net -eq 2 && "$f" == "host" ]] && host_net=1; done +if ! docker run -d --name "$CONTAINER" \ + $( [[ $host_net -eq 1 ]] || echo "-p ${PORT}:80" ) \scripts/smoke.sh (1)
82-84: Remove unused CONTAINERS arrayYou track via file now; drop the unused array to quiet SC2034.
-CONTAINERS=() PIDS=() FAILURES=0.github/workflows/build-branch.yml (1)
146-147: Unify action version across jobs for repeatabilityMixing makeplane/actions/build-push@v1.0.0 and @v1.1.0 can lead to subtle drift.
-uses: makeplane/actions/build-push@v1.0.0 +uses: makeplane/actions/build-push@v1.1.0Confirm no breaking changes in v1.1.0 for non-AIO jobs.
Also applies to: 171-172, 196-197, 221-222, 244-245, 319-320
docker-bake.hcl (1)
4-5: Fix outdated comment: Dockerfiles aren’t under plane/Header mentions “under plane/” but file uses root-level Dockerfile.node/api.
-# Uses unified Dockerfiles under plane/ for Node (Next.js) apps and API. +# Uses unified Dockerfiles at repo root for Node (Next.js) apps and API..github/workflows/docker-smoke-aio.yml (3)
40-43: Set least-privileged permissions for the workflow tokenHarden the workflow by explicitly restricting GITHUB_TOKEN.
concurrency: group: aio-${{ github.workflow }}-${{ github.ref }} cancel-in-progress: false +permissions: + contents: read
111-121: Harden shell steps and remove brittle quotingAdd strict shell flags and drop quotes around the executable for clarity.
- name: Build AIO image with bake (load into local daemon) run: | + set -euo pipefail docker buildx bake -f "./docker-bake.hcl" --load aio - name: Run AIO smoke script run: | - chmod +x "scripts/smoke-aio.sh" - "scripts/smoke-aio.sh" + set -euo pipefail + chmod +x scripts/smoke-aio.sh + scripts/smoke-aio.sh
41-42: Consider enabling cancel-in-progress to save CI minutesFor branch-level runs, setting cancel-in-progress: true avoids overlapping AIO builds. Optional.
- cancel-in-progress: false + cancel-in-progress: truedocker-compose-local.yml (4)
150-150: Pin MinIO image for reproducible local devAvoid floating latest. Pin a known RELEASE tag or digest.
- image: minio/minio + image: minio/minio:<RELEASE_TAG_OR_DIGEST>Would you like me to propose a specific current tag?
1-3: Fix path in header comment to match actual Dockerfile locationsThe comment references plane/Dockerfile.* but builds use root-level Dockerfile.*.
-# Uses unified dev targets in plane/Dockerfile.node and plane/Dockerfile.api. +# Uses unified dev targets in ./Dockerfile.node and ./Dockerfile.api.
173-175: Remove stale “Node/pnpm caches” comment or define the volumesNo matching volumes are declared. Either drop the comment or add cache volumes.
-volumes: - # Node/pnpm caches for speedy dev - +volumes:
29-36: Optional: add healthchecks and depend-on conditions for a smoother first runConsider adding basic healthchecks to plane-db, plane-mq, and plane-minio and using depends_on: condition: service_healthy for api/worker/beat/migrator to reduce flakiness on cold starts.
Also applies to: 57-62, 83-88, 109-113
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (22)
.github/workflows/build-branch.yml(11 hunks).github/workflows/docker-smoke-aio.yml(1 hunks).github/workflows/docker-smoke.yml(1 hunks)Dockerfile.aio(1 hunks)Dockerfile.api(1 hunks)Dockerfile.node(1 hunks)apps/admin/Dockerfile.admin(0 hunks)apps/admin/Dockerfile.dev(0 hunks)apps/api/Dockerfile.api(0 hunks)apps/api/Dockerfile.dev(0 hunks)apps/space/Dockerfile.dev(0 hunks)apps/space/Dockerfile.space(0 hunks)apps/web/Dockerfile.dev(0 hunks)apps/web/Dockerfile.web(0 hunks)deployments/aio/community/supervisor.conf(3 hunks)deployments/cli/community/build.yml(2 hunks)docker-bake.hcl(1 hunks)docker-compose-local.yml(1 hunks)docker-compose.yml(7 hunks)mise.toml(1 hunks)scripts/smoke-aio.sh(1 hunks)scripts/smoke.sh(1 hunks)
💤 Files with no reviewable changes (8)
- apps/space/Dockerfile.space
- apps/api/Dockerfile.dev
- apps/admin/Dockerfile.dev
- apps/api/Dockerfile.api
- apps/web/Dockerfile.web
- apps/space/Dockerfile.dev
- apps/web/Dockerfile.dev
- apps/admin/Dockerfile.admin
🚧 Files skipped from review as they are similar to previous changes (7)
- Dockerfile.node
- deployments/cli/community/build.yml
- deployments/aio/community/supervisor.conf
- Dockerfile.aio
- mise.toml
- Dockerfile.api
- .github/workflows/docker-smoke.yml
🧰 Additional context used
🧬 Code graph analysis (1)
scripts/smoke.sh (1)
scripts/smoke-aio.sh (8)
exists_cmd(127-127)log(129-129)is_port_in_use(145-159)log_err(132-132)log_warn(131-131)log_ok(130-130)usage(91-125)cleanup(134-143)
🪛 Shellcheck (0.10.0)
scripts/smoke.sh
[warning] 82-82: CONTAINERS appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 205-205: cid appears unused. Verify use (or export if used externally).
(SC2034)
[error] 302-302: 'local' is only valid in functions.
(SC2168)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Build and smoke test live
- GitHub Check: Build and smoke test admin
- GitHub Check: Build and smoke test space
- GitHub Check: Build and smoke test web
- GitHub Check: Build and smoke test AIO
- GitHub Check: Build and lint web apps
- GitHub Check: Analyze (javascript)
🔇 Additional comments (6)
scripts/smoke-aio.sh (1)
214-219: Good: docker flags parsed and expanded safelyParsing DOCKER_FLAGS into an array and expanding with "${…[@]}" avoids argv mangling. Curl includes sane timeouts. LGTM.
Also applies to: 249-250
.github/workflows/build-branch.yml (2)
151-163: Consistent root-level Dockerfile paths and APP_SCOPE arg — goodNode apps now build from ./Dockerfile.node with APP_SCOPE and buildx endpoint wired. Looks consistent with repo layout.
Also applies to: 181-187, 205-213, 253-258
221-235: Live build path/context looks correctapps/live/Dockerfile.live with context . aligns with other files. LGTM.
docker-bake.hcl (2)
57-64: Good: shared common-node bake target with cache and platformsPromotes DRY and reproducible builds. LGTM.
116-132: AIO contexts wiring looks correctTargets reference per-app runtime images via target: names; tags are parameterized. LGTM.
.github/workflows/docker-smoke-aio.yml (1)
8-21: The verification script is running to inspect the workflow triggers and ensurescripts/**is included in both theon.pathsand the dorny “common” filter, as well as to confirm the existence of yourscripts/smoke-aio.sh. I’ll analyze the results once they’re available.
| if ! cid=$(docker run -d --name "$container" -p "${port}:3000" $envflags $DOCKER_FLAGS "$image" 2>"$TMP_DIR/${container}.err"); then | ||
| # If the image tag is :ci-smoke and it fails, try falling back to :latest | ||
| base="${image%:*}" | ||
| tag="${image##*:}" | ||
| if [[ "$tag" == "$image" ]]; then | ||
| tag="" | ||
| fi | ||
| if [[ "$tag" == "ci-smoke" ]]; then | ||
| alt_image="${base}:latest" | ||
| log_warn "Failed to start ${name} with ${image}, retrying with ${alt_image} ..." | ||
| if ! cid=$(docker run -d --name "$container" -p "${port}:3000" $envflags $DOCKER_FLAGS "$alt_image" 2>"$TMP_DIR/${container}.err"); then | ||
| log_err "Failed to start container ${container}" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Pass extra docker flags safely; remove unused cid
Avoid word-splitting pitfalls and silence SC2034.
- if ! cid=$(docker run -d --name "$container" -p "${port}:3000" $envflags $DOCKER_FLAGS "$image" 2>"$TMP_DIR/${container}.err"); then
+ if ! docker run -d --name "$container" -p "${port}:3000" $envflags "${DOCKER_FLAGS_ARR[@]}" "$image" 2>"$TMP_DIR/${container}.err"; then
@@
- if ! cid=$(docker run -d --name "$container" -p "${port}:3000" $envflags $DOCKER_FLAGS "$alt_image" 2>"$TMP_DIR/${container}.err"); then
+ if ! docker run -d --name "$container" -p "${port}:3000" $envflags "${DOCKER_FLAGS_ARR[@]}" "$alt_image" 2>"$TMP_DIR/${container}.err"; thenAnd initialize the array after args are parsed:
@@
done
+# Parse DOCKER_FLAGS string into an array for safe docker invocation
+if [[ -n "${DOCKER_FLAGS:-}" ]]; then
+ read -r -a DOCKER_FLAGS_ARR <<< "$DOCKER_FLAGS"
+else
+ DOCKER_FLAGS_ARR=()
+fi
+
trap cleanup EXIT INT TERM🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 205-205: cid appears unused. Verify use (or export if used externally).
(SC2034)
| local errfile="$TMP_DIR/${container}.curl.err" | ||
| status="$(curl -sS -o /dev/null -w "%{http_code}" -L "$url" 2>"$errfile" || true)" | ||
| if [[ "$status" == "200" ]]; then | ||
| log_ok "Success: ${name} responded 200 at ${url}" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add curl timeouts to avoid hanging probes
Mirror aio script behavior for robustness.
- status="$(curl -sS -o /dev/null -w "%{http_code}" -L "$url" 2>"$errfile" || true)"
+ status="$(curl --connect-timeout 5 --max-time 10 -sS -o /dev/null -w "%{http_code}" -L "$url" 2>"$errfile" || true)"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| local errfile="$TMP_DIR/${container}.curl.err" | |
| status="$(curl -sS -o /dev/null -w "%{http_code}" -L "$url" 2>"$errfile" || true)" | |
| if [[ "$status" == "200" ]]; then | |
| log_ok "Success: ${name} responded 200 at ${url}" | |
| local errfile="$TMP_DIR/${container}.curl.err" | |
| status="$(curl --connect-timeout 5 --max-time 10 -sS -o /dev/null -w "%{http_code}" -L "$url" 2>"$errfile" || true)" | |
| if [[ "$status" == "200" ]]; then | |
| log_ok "Success: ${name} responded 200 at ${url}" |
🤖 Prompt for AI Agents
In scripts/smoke.sh around lines 226 to 229, the curl probe can hang because it
lacks timeouts; update the curl invocation to include connection and overall
timeouts (for example --connect-timeout 5 and --max-time 10) and mirror the aio
script resilience by adding short retry behavior (e.g., --retry 3 --retry-delay
1) so the command fails fast and avoids hanging the smoke tests; keep stderr
redirection to errfile and preserve the existing -sS -o /dev/null -w
"%{http_code}" -L flags.
Description
Type of Change
Screenshots and Media (if applicable)
Test Scenarios
References
Summary by CodeRabbit
New Features
Improvements
CI
Chores
Tests