-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Creates activity feed on main "My Books" page #11645
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Creates activity feed on main "My Books" page #11645
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are accessibility issues in these changes.
| $def activity_card_body(book_title, author_name, cover_url, subtitle=""): | ||
| <div class="activity-feed-card__body"> | ||
| <div class="activity-feed-card__cover-image"> | ||
| <img src="$cover_url"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This image is missing a text alternative. This is a problem for people using screen readers.
| $ title += ' (0)' | ||
| <div class="carousel-section-header"> | ||
| <a class="li-title-desktop" name="$key" href="$url">$title (0)<img class="icon-link__image li-count" src="/static/images/icons/right-chevron.svg"></a> | ||
| <a class="li-title-desktop" name="$key" href="$url">$title<img class="icon-link__image li-count" src="/static/images/icons/right-chevron.svg"></a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This image is missing a text alternative. This is a problem for people using screen readers.
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds an activity feed feature to the "My Books" page that displays up to three recent bookshelf events from either followed users or trending activity across the library.
Key Changes:
- Introduces a new
ActivityFeedclass to manage feed data retrieval with caching - Adds a
PubSub.is_following()method to determine if a user follows others - Implements async-loading of the activity feed via JavaScript with retry functionality
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
static/css/components/mybooks.less |
Imports the new activity feed stylesheet |
static/css/components/activity-feed.less |
Defines styling for activity feed cards and layout |
openlibrary/templates/account/mybooks.html |
Adds activity feed container with loading indicator and retry affordance |
openlibrary/templates/account/activity_feed.html |
Template for rendering individual feed cards with book details and user information |
openlibrary/plugins/upstream/mybooks.py |
Implements ActivityFeed class with caching logic for personalized and trending feeds |
openlibrary/plugins/openlibrary/partials.py |
Adds ActivityFeedPartial handler for async feed data requests |
openlibrary/plugins/openlibrary/js/index.js |
Triggers async loading of activity feed on page load |
openlibrary/plugins/openlibrary/js/activity-feed/index.js |
Handles fetching and rendering activity feed with error recovery |
openlibrary/core/helpers.py |
Adds compact date formatting functionality for displaying relative timestamps |
openlibrary/core/follows.py |
Implements is_following() method to check if a user follows anyone |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| $ shelf_url = "%s/books%s" % (user_key, shelf_id_to_path_segment[shelf_id]) | ||
| $ shelf_name = shelf_id_to_name[shelf_id] |
Copilot
AI
Dec 31, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No error handling for invalid shelf_id values. If the bookshelf_id is not one of the expected values (1, 2, 3), the dictionary lookups on lines 43-44 will raise a KeyError. Consider adding validation or using dict.get() with a default value.
| $ shelf_url = "%s/books%s" % (user_key, shelf_id_to_path_segment[shelf_id]) | |
| $ shelf_name = shelf_id_to_name[shelf_id] | |
| $ shelf_url = "%s/books%s" % (user_key, shelf_id_to_path_segment.get(shelf_id, '')) | |
| $ shelf_name = shelf_id_to_name.get(shelf_id, _('Books')) |
| and item['username'] != username | ||
| and has_public_reading_log(item['username']) | ||
| ): |
Copilot
AI
Dec 31, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The caching key doesn't include the username, which means all users will share the same cached trending feed. This could lead to user A seeing the cached results intended for user B. The key should include the username to ensure each user gets the appropriate filtered results (excluding their own username from the feed).
| SELECT 1 | ||
| FROM follows | ||
| WHERE subscriber=$subscriber | ||
| ) |
Copilot
AI
Dec 31, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SQL query uses a bare SELECT EXISTS which returns a boolean, but the result access pattern expects a dictionary with an 'exists' key. This may work in the database library being used, but it's worth verifying that result[0].get('exists', False) will work correctly with the query result format. Consider explicitly naming the column with AS for clarity.
| ) | |
| ) AS exists |
| ) | ||
|
|
||
| TIME_DELTA_UNITS = ( | ||
| TimeDeltaUnit('year', 'y', 3600 * 24 * 365), | ||
| TimeDeltaUnit('month', 'm', 3600 * 24 * 30), | ||
| TimeDeltaUnit('week', 'w', 3600 * 24 * 7), | ||
| TimeDeltaUnit('day', 'd', 3600 * 24), | ||
| TimeDeltaUnit('hour', 'h', 3600), | ||
| TimeDeltaUnit('minute', 'm', 60), |
Copilot
AI
Dec 31, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicate short_name 'm' for both month and minute units will cause ambiguity in the output. Users won't be able to distinguish between months and minutes when viewing compact timestamps. Consider using unique abbreviations like 'mo' for month or 'min' for minute.
| $def activity_card_body(book_title, author_name, cover_url, subtitle=""): | ||
| <div class="activity-feed-card__body"> | ||
| <div class="activity-feed-card__cover-image"> | ||
| <img src="$cover_url"> |
Copilot
AI
Dec 31, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing alt attribute text for the book cover image, which is important for accessibility. The image should have descriptive alt text such as the book title or "Cover image for [book title]".
| <img src="$cover_url"> | |
| <img src="$cover_url" alt="$book_title"> |
|
|
||
| time_delta_unit = TIME_DELTA_UNITS[-1] | ||
| for tdu in TIME_DELTA_UNITS: | ||
| if delta.total_seconds() > tdu.seconds_per_unit: | ||
| time_delta_unit = tdu | ||
| break | ||
|
|
||
| result = f'{int(delta.total_seconds()) // time_delta_unit.seconds_per_unit}{time_delta_unit.short_name}' |
Copilot
AI
Dec 31, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The loop will always select the first unit where delta exceeds the unit's seconds, but it should select the largest appropriate unit. For example, a 2-day time delta would incorrectly be formatted as seconds/minutes/hours instead of days. The logic should iterate in reverse order or find the largest unit that fits, not the first one that the delta exceeds.
| time_delta_unit = TIME_DELTA_UNITS[-1] | |
| for tdu in TIME_DELTA_UNITS: | |
| if delta.total_seconds() > tdu.seconds_per_unit: | |
| time_delta_unit = tdu | |
| break | |
| result = f'{int(delta.total_seconds()) // time_delta_unit.seconds_per_unit}{time_delta_unit.short_name}' | |
| total_seconds = int(delta.total_seconds()) | |
| time_delta_unit = TIME_DELTA_UNITS[-1] | |
| for tdu in TIME_DELTA_UNITS: | |
| if total_seconds >= tdu.seconds_per_unit: | |
| time_delta_unit = tdu | |
| break | |
| result = f'{total_seconds // time_delta_unit.seconds_per_unit}{time_delta_unit.short_name}' |
| Bookshelves.add_solr_works(logged_books) | ||
|
|
Copilot
AI
Dec 31, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The condition 'len(feed) > 2' allows 3 items to be added before breaking, but the docstring states the method returns "up to three" items, which matches the behavior. However, this should use '>= 3' for clarity and to match the pubsub feed limit of 3.
| $ work = i.get('work') | ||
| $ book_title = work.get('title') |
Copilot
AI
Dec 31, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing error handling for when 'work' is None or doesn't contain required fields like 'title'. If 'work.get('title')' returns None, it will be passed to the template and could cause display issues. Consider validating that required fields exist before processing the item.
| $ work = i.get('work') | |
| $ book_title = work.get('title') | |
| $ work = i.get('work') or {} | |
| $ book_title = work.get('title') or _('Untitled work') |
| } | ||
|
|
||
| // Hydrate retry button | ||
| retryButton.addEventListener('click', async () => { |
Copilot
AI
Dec 31, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing preventDefault() call in the click event handler. When the retry link is clicked, the default anchor behavior should be prevented to avoid page navigation to '#'.
| retryButton.addEventListener('click', async () => { | |
| retryButton.addEventListener('click', async (event) => { | |
| event.preventDefault() |
| export async function initActivityFeedRequest(elem) { | ||
| const fullPath = window.location.pathname | ||
| const splitPath = fullPath.split('/') | ||
| const username = splitPath[2] // Assumes an activity feed can only appear on the patron's "My Books" page |
Copilot
AI
Dec 31, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Magic number 2 used for extracting username from path. This assumes a specific URL structure ('/people/username/books') but is fragile if the path structure changes. Consider adding a comment explaining the expected path structure or using a more robust method like matching against a regex pattern.
| const username = splitPath[2] // Assumes an activity feed can only appear on the patron's "My Books" page | |
| // Expected path structure for a "My Books" activity feed page: | |
| // "/people/{username}/books" | |
| // After splitting on "/", this yields: | |
| // splitPath[0] === "" (leading slash) | |
| // splitPath[1] === "people" | |
| // splitPath[2] === "{username}" | |
| // splitPath[3] === "books" | |
| const USERNAME_PATH_INDEX = 2 | |
| const username = splitPath[USERNAME_PATH_INDEX] // Assumes an activity feed can only appear on the patron's "My Books" page |
Closes #10242
Supplants #11391
Adds an activity feed containing up to three cards to the main "My Books" page.
Technical
A new
PubSubmethod namedis_followinghas been created for the purpose of determining if a patron follows any other person on the site.Testing
Screenshot
Stakeholders