feat(router): optional router in VersionsList #4144
Conversation
|
""" WalkthroughThe changes introduce optional Changes
Sequence Diagram(s)sequenceDiagram
participant ParentComponent
participant VersionsMenu
participant VersionsGroup
participant VersionsList
participant VersionsItem
ParentComponent->>VersionsMenu: Render (with optional routerDisabled, internalSidebarNavigation)
VersionsMenu->>VersionsGroup: Pass props (routerDisabled, internalSidebarNavigation)
VersionsGroup->>VersionsList: Pass props (routerDisabled, internalSidebarNavigation)
loop For each version
VersionsList->>VersionsItem: Render (selection logic depends on routerDisabled/internalSidebarNavigation)
end
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
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: 1
🔭 Outside diff range comments (1)
src/elements/content-sidebar/versions/VersionsMenu.js (1)
13-14: MissingInternalSidebarNavigationimport breaks Flow type-checking
Propsnow referencesInternalSidebarNavigation, but the file doesn’t import that symbol. Flow will fail to resolve the type and halt compilation.import type { BoxItemVersion } from '../../../common/types/core'; +import type { InternalSidebarNavigation } from '../../common/types/SidebarNavigation.flow';(Adjust the relative path if the type lives elsewhere.)
Also applies to: 19-21
♻️ Duplicate comments (2)
src/elements/content-sidebar/versions/VersionsList.js (1)
11-12: Same import-path concern as in VersionsGroupEnsure the referenced
SidebarNavigation.flowfile actually exists, or switch to the correct extension.src/elements/content-sidebar/versions/__tests__/VersionsList.test.js (1)
6-18: Samedefaultexport note forVersionsItemmockFor consistency with the source file’s default export, expose it explicitly in the mock (see previous comment).
🧹 Nitpick comments (4)
src/elements/content-sidebar/versions/VersionsMenu.js (1)
19-21: Consider defaultingrouterDisabledtofalsein the prop destructuringEverywhere else you treat
routerDisabledas a boolean. Adding a default keeps the type simple and avoidsundefinedchecks downstream.-routerDisabled?: boolean, +routerDisabled?: boolean, // defaults to false downstreamOptional but recommended.
src/elements/content-sidebar/versions/VersionsList.js (2)
24-33: Leverage optional chaining & tighten null-handling
getSelectedVersionIdcan be simplified and made safer via optional chaining:- const getSelectedVersionId = () => { - if (internalSidebarNavigation && internalSidebarNavigation.versionId) { - return internalSidebarNavigation.versionId; - } - return null; - }; + const selectedVersionId = internalSidebarNavigation?.versionId ?? null;This eliminates an inner function, trims allocations, and satisfies eslint’s
useOptionalChainhint.
60-62: Edge-case: internal nav provided while router enabledIf both
routerDisabled=false(default) andinternalSidebarNavigationare supplied, the component ignores the latter, potentially surprising integrators.Consider documenting this precedence or emitting a warning when both are present.
src/elements/content-sidebar/versions/__tests__/VersionsMenu.test.js (1)
9-36: Mock module should exportdefaultto mimic real component
VersionsGroupis a default export. Returning a bare function works with Jest’s module-factory heuristics, but being explicit avoids edge-cases with ESModule interop:return { - default: MockVersionsGroup, -}; + __esModule: true, + default: MockVersionsGroup, };Minor, yet it future-proofs the test when switching to native ESM.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/elements/content-sidebar/versions/__tests__/__snapshots__/VersionsList.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (5)
src/elements/content-sidebar/versions/VersionsGroup.js(1 hunks)src/elements/content-sidebar/versions/VersionsList.js(1 hunks)src/elements/content-sidebar/versions/VersionsMenu.js(1 hunks)src/elements/content-sidebar/versions/__tests__/VersionsList.test.js(1 hunks)src/elements/content-sidebar/versions/__tests__/VersionsMenu.test.js(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
src/elements/content-sidebar/versions/VersionsMenu.js (1)
src/elements/common/types/SidebarNavigation.ts (1)
InternalSidebarNavigation(41-43)
src/elements/content-sidebar/versions/VersionsGroup.js (1)
src/elements/common/types/SidebarNavigation.ts (1)
InternalSidebarNavigation(41-43)
src/elements/content-sidebar/versions/VersionsList.js (1)
src/elements/common/types/SidebarNavigation.ts (1)
InternalSidebarNavigation(41-43)
🪛 Biome (1.9.4)
src/elements/content-sidebar/versions/VersionsGroup.js
[error] 10-10: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
src/elements/content-sidebar/versions/VersionsList.js
[error] 11-11: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 13-22: type alias are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 24-24: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 34-34: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 43-43: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 26-26: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: lint_test_build
- GitHub Check: Summary
🔇 Additional comments (1)
src/elements/content-sidebar/versions/VersionsGroup.js (1)
10-18: Path / extension appears inconsistent with existing type fileYou’re importing from
../../common/types/SidebarNavigation.flow, yet the provided repo snippet showsSidebarNavigation.ts. If the.flowversion does not exist the build will break.Verify the correct location/extension and update accordingly:
-import type { InternalSidebarNavigation } from '../../common/types/SidebarNavigation.flow'; +import type { InternalSidebarNavigation } from '../../common/types/SidebarNavigation';(or whatever the canonical Flow path is).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/elements/content-sidebar/versions/VersionsMenu.js (1)
14-22: Default-value forrouterDisabledwould avoid repeated undefined checks
routerDisabledis optional and later forwarded via the spread operator. A small improvement is to give it an explicit default (false) at the edge of the component so downstream consumers can always rely on a boolean.-const VersionsMenu = React.memo<Props>(({ intl, versions, ...rest }: Props) => { +const VersionsMenu = React.memo<Props>( + ({ intl, versions, routerDisabled = false, ...rest }: Props) => {Not critical, but it tightens the contract and avoids
undefinedchecks in children.src/elements/content-sidebar/versions/VersionsList.js (2)
24-32:renderVersionItemWithoutRouteris recreated on every renderBecause the arrow function is defined inside the component body, a brand-new function instance is created for every rerender, which prevents
React.memo(VersionsItem)from memoising effectively. Wrapping the callback inReact.useCallback(or hoisting it outside) eliminates this churn.-const renderVersionItemWithoutRouter = (version: BoxItemVersion) => ( +const renderVersionItemWithoutRouter = React.useCallback( + (version: BoxItemVersion) => ( <VersionsItem isCurrent={currentId === version.id} isSelected={internalSidebarNavigation?.versionId === version.id} version={version} {...rest} /> -); + ), + [currentId, internalSidebarNavigation?.versionId, rest], +);
47-55: Minor readability tweak – inline conditional is enoughSince the two render helpers are one-liners, you can simplify the JSX without the intermediate functions:
<li key={version.id}> {routerDisabled ? ( <VersionsItem ... /> ) : ( <Route render={...} /> )} </li>This removes some indirection and makes the branching logic self-evident.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/elements/content-sidebar/versions/VersionsList.js(1 hunks)src/elements/content-sidebar/versions/VersionsMenu.js(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
src/elements/content-sidebar/versions/VersionsList.js (1)
Learnt from: rafalmaksymiuk
PR: box/box-ui-elements#4144
File: src/elements/content-sidebar/versions/VersionsList.js:24-33
Timestamp: 2025-06-17T13:30:02.133Z
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.
src/elements/content-sidebar/versions/VersionsMenu.js (1)
Learnt from: rafalmaksymiuk
PR: box/box-ui-elements#4144
File: src/elements/content-sidebar/versions/VersionsList.js:24-33
Timestamp: 2025-06-17T13:30:02.133Z
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.
🧬 Code Graph Analysis (2)
src/elements/content-sidebar/versions/VersionsList.js (1)
src/elements/common/types/SidebarNavigation.ts (1)
InternalSidebarNavigation(41-43)
src/elements/content-sidebar/versions/VersionsMenu.js (1)
src/elements/common/types/SidebarNavigation.ts (1)
InternalSidebarNavigation(41-43)
🪛 Biome (1.9.4)
src/elements/content-sidebar/versions/VersionsList.js
[error] 11-11: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 13-22: type alias are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 24-24: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 25-25: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 34-34: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
src/elements/content-sidebar/versions/VersionsMenu.js
[error] 14-14: 'import type' 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 (2)
- GitHub Check: lint_test_build
- GitHub Check: Summary
🔇 Additional comments (1)
src/elements/content-sidebar/versions/VersionsList.js (1)
34-45: VerifyRouteusage without an explicitpath
<Route render={...} />without apathprop relies wholly on the parent route formatch.params.
That’s fine when this list always lives under a route that carries a:versionIdparam, but if the component is ever reused elsewhere (e.g., in the router-disabled mode) this implicit assumption breaks.Please double-check the mounting locations or consider making the dependency explicit:
<Route path=":versionId?" render={...} />
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/elements/common/types/SidebarNavigation.flow.js (2)
3-4: Clarify wording and fix “Type Script” typoImprove the developer-facing comment for clarity and correct spelling (“TypeScript” is one word).
-// flow version is simplified compared to Type Script due to difficult to resolve problems with Union Types -// Type Script works better with Union Types +// Flow version is simplified compared to the TypeScript version because Flow +// currently struggles with discriminated union types, whereas TypeScript handles +// them more gracefully.
24-29: Prefer exact object types to avoid silent excess propsUsing Flow’s exact object syntax (
{| … |}) prevents callers from passing undeclared keys, preserving the type-safety that was lost when the discriminated union was flattened.-export type SidebarNavigation = { +export type SidebarNavigation = {| sidebar: ViewTypeValues, versionId?: string, activeFeedEntryType?: FeedEntryTypeValues, activeFeedEntryId?: string, fileVersionId?: string, -}; +|}; -export type InternalSidebarNavigation = SidebarNavigation & { - open: boolean, -}; +export type InternalSidebarNavigation = SidebarNavigation & {| open: boolean |};src/elements/content-preview/__tests__/ContentPreview.test.js (1)
508-509: Unnecessary prop – can be removed entirely
contentSidebarPropsis now an empty object and not referenced in this test. Omitting it makes the intent clearer and reduces noise.- contentSidebarProps: {}, + // no sidebar-specific props needed for this test case
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/elements/common/types/SidebarNavigation.flow.js(2 hunks)src/elements/content-preview/__tests__/ContentPreview.test.js(1 hunks)src/elements/content-preview/stories/ContentPreview.stories.js(0 hunks)
💤 Files with no reviewable changes (1)
- src/elements/content-preview/stories/ContentPreview.stories.js
🧰 Additional context used
🧠 Learnings (1)
src/elements/common/types/SidebarNavigation.flow.js (1)
Learnt from: rafalmaksymiuk
PR: box/box-ui-elements#4144
File: src/elements/content-sidebar/versions/VersionsList.js:24-33
Timestamp: 2025-06-17T13:30:02.133Z
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.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: lint_test_build
- GitHub Check: Summary
src/elements/content-sidebar/versions/__tests__/VersionsMenu.test.js
Outdated
Show resolved
Hide resolved
src/elements/content-sidebar/versions/__tests__/VersionsMenu.test.js
Outdated
Show resolved
Hide resolved
src/elements/content-sidebar/versions/__tests__/VersionsMenu.test.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
src/elements/common/types/SidebarNavigation.js.flow (2)
3-5: Comment wording is unclear and out of placeThe comment explains why the Flow definition is simpler than the TS one, but the phrasing is awkward (“due to difficult to resolve problems”) and mixes two sentences into one. Consider re-wording or removing to keep the type file concise.
24-30: Over-permissive unified type may mask invalid state combinationsCollapsing all sidebar variants into a single “everything-is-optional” object loses the exhaustiveness checks we previously enjoyed with the dedicated union. It is now possible to form states such as
{ sidebar: ViewType.ACTIVITY, versionId: '123' }
which are meaningless for the activity view but Flow will happily accept.If retaining the union is not feasible, consider at least tightening the shape with exact object types and discriminated helpers, e.g.:
-export type SidebarNavigation = { +export type SidebarNavigation = {| sidebar: ViewTypeValues, versionId?: string, activeFeedEntryType?: FeedEntryTypeValues, activeFeedEntryId?: string, fileVersionId?: string, -}; +|};…and add runtime guards (or helper constructors) to prevent illegal mixes of props.
src/elements/content-sidebar/versions/VersionsMenu.js (1)
79-83:keymight collide when two groups share the same heading
groupHeadingalone is used as a React key. Although headings are usually unique, it only takes two “2024” or “This Month” groups rendered in the list to trigger a React warning and subtle reconciliation issues.-<li className="bcs-VersionsMenu-item" key={groupHeading}> +<li className="bcs-VersionsMenu-item" key={`${groupHeading}-${groupVersions[0].id}`}>Using the id of the first version (or the map index) guarantees uniqueness.
src/elements/content-sidebar/versions/VersionsGroup.js (1)
16-18: Unused props inside component
internalSidebarNavigationandrouterDisabledare accepted but never referenced inVersionsGroup; they are only forwarded toVersionsList. Consider documenting this “pass-through” intent or narrowing the prop spread ({...rest}) to avoid accidental leakage in the future.src/elements/content-sidebar/versions/VersionsList.js (2)
34-45: Small performance / readability tweak – avoid recreating callback per item
renderVersionItemWithRouterandrenderVersionItemWithoutRouterare declared on every render and immediately invoked in the map. You can inline the logic or hoist the callbacks withReact.useCallbackto prevent needless re-allocations, especially when the list is long.-const renderVersionItemWithoutRouter = (version: BoxItemVersion) => ( +const renderVersionItemWithoutRouter = React.useCallback( + (version: BoxItemVersion) => ( <VersionsItem isCurrent={currentId === version.id} isSelected={internalSidebarNavigation?.versionId === version.id} version={version} {...rest} /> -); + ), + [currentId, internalSidebarNavigation, rest], +);Same applies to the router variant.
34-41: Safety check for missingmatch.params.versionIdIf the current router context doesn’t provide a
versionIdparam,match.params.versionIdisundefined, which is fine but forces an unnecessary property access. Safer (and slightly faster):-isSelected={match.params.versionId === version.id} +isSelected={match.params?.versionId === version.id}Not critical, but removes a potential “cannot read property of undefined” if the Route hierarchy changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/elements/common/types/SidebarNavigation.js.flow(2 hunks)src/elements/content-sidebar/versions/VersionsGroup.js(1 hunks)src/elements/content-sidebar/versions/VersionsList.js(1 hunks)src/elements/content-sidebar/versions/VersionsMenu.js(1 hunks)src/elements/content-sidebar/versions/__tests__/VersionsMenu.test.js(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/elements/content-sidebar/versions/tests/VersionsMenu.test.js
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: rafalmaksymiuk
PR: box/box-ui-elements#4144
File: src/elements/content-sidebar/versions/VersionsList.js:24-33
Timestamp: 2025-06-17T13:30:02.133Z
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.
src/elements/common/types/SidebarNavigation.js.flow (1)
Learnt from: rafalmaksymiuk
PR: box/box-ui-elements#4144
File: src/elements/content-sidebar/versions/VersionsList.js:24-33
Timestamp: 2025-06-17T13:30:02.133Z
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.
src/elements/content-sidebar/versions/VersionsList.js (1)
Learnt from: rafalmaksymiuk
PR: box/box-ui-elements#4144
File: src/elements/content-sidebar/versions/VersionsList.js:24-33
Timestamp: 2025-06-17T13:30:02.133Z
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.
🪛 Biome (1.9.4)
src/elements/content-sidebar/versions/VersionsGroup.js
[error] 10-10: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
src/elements/content-sidebar/versions/VersionsList.js
[error] 11-11: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 13-22: type alias are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 24-24: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 25-25: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 34-34: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
src/elements/content-sidebar/versions/VersionsMenu.js
[error] 14-14: 'import type' 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 (2)
- GitHub Check: lint_test_build
- GitHub Check: Summary
…erted VersionsMenu tests to RTL
rafalmaksymiuk
force-pushed
the
FROFRA-30
branch
from
10c3db0 to
6a75b42
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/elements/content-sidebar/versions/VersionsMenu.js (1)
14-22: Props extended correctly – consider documenting default behaviour
internalSidebarNavigationandrouterDisabledare correctly added to thePropstype and forwarded via the spread operator below. To avoid readers hunting through child components, add a short JSDoc sentence (or even a tiny inline comment) explaining thatrouterDisableddefaults tofalseand thatinternalSidebarNavigationis only respected when it’strue.src/elements/content-sidebar/versions/VersionsList.js (1)
48-55: Minor render-time perf – extract mapperCreating a new closure for every list item on each render (
versions.map(version => (…)) is negligible here, but if the list grows it causes needless reconciliations. A tiny optimisation:- {versions.map(version => ( + {versions.map(renderFn)}where
renderFnisuseCallback-memoised to the chosen renderer.src/elements/content-sidebar/versions/__tests__/VersionsMenu.test.js (2)
5-32: Module mock should return an ES-module object to avoid jest interop edge-casesReturning a bare function works for CommonJS default imports, but if the source module ever switches to
export defaultES syntax the mock breaks. Safer pattern:jest.mock('../VersionsGroup', () => ({ __esModule: true, default: jest.fn(props => <Mock {...props} />), }));
49-52: Date mocking replaces global constructor – static helpers lostOverwriting
global.Datewith a function nukesDate.parse,UTC, etc. A safer approach is to usejest.requireActual('date-mock')orMockDate.set(...), or at least subclass:const MockedDate = class extends GlobalDate { constructor(date) { super(date || defaultDate); } }; global.Date = MockedDate;This preserves static helpers and avoids surprises in util functions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/elements/content-sidebar/versions/__tests__/__snapshots__/VersionsList.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (6)
src/elements/common/types/SidebarNavigation.js.flow(2 hunks)src/elements/content-sidebar/versions/VersionsGroup.js(1 hunks)src/elements/content-sidebar/versions/VersionsList.js(1 hunks)src/elements/content-sidebar/versions/VersionsMenu.js(1 hunks)src/elements/content-sidebar/versions/__tests__/VersionsList.test.js(1 hunks)src/elements/content-sidebar/versions/__tests__/VersionsMenu.test.js(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/elements/common/types/SidebarNavigation.js.flow
- src/elements/content-sidebar/versions/tests/VersionsList.test.js
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: rafalmaksymiuk
PR: box/box-ui-elements#4144
File: src/elements/content-sidebar/versions/VersionsList.js:24-33
Timestamp: 2025-06-17T13:30:02.133Z
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.
src/elements/content-sidebar/versions/VersionsList.js (1)
Learnt from: rafalmaksymiuk
PR: box/box-ui-elements#4144
File: src/elements/content-sidebar/versions/VersionsList.js:24-33
Timestamp: 2025-06-17T13:30:02.133Z
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.
🪛 Biome (1.9.4)
src/elements/content-sidebar/versions/VersionsGroup.js
[error] 10-10: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
src/elements/content-sidebar/versions/VersionsList.js
[error] 11-11: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 13-22: type alias are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 24-24: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 25-25: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 34-34: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
src/elements/content-sidebar/versions/VersionsMenu.js
[error] 14-14: 'import type' 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 (2)
- GitHub Check: lint_test_build
- GitHub Check: Summary
🔇 Additional comments (2)
src/elements/content-sidebar/versions/VersionsGroup.js (1)
10-18: Prop drilling stays thin – verifyVersionsListaccepts the extras
internalSidebarNavigationandrouterDisabledare forwarded wholesale via{...rest}. Double-check thatVersionsList’s Flow props include both, otherwise Flow will emit an “inexact/extra prop” error.src/elements/content-sidebar/versions/VersionsList.js (1)
34-45:Routewithoutpathrelies on an upstream param – confirm the contract
match.params.versionIdwill only be populated if a parentRoutealready matched:versionId. If that parent is ever removed or its param renamed, selection silently stops working. Either:
- Provide an explicit relative path here (
path=":versionId?"), or- Add a comment clarifying the dependency on the parent route structure.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/elements/content-sidebar/versions/__tests__/VersionsList.test.js (2)
86-95: Also assert that other items are not selectedThe test confirms that the routed version is marked as selected, but it never checks that the remaining list items are not selected.
A negative assertion prevents silent regressions where multiple items become selected.const version12345Container = screen.getByTestId('versions-item-12345'); expect(version12345Container).toBeInTheDocument(); expect(version12345Container).toHaveTextContent('Selected'); expect(screen.getAllByText('Selected')).toHaveLength(1); + +// Ensure the other item is not selected +const version67890Container = screen.getByTestId('versions-item-67890'); +expect(version67890Container).not.toHaveTextContent('Selected');
97-112: Mirror the negative-case assertion for internal navigationSame reasoning as above: prove that only the internally-selected version is marked.
const version67890Container = screen.getByTestId('versions-item-67890'); expect(version67890Container).toBeInTheDocument(); expect(version67890Container).toHaveTextContent('Selected'); expect(screen.getAllByText('Selected')).toHaveLength(1); + +// Ensure the other item is not selected +const version12345Container = screen.getByTestId('versions-item-12345'); +expect(version12345Container).not.toHaveTextContent('Selected');
🧹 Nitpick comments (2)
src/elements/content-sidebar/versions/__tests__/VersionsList.test.js (2)
6-18: Return an ES-module-compatible mock objectReturning the naked function works for CommonJS, but Jest will not expose it as a
defaultexport when__esModuleinterop is expected.
Wrap the function into an object with__esModule: true, default: …to guard against future ESM migration pains.- return MockVersionsItem; + return { __esModule: true, default: MockVersionsItem };
132-145: Assert via mock-call data instead of rendered textThe purpose of this test is to verify prop plumbing.
Relying on rendered text couples the test to the mock’s implementation details.
Checking the mock’s call arguments is both clearer and less brittle:-renderComponent(props); - -expect(screen.getByText('FileID: f_123')).toBeInTheDocument(); -expect(screen.getByText('VersionCount: 10')).toBeInTheDocument(); -expect(screen.getByText('VersionLimit: 100')).toBeInTheDocument(); +renderComponent(props); + +expect(require('../VersionsItem')).toHaveBeenCalledWith( + expect.objectContaining({ + fileId: 'f_123', + versionCount: 10, + versionLimit: 100, + }), + expect.anything(), // context argument +);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/elements/content-sidebar/versions/__tests__/VersionsList.test.js(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: rafalmaksymiuk
PR: box/box-ui-elements#4144
File: src/elements/content-sidebar/versions/VersionsList.js:24-33
Timestamp: 2025-06-17T13:30:02.133Z
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.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: lint_test_build
- GitHub Check: Summary
Added support for optional react-router in VersionsList component. Now when used with routerDisabled prop set to true, it will use internalSidebarNavigation to find navigation state instead of router methods.
Summary by CodeRabbit
New Features
Refactor
Tests