Conversation
Signed-off-by: Christopher Raquet <xf3778@kit.edu>
Fixed 2 file(s) based on 1 unresolved review comment. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
chore(deps): security improvements
chore(deps): update node.js to v24
…a-action-6.x chore(deps): update docker/metadata-action action to v6
…ction-4.x chore(deps): update docker/login-action action to v4
…ush-action-7.x chore(deps): update docker/build-push-action action to v7
# Conflicts: # CHANGELOG.md
Fixed 1 file(s) based on 1 unresolved review comment. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
Improved file explorer
|
Warning Review limit reached
More reviews will be available in 5 minutes and 48 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (27)
WalkthroughThis PR adds a comprehensive AI Assistant feature to NovaCrate with multi-provider LLM support (OpenAI, Anthropic, OpenRouter, OpenAI-compatible), a full chat interface with tool-driven agent interactions for RO-Crate entity/file operations, and simultaneously refactors file preview from prop-driven to tab-based state management. It includes three new API routes, security middleware with rate limiting and origin validation, provider configuration UI, and relocates validation drawer state to a new layout store. ChangesAI Assistant Backend Infrastructure
AI Assistant State Management & API Routes
Layout State & Validation Drawer Relocation
File Explorer: Prop-Driven to Tab-Based Preview Refactor
AI Assistant UI: Settings, Chat, Navigation
Supporting Changes
🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related PRs
Suggested Labels
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/file-explorer/viewers/iframe.tsx (1)
21-28:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winSandbox the HTML preview iframe.
This iframe renders crate-supplied HTML from a blob URL without any sandboxing. That gives imported HTML a same-origin execution context, so a malicious crate can run script against the app and its stored data instead of being isolated.
Suggested fix
- <iframe className="grow" src={url}></iframe> + <iframe + className="grow" + src={url} + sandbox="" + referrerPolicy="no-referrer" + ></iframe>🤖 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 `@components/file-explorer/viewers/iframe.tsx` around lines 21 - 28, The iframe in components/file-explorer/viewers/iframe.tsx is currently unsandboxed; update the JSX <iframe ...> element to include a restrictive sandbox attribute (e.g., sandbox="" or sandbox="allow-forms allow-popups allow-modals" but explicitly do NOT include allow-same-origin or allow-scripts) so crate-supplied HTML cannot obtain same-origin privileges; also add a descriptive title prop for accessibility (title="Preview") and, if you need file downloads or popups, add only the minimal sandbox tokens (like allow-popups or allow-downloads) rather than allow-same-origin/allow-scripts.
🟠 Major comments (23)
components/ai/model-selection.tsx-1-15 (1)
1-15:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd the missing
"use client"directive.This component uses React hooks and a Zustand hook, so Next will treat it as an invalid Server Component without the client directive. That will break as soon as
ModelSelectionis rendered.Suggested fix
+"use client" + import { useAIAssistantSettings } from "`@/lib/state/ai-assistant-settings`"As per coding guidelines, "Mark client components with
"use client"directive at top of file".🤖 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 `@components/ai/model-selection.tsx` around lines 1 - 15, This file defines the ModelSelection component and uses React hooks (useCallback, useContext) and the Zustand hook useAIAssistantSettings, so add the "use client" directive as the very first line of the module (before any imports) to mark it as a client component; ensure the exact string "use client" is placed on its own line at the top so ModelSelection and its hook usage run on the client.Source: Coding guidelines
lib/ai/rate-limiter.ts-10-10 (1)
10-10:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftEvict stale keys, not just stale timestamps.
check()only prunes the array for the current key. Keys that send one request and never come back remain inrequestsforever, so a public endpoint can accumulate unboundedMapentries and eventually run out of memory. This needs key-level eviction (or a periodic/global sweep), not just per-key timestamp pruning.Possible fix direction
export class RateLimiter { private readonly requests = new Map<string, number[]>() + private lastSweepAt = 0 check(key: string, now: number = Date.now()): { allowed: boolean; retryAfterMs?: number } { + if (now - this.lastSweepAt >= this.windowMs) { + const cutoff = now - this.windowMs + for (const [storedKey, storedTimestamps] of this.requests) { + const activeForKey = storedTimestamps.filter((t) => t > cutoff) + if (activeForKey.length === 0) { + this.requests.delete(storedKey) + } else { + this.requests.set(storedKey, activeForKey) + } + } + this.lastSweepAt = now + } + const windowStart = now - this.windowMs const timestamps = this.requests.get(key)Also applies to: 28-49
🤖 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 `@lib/ai/rate-limiter.ts` at line 10, The requests Map (private readonly requests) can grow unbounded because check() only prunes per-key timestamp arrays and never removes keys; update the RateLimiter logic so keys with empty or fully-stale timestamp arrays are removed from the Map (e.g., after filtering inside check()), and add either a periodic global sweep (setInterval) that iterates requests and deletes entries whose last timestamp is older than windowMs or ensure every access to check()/consume deletes the key when its timestamps array becomes empty; reference the requests Map and the check() method (and any consume/tryAcquire methods) when implementing the deletion/sweep to ensure stale keys are evicted.lib/state/file-explorer-state.ts-29-35 (1)
29-35:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCanonicalize
activeFilePreviewTabPathbefore storing it.These actions resolve tabs with
normalizeIdentifier(...)but persist the raw caller-supplied path. If a caller focuses or closes an equivalent variant like./data/file.txtvsdata/file.txt,activeFilePreviewTabPathcan reference no actual tab, and Line 64's strict equality skips the active-tab handoff while still removing the tab. Store the matched tab'sfilePath(or a single normalized key) everywhere you writeactiveFilePreviewTabPath.Suggested change
openTab(tab: IFilePreviewTab, focus?: boolean) { set((store) => { - if (focus) store.activeFilePreviewTabPath = tab.filePath - const existing = store.filePreviewTabs.findIndex( (t) => normalizeIdentifier(t.filePath) === normalizeIdentifier(tab.filePath) ) if (existing >= 0) { store.filePreviewTabs[existing] = tab + if (focus) store.activeFilePreviewTabPath = tab.filePath } else { store.filePreviewTabs.push(tab) + if (focus) store.activeFilePreviewTabPath = tab.filePath } return store }) }, focusTab(path: string) { - if ( - !get().filePreviewTabs.find( - (tab) => normalizeIdentifier(tab.filePath) === normalizeIdentifier(path) - ) - ) { + const existing = get().filePreviewTabs.find( + (tab) => normalizeIdentifier(tab.filePath) === normalizeIdentifier(path) + ) + if (!existing) { console.warn( `Tried to focus file preview tab for ${path}, but the tab does not exist` ) return } set({ - activeFilePreviewTabPath: path + activeFilePreviewTabPath: existing.filePath }) }, closeTab(path: string) { set((store) => { - if (store.activeFilePreviewTabPath === path) { + if ( + store.activeFilePreviewTabPath && + normalizeIdentifier(store.activeFilePreviewTabPath) === + normalizeIdentifier(path) + ) { const indexBefore = store.filePreviewTabs.findIndex( (tab) => normalizeIdentifier(tab.filePath) === normalizeIdentifier(path) ) if (indexBefore >= 0) { if (indexBefore > 0) { @@ closeOtherTabs(path: string) { set((store) => { - store.filePreviewTabs = store.filePreviewTabs.filter( + const remaining = store.filePreviewTabs.filter( (tab) => normalizeIdentifier(tab.filePath) === normalizeIdentifier(path) ) - store.activeFilePreviewTabPath = path + store.filePreviewTabs = remaining + store.activeFilePreviewTabPath = remaining[0]?.filePath return store }) },Also applies to: 46-59, 62-98
🤖 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 `@lib/state/file-explorer-state.ts` around lines 29 - 35, openTab (and other tab-mutating methods) currently set activeFilePreviewTabPath to the caller-supplied path which may not match the normalized identifiers used to find tabs; change these to canonicalize the value before storing: when finding an existing tab via normalizeIdentifier (e.g., in openTab, closeTab, remove tab handlers that use filePreviewTabs and normalizeIdentifier), set activeFilePreviewTabPath to the matched tab.filePath (the canonical/normalized representative) or, if no existing match, store a single normalized key (normalizeIdentifier(tab.filePath)) consistently everywhere activeFilePreviewTabPath is written so subsequent strict-equality comparisons will correctly identify the active tab.docker-compose.yml-1-6 (1)
1-6:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPass runtime config into the container.
This compose file never injects
.env/environment, so the server started bydocker compose upcannot seeBASE_PATH,IFRAME_TARGET_ORIGIN, or the newAI_ASSISTANT_*settings added in this PR. That makes the new deployment path unable to configure the AI feature at runtime.Suggested change
services: novacrate: image: "ghcr.io/kit-data-manager/novacrate:1" restart: unless-stopped + env_file: + - .env ports: - "3000:3000"🤖 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 `@docker-compose.yml` around lines 1 - 6, The docker-compose service "novacrate" doesn't pass runtime configuration into the container; update the novacrate service to load environment variables (either via env_file: .env and/or an environment: block) so BASE_PATH, IFRAME_TARGET_ORIGIN and all AI_ASSISTANT_* variables are injected at container runtime; ensure the env_file includes .env and/or explicitly list the keys under environment for the novacrate service so the app (inside the container) can read them..env.example-4-4 (1)
4-4:⚠️ Potential issue | 🟠 MajorReject loopback/private-resolution hostnames in
AI_ASSISTANT_BASE_URL_REGEX.
AI_ASSISTANT_BASE_URL_REGEX="^(?:ki-toolbox\.scc\.kit\.edu|(?!(?:[a-z0-9-]+\.)*kit\.edu$)(?:[a-z0-9-]+\.)*[a-z0-9-]+)$"
validateBaseUrl()blocks IP-literal hostnames (IPv4/IPv6) before applyingAI_ASSISTANT_BASE_URL_REGEX, sohttps://127.0.0.1/https://[::1]-style inputs won’t get through.- Hostnames that resolve to loopback/private IPs (e.g.,
localhostand other internal single-label names) aren’t blocked by the IP-literal checks; they only need to match the regex in.env.example, which currently allows any hostname that doesn’t end inkit.edu. If user-suppliedconfig.baseUrlreaches the server-side provider adapters, this enables SSRF to internal services.Prefer a strict allowlist of known public provider base URLs/domains, or explicitly deny
localhostand loopback/private targets (ideally after DNS resolution / IP classification), instead of a broad “not*.kit.edu” hostname pattern.🤖 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 @.env.example at line 4, The current AI_ASSISTANT_BASE_URL_REGEX is too permissive and allows single-label hostnames (e.g., localhost/internal names) that can resolve to loopback/private IPs; update the config and validation so either (1) replace the broad regex with an explicit allowlist of trusted provider base domains (preferred), or (2) tighten the regex to explicitly reject common loopback/private/reserved hostnames (e.g., localhost, 127.x.x.x, ::1, single-label names) and ensure validateBaseUrl() performs DNS resolution and IP classification to block addresses in private/loopback ranges before config.baseUrl is accepted and passed to provider adapters; locate AI_ASSISTANT_BASE_URL_REGEX in the env example and the validateBaseUrl function to implement the change.lib/ai/providers/OpenAIAdapter.ts-36-38 (1)
36-38:⚠️ Potential issue | 🟠 Major | ⚡ Quick winError cause may expose sensitive request details.
Including the
Requestobject as the error cause can leak sensitive information like headers (including the Authorization header with API key), request URL, and other request metadata when the error is logged or serialized.🔒 Proposed fix
} else { - throw new Error(`Failed to fetch models (${req.status}: ${req.statusText})`, { - cause: req - }) + throw new Error(`Failed to fetch models (${req.status}: ${req.statusText})`) }🤖 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 `@lib/ai/providers/OpenAIAdapter.ts` around lines 36 - 38, The current throw includes the full Request object as the error cause (in the throw inside OpenAIAdapter.ts that creates the "Failed to fetch models" Error), which may expose sensitive data like Authorization headers; remove the raw Request object from the cause and instead either omit the cause or pass a sanitized object (e.g., { status: req.status, statusText: req.statusText, url: safeUrl } or just status/statusText) so logs do not contain headers or full request metadata; update the throw site (the Error creation that includes cause: req) accordingly and ensure any downstream error handling/logging uses only the sanitized fields.app/api/ai/models/route.ts-24-30 (1)
24-30:⚠️ Potential issue | 🟠 Major | ⚡ Quick winError responses may leak sensitive provider information.
Returning the raw error message or stringified error object to the client can expose sensitive details like internal URLs, stack traces, or API key fragments. Return a generic error message for production.
🔒 Proposed fix
} catch (error) { + console.error("Failed to fetch models:", error) return Response.json( { - error: error instanceof Error ? error.message : JSON.stringify(error) + error: "Failed to fetch models from provider" }, { status: 500 } ) }🤖 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 `@app/api/ai/models/route.ts` around lines 24 - 30, The catch block that returns Response.json(...) is exposing raw error details; change it to return a generic error message to clients (e.g., "Internal server error") while logging the full error server-side for diagnostics. Update the catch in app/api/ai/models/route.ts where Response.json is used: replace the client-facing payload with a non-sensitive message and call your logger (or console.error) with error instanceof Error ? error.stack ?? error.message : JSON.stringify(error) so full details remain in server logs but are not returned to clients.app/api/ai/test/route.ts-26-32 (1)
26-32:⚠️ Potential issue | 🟠 Major | ⚡ Quick winError responses may leak sensitive connection details.
Returning the raw error message or stringified error object to the client can expose sensitive details like internal URLs, stack traces, or authentication failures. Return a generic error message for production.
🔒 Proposed fix
} catch (error) { console.error(error) return Response.json( { success: false, - error: error instanceof Error ? error.message : JSON.stringify(error) + error: "Failed to connect to provider" }, { status: 500 } ) }🤖 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 `@app/api/ai/test/route.ts` around lines 26 - 32, The current error path in the route handler returns raw error contents via Response.json (using the error variable), which can leak sensitive details; change the handler so Response.json always returns a generic message like "Internal server error" (and keep { status: 500 }) while logging the full error server-side (e.g., console.error or your app logger) for diagnostics; update the error-returning block in route.ts where Response.json is called to use the generic message and ensure the detailed error is not sent to the client.lib/ai/utils.ts-6-6 (1)
6-6:⚠️ Potential issue | 🟠 MajorAvoid internal Next.js import for basePath and dedupe error handling in AI utils
lib/ai/utils.ts: line 6 importsaddBasePathfromnext/dist/client/add-base-path(internal API / breaking-change risk). Replace with a public approach for prefixing API fetch URLs (e.g., use aNEXT_PUBLIC_BASE_PATH/build-time exported constant and prefix/api/...).testProviderandfetchModelsduplicate the same response-error parsing/throw logic—extract a shared helper.- Add explicit return type annotations for
providerDisplayName,testProvider, andfetchModelsto match the TypeScript guidelines.- Fix import order to follow the repo rule (external
next/*imports before internal@/absolute imports).🤖 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 `@lib/ai/utils.ts` at line 6, Replace the internal Next import by removing addBasePath from next/dist and instead prefix API URLs using a build-time public constant (e.g., NEXT_PUBLIC_BASE_PATH or an exported basePath constant) when constructing fetch paths; remove the internal import and reorder imports so external next/* imports appear before internal `@/` imports. Extract the duplicated response error parsing/throw logic used in testProvider and fetchModels into a shared helper function (e.g., parseApiErrorResponse) and call it from both places, then add explicit TypeScript return type annotations for providerDisplayName, testProvider, and fetchModels to match guidelines. Ensure references to the original symbols (providerDisplayName, testProvider, fetchModels) are updated to use the new basePath prefixing approach and the shared error helper.components/editor/entity-editor.tsx-40-41 (1)
40-41:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCanonicalize the resolved entity ID before using it downstream.
Switching the reads to
findEntity(...)is only half of the fix. This component still uses the rawentityIdfor changelist lookups, property mutations, save-error clearing, and preview toggling, so a non-canonical ID now renders correctly but still edits or reads the wrong map entry. Derive the stored ID once fromentity?.["@id"] ?? entityIdand use that for the rest of the component.As per coding guidelines, "Use a utility method like
findEntityfrom@/lib/utilswhen getting an entity by its ID, as an ID can be formatted in multiple ways."🤖 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 `@components/editor/entity-editor.tsx` around lines 40 - 41, The component resolves entities with findEntity but still uses the raw entityId elsewhere; derive a canonical storedId once (e.g., const storedId = entity?.["`@id`"] ?? entityId) after calling useEditorState/findEntity and replace all downstream uses of the raw entityId (changelist lookups, property mutations, save-error clearing, preview toggling, etc.) with storedId so all reads/writes target the same canonical key; keep using findEntity for initial resolution (entity and originalEntity) and update any helper calls that expect an ID to receive storedId.Source: Coding guidelines
lib/ai/providers/OpenAICompatibleAdapter.ts-14-19 (1)
14-19:⚠️ Potential issue | 🟠 Major | ⚡ Quick winProbe the same capability that the adapter actually consumes.
fetchModels()uses${baseUrl}/models, buttestConnection()validates${baseUrl}/auths/. That means a provider can be usable for this adapter and still fail setup purely because it does not expose the extra/auths/route.Suggested fix
- const url = `${this.config.baseUrl}/auths/` + const url = `${this.config.baseUrl}/models`Also applies to: 52-80
🤖 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 `@lib/ai/providers/OpenAICompatibleAdapter.ts` around lines 14 - 19, testConnection currently probes `${baseUrl}/auths/` while fetchModels uses `${baseUrl}/models`, so the health check can fail for providers that expose only the models endpoint; update testConnection to probe the same capability the adapter consumes by either calling fetchModels() directly or issuing the equivalent GET to `${this.config.baseUrl}/models` with the same Authorization header and this.config.headers, and make the same change in the other similar block (lines 52-80) so all connectivity checks and health probes align with fetchModels().components/ai/input.tsx-20-25 (1)
20-25:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBlock Enter submissions while the request is already in flight.
onKeyDowncan still callsendMessage()whilestatusis"submitted"or"streaming", even though the UI has already switched to a stop-only state. That makes overlapping submits possible through the keyboard path even when the send button is hidden.Suggested fix
+ const canSend = + !disableSend && (status === "ready" || status === "error") && !!message.trim() + const sendMessage = useCallback(() => { - if (disableSend) return - if (!message.trim()) return + if (!canSend) return setMessage("") _sendMessage(message) - }, [_sendMessage, disableSend, message]) + }, [_sendMessage, canSend, message]) ... if (e.key === "Enter" && !e.shiftKey) { e.preventDefault() sendMessage() } }} ></Textarea> {(status === "ready" || status === "error") && ( - <Button className="" onClick={sendMessage} disabled={disableSend}> + <Button className="" onClick={sendMessage} disabled={!canSend}> <SendIcon className="size-4" /> </Button> )}Also applies to: 34-42
🤖 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 `@components/ai/input.tsx` around lines 20 - 25, sendMessage currently allows keyboard-triggered sends even when a request is in flight; update sendMessage (and any onKeyDown callers) to early-return if status indicates an in-flight request (e.g., status === "submitted" || status === "streaming") and include status in the useCallback dependency array so the closure sees changes; reference the sendMessage function, the status variable, and the onKeyDown handler to locate and apply this guard (also apply the same guard to the other similar block mentioned).components/ai/chat.tsx-241-277 (1)
241-277:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThe validation tool returns before most validations finish.
Both
Array.from(...).map(...)andObject.keys(...).map(...)are inserted intoPromise.allSettledas plain arrays, so those inner validation promises are not awaited beforeresultStoreis read. The assistant can therefore receive stale or incomplete validation results.Suggested fix
- const promises = [ - validation - .validateCrate() - .catch((e) => console.error("Crate validation failed: ", e)), - Array.from(entities.values()).map((entity) => { - return Promise.allSettled([ - validation - .validateEntity(entity["`@id`"]) - .catch((e) => - console.error( - `Entity validation failed on ${entity["`@id`"]}: `, - e - ) - ), - Object.keys(entity).map((prop) => { - return validation - .validateProperty(entity["`@id`"], prop) - .catch((e) => - console.error( - `Property validation failed on ${entity["`@id`"]} ${prop}: `, - e - ) - ) - }) - ]) - }) - ] - - await Promise.allSettled(promises) + const validationTasks = [ + validation + .validateCrate() + .catch((e) => console.error("Crate validation failed: ", e)), + ...Array.from(entities.values()).flatMap((entity) => [ + validation + .validateEntity(entity["`@id`"]) + .catch((e) => + console.error( + `Entity validation failed on ${entity["`@id`"]}: `, + e + ) + ), + ...Object.keys(entity).map((prop) => + validation + .validateProperty(entity["`@id`"], prop) + .catch((e) => + console.error( + `Property validation failed on ${entity["`@id`"]} ${prop}: `, + e + ) + ) + ) + ]) + ] + + await Promise.allSettled(validationTasks)🤖 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 `@components/ai/chat.tsx` around lines 241 - 277, In the "getValidationResults" case the arrays returned by Array.from(...).map(...) and Object.keys(...).map(...) are nested plain arrays and get passed into the top-level promises array, so Promise.allSettled(promises) doesn't await the inner validation promises; fix by building a flat list of promises: call validation.validateCrate(), then for each entity (from editorState.getState().getEntities()) push the Promise from validation.validateEntity(entity["`@id`"]) and push each validation.validateProperty(entity["`@id`"], prop) for every prop into the same promises array (or use map + flat()/flatMap to produce a single-level array), then await Promise.allSettled(...) on that flattened array before reading validation.resultStore.getState().results and calling addToolOutput; ensure validateEntity and validateProperty .catch handlers remain attached to their respective promises if desired.app/api/ai/chat/route.ts-11-25 (1)
11-25:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate the whole request body before using it.
await req.json()is outside thetry, andbody.messagesis never runtime-checked. A malformed JSON payload or a body missingmessageswill skip the intended 400 path and fail later as a 500 instead.Suggested fix
- const body: { - messages: UIMessage[] - config: unknown - } = await req.json() + let body: unknown + try { + body = await req.json() + } catch (e) { + console.error("Bad JSON request to /ai/chat/", e) + return Response.json({ error: "Bad Request" }, { status: 400 }) + } + + if ( + !body || + typeof body !== "object" || + !("messages" in body) || + !Array.isArray(body.messages) || + !("config" in body) + ) { + return Response.json({ error: "Bad Request" }, { status: 400 }) + } let config try { config = z .extend(ProviderConfigurationWithoutModelsSchema, { selectedModel: z.string() }) - .parse(body.config) + .parse(body.config)🤖 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 `@app/api/ai/chat/route.ts` around lines 11 - 25, The request JSON is parsed outside the try and only config is validated, so malformed JSON or missing/invalid body.messages will cause a 500 later; update the handler to validate the entire body inside the try by building a zod schema for the whole payload (e.g., z.object({ messages: z.array(<UIMessage runtime schema>), config: z.extend(ProviderConfigurationWithoutModelsSchema, { selectedModel: z.string() }) }) ), then call await req.json() and schema.parse(...) inside the try, assign the parsed result to `body` (and use its `messages`/`config`), and on catch return the 400 Response.json({ error: "Bad Request" }).lib/ai/tools.ts-51-53 (1)
51-53:⚠️ Potential issue | 🟠 MajorFix Zod string description usage in
importPersonFromORCID
importPersonFromORCID.inputSchema.identifierusesz.string("..."), but Zod’s string factory doesn’t take a description string overload—use.describe(...)like the ROR tool does.Suggested fix
inputSchema: z.object({ - identifier: z.string("A properly formatted ORCID identifier or ORCID URL") + identifier: z.string().describe("A properly formatted ORCID identifier or ORCID URL") }),🤖 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 `@lib/ai/tools.ts` around lines 51 - 53, The inputSchema for importPersonFromORCID incorrectly passes a description string directly to z.string; change inputSchema.identifier to call z.string() with no arguments and then chain .describe("A properly formatted ORCID identifier or ORCID URL") (mirroring the ROR tool pattern) so the schema uses Zod's .describe API correctly.components/ai/configure-provider.tsx-41-56 (1)
41-56:⚠️ Potential issue | 🟠 Major | ⚡ Quick winResync the form state when
existingConfigchanges.These
useStateinitializers only run on the first mount. If the dialog stays mounted and is reopened for a different provider, the form will keep the previous provider's values and save the wrong configuration unless you reset fromexistingConfigin an effect or remount by key.🤖 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 `@components/ai/configure-provider.tsx` around lines 41 - 56, The form state is only initialized once and must be resynced when existingConfig changes; add a useEffect that watches existingConfig and updates all local state setters (setConfigureProvider, setConfigureDisplayName, setConfigureBaseUrl, setConfigureHeaders, setConfigureModels, setConfigureAPIKey) and resets transient flags (setConfigureError, setProviderTestedSuccessfully, setTestingNewProvider, setFetchingModels as appropriate) so the dialog reflects the new existingConfig whenever it is reopened or changed.components/ui/record.tsx-64-66 (1)
64-66:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGive the row delete button an accessible name.
The destructive action only renders a
TrashIcon, so assistive tech gets an unlabeled button for every row. Add anaria-labelthat includesitemNameand the row index so users can tell what will be removed.🤖 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 `@components/ui/record.tsx` around lines 64 - 66, The delete button renders only a TrashIcon and lacks an accessible name; update the Button that calls removePair(i) to include an aria-label that identifies the item and row (use the existing itemName and the index i, e.g. `Remove ${itemName} row ${i + 1}`) so screen readers can announce what will be removed; keep the onClick handler (removePair) and the TrashIcon as-is, only add the aria-label prop to Button.proxy.ts-32-48 (1)
32-48:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not derive the trusted origin from request-controlled headers.
getAllowedOrigin()builds the allowlist fromHostandx-forwarded-proto, thenvalidateOrigin()compares the request'sOriginto that derived value. On any deployment that forwards those headers unchanged, a client can send matchingHost/Originvalues and satisfy the check. Compare against a server-configured origin allowlist instead.🤖 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 `@proxy.ts` around lines 32 - 48, The origin validation currently builds the allowed origin from request-controlled headers in getAllowedOrigin and compares it in proxy via validateOrigin; instead, stop deriving trusted origins from req headers and validate against a server-configured allowlist (e.g., an environment variable or a constant loaded at startup). Replace getAllowedOrigin usage with a function or constant that reads allowed origins from config (supporting a list), then update proxy to call validateOrigin(origin, serverAllowedOrigins) and reject if not matched; ensure no logic uses req.headers ("host" or "x-forwarded-proto") to form the trusted origin.lib/ai/providers/OpenRouterAdapter.ts-75-79 (1)
75-79:⚠️ Potential issue | 🟠 MajorFix createOpenRouter base URL option name (
baseURL, notbaseUrl)
createOpenRouterin@openrouter/ai-sdk-providerexpects the optionbaseURL; the adapter currently passesbaseUrl, so the custom base URL override may not apply to real requests. Update the call (and related config naming) to usebaseURL.🤖 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 `@lib/ai/providers/OpenRouterAdapter.ts` around lines 75 - 79, The adapter passes the wrong option name to createOpenRouter—change the option from baseUrl to baseURL when calling createOpenRouter in OpenRouterAdapter so the custom base URL override is applied; update any references in the adapter (e.g., this.config.baseUrl -> this.config.baseURL or map this.config.baseUrl to the baseURL option) to ensure the config uses the provider-expected key while preserving backwards compatibility if needed.components/ai/configure-provider.tsx-58-71 (1)
58-71:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve the existing
selectedModelwhen editing a provider.
makeProviderConfig()always setsselectedModeltomodels[0]?.id. Editing any existing provider will therefore overwrite the user's persisted model choice even when that model is still present in the sanitized list.Suggested fix
return { id: existingConfig?.id ?? window.crypto.randomUUID(), models, - selectedModel: models.length > 0 ? models[0].id : undefined, + selectedModel: + existingConfig?.selectedModel && + models.some((model) => model.id === existingConfig.selectedModel) + ? existingConfig.selectedModel + : models.length > 0 + ? models[0].id + : undefined, provider: configureProvider as LanguageModelProvider,🤖 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 `@components/ai/configure-provider.tsx` around lines 58 - 71, makeProviderConfig currently always sets selectedModel to models[0]?.id which overwrites a persisted choice; change it so selectedModel uses existingConfig?.selectedModel if that id exists in the sanitized models array (i.e., check models.some(m => m.id === existingConfig?.selectedModel)), otherwise fall back to models.length > 0 ? models[0].id : undefined; update the selectedModel assignment inside makeProviderConfig accordingly and keep the rest of the returned ProviderConfiguration unchanged.lib/state/layout-state.ts-15-27 (1)
15-27:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't let persisted state override the AI feature flag.
showAIAssistantis seeded fromNEXT_PUBLIC_AI_ASSISTANT_ENABLED, but the whole store is persisted under"layout-state". If a browser already hasshowAIAssistant: true, turning the env flag off later will still rehydrate the assistant as visible in that client. Keep the feature gate derived from config, or clamp the rehydrated value with the env flag during persistence merge.🤖 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 `@lib/state/layout-state.ts` around lines 15 - 27, Persisted store "layout-state" currently allows rehydrated showAIAssistant to override the runtime feature flag; change the persist merge/rehydration logic (the persist call wrapping immer(...)) so showAIAssistant is clamped to the env-derived flag (compute const envFlag = process.env.NEXT_PUBLIC_AI_ASSISTANT_ENABLED === "true") and on rehydrate/merge set showAIAssistant = envFlag && (persisted.showAIAssistant ?? false) or simply envFlag if you want the server config to fully control it; update the same area where showAIAssistant and setShowAIAssistant are defined to use envFlag as the source of truth and ensure persisted data cannot enable the assistant when envFlag is false.lib/validation/validation-result.ts-44-46 (1)
44-46:⚠️ Potential issue | 🟠 MajorHarden
helpUrlvalidation to only allow http/https URLs
ValidationResultLineopensresult.helpUrlviawindow.open(result.helpUrl, "_blank"), butValidationResultSchemaonly checks thathelpUrlis a string. BecauseisValidUrlis permissive (new URL()acceptsjavascript:,data:,file:, etc.), malformed/unsafe schemes can slip through parsing. Validate at the schema boundary with an HTTP/HTTPS-only schema (e.g.z.httpUrl()):helpUrl: z .optional(z.string()) .check(z.describe("The URL to a help page for this validation result")),Update
lib/validation/validation-result.tssohelpUrlusesz.httpUrl()(and keep the existing describe check).🤖 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 `@lib/validation/validation-result.ts` around lines 44 - 46, Replace the permissive string validator for helpUrl in the ValidationResultSchema with an HTTP/HTTPS-only URL validator: find the helpUrl field (currently using helpUrl: z.optional(z.string()).check(z.describe("The URL to a help page for this validation result"))) and change it to use z.httpUrl() inside the optional, preserving the existing describe check so it becomes helpUrl: z.optional(z.httpUrl()).check(z.describe("The URL to a help page for this validation result"))); this hardens the schema used by ValidationResultLine (which calls window.open(result.helpUrl, "_blank")) to allow only http/https schemes.components/file-explorer/file-preview-tabs.tsx-61-69 (1)
61-69:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake the tab close affordance a real button.
The close control is currently a clickable
<div>, so it is not reachable from the keyboard and has no accessible name for assistive tech. That makes closing tabs from the strip inaccessible for keyboard-only users.🤖 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 `@components/file-explorer/file-preview-tabs.tsx` around lines 61 - 69, Replace the clickable <div> with a real button element for accessibility: render a <button type="button"> that preserves the existing onClick handler (calling close() and e.stopPropagation()), keep the same className string and XIcon component, and add an accessible name via aria-label="Close tab" (and optionally title="Close tab") so keyboard and assistive tech can operate it; ensure you do not introduce a submit button by using type="button".
🧹 Nitpick comments (4)
docker-compose.yml (1)
3-3: ⚡ Quick winPin the compose image to the release tag.
ghcr.io/kit-data-manager/novacrate:1will drift to future 1.x releases, so this file will stop reproducing thev1.12.0artifact this PR is shipping.Based on the PR objective, this file is part of the
v1.12.0release surface.🤖 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 `@docker-compose.yml` at line 3, The docker-compose image tag is pinned to the floating major tag image: "ghcr.io/kit-data-manager/novacrate:1" which will drift; change that value to the exact release tag (e.g. ghcr.io/kit-data-manager/novacrate:v1.12.0) so the compose file reproduces the v1.12.0 artifact, and update any other occurrences of the same image string in the file or repo to the same exact tag.lib/ai/utils.ts (1)
23-68: ⚡ Quick winDuplicate error handling logic in
testProviderandfetchModels.Both functions have identical error handling logic (lines 35-42 and 59-66). Extract this into a shared helper to improve maintainability.
♻️ Proposed refactor
+async function handleApiError(res: Response): Promise<never> { + let error + try { + const data: { error: string } = await res.json() + error = data.error + } catch (e) { + error = res.statusText + } + throw new Error(error) +} + export async function testProvider(config: ProviderConfiguration) { const res = await fetch(addBasePath("/api/ai/test"), { body: JSON.stringify({ config }), headers: { "Content-Type": "application/json" }, method: "POST" }) if (res.ok) { return } else { - let error - try { - const data: { error: string } = await res.json() - error = data.error - } catch (e) { - error = res.statusText - } - throw new Error(error) + await handleApiError(res) } } export async function fetchModels(config: ProviderConfiguration) { const res = await fetch(addBasePath("/api/ai/models"), { body: JSON.stringify({ config }), headers: { "Content-Type": "application/json" }, method: "POST" }) if (res.ok) { const data: { models: TextModel[] } = await res.json() return data.models } else { - let error - try { - const data: { error: string } = await res.json() - error = data.error - } catch (e) { - error = res.statusText - } - throw new Error(error) + await handleApiError(res) } }🤖 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 `@lib/ai/utils.ts` around lines 23 - 68, Extract the duplicated error-parsing logic used in testProvider and fetchModels into a shared helper (e.g., parseErrorFromResponse or getResponseError) that accepts a Response and returns/throws the proper Error message; replace the duplicate try/catch blocks in testProvider and fetchModels with calls to that helper and ensure the helper does the await res.json() attempt and falls back to res.statusText before throwing or returning the error string.app/api/ai/models/route.ts (1)
18-18: Schema already requiresapiKey; extra check only rejects empty strings
ProviderConfigurationWithoutModelsSchemaomitsmodelsfromProviderConfigurationSchema, butProviderConfigurationSchemadefinesapiKey: z.string()(required). So this guard is redundant for missing/undefinedapiKey; it only adds a “non-empty” constraint (e.g.,""). Prefer enforcing that in the Zod schema (e.g.,z.string().nonempty()/min(1)) and then removing the runtimeif (!config.apiKey)check.File: app/api/ai/models/route.ts
Lines: 18-18Current snippet
if (!config.apiKey) return Response.json({ error: "No API key provided" }, { status: 400 })🤖 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 `@app/api/ai/models/route.ts` at line 18, The runtime guard in route.ts that returns "No API key provided" is redundant because the Zod schema should enforce a non-empty apiKey; update ProviderConfigurationSchema (or ProviderConfigurationWithoutModelsSchema if that's used for this route) to use z.string().nonempty() or .min(1) for the apiKey field, remove the runtime check (the if (!config.apiKey) return ...) from app/api/ai/models/route.ts, and rely on the validated config object instead; ensure any callers using ProviderConfigurationWithoutModelsSchema are adjusted to require a non-empty apiKey as needed.lib/ai/providers/ProviderFactory.ts (1)
27-40: ⚡ Quick winAdd explicit return types to the public factory helpers.
These methods currently rely on inference, which makes accidental contract drift harder to catch on a public factory surface.
As per coding guidelines,
Use explicit types for function parameters and return values in TypeScript; based on learnings,TypeScript strict mode must be enabled for the project.Suggested fix
- makeOpenRouterAdapter(config: ProviderConfigurationWithoutModels) { + makeOpenRouterAdapter(config: ProviderConfigurationWithoutModels): IProviderAdapter { return new OpenRouterAdapter(config) } - makeOpenAICompatibleAdapter(config: ProviderConfigurationWithoutModels) { + makeOpenAICompatibleAdapter( + config: ProviderConfigurationWithoutModels + ): IProviderAdapter { return new OpenAICompatibleAdapter(config) } - makeOpenAIAdapter(config: ProviderConfigurationWithoutModels) { + makeOpenAIAdapter(config: ProviderConfigurationWithoutModels): IProviderAdapter { return new OpenAIAdapter(config) } - makeAnthropicAdapter(config: ProviderConfigurationWithoutModels) { + makeAnthropicAdapter(config: ProviderConfigurationWithoutModels): IProviderAdapter { return new AnthropicAdapter(config) }🤖 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 `@lib/ai/providers/ProviderFactory.ts` around lines 27 - 40, The four public factory methods makeOpenRouterAdapter, makeOpenAICompatibleAdapter, makeOpenAIAdapter, and makeAnthropicAdapter should declare explicit return types instead of relying on inference; update their signatures to return the concrete adapter types (OpenRouterAdapter, OpenAICompatibleAdapter, OpenAIAdapter, AnthropicAdapter respectively) while keeping the existing ProviderConfigurationWithoutModels parameter type so the factory surface has an explicit, stable contract that TypeScript strict mode can verify.Sources: Coding guidelines, Learnings
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7987074d-5e86-451c-991c-b7d9ee86bc59
⛔ Files ignored due to path filters (5)
docs/figures/ai-assistant-setup/find-ai-assistant.pngis excluded by!**/*.pngdocs/figures/ai-assistant-setup/initial-settings.pngis excluded by!**/*.pngdocs/figures/ai-assistant-setup/post-model-fetch.pngis excluded by!**/*.pngdocs/figures/ai-assistant-setup/pre-model-fetch.pngis excluded by!**/*.pngpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (85)
.dockerignore.env.example.github/workflows/docker.yml.npmrcAGENTS.mdCHANGELOG.mdDockerfileREADME.mdapp/api/ai/chat/route.tsapp/api/ai/models/route.tsapp/api/ai/test/route.tsapp/editor/[mode]/entities/page.tsxapp/editor/[mode]/file-explorer/page.tsxcomponents/actions/default-actions.tsxcomponents/ai/chat.tsxcomponents/ai/configure-provider.tsxcomponents/ai/input.tsxcomponents/ai/model-selection.tsxcomponents/ai/tool-call.tsxcomponents/changelog-modal.tsxcomponents/editor/entity-editor-tabs.tsxcomponents/editor/entity-editor.tsxcomponents/editor/validation/single-property-validation.tsxcomponents/editor/validation/validation-overview.tsxcomponents/editor/validation/validation-result-line.tsxcomponents/file-explorer/entry-context-menu.tsxcomponents/file-explorer/explorer-node.tsxcomponents/file-explorer/explorer.tsxcomponents/file-explorer/file-preview-tabs.tsxcomponents/file-explorer/preview.tsxcomponents/file-explorer/utils.tscomponents/file-explorer/viewers/base.tsxcomponents/file-explorer/viewers/iframe.tsxcomponents/file-explorer/viewers/image.tsxcomponents/file-explorer/viewers/large-view-select.tsxcomponents/file-explorer/viewers/object.tsxcomponents/file-explorer/viewers/text.tsxcomponents/modals/settings/ai-assistant.tsxcomponents/modals/settings/settings-modal.tsxcomponents/nav/nav-drawer.tsxcomponents/nav/nav-header.tsxcomponents/nav/nav-sidebar.tsxcomponents/providers/core-provider.tsxcomponents/providers/global-modals-provider.tsxcomponents/ui/record.tsxcomponents/ui/select.tsxcomponents/validation-drawer.tsxdocker-compose.build.ymldocker-compose.ymldocs/ai-assistant-setup.mdlib/ai/providers/AnthropicAdapter.tslib/ai/providers/IProviderAdapter.tslib/ai/providers/OpenAIAdapter.tslib/ai/providers/OpenAICompatibleAdapter.tslib/ai/providers/OpenRouterAdapter.tslib/ai/providers/ProviderFactory.tslib/ai/providers/sanitize-headers.tslib/ai/providers/validate-base-url.tslib/ai/rate-limiter.tslib/ai/tools.tslib/ai/utils.tslib/ai/validate-origin.tslib/core/util.tslib/file-preview.tslib/hooks/hooks.tslib/state/ai-assistant-settings.tslib/state/editor-state.tslib/state/file-explorer-state.tslib/state/layout-state.tslib/validation/validation-result.tslib/validation/validators/rules/ro-crate-v1.2.tsnext.config.mjspackage.jsonproxy.tsrenovate.jsontests/data/elabFtw.elntests/e2e/file-explorer.test.tstests/e2e/importing.test.tstests/unit/lib/ai/rate-limiter.test.tstests/unit/lib/ai/sanitize-headers.test.tstests/unit/lib/ai/validate-base-url.test.tstests/unit/lib/ai/validate-origin.test.tstests/unit/lib/core/util.test.tstests/unit/lib/state/file-explorer-state.test.tstsconfig.json
💤 Files with no reviewable changes (3)
- components/file-explorer/explorer.tsx
- tests/e2e/importing.test.ts
- lib/core/util.ts
Fix explorer button
Remove object viewer
# Conflicts: # CHANGELOG.md
… that the ai reads an entity before editing
Improvements for the AI Assistant
# Conflicts: # components/ai/chat.tsx
1 participant
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores