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

search: add "Reindex now" to index status page#45178

Merged
stefanhengl merged 4 commits into
mainfrom
sh/add-reindex-button
Dec 8, 2022
Merged

search: add "Reindex now" to index status page#45178
stefanhengl merged 4 commits into
mainfrom
sh/add-reindex-button

Conversation

@stefanhengl

@stefanhengl stefanhengl commented Dec 5, 2022

Copy link
Copy Markdown
Member

This adds a new button "Reindex now" to the index status page.

Relevant backend changes:

image

Test plan

  • manual testing: I verified via the logs that clicking the button forces a reindex.
  • CI

App preview:

Check out the client app preview documentation to learn more.

@cla-bot cla-bot Bot added the cla-signed label Dec 5, 2022
@sg-e2e-regression-test-bob

sg-e2e-regression-test-bob commented Dec 5, 2022

Copy link
Copy Markdown

Bundle size report 📦

Initial size Total size Async size Modules
0.00% (0.00 kb) 0.00% (+0.46 kb) 0.00% (+0.46 kb) 0.00% (0)

Look at the Statoscope report for a full comparison between the commits e472109 and c6336cf or learn more.

Open explanation
  • Initial size is the size of the initial bundle (the one that is loaded when you open the page)
  • Total size is the size of the initial bundle + all the async loaded chunks
  • Async size is the size of all the async loaded chunks
  • Modules is the number of modules in the initial bundle

@stefanhengl stefanhengl requested review from a team December 6, 2022 08:12
@stefanhengl stefanhengl marked this pull request as ready for review December 6, 2022 08:15

@keegancsmith keegancsmith left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM from a search-core perspective. But I have no idea if the frontend code is correct, last time I touched the frontend I introduced multiple regressions with a one line change lol. So good luck :D

Comment thread client/web/src/repo/settings/RepoSettingsIndexPage.tsx Outdated
@keegancsmith

Copy link
Copy Markdown
Member

Before landing can you include a changelog entry for this?

@stefanhengl

Copy link
Copy Markdown
Member Author

refers to #29246

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is the page reload necessary? This is a very negative user experience. We only really do this on log in/log out.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We can take it out. The goal was to refresh the data on the page after the reindex and I didn't know how to do this without a page reload. The user can trigger the reload manually.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The page can be refreshed without a full reload by reloading the data that populates the page. Essentially re-doing the work being done in componentDidMount. If you want to do this I'm happy to help you out.

Comment thread client/web/src/repo/settings/RepoSettingsIndexPage.tsx Outdated
@stefanhengl stefanhengl force-pushed the sh/add-reindex-button branch from 4985794 to e472109 Compare December 8, 2022 10:15
@stefanhengl

Copy link
Copy Markdown
Member Author

Sorry, I had to force push to solve a failing percy/Sourcegraph check

@stefanhengl stefanhengl merged commit 2f79ae3 into main Dec 8, 2022
@stefanhengl stefanhengl deleted the sh/add-reindex-button branch December 8, 2022 10:36
Comment on lines +103 to +104
() => {},
() => {}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Note that this will ignore any and all errors from the mutation, including network errors. We don't want to show an error message if the call fails?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants