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

✨ Show search results when searching by email: prefix #136

Merged
merged 1 commit into from
Jun 26, 2024
Merged

Conversation

foysalit
Copy link
Contributor

No description provided.

@foysalit foysalit requested a review from devinivy June 18, 2024 20:42
Copy link

render bot commented Jun 18, 2024

Copy link

render bot commented Jun 18, 2024

@foysalit foysalit requested review from dholms and bnewbold June 18, 2024 20:42
})

await Promise.allSettled(
data.accounts.map(async (account) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is kinda resource intensive since we're essentially firing like 25 api reqs to getRepo but this isn't expected to be a commonly used feature so we should be fine with some overhead.

Comment on lines +60 to +61
await Promise.allSettled(
data.accounts.map(async (account) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Without handling the result of allSettled() this will swallow errors, including unexpected ones. Another option is to use Promise.all() here and handle expected error conditions within each item.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is intentional, I think we have most info we need to render a partial view from the resultSet so if some requests fail, they will just not show the augmented data from getRepo()

Copy link
Collaborator

Choose a reason for hiding this comment

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

That totally makes sense and I don't think we'd need to change that— my main question is whether we want to swallow all errors versus ignore some acceptable set of errors and log or throw the rest. For example, a logic bug could cause a TypeError in here and there's no way we'd be able to observe it, could easily go unnoticed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The rule here might be that we're comfy ignoring all XRPCErrors, for example. I really appreciate libraries like bounce (though I think this may catered to server-side) which ensure developer/system errors don't get missed when permitting some errors.

Copy link
Collaborator

@devinivy devinivy left a comment

Choose a reason for hiding this comment

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

Looking good

@foysalit foysalit merged commit b0af022 into main Jun 26, 2024
3 checks passed
@matthieusieben matthieusieben deleted the email-search branch November 15, 2024 14:55
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.

2 participants