did some code refactoring for inconsitent data handling#103
Conversation
There was a problem hiding this comment.
Pull Request Overview
Centralizes Firestore data normalization into a single service and updates consumers to use it for consistent timestamp and field handling across the app.
- Introduces services/firestoreNormalizer.js with normalization utilities and unit tests.
- Refactors Messages and Profile pages, and the UI utils wrapper, to consume the new normalizers.
- Standardizes timestamps to ISO strings and unifies field name variations (e.g., imageURL/imageUrl).
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/services/firestoreNormalizer.js | New normalization utilities for items, users, messages, conversations, and announcements, plus helpers. |
| frontend/src/services/firestoreNormalizer.test.js | Unit tests covering timestamp handling and object normalizers. |
| frontend/src/pages/Messages.js | Refactors fetching, sorting, and rendering to use normalized conversation/message/item data. |
| frontend/src/pages/ProfilePage.js | Replaces local date coalescing with centralized extractDate helper. |
| frontend/src/lib/utils.js | Wraps normalizeItem for UI use; removes local category/date normalization and adds imageUrl fallback. |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| const aTime = a.lastMessageTime ? new Date(a.lastMessageTime).getTime() : 0; | ||
| const bTime = b.lastMessageTime ? new Date(b.lastMessageTime).getTime() : 0; |
There was a problem hiding this comment.
This sort dropped the previous fallback to createdAt. If lastMessageTime is missing, conversations will sort as 0 and may appear out of order. Consider falling back to createdAt: compute time from lastMessageTime, or else from createdAt (both normalized ISO strings), before comparing.
| const aTime = a.lastMessageTime ? new Date(a.lastMessageTime).getTime() : 0; | |
| const bTime = b.lastMessageTime ? new Date(b.lastMessageTime).getTime() : 0; | |
| const aTime = a.lastMessageTime | |
| ? new Date(a.lastMessageTime).getTime() | |
| : (a.createdAt ? new Date(a.createdAt).getTime() : 0); | |
| const bTime = b.lastMessageTime | |
| ? new Date(b.lastMessageTime).getTime() | |
| : (b.createdAt ? new Date(b.createdAt).getTime() : 0); |
| ...normalized, | ||
| // Override imageUrl default if it's empty | ||
| imageUrl: normalized.imageUrl || DEFAULT_IMAGE_URL, | ||
| // Add UI-specific fields | ||
| reporter, | ||
| claimed, | ||
| claimedAt, | ||
| claimedBy, | ||
| coordinates, | ||
| }; |
There was a problem hiding this comment.
This refactor removes the previous category normalization (e.g., 'keys/cards' -> 'keys-cards', 'accessories' -> 'accessory'), changing the API shape that UI code may rely on for filtering/styling. Either re-apply the mapping here (post-normalization) or move it into normalizeItem so that all consumers receive consistent, normalized category values.
| // REFACTORED: Replaced coalesceDate with extractDate from firestoreNormalizer | ||
| // This provides consistent date handling across the entire app | ||
| const coalesceDate = (item) => extractDate(item) || item.date || new Date().toISOString() |
There was a problem hiding this comment.
The fallback item.date may be a Firestore Timestamp object, causing coalesceDate to return mixed types (ISO string vs object). Use normalizeTimestamp for the fallback (and import it) or drop the raw fallback to ensure a consistent ISO string return type.
| // Firestore snapshot with .seconds property | ||
| if (value.seconds !== undefined) { | ||
| try { | ||
| return new Date(value.seconds * 1000).toISOString(); |
There was a problem hiding this comment.
[nitpick] When converting Firestore timestamp objects, include nanoseconds to avoid precision loss for ordering: new Date(value.seconds * 1000 + Math.floor((value.nanoseconds || 0) / 1e6)).toISOString().
| return new Date(value.seconds * 1000).toISOString(); | |
| return new Date(value.seconds * 1000 + Math.floor((value.nanoseconds || 0) / 1e6)).toISOString(); |
| ...normalized, | ||
| // Override imageUrl default if it's empty | ||
| imageUrl: normalized.imageUrl || DEFAULT_IMAGE_URL, | ||
| // Add UI-specific fields | ||
| reporter, | ||
| claimed, | ||
| claimedAt, | ||
| claimedBy, | ||
| coordinates, | ||
| }; |
There was a problem hiding this comment.
[nitpick] Previously this wrapper defaulted location to 'Unknown' for UI display; now missing locations will be an empty string (normalizeItem returns ''), while the error path still sets 'Unknown'. For consistent UI, consider adding location: normalized.location || 'Unknown' here.
|
Embers2512
left a comment
Embers2512
left a comment
There was a problem hiding this comment.
Everything looks great, the test works properly and good job on removing the code duplication across the codebase. Your comments are immensely helpful for me to understand, good job 👍
📌 Description
What does this PR do?
Centralizes all Firestore data transformation and timestamp normalization logic into a single services/firestoreNormalizer.js file to eliminate duplication and inconsistencies across the app.
Why is it needed?
Previously, multiple components handled Firestore data differently, causing inconsistent formats, repeated code, and harder maintenance. This refactor standardizes and simplifies data handling.
🔗 Related Issue(s)
Closes #102
✅ Changes
[x] Refactor
🧪 How to Test
Run existing unit and integration tests — all should pass.
Verify data displays correctly in Feed, Messages, and Profile pages.
Check that timestamps and field names (e.g., imageURL, date) appear consistent across all views.