Add "filters" title on dashboards#173
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughSe han realizado modificaciones en varios componentes de gráficos en el frontend, específicamente en Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (8)
frontend/src/components/charts/SeverityPieChart.tsx (2)
45-56: ¡Los cambios en la configuración del título son correctos, pero hay margen de mejora!La implementación cumple con el objetivo de añadir el título "Filters", sin embargo, los valores de estilo están hardcodeados. Deberías considerar mover estos valores a constantes de tema para mantener la consistencia en toda la aplicación.
Te sugiero este cambio:
+ // En un archivo de constantes de tema + export const CHART_TITLE_STYLES = { + color: 'white', + padding: { top: 20 }, + font: { weight: 'bold', size: 15 } + }; title: { display: true, text: t('filters'), - color: 'white', - padding: { - top: 20, - }, - font: { - weight: 'bold', - size: 15, - }, + ...CHART_TITLE_STYLES },
Line range hint
58-69: ¡El cálculo de porcentajes necesita optimización!La lógica actual funciona, pero está usando un enfoque menos eficiente con forEach. Además, el cálculo del total se está realizando en cada llamada al formatter.
Te propongo esta implementación más eficiente:
datalabels: { + // Calcular el total una sola vez fuera del formatter + const totalValue = data.reduce((sum, d) => sum + d.value, 0); formatter: (value: number) => { - let total = 0; - data.forEach(d => { - total += d.value; - }); - const percentage = ((value / total) * 100).toFixed(2); + const percentage = ((value / totalValue) * 100).toFixed(2); if (percentage === '0.00' || percentage === 'NaN') { return ''; } return percentage + '%'; },Esta implementación:
- Evita recalcular el total en cada llamada al formatter
- Usa reduce en lugar de forEach, que es más idiomático en JavaScript/TypeScript
- Mantiene la misma funcionalidad pero con mejor rendimiento
frontend/src/components/charts/TimePerAuditChart.tsx (2)
65-73: ¡Los valores de estilo deberían usar constantes del tema!La configuración del título está correcta, pero los valores de estilo están hardcodeados. Esto podría causar problemas de mantenimiento y consistencia visual en el futuro.
Te sugiero refactorizar el código así:
title: { display: true, text: t('filters'), - color: 'white', - font: { - weight: 'bold', - size: 15, - }, + color: theme.colors.text.primary, + font: { + weight: theme.typography.weights.bold, + size: theme.typography.sizes.medium, + }, },
Line range hint
76-95: ¡La implementación del formatter es ineficiente y poco segura!El código actual tiene varios problemas serios:
- Recalcula el total para cada punto de datos, lo cual es ineficiente
- Usa conversiones de tipo poco seguras
- La lógica de cálculo es más compleja de lo necesario
Te propongo esta implementación más eficiente y segura:
const calculateTotal = (data: AuditTime[]): number => data.reduce((sum, d) => sum + d.execution + d.remediation, 0); // En el componente, antes de options: const total = calculateTotal(data); // En el formatter: datalabels: { formatter: (value: number) => { return `${((value / total) * 100).toFixed(2)}%`; }, color: '#fff' as const, },Esta solución:
- Calcula el total una sola vez
- Elimina las conversiones de tipo innecesarias
- Simplifica la lógica del formatter
frontend/src/components/charts/CIATriadChart.tsx (2)
94-102: ¡Extraer valores de estilo a constantes!La implementación del título funciona, pero hay varios valores "mágicos" que deberían extraerse a constantes. Además, los colores deberían utilizar variables del tema para mantener la consistencia.
Te sugiero aplicar estos cambios:
+ const LEGEND_TITLE_FONT_SIZE = 15; + const LEGEND_TITLE_FONT_WEIGHT = 'bold'; + // Agregar en el archivo de tema + const LEGEND_TEXT_COLOR = 'white'; title: { display: true, text: t('filters'), - color: 'white', + color: LEGEND_TEXT_COLOR, font: { - weight: 'bold', - size: 15, + weight: LEGEND_TITLE_FONT_WEIGHT, + size: LEGEND_TITLE_FONT_SIZE, }, },
Line range hint
1-138: ¡El componente necesita varias mejoras estructurales!He identificado varios puntos que requieren atención inmediata:
- Los valores de las etiquetas de escala ('None', 'Low', 'High') están hardcodeados y deberían estar en constantes o usar i18n.
- Los colores están dispersos por todo el código.
- No hay optimización de rendimiento para las transformaciones de datos.
Te propongo implementar estos cambios:
+ const CIA_SCALE_LABELS = { + NONE: { value: 0, label: t('scale.none') }, + LOW: { value: 1, label: t('scale.low') }, + HIGH: { value: 2, label: t('scale.high') }, + } as const; + const CHART_COLORS = { + backgroundColor: 'rgba(130, 202, 157, 0.5)', + borderColor: 'rgb(130, 202, 157)', + gridColor: 'rgba(255, 255, 255, 0.1)', + } as const; export const CIATriadChart: React.FC<Props> = ({ data }) => { + const chartData = useMemo(() => ({ labels: data.map(d => d.subject), datasets: [{ label: 'Average', data: data.map(d => d.current), - backgroundColor: 'rgba(130, 202, 157, 0.5)', - borderColor: 'rgb(130, 202, 157)', + backgroundColor: CHART_COLORS.backgroundColor, + borderColor: CHART_COLORS.borderColor, borderWidth: 1, }], - }); + }), [data]); // En la configuración de ticks callback: (value: number | string) => { - if (value === 0) return 'None'; - if (value === 1) return 'Low'; - if (value === 2) return 'High'; + const scaleLabel = Object.values(CIA_SCALE_LABELS).find( + label => label.value === value + ); + return scaleLabel?.label ?? value; }frontend/src/components/dashboard/CVSSScore.tsx (2)
98-106: ¡La implementación del título es correcta pero podemos mejorarla!La adición del título "Filters" está bien implementada, pero sería más mantenible extraer la configuración del estilo a una constante separada. Esto facilitaría la consistencia entre diferentes gráficos y reduciría la duplicación de código.
Te sugiero aplicar este cambio:
+ const CHART_TITLE_STYLE = { + display: true, + color: 'white', + font: { + weight: 'bold', + size: 15, + }, + }; const pieChartOptions = { responsive: true, maintainAspectRatio: false, plugins: { legend: { position: 'right' as const, title: { - display: true, - text: t('filters'), - color: 'white', - font: { - weight: 'bold', - size: 15, - }, + ...CHART_TITLE_STYLE, + text: t('filters'), },
Line range hint
1-144: ¡Hay varios aspectos importantes que necesitan atención!
No hay manejo de estados de carga o error en el componente. Los usuarios no sabrán si hay un problema al cargar los datos.
La función de cálculo de porcentajes en el formateador de datalabels se está recalculando innecesariamente para cada valor.
El componente no tiene tipos TypeScript estrictos para los datos del gráfico.
Te propongo implementar estas mejoras:
+ type ChartData = { + labels: string[]; + datasets: { + data: number[]; + backgroundColor: string[]; + borderColor: string[]; + borderWidth: number; + }[]; + }; const CVSSScore: React.FC<CVSSScoreProps> = ({ auditId }) => { const paramId = useParams().auditId; if (!auditId) { auditId = paramId; } - const [pieChartData, setPieChartData] = useState({ + const [isLoading, setIsLoading] = useState(true); + const [error, setError] = useState<string | null>(null); + const [pieChartData, setPieChartData] = useState<ChartData>({ // ... resto del estado inicial }); useEffect(() => { + setIsLoading(true); + setError(null); getAuditById(auditId) .then(audit => { // ... código existente + setIsLoading(false); }) - .catch(console.error); + .catch(err => { + setError(err.message); + setIsLoading(false); + console.error(err); + }); }, [auditId]); + if (isLoading) return <div>Cargando...</div>; + if (error) return <div>Error: {error}</div>; return ( <div className="bg-gray-800 rounded-lg p-6"> <div className="chart-container" style={{ height: '400px' }}> <Pie data={pieChartData} options={pieChartOptions} /> </div> </div> ); };También deberías optimizar el cálculo de porcentajes:
plugins: { datalabels: { formatter: (value: number) => { - let total = 0; - pieChartData.datasets[0].data.forEach(d => { - total += d; - }); + const total = pieChartData.datasets[0].data.reduce((sum, d) => sum + d, 0); const percentage = ((value / total) * 100).toFixed(2); if (percentage === '0.00' || percentage === 'NaN') { return ''; } return percentage + '%'; },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
frontend/src/components/charts/CIATriadChart.tsx(1 hunks)frontend/src/components/charts/SeverityPieChart.tsx(1 hunks)frontend/src/components/charts/TimePerAuditChart.tsx(1 hunks)frontend/src/components/dashboard/CIATriad.tsx(2 hunks)frontend/src/components/dashboard/CVSSScore.tsx(1 hunks)frontend/src/i18n/en-US/index.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- frontend/src/i18n/en-US/index.ts
🔇 Additional comments (2)
frontend/src/components/dashboard/CIATriad.tsx (1)
10-10: ¡Importación correcta de i18next!
La adición de la función de traducción está bien implementada y sigue las mejores prácticas de internacionalización.
frontend/src/components/dashboard/CVSSScore.tsx (1)
Line range hint 1-3: ¡Necesitamos verificar las dependencias!
El comentario eslint-disable import/no-extraneous-dependencies sugiere que podría haber problemas con las dependencias. Debemos asegurarnos de que todas las dependencias estén correctamente declaradas en el package.json.
Ejecutemos este script para verificar:
| title: { | ||
| display: true, | ||
| text: t('filters'), | ||
| color: 'white', | ||
| font: { | ||
| weight: 'bold', | ||
| size: 15, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
¡La configuración del título necesita mejoras importantes!
La implementación actual tiene varios problemas que necesitan ser corregidos:
- El color del texto está hardcodeado a 'white', lo que no es mantenible.
- El tamaño de la fuente está hardcodeado a 15px.
- No hay consideración para temas oscuros/claros.
Te sugiero aplicar estos cambios:
title: {
display: true,
text: t('filters'),
- color: 'white',
+ color: 'var(--text-primary)',
font: {
weight: 'bold',
- size: 15,
+ size: 'var(--text-size-title)',
},
},También deberías:
- Agregar variables CSS para los colores y tamaños en tu archivo de estilos
- Implementar soporte para temas oscuros/claros
¿Necesitas ayuda para implementar un sistema de temas más robusto?
Committable suggestion skipped: line range outside the PR's diff.
…itle and labels components of CVSSScore component.
massi-ponce
requested review from
Icksir,
caverav,
iTzGooDLife,
jllanosg and
sealra
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
frontend/src/components/dashboard/AverageCVSS.tsx (1)
Line range hint
41-123: ¡El manejo de errores es insuficiente!El componente tiene varios problemas serios:
- Solo usas
console.errorpara los errores - ¡los usuarios no ven nada cuando algo falla!- No hay estados de carga
- La lógica de cálculo en el
useEffectes demasiado complejaNecesitas:
- Agregar estados de carga y error
- Extraer la lógica de cálculo
- Mostrar mensajes de error al usuario
Te propongo esta estructura:
const [isLoading, setIsLoading] = useState(true); const [error, setError] = useState<Error | null>(null); const calculateAverageScore = (findings: Finding[]) => { return Math.round( (findings.reduce((acc, finding) => acc + cvssStringToScore(finding.cvssv3 ?? ''), 0) / findings.length) * 10 ) / 10; }; useEffect(() => { const fetchData = async () => { try { setIsLoading(true); setError(null); const result = clientName === undefined ? await getAuditById(auditIdRef.current) : await getAuditsByClientName(clientName); setAverageCVSS(calculateAverageScore(result.datas.findings)); setData(/* ... */); } catch (err) { setError(err instanceof Error ? err : new Error('Error desconocido')); } finally { setIsLoading(false); } }; fetchData(); }, [auditId, clientName, paramId]); if (isLoading) return <div>Cargando...</div>; if (error) return <div>Error: {error.message}</div>;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
frontend/src/components/charts/CIATriadChart.tsx(2 hunks)frontend/src/components/charts/SeverityPieChart.tsx(1 hunks)frontend/src/components/charts/TimePerAuditChart.tsx(1 hunks)frontend/src/components/dashboard/AverageCVSS.tsx(3 hunks)frontend/src/components/dashboard/CIATriad.tsx(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- frontend/src/components/charts/CIATriadChart.tsx
- frontend/src/components/charts/SeverityPieChart.tsx
- frontend/src/components/charts/TimePerAuditChart.tsx
- frontend/src/components/dashboard/CIATriad.tsx
|
|
||
| const options: ChartOptions<'bar'> = { | ||
| indexAxis: 'y', | ||
| indexAxis: 'y' as const, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
¡Las aserciones de tipo pueden mejorarse!
Las aserciones de tipo as const que has agregado son redundantes y hacen el código más verboso. En su lugar, deberías:
- Definir las constantes al inicio del archivo
- Utilizar un tipo más específico para las opciones del gráfico
Te propongo este cambio:
+ const CHART_COLORS = {
+ GRID: 'rgba(255, 255, 255, 0.1)',
+ TEXT: 'white',
+ } as const;
const options: ChartOptions<'bar'> = {
- indexAxis: 'y' as const,
+ indexAxis: 'y',
...
ticks: {
- color: 'white' as const,
+ color: CHART_COLORS.TEXT,
},
grid: {
- color: 'rgba(255, 255, 255, 0.1)' as const,
+ color: CHART_COLORS.GRID,
}
}Also applies to: 140-140, 143-143, 148-148
| annotations: { | ||
| line1: { | ||
| type: 'line', | ||
| type: 'line' as const, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
¡Más aserciones de tipo innecesarias!
La aserción de tipo para la anotación y el color del borde también pueden mejorarse.
Aplica este cambio junto con las constantes anteriores:
const CHART_COLORS = {
GRID: 'rgba(255, 255, 255, 0.1)',
TEXT: 'white',
+ BORDER: '#2ecc71',
} as const;
// En las anotaciones
annotations: {
line1: {
- type: 'line' as const,
+ type: 'line',
- borderColor: '#2ecc71' as const,
+ borderColor: CHART_COLORS.BORDER,
...
}
}Also applies to: 168-168
2 participants
Descripción
Se agrega un título para los filtros dentro de los dashboards (tanto para los audit dashboard como para los client dashboard).
Motivación y Contexto
Mejorar UX.
¿Cómo ha sido probado?
Tipos de cambios
Lista de verificación:
Summary by CodeRabbit
Nuevas Funciones
Mejoras Visuales
Documentación