Skip to content
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

fix(web): RSS feed - Lowercase t in "contentType" query param #17282

Merged
merged 3 commits into from
Dec 19, 2024

Conversation

RunarVestmann
Copy link
Member

@RunarVestmann RunarVestmann commented Dec 18, 2024

RSS feed - Lowercase t in "contentType" query param

What

  • Since other query params are all lowercase this is to make them more consistent
  • Use lowercase letters in "contentType" query param (this is a new query param that is not available on prod so changing it won't affect anything)

Checklist:

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • Formatting passes locally with my changes
  • I have rebased against main before asking for a review

Summary by CodeRabbit

  • Bug Fixes
    • Adjusted the case sensitivity of the contentType query parameter for API requests, which may affect how clients specify content types.

@RunarVestmann RunarVestmann added the automerge Merge this PR as soon as all checks pass label Dec 18, 2024
@RunarVestmann RunarVestmann requested review from a team as code owners December 18, 2024 13:59
Copy link
Contributor

coderabbitai bot commented Dec 18, 2024

Walkthrough

The pull request introduces a minor modification to the RSS API endpoint in the apps/web/pages/api/rss/index.ts file. The change specifically alters the case sensitivity of the contentType query parameter from contentType to contenttype in the parseAsStringEnum method call. This adjustment impacts how the API processes incoming requests related to content type, potentially requiring clients to adjust their request formatting.

Changes

File Change Summary
apps/web/pages/api/rss/index.ts Modified case sensitivity of contentType query parameter from contentType to contenttype in parseAsStringEnum method

Possibly related PRs

Suggested reviewers

  • mannipje

Note: No sequence diagram is generated for this change as it represents a minor parameter case sensitivity adjustment with no significant flow modification.

Tip

CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command @coderabbitai generate docstrings to have CodeRabbit automatically generate docstrings for your pull request. We would love to hear your feedback on Discord.


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@datadog-island-is
Copy link

datadog-island-is bot commented Dec 18, 2024

Datadog Report

Branch report: fix/web-rss-feed-content-type-filter
Commit report: 5bcc3c2
Test service: web

✅ 0 Failed, 84 Passed, 0 Skipped, 25.11s Total Time
➡️ Test Sessions change in coverage: 1 no change

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
apps/web/pages/api/rss/index.ts (2)

Line range hint 1-200: Update API documentation to reflect the parameter change.

Since this introduces a new query parameter that's not in production, ensure that the API documentation is updated to reflect the correct parameter name contenttype.

Would you like me to help create documentation for the RSS API endpoint that includes:

  • Available query parameters and their types
  • Supported content types
  • Example requests and responses

Line range hint 18-18: Consider using an API request validator middleware.

The current approach of validating query parameters individually could be improved by implementing a request validation middleware using a schema validation library (e.g., Zod, Joi).

This would:

  • Centralize parameter validation
  • Ensure consistent casing rules
  • Provide better type inference
  • Generate API documentation automatically

Also applies to: 80-80

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e558168 and a194b65.

📒 Files selected for processing (1)
  • apps/web/pages/api/rss/index.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
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 (1)
apps/web/pages/api/rss/index.ts (1)

80-80: ⚠️ Potential issue

Verify backward compatibility for the query parameter change.

The change from contentType to contenttype might break existing client integrations. Consider implementing a temporary fallback to support both cases during a transition period.

-    .parseServerSide(req.query?.contenttype) as 'news' | 'genericList' | 'event'
+    .parseServerSide(
+      (req.query?.contenttype ?? req.query?.contentType)
+    ) as 'news' | 'genericList' | 'event'

Run this script to check for existing usages:

✅ Verification successful

Let me search for any client-side usage of this RSS API endpoint to better understand the impact.


Let me try one more search to find any references to this RSS endpoint with different patterns.


The query parameter change appears to be safe

Based on the search results, there are no direct client-side usages of the RSS API endpoint with the contentType parameter found in the codebase. The only references to content types are in the CMS services (news.service.ts, event.service.ts, cmsSync.service.ts) which are internal implementations and not related to the API query parameter. Since there's no evidence of existing integrations using the uppercase version, the change from contentType to contenttype is unlikely to cause compatibility issues.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for existing usages of the contentType parameter
rg -i "contentType.*=.*['\"](?:news|genericList|event)['\"]" --type ts

Length of output: 584


Script:

#!/bin/bash
# Search for RSS API endpoint usage
rg -i "api/rss.*contenttype" --type ts -A 2

# Also search for any documentation or test files mentioning the RSS endpoint
fd -e md -e test.ts -X rg -i "api/rss"

Length of output: 84


Script:

#!/bin/bash
# Search for RSS API endpoint usage with broader patterns
rg -i "/api/rss\?" --type ts -A 2

# Look for RSS-related tests
rg -i "describe.*rss" --type ts

# Check if there's a specific RSS configuration file
fd -e ts -e js -e json -X rg -i "rss.*config"

Length of output: 117

Copy link

codecov bot commented Dec 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 35.71%. Comparing base (1b5773d) to head (56d9e3b).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #17282      +/-   ##
==========================================
+ Coverage   35.68%   35.71%   +0.02%     
==========================================
  Files        6922     6944      +22     
  Lines      148524   148488      -36     
  Branches    42419    42398      -21     
==========================================
+ Hits        53008    53034      +26     
+ Misses      95516    95454      -62     
Flag Coverage Δ
web 2.40% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
apps/web/pages/api/rss/index.ts 0.00% <ø> (ø)

... and 66 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1b5773d...56d9e3b. Read the comment docs.

@RunarVestmann RunarVestmann changed the title fix(web): RSS feed - Lowercase t in "contentType" fix(web): RSS feed - Lowercase t in "contentType" query param Dec 18, 2024
@kodiakhq kodiakhq bot merged commit 075a58e into main Dec 19, 2024
34 checks passed
@kodiakhq kodiakhq bot deleted the fix/web-rss-feed-content-type-filter branch December 19, 2024 12:56
RunarVestmann added a commit that referenced this pull request Jan 6, 2025
Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
kodiakhq bot added a commit that referenced this pull request Jan 6, 2025
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Merge this PR as soon as all checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants