Skip to content
Snippets Groups Projects

Simplify ViewResolver type

Closed Administrator requested to merge simplify-view-resolver into github-snippet-resolution

Created by: lguychard

A ViewResolver is now simply a function that takes a container as input and returns Iterable<View>, instead of being defined with a selector and a resolveView() function.

Merge request reports

Approval is optional

Closed by avatar (Jul 8, 2025 10:23am UTC)

Merge details

  • The changes were not merged into github-snippet-resolution.

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Created by: lguychard

    When we wrote trackViews(), we opted not to run the resolvers on removed nodes, to guard against cases where resolution depends on state outside the code view, and the needed state would be inaccessible because the node has been removed from the tree. It wasn't a bug we actually encountered, merely something that was raised during code review, and then accounted for, but it is definitely possible. The result would have been leaked code views. Reference: !2909 (merged)

    The issue you raised in !3920 (merged) is a different bug, that leads to systematically leaked code views: if the resolver returns a different element than the selector, the code view will be leaked when removed from the DOM, because the resolver is not run on removed nodes.

    This PR does reintroduce the first bug, in order to fix the second.

    I do think that looking at the context of an element initially matched with a selector in order to return the correct code view element is a valid use case. On github.com, for instance, we can't come up with an unambiguous enough selector to match the whole code view on blob views (example), and the issue was the same for snippets.

    One way to keep this possible while avoiding both bugs would be to simply check whether removed nodes are tracked views, or contain tracked views within their subtree (without running selectors or resolvers on removed nodes).

  • Created by: felixfbecker

    That would be less performant though because we would have to iterate over all registered code views instead of doing a O(1) Map lookup.

    Anything that speaks against my suggestion?

  • Created by: lguychard

    Your suggestion is still potentially vulnerable to !2909 (merged) if findElements() does something akin to this code, isn't it?

  • Created by: felixfbecker

    Hmm yeah I think you're right. Sounds like we need to iterate.

  • Created by: felixfbecker

    FYI I think that refactor would conflict with native diff views

  • Created by: felixfbecker

    Do you want to get this in for 3.5?

  • Created by: lguychard

    If time, but it's really not the most critical thing now that #4371 is merged. I'll move this to the backlog milestone to reflect this, and rebase when I get the chance.

  • Created by: slimsag

    This PR has been open for a very long time with no progress, closing. Reopen if desired.

Please register or sign in to reply
Loading