Re: [PR] feat: add global copy ID button to all resources [apisix-dashboard]
Baluduvamsi2006 closed pull request #3309: feat: add global copy ID button to all resources URL: https://github.com/apache/apisix-dashboard/pull/3309 -- 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]
Re: [PR] feat: add global copy ID button to all resources [apisix-dashboard]
Copilot commented on code in PR #3309:
URL: https://github.com/apache/apisix-dashboard/pull/3309#discussion_r2897864126
##
e2e/tests/consumer_groups.crud-all-fields.spec.ts:
##
@@ -139,7 +139,7 @@ test('should CRUD Consumer Group with all fields', async ({
page }) => {
await consumerGroupsPom.isIndexPage(page);
// Verify consumer group exists
-await expect(page.getByRole('cell', { name: testId, exact: true
})).toBeVisible();
+await expect(page.getByRole('cell', { name: new RegExp(`^${testId}`)
})).toBeVisible();
await expect(
Review Comment:
`new RegExp(`^${testId}`)` uses an unescaped string as a regex source, which
can lead to accidental over/under-matching if `testId` changes to include regex
metacharacters. Escape `testId` (or avoid regex entirely) to keep the test
deterministic.
##
e2e/tests/consumer_groups.crud-required-fields.spec.ts:
##
@@ -94,7 +94,7 @@ test('should CRUD Consumer Group with required fields', async
({ page }) => {
// Verify consumer group exists in list
await expect(
- page.getByRole('cell', { name: testId, exact: true })
+ page.getByRole('cell', { name: new RegExp(`^${testId}`) })
).toBeVisible();
Review Comment:
`new RegExp(`^${testId}`)` treats `testId` as a regex pattern; if it ever
contains regex metacharacters, the locator can match the wrong row/cell and
make the test flaky. Escape `testId` before building the regex (or use `exact:
true` with a more robust locator that ignores the copy icon).
--
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]
Re: [PR] feat: add global copy ID button to all resources [apisix-dashboard]
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) => (
+
+{record.value.id}
+
+),
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 = (
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]
Re: [PR] feat: add global copy ID button to all resources [apisix-dashboard]
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 = (
setLoading(false);
}, [customSchema]);
+ // eslint-disable-next-line @typescript-eslint/no-explicit-any
+ const editorRef = useRef(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
Re: [PR] feat: add global copy ID button to all resources [apisix-dashboard]
Copilot commented on code in PR #3309:
URL: https://github.com/apache/apisix-dashboard/pull/3309#discussion_r2872357797
##
src/components/form/Editor.tsx:
##
@@ -145,9 +145,7 @@ export const FormItemEditor = (
trigger(props.name);
}}
onMount={(editor) => {
- if (process.env.NODE_ENV === 'test') {
-window.__monacoEditor__ = editor;
- }
+ window.__monacoEditor__ = editor;
}}
Review Comment:
`window.__monacoEditor__` is now assigned unconditionally. Since this
creates a long-lived global reference, it can keep disposed editors/models from
being garbage-collected and can leave a stale instance around after unmount.
Consider either (a) clearing the global on unmount (if `@monaco-editor/react`'s
`onUnmount` is available) or (b) guarding the assignment behind an explicit
E2E/dev flag so production doesn’t retain this global reference.
```suggestion
(window as any).__monacoEditor__ = editor;
}}
onUnmount={(editor) => {
const w = window as any;
if (w.__monacoEditor__ === editor) {
w.__monacoEditor__ = undefined;
}
}}
```
##
e2e/utils/ui/index.ts:
##
@@ -64,11 +64,24 @@ export async function uiFillHTTPStatuses(
}
}
-export const uiClearMonacoEditor = async (page: Page) => {
- await page.evaluate(() => {
-const editor = window.__monacoEditor__;
-editor.getModel()?.setValue('');
- });
+export const uiClearMonacoEditor = async (page: Page, editorLoc?: Locator) => {
+ const isSet = await page.evaluate(() => window.__monacoEditor__ !==
undefined).catch(() => false);
+ if (isSet) {
+await page.evaluate(() => {
+ const editor = window.__monacoEditor__;
+ editor?.getModel()?.setValue('');
+});
Review Comment:
`uiClearMonacoEditor` currently performs two separate `page.evaluate` calls
(one to check presence, one to clear). This can be simplified into a single
evaluate (attempt to clear if present and return a success flag), which reduces
round-trips and makes it easier to fall back to the keyboard-based clearing
when the evaluate fails.
```suggestion
const clearedViaMonaco = await page
.evaluate(() => {
try {
const editor = (window as any).__monacoEditor__;
if (!editor || typeof editor.getModel !== 'function') {
return false;
}
const model = editor.getModel();
if (!model || typeof model.setValue !== 'function') {
return false;
}
model.setValue('');
return true;
} catch {
return false;
}
})
.catch(() => false);
if (clearedViaMonaco) {
```
--
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]
Re: [PR] feat: add global copy ID button to all resources [apisix-dashboard]
Baluduvamsi2006 commented on PR #3309: URL: https://github.com/apache/apisix-dashboard/pull/3309#issuecomment-3979856467 @Baoyuantop @SkyeYoung Could you please take a moment to review this PR when you have some time? Let me know if you'd like any adjustments or have any feedback! Thank you! 🚀 -- 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]
