Skip to content

fix: issue and module link validation #5994

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.

Sign up for GitHub

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

Merged
pushya22 merged 2 commits into from
Nov 13, 2024
Merged

fix: issue and module link validation #5994

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f61aa3a
fix: issue and module link validation
aaryan610 Nov 13, 2024
e641d25
chore: removed reset logic
aaryan610 Nov 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions web/core/components/issues/issue-detail-widgets/links/helper.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ export const useLinkOperations = (workspaceSlug: string, projectId: string, issu
type: TOAST_TYPE.ERROR,
title: "Link not created",
});
throw error;
}
},
update: async (linkId: string, data: Partial<TIssueLink>) => {
Expand All @@ -44,6 +45,7 @@ export const useLinkOperations = (workspaceSlug: string, projectId: string, issu
type: TOAST_TYPE.ERROR,
title: "Link not updated",
});
throw error;
}
},
remove: async (linkId: string) => {
Expand Down
15 changes: 8 additions & 7 deletions web/core/components/issues/issue-detail/links/create-update-link-modal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,6 @@ import { Controller, useForm } from "react-hook-form";
import type { TIssueLinkEditableFields } from "@plane/types";
// plane ui
import { Button, Input, ModalCore } from "@plane/ui";
// helpers
import { checkURLValidity } from "@/helpers/string.helper";
// hooks
import { useIssueDetail } from "@/hooks/store";
// types
Expand Down Expand Up @@ -48,14 +46,18 @@ export const IssueLinkCreateUpdateModal: FC<TIssueLinkCreateEditModal> = observe

const onClose = () => {
setIssueLinkData(null);
reset();
if (handleOnClose) handleOnClose();
};

const handleFormSubmit = async (formData: TIssueLinkCreateFormFieldOptions) => {
if (!formData || !formData.id) await linkOperations.create({ title: formData.title, url: formData.url });
else await linkOperations.update(formData.id as string, { title: formData.title, url: formData.url });
onClose();
const parsedUrl = formData.url.startsWith("http") ? formData.url : `http://${formData.url}`;
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 13, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance URL protocol validation logic

The current URL processing might miss some edge cases. Consider using the URL constructor for robust URL validation and protocol handling.

-const parsedUrl = formData.url.startsWith("http") ? formData.url : `http://${formData.url}`;
+const getValidUrl = (url: string) => {
+  try {
+    const urlObject = new URL(url);
+    return urlObject.toString();
+  } catch {
+    // If URL is invalid or missing protocol, prepend https:// (preferred over http://)
+    return `https://${url}`;
+  }
+};
+const parsedUrl = getValidUrl(formData.url);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const parsedUrl = formData.url.startsWith("http") ? formData.url : `http://${formData.url}`;
const getValidUrl = (url: string) => {
try {
const urlObject = new URL(url);
return urlObject.toString();
} catch {
// If URL is invalid or missing protocol, prepend https:// (preferred over http://)
return `https://${url}`;
}
};
const parsedUrl = getValidUrl(formData.url);

try {
if (!formData || !formData.id) await linkOperations.create({ title: formData.title, url: parsedUrl });
else await linkOperations.update(formData.id, { title: formData.title, url: parsedUrl });
onClose();
} catch (error) {
console.error("error", error);
}
Comment on lines +54 to +60
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 13, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Improve error handling and user feedback

The current implementation silently fails by only logging to console. Users should be notified when operations fail.

 try {
   if (!formData || !formData.id) await linkOperations.create({ title: formData.title, url: parsedUrl });
   else await linkOperations.update(formData.id, { title: formData.title, url: parsedUrl });
   onClose();
 } catch (error) {
-  console.error("error", error);
+  console.error("Failed to save link:", error);
+  // Assuming you have a toast notification system
+  toast.error(
+    error instanceof Error 
+      ? error.message 
+      : "Failed to save link. Please try again."
+  );
 }

Committable suggestion skipped: line range outside the PR's diff.

};

useEffect(() => {
Expand All @@ -77,7 +79,6 @@ export const IssueLinkCreateUpdateModal: FC<TIssueLinkCreateEditModal> = observe
name="url"
rules={{
required: "URL is required",
validate: (value) => checkURLValidity(value) || "URL is invalid",
}}
render={({ field: { value, onChange, ref } }) => (
<Input
Expand Down
2 changes: 2 additions & 0 deletions web/core/components/issues/issue-detail/links/root.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ export const IssueLinkRoot: FC<TIssueLinkRoot> = (props) => {
type: TOAST_TYPE.ERROR,
title: "Link not created",
});
throw error;
}
},
update: async (linkId: string, data: Partial<TIssueLink>) => {
Expand All @@ -76,6 +77,7 @@ export const IssueLinkRoot: FC<TIssueLinkRoot> = (props) => {
type: TOAST_TYPE.ERROR,
title: "Link not updated",
});
throw error;
}
},
remove: async (linkId: string) => {
Expand Down
6 changes: 2 additions & 4 deletions web/core/components/modules/links/create-update-modal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,6 @@ import { Controller, useForm } from "react-hook-form";
import type { ILinkDetails, ModuleLink } from "@plane/types";
// plane ui
import { Button, Input, ModalCore, setToast, TOAST_TYPE } from "@plane/ui";
// helpers
import { checkURLValidity } from "@/helpers/string.helper";

type Props = {
createLink: (formData: ModuleLink) => Promise<void>;
Expand Down Expand Up @@ -39,9 +37,10 @@ export const CreateUpdateModuleLinkModal: FC<Props> = (props) => {
};

const handleFormSubmit = async (formData: ModuleLink) => {
const parsedUrl = formData.url.startsWith("http") ? formData.url : `http://${formData.url}`;
const payload = {
title: formData.title,
url: formData.url,
url: parsedUrl,
};

try {
Expand Down Expand Up @@ -92,7 +91,6 @@ export const CreateUpdateModuleLinkModal: FC<Props> = (props) => {
name="url"
rules={{
required: "URL is required",
validate: (value) => checkURLValidity(value) || "URL is invalid",
}}
render={({ field: { value, onChange, ref } }) => (
<Input
Expand Down