Fixed issue #1035 Correct redirection and Links have valid URLs.#1071
Fixed issue #1035 Correct redirection and Links have valid URLs.#1071arkid15r merged 6 commits intoOWASP:mainfrom
Conversation
Summary by CodeRabbit
WalkthroughThe pull request updates both frontend GraphQL queries and backend GraphQL schema/model for releases. On the frontend, the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
🪧 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
|
![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: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
frontend/src/api/queries/homeQueries.ts(1 hunks)frontend/src/api/queries/projectQueries.ts(1 hunks)frontend/src/components/ItemCardList.tsx(1 hunks)frontend/src/components/RecentCardList.tsx(1 hunks)frontend/src/pages/Home.tsx(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: CodeQL (python)
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: Run backend tests
- GitHub Check: Run frontend unit tests
- GitHub Check: Run frontend e2e tests
🔇 Additional comments (8)
frontend/src/components/ItemCardList.tsx (1)
43-43: Approve UI enhancement with line breakAdding the line break after the item title improves readability by separating the title from subsequent content, creating a cleaner visual hierarchy.
frontend/src/pages/Home.tsx (1)
28-28: Approve migration to specialized component for Recent ReleasesThe introduction of
RecentCardListcomponent and its implementation in the Home page properly addresses the issue with recent release links. This component now specifically formats recent releases data with correctly structured GitHub URLs.Also applies to: 229-244
frontend/src/components/RecentCardList.tsx (3)
1-58: Fix for broken release links implemented correctlyThe new
RecentCardListcomponent properly constructs GitHub release URLs using repository owner, repository name, and tag name. This addresses issue #1035 by ensuring valid URLs for recent releases.
42-43: Consistent with ItemCardList rendering styleThe line break after the item title maintains consistency with the ItemCardList component, which was modified with the same change.
39-39:Details
❓ Verification inconclusive
Verify URL structure reliability
The URL construction relies on deeply nested optional chaining:
href={`https://github.com/${item?.author?.releases?.[0]?.repository?.ownerKey}/${item?.author?.releases?.[0]?.repository?.key}/releases/tag/${item.tagName}`}While the optional chaining prevents errors, ensure this data structure is consistently available.
🏁 Script executed:
#!/bin/bash # Check for the data structure expected by RecentCardList echo "Checking Home query structure to verify releases data format..." rg -A 10 "GET_MAIN_PAGE_DATA" --type tsLength of output: 2388
Attention: Verify Nested Data Structure for GitHub Release URL
The URL construction
href={`https://github.com/${item?.author?.releases?.[0]?.repository?.ownerKey}/${item?.author?.releases?.[0]?.repository?.key}/releases/tag/${item.tagName}`}assumes that nested data (i.e. an object at
item.author.releases[0].repository) is always available. However, the output from theGET_MAIN_PAGE_DATAquery does not include any fields related to releases, which suggests that this data might be coming from a different source or might not be as reliable as expected.
- Review Data Source: Confirm whether the releases-related data is provided by another query or if it should be added to the existing
GET_MAIN_PAGE_DATAresponse.- Fall-back Handling: Consider adding fallback logic or validations in case any of these nested properties are absent, to avoid constructing broken URLs.
- Consistency Check: Verify that
item.tagNameis always defined as expected.Please double-check the source of the
itemobject to ensure that the nested structure used here can be reliably expected in all cases.frontend/src/api/queries/homeQueries.ts (3)
47-47: Good enhancement to include author login field.Adding the
loginfield to the author object is a necessary improvement for generating proper links to the author's profile, which directly addresses the issue of broken links mentioned in PR #1035.
48-53: Essential repository data structure for proper URL formation.Adding the nested
releases.repositorystructure withkeyandownerKeyfields provides the necessary information to construct proper URLs for the repositories. This change is crucial for fixing the broken links in the recent releases main page card mentioned in issue #1035.
58-58:Details
✅ Verification successful
Verify the correct usage of publishedAt field.
The addition of the
publishedAtfield will help display accurate publication timestamps for releases. Since the AI summary mentioned this field was previously removed but now appears added, please ensure this is consistent with the expected behavior and that related components are updated to reference this field correctly.
🏁 Script executed:
#!/bin/bash # Check if related components reference publishedAt field echo "Checking for components using publishedAt from recentReleases:" rg -A 2 -B 2 "publishedAt" --glob "frontend/src/components/*.{tsx,jsx,ts,js}" echo "Checking for usage in Home page:" rg -A 2 -B 2 "publishedAt" --glob "frontend/src/pages/Home.{tsx,jsx,ts,js}"Length of output: 2627
Ensure Consistent Integration of the
publishedAtFieldThe usage of the
publishedAtfield has been verified across related components:
- ReleaseCard.tsx: The field is correctly used with a timestamp conversion (
new Date(release.publishedAt * 1000).toLocaleDateString()).- Home.tsx: The
publishedAtvalue is properly formatted using theformatDatefunction.- ItemCardList.tsx, RecentCardList.tsx, and CardDetailsPage.tsx: References to
publishedAtare consistently implemented to display accurate publication timestamps.Please confirm that the conversion logic and formatting meet the intended behavior as discussed in the AI summary. No discrepancies were found in the current implementation.
| login | ||
| name | ||
| } | ||
| tagname |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Potential duplicate field in GraphQL query
The tagname field appears to be redundant since tagName (with camelCase) is already present at line 29. GraphQL field names are case-sensitive, so this might be unintentional.
🏁 Script executed:
#!/bin/bash
# Check if the GraphQL schema has both tagName and tagname fields
echo "Checking if GraphQL schema contains both tagName and tagname fields..."
grep -r "type.*Release" --include="*.graphql" --include="*.ts" .
# Check if the backend returns both fields
echo "Checking if both fields are being used in the codebase..."
rg "tagName|tagname" --type tsLength of output: 3035
Action: Remove duplicate 'tagname' field from the GraphQL query
- In frontend/src/api/queries/projectQueries.ts, the GraphQL query includes both tagName and tagname.
- Our search confirms that everywhere else in the codebase (and in the type definitions) only tagName is used, and there’s no evidence that the lowercase tagname is intended.
- Please remove the duplicate lowercase tagname entry to keep the query consistent and avoid unintended behavior due to case sensitivity.
|
Hey @arkid15r i have made all the links corrected in recent versions. Please review the changes. Thank you. |
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/apps/github/graphql/nodes/release.py (1)
20-26: Consider updating the fields tuple in MetaWhile the code works as is, the
urlfield is defined at the class level but not included in the Meta fields tuple. For consistency and to make the code more maintainable, consider adding it there as well.fields = ( "author", "is_pre_release", "name", "published_at", "tag_name", + "url", )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/apps/github/graphql/nodes/release.py(2 hunks)backend/apps/github/models/release.py(1 hunks)backend/tests/github/graphql/nodes/release_test.py(1 hunks)frontend/src/api/queries/homeQueries.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/api/queries/homeQueries.ts
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Run backend tests
- GitHub Check: CodeQL (python)
- GitHub Check: Run frontend e2e tests
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: Run frontend unit tests
🔇 Additional comments (4)
backend/tests/github/graphql/nodes/release_test.py (1)
28-28: Correctly updated the expected fields list to include the newurlfieldThis update to the test case ensures that the new
urlfield added to theReleaseNodeis properly tested, maintaining test coverage for the schema changes.backend/apps/github/models/release.py (1)
53-57: Well implementedurlproperty for the Release modelThe implementation correctly constructs the release URL by combining the repository URL with the release tag name, following GitHub's URL structure for releases.
backend/apps/github/graphql/nodes/release.py (2)
16-16: Correctly added theurlfield to the ReleaseNode schemaThis change properly exposes the new URL field through the GraphQL API, addressing the issue with broken links.
32-34: Appropriate resolver implementation for theurlfieldThe resolver correctly returns the value from the model's
urlproperty. This implementation maintains a clean separation between the data model and GraphQL layer.
arkid15r
left a comment
arkid15r
left a comment
There was a problem hiding this comment.
I updated the code please see the current version that I'm going to merge.
…OWASP#1071) * correct changes * changes to the links * corrected linking and reviews correction * linking resolved * Update code --------- Co-authored-by: Arkadii Yakovets <arkadii.yakovets@owasp.org>
…OWASP#1071) * correct changes * changes to the links * corrected linking and reviews correction * linking resolved * Update code --------- Co-authored-by: Arkadii Yakovets <arkadii.yakovets@owasp.org>
2 participants
Fixes issue: #1035 Recent releases main page card links are broken.
Here is the fixed output:
Screen.Recording.2025-03-11.at.1.17.13.AM.mov