-
Notifications
You must be signed in to change notification settings - Fork 47
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.
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
improvement: status type | edit/create mode #3185
base: develop
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -1,11 +1,12 @@ | ||||||||||||||||||||||||||||||||
import { EditPenUnderlineIcon, TrashIcon } from 'assets/svg'; | ||||||||||||||||||||||||||||||||
import { Button, Text, Tooltip } from 'lib/components'; | ||||||||||||||||||||||||||||||||
import Image from 'next/image'; | ||||||||||||||||||||||||||||||||
import { CHARACTER_LIMIT_TO_SHOW } from '@app/constants'; | ||||||||||||||||||||||||||||||||
import { IClassName } from '@app/interfaces'; | ||||||||||||||||||||||||||||||||
import { clsxm } from '@app/utils'; | ||||||||||||||||||||||||||||||||
import { getTextColor } from '@app/helpers'; | ||||||||||||||||||||||||||||||||
import { useTranslations } from 'next-intl'; | ||||||||||||||||||||||||||||||||
import { useEffect } from 'react'; | ||||||||||||||||||||||||||||||||
import { svgFetch } from '@app/services/server/fetch'; | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
export const StatusesListCard = ({ | ||||||||||||||||||||||||||||||||
statusIcon, | ||||||||||||||||||||||||||||||||
|
@@ -27,6 +28,10 @@ export const StatusesListCard = ({ | |||||||||||||||||||||||||||||||
const textColor = getTextColor(bgColor); | ||||||||||||||||||||||||||||||||
const t = useTranslations(); | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
useEffect(() => { | ||||||||||||||||||||||||||||||||
loadSVG(statusIcon, 'icon-container' + statusTitle, textColor); | ||||||||||||||||||||||||||||||||
}, []); | ||||||||||||||||||||||||||||||||
Comment on lines
+31
to
+33
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add missing dependencies and cleanup to useEffect. The current implementation has several issues:
Here's the suggested fix: useEffect(() => {
- loadSVG(statusIcon, 'icon-container' + statusTitle, textColor);
+ let mounted = true;
+ const loadIcon = async () => {
+ if (mounted) {
+ await loadSVG(statusIcon, 'icon-container' + statusTitle, textColor);
+ }
+ };
+ loadIcon();
+ return () => {
+ mounted = false;
+ };
-}, []);
+}, [statusIcon, statusTitle, textColor]); 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
return ( | ||||||||||||||||||||||||||||||||
<div className="border w-[21.4rem] flex items-center p-1 rounded-xl justify-between"> | ||||||||||||||||||||||||||||||||
<div | ||||||||||||||||||||||||||||||||
|
@@ -37,18 +42,7 @@ export const StatusesListCard = ({ | |||||||||||||||||||||||||||||||
)} | ||||||||||||||||||||||||||||||||
style={{ backgroundColor: bgColor === '' ? undefined : bgColor }} | ||||||||||||||||||||||||||||||||
> | ||||||||||||||||||||||||||||||||
{statusIcon && ( | ||||||||||||||||||||||||||||||||
<Image | ||||||||||||||||||||||||||||||||
src={statusIcon} | ||||||||||||||||||||||||||||||||
alt={statusTitle} | ||||||||||||||||||||||||||||||||
width={20} | ||||||||||||||||||||||||||||||||
height={20} | ||||||||||||||||||||||||||||||||
decoding="async" | ||||||||||||||||||||||||||||||||
data-nimg="1" | ||||||||||||||||||||||||||||||||
loading="lazy" | ||||||||||||||||||||||||||||||||
className="min-h-[20px]" | ||||||||||||||||||||||||||||||||
/> | ||||||||||||||||||||||||||||||||
)} | ||||||||||||||||||||||||||||||||
{statusIcon && <div id={'icon-container' + statusTitle}></div>} | ||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Improve ID generation security and uniqueness. The current ID generation method has potential security and uniqueness issues:
Consider using a more secure approach: -{statusIcon && <div id={'icon-container' + statusTitle}></div>}
+{statusIcon && (
+ <div
+ id={`icon-container-${encodeURIComponent(statusTitle)}-${Math.random().toString(36).substr(2, 9)}`}
+ />
+)} 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||
<Tooltip | ||||||||||||||||||||||||||||||||
label={statusTitle} | ||||||||||||||||||||||||||||||||
enabled={statusTitle.length >= CHARACTER_LIMIT_TO_SHOW} | ||||||||||||||||||||||||||||||||
|
@@ -84,3 +78,36 @@ export const StatusesListCard = ({ | |||||||||||||||||||||||||||||||
</div> | ||||||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
/** | ||||||||||||||||||||||||||||||||
* A function to load an SVG and gives the ability to | ||||||||||||||||||||||||||||||||
* update its attributes. e.g: fill color | ||||||||||||||||||||||||||||||||
* | ||||||||||||||||||||||||||||||||
* @param {string} url the URL of the SVG file to load | ||||||||||||||||||||||||||||||||
* @param {string} containerId the ID of the container where the SVG will be inserted | ||||||||||||||||||||||||||||||||
* @param {string} color the fill color for the SVG | ||||||||||||||||||||||||||||||||
*/ | ||||||||||||||||||||||||||||||||
const loadSVG = async (url: string, containerId: string, color: string): Promise<void> => { | ||||||||||||||||||||||||||||||||
try { | ||||||||||||||||||||||||||||||||
const response = await svgFetch(url); | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
if (!response.ok) { | ||||||||||||||||||||||||||||||||
throw new Error(`Failed to fetch SVG: ${response.statusText}`); | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
let svgContent = await response.text(); | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
// Update the fill color in the SVG content | ||||||||||||||||||||||||||||||||
svgContent = svgContent.replace(/stroke="[^"]*"/g, `stroke="${color}"`); | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
const container = document.getElementById(containerId); | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
if (container) { | ||||||||||||||||||||||||||||||||
container.innerHTML = svgContent; | ||||||||||||||||||||||||||||||||
} else { | ||||||||||||||||||||||||||||||||
console.error(`Container with ID "${containerId}" not found.`); | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
} catch (error) { | ||||||||||||||||||||||||||||||||
console.error(`Error loading SVG: ${(error as Error).message}`); | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||||||||
Comment on lines
+90
to
+113
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Codebase verification Based on the context gathered, I can now generate the final response: Security improvements needed for SVG loading implementation The current implementation in
While the security concerns are valid, here's a more targeted improvement suggestion: const loadSVG = async (url: string, containerId: string, color: string): Promise<void> => {
try {
const response = await svgFetch(url);
if (!response.ok) {
throw new Error(`Failed to fetch SVG: ${response.statusText}`);
}
const parser = new DOMParser();
const svgDoc = parser.parseFromString(await response.text(), 'image/svg+xml');
// Verify we have an SVG element
const svgElement = svgDoc.querySelector('svg');
if (!svgElement) {
throw new Error('Invalid SVG content');
}
// Update stroke colors
svgElement.querySelectorAll('*').forEach(el => {
if (el instanceof Element && el.hasAttribute('stroke')) {
el.setAttribute('stroke', color);
}
});
const container = document.getElementById(containerId);
if (container) {
container.replaceChildren(svgElement);
}
} catch (error) {
console.error(`Error loading SVG: ${(error as Error).message}`);
}
};
🔗 Analysis chainEnhance SVG loading security and robustness. The current implementation has several security and reliability concerns:
Consider implementing these security improvements: const loadSVG = async (url: string, containerId: string, color: string): Promise<void> => {
try {
const response = await svgFetch(url);
if (!response.ok) {
throw new Error(`Failed to fetch SVG: ${response.statusText}`);
}
let svgContent = await response.text();
+
+ // Create a DOMParser to safely parse the SVG
+ const parser = new DOMParser();
+ const doc = parser.parseFromString(svgContent, 'image/svg+xml');
+
+ // Validate that we received an SVG
+ if (doc.documentElement.tagName !== 'svg') {
+ throw new Error('Invalid SVG content');
+ }
+
+ // Update colors using DOM methods instead of regex
+ const elements = doc.querySelectorAll('*');
+ elements.forEach(el => {
+ if (el.hasAttribute('stroke')) {
+ el.setAttribute('stroke', color);
+ }
+ });
const container = document.getElementById(containerId);
if (container) {
- container.innerHTML = svgContent;
+ // Clear existing content
+ while (container.firstChild) {
+ container.removeChild(container.firstChild);
+ }
+ // Safely append the new SVG
+ container.appendChild(doc.documentElement);
} else {
console.error(`Container with ID "${containerId}" not found.`);
}
} catch (error) {
console.error(`Error loading SVG: ${(error as Error).message}`);
}
}; Additionally, consider implementing a content security policy (CSP) to restrict SVG sources to trusted domains. 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Check if we have any Content-Security-Policy headers defined
rg -l "Content-Security-Policy" --type-add 'config:*.{json,js,ts}' -t config
Length of output: 76 Script: #!/bin/bash
# Let's check where this SVG loading function is used and what SVG sources are being loaded
rg -A 2 "loadSVG\("
# Also check for any existing SVG security measures or sanitization
rg -l "DOMParser|createElementNS|sanitize.*svg" --type=typescript
# Check for any SVG-related utility functions or components
fd -e ts -e tsx -X rg "svg" -l
Length of output: 10728 Script: #!/bin/bash
# Let's check the svgFetch implementation and where SVGs are being loaded from
rg -A 5 "svgFetch" --type=typescript
# Check the specific usage context in list-card.tsx
cat "apps/web/lib/settings/list-card.tsx"
# Look for any existing security configurations or middleware
rg -l "security|sanitize|xss" --type=typescript
Length of output: 3616 |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -6,7 +6,7 @@ import { clsxm } from '@app/utils'; | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import { Spinner } from '@components/ui/loaders/spinner'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import { PlusIcon } from '@heroicons/react/20/solid'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import { Button, ColorPicker, InputField, Modal, Text } from 'lib/components'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import { useCallback, useEffect, useState } from 'react'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import { useCallback, useEffect, useMemo, useState } from 'react'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import { useForm } from 'react-hook-form'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import { useTranslations } from 'next-intl'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import { useAtom } from 'jotai'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -30,6 +30,8 @@ export const TaskStatusesForm = ({ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const [createNew, setCreateNew] = useState(formOnly); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const [edit, setEdit] = useState<ITaskStatusItemList | null>(null); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const t = useTranslations(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const [selectedStatusType, setSelectedStatusType] = useState<string | null>(null); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const [randomColor, setRandomColor] = useState<string | undefined>(undefined); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const taskStatusIconList: IIcon[] = generateIconList('task-statuses', [ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
'open', | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -53,11 +55,12 @@ export const TaskStatusesForm = ({ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
'low' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
]); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const iconList: IIcon[] = [ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const iconList: IIcon[] = useMemo(() => [ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
...taskStatusIconList, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
...taskSizesIconList, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
...taskPrioritiesIconList | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
]; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
// eslint-disable-next-line react-hooks/exhaustive-deps | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
],[]) ; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+58
to
+63
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add dependencies to useMemo The empty dependency array means the iconList will never be recomputed even if the source lists change. Add the source lists as dependencies. const iconList: IIcon[] = useMemo(() => [
...taskStatusIconList,
...taskSizesIconList,
...taskPrioritiesIconList
- // eslint-disable-next-line react-hooks/exhaustive-deps
- ],[]) ;
+ ],[taskStatusIconList, taskSizesIconList, taskPrioritiesIconList]) ; 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
loading, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -153,6 +156,45 @@ export const TaskStatusesForm = ({ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
: []; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const { isOpen, closeModal, openModal } = useModal(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
/** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* Get Icon by status name | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* @param {string} iconName - Name of the icon | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* @returns {IIcon} - Icon of the status | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
*/ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const getIcon = useCallback((iconName: string) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const name = iconName == "ready-for-review" ? "ready" : iconName; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const icon = iconList.find(icon => icon.title === name) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if(icon){ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
setValue("icon", icon.path) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return icon | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
},[iconList, setValue]) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+165
to
+175
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Enhance getIcon function robustness The function needs better error handling and could use a more maintainable approach for icon name mapping. +const ICON_NAME_MAP: Record<string, string> = {
+ 'ready-for-review': 'ready'
+};
const getIcon = useCallback((iconName: string) => {
- const name = iconName == "ready-for-review" ? "ready" : iconName;
+ const name = ICON_NAME_MAP[iconName] || iconName;
const icon = iconList.find(icon => icon.title === name)
+ if (!icon) {
+ console.warn(`Icon not found for status: ${iconName}`);
+ }
if(icon){
setValue("icon", icon.path)
}
return icon
},[iconList, setValue]) 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
/** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* Get random color for new status | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* @returns {string} - Random color | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
*/ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const getRandomColor = useCallback(() => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const letters = '0123456789ABCDEF'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
let color = '#'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
for (let i = 0; i < 6; i++) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
color += letters[Math.floor(Math.random() * 16)]; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return color; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}, []); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+183
to
+190
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Simplify random color generation The current implementation can be simplified using array methods. const getRandomColor = useCallback(() => {
- const letters = '0123456789ABCDEF';
- let color = '#';
- for (let i = 0; i < 6; i++) {
- color += letters[Math.floor(Math.random() * 16)];
- }
- return color;
+ return '#' + Array.from({ length: 6 }, () =>
+ Math.floor(Math.random() * 16).toString(16)
+ ).join('');
}, []); 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
useEffect(() => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if (!edit && selectedStatusType) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
setRandomColor(getRandomColor()); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}, [selectedStatusType, edit, getRandomColor]); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
<> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
<Modal isOpen={isOpen} closeModal={closeModal}> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -191,7 +233,7 @@ export const TaskStatusesForm = ({ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
variant="outline" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
className="rounded-[10px]" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Sort | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{t('common.SORT')} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
</Button> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
</div> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{(createNew || edit) && ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -214,22 +256,26 @@ export const TaskStatusesForm = ({ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{...register('name')} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
/> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
<StandardTaskStatusDropDown | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
onValueChange={(status) => setValue('template', status)} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
className=" h-14 shrink-0" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
onValueChange={(status) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
setValue('template', status) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
setSelectedStatusType(status) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
className="h-14 shrink-0" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
defaultValue={edit?.value} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+259
to
+264
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Handle defaultValue prop consistently The <StandardTaskStatusDropDown
onValueChange={(status) => {
setValue('template', status)
setSelectedStatusType(status)
}}
className="h-14 shrink-0"
- defaultValue={edit?.value}
+ defaultValue={edit?.value || selectedStatusType}
/> 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
/> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
<IconPopover | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
iconList={iconList} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
setValue={setValue} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
active={ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
edit | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
selectedStatusType ? getIcon(selectedStatusType) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
: edit | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
? (iconList.find( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
(icon) => icon.path === edit.icon | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
) as IIcon) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
: null | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
) as IIcon) : null | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
/> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
<ColorPicker | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
defaultColor={edit ? edit.color : undefined} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
defaultColor={edit ? edit.color : randomColor} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
onChange={(color) => setValue('color', color)} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
className=" shrink-0" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
/> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -243,6 +289,9 @@ export const TaskStatusesForm = ({ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
createTaskStatusLoading || editTaskStatusLoading | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
loading={createTaskStatusLoading || editTaskStatusLoading} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
onClick={() => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
setSelectedStatusType(null); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{edit ? t('common.SAVE') : t('common.CREATE')} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
</Button> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -253,6 +302,7 @@ export const TaskStatusesForm = ({ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
onClick={() => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
setCreateNew(false); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
setEdit(null); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
setSelectedStatusType(null); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{t('common.CANCEL')} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider implementing caching for SVG responses.
For better performance and reduced network calls, consider implementing a caching strategy for SVG responses.
Here's a suggested implementation using a simple in-memory cache:
Add input validation and error handling for SVG fetching.
The current implementation lacks several important safety checks and error handling mechanisms:
Consider implementing these improvements:
📝 Committable suggestion