Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds a GitHub Actions workflow for building and releasing CLI binaries across four platforms (Darwin ARM64/x64, Linux x64/ARM64) using matrix builds. Updates login page to support email-based magic link authentication via Supabase OTP with toast notifications. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…ntain permissions' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
apps/web/pages/login.tsx (1)
171-181: Add an explicit label for the email input (a11y).Placeholder-only inputs are weaker for assistive tech; add a real label (can be visually hidden).
Suggested fix
<form onSubmit={handleEmailLogin} className="space-y-3"> + <label htmlFor="email" className="sr-only"> + Email address + </label> <input + id="email" type="email" required autoComplete="email"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/pages/login.tsx` around lines 171 - 181, Add an explicit label element for the email input used in the login form: create a label (e.g., "Email address") and link it to the input by adding a matching id on the input and htmlFor on the label in the form that uses handleEmailLogin; the label may be visually hidden (e.g., using a "sr-only" utility class) to preserve the current visual layout while improving accessibility for assistive technologies; ensure the label references the input that has value bound to email and onChange setEmail and that the input keeps its existing attributes (type="email", required, disabled={sending || submitted}).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/cli-release.yml:
- Around line 3-4: The workflow currently allows workflow_dispatch to run on any
ref; restrict manual releases to main by updating the on.workflow_dispatch
trigger to include a branch filter (e.g., for refs/heads/main) or add a
top-level conditional that checks github.ref == 'refs/heads/main' so that the
dispatch only executes on main; apply the same guard to the other manual
dispatch block referenced by the second workflow_dispatch occurrence.
- Around line 28-31: Replace mutable version tags with immutable commit SHAs for
all GitHub Actions used in this workflow: pin actions/checkout (both
occurrences), oven-sh/setup-bun, actions/upload-artifact,
actions/download-artifact, and softprops/action-gh-release to their respective
full commit SHA references instead of `@v6/`@v2/@v6/@v6/@v2; locate the lines
using the identifiers actions/checkout, oven-sh/setup-bun,
actions/upload-artifact, actions/download-artifact, and
softprops/action-gh-release in the .github/workflows/cli-release.yml and update
each "uses:" value to the exact commit SHA (e.g., actions/checkout@<commit-sha>)
ensuring consistency and immutability across the workflow.
- Around line 30-34: Update the GitHub Actions steps named "Setup Bun" and
"Install dependencies": pin the Bun runner by adding the `bun-version` input
(e.g., `bun-version: "1.2.20"`) to the oven-sh/setup-bun@v2 step and switch the
install step from `bun install` to `bun ci` to enforce the lockfile; also ensure
`bun.lock` is committed to the repo so `bun ci` produces reproducible installs.
In `@apps/web/pages/login.tsx`:
- Around line 32-35: redirectTo is currently built via string concatenation and
doesn't handle redirectedFrom being a string[] or URL-encode the query; replace
the concat with a URL-based builder: create a new URL using
ROUTES.LOGIN_CALLBACK and getAppBaseURL(), normalize redirectedFrom (e.g. const
rf = Array.isArray(redirectedFrom) ? redirectedFrom[0] : redirectedFrom), and if
rf is present call url.searchParams.set('redirectedFrom', rf); then use
url.toString() for redirectTo to ensure safe, normalized query composition.
- Around line 37-59: In handleEmailLogin, ensure sending is always cleared by
wrapping the async supabase.auth.signInWithOtp call in try/catch/finally and
calling setSending(false) in the finally block; additionally, avoid
account-enumeration by not revealing "No account found" — when signInWithOtp
returns an error that indicates "signups not allowed" or "not found" treat it
the same as success (setSubmitted(true) and show the generic success
notification via notifySuccess("Check your email for the login link." or
similar), while still surfacing other unexpected errors with
notifyError(error.message)); reference handleEmailLogin, sending/setSending,
supabase.auth.signInWithOtp, setSubmitted, notifySuccess/notifyError.
---
Nitpick comments:
In `@apps/web/pages/login.tsx`:
- Around line 171-181: Add an explicit label element for the email input used in
the login form: create a label (e.g., "Email address") and link it to the input
by adding a matching id on the input and htmlFor on the label in the form that
uses handleEmailLogin; the label may be visually hidden (e.g., using a "sr-only"
utility class) to preserve the current visual layout while improving
accessibility for assistive technologies; ensure the label references the input
that has value bound to email and onChange setEmail and that the input keeps its
existing attributes (type="email", required, disabled={sending || submitted}).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 426355e6-07f8-4fbd-aad6-85bcec48ad3e
📒 Files selected for processing (2)
.github/workflows/cli-release.ymlapps/web/pages/login.tsx
| on: | ||
| workflow_dispatch: |
There was a problem hiding this comment.
Restrict release execution to main to avoid accidental off-branch releases.
workflow_dispatch can be run on arbitrary refs. Without a branch guard, Line 3 and Line 48 can release from non-main commits.
🔒 Proposed guard
jobs:
build:
+ if: github.ref == 'refs/heads/main'
name: Build ${{ matrix.target }}
@@
release:
+ if: github.ref == 'refs/heads/main'
name: Create ReleaseAlso applies to: 48-51
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/cli-release.yml around lines 3 - 4, The workflow currently
allows workflow_dispatch to run on any ref; restrict manual releases to main by
updating the on.workflow_dispatch trigger to include a branch filter (e.g., for
refs/heads/main) or add a top-level conditional that checks github.ref ==
'refs/heads/main' so that the dispatch only executes on main; apply the same
guard to the other manual dispatch block referenced by the second
workflow_dispatch occurrence.
| uses: actions/checkout@v6 | ||
|
|
||
| - name: Setup Bun | ||
| uses: oven-sh/setup-bun@v2 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify workflow action refs that are not SHA-pinned
set -euo pipefail
rg -n 'uses:\s*[^@]+@([vV].+|main|master)$' .github/workflows/cli-release.ymlRepository: techulus/changes-page
Length of output: 314
Pin action references to immutable commit SHAs.
Using mutable major version tags in a release workflow is a supply-chain risk. Lines 30, 33, 45, 59, 66, and 71 should be pinned to full commit SHAs:
- Line 30:
actions/checkout@v6 - Line 33:
oven-sh/setup-bun@v2 - Line 45:
actions/upload-artifact@v6 - Line 59:
actions/checkout@v6 - Line 66:
actions/download-artifact@v6 - Line 71:
softprops/action-gh-release@v2
🧷 Example hardening pattern
- - name: Checkout code
- uses: actions/checkout@v6
+ - name: Checkout code
+ uses: actions/checkout@<full_commit_sha> # v6
- - name: Setup Bun
- uses: oven-sh/setup-bun@v2
+ - name: Setup Bun
+ uses: oven-sh/setup-bun@<full_commit_sha> # v2
- - name: Upload artifact
- uses: actions/upload-artifact@v6
+ - name: Upload artifact
+ uses: actions/upload-artifact@<full_commit_sha> # v6🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/cli-release.yml around lines 28 - 31, Replace mutable
version tags with immutable commit SHAs for all GitHub Actions used in this
workflow: pin actions/checkout (both occurrences), oven-sh/setup-bun,
actions/upload-artifact, actions/download-artifact, and
softprops/action-gh-release to their respective full commit SHA references
instead of `@v6/`@v2/@v6/@v6/@v2; locate the lines using the identifiers
actions/checkout, oven-sh/setup-bun, actions/upload-artifact,
actions/download-artifact, and softprops/action-gh-release in the
.github/workflows/cli-release.yml and update each "uses:" value to the exact
commit SHA (e.g., actions/checkout@<commit-sha>) ensuring consistency and
immutability across the workflow.
| - name: Setup Bun | ||
| uses: oven-sh/setup-bun@v2 | ||
|
|
||
| - name: Install dependencies | ||
| run: cd packages/cli && bun install |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
According to Bun's official docs, what is the recommended CI command for lockfile-enforced installs: bun installorbun ci?
💡 Result:
According to Bun's official documentation, the recommended CI command for lockfile-enforced installs is bun ci. This command is equivalent to bun install --frozen-lockfile, installs exact versions from bun.lock, and fails if package.json does not match the lockfile. The docs explicitly state: "For CI/CD environments that want to enforce reproducible builds, use bun ci to fail the build if the package.json is out of sync with the lockfile" and "instead of running bun install, run bun ci". You must commit bun.lock to version control for this to work.
Citations:
- 1: https://bun.com/docs/pm/cli/install
- 2: https://bun.sh/docs/pm/cli/install
- 3: https://bun.com/docs/cli/install
🏁 Script executed:
# Check for .bun-version file
fd -i '\.bun-version$' || echo "No .bun-version file found"
# Check for bun.lock
fd -i 'bun\.lock$'
# Check package.json files for any bun version specification
fd 'package\.json$' -x grep -l "bun" {} \; 2>/dev/null | head -10Repository: techulus/changes-page
Length of output: 113
🏁 Script executed:
# Search for bun.lock files in the repository
find . -name "bun.lock*" -type f
# Check the packages/cli directory structure and look for package.json and bun.lock
ls -la packages/cli/ 2>/dev/null | head -20
# Check if there's a .bun-version or similar configuration in root or packages/cli
find . -maxdepth 2 -name ".*bun*" -o -name "*bun-version*"
# Check the content of packages/cli/package.json for bun references
cat packages/cli/package.json 2>/dev/null | head -50Repository: techulus/changes-page
Length of output: 1086
🏁 Script executed:
# Comprehensive search for bun.lock anywhere in repo
find . -type f -name "*bun*" | grep -v node_modules | grep -v ".git"
# Check root directory explicitly
ls -la | grep bun
# Search for any references to bun.lock or bun.ci in workflow files
rg "bun\.lock|bun ci" --type yamlRepository: techulus/changes-page
Length of output: 47
🏁 Script executed:
# Check if bun.lock exists in root
ls -la bun.lock 2>&1
# Look at the entire file list to see what lockfiles exist
ls -la *.lock 2>&1 || echo "No lockfiles found"
# Check git-tracked files for anything with 'bun' in name
git ls-files | grep -i bun || echo "No bun-related files in git"
# Verify the current workflow file content to understand full context
cat .github/workflows/cli-release.ymlRepository: techulus/changes-page
Length of output: 2441
Pin Bun version and ensure lockfile-enforced CI installs.
Line 31 installs the latest Bun implicitly, and Line 34 uses bun install without lockfile enforcement, making release outputs non-reproducible. Pin the Bun version in the workflow setup. To use bun ci for reproducible installs, first commit bun.lock to the repository (generated locally via bun install).
- Add
bun-versionto the setup step (e.g.,bun-version: "1.2.20") - Commit
bun.lockto the repository if not already present - Switch to
bun ciin the install step to enforce exact versions from the lockfile
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/cli-release.yml around lines 30 - 34, Update the GitHub
Actions steps named "Setup Bun" and "Install dependencies": pin the Bun runner
by adding the `bun-version` input (e.g., `bun-version: "1.2.20"`) to the
oven-sh/setup-bun@v2 step and switch the install step from `bun install` to `bun
ci` to enforce the lockfile; also ensure `bun.lock` is committed to the repo so
`bun ci` produces reproducible installs.
| const redirectTo = | ||
| getAppBaseURL() + | ||
| ROUTES.LOGIN_CALLBACK + | ||
| (redirectedFrom ? `?redirectedFrom=${redirectedFrom}` : ""); |
There was a problem hiding this comment.
Build redirectTo via URL to avoid malformed/unsafe query composition.
Current string concatenation does not normalize redirectedFrom (it can be string[]) and does not encode query content.
Suggested fix
- const redirectTo =
- getAppBaseURL() +
- ROUTES.LOGIN_CALLBACK +
- (redirectedFrom ? `?redirectedFrom=${redirectedFrom}` : "");
+ const redirectedFromValue = Array.isArray(redirectedFrom)
+ ? redirectedFrom[0]
+ : redirectedFrom;
+ const redirectUrl = new URL(ROUTES.LOGIN_CALLBACK, getAppBaseURL());
+ if (redirectedFromValue) {
+ redirectUrl.searchParams.set("redirectedFrom", redirectedFromValue);
+ }
+ const redirectTo = redirectUrl.toString();📝 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 redirectTo = | |
| getAppBaseURL() + | |
| ROUTES.LOGIN_CALLBACK + | |
| (redirectedFrom ? `?redirectedFrom=${redirectedFrom}` : ""); | |
| const redirectedFromValue = Array.isArray(redirectedFrom) | |
| ? redirectedFrom[0] | |
| : redirectedFrom; | |
| const redirectUrl = new URL(ROUTES.LOGIN_CALLBACK, getAppBaseURL()); | |
| if (redirectedFromValue) { | |
| redirectUrl.searchParams.set("redirectedFrom", redirectedFromValue); | |
| } | |
| const redirectTo = redirectUrl.toString(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/pages/login.tsx` around lines 32 - 35, redirectTo is currently built
via string concatenation and doesn't handle redirectedFrom being a string[] or
URL-encode the query; replace the concat with a URL-based builder: create a new
URL using ROUTES.LOGIN_CALLBACK and getAppBaseURL(), normalize redirectedFrom
(e.g. const rf = Array.isArray(redirectedFrom) ? redirectedFrom[0] :
redirectedFrom), and if rf is present call
url.searchParams.set('redirectedFrom', rf); then use url.toString() for
redirectTo to ensure safe, normalized query composition.
| async function handleEmailLogin(e: React.FormEvent) { | ||
| e.preventDefault(); | ||
| if (!email || sending) return; | ||
| setSending(true); | ||
| const { error } = await supabase.auth.signInWithOtp({ | ||
| email, | ||
| options: { shouldCreateUser: false, emailRedirectTo: redirectTo }, | ||
| }); | ||
| setSending(false); | ||
| if (error) { | ||
| const msg = error.message.toLowerCase(); | ||
| if (msg.includes("signups not allowed") || msg.includes("not found")) { | ||
| notifyError( | ||
| "No account found for this email. Sign up with Google or GitHub." | ||
| ); | ||
| } else { | ||
| notifyError(error.message); | ||
| } | ||
| return; | ||
| } | ||
| setSubmitted(true); | ||
| notifySuccess("Check your email for the login link."); | ||
| } |
There was a problem hiding this comment.
Harden OTP submit flow: always clear loading and avoid account-enumeration messaging.
Two issues here:
sendingis not reset if the async call throws."No account found..."reveals account existence; use a generic success-style response for both success and not-found cases.
Suggested fix
async function handleEmailLogin(e: React.FormEvent) {
e.preventDefault();
- if (!email || sending) return;
- setSending(true);
- const { error } = await supabase.auth.signInWithOtp({
- email,
- options: { shouldCreateUser: false, emailRedirectTo: redirectTo },
- });
- setSending(false);
- if (error) {
- const msg = error.message.toLowerCase();
- if (msg.includes("signups not allowed") || msg.includes("not found")) {
- notifyError(
- "No account found for this email. Sign up with Google or GitHub."
- );
- } else {
- notifyError(error.message);
- }
- return;
- }
- setSubmitted(true);
- notifySuccess("Check your email for the login link.");
+ const normalizedEmail = email.trim();
+ if (!normalizedEmail || sending) return;
+
+ setSending(true);
+ try {
+ const { error } = await supabase.auth.signInWithOtp({
+ email: normalizedEmail,
+ options: { shouldCreateUser: false, emailRedirectTo: redirectTo },
+ });
+
+ const msg = error?.message.toLowerCase() ?? "";
+ const isNotFound =
+ msg.includes("signups not allowed") || msg.includes("not found");
+
+ if (error && !isNotFound) {
+ notifyError(error.message);
+ return;
+ }
+
+ setSubmitted(true);
+ notifySuccess("If an account exists for this email, you'll receive a login link.");
+ } catch {
+ notifyError("Unable to send login link. Please try again.");
+ } finally {
+ setSending(false);
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/pages/login.tsx` around lines 37 - 59, In handleEmailLogin, ensure
sending is always cleared by wrapping the async supabase.auth.signInWithOtp call
in try/catch/finally and calling setSending(false) in the finally block;
additionally, avoid account-enumeration by not revealing "No account found" —
when signInWithOtp returns an error that indicates "signups not allowed" or "not
found" treat it the same as success (setSubmitted(true) and show the generic
success notification via notifySuccess("Check your email for the login link." or
similar), while still surfacing other unexpected errors with
notifyError(error.message)); reference handleEmailLogin, sending/setSending,
supabase.auth.signInWithOtp, setSubmitted, notifySuccess/notifyError.
2 participants
Summary by CodeRabbit
New Features
Chores