Skip to content
Open
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
13 changes: 4 additions & 9 deletions .claude/portless-shim.sh
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
#!/usr/bin/env bash
# Single entrypoint for terminal dev and Claude Preview, for the
# @dashframe/web app. Runs vite through portless so it gets a stable
# Compatibility entrypoint for Claude Preview. The shared web launcher starts
# the DashFrame server, then runs Vite through portless so it gets a stable
# https://dashframe.localhost URL. In a git worktree, portless prepends
# the branch slug as a subdomain (<branch>.dashframe.localhost), so every
# worktree runs its own copy without port conflicts; `portless list` shows
Expand All @@ -10,10 +10,5 @@
# where Preview will iframe.
set -euo pipefail

cd "$(dirname "${BASH_SOURCE[0]}")/../apps/web"

if [[ -n "${PORT:-}" ]]; then
exec portless run --app-port "${PORT}" "$@"
else
exec portless run "$@"
fi
ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)"
exec "${ROOT}/scripts/dev-web.sh" "$@"
6 changes: 5 additions & 1 deletion apps/server/src/assistant-run-route.ts
Original file line number Diff line number Diff line change
Expand Up @@ -417,7 +417,11 @@ export async function handleAssistantRunRequest(
terminationReason: result.terminationReason,
});
} catch (error) {
send({ type: "error", message: errorMessage(error) });
console.error("[assistant/run] run failed", error);
send({
type: "error",
message: "The assistant couldn't complete this request. Try again.",
});
} finally {
try {
controller.close();
Expand Down
30 changes: 30 additions & 0 deletions apps/server/src/functions/app-artifacts.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -721,4 +721,34 @@ describe("addDataSource / updateDataSource — same-operation minted-ref rollbac
expect(await vault.has(config.apiKey as SecretRef)).toBe(true);
expect(rows[0]?.id).toBe(result.id);
});

it("persists connector-specific config beside a vault-backed credential", async () => {
await call("addDataSource", {
type: "postgres",
name: "Warehouse",
connectionString: "postgres://user:secret@host/db",
config: { defaultSchema: "analytics" },
});

const rows = await db.select().from(dataSources);
const config = rows[0]?.config as {
connectionString?: unknown;
defaultSchema?: unknown;
};
expect(config.defaultSchema).toBe("analytics");
expect(isSecretRef(config.connectionString)).toBe(true);
expect(JSON.stringify(config)).not.toContain("postgres://user:secret");
});

it("rejects credentials smuggled through connector-specific config", async () => {
await expect(
call("addDataSource", {
type: "postgres",
name: "Unsafe",
config: { connectionString: "postgres://plaintext" },
}),
).rejects.toThrow(/typed credential fields/);

expect(await db.select().from(dataSources)).toHaveLength(0);
});
});
16 changes: 14 additions & 2 deletions apps/server/src/functions/app-artifacts.ts
Original file line number Diff line number Diff line change
Expand Up @@ -478,15 +478,27 @@ const addDataSource = mutation({
name: text,
apiKey: text.optional(),
connectionString: text.optional(),
config: jsonb.optional(),
},
handler: async (
ctx,
{ type, name, apiKey, connectionString },
{ type, name, apiKey, connectionString, config: rawConfig },
): Promise<{ id: string }> => {
const vault = vaultFromCtx(ctx);
// The id is generated by the DB default; use a pre-generated UUID as hint.
const rowId = crypto.randomUUID();
const config: DataSourceConfig = {};
if (rawConfig !== undefined && !isRecord(rawConfig)) {
throw new Error("Data source config must be an object");
}
if (
rawConfig &&
("apiKey" in rawConfig || "connectionString" in rawConfig)
) {
throw new Error(
"Data source credentials must use typed credential fields, not config",
);
}
const config: DataSourceConfig = { ...(rawConfig ?? {}) };

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Reject nested credentials in connector config

When a client sends connector config with a nested or alternate credential value, such as { nested: { connectionString: "postgres://..." } } or { dsn: "postgres://..." }, this spread persists it directly in data_sources.config; rowToDataSource then forwards non-credential config keys back to the renderer. That bypasses the new top-level smuggling guard and breaks the plaintext-never-at-rest/no-secret-in-DTO invariant for any buggy or untrusted caller of addDataSource; whitelist expected non-secret keys like defaultSchema or recursively reject credential-shaped data before copying config.

Useful? React with 👍 / 👎.

Comment on lines +493 to +501

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Blacklist-based credential-smuggling check doesn't scale.

The check hardcodes "apiKey" and "connectionString" as the only forbidden keys. If a future connector introduces a new credential field (e.g. an OAuth token), it must be added here manually or it can be smuggled through config and persisted in plaintext. Prefer deriving the blocked-key set from the canonical credential fields (or switch to an allowlist of known non-credential keys per connector type) so this stays in sync automatically.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/server/src/functions/app-artifacts.ts` around lines 493 - 501, The
credential check before constructing DataSourceConfig must derive forbidden keys
from the canonical typed credential fields instead of hardcoding only apiKey and
connectionString. Update the rawConfig validation in the surrounding data-source
creation flow to automatically reject any credential field, including future
additions, while preserving the existing error behavior and config construction.

// Refs minted in THIS call only — a fresh insert has no pre-existing config,
// so every ref that lands in `config` here was just minted by this operation.
const minted: SecretRef[] = [];
Expand Down
37 changes: 37 additions & 0 deletions apps/web/lib/posthog/loader.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";

import { isPostHogLoaded, loadPostHog, resetPostHogLoader } from "./loader";

const config = {
apiKey: "test-key",
apiHost: "https://posthog.example.com",
};

describe("PostHog loader lifecycle", () => {
let idleCallbacks: Array<() => void>;

beforeEach(async () => {
await resetPostHogLoader();
idleCallbacks = [];
window.requestIdleCallback = vi.fn((callback) => {
idleCallbacks.push(() =>
callback({ didTimeout: false, timeRemaining: () => 1 }),
);
return idleCallbacks.length;
});
});

afterEach(async () => {
await resetPostHogLoader();
});

it("does not initialize a load invalidated while waiting for idle", async () => {
const staleLoad = loadPostHog(config);

await resetPostHogLoader();
idleCallbacks[0]?.();

await expect(staleLoad).rejects.toThrow("PostHog load was cancelled");
expect(isPostHogLoaded()).toBe(false);
});
});
20 changes: 17 additions & 3 deletions apps/web/lib/posthog/loader.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ export interface PostHogLoadResult {
// Track loading state to prevent multiple initializations
let loadingPromise: Promise<PostHogLoadResult> | null = null;
let loadedInstance: PostHog | null = null;
let loadGeneration = 0;

/**
* Cross-browser requestIdleCallback with setTimeout fallback.
Expand Down Expand Up @@ -81,14 +82,21 @@ export async function loadPostHog(
return loadingPromise;
}

loadingPromise = (async () => {
const generation = loadGeneration;
const promise = (async () => {
// Wait for browser idle time before loading
await waitForIdle();

// Dynamically import posthog-js
const posthogModule = await import("posthog-js");
const posthog = posthogModule.default;

// A reset invalidates work that was already waiting for idle time or the
// dynamic import. Stale loads must never initialize the shared SDK.
if (generation !== loadGeneration) {
throw new Error("PostHog load was cancelled");
}

// Initialize PostHog with the provided config
posthog.init(config.apiKey, {
api_host: config.apiHost,
Expand All @@ -104,11 +112,14 @@ export async function loadPostHog(
// Clear the cached promise so subsequent calls can retry — without this,
// a transient chunk/network failure would permanently disable analytics
// for the rest of the session.
loadingPromise = null;
if (generation === loadGeneration) {
loadingPromise = null;
}
throw err;
});
loadingPromise = promise;

return loadingPromise;
return promise;
}

/**
Expand All @@ -131,6 +142,9 @@ export function isPostHogLoaded(): boolean {
* Properly cleans up the PostHog SDK instance (listeners/timers) before resetting state.
*/
export async function resetPostHogLoader(): Promise<void> {
// Invalidate any load currently waiting for idle time or a dynamic import.
loadGeneration += 1;

if (loadedInstance) {
try {
// Clean up listeners and timers by calling reset()
Expand Down
3 changes: 2 additions & 1 deletion apps/web/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -80,9 +80,10 @@
"scripts": {
"build": "vite build",
"clean": "rm -rf dist *.tsbuildinfo",
"dev": "portless",
"dev": "../../scripts/dev-web.sh bunx vite",
"dev:direct": "vite --host 127.0.0.1",
"lint": "bunx eslint .",
"prestart": "bun run build",
"preview": "vite preview",
"start": "vite preview",
"test": "vitest run",
Expand Down
7 changes: 6 additions & 1 deletion apps/web/vite.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,11 @@ export default defineConfig(({ mode }) => {
? rawPort
: 3000;
const wystackUrl = env.VITE_WYSTACK_URL?.trim();
if (mode === "development" && !wystackUrl) {
throw new Error(
"VITE_WYSTACK_URL is required for web development. Start the app with `bun run dev:web` so the DashFrame server and Vite proxy are launched together.",
);
}

return {
plugins: [
Expand Down Expand Up @@ -141,7 +146,7 @@ export default defineConfig(({ mode }) => {
},
server: {
port,
strictPort: false,
strictPort: true,
proxy: wystackUrl
? {
"/api": {
Expand Down
2 changes: 1 addition & 1 deletion apps/web/vitest.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ export default defineConfig({
},
resolve: {
alias: {
"@": path.resolve(__dirname, "./"),
"@": path.resolve(__dirname, "../../packages/app/src"),
"@dashframe/ui": path.resolve(__dirname, "../../packages/ui/src"),
"@dashframe/core": path.resolve(__dirname, "../../packages/core/src"),
"@dashframe/engine": path.resolve(__dirname, "../../packages/engine/src"),
Expand Down
1 change: 1 addition & 0 deletions bun.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

64 changes: 64 additions & 0 deletions design-qa.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
**Comparison Target**

- Source visual truth path: `/Users/youhaowei/.codex/visualizations/2026/07/09/019f4916-5845-7540-ab74-293e68c37230/insight-section-navigator-wireframe.html`
- Implementation URL: `https://v0-3-remote-onboarding.dashframe.localhost/insights/cafd95a3-9073-43f9-ae80-ccef86a512f3?visualize=false`
- Implementation screenshots: `/tmp/dashframe-insight-data.png`, `/tmp/dashframe-insight-visualize.png`
- Viewport: 1280 × 720 at 2× device pixel ratio
- State: existing joined insight, light theme; Data and Visualize output modes; Model, Fields, Metrics, Filters, and Sort modeling sections

**Findings**

- No actionable P0/P1/P2 issue remains in the browser-rendered implementation.
- [P3] Long generated table names truncate inside the compact model cards. The source mock uses shorter human-readable names; the implementation preserves the real dataset names and exposes source actions. This is acceptable for the current data fixture.

**Required Fidelity Surfaces**

- Fonts and typography: existing DashFrame typography tokens and weights are preserved; section hierarchy and compact labels remain readable at the target viewport.
- Spacing and layout rhythm: the left section navigator remains fixed while only the selected editor scrolls; compact model cards replace the oversized source cards; Data and Visualize controls occupy a stable header region.
- Colors and visual tokens: implementation uses existing neutral, border, emphasis, primary, and chart-series tokens.
- Image quality and asset fidelity: no raster imagery is required. Existing connector and chart icons are used; connector icons render at 12–16 px without scaling artifacts.
- Copy and content: Data is labeled as the canonical result, chart types appear only in Visualize, and every modeling section uses task-specific explanatory copy.

**Interaction Evidence**

- Model, Fields, Metrics, Filters, and Sort navigation buttons were clicked and each displayed only its corresponding editor.
- Data displayed the canonical result table without chart-type controls.
- Visualize selected an available chart, displayed chart-type controls, and removed the result table from the canvas.
- Sort uses the persisted `Insight.sorts` contract; its Add action is disabled when no selected field or metric can be sorted.
- No visible runtime error state appeared. Type checking, linting, and focused component tests cover the changed implementation.

**Comparison History**

- Initial P1: compact model cards inherited an inline connector icon whose SVG expanded to 300 × 300 px. Fixed by giving the connector wrapper an explicit inline-block formatting context. Post-fix browser measurement: 16 × 16 px icon and 104 px card height.
- Initial P2: five modeling sections were compressed into one row and their labels truncated. Fixed by using a wrapping fixed navigator with approximately 7 rem per item. Post-fix browser evidence shows full Model, Fields, Metrics, Filters, and Sort labels with counts.

**Full-view Comparison Evidence**

- Browser captures confirm the intended two-region workspace, fixed modeling navigator, compact model editor, canonical Data result, and separate Visualize chart chooser.
- The source HTML was rendered to `/tmp/insight-section-navigator-reference.html`, but the selected in-app browser rejected opening the local file under its URL security policy. A combined source-and-implementation screenshot could therefore not be produced in the permitted browser surface.

**Focused Region Comparison Evidence**

- Left model region was measured in the browser after the icon fix: compact source card 224 × 104 px, connector icon 16 × 16 px.
- A combined focused comparison is blocked by the same source-capture restriction above.

**Open Questions**

- None for implementation behavior. The remaining blocker is evidence capture, not a known product defect.

**Implementation Checklist**

- [x] Fixed section navigator for Model, Fields, Metrics, Filters, and Sort
- [x] One modeling editor visible at a time
- [x] Compact table and join cards in the left panel
- [x] Functional persisted sort editor
- [x] Separate Data and Visualize output modes
- [x] Chart types removed from the canonical Data result
- [x] Browser interaction pass at the target route
- [ ] Combined source and implementation comparison in the same visual input

**Follow-up Polish**

- Consider shorter display aliases for generated table names independently of this workspace redesign.

final result: blocked
26 changes: 23 additions & 3 deletions packages/app-data/src/assistant-run.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ describe("runAssistantPrompt error paths", () => {
vi.unstubAllGlobals();
});

it("throws the server's error message on a non-OK response", async () => {
it("does not expose a server error message on a non-OK response", async () => {
vi.stubGlobal(
"fetch",
vi.fn(async () =>
Expand All @@ -54,7 +54,7 @@ describe("runAssistantPrompt error paths", () => {

await expect(
runAssistantPrompt({ prompt: "hi", onEvent: () => {} }),
).rejects.toThrow("Unknown Anthropic model id: nope");
).rejects.toThrow("Assistant request failed (HTTP 400)");
});

it("falls back to an HTTP-status message when the error body is empty", async () => {
Expand All @@ -65,7 +65,27 @@ describe("runAssistantPrompt error paths", () => {

await expect(
runAssistantPrompt({ prompt: "hi", onEvent: () => {} }),
).rejects.toThrow("Assistant run failed with HTTP 502");
).rejects.toThrow("Assistant service is unavailable (HTTP 502)");
});

it("does not expose an HTML proxy error body to the assistant timeline", async () => {
vi.stubGlobal(
"fetch",
vi.fn(
async () =>
new Response(
"<!DOCTYPE html><html><head><title>502 - Bad Gateway</title></head></html>",
{
status: 502,
headers: { "Content-Type": "text/html" },
},
),
),
);

await expect(
runAssistantPrompt({ prompt: "hi", onEvent: () => {} }),
).rejects.toThrow("Assistant service is unavailable (HTTP 502)");
});

it("throws when an OK response carries no stream body", async () => {
Expand Down
11 changes: 3 additions & 8 deletions packages/app-data/src/assistant-run.ts
Original file line number Diff line number Diff line change
Expand Up @@ -118,12 +118,7 @@ export async function runAssistantPrompt({
}

async function readErrorMessage(res: Response): Promise<string> {
const text = await res.text().catch(() => "");
if (!text) return `Assistant run failed with HTTP ${res.status}`;
try {
const parsed = JSON.parse(text) as { error?: unknown };
return typeof parsed.error === "string" ? parsed.error : text;
} catch {
return text;
}
return res.status >= 500
? `Assistant service is unavailable (HTTP ${res.status})`
: `Assistant request failed (HTTP ${res.status})`;
}
3 changes: 3 additions & 0 deletions packages/app-data/src/data-sources.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,15 @@ export function useDataSources(): UseDataSourcesResult {
const result = useQuery(api.listDataSources);
return {
data: result.data as DataSource[] | undefined,
error: result.error,
isError: result.isError,
isLoading: result.isLoading,
// Forward the in-flight refetch flag so consumers can distinguish "no data
// yet" (isLoading) from "cached data, but a background refetch is running"
// (isFetching) — the latter must not flash a not-found state for a source
// an in-flight invalidation is about to return.
isFetching: result.isFetching,
refetch: result.refetch,
};
}

Expand Down
Loading
Loading