Skip to content

Conversation

@Annaseli
Copy link
Contributor

@Annaseli Annaseli commented Dec 11, 2025

Closes #9781

Change Description

This PR adds the plugin to implement the WebUI client logic for using the async commit and merge functionality in Enterprise.

Testing

Added a Playwright test.

The Enterprise PR that implements the functionality is here.

@Annaseli Annaseli requested a review from a team December 11, 2025 19:01
@github-actions github-actions bot added area/testing Improvements or additions to tests area/UI Improvements or additions to UI labels Dec 11, 2025
@Annaseli Annaseli added the include-changelog PR description should be included in next release changelog label Dec 11, 2025
@Annaseli Annaseli changed the title OSS WebUI Support for Async Commit/Merge WebUI in Enterprise OSS WebUI Support for Async Commit/Merge WebUI implementation Dec 11, 2025
@Annaseli Annaseli requested a review from nopcoder December 11, 2025 19:05
Copy link
Contributor

@itaigilo itaigilo left a comment

Choose a reason for hiding this comment

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

Nice and clean plugin implementation!

I think that the plugin should be split (see comment in PluginCommitMergeStrategy) though.

await refsAPI.merge(repo.id, source, dest, strategy, message, metadata);
await pluginManager.commitMergeStrategy.merge(repo.id, source, dest, {
message: message || undefined,
metadata: Object.keys(metadata).length > 0 ? metadata : undefined,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's much better to handle this in the implementation itself (and avoid checking this here).
Same goes for message.

Comment on lines 95 to 97
force: false,
allow_empty: false,
squash_merge: false
Copy link
Contributor

Choose a reason for hiding this comment

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

Since these are always false, I think they should better be removed.

reference: string;
}

export interface PluginCommitMergeStrategy {
Copy link
Contributor

Choose a reason for hiding this comment

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

In the OSS world, commit and merge are not related -
Other than the fact that they both have an async option.
Hence, let's split this into a commit plugin and a merge plugin -
It will require very little extra code.

In addition, the terminology isn't right:
The plugin doesn't provide the "strategy" (for some other component to use), but an actual implementation.

There might be better naming suggestions than the following, but I suggest:

export interface PluginCommitAction {
  run(): Promise<CommitResult>;
}

export interface PluginMergeAction {
  run(): Promise<MergeResult>;
}

@nopcoder nopcoder removed their request for review December 15, 2025 09:20
@Annaseli Annaseli requested a review from itaigilo December 23, 2025 16:07
Copy link
Contributor

@itaigilo itaigilo left a comment

Choose a reason for hiding this comment

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

Looking good, nicely done!

Only Esti fixes, and you should be good to go.

@Annaseli
Copy link
Contributor Author

@itaigilo
Can you review only the last commit? Added params docs for ts purpose (in a js index file can't declare the params types and without it I have ts errors)

@Annaseli Annaseli requested a review from itaigilo December 23, 2025 19:41
@itaigilo
Copy link
Contributor

@itaigilo Can you review only the last commit? Added params docs for ts purpose (in a js index file can't declare the params types and without it I have ts errors)

Talked on Slack, all good on my side.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/testing Improvements or additions to tests area/UI Improvements or additions to UI include-changelog PR description should be included in next release changelog

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Async commit and merge: WebUI in OSS

3 participants