fix(web): show request title on linked request and item title on request content #1300
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes in this pull request focus on enhancing the test suite and utility functions related to item and request management in the application. New utility functions, Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for reearth-cms ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (12)
web/src/components/molecules/Request/types.ts (1)
23-31: Consider adding documentation and improving type specificity.While the type structure is good, consider these improvements:
- Add JSDoc documentation to explain the purpose and usage of this type
- Consider using a more specific type for
initialValuesinstead ofRecord<string, unknown>Here's a suggested improvement:
+/** + * Represents an item within a request context. + * Contains metadata and content information for items being requested. + */ export type ItemInRequest = { id: string; title: string; modelId?: string; modelName?: string; version?: string; schema?: Schema; - initialValues: Record<string, unknown>; + initialValues: Record<string, string | number | boolean | null>; referencedItems: FormItem[]; };web/e2e/project/utils/item.ts (3)
5-5: Consider using a more descriptive default request title.Using a more descriptive value like "Test Request" or "E2E Test Request" would make test outputs and debugging more intuitive.
-export const requestTitle = "requestTitle"; +export const requestTitle = "E2E Test Request";
7-12: Add error handling and creation verification.The function should be more robust by:
- Adding try-catch blocks for UI interactions
- Verifying that the item was successfully created
- Adding a timeout configuration for slow environments
export async function createItem(page: Page) { + try { await page.getByText("Content").click(); await page.getByRole("button", { name: "plus New Item" }).click(); await page.getByRole("button", { name: "Save" }).click(); await closeNotification(page); + + // Verify item creation + await page.waitForSelector('.ant-message-success', { timeout: 5000 }); + } catch (error) { + throw new Error(`Failed to create item: ${error.message}`); + } }
Line range hint
14-31: Add error handling and document the interaction flow.The function would benefit from:
- Error handling for UI interactions
- Documentation explaining the expected flow
- Verification of request creation
+/** + * Creates a new request with the following steps: + * 1. Opens the item menu + * 2. Initiates new request creation + * 3. Sets the request title + * 4. Selects a reviewer + * 5. Confirms the request creation + * @param page Playwright Page object + * @param title Request title (defaults to requestTitle) + */ export async function createRequest(page: Page, title = requestTitle) { + try { await page.getByRole("button", { name: "ellipsis" }).click(); await page.getByRole("menuitem", { name: "New Request" }).click(); await page.getByLabel("Title").last().click(); await page.getByLabel("Title").last().fill(title); await page.locator(".ant-select-selection-overflow").click(); - const reviewerName = await page... + const reviewerName = await page + .locator('[data-testid="reviewer-name"]') + .innerText(); await page.getByTitle(reviewerName).locator("div").click(); await page.locator(".ant-select-selection-overflow").click(); await page.getByRole("button", { name: "OK" }).click(); await closeNotification(page); + + // Verify request creation + await page.waitForSelector('.ant-message-success', { timeout: 5000 }); + } catch (error) { + throw new Error(`Failed to create request: ${error.message}`); + } }web/e2e/project/utils/field.ts (2)
16-17: Consider using more meaningful test data values.The constant values being identical to their variable names (
"titleFieldName"and"itemTitle") might not represent realistic test scenarios. Consider using more meaningful values that reflect actual user input.-export const titleFieldName = "titleFieldName"; -export const itemTitle = "itemTitle"; +export const titleFieldName = "Product Name"; +export const itemTitle = "Sample Product";
20-21: Improve selector reliability.The current selectors mix different strategies (filter, getByLabel, getByRole). Consider using more reliable
data-testidattributes for critical elements to make the tests more resilient to UI changes.Example implementation:
// Add to the component: <div data-testid="text-field-option">Text</div> // Then in the test: await page.getByTestId('text-field-option').click();web/e2e/project/utils/model.ts (1)
6-6: Consider a more descriptive constant name.While extracting the hardcoded string into a constant is good practice, consider making the name more specific to its purpose in E2E tests, e.g.,
DEFAULT_E2E_MODEL_NAMEorTEST_MODEL_NAME.web/src/components/molecules/Request/Details/RequestDescription.tsx (2)
Line range hint
37-47: Consider adding null check for better type safety.The changes improve the code by using proper typing and providing a fallback for the title. However, consider adding a null check for better type safety.
- {title || itemId} + {title ?? itemId}Using the nullish coalescing operator (
??) would be more explicit about handling null/undefined cases, while still allowing empty strings if they are valid titles.
Line range hint
26-93: Consider splitting the component for better maintainability.The component currently handles multiple responsibilities including:
- Request details display
- Item navigation
- Form rendering
- Styling
Consider splitting this into smaller, focused components:
RequestHeaderfor the title and descriptionRequestItemsfor the collapsible items listRequestItemfor individual item displayThis would improve maintainability and make the code more testable.
web/src/components/organisms/DataConverters/content.ts (2)
66-71: Consider adding fallback for undefined request title.While the addition of the title property aligns with the PR objectives, consider adding a fallback value for cases where the title might be undefined.
requests: GQLItem.requests?.map(request => ({ id: request.id, state: request.state, - title: request.title, + title: request.title ?? "", })) ?? [],
109-109: Consider simplifying the nested item structure.The path
item.item?.value.titlesuggests over-nesting in the data structure. While the current implementation works and includes proper null checking, consider:
- Working with the GraphQL team to flatten this structure in the schema
- Creating an intermediate variable for better readability
items: request.items?.map(item => ({ id: item.itemId, - title: item.item?.value.title ?? "", + // Option 1: Create intermediate variable for clarity + title: (() => { + const itemValue = item.item?.value; + return itemValue?.title ?? ""; + })(), modelId: item?.item?.value.modelId,web/src/components/organisms/Project/Content/hooks.ts (1)
84-84: Consider optimizing the refetch strategy.The addition of "GetItem" to refetchQueries ensures fresh data after adding items to a request, which aligns with showing updated titles. However, refetching the entire item data might be inefficient.
Consider using more specific queries or field-level updates if available in your GraphQL schema to minimize data transfer. For example:
-refetchQueries: ["GetItem"], +refetchQueries: [{ + query: GetItemTitleQuery, + variables: { + itemIds: items.map(item => item.itemId) + } +}],
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
web/e2e/project/request.spec.ts(3 hunks)web/e2e/project/utils/field.ts(1 hunks)web/e2e/project/utils/item.ts(1 hunks)web/e2e/project/utils/model.ts(1 hunks)web/src/components/molecules/Content/Form/SidebarWrapper.tsx(1 hunks)web/src/components/molecules/Content/types.ts(1 hunks)web/src/components/molecules/Request/Details/RequestDescription.tsx(4 hunks)web/src/components/molecules/Request/types.ts(1 hunks)web/src/components/organisms/DataConverters/content.ts(2 hunks)web/src/components/organisms/Project/Content/hooks.ts(2 hunks)web/src/gql/fragments/request.ts(1 hunks)web/src/gql/graphql-client-api.tsx(7 hunks)web/src/gql/queries/item.ts(1 hunks)
🔇 Additional comments (16)
web/src/components/molecules/Request/types.ts (2)
20-21: LGTM! Good refactoring of the items type.
The change from an inline type to a referenced type improves code maintainability and reusability.
20-31: Verify GraphQL schema alignment.
Let's ensure that the GraphQL schema matches these type definitions.
✅ Verification successful
Based on the analysis of the GraphQL schema and operations, I can now provide a final response:
GraphQL schema is properly aligned with type definitions
The GraphQL schema matches the type definitions in web/src/components/molecules/Request/types.ts. The requestFragment includes all the necessary fields:
titleis included in the fragment and queriesitemsfield with proper nesting structure- All related fields (
modelId,modelName,version,schema, etc.) are present in the GraphQL operations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if GraphQL schema matches the new type structure
# Expected: GraphQL queries/fragments should include title field in items
# Search for GraphQL fragments and queries related to Request
rg -t graphql "fragment.*Request|query.*Request" --glob "web/src/gql/**/*"
# Search for GraphQL operations using items or ItemInRequest
rg "items.*{[^}]*title" --glob "web/src/gql/**/*"
Length of output: 269
Script:
#!/bin/bash
# Let's try a different approach to find GraphQL schema and operations
# Find GraphQL files
fd '\.(graphql|gql)$' web/src
# Search for Request-related queries and fragments
rg "type.*Request|fragment.*Request|query.*Request" web/src
# Search for ItemInRequest or similar types in schema
rg "type.*Item.*Request" web/src
# Look for files that might contain GraphQL operations
rg -l "gql`|graphql`" web/src
Length of output: 62938
Script:
#!/bin/bash
# Let's check the GraphQL schema and operations related to Request and ItemInRequest
# Check the request fragment definition
cat web/src/gql/fragments/request.ts
# Check the request queries
cat web/src/gql/queries/requests.ts
# Look for RequestItem type definition in GraphQL operations
rg "type RequestItem" web/src
Length of output: 6382
web/src/components/molecules/Content/types.ts (1)
42-42: LGTM! Type definition aligns with PR objectives.
The addition of the title field to the requests property type definition enables proper typing for showing request titles on linked requests, which directly supports the PR's objective.
Let's verify the consistency of this change with related files:
✅ Verification successful
Based on the shell script results, I can now generate the final response:
Type definition is correctly aligned with Request interface and GraphQL schema
The addition of the title field in Pick<Request, "id" | "state" | "title"> is verified to be consistent with:
- The base
Requesttype definition inweb/src/components/molecules/Request/types.ts - GraphQL queries/fragments that include request titles
- Other components consuming request data with title field
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the Request type includes the title field and check for any GraphQL queries using this field
# Test 1: Check if Request type includes title field
echo "Checking Request type definition..."
ast-grep --pattern 'type Request = {
$$$
title: $_
$$$
}'
# Test 2: Check GraphQL queries/fragments for request title usage
echo "Checking GraphQL queries/fragments..."
rg -l 'fragment.*on Request.*\{' | xargs rg 'title'
# Test 3: Check for any potential type mismatches
echo "Checking for type usage..."
rg -A 2 'Pick<Request.*title'
Length of output: 37199
web/e2e/project/utils/model.ts (1)
8-8: LGTM! Good use of default parameters.
The function signature update properly utilizes the new constant as a default parameter, maintaining backward compatibility while improving code maintainability.
web/src/gql/fragments/request.ts (1)
18-18: LGTM! Verify title field usage in UI components.
The addition of the title field aligns with the PR objective of showing request/item titles in the UI.
Let's verify the UI components that will consume this title field:
✅ Verification successful
Title field addition is properly integrated with request components
The title field addition in the request fragment is well-integrated with the codebase:
- Used in request queries (
web/src/gql/queries/requests.ts) - Consumed by request-specific components in
web/src/components/molecules/Request/Details/ - Integrated with link item functionality (
LinkItemRequestModal.tsx)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find UI components that might need to be updated to use the new title field
# Expected: Components rendering request/item data should now use the title field
# Search for components using the request fragment
rg -l "requestFragment" "web/src/components"
# Search for components that might render item/request titles
rg -i "title.*render|render.*title" "web/src/components"
Length of output: 585
Script:
#!/bin/bash
# Let's examine the actual usage of the request fragment and title rendering
# to ensure the changes are properly integrated
# Check how requestFragment is imported and used
rg -A 5 "requestFragment" "web/src/components"
# Look at the implementation of renderTitle
cat "web/src/components/molecules/Content/RenderTitle/index.tsx"
# Check for any GraphQL queries using the request fragment
rg -l "gql.*requestFragment" "web/src"
Length of output: 480
Script:
#!/bin/bash
# Let's try a different approach to find request fragment usage
# and GraphQL operations that might be affected
# Search for request fragment imports
rg -A 3 "from.*request\"" "web/src"
# Search for GraphQL operations containing "request"
rg "gql`[^`]*request.*`" "web/src"
# Look for components that might render request data
rg -A 5 "Request.*props|request.*props" "web/src/components"
Length of output: 215
Script:
#!/bin/bash
# Let's do one final check to find where the request fragment is used
# and how the data is rendered
# Find files importing the fragments index
rg -l "from.*gql/fragments" "web/src"
# Look for request-related components
fd -e tsx -e ts "Request" "web/src/components"
# Check for GraphQL operations using fragments
rg -l "fragments.*request|request.*fragments" "web/src"
Length of output: 725
web/src/components/molecules/Content/Form/SidebarWrapper.tsx (2)
81-81: LGTM! UX improvement by showing request title.
The change from showing request ID to request title improves user experience by displaying more meaningful information in the linked requests section.
81-81: Verify request.title availability in GraphQL schema.
Let's ensure the title field is properly defined in the GraphQL schema to prevent potential undefined rendering issues.
✅ Verification successful
The title field is properly defined in the GraphQL schema
The verification confirms that:
- The
titlefield is properly defined as a required String (String!) in the Request type in the GraphQL schema - The Request type is properly referenced in the Item schema through
requests: [Request!] - The TypeScript types in the codebase also properly define the
titlefield in the Request interface
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the title field is properly defined in GraphQL schema and fragments
# Test 1: Look for request type definition in GraphQL schema
echo "Checking request type definition..."
rg -A 10 "type Request" "*.graphql"
# Test 2: Look for request fragments to verify title field inclusion
echo "Checking request fragments..."
rg -A 10 "fragment.*on Request" "*.graphql"
# Test 3: Look for related queries/mutations that fetch request data
echo "Checking queries and mutations..."
rg "query.*Request|mutation.*Request" "*.graphql"
Length of output: 512
Script:
#!/bin/bash
# Let's first locate GraphQL schema and query files with different extensions and paths
echo "Searching for GraphQL files..."
fd -e graphql -e gql
# Then search for request type definition in the found files
echo "Searching for Request type in TypeScript files..."
rg -t typescript "type Request" -A 10
# Look for GraphQL operations in TypeScript files
echo "Searching for GraphQL operations..."
rg -t typescript "useQuery|useMutation.*Request"
# Check the component's imports and props
echo "Checking SidebarWrapper component..."
cat "web/src/components/molecules/Content/Form/SidebarWrapper.tsx"
Length of output: 4905
Script:
#!/bin/bash
# Let's check the request.graphql schema file we found
echo "Checking request schema definition..."
cat server/schemas/request.graphql
# Let's also check the Item type definition since the component uses item.requests
echo "Checking item schema definition..."
cat server/schemas/item.graphql
# Let's check for any TypeScript type definitions of Request
echo "Checking TypeScript Request types..."
rg "interface Request|type Request" -A 5 "web/src/components"
Length of output: 6361
web/src/components/molecules/Request/Details/RequestDescription.tsx (1)
10-10: LGTM: Import addition is appropriate.
The addition of ItemInRequest type import aligns with the changes to support item titles in the component.
web/src/components/organisms/Project/Content/hooks.ts (1)
104-104: Verify data consistency after removing "GetItemsByIds" query.
The removal of "GetItemsByIds" from refetchQueries while keeping "SearchItem" and "GetItem" might affect data consistency if any components still rely on the removed query.
Let's verify the usage of these queries in the codebase:
web/src/gql/queries/item.ts (2)
140-140: LGTM: Addition of request title field
The addition of the title field to the requests object aligns perfectly with the PR objective to show request titles on linked requests.
Line range hint 219-227: Verify the usage of __typename field
While adding __typename is a good practice for union type discrimination, let's verify its usage in the client code.
If the __typename field isn't being used in the client code, consider one of these approaches:
- Remove it to keep the query minimal
- Add it consistently to all union types in the queries for better maintainability
✅ Verification successful
__typename field is correctly used and necessary
The __typename field is being used in the generated TypeScript types for proper type discrimination in union types. This is evident in the graphql-client-api.tsx file where updatedBy is defined as a union type { __typename: 'Integration', name: string } | { __typename: 'User', name: string }. The field is necessary for proper type-safe handling of the union type in the client code.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for potential usage of __typename in client code
echo "Searching for __typename usage in client code..."
rg -l "__typename.*updatedBy" web/src/
Length of output: 182
Script:
#!/bin/bash
# Let's check the actual usage in the found file
rg -A 5 -B 5 "__typename.*updatedBy" web/src/gql/graphql-client-api.tsx
Length of output: 4117
web/e2e/project/request.spec.ts (3)
5-7: LGTM! Well-organized imports.
The new imports improve code organization by introducing utility functions and constants, making the tests more maintainable.
16-18: LGTM! Clear and well-structured test setup.
The setup follows a logical sequence using utility functions:
- Create title field
- Create item
- Create request
This makes the test initialization more maintainable and easier to understand.
80-80: LGTM! Consistent visibility check.
The visibility check for the request title is properly placed and follows the test's flow.
web/src/gql/graphql-client-api.tsx (2)
Line range hint 2261-3112: LGTM! The RequestFragmentFragment type is properly updated to include the title field.
The addition of the title field to the RequestFragmentFragment type is well-implemented and aligns with the PR objective to show request titles. This fragment is reused across multiple queries and mutations, ensuring consistent title handling.
Line range hint 2540-2769: LGTM! Title field changes are consistently applied across request operations.
The title field is properly added to all relevant request operations:
- GetItemQuery for fetching requests linked to items
- GetRequestQuery for fetching individual requests
- CreateRequest, UpdateRequest, and ApproveRequest mutations
This ensures consistent handling of request titles across the application.
Overview
This PR fixes to show request title on linked request and item title on request content.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Chores