Skip to content

Commit 43237b3

Browse files
fix(router): address code review findings for query param allowlist
- Add Rust tests for allowed_query across the stack: - Protocol: round-trip with non-empty allowed_query - Parser: query attribute parsed into allowed_query field - Handler: matched/non-matched routes emit query attr in SSR - Handler: RouteChainEntry.to_json includes/omits allowedQuery - Handler: collect_route_chain carries allowed_query through - Update DESIGN.md with allowed_query proto field and query attr docs - Update VitePress docs: route.md, routing.md, ai.md with query attr - Extract toKebab to module-level helper (was duplicated 3x) - Add abort guard before content-type redirect in fetchPartial - Fix fetch mocks in unit tests to include headers for content-type check Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent e852ce0 commit 43237b3

File tree

14 files changed

+820
-6
lines changed

14 files changed

+820
-6
lines changed

DESIGN.md

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -188,6 +188,7 @@ pub struct WebUIFragmentRoute {
188188
pub fragment_id: String, // Fragment containing the route body
189189
pub exact: bool, // Require exact path match
190190
pub children: Vec<WebUIFragmentRoute>, // Nested child routes
191+
pub allowed_query: String, // Comma-separated allowlist of query params forwarded as attributes
191192
}
192193
```
193194

@@ -217,9 +218,14 @@ Child paths are relative to their parent (no leading `/`). The HTML nesting IS t
217218
<route path="lessons/:lessonId" component="lesson-page" exact />
218219
</route>
219220
</route>
221+
<route path="compose" component="compose-page" query="action,to,subject" exact />
220222
</route>
221223
```
222224

225+
The optional `query` attribute declares which URL query parameters are forwarded as HTML attributes
226+
on the component (deny-by-default). Routes without `query` forward no query params. Route path
227+
params always take priority over query params to prevent URL-based attribute injection.
228+
223229
**Route matching:** The handler uses an iterative path template matcher (no regex). Segments are
224230
compared left-to-right: `:param` binds a value, `*splat` captures remaining segments, `?` marks
225231
optional parameters. Exact matches (most literal segments) take precedence over parameterized ones.

LAZY_HYDRATION.md

Lines changed: 122 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,122 @@
1+
# Lazy SSR Hydration — Analysis & Implementation Notes
2+
3+
Status: **REVERTED** — needs more work before merging.
4+
5+
## The Idea
6+
7+
Defer ALL binding creation (text, attr, cond, repeat) until the first
8+
reactive property change triggers `$update()`. Wire events + refs eagerly
9+
at mount time for immediate interactivity. This matches Preact's approach:
10+
hydration only attaches event listeners.
11+
12+
## Performance Results (before revert)
13+
14+
| Metric | Eager (p50) | Lazy (p50) | Delta |
15+
|--------|:---:|:---:|:---:|
16+
| FCP | 44 ms | 40 ms | -9% |
17+
| TTI | 29 ms | 18 ms | -38% |
18+
| INP Primary | 12 ms | 12 ms | 0% |
19+
| INP Secondary | 13 ms | 12 ms | -8% |
20+
| JS Heap | 2,138 KB | 2,225 KB | +4% |
21+
22+
Light DOM results were excellent — **FCP 40ms vs Preact 52ms, TTI 18ms
23+
vs Preact 15ms**.
24+
25+
## Implementation (reverted from commit 894ce5d)
26+
27+
### What worked
28+
29+
1. New field `$ssrPending: { root, tplDom, savedState }` stores deferred
30+
hydration context
31+
2. `$mount()` SSR path: `$applySSRState()` + `$finalize()` (events only)
32+
3. Saved SSR state snapshot via `getObservableNames` iteration
33+
4. `$update()` detects `$ssrPending`, restores SSR state, runs `$hydrate`,
34+
restores current state, runs full `$updateInstance`
35+
5. `skipFinalize` parameter on `$hydrate` prevents double event wiring
36+
6. 133/134 tests passed — all light DOM and most shadow DOM tests worked
37+
38+
### What failed
39+
40+
**sidebar-repeat [shadow DOM] test**: Sets `page = 'favorites'` expecting
41+
`[data-active]` to move from "Dashboard" to "Favorites".
42+
43+
Root cause: After deferred `$hydrate` + `$updateInstance`, attr bindings
44+
INSIDE repeat items don't update when a non-collection property changes.
45+
46+
The `$updateInstance(this.$root)` triggers:
47+
1. Root-level text/attr patches — works
48+
2. Root-level cond toggles — works
49+
3. Root-level `syncRepeat` — runs but items array is unchanged
50+
4. `syncRepeat` reuses all instances, calls `$updateInstance` per item
51+
5. Item `$updateInstance` patches item attrs — **BUT** the attr binding
52+
`?data-active="{{nav.page == page}}"` resolves `page` via `$resolver`
53+
which reads `this.page` — this SHOULD work since we restored current
54+
state before `$updateInstance`
55+
56+
Suspected deeper issue: The `$resolver` closure captures `this` but may
57+
resolve against the wrong scope chain during the deferred hydration path.
58+
Or `syncRepeat`'s unkeyed fast path isn't calling `$updateInstance` at all
59+
when items haven't changed (it bails on `rep.synced && items.length === 0`
60+
checks, or the "reuse" path updates scope values but doesn't call update).
61+
62+
## What needs to be solved
63+
64+
1. **Repeat item updates on non-collection property changes**: When `page`
65+
changes, all repeat items that reference `page` in their bindings need
66+
to be updated. The path index indexes root-level bindings but NOT
67+
repeat item bindings. Currently this works because `syncRepeat` always
68+
runs and cascades `$updateInstance` — but something in the lazy path
69+
breaks this cascade.
70+
71+
2. **State snapshot/restore complexity**: Saving and restoring state via
72+
backing fields is fragile. If new observable types are added (e.g.,
73+
computed properties), the snapshot may miss them.
74+
75+
3. **Shadow DOM refs in conditionals**: `$wireRefs` during eager `$finalize`
76+
may not find refs inside conditional blocks that haven't been hydrated.
77+
Re-wiring after deferred hydration fixes this but adds complexity.
78+
79+
## Alternative approaches to explore
80+
81+
### A: Progressive hydration via requestIdleCallback
82+
Instead of deferring ALL bindings, hydrate during idle time after paint:
83+
```ts
84+
this.$finalize(root, meta, resolver); // events eagerly
85+
requestIdleCallback(() => {
86+
this.$root = this.$hydrate(root, meta, tplDom);
87+
});
88+
```
89+
No state snapshot needed — hydration happens with original state.
90+
Risk: if user interacts before idle callback fires.
91+
92+
### B: Component-level lazy (IntersectionObserver)
93+
Only hydrate components that are visible in the viewport:
94+
```ts
95+
if (this.getBoundingClientRect().top > window.innerHeight) {
96+
// Below fold — defer until scrolled into view
97+
const observer = new IntersectionObserver(([e]) => {
98+
if (e.isIntersecting) {
99+
this.$root = this.$hydrate(root, meta, tplDom);
100+
observer.disconnect();
101+
}
102+
});
103+
observer.observe(this);
104+
} else {
105+
this.$root = this.$hydrate(root, meta, tplDom);
106+
}
107+
```
108+
For mail app: only ~10 of 50 email rows are visible. Saves ~80% of
109+
email-row hydration.
110+
111+
### C: Skip repeat item hydration entirely
112+
For `<for>` loops, don't hydrate individual items at mount time. Only
113+
create repeat instances when the collection changes. Use `syncRepeat`
114+
to create fresh instances from scratch (clearing SSR content first).
115+
This avoids the complex SSR DOM matching for repeat items entirely.
116+
117+
### D: Partial path index that includes repeat item bindings
118+
Extend `$buildPathIndex` to also index bindings inside repeat items.
119+
When a non-collection property changes, the path index would directly
120+
target the affected repeat item bindings without going through
121+
`syncRepeat`. This is the most correct fix but increases path index
122+
build cost.

LAZY_HYDRATION_IMPLEMENTATION.md

