Add sync status and failure reason to sheet list page #3595
Conversation
list Display sync status and failure reason in the SheetIntegrationList with appropriate column styling and GraphQL query updates.
WalkthroughThis PR adds per-sheet sync feedback to the Sheet Integration list. GraphQL query and mutation selections now include Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ 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). (4)
🔇 Additional comments (1)
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 |
|
🚀 Deployed on https://deploy-preview-3595--glific-frontend.netlify.app |
![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
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/containers/SheetIntegration/SheetIntegrationList/SheetIntegrationList.module.css(1 hunks)src/containers/SheetIntegration/SheetIntegrationList/SheetIntegrationList.test.tsx(1 hunks)src/containers/SheetIntegration/SheetIntegrationList/SheetIntegrationList.tsx(2 hunks)src/graphql/queries/Sheet.ts(1 hunks)src/i18n/en/en.json(1 hunks)src/mocks/Sheet.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
Files:
src/containers/SheetIntegration/SheetIntegrationList/SheetIntegrationList.test.tsxsrc/containers/SheetIntegration/SheetIntegrationList/SheetIntegrationList.tsxsrc/graphql/queries/Sheet.tssrc/mocks/Sheet.tsx
⏰ 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). (4)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: CI
- GitHub Check: glific (1.18.3-otp-27, 27.3.3)
- GitHub Check: build
🔇 Additional comments (10)
src/i18n/en/en.json (1)
351-352: LGTM!Translation entries for the new columns are correctly added and follow the existing pattern.
src/containers/SheetIntegration/SheetIntegrationList/SheetIntegrationList.tsx (4)
44-50: LGTM!The rendering functions correctly handle the display of sync status and failure reason with appropriate fallbacks to 'N/A'. The
titleattribute on the failure reason is a good UX touch for viewing truncated text.
52-59: LGTM!The
columnStylesarray is correctly updated to include the new column styles, maintaining proper order alignment with the column data.
158-164: LGTM!The
getColumnsfunction correctly integrates the new fields. The parameters and return values properly wire up the sync status and failure reason data to their respective renderers.
170-171: LGTM!Column names are correctly defined with proper translation keys that align with the i18n additions.
src/graphql/queries/Sheet.ts (1)
36-37: LGTM!The GraphQL query correctly includes the new
syncStatusandfailureReasonfields to support the UI enhancements.src/containers/SheetIntegration/SheetIntegrationList/SheetIntegrationList.test.tsx (2)
156-168: LGTM!The test correctly verifies that the new column headers are rendered. The test structure follows the existing patterns in the file.
170-189: LGTM!The test effectively verifies that sync status and failure reason values are correctly rendered, covering both success and failure scenarios from the mock data.
src/mocks/Sheet.tsx (2)
63-64: LGTM!Mock data correctly includes sync status and failure reason for the success case, aligning with test expectations.
77-78: LGTM!Mock data correctly includes sync status and failure reason for the failure case with a descriptive error message, providing good test coverage.
src/containers/SheetIntegration/SheetIntegrationList/SheetIntegrationList.module.css
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3595 +/- ##
==========================================
+ Coverage 82.55% 82.57% +0.02%
==========================================
Files 341 341
Lines 11424 11432 +8
Branches 2418 2422 +4
==========================================
+ Hits 9431 9440 +9
+ Misses 1259 1258 -1
Partials 734 734 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Glific
|
||||||||||||||||||||||||||||
| Project |
Glific
|
| Branch Review |
show-sync-status-and-failure-reasons
|
| Run status |
|
| Run duration | 27m 32s |
| Commit |
|
| Committer | Shijith Karumathil |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
1
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
182
|
| View all changes introduced in this branch ↗︎ | |
Update sheet integration to use syncStatus and failureReason fields instead of warnings. Change dialog to display single failure message with centered text alignment.
|
when we select the allowed operation as read/write, but the Google Sheet is set to read-only, the system still allows saving the sheet. |
Can we update the visual cues for the sync status to match the format used for the webhook response status? |
Can you please fix the spacing between the columns? |
Can you explain this a bit better? I am not sure I understood it correctly @priyanshu6238 |
Replaces plain text sync status with styled badges for SUCCESS, FAILED, and PENDING states. Adjusts column widths to accommodate the new badge design.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/containers/SheetIntegration/SheetIntegrationList/SheetIntegrationList.module.css (1)
45-50: Fix invalidmin-widthvalue and add missingwhite-spaceproperty.Line 46 has an invalid CSS value
min-width: 30without a unit. This will be ignored by browsers. Additionally, as noted in a previous review,text-overflow: ellipsisrequireswhite-space: nowrapto function correctly.Apply this diff to fix both issues:
.FailureReason { - min-width: 30; + min-width: 150px; width: 20%; overflow: hidden; text-overflow: ellipsis; + white-space: nowrap; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/containers/SheetIntegration/SheetIntegrationList/SheetIntegrationList.module.css(2 hunks)src/containers/SheetIntegration/SheetIntegrationList/SheetIntegrationList.test.tsx(5 hunks)src/containers/SheetIntegration/SheetIntegrationList/SheetIntegrationList.tsx(4 hunks)src/graphql/mutations/Sheet.ts(1 hunks)src/i18n/en/en.json(1 hunks)src/mocks/Sheet.tsx(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/containers/SheetIntegration/SheetIntegrationList/SheetIntegrationList.tsx
- src/i18n/en/en.json
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
Files:
src/mocks/Sheet.tsxsrc/containers/SheetIntegration/SheetIntegrationList/SheetIntegrationList.test.tsxsrc/graphql/mutations/Sheet.ts
🧬 Code graph analysis (1)
src/containers/SheetIntegration/SheetIntegrationList/SheetIntegrationList.test.tsx (1)
src/mocks/Sheet.tsx (1)
syncSheetMutationWithFailure(194-219)
🪛 Biome (2.1.2)
src/containers/SheetIntegration/SheetIntegrationList/SheetIntegrationList.module.css
[error] 42-42: Duplicate properties can lead to unexpected behavior and may override previous declarations unintentionally.
min-width is already defined here.
Remove or rename the duplicate property to ensure consistent styling.
(lint/suspicious/noDuplicateProperties)
⏰ 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). (4)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build
- GitHub Check: glific (1.18.3-otp-27, 27.3.3)
- GitHub Check: CI
🔇 Additional comments (11)
src/graphql/mutations/Sheet.ts (1)
80-81: LGTM!The addition of
syncStatusandfailureReasonfields to the mutation response is clean and aligns well with the UI changes and mock data updates throughout the PR.src/mocks/Sheet.tsx (3)
63-64: LGTM!The mock data provides good coverage with both success and failure scenarios. The first sheet demonstrates a successful sync with
nullfailure reason, while the second shows a failed sync with a descriptive error message.Also applies to: 77-78
184-185: LGTM!The mutation mock correctly includes the new fields for the success case, maintaining consistency with the query mock data.
194-221: Good semantic rename with backward compatibility.The rename from
syncSheetMutationWithWarningstosyncSheetMutationWithFailurebetter reflects the actual behavior. The backward compatibility alias on Line 221 is a thoughtful addition that prevents breaking existing test code.src/containers/SheetIntegration/SheetIntegrationList/SheetIntegrationList.module.css (3)
31-34: LGTM!The width adjustment from 20% to 15% for the LastSync column makes room for the new Sync Status and Failure Reason columns.
52-55: LGTM!The width adjustment from 30% to 25% is consistent with the overall column layout changes to accommodate the new Sync Status and Failure Reason columns.
71-105: LGTM! Well-structured badge styles.The badge implementation follows good CSS patterns with a shared base class (
SyncStatusBadge) and composable variants. The color schemes are appropriate and semantically clear:
- Green for success
- Red for error
- Blue for pending
The centered text alignment for
DialogContentimproves the dialog presentation.src/containers/SheetIntegration/SheetIntegrationList/SheetIntegrationList.test.tsx (4)
14-14: LGTM!The import update aligns with the semantic rename in the mock file.
97-121: LGTM! Test properly updated for failure dialog.The test correctly validates:
- The "Sync Failed" dialog title
- The detailed failure message from the mock
- Dialog dismissal behavior
139-157: LGTM!The test correctly verifies that clicking the cross icon dismisses the "Sync Failed" dialog.
159-192: Excellent test coverage for new columns!The two new tests provide comprehensive validation:
- Column headers test: Verifies the UI structure includes "Sync Status" and "Failure Reason" columns
- Values test: Confirms correct display of both SUCCESS and FAILED statuses along with the failure reason message
This ensures both the presence and correctness of the new functionality.
src/containers/SheetIntegration/SheetIntegrationList/SheetIntegrationList.module.css
Show resolved
Hide resolved
priyanshu6238
left a comment
priyanshu6238
left a comment
There was a problem hiding this comment.
The patch is failing ,can you please fix it?
- Removes PendingBadge CSS class and its usage in the status handler. - Also adjusts SyncStatus column width from 120px to 150px.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/containers/SheetIntegration/SheetIntegrationList/SheetIntegrationList.module.css (1)
44-49: Addwhite-space: nowrapfor ellipsis to work correctly.The
text-overflow: ellipsisproperty (line 48) requireswhite-space: nowrapto function. Without it, long text will wrap to multiple lines instead of being truncated with an ellipsis. This was flagged in a previous review and remains unresolved.Apply this diff to fix:
.FailureReason { min-width: 30px; width: 20%; overflow: hidden; text-overflow: ellipsis; + white-space: nowrap; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/containers/SheetIntegration/SheetIntegrationList/SheetIntegrationList.module.css(2 hunks)src/containers/SheetIntegration/SheetIntegrationList/SheetIntegrationList.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/containers/SheetIntegration/SheetIntegrationList/SheetIntegrationList.tsx
⏰ 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). (4)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build
- GitHub Check: CI
- GitHub Check: glific (1.18.3-otp-27, 27.3.3)
🔇 Additional comments (1)
src/containers/SheetIntegration/SheetIntegrationList/SheetIntegrationList.module.css (1)
78-98: Badge styling classes look good.The new
SyncStatusBadge,SuccessBadge, andErrorBadgeclasses follow a clean composition pattern using CSS Modules'composesfeature. Styling is appropriate for status indicators.
src/containers/SheetIntegration/SheetIntegrationList/SheetIntegrationList.module.css
Outdated
Show resolved
Hide resolved
Fixed @priyanshu6238 |
|
just this one thing otherwise looks good to me |
Display sync status and failure reason in the SheetIntegrationList with appropriate column styling and GraphQL query updates.
Summary by CodeRabbit
Screenshot