Skip to content

feat(calm-models,calm-shared,calm-hub-ui): Set up ADR Diff Engine#2500

Open
aamanrebello wants to merge 6 commits into
finos:mainfrom
aamanrebello:adr-diff
Open

feat(calm-models,calm-shared,calm-hub-ui): Set up ADR Diff Engine#2500
aamanrebello wants to merge 6 commits into
finos:mainfrom
aamanrebello:adr-diff

Conversation

@aamanrebello
Copy link
Copy Markdown
Contributor

@aamanrebello aamanrebello commented May 24, 2026

Description

Partially addresses #2526 - "As a developer, I want to run calm adr-diff between two architectures so that I can identify what ADR links have been added or removed."

We will make changes to the CLI/Calm Hub UI after consultations, but for now I am just adding ADR diff capability and typing to CALM models. Types have been updated in all affected modules.

New dependencies are introduced into the monorepo - diff and @types/diff

Type of Change

  • 🐛 Bug fix (non-breaking change which fixes an issue)
  • ✨ New feature (non-breaking change which adds functionality)
  • 💥 Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • 📚 Documentation update
  • 🎨 Code style/formatting changes
  • ♻️ Refactoring (no functional changes)
  • ⚡ Performance improvements
  • ✅ Test additions or updates
  • 🔧 Chore (maintenance, dependencies, CI, etc.)

Affected Components

  • CLI (cli/)
  • Schema (calm/)
  • CALM AI (calm-ai/)
  • CALM Hub (calm-hub/)
  • CALM Hub UI (calm-hub-ui/)
  • CALM Server (calm-server/)
  • CALM Widgets (calm-widgets/)
  • Documentation (docs/)
  • Shared (shared/)
  • VS Code Extension (calm-plugins/vscode/)
  • Dependencies
  • CI/CD

Commit Message Format ✅

Testing

  • I have tested my changes locally
  • I have added/updated unit tests
  • All existing tests pass

Checklist

  • My commits follow the conventional commit format
  • I have updated documentation if necessary
  • I have added tests for my changes (if applicable)
  • My changes follow the project's coding standards

@github-actions github-actions Bot added config calm-hub-ui Affects `calm-hub-ui` labels May 24, 2026
@aamanrebello aamanrebello force-pushed the adr-diff branch 2 times, most recently from fc27cd0 to ef8abd3 Compare May 26, 2026 23:01
@github-actions github-actions Bot added cli Affects `cli` code shared labels May 26, 2026
@aamanrebello aamanrebello changed the title feat(calm-models,calm-hub-ui): ADR Diffs feat(calm-models,calm-shared,calm-hub-ui): ADR Diffs May 27, 2026
@github-actions github-actions Bot added the docs Improvements of additions to documentation label May 27, 2026
@aamanrebello aamanrebello marked this pull request as ready for review May 27, 2026 13:11
@aamanrebello aamanrebello marked this pull request as draft May 27, 2026 13:12
@aamanrebello aamanrebello marked this pull request as ready for review May 28, 2026 17:12
@rocketstack-matt
Copy link
Copy Markdown
Member

Thanks for the PR @aamanrebello but I think we have a bit of a sequencing issue here.

As things stand, ADRs on architectures live outside the timeline, e.g.
Screenshot 2026-06-01 at 15 10 35

It's not entirely obvious, but hopefully you can see above that the ADRs at a given point in time are in a separate 'metadata' tab above the timeline.

As your PR shows we now have ADR changes showing in the timeline, but I would still need to use the metadata tab to access them - which in the case of the comparison view is not even visible, so I would now need to navigate to a specific version to access the ADR.

This brings us on to what I would consider the biggest issue, that ADRs I don't believe are renderable at all, certainly for me clicking one via either the metadata tab or from the ADRs item in the explorer view results in an entirely blank page.

Personally, I think we need to work through the flow and design of how we would like ADRs to be presented and agree that rendering ADRs is a priority before we move on to raising a PR for them. Indeed, as a group I think we need to get better at working on items that have been prioritized on the roadmap as I think a lot of us are getting excited about a feature, raising a PR and then working on it before as a group we've agreed it is a high priority.

Looking at ADRs standalone, I actually think that a different type of interface could work better, certainly from the explorer view I think perhaps a card style view would be better, not so sure on the architecture piece.

I hope I'm not sounding too negative, I think it would be worth bringing the issue to Office Hours, ideally with some proposal for how this could work in a target state and then a number of iterative PRs towards that would absolutely make sense.

@rocketstack-matt
Copy link
Copy Markdown
Member

While exercising the ADR/timeline functionality around this PR locally, I hit a separate, pre-existing bug in the CALM Hub UI worth flagging here for context: the ADR detail page renders a completely blank page for any ADR without a populated decisionOutcome — i.e. every draft ADR created via the REST API.

Root cause is an unguarded decisionOutcome.chosenOption dereference in DisplayChosenOption, with no error boundary around AdrRenderer, so the TypeError unmounts the whole view. It is not introduced by this PR — it dates back to the original ADR detail page (Issue #1222) — but it surfaced while testing ADRs in support of this work.

Logged as #2548 and fixed in #2549 (UI-only guard + regression tests).

@aamanrebello
Copy link
Copy Markdown
Contributor Author

While exercising the ADR/timeline functionality around this PR locally, I hit a separate, pre-existing bug in the CALM Hub UI worth flagging here for context: the ADR detail page renders a completely blank page for any ADR without a populated decisionOutcome — i.e. every draft ADR created via the REST API.

Root cause is an unguarded decisionOutcome.chosenOption dereference in DisplayChosenOption, with no error boundary around AdrRenderer, so the TypeError unmounts the whole view. It is not introduced by this PR — it dates back to the original ADR detail page (Issue #1222) — but it surfaced while testing ADRs in support of this work.

Logged as #2548 and fixed in #2549 (UI-only guard + regression tests).

Yes, an ADR renderer does exist, it's good that the bug was identified. Will take a look at the PR as soon as I can.

@aamanrebello
Copy link
Copy Markdown
Contributor Author

aamanrebello commented Jun 1, 2026

Thanks for the PR @aamanrebello but I think we have a bit of a sequencing issue here.

As things stand, ADRs on architectures live outside the timeline, e.g. Screenshot 2026-06-01 at 15 10 35

It's not entirely obvious, but hopefully you can see above that the ADRs at a given point in time are in a separate 'metadata' tab above the timeline.

As your PR shows we now have ADR changes showing in the timeline, but I would still need to use the metadata tab to access them - which in the case of the comparison view is not even visible, so I would now need to navigate to a specific version to access the ADR.

This brings us on to what I would consider the biggest issue, that ADRs I don't believe are renderable at all, certainly for me clicking one via either the metadata tab or from the ADRs item in the explorer view results in an entirely blank page.

Personally, I think we need to work through the flow and design of how we would like ADRs to be presented and agree that rendering ADRs is a priority before we move on to raising a PR for them. Indeed, as a group I think we need to get better at working on items that have been prioritized on the roadmap as I think a lot of us are getting excited about a feature, raising a PR and then working on it before as a group we've agreed it is a high priority.

Looking at ADRs standalone, I actually think that a different type of interface could work better, certainly from the explorer view I think perhaps a card style view would be better, not so sure on the architecture piece.

I hope I'm not sounding too negative, I think it would be worth bringing the issue to Office Hours, ideally with some proposal for how this could work in a target state and then a number of iterative PRs towards that would absolutely make sense.

To my understanding, the presentation of the ADR diff within the CalmHub UI is the issue. Fair enough, I was just trying to follow the format of your previous PR's where you made changes to the CALM models, CALM CLI and Calm Hub UI in a single PR. To be honest, the UI part of this PR isn't as important, I will get rid of those commits (LIFO). What's more important is the changes I have made in the CALM Models and CALM CLI.

I think if we have a CLI command/function to diff two CALM architectures, we cannot just stop at nodes and edges. The diff must extend to other parts of the schema as well i.e. ADRs, controls, metadata etc. That, in my opinion, is the true meaning of diffing two CALM files. Since the ADRs are string arrays, enabling diffing of these in the CLI is an easy low-hanging fruit (you can see from the code how simple it is) - and it is useful to have the functionality available.

So my conclusion is that I will roll back changes to the UI and ask for reviews only on my changes to CALM models and CALM CLI. Related changes to Calm Hub UI can be tackled later based on our priorities/decisions.

I am about to track this in #2526

@aamanrebello aamanrebello mentioned this pull request Jun 1, 2026
6 tasks
@aamanrebello aamanrebello marked this pull request as draft June 2, 2026 08:59
@rocketstack-matt
Copy link
Copy Markdown
Member

rocketstack-matt commented Jun 2, 2026

Thanks @aamanrebello to be clear, not asking you to roll anything back on this, more that I think we need to do two things as a community (not specific to you, I'm just as guilty, as are others).

  1. Raise issues and request feedback ahead of implementation (unless the PR is a POC to show the potential, not looking to stifle creativity). in this specific case I think that would have gotten us to look more at how the ADRs are currently presented, which seems a bit out of sync with the rest of things - that is in large part my fault because I implemented the timeline feature without ever looking at ADRs.
  2. Ensure we're working on things prioritized on the roadmap . . . obviously the roadmap should and will evolve, we don't look at it very often, but a whole bunch of things we decided were important we haven't looked at and we all keep raising new features not on it.

I have put this discussion on Office Hours.

@aamanrebello aamanrebello changed the title feat(calm-models,calm-shared,calm-hub-ui): ADR Diffs feat(calm-models,calm-shared,calm-hub-ui): Set up ADR Diff Engine Jun 6, 2026
@aamanrebello aamanrebello marked this pull request as ready for review June 6, 2026 14:07
@aamanrebello
Copy link
Copy Markdown
Contributor Author

aamanrebello commented Jun 6, 2026

What I have decided is to not directly affect any of the tooling until we discuss it at office hours. I am just adding the capability into calm-models and updating affected downstreams. This incorporates nice-to-have functionality without affecting any tooling.

Until we make the decisions, I have updated DiffResult to NodesAndRelationshipsDiffResult (only scoped to nodes and relationships) in downstreams - eventually we expect to extend the types to incorporate ADRs and everything else.

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

Labels

calm-hub-ui Affects `calm-hub-ui` cli Affects `cli` code config docs Improvements of additions to documentation shared

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants