Multiline import in diff view

[leomoty]

Intended outcome:

dialogx.jsx is clickable

Actual outcome:

dialogx.jsx is not clickable in diff view

How to reproduce the issue:

https://github.com/cockpit-project/cockpit/pull/9943/files#diff-a2edaced83836966f97b3d8df14beabcR24

Doesn't work on chrome (master) for me, works fine in Firefox.

It works correctly in blob (both browsers):

https://github.com/cockpit-project/cockpit/blob/7f47354e306dc5be08d235be9e3ae52c1f85d445/pkg/storaged/content-views.jsx#L25

---

Edit:

Old link doesn't work, see this one instead (split view is already pinned):

cockpit-project/cockpit@24ccf2e?diff=split#diff-fcaf1080ed0a2139de7cef9b184476a6L71

[stefanbuck]

Good spot 👍@leomoty do you want to look into this one?

A good starting point is to verify that the RegExp search made by findAndReplaceDOMText is working. The replace callback should be called with this match otherwise our advanced replace handler* can't do his job.

* Warning in advance: The advanced replace handler is the most complicated part in OctoLinker 😉

[leomoty]

Sure, will take a look this weekend. Any ideas why it works on firefox but not chrome?

[stefanbuck]

Not at all, sorry. That's really strange.

[leomoty]

So, I was able to narrow down the issue, it only fails on split view:

https://github.com/cockpit-project/cockpit/pull/9943/files?utf8=%E2%9C%93&diff=split#diff-a2edaced83836966f97b3d8df14beabcR24

It happens in both chrome and firefox.

[leomoty]

@stefanbuck I had a hunch.

When in split view, the HTML structure is as follow:

<tr>
    <td class="blob-num blob-num-deletion js-linkable-line-number" id="diff-a2edaced83836966f97b3d8df14beabcL24" data-line-number="24"></td>
    <td class="code-review blob-code blob-code-deletion ">
      <span class="blob-code-inner blob-code-marker-deletion"><span class="pl-k">var</span> <span class="pl-smi">utils</span><span class="pl-k"> =</span> <span class="pl-en">require</span>(<span class="pl-s"><span class="pl-pds">"</span>./utils.js<span class="pl-pds">"</span></span>);</span>
    </td>
    <td class="blob-num blob-num-addition js-linkable-line-number selected-line" id="diff-a2edaced83836966f97b3d8df14beabcR24" data-line-number="24"></td>
    <td class="code-review blob-code blob-code-addition selected-line">
      <span class="blob-code-inner blob-code-marker-addition">}<span class="pl-k"> from</span> <span class="pl-s"><span class="pl-pds">"</span>./dialogx.jsx<span class="pl-pds">"</span></span>;</span>
    </td>
</tr>

The regex is not really effective, because the import is across lines, which means it is splitten across multiple <tr>s and it has the other side worth of code as sibling (td).

So I wrote a little hack to test if i I was right:

const childList = Array.from(
    document.querySelectorAll('.js-blob-wrapper tr:not(.js-expandable-line)'),
  ).map(tr => tr.querySelector('td:nth-child(4)'));

  const rightBlob = document.createElement('div');

  childList.forEach(child => rightBlob.appendChild(child.cloneNode(true)));

This (rightBlob) is replacing blob.el in

OctoLinker/packages/helper-insert-link/index.js

Line 164 in 3da6100

findAndReplaceDOMText(blob.el, {

I was able to get a match:

Obviously this won't convert anything into octolinks.

[leomoty]

Tried running this by hand: https://github.com/padolsey/findAndReplaceDOMText/blob/master/src/findAndReplaceDOMText.js#L262

Unless I am mistaken, the result is what I expected:

     ...
      import cockpit from "cockpit";
      import {
      var cockpit = require("cockpit");
          dialog_open, TextInput, PassInput, SelectOne, SizeSlider,
      var dialog = require("./dialog");
          BlockingMessage, TeardownMessage
      var utils = require("./utils.js");
      } from "./dialogx.jsx";
     ...

[stefanbuck]

Thanks @leomoty, that's a really well researched report! Off the top of my head, I have no idea how to solve this best. Let me think about it. @josephfrazier do you have any smart idea?

[leomoty]

Ya me neither, unless we somehow start treating each side in split diff view as a different blob. Right now, that approach would break and it requires more than just text replacing.

[leomoty]

It seems that the multiline import does no longer exist in that PR. I'll try to find another one.

Edit:

cockpit-project/cockpit@24ccf2e?diff=split#diff-fcaf1080ed0a2139de7cef9b184476a6L71