test: add basic necessary unit tests - #8
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ 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: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds extensive test coverage and refactors backend dependency injection. The backend introduces abstract interfaces for auth, database transactions, and token issuance, refactors the API server to use these abstractions, and adds comprehensive test coverage across API handlers and packages. The frontend establishes a complete test infrastructure using Bun, Testing Library, and MSW, then exercises authentication, token management, and form submission flows. The build configuration is updated to improve web task execution. ChangesBackend Refactoring and Testing Suite
🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/pkg/auth/auth.go (1)
187-191:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard against nil auth in sign-in token creation.
This dereferences
auth.ClerkUserIDwithout a nil check, so a nil input will panic the request path.Suggested fix
func (s *Service) CreateClerkSignInToken(auth *Auth) (string, error) { + if auth == nil || strings.TrimSpace(auth.ClerkUserID) == "" { + return "", errors.New("auth is required") + } + signInToken, err := s.ClerkSignInToken.Create(context.Background(), &signintoken.CreateParams{ UserID: &auth.ClerkUserID, ExpiresInSeconds: new(int64(20)), })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/pkg/auth/auth.go` around lines 187 - 191, In CreateClerkSignInToken, guard against a nil or invalid auth before dereferencing auth.ClerkUserID: check if auth == nil and if auth.ClerkUserID is nil/empty and return a clear error (instead of proceeding) so the subsequent call to s.ClerkSignInToken.Create does not panic; update the validation at the start of CreateClerkSignInToken to return an error when auth or auth.ClerkUserID is missing.
🧹 Nitpick comments (5)
internal/app/api/issues_test.go (1)
84-125: ⚡ Quick winAssert early short-circuit before DB access in auth/validation failures.
Nice coverage overall. At the moment, Line 89 and Line 110 bake in a repository lookup even when the request is unauthenticated or has a blank title. Switching these subtests to
failDBwould better protect the contract that auth/validation failures should return early without DB work.Suggested test tightening
t.Run("anonymous request returns unauthorized", func(t *testing.T) { - mock, err := pgxmock.NewPool() - assert.Nil(t, err) - defer mock.Close() - - expectRepositoryByOwnerAndName(mock, user.Name, repository.Name, repository) - router := NewRouter(ServerDeps{ - DB: mock, + DB: failDB{t: t}, Auth: testAuthProvider{user: user}, }) @@ router.ServeHTTP(response, request) assert.That(t, response.Code == http.StatusUnauthorized) - assert.Nil(t, mock.ExpectationsWereMet()) }) t.Run("blank title returns bad request", func(t *testing.T) { - mock, err := pgxmock.NewPool() - assert.Nil(t, err) - defer mock.Close() - - expectRepositoryByOwnerAndName(mock, user.Name, repository.Name, repository) - router := NewRouter(ServerDeps{ - DB: mock, + DB: failDB{t: t}, Auth: testAuthProvider{user: user}, }) @@ assert.That(t, response.Code == http.StatusBadRequest) - assert.Nil(t, mock.ExpectationsWereMet())🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/app/api/issues_test.go` around lines 84 - 125, The tests "anonymous request returns unauthorized" and "blank title returns bad request" currently set expectRepositoryByOwnerAndName(mock, ...) which allows/expects a DB lookup; change those to a failing DB expectation so the handler cannot hit the DB on auth/validation failures—replace the expectRepositoryByOwnerAndName calls with a call to failDB(mock) (or set mock to expect no queries/fail on any query) before creating the router, keeping the same ServerDeps and assertions, so the tests assert the code short-circuits before any repository lookup.web/src/test/setup.ts (1)
15-15: 💤 Low valueMinor: Redundant
SyntaxErrorassignment.
SyntaxErroris assigned toglobals.SyntaxErrortwice (lines 15 and 40). The second assignment is unnecessary.♻️ Proposed fix
globals.cancelAnimationFrame = window.cancelAnimationFrame.bind(window); -globals.SyntaxError = SyntaxError; globals.IS_REACT_ACT_ENVIRONMENT = true;Also applies to: 40-40
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/test/setup.ts` at line 15, The file assigns the same global twice (window.SyntaxError and a later globals.SyntaxError); remove the redundant duplicate assignment so SyntaxError is exported to globals only once—keep a single assignment (either window.SyntaxError = SyntaxError or globals.SyntaxError = SyntaxError) and delete the other occurrence to avoid the unnecessary duplication.web/src/components/views/CreateIssueForm.test.tsx (2)
75-106: 💤 Low valueConsider verifying routerRefreshCalls for consistency.
In the success test case (line 72), you verify
routerRefreshCalls, but this validation test doesn't check it. Whilst it's correct that no refresh should occur on validation failure, explicitly assertingexpect(routerRefreshCalls).toBe(0)would make the intent clearer and catch any unintended refresh calls.Suggested addition
expect(await screen.findByText("Title is required")).toBeDefined(); expect(requestBodies).toEqual([]); expect(routerPushCalls).toEqual([]); + expect(routerRefreshCalls).toBe(0); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/components/views/CreateIssueForm.test.tsx` around lines 75 - 106, The test "requires a title before creating an issue" currently asserts no network call and no router.push but omits checking router refresh; update the test to also assert that routerRefreshCalls is zero to ensure no refresh occurs on validation failure. Locate the test in CreateIssueForm.test.tsx (the test function with requestBodies and server.use) and add an assertion like expect(routerRefreshCalls).toBe(0) after the other expects so the test explicitly verifies the CreateIssueForm component did not trigger a router refresh on validation error.
10-107: ⚡ Quick winConsider adding test coverage for error scenarios.
The current tests cover the happy path and validation, which is great for a first pass. However, you might want to add a test case for API error handling (e.g., when the server returns a 500 or 4xx error). This would verify that the form displays appropriate error messages and doesn't navigate on failure.
Example test case:
test("displays error message when API fails", async () => { server.use( http.post( "http://catena.test/v1/repositories/:owner/:repository/issues", () => { return HttpResponse.json( { message: "Internal server error" }, { status: 500 } ); } ) ); const CreateIssueForm = await import("./CreateIssueForm").then( (mod) => mod.default ); renderWithQueryClient( <CreateIssueForm ownerName="floffah" repoName="catena" /> ); await userEvent.type(screen.getByLabelText("Title"), "Test issue"); await userEvent.click( screen.getByRole("button", { name: "Create issue" }) ); expect(await screen.findByText(/error/i)).toBeDefined(); expect(routerPushCalls).toEqual([]); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/components/views/CreateIssueForm.test.tsx` around lines 10 - 107, Add a test that exercises API error handling for CreateIssueForm: use server.use with http.post to return an error response (e.g., HttpResponse.json({...}, { status: 500 })), import the CreateIssueForm component as in the existing tests, render it via renderWithQueryClient, fill the Title/Body (userEvent.type) and click the Create issue button, then assert that an error message is shown (await screen.findByText(/error/i) or similar) and that requestBodies remains empty or routerPushCalls is unchanged (routerPushCalls === []), ensuring the form does not navigate on failure and matches the pattern used in the other tests.web/src/components/views/RepositorySettingsForm.test.tsx (1)
23-86: ⚡ Quick winConsider adding a test for API error handling.
Similar to the CreateIssueForm tests, it would be valuable to verify how the form behaves when the API returns an error. This ensures users receive appropriate feedback when updates fail.
Example test case:
test("displays error message when update fails", async () => { server.use( http.patch( "http://catena.test/v1/repositories/:owner/:repository", () => { return HttpResponse.json( { message: "Update failed" }, { status: 500 } ); } ) ); const RepositorySettingsForm = await import("./RepositorySettingsForm").then((mod) => mod.default); renderWithQueryClient( <RepositorySettingsForm branchNames={["main", "develop"]} repository={repository} /> ); await userEvent.type( screen.getByLabelText("Description (optional)"), "New description" ); await userEvent.click( screen.getByRole("button", { name: "Save Changes" }) ); expect(await screen.findByText(/error|failed/i)).toBeDefined(); expect(routerRefreshCalls).toBe(0); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/components/views/RepositorySettingsForm.test.tsx` around lines 23 - 86, Add a test that verifies RepositorySettingsForm shows an error message and does not trigger a router refresh when the update API returns an error: in the test suite add a new test (e.g., "displays error message when update fails") that uses server.use with http.patch for the same "http://catena.test/v1/repositories/:owner/:repository" route to return an HttpResponse.json({ message: "Update failed" }, { status: 500 }), then import RepositorySettingsForm, render it with renderWithQueryClient (passing branchNames and repository), simulate changing the form (type into the Description field and click the "Save Changes" button), and assert that await screen.findByText(/error|failed/i) is defined and that routerRefreshCalls remains 0 to confirm no refresh happened.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/app/api/util_test.go`:
- Around line 79-99: Replace the realistic PAT-style literal used in the
GitAccessToken fixture and its assertion with a clearly synthetic value to avoid
leak-scanner noise: in the GitAccessTokenToAPI test where TokenPrefix is set
(the db.GitAccessToken TokenPrefix field) change "ctn_pat_12345678" to a
non-secret-looking string (e.g. "token_prefix_test") and update the
corresponding assertion that checks got.TokenPrefix to expect that synthetic
value instead.
In `@justfile`:
- Line 30: The go test invocation includes a redundant -cover flag; update the
command string "go test -v -coverprofile=coverage.out -cover ./..." by removing
the standalone -cover so it reads "go test -v -coverprofile=coverage.out ./..."
to rely on -coverprofile for coverage collection.
In `@web/src/providers/AuthProvider.test.tsx`:
- Around line 8-20: The test file sets
process.env.NEXT_PUBLIC_CATENA_INSTANCE_URL at module scope and calls
mock.module("`@clerk/nextjs`", ...) at import time, which runs before your test
preload/setup and can trigger "document is not defined"; remove the module-level
env assignment (process.env.NEXT_PUBLIC_CATENA_INSTANCE_URL) and relocate the
Clerk mock.module call into the test body or a per-test beforeEach so
useAuth/getToken are mocked only during test execution (or create a dedicated
test setup file) rather than at module import.
---
Outside diff comments:
In `@internal/pkg/auth/auth.go`:
- Around line 187-191: In CreateClerkSignInToken, guard against a nil or invalid
auth before dereferencing auth.ClerkUserID: check if auth == nil and if
auth.ClerkUserID is nil/empty and return a clear error (instead of proceeding)
so the subsequent call to s.ClerkSignInToken.Create does not panic; update the
validation at the start of CreateClerkSignInToken to return an error when auth
or auth.ClerkUserID is missing.
---
Nitpick comments:
In `@internal/app/api/issues_test.go`:
- Around line 84-125: The tests "anonymous request returns unauthorized" and
"blank title returns bad request" currently set
expectRepositoryByOwnerAndName(mock, ...) which allows/expects a DB lookup;
change those to a failing DB expectation so the handler cannot hit the DB on
auth/validation failures—replace the expectRepositoryByOwnerAndName calls with a
call to failDB(mock) (or set mock to expect no queries/fail on any query) before
creating the router, keeping the same ServerDeps and assertions, so the tests
assert the code short-circuits before any repository lookup.
In `@web/src/components/views/CreateIssueForm.test.tsx`:
- Around line 75-106: The test "requires a title before creating an issue"
currently asserts no network call and no router.push but omits checking router
refresh; update the test to also assert that routerRefreshCalls is zero to
ensure no refresh occurs on validation failure. Locate the test in
CreateIssueForm.test.tsx (the test function with requestBodies and server.use)
and add an assertion like expect(routerRefreshCalls).toBe(0) after the other
expects so the test explicitly verifies the CreateIssueForm component did not
trigger a router refresh on validation error.
- Around line 10-107: Add a test that exercises API error handling for
CreateIssueForm: use server.use with http.post to return an error response
(e.g., HttpResponse.json({...}, { status: 500 })), import the CreateIssueForm
component as in the existing tests, render it via renderWithQueryClient, fill
the Title/Body (userEvent.type) and click the Create issue button, then assert
that an error message is shown (await screen.findByText(/error/i) or similar)
and that requestBodies remains empty or routerPushCalls is unchanged
(routerPushCalls === []), ensuring the form does not navigate on failure and
matches the pattern used in the other tests.
In `@web/src/components/views/RepositorySettingsForm.test.tsx`:
- Around line 23-86: Add a test that verifies RepositorySettingsForm shows an
error message and does not trigger a router refresh when the update API returns
an error: in the test suite add a new test (e.g., "displays error message when
update fails") that uses server.use with http.patch for the same
"http://catena.test/v1/repositories/:owner/:repository" route to return an
HttpResponse.json({ message: "Update failed" }, { status: 500 }), then import
RepositorySettingsForm, render it with renderWithQueryClient (passing
branchNames and repository), simulate changing the form (type into the
Description field and click the "Save Changes" button), and assert that await
screen.findByText(/error|failed/i) is defined and that routerRefreshCalls
remains 0 to confirm no refresh happened.
In `@web/src/test/setup.ts`:
- Line 15: The file assigns the same global twice (window.SyntaxError and a
later globals.SyntaxError); remove the redundant duplicate assignment so
SyntaxError is exported to globals only once—keep a single assignment (either
window.SyntaxError = SyntaxError or globals.SyntaxError = SyntaxError) and
delete the other occurrence to avoid the unnecessary duplication.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: eba1be36-eec3-4bd5-8fe5-bfca0105eec6
⛔ Files ignored due to path filters (2)
bun.lockis excluded by!**/*.lockgo.sumis excluded by!**/*.sum
📒 Files selected for processing (29)
go.modinternal/app/api/api.gointernal/app/api/git_access_tokens.gointernal/app/api/git_access_tokens_test.gointernal/app/api/issues_test.gointernal/app/api/repositories_test.gointernal/app/api/test_helpers_test.gointernal/app/api/users_test.gointernal/app/api/util_test.gointernal/app/gitserver/server_test.gointernal/pkg/auth/auth.gointernal/pkg/db/orchestra.gointernal/pkg/gitauth/gitauth.gointernal/pkg/gitauth/gitauth_test.gointernal/pkg/gitstore/gitstore_test.gointernal/pkg/repositoryitems/repositoryitems_test.gointernal/pkg/util/strings.gojustfileweb/package.jsonweb/src/app/(site)/settings/tokens/new/page.test.tsxweb/src/components/blocks/PersonalAccessTokenList.test.tsxweb/src/components/views/CreateIssueForm.test.tsxweb/src/components/views/RepositorySettingsForm.test.tsxweb/src/noop.test.tsweb/src/providers/AuthProvider.test.tsxweb/src/test/navigation.tsweb/src/test/render.tsxweb/src/test/server.tsweb/src/test/setup.ts
💤 Files with no reviewable changes (2)
- web/src/noop.test.ts
- internal/pkg/util/strings.go
| got, err := GitAccessTokenToAPI(db.GitAccessToken{ | ||
| ID: UUIDToPgtype(tokenID), | ||
| Name: "Local laptop", | ||
| TokenPrefix: "ctn_pat_12345678", | ||
| Scopes: []string{gitauth.ScopeRepoRead, gitauth.ScopeRepoWrite}, | ||
| LastUsedAt: pgtype.Timestamptz{Time: lastUsedAt, Valid: true}, | ||
| ExpiresAt: pgtype.Timestamptz{Time: expiresAt, Valid: true}, | ||
| CreatedAt: pgtype.Timestamptz{Time: createdAt, Valid: true}, | ||
| UpdatedAt: pgtype.Timestamptz{Time: updatedAt, Valid: true}, | ||
| }) | ||
|
|
||
| assert.Nil(t, err) | ||
| assert.That(t, got.Id == tokenID) | ||
| assert.That(t, got.Name == "Local laptop") | ||
| assert.That(t, got.TokenPrefix == "ctn_pat_12345678") | ||
| assert.That(t, got.LastUsedAt != nil && got.LastUsedAt.Equal(lastUsedAt)) | ||
| assert.That(t, got.ExpiresAt != nil && got.ExpiresAt.Equal(expiresAt)) | ||
| assert.That(t, got.RevokedAt == nil) | ||
| assert.That(t, len(got.Scopes) == 2) | ||
| assert.That(t, got.Scopes[0] == gitauth.ScopeRepoRead) | ||
| assert.That(t, got.Scopes[1] == gitauth.ScopeRepoWrite) |
There was a problem hiding this comment.
Use non-secret-looking token fixtures to avoid leak-scanner noise.
Line 82 and Line 93 currently use a realistic PAT-style literal and are being flagged by Betterleaks. Swapping to a clearly synthetic value keeps the test intent while reducing security-tool false positives.
Small fixture tweak
- TokenPrefix: "ctn_pat_12345678",
+ TokenPrefix: "test_token_prefix",
@@
- assert.That(t, got.TokenPrefix == "ctn_pat_12345678")
+ assert.That(t, got.TokenPrefix == "test_token_prefix")📝 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.
| got, err := GitAccessTokenToAPI(db.GitAccessToken{ | |
| ID: UUIDToPgtype(tokenID), | |
| Name: "Local laptop", | |
| TokenPrefix: "ctn_pat_12345678", | |
| Scopes: []string{gitauth.ScopeRepoRead, gitauth.ScopeRepoWrite}, | |
| LastUsedAt: pgtype.Timestamptz{Time: lastUsedAt, Valid: true}, | |
| ExpiresAt: pgtype.Timestamptz{Time: expiresAt, Valid: true}, | |
| CreatedAt: pgtype.Timestamptz{Time: createdAt, Valid: true}, | |
| UpdatedAt: pgtype.Timestamptz{Time: updatedAt, Valid: true}, | |
| }) | |
| assert.Nil(t, err) | |
| assert.That(t, got.Id == tokenID) | |
| assert.That(t, got.Name == "Local laptop") | |
| assert.That(t, got.TokenPrefix == "ctn_pat_12345678") | |
| assert.That(t, got.LastUsedAt != nil && got.LastUsedAt.Equal(lastUsedAt)) | |
| assert.That(t, got.ExpiresAt != nil && got.ExpiresAt.Equal(expiresAt)) | |
| assert.That(t, got.RevokedAt == nil) | |
| assert.That(t, len(got.Scopes) == 2) | |
| assert.That(t, got.Scopes[0] == gitauth.ScopeRepoRead) | |
| assert.That(t, got.Scopes[1] == gitauth.ScopeRepoWrite) | |
| got, err := GitAccessTokenToAPI(db.GitAccessToken{ | |
| ID: UUIDToPgtype(tokenID), | |
| Name: "Local laptop", | |
| TokenPrefix: "test_token_prefix", | |
| Scopes: []string{gitauth.ScopeRepoRead, gitauth.ScopeRepoWrite}, | |
| LastUsedAt: pgtype.Timestamptz{Time: lastUsedAt, Valid: true}, | |
| ExpiresAt: pgtype.Timestamptz{Time: expiresAt, Valid: true}, | |
| CreatedAt: pgtype.Timestamptz{Time: createdAt, Valid: true}, | |
| UpdatedAt: pgtype.Timestamptz{Time: updatedAt, Valid: true}, | |
| }) | |
| assert.Nil(t, err) | |
| assert.That(t, got.Id == tokenID) | |
| assert.That(t, got.Name == "Local laptop") | |
| assert.That(t, got.TokenPrefix == "test_token_prefix") | |
| assert.That(t, got.LastUsedAt != nil && got.LastUsedAt.Equal(lastUsedAt)) | |
| assert.That(t, got.ExpiresAt != nil && got.ExpiresAt.Equal(expiresAt)) | |
| assert.That(t, got.RevokedAt == nil) | |
| assert.That(t, len(got.Scopes) == 2) | |
| assert.That(t, got.Scopes[0] == gitauth.ScopeRepoRead) | |
| assert.That(t, got.Scopes[1] == gitauth.ScopeRepoWrite) |
🧰 Tools
🪛 Betterleaks (1.2.0)
[high] 82-82: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 93-93: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/app/api/util_test.go` around lines 79 - 99, Replace the realistic
PAT-style literal used in the GitAccessToken fixture and its assertion with a
clearly synthetic value to avoid leak-scanner noise: in the GitAccessTokenToAPI
test where TokenPrefix is set (the db.GitAccessToken TokenPrefix field) change
"ctn_pat_12345678" to a non-secret-looking string (e.g. "token_prefix_test") and
update the corresponding assertion that checks got.TokenPrefix to expect that
synthetic value instead.
|
|
||
| test: | ||
| go test -v -coverprofile=coverage.out ./... | ||
| go test -v -coverprofile=coverage.out -cover ./... |
There was a problem hiding this comment.
Remove redundant -cover flag.
The -cover flag is unnecessary here since -coverprofile=coverage.out already enables coverage collection. The -coverprofile flag implicitly turns on coverage, so explicitly specifying -cover is redundant.
♻️ Proposed fix
- go test -v -coverprofile=coverage.out -cover ./...
+ go test -v -coverprofile=coverage.out ./...📝 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.
| go test -v -coverprofile=coverage.out -cover ./... | |
| go test -v -coverprofile=coverage.out ./... |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@justfile` at line 30, The go test invocation includes a redundant -cover
flag; update the command string "go test -v -coverprofile=coverage.out -cover
./..." by removing the standalone -cover so it reads "go test -v
-coverprofile=coverage.out ./..." to rely on -coverprofile for coverage
collection.
| process.env.NEXT_PUBLIC_CATENA_INSTANCE_URL = "http://catena.test"; | ||
|
|
||
| let token = "initial-token"; | ||
|
|
||
| mock.module("@clerk/nextjs", () => ({ | ||
| useAuth() { | ||
| return { | ||
| getToken: async () => token, | ||
| isLoaded: true, | ||
| isSignedIn: true, | ||
| }; | ||
| }, | ||
| })); |
There was a problem hiding this comment.
Critical: Module-level mocking causes test setup conflict.
The pipeline failure shows document is not defined at line 63, despite setup.ts establishing DOM globals via --preload. The module-level mock.module() call (line 12) and process.env assignment (line 8) execute during import, which can run before the preload completes or in a different context.
Additionally, line 8 redundantly sets NEXT_PUBLIC_CATENA_INSTANCE_URL (already set in setup.ts).
Solution: Move the Clerk mock into the test body or into a separate per-test setup, and remove the redundant env var assignment.
🔧 Proposed fix
Remove lines 8 and 10-20, and move the mock inside the test:
-process.env.NEXT_PUBLIC_CATENA_INSTANCE_URL = "http://catena.test";
-
let token = "initial-token";
-mock.module("`@clerk/nextjs`", () => ({
- useAuth() {
- return {
- getToken: async () => token,
- isLoaded: true,
- isSignedIn: true,
- };
- },
-}));
-
const authHeaders: (string | null)[] = [];
afterEach(() => {Then inside the test (after line 30):
describe("AuthProvider", () => {
test("adds the latest Clerk token to API requests", async () => {
+ mock.module("`@clerk/nextjs`", () => ({
+ useAuth() {
+ return {
+ getToken: async () => token,
+ isLoaded: true,
+ isSignedIn: true,
+ };
+ },
+ }));
+
server.use(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/src/providers/AuthProvider.test.tsx` around lines 8 - 20, The test file
sets process.env.NEXT_PUBLIC_CATENA_INSTANCE_URL at module scope and calls
mock.module("`@clerk/nextjs`", ...) at import time, which runs before your test
preload/setup and can trigger "document is not defined"; remove the module-level
env assignment (process.env.NEXT_PUBLIC_CATENA_INSTANCE_URL) and relocate the
Clerk mock.module call into the test body or a per-test beforeEach so
useAuth/getToken are mocked only during test execution (or create a dedicated
test setup file) rather than at module import.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the setup.ts file to see if NEXT_PUBLIC_CATENA_INSTANCE_URL is set there
cat web/src/test/setup.ts
echo "---"
# Check the current state of the AuthProvider test file
cat web/src/providers/AuthProvider.test.tsx
echo "---"
# Check if env var is set anywhere else in test files
rg "NEXT_PUBLIC_CATENA_INSTANCE_URL" web/src --type=tsx -n -A1 -B1
rg "NEXT_PUBLIC_CATENA_INSTANCE_URL" web/src -n -A1 -B1
Length of output: 5003
@Floffah, after looking at this more carefully, I need to walk back most of my original comment — sorry for the noise! 🐇
Here's what the re-review actually shows:
mock.module at module scope — my concern was wrong. Bun's --preload ./src/test/setup.ts guarantees the setup file (and its DOM globals like document) runs before any test module is loaded. There's no ordering hazard here. Additionally, top-level mock.module() is the correct and idiomatic Bun pattern — it's hoisted before ESM imports resolve, so moving it inside the test body would actually break the mock.
process.env.NEXT_PUBLIC_CATENA_INSTANCE_URL — still a minor nit. setup.ts line 7 already sets this to "http://catena.test", so line 8 in AuthProvider.test.tsx is a redundant duplicate. It's completely harmless since both values are identical, but it could be cleaned up if you want to keep things tidy:
-process.env.NEXT_PUBLIC_CATENA_INSTANCE_URL = "http://catena.test";
-
let token = "initial-token";Other than that one minor nit, the current structure of the file is fine. Again, apologies for the misleading original comment!
🧠 Learnings used
Learnt from: Floffah
Repo: Floffah/catena PR: 6
File: web/src/app/(site)/[ownerName]/[repoName]/layout.tsx:35-40
Timestamp: 2026-05-21T14:33:33.229Z
Learning: In the web/ (Next.js) frontend, treat `NEXT_PUBLIC_CATENA_INSTANCE_URL` as the backend/API domain (not the frontend app origin). When building frontend-facing absolute URLs—e.g., Open Graph/Twitter image URLs and `metadataBase` in route layouts/pages—use a separate env var such as `NEXT_PUBLIC_APP_URL` that represents the frontend origin (scheme + host, optionally port). Avoid using `NEXT_PUBLIC_CATENA_INSTANCE_URL` for browser-visible origins.
1 participant
What this changes
Unit tests
How I tested this
tests.
Checklist