refactor!: centralize fluent factories, shared UI helpers, payload hydration, formatting, and the reduced panel URL contract. - #44
Merged
Conversation
…dration, formatting, and the reduced panel URL contract.
|
Important Review skippedToo many files! This PR contains 121 files, which is 21 over the limit of 100. To get a review, reduce the PR to 100 files or fewer by splitting it into smaller PRs or changing its base branch. Upgrade to a paid plan to raise the limit. This review couldn't start because sufficient usage credits or metered capacity aren't available. Add credits or update usage-based reviews in the billing tab, then retry. ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (121)
You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #44 +/- ##
=============================================
+ Coverage 99.95% 100.00% +0.04%
+ Complexity 2191 2156 -35
=============================================
Files 164 171 +7
Lines 8694 8505 -189
=============================================
- Hits 8690 8505 -185
+ Misses 4 0 -4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🟡 Changes recommended
A critical seeder failure and unresolved API, timestamp, provenance, and documentation issues must be addressed.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Centralizes fluent factories, payload hydration, formatting, comparison models, and reusable UI helpers while narrowing URL and provenance contracts.
Changes:
- Introduces shared rendering, formatting, hydration, comparison, and fixture utilities.
- Migrates models and tests to fluent factories.
- Reduces several routing, construction, and view-model APIs.
Review findings:
- Critical (2 votes):
RequestSummary’s private constructor breaks the fixture seeder and E2E setup; migrate the remaining direct construction. - Moderate (2 votes): Preserve route-inventory provenance so captured configuration is not labeled as live.
- Moderate (1 vote): Correct negative fractional epoch formatting.
- Moderate (1 vote): Mark private constructors as breaking and document factory migration.
- Nit (2 votes): Update
SnapshotComparisondocumentation to disclose retention of complete snapshots.
File summaries
| File | Description |
|---|---|
tests/View/Sidebar/SidebarSnapshotTest.php |
Tests snapshot factory defaults. |
tests/View/History/HistorySummaryTest.php |
Uses summary factories. |
tests/View/History/HistoryScaleTest.php |
Uses shared summary fixtures. |
tests/View/History/HistoryRowTest.php |
Uses shared summary fixtures. |
tests/View/History/HistoryCellRendererTest.php |
Consolidates row fixtures. |
tests/Toolbar/ToolbarPanelTest.php |
Tests fluent panel construction. |
tests/Toolbar/ToolbarItemTest.php |
Tests fluent item construction. |
tests/Toolbar/ToolbarInjectorTest.php |
Tests toolbar insertion. |
tests/Toolbar/ToolbarDataTest.php |
Migrates toolbar fixtures. |
tests/Support/RequestSummaryFixture.php |
Adds canonical summary fixtures. |
tests/Support/MessageCatalogTestCase.php |
Shares message-catalog assertions. |
tests/Support/JobRecordFixture.php |
Adds queue-record fixtures. |
tests/Storage/SnapshotStoreTest.php |
Migrates summary setup. |
tests/Storage/RequestSummaryTest.php |
Tests factory defaults and hydration. |
tests/Storage/PayloadTest.php |
Tests list and nullable-boolean hydration. |
tests/Storage/ManifestTest.php |
Migrates summary setup. |
tests/Storage/DebugSnapshotTest.php |
Migrates summary setup. |
tests/Provider/ToolbarPanelProvider.php |
Extracts toolbar test data. |
tests/Provider/PayloadDifferenceProvider.php |
Extracts comparison cases. |
tests/Panel/Vite/ViteSectionRendererTest.php |
Expands shared-table assertions. |
tests/Panel/Request/RequestRoutingViewModelsTest.php |
Updates reduced provenance API tests. |
tests/Panel/Request/RequestRendererTest.php |
Updates routing rendering expectations. |
tests/Panel/Request/RequestHeadersRendererTest.php |
Verifies shared ledger classes. |
tests/Panel/Request/RequestDiagnosticLedgerTest.php |
Tests the diagnostic ledger. |
tests/Panel/Request/RequestDataNormalizerTest.php |
Uses shared summary fixtures. |
tests/Panel/Profile/ProfileMessageTest.php |
Uses shared catalog assertions. |
tests/Panel/PanelRenderContextTest.php |
Tests the reduced URL contract. |
tests/Panel/PanelMessageTest.php |
Uses shared catalog assertions. |
tests/Panel/Log/LogMessageTest.php |
Uses shared catalog assertions. |
tests/Panel/Inertia/InertiaMessageTest.php |
Uses shared catalog assertions. |
tests/Panel/Event/EventMessageTest.php |
Uses shared catalog assertions. |
tests/Panel/Db/NPlusOneDetectorTest.php |
Removes an obsolete API assertion. |
tests/Panel/Db/DbQueryRendererTest.php |
Tests EXPLAIN behavior through rendering. |
tests/Panel/Db/DbExplainRendererTest.php |
Tests numeric table headings. |
tests/Panel/Asset/AssetCardRendererTest.php |
Tests the simplified card API. |
tests/Helper/TableTest.php |
Tests shared table rendering. |
tests/Helper/FormatTest.php |
Tests centralized formatting; the negative epoch expectation must be corrected. |
tests/Helper/ExtensionPillTest.php |
Tests extension pills. |
tests/Helper/BadgeTest.php |
Tests shared badges. |
tests/Helper/AvatarTest.php |
Tests monogram initials. |
tests/Data/QueryInputTest.php |
Tests numeric lower bounds. |
tests/Data/PageSizeTest.php |
Tests the selector facade. |
tests/Comparison/SummaryMetricComparisonTest.php |
Tests metric difference detection. |
tests/Comparison/SnapshotComparisonTest.php |
Tests aggregate comparisons and snapshot retention. |
tests/Comparison/PayloadDifferenceTest.php |
Uses an external comparison provider. |
tests/Comparison/PanelComparisonTest.php |
Tests difference totals. |
tests/Collector/CollectorCoordinatorTest.php |
Migrates summary construction. |
src/View/Sidebar/SidebarSnapshot.php |
Restricts construction to factories. |
src/View/History/HistoryCellRenderer.php |
Uses shared duration formatting. |
src/Toolbar/ToolbarPanel.php |
Makes construction factory-only. |
src/Toolbar/ToolbarItem.php |
Makes construction factory-only. |
src/Toolbar/ToolbarInjector.php |
Adds shared toolbar injection. |
src/Toolbar/ToolbarData.php |
Trims redundant documentation. |
src/Storage/SnapshotStore.php |
Extracts shared read and error handling. |
src/Storage/RequestSummary.php |
Makes construction factory-only, but the fixture seeder still calls the private constructor. |
src/Storage/Payload.php |
Adds list and nullable-boolean hydration. |
src/Storage/ExceptionSnapshot.php |
Uses shared coercion and FQCN helpers. |
src/Storage/DebugArray.php |
Trims redundant documentation. |
src/Routing/DebugUrlGeneratorInterface.php |
Narrows URL generation to panels. |
src/PhpInfo/PhpInfoRenderer.php |
Uses shared extension pills. |
src/Panel/Vite/ViteSectionRenderer.php |
Uses shared badges and tables. |
src/Panel/Vite/ViteComponent.php |
Uses shared payload hydration. |
src/Panel/User/UserRbacRow.php |
Uses shared coercion. |
src/Panel/User/UserDataNormalizer.php |
Uses avatar and time helpers. |
src/Panel/Timeline/TimelineMemoryRenderer.php |
Extracts point tracing. |
src/Panel/Router/RouterSectionRenderer.php |
Uses shared tables, badges, and rows. |
src/Panel/Router/CurrentRouteLogRow.php |
Uses shared string coercion. |
src/Panel/Request/Routing/RouteInventoryView.php |
Removes live-state tracking, losing required provenance. |
src/Panel/Request/RequestToolbarItemFactory.php |
Uses fluent toolbar factories. |
src/Panel/Request/RequestServerRenderer.php |
Uses the shared diagnostic ledger. |
src/Panel/Request/RequestSectionRenderer.php |
Shares method and table rendering. |
src/Panel/Request/RequestRoutesRenderer.php |
Uses shared UI helpers; its live-data warning must remain provenance-aware. |
src/Panel/Request/RequestRenderer.php |
Reuses method-pill rendering. |
src/Panel/Request/RequestHeadersRenderer.php |
Uses the shared diagnostic ledger. |
src/Panel/Request/RequestDiagnosticLedger.php |
Adds a shared ledger builder. |
src/Panel/Queue/QueueSnapshot.php |
Uses shared list hydration. |
src/Panel/Queue/QueuePill.php |
Adds shared queue pills. |
src/Panel/Queue/QueueGridRenderer.php |
Uses shared pills and formatting. |
src/Panel/Queue/QueueCardRenderer.php |
Uses shared avatar and pill helpers. |
src/Panel/Profile/ProfileCellRenderer.php |
Uses shared timestamp formatting. |
src/Panel/PanelRenderContext.php |
Removes history and action URLs. |
src/Panel/Mail/MailSnapshot.php |
Uses shared list hydration. |
src/Panel/Mail/MailCardRenderer.php |
Uses shared avatar and time helpers. |
src/Panel/Log/LogSnapshot.php |
Uses shared list hydration. |
src/Panel/Log/LogCellRenderer.php |
Uses shared timestamp formatting. |
src/Panel/Event/EventSnapshot.php |
Uses shared list hydration. |
src/Panel/Event/EventCellRenderer.php |
Uses shared badge and time helpers. |
src/Panel/Dump/DumpSnapshot.php |
Uses shared list hydration. |
src/Panel/Dump/DumpCardRenderer.php |
Uses shared timestamp formatting. |
src/Panel/Db/NPlusOneFinding.php |
Removes an unused membership API. |
src/Panel/Db/DbSnapshot.php |
Uses shared list hydration. |
src/Panel/Db/DbQueryRenderer.php |
Centralizes formatting and internalizes EXPLAIN detection. |
src/Panel/Db/DbExplainRenderer.php |
Uses shared table rendering. |
src/Panel/Config/ConfigCardRenderer.php |
Uses shared extension pills. |
src/Panel/Asset/AssetSectionRenderer.php |
Uses the reduced card API. |
src/Panel/Asset/AssetCardRenderer.php |
Derives dependency anchors locally. |
src/Helper/Table.php |
Adds shared table construction. |
src/Helper/SensitiveDataRedactor.php |
Exposes pattern resolution. |
src/Helper/Format.php |
Adds shared formatting, but mishandles negative fractional epochs. |
src/Helper/ExtensionPill.php |
Adds an extension-pill helper. |
src/Helper/Badge.php |
Adds a shared badge helper. |
src/Helper/Avatar.php |
Adds monogram generation. |
src/Data/QueryInput.php |
Adds minimum-bound parsing. |
src/Data/PageSize.php |
Makes the low-level selector renderer private. |
src/Data/FilterPrefix.php |
Documents filter constants. |
src/Comparison/SummaryMetricComparison.php |
Adds a difference predicate. |
src/Comparison/SnapshotComparison.php |
Aggregates comparisons; documentation must disclose complete snapshot retention. |
src/Comparison/PanelComparison.php |
Adds difference totals. |
src/Capture/CapturePolicy.php |
Centralizes redaction-pattern resolution. |
resources/views/snapshot.php |
Extracts danger-callout construction. |
README.md |
Documents revised contracts, but must identify private constructors as breaking and explain factory migration. |
package.json |
Removes Markdown formatting targets. |
docs/browser-quality.md |
Removes browser workflow documentation. |
CHANGELOG.md |
Records the refactor. |
.gitignore |
Ignores internal documentation. |
Review details
- Files reviewed: 118/119 changed files
- Comments generated: 5
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🔵 Needs a closer look
Broad breaking API changes require human review, and the formatting tests remain timezone-dependent.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
tests/Helper/FormatTest.php:125
- These expectations hard-code UTC even though
Format::timeOfDay()intentionally delegates the date portion to PHP's current default timezone. On a non-UTC developer machine this method (and the custom-format/negative-timestamp tests below) fails—for example, this timestamp renders as17:13:20.123inAmerica/New_York. Derive the date portion withdate()as the existing renderer tests do, or temporarily set and restore UTC within these tests.
README.md:28
- The architecture section now says both that Debug Core “does not … inject toolbar markup” (line 22) and that its new
ToolbarInjectorplaces markup before</body>. Clarify the boundary by saying Core does not decide when injection occurs; adapters retain that lifecycle decision while using the Core helper.
(summary metrics and panel payloads) for the history comparison pages, and `PHPForge\Debug\Toolbar\ToolbarInjector`
places the rendered toolbar before `</body>`. Adapters provide a
- Files reviewed: 119/120 changed files
- Comments generated: 0 new
- Review effort level: Balanced
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.
Pull Request