Lazy-load config versions on expand & extract inline SVGs to icon components#96
Open
Lazy-load config versions on expand & extract inline SVGs to icon components#96
Conversation
…VGs to icon components
…VGs to icon components
|
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:
📝 WalkthroughWalkthroughThis PR centralizes config types, adds paginated/infinite-scroll list hooks, refactors the configurations page to use incremental loading and columnized layout, implements lazy per-config version loading with de-duplication, and consolidates/adds shared icon components. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ConfigsPage as Configurations Page
participant usePaginatedList as usePaginatedList Hook
participant useInfiniteScroll as useInfiniteScroll Hook
participant API as Backend API
participant configStore as Config Store
User->>ConfigsPage: Open page / type search
ConfigsPage->>usePaginatedList: init(endpoint, debouncedQuery)
usePaginatedList->>API: GET /api/v1/configs?skip=0&limit=10&query=...
API-->>usePaginatedList: return items + metadata
usePaginatedList-->>ConfigsPage: items, hasMore
ConfigsPage->>useInfiniteScroll: attach scrollRef to container
User->>useInfiniteScroll: scroll near bottom
useInfiniteScroll->>ConfigsPage: trigger onLoadMore
ConfigsPage->>usePaginatedList: loadMore()
usePaginatedList->>API: GET /api/v1/configs?skip=10&limit=10
API-->>usePaginatedList: next page
usePaginatedList-->>ConfigsPage: append items, update hasMore
User->>ConfigsPage: expand ConfigCard
ConfigsPage->>configStore: check versionItemsCache[configId]
configStore-->>ConfigsPage: cache miss
ConfigsPage->>ConfigsPage: call onLoadVersions(configId)
ConfigsPage->>API: GET /api/configs/:id/versions
API-->>configStore: store version list
ConfigsPage->>ConfigsPage: call onLoadSingleVersion(configId, version)
ConfigsPage->>API: GET /api/configs/:id/versions/:version
API-->>ConfigsPage: return SavedConfig
ConfigsPage->>User: render expanded details
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 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 docstrings
🧪 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: 3
🧹 Nitpick comments (4)
app/lib/utils.ts (1)
4-10: Consolidate imports from the same module and use consistent path alias.Lines 4-9 import from
@/app/lib/types/configswhile Line 10 imports from./types/configs. These should be consolidated into a single import statement using the@/path alias consistently.♻️ Proposed fix
import { ConfigPublic, ConfigVersionPublic, ConfigVersionItems, Tool, + SavedConfig, + ConfigGroup, } from "@/app/lib/types/configs"; -import { SavedConfig, ConfigGroup } from "./types/configs";As per coding guidelines: "Use the
@/path alias for all imports instead of relative paths."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/lib/utils.ts` around lines 4 - 10, Consolidate the two import statements into one that uses the "@/app/lib/types/configs" alias: import ConfigPublic, ConfigVersionPublic, ConfigVersionItems, Tool, SavedConfig, and ConfigGroup from the same module so all types are imported consistently; update the existing imports in app/lib/utils.ts (symbols: ConfigPublic, ConfigVersionPublic, ConfigVersionItems, Tool, SavedConfig, ConfigGroup) to a single aliased import and remove the relative "./types/configs" import.app/components/SimplifiedConfigEditor.tsx (1)
30-43: Consider using the sharedSavedConfigtype.This local
SavedConfiginterface duplicates the one defined in@/app/lib/types/configswith minor differences (missingdescription,type, andpromptContentfields). Consider importing and extending the shared type to maintain consistency and reduce duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/SimplifiedConfigEditor.tsx` around lines 30 - 43, The local SavedConfig interface in SimplifiedConfigEditor.tsx duplicates the shared type; replace it by importing the shared SavedConfig from "@/app/lib/types/configs" and either use it directly or extend it to add any component-specific optional fields (e.g., description, type, promptContent) to avoid divergence; update references in this file to use the imported/extended SavedConfig type (look for the SavedConfig interface and any functions or props that consume it) and remove the local duplicate declaration.app/components/icons/prompt-editor/ChevronRightIcon.tsx (1)
1-19: Consider extracting the sharedIconPropsinterface to reduce duplication.The
IconPropsinterface is identical across multiple icon files. While this works, extracting it to a shared location (e.g.,app/components/icons/types.ts) would reduce duplication and ensure consistency.♻️ Optional: Shared IconProps type
Create
app/components/icons/types.ts:export interface IconProps { className?: string; style?: React.CSSProperties; }Then import in each icon file:
+import { IconProps } from "../types"; -interface IconProps { - className?: string; - style?: React.CSSProperties; -}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/icons/prompt-editor/ChevronRightIcon.tsx` around lines 1 - 19, Extract the duplicated IconProps interface into a shared module and update the icon component to import it: create a new module exporting interface IconProps { className?: string; style?: React.CSSProperties } and replace the local IconProps declaration in ChevronRightIcon with an import; ensure ChevronRightIcon({ className, style }: IconProps) still types correctly and update any other icon files to import the shared IconProps to remove duplication.app/components/prompt-editor/ConfigEditorPane.tsx (1)
780-784: Keep these new borders and tooltip colors on the shared theme tokens.
border-gray-300,bg-[#1f2937],text-white, andborder-r-[#1f2937]bypass thecolorspalette, so this part of the editor will drift from the rest of the themed UI. Tailwind is fine for layout/spacing here, but the color values should stay instylevia the shared palette.Based on learnings: Use Tailwind CSS for layout and spacing, and inline styles (referencing the
colorsobject) for colors instead of mixing multiple styling approaches.Also applies to: 796-809, 824-828
🤖 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 780 - 784, The JSX in ConfigEditorPane.tsx is mixing Tailwind color classes (e.g., "border-gray-300", "text-white", and hardcoded hex borders like "border-r-[`#1f2937`]") with inline styles; remove these color-related Tailwind classes and instead use the shared theme tokens from the colors object in the element's style (e.g., set borderColor, backgroundColor, color via colors.border.*, colors.bg.*, colors.text.*) so all color values come from the shared palette; update the same change for the other affected JSX blocks referenced (around the other ranges) so no component uses hardcoded hex/Tailwind color classes and all color styling is done via the colors object.
🤖 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/`(routes)/configurations/page.tsx:
- Around line 40-50: The current round-robin split (used with setColumnCount in
the useEffect and the code that partitions configs into separate column
wrappers) causes wrong DOM order and remounts ConfigCard; instead render a
single flat list and let CSS handle columns. Remove the partitioning logic and
any function that builds per-column arrays, render configs via configs.map(...)
(keeping stable keys like config.id) inside a single container, and set the
column count via CSS grid (e.g., set style or class using gridTemplateColumns:
`repeat(columnCount, 1fr)`) so layout changes don't move nodes between different
subtrees; apply the same change wherever the round-robin split is used (the
other similar blocks noted in the review).
In `@app/components/ConfigCard.tsx`:
- Around line 221-223: The current conditional branch hides primary action
buttons when latestVersion is falsy, blocking edits/evaluations even though
handleEdit and handleUseInEvaluation can work with only config.id; update the UI
so the "Edit" and "Use in evaluation" buttons are rendered regardless of
latestVersion (either move the buttons out of the latestVersion conditional or
duplicate them in the fallback), ensuring their onClick calls still invoke
handleEdit and handleUseInEvaluation with config.id (and latestVersion where
available). Locate the buttons and the conditional using the symbols
latestVersion, handleEdit, handleUseInEvaluation, and config.id in
ConfigCard.tsx and adjust the markup so primary actions remain visible on lazy
detail load failure.
- Around line 44-47: The fast-path that returns early when latestVersion is set
(in the component using latestVersion, setExpanded, and onLoadVersions) causes
stale UI after parent updates; change the logic to also check the config
metadata timestamp (e.g., config.updated_at) or reset the cached detail state
when config.updated_at changes so that latestVersion is cleared and
onLoadVersions runs again on refresh; update the fast-path in the block
referencing latestVersion (and similar checks between lines 50-80 and 84-97) to
include a guard comparing stored metadata timestamp to config.updated_at or call
a state reset to clear latestVersion before returning.
---
Nitpick comments:
In `@app/components/icons/prompt-editor/ChevronRightIcon.tsx`:
- Around line 1-19: Extract the duplicated IconProps interface into a shared
module and update the icon component to import it: create a new module exporting
interface IconProps { className?: string; style?: React.CSSProperties } and
replace the local IconProps declaration in ChevronRightIcon with an import;
ensure ChevronRightIcon({ className, style }: IconProps) still types correctly
and update any other icon files to import the shared IconProps to remove
duplication.
In `@app/components/prompt-editor/ConfigEditorPane.tsx`:
- Around line 780-784: The JSX in ConfigEditorPane.tsx is mixing Tailwind color
classes (e.g., "border-gray-300", "text-white", and hardcoded hex borders like
"border-r-[`#1f2937`]") with inline styles; remove these color-related Tailwind
classes and instead use the shared theme tokens from the colors object in the
element's style (e.g., set borderColor, backgroundColor, color via
colors.border.*, colors.bg.*, colors.text.*) so all color values come from the
shared palette; update the same change for the other affected JSX blocks
referenced (around the other ranges) so no component uses hardcoded hex/Tailwind
color classes and all color styling is done via the colors object.
In `@app/components/SimplifiedConfigEditor.tsx`:
- Around line 30-43: The local SavedConfig interface in
SimplifiedConfigEditor.tsx duplicates the shared type; replace it by importing
the shared SavedConfig from "@/app/lib/types/configs" and either use it directly
or extend it to add any component-specific optional fields (e.g., description,
type, promptContent) to avoid divergence; update references in this file to use
the imported/extended SavedConfig type (look for the SavedConfig interface and
any functions or props that consume it) and remove the local duplicate
declaration.
In `@app/lib/utils.ts`:
- Around line 4-10: Consolidate the two import statements into one that uses the
"@/app/lib/types/configs" alias: import ConfigPublic, ConfigVersionPublic,
ConfigVersionItems, Tool, SavedConfig, and ConfigGroup from the same module so
all types are imported consistently; update the existing imports in
app/lib/utils.ts (symbols: ConfigPublic, ConfigVersionPublic,
ConfigVersionItems, Tool, SavedConfig, ConfigGroup) to a single aliased import
and remove the relative "./types/configs" import.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ca100cfd-3f06-4753-9a5f-3c3a28b044e4
📒 Files selected for processing (20)
app/(routes)/configurations/page.tsxapp/(routes)/configurations/prompt-editor/page.tsxapp/components/ConfigCard.tsxapp/components/ConfigDrawer.tsxapp/components/ConfigModal.tsxapp/components/SimplifiedConfigEditor.tsxapp/components/icons/index.tsxapp/components/icons/prompt-editor/ChevronRightIcon.tsxapp/components/icons/prompt-editor/InfoIcon.tsxapp/components/icons/prompt-editor/PlusIcon.tsxapp/components/icons/prompt-editor/SpinnerIcon.tsxapp/components/prompt-editor/ConfigDrawer.tsxapp/components/prompt-editor/ConfigEditorPane.tsxapp/components/prompt-editor/HistorySidebar.tsxapp/hooks/useConfigs.tsapp/lib/configFetchers.tsapp/lib/configTypes.tsapp/lib/store/configStore.tsapp/lib/types/configs.tsapp/lib/utils.ts
💤 Files with no reviewable changes (1)
- app/lib/configTypes.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/lib/types/configs.ts (1)
69-80: Consider documenting the reason for the index signature.The
[key: string]: anyindex signature weakens type safety. While this is acknowledged by the eslint-disable comment, consider adding a brief comment explaining why it's needed (e.g., "allows pass-through of additional backend-specific fields"). This helps future maintainers understand the intent.📝 Suggested documentation improvement
// Backend expects these as direct fields (flattened from tools array) knowledge_base_ids?: string[]; max_num_results?: number; - // eslint-disable-next-line `@typescript-eslint/no-explicit-any` + // Allow additional backend-specific fields to pass through without strict typing + // eslint-disable-next-line `@typescript-eslint/no-explicit-any` [key: string]: any;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/lib/types/configs.ts` around lines 69 - 80, The index signature "[key: string]: any" on the CompletionParams interface weakens type safety and lacks intent documentation; update the CompletionParams declaration to include a concise comment above the index signature (and retain the existing eslint-disable) explaining why it's required — e.g., "allows pass-through of additional backend-specific or provider-specific fields (flattened from frontend tools) for compatibility" — so future maintainers understand its purpose; reference the CompletionParams interface and the existing index signature when making the comment.
🤖 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/types/configs.ts`:
- Around line 69-80: The index signature "[key: string]: any" on the
CompletionParams interface weakens type safety and lacks intent documentation;
update the CompletionParams declaration to include a concise comment above the
index signature (and retain the existing eslint-disable) explaining why it's
required — e.g., "allows pass-through of additional backend-specific or
provider-specific fields (flattened from frontend tools) for compatibility" — so
future maintainers understand its purpose; reference the CompletionParams
interface and the existing index signature when making the comment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: eb6f3b05-1c07-4ee7-b8d5-eba3cb03c6f2
📒 Files selected for processing (2)
app/(routes)/configurations/prompt-editor/page.tsxapp/lib/types/configs.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- app/(routes)/configurations/prompt-editor/page.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
app/components/Loader.tsx (1)
74-74: Consider migratingLoaderBoxstates to skeleton/pulse instead of spinnerSince
LoaderBoxnow directly exposes spinner-based loading at Line 74, this is a good point to align with the component loading convention and use Tailwind pulse skeletons for these states.Based on learnings: Use skeleton loaders with Tailwind's pulse animation for loading states instead of spinners or other loading indicators.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/Loader.tsx` at line 74, The LoaderBox currently returns a spinner via the Loader component (return <Loader size={size} message={message} />); replace that spinner-based UI with a Tailwind "skeleton/pulse" pattern: update LoaderBox (and the Loader component if it enforces spinner) to render one or more divs styled with Tailwind classes such as animate-pulse, rounded, and bg-gray-200/neutral variants sized by the existing size prop, and optionally a smaller/text bar for message instead of an actual spinner; ensure the new skeleton respects the same props (size, message) and accessibility (aria-busy/aria-label) as the previous Loader so consumers of LoaderBox get the pulse skeleton loading state.app/hooks/usePaginatedList.ts (2)
126-137: Consider surfacingloadMoreerrors to the UI.Currently,
loadMorecatches errors and logs to console (line 132), whilerefetchsets the error state. This inconsistency means users won't see feedback if a pagination request fails.🔧 Proposed fix to set error state
fetchPage(skipRef.current, false) - .catch((e) => console.error(`Failed to load more from ${endpoint}:`, e)) + .catch((e) => { + console.error(`Failed to load more from ${endpoint}:`, e); + setError(e.message); + }) .finally(() => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/hooks/usePaginatedList.ts` around lines 126 - 137, The loadMore handler currently swallows errors by only logging to console; update loadMore (the function using loadingMoreRef, setIsLoadingMore and fetchPage) to surface failures to the same error state used by refetch by calling the shared setError (or setFetchError) in the catch block instead of only console.error, passing the caught error (and optionally contextual info like endpoint); keep the finally block that resets loadingMoreRef and setIsLoadingMore unchanged so UI loading state is cleared even on error.
85-87: Consider including the API error message in the thrown error.The response interface includes an
errorfield (line 26), but when the request fails, the thrown error uses a generic message. Including the API's error message would help with debugging.🔧 Proposed fix
if (!data.success || !data.data) { - throw new Error(`Failed to fetch from ${endpoint}.`); + throw new Error(data.error || `Failed to fetch from ${endpoint}.`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/hooks/usePaginatedList.ts` around lines 85 - 87, The thrown error when a fetch fails in usePaginatedList.ts currently uses a generic message; modify the failure branch that checks if (!data.success || !data.data) to include the API-provided error (data.error) in the thrown Error alongside the endpoint so the message becomes descriptive (e.g., include `${endpoint}: ${data.error || 'unknown error'}`); update any related throw site inside the function handling fetch responses (references: data.success, data.data, data.error, endpoint) so callers receive the API error text.app/hooks/useInfiniteScroll.ts (1)
25-39: Consider debouncing or additional guard for short-content scenarios.When
scrollHeight <= clientHeight(lines 31-34),onLoadMorefires immediately on every render cycle whereisLoadingis false. If the fetched content still doesn't fill the viewport, this can trigger rapid successive calls beforeisLoadingMorepropagates back.The current design relies on
loadMoreinusePaginatedListhaving its ownloadingMoreRefguard, which should prevent actual duplicate fetches. However, consumers might pass a differentonLoadMorewithout such protection.Consider either:
- Documenting that
isLoadingshould beisLoading || isLoadingMorewhen used withusePaginatedList- Adding a local debounce/throttle for the short-content case
📝 Suggestion: Add JSDoc note for consumers
/** * Generic infinite-scroll hook. * Attach the returned ref to a scrollable container; `onLoadMore` fires * when the user scrolls within `threshold` pixels of the bottom. * * Reusable across any paginated list (configs, datasets, evaluations, etc.). + * + * `@param` isLoading - Pass `true` while any fetch is in-flight (initial or pagination) + * to prevent premature `onLoadMore` calls. For usePaginatedList, + * pass `isLoading || isLoadingMore`. */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/hooks/useInfiniteScroll.ts` around lines 25 - 39, In useInfiniteScroll (checkShouldLoad), add a local short-call guard to avoid repeatedly invoking onLoadMore when scrollHeight <= clientHeight: create a ref like shortLoadRef (or a small debounce using setTimeout) and before calling onLoadMore check !shortLoadRef.current && !isLoading; when you call onLoadMore set shortLoadRef.current = true and clear it when isLoading becomes true or after a small timeout (or when content size increases), and keep existing checks for hasMore, isLoading, threshold, scrollRef to ensure compatibility with consumers who don't provide their own loading guard.
🤖 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/Loader.tsx`:
- Line 74: The component now returns only <Loader />, removing the structural
wrapper (<LoaderBox>) and breaking spacing/box affordances; restore the default
boxed wrapper by returning the original LoaderBox structure that wraps <Loader
/> (and/or a wrapper div with the same classes) and add an explicit prop (e.g.,
inline or boxed) on the Loader/LoaderBox API so callers can opt into inline
rendering; update the Loader.tsx export (LoaderBox / Loader) to honor that prop
and preserve existing usage where forms expect the boxed layout.
---
Nitpick comments:
In `@app/components/Loader.tsx`:
- Line 74: The LoaderBox currently returns a spinner via the Loader component
(return <Loader size={size} message={message} />); replace that spinner-based UI
with a Tailwind "skeleton/pulse" pattern: update LoaderBox (and the Loader
component if it enforces spinner) to render one or more divs styled with
Tailwind classes such as animate-pulse, rounded, and bg-gray-200/neutral
variants sized by the existing size prop, and optionally a smaller/text bar for
message instead of an actual spinner; ensure the new skeleton respects the same
props (size, message) and accessibility (aria-busy/aria-label) as the previous
Loader so consumers of LoaderBox get the pulse skeleton loading state.
In `@app/hooks/useInfiniteScroll.ts`:
- Around line 25-39: In useInfiniteScroll (checkShouldLoad), add a local
short-call guard to avoid repeatedly invoking onLoadMore when scrollHeight <=
clientHeight: create a ref like shortLoadRef (or a small debounce using
setTimeout) and before calling onLoadMore check !shortLoadRef.current &&
!isLoading; when you call onLoadMore set shortLoadRef.current = true and clear
it when isLoading becomes true or after a small timeout (or when content size
increases), and keep existing checks for hasMore, isLoading, threshold,
scrollRef to ensure compatibility with consumers who don't provide their own
loading guard.
In `@app/hooks/usePaginatedList.ts`:
- Around line 126-137: The loadMore handler currently swallows errors by only
logging to console; update loadMore (the function using loadingMoreRef,
setIsLoadingMore and fetchPage) to surface failures to the same error state used
by refetch by calling the shared setError (or setFetchError) in the catch block
instead of only console.error, passing the caught error (and optionally
contextual info like endpoint); keep the finally block that resets
loadingMoreRef and setIsLoadingMore unchanged so UI loading state is cleared
even on error.
- Around line 85-87: The thrown error when a fetch fails in usePaginatedList.ts
currently uses a generic message; modify the failure branch that checks if
(!data.success || !data.data) to include the API-provided error (data.error) in
the thrown Error alongside the endpoint so the message becomes descriptive
(e.g., include `${endpoint}: ${data.error || 'unknown error'}`); update any
related throw site inside the function handling fetch responses (references:
data.success, data.data, data.error, endpoint) so callers receive the API error
text.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 92716a4a-263a-4bb3-b3da-2a538cbc30da
📒 Files selected for processing (18)
app/(routes)/configurations/page.tsxapp/api/configs/route.tsapp/components/Loader.tsxapp/components/icons/common/CheckIcon.tsxapp/components/icons/common/ChevronDownIcon.tsxapp/components/icons/common/GearIcon.tsxapp/components/icons/common/PlusIcon.tsxapp/components/icons/common/RefreshIcon.tsxapp/components/icons/common/SearchIcon.tsxapp/components/icons/common/SidebarToggleIcon.tsxapp/components/icons/common/WarningTriangleIcon.tsxapp/components/icons/index.tsxapp/hooks/useConfigs.tsapp/hooks/useInfiniteScroll.tsapp/hooks/usePaginatedList.tsapp/lib/constants.tsapp/lib/context/AuthContext.tsxeslint.config.mjs
💤 Files with no reviewable changes (1)
- app/lib/context/AuthContext.tsx
✅ Files skipped from review due to trivial changes (1)
- eslint.config.mjs
🚧 Files skipped from review as they are similar to previous changes (2)
- app/hooks/useConfigs.ts
- app/(routes)/configurations/page.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/(routes)/configurations/page.tsx (1)
84-92:⚠️ Potential issue | 🟠 MajorKeep the cards in a single DOM list.
Line 90 still round-robins
configsinto per-column arrays, and Lines 397-408 render those arrays column-by-column. That keeps the visual layout, but tab/screen-reader order becomes0,3,6...instead of row order, andConfigCardlocal state is remounted whenever cards move between column wrappers. Renderconfigs.map(...)once and let CSS place the cards.Flatten the render tree
- {columns.map((col, colIdx) => ( - <div key={colIdx} className="flex flex-col gap-4"> - {col.map((config) => ( - <ConfigCard - key={config.id} - config={config} - evaluationCount={evaluationCounts[config.id] || 0} - onLoadVersions={loadVersionsForConfig} - onLoadSingleVersion={loadSingleVersion} - /> - ))} - </div> - ))} + {configs.map((config) => ( + <ConfigCard + key={config.id} + config={config} + evaluationCount={evaluationCounts[config.id] || 0} + onLoadVersions={loadVersionsForConfig} + onLoadSingleVersion={loadSingleVersion} + /> + ))}Also applies to: 393-409
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/`(routes)/configurations/page.tsx around lines 84 - 92, The current useMemo block named columns (created by useMemo with configs and columnCount) and the rendering that iterates over those per-column arrays causes DOM order and remounting issues for ConfigCard; instead remove the round-robin grouping and render a single flat list by calling configs.map(...) once (render each ConfigCard in original array order), let CSS handle the multi-column visual placement (use columnCount-driven grid or column styles), and ensure ConfigCard instances are not wrapped by per-column containers so their local state is preserved.
🤖 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/`(routes)/configurations/page.tsx:
- Around line 125-149: The cached version list (configState.versionItemsCache)
used by loadVersionsForConfig prevents refreshed server data from being shown;
update the refetch/refresh path to invalidate or version that cache so cards
will reload fresh. Concretely, when the user triggers refetch (the handler
calling refetch), clear the specific config's entry or the entire
configState.versionItemsCache and also clear pendingVersionLoads for that config
(or increment a refresh/version counter stored alongside the cache and have
loadVersionsForConfig check it) so loadVersionsForConfig(configId) will fetch
from the server instead of returning the stale cached array.
---
Duplicate comments:
In `@app/`(routes)/configurations/page.tsx:
- Around line 84-92: The current useMemo block named columns (created by useMemo
with configs and columnCount) and the rendering that iterates over those
per-column arrays causes DOM order and remounting issues for ConfigCard; instead
remove the round-robin grouping and render a single flat list by calling
configs.map(...) once (render each ConfigCard in original array order), let CSS
handle the multi-column visual placement (use columnCount-driven grid or column
styles), and ensure ConfigCard instances are not wrapped by per-column
containers so their local state is preserved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 87cdafe2-70b8-4c8a-951e-189aafb4c8a0
📒 Files selected for processing (3)
app/(routes)/configurations/page.tsxapp/components/Loader.tsxapp/globals.css
✅ Files skipped from review due to trivial changes (1)
- app/globals.css
🚧 Files skipped from review as they are similar to previous changes (1)
- app/components/Loader.tsx
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: #95
Summary:
ConfigEditorPane,Configurationspage into reusable icon componentsSummary by CodeRabbit
New Features
Improvements