Ordering feed and profile posts by published_at
#403
Conversation
WalkthroughThe changes introduce several modifications across the project. A wait condition of 1000 milliseconds is added in specific feed scenarios, affecting the execution timing of subsequent steps. In the step definitions, the published date for generated objects now uses the current date and time instead of a fixed timestamp. A new integration test case has been added to confirm that feed items are correctly sorted by their published date, verifying that the order is as expected. Additionally, the SQL queries in the feed service have been updated: the feed data retrieval now uses the Possibly related PRs
Suggested reviewers
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (9)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/feed/feed.service.ts (1)
152-152:❓ Verification inconclusive
Improved feed ordering with published_at instead of ID
The changes consistently update the pagination and ordering logic to use
published_atinstead of feed ID, which aligns with the PR objective. This approach provides a more intuitive chronological ordering of posts.Consider adding a secondary sorting criterion (like ID) to handle cases where multiple posts have the exact same timestamp:
- .orderBy('feeds.published_at', 'desc') + .orderBy('feeds.published_at', 'desc') + .orderBy('feeds.id', 'desc')Also, ensure there's an appropriate index on the
feeds.published_atcolumn to maintain query performance, especially since this is now the primary sorting field used for pagination.Also applies to: 183-183, 186-186, 202-202
🏁 Script executed:
#!/bin/bash # Check if an index exists on feeds.published_at echo "Checking for index on feeds.published_at column" rg -A 5 -B 5 "CREATE.*INDEX.*feeds.*published_at" --glob="*.sql"Length of output: 171
Action Required: Verify Index on
feeds.published_atand Add Secondary Sorting
The PR’s changes correctly update the ordering logic to usepublished_at, and the suggestion to add a secondary sort onfeeds.idhelps resolve tie situations. However, the automated search for an index onfeeds.published_atdid not yield any output. Please manually verify that an appropriate index exists on this column (or add one if missing) to ensure efficient pagination.
- Feed Service Changes (applies to lines 152, 183, 186, 202):
- .orderBy('feeds.published_at', 'desc') + .orderBy('feeds.published_at', 'desc') + .orderBy('feeds.id', 'desc')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
features/feed.feature(2 hunks)features/step_definitions/stepdefs.js(2 hunks)src/feed/feed.service.integration.test.ts(1 hunks)src/feed/feed.service.ts(3 hunks)src/post/post.service.ts(6 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
src/post/post.service.ts (1)
features/step_definitions/stepdefs.js (1) (1)
accountId(536:553)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build, Test and Deploy
🔇 Additional comments (14)
features/feed.feature (2)
25-25: Good addition of a wait condition to ensure consistent test results.Adding this wait ensures proper sequencing of events, allowing the system to correctly establish the timestamp order for the feed sorting test case.
38-42: Properly implemented wait conditions for the pagination test.These wait periods ensure that each note has a distinct published timestamp, which is critical for the updated feed sorting logic based on
published_at. This change aligns with the updates in the feed service SQL queries.features/step_definitions/stepdefs.js (3)
292-292: LGTM: Updated Article published timestamp to use current date.Changed from using a static timestamp to a dynamic one, which better reflects real-world usage and supports the new sorting by
published_at.
310-310: LGTM: Updated Note published timestamp to use current date.Same improvement as for Articles - using dynamic timestamps instead of static ones. This ensures proper chronological ordering in the feed tests.
1656-1658: Implementation looks good for the wait step.This step definition properly implements the wait functionality needed for the feature test changes, allowing for controlled time intervals between post creations.
src/post/post.service.ts (8)
254-258: Improved formatting of CASE statement.Consistent formatting improves readability while maintaining the same functionality.
264-268: Improved formatting of CASE statement for reposted posts.Similar formatting improvement as the previous CASE statement, maintaining consistency across the codebase.
282-283: Good addition of a comment for the published_at field.Clear documentation of the field's purpose for sorting, which helps other developers understand the query structure.
310-310: Switched to using published_at for date sorting.This change aligns with the PR's objective to sort by
published_atrather than creation date.
337-340: Updated repost date field for consistent date handling.Changed to use the repost's creation time as the publication date for sorting, ensuring consistent ordering based on when content appears in the feed.
345-345: Updated ORDER BY clause to use published_date.This is a key change that implements the core functionality of sorting feed items by their published date.
377-377: Updated pagination condition to use published_date.This ensures pagination works correctly with the new sorting order based on published date.
394-394: Updated nextCursor to use published_date.This change ensures that pagination continues correctly from where the previous page ended, using the published date as the reference point.
src/feed/feed.service.integration.test.ts (1)
204-276: Great addition of a test case to verify feed sorting by published_at.This test thoroughly verifies that feed items are properly sorted by their published date. It creates a realistic scenario with:
- Multiple posts with different publication dates
- A repost with a newer date
- Verification of the expected ordering (newest first)
The test correctly validates that reposts are sorted based on their repost timestamp rather than the original post's publication date, which is important for user experience.
features/feed.feature
Outdated
| @@ -22,6 +22,7 @@ Feature: Feed | |||
| Given a "Create(Note)" Activity "Note1" by "Alice" | |||
| And "Alice" sends "Note1" to the Inbox | |||
| And "Note1" is in our Inbox | |||
| And We waited for 1000 milliseconds | |||
There was a problem hiding this comment.
Is this so the published at date is later? It would be nice to not at a 3 second delay to the tests if we could use something like https://sinonjs.org/releases/latest/fake-timers/ or similar to mock the Date in the stepdefs.js file
There was a problem hiding this comment.
I tried adding the fake timer but there's a problem with that. Any mock or fake timer will only work within the scope of the stepdefs.js file. However, in this case, the posts are created via the API. Fake timers only affect time-related functions inside the test process (i.e., within the step definitions), but not inside the API server. The API server will still use the real new Date() at the moment the request is processed.
There was a problem hiding this comment.
Aha okay, I think we have a problem there then, they should use the passed published property if it exists, what do you think?
There was a problem hiding this comment.
Talked about this is in slack. Used fake timer for feed tests. For profile tests, tweak the tests for now to use the fake timer, will discuss a solution around improving tests separately.
No description provided.