Conversation
…t into migration/3.0
There was a problem hiding this comment.
Pull request overview
Upgrades the app to Meteor 3.4 and updates supporting infrastructure (startup wiring, settings validation, logging/health, patches) while expanding/modernizing the test suite and test tooling.
Changes:
- Bump Meteor release/packages and update schema/JWT/health integrations.
- Refactor collection/files validation & logging behavior (esp. under tests).
- Add new utilities/patch + expand tests and test runner scripts/CI.
Reviewed changes
Copilot reviewed 41 out of 43 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/main.js | Initializes test app env and adds new util tests. |
| tests/helpers/mockCollection.js | Adds schema attach helper for mocked collections. |
| test.sh | Adds browser driver, debugger, coverage options. |
| settings.json | Extends hosts and patch settings. |
| server/main.js | Adds health startup import. |
| run.sh | Adds optional debugger flag. |
| package.json | Updates deps/devDeps for Meteor 3.4. |
| package-lock.json | Locks updated dependency graph. |
| imports/utils/toArray.js | Adds toArray helper utility. |
| imports/utils/tests/isArray.tests.js | Adds tests for toArray behavior. |
| imports/utils/noop.js | Adds noop helper. |
| imports/utils/log.js | Disables logging during tests. |
| imports/startup/server/webapp.js | Adjusts WebApp JSON middleware wiring. |
| imports/startup/server/settingsSchema.js | Passes meteor SimpleSchema into validator. |
| imports/startup/server/rateLimit.js | Updates rate limiter hook invocation. |
| imports/startup/server/patches.js | Refactors patch execution flow. |
| imports/startup/server/health.js | Adds health inquiry method. |
| imports/startup/server/accounts.js | Changes OAuth debug behavior and logs. |
| imports/contexts/tests/UnitSet.tests.js | Adds extensive UnitSet behavior tests. |
| imports/contexts/UnitSet.js | Adds doc/comment and route behavior context. |
| imports/contexts/MediaLib.js | Removes eslint disable comment. |
| imports/contexts/Assets.js | Removes eslint disable comment. |
| imports/api/schema/Schema.js | Switches to Meteor SimpleSchema package import. |
| imports/api/progress/calculateProgress.js | Makes unitSetProgressChanged async with fetchAsync loop. |
| imports/api/patches/removeImageOrigin.js | Adds new migration patch for image URL stripping. |
| imports/api/origins/tests/getAllowedOrigins.tests.js | Updates origin tests to validate dynamically. |
| imports/api/mixins/checkPermissions.js | Switches JWT validation to leaonline:jwt. |
| imports/api/grid/tests/checkMime.tests.js | Adds tests for mime checking behavior. |
| imports/api/grid/checkuser.js | Makes user check async and adds debug hooks. |
| imports/api/grid/checkMime.js | Refactors signature, adds safer comparisons. |
| imports/api/factories/createRoute.js | Renames debug middleware option. |
| imports/api/factories/createPublication.js | Adds health mixin to publications. |
| imports/api/factories/createMethods.js | Adds health mixin to methods. |
| imports/api/factories/createFilesCollection.js | Removes always-on debug flag. |
| imports/api/context/initCollection.js | Refactors debug/i18n/mime/user validation wiring. |
| .settingsschema.js | Refactors to injectable SimpleSchema + expands schema. |
| .meteor/versions | Updates Meteor package versions list. |
| .meteor/release | Bumps Meteor release to 3.4. |
| .meteor/packages | Updates Meteor package pins and adds jwt/health. |
| .gitignore | Ignores .deploy* instead of .deploy. |
| .github/workflows/test_suite.yml | Reads Meteor version dynamically; removes coverage job. |
| .coverage.json | Adds coverage exclusion config. |
| .babelrc | Adds coverage-only istanbul config and exclusions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| UnitSetCollection = UnitSet.collection() | ||
| UnitCollection = Unit.collection() | ||
| TestCylceCollection = TestCycle.collection() | ||
| console.debug(UnitSet.methods.update) |
There was a problem hiding this comment.
console.debug(UnitSet.methods.update) adds noisy output to the test run and can make CI logs harder to read. Please remove this debug statement (or gate it behind an explicit verbose flag).
| console.debug(UnitSet.methods.update) |
| after(() => { | ||
| restoreCollection(UnitSet) | ||
| }) |
There was a problem hiding this comment.
This suite mocks collections for Unit and TestCycle as well, but only restores UnitSet in after(). Not restoring all mocked collections can leak state into other tests. Please restore Unit and TestCycle here too (and consider fixing the TestCylceCollection typo to avoid confusion).
| const checkForImages = (name, array) => { | ||
| const updates = [] | ||
| for (let i = 0; i < array.length; i++) { | ||
| const entry = array[i] |
There was a problem hiding this comment.
toArray(null) returns [null] (by design, per its docs/tests). If any of these fields can be null, entry.subtype will throw because entry is null. Add a guard like if (!entry) continue (and similarly for non-object entries) before accessing entry.subtype.
| const entry = array[i] | |
| const entry = array[i] | |
| if (!entry || typeof entry !== 'object') { | |
| continue | |
| } |
| export const unitSetProgressChanged = async ({ unitSetId, debug = () => {} }) => { | ||
| debug('(unitSetProgressChanged):', unitSetId) | ||
| TestCycle | ||
| const docs = await TestCycle | ||
| .collection() | ||
| .find({ unitSets: unitSetId }) | ||
| .forEach(testCycleDoc => updateTestCycleDoc({ testCycleDoc, debug })) | ||
| .fetchAsync() | ||
| for (const testCycleDoc of docs) { | ||
| await updateTestCycleDoc({ testCycleDoc, debug }) | ||
| } |
There was a problem hiding this comment.
unitSetProgressChanged is now async, but existing call sites/tests may still call it synchronously. Without awaiting/returning the promise, callers can observe stale TestCycle progress (and tests may become flaky). Please update callers to await (or keep a synchronous API by returning TestCycle.collection().find(...).forEachAsync(...)).
| const token = args[0]?.token | ||
| console.debug('check token for', name, 'token:', token) | ||
| const { valid, reason } = tokenIsValid(token, name) |
There was a problem hiding this comment.
This logs the raw JWT (console.debug('... token:', token)), which can leak credentials into logs (especially in production / CI). Please remove token logging or gate it behind a strict dev-only flag and ensure the validator debug output cannot print sensitive token contents.
| try { | ||
| urlSchema.validate({ urls }) | ||
| } | ||
| catch (e) { | ||
| console.error(e) | ||
| } |
There was a problem hiding this comment.
The schema validation here isn't actually checking anything because urlSchema.validate({ urls }) doesn't use validationContext; then you read validationContext.validationErrors() without ever calling validationContext.validate(...). Use the context to validate (or drop the context entirely and assert that urlSchema.validate does not throw).
| try { | |
| urlSchema.validate({ urls }) | |
| } | |
| catch (e) { | |
| console.error(e) | |
| } | |
| const isValid = validationContext.validate({ urls }) | |
| expect(isValid).to.equal(true) |
| release_number="${release_raw#METEOR@}" | ||
| echo "release_raw=$release_raw" | ||
| echo "release_number=$release_number" | ||
| echo "::set-output name=release_number::$release_number" |
There was a problem hiding this comment.
GitHub Actions ::set-output is deprecated and can break on newer runners. Write the output to $GITHUB_OUTPUT instead so ${{ steps.meteor_release.outputs.release_number }} remains available.
| echo "::set-output name=release_number::$release_number" | |
| echo "release_number=$release_number" >> "$GITHUB_OUTPUT" |
| WebApp.express.urlencoded({ extended: true }) | ||
| WebApp.express.json() | ||
| WebApp.handlers.use(WebApp.express.json()) |
There was a problem hiding this comment.
WebApp.express.urlencoded({ extended: true }) creates a middleware but it isn't mounted anywhere, so urlencoded request bodies won't actually be parsed. Use WebApp.handlers.use(WebApp.express.urlencoded({ extended: true })) (similar to the json middleware) or remove the line if not needed.
| const exists = await userExists(userId) | ||
| debug('userExists?', { userId, exists }) | ||
| if (!exists) { | ||
| debug('users', await Meteor.users.find().fetchAsync()) |
There was a problem hiding this comment.
When a user is missing, this debug path fetches and logs all users (Meteor.users.find().fetchAsync()), which is both a potential PII leak and can be very expensive on large user collections. Consider removing this branch or limiting it to tests only (and log only minimal metadata like counts / ids).
| debug('users', await Meteor.users.find().fetchAsync()) | |
| debug('userMissing', { userId }) |
| accessTokenUrl: oauth.accessTokenUrl, | ||
| identityUrl: oauth.identityUrl, | ||
| redirectUrl: oauth.redirectUrl, | ||
| debug: oauth.debug | ||
| debug: true | ||
| } |
There was a problem hiding this comment.
debug: true forces verbose OAuth logging in all environments and ignores Meteor.settings.oauth.debug. This can leak sensitive auth details; please wire this back to settings and/or gate it behind Meteor.isDevelopment.
Upgrade to Meteor 3.4