Skip to content

Conversation

@ultmaster
Copy link
Contributor

and a lot of bug fixes...

Copilot AI review requested due to automatic review settings November 11, 2025 06:31
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR integrates the dashboard as a built asset into the Python package, enabling the Agent-Lightning server to serve the web UI directly. Key changes include updating the build pipeline, fixing bugs in both frontend and backend code, and improving test coverage with Storybook integration.

Key Changes

  • Dashboard assets are now built and packaged with the Python distribution
  • API routes restructured with a dedicated router and SPA fallback handling
  • Enhanced frontend functionality with query parameter support, improved search, and better responsive layouts
  • Added comprehensive Storybook tests and vitest browser testing setup

Reviewed Changes

Copilot reviewed 28 out of 31 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
agentlightning/store/client_server.py Added dashboard static file serving, API router refactor, and SPA fallback routes
pyproject.toml Configured package to include dashboard HTML/JS/CSS/SVG assets
dashboard/vite.config.mjs Updated build output directory and added vitest browser testing support
dashboard/src/pages/Traces.page.tsx Implemented query parameter hydration, rollout search, and improved state management
dashboard/src/utils/table.ts Fixed responsive column calculations with dynamic EM-to-pixel conversion
dashboard/src/components/AppDrawer.component.tsx Added navigation link to traces page with query params
dashboard/src/features/rollouts/api.ts Fixed span normalization to preserve raw attribute structure
.github/workflows/*.yml Added Node.js setup and dashboard build steps to CI/CD pipelines
Files not reviewed (1)
  • dashboard/package-lock.json: Language not supported

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +13 to 17
const dirname = typeof __dirname !== 'undefined' ? __dirname : path.dirname(fileURLToPath(import.meta.url));

// More info at: https://storybook.js.org/docs/next/writing-tests/integrations/vitest-addon
const __filename = fileURLToPath(import.meta.url);
const __dirname = path.dirname(__filename);
Copy link

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicate variable declarations for __dirname and __filename. Line 13 defines dirname with a fallback, but lines 16-17 redefine __filename and __dirname unconditionally. Consider removing lines 16-17 and renaming dirname to __dirname on line 13, or if the fallback is needed for specific environments, remove the redundant declarations.

Copilot uses AI. Check for mistakes.
attributes: (value as any).attributes ?? {},
context: (value as any).context ?? {},
parent: (value as any).parent ?? null,
resource: (value as any).resource ?? {},
Copy link

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bypassing the camelCase conversion by casting value as any to preserve the original structure. While this may be intentional to maintain the raw format of these fields, it creates inconsistency with the rest of the normalized object which uses camelized. Consider documenting why these specific fields need to preserve their original casing, or ensure the backend consistently uses the expected case.

Suggested change
resource: (value as any).resource ?? {},
resource: camelCaseKeys((value as any).resource ?? {}),

Copilot uses AI. Check for mistakes.
return FileResponse(index_file)

@self.app.get("/{full_path:path}", include_in_schema=False)
def spa_fallback(full_path: str): # pyright: ignore[reportUnusedFunction]
Copy link

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The SPA fallback route /{full_path:path} could potentially intercept API requests if they don't exactly match the registered routes. FastAPI route ordering matters - this catch-all route is registered after the API router on line 799, but if an API route has a typo or slight variation, it would incorrectly serve the index.html. Consider adding a guard condition to check if full_path starts with 'v1' and return a 404 instead of serving the SPA.

Suggested change
def spa_fallback(full_path: str): # pyright: ignore[reportUnusedFunction]
def spa_fallback(full_path: str): # pyright: ignore[reportUnusedFunction]
# If the path looks like an API route, return 404
if full_path.startswith("v1"):
raise HTTPException(status_code=404, detail="Not Found")

Copilot uses AI. Check for mistakes.
Comment on lines 10 to +15
{ label: 'Off', value: '0' },
{ label: 'Every 5 seconds', value: '5000' },
{ label: 'Every 15 seconds', value: '15000' },
{ label: 'Every 60 seconds', value: '60000' },
{ label: 'Every 5 minutes', value: '300000' },
// TODO: Support real auto-refresh
{ label: 'Every 5 seconds', value: '5000', disabled: true },
{ label: 'Every 15 seconds', value: '15000', disabled: true },
{ label: 'Every 60 seconds', value: '60000', disabled: true },
{ label: 'Every 5 minutes', value: '300000', disabled: true },
Copy link

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Auto-refresh options are disabled with a TODO comment in the code, but users won't see any explanation in the UI. Consider adding a helper text or tooltip to inform users that auto-refresh functionality is coming soon, or remove these disabled options entirely until the feature is implemented.

Copilot uses AI. Check for mistakes.

check();

// FIXME: autorefresh only refresh server status, not the data
Copy link

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] FIXME comment indicates auto-refresh only updates server status, not the actual data. This is related to the disabled auto-refresh options in Settings.page.tsx. Consider filing a tracking issue for this limitation and referencing it in both comments for better traceability.

Suggested change
// FIXME: autorefresh only refresh server status, not the data
// FIXME: autorefresh only refreshes server status, not the actual data.
// See tracking issue: https://github.com/your-org/your-repo/issues/1234
// This is related to the disabled auto-refresh options in Settings.page.tsx.

Copilot uses AI. Check for mistakes.
import { defineConfig } from 'vite';
import tsconfigPaths from 'vite-tsconfig-paths';

const dirname = typeof __dirname !== 'undefined' ? __dirname : path.dirname(fileURLToPath(import.meta.url));
Copy link

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Variable '__dirname' is used before its declaration.

Copilot uses AI. Check for mistakes.
@ultmaster
Copy link
Contributor Author

/ci

@github-actions
Copy link

github-actions bot commented Nov 11, 2025

🚀 CI Watcher for correlation id-3515208606-mhu7a10c triggered by comment 3515208606
🏃‍♀️ Tracking 6 workflow run(s):

✅ All runs completed.

@ultmaster ultmaster merged commit 2ab977e into main Nov 11, 2025
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants