Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 WalkthroughReplaces Moon orchestration with a Yarn + Turbo toolchain: removes Moon configs and tasks, adds Changes
Sequence Diagram(s)sequenceDiagram
participant GH as GitHub Actions
participant Node as Node / Yarn
participant Turbo as Turbo
participant Pruner as Turbo Pruner
participant Docker as Docker Builder
participant Registry as Artifact Registry
GH->>Node: setup-node (node 24) + cache yarn
GH->>Node: yarn install --immutable
Node->>Turbo: yarn turbo run build --filter=<package>
Turbo->>Pruner: turbo prune -> produce out/json & out/full
Pruner->>Docker: provide pruned outputs (out/json, out/full, tsconfig.options.json)
Docker->>Node: yarn install (consume pruner outputs)
Docker->>Turbo: yarn turbo run build --filter=<package>
Docker->>Registry: produce/push artifacts or images
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/tsconfig.json (1)
4-16:⚠️ Potential issue | 🟡 MinorDuplicate project references should be cleaned up.
../libs/generatedand../libs/graphqlare each listed twice (lines 6+12 and 9+15). TypeScript silently deduplicates them, but the redundancy is confusing.🧹 Proposed cleanup
"references": [ { "path": "../libs/generated" }, - { "path": "../libs/graphql" }, - { "path": "../libs/generated" }, - { "path": "../libs/graphql" } + { "path": "../libs/graphql" } ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/tsconfig.json` around lines 4 - 16, The references array contains duplicate project entries for "../libs/generated" and "../libs/graphql"; remove the duplicate objects so each path appears only once in the "references" array (keep a single { "path": "../libs/generated" } and a single { "path": "../libs/graphql" }) to clean up redundancy and avoid confusion in tsconfig.json.apps/frontend/tsconfig.json (1)
17-17:⚠️ Potential issue | 🟡 Minor
outDiris dead code —noEmit: truesuppresses all output.
noEmit: true(Line 17) causes TypeScript to skip writing any files, makingoutDir: "dist"(Line 30) a no-op. This appears to be a mechanical substitution of the old Moon cache path that didn't account for thenoEmitflag. TheoutDirkey can be safely removed.🔧 Proposed fix
- "outDir": "dist",Also applies to: 30-30
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/frontend/tsconfig.json` at line 17, The tsconfig contains "noEmit": true which prevents any output, so the "outDir": "dist" entry is dead code; remove the "outDir" property (and any duplicate "outDir" entries) from the tsconfig while keeping "noEmit": true intact, ensuring no other build logic depends on outDir; search for the keys "noEmit" and "outDir" to locate and delete the redundant "outDir" lines.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/docs/src/contributing.md`:
- Line 7: Update the prerequisites list in contributing.md to include Node.js
and Yarn so contributors don't hit "yarn: command not found"; specifically, add
a sentence stating required Node.js (e.g., >=16 or the project's required
version) and Yarn (or Yarn Classic/berry as used) before the existing Rust and
Caddy line, and mention that these are needed to run the frontend command `yarn
turbo run dev --filter=@ryot/frontend`; ensure the wording clarifies these must
be pre-installed (or link to install instructions) rather than assumed
auto-managed by Moon/Proto.
In `@libs/transactional/package.json`:
- Line 8: The build script currently runs "email export && cp -r out/.
../../crates/services/notification/templates", which causes the cp side-effect
to be skipped on Turbo cache hits (out/** is a Turbo output but the templates
dir is not), leaving templates stale; change the "build" npm script to only run
"email export" and move the "cp -r out/.
../../crates/services/notification/templates" step into CI as a separate step
that always runs after the Turbo build (ensuring the copy always executes even
on cache hits), referencing the "build" script, the "email export" command, the
"out/**" artifact and the "../../crates/services/notification/templates"
destination when making the change.
In `@package.json`:
- Around line 4-6: The engines.node field currently pins an exact patch ("node":
"24.4.0") causing warnings for other patch/minor versions; update the
package.json engines.node to a permissive range (e.g., ">=24.4.0 <25.0.0" or
"^24.4.0") so contributors/CI using 24.x patches won’t get engine mismatch
warnings—modify the "engines" object in package.json (engines.node) accordingly
and run npm/yarn install to confirm no warnings.
In `@turbo.json`:
- Around line 8-17: The turbo task definitions for build-chrome and
build-firefox are missing workspace dependency wiring so running them directly
will skip building dependent packages; update the task entries "build-chrome"
and "build-firefox" to include dependsOn: ["^build"] so the
parent/workspace-level build runs first, ensuring all dependencies are built
before these tasks execute.
---
Outside diff comments:
In `@apps/frontend/tsconfig.json`:
- Line 17: The tsconfig contains "noEmit": true which prevents any output, so
the "outDir": "dist" entry is dead code; remove the "outDir" property (and any
duplicate "outDir" entries) from the tsconfig while keeping "noEmit": true
intact, ensuring no other build logic depends on outDir; search for the keys
"noEmit" and "outDir" to locate and delete the redundant "outDir" lines.
In `@tests/tsconfig.json`:
- Around line 4-16: The references array contains duplicate project entries for
"../libs/generated" and "../libs/graphql"; remove the duplicate objects so each
path appears only once in the "references" array (keep a single { "path":
"../libs/generated" } and a single { "path": "../libs/graphql" }) to clean up
redundancy and avoid confusion in tsconfig.json.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/website/Dockerfile (1)
29-29:⚠️ Potential issue | 🟡 MinorCMD shell form prevents graceful shutdown.
Shell form (
CMD mkdir -p tmp && ...) runs the process under/bin/sh -c, so PID 1 is the shell — not the Node process.SIGTERMsent bydocker stopwon't be forwarded, causing a hard kill after the timeout. Use exec form orexecto replace the shell process.Proposed fix (option A – exec form with a small entrypoint)
-CMD mkdir -p tmp && npx react-router-serve ./build/server/index.js +RUN mkdir -p tmp +CMD ["npx", "react-router-serve", "./build/server/index.js"]If
tmpmust be created at runtime (e.g., it's on an ephemeral volume), useexec:-CMD mkdir -p tmp && npx react-router-serve ./build/server/index.js +CMD ["sh", "-c", "mkdir -p tmp && exec npx react-router-serve ./build/server/index.js"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/website/Dockerfile` at line 29, The current Dockerfile CMD uses shell form ("CMD mkdir -p tmp && npx react-router-serve ./build/server/index.js") so PID 1 is /bin/sh and signals aren't forwarded; change to an exec-style start so the Node process becomes PID 1. Remove the inline shell chaining and either (a) create the tmp directory at image build time (replace the CMD with an exec-form array that runs npx react-router-serve ./build/server/index.js) or (b) add a tiny entrypoint script that runs mkdir -p tmp then uses exec to replace the shell with the npx process; in either case ensure the final process launched is npx react-router-serve ./build/server/index.js executed via exec form so SIGTERM is forwarded.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/browser-extension/tsconfig.json`:
- Around line 6-8: The tsconfig sets "allowImportingTsExtensions": true while
also specifying "outDir": "dist", which violates TS constraint TS5096; either
make this tsconfig for type-check-only by adding "noEmit": true (or
"emitDeclarationOnly": true) to the compilerOptions, or remove
"allowImportingTsExtensions" so emitting to "outDir" is allowed—update the
compilerOptions in tsconfig.json accordingly (refer to the keys
"allowImportingTsExtensions", "noEmit", "emitDeclarationOnly", and "outDir").
In `@apps/website/Dockerfile`:
- Line 5: Replace the unstable end-user "apt" invocation in the Dockerfile RUN
layer with the scriptable "apt-get": call "apt-get update && apt-get install -y
--no-install-recommends git curl ca-certificates xz-utils" and then remove
package lists to reduce layer size (e.g. append "&& rm -rf /var/lib/apt/lists/*"
and/or "&& apt-get clean") in the same RUN line so the update, install, and
cleanup occur in one layer; update the RUN line that currently uses "apt update
&& apt install -y --no-install-recommends git curl ca-certificates xz-utils"
accordingly.
- Around line 7-11: The Dockerfile currently installs Turbo globally without a
version, causing non-reproducible builds; update the website-pruner stage's
global install command to pin Turbo to the project's declared version
(turbo@2.8.10) so replace the existing global install (in the website-pruner
stage / the RUN npm install -g turbo line) with a pinned install (npm install -g
turbo@2.8.10).
In `@Dockerfile`:
- Line 9: Pin the globally installed turbo binary to a specific version instead
of installing latest; update the RUN instruction that currently reads `RUN npm
install -g turbo` to install a fixed release (e.g., `turbo@<version>`). Modify
the Dockerfile's `RUN npm install -g turbo` line to use the chosen version so
builds are reproducible and consistent with the pinned `concurrently@9.1.2`
usage elsewhere.
- Around line 3-5: Update the build stage declaration and package install to use
the build platform and proper APT cleanup: change the FROM line for the
frontend-build-base stage to include --platform=$BUILDPLATFORM, and replace the
`apt update && apt install -y --no-install-recommends git curl ca-certificates
xz-utils` step with an `apt-get update` followed by `apt-get install -y
--no-install-recommends ...` and immediately remove APT caches (e.g., rm -rf
/var/lib/apt/lists/*) in the same RUN layer to reduce image size and satisfy
Hadolint rules.
In `@tests/tsconfig.json`:
- Line 11: The tests tsconfig removed "noEmit" and added "outDir": "dist",
causing tsc (invoked by the typecheck script running `tsc --build --verbose`) to
attempt JS emission during test typechecks; either restore `"noEmit": true` in
the tests tsconfig or modify the `typecheck` script to add `--noEmit` so no
artifacts are emitted, and ensure the Turbo `typecheck` task explicitly
documents outputs if you intended to produce build outputs (otherwise leave
outputs empty); update the `tests/tsconfig.json` (re-add noEmit) or
`tests/package.json` (add `--noEmit` to the typecheck command) accordingly.
---
Outside diff comments:
In `@apps/website/Dockerfile`:
- Line 29: The current Dockerfile CMD uses shell form ("CMD mkdir -p tmp && npx
react-router-serve ./build/server/index.js") so PID 1 is /bin/sh and signals
aren't forwarded; change to an exec-style start so the Node process becomes PID
1. Remove the inline shell chaining and either (a) create the tmp directory at
image build time (replace the CMD with an exec-form array that runs npx
react-router-serve ./build/server/index.js) or (b) add a tiny entrypoint script
that runs mkdir -p tmp then uses exec to replace the shell with the npx process;
in either case ensure the final process launched is npx react-router-serve
./build/server/index.js executed via exec form so SIGTERM is forwarded.
---
Duplicate comments:
In `@turbo.json`:
- Around line 8-19: The review noted that the tasks "build-chrome" and
"build-firefox" needed to declare their dependency on the root build task;
ensure the "dependsOn" entries in the turbo.json tasks "build-chrome" and
"build-firefox" are set to ["^build"] (as already corrected), keep their
"outputs" arrays unchanged, and commit/approve the change so the CI will run the
build graph correctly.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/website/Dockerfile (1)
24-30:⚠️ Potential issue | 🟠 MajorMissing
USER ryot— container runs as root.The user
ryotis created and files are--chown'd to it, but there's noUSER ryotdirective beforeCMD. The container will run as root at runtime. The rootDockerfilecorrectly setsUSER ryot(line 42) before itsCMD.Proposed fix
COPY --chown=ryot:ryot apps/website/app/drizzle/migrations app/drizzle/migrations +USER ryot CMD mkdir -p tmp && npx react-router-serve ./build/server/index.js🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/website/Dockerfile` around lines 24 - 30, The Dockerfile creates user ryot (RUN useradd -m -u 1001 ryot) and chowns app files, but never switches to that user so the container runs as root; add a USER ryot directive before the CMD (so the CMD mkdir -p tmp && npx react-router-serve ./build/server/index.js runs as ryot) and ensure the tmp directory is either created/chowned to ryot earlier (or create it after switching users) so runtime writes succeed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/website/Dockerfile`:
- Around line 24-30: The Dockerfile creates user ryot (RUN useradd -m -u 1001
ryot) and chowns app files, but never switches to that user so the container
runs as root; add a USER ryot directive before the CMD (so the CMD mkdir -p tmp
&& npx react-router-serve ./build/server/index.js runs as ryot) and ensure the
tmp directory is either created/chowned to ryot earlier (or create it after
switching users) so runtime writes succeed.
---
Duplicate comments:
In `@apps/website/Dockerfile`:
- Line 9: Replace the unpinned global install "RUN npm install -g turbo" with a
pinned install that matches the version declared in the repo (e.g.,
turbo@<version>), so the Dockerfile uses the same project-declared turbo version
(from package.json devDependencies/dependencies); update the RUN line to "npm
install -g turbo@<project_version>" (or the exact semver string found in
package.json) and keep it in sync with the package.json entry.
- Line 5: Replace the unstable end-user "apt" invocation in the Dockerfile RUN
command with "apt-get" and ensure package lists are cleaned after install;
update the RUN that installs git/curl/ca-certificates/xz-utils (the Dockerfile
RUN line) to use apt-get update && apt-get install -y --no-install-recommends
... and then run apt-get clean and remove /var/lib/apt/lists/* to satisfy
Hadolint DL3027/DL3009.
In `@Dockerfile`:
- Line 9: The Dockerfile currently does RUN npm install -g turbo which pulls the
latest turbo and can cause non-reproducible builds and a mismatch with the
repo's devDependency; change this to install a pinned turbo version that matches
package.json (e.g., turbo@<version-from-root-devDependencies>) and/or use the
workspace-local turbo instead of a global install—replace the global install
step with a pinned install (RUN npm install -g turbo@<root-version>) or remove
it and invoke the local turbo via yarn turbo in the prune/builder stages so the
global turbo and the builder's yarn turbo stay consistent.
- Around line 3-5: Update the frontend-build base stage declaration and apt
usage: add --platform=$BUILDPLATFORM to the FROM line that reads "FROM
$NODE_BASE_IMAGE AS frontend-build-base" and replace the RUN line "RUN apt
update && apt install -y --no-install-recommends git curl ca-certificates
xz-utils" with apt-get invocations and list cleanup (e.g., use "apt-get update
&& apt-get install -y --no-install-recommends ... && rm -rf
/var/lib/apt/lists/*") so the build uses the build platform and satisfies
Hadolint DL3027/DL3009.
In `@libs/transactional/package.json`:
- Around line 4-10: The scripts separation is already correct: keep "build" as
the pure build step (script name "build" running "email export") and retain the
side-effect copy as a separate script ("copy-templates"); no code changes
required—ensure CI invokes "copy-templates" after "build" so the template
side-effect is preserved.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/frontend/app/routes/auth.tsx`:
- Around line 210-211: The local ref "parent" returned from useAutoAnimate (a
RefCallback<Element>) is being cast to any for the Form ref; replace that coarse
cast with a specific React ref type to avoid the biome-ignore. Change the ref
prop from "ref={parent as any}" to "ref={parent as React.Ref<HTMLFormElement>}"
(or "ref={parent as (instance: HTMLFormElement | null) => void}" if you prefer
the callback signature) on the Form component so the types align between
useAutoAnimate's callback and the Form's expected Ref<HTMLFormElement>, and then
remove the biome-ignore comment.
In `@apps/website/Dockerfile`:
- Line 16: Replace the existing RUN yarn install invocation in the Dockerfile
with RUN yarn install --immutable to enforce lockfile integrity during the
Docker build (update the RUN yarn install line to RUN yarn install --immutable
so the build fails if the lockfile is out of sync with the pruned dependencies).
- Line 31: Move the mkdir -p tmp step into the image build and call the
react-router-serve binary directly to avoid the shell wrapper: add a RUN that
creates the tmp directory (and chown if needed) before the USER directive so
ownership is correct, and replace the CMD ["sh", "-c", "mkdir -p tmp && exec npx
react-router-serve ./build/server/index.js"] with an exec-form CMD that invokes
the installed binary (react-router-serve) path directly (no npx or sh) so the
Node process receives signals cleanly.
In `@libs/graphql/tsconfig.json`:
- Around line 10-11: The tsconfig contains "noEmit": true together with
"outDir": "dist", which makes the outDir setting inert; remove the dead
configuration by deleting the "outDir": "dist" entry (and similarly remove
"outDir" from other affected tsconfig files) so the compiler options reflect
actual behavior—search for the "noEmit" and "outDir" keys in the tsconfig files
and remove only the "outDir" lines while leaving "noEmit": true intact.
In `@libs/ts-utils/tsconfig.json`:
- Line 5: The tsconfig compilerOptions currently sets "outDir": "dist" but is
missing "noEmit": true causing tsc to actually emit JS; update the libs/ts-utils
tsconfig by adding "noEmit": true to the compilerOptions (next to the existing
outDir setting) so the project performs type-check-only like the other
tsconfigs; ensure the new key is a boolean under the same compilerOptions object
and commit the change.
In `@turbo.json`:
- Around line 1-37: AGENTS.md still shows Moon commands which are now invalid;
update the file to use Turbo equivalents by replacing Moon invocations (e.g.,
"moon run X" or "moon X") with Turbo/Yarn invocations (e.g., "yarn turbo run
typecheck", "yarn turbo run build", "yarn turbo run test" or the specific task
names defined in turbo.json such as "typecheck", "build", "build-chrome",
"build-firefox", "dev", "test-watch", "backend-graphql"); search for occurrences
of "moon run" and "moon" in AGENTS.md and replace them with the matching "yarn
turbo run <task>" command examples, update any task names that were namespaced
(like "frontend:typecheck" -> "typecheck" or map to the proper workspace task),
and ensure the examples show using Yarn (or pnpm/npm if preferred) to run Turbo
so readers can run the same tasks defined in turbo.json.
---
Duplicate comments:
In `@apps/browser-extension/tsconfig.json`:
- Around line 6-7: The TS config previously triggered TS5096 because
allowImportingTsExtensions was used without noEmit; ensure "noEmit": true
remains paired with "allowImportingTsExtensions": true in the
apps/browser-extension tsconfig to avoid the TS5096 error, and optionally remove
or ignore the inert "outDir": "dist" entry if you want to eliminate the cosmetic
noEmit+outDir pattern; update the tsconfig so "noEmit" and
"allowImportingTsExtensions" are both present and consistent.
In `@apps/docs/tsconfig.json`:
- Around line 5-6: The tsconfig contains redundant compiler options: "noEmit":
true makes "outDir" inert; remove the "outDir" property (or set "noEmit" to
false if you intend to emit) so only the appropriate behavior remains—update the
tsconfig JSON by deleting the "outDir" entry or adjust "noEmit" accordingly to
reflect the intended build output.
In `@apps/website/Dockerfile`:
- Line 9: The Dockerfile currently runs an unpinned global install (RUN npm
install -g turbo); change this to pin the same Turbo version declared in
package.json (turbo@2.8.10) to satisfy Hadolint DL3016 and prior review: update
the RUN command to install turbo@2.8.10 globally so the Docker build uses the
exact version referenced in package.json.
- Line 5: The Dockerfile RUN line uses apt without cleanup; change the RUN that
currently performs "apt update && apt install -y --no-install-recommends git
curl ca-certificates xz-utils" to use apt-get (apt-get update && apt-get install
-y --no-install-recommends ...) and ensure you add the apt cache cleanup steps
(apt-get clean and remove /var/lib/apt/lists/*) in the same RUN layer so
temporary apt files are deleted and DL3027/DL3009 are resolved.
In `@libs/transactional/tsconfig.json`:
- Around line 10-11: The tsconfig contains a redundant "outDir" while "noEmit":
true, so remove the inert setting by either deleting "outDir" from the JSON or
switching "noEmit" to false depending on intended behavior; update the
libs/transactional/tsconfig.json to keep only the correct combination for your
build (adjust "noEmit" or remove "outDir") and ensure the change targets the
"noEmit" and "outDir" properties referenced in the file.
In `@tests/tsconfig.json`:
- Around line 6-8: The tsconfig.json in tests declares "noEmit": true alongside
"outDir": "dist", which is redundant because the compiler ignores outDir when
noEmit is true; remove the unused "outDir": "dist" entry from
tests/tsconfig.json (or set "noEmit" to false if you intend to emit) so the
configuration is clear—look for the "noEmit" and "outDir" keys in
tests/tsconfig.json and delete or adjust "outDir" accordingly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Transition from Moon to Turbo for task management and build processes. Update configurations and scripts accordingly to streamline development workflows.
Summary by CodeRabbit
Chores
Documentation