From 9277143822e77b0dd6d11f586592fd070522f23d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marcos=20Ben=C3=ADtez?= Date: Tue, 7 Jul 2026 17:29:48 -0300 Subject: [PATCH] =?UTF-8?q?docs(sprint-7):=20cierre=20formal=20=E2=80=94?= =?UTF-8?q?=20TECH=5FDEBT=20guards+auth,=20OBSERVATIONS=20reset=20Sprint?= =?UTF-8?q?=208,=20decisi=C3=B3n=20org=E2=86=92areas?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- OBSERVATIONS.md | 110 ++++++++++++++++++------------------------------ TECH_DEBT.md | 19 +++++++++ 2 files changed, 60 insertions(+), 69 deletions(-) diff --git a/OBSERVATIONS.md b/OBSERVATIONS.md index b0635b2..81b72be 100644 --- a/OBSERVATIONS.md +++ b/OBSERVATIONS.md @@ -1,9 +1,9 @@ -# OBSERVATIONS — Sprint 7 +# OBSERVATIONS — Sprint 8 > Hallazgos descubiertos durante el sprint actual. > Este archivo se vacía al cerrar el sprint: los hallazgos se promocionan a TECH_DEBT.md o BACKLOG.md, o quedan resueltos. > -> **Sprint 6 cerrado el 2026-06-24.** Observaciones archivadas — ítems migrados a BACKLOG.md y TECH_DEBT.md. +> **Sprint 7 cerrado el 2026-07-07.** Ver reflexiones completas en `STRATEGIC_DIRECTION.md`. --- @@ -24,75 +24,13 @@ ## Hallazgos resueltos durante el sprint -### [RESOLVED] Etapa 1 — Migraciones DB + SECURITY DEFINER helpers - -**Policy `activity_resource_assignments`:** el BRIEF asumía `"all_activity_resource_assignments"` como nombre de la policy. La migration 007 la creó como `"all_assignments"`. Claude Code auditó las migraciones 001-013 antes de escribir los DROP POLICY y corrigió el nombre. Sin la corrección, el DROP habría pasado silenciosamente (IF EXISTS) sin eliminar la policy permisiva original, manteniendo acceso sin restricción de rol. - -**Tabla `resources` sin `process_id`:** el brief del ETAPA_1_PROMPT incluía una policy `resources_select` con condición `resources.process_id IS NULL`. La columna no existe desde Sprint 3 (catálogo global). Claude Code detectó la discrepancia y adaptó la policy a `auth.uid() IS NOT NULL`, equivalente funcional para catálogo sin filtro de proceso. - -**Auto-migración `client` → `org_id`:** en el entorno de dev, los 158 procesos existentes no tienen valor en el campo `client` → 0 procesos vinculados a org. Esto es esperado: el campo `client` es opcional y en dev no está poblado con clientes reales. En producción, si hay procesos con `client` no vacío, la auto-migración los vinculará automáticamente al primer deploy. - -**OK formal del director:** 2026-07-06. - ---- - -### [RESOLVED] Etapa 2 — Edge Functions invite-user y revoke-user - -Funciones creadas en `supabase/functions/`. El diseño simplificó el scope del BRIEF (que pedía `delete-user` + `suspend-user` + `unsuspend-user` separadas) a `invite-user` + `revoke-user` (soft suspension a 'guest'). Las limitaciones quedaron documentadas en `UsersPage.tsx`: -- "Eliminar" llama `revoke-user` (baja a guest) — eliminación real requiere función futura -- "Reactivar" restaura siempre a 'client_editor' — rol previo no se guarda -**OK formal implícito del director:** 2026-07-06. - ---- - -### [RESOLVED] Etapa 3 — Auth layer: fix rol desde DB + email/password + reset-password - -Fix crítico: `syncProfileFromDB` ahora lee `platform_role` y `org_id` desde DB en lugar de inferir el rol del dominio del email. `AuthUser` tiene `orgId?: string`. `signInWithEmailPassword` implementado. `TOKEN_REFRESHED` no dispara loading. `LoginPage` con form email/password. `ResetPasswordPage` en `/reset-password`. 558 tests (+ 6 nuevos). - -**Deuda identificada:** `PASSWORD_RECOVERY` maneja vía segunda suscripción a `onAuthStateChange` (evitar importación circular). No es bug, pero el patrón de doble suscripción es frágil. Ver ítem en TECH_DEBT.md. -**OK formal del director:** 2026-07-06. - ---- - -### [RESOLVED] Etapa 4 — Panel de administración - -`/admin/organizations`, `/admin/organizations/$orgId`, `/admin/users` con guard de `platform_admin`. Invitación de usuarios via Edge Function `invite-user`. Suspensión/reactivación via `revoke-user`. Skeletons durante carga. Badge "Tú" en fila propia. Acciones deshabilitadas para self-action. RoleBadge con colores por rol. - -**Deuda identificada:** guard implementado en `AdminLayout` como componente (no en `beforeLoad` de TanStack Router) — puede haber flash breve del contenido antes del redirect. Ver ítem recurrente en TECH_DEBT.md. -**OK formal del director:** 2026-07-06 (validación visual con DOM). - ---- - -### [RESOLVED] Etapa 5 — UX de roles de cliente y routing - -Guards de rol en `WorkspacePage` e `ImportPage` vía `useEffect`. `client_viewer` redirigido a `/report/$processId` al entrar a workspace. `client_editor` bloqueado de `/import`. Botón "Importar BPMN" oculto para roles de cliente. Botón "Nuevo proceso" oculto para roles de cliente. Panel de grupos oculto para roles de cliente (lista plana). `ReportPage` con `defaultTab = 'roi'` para `client_viewer`. 578 tests (+ 20 nuevos). - -**Bug encontrado en validación:** `group_label = "un Cliente"` guardado como dato corrupto en Supabase → `"2 un clientes"` en UI. Causa: el campo era editable desde Settings y en algún momento se guardó con el artículo. Fix: `normalizeGroupLabel()` en `settings-repo.ts` que limpia artículos iniciales (`un`, `una`, `el`, `la`) al leer y al guardar. Solución transparente sin migración de datos. - -**Deuda identificada (BACKLOG):** Settings page (ícono de tuerca) accesible para `client_editor`. La página es cosmética (solo label de grupos), pero no debería ser visible para roles de cliente. Ítem agregado en BACKLOG.md. - -**Patrón recurrente identificado:** guards de rol implementados tres veces via `useEffect` en lugar de `beforeLoad` de TanStack Router (AdminLayout, WorkspacePage, ImportPage). Funcional pero subóptimo — posible flash antes del redirect. Candidato a TECH_DEBT sprint siguiente. - -**OK formal del director:** 2026-07-06 (validación visual con cambio temporal de rol en Supabase Studio). +*(vacío — sprint recién abierto)* --- ## Hallazgos diferidos (promocionar al cerrar sprint) -### [DEFERRED-TECH-DEBT] Guards de rol via `useEffect` en lugar de `beforeLoad` (3 componentes) - -`AdminLayout`, `WorkspacePage`, `ImportPage` — los tres usan `useEffect` para detectar el rol y redirigir. Con `beforeLoad` de TanStack Router, la ruta nunca monta para usuarios sin acceso. Con `useEffect`, el componente monta brevemente antes del redirect (posible flash). No es bug de seguridad (RLS protege los datos), pero es anti-patrón UX y arquitectónico para TanStack Router. -**Migrar a:** TECH_DEBT.md al cerrar Sprint 7. - -### [DEFERRED-TECH-DEBT] Doble suscripción `onAuthStateChange` en AuthContext - -AuthContext tiene dos listeners de `onAuthStateChange`: uno principal (SIGNED_IN, SIGNED_OUT, TOKEN_REFRESHED) y uno secundario para `PASSWORD_RECOVERY`. Fue implementado para evitar una importación circular. El patrón correcto es un único listener con switch/case completo. Refactorizar cuando se resuelva la dependencia circular. -**Migrar a:** TECH_DEBT.md al cerrar Sprint 7. - -### [DEFERRED-BACKLOG] Settings page accesible para client_editor/client_viewer - -El ícono de configuración (tuerca) en LibraryPage y la ruta `/settings` (si existe) son accesibles para todos los roles. La página solo configura el label de grupos (preferencia cosmética), pero un usuario cliente no debería verla. Ocultar el ícono de configuración y agregar guard de rol en la ruta. -**Migrar a:** BACKLOG.md al cerrar Sprint 7 — ya agregado por Claude Code. +*(vacío)* --- @@ -107,9 +45,43 @@ El ícono de configuración (tuerca) en LibraryPage y la ruta `/settings` (si ex ## Cierre del sprint (checklist) ``` -[x] Verificar que todos los items [CURRENT-SPRINT] están RESOLVED -[x] Mover items [DEFERRED-TECH-DEBT] a TECH_DEBT.md (pendiente commit) -[x] Mover items [DEFERRED-BACKLOG] a BACKLOG.md (ya agregado por Claude Code en Etapa 5) +[ ] Verificar que todos los items [CURRENT-SPRINT] están RESOLVED +[ ] Mover items [DEFERRED-TECH-DEBT] a TECH_DEBT.md +[ ] Mover items [DEFERRED-BACKLOG] a BACKLOG.md [ ] Archivar o purgar este archivo [ ] Crear OBSERVATIONS.md vacío para el sprint siguiente ``` + +--- + +## Aprendizajes de Sprint 7 relevantes para Sprint 8 + +> Cosecha de método — decisiones y patterns emergentes de Sprint 7 que informan cómo trabajar en Sprint 8. + +### Race condition en roles: `isProfileLoaded` como guard + +`buildAuthUser()` asigna `platformRole: 'guest'` heurísticamente (por dominio de email) antes de que `syncProfileFromDB` complete. El rol correcto llega ~200ms después. Cualquier UI condicionada por rol que renderice antes de ese sync muestra el estado incorrecto. + +**Patrón validado:** `isProfileLoaded: boolean` en AuthContext — se pone en `true` solo después de que `syncProfileFromDB` completa. Componentes que dependen del rol deben esperar `isProfileLoaded` antes de renderizar contenido rol-dependiente. + +**Aprendizaje para Sprint 8:** cualquier componente nuevo que condicione su contenido por `platformRole` debe incluir `isProfileLoaded` como prerequisito en su spec de implementación. + +### JOIN vs fetch separado: orgName en getById + +Fetch separado en `useEffect` para obtener el nombre de la org en `WorkspacePage` fue frágil: race conditions entre `currentProcess` cargando y el efecto, resets de estado al desmontar, y respuesta en array cuando `.maybeSingle()` esperaba objeto. El síntoma (botón mostrando "Biblioteca" en lugar de "Solar Banco") requirió ~8 iteraciones de diagnóstico. + +**Fix definitivo:** JOIN en `getById()` con `select('*, org:organizations!org_id(name)')`. El dato llega embebido en el proceso — sin async adicional, sin estado local, sin race condition. + +**Aprendizaje para Sprint 8:** cuando un componente necesita datos relacionados de otra tabla para display, preferir JOIN en el repositorio sobre fetch separado en el componente. Los fetches separados en `useEffect` con datos del store como dependencia son estructuralmente frágiles. + +### Dos sistemas paralelos de "cliente" → consolidar en Sprint 8 + +`process_groups`, `clientName` (texto libre) y `org_id` intentaban modelar el mismo concepto ("a qué cliente pertenece el proceso") desde tres ángulos distintos. La confusión generó bugs de UX (back button, agrupación en biblioteca) y deuda conceptual. + +**Decisión tomada:** Sprint 8 consolida en `organizations → areas → processes`. Ver `STRATEGIC_DIRECTION.md` → "Consolidación del modelo de cliente". + +### Diagnóstico por DOM observable > teorización de librerías + +El bug del freeze de perfil (Sprint 5, confirmado resuelto en Sprint 6) y el bug del back button (Sprint 7) compartieron el mismo patrón: múltiples iteraciones de diagnóstico teórico sin resolver → un dato observable del runtime (HTML del DOM, Network tab, respuesta de request) colapsó el espacio de hipótesis en 1 paso. + +**Aprendizaje consolidado:** ante bug de UI que persiste 2+ fixes teóricos, pedir el dato observable antes de seguir. Específicamente: HTML del elemento, Network tab con el request real, o `console.log` del valor en runtime. diff --git a/TECH_DEBT.md b/TECH_DEBT.md index d66fe3f..0453bf1 100644 --- a/TECH_DEBT.md +++ b/TECH_DEBT.md @@ -67,6 +67,25 @@ documentación. reload completo de página el valor guardado se relee correctamente desde la DB. **Sprint cerrado en:** Sprint 6 Etapa 3. +### [BACKLOG] Guards de rol via `useEffect` en lugar de `beforeLoad` (3 componentes) + +**Estado actual:** `AdminLayout`, `WorkspacePage`, e `ImportPage` usan `useEffect` para detectar el rol y redirigir. El componente monta brevemente antes del redirect — posible flash de contenido. +**Causa raíz:** el pattern surgió de prompts que describieron el guard en términos de comportamiento de componente ("si el rol no es X, redirigir") en lugar de especificar la sección de router. +**Impacto:** no es bug de seguridad (RLS protege los datos en Supabase), pero es anti-patrón para TanStack Router y puede causar flash visual breve en conexiones lentas. +**Solución propuesta:** migrar a `beforeLoad` de TanStack Router. Ejemplo: `beforeLoad: ({ context }) => { if (context.auth.user?.platformRole !== 'platform_admin') throw redirect({ to: '/library' }) }`. +**Archivos afectados:** `src/features/admin/AdminLayout.tsx`, `src/features/workspace/WorkspacePage.tsx`, `src/features/import/ImportPage.tsx` (aprox. línea 65-70 en WorkspacePage). +**Aprendizaje de método:** en prompts futuros que requieran guards de rol, especificar siempre el código exacto de `beforeLoad` en la sección de router — no solo el comportamiento esperado del componente. +**Origen:** Sprint 7 Etapa 5 — patrón recurrente detectado en 3 componentes. + +### [BACKLOG] Doble suscripción `onAuthStateChange` en AuthContext + +**Estado actual:** `AuthContext.tsx` tiene dos listeners de `onAuthStateChange`: uno principal (SIGNED_IN, SIGNED_OUT, TOKEN_REFRESHED, INITIAL_SESSION) y uno secundario para `PASSWORD_RECOVERY`. El secundario fue implementado para evitar importación circular entre `AuthContext` y `SupabaseAuthService`. +**Causa raíz:** `PASSWORD_RECOVERY` necesita llamar a `supabase.auth.updateUser()` directamente desde el contexto — si se delega a `SupabaseAuthService`, este importa desde `AuthContext` creando un ciclo. +**Impacto:** ambos listeners se registran en cada mount del `AuthProvider`. En la mayoría de los casos no genera bugs, pero el patrón es frágil y puede causar doble-dispatch de eventos en casos edge de remount. +**Solución propuesta:** refactorizar para un único switch/case en el listener principal. Resolver la importación circular moviendo `supabase` client a un módulo compartido sin dependencia de AuthContext. +**Archivos afectados:** `src/auth/AuthContext.tsx`. +**Origen:** Sprint 7 Etapa 3 — documentado en OBSERVATIONS.md. + ### [BACKLOG] `getCurrentUser()` en SupabaseAuthService — sin callers activos **Estado actual:** `SupabaseAuthService.getCurrentUser()` usa `getSession()` directamente (sin timeout, sin processLock workaround). Es la única excepción documentada a la regla — justificada porque necesita el objeto sesión completo, no solo una barrera. Sin callers activos hoy. **Impacto si no se resuelve:** ninguno mientras no tenga callers. Si en el futuro algún flujo llama `getCurrentUser()`, podría colgar el processLock si el servidor no responde.