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

Current commit management #40

Closed
wants to merge 1 commit into from
Closed

Current commit management #40

wants to merge 1 commit into from

Conversation

csansoon
Copy link
Contributor

@csansoon csansoon commented Jul 16, 2024

Now the user will be able to select which commit to load into the app. This will let them not only watch older versions of the project, but also create new draft commits and work on them, while other users can simultaneously work on their own drafts.

This requires some changes on the code.

  • created commitContext to store in the app's localStorage the current selected commit. Only draft commits are editable, so another isEditable property is also exposed.

TODO

The currently selected commit should be scoped by project, not the whole client.

@csansoon csansoon added the 🚧 wip Work in progress label Jul 16, 2024
@csansoon csansoon force-pushed the current-commit branch 10 times, most recently from 08fcb09 to 4eaf332 Compare July 16, 2024 15:40
@csansoon csansoon force-pushed the current-commit branch 2 times, most recently from 51b0d3f to a13cb2a Compare July 17, 2024 15:19
@andresgutgon
Copy link
Contributor

store in the app's localStorage the current selected commit

Thinking about this we could store it in a cookie so the selection of the commit happens on the server

.orderBy(desc(commits.mergedAt))
.limit(1)

if (!result.length)
Copy link
Contributor

Choose a reason for hiding this comment

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

Use {} for the ifs when are more than one line please

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They were originally one-liners but prettier apparently didn't like it 😭

.select({ mergedAt: commits.mergedAt })
.from(commits)
.where(eq(commits.uuid, commitUuid))
.limit(1)
Copy link
Contributor

Choose a reason for hiding this comment

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

For the findOne, it is better to use findFirst. You don't need to do the limit(1)[0] thing.

I believe something like this works:

tx.query.commits.findFirst({
  with: { mergedAt: true },
  where: eq(commits.uuid, commitUuid)
})

projectCommitOrderIdx: index('project_commit_order_idx').on(
table.mergedAt,
table.projectId,
),
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this change the schema?

@csansoon csansoon force-pushed the current-commit branch 5 times, most recently from bed25b5 to 32eaec1 Compare July 18, 2024 07:54
@csansoon
Copy link
Contributor Author

☝️ Redistributed as multiple different PRs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🚧 wip Work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants