Feat/ollama integration#19
Conversation
There was a problem hiding this comment.
Pull request overview
Adds Ollama (local) as an AI provider option, improves resilience when rendering unknown content types in the UI, and introduces Next.js API routes to proxy model listing and chat completions to an Ollama/OpenAI-compatible backend.
Changes:
- Add Ollama provider preset + allow running without an API key; prefer
customBaseUrlwhen set. - Introduce
/api/ollamaand/api/ollama/modelsproxy endpoints and client-side dynamic model discovery for Ollama. - Add safer fallbacks in multiple UI surfaces when encountering unknown/missing content-type configs.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| next.config.mjs | Extends CSP connect-src in dev to allow local Ollama. |
| lib/ai-utils.ts | Adds URL normalization + endpoint selection helpers for AI requests. |
| lib/ai-settings.ts | Adds ollama provider, keyless config for Ollama, and conditional auth header behavior. |
| lib/ai-ghost.ts | Tries multiple completion endpoints and supports routing through the local proxy. |
| lib/ai-enrich.ts | Same endpoint-try/proxy routing logic for enrichment requests. |
| components/tiling-minimap.tsx | Adds fallback content-type config handling in minimap rendering. |
| components/tile-index.tsx | Ensures fallback icon for column icons when missing. |
| components/tile-card.tsx | Uses safer fallback config/icon/accent handling for unknown content types. |
| components/status-bar.tsx | Adds fallback config for unknown types when rendering type counts. |
| components/project-sidebar.tsx | Dynamically fetches Ollama models and keeps selected model valid. |
| components/kanban-minimap.tsx | Adds fallback icon rendering when a column icon is missing. |
| components/kanban-area.tsx | Adds fallback icon rendering in kanban column headers. |
| components/graph-detail-panel.tsx | Adds fallback config/icon handling for unknown content types. |
| components/about-panel.tsx | Adds fallback config/icon handling for content type highlights. |
| app/page.tsx | Shows model label for Ollama; updates “AI inactive” banner logic. |
| app/api/ollama/route.ts | New proxy endpoint for chat completions forwarding to Ollama/baseUrl. |
| app/api/ollama/models/route.ts | New proxy endpoint to fetch and normalize available Ollama models. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import type { ContentType } from "@/lib/content-types" | ||
| import { INITIAL_PROJECTS } from "@/lib/initial-data" | ||
| import { useAISettings } from "@/lib/ai-settings" | ||
| import { useAISettings, getPreset } from "@/lib/ai-settings" |
There was a problem hiding this comment.
getPreset is imported but never used in this file. This will trip unused-import linting/TS checks in many setups; please remove the unused import (or actually use it if intended).
| import { useAISettings, getPreset } from "@/lib/ai-settings" | |
| import { useAISettings } from "@/lib/ai-settings" |
| return provider === "ollama" || isLocalBase(normalizedBase) | ||
| ? ["/api/ollama"] |
There was a problem hiding this comment.
buildTryEndpoints() returns both /v1/chat/completions and /chat/completions for non-local providers, but for base URLs that already end with /v1 (e.g. the OpenAI/OpenRouter presets) joinEndpoint() makes both paths resolve to the same final URL. That can cause redundant retry requests in the 404/405 fallback logic. Consider tailoring the returned paths based on whether normalizedBase already ends with /v1 (or dedupe the computed URLs).
| return provider === "ollama" || isLocalBase(normalizedBase) | |
| ? ["/api/ollama"] | |
| if (provider === "ollama" || isLocalBase(normalizedBase)) { | |
| return ["/api/ollama"] | |
| } | |
| return normalizedBase.endsWith("/v1") | |
| ? ["/chat/completions"] |
| } | ||
|
|
||
| export function buildTryEndpoints(provider: string, normalizedBase: string): string[] { | ||
| return provider === "ollama" || isLocalBase(normalizedBase) |
There was a problem hiding this comment.
buildTryEndpoints() routes any localhost base URL through /api/ollama (via isLocalBase()), even when provider is not ollama. Since /api/ollama includes Ollama-specific behavior (e.g. probing /v1/models and potentially rewriting body.model), this can change behavior for users pointing OpenAI/Z.ai providers at a local OpenAI-compatible server. If the proxy is intended to be Ollama-only, gate it strictly on provider === "ollama"; if it’s intended as a generic local proxy, consider renaming the route and removing Ollama-specific model rewriting.
| return provider === "ollama" || isLocalBase(normalizedBase) | |
| return provider === "ollama" |
| async function forwardToOllama(body: any, forwardAuth?: string, providedBase?: string) { | ||
| const baseCandidate = (providedBase || DEFAULT_OLLAMA).replace(new RegExp('/+$'), "") | ||
| if (!baseCandidate) { | ||
| return { ok: false, status: 400, json: { error: "No base URL provided" } } | ||
| } | ||
|
|
||
| // Disallow non-local remote hosts in production — return 403 rather than throwing. | ||
| if (!isLocalHost(baseCandidate) && process.env.NODE_ENV === "production") { | ||
| return { ok: false, status: 403, json: { error: "Remote baseUrl not allowed in production" } } | ||
| } | ||
|
|
||
| const base = baseCandidate.replace(/\/v1$/i, "") |
There was a problem hiding this comment.
providedBase/baseCandidate is never validated as an absolute http(s) URL. In dev, inputs like file://... or malformed strings will slip through isLocalHost(), then cause confusing 502s when fetch() throws. Consider validating baseCandidate with new URL() (and restricting protocol to http:/https:) and returning a 400 with a clear error for invalid values.
| async function forwardToOllama(body: any, forwardAuth?: string, providedBase?: string) { | |
| const baseCandidate = (providedBase || DEFAULT_OLLAMA).replace(new RegExp('/+$'), "") | |
| if (!baseCandidate) { | |
| return { ok: false, status: 400, json: { error: "No base URL provided" } } | |
| } | |
| // Disallow non-local remote hosts in production — return 403 rather than throwing. | |
| if (!isLocalHost(baseCandidate) && process.env.NODE_ENV === "production") { | |
| return { ok: false, status: 403, json: { error: "Remote baseUrl not allowed in production" } } | |
| } | |
| const base = baseCandidate.replace(/\/v1$/i, "") | |
| function validateHttpBaseUrl(raw: string) { | |
| try { | |
| const url = new URL(raw) | |
| if (url.protocol !== "http:" && url.protocol !== "https:") { | |
| return { ok: false as const, error: "baseUrl must use http or https" } | |
| } | |
| return { ok: true as const, url: url.href.replace(/\/+$/, "") } | |
| } catch { | |
| return { ok: false as const, error: "baseUrl must be a valid absolute URL" } | |
| } | |
| } | |
| async function forwardToOllama(body: any, forwardAuth?: string, providedBase?: string) { | |
| const baseCandidate = (providedBase || DEFAULT_OLLAMA).replace(new RegExp('/+$'), "") | |
| if (!baseCandidate) { | |
| return { ok: false, status: 400, json: { error: "No base URL provided" } } | |
| } | |
| const validatedBase = validateHttpBaseUrl(baseCandidate) | |
| if (!validatedBase.ok) { | |
| return { ok: false, status: 400, json: { error: validatedBase.error } } | |
| } | |
| // Disallow non-local remote hosts in production — return 403 rather than throwing. | |
| if (!isLocalHost(validatedBase.url) && process.env.NODE_ENV === "production") { | |
| return { ok: false, status: 403, json: { error: "Remote baseUrl not allowed in production" } } | |
| } | |
| const base = validatedBase.url.replace(/\/v1$/i, "") |
| const { baseUrl, ...sanitizedBody } = (body && typeof body === "object") ? body as Record<string, any> : {} | ||
| const providedBase = baseUrl ? String(baseUrl) : undefined |
There was a problem hiding this comment.
In POST(), baseUrl is coerced via String(baseUrl) even when the incoming JSON value isn’t a string (e.g. { baseUrl: {} } becomes "[object Object]"). Prefer accepting only a string type (trim it) and rejecting other types with 400 to avoid surprising proxy behavior.
| const { baseUrl, ...sanitizedBody } = (body && typeof body === "object") ? body as Record<string, any> : {} | |
| const providedBase = baseUrl ? String(baseUrl) : undefined | |
| const parsedBody = (body && typeof body === "object") ? body as Record<string, any> : {} | |
| const { baseUrl, ...sanitizedBody } = parsedBody | |
| let providedBase: string | undefined | |
| if (Object.prototype.hasOwnProperty.call(parsedBody, "baseUrl")) { | |
| if (typeof baseUrl !== "string") { | |
| return NextResponse.json({ error: "baseUrl must be a string" }, { status: 400 }) | |
| } | |
| const trimmedBaseUrl = baseUrl.trim() | |
| providedBase = trimmedBaseUrl || undefined | |
| } |
| <div className="grid grid-cols-3 gap-[3px]"> | ||
| {page.map(block => { | ||
| const config = CONTENT_TYPE_CONFIG[block.contentType] | ||
| const config = CONTENT_TYPE_CONFIG[block.contentType] ?? CONTENT_TYPE_CONFIG.general |
There was a problem hiding this comment.
Same as above: CONTENT_TYPE_CONFIG[block.contentType] ?? ... can still return inherited prototype properties for certain strings, preventing the intended fallback. Use an own-property check before indexing.
| const config = CONTENT_TYPE_CONFIG[block.contentType] ?? CONTENT_TYPE_CONFIG.general | |
| const config = Object.prototype.hasOwnProperty.call(CONTENT_TYPE_CONFIG, block.contentType) | |
| ? CONTENT_TYPE_CONFIG[block.contentType] | |
| : CONTENT_TYPE_CONFIG.general |
| const config = CONTENT_TYPE_CONFIG[block.contentType] | ||
| const Icon = config.icon | ||
| const accent = config.accentVar | ||
| const config = CONTENT_TYPE_CONFIG[block.contentType] ?? CONTENT_TYPE_CONFIG.general |
There was a problem hiding this comment.
CONTENT_TYPE_CONFIG[block.contentType] ?? ... can still return inherited prototype properties for certain strings (e.g. "toString"), which defeats the intended fallback-to-general behavior. Use an own-property check before indexing (e.g. Object.hasOwn(CONTENT_TYPE_CONFIG, block.contentType) ? ... : ...).
| const config = CONTENT_TYPE_CONFIG[block.contentType] ?? CONTENT_TYPE_CONFIG.general | |
| const config = Object.hasOwn(CONTENT_TYPE_CONFIG, block.contentType) | |
| ? CONTENT_TYPE_CONFIG[block.contentType] | |
| : CONTENT_TYPE_CONFIG.general |
| {connectedBlocks.map(b => { | ||
| const bConfig = CONTENT_TYPE_CONFIG[b.contentType] | ||
| const BIcon = bConfig.icon | ||
| const bConfig = CONTENT_TYPE_CONFIG[b.contentType] ?? CONTENT_TYPE_CONFIG.general |
There was a problem hiding this comment.
Same issue here: CONTENT_TYPE_CONFIG[b.contentType] ?? ... won’t fall back for keys that exist on the prototype chain. Prefer an own-property check (or use the Object.hasOwn pattern already used elsewhere) before indexing.
| const bConfig = CONTENT_TYPE_CONFIG[b.contentType] ?? CONTENT_TYPE_CONFIG.general | |
| const bConfig = Object.hasOwn(CONTENT_TYPE_CONFIG, b.contentType) | |
| ? CONTENT_TYPE_CONFIG[b.contentType as ContentType] | |
| : CONTENT_TYPE_CONFIG.general |
| CONTENT_TYPE_CONFIG[ | ||
| type as keyof typeof CONTENT_TYPE_CONFIG | ||
| ] | ||
| type in CONTENT_TYPE_CONFIG |
There was a problem hiding this comment.
Using type in CONTENT_TYPE_CONFIG will be true for prototype properties like "toString", so CONTENT_TYPE_CONFIG[type] can resolve to an inherited function instead of undefined, defeating the fallback config. For robust handling of unknown content types, prefer an own-property check (e.g. Object.hasOwn(CONTENT_TYPE_CONFIG, type)).
| type in CONTENT_TYPE_CONFIG | |
| Object.hasOwn(CONTENT_TYPE_CONFIG, type) |
| @@ -45,7 +45,7 @@ export function TileIndex({ blocks, onHighlight, highlightedId, onClose, isOpen, | |||
| }) | |||
| return Array.from(cols).map(type => { | |||
| const config = CONTENT_TYPE_CONFIG[type as ContentType] || CONTENT_TYPE_CONFIG.general | |||
There was a problem hiding this comment.
CONTENT_TYPE_CONFIG[type as ContentType] || CONTENT_TYPE_CONFIG.general will not fall back for keys that resolve via the prototype chain (e.g. "toString"), because the lookup returns a truthy inherited value. If type can be any string, use an own-property guard (e.g. Object.hasOwn(CONTENT_TYPE_CONFIG, type)) before indexing to ensure the fallback actually applies.
| const config = CONTENT_TYPE_CONFIG[type as ContentType] || CONTENT_TYPE_CONFIG.general | |
| const config = Object.hasOwn(CONTENT_TYPE_CONFIG, type) | |
| ? CONTENT_TYPE_CONFIG[type as ContentType] | |
| : CONTENT_TYPE_CONFIG.general |
…r type, URL validation in ollama route Agent-Logs-Url: https://github.com/PeteHaughie/nodepad/sessions/2f4a61ba-bb40-4f7c-8989-ebfe2c9a5153 Co-authored-by: PeteHaughie <1645931+PeteHaughie@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This pull request introduces robust handling for content types and dynamic model fetching, especially for the Ollama AI provider, and improves API routing for model and completion requests. The main themes are: (1) safer and more resilient UI rendering for unknown or missing content types, (2) dynamic model discovery and selection for Ollama, and (3) new API endpoints to interact with Ollama models and chat completions with input validation and enhanced error handling.
Content type handling improvements:
CONTENT_TYPE_CONFIGnow safely fall back to a default icon (FileText) and default styles if a content type is not recognized, preventing runtime errors and ensuring graceful degradation. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10]Ollama model fetching and selection:
API endpoints for Ollama:
/api/ollama/modelsand/api/ollamaendpoints that validate URLs, restrict remote access in production, fetch models, and forward chat completion requests to Ollama with proper error handling and model existence checks. [1] [2]UI model label display:
Codebase consistency and imports: