refactor(compass-global-writes): remove in-progress states

[paula-stacho]

Description

Motivation:

As new flows have been added, the complexity of the reducer grew and seems difficult to grasp/maintain.

Changes:

• separating userActionInProgress from status
  • for example status: UNMANAGING_NAMESPACE_MISMATCH now becomes status: SHARD_KEY_MISMATCH; userActionInProgress: unmanageNamespace
  • the responsibility of preventing multiple actions has moved from the reducer to the actions, while the rootState types (and through them the reducer) still maintain the status:action relationship
• moving the pollingTimeout reference outside of the RootState
  • this was complicating the reducer unnecessarily

New state diagram

stateDiagram-v2

NOT_READY --> UNSHARDED
NOT_READY --> INCOMPLETE_SHARDING_SETUP
NOT_READY --> SHARDING
NOT_READY --> SHARD_KEY_CORRECT
NOT_READY --> SHARDING_ERROR
NOT_READY --> INVALID_SHARD_KEY
NOT_READY --> SHARD_KEY_MISMATCH
NOT_READY --> LOADING_ERROR

UNSHARDED --> SHARDING

INCOMPLETE_SHARDING_SETUP --> SHARDING

SHARDING --> SHARD_KEY_CORRECT
SHARDING --> SHARDING_ERROR
SHARDING --> INVALID_SHARD_KEY
SHARDING --> SHARD_KEY_MISMATCH
SHARDING --> UNSHARDED

SHARD_KEY_CORRECT --> INCOMPLETE_SHARDING_SETUP

SHARDING_ERROR --> UNSHARDED
SHARDING_ERROR --> SHARDING

SHARD_KEY_MISMATCH --> INCOMPLETE_SHARDING_SETUP

Loading

Tests:

• improved polling testing (including checking that polling with empty results continues for a while)

Acknowledgements

Big thanks to @Sergey Petushkov for brainstorming and ideas, I have a much better feeling about the reducer complexity now!

Checklist

• New tests and/or benchmarks are included
• Documentation is changed or added
• I have signed the MongoDB Contributor License Agreement (https://www.mongodb.com/legal/contributor-agreement)

Motivation and Context

• Bugfix
• New feature
• Dependency update
• Misc

Open Questions

Dependents

Types of changes

• Backport Needed
• Patch (non-breaking change which fixes an issue)
• Minor (non-breaking change which adds functionality)
• Major (fix or feature that would cause existing functionality to change)

[djechlin-mongodb]

I think this is a really great refactor, but I'm wondering if we can be more ambitious in our refactor and just not represent userActionInProgress at all. To me the ideal behavior is the tab is fully blocked in the 10s or 100 ms when you're waiting for a response from atlas and the page crashes if there's an error. We shouldn't try to recover from these errors; instead just don't handle the error force a reload. This is better than essentially having global state saying the page is in an error state and the only thing to do is retry/refresh to get back in a good state. userActionInProgress is only used for short actions not polls right?

[djechlin-mongodb]

Let's use enums here instead of raw strings for safety, and to emphasize this is another kind of status.

[paula-stacho]

The type safety is the same with union types. We tend to use union types in Compass, unless there is a reason to prefer enums - for example if the values are not very readable and we want to label them, or if we expect the values to change.

[paula-stacho]

which makes me wonder why we used enum for ShardingStatuses 🤔

[djechlin-mongodb]

You can move userActionInProgress to RootState and restrict as needed in the more specific actions. Also recommend using enum so that ShardingStatuses + UserActions form the complete state.

[paula-stacho]

this is the restriction. I had the full type in the root before, but then I removed it because it seemed unnecessary since each union part had a different definition. I can add it again for readability

[djechlin-mongodb]

This looks OK, I guess I don't see why you would check for a timeout immediately after the request gave a response. Do you agree, and is that the reason you're removing it?

[paula-stacho]

I was happy to get rid of this part, I don't think this logic should be in the reducer, reducer is for translating action to new state. Previously I was kind of pushed to do this by the RootState types, but now the pollingTimeout is moved out of the RootState and is handled in the actions.

[paula-stacho]

I think this is a really great refactor, but I'm wondering if we can be more ambitious in our refactor and just not represent userActionInProgress at all. To me the ideal behavior is the tab is fully blocked in the 10s or 100 ms when you're waiting for a response from atlas and the page crashes if there's an error. We shouldn't try to recover from these errors; instead just don't handle the error force a reload. This is better than essentially having global state saying the page is in an error state and the only thing to do is retry/refresh to get back in a good state. userActionInProgress is only used for short actions not polls right?

Yes, userActionInProgress is used only for user-triggered actions. I'm not sure I follow completely - you say to not represent userActionInProgress, but I think you mean we'd to have userActionInProgress boolean that would always block the whole tab? And the error handling would be the same for Sounds like we are coming back to design discussions, I'd say that's out of scope for this PR, maybe we could have a brief chat and invite at least @gribnoysup as someone more familiar with Compass design patterns.