Repository navigation
Report the error screen's owner, not whether Tracy is installed - #12
Merged
Merged
Conversation
The Diagnostics page answered "is Tracy installed?" and presented it as the error screen row. That was never quite the question, and on XOOPS 2.7.3 it is the wrong one: the site declares an owner, core publishes who actually holds the handlers, and the two can differ — which is the case most worth showing. The row now reads `XOOPS_ERROR_SCREEN_OWNER`, `_SOURCE` and `_STATUS` and reports what core says, including the statuses that mean something is wrong: `dormant`, `missing`, `disabled`, `contested`, `error`. On a core without the seam the constants are absent and the row falls back to its previous behaviour. Requires the seam from XOOPS 2.7.3 (XOOPS/XoopsCore27 `feat/error-screen-provider`) to show anything new.
Reviewer's GuideDiagnostics now reports the actual error-screen owner and status via XOOPS 2.7.3 core seam constants, adds a dedicated row label for it, keeps Tracy as a plain package check, and bumps module version/changelog for the release. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The hardcoded status labels in
errorScreenRow()(e.g. 'Dormant', 'Disabled', 'Failed to start') make the diagnostics UI non-localizable; consider moving these user-facing strings into language constants similar to_AM_DEBUGBAR_DIAG_ERROR_SCREEN. - In
errorScreenRow()you special-case only when$source !== '' && $owner !== 'core'; if$sourceis empty but$owneris non-core you lose potentially useful context, so you may want to always append the owner when it differs from 'core', and conditionally add the source only when available.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The hardcoded status labels in `errorScreenRow()` (e.g. 'Dormant', 'Disabled', 'Failed to start') make the diagnostics UI non-localizable; consider moving these user-facing strings into language constants similar to `_AM_DEBUGBAR_DIAG_ERROR_SCREEN`.
- In `errorScreenRow()` you special-case only when `$source !== '' && $owner !== 'core'`; if `$source` is empty but `$owner` is non-core you lose potentially useful context, so you may want to always append the owner when it differs from 'core', and conditionally add the source only when available.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Greptile SummaryThe PR updates Diagnostics to report the active error-screen owner while retaining compatibility with legacy Tracy status reporting.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains; the previously reported loss of legacy Tracy bootstrap status is fixed by mapping
|
| Filename | Overview |
|---|---|
| class/Analysis/SystemDiagnostics.php | Adds provider-neutral error-screen diagnostics and correctly restores the legacy XOOPS_TRACY_STATUS fallback raised in the previous review. |
| admin/diagnostics.php | Adds the localized label for the new error-screen diagnostics row. |
| admin/index.php | Changes Tracy runtime updates from wholesale replacement to merge-preserving writes with a locked legacy fallback. |
| language/english/admin.php | Defines the user-facing label for the error-screen diagnostics row. |
| xoops_version.php | Bumps the module release metadata to version 1.4.1. |
| CHANGELOG.md | Documents the provider-neutral diagnostics behavior, legacy compatibility, and runtime override preservation. |
| docs/changelog.txt | Mirrors the 1.4.1 release notes in the packaged changelog. |
Reviews (3): Last reviewed commit: "fix(admin): fail closed when debug-runti..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughDiagnostics now report XOOPS error-screen ownership, status, source, and message. Tracy has a separate package check. Tracy runtime toggles preserve existing debug-runtime data. Release metadata identifies version 1.4.1. ChangesDiagnostics and Runtime Updates
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Admin
participant CoreRuntimeWriter
participant DebugRuntimeFile
Admin->>CoreRuntimeWriter: update tracy_enabled
CoreRuntimeWriter->>DebugRuntimeFile: merge and write runtime data
DebugRuntimeFile-->>Admin: preserve existing runtime keys
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Line 15: Update admin/index.php around the Tracy toggle logic to preserve
existing debug-runtime keys: use xoops_writeDebugRuntimeOverride() when
available, otherwise merge the existing JSON before writing through a temporary
file and atomic rename. Verify the toggle retains metadata such as error-screen
ownership, then keep the documented claims unchanged in CHANGELOG.md:15-15 and
docs/changelog.txt:8-8.
In `@class/Analysis/SystemDiagnostics.php`:
- Around line 336-342: In class/Analysis/SystemDiagnostics.php lines 336-342,
update errorScreenRow() to evaluate and return the legacy Tracy status before
producing the unsupported-state row when XOOPS_ERROR_SCREEN_STATUS is absent.
Update CHANGELOG.md line 11 and docs/changelog.txt line 5 to describe the
restored legacy Tracy fallback.
In `@xoops_version.php`:
- Around line 25-26: Standardize the version 1.4.1 release date across the
release records, including the $modversion['release_date'] assignment in
xoops_version.php and both changelogs. Choose the intended date, August 8, 2026
based on the changelogs, and update every 1.4.1 entry to match.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 33f1467e-c3d3-40df-81ad-532f8cb48aa7
📒 Files selected for processing (6)
CHANGELOG.mdadmin/diagnostics.phpclass/Analysis/SystemDiagnostics.phpdocs/changelog.txtlanguage/english/admin.phpxoops_version.php
Comment on lines
+336
to
+342
| if (! defined('XOOPS_ERROR_SCREEN_STATUS')) { | ||
| return $this->row( | ||
| 'error_screen', | ||
| 'Unknown', | ||
| 'info', | ||
| 'This XOOPS predates the error-screen seam, so nothing publishes who owns the handlers.' | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Restore the pre-seam diagnostic fallback.
When XOOPS_ERROR_SCREEN_STATUS is absent, errorScreenRow() returns Unknown. This removes the prior Tracy-specific diagnostic instead of preserving the fallback required by this PR. Restore the legacy status path on supported older cores. Update the release notes to describe the actual fallback.
class/Analysis/SystemDiagnostics.php#L336-L342: evaluate the legacy Tracy status before returning an unsupported-state row.CHANGELOG.md#L11-L11: describe the restored legacy fallback.docs/changelog.txt#L5-L5: describe the restored legacy fallback.
📍 Affects 3 files
class/Analysis/SystemDiagnostics.php#L336-L342(this comment)CHANGELOG.md#L11-L11docs/changelog.txt#L5-L5
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@class/Analysis/SystemDiagnostics.php` around lines 336 - 342, In
class/Analysis/SystemDiagnostics.php lines 336-342, update errorScreenRow() to
evaluate and return the legacy Tracy status before producing the
unsupported-state row when XOOPS_ERROR_SCREEN_STATUS is absent. Update
CHANGELOG.md line 11 and docs/changelog.txt line 5 to describe the restored
legacy Tracy fallback.
Contributor
Author
|
Reads the constants published by XOOPS/XoopsCore27#157. Safe to merge first: on a core |
…mpare like with like The 1.4.1 changelog credited the Tracy toggle with merging debug-runtime.json, but the code still wrote it wholesale -- so one press dropped the recorded error-screen owner core keeps in that file and the site fell back to core error handling. The toggle now writes through xoops_writeDebugRuntimeOverride() when core provides it, and on an older core does a locked read-modify-write through a temporary file and rename, so neither path loses another key. The error-handler drift row compared a detected library name (whoops, tracy) against a declared module dirname (xwhoops, xtracy) and so warned of drift on every healthy install. Both sides are normalised to the canonical dirname, legacy spellings accepted, and xWhoops-owner-with-core-holding-the-error-handler is recognised as the designed outcome rather than drift. The pre-seam Diagnostics fallback maps a legacy XOOPS_TRACY_STATUS when present instead of a blank "Unknown". Release date aligned with the changelogs.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@admin/index.php`:
- Around line 175-181: Update the runtime data loading logic around $existing,
$decoded, and $current to reject malformed JSON and valid JSON values that are
not arrays before writing. Fail the debug toggle without replacing the file when
json_decode fails or returns a non-array, preserving existing runtime state and
error-screen ownership metadata.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c723fe0b-c559-4c8e-8294-0445205e5b89
📒 Files selected for processing (4)
CHANGELOG.mdadmin/index.phpclass/Analysis/SystemDiagnostics.phpxoops_version.php
🚧 Files skipped from review as they are similar to previous changes (2)
- xoops_version.php
- class/Analysis/SystemDiagnostics.php
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
The older-core fallback that toggles tracy_enabled read the shared debug-runtime.json, and on malformed or non-array JSON it silently reset its working copy to [] and then overwrote the file -- dropping whatever the file held, including the core's error_screen_owner record kept in the same file since 2.7.3. The preferred path (xoops_writeDebugRuntimeOverride) already merges safely; make the hand-rolled fallback match by failing the toggle and leaving the file untouched when its contents will not decode to an array, so a corrupt file is preserved for inspection rather than replaced blind.
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.
The Diagnostics page answered "is Tracy installed?" and presented it as the error screen row. That was never quite the question, and on XOOPS 2.7.3 it is the wrong one: the site declares an owner, core publishes who actually holds the handlers, and the two can differ — which is the case most worth showing.
The row now reads
XOOPS_ERROR_SCREEN_OWNER,_SOURCEand_STATUSand reports what core says, including the statuses that mean something is wrong:dormant,missing,disabled,contested,error.On a core without the seam the constants are absent and the row falls back to its previous behaviour.
Requires the seam from XOOPS 2.7.3 (XOOPS/XoopsCore27
feat/error-screen-provider) to show anything new.Summary by Sourcery
Update the diagnostics module to report the actual PHP error screen owner and status, aligned with the XOOPS 2.7.3 error-screen seam, and bump the module version for the release.
New Features:
Enhancements:
Documentation:
Chores:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores