Refactor/extract changeproposal applier

[stevdrouet]

Extraction of change proposal applier from the backend into a shared package between frontend and backend so the preview when reviewing changes and result when applying changes are the same

[greptile-apps]

Greptile Summary

This PR extracts the change-proposal application logic from both the frontend (duplicated apply*Proposals.ts utilities) and the backend (*ChangeProposalsApplier classes) into a shared package in @packmind/types, ensuring the frontend preview and the backend apply operation use identical logic. The refactoring is well-structured: an AbstractChangeProposalApplier base class lives in @packmind/types, the three concrete appliers (Command, Skill, Standard) are co-located there, and the backend classes simply extend them to add persistence methods (getVersion / saveNewVersion).

Key changes and observations:

• Behavioral alignment: Fields such as updateSkillDescription, updateSkillPrompt, updateCommandDescription, and updateStandardDescription now use diff-based application on the frontend (previously they were direct replacements). This is intentional and correct — it makes conflict detection consistent between preview and apply.
• Conflict handling: All three frontend apply* functions now wrap the applier in a try/catch for ChangeProposalConflictError and return the original state on conflict. The rollback is all-or-nothing (see inline comment).
• Dead code removed: HighlightedContent.tsx, UnifiedMarkdownViewer.tsx, ProposalReviewPanel.tsx, ReviewDetailLayout.tsx, and the FieldChange / ChangeTracker / SkillChangeTracker types are all deleted — they tracked per-field change metadata that the new design no longer needs.
• uuid as a runtime dependency: uuid is added to packages/types/package.json dependencies but is only used in testHelpers.ts, which is not exported from index.ts (flagged in a previous thread and not yet addressed).
• Extra DB query in StandardChangesApplier.saveNewVersion: getRulesByStandardId is called even though the post-apply rules are already available in the incoming version argument (see inline comment).

Confidence Score: 4/5

• This PR is safe to merge; the refactoring is well-tested and the behavioral changes are intentional and correctly handled.
• The core extraction is sound, the shared appliers are covered by comprehensive spec files, and frontend conflict handling has been added. The two open items (all-or-nothing conflict rollback UX and one redundant DB query in StandardChangesApplier) are non-blocking style/performance concerns. The uuid devDependency issue carried over from a previous thread remains unaddressed but does not affect runtime correctness.
• packages/types/package.json (uuid in wrong dependency section), packages/playbook-change-management/src/application/useCases/applyChangeProposals/StandardChangesApplier.ts (redundant getRulesByStandardId call)

Important Files Changed

Filename	Overview
packages/types/src/playbookChangeManagement/applier/AbstractChangeProposalApplier.ts	New shared base class for all change proposal appliers; clean template-method pattern with DiffService injection and ChangeProposalConflictError on patch failure.
packages/types/src/playbookChangeManagement/applier/SkillChangeProposalApplier.ts	New shared skill applier; updateSkillDescription and updateSkillPrompt now use diff-based application instead of the former direct replacement used on the frontend — intentional parity with the backend.
packages/types/src/playbookChangeManagement/applier/StandardChangeProposalApplier.ts	New shared standard applier; updateStandardDescription is now diff-based; scalar fields (name, scope) and rule updates use direct replacement, consistent with the old backend applier.
packages/types/src/playbookChangeManagement/applier/CommandChangeProposalApplier.ts	New shared command applier; updateCommandDescription uses applyDiff consistently with backend; returns source unchanged for unrecognised types rather than throwing.
packages/types/src/playbookChangeManagement/applier/DiffService.ts	Direct lift of the existing DiffService from playbook-change-management into the shared types package; no logic change.
packages/types/src/playbookChangeManagement/applier/testHelpers.ts	Test-only factory utility; uuid is used here but listed as a runtime dependency in package.json rather than devDependencies.
apps/frontend/src/domain/change-proposals/utils/applySkillProposals.ts	Greatly simplified by delegating to SkillChangeProposalApplier; adds try/catch for ChangeProposalConflictError; all-or-nothing rollback when any proposal conflicts.
apps/frontend/src/domain/change-proposals/utils/applyStandardProposals.ts	Simplified to delegate to StandardChangeProposalApplier; conflict handling added; FieldChange/ChangeTracker types removed (no callers remain).
apps/frontend/src/domain/change-proposals/utils/applyRecipeProposals.ts	Simplified to delegate to CommandChangeProposalApplier; early return for empty proposals; conflict handling added.
packages/playbook-change-management/src/application/useCases/applyChangeProposals/CommandChangesApplier.ts	Backend applier now extends shared CommandChangeProposalApplier and adds persistence methods; two separate import blocks from @packmind/types (style issue, flagged in previous thread).
packages/playbook-change-management/src/application/useCases/applyChangeProposals/SkillChangesApplier.ts	Backend skill applier refactored to extend shared SkillChangeProposalApplier; persistence methods are clean.
packages/playbook-change-management/src/application/useCases/applyChangeProposals/StandardChangesApplier.ts	Backend standard applier refactored to extend shared StandardChangeProposalApplier; getRulesByStandardId is called twice (once in getVersion, once in saveNewVersion) — minor redundancy.
packages/types/package.json	uuid added to runtime dependencies but is only imported in testHelpers.ts (test utility, not exported); should be devDependencies (flagged in previous thread).
packages/types/src/playbookChangeManagement/applier/index.ts	Clean barrel export of all shared applier types; testHelpers.ts is correctly excluded from public exports.
packages/types/src/playbookChangeManagement/applier/types.ts	Defines SkillVersionWithFiles, ApplierObjectVersions, and the three change-type arrays; clean and straightforward.

Class Diagram

%%{init: {'theme': 'neutral'}}%%
classDiagram
    class AbstractChangeProposalApplier~Version~ {
        -DiffService diffService
        +applyChangeProposals(source, changeProposals) Version
        +areChangesApplicable(changeProposals) bool
        #checkChangesApplicable(changeProposals, supportedTypes) bool
        #applyChangeProposal(source, changeProposal) Version
        #applyDiff(changeProposalId, payload, sourceContent) string
    }

    class CommandChangeProposalApplier {
        +areChangesApplicable(changeProposals) bool
        #applyChangeProposal(source, changeProposal) RecipeVersion
    }

    class SkillChangeProposalApplier {
        +areChangesApplicable(changeProposals) bool
        #applyChangeProposal(source, changeProposal) SkillVersionWithFiles
    }

    class StandardChangeProposalApplier {
        +areChangesApplicable(changeProposals) bool
        #applyChangeProposal(source, changeProposal) StandardVersion
    }

    class CommandChangesApplier {
        -IRecipesPort recipesPort
        +getVersion(artefactId) RecipeVersion
        +saveNewVersion(version, ...) RecipeVersion
    }

    class SkillChangesApplier {
        -ISkillsPort skillsPort
        +getVersion(artefactId) SkillVersionWithFiles
        +saveNewVersion(skillVersion, ...) SkillVersionWithFiles
    }

    class StandardChangesApplier {
        -IStandardsPort standardsPort
        +getVersion(artefactId) StandardVersion
        +saveNewVersion(version, ...) StandardVersion
    }

    class DiffService {
        +applyLineDiff(oldValue, newValue, currentValue) DiffResult
        +hasConflict(oldValue, newValue, currentValue) bool
    }

    AbstractChangeProposalApplier <|-- CommandChangeProposalApplier : extends
    AbstractChangeProposalApplier <|-- SkillChangeProposalApplier : extends
    AbstractChangeProposalApplier <|-- StandardChangeProposalApplier : extends

    CommandChangeProposalApplier <|-- CommandChangesApplier : extends (backend)
    SkillChangeProposalApplier <|-- SkillChangesApplier : extends (backend)
    StandardChangeProposalApplier <|-- StandardChangesApplier : extends (backend)

    note for CommandChangeProposalApplier "Shared via @packmind/types"
    note for SkillChangeProposalApplier "Shared via @packmind/types"
    note for StandardChangeProposalApplier "Shared via @packmind/types"

    note for CommandChangesApplier "Backend only, adds persistence"
    note for SkillChangesApplier "Backend only, adds persistence"
    note for StandardChangesApplier "Backend only, adds persistence"

Loading

Comments Outside Diff (2)

1. apps/frontend/src/domain/change-proposals/utils/applySkillProposals.ts, line 51-74 (link)

   All-or-nothing conflict rollback discards all accepted proposals

   When ChangeProposalConflictError is caught, the catch block returns the complete original state — discarding any non-conflicting proposals that were already applied earlier in the reduce loop. For example, if a user has 5 accepted proposals and only the 3rd (a updateSkillPrompt diff) conflicts, the preview shows the original skill with none of the 5 changes visible.

   The same pattern applies to applyStandardProposals.ts and applyRecipeProposals.ts.

   This is a reasonable safety-net behavior (and consistent with how backend conflicts are surfaced), but it may be surprising in the UI: the user sees no preview at all rather than a partial one. Consider at minimum logging the conflict so it can be surfaced as a UI warning, e.g.:

   } catch (error) {
     if (error instanceof ChangeProposalConflictError) {
       console.warn('[applySkillProposals] conflict detected, reverting preview:', error.message);
       return { ... };
     }
     throw error;
   }

2. packages/playbook-change-management/src/application/useCases/applyChangeProposals/StandardChangesApplier.ts, line 40-78 (link)

   getRulesByStandardId called twice in saveNewVersion

   getRulesByStandardId(updatedStandard.id) is called here (line 71) to populate the returned StandardVersion.rules. The same call is already made in getVersion (line 33). If this method hits the database, saveNewVersion now incurs an extra query that wasn't present before — once to retrieve newVersion and once for the rules.

   Since saveNewVersion receives the new version's rules as part of the incoming version argument (they were computed by applyChangeProposals and passed through), consider returning them directly instead:

   return {
     ...newVersion,
     rules: version.rules,
   };

   This avoids the extra round-trip and preserves the rules that were just computed by the applier.

_{Last reviewed commit: ae07448}

[sonarqubecloud]

Quality Gate passed

Issues
9 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud