fix: add skill show error if manifest url can not be accessed

[zhixzhan]

Description

fix skill-add flow:

1. check manifest url before save.
2. check name should not contain special characters.

Task Item

close #2920

Screenshots

[github-actions]

Coverage remained the same at 0.0% when pulling 0d0795c on zhixzhan/skill-fix into 398dbdf on master.

[cwhitten]

@zhixzhan instead of an alert modal can we render something a bit more subtle, like a text notification in the form itself?

[zhixzhan]

@cwhitten updated. the url check result would be sent from server and display as a notification (with some lag)

[a-b-r-o-w-n]

I think we should separate the async validation from the sync validation and consolidate the skill validation logic into a utility used by both the skill form and skill modal. I'm also curious if the modal and form can share common components.

For the url validation, here is what I suggest:

useEffect(() => {
  httpClient('/projects/:projectId/skill/check', { url })
    .then(/* maybe clear error */)
    .catch(/* set validation error */)
}, [data.manifestUrl]);

Action creators should only be used to dispatch updates to the store (maybe as a result of an api call). They should not be used directly in components either to return a value or for a try/catch. It violates the uni-directional data flow model of the reducer pattern.

[zhixzhan]

I think we should separate the async validation from the sync validation and consolidate the skill validation logic into a utility used by both the skill form and skill modal. I'm also curious if the modal and form can share common components.

For the url validation, here is what I suggest:

useEffect(() => {
  httpClient('/projects/:projectId/skill/check', { url })
    .then(/* maybe clear error */)
    .catch(/* set validation error */)
}, [data.manifestUrl]);

Action creators should only be used to dispatch updates to the store (maybe as a result of an api call). They should not be used directly in components either to return a value or for a try/catch. It violates the uni-directional data flow model of the reducer pattern.

@a-b-r-o-w-n I moved two form into a component, and separate remote call to an async validation. but when I run jest ,meet a warning about useEffect, did you meet it before?

[a-b-r-o-w-n]

@zhixzhan that error is probably due to the api call resolving after the component has unmounted. This may only be a problem with the test, but something to consider is adding some cancellation functionality so that the state update is not triggered.

useEffect(() => {
  let isCancelled = false;
  asyncValidation(formData).then((errors) => {
    setFormErrors(errors);
  });

  return () => {
    isCancelled = true;
  }
}, [formData.manifestUrl]);

I'm not sure if that would work.

[zhixzhan]

@zhixzhan that error is probably due to the api call resolving after the component has unmounted. This may only be a problem with the test, but something to consider is adding some cancellation functionality so that the state update is not triggered.

useEffect(() => {
  let isCancelled = false;
  asyncValidation(formData).then((errors) => {
    setFormErrors(errors);
  });

  return () => {
    isCancelled = true;
  }
}, [formData.manifestUrl]);

I'm not sure if that would work.

I tried but no difference, ( anyway, it's only a test warning, wouldn't effect the feature. )

[a-b-r-o-w-n]

Sorry @zhixzhan my code snippet was wrong.

useEffect(() => {
  let isCancelled = false;
  asyncValidation(formData).then((errors) => {
    if (!isCancelled) {
      setFormErrors(errors);
    }
  });

  return () => {
    isCancelled = true;
  }
}, [formData.manifestUrl]);

[a-b-r-o-w-n]

@zhixzhan I pulled this and tried to fix the jest warning but didn't make any progress.