Repository navigation
Closed #119 - #123
Closed #119#123bernalalexis-try wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughNotification channel editing now uses write-only secret state, redacted channel responses, centralized form utilities, and explicit update payloads. The page supports secret replacement or removal and custom headers. The store applies returned redacted channels directly to local state. ChangesNotification secret handling
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant NotificationPage
participant NotificationChannelStore
participant NotificationService
participant API
NotificationPage->>NotificationChannelStore: Submit update payload
NotificationChannelStore->>NotificationService: updateChannel(id, data)
NotificationService->>API: PUT /notifications/channels
API-->>NotificationService: Updated channel
NotificationService-->>NotificationChannelStore: Redacted channel
NotificationChannelStore-->>NotificationPage: Update local channel
Merge Risk: 🔵 Low · up to A response missing configured-secret metadata can prevent metadata-only edits to existing channels, and assistive-technology users are not told when a secret is required. These are bounded issues that should be addressed before relying on the affected flows. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ 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: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: caf70d9c-88e7-4597-bef7-cf66440ffa86
📒 Files selected for processing (10)
app/dashboard/integrations/notifications/page.tsxcomponents/notification/SecretInput.tsxcomponents/notification/__tests__/SecretInput.test.tsxlib/notifications/__tests__/channel-form.test.tslib/notifications/channel-form.tslib/services/__tests__/notifications.test.tslib/services/notifications.tslib/types/notification.tsstores/NotificationChannelStore.tsstores/__tests__/NotificationChannelStore.test.ts
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
| <Input | ||
| id={id} | ||
| type="password" | ||
| value={state.value} | ||
| onChange={e => onChange({ ...state, value: e.target.value })} | ||
| placeholder={placeholder} | ||
| aria-describedby={describedBy} | ||
| autoComplete="new-password" | ||
| ref={inputRef} | ||
| /> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Expose the required state to assistive technology.
When required is true, the component shows only an aria-hidden asterisk. Add the native required attribute so screen readers report the constraint.
Proposed fix
<Input
id={id}
type="password"
+ required={required}
value={state.value}Based on learnings, required fields should set aria-required="true" or use the native required attribute.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <Input | |
| id={id} | |
| type="password" | |
| value={state.value} | |
| onChange={e => onChange({ ...state, value: e.target.value })} | |
| placeholder={placeholder} | |
| aria-describedby={describedBy} | |
| autoComplete="new-password" | |
| ref={inputRef} | |
| /> | |
| <Input | |
| id={id} | |
| type="password" | |
| required={required} | |
| value={state.value} | |
| onChange={e => onChange({ ...state, value: e.target.value })} | |
| placeholder={placeholder} | |
| aria-describedby={describedBy} | |
| autoComplete="new-password" | |
| ref={inputRef} | |
| /> |
Source: Learnings
| return { | ||
| ...channel, | ||
| config, | ||
| configuredSecrets: channel.configuredSecrets ?? [], |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '1,145p' lib/services/notifications.ts
sed -n '67,180p' lib/notifications/channel-form.ts
rg -n 'configuredSecrets|webhook_url|bot_token|routing_key|notifications/channels' . --glob '!node_modules' --glob '!coverage'Repository: KubeOrch/ui
Length of output: 13065
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- candidate files ---'
git ls-files | rg -i '(^|/)(notification|channel|core|api|openapi|schema|contract|integration)|README|docs' | head -200
printf '%s\n' '--- response and endpoint references ---'
rg -n -i 'configuredSecrets|notifications/channels|NotificationChannel|notification channel|rawConfig|secret.*config|config.*secret' . --glob '!node_modules' --glob '!coverage' --glob '!dist' --glob '!build' | head -300Repository: KubeOrch/ui
Length of output: 17528
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- notification type helpers ---'
sed -n '1,215p' lib/types/notification.ts
printf '%s\n' '--- form update path ---'
sed -n '180,285p' lib/notifications/channel-form.tsRepository: KubeOrch/ui
Length of output: 8155
Preserve configured status when configuredSecrets is absent.
If a channel response contains secret keys in config but omits configuredSecrets, redactChannel removes those values and replaces the status with []. buildEditFormState then marks required secrets as unconfigured, so validateChannelForm rejects metadata-only updates. Derive configured keys from the raw config, including headers.<name> entries, before redaction and merge them with channel.configuredSecrets.
|
Hey @bernalalexis-try Thanks for your contributions - could you please sign your commits? else its looks good to me |
Channel types now carry an explicit secret flag, and the service returns a redacted DTO exposing configuredSecrets rather than values, with a safety net that strips secrets should Core ever return them.
Form logic moves into channel-form.ts, and a new accessible SecretInput renders empty with a "Configured" state plus Replace and Remove actions.
Metadata-only edits no longer send secret keys.