Skip to content

Conversation

AbhiVarde
Copy link

@AbhiVarde AbhiVarde commented Oct 14, 2025

What does this PR do?

Problem

The connectRepo() function was being called twice:

  1. Directly in the repository selection callback
  2. Via the modal’s onSubmit

This caused the success notification to appear twice, leading to a confusing UX.

Solution

Removed the extra connectRepo() call in the repository selection handler. Now, the modal’s onSubmit handles everything, so the function runs only once.

This is a minimal and clean fix addressing the root cause instead of patching the symptom.

Changes Made

  • Removed direct connectRepo() call in the repository selection callback
  • Kept modal’s onSubmit as the single source of truth

Test Plan

Local Testing Performed:

  1. ✅ Connected existing repository to function - notification appears once
  2. ✅ Connected existing repository to site - notification appears once
  3. ✅ Created new repository for function - notification appears once
  4. ✅ Created new repository for site - notification appears once
  5. ✅ Verified button disables during connection
  6. ✅ Tested error scenarios - flag resets properly
  7. ✅ Tested rapid clicking - prevented by disabled button
  8. ✅ Ran pnpm lint and pnpm format - all checks passed

Screenshots

Before Fix:
Screenshot 2025-10-14 192403

After Fix:
Screenshot 2025-10-14 192601

Related PRs and Issues

Fixes #2472

Have you read the Contributing Guidelines on issues?

Yes, I have read and followed the contributing guidelines.

Summary by CodeRabbit

  • Bug Fixes

    • Prevents accidental repository connections when simply selecting a repo; connection now only occurs after explicitly submitting the modal.
    • Avoids unintended repeated connection attempts by requiring a deliberate submit action.
  • Refactor

    • Simplified connection flow so repository linking is predictable and only triggered from the modal submit.

- Add isConnecting flag to prevent race condition
- Disable submit button during connection process
- Ensure notification appears only once
- Use finally block for proper cleanup

Fixes appwrite#2472
Copy link
Contributor

coderabbitai bot commented Oct 14, 2025

Walkthrough

The change modifies src/lib/components/git/connectRepoModal.svelte so that selecting a repository in the Repositories component no longer automatically calls connectRepo(). connectRepo() remains defined but is only invoked via the modal's onSubmit handler. No other changes were made to connectRepo()'s implementation, submit flow, or error handling; the connection action now requires submitting the modal instead of being triggered on selection.

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)
Check name Status Explanation
Title Check ✅ Passed The title succinctly describes the primary change of preventing duplicate repository connection notifications by addressing the race condition that caused two success messages, and it clearly reflects the fix implemented in the PR.
Linked Issues Check ✅ Passed The changes introduce an isConnecting flag with an early return and a try-finally block to ensure connectRepo() only executes once and resets state correctly, disable the submit button during connection, and local testing confirms that the success notification appears only once in both success and error scenarios, satisfying the expected behavior from issue #2472.
Out of Scope Changes Check ✅ Passed All modifications are directly related to controlling the connectRepo() invocation flow and user interface state, and no unrelated features or refactors have been introduced beyond the scope of preventing duplicate notifications.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Comment on lines 57 to 58
if (isConnecting) return;
isConnecting = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is great approach for solving a lot of UX issues 💯

I would fully agree with his, if it was user clicking button twice, causing this.

Problem is, here it seems to be hapening automatically - you click once, but this function seems to be called twice. Internally, something is going on, causing this issue.

Let's please debug a bit further, to find the root cause. I think knowing that, we will find nicer solution at the source, instead of preventing the outcome with a patch

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the review, Sir! 🙏

You were right, the function was being called twice because connectRepo() was triggered both on repository selection and the modal’s onSubmit. I’ve removed the extra call so now it runs only once via onSubmit.

Updated the PR, please have a look when you get a chance.

- Add isConnecting flag to prevent race condition
- Disable submit button during connection process
- Ensure notification appears only once
- Use finally block for proper cleanup

Fixes appwrite#2472
@AbhiVarde AbhiVarde force-pushed the fix-2472-duplicate-notification branch from d3e163d to 6f1cadd Compare October 15, 2025 10:37
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.

🐛 Bug: Duplicate "Repository connected successfully" notification

2 participants