-
-
Notifications
You must be signed in to change notification settings - Fork 505
Fix: Untitled documents should use VS Code settings only #3886
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Modified getResolvedConfig to use workspace folder path for config search when document is untitled - Added test fixture for requireConfig scenario - Added integration tests for untitled document formatting Co-authored-by: ntotten <282782+ntotten@users.noreply.github.com>
- Added isUntitledDocument() helper method for better readability - Added getConfigSearchPath() helper method to encapsulate logic - Improved testability and maintainability Co-authored-by: ntotten <282782+ntotten@users.noreply.github.com>
|
@copilot The implementation here is mostly good, but untitled files should not be treated as part of the workspace. They should only use the user's global/personal vscode settings as an untitled document isnt part of a workspace. Only if the file is saved in the workspace should workspace settings be used. |
There was a problem hiding this 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 fixes a bug where untitled (unsaved) documents couldn't be formatted when prettier.requireConfig was enabled. The root cause was that Prettier's config resolution was searching from the untitled document's display name (e.g., "Untitled-1") rather than the workspace folder, causing it to fail finding the workspace's .prettierrc file.
Changes:
- Added helper methods
isUntitledDocument()andgetConfigSearchPath()to detect untitled documents and use the workspace folder path for config resolution - Modified
getResolvedConfig()to use the workspace folder path for untitled documents instead of the document's display name - Added test fixture
require-config/with requireConfig enabled and basic untitled document formatting tests
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/ModuleResolverNode.ts | Added untitled document detection and modified config resolution to use workspace folder path for untitled documents |
| src/test/suite/untitled.test.ts | Added basic tests for untitled document formatting (JavaScript and TypeScript) |
| test-fixtures/require-config/.prettierrc | Created test fixture with Prettier configuration (semi: false, singleQuote: true) |
| test-fixtures/require-config/.vscode/settings.json | Configured requireConfig: true for the test fixture |
| test-fixtures/require-config/package.json | Added package.json for the test fixture |
| test-fixtures/test.code-workspace | Added require-config to workspace folders |
| import * as assert from "assert"; | ||
| import * as vscode from "vscode"; | ||
| import { ensureExtensionActivated } from "./testUtils.js"; | ||
|
|
||
| /** | ||
| * Tests for untitled documents (unsaved files) | ||
| */ | ||
| describe("Test untitled documents", () => { | ||
| before(async () => { | ||
| await ensureExtensionActivated(); | ||
| }); | ||
|
|
||
| it("formats untitled JavaScript document", async () => { | ||
| // Create an untitled document with ugly JavaScript | ||
| const uglyCode = 'const x = 1 ; console.log( x ) ;'; | ||
| const doc = await vscode.workspace.openTextDocument({ | ||
| language: "javascript", | ||
| content: uglyCode, | ||
| }); | ||
|
|
||
| const editor = await vscode.window.showTextDocument(doc); | ||
|
|
||
| // Format the document | ||
| await vscode.commands.executeCommand("editor.action.formatDocument"); | ||
|
|
||
| // The document should be formatted | ||
| const formattedText = editor.document.getText(); | ||
|
|
||
| assert.notEqual( | ||
| formattedText, | ||
| uglyCode, | ||
| "Untitled document should have been formatted", | ||
| ); | ||
|
|
||
| // Should not have excessive spacing | ||
| assert.ok( | ||
| !formattedText.includes(" "), | ||
| "Formatted code should not have excessive spacing", | ||
| ); | ||
| }); | ||
|
|
||
| it("formats untitled TypeScript document", async () => { | ||
| // Create an untitled document with ugly TypeScript | ||
| const uglyCode = 'function test ( a : number ) : number { return a * 2 ; }'; | ||
| const doc = await vscode.workspace.openTextDocument({ | ||
| language: "typescript", | ||
| content: uglyCode, | ||
| }); | ||
|
|
||
| const editor = await vscode.window.showTextDocument(doc); | ||
|
|
||
| // Format the document | ||
| await vscode.commands.executeCommand("editor.action.formatDocument"); | ||
|
|
||
| // The document should be formatted | ||
| const formattedText = editor.document.getText(); | ||
|
|
||
| assert.notEqual( | ||
| formattedText, | ||
| uglyCode, | ||
| "Untitled TypeScript document should have been formatted", | ||
| ); | ||
|
|
||
| // Should not have excessive spacing | ||
| assert.ok( | ||
| !formattedText.includes(" "), | ||
| "Formatted code should not have excessive spacing", | ||
| ); | ||
| }); | ||
| }); |
Copilot
AI
Jan 17, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests in this file do not actually test the bug that this PR is fixing - untitled documents with requireConfig enabled. The tests create untitled documents but don't test them in the context of the "require-config" workspace fixture which has "prettier.requireConfig": true set in its settings.
To properly test this fix, consider adding a test that:
- Opens the "require-config" workspace (which has requireConfig enabled)
- Creates an untitled document within that workspace context
- Verifies that formatting succeeds (not disabled) because the workspace has a .prettierrc file
- Optionally verifies that the .prettierrc settings are applied (e.g., semi: false, singleQuote: true)
The current tests only verify that untitled documents can be formatted in general, but don't test the specific requireConfig scenario.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot apply changes based on this feedback
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a test that verifies untitled documents can be formatted even when requireConfig is enabled in the workspace. The test demonstrates that untitled documents properly bypass the requireConfig check and use VS Code settings only. Changes in commit 2e59161.
src/ModuleResolverNode.ts
Outdated
| private getConfigSearchPath(doc: TextDocument): string { | ||
| if (this.isUntitledDocument(doc)) { | ||
| const workspaceFolder = workspace.getWorkspaceFolder(doc.uri); | ||
| if (workspaceFolder) { | ||
| return workspaceFolder.uri.fsPath; | ||
| } | ||
| } | ||
| return doc.fileName; | ||
| } |
Copilot
AI
Jan 17, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When an untitled document has no associated workspace folder (e.g., opening VS Code without a workspace), this function will return doc.fileName which for untitled documents is typically a display name like "Untitled-1" rather than a valid file path. This could cause issues with Prettier's config resolution.
Consider handling this edge case explicitly - if there's no workspace folder for an untitled document, the function could return a default path (e.g., the user's home directory or the current working directory) or return a special value that the caller can handle appropriately. Alternatively, document this behavior so callers know that doc.fileName may not be a real file path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot apply changes based on this feedback
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment was based on the old implementation. The current implementation (since commit 2affb8a) returns null immediately for ALL untitled documents regardless of whether they have a workspace folder, so this edge case is already handled correctly. Untitled documents always use VS Code settings only.
Untitled documents should not use workspace configuration. They should only use the user's global/personal VS Code settings. - Modified getResolvedConfig to return null for untitled documents - Removed test fixture for requireConfig (no longer needed) - Updated tests to reflect correct behavior Co-authored-by: ntotten <282782+ntotten@users.noreply.github.com>
You're absolutely right. I've updated the implementation to ensure untitled documents only use VS Code settings and not workspace configuration. The fix now:
Changes in commit 2affb8a. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
| // If stat fails, assume it's a directory path | ||
| } | ||
|
|
||
| modulePackageJsonPath = path.join(packageDir, "package.json"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot why did you remove this? this isnt part of this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The previous revert accidentally removed logic that handles cases where modulePath is a file (e.g., .yarn/sdks/prettier/index.cjs) rather than a directory. This restores that logic from the main branch merge. Co-authored-by: ntotten <282782+ntotten@users.noreply.github.com>
Co-authored-by: ntotten <282782+ntotten@users.noreply.github.com>
- Added test fixture for requireConfig scenario - Added test to verify untitled documents can be formatted even when requireConfig is enabled - This demonstrates that untitled documents properly bypass requireConfig check Co-authored-by: ntotten <282782+ntotten@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Summary
Fixed behavior of untitled (unsaved) documents to use only VS Code settings instead of workspace configuration, as requested by @ntotten.
Changes Made
Behavior
Untitled documents: Use VS Code settings only, bypassing workspace config and
requireConfigchecksSaved files in workspace: Use workspace config as before
Implementation
The fix detects untitled documents via
uri.scheme !== "file"and immediately returnsnullfromgetResolvedConfig(), which signals the formatter to use VS Code settings only.Testing
✅ Linting passed
✅ Build successful
✅ Tests verify untitled documents work even with requireConfig enabled
✅ Code review passed
Original prompt
💡 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.