Skip to content

fix: Resolve : ambiguity in URL path placeholders#465

Open
simon446 wants to merge 2 commits into
mountain-loop:mainfrom
simon446:fix-openapi-colon-paths
Open

fix: Resolve : ambiguity in URL path placeholders#465
simon446 wants to merge 2 commits into
mountain-loop:mainfrom
simon446:fix-openapi-colon-paths

Conversation

@simon446
Copy link
Copy Markdown

@simon446 simon446 commented May 21, 2026

Summary

:name path 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:literal parsed 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:

Before After
Before — :cancel rendered as a spurious chip with underline After — :cancel correctly rendered as literal text

Submission

  • This PR is a bug fix or small-scope improvement.
  • I have read and followed CONTRIBUTING.md.
  • I tested this change locally.
  • I added or updated tests when reasonable.

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 :verb after 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 allows Placeholder PathSegment? so /:abc:def parses as Placeholder(:abc) + PathSegment(:def).

Resolver (crates/yaak-http/src/path_placeholders.rs): added : to the trailing-boundary regex set so :id in :id:literal substitutes correctly. One new unit test.

Editor:

  • pathParameters.ts: chip widgets only render for Placeholder nodes whose preceding document character is / — filters out the lexer's mid-segment over-emission and the long-standing env-var-followed-by-:abc quirk (${[var]}:abc no longer renders :abc as a spurious chip).
  • highlight.ts: dropped the Placeholder: t.emphasis style — chip widgets fully replace the placeholder text, so the underline was only ever visible on the spurious nodes we now skip.
  • pathPlaceholders.ts (new): shared extractPathPlaceholders helper with unit tests, used by both HttpRequestPane and WebsocketRequestPane to 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 new placeholder_followed_by_literal_colon
  • vp test --run against apps/yaak-client/lib/pathPlaceholders.test.ts and apps/yaak-client/components/core/Editor/url/url.test.ts — 8/8 pass
  • tsc --noEmit in apps/yaak-client — clean
  • npm test — 148 passing across JS workspaces, no failures
  • Manual smoke: built target/release/yaak-app-client and verified the editor visually for /:abc:def, ${[env]}:abc, and OpenAPI-imported requests (see Before / after above)

@simon446 simon446 force-pushed the fix-openapi-colon-paths branch from 954e471 to 11d6786 Compare May 21, 2026 20:57
@simon446 simon446 changed the title Fix : ambiguity in URL path placeholders fix: Resolve : ambiguity in URL path placeholders May 21, 2026
`: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>
@simon446 simon446 force-pushed the fix-openapi-colon-paths branch from 11d6786 to 7b7f1be Compare May 21, 2026 21:38
`/: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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant