Skip to content

Conversation

HarshMN2345
Copy link
Member

What does this PR do?

This PR ensures the domain verification status label in the UI updates immediately after a successful verification retry.
It dispatches a custom event from the retry modal and triggers a data refresh in the parent page, keeping the UI and backend state in sync without requiring a manual page reload.

Test Plan

image

Related PRs and Issues

(If this PR is related to any other PR or resolves any issue or related to any issue link all related PR and issues here.)

Have you read the Contributing Guidelines on issues?

yes

@HarshMN2345 HarshMN2345 requested a review from Copilot July 25, 2025 06:15
Copilot

This comment was marked as outdated.

@HarshMN2345 HarshMN2345 requested a review from Copilot July 25, 2025 06:20
Copilot

This comment was marked as outdated.

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes the issue where domain verification status wasn't updating in the UI after a successful retry. The fix ensures that when domain verification succeeds through the retry modal, the parent page immediately refreshes its data to reflect the updated verification status.

Key Changes

  • Added event dispatcher to the retry modal component to emit a 'verified' event upon successful verification
  • Implemented data reload functionality in the parent page that listens for the verification event
  • Used SvelteKit's invalidateAll() to refresh the domain data without requiring a manual page reload

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
retryDomainModal.svelte Added event dispatcher to emit 'verified' event when domain verification succeeds
+page.svelte Added event handler and data reload function to refresh domain status after successful verification
Comments suppressed due to low confidence (1)

src/routes/(console)/organization-[organization]/domains/domain-[domain]/+page.svelte:39

  • [nitpick] The function name 'reloadDomainData' could be more specific. Consider renaming it to 'handleDomainVerified' or 'onDomainVerified' to better reflect that it's an event handler for domain verification.
    async function reloadDomainData() {

ItzNotABug
ItzNotABug previously approved these changes Jul 25, 2025
@@ -35,6 +36,10 @@
let showImportModal = false;
let selectedPreset = '';

async function reloadDomainData() {
await invalidateAll();
Copy link
Member

Choose a reason for hiding this comment

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

whoa no. this is a very heavy and expensive operation.

Comment on lines +41 to +42
await Promise.all([invalidate(Dependencies.DOMAIN), invalidate(Dependencies.DOMAINS)]);
}
Copy link
Member

Choose a reason for hiding this comment

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

isn't this already done on the RetryDomainModal? What difference does this make moving up a layer via callback?

Copy link
Member Author

Choose a reason for hiding this comment

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

The modal does refresh data, but the parent page isn’t aware of it. Sending an event ensures the parent updates its UI instantly after verification

Copy link
Member

Choose a reason for hiding this comment

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

Invalidating a dependency invalidates the data fetch from page.ts and then to page.svelte.
Can you instead verify and see if the domain verification variable is updated correctly?

@ItzNotABug ItzNotABug dismissed their stale review July 25, 2025 07:53

Approved by mistake!

@Meldiron
Copy link
Contributor

Meldiron commented Aug 6, 2025

Stale PR, Darshan fixed in anoher one, already merged. Closing

@Meldiron Meldiron closed this Aug 6, 2025
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.

3 participants