fix: Resolve : ambiguity in URL path placeholders#465
Open
simon446 wants to merge 2 commits into
Open
Conversation
954e471 to
11d6786
Compare
: ambiguity in URL path placeholders: ambiguity in URL path placeholders
`:name` path placeholders were ambiguous when followed by a literal `:`
in the same segment. The grammar had no terminator on the placeholder
name, and the Rust resolver's anchor required `/?#$` after the name —
so `/:id:literal` parsed as one big placeholder and never substituted.
- URL grammar: placeholder name now excludes `:`, and a `/`-segment can
contain Placeholder followed by literal PathSegment. `/:abc:def` now
parses as Placeholder(:abc) + PathSegment(:def).
- Rust resolver: add `:` to the trailing boundary set so `:id` in
`:id:literal` substitutes correctly.
- Editor rendering: filter Placeholder chip widgets to nodes actually
at a `/`-segment start (the lexer over-emits Placeholder mid-segment
on error recovery), and drop the `t.emphasis` style on Placeholder
nodes — chip widgets fully replace the text, so the style was only
ever visible on the spurious nodes we now filter.
- Frontend regex: extract URL→placeholder extraction into a shared
`extractPathPlaceholders` helper, tighten the regex to disallow `:`
in placeholder names so partial typing and `:abc:def` are handled.
Motivated by — but does not itself fix — OpenAPI imports of AIP-136
custom methods like `/tasks/{id}:increment-importance`. That symptom
will be resolved separately when `openapi-to-postmanv2` picks up the
upstream fix at postmanlabs/openapi-to-postman (PR pending) and the
dep is bumped.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
11d6786 to
7b7f1be
Compare
`/:foo/:bar` previously left `:foo` un-rendered as a chip. Two underlying issues, both fixed: - URL grammar required `Host`. For a URL starting with `/`, the parser error-recovered past the leading slash and consumed the first segment as Host (since Host's char class includes `:` for `host:port`), eating an initial `:name` placeholder. Make Host optional so path-only URLs go straight to the Path branch. - `pathParameters.ts` looked for the `url` overlay node to find Placeholders. With Host optional, parses without a Host produce `Path` as the topmost overlay node (since parseMixed drops the inner top wrapper when there's no surrounding error). Accept either `url` or `Path` as the entry point. Regression test added in `url.test.ts` covering the `/:chip1/:chip2` shape. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
:namepath placeholders were ambiguous when followed by a literal:in the same path segment. The URL grammar matched the placeholder name greedily until/?#, and the Rust resolver's anchor required/?#$after the name — so/users/:id:literalparsed as one big placeholder and never substituted. This PR tightens the grammar so the name excludes:, extends the resolver to accept:as a trailing boundary, and adjusts the editor (chip widget filter, highlight, param-list extraction) so the new boundaries are reflected visually.Before / after
URL field rendering:
Submission
CONTRIBUTING.md.Approved feedback item (required if not a bug fix or small-scope improvement):
Related
:-suffixed path segments show up in the wild for AIP-136 custom methods (e.g./tasks/{id}:cancel,/users/{id}:archive) and any other path style that uses:verbafter a resource id. Before this PR, those URLs rendered with one greedy chip and didn't substitute when sent.Details
Grammar (
url.grammar):Placeholder { ":" ![/?#:]+ }excludes:from the name, and a/-segment now allowsPlaceholder PathSegment?so/:abc:defparses as Placeholder(:abc) + PathSegment(:def).Resolver (
crates/yaak-http/src/path_placeholders.rs): added:to the trailing-boundary regex set so:idin:id:literalsubstitutes correctly. One new unit test.Editor:
pathParameters.ts: chip widgets only render forPlaceholdernodes whose preceding document character is/— filters out the lexer's mid-segment over-emission and the long-standing env-var-followed-by-:abcquirk (${[var]}:abcno longer renders:abcas a spurious chip).highlight.ts: dropped thePlaceholder: t.emphasisstyle — chip widgets fully replace the placeholder text, so the underline was only ever visible on the spurious nodes we now skip.pathPlaceholders.ts(new): sharedextractPathPlaceholdershelper with unit tests, used by bothHttpRequestPaneandWebsocketRequestPaneto keep the URL → params sync consistent.url.test.ts(new): grammar-level test documenting the lexer over-emission the chip filter relies on.Test plan
cargo test -p yaak-http path_placeholder— 9/9 pass, includes newplaceholder_followed_by_literal_colonvp test --runagainstapps/yaak-client/lib/pathPlaceholders.test.tsandapps/yaak-client/components/core/Editor/url/url.test.ts— 8/8 passtsc --noEmitinapps/yaak-client— cleannpm test— 148 passing across JS workspaces, no failurestarget/release/yaak-app-clientand verified the editor visually for/:abc:def,${[env]}:abc, and OpenAPI-imported requests (see Before / after above)