Copilot commented on code in PR #1224:
URL:
https://github.com/apache/skywalking-banyandb/pull/1224#discussion_r3635631544
##########
Makefile:
##########
@@ -300,16 +300,20 @@ pre-push: ## Check source files before pushing to the
remote repo
include scripts/build/license.mk
-license-check: $(LICENSE_EYE)
-license-check: TARGET=license-check
-license-check: PROJECTS:=ui mcp canopy
-license-check: default ## Check license header
+# License-check / license-fix run a SINGLE license-eye invocation from the
+# repo root with the root .licenserc.yaml. This avoids:
+# - editing ui/.licenserc.yaml (forbidden by plan §Principle 3),
+# - the per-subdir loop over PROJECTS (each subdir would otherwise load
+# its own .licenserc.yaml and miss the root config's OMC-runtime-state
+# / handoff-import / playwright-mcp exclusions).
+# The root config already includes 'ui' in paths-ignore so the Vue app is
+# not double-scanned; canopy files are scanned from the root, which is the
+# desired surface for the license header check.
+license-check: $(LICENSE_EYE) ## Check license header
$(LICENSE_EYE) header check
-license-fix: $(LICENSE_EYE)
-license-fix: TARGET=license-fix
-license-fix: PROJECTS:=ui mcp canopy
-license-fix: default ## Fix license header issues
+license-fix: $(LICENSE_EYE) ## Fix license header issues
+ $(LICENSE_EYE) header fix
$(LICENSE_EYE) header fix
Review Comment:
`license-fix` runs `license-eye header fix` twice, which makes the target do
redundant work and can mask whether the command is actually idempotent. It
should be a single invocation (matching the comment above).
##########
canopy/server/src/plugins/static.ts:
##########
@@ -35,6 +35,22 @@ export async function registerStatic(app: FastifyInstance):
Promise<void> {
await app.register(fastifyStatic, {
root: WEB_DIST,
prefix: '/',
+ // SPA assets must always revalidate. Without these headers the browser
+ // caches the index.html + the hash-named bundle, so a `npm run -w web
build`
+ // that swaps the bundle filename does NOT clear the browser's view of the
+ // page — the cached HTML still points at the old hash. Sending
+ // no-cache on the HTML and a short max-age on the assets keeps the dev
+ // loop tight while still letting the browser avoid refetching on every
+ // request within a session.
Review Comment:
The comment says “no-cache on the HTML and a short max-age on the assets”,
but the code sets `no-store` for `index.html` and `no-cache` for assets (no
`max-age`). Please align the comment with the actual headers (or adjust headers
to match the comment).
##########
canopy/web/src/query/results/ResultError.tsx:
##########
@@ -0,0 +1,127 @@
+/*
+ * Licensed to 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. Apache Software Foundation (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.
+ */
+
+// ResultError.tsx — friendly error state for the query result pane.
+// Turns terse BFF/DB messages ("Upstream returned 500") into a structured
+// explanation with an actionable hint and a retry button.
+
+import React, { useState } from 'react';
+import { IconAlert, IconChevron } from '../../components/icons.js';
+
+interface ResultErrorProps {
+ readonly message: string;
+ readonly onRetry?: () => void;
+}
+
+type ErrorKind = 'server' | 'query' | 'auth' | 'network' | 'timeRange' |
'empty' | 'unknown';
+
+interface ErrorMeta {
+ readonly title: string;
+ readonly hint: string;
+}
+
+function classify(message: string): ErrorKind {
+ const m = message.toLowerCase();
+ if (m.includes('empty query')) return 'empty';
+ if (m.includes('to must be later than from')) return 'timeRange';
+ if (m.includes('401') || m.includes('403') || m.includes('unauthorized') ||
m.includes('forbidden') || m.includes('unauthenticated')) return 'auth';
+ if (m.includes('fetch') || m.includes('network') || m.includes('failed to
fetch') || m.includes('connection') || m.includes('ENOTFOUND')) return
'network';
Review Comment:
`classify()` lowercases the message into `m`, but then checks for
`'ENOTFOUND'` (uppercase). That branch can never match, so DNS errors won’t be
classified as `network`.
##########
canopy/web/src/data/fixtures/query/manifest.json:
##########
@@ -0,0 +1,12 @@
+{
+ "description": "Manifest for the query/ fixtures set. The four JSON files in
this directory are HAND-AUTHORED to match the BanyanDB 0.9.x wire shape (see
implement-m4-note.md decision #24 — live capture requires the M0 seed harness,
which is out of this session's scope). Wire shape is pinned per
banyandbVersion; when BanyanDB upstream evolves, re-run a live capture (planned
M5 helper, see `npm run -w web capture:query-fixtures` after the harness lands)
and diff against these committed files; a CI guard would warn/fail on shape
drift.",
+ "capturedAt": "2026-06-29T12:00:00Z",
+ "banyandbVersion": "0.9.x",
+ "authorship": "hand-authored (NOT live-captured)",
Review Comment:
The manifest says these fixtures match BanyanDB `0.9.x` and sets
`banyandbVersion: "0.9.x"`, but the fixtures themselves (and fixtures.test.ts)
are version-stamped `"monorepo-current"`. This inconsistency makes it unclear
what wire shape the set is intended to represent.
##########
canopy/e2e/framework/seed/factory.ts:
##########
@@ -0,0 +1,206 @@
+/*
+ * Licensed to 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. Apache Software Foundation (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.
+ */
+
+// SeedFactory — programmatic, per-test data seeding via the BFF HTTP API,
+// following the design doc's "Direct API Triggering (cy.request())" strategy:
+// seed preconditions over HTTP instead of driving the browser GUI. Two
+// properties keep it deterministic and isolated:
+//
+// 1. DYNAMIC NAMES. Every resource is named with `uniqueName(prefix)`, which
+// appends Date.now(), so parallel workers and re-runs never collide on a
+// unique constraint and never inherit a prior run's rows. This is the
+// programmatic-factory answer to schema drift + shared mutable state.
+// 2. CLEANUP. Everything created is tracked and torn down in `cleanup()`,
+// wired to the `seed` fixture's teardown so each test leaves no
footprint.
+// Resources are removed before their groups (BanyanDB rejects deleting a
+// group that still owns resources) and in reverse creation order.
+//
+// SCOPE NOTE: BanyanDB accepts *schema* writes over HTTP (group / measure /
+// stream / trace / index-rule registries) but *data* writes only over
streaming
+// gRPC. This factory therefore covers schema-level seeding end-to-end; bulk
demo
+// DATA is seeded once per run by cmd/m4-seed (see setup/global-setup.ts and
+// `seedDemoData()` below). A test-only HTTP data-seed endpoint remains the
+// tracked follow-up (TESTING.md §8) that would let this factory seed rows
+// directly too — it is deferred because the Fastify BFF has no gRPC client and
+// BanyanDB writes are gRPC-only.
+
+import { spawnSync } from 'node:child_process';
+import { resolve } from 'node:path';
+import type { APIRequestContext } from '@playwright/test';
+
+export type Catalog = 'CATALOG_MEASURE' | 'CATALOG_STREAM' | 'CATALOG_TRACE' |
'CATALOG_PROPERTY';
+export type IndexRuleType = 'TYPE_TREE' | 'TYPE_INVERTED';
+
+// A schema resource scheduled for teardown; the DELETE path is stored
verbatim.
+interface TrackedResource {
+ readonly path: string;
+}
+
+export class SeedFactory {
+ private readonly createdGroups: string[] = [];
+ private readonly createdResources: TrackedResource[] = [];
+
+ constructor(private readonly request: APIRequestContext) {}
+
+ // Append a monotonic suffix so names are unique across workers and re-runs.
+ uniqueName(prefix: string): string {
+ return `${prefix}-${Date.now()}`;
+ }
Review Comment:
`uniqueName()` uses only `Date.now()` for uniqueness. Multiple calls within
the same millisecond (which is realistic in a tight async sequence) can collide
and make the E2E suite flaky. Consider adding a per-process counter suffix.
##########
canopy/web/src/query/TraceDecoderModal.tsx:
##########
@@ -0,0 +1,221 @@
+/*
+ * Licensed to 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. Apache Software Foundation (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.
+ */
+
+// TraceDecoderModal.tsx — proto binding for trace span bytes.
+// Matches the handoff "Span bytes decoder" modal: drop/select a .proto file,
+// parse it locally, preview the root message's fields, then bind it.
+
+import React, { useCallback, useEffect, useRef, useState } from 'react';
+import { tdParseProto, tdGetBinding, tdSetBinding, tdClearBinding, type
TDBinding, type TDField, type TDParseResult } from './proto-decoder.js';
+
+interface Props {
+ readonly traceId: string;
+ readonly onClose: () => void;
+ readonly onChange?: (binding: TDBinding | null) => void;
+}
+
+interface ParsedFile {
+ readonly name: string;
+ readonly src: string;
+ readonly parsed: TDParseResult;
+}
+
+export function TraceDecoderModal({ traceId, onClose, onChange }: Props) {
+ const existing = tdGetBinding(traceId);
+ const [pending, setPending] = useState<ParsedFile | null>(null);
+ const [error, setError] = useState<string>('');
+ const [dragOver, setDragOver] = useState(false);
+ const inputRef = useRef<HTMLInputElement | null>(null);
+
+ useEffect(() => {
+ setPending(null);
+ setError('');
+ }, [traceId]);
+
+ const parseFile = useCallback(async (file: File) => {
+ if (!/\.proto$/i.test(file.name)) {
+ setError('Please choose a .proto file.');
+ setPending(null);
+ return;
+ }
+ const src = await file.text();
+ const parsed = tdParseProto(src);
+ if (!parsed.primary) {
+ setError('No message definitions found in this .proto file.');
+ setPending(null);
+ return;
+ }
+ setError('');
+ setPending({ name: file.name, src, parsed });
+ }, []);
+
+ const handleDrop = useCallback((e: React.DragEvent<HTMLDivElement>) => {
+ e.preventDefault();
+ setDragOver(false);
+ const file = e.dataTransfer.files?.[0];
+ if (file) parseFile(file);
+ }, [parseFile]);
+
+ const handleDragOver = useCallback((e: React.DragEvent<HTMLDivElement>) => {
+ e.preventDefault();
+ setDragOver(true);
+ }, []);
+
+ const handleDragLeave = useCallback((e: React.DragEvent<HTMLDivElement>) => {
+ e.preventDefault();
+ setDragOver(false);
+ }, []);
+
+ const bind = useCallback(() => {
+ if (!pending) return;
+ const next = tdSetBinding(traceId, pending.name, pending.src,
pending.parsed);
+ onChange?.(next);
+ onClose();
+ }, [pending, traceId, onChange, onClose]);
+
+ const unbind = useCallback(() => {
+ tdClearBinding(traceId);
+ setPending(null);
+ onChange?.(null);
+ onClose();
+ }, [traceId, onChange, onClose]);
+
+ const pendingBinding = pending ? {
+ traceId,
+ fileName: pending.name,
+ primary: pending.parsed.primary,
+ messages: pending.parsed.messages,
+ order: pending.parsed.order,
+ count: pending.parsed.count,
+ protoSrc: pending.src,
+ boundAt: Date.now(),
+ } : null;
+ const active = pendingBinding ?? existing;
+ const activePrimary = active?.primary ?? '';
+ const activeCount = active?.count ?? 0;
+ const activeName = active?.fileName ?? '';
+
+ return (
+ <div className="modal-overlay" role="dialog" aria-modal="true"
aria-labelledby="tdm-title" onClick={onClose}>
+ <div className="modal is-wide" onClick={(e) => e.stopPropagation()}>
+ <div className="modal-head">
+ <div>
+ <h2 id="tdm-title" className="modal-title">Span bytes decoder</h2>
+ <p className="modal-sub">Bind a protobuf schema to {traceId ||
'this trace'} to decode each span's opaque bytes.</p>
+ </div>
+ <button type="button" className="modal-x" onClick={onClose}
aria-label="Close" />
+ </div>
+
+ <div className="modal-body">
+ <div className="td-modal">
+ <div
+ className={'tdm-drop' + (dragOver ? ' is-dragover' : '') +
(active ? ' has-file' : '')}
+ onDrop={handleDrop}
+ onDragOver={handleDragOver}
+ onDragLeave={handleDragLeave}
+ onClick={() => inputRef.current?.click()}
+ role="button"
+ tabIndex={0}
+ aria-label="Upload a .proto file"
+ >
Review Comment:
The drop zone is exposed as `role="button"` with `tabIndex={0}`, but it has
no keyboard handler. As-is, keyboard users can focus it but cannot activate it
to open the file picker (Enter/Space).
--
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]
