fix(integrations): Ensure retryCount is sent as a number#37356
fix(integrations): Ensure retryCount is sent as a number#37356kodiakhq[bot] merged 6 commits intoRocketChat:developfrom
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: efd9327 The changes in this PR will be included in the next version bump. This PR includes changesets to release 40 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 |
|
|
WalkthroughReplaces the retryCount input in the outgoing webhook admin form from a text input to a NumberInput component (import added), rendered with min="0" and step="1", passing the form field through to NumberInput so retryCount is constrained as a numeric value. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Form as OutgoingWebhookForm
participant NumberInput as NumberInput (retryCount)
participant State as Form State
participant API
User->>Form: edits retryCount
Form->>NumberInput: receives input event
Note right of NumberInput `#f0f9ff`: enforces numeric input\n(min=0, step=1)
NumberInput->>State: updates field value (numeric)
State-->Form: retryCount numeric in form state
Form->>API: submit integration (retryCount numeric or omitted)
API-->>Form: response
Form-->>User: success / error
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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]](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 (1)
apps/meteor/client/views/admin/integrations/outgoing/OutgoingWebhookForm.tsx (1)
476-480: Remove commented-out code before merging.The commented-out implementation should be removed to keep the codebase clean. Version control history preserves the old implementation if needed for reference.
Apply this diff:
- {/* <Controller - name='retryCount' - control={control} - render={({ field }) => <TextInput type='number' id={retryCountField} {...field} aria-describedby={`${retryCountField}-hint`} />} - /> */} <Controller
📜 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 (1)
apps/meteor/client/views/admin/integrations/outgoing/OutgoingWebhookForm.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/meteor/client/views/admin/integrations/outgoing/OutgoingWebhookForm.tsx (1)
packages/livechat/src/components/Form/TextInput/index.tsx (1)
TextInput(38-38)
🔇 Additional comments (1)
apps/meteor/client/views/admin/integrations/outgoing/OutgoingWebhookForm.tsx (1)
484-502: Good fix; API is compatible with undefined values.The implementation correctly uses
valueAsNumberto ensure numeric conversion and properly handles empty inputs by converting NaN to undefined. The API schema inpackages/rest-typingsdefinesretryCountas optional, so sending undefined (or omitting it from the payload) is fully compatible.The logic is sound: empty inputs become undefined, React Hook Form omits undefined fields from the submission, and the API accepts the optional field.
Optional: Consider more concise inline comments.
The inline comments (lines 489, 491-492, 494-495, 498) are helpful but verbose. Suggested shorter versions:
- Line 489: "Convert to number; empty input becomes undefined"
- Line 499: "Display blank when undefined"
MrKalyanKing
force-pushed
the
fix/37346-webhook-retry-number
branch
from
1640ce5 to
bdfd51c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/meteor/client/views/admin/integrations/outgoing/OutgoingWebhookForm.tsx (1)
481-503: The numeric conversion logic is correct.The implementation properly addresses the bug by using
valueAsNumberto ensure the retry count is submitted as a number (orundefinedwhen empty), which aligns with API expectations.Consider these optional refinements:
- Add input constraints since retry count should be a non-negative integer:
<TextInput {...field} id={retryCountField} type='number' + step='1' + min='0' onChange={(e) => {
- Simplify comments - the inline comments are helpful but could be more concise:
- // This 'onChange' is the new magic onChange={(e) => { - // e.currentTarget.valueAsNumber gives us a real number (or NaN) const value = e.currentTarget.valueAsNumber; - // If the field is empty (value=NaN), send 'undefined' - // Otherwise, send the number field.onChange(isNaN(value) ? undefined : value); }} - // This 'value' prop ensures the input is blank when the value is undefined value={field.value ?? ''}
📜 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 (2)
.changeset/good-singers-kiss.md(1 hunks)apps/meteor/client/views/admin/integrations/outgoing/OutgoingWebhookForm.tsx(1 hunks)
🔇 Additional comments (1)
.changeset/good-singers-kiss.md (1)
2-2: Verify that this bug fix warrants a major version bump.Bug fixes typically warrant a patch release rather than a major version bump. Major versions are conventionally reserved for breaking changes.
If this is part of a larger planned major release, consider noting that context. Otherwise, consider changing to:
-'@rocket.chat/meteor': major +'@rocket.chat/meteor': patch
apps/meteor/client/views/admin/integrations/outgoing/OutgoingWebhookForm.tsx
Outdated
Show resolved
Hide resolved
MrKalyanKing
changed the title
MrKalyanKing
force-pushed
the
fix/37346-webhook-retry-number
branch
from
bdfd51c to
cf7a431
Compare
apps/meteor/client/views/admin/integrations/outgoing/OutgoingWebhookForm.tsx
Outdated
Show resolved
Hide resolved
apps/meteor/client/views/admin/integrations/outgoing/OutgoingWebhookForm.tsx
Outdated
Show resolved
Hide resolved
apps/meteor/client/views/admin/integrations/outgoing/OutgoingWebhookForm.tsx
Outdated
Show resolved
Hide resolved
apps/meteor/client/views/admin/integrations/outgoing/OutgoingWebhookForm.tsx
Outdated
Show resolved
Hide resolved
|
Hi @MartinSchoeler, thank you for the detailed feedback! This was very helpful. I've made all the changes you requested: Switched from TextInput to NumberInput. Removed the custom onChange and value props to use the RHF defaults correctly. Removed the extra comments. Updated the changeset file description. I've pushed the amended commit. This should be ready for another look. Thanks! |
MartinSchoeler
added
the
stat: QA assured
scuciatto
removed
the
stat: QA assured
MrKalyanKing
force-pushed
the
fix/37346-webhook-retry-number
branch
from
ad2d98a to
9f4ff15
Compare
|
Hi @MartinSchoeler, I have synced my branch with the latest develop branch to fix the outdated status. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #37356 +/- ##
===========================================
- Coverage 70.85% 70.85% -0.01%
===========================================
Files 3161 3161
Lines 109785 109785
Branches 19700 19671 -29
===========================================
- Hits 77790 77784 -6
- Misses 29973 29979 +6
Partials 2022 2022
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
MartinSchoeler
added
community
stat: QA assured
dougfabris
changed the title
dougfabris
added
contrib: valid
6 participants
Proposed changes
This PR fixes a bug where the "Retry Count" field on the Outgoing Webhook form was sending its value as a string instead of a number. This caused the API to reject the save/update with a
[invalid-params]error.I have updated the
outgoingwebhookform.tsxcomponent. TheTextInputforretryCountnow uses anonChangehandler to capture the event and set the form's state usinge.currentTarget.valueAsNumber. This ensures the value is always a number (orundefinedif the field is empty), which the API expects.I was able to reproduce the bug and confirm the fix on my local dev server, which now saves the integration successfully.
Issue(s)
Closes #373
Steps to test or reproduce
Administration>Integrations.Newand selectOutgoing.6to5).Save.Issue(s)
Closes #37346
Related to #37356
Further comments
This is my first contribution. I'm happy to make any changes if needed!
COMM-77
Summary by CodeRabbit