fix(datasource): don't load test files as entities - #3375
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThe server build now uses a dedicated configuration and explicit database registrations. Test database mutations require test mode unless seeding explicitly opts out. Network settings refresh status without checking update availability. ChangesDatabase build and test safety
Network settings status refresh
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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 |
There was a problem hiding this comment.
Pull request overview
This PR hardens and stabilizes the server’s test database setup and TypeORM entity/subscriber loading, while keeping server builds free of test-only files.
Changes:
- Added an explicit
NODE_ENV === 'test'guard to prevent accidental destructive seeding/resets, with an opt-in escape hatch for tooling. - Replaced TypeORM glob-based entity/subscriber discovery with explicit imports to ensure consistent entity loading.
- Updated the server build config to exclude
*.test.tsand adjustedbuild:serverto use the new build tsconfig.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
server/utils/seedTestDb.ts |
Adds a safety guard for destructive test DB operations and an opt-in override. |
server/utils/seedTestDb.test.ts |
Adds unit tests for the test DB guard behavior. |
server/tsconfig.build.json |
New build tsconfig excluding *.test.ts from server compilation output. |
server/scripts/prepareTestDb.ts |
Opts into seeding outside NODE_ENV=test for Cypress preparation. |
server/datasource.ts |
Switches entity/subscriber loading from glob strings to explicit imports/arrays. |
package.json |
Updates build:server to use server/tsconfig.build.json (and matching tsc-alias). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
seerr
|
||||||||||||||||||||||||||||
| Project |
seerr
|
| Branch Review |
fix/test-file-entity-loading
|
| Run status |
|
| Run duration | 01m 58s |
| Commit |
|
| Committer | fallenbagel |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
|
|
0
|
|
|
0
|
|
|
0
|
|
|
0
|
|
|
33
|
| View all changes introduced in this branch ↗︎ | |
seedTestDb and resetTestDb operate on the shared dataSource, which points at whatever NODE_ENV selects, and neither checked where it was pointing before dropping every table and seeding an admin account with a hardcoded password hash. Both now refuse unless NODE_ENV is test or ALLOW_DB_RESET is set, which cypress:prepare passes because it seeds the on-disk dev database on purpose.
The build compiled every .ts under server, so twelve test files shipped in dist. Anything under dist/entity or dist/subscriber gets loaded by the production entity and subscriber globs, which means a test file in one of those directories would run at boot. The build now uses its own tsconfig that excludes *.test.ts, while typecheck:server keeps using the base config so the test files are still type checked.
The entity and subscriber options were globs over a whole directory, so every file in server/entity and server/subscriber was required at initialize, test files included. A test file there would run its module body inside any process that opens the datasource, including a production boot. Both options now take the classes directly, which also removes the duplicate ts and js patterns across the five config variants. Migrations stay globbed since they are ordered by filename and there are hundreds of them.
StatusChecker polls /api/v1/status?checkUpdateAvailable=false, but saving network settings revalidated /api/v1/status, which is a different SWR key and nothing subscribes to it. The keys stopped matching in #3137 when the version check toggle was added, so since then the restart modal has only appeared on the next sixty second poll or on a fresh page load. The general settings cypress spec covered this and kept passing because earlier specs left the restart flag dirty, so the modal was already up when the spec loaded the page. Cleaning that up in #3368 removed the stale flag the spec was leaning on and the real gap surfaced.
a34e853 to
8dbc244
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/components/Settings/SettingsNetwork/index.tsx:179
checkUpdateAvailable=falsemay not actually disable update checks because Express query values are strings; on the server,/statustreats any defined value ascheckUpdateand then usesif (checkUpdate), so the string'false'is truthy. This means the request keyed by this URL can still trigger GitHub update checking (and related rate limiting). Consider parsingreq.query.checkUpdateAvailableas a real boolean server-side (e.g.,=== 'true'), or change the client/server contract to use a falsy value.
// the key StatusChecker polls on, so the restart modal shows at once
mutate('/api/v1/status?checkUpdateAvailable=false');
server/utils/seedTestDb.ts:27
- The new guard error message says it "drops every table", but
seedTestDbcan be called withpreserveDb: trueand then it won’t drop the DB. Also, including the currentNODE_ENVvalue would make the failure easier to diagnose in CI/automation.
throw new Error(
`Refusing to ${operation} while NODE_ENV is not test: this drops every table and seeds accounts with a known password.`
);
Description
TypeORM entities and subscribers load from directory globs over every
.tsfile underserver/entityandserver/subscriber. While it is harmless today since neither folder has a test file, but drop one in and requiring it runs the module body, which callssetupTestDb()again. That registers a second rootbeforeandbeforeEachafter the runner already built its tree, so two schema rebuilds collide on the single in-memory sqlite connection: "cannot start a transaction within a transaction," every test cancelled, nothing in the failure pointing at the actual file or the actual error.Production is worse.
build:servercompiles every.tsunderserver, test files included, and production loadsdist/entity/**/*.js, so a compiled test file there runs its module body at boot.seedTestDbcallsdropDatabase()on the shareddataSourcewith no check on which database it's pointing at, then rebuilds the schema and seedsadmin@seerr.devwithPermission.ADMINand a hardcoded bcrypt hash. I confirmed this against a throwaway sqlite file underNODE_ENV=productionand it dropped a table I'd put there first, rebuilt 15 tables, and left those two accounts behind.Important
No released version is affected. No tag ships a test file under
dist/entityordist/subscriber, so there's nothing to fix or disclose.Now,
seedTestDbandresetTestDbrefuse to run unlessNODE_ENVis test.cypress:prepare, now opts in viaseedTestDb({ allowOutsideTest: true }). This deny-unless-permitted was added because a plain node process with noNODE_ENVset resolves todevConfigagainstconfig/db/db.sqlite3, someone's real database if they're running from source.In addition,
build:serverpoints at a newserver/tsconfig.build.jsonthat excludes*.test.ts, sodiststops shipping the compiled test files it currently produces. Config was separated because becausetypecheck:serveris the only thing type checking those files, and the roottsconfigcoverssrconly and excluding at the base would've silently dropped the files from CI.Lastly, typeORM gets the
entityandsubscriberclasses directly instead of globs, which also drops the duplicatedts/jspatterns across the five config variants. Narrowing the globs to skip*.test.tswould achieve the same thing provided the files named are named that way in directories and a differently named test file, a helper, or a newly globbed directory brings the whole thing back. Classes mean nothing is matched by pattern, at the cost of a list to keep current, forget to add anentityand it throwsEntityMetadataNotFoundErrorthe first time its repository is used anyways. Much more safer this way. Migrations stay globbed.How Has This Been Tested?
Screenshots / Logs (if applicable)
Checklist:
pnpm buildpnpm i18n:extractSummary by CodeRabbit
Bug Fixes
Build Improvements
Tests