Copilot commented on code in PR #3324:
URL: https://github.com/apache/apisix-dashboard/pull/3324#discussion_r2921834445
##########
e2e/utils/ui/upstreams.ts:
##########
@@ -31,7 +31,8 @@ import { uiFillHTTPStatuses } from '.';
*/
export async function uiFillUpstreamRequiredFields(
ctx: Page | Locator,
- upstream: Partial<APISIXType['Upstream']>
+ upstream: Partial<APISIXType['Upstream']>,
+ page?: Page
) {
Review Comment:
The helper now requires callers to sometimes pass `page` separately even
though `ctx` is already `Page | Locator`. Since a `Locator` can provide its
owning page (via `locator.page()`), you can remove the extra `page?: Page`
parameter and derive a `Page` internally when needed, keeping the helper API
simpler and less error-prone for future call sites.
##########
e2e/utils/ui/upstreams.ts:
##########
@@ -50,19 +51,22 @@ export async function uiFillUpstreamRequiredFields(
const firstRowHost = rows.nth(0).getByRole('textbox').first();
await firstRowHost.fill(upstream.nodes[1].host);
await expect(firstRowHost).toHaveValue(upstream.nodes[1].host);
- await nodesSection.click();
- // Add second node
+ // Add second node - blur first, wait for useClickOutside state sync, then
click Add
+ await firstRowHost.blur();
+ if (page) await page.waitForTimeout(500);
await addNodeBtn.click();
- await expect(rows.nth(1)).toBeVisible();
+ await expect(rows).toHaveCount(2, { timeout: 10000 });
Review Comment:
`waitForTimeout(500)` introduces a fixed delay that can make E2E runs slower
and still flaky under load (500ms may be too short in CI). Prefer waiting on an
explicit UI condition that indicates the “click-outside → state sync” finished
(e.g., a specific field value/validation state, button enabled state, or any
DOM change that reflects the sync) rather than sleeping for a constant duration.
##########
package.json:
##########
@@ -100,8 +100,12 @@
"packageManager":
"[email protected]+sha512.d615db246fe70f25dcfea6d8d73dee782ce23e2245e3c4f6f888249fb568149318637dca73c2c5c8ef2a4ca0d5657fb9567188bfab47f566d1ee6ce987815c39",
"pnpm": {
"overrides": {
- "lodash": ">=4.17.21",
- "minimatch": ">=3.0.5",
+ "lodash": ">=4.17.23",
+ "lodash-es": ">=4.17.23",
+ "minimatch": ">=9.0.7",
+ "rollup": ">=4.59.0",
+ "simple-git": ">=3.32.3",
+ "diff": ">=8.0.3",
Review Comment:
Using `>=` ranges in `pnpm.overrides` can unexpectedly jump to new major
versions (e.g., a future `rollup@5`, `minimatch@10`, etc.), which may introduce
breaking changes during a routine install or lockfile refresh. Prefer
constraining to the intended major line (e.g., `^4.59.0` for Rollup, `^9.0.7`
for minimatch, `^4.17.23` for lodash/lodash-es) unless you explicitly want “any
future major” behavior.
```suggestion
"lodash": "^4.17.23",
"lodash-es": "^4.17.23",
"minimatch": "^9.0.7",
"rollup": "^4.59.0",
"simple-git": "^3.32.3",
"diff": "^8.0.3",
```
##########
e2e/tests/protos.crud-required-fields.spec.ts:
##########
@@ -75,7 +75,7 @@ test.describe('CRUD proto with required fields only', () => {
);
expect(createdProto).toBeDefined();
expect(createdProto?.value.id).toBeDefined();
- // eslint-disable-next-line playwright/no-conditional-in-test
+
createdProtoId = createdProto?.value.id || '';
Review Comment:
The updated file introduces an “empty” line here (previously it was an
ESLint suppression comment). Ensure this line is a truly empty line (no
trailing spaces) to avoid whitespace-only diffs and potential formatting/lint
noise in future changes.
##########
src/routeTree.gen.ts:
##########
@@ -310,18 +310,18 @@ export interface FileRoutesByFullPath {
'/ssls/add': typeof SslsAddRoute
'/stream_routes/add': typeof Stream_routesAddRoute
'/upstreams/add': typeof UpstreamsAddRoute
- '/consumer_groups': typeof Consumer_groupsIndexRoute
- '/consumers': typeof ConsumersIndexRoute
- '/global_rules': typeof Global_rulesIndexRoute
- '/plugin_configs': typeof Plugin_configsIndexRoute
- '/plugin_metadata': typeof Plugin_metadataIndexRoute
- '/protos': typeof ProtosIndexRoute
- '/routes': typeof RoutesIndexRoute
- '/secrets': typeof SecretsIndexRoute
- '/services': typeof ServicesIndexRoute
- '/ssls': typeof SslsIndexRoute
- '/stream_routes': typeof Stream_routesIndexRoute
- '/upstreams': typeof UpstreamsIndexRoute
+ '/consumer_groups/': typeof Consumer_groupsIndexRoute
+ '/consumers/': typeof ConsumersIndexRoute
+ '/global_rules/': typeof Global_rulesIndexRoute
+ '/plugin_configs/': typeof Plugin_configsIndexRoute
+ '/plugin_metadata/': typeof Plugin_metadataIndexRoute
+ '/protos/': typeof ProtosIndexRoute
+ '/routes/': typeof RoutesIndexRoute
+ '/secrets/': typeof SecretsIndexRoute
+ '/services/': typeof ServicesIndexRoute
+ '/ssls/': typeof SslsIndexRoute
+ '/stream_routes/': typeof Stream_routesIndexRoute
+ '/upstreams/': typeof UpstreamsIndexRoute
Review Comment:
This changes the canonical “index” URLs from `'/routes'` to `'/routes/'`
(and similarly for several top-level pages), which can be a breaking change for
bookmarks, deep links, and any code constructing paths as strings. Consider
ensuring both variants are accepted (e.g., via a redirect/alias strategy in the
router or server) so existing `'/routes'` links continue to work while
`'/routes/'` becomes canonical.
--
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]
