Repository navigation
fix(pages): keep the named action key out of c.redirect() query merging - #522
Merged
Brooooooklyn merged 3 commits intoOct 21, 2026
Merged
Conversation
c.redirect() merges the request query into internal destinations. Inside a named action the first query key only selects the action, so redirecting from POST /books?delete to /books ended up at /books?delete. Drop that key before merging and keep the rest of the query.
The action is picked by the first query key, so remove only that first non-empty segment and keep the rest of the query as sent. Deleting the key with URLSearchParams also dropped a page param with the same name (?delete&delete=x) and re-encoded the rest (%20 -> +, flag -> flag=), unlike the same redirect from a single action. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Member
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Brooooooklyn
self-requested a review
October 21, 2026 07:05
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Brooooooklyn
approved these changes
Oct 21, 2026
Brooooooklyn
added a commit
that referenced
this pull request
Oct 21, 2026
…writes and the page URL (#530) Follow-ups to #522. That PR dropped the action key (`?delete`) only from `c.redirect()` inside the action. This covers the other places it leaked. ## What changes | Case: `POST /books?delete&page=2` | Before | After | |---|---|---| | Auth / user middleware `c.redirect('/login')` | `/login?delete&page=2` | `/login?page=2` | | Action `c.rewrite('/api/x')`, then `c.redirect('/done')` | `/done?delete&page=2` | `/done?page=2` | | `c.redirect('/books-v2', 307)` (or 308) | `/books-v2?page=2` (since #522) | `/books-v2?delete&page=2` | | `PUT /books?update` + `c.redirect('/books')` (302) | `/books` (since #522) | `/books?update` | | API route under a catch-all named-action page: `POST /api/x?return=/a` + redirect | `?return` dropped (earlier commit here) | `?return=/a` kept | | Action re-renders the page: `page.url` | `/books?delete&page=2` | `/books?page=2` | | …address bar after `useForm` / `action()` | `/books?delete&page=2` | `/books?page=2` | The key goes only when the browser follows with a GET (303, or 301/302 after a POST). Other redirects send the same method again, and the destination needs the key to pick the action. Redirect and rewrite **rules** (`void.json` routing) are unchanged: they keep the query as sent. On Void Cloud, platform dispatch applies them before the Worker and knows nothing about pages, so every target leaves them alone. ## How ``` compile.ts app.use(createNamedActionMarker(matchedRoutes)) ← after routing rules, before auth and user middleware matchedRoutes comes from the app's own resolved hono/route (Hono keys it by a per-copy Symbol) marks only if the first matched Void route handler (tagged: API, page GET, page mutation) is a named-action page; user middleware of any arity is skipped routing.ts c.redirect: drop the first query segment if marked and the browser follows with a GET c.rewrite: no new query → mark the new request too protocol.ts mutation page.url: drop the key if marked client after a POST that renders: if page.url's query == our URL minus its first segment, write that to history pages/action-key.ts shared WeakSet + string helper (no regex). Rolldown drops the WeakSet from client bundles. ``` Docs: the actions guide now says what happens to the query. The rewrites reference mentions middleware and 307/308. ## Flaky tests (second commit) Each of these failed in a local full run under load: | Test | Cause | Fix | |---|---|---| | `plugin-class-a-wrapper` `… during build` | macOS reports `server.mjs`, written just before startup, as changed (2 of 6 runs in a probe). The reload gives the second import a new `Counter`. | `watch: null` (12 of 12 runs stable) | | `plugin-vite-environment` #472 test | Real dev server: about 1 s alone, >5 s under load | `watch: null`, lighter schema file, 30 s limit | | `plugin-entry-imports` | Imports the whole plugin entry: about 4.2 s alone | 30 s limit | ## Tests - `routing-runtime.test.ts`: middleware, 307, rewrite, rewrite with its own query, and page URL. 5 rows fail on main. - `pages-client.test.ts`: history URL after a POST, 4 rows. 2 fail on main; the other 2 check that single-action URLs stay unchanged. - `pages-compile.test.ts`: the marker is emitted after `routingMiddleware` and before user middleware, only for `actions`. - `vp run test --unit`: all pass, except one `cf-version-runner` test that timed out under load and passes alone (not touched here). Lint, format and typecheck are clean. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 (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.
Fixes #520
In a named action, drop the action key before
c.redirect()merges the request query.POST /books?delete&page=2→c.redirect('/books')now goes to/books?page=2. Other handlers keep the current behavior.createPageMutationHandlermarks requests that picked a named action (aWeakSet). Thec.redirectwrapper then removes only the first non-empty query segment (the one that picked the action) and keeps the rest exactly as sent. Also adds one sentence toreference/api/rewrites.md.Checked:
routing-runtime.test.ts: anit.eachtable (7 rows: valued key, leading&, a page param with the same name, encoding kept, single action unchanged). 6 rows fail without the fix.void, lint, fmt22 unit tests fail locally on main too. This PR adds no new failures.