Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

src/test/java/com/github/_1c_syntax/bsl/languageserver/types/index/WorkspaceSymbolIndexTest.java (1)

213-219: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

This cancellation test still fails on the first check, not the periodic in-loop check path.

Because CancelChecker always throws, the assertion succeeds at the initial search cancellation check and does not validate loop-time cancellation behavior.

✅ Proposed fix

@@
-    CancelChecker cancelChecker = () -> {
-      throw new CancellationException();
-    };
+    var checks = new java.util.concurrent.atomic.AtomicInteger();
+    CancelChecker cancelChecker = () -> {
+      if (checks.incrementAndGet() > 1) {
+        throw new CancellationException();
+      }
+    };

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@src/test/java/com/github/_1c_syntax/bsl/languageserver/types/index/WorkspaceSymbolIndexTest.java`
around lines 213 - 219, The test currently supplies a CancelChecker that always
throws, so the initial pre-loop cancellation check in index.search triggers and
the loop-path cancellation behavior isn't exercised; change the CancelChecker
used in WorkspaceSymbolIndexTest so its first invocation does not throw but a
subsequent invocation does (e.g., track invocations with an AtomicBoolean or
counter and throw a CancellationException only after the first call), then keep
the existing assertThatThrownBy(() -> index.search("Символ",
cancelChecker)).isInstanceOf(CancellationException.class) to verify the in-loop
cancellation path.

src/main/java/com/github/_1c_syntax/bsl/languageserver/types/index/WorkspaceSymbolIndex.java (1)

172-174: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Group updates by key to avoid repeated list rewrites during clear/reindex.

This is still doing per-entry remove/merge against the same key, which causes repeated List.copyOf churn on large same-name groups. Group by lowerName and update each key once.

⚙️ Proposed fix

@@
-      for (var entry : removed) {
-        removeEntryFromKey(entry.lowerName(), removedSet);
-      }
+      var removedByKey = new java.util.HashMap<String, Set<Entry>>();
+      for (var entry : removed) {
+        removedByKey
+          .computeIfAbsent(entry.lowerName(), k -> Collections.newSetFromMap(new IdentityHashMap<>()))
+          .add(entry);
+      }
+      for (var e : removedByKey.entrySet()) {
+        removeEntryFromKey(e.getKey(), e.getValue());
+      }
@@
-      for (var entry : collected) {
-        trie.merge(entry.lowerName(), List.of(entry), WorkspaceSymbolIndex::concat);
-      }
+      var grouped = new java.util.HashMap<String, List<Entry>>();
+      for (var entry : collected) {
+        grouped.merge(entry.lowerName(), List.of(entry), WorkspaceSymbolIndex::concat);
+      }
+      grouped.forEach((key, entries) -> trie.merge(key, entries, WorkspaceSymbolIndex::concat));

Also applies to: 305-307

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@src/main/java/com/github/_1c_syntax/bsl/languageserver/types/index/WorkspaceSymbolIndex.java`
around lines 172 - 174, The loop that calls removeEntryFromKey for each entry
causes repeated list rewrites; instead, group the entries in removed by their
lowerName and call removeEntryFromKey once per key with the grouped set (i.e.,
build a Map<String, Set<Entry>> or Map<String, Set<...>> keyed by
entry.lowerName(), then iterate that map and call removeEntryFromKey(key,
groupedSet)); apply the same refactor to the other occurrence in
WorkspaceSymbolIndex where entries are iterated and removeEntryFromKey is called
so each key is updated only once.

src/test/java/com/github/_1c_syntax/bsl/languageserver/types/index/WorkspaceSymbolIndexBenchmarkTest.java (1)

55-83: ⚡ Quick win

Use a configurable benchmark workspace path instead of a machine-specific absolute path.

The hard-coded local path prevents anyone else from running this benchmark without editing source.

♻️ Proposed refactor

@@
 import org.junit.jupiter.api.Disabled;
+import org.junit.jupiter.api.Assumptions;
 import org.junit.jupiter.api.Test;
