Add DB-backed test coverage and run Postgres tests in CI#249
Merged
dahlia merged 76 commits intohackers-pub:mainfrom Apr 16, 2026
Merged
Add DB-backed test coverage and run Postgres tests in CI#249dahlia merged 76 commits intohackers-pub:mainfrom
dahlia merged 76 commits intohackers-pub:mainfrom
Conversation
Move the rollback transaction helper and common account/post seeding logic into a shared test helper module. This lets future DB-backed model and GraphQL tests build on the same setup instead of copying the same boilerplate into each file. Refactor the existing reaction group regression test to use the shared helpers without changing its behavior. Assisted-by: OpenCode:gpt-5.4
Cover the DB-backed follow lifecycle in the models layer. These tests exercise local auto-accepted follows, pending remote follows that later become accepted, and unfollow cleanup for counts and notifications. This gives us direct coverage for the denormalized follower counters and the follow notification side effects that were previously untested. Assisted-by: OpenCode:gpt-5.4
Exercise the standard emoji reaction flow against Postgres in the models layer. The new tests cover creating a reaction, keeping reaction counts idempotent across duplicate attempts, and undoing a reaction while cleaning up notifications. The shared test federation context now exposes the URI helpers needed by reaction activities so these model tests can run without a full server context. Assisted-by: OpenCode:gpt-5.4
Add an integration test for the notification actors connection to ensure it preserves the trigger ordering stored in `notification.actorIds`. This covers the GraphQL resolver path directly so future refactors do not silently regress the actor ordering in notification UIs. Assisted-by: OpenCode:gpt-5.4
Cover the share and unshare flows in the models layer with Postgres- backed integration tests. The new tests verify share creation, idempotency, timeline fanout, and notification cleanup when a share is removed. The shared test federation context now provides a minimal KV stub so model paths that sync local actors can render markup without a full app runtime. Assisted-by: OpenCode:gpt-5.4
Move interaction counter updates in `deletePost()` to run after the matching rows have been deleted. The previous implementation updated reply counts before deleting the reply row, which left stale counts on parent posts. Add Postgres-backed regression tests for deleting replies and for cascading deletion of local replies, shares, note sources, and related notifications. Assisted-by: OpenCode:gpt-5.4
Cover the GraphQL mutation layer for post reactions and sharing. The new integration tests verify visibility rejection for hidden posts, success for visible reaction mutations, and the share/unshare round-trip through the GraphQL API. These checks complement the models-layer tests by validating the schema, error unions, and payload resolution against a real Postgres-backed context. Assisted-by: OpenCode:gpt-5.4
Add Postgres-backed tests for the core timeline write paths. The new coverage checks that direct posts only fan out to the author and mentioned accounts, and that removing the latest share falls back to the previous sharer in aggregated timeline entries. The shared test helpers now support remote actor and post fixtures so we can exercise timeline aggregation without unrelated local notification side effects. Assisted-by: OpenCode:gpt-5.4
Add Postgres-backed coverage for the timeline read paths. The new tests verify the public timeline's `local` and `withoutShares` filters, and check that the personal timeline hides pure share-only entries when `withoutShares` is enabled. This gives us direct regression coverage for the filtering logic that sits on top of the timeline tables and mixed local/remote post data. Assisted-by: OpenCode:gpt-5.4
Add integration tests for the public and personal timeline GraphQL connections. The new coverage checks forward pagination metadata on the public timeline and verifies that the `local` and `withoutShares` arguments are wired through correctly for both public and personal feeds. These tests complement the models-layer timeline coverage by asserting the connection payload and argument handling at the GraphQL boundary. Assisted-by: OpenCode:gpt-5.4
Add integration tests for the actor mutation layer. The new coverage checks self-follow rejection, follow and unfollow round-trips, removeFollower behavior, and block/unblock round-trips through the GraphQL API. These tests validate the error unions and payload resolution on top of the existing models-layer relationship coverage. Assisted-by: OpenCode:gpt-5.4
Extend the shared Postgres test helpers with a stateful in-memory KV, email transport capture, and guest context construction. This gives the GraphQL auth tests a reusable way to exercise signup, login, and passkey flows without pulling in the full runtime stack. Existing helpers now reuse the same KV instance across the GraphQL and Fedify sides of a test context so token and challenge state can be observed reliably. Assisted-by: OpenCode:gpt-5.4
Add integration coverage for the signup GraphQL flow. The new tests verify signup token lookup, validation error handling for taken usernames, and successful account creation with session issuance and inviter follow side effects. These checks exercise the GraphQL layer directly while reusing the new stateful KV helper for signup token and session state. Assisted-by: OpenCode:gpt-5.4
Add integration coverage for the login GraphQL flow. The new tests verify username and email challenge creation, challenge completion into a session, case-insensitive email lookup, and session revocation rules. These checks exercise the email transport, signin token storage, and session lifecycle at the GraphQL boundary. Assisted-by: OpenCode:gpt-5.4
Add integration coverage for the passkey GraphQL API where it is practical to exercise without synthesizing a full WebAuthn exchange. The new tests verify registration option issuance, authentication option issuance with unknown credentials returning null, and passkey revocation. These checks cover the KV-backed challenge storage and passkey mutation wiring at the GraphQL boundary. Assisted-by: OpenCode:gpt-5.4
Make local blocks tear down follow relationships in both directions, matching the behavior we already had for persisted remote block activities. Without this, blocking a local account left stale follow rows behind. Add Postgres-backed regression tests for blocking and unblocking so the relationship cleanup stays covered. Assisted-by: OpenCode:gpt-5.4
Add integration tests for invitation link creation, deletion, and redemption. The new coverage verifies invitation balance accounting, verify URL validation, email dispatch, and signup token storage for link redeems. The shared email test transport now supports single-message sends so the invitation GraphQL paths can be exercised without a real mail backend. Assisted-by: OpenCode:gpt-5.4
Reject invite requests whose verify URL does not interpolate the signup token, and add regression tests for the validation and email send paths. Delete the signup token when invitation delivery fails so the invitation state stays consistent after send errors.
Merge share-style notifications with an update-first path so duplicate notification inserts still work inside an open transaction. Add DB-backed tests that cover merged actor IDs and cleanup when the last actor is removed from a notification.
Cover signup token storage and deletion, and verify that account creation persists the inviter relationship and verified email.
Cover sign-in token storage and deletion, and verify that generated sign-in codes stay within the expected six-character uppercase format.
Cover passkey origin resolution and the model helpers that store registration and authentication challenges. Add regression coverage for missing registration options and for authentication attempts that do not have a stored challenge or a known credential.
Cover session creation, lookup, and deletion so the Keyv-backed session lifecycle has direct model-level regression coverage.
Cover persisted actor loading and actor post statistics with direct DB-backed tests. Cast aggregate actor stats to integers so the model returns numbers instead of PostgreSQL text values for note, share, and article counts.
Cover notification list ordering and relation loading so the model returns the newest notifications first and includes post and account relations as expected.
Cover username lookup after a rename and relationship state calculation at the model layer. These tests use node:test and node:assert/strict so the new DB-backed coverage stays aligned with the planned Node.js migration.
Cover viewer, accountByUsername, and invitationTree query behavior with Node-compatible GraphQL tests. These tests verify signed-in vs guest account lookup and ensure hidden accounts are redacted in the invitation tree output.
Cover searchObject for empty input, local handles, and local note URLs. These Node-compatible tests exercise the search redirect paths without requiring federation lookups.
Cover note source creation, updates, and lookup through renamed usernames. These Node-compatible DB-backed tests also verify that getNoteSource loads the related post and note media rows.
Cover note creation and updates at the model layer with Node-compatible DB-backed tests. These tests verify that authored notes create timeline entries and that updating a note source rewrites the persisted post content.
Remove follow relationships in both directions when a local account blocks another actor, regardless of whether the blockee is local or remote. Add a regression test that covers the remote-blockee case so stale follow rows are removed there as well.
Add a shared createTestDisk helper that exposes put and delete in addition to getUrl. Use that helper in the shared test contexts so tests that exercise disk writes through the common GraphQL/model harness do not fail on missing methods.
Member
Author
|
@codex review |
Member
Author
|
/gemini review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces several new GraphQL test files to improve test coverage for account, actor, invitation, and post-related functionality. The changes include adding new test suites and refining existing logic in graphql/invite.ts to improve code cleanliness. I have reviewed the comments and identified that the comment regarding test consistency is actionable and should be addressed to ensure all tests follow the Deno testing standard, while the comment regarding redundant checks in graphql/invite.ts provides a valid improvement opportunity.
Drop the redundant null checks for ctx.account and email in the invite validation guard, and use narrowed local bindings after the guard. This keeps the invite mutation logic equivalent while addressing the review feedback about duplicated validation conditions.
Member
Author
|
@codex review |
Member
Author
|
/gemini review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
♻️ Duplicate comments (1)
graphql/invite.ts (1)
213-217:⚠️ Potential issue | 🟠 MajorRollback sequencing can hide root errors and skip token cleanup.
If
refundInvitation()throws,cleanupSignupToken()is not attempted, and in thecatchpath the original email/build error gets masked.As per coding guidelines, "Use explicit typing for complex return types in TypeScript".💡 Suggested reliability fix
- const refundInvitation = () => - ctx.db.update(accountTable).set({ - leftInvitations: sql`${accountTable.leftInvitations} + 1`, - }).where(eq(accountTable.id, inviter.id)); + const refundInvitation = async (): Promise<void> => { + await ctx.db.update(accountTable).set({ + leftInvitations: sql`${accountTable.leftInvitations} + 1`, + }).where(eq(accountTable.id, inviter.id)); + }; @@ - await refundInvitation(); - await cleanupSignupToken(); + try { + await refundInvitation(); + } finally { + await cleanupSignupToken(); + } @@ - } catch (error) { - await refundInvitation(); - await cleanupSignupToken(); - throw error; - } + } catch (error) { + try { + await refundInvitation(); + } catch (refundError) { + logger.error( + "Failed to refund invitation after invite failure: {inviterId} {error}", + { inviterId: inviter.id, error: refundError }, + ); + } finally { + await cleanupSignupToken(); + } + throw error; + }Also applies to: 243-254
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@graphql/invite.ts` around lines 213 - 217, The current sequencing calls refundInvitation() before cleanupSignupToken(), so if refundInvitation() throws the signup token cleanup is skipped and the original email/build error can be masked; change the flow to always run cleanupSignupToken() (use try { await refundInvitation() } finally { await cleanupSignupToken() } or run cleanup in a finally block around the rollback) and ensure any errors from refundInvitation() do not swallow the initial error by capturing and rethrowing or aggregating errors appropriately; also add explicit TypeScript return types for these helper functions (refundInvitation and cleanupSignupToken) and any similar helper functions around lines 243-254 to satisfy the "explicit typing for complex return types" guideline.
🧹 Nitpick comments (4)
graphql/invite.ts (1)
221-224: Add stable context fields to LogTape error payloads.These logs are structured, but include only error payloads. Please add contextual identifiers (e.g.,
inviterId, phase) to improve traceability during incident triage.As per coding guidelines, "Use structured logging via LogTape for error handling and include context in error details".
Also applies to: 239-242
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@graphql/invite.ts` around lines 221 - 224, The logger.error calls in graphql/invite.ts (the "Failed to delete signup token after invite failure" and the similar call at the later location) only log the error object; update both calls to include stable structured context fields (e.g., inviterId, signupTokenId or signupToken, and a phase string like "delete-signup-token-after-invite-failure") so LogTape receives key/value context along with the error; locate the two logger.error invocations and augment their second argument to include these additional properties while preserving the error payload.test/postgres.ts (3)
132-166: Consider adding explicit return type annotations.Functions
insertRemoteActor,insertNotePost, andinsertRemotePostreturn complex object shapes but lack explicit return types. While TypeScript can infer these, explicit annotations improve API clarity and catch accidental signature changes.♻️ Example for insertRemoteActor
export async function insertRemoteActor( tx: Transaction, values: { username: string; name: string; host: string; iri?: string; inboxUrl?: string; }, -) { +): Promise<typeof actorTable.$inferSelect> {As per coding guidelines: "Use explicit typing for complex return types in TypeScript".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/postgres.ts` around lines 132 - 166, The three functions insertRemoteActor, insertNotePost, and insertRemotePost lack explicit return type annotations; add clear Promise-return types to each signature by defining or reusing appropriate interfaces/types that represent the DB record shapes they return (e.g., an ActorRecord or PostRecord type inferred from actorTable/postTable or declared DTOs) and annotate the functions as returning Promise<YourActorType> / Promise<YourPostType>; update insertRemoteActor, insertNotePost, and insertRemotePost signatures to use those types and ensure any local variables like the returned "actor" or "post" are asserted/typed accordingly so the compiler enforces the API surface.
172-172: Consider typingactorIdasUuidfor better type safety.The parameter is typed as
stringbut immediately cast toUuidat line 205. Typing it asUuid | undefinedwould catch misuse at compile time rather than silently accepting arbitrary strings.values: { account: AuthenticatedAccount; - actorId?: string; + actorId?: Uuid;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/postgres.ts` at line 172, Change the actorId parameter/type from string to Uuid (preferably Uuid | undefined) so callers are type-checked; update the declaration that currently reads "actorId?: string" to "actorId?: Uuid" (or "Uuid | undefined"), import the Uuid type where needed, and remove the runtime cast to Uuid where actorId is converted (the site that currently casts actorId to Uuid) so the code uses the strongly-typed value directly.
1-19: Consider reordering imports to place external before internal.Per coding guidelines, external imports should precede internal ones. Currently
@upyo/core(external) appears after@hackerspub/models/context(internal). A cleaner order would group all external imports first:// External import { assert } from "@std/assert/assert"; import type { RequestContext } from "@fedify/fedify"; import type { Transport } from "@upyo/core"; // Internal (`@hackerspub/`*) import type { ContextData } from "@hackerspub/models/context"; import type { Transaction } from "@hackerspub/models/db"; import { ... } from "@hackerspub/models/schema"; import { generateUuidV7 } from "@hackerspub/models/uuid"; import type { Uuid } from "@hackerspub/models/uuid"; // Relative import { db } from "../graphql/db.ts"; import type { UserContext } from "../graphql/builder.ts";As per coding guidelines: "Use external imports first, then internal imports (alphabetically within groups)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/postgres.ts` around lines 1 - 19, Reorder the import block so all external modules come first (e.g., move imports for assert from "@std/assert/assert", RequestContext from "@fedify/fedify", and Transport from "@upyo/core" to the top), then group internal "@hackerspub/*" imports together and alphabetize them (e.g., ContextData, Transaction, accountEmailTable/accountTable/actorTable/instanceTable/mentionTable/NewPost/noteSourceTable/postTable, generateUuidV7, Uuid), and finally keep relative imports last (db, UserContext); update the existing import list in this file to follow that ordering.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@graphql/invite.ts`:
- Around line 213-217: The current sequencing calls refundInvitation() before
cleanupSignupToken(), so if refundInvitation() throws the signup token cleanup
is skipped and the original email/build error can be masked; change the flow to
always run cleanupSignupToken() (use try { await refundInvitation() } finally {
await cleanupSignupToken() } or run cleanup in a finally block around the
rollback) and ensure any errors from refundInvitation() do not swallow the
initial error by capturing and rethrowing or aggregating errors appropriately;
also add explicit TypeScript return types for these helper functions
(refundInvitation and cleanupSignupToken) and any similar helper functions
around lines 243-254 to satisfy the "explicit typing for complex return types"
guideline.
---
Nitpick comments:
In `@graphql/invite.ts`:
- Around line 221-224: The logger.error calls in graphql/invite.ts (the "Failed
to delete signup token after invite failure" and the similar call at the later
location) only log the error object; update both calls to include stable
structured context fields (e.g., inviterId, signupTokenId or signupToken, and a
phase string like "delete-signup-token-after-invite-failure") so LogTape
receives key/value context along with the error; locate the two logger.error
invocations and augment their second argument to include these additional
properties while preserving the error payload.
In `@test/postgres.ts`:
- Around line 132-166: The three functions insertRemoteActor, insertNotePost,
and insertRemotePost lack explicit return type annotations; add clear
Promise-return types to each signature by defining or reusing appropriate
interfaces/types that represent the DB record shapes they return (e.g., an
ActorRecord or PostRecord type inferred from actorTable/postTable or declared
DTOs) and annotate the functions as returning Promise<YourActorType> /
Promise<YourPostType>; update insertRemoteActor, insertNotePost, and
insertRemotePost signatures to use those types and ensure any local variables
like the returned "actor" or "post" are asserted/typed accordingly so the
compiler enforces the API surface.
- Line 172: Change the actorId parameter/type from string to Uuid (preferably
Uuid | undefined) so callers are type-checked; update the declaration that
currently reads "actorId?: string" to "actorId?: Uuid" (or "Uuid | undefined"),
import the Uuid type where needed, and remove the runtime cast to Uuid where
actorId is converted (the site that currently casts actorId to Uuid) so the code
uses the strongly-typed value directly.
- Around line 1-19: Reorder the import block so all external modules come first
(e.g., move imports for assert from "@std/assert/assert", RequestContext from
"@fedify/fedify", and Transport from "@upyo/core" to the top), then group
internal "@hackerspub/*" imports together and alphabetize them (e.g.,
ContextData, Transaction,
accountEmailTable/accountTable/actorTable/instanceTable/mentionTable/NewPost/noteSourceTable/postTable,
generateUuidV7, Uuid), and finally keep relative imports last (db, UserContext);
update the existing import list in this file to follow that ordering.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c5ae9f41-2956-4e96-a289-783254b3ced2
📒 Files selected for processing (4)
graphql/invite.tsmodels/blocking.test.tsmodels/blocking.tstest/postgres.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- models/blocking.ts
- models/blocking.test.ts
Contributor
There was a problem hiding this comment.
Code Review
This pull request significantly expands the test coverage by introducing a large suite of database-backed tests for GraphQL mutations, queries, and core models, supported by new PostgreSQL test utilities. Key functional changes include a refactor of the notification system to handle event merging and idempotency, and improvements to post deletion logic for maintaining accurate interaction counts. Review feedback identifies a critical flaw in the follow notification logic that may cause notifications to be dropped, redundant error-type checking in the notification model, and an inconsistency in the use of Deno versus Node.js test runners across the new test files.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1d3eb11e87
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Let the insert-and-catch path handle the case where another transaction creates the same mergeable notification between the initial update and insert. Add a regression test that exercises concurrent share notification creation so both triggering actors are retained.
Avoid the originalPosts query in deletePost() when the post has no reply, share, or quote target IDs to resolve. Add a regression test for deleting a standalone post so the delete path stays correct without relying on an empty OR clause.
Member
Author
|
@codex review |
Member
Author
|
/gemini review |
Use a conflict-safe insert path for notifications so replayed or concurrent events do not abort the surrounding transaction on the unique index. When the insert becomes a no-op, follow up with the merge or existing notification lookup so actor merges still work and replayed events stay idempotent.
|
Codex Review: Didn't find any major issues. You're on a roll. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Contributor
There was a problem hiding this comment.
Code Review
This pull request significantly expands the test coverage for the GraphQL API and domain models by introducing a comprehensive suite of integration tests and a shared PostgreSQL test utility. It also includes critical logic refinements, such as handling email delivery failures in the invite mutation, ensuring follow relationships are pruned when blocking actors, and improving the robustness of notification merging. Review feedback identifies a potential bug in createNotification where no-op insertions might return undefined and suggests optimizing the merge logic to ensure transaction safety during unique violations.
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
node:testwithnode:assert/strictin the new tests.Fixes
deletePost().Testing
deno task testdeno task checkSummary by CodeRabbit
Bug Fixes
Tests
Chores