Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 3 additions & 13 deletions app/e2e/connections.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -187,22 +187,12 @@

// Click the card — should expand an alert below it with the error message
await card.click();
await expect(
page
.locator('[role="alert"]')
.filter({ hasText: /refused|ECONNREFUSED|failed|error/i })
.first(),
).toBeVisible({
timeout: 5_000,
});
const expandedAlert = page.locator('[role="alert"]').last();
await expect(expandedAlert).toBeVisible({ timeout: 5_000 });
Comment on lines +190 to +191

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🌐 Web query:

In Playwright, does locator.last() resolve dynamically on each assertion/action, and can it point to a different element after DOM updates?

💡 Result:

Yes, in Playwright, locator.last resolves dynamically on each assertion or action. It re-evaluates the locator query against the current DOM state every time it is used, so it can point to a different element if the DOM updates (such as elements being added, removed, or reordered) between uses. This behavior is core to how all Playwright locators work: they store a query rather than a fixed element reference, ensuring resilience to DOM changes. Official documentation states: "Every time a locator is used for an action, an up-to-date DOM element is located in the page." For example: const locator = page.locator('ul li'); await locator.last.click; // Finds current last

  • await page.waitForTimeout(1000); // DOM changes await locator.last.click; // Finds new last
  • , potentially different The .last method creates a new locator pointing to the last matching element from the parent locator's current matches at resolution time. Methods like .first, .last, and .nth are used to opt out of strictness when multiple elements match, but they inherit the dynamic re-querying. Note that .all does not wait and can be flaky with dynamic lists, but actions/assertions on .last include auto-waiting and re-resolution.

    Citations:


    🏁 Script executed:

    cat -n app/e2e/connections.spec.ts | sed -n '185,200p'

    Repository: alfredo1996/neoboard

    Length of output: 699


    🏁 Script executed:

    cat -n app/e2e/connections.spec.ts | sed -n '170,210p'

    Repository: alfredo1996/neoboard

    Length of output: 2072


  • Scope the alert locator to the clicked card; global .last() is brittle in e2e tests.

    On Line 190, page.locator('[role="alert"]').last() re-evaluates dynamically. In e2e tests where multiple cards exist, this can target the wrong alert after DOM changes or when other test cases add alerts to the page. Since card is already scoped on line 180, use it to isolate the alert:

    Suggested fix
    -    const expandedAlert = page.locator('[role="alert"]').last();
    +    const expandedAlert = card
    +      .locator('[role="alert"]')
    +      .filter({ hasText: /refused|ECONNREFUSED|failed|error/i })
    +      .first();
         await expect(expandedAlert).toBeVisible({ timeout: 5_000 });
    
         // Click again to collapse
         await card.click();
    -    await expect(expandedAlert).not.toBeVisible();
    +    await expect(expandedAlert).not.toBeVisible({ timeout: 5_000 });
    🤖 Prompt for AI Agents
    Verify each finding against the current code and only fix it if needed.
    
    In `@app/e2e/connections.spec.ts` around lines 190 - 191, The alert locator is too
    broad—don't use page.locator(...).last() because it can pick up alerts from
    other cards; instead scope the alert to the clicked card by using the existing
    card locator (e.g., replace page.locator('[role="alert"]').last() with
    card.locator('[role="alert"]') or card.locator('role=alert')) and keep the await
    expect(...).toBeVisible({ timeout: 5_000 }) call to assert visibility.
    


    // Click again to collapse
    await card.click();
    await expect(
    page
    .locator('[role="alert"]')
    .filter({ hasText: /refused|ECONNREFUSED|failed|error/i }),
    ).not.toBeVisible();
    await expect(expandedAlert).not.toBeVisible();

    Check failure on line 195 in app/e2e/connections.spec.ts

    View workflow job for this annotation

    GitHub Actions / E2E (shard 2/5)

    [chromium] › e2e/connections.spec.ts:164:7 › Connections › clicking an error card shows error details inline

    1) [chromium] › e2e/connections.spec.ts:164:7 › Connections › clicking an error card shows error details inline Retry #1 ─────────────────────────────────────────────────────────────────────────────────────── Error: expect(locator).not.toBeVisible() failed Locator: locator('[role="alert"]').last() Expected: not visible Received: visible Timeout: 5000ms Call log: - Expect "not toBeVisible" with timeout 5000ms - waiting for locator('[role="alert"]').last() 9 × locator resolved to <div role="alert" aria-live="assertive" id="__next-route-announcer__"></div> - unexpected value "visible" 193 | // Click again to collapse 194 | await card.click(); > 195 | await expect(expandedAlert).not.toBeVisible(); | ^ 196 | }); 197 | 198 | test("should delete a connection with confirmation", async ({ page }) => { at /home/runner/work/neoboard/neoboard/app/e2e/connections.spec.ts:195:37

    Check failure on line 195 in app/e2e/connections.spec.ts

    View workflow job for this annotation

    GitHub Actions / E2E (shard 2/5)

    [chromium] › e2e/connections.spec.ts:164:7 › Connections › clicking an error card shows error details inline

    1) [chromium] › e2e/connections.spec.ts:164:7 › Connections › clicking an error card shows error details inline Error: expect(locator).not.toBeVisible() failed Locator: locator('[role="alert"]').last() Expected: not visible Received: visible Timeout: 5000ms Call log: - Expect "not toBeVisible" with timeout 5000ms - waiting for locator('[role="alert"]').last() 9 × locator resolved to <div role="alert" aria-live="assertive" id="__next-route-announcer__"></div> - unexpected value "visible" 193 | // Click again to collapse 194 | await card.click(); > 195 | await expect(expandedAlert).not.toBeVisible(); | ^ 196 | }); 197 | 198 | test("should delete a connection with confirmation", async ({ page }) => { at /home/runner/work/neoboard/neoboard/app/e2e/connections.spec.ts:195:37
    });

    test("should delete a connection with confirmation", async ({ page }) => {
    Expand Down
    212 changes: 212 additions & 0 deletions app/src/components/__tests__/card-container.test.tsx
    Original file line number Diff line number Diff line change
    @@ -0,0 +1,212 @@
    /**
    * CardContainer tests — focused on the widgetIdSuffix prop that prevents
    * graph store conflicts when two CardContainers render the same widget
    * (e.g. normal view + fullscreen dialog).
    */
    import { describe, it, expect, vi, beforeEach } from "vitest";
    import { render, screen } from "@testing-library/react";
    import { QueryClient, QueryClientProvider } from "@tanstack/react-query";
    import type { DashboardWidget } from "@/lib/db/schema";

    // ── Capture ChartRenderer props to verify effectiveWidgetId ───────────
    let capturedChartProps: Record<string, unknown> = {};

    vi.mock("@/components/chart-renderer", () => ({
    ChartRenderer: (props: Record<string, unknown>) => {
    capturedChartProps = props;
    return <div data-testid="chart-renderer" />;
    },
    }));

    vi.mock("@/hooks/use-widget-query", () => ({
    useWidgetQuery: () => ({
    isPending: false,
    isError: false,
    data: null,
    fetchStatus: "idle",
    missingParams: [],
    }),
    }));

    vi.mock("@/hooks/use-click-action", () => ({
    useClickAction: () => ({
    handleChartClick: undefined,
    hasClickAction: false,
    clickableColumns: [],
    }),
    }));

    vi.mock("@/stores/parameter-store", () => ({
    useParameterValues: () => ({}),
    }));

    vi.mock("@/lib/chart-registry", () => ({
    getChartConfig: (type: string) => {
    if (type === "bar" || type === "markdown") {
    return {
    type,
    label: type,
    transform: (d: unknown) => d,
    transformWithMapping: (d: unknown) => d,
    supportsColumnMapping: false,
    validate: () => null,
    };
    }
    return null;
    },
    }));

    vi.mock("@/lib/resolve-cache-options", () => ({
    resolveCacheOptions: () => ({ staleTime: 0, gcTime: 0 }),
    }));

    vi.mock("@/lib/scroll-to-widget", () => ({
    scrollAndHighlight: () => false,
    }));

    vi.mock("@neoboard/components", () => ({
    Skeleton: () => <div data-testid="skeleton" />,
    Alert: ({ children }: { children: React.ReactNode }) => <div>{children}</div>,
    AlertDescription: ({ children }: { children: React.ReactNode }) => (
    <div>{children}</div>
    ),
    AlertTitle: ({ children }: { children: React.ReactNode }) => (
    <div>{children}</div>
    ),
    Button: ({
    children,
    onClick,
    }: {
    children: React.ReactNode;
    onClick?: () => void;
    }) => <button onClick={onClick}>{children}</button>,
    EmptyState: ({
    title,
    description,
    }: {
    title: string;
    description?: string;
    }) => (
    <div data-testid="empty-state">
    <span>{title}</span>
    {description && <span>{description}</span>}
    </div>
    ),
    ColumnMappingOverlay: () => null,
    substituteParams: (s: string) => s,
    Popover: ({ children }: { children: React.ReactNode }) => (
    <div>{children}</div>
    ),
    PopoverTrigger: ({ children }: { children: React.ReactNode }) => (
    <div>{children}</div>
    ),
    PopoverContent: ({ children }: { children: React.ReactNode }) => (
    <div>{children}</div>
    ),
    }));

    vi.mock("@/lib/data-transforms", () => ({
    applyTransforms: (data: unknown) => data,
    }));

    vi.mock("@/lib/card-utils", () => ({
    extractColumnNames: () => [],
    resolveStylingConfig: () => undefined,
    }));

    // Import after mocks
    import { CardContainer } from "../card-container";

    function createWidget(overrides?: Partial<DashboardWidget>): DashboardWidget {
    return {
    id: "widget-123",
    chartType: "markdown",
    connectionId: "conn-1",
    query: "",
    settings: {
    chartOptions: { content: "hello" },
    },
    ...overrides,
    };
    }

    function renderWithProviders(ui: React.ReactElement) {
    const queryClient = new QueryClient({
    defaultOptions: { queries: { retry: false } },
    });
    return render(
    <QueryClientProvider client={queryClient}>{ui}</QueryClientProvider>,
    );
    }

    describe("CardContainer", () => {
    beforeEach(() => {
    capturedChartProps = {};
    vi.clearAllMocks();
    });

    describe("widgetIdSuffix prop", () => {
    it("passes widget.id as widgetId in meta when widgetIdSuffix is not provided", () => {
    const widget = createWidget();
    renderWithProviders(<CardContainer widget={widget} />);

    const meta = capturedChartProps.meta as { widgetId?: string };
    expect(meta?.widgetId).toBe("widget-123");
    });

    it("appends suffix to widgetId when widgetIdSuffix is provided", () => {
    const widget = createWidget();
    renderWithProviders(
    <CardContainer widget={widget} widgetIdSuffix="fullscreen" />,
    );

    const meta = capturedChartProps.meta as { widgetId?: string };
    expect(meta?.widgetId).toBe("widget-123--fullscreen");
    });

    it("uses double-dash separator between widget id and suffix", () => {
    const widget = createWidget({ id: "w-99" });
    renderWithProviders(
    <CardContainer widget={widget} widgetIdSuffix="preview" />,
    );

    const meta = capturedChartProps.meta as { widgetId?: string };
    expect(meta?.widgetId).toBe("w-99--preview");
    });

    it("passes original widget.id when widgetIdSuffix is empty string", () => {
    // Empty string is falsy, so effectiveWidgetId should be widget.id
    const widget = createWidget();
    renderWithProviders(<CardContainer widget={widget} widgetIdSuffix="" />);

    const meta = capturedChartProps.meta as { widgetId?: string };
    expect(meta?.widgetId).toBe("widget-123");
    });
    });

    describe("preview data path with widgetIdSuffix", () => {
    it("passes effectiveWidgetId through meta when rendering with previewData", () => {
    const widget = createWidget({ chartType: "bar" });
    const previewData = [{ name: "A", value: 1 }];
    renderWithProviders(
    <CardContainer
    widget={widget}
    previewData={previewData}
    widgetIdSuffix="fullscreen"
    />,
    );

    const meta = capturedChartProps.meta as { widgetId?: string };
    expect(meta?.widgetId).toBe("widget-123--fullscreen");
    });
    });

    describe("unknown chart type", () => {
    it("shows empty state for unknown chart types", () => {
    const widget = createWidget({ chartType: "nonexistent" });
    renderWithProviders(<CardContainer widget={widget} />);

    expect(screen.getByText("Unknown chart type")).toBeInTheDocument();
    });
    });
    });
    21 changes: 16 additions & 5 deletions app/src/components/card-container.tsx
    Original file line number Diff line number Diff line change
    Expand Up @@ -61,6 +61,10 @@ interface CardContainerProps {
    onNavigateToPage?: (pageId: string, scrollToWidgetId?: string) => void;
    /** When true, graph widgets trigger a fit-to-viewport after mount. */
    autoFit?: boolean;
    /** Optional suffix appended to the widget ID used for graph store keys.
    * Prevents store conflicts when two CardContainers render the same widget
    * (e.g. normal view + fullscreen dialog). */
    widgetIdSuffix?: string;
    /** Maps parameter names to the widgets that set them (for clickable badges). */
    parameterSourceMap?: ParameterSourceMap;
    }
    Expand Down Expand Up @@ -152,8 +156,12 @@ export function CardContainer({
    refetchInterval,
    onNavigateToPage,
    autoFit,
    widgetIdSuffix,
    parameterSourceMap,
    }: CardContainerProps) {
    const effectiveWidgetId = widgetIdSuffix
    ? `${widget.id}--${widgetIdSuffix}`
    : widget.id;
    const chartConfig = getChartConfig(widget.chartType);
    const { handleChartClick, hasClickAction, clickableColumns } = useClickAction(
    widget,
    Expand Down Expand Up @@ -321,7 +329,7 @@ export function CardContainer({
    }
    meta={{
    connectionId: widget.connectionId,
    widgetId: widget.id,
    widgetId: effectiveWidgetId,
    resultId: previewResultId,
    autoFit,
    }}
    Expand All @@ -348,7 +356,10 @@ export function CardContainer({
    type={chartConfig.type}
    data={null}
    settings={chartOptions}
    meta={{ connectionId: widget.connectionId, widgetId: widget.id }}
    meta={{
    connectionId: widget.connectionId,
    widgetId: effectiveWidgetId,
    }}
    />
    </div>
    </div>
    Expand All @@ -366,7 +377,7 @@ export function CardContainer({
    settings={widget.settings as Record<string, unknown>}
    meta={{
    connectionId: widget.connectionId,
    widgetId: widget.id,
    widgetId: effectiveWidgetId,
    query: widget.query,
    }}
    />
    Expand Down Expand Up @@ -399,7 +410,7 @@ export function CardContainer({
    type={chartConfig.type}
    data={null}
    settings={resolvedContentOptions}
    meta={{ widgetId: widget.id }}
    meta={{ widgetId: effectiveWidgetId }}
    />
    </div>
    </div>
    Expand Down Expand Up @@ -549,7 +560,7 @@ export function CardContainer({
    }
    meta={{
    connectionId: widget.connectionId,
    widgetId: widget.id,
    widgetId: effectiveWidgetId,
    resultId: widgetQuery.data.resultId,
    autoFit,
    }}
    Expand Down
    1 change: 1 addition & 0 deletions app/src/components/dashboard-container.tsx
    Original file line number Diff line number Diff line change
    Expand Up @@ -358,6 +358,7 @@ export function DashboardContainer({
    onNavigateToPage={onNavigateToPage}
    parameterSourceMap={parameterSourceMap}
    autoFit
    widgetIdSuffix="fullscreen"
    />
    ) : (
    <div className="flex h-full items-center justify-center text-muted-foreground">
    Expand Down
    Loading
    Loading