feat(preview): add minimalFile prop for content sidebar to render faster #4412
Conversation
WalkthroughContentSidebar now accepts an optional Changes
Sequence Diagram(s)(omitted — change is focused within component rendering logic and tests; no multi-component sequential flow requiring visualization) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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. Comment |
![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 (1)
src/elements/content-sidebar/__tests__/ContentSidebar.test.js (1)
356-382: Remove duplicate test cases.Tests 4 and 5 (lines 356-382) are functionally identical to test 1 (lines 310-324). All three tests:
- Set up a wrapper with
fileIdandminimalFile- Set state to
{ file: undefined, isLoading: true }- Assert that
isLoadingistrueTest 1 already comprehensively covers this scenario by verifying that the sidebar renders, receives the correct file prop, and has
isLoadingset totrue.♻️ Consolidate duplicate tests
Remove tests 4 and 5, as test 1 already validates the same behavior:
- test('should pass isLoading as true when using minimalFile without full file data', () => { - const wrapper = getWrapper({ - fileId: 'test_id', - minimalFile, - }); - - act(() => { - wrapper.setState({ file: undefined, isLoading: true }); - }); - wrapper.update(); - - expect(wrapper.find('sidebar').prop('isLoading')).toBe(true); - }); - - test('should pass isLoading as true when file is loading even with minimalFile', () => { - const wrapper = getWrapper({ - fileId: 'test_id', - minimalFile, - }); - - act(() => { - wrapper.setState({ file: undefined, isLoading: true }); - }); - wrapper.update(); - - expect(wrapper.find('sidebar').prop('isLoading')).toBe(true); - });
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/elements/content-sidebar/ContentSidebar.jssrc/elements/content-sidebar/__tests__/ContentSidebar.test.js
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: rafalmaksymiuk
Repo: box/box-ui-elements PR: 4136
File: src/elements/common/types/SidebarNavigation.ts:16-26
Timestamp: 2025-06-11T16:30:10.431Z
Learning: `VersionSidebarView` intentionally uses the `versionId` field to stay consistent with current URL parameter naming; a potential rename to `fileVersionId` is deferred until React Router is removed.
Learnt from: fpan225
Repo: box/box-ui-elements PR: 4239
File: src/elements/content-sidebar/SidebarPanels.js:0-0
Timestamp: 2025-09-03T18:30:44.447Z
Learning: In the CustomSidebarPanel type, the component field is required (React.ComponentType<any>), so runtime checks for component existence are unnecessary since Flow will catch missing components at compile time. User fpan225 prefers to rely on the type system rather than adding redundant runtime checks.
📚 Learning: 2025-06-11T16:30:10.431Z
Learnt from: rafalmaksymiuk
Repo: box/box-ui-elements PR: 4136
File: src/elements/common/types/SidebarNavigation.ts:16-26
Timestamp: 2025-06-11T16:30:10.431Z
Learning: `VersionSidebarView` intentionally uses the `versionId` field to stay consistent with current URL parameter naming; a potential rename to `fileVersionId` is deferred until React Router is removed.
Applied to files:
src/elements/content-sidebar/ContentSidebar.js
📚 Learning: 2025-06-17T13:30:02.172Z
Learnt from: rafalmaksymiuk
Repo: box/box-ui-elements PR: 4144
File: src/elements/content-sidebar/versions/VersionsList.js:24-33
Timestamp: 2025-06-17T13:30:02.172Z
Learning: In the box-ui-elements codebase, Flow components use .flow.js type definition files, not TypeScript .ts files. The InternalSidebarNavigation type is a union type where different variants may have different properties like versionId, and proper type safety is ensured through conditional checks in methods like getSelectedVersionId.
Applied to files:
src/elements/content-sidebar/ContentSidebar.js
🧬 Code graph analysis (1)
src/elements/content-sidebar/__tests__/ContentSidebar.test.js (2)
src/elements/content-sidebar/__tests__/SidebarFileProperties.test.js (1)
getWrapper(10-10)src/elements/content-sidebar/__tests__/DetailsSidebar.test.js (2)
getWrapper(26-36)file(15-18)
🪛 Biome (2.1.2)
src/elements/content-sidebar/ContentSidebar.js
[error] 389-389: type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 390-390: type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
⏰ 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: semgrep-cloud-platform/scan
- GitHub Check: lint_test_build
- GitHub Check: Summary
🔇 Additional comments (5)
src/elements/content-sidebar/ContentSidebar.js (5)
99-100: LGTM!The
minimalFileprop is well-documented and correctly typed as an optionalBoxItem. The JSDoc clearly explains its purpose for rendering an immediate shell while full data loads.
388-393: LGTM!The
displayFilelogic correctly implements the fallback pattern, prioritizing full file data over minimal file data. This ensures the sidebar uses the most complete data available.
424-424: LGTM!The
isLoadinglogic correctly ensures that the loading state istruewhenever full file data is unavailable, even whenminimalFileis present. This provides accurate feedback to users that complete data is still being fetched.
395-397: No issues found. The implementation safely handles minimal file data throughlodash/get(getProp) which uses default values for missing paths. ThehasSkillsDatafunction accesses nested properties likemetadata.global.boxSkillsCards.cardswith a fallback to an empty array, so minimalFile objects lacking these properties simply return false without errors. Existing test coverage (line 310-324 of ContentSidebar.test.js) validates this behavior.
413-413: BoxAISidebar may receive undefinedfileExtensionwhen rendering withminimalFile.The
BoxAISidebarcomponent requires a non-optionalfileExtension: stringprop (line 21 ofBoxAISidebar.tsx), butminimalFilelacks anextensionproperty. WhendisplayFileis set tominimalFile(line 393 ofContentSidebar.js), the code at line 284 ofSidebarPanels.jspassesfileExtension={file.extension}which evaluates toundefined, creating a type mismatch.Ensure
minimalFileincludes anextensionproperty, or add a defensive fallback likefileExtension={file.extension || ''}when passing it toBoxAISidebar.⛔ Skipped due to learnings
Learnt from: rafalmaksymiuk Repo: box/box-ui-elements PR: 4136 File: src/elements/common/types/SidebarNavigation.ts:16-26 Timestamp: 2025-06-11T16:30:10.431Z Learning: `VersionSidebarView` intentionally uses the `versionId` field to stay consistent with current URL parameter naming; a potential rename to `fileVersionId` is deferred until React Router is removed.Learnt from: fpan225 Repo: box/box-ui-elements PR: 4239 File: src/elements/content-sidebar/SidebarPanels.js:0-0 Timestamp: 2025-09-03T18:30:44.447Z Learning: In the CustomSidebarPanel type, the component field is required (React.ComponentType<any>), so runtime checks for component existence are unnecessary since Flow will catch missing components at compile time. User fpan225 prefers to rely on the type system rather than adding redundant runtime checks.Learnt from: rafalmaksymiuk Repo: box/box-ui-elements PR: 4144 File: src/elements/content-sidebar/versions/VersionsList.js:24-33 Timestamp: 2025-06-17T13:30:02.172Z Learning: In the box-ui-elements codebase, Flow components use .flow.js type definition files, not TypeScript .ts files. The InternalSidebarNavigation type is a union type where different variants may have different properties like versionId, and proper type safety is ensured through conditional checks in methods like getSelectedVersionId.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/elements/content-sidebar/__tests__/ContentSidebar.test.js (2)
343-350: Remove redundant mock setup.Line 344 redundantly sets
SidebarUtils.shouldRenderSidebarwhen it's already configured in thebeforeEachat line 307.♻️ Proposed refactor
test('should return null when neither file nor minimalFile is available', () => { - SidebarUtils.shouldRenderSidebar = jest.fn().mockReturnValue(true); const wrapper = getWrapper({ fileId: 'test_id', });
352-376: Consider consolidating duplicate test coverage.Tests 4 and 5 duplicate assertions already covered in tests 1 and 2:
- Test 4 checks
isLoadingwithminimalFile(already verified at line 321)- Test 5 checks
isLoadingafter fetch completes (already verified at line 340)Consider removing these tests to reduce maintenance overhead.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/elements/content-sidebar/__tests__/ContentSidebar.test.js
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: rafalmaksymiuk
Repo: box/box-ui-elements PR: 4136
File: src/elements/common/types/SidebarNavigation.ts:16-26
Timestamp: 2025-06-11T16:30:10.431Z
Learning: `VersionSidebarView` intentionally uses the `versionId` field to stay consistent with current URL parameter naming; a potential rename to `fileVersionId` is deferred until React Router is removed.
⏰ 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: semgrep-cloud-platform/scan
- GitHub Check: Summary
🔇 Additional comments (2)
src/elements/content-sidebar/__tests__/ContentSidebar.test.js (2)
298-308: LGTM! Clean test suite setup.The
minimalFilefixture and mock setup are appropriate for testing the minimal file rendering scenarios.
310-341: LGTM! Core transition logic is well-tested.Tests 1 and 2 correctly verify the minimalFile → full file transition and the corresponding loading state changes.
Merge Queue Status✅ The pull request has been merged at 6c5da06 This pull request spent 5 seconds in the queue, with no time running CI. Required conditions to merge
|
…ter (#4412) * feat(preview): add minimalFile prop for content sidebar to render faster * feat(preview): add minimalFile prop for content sidebar to render faster * fix: address comments
3 participants
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.