Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat: Optimistic writes #210

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Conversation

WillCorrigan
Copy link
Collaborator

@WillCorrigan WillCorrigan commented Nov 26, 2024

Description

Add optimistic writes to the solution, allowing to write to the db before ingestion and opens us up to be able to use useOptimistic()

Does this fix an issue? If so:
Fixes #207

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • New and existing unit tests pass locally with my changes

todo:

use useOptimistic() to provide optimistic state while waiting for db for a later PR

  • convert actions to use api layer where possible
  • refactor where can
  • fix race conditions between db/jetstream
  • fix weird consumed offset unique constraint bug (jetstream resending ops maybe?)
  • check if vote is already deleted before trying to delete db/record
  • make atproto creates use randomly generated rkey

Copy link

linear bot commented Nov 26, 2024

FP-108 Optimistic writes

Copy link

vercel bot commented Nov 26, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
frontpage ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 22, 2024 3:59pm
2 Skipped Deployments
Name Status Preview Comments Updated (UTC)
atproto-browser ⬜️ Skipped (Inspect) Dec 22, 2024 3:59pm
unravel ⬜️ Skipped (Inspect) Dec 22, 2024 3:59pm

@vercel vercel bot temporarily deployed to Preview – unravel November 27, 2024 05:11 Inactive
@vercel vercel bot temporarily deployed to Preview – atproto-browser November 27, 2024 05:11 Inactive
@WillCorrigan
Copy link
Collaborator Author

WillCorrigan commented Nov 27, 2024

todo:

  • convert actions to use api layer where possible
  • use useOptimistic() to provide optimistic state while waiting for db
  • refactor where can
  • fix race conditions between db/jetstream
  • fix weird consumed offset unique constraint bug (jetstream resending ops maybe?)

…te APIs; add consumed offset check in receive hook
…Did; update createPost to accept createdAt parameter; add new dependency for common-web
…ure proper async handling; add logging for delete operations
@vercel vercel bot temporarily deployed to Preview – unravel December 2, 2024 09:18 Inactive
@vercel vercel bot temporarily deployed to Preview – atproto-browser December 2, 2024 09:18 Inactive
…te createVote and related functions for improved readability and maintainability
@vercel vercel bot temporarily deployed to Preview – atproto-browser December 2, 2024 15:12 Inactive
@vercel vercel bot temporarily deployed to Preview – unravel December 2, 2024 15:12 Inactive
@WillCorrigan WillCorrigan marked this pull request as ready for review December 3, 2024 16:30
Copy link
Contributor

@tom-sherman tom-sherman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't reviewd the db/ changes yet but overall implementation looks good.

@@ -46,7 +46,7 @@ export const Post = sqliteTable(
{
id: integer("id").primaryKey(),
rkey: text("rkey").notNull(),
cid: text("cid").notNull().unique(),
cid: text("cid").notNull().default(""),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we not remove the non null constraint here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there's a hard dependency on cids for strong refs currently and if we wanna remove this and make it actually nullable it should be done in a future PR I think

packages/frontpage/lib/api/relayHandler.ts Outdated Show resolved Hide resolved
packages/frontpage/lib/api/vote.ts Outdated Show resolved Hide resolved
export type ApiCreateVoteInput = {
subject: {
rkey: string;
cid: string;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not used, should be removed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's used as a strong ref on line 41 for referencing the subject

packages/frontpage/app/(app)/post/new/_action.ts Outdated Show resolved Hide resolved

const bskyProfile = await getBlueskyProfile(user.did);

await sendDiscordMessage({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to go in the receive hook so that it's called on out-of-AppView writes also.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it already is as well

packages/frontpage/lib/api/post.ts Outdated Show resolved Hide resolved
packages/frontpage/lib/api/post.ts Outdated Show resolved Hide resolved
packages/frontpage/lib/api/vote.ts Outdated Show resolved Hide resolved
packages/frontpage/lib/data/atproto/post.ts Outdated Show resolved Hide resolved
… return values for post, vote, and comment retrieval, and ensuring proper async handling in delete operations
…ate logic; add checks to prevent redundant deletions and enhance error handling
… post, and comment actions; enhance validation to ensure users can only act on their own content
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Client-Side Exception: Unable to Post Comments
2 participants