blog: support Markdown rendering and 404 routes#38
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the blog's functionality by introducing dynamic Markdown rendering and robust 404 error handling. It refactors the main application component to interpret URL paths as requests for specific Markdown blog posts, fetching and displaying them accordingly. For any invalid or non-existent paths, a user-friendly 404 page is now presented, improving the overall user experience and site navigation. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR adds Markdown rendering and 404 route handling functionality. Two critical logic errors need to be addressed:
- Error handling masking: Network failures are incorrectly treated as 404 errors, misleading users
- Missing error state: The error status is set but never handled in the render logic, causing blank screens
These issues will cause incorrect user experiences during network failures. Please address these before merging.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
| fetch(`/posts/${year}/${slug}.md`) | ||
| .then(res => res.text()) | ||
| .then(text => { | ||
| setContent(text); | ||
| setStatus('success'); | ||
| }) | ||
| .catch(() => setStatus('404')); |
There was a problem hiding this comment.
🛑 Logic Error: The fetch error handler catches all errors and displays a 404 page, which will mislead users when network errors or server errors occur. Users experiencing connection issues will see "Page Not Found" instead of an appropriate error message.
| fetch(`/posts/${year}/${slug}.md`) | |
| .then(res => res.text()) | |
| .then(text => { | |
| setContent(text); | |
| setStatus('success'); | |
| }) | |
| .catch(() => setStatus('404')); | |
| fetch(`/posts/${year}/${slug}.md`) | |
| .then(res => { | |
| if (!res.ok) { | |
| setStatus('404'); | |
| return; | |
| } | |
| return res.text(); | |
| }) | |
| .then(text => { | |
| if (text) { | |
| setContent(text); | |
| setStatus('success'); | |
| } | |
| }) | |
| .catch(() => setStatus('error')); |
| if (status === 'loading') return <div>Loading...</div>; | ||
| if (status === '404') return <NotFound />; |
There was a problem hiding this comment.
🛑 Logic Error: Missing status condition handler. The code sets status to 'error' for network failures but only checks for 'loading' and '404' states, causing the component to render nothing when network errors occur.
| if (status === 'loading') return <div>Loading...</div>; | |
| if (status === '404') return <NotFound />; | |
| if (status === 'loading') return <div>Loading...</div>; | |
| if (status === '404') return <NotFound />; | |
| if (status === 'error') return <div>Network error. Please try again later.</div>; |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request successfully adds Markdown rendering for blog posts and a 404 routing mechanism. A security audit confirmed no significant vulnerabilities, noting proper validation of Markdown file paths and safe rendering using react-markdown. However, a high-severity issue was identified in error handling when fetching posts, which could lead to unexpected behavior if a file is not found on the server. Additionally, several suggestions were made to improve code organization and maintainability, such as extracting logic into custom hooks, using a layout component to avoid duplication, and moving inline styles to CSS files.
|
|
||
| if (allPostFiles[expectedPath]) { | ||
| fetch(`/posts/${year}/${slug}.md`) | ||
| .then(res => res.text()) |
There was a problem hiding this comment.
The fetch API does not reject on HTTP error statuses (like 404 or 500). This means that if the markdown file is not found on the server, the .then block will still execute on an error page's content. You should check if the response was successful before processing it to ensure you show your custom 404 page correctly.
For example:
.then(res => {
if (!res.ok) {
throw new Error(`HTTP error! status: ${res.status}`);
}
return res.text();
})| ### Code Example | ||
| ```javascript | ||
| console.log("Hello from 2026!"); | ||
| ``` No newline at end of file |
There was a problem hiding this comment.
It's a good practice for text files to end with a newline character. Some tools and systems expect this and may behave unexpectedly without it.
| function App() { | ||
| const [content, setContent] = useState(''); | ||
| const [status, setStatus] = useState('loading'); | ||
|
|
||
| useEffect(() => { | ||
| const params = new URLSearchParams(window.location.search); | ||
| const redirectedPath = params.get('p'); | ||
| const currentPath = redirectedPath || window.location.pathname; | ||
|
|
||
| if (redirectedPath) { | ||
| window.history.replaceState(null, '', redirectedPath); | ||
| } | ||
|
|
||
| if (currentPath === '/' || currentPath === '/index.html') { | ||
| setContent('# Welcome My Blog'); | ||
| setStatus('success'); | ||
| return; | ||
| } | ||
|
|
||
| const parts = currentPath.replace(/\.html$/, '').split('/').filter(Boolean); | ||
| const [year, slug] = parts; | ||
|
|
||
| if (year && slug) { | ||
| const expectedPath = `/public/posts/${year}/${slug}.md`; | ||
|
|
||
| if (allPostFiles[expectedPath]) { | ||
| fetch(`/posts/${year}/${slug}.md`) | ||
| .then(res => res.text()) | ||
| .then(text => { | ||
| setContent(text); | ||
| setStatus('success'); | ||
| }) | ||
| .catch(() => setStatus('404')); | ||
| } else { | ||
| setStatus('404'); | ||
| } | ||
| } else { | ||
| setStatus('404'); | ||
| } | ||
| }, []); | ||
|
|
||
| if (status === 'loading') return <div>Loading...</div>; | ||
| if (status === '404') return <NotFound />; | ||
|
|
||
| function App() { | ||
| return ( | ||
| <> | ||
| <Analytics /> | ||
| <h1 className="main-title">Hello World</h1> | ||
| <article style={{ padding: '40px', maxWidth: '800px', margin: '0 auto' }}> | ||
| <ReactMarkdown>{content}</ReactMarkdown> | ||
| </article> | ||
| </> | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
This component currently handles routing, data fetching, and rendering. This can make it harder to maintain. Consider these refactors:
- Extract a custom hook: The logic inside
useEffectfor routing and fetching post data could be extracted into a custom hook (e.g.,usePost()). - Create a Layout component:
<Analytics />is included here and also inNotFound.jsx. To avoid duplication, you could create aLayoutcomponent that renders<Analytics />and then the specific page content. This would also ensure analytics are always active, even on the loading screen.
| } | ||
|
|
||
| if (currentPath === '/' || currentPath === '/index.html') { | ||
| setContent('# Welcome My Blog'); |
There was a problem hiding this comment.
For better maintainability, it's good practice to avoid hardcoding strings directly in the logic. Consider extracting this welcome message into a constant, e.g., const HOME_PAGE_CONTENT = '# Welcome My Blog'; at the top of the file.
| <> | ||
| <Analytics /> | ||
| <h1 className="main-title">Hello World</h1> | ||
| <article style={{ padding: '40px', maxWidth: '800px', margin: '0 auto' }}> |
There was a problem hiding this comment.
Using inline styles can make the component harder to maintain and reuse, especially as the application grows. Consider moving these styles to a dedicated CSS file or using CSS Modules for better separation of concerns and reusability.
| <div style={{ textAlign: 'center', padding: '10vh 20px' }}> | ||
| <h1 style={{ fontSize: '3rem', color: '#ff4d4f' }}>404</h1> | ||
| <h2>Page Not Found</h2> | ||
| <p>Sorry, the article or page you are looking for seems to have moved or no longer exists.</p> | ||
| <a href="/" style={{ color: '#1890ff', textDecoration: 'none' }}> | ||
| ← Return to homepage | ||
| </a> | ||
| </div> |
There was a problem hiding this comment.
Similar to App.jsx, these inline styles could be extracted to a CSS file or CSS Module. This centralizes styling, making it easier to manage and maintain a consistent look and feel across the application.
| </div> | ||
| </> | ||
| ); | ||
| } No newline at end of file |
There was a problem hiding this comment.
It's a good practice for files to end with a newline character. Please add one here.
| } | |
| } | |
1 participant
No description provided.