fix(Popup): placement type#2700
Open
xexys wants to merge 1 commit into
Open
Conversation
Contributor
Reviewer's GuideAligns Popup and Popover placement typing and docs with readonly placement arrays, and introduces a shared runtime guard for readonly arrays used in Popup placement handling. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The new
isReadonlyArraywrapper is currently just an alias forArray.isArray; consider either inliningArray.isArraywhere used or expanding this helper’s responsibilities (e.g., colocating with other shared type guards or documenting its intended use) so the indirection is justified. - You can make
isReadonlyArraygeneric (e.g.,export function isReadonlyArray<T>(value: readonly T[] | unknown): value is readonly T[]) so callers get proper element type narrowing instead ofReadonlyArray<unknown>.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `isReadonlyArray` wrapper is currently just an alias for `Array.isArray`; consider either inlining `Array.isArray` where used or expanding this helper’s responsibilities (e.g., colocating with other shared type guards or documenting its intended use) so the indirection is justified.
- You can make `isReadonlyArray` generic (e.g., `export function isReadonlyArray<T>(value: readonly T[] | unknown): value is readonly T[]`) so callers get proper element type narrowing instead of `ReadonlyArray<unknown>`.
## Individual Comments
### Comment 1
<location path="src/components/utils/isReadonlyArray.ts" line_range="1-2" />
<code_context>
+export function isReadonlyArray(value: unknown): value is ReadonlyArray<unknown> {
+ return Array.isArray(value);
+}
</code_context>
<issue_to_address>
**suggestion:** Preserve element type in the `isReadonlyArray` type guard using a generic signature.
The current predicate `value is ReadonlyArray<unknown>` prevents callers from getting element-type narrowing from this helper. Making it generic preserves the element type and avoids widening to `unknown`, e.g.:
```ts
export function isReadonlyArray<T>(
value: readonly T[] | T[] | unknown,
): value is ReadonlyArray<T> {
return Array.isArray(value);
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
xexys
marked this pull request as draft
June 3, 2026 12:01
Contributor
|
Preview is ready. |
xexys
force-pushed
the
fix-popup-placement-types
branch
from
June 3, 2026 12:03
525303d to
59e2fd5
Compare
Contributor
|
🎭 Component Tests Report is ready. |
xexys
force-pushed
the
fix-popup-placement-types
branch
3 times, most recently
from
June 3, 2026 12:17
a6394d3 to
71ff73a
Compare
xexys
changed the title
xexys
force-pushed
the
fix-popup-placement-types
branch
from
June 3, 2026 12:20
71ff73a to
58253ad
Compare
xexys
marked this pull request as ready for review
June 3, 2026 12:44
Contributor
There was a problem hiding this comment.
Hey - I've found 5 issues, and left some high level feedback:
- The
isReadonlyArrayhelper currently just forwards toArray.isArray; unless you plan to reuse this type guard in multiple places, it may be clearer to inlineArray.isArrayand avoid introducing a new utility with a potentially misleading name. - Renaming the union member from
Placement[]toReadonlyArray<Placement>doesn’t actually forbid mutable arrays at runtime; if the intent is only to better model existing readonly usage, consider adding a short code comment nearPopupPlacementto clarify that both mutable and readonly arrays are accepted.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `isReadonlyArray` helper currently just forwards to `Array.isArray`; unless you plan to reuse this type guard in multiple places, it may be clearer to inline `Array.isArray` and avoid introducing a new utility with a potentially misleading name.
- Renaming the union member from `Placement[]` to `ReadonlyArray<Placement>` doesn’t actually forbid mutable arrays at runtime; if the intent is only to better model existing readonly usage, consider adding a short code comment near `PopupPlacement` to clarify that both mutable and readonly arrays are accepted.
## Individual Comments
### Comment 1
<location path="src/components/utils/isReadonlyArray.ts" line_range="1-2" />
<code_context>
+export function isReadonlyArray(value: unknown): value is ReadonlyArray<unknown> {
+ return Array.isArray(value);
+}
</code_context>
<issue_to_address>
**suggestion:** Consider making the type guard generic to preserve element types when narrowing.
The current predicate always narrows to `ReadonlyArray<unknown>`, discarding any existing element type. Consider making it generic, e.g. `export function isReadonlyArray<T>(value: unknown): value is ReadonlyArray<T>` (or `readonly T[]`), so callers preserve their specific element type after the check.
Suggested implementation:
```typescript
export function isReadonlyArray<T>(value: unknown): value is ReadonlyArray<T> {
```
```typescript
return Array.isArray(value);
}
```
</issue_to_address>
### Comment 2
<location path="src/components/Popup/README-ru.md" line_range="157" />
<code_context>
+| focusOrder | The order in which focus circle | `Array<'reference' \| 'floating' \| 'content'>` | `['content']` |
</code_context>
<issue_to_address>
**suggestion (typo):** Fix the incomplete sentence in the `focusOrder` description.
"The order in which focus circle" is grammatically incomplete. Please rephrase, e.g. "The order in which focus moves" or "The order in which focus is cycled," to clarify the description.
```suggestion
| focusOrder | The order in which focus is cycled | `Array<'reference' \| 'floating' \| 'content'>` | `['content']` |
```
</issue_to_address>
### Comment 3
<location path="src/components/Popup/README-ru.md" line_range="165-168" />
<code_context>
+| onTransitionIn | On start open popup animation | `Function` | |
</code_context>
<issue_to_address>
**suggestion (typo):** Improve grammar in the `onTransition*` property descriptions.
These descriptions are grammatically awkward. Suggest: "Called when the open popup animation starts/finishes" and "Called when the close popup animation starts/finishes" for clearer wording.
Suggested implementation:
```
| onTransitionIn | Called when the open popup animation starts | `Function` | |
```
```
| onTransitionInComplete | Called when the open popup animation finishes | `Function` | |
```
```
| onTransitionOut | Called when the close popup animation starts | `Function` | |
```
```
| onTransitionOutComplete | Called when the close popup animation finishes | `Function` | |
```
</issue_to_address>
### Comment 4
<location path="src/components/Popup/README.md" line_range="157" />
<code_context>
+| focusOrder | The order in which focus circle | `Array<'reference' \| 'floating' \| 'content'>` | `['content']` |
</code_context>
<issue_to_address>
**suggestion (typo):** Clarify and complete the `focusOrder` description sentence.
The phrase "The order in which focus circle" is incomplete. Consider revising to something like "The order in which focus moves" or "The order in which focus is cycled" so the description is grammatically correct.
```suggestion
| focusOrder | The order in which focus is cycled between popup elements | `Array<'reference' \| 'floating' \| 'content'>` | `['content']` |
```
</issue_to_address>
### Comment 5
<location path="src/components/Popup/README.md" line_range="165-168" />
<code_context>
+| onTransitionIn | On start open popup animation | `Function` | |
</code_context>
<issue_to_address>
**suggestion (typo):** Tighten up the grammar in the `onTransition*` descriptions.
The current phrasing (e.g. "On start open popup animation") is ungrammatical. Consider something like "Called when the open popup animation starts/finishes" and "Called when the close popup animation starts/finishes" to improve readability.
Suggested implementation:
```
| onTransitionIn | Called when the open popup animation starts | `Function` | |
```
```
| onTransitionInComplete | Called when the open popup animation finishes | `Function` | |
```
```
| onTransitionOut | Called when the close popup animation starts | `Function` | |
```
```
| onTransitionOutComplete | Called when the close popup animation finishes | `Function` | |
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+1
to
+2
| export function isReadonlyArray(value: unknown): value is ReadonlyArray<unknown> { | ||
| return Array.isArray(value); |
Contributor
There was a problem hiding this comment.
suggestion: Consider making the type guard generic to preserve element types when narrowing.
The current predicate always narrows to ReadonlyArray<unknown>, discarding any existing element type. Consider making it generic, e.g. export function isReadonlyArray<T>(value: unknown): value is ReadonlyArray<T> (or readonly T[]), so callers preserve their specific element type after the check.
Suggested implementation:
export function isReadonlyArray<T>(value: unknown): value is ReadonlyArray<T> { return Array.isArray(value);
}| | floatingInteractions | Override `Floating UI` interactions | `Array<ElementProps>` | | | ||
| | floatingMiddlewares | `Floating UI` middlewares. If set, they will completely overwrite the default middlewares. | `Array<Middleware>` | | | ||
| | floatingStyles | Styles to apply to the `Floating UI` element | `React.CSSProperties` | | | ||
| | focusOrder | The order in which focus circle | `Array<'reference' \| 'floating' \| 'content'>` | `['content']` | |
Contributor
There was a problem hiding this comment.
suggestion (typo): Fix the incomplete sentence in the focusOrder description.
"The order in which focus circle" is grammatically incomplete. Please rephrase, e.g. "The order in which focus moves" or "The order in which focus is cycled," to clarify the description.
Suggested change
| | focusOrder | The order in which focus circle | `Array<'reference' \| 'floating' \| 'content'>` | `['content']` | | |
| | focusOrder | The order in which focus is cycled | `Array<'reference' \| 'floating' \| 'content'>` | `['content']` | |
Comment on lines
+165
to
+168
| | onTransitionIn | On start open popup animation | `Function` | | | ||
| | onTransitionInComplete | On finish open popup animation | `Function` | | | ||
| | onTransitionOut | On start close popup animation | `Function` | | | ||
| | onTransitionOutComplete | On finish close popup animation | `Function` | | |
Contributor
There was a problem hiding this comment.
suggestion (typo): Improve grammar in the onTransition* property descriptions.
These descriptions are grammatically awkward. Suggest: "Called when the open popup animation starts/finishes" and "Called when the close popup animation starts/finishes" for clearer wording.
Suggested implementation:
| onTransitionIn | Called when the open popup animation starts | `Function` | |
| onTransitionInComplete | Called when the open popup animation finishes | `Function` | |
| onTransitionOut | Called when the close popup animation starts | `Function` | |
| onTransitionOutComplete | Called when the close popup animation finishes | `Function` | |
| | floatingInteractions | Override `Floating UI` interactions | `Array<ElementProps>` | | | ||
| | floatingMiddlewares | `Floating UI` middlewares. If set, they will completely overwrite the default middlewares. | `Array<Middleware>` | | | ||
| | floatingStyles | Styles to apply to the `Floating UI` element | `React.CSSProperties` | | | ||
| | focusOrder | The order in which focus circle | `Array<'reference' \| 'floating' \| 'content'>` | `['content']` | |
Contributor
There was a problem hiding this comment.
suggestion (typo): Clarify and complete the focusOrder description sentence.
The phrase "The order in which focus circle" is incomplete. Consider revising to something like "The order in which focus moves" or "The order in which focus is cycled" so the description is grammatically correct.
Suggested change
| | focusOrder | The order in which focus circle | `Array<'reference' \| 'floating' \| 'content'>` | `['content']` | | |
| | focusOrder | The order in which focus is cycled between popup elements | `Array<'reference' \| 'floating' \| 'content'>` | `['content']` | |
Comment on lines
+165
to
+168
| | onTransitionIn | On start open popup animation | `Function` | | | ||
| | onTransitionInComplete | On finish open popup animation | `Function` | | | ||
| | onTransitionOut | On start close popup animation | `Function` | | | ||
| | onTransitionOutComplete | On finish close popup animation | `Function` | | |
Contributor
There was a problem hiding this comment.
suggestion (typo): Tighten up the grammar in the onTransition* descriptions.
The current phrasing (e.g. "On start open popup animation") is ungrammatical. Consider something like "Called when the open popup animation starts/finishes" and "Called when the close popup animation starts/finishes" to improve readability.
Suggested implementation:
| onTransitionIn | Called when the open popup animation starts | `Function` | |
| onTransitionInComplete | Called when the open popup animation finishes | `Function` | |
| onTransitionOut | Called when the close popup animation starts | `Function` | |
| onTransitionOutComplete | Called when the close popup animation finishes | `Function` | |
xexys
force-pushed
the
fix-popup-placement-types
branch
from
June 5, 2026 13:06
2eed025 to
b8cba09
Compare
amje
requested changes
Jun 11, 2026
| @@ -0,0 +1,3 @@ | |||
| export function isReadonlyArray(value: unknown): value is ReadonlyArray<unknown> { | |||
Contributor
There was a problem hiding this comment.
Let's use this helper in utils file, instead of adding new file
const isArray = Array.isArray as <T>(value: T) => value is Extract<T, readonly unknown[]>;
xexys
force-pushed
the
fix-popup-placement-types
branch
from
June 11, 2026 15:55
b8cba09 to
6b703ef
Compare
xexys
force-pushed
the
fix-popup-placement-types
branch
from
June 11, 2026 15:57
6b703ef to
6a3ea1b
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Align Popup and Popover placement typing and docs to support readonly placement arrays and introduce a shared helper for detecting readonly arrays.
Enhancements: