Conversation
There was a problem hiding this comment.
Pull request overview
This pull request implements a comprehensive inspector configuration system for data sources, including default inspector settings that can be configured per data source and applied when adding them to maps. The changes include database schema updates, new UI components for configuring inspector panels, and enhancements to the inspector display functionality.
Changes:
- Added database schema fields for storing default inspector configurations per data source (defaultInspectorConfig, defaultInspectorConfigUpdatedAt)
- Implemented a new inspector settings modal with drag-and-drop column ordering, label dividers, custom formatting (percentage bars, scale bars), and layout options
- Added data source management UI for configuring default inspector settings with live preview
- Enhanced inspector panel displays with custom icons, colors, column metadata, and multi-column grid layouts
Reviewed changes
Copilot reviewed 44 out of 44 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| migrations/*.ts | Database migrations for new inspector config fields and geometry type fix |
| src/server/services/database/schema.ts | Added defaultInspectorConfig and defaultInspectorConfigUpdatedAt fields to DataSource table |
| src/server/trpc/routers/dataSource.ts | Added isOwner field to listReadable response and updateConfig mutation support |
| src/server/models/MapView.ts | Extended inspector config schema with column metadata, formatting, layouts, and default config |
| src/server/models/DataSource.ts | Added default inspector config fields to data source schema |
| src/app/map/[id]/hooks/*.ts | New debounced hooks and improved view state management with getLatestView |
| src/app/map/[id]/components/inspector/* | Major refactor of inspector components with settings modal, column ordering, and enhanced displays |
| src/app/map/[id]/components/TogglePanel.tsx | Updated default expanded state and added wrapper className support |
| src/app/(private)/data-sources/[id]/* | Added inspector configuration section to data source management page |
| src/components/ColorPalette.tsx | Exported COLOR_PALETTE_DATA for reuse in inspector settings |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/app/map/[id]/components/inspector/InspectorSettingsModal/SortableDividerRow.tsx
Outdated
Show resolved
Hide resolved
src/app/map/[id]/components/inspector/InspectorSettingsModal/ColumnsSection.tsx
Outdated
Show resolved
Hide resolved
src/app/map/[id]/components/inspector/InspectorSettingsModal/ColumnsSection.tsx
Outdated
Show resolved
Hide resolved
| void _unusedId; | ||
| void _unusedDsId; |
There was a problem hiding this comment.
The void statements on lines 44-45 are unnecessary. When destructuring with rest properties, the unused variables are already clearly indicated by the underscore prefix (_unusedId, _unusedDsId). The void statements don't add value and can be removed. This pattern is typically used to silence unused variable warnings, but modern TypeScript and ESLint configurations handle this with the underscore prefix convention.
| void _unusedId; | |
| void _unusedDsId; |
| ...(input.defaultInspectorConfig !== undefined && { | ||
| defaultInspectorConfigUpdatedAt: new Date(), | ||
| }), |
There was a problem hiding this comment.
The conditional spread operator on lines 247-249 may cause issues when input.defaultInspectorConfig is explicitly set to null or undefined. When input.defaultInspectorConfig is null, the condition input.defaultInspectorConfig !== undefined evaluates to true, causing defaultInspectorConfigUpdatedAt to be set to a new Date, even though the config was cleared. Consider using a more explicit check: input.defaultInspectorConfig !== undefined && input.defaultInspectorConfig !== null to only update the timestamp when a config is actually being set, not when it's being cleared.
…ortableDividerRow.tsx Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…olumnsSection.tsx Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…olumnsSection.tsx Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 44 out of 44 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import { sql } from "kysely"; | ||
| import type { Kysely } from "kysely"; | ||
|
|
||
| export async function up(db: Kysely<any>): Promise<void> { | ||
| await sql` | ||
| ALTER TABLE "data_source" | ||
| ADD COLUMN "default_inspector_config" jsonb DEFAULT NULL | ||
| `.execute(db); | ||
| } | ||
|
|
||
| export async function down(db: Kysely<any>): Promise<void> { | ||
| await sql` | ||
| ALTER TABLE "data_source" | ||
| DROP COLUMN "default_inspector_config" | ||
| `.execute(db); |
There was a problem hiding this comment.
Migration style inconsistency: Migration 1772020000000 uses raw SQL with snake_case table name, while migration 1772030000000 uses Kysely schema builder with camelCase. For consistency and to match the pattern in migration 1770912056401_data_source_record_count.ts (which adds columns using schema builder then updates with raw SQL), consider using Kysely's schema builder for both or keeping the same style. This is a minor inconsistency but could confuse future developers.
| import { sql } from "kysely"; | |
| import type { Kysely } from "kysely"; | |
| export async function up(db: Kysely<any>): Promise<void> { | |
| await sql` | |
| ALTER TABLE "data_source" | |
| ADD COLUMN "default_inspector_config" jsonb DEFAULT NULL | |
| `.execute(db); | |
| } | |
| export async function down(db: Kysely<any>): Promise<void> { | |
| await sql` | |
| ALTER TABLE "data_source" | |
| DROP COLUMN "default_inspector_config" | |
| `.execute(db); | |
| import type { Kysely } from "kysely"; | |
| export async function up(db: Kysely<any>): Promise<void> { | |
| await db.schema | |
| .alterTable("dataSource") | |
| .addColumn("defaultInspectorConfig", "jsonb", (col) => col.defaultTo(null)) | |
| .execute(); | |
| } | |
| export async function down(db: Kysely<any>): Promise<void> { | |
| await db.schema | |
| .alterTable("dataSource") | |
| .dropColumn("defaultInspectorConfig") | |
| .execute(); |
| <ChevronDown | ||
| size={16} | ||
| className={cn( | ||
| "transition-transform", | ||
| "transition group-hover:opacity-100 opacity-0 ", | ||
| expanded ? "rotate-0" : "-rotate-90", | ||
| )} | ||
| /> |
There was a problem hiding this comment.
Accessibility issue: The chevron icon has opacity-0 by default and only shows on group-hover:opacity-100. This creates confusion for keyboard users and screen reader users who can't hover. The chevron should always be visible or have proper keyboard focus states. Consider using opacity-70 group-hover:opacity-100 instead or ensuring the button element itself has proper focus-visible styles.
Description
Motivation and Context
How Can It Be Tested?
How Will This Be Deployed?
Screenshots (if appropriate):
Types of changes
Checklist: