feat(apps): export ENGINE_VERSION from definition layer#40183
feat(apps): export ENGINE_VERSION from definition layer#40183d-gubert wants to merge 2 commits intofeat/apps-engine-splitfrom
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
WalkthroughThe changes refactor Apps Engine version detection from dynamic runtime filesystem operations to a compile-time constant. A new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feat/apps-engine-split #40183 +/- ##
=======================================================
Coverage 69.81% 69.81%
=======================================================
Files 3296 3296
Lines 119173 119173
Branches 21479 21491 +12
=======================================================
+ Hits 83197 83205 +8
+ Misses 32668 32652 -16
- Partials 3308 3316 +8
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
d-gubert
changed the title
d-gubert
force-pushed
the
feat/apps-engine-split--pr1-engine-version
branch
from
bdb5d73 to
2ab8b32
Compare
d-gubert
force-pushed
the
feat/apps-engine-split--pr1-engine-version
branch
from
a4a14c1 to
6ea43b5
Compare
Adds `src/definition/version.ts` which reads the package version via
`resolveJsonModule` and exports it as `ENGINE_VERSION`.
Replaces `AppPackageParser.getEngineVersion()` — which resolved the
version by traversing the filesystem relative to `__dirname` — with a
direct import of `ENGINE_VERSION`. This removes the assumption that
`package.json` lives at a predictable relative path, which will break
when `AppPackageParser` moves to a different package during the
apps-engine split.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
fix(apps-engine): fix ENGINE_VERSION runtime path in compiled output
Static `import { version } from '../../package.json'` resolves correctly
during TypeScript compilation (source lives at src/definition/) but the
emitted require('../../package.json') exits the package root at runtime
once compiled to definition/version.js (outDir='.', rootDir='./src').
Switching to require('../package.json') — which is the correct path
relative to the compiled output — and bypassing TypeScript's compile-time
module resolution avoids the path mismatch entirely.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
d-gubert
force-pushed
the
feat/apps-engine-split--pr1-engine-version
branch
from
6ea43b5 to
04703c2
Compare
d-gubert
force-pushed
the
feat/apps-engine-split--pr1-engine-version
branch
from
04703c2 to
7664485
Compare
coderabbitai
Bot
added
the
type: feature
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/apps-engine/src/definition/version.ts (2)
1-11: Trim the leading doc block per coding guidelines.The repo guideline for
**/*.{ts,tsx,js}is to avoid code comments in the implementation. The rationale here is genuinely non-obvious (compile-time vs runtime path resolution), so consider keeping at most a one-line note next to therequire()call and dropping the rest, or moving the explanation to the PR/commit description.As per coding guidelines: "Avoid code comments in the implementation".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/apps-engine/src/definition/version.ts` around lines 1 - 11, Remove the long leading doc block and replace it with a single-line comment immediately adjacent to the require() that reads package.json (e.g. "// Use require() so TS doesn't resolve the path at compile time"), keeping the actual code (the require() call and exported version constant) unchanged; move the detailed explanation into the PR/commit message instead of the source.
16-16: Consider freezing the dependency on a moving package shape.
require(requirePath).versionwill silently produceundefined(typed asstring) if the field is ever missing or the file is renamed. Since the whole point of this module is to fail loudly when version detection breaks, a small assertion would be cheap and would surface misconfiguration immediately at module load:🛡️ Suggested guard
-// eslint-disable-next-line import/no-dynamic-require, `@typescript-eslint/no-require-imports` -- Paths are bounded, we just need to decide which package.json file to target -export const ENGINE_VERSION: string = require(requirePath).version; +// eslint-disable-next-line import/no-dynamic-require, `@typescript-eslint/no-require-imports` -- Paths are bounded, we just need to decide which package.json file to target +const { version } = require(requirePath) as { version?: string }; +if (typeof version !== 'string') { + throw new Error(`Apps-Engine: failed to resolve version from ${requirePath}`); +} +export const ENGINE_VERSION: string = version;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/apps-engine/src/definition/version.ts` at line 16, The current export ENGINE_VERSION uses require(requirePath).version which can silently be undefined; update the module so after loading the package JSON via require(requirePath) (using the existing requirePath symbol) you assert that the loaded object has a non-empty version property and throw a clear error if not present, then assign that value to ENGINE_VERSION; ensure the runtime check happens at module load so any missing/renamed field fails loudly with a helpful message mentioning requirePath and "version".packages/apps-engine/src/server/compiler/AppPackageParser.ts (1)
10-10: Optional: drop the redundant instance field.
appsEngineVersionis now just a copy of the module-levelENGINE_VERSIONconstant — the per-instance field, the explicitstringannotation, and the indirection at the call sites no longer add value. Using the constant directly makes the dependency more obvious and removes one layer of state on the parser.♻️ Proposed simplification
- private appsEngineVersion: string = ENGINE_VERSION; - @@ - if (!semver.satisfies(this.appsEngineVersion, info.requiredApiVersion)) { - throw new RequiredApiVersionError(info, this.appsEngineVersion); + if (!semver.satisfies(ENGINE_VERSION, info.requiredApiVersion)) { + throw new RequiredApiVersionError(info, ENGINE_VERSION); }Skip if you want to preserve the field as a stable seam for future test injection.
Also applies to: 18-18
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/apps-engine/src/server/compiler/AppPackageParser.ts` at line 10, The instance field appsEngineVersion on AppPackageParser is redundant because it just mirrors the module-level ENGINE_VERSION; remove the appsEngineVersion declaration and any constructor assignment, then change all call sites inside AppPackageParser that reference this.appsEngineVersion to use ENGINE_VERSION directly (retain import of ENGINE_VERSION). Ensure you also remove the explicit string type annotation and any related tests or references that assume the per-instance field.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/apps-engine/src/definition/version.ts`:
- Around line 12-13: The OS-dependent dirname check using
__dirname.endsWith('src/definition') can fail on Windows; update the
runningFromSource logic to use path utilities instead (import path) — e.g.,
determine runningFromSource by checking path.basename(path.dirname(__dirname))
=== 'src' && path.basename(__dirname) === 'definition' or by normalizing
separators with path.join/normalize, then set requirePath accordingly; modify
the runningFromSource and requirePath calculation (symbols: runningFromSource,
__dirname, requirePath) to use path methods so the correct package.json path is
chosen cross-platform.
---
Nitpick comments:
In `@packages/apps-engine/src/definition/version.ts`:
- Around line 1-11: Remove the long leading doc block and replace it with a
single-line comment immediately adjacent to the require() that reads
package.json (e.g. "// Use require() so TS doesn't resolve the path at compile
time"), keeping the actual code (the require() call and exported version
constant) unchanged; move the detailed explanation into the PR/commit message
instead of the source.
- Line 16: The current export ENGINE_VERSION uses require(requirePath).version
which can silently be undefined; update the module so after loading the package
JSON via require(requirePath) (using the existing requirePath symbol) you assert
that the loaded object has a non-empty version property and throw a clear error
if not present, then assign that value to ENGINE_VERSION; ensure the runtime
check happens at module load so any missing/renamed field fails loudly with a
helpful message mentioning requirePath and "version".
In `@packages/apps-engine/src/server/compiler/AppPackageParser.ts`:
- Line 10: The instance field appsEngineVersion on AppPackageParser is redundant
because it just mirrors the module-level ENGINE_VERSION; remove the
appsEngineVersion declaration and any constructor assignment, then change all
call sites inside AppPackageParser that reference this.appsEngineVersion to use
ENGINE_VERSION directly (retain import of ENGINE_VERSION). Ensure you also
remove the explicit string type annotation and any related tests or references
that assume the per-instance field.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5d087932-7c0a-49de-8a34-c7617688fe4f
📒 Files selected for processing (3)
packages/apps-engine/src/definition/version.tspackages/apps-engine/src/server/compiler/AppPackageParser.tspackages/apps-engine/tsconfig.json
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
packages/apps-engine/src/definition/version.tspackages/apps-engine/src/server/compiler/AppPackageParser.ts
🧠 Learnings (4)
📓 Common learnings
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: In Rocket.Chat PRs, keep feature PRs free of unrelated lockfile-only dependency bumps; prefer reverting lockfile drift or isolating such bumps into a separate "chore" commit/PR, and always use yarn install --immutable with the Yarn version pinned in package.json via Corepack.
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38068
File: packages/apps-engine/deno-runtime/handlers/lib/assertions.ts:8-12
Timestamp: 2026-01-08T15:07:57.458Z
Learning: In packages/apps-engine/deno-runtime, App objects are never serialized and are instantiated and kept in the runtime context, so duck-typing checks on App methods (including protected methods like extendConfiguration) are reliable for runtime assertions.
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 39701
File: packages/apps-engine/src/server/runtime/deno/AppsEngineDenoRuntime.ts:181-188
Timestamp: 2026-03-18T16:45:52.113Z
Learning: In `packages/apps-engine/src/server/runtime/deno/AppsEngineDenoRuntime.ts`, granting `--allow-read` to the full `this.tempFilePath` (entire temp directory) in the Deno subprocess spawn options is intentional. The `deno.jsonc` config file can affect path resolution in ways that require read access to the broader temp directory, not just the `deno-runtime` subdirectory symlink. Do not flag this as an overly broad permission grant.
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
packages/apps-engine/src/definition/version.tspackages/apps-engine/src/server/compiler/AppPackageParser.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
packages/apps-engine/src/definition/version.tspackages/apps-engine/src/server/compiler/AppPackageParser.ts
📚 Learning: 2026-01-08T15:07:57.458Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38068
File: packages/apps-engine/deno-runtime/handlers/lib/assertions.ts:8-12
Timestamp: 2026-01-08T15:07:57.458Z
Learning: In packages/apps-engine/deno-runtime, App objects are never serialized and are instantiated and kept in the runtime context, so duck-typing checks on App methods (including protected methods like extendConfiguration) are reliable for runtime assertions.
Applied to files:
packages/apps-engine/src/server/compiler/AppPackageParser.ts
🔇 Additional comments (1)
packages/apps-engine/tsconfig.json (1)
13-13: LGTM.Enabling
resolveJsonModuleis consistent with the new JSON consumption insrc/definition/version.ts. Note the actual version load there still uses dynamicrequire()(intentionally, to avoid TS resolving the path at compile time and emitting an incorrect relative path from the compiled location), so this option is mainly defensive for any future static JSON imports.
| const runningFromSource = __dirname.endsWith('src/definition'); | ||
| const requirePath = runningFromSource ? '../../package.json' : '../package.json'; |
There was a problem hiding this comment.
Path heuristic is OS-dependent — breaks on Windows.
__dirname on Windows uses backslashes (e.g. C:\…\src\definition), so endsWith('src/definition') will always be false there and the wrong relative path will be selected when running tests via ts-node on Windows. Even if Windows is not a primary target, it's a one-line fix:
🔧 Suggested fix
-const runningFromSource = __dirname.endsWith('src/definition');
-const requirePath = runningFromSource ? '../../package.json' : '../package.json';
+const runningFromSource = __dirname.split(/[\\/]/).slice(-2).join('/') === 'src/definition';
+const requirePath = runningFromSource ? '../../package.json' : '../package.json';Or use path.basename(path.dirname(__dirname)) === 'src' && path.basename(__dirname) === 'definition'.
📝 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.
| const runningFromSource = __dirname.endsWith('src/definition'); | |
| const requirePath = runningFromSource ? '../../package.json' : '../package.json'; | |
| const runningFromSource = __dirname.split(/[\\/]/).slice(-2).join('/') === 'src/definition'; | |
| const requirePath = runningFromSource ? '../../package.json' : '../package.json'; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/apps-engine/src/definition/version.ts` around lines 12 - 13, The
OS-dependent dirname check using __dirname.endsWith('src/definition') can fail
on Windows; update the runningFromSource logic to use path utilities instead
(import path) — e.g., determine runningFromSource by checking
path.basename(path.dirname(__dirname)) === 'src' && path.basename(__dirname) ===
'definition' or by normalizing separators with path.join/normalize, then set
requirePath accordingly; modify the runningFromSource and requirePath
calculation (symbols: runningFromSource, __dirname, requirePath) to use path
methods so the correct package.json path is chosen cross-platform.
1 participant
Proposed changes (including videos or screenshots)
Adds
src/definition/version.tswhich reads the package version viaresolveJsonModuleand exports it asENGINE_VERSION.Replaces
AppPackageParser.getEngineVersion()— which resolved the version by traversing the filesystem relative to__dirname— with a direct import ofENGINE_VERSION. This removes the assumption thatpackage.jsonlives at a predictable relative path, which will break whenAppPackageParsermoves to a different package during theapps-engine split.
Issue(s)
Steps to test or reproduce
Further comments
Related to the "Apps-Engine split" stack:
Summary by CodeRabbit