10760/add global book filter #11331

BearSunny · 2025-10-07T14:15:18Z

This PR aims to create a global filter with the following flow: UI change -> Save to localStorage for persistence across page loads
-> Data are sent to backend -> Backend sets cookies -> On success, trigger carousels reload -> Carousels read preferences from localStorage -> Carousels use preferences to fetch filtered data from Solr.

Technical

This implementation already confirms that UI interaction and cookies work correctly. However, since I haven't tested it on carousels with loaded books yet, I cannot confirm that carousels are able to load properly.

P/S: I sincerely apologize for my inactivity over the past month since I was away for military service. If you could kindly share the instructions on how to load books for testing, I’d really appreciate it. Thank you so much!

for more information, see https://pre-commit.ci

Copilot

Pull Request Overview

This PR implements a global book filter system that allows users to filter books by type (readable/preview/all), language, and publication date range. The filter preferences persist across page loads using localStorage and are synchronized with backend cookies to maintain consistency.

Added client-side preference management with localStorage persistence and cookie synchronization
Created a slide-out filter panel UI integrated into the top navigation bar
Modified carousel components to respect global filter preferences when loading book data

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
static/js/preferences.js	Core preference management functions for getting/setting filters and cookie synchronization
static/js/preferences-handler.js	DOM event handlers for the filter panel UI interactions
openlibrary/templates/site/alert.html	Added filter panel UI with styling and removed language dropdown from top bar
openlibrary/plugins/upstream/account.py	Backend endpoint to handle preference updates and set cookies
openlibrary/plugins/openlibrary/js/carousel/Carousel.js	Modified carousel to use global preferences for filtering book data
openlibrary/i18n/messages.pot	Updated translation references after removing language dropdown

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

static/js/preferences-handler.js

Copilot · 2025-10-16T18:10:19Z

static/js/preferences.js

+                Math.max(1800, Math.min(2025, isNaN(startYear) ? 1900 : startYear)),
+                Math.max(1800, Math.min(2025, isNaN(endYear) ? 2025 : endYear))


The year range validation logic uses hardcoded values (1800, 2025, 1900) that appear in multiple places. Consider defining these as constants at the top of the file for better maintainability.

openlibrary/plugins/upstream/account.py

openlibrary/templates/site/alert.html

mekarpeles · 2025-10-16T18:20:56Z

Looking at the javascript CI:

[lint:js]   1:32  error  'getGlobalPreferences' is defined but never used       no-unused-vars
[lint:js]   1:54  error  'onGlobalPreferencesChange' is defined but never used  no-unused-vars
[lint:js]   1:81  error  'mapPreferencesToBackend' is defined but never used    no-unused-vars
[lint:js]   3:7   error  'selectedLang' is assigned a value but never used      no-unused-vars
[lint:js] 
[lint:js] /home/runner/work/openlibrary/openlibrary/static/js/preferences.js
[lint:js]    6:9  error  Unexpected console statement  no-console
[lint:js]   22:9  error  Unexpected console statement  no-console
[lint:js]   43:9  error  Unexpected console statement  no-console
[lint:js]   60:9  error  Unexpected console statement  no-console
[lint:js]   71:9  error  Unexpected console statement  no-console

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

BearSunny · 2025-11-24T01:57:16Z

Hi @mekarpeles, I’ve implemented the suggested fixes from my last PR. While testing by loading some books, it looks like they aren’t reflecting the updated preference logic. Could you please let me know if there are additional steps I should take, or if there might be a conflict between the new implementation and the existing codebase?

mekarpeles · 2025-11-25T14:53:28Z

openlibrary/templates/site/alert.html

 $ supported_languages = get_supported_languages()
 $ active_ui_lang = supported_languages.get(lang) or supported_languages.get('en')

+<style>


Fine approach for prototyping

Before merge, we'll want to move these styles to the appropriate location with /static/less :)

https://github.com/internetarchive/openlibrary/wiki/frontend-Guide#working-with-css

Hi @mekarpeles, let me read through the documentation as well as separate our Language option and our filter.

BearSunny · 2025-12-02T04:11:50Z

Hey @mekarpeles, I have migrated this <style> to the proper CSS files and here's the result for now:

Could you please review the modified files and specifically focus on the carousel loading after user has selected their preferences in the Filter modal since I'm not sure if the Solr queries are correct?

In Carousel.js:

const handleGlobalFilterChange = (backendParams) => {
              loadMore.extraParams = backendParams;
              if (loadMore.pageMode === 'page') {
                  loadMore.page = 1;
              } else {
                  loadMore.page = 0;
              }
              loadMore.allDone = false;
              this.clearCarousel();
              this.fetchPartials();
          };

          document.addEventListener('global-preferences-changed', (ev) => {
              const backendParams = mapPreferencesToBackend(ev.detail);
              handleGlobalFilterChange(backendParams);
          });

          // On initial load, fetch with global preferences
          const initialPrefs = getGlobalPreferences();
          const initialBackendParams = mapPreferencesToBackend(initialPrefs);
          handleGlobalFilterChange(initialBackendParams);
}

Frontend mapping in preferences.js:

export function mapPreferencesToBackend(prefs) {
    const params = {
        formats: prefs.mode === 'fulltext' ? 'has_fulltext' 
                 : prefs.mode === 'preview' ? 'ebook_access' 
                 : null,
        first_publish_year: prefs.date
    };
    
    if (prefs.language && prefs.language !== 'all') {
        params.languages = [prefs.language];
    }
    
    return params;
}

Backend processing in account.py:

prefs = {
            'mode': d.get('mode', 'all'),
            'language': d.get('language', 'en'),
            'date': d.get('date', [1900, 2025]),
        }

# Transform to backend format
backend_prefs = {
    'formats': (
        'has_fulltext'
        if prefs['mode'] == 'fulltext'
        else 'ebook_access' if prefs['mode'] == 'preview' else None
    ),
    'first_publish_year': prefs['date'],
}
if prefs['language'] != 'all':
    backend_prefs['languages'] = [prefs['language']]

Thank you so much!

Copilot

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 19 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-02T04:20:08Z

static/js/preferences.js

+        return {
+            mode: parsed.global?.mode || 'all',
+            language: parsed.global?.language || 'all',
+            date: parsed.global?.date || [1900, 2025]


The hardcoded year range [1900, 2025] should be made a constant for maintainability and consistency. This value appears multiple times throughout the codebase (lines 18, 21, 46, 51-52, 65, 100) and will need annual updates. Consider defining const DEFAULT_YEAR_RANGE = [1900, new Date().getFullYear()] at the top of the file.

Copilot · 2025-12-02T04:20:08Z

openlibrary/templates/site/alert.html

+<script type="module">
+  import '/static/js/preferences-handler.js';
+
+  const filterTrigger = document.getElementById('filter-panel-trigger');


[nitpick] Inconsistent naming: The CSS class is filter-panel-trigger (line 24) but the variable name in JavaScript is filterTrigger (line 78). While this works, for better searchability across the codebase, consider using a consistent hyphenated or camelCase naming convention.

Copilot · 2025-12-02T04:20:08Z

openlibrary/plugins/openlibrary/js/carousel/Carousel.js

+            const initialBackendParams = mapPreferencesToBackend(initialPrefs);
+            handleGlobalFilterChange(initialBackendParams);
        }


The carousel initialization logic unconditionally calls handleGlobalFilterChange on initial load (line 149), which will immediately clear and reload the carousel even if no preferences are set. This could cause unnecessary API calls and poor user experience. Consider only applying filters if they differ from defaults or if the carousel hasn't already loaded data.

Suggested change

const initialBackendParams = mapPreferencesToBackend(initialPrefs);

handleGlobalFilterChange(initialBackendParams);

}

const defaultPrefs = {}; // Define default preferences as an empty object or as appropriate

const initialBackendParams = mapPreferencesToBackend(initialPrefs);

const defaultBackendParams = mapPreferencesToBackend(defaultPrefs);

// Only reload if preferences differ from defaults or carousel is empty

if (

JSON.stringify(initialBackendParams) !== JSON.stringify(defaultBackendParams) ||

this.slick.$slides.length === 0

) {

handleGlobalFilterChange(initialBackendParams);

}

Copilot · 2025-12-02T04:20:09Z

static/css/components/alert.less

+@white: #fff;
+@border: #ccc;
+@shadow: rgba(0, 0, 0, 0.15);
+@header-bg: #f8f9fa;
+@header-border: #e9ecef;
+@text-dark: #333;
+@text-mid: #666;
+@placeholder: #999;
+@primary: #4a90e2;
+@primary-hover: #357abd;
+@focus-shadow: rgba(74, 144, 226, 0.2);
+@focus-shadow-strong: rgba(74, 144, 226, 0.3);
+@z-index-panel: 1000;
+@black: #000;


[nitpick] Hardcoded color values are defined as LESS variables but should use the project's existing color system for consistency. Duplicating color definitions (like @white, @black, @border, etc.) across files can lead to inconsistencies. Consider importing and reusing existing color variables from the project's design system instead of redefining them.

Suggested change

@white: #fff;

@border: #ccc;

@shadow: rgba(0, 0, 0, 0.15);

@header-bg: #f8f9fa;

@header-border: #e9ecef;

@text-dark: #333;

@text-mid: #666;

@placeholder: #999;

@primary: #4a90e2;

@primary-hover: #357abd;

@focus-shadow: rgba(74, 144, 226, 0.2);

@focus-shadow-strong: rgba(74, 144, 226, 0.3);

@z-index-panel: 1000;

@black: #000;

@import (reference) "../../design-system/colors.less";

@shadow: rgba(0, 0, 0, 0.15);

@focus-shadow: rgba(74, 144, 226, 0.2);

@focus-shadow-strong: rgba(74, 144, 226, 0.3);

@z-index-panel: 1000;

Copilot · 2025-12-02T04:20:09Z

openlibrary/templates/site/alert.html

+      <div class="language-component header-dropdown iabar-mobile">
          <summary>


Missing opening <details> tag: The code shows a closing </details> tag on line 22, but the corresponding opening tag appears to have been removed in the changes (line 14 shows a <div> instead). The <summary> element on line 15 must be inside a <details> element. Add <details> after line 14 or restore the removed opening tag.

Copilot · 2025-12-02T04:20:12Z

openlibrary/plugins/upstream/account.py

+            logger.info("Parsed preferences data: %s", d)
+        except Exception as e:
+            logger.error("Failed to process preferences update: %s", str(e))
+            return json.dumps({"error": "Failed to process request"})


The error handler returns a JSON string directly instead of using delegate.RawText with proper content type like the success case does (lines 808-811). This inconsistency could cause issues with the frontend expecting JSON.

Suggested fix:

return delegate.RawText( json.dumps({"error": "Failed to process request"}), content_type="application/json" )

Suggested change

return json.dumps({"error": "Failed to process request"})

return delegate.RawText(

json.dumps({"error": "Failed to process request"}),

content_type="application/json"

)

Copilot · 2025-12-02T04:20:12Z

openlibrary/plugins/openlibrary/js/carousel/Carousel.js

 import 'slick-carousel';
 import '../../../../../static/css/components/carousel--js.less';
 import { buildPartialsUrl } from  '../utils.js';
+import { getGlobalPreferences, mapPreferencesToBackend } from '../../../../../../openlibrary/static/js/preferences.js'


The import path '../../../../../../openlibrary/static/js/preferences.js' is extremely fragile and appears incorrect. It goes up 7 levels and then into openlibrary/static, which suggests the file structure understanding may be wrong. The path should likely be '../../../../../static/js/preferences.js' or use an absolute path/alias. This will cause a runtime error when the module cannot be found.

Suggested change

import { getGlobalPreferences, mapPreferencesToBackend } from '../../../../../../openlibrary/static/js/preferences.js'

import { getGlobalPreferences, mapPreferencesToBackend } from '../../../../../static/js/preferences.js'

Copilot · 2025-12-02T04:20:12Z

openlibrary/templates/site/alert.html

+      filterPanel.classList.add('hidden');
+      filterTrigger.setAttribute('aria-expanded', 'false');
+    }
+  });


Missing keyboard support: The filter panel can be closed by clicking outside (lines 101-107), but there's no keyboard equivalent (e.g., pressing Escape key). This creates an accessibility barrier for keyboard-only users. Consider adding an event listener for the Escape key to close the panel.

Suggested change

});

});

// Close panel with Escape key for accessibility

document.addEventListener('keydown', (e) => {

// Only close if panel is open and Escape is pressed

if (!filterPanel.classList.contains('hidden') && (e.key === 'Escape' || e.key === 'Esc')) {

filterPanel.classList.remove('show');

filterPanel.classList.add('hidden');

filterTrigger.setAttribute('aria-expanded', 'false');

filterTrigger.focus();

}

});

Copilot · 2025-12-02T04:20:13Z

static/js/preferences-handler.js

