Skip to content

fix: breadcrumb issue#3111

Merged
wangdan-fit2cloud merged 1 commit intomainfrom
pr@main/breadcrumb-issue
May 19, 2025
Merged

fix: breadcrumb issue#3111
wangdan-fit2cloud merged 1 commit intomainfrom
pr@main/breadcrumb-issue

Conversation

@wangdan-fit2cloud
Copy link
Contributor

What this PR does / why we need it?

Summary of your change

Please indicate you've done the following:

  • Made sure tests are passing and test coverage is added if needed.
  • Made sure commit message follow the rule of Conventional Commits specification.
  • Considered the docs impact and opened a new docs issue or PR with docs changes if needed.

@wangdan-fit2cloud wangdan-fit2cloud merged commit a30316d into main May 19, 2025
4 checks passed
@wangdan-fit2cloud wangdan-fit2cloud deleted the pr@main/breadcrumb-issue branch May 19, 2025 10:18
}
}
onMounted(() => {
if (!breadcrumbData.value) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The refresh() function has a logic issue. It checks whether isDataset.value is true, then calls getDataset(). If not, it checks isApplication.value, which should be checked only if isDataset.value was false to avoid calling both functions.

For better readability and maintainability:

  1. Ensure that all conditional blocks end with an indentation change.
  2. Add comments explaining the conditional logic.

Here's a suggested revision:

function refresh() {
  common.saveBreadcrumb(null);

  if (isDataset.value) {
    // Update dataset logic here if needed
    getDataset();    
  } else if (isApplication.value && !isDataset.value) {
    // Update application logic here if needed
    getApplication();
  } else {
    console.error("Unexpected value of isDataset and isApplication");
  }
}

Additionally, consider adding error handling when making network requests in your getApplication() or getDataset() calls. This will help manage edge cases more gracefully during development and production environments.

emit('refresh')
if (isWorkFlow(applicationForm.value.type)) {
router.push({ path: `/application/${res.data.id}/workflow` })
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

There are no apparent errors in the provided code snippet regarding logic or syntax. However, there is a suggestion for an additional optimization:

Optimization Suggestion:
Before calling emit('refresh'), you might consider checking if there's an instance of a component that can handle this refresh event to avoid unnecessary emissions.

Here’s how the updated function could look with this consideration added:

const submitHandler = async (formEl: FormInstance | undefined) => {
  if(formEl?.validateForm()) { // Ensure form validation passes before proceeding
    applicationApi.postApplication(applicationForm.value, loading).then((res) => {
      MsgSuccess(t('common.createSuccess'));
      
      if(isWorkFlow(applicationForm.value.type)) {
        router.push({ path: `/application/${res.data.id}/workflow` });
      } else {
        
        // Check if there's a specific component that needs refreshing
        const componentsToRefresh = [/* List of component IDs here */];
        const foundComponent = document.getElementById(componentsToRefresh.find(componentId => 
          !!document.querySelector(`#${componentId}`)
        )) as HTMLElement;

        if(foundComponent || window.location.reload) {
          
          emit('refresh');
          
          if(window.location.reload && !foundComponent)
            window.location.reload();
        }

      }
    }).catch(errorHandling); // Handle any other error scenario(s)
  }
};

This enhancement ensures that only when necessary updates occur, leading to potentially improved performance in scenarios where multiple parts of the UI need to be refreshed due to changes related to workflow creation.

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