Improved the "/sponsors" command for slack #630
Conversation
|
hey @arkid15r this pr is ready for review!!!! |
abhayymishraa
force-pushed
the
feat/sponsors
branch
from
977492b to
8d3b4f2
Compare
|
@arkid15r It is ready for the review !!! |
![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 (8)
frontend/tailwind.config.js (1)
56-64: Consider optimizing the scrolling animation parameters.The infinite scrolling animation with a very short duration (0.5s) may cause visual distraction and potential performance issues, especially on lower-end devices. The animation runs continuously and at a fast pace, which might not provide the best user experience.
Consider these improvements:
- Increase the animation duration for a smoother, less distracting effect
- Add a pause between iterations using
animation-delay- Consider using
transform: translateX(calc(-50% - var(--gap)))for a more responsive approachkeyframes: { scroll: { '0%': { transform: 'translateX(0)' }, - '100%': { transform: 'translateX(-500%)' }, + '100%': { transform: 'translateX(-100%)' }, }, }, animation: { - scroll: 'scroll 0.5s linear infinite', + scroll: 'scroll 15s linear infinite', },backend/apps/owasp/graphql/queries/sponsors.py (1)
10-17: Consider adding pagination to the sponsors query.The current implementation returns all sponsor objects without pagination, which may lead to performance issues if the number of sponsors grows significantly. GraphQL best practices recommend implementing pagination for list queries.
Here's how you could implement pagination:
class SponsorQuery(BaseQuery): """Sponsor queries.""" - sponsors = graphene.List(SponsorNode) + sponsors = graphene.List( + SponsorNode, + first=graphene.Int(), + offset=graphene.Int(), + ) - def resolve_sponsors(root, info): + def resolve_sponsors(root, info, first=None, offset=None): """Resolve sponsors.""" - return Sponsor.objects.all() + queryset = Sponsor.objects.all() + if offset is not None: + queryset = queryset[offset:] + if first is not None: + queryset = queryset[:first] + return querysetbackend/apps/owasp/migrations/0022_sponsor.py (1)
12-79: Consider adding explicit indexing if you anticipate frequent sponsor lookups.
While thekeyfield is already unique, thename,sort_name, or other fields might benefit from dedicated indexes if they are frequently searched or filtered.frontend/src/components/MovingLogo.tsx (3)
15-22: Avoid direct DOM manipulation withinnerHTML.
UsinginnerHTML += ...can be risky (e.g., potential XSS with untrusted input) and bypasses React's virtual DOM. Consider duplicating children using React state or a mapping approach for a more maintainable solution.
38-39: Provide a reliable fallback for missing sponsor images.
Currently, ifimageUrlis blank, you fall back to something like “ logo” as the src, which may cause a 404. You might consider a default image asset or explicitly handle no-image cases to improve user experience.
31-49: Consider adding component tests.
This new component is visually critical. A simple snapshot or integration test can help maintain confidence in future refactors.Would you like help creating Jest/React Testing Library tests for
MovingLogos?backend/apps/owasp/models/sponsors.py (2)
11-19: Optional: Define a default ordering.
If you often query sponsors bysort_nameorname, consider adding a default ordering inMetato simplify queries and preserve consistency in the admin panel and across the API.
94-135: Ensure robust handling of unexpected “sponsor” or “membertype” values.
The mapping sets a fallback to “Not a Sponsor” or “Silver,” but if future data entries use unmapped keys, logging a warning or error may help identify data discrepancies promptly.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
backend/apps/owasp/admin.py(2 hunks)backend/apps/owasp/constants.py(1 hunks)backend/apps/owasp/graphql/nodes/sponsors.py(1 hunks)backend/apps/owasp/graphql/queries/__init__.py(1 hunks)backend/apps/owasp/graphql/queries/sponsors.py(1 hunks)backend/apps/owasp/migrations/0022_sponsor.py(1 hunks)backend/apps/owasp/models/sponsors.py(1 hunks)frontend/src/api/queries/homeQueries.ts(1 hunks)frontend/src/components/LoadingSpinner.tsx(1 hunks)frontend/src/components/MovingLogo.tsx(1 hunks)frontend/src/components/MultiSearch.tsx(1 hunks)frontend/src/pages/Home.tsx(2 hunks)frontend/src/types/home.ts(1 hunks)frontend/tailwind.config.js(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- frontend/src/components/LoadingSpinner.tsx
- frontend/src/components/MultiSearch.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/apps/owasp/admin.py
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Run frontend unit tests
- GitHub Check: CodeQL (python)
- GitHub Check: Run backend tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (12)
backend/apps/owasp/constants.py (1)
4-4: URL design looks good.The new constant is well-named and appropriately references the raw GitHub URL for retrieving OWASP sponsor information.
backend/apps/owasp/graphql/nodes/sponsors.py (1)
1-16: Node implementation is clean and follows project conventions.The new SponsorNode implementation follows the established BaseNode pattern and correctly exposes only the necessary fields (name and image_url) from the Sponsor model.
frontend/src/types/home.ts (2)
26-26: Type definition is correctly integrated.The sponsors array property has been properly added to the MainPageData type, following the established pattern in the file.
35-38: SponsorType definition is well-structured.The new SponsorType export is clean, uses appropriate camelCase naming conventions for TypeScript, and correctly defines the necessary properties.
frontend/src/api/queries/homeQueries.ts (1)
54-57: GraphQL query field is properly structured.The sponsors field is correctly added to the GET_MAIN_PAGE_DATA query with the appropriate subfields that match the SponsorType definition.
backend/apps/owasp/graphql/queries/__init__.py (2)
8-8: Import looks good!The import of SponsorQuery is properly integrated with the existing query structure.
12-14: LGTM - Successfully extended the OwaspQuery class.The SponsorQuery has been correctly added to the inheritance chain of the OwaspQuery class, maintaining the proper class inheritance structure.
backend/apps/owasp/graphql/queries/sponsors.py (1)
3-7: Imports are correctly structured.All necessary imports are properly included, following project's import structure.
frontend/src/pages/Home.tsx (2)
24-24: Import statement looks good.The MovingLogos component is properly imported.
219-224: Verify the position of the sponsors section in the page layout.The sponsors section is currently positioned between the "Recent Releases" and the statistics counters. Consider if this is the optimal placement for visibility and user experience.
Consider whether the sponsors section should be:
- Placed higher on the page for better visibility
- Kept in its current position between content sections
- Moved to the footer area, which is a common location for sponsor acknowledgments
The decision should balance the importance of sponsor recognition with the overall user experience and flow of the page.
backend/apps/owasp/migrations/0022_sponsor.py (1)
80-83: Schema looks good!
The migration correctly sets up theSponsortable with sensible defaults and constraints. No issues spotted.backend/apps/owasp/models/sponsors.py (1)
60-63: String representation looks good.
This straightforward__str__method will be helpful for debugging and the admin interface.
frontend/src/pages/Home.tsx
Outdated
| <SecondaryCard> | ||
| <MovingLogos sponsors={data.sponsors} /> | ||
| </SecondaryCard> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add a title to the sponsors section and handle empty state.
Unlike other sections on the page (e.g., "New Chapters", "New Projects"), the sponsors section lacks a descriptive title. Also, there's no handling for cases where there are no sponsors to display.
Consider these changes:
-<SecondaryCard>
+<SecondaryCard title="Our Sponsors">
- <MovingLogos sponsors={data.sponsors} />
+ {data.sponsors && data.sponsors.length > 0 ? (
+ <MovingLogos sponsors={data.sponsors} />
+ ) : (
+ <p className="text-center text-muted-foreground">No sponsors to display</p>
+ )}
</SecondaryCard>This will provide consistency with other sections and handle the case when there are no sponsors available.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <SecondaryCard> | |
| <MovingLogos sponsors={data.sponsors} /> | |
| </SecondaryCard> | |
| <SecondaryCard title="Our Sponsors"> | |
| {data.sponsors && data.sponsors.length > 0 ? ( | |
| <MovingLogos sponsors={data.sponsors} /> | |
| ) : ( | |
| <p className="text-center text-muted-foreground">No sponsors to display</p> | |
| )} | |
| </SecondaryCard> |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
backend/tests/owasp/models/sponsors_test.py (3)
7-7: Remove excess empty line.There's an unnecessary empty line here that can be removed to maintain consistent spacing throughout the file.
- class TestSponsorModel:
57-70: Consider adding more test cases for sponsor type mapping.The test covers mapping for "1", "2", and "-1", but there are more sponsor types mentioned in the readable_sponsor_type test ("Gold", "Silver", "Supporter"). Consider adding tests for these mappings as well for complete coverage.
@pytest.mark.parametrize( ("sponsor_type_value", "expected_sponsor_type"), [ ("1", "Diamond"), ("2", "Platinum"), + ("3", "Gold"), + ("4", "Silver"), + ("5", "Supporter"), ("-1", "Not a Sponsor"), ], )
1-84: Consider adding tests for edge cases and error handling.The current tests provide good coverage of the happy path scenarios, but consider adding tests for potential edge cases:
- Invalid sponsor/member type values
- Empty dictionaries passed to from_dict
- Handling of null/None values
- Any validation or constraints on the Sponsor model
These would enhance the robustness of your test suite.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/tests/owasp/models/sponsors_test.py(1 hunks)
🔇 Additional comments (5)
backend/tests/owasp/models/sponsors_test.py (5)
8-20: Good test implementation for string representation!The parameterized test approach ensures thorough coverage of different name values, including the empty string case. The docstring clearly describes the test purpose.
37-49: LGTM - Comprehensive test for member type property.The test correctly verifies all member type mappings (Platinum, Gold, Silver) with clear parameterization.
50-56: Well-implemented mock test for bulk_save method.The test correctly mocks the BulkSaveModel.bulk_save method and verifies it's called with the right parameters. Good use of patch to isolate the test from actual implementation.
71-84: Good test coverage for member type mapping.The test effectively verifies all member type mappings with clear comments explaining what each mapping represents.
21-36:✅ Verification successful
Verify if the "Not a Sponsor" mapping is intentional.
The test data maps "Not a Sponsor" to "Not Sponsor" (without "a"), which seems inconsistent. Is this an intentional conversion or should the expected label match the input type exactly?
🏁 Script executed:
#!/bin/bash # Search for the Sponsor model definition to verify the correct mapping cat backend/apps/owasp/models/sponsors.py | grep -A 20 "readable_sponsor_type"Length of output: 730
Mapping Verified as Intentional
After reviewing the implementation inbackend/apps/owasp/models/sponsors.py, it’s clear that thereadable_sponsor_typeproperty returns a label from theSponsorTypeenum. This enum intentionally maps"Not a Sponsor"to"Not Sponsor", which aligns with the test expectations. No changes are required.
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
backend/apps/owasp/models/sponsors.py (1)
95-135: 🛠️ Refactor suggestionImprove data validation and error handling in from_dict.
The
from_dictmethod doesn't validate incoming data or handle potential exceptions. Additionally, hardcoded mapping values make maintenance challenging if the data source changes.
- Add validation for required fields
- Extract the mapping dictionaries as class constants
- Normalize all URLs consistently
+ # Class constants for mappings + MEMBER_TYPE_MAPPING = { + "2": MemberType.PLATINUM, + "3": MemberType.GOLD, + "4": MemberType.SILVER, + } + + SPONSOR_TYPE_MAPPING = { + "1": SponsorType.DIAMOND, + "2": SponsorType.PLATINUM, + "3": SponsorType.GOLD, + "4": SponsorType.SILVER, + "5": SponsorType.SUPPORTER, + "-1": SponsorType.NOT_SPONSOR, + } def from_dict(self, data): """Update instance based on the dict data.""" + # Validate required data + if not data.get("name"): + raise ValueError("Sponsor name is required") + image_path = data.get("image", "").lstrip("/") image_url = f"{OWASP_ORGANIZATION_DATA_URL}/{image_path}" if image_path else "" sponsor_key = str(data.get("sponsor", "-1")) member_key = str(data.get("membertype", "4")) - member_type_mapping = { - "2": self.MemberType.PLATINUM, - "3": self.MemberType.GOLD, - "4": self.MemberType.SILVER, - } - - sponsor_type_mapping = { - "1": self.SponsorType.DIAMOND, - "2": self.SponsorType.PLATINUM, - "3": self.SponsorType.GOLD, - "4": self.SponsorType.SILVER, - "5": self.SponsorType.SUPPORTER, - "-1": self.SponsorType.NOT_SPONSOR, - } - - sponsor_type_label = sponsor_type_mapping.get(sponsor_key, self.SponsorType.NOT_SPONSOR) - member_type_label = member_type_mapping.get(member_key, self.MemberType.SILVER) + sponsor_type_label = self.SPONSOR_TYPE_MAPPING.get(sponsor_key, self.SponsorType.NOT_SPONSOR) + member_type_label = self.MEMBER_TYPE_MAPPING.get(member_key, self.MemberType.SILVER) fields = { "name": data.get("name", ""), "sort_name": data.get("sortname", "").capitalize(), "description": data.get("description", ""), "url": normalize_url(data.get("url", "")) or "", "job_url": normalize_url(data.get("job_url", "")) or "", - "image_url": image_url, + "image_url": image_url or "", "is_member": bool(data.get("member", False)), "sponsor_type": sponsor_type_label, "member_type": member_type_label if bool(data.get("member", False)) else "", } for key, value in fields.items(): setattr(self, key, value)
🧹 Nitpick comments (5)
backend/apps/owasp/models/sponsors.py (5)
28-32: Consider adding 'NOT_MEMBER' option in MemberType.While there's an
is_memberfield to track membership status, it would be more consistent to have a "Not a Member" option in the MemberType enum, similar to how SponsorType has a "Not a Sponsor" option. This would provide better data integrity whenis_memberis False.class MemberType(models.TextChoices): PLATINUM = "Platinum" GOLD = "Gold" SILVER = "Silver" + NOT_MEMBER = "Not a Member"
35-37: Consider adding index to key field for faster lookups.Since the
keyfield is used for lookups (as seen in theupdate_datamethod) and has a unique constraint, adding a database index would improve query performance.- key = models.CharField(verbose_name="Key", max_length=100, unique=True) + key = models.CharField(verbose_name="Key", max_length=100, unique=True, db_index=True)
80-93: Consider using a transaction for update_data method.The
update_datamethod should ideally be wrapped in a transaction to ensure data consistency, especially if exceptions occur during the process.Wrap the operation in a transaction:
@staticmethod def update_data(data, save=True): """Update sponsor data.""" + from django.db import transaction + + with transaction.atomic(): key = slugify(data["name"]) try: sponsor = Sponsor.objects.get(key=key) except Sponsor.DoesNotExist: sponsor = Sponsor(key=key) sponsor.from_dict(data) if save: sponsor.save() return sponsor
100-120: Document the meaning of numeric keys in sponsor and member type mappings.The mappings from numeric keys to sponsor and member types lack documentation. It's not clear what these numbers represent or where they come from, which makes maintenance difficult.
Add comments to explain the source and meaning of these mappings:
sponsor_key = str(data.get("sponsor", "-1")) member_key = str(data.get("membertype", "4")) +# Mapping from OWASP API membertype values to our MemberType enum member_type_mapping = { "2": self.MemberType.PLATINUM, # OWASP API value 2 = Platinum "3": self.MemberType.GOLD, # OWASP API value 3 = Gold "4": self.MemberType.SILVER, # OWASP API value 4 = Silver } +# Mapping from OWASP API sponsor values to our SponsorType enum sponsor_type_mapping = { "1": self.SponsorType.DIAMOND, # OWASP API value 1 = Diamond "2": self.SponsorType.PLATINUM, # OWASP API value 2 = Platinum "3": self.SponsorType.GOLD, # OWASP API value 3 = Gold "4": self.SponsorType.SILVER, # OWASP API value 4 = Silver "5": self.SponsorType.SUPPORTER, # OWASP API value 5 = Supporter "-1": self.SponsorType.NOT_SPONSOR, # OWASP API value -1 = Not a Sponsor }
125-127: Add validation for URL fields.The URL fields are normalized but not validated for format correctness. Consider adding validation to ensure they are valid URLs.
+ from django.core.validators import URLValidator + from django.core.exceptions import ValidationError def from_dict(self, data): """Update instance based on the dict data.""" + url_validator = URLValidator() + + # Helper function to validate URLs + def validate_url(url): + try: + if url: + url_validator(url) + return url + except ValidationError: + return "" # ... existing code ... fields = { "name": data.get("name", ""), "sort_name": data.get("sortname", "").capitalize(), "description": data.get("description", ""), - "url": normalize_url(data.get("url", "")) or "", - "job_url": normalize_url(data.get("job_url", "")) or "", + "url": validate_url(normalize_url(data.get("url", "")) or ""), + "job_url": validate_url(normalize_url(data.get("job_url", "")) or ""), "image_url": image_url, # ... rest of the fields ... }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/apps/owasp/models/sponsors.py(1 hunks)
🔇 Additional comments (3)
backend/apps/owasp/models/sponsors.py (3)
11-13: Model looks good and follows Django conventions!The Sponsor model is well-structured and inherits from appropriate base classes
BulkSaveModelandTimestampedModel.
16-18: Database configuration looks good.The Meta class correctly sets the database table name and readable verbose name for the admin interface.
20-26: Well-defined choices using TextChoices enum.Using Django's TextChoices for sponsor types provides type safety and self-documentation. Good practice!
| is_member = models.BooleanField(verbose_name="Is Corporate Sponsor", default=False) | ||
| member_type = models.CharField( | ||
| verbose_name="Member Type", | ||
| max_length=20, | ||
| choices=MemberType.choices, | ||
| default=MemberType.SILVER, | ||
| blank=True, | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider aligning default behavior of is_member and member_type.
Currently, is_member defaults to False, but member_type defaults to MemberType.SILVER. These defaults seem inconsistent - if not a member, the member type shouldn't be set to SILVER.
is_member = models.BooleanField(verbose_name="Is Corporate Sponsor", default=False)
member_type = models.CharField(
verbose_name="Member Type",
max_length=20,
choices=MemberType.choices,
- default=MemberType.SILVER,
+ default="",
blank=True,
)Or better yet, make the member_type field conditional on is_member:
def save(self, *args, **kwargs):
"""Override save method to ensure data consistency."""
if not self.is_member:
self.member_type = ""
super().save(*args, **kwargs)| image_path = data.get("image", "").lstrip("/") | ||
| image_url = f"{OWASP_ORGANIZATION_DATA_URL}/{image_path}" | ||
|
|
There was a problem hiding this comment.
Image URL construction should handle empty paths.
The current image URL construction doesn't handle the case where image_path is empty, which could result in an invalid URL.
- image_path = data.get("image", "").lstrip("/")
- image_url = f"{OWASP_ORGANIZATION_DATA_URL}/{image_path}"
+ image_path = data.get("image", "").lstrip("/")
+ image_url = f"{OWASP_ORGANIZATION_DATA_URL}/{image_path}" if image_path else ""📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| image_path = data.get("image", "").lstrip("/") | |
| image_url = f"{OWASP_ORGANIZATION_DATA_URL}/{image_path}" | |
| image_path = data.get("image", "").lstrip("/") | |
| image_url = f"{OWASP_ORGANIZATION_DATA_URL}/{image_path}" if image_path else "" |
| @property | ||
| def readable_member_type(self): | ||
| """Get human-readable member type.""" | ||
| return self.MemberType(self.member_type).label | ||
|
|
||
| @property | ||
| def readable_sponsor_type(self): | ||
| """Get human-readable sponsor type.""" | ||
| return self.SponsorType(self.sponsor_type).label | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Properties for human-readable types might throw exceptions.
The readable_member_type and readable_sponsor_type properties don't handle the case where the field might be blank or have an invalid value. This could lead to exceptions at runtime.
Add error handling to prevent exceptions:
@property
def readable_member_type(self):
"""Get human-readable member type."""
- return self.MemberType(self.member_type).label
+ try:
+ return self.MemberType(self.member_type).label if self.member_type else ""
+ except ValueError:
+ return ""
@property
def readable_sponsor_type(self):
"""Get human-readable sponsor type."""
- return self.SponsorType(self.sponsor_type).label
+ try:
+ return self.SponsorType(self.sponsor_type).label
+ except ValueError:
+ return ""📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @property | |
| def readable_member_type(self): | |
| """Get human-readable member type.""" | |
| return self.MemberType(self.member_type).label | |
| @property | |
| def readable_sponsor_type(self): | |
| """Get human-readable sponsor type.""" | |
| return self.SponsorType(self.sponsor_type).label | |
| @property | |
| def readable_member_type(self): | |
| """Get human-readable member type.""" | |
| try: | |
| return self.MemberType(self.member_type).label if self.member_type else "" | |
| except ValueError: | |
| return "" | |
| @property | |
| def readable_sponsor_type(self): | |
| """Get human-readable sponsor type.""" | |
| try: | |
| return self.SponsorType(self.sponsor_type).label | |
| except ValueError: | |
| return "" |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
backend/apps/owasp/models/sponsor.py (1)
44-57:⚠️ Potential issueAdd validation for member type consistency.
There's a logical inconsistency in the model:
is_membercan beFalsewhilemember_typehas a default value ofSILVER. This could lead to confusion when a non-member has a member type.Consider adding validation to enforce consistency between
is_memberandmember_typefields:def __str__(self): """Sponsor human readable representation.""" return f"{self.name}" +def clean(self): + """Validate model fields.""" + super().clean() + errors = {} + + # Ensure member_type consistency with is_member status + if not self.is_member and self.member_type != self.MemberType.SILVER: + self.member_type = "" + elif self.is_member and not self.member_type: + errors["member_type"] = "A valid member type must be selected for members." + + if errors: + raise ValidationError(errors)Additionally, consider updating the fields to default
member_typeto blank whenis_memberisFalse:member_type = models.CharField( verbose_name="Member Type", max_length=20, choices=MemberType.choices, - default=MemberType.SILVER, + default="", blank=True, )
🧹 Nitpick comments (6)
backend/apps/owasp/models/sponsor.py (3)
96-98: Add null checking for image path.The image URL is being constructed unconditionally, even if no image path is provided. This could result in an invalid URL being stored.
def from_dict(self, data): """Update instance based on the dict data.""" image_path = data.get("image", "").lstrip("/") - image_url = f"https://raw.githubusercontent.com/OWASP/owasp.github.io/main/{image_path}" + image_url = "" + if image_path: + image_url = f"https://raw.githubusercontent.com/OWASP/owasp.github.io/main/{image_path}"
78-93: Consider handling race conditions in update_data method.In concurrent environments, the
update_datamethod could lead to race conditions when multiple processes try to create the same sponsor simultaneously.Consider using Django's
get_or_createor a transaction withselect_for_update:@staticmethod def update_data(data, save=True): """Update sponsor data.""" key = slugify(data["name"]) + + from django.db import transaction + + with transaction.atomic(): + sponsor, created = Sponsor.objects.get_or_create(key=key) + sponsor.from_dict(data) + + if save: + sponsor.save() + + return sponsor - try: - sponsor = Sponsor.objects.get(key=key) - except Sponsor.DoesNotExist: - sponsor = Sponsor(key=key) - - sponsor.from_dict(data) - - if save: - sponsor.save() - - return sponsor
113-116: Add a safety check for unexpected member and sponsor keys.The current implementation assumes that the provided member and sponsor keys will always be in the mapping dictionaries, but falls back to defaults if not. Adding some logging or validation for unexpected values could help with debugging.
member_key = data.get("membertype", 4) sponsor_key = data.get("sponsor", -1) + +# Log unexpected values to help identify data issues +if member_key not in member_type_mapping: + import logging + logger = logging.getLogger(__name__) + logger.warning(f"Unexpected member key: {member_key} for sponsor {data['name']}") + +if sponsor_key not in sponsor_type_mapping: + import logging + logger = logging.getLogger(__name__) + logger.warning(f"Unexpected sponsor key: {sponsor_key} for sponsor {data['name']}") + member_type_label = member_type_mapping.get(member_key, self.MemberType.SILVER) sponsor_type_label = sponsor_type_mapping.get(sponsor_key, self.SponsorType.NOT_SPONSOR)backend/apps/owasp/graphql/nodes/sponsor.py (1)
7-15: Consider exposing more useful sponsor fields.The current implementation only exposes
image_urlandnamefields. Consider adding more useful fields that might be needed by frontend components or API consumers.class SponsorNode(BaseNode): """Sponsor node.""" class Meta: model = Sponsor fields = ( "image_url", "name", + "description", + "url", + "job_url", + "sponsor_type", + "is_member", + "member_type", )backend/apps/owasp/graphql/queries/sponsor.py (1)
15-17: Add result ordering for consistent results.The sponsors list is currently not ordered, which can lead to inconsistent results between API calls. Adding a default ordering would improve user experience.
def resolve_sponsors(root, info): """Resolve sponsors.""" - return Sponsor.objects.all() + return Sponsor.objects.all().order_by('sort_name', 'name')frontend/src/components/LogoCarousel.tsx (1)
28-28: Add responsive animation duration based on screen size.The current animation duration is only based on the number of sponsors, but should also consider the screen size for a consistent experience across devices.
+ const [animationDuration, setAnimationDuration] = useState(sponsors.length * 2); + useEffect(() => { + // Adjust animation duration based on screen width + const handleResize = () => { + const baseSpeed = sponsors.length * 2; + // Slow down on smaller screens + if (window.innerWidth < 768) { + setAnimationDuration(baseSpeed * 0.7); + } else { + setAnimationDuration(baseSpeed); + } + }; + + handleResize(); // Initial call + window.addEventListener('resize', handleResize); + return () => window.removeEventListener('resize', handleResize); + }, [sponsors.length]); <div ref={scrollerRef} className="flex w-full animate-scroll gap-6" - style={{ animationDuration: `${sponsors.length * 2}s` }} + style={{ animationDuration: `${animationDuration}s` }} >
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
backend/apps/owasp/admin.py(2 hunks)backend/apps/owasp/graphql/nodes/sponsor.py(1 hunks)backend/apps/owasp/graphql/queries/__init__.py(1 hunks)backend/apps/owasp/graphql/queries/sponsor.py(1 hunks)backend/apps/owasp/management/commands/owasp_update_sponsors.py(1 hunks)backend/apps/owasp/models/sponsor.py(1 hunks)backend/apps/slack/utils.py(2 hunks)backend/tests/owasp/models/sponsors_test.py(1 hunks)frontend/__tests__/unit/data/mockHomeData.ts(1 hunks)frontend/src/components/LogoCarousel.tsx(1 hunks)frontend/src/pages/Home.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- frontend/tests/unit/data/mockHomeData.ts
- backend/apps/slack/utils.py
- frontend/src/pages/Home.tsx
- backend/apps/owasp/graphql/queries/init.py
- backend/apps/owasp/management/commands/owasp_update_sponsors.py
- backend/tests/owasp/models/sponsors_test.py
- backend/apps/owasp/admin.py
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Run frontend unit tests
- GitHub Check: CodeQL (python)
- GitHub Check: Run frontend e2e tests
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: Run backend tests
🔇 Additional comments (1)
backend/apps/owasp/models/sponsor.py (1)
63-71: Add error handling to readable properties.The current implementation of
readable_member_typeandreadable_sponsor_typewill raise an exception if an invalid type is stored in the database. This can happen if choices change over time or if data is imported incorrectly.@property def readable_member_type(self): """Get human-readable member type.""" - return self.MemberType(self.member_type).label + try: + return self.MemberType(self.member_type).label + except ValueError: + return "Unknown" @property def readable_sponsor_type(self): """Get human-readable sponsor type.""" - return self.SponsorType(self.sponsor_type).label + try: + return self.SponsorType(self.sponsor_type).label + except ValueError: + return "Unknown"
| class SponsorQuery(BaseQuery): | ||
| """Sponsor queries.""" | ||
|
|
||
| sponsors = graphene.List(SponsorNode) | ||
|
|
||
| def resolve_sponsors(root, info): | ||
| """Resolve sponsors.""" | ||
| return Sponsor.objects.all() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement pagination and filtering for better performance.
The current implementation fetches all sponsors without pagination or filtering. This could lead to performance issues if there are many sponsors.
Consider implementing pagination and filtering:
class SponsorQuery(BaseQuery):
"""Sponsor queries."""
- sponsors = graphene.List(SponsorNode)
+ sponsors = graphene.List(
+ SponsorNode,
+ sponsor_type=graphene.String(required=False),
+ is_member=graphene.Boolean(required=False),
+ limit=graphene.Int(required=False, default_value=10),
+ offset=graphene.Int(required=False, default_value=0),
+ )
- def resolve_sponsors(root, info):
+ def resolve_sponsors(root, info, sponsor_type=None, is_member=None, limit=10, offset=0):
"""Resolve sponsors."""
- return Sponsor.objects.all()
+ query = Sponsor.objects.all()
+
+ if sponsor_type:
+ query = query.filter(sponsor_type=sponsor_type)
+
+ if is_member is not None:
+ query = query.filter(is_member=is_member)
+
+ return query[offset:offset+limit]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| class SponsorQuery(BaseQuery): | |
| """Sponsor queries.""" | |
| sponsors = graphene.List(SponsorNode) | |
| def resolve_sponsors(root, info): | |
| """Resolve sponsors.""" | |
| return Sponsor.objects.all() | |
| class SponsorQuery(BaseQuery): | |
| """Sponsor queries.""" | |
| sponsors = graphene.List( | |
| SponsorNode, | |
| sponsor_type=graphene.String(required=False), | |
| is_member=graphene.Boolean(required=False), | |
| limit=graphene.Int(required=False, default_value=10), | |
| offset=graphene.Int(required=False, default_value=0), | |
| ) | |
| def resolve_sponsors(root, info, sponsor_type=None, is_member=None, limit=10, offset=0): | |
| """Resolve sponsors.""" | |
| query = Sponsor.objects.all() | |
| if sponsor_type: | |
| query = query.filter(sponsor_type=sponsor_type) | |
| if is_member is not None: | |
| query = query.filter(is_member=is_member) | |
| return query[offset:offset+limit] |
| <div className="relative overflow-hidden py-2"> | ||
| <div | ||
| ref={scrollerRef} | ||
| className="flex w-full animate-scroll gap-6" | ||
| style={{ animationDuration: `${sponsors.length * 2}s` }} | ||
| > | ||
| {sponsors.map((sponsor, index) => ( | ||
| <div | ||
| key={`${sponsor.name}-${index}`} | ||
| className="flex min-w-[220px] flex-shrink-0 flex-col items-center rounded-lg p-5 shadow-sm hover:shadow-lg" | ||
| > | ||
| <div className="relative mb-4 flex h-16 w-full items-center justify-center"> | ||
| {sponsor.imageUrl ? ( | ||
| <img | ||
| src={sponsor.imageUrl} | ||
| alt={`${sponsor.name} logo`} | ||
| className="h-full w-full object-cover" | ||
| loading="lazy" | ||
| /> | ||
| ) : ( | ||
| <div className="flex h-full w-full items-center justify-center"></div> | ||
| )} | ||
| </div> | ||
| </div> | ||
| ))} | ||
| </div> | ||
| </div> | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance accessibility and add error handling for images.
The current implementation lacks essential accessibility attributes and error handling for images.
return (
- <div className="relative overflow-hidden py-2">
+ <div
+ className="relative overflow-hidden py-2"
+ aria-label="Sponsors logo carousel"
+ role="region"
+ >
<div
ref={scrollerRef}
className="flex w-full animate-scroll gap-6"
style={{ animationDuration: `${sponsors.length * 2}s` }}
>
{sponsors.map((sponsor, index) => (
<div
key={`${sponsor.name}-${index}`}
className="flex min-w-[220px] flex-shrink-0 flex-col items-center rounded-lg p-5 shadow-sm hover:shadow-lg"
+ aria-label={`${sponsor.name} sponsor`}
>
<div className="relative mb-4 flex h-16 w-full items-center justify-center">
{sponsor.imageUrl ? (
<img
src={sponsor.imageUrl}
alt={`${sponsor.name} logo`}
className="h-full w-full object-cover"
loading="lazy"
+ onError={(e) => {
+ // Fallback to text if image fails to load
+ const target = e.target as HTMLImageElement;
+ target.style.display = 'none';
+ target.parentElement?.insertAdjacentHTML('beforeend',
+ `<div class="text-center font-medium">${sponsor.name}</div>`);
+ }}
/>
) : (
- <div className="flex h-full w-full items-center justify-center"></div>
+ <div className="flex h-full w-full items-center justify-center text-center font-medium">
+ {sponsor.name}
+ </div>
)}
</div>
</div>
))}
</div>
</div>
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <div className="relative overflow-hidden py-2"> | |
| <div | |
| ref={scrollerRef} | |
| className="flex w-full animate-scroll gap-6" | |
| style={{ animationDuration: `${sponsors.length * 2}s` }} | |
| > | |
| {sponsors.map((sponsor, index) => ( | |
| <div | |
| key={`${sponsor.name}-${index}`} | |
| className="flex min-w-[220px] flex-shrink-0 flex-col items-center rounded-lg p-5 shadow-sm hover:shadow-lg" | |
| > | |
| <div className="relative mb-4 flex h-16 w-full items-center justify-center"> | |
| {sponsor.imageUrl ? ( | |
| <img | |
| src={sponsor.imageUrl} | |
| alt={`${sponsor.name} logo`} | |
| className="h-full w-full object-cover" | |
| loading="lazy" | |
| /> | |
| ) : ( | |
| <div className="flex h-full w-full items-center justify-center"></div> | |
| )} | |
| </div> | |
| </div> | |
| ))} | |
| </div> | |
| </div> | |
| ) | |
| return ( | |
| <div | |
| className="relative overflow-hidden py-2" | |
| aria-label="Sponsors logo carousel" | |
| role="region" | |
| > | |
| <div | |
| ref={scrollerRef} | |
| className="flex w-full animate-scroll gap-6" | |
| style={{ animationDuration: `${sponsors.length * 2}s` }} | |
| > | |
| {sponsors.map((sponsor, index) => ( | |
| <div | |
| key={`${sponsor.name}-${index}`} | |
| className="flex min-w-[220px] flex-shrink-0 flex-col items-center rounded-lg p-5 shadow-sm hover:shadow-lg" | |
| aria-label={`${sponsor.name} sponsor`} | |
| > | |
| <div className="relative mb-4 flex h-16 w-full items-center justify-center"> | |
| {sponsor.imageUrl ? ( | |
| <img | |
| src={sponsor.imageUrl} | |
| alt={`${sponsor.name} logo`} | |
| className="h-full w-full object-cover" | |
| loading="lazy" | |
| onError={(e) => { | |
| // Fallback to text if image fails to load | |
| const target = e.target as HTMLImageElement; | |
| target.style.display = 'none'; | |
| target.parentElement?.insertAdjacentHTML('beforeend', | |
| `<div class="text-center font-medium">${sponsor.name}</div>`); | |
| }} | |
| /> | |
| ) : ( | |
| <div className="flex h-full w-full items-center justify-center text-center font-medium"> | |
| {sponsor.name} | |
| </div> | |
| )} | |
| </div> | |
| </div> | |
| ))} | |
| </div> | |
| </div> | |
| ) |
| useEffect(() => { | ||
| if (scrollerRef.current) { | ||
| const scrollContainer = scrollerRef.current | ||
|
|
||
| scrollContainer.innerHTML += scrollContainer.innerHTML | ||
| } | ||
| }, [sponsors]) |
There was a problem hiding this comment.
Avoid direct DOM manipulation with innerHTML in React.
Using innerHTML can lead to React reconciliation issues and potential security vulnerabilities (XSS). Consider a more React-friendly approach to create a continuous scrolling effect.
useEffect(() => {
- if (scrollerRef.current) {
- const scrollContainer = scrollerRef.current
-
- scrollContainer.innerHTML += scrollContainer.innerHTML
- }
+ // No need to manipulate DOM directly
}, [sponsors])
// Then modify the render method to duplicate content
return (
<div className="relative overflow-hidden py-2">
<div
ref={scrollerRef}
className="flex w-full animate-scroll gap-6"
style={{ animationDuration: `${sponsors.length * 2}s` }}
>
{sponsors.map((sponsor, index) => (
<div
key={`${sponsor.name}-${index}`}
className="flex min-w-[220px] flex-shrink-0 flex-col items-center rounded-lg p-5 shadow-sm hover:shadow-lg"
>
{/* Sponsor content */}
</div>
))}
+ {/* Duplicate the sponsors for continuous scrolling */}
+ {sponsors.map((sponsor, index) => (
+ <div
+ key={`${sponsor.name}-duplicate-${index}`}
+ className="flex min-w-[220px] flex-shrink-0 flex-col items-center rounded-lg p-5 shadow-sm hover:shadow-lg"
+ >
+ {/* Same sponsor content */}
+ </div>
+ ))}
</div>
</div>
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| useEffect(() => { | |
| if (scrollerRef.current) { | |
| const scrollContainer = scrollerRef.current | |
| scrollContainer.innerHTML += scrollContainer.innerHTML | |
| } | |
| }, [sponsors]) | |
| useEffect(() => { | |
| // No need to manipulate DOM directly | |
| }, [sponsors]) | |
| return ( | |
| <div className="relative overflow-hidden py-2"> | |
| <div | |
| ref={scrollerRef} | |
| className="flex w-full animate-scroll gap-6" | |
| style={{ animationDuration: `${sponsors.length * 2}s` }} | |
| > | |
| {sponsors.map((sponsor, index) => ( | |
| <div | |
| key={`${sponsor.name}-${index}`} | |
| className="flex min-w-[220px] flex-shrink-0 flex-col items-center rounded-lg p-5 shadow-sm hover:shadow-lg" | |
| > | |
| {/* Sponsor content */} | |
| </div> | |
| ))} | |
| {/* Duplicate the sponsors for continuous scrolling */} | |
| {sponsors.map((sponsor, index) => ( | |
| <div | |
| key={`${sponsor.name}-duplicate-${index}`} | |
| className="flex min-w-[220px] flex-shrink-0 flex-col items-center rounded-lg p-5 shadow-sm hover:shadow-lg" | |
| > | |
| {/* Same sponsor content */} | |
| </div> | |
| ))} | |
| </div> | |
| </div> | |
| ) |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
frontend/src/components/LogoCarousel.tsx (2)
11-17:⚠️ Potential issueAvoid direct DOM manipulation with innerHTML in React.
Using
innerHTMLcan lead to React reconciliation issues and potential security vulnerabilities (XSS). Consider a more React-friendly approach to create a continuous scrolling effect.useEffect(() => { - if (scrollerRef.current) { - const scrollContainer = scrollerRef.current - - scrollContainer.innerHTML += scrollContainer.innerHTML - } + // No need to manipulate DOM directly }, [sponsors]) // Then modify the render method to duplicate content return ( <div className="relative overflow-hidden py-2"> <div ref={scrollerRef} className="flex w-full animate-scroll gap-6" style={{ animationDuration: `${sponsors.length * 2}s` }} > {sponsors.map((sponsor, index) => ( // Sponsor content ))} + {/* Duplicate the sponsors for continuous scrolling */} + {sponsors.map((sponsor, index) => ( + <div + key={`${sponsor.name}-duplicate-${index}`} + // Same styling as original items + > + {/* Same sponsor content */} + </div> + ))} </div> </div> )
19-54: 🛠️ Refactor suggestionEnhance accessibility and add error handling for images.
The current implementation lacks essential accessibility attributes and error handling for images. Also, when no image URL is provided, you should display the sponsor name as fallback.
return ( - <div className="relative overflow-hidden py-2"> + <div + className="relative overflow-hidden py-2" + aria-label="Sponsors logo carousel" + role="region" + > <div ref={scrollerRef} className="flex w-full animate-scroll gap-6" style={{ animationDuration: `${sponsors.length * 2}s` }} > {sponsors.map((sponsor, index) => ( <div key={`${sponsor.name}-${index}`} className="flex min-w-[220px] flex-shrink-0 flex-col items-center rounded-lg p-5" + aria-label={`${sponsor.name} sponsor`} > <a href={sponsor.url} target="_blank" rel="noopener noreferrer" className="flex h-full w-full flex-col items-center justify-center" + aria-label={`Visit ${sponsor.name} website`} + {...(!sponsor.url && { tabIndex: -1, onClick: (e) => e.preventDefault() })} > <div className="relative mb-4 flex h-16 w-full items-center justify-center"> {sponsor.imageUrl ? ( <img src={sponsor.imageUrl} alt={`${sponsor.name} logo`} className="h-full w-full object-cover" loading="lazy" + onError={(e) => { + // Fallback to text if image fails to load + const target = e.target as HTMLImageElement; + target.style.display = 'none'; + const parent = target.parentElement; + if (parent) { + const fallbackDiv = document.createElement('div'); + fallbackDiv.className = 'text-center font-medium'; + fallbackDiv.textContent = sponsor.name; + parent.appendChild(fallbackDiv); + } + }} /> ) : ( - <div className="flex h-full w-full items-center justify-center"></div> + <div className="flex h-full w-full items-center justify-center text-center font-medium"> + {sponsor.name} + </div> )} </div> </a> </div> ))} </div> </div> )
🧹 Nitpick comments (5)
backend/apps/owasp/graphql/nodes/sponsor.py (2)
1-16: Good implementation of the GraphQL node for sponsors.The
SponsorNodecorrectly inherits fromBaseNodeand defines the necessary fields for sponsor representation in the GraphQL API.Consider adding more detailed docstrings to explain what each field represents, making it clearer for other developers. For example:
class SponsorNode(BaseNode): - """Sponsor node.""" + """ + GraphQL node representing an OWASP sponsor. + + Exposes sponsor details like name, URL, and image for display on the website + and in Slack responses. + """
6-6: Remove extra empty line.For consistency with the project's coding style, you should remove this extra empty line.
from apps.owasp.models.sponsor import Sponsor - class SponsorNode(BaseNode):frontend/src/components/LogoCarousel.tsx (3)
31-36: Add conditional URL handling to prevent invalid links.The current implementation doesn't check if
sponsor.urlexists before using it as anhref. This could result in invalid links or unexpected behavior.<a - href={sponsor.url} + href={sponsor.url || '#'} target="_blank" rel="noopener noreferrer" className="flex h-full w-full flex-col items-center justify-center" + {...(!sponsor.url && { + onClick: (e) => e.preventDefault(), + style: { cursor: 'default' } + })} >
1-7: Consider using a more descriptive component name.The component name
MovingLogosis somewhat generic. Since this is specifically for sponsors, a more descriptive name likeSponsorCarouselorSponsorLogoSliderwould better communicate its purpose.-interface MovingLogosProps { +interface SponsorCarouselProps { sponsors: SponsorType[] } -export default function MovingLogos({ sponsors }: MovingLogosProps) { +export default function SponsorCarousel({ sponsors }: SponsorCarouselProps) {
24-24: Improve responsive animation duration calculation.The current animation duration calculation
${sponsors.length * 2}smay result in too slow or too fast animation with very few or many sponsors. Consider adding bounds or a more sophisticated formula.-style={{ animationDuration: `${sponsors.length * 2}s` }} +style={{ + animationDuration: `${Math.max(10, Math.min(sponsors.length * 2, 60))}s` +}}This sets a minimum duration of 10 seconds and a maximum of 60 seconds, which provides a better user experience across different sponsor counts.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/apps/owasp/graphql/nodes/sponsor.py(1 hunks)frontend/src/api/queries/homeQueries.ts(1 hunks)frontend/src/components/LogoCarousel.tsx(1 hunks)frontend/src/pages/Home.tsx(3 hunks)frontend/src/types/home.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/src/types/home.ts
- frontend/src/api/queries/homeQueries.ts
- frontend/src/pages/Home.tsx
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: CodeQL (python)
- GitHub Check: Run backend tests
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
🔇 Additional comments (1)
backend/apps/owasp/graphql/nodes/sponsor.py (1)
12-16:❓ Verification inconclusive
Verify fields completeness for sponsor requirements.
The fields currently exposed (image_url, name, url) seem sufficient for basic sponsor display needs. However, ensure this covers all requirements for both the Slack command and frontend displays mentioned in PR objectives.
🏁 Script executed:
#!/bin/bash # Check if the Sponsor model has additional fields that might be useful to expose # Description: This script checks for additional fields in the Sponsor model echo "Checking Sponsor model fields..." cat backend/apps/owasp/models/sponsor.py | grep -E "^ [a-z_]+ = " | grep -v "_id"Length of output: 862
Please Confirm Sponsor Field Exposure for Slack & Frontend
The current GraphQL node exposes only
image_url,name, andurl, yet the Sponsor model includes several additional fields (such asdescription,key,sort_name,job_url,is_member, andsponsor_type). Please verify against the PR objectives that this minimal subset meets the display requirements for both the Slack command and the frontend. If the extra details are needed for enhanced sponsor presentation, consider exposing the relevant fields accordingly.
- File:
backend/apps/owasp/graphql/nodes/sponsor.py(lines 12–16)- Action: Confirm whether additional model fields should be exposed based on the sponsor requirements.
| {sponsor.imageUrl ? ( | ||
| <img | ||
| src={sponsor.imageUrl} | ||
| alt={`${sponsor.name} logo`} | ||
| className="h-full w-full object-cover" | ||
| loading="lazy" | ||
| /> | ||
| ) : ( | ||
| <div className="flex h-full w-full items-center justify-center"></div> | ||
| )} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add React-friendly image error handling with state.
The onError approach using direct DOM manipulation is not React-friendly. Use state to manage fallback rendering instead.
+import { useEffect, useRef, useState } from 'react'
// Inside component:
+const [imageErrors, setImageErrors] = useState<Record<number, boolean>>({})
// Then in render:
<div className="relative mb-4 flex h-16 w-full items-center justify-center">
- {sponsor.imageUrl ? (
+ {sponsor.imageUrl && !imageErrors[index] ? (
<img
src={sponsor.imageUrl}
alt={`${sponsor.name} logo`}
className="h-full w-full object-cover"
loading="lazy"
+ onError={() => setImageErrors(prev => ({ ...prev, [index]: true }))}
/>
) : (
<div className="flex h-full w-full items-center justify-center text-center font-medium">
+ {sponsor.name}
</div>
)}
</div>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {sponsor.imageUrl ? ( | |
| <img | |
| src={sponsor.imageUrl} | |
| alt={`${sponsor.name} logo`} | |
| className="h-full w-full object-cover" | |
| loading="lazy" | |
| /> | |
| ) : ( | |
| <div className="flex h-full w-full items-center justify-center"></div> | |
| )} | |
| // At the top of your file, ensure you have the following import: | |
| import { useEffect, useRef, useState } from 'react'; | |
| // Inside your LogoCarousel component, initialize the state: | |
| const [imageErrors, setImageErrors] = useState<Record<number, boolean>>({}); | |
| // In your render method, update the sponsor render block as follows: | |
| {sponsors.map((sponsor, index) => ( | |
| <div key={`${sponsor.name}-${index}`} className="flex min-w-[220px] flex-shrink-0 flex-col items-center rounded-lg p-5 shadow-sm hover:shadow-lg"> | |
| <div className="relative mb-4 flex h-16 w-full items-center justify-center"> | |
| {sponsor.imageUrl && !imageErrors[index] ? ( | |
| <img | |
| src={sponsor.imageUrl} | |
| alt={`${sponsor.name} logo`} | |
| className="h-full w-full object-cover" | |
| loading="lazy" | |
| onError={() => setImageErrors(prev => ({ ...prev, [index]: true }))} | |
| /> | |
| ) : ( | |
| <div className="flex h-full w-full items-center justify-center text-center font-medium"> | |
| {sponsor.name} | |
| </div> | |
| )} | |
| </div> | |
| </div> | |
| ))} |
|
* added model, command , algolia , sponsors * Enhanced command and removed extra indexed from data * Added testcases * verified commit * verified commit with chnages * fixed importing * resolved conflict * fixed testcase * fixed makefile * pre-commit after resolve conflict * migrations * pre-commmit * fixed sonaQube warning * refractor code removed algolia * updated testcase * added moving Logo compoennt * backned test fix * frontend test case fix * pre-commit * fix e2e case * fix bug * improvements * Update code * changes * increase testcase timing --------- Co-authored-by: abhayymishraaa <[email protected]> Co-authored-by: Arkadii Yakovets <[email protected]>
2 participants
Resolves #616
Preview (Present) :
Screencast.from.28-01-25.06.12.41.PM.IST.webm
Tasks: