TP · Cierre del Entregable 3: visor rediseñado, capturas regenerables y PRs del grupo en el informe - #50
Merged
Conversation
…ables - Colores de los 8 arquetipos con la paleta de Bang Wong (segura para daltonismo) en el agregador de Go y en los JSON del visor; fuera los emojis, cada arquetipo se identifica por su punto de color. - Paneles colapsables, modo mapa limpio y deep links (?hour, ?day, ?zone, ?filter, ?min) en tp/app. - En pantallas angostas, con un filtro activo, los chips de la barra superior quedan solo con su ícono: a 412 px «Arquetipos» se salía de la pantalla.
Las dos figuras de fig:app-movil eran del diseño anterior. tp/scripts/ capturas_app.py las rehace desde los deep links del visor (Midtown a las 12:00 con el detalle expandido; Cluster 5 aislado a las 15:00), así que el pie de la figura describe exactamente el estado capturado y se pueden volver a generar.
…nforme del TP Cierra los \pendiente de la tabla de pull requests y de la participación del Entregable 3 con lo que trae cada PR, y regenera el historial de commits.
Reviewer's GuideEl PR cierra el Entregable 3 rediseñando el visor para accesibilidad y móviles, incorporando estados reproducibles mediante deep links y un generador Playwright de capturas, y sincronizando el informe con las PRs, participaciones e historial actualizados. Sequence diagram for deep-link mobile screenshot generationsequenceDiagram
participant Script as capturas_app.py
participant Server as HTTP server
participant Browser as Playwright browser
participant App as NYC Taxi Pulse app
participant Map as Leaflet map
Script->>Server: serve_forever()
Script->>Browser: launch()
Script->>Browser: new_context()
Browser->>App: goto(index.html?hour&day&zone&filter&min)
App->>Map: initMap()
App->>App: toggleCleanMapMode()
Browser->>App: click(sheetHandle)
Browser->>Browser: screenshot()
Browser-->>Script: screenshot file
Flow diagram for responsive map and panel controlsflowchart TD
Start[User opens visor] --> Query{Deep link contains min?}
Query -->|Yes| Minimized[Add minimized to temporalControls]
Query -->|No| Expanded[Show temporal controls]
Minimized --> Panels{User toggles panels}
Expanded --> Panels
Panels -->|Hide| Clean[Add clean-map-mode]
Clean --> Restore[Show restore control]
Panels -->|Restore| Visible[Show HUD and action deck]
Restore --> Visible
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
R0SEWT
added a commit
that referenced
this pull request
Oct 2, 2026
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 6 security issues, and 2 other issues
Security issues:
- User controlled data in methods like
innerHTML,outerHTMLordocument.writeis an anti-pattern that can lead to XSS vulnerabilities (link) - User controlled data in a
archetypeDesc.innerHTMLis an anti-pattern that can lead to XSS vulnerabilities (link) - User controlled data in methods like
innerHTML,outerHTMLordocument.writeis an anti-pattern that can lead to XSS vulnerabilities (link) - User controlled data in a
legItem.innerHTMLis an anti-pattern that can lead to XSS vulnerabilities (link) - User controlled data in methods like
innerHTML,outerHTMLordocument.writeis an anti-pattern that can lead to XSS vulnerabilities (link) - User controlled data in a
card.innerHTMLis an anti-pattern that can lead to XSS vulnerabilities (link)
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="tp/scripts/capturas_app.py" line_range="63" />
<code_context>
+ pag.wait_for_timeout(900)
+ if errores:
+ raise SystemExit(f"{nombre}: errores de JavaScript: {errores}")
+ pag.screenshot(path=args.salida / nombre)
+ print(f"→ {args.salida / nombre}")
+ nav.close()
</code_context>
<issue_to_address>
**nitpick (bug_risk):** `pag.screenshot(path=args.salida / nombre)` raises an error when the output directory does not already exist, so the documented `--salida` option cannot write captures to a new directory.
**Triggers:** When the caller passes a new or otherwise nonexistent directory via `--salida`.
**Suggested fix:** Create the destination before the capture loop with `args.salida.mkdir(parents=True, exist_ok=True)`.
```suggestion
args.salida.mkdir(parents=True, exist_ok=True)
for nombre, (query, expandir) in VISTAS.items():
```
</issue_to_address>
### Comment 2
<location path="tp/scripts/capturas_app.py" line_range="47-49" />
<code_context>
+ pass
+
+ manejador = functools.partial(Silencioso, directory=TP / "app")
+ servidor = http.server.ThreadingHTTPServer(("127.0.0.1", 0), manejador)
+ threading.Thread(target=servidor.serve_forever, daemon=True).start()
+ base = f"http://127.0.0.1:{servidor.server_port}/index.html"
+
+ with sync_playwright() as pw:
</code_context>
<issue_to_address>
**nitpick (bug_risk):** The HTTP server is never shut down when browser launch, navigation, screenshot, or JavaScript-error handling raises; the cleanup call is reached only on the successful path, leaving the server thread and listening socket alive until interpreter termination.
**Triggers:** When Playwright startup or any capture operation fails before the final `servidor.shutdown()` call.
**Suggested fix:** Wrap the server and Playwright work in `try/finally` and call `servidor.shutdown()` (and close the browser/context) from the `finally` block.
</issue_to_address>
### Comment 3
<location path="tp/app/app.js" line_range="581-584" />
<code_context>
archetypeDesc.innerHTML = `
<strong>${cluster.nombre}:</strong> ${cluster.subtitulo}.${notaMuestra}<br>
${cluster.desc}
`;
</code_context>
<issue_to_address>
**security (javascript.browser.security.insecure-document-method):** User controlled data in methods like `innerHTML`, `outerHTML` or `document.write` is an anti-pattern that can lead to XSS vulnerabilities
*Source: opengrep*
</issue_to_address>
### Comment 4
<location path="tp/app/app.js" line_range="581-584" />
<code_context>
archetypeDesc.innerHTML = `
<strong>${cluster.nombre}:</strong> ${cluster.subtitulo}.${notaMuestra}<br>
${cluster.desc}
`;
</code_context>
<issue_to_address>
**security (javascript.browser.security.insecure-innerhtml):** User controlled data in a `archetypeDesc.innerHTML` is an anti-pattern that can lead to XSS vulnerabilities
*Source: opengrep*
</issue_to_address>
### Comment 5
<location path="tp/app/app.js" line_range="633-636" />
<code_context>
legItem.innerHTML = `
<span class="dist-color-box" style="background: ${cMeta.color};"></span>
<span>${cMeta.nombre} (${Math.round(pct)}%)</span>
`;
</code_context>
<issue_to_address>
**security (javascript.browser.security.insecure-document-method):** User controlled data in methods like `innerHTML`, `outerHTML` or `document.write` is an anti-pattern that can lead to XSS vulnerabilities
*Source: opengrep*
</issue_to_address>
### Comment 6
<location path="tp/app/app.js" line_range="633-636" />
<code_context>
legItem.innerHTML = `
<span class="dist-color-box" style="background: ${cMeta.color};"></span>
<span>${cMeta.nombre} (${Math.round(pct)}%)</span>
`;
</code_context>
<issue_to_address>
**security (javascript.browser.security.insecure-innerhtml):** User controlled data in a `legItem.innerHTML` is an anti-pattern that can lead to XSS vulnerabilities
*Source: opengrep*
</issue_to_address>
### Comment 7
<location path="tp/app/app.js" line_range="926-942" />
<code_context>
card.innerHTML = `
<div class="arch-header">
<div class="arch-title-group">
<span class="arch-color-badge" style="background: ${c.color};"></span>
<span class="arch-name">${c.nombre}</span>
</div>
<span class="arch-pct">${c.porcentaje}% (${c.viajes_total.toLocaleString()})</span>
</div>
<p class="arch-desc">${c.desc}</p>
<div class="arch-metrics">
<span>Duración: ~${c.duracion_min} min</span>
<span>Distancia: ~${c.distancia_mi} mi</span>
<span>Pico: ~${Math.round(c.hora_pico)}:00 (${c.dia_nombre})</span>
</div>
<div style="font-size: 10.5px; color: ${isSelected ? '#38BDF8' : '#6B7280'}; margin-top: 4px; font-weight: 600;">
${isSelected ? 'Activo en el mapa (toca para quitar)' : 'Toca para aislar en el mapa'}
</div>
`;
</code_context>
<issue_to_address>
**security (javascript.browser.security.insecure-document-method):** User controlled data in methods like `innerHTML`, `outerHTML` or `document.write` is an anti-pattern that can lead to XSS vulnerabilities
*Source: opengrep*
</issue_to_address>
### Comment 8
<location path="tp/app/app.js" line_range="926-942" />
<code_context>
card.innerHTML = `
<div class="arch-header">
<div class="arch-title-group">
<span class="arch-color-badge" style="background: ${c.color};"></span>
<span class="arch-name">${c.nombre}</span>
</div>
<span class="arch-pct">${c.porcentaje}% (${c.viajes_total.toLocaleString()})</span>
</div>
<p class="arch-desc">${c.desc}</p>
<div class="arch-metrics">
<span>Duración: ~${c.duracion_min} min</span>
<span>Distancia: ~${c.distancia_mi} mi</span>
<span>Pico: ~${Math.round(c.hora_pico)}:00 (${c.dia_nombre})</span>
</div>
<div style="font-size: 10.5px; color: ${isSelected ? '#38BDF8' : '#6B7280'}; margin-top: 4px; font-weight: 600;">
${isSelected ? 'Activo en el mapa (toca para quitar)' : 'Toca para aislar en el mapa'}
</div>
`;
</code_context>
<issue_to_address>
**security (javascript.browser.security.insecure-innerhtml):** User controlled data in a `card.innerHTML` is an anti-pattern that can lead to XSS vulnerabilities
*Source: opengrep*
</issue_to_address>Sourcery assessment
Approval pending. 6 findings to address first.
Blocking findings: tp/app/app.js:584, tp/app/app.js:584, tp/app/app.js:636, tp/app/app.js:636, tp/app/app.js:942, and 1 more
…apturas Respuesta a la revisión de la PR #50: - app.js: los textos del JSON que entran por innerHTML pasan por esc() y los colores por colorSeguro() (#RRGGBB). No son datos del usuario, pero los nombres ya traen '&' y una descripción '<8 mph', que se insertaban como HTML. - capturas_app.py: crea el directorio de --salida y apaga el servidor en un finally aunque falle el navegador o una captura.
R0SEWT
added a commit
that referenced
this pull request
Oct 2, 2026
…apturas Respuesta a la revisión de la PR #50: - app.js: los textos del JSON que entran por innerHTML pasan por esc() y los colores por colorSeguro() (#RRGGBB). No son datos del usuario, pero los nombres ya traen '&' y una descripción '<8 mph', que se insertaban como HTML. - capturas_app.py: crea el directorio de --salida y apaga el servidor en un finally aunque falle el navegador o una captura. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Owner
Author
|
Respondí los 14 comentarios de la revisión: los 8 hallazgos se resolvieron en 6455e40. Una corrección a la guía del revisor: la tabla de cambios menciona |
R0SEWT
force-pushed
the
feature/tp-cierre
branch
from
October 2, 2026 03:26
d7ca47a to
18bef1b
Compare
… interpolado Sourcery (opengrep) seguía marcando los cuatro innerHTML aunque los valores ya pasaban por esc(): la regla mira el patrón, no el escape. Con el() los textos del JSON entran como texto y los colores por style.background validado.
R0SEWT
commented
Oct 2, 2026
R0SEWT
left a comment
R0SEWT
left a comment
Owner
Author
There was a problem hiding this comment.
🤖 Revisión semanal
Lista para merge
Repasé el diff completo (app.js/index.html/style.css, el script nuevo capturas_app.py, y los cambios en tp/kmeans/cmd/resumen_zonas/main.go). go test -race ./... pasa en tp/kmeans y ruff check no marca nada en capturas_app.py. No encontré bugs ni tests rotos. Un único hallazgo menor, no bloqueante:
tp/app/app.js:297,537,585,975— estas cuatro asignaciones a*.style.backgroundColorusancluster.color/c.colordirecto, sin pasar porcolorSeguro(), mientras que las deapp.js:656yapp.js:948sí la usan. El comentario enapp.js:15("Los colores van a style.background: solo se acepta #RRGGBB") sugiere que la regla debería aplicarse en todos los puntos. No es explotable (una asignación astyle.backgroundColorno interpreta HTML, el navegador simplemente ignora un valor inválido), pero para ser consistentes con la regla que el propio código declara, conviene envolver esos cuatro casos también encolorSeguro(...).
Generated by Claude Code
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Qué trae
tp/app): paleta de Bang Wong (segura para daltonismo) en los 8 arquetipos, sin emojis, paneles colapsables y deep links (?hour,?day,?zone,?filter,?min). A 412 px con un filtro activo los chips de la barra superior quedan solo con su ícono (antes «Arquetipos» se salía de la pantalla).fig:app-movil): regeneradas contp/scripts/capturas_app.py(Playwright) desde los deep links, así que se pueden rehacer y el pie describe exactamente el estado capturado.Verificación
go test -race ./...entp/kmeans: pasa.ruff check/ruff format --checksobre el script nuevo: limpio.tp/informe/tp/compilar.sh: compila, 79 páginas.Pendiente fuera de esta PR (bloquea
empaquetar-tp.sh)\verificarde Rody enconclusiones/rody.texy en el Anexo C.participacion-tp/main.tex.release/tp→mainy tagtpantes del 2026-10-05 09:00.