feat(refactor): merge note command

[hikchoi]

feat(refactor): merge note command

This PR:

• adds a new command Dendron: Merge Note
• enhances Lookup provider so that it can take a list of preAcceptValidators when it is created by the lookup provider factory.
  • this will allow lookup-derivative commands (such as this one) to pass in list of a predicate functions that would be executed when the user hits enter in the quickpick.
  • if any of the preAcceptValidator fails, lookup will not be accepted until the user selects a valid item from the quickpick
  • In merge note command, this is used to prevent users from merging the active note to itself.

Pull Request Checklist

Code

Basics

• [x] code should follow Code Conventions
• [x] circular dependency check: make sure your code is not introducing new circular dependencies in plugin-core. See Avoiding Circular Dependencies.
• [x] sticking to existing conventions instead of creating new ones
  • eg: if configuration for utilities are already in one module or package, add future utilities there as well

Extended

• General
  • [x] check whether code be simplified
  • [x] check if similar function already exist in the codebase. if so, can it be re-used?
  • [x] check if this change adversely impact performance
• Operations
  • [x] when shipping this change, will it just work or will it introduce additional operational overhead due to complicated interface or known bugs?
• Architecture
  • [x] check if code is introducing changes on a foundational class or interface. if so, call for design review if needed
    • eg: making changes to DNode

Instrumentation

Basics

• [x] if you are adding analytics related changes, make sure the Telemetry docs are updated

Extended

• [x] can we track the performance of this change to know if it is successful?
  • eg: see usage for export pod

Tests

Basics

• [x] Write Tests
• [x] Confirm existing tests pass
• [x] Confirm manual testing
• [x] Common cases tested
• [x] 1-2 Edge cases tested
• [~] If your tests changes an existing snapshot, snapshots have been updated

Extended

• [~] If you are adding a new language feature (graphically visible in VS Code/preview/publishing), an example is included in the test workspace
• CSS
  • [~] display is correct for following dimensions
    • [~] sm: screen ≥ 576px, eg. iphonex, (375x812)
    • [~] lg: screen ≥ 992px
    • [~] xxl: screen ≥ 1600px eg. mac (1600x900)
  • [~] display is correct for following browsers (across the various dimensions)
    • [~] safari
    • [~] firefox
    • [~] chrome

Docs

• [x] if your change reflects documentation changes, also submit a PR to dendron-site and mention the doc PR link in your current PR
• [ ] does this change introduce a new or better way of doing things that others need to be aware of? if so, an async should be created and a process added in Development or Packages

Close the Loop

Extended

• [~] is this a developer BREAKING change? if another person cloning from this branch will need to adjust their dependencies or mental model of the architecture, then it is. if this is the case, make sure this is communicated according to Close Loop
  • eg: breaking dev change due to new dependency

[hikchoi]

pending:

• [x] clean up command logic
• [x] add proxy metrics
• [x] add tests
• [x] confirm engine state is sound after the command
• [x] add documents

[hikchoi]

related to: https://github.com/dendronhq/dendron/issues/906

[hikchoi]

new proxy metrics props added to airtable ✅

[hikchoi]

Will create async on the new preAcceptValidator added to lookup provider when this PR is good to go.

[hikchoi]

doc updates at https://github.com/dendronhq/dendron-site/pull/609

[hikchoi]

Nice!

Maybe as a later enhancement - should we run this command automatically when a user does 'Move Note' and selects a destination that already exists? (Maybe first prompting them w/ "Would you like to merge these two notes?")

Good question. I did think about the UX / implementation of this as Move note and Merge note are pretty similar ideas on a surface level.

I think first we need to be careful about intent. When users try to use move note, and select an existing note for the destination, do they really want to do that? what are they expecting the end result to be? Or stepping back once more, does this actually happen frequently, and do we know if that action is intentional (they want / expect it to work) or is it a mistake?

Slightly tangential as I know you aren't suggesting we lump the implementation together with Move note, but I do want to break down the move note command and simplify it so that it does one thing. Currently it's taking care of rename note as a special case of move note, and as a result there are extra moving parts in it that could potentially break and make everything that depend on it not work if that happens.

So I prefer move note's capability to be simple and have it not serve as an actual entry / trigger point for another note-level refactoring capability. Though I do see the benefits of what you're suggesting.

My ideal way of doing this would be disallow and notify things that shouldn't happen in one command, and think about a way to give the users sufficient and clear signifiers that if their intention was different, that some other command can do it for them. So for example, instead of how move note just short-circuits when they pick an existing note in lookup, enhance it so that it uses a preAcceptValidator that disallows it in the first place, and show a toast that serve as a signifier that maybe you want to do this instead?, but not chain it there directly so that it becomes a trigger point for the Merge Note command.

[kevinslin]

looks like we have build errors

[hikchoi]

looks like we have build errors

@kevinslin sorry about that. this week's release had a breaking change with the engine (return signature changed for writeNote). Rebased / fixed now