feat(router): withRouterIfEnabled #4221
Conversation
WalkthroughAdds a conditional HOC Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant HOC as withRouterIfEnabled
participant Flags as isFeatureEnabled
participant Router as react-router withRouter
participant C as WrappedComponent
App->>HOC: Render(WrappedComponent, props)
HOC->>Flags: check props.features for "routerDisabled.value"
alt router disabled (prop or feature)
HOC->>C: Render original component with props (no router injection)
else router enabled
HOC->>Router: wrap component => WrappedWithRouter
HOC->>WrappedWithRouter: Render (injected history/location/match)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ 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: 1
🧹 Nitpick comments (4)
src/elements/common/routing/withRouterIfEnabled.js (2)
17-21: Hoist non-React statics from WrappedThe outer HOC doesn’t hoist statics (defaultProps, static methods, etc.). react-router’s withRouter hoists them; your wrapper should, too, to avoid regressions where consumers read statics off the exported component.
Suggested change:
import * as React from 'react'; import { withRouter } from 'react-router-dom'; import { isFeatureEnabled } from '../feature-checking'; +import hoistNonReactStatics from 'hoist-non-react-statics'; export default function withRouterIfEnabled(Wrapped: React.ComponentType<any>) { const WrappedWithRouter = withRouter(Wrapped); const WithRouterIfEnabled = (props: any) => { const routerDisabled = props?.routerDisabled === true || isFeatureEnabled(props?.features, 'routerDisabled.value'); const Component = routerDisabled ? Wrapped : WrappedWithRouter; return <Component {...props} />; }; const name = Wrapped.displayName || Wrapped.name || 'Component'; WithRouterIfEnabled.displayName = `withRouterIfEnabled(${name})`; + hoistNonReactStatics(WithRouterIfEnabled, Wrapped); return WithRouterIfEnabled; }
6-6: Optional: tighten types with genericsIf you want Flow to preserve prop types through the HOC, you can add a generic:
export default function withRouterIfEnabled<T: {}>( Wrapped: React.ComponentType<T>, ): React.ComponentType<T & { routerDisabled?: boolean, features?: mixed }> { ... }This is optional, but helps catch prop mistakes at compile time.
src/elements/content-sidebar/versions/VersionsSidebarContainer.js (1)
353-353: Consider marking history/match as optional in PropsUnder routerDisabled, withRouterIfEnabled won’t inject router props. The implementation already guards usage, but Flow types still require
historyandmatch. Consider:type Props = { - history: RouterHistory, + history?: RouterHistory, - match: Match, + match?: Match, ... }This better reflects runtime and avoids misleading type requirements.
src/elements/common/routing/__tests__/withRouterIfEnabled.test.js (1)
52-63: Feature-flag disable test LGTM; consider adding displayName/static hoisting checksOptionally add:
- displayName assertion: expect(WithRouterIfEnabled.displayName).toBe('withRouterIfEnabled(TestComponent)')
- If you adopt static hoisting, a test that a static property on TestComponent is visible on WithRouterIfEnabled.
These would guard future regressions in the HOC wrapper behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/elements/common/routing/__tests__/withRouterIfEnabled.test.js(1 hunks)src/elements/common/routing/index.js(1 hunks)src/elements/common/routing/withRouterIfEnabled.js(1 hunks)src/elements/content-sidebar/AddTaskButton.js(2 hunks)src/elements/content-sidebar/SidebarToggle.js(2 hunks)src/elements/content-sidebar/versions/VersionsSidebarContainer.js(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-06-11T16:30:10.431Z
Learnt from: rafalmaksymiuk
PR: box/box-ui-elements#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/versions/VersionsSidebarContainer.js
📚 Learning: 2025-06-17T13:30:02.172Z
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.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/versions/VersionsSidebarContainer.js
🧬 Code Graph Analysis (5)
src/elements/common/routing/__tests__/withRouterIfEnabled.test.js (1)
src/elements/common/routing/withRouterIfEnabled.js (2)
WithRouterIfEnabled(9-15)withRouterIfEnabled(6-21)
src/elements/content-sidebar/AddTaskButton.js (1)
src/elements/common/routing/withRouterIfEnabled.js (1)
withRouterIfEnabled(6-21)
src/elements/content-sidebar/versions/VersionsSidebarContainer.js (1)
src/elements/common/routing/withRouterIfEnabled.js (1)
withRouterIfEnabled(6-21)
src/elements/common/routing/withRouterIfEnabled.js (1)
src/elements/common/routing/__tests__/withRouterIfEnabled.test.js (1)
WithRouterIfEnabled(23-23)
src/elements/content-sidebar/SidebarToggle.js (1)
src/elements/common/routing/withRouterIfEnabled.js (1)
withRouterIfEnabled(6-21)
🪛 Biome (2.1.2)
src/elements/common/routing/__tests__/withRouterIfEnabled.test.js
[error] 8-8: 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/AddTaskButton.js
[error] 3-3: 'import { type x ident }' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
src/elements/common/routing/withRouterIfEnabled.js
[error] 6-6: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 9-9: 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/SidebarToggle.js
[error] 8-8: 'import { type x ident }' 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). (2)
- GitHub Check: lint_test_build
- GitHub Check: Summary
🔇 Additional comments (12)
src/elements/common/routing/withRouterIfEnabled.js (3)
13-15: Heads-up: toggling routerDisabled remounts the wrapped componentSwitching between Wrapped and WrappedWithRouter changes the element type and will unmount/remount the child, losing its local state. If routerDisabled can change at runtime (feature flag flips, prop updates), this is a UX risk.
- If the flag is static per session, this is fine. Otherwise, consider a strategy that preserves instance identity or ensure the flag doesn’t change post-mount.
6-6: Flow syntax vs Biome errorsThis file is Flow-typed (// @flow). Biome’s “TypeScript-only” parse errors are false positives. Prefer configuring Biome to understand Flow or exclude Flow files from Biome’s parser.
Examples to consider:
- Configure Biome to ignore files with // @flow.
- Or scope Biome to TS/JS without Flow in this folder.
No code changes required here.
Also applies to: 9-11
6-21: Overall HOC approach looks goodDisplayName is set, feature gate is consistent with existing patterns, and props are preserved.
src/elements/common/routing/index.js (1)
2-2: Public re-export LGTMwithRouterIfEnabled is correctly re-exported from the routing index.
src/elements/content-sidebar/SidebarToggle.js (2)
56-57: Wrapper swap LGTMDefault export now uses withRouterIfEnabled, matching the new pattern. Internals already branch on routerDisabled/history, so behavior remains correct.
8-9: Flow type import is correct; Biome warning is a false positive
import { type RouterHistory } from 'react-router-dom';is valid Flow syntax. Please adjust Biome settings to handle Flow or exclude this file from Biome parsing to avoid “TypeScript-only” parse errors.src/elements/content-sidebar/AddTaskButton.js (1)
3-4: Flow vs Biome configurationThe
import { type RouterHistory }Flow syntax is valid. Please configure Biome to support Flow or exclude this file to avoid spurious parse errors.src/elements/content-sidebar/versions/VersionsSidebarContainer.js (2)
353-353: Composition order looks correctflow([withRouterIfEnabled, withAPIContext, withFeatureConsumer]) results in withFeatureConsumer being the outermost HOC, so features will be available to withRouterIfEnabled at render time. Good.
12-12: Verify route pattern compatibility with generatePath
history.push(generatePath(match.path, { ...match.params, versionId }))depends onmatch.pathdeclaring:versionId?as optional. If versionId is undefined, generatePath will omit it only when the path param is optional.Please confirm the route path uses an optional param for versionId.
src/elements/common/routing/__tests__/withRouterIfEnabled.test.js (3)
25-39: Nice coverage for default-enabled behaviorTest validates injected router props in a Router context and absence of routerDisabled. Good.
41-50: Prop-based disable test LGTMCorrectly asserts no router props and routerDisabled flag presence.
8-8: Flow types in tests vs BiomeThe
props: anytype annotation is Flow; Biome’s “TypeScript-only” parse error is a tooling issue. Please adjust Biome config or ignore Flow-typed test files.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/elements/common/routing/withRouterIfEnabled.js (1)
6-8: Biome parse errors on Flow types — adjust linter configBiome doesn’t parse Flow. Either ignore Flow-typed JS files in Biome or disable Biome lint for this path to avoid false parse errors on Flow annotations.
Suggested steps:
- Add this file (or directory) to Biome’s ignore list or disable its linter for
src/elements/common/routing/**.- Keep ESLint/Flow handling as before.
If helpful, I can draft a targeted Biome config update.
src/elements/content-sidebar/AddTaskButton.js (2)
1-5: Biome parse error on Flow imports — exclude or disable Biome for Flow JS
import { type ... }is Flow; Biome flags it as TS-only. Adjust Biome config to ignore/disable lint for Flow files.
48-60: Optional: add a test to cover router-disabled vs. history pathsA small unit test for
handleClickMenuItemcan assert:
- when
routerDisabled && internalSidebarNavigationHandler→ handler is called;- else-if
history→history.replaceis called.src/elements/common/routing/__tests__/withRouterIfEnabled.test.js (1)
1-8: Biome parse error on Flow in tests — ignore Flow files in BiomeTests use Flow (
// @flow,props: any); Biome can’t parse them. Add test paths to Biome ignore or disable Biome lint for tests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/elements/content-sidebar/__tests__/__snapshots__/ActivitySidebar.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (4)
src/elements/common/routing/__tests__/withRouterIfEnabled.test.js(1 hunks)src/elements/common/routing/withRouterIfEnabled.js(1 hunks)src/elements/content-sidebar/AddTaskButton.js(4 hunks)src/elements/content-sidebar/versions/VersionsSidebarContainer.js(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/elements/content-sidebar/versions/VersionsSidebarContainer.js
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-17T13:30:02.172Z
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.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/AddTaskButton.js
🧬 Code Graph Analysis (3)
src/elements/content-sidebar/AddTaskButton.js (1)
src/elements/common/routing/withRouterIfEnabled.js (1)
withRouterIfEnabled(6-21)
src/elements/common/routing/__tests__/withRouterIfEnabled.test.js (1)
src/elements/common/routing/withRouterIfEnabled.js (2)
WithRouterIfEnabled(8-15)withRouterIfEnabled(6-21)
src/elements/common/routing/withRouterIfEnabled.js (1)
src/elements/common/routing/__tests__/withRouterIfEnabled.test.js (2)
WithRouterIfEnabled(21-21)props(8-8)
🪛 Biome (2.1.2)
src/elements/content-sidebar/AddTaskButton.js
[error] 3-3: 'import { type x ident }' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
src/elements/common/routing/__tests__/withRouterIfEnabled.test.js
[error] 7-7: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
src/elements/common/routing/withRouterIfEnabled.js
[error] 6-6: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 8-8: Type annotations 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). (2)
- GitHub Check: lint_test_build
- GitHub Check: Summary
🔇 Additional comments (4)
src/elements/content-sidebar/AddTaskButton.js (2)
15-22: Good: history made optional to reflect conditional router injectionAligns types with runtime when the router is disabled. Matches how SidebarToggle was handled.
58-60: Good: guard navigation behind history availabilityPrevents errors when router is disabled and
historyis absent.src/elements/common/routing/__tests__/withRouterIfEnabled.test.js (2)
23-35: Good: verifies router injection when inside a RouterSolid positive-path coverage for default behavior.
37-54: Good: verifies both router-disabled paths (prop and feature flag)Covers the two opt-out mechanisms.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/elements/common/routing/withRouterIfEnabled.js (1)
8-16: Resolved: Stable HOC identity (no HOC creation in render path)Creating
WrappedWithRouteroutside the render path avoids remounts/state loss from new component identities each render. Good fix.
🧹 Nitpick comments (1)
src/elements/common/routing/withRouterIfEnabled.js (1)
11-13: Confirm feature flag key path'routerDisabled.value'Validate that
isFeatureEnabledsupports dotted paths and that this flag name is correct. If yes, consider extracting to a constant to avoid typos.Suggested tweak:
- const routerDisabled = - props?.routerDisabled === true || isFeatureEnabled(props?.features, 'routerDisabled.value'); + const routerDisabledFlag = 'routerDisabled.value'; + const routerDisabled = + props?.routerDisabled === true || isFeatureEnabled(props?.features, routerDisabledFlag);Also ensure
isFeatureEnabledsafely handlesundefinedfeatures and returnsfalserather than throwing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/elements/common/routing/withRouterIfEnabled.js(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/elements/common/routing/withRouterIfEnabled.js (1)
src/elements/common/routing/__tests__/withRouterIfEnabled.test.js (2)
WithRouterIfEnabled(21-21)props(8-8)
🪛 Biome (2.1.2)
src/elements/common/routing/withRouterIfEnabled.js
[error] 6-6: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 10-10: Type annotations 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). (2)
- GitHub Check: Summary
- GitHub Check: lint_test_build
🔇 Additional comments (1)
src/elements/common/routing/withRouterIfEnabled.js (1)
6-6: Biome parse errors: Flow type annotations flagged as “TypeScript-only”This file is Flow-typed (
// @flow), but Biome 2.1.2 doesn’t support Flow parsing. Adjust CI/lint config so Biome doesn’t parse Flow files (or this path), and rely on the Flow toolchain/ESLint for these files. Otherwise, convert to TS (out of scope for this PR).Action options:
- Exclude Flow files or this directory in Biome config (recommended).
- Keep Flow and ensure the Babel/Flow toolchain strips types before Biome runs.
- If you’re migrating to TS, convert in a follow-up PR.
Also applies to: 10-10
made sure history is treated as optional in affected components
rafalmaksymiuk
force-pushed
the
FROFRA-135
branch
from
97f3ece to
77ab5d2
Compare
src/elements/common/routing/__tests__/withRouterIfEnabled.test.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/elements/common/routing/__tests__/withRouterIfEnabled.test.js (2)
56-56: Add a regression test to ensure no remount across re-renders (stable identity).This catches the classic pitfall of creating the wrapped component inside render, which remounts every render. The current implementation defines
WrappedWithRouteroutside the render path, so this should pass; let’s lock it in.Add this test at the end of the file:
test('does not remount across re-renders with unchanged props', () => { let mountCount = 0; const Stateful = () => { React.useEffect(() => { mountCount += 1; }, []); return <div data-testid="stateful" />; }; const WrappedStateful = withRouterIfEnabled(Stateful); const { rerender } = render( <MemoryRouter> <WrappedStateful /> </MemoryRouter>, ); // Re-render with identical props; component should not remount rerender( <MemoryRouter> <WrappedStateful /> </MemoryRouter>, ); expect(mountCount).toBe(1); });
1-1: Fix Biome parse error by removing Flow/TS annotations in a .js test (or convert to TS).Biome flags the type annotation at Line 7 and won’t parse Flow. Since this is a .js test and types are non-essential here, drop the Flow directive and the inline type to unblock CI. Alternatively, migrate the file to TypeScript (.tsx) and adjust configs, but that’s heavier.
Apply this diff:
-// @flow @@ -const TestComponent = (props: any) => { +const TestComponent = props => {Also applies to: 7-7
🧹 Nitpick comments (1)
src/elements/common/routing/__tests__/withRouterIfEnabled.test.js (1)
46-54: Optional: assert thatrouterDisabledis not leaked via feature-flag path.Helps lock in the contract that the computed disable state is not forwarded as a prop.
Apply this diff:
const component = getByTestId('test-component'); expect(component).not.toHaveAttribute('data-has-history'); expect(component).not.toHaveAttribute('data-has-location'); expect(component).not.toHaveAttribute('data-has-match'); + expect(component).not.toHaveAttribute('data-router-disabled');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/elements/common/routing/__tests__/withRouterIfEnabled.test.js(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/elements/common/routing/__tests__/withRouterIfEnabled.test.js (1)
src/elements/common/routing/withRouterIfEnabled.js (2)
WithRouterIfEnabled(10-17)withRouterIfEnabled(6-23)
🪛 Biome (2.1.2)
src/elements/common/routing/__tests__/withRouterIfEnabled.test.js
[error] 7-7: Type annotations 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). (2)
- GitHub Check: lint_test_build
- GitHub Check: Summary
🔇 Additional comments (2)
src/elements/common/routing/__tests__/withRouterIfEnabled.test.js (2)
23-35: LGTM: router-enabled scenario is covered well.Verifies that history/location/match are injected when under a Router and not disabled. Clear and correct.
37-44: LGTM: prop-based disable path behaves correctly without a Router.Good coverage for the
routerDisabledprop path and absence of router props.
tjuanitas
left a comment
tjuanitas
left a comment
There was a problem hiding this comment.
try to write new files in typescript
4 participants
Created thin wrapper over withRouter that respects routerDisabled prop and is transparent wrapper if it is set to true.
Summary by CodeRabbit
New Features
Refactor
Tests