Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughRefactors the repo into a multi-app Makefile structure (frontend, backend, infra/dataprep), moving app-specific config and targets into per-app Makefiles. Standardizes variables on APP_FRONTEND/APP_BACKEND, updates docker-compose and k8s manifests accordingly, removes ES proxy path usage in frontend, and introduces an infra Makefile for Elasticsearch lifecycle/restore. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant Root as Root Makefile
participant FE as packages/deces-ui/Makefile
participant BE as packages/deces-backend/Makefile
participant INFRA as packages/deces-infra/Makefile
participant DC as docker compose
Dev->>Root: make frontend-dev / backend-dev / elasticsearch-restore
Note over Root: -include per-app Makefiles
alt Frontend flow
Root->>FE: delegate frontend-* targets
FE->>DC: up -f docker-compose-dev.yml
FE->>DC: build/push/test (as requested)
end
alt Backend flow
Root->>BE: delegate backend-* targets
BE->>DC: up -f ${BACKEND_PATH}/docker-compose*.yml
end
alt Infra (Elasticsearch)
Root->>INFRA: elasticsearch-*
INFRA->>DC: up -f docker-compose-elasticsearch.yml
INFRA->>INFRA: repo creds/config, restore (sync/async)
end
sequenceDiagram
autonumber
participant M as packages/deces-infra/Makefile
participant ES as deces-elasticsearch
participant S3 as Snapshot Repo
participant DC as docker compose
M->>DC: up -d (elasticsearch-start)
M->>ES: wait for API (elasticsearch-check)
M->>ES: put keystore creds (elasticsearch-repository-creds)
M->>ES: create snapshot repository (elasticsearch-repository-config)
M->>ES: restore snapshot (elasticsearch-restore / -async)
ES-->>M: status green / task id
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Pre-merge checks✅ Passed checks (3 passed)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/deces-ui/k8s/frontend.yaml (1)
17-19: Undefined DC_PREFIX in K8s manifest — container/image names will render emptyUse app-scoped identifiers. Replace DC_PREFIX with APP_FRONTEND for both container name and image.
Apply:
- - name: ${DC_PREFIX} - image: ${DOCKER_USERNAME}/${DC_PREFIX}:${APP_VERSION} + - name: ${APP_FRONTEND} + image: ${DOCKER_USERNAME}/${APP_FRONTEND}:${APP_VERSION}
🧹 Nitpick comments (9)
packages/deces-ui/docker-compose-test.yml (1)
4-4: Don’t hard-code test container_nameFixed names hinder concurrent test runs and CI matrix jobs. Let Compose auto-prefix or template it if really needed.
Suggested change:
- container_name: deces-ui-test + # container_name omitted to allow parallel runspackages/deces-ui/nginx/run.sh (1)
10-10: SC2166: Replace-owith||for portable checks; show missing vars
[ ... -o ... ]is not well-defined across shells. Use||or an if-block and echo which var is missing.-[ -z "${APP_FRONTEND}" -o -z "${BACKEND_PORT}" -o -z "${BACKEND_HOST}" -o -z "${BACKEND_PROXY_PATH}" ] && echo "missing some env var" && exit 1 +if [ -z "${APP_FRONTEND}" ] || [ -z "${BACKEND_PORT}" ] || [ -z "${BACKEND_HOST}" ] || [ -z "${BACKEND_PROXY_PATH}" ]; then + echo "missing some env var: APP_FRONTEND=${APP_FRONTEND:-}, BACKEND_PORT=${BACKEND_PORT:-}, BACKEND_HOST=${BACKEND_HOST:-}, BACKEND_PROXY_PATH=${BACKEND_PROXY_PATH:-}" >&2 + exit 1 +fipackages/deces-ui/docker-compose.yml (1)
13-13: Avoid fixed container_nameSame rationale as infra/test: fixed names block parallel stacks and CI.
- container_name: deces-ui + # container_name omitted to keep Compose project scopingpackages/deces-ui/Dockerfile (2)
19-21: Bug: wrong var in ENV; should use app_name
${APP_FRONTEND}isn’t defined at this point; use the build arg.-ENV APP_FRONTEND ${APP_FRONTEND} +ENV APP_FRONTEND ${app_name}
130-133: Use consistent vars when cleaning tarballMinor consistency: use APP_FRONTEND/APP_VERSION for the cleanup too.
- rm -rf ${app_name}-${app_ver}-frontend.tar.gz + rm -rf ${APP_FRONTEND}-${APP_VERSION}-frontend.tar.gzpackages/deces-ui/k8s/frontend.yaml (1)
24-26: Retain APP for backward compatibilitySeveral scripts likely still reference APP. Keep APP_FRONTEND, but also export APP derived from it.
- name: APP_FRONTEND value: "${APP_FRONTEND}" + - name: APP + value: "${APP_FRONTEND}"Makefile (1)
198-206: Mark dev/build as PHONYPrevents accidental file collisions; addresses checkmake warnings.
+.PHONY: dev dev-stop build stop start up down restartpackages/deces-ui/Makefile (2)
1-1: Add standard PHONY targets (all/clean/test) to satisfy checkmakeKeeps lint happy and clarifies intent.
+.PHONY: all clean test frontend frontend-build frontend-dev frontend-stop nginx-build clean-frontend +all: frontend-build +clean: clean-frontend +test: frontend-test
11-11: Wrap NGINX_CSP in double quotes to keep the CSP a single token
Export works in GNU Make, but shells/envsubst can split on spaces/semicolons — wrap NGINX_CSP in double quotes in packages/deces-ui/Makefile (line 11). Verified by exporting the quoted value locally; value preserved.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
Makefile(15 hunks)packages/deces-backend/Makefile(7 hunks)packages/deces-infra/Makefile(1 hunks)packages/deces-infra/docker-compose-elasticsearch.yml(1 hunks)packages/deces-ui/Dockerfile(3 hunks)packages/deces-ui/Makefile(1 hunks)packages/deces-ui/docker-compose-build.yml(2 hunks)packages/deces-ui/docker-compose-dev.yml(3 hunks)packages/deces-ui/docker-compose-test.yml(1 hunks)packages/deces-ui/docker-compose.yml(1 hunks)packages/deces-ui/k8s/backend.yaml(1 hunks)packages/deces-ui/k8s/frontend.yaml(4 hunks)packages/deces-ui/nginx/run.sh(1 hunks)packages/deces-ui/rollup.config.js(1 hunks)packages/deces-ui/src/components/tools/stores.js(1 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
packages/deces-ui/nginx/run.sh
[warning] 10-10: Prefer [ p ] || [ q ] as [ p -o q ] is not well defined.
(SC2166)
[warning] 10-10: Prefer [ p ] || [ q ] as [ p -o q ] is not well defined.
(SC2166)
[warning] 10-10: Prefer [ p ] || [ q ] as [ p -o q ] is not well defined.
(SC2166)
🪛 checkmake (0.2.2)
packages/deces-infra/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
[warning] 84-84: Target "elasticsearch" should be declared PHONY.
(phonydeclared)
packages/deces-ui/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
packages/deces-backend/Makefile
[warning] 236-236: Target "dev" should be declared PHONY.
(phonydeclared)
Makefile
[warning] 198-198: Target "dev" should be declared PHONY.
(phonydeclared)
[warning] 202-202: Target "build" should be declared PHONY.
(phonydeclared)
🔇 Additional comments (16)
packages/deces-ui/src/components/tools/stores.js (1)
522-523: LGTM: placeholder aligns with new APP_FRONTENDValue matches rollup replacement and the monorepo rename.
Please ensure APP_FRONTEND is set in the build env for both dev and prod to avoid leaking the literal placeholder at runtime.
packages/deces-ui/docker-compose.yml (2)
11-12: Confirm build arg and Dockerfile alignmentapp_name now passes ${APP_FRONTEND}. Ensure Dockerfile reads ARG app_name and sets ENV accordingly (see Dockerfile comments).
15-49: ES_PROXY_PATH removed — frontend checked; backend still references es_proxy_pathNo occurrences of ES_PROXY_PATH or ES_PROXY_PATH under packages/deces-ui. Backend references found — update if removal should be global:
- packages/dataprep-backend/conf/conf.yml:8 (es_proxy_path: '/matchID/api/v0/datasets')
- packages/dataprep-backend/conf/matchID-validation.yml:14 (host: es_proxy_path)
- packages/dataprep-backend/code/api.py:705, 733 (config.conf["global"]["api"]["es_proxy_path"])
packages/deces-ui/Dockerfile (2)
112-114: LGTM: build-stage APP_FRONTEND wiring is correctENV uses app_name here; consistent with compose build args.
128-129: Verify tarball name exists in build contextEnsure the Makefile produces
${APP_FRONTEND}-${APP_VERSION}-frontend.tar.gzand it’s included in the docker build context.packages/deces-ui/docker-compose-build.yml (2)
15-17: LGTM — build args aligned to APP_FRONTENDapp_path and app_name now consistent across the toolchain.
33-35: LGTM — deterministic image/container namesUsing deces-ui-frontend-build improves clarity and avoids prefix drift.
packages/deces-ui/docker-compose-dev.yml (2)
72-74: LGTM — dev build args migrated to APP_FRONTENDPath and name now match container filesystem layout used by the build.
98-104: LGTM — volume paths updated to ${APP_FRONTEND}Volumes now mount to the correct in-container app dir.
Makefile (2)
145-146: LGTM — tool bootstrap moved to tools/configRoot config centralizes on tools; sensible for monorepo orchestration.
122-126: Backend include intentionally commented — confirm BACKEND_TOKEN_ sources*
- Root Makefile exports BACKEND_PROXY_PATH, BACKEND_PORT, BACKEND_HOST (Makefile lines 50–53) — leaving -include ${BACKEND_PATH}/Makefile commented is OK.
- BACKEND_TOKEN_KEY and BACKEND_TOKEN_PASSWORD are referenced when invoking the job (Makefile lines 330–331) but aren’t defined in the root Makefile output above — confirm they come from the environment or the backend Makefile; if not, define them in the root Makefile or re-enable the include.
packages/deces-ui/Makefile (3)
60-71: LGTM — dev flow with commit gatingNice touch writing .lastcommit to avoid needless rebuilds.
129-136: LGTM — nginx build wiring looks correctCopies the dist tar into nginx context and builds with DC args.
148-153: Package the right directory — use APP_FRONTENDExclude list and tar root should match the in-repo app dir.
- ( cd ${FRONTEND_PATH} && tar -zcvf $(FILE_FRONTEND_APP_VERSION) --exclude ${APP}.tar.gz \ + ( cd ${FRONTEND_PATH} && tar -zcvf $(FILE_FRONTEND_APP_VERSION) --exclude ${APP_FRONTEND}.tar.gz \ .eslintrc.js \ rollup.config.js \ - src \ - public ) + src \ + public )Likely an incorrect or invalid review comment.
packages/deces-ui/k8s/frontend.yaml (1)
36-37: Do not rename BACKEND_APP → APP_BACKEND (inconsistent across manifests)BACKEND_APP is the canonical variable used by the backend deployment/service and exported env; frontend references it. Changing only frontend will break DNS/env consistency. Locations: packages/deces-ui/k8s/backend.yaml — (lines 4,10,14,18,27,76,85); packages/deces-ui/k8s/frontend.yaml — (lines 36–37). Either keep BACKEND_APP or rename it consistently across all manifests.
Likely an incorrect or invalid review comment.
packages/deces-backend/Makefile (1)
167-167: Resolved — APP_BACKEND matches compose container nameAPP_BACKEND is exported as "deces-backend" (Makefile:24,33) and packages/deces-backend/docker-compose.yml sets container_name: deces-backend (line 6); the docker exec usage in packages/deces-backend/Makefile (line 167) is valid.
|
|
||
| export PROOFS=${DATA_DIR}/proofs | ||
| export MONITOR_DIR = ${APP}/log/instances/${APP_GROUP}-${APP}-${GIT_BRANCH} | ||
| export MONITOR_DIR = ${APP_FRONTED}/log/instances/${APP_GROUP}-${APP_FRONTEND}-${GIT_BRANCH} |
There was a problem hiding this comment.
Typo breaks monitor path: APP_FRONTED → APP_FRONTEND
Directory construction is wrong as-is.
-export MONITOR_DIR = ${APP_FRONTED}/log/instances/${APP_GROUP}-${APP_FRONTEND}-${GIT_BRANCH}
+export MONITOR_DIR = ${APP_FRONTEND}/log/instances/${APP_GROUP}-${APP_FRONTEND}-${GIT_BRANCH}📝 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.
| export MONITOR_DIR = ${APP_FRONTED}/log/instances/${APP_GROUP}-${APP_FRONTEND}-${GIT_BRANCH} | |
| export MONITOR_DIR = ${APP_FRONTEND}/log/instances/${APP_GROUP}-${APP_FRONTEND}-${GIT_BRANCH} |
🤖 Prompt for AI Agents
In Makefile around line 70, the variable name used to build MONITOR_DIR has a
typo: APP_FRONTED should be APP_FRONTEND, which breaks the constructed path; fix
the typo so the export reads MONITOR_DIR =
${APP_FRONTEND}/log/instances/${APP_GROUP}-${APP_FRONTEND}-${GIT_BRANCH} (use
the correct APP_FRONTEND variable everywhere in this line).
| export SMTP_PORT?=1025 | ||
| export SMTP_USER?=${API_EMAIL} | ||
| export SMTP_PWD?= | ||
| export SMTP_PWD?=$(shell echo $$RANDOM ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Weak entropy for SMTP_PWD
Prefer cryptographically strong randomness.
-export SMTP_PWD?=$(shell echo $$RANDOM )
+export SMTP_PWD?=$(shell openssl rand -base64 16)📝 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.
| export SMTP_PWD?=$(shell echo $$RANDOM ) | |
| export SMTP_PWD?=$(shell openssl rand -base64 16) |
🤖 Prompt for AI Agents
In packages/deces-backend/Makefile around line 52, the SMTP_PWD default uses
weak entropy via $$RANDOM; replace that with a cryptographically strong
generator (for example use openssl rand -hex 16 or read from /dev/urandom piped
to base64) so the make default password is securely generated; update the shell
command in the assignment to call the stronger generator and optionally add a
fallback if openssl is unavailable (e.g., try openssl, then /dev/urandom) while
keeping the export syntax the same.
packages/deces-backend/Makefile
Outdated
| dev: network backend-dev-stop backend-dev | ||
|
|
There was a problem hiding this comment.
'dev' depends on undefined 'network' in this Makefile
Running make dev here fails. Proxy the network target to the root Makefile.
-dev: network backend-dev-stop backend-dev
+dev: network backend-dev-stop backend-dev
+
+.PHONY: network
+network:
+ @${MAKE} -C ${APP_PATH} network📝 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.
| dev: network backend-dev-stop backend-dev | |
| dev: network backend-dev-stop backend-dev | |
| .PHONY: network | |
| network: | |
| @${MAKE} -C ${APP_PATH} network |
🧰 Tools
🪛 checkmake (0.2.2)
[warning] 236-236: Target "dev" should be declared PHONY.
(phonydeclared)
🤖 Prompt for AI Agents
In packages/deces-backend/Makefile around lines 236-237, the dev target depends
on a non-existent local target 'network'; create a proxy 'network' target that
invokes the root Makefile's network target (e.g., add a phony target named
network that runs make -C .. network) so running make dev will call the root
network target first and then proceed with backend-dev-stop and backend-dev.
| elasticsearch: | ||
| image: docker.elastic.co/elasticsearch/elasticsearch:${ES_VERSION} | ||
| container_name: ${DC_PREFIX}-elasticsearch | ||
| container_name: deces-elasticsearch |
There was a problem hiding this comment.
Avoid fixed container_name to prevent collisions and ease parallel stacks
A hard-coded container_name disables Compose's project scoping and breaks parallel dev/CI runs. Prefer omitting it so Compose auto-names with the project prefix, or keep it templated.
Apply this diff to drop the hard-coded name:
- container_name: deces-elasticsearch
+ # container_name intentionally omitted to allow project‑scoped names📝 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.
| container_name: deces-elasticsearch | |
| # container_name intentionally omitted to allow project‑scoped names |
🤖 Prompt for AI Agents
In packages/deces-infra/docker-compose-elasticsearch.yml around line 9, the
service sets a fixed container_name which prevents Compose project scoping and
can cause container name collisions; remove the hard-coded container_name line
(or replace it with a templated name using an environment var like
${COMPOSE_PROJECT_NAME}_deces-elasticsearch) so Compose can auto-name containers
per project and support parallel stacks/CI runs.
| export ES_JAVA_OPTS=-Xms${ES_MEM} -Xmx${ES_MEM} | ||
| export ES_VERSION = 8.6.1 | ||
| export ES_BACKUP_BASENAME := esdata | ||
| export ES_MEM_KUBE?=$(shell echo -n ${ES_MEM} | sed 's/\s*m/Mi/') |
There was a problem hiding this comment.
Broken sed expression for ES_MEM_KUBE
sed '\s' isn’t portable. Convert “512m” → “512Mi” reliably.
-export ES_MEM_KUBE?=$(shell echo -n ${ES_MEM} | sed 's/\s*m/Mi/')
+export ES_MEM_KUBE?=$(shell echo -n ${ES_MEM} | sed -E 's/m$$/Mi/')📝 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.
| export ES_MEM_KUBE?=$(shell echo -n ${ES_MEM} | sed 's/\s*m/Mi/') | |
| export ES_MEM_KUBE?=$(shell echo -n ${ES_MEM} | sed -E 's/m$$/Mi/') |
🤖 Prompt for AI Agents
In packages/deces-infra/Makefile around line 20, the sed expression 's/\s*m/Mi/'
is non-portable; replace it with a reliable transformation that converts values
like "512m" or "512M" to "512Mi" (for example use a POSIX-safe sed that replaces
a trailing m or M: s/[mM]$/Mi/ or use shell/Make parameter expansion to append
'i' when the value ends with m/M). Ensure the replacement only affects a
trailing m/M and preserves other inputs unchanged.
| value: "${DATAGOUV_PROXY_PATH}" | ||
| - name: ES_URL | ||
| value: "${APP}-es-svc:${ES_PORT}" | ||
| value: "deces-es-svc:${ES_PORT}" |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify ES service name/namespace wiring
Switching to deces-es-svc couples the backend to a global service. Confirm the Service exists in ${KUBE_NAMESPACE} (or use full DNS/cross-namespace if not), and that the app expects host:port (not a full URL).
Run to verify presence across manifests:
🏁 Script executed:
#!/bin/bash
rg -nC2 -g 'packages/**/k8s/*.y?(a)ml' -e '\bkind:\s*Service\b' -e 'name:\s*deces-es-svc'Length of output: 148
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Searching YAML files under packages/*/k8s for 'deces-es-svc' or 'kind: Service'..."
find . -type f \( -iname '*.yml' -o -iname '*.yaml' \) -path './packages/*/k8s/*' -print0 | xargs -0 -r rg -nC2 -e 'deces-es-svc' -e 'kind:\s*Service' || true
echo
echo "2) Showing packages/deces-ui/k8s/backend.yaml (lines 1-140) if it exists..."
if [ -f packages/deces-ui/k8s/backend.yaml ]; then
sed -n '1,140p' packages/deces-ui/k8s/backend.yaml | nl -ba | sed -n '1,140p'
else
echo "packages/deces-ui/k8s/backend.yaml not found"
fi
echo
echo "3) Fallback: search entire repo for 'deces-es-svc'..."
rg -n --hidden -S 'deces-es-svc' || trueLength of output: 1680
Confirm ES service name/namespace wiring
- packages/deces-ui/k8s/backend.yaml (line 69) sets ES_URL = "deces-es-svc:${ES_PORT}" — no Service named "deces-es-svc" found in packages/*/k8s manifests.
- packages/deces-ui/k8s/elasticsearch.yaml defines a Service named "elasticsearch".
Action: update ES_URL to "elasticsearch:${ES_PORT}", or add/rename a Service to "deces-es-svc" in the same namespace, or use full cross-namespace DNS (deces-es-svc..svc.cluster.local:port). Confirm the app expects host:port (not a full HTTP URL).
🤖 Prompt for AI Agents
In packages/deces-ui/k8s/backend.yaml around line 69 the ES_URL is set to
"deces-es-svc:${ES_PORT}" but no Service named deces-es-svc exists in
packages/*/k8s manifests (the Service defined in
packages/deces-ui/k8s/elasticsearch.yaml is named "elasticsearch"); update
ES_URL to point to the actual service (e.g., "elasticsearch:${ES_PORT}"), or
add/rename a Service to "deces-es-svc" in the same namespace, or use a full
cross-namespace DNS name like
"deces-es-svc.<namespace>.svc.cluster.local:${ES_PORT}"; also confirm the
application expects host:port (not an http:// URL) and adjust the value
accordingly.
| export FILE_FRONTEND_APP_VERSION = $(APP)-$(APP_VERSION)-frontend.tar.gz | ||
| export FILE_FRONTEND_DIST_APP_VERSION = $(APP)-$(APP_VERSION)-frontend-dist.tar.gz | ||
| export FILE_FRONTEND_DIST_LATEST_VERSION = $(APP)-latest-frontend-dist.tar.gz |
There was a problem hiding this comment.
Tarball names still use
Artifacts will be named with “deces-…”, inconsistent with the new monorepo naming.
-export FILE_FRONTEND_APP_VERSION = $(APP)-$(APP_VERSION)-frontend.tar.gz
-export FILE_FRONTEND_DIST_APP_VERSION = $(APP)-$(APP_VERSION)-frontend-dist.tar.gz
-export FILE_FRONTEND_DIST_LATEST_VERSION = $(APP)-latest-frontend-dist.tar.gz
+export FILE_FRONTEND_APP_VERSION = $(APP_FRONTEND)-$(APP_VERSION)-frontend.tar.gz
+export FILE_FRONTEND_DIST_APP_VERSION = $(APP_FRONTEND)-$(APP_VERSION)-frontend-dist.tar.gz
+export FILE_FRONTEND_DIST_LATEST_VERSION = $(APP_FRONTEND)-latest-frontend-dist.tar.gz📝 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.
| export FILE_FRONTEND_APP_VERSION = $(APP)-$(APP_VERSION)-frontend.tar.gz | |
| export FILE_FRONTEND_DIST_APP_VERSION = $(APP)-$(APP_VERSION)-frontend-dist.tar.gz | |
| export FILE_FRONTEND_DIST_LATEST_VERSION = $(APP)-latest-frontend-dist.tar.gz | |
| export FILE_FRONTEND_APP_VERSION = $(APP_FRONTEND)-$(APP_VERSION)-frontend.tar.gz | |
| export FILE_FRONTEND_DIST_APP_VERSION = $(APP_FRONTEND)-$(APP_VERSION)-frontend-dist.tar.gz | |
| export FILE_FRONTEND_DIST_LATEST_VERSION = $(APP_FRONTEND)-latest-frontend-dist.tar.gz |
🤖 Prompt for AI Agents
In packages/deces-ui/Makefile around lines 17 to 19, the tarball variable
definitions still use $(APP) which yields old "deces-…" names; replace $(APP)
with $(APP_FRONTEND) in each exported variable (FILE_FRONTEND_APP_VERSION,
FILE_FRONTEND_DIST_APP_VERSION, FILE_FRONTEND_DIST_LATEST_VERSION) so artifacts
use the monorepo frontend app name; also ensure APP_FRONTEND (and APP_VERSION if
different) is defined or available in the Makefile or included makefiles.
| PLAYWRIGHT_VERSION=$$(curl -s https://mcr.microsoft.com/v2/playwright/tags/list | jq -r '.tags | map(select(test("^v[0-9]+\\.[0-9]+\\.[0-9]+$$"))) | .[]' | sort -V | tail -1 | sed 's/^v//') ${DC} -f ${DC_FILE}-test.yml run ui-test sh -c "yarn install && node runAllTests.js" | ||
|
|
There was a problem hiding this comment.
frontend-test references undefined DC_FILE
Use the frontend’s test compose file directly.
- PLAYWRIGHT_VERSION=$$(curl -s https://mcr.microsoft.com/v2/playwright/tags/list | jq -r '.tags | map(select(test("^v[0-9]+\\.[0-9]+\\.[0-9]+$$"))) | .[]' | sort -V | tail -1 | sed 's/^v//') ${DC} -f ${DC_FILE}-test.yml run ui-test sh -c "yarn install && node runAllTests.js"
+ PLAYWRIGHT_VERSION=$$(curl -s https://mcr.microsoft.com/v2/playwright/tags/list | jq -r '.tags | map(select(test("^v[0-9]+\\.[0-9]+\\.[0-9]+$$"))) | .[]' | sort -V | tail -1 | sed 's/^v//') ${DC} -f ${FRONTEND_PATH}/docker-compose-test.yml run ui-test sh -c "yarn install && node runAllTests.js"📝 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.
| PLAYWRIGHT_VERSION=$$(curl -s https://mcr.microsoft.com/v2/playwright/tags/list | jq -r '.tags | map(select(test("^v[0-9]+\\.[0-9]+\\.[0-9]+$$"))) | .[]' | sort -V | tail -1 | sed 's/^v//') ${DC} -f ${DC_FILE}-test.yml run ui-test sh -c "yarn install && node runAllTests.js" | |
| PLAYWRIGHT_VERSION=$$(curl -s https://mcr.microsoft.com/v2/playwright/tags/list | jq -r '.tags | map(select(test("^v[0-9]+\\.[0-9]+\\.[0-9]+$$"))) | .[]' | sort -V | tail -1 | sed 's/^v//') ${DC} -f ${FRONTEND_PATH}/docker-compose-test.yml run ui-test sh -c "yarn install && node runAllTests.js" |
🤖 Prompt for AI Agents
In packages/deces-ui/Makefile around lines 102-103, the frontend test target
uses an undefined DC_FILE variable; change the command to reference the
frontend's test compose file directly (for example
docker-compose.frontend-test.yml or docker-compose.test.yml) instead of
${DC_FILE}-test.yml so the compose file is resolvable, keeping the existing
${DC} invocation and the PLAYWRIGHT_VERSION logic intact.
| ${DC} -f $(DC_BUILD_FRONTEND) run -T --rm frontend-build tar czf - $$(basename /$(APP)/public) -C $$(dirname /$(APP)/public) > $(BUILD_DIR)/$(FILE_FRONTEND_DIST_APP_VERSION) | ||
| cp $(BUILD_DIR)/$(FILE_FRONTEND_DIST_APP_VERSION) $(BUILD_DIR)/$(FILE_FRONTEND_DIST_LATEST_VERSION) | ||
| if [ -f $(BUILD_DIR)/$(FILE_FRONTEND_DIST_APP_VERSION) ]; then ls -alsrt $(BUILD_DIR)/$(FILE_FRONTEND_DIST_APP_VERSION) && sha1sum $(BUILD_DIR)/$(FILE_FRONTEND_DIST_APP_VERSION) ; fi | ||
| if [ -f $(BUILD_DIR)/$(FILE_FRONTEND_DIST_LATEST_VERSION) ]; then ls -alsrt $(BUILD_DIR)/$(FILE_FRONTEND_DIST_LATEST_VERSION) && sha1sum $(BUILD_DIR)/$(FILE_FRONTEND_DIST_LATEST_VERSION) ; fi | ||
|
|
There was a problem hiding this comment.
Build-dist tar uses /$(APP)/public inside container
The container paths were migrated to /${APP_FRONTEND}. Fix both basename and dirname.
- ${DC} -f $(DC_BUILD_FRONTEND) run -T --rm frontend-build tar czf - $$(basename /$(APP)/public) -C $$(dirname /$(APP)/public) > $(BUILD_DIR)/$(FILE_FRONTEND_DIST_APP_VERSION)
+ ${DC} -f $(DC_BUILD_FRONTEND) run -T --rm frontend-build tar czf - $$(basename /$(APP_FRONTEND)/public) -C $$(dirname /$(APP_FRONTEND)/public) > $(BUILD_DIR)/$(FILE_FRONTEND_DIST_APP_VERSION)📝 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.
| ${DC} -f $(DC_BUILD_FRONTEND) run -T --rm frontend-build tar czf - $$(basename /$(APP)/public) -C $$(dirname /$(APP)/public) > $(BUILD_DIR)/$(FILE_FRONTEND_DIST_APP_VERSION) | |
| cp $(BUILD_DIR)/$(FILE_FRONTEND_DIST_APP_VERSION) $(BUILD_DIR)/$(FILE_FRONTEND_DIST_LATEST_VERSION) | |
| if [ -f $(BUILD_DIR)/$(FILE_FRONTEND_DIST_APP_VERSION) ]; then ls -alsrt $(BUILD_DIR)/$(FILE_FRONTEND_DIST_APP_VERSION) && sha1sum $(BUILD_DIR)/$(FILE_FRONTEND_DIST_APP_VERSION) ; fi | |
| if [ -f $(BUILD_DIR)/$(FILE_FRONTEND_DIST_LATEST_VERSION) ]; then ls -alsrt $(BUILD_DIR)/$(FILE_FRONTEND_DIST_LATEST_VERSION) && sha1sum $(BUILD_DIR)/$(FILE_FRONTEND_DIST_LATEST_VERSION) ; fi | |
| ${DC} -f $(DC_BUILD_FRONTEND) run -T --rm frontend-build tar czf - $$(basename /$(APP_FRONTEND)/public) -C $$(dirname /$(APP_FRONTEND)/public) > $(BUILD_DIR)/$(FILE_FRONTEND_DIST_APP_VERSION) | |
| cp $(BUILD_DIR)/$(FILE_FRONTEND_DIST_APP_VERSION) $(BUILD_DIR)/$(FILE_FRONTEND_DIST_LATEST_VERSION) | |
| if [ -f $(BUILD_DIR)/$(FILE_FRONTEND_DIST_APP_VERSION) ]; then ls -alsrt $(BUILD_DIR)/$(FILE_FRONTEND_DIST_APP_VERSION) && sha1sum $(BUILD_DIR)/$(FILE_FRONTEND_DIST_APP_VERSION) ; fi | |
| if [ -f $(BUILD_DIR)/$(FILE_FRONTEND_DIST_LATEST_VERSION) ]; then ls -alsrt $(BUILD_DIR)/$(FILE_FRONTEND_DIST_LATEST_VERSION) && sha1sum $(BUILD_DIR)/$(FILE_FRONTEND_DIST_LATEST_VERSION) ; fi |
🤖 Prompt for AI Agents
In packages/deces-ui/Makefile around lines 155 to 159, the build step still
references the old container path /$(APP)/public; update both basename and
dirname invocations to use the new container path /${APP_FRONTEND}/public so the
tar command targets the correct directory inside the container (replace
$$(basename /$(APP)/public) with $$(basename /${APP_FRONTEND}/public) and
$$(dirname /$(APP)/public) with $$(dirname /${APP_FRONTEND}/public)); keep the
rest of the command and file copying/checksum logic unchanged.
| __APP_FRONTEND__: process.env.APP_FRONTEND, | ||
| __APP_VERSION__: process.env.APP_VERSION, | ||
| __THEME_DNUM__: process.env.THEME_DNUM | ||
| }), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden replace() usage
- Add preventAssignment to avoid accidental replacements.
- Optional: run replace() before svelte() so placeholders inside .svelte files are also replaced.
Minimal change:
- replace({
+ replace({
+ preventAssignment: true,
__AB_THRESHOLD__: process.env.AB_THRESHOLD,
__API_EMAIL__: process.env.API_EMAIL,Optional reordering (no diff shown): move replace() above svelte() in the plugins array.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/deces-ui/rollup.config.js around lines 46 to 49 the replace() plugin
invocation is missing the preventAssignment option which can cause accidental
replacements; update the replace(...) call to include preventAssignment: true in
its options object, and optionally move the replace(...) plugin earlier in the
plugins array (before svelte()) so replacements inside .svelte files are handled
as well.
rhanka
force-pushed
the
feat/refacto-make
branch
from
97eae3d to
997945a
Compare
1 participant
Makefile target structure:
Scope for PR:
Goal: make dev working (no work on deploy)
Detailed work
Summary by CodeRabbit
Refactor
New Features
Security
Performance
Tests
Chores