Skip to content

ui: Improve ui#136

Merged
aszenz merged 4 commits intomasterfrom
feature/organized-changes
Feb 3, 2026
Merged

ui: Improve ui#136
aszenz merged 4 commits intomasterfrom
feature/organized-changes

Conversation

@aszenz
Copy link
Owner

@aszenz aszenz commented Feb 3, 2026

  • Easily downloadable links
  • Better mobile view
  • Improve menu
  • Easier navigation

Copilot AI review requested due to automatic review settings February 3, 2026 20:59
@aszenz aszenz force-pushed the feature/organized-changes branch from af13f31 to b2b5424 Compare February 3, 2026 21:05
Copy link

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 improves UI navigation and mobile layout, and introduces stable static download links for models, notebooks, and raw data in the built site.

Changes:

  • Add a Vite build plugin to copy model/notebook/data files into dist/downloads/ for static downloading.
  • Update model/notebook/data download UX to use stable /downloads/... URLs and add a mobile schema tab menu.
  • Rework layout/styling (sticky breadcrumbs, responsive home/model/schema layouts) and adjust result rendering sizing.

Reviewed changes

Copilot reviewed 16 out of 17 changed files in this pull request and generated 8 comments.

Show a summary per file
File Description
vite.config.ts Registers the new Vite plugin to copy downloadable assets into the build output.
src/index.css Large responsive/sticky nav and layout updates; adds styling for new menu chevrons and mobile schema menu.
src/download-utils.ts Adds helpers to build stable /downloads/... URLs and a download trigger utility.
src/Schema.tsx Moves schema tab/expand state into URL params, adds mobile menu, and switches raw data links to static downloads.
src/RenderedResult.tsx Makes height a required prop and applies it to the viz container.
src/QueryResult.tsx Passes height="100%" to RenderedResult for full-height rendering.
src/PreviewResult.tsx Passes height="100%" to RenderedResult for full-height rendering.
src/NotebookViewer.tsx Switches notebook download from blob-based to static /downloads/notebooks/... link.
src/NotebookCellRenderer.tsx Updates RenderedResult usage to provide an explicit height.
src/ModelHome.tsx Adds “Download Model” link and tweaks explorer navigation query params.
src/Menu.tsx Adds a chevron icon to menu triggers.
src/Home.tsx Adjusts home layout and removes card descriptions to simplify navigation UI.
src/Card.tsx Simplifies card API and UI (removes description and arrow icon).
src/Breadcrumbs.tsx Improves breadcrumbs structure (logo home link, new view-type dropdown).
plugins/vite-plugin-copy-downloads.ts New build plugin to copy .malloy, .malloynb, and data files into dist/downloads/.
img/chevron_down.svg Updates chevron SVG to use currentColor for theme-consistent coloring.

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

Copy link

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

Copilot reviewed 17 out of 18 changed files in this pull request and generated 4 comments.


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

@aszenz aszenz force-pushed the feature/organized-changes branch from 992ea9d to c83d496 Compare February 3, 2026 21:34
Copilot AI review requested due to automatic review settings February 3, 2026 21:53
@aszenz aszenz force-pushed the feature/organized-changes branch from 58305d9 to 62225e5 Compare February 3, 2026 21:56
Copy link

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

Copilot reviewed 18 out of 19 changed files in this pull request and generated 4 comments.


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

Comment on lines +54 to +86
// Decode URI components
const category = decodeURIComponent(rawCategory);
const restParts = rawRestParts.map((part) => decodeURIComponent(part));

// Reject any path traversal attempts
if (restParts.some((part) => part === ".." || part === ".")) {
res.statusCode = 400;
res.end("Bad Request");
return;
}

const rest = restParts.join("/");
let filePath: string;

if (category === "models" || category === "notebooks") {
// Models and notebooks both live in the top-level models directory.
filePath = join(modelsDir, rest);
} else if (category === "data") {
filePath = join(modelsDir, "data", rest);
} else {
next();
return;
}

// Verify the resolved path is within the allowed directory
const normalizedPath = resolve(filePath);
const allowedDir =
category === "data" ? resolve(modelsDir, "data") : resolve(modelsDir);
if (!normalizedPath.startsWith(allowedDir)) {
res.statusCode = 403;
res.end("Forbidden");
return;
}
Copy link

Copilot AI Feb 3, 2026

