Skip to content

Conversation

koukibadr
Copy link
Contributor

What is it?

  • Bugfix (user facing)
  • Feature (user facing)
  • Codebase improvement (dev facing)
  • Meta improvement to the project (dev facing)

Description of the changes in your PR

  • Handle display search results when nextPage is null on Albums mode

Before/After Screenshots/Screen Record

  • Before:
before_fix.mov
  • After:
after_fix.mov

Fixes the following issue(s)

Due diligence

@github-actions github-actions bot added the size/small PRs with less than 50 changed lines label Jul 5, 2025
@ShareASmile ShareASmile added bug Issue is related to a bug search Anything related to the search function labels Jul 5, 2025
@snaik20
Copy link
Contributor

snaik20 commented Jul 6, 2025

Thanks for this.

The NextPage field is used in few more places. Could you make sure those places are also handling the nullability correctly.

Also, the extractor function also needs to correctly set the @Nullable annotation to indicate that the returned value can be null. It would be nice if you could help with that as well (TeamNewPipe/NewPipeExtractor#1312). Thanks

@koukibadr
Copy link
Contributor Author

@snaik20 yes sure I'll check these points and update the PR if needed then

@snaik20 snaik20 self-requested a review July 8, 2025 17:33
@snaik20 snaik20 force-pushed the fix/search/resolve-search-albums-crash branch from 7d494c0 to 799298a Compare July 19, 2025 18:51
…ady handled in these areas so no other changes needed
@snaik20 snaik20 merged commit a4bd82b into TeamNewPipe:dev Jul 22, 2025
5 checks passed
Copy link
Contributor

@TobiGr TobiGr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR!

However, I think the provided solution does not solve the underlying problem. I opened #12455 to fix it.

nextPage = result.getNextPage();

if (!result.getErrors().isEmpty()) {
if (!result.getErrors().isEmpty() && nextPage != null) {
Copy link
Contributor

@TobiGr TobiGr Jul 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, why is the nextPageInfo included in the error?
The nextPage might be null, but that does not mean that the extraction of the current page does not have any errors. We'd still want to get error reports in case the nextPage couldn't be extracted. The reassignment of nextPage needs to happen after the error report is generated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Issue is related to a bug search Anything related to the search function size/small PRs with less than 50 changed lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Crashed during Scrolling Search

4 participants