-
Notifications
You must be signed in to change notification settings - Fork 61
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(web): RSS feed for events and generic list items #17270
Conversation
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
warning eslint@8.57.0: This version is no longer supported. Please see https://eslint.org/version-support for other options. WalkthroughThe pull request introduces enhancements to the RSS feed generation system, expanding its capabilities to support multiple content types (news, generic lists, and events). The changes involve modifying the RSS generation endpoint to dynamically handle different content sources, adding a new Changes
Sequence DiagramsequenceDiagram
participant Client
participant RSSEndpoint
participant ContentService
participant GraphQLAPI
Client->>RSSEndpoint: Request RSS Feed
RSSEndpoint->>RSSEndpoint: Determine Content Type
RSSEndpoint->>GraphQLAPI: Fetch Content Items
GraphQLAPI-->>RSSEndpoint: Return Items
RSSEndpoint->>RSSEndpoint: Generate XML Items
RSSEndpoint-->>Client: Return RSS Feed XML
Possibly related PRs
Suggested labels
Suggested reviewers
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17270 +/- ##
==========================================
- Coverage 35.71% 35.71% -0.01%
==========================================
Files 6944 6944
Lines 148502 148513 +11
Branches 42383 42397 +14
==========================================
Hits 53043 53043
- Misses 95459 95470 +11
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 2 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ✅ 35 Total Test Services: 0 Failed, 33 Passed Test ServicesThis report shows up to 10 services
🔻 Code Coverage Decreases vs Default Branch (1)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Nitpick comments (3)
apps/web/pages/api/rss/index.ts (2)
26-31
: Simplify TypeScript Optional Properties in 'Item' InterfaceThe properties
date
anddescription
in theItem
interface can be simplified by using optional properties. Instead of explicitly includingundefined
andnull
in the type, you can define them as optional properties withstring | null
.Apply this diff to simplify the interface:
interface Item { - date: string | null | undefined + date?: string | null fullUrl: string title: string - description: string | null | undefined + description?: string | null }
181-193
: Handle Possible Undefined Values in Event TimeIn the event content type handling, the code assumes that
item.time.startTime
anditem.time.endTime
are present. If these values are undefined, it could lead to 'undefined' strings in the RSS feed.Add null checks or default values to ensure that the time strings are correctly formatted.
const formattedStartDate = format( new Date(item.startDate), 'dd. MMMM yyyy', { locale: localeMap[locale], }, ) return generateItemString({ title: item.title, description: `${formattedStartDate} ${ - item.time.startTime as string + item.time.startTime || '' } ${item.time.endTime ? '-' : ''} ${ - item.time.endTime as string + item.time.endTime || '' }`, fullUrl: organizationlibs/cms/src/lib/models/event.model.ts (1)
81-82
: Add Documentation for 'firstPublishedAt' FieldFor consistency and clarity, consider adding a comment or JSDoc annotation to the new
firstPublishedAt
field explaining its purpose.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/web/pages/api/rss/index.ts
(3 hunks)apps/web/screens/queries/Events.ts
(1 hunks)libs/cms/src/lib/models/event.model.ts
(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
apps/web/screens/queries/Events.ts (1)
Pattern apps/**/*
: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
libs/cms/src/lib/models/event.model.ts (1)
Pattern libs/**/*
: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
apps/web/pages/api/rss/index.ts (1)
Pattern apps/**/*
: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
🔇 Additional comments (2)
apps/web/pages/api/rss/index.ts (1)
195-202
:
Provide Default URL When Organization is Undefined
In the event content type, when organization
is not provided, the fullUrl
defaults to an empty string. This could result in RSS items with missing links.
Ensure that a valid URL is provided even when organization
is undefined.
Check if item.slug
can be used to construct the URL when organization
is not specified. You might adjust the fullUrl
assignment:
fullUrl: organization
? `${baseUrl}${
linkResolver(
'organizationevent',
[organization, item.slug],
locale,
).href
}`
- : '',
+ : `${baseUrl}${linkResolver('event', [item.slug], locale).href}`,
apps/web/screens/queries/Events.ts (1)
62-62
: Ensure 'firstPublishedAt' Field Exists and Is Accessible
The addition of the firstPublishedAt
field to the GET_EVENTS_QUERY
requires that this field is available and correctly resolved in the GraphQL schema.
Run the following script to confirm that firstPublishedAt
is available in the Event
type:
✅ Verification successful
firstPublishedAt
field is available and properly defined
The field firstPublishedAt
is correctly defined in the GraphQL schema as part of the Sys
type, which is accessible through the sys
field that every entry implements via the Entry
interface. The field is being properly used in the GET_EVENTS_QUERY
query.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if 'firstPublishedAt' field exists in the GraphQL schema for Event.
# Fetch the schema and search for 'firstPublishedAt' in the Event type
curl -s 'https://your-graphql-endpoint/graphql' \
-H 'Content-Type: application/json' \
-d '{"query":"{ __type(name: \"Event\") { fields { name } } }"}' \
| jq '.data.__type.fields[] | select(.name == "firstPublishedAt")'
Length of output: 215
Script:
#!/bin/bash
# Search for GraphQL schema definitions or type definitions containing 'firstPublishedAt'
rg -l "firstPublishedAt" --type graphql
# Search for GraphQL query definitions containing 'firstPublishedAt'
rg "type\s+Event\b" -A 10 -B 10 --type graphql
# Look for TypeScript interfaces or types related to Event
ast-grep --pattern 'interface Event {
$$$
}'
# Search for any usage of firstPublishedAt in the codebase
rg "firstPublishedAt" -A 2 -B 2
Length of output: 72512
Script:
#!/bin/bash
# Search for GraphQL schema type definitions for Event
rg "type\s+Event\s*{" -A 20 libs/clients/cms/schema.graphql
# Search for GraphQL queries using firstPublishedAt field
rg "query.*\{" -A 10 -B 2 apps/web/screens/queries/Events.ts
Length of output: 734
Script:
#!/bin/bash
# Search for Event type definition with fixed regex
rg "type Event \{" -A 20 libs/clients/cms/schema.graphql
# Search for sys field definitions that might contain firstPublishedAt
rg "type Sys \{" -A 10 libs/clients/cms/schema.graphql
# Look for any interface definitions that might include firstPublishedAt
rg "interface \w+ \{" -A 10 libs/clients/cms/schema.graphql
Length of output: 1057
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
🧹 Nitpick comments (1)
apps/web/pages/api/rss/index.ts (1)
103-121
: Refactor to eliminate code duplication in item processingThe logic for fetching data and generating
itemString
for 'news', 'genericList', and 'event' content types is similar. Consider refactoring the code to reduce duplication by creating a common function or utilizing a more generic approach.For example, you could create a function that handles data fetching and item string generation based on the content type.
Also applies to: 143-162, 179-208
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/web/pages/api/rss/index.ts
(3 hunks)apps/web/screens/queries/Events.ts
(1 hunks)libs/cms/src/lib/models/event.model.ts
(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
apps/web/screens/queries/Events.ts (1)
Pattern apps/**/*
: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/web/pages/api/rss/index.ts (1)
Pattern apps/**/*
: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
libs/cms/src/lib/models/event.model.ts (1)
Pattern libs/**/*
: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
🔇 Additional comments (1)
apps/web/screens/queries/Events.ts (1)
62-62
: Verify that firstPublishedAt
is available in the GraphQL schema
Ensure that the firstPublishedAt
field exists in the GraphQL schema for the Event
type and that it is being correctly resolved. Missing or misconfigured fields can lead to runtime errors.
Run the following script to check if firstPublishedAt
is available in the Event
type:
If the output is empty, the field is not present and needs to be added to the schema.
✅ Verification successful
firstPublishedAt
field is correctly defined and used in the codebase
The field is properly defined in the CMS schema and is actively used across the codebase:
- Defined in
libs/cms/src/lib/models/event.model.ts
as part of the Event model - Present in the Contentful CMS schema at
libs/clients/cms/schema.graphql
- Actively used in multiple places including the Events query and RSS feed generation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the `firstPublishedAt` field in the GraphQL schema for `Event`
# Fetch the schema and search for `firstPublishedAt` in `Event` type
gh api graphql -f query='
{
__type(name: "Event") {
fields {
name
}
}
}' | jq '.data.__type.fields[] | select(.name=="firstPublishedAt")'
Length of output: 215
Script:
#!/bin/bash
# Let's check the schema definition files in the repository
fd "schema.*" --type f --exec rg -l "firstPublishedAt"
# Also search for GraphQL type definitions containing Event
fd ".*\.(graphql|gql)" --type f --exec rg -A 5 -B 5 "type Event"
# Search for any GraphQL queries using firstPublishedAt
rg "firstPublishedAt" --type ts
Length of output: 4903
* WIP * Add support for generic list items and events --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
* feat(web): RSS feed for events and generic list items (#17270) * WIP * Add support for generic list items and events --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> * fix(web): RSS feed - Lowercase t in "contentType" query param (#17282) Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> * fix(web): RSS feed - Event dates should show single digit for date if it can (#17297) Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
RSS feed for events and generic list items
What
Checklist:
Summary by CodeRabbit
New Features
firstPublishedAt
to theGET_EVENTS_QUERY
, improving event data returned.Bug Fixes
Documentation
Event
class to include the newfirstPublishedAt
field.