refactor(media-query): migrate MediaQuery from Flow to TypeScript - #4735
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR adds typed media-query constants, types, a ChangesMedia query support
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant useMediaQuery
participant react-responsive
participant Window
participant withMediaQuery
participant WrappedComponent
useMediaQuery->>react-responsive: Evaluate formatted media queries
react-responsive->>Window: Read viewport dimensions
Window-->>react-responsive: Return width and height
react-responsive-->>useMediaQuery: Return query matches
useMediaQuery-->>withMediaQuery: Return MediaShape
withMediaQuery-->>WrappedComponent: Pass media properties
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/media-query/types.js.flow`:
- Around line 3-6: Update the MediaPointerType and MediaHoverType aliases to use
valid Flow syntax: define them as string-literal unions matching the
POINTER_TYPE and HOVER_TYPE values, or use typeof property queries for those
constants. Keep the existing exported names and supported values unchanged.
In `@src/components/media-query/types.ts`:
- Around line 6-19: Remove the string index signatures from the MediaFeatures
and MediaShape interfaces so their contracts reject arbitrary keys. Preserve the
explicitly declared media feature properties and their existing types, ensuring
invalid feature objects cannot pass through toQuery.
In `@src/components/media-query/useMediaQuery.ts`:
- Around line 24-26: Move the window access out of the render path by
establishing a separate subscription effect that listens for window resize
events instead of calling getViewDimensions during render. Wrap the window
access in an SSR-safe guard that provides fallback dimensions when window is
unavailable, and set up an event listener on the resize event to update viewport
dimensions whenever the window size changes rather than only when media query
conditions are evaluated.
In `@src/components/media-query/withMediaQuery.tsx`:
- Around line 5-12: Update the withMediaQuery function in
src/components/media-query/withMediaQuery.tsx lines 5-12 to preserve the wrapped
component's original prop contract by changing the return type from
React.ComponentType<Record<string, unknown>> to a generic that reflects
Omit<Props, keyof MediaShape>, and filter the injected media props from the
original props before spreading them into WrappedComponent using an explicit set
of known media property keys instead of relying on MediaShape's index signature.
Apply the equivalent changes to
src/components/media-query/withMediaQuery.js.flow lines 7-23 to maintain the
Flow type contract in parallel. Add a type test that includes a required
non-media prop on the wrapped component to verify callers must provide it after
wrapping.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b36e9b1e-5c57-42a6-ad40-927b21fbc7ee
📒 Files selected for processing (14)
src/components/index.tssrc/components/media-query/__tests__/useMediaQuery.test.tsxsrc/components/media-query/__tests__/withMediaQuery.test.tsxsrc/components/media-query/constants.js.flowsrc/components/media-query/constants.tssrc/components/media-query/index.js.flowsrc/components/media-query/index.tssrc/components/media-query/stories/MediaQuery.stories.tsxsrc/components/media-query/types.js.flowsrc/components/media-query/types.tssrc/components/media-query/useMediaQuery.js.flowsrc/components/media-query/useMediaQuery.tssrc/components/media-query/withMediaQuery.js.flowsrc/components/media-query/withMediaQuery.tsx
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/media-query/types.js.flow`:
- Around line 3-6: Update the MediaPointerType and MediaHoverType aliases to use
valid Flow syntax: define them as string-literal unions matching the
POINTER_TYPE and HOVER_TYPE values, or use typeof property queries for those
constants. Keep the existing exported names and supported values unchanged.
In `@src/components/media-query/types.ts`:
- Around line 6-19: Remove the string index signatures from the MediaFeatures
and MediaShape interfaces so their contracts reject arbitrary keys. Preserve the
explicitly declared media feature properties and their existing types, ensuring
invalid feature objects cannot pass through toQuery.
In `@src/components/media-query/useMediaQuery.ts`:
- Around line 24-26: Move the window access out of the render path by
establishing a separate subscription effect that listens for window resize
events instead of calling getViewDimensions during render. Wrap the window
access in an SSR-safe guard that provides fallback dimensions when window is
unavailable, and set up an event listener on the resize event to update viewport
dimensions whenever the window size changes rather than only when media query
conditions are evaluated.
In `@src/components/media-query/withMediaQuery.tsx`:
- Around line 5-12: Update the withMediaQuery function in
src/components/media-query/withMediaQuery.tsx lines 5-12 to preserve the wrapped
component's original prop contract by changing the return type from
React.ComponentType<Record<string, unknown>> to a generic that reflects
Omit<Props, keyof MediaShape>, and filter the injected media props from the
original props before spreading them into WrappedComponent using an explicit set
of known media property keys instead of relying on MediaShape's index signature.
Apply the equivalent changes to
src/components/media-query/withMediaQuery.js.flow lines 7-23 to maintain the
Flow type contract in parallel. Add a type test that includes a required
non-media prop on the wrapped component to verify callers must provide it after
wrapping.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b36e9b1e-5c57-42a6-ad40-927b21fbc7ee
📒 Files selected for processing (14)
src/components/index.tssrc/components/media-query/__tests__/useMediaQuery.test.tsxsrc/components/media-query/__tests__/withMediaQuery.test.tsxsrc/components/media-query/constants.js.flowsrc/components/media-query/constants.tssrc/components/media-query/index.js.flowsrc/components/media-query/index.tssrc/components/media-query/stories/MediaQuery.stories.tsxsrc/components/media-query/types.js.flowsrc/components/media-query/types.tssrc/components/media-query/useMediaQuery.js.flowsrc/components/media-query/useMediaQuery.tssrc/components/media-query/withMediaQuery.js.flowsrc/components/media-query/withMediaQuery.tsx
🛑 Comments failed to post (1)
src/components/media-query/types.js.flow (1)
3-6: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail rg -n -C 2 \ 'MediaPointerType|MediaHoverType|POINTER_TYPE\.none|HOVER_TYPE\.none|typeof .*\[' \ src/components/media-query --glob '*.js.flow' fd -a '^package\.json$' . -E node_modules -x jq -r \ '.scripts // {} | to_entries[] | select(.key | test("flow"; "i")) | "\(.key): \(.value)"' {}Repository: box/box-ui-elements
Length of output: 3608
🏁 Script executed:
#!/bin/bash set -euo pipefail printf 'Files:\n' git ls-files 'src/components/media-query/*' printf '\nRelevant source:\n' for f in src/components/media-query/types.js.flow src/components/media-query/*.js.flow package.json; do if [ -f "$f" ]; then echo "--- $f" sed -n '1,80p' "$f" fi done printf '\nFind flow/config references:\n' rg -n '"flow"|"flow-bin"|flow' package.json .flowconfig jsconfig.json .vimrc 2>/dev/null || true printf '\nCheck for external flow executable/version hints:\n' (command -v flow && flow --version 2>/dev/null) || true npm list flow-bin 2>/dev/null || trueRepository: box/box-ui-elements
Length of output: 13025
🌐 Web query:
Flow type syntax type alias object literal property union imported const property string literal union💡 Result:
In Flow, you can combine type aliases, object literals, union types, and property types to model complex data structures, such as disjoint unions, which are often used for state management or event handling [1][2][3]. Type Alias and Object Literal A type alias provides a reusable name for a type [4]. You can define an object literal type using the syntax { key: Type } [5][6]. For example: type User = { id: number, name: string }; String Literal Unions Literal types restrict a value to specific constants [7]. Combining them with unions allows you to define a finite set of allowed values, such as specific string tags [7][8]. type ActionType = 'LOGIN' | 'LOGOUT'; Disjoint Object Unions A disjoint object union is a union of object types that share a common "discriminant" property with distinct literal values [1]. This allows Flow to narrow (refine) the type based on the value of that property [1][2]. type UserAction = | { type: 'LOGIN', userId: number } | { type: 'LOGOUT', userId: number }; Using Imported Const Properties To derive a union of string literals from a collection of constants (e.g., imported from another module), you can use the typeof operator [4][9]. If you have a constant object, you can extract the union of its property types [10]. // config.js export const ACTIONS = { LOGIN: 'LOGIN', LOGOUT: 'LOGOUT' }; // types.js import { ACTIONS } from './config'; type ActionValues = $Values; // 'LOGIN' | 'LOGOUT' Key Syntax Considerations 1. Exact Objects: By default, Flow object types are exact (they reject extra properties) [5][6]. To allow extra properties, use the inexact syntax: { property: string,... } [5][6]. 2. Const Expressions: Use as const to preserve narrow literal types in objects or arrays, preventing them from widening to their base primitive types [7][9]. 3. Refinements: When using disjoint unions, ensure the discriminant property has a concrete literal type so Flow can successfully refine the type within control flow statements like if or switch [1][2].
Citations:
- 1: https://flow.org/en/docs/types/unions/
- 2: https://flow.org/en/docs/lang/refinements/
- 3: https://stackoverflow.com/questions/50872258/flow-type-refinement-by-checking-for-existing-properties
- 4: https://flow.org/en/docs/types/aliases/
- 5: https://flow.org/en/docs/types/objects/
- 6: https://github.com/facebook/flow/blob/main/website/docs/types/objects.md
- 7: https://flow.org/en/docs/types/literals/
- 8: https://flow.org/en/docs/types/primitives/
- 9: https://flow.org/en/docs/types/const-expression/
- 10: facebook/flow#7547
Use a valid Flow type alias.
POINTER_TYPE.noneandHOVER_TYPE.noneare value expressions, so the aliases on lines 5-6 are not valid Flow type syntax. TypeMediaPointerType/MediaHoverTypeas string-literal unions or use atypeofproperty query in the alias.Proposed fix
-import { POINTER_TYPE, HOVER_TYPE } from './constants'; - -export type MediaPointerType = POINTER_TYPE.none | POINTER_TYPE.coarse | POINTER_TYPE.fine; -export type MediaHoverType = HOVER_TYPE.none | HOVER_TYPE.hover; +export type MediaPointerType = 'none' | 'coarse' | 'fine'; +export type MediaHoverType = 'none' | 'hover';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.export type MediaPointerType = 'none' | 'coarse' | 'fine'; export type MediaHoverType = 'none' | 'hover';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/media-query/types.js.flow` around lines 3 - 6, Update the MediaPointerType and MediaHoverType aliases to use valid Flow syntax: define them as string-literal unions matching the POINTER_TYPE and HOVER_TYPE values, or use typeof property queries for those constants. Keep the existing exported names and supported values unchanged.
87a16fb to
e85e6f9
Compare
e85e6f9 to
25092e3
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
25092e3 to
e8290ec
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
e8290ec to
4879bfa
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
4879bfa to
93a1208
Compare
93a1208 to
f62724d
Compare
Merge Queue Status
This pull request spent 21 seconds in the queue, including 3 seconds running CI. Required conditions to merge
|
3 participants
Convert
MediaQueryutilities to TypeScriptThis PR converts
src/components/media-queryfrom JavaScript with Flow to TypeScript.Changes
.js.flowfiles for backward compatibilityindex.tssrc/components/index.tsTesting
src/components/media-query; all 3 passyarn lint:ts, ESLint, and Flow checks passSummary by CodeRabbit
New Features
Tests