diff --git a/app/client/components/BaseView.js b/app/client/components/BaseView.js index 445c1700..585c73f5 100644 --- a/app/client/components/BaseView.js +++ b/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; diff --git a/app/client/components/Cursor.ts b/app/client/components/Cursor.ts index ad35bcc7..6df688cb 100644 --- a/app/client/components/Cursor.ts +++ b/app/client/components/Cursor.ts @@ -16,6 +16,26 @@ function nullAsUndefined(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; + // 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; + // _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 { - if (cursorPos.rowId !== undefined && this.viewData.getRowIndex(cursorPos.rowId) >= 0) { - 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); + 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)); + } 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()); } + } } diff --git a/app/client/components/GristDoc.ts b/app/client/components/GristDoc.ts index fc93cabf..45c4b641 100644 --- a/app/client/components/GristDoc.ts +++ b/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 { + silent: boolean = false, + visitedSections: number[] = []): Promise { 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(); diff --git a/app/client/components/LinkingState.ts b/app/client/components/LinkingState.ts index ef532816..115e127a 100644 --- a/app/client/components/LinkingState.ts +++ b/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; + public readonly cursorPos?: ko.Computed; + + // 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 - this.cursorPos = this.autoDispose(ko.computed(() => - srcValueFunc(srcSection.activeRowId()) as UIRowId - )); + // check for failure + if (srcValueFunc) { + //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) diff --git a/app/client/models/entities/ViewSectionRec.ts b/app/client/models/entities/ViewSectionRec.ts index ac671298..f7f1072d 100644 --- a/app/client/models/entities/ViewSectionRec.ts +++ b/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; // May be null when there are no rows. + lastCursorEdit: ko.Observable; + // If the view instance for section is instantiated, it will be accessible here. viewInstance: ko.Observable; @@ -627,6 +630,7 @@ export function createViewSectionRec(this: ViewSectionRec, docModel: DocModel): this.linkTargetCol = refRecord(docModel.columns, this.linkTargetColRef); this.activeRowId = ko.observable(null); + this.lastCursorEdit = ko.observable(SequenceNEVER); this._linkingState = Holder.create(this); this.linkingState = this.autoDispose(ko.pureComputed(() => { diff --git a/app/client/ui/RightPanel.ts b/app/client/ui/RightPanel.ts index a4d887c3..27232392 100644 --- a/app/client/ui/RightPanel.ts +++ b/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 
 block
       let preString = "No Incoming Link";
diff --git a/app/client/ui/selectBy.ts b/app/client/ui/selectBy.ts
index a6664781..2c754d00 100644
--- a/app/client/ui/selectBy.ts
+++ b/app/client/ui/selectBy.ts
@@ -49,8 +49,15 @@ interface LinkNode {
   groupbyColumns?: Set;
 
   // list of ids of the sections that are ancestors to this section according to the linked section
-  // relationship
-  ancestors: Set;
+  // relationship. ancestors[0] is this.section, ancestors[length-1] is oldest ancestor
+  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,9 +148,51 @@ function isValidLink(source: LinkNode, target: LinkNode) {
     }
   }
 
-  // The link must not create a cycle
-  if (source.ancestors.has(target.section.getRowId())) {
-    return false;
+  // 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
+    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();
+  const ancestors: number[] = [];
+
+  const isAncestorSameTableCursorLink: boolean[] = [];
 
   for (let sec = section; sec.getRowId(); sec = sec.linkSrcSection.peek()) {
-    if (ancestors.has(sec.getRowId())) {
-      // tslint:disable-next-line:no-console
-      console.warn(`Links should not create a cycle - section ids: ${Array.from(ancestors)}`);
+    if (ancestors.includes(sec.getRowId())) {
+      // There's a cycle in the existing link graph
+      // 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),
   };
 
diff --git a/test/nbrowser/RightPanelSelectBy.ts b/test/nbrowser/RightPanelSelectBy.ts
index 1cc6df16..e53a98ae 100644
--- a/test/nbrowser/RightPanelSelectBy.ts
+++ b/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();
   });