[Riccardo doubts about this] fix: Fix 404 on talk and article detail pages #9
Conversation
|
🤖 Hi @palladius, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
|
this fixes #7 @gemini-cli I see there's a JS where a lot of code is removed, do you think its safe to merge? |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
📋 Review Summary
This pull request effectively resolves the 404 errors on talk and article detail pages by standardizing the date format in slugs. The approach is solid and the core logic is sound.
🔍 General Feedback
- The transition from mocked unit tests to integration tests is a significant improvement and greatly increases confidence in the data-fetching logic.
- The introduction of a
formatDateutility is a good step towards consistency. I've provided a suggestion to make it more robust against timezone-related issues. - I've also suggested a small improvement for date parsing to leverage the existing
parseDateStringutility for better consistency and reliability.
Overall, these are great changes. The author's concern about "deleting plenty of stuff" is understandable given the test file rewrite, but the new tests are much more valuable. Well done!
|
|
||
| export const formatDate = (date: Date): string => { | ||
| if (!date) return ''; | ||
| return new Date(date).toISOString().split('T')[0]; |
There was a problem hiding this comment.
🟢 Using toISOString can lead to off-by-one day errors due to timezone conversions, as it always returns the date in UTC. A safer approach is to construct the date string from the local date components (getFullYear, getMonth, getDate). This ensures that the formatted date matches the local date, regardless of the timezone.
| return new Date(date).toISOString().split('T')[0]; | |
| const d = new Date(date); | |
| const year = d.getFullYear(); | |
| const month = `${d.getMonth() + 1}`.padStart(2, '0'); | |
| const day = `${d.getDate()}`.padStart(2, '0'); | |
| return `${year}-${month}-${day}`; |
| @@ -31,7 +31,7 @@ export async function getArticles() { | |||
| }); | |||
| return articles.map((article) => ({ | |||
| ...article, | |||
| slug: `${article.publish_date}-${slugify(article.title)}`, | |||
| slug: `${formatDate(new Date(article.publish_date))}-${slugify(article.title)}`, | |||
There was a problem hiding this comment.
🟢 For consistency and robustness, it's better to use the existing parseDateString utility to parse the publish_date string. The new Date(dateString) constructor can have inconsistent behavior across different JavaScript environments, whereas parseDateString is guaranteed to handle the YYYY-MM-DD format correctly. You'll also need to import parseDateString from ../../lib/utils.
| slug: `${formatDate(new Date(article.publish_date))}-${slugify(article.title)}`, | |
| slug: `${formatDate(parseDateString(article.publish_date))}-${slugify(article.title)}`, |
Not sure if this is safe. It's deleting plenty of stuff