Skip to content

Fix search showing limited results#1139

Open
carma12 wants to merge 1 commit into
freeipa:mainfrom
carma12:fix-search-limited-results
Open

Fix search showing limited results#1139
carma12 wants to merge 1 commit into
freeipa:mainfrom
carma12:fix-search-limited-results

Conversation

@carma12

@carma12 carma12 commented Jul 13, 2026

Copy link
Copy Markdown
Collaborator

Fix search from non-first pages returning
wrong or empty results

Reset pagination to page 1 and use startIdx: 0
when submitting a search. Previously, search used
the current page offset, which skipped matching
entries in the LDAP result set. Fixed across all
affected pages, and updated the corresponding
main-pages file.

Assisted-by: Claude noreply@anthropic.com

Summary by Sourcery

Reset search pagination and result indices so searches always start from the first page and return complete results across all affected listing pages.

Bug Fixes:

  • Fix searches performed from non-first pages returning empty or incomplete results by resetting to page 1.
  • Ensure LDAP-backed searches consistently query from the first result up to the current page size instead of using paginated offsets.

Documentation:

  • Update the main-pages walkthrough design document to reflect the corrected search pagination behavior.

@carma12 carma12 added the needs-review This PR is waiting on a review label Jul 13, 2026

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hey - I've left some high level feedback:

  • The submitSearchValue logic (resetting page to 1 and using startIdx: 0/stopIdx: perPage) is duplicated across many pages; consider extracting a shared hook or helper to keep this behavior consistent and easier to maintain.
  • Where the page number is shown in the UI, verify that resetting setPage(1) on search aligns with user expectations (e.g., when clearing search or changing filters) so pagination indicators don’t become inconsistent with the underlying data.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- The `submitSearchValue` logic (resetting page to 1 and using `startIdx: 0`/`stopIdx: perPage`) is duplicated across many pages; consider extracting a shared hook or helper to keep this behavior consistent and easier to maintain.
- Where the page number is shown in the UI, verify that resetting `setPage(1)` on search aligns with user expectations (e.g., when clearing search or changing filters) so pagination indicators don’t become inconsistent with the underlying data.

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

This was referenced Jul 13, 2026
@carma12 carma12 force-pushed the fix-search-limited-results branch from 1371ef1 to 5823bdc Compare July 13, 2026 13:40

@veronnicka veronnicka left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hi, I tried the changes and the bug seems to be still there, I was trying it in the hbac rules pages.

Screencast.From.2026-07-13.15-54-53.mp4

upon the first enter, it just changes to the first page and upon the second click it actually searches for the name

EDIT: the active users are working, I think this difference might be because of the responsive search upon each keystroke? but that is just a guess

startIdx: firstIdx,
stopIdx: lastIdx,
startIdx: 0,
stopIdx: perPage,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Won't this limit the search to only perPage entries? What if I have more entries than that? How can I search for them?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

You are right. Not sure why this was changed like that...

@carma12 carma12 force-pushed the fix-search-limited-results branch 2 times, most recently from 18fb8c3 to da4d9d7 Compare July 14, 2026 13:46

@veronnicka veronnicka left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks, I tried Active users and Hbac rules pages again and it works as expected. You even fixed the responsive search, so we should add Fixes: #1138

I think there is a redundant setPage(1) in the solution so please look into that.

I noticed that the changes are not consistent and decided to dig a bit into it, then I also noticed some files were not touched and are still not working, I am sending a screenshot from Claude:

Image

I checked the OtpTokens in the UI and the bug is still present. The list is just orientational, but a good starting point. I suggest asking claude for incosistencies and then check it and make sure all files are fixed.

Comment thread doc/designs/main-pages/03-walkthrough-init-fetch.md
Comment thread src/pages/ActiveUsers/ActiveUsers.tsx Outdated
}

setUsersTotalCount(totalCount);
setPage(1);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why is it here again? you already set it at the begginning of the call

@carma12 carma12 Jul 15, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This is because, when a search is done from, e.g, page 2, the results are listed (and most likely will be one or more entries, but less than what's specified in perPage, e.g. 10), but the page is still located in 2 so the results cannot be seen. That's why I'm settings the page to the very beginning so that the data can be shown.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

yes that I understand. But the setPage is done twice in the call, once at the very begginning a few lines up, and then again here.

@carma12

carma12 commented Jul 15, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks, I tried Active users and Hbac rules pages again and it works as expected. You even fixed the responsive search, so we should add Fixes: #1138

I think there is a redundant setPage(1) in the solution so please look into that.

I noticed that the changes are not consistent and decided to dig a bit into it, then I also noticed some files were not touched and are still not working, I am sending a screenshot from Claude:
Image

I checked the OtpTokens in the UI and the bug is still present. The list is just orientational, but a good starting point. I suggest asking claude for incosistencies and then check it and make sure all files are fixed.

I didn't realized about the responsive search until you mentioned it, haha. I have already fixed the changes mentioned and I have tested it in DNS zones page. Could you please confirm that everything works fine in your end? TY!

Comment on lines +201 to +204
> **Important — `stopIdx: 100`:** Use a fixed upper bound (100) instead of `perPage`.
> The LDAP backend has a size limit close to this value, and using `perPage` (e.g. 10)
> would miss entries beyond the first page of results when searching from a page other
> than page 1.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It's not an LDAP limitation, it's a global setting, that is overridable

Image

I think we could even omit the stopIdx.

https://webui.ipa.test/ipa/ui/#/e/config/details

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Right. But even if you put a wild number like, e.g. 1000 there, the limitation will be around ~200 elements.

@veronnicka

Copy link
Copy Markdown
Contributor

Thanks, I tried Active users and Hbac rules pages again and it works as expected. You even fixed the responsive search, so we should add Fixes: #1138
I think there is a redundant setPage(1) in the solution so please look into that.
I noticed that the changes are not consistent and decided to dig a bit into it, then I also noticed some files were not touched and are still not working, I am sending a screenshot from Claude:
Image
I checked the OtpTokens in the UI and the bug is still present. The list is just orientational, but a good starting point. I suggest asking claude for incosistencies and then check it and make sure all files are fixed.

I didn't realized about the responsive search until you mentioned it, haha. I have already fixed the changes mentioned and I have tested it in DNS zones page. Could you please confirm that everything works fine in your end? TY!

I dont see any recent push(aybe some issue on my end?), will wait for that then check.

@carma12

carma12 commented Jul 15, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks, I tried Active users and Hbac rules pages again and it works as expected. You even fixed the responsive search, so we should add Fixes: #1138
I think there is a redundant setPage(1) in the solution so please look into that.
I noticed that the changes are not consistent and decided to dig a bit into it, then I also noticed some files were not touched and are still not working, I am sending a screenshot from Claude:
Image
I checked the OtpTokens in the UI and the bug is still present. The list is just orientational, but a good starting point. I suggest asking claude for incosistencies and then check it and make sure all files are fixed.

I didn't realized about the responsive search until you mentioned it, haha. I have already fixed the changes mentioned and I have tested it in DNS zones page. Could you please confirm that everything works fine in your end? TY!

I dont see any recent push(aybe some issue on my end?), will wait for that then check.

I pushed them. But I can do the push again...

@carma12 carma12 force-pushed the fix-search-limited-results branch from 63d00a2 to ba2ccff Compare July 15, 2026 12:16

@veronnicka veronnicka left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hi, I tried running it, looked into Hbac rules, Active users, User groups and all seems to work great. Thanks for the effort. Just one comment in the doc, please add the Fixes: as I suggested, other than that we can merge. Providing an ack

elementsList: entities,
totalCount: searchTotalCount,
});
setPage(1);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

the code no longer calls the setPage in the success branch so please remove from the doc as well

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Sure, will do it.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done

Fix search from non-first pages returning
wrong or empty results

Reset pagination to page 1 and use `startIdx: 0`
when submitting a search. Previously, search used
the current page offset, which skipped matching
entries in the LDAP result set. Fixed across all
affected pages, and updated the corresponding
`main-pages` file.

Assisted-by: Claude <noreply@anthropic.com>
Signed-off-by: Carla Martinez <carlmart@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-review This PR is waiting on a review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants