Skip to content
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

feat(mentions): improve path handling for Windows and escaped spaces #1319

Merged
merged 2 commits into from
Mar 3, 2025

Conversation

samhvw8
Copy link
Collaborator

@samhvw8 samhvw8 commented Mar 2, 2025

Enhance the mention regex to properly support:

  • Windows paths with drive letters (C:\folder\file.txt)
  • Windows network shares (\server\share\file.txt)
  • Windows relative paths (folder\file.txt)
  • Paths with escaped spaces in both Unix and Windows formats
  • Maintain compatibility with existing URL, git hash, and keyword patterns
  • Add comprehensive test suite with 200+ test cases validating all path formats and edge cases to ensure reliable pattern matching across platforms.

Context

This pr will make user can use window path style on mention & file has space on file path

Implementation

  • separate current regex into multiple part for easy to maintain
  • add support to window path format
  • add support path with space using escape "/" for window "" for unix

Screenshots

image
before after

How to Test

Get in Touch


Important

Enhance mention regex to support Windows paths and escaped spaces, with comprehensive tests in context-mentions.ts.

  • Regex Enhancements:
    • Supports Windows paths with drive letters, network shares, and relative paths in context-mentions.ts.
    • Handles paths with escaped spaces for both Unix and Windows formats.
    • Maintains compatibility with existing URL, git hash, and keyword patterns.
  • Testing:
    • Adds context-mentions.test.ts with 200+ test cases covering all path formats and edge cases.
    • Validates regex against Windows paths, Unix paths, URLs, git hashes, and special keywords.
    • Ensures invalid patterns are correctly rejected and mentions are matched within text context.

This description was created by Ellipsis for 500c817. It will automatically update as commits are pushed.

Enhance the mention regex to properly support:
- Windows paths with drive letters (C:\folder\file.txt)
- Windows network shares (\\server\share\file.txt)
- Windows relative paths (folder\file.txt)
- Paths with escaped spaces in both Unix and Windows formats
- Maintain compatibility with existing URL, git hash, and keyword patterns
- Add comprehensive test suite with 200+ test cases validating all path formats
and edge cases to ensure reliable pattern matching across platforms.
@samhvw8 samhvw8 requested review from mrubens and cte as code owners March 2, 2025 16:56
Copy link

changeset-bot bot commented Mar 2, 2025

⚠️ No Changeset found

Latest commit: 41a1c82

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@samhvw8 samhvw8 force-pushed the feat/windown-path-and-escape-space-path branch from 500c817 to dd961d1 Compare March 2, 2025 16:56
@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. enhancement New feature or request labels Mar 2, 2025
Copy link
Collaborator

@cte cte left a comment

Choose a reason for hiding this comment

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

Looks good! I had Roo add some additional tests and it flagged some regex behavior that might not be intended; please take a look (but feel free to ignore if they seem too edge casey):

describe("Special Characters in Paths", () => {
	it("handles special characters in file paths", () => {
		const cases: Array<[string, string]> = [
			["@/path/with-dash/file_underscore.txt", "@/path/with-dash/file_underscore.txt"],
			["@C:\\folder+plus\\file(parens)[]brackets.txt", "@C:\\folder+plus\\file(parens)[]brackets.txt"],
			["@/path/with/file#hash%percent.txt", "@/path/with/file#hash%percent.txt"],
			["@/path/with/file@symbol$dollar.txt", "@/path/with/file@symbol$dollar.txt"],
		]

		cases.forEach(([input, expected]) => {
			const result = testMention(input, expected)
			expectMatch(result)
		})
	})
})

describe("Mixed Path Types in Single String", () => {
	it("correctly identifies the first path in a string with multiple path types", () => {
		const text = "Check both @/unix/path and @C:\\windows\\path for details."
		const result = mentionRegex.exec(text)
		expect(result?.[0]).toBe("@/unix/path")

		// Test starting from after the first match
		const secondSearchStart = text.indexOf("@C:")
		const secondResult = mentionRegex.exec(text.substring(secondSearchStart))
		expect(secondResult?.[0]).toBe("@C:\\windows\\path")
	})
})

describe("Non-Latin Character Support", () => {
	it("handles international characters in paths", () => {
		const cases: Array<[string, string]> = [
			["@/path/to/你好/file.txt", "@/path/to/你好/file.txt"],
			["@C:\\用户\\документы\\файл.txt", "@C:\\用户\\документы\\файл.txt"],
			["@/путь/к/файлу.txt", "@/путь/к/файлу.txt"],
			["@C:\\folder\\file_äöü.txt", "@C:\\folder\\file_äöü.txt"],
		]

		cases.forEach(([input, expected]) => {
			const result = testMention(input, expected)
			expectMatch(result)
		})
	})
})

describe("Mixed Path Delimiters", () => {
	// Modifying expectations to match current behavior
	it("documents behavior with mixed forward and backward slashes in Windows paths", () => {
		const cases: Array<[string, null]> = [
			// Current implementation doesn't support mixed slashes
			["@C:\\Users/Documents\\folder/file.txt", null],
			["@C:/Windows\\System32/drivers\\etc/hosts", null],
		]

		cases.forEach(([input, expected]) => {
			const result = testMention(input, expected)
			expectMatch(result)
		})
	})
})

describe("Extended Negative Tests", () => {
	// Modifying expectations to match current behavior
	it("documents behavior with potentially invalid characters", () => {
		const cases: Array<[string, string]> = [
			// Current implementation actually matches these patterns
			["@/path/with<illegal>chars.txt", "@/path/with<illegal>chars.txt"],
			["@C:\\folder\\file|with|pipe.txt", "@C:\\folder\\file|with|pipe.txt"],
			['@/path/with"quotes".txt', '@/path/with"quotes".txt'],
		]

		cases.forEach(([input, expected]) => {
			const result = testMention(input, expected)
			expectMatch(result)
		})
	})
})

// These are documented as "not implemented yet"
describe("Future Enhancement Candidates", () => {
	it("identifies patterns that could be supported in future enhancements", () => {
		// These patterns aren't currently supported by the regex
		// but might be considered for future improvements
		console.log(
			"The following patterns are not currently supported but might be considered for future enhancements:",
		)
		console.log("- Paths with double slashes: @/path//with/double/slash.txt")
		console.log("- Complex path traversals: @/very/./long/../../path/.././traversal.txt")
		console.log("- Environment variables in paths: @$HOME/file.txt, @C:\\Users\\%USERNAME%\\file.txt")
	})
})

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Mar 3, 2025
@samhvw8
Copy link
Collaborator Author

samhvw8 commented Mar 3, 2025

@cte cool let me update that test

@cte cte merged commit 464d440 into RooVetGit:main Mar 3, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request lgtm This PR has been approved by a maintainer size:L This PR changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants