Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Dec 25, 2025

The current architecture requires ServerFormatter and ServerLinter to call static builder methods (ServerFormatterBuilder::build()) when rebuilding, preventing builders from holding and reusing state across rebuilds.

Changes

Tool trait (tool.rs)

  • Added builder: &dyn ToolBuilder parameter to handle_configuration_change and handle_watched_file_change

Tool implementations (server_formatter.rs, server_linter.rs)

  • Replaced static ServerFormatterBuilder::build() / ServerLinterBuilder::build() calls with builder.build_boxed()
  • Both tools now receive and use the builder reference for rebuilding

WorkspaceWorker (worker.rs)

  • Stores Arc<Vec<Box<dyn ToolBuilder>>> alongside tools
  • Constructor accepts builders parameter; start_worker() uses stored builders
  • Passes appropriate builder reference to each tool during handle_configuration_change and handle_watched_file_change

Backend (backend.rs)

  • Wraps tool_builders in Arc for shared ownership across workers
  • Updated all WorkspaceWorker instantiations to pass builder Arc

Example

Before:

fn handle_configuration_change(&self, root_uri: &Uri, ...) -> ToolRestartChanges {
    let new_formatter = ServerFormatterBuilder::build(root_uri, new_options);
    // ...
}

After:

fn handle_configuration_change(&self, builder: &dyn ToolBuilder, root_uri: &Uri, ...) -> ToolRestartChanges {
    let new_formatter = builder.build_boxed(root_uri, new_options);
    // ...
}

This enables ToolBuilder implementations to hold state without further API changes.

Original prompt

Problem Statement

Currently, ServerFormatter and ServerLinter rebuild themselves in handle_configuration_change and handle_watched_file_change by calling static methods like ServerFormatterBuilder::build(). This prevents the ToolBuilder from holding and reusing state during tool rebuilds.

Goal

Refactor the architecture so that:

  1. The ToolBuilder can hold its own data/state
  2. The Tool trait methods handle_configuration_change and handle_watched_file_change receive a reference to the ToolBuilder (&dyn ToolBuilder)
  3. Tools can use the builder reference to rebuild themselves, allowing the builder to utilize its stored data

Required Changes

1. Update the Tool trait in crates/oxc_language_server/src/tool.rs

Modify the trait methods to accept a builder parameter:

fn handle_configuration_change(
    &self,
    builder: &dyn ToolBuilder,
    root_uri: &Uri,
    old_options_json: &serde_json::Value,
    new_options_json: serde_json::Value,
) -> ToolRestartChanges;

fn handle_watched_file_change(
    &self,
    builder: &dyn ToolBuilder,
    changed_uri: &Uri,
    root_uri: &Uri,
    options: serde_json::Value,
) -> ToolRestartChanges;

2. Update ServerFormatter implementation in crates/oxc_language_server/src/formatter/server_formatter.rs

Update the Tool trait implementation to use the builder reference:

fn handle_configuration_change(
    &self,
    builder: &dyn ToolBuilder,
    root_uri: &Uri,
    old_options_json: &serde_json::Value,
    new_options_json: serde_json::Value,
) -> ToolRestartChanges {
    // ... existing comparison logic ...
    
    let new_formatter = builder.build_boxed(root_uri, new_options_json.clone());
    let watch_patterns = new_formatter.get_watcher_patterns(new_options_json);
    ToolRestartChanges {
        tool: Some(new_formatter),
        watch_patterns: Some(watch_patterns),
    }
}

fn handle_watched_file_change(
    &self,
    builder: &dyn ToolBuilder,
    _changed_uri: &Uri,
    root_uri: &Uri,
    options: serde_json::Value,
) -> ToolRestartChanges {
    let new_formatter = builder.build_boxed(root_uri, options);
    ToolRestartChanges {
        tool: Some(new_formatter),
        watch_patterns: None,
    }
}

3. Update ServerLinter implementation in crates/oxc_language_server/src/linter/server_linter.rs

Similar changes to use the builder reference instead of static ServerLinterBuilder::build() calls.

4. Update WorkspaceWorker in crates/oxc_language_server/src/worker.rs