Lines changed: 183 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,183 @@
1+
# Lazy Hydration — Implementation Reference
2+
3+
Status: **REVERTED** — kept for future reference.
4+
5+
## Approach
6+
7+
Component-level lazy hydration using a shared `IntersectionObserver`.
8+
SSR DOM stays intact and visible. Hydration runs when the element
9+
enters the viewport (with 200px margin). Opt-in via `static lazy = true`
10+
on any `WebUIElement` subclass.
11+
12+
## How it worked
13+
14+
### Module-level shared observer
15+
16+
One `IntersectionObserver` per document. A `Map<Element, () => void>`
17+
stores pending hydration callbacks. When an element intersects, its
18+
callback fires and the element is unobserved.
19+
20+
```ts
21+
const lazyCallbacks = new Map<Element, () => void>();
22+
let lazyObserver: IntersectionObserver | null = null;
23+
24+
function getLazyObserver(): IntersectionObserver {
25+
if (lazyObserver) return lazyObserver;
26+
lazyObserver = new IntersectionObserver(
27+
(entries) => {
28+
for (let i = 0; i < entries.length; i++) {
29+
const entry = entries[i];
30+
if (entry.isIntersecting) {
31+
const cb = lazyCallbacks.get(entry.target);
32+
if (cb) {
33+
lazyCallbacks.delete(entry.target);
34+
lazyObserver!.unobserve(entry.target);
35+
cb();
36+
}
37+
}
38+
}
39+
},
40+
{ rootMargin: '200px' },
41+
);
42+
return lazyObserver;
43+
}
44+
```
45+
46+
### Class field
47+
48+
```ts
49+
export class WebUIElement extends HTMLElement {
50+
private $lazyPending = false;
51+
// ... existing fields
52+
}
53+
```
54+
55+
### $mount() modification
56+
57+
Inside the `if (isSSR)` branch, before the normal `$applySSRState()` +
58+
`$hydrate()` path:
59+
60+
```ts
61+
if (isSSR) {
62+
const ctor = this.constructor as typeof WebUIElement & { lazy?: boolean };
63+
if (ctor.lazy && typeof IntersectionObserver !== 'undefined') {
64+
this.$lazyPending = true;
65+
this.$meta = meta;
66+
const observer = getLazyObserver();
67+
lazyCallbacks.set(this, () => {
68+
this.$lazyPending = false;
69+
this.$applySSRState();
70+
this.$root = this.$hydrate(root, meta, getTemplateDom(meta));
71+
this.$hydrated = true;
72+
this.$ready = true;
73+
});
74+
observer.observe(this);
75+
// Don't block TTI — lazy components balance the lifecycle counter immediately
76+
hydrationEnd(tag);
77+
return;
78+
}
79+
80+
// Normal eager SSR path
81+
this.$applySSRState();
82+
this.$root = this.$hydrate(root, meta, getTemplateDom(meta));
83+
}
84+
```
85+
86+
### disconnectedCallback cleanup
87+
88+
```ts
89+
disconnectedCallback(): void {
90+
if (this.$lazyPending) {
91+
lazyCallbacks.delete(this);
92+
if (lazyObserver) lazyObserver.unobserve(this);
93+
this.$lazyPending = false;
94+
}
95+
}
96+
```
97+
98+
### Usage
99+
100+
```ts
101+
class EmailRow extends WebUIElement {
102+
static lazy = true;
103+
// ...
104+
}
105+
```
106+
107+
## What it cost
108+
109+
- ~300 bytes bundle size increase
110+
- One `IntersectionObserver` allocation (shared across all lazy elements)
111+
- Per lazy element: one `Map.set()` + one `observer.observe()` call
112+
113+
## What it improved
114+
115+
| Metric | mail-webui | mail-webui-light |
116+
|--------|:---:|:---:|
117+
| Event listeners | -32% (532→360) | -55% (622→282) |
118+
| JS Heap Used | -1.8% | -2.0% |
119+
| JS Script Duration | -5% | -3% |
120+
121+
## Why it was reverted
122+
123+
### 1. No FCP/LCP improvement
124+
125+
FCP and LCP measure when the browser paints pixels. Paint happens from
126+
SSR HTML before any JS runs. Lazy hydration only defers JS work that
127+
happens after paint — it cannot improve FCP/LCP by design.
128+
129+
### 2. Breaks repeat children
130+
131+
When a parent component updates an array (e.g., adding a todo item),
132+
`syncRepeat` sets attributes on all child instances. For lazy children
133+
with `$ready = false`, the `$update()` call is silently dropped. The
134+
SSR DOM goes stale, causing visual mismatches.
135+
136+
Attempted fix: force-hydrate lazy children when `$update()` is called.
137+
This triggered full hydration on every repeat child during every array
138+
update — worse than no lazy at all.
139+
140+
### 3. IntersectionObserver misses scroll containers
141+
142+
`IntersectionObserver` checks viewport intersection, not scroll position
143+
within a parent container. In the todo app with 1000 items in a scrollable
144+
`<div>`, ALL items register as "in viewport" because their host element
145+
is visible — even though 990 items are scrolled out of view.
146+
147+
### 4. TTI regression for shadow DOM
148+
149+
The async nature of `IntersectionObserver` added a layout cycle for
150+
visible lazy components. Shadow DOM TTI regressed +39% (12→17ms).
151+
Light DOM was slightly better (-6%).
152+
153+
## Lessons learned
154+
155+
1. **Component-level lazy is the wrong granularity.** The expensive work
156+
is in repeat item hydration (1000 binding sets), not component hydration
157+
(one component, one set of bindings).
158+
159+
2. **The right approach is virtualized repeat** — only create repeat
160+
instances for visible items. See `TODO_RENDER_PERF.md`.
161+
162+
3. **Never silently drop `$update()` calls.** If a component receives a
163+
property change, it must either process it or queue it — not ignore it.
164+
165+
4. **`hydrationEnd()` must fire immediately** for deferred components,
166+
or TTI measurement is blocked.
167+
168+
## Test coverage that was created
169+
170+
16 tests were written covering:
171+
172+
- **Core behavior (4):** below-fold deferral, scroll-into-view hydration,
173+
observer async behavior, SSR DOM preservation
174+
- **Lifecycle (3):** disconnect-before-hydrate cleanup, reconnect
175+
re-observation, observer disconnect after hydration
176+
- **State/reactivity (3):** SSR state preservation, pre-hydration property
177+
updates, reactive updates after lazy hydration
178+
- **Interaction (2):** events wired on hydrate, conditional hydration
179+
- **Integration (3):** non-lazy sibling unaffected, shared observer
180+
verification, bulk scroll hydration
181+
182+
These tests passed but were removed with the revert since the feature
183+
code was removed.

