enhancement(billing): ensure typescript & esm syntax #2687
Conversation
p6l-richard
requested review from
perkinsjr,
chronark,
mcstepp and
MichaelUnkey
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@p6l-richard is attempting to deploy a commit to the Unkey Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThe changes in this pull request include updates to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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
CodeRabbit Configuration File (
|
|
Hey there and thank you for opening this pull request! 👋🏼 We require pull request titles to follow the Conventional Commits specification and it looks like your proposed title needs to be adjusted. Details: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
apps/billing/src/trigger/glossary/draft-sections.ts (2)
154-156: Consider improving prompt formatting for better readability.The indentation of the new guidelines differs from other items in the list. Consider maintaining consistent indentation for better maintainability.
9. Code Snippets - - When you include code snippets in JavaScript, make sure to always use TypeScript syntax. - - Ensure that you always use ESM syntax and not CommonJS syntax (ie use import/export and not 'require'). + - When you include code snippets in JavaScript, make sure to always use TypeScript syntax. + - Ensure that you always use ESM syntax and not CommonJS syntax (ie use import/export and not 'require').
Line range hint
1-156: Consider adding type safety for OpenAI model names.The OpenAI model names are currently passed as string literals. Consider adding type safety to prevent potential typos or invalid model names.
// Add an enum or const object for model names const OPENAI_MODELS = { GPT4_TURBO: "gpt-4-turbo", GPT4_MINI: "gpt-4o-mini", } as const; type OpenAIModel = typeof OPENAI_MODELS[keyof typeof OPENAI_MODELS]; // Update the model parameter usage model: openai(OPENAI_MODELS.GPT4_TURBO)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
apps/billing/package.json(1 hunks)apps/billing/src/trigger/glossary/create-pr.ts(5 hunks)apps/billing/src/trigger/glossary/draft-sections.ts(1 hunks)
🔇 Additional comments (3)
apps/billing/package.json (2)
28-28: LGTM! TypeScript type definitions added for js-yaml.
Adding @types/js-yaml aligns with the PR objective of ensuring TypeScript compatibility. The version (^4.0.9) correctly matches the major version of the runtime js-yaml dependency (^4.1.0).
25-27: LGTM! Verify the changelog for @trigger.dev packages.
The patch version updates (3.2.0 → 3.2.1) for @trigger.dev packages look good. All packages are kept in sync at the same version.
✅ Verification successful
Patch update (3.2.0 → 3.2.1) includes React hooks improvements and Zod upgrade
The changelog shows that version 3.2.1 includes:
- React hooks now support
accessTokenandbaseURLoptions without requiring Provider - Upgrade of Zod dependency to 3.23.8
- Addition of realtime streams feature
- Updates to core dependencies
These changes are safe improvements that maintain backward compatibility.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the changelog for @trigger.dev packages to verify the patch updates
# Fetch the latest releases for trigger.dev packages
gh api graphql -f query='
{
repository(owner: "triggerdotdev", name: "trigger.dev") {
releases(first: 2, orderBy: {field: CREATED_AT, direction: DESC}) {
nodes {
tagName
description
}
}
}
}'
Length of output: 1115
apps/billing/src/trigger/glossary/draft-sections.ts (1)
154-156: LGTM! Guidelines for TypeScript and ESM syntax.
The added guidelines ensure consistency in code examples by mandating TypeScript and ESM syntax, which aligns well with modern JavaScript practices.
| sortKeys: (a, b) => { | ||
| // Ensure that 'question' always comes first | ||
| if (a === "question" || b === "question") { | ||
| return a === "question" ? -1 : 1; | ||
| } | ||
| return 0; | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure 'sortKeys' function is effective
The sortKeys function aims to position the 'question' key first in the YAML output. However, the provided data does not have a top-level 'question' key, so this sorting logic may not have the intended effect. Please verify the data structure and adjust the sorting function accordingly.
| const existingFile = await octokit.repos.getContent({ | ||
| owner, | ||
| repo, | ||
| ref: existingPr.data[0].head.ref, | ||
| path, | ||
| }); |
There was a problem hiding this comment.
Handle missing files when retrieving content
When using octokit.repos.getContent, a 404 error is thrown if the file does not exist. This can cause unhandled exceptions. Please implement error handling to manage cases where the file may not exist.
Apply this diff to add error handling:
-let existingFile = await octokit.repos.getContent({
+let existingFile;
+try {
+ existingFile = await octokit.repos.getContent({
owner,
repo,
ref: existingPr.data[0].head.ref,
path,
-});
+ });
+} catch (error) {
+ if (error.status === 404) {
+ existingFile = null;
+ } else {
+ throw error;
+ }
+}Adjust the subsequent code to handle existingFile being null:
...("sha" in existingFile.data && { sha: existingFile.data.sha }),
+...(existingFile && "sha" in existingFile.data && { sha: existingFile.data.sha }),Committable suggestion skipped: line range outside the PR's diff.
| const owner = "unkeyed"; | ||
| const repo = "unkey"; | ||
| const branch = `richard/add-${input.replace(/\s+/g, "-").toLowerCase()}`; | ||
| const path = `apps/www/content/glossary/${input.replace(/\s+/g, "-").toLowerCase()}.mdx`; |
There was a problem hiding this comment.
Use consistent slug generation to prevent potential issues
To ensure consistency and handle special characters safely, consider using the existing slugger instance for creating the branch and path variables instead of manually replacing spaces and changing to lowercase.
Apply this diff to use slugger.slug(input):
const owner = "unkeyed";
const repo = "unkey";
-const branch = `richard/add-${input.replace(/\s+/g, "-").toLowerCase()}`;
-const path = `apps/www/content/glossary/${input.replace(/\s+/g, "-").toLowerCase()}.mdx`;
+const slug = slugger.slug(input);
+const branch = `richard/add-${slug}`;
+const path = `apps/www/content/glossary/${slug}.mdx`;Committable suggestion skipped: line range outside the PR's diff.
| const existingFile = await octokit.repos.getContent({ | ||
| owner, | ||
| repo, | ||
| path, | ||
| }); |
There was a problem hiding this comment.
Add error handling for getContent when the file may not exist
Similar to the previous instance, when committing to the new branch, the file might not exist, leading to an error. Please wrap the getContent call in a try-catch block to handle this scenario.
Apply this diff to handle the potential error:
-let existingFile = await octokit.repos.getContent({
+let existingFile;
+try {
+ existingFile = await octokit.repos.getContent({
owner,
repo,
path,
-});
+ });
+} catch (error) {
+ if (error.status === 404) {
+ existingFile = null;
+ } else {
+ throw error;
+ }
+}And adjust the subsequent code accordingly:
...("sha" in existingFile.data && { sha: existingFile.data.sha }),
+...(existingFile && "sha" in existingFile.data && { sha: existingFile.data.sha }),📝 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.
| const existingFile = await octokit.repos.getContent({ | |
| owner, | |
| repo, | |
| path, | |
| }); | |
| let existingFile; | |
| try { | |
| existingFile = await octokit.repos.getContent({ | |
| owner, | |
| repo, | |
| path, | |
| }); | |
| } catch (error) { | |
| if (error.status === 404) { | |
| existingFile = null; | |
| } else { | |
| throw error; | |
| } | |
| } |
What does this PR do?
draftSectionsLLM prompts to include a guideline for code snippetsFixes # (issue)
Type of change
How should this be tested?
draft_sectionsrun for for transport layer security or single sign-on in production on trigger to see if it still contains requires.Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
New Features
Bug Fixes
Chores