|
| 1 | +# Shim-First Toolkit Improvements — Forge's Proposal |
| 2 | + |
| 3 | +**Date:** 2026-07-30 |
| 4 | +**By:** Forge (Lead / Web Forms Reviewer) |
| 5 | +**Status:** Proposed |
| 6 | +**Requested by:** Jeffrey T. Fritz |
| 7 | +**Builds on:** forge-shim-review.md (same date) |
| 8 | + |
| 9 | +--- |
| 10 | + |
| 11 | +## Executive Summary |
| 12 | + |
| 13 | +The shim-first approach is proven (~95% compile-as-is for page classes), but three detection gaps and one silent risk leave non-trivial migration failures at compile time and runtime. This proposal adds **4 CLI transform enhancements**, **2 skill documentation patches**, and **1 new non-page class detection strategy** — all surgical changes that extend the existing architecture without introducing new base classes or breaking changes. |
| 14 | + |
| 15 | +--- |
| 16 | + |
| 17 | +## 1. Proposed Changes — CLI Transforms |
| 18 | + |
| 19 | +### 1.1 ServerShimTransform: Detect Transfer/GetLastError/ClearError |
| 20 | + |
| 21 | +**File:** `src/BlazorWebFormsComponents.Cli/Transforms/CodeBehind/ServerShimTransform.cs` |
| 22 | +**Gap:** Server.Transfer(), Server.GetLastError(), Server.ClearError() are undetected; WingtipToys ErrorPage.aspx.cs, Default.aspx.cs, Global.asax.cs won't compile |
| 23 | +**Priority:** P0 (must-have — compile failure) |
| 24 | +**Complexity:** Small |
| 25 | + |
| 26 | +**What to change:** |
| 27 | + |
| 28 | +Add three new regex patterns after line 28: |
| 29 | + |
| 30 | +```csharp |
| 31 | +// Server.Transfer("page.aspx") — no Blazor equivalent; must become NavigationManager.NavigateTo |
| 32 | +private static readonly Regex ServerTransferRegex = new( |
| 33 | + @"\bServer\.Transfer\s*\(", |
| 34 | + RegexOptions.Compiled); |
| 35 | + |
| 36 | +// Server.GetLastError() — no direct equivalent; use IExceptionHandlerFeature or middleware |
| 37 | +private static readonly Regex ServerGetLastErrorRegex = new( |
| 38 | + @"\bServer\.GetLastError\s*\(\s*\)", |
| 39 | + RegexOptions.Compiled); |
| 40 | + |
| 41 | +// Server.ClearError() — no direct equivalent; errors flow through middleware pipeline |
| 42 | +private static readonly Regex ServerClearErrorRegex = new( |
| 43 | + @"\bServer\.ClearError\s*\(\s*\)", |
| 44 | + RegexOptions.Compiled); |
| 45 | +``` |
| 46 | + |
| 47 | +Update `Apply()` to detect these and emit specific TODO guidance: |
| 48 | + |
| 49 | +``` |
| 50 | +// TODO(bwfc-server): Server.Transfer() has NO shim — replace with Response.Redirect() or NavigationManager.NavigateTo(). |
| 51 | +// Server.Transfer preserves URL; for same effect use forceLoad:true on NavigateTo. |
| 52 | +// TODO(bwfc-server): Server.GetLastError() has NO shim — use IExceptionHandlerFeature in error-handling middleware. |
| 53 | +// TODO(bwfc-server): Server.ClearError() has NO shim — error handling uses middleware pipeline, remove this call. |
| 54 | +``` |
| 55 | + |
| 56 | +**Key detail:** These are NOT shimmable. The guidance must clearly say "manual rewrite required" — not "works via shim." |
| 57 | + |
| 58 | +--- |
| 59 | + |
| 60 | +### 1.2 SessionDetectTransform: Detect HttpContext.Current.Session |
| 61 | + |
| 62 | +**File:** `src/BlazorWebFormsComponents.Cli/Transforms/CodeBehind/SessionDetectTransform.cs` |
| 63 | +**Gap:** `HttpContext.Current.Session["key"]` pattern (5 occurrences in ShoppingCartActions.cs) not caught by existing `\bSession\s*\[` regex |
| 64 | +**Priority:** P0 (must-have — compile failure) |
| 65 | +**Complexity:** Small |
| 66 | + |
| 67 | +**What to change:** |
| 68 | + |
| 69 | +Add new regex after line 22: |
| 70 | + |
| 71 | +```csharp |
| 72 | +// HttpContext.Current.Session["key"] — common in non-page helper classes |
| 73 | +private static readonly Regex HttpContextSessionRegex = new( |
| 74 | + @"\bHttpContext\.Current\.Session\s*\[", |
| 75 | + RegexOptions.Compiled); |
| 76 | +``` |
| 77 | + |
| 78 | +Update `Apply()`: |
| 79 | +1. Check `HttpContextSessionRegex.IsMatch(content)` alongside existing `hasSession` |
| 80 | +2. When matched, **replace** `HttpContext.Current.Session[` → `Session[` (safe textual transform) |
| 81 | +3. Emit specific TODO: `// TODO(bwfc-session-state): HttpContext.Current.Session replaced with Session. This class needs [Inject] SessionShim or constructor injection — see non-page class guidance below.` |
| 82 | + |
| 83 | +Also detect `HttpContext.Current.` broadly for awareness: |
| 84 | + |
| 85 | +```csharp |
| 86 | +private static readonly Regex HttpContextCurrentRegex = new( |
| 87 | + @"\bHttpContext\.Current\b", |
| 88 | + RegexOptions.Compiled); |
| 89 | +``` |
| 90 | + |
| 91 | +--- |
| 92 | + |
| 93 | +### 1.3 ResponseRedirectTransform: Detect ThreadAbortException dead code |
| 94 | + |
| 95 | +**File:** `src/BlazorWebFormsComponents.Cli/Transforms/CodeBehind/ResponseRedirectTransform.cs` |
| 96 | +**Gap:** `Response.Redirect(url, true)` silently ignores `endResponse` param; `catch (ThreadAbortException)` blocks become unreachable dead code |
| 97 | +**Priority:** P1 (should-have — runtime risk, not compile failure) |
| 98 | +**Complexity:** Small |
| 99 | + |
| 100 | +**What to change:** |
| 101 | + |
| 102 | +Add regex after line 28: |
| 103 | + |
| 104 | +```csharp |
| 105 | +// catch (ThreadAbortException) — dead code in Blazor; Web Forms threw this on Redirect(url, true) |
| 106 | +private static readonly Regex ThreadAbortCatchRegex = new( |
| 107 | + @"catch\s*\(\s*ThreadAbortException\b", |
| 108 | + RegexOptions.Compiled); |
| 109 | + |
| 110 | +// Response.Redirect(url, true) — endResponse parameter silently ignored |
| 111 | +private static readonly Regex RedirectEndResponseRegex = new( |
| 112 | + @"Response\.Redirect\s*\([^,]+,\s*true\s*\)", |
| 113 | + RegexOptions.Compiled); |
| 114 | +``` |
| 115 | + |
| 116 | +Emit warnings: |
| 117 | +``` |
| 118 | +// TODO(bwfc-navigation): Response.Redirect(url, true) — the endResponse parameter is silently ignored. |
| 119 | +// In Web Forms, true caused ThreadAbortException to end page processing. |
| 120 | +// In Blazor, code after Redirect() always continues. Review control flow. |
| 121 | +// TODO(bwfc-navigation): catch (ThreadAbortException) is dead code — Blazor never throws this. Remove the catch block. |
| 122 | +``` |
| 123 | + |
| 124 | +--- |
| 125 | + |
| 126 | +### 1.4 New: NonPageClassDetectTransform |
| 127 | + |
| 128 | +**File:** `src/BlazorWebFormsComponents.Cli/Transforms/CodeBehind/NonPageClassDetectTransform.cs` (NEW) |
| 129 | +**Gap:** Classes like ShoppingCartActions.cs that use Session/Response/Server but don't inherit WebFormsPageBase get shim guidance that says "works via WebFormsPageBase" — which is wrong for them |
| 130 | +**Priority:** P1 (should-have) |
| 131 | +**Complexity:** Medium |
| 132 | + |
| 133 | +**What to change:** |
| 134 | + |
| 135 | +Create a new transform (Order: 50, runs early) that detects whether a class inherits from a page/component base: |
| 136 | + |
| 137 | +```csharp |
| 138 | +// Detect if class inherits WebFormsPageBase, ComponentBase, LayoutComponentBase, etc. |
| 139 | +private static readonly Regex PageBaseClassRegex = new( |
| 140 | + @"class\s+\w+\s*:\s*(?:.*?)(?:WebFormsPageBase|ComponentBase|LayoutComponentBase|Page)\b", |
| 141 | + RegexOptions.Compiled); |
| 142 | +``` |
| 143 | + |
| 144 | +When a file uses shim-relevant APIs (`Session[`, `Response.Redirect`, `Server.MapPath`, `Cache[`) but does NOT inherit a page base class, inject a **different** guidance block: |
| 145 | + |
| 146 | +``` |
| 147 | +// --- Non-Page Class: Manual DI Required --- |
| 148 | +// TODO(bwfc-non-page): This class uses Web Forms APIs but does NOT inherit WebFormsPageBase. |
| 149 | +// Shims are NOT automatically available. You must inject them: |
| 150 | +// |
| 151 | +// Option A: Constructor injection (recommended for services) |
| 152 | +// public class ShoppingCartActions |
| 153 | +// { |
| 154 | +// private readonly SessionShim _session; |
| 155 | +// public ShoppingCartActions(SessionShim session) => _session = session; |
| 156 | +// } |
| 157 | +// |
| 158 | +// Option B: [Inject] attribute (for Blazor components only) |
| 159 | +// [Inject] SessionShim Session { get; set; } |
| 160 | +// |
| 161 | +// Register in Program.cs: builder.Services.AddBlazorWebFormsComponents(); |
| 162 | +``` |
| 163 | + |
| 164 | +Store the `IsPageClass` result in `FileMetadata` (add a bool property) so downstream transforms can adjust their guidance between "works via WebFormsPageBase" vs. "needs manual DI." |
| 165 | + |
| 166 | +--- |
| 167 | + |
| 168 | +## 2. Proposed Changes — Migration Skills |
| 169 | + |
| 170 | +### 2.1 bwfc-migration SKILL.md: Add gap patterns |
| 171 | + |
| 172 | +**File:** `migration-toolkit/skills/bwfc-migration/SKILL.md` |
| 173 | +**Priority:** P1 |
| 174 | +**Complexity:** Small |
| 175 | + |
| 176 | +Add a new section after the shim table (after line 57): |
| 177 | + |
| 178 | +```markdown |
| 179 | +### ⚠️ Server Methods NOT Covered by ServerShim |
| 180 | + |
| 181 | +These `Server.*` methods have NO shim equivalent and require manual rewrite: |
| 182 | + |
| 183 | +| Pattern | Replacement | |
| 184 | +|---------|------------| |
| 185 | +| `Server.Transfer("page.aspx")` | `Response.Redirect("/page")` or `NavigationManager.NavigateTo("/page", forceLoad: true)` | |
| 186 | +| `Server.GetLastError()` | Error-handling middleware with `IExceptionHandlerFeature` | |
| 187 | +| `Server.ClearError()` | Remove — error handling is middleware-based | |
| 188 | +| `Server.Execute("page.aspx")` | Not applicable in Blazor — decompose into shared components | |
| 189 | + |
| 190 | +### ⚠️ Non-Page Classes Need Manual DI Wiring |
| 191 | + |
| 192 | +Classes that are not Blazor components (services, helpers, utilities) do NOT inherit WebFormsPageBase. They cannot use `Session["key"]` or `Response.Redirect()` directly. |
| 193 | + |
| 194 | +**Pattern to detect:** Any `.cs` file that uses `HttpContext.Current.Session`, `HttpContext.Current.Response`, or `HttpContext.Current.Server` — these must be refactored to accept shims via constructor injection. |
| 195 | + |
| 196 | +**Example (ShoppingCartActions.cs):** |
| 197 | +```csharp |
| 198 | +// Before: HttpContext.Current.Session["CartId"] |
| 199 | +// After: |
| 200 | +public class ShoppingCartActions |
| 201 | +{ |
| 202 | + private readonly SessionShim _session; |
| 203 | + public ShoppingCartActions(SessionShim session) => _session = session; |
| 204 | + |
| 205 | + public string GetCartId() => _session["CartId"]?.ToString(); |
| 206 | +} |
| 207 | +``` |
| 208 | +``` |
| 209 | + |
| 210 | +### 2.2 bwfc-data-migration SKILL.md: Add ThreadAbortException warning |
| 211 | + |
| 212 | +**File:** `migration-toolkit/skills/bwfc-data-migration/SKILL.md` |
| 213 | +**Priority:** P2 |
| 214 | +**Complexity:** Small |
| 215 | + |
| 216 | +In the "Static Helpers with HttpContext" section (line 492), add: |
| 217 | + |
| 218 | +```markdown |
| 219 | +### ThreadAbortException Is Dead Code |
| 220 | + |
| 221 | +In Web Forms, `Response.Redirect(url, true)` threw `ThreadAbortException` to halt page processing. Any `catch (ThreadAbortException)` blocks are dead code in Blazor — remove them. Review control flow after `Response.Redirect()` calls, as code after the redirect WILL execute in Blazor (it doesn't halt like Web Forms). |
| 222 | +``` |
| 223 | + |
| 224 | +--- |
| 225 | + |
| 226 | +## 3. Proposed Changes — Copilot Instructions Template |
| 227 | + |
| 228 | +### 3.1 copilot-instructions-template.md: Add gap warnings |
| 229 | + |
| 230 | +**File:** `migration-toolkit/copilot-instructions-template.md` |
| 231 | +**Priority:** P1 |
| 232 | +**Complexity:** Small |
| 233 | + |
| 234 | +Add to the "Common Gotchas" section (after item 10, line 216): |
| 235 | + |
| 236 | +```markdown |
| 237 | +11. **Server.Transfer has no shim** — Replace with `Response.Redirect()` or `NavigationManager.NavigateTo()`. Server.Transfer preserved the URL; use `forceLoad: true` for similar behavior. |
| 238 | +12. **Non-page classes need DI** — Service/helper classes that used `HttpContext.Current.Session` cannot use shims directly. Inject `SessionShim`, `ResponseShim`, etc. via constructor. |
| 239 | +13. **ThreadAbortException is dead code** — `catch (ThreadAbortException)` blocks after `Response.Redirect(url, true)` never execute in Blazor. Remove them and review control flow. |
| 240 | +14. **IdentityHelper.RedirectToReturnUrl** — This helper takes `System.Web.HttpResponse`, not `ResponseShim`. Rewrite to accept `NavigationManager` or `ResponseShim` parameter. |
| 241 | +``` |
| 242 | + |
| 243 | +--- |
| 244 | + |
| 245 | +## 4. What NOT to Do |
| 246 | + |
| 247 | +1. **❌ Do NOT create a new base class for non-page classes** (e.g., `WebFormsServiceBase`). Constructor DI is the correct pattern for services. A base class would create artificial coupling and fight against ASP.NET Core's DI model. |
| 248 | + |
| 249 | +2. **❌ Do NOT add Server.Transfer to ServerShim as a method.** Transfer implies server-side URL rewriting without browser redirect — a fundamentally incompatible concept in Blazor's component model. A `Transfer()` method on ServerShim would give false confidence. |
| 250 | + |
| 251 | +3. **❌ Do NOT auto-remove ThreadAbortException catch blocks.** The CLI should emit TODO guidance only — auto-removal could delete meaningful error-handling logic that the developer wrapped inside the catch block. |
| 252 | + |
| 253 | +4. **❌ Do NOT auto-inject `[Inject] SessionShim` into non-page classes.** The CLI cannot determine the correct DI pattern (constructor vs. property injection, scoping, lifetime). Emit guidance and let the developer or L2 Copilot decide. |
| 254 | + |
| 255 | +5. **❌ Do NOT add `HttpContext.Current` as a shim.** `HttpContext.Current` is a static accessor pattern that fundamentally doesn't work in async/Blazor contexts. The correct fix is to replace it with DI, not to shim it. |
| 256 | + |
| 257 | +--- |
| 258 | + |
| 259 | +## 5. Implementation Order |
| 260 | + |
| 261 | +| Order | Item | Priority | Depends On | Est. Effort | |
| 262 | +|-------|------|----------|------------|-------------| |
| 263 | +| 1 | SessionDetectTransform: `HttpContext.Current.Session` regex | P0 | None | 1 hour | |
| 264 | +| 2 | ServerShimTransform: Transfer/GetLastError/ClearError detection | P0 | None | 1 hour | |
| 265 | +| 3 | ResponseRedirectTransform: ThreadAbortException detection | P1 | None | 1 hour | |
| 266 | +| 4 | NonPageClassDetectTransform: new transform + FileMetadata.IsPageClass | P1 | #1-3 for full benefit | 3 hours | |
| 267 | +| 5 | bwfc-migration SKILL.md: gap patterns section | P1 | None | 30 min | |
| 268 | +| 6 | copilot-instructions-template.md: gotcha items | P1 | None | 15 min | |
| 269 | +| 7 | bwfc-data-migration SKILL.md: ThreadAbort warning | P2 | None | 15 min | |
| 270 | + |
| 271 | +Items 1-3 are independent and can be done in parallel. Item 4 benefits from 1-3 being done first (so the downstream transforms can check `IsPageClass`). Items 5-7 are documentation-only and can ship independently. |
| 272 | + |
| 273 | +--- |
| 274 | + |
| 275 | +## 6. Verification Plan |
| 276 | + |
| 277 | +- **Unit tests:** Add test cases to existing CLI transform test suite for each new regex pattern: |
| 278 | + - `Server.Transfer("ErrorPage.aspx")` → detected, TODO emitted |
| 279 | + - `HttpContext.Current.Session["CartId"]` → detected, rewritten, TODO emitted |
| 280 | + - `catch (ThreadAbortException)` → detected, TODO emitted |
| 281 | + - Non-page class with `Session[` → different guidance than page class |
| 282 | +- **Integration test:** Run CLI against WingtipToys source, verify all 3 gaps produce TODO comments |
| 283 | +- **Regression:** Existing 373 tests must continue passing |
0 commit comments