feat: handle nested and nullable lists #2088
Conversation
Noroth
requested review from
a team,
StarpTech,
devsergiy and
jensneuse
as code owners
|
Warning Rate limit exceeded@Noroth has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 27 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThis change introduces comprehensive support for nullable and nested list fields across the projects subgraph, impacting the data layer, service logic, GraphQL schema, and tests. Data structures for projects, tasks, milestones, employees, and products are refactored to use nested protobuf list wrappers. Numerous new helper functions and service methods are added to populate and query these complex list structures, and the schema is extended to expose them. Test suites are updated and expanded to validate these new patterns and edge cases. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
✨ 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. 🪧 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 (
|
Router image scan passed✅ No security vulnerabilities found in image: |
|
@coderabbitai summary |
✅ Actions performedSummary regeneration triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
demo/pkg/subgraphs/projects/src/data/milestones.go (1)
212-219: Consider moving GetEmployeeByID to employee.goAs noted in the comment, this helper function logically belongs in
employee.go. Moving it there would improve code organization and prevent potential circular dependency issues in the future.Consider moving this function to
employee.gowhere it logically belongs, which would improve code organization and maintainability.demo/pkg/subgraphs/projects/src/service/service.go (1)
640-681: Consider removing test-specific logic from production code.The method correctly implements task grouping by priority. However, lines 674-675 add an empty list and a nil element specifically for testing purposes. Consider moving test-specific data to test fixtures or mock data providers rather than embedding it in production code.
Consider creating a separate test configuration or mock data provider to handle test-specific scenarios instead of hardcoding them in the service layer.
demo/pkg/subgraphs/projects/src/data/projects.go (2)
227-247: Consider separating test data from production code.The function correctly implements tag collection with proper nil checks. However, line 244 adds test-specific data (empty string and "nullable-tag"). Consider moving test-specific data to test fixtures.
249-264: Test-specific nil value in production code.The function correctly filters completed projects as archived. However, line 261 appends a nil value specifically for testing. Consider using test fixtures for such scenarios.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (7)
demo/pkg/subgraphs/projects/generated/mapping.jsonis excluded by!**/generated/**demo/pkg/subgraphs/projects/generated/service.pb.gois excluded by!**/*.pb.go,!**/generated/**demo/pkg/subgraphs/projects/generated/service.protois excluded by!**/generated/**demo/pkg/subgraphs/projects/generated/service.proto.lock.jsonis excluded by!**/generated/**demo/pkg/subgraphs/projects/generated/service_grpc.pb.gois excluded by!**/*.pb.go,!**/generated/**router-tests/go.sumis excluded by!**/*.sumrouter/go.sumis excluded by!**/*.sum
📒 Files selected for processing (13)
demo/pkg/subgraphs/projects/src/data/employee.go(1 hunks)demo/pkg/subgraphs/projects/src/data/milestones.go(5 hunks)demo/pkg/subgraphs/projects/src/data/products.go(1 hunks)demo/pkg/subgraphs/projects/src/data/projects.go(2 hunks)demo/pkg/subgraphs/projects/src/data/tasks.go(4 hunks)demo/pkg/subgraphs/projects/src/main_test.go(9 hunks)demo/pkg/subgraphs/projects/src/schema.graphql(5 hunks)demo/pkg/subgraphs/projects/src/service/service.go(7 hunks)protographic/SDL_PROTO_RULES.md(3 hunks)router-tests/go.mod(1 hunks)router-tests/grpc_subgraph_test.go(3 hunks)router-tests/router_plugin_test.go(3 hunks)router/go.mod(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
demo/pkg/subgraphs/projects/src/data/employee.go (2)
demo/pkg/subgraphs/projects/generated/service.pb.go (24)
ListOfListOfProject(498-504)ListOfListOfProject(519-519)ListOfListOfProject(534-536)ListOfProject(738-744)ListOfProject(759-759)ListOfProject(774-776)ListOfProject_List(5485-5491)ListOfProject_List(5506-5506)ListOfProject_List(5521-5523)Project(3507-3531)Project(3546-3546)Project(3561-3563)ListOfListOfProject_List(5250-5256)ListOfListOfProject_List(5271-5271)ListOfListOfProject_List(5286-5288)Employee(4000-4012)Employee(4027-4027)Employee(4042-4044)ListOfString(834-840)ListOfString(855-855)ListOfString(870-872)ListOfString_List(5579-5585)ListOfString_List(5600-5600)ListOfString_List(5615-5617)demo/pkg/subgraphs/projects/src/data/projects.go (1)
ServiceProjects(8-156)
demo/pkg/subgraphs/projects/src/main_test.go (1)
demo/pkg/subgraphs/projects/generated/service.pb.go (15)
ProjectStatus_PROJECT_STATUS_PLANNING(28-28)ProjectStatus_PROJECT_STATUS_ON_HOLD(31-31)ProjectStatus_PROJECT_STATUS_COMPLETED(30-30)QueryTasksByPriorityRequest(2750-2756)QueryTasksByPriorityRequest(2771-2771)QueryTasksByPriorityRequest(2786-2788)QueryResourceMatrixRequest(2846-2852)QueryResourceMatrixRequest(2867-2867)QueryResourceMatrixRequest(2882-2884)LookupEmployeeByIdRequest(1459-1467)LookupEmployeeByIdRequest(1482-1482)LookupEmployeeByIdRequest(1497-1499)LookupEmployeeByIdRequestKey(1410-1417)LookupEmployeeByIdRequestKey(1432-1432)LookupEmployeeByIdRequestKey(1447-1449)
demo/pkg/subgraphs/projects/src/data/projects.go (1)
demo/pkg/subgraphs/projects/generated/service.pb.go (91)
ProjectStatus_PROJECT_STATUS_ACTIVE(29-29)Employee(4000-4012)Employee(4027-4027)Employee(4042-4044)Product(4095-4103)Product(4118-4118)Product(4133-4135)ListOfString(834-840)ListOfString(855-855)ListOfString(870-872)ListOfString_List(5579-5585)ListOfString_List(5600-5600)ListOfString_List(5615-5617)ListOfProject(738-744)ListOfProject(759-759)ListOfProject(774-776)ListOfProject_List(5485-5491)ListOfProject_List(5506-5506)ListOfProject_List(5521-5523)Project(3507-3531)Project(3546-3546)Project(3561-3563)ListOfListOfProjectResource(546-552)ListOfListOfProjectResource(567-567)ListOfListOfProjectResource(582-584)ListOfListOfTask(642-648)ListOfListOfTask(663-663)ListOfListOfTask(678-680)ListOfListOfMilestone(450-456)ListOfListOfMilestone(471-471)ListOfListOfMilestone(486-488)ListOfListOfListOfTask(402-408)ListOfListOfListOfTask(423-423)ListOfListOfListOfTask(438-440)ProjectStatus_PROJECT_STATUS_PLANNING(28-28)ProjectStatus_PROJECT_STATUS_ON_HOLD(31-31)ProjectStatus_PROJECT_STATUS_COMPLETED(30-30)Task(3825-3847)Task(3862-3862)Task(3877-3879)TaskStatus_TASK_STATUS_TODO(138-138)TaskStatus_TASK_STATUS_IN_PROGRESS(139-139)TaskStatus_TASK_STATUS_REVIEW(140-140)TaskStatus_TASK_STATUS_COMPLETED(141-141)TaskStatus_TASK_STATUS_BLOCKED(142-142)ListOfTask(882-888)ListOfTask(903-903)ListOfTask(918-920)ListOfTask_List(5626-5632)ListOfTask_List(5647-5647)ListOfTask_List(5662-5664)ListOfListOfTask_List(5391-5397)ListOfListOfTask_List(5412-5412)ListOfListOfTask_List(5427-5429)Milestone(3698-3714)Milestone(3729-3729)Milestone(3744-3746)ListOfMilestone(690-696)ListOfMilestone(711-711)ListOfMilestone(726-728)ListOfMilestone_List(5438-5444)ListOfMilestone_List(5459-5459)ListOfMilestone_List(5474-5476)ListOfListOfMilestone_List(5203-5209)ListOfListOfMilestone_List(5224-5224)ListOfListOfMilestone_List(5239-5241)TaskPriority_TASK_PRIORITY_LOW(196-196)TaskPriority_TASK_PRIORITY_MEDIUM(197-197)TaskPriority_TASK_PRIORITY_HIGH(198-198)TaskPriority_TASK_PRIORITY_URGENT(199-199)ListOfListOfListOfTask_List(5156-5162)ListOfListOfListOfTask_List(5177-5177)ListOfListOfListOfTask_List(5192-5194)ListOfProjectResource(786-792)ListOfProjectResource(807-807)ListOfProjectResource(822-824)ProjectResource(4158-4170)ProjectResource(4185-4185)ProjectResource(4200-4202)ProjectResource_Employee(4243-4245)ProjectResource_Employee(4259-4259)ListOfProjectResource_List(5532-5538)ListOfProjectResource_List(5553-5553)ListOfProjectResource_List(5568-5570)ProjectResource_Milestone(4251-4253)ProjectResource_Milestone(4263-4263)ProjectResource_Task(4255-4257)ProjectResource_Task(4265-4265)ListOfListOfProjectResource_List(5297-5303)ListOfListOfProjectResource_List(5318-5318)ListOfListOfProjectResource_List(5333-5335)
🪛 LanguageTool
protographic/SDL_PROTO_RULES.md
[style] ~264-~264: Consider a more concise word here.
Context: ...ed fields are not nullable in Protobuf. In order to ensure correct nullability, this is han...
(IN_ORDER_TO_PREMIUM)
⏰ 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). (7)
- GitHub Check: build_push_image (nonroot)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_push_image
- GitHub Check: integration_test (./telemetry)
- GitHub Check: build_test
- GitHub Check: integration_test (./events)
- GitHub Check: Analyze (go)
🔇 Additional comments (47)
router/go.mod (1)
34-34: LGTM! Dependency version update looks good.The version bump from
v2.0.0-rc.210tov2.0.0-rc.212appears to be coordinated with similar updates across the repository to support the new nested and nullable list features.demo/pkg/subgraphs/projects/src/data/employee.go (3)
7-101: Well-structured helper function for project history.The
GetProjectHistoryByEmployeeIdfunction is well-implemented with:
- Clear switch-case logic for different employees
- Proper use of protobuf wrapper types (
ListOfProject,ListOfListOfProject)- Appropriate fallback to empty history for unknown employees
- Good separation of historical periods with descriptive comments
105-125: Good use of nested list structures for employee data.The employee data structure updates properly utilize the protobuf wrapper types and demonstrate various nullable patterns. The addition of Skills, Certifications, and ProjectHistory fields enhances the data model effectively.
127-143: Nice demonstration of nullable list patterns.Employee ID 2 demonstrates nullable list handling well by setting
Skillstonilwhile populatingCertifications, which provides good test coverage for nullable vs populated list scenarios.router-tests/go.mod (1)
29-29: LGTM! Consistent dependency update.This version update aligns with the corresponding change in
router/go.modand maintains consistency across the repository.demo/pkg/subgraphs/projects/src/data/products.go (3)
3-5: Import formatting improvement.Good practice to use multi-line import block format for better readability and maintainability.
9-23: Excellent test coverage for nested nullable lists.The
FeatureMatrixfield demonstrates comprehensive edge case handling:
- Populated nested lists with actual data
- Empty list elements (
{})- Empty string elements (
{""})- Null elements (
nil)This provides excellent test coverage for the nullable and nested list functionality.
25-29: Good nullable field example.Setting
FeatureMatrixtonilfor the SDK product provides a clear example of nullable nested list behavior.router-tests/router_plugin_test.go (4)
105-105: Updated project name reflected correctly.The change from "Mobile App Redesign" to "Mobile App Development" is properly reflected in the expected response.
120-120: Comprehensive nested field test update.The expected response properly reflects the updated data structure with nested team member details and project assignments.
125-125: Employee project relationships updated correctly.The expected response accurately reflects the updated employee-project associations in the data model.
142-211: Excellent test coverage for nullable and nested lists.The new test cases provide comprehensive coverage for:
- Nullable list fields (
tags,skills,certifications)- Nested lists (
tasksByPriority,projectHistory,milestoneGroups)- Triple nested lists (
priorityMatrix)- Edge cases (empty lists, null elements, empty strings)
- Complex data structures (
resourceMatrix)The test cases are well-named and have properly formatted expected JSON responses that match the enhanced data model.
protographic/SDL_PROTO_RULES.md (3)
264-265: Clear explanation of nullable list handlingThe documentation effectively clarifies why nested
Listmessages are required for nullable lists and where nullability enforcement occurs. This is crucial information for service implementers.
277-280: Correct nested message pattern for nullable listsThe nested
Listmessage structure properly enables nullable list support while maintaining compatibility with Protobuf's repeated field limitations.
302-312: Consistent pattern application across nesting levelsThe nested
Listmessage pattern is correctly and consistently applied to both single and nested list scenarios, ensuring uniform handling throughout the codebase.demo/pkg/subgraphs/projects/src/data/tasks.go (4)
22-26: Well-structured list field additions with clear nullability semanticsThe new list fields effectively demonstrate various nullability patterns with helpful inline comments explaining each field's behavior. This provides good test coverage for the nested and nullable list functionality.
221-258: Good test coverage for nullable items in non-nullable listsThe helper function appropriately provides test data with intentional nil entries to validate the handling of nullable items within non-nullable lists. The switch-based logic is clear and maintainable for test purposes.
261-323: Consistent naming and comprehensive nullable list testingThe rename to
GetTaskByIDfollows Go naming conventions. TheGetTaskSubtasksfunction provides excellent test coverage by returning both nil lists and lists with nil items, thoroughly testing nullable list semantics.
325-349: Clean separation of static and dynamic dataThe
PopulateTaskRelationshipsfunction elegantly separates static task data from dynamically populated relationships, preventing initialization cycles and providing flexibility for testing different relationship scenarios.demo/pkg/subgraphs/projects/src/data/milestones.go (2)
18-46: Consistent pattern for dynamic relationship fieldsThe milestone data structure follows the same pattern as tasks, with placeholder fields for relationships that will be populated dynamically. The mix of empty lists and nil values provides good test coverage for different nullable list scenarios.
124-238: Well-structured helper functions with consistent patternsThe milestone helper functions follow the same patterns as tasks, providing consistent behavior across the codebase. The inclusion of nil entries and various list scenarios ensures comprehensive test coverage.
demo/pkg/subgraphs/projects/src/main_test.go (4)
131-177: Appropriate test adjustments for data model changesThe updated test counts correctly reflect the changed project data, and the improved handling of zero results (checking for both nil and empty slices) makes the tests more robust.
187-285: Updated tests reflect federation-style entity lookupsThe tests now correctly validate that missing entities return empty objects rather than errors, which aligns with GraphQL federation patterns. The addition of multiple ID testing improves test coverage for batch lookups.
307-310: Correct value comparison for protobuf wrapper typesThe fix properly compares the inner values of protobuf wrapper types rather than the wrapper objects themselves, ensuring accurate test validation.
314-388: Comprehensive test coverage for nested list functionalityThe new tests thoroughly validate the nested list features, including:
- Tasks grouped by priority with nullable entries
- Resource matrices with heterogeneous types
- Employee project history with multiple nesting levels
- Proper handling of empty and null lists
This provides excellent coverage of the new functionality.
router-tests/grpc_subgraph_test.go (2)
24-24: Test data updated to match project name changeThe test correctly reflects the project rename from "Mobile App Redesign" to "Mobile App Development".
61-130: Excellent comprehensive test coverage for nested listsThe new test cases provide thorough validation of the nested and nullable list functionality, covering:
- Various nullability patterns (nullable lists, nullable items)
- Multiple nesting levels (up to triple-nested for priority matrix)
- Empty vs null list differentiation
- Mixed type collections (resource matrix)
- Edge cases with empty items and null entries
This level of test coverage ensures the robustness of the implementation.
demo/pkg/subgraphs/projects/src/service/service.go (7)
52-65: LGTM! Well-structured population of nested relationships.The addition of new fields and their population through dedicated helper functions follows good separation of concerns. The use of helper functions from the data layer for populating complex nested structures is a clean approach.
70-86: Clean helper functions for populating relationships.The helper functions follow a consistent pattern and properly delegate the complex population logic to the data layer. This maintains good separation of concerns.
120-122: Proper population of relationships in lookup methods.The lookup methods now return fully populated entities with their relationships, which is consistent with the enhanced data model supporting nested structures.
Also applies to: 147-149
546-546: Correct initialization of nested list structure.The change from a simple slice to a protobuf wrapped list structure aligns with the new data model pattern for supporting nullable lists.
622-629: LGTM! Simple and correct implementation.The method follows the established pattern with proper read locking and delegates to the data layer appropriately.
631-638: Consistent implementation pattern.The method correctly implements the query for archived projects with proper locking.
683-741: Well-structured resource matrix implementation.The method correctly implements a matrix structure grouping project resources by type. The nested list structure is properly constructed and aligns with the schema requirements.
demo/pkg/subgraphs/projects/src/schema.graphql (5)
19-24: Comprehensive list pattern examples in query fields.The new query fields demonstrate various list patterns with clear documentation. The nullable and non-nullable list combinations provide good coverage for testing different scenarios.
96-104: Excellent variety of list patterns in Project type.The new fields provide comprehensive coverage of different list patterns including nullable lists, nested lists, and even triple-nested structures. The field names clearly indicate their purpose and the patterns are well-chosen for testing various scenarios.
117-121: Consistent list patterns in Milestone type.The new fields follow the established pattern with clear nullability semantics and good documentation.
137-143: Good coverage of list patterns in Task type.The new fields provide a nice mix of nullable and non-nullable lists, including primitive type lists (reviewerIds). This variety will be useful for comprehensive testing.
209-213: Well-designed list fields for federated types.The new fields in Employee and Product types maintain consistency with the list pattern approach while providing good examples for federated entities.
Also applies to: 220-222
demo/pkg/subgraphs/projects/src/data/projects.go (8)
8-156: Comprehensive refactoring to support nested list structures.The project data has been successfully refactored to use protobuf wrapped list structures. The initialization of all new fields is consistent and provides good examples of nullable and non-nullable patterns. Note that project ID "6" has been renamed from "Mobile App Redesign" to "Mobile App Development" with updated metadata.
212-225: Proper nil handling for nested list structures.The updated function correctly handles the new nested list structure with appropriate nil checks to prevent null pointer dereferences.
266-286: Simple dependency logic suitable for demo purposes.The function implements a reasonable dependency structure for demonstration. Note that line 282 adds a nil dependency for testing nullable items within lists.
288-303: Clean implementation of alternative projects logic.The function follows a similar pattern to dependencies and provides reasonable alternative project suggestions.
305-346: Well-structured task grouping by phase.The function correctly groups tasks by their status and creates a proper nested list structure. Note that line 340 adds a nil value for testing nullable lists.
348-385: Proper handling of milestone grouping with null case.The function correctly groups milestones by status and appropriately returns nil for projects without milestones, demonstrating proper nullable list handling.
387-436: Complex triple-nested structure implemented correctly.The function successfully creates a triple-nested list structure grouping tasks by priority. The empty case is properly handled by returning an empty structure rather than null.
438-484: Well-designed resource grouping with union types.The function effectively groups different resource types using the ProjectResource union pattern. The 3-task limit (line 470-472) appears intentional for demo purposes.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
demo/pkg/subgraphs/projects/src/data/milestones.go (2)
123-157: Consider refactoring dependency logic for maintainability.The dependency logic uses hardcoded switch cases which could become difficult to maintain as the number of milestones grows. Consider using a more data-driven approach.
+// Milestone dependency mappings +var milestoneDependencies = map[string][]string{ + "2": {"1"}, // Cloud Environment Setup depends on Infrastructure Assessment + "3": {"2"}, // Application Migration depends on Cloud Environment Setup + "5": {"4"}, // Microservice Architecture depends on Service Decomposition + "6": {"5"}, // API Development depends on Microservice Architecture +} + func GetMilestoneDependencies(milestoneID string) []*projects.Milestone { var dependencies []*projects.Milestone - // Simple dependency logic for testing - switch milestoneID { - case "2": // Cloud Environment Setup depends on Infrastructure Assessment - dep := GetMilestoneByID("1") - if dep != nil { - dependencies = append(dependencies, dep) - } - case "3": // Application Migration depends on Cloud Environment Setup - dep := GetMilestoneByID("2") - if dep != nil { - dependencies = append(dependencies, dep) - } - case "5": // Microservice Architecture depends on Service Decomposition - dep := GetMilestoneByID("4") - if dep != nil { - dependencies = append(dependencies, dep) - } - case "6": // API Development depends on Microservice Architecture - dep := GetMilestoneByID("5") - if dep != nil { - dependencies = append(dependencies, dep) - } + // Look up dependencies from mapping + if depIDs, exists := milestoneDependencies[milestoneID]; exists { + for _, depID := range depIDs { + if dep := GetMilestoneByID(depID); dep != nil { + dependencies = append(dependencies, dep) + } + } }
151-154: Review the intentional nil injection for testing.Adding nil dependencies for testing nullable semantics is understandable, but ensure this behavior is clearly documented and doesn't cause issues in production environments.
// Add nil for testing nullable items in non-nullable list + // TODO: Consider making this behavior configurable for production vs test environments if len(dependencies) > 0 { dependencies = append(dependencies, nil) }Verify that the GraphQL schema properly handles nil items in dependency lists without causing client-side errors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
demo/pkg/subgraphs/projects/src/data/milestones.go(5 hunks)demo/pkg/subgraphs/projects/src/main_test.go(9 hunks)router-tests/router_plugin_test.go(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
demo/pkg/subgraphs/projects/src/main_test.go (1)
Learnt from: Noroth
PR: #2088
File: demo/pkg/subgraphs/projects/src/main_test.go:0-0
Timestamp: 2025-07-29T08:19:55.720Z
Learning: In Go testing, t.Fatal, t.FailNow, t.Skip* and similar methods cannot be called from goroutines other than the main test goroutine, as they will cause a runtime panic. Use channels, t.Error*, or other synchronization mechanisms to communicate errors from goroutines back to the main test thread.
⏰ 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). (11)
- GitHub Check: build-router
- GitHub Check: image_scan
- GitHub Check: build_push_image (nonroot)
- GitHub Check: integration_test (./events)
- GitHub Check: build_push_image
- GitHub Check: image_scan (nonroot)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: build_test
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (17)
router-tests/router_plugin_test.go (4)
88-89: LGTM! Proper test data update and timeout optimization.The project name change from "Mobile App Redesign" to "Mobile App Development" aligns with the data model updates, and reducing the timeout from 10 to 2 seconds improves test efficiency without compromising reliability.
120-120: LGTM! Test data reflects the updated project structure.The expected response correctly includes the nullable notes fields and updated team member data, which aligns with the enhanced data model supporting nullable and nested lists.
125-125: LGTM! Employee project associations updated correctly.The expected response reflects the updated project associations and descriptions, maintaining consistency with the data model changes.
142-211: Excellent test coverage for nested and nullable list functionality.The new test cases comprehensively cover the PR objectives by testing:
- Nullable list fields (
tags,alternativeProjects,dependencies)- Nested list structures (
tasksByPriority,projectHistory,milestoneGroups)- Empty and null handling in complex data structures
- Triple-nested lists (
priorityMatrix)- Mixed nullable and non-nullable scenarios
The test cases validate both the happy path and edge cases, ensuring robust handling of the new nested and nullable list features.
demo/pkg/subgraphs/projects/src/data/milestones.go (5)
18-21: LGTM! Proper initialization strategy for dynamic relationship population.Initializing relationship fields with empty structures rather than nil values is the correct approach for protobuf-based systems. This prevents nil pointer exceptions while allowing dynamic population later through the helper functions.
Also applies to: 32-34, 44-47, 57-60
159-178: LGTM! Proper nullable list handling with clear test semantics.The function correctly demonstrates nullable list patterns by returning nil for specific milestone IDs and adding nil items to test nullable elements within lists.
180-197: LGTM! Clean reviewer assignment logic.The reviewer assignment logic is straightforward and properly structured. The switch-case approach works well for the current scale.
200-200: LGTM! Proper function naming convention.Renaming from
GetMilestoneByIdtoGetMilestoneByIDfollows Go naming conventions where ID should be capitalized as an acronym.
219-236: LGTM! Well-designed relationship population pattern.The
PopulateMilestoneRelationshipsfunction follows a clean pattern by creating a new instance with populated relationships rather than mutating the original. This approach prevents side effects and ensures thread safety.demo/pkg/subgraphs/projects/src/main_test.go (8)
46-46: LGTM! Proper error handling in goroutine.The change from
t.Fatalftot.Errorfcorrectly addresses the Go testing limitation wheret.Fatal*methods cannot be called from goroutines other than the main test goroutine. This prevents runtime panics while still logging server startup errors.
131-131: LGTM! Test expectations updated to match data model changes.The adjustment from 4 to 3 project statuses correctly reflects the removal of completed projects from the current test data set.
146-146: LGTM! Project count expectations align with updated data model.The adjusted project counts correctly reflect the changes in the underlying test data:
- Active projects: 5 (increased from previous count)
- Planning projects: 1
- On hold projects: 1
- Completed projects: 0 (removed from current data)
Also applies to: 151-151, 156-156, 161-161
171-178: LGTM! Improved handling of zero-result test cases.The updated logic properly handles cases where no projects match the given status, correctly asserting that the response has zero length rather than making assumptions about nil vs empty slice behavior.
187-205: Excellent enhancement to lookup tests for nullable semantics.The updated lookup tests now properly handle:
- Multiple ID lookups in a single test case
- Empty object responses for missing entities (following gRPC conventions)
- Mixed scenarios with both existing and non-existent entities
This aligns perfectly with the nullable and nested list semantics introduced in the PR.
Also applies to: 240-258
218-230: LGTM! Proper assertion logic for missing entities.The test correctly expects empty objects (with empty/zero IDs) rather than errors for missing entities, which is the appropriate behavior for GraphQL federation and nullable list handling.
Also applies to: 271-283
307-310: LGTM! Corrected wrapper type comparisons.The mutation test now properly compares the inner
.Valueofwrapperspb.StringValuetypes, ensuring accurate validation of the protobuf wrapper fields.
313-388: Excellent test coverage for new nested list functionality.The new test functions comprehensively validate the core features introduced in this PR:
TestQueryTasksByPriority: Tests nested list structures for task priority groupingTestQueryResourceMatrix: Validates resource matrix grouping functionalityTestEmployeeProjectHistory: Thoroughly tests triple-nested project history lists with proper null/empty handlingThe tests include appropriate assertions for:
- Non-null nested list structures
- Proper list wrapping with protobuf types
- Counting actual content vs empty groups
- Multiple employee scenarios
These tests directly exercise the new nested and nullable list capabilities that are the focus of this PR.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Documentation
Chores
Checklist