feat: ES-198 enhance performance when loading big projects #93
Conversation
WalkthroughThe changes update both backend and frontend functionalities. In the backend, the scroll synchronization action is refined by emitting an event and renaming constants, while the result mapping has been refactored for concurrency with a caching mechanism. In the frontend, several UI components have been modified to improve result management, editor initialization, and event handling. A new dependency is added, and a custom hook for results management is introduced. Additional adjustments include updates in state stores and layout structure. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AppMenu
participant Runtime
participant EditorToolbar
User->>AppMenu: Select "Sync Scroll Position"
AppMenu->>Runtime: Emit ActionToggleSyncScrollPosition event
Runtime->>EditorToolbar: Dispatch ToggleSyncScrollPosition event
EditorToolbar->>EditorToolbar: Handle scroll sync toggle
sequenceDiagram
participant User
participant SortSelector
participant useResults
participant API
User->>SortSelector: Click sort option button
SortSelector->>useResults: Call reset() & update sort
useResults->>API: Fetch updated results based on new sort criteria
API-->>useResults: Return filtered results
useResults->>SortSelector: Provide new results data
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (11)
frontend/src/components/CodeViewer.tsx (3)
95-105: Guard against undefined content.
Ifcontentis undefined, casting it to string might result in'undefined'. Consider defaulting to an empty string to avoid unexpected display or logs.- const nModel = monaco.editor.createModel(content as string, language as string); + const safeContent = typeof content === 'string' ? content : ''; + const nModel = monaco.editor.createModel(safeContent, language as string);
107-132: Consider clamping highlight ranges.
If a highlight range exceeds the total number of lines, it may be silently ignored. You could clamp or validate them before creating decorations. This helps avoid unexpected behavior.- range: new monaco.Range(start, 1, end, 1), + const maxLines = model.getLineCount(); + const safeEnd = end > maxLines ? maxLines : end; + range: new monaco.Range(start, 1, safeEnd, 1),
143-145: Ensure highlights update if props change.
Currently,updateHighlight()only runs once on mount. IfhighlightLineschanges, the highlights won't refresh. Consider adding it to the dependency array so changes are reflected.-useEffect(() => { - updateHighlight(); -}, []); +useEffect(() => { + updateHighlight(); +}, [highlightLines]);backend/mappers/result_mapper_impl.go (3)
28-30: sync.Map usage and concurrency.
The addition of"runtime","sync", and a globalsync.Mapis a practical approach for caching. However,sync.Maphas no eviction policy, so it may grow unbounded if many different PURLs are processed. Consider a more controlled cache if memory usage is a concern.Also applies to: 42-42
72-92: Parallel mapping is a good performance boost.
Usingruntime.NumCPU()workers to process theresultsconcurrently is a reasonable approach. Monitor memory usage if the dataset is very large, and consider error handling or partial-failure scenarios if needed.
126-137: Caching logic usinggetProjectURL.
The lazy loading of cached URLs is safe and straightforward. Be aware that if multiple threads compute the same PURL concurrently the first time, each might do the work before the cache is updated. This is typically acceptable but worth noting for high-traffic scenarios.frontend/src/hooks/useResults.tsx (1)
8-31: Effective custom hook for result fetching.
Using a debounced query and React Query provides a neat, optimized approach. Consider handling errors or customizing stale times for performance. Otherwise, this is straightforward and maintainable.useQuery({ queryKey: ['results', debouncedQuery, filterByMatchType, sort], queryFn: () => fetchResults(), + staleTime: 5000, // Example: 5 seconds stale time });frontend/src/components/SortSelector.tsx (1)
76-79: Consider debouncing the reset operation.While resetting after sort changes is good for consistency, consider debouncing the reset to prevent multiple unnecessary resets when users are quickly trying different sort options.
+import { debounce } from 'lodash'; + export default function SortSelector() { const { reset } = useResults(); + const debouncedReset = debounce(reset, 300); // ... onClick={() => { setSort(option.value, sort.order); - reset(); + debouncedReset(); }}frontend/src/components/FilterComponentActions.tsx (2)
41-43: Consider using a single state object for dialog management.Multiple related state variables could be combined into a single state object for better maintainability and to prevent potential state synchronization issues.
- const [showReplaceComponentDialog, setShowReplaceComponentDialog] = useState(false); - const [withComment, setWithComment] = useState(false); - const [filterBy, setFilterBy] = useState<'by_file' | 'by_purl'>(); + const [dialogState, setDialogState] = useState<{ + isOpen: boolean; + withComment: boolean; + filterBy?: 'by_file' | 'by_purl'; + }>({ + isOpen: false, + withComment: false, + });
102-106: Update dialog state management to use the combined state object.If implementing the previous suggestion, update the state management here accordingly.
- setShowReplaceComponentDialog(true); - setFilterBy(args.filterBy); - setWithComment(args.withComment ?? false); + setDialogState({ + isOpen: true, + filterBy: args.filterBy, + withComment: args.withComment ?? false, + });frontend/src/components/Sidebar.tsx (1)
163-168: Consider adjusting virtualization parameters for optimal performance.The current overscan value of 5 might need tuning based on actual usage patterns. A larger overscan could provide smoother scrolling at the cost of memory, while a smaller value would be more memory efficient but might show blank spaces during fast scrolling.
const virtualizer = useVirtualizer({ count: results.length, getScrollElement: () => parentRef.current?.querySelector('[data-radix-scroll-area-viewport]') as Element, estimateSize: () => ITEM_HEIGHT, - overscan: 5, + overscan: 10, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (17)
app.go(1 hunks)backend/entities/keyboard.go(3 hunks)backend/mappers/result_mapper_impl.go(3 hunks)frontend/package.json(1 hunks)frontend/package.json.md5(1 hunks)frontend/src/components/ActionToolbar.tsx(4 hunks)frontend/src/components/CodeViewer.tsx(3 hunks)frontend/src/components/EditorToolbar.tsx(2 hunks)frontend/src/components/FilterComponentActions.tsx(3 hunks)frontend/src/components/MatchComparison.tsx(5 hunks)frontend/src/components/Sidebar.tsx(5 hunks)frontend/src/components/SortSelector.tsx(4 hunks)frontend/src/hooks/useResults.tsx(1 hunks)frontend/src/lib/editor.ts(2 hunks)frontend/src/modules/components/stores/useComponentFilterStore.ts(0 hunks)frontend/src/modules/results/stores/useResultsStore.ts(3 hunks)frontend/src/routes/root.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- frontend/src/modules/components/stores/useComponentFilterStore.ts
✅ Files skipped from review due to trivial changes (2)
- frontend/package.json.md5
- frontend/src/routes/root.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (22)
frontend/src/components/CodeViewer.tsx (4)
52-53: Use of refs looks straightforward.
These newuseRefdeclarations for the editor and its container are standard and pose no immediate concerns.
65-73: Editor default options are reasonable.
Configuring a read-only, minimal editor with automatic layout and a modest font size is good for performance and readability. No issues found.
76-86: Monaco editor initialization is sound.
TheinitMonacofunction correctly checks for the container and registers the new editor instance. Consider adding defensive checks or logs iflanguageis missing, but otherwise this is fine.
88-94: Cleanup withdestroyMonacois correct.
Disposing the editor and removing it frommonacoManagerensures no resource leaks. Good practice for unmounts.backend/mappers/result_mapper_impl.go (3)
51-63: Reading from BOM entry is consistent.
PopulatingConcludedPurlandCommentfrom the BOM entry aligns with existing patterns. No immediate issues.
121-124: Deferred computation for concluded PURL URL.
Wrappingpurlutils.ProjectUrlinsidegetProjectURLensures caching and consistent error handling. Looks good.
152-154: Detected PURL gets the same caching benefits.
ReusinggetProjectURLfor the detected PURL is consistent with the concluded URL approach. No concerns here.frontend/src/hooks/useResults.tsx (1)
1-7: Imports are well-organized.
The React Query and local store imports are appropriate. No issues noted.frontend/src/components/EditorToolbar.tsx (2)
26-26: LGTM! Improved imports organization.The changes properly separate runtime and model imports, enhancing code organization.
Also applies to: 33-34
49-55: Great improvement in event handling architecture!The changes implement a proper event subscription model with cleanup, preventing potential memory leaks and improving performance.
frontend/src/components/MatchComparison.tsx (3)
93-94: LGTM! Improved loading state management.The combined loading states provide a better user experience by ensuring both component and file content loading states are considered.
97-97: LGTM! Added overflow handling.The
overflow-hiddenclass prevents layout shifts during loading states.
111-138: LGTM! Enhanced loading state UI.The conditional rendering with Loading component improves user experience by:
- Showing clear loading indicators
- Preventing premature CodeViewer rendering
- Providing specific loading messages for local and remote files
app.go (1)
135-137: LGTM! Improved event handling for scroll sync.The change to emit an event for scroll sync toggle aligns with event-driven architecture principles and maintains consistency with keyboard shortcuts.
frontend/src/lib/editor.ts (2)
87-92: LGTM! Added editor cleanup.The
removeEditormethod helps prevent memory leaks by allowing proper cleanup of editor instances.
179-191: LGTM! Optimized scroll sync toggle.The refactored toggle logic ensures proper cleanup of existing listeners before re-syncing, preventing potential memory leaks and improving performance.
frontend/src/modules/results/stores/useResultsStore.ts (2)
46-46: LGTM! Enhanced API contract.The updated return type makes the API contract more explicit and enables better control over results handling.
114-114: LGTM! Improved results handling.Explicitly returning filtered results allows consumers to handle pending and completed results more effectively.
frontend/src/components/Sidebar.tsx (2)
24-24: Great performance optimization using virtualization!The addition of
@tanstack/react-virtualand the implementation ofuseResultshook is a solid approach for handling large result sets efficiently.Also applies to: 58-60
179-182: LGTM! Good UX with loading state.The loading state implementation provides clear feedback to users during data fetching.
backend/entities/keyboard.go (1)
56-56: LGTM! Improved action naming and grouping.The renaming of
ActionSyncScrollPositiontoActionToggleSyncScrollPositionbetter reflects its toggle behavior, and moving it toGroupViewis more appropriate for its functionality.Also applies to: 299-300
frontend/package.json (1)
32-32: LGTM! Appropriate dependency addition.The addition of
@tanstack/react-virtualwith version^3.13.0is appropriate for implementing virtualization in the Sidebar component.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
backend/repository/result_repository_json_impl.go (3)
52-65: Double-checked locking pattern caution.
The double-checked approach with anRLock→shouldRefreshCache()→ unlock →Lock→ re-check can be correct but is prone to complexity. Make sure all read & write paths validate the cache state consistently to avoid race conditions.
85-97:shouldRefreshCache()logic is concise.
Checking ifr.cacheisnilor if the file was modified is a sound refresh trigger. Consider logging file path or modification time in debug logs to aid troubleshooting.
99-122:refreshCache()implementation is robust.
Gracefully handles missing files by resetting the cache, logs parse errors correctly, and updateslastModified. Consider verifying the file path from config for non-empty or invalid values.backend/mappers/result_mapper_impl.go (2)
41-44: Global sync.Map usage.
resultDTOCacheandpurlCacheeffectively store computed values, but global caches can bloat if entries accumulate indefinitely. Consider an eviction policy for long-running systems.
93-115: Worker pool for concurrent mapping.
Usingruntime.NumCPU()for worker count and distributing indexes viajobChanis a good design. For extremely large slices, consider dynamic scheduling or bounding concurrency further, but this is a great start.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/mappers/result_mapper_impl.go(4 hunks)backend/repository/result_repository_json_impl.go(3 hunks)frontend/src/components/ActionToolbar.tsx(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: integration-tests
🔇 Additional comments (10)
frontend/src/components/ActionToolbar.tsx (3)
29-30: LGTM! Good improvements in state management.The switch from
useResultsStoretouseResultshook and the addition ofwithErrorHandlingdemonstrate good practices in state management and error handling.Also applies to: 48-48
50-66: LGTM! Good error handling pattern.The mutation implementation follows best practices with proper error handling and user feedback through toast notifications.
96-98: LGTM! Good keyboard shortcut implementation and UI accessibility.The keyboard shortcuts are well-implemented and properly documented in tooltips. The UI components follow accessibility best practices.
Also applies to: 100-135
backend/repository/result_repository_json_impl.go (3)
29-30: Imports for concurrency and time usage look appropriate.
Good addition of"sync"and"time"to manage caching and timestamps.
39-42: New fields for caching and concurrency.
Introducingcache,lastModified, andmutexis a clear step toward implementing a thread-safe caching mechanism. Ensure they’re consistently initialized and accessed only within locked sections.
69-73: Filter handling is straightforward.
Returning the entire cache whenfilterisnilis efficient, and iterating overr.cachefor filtering is clear. Keep an eye on filter performance ifcachegrows large.backend/mappers/result_mapper_impl.go (4)
28-30: Runtime and sync imports for concurrency.
These imports support the newly introduced parallel processing and caching logic.
52-62:generateCacheKeyfunctionality.
Combining multiple fields from theResultand BOM entry is sensible. Confirmresult.Purlis never nil to avoid a panic withstrings.Join(*result.Purl, ",").
65-72: Caching inMapToResultDTO.
Storing and retrievingResultDTOfromresultDTOCacheprevents repetitive computation. This is a clean approach. Careful with partial initialization if multiple goroutines miss simultaneously;sync.Mapshould handle that gracefully.Also applies to: 77-77, 82-82, 85-86
144-147:getProjectURLusage for concluded and detected PURL.
Fetching and caching project URLs viapurlCacheis straightforward. The approach logs errors and returns an empty string on failure, ensuring no panics.Also applies to: 149-160, 175-177
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
frontend/src/modules/results/constants.ts (1)
24-25: Consider consolidating debounce constants.Since both constants are used for debouncing with the same value, consider consolidating them into a single object for better organization and maintainability.
-export const DEBOUNCE_QUERY_MS = 300; -export const DEBOUNCE_RESET_RESULTS_MS = 300; +export const DEBOUNCE = { + QUERY_MS: 300, + RESET_RESULTS_MS: 300, +} as const;backend/repository/component_repository_json_impl.go (1)
49-50: Consider documenting the new field usage.Adding a short doc comment for the new
resultsRepositoryfield will help future maintainers quickly understand its purpose and usage in the repository.backend/repository/result_repository.go (1)
30-30: Add a brief method comment.A short doc comment clarifying the purpose and expected behavior of
GetResultByPathwill improve readability and maintainability.backend/repository/result_repository_json_impl.go (1)
171-182: Consider adding an index for path-based lookups.The current implementation uses linear search which could be slow for large projects. Consider using a map for O(1) lookups.
type ResultRepositoryJsonImpl struct { fr utils.FileReader cache []entities.Result + pathIndex map[string]int lastModified time.Time mutex sync.RWMutex watcher *fsnotify.Watcher } func (r *ResultRepositoryJsonImpl) refreshCache() error { // ... existing code ... r.cache = scanResults + // Update path index + r.pathIndex = make(map[string]int, len(scanResults)) + for i, result := range scanResults { + r.pathIndex[result.Path] = i + } return nil } func (r *ResultRepositoryJsonImpl) GetResultByPath(path string) (entities.Result, error) { r.mutex.RLock() defer r.mutex.RUnlock() - for _, result := range r.cache { - if result.Path == path { - return result, nil - } - } + if idx, ok := r.pathIndex[path]; ok { + return r.cache[idx], nil + } return entities.Result{}, fmt.Errorf("result not found: %s", path) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
backend/entities/result.go(1 hunks)backend/repository/component_repository_json_impl.go(1 hunks)backend/repository/result_repository.go(1 hunks)backend/repository/result_repository_json_impl.go(2 hunks)backend/service/result_service_impl.go(1 hunks)frontend/src/components/ActionToolbar.tsx(5 hunks)frontend/src/components/SortSelector.tsx(4 hunks)frontend/src/modules/results/constants.ts(1 hunks)main.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (14)
frontend/src/components/SortSelector.tsx (1)
28-30: LGTM! Great performance optimization.The implementation of debounced reset functionality effectively prevents rapid consecutive resets and reduces unnecessary re-renders when sorting options change. This aligns well with the PR objective to enhance performance when loading big projects.
Also applies to: 59-62, 79-82, 96-104
frontend/src/components/ActionToolbar.tsx (5)
29-30: LGTM!The new imports are correctly placed and support the enhanced error handling and results management functionality.
48-48: LGTM!The migration from
useResultsStore.fetchResultstouseResults.resetaligns with the PR's performance enhancement objectives.
57-57: LGTM!The
reset()call is correctly placed in theonSuccesscallback to refresh results after saving changes.
68-94: LGTM! Error handling has been implemented as suggested.The implementation includes comprehensive error handling for undo/redo operations with appropriate error messages and success callbacks.
96-97: LGTM! Handlers are now consistently used across keyboard shortcuts and button clicks.The implementation ensures consistent behavior between keyboard shortcuts and button clicks as suggested.
Also applies to: 105-105, 116-116
backend/repository/component_repository_json_impl.go (1)
53-58: Constructor parameter naming looks good.The additional constructor parameter aligns well with dependency injection, improving testability and maintainability of the repository.
backend/service/result_service_impl.go (2)
38-40: Verify removal of the watcher.Confirm that removing the file system watcher poses no unintended side effects for services depending on dynamic or real-time updates. If there's no remaining need to watch the filesystem for changes, this simplification is beneficial.
45-46: Constructor refactoring looks good.The simpler constructor and removal of the watcher field reduce complexity and potential maintenance overhead.
backend/entities/result.go (2)
41-47: LGTM! Good optimization of the Result struct.The change from
[]Matchto[]Componentreduces indirection and simplifies the data structure, which should improve memory usage when dealing with large projects.
69-87:❓ Verification inconclusive
Verify the GetMatchPercentage implementation with Component struct.
The method now uses the Component struct directly. Let's verify that all Component instances have the required
Matchedfield populated correctly.
🏁 Script executed:
#!/bin/bash # Search for Component struct definition and verify Matched field ast-grep --pattern 'type Component struct { $$$ Matched string $$$ $$$ }' # Search for all places where Component.Matched is set rg -A 2 '\.Matched\s*='Length of output: 107
Action Required: Manually Verify Component's
MatchedField UsageThe current automated checks did not return any output. Please run the following additional shell commands to further inspect the codebase and confirm that all instances of the
Componentstruct are correctly populated with theMatchedfield:If these additional checks continue to yield no output, please perform a manual review of the
Componentstruct definitions and their field assignments. This verification is essential to ensure that theGetMatchPercentageimplementation relying on theMatchedfield is operating on properly populated data.
🏁 Script executed:
#!/bin/bash # List all occurrences of the Component struct along with context to inspect its fields rg -C 10 "type Component struct" . # Look up where the Matched field is assigned within the codebase (include surrounding context) rg -C 5 "\.Matched\s*=" .Length of output: 1727
Action Required: Manual Verification of
MatchedField PopulationThe Component struct (in
backend/entities/component.go) does include theMatchedfield, which is used in theGetMatchPercentagemethod (inbackend/entities/result.go). However, our automated searches did not reveal explicit code assignments forMatched. It appears that this field is likely populated via JSON unmarshalling or another indirect mechanism.Please manually verify that:
- All instances of the Component struct have their
Matchedfield correctly populated (e.g., via JSON decoding or other initialization paths).- The
GetMatchPercentagemethod reliably receives a valid percentage value through theMatchedfield when processing snippet matches.If the population via JSON or another mechanism is intentional and properly handled, no changes are needed. Otherwise, consider updating the relevant code paths to ensure the field is not inadvertently left empty.
main.go (1)
79-79: LGTM! Good optimization of component repository initialization.Adding
resultRepositoryas a dependency enables the component repository to leverage cached results, reducing redundant file I/O operations.backend/repository/result_repository_json_impl.go (2)
78-99: LGTM! Efficient file watching implementation.The implementation correctly handles file changes and updates the cache atomically with proper error handling.
101-120: LGTM! Thread-safe cached results retrieval.Good use of RWMutex for concurrent reads while maintaining thread safety during updates.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
backend/repository/result_repository_json_impl.go (1)
48-78:⚠️ Potential issueAdd cleanup mechanism for resources.
The constructor initializes resources (watcher, goroutine) but lacks a cleanup mechanism. This could lead to resource leaks.
Add a Close method to properly cleanup resources:
+func (r *ResultRepositoryJsonImpl) Close() { + if r.watcher != nil { + r.watcher.Close() + } + // Unregister config listener + config.GetInstance().UnregisterListener(r.onConfigChange) +}
🧹 Nitpick comments (5)
backend/repository/result_repository_json_impl.go (3)
73-77: Consider using context for graceful shutdown.The watchForChanges goroutine runs indefinitely without a way to stop it gracefully.
-go repo.watchForChanges() +ctx, cancel := context.WithCancel(context.Background()) +go repo.watchForChanges(ctx) +repo.cancel = cancel // Store in struct
89-110: Consider debouncing file system events.Multiple write events in quick succession could trigger unnecessary cache refreshes.
+var debounceTimer *time.Timer if event.Op&fsnotify.Write == fsnotify.Write { + if debounceTimer != nil { + debounceTimer.Stop() + } + debounceTimer = time.AfterFunc(100*time.Millisecond, func() { r.mutex.Lock() if err := r.refreshCache(); err != nil { log.Error().Err(err).Msg("Error refreshing cache after file change") } r.mutex.Unlock() + }) }
182-193: Consider using a map for O(1) path lookups.The current implementation performs a linear search through the cache. For large projects, using a map would provide constant-time lookups.
type ResultRepositoryJsonImpl struct { fr utils.FileReader cache []entities.Result + pathIndex map[string]entities.Result lastModified time.Time mutex sync.RWMutex watcher *fsnotify.Watcher } func (r *ResultRepositoryJsonImpl) refreshCache() error { // ... existing code ... r.cache = scanResults + r.pathIndex = make(map[string]entities.Result) + for _, result := range scanResults { + r.pathIndex[result.Path] = result + } return nil } func (r *ResultRepositoryJsonImpl) GetResultByPath(path string) (entities.Result, error) { r.mutex.RLock() defer r.mutex.RUnlock() - for _, result := range r.cache { - if result.Path == path { - return result, nil - } - } + if result, ok := r.pathIndex[path]; ok { + return result, nil + } return entities.Result{}, fmt.Errorf("result not found: %s", path) }frontend/src/components/CodeViewer.tsx (2)
65-74: Consider moving static options outside component.The
editorDefaultOptionsobject is static and memoized. Since it doesn't depend on any props or state, it could be moved outside the component to reduce unnecessary re-renders and improve performance.+const EDITOR_DEFAULT_OPTIONS: monaco.editor.IStandaloneEditorConstructionOptions = { + readOnly: true, + theme: 'vs-dark', + minimap: { enabled: false }, + automaticLayout: true, + fontSize: 12, +}; export default function CodeViewer({ - const editorDefaultOptions = useMemo( - () => ({ - readOnly: true, - theme: 'vs-dark', - minimap: { enabled: false }, - automaticLayout: true, - fontSize: 12, - }), - [] - );
76-93: Enhance error handling in editor lifecycle.The editor initialization and cleanup are well-implemented, but consider adding error handling and race condition protection:
- Add error handling for editor creation
- Protect against potential race conditions during initialization
const initMonaco = () => { - if (editorContainer.current) { + try { + if (!editorContainer.current) return; + editor.current = monaco.editor.create(editorContainer.current, { language: language || 'plaintext', model: null, ...editorDefaultOptions, }); monacoManager.addEditor(editorId, editor.current); + } catch (error) { + console.error('Failed to initialize Monaco editor:', error); + // Consider adding error boundary or fallback UI } }; const destroyMonaco = () => { + // Protect against race conditions during cleanup + const currentEditor = editor.current; + if (!currentEditor) return; + + editor.current = null; try { - if (editor.current) { - editor.current.dispose(); - monacoManager.removeEditor(editorId); - } + currentEditor.dispose(); + monacoManager.removeEditor(editorId); + } catch (error) { + console.error('Failed to cleanup Monaco editor:', error); } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
backend/repository/result_repository_json_impl.go(2 hunks)frontend/src/components/CodeViewer.tsx(3 hunks)frontend/src/components/ScanDialog.tsx(3 hunks)frontend/src/components/SelectResultsFile.tsx(1 hunks)frontend/src/components/SelectScanRoot.tsx(2 hunks)frontend/src/components/SelectSettingsFile.tsx(2 hunks)frontend/src/hooks/useResults.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/hooks/useResults.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (16)
frontend/src/components/SelectSettingsFile.tsx (3)
26-26: LGTM! Good transition to the new results management hook.The change from direct store usage to a dedicated hook aligns with the PR's performance objectives and promotes better separation of concerns.
36-36: LGTM! Efficient results management through reset.The transition from fetching to resetting results is a good optimization, especially for big projects where unnecessary fetches could impact performance.
47-47: LGTM! Appropriate reset timing.Resetting results immediately after changing settings is the correct behavior, ensuring data consistency while avoiding unnecessary fetches.
backend/repository/result_repository_json_impl.go (1)
26-46: LGTM! Well-structured implementation of caching and file watching.The new fields and imports effectively support thread-safe caching and file system monitoring.
frontend/src/components/SelectResultsFile.tsx (3)
26-26: LGTM! Good architectural change.The shift from store-based to hook-based approach aligns with the performance enhancement objective.
40-40: LGTM! Good separation of concerns.Moving results management to a dedicated hook improves code organization and potentially reduces unnecessary re-renders.
47-47:❓ Verification inconclusive
Verify the timing of results fetching.
The change from immediate fetching to resetting suggests a performance optimization. However, please ensure:
- Results are fetched at an appropriate time later in the flow
- Users receive proper feedback during the process
- The UI remains responsive
Let's verify the implementation of result fetching in the new hook:
🏁 Script executed:
#!/bin/bash # Search for result fetching implementation in the useResults hook rg -A 10 "export.*useResults" frontend/src/hooks/Length of output: 894
Ensure deferred result fetching works as intended.
We've observed that the
SelectResultsFile.tsxcomponent now callsresetResults()instead of immediately triggeringfetchResults(). Please verify the following:
- Confirm that after calling
resetResults(), thefetchResultsfunction (as provided in theuseResultshook) is invoked at the correct later stage—for example, when a debounced query update or other user-triggered event occurs.- Check that any loading indicators or user feedback mechanisms are updated appropriately to maintain a smooth, responsive UI experience.
frontend/src/components/SelectScanRoot.tsx (3)
27-27: LGTM!The import of
useResultshook aligns with the broader changes in result management across components, supporting the performance enhancement objective.
45-45: LGTM!The transition from
fetchResultstoresetfunction improves performance by avoiding unnecessary data fetching operations. The clear aliasing asresetResultsmaintains code readability.
51-55:❓ Verification inconclusive
LGTM! Verify performance impact.
The sequence of operations is well-structured:
- Set new scan root
- Reset results to clear stale data
- Invalidate queries to ensure fresh data
This approach prevents stale data accumulation and maintains data consistency, which should improve performance when loading big projects.
To verify the performance improvement, please run the following script to check for any remaining references to the old pattern:
🏁 Script executed:
#!/bin/bash # Description: Check for any remaining instances of the old fetchResults pattern # that might impact performance # Search for any remaining fetchResults usage rg -A 5 'fetchResults' frontend/src/ # Search for useResultsStore usage rg -A 5 'useResultsStore' frontend/src/Length of output: 14012
Approved: Scan Root Update Sequence is Correct!
The changes in
frontend/src/components/SelectScanRoot.tsxare implemented in a proper sequence, ensuring that:
- The scan root is updated with the new directory.
- Stale data is cleared via
resetResults().- Cached queries are invalidated to prompt fresh data retrieval.
The performance verification script indicates that the legacy
fetchResultspattern is still present in several modules (e.g., infrontend/src/hooks/useResults.tsxand within the state store infrontend/src/modules/results/stores/useResultsStore.ts). These usages appear to be part of the existing state management and are not directly related to the logic inSelectScanRoot.tsx. However, please ensure that their continued usage does not inadvertently impact performance in scenarios involving large projects.
- Action Item: Verify that the legacy state management (i.e., the use of
fetchResults) in other parts of the codebase meets your performance criteria.Overall, LGTM!
frontend/src/components/ScanDialog.tsx (3)
32-32: LGTM! Import of new hook for result management.The addition of the
useResultshook aligns with the broader refactoring effort to improve result management across components.
71-71: LGTM! Clear and consistent hook usage.The destructuring and renaming of the
resetfunction toresetResultsmaintains clarity and aligns with the new result management approach.
119-120:✅ Verification successful
Verify performance impact of result management changes.
The sequence of clearing selected results followed by resetting all results is logical. Given the PR's focus on performance improvements for big projects, let's verify the impact:
🏁 Script executed:
#!/bin/bash # Description: Search for other result management patterns that might affect performance # Check for result management patterns echo "Searching for result management patterns..." rg -A 5 "useResults|fetchResults|resetResults" --type ts --type tsx # Check for performance-related comments or issues echo "Searching for performance-related comments..." rg -i "performance|slow|optimization" --type ts --type tsxLength of output: 384
Ensure that the new result-clearing sequence truly benefits performance at scale.
The updated code—first clearing the selected results and then resetting all results—appears logically sound, especially given its role in the scan completion flow. However, the initial verification attempt using ripgrep flagged a file type issue (unrecognized file type: tsx). To address this and better confirm that there aren’t any overlooked performance-related nuances, please run the revised verification script below:
Based on the script’s output, if no concerning patterns or performance comments are found, the change can be considered effective. Otherwise, please review any identified issues to ensure that performance improvements hold for large projects.
🏁 Script executed:
#!/bin/bash # Updated search for result management patterns and performance comments in TypeScript files echo "Searching for result management patterns in .ts and .tsx files..." rg -g "*.ts" -g "*.tsx" -A 5 "resetResults" echo "Searching for performance-related comments in .ts and .tsx files..." rg -g "*.ts" -g "*.tsx" -i "performance|slow|optimization"Length of output: 4064
Result Management Sequence Verified: No Performance Concerns Found
The ordering of clearing selected results before calling
resetResults()infrontend/src/components/ScanDialog.tsxis consistent with similar usage found in other parts of the codebase (e.g.,SelectScanRoot.tsx,SelectSettingsFile.tsx, andSelectResultsFile.tsx). The additional search for performance-related patterns/comments did not reveal any issues. This sequence supports the performance improvements targeted for large projects.frontend/src/components/CodeViewer.tsx (3)
30-39: LGTM! Props interface has been simplified.The removal of
editorTypeandisLoadingprops aligns well with the direct Monaco editor implementation, making the interface cleaner and more focused.
78-82: Verify language type safety.The type assertion
language as stringcould be unsafe iflanguageisnullorundefined. Consider adding a runtime check or using a default value.- language: language as string, + language: language || 'plaintext',
144-152: LGTM! Clean and efficient rendering logic.The error handling and editor container rendering are well-implemented, with proper error message extraction and minimal DOM structure.
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (1)
backend/entities/mocks/mock_FileWatcher.go (1)
1-123: 💡 Verification agent🧩 Analysis chain
Update mock after FileWatcher interface changes.
The mock implementation needs to be regenerated after adding context support to the
Start()method in theFileWatcherinterface.Run the following command to regenerate the mock:
🏁 Script executed:
#!/bin/bash # Verify the FileWatcher interface and regenerate mock ast-grep --pattern 'type FileWatcher interface { Start(ctx context.Context) error $$$ }' # If the pattern matches, regenerate the mock if [ $? -eq 0 ]; then echo "Interface updated. Regenerate mock using: mockery --name FileWatcher --dir backend/entities" fiLength of output: 305
Mock regeneration required: Update for new context parameter
- The current mock implementation of the
Start()method does not include the requiredctx context.Contextparameter as defined in the updatedFileWatcherinterface.- Since the interface now specifies
Start(ctx context.Context) error, please regenerate the mock to ensure it correctly reflects the new signature.- Run the following command to update the mock:
mockery --name FileWatcher --dir backend/entities
🧹 Nitpick comments (5)
frontend/src/components/CodeViewer.tsx (1)
92-104: Enhance model cleanup in updateContent.While the model disposal has been improved as per past review comments, it could be more robust.
Consider this enhancement:
const updateContent = () => { const { current: mEditor } = editor; let oldModel: monaco.editor.ITextModel | null = null; if (mEditor) { oldModel = mEditor.getModel(); + if (!content) { + if (oldModel) { + oldModel.dispose(); + mEditor.setModel(null); + } + return; + } const nModel = monaco.editor.createModel(content as string, language as string); mEditor.setModel(nModel); if (oldModel) oldModel.dispose(); } };backend/repository/component_repository_json_impl.go (1)
72-78: Consider adding error handling for license ordering.The license ordering operation could potentially modify the component data but doesn't handle any errors that might occur during the sort operation.
Consider wrapping the operation in a separate error-handling function:
- // Order component licenses by source - if len(components[0].Licenses) > 0 { - r.orderComponentLicensesBySourceType(&components[0]) - } - return components[0], nil + component := components[0] + if err := r.orderLicenses(&component); err != nil { + return entities.Component{}, fmt.Errorf("failed to order licenses: %w", err) + } + return component, nil +func (r *JSONComponentRepository) orderLicenses(component *entities.Component) error { + if len(component.Licenses) == 0 { + return nil + } + r.orderComponentLicensesBySourceType(component) + return nil +}main.go (1)
78-82: Improve error message with more details.The error message could be more descriptive by including the actual error.
- return fmt.Errorf("error initializing results repository") + return fmt.Errorf("error initializing results repository: %v", err)backend/repository/result_repository_json_impl.go (2)
80-87: Add error recovery mechanism for cache refresh failures.The cache refresh error in the watcher callback is logged but not handled. Consider adding a recovery mechanism or fallback to the previous cache state.
watcher, err := entities.NewFsNotifyWatcher(resultPath, func() { repo.mutex.Lock() defer repo.mutex.Unlock() + oldCache := repo.cache + oldIndex := repo.pathIndex if err := repo.refreshCache(); err != nil { log.Error().Err(err).Msg("Error refreshing cache after file change") + // Rollback to previous state + repo.cache = oldCache + repo.pathIndex = oldIndex } })
127-154: Consider adding cache expiration mechanism.The cache implementation could benefit from an expiration mechanism to prevent memory growth in long-running applications.
Consider adding:
- Maximum cache size limit
- Time-based cache expiration
- Memory usage monitoring
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
backend/entities/mocks/mock_FileWatcher.go(1 hunks)backend/entities/watcher.go(1 hunks)backend/repository/component_repository_json_impl.go(1 hunks)backend/repository/mocks/mock_ResultRepository.go(1 hunks)backend/repository/result_repository_json_impl.go(2 hunks)backend/repository/result_repository_json_impl_test.go(4 hunks)backend/service/component_service_test.go(1 hunks)backend/service/mocks/mock_ResultService.go(2 hunks)frontend/src/components/CodeViewer.tsx(3 hunks)main.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (12)
frontend/src/components/CodeViewer.tsx (3)
25-25: LGTM! Well-structured imports and editor configuration.The imports are minimal and the editor default options are well-defined with reasonable defaults.
Also applies to: 30-36
38-47: LGTM! Clean and well-defined props interface.The interface has been simplified by removing unnecessary props while maintaining clear type definitions.
143-152: LGTM! Clean rendering logic with proper error handling.The component properly handles error states and renders efficiently.
backend/repository/component_repository_json_impl.go (1)
68-70: Empty components check is now handled correctly.The code now properly checks for empty components before accessing the first element, addressing the potential index out-of-range panic issue mentioned in previous reviews.
backend/service/mocks/mock_ResultService.go (1)
83-114:✅ Verification successful
Verify SetContext implementation in ResultService interface.
The mock includes a new
SetContextmethod, but we should verify this matches the actual interface implementation.Run the following command to verify the interface consistency:
🏁 Script executed:
#!/bin/bash # Verify ResultService interface includes SetContext method ast-grep --pattern 'type ResultService interface { $$$ SetContext(ctx context.Context) $$$ }' # If the pattern doesn't match, the mock may need to be updated if [ $? -ne 0 ]; then echo "Warning: SetContext method may not match interface definition" fiLength of output: 403
Interface Consistency Verified: SetContext Implementation Matches the ResultService Interface
The shell script verification confirms that the
SetContext(ctx context.Context)method in theResultServiceinterface (located inbackend/service/result_service.go) correctly matches the mock implementation inMockResultService. No further changes are required.backend/repository/result_repository_json_impl_test.go (1)
48-49: LGTM! Test cases properly handle the new constructor signature.The test cases have been correctly updated to handle the new constructor signature that includes the
watcherparameter and error return value.Also applies to: 65-66, 79-80, 90-91, 103-104
main.go (1)
82-83: LGTM! Proper resource cleanup with defer.The
Close()method is properly deferred to ensure cleanup of repository resources.backend/repository/mocks/mock_ResultRepository.go (1)
23-49: LGTM! Mock implementation is correctly generated.The mock implementation for
GetResultByPathis properly generated with appropriate type checking and error handling.backend/repository/result_repository_json_impl.go (3)
39-46: LGTM! Thread-safe cache implementation.The struct is well-designed with:
- Cache and index for fast lookups
- RWMutex for thread-safe access
- File watcher for cache invalidation
184-194: LGTM! Efficient path-based lookup with index.The
GetResultByPathimplementation efficiently uses the path index with proper thread safety.
196-198: LGTM! Proper cleanup of watcher resources.The
Closemethod ensures proper cleanup of the file watcher resources.backend/service/component_service_test.go (1)
26-27: Consider handling initialization errors.The changes look good and align with the performance enhancement objectives. However, consider capturing and asserting the error from
NewResultRepositoryJsonImplto ensure proper error handling in test scenarios.Apply this diff to improve error handling:
-resultRepo, _ := repository.NewResultRepositoryJsonImpl(fr, nil) +resultRepo, err := repository.NewResultRepositoryJsonImpl(fr, nil) +require.NoError(t, err)
matiasdaloia
deleted the
feature/mdaloia/ES-198-SCANOSS-Code-Compare-Enhance-performance-for-big-projects
branch
Summary by CodeRabbit
New Features
useResults, has been introduced to manage and fetch results more effectively.Refactor
MatchComparisoncomponent now manages loading states more effectively, improving user experience.JSONComponentRepositoryhas been updated to simplify result retrieval without file I/O operations.ResultRepositoryJsonImplnow features a file watching mechanism for enhanced cache management.Resultstruct has been updated to reflect a new data structure for matches, enhancing data processing.