Page MenuHomePhabricator

[web] Add message search context
ClosedPublic

Authored by inka on Jun 21 2023, 5:13 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Nov 23, 1:45 PM
Unknown Object (File)
Sat, Nov 23, 1:45 PM
Unknown Object (File)
Sat, Nov 23, 1:09 PM
Unknown Object (File)
Sat, Nov 23, 1:08 PM
Unknown Object (File)
Sat, Nov 23, 1:08 PM
Unknown Object (File)
Sat, Nov 23, 1:08 PM
Unknown Object (File)
Sat, Nov 23, 1:08 PM
Unknown Object (File)
Fri, Nov 22, 5:32 PM
Subscribers

Details

Summary

issue: https://linear.app/comm/issue/ENG-3466/add-some-state-for-keeping-the-search-query
We want to be able to remeber the query the user has typed in, for each chat.

Test Plan

Tested with subsequent diffs

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

inka requested review of this revision.Jun 21 2023, 5:31 AM

Add clearQuery

web/search/message-search-state-provider.react.js
38–41 ↗(On Diff #27926)

Not sure if there is a better way of doing this?

tomek requested changes to this revision.Jun 21 2023, 5:43 AM
tomek added inline comments.
web/search/message-search-state-provider.react.js
38–41 ↗(On Diff #27926)

The current implementation mutates the state. If there would be no better solution, we should do something like:

setQueries(prevQueries => {
  const newState = {...prevQueries};
  delete newState[threadID];
  return newState;
}),

but there's a better solution

setQueries(prevQueries => {
  const {[threadID]:deleted, ...newState} = prevQueries;
  return newState;
}),

(but Flow might complain about unused variable deleted)

This revision now requires changes to proceed.Jun 21 2023, 5:43 AM
tomek requested changes to this revision.Jun 22 2023, 2:26 AM
tomek added inline comments.
web/search/message-search-state-provider.react.js
26 ↗(On Diff #27929)

What do you think about changing the approach so that a thread id is a parameter of set query?

28 ↗(On Diff #27929)

Where this context will be rendered? Are we sure that this will always be true? Can we avoid having this invariant?

This revision now requires changes to proceed.Jun 22 2023, 2:26 AM
web/search/message-search-state-provider.react.js
26 ↗(On Diff #27929)

Why? I thought this was a way better approach because the calling code doesn't have to do this, it's abstracted away.
If you're thinking about global search, we can add thread id as an optional argument then, and if present use it instead of threadID.

This revision is now accepted and ready to land.Jun 27 2023, 3:24 AM