Antes de abrir el diff

Lo primero que hay que hacer antes de revisar línea por línea es entender qué hace el PR. Leer la descripción, el ticket vinculado, y si hay una captura de pantalla o video del comportamiento nuevo. Un PR sin descripción ya merece un comentario — no sobre el código, sino sobre la costumbre.

Las preguntas que guían todo el review:

  • ¿Este cambio resuelve el problema correcto?
  • ¿El tamaño del PR es razonable? Un PR que toca 40 archivos y no tiene una descripción clara es difícil de revisar bien — más probable que pasen cosas por alto.
  • ¿Los tests cubren el comportamiento nuevo?
  • ¿Hay algo que podría romper en producción que no sea obvio desde el diff?

Arquitectura y capas — lo más importante

Los errores de arquitectura son los más costosos de corregir después. Un bug de lógica se arregla en una hora; un acoplamiento incorrecto entre capas puede requerir refactors de días.

Dependencias en la dirección incorrecta

// ❌ Un ViewModel importando Context directamente
class ProductosViewModel(
    private val context: Context,  // ← señal de alarma
    private val repo: ProductoRepository
) : ViewModel() {
    fun cargar() {
        // Usa context para algo que debería estar en el repo o en un helper
        val prefs = context.getSharedPreferences("config", Context.MODE_PRIVATE)
    }
}
// Si el ViewModel necesita el contexto, casi siempre significa que hay lógica
// que debería estar en otra capa (repositorio, helper, use case)

// ✓ El ViewModel solo conoce interfaces — el contexto lo maneja la capa de datos
class ProductosViewModel(
    private val repo: ProductoRepository,
    private val configRepo: ConfigRepository  // la capa de datos maneja las prefs
) : ViewModel()

Lógica de negocio en el Fragment o Composable

// ❌ Lógica de negocio en el Fragment
class ProductosFragment : Fragment() {
    override fun onViewCreated(view: View, savedInstanceState: Bundle?) {
        binding.btnComprar.setOnClickListener {
            val total = productos.sumOf { it.precio * it.cantidad }
            if (total > 50000) {
                // aplicar descuento del 10%
                val descuento = total * 0.1
                navegarACheckout(total - descuento)
            } else {
                navegarACheckout(total)
            }
        }
    }
}
// Esta lógica de descuento no tiene tests, no es reutilizable,
// y si cambia el criterio hay que buscarla en la UI

// ✓ La lógica vive en un Use Case o en el ViewModel
binding.btnComprar.setOnClickListener {
    viewModel.iniciarCheckout()  // el cómo es responsabilidad del ViewModel/UseCase
}

Otras señales de alerta en arquitectura

# Buscar en el diff:
# → Un Fragment/Composable que instancia sus propias dependencias (new Repository())
# → Un Repository que conoce ViewModels o Fragments
# → Un UseCase que importa clases de android.* (debería ser Kotlin puro)
# → Data classes del dominio con anotaciones de Room (@Entity) o Retrofit (@SerializedName)
#   — los modelos de dominio no deberían acoplarse a frameworks de infraestructura
# → La Activity haciendo demasiado — más de ~50 líneas en onCreate es señal

Coroutines y threading — fuente de bugs silenciosos

Scope incorrecto

// ❌ GlobalScope — sobrevive a todo, incluyendo cuando ya no importa
class ProductosViewModel : ViewModel() {
    fun cargar() {
        GlobalScope.launch {  // ← siempre es incorrecto en Android
            repo.getProductos()
        }
    }
}

// ❌ lifecycleScope en el ViewModel — el ViewModel no tiene lifecycle propio
class ProductosViewModel(lifecycle: Lifecycle) : ViewModel() {
    fun cargar() {
        lifecycle.coroutineScope.launch { /* ... */ }
    }
}

// ✓ viewModelScope — se cancela automáticamente cuando el ViewModel se destruye
class ProductosViewModel : ViewModel() {
    fun cargar() {
        viewModelScope.launch { repo.getProductos() }
    }
}

Excepciones que se tragan silenciosamente

// ❌ Excepción ignorada — el usuario no ve nada, nadie sabe qué pasó
viewModelScope.launch {
    try {
        _productos.value = repo.getProductos()
    } catch (e: Exception) {
        // no hacer nada
    }
}

// ❌ Capturando CancellationException — interfiere con la cancelación de coroutines
viewModelScope.launch {
    try {
        repo.getProductos()
    } catch (e: Exception) {  // captura TODO, incluyendo CancellationException
        _error.value = e.message
    }
}

// ✓ Manejar errores correctamente sin capturar CancellationException
viewModelScope.launch {
    try {
        _productos.value = repo.getProductos()
    } catch (e: CancellationException) {
        throw e  // re-lanzar siempre
    } catch (e: IOException) {
        _uiState.update { it.copy(error = "Sin conexión") }
    } catch (e: Exception) {
        _uiState.update { it.copy(error = e.message) }
    }
}

Dispatcher incorrecto

// ❌ Operación de I/O en el Main thread
viewModelScope.launch {  // por default corre en Main
    val archivo = File(path).readText()  // bloquea el Main thread
    _contenido.value = archivo
}

// ✓ Cambiar al dispatcher correcto para I/O
viewModelScope.launch {
    val archivo = withContext(Dispatchers.IO) {
        File(path).readText()
    }
    _contenido.value = archivo  // de vuelta en Main para actualizar el state
}

// Nota: Room y Retrofit ya manejan el threading internamente cuando usás
// suspend functions — no necesitás withContext(IO) para esas llamadas

Compose — lo que más se escapa en review

Estado que debería ser local subiendo innecesariamente

// ❌ El ViewModel maneja estado que solo la UI necesita
class BusquedaViewModel : ViewModel() {
    val queryTexto = MutableStateFlow("")  // solo lo usa el TextField
    val resultados = MutableStateFlow<List<Producto>>(emptyList())

    fun onQueryChange(q: String) {
        queryTexto.value = q  // actualización de UI pura en el ViewModel
    }
}

// ✓ El texto del input es estado local del composable
@Composable
fun PantallaBusqueda(viewModel: BusquedaViewModel) {
    var query by remember { mutableStateOf("") }
    val resultados by viewModel.resultados.collectAsStateWithLifecycle()

    SearchBar(
        query = query,
        onQueryChange = { query = it },
        onSearch = { viewModel.buscar(it) }  // sube al VM solo al confirmar
    )
    ListaResultados(resultados)
}

Recomposición innecesaria

// ❌ List<T> como parámetro — siempre inestable
@Composable
fun ListaProductos(productos: List<Producto>) { /* ... */ }
// Compose no puede saber si List cambió sin recomponer

// ✓ ImmutableList o pasar el ViewModel directamente
@Composable
fun ListaProductos(productos: ImmutableList<Producto>) { /* ... */ }

// ❌ Lambda que captura estado del padre y se recrea en cada recomposición
@Composable
fun Padre(items: List<Item>) {
    items.forEach { item ->
        Card(onClick = { viewModel.seleccionar(item.id) })
        // nueva lambda en cada recomposición del Padre
    }
}

// También buscar: objetos costosos creados sin remember
// (DateTimeFormatter, NumberFormat, etc.)

side effects fuera de LaunchedEffect

// ❌ Navegación directamente en la composición
@Composable
fun PantallaLogin(viewModel: LoginViewModel) {
    val estado by viewModel.estado.collectAsStateWithLifecycle()

    if (estado.autenticado) {
        navController.navigate("home")  // se ejecuta en cada recomposición
    }
}

// ✓ Navegación como side effect de un solo uso
@Composable
fun PantallaLogin(viewModel: LoginViewModel) {
    val efectos = viewModel.efectos
    LaunchedEffect(Unit) {
        efectos.collect { efecto ->
            when (efecto) {
                LoginEfecto.NavegaAHome -> navController.navigate("home")
            }
        }
    }
}

Ciclo de vida — leaks y crashes

// ❌ Referencia a una View o Fragment después de que fue destruida
class ProductosFragment : Fragment() {
    override fun onViewCreated(view: View, savedInstanceState: Bundle?) {
        viewModel.productos.observe(viewLifecycleOwner) { lista ->
            // binding podría ser null si el Fragment se destruyó
            binding.recycler.adapter = ProductosAdapter(lista)
        }
    }
    // Si el Fragment se destruye mientras el Observer corre → NullPointerException
}

