feat(compass-indexes): also poll regular indexes COMPASS-8214 #6264
Conversation
| inProgressIndexes: InProgressIndex[]; | ||
| error: string | null; | ||
| error?: string; |
There was a problem hiding this comment.
This was string or null here and string or undefined in search indexes. I just picked one and made them the same.
| : SearchIndexesStatuses.READY, | ||
| state.status === FetchStatuses.FETCHING ? action.error : state.error, | ||
| status: | ||
| state.status === FetchStatuses.FETCHING |
There was a problem hiding this comment.
This was actually a nasty bug I introduced in the refactor. FetchStatuses.FETCHING is always true 😄
lerouxb
changed the title
| @@ -567,7 +569,9 @@ export function DropdownMenuButton<Action extends string>({ | |||
| rightGlyph={<Icon glyph={'CaretDown'} />} | |||
| title={buttonText} | |||
| > | |||
| <span className={hiddenOnNarrowStyles}>{buttonText}</span> | |||
| <span className={hideOnNarrow ? hiddenOnNarrowStyles : undefined}> | |||
There was a problem hiding this comment.
This is a drive-by. Recently I made the dropdown button collapse when the view gets narrow, but for the create index button you get when search indexes is available that's a bit much. So I added a way to opt out of it.
| @@ -18,6 +24,27 @@ export default function reducer( | |||
| state = INITIAL_STATE, | |||
| action: AnyAction | |||
| ): IndexView { | |||
| // The create index button has a dropdown where you can select regular or | |||
There was a problem hiding this comment.
This is another drive-by. This is functionality we had before I refactored it, then accidentally dropped during the recent refactor.
...egations/src/components/aggregation-side-panel/stage-wizard-use-cases/search/text-search.tsx
Outdated
Show resolved
Hide resolved
|
|
||
| export const POLLING_INTERVAL = 5000; | ||
|
|
||
| let pollInterval: ReturnType<typeof setInterval> | undefined = undefined; |
There was a problem hiding this comment.
Hmmm, I know that we're technically stop polling if the tab goes off screen, but I wonder if it would maybe make sense to already take the tab id into account somehow here. We have this clunky way of passing tab id around as a prop for the components, potentially we'd have to change that too if we'd decide to change that. Just thinking that if we ever allow to splitting tabs in the same window, this will cause an issue that's hard to spot
There was a problem hiding this comment.
Like const pollIntervalByTabId = {}; ?
There was a problem hiding this comment.
We usually stick with Map for similar cases in other stores, but just an object like that would also work, yeah 👍
There was a problem hiding this comment.
Aah only saw this comment now. Changed to a Map.
| const reason: FetchReason = | ||
| status === FetchStatuses.READY | ||
| ? FetchReasons.REFRESH | ||
| : FetchReasons.INITIAL_FETCH; |
There was a problem hiding this comment.
I think one thing that I was missing so far is that we're using the same action for user triggering through the refresh button, initial plugin activation, and re-fetching the indexes after we created one through the modal. I was hoping that introducing this reason to the action we would be able to avoid deriving the state in the action from the previous state, that is generally not recommended, but seems like specifically because this particular action is used for this initial fetching triggered from the activate, we still have to keep this around.
Using this for both user triggered and programmatic refreshes kinda makes sense to me, but do you think we can remove the initial fetch from the list of things that this action creator is used for and either have a dedicated action creator for that, or just use fetchIndexes with INITIAL_FETCH passed there as a reason directly in the activate method? I'd really want us to remove the deriving state from a previous one like that in these actions
There was a problem hiding this comment.
I made separate functions for the initial fetch, refresh and poll.
| @@ -83,7 +47,7 @@ export enum ActionTypes { | |||
|
|
|||
| type FetchSearchIndexesStartedAction = { | |||
| type: ActionTypes.FetchSearchIndexesStarted; | |||
| status: 'REFRESHING' | 'POLLING' | 'FETCHING'; | |||
| reason: FetchReason; | |||
I was initially going to do more in this PR, but I decided to file COMPASS-8321 for a separate PR rather.