fix(content-explorer): Fix flaky SubheaderV2 folder title #4229
Conversation
WalkthroughRemoved immediate ancestor-folder-name fetch from ContentExplorer mount; ancestor name resolution now occurs after metadata query results when Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CE as ContentExplorer
participant Metadata as MetadataQuery
participant API as FolderAPI
User->>CE: Open metadata view
CE->>Metadata: ensure metadataQuery loaded (no immediate ancestor fetch on mount)
Metadata-->>CE: metadata results (may include ancestor_folder_id)
alt metadataViewV2 enabled and ancestor_folder_id present
CE->>API: getFolderFields(ancestor_folder_id)
API-->>CE: folder (with name)
CE->>CE: set rootName = folder.name
end
CE-->>User: Render metadata view (with or without rootName)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx (1)
184-186: MSW handler works, but hard-coding folder ID risks flakiness. Generalize the route.If global.FOLDER_ID differs from "0" locally, this mock won’t intercept the request, breaking the story. Make the handler dynamic.
Apply one of the following diffs:
Option A — bind to global.FOLDER_ID:
- http.get(`${DEFAULT_HOSTNAME_API}/2.0/folders/0`, () => { + http.get(`${DEFAULT_HOSTNAME_API}/2.0/folders/${global.FOLDER_ID}`, () => { return HttpResponse.json(mockRootFolder); }),Option B — match any folder id (MSW path params):
- http.get(`${DEFAULT_HOSTNAME_API}/2.0/folders/0`, () => { - return HttpResponse.json(mockRootFolder); - }), + http.get(`${DEFAULT_HOSTNAME_API}/2.0/folders/:id`, () => { + return HttpResponse.json(mockRootFolder); + }),src/elements/content-explorer/ContentExplorer.tsx (1)
1642-1653: Avoid flipping the whole view to error on a non-critical root-name fetch failure.fetchFolderName only decorates Subheader; failing it shouldn’t set VIEW_ERROR via the shared error handler. Prefer a non-fatal handler (log/warn) here.
this.api.getFolderAPI().getFolder( folderId, 1, 0, this.state.sortBy, this.state.sortDirection, (collection: Collection) => { this.setState({ rootName: collection.name }); }, - this.errorCallback, + (error) => { + // Non-fatal: keep the view usable even if the title fails to load + /* eslint-disable no-console */ + console.warn('fetchFolderName failed', error); + /* eslint-enable no-console */ + }, { fields: [FIELD_NAME], forceFetch: true }, );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/elements/content-explorer/ContentExplorer.tsx(1 hunks)src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx (1)
src/elements/common/__mocks__/mockRootFolder.ts (1)
mockRootFolder(925-925)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: lint_test_build
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Summary
🔇 Additional comments (2)
src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx (1)
11-11: Good addition: mockRootFolder import aligns with new getFolder usage.This keeps the story self-contained and supports the new root folder fetch path.
src/elements/content-explorer/ContentExplorer.tsx (1)
1642-1653: Switching to FolderAPI.getFolder with forceFetch is appropriate; sets a stable root title.
- Using getFolder matches how other parts fetch folder data and avoids stale cache via forceFetch.
- Reading collection.name is correct given other call sites also rely on the collection shape.
jfox-box
force-pushed
the
fix-flaky-mdv-title
branch
2 times, most recently
from
7eb4220 to
f500dca
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/elements/content-explorer/ContentExplorer.tsx (1)
1647-1654: Use forceFetch and correct the callback type; don’t flip to error view on ancillary failure
- To avoid cache reliance (the core flakiness), pass
forceFetch: truetogetFolderFields. This aligns with earlier feedback to prefergetFolderFieldswith forceFetch overgetFolderfor name-only.- The callback receives a Folder/BoxItem, not a Collection; tighten the type for clarity.
- Since the folder name is ancillary, don’t send the app to the error view if this call fails. Use
noopas the error handler instead.Apply this diff:
- this.api.getFolderAPI().getFolderFields( - folderId, - (collection: Collection) => { - this.setState({ rootName: collection.name }); - }, - this.errorCallback, - { fields: [FIELD_NAME] }, - ); + this.api.getFolderAPI().getFolderFields( + folderId, + (folder: BoxItem) => { + // Avoid stale updates if the user navigates away during the request + if (this.state.view === VIEW_METADATA) { + this.setState({ rootName: folder.name }); + } + }, + noop, // Non-blocking: don't switch to error view if fetching folder name fails + { fields: [FIELD_NAME], forceFetch: true }, + );
🧹 Nitpick comments (1)
src/elements/content-explorer/ContentExplorer.tsx (1)
406-412: Avoid stale rootName updates when view changes mid-flightIf the user navigates away from the metadata view (or updates the query) before the folder-name request resolves, we could set an out-of-date rootName. Guarding against view drift would make this more robust.
As you already centralize the folder-name fetch in
fetchFolderName, the guard can live there (see next comment’s diff). Alternatively, add a quick check here before callingfetchFolderName:
- Only call when
this.state.view === VIEW_METADATA- Optionally, compare the ancestor id with a tracked “latest” ancestor id before applying the result
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/elements/content-explorer/ContentExplorer.tsx(3 hunks)src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: lint_test_build
- GitHub Check: Summary
🔇 Additional comments (1)
src/elements/content-explorer/ContentExplorer.tsx (1)
341-354: Mount flow cleanup looks goodThe simplified mount logic and deferral of metadata-related work read clean and predictable.
jfox-box
force-pushed
the
fix-flaky-mdv-title
branch
from
f500dca to
17f2a46
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx (1)
176-187: Optional: deduplicate the folder-by-id handler across storiesYou added the same GET /2.0/folders/:id handler here and in MetadataView.stories.tsx. Consider extracting a shared handler to avoid drift and future copy edits.
Apply within this range:
handlers: [ http.post(`${DEFAULT_HOSTNAME_API}/2.0/metadata_queries/execute_read`, () => { return HttpResponse.json(mockMetadata); }), http.get(`${DEFAULT_HOSTNAME_API}/2.0/metadata_templates/enterprise/templateName/schema`, () => { return HttpResponse.json(mockSchema); }), - http.get(`${DEFAULT_HOSTNAME_API}/2.0/folders/:id`, () => { - return HttpResponse.json(mockRootFolder); - }), + folderByIdHandler, ],Outside this range, add a reusable handler and import it:
- New module (example):
// src/elements/content-explorer/stories/handlers/folderHandlers.ts import { http, HttpResponse } from 'msw'; import { DEFAULT_HOSTNAME_API } from '../../../../constants'; import { mockRootFolder } from '../../../common/__mocks__/mockRootFolder'; export const folderByIdHandler = http.get(`${DEFAULT_HOSTNAME_API}/2.0/folders/:id`, () => { return HttpResponse.json(mockRootFolder); });
- Import in both stories:
import { folderByIdHandler } from '../handlers/folderHandlers';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/elements/content-explorer/ContentExplorer.tsx(3 hunks)src/elements/content-explorer/stories/MetadataView.stories.tsx(2 hunks)src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/elements/content-explorer/ContentExplorer.tsx
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/elements/content-explorer/stories/MetadataView.stories.tsx (1)
src/elements/common/__mocks__/mockRootFolder.ts (1)
mockRootFolder(925-925)
src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx (1)
src/elements/common/__mocks__/mockRootFolder.ts (1)
mockRootFolder(925-925)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: lint_test_build
- GitHub Check: Summary
🔇 Additional comments (4)
src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx (2)
11-11: Importing mockRootFolder to back folder-title fetch — LGTMThis aligns the visual story with the new folder-fields fetch and prevents real network calls for GET /folders/:id.
184-186: MSW GET /2.0/folders/:id mock added — LGTMThis should eliminate flakiness by ensuring the folder title is available once metadata loads. The route shape matches the app call and the mock payload is appropriate.
src/elements/content-explorer/stories/MetadataView.stories.tsx (2)
8-8: Importing mockRootFolder for folder-name resolution — LGTMConsistent with the visual story and supports the updated folder-fields fetch path.
124-126: MSW handler for GET /2.0/folders/:id — LGTMMatches the app’s GET-by-id call and returns the correct mock shape for the title. This should prevent the previously observed flakiness.
jfox-box
changed the title
jfox-box
force-pushed
the
fix-flaky-mdv-title
branch
from
17f2a46 to
aefbaf3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/elements/content-explorer/stories/MetadataView.stories.tsx (1)
124-126: MSW handler for folder fetch added; consider parameter-aware mockThe handler correctly intercepts GET /2.0/folders/:id and returns mockRootFolder, unblocking the folder-title fetch. As a minor improvement, you could respond conditionally based on the requested id (e.g., only return mockRootFolder for id === '0'), which helps catch accidental requests for unexpected folder ids in stories.
Example tweak:
- http.get(`${DEFAULT_HOSTNAME_API}/2.0/folders/:id`, () => { - return HttpResponse.json(mockRootFolder); - }), + http.get(`${DEFAULT_HOSTNAME_API}/2.0/folders/:id`, ({ params }) => { + const { id } = params as { id: string }; + return HttpResponse.json(id === '0' ? mockRootFolder : { message: 'Not found' }, { status: id === '0' ? 200 : 404 }); + }),src/elements/content-explorer/ContentExplorer.tsx (2)
407-411: Defer ancestor-folder-name fetch to avoid races; add guard to avoid redundant callsFetching after metadata results load removes the concurrency race. To prevent repeated calls on subsequent page loads (marker pagination) or successive queries, gate the call so it runs once per load.
Apply this diff to limit redundant calls:
- // Fetch ancestor folder name after metadata loads because the API calls interfere with each other - const { metadataQuery } = this.props; - if (metadataQuery?.ancestor_folder_id) { - this.fetchFolderName(metadataQuery.ancestor_folder_id); - } + // Fetch ancestor folder name after metadata loads (avoid racing with metadata requests) + const { metadataQuery } = this.props; + const folderId = metadataQuery?.ancestor_folder_id; + // Only fetch if we don't already have a rootName set to prevent duplicate requests across pages + if (folderId && !this.state.rootName) { + this.fetchFolderName(folderId); + }
647-654: Don’t escalate folder-name fetch failures to global error viewRouting getFolderFields errors to errorCallback will flip the entire UI into VIEW_ERROR even though metadata results succeeded. Swallowing errors (or logging) keeps UX resilient when only the title fetch fails.
Apply this diff:
- this.api.getFolderAPI().getFolderFields( + this.api.getFolderAPI().getFolderFields( folderId, ({ name }) => { this.setState({ rootName: name }); }, - this.errorCallback, + noop, // Don't escalate to global error view if fetching the folder name fails { fields: [FIELD_NAME] }, );If you prefer to log:
// replace noop with: (err: unknown) => console.warn('fetchFolderName failed', err)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/elements/content-explorer/ContentExplorer.tsx(3 hunks)src/elements/content-explorer/stories/MetadataView.stories.tsx(2 hunks)src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/elements/content-explorer/stories/MetadataView.stories.tsx (1)
src/elements/common/__mocks__/mockRootFolder.ts (1)
mockRootFolder(925-925)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: lint_test_build
- GitHub Check: Summary
🔇 Additional comments (2)
src/elements/content-explorer/stories/MetadataView.stories.tsx (1)
8-8: LGTM: Deterministic folder-title mocking is addedImporting mockRootFolder keeps the story stable and prevents external dependencies from flaking.
src/elements/content-explorer/ContentExplorer.tsx (1)
341-355: Mount flow simplification looks goodRemoving metadataQuery from the initial destructure and deferring to showMetadataQueryResults() in the DEFAULT_VIEW_METADATA branch is consistent with the intent to avoid concurrent calls on mount.
jfox-box
force-pushed
the
fix-flaky-mdv-title
branch
from
aefbaf3 to
b618b36
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/elements/content-explorer/ContentExplorer.tsx (2)
392-398: Be mindful of stale props in async flowsReading
metadataQueryandfeaturesonce here is fine, but they’re later used to kick off an async request. If the user changes filters/sorts/pages quickly, the eventual callback may reflect a stalemetadataQuery. See next comment for a guard strategy in the success callback.
399-407: Avoid carrying over a stalerootNamebetween metadata query runs
nextStatedoesn’t includerootName, so when the ancestor fetch is skipped (no ancestor id or v2 flag off), the previousrootNamemay persist. Clearing it on each metadata results update avoids a misleading subheader.One option is to set state with
rootName: ''immediately (and then update it once the folder name arrives), as suggested in the next comment’s diff.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/elements/content-explorer/ContentExplorer.tsx(3 hunks)src/elements/content-explorer/stories/MetadataView.stories.tsx(2 hunks)src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx
- src/elements/content-explorer/stories/MetadataView.stories.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: lint_test_build
- GitHub Check: Summary
🔇 Additional comments (3)
src/elements/content-explorer/ContentExplorer.tsx (3)
341-354: Good call deferring ancestor-folder fetch until after metadata results loadRemoving the early ancestor fetch from mount reduces overlapping API calls and addresses the flakiness described. Switching solely on defaultView here looks correct.
473-479: Comment helps clarify intentThe added comment makes the flow easier to follow. No issues.
409-425: Apply metadata immediately with asynchronous folder-name lookup
- Update state right away with
nextStateand initializerootNameto''for a snappier UX.- If
metadataQuery.ancestor_folder_idis set andcontentExplorer.metadataViewV2is enabled, fetch the folder name in the background.- In the success callback, guard against stale or out-of-order responses by verifying that:
this.props.metadataQuery?.ancestor_folder_id === ancestorFolderIdthis.state.view === VIEW_METADATA- Use a no-op error handler so that failures in this non-critical lookup don’t switch the entire view to error.
- Optionally, if the underlying API supports it, pass
{ forceFetch: true }to bypass cache/coalescing.Apply this diff:
// Apply metadata immediately; fetch folder name asynchronously if needed - // if v2, fetch folder name and add to state - if (metadataQuery?.ancestor_folder_id && isFeatureEnabled(features, 'contentExplorer.metadataViewV2')) { - this.api.getFolderAPI().getFolderFields( - metadataQuery.ancestor_folder_id, - ({ name }) => { - this.setState({ - ...nextState, - rootName: name || '', - }); - }, - this.errorCallback, - { fields: [FIELD_NAME] }, - ); - } else { - // No folder name to fetch, update state immediately with just metadata - this.setState(nextState); - } + this.setState({ ...nextState, rootName: '' }); + + if (metadataQuery?.ancestor_folder_id && isFeatureEnabled(features, 'contentExplorer.metadataViewV2')) { + const ancestorFolderId = metadataQuery.ancestor_folder_id; + this.api.getFolderAPI().getFolderFields( + ancestorFolderId, + ({ name }) => { + if ( + this.props.metadataQuery?.ancestor_folder_id !== ancestorFolderId || + this.state.view !== VIEW_METADATA + ) { + return; + } + this.setState({ rootName: name || '' }); + }, + () => { + // no-op: folder-name lookup failure should not show error view + }, + { fields: [FIELD_NAME] /*, forceFetch: true*/ }, + ); + }Please verify whether
getFolderFieldssupports aforceFetchoption before uncommenting and including it.
jfox-box
force-pushed
the
fix-flaky-mdv-title
branch
from
b618b36 to
13c7d7b
Compare
| // if v2, fetch folder name and add to state | ||
| if (metadataQuery?.ancestor_folder_id && isFeatureEnabled(features, 'contentExplorer.metadataViewV2')) { | ||
| this.api.getFolderAPI().getFolderFields( | ||
| metadataQuery.ancestor_folder_id, | ||
| ({ name }) => { | ||
| this.setState({ | ||
| ...nextState, | ||
| rootName: name || '', | ||
| }); | ||
| }, | ||
| this.errorCallback, | ||
| { fields: [FIELD_NAME] }, | ||
| ); | ||
| } else { | ||
| // No folder name to fetch, update state immediately with just metadata | ||
| this.setState(nextState); | ||
| } |
There was a problem hiding this comment.
to get to this point in the code, there already needs to exist a ancestor_folder_id since it's a required field on the metadata request. i.e. this callback wouldn't have been called if the ancestor id was missing so you don't need to check for it
let's reverse the if clause so we can remove the extra indents:
if (title || !isFeatureEnabled(features, 'contentExplorer.metadataViewV2')) {
this.setState(nextState);
return;
}
this.api.getFolderAPI().getFolderFields(
metadataQuery.ancestor_folder_id,
({ name }) => {
this.setState({ ..nextState, rootName: name });
},
this.errorCallback,
{ fields: [FIELD_NAME] },
);
you shouldn't need to set an empty string for name || '' since name is a required property a folder
There was a problem hiding this comment.
Sorry branch merged before I had a chance to respond to this, will address this in a future PR
All of our API calls delete each other when run, so we can't run them concurrently or we run into race conditions. This fixes the flaky test by running the fetchFolderFields after the metadata query finishes.
Also updates the metadata view story to use a mocked folder to show this title.
Summary by CodeRabbit
Bug Fixes
Tests