From 966e80e89efd137839f53a47aa3002fb393941ef Mon Sep 17 00:00:00 2001 From: Andre Klapper Date: Fri, 27 Jun 2025 15:48:50 +0200 Subject: [PATCH] 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 --- resources/celerity/map.php | 18 ++-- src/view/layout/PhabricatorSourceCodeView.php | 4 +- webroot/rsrc/js/core/behavior-line-linker.js | 95 +++++++++++++++++-- 3 files changed, 101 insertions(+), 16 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index c380c71802..ee95bdde25 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -465,7 +465,7 @@ return array( 'rsrc/js/core/behavior-keyboard-pager.js' => '1325b731', 'rsrc/js/core/behavior-keyboard-shortcuts.js' => '42c44e8b', 'rsrc/js/core/behavior-lightbox-attachments.js' => '14c7ab36', - 'rsrc/js/core/behavior-line-linker.js' => '0d915ff5', + 'rsrc/js/core/behavior-line-linker.js' => '8cbbcfc5', 'rsrc/js/core/behavior-linked-container.js' => '74446546', 'rsrc/js/core/behavior-more.js' => '506aa3f4', 'rsrc/js/core/behavior-object-selector.js' => '98ef467f', @@ -627,7 +627,7 @@ return array( 'javelin-behavior-phabricator-gesture-example' => '242dedd0', 'javelin-behavior-phabricator-keyboard-pager' => '1325b731', 'javelin-behavior-phabricator-keyboard-shortcuts' => '42c44e8b', - 'javelin-behavior-phabricator-line-linker' => '0d915ff5', + 'javelin-behavior-phabricator-line-linker' => '8cbbcfc5', 'javelin-behavior-phabricator-notification-example' => '29819b75', 'javelin-behavior-phabricator-object-selector' => '98ef467f', 'javelin-behavior-phabricator-oncopy' => 'da8f5259', @@ -985,13 +985,6 @@ return array( '0d2490ce' => array( 'javelin-install', ), - '0d915ff5' => array( - 'javelin-behavior', - 'javelin-stratcom', - 'javelin-dom', - 'javelin-history', - 'javelin-external-editor-link-engine', - ), '0eaa33a9' => array( 'javelin-behavior', 'javelin-dom', @@ -1669,6 +1662,13 @@ return array( 'phabricator-shaped-request', 'conpherence-thread-manager', ), + '8cbbcfc5' => array( + 'javelin-behavior', + 'javelin-stratcom', + 'javelin-dom', + 'javelin-history', + 'javelin-external-editor-link-engine', + ), '8e0aa661' => array( 'javelin-install', 'javelin-dom', diff --git a/src/view/layout/PhabricatorSourceCodeView.php b/src/view/layout/PhabricatorSourceCodeView.php index ae20fe2501..73681a84f9 100644 --- a/src/view/layout/PhabricatorSourceCodeView.php +++ b/src/view/layout/PhabricatorSourceCodeView.php @@ -131,8 +131,9 @@ final class PhabricatorSourceCodeView extends AphrontView { } if ($this->canClickHighlight) { + $line_id = 'L'.$line_number; if ($base_uri) { - $line_href = $base_uri.'$'.$line_number; + $line_href = $base_uri.'#'.$line_id; } else { $line_href = null; } @@ -142,6 +143,7 @@ final class PhabricatorSourceCodeView extends AphrontView { array( 'href' => $line_href, 'data-n' => $line_number, + 'id' => $line_id, )); } else { $tag_number = phutil_tag( diff --git a/webroot/rsrc/js/core/behavior-line-linker.js b/webroot/rsrc/js/core/behavior-line-linker.js index fd8510646e..e0fbabfed4 100644 --- a/webroot/rsrc/js/core/behavior-line-linker.js +++ b/webroot/rsrc/js/core/behavior-line-linker.js @@ -130,6 +130,57 @@ JX.behavior('phabricator-line-linker', function() { } }; + /** + * Get a valid line number (int) from a string, or scream violently. + * + * @param {String} vRaw String containing a number, like '1'. + * @return {Integer} Integer like 1. + * @throws Do not accept zero. Do not accept negative numbers. + */ + var _parseLineNumber = function(vRaw) { + var v = parseInt(vRaw); + if (isNaN(v) || v <= 0) { + throw 'Input fragment parts must be positive integer. Got: ' + vRaw; + } + return v; + }; + + /** + * Parse the highlighted lines from web fragment, or scream violently. + * + * @param {String} Input string like 'L123' or 'L123-124'. + * @return {Array} Array with always 2 elements: min and max line. + * From the fragment '#L123' you get the array [123, 123]. + * From the fragment '#L123-124' you get the array [123, 124]. + * From the fragment '#L123-123' you get the array [123, 123]. + * @throws Do not accept trash like '#Labc', '#L123-456-789', '#L123-abc'. + */ + var parseMinMaxSelectedLineFromFragment = function(input) { + // The web fragment must be 'L123' or 'L123-124' or similar. + if (!input || input.charAt(0) !== 'L') { + throw 'Input fragment is not a line fragment.'; + } + + // Strip the 'L' and parse the '-' interval (if any). + var linesStr = input.substring(1); + var lines = linesStr.split('-', 2); + var hasOne = lines.length === 1; + var hasTwo = lines.length === 2; + if (!hasOne && !hasTwo) { + throw 'Input fragment must be valid, like L123 or L123-456.'; + } + + // Require valid integers. + var a = _parseLineNumber(lines[0]); + var b = hasTwo ? _parseLineNumber(lines[1]) : a; + + // Sort interval. Avoid dumb JavaScript sort() that returns strings. + if (a < b) { + return [a, b]; + } + return [b, a]; + }; + JX.Stratcom.listen('mouseover', 'phabricator-source', highlight); JX.Stratcom.listen( @@ -151,6 +202,8 @@ JX.behavior('phabricator-line-linker', function() { if (!uri) { uri = JX.$U(window.location); path = uri.getPath(); + + // Cleanup legacy URIs using '$123' to highlight that line. path = path.replace(/\$[\d-]+$/, ''); uri.setPath(path); uri = uri.toString(); @@ -160,11 +213,15 @@ JX.behavior('phabricator-line-linker', function() { target = null; root = null; - var lines = (o == t ? o : Math.min(o, t) + '-' + Math.max(o, t)); - uri = JX.$U(uri); path = uri.getPath(); - path = path + '$' + lines; + + // Check if we should highlight a single line or an interval. + // Refresh the web fragment. + var lineInterval = (o == t) ? o : Math.min(o, t) + '-' + Math.max(o, t); + var lineIdentifier = 'L' + lineInterval; + uri.setFragment(lineIdentifier); + uri = uri.setPath(path).toString(); JX.History.replace(uri); @@ -188,9 +245,35 @@ JX.behavior('phabricator-line-linker', function() { }); - // Try to jump to the highlighted lines if we don't have an explicit anchor - // in the URI. - if (!window.location.hash.length) { + // Try to jump to the highlighted lines at startup. + if (window.location.hash.length) { + // Parse the web fragment '#L123' or '#L123-124' and highlight that. + var currentFragment = JX.$U(window.location).getFragment(); + try { + var lines = parseMinMaxSelectedLineFromFragment(currentFragment); + var minLine = lines[0]; + var maxLine = lines[1]; + + // Scroll to the very first line. + var lineNode = JX.$('L' + minLine); + var tr = JX.DOM.findAbove(lineNode, 'tr'); + JX.DOM.scrollToPosition(0, JX.$V(tr).y - 60); + + // Highlight every line in the interval. + // Note that this crashes successfully on the first non-existing element, + // so you cannot really use '#L1-9999999999 to cause JS overheat. + for (var i = minLine; i <= maxLine; i++) { + lineNode = JX.$('L' + i); + tr = JX.DOM.findAbove(lineNode, 'tr'); + JX.DOM.alterClass(tr, 'phabricator-source-highlight', true); + } + } catch (ex) { + // If the '#L' fragment parser crashed, just move on. + // If we didn't hit an element on the page, just move on. + } + } else { + // in the URI. + // This is from legacy '$123' URIs. try { var anchor = JX.$('phabricator-line-linker-anchor'); JX.DOM.scrollToPosition(0, JX.$V(anchor).y - 60); -- 2.51.2