-
Notifications
You must be signed in to change notification settings - Fork 689
feat(frontend) : triggers panel polishing #5825
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
Looks good to me! 👍
Reviewed everything up to febd6ec in 2 minutes and 13 seconds. Click for details.
- Reviewed
894
lines of code in23
files - Skipped
0
files when reviewing. - Skipped posting
17
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. frontend/src/lib/components/ScheduleEditorInner.svelte:16
- Draft comment:
Refactor alert: Instead of passing separate booleans (e.g. isDraftOnly), use a unified trigger object. Ensure the Trigger type includes all required properties. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. frontend/src/lib/components/triggers/TriggerEditorToolbar.svelte:37
- Draft comment:
Use of new 'trigger' prop: Verify the trigger object contains necessary flags (e.g. isDraft) for conditional rendering of buttons. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the PR author to verify the contents of thetrigger
object, which is against the rules as it asks for confirmation or verification. It does not provide a specific suggestion or point out a specific issue.
3. frontend/src/lib/components/triggers/TriggersTable.svelte:90
- Draft comment:
Confirm that DeleteTriggerButton now correctly uses the trigger prop; review triggerIconMap and TriggerLabel mappings for the unified trigger object. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. frontend/src/lib/components/triggers/TriggersWrapper.svelte:82
- Draft comment:
Ensure that instances where legacy boolean props were used now rely on the unified trigger object, with proper null checks if undefined. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. frontend/src/lib/components/triggers/gcp/GcpTriggerEditorInner.svelte:59
- Draft comment:
Review changes in GCP trigger editor: Confirm that openEdit and openNew correctly extract properties from the trigger object. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the PR author to confirm that certain functions correctly extract properties. This falls under the rule of not asking the author to confirm their intention or to ensure behavior is intended. Therefore, this comment should be removed.
6. frontend/src/lib/components/triggers/http/RouteEditorInner.svelte:27
- Draft comment:
Check data flow: Verify that defaultValues and trigger.draftConfig are handled consistently in openEdit/openNew using the new trigger prop. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
7. frontend/src/lib/components/triggers/kafka/KafkaTriggerEditorInner.svelte:22
- Draft comment:
Consolidation of props: Ensure Kafka trigger editor correctly extracts flags like isDraft and draftConfig from the unified trigger. Check type safety and fallbacks. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
8. frontend/src/lib/components/triggers/mqtt/MqttTriggerEditorInner.svelte:22
- Draft comment:
Unified trigger prop usage: Verify that the new pattern is propagated consistently and that loadTrigger/save functions handle errors correctly. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the PR author to verify that a new pattern is propagated consistently and that certain functions handle errors correctly. This falls under asking the author to double-check things and ensure behavior, which violates the rules.
9. frontend/src/lib/components/triggers/nats/NatsTriggerEditorInner.svelte:22
- Draft comment:
Ensure Nats trigger configuration loads and saves correctly using the unified trigger object; properly use flags like draftConfig. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
10. frontend/src/lib/components/triggers/postgres/PostgresTriggerEditorInner.svelte:52
- Draft comment:
Verify that passing the unified 'trigger' prop doesn’t break error recovery logic; ensure relation validation and required fields are computed using the trigger object state. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
11. frontend/src/lib/components/triggers/schedules/ScheduleEditorInner.svelte:50
- Draft comment:
Review schedule changes: Ensure schedule-specific properties are consistently computed from the trigger or defaultValues, maintaining legacy behavior. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is asking the author to ensure that schedule-specific properties are consistently computed, which falls under the category of asking the author to ensure behavior is intended or tested. This violates the rules.
12. frontend/src/lib/components/triggers/sqs/SqsTriggerEditorInner.svelte:20
- Draft comment:
SQS Trigger Editor: Verify that properties like draftConfig are properly used in openEdit/openNew and that getSaveCfg() reflects the trigger’s data. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is asking the PR author to verify the usage of certain properties and ensure that a function reflects the trigger's data. It is not making a specific suggestion or pointing out a specific issue, but rather asking for confirmation and verification, which violates the rules.
13. frontend/src/lib/components/triggers/sqs/SqsTriggerPanel.svelte:9
- Draft comment:
SQS Trigger Panel updates: Confirm that the panel now passes the 'trigger' prop to the editor, ensuring correct draft/deployed status is displayed. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
14. frontend/src/lib/components/triggers/websocket/WebsocketTriggerEditorInner.svelte:29
- Draft comment:
WebSocket trigger editor: Ensure the new trigger prop is correctly destructured and that openEdit/openNew functions properly use trigger.draftConfig and trigger.path. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the author to ensure that the new trigger prop is correctly destructured and that certain functions use specific properties. This is asking the author to double-check their work, which violates the rules.
15. frontend/src/lib/components/triggers/websocket/WebsocketTriggersPanel.svelte:8
- Draft comment:
WebSocket Triggers Panel: Verify that selectedTrigger (passed as trigger prop) is defined and its properties (isDraft, draftConfig) are available for conditional rendering. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
16. frontend/src/lib/components/triggers/websocket/WebsocketTriggerEditorInner.svelte:52
- Draft comment:
Double-check that derived state (e.g. isValid, emptyString checks) now properly use trigger properties instead of separate booleans. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
17. frontend/src/lib/components/triggers/DeleteTriggerButton.svelte:21
- Draft comment:
Typographical note: In the confirmation modal title, there is an unnecessary space before the question mark. Consider changing "Are you sure you want to delete this draft trigger ?" to "Are you sure you want to delete this draft trigger?". - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While technically correct, this is an extremely minor UI text issue. The rules state not to comment on pure UI changes and to only keep comments that require clear code changes. This feels more like a nitpick than a substantial issue that needs addressing. The spacing issue could affect user experience and professionalism of the UI. Some might argue that maintaining consistent, correct punctuation is important for product quality. While correct punctuation is good, this is too minor of an issue to warrant a code review comment. It doesn't affect functionality and falls under UI polish that we're told to ignore. Delete this comment as it's too minor and relates to UI text formatting, which we're instructed to ignore.
Workflow ID: wflow_ZvVFYqQrOGzkQuAH
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Important
Refactor trigger components to consolidate properties into a single
trigger
object, addDeleteTriggerButton
, and improve UI/UX.hasDraft
,isDraftOnly
, andprimary
into a singletrigger
object inSchedulePanel.svelte
,GcpTriggerPanel.svelte
,RoutesPanel.svelte
, and other trigger-related components.TriggerEditorToolbar.svelte
to usetrigger
object for draft and primary checks.DeleteTriggerButton.svelte
for handling trigger deletion with confirmation modal.DeleteTriggerButton
inTriggersTable.svelte
,TriggerEditorToolbar.svelte
, and other relevant components.CaptureSection.svelte
to clarify event loss risk.customLabel
condition inTriggersWrapper.svelte
and related components.This description was created by
for febd6ec. You can customize this summary. It will automatically update as commits are pushed.