-
Notifications
You must be signed in to change notification settings - Fork 32
feat: 🎸 Add ability to search other resources as well #3021
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: main
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
26a23e0 to
26ec64b
Compare
26ec64b to
68c8c48
Compare
0903065 to
700e37b
Compare
68c8c48 to
08215e9
Compare
| text: search, | ||
| relatedSearches: [ | ||
| { | ||
| resource: 'target', |
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 makes the assumption that the destionation_id is always a target but that's the current state of aliases right now. If/when destination_id can be more than just targets, we'll need to re-visit this.
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 looks great! Test cases helped to follow along 🙌
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.
looks good!
700e37b to
c84ad0b
Compare
08215e9 to
18ca138
Compare
233b37b to
d5a8099
Compare
a7cf647 to
9354e05
Compare
d5a8099 to
0a74f4a
Compare
be958e8 to
7fefe8c
Compare
7d57839 to
4de58b9
Compare
7fefe8c to
6b8f735
Compare
4de58b9 to
bc8e006
Compare
6b8f735 to
73c3c0f
Compare
The base branch was changed.
73c3c0f to
e58d6d1
Compare
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.
I'm still reviewing, it all looks really good. I have this one question at the moment that maybe you can check my understanding on
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.
🚀 Looks great! Cool to see how it all came together from the api to the query builder
Description
For pages that have tables that display data from associates resources, we currently retrieve each associated resource 1 by 1 for each shown row to get the associated data and are unable to search columns that source data that doesn't exist in the primary table.
This PR adds the ability to search across multiple tables with joins. This allows us to search across multiple tables in the case where we have columns that use data from different resources. This does however force a dependency on us to load all the associated resources first as compared to making individual
GETrequests for each associated resource. But this is likely better than making all those GET requests at once (especially if they choose 50 items per page).The main downside is there can be a noticeable increase in latency for the searches depending on the number of resources.
Screenshots (if appropriate)
Example with 400k aliases + 400k targets and searching between all the resources in both tables.
Before:
Screen.Recording.2025-10-01.at.6.10.41.PM.mov
After:
Screen.Recording.2025-10-01.at.6.13.51.PM.mov
How to Test
Go to the aliases page and try searching target names or alias IDs. It should search across both resources.
Checklist
[ ] I have added JSON response output for API changes[ ] I have added steps to reproduce and test for bug fixes in the description[ ] I have addeda11y-testslabel to run a11y audit tests if neededPCI review checklist
Examples of changes to security controls include using new access control methods, adding or removing logging pipelines, etc.