Copilot commented on code in PR #520:
URL:
https://github.com/apache/skywalking-booster-ui/pull/520#discussion_r2767944045
##########
src/router/guards.ts:
##########
@@ -55,9 +55,7 @@ export function createValidationGuard() {
// Validate route parameters if needed
if (to.params && Object.keys(to.params).length > 0) {
// Add custom validation logic here
- const hasValidParams = Object.values(to.params).every(
- (param) => param !== undefined && param !== null && param !== "",
- );
+ const hasValidParams = Object.values(to.params).every((param) => param
!== undefined);
Review Comment:
This change relaxes the validation to only check for `undefined` values,
allowing `null` and empty strings to pass through. However, it's worth
considering whether allowing empty strings and null is the intended behavior.
In Vue Router, route parameters are typically only `undefined` when not
provided in the URL. Empty strings or null would only occur through
programmatic navigation (e.g., `router.push({ name: 'Route', params: { id: '' }
})`).
If the intent is to support optional parameters, consider whether the route
definitions should explicitly mark parameters as optional (e.g., `:layerId?`)
or if the application logic should handle empty/missing parameters differently.
The current implementation in Edit.vue checks `if (!p.entity)` which
defensively handles both undefined and empty strings, but it may be clearer to
have consistent handling throughout the application.
```suggestion
const hasValidParams = Object.values(to.params).every(
(param) => param !== undefined && param !== null && param !== ""
);
```
##########
src/router/guards.ts:
##########
@@ -55,9 +55,7 @@ export function createValidationGuard() {
// Validate route parameters if needed
if (to.params && Object.keys(to.params).length > 0) {
// Add custom validation logic here
- const hasValidParams = Object.values(to.params).every(
- (param) => param !== undefined && param !== null && param !== "",
- );
+ const hasValidParams = Object.values(to.params).every((param) => param
!== undefined);
Review Comment:
The validation logic has been relaxed to only check for `undefined` values,
but the existing test cases in `src/router/__tests__/guards.spec.ts` still
expect empty strings and null values to be invalid. Specifically, tests at
lines 127-135 and 147-155 will fail with this change because they expect routes
with `id: ""`, `name: null`, and `id: "123", name: ""` to be redirected to
NotFound, but the new validation will allow these through.
Either the tests need to be updated to match the new validation behavior, or
the validation logic needs to maintain checks for null and empty strings if
those are genuinely invalid states for route parameters.
--
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]
