Copilot commented on code in PR #3309:
URL: https://github.com/apache/apisix-dashboard/pull/3309#discussion_r2895139078
##########
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:
`filterItemsNotInPage` uses `idText?.includes(d.id)` to decide whether an ID
is present in the current page. This can produce false positives (e.g.,
`secret_id_1` is a substring of `secret_id_10`), making the pagination test
incorrect/flaky. Prefer extracting the ID via regex and comparing exact values
(or comparing `idText?.trim() === d.id`).
```suggestion
return secrets.filter((d) =>
!ids.some((idText) => idText?.trim() === d.id)
);
```
##########
src/routes/services/index.tsx:
##########
@@ -41,7 +41,16 @@ const ServiceList = () => {
dataIndex: ['value', 'id'],
title: 'ID',
key: 'id',
- valueType: 'text',
+ render: (_, record) => (
+ <Typography.Text
+ copyable={{
+ text: String(record.value.id),
+ tooltips: [t('copy'), t('copy_success')],
+ }}
+ >
+ {record.value.id}
+ </Typography.Text>
+ ),
Review Comment:
The `Typography.Text copyable` render logic is duplicated across many list
pages in this PR. To avoid future drift (e.g., tooltip keys, casting, styling),
consider extracting a small shared component/helper (e.g. `CopyableId`) and
reusing it for all resource ID cells.
##########
src/components/form/Editor.tsx:
##########
@@ -145,8 +145,11 @@ export const FormItemEditor = <T extends FieldValues>(
trigger(props.name);
}}
onMount={(editor) => {
- if (process.env.NODE_ENV === 'test') {
- window.__monacoEditor__ = editor;
+ window.__monacoEditor__ = editor;
+ }}
+ onUnmount={(editor) => {
+ if (window.__monacoEditor__ === editor) {
+ window.__monacoEditor__ = undefined;
}
Review Comment:
`window.__monacoEditor__` is now assigned for every mount (not just tests).
That exposes a mutable global reference in production builds and can be
clobbered when multiple Monaco instances exist, which may cause e2e helpers to
clear the wrong editor. Consider gating this behind an explicit e2e/dev flag
(e.g. a Vite env var) and/or storing the editor reference in a way that targets
the specific editor instance used in the test.
--
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]
