feat: Enhance scan with options UX/UI #86
Conversation
matiasdaloia
force-pushed
the
feature/mdaloia/ES-140-SCANOSS-Code-Compare-Scan-with-options-enhancements
branch
from
d91e7a1 to
8449e53
Compare
… if does not exist, enhance ux/ui for scanning
WalkthroughThe changes span both backend and frontend components. A new ignore rule (“.cursor”) has been added and the changelog is updated to version 0.6.0. The configuration management is refactored: a singleton config instance is now embedded in the App struct and accessed through getter/setter methods. Several repository methods, command-line argument handling, and UI components have been modified to use these new configuration methods. In addition, new features such as scan argument definitions and improved scan dialog workflows have been introduced alongside enhanced thread-safety and listener notifications in the configuration module. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Component
participant Config as Config Instance
participant L as Listener
C->>Config: Call SetScanRoot(path)
activate Config
Config-->>Config: Acquire write lock
Config-->>Config: Update scanRoot value
Config->>Config: notifyListeners()
Config-->>L: Notify registered listeners
deactivate Config
sequenceDiagram
participant User as User
participant SD as ScanDialog
participant SS as ScanService
participant Ext as External Process
User->>SD: Initiate scan action
SD->>SD: maybeCreateOutputFolder(args)
SD->>SD: Build scan arguments (cmdArgs)
SD->>SS: executeScanWithPipes(cmdArgs)
SS->>Ext: Run scan command
Ext-->>SS: Return scan results
SS->>SD: Return scan results to UI
Poem
✨ 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: 3
🧹 Nitpick comments (13)
internal/config/config.go (1)
158-158: SetScanRoot overrides
ResultFilePathandScanSettingsFilePathget overridden with defaults, which might surprise users if they intended custom locations.Also applies to: 160-163
frontend/src/modules/results/stores/useResultsStore.ts (1)
89-115: Removed error handling
Eliminating the try-catch logic andisLoading/errorstates might reduce visibility on request failures. Consider restoring or replacing them to maintain robust UX.frontend/src/components/ScanDialog.tsx (3)
95-114: Enhanced scan argument handling
BuildingcmdArgsconditionally is a neat approach, but consider logging final args for debugging.
159-190: Initialization effect
Loads default arguments and sets up initial directory-based paths. Error handling is present, but you might consider more UI feedback if initialization fails.
203-207: Passive success fadeout
setTimeoutapproach is straightforward, though using a cancellable effect might be safer if the component unmounts early.backend/repository/result_repository_json_impl.go (1)
49-55: Consider idiomatic Go error handling.The current error handling could be simplified to be more idiomatic Go.
resultByte, err := r.fr.ReadFile(resultFilePath) if err != nil { if os.IsNotExist(err) { return []entities.Result{}, nil } - log.Error().Err(err).Msg("Error reading result file") - return []entities.Result{}, entities.ErrReadingResultFile + return nil, fmt.Errorf("reading result file: %w", err) }backend/entities/scan.go (2)
32-40: LGTM! Well-structured argument definition.The
ScanArgDefstruct provides a clean and maintainable way to define command-line arguments with rich metadata.Consider using generics to make the
Defaultfield type-safe:-type ScanArgDef struct { +type ScanArgDef[T any] struct { Name string Shorthand string - Default interface{} + Default T Usage string Type string IsCore bool IsFileSelector bool }
58-64: Consider consolidating related dependency flags.The dependency-related flags (
dependencies,dependencies-only,dep-scope,dep-scope-inc,dep-scope-exc) have overlapping functionality. Consider consolidating them into a more structured approach.Example consolidation:
-{"dependencies", "D", false, "Add Dependency scanning", "bool", false, false}, -{"dependencies-only", "", false, "Run Dependency scanning only", "bool", false, false}, -{"dep-scope", "", "", "Filter dependencies by scope - default all (options: dev/prod)", "string", false, false}, -{"dep-scope-inc", "", "", "Include dependencies with declared scopes", "string", false, false}, -{"dep-scope-exc", "", "", "Exclude dependencies with declared scopes", "string", false, false}, +{"dep-mode", "D", "none", "Dependency scanning mode (none/include/only)", "string", false, false}, +{"dep-scope", "", "all", "Dependency scope configuration (all/dev/prod/custom)", "string", false, false}, +{"dep-scope-filter", "", "", "Custom scope filter (when dep-scope=custom)", "string", false, false},cmd/scan.go (1)
65-88: LGTM! Clean argument processing implementation.The refactoring to use centralized argument definitions improves maintainability while maintaining proper type handling.
Consider enhancing the error message in the stringSlice case to be more descriptive:
- return fmt.Errorf("an error occurred with argument %s: %w", arg.Name, err) + return fmt.Errorf("failed to parse comma-separated values for argument '%s': %w", arg.Name, err)frontend/src/components/ScanOption.tsx (1)
105-107: Remove redundant case clause.The
case 'string'clause is redundant since it falls through to the default case.Apply this diff:
- case 'string': default:🧰 Tools
🪛 Biome (1.9.4)
[error] 105-105: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
backend/service/scan_service_python_cli_impl.go (1)
238-245: Consider using flag package for argument parsing.The current implementation of
getOutputPathFromArgsis fragile. Consider using Go'sflagpackage or a dedicated argument parser for more robust handling.backend/repository/scanoss_settings_repository_json_impl.go (1)
70-85: Consider adding error recovery.While the error handling is improved with logging, consider adding recovery logic when the settings file read fails, such as creating a default settings file.
func (r *ScanossSettingsJsonRepository) setSettingsFile(path string) { sf, err := r.Read() if err != nil { log.Error().Err(err).Msgf("Error reading settings file: %v", path) + // Create default settings + sf = entities.SettingsFile{ + Settings: &entities.ScanossSettingsSchema{}, + Bom: &entities.Bom{}, + } return } if entities.ScanossSettingsJson == nil { entities.ScanossSettingsJson = &entities.ScanossSettings{ SettingsFile: &sf, } } entities.ScanossSettingsJson.SettingsFile = &sf }CHANGELOG.md (1)
13-40: Fix grammar in changelog entries.Consider these improvements:
- Line 26: Change "Check for several python default install locations" to "Check for several Python installation locations"
- Line 38: Remove redundant "Remove" as it's already under the "Removed" section
-Check for several python default install locations when running scan command +Check for several Python installation locations when running scan command -Remove "Scan Current Directory" menu option +"Scan Current Directory" menu option🧰 Tools
🪛 LanguageTool
[grammar] ~26-~26: Possible agreement error. The noun ‘default’ seems to be countable; consider using: “several python defaults”.
Context: ...without running the process - Check for several python default install locations when running scan com...(MANY_NN)
[grammar] ~38-~38: You’ve repeated a verb. Did you mean to only write one of them?
Context: ...oss directory if it does not exist ### Removed - Remove "Scan Current Directory" menu option ...(REPEATED_VERBS)
📜 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 (32)
.gitignore(1 hunks)CHANGELOG.md(2 hunks)app.go(3 hunks)backend/entities/scan.go(1 hunks)backend/repository/component_repository_json_impl.go(1 hunks)backend/repository/file_repository_impl.go(2 hunks)backend/repository/file_repository_impl_test.go(1 hunks)backend/repository/result_repository_json_impl.go(2 hunks)backend/repository/scanoss_settings_repository_json_impl.go(1 hunks)backend/service/scan_service_python_cli_impl.go(5 hunks)cmd/root.go(1 hunks)cmd/scan.go(3 hunks)frontend/package.json(1 hunks)frontend/package.json.md5(1 hunks)frontend/src/components/EmptyState.tsx(1 hunks)frontend/src/components/Link.tsx(1 hunks)frontend/src/components/MatchComparison.tsx(2 hunks)frontend/src/components/ScanDialog.tsx(9 hunks)frontend/src/components/ScanOption.tsx(1 hunks)frontend/src/components/SelectResultsFile.tsx(1 hunks)frontend/src/components/SelectScanRoot.tsx(2 hunks)frontend/src/components/ui/dialog.tsx(2 hunks)frontend/src/components/ui/input.tsx(1 hunks)frontend/src/modules/results/stores/useResultsStore.ts(2 hunks)frontend/src/routes/root.tsx(3 hunks)frontend/src/stores/useConfigStore.ts(1 hunks)frontend/wailsjs/go/main/App.d.ts(0 hunks)frontend/wailsjs/go/main/App.js(0 hunks)frontend/wailsjs/go/models.ts(1 hunks)frontend/wailsjs/go/service/ScanServicePythonImpl.d.ts(1 hunks)frontend/wailsjs/go/service/ScanServicePythonImpl.js(1 hunks)internal/config/config.go(6 hunks)
💤 Files with no reviewable changes (2)
- frontend/wailsjs/go/main/App.d.ts
- frontend/wailsjs/go/main/App.js
✅ Files skipped from review due to trivial changes (3)
- .gitignore
- frontend/package.json.md5
- frontend/src/components/ui/input.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
frontend/src/components/ScanOption.tsx
[error] 105-105: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
[error] 75-75: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
🪛 LanguageTool
CHANGELOG.md
[grammar] ~26-~26: Possible agreement error. The noun ‘default’ seems to be countable; consider using: “several python defaults”.
Context: ...without running the process - Check for several python default install locations when running scan com...
(MANY_NN)
[grammar] ~38-~38: You’ve repeated a verb. Did you mean to only write one of them?
Context: ...oss directory if it does not exist ### Removed - Remove "Scan Current Directory" menu option ...
(REPEATED_VERBS)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (70)
internal/config/config.go (30)
62-63: Add concurrency safeguards
Introducingsync.RWMutexand a dedicatedlistenersslice is a good approach for thread-safe listener operations.
76-80: Ensure correct listener registration
Locking and appending to thelistenersslice is correctly handled.
82-92: Prevent data race when notifying
Copying the listener slice under a read lock avoids data races while iterating.
94-96: Generated path helper
Usingfilepath.Joinhere is cross-platform friendly.
98-100: Maintain consistent naming
getDefaultScanSettingsFilePathmatches the pattern ofgetDefaultResultFilePath.
102-106: Safe concurrent read
Read locks ensureApiTokenretrieval is thread-safe.
108-112: API URL accessor
Similar thread-safe pattern maintains consistency across getters.
114-118: ResultFilePath getter
No issues spotted; consistently follows the rlock pattern.
120-124: GetScanRoot
Method naming is consistent with other getters.
126-130: GetScanSettingsFilePath
Completes the set of read-locked accessors.
133-133: SetApiToken change
Refreshed concurrency usage withmu.Unlock()followed bynotifyListeners()—looks correct.Also applies to: 136-137
142-142: SetApiUrl change
Same concurrency pattern asSetApiToken, no concerns.Also applies to: 145-146
151-151: SetResultFilePath
Forces a listener notification right after unlocking, ensuring updates are propagated.Also applies to: 153-154
167-167: SetScanSettingsFilePath
Matches concurrency-safe pattern found in other setters.Also applies to: 169-170
173-173: GetDefaultConfigFolder
Exposes a clear helper for retrieving the default config directory.
183-183: setupLogger with concurrency
Method bounding toConfigis consistent with the rest of the design.
209-210: InitializeConfig
CallssetupLoggerfirst, ensuring logs are established early.
225-225: AddConfigPath usage
Allows custom config folder usage—a practical approach.
228-229: Default configuration
Explicitly initializing defaults for APIURL and APIToken is clear.
233-234: Create default config directory
Gracefully handles missing directories without failing.
247-248: Retrieve persisted token & URL
Immediately sets them in config, then notifies listeners.
250-250: Debug flag
Explicit boolean assignment keeps it straightforward.
254-255: SetApiToken from CLI
Validates saving token and logs any issues.
259-260: SetApiUrl from CLI
Handles potential errors consistently, similar to token logic.
264-264: Set scan root from CLI
Applying the root from provided argument.
267-267: Override result file
Sets a user-specified value for the file path.
270-270: Override scan settings
Also allows direct user input from CLI.
274-274: Fallback to working directory
Assigns a default scan root if none is provided or discovered.Also applies to: 276-276, 282-282
285-286: Re-check result file path
Ensures the file path is not empty, falling back to defaults.
288-289: Re-check settings file path
Likewise ensures a default setting file if none is specified.frontend/src/modules/results/stores/useResultsStore.ts (1)
73-74: Changed default sort
Sorting by'path'ascending can shift user expectations but is likely simpler to navigate.frontend/src/components/ScanDialog.tsx (14)
3-3: Update copyright
Reflecting the correct year is good housekeeping.
24-25: New animation imports
Incorporatingframer-motionandlucide-reacticons enhances interactivity.
31-31: Label import
Still in use within this file, so no issues spotted.
36-38: Refined WailsJS imports
Pulling inScanOptionfosters better component modularity.Also applies to: 41-41
63-63:showSuccessstate
Allows ephemeral success feedback post-scan.
65-69: Additional local states
directory,scanArgs,advancedScanArgs, andoptionsfacilitate flexible scan parameterization.
133-135: handleOptionChange
Neat helper pattern to unify updates for any option.
137-157: handleFileSelect logic
Selecting a file into an arg maintains a consistent approach with other input changes.
219-219: Filtering for core options
An elegant way to separate advanced from core arguments.
229-239: Directory selection block
Showcases a read-only input plus a folder button for picking directories—user-friendly design.
240-257: Rendering core options
UsingScanOptionfor each argument ensures consistent UI across varied argument types.
258-278: Advanced options
Letting users directly provide extra flags is a flexible extension.
280-286: Conditional console output
Only shown if lines exist—keeps the UI clean.
287-339: Scan button animations
Visual feedback for loading vs. success states improves UX.frontend/wailsjs/go/service/ScanServicePythonImpl.d.ts (1)
3-3: LGTM! The new type-safe scan arguments API aligns with the PR objectives.The addition of
GetScanArgs()returningPromise<Array<entities.ScanArgDef>>provides a type-safe way to fetch scan argument definitions, which supports the enhanced scan options UX/UI objective.Also applies to: 10-10
frontend/wailsjs/go/service/ScanServicePythonImpl.js (1)
13-15: LGTM! The implementation follows the established pattern.The
GetScanArgs()implementation correctly follows the same pattern as other service methods in the file.frontend/src/components/EmptyState.tsx (3)
1-2: LGTM! Good use of UI components and utilities.The imports of Button component and cn utility function support the enhanced functionality.
4-14: LGTM! Well-structured interface with optional properties.The EmptyStateProps interface is well-designed with:
- Optional icon and image for visual flexibility
- Optional action for interactive states
- Optional className for styling customization
16-33: LGTM! Clean implementation with proper conditional rendering.The component implementation:
- Uses proper TypeScript props destructuring
- Implements conditional rendering for optional elements
- Uses the cn utility for className composition
- Follows good accessibility practices with alt text for images
frontend/src/components/Link.tsx (1)
36-36: LGTM! Enhanced hover state improves visual feedback.The addition of
hover:text-blue-600provides better visual feedback for interactive states.frontend/src/stores/useConfigStore.ts (1)
53-53: LGTM! Improved state management with synchronization.The addition of the
getparameter and subsequent call togetInitialConfigafter setting the scan root ensures that the configuration stays synchronized.Also applies to: 60-60
backend/repository/file_repository_impl_test.go (1)
52-52: LGTM! Enhanced encapsulation.Changed from direct property access to method call, improving encapsulation while maintaining test functionality.
backend/repository/file_repository_impl.go (2)
44-44: LGTM! Enhanced encapsulation with getter method.Changed from direct property access to getter method for scan root, improving encapsulation.
57-61: LGTM! Improved code organization and readability.Good refactoring:
- Introduced local variable to avoid repeated GetInstance() calls
- Changed to getter methods for better encapsulation
backend/repository/component_repository_json_impl.go (1)
60-60: LGTM! Good encapsulation practice.The change from direct property access to using a getter method improves encapsulation and maintainability.
frontend/src/components/SelectScanRoot.tsx (1)
30-30: LGTM! Good UX improvement.Fetching results immediately after setting the scan root ensures the UI stays in sync with the selected directory.
Also applies to: 45-45, 52-52
frontend/src/routes/root.tsx (1)
38-41: LGTM! Good state management simplification.The changes simplify the modal state management by:
- Using a boolean instead of an object
- Simplifying the event handlers
- Cleaning up the component rendering
Also applies to: 43-45, 53-54, 78-78
backend/repository/result_repository_json_impl.go (1)
47-47: LGTM! Good encapsulation practice.The change from direct property access to using a getter method improves encapsulation and maintainability.
cmd/root.go (1)
88-90: LGTM! Good architectural improvement.The refactoring to use a singleton pattern for configuration management is a solid improvement that ensures consistent configuration access across the application.
frontend/src/components/ui/dialog.tsx (1)
40-47: LGTM! Great UX improvements.The changes enhance the dialog's user experience by:
- Adding proper scrolling behavior with ScrollArea
- Ensuring content fits within viewport using dynamic viewport units
- Maintaining proper padding and layout
cmd/scan.go (1)
94-105: LGTM! Clean flag registration.The type-safe flag registration using a switch statement ensures proper handling of different argument types.
frontend/src/components/MatchComparison.tsx (1)
76-89: LGTM! Great UX improvement.The EmptyState component provides clear feedback and guidance when no file is selected, with a helpful action button to initiate a new scan.
frontend/src/components/ScanOption.tsx (1)
32-41: LGTM! Well-defined props interface.The
ScanOptionPropsinterface is well-structured and provides clear type definitions for all required props.app.go (1)
41-46: LGTM! Improved configuration management.The addition of the
cfgfield and its initialization inInitimproves encapsulation and makes configuration access more maintainable.Also applies to: 52-57
backend/repository/scanoss_settings_repository_json_impl.go (3)
49-63: LGTM! Improved error handling and configuration management.The changes enhance the initialization process with proper error handling and configuration change notifications.
65-68: LGTM! Well-documented configuration change handler.The function properly handles configuration updates by refreshing the settings file path.
98-120: LGTM! Thread-safe implementation with proper error handling.The changes improve thread safety with proper locking and enhance error handling with config instance checks.
frontend/wailsjs/go/models.ts (1)
373-396: LGTM! Well-structured scan argument definition class.The class provides a robust structure for defining scan arguments with proper type definitions and JSON handling.
frontend/package.json (1)
35-35: LGTM! Animation library addition aligns with UI/UX improvements.The addition of framer-motion supports the PR's objective of enhancing scan options UX/UI.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
frontend/src/components/ScanOption.tsx (3)
32-41: Enhance type safety with discriminated union types.Consider using a discriminated union type to ensure value and defaultValue match the specified type. Also, consider making isFileSelector required when onSelectFile is provided.
Here's an improved type definition:
type ScanOptionType = 'string' | 'int' | 'bool' | 'stringSlice'; interface BaseScanOptionProps { name: string; usage: string; } interface StringScanOptionProps extends BaseScanOptionProps { type: 'string'; value: string; defaultValue: string; onSelectFile?: () => void; isFileSelector?: boolean; } interface IntScanOptionProps extends BaseScanOptionProps { type: 'int'; value: number; defaultValue: number; } interface BoolScanOptionProps extends BaseScanOptionProps { type: 'bool'; value: boolean; defaultValue: boolean; } interface StringSliceScanOptionProps extends BaseScanOptionProps { type: 'stringSlice'; value: string[]; defaultValue: string[]; onSelectFile?: () => void; isFileSelector?: boolean; } type ScanOptionProps = | StringScanOptionProps | IntScanOptionProps | BoolScanOptionProps | StringSliceScanOptionProps;
44-47: Extract display name formatting to a utility function.Consider moving the display name formatting logic to a reusable utility function.
const formatDisplayName = (name: string): string => name .split('-') .map((word) => word.charAt(0).toUpperCase() + word.slice(1)) .join(' ');
105-107: Remove redundant case clause.The 'string' case is redundant since it's handled by the default clause.
- case 'string': default:🧰 Tools
🪛 Biome (1.9.4)
[error] 105-105: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
backend/service/scan_service_python_cli_impl.go (1)
227-238: Consider moving output folder creation to Python CLI.The comment on line 118 suggests this should be handled by the Python CLI. Consider creating an issue to track moving this responsibility to the Python CLI for better separation of concerns.
Would you like me to help create an issue to track this improvement?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/service/scan_service_python_cli_impl.go(5 hunks)frontend/src/components/ScanOption.tsx(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
frontend/src/components/ScanOption.tsx
[error] 105-105: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (6)
frontend/src/components/ScanOption.tsx (2)
1-31: LGTM!The license header is properly included, and imports are well-organized.
43-139: LGTM! Well-implemented component with good UX.The component is well-structured with appropriate input types, error handling, and tooltips. The implementation aligns well with the PR objectives of enhancing scan options UX/UI.
🧰 Tools
🪛 Biome (1.9.4)
[error] 105-105: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
backend/service/scan_service_python_cli_impl.go (4)
33-37: LGTM!The new imports are properly organized and necessary for the added functionality.
191-197: LGTM!The changes improve encapsulation by using getter methods instead of direct field access, consistent with the broader codebase modifications.
Also applies to: 206-212
223-225: LGTM!The implementation aligns with the PR objectives by providing access to predefined scan argument definitions.
240-247:⚠️ Potential issueAdd bounds checking to prevent potential panic.
The current implementation could panic if "--output" is the last argument in the array.
Apply this diff to make the implementation more robust:
func (s *ScanServicePythonImpl) getOutputPathFromArgs(args []string) string { - for i := 0; i < len(args)-1; i++ { + for i := 0; i < len(args); i++ { if args[i] == "--output" { + if i+1 >= len(args) { + log.Warn().Msg("--output flag provided without a value") + return "" + } return args[i+1] } } return "" }Likely invalid or redundant comment.
matiasdaloia
deleted the
feature/mdaloia/ES-140-SCANOSS-Code-Compare-Scan-with-options-enhancements
branch
What
Summary by CodeRabbit
New Features
Bug Fixes
Style