-
-
Notifications
You must be signed in to change notification settings - Fork 8
Gianfranco mastogiovanni challenge #3
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
base: main
Are you sure you want to change the base?
Conversation
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.
Hola Gianfranco! Soy Joaco de NaNLABS. Hoy voy a estar revisando tu ejercicio y espero darte feedback que te sirva.
Me gustó que hayas seguido las indicaciones y que hayas dejado un README legible con instrucciones para levantar el proyecto. También hubiera sumado una descripción en el PR.
Mi percepción general es que el backend está súper prolijo y minimalista, en cambio el frontend tiene componentes gigantes que pueden ser difíciles de seguir.
Lo levanté localmente. Las instrucciones fueron claras y salió andando de una 🔥 .
La app parece funcionar perfecto, se navega bastante bien y es responsiva.
Lo único que cambiaría de ahí es la legibilidad del texto, y que cuando abras los modals no se pierda el contexto de lo que hay detrás.
Muy bueno que todos los prompts funcionen, y que puedas cambiar entre versiones del modelo (un nice to have podría ser meter Anthropic o algun otro modelo para comparar resultados). Me divertí bastante probando casos de contenido, traduciendo, regenerando y probando los distintos estados. 🎊
Te dejo algunos comentarios con mejoras y cositas que detecté. No espero que cambies nada pero sentite libre de hacerlo. El ejercicio cumple los requerimientos.
También te dejo unas preguntas, solo de curiosidad:
- Usaste herramientas de GenAI durante este ejercicio? Cuáles y por qué?
- Alguna configuración específica o curiosidad de la misma que nos quieras compartir?
Muchas gracias por el tiempo y dedicación! 💯
|
||
return { | ||
socket: socketRef.current, | ||
isConnected: isConnectedRef.current, |
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.
Acá intentaría usar un useState
para definir isConnected
. useRef
no es reactivo y puede generar que nuestros componentes no se re-rendericen cuando lo esperamos. En principio, esto afectaría a NotificationProvider
y a useRealTimeUpdates
const [isConnected, setIsConnected] = useState(false);
s.on('connect', () => setIsConnected(true));
s.on('disconnect', () => setIsConnected(false));
isConnectedRef.current = true; | ||
|
||
// Join user-specific room | ||
socket.emit('join', { userId }); |
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.
Hacer el decode del token y meter al usuario en el room correcto es, idealmente, responsabilidad del server. Por ejemplo podríamos hacer algo como esto:
// client
socketRef.current = io(BACKEND_URL, {
withCredentials: true,
auth: { token: Cookies.get('token') },
});
// server (pseudo)
io.use((socket, next) => {
const token = socket.handshake.auth?.token;
const user = verifyJwt(token);
socket.data.userId = user.id;
next();
}).on('connection', (socket) => {
socket.join(`user:${socket.data.userId}`);
});
|
||
interface WebSocketEvents { | ||
// Campaign events | ||
'campaign:created': (data: any) => void; |
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.
Acá tenemos muchos any
. Incluso quitar esta interface podría ser una opción. La mejora ideal sería crear algunos types más a partir de los que ya tenes definidos en types/index.ts
.
Ejemplo:
type Campaign = { id: string; name: string; status?: CampaignStatus };
type Content = { id: string; title: string; campaignId?: string; status?: ContentStatus};
interface WebSocketEvents {
'campaign:created': (data: Campaign) => void;
'campaign:deleted': (data: { id: string }) => void;
'content:created': (data: Content) => void;
// ...
}
}; | ||
|
||
// Check token every 5 seconds | ||
const interval = setInterval(checkToken, 5000); |
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.
El setInterval
que chequea el token cada 5s es costoso y puede generar reconexiones innecesarias (incluso con autoConnect=false). Estaría bueno disparar la lógica de login/logout desde el AuthContext
en lugar de hacer polling.
'use client'; | ||
|
||
import { useState } from 'react'; | ||
import { CheckIcon, XMarkIcon } from '@heroicons/react/24/outline'; |
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.
Me gustó esta lib de iconos. Jamás la había visto. Es buena opción?
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.
La use en un proyecto anterior y me parecieron lindos los iconos jeje. Como curiosidad, están creados por el equipo de tailwind.
// This is your Prisma schema file, | ||
// learn more about it in the docs: https://pris.ly/d/prisma-schema | ||
|
||
generator client { |
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.
Me gusta el uso de Prisma. Toda la implementación quedó prolija y está bueno que hayas sido estricto en documentar cada migración, es muy útil para trabajo colaborativo. Para un solo PR quizás son muchas migraciones, pero eso es tema aparte y lo menciono de quisquilloso nomás.
@@ -0,0 +1,756 @@ | |||
'use client'; | |||
|
|||
import { useEffect, useState, useCallback } from 'react'; |
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.
Esta página tiene ~750 líneas y mezcla lógica de datos, estado de UI y vistas. Para mejorar legibilidad y mantenimiento, propongo:
- extraer componentes (Card de contenido, toolbars y cada Modal),
- mover la lógica de fetch/RTU a un hook (useCampaignDetail),
- centralizar acciones (crear/editar/borrar/cambiar estado) en un módulo de actions o en apiClient,
- derivar estado en lugar de duplicarlo (hay dos flujos de “delete” y dos modales de confirmación).
Este tipo de cambios facilita tanto la revisión en una instancia como esta, como la mantenibilidad a futuro.
[ContentStatus.DRAFT]: [ContentStatus.AI_GENERATED, ContentStatus.APPROVED], | ||
[ContentStatus.AI_GENERATED]: [ContentStatus.DRAFT, ContentStatus.APPROVED, ContentStatus.REJECTED], | ||
[ContentStatus.REJECTED]: [ContentStatus.DRAFT], | ||
[ContentStatus.APPROVED]: [], // Final state, no transitions allowed |
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.
@@ -0,0 +1,150 @@ | |||
# 🔗 Sistema de WebSockets en Tiempo Real |
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.
Idealmente este archivo estaría en una carpeta /docs
en el root.
Muchas gracias Joaquín por tomarte el tiempo de dejarme este feedback, es muy útil. En cuanto a uso de IA, usé vscode con copilot, principalmente en el front. Empecé por el back, y cuando tocó la parte del front, me estaba quedando sin tiempo para entregar el challenge. Empece a hacerlo el finde, antes no pude. Quisiera agregar que me pareció muy interesante el challenge. Me quedé con las ganas de implementar langchain. Estamos en contacto. Saludos! |
Description
Please include a summary of the changes and the related issue. List any dependencies that are required for this change.
Fixes # (issue)
Type of Change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce.
Checklist