Address reviewer comments

app/client/components/Cursor.ts

@@ -17,15 +17,20 @@ function nullAsUndefined<T>(value: T|null|undefined): T|undefined {
 }
 
 // ================ SequenceNum: used to keep track of cursor edits (lastEditedAt)
-// basically just a global auto-incrementing counter, with some types to make intent a bit more clear
+// Basically just a global auto-incrementing counter, with some types to make intent more clear
-// Each cursor starts at SequenceNEVER (0), and after that all cursors using NextSequenceNum() will have
+// Cursors are constructed at SequenceNEVER (0). After that, changes to their sequenceNum will go through
-// a unique, monotonically increasing number for their lastEditedAt()
+// NextSequenceNum(), so they'll have unique, monotonically increasing numbers for their lastEditedAt()
+// NOTE: (by the time the page loads they'll already be at nonzero numbers, the never is intended to be transient)
 export type SequenceNum = number;
-export const SequenceNEVER: SequenceNum = 0;
+export const SequenceNEVER: SequenceNum = 0; // Cursors will start here
 let latestGlobalSequenceNum = SequenceNEVER;
-const nextSequenceNum = () => { return latestGlobalSequenceNum++; };
+function nextSequenceNum() { // First call to this func should return 1
+  latestGlobalSequenceNum++;
+  return latestGlobalSequenceNum;
+}
 
-// Note: we don't make any provisions for handling overflow. It's fine because:
+// NOTE: If latestGlobalSequenceNum overflows, I think it would stop incrementing because of floating point imprecision
+// However, we don't need to worry about overflow because:
 //   - Number.MAX_SAFE_INTEGER is 9,007,199,254,740,991 (9 * 10^15)
 //   - even at 1000 cursor-edits per second, it would take ~300,000 yrs to overflow
 //   - Plus it's client-side, so that's a single continuous 300-millenia-long session, which would be impressive uptime
@@ -127,6 +132,10 @@ export class Cursor extends Disposable {
     // IMPORTANT: need to subscribe AFTER the properRowId->activeRowId subscription.
     //  (Cursor-linking observables depend on lastCursorEdit, but only peek at activeRowId. Therefore, updating the
     //   edit time triggers a re-read of activeRowId, and swapping the order will read stale values for rowId)
+    // NOTE: this may update sequence number twice for a single edit, but this shouldn't cause any issues.
+    //       For determining priority, this cursor will become the latest edited whether we call it once or twice.
+    //       For updating observables, the double-update might cause cursor-linking observables in LinkingState to
+    //       double-update, but it should be transient and get resolved immediately.
     this.autoDispose(this._properRowId.subscribe(() => { this.cursorEdited(); }));
     this.autoDispose(this.fieldIndex.subscribe(() => { this.cursorEdited(); }));
 
@@ -156,28 +165,30 @@ export class Cursor extends Disposable {
    * @param isFromLink: should be set if this is a cascading update from cursor-linking
    */
   public setCursorPos(cursorPos: CursorPos, isFromLink: boolean = false): void {
-    //If updating as a result of links, we want to NOT update lastEditedAt
+
-    if(isFromLink) { this._silentUpdatesFlag = true; }
+    try {
-    //console.log(`CURSOR: ${silentUpdate}, silentUpdate=${this._silentUpdatesFlag},
+      // If updating as a result of links, we want to NOT update lastEditedAt
-    //            lastUpdated = ${this.lastUpdated.peek()}`) //TODO JV DEBUG TEMP
+      if (isFromLink) { this._silentUpdatesFlag = true; }
 
       if (cursorPos.rowId !== undefined && this.viewData.getRowIndex(cursorPos.rowId) >= 0) {
-      this.rowIndex(this.viewData.getRowIndex(cursorPos.rowId) );
+        this.rowIndex(this.viewData.getRowIndex(cursorPos.rowId));
       } else if (cursorPos.rowIndex !== undefined && cursorPos.rowIndex >= 0) {
         this.rowIndex(cursorPos.rowIndex);
       } else {
         // Write rowIndex to itself to force an update of rowId if needed.
         this.rowIndex(this.rowIndex.peek());
       }
+
       if (cursorPos.fieldIndex !== undefined) {
         this.fieldIndex(cursorPos.fieldIndex);
       }
 
-    //console.log(`CURSOR-END: silentUpdate=${this._silentUpdatesFlag},
+    } finally { // Make sure we reset this even on error
-    //            lastEditedAt = ${this._lastEditedAt.peek()}  `); //TODO JV DEBUG TEMP
       this._silentUpdatesFlag = false;
     }
 
+  }
+
 
 
 
@@ -185,13 +196,13 @@ export class Cursor extends Disposable {
     this._isLive(isLive);
   }
 
-  //Should be called whenever the cursor is updated
+  // Should be called whenever the cursor is updated
-  //EXCEPT FOR: when cursor is set by linking
+  // _EXCEPT FOR: when cursor is set by linking
-  //this is used to determine which widget/cursor has most recently been touched,
+  // this is used to determine which widget/cursor has most recently been touched,
-  //and therefore which one should be used to drive linking if there's a conflict
+  // and therefore which one should be used to drive linking if there's a conflict
-  public cursorEdited(): void {
+  private cursorEdited(): void {
-    //If updating as a result of links, we want to NOT update lastEdited
+    // If updating as a result of links, we want to NOT update lastEdited
-    if(!this._silentUpdatesFlag)
+    if (!this._silentUpdatesFlag)
       { this._lastEditedAt(nextSequenceNum()); }
   }
 }

app/client/components/GristDoc.ts

@@ -1135,7 +1135,7 @@ export class GristDoc extends DisposableWithEvents {
       }
 
       if (visitedSections.includes(section.id.peek())) {
-        //We've already been here (we hit a cycle), just return immediately
+        // We've already been here (we hit a cycle), just return immediately
         return true;
       }
 

app/client/components/LinkingState.ts

@@ -1,3 +1,4 @@
+import {SequenceNEVER, SequenceNum} from "app/client/components/Cursor";
 import {DataRowModel} from "app/client/models/DataRowModel";
 import DataTableModel from "app/client/models/DataTableModel";
 import {DocModel} from 'app/client/models/DocModel';
@@ -17,7 +18,6 @@ import merge = require('lodash/merge');
 import mapValues = require('lodash/mapValues');
 import pick = require('lodash/pick');
 import pickBy = require('lodash/pickBy');
-import {SequenceNEVER, SequenceNum} from "./Cursor";
 
 
 // Descriptive string enum for each case of linking
@@ -211,7 +211,7 @@ export class LinkingState extends Disposable {
       //                   [ A ]--------/        [ B ]   --------------/       [ C ]                                   |
       //                        A.actRowId                B.actRowId                                                   |
       //
-      // However, if e.g. viewSec B is filtered, the correct rowId might not exist in B, and so it's rowId would be
+      // However, if e.g. viewSec B is filtered, the correct rowId might not exist in B, and so its activeRowId would be
       // on a different row, and therefore the cursor linking would set C to a different row from A, even if it existed
       // in C
       //
@@ -239,9 +239,10 @@ export class LinkingState extends Disposable {
       // viewSection.lastCursorEdit). incomingCursorPos is a pair of [rowId, sequenceNum], so each linkingState sets its
       // incomingCursorPos to whichever is most recent between its srcSection, and the previous LS's incCursPos.
       //
-      // The end result is that there is that because the lastCursorEdits are guaranteed to be unique, there is always
+      // If we do this right, the end result is that because the lastCursorEdits are guaranteed to be unique,
-      // a stable configuration of links, where even in the case of a cycle the incomingCursorPos will all take their
+      // there is always a stable configuration of links, where even in the case of a cycle the incomingCursorPos-es
-      // rowId and version from the most recently edited viewSection in the cycle, which is what the user expects
+      // will all take their rowId and version from the most recently edited viewSection in the cycle,
+      // which is what the user expects
       //
       //               ...from C--> [A.LS] -------->  [B.LS]               --> [C.LS] ----->...to A                    |
       //                               |                 |                /       |                                    |
@@ -252,8 +253,10 @@ export class LinkingState extends Disposable {
       // Once the incomingCursorPos-es are determined correctly, the cursorPos-es just need to pull out the rowId,
       // and that will drive the cursors of the associated tgt section for each LS.
       //
-      // NOTE: setting cursorPos will change the viewSections' cursor, but it's special-cased to
+      // NOTE: setting cursorPos *WILL* change the viewSections' cursor, but it's special-cased to
-      // not affect their lastCursorEdit times.
+      // so that cursor-driven linking doesn't modify their lastCursorEdit times, so that lastCursorEdit
+      // reflects only changes driven by external factors
+      // (e.g. page load, user moving cursor, user changing linking settings/filter settings)
       // =============================
 
       // gets the relevant col value for the passed-in rowId, or return rowId unchanged if same-table link
@@ -264,25 +267,31 @@ export class LinkingState extends Disposable {
         //Incoming-cursor-pos determines what the linked cursor position should be, considering the previous
         //linked section (srcSection) and all upstream sections (through srcSection.linkingState)
         this.incomingCursorPos = this.autoDispose((ko.computed(() => {
-          // Get previous linkingstate's info
+          // NOTE: This computed primarily decides between srcSec and prevLink. Here's what those mean:
-          // NOTE: prevlink = this.srcSec.linkingState is 2 hops back, (i.e. it reads from from this.srcSec.linkSrcSec)
+          // e.g. consider sections A->B->C, (where this === C)
-          //       The section 1 link back from the current (tgt) section is this.srcSec;
+          // We need to decide between taking cursor info from B, our srcSection (1 hop back)
-          const prevLink = this._srcSection.linkingState?.();
+          //    vs taking cursor info from further back, e.g. A, or before (2+ hops back)
-          const prevLinkHasCursor = prevLink &&
+          // To take cursor info from further back, we rely on B's linkingState, since B's linkingState will
-            (prevLink.linkTypeDescription() == "Cursor:Same-Table" ||
+          //    be looking at the preceding sections, either A or whatever is behind A.
-             prevLink.linkTypeDescription() == "Cursor:Reference");
+          // Therefore: we either use srcSection (1 back), or prevLink = srcSection.linkingState (2+ back)
-          const [prevLinkedPos, prevLinkedVersion] = prevLinkHasCursor ? prevLink.incomingCursorPos() :
+
-                                                                         [null, SequenceNEVER];
+          // Get srcSection's info (1 hop back)
-          // Get srcSection's info
           const srcSecPos = this._srcSection.activeRowId.peek(); //we don't depend on this, only on its cursor version
           const srcSecVersion = this._srcSection.lastCursorEdit();
 
-          // Sanity check: version might be NEVER if viewSection's cursor hasn't initialized (shouldn't happen I think)
+          // If cursors haven't been initialized, cursor-linking doesn't make sense, so don't do it
-          if(srcSecVersion == SequenceNEVER) {
+          if(srcSecVersion === SequenceNEVER) {
-            console.warn("=== linkingState: cursor-linking, srcSecVersion = NEVER");
             return [null, SequenceNEVER] as [UIRowId|null, SequenceNum];
           }
 
+          // Get previous linkingstate's info, if applicable (2 or more hops back)
+          const prevLink = this._srcSection.linkingState?.();
+          const prevLinkHasCursor = prevLink &&
+            (prevLink.linkTypeDescription() === "Cursor:Same-Table" ||
+              prevLink.linkTypeDescription() === "Cursor:Reference");
+          const [prevLinkedPos, prevLinkedVersion] = prevLinkHasCursor ? prevLink.incomingCursorPos() :
+            [null, SequenceNEVER];
+
           // ==== Determine whose info to use:
           // If prevLinkedVersion < srcSecVersion, then the prev linked data is stale, don't use it
           // If prevLinkedVersion == srcSecVersion, then srcSec is the driver for this link cycle (i.e. we're its first

app/client/ui/RightPanel.ts

@@ -645,8 +645,8 @@ export class RightPanel extends Disposable {
       const lfilter = lstate?.filterState ? use(lstate.filterState) : undefined;
 
       // Debug info for cursor linking
-      const inPos = lstate?.incomingCursorPos?.();
+      const inPos = lstate?.incomingCursorPos ? use(lstate.incomingCursorPos) : null;
-      const cursorPosStr = (lstate?.cursorPos ? `${tgtSec.tableId()}[${use(lstate.cursorPos)}]` : "N/A") +
+      const cursorPosStr = (lstate?.cursorPos ? `${use(tgtSec.tableId)}[${use(lstate.cursorPos)}]` : "N/A") +
       // TODO: the lastEdited and incomingCursorPos is kinda technical, to do with how bidirectional linking determines
       //       priority for cyclical cursor links. Might be too technical even for the "advanced info" box
         `\n srclastEdited: T+${use(srcSec.lastCursorEdit)} \n tgtLastEdited: T+${use(tgtSec.lastCursorEdit)}` +

app/client/ui/selectBy.ts

@@ -52,9 +52,11 @@ interface LinkNode {
   // relationship. ancestors[0] is this.section, ancestors[length-1] is oldest ancestor
   ancestors: number[];
 
-  // Records which links between ancestors are same-table cursor-links
+  // For bidirectional linking, cycles are only allowed if all links on that cycle are same-table cursor-link
-  // if isAncCursLink[0] == true, that means the link from ancestors[0] to ancestors[1] is a same-table cursor-link
+  // this.ancestors only records what the ancestors are, but we need to record info about the edges between them.
-  // NOTE: (it is therefore 1 shorter than ancestors, unless the links form a cycle in which case it's the same len)
+  // isAncCursLink[i]==true  means the link from ancestors[i] to ancestors[i+1] is a same-table cursor-link
+  // NOTE: (Since ancestors is a list of nodes, and this is a list of the edges between those nodes, this list will
+  //        be 1 shorter than ancestors (if there's no cycle), or will be the same length (if there is a cycle))
   isAncestorSameTableCursorLink: boolean[];
 
   // the section record. Must be the empty record sections that are to be created.
@@ -149,8 +151,8 @@ function isValidLink(source: LinkNode, target: LinkNode) {
   // The link must not create a cycle, unless it's only same-table cursor-links all the way to target
   if (source.ancestors.includes(target.section.getRowId())) {
 
-    //cycles only allowed for cursor links
+    // cycles only allowed for cursor links
-    if(source.column || target.column || source.isSummary) {
+    if (source.column || target.column || source.isSummary) {
       return false;
     }
 
@@ -161,19 +163,20 @@ function isValidLink(source: LinkNode, target: LinkNode) {
     //    they would stop being ancestors of src)
     // NOTE: we're guaranteed to hit target before the end of the array (because of the `if(...includes...)` above)
     // ALSO NOTE: isAncestorSameTableCursorLink may be 1 shorter than ancestors, but it's accounted for by the above
-
+    let i = 0
-    for(let i = 0; ; i++) {
+    while(true) {
-      if(source.ancestors[i] == target.section.getRowId()) {
+      if (source.ancestors[i] == target.section.getRowId()) {
         // We made it! All is well
         break;
       }
       // If we've hit the last ancestor and haven't found target, error out (shouldn't happen, we checked for it)
-      if(i == source.ancestors.length-1) { throw Error("Error: Array doesn't include targetSection"); }
+      if (i == source.ancestors.length-1) { throw Error("Array doesn't include targetSection"); }
 
-      // Need to follow another link back, verify that it's a same-table cursor-link
+      // We're not done with the cycle. Check next link, if it's non-same-table-cursor-link, the cycle is invalid.
-      if(!source.isAncestorSameTableCursorLink[i]) { return false; }
+      if (!source.isAncestorSameTableCursorLink[i]) { return false; }
+
+      i++; // Walk back
     }
-    console.log("===== selectBy found valid cycle", JSON.stringify(source)); //TODO JV TEMP DEBUG
     // Yay, this is a valid cycle of same-table cursor-links
   }
 
@@ -269,15 +272,16 @@ function fromViewSectionRec(section: ViewSectionRec): LinkNode[] {
     ancestors.push(sec.getRowId());
 
     //Determine if this link is a same-table cursor link
-    if(sec.linkSrcSection.peek().getRowId()) {
+    if (sec.linkSrcSection.peek().getRowId()) { // if sec has incoming link
       const srcCol = sec.linkSrcCol.peek().getRowId();
       const tgtCol = sec.linkTargetCol.peek().getRowId();
       const srcTable = sec.linkSrcSection.peek().table.peek();
       const srcIsSummary = srcTable.primaryTableId.peek() !== srcTable.tableId.peek();
-      isAncestorSameTableCursorLink.push(srcCol == 0 && tgtCol == 0 && !srcIsSummary);
+      isAncestorSameTableCursorLink.push(srcCol === 0 && tgtCol === 0 && !srcIsSummary);
     }
-    // NOTE: isAncestorSameTableCursorLink may be 1 shorter than ancestors, since last ancestor has no incoming link
+    // NOTE: isAncestorSameTableCursorLink may be 1 shorter than ancestors, since we might skip pushing
-    // however if we have a cycle (of cursor-links), then they'll be the same length
+    // when we hit the last ancestor (which has no incoming link)
+    // however if we have a cycle (of cursor-links), then they'll be the same length, because we won't skip last push
   }
 
   const isSummary = table.primaryTableId.peek() !== table.tableId.peek();

test/nbrowser/RightPanelSelectBy.ts

@@ -78,7 +78,7 @@ describe('RightPanelSelectBy', function() {
   it('should allow creating cursor-linked-cycles', async function() {
     assert.equal(await driver.findContent('.test-select-row', /Performances detail/).isPresent(), true);
 
-    // undo, link is expected to be unset for next test
+    // undo, the operation from the previous test; link is expected to be unset for next test
     await gu.undo();
   });