New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CON-1067: Create 'indexing failed', 'indexing not loaded', states #1220
CON-1067: Create 'indexing failed', 'indexing not loaded', states #1220
Conversation
@justinmilner1 I really like the design of this, and appreciate the ample proof of correctness : ) One thought about how this could be simplified: instead of sending a different messageType for each possible state (setIndexingFailed, etc...), can we add a property to the IndexingProgress type, like |
@sestinj Thanks! I refactored 'setIndexingFailed' as an 'indexProgress' parameter - that simplified some of the logic and reduced the number of webviewProtocol requests a bit. I looked into integrating/simplifying other areas as well, but the most recent commit is what makes the most sense to me. Let me know if this is what you meant! The most recent commit has been tested the same before. |
@justinmilner1 this was perfect. I then got really excited about the direction and layered on a refactor of my own lol Now have indexing state in a single object, so there's less to keep track of and it's easier to add state later. I tested all the stuff you showed here and it looks perfect! Ice blue was working as well when I tried in a large enough repo for there to be time The one thing I did change, and it might be that I misunderstood, but the globalContext storage of the indexing state was a method to save current progress so that on the next window load, it could pick up in the state it left off in. I figured that this conflicts in a way with the "starting" state, and that we always try to redo indexing on window reload anyway. Perhaps it would make sense to say "if indexing failed, don't try again even on window reload", in which case we could keep this state in localstorage anyway...thanks for an awesome PR! |
Description
Failed state:
When indexing fails, the 'indexing state' dot indicator defaults to green. This is causing a confusion for users.
This pr attempts to catch indexing failures and present a red 'indexing failed' dot.
Pre-indexing state:
If the sidebar is opened shortly after the ide starts up, the indexing may not have started - Rather than default to the progress bar, I've created an ice-blue 'codebase indexing is starting up' dot state.
When indexing starts, this state will then transition to the progress bar (or another state if it should). This way we don't have the progress bar sitting at 0% for a long time before indexing even starts.
Related to: #1067
Development notes:
I chose to display a generic 'codebase not indexed, click to retry' on hover over the dot, rather than a specific error message. I think this is best for now because the errors messages can be quite long (too much for hover), and I want to observe what the failure points are a bit more before trying to filter them in some way.
Bug: Sometimes, if you attempt to re-index early in extension loading, the indexing will fail due to the abortController aborting - I believe this is unrelated to this pr, and I'll look into what could be causing this.
I'm using global state as well as props because the props can not be updated while the indexingProgressBar has not been initialized. (Indexing begins, and can fail, before indexingProgressBar has been initialized)
I've ensured that global states are created/updated as well for progress updates- this way, when the sidebar is opened, regardless of what stage indexing is in, it can trigger an update via the global states and get the indexing status display to the correct state.
Test cases:
simplescreenrecorder-2024-05-03_13.18.41.mp4
simplescreenrecorder-2024-05-03_13.21.22.mp4
simplescreenrecorder-2024-05-03_13.26.09.mp4
simplescreenrecorder-2024-05-03_13.27.52.mp4