Dev#98
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the Webhooks feature by adding a permissions field to the payload, implementing pagination support in filters and hooks, and significantly expanding the test suite for the Webhook form. It also includes general cleanup of unused imports, icons, and providers across the codebase. Review feedback identifies a regression where the danger prop was removed from ConfirmModal in task pages without being migrated to variant="danger", as seen in other files. Additionally, the retry functionality in the Webhook details page is currently non-functional due to hardcoded props, and a specific test assertion for secret visibility needs to be more explicit.
| onCancel={() => setShowDeleteModal(false)} | ||
| isLoading={deleteTask.isPending} | ||
| danger | ||
|
|
There was a problem hiding this comment.
The danger prop was removed but not replaced with variant="danger". Based on the refactoring seen in WebhookListPage.tsx, it appears the ConfirmModal component now expects a variant prop. Removing this without replacement will likely cause the delete button to lose its destructive visual styling (e.g., red color).
| variant="danger" |
| onCancel={() => setTaskToDelete(null)} | ||
| isLoading={deleteTask.isPending} | ||
| danger | ||
|
|
There was a problem hiding this comment.
Similar to the change in TaskDetailPage.tsx, the danger prop was removed without being replaced by variant="danger". This regression should be fixed to maintain visual consistency for destructive actions.
| variant="danger" |
| onRetry={() => {}} | ||
| isRetrying={false} |
There was a problem hiding this comment.
The onRetry and isRetrying props are currently hardcoded or empty. This means the retry functionality in the "Recent Deliveries" table is non-functional on this page. Consider implementing the retry logic using the useRetryWebhookDelivery hook, similar to how it is handled in WebhookLogsPage.tsx.
|
|
||
| fireEvent.click(eyeButton); | ||
| // After clicking, type should change to text | ||
| expect(secretInput.type === "text" || secretInput.type === "password").toBeTruthy(); |
There was a problem hiding this comment.
This assertion is a tautology and does not effectively test the toggle functionality. Since the input type is initialized as 'password', the test should explicitly verify that it changes to 'text' after the eye button is clicked.
| expect(secretInput.type === "text" || secretInput.type === "password").toBeTruthy(); | |
| expect(secretInput.type).toBe("text"); |
1 participant
No description provided.