-
Notifications
You must be signed in to change notification settings - Fork 0
Paths no any #77
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
Paths no any #77
Conversation
* Agregado tipos en la función loadSectionData y en la función loadSectionDataFromFile, * Agregado un nuevo test para verificar que se cargue correctamente el tipo de sección "paths" sin usar "any". * Corregido un error en el test anterior donde se esperaba un tipo incorrecto para la sección "paths".
|
|
✅ Deploy Preview for secorto-astro ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
SeCOrTo web
|
||||||||||||||||||||||||||||
| Project |
SeCOrTo web
|
| Branch Review |
pull/77/head
|
| Run status |
|
| Run duration | 01m 40s |
| Commit |
|
| Committer | Sergio Orozco |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
|
|
0
|
|
|
0
|
|
|
0
|
|
|
0
|
|
|
6
|
| View all changes introduced in this branch ↗︎ | |
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.
Pull request overview
Este PR continúa el refactor para eliminar usos de any, fortaleciendo el tipado en la carga de secciones/entradas y agregando tests unitarios para asegurar el comportamiento de routing, RSS y utilidades relacionadas.
Changes:
- Refactor de utilidades (
sectionLoader,sectionContext,paths,rssBuilder) para mejorar tipado y centralizar helpers (extractCleanId). - Ajustes en páginas Astro para adaptarse a los nuevos tipos en el flujo de carga/render.
- Adición de nuevos tests unitarios (Vitest) para
sections,paths,sectionLoader,sectionContexty helpers de RSS.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/utils/layout.ts | Elimina un as any en el attachment de Playwright. |
| tests/unit/utils/sectionLoader.route.test.ts | Nuevos tests para carga de sección/entrada por ruta. |
| tests/unit/utils/sectionContext.test.ts | Nuevos tests para helpers de contexto (tags/detalle + fallback de locale). |
| tests/unit/utils/rssBuilder.helpers.test.ts | Nuevos tests para el mapeo de posts a items RSS. |
| tests/unit/utils/paths.test.ts | Amplía cobertura de getPostsByLocale y getUniqueTags. |
| tests/unit/config/sections.test.ts | Nuevos tests para helpers de config por collection/route. |
| tests/unit/config/sections.route.test.ts | Nuevos tests para helpers de rutas/URLs de sección. |
| src/utils/sectionLoader.ts | Ajusta tipado y lógica de tags en loadSectionByRoute; elimina loadSection. |
| src/utils/sectionContext.ts | Refactor para tipar mejor el contexto y reutilizar extractCleanId. |
| src/utils/rssBuilder.ts | Refactor para resolver config por colección y exponer helper testeable. |
| src/utils/paths.ts | Tipado más estricto para tags y cleanId (aunque con algunos detalles a pulir). |
| src/utils/ids.ts | Nuevo helper compartido extractCleanId. |
| src/pages/[locale]/[section]/tags/[tag].astro | Ajustes para tipado de posts al renderizar listas. |
| src/pages/[locale]/[section]/[...id].astro | Ajustes de tipado para render() y consumo de entry. |
| src/content.config.ts | Cambio de tipado del helper image en el schema base. |
| src/config/sections.ts | Añade getSectionConfigByCollection y refactoriza búsqueda interna. |
| docs/refactor-sections/CHANGE_LOG.md | Actualiza documentación de exports tras el refactor. |
* loadSectionByRoute no requiere inferir tipos * TagsPageContext mejorado ya es mas claro y limpio * DetailPageContext usando tips complejos de forma provisional * Eliminados extraños ; en tags * Template astro para tags esclarecida work no tiene tags asi que es eliminado, y ahora no se requieren cast extraños para los list posts
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.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 11 comments.
Comments suppressed due to low confidence (2)
src/utils/sectionLoader.ts:13
- This file still defines a local
extractCleanId, but the PR introducessrc/utils/ids.tsexporting the same helper. Consider importingextractCleanIdfrom./idsand removing this duplicate to keep the behavior centralized.
/**
* Carga una sección basada en el parámetro de ruta (URL)
* Útil para el routing dinámico en [section].astro
*/
export async function loadSectionByRoute(
sectionSlug: string,
locale: UILanguages
src/utils/sectionLoader.ts:27
getSectionConfigByRoute()now throws (it returnsSectionConfig), so thisif (!config) return nullbranch is dead code and invalid routes will throw instead of returningnull. Either restoregetSectionConfigByRouteto returnSectionConfig | null, or wrap the call in a try/catch and translate the failure intonullhere.
const tags: string[] = config.hasTags ? getUniqueTags(posts as EntryWithCleanId<CollectionWithTags>[]) : []
return {
config,
posts,
| export function getSectionConfigByRoute( | ||
| routeParam: string, | ||
| locale: UILanguages | ||
| ): SectionConfig | null { | ||
| for (const [_key, config] of Object.entries(sectionsConfig)) { | ||
| if (config.routes[locale] === routeParam) { | ||
| return config | ||
| } | ||
| } | ||
| return null | ||
| ): SectionConfig { | ||
| return findSectionConfig((config) => config.routes[locale] === routeParam) | ||
| } |
Copilot
AI
Feb 8, 2026
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.
This changes getSectionConfigByRoute from a nullable lookup to a throwing function (SectionConfig return type + findSectionConfig throwing). Multiple call sites in this PR rely on a null return to produce null/404 flows. Consider restoring the old contract (SectionConfig | null) or adding a separate requireSectionConfigByRoute that throws, and keep getSectionConfigByRoute non-throwing.
src/utils/rssBuilder.ts
Outdated
| */ | ||
| /** | ||
| * Mapea un post fuente a un `RSSItem` |
Copilot
AI
Feb 8, 2026
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.
There are two consecutive docblocks describing the same thing. Remove the duplicate so the JSDoc stays clean and tooling doesn’t pick up conflicting comments.
| */ | |
| /** | |
| * Mapea un post fuente a un `RSSItem` |
src/utils/sectionContext.ts
Outdated
| import { getSectionConfigByRoute, sectionsConfig, type SectionConfig } from '@config/sections' | ||
| import { getPostsByLocale } from './paths' | ||
| import type { EntryWithCleanId, CollectionWithTags } from './paths' | ||
| import type { UILanguages } from '@i18n/ui' | ||
| import { getCollection } from 'astro:content' | ||
| import { getCollection, type CollectionEntry } from 'astro:content' | ||
| import { languageKeys } from '@i18n/ui' | ||
| import { extractCleanId } from './ids' |
Copilot
AI
Feb 8, 2026
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.
After getSectionConfigByRoute() was changed to throw, buildSectionContext() (in this same module) will now throw an Error instead of the documented 404 Response. Consider wrapping that call in try/catch and throwing new Response('Not found', { status: 404 }) to preserve the page-level behavior.
| const entry = entries.find((e: {id: string, data: {slug?: string}}) => { | ||
| const cleanId = extractCleanId(e.id) | ||
| const entrySlug = e.data.slug || cleanId | ||
| return e.id.startsWith(`${locale}/`) && entrySlug === id |
Copilot
AI
Feb 8, 2026
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.
Same issue as above: getSectionConfigByRoute() throws now, so this null-check is dead code and loadEntryByRoute will throw for unknown sections instead of returning null. Adjust this function (or the config helper) so the public contract remains consistent.
|
|
||
| export interface DetailPageContext { | ||
| entry: any | ||
| entry: CollectionEntry<keyof import('astro:content').DataEntryMap> | { id: string; data: Record<string, unknown> } |
Copilot
AI
Feb 8, 2026
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.
DetailPageContext.entry is widened to a union with a plain { id; data } object, which forces unsafe casts downstream (see [...id].astro). Prefer keeping entry as a CollectionEntry<...> in production types and, if needed, cast test fixtures to that type instead of relaxing the public interface.
| entry: CollectionEntry<keyof import('astro:content').DataEntryMap> | { id: string; data: Record<string, unknown> } | |
| entry: CollectionEntry<keyof import('astro:content').DataEntryMap> |
| const { entry, config, cleanId, isUntranslated, translationOriginLocale } = context | ||
| const entryData = entry.data as BaseEntryData | ||
| const entryTyped = entry as CollectionEntry<CollectionKey> | ||
| const entryData = entryTyped.data as BaseEntryData |
Copilot
AI
Feb 8, 2026
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.
const entryTyped = entry as CollectionEntry<CollectionKey> is an unsafe cast that’s only necessary because DetailPageContext.entry is currently a union with a non-CollectionEntry shape. Tighten the context type (and test mocks) so this page can use entry without casting.
src/utils/paths.ts
Outdated
| import { getCollection } from "astro:content" | ||
| import type { CollectionEntry, CollectionKey } from "astro:content" | ||
| import { extractCleanId } from './ids' |
Copilot
AI
Feb 8, 2026
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.
These imports mix double quotes and single quotes in the same file. The rest of the repo uses single quotes in TS files (e.g. src/utils/sectionContext.ts). Align these imports to a single quoting convention to avoid noisy diffs.
| const posts = await getCollection(collection); | ||
| return posts | ||
| .filter(post => post.id.startsWith(`${locale}/`)) | ||
| .map(post => ({ | ||
| ...post, | ||
| cleanId: (post.data as any).slug || extractCleanId(post.id) | ||
| cleanId: post.data.slug || extractCleanId(post.id) | ||
| })) | ||
| .sort((a, b) => b.cleanId.localeCompare(a.cleanId)); | ||
| } |
Copilot
AI
Feb 8, 2026
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.
There are still semicolons left in this file (getCollection(collection); and the .sort(...); line), which is inconsistent with the repo convention of omitting semicolons. Remove them for consistency.
src/utils/sectionContext.ts
Outdated
| @@ -69,11 +71,11 @@ export async function buildTagsPageContext( | |||
| } | |||
Copilot
AI
Feb 8, 2026
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.
Same as buildSectionContext: this null-check is dead code after getSectionConfigByRoute() was changed to throw. Ensure an unknown section results in the intended 404 Response, not an uncaught Error.
src/utils/sectionContext.ts
Outdated
| section: string, | ||
| locale: UILanguages, | ||
| id: string | ||
| ) => Promise<{ entry: CollectionEntry<keyof import('astro:content').DataEntryMap> | { id: string; data: Record<string, unknown> }; config: SectionConfig } | null> |
Copilot
AI
Feb 8, 2026
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.
This callback type also returns a unioned entry shape, which propagates the loosened typing throughout the app. Tighten this to return a CollectionEntry<...> (matching what loadEntryByRoute returns in real code) and adapt tests/mocks accordingly.
| ) => Promise<{ entry: CollectionEntry<keyof import('astro:content').DataEntryMap> | { id: string; data: Record<string, unknown> }; config: SectionConfig } | null> | |
| ) => Promise<{ entry: CollectionEntry<keyof import('astro:content').DataEntryMap>; config: SectionConfig } | null> |
* getSectionConfigByRoute arroja una excepción * Eliminados null checks en sectionContext * Eliminados test que validaban config null
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.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
src/utils/sectionLoader.ts:22
- Cambio de contrato:
loadSectionByRouteya no puede devolvernullporquegetSectionConfigByRoutelanza si no encuentra sección. Hay call sites que tratanloadSectionByRoute(...)como nullable (p. ej. hacenif (!sectionData) return 404), por lo que este cambio puede dejar paths inválidos como excepción/500 y además deja código muerto. O bien vuelve a retornarnullcuando no hay config (capturando el error), o ajusta el API y actualiza todos los consumidores.
export async function loadSectionByRoute(
sectionSlug: string,
locale: UILanguages
) {
const config = getSectionConfigByRoute(sectionSlug, locale)
const posts = await getPostsByLocale(config.collection, locale)
const tags: string[] = config.hasTags ? getUniqueTags(posts as EntryWithCleanId<CollectionWithTags>[]) : []
return {
config,
posts,
tags
}
| /** | ||
| * Construye el contexto de una página de sección (índice). | ||
| * Valida que la sección exista en la configuración. | ||
| * @throws Response 404 si la sección no existe | ||
| * @returns Contexto con configuración validada | ||
| */ | ||
| export function buildSectionContext(section: string, locale: UILanguages): SectionContext { | ||
| const config = getSectionConfigByRoute(section, locale) | ||
|
|
Copilot
AI
Feb 8, 2026
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.
getSectionConfigByRoute ya no retorna null; ahora lanza Error. Este helper quedó documentado/consumido como si pudiera producir un 404 controlado (antes se devolvía null o se lanzaba Response). Con el cambio, rutas inválidas terminan como excepción genérica (típicamente 500) y además el JSDoc aquí (@throws Response 404) queda incorrecto. Considera reintroducir un retorno nullable o proveer una variante ...OrThrow y mantener esta función/los call sites devolviendo 404 de forma explícita.
* ; eliminado paths * eliminado doble jsdoc mapPostToRSSItem * imports de id son con "@utils/ids" * Eliminado problemático test buildDetailPageContext returns loaded entry when loadEntryByRoute finds i
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.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
src/utils/sectionContext.ts:45
buildSectionContextused to throw a 404Responsewhen the section didn’t exist; now it will throw a generic error viagetSectionConfigByRoute, which typically becomes a 500. If this function is used with dynamic params, consider preserving the 404 behavior (e.g., catch and thrownew Response('Not found', { status: 404 })).
export function buildSectionContext(section: string, locale: UILanguages): SectionContext {
const config = getSectionConfigByRoute(section, locale)
return {
config,
locale,
section
}
| function findSectionConfig(predicate: (c: SectionConfig) => boolean): SectionConfig { | ||
| for (const config of Object.values(sectionsConfig)) { | ||
| if (predicate(config)) return config | ||
| } | ||
| throw new Error('Section config not found') | ||
| } |
Copilot
AI
Feb 8, 2026
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.
The thrown error message 'Section config not found' is too generic for debugging. Include context (e.g., whether lookup was by route+locale or collection, and the values) so logs clearly identify what failed.
| @@ -68,14 +32,11 @@ export async function loadEntryByRoute( | |||
| id: string | |||
| ) { | |||
| const config = getSectionConfigByRoute(sectionSlug, locale) | |||
Copilot
AI
Feb 8, 2026
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.
loadEntryByRoute will now throw for an unknown sectionSlug because getSectionConfigByRoute throws. This prevents buildDetailPageContext from returning null and the page from responding with a 404. Catch the lookup failure here (or use a non-throwing config lookup) and return null for unknown sections so routing behaves predictably.
| const config = getSectionConfigByRoute(sectionSlug, locale) | |
| let config | |
| try { | |
| config = getSectionConfigByRoute(sectionSlug, locale) | |
| } catch { | |
| // Si la sección no existe, retornamos null para permitir un 404 correcto | |
| return null | |
| } |
| const config = getSectionConfigByRoute(section, locale) | ||
|
|
Copilot
AI
Feb 8, 2026
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.
buildTagsPageContext’s JSDoc says it throws a Response 404 when the section doesn’t exist, but the function now just calls getSectionConfigByRoute (which throws a generic Error). Either update the doc, or (preferably) convert “section not found” into a 404 Response here so the runtime behavior matches the contract.
| const config = getSectionConfigByRoute(section, locale) | |
| let config: SectionConfig | |
| try { | |
| config = getSectionConfigByRoute(section, locale) | |
| } catch (error) { | |
| // Convertimos explícitamente el caso de "sección no encontrada" | |
| // en una Response 404 para respetar el contrato del JSDoc. | |
| if (error instanceof Error) { | |
| const message = error.message.toLowerCase() | |
| const mentionsSection = message.includes('section') || message.includes('sección') | |
| const mentionsNotFound = | |
| message.includes('not found') || | |
| message.includes('no existe') || | |
| message.includes('missing') || | |
| message.includes('unknown') | |
| if (mentionsSection && mentionsNotFound) { | |
| throw new Response(null, { status: 404 }) | |
| } | |
| } | |
| throw error | |
| } |
* removida mención a 404
* buildTagsPageContext filters posts and aggregates tags
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.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.
| const pageDescription = entry.data.excerpt || entry.data.description || '' | ||
| const { Content } = await render(entryTyped) | ||
| const pageTitle = entryTyped.data.title || t(config.translationKey) | ||
| const pageDescription = (entryTyped.data as any).excerpt || (entryTyped.data as any).description || '' |
Copilot
AI
Feb 8, 2026
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.
pageDescription is computed via (entryTyped.data as any)...., which reintroduces any in a file that otherwise already has a typed entryData (BaseEntryData). Add excerpt?: string / description?: string to BaseEntryData (or define a dedicated type for the collections used here) and use entryData.excerpt || entryData.description || '' instead.
| const posts = await getPostsByLocale(config.collection, locale) | ||
| const tags = config.hasTags ? getUniqueTags(posts) : [] | ||
|
|
||
| const tags: string[] = config.hasTags ? getUniqueTags(posts as EntryWithCleanId<CollectionWithTags>[]) : [] |
Copilot
AI
Feb 8, 2026
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.
getUniqueTags(posts as EntryWithCleanId<CollectionWithTags>[]) relies on a type assertion that can hide real mismatches between config.collection and the collections that actually have tags. Prefer narrowing config.collection (e.g. if (config.collection === 'blog' || config.collection === 'talk')) before calling getUniqueTags so the types line up without casts.
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.
I don't want as type assertions everywhere I'm removing it, so no, thanks

Pull request
Eliminación masiva any
refactor sobre sectionContext para soportar tipado
actualizadas dependencias de section context para reflejar la nueva realidad