@@
-  private static final String PATH_TO_CONFIGURATION = "/home/nfedkin/git_tree/github/1c-syntax/ssl_3_2";
+  private static final String BENCH_PATH_PROPERTY = "workspaceSymbolIndex.bench.path";
@@
-    var workspaceUri = Absolute.uri(Path.of(PATH_TO_CONFIGURATION).toUri());
+    var pathToConfiguration = System.getProperty(BENCH_PATH_PROPERTY);
+    Assumptions.assumeTrue(pathToConfiguration != null && !pathToConfiguration.isBlank(),
+      () -> "Set -D" + BENCH_PATH_PROPERTY + "=/path/to/workspace before running benchmark");
+    var workspaceUri = Absolute.uri(Path.of(pathToConfiguration).toUri());

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@src/test/java/com/github/_1c_syntax/bsl/languageserver/types/index/WorkspaceSymbolIndexBenchmarkTest.java`
around lines 55 - 83, Replace the machine-specific PATH_TO_CONFIGURATION
constant with a configurable lookup (e.g., read
System.getProperty("benchmark.workspace.path") or an environment variable with a
sensible project-relative default) and update benchmarkSearch to use that value
when building workspaceUri for serverContextProvider.addWorkspace; specifically,
remove the hard-coded "/home/..." string in the PATH_TO_CONFIGURATION
definition, implement a method or inline logic to resolve the path from
System.getProperty/System.getenv (fallback to a relative path inside the repo or
test-resources), and ensure benchmarkSearch uses the new configurable
constant/reader so the benchmark can run on other machines without source edits.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

src/main/java/com/github/_1c_syntax/bsl/languageserver/providers/SymbolProvider.java (1)

84-93: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Avoid leaking mutable cached objects into WorkspaceSymbol.

entry.range() and entry.tags() are passed by reference. Mutating the returned WorkspaceSymbol can feed back into shared indexed data and skew later results.

Proposed minimal fix

 import org.eclipse.lsp4j.Location;
+import org.eclipse.lsp4j.Position;
+import org.eclipse.lsp4j.Range;
@@
   private static WorkspaceSymbol createWorkspaceSymbol(Entry entry) {
-    var location = new Location(entry.uri().toString(), entry.range());
+    var location = new Location(entry.uri().toString(), copyRange(entry.range()));
@@
-    workspaceSymbol.setTags(entry.tags());
+    workspaceSymbol.setTags(List.copyOf(entry.tags()));
@@
     return workspaceSymbol;
   }
+
+  private static Range copyRange(Range range) {
+    return new Range(
+      new Position(range.getStart().getLine(), range.getStart().getCharacter()),
+      new Position(range.getEnd().getLine(), range.getEnd().getCharacter())
+    );
+  }

In Eclipse LSP4J, are `org.eclipse.lsp4j.Range` and `WorkspaceSymbol` field objects mutable, and is defensive copying recommended when mapping from shared cache objects?

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@src/main/java/com/github/_1c_syntax/bsl/languageserver/providers/SymbolProvider.java`
around lines 84 - 93, createWorkspaceSymbol is leaking mutable cached objects by
passing entry.range() and entry.tags() directly into the WorkspaceSymbol; to fix
it, defensively copy those objects before setting them: construct a new Range
(copying start/end positions from entry.range()) and use that new Range when
creating the Location for setLocation, and setTags with a new List copy (e.g.,
new ArrayList<>(entry.tags())) or an unmodifiable copy instead of entry.tags()
directly; keep the containerName check as-is and handle possible null tags/range
gracefully.

src/test/java/com/github/_1c_syntax/bsl/languageserver/types/index/WorkspaceSymbolIndexTest.java (1)

286-302: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

The cancellation test still does not exercise periodic in-loop checks.

The CancelChecker at lines 295-297 throws CancellationException immediately on the first checkCanceled() call. This prevents validating that index.search() performs periodic cancellation checks during iteration (not just an initial check before work begins). The test currently only confirms that CancellationException propagates, but does not verify that the search method calls checkCanceled() repeatedly while processing the 2000 symbols.

The suggested fix from the previous review (allowing the first N checks to pass by using an AtomicInteger counter) does not appear to be implemented in the current code.

✅ Suggested change to test periodic checks

+    var checks = new java.util.concurrent.atomic.AtomicInteger();
     CancelChecker cancelChecker = () -> {
-      throw new CancellationException();
+      if (checks.incrementAndGet() > 5) {
+        throw new CancellationException();
+      }
     };

As per coding guidelines, "Always run tests before submitting changes and maintain or improve test coverage using appropriate test frameworks (JUnit, AssertJ, Mockito)".

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@src/test/java/com/github/_1c_syntax/bsl/languageserver/types/index/WorkspaceSymbolIndexTest.java`
around lines 286 - 302, The CancelChecker lambda in the
cancelledCheckerInterruptsSearchEventually test method throws
CancellationException immediately, which only validates that the exception
propagates but does not verify that index.search() actually performs periodic
cancellation checks during iteration through the 2000 symbols. Replace the
immediate throw with an AtomicInteger counter that tracks calls to the cancel
checker. Allow the first several calls to pass (so the search begins processing)
and then throw CancellationException on a subsequent call to verify that the
search method truly calls checkCanceled() repeatedly during iteration, not just
once before starting work.

Source: Coding guidelines

src/test/java/com/github/_1c_syntax/bsl/languageserver/types/index/WorkspaceSymbolIndexBenchmarkTest.java (1)

145-180: ⚡ Quick win

Validate that flat scan and trie produce the same result count.

The benchmark compares latency of two subsequence-matching implementations but does not verify they produce identical results. Line 160 uses countTrie for logging, but a mismatch between countFlatScan and countTrie would go undetected. Adding a simple assertion after warmup ensures both methods enumerate the same candidate set.

✅ Proposed validation

     for (var query : SUBSEQUENCE_QUERIES) {
       var lowerQuery = query.toLowerCase(Locale.ENGLISH);
 
       for (var i = 0; i < WARMUP_ITERATIONS; i++) {
         countFlatScan(entries, lowerQuery);
         countTrie(trie, lowerQuery);
       }
 
-      var matches = countTrie(trie, lowerQuery);
+      var flatMatches = countFlatScan(entries, lowerQuery);
+      var trieMatches = countTrie(trie, lowerQuery);
+      if (flatMatches != trieMatches) {
+        throw new AssertionError(String.format(
+          "Subsequence mismatch for query=\"%s\": flat=%d trie=%d",
+          query, flatMatches, trieMatches));
+      }
+      var matches = trieMatches;

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@src/test/java/com/github/_1c_syntax/bsl/languageserver/types/index/WorkspaceSymbolIndexBenchmarkTest.java`
around lines 145 - 180, The benchmarkSubsequenceEnumeration method compares
performance between countFlatScan and countTrie but does not verify they produce
identical results. After the warmup loop completes (after the loop at line
151-154 that calls both countFlatScan and countTrie), add an assertion that
validates both methods return the same count for the current lowerQuery. Capture
the result from countFlatScan and compare it to the result from countTrie to
ensure the benchmark is comparing equivalent implementations.