Merged
Conversation
Signed-off-by: E99p1ant <i@github.red>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 14 minutes and 51 seconds. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds Playwright-based E2E tests and a GitHub Actions workflow that provisions PostgreSQL, Redis, MinIO, and MailHog, starts the Go backend and Vite frontend, runs browser tests, and collects/upload coverage and reports. Also adds frontend Recaptcha helpers, E2E configs, S3 uploader change, and dependency bumps. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ 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 |
Signed-off-by: E99p1ant <i@github.red>
Deploying nekobox-web with
|
| Latest commit: |
9614808
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://466609c1.nekobox-web.pages.dev |
| Branch Preview URL: | https://wh-e2e-test.nekobox-web.pages.dev |
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
![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>
Signed-off-by: E99p1ant <i@github.red>
Signed-off-by: E99p1ant <i@github.red>
Signed-off-by: E99p1ant <i@github.red>
Signed-off-by: E99p1ant <i@github.red>
Signed-off-by: E99p1ant <i@github.red>
Contributor
There was a problem hiding this comment.
Pull request overview
Adds an end-to-end testing setup (Playwright + GitHub Actions) and introduces supporting application changes to make E2E runs reliable (reCAPTCHA handling, backend upload behavior, and additional DB tests).
Changes:
- Add Playwright-based E2E test suite + CI workflow and supporting configs.
- Centralize and optionally bypass reCAPTCHA handling in the frontend (E2E/dev modes), updating auth/question flows to use the helper.
- Backend updates for S3 uploads (manager uploader), configurable reCAPTCHA verify URL, and expanded DB test coverage (censor metadata + SetName).
Reviewed changes
Copilot reviewed 21 out of 24 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| web/src/utils/recaptcha.ts | New helper to wait for reCAPTCHA readiness and fetch tokens (with E2E/test-key bypass). |
| web/src/pages/auth/SignUp.vue | Switch signup flow to use shared reCAPTCHA helper. |
| web/src/pages/auth/SignIn.vue | Switch signin flow to use shared reCAPTCHA helper. |
| web/src/pages/auth/ForgotPassword.vue | Switch forgot-password flow to use shared reCAPTCHA helper. |
| web/src/components/NewQuestion.vue | Switch question posting flow to use shared reCAPTCHA helper. |
| web/package.json | Add dev:e2e script to run Vite with --mode e2e. |
| web/.env.e2e | Add Vite env file for E2E mode (including reCAPTCHA test site key). |
| internal/route/user.go | Use AWS S3 manager uploader for image upload path. |
| internal/route/route.go | Allow overriding reCAPTCHA verify URL via config. |
| internal/db/users_test.go | Add unit test coverage for SetName. |
| internal/db/questions_test.go | Expand UpdateCensor test behavior + add checkTextCensorResponseValid test. |
| internal/conf/static.go | Add recaptcha.verify_url config field. |
| go.mod | Bump AWS SDK deps and add feature/s3/manager. |
| go.sum | Update dependency checksums for AWS SDK changes. |
| e2e/tests/questions.spec.ts | New E2E coverage for question lifecycle, email notifications, and image upload. |
| e2e/tests/helpers.ts | New helper utilities for E2E flows + MailHog polling. |
| e2e/tests/auth.spec.ts | New E2E coverage for auth flows and error cases. |
| e2e/playwright.config.ts | New Playwright config tuned for CI stability. |
| e2e/package.json | New E2E package definition for Playwright runner. |
| e2e/package-lock.json | Lockfile for E2E Node dependencies. |
| conf/app.e2e.ini | New backend config profile for E2E runs (DB/Redis/Mail/Upload/Recaptcha). |
| .gitignore | Ignore E2E artifacts and node_modules. |
| .github/workflows/go.yml | Upload Go test coverage to Codecov. |
| .github/workflows/e2e.yml | New GitHub Actions workflow to run E2E tests and upload artifacts/coverage. |
Files not reviewed (1)
- e2e/package-lock.json: Language not supported
Comments suppressed due to low confidence (1)
internal/route/user.go:312
- The error context string still says "put object" even though this code now uses the S3 manager uploader. Renaming the wrap message (or using a more general "upload object") will make logs/errors easier to understand.
uploader := manager.NewUploader(client)
if _, err := uploader.Upload(ctx.Request().Context(), &s3.PutObjectInput{
Bucket: aws.String(conf.Upload.ImageBucket),
Key: aws.String(fileKey),
Body: reader,
ContentLength: aws.Int64(fileSize),
}); err != nil {
return nil, errors.Wrap(err, "put object")
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
internal/db/users_test.go (1)
268-293: LGTM: Test implementation is correct and thorough.The test properly validates both the happy path and error handling:
- The "normal" subtest correctly verifies the name update persists by retrieving the user and asserting the Name field.
- The "empty name" subtest correctly validates input sanitization using
require.EqualError, which is appropriate since the implementation returns an unwrapped ad-hoc error rather than a defined error constant.💡 Optional: Consider adding test coverage for non-existent user
You might want to verify the behavior when SetName is called with a non-existent user ID:
t.Run("user not found", func(t *testing.T) { err := db.SetName(ctx, 999, "new-name") // Assert expected behavior - currently GORM Update returns nil for 0 affected rows // If you want to enforce user existence, the implementation would need to check RowsAffected require.Nil(t, err) // or require.Equal(t, ErrUserNotExists, err) if implementation changes })Note: GORM's
Updatedoes not error when updating 0 rows, so the current implementation returnsnilfor non-existent users. If you want to enforce user existence, the implementation would need to checkRowsAffected.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/db/users_test.go` around lines 268 - 293, Add a subtest to testUsersSetName that covers calling db.SetName with a non-existent user ID (e.g., 999) to document current behavior and prevent regressions: in a new t.Run("user not found", ...) call db.SetName(ctx, 999, "new-name") and assert the expected result (currently require.Nil(t, err) because GORM Update on 0 rows returns no error) or, if you change implementation to enforce existence, assert require.Equal(t, ErrUserNotExists, err); reference the test function testUsersSetName and the method SetName (and check RowsAffected in the implementation if you choose to change behavior).internal/db/questions_test.go (1)
376-403: Avoid hard-coded DB IDs in subtests.Using fixed ID
2makes this case fragile if fixture setup changes. Use the created question ID directly.♻️ Suggested stabilization
- _, err := db.Create(ctx, CreateQuestionOptions{ + question, err := db.Create(ctx, CreateQuestionOptions{ FromIP: "114.5.1.4", UserID: 1, Content: "Content - 2", @@ - err = db.UpdateCensor(ctx, 2, UpdateQuestionCensorOptions{ + err = db.UpdateCensor(ctx, question.ID, UpdateQuestionCensorOptions{ ContentCensorMetadata: originContentMetadata, AnswerCensorMetadata: originAnswerMetadata, }) @@ - err = db.UpdateCensor(ctx, 2, UpdateQuestionCensorOptions{ + err = db.UpdateCensor(ctx, question.ID, UpdateQuestionCensorOptions{ ContentCensorMetadata: []byte(`null`), AnswerCensorMetadata: []byte(`{"source_name":}`), }) @@ - got, err := db.GetByID(ctx, 2) + got, err := db.GetByID(ctx, question.ID)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/db/questions_test.go` around lines 376 - 403, The test uses a hard-coded ID (2) for UpdateCensor and GetByID which makes it fragile; capture the ID returned by db.Create (from Create(ctx, CreateQuestionOptions{...})) into a variable (e.g., createdID) and pass that variable into subsequent calls to db.UpdateCensor(ctx, createdID, UpdateQuestionCensorOptions{...}) and db.GetByID(ctx, createdID) so the test operates on the actual created question rather than a fixed DB ID.internal/route/route.go (1)
101-103: Validateverify_urlbefore overriding the endpoint.A malformed configured URL can degrade all auth flows. Validate and fall back to default selection when invalid.
♻️ Suggested hardening
import ( + "net/url" "net/http" "strings" "time" @@ Secret: conf.Recaptcha.ServerKey, VerifyURL: func() recaptcha.VerifyURL { - if conf.Recaptcha.VerifyURL != "" { - return recaptcha.VerifyURL(conf.Recaptcha.VerifyURL) + if raw := strings.TrimSpace(conf.Recaptcha.VerifyURL); raw != "" { + if _, err := url.ParseRequestURI(raw); err == nil { + return recaptcha.VerifyURL(raw) + } } if conf.Recaptcha.TurnstileStyle {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/route/route.go` around lines 101 - 103, The code currently overrides the recaptcha endpoint unconditionally when conf.Recaptcha.VerifyURL is non-empty; validate conf.Recaptcha.VerifyURL (e.g., parse with net/url and ensure a valid scheme and host) before calling recaptcha.VerifyURL(conf.Recaptcha.VerifyURL), and if validation fails, log a warning and fall back to the default endpoint selection path instead of using the malformed URL; locate the check around conf.Recaptcha.VerifyURL and the call to recaptcha.VerifyURL to add the validation and fallback logic..github/workflows/e2e.yml (1)
67-70: Consider pinning pnpm version for reproducible builds.Using
version: latestcan cause unexpected failures when a new pnpm version introduces breaking changes. Pinning to a specific version (e.g.,version: 9) improves CI stability.Proposed fix
- name: Set up pnpm uses: pnpm/action-setup@v4 with: - version: latest + version: 9🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/e2e.yml around lines 67 - 70, The workflow currently sets up pnpm using pnpm/action-setup@v4 with version: latest which can break reproducibility; change the action input "version" from "latest" to a pinned stable release (for example "9" or a specific minor/patch like "9.32.0") in the job step that uses pnpm/action-setup@v4 so CI uses a fixed pnpm version and avoids unexpected breakages.e2e/tests/auth.spec.ts (2)
75-77:awaitis unnecessary for synchronous expect assertions.
expect(duplicateSignUpPayload.msg).toContain('邮箱')is a synchronous assertion on a string value. Theawaitdoesn't cause harm but is misleading since no Promise is involved.Proposed fix
- await expect(duplicateSignUpPayload.msg).toContain('邮箱'); + expect(duplicateSignUpPayload.msg).toContain('邮箱');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/auth.spec.ts` around lines 75 - 77, The test uses an unnecessary await on a synchronous assertion: remove the await before the expect call so that expect(duplicateSignUpPayload.msg).toContain('邮箱') is executed synchronously; update the assertion in the test that references duplicateSignUpPayload.msg (in the failing test block in auth.spec.ts) to drop the leading await and leave the expect(...) call as a plain synchronous assertion.
105-107: Same unnecessaryawaitpattern.Proposed fix
- await expect(wrongPasswordSignInPayload.msg).toContain('密码'); + expect(wrongPasswordSignInPayload.msg).toContain('密码');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/auth.spec.ts` around lines 105 - 107, The first assertion uses an unnecessary await on a synchronous expectation: remove the leading await for the string check so use expect(wrongPasswordSignInPayload.msg).toContain('密码'); but keep the asynchronous page assertion awaited (await expect(page).toHaveURL('/sign-in')); — update the code lines referencing wrongPasswordSignInPayload.msg and the expect(page).toHaveURL call accordingly.
🤖 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/e2e.yml:
- Around line 132-141: The "Wait for frontend to be ready" step's retry loop can
silently continue when the frontend never becomes ready; modify the step so that
after the for loop completes it checks whether the curl ever succeeded and, if
not, prints a clear error message and exits non‑zero (e.g., set a success flag
inside the loop on curl success and after the loop test that flag and call exit
1 with a descriptive message if unset) so CI fails fast when the frontend never
starts.
In `@conf/app.e2e.ini`:
- Line 43: The backend is hardcoding the "https://" prefix when composing image
URLs in internal/route/user.go (the code that builds avatar/image URLs at the
sites where "https://" is prefixed), which conflicts with your E2E MinIO setup;
update the URL composition to use a configurable scheme instead of a hardcoded
"https://": either accept a full URL including scheme in image_bucket_cdn_host
(and trim/normalize it before use) or add a new config key (e.g.,
image_bucket_cdn_scheme) and use that when building URLs, and ensure the
URL-building functions in internal/route/user.go consume the config value
(image_bucket_cdn_host and/or image_bucket_cdn_scheme) so E2E can use "http"
while prod can use "https".
---
Nitpick comments:
In @.github/workflows/e2e.yml:
- Around line 67-70: The workflow currently sets up pnpm using
pnpm/action-setup@v4 with version: latest which can break reproducibility;
change the action input "version" from "latest" to a pinned stable release (for
example "9" or a specific minor/patch like "9.32.0") in the job step that uses
pnpm/action-setup@v4 so CI uses a fixed pnpm version and avoids unexpected
breakages.
In `@e2e/tests/auth.spec.ts`:
- Around line 75-77: The test uses an unnecessary await on a synchronous
assertion: remove the await before the expect call so that
expect(duplicateSignUpPayload.msg).toContain('邮箱') is executed synchronously;
update the assertion in the test that references duplicateSignUpPayload.msg (in
the failing test block in auth.spec.ts) to drop the leading await and leave the
expect(...) call as a plain synchronous assertion.
- Around line 105-107: The first assertion uses an unnecessary await on a
synchronous expectation: remove the leading await for the string check so use
expect(wrongPasswordSignInPayload.msg).toContain('密码'); but keep the
asynchronous page assertion awaited (await expect(page).toHaveURL('/sign-in'));
— update the code lines referencing wrongPasswordSignInPayload.msg and the
expect(page).toHaveURL call accordingly.
In `@internal/db/questions_test.go`:
- Around line 376-403: The test uses a hard-coded ID (2) for UpdateCensor and
GetByID which makes it fragile; capture the ID returned by db.Create (from
Create(ctx, CreateQuestionOptions{...})) into a variable (e.g., createdID) and
pass that variable into subsequent calls to db.UpdateCensor(ctx, createdID,
UpdateQuestionCensorOptions{...}) and db.GetByID(ctx, createdID) so the test
operates on the actual created question rather than a fixed DB ID.
In `@internal/db/users_test.go`:
- Around line 268-293: Add a subtest to testUsersSetName that covers calling
db.SetName with a non-existent user ID (e.g., 999) to document current behavior
and prevent regressions: in a new t.Run("user not found", ...) call
db.SetName(ctx, 999, "new-name") and assert the expected result (currently
require.Nil(t, err) because GORM Update on 0 rows returns no error) or, if you
change implementation to enforce existence, assert require.Equal(t,
ErrUserNotExists, err); reference the test function testUsersSetName and the
method SetName (and check RowsAffected in the implementation if you choose to
change behavior).
In `@internal/route/route.go`:
- Around line 101-103: The code currently overrides the recaptcha endpoint
unconditionally when conf.Recaptcha.VerifyURL is non-empty; validate
conf.Recaptcha.VerifyURL (e.g., parse with net/url and ensure a valid scheme and
host) before calling recaptcha.VerifyURL(conf.Recaptcha.VerifyURL), and if
validation fails, log a warning and fall back to the default endpoint selection
path instead of using the malformed URL; locate the check around
conf.Recaptcha.VerifyURL and the call to recaptcha.VerifyURL to add the
validation and fallback logic.
🪄 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: 754d0faa-ee7d-4be3-8c94-8f823b21c71b
⛔ Files ignored due to path filters (2)
e2e/package-lock.jsonis excluded by!**/package-lock.jsongo.sumis excluded by!**/*.sum
📒 Files selected for processing (22)
.github/workflows/e2e.yml.github/workflows/go.yml.gitignoreconf/app.e2e.inie2e/package.jsone2e/playwright.config.tse2e/tests/auth.spec.tse2e/tests/helpers.tse2e/tests/questions.spec.tsgo.modinternal/conf/static.gointernal/db/questions_test.gointernal/db/users_test.gointernal/route/route.gointernal/route/user.goweb/.env.e2eweb/package.jsonweb/src/components/NewQuestion.vueweb/src/pages/auth/ForgotPassword.vueweb/src/pages/auth/SignIn.vueweb/src/pages/auth/SignUp.vueweb/src/utils/recaptcha.ts
Signed-off-by: E99p1ant <i@github.red>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Tests
Chores
Refactor