Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Fix incorrect assumptions about required fields in /search response #12575

Merged
merged 1 commit into from
Jun 6, 2024
Merged

Fix incorrect assumptions about required fields in /search response #12575

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions src/Searching.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,7 @@ async function localPagination(

// We only need to restore the encryption state for the new results, so
// remember how many of them we got.
const newResultCount = localResult.results.length;
const newResultCount = localResult.results?.length ?? 0;
Copy link
Member

Choose a reason for hiding this comment

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

Trying to understand what's going on here. This looks to be dealing with seshat results rather than /search response? Is that correct, or am I missing something?

According to the types, results is always present, so are the types wrong?

Copy link
Member Author

Choose a reason for hiding this comment

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

This looks to be dealing with seshat results rather than /search response? Is that correct, or am I missing something?

You are correct, but seshat mimics the C-S API so we should assume the same lack of required fields.

According to the types, results is always present, so are the types wrong?

Yes, those are handled by matrix-org/matrix-js-sdk#4228


const response = {
search_categories: {
Expand Down Expand Up @@ -419,21 +419,21 @@ function combineEvents(
// This is a first search call, combine the events from the server and
// the local index. Note where our oldest event came from, we shall
// fetch the next batch of events from the other source.
if (compareOldestEvents(localEvents.results, serverEvents.results) < 0) {
if (compareOldestEvents(localEvents.results ?? [], serverEvents.results) < 0) {
oldestEventFrom = "local";
}

combineEventSources(previousSearchResult, response, localEvents.results, serverEvents.results);
response.highlights = localEvents.highlights.concat(serverEvents.highlights);
combineEventSources(previousSearchResult, response, localEvents.results ?? [], serverEvents.results);
response.highlights = (localEvents.highlights ?? []).concat(serverEvents.highlights ?? []);
} else if (localEvents) {
// This is a pagination call fetching more events from the local index,
// meaning that our oldest event was on the server.
// Change the source of the oldest event if our local event is older
// than the cached one.
if (compareOldestEvents(localEvents.results, cachedEvents) < 0) {
if (compareOldestEvents(localEvents.results ?? [], cachedEvents) < 0) {
oldestEventFrom = "local";
}
combineEventSources(previousSearchResult, response, localEvents.results, cachedEvents);
combineEventSources(previousSearchResult, response, localEvents.results ?? [], cachedEvents);
} else if (serverEvents && serverEvents.results) {
// This is a pagination call fetching more events from the server,
// meaning that our oldest event was in the local index.
Expand Down
2 changes: 1 addition & 1 deletion src/components/structures/RoomSearchView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ export const RoomSearchView = forwardRef<ScrollPanel, Props>(
}, []); // eslint-disable-line react-hooks/exhaustive-deps

// show searching spinner
if (results?.count === undefined) {
if (results === null) {
Copy link
Member

Choose a reason for hiding this comment

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

this looks sensible.

But: how does it differ from the spinner shown when inProgress is true?

(Maybe we can rip out the inProgress stuff?)

Copy link
Member Author

Choose a reason for hiding this comment

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

inProgress is used for any loading including pagination, this is for the initial load. Not sure why its not just the same spinner but would rather not make aesthetic changes here.

return (
<div
className="mx_RoomView_messagePanel mx_RoomView_messagePanelSearchSpinner"
Expand Down
Loading