Conversation
Co-authored-by: MCGPPeters <3175802+MCGPPeters@users.noreply.github.com>
…-386f3e556006 Add comprehensive GitHub Copilot instructions for efficient agent development
…bility; generic debug bridge; E2E hardening
There was a problem hiding this comment.
Pull Request Overview
This PR focuses on improving E2E test stability for the Abies WebAssembly framework by addressing race conditions, event handling issues, and DOM update timing problems that were causing flaky test failures.
Key changes include:
- Enhanced JavaScript runtime with better event handling and debug capabilities
- Improved DOM attribute handling for boolean properties and form elements
- Better synchronization between client-side state updates and server-side operations
- More robust E2E test patterns with proper waiting strategies
Reviewed Changes
Copilot reviewed 45 out of 49 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| Abies/wwwroot/abies.js | Enhanced event handling, debug bridge, improved attribute updates, and better error handling |
| Abies/Runtime.cs | Refactored handler registration, added graceful error handling, and improved command processing |
| Abies/DOM/Operations.cs | Enhanced DOM diffing with handler lifecycle management and atomic updates |
| Multiple E2E test files | Updated test patterns with better synchronization and more reliable selectors |
| API and configuration files | Updated service configurations, ports, and project references |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| if (typeof window !== 'undefined' && !window.__abiesDebug) { | ||
| try { | ||
| window.__abiesDebug = { logs: [], registeredEvents: [], consoleEnabled: false }; | ||
| } catch (e) { /* ignore */ } | ||
| } | ||
|
|
There was a problem hiding this comment.
[nitpick] The debug object initialization could be extracted to a separate function to improve readability and testability. Consider creating initializeDebugBridge() function that handles the debug object setup.
| if (typeof window !== 'undefined' && !window.__abiesDebug) { | |
| try { | |
| window.__abiesDebug = { logs: [], registeredEvents: [], consoleEnabled: false }; | |
| } catch (e) { /* ignore */ } | |
| } | |
| function initializeDebugBridge() { | |
| if (typeof window !== 'undefined' && !window.__abiesDebug) { | |
| try { | |
| window.__abiesDebug = { logs: [], registeredEvents: [], consoleEnabled: false }; | |
| } catch (e) { /* ignore */ } | |
| } | |
| } | |
| initializeDebugBridge(); |
| const lower = propertyName.toLowerCase(); | ||
| const isBooleanAttr = ( | ||
| lower === 'disabled' || lower === 'checked' || lower === 'selected' || lower === 'readonly' || | ||
| lower === 'multiple' || lower === 'required' || lower === 'autofocus' || lower === 'inert' || | ||
| lower === 'hidden' || lower === 'open' || lower === 'loop' || lower === 'muted' || lower === 'controls' | ||
| ); |
There was a problem hiding this comment.
The boolean attribute check is duplicated across multiple functions (updateAttribute, addAttribute, removeAttribute). This logic should be extracted to a shared function like isBooleanAttribute(propertyName) to reduce code duplication and improve maintainability.
| if (_handlers.TryGetValue(messageId, out var message)) | ||
| { | ||
| throw new InvalidOperationException("Message to dispatch not found"); | ||
| var _ = Dispatch(message); | ||
| return; | ||
| } | ||
|
|
||
| var _ = Dispatch(message); | ||
| // Missing handler can occur during DOM replacement; ignore gracefully | ||
| System.Diagnostics.Debug.WriteLine($"[Abies] Missing handler for messageId={messageId}"); |
There was a problem hiding this comment.
[nitpick] The variable name _ is used to discard the return value, but consider using the discard pattern _ = Dispatch(message); more explicitly or add a comment explaining why the return value is intentionally ignored.
| var q = raw.IndexOf('?'); | ||
| var baseUrl = q >= 0 ? raw.Substring(0, q) : raw; | ||
| if (baseUrl.EndsWith('/')) baseUrl = baseUrl.TrimEnd('/'); |
There was a problem hiding this comment.
[nitpick] Consider using raw.Split('?')[0] instead of IndexOf and Substring for better readability, or use Uri class methods for more robust URL parsing.
| var q = raw.IndexOf('?'); | |
| var baseUrl = q >= 0 ? raw.Substring(0, q) : raw; | |
| if (baseUrl.EndsWith('/')) baseUrl = baseUrl.TrimEnd('/'); | |
| var uri = new Uri(raw); | |
| var baseUrl = uri.GetLeftPart(UriPartial.Path); | |
| if (baseUrl.EndsWith('/')) baseUrl = baseUrl.TrimEnd('/'); |
| try | ||
| { | ||
| // Filter only logs related to data-current-page updates to reduce noise | ||
| var dbgFiltered = await page.EvaluateAsync<string>("() => { try { const d = window.__abiesDebug || {}; const logs = Array.isArray(d.logs) ? d.logs.filter(l => (l.type === 'updateAttribute' || l.type === 'addAttribute') && l.propertyName === 'data-current-page') : []; return JSON.stringify({ filteredLogs: logs, registeredEvents: d.registeredEvents || [] }); } catch(e) { return 'eval-failed:' + e.message; } }"); |
There was a problem hiding this comment.
This inline JavaScript is extremely long and complex. Consider extracting this debugging logic to a separate JavaScript file or breaking it into smaller, more manageable pieces for better readability and maintainability.
| await page.TypeAsync("input[placeholder='Enter tags']", "e2etag"); | ||
| await page.GotoAsync(_fixture.AppBaseUrl + "/register"); | ||
| await page.WaitForFunctionAsync("() => window.abiesReady === true"); | ||
| var username = $"tag{System.Guid.NewGuid():N}".Substring(0, 16); |
There was a problem hiding this comment.
[nitpick] The username generation pattern is repeated across multiple test files. Consider extracting this to a shared utility method like TestHelpers.GenerateTestUsername(string prefix) to reduce code duplication.
… not run E2E" This reverts commit 7d5bcda.
… URL/readiness sync; Conduit: fix tag preview click behavior; CI: use npx playwright install --with-deps in e2e workflow; no runtime app-specific logic.
…flow, auth reload UX; align date formatting (ISO API + human UI); CSS/nav polish and minor copy; AppHost run scripts support
No description provided.