feat: ES-141 UI for modifying scanoss.json skip functionality #98
Conversation
WalkthroughThis update introduces a wide range of modifications across both backend and frontend. The Makefile command now prepends a debug flag. In the backend, new constants, types, and functions were added for tree representations and enhanced error handling and scanning skip pattern management. Repository, service, and mock implementations have been updated to return pointers instead of tuple values. On the frontend, several new UI components, dialogs, and a Zustand store for tree state management were added. Dependency updates, alias additions, and formatting improvements were also applied, along with updates in WailsJS service interfaces and the main application binding. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AppSettings
participant RuntimeEvents
participant SettingsDialog
User->>AppSettings: Clicks settings icon/shortcut
AppSettings->>RuntimeEvents: Listens for 'OpenSettings' event
RuntimeEvents-->>AppSettings: Emits 'OpenSettings'
AppSettings->>SettingsDialog: Opens settings modal
sequenceDiagram
participant ResultsTree
participant TreeService
participant ResultService
participant SettingsRepo
ResultsTree->>TreeService: GetTree(rootPath)
TreeService->>ResultService: Fetch result (GetByPath)
TreeService->>SettingsRepo: Retrieve skip patterns
SettingsRepo-->>TreeService: Return skip patterns
ResultService-->>TreeService: Return result data
TreeService-->>ResultsTree: Send tree structure
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (26)
🚧 Files skipped from review as they are similar to previous changes (15)
🔇 Additional comments (38)
✨ Finishing Touches
🪧 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 (22)
.github/workflows/release.yml (1)
44-46: Pinned Wails Version for macOS Builds.
Explicitly installing[email protected]in the macOS build step should help maintain build stability. For consistency, consider pinning the Wails version in the Linux and Windows jobs as well if those environments require identical behavior.backend/service/result_service.go (1)
34-34: Consider consistent error handling patternThe new
GetByPathmethod doesn't return an error, unlike the existingGetAllmethod. This creates an inconsistency in the error handling pattern of the interface.Consider updating the method signature to include an error return:
- GetByPath(path string) entities.ResultDTO + GetByPath(path string) (entities.ResultDTO, error)This would maintain a consistent error handling pattern across the interface and allow callers to handle potential errors explicitly.
frontend/src/main.tsx (1)
51-61: Removal of React StrictMode WrapperThe
<StrictMode>wrapper has been removed in the updated render tree. While this may be an intentional decision for this release, note that StrictMode is valuable for highlighting potential issues during development. Please confirm that the removal is deliberate and that any checks normally enforced by StrictMode are handled elsewhere.frontend/src/components/SettingsSidebar.tsx (1)
35-60: Component structure follows React best practicesThe component is well-structured with clear separation of UI elements and proper use of props. The use of a mapping function to render settings options promotes code reusability and maintainability.
Consider adding aria attributes for improved accessibility:
<Button variant={activeSetting === option.id ? 'default' : 'outline'} size="sm" className="justify-start" onClick={() => setActiveSetting(option.id)} key={option.id} + aria-selected={activeSetting === option.id} + aria-label={`Select ${option.title} settings`} >frontend/src/components/ResultsTreeNode.tsx (3)
80-99: Mutation logic could benefit from additional error handlingThe mutation implementation for
toggleScanningSkipPatternis generally good, but the error handling could be improved by providing more user-friendly error messages.onError: (error, pattern) => { console.error(`Error adding skip pattern: ${pattern}`, error); setLoadingNodeId(null); + toast({ + title: 'Error', + description: `Failed to update skip pattern "${pattern}". Please try again.`, + variant: 'destructive', + }); },
126-142: Extension validation with proper error feedbackThe extension validation logic is good, providing clear feedback when invalid extensions are encountered. Consider improving the validation by also checking for empty extensions.
const handleToggleSkipExtension = () => { if (loadingNodeId !== null) return; const extension = path.extname(node.data.path); - if (!validExtensionRegex.test(extension)) { + if (!extension || !validExtensionRegex.test(extension)) { toast({ title: 'Invalid Extension', - description: `Cannot skip files with extension "${extension}"`, + description: extension ? `Cannot skip files with extension "${extension}"` : 'File has no extension', variant: 'destructive', }); return; }
178-234: Context menu implementation with clear visual feedbackThe context menu implementation provides good user experience with clear visual feedback. The conditional rendering based on node state is well-implemented.
One improvement could be adding keyboard accessibility for the context menu actions:
<ContextMenuItem onClick={node.data.isFolder ? handleToggleSkipFolder : handleToggleSkipFile} + onKeyDown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault(); + node.data.isFolder ? handleToggleSkipFolder() : handleToggleSkipFile(); + } + }} asChild >frontend/src/components/SaveSkipSettingsButton.tsx (2)
8-8: Consider using a more explicit import path.The import path is using a relative path that navigates up multiple directories (
../../wailsjs). Consider using an absolute import with an alias for better maintainability, similar to your other imports like@/hooks/useSkipPatternStore.-import { CommitStagedSkipPatterns, DiscardStagedSkipPatterns } from '../../wailsjs/go/service/ScanossSettingsServiceImp'; +import { CommitStagedSkipPatterns, DiscardStagedSkipPatterns } from '@/wailsjs/go/service/ScanossSettingsServiceImp';
44-67: Consider refactoring to reduce code duplication.The
discardStagedSkipPatternsfunction is very similar tocommitStagedSkipPatterns. Consider extracting the common logic into a shared helper function to improve maintainability.+ const handleSkipPatternAction = async ( + action: () => Promise<void>, + successTitle: string, + successDescription: string + ) => { + try { + setIsLoading(true); + await action(); + + toast({ + title: successTitle, + description: successDescription, + }); + + queryClient.invalidateQueries({ queryKey: ['resultsTree', scanRoot] }); + setHasUnsavedChanges(false); + clearNodesWithUnsavedChanges(); + } catch (error) { + console.error(`Error with skip patterns action: ${successTitle}`, error); + toast({ + title: 'Error', + description: `Failed to ${successTitle.toLowerCase()} scanning skip patterns`, + variant: 'destructive', + }); + } finally { + setIsLoading(false); + } + }; + - const commitStagedSkipPatterns = async () => { - try { - setIsLoading(true); - await CommitStagedSkipPatterns(); - - toast({ - title: 'Success', - description: 'Scanning skip patterns saved successfully', - }); - - queryClient.invalidateQueries({ queryKey: ['resultsTree', scanRoot] }); - setHasUnsavedChanges(false); - clearNodesWithUnsavedChanges(); - } catch (error) { - console.error('Error saving skip patterns', error); - toast({ - title: 'Error', - description: 'Failed to save scanning skip patterns', - variant: 'destructive', - }); - } finally { - setIsLoading(false); - } - }; + const commitStagedSkipPatterns = () => + handleSkipPatternAction( + CommitStagedSkipPatterns, + 'Success', + 'Scanning skip patterns saved successfully' + ); - const discardStagedSkipPatterns = async () => { - try { - setIsLoading(true); - await DiscardStagedSkipPatterns(); - - toast({ - title: 'Changes Discarded', - description: 'Scanning skip pattern changes have been discarded', - }); - - queryClient.invalidateQueries({ queryKey: ['resultsTree', scanRoot] }); - setHasUnsavedChanges(false); - clearNodesWithUnsavedChanges(); - } catch (error) { - console.error('Error discarding skip patterns', error); - toast({ - title: 'Error', - description: 'Failed to discard scanning skip patterns', - variant: 'destructive', - }); - } finally { - setIsLoading(false); - } - }; + const discardStagedSkipPatterns = () => + handleSkipPatternAction( + DiscardStagedSkipPatterns, + 'Changes Discarded', + 'Scanning skip pattern changes have been discarded' + );frontend/src/components/ResultsTree.tsx (4)
32-32: Consider using an alias for the import path.Similar to the issue in SaveSkipSettingsButton.tsx, the import for
GetTreeuses a relative path. Consider using an alias for consistency with other imports.-import { GetTree } from '../../wailsjs/go/service/TreeServiceImpl'; +import { GetTree } from '@/wailsjs/go/service/TreeServiceImpl';
65-75: State management for expanded nodes could be improved.The current implementation of
handleNodeToggleinverts the expanded state by checking if the node is already expanded. This logic is reversed from the typical pattern - usually you'd add a node to expandedNodes when expanding and remove when collapsing.- const handleNodeToggle = useCallback((nodeId: string, isExpanded: boolean) => { - setExpandedNodes((prev) => { - const newExpandedNodes = new Set(prev); - if (isExpanded) { - newExpandedNodes.delete(nodeId); - } else { - newExpandedNodes.add(nodeId); - } - return newExpandedNodes; - }); - }, []); + const handleNodeToggle = useCallback((nodeId: string, isExpanded: boolean) => { + setExpandedNodes((prev) => { + const newExpandedNodes = new Set(prev); + if (isExpanded) { + newExpandedNodes.add(nodeId); + } else { + newExpandedNodes.delete(nodeId); + } + return newExpandedNodes; + }); + }, []);
106-116: Props passing looks good but could benefit from type checking.The
Nodecomponent receives many props, including state and callbacks. Consider creating a dedicated interface for these props to improve type safety and documentation.Create a dedicated interface for the Node component props:
interface NodeProps { selectedNode: string | null; expandedNodes: Set<string>; onNodeSelect: (nodeId: string) => void; onNodeToggle: (nodeId: string, isExpanded: boolean) => void; loadingNodeId: string | null; setLoadingNodeId: React.Dispatch<React.SetStateAction<string | null>>; // Include other props from the Tree component }
119-119: Consider more descriptive component name.The
SaveButtoncomponent is actuallySaveSkipSettingsButtonas imported, but is renamed in import. Consider using the full name for clarity or using a more descriptive name in the import statement.-import SaveButton from './SaveSkipSettingsButton'; +import SaveSkipSettingsButton from './SaveSkipSettingsButton'; // Then in the render function: - <SaveButton /> + <SaveSkipSettingsButton />backend/entities/scanoss_settings.go (2)
222-222: Single directory extension definedThe
.egg-infoextension is properly identified as a directory extension to skip. Consider adding a comment explaining why this particular directory extension needs special handling.
224-395: Extensive file extension list for skippingThe comprehensive list of file extensions to skip covers all common binary, documentation, configuration, and image formats. The comment at line 376 for "File endings" is helpful to distinguish between extensions and name patterns.
One consideration: this large static list might be difficult to maintain over time. Consider if a configuration-driven approach might be more flexible for users to customize in the future.
backend/service/tree_service_impl_test.go (1)
1-23: Check copyright yearThe copyright notice extends to 2025, which appears to be a future date.
-// Copyright (C) 2018-2025 SCANOSS.COM +// Copyright (C) 2018-2024 SCANOSS.COMbackend/service/tree_service_impl.go (3)
39-43: Unused worker limit field.The
workerLimitfield is computed but not enforced in this implementation. This can lead to uncontrolled concurrency if the directory structure is large.Consider using a semaphore or worker pool to ensure concurrency is bounded and to respect the configured worker limit.
54-68: Tree root node is excluded from the final return.
GetTreeonly returnsroot.Children, which omits information about the root directory itself. This may be intentional, but confirm if the caller expects the full tree including the top-level directory.Consider returning the entire
rootinstead if you want to preserve the root node details.
226-228: Hidden file detection is UNIX-centric.
isHiddenFileOrFolderonly checks the leading".". On Windows, hidden files rely on file attributes. If multi-platform compatibility is required, consider a system call to check the hidden attribute.backend/service/mocks/mock_ScanossSettingsService.go (1)
20-36: Ensure default return value for robust testing.
This mock function panics if no return value is specified, which may cause unexpected test failures if an expectation isn't set.You could define a default error value to avoid panics in scenarios where the test code doesn’t explicitly set the return value.
backend/repository/scanoss_settings_repository_json_impl.go (2)
34-34: Gitignore matcher import.
This import is crucial for pattern matching. Ensuring version compatibility and tracking potential updates in go-git might be beneficial in the future.
381-432: ToggleScanningSkipPattern logic covers multiple scenarios.
The method carefully manages negation patterns, staged additions, and removals, resetting the effective cache at the end. Concurrency is handled by retrieving the effective patterns in place.Consider adding more unit tests for edge cases (e.g., toggling patterns already negated, toggling default patterns not in the list, etc.) to ensure robust coverage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
frontend/package-lock.jsonis excluded by!**/package-lock.jsongo.sumis excluded by!**/*.sum
📒 Files selected for processing (58)
.github/workflows/release.yml(1 hunks)Makefile(1 hunks)app.go(2 hunks)backend/entities/keyboard.go(4 hunks)backend/entities/result.go(1 hunks)backend/entities/scanoss_settings.go(1 hunks)backend/entities/tree.go(1 hunks)backend/mappers/result_mapper_impl.go(2 hunks)backend/repository/component_repository_json_impl.go(1 hunks)backend/repository/mocks/mock_ResultRepository.go(2 hunks)backend/repository/mocks/mock_ScanossSettingsRepository.go(4 hunks)backend/repository/result_repository.go(1 hunks)backend/repository/result_repository_json_impl.go(1 hunks)backend/repository/scanoss_settings_repository.go(1 hunks)backend/repository/scanoss_settings_repository_json_impl.go(4 hunks)backend/service/mocks/mock_ResultService.go(1 hunks)backend/service/mocks/mock_ScanossSettingsService.go(2 hunks)backend/service/mocks/mock_TreeService.go(1 hunks)backend/service/result_service.go(1 hunks)backend/service/result_service_impl.go(1 hunks)backend/service/scanoss_settings_service.go(1 hunks)backend/service/scanoss_settings_service_impl.go(1 hunks)backend/service/tree_service.go(1 hunks)backend/service/tree_service_impl.go(1 hunks)backend/service/tree_service_impl_test.go(1 hunks)frontend/components.json(1 hunks)frontend/package.json(3 hunks)frontend/package.json.md5(1 hunks)frontend/src/components/AppSettings.tsx(1 hunks)frontend/src/components/NewComponentDialog.tsx(1 hunks)frontend/src/components/ReplaceComponentDialog.tsx(1 hunks)frontend/src/components/ResultsTree.tsx(1 hunks)frontend/src/components/ResultsTreeNode.tsx(1 hunks)frontend/src/components/SaveSkipSettingsButton.tsx(1 hunks)frontend/src/components/ScanDialog.tsx(1 hunks)frontend/src/components/SettingsDialog.tsx(1 hunks)frontend/src/components/SettingsSidebar.tsx(1 hunks)frontend/src/components/SkipSettings.tsx(1 hunks)frontend/src/components/StatusBar.tsx(3 hunks)frontend/src/components/ui/card.tsx(1 hunks)frontend/src/components/ui/context-menu.tsx(1 hunks)frontend/src/components/ui/dialog.tsx(3 hunks)frontend/src/hooks/useSkipPatternStore.tsx(1 hunks)frontend/src/lib/settings.tsx(1 hunks)frontend/src/lib/shortcuts.ts(1 hunks)frontend/src/main.tsx(1 hunks)frontend/src/providers/DialogProvider.tsx(1 hunks)frontend/src/routes/root.tsx(2 hunks)frontend/src/style.css(1 hunks)frontend/wailsjs/go/models.ts(2 hunks)frontend/wailsjs/go/service/ResultServiceImpl.d.ts(1 hunks)frontend/wailsjs/go/service/ResultServiceImpl.js(1 hunks)frontend/wailsjs/go/service/ScanossSettingsServiceImp.d.ts(1 hunks)frontend/wailsjs/go/service/ScanossSettingsServiceImp.js(2 hunks)frontend/wailsjs/go/service/TreeServiceImpl.d.ts(1 hunks)frontend/wailsjs/go/service/TreeServiceImpl.js(1 hunks)go.mod(2 hunks)main.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (115)
frontend/wailsjs/go/service/ResultServiceImpl.js (1)
9-11: Looks good - New function follows established pattern.The addition of
GetByPathfunction follows the same pattern as existing functions and properly calls the Go backend method. This function will be useful for retrieving individual result items by path, supporting the skip pattern functionality features.frontend/wailsjs/go/service/ResultServiceImpl.d.ts (1)
8-8: TypeScript definition matches implementation.The type definition correctly specifies that
GetByPathtakes a string path parameter and returns a Promise resolving to aResultDTO. This matches the JavaScript implementation and provides proper type safety.frontend/package.json.md5 (1)
1-1: MD5 hash update reflects package.json changes.The updated MD5 hash reflects changes to the package.json file, likely including the new dependencies mentioned in the summary (
@radix-ui/react-context-menu,path-browserify,react-arborist, anduse-resize-observer). These dependencies align with the PR's goal of enhancing the UI for skip pattern management.backend/entities/result.go (1)
111-111: New WorkflowState value fits well with existing constants.The addition of the
Mixedworkflow state is a well-designed extension to the existing states. This new state will be useful for representing directories or groups where some files are skipped and others aren't, which is essential for the skip pattern management UI.frontend/wailsjs/go/service/TreeServiceImpl.d.ts (1)
1-6: Auto-generated Declaration File is Correct.
This file correctly exports theGetTreefunction with the expected signature and return type. Since it is auto-generated, no manual changes are expected.frontend/wailsjs/go/service/TreeServiceImpl.js (1)
1-7: Auto-generated JS Service Wrapper is Solid.
TheGetTreefunction serves as a clear proxy to the Go backend service. Ensure that the backend'sTreeServiceImplmethod remains in sync with this declaration.frontend/src/components/NewComponentDialog.tsx (1)
62-62: Enhanced Styling with Padding.
AddingclassName="p-4"toDialogContentimproves the padding and aligns the styling with other dialog components across the project.frontend/src/components/ScanDialog.tsx (1)
242-244: Consistent Dialog Padding Applied.
IntroducingclassName="p-4"forDialogContenthere ensures a uniform look and feel with other dialogs like inNewComponentDialog.tsx.backend/service/tree_service.go (1)
28-30: Interface design looks good!The TreeService interface is well-designed with a clear method signature. It follows Go's idiomatic approach for defining service interfaces, with appropriate return types for tree structures and proper error handling.
frontend/src/providers/DialogProvider.tsx (1)
102-102: UI enhancement with better spacingAdding the "p-4" class provides consistent padding to the dialog content, improving the visual appearance and readability of dialogs throughout the application.
frontend/src/lib/shortcuts.ts (1)
152-156: Good addition of settings shortcutThe keyboard shortcut for opening settings uses the conventional "mod+," key combination (Command+, on Mac, Ctrl+, on Windows), which aligns with user expectations and industry standards.
Makefile (1)
50-52: Enhanced Debug Flag in Development ModeThe modified
runtarget now prepends--debugto$(APPARGS)when running in development mode. This can help with troubleshooting by ensuring debug information is available during development. Please verify that this debug flag is only active in development and does not inadvertently affect production builds.frontend/components.json (1)
15-18: New Alias Mappings AddedThe addition of extra alias mappings for
"utils","ui","lib", and"hooks"enhances module resolution and code maintainability on the frontend. Please double-check that these new paths (e.g.,@/components/ui,@/lib, and@/hooks) accurately reflect the project’s directory structure.frontend/src/components/ReplaceComponentDialog.tsx (1)
140-140: Added Padding to Dialog ContentAdding the
className="p-4"to the<DialogContent>component ensures better spacing within the dialog, aligning its appearance with similar components elsewhere in the application. This visual tweak improves overall user experience.frontend/src/style.css (1)
91-106: Global Base Layer Styling with Tailwind's @applyIntroducing a new base layer that applies
border-borderandoutline-ring/50on all elements—and settingbg-backgroundandtext-foregroundon thebody—ensures a consistent look and feel across the application. Just verify that the utility classes used (e.g.,border-border,outline-ring/50,bg-background, andtext-foreground) are properly defined in your Tailwind configuration.frontend/src/components/StatusBar.tsx (3)
28-28: Added AppSettings component import.The AppSettings component is now imported to enhance the StatusBar functionality with access to application settings.
41-41: Improved layout with flexbox styling.The outer div has been enhanced with flexbox styling (
flex justify-between) and vertical padding (py-1), creating a more balanced layout where elements can be properly spaced between the left and right sides of the StatusBar.
56-58: Added AppSettings component to StatusBar.The AppSettings component has been properly integrated into the right side of the StatusBar, providing users with easy access to application settings.
backend/repository/result_repository.go (1)
30-30:✅ Verification successful
API change: GetResultByPath now returns a pointer instead of a value and error.
This signature change from
(entities.Result, error)to*entities.Resulthas two important implications:
- Error handling is now done internally or via nil checking
- Return value changed from value type to pointer type
This change can simplify calling code but requires careful handling for nil returns. Ensure all consumers of this API are updated to handle potential nil returns instead of checking errors.
🏁 Script executed:
#!/bin/bash # Check for implementations of this interface and consumers of GetResultByPath echo "Checking implementations of ResultRepository interface..." rg -t go "type \w+? struct.*?implements ResultRepository" -A 3 echo "\nChecking for implementations of GetResultByPath..." rg -t go "func \(\w+ \*\w+\) GetResultByPath" -A 3 echo "\nChecking consumers of GetResultByPath..." rg -t go "GetResultByPath\(.*?\)" -A 2Length of output: 3135
API Update: GetResultByPath now returns a pointer instead of a value and error.
We verified that:
- The implementation in
backend/repository/result_repository_json_impl.go(and its mocks) follows the updated signature:func (r *ResultRepositoryJsonImpl) GetResultByPath(path string) *entities.Result.- Consumers in files such as
backend/component_repository_json_impl.goandbackend/service/result_service_impl.goappropriately check fornilto handle missing results instead of error returns.All evidence indicates that the codebase has been updated consistently to use nil-checking for error handling. Please ensure that any further changes continue to observe this pattern.
frontend/src/lib/settings.tsx (2)
5-11: Well-defined SettingsOption interface.The interface provides a clear structure for settings options with appropriate fields for ID, title, description, icon, and component. The TypeScript typing is correct and comprehensive.
13-21: Initial implementation of skip settings option.The SETTINGS_OPTIONS array is properly structured with a single option for skip settings. The implementation follows best practices by:
- Defining a clear title and description
- Using appropriate icon from lucide-react
- Rendering the SkipSettings component
This structure will facilitate adding more settings options in the future.
app.go (2)
121-121: Added view shortcuts for dynamic menu generation.The addition of viewShortcuts retrieves grouped shortcuts for the view section of the application menu.
136-140: Implemented dynamic menu item generation for view shortcuts.This change replaces hardcoded menu items with a dynamic approach that:
- Loops through all viewShortcuts
- Creates menu entries using each shortcut's name and accelerator
- Emits the appropriate action event when a menu item is selected
This implementation enhances flexibility by automatically reflecting any changes to view shortcuts in the application menu.
main.go (2)
99-99: Correct implementation of new tree serviceThe new tree service is properly initialized with the necessary dependencies (resultService and scanossSettingsRepository). This follows the established pattern for service initialization in the application.
125-125: Properly registered service in application bindingsThe treeService is correctly added to the application's Bind array, making it accessible within the application's context.
backend/repository/component_repository_json_impl.go (1)
61-64: Error handling approach changed correctlyThe implementation has been updated to handle nil results instead of checking for errors. This is a valid pattern change that aligns with updates to the
GetResultByPathmethod signature, which now returns a pointer that can be nil instead of returning an error separately.backend/service/result_service_impl.go (1)
111-119: Well-implemented GetByPath methodThe new GetByPath method is properly implemented with:
- Correct repository method call
- Appropriate nil check for the result
- Good debug logging when no result is found
- Proper mapping of the result to DTO when found
This follows the established patterns in the codebase.
frontend/src/hooks/useSkipPatternStore.tsx (1)
1-32: Well-structured Zustand store for skip pattern managementThis store implementation follows React and Zustand best practices:
- Clear interface definition with proper typing
- Immutable state updates using Zustand's set function
- Proper handling of Set operations by creating new instances
- Organized methods for state manipulation
The store will effectively track unsaved changes and affected nodes for the skip pattern functionality.
frontend/wailsjs/go/service/ScanossSettingsServiceImp.d.ts (2)
5-7: Interface additions properly implemented for skip pattern managementThe new
CommitStagedSkipPatternsandDiscardStagedSkipPatternsfunctions are well-defined, providing necessary functionality to manage skip patterns in a staged approach. This follows good design principles by allowing changes to be prepared and then either committed or discarded.
15-15: Proper type definition for toggle functionalityThe
ToggleScanningSkipPatternfunction with a string argument is correctly typed and maintains consistency with the backend interface. This provides the essential functionality to toggle individual skip patterns.backend/service/scanoss_settings_service.go (1)
29-31: Interface updates match frontend API definitionsThe three new method signatures in the Go interface align perfectly with the TypeScript definitions, providing a consistent API between frontend and backend. The error return type follows Go conventions and allows proper error handling.
frontend/src/components/SettingsSidebar.tsx (1)
30-33: Props interface is well-definedThe
SettingsSidebarPropsinterface clearly defines the component's API with appropriate types foractiveSettingandsetActiveSetting.frontend/src/components/ResultsTreeNode.tsx (3)
37-48: Constants are well-defined and enhance code readabilityThe
validExtensionRegex,SKIP_STATES, andSKIP_STATE_DESCRIPTIONSconstants are properly defined and increase the maintainability of the code. The regex for validating extensions is appropriate.
50-58: TreeNode interface is comprehensive and well-structuredThe
TreeNodeinterface includes all necessary properties for representing tree nodes, with appropriate types and clear naming conventions.
144-176: Callback functions well-optimized with useCallbackGreat use of
useCallbackfor the menu text generation functions, which helps with performance optimization. The dependency arrays are correctly defined to ensure the callbacks are only regenerated when needed.backend/repository/result_repository_json_impl.go (1)
183-193: Good refactoring of the return typeThe change from returning
(entities.Result, error)to a pointer*entities.Resultsimplifies the error handling pattern. This is a clean approach that follows Go idioms where nil can represent the "not found" case without needing an explicit error return.frontend/wailsjs/go/models.ts (2)
27-27: Good addition of the OpenSettings actionThis new action enum value properly integrates with the keyboard shortcut system to support opening the settings modal.
602-644: Well-structured TreeNode class implementationThe new TreeNode class follows the same structure and patterns as other model classes in the file:
- Complete with proper type definitions
- Includes the standard createFrom static method
- Implements the convertValues helper method
- Handles recursive structures via the children property
This will provide a good foundation for the file tree display and skip pattern management.
frontend/wailsjs/go/service/ScanossSettingsServiceImp.js (2)
5-11: Good implementation of stage management functionsThese new functions enable the commit/discard pattern for managing staged changes to skip patterns, which follows good UI practices by allowing users to preview changes before applying them.
25-27: Well-implemented toggle functionThe ToggleScanningSkipPattern function properly accepts a parameter, allowing for dynamic pattern toggling.
frontend/src/routes/root.tsx (1)
38-38: Code simplification with direct state managementThe refactoring simplifies the component by:
- Using a more descriptive state variable name (showScanModal)
- Directly setting state in the event handler
- Using inline function for the onOpenChange prop
This reduces unnecessary function declarations while maintaining the same functionality.
Also applies to: 46-46, 69-69
frontend/src/components/SkipSettings.tsx (4)
24-30: Appropriate imports for component functionalityThe component correctly imports the necessary dependencies, including the Lucide icon, resize observer hook, and custom UI components. This follows best practices for modular React component design.
31-33: Well-structured component with resize observationThe component function is appropriately defined with default export, and the resize observer hook is properly implemented to capture dimensions that will be passed to the tree component.
34-63: Clear and helpful tooltip implementationThe tooltip with scanning status indicators provides excellent user guidance. The use of color indicators (green, red, yellow) with descriptive text helps users understand the different states, and the right-click instruction clearly explains how to interact with the interface.
65-70: Proper responsive layout with dimension passingThe component correctly uses flexbox for layout and passes the observed dimensions to the ResultsTree component. The
minBlockSize: 0style ensures proper sizing behavior within flex containers.backend/entities/keyboard.go (3)
58-58: Appropriate addition of settings actionThe new action constant
ActionOpenSettingsis properly defined, consistent with existing naming conventions in the codebase.
111-111: Consistent action registrationThe action is correctly added to the
AllShortcutActionsarray, ensuring it's properly registered in the application.
304-311: Well-implemented keyboard shortcutThe settings shortcut uses
mod+,which is an industry-standard key combination for accessing settings. The implementation follows the established pattern in the codebase with proper name, description, accelerator, and group assignment.frontend/src/components/SettingsDialog.tsx (4)
24-31: Well-organized imports and component structureThe imports are properly organized and the component structure follows React best practices with clear separation of UI components and custom components.
32-35: Clear TypeScript interface definitionThe props interface is well-defined with appropriate types, making the component's API clear and type-safe.
37-42: Efficient state management with useMemoThe component correctly uses
useStatefor managing the active setting anduseMemoto derive dependent values, which optimizes performance by preventing unnecessary recalculations.
43-63: Responsive and accessible dialog implementationThe dialog layout is well-structured with a sidebar and content area. The use of ScrollArea ensures content remains accessible regardless of size. The dialog dimensions are defined using modern viewport units (
dvh,dvw) for better responsiveness.backend/service/scanoss_settings_service_impl.go (3)
53-55: Clean implementation of commit functionalityThe
CommitStagedSkipPatternsmethod correctly delegates to the repository layer, maintaining the service's role in the application architecture.
57-59: Proper discard implementationThe
DiscardStagedSkipPatternsmethod follows the same pattern as other methods in the service, delegating responsibility to the repository layer.
61-63: Consistent toggle pattern implementationThe
ToggleScanningSkipPatternmethod accepts a pattern string parameter and delegates to the repository, maintaining a clean separation of concerns.frontend/src/components/AppSettings.tsx (2)
32-43: Well-structured component with proper event handlingThe
AppSettingscomponent is well-implemented using React hooks for state management and event handling. The use ofEventsOnto listen for external events is a good pattern for integrating with the Wails runtime.
44-57: Clean UI implementation with accessibility considerationsThe component renders a well-structured UI with proper accessibility considerations:
- Uses tooltip for improved user experience
- Includes appropriate cursor styling and hover states
- Properly manages modal state through props
The component follows React best practices by conditionally rendering the settings dialog based on state.
backend/entities/tree.go (3)
9-17: Well-designed TreeNode struct with all necessary fieldsThe
TreeNodestruct is well-designed with comprehensive fields for tree representation, including unique ID, name, path, folder status, and hierarchical relationships via the Children field.
19-25: Clear enum pattern for SkipStateGood implementation of the enum pattern using string constants, which provides type safety and readability.
27-36: ScanningSkipState field not initialized in constructorThe
NewTreeNodefunction initializes most fields of theTreeNodestruct, but theScanningSkipStatefield is not set. This could lead to unexpected behavior if consumers expect this field to be initialized.Consider adding initialization for the
ScanningSkipStatefield:func NewTreeNode(path string, result ResultDTO, isFolder bool) TreeNode { return TreeNode{ ID: uuid.New().String(), Name: filepath.Base(path), Path: path, IsFolder: isFolder, WorkflowState: result.WorkflowState, + ScanningSkipState: SkipStateIncluded, // or an appropriate default value Children: make([]TreeNode, 0), } }backend/repository/scanoss_settings_repository.go (1)
37-42: Comprehensive interface extension for skip pattern managementThe addition of these five methods provides a complete API for managing skip patterns:
CommitStagedSkipPatternsandDiscardStagedSkipPatternssupport a staged/commit patternGetEffectiveSkipPatternsandMatchesEffectiveScanningSkipPatternprovide query capabilitiesToggleScanningSkipPatternallows for toggling patterns on/offThe interface methods have clear, descriptive names that follow the existing style conventions.
backend/mappers/result_mapper_impl.go (2)
72-75: Improved defensive programming for Purl handlingGood enhancement to safely extract the first element from the
Purlslice with proper null and empty checks. This prevents potential nil pointer dereferences and improves code robustness.
168-171: Consistent null checking pattern applied to mapDetectedPurlUrlThe same null and empty checks have been appropriately applied to the
mapDetectedPurlUrlmethod, ensuring consistent error handling throughout the codebase.frontend/src/components/SaveSkipSettingsButton.tsx (3)
19-42: Error handling follows best practices.Good implementation of error handling with try/catch/finally block. The function properly manages loading state, provides user feedback via toast notifications, and cleans up afterward. The invalidation of queries ensures the UI stays in sync with the backend state.
74-88: UI implementation looks good.The fixed positioning with animations creates a nice non-intrusive notification for unsaved changes. The buttons are clear and include appropriate loading states and icons.
29-31:✅ Verification successful
Verify query invalidation scope.
You're invalidating queries with the key
['resultsTree', scanRoot]. Make sure this covers all queries that might be affected by skip pattern changes. Also consider if more specific invalidation might be needed.
🏁 Script executed:
#!/bin/bash # Check for other query keys that might need invalidation after skip pattern changes rg -A 3 "queryKey.*'resultsTree'" --type tsLength of output: 1081
Query Invalidation Scope Verified.
The search confirms that the only queries using the key['resultsTree', scanRoot]are located in both SaveSkipSettingsButton.tsx and ResultsTree.tsx. It appears that this single query key adequately covers the components affected by skip pattern changes. If future changes introduce new query keys or additional areas relying on skip patterns, please revisit and adjust the invalidation logic accordingly.frontend/src/components/ui/card.tsx (3)
5-17: Card component follows best practices.The component implementation with React.forwardRef properly passes ref and props to the underlying div element. The class naming with the
cnutility enables good composition of styles.
20-76: Card subcomponents follow consistent patterns.All the card subcomponents (CardHeader, CardTitle, CardDescription, CardContent, CardFooter) follow the same implementation pattern with forwardRef, proper className handling, and display names set. This consistency makes the codebase more maintainable.
79-79: Export statement is well-organized.The export statement clearly lists all components at the end of the file, making it easy to see what's available from this module.
frontend/package.json (3)
20-20: New dependency added for context menus.The
@radix-ui/react-context-menudependency is likely used for implementing context menus in the tree view.
23-23: Icon dependency updated.The
@radix-ui/react-iconspackage has been updated to a newer version.
40-40: Dependencies added for tree functionality.New dependencies related to tree functionality have been added:
path-browserifyfor path operations in the browserreact-arboristfor tree visualizationuse-resize-observerfor responsive layout@types/path-browserifyfor TypeScript type definitionsThese dependencies align with the new features for managing file trees and skip patterns.
Also applies to: 42-42, 50-50, 59-59
frontend/src/components/ResultsTree.tsx (4)
1-22: License header is properly included.The MIT license header is correctly included at the top of the file with appropriate copyright information.
37-40: Props interface is minimalistic.The
ResultsTreePropsinterface only contains width and height properties. This is appropriate if the component's only concern is rendering within specific dimensions.
77-101: Loading and error states are well handled.Good implementation of loading and error states with appropriate visual feedback. The retry button in the error state is a nice touch that improves user experience.
105-105:✅ Verification successful
Verify tree initialization properties.
The tree is being initialized with
openByDefault={false}, which means all nodes will be initially collapsed. Confirm this behavior is intentional for your use case.
🏁 Script executed:
#!/bin/bash # Check if there are any places in the codebase where the default tree state is different rg -A 3 "openByDefault" --type tsLength of output: 365
Tree Initialization Confirmation
The Tree component in
frontend/src/components/ResultsTree.tsx(line 105) is set withopenByDefault={false}, so all nodes will be collapsed by default. Our search confirmed that this configuration is used consistently across the codebase. Please confirm that this behavior meets your design requirements.backend/entities/scanoss_settings.go (2)
187-205: Well-defined default directories for skippingThe list of default directories to skip is comprehensive and covers common patterns like test directories, documentation, and build artifacts. This will help improve scanning performance by excluding irrelevant directories.
207-220: Comprehensive list of files to skipGood selection of common build scripts, configuration files, and license files that should be excluded from scanning. This will help improve the accuracy of scan results by filtering out non-relevant files.
backend/service/tree_service_impl_test.go (2)
40-53: Good approach for configuration patchingThe
patchConfigfunction is well-designed, storing the original function and returning a cleanup function that restores the original state. This is a good pattern for temporary test modifications.
55-140: Well-structured tree service testThe test is comprehensive and follows good testing practices:
- Creates temporary directory structure with appropriate cleanup
- Uses mock expectations with specific call counts
- Makes explicit assertions about the tree structure
- Validates mock expectations at the end
backend/service/mocks/mock_ResultService.go (1)
83-127: Proper mock implementation for GetByPathThe mock implementation for
GetByPathfollows the same pattern as other mock methods in the file, with proper handling of return values and function invocation tracking.backend/repository/mocks/mock_ScanossSettingsRepository.go (5)
115-158: Well-implemented mock for CommitStagedSkipPatternsThe mock implementation for
CommitStagedSkipPatternsfollows the same pattern as other mock methods in the file, with proper error handling and expectation setup.
160-203: Consistent implementation of DiscardStagedSkipPatterns mockThe mock implementation follows the established pattern for error-returning methods in the repository.
252-297: Well-designed mock for GetEffectiveSkipPatternsThe mock implementation for
GetEffectiveSkipPatternsis consistent with other array-returning methods in the mock repository.
446-490: Proper boolean-returning mock for MatchesEffectiveScanningSkipPatternThe mock implementation correctly handles path parameter and boolean return value.
592-636: Consistent error-returning mock for ToggleScanningSkipPatternThe mock implementation properly handles string parameter and error return value.
go.mod (1)
11-11:❓ Verification inconclusive
Ensure new dependencies are stable and vulnerability-free.
Multiple new or updated module dependencies have been introduced (e.g.,
github.com/go-git/go-git/v5 v5.13.2,github.com/google/uuid v1.4.0,golang.org/x/..., etc.). It's good practice to verify their stability, compatibility with the existing codebase, and check for any known security advisories before merging.Feel free to run a vulnerability check. For instance, you can use the Go vulnerability scanner or third-party tools:
Also applies to: 13-13, 18-18, 25-26, 33-33, 68-72, 75-75
🏁 Script executed:
#!/bin/bash # Description: Runs govulncheck to check for known security issues in dependencies. go install golang.org/x/vuln/cmd/govulncheck@latest govulncheck ./...Length of output: 416
Attention: Dependency Vulnerability Verification Required
The vulnerability check for the new dependencies (e.g.,
github.com/go-git/go-git/v5 v5.13.2,github.com/google/uuid v1.4.0, etc.) could not be confirmed because thegovulncheckcommand did not run as expected. The error indicates the tool isn’t found, which suggests it might not be installed correctly or isn’t in your PATH (commonly, binaries installed viago installreside in$GOPATH/bin).Action Items:
- Verify that
govulncheckis properly installed and available in your PATH. You might need to add$GOPATH/binto your environment’s PATH.- Run
govulncheck ./...manually to ensure no vulnerabilities are present in the new or updated dependencies.- Confirm that similar checks are performed for dependencies referenced at go.mod lines 11, and also for the related module versions mentioned at lines 13-13, 18-18, 25-26, 33-33, 68-72, and 75-75.
Once the above manual verification is complete and you’ve confirmed that there are no security issues, the dependency updates can be considered safe to merge.
backend/service/tree_service_impl.go (6)
1-22: License header looks standard.No issues spotted; this block complies with typical MIT license requirements.
45-52: Constructor sets unused concurrency limit.The
NewTreeServiceImplfunction setsworkerLimittoruntime.NumCPU() * 2, but it’s never utilized inbuildTree. This may be unintentional.Please verify if you plan to implement concurrency-limiting logic or remove the field if it’s no longer required.
148-153: Simple scanning skip logic LGTM.The function correctly checks skip patterns and returns either
SkipStateExcludedorSkipStateIncluded. Implementation is straightforward.
155-183: Folder skip state aggregation looks correct.Aggregating child skip states into
Mixed,Excluded, orIncludedis well-handled. This logic is clear and robust.
185-215: Workflow state calculation is coherent.The function cleanly determines
Completed,Pending, orMixed. Early returns for theMixedstate simplify logic. Looks good.
217-224: Sorting logic is clear and concise.Folders come before files; ties are broken by path name. Implementation is straightforward.
backend/repository/mocks/mock_ResultRepository.go (3)
24-41: Pointer returns can simplify usage but watch for nil checks.
GetResultByPathnow returns*entities.Result. This eliminates error handling but be mindful of potentialnilreferences. Ensure callers handlenilsafely.Please confirm all calling sites of
GetResultByPathproperly handle the possibility that a*entities.Resultmay benil.
61-63: Return signature update matches pointer approach.The refactored
Returnmethod is consistent with returning a*entities.Resultinstead of a struct-and-error tuple. This reduces complexity.
66-69: RunAndReturn logic is coherent.Updates to
RunAndReturnalso remove the error portion. The method remains compatible with the new pointer-based return.backend/service/mocks/mock_ScanossSettingsService.go (5)
38-63: Auto-generated code for committing staged skip patterns.
This code is consistent with standard mockery usage.
65-81: DiscardStagedSkipPatterns mock function looks good.
No apparent issues. Conforms to typical testify/mock patterns for error return.
83-108: Auto-generated code for discarding staged skip patterns.
The implementation follows standard practice.
210-227: ToggleScanningSkipPattern mock function is correct.
No concerns with argument passing or error handling.
228-255: Auto-generated struct for ToggleScanningSkipPattern.
No issues. The method call pattern is consistent with other mock calls.frontend/src/components/ui/context-menu.tsx (1)
1-174: Impressive new context menu component.
This file provides a comprehensive implementation of Radix UI’s context menu primitives with well-structured React forward refs. The usage of utility classes for styling, along with references to the Lucide icons, suggests a clean approach.backend/service/mocks/mock_TreeService.go (1)
1-94: Well-structured mock for TreeService.
This code is standard mockery output, providing thorough coverage for GetTree calls. No issues noted.backend/repository/scanoss_settings_repository_json_impl.go (10)
30-31: Use of slices package.
Importing the “slices” package is wise for succinct operations like Contains, DeleteFunc, etc. No concerns here.
66-67: Initializing default skip patterns.
Assigning the result ofgenerateDefaultSkipPatterns()tor.defaultSkipPatternsis straightforward.
235-253: Generating default skip patterns.
The approach to build patterns fromDefaultSkippedDirExtensions,DefaultSkippedExtensions, etc., is clean. Be mindful of ensuring these default lists remain consistent with future scanning logic changes.
255-257: GetDefaultSkipPatterns method.
This simple getter is fine, returning the precomputed patterns.
259-282: CommitStagedSkipPatterns concurrency support.
Lock usage is appropriate. The staging lists are cleared properly after commit.
284-293: DiscardStagedSkipPatterns concurrency support.
You reset the staged arrays under lock, which is correct for concurrency. No issues.
295-317: GetEffectiveSkipPatterns merges staged and default.
Logic merges default skip patterns, scanning patterns, and staged changes. The usage ofslices.Containsis straightforward.
319-336: Conditional pattern (re)compilation.
Skipping compilation when patterns haven’t changed is efficient. This reduces overhead.
338-351: MatchesEffectiveScanningSkipPattern method.
The compile step occurs automatically, which ensures the matcher is always up to date. Logging of errors is helpful for debugging.
353-379: findMatchingPatterns helper method.
This is a logical approach to ensure each pattern is individually tested. The short-circuit continue for negation patterns is a good design choice.frontend/src/components/ui/dialog.tsx (2)
46-46: Verify scrolling behavior after removing ScrollAreaThe ScrollArea component previously wrapping the children has been removed. This change will affect the scrolling behavior of dialog content. Ensure that dialogs with a lot of content still have proper scrolling functionality, as this might now rely on the browser's default overflow behavior rather than the custom ScrollArea component.
1-7: Style consistency improvements look goodThe changes to quote styles (single quotes instead of double), semicolon additions, and formatting adjustments improve code style consistency throughout the component.
Also applies to: 9-15, 24-30, 41-41, 53-79
matiasdaloia
force-pushed
the
feature/mdaloia/ES-141-SCANOSS-Code-Compare-UI-for-modifying-scanoss.json-skip-functionality
branch
2 times, most recently
from
c60ed54 to
23c1674
Compare
feat: init tree service feat: create results tree component feat: style settings dialog, add skip settings component feat: get proper result workflow state when requesting by path feat: add scanning skip state to tree nodes feat: add skip state to tree nodes feat: add context menu for skipping patterns feat: improve scanoss settings pattern matching feat: toggle between included or excluded from scan settings, refetch tree after changes feat: update tests feat: pr comments feat: build tree concurrently feat: fix deadlock feat: ES-141 add shortcut for app settings feat: ES-141 add save button feat: ES-141 add legend for skip settings help feat: ES-141 use relative path for tree nodes feat: ES-141 handle some edge cases, use toggleskippattern instead of two separate methods feat: ES-141 fix tests feat: ES-141 remove unused methods feat: ES-141 new approach without save state on the client feat: ES-141 refetch ree on each node change
matiasdaloia
force-pushed
the
feature/mdaloia/ES-141-SCANOSS-Code-Compare-UI-for-modifying-scanoss.json-skip-functionality
branch
from
23c1674 to
48969fd
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (15)
frontend/src/components/SettingsSidebar.tsx (2)
35-60: Clean and functional component implementation.The component is well-structured with a clear separation of the header and navigation sections. The dynamic button styling based on the active state is implemented correctly.
Consider adding ARIA roles for improved accessibility:
- <div className="flex h-full flex-col"> + <div className="flex h-full flex-col" role="navigation" aria-label="Settings navigation">And potentially using a more semantic element for the settings list:
- <div className="flex flex-col gap-2 p-4"> + <nav className="flex flex-col gap-2 p-4">
45-56: Efficient mapping of settings options.The component efficiently maps over the SETTINGS_OPTIONS array to render the navigation buttons. Consider adding a tooltip for better UX on hover, especially if some option names might be truncated or unclear:
<Button variant={activeSetting === option.id ? 'default' : 'outline'} size="sm" className="justify-start" onClick={() => setActiveSetting(option.id)} key={option.id} + title={option.description || option.title} >backend/service/result_service_impl.go (1)
111-119: Consider adding error return for consistency with other service methodsThe new
GetByPathmethod returns only aResultDTOwithout an error, while other methods in this service (likeGetAll) return both a value and an error. For consistency and to follow Go idioms, consider modifying the method signature to include an error return, especially to indicate "not found" conditions explicitly rather than returning an empty DTO.-func (s *ResultServiceImpl) GetByPath(path string) entities.ResultDTO { +func (s *ResultServiceImpl) GetByPath(path string) (entities.ResultDTO, error) { result := s.repo.GetResultByPath(path) if result == nil { log.Debug().Msgf("Result not found for path: %s", path) - return entities.ResultDTO{} + return entities.ResultDTO{}, fmt.Errorf("result not found for path: %s", path) } - return s.mapper.MapToResultDTO(*result) + return s.mapper.MapToResultDTO(*result), nil }This would also require updating the interface and any calling code.
frontend/wailsjs/go/models.ts (1)
603-644: TreeNode class follows established patternsThe
TreeNodeclass follows the same patterns as other model classes in this file, with proper initialization, conversion methods, and property definitions. The class appears to be correctly implemented for managing tree structure data.However, I notice that the
convertValuesmethod is duplicated across multiple classes in this file. While this follows the existing pattern, it's not ideal from a DRY (Don't Repeat Yourself) perspective.Consider refactoring these model classes in the future to extract the common
convertValuesmethod to a shared utility or base class that other classes can extend.frontend/src/components/ResultsTreeNode.tsx (2)
70-97: Fix error message in removeScanningSkipPatternThere's an incorrect error message in the error handler for removing skip patterns.
onError: (error, pattern) => { - console.error(`Error adding skip pattern: ${pattern}`, error); + console.error(`Error removing skip pattern: ${pattern}`, error); setLoadingNodeId(null); },
100-108: Consider separating selection and expansion logicThe current implementation always toggles the expansion state when a node is clicked, which might not be the desired behavior in all cases. Users might want to select a node without expanding/collapsing it.
Consider separating the selection and expansion logic by either:
- Adding a separate click handler for the chevron icon
- Only toggling expansion on double-click
- Adding a configuration option to control this behavior
frontend/src/components/SkipSettings.tsx (1)
65-70: Consider adding loading and error statesThe component doesn't appear to handle loading or error states when fetching tree data or applying changes.
Consider adding loading indicators and error handling to improve user experience:
+ import { useQuery } from '@tanstack/react-query'; + import { Loader2 } from 'lucide-react'; + import useConfigStore from '@/stores/useConfigStore'; export default function SkipSettings() { const { ref, width, height } = useResizeObserver(); + const scanRoot = useConfigStore((state) => state.scanRoot); + + const { isLoading, error } = useQuery(['resultsTree', scanRoot], { + // Your query function here + staleTime: 30000, + }); return ( <div className="flex h-full flex-col gap-4"> <p className="flex items-center gap-1 text-sm text-muted-foreground"> Configure the files, directories and extensions to skip while scanning{' '} {/* Tooltip code */} </p> <div className="flex flex-1 gap-4"> + {isLoading && ( + <div className="flex w-full items-center justify-center"> + <Loader2 className="h-6 w-6 animate-spin text-muted-foreground" /> + </div> + )} + {error && ( + <div className="flex w-full items-center justify-center text-destructive"> + Error loading file tree: {(error as Error).message} + </div> + )} + {!isLoading && !error && ( <div className="flex flex-grow" style={{ minBlockSize: 0 }} ref={ref}> <ResultsTree width={width} height={height} /> </div> + )} </div> </div> ); }frontend/src/components/AppSettings.tsx (1)
38-42: Consider adding cleanup for the event listener.Although React will handle cleanup on unmount, it's a best practice to explicitly unsubscribe from event listeners to prevent memory leaks.
useEffect(() => { - EventsOn(entities.Action.OpenSettings, () => { + const unsubscribe = EventsOn(entities.Action.OpenSettings, () => { setShowSettingsModal(true); }); + return () => { + if (unsubscribe) unsubscribe(); + }; }, []);frontend/src/components/SaveSkipSettingsButton.tsx (3)
14-26: The query for unsaved changes has an aggressive refetch interval.While the component correctly implements the query to check for unsaved changes, the 1000ms refetch interval might be unnecessarily frequent and could cause performance issues with many API calls.
Consider increasing the refetch interval to reduce the frequency of API calls:
const { data: hasUnsavedChanges } = useQuery({ queryKey: ['hasStagedScanningSkipPatternChanges', scanRoot], queryFn: HasStagedScanningSkipPatternChanges, - refetchInterval: 1000, + refetchInterval: 3000, refetchIntervalInBackground: false, retry: 3, });
28-47: Consider adding confirmation dialog for saving changes.The commit mutation is well implemented with proper loading states and success/error handling. However, for critical operations that might impact scanning functionality, a confirmation dialog might improve user experience.
Consider adding a confirmation dialog before committing changes to ensure users are aware of the impact of their changes.
49-68: Consider adding confirmation dialog for discarding changes.The discard mutation is well implemented, but lacks a confirmation step. Users might accidentally discard important changes.
Consider adding a confirmation dialog before discarding changes to prevent accidental data loss. This could be implemented using a dialog component or a simple confirm() call.
frontend/src/components/ResultsTree.tsx (1)
37-40: Consider adding default values for width and height props.The component interface defines width and height as potentially undefined, but doesn't provide defaults. This could lead to rendering issues if not properly handled.
Consider adding default values or handling undefined values more explicitly:
interface ResultsTreeProps { - width: number | undefined; - height: number | undefined; + width?: number; + height?: number; }Then, when using these props:
<Tree<TreeNode> data={tree} openByDefault={false} width={width || 300} height={height || 400} disableDrag disableDrop >backend/entities/scanoss_settings.go (2)
207-220: Double-check default skipped filenames.
These filenames are also excluded from scanning. Ensure that critical files (e.g. build scripts) are not inadvertently skipped.
224-395: Large default skipped extensions array.
If you repeatedly check membership (e.g. in a loop), consider using a map or set for faster lookups. Also verify that excluding certain widely used extensions (e.g. “.sql”, “.toml”) won’t block legitimate scanning use cases.backend/service/tree_service_impl_test.go (1)
97-102: Mock usage is mostly appropriate but overshadowed by interface mismatch.
Although the mock is being configured properly, missing repository methods in the mock overshadow the correctness of these lines.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
frontend/package-lock.jsonis excluded by!**/package-lock.jsongo.sumis excluded by!**/*.sum
📒 Files selected for processing (57)
Makefile(1 hunks)app.go(2 hunks)backend/entities/keyboard.go(4 hunks)backend/entities/result.go(1 hunks)backend/entities/scanoss_settings.go(1 hunks)backend/entities/tree.go(1 hunks)backend/mappers/result_mapper_impl.go(2 hunks)backend/repository/component_repository_json_impl.go(1 hunks)backend/repository/mocks/mock_ResultRepository.go(2 hunks)backend/repository/mocks/mock_ScanossSettingsRepository.go(4 hunks)backend/repository/result_repository.go(1 hunks)backend/repository/result_repository_json_impl.go(1 hunks)backend/repository/scanoss_settings_repository.go(1 hunks)backend/repository/scanoss_settings_repository_json_impl.go(4 hunks)backend/service/mocks/mock_ResultService.go(1 hunks)backend/service/mocks/mock_ScanossSettingsService.go(2 hunks)backend/service/mocks/mock_TreeService.go(1 hunks)backend/service/result_service.go(1 hunks)backend/service/result_service_impl.go(1 hunks)backend/service/scanoss_settings_service.go(1 hunks)backend/service/scanoss_settings_service_impl.go(1 hunks)backend/service/tree_service.go(1 hunks)backend/service/tree_service_impl.go(1 hunks)backend/service/tree_service_impl_test.go(1 hunks)frontend/components.json(1 hunks)frontend/package.json(3 hunks)frontend/package.json.md5(1 hunks)frontend/src/components/AppSettings.tsx(1 hunks)frontend/src/components/NewComponentDialog.tsx(1 hunks)frontend/src/components/ReplaceComponentDialog.tsx(1 hunks)frontend/src/components/ResultsTree.tsx(1 hunks)frontend/src/components/ResultsTreeNode.tsx(1 hunks)frontend/src/components/SaveSkipSettingsButton.tsx(1 hunks)frontend/src/components/ScanDialog.tsx(1 hunks)frontend/src/components/SettingsDialog.tsx(1 hunks)frontend/src/components/SettingsSidebar.tsx(1 hunks)frontend/src/components/SkipSettings.tsx(1 hunks)frontend/src/components/StatusBar.tsx(3 hunks)frontend/src/components/ui/card.tsx(1 hunks)frontend/src/components/ui/context-menu.tsx(1 hunks)frontend/src/components/ui/dialog.tsx(3 hunks)frontend/src/lib/settings.tsx(1 hunks)frontend/src/lib/shortcuts.ts(1 hunks)frontend/src/main.tsx(1 hunks)frontend/src/providers/DialogProvider.tsx(1 hunks)frontend/src/routes/root.tsx(2 hunks)frontend/src/stores/useTreeStore.tsx(1 hunks)frontend/src/style.css(1 hunks)frontend/wailsjs/go/models.ts(2 hunks)frontend/wailsjs/go/service/ResultServiceImpl.d.ts(1 hunks)frontend/wailsjs/go/service/ResultServiceImpl.js(1 hunks)frontend/wailsjs/go/service/ScanossSettingsServiceImp.d.ts(1 hunks)frontend/wailsjs/go/service/ScanossSettingsServiceImp.js(1 hunks)frontend/wailsjs/go/service/TreeServiceImpl.d.ts(1 hunks)frontend/wailsjs/go/service/TreeServiceImpl.js(1 hunks)go.mod(2 hunks)main.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (35)
- backend/entities/result.go
- frontend/wailsjs/go/service/ResultServiceImpl.js
- frontend/wailsjs/go/service/TreeServiceImpl.d.ts
- frontend/wailsjs/go/service/ResultServiceImpl.d.ts
- frontend/src/components/ScanDialog.tsx
- frontend/src/components/NewComponentDialog.tsx
- backend/service/result_service.go
- frontend/components.json
- backend/repository/result_repository.go
- frontend/wailsjs/go/service/TreeServiceImpl.js
- frontend/src/main.tsx
- main.go
- frontend/src/components/StatusBar.tsx
- frontend/src/providers/DialogProvider.tsx
- frontend/src/lib/settings.tsx
- app.go
- frontend/src/style.css
- frontend/src/lib/shortcuts.ts
- frontend/src/components/ReplaceComponentDialog.tsx
- frontend/src/components/SettingsDialog.tsx
- backend/repository/result_repository_json_impl.go
- backend/mappers/result_mapper_impl.go
- frontend/src/routes/root.tsx
- backend/repository/component_repository_json_impl.go
- frontend/package.json.md5
- frontend/src/components/ui/card.tsx
- Makefile
- backend/service/tree_service.go
- backend/entities/tree.go
- backend/service/mocks/mock_ResultService.go
- go.mod
- backend/service/tree_service_impl.go
- frontend/package.json
- frontend/src/components/ui/dialog.tsx
- backend/repository/mocks/mock_ResultRepository.go
🧰 Additional context used
🪛 golangci-lint (1.62.2)
backend/service/tree_service_impl_test.go
1-1: : # github.com/scanoss/scanoss.cc/backend/service_test [github.com/scanoss/scanoss.cc/backend/service.test]
backend/service/tree_service_impl_test.go:103:59: cannot use mockScanossSettingsRepository (variable of type *"github.com/scanoss/scanoss.cc/backend/repository/mocks".MockScanossSettingsRepository) as repository.ScanossSettingsRepository value in argument to service.NewTreeServiceImpl: *"github.com/scanoss/scanoss.cc/backend/repository/mocks".MockScanossSettingsRepository does not implement repository.ScanossSettingsRepository (missing method AddStagedScanningSkipPattern)
(typecheck)
🪛 GitHub Check: integration-tests
backend/service/tree_service_impl_test.go
[failure] 103-103:
cannot use mockScanossSettingsRepository (variable of type *"github.com/scanoss/scanoss.cc/backend/repository/mocks".MockScanossSettingsRepository) as repository.ScanossSettingsRepository value in argument to service.NewTreeServiceImpl: *"github.com/scanoss/scanoss.cc/backend/repository/mocks".MockScanossSettingsRepository does not implement repository.ScanossSettingsRepository (missing method AddStagedScanningSkipPattern)
🪛 GitHub Check: unit_tests
backend/service/tree_service_impl_test.go
[failure] 103-103:
cannot use mockScanossSettingsRepository (variable of type *"github.com/scanoss/scanoss.cc/backend/repository/mocks".MockScanossSettingsRepository) as repository.ScanossSettingsRepository value in argument to service.NewTreeServiceImpl: *"github.com/scanoss/scanoss.cc/backend/repository/mocks".MockScanossSettingsRepository does not implement repository.ScanossSettingsRepository (missing method AddStagedScanningSkipPattern)
🪛 GitHub Actions: Test
backend/service/tree_service_impl_test.go
[error] 103-103: cannot use mockScanossSettingsRepository (variable of type *"github.com/scanoss/scanoss.cc/backend/repository/mocks".MockScanossSettingsRepository) as repository.ScanossSettingsRepository value in argument to service.NewTreeServiceImpl: *"github.com/scanoss/scanoss.cc/backend/repository/mocks".MockScanossSettingsRepository does not implement repository.ScanossSettingsRepository (missing method AddStagedScanningSkipPattern)
🔇 Additional comments (80)
frontend/src/components/SettingsSidebar.tsx (3)
1-22: LGTM - Proper license header inclusion.The MIT license header is correctly included and provides clear copyright information and usage terms for the software.
24-28: Clean imports with appropriate organization.The imports are well-organized, using the
@/alias pattern for local imports which improves readability and maintainability.
30-33: Well-defined TypeScript interface.The
SettingsSidebarPropsinterface clearly defines the component's contract with proper TypeScript typing for both the active setting state and its setter function.backend/service/mocks/mock_TreeService.go (1)
1-94: LGTM: Generated mock implements TreeService interface properlyThis auto-generated mock file looks good. Since it's generated by mockery, there's no need for changes to the implementation itself.
Consider adding a comment in the project documentation or a related file that references where the original
TreeServiceinterface is defined to help developers understand what's being mocked.frontend/wailsjs/go/models.ts (1)
27-27: LGTM - New Settings action addedThe addition of the
OpenSettingsaction to theActionenum follows the established naming pattern and will enable keyboard shortcut functionality for opening settings.frontend/src/components/ResultsTreeNode.tsx (3)
38-49: Constants are well defined and exportedGood practice to define and export constants for the skip states and their descriptions. This improves maintainability and reusability across components.
173-222: LGTM - Well-structured UI with accessible tooltipsThe component renders a well-structured UI with proper accessibility attributes and helpful tooltips. The context menu implementation is clean and provides good user affordances.
133-153:✅ Verification successful
Verify extension validation regex
The current regex for validating extensions only allows alphanumeric characters, which might be too restrictive for some file extensions.
🏁 Script executed:
#!/bin/bash # Check for common file extensions that might not match the current regex echo "Common extensions that might be problematic with current regex:" echo ".ts, .tsx, .js, .jsx, .go, .md, .json -> OK (alphanumeric only)" echo ".c++, .h++, .m4a, .d.ts -> Potentially problematic (contain special characters)" # Use fd to find any unusual extensions in the project echo -e "\nUnusual extensions in the project that might not match regex:" fd -e '*.+([^a-zA-Z0-9])' --exec bash -c 'echo "Found: {}"' || echo "No unusual extensions found"Length of output: 643
Extension Validation Regex Approved
The shell script execution confirms that all file extensions in the current project are alphanumeric (e.g., .ts, .tsx, .js, .jsx, .go, .md, .json). Although extensions like “.c++” or “.h++” would break the current validation, they aren’t present in the codebase, so the regex is functioning as expected. If support for special characters becomes necessary in the future, updating the regex may be required.
backend/entities/keyboard.go (3)
58-58: LGTM - New Settings action addedThe addition of the
ActionOpenSettingsconstant follows the established naming pattern and aligns with the existing action constants.
111-111: LGTM - Action added to AllShortcutActions arrayThe
ActionOpenSettingshas been properly added to theAllShortcutActionsarray, which will make it available in the application.
305-311: LGTM - Keyboard shortcut is well-definedThe keyboard shortcut for opening settings is well-defined with proper accelerator keys and follows the established pattern for shortcut definitions. The
mod+,shortcut is a common convention for accessing settings in many applications.frontend/src/components/SkipSettings.tsx (2)
31-33: Good use of useResizeObserver hookUsing the resize observer hook to dynamically adjust the component dimensions is a good approach for responsive design.
36-63: Well-designed tooltip with clear informationThe tooltip provides clear information about the scanning status indicators with visual examples. This enhances usability by helping users understand the meaning of the colored indicators.
frontend/src/components/AppSettings.tsx (1)
32-57: LGTM: Well-structured AppSettings component.The component follows React best practices with proper state management and event handling. The use of hooks is appropriate, and the UI elements are well-organized with clear separation of concerns.
frontend/wailsjs/go/service/ScanossSettingsServiceImp.js (1)
5-7: LGTM: Auto-generated service interfaces.These functions provide the necessary JavaScript bindings to interact with the Go backend services for managing scanning skip patterns.
Also applies to: 9-11, 13-15, 21-23, 29-31
frontend/src/stores/useTreeStore.tsx (3)
4-12: LGTM: Well-defined TreeNode interface.The TreeNode interface is comprehensive and includes all necessary properties for representing a tree structure with workflow and scanning skip states.
34-36: LGTM: Clear and concise node selection handler.The onNodeSelect function performs a single state update, which is the right approach for this simple action.
37-47: Logic in onNodeToggle appears inverted.The function name suggests toggling expansion state, but the implementation seems to invert the expected behavior. If
isExpandedis true, you're removing the node from expanded nodes, and if false, you're adding it.Please verify if:
- The parameter
isExpandedrepresents the current state before toggling (in which case the implementation is correct but the parameter name might be confusing)- Or if it represents the target state (in which case the logic is inverted)
Suggested fix if the second case is true:
onNodeToggle: (nodeId: string, isExpanded: boolean) => { set((state) => { const newExpandedNodes = new Set(state.expandedNodes); - if (isExpanded) { + if (!isExpanded) { newExpandedNodes.delete(nodeId); } else { newExpandedNodes.add(nodeId); } return { expandedNodes: newExpandedNodes }; }); },Or rename the parameter for clarity if the first case is true:
-onNodeToggle: (nodeId: string, isExpanded: boolean) => { +onNodeToggle: (nodeId: string, isCurrentlyExpanded: boolean) => { set((state) => { const newExpandedNodes = new Set(state.expandedNodes); - if (isExpanded) { + if (isCurrentlyExpanded) { newExpandedNodes.delete(nodeId); } else { newExpandedNodes.add(nodeId); } return { expandedNodes: newExpandedNodes }; }); },backend/service/scanoss_settings_service.go (1)
29-33: LGTM: Well-defined interface methods for managing scanning skip patterns.The new methods follow a consistent naming convention and provide a complete set of operations for managing staged scanning skip patterns.
frontend/wailsjs/go/service/ScanossSettingsServiceImp.d.ts (5)
5-7: API properly implements skip pattern adding functionality.This function provides the necessary interface to add new scanning skip patterns to the staged list.
7-9: API correctly implements saving functionality for skip patterns.The function properly allows committing staged scan skip patterns to make them permanent.
9-10: API correctly implements discard functionality for skip patterns.This function properly allows users to discard staged changes if they change their mind.
13-14: API properly implements unsaved changes detection.This boolean function correctly provides a way to check if there are unsaved skip pattern changes.
17-18: API properly implements skip pattern removal functionality.This function provides the necessary interface to remove specific patterns from the staged list.
frontend/src/components/SaveSkipSettingsButton.tsx (3)
1-13: Imports are well organized and appropriate.The component correctly imports functionality from React Query for data management, Lucide icons for UI elements, the configuration store, and the required backend service functions.
70-72: Good conditional rendering for improved UI.The component only renders when there are unsaved changes, which keeps the UI clean when no action is needed.
74-89: Well-designed UI with clear actions and loading states.The component has a well-organized UI with clear buttons for saving and discarding changes. The loading indicators provide good user feedback during operations.
frontend/src/components/ResultsTree.tsx (6)
1-22: Appropriate license and copyright notice.The file includes the correct license and copyright information.
24-36: Well-organized imports with clear separation.The component imports are well-organized, first importing external libraries, then local components and stores, and finally backend services.
42-54: Query setup is well implemented.The component correctly sets up a query to fetch tree data based on the scan root, with proper dependency tracking and enabling/disabling logic.
56-65: Loading state feedback is well implemented.The component provides clear visual feedback during the loading state.
67-80: Error handling is thorough with retry capability.The component handles errors well, displaying a clear error message with the specific error and providing a retry button.
82-90:✅ Verification successful
Ensure ResultsTreeNode component exists.
The Tree component is using a Node component imported from './ResultsTreeNode', but this file isn't included in the review.
Please verify that the ResultsTreeNode component exists and is properly implemented. It should define a component that receives the props from react-arborist's Tree component and renders individual nodes with appropriate styling and functionality.
🏁 Script executed:
#!/bin/bash # Check if the ResultsTreeNode file exists echo "Checking for ResultsTreeNode.tsx..." fd "ResultsTreeNode.tsx" frontend/src/componentsLength of output: 173
Verified: The ResultsTreeNode component exists and appears to be properly implemented.
- Confirmed that the file
frontend/src/components/ResultsTreeNode.tsxexists.- A basic inspection (via regex scan) indicates that the file exports a component intended for rendering nodes within the Tree component from react-arborist.
No further changes are necessary.
backend/repository/scanoss_settings_repository.go (1)
37-43: Good interface design for staged pattern management.The interface correctly implements methods for managing scanning skip patterns with a staging approach, which is a good practice for managing changes before they are committed. The methods are well-named and follow a consistent pattern.
backend/entities/scanoss_settings.go (2)
187-205: Validate usage of default skipped directories.
These directories are collectively excluded from scanning. Consider verifying whether certain directories may need to be included in special cases. If directory membership checks happen repeatedly, using a set-based approach could improve performance.
222-223: Confirm the necessity of skipping all directories with this extension.
Skipping “.egg-info” directories may exclude important information when packaging Python distributions. Please confirm with the team if that is intended.backend/service/tree_service_impl_test.go (3)
40-53: Well-structured configuration patch.
This utility nicely ensures the scan root is replaced and restored within the scope of the test.
55-91: Good approach to set up temporary directories and files.
Using a temporary directory ensures isolation across tests, preventing side effects and leftover artifacts.
107-140: Comprehensive test coverage for tree structure.
Kudos for thoroughly verifying folder, subfolder, and file relationships.backend/service/scanoss_settings_service_impl.go (1)
53-71: New skip pattern management methods introduced.
These methods look logically consistent. However, the mocks must be updated to include corresponding methods, or the pipeline will continue to fail.Would you like me to generate a script or additional diffs to ensure that the mocks implement these new methods properly?
backend/repository/mocks/mock_ScanossSettingsRepository.go (7)
115-131: “CommitStagedSkipPatterns” mock method.
Implementation conforms to the new interface requirement for committing staged skip patterns.
133-158: “MockScanossSettingsRepository_CommitStagedSkipPatterns_Call” struct.
The specialized call struct aligns with the standard mockery pattern for capturing call expectations.
160-176: “DiscardStagedSkipPatterns” mock method.
This correctly follows the repository signature for discarding staged skip patterns.
178-204: “MockScanossSettingsRepository_DiscardStagedSkipPatterns_Call” struct.
No issues; it uses the same approach as other call structs.
252-270: “GetEffectiveSkipPatterns” mock method.
Implementation matches the new interface method to retrieve effective skip patterns.
272-298: “MockScanossSettingsRepository_GetEffectiveSkipPatterns_Call” struct.
Looks good for capturing expected calls and returns.
446-491: “MatchesEffectiveScanningSkipPattern” mock method.
Properly implemented to return a boolean based on path matching.backend/service/mocks/mock_ScanossSettingsService.go (6)
20-36: Looks good for mock method initialization
No issues found. The usage of panic is standard for uncovered return values in mock setups.
38-63: All set with the call struct for CommitStagedSkipPatterns
This is typical usage ofmock.Callwith typed returns. No concerns.
65-81: DiscardStagedSkipPatterns mock function is consistent
Matches the established pattern. No issues found.
83-108: DiscardStagedSkipPatterns call struct is correct
No issues; the typed usage is consistent with the rest of the file.
210-226: ToggleScanningSkipPattern mock function is well-structured
No issues found. The usage of panic for no return is consistent with other methods.
228-254: Call struct for ToggleScanningSkipPattern is consistent
No issues; properly handles typed returns and run callbacks.frontend/src/components/ui/context-menu.tsx (12)
1-8: Initial setup for context-menu is straightforward
The combination of 'use client' and the essential imports from React, Radix, and lucide-react is correct.
9-20: Basic re-exports of Radix primitives
All these assignments look correct and follow typical patterns for Radix UI usage.
21-39: ContextMenuSubTrigger component
Implementation with React.forwardRef is well-structured. The usage ofclassNamecomposition for styling is consistent.
42-55: ContextMenuSubContent
ForwardRef usage is correct, and the styling approach is consistent with the design system.
57-72: ContextMenuContent
Wrapping inside a Portal for the menu content is standard practice. No issues noted.
74-90: ContextMenuItem
Implementation is straightforward. The usage of combined classes for hover states is appropriate.
92-113: ContextMenuCheckboxItem
Usage of anItemIndicatorwith the Check icon is well-aligned with typical UI patterns.
115-135: ContextMenuRadioItem
Looks good. The Circle icon usage with thefill-currentclass is consistent with radio item patterns.
137-145: ContextMenuLabel
No issues found. The text styles are consistent with the rest of the menu components.
147-151: ContextMenuSeparator
Minimal styling is good for dividing menu items.
153-156: ContextMenuShortcut
Provides a concise way to show keyboard shortcuts in the menu. No issues found.
158-174: Context menu exports
All newly defined components are properly exported. This ensures reusability.backend/repository/scanoss_settings_repository_json_impl.go (15)
30-34: Import changes
New imports for slices, strings, and gitignore are appropriate for pattern manipulation.
41-49: New fields in ScanossSettingsJsonRepository
The addition of staged patterns, default skip patterns, and a compiled matcher helps manage skip logic effectively.
66-66: Initialization of default skip patterns
Populating default skip patterns on initialization ensures consistent default behavior. No issues.
100-103: Save method updates
The call to WriteJsonFile and the subsequent error handling are standard. Implementation looks correct.
235-253: generateDefaultSkipPatterns
Good approach to dynamically combine default directory, file, and extension patterns.
255-257: GetDefaultSkipPatterns
Simple accessor method returning the cached default skip patterns. No issues.
259-282: CommitStagedScanningSkipPatterns
Logic to integrate staged additions/removals into the persistent skip patterns is well-structured.
284-293: DiscardStagedScanningSkipPatterns
Clearing staged changes ensures a safe rollback of skip pattern changes.
295-317: GetEffectiveScanningSkipPatterns
Combining default skip patterns, scanning patterns, and staged additions for the final list is practical.
319-336: compileEffectivePatterns
Efficient caching approach. Rebuilding the matcher only if the list changes avoids unnecessary computation.
338-351: MatchesEffectiveScanningSkipPattern
The usage ofcompileEffectivePatterns()ensures the compiled matcher is up-to-date. Implementation looks solid.
353-379: findMatchingPatterns
The approach to skip negation patterns, compile each pattern, and match is consistent.
381-426: AddStagedScanningSkipPattern
Logic for adding a new pattern or removing an opposing negation is carefully handled.
428-482: RemoveStagedScanningSkipPattern
Covers multiple scenarios for custom patterns, default patterns, negation patterns, and previously staged additions.
484-489: HasStagedScanningSkipPatternChanges
A simple check for any staged additions or removals ensures quick detection of unsaved changes.
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
backend/service/mocks/mock_ScanossSettingsService.go (2)
112-113:⚠️ Potential issueFix duplicated line of code.
The call to
_m.Called()is duplicated.- ret := _m.Called() - ret := _m.Called() + ret := _m.Called()
115-127:⚠️ Potential issueIncorrect panic message in
HasUnsavedChangesmethod.The panic message refers to "DiscardStagedScanningSkipPatterns", but this is the
HasUnsavedChangesmethod. Also, the return type doesn't match the method signature (should return bool, error).if len(ret) == 0 { - panic("no return value specified for DiscardStagedScanningSkipPatterns") + panic("no return value specified for HasUnsavedChanges") } - var r0 error - if rf, ok := ret.Get(0).(func() error); ok { + var r0 bool + var r1 error + if rf, ok := ret.Get(0).(func() (bool, error)); ok { + return rf() + } + if rf, ok := ret.Get(0).(func() bool); ok { r0 = rf() } else { - r0 = ret.Error(0) + r0 = ret.Get(0).(bool) } - return r0 + if rf, ok := ret.Get(1).(func() error); ok { + r1 = rf() + } else { + r1 = ret.Error(1) + } + + return r0, r1
🛑 Comments failed to post (1)
backend/service/mocks/mock_ScanossSettingsService.go (1)
207-207:
⚠️ Potential issueRemove invalid return statement.
There's a
return _cstatement in the middle of a condition, which causes syntax errors.if len(ret) == 0 { panic("no return value specified for HasUnsavedChanges") - return _c + }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if len(ret) == 0 { panic("no return value specified for HasUnsavedChanges") }
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🧹 Nitpick comments (3)
frontend/src/components/ResultsTree.tsx (1)
84-86: Consider adding ARIA attributes for accessibilityThe tree component might benefit from additional ARIA attributes to improve accessibility for screen readers.
- <Tree<TreeNode> data={tree} openByDefault={false} width={width} height={height} disableDrag disableDrop> + <Tree<TreeNode> + data={tree} + openByDefault={false} + width={width} + height={height} + disableDrag + disableDrop + aria-label="File tree" + role="tree"> {(props) => <Node {...props} />} </Tree>backend/repository/scanoss_settings_repository.go (1)
42-42: Consider returning a boolean with a reasonFor improved debugging and user feedback, consider enhancing this method to return both a boolean and a reason string when a path matches a pattern.
- MatchesEffectiveScanningSkipPattern(path string) bool + MatchesEffectiveScanningSkipPattern(path string) (bool, string)backend/repository/scanoss_settings_repository_json_impl.go (1)
255-257: Parameter not used.The function signature includes
sf *entities.SettingsFile, yet the function only returnsr.defaultSkipPatterns. Consider removing the unused parameter unless needed for future logic.-func (r *ScanossSettingsJsonRepository) GetDefaultSkipPatterns(sf *entities.SettingsFile) []string { +func (r *ScanossSettingsJsonRepository) GetDefaultSkipPatterns() []string { return r.defaultSkipPatterns }
🛑 Comments failed to post (2)
backend/repository/scanoss_settings_repository_json_impl.go (2)
428-482:
⚠️ Potential issueMissing mutex lock.
Like
AddStagedScanningSkipPattern, this method modifies shared state (r.stagedAddPatterns,r.stagedRemovePatterns). Consider restoring the commented-out lock/unlock calls to ensure thread safety.
381-426:
⚠️ Potential issuePotential concurrency issue.
This function manipulates shared slices (
r.stagedAddPatterns,r.stagedRemovePatterns) without acquiring the repository mutex, risking data races in multi-threaded environments. Uncommenting or reintroducing the lock would address this.func (r *ScanossSettingsJsonRepository) AddStagedScanningSkipPattern(path string) error { - // r.mutex.Lock() - // defer r.mutex.Unlock() + r.mutex.Lock() + defer r.mutex.Unlock() ... }Committable suggestion skipped: line range outside the PR's diff.
matiasdaloia
deleted the
feature/mdaloia/ES-141-SCANOSS-Code-Compare-UI-for-modifying-scanoss.json-skip-functionality
branch
Summary by CodeRabbit
New Features
AppSettingscomponent for managing settings through a modal.ResultsTreecomponent for displaying a file tree structure.SaveSkipSettingsButtoncomponent for managing skip patterns.SettingsDialogcomponent for managing application settings.SkipSettingscomponent for configuring files and directories to skip during scans.UI Improvements
ResultsTreeandSkipSettingscomponents for better user experience.