Update the code that calls these trait methods to pass the builder:

  • Store the builder alongside each tool
  • Pass the builder reference when calling handle_configuration_change and handle_watched_file_change

5. Update backend initialization in crates/oxc_language_server/src/backend.rs

Ensure builders are properly stored and accessible when creating workspace workers.

6. Update test code

  • Update FakeTool in crates/oxc_language_server/src/tests.rs
  • Update test helpers in crates/oxc_language_server/src/formatter/tester.rs
  • Update test helpers in crates/oxc_language_server/src/linter/tester.rs

Implementation Notes

  • The ToolBuilder trait doesn't need changes to its interface
  • ServerFormatterBuilder and ServerLinterBuilder can remain zero-sized structs for now, but this change enables them to hold state in the future
  • Ensure all call sites are updated to maintain existing behavior
  • The refactoring should not change external behavior, only internal architecture

Success Criteria

  • All existing tests pass
  • The code compiles without errors
  • handle_configuration_change and handle_watched_file_change use builder.build_boxed() instead of static method calls
  • The architecture supports ToolBuilder holding state for future enhancements

This pull request was created from Copilot chat.


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Copilot AI and others added 2 commits December 25, 2025 23:24
- Updated Tool trait methods to accept `builder: &dyn ToolBuilder` parameter
- Modified ServerFormatter and ServerLinter to use builder reference
- Updated WorkspaceWorker to store builders and pass them to tools
- Modified Backend to use Arc<Vec<Box<dyn ToolBuilder>>>
- Updated all test code to work with new architecture
- All tests passing

Co-authored-by: Sysix <[email protected]>
Copilot AI changed the title [WIP] Refactor Tool architecture to utilize ToolBuilder state refactor(language_server): Pass ToolBuilder reference to Tool trait methods Dec 25, 2025
Copilot AI requested a review from Sysix December 25, 2025 23:29
@github-actions github-actions bot added A-editor Area - Editor and Language Server C-cleanup Category - technical debt or refactoring. Solution not expected to change behavior labels Dec 25, 2025
@Sysix Sysix requested a review from Copilot December 25, 2025 23:55
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors the Tool trait architecture to pass ToolBuilder references to handle_configuration_change and handle_watched_file_change methods, enabling builders to hold and reuse state across tool rebuilds.

Key Changes:

  • Added builder: &dyn ToolBuilder parameter to Tool trait methods
  • Updated all Tool implementations to use builder.build_boxed() instead of static builder method calls
  • Modified WorkspaceWorker to store builders alongside tools and pass appropriate builder references during tool operations

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
crates/oxc_language_server/src/tool.rs Added builder parameter to handle_configuration_change and handle_watched_file_change trait methods
crates/oxc_language_server/src/formatter/server_formatter.rs Updated ServerFormatter to use builder.build_boxed() instead of static ServerFormatterBuilder::build() calls
crates/oxc_language_server/src/linter/server_linter.rs Updated ServerLinter to use builder.build_boxed() instead of static ServerLinterBuilder::build() calls
crates/oxc_language_server/src/worker.rs Added builders: Arc<[Box<dyn ToolBuilder>]> field, updated constructor and start_worker() to use stored builders, modified handle_tool_changes() to pass builder references
crates/oxc_language_server/src/backend.rs Wrapped tool_builders in Arc and updated all WorkspaceWorker instantiations to pass cloned Arc references
crates/oxc_language_server/src/tests.rs Updated FakeTool implementation to use builder parameter in trait methods
crates/oxc_language_server/src/formatter/tester.rs Updated test helper to instantiate builder and pass it to handle_configuration_change()
crates/oxc_language_server/src/linter/tester.rs Updated test helper to instantiate builder and pass it to handle_configuration_change()

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Co-authored-by: Copilot <[email protected]>
Signed-off-by: Alexander S. <[email protected]>
@Sysix Sysix marked this pull request as ready for review December 26, 2025 00:16
@Sysix Sysix requested a review from camc314 as a code owner December 26, 2025 00:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-editor Area - Editor and Language Server C-cleanup Category - technical debt or refactoring. Solution not expected to change behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants