Skip to content

Conversation

nbbeeken
Copy link
Collaborator

@nbbeeken nbbeeken commented Aug 15, 2025

Description

Add a percentage to the index build row in the table if it is in progress.

https://jira.mongodb.org/browse/COMPASS-9495

Checklist

  • New tests and/or benchmarks are included
  • Documentation is changed or added
  • If this change updates the UI, screenshots/videos are added and a design review is requested
  • I have signed the MongoDB Contributor License Agreement (https://www.mongodb.com/legal/contributor-agreement)

Motivation and Context

  • Bugfix
  • New feature - parity with legacy DE
  • Dependency update
  • Misc

Open Questions

For reviewers:

  • Did I make good use of hooks by writing useIndexProgress?
    • No 😅
  • The getInProgressIndexInfo used to just list stub indexes because even in progress ones would be listed as "ready" while they continued to build on the server. We now will list "real" indexes via this function. Is that the right approach?

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)
Screen.Recording.2025-08-15.at.2.29.25.PM.mov
index_finish.mov

@github-actions github-actions bot added the feat label Aug 15, 2025
@nbbeeken nbbeeken marked this pull request as ready for review August 15, 2025 19:18
@Copilot Copilot AI review requested due to automatic review settings August 15, 2025 19:18
@nbbeeken nbbeeken requested a review from a team as a code owner August 15, 2025 19:18
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds index build progress tracking to the MongoDB Compass indexes table, showing a percentage indicator when indexes are being built. The implementation introduces a new hook for polling index progress via currentOp command and updates the UI to display build progress and a spinner for in-progress indexes.

  • Adds useIndexProgress hook for tracking index build progress via polling
  • Updates Redux state to track progress percentage for in-progress indexes
  • Modifies UI components to display progress indicators and spinners for building indexes

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
test/setup-store.ts Adds mock currentOp method to test data service
src/stores/store.ts Extends data service type to include currentOp for progress tracking
src/modules/regular-indexes.ts Adds progress tracking logic, new actions, and getIndexesProgress thunk
src/modules/regular-indexes.spec.ts Adds tests for index progress tracking functionality
src/hooks/use-index-progress.ts New custom hook for managing index build progress polling
src/components/regular-indexes-table/regular-indexes-table.tsx Integrates progress hook and updates index filtering logic
src/components/regular-indexes-table/regular-indexes-table.spec.tsx Updates tests to mock the progress hook
src/components/regular-indexes-table/regular-index-actions.tsx Adds progress display UI with spinner and percentage

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@@ -43,12 +56,27 @@ const IndexActions: React.FunctionComponent<IndexActionsProps> = ({
[onDeleteFailedIndexClick, index]
);

const progress = index.buildProgress * 100;
const isBuilding = progress > 0 && progress < 100;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure I follow this logic, we either have these in "in-progress" bucket or in "regular", how are we expecting this UI to show up for both?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Perhaps I've over engineered a bit, and I can confirm this again. IIUC the inprogress state is a "fake" one to make the UI show the user's index in the table right away after they create it. It changes to "ready" as soon as we see the index in a listIndexes response. I think the progress logic is left over here from when I would show users 0% when we had a true 0 value. I think I can remove this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, your understanding is totally right here, although I guess now that we actually include in progress in "real" ones, the "in progress" name doesn't make much sense anymore

@nbbeeken nbbeeken requested a review from gribnoysup August 22, 2025 19:09
Copy link
Collaborator

@gribnoysup gribnoysup left a comment

Choose a reason for hiding this comment

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

I haven't had a chance to track this down, but I'm definitely seeing some weird behaviors locally: if I start the index creation (so it first gets into the "in progress" bucket in the state) I never see the progress shown for it, it just shows "in progress" badge until the index finished building fully, if I look at the indexes for the same collection in a separate tab (so the index is fetched from the beginning, not created as a virtual "in progress" one) it correctly shows the building progress, but the badge shows ready. Screenshots below show how this looks, notice that title_1 is in progress but without the progress in one tab, and ready but with progress in another

image image

if (buildProgress > 0 && buildProgress < 1) {
return (
<div className={buildProgressStyles} data-testid="index-building-spinner">
<Body>Building... {(buildProgress * 100) | 0}%</Body>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: we're not using much of bitwise operators in the codebase, can we use truncate here instead for readability?

Suggested change
<Body>Building... {(buildProgress * 100) | 0}%</Body>
<Body>Building{Math.trunc(buildProgress * 100)}%</Body>

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done!

Comment on lines 263 to 264
.filter((index) => !rollingIndexNames.has(index.name))
.filter((index) => !inProgressIndexNames.has(index.name))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Super nit, to avoid two passes

Suggested change
.filter((index) => !rollingIndexNames.has(index.name))
.filter((index) => !inProgressIndexNames.has(index.name))
.filter((index) => !rollingIndexNames.has(index.name) && !inProgressIndexNames.has(index.name))

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

More than a nit to me, good catch.

Someday we'll use the new Iterator to stack complex set of filters and maps perhaps...

@@ -43,12 +56,27 @@ const IndexActions: React.FunctionComponent<IndexActionsProps> = ({
[onDeleteFailedIndexClick, index]
);

const progress = index.buildProgress * 100;
const isBuilding = progress > 0 && progress < 100;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, your understanding is totally right here, although I guess now that we actually include in progress in "real" ones, the "in progress" name doesn't make much sense anymore

Comment on lines 58 to 59
expect(() => screen.getByTestId('index-building-spinner')).to.throw;
expect(() => screen.getByText(/Building\.\.\. \d+%/)).to.throw;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Needs to be called to validate

Suggested change
expect(() => screen.getByTestId('index-building-spinner')).to.throw;
expect(() => screen.getByText(/Building\.\.\. \d+%/)).to.throw;
expect(() => screen.getByTestId('index-building-spinner')).to.throw();
expect(() => screen.getByText(/Building\.\.\. \d+%/)).to.throw();

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed, dunno how I missed this. On the node team I recall we either had a lint rule or we monkey-patched this assertion to require it has an argument because you could start throwing something else and not realize.

@nbbeeken
Copy link
Collaborator Author

I am now seeing it display the build percentage correctly by undoing the filtering changes. This does mean the index now has a "ready" badge while there's a "building... x%" string right next to it. Not ideal 🤔

We also have a "building" state but for rolling indexes only, I'm not sure whats the best approach here because it seems like it would be a heavy lift to reframe the statuses from how they are currently used since they're bound to how the UI treats them (like how in-progress is really a dummy state until we get a ping from the server)

Any thoughts on the best investments we can make here? Could the status badge also rely on buildProgress if there is any?

@nbbeeken nbbeeken requested a review from gribnoysup August 27, 2025 17:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants