Agent: soften panic assertions (#4291)

sourcegraph · May 24, 2024 · 0e6b7dc · 0e6b7dc
1 parent 468478c
commit 0e6b7dc
Show file tree

Hide file tree

Showing 3 changed files with 59 additions and 24 deletions.
diff --git a/agent/src/panicWhenClientIsOutOfSync.ts b/agent/src/panicWhenClientIsOutOfSync.ts
@@ -4,6 +4,16 @@ import type { ProtocolTextDocument } from './protocol-alias'
 import { renderUnifiedDiff } from './renderUnifiedDiff'
 import { protocolRange, vscodeRange } from './vscode-type-converters'
 
+// Allows the client to send a "source of truth" document that reflects the
+// client's current state.  Should only be used during testing (or local
+// development) to ensure that the document state on the client and server match
+// each other. This function exits the process on the first mis-match by
+// default, unless a custom `doPanic` function is provided (only used for
+// testing this function).
+// The motivation to create this function was that we've spent quite a while
+// debugging document synchronization bugs in the JetBrains plugin. These bugs
+// can be very difficult to debug, esp. when looking at raw logs of
+// line/character numbers.
 export function panicWhenClientIsOutOfSync(
     mostRecentlySentClientDocument: ProtocolTextDocument,
     serverEditor: AgentTextEditor,
@@ -12,41 +22,47 @@ export function panicWhenClientIsOutOfSync(
     const serverDocument = serverEditor.document
     if (mostRecentlySentClientDocument.testing?.sourceOfTruthDocument) {
         const clientSourceOfTruthDocument = mostRecentlySentClientDocument.testing.sourceOfTruthDocument
-        if (clientSourceOfTruthDocument.content !== serverDocument.content) {
+
+        if (
+            typeof clientSourceOfTruthDocument.content === 'string' && // Skip content assertion if the client doesn't send content
+            clientSourceOfTruthDocument.content !== serverDocument.content
+        ) {
             const diff = renderUnifiedDiff(
                 {
                     header: `${clientSourceOfTruthDocument.uri} (client side)`,
                     text: clientSourceOfTruthDocument.content ?? '',
                 },
                 {
-                    header: `${clientSourceOfTruthDocument.uri} (server side)`,
+                    header: `${serverDocument.uri} (server side)`,
                     text: serverDocument.content ?? '',
                 }
             )
             params.doPanic(diff)
         }
 
-        const clientCompareObject = {
-            selection: clientSourceOfTruthDocument.selection,
-            // Ignoring visibility for now. It was causing low-priority panics
-            // when we were still debugging higher-priority content/selection
-            // bugs.
-        }
-        const serverCompareObject = {
-            selection: protocolRange(serverEditor.selection),
-        }
-        if (!isEqual(clientCompareObject, serverCompareObject)) {
-            const diff = renderUnifiedDiff(
-                {
-                    header: `${clientSourceOfTruthDocument.uri} (client side)`,
-                    text: JSON.stringify(clientCompareObject, null, 2),
-                },
-                {
-                    header: `${clientSourceOfTruthDocument.uri} (server side)`,
-                    text: JSON.stringify(serverCompareObject, null, 2),
-                }
-            )
-            params.doPanic(diff)
+        if (typeof clientSourceOfTruthDocument.selection === 'object') {
+            const clientCompareObject = {
+                selection: clientSourceOfTruthDocument.selection,
+                // Ignoring visibility for now. It was causing low-priority panics
+                // when we were still debugging higher-priority content/selection
+                // bugs.
+            }
+            const serverCompareObject = {
+                selection: protocolRange(serverEditor.selection),
+            }
+            if (!isEqual(clientCompareObject, serverCompareObject)) {
+                const diff = renderUnifiedDiff(
+                    {
+                        header: `${clientSourceOfTruthDocument.uri} (client side)`,
+                        text: JSON.stringify(clientCompareObject, null, 2),
+                    },
+                    {
+                        header: `${serverDocument.uri} (server side)`,
+                        text: JSON.stringify(serverCompareObject, null, 2),
+                    }
+                )
+                params.doPanic(diff)
+            }
         }
     }
 

diff --git a/agent/src/renderUnifiedDiff.test.ts b/agent/src/renderUnifiedDiff.test.ts
@@ -71,4 +71,18 @@ describe('renderUnifiedDiff', () => {
             i"
         `)
     })
+
+    it('trailing whitespace', () => {
+        expect(
+            diff(['a ', 'b  ', 'c   '].join('\n'), ['a ', 'b ', 'c '].join('\n'))
+        ).toMatchInlineSnapshot(`
+          "--- a
+          +++ b
+            a␣
+          - b␣␣
+          - c␣␣␣
+          + b␣
+          + c␣"
+        `)
+    })
 })
diff --git a/agent/src/renderUnifiedDiff.ts b/agent/src/renderUnifiedDiff.ts
@@ -12,7 +12,12 @@ export function renderUnifiedDiff(
         // TODO: may want to limit the context size. We currently show all lines
         // including the ones that have no diff.
         for (const part of parts) {
-            lines.push(prefix + part)
+            // Replace trailing white characters with '␣' to make it easier to
+            // debug whitespace diffs.
+            const withHighlightedTrailingWhitespace = part.replace(/(\s+)$/, match =>
+                '␣'.repeat(match.length)
+            )
+            lines.push(prefix + withHighlightedTrailingWhitespace)
         }
     }
     return lines.join('\n')