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.

Sign up for GitHub

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

enhancement(billing): ensure typescript & esm syntax #2687

Merged
merged 4 commits into from
Nov 27, 2024
Merged

enhancement(billing): ensure typescript & esm syntax #2687

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b6c45c5
ensure ESM syntax (import/export) instead of CJS (require)
p6l-richard Nov 26, 2024
ab37835
update the create-pr flow to re-use existing PRs
p6l-richard Nov 26, 2024
f419de4
update trigger
p6l-richard Nov 26, 2024
4515ac0
`pnpm fmt`
p6l-richard Nov 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions apps/billing/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,10 @@
"@mendable/firecrawl-js": "^1.5.2",
"@octokit/rest": "^21.0.2",
"@planetscale/database": "^1.16.0",
"@trigger.dev/nextjs": "3.2.0",
"@trigger.dev/sdk": "3.2.0",
"@trigger.dev/slack": "3.2.0",
"@trigger.dev/nextjs": "3.2.1",
"@trigger.dev/sdk": "3.2.1",
"@trigger.dev/slack": "3.2.1",
"@types/js-yaml": "^4.0.9",
"@unkey/billing": "workspace:^",
"@unkey/clickhouse": "workspace:^",
"@unkey/db": "workspace:^",
Expand Down
125 changes: 96 additions & 29 deletions apps/billing/src/trigger/glossary/create-pr.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ export const createPrTask = task({
if (existing?.githubPrUrl && onCacheHit === "stale") {
return {
entry: existing,
message: `Found existing PR for ${input}.mdx`,
};
}

Expand Down Expand Up @@ -76,6 +75,13 @@ export const createPrTask = task({
slug: slugger.slug(entry.inputTerm),
},
{
sortKeys: (a, b) => {
// Ensure that 'question' always comes first
if (a === "question" || b === "question") {
return a === "question" ? -1 : 1;
}
return 0;
},
Comment on lines +78 to +84
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ 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.

lineWidth: -1,
noRefs: true,
quotingType: '"',
Expand All @@ -101,44 +107,97 @@ export const createPrTask = task({
auth: process.env.GITHUB_PERSONAL_ACCESS_TOKEN,
});

const owner = "p6l-richard";
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`;
Comment on lines +110 to 113
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.


// Create a new branch
const mainRef = await octokit.git.getRef({
const existingPr = await octokit.rest.pulls.list({
owner,
repo,
ref: "heads/main",
base: "main",
head: `${owner}:${branch}`,
state: "open",
});

console.info(`2.1 'main' branch found. Should branch off of: ${mainRef.data.object.sha}`);
if (existingPr?.data?.length > 0) {
console.info("2.1 ⏩︎ Pending (open) PR found. Updating the content of the file directly...");

console.info(`this is for debugging, check if the file actually exists or not in the github ui: ${existingPr.data[0].head.ref}
URL: https://github.com/unkeyed/unkey/pull/${existingPr.data[0].number}`);
// get the blob sha of the file being replaced:
const existingFile = await octokit.repos.getContent({
owner,
repo,
ref: existingPr.data[0].head.ref,
path,
});
Comment on lines +129 to +134
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

// if an open PR exists, update the content of the file directly
await octokit.repos.createOrUpdateFileContents({
owner,
repo,
path,
message: `feat(glossary): Update ${input}.mdx`,
content: Buffer.from(await file.arrayBuffer()).toString("base64"),
branch,
committer: {
name: "Richard Poelderl",
email: "richard.poelderl@gmail.com",
},
...("sha" in existingFile.data && { sha: existingFile.data.sha }),
});

console.info("2.2 💽 PR updated. Storing the URL...");
// update the entry in the database with the PR URL
await db
.update(entries)
.set({ githubPrUrl: existingPr.data[0].html_url })
.where(eq(entries.inputTerm, input));

const updated = await db.query.entries.findFirst({
columns: {
id: true,
inputTerm: true,
githubPrUrl: true,
},
where: eq(entries.inputTerm, input),
orderBy: (entries, { desc }) => [desc(entries.createdAt)],
});

console.info("2.3 🎉 PR updated. Returning the entry...");

return {
entry: updated,
};
}

// if there's no open PR, we have to handle the merged PR case:
const existingMergedPr = await octokit.rest.pulls.list({
owner,
repo,
base: "main",
head: `${owner}:${branch}`,
state: "closed",
});

console.info("2.2 Handling possible duplicate branches");
const branchExists = await octokit.git
.listMatchingRefs({
if (existingMergedPr?.data?.length > 0) {
console.info("2.1 ⚠️ Merged PR found. Deleting the stale branch...");
// if a merged PR exists, we can delete the branch to create a new one & commit the file
await octokit.git.deleteRef({
owner,
repo,
ref: `heads/${branch}`,
})
.then((response) => response.data.length > 0);

if (branchExists) {
console.info("2.2.1 ⚠️ Duplicate branch found, deleting it");
try {
await octokit.git.deleteRef({
owner,
repo,
ref: `heads/${branch}`,
});
console.info("2.2.2 ⌫ Branch deleted");
} catch (error) {
console.error(`2.2.3 ❌ Error deleting branch: ${error}`);
}
});
}

console.info("2.4 🛣️ Creating the new branch");
console.info("2.2 🛣️ Creating the new branch");
// create a new branch off of main
const mainRef = await octokit.git.getRef({
owner,
repo,
ref: "heads/main",
});
// create a new branch off of main
await octokit.git.createRef({
owner,
repo,
Expand All @@ -147,17 +206,24 @@ export const createPrTask = task({
});

// Commit the MDX file to the new branch
console.info(`2.5 📦 Committing the MDX file to the new branch "${branch}"`);
console.info(`2.3 📦 Committing the MDX file to the new branch "${branch}"`);
// get the existing file's sha (if exists):
const existingFile = await octokit.repos.getContent({
owner,
repo,
path,
});
Comment on lines +211 to +215
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

Suggested change
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;
}
}

await octokit.repos.createOrUpdateFileContents({
owner,
repo,
path,
message: `feat(glossary): Add ${input}.mdx to glossary`,
content: Buffer.from(await file.arrayBuffer()).toString("base64"),
branch,
...("sha" in existingFile.data && { sha: existingFile.data.sha }),
});

console.info("2.6 📝 Creating the pull request");
console.info("2.4 📝 Creating the pull request");
// Create a pull request
const pr = await octokit.pulls.create({
owner,
Expand All @@ -168,7 +234,7 @@ export const createPrTask = task({
body: `This PR adds the ${input}.mdx file to the API documentation.`,
});

console.info("2.7 💽 PR created. Storing the URL...");
console.info("2.5 💽 PR created. Storing the URL...");
// Update the entry in the database with the PR URL
await db
.update(entries)
Expand All @@ -185,9 +251,10 @@ export const createPrTask = task({
orderBy: (entries, { asc }) => [asc(entries.createdAt)],
});

console.info("2.6 🎉 PR created. Returning the entry...");

return {
entry: updated,
message: `feat(glossary): Add ${input}.mdx to glossary`,
};
},
});
3 changes: 3 additions & 0 deletions apps/billing/src/trigger/glossary/draft-sections.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,9 @@ Guidelines:
6. Keep the content concise but informative, ensure that there are no fluff phrases or statements that don't provide concrete information, context & background to the term.
7. Don't repeat content between sections, ensure that each section adds value
8. Only write the content for the section, do not provide any other context, introductions or statements regarding this task.
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').
`;

const completion = await generateText({
Expand Down
Loading
Loading