Skip to content
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

Refresh connection data on add/delete and improve loading spinner states. #252

Merged
merged 4 commits into from
Feb 6, 2025

Conversation

Rodri77
Copy link
Collaborator

@Rodri77 Rodri77 commented Feb 6, 2025

OINT-1076
OINT-1084
OINT-1085
OINT-1080

Important

Refresh connection data on add/delete events and update loading spinners across components for improved UX.

  • Behavior:
    • Refresh connection data on success and error events in AddConnectionTabContent.
    • Update onSuccessCallback in ConnectionPortal to refetch connections after navigation.
  • Loading Spinner:
    • Replace Loader with Loader2 in ConnectionPortal, IntegrationSearch, ConnectionCard, ConnectionDetails, and IntegrationCard for a consistent loading animation.
    • Add loading overlay in ConnectionDetails and ConnectionCard when deleting or processing.
  • Misc:
    • Comment out destination_id in ConnectorConfigForm schema definition.

This description was created by Ellipsis for 59ddca0. It will automatically update as commits are pushed.

@Rodri77 Rodri77 requested a review from pellicceama February 6, 2025 00:22
Copy link

vercel bot commented Feb 6, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
openint ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 6, 2025 0:26am

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ Changes requested. Reviewed everything up to 59ddca0 in 1 minute and 15 seconds

More details
  • Looked at 224 lines of code in 8 files
  • Skipped 0 files when reviewing.
  • Skipped posting 11 drafted comments based on config settings.
1. packages/engine-frontend/components/ConnectionPortal.tsx:127
  • Draft comment:
    Removing deleteConnection.isLoading from isLoading may lead to unintended UX if deletion is considered a loading state. Verify intended behavior.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 40% <= threshold 50%
    The comment is asking the PR author to verify the intended behavior, which violates the rule against asking for confirmation of intention. However, it does point out a potential issue with UX that could be useful for the author to consider. The comment could be rephrased to suggest a specific change or to highlight the potential issue without asking for verification.
2. packages/engine-frontend/components/ConnectionPortal.tsx:159
  • Draft comment:
    Ensure async onSuccessCallback handles errors from refetch if necessary.
  • Reason this comment was not posted:
    Confidence changes required: 50% <= threshold 50%
    None
3. packages/ui/domain-components/ConnectionDetails.tsx:91
  • Draft comment:
    Good enhancement: overlay spinner added for delete state and disable button.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
4. packages/ui/domain-components/IntegrationCard.tsx:27
  • Draft comment:
    Auto-connect behavior using debounce looks fine; verify that forced auto connection is desired.
  • Reason this comment was not posted:
    Confidence changes required: 30% <= threshold 50%
    None
5. apps/web/app/dashboard/(authenticated)/connector-configs/ConnectorConfigForm.tsx:80
  • Draft comment:
    Why is 'destination_id' commented out? Consider adding a TODO or explanation if it's temporary.
  • Reason this comment was not posted:
    Marked as duplicate.
6. packages/engine-frontend/components/AddConnectionTabContent.tsx:30
  • Draft comment:
    Refetch on error event is added. Confirm if additional user feedback (e.g. error toast) is needed.
  • Reason this comment was not posted:
    Confidence changes required: 50% <= threshold 50%
    None
7. packages/engine-frontend/components/ConnectionPortal.tsx:127
  • Draft comment:
    Main loading state now excludes isRefetching and deleteConnection.isLoading. Verify this change reflects the intended UX for spinners.
  • Reason this comment was not posted:
    Marked as duplicate.
8. packages/engine-frontend/components/ConnectionPortal.tsx:159
  • Draft comment:
    The onSuccessCallback is now async and awaits refetch. Ensure that awaiting the refetch fits the desired flow and consider error handling if needed.
  • 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%
    The comment is asking the author to "ensure" something and "consider" error handling - this violates our rules about not asking authors to verify things. The change to make the callback async and await the refetch is likely intentional to ensure data is fresh when showing the manage tab. Error handling already exists in the TRPC query setup.
    Maybe waiting for the refetch could cause the UI to feel slower or less responsive? Maybe there's a race condition I'm missing?
    The change appears intentional to ensure data consistency. Any errors would be handled by the existing TRPC error handling. The brief delay is acceptable for data consistency.
    Delete this comment as it's asking for verification of intentional behavior and suggesting speculative improvements without clear evidence of issues.
9. packages/engine-frontend/components/ConnectionsTabContent.tsx:22
  • Draft comment:
    isDeleting prop is now passed to ConnectionDetails. Confirm all dependent components use this flag consistently.
  • Reason this comment was not posted:
    Confidence changes required: 50% <= threshold 50%
    None
10. packages/ui/domain-components/ConnectionDetails.tsx:32
  • Draft comment:
    Overlay spinner and disabled delete button during deletion improve UX. Verify if the overlay covers all interactive elements.
  • Reason this comment was not posted:
    Confidence changes required: 30% <= threshold 50%
    None
11. packages/ui/domain-components/IntegrationCard.tsx:1
  • Draft comment:
    Consistent use of Loader2 throughout components is good for uniformity.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None

Workflow ID: wflow_1vDTHPgmmvUF9Kxx


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link

recurseml bot commented Feb 6, 2025

✨ No issues found! Your code is sparkling clean! ✨

@pellicceama pellicceama merged commit 8f87438 into main Feb 6, 2025
5 checks passed
@pellicceama pellicceama deleted the connect-enhancements branch February 6, 2025 03:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants