[WIP] Search redux by bomsy · Pull Request #3370 · firefox-devtools/debugger

bomsy · 2017-07-19T01:10:06Z

Associated Issue: #3342

Summary of Changes

Move search functionality to redux
Keeping search state
Added opening source file on clicking the match

Todo

Fix issue with multiple children having the same key

bomsy · 2017-07-19T01:11:29Z


      while ((result = query.exec(_text))) {
        indices.push({
+          id: source.id,


This enables us know the source attached to the match

perhaps sourceId

jasonLaster

Wow - looking great!

jasonLaster · 2017-07-19T05:32:57Z

 import * as ui from "./ui";
 import * as ast from "./ast";
 import * as coverage from "./coverage";
+import * as search from "./search";


perhaps project-text-search

jasonLaster · 2017-07-19T05:35:34Z

+      .filter(source => source.has("text"))
+      .toJS();
+
+    validSources.forEach(source => {


perhaps:

for (const source of validSources) { const matches = await searchSource(source, query); dispatch({ ya da ya da }); }

This strategy will let us defer the work to the worker. each search will be a new worker call and will wait for the response to come back...

jasonLaster · 2017-07-19T05:35:46Z


+  componentDidMount() {
+    if (this.state.inputValue == "") {
+      this.props.loadAllSources();


another option would be to move this to the searchSources action...

the advantage is that we always do it before we search. often it will be very fast... and then we know we have everything to search

jasonLaster · 2017-07-19T05:36:49Z

  }

+  selectMatchItem(matchItem) {
+    this.props.selectSource(matchItem.id);


jasonLaster · 2017-07-19T05:37:04Z

        className: classnames("result", { focused }),
-        key: `${match.line}/${match.column}`,
-        onClick: () => console.log(`clicked ${match}`)
+        key: `${match.line}-${match.column}`,


we might need to have the source id here too

jasonLaster · 2017-07-19T05:38:10Z

+    case "ADD_QUERY":
+      return state.update("query", value => action.query);
+
+    case "REMOVE_QUERY":


minor thing, but clear is being used in other places clear_query

jasonLaster · 2017-07-19T05:38:27Z

+
+    case "ADD_SEARCH_RESULT":
+      return state.updateIn(
+        ["results", action.result.id],


perhaps action.result.sourceId

jasonLaster · 2017-07-19T05:38:49Z

+type OuterState = { search: Record<SearchState> };
+
+export function getSearchResults(state: OuterState) {
+  return state.search.get("results").valueSeq().toJS();


lets expose I state.search.get("results")

@jlast Is there any particular reason behind this? Just for my understanding.

Immutable lets us compare items and see if they change list.get("id") === list.get("id")

Can you please tag jasonlaster instead of me?

codecov · 2017-07-20T08:06:13Z

Codecov Report

Merging #3370 into master will decrease coverage by 0.33%.
The diff coverage is 24.24%.

@@            Coverage Diff             @@
##           master    #3370      +/-   ##
==========================================
- Coverage   50.76%   50.43%   -0.34%     
==========================================
  Files         105      107       +2     
  Lines        4353     4386      +33     
  Branches      896      900       +4     
==========================================
+ Hits         2210     2212       +2     
- Misses       2143     2174      +31

Impacted Files	Coverage Δ
src/selectors.js	`100% <ø> (ø)`	⬆️
src/utils/search/project-search.js	`94.73% <ø> (ø)`	⬆️
src/actions/search.js	`0% <0%> (ø)`
src/reducers/search.js	`42.1% <42.1%> (ø)`
src/actions/ui.js	`76.66% <0%> (-6.67%)`	⬇️
src/reducers/ui.js	`79.03% <0%> (-6.46%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0533fbd...b0cdd86. Read the comment docs.

codecov · 2017-07-20T08:06:13Z

Codecov Report

Merging #3370 into next will increase coverage by 0.49%.
The diff coverage is 17.14%.

@@            Coverage Diff             @@
##             next    #3370      +/-   ##
==========================================
+ Coverage   50.03%   50.52%   +0.49%     
==========================================
  Files         107      107              
  Lines        4457     4388      -69     
  Branches      920      900      -20     
==========================================
- Hits         2230     2217      -13     
+ Misses       2227     2171      -56

Impacted Files	Coverage Δ
src/utils/search/project-search.js	`94.73% <ø> (ø)`	⬆️
src/utils/makeRecord.js	`100% <ø> (ø)`	⬆️
src/selectors.js	`100% <ø> (ø)`	⬆️
src/actions/project-text-search.js	`0% <0%> (ø)`
src/reducers/project-text-search.js	`31.57% <31.57%> (ø)`
src/utils/breakpoint/index.js	`70.9% <0%> (-7.56%)`	⬇️
src/utils/source.js	`44.64% <0%> (-6.18%)`	⬇️
src/actions/ast.js	`74.41% <0%> (-2.37%)`	⬇️
src/client/firefox.js	`87.5% <0%> (-0.74%)`	⬇️
src/actions/breakpoints.js	`83.11% <0%> (-0.64%)`	⬇️
... and 27 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 996d8c8...db97890. Read the comment docs.

jasonLaster

couple small comments, then it's ready to go in!

jasonLaster · 2017-07-20T14:58:35Z

+
+export function searchSources(query) {
+  return async ({ dispatch, getState }: ThunkArgs) => {
+    dispatch(await loadAllSources());


perhaps we want to do something to disallow new searches while one is going on?

I do this with preview, by having an updating state: https://github.com/devtools-html/debugger.html/blob/master/src/reducers/ast.js#L21-L22

jasonLaster · 2017-07-20T15:01:53Z


      while ((result = query.exec(_text))) {
        indices.push({
+          id: source.id,


perhaps sourceId

jasonLaster · 2017-07-20T15:02:18Z

 import type { PauseState } from "./pause";
 import type { SourcesState } from "./source";
 import type { BreakpointsState } from "./breakpoints";
+import type { SearchState } from "./search";


hmm - does this need to be updated?

opppsieee!! :)

…have all loaded

jasonLaster · 2017-07-21T13:24:23Z

  ast,
-  coverage
+  coverage,
+  search


should be projectTextSearch

jasonLaster · 2017-07-21T13:24:44Z

  ui,
  ast,
  coverage,
+  search,


should be projectTextSearch

jasonLaster · 2017-07-21T13:41:54Z

Happy to land this and iterate. I'll leave some UX feedback on the parent issue

bomsy · 2017-07-23T01:05:38Z

      type: "CLEAR_SELECTION"
    };

+export type ProjectTextSearchAction = {


it was a nightmare resolving flow issues

bomsy · 2017-07-24T12:24:28Z

To add tests

jasonLaster · 2017-07-24T12:57:02Z

  deleteIn(keyPath: Array<any>, ...iterables: Array<any>): Record<T>,
+  update<A>(key: $Keys<T>, value: A): Record<T>,
+  updateIn(keyPath: Array<any>, ...iterables: Array<any>): Record<T>,
+  remove<A>(key: $Keys<T>): Record<T>,


oh wow, nice!

jasonLaster · 2017-07-24T14:37:36Z

Great work @bomsy!

bomsy added 4 commits July 18, 2017 20:12

add search redux

581b181

get the query from the reducer

265b4f6

fix bug

fde6458

add selecting a match opens a source

fa897c4

bomsy commented Jul 19, 2017

View reviewed changes

jasonLaster reviewed Jul 19, 2017

View reviewed changes

fix child items with same key issue

b0cdd86

fix reviews

b5c23d4

jasonLaster reviewed Jul 20, 2017

View reviewed changes

allow re searching of the query since all the source texts might not …

989dab2

…have all loaded

jasonLaster changed the base branch from master to next July 20, 2017 20:39

jasonLaster reviewed Jul 21, 2017

View reviewed changes

bomsy added 3 commits July 22, 2017 03:30

rename to project-text-search

648d2a1

fix flow errors

8e6f70a

fix lint issues

2bbfd17

bomsy commented Jul 23, 2017

View reviewed changes

fix conflicting selectors which cause the tests to fail

db97890

jasonLaster approved these changes Jul 24, 2017

View reviewed changes

jasonLaster merged commit 804e92d into firefox-devtools:next Jul 24, 2017

jasonLaster pushed a commit to jasonLaster/debugger.html that referenced this pull request Aug 1, 2017

[WIP] Search redux (firefox-devtools#3370)

15b7f93

Conversation

bomsy commented Jul 19, 2017

Summary of Changes

Todo

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasonLaster left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bomsy Jul 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasonLaster Jul 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Jul 20, 2017

Codecov Report

Uh oh!

codecov Bot commented Jul 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jasonLaster left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasonLaster commented Jul 21, 2017

Uh oh!

bomsy Jul 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bomsy commented Jul 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasonLaster commented Jul 24, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

bomsy Jul 19, 2017 •

edited

Loading

jasonLaster Jul 20, 2017 •

edited

Loading

codecov Bot commented Jul 20, 2017 •

edited

Loading

bomsy Jul 23, 2017 •

edited

Loading