Copilot commented on code in PR #3309:
URL: https://github.com/apache/apisix-dashboard/pull/3309#discussion_r2893230767
##########
e2e/tests/secrets.list.spec.ts:
##########
@@ -88,7 +88,7 @@ test.describe('page and page_size should work correctly', ()
=> {
.getByRole('cell', { name: /secret_id_/ })
.all();
const ids = await Promise.all(itemsInPage.map((v) => v.textContent()));
- return secrets.filter((d) => !ids.includes(d.id));
+ return secrets.filter((d) => !ids.some(idText => idText?.includes(d.id)));
Review Comment:
`ids.some(idText => idText?.includes(d.id))` can treat `secret_id_1` as
present when only `secret_id_10` is on the page, making the pagination filter
incorrect. Prefer comparing against the parsed/trimmed ID text for equality, or
use a stricter boundary-aware match.
##########
e2e/tests/consumer_groups.list.spec.ts:
##########
@@ -69,7 +70,7 @@ test.describe('page and page_size should work correctly', ()
=> {
.getByRole('cell', { name: /test-consumer-group-/ })
.all();
const ids = await Promise.all(itemsInPage.map((v) => v.textContent()));
- return consumerGroups.filter((d) => !ids.includes(d.id));
+ return consumerGroups.filter((d) => !ids.some(idText =>
idText?.includes(d.id)));
Review Comment:
Substring matching (`idText?.includes(d.id)`) can falsely match
`test-consumer-group-1` inside `test-consumer-group-10`, causing the pagination
filter to return incorrect results. Prefer equality after extracting the ID
text or use a boundary-aware/escaped regex match.
```suggestion
return consumerGroups.filter((d) => !ids.some((idText) => idText?.trim()
=== d.id));
```
##########
e2e/tests/protos.list.spec.ts:
##########
@@ -84,7 +85,7 @@ test.describe('page and page_size should work correctly', ()
=> {
.getByRole('cell', { name: /proto_id_/ })
.all();
const ids = await Promise.all(itemsInPage.map((v) => v.textContent()));
- return protos.filter((d) => !ids.includes(d.id));
+ return protos.filter((d) => !ids.some(idText => idText?.includes(d.id)));
Review Comment:
Using `includes(d.id)` for presence checks can cause `proto_id_1` to match
`proto_id_10`/`proto_id_11`, so `filterItemsNotInPage` may exclude the wrong
items. Consider extracting the ID from `textContent()` and comparing for
equality (or use a boundary-aware regex with escaped ID).
```suggestion
return protos.filter((d) => !ids.some((idText) => idText?.trim() ===
d.id));
```
##########
e2e/tests/global_rules.list.spec.ts:
##########
@@ -114,7 +114,7 @@ test.describe('page and page_size should work correctly',
() => {
.getByRole('cell', { name: /global_rule_id_/ })
.all();
const ids = await Promise.all(itemsInPage.map((v) => v.textContent()));
- return globalRules.filter((d) => !ids.includes(d.id));
+ return globalRules.filter((d) => !ids.some(idText =>
idText?.includes(d.id)));
Review Comment:
`idText?.includes(d.id)` can return true for partial matches (e.g.
`global_rule_id_1` vs `global_rule_id_10`), which makes `filterItemsNotInPage`
unreliable. Use an exact/escaped match against the ID portion of the cell text
instead of substring matching.
```suggestion
const cellIds = ids
.map((text) => text?.match(/global_rule_id_\d+/)?.[0] ?? '')
.filter(Boolean) as string[];
return globalRules.filter((d) => !cellIds.includes(d.id));
```
##########
e2e/tests/stream_routes.list.spec.ts:
##########
@@ -85,7 +86,7 @@ test.describe('page and page_size should work correctly', ()
=> {
.getByRole('cell', { name: /stream_route_id_/ })
.all();
const ids = await Promise.all(itemsInPage.map((v) => v.textContent()));
- return streamRoutes.filter((d) => !ids.includes(d.id));
+ return streamRoutes.filter((d) => !ids.some(idText =>
idText?.includes(d.id)));
Review Comment:
`ids.some(idText => idText?.includes(d.id))` can produce false matches when
IDs share prefixes (e.g. `stream_route_id_1` is a substring of
`stream_route_id_10`), which will break the pagination filter. Use an exact
match strategy (e.g. normalize/trim the cell text then compare equality) or a
stricter match like `startsWith` with a delimiter-aware check/escaped regex.
```suggestion
return streamRoutes.filter((d) => !ids.some((idText) => idText?.trim()
=== d.id));
```
##########
src/components/form/Editor.tsx:
##########
@@ -109,6 +109,19 @@ export const FormItemEditor = <T extends FieldValues>(
setLoading(false);
}, [customSchema]);
+ // eslint-disable-next-line @typescript-eslint/no-explicit-any
+ const editorRef = useRef<any>(null);
+
+ useEffect(() => {
+ return () => {
+ // eslint-disable-next-line @typescript-eslint/no-explicit-any
+ const w = window as any;
+ if (w.__monacoEditor__ === editorRef.current) {
+ w.__monacoEditor__ = undefined;
+ }
+ };
+ }, []);
+
Review Comment:
`editorRef` is initialized but never assigned to the mounted editor, so the
cleanup comparison against `editorRef.current` will always be `null` and can
never clear `window.__monacoEditor__`. Either set `editorRef.current = editor`
in `onMount` (and clear it in `onUnmount`) or remove this ref/effect and rely
on a single cleanup path.
```suggestion
```
##########
src/components/form/Editor.tsx:
##########
@@ -145,8 +158,13 @@ export const FormItemEditor = <T extends FieldValues>(
trigger(props.name);
}}
onMount={(editor) => {
- if (process.env.NODE_ENV === 'test') {
- window.__monacoEditor__ = editor;
+ window.__monacoEditor__ = editor;
+ (window as any).__monacoEditor__ = editor;
+ }}
+ onUnmount={(editor) => {
+ const w = window as any;
+ if (w.__monacoEditor__ === editor) {
+ w.__monacoEditor__ = undefined;
Review Comment:
`onMount` assigns `__monacoEditor__` twice (`window.__monacoEditor__` and
`(window as any).__monacoEditor__`), which is redundant (the property is
already typed in `src/types/global.d.ts`) and bypasses type checking. Consider
keeping a single assignment (and, if the global exposure is only meant for
E2E/tests, guard it to avoid introducing a production-only global side effect).
```suggestion
}}
onUnmount={(editor) => {
if (window.__monacoEditor__ === editor) {
window.__monacoEditor__ = undefined;
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
