Comments, adjust for loop

app/client/components/Cursor.ts

@@ -186,11 +186,15 @@ export class Cursor extends Disposable {
       // NOTE: _cursorEdited
       // We primarily update cursorEdited counter from a this._properRowId.subscribe(), since that catches updates
       //   from many sources (setCursorPos, arrowKeys, save/load, filter/sort-changes, etc)
-      // However, when deleting a row with several sections linked together, there can be a case where this fails:
-      //   When GridView.deleteRows calls setCursorPos to keep cursor from jumping after delete, the observable
+      // However, there's some cases where we user touches a section and properRowId doesn't change. Obvious one is
+      //   clicking in a section on the cell the cursor is already on. This doesn't change the cursor position, but it
+      //   SHOULD still update cursors to use that section as most up-to-date (user just clicked on a cell!), so we do
+      //   it here. (normally is minor issue, but can matter when a section has rows filtered out so cursors desync)
+      // Also a more subtle case: when deleting a row with several sections linked together, properRowId can fail to
+      //   update. When GridView.deleteRows calls setCursorPos to keep cursor from jumping after delete, the observable
       //   doesn't trigger cursorEdited(), because (I think) _properRowId has already been updated that cycle.
-      //   This caused a bug when several viewSections were cursor-linked to each other and a row was deleted.
-      // We can explicitly call cursorEdited again here. It'll cause cursorEdited to be called twice sometimes,
+      //   This caused a bug when several viewSections were cursor-linked to each other and a row was deleted
+      // NOTE: Calling it explicitly here will cause cursorEdited to be called twice sometimes,
       //   but that shouldn't cause any problems, since we don't care about edit counts, just who was edited latest.
       this._cursorEdited();
 
@@ -208,7 +212,7 @@ export class Cursor extends Disposable {
   }
 
   // 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,
   // and therefore which one should be used to drive linking if there's a conflict
   private _cursorEdited(): void {

app/client/ui/selectBy.ts

@@ -156,27 +156,42 @@ function isValidLink(source: LinkNode, target: LinkNode) {
       return false;
     }
 
-    // Walk backwards along the chain of ancestors
-    // - if we hit a non-cursor link before reaching target, then that would be an illegal cycle
-    // - when we hit target, we've verified that this is a legal cycle, so break
-    //   (ancestors further up the hierarchy past target don't matter, since once we set target.linkSrcSec = src.sec,
-    //    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 (;;) {
+    // We know our ancestors cycle back around to ourselves
+    // - lets walk back along the cyclic portion of the ancestor chain and verify that each link in that chain is
+    //   a cursor-link
+
+    // e.g. if the current link graph is:
+    //                     A->B->TGT->C->D->SRC
+    //    (SRC.ancestors):[5][4] [3] [2][1] [0]
+    // We're verifying the new potential link SRC->TGT, which would turn the graph into:
+    //             [from SRC] -> TGT -> C -> D -> SRC -> [to TGT]
+    // (Note that A and B will be cut away, since we change TGT's link source)
+    //
+    // We need to make sure that each link going backwards from `TGT -> C -> D -> SRC` is a same-table-cursor-link,
+    // since we disallow cycles with other kinds of links.
+    // isAncestorCursorLink[i] will tell us if the link going into ancestors[i] is a same-table-cursor-link
+    // So before we step from i=0 (SRC) to i=1 (D), we check isAncestorCursorLink[0], which tells us about D->SRC
+    let i;
+    for (i = 0; i < source.ancestors.length; i++) { // Walk backwards through the ancestors
+
+      // Once we hit the target section, we've seen all links that will be part of the cycle, and they've all been valid
       if (source.ancestors[i] == target.section.getRowId()) {
-        // We made it! All is well
-        break;
+        break; // Success!
       }
-      // 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("Array doesn't include targetSection"); }
 
-      // 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; }
-
-      i++; // Walk back
+      // Check that the link to the preceding section is valid
+      // NOTE! isAncestorSameTableCursorLink could be 1 shorter than ancestors!
+      // (e.g. if the graph looks like A->B->C, there's 3 ancestors but only two links)
+      // (however, if there's already a cycle, they'll be the same length ( [from C]->A->B->C, 3 ancestors & 3 links)
+      // If the link doesn't exist (shouldn't happen?) OR the link is not same-table-cursor, the cycle is invalid
+      if (i >= source.isAncestorSameTableCursorLink.length ||
+          !source.isAncestorSameTableCursorLink[i]) { return false; }
     }
+
+    // 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) { throw Error("Array doesn't include targetSection"); }
+
+
     // Yay, this is a valid cycle of same-table cursor-links
   }