fix: toggle ui logic and search to consider all operations #1982
Conversation
Router image scan passed✅ No security vulnerabilities found in image: |
WalkthroughThis change introduces new optional search fields to several protobuf request and response messages and propagates these fields through backend repositories, service logic, and the UI. The backend now supports server-side filtering of operations by search term, and the frontend provides a search input and a toggle to limit overrides to filtered operations, removing previous client-side filtering. Changes
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
controlplane/src/core/repositories/SchemaCheckRepository.ts (1)
340-349: Consider performance optimization for search queries.The search implementation is functionally correct but may have performance implications with wildcard patterns (
%${search}%) on large datasets. As discussed in previous comments, consider implementing pg_trgm indexes for better performance with pattern matching queries.Consider adding GIN indexes with pg_trgm extension:
CREATE EXTENSION IF NOT EXISTS pg_trgm; CREATE INDEX CONCURRENTLY idx_operation_usage_name_trgm ON schema_check_change_action_operation_usage USING GIN (name gin_trgm_ops); CREATE INDEX CONCURRENTLY idx_operation_usage_hash_trgm ON schema_check_change_action_operation_usage USING GIN (hash gin_trgm_ops);Also applies to: 404-413
proto/wg/cosmo/platform/v1/platform.proto (1)
573-575: Field names violate protobuf style & are excessively verboseBuf already flags these as non-snake_case. In addition, the names are so long that they harm readability and increase boilerplate in generated code.
- bool doAllOperationsHaveIgnoreAllOverride = 7; - bool doAllOperationsHaveAllTheirChangesMarkedSafe = 8; + bool all_operations_have_ignore_all_override = 7; + bool all_operations_have_all_changes_marked_safe = 8;Because these fields are brand-new, renaming now avoids a locked-in breaking change later.
🧹 Nitpick comments (2)
studio/src/components/checks/operations.tsx (1)
72-74: Initialize search state more safely from router query.The search state initialization should handle undefined values more explicitly to avoid potential TypeScript issues.
Apply this diff to make the initialization more explicit:
- const [search, setSearch] = useState(router.query.search as string); + const [search, setSearch] = useState((router.query.search as string) || "");controlplane/src/core/repositories/OperationsRepository.ts (1)
371-408: Add performance benchmarks and consider optimizinggetOperationOverrideStatusOfCheckI didn’t find any existing performance tests or benchmarks for this method—adding them will help catch regressions and guide future optimizations.
• File:
controlplane/src/core/repositories/OperationsRepository.ts(lines 371–408)
• Function:getOperationOverrideStatusOfCheckSuggested next steps:
- Offload aggregate logic to the database (SQL‐level computation)
- Replace
Array.prototype.everywith manual loops for early exits- Process large operation sets in batches
- Add benchmark tests (e.g., using [your preferred testing/benchmarking tool]) and monitor under realistic load
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.gois excluded by!**/*.pb.go,!**/gen/**
📒 Files selected for processing (9)
connect/src/wg/cosmo/platform/v1/platform_pb.ts(8 hunks)controlplane/src/core/bufservices/check/createIgnoreOverridesForAllOperations.ts(1 hunks)controlplane/src/core/bufservices/check/getCheckOperations.ts(5 hunks)controlplane/src/core/bufservices/check/toggleChangeOverridesForAllOperations.ts(1 hunks)controlplane/src/core/repositories/OperationsRepository.ts(4 hunks)controlplane/src/core/repositories/SchemaCheckRepository.ts(6 hunks)controlplane/src/db/models.ts(2 hunks)proto/wg/cosmo/platform/v1/platform.proto(4 hunks)studio/src/components/checks/operations.tsx(8 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
controlplane/src/db/models.ts (1)
controlplane/src/db/schema.ts (1)
schemaChangeTypeEnum(634-711)
studio/src/components/checks/operations.tsx (5)
studio/src/components/ui/dropdown-menu.tsx (1)
DropdownMenuSeparator(196-196)connect/src/wg/cosmo/platform/v1/platform_pb.ts (1)
Label(340-378)connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.go (3)
Label(562-569)Label(584-584)Label(599-601)controlplane/src/types/index.ts (1)
Label(55-58)studio/src/components/ui/switch.tsx (1)
Switch(27-27)
🪛 Buf (1.54.0)
proto/wg/cosmo/platform/v1/platform.proto
573-573: Field name "doAllOperationsHaveIgnoreAllOverride" should be lower_snake_case, such as "do_all_operations_have_ignore_all_override".
(FIELD_LOWER_SNAKE_CASE)
574-574: Field name "doAllOperationsHaveAllTheirChangesMarkedSafe" should be lower_snake_case, such as "do_all_operations_have_all_their_changes_marked_safe".
(FIELD_LOWER_SNAKE_CASE)
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: build-router
- GitHub Check: build_test
- GitHub Check: integration_test (./telemetry)
- GitHub Check: image_scan
- GitHub Check: integration_test (./events)
- GitHub Check: build_push_image
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: build_test
- GitHub Check: Analyze (go)
- GitHub Check: build_push_image
🔇 Additional comments (26)
controlplane/src/db/models.ts (1)
19-19: LGTM! Clean type definition for schema change enum.The import and type definition follow the established patterns in this file and provide proper type safety for schema change types.
Also applies to: 39-39
controlplane/src/core/bufservices/check/toggleChangeOverridesForAllOperations.ts (1)
57-57: LGTM! Clean parameter propagation for search functionality.The search parameter is properly propagated from the request to the repository method, enabling search filtering for affected operations.
controlplane/src/core/bufservices/check/createIgnoreOverridesForAllOperations.ts (1)
54-54: LGTM! Consistent search parameter propagation.The search parameter is properly propagated, maintaining consistency with other similar service methods.
controlplane/src/core/bufservices/check/getCheckOperations.ts (4)
44-45: LGTM! Proper initialization of new boolean flags in early returns.The new override status flags are correctly initialized to
falsein all early return scenarios, ensuring consistent response structure.Also applies to: 71-72, 88-89
97-97: LGTM! Consistent search parameter propagation.The search parameter is properly propagated to both repository methods, enabling filtering of affected operations and their counts.
Also applies to: 114-114
117-123: LGTM! Well-structured override status calculation.The new method call properly aggregates override status information using the check details and existing overrides, enhancing the response with useful metadata.
145-146: LGTM! Complete response structure enhancement.The new boolean flags are properly included in the final response, completing the enhancement to provide override status information to clients.
controlplane/src/core/repositories/SchemaCheckRepository.ts (3)
14-14: LGTM! Proper import addition for search functionality.The
ilikeimport is correctly added to support case-insensitive pattern matching.
320-320: LGTM! Clean method signature extension.The optional search parameter is properly added to both method signatures, maintaining backwards compatibility.
Also applies to: 325-325, 390-390
368-373: LGTM! Proper condition integration in where clauses.The search conditions are correctly combined with existing filters using proper logical operators, maintaining query correctness.
Also applies to: 422-427
proto/wg/cosmo/platform/v1/platform.proto (3)
548-553: 👍 Search filter addition looks correct
searchis appended with tag6, preserving field-number stability and markedoptional, matching the other request DTOs. No concerns here.
1568-1574: Consistent filter field addition – looks good
search = 5is appended at the end, so wire compatibility is preserved.
1580-1585: Consistent filter field addition – looks goodSame comment as above; the tag number is appended safely.
connect/src/wg/cosmo/platform/v1/platform_pb.ts (4)
4259-4262: LGTM! Consistent addition of search field to GetCheckOperationsRequest.The optional
searchfield (field 6) is properly defined with correct TypeScript and protobuf schema definitions. The optional modifier ensures backward compatibility.Also applies to: 4277-4277
4331-4339: LGTM! Well-structured addition of aggregate status fields.The two new boolean fields provide useful aggregate information about operation override statuses:
doAllOperationsHaveIgnoreAllOverride(field 7)doAllOperationsHaveAllTheirChangesMarkedSafe(field 8)The field names are descriptive and the boolean type is appropriate for these status indicators.
Also applies to: 4355-4356
12077-12080: LGTM! Consistent search field addition to ToggleChangeOverridesForAllOperationsRequest.The optional
searchfield (field 5) follows the same pattern as other request messages, maintaining consistency across the API.Also applies to: 12094-12094
12170-12173: LGTM! Consistent search field addition to CreateIgnoreOverridesForAllOperationsRequest.The optional
searchfield (field 4) completes the consistent addition of search capability across all related request messages.Also applies to: 12186-12186
studio/src/components/checks/operations.tsx (6)
57-59: LGTM! Good library choices for the new functionality.The new imports for debounced search, Switch component, and Label component are appropriate for the added functionality.
84-84: LGTM! Debounced search parameter implementation.Using
debouncedSearchinstead of the raw search value is a good practice to avoid excessive API calls during typing.
253-258: LGTM! Simplified operations handling with server-side data.The removal of client-side filtering and direct use of server-provided boolean flags simplifies the component logic and improves performance.
339-339: LGTM! Conditional search parameter for filtered operations.The logic correctly passes the search parameter only when
applyOnlyFilteredis enabled, allowing users to choose the scope of override operations.Also applies to: 364-364
381-390: LGTM! Well-implemented toggle switch for filtered operations.The new toggle switch is properly integrated into the dropdown menu with clear labeling and appropriate state management. The UI provides good control over the scope of override operations.
400-400: LGTM! Direct use of server-filtered operations.Using
operationsdirectly instead of applying client-side filtering is correct since the server now handles the filtering based on the search parameter.controlplane/src/core/repositories/OperationsRepository.ts (3)
5-5: LGTM! Appropriate imports for the new functionality.The additional imports for database models, schema check details DTO, and the SchemaCheckRepository are necessary for the new method implementation.
Also applies to: 13-13, 16-16
18-30: LGTM! Well-defined type definitions.The
ChangeOverrideandIgnoreAllOverridetypes provide better type safety for the repository methods. The structure is appropriate for the data being handled.
348-348: LGTM! Explicit return types improve type safety.Adding explicit return types for
getChangeOverridesandgetIgnoreAllOverridesmethods improves type safety and code documentation.Also applies to: 360-360
Motivation and Context
This PR fixes the toggle ui logic and search to account for all the operations affected rather than just the first page of the paginated operations list.
Checklist
Screen.Recording.2025-06-23.at.4.14.41.PM.mov
Summary by CodeRabbit
New Features
Enhancements
User Interface