-
Notifications
You must be signed in to change notification settings - Fork 5.5k
Biz/dj 2899 create a GitHub action to upsert any changed app files into #15614
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
Biz/dj 2899 create a GitHub action to upsert any changed app files into #15614
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 3 Skipped Deployments
|
Warning Rate limit exceeded@bzwrk has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 20 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis pull request introduces a new GitHub Action designed to push registry app files to Supabase. The changes include the addition of documentation (README), the action configuration (action.yml), the project setup (package.json), and the functional implementation (src/index.js). In addition, a workflow file is provided to trigger the action on pull requests to the master branch, specifically for modified files in the 'components/**' path. The action supports processing both Changes
Sequence Diagram(s)sequenceDiagram
participant W as GitHub Workflow
participant A as GitHub Action (push-registry-app-files-supabase)
participant BS as Build Process (pnpm)
participant SB as Supabase
W->>A: Trigger on pull request
A->>A: Parse "changed_files" input
A->>A: Filter files (.app.mjs, .app.ts)
alt File is .app.ts
A->>BS: Run "pnpm build" to generate .app.mjs file
BS-->>A: Return built file output
end
A->>SB: Upload payload to Supabase
Suggested labels
Suggested reviewers
Poem
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 (
|
Thank you so much for submitting this! We've added it to our backlog to review, and our team has been notified. |
Thanks for submitting this PR! When we review PRs, we follow the Pipedream component guidelines. If you're not familiar, here's a quick checklist:
|
|
||
import { createClient } from "@supabase/supabase-js"; | ||
|
||
const supabaseUrl = "https://ywxbxfcsskoyjwnlpqms.supabase.co" |
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.
Let's actually move this to env, I added a SUPABASE_URI
var that holds this full URL with protocol
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 (11)
.github/actions/push-registry-app-files-supabase/src/index.js (6)
1-5
: Avoid mixing CommonJS and ES Module imports.
You're mixingrequire
andimport
syntax in the same file, which may cause runtime issues unless you're transpiling. Stick to one module system for consistency.You could unify everything under CommonJS:
- import { createClient } from "@supabase/supabase-js"; + const { createClient } = require("@supabase/supabase-js");
11-15
: Confirm whether components in these directories should really be excluded.
Consider removing or limiting these ignores if you need to push app files from these folders in the future.
30-41
: Ensure robust error handling while reading files.
If any.app.mjs
file is missing or unreadable,fs.readFileSync
might throw an error. Either wrap in try/catch or maintain a fail-safe.
43-63
: Validate build commands for.app.ts
files.
The code runspnpm install -r && pnpm run build
when any.app.ts
is found. This is correct for local dev, but might slow down builds if your workspace grows. Consider caching or partially building only changed TS apps if possible.
65-77
: Optional chaining vs. null checks.
You can simplifyif (payload && payload.length)
by using optional chaining. That said, the current check is also totally valid.- if (payload && payload.length) { + if (payload?.length) {🧰 Tools
🪛 Biome (1.9.4)
[error] 66-66: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
79-90
: Consider parallelizing large file processing.
If you expect a large number of changed.app.mjs
or.app.ts
files, grouping them into parallel upload calls (or a single bulk upsert) might improve performance. Currently, you do a single array upsert, which is okay. If performance becomes a bottleneck, revisit..github/actions/push-registry-app-files-supabase/action.yml (1)
1-12
: Add a newline at file end.
Some linting rules require a trailing newline at the end of the file.name: "Push registry app files to Supabase" ... main: "dist/index.js" +
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 12-12: no new line character at the end of file
(new-line-at-end-of-file)
.github/actions/push-registry-app-files-supabase/package.json (1)
1-20
: Consider removing the duplicatencc
dependency.
You have both@vercel/ncc
andncc
in your dependencies. Typically,@vercel/ncc
is enough."dependencies": { "@actions/core": "^1.9.0", "@supabase/supabase-js": "^2.48.1", "@vercel/ncc": "^0.34.0", - "ncc": "^0.3.6" }
.github/actions/push-registry-app-files-supabase/README.md (3)
1-14
: Enhance documentation with additional details.The documentation would benefit from additional information about:
- The expected outcome after files are pushed to Supabase
- How
.app.ts
files are handled (mentioned in description but not detailed)- The expected format of the
changed_files
input
25-31
: Consider enhancing the example usage section.The example could be more comprehensive by:
- Showing a complete workflow example
- Including an example for
.app.ts
files- Adding comments explaining each step
33-44
: Enhance local development instructions.Consider adding:
- Prerequisites (Node.js version, required tools)
- Initial setup steps (npm install)
- Testing instructions
Also, consider rephrasing line 42-43 for better readability:
-You might want to change the `on` trigger to `push` event on your local branch when making changes to the action so you -can test it on GitHub: +To test the action on GitHub during development, consider changing the `on` trigger to a `push` event on your local branch:🧰 Tools
🪛 LanguageTool
[style] ~42-~42: Consider shortening or rephrasing this to strengthen your wording.
Context: ...push
event on your local branch when making changes to the action so you can test it on GitHub...(MAKE_CHANGES)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
.github/actions/push-registry-app-files-supabase/dist/index.js
is excluded by!**/dist/**
.github/actions/push-registry-app-files-supabase/dist/licenses.txt
is excluded by!**/dist/**
.github/actions/push-registry-app-files-supabase/package-lock.json
is excluded by!**/package-lock.json
pnpm-lock.yaml
is excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
.github/actions/push-registry-app-files-supabase/README.md
(1 hunks).github/actions/push-registry-app-files-supabase/action.yml
(1 hunks).github/actions/push-registry-app-files-supabase/package.json
(1 hunks).github/actions/push-registry-app-files-supabase/src/index.js
(1 hunks).github/workflows/push-registry-app-files-supabase.yaml
(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/push-registry-app-files-supabase.yaml
30-30: workflow command "set-output" was deprecated. use echo "{name}={value}" >> $GITHUB_OUTPUT
instead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions
(deprecated-commands)
51-51: invalid runner name "node16" at runs.using in "Push registry app files to Supabase" action defined at "/home/jailuser/git/.github/actions/push-registry-app-files-supabase". valid runners are "composite", "docker", and "node20". see https://docs.github.com/en/actions/creating-actions/metadata-syntax-for-github-actions#runs
(action)
🪛 LanguageTool
.github/actions/push-registry-app-files-supabase/README.md
[style] ~42-~42: Consider shortening or rephrasing this to strengthen your wording.
Context: ... push
event on your local branch when making changes to the action so you can test it on GitHub...
(MAKE_CHANGES)
🪛 markdownlint-cli2 (0.17.2)
.github/actions/push-registry-app-files-supabase/README.md
15-15: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
🪛 YAMLlint (1.35.1)
.github/actions/push-registry-app-files-supabase/action.yml
[error] 12-12: no new line character at the end of file
(new-line-at-end-of-file)
🪛 Biome (1.9.4)
.github/actions/push-registry-app-files-supabase/src/index.js
[error] 66-66: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (4)
.github/actions/push-registry-app-files-supabase/src/index.js (3)
7-9
: Check for safe handling of your environment secrets.
You're retrievingsupabaseKey
viacore.getInput("supabase_anon_key")
. This should be fine as long as the secret is properly masked in GitHub Actions logs.
17-28
: Validate directory checks withinshouldInclude
.
The logic correctly filters undesired directories and extensions. Ensure no newly introduced directories should be excluded.
91-92
: Graceful error handling.
Usingcore.setFailed
is a good default for failing the GitHub Action. Ensure that your error messages are sufficiently descriptive if the action fails..github/workflows/push-registry-app-files-supabase.yaml (1)
1-18
: LGTM!The workflow trigger and permissions are correctly configured for the intended use case.
``` | ||
... | ||
with: | ||
format: json | ||
``` | ||
|
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.
Add language specifier to the code block.
The code block is missing a language specifier, which affects syntax highlighting and documentation consistency.
Apply this diff:
-```
+```json
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
15-15: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
run: | | ||
echo "::set-output name=pnpm_cache_dir::$(pnpm store path)" |
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.
Update deprecated workflow command.
The set-output
command is deprecated. Use the new syntax instead.
Apply this diff:
- echo "::set-output name=pnpm_cache_dir::$(pnpm store path)"
+ echo "pnpm_cache_dir=$(pnpm store path)" >> $GITHUB_OUTPUT
📝 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.
run: | | |
echo "::set-output name=pnpm_cache_dir::$(pnpm store path)" | |
run: | | |
echo "pnpm_cache_dir=$(pnpm store path)" >> $GITHUB_OUTPUT |
🧰 Tools
🪛 actionlint (1.7.4)
30-30: workflow command "set-output" was deprecated. use echo "{name}={value}" >> $GITHUB_OUTPUT
instead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions
(deprecated-commands)
uses: ./.github/actions/push-registry-app-files-supabase | ||
with: |
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.
💡 Verification agent
🧩 Analysis chain
Update runner in action.yml.
The action's metadata file specifies an invalid runner (node16
). Update it to use node20
.
Run this script to verify the runner configuration:
🏁 Script executed:
#!/bin/bash
# Description: Check the runner configuration in action.yml
cat .github/actions/push-registry-app-files-supabase/action.yml | grep -A 5 "runs:"
Length of output: 131
Action Metadata Runner Update Required
The file .github/actions/push-registry-app-files-supabase/action.yml
currently specifies the runner as "node16"
(see below). This is invalid; please update it to "node20"
to meet the current requirements.
runs:
using: "node16" # Change to using: "node20"
main: "dist/index.js"
🧰 Tools
🪛 actionlint (1.7.4)
51-51: invalid runner name "node16" at runs.using in "Push registry app files to Supabase" action defined at "/home/jailuser/git/.github/actions/push-registry-app-files-supabase". valid runners are "composite", "docker", and "node20". see https://docs.github.com/en/actions/creating-actions/metadata-syntax-for-github-actions#runs
(action)
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: 2
♻️ Duplicate comments (1)
.github/workflows/push-registry-app-files-supabase.yaml (1)
30-31
: 🛠️ Refactor suggestionReplace deprecated set-output command.
GitHub Actions’
set-output
command is deprecated. Use the new syntax to write output variables:- echo "::set-output name=pnpm_cache_dir::$(pnpm store path)" + echo "pnpm_cache_dir=$(pnpm store path)" >> $GITHUB_OUTPUT🧰 Tools
🪛 actionlint (1.7.4)
30-30: workflow command "set-output" was deprecated. use
echo "{name}={value}" >> $GITHUB_OUTPUT
instead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions(deprecated-commands)
🧹 Nitpick comments (3)
.github/actions/push-registry-app-files-supabase/src/index.js (2)
1-5
: Use a consistent import style.You're mixing CommonJS (
require
) and ES Modules (import
). Consider staying consistent to avoid runtime issues in certain Node configurations.-const fs = require("fs") -const core = require("@actions/core"); -const { execSync } = require("child_process"); -import { createClient } from "@supabase/supabase-js"; +import fs from "fs"; +import * as core from "@actions/core"; +import { execSync } from "child_process"; +import { createClient } from "@supabase/supabase-js";
66-66
: Optional chaining might simplify null checks.Instead of checking
payload && payload.length
, you can leverage optional chaining to avoid potential null/undefined edge cases.-if (payload && payload.length) { +if (payload?.length) {🧰 Tools
🪛 Biome (1.9.4)
[error] 66-66: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
.github/actions/push-registry-app-files-supabase/action.yml (1)
15-15
: Add a newline at the end of the file.YAML files should end with a newline for best compatibility.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 15-15: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
.github/actions/push-registry-app-files-supabase/dist/index.js
is excluded by!**/dist/**
📒 Files selected for processing (3)
.github/actions/push-registry-app-files-supabase/action.yml
(1 hunks).github/actions/push-registry-app-files-supabase/src/index.js
(1 hunks).github/workflows/push-registry-app-files-supabase.yaml
(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.github/actions/push-registry-app-files-supabase/action.yml
[error] 15-15: no new line character at the end of file
(new-line-at-end-of-file)
🪛 Biome (1.9.4)
.github/actions/push-registry-app-files-supabase/src/index.js
[error] 66-66: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🪛 actionlint (1.7.4)
.github/workflows/push-registry-app-files-supabase.yaml
30-30: workflow command "set-output" was deprecated. use echo "{name}={value}" >> $GITHUB_OUTPUT
instead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions
(deprecated-commands)
51-51: invalid runner name "node22" at runs.using in "Push registry app files to Supabase" action defined at "/home/jailuser/git/.github/actions/push-registry-app-files-supabase". valid runners are "composite", "docker", and "node20". see https://docs.github.com/en/actions/creating-actions/metadata-syntax-for-github-actions#runs
(action)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Lint Code Base
- GitHub Check: pnpm publish
console.log("Generating mjs files from ts files for: ", appTsFiles) | ||
execSync(`pnpm install -r && pnpm run build`); |
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.
🛠️ Refactor suggestion
Add error handling for the build step.
Currently, execSync
will throw an error on failure without a graceful message. Consider wrapping it in a try/catch
to log more context when builds fail.
- execSync(`pnpm install -r && pnpm run build`);
+ try {
+ execSync(`pnpm install -r && pnpm run build`, { stdio: "inherit" });
+ } catch (error) {
+ console.error("Build step failed:", error);
+ throw error;
+ }
📝 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.
console.log("Generating mjs files from ts files for: ", appTsFiles) | |
execSync(`pnpm install -r && pnpm run build`); | |
console.log("Generating mjs files from ts files for: ", appTsFiles) | |
try { | |
execSync(`pnpm install -r && pnpm run build`, { stdio: "inherit" }); | |
} catch (error) { | |
console.error("Build step failed:", error); | |
throw error; | |
} |
description: "Supabase URI" | ||
required: true | ||
runs: | ||
using: "node22" |
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.
Use a valid Node runner.
“node22” is not a supported runner name in GitHub Actions. Please use “node20” or another officially recognized version.
- using: "node22"
+ using: "node20"
📝 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.
using: "node22" | |
using: "node20" |
WHY
Summary by CodeRabbit
New Features
Documentation