Plugin and advertisement screen revamp #2006
Conversation
WalkthroughThe changes in this pull request involve updates to various components and their related files in the codebase. Key modifications include terminology changes in the translation files, enhancements in the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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
Documentation and Community
|
Our Pull Request Approval ProcessThanks for contributing! Testing Your CodeRemember, your PRs won't be reviewed until these criteria are met:
Our policies make our code better. ReviewersDo not assign reviewers. Our Queue Monitors will review your PR and assign them.
Reviewing Your CodeYour reviewer(s) will have the following roles:
CONTRIBUTING.mdRead our CONTRIBUTING.md file. Most importantly:
Other
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Outside diff range and nitpick comments (12)
src/components/Advertisements/core/AdvertisementEntry/AdvertisementEntry.tsx (3)
Line range hint
77-81: Specify thetypeattribute for the button to ensure correct behavior in forms.+ type="button"
Line range hint
99-99: Add keyboard accessibility to the delete button by handling key events.+ onKeyUp={(event) => { if (event.key === 'Enter') toggleShowDeleteModal(); }}
Line range hint
147-147: Self-close JSX elements that don't have children.- <Card.Img className={styles.admedia} variant="top" src={mediaUrl} data-testid="media"></Card.Img> + <Card.Img className={styles.admedia} variant="top" src={mediaUrl} data-testid="media" />Also applies to: 158-158
src/components/AddOn/core/AddOnStore/AddOnStore.tsx (1)
Line range hint
75-75: Self-close JSX elements that don't have children.- <div className={styles.loader}></div> + <div className={styles.loader} />src/components/Advertisements/Advertisements.tsx (3)
Line range hint
53-53: Use optional chaining to safely access nested properties.- orgAdvertisementListData && orgAdvertisementListData.organizations + orgAdvertisementListData?.organizationsAlso applies to: 67-67
Line range hint
126-128: Self-close JSX elements that don't have children.- <div className={styles.itemCard}></div> + <div className={styles.itemCard} />Also applies to: 130-130, 205-207, 209-209
Line range hint
123-123: Avoid using the index of an array as a key in React lists.- key={index} + key={`ad-${index}`}Also applies to: 176-176, 202-202, 255-255
src/components/Advertisements/core/AdvertisementRegister/AdvertisementRegister.tsx (5)
Line range hint
217-217: Self-close JSX elements that don't have children.- <video></video> + <video />Also applies to: 312-312
Line range hint
221-221: Add keyboard accessibility to interactive elements by handling key events.+ onKeyUp={(event) => { if (event.key === 'Enter') handleShow(); }}
Line range hint
267-267: Use optional chaining to safely access nested properties.- if (error instanceof Error) { + if (error?.message) {
Line range hint
292-293: Provide alt text for images to improve accessibility.- <img src={formState.advertisementMedia} /> + <img src={formState.advertisementMedia} alt="Advertisement Media" />
Line range hint
295-311: Specify thetypeattribute for buttons to ensure correct behavior in forms.+ type="button"
Review Details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (15)
- public/locales/en.json (2 hunks)
- src/components/ActionItems/ActionItemsModal.tsx (2 hunks)
- src/components/AddOn/core/AddOnEntry/AddOnEntry.module.css (1 hunks)
- src/components/AddOn/core/AddOnEntry/AddOnEntry.tsx (5 hunks)
- src/components/AddOn/core/AddOnStore/AddOnStore.module.css (2 hunks)
- src/components/AddOn/core/AddOnStore/AddOnStore.tsx (6 hunks)
- src/components/AddOn/support/services/Plugin.helper.test.ts (1 hunks)
- src/components/AddOn/support/services/Plugin.helper.ts (1 hunks)
- src/components/Advertisements/Advertisements.module.css (2 hunks)
- src/components/Advertisements/Advertisements.tsx (7 hunks)
- src/components/Advertisements/core/AdvertisementEntry/AdvertisementEntry.module.css (1 hunks)
- src/components/Advertisements/core/AdvertisementEntry/AdvertisementEntry.tsx (6 hunks)
- src/components/Advertisements/core/AdvertisementRegister/AdvertisementRegister.module.css (1 hunks)
- src/components/Advertisements/core/AdvertisementRegister/AdvertisementRegister.tsx (2 hunks)
- src/components/EventCalendar/EventCalendar.module.css (1 hunks)
Files not reviewed due to errors (3)
- src/components/AddOn/core/AddOnStore/AddOnStore.module.css (no review received)
- src/components/AddOn/support/services/Plugin.helper.test.ts (no review received)
- src/components/AddOn/core/AddOnEntry/AddOnEntry.tsx (no review received)
Files skipped from review due to trivial changes (5)
- public/locales/en.json
- src/components/ActionItems/ActionItemsModal.tsx
- src/components/Advertisements/core/AdvertisementEntry/AdvertisementEntry.module.css
- src/components/Advertisements/core/AdvertisementRegister/AdvertisementRegister.module.css
- src/components/EventCalendar/EventCalendar.module.css
Additional Context Used
Biome (42)
src/components/AddOn/core/AddOnEntry/AddOnEntry.tsx (1)
102-104: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing.
src/components/AddOn/core/AddOnStore/AddOnStore.tsx (12)
75-75: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing.
108-108: Do not use template literals if interpolation and special-character handling are not needed.
144-144: Use === instead of ==.
== is only allowed when comparing againstnull
146-152: This else clause can be omitted because previous branches break early.
159-159: Use === instead of ==.
== is only allowed when comparing againstnull
161-167: This else clause can be omitted because previous branches break early.
202-208: This else clause can be omitted because previous branches break early.
218-218: Use === instead of ==.
== is only allowed when comparing againstnull
220-226: This else clause can be omitted because previous branches break early.
1-1: The default import is only used as a type.
174-174: Avoid using the index of an array as key property in an element.
233-233: Avoid using the index of an array as key property in an element.
src/components/AddOn/support/services/Plugin.helper.ts (1)
22-22: Do not use template literals if interpolation and special-character handling are not needed.
src/components/Advertisements/Advertisements.tsx (16)
38-38: Unexpected any. Specify a different type.
53-53: Change to an optional chain.
67-67: Change to an optional chain.
99-99: Do not use template literals if interpolation and special-character handling are not needed.
126-128: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing.
130-130: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing.
130-130: Provide screen reader accessible content when using heading elements.
205-207: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing.
209-209: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing.
209-209: Provide screen reader accessible content when using heading elements.
1-1: The default import is only used as a type.
52-52: This hook does not specify all of its dependencies: advertisements
123-123: Avoid using the index of an array as key property in an element.
176-176: Avoid using the index of an array as key property in an element.
202-202: Avoid using the index of an array as key property in an element.
255-255: Avoid using the index of an array as key property in an element.
src/components/Advertisements/core/AdvertisementEntry/AdvertisementEntry.tsx (5)
99-99: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event.
147-147: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing.
158-158: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing.
74-74: Avoid using the index of an array as key property in an element.
77-81: Provide an explicit type prop for the button element.
src/components/Advertisements/core/AdvertisementRegister/AdvertisementRegister.tsx (7)
217-217: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing.
221-221: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event.
267-267: Change to an optional chain.
292-293: Provide a text alternative through the alt, aria-label or aria-labelledby attribute
312-312: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing.
1-1: The default import is only used as a type.
295-311: Provide an explicit type prop for the button element.
Additional comments not posted (9)
src/components/AddOn/core/AddOnEntry/AddOnEntry.module.css (2)
10-11: Updated background and text color for.entryactionclass enhances visual consistency and appeal.
14-14: Green border addition to.cardclass aligns with the redesign objectives to make UI elements more visually appealing.src/components/Advertisements/Advertisements.module.css (3)
4-10: Updated.listBoxto use grid layout enhances alignment and spacing, improving the visual structure of the component.
21-25: Updated.inputclass with specific width and position settings enhances control over the layout and appearance.
26-30: Updated.justifyspto use grid layout enhances alignment and spacing, improving the visual structure of the component.src/components/AddOn/support/services/Plugin.helper.ts (3)
1-12: Introduction ofInterfacePluginHelperenhances type safety and clarity in handling plugin data.
26-31: Refinement ofgenerateLinksto useInterfacePluginHelperenhances data handling and consistency.
15-18: Ensure type consistency by updating the return type offetchStoretoPromise<InterfacePluginHelper[]>.- const result = await fetch(`http://localhost:${port}/store`); + const result = await fetch('http://localhost:4321/store');Avoid using template literals when not necessary for interpolation.
Likely invalid or redundant comment.
src/components/Advertisements/core/AdvertisementEntry/AdvertisementEntry.tsx (1)
21-21: Ensure thesetAfterfunction is properly documented and tested, especially since it's part of the component's interface.
src/components/Advertisements/core/AdvertisementEntry/AdvertisementEntry.tsx
Outdated
Show resolved
Hide resolved
src/components/Advertisements/core/AdvertisementEntry/AdvertisementEntry.tsx
Show resolved
Hide resolved
|
Please make the suggested code rabbit updates to the code |
|
Please fix the conflicting files and address the coderabbit suggestions |
|
sorry for late response i'll make the suggested changes |
|
This pull request did not get any activity in the past 10 days and will be closed in 180 days if no update occurs. Please verify it has no conflicts with the develop branch and rebase if needed. Mention it now if you need help or give permission to other people to finish your work. |
|
Please fix the conflicting files. |
|
This pull request did not get any activity in the past 10 days and will be closed in 180 days if no update occurs. Please verify it has no conflicts with the develop branch and rebase if needed. Mention it now if you need help or give permission to other people to finish your work. |
|
Closing. Inactivity and many conflicting files |
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (7)
src/components/AddOn/core/AddOnEntry/AddOnEntry.tsx (3)
62-62: Remove commented console.log statementDebug statements should be removed before committing to production code.
- // console.log(currentOrg);
105-108: Move inline styles to CSS moduleConsider moving the Card's inline styles to the existing
AddOnEntry.module.cssfile for better maintainability and consistency.- <Card - data-testid="AddOnEntry" - style={{ border: '1px solid #31BB6B', borderRadius: '10px' }} - > + <Card + data-testid="AddOnEntry" + className={styles.addOnCard} + >Add to AddOnEntry.module.css:
.addOnCard { border: 1px solid #31BB6B; border-radius: 10px; }
121-121: Move title font weight to CSS moduleConsider moving the Card.Title's inline style to the CSS module for consistency.
- <Card.Title style={{ fontWeight: '800' }}>{title}</Card.Title> + <Card.Title className={styles.cardTitle}>{title}</Card.Title>Add to AddOnEntry.module.css:
.cardTitle { font-weight: 800; }src/components/Advertisements/Advertisements.tsx (2)
103-103: Use translation for placeholder textThe placeholder text should use the translation system for consistency with the rest of the application.
-placeholder={'Search..'} +placeholder={t('searchPlaceholder')}
Line range hint
126-275: Reduce code duplicationThe InfiniteScroll implementation and date filtering logic is duplicated between the active and archived tabs. Consider:
- Creating a reusable component for the InfiniteScroll implementation
- Extracting the date comparison logic into a utility function
Example refactor for the date logic:
const isAdvertisementActive = (ad: Ad): boolean => new Date(ad.endDate) > new Date(); const isAdvertisementArchived = (ad: Ad): boolean => !isAdvertisementActive(ad); // Usage in JSX: advertisements.filter(isAdvertisementActive).map(...) advertisements.filter(isAdvertisementArchived).map(...)src/components/AddOn/core/AddOnStore/AddOnStore.tsx (2)
45-45: Remove unused state variable for cleaner code.The state variable
dataListis not used anywhere in the code. Removing it can simplify the component and reduce unnecessary state management.Apply this diff to remove the unused state variable:
- const [, setDataList] = useState<InterfacePluginHelper[]>([]); + // Removed unused state variable
232-232: Use strict equality checks (===) for accurate comparisons.Replace
==with===when comparingsearchTextto ensure strict type comparison and avoid unexpected behavior.Apply this diff:
- if (searchText == '') { + if (searchText === '') {Also applies to lines: 176, 233
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
public/locales/en/translation.json(2 hunks)src/components/AddOn/core/AddOnEntry/AddOnEntry.tsx(5 hunks)src/components/AddOn/core/AddOnStore/AddOnStore.tsx(9 hunks)src/components/Advertisements/Advertisements.tsx(5 hunks)src/components/Advertisements/core/AdvertisementEntry/AdvertisementEntry.tsx(3 hunks)src/components/Advertisements/core/AdvertisementRegister/AdvertisementRegister.module.css(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- public/locales/en/translation.json
🔇 Additional comments (4)
src/components/AddOn/core/AddOnEntry/AddOnEntry.tsx (2)
22-22: LGTM: Improved type safety
The change from any to void return type for getInstalledPlugins is a good improvement in type safety.
Line range hint 1-153: Add test coverage for the UI changes
The PR objectives mention that no tests were added for these changes. Consider adding tests for:
- Visual regression tests for the new styling
- Unit tests for the installation/uninstallation logic
- Integration tests for the plugin management workflow
Would you like help creating the test cases?
src/components/Advertisements/Advertisements.tsx (2)
237-237: Verify translation key exists
The 'endOfResults' translation key has been moved from the common namespace to the advertisement namespace. Please ensure this key exists in the advertisement translations.
✅ Verification successful
Translation key exists in both namespaces
The 'endOfResults' translation key exists in both the common namespace (common.json) and the default namespace (translation.json) for all supported languages (en, fr, hi, sp, zh). The change from tCommon('endOfResults') to t('endOfResults') is safe as the key is present in both locations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the translation key exists in the advertisement namespace
fd -e json . public/locales | xargs rg '"endOfResults"'
Length of output: 1823
122-126:
Fix spelling in tab key
The tab key 'archievedAds' contains a spelling error.
-eventKey="archievedAds"
+eventKey="archivedAds"Likely invalid or redundant comment.
src/components/Advertisements/core/AdvertisementRegister/AdvertisementRegister.module.css
Show resolved
Hide resolved
src/components/Advertisements/core/AdvertisementEntry/AdvertisementEntry.tsx
Show resolved
Hide resolved
src/components/Advertisements/core/AdvertisementEntry/AdvertisementEntry.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
src/components/AddOn/core/AddOnStore/AddOnStore.test.tsx (2)
323-334: LGTM! Consider enhancing test coverage.The updated test assertions correctly reflect the new dropdown-based filter UI. However, consider adding these test scenarios for more comprehensive coverage:
- Verify that selecting different filter options updates the displayed plugins
- Test edge cases like rapid toggling of the dropdown
- Ensure proper keyboard navigation support
Add these test cases:
// Test filter functionality test('filters plugins correctly when dropdown option is selected', async () => { // ... setup code ... const dropdownButton = screen.getByRole('button', { name: /enabled/i }); userEvent.click(dropdownButton); // Select "Disabled" option userEvent.click(screen.getByText(/disabled/i)); // Verify only disabled plugins are shown expect(screen.queryByText('EnabledPlugin')).not.toBeInTheDocument(); expect(screen.getByText('DisabledPlugin')).toBeInTheDocument(); }); // Test keyboard navigation test('supports keyboard navigation in dropdown', async () => { // ... setup code ... const dropdownButton = screen.getByRole('button', { name: /enabled/i }); // Open dropdown with Enter key dropdownButton.focus(); fireEvent.keyDown(dropdownButton, { key: 'Enter' }); // Verify dropdown options are focusable expect(screen.getByText(/disabled/i)).toHaveFocus(); });
Line range hint
1-424: Consider enhancing overall test structure and coverage.While the test file is well-organized, consider these improvements for better maintainability and coverage:
- Extract mock data to a separate file to improve readability
- Add more varied test cases for error scenarios
- Enhance the loading test to verify loading states for both tabs
Here's a suggested approach for the loading test:
test('displays loading states correctly', async () => { const mocks = [ORGANIZATIONS_LIST_MOCK, PLUGIN_LOADING_MOCK]; render( // ... setup code ... ); // Verify initial loading state expect(screen.getByTestId('AddOnEntryStore')).toBeInTheDocument(); expect(screen.getByRole('progressbar')).toBeInTheDocument(); // Switch to Installed tab userEvent.click(screen.getByText('Installed')); expect(screen.getByRole('progressbar')).toBeInTheDocument(); // Wait for loading to complete await wait(); expect(screen.queryByRole('progressbar')).not.toBeInTheDocument(); });Consider moving mock data to a separate file:
// __mocks__/addOnStore.mocks.ts export const PLUGIN_GET_MOCK = { // ... existing mock data ... }; export const ORGANIZATIONS_LIST_MOCK = { // ... existing mock data ... };src/components/Advertisements/Advertisements.test.tsx (4)
464-464: Consider making date validation tests more robust.The change from
date[1]todate[0]suggests a dependency on array order. Consider making the test more resilient by:
- Adding explicit assertions about the expected number of date elements
- Using a more specific test ID or data attribute to target the exact date element
- const dateString = date[0].innerHTML; + expect(date).toHaveLength(2); // Verify expected number of dates + const dateString = date.find(el => el.getAttribute('data-type') === 'end-date')?.innerHTML; + expect(dateString).toBeDefined(); // Ensure we found the correct date element
Line range hint
676-677: Remove console.log statements from test code.Debug logging statements should not be committed to test files as they add noise to test output.
- console.log('before scroll', moreiconbtn); - fireEvent.scroll(window, { target: { scrollY: 500 } }); + fireEvent.scroll(window, { target: { scrollY: 500 } });
Line range hint
679-679: Remove console.log statement.Debug logging statement should be removed.
- console.log('after scroll', moreiconbtn);
Line range hint
462-482: Consider using a mock date to make tests deterministic.The test relies on comparing dates with the current time, which can make tests non-deterministic. Consider using a mocked date for consistent test results.
+ // Mock current date to ensure consistent test results + const mockCurrentDate = new Date('2024-01-01'); + jest.spyOn(global, 'Date').mockImplementation(() => mockCurrentDate); const date = await screen.findAllByTestId('Ad_end_date'); const dateString = date[0].innerHTML; const dateMatch = dateString.match( /\b(?:Sun|Mon|Tue|Wed|Thu|Fri|Sat)\s+(Jan|Feb|Mar|Apr|May|Jun|Jul|Aug|Sep|Oct|Nov|Dec)\s+(\d{1,2})\s+(\d{4})\b/, ); let dateObject = new Date(); if (dateMatch) { const monthName = dateMatch[1]; const day = parseInt(dateMatch[2], 10); const year = parseInt(dateMatch[3], 10); const monthIndex = 'JanFebMarAprMayJunJulAugSepOctNovDec'.indexOf(monthName) / 3; dateObject = new Date(year, monthIndex, day); } - expect(dateObject.getTime()).toBeLessThan(new Date().getTime()); + expect(dateObject.getTime()).toBeLessThan(mockCurrentDate.getTime()); + + // Restore Date after test + jest.restoreAllMocks();
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
src/components/AddOn/core/AddOnStore/AddOnStore.test.tsx(1 hunks)src/components/Advertisements/Advertisements.test.tsx(1 hunks)
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop-postgres #2006 +/- ##
=====================================================
- Coverage 95.82% 83.75% -12.07%
=====================================================
Files 295 312 +17
Lines 7036 8114 +1078
Branches 1519 1771 +252
=====================================================
+ Hits 6742 6796 +54
- Misses 98 1179 +1081
+ Partials 196 139 -57 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
src/components/AddOn/core/AddOnStore/AddOnStore.tsx (2)
102-113: Enhance robustness of plugin filtering.The current implementation has potential issues with null safety and case handling.
Consider this improved implementation:
const filterPlugins = ( plugins: InterfacePluginHelper[], searchTerm: string, ): InterfacePluginHelper[] => { - if (!searchTerm) { + if (!searchTerm.trim()) { return plugins; } + const normalizedSearch = searchTerm.toLowerCase().trim(); return plugins.filter((plugin) => - plugin.pluginName?.toLowerCase().includes(searchTerm.toLowerCase()), + plugin.pluginName.toLowerCase().includes(normalizedSearch), ); };
126-141: Move inline styles to CSS modules.Inline styles make the code harder to maintain and don't support media queries or pseudo-classes. Consider moving these styles to your CSS module.
Move the styles to
AddOnStore.module.css:.container { background-color: white; margin: 2px; padding: 10px; border-radius: 20px; } .headerCol { display: flex; align-items: center; justify-content: space-between; }Then update the JSX:
- <Row - className="" - style={{ - backgroundColor: 'white', - margin: '2px', - padding: '10px', - borderRadius: '20px', - }} - > + <Row className={styles.container}> - <Col - style={{ - display: 'flex', - alignItems: 'center', - justifyContent: 'space-between', - }} - > + <Col className={styles.headerCol}>
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
src/components/AddOn/core/AddOnStore/AddOnStore.test.tsx(2 hunks)src/components/AddOn/core/AddOnStore/AddOnStore.tsx(7 hunks)src/components/Advertisements/Advertisements.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/AddOn/core/AddOnStore/AddOnStore.test.tsx
- src/components/Advertisements/Advertisements.tsx
Bumps [sass](https://github.com/sass/dart-sass) from 1.80.6 to 1.80.7. - [Release notes](https://github.com/sass/dart-sass/releases) - [Changelog](https://github.com/sass/dart-sass/blob/main/CHANGELOG.md) - [Commits](sass/dart-sass@1.80.6...1.80.7) --- updated-dependencies: - dependency-name: sass dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…oesFoundation#2434) Bumps [eslint-plugin-import](https://github.com/import-js/eslint-plugin-import) from 2.30.0 to 2.31.0. - [Release notes](https://github.com/import-js/eslint-plugin-import/releases) - [Changelog](https://github.com/import-js/eslint-plugin-import/blob/main/CHANGELOG.md) - [Commits](import-js/eslint-plugin-import@v2.30.0...v2.31.0) --- updated-dependencies: - dependency-name: eslint-plugin-import dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…dation#2435) Bumps [@mui/x-charts](https://github.com/mui/mui-x/tree/HEAD/packages/x-charts) from 7.22.1 to 7.22.2. - [Release notes](https://github.com/mui/mui-x/releases) - [Changelog](https://github.com/mui/mui-x/blob/v7.22.2/CHANGELOG.md) - [Commits](https://github.com/mui/mui-x/commits/v7.22.2/packages/x-charts) --- updated-dependencies: - dependency-name: "@mui/x-charts" dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…dation#2436) Bumps [@types/react](https://github.com/DefinitelyTyped/DefinitelyTyped/tree/HEAD/types/react) from 18.3.3 to 18.3.12. - [Release notes](https://github.com/DefinitelyTyped/DefinitelyTyped/releases) - [Commits](https://github.com/DefinitelyTyped/DefinitelyTyped/commits/HEAD/types/react) --- updated-dependencies: - dependency-name: "@types/react" dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
* added docker check to workflow * made recommended changes to docker check in workflow * added changes to docker check inn workflow as recommended * added changes * updated indentation in pull-request.yml file * updated indentation in pull-request.yml file * added Dockerfile and Docker-compose.yml file * added Dockerfile and Docker-compose.yml file * updated .docker-ignore file * added recommended changes by code rabbit * added recommended changes by code rabbit * added recommended changes by code rabbit * added recommended changes by code rabbit * added recommended changes by code rabbit * added recommended changes by code rabbit * properly formatted code * trying to make docker check pass * trying to make docker check pass * updated INSTALLATION.md * updated INSTALLATION.md * added recommended changes to INSTALLATION.md * added recommended changes to INSTALLATION.md * added recommended changes to INSTALLATION.md * updated docker workflow * updated INSTALLATION.md
* Updated pr template with checklist * Additional changes for the PR template * Changed the url for the PR template
duplixx
dismissed
coderabbitai[bot]’s stale review
The base branch was changed.
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
.github/workflows/codeql-codescan.yml (1)
37-37: Fix YAML indentation for consistency.The
withblock should be indented with 11 spaces to maintain consistent alignment with other blocks in the workflow.Apply this fix:
uses: github/codeql-action/init@v3 - with: + with: languages: ${{ matrix.language }}🧰 Tools
🪛 yamllint
[warning] 37-37: wrong indentation: expected 11 but found 10
(indentation)
.github/workflows/push.yml (1)
56-56: Consider separating CI/CD changes into a dedicated PR.This workflow change appears unrelated to the PR's stated objective of redesigning the advertisement and plugin screens. To maintain clear change boundaries and simplify review/testing, consider moving this CI/CD update to a separate PR focused on workflow improvements.
.github/workflows/pull-request.yml (1)
Line range hint
209-213: Consider type checking dependencies of changed filesWhile compiling only changed TypeScript files is more efficient, it might miss type errors in dependent files. Consider:
- Using TypeScript's
--buildmode to include dependent files- Running full type checks periodically
-for file in ${{ steps.changed-files.outputs.all_changed_files }}; do - if [[ "$file" == *.ts || "$file" == *.tsx ]]; then - npx tsc --noEmit "$file" - fi -done +# Create a temporary tsconfig for changed files +echo '{ + "extends": "./tsconfig.json", + "files": [] +}' > tsconfig.changed.json + +# Add changed files to the config +for file in ${{ steps.changed-files.outputs.all_changed_files }}; do + if [[ "$file" == *.ts || "$file" == *.tsx ]]; then + echo $(jq --arg file "$file" '.files += [$file]' tsconfig.changed.json) > tsconfig.changed.json + fi +done + +# Build with dependencies +npx tsc --build tsconfig.changed.json🧰 Tools
🪛 actionlint
208-208: shellcheck reported issue in this script: SC2043:warning:1:13: This loop will only ever run once. Bad quoting or missing glob/expansion?
(shellcheck)
🛑 Comments failed to post (1)
.github/workflows/push.yml (1)
56-56: 💡 Codebase verification
Update all Codecov action references to v5 for consistency
Found inconsistent versions of Codecov action across workflow files:
.github/workflows/push.yml: Using v5.github/workflows/pull-request.yml: Using v4The v5 action is compatible with the current setup as it doesn't introduce breaking changes in the required inputs. However, using different versions across workflows may lead to inconsistent coverage reporting behavior.
🔗 Analysis chain
Verify compatibility with Codecov action v5.
The upgrade from
codecov/codecov-action@v4tov5is a major version change that could introduce breaking changes.Let's verify the requirements and changes:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check Codecov action version compatibility and requirements # Get the current Node.js version from the workflow NODE_VERSION=$(grep 'node-version:' .github/workflows/push.yml | head -n 1 | awk -F'[' '{print $2}' | tr -d '[]') # Fetch Codecov action v5 requirements echo "Checking Codecov action v5 requirements..." curl -s https://raw.githubusercontent.com/codecov/codecov-action/v5/action.yml # Check if there are any other Codecov action usages that need to be updated echo "Checking for other Codecov action usages..." rg "codecov/codecov-action@v" .github/workflows/Length of output: 11452
|
Are you still working on this? |
revamped has already been done here, i guess there are some conflict after the branch change.. I'll fix it so that it can be merged ASAP |
…advertisement-revamp
palisadoes
added
the
ignore-sensitive-files-pr
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/components/AddOn/core/AddOnStore/AddOnStore.tsx (2)
100-111: Improve null safety in filterPlugins function.The function should handle cases where
pluginNameis undefined more gracefully.const filterPlugins = ( plugins: InterfacePluginHelper[], searchTerm: string, ): InterfacePluginHelper[] => { if (!searchTerm) { return plugins; } return plugins.filter((plugin) => - plugin.pluginName?.toLowerCase().includes(searchTerm.toLowerCase()), + plugin.pluginName?.toLowerCase().includes(searchTerm.toLowerCase()) ?? false, ); };
125-139: Move inline styles to CSS module.Consider moving the inline styles to the CSS module for better maintainability and consistency with the existing styling approach.
- <Row - className="" - style={{ - backgroundColor: 'white', - margin: '2px', - padding: '10px', - borderRadius: '20px', - }} - > + <Row className={styles.storeContainer}> - <Col - style={{ - display: 'flex', - alignItems: 'center', - justifyContent: 'space-between', - }} - > + <Col className={styles.storeHeader}>Add to AddOnStore.module.css:
.storeContainer { background-color: white; margin: 2px; padding: 10px; border-radius: 20px; } .storeHeader { display: flex; align-items: center; justify-content: space-between; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (1)
src/components/AddOn/core/AddOnStore/AddOnStore.tsx(6 hunks)
🔇 Additional comments (3)
src/components/AddOn/core/AddOnStore/AddOnStore.tsx (3)
13-24: Improve type safety in the InterfacePluginHelper interface.
The interface has several issues that could be improved:
- Redundant fields:
nameandpluginNameserve the same purpose - Optional fields that appear to be required in usage
componentfield is hardcoded but typed as string
Apply this diff to improve type safety:
interface InterfacePluginHelper {
_id: string;
- pluginName?: string;
- pluginDesc?: string;
+ pluginName: string; // Required as it's used in rendering
+ pluginDesc: string; // Required as it's used in rendering
pluginCreatedBy: string;
- pluginInstallStatus?: boolean;
+ pluginInstallStatus: boolean;
uninstalledOrgs: string[];
installed: boolean;
enabled: boolean;
- name: string; // Remove redundant field
- component: string;
+ component: 'Special Component'; // Use literal type
}44-50: LGTM! Well-typed state management.
The state management and GraphQL query are properly typed with TypeScript generics, making the code type-safe.
215-228: Refactor duplicate plugin rendering logic and fix React keys.
There are two issues in the rendering logic:
- Using array indices as keys (lines 216, 254)
- Duplicated plugin rendering logic between tabs
Extract the common rendering logic into a reusable component and use unique plugin IDs as keys:
interface PluginListProps {
plugins: InterfacePluginHelper[];
getInstalledPlugins: () => void;
}
const PluginList: React.FC<PluginListProps> = ({ plugins, getInstalledPlugins }) => (
<div className={styles.justifysp}>
{plugins.map((plug) => (
<div className={styles.cardGridItem} key={plug._id}>
<AddOnEntry
id={plug._id}
title={plug.pluginName}
description={plug.pluginDesc}
createdBy={plug.pluginCreatedBy}
component={'Special Component'}
modified={true}
getInstalledPlugins={getInstalledPlugins}
uninstalledOrgs={plug.uninstalledOrgs}
/>
</div>
))}
</div>
);Also applies to: 253-266
What kind of change does this PR introduce?
Redesign
Issue Number:
Fixes #1918
Did you add tests for your changes?
No
Snapshots/Videos:
If relevant, did you update the documentation?
Summary
Does this PR introduce a breaking change?
Other information
Have you read the contributing guide?
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Style