Bidirectional Linking (#622)

Allows bidirectional / cyclic linking for same-record cursor links.
This should allow multiple sections to all synchronize their cursors,
such that clicking in any one of them will move all the others.

Works even if some sections in the cycle have rows filtered out (the 
filtered-out sections might desync their cursors, but the correct cursor
position will still propagate downstream, and they'll re-sync if clicking on
a row that is present in them)

Under the hood, each cursor has a _lastEditedAt counter, updated when
a user's action changes the cursor in a section, such that we can always
tell which section was touched most recently. This is used to resolve
conflicts stably when dealing with cycles or chains of cursor-links.

Updated selectBy and recursiveMoveToCursorPos to handle cycles

Updated tests for selectBy behavior

However, main bidirectional-linking tests are not in this commit, they'll come in a subsequent PR

app/client/components/BaseView.js

@@ -132,7 +132,7 @@ function BaseView(gristDoc, viewSectionModel, options) {
   // Update the cursor whenever linkedRowId() changes (but only if we have any linking).
   this.autoDispose(this.linkedRowId.subscribe(rowId => {
     if (this.viewSection.linkingState.peek()) {
-      this.setCursorPos({rowId: rowId || 'new'});
+      this.setCursorPos({rowId: rowId || 'new'}, true);
     }
   }));
 
@@ -282,14 +282,14 @@ BaseView.prototype.deleteRecords = function(source) {
 
 /**
  * Sets the cursor to the given position, deferring if necessary until the current query finishes
- * loading.
+ * loading. isFromLink will be set when called as result of cursor linking(see Cursor.setCursorPos for info)
  */
-BaseView.prototype.setCursorPos = function(cursorPos) {
+BaseView.prototype.setCursorPos = function(cursorPos, isFromLink = false) {
   if (this.isDisposed()) {
     return;
   }
   if (!this._isLoading.peek()) {
-    this.cursor.setCursorPos(cursorPos);
+    this.cursor.setCursorPos(cursorPos, isFromLink);
   } else {
     // This is the first step; the second happens in onTableLoaded.
     this._pendingCursorPos = cursorPos;

app/client/components/Cursor.ts

@@ -16,6 +16,26 @@ function nullAsUndefined<T>(value: T|null|undefined): T|undefined {
   return value == null ? undefined : value;
 }
 
+// ================ SequenceNum: used to keep track of cursor edits (lastEditedAt)
+// Basically just a global auto-incrementing counter, with some types to make intent more clear
+// Cursors are constructed at SequenceNEVER (0). After that, changes to their sequenceNum will go through
+// 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; // Cursors will start here
+let latestGlobalSequenceNum = SequenceNEVER;
+function nextSequenceNum() { // First call to this func should return 1
+  latestGlobalSequenceNum++;
+  return latestGlobalSequenceNum;
+}
+
+// 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
+
+
 /**
  * Cursor represents the location of the cursor in the viewsection. It is maintained by BaseView,
  * and implements the shared functionality related to the cursor cell.
@@ -63,6 +83,14 @@ export class Cursor extends Disposable {
 
   private _properRowId: ko.Computed<UIRowId|null>;
 
+  // lastEditedAt is updated on _properRowId or fieldIndex update (including through setCursorPos)
+  // Used to determine which section takes priority for cursorLinking (specifically cycles/bidirectional linking)
+  private _lastEditedAt: ko.Observable<SequenceNum>;
+  // _silentUpdatesFlag prevents lastEditedAt from being updated, when a change in cursorPos isn't driven by the user.
+  // It's used when cursor linking calls setCursorPos, so that linked cursor moves don't trample lastEditedAt.
+  // WARNING: the flag approach relies on ko observables being resolved synchronously, may break if changed to grainjs?
+  private _silentUpdatesFlag: boolean = false;
+
   constructor(baseView: BaseView, optCursorPos?: CursorPos) {
     super();
     optCursorPos = optCursorPos || {};
@@ -83,6 +111,7 @@ export class Cursor extends Disposable {
     }));
 
     this.fieldIndex = baseView.viewSection.viewFields().makeLiveIndex(optCursorPos.fieldIndex || 0);
+
     this.autoDispose(commands.createGroup(Cursor.editorCommands, this, baseView.viewSection.hasFocus));
 
     // RowId might diverge from the one stored in _rowId when the data changes (it is filtered out). So here
@@ -93,8 +122,22 @@ export class Cursor extends Disposable {
       return rowId;
     }));
 
-    // Update the section's activeRowId when the cursor's rowIndex is changed.
+    this._lastEditedAt = ko.observable(SequenceNEVER);
+
+    // update the section's activeRowId and lastCursorEdit when needed
     this.autoDispose(this._properRowId.subscribe((rowId) => baseView.viewSection.activeRowId(rowId)));
+    this.autoDispose(this._lastEditedAt.subscribe((seqNum) => baseView.viewSection.lastCursorEdit(seqNum)));
+
+    // Update the cursor edit time if either the row or column change
+    // 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(); }));
 
     // On dispose, save the current cursor position to the section model.
     this.onDispose(() => { baseView.viewSection.lastCursorPos = this.getCursorPos(); });
@@ -116,23 +159,65 @@ export class Cursor extends Disposable {
   /**
    * Moves the cursor to the given position. Only moves the row if rowId or rowIndex is valid,
    * preferring rowId.
+   *
+   * isFromLink prevents lastEditedAt from being updated, so lastEdit reflects only user-driven edits
    * @param cursorPos: Position as { rowId?, rowIndex?, fieldIndex? }, as from getCursorPos().
+   * @param isFromLink: should be set if this is a cascading update from cursor-linking
    */
-  public setCursorPos(cursorPos: CursorPos): void {
+  public setCursorPos(cursorPos: CursorPos, isFromLink: boolean = false): void {
+
+    try {
+      // If updating as a result of links, we want to NOT update lastEditedAt
+      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);
       }
+
+      // 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, 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
+      // 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();
+
+    } finally { // Make sure we reset this even on error
+      this._silentUpdatesFlag = false;
     }
 
+  }
+
+
+
+
   public setLive(isLive: boolean): void {
     this._isLive(isLive);
   }
+
+  // Should be called whenever the cursor is updated
+  // 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 {
+    // If updating as a result of links, we want to NOT update lastEdited
+    if (!this._silentUpdatesFlag)
+      { this._lastEditedAt(nextSequenceNum()); }
+  }
 }

app/client/components/GristDoc.ts

@@ -1133,7 +1133,8 @@ export class GristDoc extends DisposableWithEvents {
   public async recursiveMoveToCursorPos(
     cursorPos: CursorPos,
     setAsActiveSection: boolean,
-    silent: boolean = false): Promise<boolean> {
+    silent: boolean = false,
+    visitedSections: number[] = []): Promise<boolean> {
     try {
       if (!cursorPos.sectionId) {
         throw new Error('sectionId required');
@@ -1145,6 +1146,12 @@ export class GristDoc extends DisposableWithEvents {
       if (!section.id.peek()) {
         throw new Error(`Section ${cursorPos.sectionId} does not exist`);
       }
+
+      if (visitedSections.includes(section.id.peek())) {
+        // We've already been here (we hit a cycle), just return immediately
+        return true;
+      }
+
       const srcSection = section.linkSrcSection.peek();
       if (srcSection.id.peek()) {
         // We're in a linked section, so we need to recurse to make sure the row we want
@@ -1194,7 +1201,7 @@ export class GristDoc extends DisposableWithEvents {
         await this.recursiveMoveToCursorPos({
           rowId: srcRowId,
           sectionId: srcSection.id.peek(),
-        }, false, silent);
+        }, false, silent, visitedSections.concat([section.id.peek()]));
       }
       const view: ViewRec = section.view.peek();
       const docPage: ViewDocPage = section.isRaw.peek() ? "data" : view.getRowId();

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';
@@ -51,7 +52,12 @@ export const EmptyFilterColValues: FilterColValues = FilterStateToColValues(Empt
 export class LinkingState extends Disposable {
   // If linking affects target section's cursor, this will be a computed for the cursor rowId.
   // Is undefined if not cursor-linked
-  public readonly cursorPos?: ko.Computed<UIRowId>;
+  public readonly cursorPos?: ko.Computed<UIRowId|null>;
+
+  // Cursor-links can be cyclic, need to keep track of both rowId and the lastCursorEdit that it came from to
+  // resolve it correctly, (use just one observable so they update at the same time)
+  //NOTE: observables don't do deep-equality check, so need to replace the whole array when updating
+  public readonly incomingCursorPos: ko.Computed<[UIRowId|null, SequenceNum]>;
 
   // If linking affects filtering, this is a computed for the current filtering state, including user-facing
   // labels for filter values and types of the filtered columns
@@ -194,13 +200,120 @@ export class LinkingState extends Disposable {
       //        either same-table cursor-link (!srcCol && !tgtCol, so do activeRowId -> cursorPos)
       //        or cursor-link by reference   ( srcCol && !tgtCol, so do srcCol -> cursorPos)
 
-      //colVal, or rowId if no srcCol
+      // Cursor linking notes:
+      //
+      // If multiple viewSections are cursor-linked together A->B->C, we need to propagate the linked cursorPos along.
+      // The old way was to have: A.activeRowId -> (sets by cursor-link) -> B.activeRowId, and so on
+      //                                                                                                               |
+      //                                   -->  [B.LS]                    --> [C.LS]                                   |
+      //                                  /        | B.LS.cursorPos      /       | C.LS.cursorPos                      |
+      //                                 /         v                    /        v                                     |
+      //                   [ 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 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
+      //
+      // Normally this wouldn't be too bad, but to implement bidirectional linking requires allowing cycles of
+      // cursor-links, in which case this behavior becomes extra-problematic, both in being more unexpected from a UX
+      // perspective and because a section will eventually be linked to itself, which is an unstable loop.
+      //
+      // A better solution is to propagate the linked rowId directly through the chain of linkingStates without passing
+      // through the activeRowIds of the sections, so whether a section is filtered or not doesn't affect propagation.
+      //
+      //                                                B.LS.incCursPos                                                |
+      //                                 -->  [B.LS]   -------------->   [C.LS]                                        |
+      //                                /        |                          |                                          |
+      //                               /         v B.LS.cursorPos           v C.LS.cursorPos                           |
+      //                 [ A ]--------/        [ B ]                      [ C ]                                        |
+      //                      A.actRowId                                                                               |
+      //
+      // If the previous section has a linkingState, we use the previous LS's incomingCursorPos
+      // (i.e. two sections back) instead of looking at our srcSection's activeRowId. This way it doesn't matter how
+      // section B is filtered, since we're getting our cursorPos straight from A (through a computed in B.LS)
+      //
+      // However, each linkingState needs to decide whether to use the cursorPos from the srcSec (i.e. its activeRowId),
+      // or to use the previous linkState's incomingCursorPos. We want to use whichever section the user most recently
+      // interacted with, i.e. whichever cursor update was most recent. For this we use, the cursor version (given in
+      // 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.
+      //
+      // If we do this right, the end result is that because the lastCursorEdits are guaranteed to be unique,
+      // there is always a stable configuration of links, where even in the case of a cycle the incomingCursorPos-es
+      // 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                    |
+      //                               |                 |                /       |                                    |
+      //                               v                 v               /        v                                    |
+      //                             [ A ]             [ B ]   ---------/       [ C ]                                  |
+      //                                          (most recently edited)                                               |
+      //
+      // 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
+      // 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
       const srcValueFunc = this._makeValGetter(this._srcSection.table(), this._srcColId);
 
-      if (srcValueFunc) { // if makeValGetter succeeded, set up cursorPos
+      // check for failure
-        this.cursorPos = this.autoDispose(ko.computed(() =>
+      if (srcValueFunc) {
-          srcValueFunc(srcSection.activeRowId()) as UIRowId
+        //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(() => {
+          // NOTE: This computed primarily decides between srcSec and prevLink. Here's what those mean:
+          // e.g. consider sections A->B->C, (where this === C)
+          // We need to decide between taking cursor info from B, our srcSection (1 hop back)
+          //    vs taking cursor info from further back, e.g. A, or before (2+ hops back)
+          // To take cursor info from further back, we rely on B's linkingState, since B's linkingState will
+          //    be looking at the preceding sections, either A or whatever is behind A.
+          // Therefore: we either use srcSection (1 back), or prevLink = srcSection.linkingState (2+ back)
+
+          // Get srcSection's info (1 hop back)
+          const srcSecPos = this._srcSection.activeRowId.peek(); //we don't depend on this, only on its cursor version
+          const srcSecVersion = this._srcSection.lastCursorEdit();
+
+          // If cursors haven't been initialized, cursor-linking doesn't make sense, so don't do it
+          if(srcSecVersion === SequenceNEVER) {
+            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
+          //                                        outgoing link), AND the link cycle has come all the way around
+          const usePrev = prevLinkHasCursor && prevLinkedVersion > srcSecVersion;
+
+          // srcSec/prevLinkedPos is rowId from srcSec. However if "Cursor:Reference", we must follow the ref in srcCol
+          // srcValueFunc will get the appropriate value based on this._srcColId if that's the case
+          const tgtCursorPos = (srcValueFunc(usePrev ? prevLinkedPos : srcSecPos) || "new") as UIRowId;
+          // NOTE: srcValueFunc returns 'null' if rowId is the add-row, so we coerce that back into || "new"
+          // NOTE: cursor linking is only ever done by the id column (for same-table) or by single Ref col (cursor:ref),
+          //     so we'll never have to worry about `null` showing up as an actual cell-value. (A blank Ref is just `0`)
+
+          return [
+              tgtCursorPos,
+              usePrev ? prevLinkedVersion : srcSecVersion, //propagate which version our cursorPos is from
+          ] as [UIRowId|null, SequenceNum];
+        })));
+
+        // Pull out just the rowId from incomingCursor Pos
+        // (This get applied directly to tgtSection's cursor),
+        this.cursorPos = this.autoDispose(ko.computed(() => this.incomingCursorPos()[0]));
       }
 
       if (!srcColId) { // If same-table cursor-link, copy getDefaultColValues from the source if possible
@@ -210,6 +323,8 @@ export class LinkingState extends Disposable {
         }
       }
     }
+    // ======= End of cursor linking
+
 
     // Make filterColValues, which is just the filtering-relevant parts of filterState
     // (it's used in places that don't need the user-facing labels, e.g. CSV export)

app/client/models/entities/ViewSectionRec.ts

@@ -1,4 +1,5 @@
 import BaseView from 'app/client/components/BaseView';
+import {SequenceNEVER, SequenceNum} from 'app/client/components/Cursor';
 import {EmptyFilterColValues, LinkingState} from 'app/client/components/LinkingState';
 import {KoArray} from 'app/client/lib/koArray';
 import {ColumnToMapImpl} from 'app/client/models/ColumnToMap';
@@ -155,6 +156,8 @@ export interface ViewSectionRec extends IRowModel<"_grist_Views_section">, RuleO
 
   activeRowId: ko.Observable<UIRowId | null>;     // May be null when there are no rows.
 
+  lastCursorEdit: ko.Observable<SequenceNum>;
+
   // If the view instance for section is instantiated, it will be accessible here.
   viewInstance: ko.Observable<BaseView | null>;
 
@@ -627,6 +630,7 @@ export function createViewSectionRec(this: ViewSectionRec, docModel: DocModel):
   this.linkTargetCol = refRecord(docModel.columns, this.linkTargetColRef);
 
   this.activeRowId = ko.observable<UIRowId|null>(null);
+  this.lastCursorEdit = ko.observable<SequenceNum>(SequenceNEVER);
 
   this._linkingState = Holder.create(this);
   this.linkingState = this.autoDispose(ko.pureComputed(() => {

app/client/ui/RightPanel.ts

@@ -643,7 +643,13 @@ export class RightPanel extends Disposable {
       const lstate = use(tgtSec.linkingState);
       const lfilter = lstate?.filterState ? use(lstate.filterState) : undefined;
 
-      const cursorPosStr = lstate?.cursorPos ? `${tgtSec.tableId()}[${use(lstate.cursorPos)}]` : "N/A";
+      // Debug info for cursor linking
+      const inPos = lstate?.incomingCursorPos ? use(lstate.incomingCursorPos) : null;
+      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)}` +
+        `\n incomingCursorPos: ${inPos ? `${inPos[0]}@T+${inPos[1]}` : "N/A"}`;
 
       //Main link info as a big string, will be in a <pre></pre> block
       let preString = "No Incoming Link";

app/client/ui/selectBy.ts

@@ -49,8 +49,15 @@ interface LinkNode {
   groupbyColumns?: Set<number>;
 
   // list of ids of the sections that are ancestors to this section according to the linked section
-  // relationship
+  // relationship. ancestors[0] is this.section, ancestors[length-1] is oldest ancestor
-  ancestors: Set<number>;
+  ancestors: number[];
+
+  // For bidirectional linking, cycles are only allowed if all links on that cycle are same-table cursor-link
+  // this.ancestors only records what the ancestors are, but we need to record info about the edges between them.
+  // 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.
   section: ViewSectionRec;
@@ -141,11 +148,53 @@ function isValidLink(source: LinkNode, target: LinkNode) {
     }
   }
 
-  // The link must not create a cycle
+  // The link must not create a cycle, unless it's only same-table cursor-links all the way to target
-  if (source.ancestors.has(target.section.getRowId())) {
+  if (source.ancestors.includes(target.section.getRowId())) {
+
+    // cycles only allowed for cursor links
+    if (source.column || target.column || source.isSummary) {
       return false;
     }
 
+    // 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()) {
+        break; // Success!
+      }
+
+      // 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
+  }
+
   return true;
 }
 
@@ -224,15 +273,30 @@ function fromViewSectionRec(section: ViewSectionRec): LinkNode[] {
     return [];
   }
   const table = section.table.peek();
-  const ancestors = new Set<number>();
+  const ancestors: number[] = [];
+
+  const isAncestorSameTableCursorLink: boolean[] = [];
 
   for (let sec = section; sec.getRowId(); sec = sec.linkSrcSection.peek()) {
-    if (ancestors.has(sec.getRowId())) {
+    if (ancestors.includes(sec.getRowId())) {
-      // tslint:disable-next-line:no-console
+      // There's a cycle in the existing link graph
-      console.warn(`Links should not create a cycle - section ids: ${Array.from(ancestors)}`);
+      // TODO if we're feeling fancy, can test here whether it's an all-same-table cycle and warn if not
+      //      but there's other places we check for that
       break;
     }
-    ancestors.add(sec.getRowId());
+    ancestors.push(sec.getRowId());
+
+    //Determine if this link is a same-table cursor link
+    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);
+    }
+    // NOTE: isAncestorSameTableCursorLink may be 1 shorter than ancestors, since we might skip pushing
+    // 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();
@@ -243,6 +307,7 @@ function fromViewSectionRec(section: ViewSectionRec): LinkNode[] {
     groupbyColumns: isSummary ? table.summarySourceColRefs.peek() : undefined,
     widgetType: section.parentKey.peek(),
     ancestors,
+    isAncestorSameTableCursorLink,
     section,
   };
 
@@ -280,7 +345,8 @@ function fromPageWidget(docModel: DocModel, pageWidget: IPageWidget): LinkNode[]
     // (e.g.: link from summary table with Attachments in group-by) but it seems to work fine as is
     groupbyColumns,
     widgetType: pageWidget.type,
-    ancestors: new Set(),
+    ancestors: [],
+    isAncestorSameTableCursorLink: [],
     section: docModel.viewSections.getRowModel(pageWidget.section),
   };
 

test/nbrowser/RightPanelSelectBy.ts

@@ -63,9 +63,23 @@ describe('RightPanelSelectBy', function() {
   });
 
 
-  it('should disallow creating cycles', async function() {
+  it('should disallow creating cycles if not cursor-linked', async function() {
+
+    //Link "films record" by "performances record"
+    await gu.openSelectByForSection('FILMS RECORD');
+    await driver.findContent('.test-select-row', /Performances record.*Film/i).click();
+    await gu.waitForServer();
+
+    // this link should no longer be present, since it would create a cycle with a filter link in it
     await gu.openSelectByForSection('PERFORMANCES RECORD');
-    assert.equal(await driver.findContent('.test-select-row', /Performances detail/).isPresent(), false);
+    assert.equal(await driver.findContent('.test-select-row', /Performances record.*Film/i).isPresent(), false);
+  });
+
+  it('should allow creating cursor-linked-cycles', async function() {
+    assert.equal(await driver.findContent('.test-select-row', /Performances detail/).isPresent(), true);
+
+    // undo, the operation from the previous test; link is expected to be unset for next test
+    await gu.undo();
   });