TODO_RENDER_PERF.md

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
# TODO: Render Performance Improvements
2+
3+
## Virtualized Repeat (`<for virtualize>`)
4+
5+
Instead of hydrating all repeat items, only create repeat instances for visible items.
6+
7+
### Problem
8+
For large lists (1,000+ items), the framework creates bindings, scope objects, and DOM nodes
9+
for every item — even those the user will never scroll to see. This dominates TTI and memory.
10+
11+
### Proposed approach
12+
- Server SSR renders all items (for SEO/FCP)
13+
- At hydration, `syncRepeat` with `virtualize` flag:
14+
1. Measures container height and item height
15+
2. Keeps only ~15 DOM nodes (visible window + small buffer)
16+
3. Removes the other 985 SSR nodes from DOM
17+
4. On scroll, recycles DOM nodes with new data (no create/destroy)
18+
19+
### Expected savings (1,000 items)
20+
- Bindings: 45 instead of 3,000
21+
- Event listeners: 30 instead of 2,000
22+
- DOM nodes: ~75 instead of ~5,000
23+
- Memory: dramatically less
24+
25+
### Design notes
26+
- Opt-in via `virtualize` attribute on `<for>` in template
27+
- Requires known/estimated item height for positioning
28+
- Scroll handler recycles DOM nodes by updating scope data
29+
- Should work with both keyed and unkeyed repeat
30+
- Must handle dynamic item heights (measure after render, cache)
31+
32+
### Prior art
33+
- React Virtuoso, TanStack Virtual
34+
- Lit `@lit-labs/virtualizer`
35+
- iOS `UITableView` / Android `RecyclerView`

0 commit comments

Comments
 (0)