Wireup Department History Timeline API#147
Conversation
…tmentHistoryTimeline component
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 improves the architecture for displaying department history by centralizing data fetching and processing. It introduces a dedicated API endpoint and a custom React hook, 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
|
There was a problem hiding this comment.
Code Review
This pull request is a great improvement, refactoring the DepartmentHistoryTimeline component to use a new useDepartmentHistory hook and a dedicated API endpoint. This change significantly simplifies the frontend by moving complex data processing to the backend. My feedback focuses on making the timeline component more robust by ensuring data is always sorted correctly and by using more stable keys for list rendering.
| {departmentHistory.map((entry, idx) => { | ||
| return ( | ||
| <VerticalTimelineElement | ||
| key={idx} |
There was a problem hiding this comment.
Using the array index as a key is not recommended, as it can lead to incorrect behavior and performance issues when the list is sorted or items are added/removed. It's best to use a unique and stable identifier from the data. Since a simple unique ID doesn't seem to be available on the entry object, you can create a composite key from its properties.
| key={idx} | |
| key={`${entry.ministry_name}-${entry.period}`} |
| }, [selectedDepartment]); | ||
| const { | ||
| data: departmentHistory, | ||
| isLoading, |
There was a problem hiding this comment.
catch the error state as well in case the request breaks we need it to fallback
| isLoading, | |
| isLoading, | |
| error |
| <div className="flex justify-center items-center h-20"> | ||
| <ClipLoader color={colors.textPrimary} loading={loading} size={25} /> | ||
| <ClipLoader color={colors.textPrimary} loading={isLoading} size={25} /> | ||
| </div> |
There was a problem hiding this comment.
Add a fallback component here in case the api throws an error.
| </div> | |
| </div> | |
| ) : error ? ( | |
| <p className="text-primary mt-5">Error loading department history.</p> |
this is just an idea for you to look at, implement a fallback ui for the DepartmentHistoryTimeline.
There was a problem hiding this comment.
Make sure all the unused functions are removed from this file
| <h2 className="text-md font-semibold text-gray-900 dark:text-white mb-2"> | ||
| Department History Not Available | ||
| </h2> |
Wireup the new api, introduce useDepartmentHistory hook and remove history enriching logic from DepartmentHistoryTimeline component
closes #66