Minimize API Calls Across Config/Evaluation Flows#83
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new client hook Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant UI as Config Dropdown / Editor
participant Hook as useConfigs Hook
participant Store as Config Store
participant API as Config API
User->>UI: Open versions dropdown / Expand group
UI->>Hook: loadVersionsForConfig(config_id)
Hook->>Store: check pendingVersionLoads[config_id]
alt pending exists
Store-->>Hook: return pending promise
else
Hook->>API: GET /api/configs/{id}/versions
API-->>Hook: return versionItems
Hook->>Store: cache versionItemsMap[config_id]
end
Hook-->>UI: resolve with versionItems
UI->>User: render versions list
sequenceDiagram
actor User
participant Page as Configs Page
participant Hook as useConfigs Hook
participant Store as Config Store
participant API as /api/configs
User->>Page: initial mount (useConfigs({pageSize:10}))
Page->>Hook: request configs
Hook->>Store: check inMemoryCache / localStorage
alt cache fresh
Store-->>Hook: return cached batch
else
Hook->>API: GET /api/configs (metadata)
API-->>Hook: configMeta
Hook->>API: GET /api/configs/{id}/versions (batched)
API-->>Hook: versionItems per config
Hook->>API: GET /api/configs/{id}/versions/{latest}
API-->>Hook: latest SavedConfig items
Hook->>Store: persist cache & versionItems
end
Hook-->>Page: return configs + hasMoreConfigs
User->>Page: Click "Load More"
Page->>Hook: loadMoreConfigs()
Hook->>API: fetch next batch versions/details
Hook-->>Page: append new configs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
🧹 Nitpick comments (7)
app/configurations/page.tsx (1)
26-26: Consider using thePAGE_SIZEconstant.The hardcoded
pageSize: 10duplicates the value defined inapp/lib/constants.ts. Using the constant would ensure consistency if the value changes.♻️ Optional improvement
+import { PAGE_SIZE } from '@/app/lib/constants'; // ... -const { configGroups, isLoading, error, refetch, isCached, loadMoreConfigs, hasMoreConfigs, isLoadingMore } = useConfigs({ pageSize: 10 }); +const { configGroups, isLoading, error, refetch, isCached, loadMoreConfigs, hasMoreConfigs, isLoadingMore } = useConfigs({ pageSize: PAGE_SIZE });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/configurations/page.tsx` at line 26, Replace the hardcoded pageSize value with the shared constant to avoid duplication: in the call to useConfigs({ pageSize: 10 }) change the object to use PAGE_SIZE from app/lib/constants.ts (import PAGE_SIZE at the top if not already). This updates the useConfigs invocation (function symbol: useConfigs) to use PAGE_SIZE so the component automatically follows the centralized constant.app/components/prompt-editor/ConfigEditorPane.tsx (1)
74-80: Consider addingversionsFullyLoadedcheck for consistency.Unlike
ConfigSelector(which checks!group.versionsFullyLoadedbefore calling), this implementation callsloadVersionsForConfigfor every config group unconditionally. WhileloadVersionsForConfighas internal caching guards that prevent redundant API calls, adding the same check here would be more consistent and make the intent clearer.♻️ Suggested improvement
const handleOpenLoadDropdown = () => { // When opening, trigger lazy loading for any config that only has its latest version if (!isDropdownOpen && loadVersionsForConfig) { - configGroups.forEach(group => loadVersionsForConfig(group.config_id)); + configGroups.forEach(group => { + if (group.versions.length < (group as any).totalVersions) { + loadVersionsForConfig(group.config_id); + } + }); } setIsDropdownOpen(prev => !prev); };Note: The
ConfigGroupForDropdowninterface doesn't haveversionsFullyLoadedortotalVersions. You may want to either extend the interface or rely on the hook's internal caching (current behavior is safe).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/prompt-editor/ConfigEditorPane.tsx` around lines 74 - 80, The handleOpenLoadDropdown function calls loadVersionsForConfig for every configGroup when opening the dropdown; update it to only call loadVersionsForConfig for groups that are not already fully loaded (check group.versionsFullyLoaded) to match ConfigSelector's behavior and make intent explicit—i.e., inside the configGroups.forEach loop only invoke loadVersionsForConfig(group.config_id) when !group.versionsFullyLoaded; if the ConfigGroupForDropdown type lacks versionsFullyLoaded/totalVersions, either extend that interface to include versionsFullyLoaded or document the reliance on the hook's caching before adding the conditional.app/lib/store/configStore.ts (1)
84-93: Consider resettingallConfigMetainclearConfigCache.The function clears
inMemoryCacheandversionItemsCachebut leavesallConfigMetaintact. While the current flow handles this (the event triggers a force refetch which repopulatesallConfigMeta), explicitly clearing it would make the invalidation more complete and prevent potential edge cases with stale metadata.♻️ Proposed fix
export const clearConfigCache = (): void => { if (typeof window === 'undefined') return; try { localStorage.removeItem(CACHE_KEY); configState.inMemoryCache = null; configState.versionItemsCache = {}; + configState.allConfigMeta = null; } catch (e) { console.error('Failed to clear config cache:', e); } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/lib/store/configStore.ts` around lines 84 - 93, The clearConfigCache function currently removes CACHE_KEY, resets configState.inMemoryCache and configState.versionItemsCache but leaves configState.allConfigMeta intact; update clearConfigCache to also reset configState.allConfigMeta (e.g., set to an empty object or null) so that metadata is invalidated consistently alongside inMemoryCache and versionItemsCache; locate the function clearConfigCache and modify the configState object mutation to include resetting allConfigMeta.app/hooks/useConfigs.ts (1)
376-389: Consider memoizinggroupConfigscall.The
groupConfigs(configs, versionCounts, versionItemsMap)call on line 378 runs on every render. Consider usinguseMemoto avoid recalculating when inputs haven't changed.♻️ Proposed fix
+import { useState, useEffect, useCallback, useMemo } from 'react'; // ... in the hook body + const configGroups = useMemo( + () => groupConfigs(configs, versionCounts, versionItemsMap), + [configs, versionCounts, versionItemsMap], + ); + return { configs, - configGroups: groupConfigs(configs, versionCounts, versionItemsMap), + configGroups, isLoading,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/hooks/useConfigs.ts` around lines 376 - 389, In useConfigs, avoid recalculating groupConfigs on every render by wrapping the call to groupConfigs(configs, versionCounts, versionItemsMap) in a useMemo; compute const configGroups = useMemo(() => groupConfigs(configs, versionCounts, versionItemsMap), [configs, versionCounts, versionItemsMap]) (or equivalent) and return configGroups instead of calling groupConfigs inline so the grouping only recomputes when inputs change.app/lib/utils.ts (1)
59-72: Consider extracting the localStorage key to a constant.The hardcoded
'kaapi_api_keys'string should be defined as a constant for maintainability, consistent with other localStorage keys in the codebase. Based on learnings: "Avoid hardcoding localStorage keys; define them as constants in utility files for maintainability."♻️ Proposed fix
+// In app/lib/constants.ts +export const API_KEYS_STORAGE_KEY = 'kaapi_api_keys'; // In app/lib/utils.ts +import { API_KEYS_STORAGE_KEY } from './constants'; export const getApiKey = (): string | null => { if (typeof window === 'undefined') return null; try { - const stored = localStorage.getItem('kaapi_api_keys'); + const stored = localStorage.getItem(API_KEYS_STORAGE_KEY); if (stored) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/lib/utils.ts` around lines 59 - 72, Extract the hardcoded localStorage key string 'kaapi_api_keys' into a module-level constant (e.g., API_KEYS_STORAGE_KEY) in app/lib/utils.ts and replace the literal in getApiKey with that constant; update any other functions in the same file that reference the same literal to use the constant as well so key usage is centralized and maintainable (target symbol: getApiKey and the localStorage.getItem call).app/lib/configFetchers.ts (1)
193-209: Consider batching parallel version fetches for configs with many versions.The
Promise.allonmissingVersionscould trigger many concurrent requests if a config has many versions (e.g., 50+ versions). Consider applying the same batching pattern used infetchAllConfigs(BATCH_SIZE=5) for consistency.♻️ Proposed batched approach
+ const BATCH_SIZE = 5; + const fetchedVersions: (SavedConfig | null)[] = []; + + for (let i = 0; i < missingVersions.length; i += BATCH_SIZE) { + const batch = missingVersions.slice(i, i + BATCH_SIZE); + const batchResults = await Promise.all( + batch.map(async (versionItem) => { - const fetchedVersions = await Promise.all( - missingVersions.map(async (versionItem) => { try { const versionResponse = await fetch( `/api/configs/${config_id}/versions/${versionItem.version}`, { headers: { 'X-API-KEY': apiKey } }, ); const versionData: ConfigVersionResponse = await versionResponse.json(); if (versionData.success && versionData.data) { return flattenConfigVersion(config, versionData.data); } } catch (e) { console.error(`Failed to fetch version ${versionItem.version} for config ${config_id}:`, e); } return null; - }), - ); + }), + ); + fetchedVersions.push(...batchResults); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/lib/configFetchers.ts` around lines 193 - 209, The current Promise.all over missingVersions (creating fetchedVersions) can open hundreds of concurrent requests; change it to a batched fetch similar to fetchAllConfigs using a BATCH_SIZE constant (e.g., 5): split missingVersions into chunks, iterate chunks sequentially and for each chunk run Promise.all for that chunk to call fetch(`/api/configs/${config_id}/versions/${versionItem.version}`) and call flattenConfigVersion on successful responses, preserving the existing try/catch/error logging behavior; collect results into fetchedVersions (keeping nulls for failures) and return the flattened array as before. Use the same BATCH_SIZE symbol and mirror the batching logic used in fetchAllConfigs to ensure consistency.app/components/prompt-editor/DiffView.tsx (1)
135-140: Consider debouncing or limiting eager version loading on focus.Calling
loadVersionsForConfigfor all config groups on every focus could trigger multiple API calls when many configs exist. While the hook deduplicates concurrent calls, consider whether this eager approach is necessary or if lazy loading per-group on expansion would be more efficient.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/prompt-editor/DiffView.tsx` around lines 135 - 140, The onFocus handler in DiffView.tsx currently calls loadVersionsForConfig for every item in configGroups, which can fire many API requests; change this to a limited/debounced strategy: either debounce the onFocus call (so repeated focuses within short windows trigger a single batch) or replace eager loading with lazy loading per group (call loadVersionsForConfig(g.config_id) only when a group is expanded/visible). Update the handler that references loadVersionsForConfig and configGroups (and any UI expand/visibility toggle handlers) to perform per-group loading on expansion or to enqueue config_ids and call loadVersionsForConfig in a single batched/debounced invocation. Ensure deduplication remains in place and avoid triggering loads for all configGroups on every focus.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/components/prompt-editor/ConfigEditorPane.tsx`:
- Around line 74-80: The handleOpenLoadDropdown function calls
loadVersionsForConfig for every configGroup when opening the dropdown; update it
to only call loadVersionsForConfig for groups that are not already fully loaded
(check group.versionsFullyLoaded) to match ConfigSelector's behavior and make
intent explicit—i.e., inside the configGroups.forEach loop only invoke
loadVersionsForConfig(group.config_id) when !group.versionsFullyLoaded; if the
ConfigGroupForDropdown type lacks versionsFullyLoaded/totalVersions, either
extend that interface to include versionsFullyLoaded or document the reliance on
the hook's caching before adding the conditional.
In `@app/components/prompt-editor/DiffView.tsx`:
- Around line 135-140: The onFocus handler in DiffView.tsx currently calls
loadVersionsForConfig for every item in configGroups, which can fire many API
requests; change this to a limited/debounced strategy: either debounce the
onFocus call (so repeated focuses within short windows trigger a single batch)
or replace eager loading with lazy loading per group (call
loadVersionsForConfig(g.config_id) only when a group is expanded/visible).
Update the handler that references loadVersionsForConfig and configGroups (and
any UI expand/visibility toggle handlers) to perform per-group loading on
expansion or to enqueue config_ids and call loadVersionsForConfig in a single
batched/debounced invocation. Ensure deduplication remains in place and avoid
triggering loads for all configGroups on every focus.
In `@app/configurations/page.tsx`:
- Line 26: Replace the hardcoded pageSize value with the shared constant to
avoid duplication: in the call to useConfigs({ pageSize: 10 }) change the object
to use PAGE_SIZE from app/lib/constants.ts (import PAGE_SIZE at the top if not
already). This updates the useConfigs invocation (function symbol: useConfigs)
to use PAGE_SIZE so the component automatically follows the centralized
constant.
In `@app/hooks/useConfigs.ts`:
- Around line 376-389: In useConfigs, avoid recalculating groupConfigs on every
render by wrapping the call to groupConfigs(configs, versionCounts,
versionItemsMap) in a useMemo; compute const configGroups = useMemo(() =>
groupConfigs(configs, versionCounts, versionItemsMap), [configs, versionCounts,
versionItemsMap]) (or equivalent) and return configGroups instead of calling
groupConfigs inline so the grouping only recomputes when inputs change.
In `@app/lib/configFetchers.ts`:
- Around line 193-209: The current Promise.all over missingVersions (creating
fetchedVersions) can open hundreds of concurrent requests; change it to a
batched fetch similar to fetchAllConfigs using a BATCH_SIZE constant (e.g., 5):
split missingVersions into chunks, iterate chunks sequentially and for each
chunk run Promise.all for that chunk to call
fetch(`/api/configs/${config_id}/versions/${versionItem.version}`) and call
flattenConfigVersion on successful responses, preserving the existing
try/catch/error logging behavior; collect results into fetchedVersions (keeping
nulls for failures) and return the flattened array as before. Use the same
BATCH_SIZE symbol and mirror the batching logic used in fetchAllConfigs to
ensure consistency.
In `@app/lib/store/configStore.ts`:
- Around line 84-93: The clearConfigCache function currently removes CACHE_KEY,
resets configState.inMemoryCache and configState.versionItemsCache but leaves
configState.allConfigMeta intact; update clearConfigCache to also reset
configState.allConfigMeta (e.g., set to an empty object or null) so that
metadata is invalidated consistently alongside inMemoryCache and
versionItemsCache; locate the function clearConfigCache and modify the
configState object mutation to include resetting allConfigMeta.
In `@app/lib/utils.ts`:
- Around line 59-72: Extract the hardcoded localStorage key string
'kaapi_api_keys' into a module-level constant (e.g., API_KEYS_STORAGE_KEY) in
app/lib/utils.ts and replace the literal in getApiKey with that constant; update
any other functions in the same file that reference the same literal to use the
constant as well so key usage is centralized and maintainable (target symbol:
getApiKey and the localStorage.getItem call).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7141abe5-7502-4011-8bc2-693d773aeaa6
📒 Files selected for processing (16)
app/components/ConfigCard.tsxapp/components/ConfigSelector.tsxapp/components/prompt-editor/ConfigDiffPane.tsxapp/components/prompt-editor/ConfigEditorPane.tsxapp/components/prompt-editor/DiffView.tsxapp/components/prompt-editor/HistorySidebar.tsxapp/components/prompt-editor/PromptDiffPane.tsxapp/configurations/page.tsxapp/configurations/prompt-editor/page.tsxapp/hooks/useConfigs.tsapp/lib/configFetchers.tsapp/lib/constants.tsapp/lib/store/configStore.tsapp/lib/types/configs.tsapp/lib/useConfigs.tsapp/lib/utils.ts
💤 Files with no reviewable changes (1)
- app/lib/useConfigs.ts
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (6)
app/lib/utils.ts (2)
16-23: Misplaced comment.The comment "Format timestamp as relative time / Handles UTC timestamps..." appears above
getExistingForProviderbut describesformatRelativeTimewhich is defined at line 25.📝 Proposed fix
-// Format timestamp as relative time -// Handles UTC timestamps from the database and converts them to local time export function getExistingForProvider( provider: ProviderDef, creds: Credential[], ): Credential | null { return creds.find((c) => c.provider === provider.credentialKey) || null; } +// Format timestamp as relative time +// Handles UTC timestamps from the database and converts them to local time export const formatRelativeTime = (timestamp: string | number): string => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/lib/utils.ts` around lines 16 - 23, The comment about formatting timestamps is misplaced above getExistingForProvider; move or copy that comment so it sits immediately above the formatRelativeTime function instead of above getExistingForProvider. Locate getExistingForProvider and formatRelativeTime in app/lib/utils.ts and reposition the comment text ("Format timestamp as relative time / Handles UTC timestamps from the database and converts them to local time") to precede formatRelativeTime so comments match the related implementation.
57-70: Consider extracting the localStorage key to a constant.The
kaapi_api_keysstring is hardcoded. Based on learnings, localStorage keys should be defined as constants in utility files for maintainability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/lib/utils.ts` around lines 57 - 70, Extract the hardcoded 'kaapi_api_keys' string into a named constant (e.g., API_KEYS_STORAGE_KEY) in the same module and update getApiKey to use that constant; modify the top of app/lib/utils.ts to declare the constant and replace occurrences of the literal in the getApiKey function so future changes and reuse are centralized.app/hooks/useConfigs.ts (1)
387-389:configGroupsis recomputed on every render.
groupConfigs(configs, versionCounts, versionItemsMap)is called directly in the return statement, meaning it runs on every render even when the inputs haven't changed. Consider memoizing for better performance with large config lists.♻️ Proposed optimization using useMemo
+import { useState, useEffect, useCallback, useMemo } from 'react'; + const configGroups = useMemo( + () => groupConfigs(configs, versionCounts, versionItemsMap), + [configs, versionCounts, versionItemsMap] + ); return { configs, - configGroups: groupConfigs(configs, versionCounts, versionItemsMap), + configGroups, isLoading,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/hooks/useConfigs.ts` around lines 387 - 389, configGroups is being recomputed on every render because groupConfigs(configs, versionCounts, versionItemsMap) is called directly in the return; wrap that call in a memo so it only recalculates when inputs change — e.g. create a memoized variable (e.g. const configGroups = useMemo(() => groupConfigs(configs, versionCounts, versionItemsMap), [configs, versionCounts, versionItemsMap])) and return that instead of calling groupConfigs inline; update the return to use the memoized configGroups so renders don't repeatedly run groupConfigs.app/components/prompt-editor/HistorySidebar.tsx (1)
98-111: Duplicate relative time formatting logic.The
formatTimestampfunction here duplicates the logic informatRelativeTimefromapp/lib/utils.ts. Consider using the shared utility for consistency.♻️ Proposed refactor
+import { formatRelativeTime } from '@/app/lib/utils'; - // Format timestamp - calculate relative time from UTC timestamps - const formatTimestamp = (timestamp: string) => { - const now = Date.now(); - const date = new Date(timestamp).getTime(); - const diff = now - date; - const minutes = Math.floor(diff / 60000); - const hours = Math.floor(diff / 3600000); - const days = Math.floor(diff / 86400000); - - if (minutes < 1) return 'just now'; - if (minutes < 60) return `${minutes} min ago`; - if (hours < 24) return `${hours} hr ago`; - return `${days} day${days > 1 ? 's' : ''} ago`; - }; + const formatTimestamp = formatRelativeTime;Note: The output format differs slightly (
minvsm,hrvsh). You may want to standardize on one format across the app.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/prompt-editor/HistorySidebar.tsx` around lines 98 - 111, The local formatTimestamp function duplicates relative-time logic already implemented in formatRelativeTime; replace formatTimestamp by importing and using formatRelativeTime (instead of the local function) in HistorySidebar, remove the duplicated formatTimestamp implementation, and update any call sites in this component to call formatRelativeTime(<timestamp>) so the shared utility is used consistently; also ensure you reconcile output formats if needed (adjust formatRelativeTime or convert its output here) so the displayed units match the desired "min/hr" style.app/components/prompt-editor/ConfigEditorPane.tsx (1)
534-551: No loading indicator when fetching version details on selection.When the user clicks a version that isn't already loaded,
loadSingleVersionis called but there's no visual feedback. The dropdown remains open and clickable, potentially allowing multiple clicks.💡 Consider adding loading state similar to ConfigSelector
You could track a
fetchingVersionKeystate (likeConfigSelectordoes) and disable the button or show a spinner while the fetch is in progress.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/prompt-editor/ConfigEditorPane.tsx` around lines 534 - 551, The onClick handler for version items (inside ConfigEditorPane) calls loadSingleVersion without any loading state, so users can click multiple times and the dropdown stays interactive; add a fetchingVersionKey (string | null) state in ConfigEditorPane, set it to a unique key (e.g., `${item.config_id}:${item.version}`) before awaiting loadSingleVersion, disable the version button and/or show a spinner when fetchingVersionKey matches that key, and clear fetchingVersionKey in finally; ensure you still call onLoadConfig(config) and setIsDropdownOpen(false) only after a successful load and handle errors (log/show) while preventing duplicate clicks during the fetch.app/components/prompt-editor/DiffView.tsx (1)
135-140: Loading versions for all configs on every focus is inefficient.When the dropdown receives focus,
loadVersionsForConfigis called for every config group. If there are many configs, this triggers multiple simultaneous API calls, even for configs the user may never expand.♻️ Consider lazy-loading only on group expansion
Instead of loading all version lists on focus, consider loading them only when a specific group is expanded (similar to
ConfigSelectorandConfigEditorPane). Alternatively, add a guard to skip already-loaded configs:onFocus={() => { - // Ensure lightweight version lists are populated for all configs if (loadVersionsForConfig) { - configGroups.forEach(g => loadVersionsForConfig(g.config_id)); + // Only load for configs not yet in versionItemsMap + configGroups.forEach(g => { + if (!versionItemsMap?.[g.config_id]) { + loadVersionsForConfig(g.config_id); + } + }); } }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/prompt-editor/DiffView.tsx` around lines 135 - 140, The onFocus handler currently calls loadVersionsForConfig for every entry in configGroups causing many unnecessary API calls; change this to lazy-load versions only when a user expands a specific group (move the loadVersionsForConfig call out of the onFocus handler into the group expansion/toggle handler so each group's versions are fetched on-demand), and/or add a guard before calling loadVersionsForConfig (check a flag or existing data on each group such as g.versions or g.versionsLoaded) so you skip calling loadVersionsForConfig(g.config_id) for configs that are already loaded; update the group expansion logic used by the component that renders each config group to call loadVersionsForConfig when the group becomes expanded.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/components/ConfigSelector.tsx`:
- Around line 83-104: The useEffect in ConfigSelector relies on the configs
update to clear isLoadingPreview, which can leave the spinner active if
loadSingleVersion succeeds but configs isn't updated immediately; update the
Promise chain for loadSingleVersion inside the effect so that on success you
explicitly call setIsLoadingPreview(false) (not only on failure or when result
is falsy) — i.e., in the .then handler for loadSingleVersion (used in the effect
with selectedConfigId/selectedVersion) always call setIsLoadingPreview(false)
after processing the result so the loading state is cleared deterministically
even if configs updates are batched or delayed.
In `@app/components/prompt-editor/HistorySidebar.tsx`:
- Around line 1-3: The component HistorySidebar.tsx uses React hooks (useState,
useEffect) but is missing the Next.js App Router client directive; add the
string "use client" as the very first line of HistorySidebar.tsx so the file is
explicitly a client component (above the existing imports) to ensure hooks like
useState and useEffect are allowed.
In `@app/hooks/useConfigs.ts`:
- Around line 270-272: The synthetic ConfigPublic being constructed in the
configPublic assignment uses hardcoded placeholders (project_id: 0, inserted_at:
'', updated_at: '') which can be misleading or break downstream consumers;
update the creation logic for configPublic (the branch that uses configSource
and the variables config_id, configSource) to either populate
project_id/inserted_at/updated_at from actual fields on configSource if
available or set them to explicit null/undefined per the ConfigPublic type, and
add a short inline comment above the assignment indicating these are intentional
placeholders if you must keep them; ensure the ConfigPublic type is respected
(adjust typing/casting) and that downstream code does not rely on those
placeholder values.
In `@app/lib/utils.ts`:
- Around line 52-55: invalidateConfigCache currently just calls
clearConfigCache() which clears cache silently; update invalidateConfigCache to
also dispatch the CACHE_INVALIDATED_EVENT so listeners (e.g., useConfigs) get
notified to refetch. Specifically, in the invalidateConfigCache function call
clearConfigCache() then emit/dispatch the CACHE_INVALIDATED_EVENT (the same
event used in configStore.ts) so subscribers receive the invalidation; keep the
existing signature invalidateConfigCache(): void and reuse the same event
constant name CACHE_INVALIDATED_EVENT to ensure listener compatibility.
- Around line 84-100: The code mutates the incoming params.tools array by
assigning const tools = params.tools and later calling tools.push; instead,
avoid mutating the original API object by creating a new array copy (e.g., use a
shallow copy of params.tools or build a new array) before adding file_search
entries when params.knowledge_base_ids is present; update the logic around the
tools variable and the loop that processes params.knowledge_base_ids so you push
into the new array (or use concat/spread to produce a new array) and then use
that new array for downstream logic, leaving params.tools untouched.
- Around line 29-35: The timezone detection around the string branch is fragile;
replace the current composite check that uses timestamp.includes('T') and
timestamp.split('T')[1].includes('-') with a single regex test that reliably
detects explicit timezone info (a trailing 'Z' or a ±HH:MM/±HH offset).
Specifically, in the block that sets utcTimestamp (refer to the timestamp
variable and utcTimestamp constant), use a regex like one that matches
/Z$|[+-]\d{2}(:?\d{2})?$/ to decide if the input already contains timezone info
and only append 'Z' when the regex test fails, then parse with new
Date(utcTimestamp).getTime().
---
Nitpick comments:
In `@app/components/prompt-editor/ConfigEditorPane.tsx`:
- Around line 534-551: The onClick handler for version items (inside
ConfigEditorPane) calls loadSingleVersion without any loading state, so users
can click multiple times and the dropdown stays interactive; add a
fetchingVersionKey (string | null) state in ConfigEditorPane, set it to a unique
key (e.g., `${item.config_id}:${item.version}`) before awaiting
loadSingleVersion, disable the version button and/or show a spinner when
fetchingVersionKey matches that key, and clear fetchingVersionKey in finally;
ensure you still call onLoadConfig(config) and setIsDropdownOpen(false) only
after a successful load and handle errors (log/show) while preventing duplicate
clicks during the fetch.
In `@app/components/prompt-editor/DiffView.tsx`:
- Around line 135-140: The onFocus handler currently calls loadVersionsForConfig
for every entry in configGroups causing many unnecessary API calls; change this
to lazy-load versions only when a user expands a specific group (move the
loadVersionsForConfig call out of the onFocus handler into the group
expansion/toggle handler so each group's versions are fetched on-demand), and/or
add a guard before calling loadVersionsForConfig (check a flag or existing data
on each group such as g.versions or g.versionsLoaded) so you skip calling
loadVersionsForConfig(g.config_id) for configs that are already loaded; update
the group expansion logic used by the component that renders each config group
to call loadVersionsForConfig when the group becomes expanded.
In `@app/components/prompt-editor/HistorySidebar.tsx`:
- Around line 98-111: The local formatTimestamp function duplicates
relative-time logic already implemented in formatRelativeTime; replace
formatTimestamp by importing and using formatRelativeTime (instead of the local
function) in HistorySidebar, remove the duplicated formatTimestamp
implementation, and update any call sites in this component to call
formatRelativeTime(<timestamp>) so the shared utility is used consistently; also
ensure you reconcile output formats if needed (adjust formatRelativeTime or
convert its output here) so the displayed units match the desired "min/hr"
style.
In `@app/hooks/useConfigs.ts`:
- Around line 387-389: configGroups is being recomputed on every render because
groupConfigs(configs, versionCounts, versionItemsMap) is called directly in the
return; wrap that call in a memo so it only recalculates when inputs change —
e.g. create a memoized variable (e.g. const configGroups = useMemo(() =>
groupConfigs(configs, versionCounts, versionItemsMap), [configs, versionCounts,
versionItemsMap])) and return that instead of calling groupConfigs inline;
update the return to use the memoized configGroups so renders don't repeatedly
run groupConfigs.
In `@app/lib/utils.ts`:
- Around line 16-23: The comment about formatting timestamps is misplaced above
getExistingForProvider; move or copy that comment so it sits immediately above
the formatRelativeTime function instead of above getExistingForProvider. Locate
getExistingForProvider and formatRelativeTime in app/lib/utils.ts and reposition
the comment text ("Format timestamp as relative time / Handles UTC timestamps
from the database and converts them to local time") to precede
formatRelativeTime so comments match the related implementation.
- Around line 57-70: Extract the hardcoded 'kaapi_api_keys' string into a named
constant (e.g., API_KEYS_STORAGE_KEY) in the same module and update getApiKey to
use that constant; modify the top of app/lib/utils.ts to declare the constant
and replace occurrences of the literal in the getApiKey function so future
changes and reuse are centralized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fa014787-d7db-4959-9fb9-8aad0db406cf
📒 Files selected for processing (10)
app/components/ConfigSelector.tsxapp/components/prompt-editor/ConfigDiffPane.tsxapp/components/prompt-editor/ConfigEditorPane.tsxapp/components/prompt-editor/DiffView.tsxapp/components/prompt-editor/HistorySidebar.tsxapp/configurations/prompt-editor/page.tsxapp/hooks/useConfigs.tsapp/lib/constants.tsapp/lib/types/configs.tsapp/lib/utils.ts
✅ Files skipped from review due to trivial changes (1)
- app/lib/types/configs.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- app/components/prompt-editor/ConfigDiffPane.tsx
- app/lib/constants.ts
- app/configurations/prompt-editor/page.tsx
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/components/prompt-editor/ConfigEditorPane.tsx (1)
119-128:⚠️ Potential issue | 🔴 CriticalDon’t assume every provider has a model list.
This dereferences
MODEL_OPTIONS[newProvider][0]unconditionally. With the current option set, selecting a provider that has no entry inMODEL_OPTIONSthrows before the editor state updates.🛠️ Suggested guard
const handleProviderChange = (newProvider: string) => { + const providerModels = + MODEL_OPTIONS[newProvider as keyof typeof MODEL_OPTIONS]; + if (!providerModels?.length) return; + onConfigChange({ ...configBlob, completion: { ...configBlob.completion, provider: newProvider as any, params: { ...params, - model: - MODEL_OPTIONS[newProvider as keyof typeof MODEL_OPTIONS][0].value, + model: providerModels[0].value, }, }, }); };You’ll still want to keep
PROVIDES_OPTIONSandMODEL_OPTIONSin sync so unsupported providers are hidden or disabled instead of becoming a no-op.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/prompt-editor/ConfigEditorPane.tsx` around lines 119 - 128, The handler handleProviderChange currently assumes MODEL_OPTIONS[newProvider] exists and accesses [0].value, which throws for providers with no model list; change it to check MODEL_OPTIONS[newProvider] (or use optional chaining) and only set params.model to MODEL_OPTIONS[newProvider][0].value when the array exists and has length, otherwise preserve the existing params.model or set it to undefined/null; update the onConfigChange call that builds completion in configBlob to use this guarded value so the editor state won't error for providers missing MODEL_OPTIONS entries.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/components/prompt-editor/DiffView.tsx`:
- Around line 95-100: Wrap the async fetch call in a try/catch/finally: call
setIsLoadingCompare(true) before awaiting onFetchVersionDetail(config_id,
version), assign detail inside the try, set detail = undefined in the catch (or
handle/log the error) so the rejected promise doesn't escape, and ensure
setIsLoadingCompare(false) is called in the finally block; finally call
onCompareChange(detail ?? null). Reference functions: onFetchVersionDetail,
setIsLoadingCompare, onCompareChange and variables config_id/version.
In `@app/components/prompt-editor/HistorySidebar.tsx`:
- Around line 273-279: handleAction currently sets fetchingVersion before
awaiting onFetchVersionDetail but only clears it on the success path, so a
rejected promise leaves the row stuck; wrap the await in a try/finally (or
try/catch and always call setFetchingVersion(null) in finally) around the
onFetchVersionDetail call in handleAction, so setFetchingVersion(null) is
executed regardless of success or error for the given item.version and ensure
any errors are rethrown or handled as appropriate.
In `@app/lib/configFetchers.ts`:
- Around line 41-61: The validator currently treats any config missing from
cache.configMeta as "new", which falsely invalidates lightweight/partial caches
(e.g., when pageSize: 0). Update the new/updated check in fetchAllConfigs so it
only compares entries that exist in cache.configMeta: when iterating data.data,
skip configs where currentMeta[config.id] is undefined (do not treat missing
cached entries as a change), and only mark needsRefresh when an existing cached
entry has a differing updated_at; apply the same pattern to the other
occurrences referenced (around the checks at lines 108-109 and 128-135) so all
comparisons only act on IDs present in configMeta rather than assuming
configMeta is a full list.
---
Outside diff comments:
In `@app/components/prompt-editor/ConfigEditorPane.tsx`:
- Around line 119-128: The handler handleProviderChange currently assumes
MODEL_OPTIONS[newProvider] exists and accesses [0].value, which throws for
providers with no model list; change it to check MODEL_OPTIONS[newProvider] (or
use optional chaining) and only set params.model to
MODEL_OPTIONS[newProvider][0].value when the array exists and has length,
otherwise preserve the existing params.model or set it to undefined/null; update
the onConfigChange call that builds completion in configBlob to use this guarded
value so the editor state won't error for providers missing MODEL_OPTIONS
entries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3d7d1b29-edd4-4ce8-a4cd-24beeaeae05b
📒 Files selected for processing (6)
app/components/ConfigSelector.tsxapp/components/prompt-editor/ConfigEditorPane.tsxapp/components/prompt-editor/DiffView.tsxapp/components/prompt-editor/HistorySidebar.tsxapp/lib/configFetchers.tsapp/lib/constants.ts
Collaborator
Author
|
@vprashrex @Prajna1999 When you review this PR, could you please also test the Evaluation and Prompt Editor flows for a quick sanity check? Since this includes a lot of refactoring and some improvements, it would be good to double-check everything. @AkhileshNegi When you get the chance can you also please check the flow and UI. I made some UI update also. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/lib/configFetchers.ts (1)
214-233: Add batching to parallel version fetches for consistency.Unlike
fetchAllConfigsandfetchNextConfigBatchwhich batch requests at 5 concurrent calls, this function fires all missing version requests simultaneously viaPromise.all(missingVersions.map(...)). If a config has many versions (e.g., 50) with only 1 loaded, this could overwhelm the API or hit rate limits.♻️ Proposed fix to add batching
- const fetchedVersions = await Promise.all( - missingVersions.map(async (versionItem) => { + const BATCH_SIZE = 5; + const fetchedVersions: (SavedConfig | null)[] = []; + + for (let i = 0; i < missingVersions.length; i += BATCH_SIZE) { + const batch = missingVersions.slice(i, i + BATCH_SIZE); + const batchResults = await Promise.all( + batch.map(async (versionItem) => { try { const versionDataResponse = await apiFetch<ConfigVersionResponse>( `/api/configs/${config_id}/versions/${versionItem.version}`, apiKey, ); const versionData: ConfigVersionResponse = versionDataResponse; if (versionData.success && versionData.data) { return flattenConfigVersion(config, versionData.data); } } catch (e) { console.error( `Failed to fetch version ${versionItem.version} for config ${config_id}:`, e, ); } return null; - }), - ); + }), + ); + fetchedVersions.push(...batchResults); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/lib/configFetchers.ts` around lines 214 - 233, The code in fetcher that creates fetchedVersions runs all missingVersions concurrently; change the logic in the block that builds fetchedVersions to run requests in bounded concurrency (e.g., 5 at a time) instead of Promise.all over missingVersions—use a simple batching loop or a concurrency limiter to iterate missingVersions in chunks of 5, calling apiFetch<ConfigVersionResponse>(`/api/configs/${config_id}/versions/${versionItem.version}`, apiKey) for each item and pushing flattenConfigVersion(config, versionData.data) results into the fetchedVersions array while preserving error handling; update references to fetchedVersions, missingVersions, apiFetch, and flattenConfigVersion accordingly so the result matches the original shape but with max 5 concurrent network requests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/lib/configFetchers.ts`:
- Around line 214-233: The code in fetcher that creates fetchedVersions runs all
missingVersions concurrently; change the logic in the block that builds
fetchedVersions to run requests in bounded concurrency (e.g., 5 at a time)
instead of Promise.all over missingVersions—use a simple batching loop or a
concurrency limiter to iterate missingVersions in chunks of 5, calling
apiFetch<ConfigVersionResponse>(`/api/configs/${config_id}/versions/${versionItem.version}`,
apiKey) for each item and pushing flattenConfigVersion(config, versionData.data)
results into the fetchedVersions array while preserving error handling; update
references to fetchedVersions, missingVersions, apiFetch, and
flattenConfigVersion accordingly so the result matches the original shape but
with max 5 concurrent network requests.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue: #82
Summary:
1. Config Library N+1 calls
Before: fetchAllConfigs made 1 + N + N×M calls (list → versions list per config → full detail for every version of every config).
Fix:
FetchAllConfigsnow makes 1 + 2N calls maximum - oneGET /api/configs, then per config: oneGET /versions(lightweight, no config_blob) + oneGET /versions/{latest}only. Historical versions are never pre-fetched.2. Config Library initial load volume (Load More)
Before: All configs had their version details fetched upfront on page load.
Fix: Config Library page uses
useConfigs({ pageSize: 10 })- only the first 10 configs have version details fetched on load. A Load More button fetches the next 10. Other pages (Prompt Editor, Evaluations) are unaffected and still get all configs.3. Evaluation page / ConfigSelector N+1 calls
Before: The
ConfigSelectordropdown could trigger repeated GET /versions/{n} fetches across concurrent component mounts.Fix: Module-level
pendingFetchdeduplication - all mounted components share one in-flight request. Background cache validation reduced to a singleGET /api/configsusingupdated_atcomparison (no per-config version-count loops).4. Version Comparison (History sidebar + DiffView) fetching all versions upfront
Before: Opening the Prompt Editor for any config called
loadVersionsForConfig()which fired N parallel GET /versions/{n} requests — one per historical version. A config with 200 versions caused 200 simultaneous calls.Fix (this session):
loadVersionsForConfignow only fetches the lightweight version list(GET /versions, no config_blob)- 0 calls if already cached, 1 call otherwise.New
loadSingleVersion(config_id, version)fetches exactly 1 full version on demand when the user clicks Load or Compare.HistorySidebarrenders the complete version list from lightweight data (version number, commit message, timestamp visible immediately). Full details (config_blob) are fetched only on button click.DiffViewcompare dropdown shows all versions from the lightweight map. Selecting a version fetches its full details in 1 call, with loading state on the select.Summary by CodeRabbit
New Features
UX Improvements
Performance