feat: validate password policy length on settings save - #41173
Conversation
|
Looks like this PR is ready to merge! 🎉 |
🦋 Changeset detectedLatest commit: 1f9850b The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
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:
WalkthroughThis PR adds declarative setting validation, wires it into single and bulk save paths plus settings API handlers, and applies cross-field password policy rules for min/max length with new types, messages, tests, and a changeset. ChangesCentralized settings validation
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant SaveMethod as saveSetting/saveSettings/settings API
participant Validator as validateSettingRules
participant SettingsStore as Settings storage
Client->>SaveMethod: submit setting change(s)
SaveMethod->>Validator: validate incoming values
alt validation fails
Validator-->>SaveMethod: throw Error(rule.errorKey)
SaveMethod-->>Client: Meteor.Error/API.v1.failure error-setting-validation-failed
else validation passes
SaveMethod->>SettingsStore: persist setting value(s)
end
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/app/lib/server/functions/saveSettingsBulk.ts (1)
68-95: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftCollapse duplicate setting IDs before validation and update.
validateSettings()resolves cross-field rules against the first matching_id, but this path still updates every entry inparams. With duplicate IDs, one value can be validated against another and the final stored value becomes order-dependent.🤖 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 `@apps/meteor/app/lib/server/functions/saveSettingsBulk.ts` around lines 68 - 95, Collapse duplicate setting IDs before running validateSettings() and before building the update promises in saveSettingsBulk so each _id is processed once with a deterministic value. Ensure the deduped params are what updateAuditedByUser, Settings.updateValueById, Settings.findOneById, and notifyOnSettingChangedById operate on, so repeated entries cannot make validation or the final stored setting order-dependent.
🧹 Nitpick comments (6)
packages/core-typings/src/ISetting.ts (1)
34-39: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider reusing
EnableQueryfor thequeryfield type.
enableQuery/displayQueryon the same interface use the namedEnableQuerytype, whileSettingValidationRule.queryis typed as a looseRecord<string, unknown>. Aligning it withEnableQuery(or a shared query type) would improve type safety and consistency across similar query-filter fields.🤖 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 `@packages/core-typings/src/ISetting.ts` around lines 34 - 39, The SettingValidationRule.query field is using a loose Record<string, unknown> while similar query-filter fields on the same area use the named EnableQuery type. Update the SettingValidationRule definition in ISetting to reuse EnableQuery (or a shared query type) for query so the interface stays consistent and more type-safe.apps/meteor/app/lib/server/lib/settingValidationRules.ts (2)
10-14: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueComments in implementation code violate the coding guideline.
Several explanatory comments are added to this implementation file (JSDoc block, inline comments on
matches,resolveObject,conditions,unknownReferences,conditionHolds).As per coding guidelines,
**/*.{ts,tsx,js}: "Avoid code comments in the implementation."Also applies to: 22-26, 44-44, 51-52, 59-59
🤖 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 `@apps/meteor/app/lib/server/lib/settingValidationRules.ts` around lines 10 - 14, The implementation file contains multiple explanatory comments that violate the no-comments guideline. Remove the JSDoc block and the inline comments around the relevant logic in settingValidationRules.ts, including the sections tied to matches, resolveObject, conditions, unknownReferences, and conditionHolds, while keeping the existing behavior unchanged.Source: Coding guidelines
44-49: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
appliesWhencondition values are not resolved for$settingreferences, unlikerule.query.
resolveObject/resolveNode(lines 27-42) resolve{ $setting: id }nodes anywhere insiderule.query, and any such reference gets tracked inreferencesfor the "unknown setting" safety check. However,conditionHolds(line 60-61) usescondition.valueverbatim — if a future rule author puts a$settingreference insideappliesWhen[].value, it will be treated as a literal object instead of being resolved, and won't be captured by the unknown-reference safety net either. No currently-declared rule (per the mirrored fixtures invalidateSettings.spec.ts) exercises this path, so it's not an active bug, but it's an inconsistency in the resolution contract that could silently misbehave for future rules.Also applies to: 60-61
🤖 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 `@apps/meteor/app/lib/server/lib/settingValidationRules.ts` around lines 44 - 49, The appliesWhen condition handling in settingValidationRules is inconsistent with rule.query resolution because condition.value is used raw, so $setting references inside appliesWhen can be left unresolved and skipped by the unknown-reference check. Update the condition processing in validateSettings/conditionHolds to run appliesWhen values through the same resolveObject/resolveNode path used for rule.query, and make sure any resolved references are added to references so the safety net covers them too.apps/meteor/app/lib/server/lib/validateSettings.ts (1)
10-14: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueComments in implementation code violate the coding guideline.
This file also has explanatory comments (module-level doc comment, and inline comments on
getValueOf, "validate the value against its type", "evaluate the setting's declared cross-field / domain rules").As per coding guidelines,
**/*.{ts,tsx,js}: "Avoid code comments in the implementation."Also applies to: 53-53, 64-64, 67-67
🤖 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 `@apps/meteor/app/lib/server/lib/validateSettings.ts` around lines 10 - 14, Remove the implementation comments from validateSettings.ts, including the module-level explanatory note near SETTING_VALIDATION_ERROR and the inline comments around getValueOf and the validation flow. Keep the logic in getValueOf, validateSettings, and any related helpers unchanged, but rely on clear naming and structure instead of code comments to explain the type/bounds checks and declared-rule validation.Source: Coding guidelines
apps/meteor/tests/unit/app/lib/server/lib/settingValidationRules.spec.ts (1)
70-76: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMissing test: unknown reference via
appliesWhen._id(not just via$settinginquery).Only the
$setting-in-querypath is tested for the "unknown reference" safety net. Consider adding a case whereappliesWhen._iditself points to a non-existent setting, to lock in that this path is also treated as passing/logged.🤖 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 `@apps/meteor/tests/unit/app/lib/server/lib/settingValidationRules.spec.ts` around lines 70 - 76, The unknown-reference coverage in evaluateSettingValidationRule is incomplete because it only verifies the $setting-in-query path. Add a unit test in settingValidationRules.spec.ts that exercises a rule with appliesWhen._id pointing to a missing setting, and assert it still passes while systemLoggerErrorMock is called and the logged references include that missing setting; use evaluateSettingValidationRule, getterFor, and systemLoggerErrorMock to match the existing test style.apps/meteor/tests/unit/app/lib/server/lib/validateSettings.spec.ts (1)
74-89: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNo coverage for
boolean,roomPick,multiSelect,range,timespan, orcode/JSON validators.Only the
inttype validator and bounds delegation are exercised. GivenvalidateJsonhas the error-contract issue flagged invalidateSettings.ts, a direct test asserting the thrown error's shape would have caught it.🤖 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 `@apps/meteor/tests/unit/app/lib/server/lib/validateSettings.spec.ts` around lines 74 - 89, Add unit coverage in validateSettings.spec.ts for the other validator paths in validateSettings, especially boolean, roomPick, multiSelect, range, timespan, and code/JSON, so the suite exercises each type-specific validator rather than only the int path. Also add a direct assertion around validateJson’s thrown error shape in validateSettings to lock down the error contract, using the existing validateSettings and validateJson symbols to locate the affected logic.
🤖 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 `@apps/meteor/app/lib/server/lib/validateSettings.ts`:
- Around line 28-38: The JSON validation in validateJson currently throws a
generic Meteor.Error message that does not follow the settings error contract
used by the other validators. Update the catch path in validateJson to throw the
standard error-invalid-setting-value error with a clear reason and include {
method: 'saveSettings' }, keeping the behavior aligned with the rest of
validateSettings and isSettingCode-based validation.
---
Outside diff comments:
In `@apps/meteor/app/lib/server/functions/saveSettingsBulk.ts`:
- Around line 68-95: Collapse duplicate setting IDs before running
validateSettings() and before building the update promises in saveSettingsBulk
so each _id is processed once with a deterministic value. Ensure the deduped
params are what updateAuditedByUser, Settings.updateValueById,
Settings.findOneById, and notifyOnSettingChangedById operate on, so repeated
entries cannot make validation or the final stored setting order-dependent.
---
Nitpick comments:
In `@apps/meteor/app/lib/server/lib/settingValidationRules.ts`:
- Around line 10-14: The implementation file contains multiple explanatory
comments that violate the no-comments guideline. Remove the JSDoc block and the
inline comments around the relevant logic in settingValidationRules.ts,
including the sections tied to matches, resolveObject, conditions,
unknownReferences, and conditionHolds, while keeping the existing behavior
unchanged.
- Around line 44-49: The appliesWhen condition handling in
settingValidationRules is inconsistent with rule.query resolution because
condition.value is used raw, so $setting references inside appliesWhen can be
left unresolved and skipped by the unknown-reference check. Update the condition
processing in validateSettings/conditionHolds to run appliesWhen values through
the same resolveObject/resolveNode path used for rule.query, and make sure any
resolved references are added to references so the safety net covers them too.
In `@apps/meteor/app/lib/server/lib/validateSettings.ts`:
- Around line 10-14: Remove the implementation comments from
validateSettings.ts, including the module-level explanatory note near
SETTING_VALIDATION_ERROR and the inline comments around getValueOf and the
validation flow. Keep the logic in getValueOf, validateSettings, and any related
helpers unchanged, but rely on clear naming and structure instead of code
comments to explain the type/bounds checks and declared-rule validation.
In `@apps/meteor/tests/unit/app/lib/server/lib/settingValidationRules.spec.ts`:
- Around line 70-76: The unknown-reference coverage in
evaluateSettingValidationRule is incomplete because it only verifies the
$setting-in-query path. Add a unit test in settingValidationRules.spec.ts that
exercises a rule with appliesWhen._id pointing to a missing setting, and assert
it still passes while systemLoggerErrorMock is called and the logged references
include that missing setting; use evaluateSettingValidationRule, getterFor, and
systemLoggerErrorMock to match the existing test style.
In `@apps/meteor/tests/unit/app/lib/server/lib/validateSettings.spec.ts`:
- Around line 74-89: Add unit coverage in validateSettings.spec.ts for the other
validator paths in validateSettings, especially boolean, roomPick, multiSelect,
range, timespan, and code/JSON, so the suite exercises each type-specific
validator rather than only the int path. Also add a direct assertion around
validateJson’s thrown error shape in validateSettings to lock down the error
contract, using the existing validateSettings and validateJson symbols to locate
the affected logic.
In `@packages/core-typings/src/ISetting.ts`:
- Around line 34-39: The SettingValidationRule.query field is using a loose
Record<string, unknown> while similar query-filter fields on the same area use
the named EnableQuery type. Update the SettingValidationRule definition in
ISetting to reuse EnableQuery (or a shared query type) for query so the
interface stays consistent and more type-safe.
🪄 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: 2de970ad-88c6-4dac-b395-d6a0b2281046
📒 Files selected for processing (10)
apps/meteor/app/lib/server/functions/saveSettingsBulk.tsapps/meteor/app/lib/server/lib/settingValidationRules.tsapps/meteor/app/lib/server/lib/validateSettings.tsapps/meteor/app/lib/server/methods/saveSetting.tsapps/meteor/server/api/v1/settings.tsapps/meteor/server/settings/accounts.tsapps/meteor/tests/unit/app/lib/server/lib/settingValidationRules.spec.tsapps/meteor/tests/unit/app/lib/server/lib/validateSettings.spec.tspackages/core-typings/src/ISetting.tspackages/i18n/src/locales/en.i18n.json
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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:
apps/meteor/app/lib/server/lib/settingValidationRules.tsapps/meteor/server/settings/accounts.tspackages/core-typings/src/ISetting.tsapps/meteor/app/lib/server/methods/saveSetting.tsapps/meteor/server/api/v1/settings.tsapps/meteor/tests/unit/app/lib/server/lib/settingValidationRules.spec.tsapps/meteor/tests/unit/app/lib/server/lib/validateSettings.spec.tsapps/meteor/app/lib/server/lib/validateSettings.tsapps/meteor/app/lib/server/functions/saveSettingsBulk.ts
**/*.spec.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.spec.ts: Use descriptive test names that clearly communicate expected behavior in Playwright tests
Use.spec.tsextension for test files (e.g.,login.spec.ts)
Files:
apps/meteor/tests/unit/app/lib/server/lib/settingValidationRules.spec.tsapps/meteor/tests/unit/app/lib/server/lib/validateSettings.spec.ts
🧠 Learnings (5)
📚 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:
apps/meteor/app/lib/server/lib/settingValidationRules.tsapps/meteor/server/settings/accounts.tspackages/core-typings/src/ISetting.tsapps/meteor/app/lib/server/methods/saveSetting.tsapps/meteor/server/api/v1/settings.tsapps/meteor/tests/unit/app/lib/server/lib/settingValidationRules.spec.tsapps/meteor/tests/unit/app/lib/server/lib/validateSettings.spec.tsapps/meteor/app/lib/server/lib/validateSettings.tsapps/meteor/app/lib/server/functions/saveSettingsBulk.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:
apps/meteor/app/lib/server/lib/settingValidationRules.tsapps/meteor/server/settings/accounts.tspackages/core-typings/src/ISetting.tsapps/meteor/app/lib/server/methods/saveSetting.tsapps/meteor/server/api/v1/settings.tsapps/meteor/tests/unit/app/lib/server/lib/settingValidationRules.spec.tsapps/meteor/tests/unit/app/lib/server/lib/validateSettings.spec.tsapps/meteor/app/lib/server/lib/validateSettings.tsapps/meteor/app/lib/server/functions/saveSettingsBulk.ts
📚 Learning: 2026-05-06T12:21:44.083Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 40256
File: apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx:121-149
Timestamp: 2026-05-06T12:21:44.083Z
Learning: Field wrappers in rocket.chat/fuselage-forms (Field, FieldLabel, FieldRow, FieldError, FieldHint) auto-create htmlFor/id associations, aria-describedby, and role="alert" for errors. Do not manually set htmlFor, id, aria-describedby, or role attributes when using these wrappers. This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs. In code reviews, prefer using fuselage-forms wrappers for form fields and verify there is no unnecessary manual ID/aria wiring in files that use these wrappers. If a component uses plain fuselage components, ensure proper id wiring as per docs.
Applied to files:
apps/meteor/app/lib/server/lib/settingValidationRules.tsapps/meteor/server/settings/accounts.tspackages/core-typings/src/ISetting.tsapps/meteor/app/lib/server/methods/saveSetting.tsapps/meteor/server/api/v1/settings.tsapps/meteor/tests/unit/app/lib/server/lib/settingValidationRules.spec.tsapps/meteor/tests/unit/app/lib/server/lib/validateSettings.spec.tsapps/meteor/app/lib/server/lib/validateSettings.tsapps/meteor/app/lib/server/functions/saveSettingsBulk.ts
📚 Learning: 2026-02-24T19:22:48.358Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 38493
File: apps/meteor/tests/e2e/omnichannel/omnichannel-send-pdf-transcript.spec.ts:66-67
Timestamp: 2026-02-24T19:22:48.358Z
Learning: In Playwright end-to-end tests (e.g., under apps/meteor/tests/e2e/...), prefer locating elements by translated text (getByText) and ARIA roles (getByRole) over data-qa attributes. If translation values change, update the corresponding test locators accordingly. Never use data-qa locators. This guideline applies to all Playwright e2e test specs in the repository and helps keep tests robust to UI text changes and accessible semantics.
Applied to files:
apps/meteor/tests/unit/app/lib/server/lib/settingValidationRules.spec.tsapps/meteor/tests/unit/app/lib/server/lib/validateSettings.spec.ts
📚 Learning: 2026-03-06T18:10:15.268Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39397
File: packages/gazzodown/src/code/CodeBlock.spec.tsx:47-68
Timestamp: 2026-03-06T18:10:15.268Z
Learning: In tests (especially those using testing-library/dom/jsdom) for Rocket.Chat components, the HTML <code> element has an implicit ARIA role of 'code'. Therefore, screen.getByRole('code') or screen.findByRole('code') will locate <code> elements even without a role attribute. Do not flag findByRole('code') as invalid in reviews; prefer using the implicit role instead of adding role="code" unless necessary for accessibility.
Applied to files:
apps/meteor/tests/unit/app/lib/server/lib/settingValidationRules.spec.tsapps/meteor/tests/unit/app/lib/server/lib/validateSettings.spec.ts
🔇 Additional comments (10)
packages/core-typings/src/ISetting.ts (1)
40-102: LGTM!packages/i18n/src/locales/en.i18n.json (1)
395-400: LGTM!apps/meteor/app/lib/server/functions/saveSettingsBulk.ts (1)
2-13: LGTM!Also applies to: 47-58
apps/meteor/app/lib/server/methods/saveSetting.ts (1)
4-14: LGTM!Also applies to: 58-58
apps/meteor/server/api/v1/settings.ts (1)
31-31: LGTM!Also applies to: 385-385
apps/meteor/server/settings/accounts.ts (1)
829-855: LGTM!apps/meteor/app/lib/server/lib/settingValidationRules.ts (1)
1-67: LGTM! The$settingresolution,appliesWhengating, and unknown-reference safety fallback are logically sound — verified by manually tracing every test case in the accompanying spec file and the password-policy fixtures invalidateSettings.spec.ts.apps/meteor/tests/unit/app/lib/server/lib/settingValidationRules.spec.ts (1)
1-77: LGTM! Good coverage of the resolution, gating, and unknown-reference logging paths; traced each assertion againstevaluateSettingValidationRuleand all pass as expected.apps/meteor/app/lib/server/lib/validateSettings.ts (1)
52-78: LGTM! Batch-first value resolution, per-type validation, and rule evaluation wiring are correct — traced against thevalidateSettings.spec.tsfixtures including the batch-override and disabled-bound edge cases.apps/meteor/tests/unit/app/lib/server/lib/validateSettings.spec.ts (1)
60-199: LGTM! Strong coverage of batch-first resolution, disabled-bound (-1) semantics, and cross-field password-policy rules; manually traced several scenarios (raising min above stored max, lowering max below stored min, disabled bound skip) and all match the implementation.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #41173 +/- ##
===========================================
+ Coverage 68.48% 68.49% +0.01%
===========================================
Files 4092 4116 +24
Lines 158216 159777 +1561
Branches 28678 29007 +329
===========================================
+ Hits 108351 109444 +1093
- Misses 44827 45281 +454
- Partials 5038 5052 +14
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
393882d to
ff7cc36
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
apps/meteor/app/lib/server/methods/saveSettings.ts (1)
41-41: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueComment added to implementation.
As per coding guidelines,
**/*.{ts,tsx,js}files should "Avoid code comments in the implementation."🤖 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 `@apps/meteor/app/lib/server/methods/saveSettings.ts` at line 41, The implementation in saveSettings should remove the inline code comment inside the server method, since the guideline for ts/tsx/js files is to avoid comments in implementation. Delete the comment near the Meteor.Error rethrow logic and keep the behavior in the surrounding code unchanged.Source: Coding guidelines
apps/meteor/app/lib/server/methods/saveSetting.ts (2)
76-76: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueComment added to implementation.
As per coding guidelines,
**/*.{ts,tsx,js}files should "Avoid code comments in the implementation." The inline comment explaining the rethrow can be dropped since the code is self-explanatory (Meteor.Error with a descriptive error key).🤖 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 `@apps/meteor/app/lib/server/methods/saveSetting.ts` at line 76, The inline implementation comment in saveSetting should be removed because the surrounding Meteor.Error rethrow is already self-explanatory. Update the saveSetting method in saveSetting.ts by deleting the comment near the rethrow logic and keep the existing error-handling behavior unchanged.Source: Coding guidelines
73-78: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winValidation wiring looks correct; consider extracting the duplicated error-conversion logic.
The
try/catchthat converts a thrown validationErrorintoMeteor.Error('error-setting-validation-failed', ...)is identical to the block added insaveSettings.ts(Lines 38-43). Extracting a small shared helper (e.g.validateSettingRulesOrThrowinsettingValidationRules.ts) would remove this duplication across both Meteor methods.♻️ Suggested helper
+export const validateSettingRulesOrThrow = (changes: { _id: ISetting['_id']; value: ISetting['value'] }[]): void => { + try { + validateSettingRules(changes); + } catch (error) { + throw new Meteor.Error('error-setting-validation-failed', error instanceof Error ? error.message : String(error)); + } +};🤖 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 `@apps/meteor/app/lib/server/methods/saveSetting.ts` around lines 73 - 78, The error-conversion block in saveSetting duplicates the same try/catch logic used in saveSettings, so factor it into a shared helper instead of repeating it. Move the validation-and-rethrow behavior into a reusable function such as validateSettingRulesOrThrow in settingValidationRules.ts, then call that helper from both saveSetting and saveSettings so both methods keep the same Meteor.Error('error-setting-validation-failed', ...) behavior through a single implementation.apps/meteor/server/api/v1/settings.ts (1)
381-381: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueComments added to implementation.
As per coding guidelines,
**/*.{ts,tsx,js}files should "Avoid code comments in the implementation." Several new comments (the twoTODO(next major)notes and the two "message is the i18n key" explanations) were added across these blocks.Also applies to: 387-387, 429-429, 433-433
🤖 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 `@apps/meteor/server/api/v1/settings.ts` at line 381, The implementation in settings.ts contains new in-code comments that violate the no-comments guideline. Remove the two TODO(next major) notes and the two “message is the i18n key” explanation comments, keeping the logic in the affected validation blocks unchanged; use the nearby validation/translation code paths to ensure the behavior remains the same without inline commentary.Source: Coding guidelines
apps/meteor/tests/end-to-end/api/users.ts (1)
3638-3639: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueComment added to implementation.
As per coding guidelines,
**/*.{ts,tsx,js}files should "Avoid code comments in the implementation."🤖 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 `@apps/meteor/tests/end-to-end/api/users.ts` around lines 3638 - 3639, Remove the implementation comment in the test setup around the Accounts_Password_Policy_MinLength update, since the coding guidelines for this TypeScript file disallow code comments in implementation. Keep the behavior in the same test block unchanged and just delete the explanatory comment near updateSetting.Source: Coding guidelines
apps/meteor/tests/unit/app/lib/server/lib/settingValidationRules.spec.ts (3)
122-145: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider consolidating the duplicate assertion into one test.
Lines 122-129 and 131-145 both assert the same scenario (min > max fails with the min-side message) — one via chai's substring throw matcher, the other via manual try/catch for exact message equality. Could merge into a single test asserting the exact message.
🤖 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 `@apps/meteor/tests/unit/app/lib/server/lib/settingValidationRules.spec.ts` around lines 122 - 145, The two tests around validateSettingRules are duplicating the same failure scenario, so consolidate them into one spec. Keep a single test in settingValidationRules.spec.ts that exercises the min/max invalid batch and asserts the exact error message for Accounts_Password_Policy_MinLength_Invalid, using validateSettingRules directly instead of both a throw matcher and a manual try/catch.
84-107: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winManually mirrored validation rules risk silent drift from production.
This block duplicates the
Accounts_Password_Policy_MinLength/MaxLengthvalidation arrays fromserver/settings/accounts.tsby hand (per the comment on line 84). If the production rules change, this file won't fail to compile or flag the mismatch — it'll just keep testing stale rules. Consider importing the real validation arrays (or the settings module itself) so the tests stay in sync with production automatically.🤖 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 `@apps/meteor/tests/unit/app/lib/server/lib/settingValidationRules.spec.ts` around lines 84 - 107, The test fixture is manually duplicating the password policy validation rules, which can drift from the production source without any signal. Update the `validationBySettingId` setup in `settingValidationRules.spec.ts` to import and reuse the वास्तविक validation arrays from `server/settings/accounts.ts` (or the module exporting them) instead of mirroring `Accounts_Password_Policy_MinLength` and `Accounts_Password_Policy_MaxLength` by hand, so the spec stays aligned automatically.
9-9: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCode comments present in a
.tstest file.As per coding guidelines,
**/*.{ts,tsx,js}files should avoid code comments in the implementation. This file has several explanatory comments; note the guideline is framed around Playwright tests specifically, and this is a Mocha/chai unit test, so applicability may be limited.Also applies to: 49-50, 69-71, 205-206, 223-223
🤖 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 `@apps/meteor/tests/unit/app/lib/server/lib/settingValidationRules.spec.ts` at line 9, Remove the explanatory inline comments from settingValidationRules.spec.ts and keep the test self-explanatory through the surrounding describe/it names and assertions instead. Update the affected test blocks in settingValidationRules.spec.ts so the notes about createPredicateFromFilter and isRecord being pure, as well as the other listed explanatory comments, are no longer present while preserving the test behavior.Source: Coding guidelines
🤖 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 `@apps/meteor/tests/unit/app/lib/server/lib/settingValidationRules.spec.ts`:
- Around line 204-225: The two tests are not actually proving the behaviors in
their names. In the spec for validateSettingRules, update the “policy is
disabled” case so it targets a setting pair that is validated by
validationBySettingId and demonstrates the rule still runs regardless of
Accounts_Password_Policy_Enabled, and update the “ignores value types” case so
it uses a setting that does have validation rules rather than Some_Int_Setting.
Keep the assertions aligned with the actual rule logic so the tests fail if
those behaviors regress.
---
Nitpick comments:
In `@apps/meteor/app/lib/server/methods/saveSetting.ts`:
- Line 76: The inline implementation comment in saveSetting should be removed
because the surrounding Meteor.Error rethrow is already self-explanatory. Update
the saveSetting method in saveSetting.ts by deleting the comment near the
rethrow logic and keep the existing error-handling behavior unchanged.
- Around line 73-78: The error-conversion block in saveSetting duplicates the
same try/catch logic used in saveSettings, so factor it into a shared helper
instead of repeating it. Move the validation-and-rethrow behavior into a
reusable function such as validateSettingRulesOrThrow in
settingValidationRules.ts, then call that helper from both saveSetting and
saveSettings so both methods keep the same
Meteor.Error('error-setting-validation-failed', ...) behavior through a single
implementation.
In `@apps/meteor/app/lib/server/methods/saveSettings.ts`:
- Line 41: The implementation in saveSettings should remove the inline code
comment inside the server method, since the guideline for ts/tsx/js files is to
avoid comments in implementation. Delete the comment near the Meteor.Error
rethrow logic and keep the behavior in the surrounding code unchanged.
In `@apps/meteor/server/api/v1/settings.ts`:
- Line 381: The implementation in settings.ts contains new in-code comments that
violate the no-comments guideline. Remove the two TODO(next major) notes and the
two “message is the i18n key” explanation comments, keeping the logic in the
affected validation blocks unchanged; use the nearby validation/translation code
paths to ensure the behavior remains the same without inline commentary.
In `@apps/meteor/tests/end-to-end/api/users.ts`:
- Around line 3638-3639: Remove the implementation comment in the test setup
around the Accounts_Password_Policy_MinLength update, since the coding
guidelines for this TypeScript file disallow code comments in implementation.
Keep the behavior in the same test block unchanged and just delete the
explanatory comment near updateSetting.
In `@apps/meteor/tests/unit/app/lib/server/lib/settingValidationRules.spec.ts`:
- Around line 122-145: The two tests around validateSettingRules are duplicating
the same failure scenario, so consolidate them into one spec. Keep a single test
in settingValidationRules.spec.ts that exercises the min/max invalid batch and
asserts the exact error message for Accounts_Password_Policy_MinLength_Invalid,
using validateSettingRules directly instead of both a throw matcher and a manual
try/catch.
- Around line 84-107: The test fixture is manually duplicating the password
policy validation rules, which can drift from the production source without any
signal. Update the `validationBySettingId` setup in
`settingValidationRules.spec.ts` to import and reuse the वास्तविक validation
arrays from `server/settings/accounts.ts` (or the module exporting them) instead
of mirroring `Accounts_Password_Policy_MinLength` and
`Accounts_Password_Policy_MaxLength` by hand, so the spec stays aligned
automatically.
- Line 9: Remove the explanatory inline comments from
settingValidationRules.spec.ts and keep the test self-explanatory through the
surrounding describe/it names and assertions instead. Update the affected test
blocks in settingValidationRules.spec.ts so the notes about
createPredicateFromFilter and isRecord being pure, as well as the other listed
explanatory comments, are no longer present while preserving the test behavior.
🪄 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: 92f93f03-fcb7-4606-9505-503ff4da1627
📒 Files selected for processing (9)
apps/meteor/app/lib/server/lib/settingValidationRules.tsapps/meteor/app/lib/server/methods/saveSetting.tsapps/meteor/app/lib/server/methods/saveSettings.tsapps/meteor/server/api/v1/settings.tsapps/meteor/server/settings/accounts.tsapps/meteor/tests/end-to-end/api/users.tsapps/meteor/tests/unit/app/lib/server/lib/settingValidationRules.spec.tspackages/core-typings/src/ISetting.tspackages/i18n/src/locales/en.i18n.json
✅ Files skipped from review due to trivial changes (1)
- packages/i18n/src/locales/en.i18n.json
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/meteor/server/settings/accounts.ts
- packages/core-typings/src/ISetting.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
⚠️ CI failures not shown inline (5)
GitHub Check: Dionisio QA: Some checks did not pass
Conclusion: failure
**Conclusion:** failure
### Steps
- ✅ **No merge conflicts**
- ❌ **QA assured** — This PR is missing the 'stat: QA assured' label
- ✅ **Mergeable**
- ❌ **Has milestone or project** — This PR is missing the required milestone or project
- ✅ **Valid PR title**
- ✅ **Correct target version**
GitHub Check: Dionisio QA: Some checks did not pass
Conclusion: failure
**Conclusion:** failure
### Steps
- ✅ **No merge conflicts**
- ❌ **QA assured** — This PR is missing the 'stat: QA assured' label
- ✅ **Mergeable**
- ❌ **Has milestone or project** — This PR is missing the required milestone or project
- ✅ **Valid PR title**
- ✅ **Correct target version**
GitHub Check: Dionisio QA: Some checks did not pass
Conclusion: failure
**Conclusion:** failure
### Steps
- ✅ **No merge conflicts**
- ❌ **QA assured** — This PR is missing the 'stat: QA assured' label
- ✅ **Mergeable**
- ❌ **Has milestone or project** — This PR is missing the required milestone or project
- ✅ **Valid PR title**
- ✅ **Correct target version**
GitHub Check: Dionisio QA: Some checks did not pass
Conclusion: failure
**Conclusion:** failure
### Steps
- ✅ **No merge conflicts**
- ❌ **QA assured** — This PR is missing the 'stat: QA assured' label
- ✅ **Mergeable**
- ❌ **Has milestone or project** — This PR is missing the required milestone or project
- ✅ **Valid PR title**
- ✅ **Correct target version**
GitHub Check: Dionisio QA: Some checks did not pass
Conclusion: failure
**Conclusion:** failure
### Steps
- ✅ **No merge conflicts**
- ❌ **QA assured** — This PR is missing the 'stat: QA assured' label
- ✅ **Mergeable**
- ❌ **Has milestone or project** — This PR is missing the required milestone or project
- ✅ **Valid PR title**
- ✅ **Correct target version**
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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:
apps/meteor/app/lib/server/methods/saveSettings.tsapps/meteor/tests/end-to-end/api/users.tsapps/meteor/server/api/v1/settings.tsapps/meteor/app/lib/server/lib/settingValidationRules.tsapps/meteor/app/lib/server/methods/saveSetting.tsapps/meteor/tests/unit/app/lib/server/lib/settingValidationRules.spec.ts
**/*.spec.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.spec.ts: Use descriptive test names that clearly communicate expected behavior in Playwright tests
Use.spec.tsextension for test files (e.g.,login.spec.ts)
Files:
apps/meteor/tests/unit/app/lib/server/lib/settingValidationRules.spec.ts
🧠 Learnings (5)
📚 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:
apps/meteor/app/lib/server/methods/saveSettings.tsapps/meteor/tests/end-to-end/api/users.tsapps/meteor/server/api/v1/settings.tsapps/meteor/app/lib/server/lib/settingValidationRules.tsapps/meteor/app/lib/server/methods/saveSetting.tsapps/meteor/tests/unit/app/lib/server/lib/settingValidationRules.spec.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:
apps/meteor/app/lib/server/methods/saveSettings.tsapps/meteor/tests/end-to-end/api/users.tsapps/meteor/server/api/v1/settings.tsapps/meteor/app/lib/server/lib/settingValidationRules.tsapps/meteor/app/lib/server/methods/saveSetting.tsapps/meteor/tests/unit/app/lib/server/lib/settingValidationRules.spec.ts
📚 Learning: 2026-05-06T12:21:44.083Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 40256
File: apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx:121-149
Timestamp: 2026-05-06T12:21:44.083Z
Learning: Field wrappers in rocket.chat/fuselage-forms (Field, FieldLabel, FieldRow, FieldError, FieldHint) auto-create htmlFor/id associations, aria-describedby, and role="alert" for errors. Do not manually set htmlFor, id, aria-describedby, or role attributes when using these wrappers. This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs. In code reviews, prefer using fuselage-forms wrappers for form fields and verify there is no unnecessary manual ID/aria wiring in files that use these wrappers. If a component uses plain fuselage components, ensure proper id wiring as per docs.
Applied to files:
apps/meteor/app/lib/server/methods/saveSettings.tsapps/meteor/tests/end-to-end/api/users.tsapps/meteor/server/api/v1/settings.tsapps/meteor/app/lib/server/lib/settingValidationRules.tsapps/meteor/app/lib/server/methods/saveSetting.tsapps/meteor/tests/unit/app/lib/server/lib/settingValidationRules.spec.ts
📚 Learning: 2026-02-24T19:22:48.358Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 38493
File: apps/meteor/tests/e2e/omnichannel/omnichannel-send-pdf-transcript.spec.ts:66-67
Timestamp: 2026-02-24T19:22:48.358Z
Learning: In Playwright end-to-end tests (e.g., under apps/meteor/tests/e2e/...), prefer locating elements by translated text (getByText) and ARIA roles (getByRole) over data-qa attributes. If translation values change, update the corresponding test locators accordingly. Never use data-qa locators. This guideline applies to all Playwright e2e test specs in the repository and helps keep tests robust to UI text changes and accessible semantics.
Applied to files:
apps/meteor/tests/unit/app/lib/server/lib/settingValidationRules.spec.ts
📚 Learning: 2026-03-06T18:10:15.268Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39397
File: packages/gazzodown/src/code/CodeBlock.spec.tsx:47-68
Timestamp: 2026-03-06T18:10:15.268Z
Learning: In tests (especially those using testing-library/dom/jsdom) for Rocket.Chat components, the HTML <code> element has an implicit ARIA role of 'code'. Therefore, screen.getByRole('code') or screen.findByRole('code') will locate <code> elements even without a role attribute. Do not flag findByRole('code') as invalid in reviews; prefer using the implicit role instead of adding role="code" unless necessary for accessibility.
Applied to files:
apps/meteor/tests/unit/app/lib/server/lib/settingValidationRules.spec.ts
🔇 Additional comments (11)
apps/meteor/app/lib/server/lib/settingValidationRules.ts (3)
1-9: LGTM!
16-67: LGTM!
69-91: LGTM!apps/meteor/app/lib/server/methods/saveSettings.ts (1)
38-43: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate error-conversion logic.
Same pattern as
saveSetting.ts(Lines 73-78). See the consolidated refactor suggestion there — extracting a shared helper would avoid maintaining this conversion in two (soon four, countingsettings.ts) places.apps/meteor/server/api/v1/settings.ts (4)
384-390: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate error-conversion logic (REST variant).
Same
try { validateSettingRules(...) } catch { return API.v1.failure(...) }pattern is repeated twice in this file and mirrors the Meteor-method variant insaveSetting.ts/saveSettings.ts. Consider a shared helper (e.g., returning a discriminated result) reused across both REST handlers and the two Meteor methods.Also applies to: 429-436
381-390: 📐 Maintainability & Code QualityDual validation paths run for the same value; consider tracking consolidation.
checkSettingValueBounds(setting, bodyParams.value)(unwrapped, throwsMeteor.Error('error-invalid-setting-value', ...)) still runs immediately before the newvalidateSettingRulescheck. This is already flagged by the existingTODO(next major)comment, so it's an acknowledged transitional state rather than a new defect, but note that a numeric-bounds failure will surface a different error shape (thrownMeteor.Error, notAPI.v1.failure) than avalidateSettingRulesfailure on the same request.
327-327: LGTM!
32-32: 🎯 Functional Correctness
checkSettingValueBoundsis still imported and used. The summary is outdated;apps/meteor/server/api/v1/settings.tsstill importscheckSettingValueBoundsand calls it at line 382.> Likely an incorrect or invalid review comment.apps/meteor/tests/end-to-end/api/users.ts (1)
3596-3609: 🎯 Functional CorrectnessNo change needed:
Accounts_Password_Policy_MinLengthis already lowered to1in the block and never raised again, so restoringAccounts_Password_Policy_MaxLengthfirst inafter()does not violate the min/max constraint.> Likely an incorrect or invalid review comment.apps/meteor/tests/unit/app/lib/server/lib/settingValidationRules.spec.ts (2)
1-83: LGTM!Also applies to: 109-203, 215-221, 226-227
12-15: 🎯 Functional CorrectnessProxyquire stubs match the imports
The stub keys align withsettingValidationRules.ts’s import specifiers, so this test setup is correct.
ff7cc36 to
1a506c3
Compare
61dd515 to
2c2f0a9
Compare
coderabbitai
Bot
added
type: bug
and removed
type: feature
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/core-typings/src/ISetting.ts (1)
34-41: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider exporting
SettingValidation.
SettingValidationRuleis exported but the union aliasSettingValidation(line 40) is not, even though it's the type used on the publicvalidationfield ofISettingBase. Other modules (e.g.getSettingDefaults.ts) referenceoptions.validationstructurally rather than importing this alias directly, but exporting it would make the contract more discoverable/reusable.♻️ Proposed fix
-type SettingValidation = SettingValidationRule[] | string; +export type SettingValidation = SettingValidationRule[] | string;🤖 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 `@packages/core-typings/src/ISetting.ts` around lines 34 - 41, Export the `SettingValidation` union from `ISetting.ts` so the public `validation` contract on `ISettingBase` is discoverable and reusable alongside `SettingValidationRule`. Update the type declaration near `SettingValidationRule` and keep the existing shape (`SettingValidationRule[] | string`) unchanged; this lets other modules like `getSettingDefaults` refer to the shared alias instead of relying on structural typing.apps/meteor/app/lib/server/lib/settingValidationRules.ts (2)
35-39: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCode comments present despite the "avoid code comments" guideline.
This file has explanatory inline comments (the docstring at Lines 35-39 and single-line comments at Lines 47, 51-52, 82-83, 94-95). As per coding guidelines,
**/*.{ts,tsx,js}files should avoid code comments in the implementation. Given the non-trivial recursive resolution logic here, these comments genuinely aid readability, but per the stated guideline they should be reconsidered/removed if strict compliance is required.Also applies to: 47-52, 82-95
🤖 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 `@apps/meteor/app/lib/server/lib/settingValidationRules.ts` around lines 35 - 39, Remove the implementation comments in settingValidationRules.ts to comply with the “avoid code comments” guideline, including the top docstring and the inline comments around the recursive validation flow. Keep the behavior in the same functions and symbols (such as the validation rule evaluator and its helper logic) unchanged, and if any explanation is still needed, prefer clearer naming or refactoring over comments.Source: Coding guidelines
8-9: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winStrengthen
isValidationRuleArrayshape validation.The guard only checks that
'query'and'errorKey'keys exist on each parsed element, not thatqueryis actually an object or thaterrorKeyis a string. A malformed rule like{ query: "oops", errorKey: 42 }will pass this check and flow intoevaluateSettingValidationRule, whereresolveObject/Object.entrieson a non-objectquery(e.g. a string) silently produces a bogus filter instead of failing open with a logged error, which is the stated intent for broken rule declarations.♻️ Proposed fix
-const isValidationRuleArray = (value: unknown): value is SettingValidationRule[] => - Array.isArray(value) && value.every((rule) => isRecord(rule) && 'query' in rule && 'errorKey' in rule); +const isValidationRuleArray = (value: unknown): value is SettingValidationRule[] => + Array.isArray(value) && + value.every((rule) => isRecord(rule) && isRecord(rule.query) && typeof rule.errorKey === 'string');Also applies to: 24-27
🤖 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 `@apps/meteor/app/lib/server/lib/settingValidationRules.ts` around lines 8 - 9, Strengthen the `isValidationRuleArray` type guard so it validates the actual shape of each `SettingValidationRule`, not just the presence of keys. Update the check in `settingValidationRules.ts` to require that each item is a record with `query` being a non-null object and `errorKey` being a string before it can pass. This should align with how `evaluateSettingValidationRule` and `resolveObject` expect to consume `query`, preventing malformed rules from slipping through and producing bogus filters.
🤖 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.
Nitpick comments:
In `@apps/meteor/app/lib/server/lib/settingValidationRules.ts`:
- Around line 35-39: Remove the implementation comments in
settingValidationRules.ts to comply with the “avoid code comments” guideline,
including the top docstring and the inline comments around the recursive
validation flow. Keep the behavior in the same functions and symbols (such as
the validation rule evaluator and its helper logic) unchanged, and if any
explanation is still needed, prefer clearer naming or refactoring over comments.
- Around line 8-9: Strengthen the `isValidationRuleArray` type guard so it
validates the actual shape of each `SettingValidationRule`, not just the
presence of keys. Update the check in `settingValidationRules.ts` to require
that each item is a record with `query` being a non-null object and `errorKey`
being a string before it can pass. This should align with how
`evaluateSettingValidationRule` and `resolveObject` expect to consume `query`,
preventing malformed rules from slipping through and producing bogus filters.
In `@packages/core-typings/src/ISetting.ts`:
- Around line 34-41: Export the `SettingValidation` union from `ISetting.ts` so
the public `validation` contract on `ISettingBase` is discoverable and reusable
alongside `SettingValidationRule`. Update the type declaration near
`SettingValidationRule` and keep the existing shape (`SettingValidationRule[] |
string`) unchanged; this lets other modules like `getSettingDefaults` refer to
the shared alias instead of relying on structural typing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: edb8b728-fd55-41f1-b331-ea608651a350
📒 Files selected for processing (11)
.changeset/empty-garlics-reply.mdapps/meteor/app/lib/server/lib/settingValidationRules.tsapps/meteor/app/lib/server/methods/saveSetting.tsapps/meteor/app/lib/server/methods/saveSettings.tsapps/meteor/app/settings/server/functions/getSettingDefaults.tsapps/meteor/server/api/v1/settings.tsapps/meteor/server/settings/accounts.tsapps/meteor/tests/end-to-end/api/users.tsapps/meteor/tests/unit/app/lib/server/lib/settingValidationRules.spec.tspackages/core-typings/src/ISetting.tspackages/i18n/src/locales/en.i18n.json
✅ Files skipped from review due to trivial changes (2)
- .changeset/empty-garlics-reply.md
- packages/i18n/src/locales/en.i18n.json
🚧 Files skipped from review as they are similar to previous changes (6)
- apps/meteor/app/lib/server/methods/saveSetting.ts
- apps/meteor/tests/end-to-end/api/users.ts
- apps/meteor/app/lib/server/methods/saveSettings.ts
- apps/meteor/server/settings/accounts.ts
- apps/meteor/server/api/v1/settings.ts
- apps/meteor/tests/unit/app/lib/server/lib/settingValidationRules.spec.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Hacktron Security Check
⚠️ CI failures not shown inline (2)
GitHub Actions: CI / 19_📦 Track Image Sizes.txt: feat: validate password policy length on settings save
Conclusion: failure
##[group]Run current_total=$(jq -r '.total' current-sizes.json)
�[36;1mcurrent_total=$(jq -r '.total' current-sizes.json)�[0m
�[36;1m�[0m
�[36;1mif [[ ! -f baseline-sizes.json ]]; then�[0m
�[36;1m echo "No baseline available"�[0m
�[36;1m echo "size-diff=0" >> $GITHUB_OUTPUT�[0m
�[36;1m echo "size-diff-percent=0" >> $GITHUB_OUTPUT�[0m
�[36;1m echo "comment-triggered=false" >> $GITHUB_OUTPUT�[0m
�[36;1m�[0m
�[36;1m cat > report.md << 'EOF'�[0m
�[36;1m# 📦 Docker Image Size Report�[0m
�[36;1m�[0m
�[36;1m**Status:** First measurement - no baseline for comparison�[0m
�[36;1m�[0m
�[36;1m**Total Size:** $(numfmt --to=iec-i --suffix=B $current_total)�[0m
�[36;1mEOF�[0m
�[36;1m exit 0�[0m
�[36;1mfi�[0m
�[36;1m�[0m
�[36;1mbaseline_total=$(jq -r '.total' baseline-sizes.json)�[0m
�[36;1mdiff=$((current_total - baseline_total))�[0m
�[36;1m�[0m
�[36;1mif [[ $baseline_total -gt 0 ]]; then�[0m
�[36;1m percent=$(awk "BEGIN {printf \"%.2f\", ($diff / $baseline_total) * 100}")�[0m
�[36;1melse�[0m
�[36;1m percent=0�[0m
�[36;1mfi�[0m
�[36;1m�[0m
�[36;1mecho "size-diff=$diff" >> $GITHUB_OUTPUT�[0m
�[36;1mecho "size-diff-percent=$percent" >> $GITHUB_OUTPUT�[0m
�[36;1m�[0m
�[36;1m# Only comment when size is bigger than baseline; optionally require per-image thresholds�[0m
�[36;1mTHRESHOLDS="$SIZE_THRESHOLDS"�[0m
�[36;1mFAIL_THRESHOLDS="$FAIL_THRESHOLDS"�[0m
�[36;1mcomment_triggered=false�[0m
�[36;1mfail_triggered=false�[0m
�[36;1mif [[ $diff -gt 0 ]]; then�[0m
�[36;1m if [[ -z "$THRESHOLDS" ]] || [[ "$THRESHOLDS" == "{}" ]]; then�[0m
�[36;1m comment_triggered=true�[0m
�[36;1m fi�[0m
�[36;1mfi�[0m
�[36;1m�[0m
�[36;1mcolor="gray"�[0m
�[36;1mif (( $(awk "BEGIN {print ($percent > 0.01)}") )); then�[0m
�[36;1m color="red"�[0m
�[36;1melif (( $(awk "BEGIN {print ($percent < -0.01)}") )); then�[0m
�[36;1m color="green"�[0m
�[36;1mfi�[0m
�[36;1m�[0m
�[36;1m# Generate report�[0m
�[36;1mif [[ $diff -gt 0 ]]; then�[0m
�[36;1m emoji=...
GitHub Actions: CI / 33_🔨 Test Unit _ Unit Tests.txt: feat: validate password policy length on settings save
Conclusion: failure
et=rs0&directConnection=true
TOOL_NODE_FLAGS: --max_old_space_size=4096
ENTERPRISE_LICENSE: MK+bpK5NveUuNlWGaQXGoy+8b74Luet82M3ZGcBB8b5P9Y+m67NEtpW64dc1d5lEWi6d0nFjCjtCMneVD7bKxodz/Cml8URKEo5P7cQb/9wmeT0MzAhYNaRFZlIGkZ3ITF59pDV2u4HZuosEDJikVRwnaJ5ZoU/pOsHSPUPhTyGNIqLeKynODtUpfwDdIKEmHxpf2yVkKjgRiIJmbWjM6A4k+MNNYXWVXHzye7GggqWVg/ZcT7nKU1CCadpLhTJiIrgrrPzil1G5DQ4xnLs3Q2tu2dILSDiW5OYw/ywu2yCMicTjMq4MLL5SXDQJj6WoJzZ54HosbvsDzOXvsdC9gI1CjhPL2uRuvC8XLrzn3vL2UgXnifzD1VrLTtdZ+aSADveqtlzYlRWtqoUFBbNw8o+YVHdhbZGR0beMoAyRbHi5EMpxpad3L+NyztUIT/Uh/IjQ/C2SQZ6jB0GKPBOPxFLN56FNhTGrffLFR++TVoBu0Iquc7kajWkNit3bVbZvbx+oFcVW2PcjQ/+i2jpJjbgtUFUKrTKxGMAXTWoDzIQQ35zNzGAy268IM4Ymp5JmsVEnBOEUkbF9yx6fzkO6xZhpsHf0muklnW0kA+Tlore/TUrBWh1/RwWlQeZlxM5NyWoRM5onQmr/k/4BmObtL1Hpmbk8oMG29z89xtE9y/4=
NODE_VERSION: 22.22.3
DENO_VERSION: 2.3.1
MONGOMS_DOWNLOAD_DIR: /home/runner/work/_temp/mongodb-memory-server
MONGOMS_PREFER_GLOBAL_PATH: false
TURBOGHA_PORT: 41230
TURBO_API: http://localhost:41230
TURBO_***REDACTED***
TURBO_TEAM: turbogha
##[endgroup]
##[group]Run missing_deps=""
�[36;1mmissing_deps=""�[0m
�[36;1m�[0m
�[36;1m# Check for always-required commands�[0m
�[36;1mfor cmd in bash git curl; do�[0m
�[36;1m if ! command -v "$cmd" >/dev/null 2>&1; then�[0m
�[36;1m missing_deps="$missing_deps $cmd"�[0m
�[36;1m fi�[0m
�[36;1mdone�[0m
�[36;1m�[0m
�[36;1m# Check for gpg only if validation is not being skipped�[0m
�[36;1mif [ "$INPUT_SKIP_VALIDATION" != "true" ]; then�[0m
�[36;1m if ! command -v gpg >/dev/null 2>&1; then�[0m
�[36;1m missing_deps="$missing_deps gpg"�[0m
�[36;1m fi�[0m
�[36;1mfi�[0m
�[36;1m�[0m
�[36;1m# Report missing required dependencies�[0m
�[36;1mif [ -n "$missing_deps" ]; then�[0m
�[36;1m echo "Error: The following required dependencies are missing:$missing_deps"�[0m
�[36;1m echo "Please install these dependencies before using this action."�[0m
�[36;1m exit 1�[0m
�[36;1mfi�[0m
�[36;1m�[0m
�[36;1mecho "All require...
🧰 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:
apps/meteor/app/settings/server/functions/getSettingDefaults.tspackages/core-typings/src/ISetting.tsapps/meteor/app/lib/server/lib/settingValidationRules.ts
🧠 Learnings (3)
📚 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:
apps/meteor/app/settings/server/functions/getSettingDefaults.tspackages/core-typings/src/ISetting.tsapps/meteor/app/lib/server/lib/settingValidationRules.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:
apps/meteor/app/settings/server/functions/getSettingDefaults.tspackages/core-typings/src/ISetting.tsapps/meteor/app/lib/server/lib/settingValidationRules.ts
📚 Learning: 2026-05-06T12:21:44.083Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 40256
File: apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx:121-149
Timestamp: 2026-05-06T12:21:44.083Z
Learning: Field wrappers in rocket.chat/fuselage-forms (Field, FieldLabel, FieldRow, FieldError, FieldHint) auto-create htmlFor/id associations, aria-describedby, and role="alert" for errors. Do not manually set htmlFor, id, aria-describedby, or role attributes when using these wrappers. This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs. In code reviews, prefer using fuselage-forms wrappers for form fields and verify there is no unnecessary manual ID/aria wiring in files that use these wrappers. If a component uses plain fuselage components, ensure proper id wiring as per docs.
Applied to files:
apps/meteor/app/settings/server/functions/getSettingDefaults.tspackages/core-typings/src/ISetting.tsapps/meteor/app/lib/server/lib/settingValidationRules.ts
🔇 Additional comments (4)
packages/core-typings/src/ISetting.ts (1)
76-76: LGTM!apps/meteor/app/settings/server/functions/getSettingDefaults.ts (1)
29-29: LGTM! Consistent with the existingenableQuery/displayQuerystringification pattern.apps/meteor/app/lib/server/lib/settingValidationRules.ts (2)
93-115: LGTM! Batch-first value resolution and fail-open handling of missing settings look correct.
40-91: 🎯 Functional CorrectnessNo issue:
createPredicateFromFilteraccepts this document shape, andundefinedvalues are already handled as expected.> Likely an incorrect or invalid review comment.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 11 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 5 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
2352fd2 to
528f8d2
Compare
72b413e to
d9000ab
Compare
ricardogarim
added
the
stat: QA assured
dionisio-bot
Bot
added
the
stat: ready to merge
dionisio-bot
Bot
removed
the
stat: ready to merge
dionisio-bot
Bot
added
the
stat: ready to merge
3 participants
Proposed changes (including videos or screenshots)
Admins could save a password policy with an enabled maximum length (
>= 1) smaller than the minimum length (e.g. min=12, max=8), making it impossible to create any valid password.The server now rejects that configuration on every settings write path (REST and the deprecated methods), and the admin sees an error toast.
validationfield (SettingValidationRule[]): a mongo-stylequery, an optionalappliesWhengate, and an i18nerrorKey. Queries can reference other settings via{ $setting: '<id>' }, so cross-setting constraints are expressible — and future ones are just data on the spec.-1(disabled) always passes.error(witherrorType: 'error-setting-validation-failed') or as theMeteor.Errorreason on the methods — the existing client error handling translates it, so no client changes.saveSettingsBulkhas no diff): no request that succeeded before fails now, except the invalid pair. Broken rule declarations fail open (logged).TODO(next major)marks where the per-path validations should be unified.Issue(s)
CORE-2081
Steps to test or reproduce
-1→ any minimum saves normally.POST /api/v1/settings/Accounts_Password_Policy_MaxLengthwith{ "value": 4 }(while min=12) →400,errorType: "error-setting-validation-failed".Further comments
Other settings that could adopt this validation in the future
Candidates mapped while designing this — none implemented here, but each was verified against its registration and runtime consumer (exact ids, defaults, gating toggles, and what
0/negative values actually do):Conditional rules (
appliesWhena feature toggle):Accounts_Password_Policy_ForbidRepeatingCharactersCount≥ 1whenForbidRepeatingCharactersis on (0makes the regex reject every password)Accounts_Password_History_Amount≥ 1whenPassword_History_EnabledBlock_Multiple_Failed_Logins_Attempts_Until_Block_by_User/_By_Ip≥ 1when the corresponding block toggle is onBlock_Multiple_Failed_Logins_Time_To_Unblock_By_User_In_Minutes/_By_Ip_In_Minutes≥ 1when the corresponding block toggle is onAccounts_TwoFactorAuthentication_Max_Invalid_Email_Code_Attempts≥ 1when 2FA by email is onAccounts_TwoFactorAuthentication_By_Email_Code_Expiration> 0when 2FA by email is onAccounts_TwoFactorAuthentication_MaxDelta/_RememberFor≥ 0when 2FA is onMagic-value rules (like
-1here):FileUpload_MaxFileSize—-1(unlimited) or≥ 1(the guard is> -1, so0blocks every upload)Livechat_max_queue_wait_time—-1(unlimited) or≥ 1(0also disables at runtime today; the rule would canonicalize-1)Livechat_maximum_chats_per_agent— confirm0= unlimitedMessage_AllowEditing_BlockEditInMinutes/Message_AllowDeleting_BlockDeleteInMinutes—0disables blocking, so0or≥ 1Cross-field pairs: a repo-wide scan found no other min/max pair — the password policy lengths are currently the only settings needing a
$settingcross-reference.Plain static bounds — expressible today as a degenerate
{ value: { $gte: N } }rule (also localizes the error), or, more leanly, by unlockingminValue/maxValueonintsettings (typings currently permit them only onrange;checkSettingValueBoundsalready enforces them at runtime but with a hardcoded English message):DirectMesssage_maxUsers≥ 2(a DM needs at least two people)RetentionPolicy_MaxAge_Channels/_DMs/_Groups≥ 1(days)Accounts_AvatarSize≥ 1(pixels)Message_MaxAllowedSize≥ 1Notifications_Max_Room_Members≥ 0Accounts_Default_User_Preferences_idleTimeLimit≥ 0(seconds)LDAP_Search_Size_Limit≥ 0*_Requests_Allowed/*_Interval_Time≥ 1(batch)Summary by CodeRabbit