feat(hotkeys): Remove legacy context from Hotkeys - #4382
Conversation
WalkthroughAdds a HotkeyContext and migrates hotkey components and tests from the legacy React context API to the modern Context API (introduces Changes
Sequence Diagram(s)sequenceDiagram
participant HotkeyLayer as HotkeyLayer (Provider)
participant Provider as HotkeyContext.Provider
participant Hotkeys as Hotkeys (consumer)
participant HelpModal as HotkeyHelpModal (consumer)
HotkeyLayer->>Provider: render with value = hotkeyService
Note right of Provider: Provides hotkeyLayer API\n(registerHotkey, deregisterHotkey, showHelp)
Hotkeys->>Provider: read this.context on mount
Hotkeys->>Provider: call registerHotkey(hotkeyConfig)
HelpModal->>Provider: read this.context when opened
alt Modal opens
HelpModal->>Provider: read types / hotkeys
end
Hotkeys->>Provider: on unmount call deregisterHotkey(hotkeyConfig)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/components/hotkeys/HotkeyContext.js (1)
4-10: Clarify or tightenHotkeyContextPropTypesto match the actual context value
HotkeyContext’s value is the hotkey layer/service instance itself, whereasHotkeyContextPropTypesexposes ahotkeyLayer: PropTypes.objectfield. This is a bit confusing with the new contextType-based consumers, which now usethis.contextdirectly.Consider either:
- Updating the shape to more accurately describe the value (e.g., something like
PropTypes.shape({ registerHotkey: PropTypes.func, deregisterHotkey: PropTypes.func, ... })), or- Removing this helper entirely if nothing still relies on legacy
contextTypes.This will keep the context contract clearer for future consumers.
src/components/hotkeys/__tests__/testHelpers.js (1)
7-23: You can passthis.setStatedirectly instead of bindingIn
createContextTestWrapper, you don’t need to bindsetState:- <HotkeyContext.Provider value={contextValue}> - {renderChild(this.state, this.setState.bind(this))} - </HotkeyContext.Provider> + <HotkeyContext.Provider value={contextValue}> + {renderChild(this.state, this.setState)} + </HotkeyContext.Provider>
this.setStateis already bound to the component instance in React class components, so the explicit.bind(this)is redundant. This slightly reduces noise in a helper used across tests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/components/hotkeys/HotkeyContext.js(1 hunks)src/components/hotkeys/HotkeyHelpModal.js(3 hunks)src/components/hotkeys/HotkeyLayer.js(2 hunks)src/components/hotkeys/Hotkeys.js(4 hunks)src/components/hotkeys/__tests__/HotkeyHelpModal.test.js(6 hunks)src/components/hotkeys/__tests__/HotkeyLayer.test.js(2 hunks)src/components/hotkeys/__tests__/Hotkeys.test.js(4 hunks)src/components/hotkeys/__tests__/testHelpers.js(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-15T17:04:28.279Z
Learnt from: bfoxx1906
Repo: box/box-ui-elements PR: 4275
File: src/elements/content-sidebar/activity-feed/comment/__tests__/Comment.test.js:259-260
Timestamp: 2025-09-15T17:04:28.279Z
Learning: When wrapping enzyme mount() tests with IntlProvider for class-based components, use wrapper.find('ComponentName') to access the actual component instance and state, since the wrapper becomes the provider instead of the component.
Applied to files:
src/components/hotkeys/__tests__/Hotkeys.test.jssrc/components/hotkeys/__tests__/testHelpers.jssrc/components/hotkeys/__tests__/HotkeyLayer.test.jssrc/components/hotkeys/__tests__/HotkeyHelpModal.test.js
🧬 Code graph analysis (6)
src/components/hotkeys/__tests__/Hotkeys.test.js (2)
src/components/hotkeys/HotkeyContext.js (2)
HotkeyContext(4-4)HotkeyContext(4-4)src/components/hotkeys/__tests__/testHelpers.js (2)
createContextTestWrapper(7-23)createContextTestWrapper(7-23)
src/components/hotkeys/__tests__/testHelpers.js (1)
src/components/hotkeys/HotkeyContext.js (2)
HotkeyContext(4-4)HotkeyContext(4-4)
src/components/hotkeys/HotkeyHelpModal.js (1)
src/components/hotkeys/HotkeyContext.js (2)
HotkeyContext(4-4)HotkeyContext(4-4)
src/components/hotkeys/Hotkeys.js (1)
src/components/hotkeys/HotkeyContext.js (2)
HotkeyContext(4-4)HotkeyContext(4-4)
src/components/hotkeys/HotkeyLayer.js (1)
src/components/hotkeys/HotkeyContext.js (2)
HotkeyContext(4-4)HotkeyContext(4-4)
src/components/hotkeys/__tests__/HotkeyHelpModal.test.js (2)
src/components/hotkeys/HotkeyContext.js (2)
HotkeyContext(4-4)HotkeyContext(4-4)src/components/hotkeys/__tests__/testHelpers.js (2)
createContextTestWrapper(7-23)createContextTestWrapper(7-23)
⏰ 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 (6)
src/components/hotkeys/Hotkeys.js (1)
5-6: Context-basedHotkeyswiring and lifecycle are correctly configuredThe migration to
HotkeyContextviaHotkeys.contextType = HotkeyContextis sound. Per React's Context API,this.contextis guaranteed to contain the value from the nearest<HotkeyContext.Provider>in all lifecycle methods includingcomponentDidMount,componentDidUpdate, andcomponentWillUnmount.
componentDidMountfails fast with a clear error if used outside a provider, which matches the expected contract._addHotkeysand_removeHotkeysdirectly access the context value (the hotkey layer service), consistent withHotkeyLayer's provider setup.- The null check in
_removeHotkeysprovides defensive safety for edge cases, though context should be available per React's guarantees.src/components/hotkeys/HotkeyLayer.js (1)
6-7: React Context API pattern withstatic contextTypeis correct in principleThe pattern described—wrapping
Hotkeysin<HotkeyContext.Provider value={this.hotkeyService}>and consuming viastatic contextType = HotkeyContextto accessthis.context—aligns with React's documented behavior for class components. The Provider correctly exposes the service instance to all child components using that context.However, verification of the actual implementation could not be completed due to repository access limitations. Recommend manual confirmation that:
HotkeyContextis the context object itself (notHotkeyContext.Provider)- The
contextTypeassignment targets the correct context objectHotkeyHelpModalis positioned as a child of the Provider- Unmount ordering still preserves the deregistration sequence
src/components/hotkeys/HotkeyHelpModal.js (1)
10-14: FixHotkeyHelpModalconstructor to properly access context in initial state setupThe review comment's technical recommendation is sound: when using
static contextType = MyContext, the context value is passed as the second constructor parameter, and callingsuper(props, context)ensuresthis.contextis available within the constructor body.However, the explanation contains an inaccuracy. According to React documentation, when you properly pass context to
super(props, context), React does assignthis.contextduring the super call, making it available for the remainder of the constructor. The constructor signature should beconstructor(props, context)withsuper(props, context)to ensure context is accessible in initial state setup.The core issues remain valid:
- If the constructor currently reads
this.contextwithout the context parameter, it will beundefined- If
isOpen=trueon initial mount with context undefined, hotkeys/types won't load until an update triggerscomponentDidUpdate- The test helper needing manual reassignment suggests the current implementation doesn't properly initialize from context
The recommended fix (using
constructor(props, context)andsuper(props, context)) is the correct approach to ensure context is available during initial state initialization. This preserves the previous behavior regardless ofisOpenstate on first mount.src/components/hotkeys/__tests__/HotkeyLayer.test.js (1)
22-33: The suggested selector pattern won't work reliably with shallow renderingWhile preferring component types over string selectors is generally sound advice,
wrapper.find(HotkeyContext.Provider)does not work reliably with Enzyme'sshallow()rendering. The shallow renderer doesn't fully traverse Context providers from the modern Context API.The current test approach—asserting
wrapper.instance().hotkeyServicedirectly—is already valid and doesn't depend on fragile selectors. If you want to test the Provider presence itself, consider switching tomount()instead ofshallow(), or use React Testing Library, which handles context naturally.src/components/hotkeys/__tests__/Hotkeys.test.js (1)
21-37: Hotkeys tests properly exercise the new context-based implementationThe tests do a good job of covering
HotkeyContextusage:
componentDidMounttests mountHotkeysinside<HotkeyContext.Provider>and assertregisterHotkeycalls; error handling verifies behavior when no provider exists.- The
componentDidUpdatetest usescreateContextTestWrapperto manage state, wraps the setState call inact(), and callswrapper.update()to confirm that removed configs trigger matchingderegisterHotkeycalls.- The explicit
wrapper.unmount()while the provider is mounted ensurescomponentWillUnmountreceives a non-null context, complementing the defensive null-check in the component.- Render tests verify both children rendering and the null render path when no children are present.
These tests align well with the context-based implementation in
Hotkeys.js.Also applies to: 64-78, 79-94, 123-143, 148-176
src/components/hotkeys/__tests__/HotkeyHelpModal.test.js (1)
16-38: RefactorgetWrapperto usemount()withcreateContextTestWrapperpattern instead ofshallow()+dive()The manual context assignment in
getWrapper(lines 16-38) works around an Enzyme limitation:shallow()+dive()doesn't automatically bind React's modern Context API (static contextType) to the component. This forces the workaround of manually settinginstance.context, reinitializinghotkeys/types, and patching state.The same test file already demonstrates the correct pattern at lines 51-57 with
createTestWrapperusingmount(). This approach—mounting the component within a Provider—automatically binds context without manual intervention.Refactor
getWrapperto follow thecreateTestWrapperpattern, which will:
- Eliminate manual context assignment
- Remove the need to reinitialize instance properties
- Make the test setup clearer and less coupled to implementation details
The
componentDidUpdate()tests (lines 86-130) already use this pattern successfully and can serve as a reference.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/components/hotkeys/HotkeyHelpModal.js(3 hunks)src/components/hotkeys/Hotkeys.stories.js(1 hunks)src/components/hotkeys/__tests__/HotkeyHelpModal.test.js(5 hunks)src/components/hotkeys/__tests__/HotkeyTestWrapper.js(1 hunks)src/components/hotkeys/__tests__/Hotkeys.test.js(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-15T17:04:28.279Z
Learnt from: bfoxx1906
Repo: box/box-ui-elements PR: 4275
File: src/elements/content-sidebar/activity-feed/comment/__tests__/Comment.test.js:259-260
Timestamp: 2025-09-15T17:04:28.279Z
Learning: When wrapping enzyme mount() tests with IntlProvider for class-based components, use wrapper.find('ComponentName') to access the actual component instance and state, since the wrapper becomes the provider instead of the component.
Applied to files:
src/components/hotkeys/__tests__/HotkeyHelpModal.test.jssrc/components/hotkeys/__tests__/HotkeyTestWrapper.jssrc/components/hotkeys/__tests__/Hotkeys.test.js
🧬 Code graph analysis (3)
src/components/hotkeys/HotkeyHelpModal.js (1)
src/components/hotkeys/HotkeyContext.js (2)
HotkeyContext(4-4)HotkeyContext(4-4)
src/components/hotkeys/__tests__/HotkeyHelpModal.test.js (2)
src/components/hotkeys/HotkeyContext.js (2)
HotkeyContext(4-4)HotkeyContext(4-4)src/components/hotkeys/__tests__/HotkeyTestWrapper.js (1)
HotkeyTestWrapper(8-22)
src/components/hotkeys/__tests__/Hotkeys.test.js (2)
src/components/hotkeys/HotkeyContext.js (2)
HotkeyContext(4-4)HotkeyContext(4-4)src/components/hotkeys/__tests__/HotkeyTestWrapper.js (1)
HotkeyTestWrapper(8-22)
⏰ 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 (10)
src/components/hotkeys/__tests__/HotkeyTestWrapper.js (1)
1-22: LGTM! Clean test utility for context-driven testing.This wrapper component provides a good pattern for testing components that consume context and need to verify
componentDidUpdatebehavior. Based on learnings, this approach correctly allows accessing component instances viawrapper.find('ComponentName')when the wrapper is a provider.src/components/hotkeys/__tests__/HotkeyHelpModal.test.js (2)
16-28: Good use ofact()with provider-wrapped mounting.The pattern correctly wraps
mountinact()and useswrapper.update()to ensure React has processed all updates before querying the component. Returning bothwrapperandhotkeyHelpModalfollows the learning about accessing component instances when wrapped in a provider.
111-127: Relative call count assertions address previous feedback.The test now compares against initial call counts rather than expecting exact values, which is more robust when context timing may vary. This addresses the past review concern about call count assertions.
src/components/hotkeys/__tests__/Hotkeys.test.js (3)
20-37: LGTM! Clean migration to context-based testing.The test correctly uses
HotkeyContext.Providerto supply the mock hotkey layer, andsandbox.mock().thrice()properly expects exactly 3registerHotkeycalls for the 3 configs.
57-89: Good use of HotkeyTestWrapper for natural state updates.Using
HotkeyTestWrapperto manage state and triggercomponentDidUpdatenaturally (viasetState) is cleaner than manually calling lifecycle methods. Theact()wrapper ensures React processes the update synchronously.
116-138: LGTM! Unmount test correctly verifies deregistration.The test mounts with context, then unmounts and verifies that
deregisterHotkeyis called for each config via the mock expectation.src/components/hotkeys/HotkeyHelpModal.js (3)
39-58: Clean migration from constructor context to componentDidMount.Moving context-dependent initialization to
componentDidMountis the correct approach with the modern Context API, asthis.contextisn't reliably available during construction. The guard against missing context prevents runtime errors.
60-79: Defensive early return prevents errors when context is unavailable.The guard
if (!isOpen || !hotkeyLayer)correctly handles cases where the component renders before context is available or when the modal is closed.
201-202: Correct modern Context API usage.Assigning
HotkeyHelpModal.contextType = HotkeyContextis the standard pattern for class components consuming context. This replaces the legacystatic contextTypesand enablesthis.contextaccess.src/components/hotkeys/Hotkeys.stories.js (1)
1-299: Excellent comprehensive Storybook documentation.This story effectively demonstrates all hotkey features including:
- Basic single-key and combination hotkeys
- Hidden hotkeys (no type)
- Multiple key bindings for the same action
- Nested HotkeyLayer for modal context with override behavior
- Help modal integration
This provides valuable documentation for developers using the hotkey system.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/components/hotkeys/__tests__/HotkeyHelpModal.test.js (1)
2-2: Inconsistentactimport location.This file imports
actfrom'react'whileHotkeys.test.jsimports it from'react-dom/test-utils'. Thereactexport is available in React 18+, but for consistency across the test suite and backward compatibility, consider using the same import source.-import { act } from 'react'; +import { act } from 'react-dom/test-utils';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/hotkeys/__tests__/HotkeyHelpModal.test.js(4 hunks)src/components/hotkeys/__tests__/Hotkeys.test.js(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-15T17:04:28.279Z
Learnt from: bfoxx1906
Repo: box/box-ui-elements PR: 4275
File: src/elements/content-sidebar/activity-feed/comment/__tests__/Comment.test.js:259-260
Timestamp: 2025-09-15T17:04:28.279Z
Learning: When wrapping enzyme mount() tests with IntlProvider for class-based components, use wrapper.find('ComponentName') to access the actual component instance and state, since the wrapper becomes the provider instead of the component.
Applied to files:
src/components/hotkeys/__tests__/HotkeyHelpModal.test.jssrc/components/hotkeys/__tests__/Hotkeys.test.js
🧬 Code graph analysis (2)
src/components/hotkeys/__tests__/HotkeyHelpModal.test.js (2)
src/components/hotkeys/HotkeyContext.js (2)
HotkeyContext(4-4)HotkeyContext(4-4)src/components/hotkeys/__tests__/HotkeyTestWrapper.js (1)
HotkeyTestWrapper(8-22)
src/components/hotkeys/__tests__/Hotkeys.test.js (2)
src/components/hotkeys/HotkeyContext.js (2)
HotkeyContext(4-4)HotkeyContext(4-4)src/components/hotkeys/__tests__/HotkeyTestWrapper.js (1)
HotkeyTestWrapper(8-22)
⏰ 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 (12)
src/components/hotkeys/__tests__/Hotkeys.test.js (6)
1-10: Imports look correct for the context migration.The imports properly bring in
actfromreact-dom/test-utils,HotkeyContextfor providing context, andHotkeyTestWrapperfor state-driven tests. The setup aligns with the migration from legacy context to modern Context API.
19-54: LGTM!The
componentDidMounttests correctly verify:
- Registration of 3 hotkeys with a mock expecting exactly 3 calls (
thrice())- Error thrown when mounted without
HotkeyContext.ProviderThe migration to mount with context provider is appropriate.
57-86: LGTM!The test correctly uses
HotkeyTestWrapperto manage state changes and verify thatderegisterHotkeyis called twice when removing 2 of 3 configs. The use ofact()for state updates follows React testing best practices.
88-110: Shallow rendering retained for isolated error testing.Using
shallowwithdisableLifecycleMethodsis appropriate here to testcomponentDidUpdateerror handling in isolation without triggeringcomponentDidMount. This is a valid pattern for testing specific lifecycle behavior.
113-135: LGTM!The unmount test correctly verifies that all 3 hotkeys are deregistered when the component unmounts. The mock expectation (
thrice()) aligns with the 3 configs provided.
138-168: LGTM!The render tests correctly verify:
- Children are rendered when provided
- No children are rendered when none are passed
Using
wrapper.find('Hotkeys').children().lengthis an appropriate way to verify null rendering after context migration. Based on learnings, usingwrapper.find('ComponentName')to access the component when wrapped with a provider is the correct approach.src/components/hotkeys/__tests__/HotkeyHelpModal.test.js (6)
16-28: LGTM!The
getWrapperhelper correctly wraps mounting inact(), callswrapper.update()to sync enzyme's state, and extracts the innerHotkeyHelpModalviafind('HotkeyHelpModal'). This pattern aligns with best practices for testing wrapped components. Based on learnings, accessing the actual component viawrapper.find('ComponentName')is the correct approach when wrapped with a provider.
41-68: LGTM!The render tests properly verify modal rendering,
isOpenprop propagation, and null rendering when no hotkeys exist. The assertions are appropriate for the context-wrapped testing approach.
70-115: LGTM!The
componentDidUpdatetests correctly verify:
currentTypeis set when the modal opens with nullcurrentType- Service methods are called both on mount and when
isOpenchanges (2 calls each)The use of
HotkeyTestWrapperfor state management andact()for updates follows proper testing patterns.
117-142: LGTM!The test correctly verifies that
DropdownMenurenders with the expected types. Accessinginstance.typesto verify internal state is appropriate for this unit test.
144-201: LGTM!The test correctly verifies hotkey counts per type:
- 2 navigation hotkeys → expects 2
.hotkey-itemelements- 3 other hotkeys → expects 3
.hotkey-itemelementsThe previous review comment about assertion mismatch has been addressed.
204-244: LGTM!The
renderHotkeytests correctly verify:
- Single combo key
shift+a+b+crenders 4<kbd>elements- Multiple hotkeys
['shift+a', 'alt+a']render 3 children (2 keys + separator) with 4 total<kbd>elementsThe assertions accurately reflect the expected DOM structure.
66d9b96 to
c00471d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/components/hotkeys/__tests__/Hotkeys.test.js (2)
57-86: Add cleanup to prevent test isolation issues.The test correctly uses
act()for state updates and properly verifies deregistration behavior. However, the mounted wrapper should be unmounted after assertions to ensure proper cleanup.Apply this diff to add cleanup:
act(() => { wrapper.find('HotkeyTestWrapper').setState({ configs: [configs[1]] }); }); wrapper.update(); + wrapper.unmount(); });
88-110: Consider migrating to mount-based error testing for consistency.This test uses shallow rendering with manual
componentDidUpdateinvocation to test the error path. However, with the newcontextTypeusage, the behavior ofthis.contextin a shallow render might not accurately reflect production behavior. For consistency with thecomponentDidMounterror test (lines 40-54) and the rest of the migration, consider usingmountwithout a context provider.Apply this diff to align with the mount-based testing pattern:
- test('should throw error when hotkey layer does not exist', () => { - const wrapper = shallow( - <Hotkeys - configs={[ - new HotkeyRecord({ key: 'a' }), - new HotkeyRecord({ key: 'b' }), - new HotkeyRecord({ key: 'c' }), - ]} - > - <div /> - </Hotkeys>, - { - disableLifecycleMethods: true, - }, - ); - - // componentDidUpdate would throw when trying to add hotkeys if context is null - expect(() => { - wrapper.instance().componentDidUpdate({ - configs: [new HotkeyRecord({ key: 'a' })], - }); - }).toThrow(); + test('should throw error when hotkey layer does not exist', () => { + const wrapper = mount( + <Hotkeys configs={[new HotkeyRecord({ key: 'a' })]} />, + { disableLifecycleMethods: true }, + ); + + expect(() => { + wrapper.setProps({ + configs: [ + new HotkeyRecord({ key: 'a' }), + new HotkeyRecord({ key: 'b' }), + ], + }); + }).toThrow(); });src/components/hotkeys/__tests__/HotkeyHelpModal.test.js (1)
95-102: Missingwrapper.update()afteract()for consistency.Other tests in this file (lines 177, 185) call
wrapper.update()afteract()to synchronize the enzyme wrapper with React's state. While this test checks mock call counts rather than DOM, addingwrapper.update()would maintain consistency and ensure the wrapper is properly synchronized.act(() => { wrapper.find('HotkeyTestWrapper').setState({ isOpen: true }); }); + wrapper.update(); // One call for componentDidMount, one call for componentDidUpdate expect(HotkeyServiceMock.getActiveHotkeys.callCount).toBe(2);src/components/hotkeys/Hotkeys.stories.js (1)
289-299: Consider exporting the story component in the default config.Line 291 exports
HotkeyLayeras the component, but the actual story renders theHotkeyscomponent (defined at line 10). For Storybook documentation clarity, consider whethercomponent: Hotkeyswould be more accurate, unless the intent is to document theHotkeyLayerAPI withHotkeysas a usage example.If you want to align the export with the story:
export default { title: 'Components/Hotkeys', - component: HotkeyLayer, + component: Hotkeys, parameters: {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/hotkeys/Hotkeys.stories.js(1 hunks)src/components/hotkeys/__tests__/HotkeyHelpModal.test.js(4 hunks)src/components/hotkeys/__tests__/Hotkeys.test.js(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-15T17:04:28.279Z
Learnt from: bfoxx1906
Repo: box/box-ui-elements PR: 4275
File: src/elements/content-sidebar/activity-feed/comment/__tests__/Comment.test.js:259-260
Timestamp: 2025-09-15T17:04:28.279Z
Learning: When wrapping enzyme mount() tests with IntlProvider for class-based components, use wrapper.find('ComponentName') to access the actual component instance and state, since the wrapper becomes the provider instead of the component.
Applied to files:
src/components/hotkeys/__tests__/Hotkeys.test.jssrc/components/hotkeys/__tests__/HotkeyHelpModal.test.js
🧬 Code graph analysis (1)
src/components/hotkeys/__tests__/HotkeyHelpModal.test.js (2)
src/components/hotkeys/HotkeyContext.js (2)
HotkeyContext(4-4)HotkeyContext(4-4)src/components/hotkeys/__tests__/HotkeyTestWrapper.js (1)
HotkeyTestWrapper(8-22)
⏰ 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 (14)
src/components/hotkeys/__tests__/Hotkeys.test.js (4)
1-10: LGTM! Imports properly support the context migration.The addition of
act,HotkeyContext, andHotkeyTestWrapperaligns with the migration from legacy context to modern Context API. The retention ofshallowis appropriate for the error-handling test at line 89.
19-55: LGTM! componentDidMount tests properly validate context usage.Both tests correctly migrate to mount-based rendering. The first test wraps the component in
HotkeyContext.Providerwith a mock layer, and the second test properly validates the error path when no context is provided.
113-136: LGTM! componentWillUnmount test properly validates cleanup.The test correctly uses mount with
HotkeyContext.Providerand properly callsunmount()to verify that hotkeys are deregistered during cleanup.
138-168: LGTM! Render tests properly validate component output.Both tests correctly use mount with
HotkeyContext.Providerto test rendering behavior. The assertions appropriately verify that children are rendered when provided and that the component renders nothing when no children are present.src/components/hotkeys/__tests__/HotkeyHelpModal.test.js (6)
1-9: LGTM on import changes.The new imports properly support the migration to the modern Context API. Using
mountinstead of shallow rendering is correct when testing components wrapped in context providers, andactis appropriate for React 18 state update handling. Based on learnings, usingwrapper.find('ComponentName')to access the actual component instance is the correct pattern here.
16-28: Well-structured test helper.The
getWrapperfunction correctly wraps the mount inact(), synchronizes the enzyme wrapper withwrapper.update(), and returns both the outer wrapper and inner component reference. This pattern aligns with the retrieved learnings for testing class-based components wrapped with providers.
42-67: Tests correctly updated for context-based rendering.The render tests properly leverage the new
getWrapperhelper and context pattern. The assertion at line 66 validly checks for null rendering—when a component returns null,children().get(0)returns undefined which is falsy.
105-129: LGTM on dropdown menu test.The test properly creates a custom mock context and verifies both the internal
typesproperty and the DOM rendering of the dropdown components.
132-189: LGTM on hotkey list rendering test.The test correctly uses the
HotkeyTestWrapperpattern for state management and properly wraps state updates inact()followed bywrapper.update(). The assertions at lines 180 and 188 correctly match the mock data (2 navigation hotkeys, 3 other hotkeys).
192-231: LGTM on hotkey rendering tests.Both tests properly use the
getWrapperhelper and assert on thehotkeyHelpModalreference to verify correct rendering of hotkey elements and kbd components.src/components/hotkeys/Hotkeys.stories.js (4)
10-16: LGTM!Clean state management with well-named variables for the demonstration.
142-167: LGTM!The modal hotkeys correctly demonstrate layer-based overriding with the 'x' and 's' keys defined in both base and modal layers to show context-aware behavior.
170-252: Confirm duplicate helpModalShortcut is intentional.Both the base
HotkeyLayer(line 170) and the nested modalHotkeyLayer(line 252) use the samehelpModalShortcut="?". Based on the UI text at line 271 ("Press ? - see only modal hotkeys in help"), this appears intentional to show context-aware help. Verify that pressing "?" in the modal correctly shows only modal-layer hotkeys and doesn't conflict with the base layer's help modal.
1-8: No changes needed. HotkeyContext is correctly provided by HotkeyLayer, which wraps the story's JSX. The story correctly uses HotkeyLayer as its root component, and HotkeyLayer internally provides the HotkeyContext.Provider (as seen in HotkeyLayer.js lines 75-86). This pattern is consistent with the codebase architecture and requires no explicit provider wrapper in the story.
c00471d to
d731a2d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/components/hotkeys/Hotkeys.js (1)
18-21: Hotkey registration logic is sound; consider small consistency cleanupThe lifecycle/context flow is coherent: you validate
this.contextincomponentDidMount, then use it in_addHotkeys, and_removeHotkeysis defensively guarded withif (this.context).If you want to tighten things up further (optional):
- Either mirror the guard in
_addHotkeys(if (this.context) { ... }) for symmetry, or- Reuse the
hotkeyLayeralias fromcomponentDidMount(or a local alias in the helpers) instead of hittingthis.contextinline, which makes it more explicit what you expect the context value to be.Behavior is fine as-is; this is just for consistency/readability.
Also applies to: 44-52
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/hotkeys/Hotkeys.js(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/components/hotkeys/Hotkeys.js (1)
src/components/hotkeys/HotkeyContext.js (2)
HotkeyContext(4-4)HotkeyContext(4-4)
⏰ 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/components/hotkeys/Hotkeys.js (1)
5-5: Context wiring looks correct; confirmHotkeyContextexport shape matches importThe shift to
Hotkeys.contextType = HotkeyContextwith a named import is the right pattern for replacing legacy context usage in this class. Just make suresrc/components/hotkeys/HotkeyContext.jsactually exportsHotkeyContextas a named export (e.g.,export const HotkeyContext = React.createContext(null);). If it’s a default export instead, this import will resolve toundefinedandthis.contextwill never be populated.Also applies to: 62-62
Merge Queue Status✅ The pull request has been merged This pull request spent 11 minutes 30 seconds in the queue, including 11 minutes 20 seconds running CI. Required conditions to merge
|
Removes legacy context from HotkeyLayer and replaces it with HotkeyContext. This is required to support React 19 in the future.
Summary by CodeRabbit
New Features
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.