Add PnP Extraction Result to Planning Sheet Card #1621
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe changes introduce a new functional component named Changes
Poem
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 (
|
![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
🧹 Outside diff range and nitpick comments (9)
src/scenes/Engagement/LanguageEngagement/PlanningSpreadsheet/PlanningSpreadsheet.graphql (1)
10-12: Consider documenting the field purpose with GraphQL descriptionsThe field appears to be part of a feature for displaying PnP extraction results. Consider adding a GraphQL description to document its purpose and relationship with the
pnpfield above it.+ """ + The extraction result of the PnP document, used for validation and preview purposes. + """ pnpExtractionResult { ...pnpExtractionResult }src/components/Preview.tsx (2)
4-4: Consider using a more descriptive type alias nameThe alias
FilefromNonDirectoryActionItemmight not clearly convey its purpose or constraints. Consider using a more specific name likePreviewableFileorActionableFileto better represent its role.- NonDirectoryActionItem as File, + NonDirectoryActionItem as PreviewableFile,
8-17: Consider architectural improvementsSince this is a reusable component, consider:
- Adding unit tests to verify behavior
- Making the component more configurable for different use cases
Would you like me to:
- Generate unit test cases for this component?
- Propose a more configurable version with additional props for customization?
src/scenes/ProgressReports/Detail/ProgressReportCard.tsx (3)
Line range hint
31-45: Consider adding error handling for the reextract mutation.While the mutation implementation is good, it would be beneficial to handle potential errors and provide user feedback.
Consider adding error handling:
const [reextract, { loading: reextracting }] = useMutation( ReextractPnpProgressDocument, { variables: { reportId: progressReport.id }, + onError: (error) => { + // Add error notification/handling here + console.error('Failed to reextract PnP progress:', error); + }, update: (cache, result) => { cache.modify({ id: cache.identify(progressReport), fields: { pnpExtractionResult: () => result.data?.reextractPnpProgress, }, }); }, } );
Line range hint
47-49: Remove commented code and unnecessary fragment.The commented Tooltip and eslint-disable comment with empty fragment can be removed to improve code cleanliness.
- {/*<Tooltip title="This holds the progress report PnP file" placement="top">*/} - {/* eslint-disable-next-line react/jsx-no-useless-fragment */} - <> + <div> <DefinedFileCard // ... rest of the component - </> + </div>
Line range hint
54-97: Consider extracting the header component for better maintainability.The header structure is complex with multiple conditional renders. Extracting it into a separate component would improve readability and maintainability.
Consider creating a new component:
interface ProgressReportHeaderProps { file: File | null; reextracting: boolean; onReextract: () => void; pnpExtractionResult: typeof progressReport.pnpExtractionResult; parent: { id: string }; } const ProgressReportHeader = ({ file, reextracting, onReextract, pnpExtractionResult, parent, }: ProgressReportHeaderProps) => ( <Stack sx={{ mt: -1, flexDirection: 'row', gap: 1, alignItems: 'center', }} > <Typography variant="h3">PnP File</Typography> {file && ( <> <Preview file={file} /> <Feature flag="pnp-validation" match={true} sx={{ display: 'inherit', flexDirection: 'inherit', gap: 'inherit', }} > {reextracting ? ( <CircularProgress size={15} sx={{ ml: 1.1 }} /> ) : ( <PnPReextractIconButton size="small" onClick={onReextract} /> )} {pnpExtractionResult && !reextracting && ( <PnPValidationIcon file={file} result={pnpExtractionResult} engagement={parent} size="small" /> )} </Feature> </> )} </Stack> );src/scenes/Engagement/LanguageEngagement/PlanningSpreadsheet/PlanningSpreadsheet.tsx (3)
51-51: Remove commented out codeRemove the commented-out Tooltip code instead of leaving it in the codebase.
- {/* <Tooltip title="This holds the planning info of PnP files" placement="top"> */}
63-70: Consider moving styles to useStyles hookThe inline sx styles could be moved to the useStyles hook for better maintainability and consistency with the project's styling patterns.
const useStyles = makeStyles()(({ spacing, typography }) => ({ // ... existing styles ... + headerStack: { + marginTop: spacing(-1), + flexDirection: 'row', + gap: spacing(1), + alignItems: 'center', + }, })); // Then in JSX: -<Stack - sx={{ - mt: -1, - flexDirection: 'row', - gap: 1, - alignItems: 'center', - }} -> +<Stack className={classes.headerStack}>
75-92: Optimize Feature component stylingThe Feature component duplicates the parent Stack's styling properties. Consider using a more maintainable approach.
-<Feature - flag="pnp-validation" - match={true} - sx={{ - display: 'inherit', - flexDirection: 'inherit', - gap: 'inherit', - }} -> +<Feature + flag="pnp-validation" + match={true} + sx={{ display: 'contents' }} >Using
display: 'contents'will make the Feature component inherit its parent's layout properties without needing to explicitly copy them.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
src/components/Preview.tsx(1 hunks)src/scenes/Engagement/LanguageEngagement/PlanningSpreadsheet/PlanningSpreadsheet.graphql(1 hunks)src/scenes/Engagement/LanguageEngagement/PlanningSpreadsheet/PlanningSpreadsheet.tsx(3 hunks)src/scenes/ProgressReports/Detail/ProgressReportCard.tsx(1 hunks)
🔇 Additional comments (7)
src/scenes/Engagement/LanguageEngagement/PlanningSpreadsheet/PlanningSpreadsheet.graphql (1)
10-12: Verify the existence of the spread fragment
The schema spreads a fragment named pnpExtractionResult, but we need to ensure this fragment is properly defined elsewhere in the codebase.
✅ Verification successful
Fragment pnpExtractionResult exists and is properly defined
The fragment is defined in src/scenes/ProgressReports/PnpValidation/pnpExtractionResult.graphql with a valid structure that includes:
- A nested
problemsfield that spreads another fragmentpnpProblem - The
pnpProblemfragment is defined in the same file with all necessary fields
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the definition of pnpExtractionResult fragment
rg "fragment pnpExtractionResult on" --type graphql
Length of output: 85
Script:
#!/bin/bash
# Search for the definition of pnpExtractionResult fragment using ripgrep
# without file type filter since .graphql extension might not be recognized
rg "fragment pnpExtractionResult on"
Length of output: 161
Script:
#!/bin/bash
# Let's verify the content of the fragment to ensure it's properly defined
cat src/scenes/ProgressReports/PnpValidation/pnpExtractionResult.graphql
Length of output: 238
src/components/Preview.tsx (1)
1-17: Verify component usage across the codebase
Let's ensure consistent implementation and proper file type handling across the codebase.
✅ Verification successful
Preview component implementation is consistent and properly integrated
The verification shows that:
- The Preview component is correctly used in two locations (ProgressReportCard and PlanningSpreadsheet)
- It's properly integrated with FileActionsContext for file preview handling
- The NonDirectoryActionItem type is correctly defined and used
- File preview functionality has proper error boundaries and loading states
The implementation is consistent across the codebase with appropriate type safety and error handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify Preview component usage and file type handling
# Check Preview component usage patterns
echo "Checking Preview component usage patterns:"
rg -A 2 "<Preview\s.*?file=" --type tsx
# Verify file type handling
echo "Checking NonDirectoryActionItem usage:"
ast-grep --pattern 'NonDirectoryActionItem'
# Check for potential file preview related error handling
echo "Checking error handling patterns:"
rg "openFilePreview.*?catch" --type tsx
Length of output: 725
Script:
#!/bin/bash
# Let's modify our search patterns for better results
# Check Preview component usage
echo "Checking Preview component usage:"
rg "<Preview\s.*?file="
# Check FileActions implementation
echo "\nChecking FileActions implementation:"
cat src/components/files/FileActions.ts || cat src/components/files/FileActions.tsx
# Check openFilePreview implementation
echo "\nChecking openFilePreview implementation:"
rg "openFilePreview.*?=" -A 5
# Check where Preview is imported
echo "\nChecking Preview component imports:"
rg "import.*Preview.*from.*components/Preview"
Length of output: 5347
Script:
#!/bin/bash
# Let's check the FileActions implementation and type definitions
# Check FileActions directory content
echo "Checking FileActions directory content:"
fd FileActions --type f
# Try to find NonDirectoryActionItem type definition
echo "\nChecking NonDirectoryActionItem definition:"
rg -B 2 -A 5 "type.*NonDirectoryActionItem"
# Check FileActionsContext implementation
echo "\nChecking FileActionsContext implementation:"
cat src/components/files/FileActions/FileActionsContext.tsx
# Check where Preview component is actually used
echo "\nChecking actual Preview usage with context:"
rg -B 2 -A 2 "<Preview.*?/>"
Length of output: 8825
src/scenes/ProgressReports/Detail/ProgressReportCard.tsx (2)
Line range hint 1-16: LGTM! Clean import structure and type definitions.
The refactoring to use a shared Preview component improves code reusability. The Props interface is well-defined with proper type safety.
3-3: Verify Preview component migration completeness.
The Preview component has been migrated to a shared component. Let's verify all references have been updated correctly.
Also applies to: 71-71
✅ Verification successful
Preview component migration is correctly implemented
The verification shows that:
- The Preview component is properly defined in the shared components directory (
src/components/Preview.tsx) - The file under review (
ProgressReportCard.tsx) correctly imports it from the shared location - No local Preview component definitions with file props remain in the codebase
- Other Preview-related imports in the codebase are unrelated (different components like NodePreview, FilePreview, etc.)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining local Preview component definitions or old imports
rg -l "const Preview.*=.*{.*file.*}.*=>" --type typescript
rg -l "import.*PreviewIcon.*from" --type typescript
Length of output: 283
Script:
#!/bin/bash
# Search for any remaining local Preview component definitions
rg "const Preview.*=.*\{.*file"
# Search for Preview component imports
rg "import.*Preview.*from"
Length of output: 3494
src/scenes/Engagement/LanguageEngagement/PlanningSpreadsheet/PlanningSpreadsheet.tsx (3)
2-2: LGTM: Import statements are properly organized
The new imports are correctly added and organized according to the project's conventions.
Also applies to: 15-16, 23-23
84-91: Verify GraphQL schema integration
Ensure that the pnpExtractionResult field is properly defined in the GraphQL schema and that the EngagementPlanningSpreadsheetFragment includes this field.
✅ Verification successful
GraphQL schema integration is properly configured
The verification confirms that:
- The
pnpExtractionResultfield is included in theEngagementPlanningSpreadsheetfragment - The
pnpExtractionResultfragment is properly defined with its type and fields - The component's TypeScript integration is correct with proper type imports and usage
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the GraphQL fragment definition
rg -A 10 'fragment EngagementPlanningSpreadsheet on LanguageEngagement'
# Search for pnpExtractionResult type definition
rg 'type.*PnpExtractionResult'
Length of output: 1283
Script:
#!/bin/bash
# Search for the pnpExtractionResult fragment definition
rg -A 10 'fragment pnpExtractionResult'
# Search for any type definitions or usages of PnpExtractionResult
rg -g '*.graphql' -g '*.ts' -g '*.tsx' 'PnpExtractionResult'
Length of output: 1634
52-61: Verify the preview functionality configuration
The component has disablePreview={true} but still uses the Preview component in the header. Please verify if this is intentional or if it could lead to inconsistent user experience.
✅ Verification successful
Preview component usage is intentionally separate from DefinedFileCard's preview functionality
The current implementation is correct. The disablePreview={true} prop on DefinedFileCard controls the card's internal preview functionality, while the separate Preview component in the header is an independent UI element that provides preview functionality specifically for the planning spreadsheet when a file exists (engagement.pnp.value). This separation of concerns allows for more flexible control over where and how the preview functionality is presented to users.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other usages of DefinedFileCard with disablePreview to understand the pattern
ast-grep --pattern 'DefinedFileCard $props disablePreview={$_}'
# Search for Preview component usage patterns
ast-grep --pattern 'Preview file={$_}'
Length of output: 106
Script:
#!/bin/bash
# Let's try a different approach to search for DefinedFileCard usage
rg -l "DefinedFileCard"
# Then let's look at the implementation of DefinedFileCard
rg -A 10 "export .* DefinedFileCard"
# Also search for Preview related code
rg -l "Preview.*file="
Length of output: 3262
Script:
#!/bin/bash
# Let's look at the DefinedFileCard implementation to understand how disablePreview is used
rg -A 20 "disablePreview.*," src/components/DefinedFileCard/DefinedFileCard.tsx
# Also check how Preview is used in PlanningSpreadsheet.tsx
cat src/scenes/Engagement/LanguageEngagement/PlanningSpreadsheet/PlanningSpreadsheet.tsx
Length of output: 5935
|
@CarsonF , I tried putting the API PR in the comments above, but it doesn't look like that is fixing the issue of this PR not recognizing the schema change. I can't remember if this is the issue that requires a specific label to be selected above, but I don't see one that looks familiar. Everything works on my local, so I'm guessing I'm not referencing something correctly or I'm missing a label that will ignore the schema change. |
Nevermind, got it working :) |
src/scenes/Engagement/LanguageEngagement/PlanningSpreadsheet/PlanningSpreadsheet.tsx
Outdated
Show resolved
Hide resolved
CarsonF
force-pushed
the
0242-PnP-Extraction-Validation-For-Planning-Uploads
branch
from
79a3ea9 to
1fd3ea9
Compare
CarsonF
deleted the
0242-PnP-Extraction-Validation-For-Planning-Uploads
branch
Monday
related: SeedCompany/cord-api-v3#3334
Summary by CodeRabbit
Release Notes
New Features
Previewcomponent for file previews, enhancing user interaction.pnpExtractionResultfield to provide detailed extraction data in the engagement planning spreadsheet.Bug Fixes
Previewcomponent from theProgressReportCard, streamlining the user interface.Enhancements
PlanningSpreadsheetto improve the visual presentation and interaction with file cards.