Copilot commented on code in PR #83: URL: https://github.com/apache/skywalking-horizon-ui/pull/83#discussion_r3486387421
########## scripts/check-source-budget.mjs: ########## @@ -0,0 +1,85 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +// Source-budget guardrail. ESLint's max-lines covers code size in-editor; +// this gate adds the comment-VOLUME cap ESLint cannot express, and is the +// CI-runnable enforcer of both. A file is over budget when it carries more +// than CODE_MAX lines of code OR more than COMMENT_MAX lines of comment. +// SKIP is an explicit exemption list for files still mid-decomposition; +// empty now that every source file is within budget. + +import { readFileSync } from 'node:fs'; +import { execSync } from 'node:child_process'; + +const CODE_MAX = 2000; +const COMMENT_MAX = 500; +const SKIP = new Set([]); + +const files = execSync('git ls-files', { encoding: 'utf8' }) + .split('\n') + .filter((f) => /\.(ts|vue|js|mjs)$/.test(f) && !f.endsWith('.d.ts')) + .filter((f) => f.startsWith('apps/') || f.startsWith('packages/') || f.startsWith('scripts/')) + .filter((f) => !/\.(test|spec)\.ts$/.test(f) && !SKIP.has(f)); Review Comment: This script hard-depends on `git ls-files`. That works in CI, but it will fail in git-less environments (e.g. a source zip/tarball without a `.git` directory), making `pnpm lint` unusable there. At minimum, it would be good to fail with a clear, purpose-specific error message (or add a non-git fallback file walker). ########## apps/ui/src/features/admin/layer-templates/ServiceListMetricsEditor.vue: ########## @@ -0,0 +1,334 @@ +<!-- + Licensed to the Apache Software Foundation (ASF) under one or more + contributor license agreements. See the NOTICE file distributed with + this work for additional information regarding copyright ownership. + The ASF licenses this file to You under the Apache License, Version 2.0 + (the "License"); you may not use this file except in compliance with + the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +--> +<!-- + Service-list metrics editor — the columns (+ default sort) shown in the + picker zone's services table, used across the per-layer page. Config-local: + owns the template's `metrics` block via v-model and seeds an empty one on + mount (mirrors the legacy ensureMetrics) so the operator can add columns + without hand-editing JSON. The block is mutated IN PLACE (it is part of the + live draft), so the model value is the same object the parent's draft holds. + The sample-data preview fires no MQE — it shows how the configured columns + render (label, scale, precision, unit, default-sort marker). +--> +<script setup lang="ts"> +import { computed, onMounted } from 'vue'; +import type { AdminLayerTemplate } from '@/api/client'; +import { fmtMetric } from '@/utils/formatters'; + +const config = defineModel<AdminLayerTemplate['metrics'] | undefined>('config'); +defineProps<{ serviceLabel: string }>(); + +function ensure(): NonNullable<AdminLayerTemplate['metrics']> { + if (!config.value) config.value = {}; + return config.value; +} +onMounted(ensure); + +// Mirrors the legacy metricsModel: seeds the block on access so the orderBy +// control + columns table render even before the operator touches the JSON +// (the block is part of the live draft and is mutated in place). +const metricsModel = computed(() => { + ensure(); + return config.value; +}); +const metricsColumns = computed(() => { + const m = ensure(); + if (!m.columns) m.columns = []; + return m.columns; +}); +function addMetricColumn(): void { + const m = ensure(); + if (!m.columns) m.columns = []; + m.columns.push({ + metric: `metric_${m.columns.length + 1}`, + label: `Metric ${m.columns.length + 1}`, + aggregation: 'avg', + }); +} +function deleteMetricColumn(i: number): void { + if (!config.value?.columns) return; + config.value.columns.splice(i, 1); +} + +/** Sample rows for the service-list preview — three fake services with + * deterministic per-column values so the author sees how the configured + * columns render (label, scale, precision, unit, default-sort marker) + * without firing any MQE. */ +const SAMPLE_SERVICES = ['checkout', 'inventory', 'gateway']; +function previewBase(seed: number): number { + return [42.37, 1280.5, 0.918][seed % 3] * (1 + (seed % 5) * 0.35); +} +function previewCell(col: { scale?: number; precision?: number; unit?: string }, seed: number): string { + const v = previewBase(seed) * (col.scale ?? 1); + const num = col.precision != null ? v.toFixed(col.precision) : fmtMetric(v); + return col.unit ? `${num} ${col.unit}` : num; +} +/** The metric the service list sorts by — explicit `orderBy`, else the + * first column (mirrors the renderer's fallback). */ +const effectiveOrderBy = computed( + () => config.value?.orderBy ?? metricsColumns.value[0]?.metric, +); +</script> + +<template> + <!-- Service-list metrics: the columns shown in the picker + zone's services table + the default sort. Used across + the per-layer page. --> + <section class="sw-card metrics-card"> + <div class="card-head"> + <h4>Service list metrics</h4> + <span class="sub">columns + default sort for the service list (picker zone)</span> + <button class="sw-btn add" type="button" @click="addMetricColumn">＋ Add column</button> + </div> + <div v-if="metricsModel" class="metrics-keys"> + <label> + <span>Default sort (orderBy)</span> + <select v-model="metricsModel.orderBy"> + <option :value="undefined">(first column)</option> + <option v-for="c in metricsColumns" :key="c.metric" :value="c.metric">{{ c.metric }}</option> + </select> + </label> + </div> + <div v-if="metricsColumns.length === 0" class="empty inset"> + No metric columns defined. Click "Add column" to start. + </div> + <table v-else class="sw-table metrics-table"> + <thead> + <tr> + <th>metric</th> + <th>label</th> + <th>unit</th> + <th>aggregation</th> + <th class="grow">mqe</th> + <th>scale</th> + <th>precision</th> + <th></th> + </tr> + </thead> + <tbody> + <tr v-for="(c, i) in metricsColumns" :key="i"> Review Comment: Using the loop index as the Vue key will cause DOM/input reuse when rows are deleted, which can make edited values appear to “move” to a different row. Since this table supports deleting columns, it should use a stable per-row key. ########## apps/ui/src/features/admin/layer-templates/ServiceListMetricsEditor.vue: ########## @@ -0,0 +1,334 @@ +<!-- + Licensed to the Apache Software Foundation (ASF) under one or more + contributor license agreements. See the NOTICE file distributed with + this work for additional information regarding copyright ownership. + The ASF licenses this file to You under the Apache License, Version 2.0 + (the "License"); you may not use this file except in compliance with + the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +--> +<!-- + Service-list metrics editor — the columns (+ default sort) shown in the + picker zone's services table, used across the per-layer page. Config-local: + owns the template's `metrics` block via v-model and seeds an empty one on + mount (mirrors the legacy ensureMetrics) so the operator can add columns + without hand-editing JSON. The block is mutated IN PLACE (it is part of the + live draft), so the model value is the same object the parent's draft holds. + The sample-data preview fires no MQE — it shows how the configured columns + render (label, scale, precision, unit, default-sort marker). +--> +<script setup lang="ts"> +import { computed, onMounted } from 'vue'; +import type { AdminLayerTemplate } from '@/api/client'; +import { fmtMetric } from '@/utils/formatters'; + +const config = defineModel<AdminLayerTemplate['metrics'] | undefined>('config'); +defineProps<{ serviceLabel: string }>(); + +function ensure(): NonNullable<AdminLayerTemplate['metrics']> { + if (!config.value) config.value = {}; + return config.value; +} +onMounted(ensure); + +// Mirrors the legacy metricsModel: seeds the block on access so the orderBy +// control + columns table render even before the operator touches the JSON +// (the block is part of the live draft and is mutated in place). +const metricsModel = computed(() => { + ensure(); + return config.value; +}); +const metricsColumns = computed(() => { + const m = ensure(); + if (!m.columns) m.columns = []; + return m.columns; +}); +function addMetricColumn(): void { + const m = ensure(); + if (!m.columns) m.columns = []; + m.columns.push({ + metric: `metric_${m.columns.length + 1}`, + label: `Metric ${m.columns.length + 1}`, + aggregation: 'avg', + }); +} +function deleteMetricColumn(i: number): void { + if (!config.value?.columns) return; + config.value.columns.splice(i, 1); +} + +/** Sample rows for the service-list preview — three fake services with + * deterministic per-column values so the author sees how the configured + * columns render (label, scale, precision, unit, default-sort marker) + * without firing any MQE. */ +const SAMPLE_SERVICES = ['checkout', 'inventory', 'gateway']; +function previewBase(seed: number): number { + return [42.37, 1280.5, 0.918][seed % 3] * (1 + (seed % 5) * 0.35); +} +function previewCell(col: { scale?: number; precision?: number; unit?: string }, seed: number): string { + const v = previewBase(seed) * (col.scale ?? 1); + const num = col.precision != null ? v.toFixed(col.precision) : fmtMetric(v); + return col.unit ? `${num} ${col.unit}` : num; +} +/** The metric the service list sorts by — explicit `orderBy`, else the + * first column (mirrors the renderer's fallback). */ +const effectiveOrderBy = computed( + () => config.value?.orderBy ?? metricsColumns.value[0]?.metric, +); +</script> + +<template> + <!-- Service-list metrics: the columns shown in the picker + zone's services table + the default sort. Used across + the per-layer page. --> + <section class="sw-card metrics-card"> + <div class="card-head"> + <h4>Service list metrics</h4> + <span class="sub">columns + default sort for the service list (picker zone)</span> + <button class="sw-btn add" type="button" @click="addMetricColumn">＋ Add column</button> + </div> + <div v-if="metricsModel" class="metrics-keys"> + <label> + <span>Default sort (orderBy)</span> + <select v-model="metricsModel.orderBy"> + <option :value="undefined">(first column)</option> + <option v-for="c in metricsColumns" :key="c.metric" :value="c.metric">{{ c.metric }}</option> + </select> + </label> + </div> + <div v-if="metricsColumns.length === 0" class="empty inset"> + No metric columns defined. Click "Add column" to start. + </div> + <table v-else class="sw-table metrics-table"> + <thead> + <tr> + <th>metric</th> + <th>label</th> + <th>unit</th> + <th>aggregation</th> + <th class="grow">mqe</th> + <th>scale</th> + <th>precision</th> + <th></th> + </tr> + </thead> + <tbody> + <tr v-for="(c, i) in metricsColumns" :key="i"> + <td><input class="mono" v-model="c.metric" /></td> + <td><input v-model="c.label" /></td> + <td><input v-model="c.unit" placeholder="—" /></td> + <td> + <select v-model="c.aggregation"> + <option value="sum">sum</option> + <option value="avg">avg</option> + </select> + </td> + <td><input class="mono" v-model="c.mqe" placeholder="catalog default" /></td> + <td><input type="number" step="any" v-model.number="c.scale" placeholder="1" /></td> + <td><input type="number" min="0" max="6" v-model.number="c.precision" placeholder="auto" /></td> Review Comment: `v-model.number` leaves `''` (a string) when the input is cleared; since `scale`/`precision` are typed as optional numbers in `AdminLayerTemplate`, clearing these fields would serialize invalid values ("" instead of `undefined`). Consider normalizing empty input back to `undefined`. ########## apps/ui/src/features/admin/layer-templates/TopologyConfigEditor.vue: ########## @@ -0,0 +1,359 @@ +<!-- + Licensed to the Apache Software Foundation (ASF) under one or more + contributor license agreements. See the NOTICE file distributed with + this work for additional information regarding copyright ownership. + The ASF licenses this file to You under the Apache License, Version 2.0 + (the "License"); you may not use this file except in compliance with + the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +--> +<!-- + Topology config editor — service-topology node/link metrics plus the optional + instance-topology drill-down block. Config-local: owns the `topology` block + via v-model, seeds an empty one on mount, and the instance-topology block is + created/removed by the Enable toggle (instance buckets never auto-create — + they read straight off the nested block). Alias-aware nouns (service / + instance) come in as props because they resolve against the live template's + slots, which the parent owns. +--> +<script setup lang="ts"> +import { computed, onMounted } from 'vue'; +import type { TopologyConfig, TopologyMetricDef } from '@skywalking-horizon-ui/api-client'; +import { TOPOLOGY_ROLE_OPTIONS } from './layer-dashboards.scopes'; +import MetricDefinitionRow from './MetricDefinitionRow.vue'; + +const config = defineModel<TopologyConfig | undefined>('config'); +defineProps<{ serviceNoun: string; instanceNoun: string }>(); + +function ensure(): TopologyConfig { + if (!config.value) config.value = { nodeMetrics: [], linkServerMetrics: [], linkClientMetrics: [] }; + if (!config.value.linkServerMetrics) config.value.linkServerMetrics = []; + if (!config.value.linkClientMetrics) config.value.linkClientMetrics = []; + return config.value; +} +onMounted(ensure); + +const nodeMetrics = computed(() => config.value?.nodeMetrics ?? []); +const serverMetrics = computed(() => config.value?.linkServerMetrics ?? []); +const clientMetrics = computed(() => config.value?.linkClientMetrics ?? []); +const showGroup = computed(() => Boolean(config.value?.showGroup)); +const instanceEnabled = computed(() => !!config.value?.instanceTopology); +const instNodeMetrics = computed(() => config.value?.instanceTopology?.nodeMetrics ?? []); +const instServerMetrics = computed(() => config.value?.instanceTopology?.linkServerMetrics ?? []); +const instClientMetrics = computed(() => config.value?.instanceTopology?.linkClientMetrics ?? []); + +function toggleShowGroup(): void { + const t = ensure(); + t.showGroup = !t.showGroup; +} +// Start empty — the operator authors instance-scope metrics +// (service_instance_* / service_instance_relation_*) themselves; we never copy +// the service-scope metrics down (wrong scope, misleading). +function toggleInstance(): void { + const t = ensure(); + if (t.instanceTopology) delete t.instanceTopology; + else t.instanceTopology = { nodeMetrics: [], linkServerMetrics: [], linkClientMetrics: [] }; +} +function blankMetric(n: number): TopologyMetricDef { + return { id: `metric_${n + 1}`, label: `Metric ${n + 1}`, mqe: '', unit: '', aggregation: 'avg' }; +} +function addNode(): void { ensure().nodeMetrics.push(blankMetric(nodeMetrics.value.length)); } +function addServer(): void { ensure().linkServerMetrics!.push(blankMetric(serverMetrics.value.length)); } +function addClient(): void { ensure().linkClientMetrics!.push(blankMetric(clientMetrics.value.length)); } +function addInstNode(): void { ensure().instanceTopology!.nodeMetrics.push(blankMetric(instNodeMetrics.value.length)); } +function addInstServer(): void { ensure().instanceTopology!.linkServerMetrics!.push(blankMetric(instServerMetrics.value.length)); } +function addInstClient(): void { ensure().instanceTopology!.linkClientMetrics!.push(blankMetric(instClientMetrics.value.length)); } +function move(list: TopologyMetricDef[], i: number, dir: -1 | 1): void { + const j = i + dir; + if (j < 0 || j >= list.length) return; + [list[i], list[j]] = [list[j], list[i]]; +} +function remove(list: TopologyMetricDef[], i: number): void { + list.splice(i, 1); +} +</script> + +<template> + <section class="sw-card editor-card topo-cfg-card"> + <div class="card-head"> + <h4>Topology config</h4> + <span class="sub">node + server-side + client-side line metrics. Add rows; bind a metric to a visual role.</span> + </div> + <div class="naming-prefix-row"> + <label class="comp-toggle" :class="{ on: showGroup }"> + <input type="checkbox" :checked="showGroup" @change="toggleShowGroup" /> + <span class="comp-label">Show <code><group>::</code> as a chip in the node panel</span> + </label> + <span class="naming-prefix-hint"> + Off: <code>mesh-svr::reviews</code> reads as <code>reviews</code> everywhere. + On: <code>mesh-svr</code> appears as a separate chip in the clicked-node panel. + Topology graph labels are always pure service names. + </span> + </div> + <div class="topo-cfg-body"> + <div class="topo-cfg-group"> + <span class="tg-title">Service topology</span> + <span class="tg-sub">node = {{ serviceNoun }} · edges = service-to-service relations</span> + </div> + <div class="topo-cfg-section"> + <header class="topo-cfg-head"> + <h5>{{ serviceNoun }} node metrics</h5> + <span class="sub">drives each node's center number + ring colour band</span> + <button class="sw-btn add" type="button" @click="addNode">＋ Add</button> + </header> + <div v-if="nodeMetrics.length === 0" class="topo-cfg-empty">No node metrics. Click "+ Add" to start.</div> + <div v-else class="metric-list"> + <MetricDefinitionRow + v-for="(_m, i) in nodeMetrics" + :key="i" + v-model:metric="nodeMetrics[i]" Review Comment: These metric rows are re-ordered via the Move up/down actions, but the v-for key is the array index. Index keys can cause Vue to reuse the wrong DOM nodes when reordering, leading to confusing input/focus behavior. Since each metric has an `id`, it should be used as the key. -- 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]
