Skip to content

CWE Model Update#161

Merged
caverav merged 20 commits intodevelopmentfrom
feature/check-cwe-update
Nov 6, 2024
Merged

CWE Model Update#161
caverav merged 20 commits intodevelopmentfrom
feature/check-cwe-update

Conversation

@jllanosg
Copy link
Copy Markdown
Collaborator

@jllanosg jllanosg commented Nov 3, 2024

Descripción

Agrega el backend y el frontend para actualizar el modelo de CWE Si es necesario

Motivación y Contexto

HU04

¿Cómo ha sido probado?

Si hay una actualización disponible, aparece un modal que indica si se quiere actualizar o no. En caso de aceptarla, debería comenzar a actualizarse, apareciendo un icono de carga. Si se rechaza, entonces aparece el mismo ícono pero que permite abrir nuevamente el modal y aceptar la actualización.

Como esto es dificil de hacer ya que el modelo no cambia constantemente, se puede moficar el efecto que hace el checkeo en ModeUpdateContainer.tsx, linea 26:

setIsOpenModal(result);

De esta manera abrirá el modal aunque no haya actualización, y permite hacer el flujo de actualizar el modelo.

Capturas de pantalla (si es apropiado):

Tipos de cambios

  • Bugfix (cambio que no interrumpe el funcionamiento y que soluciona un problema)
  • New feature (cambio que no interrumpe el funcionamiento y que añade funcionalidad)
  • Breaking change (corrección o funcionalidad que podría causar que la funcionalidad existente cambie)

Lista de verificación:

  • Mi código sigue el estilo de código de este proyecto.
  • Mi cambio requiere una modificación en la documentación.
  • He actualizado la documentación en consecuencia.
  • Requiere nuevos tests.

Summary by CodeRabbit

  • Nuevas Funciones

    • Se han añadido nuevas rutas para verificar y actualizar el modelo CWE.
    • Nuevos endpoints API para comprobar actualizaciones y actualizar el modelo CWE.
    • Se ha introducido un componente para gestionar el proceso de actualización del modelo en la interfaz de usuario.
  • Mejoras en la Interfaz de Usuario

    • Nuevos mensajes informativos y un botón de "dismiss" para mejorar la interacción del usuario.
  • Corrección de Errores

    • Mejoras en la gestión de errores para las operaciones de verificación y actualización del modelo CWE.
    • Se ha mejorado la configuración de seguridad relacionada con la generación de secretos JWT.
    • Se han añadido tiempos de espera configurables para las operaciones de verificación y actualización del modelo CWE.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Nov 3, 2024

Caution

Review failed

The pull request is closed.

📝 Walkthrough
<details>
<summary>📝 Walkthrough</summary>

## Walkthrough
Este pull request introduce nuevas funcionalidades en la aplicación, específicamente relacionadas con la gestión de actualizaciones del modelo CWE. Se agregan nuevas rutas en el backend para verificar y actualizar el modelo CWE, así como mejoras en la generación de secretos JWT y permisos de usuario. También se implementan nuevos componentes en el frontend para manejar el proceso de actualización, incluyendo la verificación de actualizaciones y la interacción del usuario a través de un modal.

## Changes

| Archivo                               | Resumen de cambios                                                                                                                                               |
|---------------------------------------|-----------------------------------------------------------------------------------------------------------------------------------------------------------------|
| `backend/src/app.js`                 | Se agregan nuevas rutas: `check-cwe-update` y `update-cwe-model`.                                                                                           |
| `backend/src/lib/auth.js`            | Se modifica la generación de secretos JWT y se amplían los permisos del rol `user` para incluir `'check-update:all'` y `'update-model:all'`.                  |
| `backend/src/routes/check-cwe-update.js` | Se crea un nuevo manejador de ruta para `/api/check-cwe-update` que verifica actualizaciones del modelo CWE.                                                  |
| `backend/src/routes/update-cwe-model.js` | Se crea un nuevo manejador de ruta para `/api/update-cwe-model` que permite actualizar el modelo CWE.                                                        |
| `cwe_api/Dockerfile`                 | Se modifica la lógica de instalación del modelo `modelo_cwe`, eliminando la eliminación del archivo zip después de descomprimir.                               |
| `cwe_api/main.py`                    | Se añaden constantes y funciones para gestionar el modelo CWE, incluyendo nuevos endpoints para verificar y actualizar el modelo.                             |
| `frontend/src/i18n/en-US/index.ts`   | Se agregan nuevos mensajes y etiquetas para la interfaz de usuario relacionados con la actualización del modelo CWE.                                           |
| `frontend/src/routes/root.tsx`       | Se importa y se añade el componente `ModelUpdateContainer` dentro de la jerarquía del componente `Root`.                                                     |
| `frontend/src/routes/settings/ModelUpdateContainer.tsx` | Se crea un nuevo componente para gestionar el proceso de actualización del modelo, incluyendo lógica para verificar y realizar actualizaciones.                |
| `frontend/src/services/settings.ts`   | Se añaden nuevas funciones asíncronas para verificar y actualizar el modelo CWE.                                                                               |

## Possibly related PRs
- **#104**: Este PR también involucra la adición de nuevas rutas en `backend/src/app.js`, que está directamente relacionado con las nuevas rutas añadidas en el PR principal.
- **#161**: Este PR introduce nuevas rutas en `backend/src/app.js` para verificar y actualizar el modelo CWE, alineándose con los cambios realizados en el PR principal.

## Suggested reviewers
- Icksir
- massi-ponce
- jllanosg
- Sealra

</details>

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

‼️ IMPORTANT
Auto-reply has been disabled for this repository in the CodeRabbit settings. The CodeRabbit bot will not respond to your replies unless it is explicitly tagged.

  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@jllanosg jllanosg added the enhancement New feature or request label Nov 3, 2024
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 22

🧹 Outside diff range and nitpick comments (9)
frontend/src/routes/root.tsx (1)

¡El ModelUpdateContainer definitivamente NO debe estar dentro de ProtectedRoute!

La implementación actual es incorrecta por las siguientes razones críticas:

  • El ModelUpdateContainer maneja actualizaciones del modelo CWE que son fundamentales para el funcionamiento del sistema
  • El componente muestra notificaciones y permite actualizaciones que deberían estar disponibles para todos los usuarios
  • Colocarlo dentro de ProtectedRoute significa que los usuarios no autenticados no recibirán notificaciones de actualizaciones importantes

Para corregir esto:

  • Mover el <ModelUpdateContainer /> fuera del <ProtectedRoute> y colocarlo al mismo nivel que el <Toaster />
  • La estructura correcta debería ser:
<>
  <ProtectedRoute>
    <Navbar />
    <Outlet />
  </ProtectedRoute>
  <ModelUpdateContainer />
  <Toaster {...} />
</>
🔗 Analysis chain

Line range hint 1-29: ¡Atención! El manejo de rutas protegidas requiere verificación adicional

El ModelUpdateContainer está dentro de ProtectedRoute, lo que significa que solo los usuarios autenticados podrán ver las actualizaciones del modelo. ¿Es esto realmente lo que queremos? ¿No deberíamos permitir que todos los usuarios vean cuando hay actualizaciones disponibles?

Considera mover el ModelUpdateContainer fuera del ProtectedRoute si las actualizaciones del modelo son relevantes para todos los usuarios.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Verificar la lógica de autenticación relacionada
rg -g '*.ts' -g '*.tsx' 'isAuthenticated|requireAuth|protected'

Length of output: 1088


Script:

#!/bin/bash
# Verificar el contenido de ModelUpdateContainer para entender su propósito
ast-grep --pattern 'function ModelUpdateContainer() {
  $$$
}'

# Buscar referencias a autenticación en el componente
rg -g '*.tsx' -g '*.ts' -C 3 'ModelUpdateContainer'

# Verificar la implementación de ProtectedRoute
ast-grep --pattern 'function ProtectedRoute($$$) {
  $$$
}'

Length of output: 1668


Script:

#!/bin/bash
# Verificar el contenido completo de ModelUpdateContainer para entender su funcionalidad
cat frontend/src/routes/settings/ModelUpdateContainer.tsx

