@recaptime-dev's working patches + fork for Phorge, a community fork of Phabricator. (Upstream dev and stable branches are at upstream/main and upstream/stable respectively.) hq.recaptime.dev/wiki/Phorge
phorge phabricator
1
fork

Configure Feed

Select the types of activity you want to include in your feed.

Avoid to generate a permalink for every clicked line number: migrate to web fragments

Summary:
Historically, when somebody visited a Paste or Diffusion `file` and clicked on a line number, we always constructed URIs in the format `file$45` as the link target for line number `45` in the line number column of document `file` (resp. `file$1-10` for lines 1-10), providing the ability to highlight one or several lines. Accessing this URI highlights the corresponding lines and automatically scrolls to the highlighted line(s) with a 60px top margin.

It is important to note that when `$45` is present in a URI, it is sent to the server (making a "permalink"), while `#45` would be not. So, a downside of using this permalink-based approach instead of standard `#` HTML anchor fragments ("web fragments") is that every URI obtained and shared around while clicking from the line number column in Paste and Diffusion file pages, that permalink `file$123` is technically a separate URI to be visited and downloaded compared to `file`. When you click on a line number and you share that permalink on a public comment, that permalink attracts crawlers, and they download that page again, even if crawlers already accessed that content. In an age of pirate AI trainers that scrape any permalink every second, it's better to adopt web fragments instead, so that they download your file once (or at least less often....), whatever the amount of comments you shared talking about lines.

As a solution:

* When clicking a single line, such as 123, generate a web fragment like `#L123` for the link target. This preserves line highlighting but vastly reduces the number of links a crawler could follow when it reaches such URIs, e.g., when a user mentions that line `#L123` from a comment.
* When clicking on multiple lines 123-124, also use web fragments like `#L123-124`.
* When receiving legacy visits to `$123` or `$L123-124`, it still works; we have not introduced link rot. However, if you visit these and if you click again on a different line, you get the new fragment-based URI.

Don't panic. Note that the previous situation was not extremely bad anyway, as the Paste or Diffusion did __not__ really allow a permalink to be found for each line anyway. A click was still needed to generate such permalink and a human being was still needed to share such permalink somewhere (e.g. a comment).

Note that this modification cannot replace all comments previously shared by human beings, and they probably shouldn't.

Note that Phorge cannot travel back in time, and we cannot obliviate crawlers to do not try to visit anymore old `$123` URIs. At least the promise is that your coworkers will not generate any more of these additional permalinks, and no link is broken.

See T15670
Closes T16100

Test Plan:
About the new feature:

* Go to http://phorge.localhost/P1, click on line number 11, see that highlighting still works and URL in the address bar becomes http://phorge.localhost/P1#L11 without a page reload
* Access URL http://phorge.localhost/P1#L13 and see that position scrolling by the browser works as expected
* Go to http://phorge.localhost/P1, click on line number 11 and drag to number 17, see that highlighting still works as before and URL in the address bar becomes http://phorge.localhost/P1#L11-17
* Go to http://phorge.localhost/P1#L11-17 directly, still works
* Go to http://phorge.localhost/P1#L17-11 (that is, first 17, then 11) and you just highlighted from line 11 to 17 successfully, like above, no nuclear crash
* Go to http://phorge.localhost/P1#LMIAAAO and really nothing special happens, no crash, just a nonsense web fragment
* Go to http://phorge.localhost/P1#L2-99999999999 and you highlight only the existing lines, without JavaScript burnout

About possible regressions:

* Access URL http://phorge.localhost/P1$9 and see that highlighting and position scrolling by the server-side JS still works as expected
* Access URL http://phorge.localhost/P1$9-16 and see that highlighting and position scrolling by the server-side JS still works as expected
* Access URL http://phorge.localhost/P1$16-9, same as above
* Access the above legacy URIs and click again on another line number, e.g. 2, and you get the new URIs with fragments (no silly things like `P1$9#2`, but just `P1#L2`)

----

For both, perform the same tests in a source code file rendered in Diffusion. Same results.

----

As additional test, you can define the new JavaScript functions in your browser console. E.g. copy-pasting this:

```javascript
var _parseLineNumber = function(vRaw) { ....
var parseMinMaxSelectedLineFromFragment = function(input) { ...
```

Then, test if JavaScript can recognize the fragment interval:

```
$ parseMinMaxSelectedLineFromFragment("L123")
Array [ 123, 123 ]
```

```
$ parseMinMaxSelectedLineFromFragment("L2-50")
Array [ 2, 50 ]
```

```
$ parseMinMaxSelectedLineFromFragment("L50-2")
Array [ 2, 50 ]
```

Also test some nonsense inputs, and they successfully crash:

```lang=javascript,counterexample
$ // Test nonsense line.
$ parseMinMaxSelectedLineFromFragment("LMIAO")
Uncaught Input fragment parts must be positive integer. Got: MIAO debugger eval code:11:70
_parseLineNumber debugger eval code:11
parseMinMaxSelectedLineFromFragment debugger eval code:42
```

```lang=javascript,counterexample
$ // Test nonsense zero line.
$ parseMinMaxSelectedLineFromFragment("L123-0")
Uncaught Input fragment parts must be positive integer. Got: 0 debugger eval code:11:70
_parseLineNumber debugger eval code:11
parseMinMaxSelectedLineFromFragment debugger eval code:43
```

```lang=javascript,counterexample
$ // Test nonsense negative line.
$ parseMinMaxSelectedLineFromFragment("L-123")
Uncaught Input fragment parts must be positive integer. Got: debugger eval code:11:70
_parseLineNumber debugger eval code:11
parseMinMaxSelectedLineFromFragment debugger eval code:42
```

Note: these crashes are __not__ shown to end-users while using Phorge. These exceptions are only for test plan lovers, and for developers.

So we recognize correct web fragments, and discard nonsense ones. Giving backward compatibility.

Reviewers: O1 Blessed Committers, aklapper, mainframe98

Reviewed By: O1 Blessed Committers, mainframe98

Subscribers: mainframe98, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno

Maniphest Tasks: T16100

Differential Revision: https://we.phorge.it/D25569

authored by

Andre Klapper and committed by
Valerio Bozzolan
966e80e8 81f13876

+101 -16
+9 -9
resources/celerity/map.php
··· 465 465 'rsrc/js/core/behavior-keyboard-pager.js' => '1325b731', 466 466 'rsrc/js/core/behavior-keyboard-shortcuts.js' => '42c44e8b', 467 467 'rsrc/js/core/behavior-lightbox-attachments.js' => '14c7ab36', 468 - 'rsrc/js/core/behavior-line-linker.js' => '0d915ff5', 468 + 'rsrc/js/core/behavior-line-linker.js' => '8cbbcfc5', 469 469 'rsrc/js/core/behavior-linked-container.js' => '74446546', 470 470 'rsrc/js/core/behavior-more.js' => '506aa3f4', 471 471 'rsrc/js/core/behavior-object-selector.js' => '98ef467f', ··· 627 627 'javelin-behavior-phabricator-gesture-example' => '242dedd0', 628 628 'javelin-behavior-phabricator-keyboard-pager' => '1325b731', 629 629 'javelin-behavior-phabricator-keyboard-shortcuts' => '42c44e8b', 630 - 'javelin-behavior-phabricator-line-linker' => '0d915ff5', 630 + 'javelin-behavior-phabricator-line-linker' => '8cbbcfc5', 631 631 'javelin-behavior-phabricator-notification-example' => '29819b75', 632 632 'javelin-behavior-phabricator-object-selector' => '98ef467f', 633 633 'javelin-behavior-phabricator-oncopy' => 'da8f5259', ··· 984 984 ), 985 985 '0d2490ce' => array( 986 986 'javelin-install', 987 - ), 988 - '0d915ff5' => array( 989 - 'javelin-behavior', 990 - 'javelin-stratcom', 991 - 'javelin-dom', 992 - 'javelin-history', 993 - 'javelin-external-editor-link-engine', 994 987 ), 995 988 '0eaa33a9' => array( 996 989 'javelin-behavior', ··· 1668 1661 'phabricator-title', 1669 1662 'phabricator-shaped-request', 1670 1663 'conpherence-thread-manager', 1664 + ), 1665 + '8cbbcfc5' => array( 1666 + 'javelin-behavior', 1667 + 'javelin-stratcom', 1668 + 'javelin-dom', 1669 + 'javelin-history', 1670 + 'javelin-external-editor-link-engine', 1671 1671 ), 1672 1672 '8e0aa661' => array( 1673 1673 'javelin-install',
+3 -1
src/view/layout/PhabricatorSourceCodeView.php
··· 131 131 } 132 132 133 133 if ($this->canClickHighlight) { 134 + $line_id = 'L'.$line_number; 134 135 if ($base_uri) { 135 - $line_href = $base_uri.'$'.$line_number; 136 + $line_href = $base_uri.'#'.$line_id; 136 137 } else { 137 138 $line_href = null; 138 139 } ··· 142 143 array( 143 144 'href' => $line_href, 144 145 'data-n' => $line_number, 146 + 'id' => $line_id, 145 147 )); 146 148 } else { 147 149 $tag_number = phutil_tag(
+89 -6
webroot/rsrc/js/core/behavior-line-linker.js
··· 130 130 } 131 131 }; 132 132 133 + /** 134 + * Get a valid line number (int) from a string, or scream violently. 135 + * 136 + * @param {String} vRaw String containing a number, like '1'. 137 + * @return {Integer} Integer like 1. 138 + * @throws Do not accept zero. Do not accept negative numbers. 139 + */ 140 + var _parseLineNumber = function(vRaw) { 141 + var v = parseInt(vRaw); 142 + if (isNaN(v) || v <= 0) { 143 + throw 'Input fragment parts must be positive integer. Got: ' + vRaw; 144 + } 145 + return v; 146 + }; 147 + 148 + /** 149 + * Parse the highlighted lines from web fragment, or scream violently. 150 + * 151 + * @param {String} Input string like 'L123' or 'L123-124'. 152 + * @return {Array} Array with always 2 elements: min and max line. 153 + * From the fragment '#L123' you get the array [123, 123]. 154 + * From the fragment '#L123-124' you get the array [123, 124]. 155 + * From the fragment '#L123-123' you get the array [123, 123]. 156 + * @throws Do not accept trash like '#Labc', '#L123-456-789', '#L123-abc'. 157 + */ 158 + var parseMinMaxSelectedLineFromFragment = function(input) { 159 + // The web fragment must be 'L123' or 'L123-124' or similar. 160 + if (!input || input.charAt(0) !== 'L') { 161 + throw 'Input fragment is not a line fragment.'; 162 + } 163 + 164 + // Strip the 'L' and parse the '-' interval (if any). 165 + var linesStr = input.substring(1); 166 + var lines = linesStr.split('-', 2); 167 + var hasOne = lines.length === 1; 168 + var hasTwo = lines.length === 2; 169 + if (!hasOne && !hasTwo) { 170 + throw 'Input fragment must be valid, like L123 or L123-456.'; 171 + } 172 + 173 + // Require valid integers. 174 + var a = _parseLineNumber(lines[0]); 175 + var b = hasTwo ? _parseLineNumber(lines[1]) : a; 176 + 177 + // Sort interval. Avoid dumb JavaScript sort() that returns strings. 178 + if (a < b) { 179 + return [a, b]; 180 + } 181 + return [b, a]; 182 + }; 183 + 133 184 JX.Stratcom.listen('mouseover', 'phabricator-source', highlight); 134 185 135 186 JX.Stratcom.listen( ··· 151 202 if (!uri) { 152 203 uri = JX.$U(window.location); 153 204 path = uri.getPath(); 205 + 206 + // Cleanup legacy URIs using '$123' to highlight that line. 154 207 path = path.replace(/\$[\d-]+$/, ''); 155 208 uri.setPath(path); 156 209 uri = uri.toString(); ··· 160 213 target = null; 161 214 root = null; 162 215 163 - var lines = (o == t ? o : Math.min(o, t) + '-' + Math.max(o, t)); 164 - 165 216 uri = JX.$U(uri); 166 217 path = uri.getPath(); 167 - path = path + '$' + lines; 218 + 219 + // Check if we should highlight a single line or an interval. 220 + // Refresh the web fragment. 221 + var lineInterval = (o == t) ? o : Math.min(o, t) + '-' + Math.max(o, t); 222 + var lineIdentifier = 'L' + lineInterval; 223 + uri.setFragment(lineIdentifier); 224 + 168 225 uri = uri.setPath(path).toString(); 169 226 170 227 JX.History.replace(uri); ··· 188 245 }); 189 246 190 247 191 - // Try to jump to the highlighted lines if we don't have an explicit anchor 192 - // in the URI. 193 - if (!window.location.hash.length) { 248 + // Try to jump to the highlighted lines at startup. 249 + if (window.location.hash.length) { 250 + // Parse the web fragment '#L123' or '#L123-124' and highlight that. 251 + var currentFragment = JX.$U(window.location).getFragment(); 252 + try { 253 + var lines = parseMinMaxSelectedLineFromFragment(currentFragment); 254 + var minLine = lines[0]; 255 + var maxLine = lines[1]; 256 + 257 + // Scroll to the very first line. 258 + var lineNode = JX.$('L' + minLine); 259 + var tr = JX.DOM.findAbove(lineNode, 'tr'); 260 + JX.DOM.scrollToPosition(0, JX.$V(tr).y - 60); 261 + 262 + // Highlight every line in the interval. 263 + // Note that this crashes successfully on the first non-existing element, 264 + // so you cannot really use '#L1-9999999999 to cause JS overheat. 265 + for (var i = minLine; i <= maxLine; i++) { 266 + lineNode = JX.$('L' + i); 267 + tr = JX.DOM.findAbove(lineNode, 'tr'); 268 + JX.DOM.alterClass(tr, 'phabricator-source-highlight', true); 269 + } 270 + } catch (ex) { 271 + // If the '#L' fragment parser crashed, just move on. 272 + // If we didn't hit an element on the page, just move on. 273 + } 274 + } else { 275 + // in the URI. 276 + // This is from legacy '$123' URIs. 194 277 try { 195 278 var anchor = JX.$('phabricator-line-linker-anchor'); 196 279 JX.DOM.scrollToPosition(0, JX.$V(anchor).y - 60);