Choose a reason for hiding this comment

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

Path validation in the dev middleware is bypassable: decodeURIComponent can introduce / inside a segment (e.g. %2e%2e%2fmodels-secret/file -> ../models-secret/file), which is not caught by the part === ".." check. Combined with normalizedPath.startsWith(allowedDir), this can allow reads outside modelsDir due to prefix matches (e.g. /repo/models-secret startsWith /repo/models). Consider resolving against allowedDir and verifying containment via path.relative(allowedDir, candidate) (must not start with .. or be absolute), and/or rejecting decoded segments containing / or \\.

Copilot uses AI. Check for mistakes.
Comment on lines +54 to +57
// Decode URI components
const category = decodeURIComponent(rawCategory);
const restParts = rawRestParts.map((part) => decodeURIComponent(part));

Copy link

Copilot AI Feb 3, 2026

Choose a reason for hiding this comment

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

decodeURIComponent(rawCategory) / decodeURIComponent(part) can throw a URIError on malformed percent-encoding in the request URL, which would bubble out of the middleware and can crash the dev server. Wrap the decoding in a try/catch and respond with 400 (or next()), rather than throwing.

Copilot uses AI. Check for mistakes.
Comment on lines +103 to +108
const tabParam = searchParams.get("tab");
const activeTab =
tabParam && validTabs.includes(tabParam as (typeof validTabs)[number])
? (tabParam as "sources" | "queries" | "code" | "data")
: getDefaultTab();

Copy link

Copilot AI Feb 3, 2026

Choose a reason for hiding this comment

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

activeTab accepts any value in validTabs even when the corresponding section isn't available (e.g. ?tab=queries when hasQueries is false). In that case, none of the tab panes render and the schema area becomes blank. Consider validating the URL tab against available tabs (hasExplores/hasQueries/modelCode/hasDataSources) and falling back to getDefaultTab() when the requested tab isn't renderable.

Suggested change
const tabParam = searchParams.get("tab");
const activeTab =
tabParam && validTabs.includes(tabParam as (typeof validTabs)[number])
? (tabParam as "sources" | "queries" | "code" | "data")
: getDefaultTab();
// Only tabs that have corresponding content available should be considered renderable
const availableTabs: Array<"sources" | "queries" | "code" | "data"> = [];
if (hasExplores) {
availableTabs.push("sources");
}
if (hasQueries) {
availableTabs.push("queries");
}
if (modelCode) {
availableTabs.push("code");
}
if (hasDataSources) {
availableTabs.push("data");
}
const tabParam = searchParams.get("tab");
const isValidRequestedTab =
tabParam !== null &&
validTabs.includes(tabParam as (typeof validTabs)[number]) &&
availableTabs.includes(tabParam as "sources" | "queries" | "code" | "data");
const activeTab = isValidRequestedTab
? (tabParam as "sources" | "queries" | "code" | "data")
: getDefaultTab();

Copilot uses AI. Check for mistakes.
Comment on lines +89 to +110
const originalDocument = global.document;
global.document = {
createElement: mockCreateElement,
body: {
appendChild: mockAppendChild,
removeChild: mockRemoveChild,
},
} as unknown as Document;

try {
triggerDownload("/test/url", "test-file.txt");

expect(mockCreateElement).toHaveBeenCalledWith("a");
expect(mockAnchor.href).toBe("/test/url");
expect(mockAnchor.download).toBe("test-file.txt");
expect(mockAppendChild).toHaveBeenCalledWith(mockAnchor);
expect(mockAnchor.click).toHaveBeenCalledTimes(1);
expect(mockRemoveChild).toHaveBeenCalledWith(mockAnchor);
} finally {
// Restore
global.document = originalDocument;
}
Copy link

Copilot AI Feb 3, 2026

Choose a reason for hiding this comment

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

The test mutates global.document and restores it by assignment in finally. If document was originally undefined, this leaves a document property set to undefined on global, which can leak into other tests. Consider using globalThis + vi.stubGlobal("document", ...) and vi.unstubAllGlobals() (or delete the property when it wasn’t originally present) for cleaner isolation.

Copilot uses AI. Check for mistakes.
@aszenz aszenz merged commit bf8179c into master Feb 3, 2026
2 of 3 checks passed
@aszenz aszenz deleted the feature/organized-changes branch February 3, 2026 22:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants