refactor!: move base extension contracts to abstractions#1983
Open
yuto-trd wants to merge 2 commits into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughAdds a new ChangesExtensibility abstractions split
Sequence Diagram(s)sequenceDiagram
participant PackageManager
participant Extension
participant ExtensionSettings
participant ConditionalWeakTable
PackageManager->>ConditionalWeakTable: remove prior subscription for Extension
PackageManager->>ExtensionSettings: subscribe ConfigurationChanged
ExtensionSettings->>PackageManager: raise ConfigurationChanged
PackageManager->>ExtensionSettings: save settings
PackageManager->>ConditionalWeakTable: store SettingsSubscription
PackageManager->>ConditionalWeakTable: remove subscription on cleanup
PackageManager->>ExtensionSettings: unsubscribe stored handler
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Extract the root extension contracts into Beutl.Extensibility.Abstractions so plugin authors can depend on the lightweight base surface without pulling the full UI/media extension assembly. Keep UI, media, property-editor, and built-in primitive extension contracts in Beutl.Extensibility. Move extension settings persistence handler state out of the Extension contract and into PackageManager, tracked in a ConditionalWeakTable so a handler entry can never outlive its extension (and its collectible AssemblyLoadContext). Teach package metadata parsing to recognize Beutl.Extensibility.Abstractions dependencies. Publish Beutl.Extensibility.Abstractions as its own package and expose it through Beutl.Extensibility.Sdk via the BeutlAutoReferenceExtensibilityAbstractions opt-in, so an author can drop the heavy assembly and reference only the thin contracts. Cover the handler lifecycle, the dependency-id fallback precedence, and the abstractions dependency-closure thinness invariant with NUnit tests. Refs: Project #9 "AI Review" item "設計改善: 拡張 API の境界を薄くする" BREAKING CHANGE: Extension, ExtensionSettings, and ExportAttribute moved from the Beutl.Extensibility assembly to Beutl.Extensibility.Abstractions while keeping the Beutl.Extensibility namespace. Downstream plugins that reference these root contracts must reference the new Beutl.Extensibility.Abstractions package/assembly or rebuild against packages that transitively include it.
yuto-trd
force-pushed
the
yuto-trd/thin-extensibility-boundary
branch
from
e01ed9f to
ed906be
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors the plugin API boundary by extracting the core extension entry-point contracts (Extension, ExtensionSettings, ExportAttribute) into a new lightweight Beutl.Extensibility.Abstractions project/assembly while keeping the Beutl.Extensibility namespace, and adjusts package/runtime plumbing to recognize the new assembly.
Changes:
- Added
Beutl.Extensibility.Abstractionsproject and tests to enforce dependency thinness and contract placement. - Moved extension-settings change-subscription bookkeeping into
PackageManagerand added lifecycle tests to prevent leaked subscriptions. - Updated local package target-version detection, runtime dependency enumeration, SDK auto-references, and build/solution registration to include the new abstractions package.
Reviewed changes
Copilot reviewed 15 out of 17 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/Beutl.UnitTests/Api/PackageManagerExtensionLifecycleTests.cs | Adds tests for settings subscription setup/repeat setup/unload cleanup. |
| tests/Beutl.UnitTests/Api/LocalPackageTests.cs | Adds tests for target version selection precedence including the new abstractions dependency. |
| tests/Beutl.Extensibility.Abstractions.Tests/ExtensibilityAbstractionsAssemblyTests.cs | Verifies base contracts live in the abstractions assembly and enforces a “thin” reference closure. |
| tests/Beutl.Extensibility.Abstractions.Tests/Beutl.Extensibility.Abstractions.Tests.csproj | Adds a new test project targeting net10.0 and referencing the abstractions project. |
| src/Beutl.Extensibility/Beutl.Extensibility.csproj | Layers the full extensibility project on top of the new abstractions project. |
| src/Beutl.Extensibility.Abstractions/ExtensionSettings.cs | Introduces ExtensionSettings in the abstractions assembly (still in Beutl.Extensibility namespace). |
| src/Beutl.Extensibility.Abstractions/Extension.cs | Moves Extension into abstractions and removes settings-handler bookkeeping from the contract. |
| src/Beutl.Extensibility.Abstractions/ExportAttribute.cs | Introduces ExportAttribute in the abstractions assembly. |
| src/Beutl.Extensibility.Abstractions/Beutl.Extensibility.Abstractions.csproj | Defines the new abstractions project and its minimal project references. |
| src/Beutl.Api/Services/PackageManager.cs | Tracks settings-change handlers in PackageManager (instead of on Extension) to support clean unloads. |
| src/Beutl.Api/Services/LocalPackage.cs | Recognizes Beutl.Extensibility.Abstractions when determining TargetVersion. |
| src/Beutl.Api/Services/CoreLibraries.cs | Includes Beutl.Extensibility.Abstractions in runtime dependency collection. |
| sdk/README.md | Documents the new auto-reference option for abstractions-only plugins and usage example. |
| sdk/Beutl.Extensibility.Sdk/Sdk/Sdk.targets | Adds BeutlAutoReferenceExtensibilityAbstractions and conditionally emits the abstractions package reference. |
| nukebuild/Build.cs | Ensures the new abstractions package is considered in build packaging/version flows. |
| Beutl.slnx | Adds the new abstractions project and its test project to the solution. |
| AGENTS.md | Updates the module boundary map to describe the new abstractions layer and its intended dependency profile. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/Beutl.Api/Services/CoreLibraries.cs (1)
78-78: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a direct test for this runtime-map entry.
IncludedInRuntimeDependenciesfeeds both plugin loading and package-install resolution, but the changed test set never exercises the newBeutl.Extensibility.Abstractionsbranch. Please pin this with a small NUnit test intests/Beutl.UnitTests/so the boundary cannot regress silently. Based on learnings,New logic must include a NUnit test in the matching tests/ project (for example, tests/Beutl.UnitTests/ for general unit tests or tests/SourceGeneratorTest/ for generator changes).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Beutl.Api/Services/CoreLibraries.cs` at line 78, Add a direct NUnit test in tests/Beutl.UnitTests that exercises the IncludedInRuntimeDependencies runtime-map branch for Beutl.Extensibility.Abstractions. Use the CoreLibraries entry point and the IncludedInRuntimeDependencies method to verify this package is treated as included, so the new case is covered explicitly and cannot regress unnoticed.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Beutl.Api/Services/LocalPackage.cs`:
- Around line 41-43: The package lookup in LocalPackage’s dependency selection
uses exact string comparisons, so it can miss valid NuGet dependencies when
casing differs. Update the PackageDependency search in the sdkDep assignment to
compare package IDs case-insensitively for Beutl.Sdk, Beutl.Extensibility, and
Beutl.Extensibility.Abstractions, and keep the existing fallback order so
TargetVersion can still be resolved.
In `@src/Beutl.Api/Services/PackageManager.cs`:
- Around line 30-33: The subscription bookkeeping in PackageManager is using
only the Extension as the key, but CleanupExtensionSettings must unsubscribe
from the exact ExtensionSettings instance that SetupExtensionSettings subscribed
to. Update the handler tracking around SetupExtensionSettings and
CleanupExtensionSettings to store both the subscribed ExtensionSettings
publisher and its ConfigurationChanged delegate per extension, then remove the
delegate from that stored publisher during cleanup even if extension.Settings
has changed or been cleared. Keep the fix localized to the PackageManager
subscription/unsubscription flow and the related handler map.
In
`@tests/Beutl.Extensibility.Abstractions.Tests/ExtensibilityAbstractionsAssemblyTests.cs`:
- Around line 7-8: Add the NUnit [TestFixture] attribute to the
ExtensibilityAbstractionsAssemblyTests class so it matches the repo’s test
convention; update the class declaration in
ExtensibilityAbstractionsAssemblyTests and keep the existing [Test] methods
unchanged.
In `@tests/Beutl.UnitTests/Api/LocalPackageTests.cs`:
- Around line 7-8: Add the NUnit [TestFixture] attribute to the
LocalPackageTests class so it is recognized as a test fixture and matches the
repo’s NUnit conventions; keep the existing [Test] methods unchanged and place
the attribute directly on the class declaration.
---
Nitpick comments:
In `@src/Beutl.Api/Services/CoreLibraries.cs`:
- Line 78: Add a direct NUnit test in tests/Beutl.UnitTests that exercises the
IncludedInRuntimeDependencies runtime-map branch for
Beutl.Extensibility.Abstractions. Use the CoreLibraries entry point and the
IncludedInRuntimeDependencies method to verify this package is treated as
included, so the new case is covered explicitly and cannot regress unnoticed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bab12559-91fb-4a32-971a-757969b17c41
📒 Files selected for processing (17)
AGENTS.mdBeutl.slnxnukebuild/Build.cssdk/Beutl.Extensibility.Sdk/Sdk/Sdk.targetssdk/README.mdsrc/Beutl.Api/Services/CoreLibraries.cssrc/Beutl.Api/Services/LocalPackage.cssrc/Beutl.Api/Services/PackageManager.cssrc/Beutl.Extensibility.Abstractions/Beutl.Extensibility.Abstractions.csprojsrc/Beutl.Extensibility.Abstractions/ExportAttribute.cssrc/Beutl.Extensibility.Abstractions/Extension.cssrc/Beutl.Extensibility.Abstractions/ExtensionSettings.cssrc/Beutl.Extensibility/Beutl.Extensibility.csprojtests/Beutl.Extensibility.Abstractions.Tests/Beutl.Extensibility.Abstractions.Tests.csprojtests/Beutl.Extensibility.Abstractions.Tests/ExtensibilityAbstractionsAssemblyTests.cstests/Beutl.UnitTests/Api/LocalPackageTests.cstests/Beutl.UnitTests/Api/PackageManagerExtensionLifecycleTests.cs
![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: ed906beba1
ℹ️ 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".
- PackageManager: capture the subscribed ExtensionSettings publisher alongside the handler and unsubscribe before Restore, so a repeated SetupExtensionSettings cannot Save mid-restore and cleanup unsubscribes from the exact publisher even if Settings was swapped or cleared (keeps the collectible AssemblyLoadContext collectible). - LocalPackage: compare Beutl dependency ids case-insensitively, since NuGet package ids are case-insensitive. - Beutl.Extensibility.Sdk: in the prevent-copy block reference Beutl.Extensibility.Abstractions directly whenever it is in the closure so its runtime asset is excluded from sideload output; document the all-off + abstractions-on path for a truly thin plugin (ProjectSystem / NodeGraph / Editor otherwise pull Beutl.Extensibility back transitively). - Add [TestFixture] to the new NUnit fixtures.
Contributor
|
No TODO comments were found. |
Contributor
Minimum allowed line rate is |
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.
Description
Extension,ExtensionSettings, andExportAttributeinto the newBeutl.Extensibility.Abstractionsproject while preserving theBeutl.Extensibilitynamespace.Beutl.Extensibility;Beutl.Extensibilitynow references the abstraction project.PackageManagerso host persistence state is not part of the root extension contract.Beutl.Extensibility.Abstractionsdependencies when reading local package target versions.Affected areas
Beutl.Engine(rendering / scene / track)Beutl.ProjectSystem(project / document persistence)Beutl.Editor,Beutl.Editor.Components,Beutl.Controls)Beutl.Extensibility(plugin abstractions)Beutl.NodeGraph(node editor)Beutl.FFmpegIpc/Beutl.FFmpegWorker(media IPC boundary)Beutl.Api(server API client)Breaking changes
Extension,ExtensionSettings, andExportAttributemoved from theBeutl.Extensibilityassembly toBeutl.Extensibility.Abstractionswhile keeping theBeutl.Extensibilitynamespace. Downstream plugins that reference these root contracts must reference the new abstraction package/assembly or rebuild against packages that transitively include it.Test plan
dotnet test tests\Beutl.Extensibility.Abstractions.Tests\Beutl.Extensibility.Abstractions.Tests.csproj -f net10.0 --no-restore— passed, 3 tests.dotnet test tests\Beutl.UnitTests\Beutl.UnitTests.csproj -f net10.0 --no-build --filter "FullyQualifiedName~Beutl.UnitTests.Api.LocalPackageTests"— passed, 3 tests.dotnet build Beutl.slnx --no-restore— passed with existing nullable/NU190x warnings.dotnet format Beutl.slnx --include <changed files> --verify-no-changes— passed.dotnet test tests\Beutl.UnitTests\Beutl.UnitTests.csproj -f net10.0 --no-buildwas run and failed in 3AutoSaveServiceTests(SaveError_Subscription_ShouldReceiveErrors,SaveObjects_WhenExceptionOccurs_ShouldContinueWithNextObject,SaveObjects_WithNonExistentPath_ShouldEmitSaveError). The same 3-test filter also fails on unchangedmain, so this is treated as a pre-existing local baseline failure rather than a regression from this PR.Fixed issues / References
Summary by CodeRabbit
Beutl.Extensibility.Abstractionsas a lightweight abstractions-only extensibility package, included in the solution/build and supported by SDK auto-referencing with a dedicated opt-out property.