refactor: extract server version helper#545
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (2)
🔇 Additional comments (1)
📝 WalkthroughSummary by CodeRabbit
Walkthrough
ChangesSERVER_VERSION module extraction
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: New test missing test:mock
- Added test/server/version.test.ts to the test:mock script in package.json so it runs in CI and npm run test:all.
You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit 86a3679. Configure here.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/version.ts`:
- Around line 8-13: The packageJsonPaths array in the version.ts file tries to
resolve the built file path before the source file path. When running from
source, this can incorrectly resolve to a parent workspace's package.json.
Reorder the paths in the packageJsonPaths array so that the source-local path
(the one using ../package.json relative to __dirname) is attempted first,
followed by the built file path (../../package.json), ensuring that source runs
pick up the correct package.json before falling back to the built location.
In `@test/server/version.test.ts`:
- Line 6: The packageVersion constant is reading package.json using a relative
path which depends on the process current working directory, making the test
flaky when run from different directories. Instead, construct an absolute path
to package.json using __dirname (the directory of the current test file) and the
path module to navigate to the project root where package.json is located.
Replace the relative "package.json" string in the fs.readFileSync call with this
absolute path so the test can find the file regardless of which directory the
test is invoked from.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 8785fb46-9cee-45c5-a545-9d84ff7f9707
📒 Files selected for processing (3)
index.tsserver/version.tstest/server/version.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: Cursor Bugbot Autofix
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: coverage
- GitHub Check: test
🔇 Additional comments (1)
index.ts (1)
191-193: LGTM!Also applies to: 202-202
| const packageJsonPaths = [ | ||
| // Built file: build/server/version.js -> ../../package.json | ||
| path.resolve(__dirname, "../../package.json"), | ||
| // Source file under tsx/node:test: server/version.ts -> ../package.json | ||
| path.resolve(__dirname, "../package.json"), | ||
| ]; |
There was a problem hiding this comment.
Potentially resolves the wrong package.json in source runs.
At Line 10, ../../package.json is tried before the source-local candidate. When running from server/version.ts, this can pick a parent workspace package.json and report an incorrect SERVER_VERSION.
Suggested fix
const packageJsonPaths = [
- // Built file: build/server/version.js -> ../../package.json
- path.resolve(__dirname, "../../package.json"),
// Source file under tsx/node:test: server/version.ts -> ../package.json
path.resolve(__dirname, "../package.json"),
+ // Built file: build/server/version.js -> ../../package.json
+ path.resolve(__dirname, "../../package.json"),
];📝 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 packageJsonPaths = [ | |
| // Built file: build/server/version.js -> ../../package.json | |
| path.resolve(__dirname, "../../package.json"), | |
| // Source file under tsx/node:test: server/version.ts -> ../package.json | |
| path.resolve(__dirname, "../package.json"), | |
| ]; | |
| const packageJsonPaths = [ | |
| // Source file under tsx/node:test: server/version.ts -> ../package.json | |
| path.resolve(__dirname, "../package.json"), | |
| // Built file: build/server/version.js -> ../../package.json | |
| path.resolve(__dirname, "../../package.json"), | |
| ]; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/version.ts` around lines 8 - 13, The packageJsonPaths array in the
version.ts file tries to resolve the built file path before the source file
path. When running from source, this can incorrectly resolve to a parent
workspace's package.json. Reorder the paths in the packageJsonPaths array so
that the source-local path (the one using ../package.json relative to __dirname)
is attempted first, followed by the built file path (../../package.json),
ensuring that source runs pick up the correct package.json before falling back
to the built location.
| import fs from "node:fs"; | ||
| import { SERVER_VERSION } from "../../server/version.js"; | ||
|
|
||
| const packageVersion = JSON.parse(fs.readFileSync("package.json", "utf8")).version; |
There was a problem hiding this comment.
Make the fixture path independent of process CWD.
At Line 6, reading "package.json" relative to process.cwd() makes this test flaky when invoked from another directory.
Suggested fix
-const packageVersion = JSON.parse(fs.readFileSync("package.json", "utf8")).version;
+const packageJsonUrl = new URL("../../package.json", import.meta.url);
+const packageVersion = JSON.parse(fs.readFileSync(packageJsonUrl, "utf8")).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.
| const packageVersion = JSON.parse(fs.readFileSync("package.json", "utf8")).version; | |
| const packageJsonUrl = new URL("../../package.json", import.meta.url); | |
| const packageVersion = JSON.parse(fs.readFileSync(packageJsonUrl, "utf8")).version; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/server/version.test.ts` at line 6, The packageVersion constant is
reading package.json using a relative path which depends on the process current
working directory, making the test flaky when run from different directories.
Instead, construct an absolute path to package.json using __dirname (the
directory of the current test file) and the path module to navigate to the
project root where package.json is located. Replace the relative "package.json"
string in the fs.readFileSync call with this absolute path so the test can find
the file regardless of which directory the test is invoked from.
2 participants
Summary
Part of #516.
This is a small behavior-preserving extraction:
server/version.tsSERVER_VERSIONmatchespackage.jsonVerification
rtk npm run buildnode --import tsx/esm --test test/server/version.test.tsnode -e "import('./build/server/version.js').then(m => { if (!m.SERVER_VERSION || m.SERVER_VERSION === 'unknown') process.exit(1); console.log(m.SERVER_VERSION); })"rtk npm run test:remote-authNote
Low Risk
Small refactor with no auth or API logic changes; version is still read from
package.jsonfor health and MCP metadata.Overview
Moves package version loading out of
index.tsinto a dedicatedserver/version.tsmodule that exportsSERVER_VERSION, so the main entry no longer readspackage.jsoninline.The helper resolves
package.jsonfrom either the compiled layout (build/server/version.js→ repo root) or when loaded from source/tests (server/version.ts→ parent), falling back to"unknown"if read fails.index.tsnow importsSERVER_VERSIONfor the MCP serverversionfield and the/healthJSONversionfield—behavior is intended to stay the same.Adds
test/server/version.test.ts(assertsSERVER_VERSIONmatches rootpackage.json) and includes it in thetest:mockscript.Reviewed by Cursor Bugbot for commit 1a1f1d0. Bugbot is set up for automated code reviews on this repo. Configure here.