// ✓ Usar repeatOnLifecycle para Flows, o viewLifecycleOwner para LiveData
lifecycleScope.launch {
    repeatOnLifecycle(Lifecycle.State.STARTED) {
        viewModel.productos.collect { lista ->
            binding.recycler.adapter = ProductosAdapter(lista)
        }
    }
}

// ❌ collectAsState en lugar de collectAsStateWithLifecycle en Compose
// collectAsState no se pausa cuando la pantalla va al background
val productos by viewModel.productos.collectAsState()  // sigue colectando en background

// ✓
val productos by viewModel.productos.collectAsStateWithLifecycle()  // lifecycle-aware
# Otras cosas a buscar relacionadas al ciclo de vida:
# → Listeners o callbacks registrados en onStart/onResume sin desregistrar en onStop/onPause
# → Coroutines lanzadas en lifecycleScope sin repeatOnLifecycle (no se cancelan en background)
# → ViewBinding accedido fuera de onViewCreated — puede ser null antes/después
# → Handler.postDelayed sin cancelar en onDestroyView

Seguridad — lo que se escapa fácil

// ❌ Credenciales o tokens en SharedPreferences sin cifrar
sharedPrefs.edit().putString("access_token", token).apply()
// SharedPreferences es texto plano en el filesystem del dispositivo

// ✓ EncryptedSharedPreferences o Keystore
val encryptedPrefs = EncryptedSharedPreferences.create(
    "secure_prefs", masterKey, context,
    EncryptedSharedPreferences.PrefKeyEncryptionScheme.AES256_SIV,
    EncryptedSharedPreferences.PrefValueEncryptionScheme.AES256_GCM
)

// ❌ Datos sensibles en logs
Log.d("Auth", "Token recibido: $accessToken")  // visible en logcat y en crash reports

// ❌ URLs hardcodeadas con tokens o API keys
val url = "https://api.ejemplo.com/data?key=abc123def456"
// Las keys van en local.properties o en el servidor, nunca en el código

// ❌ WebView con configuración permisiva
webView.settings.javaScriptEnabled = true
webView.settings.allowFileAccess = true  // permite acceder al filesystem
webView.settings.allowUniversalAccessFromFileURLs = true  // muy peligroso

Tests — lo que falta y lo que está mal

# Preguntas para el PR que toca lógica de negocio:
# → ¿Hay un test para el caso feliz?
# → ¿Hay un test para el caso de error? (sin red, sin datos, etc.)
# → ¿Los casos límite están cubiertos?
# → Si arreglaron un bug, ¿hay un test de regresión que lo reproduzca?

# Señales de tests que no agregan valor real:
# → Tests que solo verifican que un método fue llamado, sin verificar el resultado
# → Tests con nombres como test1(), testFuncion() — sin contexto ni escenario
# → Tests que mockean TODO, incluyendo la clase que están testeando
# → Un solo test "happy path" para un ViewModel con 5 estados posibles
// ❌ Test que no verifica nada útil
@Test
fun testCargar() {
    viewModel.cargar()
    verify { mockRepo.getProductos() }  // solo verifica que se llamó, no el resultado
}

// ✓ Test que verifica el comportamiento observable
@Test
fun `cargar con exito actualiza la lista de productos`() = runTest {
    fakeRepo.setProductos(listOf(p1, p2))
    viewModel.cargar()
    assertThat(viewModel.uiState.value.productos).containsExactly(p1, p2)
    assertThat(viewModel.uiState.value.isLoading).isFalse()
    assertThat(viewModel.uiState.value.error).isNull()
}

Cómo dar feedback que mejora sin dañar

El código lo escribió una persona. La forma en que se da el feedback determina si esa persona mejora, se defiende, o deja de tomar riesgos.

Comentar el código, no a la persona

# ❌ Suena como ataque personal
# "Esto está mal hecho"
# "No entiendo cómo pensaste que esto era una buena idea"
# "Básico: nunca hagas X"

# ✓ Describe el problema y propone la solución
# "Este Observer no usa viewLifecycleOwner — puede causar un crash si el Fragment
# se destruye mientras la lista está cargando. Reemplazar por repeatOnLifecycle."

# ✓ Explica el por qué, no solo el qué
# "Prefiero ImmutableList acá porque List<T> es inestable para Compose y va a
# causar recomposiciones innecesarias cada vez que el padre se recomponga."

Clasificar la severidad de los comentarios

No todos los comentarios tienen el mismo peso. Ser explícito sobre la severidad ayuda al autor a priorizar y evita que un comentario de estilo bloquee un arreglo urgente:

# [bloquea] — debe corregirse antes de aprobar
# "Este leak de memoria va a crashear la app en producción bajo carga. [bloquea]"

# [sugerencia] — mejora válida pero no obligatoria para este PR
# "Podríamos extraer esta lógica a un UseCase para que sea testeable. [sugerencia]"

# [nit] — detalles de estilo o convención, sin impacto real
# "El nombre podría ser más descriptivo. [nit]"

# [pregunta] — para entender la decisión, no para criticarla
# "¿Por qué elegiste GlobalScope acá en lugar de viewModelScope? [pregunta]"

Aprobar con comentarios menores pendientes

Si los únicos comentarios son nits o sugerencias, aprobá el PR igual y dejá que el autor decida si los incorpora. Bloquear un PR por indentación o nombres de variables que no son incorrectos, solo diferentes a tu preferencia, es ruido que afecta la velocidad del equipo.

El proceso en la práctica

Tamaño de PR razonable

# PRs chicos se revisan mejor y más rápido:
# → <400 líneas cambiadas: revisión posible y completa
# → 400-800 líneas: revisión posible pero requiere más esfuerzo y tiempo
# → >800 líneas: difícil revisar bien — considerar dividir

# Lo que NO cuenta como argumento para PRs grandes:
# "Fue todo o nada" — casi siempre se puede dividir
# "Tiene mucho boilerplate" — el boilerplate se puede hacer en un PR separado
# "Si lo divido se rompe" — señal de que el diseño tiene acoplamiento fuerte

Tiempo de respuesta

# La regla más importante para que el proceso funcione:
# Un PR sin revisión por más de un día hábil es un bloqueo para el autor

# Prácticas que funcionan:
# → Slot fijo de review (ej: primer hora de la mañana y después del almuerzo)
# → Si no podés hacer el review completo, decirlo: "Lo veo mañana a la mañana"
# → Si un PR está urgente, marcarlo explícitamente en el título: [URGENTE]

# Lo que más frustra a quien esperó el review:
# → Aprobar sin leer ("LGTM" en un PR de 600 líneas en 3 minutos)
# → Comentar solo sobre estilo e ignorar bugs de lógica
# → Pedir cambios y después desaparecer por dos días

El autor también tiene responsabilidades

# Antes de abrir el PR:
# → Self-review: revisá tu propio diff antes de asignarlo
#   (encontrás el 30% de los problemas antes de molestar a alguien)
# → Descripción clara: qué cambia, por qué, cómo testear
# → Tests pasando localmente
# → Sin console logs, TODOs no intencionales ni archivos temporales

# Al recibir comentarios:
# → Responder a cada comentario (aunque sea "hecho" o "no aplicaré esto porque X")
# → No resolver conversaciones que el revisor abrió — el revisor las resuelve
# → Si no entendés un comentario, preguntar antes de hacer el cambio

Cómo recibir un review — la otra mitad

La calidad del proceso depende tanto de quien revisa como de quien recibe la revisión. Un comentario bien dado pero mal recibido termina igual de mal que un comentario mal dado.

  • Los comentarios son sobre el código, no sobre vos. Un reviewer que señala un leak de memoria no está diciendo que sos un mal dev — está haciendo su trabajo.
  • Agradecer los comentarios útiles. "Buen punto, no había pensado en ese caso" mejora la cultura del equipo y hace que el reviewer quiera hacer reviews de calidad la próxima vez.
  • Discrepar con argumentos, no con autoridad. "Lo hago así porque siempre lo hicimos así" no es un argumento. "Lo hago así porque X e Y" sí lo es. Si el reviewer tiene razón, cambiarlo. Si no, explicar por qué.
  • No tomar los nits como insultos. Un comment de "[nit] este nombre podría ser más descriptivo" no merece una respuesta defensiva de cinco líneas — merece "hecho" o "prefiero dejarlo así porque se usa en todo el módulo".