adds discussions to lix

[martin-lysk]

This adds discussions and comments to lix

Notice this is WIP to see what we should add

[changeset-bot]

⚠️ No Changeset found

Latest commit: cf9032c

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[samuelstroschein]

From Discord https://discord.com/channels/897438559458430986/1283682858489221161/1283785662373429410

I didn't touch merging of comments yet since the merging model will change with branching - right?

Branches don't seem to influence comments. Remember the definition of a branch "A branch is a filter for changes that are currently applied". The definition is not a completely isolated state. From a user perspective, any comment on a change seems to be good if it's available in another branch. That avoids conflicts + unnecessary switching of branches. If I am in branch x and you are in branch y, we can still discuss a change that is shown in both of our branches.

Merging two lix'es would be copying the comments from one to another (like we do with commits atm).

Proposal

1. Add copying the change discussions in merge from source to target with an onConflict do nothing. Similar to copying the commits over

   monorepo/lix/packages/sdk/src/merge/merge.ts

   Lines 106 to 114 in bd822e0

   	// 2. copy the commits from source
   	if (sourceCommits.length > 0) {
   	await trx
   	.insertInto("commit")
   	.values(sourceCommits)
   	// ignore if already exists
   	.onConflict((oc) => oc.doNothing())
   	.execute();
   	}

   .

2. Ignore conflicts (two changes have the same parent_id) for now. We can work on discussion conflicts for the release candidate https://linear.app/opral/issue/LIXDK-160/model-conflicts-in-change-discussions

PS it seems like "merging two branches" is different from "merging to lixes". Merging a branch does not merge data because the data of changes, comments, etc. is available in all branches. cc @janfjohannes cautious when implementing branches. it might be a good idea not to touch the merge function and create a new mergeBranch function instead. Afterward, we can evaluate if finding a better word for merging branches is appropriate (given that no data is merged)

I added only wrting functions "startDiscussion" and "comment" to lix. -hope this matches your idea of a minimal API

Yes 👍

What would you like to see tested to close this pr?

Add a testing for merging two lixes. The discussions should be copied over. And I would appreciate rename comment etc to changeDiscussionComment. Then ready to go! 🚀

[martin-lysk]

@samuelstroschein I Agree, that we can merge the changes without branches and use the same approach of copying over the changes.

Branches don't seem to influence comments. Remember the definition of a branch "A branch is a filter for changes that are currently applied". The definition is not a completely isolated state. From a user perspective, any comment on a change seems to be good if it's available in another branch. That avoids conflicts + unnecessary switching of branches. If I am in branch x and you are in branch y, we can still discuss a change that is shown in both of our branches.

Lets say you have two branches main and start-day-updated and you model company spend on salary:

state in main

Employee	Yearly Salary	Start Day	Cost 2024
Max Mustermann	70k €	1. April	58.3k €

state in start-day-updated

Employee	Yearly Salary	Start Day	Cost 2024
Max Mustermann	70k €	1. March	58.3k €

A view on the change table with a join on the branch table would contain something like this:

change id	parent change	value.id	value.value	branches
1	null	a0	Max Musterman	main, start-day-updated
2	null	b0	70k €	main, start-day-updated
3	null	c0	1. April	main, start-day-updated
4	null	d0	58.3k€	main, start-day-updated
5	3	c0	1. March	start-day-updated

Now someone points to the fact that the column "Cost 2024" was not updated instart-day-updated.
One could start a discussion on different entities:

1. Only on the column "Cost 2024" which contains the error

A discussion with the discussion_change_map and comment table joined would look like this:

discussion	changes	comments
1	4	"Cost have not been updated after changing the start date!"

2. The Whole line (pointing to all leaf changes in the line of Max Mustermann"

A discussion with the discussion_change_map and comment table joined would look like this:

discussion	changes	comments
1	1,2,4,5	"Cost have not been updated after changing the start date!"

3. Only on the column that is wrong "Cost 2024" and the one that produced the error "Start Day"

A discussion with the discussion_change_map and comment table joined would look like this:

discussion	changes	comments
1	4 5	"Cost have not been updated after changing the start date!"

We would need to have a reference to branches because this comment makes no sense for main. While one could argue that we could filter comments for a branch that only reference changes available in the current branch - case 1. would be visible anyway.

cc @janfjohannes @NilsJacobsen

[samuelstroschein]

sync call with @martin-lysk and @janfjohannes :

• yes, discussions might be tied to a branch (sequence id)
• we should be able to merge this pr to unblock apps and later implement tying to branches
• conflicts don't need to be modeled because we use the parent_id and created_at information for sorting and in the UI we would show a "this comment is a reply to"

[samuelstroschein]

@martin-lysk github notified me that this pr is ready for review but i don't see an explicit prompt from you. lmk if i should review this pr

[martin-lysk]

yes @samuelstroschein , i worked in the signature part. Plz review

[samuelstroschein]

Yep, that's what i had in mind. requesting two changes before approval (rest of the comments is minor and can be ignored or implemented):

• change comment to addComment
• change lix.comment(args: { parentChange: { id: string }}) to lix.comment(args: { parentChangeId: string})

[martin-lysk]

Worked in you feedback - feel free to merge if it looks good to you @samuelstroschein