BUG: Incorrect tagging on memory cards for 'Dates' & 'Location' issue #1178#1267
BUG: Incorrect tagging on memory cards for 'Dates' & 'Location' issue #1178#1267codex-yv wants to merge 4 commits into
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
WalkthroughThis pull request refactors how memories are classified as date-based or location-based. The backend now uses ChangesMemory Classification via Location Name Nullability
Sequence Diagram(s)sequenceDiagram
participant Client as Client (Frontend)
participant API as API /generate
participant Cluster as Memory Clustering
participant Response as Response Builder
Client->>API: Request memory generation
API->>Cluster: Generate clustered memories
Cluster->>Cluster: Create memory with location_name: string | null
Cluster->>Response: Return memories array
Response->>Response: Compute total_location via find_total_location_memories()
Response->>Client: Return {memories, total_location}
Client->>Client: Use total_location for location count
Client->>Client: Classify memories by location_name !== null
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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 (1)
backend/app/routes/memories.py (1)
166-183: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftAdd regression tests for
total_locationand null-based classification.This endpoint now depends on
location_namenullability for classification/counting. Please add API-level tests that cover mixed memories and asserttotal_location,memory_count, and date/location split consistency.As per coding guidelines: "Ensure that test code is automated, comprehensive, and follows testing best practices" and "Verify that all critical functionality is covered by tests".
🤖 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 `@backend/app/routes/memories.py` around lines 166 - 183, Add API-level tests for the memories endpoint that exercise mixed memory records including some with location_name set to null to verify classification and counts; call the route (using the project's test client) with a payload or fixture containing mixed types, then assert response["data"]["memory_count"] equals the number sent, response["total_location"] equals the value returned by the server-side helper (find_total_location_memories), and that the location/date breakdown in the message and in computed counts (location_count/date_count) matches counts derived from records where location_name nulls are treated as non-location; ensure tests are automated, cover null-based classification, and fail if total_location or the location/date split are inconsistent.
🧹 Nitpick comments (1)
frontend/src/components/Memories/MemoriesPage.tsx (1)
149-151: ⚡ Quick winReplace new
anycasts with narrow payload typing.The added
as anyusages hide API-shape drift fortoday,years, andtotal_location. A narrow local type keeps these checks compile-time safe.Suggested refactor
+type OnThisDayPayload = { + today?: string; + years?: number[]; +}; + +type AllMemoriesPayload = { + total_location?: number; +}; -const onThisDayMeta = onThisDayQuery.data?.data +const onThisDayData = onThisDayQuery.data?.data as OnThisDayPayload | undefined; +const onThisDayMeta = onThisDayData ? { - today: (onThisDayQuery.data.data as any).today, - years: (onThisDayQuery.data.data as any).years, + today: onThisDayData.today, + years: onThisDayData.years, } : null; +const allMemoriesPayload = allMemoriesQuery.data as AllMemoriesPayload | undefined; const locationCount = - (allMemoriesQuery.data as any)?.total_location ?? + allMemoriesPayload?.total_location ?? memoriesWithMultipleImages(allMemories).filter((m) => m.location_name !== null) .length;As per coding guidelines: "Avoid 'any', use explicit types."
Also applies to: 224-226
🤖 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 `@frontend/src/components/Memories/MemoriesPage.tsx` around lines 149 - 151, The code uses wide casts "(onThisDayQuery.data.data as any)" for fields today, years and total_location; define a narrow TypeScript interface (e.g., OnThisDayPayload with typed today, years, total_location) and use it instead of any by applying the interface to the query result (or to a local const typed as that interface) so accesses in MemoriesPage (references: onThisDayQuery, today, years, total_location) are compile-time checked; update the two places noted (around the current lines and the occurrences at 224-226) to use the new type rather than any.
🤖 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 `@backend/app/utils/memory_clustering.py`:
- Around line 108-113: The function find_total_location_memories currently
indexes memory["location_name"] which can raise KeyError for items missing that
key; update the loop to safely access the field (e.g., use
memory.get("location_name") or check "location_name" in memory) and only
increment tlm when the safe access returns a non-None value, leaving the rest of
the function behavior unchanged; reference the function name
find_total_location_memories and the memory variable in your change.
---
Outside diff comments:
In `@backend/app/routes/memories.py`:
- Around line 166-183: Add API-level tests for the memories endpoint that
exercise mixed memory records including some with location_name set to null to
verify classification and counts; call the route (using the project's test
client) with a payload or fixture containing mixed types, then assert
response["data"]["memory_count"] equals the number sent,
response["total_location"] equals the value returned by the server-side helper
(find_total_location_memories), and that the location/date breakdown in the
message and in computed counts (location_count/date_count) matches counts
derived from records where location_name nulls are treated as non-location;
ensure tests are automated, cover null-based classification, and fail if
total_location or the location/date split are inconsistent.
---
Nitpick comments:
In `@frontend/src/components/Memories/MemoriesPage.tsx`:
- Around line 149-151: The code uses wide casts "(onThisDayQuery.data.data as
any)" for fields today, years and total_location; define a narrow TypeScript
interface (e.g., OnThisDayPayload with typed today, years, total_location) and
use it instead of any by applying the interface to the query result (or to a
local const typed as that interface) so accesses in MemoriesPage (references:
onThisDayQuery, today, years, total_location) are compile-time checked; update
the two places noted (around the current lines and the occurrences at 224-226)
to use the new type rather than any.
🪄 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
Run ID: c3535120-8fdb-495b-bd13-36e2f82731c6
📒 Files selected for processing (5)
backend/app/routes/memories.pybackend/app/utils/memory_clustering.pyfrontend/src/api/api-functions/memories.tsfrontend/src/components/Memories/MemoriesPage.tsxfrontend/src/components/Memories/MemoryCard.tsx
| def find_total_location_memories(data:list) -> int: | ||
| tlm = 0 # total location memories | ||
| for memory in data: | ||
| if memory["location_name"] != None: | ||
| tlm +=1 | ||
| return tlm |
There was a problem hiding this comment.
Guard aggregation against missing location_name keys.
Using memory["location_name"] can raise KeyError and fail the response build if any item is missing that key. Prefer safe access here.
Suggested fix
-def find_total_location_memories(data:list) -> int:
- tlm = 0 # total location memories
+def find_total_location_memories(data: list) -> int:
+ tlm = 0 # total location memories
for memory in data:
- if memory["location_name"] != None:
- tlm +=1
- return tlm
+ if memory.get("location_name") is not None:
+ tlm += 1
+ return tlm🤖 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 `@backend/app/utils/memory_clustering.py` around lines 108 - 113, The function
find_total_location_memories currently indexes memory["location_name"] which can
raise KeyError for items missing that key; update the loop to safely access the
field (e.g., use memory.get("location_name") or check "location_name" in memory)
and only increment tlm when the safe access returns a non-None value, leaving
the rest of the function behavior unchanged; reference the function name
find_total_location_memories and the memory variable in your change.
1 participant
Addressed Issues: #1178
BUG: Incorrect tagging on memory cards for 'Dates' & 'Location'
Screenshots/Recordings:
TODO: Before and after screen shots are added at issue #1178
Additional Notes:
Please refer to the issue number #1178 for detailed description and implementation of the issue.
AI Usage Disclosure:
I used Antigravity to modify the frontend code and thoroughly reviewed the changes it made.
Check one of the checkboxes below:
I have used the following AI models and tools: TODO
Checklist
Summary by CodeRabbit
Bug Fixes
New Features