feat: Introduce RangeSettingInput
#37038
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
🦋 Changeset detectedLatest commit: 36bd4ec The changes in this PR will be included in the next version bump. This PR includes changesets to release 42 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 |
WalkthroughAdds a slider-style Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Admin
participant UI as RangeSettingInput (client)
participant API as Settings API / saveSettings (server)
participant Store as Settings Store
Admin->>UI: adjust slider / submit value
UI->>API: POST /settings {_id, value}
API->>API: checkInteger(value)
API->>API: checkSettingValueBounds(setting, value)
alt value within bounds
API->>Store: persist setting value
Store-->>API: success
API->>UI: success / notify
API->>API: audit(action, meta)
else invalid
API-->>UI: Meteor.Error (error-invalid-setting-value)
end
sequenceDiagram
autonumber
participant Loader as Settings Loader
participant Defaults as getSettingDefaults()
participant Typings as isSettingRange()
Loader->>Defaults: request defaults
Defaults->>Typings: isSettingRange(setting)
alt setting is 'range'
Defaults-->>Loader: setting + { minValue: 0, maxValue: 100 }
else
Defaults-->>Loader: setting unchanged
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #37038 +/- ##
===========================================
+ Coverage 67.62% 67.66% +0.04%
===========================================
Files 3339 3339
Lines 113919 113876 -43
Branches 20664 20641 -23
===========================================
+ Hits 77034 77056 +22
+ Misses 34213 34148 -65
Partials 2672 2672
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
dougfabris
force-pushed
the
feat/slider-setting
branch
from
afee567 to
10f5e7e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
packages/core-typings/src/ISetting.ts (2)
27-31: Capture future needs: step and documentationConsider adding an optional step to express non-unit increments and document that minValue ≤ maxValue and the value is integer (per server validation).
export interface ISettingSlider extends Omit<ISettingBase, 'type'> { type: 'slider'; minValue: number; maxValue: number; + /** Increment step for the slider; defaults to 1 if omitted */ + step?: number; }
33-33: Union looks good; add a dedicated type guard for ergonomicsMatching existing patterns (e.g., color/code), export a helper to narrow to ISettingSlider.
Add below the other guards:
export const isSettingSlider = (setting: ISettingBase): setting is ISettingSlider => setting.type === 'slider';apps/meteor/client/views/admin/settings/Setting/inputs/SliderSettingInput.stories.tsx (2)
6-14: Consider adding an explicit title for stable Storybook IDsHelps with permalinks and test runner selectors.
export default { component: SliderSettingInput, + title: 'Admin/Settings/SliderSettingInput', parameters: { actions: { argTypesRegex: '^on.*', }, }, decorators: [(fn) => <Field>{fn()}</Field>], } satisfies Meta<typeof SliderSettingInput>;
38-42: Provide a reset handler in the storyWithout an explicit handler, clicking Reset may no-op depending on auto-actions detection. Add a stub for clarity.
export const WithResetButton = Template.bind({}); WithResetButton.args = { value: 50, hasResetButton: true, + onResetButtonClick: () => {}, };.changeset/new-bats-pull.md (1)
2-6: Changeset targets correct packages
Only@rocket.chat/core-typingsand@rocket.chat/meteorrequire bumps for this change. Consider updating the changeset description to mention the newISettingSlidertypings incore-typingsand the server-side validation added inapps/meteor.apps/meteor/client/views/admin/settings/Setting/inputs/SliderSettingInput.tsx (1)
46-46: Consider bounds validation for the value prop.The value coercion
Number(value || 0)handles undefined/null cases correctly. However, there's no validation that the value falls within[minValue, maxValue]. While theSlidercomponent likely enforces this, adding explicit bounds clamping would make the component more defensive:-value={Number(value || 0)} +value={Math.max(minValue, Math.min(maxValue, Number(value || 0)))}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
apps/meteor/client/views/admin/settings/Setting/inputs/__snapshots__/SliderSettingInput.spec.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (8)
.changeset/new-bats-pull.md(1 hunks)apps/meteor/app/lib/server/methods/saveSettings.ts(1 hunks)apps/meteor/client/views/admin/settings/Setting/MemoizedSetting.tsx(2 hunks)apps/meteor/client/views/admin/settings/Setting/inputs/SliderSettingInput.spec.tsx(1 hunks)apps/meteor/client/views/admin/settings/Setting/inputs/SliderSettingInput.stories.tsx(1 hunks)apps/meteor/client/views/admin/settings/Setting/inputs/SliderSettingInput.tsx(1 hunks)apps/meteor/server/settings/accounts.ts(1 hunks)packages/core-typings/src/ISetting.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: 🚢 Build Docker Images for Testing (alpine)
🔇 Additional comments (13)
packages/core-typings/src/ISetting.ts (1)
57-58: Confirm inclusion of 'slider' in base type is intentionalIncluding 'slider' in ISettingBase['type'] keeps inputsByType keyed by base types working. Just ensure code that relies on min/max narrows to ISettingSlider (via the guard above) before accessing those fields.
apps/meteor/client/views/admin/settings/Setting/MemoizedSetting.tsx (2)
22-22: Import looks good
45-46: Slider bounds are forwarded correctly
minValueandmaxValuefrom ISettingSlider are included in {...setting}, passed into MemoizedSetting as inputProps, and reach SliderSettingInput as expected.apps/meteor/app/lib/server/methods/saveSettings.ts (1)
92-100: Enforce slider min/max on the server
ImportISettingSliderand add a range check for slider settings:-import type { ISetting } from '@rocket.chat/core-typings'; +import type { ISetting, ISettingSlider } from '@rocket.chat/core-typings'; @@ - case 'timespan': - case 'int': - case 'slider': + case 'timespan': + case 'int': + case 'slider': check(value, Number); if (!Number.isInteger(value)) { throw new Meteor.Error(`Invalid setting value ${value}`, 'Invalid setting value', { method: 'saveSettings', }); } + if (setting?.type === 'slider') { + const { minValue, maxValue } = setting as ISettingSlider; + if (value < minValue || value > maxValue) { + throw new Meteor.Error( + `Invalid setting value ${value}`, + `Value must be between ${minValue} and ${maxValue}`, + { method: 'saveSettings' }, + ); + } + } break;Verify that
ISettingSliderin@rocket.chat/core-typingsdefinesminValueandmaxValue.apps/meteor/client/views/admin/settings/Setting/inputs/SliderSettingInput.tsx (3)
1-11: LGTM!The imports and type definition are well-structured. The
SliderSettingInputPropscorrectly extends the baseSettingInputProps<number>with slider-specific properties.
13-26: LGTM!The component signature is comprehensive with sensible defaults (
minValue=0,maxValue=100) that align with the volume settings use case mentioned in the PR objectives.
41-48: Add explicitidto<Slider>or confirm its auto-generation.The
<FieldLabel htmlFor={_id}/>needs a matchingidon the<Slider>for proper association. Either passid={_id}to the Slider or verify that@rocket.chat/fuselage’s Slider auto-sets itsidfromdata-qa-setting-id.apps/meteor/client/views/admin/settings/Setting/inputs/SliderSettingInput.spec.tsx (3)
1-7: LGTM!The test setup correctly uses
composeStoriesto load stories and generate test cases. This is a standard pattern for testing Storybook components.
9-12: LGTM!The snapshot test correctly renders each story and captures the
baseElementfor comprehensive snapshot coverage.
14-19: LGTM!The accessibility test properly uses
jest-axeto validate that each story has no a11y violations. This is excellent practice for ensuring the component meets accessibility standards.apps/meteor/server/settings/accounts.ts (3)
711-727: LGTM!The conversion of
notificationsSoundVolumeandvoipRingerVolumesettings to slider type follows the same pattern asmasterVolume, with consistentminValue: 0andmaxValue: 100constraints. This provides a uniform user experience across all volume settings.
702-709: i18n slider description keys present
The keys Master_volume_hint, Notification_volume_hint and Call_ringer_volume_hint exist in the translation files; no further action required.
702-727: No action needed: slider type is already supported.
saveSettings.tsincludes acase 'slider'alongsideint, validating the value as a Number and enforcing integer constraints, so server-side handling is correct.
dougfabris
changed the title
SliderSettingInputRangeSettingInput
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/meteor/client/views/admin/settings/Setting/inputs/RangeSettingInput.stories.tsx (1)
16-18: Consider adding stories for different range configurations.The Template hardcodes
minValue={0}andmaxValue={100}, which limits the ability to demonstrate different range configurations or edge cases.Consider adding stories that showcase:
- Different ranges (e.g., 1-10, 0-1, -100-100)
- Boundary values (value at minValue, value at maxValue)
- Edge cases (minValue === maxValue)
Example:
export const CustomRange = Template.bind({}); CustomRange.args = { value: 5, minValue: 1, maxValue: 10, }; export const AtMinValue = Template.bind({}); AtMinValue.args = { value: 0, }; export const AtMaxValue = Template.bind({}); AtMaxValue.args = { value: 100, };This would improve test coverage and documentation of the component's behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
.changeset/new-bats-pull.md(1 hunks)apps/meteor/app/lib/server/methods/saveSettings.ts(1 hunks)apps/meteor/app/settings/server/functions/getSettingDefaults.ts(2 hunks)apps/meteor/client/views/admin/settings/Setting/MemoizedSetting.tsx(2 hunks)apps/meteor/client/views/admin/settings/Setting/inputs/RangeSettingInput.spec.tsx(1 hunks)apps/meteor/client/views/admin/settings/Setting/inputs/RangeSettingInput.stories.tsx(1 hunks)apps/meteor/client/views/admin/settings/Setting/inputs/RangeSettingInput.tsx(1 hunks)apps/meteor/server/settings/accounts.ts(1 hunks)packages/core-typings/src/ISetting.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/meteor/server/settings/accounts.ts
- apps/meteor/client/views/admin/settings/Setting/MemoizedSetting.tsx
- apps/meteor/app/lib/server/methods/saveSettings.ts
🧰 Additional context used
🧬 Code graph analysis (1)
apps/meteor/app/settings/server/functions/getSettingDefaults.ts (1)
packages/core-typings/src/ISetting.ts (2)
isSettingRange(170-170)ISetting(27-27)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (4)
.changeset/new-bats-pull.md (1)
1-6: LGTM!The changeset correctly documents the addition of the
RangeSettingInputcomponent and appropriately marks this as a minor version bump for the affected packages.apps/meteor/client/views/admin/settings/Setting/inputs/RangeSettingInput.spec.tsx (1)
1-19: LGTM!The test suite correctly uses Storybook stories to drive both snapshot and accessibility tests. This approach ensures consistency between the stories and tests.
apps/meteor/app/settings/server/functions/getSettingDefaults.ts (1)
40-43: LGTM!The addition of default
minValue: 0andmaxValue: 100for range settings provides sensible defaults. These values can be overridden by individual setting definitions, ensuring flexibility while maintaining safe defaults.apps/meteor/client/views/admin/settings/Setting/inputs/RangeSettingInput.tsx (1)
1-54: LGTM!The component implementation is clean and follows established patterns:
- Properly uses Fuselage components for consistent styling
- Handles optional hint display
- Delegates range validation to the underlying Slider component
- Correctly treats readonly as disabled for the slider
The use of
Number(value || 0)on Line 46 safely handles null/undefined values while preserving explicit 0 values.
dougfabris
force-pushed
the
feat/slider-setting
branch
from
9e95e47 to
1bae81a
Compare
dougfabris
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
dougfabris
removed
stat: ready to merge
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
apps/meteor/app/api/server/v1/settings.ts(2 hunks)apps/meteor/app/lib/server/lib/checkSettingValueBonds.ts(1 hunks)apps/meteor/app/lib/server/methods/saveSettings.ts(3 hunks)apps/meteor/tests/end-to-end/api/settings.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use descriptive test names that clearly communicate expected behavior
Applied to files:
apps/meteor/tests/end-to-end/api/settings.ts
🧬 Code graph analysis (4)
apps/meteor/app/lib/server/methods/saveSettings.ts (2)
packages/core-typings/src/ISetting.ts (1)
ISetting(27-27)apps/meteor/app/lib/server/lib/checkSettingValueBonds.ts (1)
checkSettingValueBounds(8-28)
apps/meteor/app/lib/server/lib/checkSettingValueBonds.ts (1)
packages/core-typings/src/ISetting.ts (1)
ISetting(27-27)
apps/meteor/tests/end-to-end/api/settings.ts (1)
apps/meteor/tests/data/api-data.ts (2)
request(10-10)credentials(39-42)
apps/meteor/app/api/server/v1/settings.ts (1)
apps/meteor/app/lib/server/lib/checkSettingValueBonds.ts (1)
checkSettingValueBounds(8-28)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (4)
apps/meteor/app/api/server/v1/settings.ts (1)
24-24: LGTM! Proper validation gate placement.The bounds validation is correctly positioned before the audit and update operations, ensuring that invalid values are rejected early without side effects.
Also applies to: 234-235
apps/meteor/tests/end-to-end/api/settings.ts (2)
131-131: Good catch on the typo fix.The correction from "succesfully" to "successfully" improves clarity.
Also applies to: 157-157
171-205: Excellent test coverage for bounds validation.The new tests thoroughly verify that:
- Values below
minValueare rejected with a 400 status and descriptive error message- Values above
maxValueare rejected with a 400 status and descriptive error messageThe error message assertions ensure the user receives clear feedback about the constraint violation.
apps/meteor/app/lib/server/methods/saveSettings.ts (1)
38-44: Well-structured validation flow.The addition of the
checkIntegerhelper and the sequential validation (type → integer → bounds) provides clear, maintainable validation logic. Therangecase is properly integrated alongside other numeric types.Also applies to: 102-105
dougfabris
force-pushed
the
feat/slider-setting
branch
from
0f60f64 to
2ae4b3c
Compare
dougfabris
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
Proposed changes (including videos or screenshots)
This PR introduces the
RangeSettingInput, so we can use ourSlidercomponent to handle settings, such as volumes. It improves the user experience allowing users to set a specific range of values, preventing undesirable valuesIssue(s)
Steps to test or reproduce
Further comments
CORE-1395
Summary by CodeRabbit