Skip to content
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

New View: Server Stats #3335

Open
wants to merge 21 commits into
base: master
Choose a base branch
from
Open

Conversation

Vito0912
Copy link
Contributor

Based on #3330

This PR introduces a new page titled "All Libraries Stats," which mirrors the existing "Library Stats" view but combines data from all libraries.

We might want to consider refactoring the design since this results in some duplicated code. While there are slight modifications, primarily in the computed section, much of the functionality remains the same.

Image:
image

@faush01
Copy link
Contributor

faush01 commented Aug 26, 2024

cant this just be a toggle on the stats page to include all libraries?
having multiple pages for stats like this may be confusing.

@Vito0912
Copy link
Contributor Author

cant this just be a toggle on the stats page to include all libraries?
having multiple pages for stats like this may be confusing.

I wouldn't recommend making this change. Although the design may appear similar, the "library stats" are actually located on the left-hand side of the current page, not in the settings, since they pertain specifically to this library. Having the label in the settings is, in my opinion, incorrect (and frustrating when navigating through pages and getting redirected), since it was recently moved.

Since "all libraries stats" are independent of the selected libraries, I suggest removing the incorrect button from the settings instead and having a button for all stats.

@Vito0912
Copy link
Contributor Author

Vito0912 commented Jan 1, 2025

This is now an uniform view:
grafik
grafik

Genres, Authors, Longest Items (hrs) and Largest Items are merged together and using the biggest values across

@advplyr
Copy link
Owner

advplyr commented Jan 1, 2025

I'm not a fan of merging podcast library and audiobook library stats and using the same stat info.

What is the motivation for an all library stats page? What is the data you are actually interested in seeing across all libraries?

@Vito0912
Copy link
Contributor Author

Vito0912 commented Jan 1, 2025

Are there specific reasons for this?

Generally, this overview should be an overview of all media on the server.
I've been planning to split my libraries for a while, but then I'd lose the statistics about total items, authors etc. broken down across everything.

In principle, I think it would be a quick task to differentiate between podcast and book media types, and then add up all podcasts for a podcast library and all books for book libraries. But I don't quite understand exactly why (Yes, podcasts are usually longer and larger), but that's basically irrelevant for the overall overview. Perhaps, and this would solve the problem, each episode could be considered as an item for the statistics.

But I am open to change that

@nichwall
Copy link
Contributor

nichwall commented Jan 1, 2025

I did some research about the CodeQL failure because that seems to be unrelated to this PR. For reference, the error is from ApiRouter.js, where the matching for paths is case sensitive. I believe this really only matters for the API Cache Manager and isn't really an error, but probably something good to handle globally.

I've found two main ways to fix it. Both are working in my local testing.

  1. Convert paths to lowercase before matching
    // Middleware to convert request paths to lowercase
    const caseInsensitivePaths = (req, res, next) => {
      req.url = req.url.toLowerCase();
      next();
    };

    // Apply the middleware to all routes
    this.router.use(caseInsensitivePaths);

    // Define all of the existing routes
    this.router.get(/^\/libraries/, this.apiCacheManager.middleware)
    this.router.post('/libraries', LibraryController.create.bind(this))
    // rest of the routes...
  1. Change the regex to be case insensitive.
this.router.get(/^\/libraries$/i, LibraryController.allStats.bind(this));

@nichwall
Copy link
Contributor

nichwall commented Jan 2, 2025

Also, responding to the design thing. I think having a view for all server stats together makes sense, but I agree it feels a bit weird to have stats from two different formats (books and podcasts) showing up together.

Maybe it makes sense for the "server stats" to show up on the Libraries page, like below all libraries? I don't think it really fits with this page, but also feel like it's too little information for its own "server stats page".
Screenshot from 2025-01-01 17-10-14

We could also make the server stats page more generic, and just show library summaries in a table instead of showing all of the "percent breakdown" for genres, size, top authors, etc. To view the "top authors" or "top genres" for a library, you could click on the library name and navigate to the stats page for that library.
PXL_20250102_001650535 MP

@Vito0912 Vito0912 changed the title Feature - New view: merged all stats from all libraries New View: Server Stats Jan 2, 2025
@nichwall
Copy link
Contributor

nichwall commented Jan 3, 2025

Stealing the image you posted on Discord for reference...
image

Just tested it and it is working great for me. The only thing I noticed is that if you are in one library click on a different library, the "library stats" page goes to the current library page instead of the library you clicked on. For example, regardless of if you click on "Books" or "Podcasts" in the above image, it will go to the "Audiobooks" library stats.

@Vito0912
Copy link
Contributor Author

Vito0912 commented Jan 3, 2025

Waking up to an email starting with "Stealing the image you posted" ^^.

I fixed the clicking issue. Now it should switch and display the correct library. @advplyr I'm not sure if this is the proper way to do it—I just borrowed the code from the library switcher.

I also reverted the changes I made to the library stats view.

@nichwall
Copy link
Contributor

nichwall commented Jan 3, 2025

Waking up to an email starting with "Stealing the image you posted" ^^.

Apologies, didn't think about how that would show up in an e-mail.

Just tested and switching to the correct library is working for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants