[SP-2882] feat: add online component search #112
Conversation
WalkthroughAdds online component search and license lookup across backend and frontend: new entities and models, Scanoss API interface and HTTP implementation, service wiring and Wails bindings, frontend search/dialog/keyboard-table/dialog-state UI, utility and CI/dependency updates, generated Wails models, and removal of two backend tests. Changes
Sequence Diagram(s)sequenceDiagram
participant FE as OnlineComponentSearchDialog (FE)
participant W as Wails Bridge
participant CS as ComponentServiceImpl (BE)
participant API as ScanossApiServiceHttpImpl
participant SC as Scanoss API
FE->>W: SearchComponents(request)
W->>CS: SearchComponents(request)
CS->>API: SearchComponents(request)
API->>SC: GET /api/v2/components/search?...
SC-->>API: 200 JSON (components[])
API-->>CS: ComponentSearchResponse
CS-->>W: ComponentSearchResponse
W-->>FE: components[]
sequenceDiagram
participant FE as ReplaceComponentDialog (FE)
participant W as Wails Bridge
participant LS as LicenseServiceImpl (BE)
participant API as ScanossApiServiceHttpImpl
participant SC as Scanoss API
FE->>W: GetLicensesByPurl({purl,...})
W->>LS: GetLicensesByPurl(request)
LS->>API: GetLicensesByPurl(request)
API->>SC: GET /api/v2/licenses/component?purl=...
SC-->>API: 200 JSON (component licenses)
API-->>LS: GetLicensesByPurlResponse
LS-->>W: GetLicensesByPurlResponse
W-->>FE: matchedLicenses
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
internal/utils/url_utils.go (1)
5-13: Function works correctly but consider minor improvements.The logic is sound and handles both HTTP and HTTPS protocols properly. Consider these enhancements:
- Naming convention: Go prefers
URLoverUrl- considerRemoveProtocolFromURL- Documentation: Add a comment describing the function's purpose
- Input validation: Consider handling edge cases like empty strings
+// RemoveProtocolFromURL removes the HTTP or HTTPS protocol prefix from a URL string. +// If no protocol is found, returns the original string unchanged. -func RemoveProtocolFromUrl(url string) string { +func RemoveProtocolFromURL(url string) string { + if url == "" { + return url + } if after, ok := strings.CutPrefix(url, "http://"); ok { return after } if after, ok := strings.CutPrefix(url, "https://"); ok { return after } return url }frontend/src/components/OnlineComponentSearchDialog.tsx (2)
43-82: Consider externalizing the catalog packages configuration.The large
catalogPackagesarray might be better maintained in a separate configuration file or fetched from the backend to ensure consistency.Consider moving this to a separate config file:
// config/catalogPackages.ts export const catalogPackages = [ 'angular', 'apache', // ... rest of packages ];
143-147: Consider responsive dialog sizing.The dialog uses fixed viewport units (
calc(80dvh),calc(60dvw)) which might not work well on very small screens.Consider adding responsive breakpoints:
- <DialogContent className="max-h-[calc(80dvh)] max-w-[calc(60dvw)] p-4"> + <DialogContent className="max-h-[calc(80dvh)] max-w-[calc(60dvw)] p-4 sm:max-w-[calc(90vw)] md:max-w-[calc(60dvw)]">backend/service/scanoss_api_service_grpc_impl.go (1)
52-68: Consider adding API key validation.While the API key is properly transmitted over TLS, consider adding validation to ensure it meets expected format requirements (e.g., length, character set) to prevent potential injection attacks.
Add validation in the constructor:
func NewScanossApiServiceGrpcImpl( mapper mappers.ScanossApiMapper, ) ScanossApiService { cfg := config.GetInstance() + apiKey := cfg.GetApiToken() + + // Validate API key format if present + if apiKey != "" && !isValidApiKey(apiKey) { + log.Error().Msg("Invalid API key format") + apiKey = "" + } + service := &ScanossApiServiceGrpcImpl{ mapper: mapper, timeout: 30 * time.Second, endpoint: utils.RemoveProtocolFromUrl(cfg.GetApiUrl()), - apiKey: cfg.GetApiToken(), + apiKey: apiKey, }Also add a validation function:
func isValidApiKey(key string) bool { // Implement validation based on expected API key format // e.g., check length, allowed characters, etc. return len(key) > 0 && len(key) <= 256 }
📜 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 (18)
backend/entities/component_search.go(1 hunks)backend/mappers/scanoss_api_mapper.go(1 hunks)backend/service/component_service.go(2 hunks)backend/service/component_service_impl.go(2 hunks)backend/service/component_service_test.go(0 hunks)backend/service/scanoss_api_service.go(1 hunks)backend/service/scanoss_api_service_grpc_impl.go(1 hunks)frontend/src/components/ComponentSearchTable.tsx(1 hunks)frontend/src/components/OnlineComponentSearchDialog.tsx(1 hunks)frontend/src/components/ReplaceComponentDialog.tsx(7 hunks)frontend/src/components/ui/select.tsx(1 hunks)frontend/src/components/ui/table.tsx(1 hunks)frontend/wailsjs/go/models.ts(3 hunks)frontend/wailsjs/go/service/ComponentServiceImpl.d.ts(1 hunks)frontend/wailsjs/go/service/ComponentServiceImpl.js(1 hunks)go.mod(4 hunks)internal/utils/url_utils.go(1 hunks)main.go(1 hunks)
💤 Files with no reviewable changes (1)
- backend/service/component_service_test.go
🧰 Additional context used
🧬 Code Graph Analysis (6)
backend/service/scanoss_api_service.go (2)
backend/entities/component_search.go (2)
ComponentSearchRequest(27-34)ComponentSearchResponse(37-40)frontend/wailsjs/go/models.ts (2)
ComponentSearchRequest(182-203)ComponentSearchResponse(234-265)
backend/entities/component_search.go (2)
backend/entities/component.go (1)
Component(104-158)frontend/wailsjs/go/models.ts (2)
SearchedComponent(218-233)StatusResponse(204-217)
frontend/wailsjs/go/service/ComponentServiceImpl.d.ts (3)
frontend/wailsjs/go/service/ComponentServiceImpl.js (1)
SearchComponents(37-39)backend/entities/component_search.go (2)
ComponentSearchRequest(27-34)ComponentSearchResponse(37-40)frontend/wailsjs/go/models.ts (2)
ComponentSearchRequest(182-203)ComponentSearchResponse(234-265)
frontend/wailsjs/go/models.ts (1)
backend/entities/component_search.go (4)
ComponentSearchRequest(27-34)StatusResponse(50-53)SearchedComponent(43-47)ComponentSearchResponse(37-40)
main.go (3)
backend/mappers/scanoss_api_mapper.go (1)
NewScanossApiMapper(40-42)backend/service/scanoss_api_service_grpc_impl.go (1)
NewScanossApiServiceGrpcImpl(52-68)backend/service/component_service_impl.go (1)
NewComponentServiceImpl(50-65)
frontend/src/components/ui/table.tsx (1)
frontend/src/lib/utils.ts (1)
cn(27-29)
🔇 Additional comments (55)
frontend/src/components/ui/select.tsx (1)
25-25: LGTM! Height adjustment improves visual consistency.The change from
h-8toh-9increases the SelectTrigger height slightly for better alignment with other form elements in the new component search dialogs.frontend/wailsjs/go/service/ComponentServiceImpl.js (1)
37-39: LGTM! Auto-generated binding follows established pattern.The new
SearchComponentsfunction is correctly generated and follows the same pattern as other service bindings in this file.frontend/wailsjs/go/service/ComponentServiceImpl.d.ts (1)
21-21: LGTM! TypeScript definition is correctly typed.The function signature properly uses
entities.ComponentSearchRequestand returnsPromise<entities.ComponentSearchResponse>, maintaining type safety between frontend and backend.backend/service/component_service.go (2)
3-3: LGTM! Copyright year update is appropriate.
36-36: LGTM! Well-designed interface method addition.The
SearchComponentsmethod follows Go conventions with proper parameter and return types, including error handling. The signature is consistent with other interface methods.main.go (3)
90-90: LGTM! Proper mapper instantiation.The
scanossApiMapperis correctly instantiated using the factory method pattern.
93-93: LGTM! Proper service instantiation with dependency injection.The
scanossApiServiceis correctly instantiated with the mapper dependency injected through the constructor.
94-94: LGTM! Component service updated with new dependencies.The component service constructor has been properly updated to include the new
scanossApiServiceand maintains the correct parameter order as expected by the implementation.backend/service/scanoss_api_service.go (1)
28-30: LGTM! Well-designed service interface.The interface follows Go best practices with:
- Single responsibility principle
- Clear method signature with proper error handling
- Abstraction of implementation details
- Use of domain entities for strong typing
frontend/src/components/ComponentSearchTable.tsx (2)
7-13: LGTM! Well-typed component interface.The component props are properly typed using the generated entities, and the callback pattern for component selection is appropriate.
24-37: LGTM! Proper table implementation with good UX.The table implementation includes:
- Semantic table structure with proper headers
- Unique key using
component.purl(appropriate for component search results)- Clickable rows with visual feedback (
cursor-pointer)- External link icon for URLs enhancing user understanding
- Clean, accessible markup
go.mod (2)
3-5: LGTM! Reasonable Go version upgrade.Upgrading to Go 1.23.0 with toolchain 1.23.2 is appropriate for accessing latest language features and improvements.
15-20: Dependency Security Verification
- google.golang.org/grpc v1.73.0: No known unpatched vulnerabilities as of July 2025; incorporates all previous HTTP/2 and CVE fixes.
- github.com/scanoss/papi v0.12.0: No publicly reported security issues.
Both versions are current stable releases. Continue routine monitoring for new advisories.
frontend/wailsjs/go/models.ts (4)
182-203: LGTM! ComponentSearchRequest model properly generated.The model correctly mirrors the backend Go struct with:
- Required
searchfield- Optional fields properly marked with
?- Standard createFrom/constructor pattern consistent with other models
204-217: LGTM! StatusResponse model is correctly structured.Simple model with optional
codeandmessagefields matching the backend entity definition.
218-233: LGTM! SearchedComponent model properly defined.All required fields (
component,purl,url) are correctly typed as strings, matching the backend struct.
234-265: LGTM! ComponentSearchResponse model with proper nested object handling.The model correctly:
- Handles the
componentsarray with proper type conversion- Includes
statusfield with nested object conversion- Uses the standard
convertValuesmethod for recursive object instantiationThe convertValues implementation properly handles arrays, objects, and primitive types.
backend/entities/component_search.go (4)
27-34: LGTM! Well-structured request entity with appropriate validation.The
ComponentSearchRequeststruct is well-designed with proper JSON tags and validation. Therequired,min=1validation on the search field ensures non-empty searches.
37-40: LGTM! Response structure follows good API design patterns.The
ComponentSearchResponsestruct properly encapsulates both the data (components) and metadata (status) in a single response structure.
43-47: LGTM! Clean and focused component representation.The
SearchedComponentstruct contains the essential fields needed for component identification and linking.
50-53: Confirm Alignment of StatusResponse.Code with gRPC APIWe’ve verified that
StatusResponse.Codeand its mappings inscanoss_api_mapper.goconsistently useint32across the codebase. No.protofiles were found here, so please double-check that your external gRPC API definitions also useint32(rather than, for example,uint32or an enum). If they differ, update this struct to match the spec.frontend/src/components/ui/table.tsx (7)
5-17: LGTM! Well-implemented Table component with proper overflow handling.The Table component correctly uses forwardRef and wraps the table in a scrollable container for responsive design.
19-25: LGTM! TableHeader follows semantic HTML structure.The component properly uses
theadelement with appropriate border styling for table headers.
27-37: LGTM! TableBody component is correctly implemented.The component uses proper
tbodysemantics and handles last row border styling appropriately.
54-67: LGTM! TableRow includes good interactive states.The component properly implements hover states and selected state styling using data attributes.
69-82: LGTM! TableHead component handles accessibility features.The component includes proper checkbox positioning and role attribute handling for accessibility.
84-97: LGTM! TableCell maintains consistent styling with TableHead.The component mirrors the checkbox and role handling from TableHead while using appropriate cell padding.
111-120: LGTM! Clean exports for the table component library.All table components are properly exported for external consumption.
backend/service/component_service_impl.go (4)
43-43: LGTM! Proper dependency injection pattern.Adding the
scanossApiServicefield follows good dependency injection practices.
55-55: LGTM! Proper field initialization.The new dependency is correctly assigned in the constructor.
287-295: LGTM! Clean delegation pattern with appropriate logging.The
SearchComponentsmethod properly delegates to the injected service with good debug logging. The log fields provide useful context without exposing sensitive information.
50-50: Constructor signature change fully propagatedAll calls to
NewComponentServiceImplinclude the newscanossApiServiceargument (e.g., inmain.go), and no other instantiation sites were found. No further updates required.backend/mappers/scanoss_api_mapper.go (4)
31-34: LGTM! Well-defined mapper interface.The interface clearly defines the mapping operations needed for gRPC communication.
45-54: LGTM! Straightforward request mapping.The mapping from internal entity to gRPC request is direct and correct.
57-66: LGTM! Proper slice mapping with type conversion.The component slice mapping correctly handles the conversion between gRPC and internal types.
76-81: LGTM! Proper status mapping with nil check.The status mapping correctly handles the optional status field and properly converts the gRPC status to internal format.
frontend/src/components/OnlineComponentSearchDialog.tsx (7)
40-42: LGTM! Well-defined constants for configuration.The debounce timing and status code constants improve code readability and maintainability.
104-122: LGTM! Robust query function with proper error handling.The React Query implementation correctly handles empty searches, API calls, and error scenarios.
131-141: LGTM! Good error handling with user feedback.The error handling properly displays toast notifications and logs errors for debugging.
184-189: LGTM! Clear loading state with context.The loading indicator includes the current search term, providing good user feedback.
191-199: LGTM! Comprehensive error state messaging.The error state provides clear messaging and actionable guidance for users.
211-218: LGTM! Well-structured results display with scrollable area.The results section properly handles the display of search results with appropriate scrolling and count information.
116-118: Confirm status code mapping
I couldn’t find a matching “success” code definition in the backend for the frontend’sconst STATUS_CODE_OK = 1Please verify that your HTTP/JSON handlers (or your
.proto-definedStatusCodeenum) actually returnstatus.code = 1on success. If the backend uses a different value (e.g.0), either updateSTATUS_CODE_OKor align the backend’s constant accordingly.• Frontend:
frontend/src/components/OnlineComponentSearchDialog.tsx:41
• Look in your Go code for the JSON response wrapper (e.g. aStatusstruct underbackend/…withjson:"status"and its code constants)
• If you’re using gRPC, inspect all*.protofiles for theStatusCodeenum valuesfrontend/src/components/ReplaceComponentDialog.tsx (7)
26-26: LGTM!The Search icon import is correctly added alongside other lucide-react icons.
41-41: LGTM!The OnlineComponentSearchDialog component import follows the existing import pattern.
74-76: LGTM!The new state variables are properly initialized and follow React conventions.
129-144: LGTM!The handler correctly manages component selection, prevents duplicates, and updates the form consistently with the existing patterns.
194-194: LGTM!Converting CommandInput to a controlled component enables proper tracking of the search value.
209-222: LGTM!The online search option is well-implemented with proper conditional rendering and state management.
310-316: LGTM!The OnlineComponentSearchDialog integration follows the established pattern and properly manages dialog state.
backend/service/scanoss_api_service_grpc_impl.go (5)
24-41: LGTM!Package declaration and imports are well-organized following Go conventions.
43-50: LGTM!The struct is well-designed with all necessary fields for a gRPC client service.
52-68: Verify error handling strategy for gRPC client initialization.The constructor logs but doesn't return the error when gRPC client initialization fails. This allows service creation to succeed even without a working gRPC connection. Ensure this graceful degradation approach is intentional and that downstream error handling in
SearchComponentsis sufficient.
70-108: LGTM!The SearchComponents method is well-implemented with proper validation, error handling, and timeout management.
140-154: LGTM!The helper methods for authentication and connection cleanup are properly implemented.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
backend/service/scanoss_api_service_grpc_impl.go (3)
26-41: Organize imports by group for better readability.Group imports following Go conventions: standard library, external packages, then internal packages.
import ( "context" "crypto/tls" "fmt" "time" "github.com/rs/zerolog/log" "github.com/scanoss/papi/api/componentsv2" + "google.golang.org/grpc" + "google.golang.org/grpc/credentials" + "google.golang.org/grpc/metadata" + "github.com/scanoss/scanoss.cc/backend/entities" "github.com/scanoss/scanoss.cc/backend/mappers" "github.com/scanoss/scanoss.cc/internal/config" "github.com/scanoss/scanoss.cc/internal/utils" - "google.golang.org/grpc" - "google.golang.org/grpc/credentials" - "google.golang.org/grpc/metadata" )
58-58: Consider making the timeout configurable.The timeout is hardcoded to 30 seconds. Consider making it configurable through environment variables or configuration.
+const defaultTimeout = 30 * time.Second + func NewScanossApiServiceGrpcImpl( mapper mappers.ScanossApiMapper, ) ScanossApiService { cfg := config.GetInstance() + timeout := defaultTimeout + if configuredTimeout := cfg.GetGrpcTimeout(); configuredTimeout > 0 { + timeout = configuredTimeout + } service := &ScanossApiServiceGrpcImpl{ mapper: mapper, - timeout: 30 * time.Second, + timeout: timeout, endpoint: utils.RemoveProtocolFromURL(cfg.GetApiUrl()), apiKey: cfg.GetApiToken(), }
125-125: Consider adding gRPC connection options for production use.The gRPC client creation could benefit from additional options for better reliability and observability.
- conn, err := grpc.NewClient(s.endpoint, grpc.WithTransportCredentials(credentials.NewTLS(config))) + conn, err := grpc.NewClient( + s.endpoint, + grpc.WithTransportCredentials(credentials.NewTLS(config)), + grpc.WithDefaultCallOptions( + grpc.MaxCallRecvMsgSize(10*1024*1024), // 10MB max message size + ), + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/service/scanoss_api_service_grpc_impl.go(1 hunks)frontend/src/components/OnlineComponentSearchDialog.tsx(1 hunks)internal/utils/url_utils.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- frontend/src/components/OnlineComponentSearchDialog.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/utils/url_utils.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/service/scanoss_api_service_grpc_impl.go (7)
backend/mappers/scanoss_api_mapper.go (1)
ScanossApiMapper(31-34)backend/service/scanoss_api_service.go (1)
ScanossApiService(28-30)internal/config/config.go (3)
GetInstance(111-116)SCANOSS_PREMIUM_API_URL(55-55)Config(58-68)internal/utils/url_utils.go (1)
RemoveProtocolFromURL(7-19)backend/entities/component_search.go (2)
ComponentSearchRequest(27-34)ComponentSearchResponse(37-40)internal/utils/custom_validations.go (1)
GetValidator(37-39)backend/entities/component.go (1)
Component(104-158)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
frontend/src/components/ui/popover.tsx (1)
16-27: ReplacePopoverPrimitive.PopoverPortalwithPopoverPrimitive.PortalIn @radix-ui/react-popover v1.1.14, the Popover portal component is exported as
Portal. Continuing to usePopoverPrimitive.PopoverPortalwill be undefined at runtime.Please update in
frontend/src/components/ui/popover.tsx(around lines 16–27):- <PopoverPrimitive.PopoverPortal> + <PopoverPrimitive.Portal> <PopoverPrimitive.Content ref={ref} align={align} sideOffset={sideOffset} className={cn( 'z-50 w-72 rounded-md border bg-popover p-4 text-popover-foreground shadow-md outline-none data-[state=open]:animate-in data-[state=closed]:animate-out data-[state=closed]:fade-out-0 data-[state=open]:fade-in-0 data-[state=closed]:zoom-out-95 data-[state=open]:zoom-in-95 data-[side=bottom]:slide-in-from-top-2 data-[side=left]:slide-in-from-right-2 data-[side=right]:slide-in-from-left-2 data-[side=top]:slide-in-from-bottom-2', className )} {...props} /> - </PopoverPrimitive.PopoverPortal> + </PopoverPrimitive.Portal>frontend/src/components/SelectLicenseList.tsx (1)
48-52: Fix stale callbacks and reduce shadowing.
useCallbackhooks have empty deps and captureonSelectfrom props; this can stale ifonSelectchanges. Also,valueparameter shadows statevalue, which is confusing.- const handleSelect = useCallback((value: string) => { - setValue(value); - onSelect(value); - setPopoverOpen(false); - }, []); + const handleSelect = useCallback((licenseId: string) => { + setValue(licenseId); + onSelect(licenseId); + setPopoverOpen(false); + }, [onSelect]); - const handleClearSelection = useCallback(() => { - setValue(undefined); - onSelect(undefined); - }, []); + const handleClearSelection = useCallback(() => { + setValue(undefined); + onSelect(undefined); + }, [onSelect]);Also applies to: 54-57
🧹 Nitpick comments (1)
frontend/src/components/SelectLicenseList.tsx (1)
69-71: Add accessible label to the clear button.Small a11y nit: provide an aria-label so the icon-only button is announced properly.
- <Button size="icon" variant="ghost" onClick={handleClearSelection}> + <Button size="icon" variant="ghost" onClick={handleClearSelection} aria-label="Clear selected license"> <X className="h-4 w-4" /> </Button>
📜 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 (5)
frontend/package.json(1 hunks)frontend/package.json.md5(1 hunks)frontend/src/components/SelectLicenseList.tsx(1 hunks)frontend/src/components/ui/dialog.tsx(0 hunks)frontend/src/components/ui/popover.tsx(2 hunks)
💤 Files with no reviewable changes (1)
- frontend/src/components/ui/dialog.tsx
✅ Files skipped from review due to trivial changes (2)
- frontend/package.json
- frontend/package.json.md5
🧰 Additional context used
🧬 Code Graph Analysis (1)
frontend/src/components/SelectLicenseList.tsx (1)
frontend/src/components/ui/popover.tsx (1)
Popover(31-31)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
frontend/src/components/ShortcutBadge.tsx (1)
1-3: Use semantic and add a11y hints; consider a slightly larger fontSpan works, but is the semantic element for keyboard shortcuts. Also, 8px text is very small; 10–11px is typically the lower bound for readability.
Apply this minimal change:
-export default function ShortcutBadge({ shortcut }: { shortcut: string }) { - return <span className="ml-2 rounded-sm bg-card p-1 text-[8px] leading-none">{shortcut}</span>; -} +export default function ShortcutBadge({ shortcut }: { shortcut: string }) { + return ( + <kbd + title={`Shortcut: ${shortcut}`} + aria-label={`Shortcut: ${shortcut}`} + className="ml-2 rounded-sm bg-card p-1 text-[10px] leading-none font-mono" + > + {shortcut} + </kbd> + ); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src/components/ReplaceComponentDialog.tsx(9 hunks)frontend/src/components/ShortcutBadge.tsx(1 hunks)
🔇 Additional comments (3)
frontend/src/components/ReplaceComponentDialog.tsx (3)
204-204: Controlled CommandInput looks goodBinding value/onValueChange to searchValue is correct and unlocks the inline “Search online” action. No issues.
146-150: Good UX: opening online search closes the popoverThe flow to open the online search dialog and close the popover is sound. This prevents layered overlays.
317-323: Online dialog wiring looks correctProps passed (onOpenChange, searchTerm, onComponentSelect) align with the expected flow. Rendering is gated by onlineSearchDialogOpen. LGTM.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
main.go (1)
92-96: LGTM; minor nit on error wrappingWiring the new Scanoss API service and injecting it into ComponentServiceImpl is correct. Consider
%winstead of%vto preserve error wrapping.- return fmt.Errorf("error initializing scanoss api service: %v", err) + return fmt.Errorf("error initializing scanoss api service: %w", err)Also, for future enhancements, consider propagating a context into outbound API calls for cancellation/timeout alignment with app lifecycle.
backend/service/scanoss_api_service_http_impl.go (4)
92-102: Safer URL constructionIf
endpointalready includes a trailing slash, simple formatting can yield a double slash. Preferurl.JoinPath, which handles slashes correctly.- apiURL := fmt.Sprintf("%s/api/v2/components/search", s.endpoint) + apiURL, err := url.JoinPath(s.endpoint, "/api/v2/components/search") + if err != nil { + return entities.ComponentSearchResponse{}, fmt.Errorf("failed to construct API URL: %w", err) + }Add the import:
import "net/url"
110-130: Avoid reading full body into memory; stream-decode JSONFor large responses, prefer streaming decode over
io.ReadAll. This also reduces memory spikes.- body, err := io.ReadAll(resp.Body) - if err != nil { - log.Error().Err(err).Msg("Error reading API response") - return entities.ComponentSearchResponse{}, fmt.Errorf("failed to read response: %w", err) - } - - if resp.StatusCode != http.StatusOK { - log.Error(). - Int("statusCode", resp.StatusCode). - Str("body", string(body)). - Msg("API returned non-200 status") - return entities.ComponentSearchResponse{}, fmt.Errorf("API returned status %d: %s", resp.StatusCode, string(body)) - } - - var apiResponse entities.ComponentSearchResponse - - if err := json.Unmarshal(body, &apiResponse); err != nil { - log.Error().Err(err).Str("body", string(body)).Msg("Error parsing API response") - return entities.ComponentSearchResponse{}, fmt.Errorf("failed to parse response: %w", err) - } + if resp.StatusCode != http.StatusOK { + // Read a bounded portion of the body for logging on error + limited := io.LimitedReader{R: resp.Body, N: 4096} + errBody, _ := io.ReadAll(&limited) + log.Error(). + Int("statusCode", resp.StatusCode). + Str("body", string(errBody)). + Msg("API returned non-200 status") + return entities.ComponentSearchResponse{}, fmt.Errorf("API returned status %d: %s", resp.StatusCode, string(errBody)) + } + + var apiResponse entities.ComponentSearchResponse + if err := json.NewDecoder(resp.Body).Decode(&apiResponse); err != nil { + log.Error().Err(err).Msg("Error parsing API response") + return entities.ComponentSearchResponse{}, fmt.Errorf("failed to parse response: %w", err) + }
131-146: Redundant slice copy; simplify and ensure empty slice vs nilYou copy fields 1:1 into a new slice, then rebuild the response. Instead, return the decoded response and normalize a nil slice to an empty slice to keep
[]in JSON (better for TS consumers).- components := make([]entities.SearchedComponent, len(apiResponse.Components)) - for i, comp := range apiResponse.Components { - components[i] = entities.SearchedComponent{ - Component: comp.Component, - Purl: comp.Purl, - URL: comp.URL, - } - } - - response := entities.ComponentSearchResponse{ - Components: components, - Status: entities.StatusResponse{ - Status: apiResponse.Status.Status, - Message: apiResponse.Status.Message, - }, - } + if apiResponse.Components == nil { + apiResponse.Components = []entities.SearchedComponent{} + } + response := apiResponse
69-73: Validation log levelInvalid input from the UI isn’t necessarily an error condition; consider logging at warn/debug and returning a typed validation error so the UI can present field-level messages.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
backend/entities/component_search.go(1 hunks)backend/service/scanoss_api_service_http_impl.go(1 hunks)frontend/src/components/OnlineComponentSearchDialog.tsx(1 hunks)frontend/wailsjs/go/models.ts(3 hunks)go.mod(3 hunks)main.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/entities/component_search.go
- frontend/src/components/OnlineComponentSearchDialog.tsx
🧰 Additional context used
🧬 Code Graph Analysis (3)
frontend/wailsjs/go/models.ts (1)
backend/entities/component_search.go (4)
ComponentSearchRequest(27-34)StatusResponse(50-53)SearchedComponent(43-47)ComponentSearchResponse(37-40)
backend/service/scanoss_api_service_http_impl.go (5)
backend/service/scanoss_api_service.go (1)
ScanossApiService(28-30)internal/config/config.go (2)
GetInstance(111-116)SCANOSS_PREMIUM_API_URL(55-55)backend/entities/component_search.go (4)
ComponentSearchRequest(27-34)ComponentSearchResponse(37-40)SearchedComponent(43-47)StatusResponse(50-53)internal/utils/custom_validations.go (1)
GetValidator(37-39)internal/fetch/fetch.go (2)
Options(32-36)Fetch(39-59)
main.go (2)
backend/service/scanoss_api_service_http_impl.go (1)
NewScanossApiServiceHttpImpl(47-65)backend/service/component_service_impl.go (1)
NewComponentServiceImpl(50-65)
🔇 Additional comments (6)
go.mod (1)
30-31: Indirect deps look fineBumping github.com/google/uuid to v1.6.0 and adding github.com/google/go-cmp v0.7.0 as indirects is okay. Make sure
go mod tidy -compat=1.23has been run so go.sum is consistent across environments.frontend/wailsjs/go/models.ts (5)
182-203: TS model matches backend request schemaFields align with backend entities.ComponentSearchRequest. This looks correct and consistent with Wails’ generated models.
204-217: StatusResponse is consistentOptional fields match backend omitempty semantics.
218-233: SearchedComponent aligns with backendNaming and required fields are consistent with Go entity.
234-265: ComponentSearchResponse constructor is correct; consider empty-array default (handled in backend)TS constructor is fine. Since backend ensures
componentsis an empty slice (not null) on response, the UI won’t seeundefinedhere. No change needed in generated models.
689-730: TreeNode model addition looks goodShape and recursive convertValues utility are consistent with the rest of the generated models.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
backend/service/license_service_impl.go (1)
26-31: Add defensive checks and basic request validation in GetLicensesByPurlIf
scanossApiServiceis ever nil (e.g., misconfigured wiring), this will panic. Also, validaterequest.Purlearly to avoid unnecessary calls.Apply this diff:
import ( "sort" "github.com/scanoss/scanoss.cc/backend/entities" "github.com/scanoss/scanoss.cc/backend/repository" + "fmt" + "strings" ) @@ func (s *LicenseServiceImpl) GetLicensesByPurl(request entities.ComponentRequest) (entities.GetLicensesByPurlResponse, error) { - return s.scanossApiService.GetLicensesByPurl(request) + if s.scanossApiService == nil { + return entities.GetLicensesByPurlResponse{}, fmt.Errorf("scanossApiService is not configured") + } + if strings.TrimSpace(request.Purl) == "" { + return entities.GetLicensesByPurlResponse{}, fmt.Errorf("purl is required") + } + return s.scanossApiService.GetLicensesByPurl(request) }Also applies to: 58-60
🧹 Nitpick comments (8)
backend/entities/common.go (2)
3-6: Exported types lack GoDoc commentsAdd brief comments for exported types to satisfy Go conventions and improve readability.
Apply this diff:
type ComponentRequest struct { + // Purl is the package URL of the component to query (required). Purl string `json:"purl" validate:"required,min=1"` + // Requirement is an optional version constraint (e.g., semver range). Requirement string `json:"requirement,omitempty"` }
8-11: Consider making StatusResponse optional in responsesIf some APIs don’t always populate status, consider
omitemptyon the field that embeds StatusResponse (e.g.,Status StatusResponse 'json:"status,omitempty"') to avoid empty objects in JSON.backend/service/license_service.go (1)
30-30: Consider context-aware APISince downstream
ScanossApiServicemanages context (viaSetContext(ctx)), consider addingcontext.Contextto this interface method to propagate cancellations/deadlines.Example:
GetLicensesByPurl(ctx context.Context, request entities.ComponentRequest) (entities.GetLicensesByPurlResponse, error)This would be a broader change; treat as a follow-up enhancement.
backend/service/license_service_impl.go (2)
45-56: Sorting by LicenseId is fine; consider stable sort by Name if UX prefersNot a blocker. If UI expects alphabetical names, sorting by
Namemay be more intuitive.
58-60: Unit tests missing for new methodPlease add unit tests for
GetLicensesByPurlcovering:
- Valid request path (delegation to scanossApiService)
- Empty PURL validation (if adopted)
- Error propagation from scanossApiService
I can scaffold a test with a mock
ScanossApiServiceif you want.backend/entities/license.go (2)
37-41: Go naming: preferIDoverIdFor consistency with Go conventions, consider
ID(e.g.,ID string) instead ofId. The file already usesLicenseIdelsewhere, so align project-wide or leave as-is for consistency.
50-53: Consideromitemptyon response fieldsIf
statusorcomponentmay be absent, addomitemptyto avoid emitting empty objects in JSON.Apply this diff:
type GetLicensesByPurlResponse struct { - Status StatusResponse `json:"status"` - Component ComponentLicenseInfo `json:"component"` + Status StatusResponse `json:"status,omitempty"` + Component ComponentLicenseInfo `json:"component,omitempty"` }backend/service/scanoss_api_service_http_impl.go (1)
172-209: Fix inconsistent error message in GetLicensesByPurl.Line 192 contains an incorrect error message referring to "component search API" when it should refer to "license API".
- log.Error().Err(err).Msg("Error calling SCANOSS component search API") + log.Error().Err(err).Msg("Error calling SCANOSS license API")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
backend/entities/common.go(1 hunks)backend/entities/component_search.go(1 hunks)backend/entities/license.go(1 hunks)backend/service/component_service_impl.go(2 hunks)backend/service/license_service.go(1 hunks)backend/service/license_service_impl.go(2 hunks)backend/service/scanoss_api_service.go(1 hunks)backend/service/scanoss_api_service_http_impl.go(1 hunks)frontend/src/components/ReplaceComponentDialog.tsx(10 hunks)frontend/wailsjs/go/models.ts(5 hunks)frontend/wailsjs/go/service/LicenseServiceImpl.d.ts(1 hunks)frontend/wailsjs/go/service/LicenseServiceImpl.js(1 hunks)main.go(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- frontend/wailsjs/go/service/LicenseServiceImpl.js
🚧 Files skipped from review as they are similar to previous changes (3)
- backend/service/component_service_impl.go
- backend/service/scanoss_api_service.go
- frontend/src/components/ReplaceComponentDialog.tsx
🧰 Additional context used
🧬 Code Graph Analysis (8)
backend/service/license_service.go (4)
frontend/wailsjs/go/service/LicenseServiceImpl.js (1)
GetLicensesByPurl(9-11)backend/entities/common.go (1)
ComponentRequest(3-6)frontend/wailsjs/go/models.ts (2)
ComponentRequest(234-247)GetLicensesByPurlResponse(378-409)backend/entities/license.go (1)
GetLicensesByPurlResponse(50-53)
frontend/wailsjs/go/service/LicenseServiceImpl.d.ts (4)
frontend/wailsjs/go/service/LicenseServiceImpl.js (1)
GetLicensesByPurl(9-11)backend/entities/common.go (1)
ComponentRequest(3-6)frontend/wailsjs/go/models.ts (2)
ComponentRequest(234-247)GetLicensesByPurlResponse(378-409)backend/entities/license.go (1)
GetLicensesByPurlResponse(50-53)
backend/entities/license.go (3)
frontend/wailsjs/go/models.ts (4)
License(444-459)LicenseInfo(182-195)StatusResponse(270-283)ComponentLicenseInfo(196-233)backend/entities/common.go (1)
StatusResponse(8-11)backend/entities/component.go (1)
Component(104-158)
backend/service/license_service_impl.go (6)
backend/repository/license_repository.go (1)
LicenseRepository(28-30)backend/service/scanoss_api_service.go (1)
ScanossApiService(32-36)backend/service/license_service.go (1)
LicenseService(28-31)backend/entities/common.go (1)
ComponentRequest(3-6)frontend/wailsjs/go/models.ts (2)
ComponentRequest(234-247)GetLicensesByPurlResponse(378-409)backend/entities/license.go (1)
GetLicensesByPurlResponse(50-53)
backend/entities/component_search.go (3)
backend/entities/component.go (1)
Component(104-158)frontend/wailsjs/go/models.ts (2)
SearchedComponent(284-299)StatusResponse(270-283)backend/entities/common.go (1)
StatusResponse(8-11)
main.go (4)
backend/service/scanoss_api_service_http_impl.go (1)
NewScanossApiServiceHttpImpl(55-74)backend/service/component_service_impl.go (1)
NewComponentServiceImpl(50-65)backend/service/scanoss_settings_service_impl.go (1)
NewScanossSettingsServiceImpl(35-39)backend/service/license_service_impl.go (1)
NewLicenseServiceImpl(38-43)
frontend/wailsjs/go/models.ts (3)
backend/entities/license.go (3)
LicenseInfo(37-40)ComponentLicenseInfo(42-48)GetLicensesByPurlResponse(50-53)backend/entities/common.go (2)
ComponentRequest(3-6)StatusResponse(8-11)backend/entities/component_search.go (3)
ComponentSearchRequest(27-34)SearchedComponent(37-41)ComponentSearchResponse(44-47)
backend/service/scanoss_api_service_http_impl.go (7)
backend/service/scanoss_api_service.go (1)
ScanossApiService(32-36)internal/config/config.go (2)
GetInstance(111-116)SCANOSS_PREMIUM_API_URL(55-55)backend/entities/component_search.go (2)
ComponentSearchRequest(27-34)ComponentSearchResponse(44-47)frontend/wailsjs/go/models.ts (4)
ComponentSearchRequest(248-269)ComponentSearchResponse(300-331)ComponentRequest(234-247)GetLicensesByPurlResponse(378-409)internal/utils/custom_validations.go (1)
GetValidator(37-39)backend/entities/common.go (1)
ComponentRequest(3-6)backend/entities/license.go (1)
GetLicensesByPurlResponse(50-53)
🔇 Additional comments (26)
frontend/wailsjs/go/service/LicenseServiceImpl.d.ts (1)
7-7: LGTM: New declaration aligns with models and JS wrapperThe signature
GetLicensesByPurl(arg1: entities.ComponentRequest): Promise<entities.GetLicensesByPurlResponse>matches frontend models and the JS bridge wrapper.backend/service/license_service_impl.go (1)
38-42: Constructor signature change: verify all call sites updated
NewLicenseServiceImplnow requiresscanossApiService. Ensure all initializers (e.g., main wiring, tests) are updated accordingly to avoid nil injection.I can generate a repo-wide check to locate calls to
NewLicenseServiceImpland verify argument counts if useful.backend/entities/component_search.go (4)
1-23: LGTM - Standard MIT license header.The license header follows proper MIT license format and is consistent with the project's licensing approach.
26-34: LGTM - Well-structured request entity with proper validation.The
ComponentSearchRequeststruct is well-designed with appropriate JSON tags and validation annotations. The requiredSearchfield with minimum length validation ensures API calls have meaningful search terms.
36-41: LGTM - Clean and focused response component entity.The
SearchedComponentstruct provides the essential fields needed for component search results with clear, descriptive field names.
43-47: Confirm StatusResponse Reference and JSON Structure
StatusResponseis defined inbackend/entities/common.go(lines 8–11) with fieldsStatus string `json:"status,omitempty"` Message string `json:"message,omitempty"`- No other API response struct in the codebase embeds
StatusResponse.ComponentSearchResponsecorrectly imports and uses it underjson:"status".- Please verify that the JSON keys (
componentsandstatus) match the actual API response format, and consider addingomitemptyto the outerstatustag if empty statuses should be omitted.main.go (3)
92-96: LGTM - Proper service initialization with error handling.The
scanossApiServiceinitialization follows the established pattern with appropriate error handling and clear error messaging.
96-96: LGTM - Correct service dependency injection.The updated service constructors properly inject the new
scanossApiServicedependency into bothcomponentServiceandlicenseService.Also applies to: 101-101
116-116: LGTM - Context propagation to the new service.The context setup for
scanossApiServicefollows the established pattern and ensures proper context propagation for HTTP operations.frontend/wailsjs/go/models.ts (8)
182-195: LGTM - Properly generated TypeScript binding for LicenseInfo.The
LicenseInfoclass correctly mirrors the backend entity with appropriate field mapping and follows the established pattern for generated TypeScript bindings.
196-233: LGTM - Comprehensive TypeScript binding with nested object handling.The
ComponentLicenseInfoclass properly handles nestedLicenseInfoobject conversion using theconvertValueshelper method, maintaining type safety.
234-247: LGTM - Clean request entity binding.The
ComponentRequestclass correctly represents the backend entity with appropriate field mapping.
248-269: LGTM - Well-structured search request binding.The
ComponentSearchRequestclass properly maps all the backend fields including optional parameters with correct TypeScript typing.
270-283: LGTM - Status response binding maintains consistency.The
StatusResponseclass follows the established pattern and correctly maps the backend entity fields.
284-299: LGTM - Component search result binding.The
SearchedComponentclass correctly represents the search result entity with all required fields properly mapped.
300-331: LGTM - Complex response binding with proper nested object handling.The
ComponentSearchResponseclass correctly handles the nested array ofSearchedComponentobjects and theStatusResponseusing theconvertValueshelper method.
378-409: LGTM - License response binding with nested object conversion.The
GetLicensesByPurlResponseclass properly handles nested object conversion for bothStatusResponseandComponentLicenseInfousing theconvertValueshelper.backend/service/scanoss_api_service_http_impl.go (9)
1-23: LGTM - Standard MIT license header.The license header follows proper MIT license format and is consistent with the project's licensing approach.
41-46: LGTM - Clean interface abstraction and type definitions.The
HTTPClientinterface provides good testability and theQueryParamstype alias improves code readability.
47-54: LGTM - Well-structured service implementation.The
ScanossApiServiceHttpImplstruct includes all necessary fields for HTTP operations with proper timeout management.
55-74: LGTM - Robust service constructor with configuration.The constructor properly reads configuration, provides sensible defaults, and initializes the HTTP client with appropriate timeout settings.
76-78: LGTM - Context management implementation.The
SetContextmethod properly stores the context for use in HTTP requests.
80-96: LGTM - Robust URL building with proper encoding.The
buildURLmethod correctly handles URL construction and query parameter encoding, with proper error handling.
98-118: LGTM - HTTP request implementation with proper context and headers.The
GetWithParamsmethod correctly uses the stored context, sets appropriate headers, and handles errors properly. This addresses the previous concern about unused client/timeout fields.
121-169: LGTM - Comprehensive component search implementation.The
SearchComponentsmethod includes proper input validation, detailed logging, API key verification, parameter building, error handling, and response decoding. The implementation is thorough and follows good practices.
172-209: LGTM - Well-implemented license fetching method.Aside from the minor error message issue, the
GetLicensesByPurlmethod follows the same robust pattern asSearchComponentswith proper validation, logging, and error handling.
| type ComponentRequest struct { | ||
| Purl string `json:"purl" validate:"required,min=1"` | ||
| Requirement string `json:"requirement,omitempty"` | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Validation tags are inert unless explicitly enforced
You’re using validate:"required,min=1" but there’s no enforcement shown here. Ensure the handling layer (service/controller) runs a validator against ComponentRequest to prevent empty purls from reaching downstream services.
If you’re using go-playground/validator, wire it where requests enter the system. For example, in your service method:
// pseudo-code in the service handling layer
if strings.TrimSpace(req.Purl) == "" {
return ..., fmt.Errorf("purl is required")
}Do you want me to propose a concrete validation snippet in the call sites?
Also applies to: 8-11
🏁 Script executed:
#!/bin/bash
# Search for Go Playground Validator imports and usage in the repository
rg 'go-playground/validator' -n
rg 'validator.New' -n
rg 'validate.Struct' -nLength of output: 656
🏁 Script executed:
#!/bin/bash
# Display main.go around validator setup and handler registration
sed -n '1,200p' main.go
echo "----"
sed -n '200,400p' main.goLength of output: 4916
🏁 Script executed:
#!/bin/bash
# Inspect internal/utils package for validator setup and custom validations
echo "Contents of internal/utils:"
ls internal/utils
echo ""
# Locate SetValidator definition
rg "func SetValidator" -n internal/utils
echo ""
# Show custom_validations.go to see ValidatePurl and SetValidator
sed -n '1,200p' internal/utils/custom_validations.goLength of output: 2006
🏁 Script executed:
#!/bin/bash
# Locate the ComponentServiceImpl definition and its methods to identify where to insert validation
rg -n "type .*ComponentServiceImpl" -n
rg -A3 "NewComponentServiceImpl" -n
rg -R "ComponentServiceImpl" -n serviceLength of output: 1559
Enforce validation on ComponentRequest inputs
The validate:"required,min=1" tag on Purl in backend/entities/common.go only takes effect if you explicitly invoke the validator. Add a call to your global validator at each entry point handling ComponentRequest to reject empty or malformed PURLs before they reach downstream logic:
• backend/service/component_service_impl.go
- In
CreateComponent(and similarly in any other method that acceptsentities.ComponentRequest), invoke:func (s *ComponentServiceImpl) CreateComponent(ctx context.Context, req entities.ComponentRequest) (entities.Component, error) { + if err := utils.GetValidator().Struct(req); err != nil { + return entities.Component{}, fmt.Errorf("invalid component request: %w", err) + } entity := s.mapper.ToEntity(req) // … } - Ensure you have:
import ( // … "github.com/scanoss/scanoss.cc/internal/utils" )
Apply the same pattern wherever ComponentRequest is consumed (e.g., update or batch handlers) so that your required,min=1 and custom valid-purl rules are actually enforced.
🤖 Prompt for AI Agents
In backend/entities/common.go around lines 3 to 6, the struct tag
validate:"required,min=1" on Purl won't run automatically; update each entry
point that accepts entities.ComponentRequest (start with
backend/service/component_service_impl.go in CreateComponent, and any
update/batch handlers) to call the global validator before further processing:
import the validator utility (github.com/scanoss/scanoss.cc/internal/utils) and
invoke the validation helper on the received ComponentRequest, and if validation
returns an error immediately return/propagate a bad-request validation error so
empty or malformed PURLs are rejected early.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (3)
frontend/src/components/SelectLicenseList.tsx (3)
61-66: Stale closure: addonSelectto useCallback deps and avoid param shadowing
handleSelectcapturesonSelectfrom the initial render; if the parent changesonSelect, this handler won’t update. Also, the param namevalueshadows the state variable.-const handleSelect = useCallback((value: string) => { - setValue(value); - onSelect(value); - setPopoverOpen(false); -}, []); +const handleSelect = useCallback((licenseId: string) => { + setValue(licenseId); + onSelect(licenseId); + setPopoverOpen(false); +}, [onSelect]);
67-70: Clear handler should close the popover and update depsWhen clearing, also close the popover. Add
onSelectto deps to avoid stale references.-const handleClearSelection = useCallback(() => { - setValue(undefined); - onSelect(undefined); -}, []); +const handleClearSelection = useCallback(() => { + setValue(undefined); + onSelect(undefined); + setPopoverOpen(false); +}, [onSelect]);
77-79: Selected label can render empty when chosen from Matched listIf the selected license exists only in
matchedLicensesand not inlicenses,licenses?.find(...)? .licenseIdreturnsundefinedand the button renders nothing. Use the selectedvaluedirectly or resolve from both lists.Simple and robust:
- {value ? licenses?.find((license) => license.licenseId === value)?.licenseId : 'Select license'} + {value ?? 'Select license'}If you prefer to resolve the full object, search both sources with a fallback:
const selected = value ? (matchedLicenses?.find(l => l.licenseId === value) ?? licenses?.find(l => l.licenseId === value)) : undefined;And render:
{selected ? selected.licenseId : 'Select license'}
♻️ Duplicate comments (5)
frontend/src/components/SelectLicenseList.tsx (1)
73-73: Modal popover blocks outside interactions; Clear button becomes unclickableUsing
modalprevents pointer events outside thePopoverContent. Since the Clear button is outside the content, it won’t be clickable while the popover is open. Either removemodal, move the Clear button into the content, or ensure clearing also closes the popover.Minimal fix (remove modal):
-<Popover open={popoverOpen} onOpenChange={setPopoverOpen} modal> +<Popover open={popoverOpen} onOpenChange={setPopoverOpen}>frontend/src/components/ReplaceComponentDialog.tsx (4)
116-129: Replace inline type with sharedentities.SearchedComponenttypeThe inline type
{ component: string; purl: string; url: string }inonOnlineComponentSelectedduplicates the structure defined inOnlineComponentSearchDialog. Use the sharedentities.SearchedComponenttype for consistency and type safety.-const onOnlineComponentSelected = (component: { component: string; purl: string; url: string }) => { +const onOnlineComponentSelected = (component: entities.SearchedComponent) => {Also consider clearing
searchValueafter selection to avoid stale text in the input:setOnlineSearchDialogOpen(false); + setSearchValue('');
173-180: Critical: Conflicting keyboard shortcut bindings cause unintended behaviorYou have two
useKeyboardShortcuthooks bound to the sameKEYBOARD_SHORTCUTS.confirm.keys, which will both fire when the popover is open, potentially causing the form to submit while trying to search online.Consolidate into a single handler that branches based on
popoverOpen:- const ref = useKeyboardShortcut(KEYBOARD_SHORTCUTS.confirm.keys, () => submitButtonRef.current?.click(), { - enableOnFormTags: true, - }); - - useKeyboardShortcut(KEYBOARD_SHORTCUTS.confirm.keys, () => handleSearchOnline(), { - enableOnFormTags: true, - enabled: popoverOpen, - }); + const ref = useKeyboardShortcut( + KEYBOARD_SHORTCUTS.confirm.keys, + () => { + if (popoverOpen) { + handleSearchOnline(); + } else { + submitButtonRef.current?.click(); + } + }, + { enableOnFormTags: true } + );
225-235: Use semantic button element for accessibilityThe clickable
<div>for "Search online" is not keyboard accessible. Replace it with a semantic<button>element to ensure proper keyboard navigation and screen reader support.- {searchValue ? ( - <CommandItem asChild> - <div onClick={handleSearchOnline} className="flex items-center justify-between gap-2"> - <div className="flex items-center gap-2"> - <Search className="h-3 w-3" /> - Search “{searchValue}” online - </div> - <ShortcutBadge shortcut="⌘ + Enter" /> - </div> - </CommandItem> - ) : null} + {searchValue ? ( + <CommandItem asChild> + <button + type="button" + onClick={handleSearchOnline} + className="flex w-full items-center justify-between gap-2 text-left" + aria-label={`Search "${searchValue}" online`} + > + <div className="flex items-center gap-2"> + <Search className="h-3 w-3" /> + Search “{searchValue}” online + </div> + <ShortcutBadge shortcut="⌘ + Enter" /> + </button> + </CommandItem> + ) : null}
232-232: Use platform-aware shortcut display helper instead of hardcodingThe hardcoded "⌘ + Enter" is Mac-specific. Windows/Linux users expect "Ctrl + Enter". Import and use the existing
getShortcutDisplayhelper to automatically handle platform differences.Add the import at the top of the file:
+import { getShortcutDisplay } from '@/utils/keyboardShortcuts';Then update both occurrences:
- <ShortcutBadge shortcut="⌘ + Enter" /> + <ShortcutBadge shortcut={getShortcutDisplay(KEYBOARD_SHORTCUTS.confirm.keys)} />- Confirm <ShortcutBadge shortcut="⌘ + Enter" /> + Confirm <ShortcutBadge shortcut={getShortcutDisplay(KEYBOARD_SHORTCUTS.confirm.keys)} />Also applies to: 313-313
🧹 Nitpick comments (6)
frontend/src/components/SelectLicenseList.tsx (5)
41-51: Avoid shadowing and improve clarity by renaming propThe prop
valueonLicenseItemis easy to confuse with the parent statevalue. Rename it to something explicit likeselectedId.Apply:
-function LicenseItem({ license, value }: { license: entities.License; value: string }) { +function LicenseItem({ license, selectedId }: { license: entities.License; selectedId: string }) { return ( <> - <Check className={cn('mr-2 h-4 w-4', license.licenseId === value ? 'opacity-100' : 'opacity-0')} /> + <Check className={cn('mr-2 h-4 w-4', license.licenseId === selectedId ? 'opacity-100' : 'opacity-0')} /> <div> <p>{license.licenseId}</p> <p className="text-xs text-muted-foreground">{license.name}</p> </div> </> ); }Note: If you adopt the direct type import (previous comment), replace
entities.Licenseaccordingly.
53-53: Consider de-duplication between “Matched” and “Catalogued”If a license appears in both lists, it will render twice. Filtering out matched IDs from the catalog improves UX.
Use this helper inside the component (outside the return) to compute a filtered list:
const matchedIds = new Set((matchedLicenses ?? []).map(l => l.licenseId)); const filteredLicenses = (licenses ?? []).filter(l => !matchedIds.has(l.licenseId));Then render
filteredLicensesin the Catalogued group (see suggested diff on Lines 102-106).
87-87: Constrain popover height to prevent viewport overflowWithout a scroll container, long lists can overflow. Add max height and scrolling.
-<PopoverContent className="min-w-[420px] p-0"> +<PopoverContent className="min-w-[420px] max-h-80 overflow-auto p-0">
95-97: UpdateLicenseItemprop name after renameIf you rename the prop in
LicenseItem, update the usages accordingly.- <LicenseItem license={license} value={value || ''} /> + <LicenseItem license={license} selectedId={value || ''} />
102-106: Use de-duplicated catalog list and updateLicenseItemprop nameRender the filtered list (see helper in earlier comment), and update the prop if you renamed it.
- {licenses?.map((license) => ( + {filteredLicenses?.map((license) => ( <CommandItem value={license.licenseId} key={license.licenseId} onSelect={handleSelect}> - <LicenseItem license={license} value={value || ''} /> + <LicenseItem license={license} selectedId={value || ''} /> </CommandItem> ))}Optionally add a loading state (non-blocking):
const { data: licenses, isLoading } = useQuery({ queryKey: ['licenses'], queryFn: GetAllLicenses, staleTime: 24 * 60 * 60 * 1000 });And show a hint:
<CommandEmpty>{isLoading ? 'Loading licenses…' : 'No licenses found.'}</CommandEmpty>frontend/src/components/ReplaceComponentDialog.tsx (1)
131-150: Consider handling edge cases in license matchingThe
getMatchedLicensesfunction handles errors well, but consider these improvements:
- The hardcoded SPDX URL template on line 140 assumes all licenses have SPDX identifiers. Some licenses might not be in SPDX.
- No loading state is shown while fetching licenses.
Consider making the reference URL more robust:
const matchedLicenses: entities.License[] = component.licenses.map((license) => ({ name: license.full_name, licenseId: license.id, - reference: `https://spdx.org/licenses/${license.id}.html`, // This is a workaround for the fact that the reference is not available in the response + reference: license.reference || `https://spdx.org/licenses/${license.id}.html`, }));For loading state, you could add a state variable and show a spinner/message while fetching.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/entities/license.go(1 hunks)frontend/src/components/OnlineComponentSearchDialog.tsx(1 hunks)frontend/src/components/ReplaceComponentDialog.tsx(10 hunks)frontend/src/components/SelectLicenseList.tsx(3 hunks)frontend/wailsjs/go/models.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- backend/entities/license.go
- frontend/src/components/OnlineComponentSearchDialog.tsx
- frontend/wailsjs/go/models.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
frontend/src/components/ReplaceComponentDialog.tsx (7)
frontend/wailsjs/go/models.ts (2)
DeclaredComponent(332-345)License(444-459)frontend/wailsjs/go/service/LicenseServiceImpl.js (1)
GetLicensesByPurl(9-11)frontend/src/hooks/useKeyboardShortcut.tsx (1)
useKeyboardShortcut(31-52)frontend/src/lib/shortcuts.ts (1)
KEYBOARD_SHORTCUTS(32-164)frontend/src/components/ShortcutBadge.tsx (1)
ShortcutBadge(1-3)frontend/src/components/NewComponentDialog.tsx (1)
NewComponentDialog(46-107)frontend/src/components/OnlineComponentSearchDialog.tsx (1)
OnlineComponentSearchDialog(92-223)
frontend/src/components/SelectLicenseList.tsx (2)
backend/entities/license.go (1)
License(31-35)frontend/wailsjs/go/models.ts (1)
License(444-459)
🔇 Additional comments (4)
frontend/src/components/SelectLicenseList.tsx (3)
37-39: Prop extension looks goodOptional
matchedLicensesis a sensible addition; keeps the component flexible for pre-filtered results.
92-100: Conditional rendering for Matched list looks goodOnly rendering the “Matched” group when there are results is clean and avoids empty headings.
30-30: No changes needed:entitiesnamespace correctly exported in Wails modelsVerified that
frontend/wailsjs/go/models.tsdefinesexport namespace entities { … export class License { … } }so
import { entities } from '../../wailsjs/go/models'and usage ofentities.Licenseare valid.frontend/src/components/ReplaceComponentDialog.tsx (1)
327-330: Good async error handling patternThe async handling in
onComponentSelectproperly awaits the license fetch after component selection, ensuring the matched licenses are loaded. The error handling is delegated to thegetMatchedLicensesfunction.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
frontend/src/components/ReplaceComponentDialog.tsx (3)
176-184: Fix duplicate keyboard shortcut binding conflict.There are two bindings for the same
KEYBOARD_SHORTCUTS.confirm.keyswhich will both fire whenpopoverOpenis true, causing unintended behavior.Remove the duplicate binding and consolidate into a single handler:
- const ref = useKeyboardShortcut(KEYBOARD_SHORTCUTS.confirm.keys, () => submitButtonRef.current?.click(), { - enableOnFormTags: true, - }); - - useKeyboardShortcut(KEYBOARD_SHORTCUTS.confirm.keys, () => handleSearchOnline(), { - enableOnFormTags: true, - enabled: popoverOpen, - }); + const ref = useKeyboardShortcut( + KEYBOARD_SHORTCUTS.confirm.keys, + () => { + if (popoverOpen) { + handleSearchOnline(); + } else { + submitButtonRef.current?.click(); + } + }, + { enableOnFormTags: true } + );
330-333: Consider clearing the search value after online component selection.After selecting a component from the online search, the
searchValuestate retains the old query which might be confusing when reopening the popover.onComponentSelect={async (c) => { onComponentSelected(c); await getMatchedLicenses(c.purl); + setSearchValue(''); }}
235-235: Use platform-aware shortcut display helper.The hardcoded "⌘ + Enter" won't display correctly on Windows/Linux where users expect "Ctrl + Enter".
Import and use the existing shortcut display helper:
+ import { getShortcutDisplay } from '@/utils/keyboardShortcuts'; // Line 235 - <ShortcutBadge shortcut="⌘ + Enter" /> + <ShortcutBadge shortcut={getShortcutDisplay(KEYBOARD_SHORTCUTS.confirm.keys)} /> // Line 316 - Confirm <ShortcutBadge shortcut="⌘ + Enter" /> + Confirm <ShortcutBadge shortcut={getShortcutDisplay(KEYBOARD_SHORTCUTS.confirm.keys)} />#!/bin/bash # Verify the existence of getShortcutDisplay helper fd -t f "keyboardShortcuts" --exec grep -l "getShortcutDisplay" # Check if there's already a utility for platform-aware shortcut display rg "getShortcutDisplay|formatShortcut|displayShortcut" -A 3Also applies to: 316-316
🧹 Nitpick comments (9)
frontend/src/components/Sidebar.tsx (1)
92-103: Consider consolidating navigation shortcuts configuration.The keyboard navigation blocking is correctly implemented. However, the Enter key shortcut (line 100-103) could benefit from being added to the KEYBOARD_SHORTCUTS constant for consistency with other shortcuts.
Consider adding the Enter key shortcut to the centralized KEYBOARD_SHORTCUTS configuration:
// In frontend/src/lib/shortcuts.ts export const KEYBOARD_SHORTCUTS: Record<entities.Action, KeyboardShortcut> = { // ... existing shortcuts ... // Navigation [entities.Action.MoveUp]: { /* ... */ }, [entities.Action.MoveDown]: { /* ... */ }, + [entities.Action.FocusResults]: { + name: 'Focus Results', + description: 'Move focus to results list', + keys: 'enter', + },Then use it in Sidebar:
- useKeyboardShortcut('enter', moveFocusToResults, { + useKeyboardShortcut(KEYBOARD_SHORTCUTS.focusResults.keys, moveFocusToResults, {frontend/src/components/ReplaceComponentDialog.tsx (2)
119-132: Consider handling the duplicate PURL case more gracefully.The current implementation shows a toast when a duplicate PURL exists but doesn't prevent the dialog from closing. This might confuse users.
Consider keeping the dialog open when a duplicate is detected:
const onComponentSelected = (component: entities.DeclaredComponent) => { const alreadyExists = declaredComponents.some((c) => c.purl === component.purl); if (alreadyExists) { - return toast({ + toast({ title: 'Warning', description: `A component with the same PURL already exists. Please select it from the list or enter a different PURL.`, }); + // Don't close the new component dialog on duplicate + return; } setDeclaredComponents((prevState) => [...prevState, component]); form.setValue('purl', component.purl); form.setValue('name', component.name); resetLicense(); setNewComponentDialogOpen(false); };
134-153: Add null check for component.licenses and improve error message.The function handles the case where component or licenses might be missing, but could provide more specific error information.
const getMatchedLicenses = async (purl: string) => { try { const { component } = await GetLicensesByPurl({ purl }); - if (!component || !component.licenses) return; + if (!component || !component.licenses) { + setMatchedLicenses([]); + return; + } const matchedLicenses: entities.License[] = component.licenses.map((license) => ({ name: license.full_name, licenseId: license.id, reference: `https://spdx.org/licenses/${license.id}.html`, // This is a workaround for the fact that the reference is not available in the response })); setMatchedLicenses(matchedLicenses); } catch (error) { + console.error('Failed to fetch licenses:', error); toast({ title: 'Error', - description: 'Error fetching matched licenses', + description: `Failed to fetch licenses for ${purl}`, variant: 'destructive', }); } };frontend/src/components/ui/keyboard-table.tsx (1)
59-59: UseNumber.MAX_SAFE_INTEGERinstead of magic number.Using a hardcoded large number is less clear than using the appropriate constant.
- move(99999); + move(Number.MAX_SAFE_INTEGER);frontend/src/components/OnlineComponentSearchDialog.tsx (5)
131-142: Fix effect dependencies and avoid stale closures in toastCompute the message inside the effect and include toast in deps to satisfy exhaustive-deps and avoid stale references.
Apply:
- useEffect(() => { - if (error) { - console.error('Error searching components:', error); - toast({ - title: 'Search Error', - description: errorMessage + '. Please try again.', - variant: 'destructive', - }); - } - }, [error]); + useEffect(() => { + if (error) { + const message = error instanceof Error ? error.message : 'Failed to search for components online'; + console.error('Error searching components:', error); + toast({ + title: 'Search Error', + description: message + '. Please try again.', + variant: 'destructive', + }); + } + }, [error, toast]);
145-146: Verify dialog control strategy (open is hardcoded true)Dialog is always open while mounted. Ensure the parent unmounts this component on onOpenChange(false) (Escape/overlay click), otherwise the dialog won’t close. If you want controlled behavior, expose an open prop and bind it to Dialog.
Example (requires API changes elsewhere):
-interface OnlineComponentSearchDialogProps { - onOpenChange: (open: boolean) => void; - searchTerm: string; - onComponentSelect: (component: entities.DeclaredComponent) => void; -} +interface OnlineComponentSearchDialogProps { + open: boolean; + onOpenChange: (open: boolean) => void; + searchTerm: string; + onComponentSelect: (component: entities.DeclaredComponent) => void; +} @@ -export default function OnlineComponentSearchDialog({ onOpenChange, searchTerm, onComponentSelect }: OnlineComponentSearchDialogProps) { +export default function OnlineComponentSearchDialog({ open, onOpenChange, searchTerm, onComponentSelect }: OnlineComponentSearchDialogProps) { @@ - <Dialog open onOpenChange={onOpenChange}> + <Dialog open={open} onOpenChange={onOpenChange}>
186-191: Show the actually queried term in the loading messageUsing currentSearch may mismatch the in-flight query (which uses debouncedSearchTerm).
Apply:
- <span className="ml-2 text-sm text-muted-foreground">Searching for “{currentSearch}”...</span> + <span className="ml-2 text-sm text-muted-foreground">Searching for “{debouncedSearchTerm}”...</span>
43-82: Centralize/validate catalogPackages against backendThe package list is hardcoded and may drift from what the backend/Scanoss supports. Consider:
- Sourcing the list from the backend (endpoint or bindings).
- Or centralizing it in a shared constants module used across components.
If you prefer a quick win, extract to a shared constants file:
// src/constants/catalogPackages.ts export const CATALOG_PACKAGES = [/* current list */] as const; export type CatalogPackage = typeof CATALOG_PACKAGES[number];Then import here and type selectedPackage as CatalogPackage to prevent invalid values at compile time.
100-115: Consider returning previous data during refetch to reduce UI flickerOptional: keep prior search results visible while a new query is in flight to avoid empty states during fast typing.
If you’re on TanStack Query v4:
- } = useQuery<entities.SearchedComponent[], Error>({ + } = useQuery<entities.SearchedComponent[], Error>({ @@ - retry: false, + retry: false, + keepPreviousData: true, });If on v5, use placeholderData: (previous) => previous instead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
frontend/src/components/OnlineComponentSearchDialog.tsx(1 hunks)frontend/src/components/ReplaceComponentDialog.tsx(10 hunks)frontend/src/components/Sidebar.tsx(3 hunks)frontend/src/components/ui/keyboard-table.tsx(1 hunks)frontend/src/contexts/DialogStateContext.tsx(1 hunks)frontend/src/main.tsx(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (5)
frontend/src/main.tsx (3)
frontend/src/contexts/DialogStateContext.tsx (1)
DialogStateProvider(39-75)frontend/src/providers/DialogProvider.tsx (1)
DialogProvider(46-122)frontend/src/components/ui/toaster.tsx (1)
Toaster(11-33)
frontend/src/components/Sidebar.tsx (3)
frontend/src/contexts/DialogStateContext.tsx (1)
useDialogState(77-83)frontend/src/hooks/useKeyboardShortcut.tsx (1)
useKeyboardShortcut(31-52)frontend/src/lib/shortcuts.ts (1)
KEYBOARD_SHORTCUTS(32-164)
frontend/src/components/ui/keyboard-table.tsx (1)
frontend/src/components/ui/table.tsx (6)
Table(112-112)TableHeader(113-113)TableRow(117-117)TableHead(116-116)TableBody(114-114)TableCell(118-118)
frontend/src/components/ReplaceComponentDialog.tsx (8)
frontend/wailsjs/go/models.ts (2)
DeclaredComponent(332-345)License(444-459)frontend/src/contexts/DialogStateContext.tsx (1)
useDialogRegistration(85-94)frontend/wailsjs/go/service/LicenseServiceImpl.js (1)
GetLicensesByPurl(9-11)frontend/src/lib/shortcuts.ts (1)
KEYBOARD_SHORTCUTS(32-164)frontend/src/components/ui/command.tsx (2)
CommandInput(146-146)CommandItem(150-150)frontend/src/components/ShortcutBadge.tsx (1)
ShortcutBadge(1-3)frontend/src/components/NewComponentDialog.tsx (1)
NewComponentDialog(46-107)frontend/src/components/OnlineComponentSearchDialog.tsx (1)
OnlineComponentSearchDialog(92-233)
frontend/src/components/OnlineComponentSearchDialog.tsx (11)
frontend/wailsjs/go/models.ts (3)
DeclaredComponent(332-345)ComponentSearchRequest(248-269)SearchedComponent(284-299)frontend/src/components/ui/use-toast.ts (2)
useToast(192-192)toast(192-192)frontend/src/contexts/DialogStateContext.tsx (1)
useDialogRegistration(85-94)backend/entities/component_search.go (2)
ComponentSearchRequest(27-34)SearchedComponent(37-41)frontend/wailsjs/go/service/ComponentServiceImpl.js (1)
SearchComponents(37-39)frontend/src/components/ui/dialog.tsx (4)
Dialog(77-77)DialogContent(77-77)DialogHeader(77-77)DialogTitle(77-77)frontend/src/components/ui/label.tsx (1)
Label(24-24)frontend/src/components/ui/input.tsx (1)
Input(25-25)frontend/src/components/ui/select.tsx (6)
Select(152-152)SelectTrigger(155-155)SelectValue(154-154)SelectContent(156-156)SelectGroup(153-153)SelectItem(158-158)frontend/src/components/ui/scroll-area.tsx (1)
ScrollArea(46-46)frontend/src/components/ui/keyboard-table.tsx (1)
KeyboardTable(21-124)
🔇 Additional comments (7)
frontend/src/contexts/DialogStateContext.tsx (1)
1-94: LGTM! Well-implemented dialog state management context.The implementation correctly manages dialog state with keyboard navigation blocking. The use of Map for tracking multiple dialogs and the cleanup via useEffect in the registration hook ensures proper lifecycle management.
frontend/src/main.tsx (1)
57-62: LGTM! Correct provider nesting order.The DialogStateProvider is correctly placed as the outermost provider wrapping DialogProvider, which allows DialogProvider and its child dialogs to use the dialog state context for keyboard navigation blocking.
frontend/src/components/Sidebar.tsx (1)
31-31: LGTM! Good integration with dialog state context.The component correctly imports and uses the dialog state context to control keyboard navigation.
Also applies to: 50-50
frontend/src/components/ReplaceComponentDialog.tsx (2)
83-83: LGTM! Proper dialog registration for keyboard navigation blocking.The dialog correctly registers itself with keyboard navigation blocking enabled.
228-238: LGTM! Good UX improvement with inline search action.The inline search functionality provides a smooth user experience by allowing direct online search from the input field.
frontend/src/components/ui/keyboard-table.tsx (1)
21-28: LGTM! Well-structured generic table component with keyboard navigation.The component provides excellent keyboard navigation support with proper TypeScript generics and flexible column rendering.
frontend/src/components/OnlineComponentSearchDialog.tsx (1)
122-129: Mapping SearchedComponent to DeclaredComponent looks goodThe structural mapping (component -> name, purl passthrough) is correct and aligns with models.ts definitions.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (9)
.github/workflows/test.yml (1)
36-41: Broken env var usage when passing APPARGS to wails buildShell syntax is wrong: APPARGS must be set in the environment before the command, not after it. As written, APPARGS becomes a CLI argument to wails, which will be ignored or cause errors.
Fix:
- wails build APPARGS="--key test" + APPARGS="--key test" wails buildAlternatively:
export APPARGS="--key test" wails build.github/workflows/release.yml (7)
168-176: APP_VERSION is undefined in this step (commit message becomes blank version)The variable is computed in the previous step only. Each GitHub Actions step runs in a new process; APP_VERSION won’t persist here.
Apply this diff to recompute APP_VERSION before using it:
cd homebrew-dist git config user.name "SCANOSS Bot" git config user.email "[email protected]" + APP_VERSION=$(git tag --sort=-version:refname | head -n 1) git add Casks/scanoss-code-compare.rb git commit -m "Update scanoss-code-compare to ${APP_VERSION}" git push
25-27: Ensure tags are available for git tag lookups (AppVersion ldflags, Homebrew bump)checkout@v4 defaults to shallow clones without tags; git tag may return nothing on runners. Fetch tags explicitly.
Apply this diff:
- name: Checkout uses: actions/checkout@v4 with: submodules: recursive + fetch-depth: 0 + fetch-tags: trueRepeat for Linux and Windows jobs (see corresponding comments).
189-192: Linux job: fetch tags for version embeddingSame rationale as macOS job: wails build reads AppVersion from git tags.
- name: Checkout uses: actions/checkout@v4 with: submodules: recursive + fetch-depth: 0 + fetch-tags: true
255-258: Windows job: fetch tags for version embeddingEnsure tags are present before calling git tag in the build step.
- name: Checkout uses: actions/checkout@v4 with: submodules: recursive + fetch-depth: 0 + fetch-tags: true
208-211: Pin Wails version on Linux for reproducible builds (avoid @latest drift)Using @latest can break unexpectedly, especially after a Go toolchain bump. Align with macOS’ pinned version or centralize in env.
- - name: Install Wails - run: go install github.com/wailsapp/wails/v2/cmd/wails@latest + - name: Install Wails + run: go install github.com/wailsapp/wails/v2/cmd/[email protected] shell: bashIf macOS version changes, keep all three jobs in sync.
275-277: Pin Wails version on Windows for reproducible buildsMirror the Linux/macOS recommendation.
- - name: Install Wails - run: go install github.com/wailsapp/wails/v2/cmd/wails@latest + - name: Install Wails + run: go install github.com/wailsapp/wails/v2/cmd/[email protected] shell: bash
317-317: Pin CodeSignTool action to a stable tag
Based on the repository’s tags, the latest stable release is v1.3.1. Pinning to this exact tag ensures reproducibility and supply-chain integrity.• File: .github/workflows/release.yml (line 317)
- uses: sslcom/esigner-codesign@develop + uses: sslcom/[email protected]frontend/src/components/ReplaceComponentDialog.tsx (1)
220-230: Use semantic buttons instead of clickable divs inside CommandItem for a11y.Clickable divs are not focusable and hurt keyboard/screen reader accessibility. Since CommandItem supports asChild, render buttons.
Apply this diff:
- <CommandItem asChild> - <div - onClick={() => { - setNewComponentDialogOpen(true); - setPopoverOpen(false); - }} - > - <Plus className="mr-2 h-4 w-4" /> - Add new component - </div> - </CommandItem> + <CommandItem asChild> + <button + type="button" + onClick={() => { + setNewComponentDialogOpen(true); + setPopoverOpen(false); + }} + className="flex w-full items-center gap-2 text-left" + aria-label="Add new component" + > + <Plus className="mr-2 h-4 w-4" /> + Add new component + </button> + </CommandItem>- {searchValue ? ( - <CommandItem asChild> - <div onClick={handleSearchOnline} className="flex items-center justify-between gap-2"> - <div className="flex items-center gap-2"> - <Search className="h-3 w-3" /> - Search “{searchValue}” online - </div> - <ShortcutBadge shortcut={getShortcutDisplay(KEYBOARD_SHORTCUTS.confirm.keys, modifierKey.label)[0]} /> - </div> - </CommandItem> - ) : null} + {searchValue ? ( + <CommandItem asChild> + <button + type="button" + onClick={handleSearchOnline} + className="flex w-full items-center justify-between gap-2" + aria-label={`Search "${searchValue}" online`} + > + <div className="flex items-center gap-2"> + <Search className="h-3 w-3" /> + Search “{searchValue}” online + </div> + <ShortcutBadge shortcut={getShortcutDisplay(KEYBOARD_SHORTCUTS.confirm.keys, modifierKey.label)[0]} /> + </button> + </CommandItem> + ) : null}Also applies to: 231-241
♻️ Duplicate comments (2)
frontend/src/components/ReplaceComponentDialog.tsx (2)
179-187: Fix duplicate confirm shortcut binding (both handlers fire when popover is open).You bind KEYBOARD_SHORTCUTS.confirm twice; the submit handler is always enabled, and the second handler is enabled when popoverOpen is true. Result: pressing the shortcut while the popover is open can both submit and open online search.
Unify into a single binding that branches on popoverOpen and remove the second hook.
Apply this diff:
- const ref = useKeyboardShortcut(KEYBOARD_SHORTCUTS.confirm.keys, () => submitButtonRef.current?.click(), { - enableOnFormTags: true, - }); - - useKeyboardShortcut(KEYBOARD_SHORTCUTS.confirm.keys, () => handleSearchOnline(), { - enableOnFormTags: true, - enabled: popoverOpen, - }); + const ref = useKeyboardShortcut( + KEYBOARD_SHORTCUTS.confirm.keys, + () => { + if (popoverOpen) handleSearchOnline(); + else submitButtonRef.current?.click(); + }, + { enableOnFormTags: true } + );
122-135: Clear inline search input after selection and refresh license matches for existing components.The search input retains stale text after selecting a component. Also, license matches are fetched only for online selections; selecting an existing component can show stale matches. Clear the input and refresh matches on all selection paths.
Apply this diff:
const onComponentSelected = (component: entities.DeclaredComponent) => { const alreadyExists = declaredComponents.some((c) => c.purl === component.purl); if (alreadyExists) { return toast({ title: 'Warning', description: `A component with the same PURL already exists. Please select it from the list or enter a different PURL.`, }); } setDeclaredComponents((prevState) => [...prevState, component]); form.setValue('purl', component.purl); form.setValue('name', component.name); resetLicense(); setNewComponentDialogOpen(false); + setSearchValue(''); };onSelect={() => { form.setValue('purl', component.purl); form.setValue('name', component.name); resetLicense(); setPopoverOpen(false); + setSearchValue(''); + // Refresh matched licenses for existing components too + void getMatchedLicenses(component.purl); }}onComponentSelect={async (c) => { onComponentSelected(c); await getMatchedLicenses(c.purl); + setSearchValue(''); }}Also applies to: 249-254, 333-336
🧹 Nitpick comments (3)
.github/workflows/test.yml (2)
14-14: GOLANG_VERSION env is unused; either wire it into setup-go or remove itYou’re using actions/setup-go with go-version-file, so this env var doesn’t affect the toolchain. Keeping it risks drift/confusion.
Apply one of the following:
Option A — remove the unused env:
- GOLANG_VERSION: "1.23"Option B — use it explicitly (then drop go-version-file):
# Outside this range: in both setup-go steps, replace: # with: # go-version-file: go.mod # with: with: go-version: ${{ env.GOLANG_VERSION }}Please verify go.mod also targets 1.23 to avoid mismatches.
98-102: LGTM; consider aligning action versions in this job for consistencyIntegration tests look fine, SCANOSS_API_KEY is correctly sourced. Minor consistency nit: elsewhere you use actions/checkout@v4 and actions/setup-go@v5; here you’re on @V3 and @v4 respectively. Consider upgrading for uniformity and latest fixes.
Suggested updates outside this range:
- uses: actions/checkout@v3 + uses: actions/checkout@v4 - uses: actions/setup-go@v4 + uses: actions/setup-go@v5 - uses: actions/setup-python@v4 + uses: actions/setup-python@v5.github/workflows/release.yml (1)
134-135: Use xcrun for stapler validate for consistencyYou already use xcrun stapler staple; do the same for validate to avoid PATH issues on macOS runners.
- stapler validate "$DMG_PATH" + xcrun stapler validate "$DMG_PATH"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/release.yml(1 hunks).github/workflows/test.yml(3 hunks)frontend/src/components/OnlineComponentSearchDialog.tsx(1 hunks)frontend/src/components/ReplaceComponentDialog.tsx(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/components/OnlineComponentSearchDialog.tsx
🧰 Additional context used
🧬 Code Graph Analysis (1)
frontend/src/components/ReplaceComponentDialog.tsx (13)
frontend/wailsjs/go/models.ts (2)
DeclaredComponent(332-345)License(444-459)frontend/src/hooks/useEnvironment.tsx (1)
useEnvironment(37-66)frontend/src/contexts/DialogStateContext.tsx (1)
useDialogRegistration(85-94)frontend/wailsjs/go/service/LicenseServiceImpl.js (1)
GetLicensesByPurl(9-11)frontend/src/lib/shortcuts.ts (1)
getShortcutDisplay(166-180)frontend/src/components/ui/popover.tsx (1)
Popover(31-31)frontend/src/components/ui/command.tsx (2)
CommandInput(146-146)CommandItem(150-150)frontend/src/components/ShortcutBadge.tsx (1)
ShortcutBadge(1-3)frontend/src/components/SelectLicenseList.tsx (1)
SelectLicenseList(53-113)frontend/src/components/ui/dialog.tsx (3)
DialogFooter(77-77)DialogContent(77-77)Dialog(77-77)frontend/src/components/ui/form.tsx (1)
Form(169-169)frontend/src/components/NewComponentDialog.tsx (1)
NewComponentDialog(46-107)frontend/src/components/OnlineComponentSearchDialog.tsx (1)
OnlineComponentSearchDialog(91-234)
🔇 Additional comments (5)
.github/workflows/release.yml (1)
15-15: Go toolchain bump to 1.23: validate cross-job compatibilityActions/setup-go@v5 supports 1.23, but please ensure:
- go.mod declares go 1.23 (or toolchain go1.23).
- Wails version(s) used build cleanly on 1.23 (see related suggestions below).
frontend/src/components/ReplaceComponentDialog.tsx (4)
33-34: Good: platform-aware shortcut labels via centralized helper.Importing getShortcutDisplay and using modifierKey from useEnvironment to render the correct confirm shortcut is clean and consistent with the rest of the app.
Also applies to: 84-85, 238-239, 319-319
205-205: Good: Popover uses modal for proper focus management.Enabling modal improves keyboard and focus trapping inside the combobox popover.
122-122: Type alignment with search dialog looks correct.Using entities.DeclaredComponent for onComponentSelected matches OnlineComponentSearchDialog’s onComponentSelect contract.
283-284: LGTM: matchedLicenses plumbed into SelectLicenseList.Prop wiring aligns with the new “Matched” section in the license picker.
matiasdaloia
force-pushed
the
feature/mdaloia/sp-2882-add-component-search
branch
from
2cad7ee to
7901638
Compare
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (23)
internal/utils/url_utils.go (4)
12-18: Handle scheme case-insensitively (RFC 3986).URL schemes are case-insensitive; “HTTP://example.com” won’t be stripped currently.
Apply this diff:
- if after, ok := strings.CutPrefix(url, "http://"); ok { - return after - } - if after, ok := strings.CutPrefix(url, "https://"); ok { - return after - } + lu := strings.ToLower(url) + if strings.HasPrefix(lu, "http://") { + return url[len("http://"):] + } + if strings.HasPrefix(lu, "https://") { + return url[len("https://"):] + }
5-7: Name vs. behavior mismatch (optional).Function name suggests removing any URL scheme; implementation/docs only handle HTTP/HTTPS. Either:
- keep name and generalize to strip any scheme ending with "://", or
- keep behavior and rename (e.g., RemoveHTTPSchemeFromURL) for precision.
Given the PR scope, consider deferring to a follow-up.
8-11: Minor: early-return is fine but not required.CutPrefix/HasPrefix guards already cover empty input. Keeping the early return is okay for clarity; can be removed to reduce branching if preferred.
5-19: Add table-driven tests for edge cases.Recommend tests covering: empty string, http, https, uppercase scheme variants, no scheme, and path/query fragments.
Example (utils/url_utils_test.go):
package utils import "testing" func TestRemoveProtocolFromURL(t *testing.T) { cases := []struct{ in, out string }{ {"", ""}, {"http://example.com", "example.com"}, {"https://example.com/path?x=1#frag", "example.com/path?x=1#frag"}, {"HTTP://EXAMPLE.COM", "EXAMPLE.COM"}, {"Https://Mixed.Case/Path", "Mixed.Case/Path"}, {"ftp://example.com", "ftp://example.com"}, {"example.com", "example.com"}, } for _, c := range cases { if got := RemoveProtocolFromURL(c.in); got != c.out { t.Fatalf("input %q => got %q, want %q", c.in, got, c.out) } } }Happy to commit these in this PR if you want.
backend/service/scanoss_api_service_http_impl.go (4)
127-133: Optional: include package/offset in debug logsHelps troubleshoot query shape.
log.Debug(). Str("search", request.Search). Str("vendor", request.Vendor). Str("component", request.Component). + Str("package", request.Package). + Int32("offset", request.Offset). Int32("limit", request.Limit). Msg("Searching components via SCANOSS API")
171-172: Fix doc comment to match method nameMinor naming mismatch.
-// GetLicenseByPurl fetches licenses for a given purl/component combination using the SCANOSS HTTP API +// GetLicensesByPurl fetches licenses for a given purl using the SCANOSS HTTP API
190-194: Fix log message: wrong API name in GetLicensesByPurl error pathMessage says “component search API”.
- log.Error().Err(err).Msg("Error calling SCANOSS component search API") + log.Error().Err(err).Msg("Error calling SCANOSS licenses API")
80-96: Optional: robust URL joiningAvoid double slashes by using url.JoinPath if using Go >=1.20; otherwise trim slashes.
Example:
- u, err := url.Parse(s.baseURL + endpoint) + uStr, err := url.JoinPath(s.baseURL, endpoint) + if err != nil { + return "", err + } + u, err := url.Parse(uStr)backend/service/license_service_impl.go (1)
58-60: Defensive: guard against nil scanossApiServicePrevent runtime panic if dependency wiring fails.
func (s *LicenseServiceImpl) GetLicensesByPurl(request entities.ComponentRequest) (entities.GetLicensesByPurlResponse, error) { - return s.scanossApiService.GetLicensesByPurl(request) + if s.scanossApiService == nil { + return entities.GetLicensesByPurlResponse{}, fmt.Errorf("scanossApiService not configured") + } + return s.scanossApiService.GetLicensesByPurl(request) }Note: add fmt import.
backend/service/scanoss_api_service.go (1)
32-36: Design: prefer per-call context over stored stateConsider passing context.Context as a parameter to each method instead of SetContext to avoid shared mutable state and ease cancellation per call.
frontend/src/components/ui/keyboard-table.tsx (5)
29-33: Clamp focusedIndex on init and when rows changePrevents invalid aria references and out-of-range indices when data updates.
const containerRef = useRef<HTMLDivElement>(null); - const [focusedIndex, setFocusedIndex] = useState<number>(initialFocusedIndex); + const [focusedIndex, setFocusedIndex] = useState<number>(() => + rows.length ? Math.max(0, Math.min(initialFocusedIndex, rows.length - 1)) : -1 + ); + + useEffect(() => { + setFocusedIndex((prev) => { + const last = rows.length - 1; + if (last < 0) return -1; + const next = prev < 0 ? 0 : prev; + return Math.max(0, Math.min(next, last)); + }); + }, [rows.length]);
32-37: Use data length instead of querying the DOMSimpler and avoids layout thrash from DOM queries.
const move = (next: number) => { - const count = containerRef.current?.querySelectorAll<HTMLElement>('[data-row-index]').length ?? 0; + const count = rows.length; if (!count) return; const last = count - 1; setFocusedIndex(Math.max(0, Math.min(last, next))); };
87-87: Avoid mixing ARIA grid roles with native table semanticsThe children are native table elements (tr/td). Using role="grid" + aria-activedescendant with table semantics is inconsistent. Prefer native table semantics here.
- <div ref={containerRef} tabIndex={0} role="grid" aria-activedescendant={`row-${focusedIndex}`} onKeyDown={onKeyDown}> + <div ref={containerRef} tabIndex={0} onKeyDown={onKeyDown}>If you want full grid semantics instead, cells/headers should use grid roles (gridcell/row/columnheader) across the Table primitives.
103-111: Expose selection state to assistive techMark the focused row as selected for better SR feedback.
data-row-index={i} + aria-selected={isFocused}
99-101: Row styling nits: ring/rounded on and className override
- rounded/ring utilities often don’t render on tr; consider moving focus styles to cells (e.g., apply to first/last cell) or use outline on the row wrapper.
- Consider merging default and custom row class names to preserve focus styling.
- const customClassName = rowClassName ? rowClassName(row, i, isFocused) : defaultClassName; + const customClassName = clsx(defaultClassName, rowClassName?.(row, i, isFocused));frontend/src/components/SelectLicenseList.tsx (4)
61-66: Fix stale onSelect reference in callback depsInclude onSelect in the dependency array to avoid stale closures.
- const handleSelect = useCallback((value: string) => { + const handleSelect = useCallback((value: string) => { setValue(value); onSelect(value); setPopoverOpen(false); - }, []); + }, [onSelect]);
67-70: Fix stale onSelect reference in callback depsSame here: add onSelect to dependencies.
- const handleClearSelection = useCallback(() => { + const handleClearSelection = useCallback(() => { setValue(undefined); onSelect(undefined); - }, []); + }, [onSelect]);
82-84: Add accessible name to Clear icon buttonImprove a11y with an aria-label.
- <Button size="icon" variant="ghost" onClick={handleClearSelection}> + <Button size="icon" variant="ghost" onClick={handleClearSelection} aria-label="Clear selection"> <X className="h-4 w-4" /> </Button>
101-106: De-duplicate matched items from the catalogued listAvoid showing the same license twice if it appears in matchedLicenses.
- {licenses?.map((license) => ( + {licenses + ?.filter((license) => !matchedLicenses?.some((m) => m.licenseId === license.licenseId)) + .map((license) => ( <CommandItem value={license.licenseId} key={license.licenseId} onSelect={handleSelect}> <LicenseItem license={license} value={value || ''} /> </CommandItem> - ))} + ))}frontend/src/components/ReplaceComponentDialog.tsx (4)
221-229: Use a semantic button for “Add new component” (a11y)Clickable divs aren’t accessible. Use a button.
- <CommandItem asChild> - <div - onClick={() => { - setNewComponentDialogOpen(true); - setPopoverOpen(false); - }} - > - <Plus className="mr-2 h-4 w-4" /> - Add new component - </div> - </CommandItem> + <CommandItem asChild> + <button + type="button" + className="flex w-full items-center" + onClick={() => { + setNewComponentDialogOpen(true); + setPopoverOpen(false); + }} + > + <Plus className="mr-2 h-4 w-4" /> + Add new component + </button> + </CommandItem>
122-135: Clear matchedLicenses when selecting a declared componentAvoid carrying matches from a previous selection.
form.setValue('name', component.name); resetLicense(); + setMatchedLicenses([]); setNewComponentDialogOpen(false);
249-254: Also clear matches when picking from the listEnsure matchedLicenses doesn’t reflect a prior selection.
form.setValue('name', component.name); resetLicense(); + setMatchedLicenses([]); setPopoverOpen(false);
334-336: Optional: clear the inline search input after online selectionPrevents stale text when the popover is reopened.
onComponentSelect={async (c) => { onComponentSelected(c); await getMatchedLicenses(c.purl); + setSearchValue(''); }}
📜 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 (34)
.github/workflows/release.yml(1 hunks).github/workflows/test.yml(3 hunks)backend/entities/common.go(1 hunks)backend/entities/component_search.go(1 hunks)backend/entities/license.go(1 hunks)backend/service/component_service.go(2 hunks)backend/service/component_service_impl.go(2 hunks)backend/service/component_service_test.go(0 hunks)backend/service/license_service.go(1 hunks)backend/service/license_service_impl.go(2 hunks)backend/service/scanoss_api_service.go(1 hunks)backend/service/scanoss_api_service_http_impl.go(1 hunks)frontend/package.json(1 hunks)frontend/package.json.md5(1 hunks)frontend/src/components/OnlineComponentSearchDialog.tsx(1 hunks)frontend/src/components/ReplaceComponentDialog.tsx(10 hunks)frontend/src/components/SelectLicenseList.tsx(3 hunks)frontend/src/components/ShortcutBadge.tsx(1 hunks)frontend/src/components/Sidebar.tsx(3 hunks)frontend/src/components/ui/dialog.tsx(0 hunks)frontend/src/components/ui/keyboard-table.tsx(1 hunks)frontend/src/components/ui/popover.tsx(2 hunks)frontend/src/components/ui/select.tsx(1 hunks)frontend/src/components/ui/table.tsx(1 hunks)frontend/src/contexts/DialogStateContext.tsx(1 hunks)frontend/src/main.tsx(2 hunks)frontend/wailsjs/go/models.ts(5 hunks)frontend/wailsjs/go/service/ComponentServiceImpl.d.ts(1 hunks)frontend/wailsjs/go/service/ComponentServiceImpl.js(1 hunks)frontend/wailsjs/go/service/LicenseServiceImpl.d.ts(1 hunks)frontend/wailsjs/go/service/LicenseServiceImpl.js(1 hunks)go.mod(2 hunks)internal/utils/url_utils.go(1 hunks)main.go(2 hunks)
💤 Files with no reviewable changes (2)
- backend/service/component_service_test.go
- frontend/src/components/ui/dialog.tsx
✅ Files skipped from review due to trivial changes (1)
- frontend/package.json.md5
🚧 Files skipped from review as they are similar to previous changes (23)
- backend/service/license_service.go
- backend/entities/common.go
- frontend/src/components/OnlineComponentSearchDialog.tsx
- frontend/wailsjs/go/service/ComponentServiceImpl.js
- backend/entities/license.go
- frontend/src/components/Sidebar.tsx
- frontend/src/components/ui/table.tsx
- frontend/src/main.tsx
- .github/workflows/test.yml
- backend/entities/component_search.go
- frontend/wailsjs/go/service/LicenseServiceImpl.d.ts
- backend/service/component_service_impl.go
- frontend/src/components/ShortcutBadge.tsx
- frontend/wailsjs/go/models.ts
- frontend/src/contexts/DialogStateContext.tsx
- frontend/src/components/ui/popover.tsx
- frontend/wailsjs/go/service/LicenseServiceImpl.js
- go.mod
- main.go
- frontend/package.json
- frontend/src/components/ui/select.tsx
- .github/workflows/release.yml
- frontend/wailsjs/go/service/ComponentServiceImpl.d.ts
🧰 Additional context used
🧬 Code graph analysis (7)
backend/service/scanoss_api_service.go (3)
backend/entities/component_search.go (2)
ComponentSearchRequest(27-34)ComponentSearchResponse(44-47)backend/entities/common.go (1)
ComponentRequest(3-6)backend/entities/license.go (1)
GetLicensesByPurlResponse(50-53)
backend/service/component_service.go (3)
frontend/wailsjs/go/service/ComponentServiceImpl.js (1)
SearchComponents(37-39)frontend/wailsjs/go/models.ts (2)
ComponentSearchRequest(248-269)ComponentSearchResponse(300-331)backend/entities/component_search.go (2)
ComponentSearchRequest(27-34)ComponentSearchResponse(44-47)
frontend/src/components/SelectLicenseList.tsx (2)
backend/entities/license.go (1)
License(31-35)frontend/wailsjs/go/models.ts (1)
License(444-459)
backend/service/license_service_impl.go (6)
backend/service/scanoss_api_service.go (1)
ScanossApiService(32-36)backend/service/license_service.go (1)
LicenseService(28-31)frontend/wailsjs/go/service/LicenseServiceImpl.js (1)
GetLicensesByPurl(9-11)frontend/wailsjs/go/models.ts (2)
ComponentRequest(234-247)GetLicensesByPurlResponse(378-409)backend/entities/common.go (1)
ComponentRequest(3-6)backend/entities/license.go (1)
GetLicensesByPurlResponse(50-53)
frontend/src/components/ReplaceComponentDialog.tsx (9)
frontend/wailsjs/go/models.ts (2)
DeclaredComponent(332-345)License(444-459)frontend/src/hooks/useEnvironment.tsx (1)
useEnvironment(37-66)frontend/src/contexts/DialogStateContext.tsx (1)
useDialogRegistration(85-94)frontend/wailsjs/go/service/LicenseServiceImpl.js (1)
GetLicensesByPurl(9-11)frontend/src/hooks/useKeyboardShortcut.tsx (1)
useKeyboardShortcut(31-52)frontend/src/lib/shortcuts.ts (1)
getShortcutDisplay(166-180)frontend/src/components/ShortcutBadge.tsx (1)
ShortcutBadge(1-3)frontend/src/components/SelectLicenseList.tsx (1)
SelectLicenseList(53-113)frontend/src/components/OnlineComponentSearchDialog.tsx (1)
OnlineComponentSearchDialog(91-234)
backend/service/scanoss_api_service_http_impl.go (6)
backend/service/scanoss_api_service.go (1)
ScanossApiService(32-36)internal/config/config.go (2)
GetInstance(111-116)SCANOSS_PREMIUM_API_URL(55-55)backend/entities/component_search.go (2)
ComponentSearchRequest(27-34)ComponentSearchResponse(44-47)internal/utils/custom_validations.go (1)
GetValidator(37-39)backend/entities/common.go (1)
ComponentRequest(3-6)backend/entities/license.go (1)
GetLicensesByPurlResponse(50-53)
frontend/src/components/ui/keyboard-table.tsx (1)
frontend/src/components/ui/table.tsx (6)
Table(112-112)TableHeader(113-113)TableRow(117-117)TableHead(116-116)TableBody(114-114)TableCell(118-118)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (10)
internal/utils/url_utils.go (1)
5-19: Approved — strings.CutPrefix is safe (repo targets Go >= 1.23)
go.mod declares "go 1.23.0" and "toolchain go1.23.2", so strings.CutPrefix (added in Go 1.20) is supported; no change required.backend/service/license_service_impl.go (1)
38-42: New constructor parameter already propagated — no action requiredCall site main.go:101 passes scanossApiService; no other usages of NewLicenseServiceImpl were found.
backend/service/scanoss_api_service_http_impl.go (1)
55-74: Verified — SetContext is invoked by callers (nil-context risk mitigated).
main.go:114–116: scanService.SetContext(ctx), resultService.SetContext(ctx), scanossApiService.SetContext(ctx)backend/service/component_service.go (1)
28-37: Interface change: confirm all implementers updated to include SearchComponentsAdding SearchComponents to ComponentService is a breaking change — ensure every concrete implementer (e.g., ComponentServiceImpl) adds this method or provide a compatibility adapter. Repository search returned no matches for SearchComponents; manually verify implementations and update tests/call sites.
frontend/src/components/ui/keyboard-table.tsx (1)
106-106: Remove redundant role="row" on TableRow- role="row"frontend/src/components/SelectLicenseList.tsx (1)
73-85: Modal popover blocks the Clear button interactionThis was flagged earlier; using modal blocks outside clicks, making Clear unusable while open. Remove modal or move Clear inside PopoverContent.
- <Popover open={popoverOpen} onOpenChange={setPopoverOpen} modal> + <Popover open={popoverOpen} onOpenChange={setPopoverOpen}>frontend/src/components/ReplaceComponentDialog.tsx (4)
179-186: Conflicting confirm shortcut: double-bound handlers fire togetherBoth hooks bind the same keys; when popoverOpen is true, submit and online search can trigger simultaneously. Use a single binding that branches.
- const ref = useKeyboardShortcut(KEYBOARD_SHORTCUTS.confirm.keys, () => submitButtonRef.current?.click(), { - enableOnFormTags: true, - }); - - useKeyboardShortcut(KEYBOARD_SHORTCUTS.confirm.keys, () => handleSearchOnline(), { - enableOnFormTags: true, - enabled: popoverOpen, - }); + const ref = useKeyboardShortcut( + KEYBOARD_SHORTCUTS.confirm.keys, + () => { + if (popoverOpen) handleSearchOnline(); + else submitButtonRef.current?.click(); + }, + { enableOnFormTags: true }, + [popoverOpen] + );
231-241: Use a semantic button for “Search online” (a11y)Replace the clickable div with a button and full-width hit area.
- {searchValue ? ( - <CommandItem asChild> - <div onClick={handleSearchOnline} className="flex items-center justify-between gap-2"> - <div className="flex items-center gap-2"> - <Search className="h-3 w-3" /> - Search “{searchValue}” online - </div> - <ShortcutBadge shortcut={getShortcutDisplay(KEYBOARD_SHORTCUTS.confirm.keys, modifierKey.label)[0]} /> - </div> - </CommandItem> - ) : null} + {searchValue ? ( + <CommandItem asChild> + <button + type="button" + onClick={handleSearchOnline} + className="flex w-full items-center justify-between gap-2" + aria-label={`Search "${searchValue}" online`} + > + <div className="flex items-center gap-2"> + <Search className="h-3 w-3" /> + Search “{searchValue}” online + </div> + <ShortcutBadge shortcut={getShortcutDisplay(KEYBOARD_SHORTCUTS.confirm.keys, modifierKey.label)[0]} /> + </button> + </CommandItem> + ) : null}
137-156: Clear matchedLicenses to prevent stale results (start/empty/error)Reset before fetch, when no results, and on error to avoid showing old matches.
- const getMatchedLicenses = async (purl: string) => { - try { - const { component } = await GetLicensesByPurl({ purl }); - - if (!component || !component.licenses) return; - - const matchedLicenses: entities.License[] = component.licenses.map((license) => ({ - name: license.full_name, - licenseId: license.id, - reference: `https://spdx.org/licenses/${license.id}.html`, // This is a workaround for the fact that the reference is not available in the response - })); - setMatchedLicenses(matchedLicenses); - } catch (error) { - toast({ - title: 'Error', - description: 'Error fetching matched licenses', - variant: 'destructive', - }); - } - }; + const getMatchedLicenses = async (purl: string) => { + // Reset before fetching to avoid showing stale results + setMatchedLicenses([]); + try { + const { component } = await GetLicensesByPurl({ purl }); + if (!component || !component.licenses || component.licenses.length === 0) { + setMatchedLicenses([]); + return; + } + const licenses: entities.License[] = component.licenses.map((license) => ({ + name: license.full_name, + licenseId: license.id, + reference: `https://spdx.org/licenses/${license.id}.html`, + })); + setMatchedLicenses(licenses); + } catch (error) { + setMatchedLicenses([]); + toast({ + title: 'Error', + description: 'Error fetching matched licenses', + variant: 'destructive', + }); + } + };
205-215: Verify intent for Popover modal inside DialogWith modal, Cancel/Confirm and other outside controls are non-interactive while open. Confirm this UX; if not intended, drop modal.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/components/ReplaceComponentDialog.tsx (1)
122-135: Clear matchedLicenses when switching components to avoid stale suggestionsIf a user selects a declared/new component, previous online “Matched” licenses persist. Clear them on selection.
const onComponentSelected = (component: entities.DeclaredComponent) => { @@ form.setValue('name', component.name); resetLicense(); + setMatchedLicenses([]); // avoid stale matched list when switching sources setNewComponentDialogOpen(false); };onSelect={() => { form.setValue('purl', component.purl); form.setValue('name', component.name); resetLicense(); + setMatchedLicenses([]); setPopoverOpen(false); }}Also applies to: 251-260
🧹 Nitpick comments (3)
frontend/src/components/ReplaceComponentDialog.tsx (2)
225-235: Use a semantic button for “Add new component”For a11y/semantics, avoid clickable divs.
- <CommandItem asChild> - <div - onClick={() => { - setNewComponentDialogOpen(true); - setPopoverOpen(false); - }} - > - <Plus className="mr-2 h-4 w-4" /> - Add new component - </div> - </CommandItem> + <CommandItem asChild> + <button + type="button" + onClick={() => { + setNewComponentDialogOpen(true); + setPopoverOpen(false); + }} + className="flex w-full items-center" + > + <Plus className="mr-2 h-4 w-4" /> + Add new component + </button> + </CommandItem>
98-104: Optional: auto-fetch matched licenses when purl changesKeeps “Matched” section in sync for any source of selection (declared/new/online).
Add after selectedPurl is defined:
useEffect(() => { if (!selectedPurl) { setMatchedLicenses([]); return; } getMatchedLicenses(selectedPurl); // eslint-disable-next-line react-hooks/exhaustive-deps }, [selectedPurl]);backend/service/license_service_impl.go (1)
41-45: Fail‑fast nil check in NewLicenseServiceImplVerified: main.go (line 101) constructs scanossApiService via NewScanossApiServiceHttpImpl() and passes it to NewLicenseServiceImpl; no explicit nil injections found. Still add a defensive nil-check in NewLicenseServiceImpl to surface wiring errors early — e.g. if repo == nil || scanossApiService == nil { panic("NewLicenseServiceImpl: nil dependency") }.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/release.yml(1 hunks)backend/service/license_service_impl.go(2 hunks)frontend/src/components/ReplaceComponentDialog.tsx(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/release.yml
🧰 Additional context used
🧬 Code graph analysis (2)
frontend/src/components/ReplaceComponentDialog.tsx (8)
frontend/wailsjs/go/models.ts (2)
DeclaredComponent(332-345)License(444-459)frontend/src/hooks/useEnvironment.tsx (1)
useEnvironment(37-66)frontend/src/contexts/DialogStateContext.tsx (1)
useDialogRegistration(85-94)frontend/wailsjs/go/service/LicenseServiceImpl.js (1)
GetLicensesByPurl(9-11)frontend/src/lib/shortcuts.ts (2)
KEYBOARD_SHORTCUTS(32-164)getShortcutDisplay(166-180)frontend/src/components/ShortcutBadge.tsx (1)
ShortcutBadge(1-3)frontend/src/components/SelectLicenseList.tsx (1)
SelectLicenseList(53-113)frontend/src/components/OnlineComponentSearchDialog.tsx (1)
OnlineComponentSearchDialog(91-234)
backend/service/license_service_impl.go (6)
backend/repository/license_repository.go (1)
LicenseRepository(28-30)backend/service/scanoss_api_service.go (1)
ScanossApiService(32-36)backend/service/license_service.go (1)
LicenseService(28-31)backend/entities/common.go (1)
ComponentRequest(3-6)backend/entities/license.go (1)
GetLicensesByPurlResponse(50-53)internal/utils/custom_validations.go (1)
GetValidator(37-39)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: integration-tests
- GitHub Check: unit_tests
🔇 Additional comments (5)
frontend/src/components/ReplaceComponentDialog.tsx (4)
184-192: Fix conflicting confirm shortcut: consolidate into a single bindingBoth handlers fire when the popover is open, causing submit + open-search to trigger together. Use one binding that branches on popoverOpen and remove the second hook.
- const ref = useKeyboardShortcut(KEYBOARD_SHORTCUTS.confirm.keys, () => submitButtonRef.current?.click(), { - enableOnFormTags: true, - }); - - useKeyboardShortcut(KEYBOARD_SHORTCUTS.confirm.keys, () => handleSearchOnline(), { - enableOnFormTags: true, - enabled: popoverOpen, - }); + const ref = useKeyboardShortcut( + KEYBOARD_SHORTCUTS.confirm.keys, + () => { + if (popoverOpen) handleSearchOnline(); + else submitButtonRef.current?.click(); + }, + { enableOnFormTags: true } + );
236-246: Use a semantic button for the “Search online” actionClickable div is not accessible; use a button to ensure focus/keyboard semantics.
- {searchValue ? ( - <CommandItem asChild> - <div onClick={handleSearchOnline} className="flex items-center justify-between gap-2"> - <div className="flex items-center gap-2"> - <Search className="h-3 w-3" /> - Search “{searchValue}” online - </div> - <ShortcutBadge shortcut={getShortcutDisplay(KEYBOARD_SHORTCUTS.confirm.keys, modifierKey.label)[0]} /> - </div> - </CommandItem> - ) : null} + {searchValue ? ( + <CommandItem asChild> + <button + type="button" + onClick={handleSearchOnline} + className="flex w-full items-center justify-between gap-2" + aria-label={`Search "${searchValue}" online`} + > + <div className="flex items-center gap-2"> + <Search className="h-3 w-3" /> + Search “{searchValue}” online + </div> + <ShortcutBadge shortcut={getShortcutDisplay(KEYBOARD_SHORTCUTS.confirm.keys, modifierKey.label)[0]} /> + </button> + </CommandItem> + ) : null}
335-341: Clear local search input after picking from online searchPrevents stale inline search text when returning to the popover.
onComponentSelect={async (c) => { onComponentSelected(c); await getMatchedLicenses(c.purl); + setSearchValue(''); }}
323-325: Prevent double submit: remove onClick from submit buttonForm has onSubmit; keeping onClick risks submitting twice.
- <Button ref={submitButtonRef} type="submit" onClick={form.handleSubmit(onSubmit)}> + <Button ref={submitButtonRef} type="submit"> Confirm <ShortcutBadge shortcut={getShortcutDisplay(KEYBOARD_SHORTCUTS.confirm.keys, modifierKey.label)[0]} /> </Button>backend/service/license_service_impl.go (1)
37-39: DI addition looks goodAdding
scanossApiServiceto the impl is consistent with the new interface and keeps concerns separated.
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
frontend/src/components/ui/keyboard-table.tsx (1)
98-106: Remove redundantrole="row"on<TableRow>.The
<tr>element has an implicit ARIA role of "row," and theTableRowcomponent spreads all props onto<tr>. Explicitly settingrole="row"is unnecessary and may lead to duplicate role declarations, which can confuse assistive technologies.Apply this diff to remove the redundant role:
<TableRow key={getRowId ? getRowId(row, i) : i} id={`row-${i}`} - role="row" data-row-index={i} onClick={() => onRowSelect(row)} className={customClassName} onMouseEnter={() => setFocusedIndex(i)} >Based on previous review feedback.
backend/service/scanoss_api_service_http_impl.go (1)
98-127: Thetimeoutfield is unused; per-request timeout not enforced.Lines 109-110 use
context.WithCancel, which doesn't enforce thes.timeoutduration. The only timeout in effect is the one configured on thehttp.Clientitself (line 68). This means thetimeoutfield (line 52, line 70) is effectively unused, and per-request timeout control is not available.Apply this diff to enforce the per-request timeout:
ctx := s.ctx if ctx == nil { ctx = context.Background() } - ctx, cancel := context.WithCancel(ctx) + ctx, cancel := context.WithTimeout(ctx, s.timeout) defer cancel()If the
timeoutfield remains unused, consider removing it from the struct (line 52) and the constructor (line 70) to reduce confusion.frontend/src/components/ReplaceComponentDialog.tsx (3)
236-246: Clickable div still present—a11y issue not resolvedThe past review comment recommended changing the clickable
<div>to a semantic<button>for accessibility, marked as addressed. However, line 238 still uses a<div onClick={...}>, which is not keyboard-accessible or properly announced to screen readers.Apply the suggested fix:
- {searchValue ? ( - <CommandItem asChild> - <div onClick={handleSearchOnline} className="flex items-center justify-between gap-2"> - <div className="flex items-center gap-2"> - <Search className="h-3 w-3" /> - Search “{searchValue}” online - </div> - <ShortcutBadge shortcut={getShortcutDisplay(KEYBOARD_SHORTCUTS.confirm.keys, modifierKey.label)[0]} /> - </div> - </CommandItem> - ) : null} + {searchValue ? ( + <CommandItem asChild> + <button + type="button" + onClick={handleSearchOnline} + className="flex w-full items-center justify-between gap-2" + > + <div className="flex items-center gap-2"> + <Search className="h-3 w-3" /> + Search “{searchValue}” online + </div> + <ShortcutBadge shortcut={getShortcutDisplay(KEYBOARD_SHORTCUTS.confirm.keys, modifierKey.label)[0]} /> + </button> + </CommandItem> + ) : null}
323-325: Double submit risk: onClick handler still present on buttonThe past review comment recommended removing
onClick={form.handleSubmit(onSubmit)}from the Confirm button to prevent double submission (form'sonSubmitat line 197 already handles it). The code still has theonClickhandler.Apply the fix:
- <Button ref={submitButtonRef} onClick={form.handleSubmit(onSubmit)}> + <Button ref={submitButtonRef} type="submit"> Confirm <ShortcutBadge shortcut={getShortcutDisplay(KEYBOARD_SHORTCUTS.confirm.keys, modifierKey.label)[0]} /> </Button>
188-191: Critical: duplicate keyboard shortcut binding still presentDespite being marked as addressed, the duplicate
KEYBOARD_SHORTCUTS.confirmbinding remains. When the popover is open, both hooks fire—the first (lines 184–186) clicks submit, the second (lines 188–191) opens online search—causing conflicting actions and an unintended submit.Remove this second hook and update the first hook to branch on
popoverOpen:- const ref = useKeyboardShortcut(KEYBOARD_SHORTCUTS.confirm.keys, () => submitButtonRef.current?.click(), { + const ref = useKeyboardShortcut(KEYBOARD_SHORTCUTS.confirm.keys, () => { + if (popoverOpen) { + handleSearchOnline(); + } else { + submitButtonRef.current?.click(); + } + }, { enableOnFormTags: true, }); - useKeyboardShortcut(KEYBOARD_SHORTCUTS.confirm.keys, () => handleSearchOnline(), { - enableOnFormTags: true, - enabled: popoverOpen, - });
🧹 Nitpick comments (5)
frontend/src/components/ui/keyboard-table.tsx (2)
25-30: Preferrows.lengthover DOM query for row count.Querying the DOM to determine the number of rows on every
movecall is less reliable and slightly less efficient than usingrows.lengthdirectly. If the DOM hasn't re-rendered yet,countmay be stale, leading to incorrect clamping.Apply this diff to use
rows.length:const move = (next: number) => { - const count = containerRef.current?.querySelectorAll<HTMLElement>('[data-row-index]').length ?? 0; + const count = rows.length; if (!count) return; const last = count - 1; setFocusedIndex(Math.max(0, Math.min(last, next))); };
49-52: Replace magic number99999with a more explicit constant.Using
99999as a stand-in for "end of list" works but is not self-documenting. Consider usingrows.lengthorNumber.MAX_SAFE_INTEGERfor clarity.Apply this diff for clarity:
case 'End': e.preventDefault(); e.stopPropagation(); - move(99999); + move(Number.MAX_SAFE_INTEGER); break;Alternatively, if you refactor
moveto acceptrows.length, you could callmove(rows.length - 1)directly for the End key.backend/service/scanoss_api_service_http_impl.go (2)
55-74: Constructor always returns nil error; consider simplifying the signature.The constructor never returns an error, so the
errorreturn value is unnecessary. Consider changing the signature toNewScanossApiServiceHttpImpl() ScanossApiService.Apply this diff:
-func NewScanossApiServiceHttpImpl() (ScanossApiService, error) { +func NewScanossApiServiceHttpImpl() ScanossApiService { cfg := config.GetInstance() baseURL := cfg.GetApiUrl() apiKey := cfg.GetApiToken() if baseURL == "" { baseURL = config.SCANOSS_PREMIUM_API_URL } service := &ScanossApiServiceHttpImpl{ apiKey: apiKey, baseURL: baseURL, client: &http.Client{ Timeout: 30 * time.Second, }, timeout: 30 * time.Second, } - return service, nil + return service }
197-235: Always sendingrequirementparameter, even when empty.Line 213 adds
requirementto query parameters unconditionally. Ifrequest.Requirementis empty, this sendsrequirement=in the query string. For consistency withSearchComponents(which only adds non-empty optional parameters), consider addingrequirementonly when non-empty.Apply this diff:
params := QueryParams{ - "purl": request.Purl, - "requirement": request.Requirement, + "purl": request.Purl, } + + if request.Requirement != "" { + params["requirement"] = request.Requirement + }frontend/src/components/ReplaceComponentDialog.tsx (1)
334-343: Consider clearing search value after online component selectionAfter selecting a component from the online search,
searchValueisn't cleared. This means the next time the popover opens, the stale search term remains visible. While this preserves context, it may confuse users.Clear the search input after closing the online search dialog:
<OnlineComponentSearchDialog onOpenChange={() => setOnlineSearchDialogOpen(false)} searchTerm={searchValue} onComponentSelect={async (c) => { onComponentSelected(c); await getMatchedLicenses(c.purl); + setSearchValue(''); }} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/service/scanoss_api_service_http_impl.go(1 hunks)frontend/src/components/ReplaceComponentDialog.tsx(10 hunks)frontend/src/components/ui/keyboard-table.tsx(1 hunks)scanoss.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
backend/service/scanoss_api_service_http_impl.go (6)
backend/service/scanoss_api_service.go (1)
ScanossApiService(32-36)internal/config/config.go (2)
GetInstance(111-116)SCANOSS_PREMIUM_API_URL(55-55)backend/entities/component_search.go (2)
ComponentSearchRequest(27-34)ComponentSearchResponse(44-47)internal/utils/custom_validations.go (1)
GetValidator(37-39)backend/entities/common.go (1)
ComponentRequest(3-6)backend/entities/license.go (1)
GetLicensesByPurlResponse(50-53)
frontend/src/components/ReplaceComponentDialog.tsx (9)
frontend/wailsjs/go/models.ts (2)
DeclaredComponent(332-345)License(444-459)frontend/src/hooks/useEnvironment.tsx (1)
useEnvironment(37-66)frontend/src/contexts/DialogStateContext.tsx (1)
useDialogRegistration(85-94)frontend/wailsjs/go/service/LicenseServiceImpl.js (1)
GetLicensesByPurl(9-11)frontend/src/hooks/useKeyboardShortcut.tsx (1)
useKeyboardShortcut(31-52)frontend/src/lib/shortcuts.ts (1)
getShortcutDisplay(166-180)frontend/src/components/ShortcutBadge.tsx (1)
ShortcutBadge(1-3)frontend/src/components/SelectLicenseList.tsx (1)
SelectLicenseList(53-113)frontend/src/components/OnlineComponentSearchDialog.tsx (1)
OnlineComponentSearchDialog(91-234)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (14)
frontend/src/components/ui/keyboard-table.tsx (2)
64-71: Enter key guard correctly added.The guard at line 67 correctly prevents
onRowSelectfrom being called with an invalid index, addressing the previous review comment. This ensures robustness when the table is empty orfocusedIndexis out of bounds.
81-118: Render logic is well-structured.The component correctly uses
role="grid"andaria-activedescendantfor keyboard navigation, applies focus styling withclsx, and safely handles missing cell data with fallback logic. The interaction handlers (onClick, onMouseEnter) provide a good user experience.scanoss.json (1)
139-152: LGTM!These replacement entries are properly structured with all required fields.
backend/service/scanoss_api_service_http_impl.go (5)
41-45: LGTM!The
HTTPClientinterface enables testability via dependency injection, and theQueryParamstype alias improves readability for query parameter handling.
47-53: Good structure for managing API client state.The struct properly separates concerns with context, credentials, HTTP client interface, and timeout configuration.
76-78: LGTM!The
SetContextmethod follows the standard pattern for injecting a parent context into the service.
80-96: LGTM!The
buildURLhelper correctly uses theurlpackage to construct URLs with properly encoded query parameters.
129-195: LGTM!The
SearchComponentsmethod correctly:
- Validates the request with the validator
- Builds query parameters conditionally (only non-empty/non-zero values)
- Handles errors with proper logging
- Checks for 200 status and decodes the response
frontend/src/components/ReplaceComponentDialog.tsx (6)
78-86: LGTM!State initialization and dialog registration are properly implemented. The dialog correctly registers as blocking keyboard navigation.
122-135: LGTM!The component selection handler properly prevents duplicates, updates form state, and resets dependent fields.
137-161: LGTM!The license fetching logic properly clears stale state before fetch, on empty results, and on errors. The workaround for missing reference URLs is clearly documented.
163-166: LGTM!Handler correctly toggles dialog states for the online search flow.
210-221: LGTM!Popover modal behavior and controlled CommandInput are correctly implemented.
288-288: LGTM!Passing
matchedLicensestoSelectLicenseListcorrectly integrates the fetched license suggestions into the UI.
| export function KeyboardTable<T>({ columns, rows, onRowSelect, getRowId, rowClassName, initialFocusedIndex = 0 }: KeyboardTableProps<T>) { | ||
| const containerRef = useRef<HTMLDivElement>(null); | ||
| const [focusedIndex, setFocusedIndex] = useState<number>(initialFocusedIndex); |
There was a problem hiding this comment.
Clamp initialFocusedIndex on mount to avoid invalid aria-activedescendant.
If initialFocusedIndex exceeds the number of rows, focusedIndex will point to a non-existent row until the first keyboard or hover interaction. This can cause aria-activedescendant to reference an invalid ID and may confuse assistive technologies.
Consider initializing focusedIndex with a clamped value:
- const [focusedIndex, setFocusedIndex] = useState<number>(initialFocusedIndex);
+ const [focusedIndex, setFocusedIndex] = useState<number>(() =>
+ Math.max(0, Math.min(initialFocusedIndex, rows.length - 1))
+ );Alternatively, use a useEffect to clamp focusedIndex whenever rows.length changes, ensuring the focused index remains valid if rows are updated dynamically.
📝 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.
| export function KeyboardTable<T>({ columns, rows, onRowSelect, getRowId, rowClassName, initialFocusedIndex = 0 }: KeyboardTableProps<T>) { | |
| const containerRef = useRef<HTMLDivElement>(null); | |
| const [focusedIndex, setFocusedIndex] = useState<number>(initialFocusedIndex); | |
| export function KeyboardTable<T>({ | |
| columns, | |
| rows, | |
| onRowSelect, | |
| getRowId, | |
| rowClassName, | |
| initialFocusedIndex = 0, | |
| }: KeyboardTableProps<T>) { | |
| const containerRef = useRef<HTMLDivElement>(null); | |
| const [focusedIndex, setFocusedIndex] = useState<number>(() => | |
| Math.max(0, Math.min(initialFocusedIndex, rows.length - 1)) | |
| ); | |
| // ... | |
| } |
🤖 Prompt for AI Agents
In frontend/src/components/ui/keyboard-table.tsx around lines 21-23, the
focusedIndex is initialized directly from initialFocusedIndex which can exceed
rows.length and produce an invalid aria-activedescendant; initialize
focusedIndex with a clamped value (e.g., clamp initialFocusedIndex between 0 and
rows.length - 1) and also add a useEffect that watches rows.length (and
initialFocusedIndex if desired) to re-clamp and setFocusedIndex whenever the
number of rows changes so the focused index always points to an existing row.
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.github/workflows/test.yml (1)
14-14: Remove unused GOLANG_VERSION env from workflowIn .github/workflows/test.yml (line 14), the GOLANG_VERSION env is ignored by actions/setup-go (it reads go.mod via go-version-file). Remove this line or switch setup-go to use go-version: ${{ env.GOLANG_VERSION }}.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/test.yml(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
| mkdir build | ||
| cp -r assets build/assets | ||
| wails build | ||
| wails build APPARGS="--key test" |
There was a problem hiding this comment.
❓ Verification inconclusive
APPARGS is positioned incorrectly; build likely fails.
Environment variables must precede the command. Current syntax passes APPARGS=... as a CLI arg to wails.
Apply:
- wails build APPARGS="--key test"
+ APPARGS="--key test" wails buildAlso confirm APPARGS is intended for build here; it’s typically used for wails dev.
Move APPARGS before the command
Replace the line so the environment variable is set correctly:
- wails build APPARGS="--key test"
+ APPARGS="--key test" wails buildAlso confirm that APPARGS should be used with wails build here (it’s typically passed to wails dev).
🤖 Prompt for AI Agents
In .github/workflows/test.yml around line 40, the environment variable is
currently placed after the command; move APPARGS before the command so it is
exported for the process (i.e., set APPARGS="--key test" wails build), and
verify whether APPARGS is intended for a build step — if it was meant for
development, change the command to use wails dev instead of wails build.
| - name: Run integration tests | ||
| env: | ||
| SCANOSS_API_KEY: ${{ secrets.SCANOSS_API_KEY }} | ||
| SCANOSS_API_KEY: ${{ secrets.SC_API_KEY }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Secret mapping OK; guard integration tests when secret is absent.
Forked PRs don’t receive repo secrets; this step will fail. Add a conditional so CI skips gracefully.
Apply:
- name: Run integration tests
+ if: ${{ secrets.SC_API_KEY != '' }}
env:
SCANOSS_API_KEY: ${{ secrets.SC_API_KEY }}
run: go test -v ./... -tags=integrationCheck the code expects SCANOSS_API_KEY (not SC_API_KEY):
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Searching for env usage:"
rg -nP 'SCANOSS_API_KEY|SC_API_KEY' -C2Length of output: 508
Guard integration tests when SC_API_KEY secret is absent.
Forked PRs don't receive repo secrets; skip the integration test step when the secret is empty. Verified: README expects SCANOSS_API_KEY and Makefile maps SC_API_KEY -> SCANOSS_API_KEY; workflow currently sets SCANOSS_API_KEY: ${{ secrets.SC_API_KEY }}. Apply:
- name: Run integration tests
+ if: ${{ secrets.SC_API_KEY != '' }}
env:
SCANOSS_API_KEY: ${{ secrets.SC_API_KEY }}
run: go test -v ./... -tags=integration📝 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.
| SCANOSS_API_KEY: ${{ secrets.SC_API_KEY }} | |
| - name: Run integration tests | |
| if: ${{ secrets.SC_API_KEY != '' }} | |
| env: | |
| SCANOSS_API_KEY: ${{ secrets.SC_API_KEY }} | |
| run: go test -v ./... -tags=integration |
🤖 Prompt for AI Agents
.github/workflows/test.yml around line 100: the workflow unconditionally sets
SCANOSS_API_KEY from secrets which causes forked PRs (no secret) to fail; guard
the integration test step or job by adding a conditional so it only runs when
the secret is present, for example add an if: ${{ secrets.SC_API_KEY != '' }} on
the integration test step or job (or set SCANOSS_API_KEY: ${{ secrets.SC_API_KEY
}} only within that conditional) so the step is skipped for forks without the
secret.
Summary by CodeRabbit
New Features
UI/UX Improvements
Chores