issue: https://linear.app/comm/issue/ENG-3317/implement-fetching-messages-based-on-the-query
I'm using the same cursor pattern as we use in fetchMessageInfos(fetchMessagesBeforeCursor), fetchErrorReportInfos
Joining with messages m2 is used to fetch the message a sidebar_source is pointing to.
Details
Details
Called this function from createMessages and checked that it returns messages matching the provided pattern, from the given thread. Checked that it returns correct related messages, especially for the
message that is the first message in a sidebar.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Branch
- inka/search_backend
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| keyserver/src/database/search-utils.js | ||
|---|---|---|
| 137 ↗ | (On Diff #25652) | Can you clarify if it's really helpful to split based on this versus just splitting on a single space character? My impression was that segmentAndStem converts extraneous whitespace to a single space, but I may be wrong |
| 144 ↗ | (On Diff #25652) | I feel like this would be more readable |
| keyserver/src/fetchers/message-fetchers.js | ||
|---|---|---|
| 923 ↗ | (On Diff #25652) | Can you clarify in what scenarios viewer won't be set? |
| 925 ↗ | (On Diff #25652) | It doesn't make sense to have a required parameter after optional ones. We can make the optional parameters required (but with an optional type), eg. cursor: ?string instead of cursor?: string. But perhaps we should just reorder so that the required parameter comes first? |
Comment Actions
Address review
| keyserver/src/database/search-utils.js | ||
|---|---|---|
| 137 ↗ | (On Diff #25652) | segmentAndStem does remove additional whitespaces. This is not incorrect, but unnecessarily broad. |
| keyserver/src/fetchers/message-fetchers.js | ||
| 923 ↗ | (On Diff #25652) | My thinking was that since the function doesn't need the viewer to be defined, I shouldn't artificially create this constraint in case someone needs to use it without a user for some reason. |
Comment Actions
Requesting review because after a discussion in ENG-3744 we decided that for now we only want to exclude + < > ~ signs from the query. Those are signs that are both not removed by segmentAndStem, and at the same time are operands of the MATCH AGAINST query, so keeping them in the query as words results in errors.
| keyserver/src/fetchers/message-fetchers.js | ||
|---|---|---|
| 911 ↗ | (On Diff #26152) | The limit has been increased by 1 in the most recent diff update. It was my impression that we weren't planning on doing the "fetch 1 extra" strategy anymore, though I don't remember why that decision was made. @inka, can you clarify if we're using the "fetch 1 extra" strategy still, and why the change was made? |
| keyserver/src/fetchers/message-fetchers.js | ||
|---|---|---|
| 911 ↗ | (On Diff #26152) | Looks like we're using the "fetch 1 extra" strategy in D7651 now. So that answers the first question... just curious about the second question. Basically, I recall that we were initially doing this, and then stopped doing it, and now looks like we're doing it again... curious for context on that |
| keyserver/src/fetchers/message-fetchers.js | ||
|---|---|---|
| 911 ↗ | (On Diff #26152) | Initially I used it somewhere else - in D7117, when I was fetching messages to be processed entirely on the server side. It didn't make sense there because the server had to do the same amount of work either way (run the query the same number of times). Here is what you wrote on that diff, that explains it:
|
| keyserver/src/fetchers/message-fetchers.js | ||
|---|---|---|
| 911 ↗ | (On Diff #26152) | Thanks for explaining! |