# Verificar la implementación de ProtectedRoute para entender la lógica de autenticación
cat frontend/src/components/ProtectedRoute.tsx

Length of output: 3489

cwe_api/Dockerfile (1)

Estructura del modelo requiere ajustes importantes en el Dockerfile

La verificación revela problemas críticos que deben ser corregidos:

  • El código espera que el modelo CWE esté en ./modelo_cwe/checkpoint-141693, pero el Dockerfile no garantiza esta estructura específica al descomprimir.
  • No se verifica la existencia del archivo id2label.json y label2id.json que son requeridos por el inferencer.
  • El manejo de errores en el Dockerfile no valida la estructura correcta después de la extracción.

Correcciones necesarias:

  • Validar la existencia del directorio checkpoint-141693 después de descomprimir
  • Verificar la presencia de los archivos JSON necesarios
  • Agregar validaciones post-extracción para la estructura completa del modelo
🔗 Analysis chain

Line range hint 1-42: Verificación necesaria de la estructura del modelo.

Es crucial verificar que la estructura del modelo extraído sea la esperada para evitar problemas en tiempo de ejecución.

Ejecutemos este script para verificar la estructura del modelo:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the expected structure of the CWE model after extraction

# Test: Check if the model has the expected structure
echo "Verificando estructura del modelo CWE..."
fd -t f -e pt -e pth . | while read -r file; do
    echo "Archivo de modelo encontrado: $file"
done

# Test: Check for any remaining zip files that should have been cleaned
echo "Verificando archivos zip residuales..."
fd -e zip . 

Length of output: 231


Script:

#!/bin/bash
# Verificar la existencia y estructura de los directorios principales
echo "=== Verificando directorios principales ==="
fd -t d . -d 1

# Verificar archivos Python relacionados con el modelo
echo -e "\n=== Verificando archivos Python relacionados con el modelo ==="
rg -l "load_state_dict|torch.load" --type py

# Verificar estructura de requirements.txt para dependencias del modelo
echo -e "\n=== Verificando requirements.txt ==="
if [ -f requirements.txt ]; then
    cat requirements.txt | grep -i "torch\|transformers\|numpy"
fi

# Verificar archivos de configuración del modelo
echo -e "\n=== Verificando archivos de configuración ==="
fd -e json -e yaml -e yml -e config

Length of output: 1155


Script:

#!/bin/bash
# Verificar contenido específico de la API de CWE
echo "=== Verificando estructura de cwe_api ==="
fd . cwe_api/ -t f -d 2

# Buscar referencias al manejo de modelos en archivos Python
echo -e "\n=== Verificando código de carga de modelos ==="
rg -C 3 "modelo_cwe|modelo_cvss" --type py

# Verificar requirements.txt específico de cwe_api
echo -e "\n=== Verificando requirements.txt de cwe_api ==="
if [ -f cwe_api/requirements.txt ]; then
    cat cwe_api/requirements.txt
fi

# Verificar archivos de configuración específicos
echo -e "\n=== Verificando archivos de configuración en cwe_api ==="
fd -e json -e yaml -e yml -e config . cwe_api/

Length of output: 1695

backend/src/routes/check-cwe-update.js (1)

1-43: Falta manejo de reintentos y validación de respuesta

La implementación actual no maneja reintentos en caso de fallos temporales y no valida la estructura de la respuesta.

Recomendaciones:

  1. Implementa una estrategia de reintentos para fallos temporales
  2. Valida la estructura de la respuesta JSON antes de enviarla al cliente
  3. Considera implementar un circuit breaker para evitar sobrecarga del servicio CWE

¿Necesitas ayuda para implementar estas mejoras?

backend/src/routes/update-cwe-model.js (2)

35-40: El manejo de errores necesita más detalle

El logging actual es demasiado básico y no proporciona suficiente contexto para diagnóstico.

Mejora el logging así:

     } catch (error) {
-      console.error(error);
+      console.error('Error en la actualización del modelo CWE:', {
+        error: error.message,
+        type: error.name,
+        url: `${cweConfig.host}:${cweConfig.port}`,
+        timestamp: new Date().toISOString()
+      });
       error.name === 'AbortError'

1-43: Falta documentación y pruebas unitarias

El código carece de:

  1. JSDoc para documentar la ruta y sus parámetros
  2. Pruebas unitarias para verificar el comportamiento
  3. Manejo de reintentos para errores temporales

¿Necesitas ayuda para generar la documentación y las pruebas unitarias?

frontend/src/services/settings.ts (1)

Line range hint 1-59: ¡Atención a la seguridad!

La implementación actual carece de medidas de seguridad importantes:

  1. No hay manejo de timeouts en las peticiones
  2. No hay protección explícita contra CSRF
  3. No hay retry logic para errores temporales

Recomiendo crear un wrapper de fetch con estas características:

const DEFAULT_TIMEOUT = 5000;

interface FetchOptions extends RequestInit {
  timeout?: number;
}

async function safeFetch(url: string, options: FetchOptions = {}): Promise<Response> {
  const controller = new AbortController();
  const timeout = options.timeout || DEFAULT_TIMEOUT;
  
  const timeoutId = setTimeout(() => controller.abort(), timeout);
  
  try {
    const response = await fetch(url, {
      ...options,
      credentials: 'include',
      headers: {
        'Content-Type': 'application/json',
        'X-XSRF-TOKEN': getCsrfToken(), // Implementar según tu sistema
        ...options.headers,
      },
      signal: controller.signal,
    });
    
    return response;
  } finally {
    clearTimeout(timeoutId);
  }
}

Utiliza este wrapper en lugar de fetch directo en tus funciones.

frontend/src/routes/settings/ModelUpdateContainer.tsx (1)

52-65: ¡Hay un error tipográfico en las props del Modal!

La prop disablehr parece ser un error tipográfico. Debería ser disableHr o similar siguiendo las convenciones de camelCase.

frontend/src/i18n/en-US/index.ts (2)

739-742: ¡El texto del modal de actualización necesita más contexto!

El mensaje actual es demasiado genérico. Los usuarios necesitan más información para tomar una decisión informada sobre la actualización.

Considera este cambio para mejorar la claridad:

  updatingCWEModel: 'Updating CWE Model',
  CWEModelUpdateText:
-    'A new version of the CWE Model is available, do you want to upgrade?',
+    'A new version of the CWE Model (v${newVersion}) is available. Current version: v${currentVersion}. This update includes security improvements and new vulnerability classifications. Do you want to upgrade?',
  updateAvailable: 'Update Available',

739-742: ¡La organización de los mensajes relacionados con CWE podría mejorarse!

Los mensajes relacionados con CWE están dispersos en diferentes secciones del archivo. Deberían estar agrupados para mejor mantenibilidad.

Sugiero crear una nueva sección específica para CWE:

export default {
+ cwe: {
+   updating: 'Updating CWE Model',
+   updateAvailable: 'Update Available',
+   updateText: 'A new version of the CWE Model is available, do you want to upgrade?',
+   updateSuccess: 'CWE Model Updated Successfully',
+   updateError: 'Failed Updating CWE Model',
+ },
  // ... resto del código
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 8c0bcda and 3a59137.

📒 Files selected for processing (10)
  • backend/src/app.js (1 hunks)
  • backend/src/lib/auth.js (1 hunks)
  • backend/src/routes/check-cwe-update.js (1 hunks)
  • backend/src/routes/update-cwe-model.js (1 hunks)
  • cwe_api/Dockerfile (1 hunks)
  • cwe_api/main.py (3 hunks)
  • frontend/src/i18n/en-US/index.ts (4 hunks)
  • frontend/src/routes/root.tsx (1 hunks)
  • frontend/src/routes/settings/ModelUpdateContainer.tsx (1 hunks)
  • frontend/src/services/settings.ts (2 hunks)
🧰 Additional context used
🪛 Ruff
cwe_api/main.py

103-103: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)

🔇 Additional comments (3)
frontend/src/routes/root.tsx (1)

6-6: ¡La ubicación del ModelUpdateContainer podría causar problemas de UX!

El ModelUpdateContainer está colocado después del Outlet, lo que podría causar problemas de superposición con el contenido principal de la aplicación. Considera estas implicaciones:

  1. El modal de actualización podría aparecer en una posición que interfiera con elementos importantes de la interfaz
  2. No hay garantía de que el z-index se maneje correctamente en todas las situaciones
#!/bin/bash
# Verificar si hay definiciones de z-index que puedan entrar en conflicto
rg -g '*.css' -g '*.scss' 'z-index'

# Buscar otros modales o elementos superpuestos
ast-grep --pattern 'modal|dialog|overlay|drawer'

Te sugiero mover el ModelUpdateContainer antes del Outlet para mantener una jerarquía más clara:

<ProtectedRoute>
  <Navbar />
+ <ModelUpdateContainer />
  <Outlet />
- <ModelUpdateContainer />
</ProtectedRoute>

Also applies to: 14-14

backend/src/app.js (1)

159-160: ¡Necesitamos verificar la implementación de seguridad en las nuevas rutas!

Las nuevas rutas check-cwe-update y update-cwe-model se están agregando a una aplicación con medidas de seguridad estrictas. Es crucial verificar que estas rutas implementen:

  • Los mismos controles de acceso que las rutas existentes
  • Manejo adecuado de errores
  • Validación de entrada
  • Cumplimiento con las políticas CSP existentes

Ejecutemos algunas verificaciones:

Asegúrate de que las nuevas rutas:

  1. Utilicen el mismo patrón de manejo de errores que las rutas existentes
  2. Implementen logging consistente
  3. Sigan las mismas prácticas de validación de entrada
frontend/src/i18n/en-US/index.ts (1)

166-166: ¡Los mensajes de éxito y error están correctamente implementados!

Los mensajes siguen el patrón establecido y son consistentes con el resto de la aplicación.

Also applies to: 207-207

Comment thread cwe_api/Dockerfile
Comment thread cwe_api/Dockerfile
Comment thread backend/src/routes/check-cwe-update.js
Comment thread backend/src/routes/check-cwe-update.js
Comment thread backend/src/routes/check-cwe-update.js
Comment thread frontend/src/routes/settings/ModelUpdateContainer.tsx
Comment thread backend/src/lib/auth.js
Comment thread cwe_api/main.py Outdated
Comment thread cwe_api/main.py
Comment thread cwe_api/main.py Outdated
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🧹 Outside diff range and nitpick comments (1)
frontend/src/routes/settings/ModelUpdateContainer.tsx (1)

82-82: ¡La animación del ícono debe ser condicional!

El ícono no debería girar constantemente, solo cuando se está actualizando.

- <ArrowPathIcon className="h-8 w-8 animate-spin text-gray-100" />
+ <ArrowPathIcon 
+   className={clsx(
+     'h-8 w-8 text-gray-100',
+     { 'animate-spin': isUpdating }
+   )} 
+ />
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 3a59137 and 2bdd42b.

📒 Files selected for processing (1)
  • frontend/src/routes/settings/ModelUpdateContainer.tsx (1 hunks)
🔇 Additional comments (3)
frontend/src/routes/settings/ModelUpdateContainer.tsx (3)

1-1: ¡El comentario de ESLint sigue siendo inaceptable!

El comentario anterior sigue siendo válido. Es completamente inaceptable deshabilitar ESLint de esta manera. Debes resolver los problemas de importación correctamente.


16-20: ¡Los tipos son obligatorios en TypeScript!

El comentario anterior sobre la falta de tipos sigue siendo válido. Es inadmisible tener estados sin tipos explícitos en un proyecto TypeScript.


31-48: 🛠️ Refactor suggestion

¡La estructura del manejador de actualización es deficiente!

El código actual tiene varios problemas graves:

  1. La función updateCWEHandler está innecesariamente anidada
  2. El manejo de errores es básico y usa console.error
  3. No hay tipos para el error

Aplica esta corrección inmediatamente:

- const onClickUpdate = () => {
+ const updateCWEHandler = async () => {
+   try {
+     setIsUpdating(true);
+     await updateCWEModel();
+     toast.success(t('msg.updateCWEModelOk'));
+   } catch (error: unknown) {
+     // Usar un servicio de logging apropiado
+     toast.error(t('err.updateCWEModelFailed'));
+   } finally {
+     setIsUpdateAvailable(false);
+     setIsUpdating(false);
+   }
+ };

+ const onClickUpdate = () => {
    setIsOpenModal(false);
-   const updateCWEHandler = async () => {
-     try {
-       setIsUpdating(true);
-       await updateCWEModel();
-       toast.success(t('msg.updateCWEModelOk'));
-     } catch (err) {
-       console.error(err);
-       toast.error(t('err.updateCWEModelFailed'));
-     } finally {
-       setIsUpdateAvailable(false);
-       setIsUpdating(false);
-     }
-   };
    void updateCWEHandler();
  };

Likely invalid or redundant comment.

Comment thread frontend/src/routes/settings/ModelUpdateContainer.tsx
Comment thread frontend/src/routes/settings/ModelUpdateContainer.tsx
Comment thread frontend/src/routes/settings/ModelUpdateContainer.tsx
jllanosg and others added 2 commits November 3, 2024 19:03
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
coderabbitai[bot]
coderabbitai Bot previously approved these changes Nov 3, 2024
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 8

🧹 Outside diff range and nitpick comments (1)
backend/src/routes/check-cwe-update.js (1)

48-57: Mejora la granularidad del manejo de errores

El manejo actual solo distingue entre errores de timeout y red. Deberías manejar más tipos de errores específicos.

Implementa un manejo de errores más detallado:

  } catch (error) {
    console.error('Error en check-cwe-update:', {
      name: error.name,
      message: error.message,
      stack: error.stack,
    });
-   error.name === 'AbortError'
-     ? Response.Internal(res, { ...timeoutError, details: error.message })
-     : Response.Internal(res, { ...networkError, details: error.message });
+   switch(error.name) {
+     case 'AbortError':
+       return Response.Internal(res, { ...timeoutError, details: error.message });
+     case 'TypeError':
+       return Response.Internal(res, { 
+         error: 'Error de conexión',
+         details: 'No se pudo conectar al servidor CWE'
+       });
+     case 'FetchError':
+       return Response.Internal(res, { ...networkError, details: error.message });
+     default:
+       return Response.Internal(res, {
+         error: 'Error inesperado',
+         details: error.message
+       });
+   }
  }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 1c6866f and 5da145d.

📒 Files selected for processing (4)
  • backend/src/config/config-cwe.json (1 hunks)
  • backend/src/routes/check-cwe-update.js (1 hunks)
  • backend/src/routes/update-cwe-model.js (1 hunks)
  • cwe_api/main.py (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • backend/src/routes/update-cwe-model.js
🔇 Additional comments (2)
backend/src/config/config-cwe.json (1)

4-10: Verificar la documentación de la API

La adición de nuevos endpoints y configuraciones requiere actualización de la documentación de la API.

Ejecutemos una búsqueda para verificar si existe documentación:

#!/bin/bash
# Buscar archivos de documentación de API
echo "Buscando archivos de documentación..."
fd -e md -e yaml -e json . | grep -i "api\|swagger\|openapi"

También sugiero:

  1. Agregar validación de esquema para estos valores de configuración
  2. Considerar hacer estos timeouts configurables por variables de entorno
backend/src/routes/check-cwe-update.js (1)

13-16: ¡Excelente implementación del middleware de autorización!

La ruta está bien definida y el control de acceso está correctamente implementado usando ACL.

Comment thread backend/src/config/config-cwe.json
Comment thread backend/src/config/config-cwe.json
Comment thread backend/src/routes/check-cwe-update.js
Comment thread backend/src/routes/check-cwe-update.js
Comment thread cwe_api/main.py
Comment thread cwe_api/main.py
Comment thread cwe_api/main.py
coderabbitai[bot]
coderabbitai Bot previously approved these changes Nov 3, 2024
@caverav
Copy link
Copy Markdown
Owner

caverav commented Nov 4, 2024

Sin cambiar el set state me salta el modal para la actualización, y por más que le doy que sí, sigue saltando

Comment thread cwe_api/main.py Outdated
coderabbitai[bot]
coderabbitai Bot previously approved these changes Nov 4, 2024
@Icksir
Copy link
Copy Markdown
Collaborator

Icksir commented Nov 4, 2024

Cómo se hace correctamente el testeo? Modifiqué el link de descarga del archivo de CWE a otro archivo en el .env, pero no me saltó ningún pop up indicándome la actualización

@jllanosg
Copy link
Copy Markdown
Collaborator Author

jllanosg commented Nov 4, 2024

Cómo se hace correctamente el testeo? Modifiqué el link de descarga del archivo de CWE a otro archivo en el .env, pero no me saltó ningún pop up indicándome la actualización

image

@jllanosg jllanosg requested a review from caverav November 4, 2024 17:20
@Icksir
Copy link
Copy Markdown
Collaborator

Icksir commented Nov 4, 2024

Me queda duda eso sí respecto cómo evalúan cuál es el último modelo, probé cambiando el link al modelo antiguo y efectivamente se actualizó al actual (al menos así me indicaba). No quiero cambiar el link del modelo actual pa no pitearmelo, pero de qué forma consideran el más nuevo?

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Outside diff range and nitpick comments (1)
frontend/src/services/settings.ts (1)

Line range hint 1-64: ¡La arquitectura necesita una mejor organización!

Todas las llamadas API siguen patrones similares. Deberías considerar:

  1. Crear un cliente API centralizado con interceptores para el manejo de errores
  2. Implementar un sistema de retry para errores temporales
  3. Centralizar los tipos de respuesta en un archivo separado

¿Te gustaría que te ayude a implementar esta arquitectura más robusta?

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 088d435 and 4787b48.

📒 Files selected for processing (2)
  • frontend/src/routes/settings/ModelUpdateContainer.tsx (1 hunks)
  • frontend/src/services/settings.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • frontend/src/routes/settings/ModelUpdateContainer.tsx

Comment thread frontend/src/services/settings.ts
Comment thread frontend/src/services/settings.ts
coderabbitai[bot]
coderabbitai Bot previously approved these changes Nov 4, 2024
@sealra
Copy link
Copy Markdown
Collaborator

sealra commented Nov 4, 2024

Me queda duda eso sí respecto cómo evalúan cuál es el último modelo, probé cambiando el link al modelo antiguo y efectivamente se actualizó al actual (al menos así me indicaba). No quiero cambiar el link del modelo actual pa no pitearmelo, pero de qué forma consideran el más nuevo?

Se considera en base a que supuestamente es una url fija (o al menos en eso se había quedado en las reuniones), entonces siempre el archivo de la url será el 'más' nuevo, mientras que el .zip local será la versión anterior.

@massi-ponce
Copy link
Copy Markdown
Collaborator

Hay algo que me genera un poco de dudas. Y es el hecho de que, al refrescar la página (en cualquier sección dentro de AuditForge), el modal de actualización vuelve a aparecer incluso cuando anteriormente uno clickeó en "Dismiss". ¿Ese comportamiento es el esperado?

Lo contrasto con la situación en la que: el usuario clickee en "Dismiss", aparece el spinner abajo a la derecha, y si refresca la página, se mantiene el spinner (no aparece nuevamente el modal).

@massi-ponce
Copy link
Copy Markdown
Collaborator

massi-ponce commented Nov 5, 2024

Si le doy a actualizar el modelo y luego refresco la página, el spinner de abajo a la derecha desaparece, dejando sin feedback al usuario de lo que está sucediendo (el modelo se sigue actualizando, se canceló la actualización, etc).
Luego, debo volver a refrescar para que nuevamente se abra el modal de actualización.
(esto también pasa si le doy a actualizar, me deslogueo, y luego, vuelvo a loguearme)

Esto me genera la duda de cómo se maneja la actualización "por debajo", ya que no sé si este "bug" aparece solamente por la forma en la que estamos probando el PR o no.

@jllanosg
Copy link
Copy Markdown
Collaborator Author

jllanosg commented Nov 5, 2024

Hay algo que me genera un poco de dudas. Y es el hecho de que, al refrescar la página (en cualquier sección dentro de AuditForge), el modal de actualización vuelve a aparecer incluso cuando anteriormente uno clickeó en "Dismiss". ¿Ese comportamiento es el esperado?

Lo contrasto con la situación en la que: el usuario clickee en "Dismiss", aparece el spinner abajo a la derecha, y si refresca la página, se mantiene el spinner (no aparece nuevamente el modal).

Técnicamente como es una single page application no está considerado el recargar la página, tampoco existe forma actualmente de saber en tiempo real el estado de la actualización. Lo que se espera es que muy de vez en cuando el usuario tenga una actualización disponible y la aplique si es que lo haya necesario. Este es el comportamiento esperado dado el diseño de figma. Es deficiente en el sentido que mencionas? Si, pero para cambiarlo se necesitaría mucho más tiempo del disponible

coderabbitai[bot]
coderabbitai Bot previously approved these changes Nov 5, 2024
massi-ponce
massi-ponce previously approved these changes Nov 5, 2024
Copy link
Copy Markdown
Collaborator

@massi-ponce massi-ponce left a comment

Choose a reason for hiding this comment

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

Considerando que las 2 cosas que comenté ocurren cuando se refresca la página, y que nuestra app actualmente no es completamente una "single page application" (hay secciones en las que hay que refrescar para observar cambios)... LGTM

@jllanosg
Copy link
Copy Markdown
Collaborator Author

jllanosg commented Nov 5, 2024

Considerando que las 2 cosas que comenté ocurren cuando se refresca la página, y que nuestra app actualmente no es completamente una "single page application" (hay secciones en las que hay que refrescar para observar cambios)... LGTM

Que haya secciones hechas a medias no cambia el hecho de que nuestra app esté planteada como una SPA.

@massi-ponce
Copy link
Copy Markdown
Collaborator

Considerando que las 2 cosas que comenté ocurren cuando se refresca la página, y que nuestra app actualmente no es completamente una "single page application" (hay secciones en las que hay que refrescar para observar cambios)... LGTM

Que haya secciones hechas a medias no cambia el hecho de que nuestra app esté planteada como una SPA.

Nunca dije que nuestra app no estuviera planteada como una SPA. Solamente aclaré que las cosas que pillé ocurren solamente cuando se refresca la página y que, actualmente en la app, hay secciones que requieren refrescar la página. No le veo lo malo(?

Icksir
Icksir previously approved these changes Nov 6, 2024
Copy link
Copy Markdown
Collaborator

@Icksir Icksir left a comment

Choose a reason for hiding this comment

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

Probé el flujo con el modelo antiguo, y lo actualiza al modelo actual mostrando pop up e ícono de actualización (ta bonito).
Lo comprobé checkeando los archivos dentro del contenedor auditforge-cwe-api para que efectivamente se cambien los archivos. Pasa de la carpeta checkpoint-20790 (modelo antiguo) a checkpoint-141693 (modelo nuevo) con los respectivos archivos modificados (comprobado mediante lectura de trainer_state.json que indica el log de entrenamiento).

@jllanosg
Copy link
Copy Markdown
Collaborator Author

jllanosg commented Nov 6, 2024

Considerando que las 2 cosas que comenté ocurren cuando se refresca la página, y que nuestra app actualmente no es completamente una "single page application" (hay secciones en las que hay que refrescar para observar cambios)... LGTM

Que haya secciones hechas a medias no cambia el hecho de que nuestra app esté planteada como una SPA.

Nunca dije que nuestra app no estuviera planteada como una SPA. Solamente aclaré que las cosas que pillé ocurren solamente cuando se refresca la página y que, actualmente en la app, hay secciones que requieren refrescar la página. No le veo lo malo(?

El tema es que no debería ser así

@caverav caverav merged commit 1a1ef20 into development Nov 6, 2024
@caverav caverav deleted the feature/check-cwe-update branch November 6, 2024 14:33
@coderabbitai coderabbitai Bot mentioned this pull request Feb 1, 2025
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants