#4079 Gantt Chart Improvements Data Optimization#4080
#4079 Gantt Chart Improvements Data Optimization#4080JoshuaGoldberg wants to merge 16 commits intofeature/gantt-chart-improvementsfrom
Conversation
…ata-optimizations
…m/Northeastern-Electric-Racing/FinishLine into #4079-gantt-chart-improvements-data-optimizations
There was a problem hiding this comment.
Pull request overview
This PR focuses on improving Gantt chart performance by reducing unnecessary re-renders and deferring expensive task-tree transformations until the user expands a project (via loadChildren), along with related UI cleanup (removing global expand/collapse controls).
Changes:
- Introduces lazy child loading for project tasks in the Gantt data model (
loadChildren) and updates task rendering to load children on expansion. - Refactors Gantt chart components/pages to remove top-level “show children” state/handlers and reduce re-render propagation.
- Optimizes app-level state initialization to prevent recreating providers/layout on interaction (sidebar/query context adjustments).
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/frontend/src/utils/gantt.utils.tsx | Adds loadChildren to Gantt tasks and defers project child transformation until expansion. |
| src/frontend/src/pages/RetrospectivePage/Retrospective.tsx | Removes per-element expand/collapse state and passes simplified props to GanttChart. |
| src/frontend/src/pages/GanttPage/ProjectGanttChart/ProjectGanttChartPage.tsx | Removes expand/collapse state wiring and related handlers from the project Gantt page. |
| src/frontend/src/pages/GanttPage/ProjectGanttChart/GanttChartFiltersButton.tsx | Drops expand/collapse props passthrough from filters button. |
| src/frontend/src/pages/GanttPage/ProjectGanttChart/GanttChartFilters.tsx | Removes expand/collapse UI controls from the filters popover. |
| src/frontend/src/pages/GanttPage/GanttChart/GanttChartSection.tsx | Refactors tooltip handling and adds Archer refresh on subtree toggle. |
| src/frontend/src/pages/GanttPage/GanttChart/GanttChartComponents/GanttTaskBar/GanttTaskBarView.tsx | Implements local expand state + lazy loading for view mode task bars. |
| src/frontend/src/pages/GanttPage/GanttChart/GanttChartComponents/GanttTaskBar/GanttTaskBarEdit.tsx | Updates edit rendering to fall back to loadChildren when children is empty. |
| src/frontend/src/pages/GanttPage/GanttChart/GanttChartComponents/GanttTaskBar/GanttTaskBarDisplay.tsx | Uses task.overlays (instead of children) for rendering overlay bars. |
| src/frontend/src/pages/GanttPage/GanttChart/GanttChartComponents/GanttTaskBar/GanttTaskBar.tsx | Removes global show-children props and threads through onToggle for Archer refresh. |
| src/frontend/src/pages/GanttPage/GanttChart/GanttChartCollectionSection.tsx | Removes per-render child sorting and eliminates show-children prop plumbing. |
| src/frontend/src/pages/GanttPage/GanttChart/GanttChart.tsx | Simplifies GanttChart API by removing show-children props. |
| src/frontend/src/app/AppContextUser.tsx | Tightens component typing to explicitly accept children. |
| src/frontend/src/app/AppContextQuery.tsx | Moves QueryClient creation to module scope to avoid recreating it on render. |
| src/frontend/src/app/AppAuthenticated.tsx | Extracts sidebar layout wrapper to reduce re-render impact from sidebar interactions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| {(task.children.length > 0 ? task.children : (task.loadChildren?.() ?? [])).map((child) => { | ||
| return ( |
There was a problem hiding this comment.
Calling task.loadChildren?.() during render will recompute the full child list on every edit-mode re-render (and will create new task wrapper objects each time). This undermines the lazy-load optimization and can get expensive. Consider loading/caching children once (e.g., in state/effect) or ensuring the edit-mode task graph is pre-materialized before rendering.
There was a problem hiding this comment.
I agree, also we need to make sure that we don't end up having the same re-rendering issue for projects with no tasks
| console.log('I Rerender!'); | ||
|
|
There was a problem hiding this comment.
Remove the leftover debug console.log — it will spam the browser console on every render and can noticeably slow down the Gantt chart when many tasks are present.
| console.log('I Rerender!'); |
| const [showChildren, setShowChildren] = useState(false); | ||
| const [loadedChildren, setLoadedChildren] = useState<GanttTask<T>[]>(task.children); | ||
| const [hasLoaded, setHasLoaded] = useState(task.children.length > 0); | ||
|
|
||
| const handleToggle = () => { | ||
| if (!hasLoaded && task.loadChildren) { | ||
| setLoadedChildren(task.loadChildren()); | ||
| setHasLoaded(true); |
There was a problem hiding this comment.
loadedChildren/hasLoaded are initialized from task.children once and never reset if the task prop changes (same task.id key). This can leave stale children visible when filters/data change (e.g., toggling “hide tasks” changes task.loadChildren output, but hasLoaded stays true so it won’t reload). Consider resetting these pieces of state in an effect when task.loadChildren (or other task identity/data inputs) change, or derive children directly from props when possible.
There was a problem hiding this comment.
Changing the hide tasks removes the tasks right now so this isn't currently a problem, although that toggle also takes forever so it might be worth considering if theres a better way to do that. Instead of keeping a state for ehther or not its loaded, I feel like it would be better to encode that in the parameter itself with the optional field but idk if there was something preventing that which lead to the current approach
|
|
||
| updateRef.current = (options, y = 0) => { | ||
| setTooltipOptions(options); | ||
| if (options && y) setCursorY(y); |
There was a problem hiding this comment.
if (options && y) setCursorY(y); won’t update the tooltip position when y is 0 (valid at the very top of the viewport), causing the tooltip to keep the previous Y position. Use an explicit undefined/null check for y instead of a truthiness check.
| if (options && y) setCursorY(y); | |
| if (options && y !== undefined && y !== null) setCursorY(y); |
| [ | ||
| ...project.workPackages.map((workPackage) => transformWorkPackageToGanttTask(workPackage, project.workPackages)), | ||
| ...taskList.map((task) => transformTaskToGanttTask(task, endDate)) | ||
| ].sort((a, b) => new Date(a.start).getTime() - new Date(b.start).getTime()), |
There was a problem hiding this comment.
how much time does this actually take? If you haven't tested the times please do so bc maybe im wrong but id be shocked if this function call took anywhere near a meaningful amount of time in which case it wouldn't be worth the added complexity
Changes
Added a variety of new features:
Data is transformed only on dropdown click via loadChildren()
callbacks and useRef were added to GanttChartSection
additional callBacks were added to App/Sidebar to prevent rerenders on click,
for sidebar in particular, this prevents re-renders of everything when the sidebar opens/closes, reducing the lag.
A simple fix for state was also implemented to prevented rerenders at the top level.
Various other small fixes.
Notes
N/A
Screenshots
N/A
Checklist
It can be helpful to check the
ChecksandFiles changedtabs.Please review the contributor guide and reach out to your Tech Lead if anything is unclear.
Please request reviewers and ping on slack only after you've gone through this whole checklist.
yarn.lockchanges (unless dependencies have changed)Closes # (issue #)