⚡ Bolt: Derive project lists in-memory to avoid duplicate queries#151
⚡ Bolt: Derive project lists in-memory to avoid duplicate queries#151aicoder2009 wants to merge 1 commit into
Conversation
… project detail page Co-authored-by: aicoder2009 <127642633+aicoder2009@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR implements in-memory subset derivation for the project page. Instead of issuing a dedicated API call for project-specific lists, the page now fetches the global lists collection and the project concurrently, then derives the project's lists by filtering the global collection in-memory. The pattern is documented in the decision log. ChangesIn-Memory List Derivation
Sequence DiagramsequenceDiagram
participant Client
participant ProjectAPI as /api/projects/{id}
participant ListsAPI as /api/lists
participant Page as Project Page
Client->>ProjectAPI: Fetch project details
Client->>ListsAPI: Fetch all lists
ProjectAPI-->>Client: project data
ListsAPI-->>Client: all lists array
Note over Page: Filter allLists where projectId matches
Page->>Page: Derive project-specific lists in-memory
Client->>Client: Render page with filtered lists
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/app/projects/[id]/page.tsx (2)
213-215:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUpdate
allListsstate to maintain consistency.When adding a list to the project, the code updates
listsbut notallLists. This causesallListsto become stale, holding the old version of the list without the updatedprojectId. BecauseavailableLists(line 236) is computed fromallLists, it will incorrectly show the just-added list as still available, breaking the add-existing-list UI.🔧 Proposed fix to update both state variables
const addedList = allLists.find((l) => l.id === listId); if (addedList) { setLists((prev) => [...prev, { ...addedList, projectId }]); + setAllLists((prev) => + prev.map((l) => (l.id === listId ? { ...l, projectId } : l)) + ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/projects/`[id]/page.tsx around lines 213 - 215, When adding an existing list you update lists but leave allLists stale, so availableLists (computed from allLists) still shows the added list; after finding addedList in the block that calls setLists((prev) => [...prev, { ...addedList, projectId }]), also update allLists via setAllLists to replace the matching entry (by id) with the same updated object ({ ...addedList, projectId }) so both lists and allLists remain consistent and availableLists reflects the change.
191-191:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUpdate
allListsstate to maintain consistency.When removing a list from the project, the code updates
listsbut notallLists. This causesallListsto become stale, still holding the list with itsprojectIdset. BecauseavailableLists(line 236) is computed fromallLists, it will not include the just-removed list, preventing the user from re-adding it to the project.🔧 Proposed fix to update both state variables
if (result.success) { setLists((prev) => prev.filter((l) => l.id !== listId)); + setAllLists((prev) => + prev.map((l) => (l.id === listId ? { ...l, projectId: undefined } : l)) + ); posthog.capture("list_removed_from_project", { project_id: projectId }); } else {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/projects/`[id]/page.tsx at line 191, The removal only updates lists but leaves allLists stale; in the same handler that calls setLists((prev) => prev.filter((l) => l.id !== listId)), also update allLists so the removed list no longer carries the projectId (so availableLists includes it). For example, call setAllLists(prev => prev.map(l => l.id === listId ? { ...l, projectId: null } : l)) or an equivalent transformation; apply this change in the function where setLists and listId are used so both state arrays remain consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/app/projects/`[id]/page.tsx:
- Around line 84-91: The fetch result handling for lists currently only updates
state when allListsResult.success is true, leaving the UI silent on failure; add
an else branch for allListsResult.success that records the failure and surfaces
it to the user by (a) setting an error state (e.g., create/use setListsError or
reuse an existing error state) and/or logging the failure, and (b) showing user
feedback (toast/alert) so the user knows lists failed to load; implement this
immediately after the allListsResult check near the setAllLists/setLists logic
and reference allListsResult, setAllLists, setLists and projectId to locate the
block. Ensure the UI uses the new error state to display an error message
instead of silently showing zero lists.
---
Outside diff comments:
In `@src/app/projects/`[id]/page.tsx:
- Around line 213-215: When adding an existing list you update lists but leave
allLists stale, so availableLists (computed from allLists) still shows the added
list; after finding addedList in the block that calls setLists((prev) =>
[...prev, { ...addedList, projectId }]), also update allLists via setAllLists to
replace the matching entry (by id) with the same updated object ({ ...addedList,
projectId }) so both lists and allLists remain consistent and availableLists
reflects the change.
- Line 191: The removal only updates lists but leaves allLists stale; in the
same handler that calls setLists((prev) => prev.filter((l) => l.id !== listId)),
also update allLists so the removed list no longer carries the projectId (so
availableLists includes it). For example, call setAllLists(prev => prev.map(l =>
l.id === listId ? { ...l, projectId: null } : l)) or an equivalent
transformation; apply this change in the function where setLists and listId are
used so both state arrays remain consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e72964d5-d7b4-4a63-8e78-8b1848e6a2ea
📒 Files selected for processing (2)
.jules/bolt.mdsrc/app/projects/[id]/page.tsx
| if (allListsResult.success) { | ||
| setAllLists(allListsResult.data); | ||
| // Derive the project lists in-memory | ||
| const projectLists = allListsResult.data.filter( | ||
| (list: List) => list.projectId === projectId | ||
| ); | ||
| setLists(projectLists); | ||
| } |
There was a problem hiding this comment.
Add error handling for failed lists fetch.
If allListsResult.success is false, the code silently skips updating lists, leaving them as an empty array. The user will see the project with zero lists without any indication that the lists fetch failed, which could be confusing.
🛡️ Proposed fix to add error feedback
if (allListsResult.success) {
setAllLists(allListsResult.data);
// Derive the project lists in-memory
const projectLists = allListsResult.data.filter(
(list: List) => list.projectId === projectId
);
setLists(projectLists);
+} else {
+ setError("Failed to load lists");
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/projects/`[id]/page.tsx around lines 84 - 91, The fetch result
handling for lists currently only updates state when allListsResult.success is
true, leaving the UI silent on failure; add an else branch for
allListsResult.success that records the failure and surfaces it to the user by
(a) setting an error state (e.g., create/use setListsError or reuse an existing
error state) and/or logging the failure, and (b) showing user feedback
(toast/alert) so the user knows lists failed to load; implement this immediately
after the allListsResult check near the setAllLists/setLists logic and reference
allListsResult, setAllLists, setLists and projectId to locate the block. Ensure
the UI uses the new error state to display an error message instead of silently
showing zero lists.
💡 What: Replaced the redundant
/api/projects/[id]/listsfetch in thefetchProjectAndListsfunction ofsrc/app/projects/[id]/page.tsx. Instead of making a separate API call, we now filter the all-lists data (allListsResult.data) in-memory to derive the subset of lists that belong to the current project.🎯 Why: The project detail page was fetching all lists and project-specific lists simultaneously, leading to unnecessary network traffic and duplicate database querying on the backend, creating performance bottlenecks.
📊 Impact: Reduces network requests on page load by 1 and cuts backend database querying in half for list fetching on this page, improving server load and potential TTFB slightly.
🔬 Measurement: You can verify this by observing the network tab in the browser or by running
pnpm testandpnpm lintlocally which both pass. Tests passed globally and I manually verified the logic.PR created automatically by Jules for task 2402460240466431541 started by @aicoder2009
Summary by CodeRabbit
Chores
Refactor