Обработка событий workspace/didChangeWatchedFiles#3628
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds async processing for file-system watch events in BSLWorkspaceService, introducing per-event handlers that add/rebuild/remove DocumentContext entries, publishes a ServerContextDocumentRemovedEvent via AOP, exposes ServerContext.isDocumentOpened, and adds index/annotation cleanup handlers plus tests and documentation formatting changes. Changes
Sequence Diagram(s)sequenceDiagram
participant Watcher as File Watcher
participant WS as BSLWorkspaceService
participant Exec as executorService
participant SC as ServerContext
participant Bus as ApplicationEventBus
participant Finder as AnnotationReferenceFinder
participant Index as ReferenceIndexFiller
Watcher->>WS: didChangeWatchedFiles(events)
WS->>Exec: submit(process events)
activate Exec
loop per event
alt Deleted
Exec->>SC: getDocument(uri)
Exec->>SC: isDocumentOpened(doc)
alt opened
Exec->>SC: closeDocument(doc)
end
Exec->>SC: removeDocument(uri)
Exec->>Bus: publish(ServerContextDocumentRemovedEvent(uri))
Bus->>Finder: handleServerContextDocumentRemovedEvent(event)
Bus->>Index: handleEvent(event)
else Created/Changed
Exec->>SC: getOrCreateDocument(uri)
Exec->>SC: isDocumentOpened(doc)
alt not opened
Exec->>SC: rebuildFromDisk(uri)
Exec->>SC: clearSecondaryData(uri)
Exec->>Finder: notifyDocumentContextChanged(uri)
Exec->>Index: notifyDocumentChanged(uri)
end
end
end
deactivate Exec
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/main/java/com/github/_1c_syntax/bsl/languageserver/BSLWorkspaceService.java (2)
92-105: Consider adding exception handling for async file event processing.Exceptions thrown within
CompletableFuture.runAsyncwill be silently swallowed unless explicitly handled. This could make debugging issues difficult when file events fail to process.CompletableFuture.runAsync( () -> { for (var fileEvent : params.getChanges()) { - var uri = Absolute.uri(fileEvent.getUri()); - - switch (fileEvent.getType()) { - case Deleted -> handleDeletedFileEvent(uri); - case Created -> handleCreatedFileEvent(uri); - case Changed -> handleChangedFileEvent(uri); + try { + var uri = Absolute.uri(fileEvent.getUri()); + + switch (fileEvent.getType()) { + case Deleted -> handleDeletedFileEvent(uri); + case Created -> handleCreatedFileEvent(uri); + case Changed -> handleChangedFileEvent(uri); + } + } catch (Exception e) { + LOGGER.error("Error processing file event: {}", fileEvent.getUri(), e); } } }, executorService );Note: This would require adding
@Slf4jannotation to the class.
132-136: RedundanttryClearDocumentcall aftercloseDocument.Looking at
ServerContext.closeDocument()(lines 286-290), it already removes the document fromopenedDocumentsand callsclearSecondaryData(). The subsequenttryClearDocument()call will executeclearSecondaryData()a second time unnecessarily.var wasDocumentOpenedBefore = serverContext.isDocumentOpened(documentContext); if (wasDocumentOpenedBefore) { serverContext.closeDocument(documentContext); - serverContext.tryClearDocument(documentContext); } serverContext.removeDocument(uri);src/test/java/com/github/_1c_syntax/bsl/languageserver/BSLWorkspaceServiceTest.java (1)
97-97: Consider using a more explicit pattern for "no change" assertions.The
await().pollDelay(Duration.ofMillis(200)).until(() -> true)pattern is essentially a fixed delay. While pragmatic for verifying "nothing happens," it can be brittle on slow CI systems. Consider adding a brief comment explaining the intent or usingAwaitility.await().during(Duration.ofMillis(200)).atMost(Duration.ofMillis(300)).until(() -> serverContext.getDocument(uri) != null)to make the expectation explicit.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
docs/en/index.md(1 hunks)docs/index.md(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/BSLWorkspaceService.java(5 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/context/ServerContext.java(1 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/BSLWorkspaceServiceTest.java(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.md
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Keep documentation up to date with code changes
Files:
docs/index.mddocs/en/index.md
**/*.java
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.java: Follow the Style Guide provided in docs/en/contributing/StyleGuide.md
Use Lombok annotations to reduce boilerplate code and enable annotation processing in your IDE
Optimize imports before committing but do NOT optimize imports across the entire project unless specifically working on that task
Follow Java naming conventions with meaningful, descriptive names; keep class and method names concise but clear
Write JavaDoc for public APIs and include comments for complex logic
Use Target Java 17 as the language version
Files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/context/ServerContext.javasrc/test/java/com/github/_1c_syntax/bsl/languageserver/BSLWorkspaceServiceTest.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/BSLWorkspaceService.java
**/test/java/**/*.java
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use appropriate test frameworks (JUnit, AssertJ, Mockito) for testing
Files:
src/test/java/com/github/_1c_syntax/bsl/languageserver/BSLWorkspaceServiceTest.java
🧠 Learnings (4)
📚 Learning: 2025-11-27T07:17:33.726Z
Learnt from: CR
Repo: 1c-syntax/bsl-language-server PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-27T07:17:33.726Z
Learning: Applies to **/*.md : Keep documentation up to date with code changes
Applied to files:
docs/index.mddocs/en/index.md
📚 Learning: 2025-11-19T09:02:03.154Z
Learnt from: theshadowco
Repo: 1c-syntax/bsl-language-server PR: 3610
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/folding/QueryPackageFoldingRangeSupplier.java:26-26
Timestamp: 2025-11-19T09:02:03.154Z
Learning: В проекте bsl-language-server используется кастомная версия ANTLR4 от 1c-syntax (io.github.1c-syntax:antlr4), которая включает базовый класс org.antlr.v4.runtime.Tokenizer с методами getAst() и getTokens(). SDBLTokenizer и BSLTokenizer наследуются от этого базового класса.
Applied to files:
docs/index.md
📚 Learning: 2025-02-10T17:12:56.150Z
Learnt from: nixel2007
Repo: 1c-syntax/bsl-language-server PR: 3408
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/ProtectedModuleDiagnostic.java:63-66
Timestamp: 2025-02-10T17:12:56.150Z
Learning: In BSL Language Server, `documentContext.getServerContext().getConfiguration()` is guaranteed to return a non-null value, making null checks unnecessary.
Applied to files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/context/ServerContext.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/BSLWorkspaceService.java
📚 Learning: 2025-01-19T20:47:40.061Z
Learnt from: nixel2007
Repo: 1c-syntax/bsl-language-server PR: 3388
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/configuration/codelens/TestRunnerAdapterOptions.java:46-46
Timestamp: 2025-01-19T20:47:40.061Z
Learning: Configuration classes in the BSL Language Server project use mutable collections (HashMap, ArrayList) and Data annotation from Lombok, allowing for modification of configuration properties after initialization.
Applied to files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/BSLWorkspaceService.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: CodeQL analysis (java)
- GitHub Check: Benchmark
- GitHub Check: build (21, windows-latest)
- GitHub Check: build (25, windows-latest)
- GitHub Check: build (17, macOS-latest)
- GitHub Check: build (25, ubuntu-latest)
- GitHub Check: build (17, windows-latest)
- GitHub Check: build (25, macOS-latest)
- GitHub Check: build (17, ubuntu-latest)
- GitHub Check: build
- GitHub Check: build (17, windows-latest)
- GitHub Check: build (25, macOS-latest)
- GitHub Check: build (25, ubuntu-latest)
- GitHub Check: build (21, macOS-latest)
- GitHub Check: build (25, windows-latest)
- GitHub Check: build (17, ubuntu-latest)
- GitHub Check: build (17, macOS-latest)
- GitHub Check: build (21, windows-latest)
- GitHub Check: build (21, ubuntu-latest)
- GitHub Check: build
🔇 Additional comments (9)
docs/index.md (1)
61-61: Status update correctly reflects implementation.The
didChangeWatchedFilesoperation now marked as supported with a checkmark, consistent with the PR's implementation of this functionality.docs/en/index.md (1)
57-76: Table reformatting maintains markdown integrity.The workspace and Text Synchronization sections are reformatted for improved alignment and consistency. The restructuring preserves markdown table syntax and the
didChangeWatchedFilesstatus update aligns with the Russian documentation.src/main/java/com/github/_1c_syntax/bsl/languageserver/context/ServerContext.java (1)
223-238: LGTM!The
isDocumentOpenedmethod is well-implemented with clear, comprehensive JavaDoc. The method properly delegates to the thread-safeConcurrentHashMap-backed set, making it safe for concurrent access.src/main/java/com/github/_1c_syntax/bsl/languageserver/BSLWorkspaceService.java (2)
64-64: LGTM!The
serverContextfield is properly injected via constructor (Lombok's@RequiredArgsConstructor), following the project's Spring DI conventions.
150-183: LGTM!The
handleCreatedFileEventandhandleChangedFileEventimplementations correctly handle the document lifecycle. The logic properly distinguishes between opened documents (managed via textDocument events) and non-opened documents (managed via file system events). Good JavaDoc explains the rationale clearly.src/test/java/com/github/_1c_syntax/bsl/languageserver/BSLWorkspaceServiceTest.java (4)
47-62: LGTM!Good test setup with proper Spring Boot integration test annotations. The
@CleanupContextBeforeClassAndAfterEachTestMethodensures test isolation, and@TempDirprovides clean file system isolation per test.
200-221: LGTM!Good test case verifying that deleted files are removed from context even when opened. This correctly tests the design decision that file system deletion takes precedence over editor state.
240-271: LGTM!Good test for batch event processing. The test correctly verifies that multiple events of different types are processed in a single notification.
273-286: LGTM!Clean helper method that creates valid BSL test files with appropriate content.
There was a problem hiding this comment.
Pull request overview
This PR implements support for the LSP workspace/didChangeWatchedFiles notification, allowing the language server to respond to file system changes (create, change, delete) reported by the client. The implementation handles these events asynchronously and respects the distinction between opened files (managed by the editor) and unopened files (managed by the file system).
Key Changes:
- Implemented file event handlers for Created, Changed, and Deleted events in
BSLWorkspaceService - Added
isDocumentOpened()method toServerContextfor checking document open status - Added comprehensive test coverage for all file event scenarios
- Updated documentation to reflect the new capability
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/com/github/_1c_syntax/bsl/languageserver/BSLWorkspaceService.java | Implements async handlers for file system events (create/change/delete) with proper handling of opened vs unopened files |
| src/main/java/com/github/_1c_syntax/bsl/languageserver/context/ServerContext.java | Adds public method to check if a document is currently opened in the editor |
| src/test/java/com/github/_1c_syntax/bsl/languageserver/BSLWorkspaceServiceTest.java | Comprehensive test suite covering all file event scenarios including edge cases with opened/unopened files |
| docs/index.md | Updates Russian documentation to show didChangeWatchedFiles as supported; includes unrelated YourKit section removal |
| docs/en/index.md | Updates English documentation to show didChangeWatchedFiles as supported; includes minor table formatting changes and unrelated YourKit section removal |
Comments suppressed due to low confidence (1)
docs/index.md:271
- [nitpick] This PR includes unrelated changes that remove the YourKit acknowledgment section from the documentation. These changes (lines 265-272 in Russian docs and equivalent in English docs) are not related to the workspace/didChangeWatchedFiles feature implementation. Consider moving these documentation cleanup changes to a separate PR to keep the changes focused on the feature being implemented.
[](https://www.jetbrains.com?from=bsl-language-server)
Создатель профессиональных инструментов разработки программного обеспечения, инновационных и мощных, [JetBrains](https://www.jetbrains.com?from=bsl-language-server) поддержал наш проект, предоставив лицензии на свои продукты, в том числе на `IntelliJ IDEA Ultimate`.
`IntelliJ IDEA Ultimate` один из лучших инструментов в своем классе.
---
| @Test | ||
| void testDidChangeWatchedFiles_Changed_Opened() throws IOException { | ||
| // given | ||
| var testFile = createTestFile("test_changed_opened.bsl"); | ||
| var uri = Absolute.uri(testFile.toURI()); | ||
| var content = FileUtils.readFileToString(testFile, StandardCharsets.UTF_8); | ||
|
|
||
| var documentContext = serverContext.addDocument(uri); | ||
| serverContext.openDocument(documentContext, content, 1); | ||
|
|
||
| // Изменяем содержимое файла на диске | ||
| var newContentOnDisk = "// Измененное содержимое\n"; | ||
| FileUtils.writeStringToFile(testFile, newContentOnDisk, StandardCharsets.UTF_8); | ||
|
|
||
| var fileEvent = new FileEvent(uri.toString(), FileChangeType.Changed); | ||
| var params = new DidChangeWatchedFilesParams(List.of(fileEvent)); | ||
|
|
||
| // when | ||
| workspaceService.didChangeWatchedFiles(params); | ||
| await().pollDelay(Duration.ofMillis(200)).until(() -> true); | ||
|
|
||
| // then | ||
| // Для открытого файла событие Changed должно быть проигнорировано | ||
| // Документ должен остаться в контексте | ||
| assertThat(serverContext.getDocument(uri)).isNotNull(); | ||
| } |
There was a problem hiding this comment.
The test modifies a file on disk and sends a Changed event for an opened file. However, it doesn't verify that the in-memory content remains unchanged (which is the expected behavior according to the implementation). Consider adding an assertion to verify that the document content still matches the original in-memory content and hasn't been reloaded from the modified disk file.
src/main/java/com/github/_1c_syntax/bsl/languageserver/BSLWorkspaceService.java
Show resolved
Hide resolved
src/test/java/com/github/_1c_syntax/bsl/languageserver/BSLWorkspaceServiceTest.java
Show resolved
Hide resolved
| await().pollDelay(Duration.ofMillis(200)).until(() -> true); | ||
|
|
There was a problem hiding this comment.
Similar to the previous test, this uses await().pollDelay(Duration.ofMillis(200)).until(() -> true) as a simple delay. This is a test anti-pattern that makes tests slower and non-deterministic. Consider using a proper assertion or verifying the expected behavior (e.g., that the document state remains unchanged).
| await().pollDelay(Duration.ofMillis(200)).until(() -> true); |
|
|
||
| // when | ||
| workspaceService.didChangeWatchedFiles(params); | ||
| await().pollDelay(Duration.ofMillis(100)).until(() -> true); |
There was a problem hiding this comment.
This test uses await().pollDelay(Duration.ofMillis(100)).until(() -> true) as a delay mechanism. This is an anti-pattern that makes tests slower and non-deterministic. Consider removing the delay and directly asserting the expected state, or use a proper condition that checks for the completion of the async operation.
| await().pollDelay(Duration.ofMillis(100)).until(() -> true); | |
| await().until(() -> serverContext.getDocument(uri) != null); |
src/main/java/com/github/_1c_syntax/bsl/languageserver/BSLWorkspaceService.java
Outdated
Show resolved
Hide resolved
|
|
||
| // when | ||
| workspaceService.didChangeWatchedFiles(params); | ||
| await().pollDelay(Duration.ofMillis(200)).until(() -> true); |
There was a problem hiding this comment.
The test uses await().pollDelay(Duration.ofMillis(200)).until(() -> true) as a simple delay without any actual condition to verify. This is an anti-pattern as it makes the test non-deterministic and slower than necessary. Consider using a proper assertion or verifying that the operation completed without changing the state (e.g., verifying that the document remains open).
| await().pollDelay(Duration.ofMillis(200)).until(() -> true); |
src/main/java/com/github/_1c_syntax/bsl/languageserver/BSLWorkspaceService.java
Outdated
Show resolved
Hide resolved
src/main/java/com/github/_1c_syntax/bsl/languageserver/BSLWorkspaceService.java
Outdated
Show resolved
Hide resolved
| @Test | ||
| void testDidChangeWatchedFiles_Created_AlreadyOpened() throws IOException { | ||
| // given | ||
| var testFile = createTestFile("test_created_opened.bsl"); | ||
| var uri = Absolute.uri(testFile.toURI()); | ||
| var content = FileUtils.readFileToString(testFile, StandardCharsets.UTF_8); | ||
|
|
||
| var documentContext = serverContext.addDocument(uri); | ||
| serverContext.openDocument(documentContext, content, 1); | ||
|
|
||
| var fileEvent = new FileEvent(uri.toString(), FileChangeType.Created); | ||
| var params = new DidChangeWatchedFilesParams(List.of(fileEvent)); | ||
|
|
||
| // when | ||
| workspaceService.didChangeWatchedFiles(params); | ||
| await().pollDelay(Duration.ofMillis(200)).until(() -> true); | ||
|
|
||
| // then | ||
| // Для открытого файла событие Created должно быть проигнорировано | ||
| // Документ должен остаться в контексте | ||
| assertThat(serverContext.getDocument(uri)).isNotNull(); | ||
| } |
There was a problem hiding this comment.
The test creates a file and opens it in the document context, then sends a Created event for the already-opened file. However, in real LSP scenarios, if a file is already opened in an editor, a Created event from the file system watcher would typically indicate that the file was created while already being edited in memory. The test should verify that the in-memory content is preserved and not reloaded from disk. Consider adding an assertion to verify that the document content or version hasn't changed.
nixel2007
force-pushed
the
feature/handle-didChangeWatchedFiles
branch
from
def268f to
d61a13b
Compare
|
…ovements Co-authored-by: nixel2007 <1132840+nixel2007@users.noreply.github.com>
Co-authored-by: nixel2007 <1132840+nixel2007@users.noreply.github.com>
…ents-3628 Fix PR #3628 review comments: variable naming, JavaDoc, and test improvements
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/test/java/com/github/_1c_syntax/bsl/languageserver/BSLWorkspaceServiceTest.java (1)
130-155: Verify in-memory content is preserved when disk changes for opened file.Per past review, this test modifies the file on disk but doesn't verify the in-memory content remains unchanged. This is the key behavior being tested.
// then // Для открытого файла событие Changed должно быть проигнорировано // Документ должен остаться в контексте - assertThat(serverContext.getDocument(uri)).isNotNull(); + var doc = serverContext.getDocument(uri); + assertThat(doc).isNotNull(); + assertThat(serverContext.isDocumentOpened(doc)).isTrue(); + // Verify content wasn't reloaded from modified disk file + assertThat(doc.getContentList().get(0)).doesNotContain("Измененное содержимое");
🧹 Nitpick comments (2)
src/test/java/com/github/_1c_syntax/bsl/languageserver/BSLWorkspaceServiceTest.java (2)
97-97: Replace hard-coded delays with proper assertions.The tests use
await().pollDelay(Duration.ofMillis(X)).until(() -> true)which is essentially aThread.sleep()disguised as an await. This makes tests slower and non-deterministic.For tests verifying that state doesn't change (opened files), consider:
- Using a shorter deterministic wait with a meaningful condition
- Verifying observable side-effects (e.g., document version unchanged)
- Using
AtomicBooleanflags set by mocked/spied componentsExample for line 97:
- await().pollDelay(Duration.ofMillis(200)).until(() -> true); + // Verify document remains in context with same version + await().atMost(Duration.ofMillis(200)).until(() -> serverContext.getDocument(uri) != null); + assertThat(serverContext.getDocument(uri).getVersion()).isEqualTo(1);Also applies to: 123-123, 149-149, 233-233
82-103: Strengthen assertion for opened file behavior.The test verifies the document still exists but doesn't confirm the in-memory content was preserved. Since the implementation explicitly skips reprocessing for opened files, the assertion should verify this behavior.
// then // Для открытого файла событие Created должно быть проигнорировано // Документ должен остаться в контексте - assertThat(serverContext.getDocument(uri)).isNotNull(); + var doc = serverContext.getDocument(uri); + assertThat(doc).isNotNull(); + assertThat(serverContext.isDocumentOpened(doc)).isTrue(); + assertThat(doc.getVersion()).isEqualTo(1);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/com/github/_1c_syntax/bsl/languageserver/BSLWorkspaceService.java(5 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/BSLWorkspaceServiceTest.java(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.java
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.java: Follow the Style Guide provided in docs/en/contributing/StyleGuide.md
Use Lombok annotations to reduce boilerplate code and enable annotation processing in your IDE
Optimize imports before committing but do NOT optimize imports across the entire project unless specifically working on that task
Follow Java naming conventions with meaningful, descriptive names; keep class and method names concise but clear
Write JavaDoc for public APIs and include comments for complex logic
Use Target Java 17 as the language version
Files:
src/test/java/com/github/_1c_syntax/bsl/languageserver/BSLWorkspaceServiceTest.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/BSLWorkspaceService.java
**/test/java/**/*.java
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use appropriate test frameworks (JUnit, AssertJ, Mockito) for testing
Files:
src/test/java/com/github/_1c_syntax/bsl/languageserver/BSLWorkspaceServiceTest.java
🧠 Learnings (2)
📚 Learning: 2025-02-10T17:12:56.150Z
Learnt from: nixel2007
Repo: 1c-syntax/bsl-language-server PR: 3408
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/ProtectedModuleDiagnostic.java:63-66
Timestamp: 2025-02-10T17:12:56.150Z
Learning: In BSL Language Server, `documentContext.getServerContext().getConfiguration()` is guaranteed to return a non-null value, making null checks unnecessary.
Applied to files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/BSLWorkspaceService.java
📚 Learning: 2025-01-19T20:47:40.061Z
Learnt from: nixel2007
Repo: 1c-syntax/bsl-language-server PR: 3388
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/configuration/codelens/TestRunnerAdapterOptions.java:46-46
Timestamp: 2025-01-19T20:47:40.061Z
Learning: Configuration classes in the BSL Language Server project use mutable collections (HashMap, ArrayList) and Data annotation from Lombok, allowing for modification of configuration properties after initialization.
Applied to files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/BSLWorkspaceService.java
🔇 Additional comments (6)
src/test/java/com/github/_1c_syntax/bsl/languageserver/BSLWorkspaceServiceTest.java (2)
267-271: Good improvement: await condition now checks all three URIs.The await condition now correctly verifies all three files' expected states including that
uri3is deleted. This addresses the previous review concern about incomplete verification.
279-292: LGTM!The helper method correctly creates temporary test files with meaningful BSL content using the temp directory provided by JUnit's
@TempDir.src/main/java/com/github/_1c_syntax/bsl/languageserver/BSLWorkspaceService.java (4)
126-138: Document forceful removal of opened files is intentional.The implementation closes and removes documents even if they're opened in the editor. This diverges from typical LSP behavior where opened files remain in memory after filesystem deletion (allowing unsaved changes to persist).
If this is intentional (e.g., BSL-specific requirement), the current implementation is fine but should be documented in the class-level JavaDoc. If not intentional, opened files should potentially be kept until explicitly closed via
textDocument/didClose.Is this the intended behavior for your LSP implementation? Some editors expect opened files to survive filesystem deletion.
172-183: LGTM!The handler correctly adds missing documents and rebuilds only non-opened ones. The variable naming
isDocumentOpenedaccurately reflects the current state check (addressing past review feedback).
140-158: JavaDoc accurately describes the behavior.The documentation clearly states that opened files are skipped because their content is managed via
textDocument/didOpen. This addresses the previous review comment about clarity.
150-158: addDocument safely handles existing documents—no verification needed.The
addDocument(uri)method first callsgetDocument(uri)to check if a document already exists (line 174 in ServerContext.java). If it does, that existing DocumentContext is returned; if not, a new one is created. This prevents duplicates. The subsequent rebuild logic is appropriate regardless of whether the document is new or pre-existing.
src/main/java/com/github/_1c_syntax/bsl/languageserver/BSLWorkspaceService.java
Show resolved
Hide resolved
src/main/java/com/github/_1c_syntax/bsl/languageserver/BSLWorkspaceService.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/main/java/com/github/_1c_syntax/bsl/languageserver/BSLWorkspaceService.java (2)
126-137: Verify behavior for opened files deleted from filesystem.When a file opened in an editor is deleted from the filesystem, this implementation closes and removes it from memory. LSP clients typically expect opened files to remain in the editor (with unsaved changes visible) even after filesystem deletion. Please verify this behavior aligns with user expectations.
92-105: Add error handling for async file event processing.Exceptions thrown inside
CompletableFuture.runAsyncare silently swallowed unless handled. File I/O or context operations may throw, and failures would go unnoticed.Apply this diff to add error handling and logging:
CompletableFuture.runAsync( () -> { - for (var fileEvent : params.getChanges()) { - var uri = Absolute.uri(fileEvent.getUri()); - - switch (fileEvent.getType()) { - case Deleted -> handleDeletedFileEvent(uri); - case Created -> handleCreatedFileEvent(uri); - case Changed -> handleChangedFileEvent(uri); + try { + for (var fileEvent : params.getChanges()) { + var uri = Absolute.uri(fileEvent.getUri()); + + switch (fileEvent.getType()) { + case Deleted -> handleDeletedFileEvent(uri); + case Created -> handleCreatedFileEvent(uri); + case Changed -> handleChangedFileEvent(uri); + } } + } catch (Exception e) { + // Add logging with SLF4J logger + // LOGGER.error("Error processing didChangeWatchedFiles event", e); } }, executorService );
🧹 Nitpick comments (3)
src/main/java/com/github/_1c_syntax/bsl/languageserver/BSLWorkspaceService.java (3)
66-66: Consider using a fixed-size thread pool instead of cached.The cached thread pool can grow unbounded during heavy file system activity (e.g., IDE indexing, build processes). For file watching events, a fixed-size pool or even a single-threaded executor may be more appropriate to prevent resource exhaustion.
Apply this diff to use a fixed thread pool:
- private final ExecutorService executorService = Executors.newCachedThreadPool(new CustomizableThreadFactory("workspace-service-")); + private final ExecutorService executorService = Executors.newFixedThreadPool(2, new CustomizableThreadFactory("workspace-service-"));
149-157: Extract common rebuild-and-clear pattern.Both
handleCreatedFileEventandhandleChangedFileEventcontain identical logic for rebuilding and clearing non-opened documents. Consider extracting this pattern to reduce duplication.Add a helper method:
/** * Обрабатывает документ: выполняет rebuild и очистку вторичных данных, * если документ не открыт в редакторе. * * @param documentContext контекст документа для обработки */ private void rebuildAndClearIfNotOpened(DocumentContext documentContext) { var isDocumentOpened = serverContext.isDocumentOpened(documentContext); if (!isDocumentOpened) { serverContext.rebuildDocument(documentContext); serverContext.tryClearDocument(documentContext); } }Then refactor both methods:
private void handleCreatedFileEvent(URI uri) { var documentContext = serverContext.addDocument(uri); - - var isDocumentOpened = serverContext.isDocumentOpened(documentContext); - if (!isDocumentOpened) { - serverContext.rebuildDocument(documentContext); - serverContext.tryClearDocument(documentContext); - } + rebuildAndClearIfNotOpened(documentContext); }private void handleChangedFileEvent(URI uri) { var documentContext = serverContext.getDocument(uri); if (documentContext == null) { documentContext = serverContext.addDocument(uri); } - - var isDocumentOpened = serverContext.isDocumentOpened(documentContext); - if (!isDocumentOpened) { - serverContext.rebuildDocument(documentContext); - serverContext.tryClearDocument(documentContext); - } + rebuildAndClearIfNotOpened(documentContext); }Also applies to: 171-182
126-182: Add logging for file event handling.The handler methods lack logging, making it difficult to debug file watching issues. Consider adding structured logging at appropriate levels (debug for normal operations, warn for unexpected states).
Example for
handleDeletedFileEvent:private void handleDeletedFileEvent(URI uri) { LOGGER.debug("Processing file deletion event for URI: {}", uri); var documentContext = serverContext.getDocument(uri); if (documentContext == null) { LOGGER.debug("Document not found in context, skipping deletion: {}", uri); return; } var isDocumentOpened = serverContext.isDocumentOpened(documentContext); if (isDocumentOpened) { LOGGER.debug("Closing opened document before deletion: {}", uri); serverContext.closeDocument(documentContext); } serverContext.removeDocument(uri); LOGGER.debug("Document removed from context: {}", uri); }Note: Ensure an SLF4J logger is declared at the class level:
private static final Logger LOGGER = LoggerFactory.getLogger(BSLWorkspaceService.class);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/BSLWorkspaceService.java(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.java: Follow the Style Guide provided in docs/en/contributing/StyleGuide.md
Use Lombok annotations to reduce boilerplate code and enable annotation processing in your IDE
Optimize imports before committing but do NOT optimize imports across the entire project unless specifically working on that task
Follow Java naming conventions with meaningful, descriptive names; keep class and method names concise but clear
Write JavaDoc for public APIs and include comments for complex logic
Use Target Java 17 as the language version
Files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/BSLWorkspaceService.java
🧠 Learnings (2)
📚 Learning: 2025-02-10T17:12:56.150Z
Learnt from: nixel2007
Repo: 1c-syntax/bsl-language-server PR: 3408
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/ProtectedModuleDiagnostic.java:63-66
Timestamp: 2025-02-10T17:12:56.150Z
Learning: In BSL Language Server, `documentContext.getServerContext().getConfiguration()` is guaranteed to return a non-null value, making null checks unnecessary.
Applied to files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/BSLWorkspaceService.java
📚 Learning: 2025-01-19T20:47:40.061Z
Learnt from: nixel2007
Repo: 1c-syntax/bsl-language-server PR: 3388
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/configuration/codelens/TestRunnerAdapterOptions.java:46-46
Timestamp: 2025-01-19T20:47:40.061Z
Learning: Configuration classes in the BSL Language Server project use mutable collections (HashMap, ArrayList) and Data annotation from Lombok, allowing for modification of configuration properties after initialization.
Applied to files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/BSLWorkspaceService.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze the repo with CodeSee
| CompletableFuture.runAsync( | ||
| () -> { | ||
| for (var fileEvent : params.getChanges()) { | ||
| var uri = Absolute.uri(fileEvent.getUri()); |
There was a problem hiding this comment.
в принципе не обязательно, т.к. серверконтекст сам следит за качеством uri
There was a problem hiding this comment.
да, только там большинство методов уже принимают в себя URI, а не строку. а делать URI.create из входящей строки небезопасно, т.к. там херня летит. поэтому явный каст к Absolute. и да, будет дважды вызов
Описание
Связанные задачи
Closes
Чеклист
Общие
gradlew precommit)Для диагностик
Дополнительно
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.