+        }).then(res => res.json()).then(() => {
+            // 3. Trigger local UI update (carousel reload)
+            updateAllCarousels();


Missing error handling: The fetch request lacks error handling for network failures or non-2xx responses. If the request fails, carousels will reload with new preferences even though the backend cookies weren't set, causing inconsistency between localStorage and server state.

Suggested fix:

fetch('/account/preferences', { method: 'POST', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({ ...prefs, redirect: false }) }).then(res => { if (!res.ok) throw new Error('Failed to save preferences'); return res.json(); }).then(() => { updateAllCarousels(); }).catch(err => { console.error('Error saving preferences:', err); // Consider showing user feedback });

Suggested change

}).then(res => res.json()).then(() => {

// 3. Trigger local UI update (carousel reload)

updateAllCarousels();

}).then(res => {

if (!res.ok) throw new Error('Failed to save preferences');

return res.json();

}).then(() => {

// 3. Trigger local UI update (carousel reload)

updateAllCarousels();

}).catch(err => {

console.error('Error saving preferences:', err);

// Optionally, show user feedback here

Copilot · 2025-12-02T04:20:13Z

openlibrary/plugins/upstream/account.py

+# Add a POST redirect for prefs from global filter
+class account_preferences(delegate.page):
+    path = "/account/preferences"
+    encoding = "json"
+
+    def POST(self):
+        logger.info("Received preferences update request")
+        try:
+            raw_data = web.data()
+            logger.info("Raw request data: %s", raw_data)
+            d = json.loads(raw_data)
+            logger.info("Parsed preferences data: %s", d)
+        except Exception as e:
+            logger.error("Failed to process preferences update: %s", str(e))
+            return json.dumps({"error": "Failed to process request"})
+        prefs = {
+            'mode': d.get('mode', 'all'),
+            'language': d.get('language', 'en'),
+            'date': d.get('date', [1900, 2025]),
+        }
+
+        # Transform to backend format
+        backend_prefs = {
+            'formats': (
+                'has_fulltext'
+                if prefs['mode'] == 'fulltext'
+                else 'ebook_access' if prefs['mode'] == 'preview' else None
+            ),
+            'first_publish_year': prefs['date'],
+        }
+        if prefs['language'] != 'all':
+            backend_prefs['languages'] = [prefs['language']]
+
+        expires = 3600 * 24 * 365
+        web.setcookie('ol_mode', prefs['mode'], expires=expires)
+        web.setcookie('ol_lang', prefs['language'], expires=expires)
+        web.setcookie('ol_date', ",".join(map(str, prefs['date'])), expires=expires)
+
+        if d.get('redirect', True):
+            raise web.seeother("/account")
+        else:
+            return delegate.RawText(
+                json.dumps({'status': 'ok', 'backend_prefs': backend_prefs}),
+                content_type="application/json",
+            )
+


The new account_preferences class lacks test coverage. Since this repository has comprehensive test coverage for the account module (see openlibrary/plugins/upstream/tests/test_account.py), this new API endpoint should have tests covering successful requests, error handling, cookie setting, and both redirect and non-redirect scenarios.

openlibrary/templates/site/alert.html

static/css/components/alert.less

BearSunny · 2026-01-15T13:50:53Z

Hi @mekarpeles,
I am so sorry for the long pause (again), but here's my update on code fixes. So far, I have implemented all 4 of your requested changes. I have also checked my implementation and noticed that my work right now mostly focuses on only the frontend. Specifically, alert.html displays the UI panel for users to select their preferences, which are then saved to localStorage and mapped to backend parameters. The key question here, though, is that I am not really clear how backend endpoints receive and apply these parameters. The crucial questions I need right now are:

Which file handles carousel data fetching? Where is the endpoint?
Is there an existing function that builds search queries for carousels that I should modify?
How do I format users' inputs for queries? What field names should I use?

Answering these 3 questions will allow me to continue updating the backend pipeline for our filtering system. For now, here's what we have so far:

Thank you so much for your response!

BearSunny added 3 commits October 7, 2025 20:13

complete all features for global book filter

bc8c06b

add carousel loading

8a0d571

final touch to UI and add debugging logs

17007c0

github-actions bot assigned cdrini Oct 7, 2025

[pre-commit.ci] auto fixes from pre-commit.com hooks

5de1e3b

for more information, see https://pre-commit.ci

mekarpeles requested a review from Copilot October 16, 2025 18:09

Copilot AI reviewed Oct 16, 2025

View reviewed changes

BearSunny and others added 6 commits October 18, 2025 20:09

Update static/js/preferences-handler.js

0846b48

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update openlibrary/plugins/upstream/account.py

e82fe65

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update openlibrary/templates/site/alert.html

6018a73

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

fix eslint errors

6b1222e

Final final touch to global filter feature

a15745b

fix eslint error 2

4f2d623

github-actions bot added the Needs: Response Issues which require feedback from lead label Nov 24, 2025

mekarpeles reviewed Nov 25, 2025

View reviewed changes

migrate <style> to static/css

bc0eac2

BearSunny requested a review from Copilot December 2, 2025 04:13

Copilot started reviewing on behalf of BearSunny December 2, 2025 04:14 View session

Copilot finished reviewing on behalf of BearSunny December 2, 2025 04:17

Copilot AI reviewed Dec 2, 2025

View reviewed changes

mekarpeles requested changes Dec 9, 2025

View reviewed changes

openlibrary/templates/site/alert.html Show resolved Hide resolved

openlibrary/templates/site/alert.html Outdated Show resolved Hide resolved

openlibrary/templates/site/alert.html Outdated Show resolved Hide resolved

static/css/components/alert.less Outdated Show resolved Hide resolved

Adding mek's required changes and fixing i18n bugs final v1

953673d

		Math.max(1800, Math.min(2025, isNaN(startYear) ? 1900 : startYear)),
		Math.max(1800, Math.min(2025, isNaN(endYear) ? 2025 : endYear))

-            const initialBackendParams = mapPreferencesToBackend(initialPrefs);
-            handleGlobalFilterChange(initialBackendParams);
-        }
+            const defaultPrefs = {}; // Define default preferences as an empty object or as appropriate
+            const initialBackendParams = mapPreferencesToBackend(initialPrefs);
+            const defaultBackendParams = mapPreferencesToBackend(defaultPrefs);
+            // Only reload if preferences differ from defaults or carousel is empty
+            if (
+                JSON.stringify(initialBackendParams) !== JSON.stringify(defaultBackendParams) ||
+                this.slick.$slides.length === 0
+            ) {
+                handleGlobalFilterChange(initialBackendParams);
+            }

		<div class="language-component header-dropdown iabar-mobile">
		<summary>

	import { getGlobalPreferences, mapPreferencesToBackend } from '../../../../../../openlibrary/static/js/preferences.js'
	import { getGlobalPreferences, mapPreferencesToBackend } from '../../../../../static/js/preferences.js'

-  });
+  });
+  // Close panel with Escape key for accessibility
+  document.addEventListener('keydown', (e) => {
+    // Only close if panel is open and Escape is pressed
+    if (!filterPanel.classList.contains('hidden') && (e.key === 'Escape' || e.key === 'Esc')) {
+      filterPanel.classList.remove('show');
+      filterPanel.classList.add('hidden');
+      filterTrigger.setAttribute('aria-expanded', 'false');
+      filterTrigger.focus();
+    }
+  });

-        }).then(res => res.json()).then(() => {
-            // 3. Trigger local UI update (carousel reload)
-            updateAllCarousels();
+        }).then(res => {
+            if (!res.ok) throw new Error('Failed to save preferences');
+            return res.json();
+        }).then(() => {
+            // 3. Trigger local UI update (carousel reload)
+            updateAllCarousels();
+        }).catch(err => {
+            console.error('Error saving preferences:', err);
+            // Optionally, show user feedback here

Uh oh!

10760/add global book filter #11331

Are you sure you want to change the base?

10760/add global book filter #11331

Uh oh!

Conversation

BearSunny commented Oct 7, 2025

Technical

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Copilot AI Oct 16, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mekarpeles commented Oct 16, 2025

Uh oh!

BearSunny commented Nov 24, 2025

Uh oh!

mekarpeles Nov 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mekarpeles Nov 25, 2025

Choose a reason for hiding this comment

Uh oh!

BearSunny Nov 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BearSunny commented Dec 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

BearSunny commented Jan 15, 2026

Uh oh!

Reviewers

Assignees

mekarpeles Nov 25, 2025 •

edited

Loading

BearSunny Nov 29, 2025 •

edited

Loading

BearSunny commented Dec 2, 2025 •

edited

Loading