Repository navigation
fix: make normalizePath idempotent by adding global flag to double-slash regex - #177
darzasassing-hub wants to merge 2 commits into
Conversation
…ash regex
The second .replace(/\/\/+/, '/') was missing the global flag,
so only the first group of consecutive slashes was collapsed.
For example:
normalizePath('a///b///c.txt') returned 'a/b///c.txt' instead of 'a/b/c.txt'
normalizePath('a//b//c//d') returned 'a/b//c//d' instead of 'a/b/c/d'
Fix: change /\/\/+/ to /\/\/+/g so all slash groups are collapsed in one pass.
Fixes tscircuit#161
andrewsiah
left a comment
There was a problem hiding this comment.
Nice minimal fix — verified it on current main. Two things worth adding before this merges, though.
1. The one-line diff is correct, and here's a drop-in regression test
Without the fix, normalizePath("a///b///c.txt") returns "a/b///c.txt" and lookups in lib/db/db-client.ts (which calls normalizePath on both store and lookup) miss rows that exist. With this PR's change, all cases collapse correctly.
I checked the fix in both directions:
tests/utils/normalize-path.test.tson unpatched main → 2 of 3 tests fail (repro confirmed)- Same tests with only this PR's source change → 3/3 pass
2. Please add this test file to the PR (tests/utils/normalize-path.test.ts, follows the repo's existing bun:test conventions):
import { expect, test } from "bun:test"
import { normalizePath } from "../../lib/utils/normalize-path"
test("collapses every repeated slash group, not just the first", () => {
expect(normalizePath("a///b///c.txt")).toBe("a/b/c.txt")
expect(normalizePath("a//b//c//d")).toBe("a/b/c/d")
expect(normalizePath("//a//b")).toBe("a/b")
})
test("normalizePath is idempotent", () => {
const cases = ["a///b///c.txt", "a//b//c//d", "//a//b", "a//", "///", "a/b", "", "/"]
for (const input of cases) {
const once = normalizePath(input)
expect(normalizePath(once)).toBe(once)
}
})
test("keeps existing behaviour for backslashes, leading and trailing slashes", () => {
expect(normalizePath("a\\\\b")).toBe("a/b")
expect(normalizePath("/a/b")).toBe("a/b")
expect(normalizePath("a/b/")).toBe("a/b")
expect(normalizePath("/")).toBe("")
})With the source change plus this file, the full suite is green at 32 pass / 0 fail.
Given this issue has accumulated 8+ open PRs with the same one-line diff, the differentiator for getting this one merged is exactly this: the only PR that ships with tests proving the fix and pinning the behavior.
|
Thank you @andrewsiah! Added The PR now includes both the minimal fix and the full test suite pinning the idempotent behavior. Ready for merge! /claim #161 |
Summary
Fixes #161 —
ormalizePath was not idempotent when a path contained multiple consecutive slash groups.
Root Cause
The second .replace() call in lib/utils/normalize-path.ts used the regex ///+/ without the g (global) flag, so only the first group of repeated slashes was collapsed. Every subsequent group was left untouched.
` s
// Before (buggy)
let normalized = path.replace(/\+/g, "/").replace(///+/, "/")
// ^ missing g flag
// After (fix)
let normalized = path.replace(/\+/g, "/").replace(///+/g, "/")
// ^ global flag added
`
Verification
Tested manually against 9 cases including the exact repros from the issue:
a///b///c.txta/b///c.txt❌a/b/c.txt✅a//b//c//da/b//c//d❌a/b/c/d✅All 9 tests pass (9/9).
/claim #161
/attempt #161