Refactor Webhooks page#303
Conversation
- Refactored the webhooks page to utilize a new layout with improved content organization. - Replaced the previous card and frame structure with a more streamlined Page component. - Introduced WebhooksPageContent for better separation of concerns and added search functionality. - Removed unused WebhooksEmpty and TableBody components to clean up the codebase. - Updated WebhooksTable to handle error states and display appropriate messages based on webhook data.
- Removed the unused `hasActiveSearch` prop from the `WebhooksTable` component. - Streamlined the logic for displaying empty states based on webhook data. - Updated the `WebhooksPageContent` component to enhance layout and button styling. - Added `.cursor/` to `.gitignore` to exclude cursor-related files from version control.
- Updated the Page component to include padding for improved spacing. - Adjusted the DefaultDashboardLayout to modify padding for better responsiveness. - Refined the WebhooksTable to conditionally apply border styles based on webhook data presence.
…ent labels - Introduced a new `WebhookExamplePayload` component to display a sample JSON payload for webhooks. - Replaced hardcoded event names with corresponding labels from `WEBHOOK_EVENT_LABELS` for better readability. - Removed unused imports and constants related to the previous payload structure. - Updated documentation link for webhook signature validation.
- Enhanced the `Input` component to support a clearable feature, allowing users to easily reset input fields. - Updated `WebhookAddEditDialogSteps` to utilize the new clearable input functionality for webhook URLs and example webhooks. - Adjusted padding in the `WebhookAddEditDialog` for improved layout consistency.
- Updated `WebhookExamplePayload` to accept an `eventType` prop for dynamic rendering of example payloads. - Simplified the display of the webhook secret description and removed the link to signature validation documentation. - Introduced state management for the last selected event to improve user experience in the webhook dialog. - Adjusted layout and styling for better clarity and usability in the webhook add/edit dialog.
# Conflicts: # .gitignore
…ling - Added a custom input ref to focus on the secret input when 'custom' secret type is selected. - Updated the button to show a check icon when the copy action is successful, improving user feedback. - Adjusted layout and styling for better responsiveness and clarity in the dialog components.
- Added clipboard support to the `WebhookAddEditDialogSteps` for copying the generated webhook secret. - Introduced state management for copy success feedback, enhancing user experience. - Updated button styles to reflect copy status, improving visual feedback during interactions.
- Updated the `WebhookSecretSchema` to maintain a minimum character requirement of 32 characters for the secret. - Adjusted the `WebhookAddEditDialogSteps` to validate the `signatureSecret` field correctly, ensuring user feedback is displayed when errors occur.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 63411f703c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| await copy(preGeneratedSecret) | ||
| setTimeout(onCopied, 150) |
There was a problem hiding this comment.
handleCopy always calls onCopied after awaiting copy, but useClipboard catches clipboard errors internally and does not rethrow. In browsers where navigator.clipboard.writeText is blocked (permission denied, insecure context, etc.), this still marks the secret as copied in parent state, which can mislead users into continuing without actually storing a secret they cannot view again after creation.
Useful? React with 👍 / 👎.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| const handleCopy = async () => { | ||
| try { | ||
| await navigator.clipboard.writeText(preGeneratedSecret) | ||
| setCopied(true) | ||
| setTimeout(() => setCopied(false), 2000) | ||
| } catch (err) { | ||
| console.error('Failed to copy:', err) | ||
| } | ||
| await copy(preGeneratedSecret) | ||
| setTimeout(onCopied, 150) | ||
| } |
There was a problem hiding this comment.
🔴 The new hasCopied parent state introduced in this refactor has three bugs that combine to make the Copy / Add button variants misleading.
(1) handleCopy in add-edit-dialog-steps.tsx:135-138 does setTimeout(onCopied, 150) without storing/clearing the id. The parent WebhookAddEditDialog stays mounted across opens, so if the user clicks Copy and closes the dialog within 150ms, handleDialogChange(false) resets hasCopied to false but the late timeout fires afterward and flips it back to true — leaking into the next dialog open. The neighbouring customSecretInputRef useEffect already shows the right cleanup pattern.
(2) useClipboard swallows errors internally (use-clipboard.ts:17-28): on insecure-context / permission-denied / unsupported-clipboard the promise still resolves, so await copy() succeeds, the local copied state stays false (no Check icon), but onCopied still fires and flips the parent variant as if the secret was captured.
(3) In add-edit-dialog.tsx:275 the Add button uses variant={hasCopied ? 'primary' : 'secondary'}, but the Custom-secret tab has no path to set hasCopied — typing a fully valid 32+ char custom secret leaves Add stuck on secondary, while the equally-valid pre-generated path gets primary.
Suggested fixes: gate the parent signal on the local copied success state (or expose success from useClipboard), call onCopied() directly without the redundant 150ms setTimeout (copied already provides the visual feedback), and treat isStep2Valid as a primary-trigger for the custom path.
Extended reasoning...
Bug 1 — setTimeout(onCopied, 150) leaks state across dialog open/close
const handleCopy = async () => {
await copy(preGeneratedSecret)
setTimeout(onCopied, 150) // id not captured, no cleanup
}hasCopied lives in the parent WebhookAddEditDialog (add-edit-dialog.tsx:52) and is only reset on close in handleDialogChange (add-edit-dialog.tsx:119). Radix unmounts DialogContent's body (where WebhookAddEditDialogSteps lives), but the parent component stays mounted because it is rendered from webhooks-page-content.tsx, not inside the portal. The pending timeout closure captures the parent's stable setHasCopied, so it still successfully mutates parent state after the steps subtree has unmounted.
Step-by-step proof:
- User reaches step 2, clicks Copy.
copy()resolves quickly (typically <10 ms),setTimeout(onCopied, 150)is scheduled. - User closes the dialog within the 150 ms window (Esc, click-outside, or the X).
handleDialogChange(false)fires, callingsetHasCopied(false). - 150 ms timer fires →
onCopied()→setHasCopied(true)on the still-mounted parent. - User reopens the dialog.
WebhookAddEditDialogStepsremounts and generates a newpreGeneratedSecretviauseMemo([]), buthasCopiedis stilltrue. The Copy button now rendersvariant='secondary'(line 321) and the Add button rendersvariant='primary'(add-edit-dialog.tsx:275) — both wrong for a fresh session, since the user has not copied this new secret.
The same file already shows the correct cleanup pattern at lines 106–112 for customSecretInputRef:
const id = window.setTimeout(() => { ... }, 0)
return () => window.clearTimeout(id)The fix is either to capture the id and clear it in a cleanup, or to drop the 150 ms setTimeout entirely — the local copied state from useClipboard already provides the same 150 ms visual feedback, so calling onCopied() directly is sufficient.
Bug 2 — onCopied fires even when the clipboard write fails
// use-clipboard.ts:17-28
try {
await navigator.clipboard.writeText(text)
setWasCopied(true)
setTimeout(() => setWasCopied(false), duration)
} catch (err) {
console.error('Failed to copy text:', err)
setWasCopied(false)
}The hook catches the error, logs it, sets wasCopied = false, and never rethrows. The returned promise always resolves successfully. Therefore await copy(preGeneratedSecret) in handleCopy returns control regardless of outcome, and setTimeout(onCopied, 150) is scheduled unconditionally.
Step-by-step proof (insecure context / permission denied):
- User clicks Copy.
navigator.clipboard.writeTextthrows (e.g. served over plain HTTP, iframe without permission, sandboxed browser). useClipboardcatches, logs, setswasCopied = false.copy()resolves.handleCopyproceeds to schedulesetTimeout(onCopied, 150).- 150 ms later,
hasCopiedflips to true on the parent. - UI: the Check icon never appears (gated on local
copied, lines 326–328), but the Copy button flips from primary → secondary and the Add button flips from secondary → primary. The button-variant cue lies — it tells the user they captured a secret they actually never copied, and the warning text says "You won't be able to view it again."
The pre-PR implementation handled this correctly with a try/catch around navigator.clipboard.writeText that only set the success state inside the try. Either expose a success boolean from useClipboard, or gate onCopied on the copied state (e.g. useEffect(() => { if (copied) onCopied() }, [copied])), or revert to a direct try/catch in handleCopy.
Bug 3 — Add button stuck on 'secondary' in the Custom-secret flow
// add-edit-dialog.tsx:275
<Button
type="submit"
variant={hasCopied ? 'primary' : 'secondary'}
...
>Grepping the codebase confirms onCopied is wired only to setHasCopied(true) (add-edit-dialog.tsx:195) and is invoked exclusively from handleCopy on the pre-generated tab. The Custom tab has no Copy button, no handleCopy invocation, and no other path that flips hasCopied. So a user who completes the Custom flow with a valid 32+ char secret has isStep2Valid === true (the form will submit) but hasCopied === false, leaving Add visually muted on one of two equally valid completion paths.
Step-by-step proof:
- Open dialog, advance to step 2.
- Click the Custom tab. Type a 40-character secret.
isStep2Validis true (button is enabled), but the variant resolves tosecondary.- Switch to Pre-generated, click Copy. After ~150 ms the variant flips to
primaryfor the same form-validity state.
This is a real visual asymmetry between the two valid paths, not a purely subjective preference.
Addressing the refutation on Bug 3
One verifier argued the asymmetry is intentional — the pre-generated path is special because the secret "won't be shown again," so emphasizing Add after Copy reinforces a critical action, whereas in the Custom path the user owns the secret already.
That framing has merit for the pre-generated branch, but it doesn't justify leaving Custom muted: in both flows the gating concept is "the user has finished step 2." Today the cue lies in two directions — Bug 2 promotes Add to primary on a clipboard failure (no real copy), and Bug 3 keeps Add on secondary for a fully valid custom submission. Both inversions point to the same root cause: hasCopied is not the right primary-trigger predicate. Gating on isStep2Valid (or hasCopied || (secretType === 'custom' && isStep2Valid)) keeps the intent and removes the asymmetry. The fix is one line, so the cost of fixing is well below the cost of two paths disagreeing on the visual cue.
Severity
I am filing this as normal: the merged bug spans both a state-leak (cosmetic, narrow window) and a clipboard-failure path that actively misleads the user about whether the secret was captured (combined with the warning text "You won't be able to view it again"). The clipboard-failure case is rare on production HTTPS but real for previews / iframes / locked-down browsers, and the symptom is exactly the wrong direction (false positive). Worth fixing before merge while the file is open.
1 participant
Ticket