Open
Conversation
WalkthroughModernized the DebugBar frontend to ES6 classes and vanilla DOM, added CSS-variable theming with dark mode and many icon assets, introduced syntax highlighting, refactored widgets, and added a configurable query logging mode with logger support and module config. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant DebugBar as PhpDebugBar.DebugBar
participant Ajax as PhpDebugBar.AjaxHandler
participant Server
Browser->>DebugBar: user action (open/tab/dataset)
DebugBar->>Ajax: request (loadFromId / loadFromData)
Ajax->>Server: fetch resource (fetch / XHR)
Server-->>Ajax: response (headers + body)
Ajax->>Ajax: parse headers -> extract debug payload
Ajax-->>DebugBar: deliver parsed data
DebugBar->>Browser: render tabs / widgets / datasets
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the debugbar module’s configuration and front-end assets to modernize the DebugBar UI (new widgets/styles, new vendor assets), and adds a new query logging mode to reduce query-collector overhead. It also introduces a new core helper class for password recovery security (LostPassSecurity), which broadens the PR scope beyond DebugBar.
Changes:
- Add a DebugBar module config option to switch query logging between “all” vs “slow & errors only”, and wire it into the logger.
- Large refactor of DebugBar widget JS/CSS (jQuery → vanilla JS, theming via CSS variables, new icons/highlight/css, SQL formatter vendor JS).
- Add new core class
htdocs/class/LostPassSecurity.phpfor token-based password recovery + rate limiting.
Reviewed changes
Copilot reviewed 22 out of 27 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| htdocs/modules/debugbar/xoops_version.php | Adds query_log_mode module config option. |
| htdocs/modules/debugbar/preloads/core.php | Applies query logging mode; loads debugbar main language when active. |
| htdocs/modules/debugbar/language/english/modinfo.php | Adds language strings for query logging mode. |
| htdocs/modules/debugbar/language/english/common.php | Changes stats summary constant definition. |
| htdocs/modules/debugbar/include/install.php | Updates vendor resource discovery path for DebugBar assets. |
| htdocs/modules/debugbar/class/DebugbarLogger.php | Adds queryLogMode + filtering of logged queries. |
| htdocs/modules/debugbar/assets/widgets/templates/widget.js | Refactors Templates widget to vanilla JS (introduces DOM building changes). |
| htdocs/modules/debugbar/assets/widgets/templates/widget.css | Updates Templates widget styling to CSS variables + dark theme tweaks. |
| htdocs/modules/debugbar/assets/widgets/sqlqueries/widget.js | Refactors SQL Queries widget to vanilla JS + adds formatting/sorting behaviors. |
| htdocs/modules/debugbar/assets/widgets/sqlqueries/widget.css | Updates SQL Queries widget layout/styling and theming variables. |
| htdocs/modules/debugbar/assets/widgets/mails/widget.js | Refactors Mails widget to vanilla JS + popup preview handling. |
| htdocs/modules/debugbar/assets/widgets/mails/widget.css | Updates Mails widget styling to CSS variables. |
| htdocs/modules/debugbar/assets/widgets/http/widget.js | Adds a new HTTP client/events widget (vanilla JS). |
| htdocs/modules/debugbar/assets/widgets.css | Broad styling updates (params tables, message icons, layout, datasets UI). |
| htdocs/modules/debugbar/assets/vendor/sql-formatter/sql-formatter.min.js | Adds bundled SQL formatter for client-side formatting. |
| htdocs/modules/debugbar/assets/openhandler.js | Refactors OpenHandler UI to vanilla JS + fetch-based calls. |
| htdocs/modules/debugbar/assets/openhandler.css | Updates OpenHandler styling (CSS variables, layout, z-index). |
| htdocs/modules/debugbar/assets/icons.css | Adds generated Tabler icon CSS variables + classes. |
| htdocs/modules/debugbar/assets/highlight.css | Adds highlight.js theme styles (light/dark). |
| htdocs/modules/debugbar/assets/debugbar.css | Major DebugBar theming/layout update (CSS variables, icons, positions). |
| htdocs/modules/debugbar/admin/index.php | Updates admin constant lookup for module status infobox. |
| htdocs/class/LostPassSecurity.php | New core password recovery security helper (tokens, rate limiting, optional Protector logging). |
Comment on lines
+254
to
+255
| .catch((err) => { | ||
| callback(null, err); |
There was a problem hiding this comment.
In ajax(), the catch calls callback(null, err), but callers like handleFind(data) assume data is iterable and will throw if data is null (network error / non-JSON response). Either keep the callback signature consistent (e.g. call callback([])), or update handleFind (and other callers) to handle error/empty responses safely.
Suggested change
| .catch((err) => { | |
| callback(null, err); | |
| .catch(() => { | |
| callback([]); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 28
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
htdocs/modules/debugbar/language/english/common.php (1)
12-13:⚠️ Potential issue | 🟡 MinorRemove dead code —
$moduleDirNameand$moduleDirNameUpperare no longer used.Both variables were only needed to construct the old dynamic constant name. With the static
_CO_DEBUGBAR_STATS_SUMMARYdefine on line 16, they serve no purpose and should be removed.🗑️ Proposed cleanup
-$moduleDirName = \basename(\dirname(__DIR__, 3)); -$moduleDirNameUpper = \mb_strtoupper($moduleDirName); - // Module Stats \define('_CO_DEBUGBAR_STATS_SUMMARY', 'Module Status');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/debugbar/language/english/common.php` around lines 12 - 13, Remove the now-unused variables $moduleDirName and $moduleDirNameUpper by deleting their declarations (the lines assigning basename(dirname(...)) and mb_strtoupper(...)); after removal, run a quick grep for $moduleDirName and $moduleDirNameUpper to confirm there are no remaining references and update any code that relied on them (none expected since _CO_DEBUGBAR_STATS_SUMMARY is static).htdocs/modules/debugbar/class/DebugbarLogger.php (1)
31-36:⚠️ Potential issue | 🟡 MinorClass docblock is missing required tags.
Per coding guidelines, class docblocks must include
@category,@package,@author,@copyright,@license, and@link. All of these are on the file-level docblock (lines 7–22) but the class-level docblock has none of them.📝 Proposed addition
/** * DebugbarLogger — collects XOOPS debug data and renders via PHP DebugBar. * * Registers itself with XoopsLogger::addLogger() so it receives all * dispatched log entries (queries, blocks, errors, deprecations, extras). + * + * `@category` Module + * `@package` debugbar + * `@author` Richard Griffith <richard@geekwright.com> + * `@author` trabis <lusopoemas@gmail.com> + * `@copyright` (c) 2000-2026 XOOPS Project (https://xoops.org) + * `@license` GNU GPL 2 (https://www.gnu.org/licenses/gpl-2.0.html) + * `@link` https://xoops.org */As per coding guidelines, "Class docblocks must include
@category,@package,@author,@copyright,@license, and@linktags."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/debugbar/class/DebugbarLogger.php` around lines 31 - 36, The class docblock for DebugbarLogger is missing required tags; update the docblock immediately above the class DebugbarLogger declaration to include the standard tags: `@category`, `@package`, `@author`, `@copyright`, `@license`, and `@link`, mirroring the file-level values (or appropriate class-specific values) so the class-level docblock complies with the coding guidelines.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@htdocs/modules/debugbar/admin/index.php`:
- Line 27: Replace the dynamic call to \constant() with a direct constant
reference: change the call that passes \constant('_CO_DEBUGBAR_STATS_SUMMARY')
to pass the constant _CO_DEBUGBAR_STATS_SUMMARY directly when invoking
$adminObject->addInfoBox; this removes the unnecessary runtime lookup and
simplifies the call site.
In `@htdocs/modules/debugbar/assets/debugbar.css`:
- Line 398: Change the offending CSS declarations: replace the background-color
value "currentColor" with the lowercase "currentcolor" (fix the
background-color: currentColor instance) and replace any use of the deprecated
"grid-gap" property with the modern "gap" property (update the grid-gap
declarations to gap) so the file debugbar.css satisfies the value-keyword-case
and deprecation rules.
In `@htdocs/modules/debugbar/assets/debugbar.js`:
- Around line 442-443: The assignment to this.settings uses JSON.parse on
localStorage.getItem('phpdebugbar-settings') which can throw for malformed JSON;
wrap the parse in a try/catch (or a small safeParse helper) around the retrieval
of 'phpdebugbar-settings', fall back to {} on error, and optionally remove the
corrupted key from localStorage or log the error so initialization continues
(update the code that sets this.settings accordingly).
- Around line 1553-1557: The setter setOpenHandler currently dereferences
this.openHandler.el before checking for null; change it to assign
this.openHandler = handler first and only access handler.el (or
this.openHandler.el) if handler is non-null — e.g., guard the setAttribute call
with if (handler) or if (this.openHandler) — and then set this.openbtn.hidden =
handler == null; this prevents a runtime error when setOpenHandler(null) is
called.
- Around line 296-299: The data callback passed to this.bindAttr('data', ...)
can receive null/undefined so calling Object.keys(data) may throw; update the
handler (the 'data' bind callback that sets this.tab.setAttribute and checks
this.widgetRendered/this.active) to first guard against nullish payloads (e.g.,
treat null/undefined as empty) before calling Object.keys or accessing
data.count, then compute the empty condition and pass that to
this.tab.setAttribute; ensure the rest of the branch (checks on
this.widgetRendered and this.active) still uses the guarded data value.
In `@htdocs/modules/debugbar/assets/highlight.css`:
- Line 131: The dark-theme CSS selectors are malformed by repeating
[data-theme='dark'] inside the descendant chain; update the selectors so the
theme attribute appears only once at the root and the elements are proper
descendants (e.g., change the selector containing ".phpdebugbar-hljs-meta
[data-theme='dark'] .phpdebugbar-hljs-keyword" to "[data-theme='dark']
.phpdebugbar-hljs-meta .phpdebugbar-hljs-keyword" and do the same for the
.phpdebugbar-hljs-string variant), ensuring .phpdebugbar-hljs-meta,
.phpdebugbar-hljs-keyword and .phpdebugbar-hljs-string remain targeted correctly
under the dark theme.
In `@htdocs/modules/debugbar/assets/openhandler.js`:
- Around line 109-112: The loop that builds the search object copies any truthy
form value (for example from formData.entries()) including the placeholder
"(Method)", causing spurious filters; update the logic in the handler that
iterates formData.entries() (and the similar handling around the select at the
other block mentioned) to only copy values that are non-empty and not equal to
the placeholder string (e.g. value !== '' && value !== '(Method)') or otherwise
normalize placeholder options to an empty string before assignment so the
default "(Method)" is never submitted as a real filter.
- Around line 152-158: The per-row method link never appears because the code
creates methodLink (and wires its click to call self.find with { method:
meta.method }) but later renders the method as plain text instead of inserting
the anchor; replace the code that sets the method cell's textContent with
appending the pre-built methodLink (ensure the link has an href like "#" to be
focusable and keep the existing click handler), so the anchor you created
(methodLink) is actually inserted into the row DOM and triggers self.find(...)
via its click listener (refer to methodLink, meta.method, self.find, and
self.handleFind to locate the creation and the render site).
- Around line 142-150: The snippet creates a DOM node and listener for loadLink
but never appends it, leaking allocations; either remove the loadLink creation
entirely or append it to the appropriate row/action container so it becomes
reachable. If you want the “Load dataset” action, move the creation+listener
(loadLink, its click handler calling self.hide(), self.load(meta.id, ...), and
self.callback(meta.id, data)) into the same place where other per-row action
elements are appended (e.g., the row element or actions container) and call
appendChild on that container; otherwise delete the loadLink variable and its
addEventListener to avoid unused DOM allocations.
- Around line 240-256: ajax's failure path calls callback(null, err) which
breaks callers like handleFind that expect an iterable first argument; change
the catch to call callback with an empty iterable as the first arg (e.g.,
callback([], err)) so code using for...of over the data won't throw, keeping the
error as the second parameter for existing error handling.
- Around line 20-23: Replace anchor-based action elements with semantic buttons:
for each element created as document.createElement('a')—notably closebtn,
loadmorebtn, showonlycurrentbtn, refreshbtn, clearbtn and row action links
loadLink, methodLink, uriLink, ipLink, searchLink—create
document.createElement('button') instead, set button.type = 'button', preserve
the same classList and innerHTML/textContent, and remove any reliance on href
attributes; ensure event listeners remain attached to the new button elements so
behavior is unchanged.
- Around line 236-238: The clear operation is currently callable via GET and
lacks CSRF validation; update OpenHandler::handle to reject non-POST requests
when the op == 'clear' and perform XOOPS CSRF token verification (use the
xoopsSecurity token check API) before executing the clear logic; also update the
frontend clear() (in openhandler.js) to send the request as POST and include the
XOOPS security token (e.g., in request body or header) so the backend can
validate it; ensure clear-related code paths in OpenHandler::handle abort with a
proper error response when method or token validation fails.
In `@htdocs/modules/debugbar/assets/widgets.js`:
- Around line 605-608: The code is inserting untrusted collector data into DOM
with td2.innerHTML = value.context[key] (and the similar occurrence around lines
833–835), creating an XSS risk; change these to assign the data as text (e.g.,
use td2.textContent = value.context[key] or createTextNode and append) or
otherwise escape the value before insertion so the content is treated as plain
text rather than HTML; update both the td2 assignment and the other places that
currently use innerHTML (references: td2, csscls(...), value.context[key], and
the analogous element at the other occurrence).
- Around line 16-17: The htmlize function (PhpDebugBar.Widgets.htmlize) calls
text.replace(...) and will throw for non-string inputs; fix by ensuring text is
a string before calling replace — e.g., guard with typeof text === 'string' ?
text : String(text) (or text == null ? '' : String(text)) and then perform
.replace(/\n/g, '<br>').replace(/\s/g, ' ') so numbers, objects, null or
undefined don't crash the widget.
- Around line 699-701: Guard the message used for filtering so it doesn't call
toLowerCase() on undefined: when iterating this.get('data') (inside the forEach
where caseless is used), derive a safe string from item.message or fall back to
item.message_html or '' before applying caseless ? ...toLowerCase(): ..., i.e.
ensure item.message is checked and a default string is used so the caseless
branch never calls toLowerCase() on undefined.
- Around line 841-843: The click handler added via li.addEventListener uses
event.target but the callback is declared without an event parameter; update the
anonymous function passed to li.addEventListener to accept the event argument
(e.g., change function () { ... } to function (event) { ... } or to an arrow
(event) => { ... }) so references to event.target and event propagation checks
inside that handler succeed; ensure you modify the same handler that contains
the window.getSelection().type === 'Range' or event.target.closest('.sf-dump')
check.
- Line 685: The forEach callback currently uses an arrow expression that
implicitly returns createFilterItem, which violates Biome's
useIterableCallbackReturn rule; update the collectors.forEach callback to use a
block body and call createFilterItem('collector', collector) as a statement (no
return) so the callback does not return a value—locate the
collectors.forEach(...) call and change the arrow function to a statement-body
form that simply invokes createFilterItem.
In `@htdocs/modules/debugbar/assets/widgets/http/widget.js`:
- Around line 81-85: The current loop over request.details constructs a row via
row.innerHTML using unescaped key and request.details[key], allowing DOM XSS;
change the implementation inside the for (const key in request.details) loop to
create elements manually (tr, td, pre, code) and assign text nodes or
element.textContent for the key and value instead of using innerHTML, preserving
the csscls(...) class usage and appending the created elements to tbody; ensure
you still skip function values as the existing condition does.
In `@htdocs/modules/debugbar/assets/widgets/mails/widget.js`:
- Around line 36-37: The 'View Mail' anchor's click handler
(link.addEventListener('click', ...)) is bubbling up and triggering the parent
li toggle; update the click listeners to accept the event parameter and call
event.stopPropagation() (and optionally event.preventDefault() if navigation
should be suppressed) before opening the popup. Apply the same change to the
second occurrence of the link click handler around the other instance (the block
at lines ~93-95) so neither click will bubble to the li handler.
In `@htdocs/modules/debugbar/assets/widgets/sqlqueries/widget.js`:
- Around line 329-336: The duplicate CSS marker (csscls('sql-duplicate')) is
only applied once when building items but is lost when the list is re-rendered
on sorts/updates; extract the duplicate-marking logic into a reusable method
(e.g., markDuplicateStatements or applyDuplicateMarkers) that iterates the sql
object and adds csscls('sql-duplicate') to matching
this.list.el.querySelectorAll(`.${csscls('list-item')}`)[index], then call that
method both after the initial loop (where duplicates are detected) and after any
list re-render/sort handler (where this.list.render or the sort click callback
currently re-renders the list, e.g., the code around the sort click at the
second block), ensuring duplicate classes are reapplied whenever the DOM list is
rebuilt so "Show only duplicated" stays correct.
- Around line 75-93: In the object branch of the row-building logic (inside the
block that checks typeof value === 'object' && value !== null), you construct
valueTd with namespace/name/file/line but never append it to the row; update the
code that builds the row (the tr creation scope) to call tr.append(valueTd)
after finishing valueTd so the value column renders; look for the variables
valueTd and tr in widget.js and add the append immediately after the block that
builds valueTd.
In `@htdocs/modules/debugbar/assets/widgets/templates/widget.css`:
- Around line 30-31: The CSS contains a duplicated selector
"div.phpdebugbar-widgets-templates a.phpdebugbar-widgets-editor-link:before" in
the pseudo-element group; remove the repeated occurrence so the selector appears
only once in the rule set (leave the single instance of
div.phpdebugbar-widgets-templates a.phpdebugbar-widgets-editor-link:before and
delete the duplicate).
In `@htdocs/modules/debugbar/assets/widgets/templates/widget.js`:
- Around line 97-101: The loop that builds rows for tpl.params in the widget
uses row.innerHTML with interpolated key and tpl.params[key], which allows HTML
injection; change the implementation in the for..in loop that references
tpl.params, csscls and tbody so you create elements explicitly: create a tr,
then two td elements (set className using csscls('name')/csscls('value')), set
the key into the name td via textContent, create a pre and code element, set the
param value into code.textContent (not innerHTML), append code into pre then pre
into the value td, and finally append the tds into the tr and tr into tbody;
preserve the existing skip-for-functions check.
In `@htdocs/modules/debugbar/class/DebugbarLogger.php`:
- Around line 637-642: Update the inline comment above the guard that checks
($this->queryLogMode === 1 && $level !== LogLevel::ERROR) in DebugbarLogger
(class DebugbarLogger.php) to explicitly state that in "slow & errors only" mode
fast queries are suppressed even if they are duplicates that were level-promoted
to WARNING, i.e. only queries with LogLevel::ERROR are added to the Queries
collector in this mode; reference the level-promotion handling (the block that
can change a duplicate's $level to LogLevel::WARNING) so readers know why
promoted duplicates are also dropped.
- Around line 408-417: The new public method DebugbarLogger::setQueryLogMode
lacks a `@throws` PHPDoc tag; update its docblock to include an explicit `@throws`
entry stating that the method does not throw any exceptions (e.g. "@throws void"
or a short sentence "Does not throw any exceptions") so the `@param`, `@return` and
`@throws` contract is satisfied for setQueryLogMode; ensure you modify the
docblock immediately above the setQueryLogMode method.
In `@htdocs/modules/debugbar/include/install.php`:
- Line 72: The current use of dirname($path, 4) when building the $checkedDirs
entry is brittle and relies on a magic depth; change it to compute the depth
dynamically from the expected suffix segments (or derive it from the known
suffix string) instead of hardcoding 4 so the trimmed display remains correct if
path nesting changes — update the code that assigns $checkedDirs[] (the line
using dirname($path, 4)) to calculate the skip depth from the number of segments
in the known suffix (or use a comment explaining the meaning if dynamic
computation is not used) and use that value in dirname() to produce a
self-documenting, robust result.
- Line 58: The vendor assets path is wrong: update the $vendorPaths[] entry that
currently appends '/php-debugbar/php-debugbar/resources' so it points to the
Composer-installed assets at
'/php-debugbar/php-debugbar/src/DebugBar/Resources'; locate the $vendorPaths[]
assignment in install.php (the line adding to $vendorPaths using $vendorBase)
and replace the resources segment accordingly so asset discovery uses the
Composer layout.
In `@htdocs/modules/debugbar/xoops_version.php`:
- Line 99: The options array for the select is inverted: the display labels and
stored values are swapped so string labels are being stored instead of the
integer constants; update the 'options' entry so the array keys are the
constants (_MI_DEBUGBAR_QUERYMODE_ALL, _MI_DEBUGBAR_QUERYMODE_SLOW) and the
values are the human-readable strings ("All queries", "Slow & errors only") so
the stored value is an integer constant and (int)$moduleConfig['query_log_mode']
in preloads/core.php works correctly.
---
Outside diff comments:
In `@htdocs/modules/debugbar/class/DebugbarLogger.php`:
- Around line 31-36: The class docblock for DebugbarLogger is missing required
tags; update the docblock immediately above the class DebugbarLogger declaration
to include the standard tags: `@category`, `@package`, `@author`, `@copyright`,
`@license`, and `@link`, mirroring the file-level values (or appropriate
class-specific values) so the class-level docblock complies with the coding
guidelines.
In `@htdocs/modules/debugbar/language/english/common.php`:
- Around line 12-13: Remove the now-unused variables $moduleDirName and
$moduleDirNameUpper by deleting their declarations (the lines assigning
basename(dirname(...)) and mb_strtoupper(...)); after removal, run a quick grep
for $moduleDirName and $moduleDirNameUpper to confirm there are no remaining
references and update any code that relied on them (none expected since
_CO_DEBUGBAR_STATS_SUMMARY is static).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (3)
htdocs/modules/debugbar/assets/dist/debugbar.min.cssis excluded by!**/dist/**htdocs/modules/debugbar/assets/dist/debugbar.min.jsis excluded by!**/dist/**,!**/*.min.jshtdocs/modules/debugbar/assets/vendor/sql-formatter/sql-formatter.min.jsis excluded by!**/*.min.js
📒 Files selected for processing (24)
htdocs/class/LostPassSecurity.phphtdocs/modules/debugbar/admin/index.phphtdocs/modules/debugbar/assets/debugbar.csshtdocs/modules/debugbar/assets/debugbar.jshtdocs/modules/debugbar/assets/highlight.csshtdocs/modules/debugbar/assets/icons.csshtdocs/modules/debugbar/assets/openhandler.csshtdocs/modules/debugbar/assets/openhandler.jshtdocs/modules/debugbar/assets/vendor/highlightjs/highlight.pack.jshtdocs/modules/debugbar/assets/widgets.csshtdocs/modules/debugbar/assets/widgets.jshtdocs/modules/debugbar/assets/widgets/http/widget.jshtdocs/modules/debugbar/assets/widgets/mails/widget.csshtdocs/modules/debugbar/assets/widgets/mails/widget.jshtdocs/modules/debugbar/assets/widgets/sqlqueries/widget.csshtdocs/modules/debugbar/assets/widgets/sqlqueries/widget.jshtdocs/modules/debugbar/assets/widgets/templates/widget.csshtdocs/modules/debugbar/assets/widgets/templates/widget.jshtdocs/modules/debugbar/class/DebugbarLogger.phphtdocs/modules/debugbar/include/install.phphtdocs/modules/debugbar/language/english/common.phphtdocs/modules/debugbar/language/english/modinfo.phphtdocs/modules/debugbar/preloads/core.phphtdocs/modules/debugbar/xoops_version.php
| mask-size: contain; | ||
| mask-repeat: no-repeat; | ||
| mask-position: center; | ||
| background-color: currentColor; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if file exists and get total lines
if [ -f "htdocs/modules/debugbar/assets/debugbar.css" ]; then
echo "=== File Info ==="
wc -l htdocs/modules/debugbar/assets/debugbar.css
echo ""
echo "=== Lines around 398 ==="
sed -n '395,401p' htdocs/modules/debugbar/assets/debugbar.css | cat -n
echo ""
echo "=== Lines around 513 ==="
sed -n '510,516p' htdocs/modules/debugbar/assets/debugbar.css | cat -n
echo ""
echo "=== Search for currentColor and grid-gap ==="
rg -n "currentColor|grid-gap" htdocs/modules/debugbar/assets/debugbar.css
else
echo "File not found"
fiRepository: XOOPS/XoopsCore25
Length of output: 787
🌐 Web query:
stylelint value-keyword-case CSS keyword lowercase
💡 Result:
To enforce lowercase CSS keyword values in Stylelint, enable the value-keyword-case rule with the "lower" option:
{
"rules": {
"value-keyword-case": "lower"
}
}This flags things like display: Block; and allows display: block; (and also handles vendor-prefixed keywords like -webkit-transform). The rule is autofixable via Stylelint’s --fix. [1]
Optional: ignore specific keywords (or regex) if needed:
{
"rules": {
"value-keyword-case": ["lower", { "ignoreKeywords": ["currentColor", "/^(f|F)lex$/"] }]
}
}
``` [1]
**Sources:** [1]
---
🌐 Web query:
`CSS grid-gap deprecated gap replacement`
💡 Result:
`grid-gap` (plus `grid-row-gap` / `grid-column-gap`) is deprecated as the *prefixed* Grid-only name. The modern replacement is:
- **`gap`** (shorthand for both directions)
- **`row-gap`** and **`column-gap`** (longhands)
Example:
```css
.grid {
display: grid;
gap: 16px; /* replaces grid-gap */
/* or: row-gap: 12px; column-gap: 24px; */
}Browsers still accept grid-gap as an alias for gap for legacy compatibility, but new code should use gap / row-gap / column-gap. [1] [2]
Resolve stylelint-blocking declarations in debugbar.css.
Line 398: currentColor must be lowercase per the value-keyword-case rule.
Line 513: grid-gap is deprecated; use gap instead.
Suggested fixes
- background-color: currentColor;
+ background-color: currentcolor;- grid-gap: 4px 10px;
+ gap: 4px 10px;🧰 Tools
🪛 Stylelint (17.3.0)
[error] 398-398: Expected "currentColor" to be "currentcolor" (value-keyword-case)
(value-keyword-case)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/modules/debugbar/assets/debugbar.css` at line 398, Change the
offending CSS declarations: replace the background-color value "currentColor"
with the lowercase "currentcolor" (fix the background-color: currentColor
instance) and replace any use of the deprecated "grid-gap" property with the
modern "gap" property (update the grid-gap declarations to gap) so the file
debugbar.css satisfies the value-keyword-case and deprecation rules.
Comment on lines
+408
to
+417
| /** | ||
| * Set the query logging mode. | ||
| * | ||
| * @param int $mode 0 = all queries, 1 = slow & errors only | ||
| * @return void | ||
| */ | ||
| public function setQueryLogMode(int $mode): void | ||
| { | ||
| $this->queryLogMode = $mode; | ||
| } |
There was a problem hiding this comment.
Add @throws tag to satisfy the PHPDoc contract for new public methods.
Coding guidelines require @param, @return, and @throws on all new public methods. Since no exception is thrown here, document that explicitly.
📝 Proposed addition
/**
* Set the query logging mode.
*
* `@param` int $mode 0 = all queries, 1 = slow & errors only
* `@return` void
+ * `@throws` void
*/As per coding guidelines, "New public methods must have PHPDoc with @param, @return, and @throws tags."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/modules/debugbar/class/DebugbarLogger.php` around lines 408 - 417, The
new public method DebugbarLogger::setQueryLogMode lacks a `@throws` PHPDoc tag;
update its docblock to include an explicit `@throws` entry stating that the method
does not throw any exceptions (e.g. "@throws void" or a short sentence "Does not
throw any exceptions") so the `@param`, `@return` and `@throws` contract is satisfied
for setQueryLogMode; ensure you modify the docblock immediately above the
setQueryLogMode method.
Comment on lines
+637
to
+642
| // In "slow & errors only" mode, queryCount/duplicateCount are still | ||
| // tracked above for the summary, but fast normal queries are not | ||
| // added to the Queries collector — this is the main performance gain. | ||
| if ($this->queryLogMode === 1 && $level !== LogLevel::ERROR) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Fast duplicate queries are silently suppressed in mode 1 — the guard comment is incomplete.
After the level-promotion block (lines 629–635), a duplicate fast query has $level === LogLevel::WARNING, not ERROR. The mode-1 guard therefore also drops fast duplicates from the Queries collector, which may surprise users who expect to see "all duplicates regardless of speed" when debugging.
If the intent is indeed "fast queries never appear in mode 1, even if duplicate", the inline comment should say so explicitly to avoid future confusion.
📝 Suggested comment update
- // In "slow & errors only" mode, queryCount/duplicateCount are still
- // tracked above for the summary, but fast normal queries are not
- // added to the Queries collector — this is the main performance gain.
- if ($this->queryLogMode === 1 && $level !== LogLevel::ERROR) {
+ // In "slow & errors only" mode (queryLogMode=1): only queries that
+ // have been escalated to ERROR (slow or DB error) are forwarded to
+ // the Queries collector. Fast queries — including fast duplicates
+ // (WARNING) — are suppressed. Counters above are still updated for
+ // the summary line regardless of mode.
+ if ($this->queryLogMode === 1 && $level !== LogLevel::ERROR) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // In "slow & errors only" mode, queryCount/duplicateCount are still | |
| // tracked above for the summary, but fast normal queries are not | |
| // added to the Queries collector — this is the main performance gain. | |
| if ($this->queryLogMode === 1 && $level !== LogLevel::ERROR) { | |
| return; | |
| } | |
| // In "slow & errors only" mode (queryLogMode=1): only queries that | |
| // have been escalated to ERROR (slow or DB error) are forwarded to | |
| // the Queries collector. Fast queries — including fast duplicates | |
| // (WARNING) — are suppressed. Counters above are still updated for | |
| // the summary line regardless of mode. | |
| if ($this->queryLogMode === 1 && $level !== LogLevel::ERROR) { | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/modules/debugbar/class/DebugbarLogger.php` around lines 637 - 642,
Update the inline comment above the guard that checks ($this->queryLogMode === 1
&& $level !== LogLevel::ERROR) in DebugbarLogger (class DebugbarLogger.php) to
explicitly state that in "slow & errors only" mode fast queries are suppressed
even if they are duplicates that were level-promoted to WARNING, i.e. only
queries with LogLevel::ERROR are added to the Queries collector in this mode;
reference the level-promotion handling (the block that can change a duplicate's
$level to LogLevel::WARNING) so readers know why promoted duplicates are also
dropped.
| foreach ($vendorBases as $vendorBase) { | ||
| $vendorPaths[] = $vendorBase . '/maximebf/debugbar/src/DebugBar/Resources'; | ||
| $vendorPaths[] = $vendorBase . '/php-debugbar/php-debugbar/src/DebugBar/Resources'; | ||
| $vendorPaths[] = $vendorBase . '/php-debugbar/php-debugbar/resources'; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
php-debugbar/php-debugbar GitHub repository directory structure resources assets
💡 Result:
In php-debugbar/php-debugbar, the “assets” (JS/CSS + vendored frontend dependencies) live alongside the PHP source, but where you’ll see them depends on whether you’re looking at the Git repo or an installed Composer package:
1) GitHub repo (source tree)
At the repository root you’ll see (among others) these top-level directories: resources/, src/, build/, demo/, docs/, tests/. The presence of a top-level resources/ directory is intentional for the frontend assets. [1]
The official docs describe that the debugbar’s CSS/JS assets are “located in the resources folder”, and also reference a dist folder containing minified bundles that are used by default. [2]
2) Composer-installed package path (what your app usually serves)
When installed via Composer, many integrations historically point at:
vendor/php-debugbar/php-debugbar/src/DebugBar/Resources/
A typical Resources directory contains the core files:
debugbar.css,debugbar.jswidgets.css,widgets.jsopenhandler.css,openhandler.js
…and subfolders like:
vendor/(third-party frontend deps)widgets/(widget-specific assets) [3]
So, if you’re wiring up “public assets” in your app, you generally expose/copy/symlink the Resources directory (or use the library’s asset handling), rather than the whole vendor/ tree. [2][3]
Sources: [1] repo root structure on GitHub, [2] official “Rendering / Assets” docs (resources + dist), [3] example directory index of src/DebugBar/Resources showing file/subdir layout.
Fix the php-debugbar resources directory path — it will not exist in Composer installations.
The path php-debugbar/php-debugbar/resources is incorrect. The GitHub repo has a top-level resources/ directory, but when the package is installed via Composer, the actual assets are located at src/DebugBar/Resources/. This code will silently fail to find assets on every installation, triggering the error path consistently.
Change to: php-debugbar/php-debugbar/src/DebugBar/Resources
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/modules/debugbar/include/install.php` at line 58, The vendor assets
path is wrong: update the $vendorPaths[] entry that currently appends
'/php-debugbar/php-debugbar/resources' so it points to the Composer-installed
assets at '/php-debugbar/php-debugbar/src/DebugBar/Resources'; locate the
$vendorPaths[] assignment in install.php (the line adding to $vendorPaths using
$vendorBase) and replace the resources segment accordingly so asset discovery
uses the Composer layout.
| $checkedDirs = []; | ||
| foreach ($vendorPaths as $path) { | ||
| $checkedDirs[] = basename(dirname(dirname(dirname(dirname($path))))) . '/…/' . basename($path); | ||
| $checkedDirs[] = basename(dirname($path, 4)) . '/…/' . basename($path); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
dirname depth is calibrated only for the new 3-segment path.
dirname($path, 4) is correct for {vendorBase}/php-debugbar/php-debugbar/resources (3 path segments after vendorBase, so depth 4 reaches vendorBase's parent). However, this magic number is brittle — if the source path structure ever changes (e.g., a deeper nesting), the displayed "checked dirs" will silently become meaningless. Consider deriving it from the known suffix depth or at minimum adding a comment like // 4 = skip resources/pkg/vendor/pkgname.
🔧 Proposed improvement: make the depth self-documenting
- $checkedDirs[] = basename(dirname($path, 4)) . '/…/' . basename($path);
+ // 4 levels up: resources → php-debugbar (pkg) → php-debugbar (vendor) → vendor base → its parent
+ $checkedDirs[] = basename(dirname($path, 4)) . '/…/' . basename($path);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/modules/debugbar/include/install.php` at line 72, The current use of
dirname($path, 4) when building the $checkedDirs entry is brittle and relies on
a magic depth; change it to compute the depth dynamically from the expected
suffix segments (or derive it from the known suffix string) instead of
hardcoding 4 so the trimmed display remains correct if path nesting changes —
update the code that assigns $checkedDirs[] (the line using dirname($path, 4))
to calculate the skip depth from the number of segments in the known suffix (or
use a comment explaining the meaning if dynamic computation is not used) and use
that value in dirname() to produce a self-documenting, robust result.
| 'formtype' => 'select', | ||
| 'valuetype' => 'int', | ||
| 'default' => 1, | ||
| 'options' => [_MI_DEBUGBAR_QUERYMODE_ALL => 0, _MI_DEBUGBAR_QUERYMODE_SLOW => 1], |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find XoopsFormSelect implementation to verify key/value orientation
find . -name "*.php" -type f | xargs grep -l "class XoopsFormSelect" | head -5Repository: XOOPS/XoopsCore25
Length of output: 342
🏁 Script executed:
# Search for addOptionArray implementation
rg "addOptionArray" --type php -A 10 | head -100Repository: XOOPS/XoopsCore25
Length of output: 8061
🏁 Script executed:
# Check the actual debugbar xoops_version.php file context
cat -n htdocs/modules/debugbar/xoops_version.php | sed -n '92,105p'Repository: XOOPS/XoopsCore25
Length of output: 663
🏁 Script executed:
# Search for how module config options are stored/read in XOOPS
rg "config.*options\|addOptionArray" htdocs/class/libraries --type php -B 2 -A 5Repository: XOOPS/XoopsCore25
Length of output: 43
🏁 Script executed:
# Check preloads/core.php for the casting behavior mentioned
find . -path "*preloads/core.php" -type f -exec cat -n {} \; | grep -A 5 -B 5 "query_log_mode"Repository: XOOPS/XoopsCore25
Length of output: 728
🏁 Script executed:
# Find language files for debugbar module
find htdocs/modules/debugbar -name "*.php" -type f | xargs grep -l "_MI_DEBUGBAR_QUERYMODE_ALL\|_MI_DEBUGBAR_QUERYMODE_SLOW"Repository: XOOPS/XoopsCore25
Length of output: 155
🏁 Script executed:
# Check the language definitions
rg "_MI_DEBUGBAR_QUERYMODE" --type php -B 1 -A 1 htdocs/modules/debugbarRepository: XOOPS/XoopsCore25
Length of output: 1227
Fix inverted options array — stored values and display labels are swapped, breaking slow-query logging mode.
XOOPS form select options use the array key as the stored value and array value as the display label. The current code inverts this:
['All queries' => 0, 'Slow & errors only' => 1]
// key = display string (stored as-is) ✗
// value = int (shown as label) ✗This stores string values instead of integers. In preloads/core.php:113, the cast (int)$moduleConfig['query_log_mode'] evaluates both strings to 0, so selecting "Slow & errors only" still behaves as "All queries" mode.
Fix
- 'options' => [_MI_DEBUGBAR_QUERYMODE_ALL => 0, _MI_DEBUGBAR_QUERYMODE_SLOW => 1],
+ 'options' => [0 => _MI_DEBUGBAR_QUERYMODE_ALL, 1 => _MI_DEBUGBAR_QUERYMODE_SLOW],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/modules/debugbar/xoops_version.php` at line 99, The options array for
the select is inverted: the display labels and stored values are swapped so
string labels are being stored instead of the integer constants; update the
'options' entry so the array keys are the constants (_MI_DEBUGBAR_QUERYMODE_ALL,
_MI_DEBUGBAR_QUERYMODE_SLOW) and the values are the human-readable strings ("All
queries", "Slow & errors only") so the stored value is an integer constant and
(int)$moduleConfig['query_log_mode'] in preloads/core.php works correctly.
… event param - Replace innerHTML with safe DOM construction (textContent) in widgets.js, http/widget.js, and templates/widget.js to prevent XSS - Guard setOpenHandler() against null dereference in debugbar.js - Wrap localStorage JSON.parse in try/catch in debugbar.js - Guard Object.keys(data) against null/undefined payloads in debugbar.js - Fix AJAX error callback to return [] instead of null in openhandler.js - Add missing event parameter to click handler in widgets.js - Harden htmlize() against non-string input in widgets.js - Guard item.message filtering against undefined in widgets.js Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
mambax7
requested review from
ForXoops,
GregMage,
alain01,
djculex,
geekwright,
ggoffy,
ihackcode,
kevinpetit,
liomj,
montuy337513,
redheadedrod and
tad0616
| // In "slow & errors only" mode, queryCount/duplicateCount are still | ||
| // tracked above for the summary, but fast normal queries are not | ||
| // added to the Queries collector — this is the main performance gain. | ||
| if ($this->queryLogMode === 1 && $level !== LogLevel::ERROR) { |
There was a problem hiding this comment.
The query filtering logic at line 640-642 only checks for ERROR level, but doesn't check for WARNING level (duplicates). This means that in "slow & errors only" mode, duplicate queries that are marked as WARNING will still be filtered out, even though they might be useful for debugging.
Consider changing the condition to:
if ($this->queryLogMode === 1 && $level !== LogLevel::ERROR && $level !== LogLevel::WARNING) {This would ensure slow queries (ERROR) and duplicates (WARNING) are both shown when in "slow & errors only" mode.
Suggested change
| if ($this->queryLogMode === 1 && $level !== LogLevel::ERROR) { | |
| if ($this->queryLogMode === 1 && $level !== LogLevel::ERROR && $level !== LogLevel::WARNING) { |
| color: #000; | ||
| font-family: var(--debugbar-font-sans); | ||
| background: var(--debugbar-background); | ||
| z-index: 100000000; |
There was a problem hiding this comment.
The z-index value has been changed from 10000 to 100000000 (100 million). This is an extremely high value that could cause stacking context issues with other UI elements on the page.
A more reasonable z-index value would be in the range of 9999-100000. Consider using a value like 99999 instead, which should be sufficient for most debugging scenarios while avoiding potential conflicts.
Suggested change
| z-index: 100000000; | |
| z-index: 99999; |
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (6)
htdocs/modules/debugbar/assets/widgets.js (1)
686-686:⚠️ Potential issue | 🟡 MinorUse a block-body callback for
forEachto satisfy Biome rule.The expression-body callback is flagged by
useIterableCallbackReturn.Suggested fix
-collectors.forEach(collector => createFilterItem('collector', collector)); +collectors.forEach((collector) => { + createFilterItem('collector', collector); +});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/debugbar/assets/widgets.js` at line 686, Change the expression-bodied arrow callback on collectors.forEach to a block-bodied callback so it satisfies the Biome rule; update the call that currently uses collectors.forEach(collector => createFilterItem('collector', collector)) to use a block body and call createFilterItem('collector', collector) inside it (referencing the collectors.forEach invocation and the createFilterItem function).htdocs/modules/debugbar/assets/openhandler.js (5)
142-150:⚠️ Potential issue | 🟠 MajorRow action handlers are wired but unreachable.
loadLinkandmethodLinkare created with listeners but never appended. This leaves dead handlers and breaks quick filtering by method.Suggested fix
-const methodTd = document.createElement('td'); -methodTd.textContent = meta.method; +const methodTd = document.createElement('td'); +methodTd.append(methodLink); tr.append(methodTd);If “Load dataset” is intended, append
loadLinkin a dedicated actions cell; otherwise remove its allocation/listener.Also applies to: 152-158, 191-193
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/debugbar/assets/openhandler.js` around lines 142 - 150, The handlers for the row actions (e.g., loadLink and methodLink) are created and wired but never appended to the DOM, so make them reachable: either append loadLink and methodLink into the row's actions container (create or use an existing cell like an actionsCell or metaActions element) so clicks call self.hide(), self.load(meta.id, ...) and self.callback(meta.id, data), or remove the unused element creation/listeners entirely; update the code paths around the loadLink and methodLink creation points (the addEventListener blocks) to insert the corresponding anchor elements into the row UI so the quick-filter/method actions work.
109-112:⚠️ Potential issue | 🟠 MajorDefault
(Method)placeholder is still submitted as a real filter.The current form serialization captures
(Method)and sends it asmethod, which can produce false-empty results.Suggested fix
-+ '<select name="method"><option selected>(Method)</option><option>GET</option><option>POST</option><option>PUT</option><option>DELETE</option></select>'; ++ '<select name="method"><option value="" selected>(Method)</option><option value="GET">GET</option><option value="POST">POST</option><option value="PUT">PUT</option><option value="DELETE">DELETE</option></select>'; ... - if (value) { + if (value && value !== '(Method)') { search[name] = value; }Also applies to: 121-122
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/debugbar/assets/openhandler.js` around lines 109 - 112, The form serialization loop that iterates over formData.entries() and assigns to the search object (variables name/value and search) is treating the placeholder "(Method)" as a real value; update the check so that you only set search[name] when value is truthy and not equal to the placeholder string "(Method)" (and apply the same guard in the other identical serialization block that also processes formData.entries()); this ensures placeholder values are ignored before assigning to search.
20-23:⚠️ Potential issue | 🟠 MajorReplace anchor-only controls with semantic buttons.
These interactive controls are rendered as
<a>withouthref, which is not reliably keyboard accessible and hurts assistive-tech semantics. Use<button type="button">for close/load/refresh/clear and row-level actions.Also applies to: 51-80, 142-184
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/debugbar/assets/openhandler.js` around lines 20 - 23, Replace anchor-only interactive elements with semantic buttons: instead of creating an <a> for controls like this.closebtn (and similar elements created in the regions referenced for load/refresh/clear and row-level actions), create a HTMLButtonElement, set type="button", copy over the same class list (csscls('closebtn') etc.) and innerHTML/icon markup, and reattach the existing event listeners and attributes; ensure any aria-labels or role attributes required for accessibility are preserved or added. Locate the element creations (e.g., this.closebtn, load/refresh/clear control variables and row action elements) and replace document.createElement('a') with document.createElement('button') and set button.type = 'button' while keeping visual classes and behavior unchanged.
236-247:⚠️ Potential issue | 🟠 Major
clearis still sent as GET; destructive action should be POST + CSRF-protected.
op=clearin query string is vulnerable to CSRF/prefetch-style side effects unless the backend strictly enforces method/token checks.#!/bin/bash # Verify backend enforcement for clear-operation method/token checks rg -n "op.*clear|clear.*op|REQUEST_METHOD|_REQUEST|xoopsSecurity|check.*token|security" --type=php fd OpenHandler.php🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/debugbar/assets/openhandler.js` around lines 236 - 247, The clear method currently sends a destructive op via GET which is vulnerable to CSRF; update ClearHandler by changing clear() to send the request as POST (not GET) through ajax(), include the CSRF token in the payload (e.g., pull token via this.get('token') or similar accessor), and modify ajax(data, callback) to use fetch with method: 'POST', set appropriate form-encoded headers and send new URLSearchParams(data) in the request body instead of appending to the URL (ensure get('url') is used only as the endpoint). Also confirm server-side expects POST and token name matches backend (e.g., xoopsSecurity token) and include that token key in the data you post.
252-256:⚠️ Potential issue | 🟠 MajorAJAX error path drops the error argument and breaks callback contract.
Failure currently calls
callback([])only. Callers expecting(data, err)cannot distinguish empty results from transport/parsing errors.Suggested fix
- fetch(url, { + fetch(url, { method: 'GET', headers: { Accept: 'application/json' } }) - .then(data => data.json()) - .then(callback) - .catch(() => { - callback([]); + .then((response) => { + if (!response.ok) { + throw new Error(`OpenHandler request failed with status ${response.status}`); + } + return response.json(); + }) + .then((payload) => callback(payload, null)) + .catch((err) => { + callback([], err); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/debugbar/assets/openhandler.js` around lines 252 - 256, The AJAX error path currently swallows the error and calls callback([]), breaking the expected (data, err) contract; update the promise handlers so successful responses call callback(data, null) (replace .then(callback) with .then(data => callback(data, null))) and the catch passes the error as the second argument (change .catch(() => { callback([]); }) to .catch(err => { callback([], err); })) so callers can distinguish empty results from transport/parsing errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@htdocs/modules/debugbar/assets/debugbar.js`:
- Around line 259-260: Replace action-oriented anchor elements with semantic
buttons: change code creating this.tab (and the other toolbar/tab elements
around the 873-975 block) from document.createElement('a') to
document.createElement('button'), add type="button" and keep the same
csscls('tab') class, and ensure any existing click/key event listeners work with
buttons; remove any reliance on href attributes and update ARIA attributes
(role/aria-pressed/aria-controls) if previously used for accessibility. Also
update any CSS/selector logic that specifically targets anchor tags to target
the class (e.g., .<tab-class>) so styling/behavior remains unchanged.
- Around line 1624-1629: The JSON.parse call reading the stack header can throw
on malformed input; update the code in the handler that reads this header (use
getHeader(this, response, `${this.headerName}-stack`) context or the same block
where headerName and getHeader are used) to wrap JSON.parse(stack) in a
try-catch, and if parsing fails, swallow or log the error (but do not rethrow)
and skip iterating stackIds so subsequent calls like
this.debugbar.loadDataSet(...) still run for any other valid data; ensure you
reference the same symbols (getHeader, headerName, debugbar.loadDataSet) when
making the change.
- Around line 656-657: The condition using Number.isNaN(filename) is incorrect
because filename is a string; parse filename to a number first and test that
parsed value for NaN before using it: convert filename (e.g., via
Number(filename) or parseInt/parseFloat as appropriate) to a numeric variable
(e.g., parsedFilename) and then use if (uri.length &&
!Number.isNaN(parsedFilename)) to decide whether to prepend uri.pop() to
filename; update references in the same block that use filename and
parsedFilename accordingly.
In `@htdocs/modules/debugbar/assets/widgets.js`:
- Around line 653-658: Replace non-semantic anchors used as action triggers with
real buttons: where the code creates an anchor element (const link =
document.createElement('a')) in widgets.js (and the similar blocks around the
other occurrences), create a button element instead
(document.createElement('button')) and set button.type = 'button'; keep the same
classList additions (csscls('filter') and csscls(type)), textContent, and event
listener logic, and move any semantic attributes like rel to a data- attribute
(e.g., data-rel) if needed for behavior; update all similar blocks referenced
(the current diff, the block at 682-686, and 1100-1129) so they use buttons to
restore keyboard/accessibility semantics.
- Around line 16-18: The htmlize function (PhpDebugBar.Widgets.htmlize)
currently only converts newlines and spaces and fails to escape HTML entities,
allowing injection when its return is later used with innerHTML; update htmlize
to first escape the characters &, <, >, " and ' (replace & first to avoid
double-encoding), then convert newlines to <br> and spaces to so the
returned string contains escaped HTML plus the intended whitespace markup,
preserving safe fragments for insertion as innerHTML.
In `@htdocs/modules/debugbar/assets/widgets/http/widget.js`:
- Around line 20-67: The code sets many inline styles on the DOM nodes (table,
method, url, status, duration) — replace these inline style mutations with
semantic CSS classes and move the visual rules into the widget stylesheet: add
classes (e.g., csscls('summary-row') on the table, csscls('method'),
csscls('url'), csscls('status'), csscls('duration')) and remove style.*
assignments in the code that manipulate display, gap, alignItems, fontWeight,
minWidth, flex, overflow, textOverflow, whiteSpace, textAlign and color logic;
keep only structural/data assignments (textContent, classList.add) and implement
the layout, spacing, truncation and status color rules in the CSS rule set for
those classes, including status color rules via status class variants or
modifiers rather than inline color.
---
Duplicate comments:
In `@htdocs/modules/debugbar/assets/openhandler.js`:
- Around line 142-150: The handlers for the row actions (e.g., loadLink and
methodLink) are created and wired but never appended to the DOM, so make them
reachable: either append loadLink and methodLink into the row's actions
container (create or use an existing cell like an actionsCell or metaActions
element) so clicks call self.hide(), self.load(meta.id, ...) and
self.callback(meta.id, data), or remove the unused element creation/listeners
entirely; update the code paths around the loadLink and methodLink creation
points (the addEventListener blocks) to insert the corresponding anchor elements
into the row UI so the quick-filter/method actions work.
- Around line 109-112: The form serialization loop that iterates over
formData.entries() and assigns to the search object (variables name/value and
search) is treating the placeholder "(Method)" as a real value; update the check
so that you only set search[name] when value is truthy and not equal to the
placeholder string "(Method)" (and apply the same guard in the other identical
serialization block that also processes formData.entries()); this ensures
placeholder values are ignored before assigning to search.
- Around line 20-23: Replace anchor-only interactive elements with semantic
buttons: instead of creating an <a> for controls like this.closebtn (and similar
elements created in the regions referenced for load/refresh/clear and row-level
actions), create a HTMLButtonElement, set type="button", copy over the same
class list (csscls('closebtn') etc.) and innerHTML/icon markup, and reattach the
existing event listeners and attributes; ensure any aria-labels or role
attributes required for accessibility are preserved or added. Locate the element
creations (e.g., this.closebtn, load/refresh/clear control variables and row
action elements) and replace document.createElement('a') with
document.createElement('button') and set button.type = 'button' while keeping
visual classes and behavior unchanged.
- Around line 236-247: The clear method currently sends a destructive op via GET
which is vulnerable to CSRF; update ClearHandler by changing clear() to send the
request as POST (not GET) through ajax(), include the CSRF token in the payload
(e.g., pull token via this.get('token') or similar accessor), and modify
ajax(data, callback) to use fetch with method: 'POST', set appropriate
form-encoded headers and send new URLSearchParams(data) in the request body
instead of appending to the URL (ensure get('url') is used only as the
endpoint). Also confirm server-side expects POST and token name matches backend
(e.g., xoopsSecurity token) and include that token key in the data you post.
- Around line 252-256: The AJAX error path currently swallows the error and
calls callback([]), breaking the expected (data, err) contract; update the
promise handlers so successful responses call callback(data, null) (replace
.then(callback) with .then(data => callback(data, null))) and the catch passes
the error as the second argument (change .catch(() => { callback([]); }) to
.catch(err => { callback([], err); })) so callers can distinguish empty results
from transport/parsing errors.
In `@htdocs/modules/debugbar/assets/widgets.js`:
- Line 686: Change the expression-bodied arrow callback on collectors.forEach to
a block-bodied callback so it satisfies the Biome rule; update the call that
currently uses collectors.forEach(collector => createFilterItem('collector',
collector)) to use a block body and call createFilterItem('collector',
collector) inside it (referencing the collectors.forEach invocation and the
createFilterItem function).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
htdocs/modules/debugbar/assets/debugbar.jshtdocs/modules/debugbar/assets/openhandler.jshtdocs/modules/debugbar/assets/widgets.jshtdocs/modules/debugbar/assets/widgets/http/widget.jshtdocs/modules/debugbar/assets/widgets/templates/widget.js
Comment on lines
+259
to
+260
| this.tab = document.createElement('a'); | ||
| this.tab.classList.add(csscls('tab')); |
There was a problem hiding this comment.
Core toolbar/tab controls should not be anchor elements without href.
Tabs and header controls are action buttons, not navigation links. Rendering them as <a> without href creates keyboard and semantics issues.
Also applies to: 873-975
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/modules/debugbar/assets/debugbar.js` around lines 259 - 260, Replace
action-oriented anchor elements with semantic buttons: change code creating
this.tab (and the other toolbar/tab elements around the 873-975 block) from
document.createElement('a') to document.createElement('button'), add
type="button" and keep the same csscls('tab') class, and ensure any existing
click/key event listeners work with buttons; remove any reliance on href
attributes and update ARIA attributes (role/aria-pressed/aria-controls) if
previously used for accessibility. Also update any CSS/selector logic that
specifically targets anchor tags to target the class (e.g., .<tab-class>) so
styling/behavior remains unchanged.
Comment on lines
+656
to
+657
| if (uri.length && !Number.isNaN(filename)) { | ||
| filename = `${uri.pop()}/${filename}`; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and examine the debugbar.js file
find . -path "*debugbar/assets/debugbar.js" -type fRepository: XOOPS/XoopsCore25
Length of output: 105
🏁 Script executed:
# Once found, let's examine the code around lines 656-657
if [ -f "htdocs/modules/debugbar/assets/debugbar.js" ]; then
echo "=== Lines 650-665 of debugbar.js ==="
sed -n '650,665p' "htdocs/modules/debugbar/assets/debugbar.js" | cat -n
echo ""
echo "=== Looking for context about filename variable ==="
sed -n '640,670p' "htdocs/modules/debugbar/assets/debugbar.js" | cat -n
fiRepository: XOOPS/XoopsCore25
Length of output: 2175
🏁 Script executed:
# Search for the function/context containing this code
rg -B 10 -A 5 "Number\.isNaN\(filename\)" --type jsRepository: XOOPS/XoopsCore25
Length of output: 3077
Numeric filename detection is broken; condition is always true for any string.
Number.isNaN(filename) returns false for any string (including non-numeric ones like "hello"), so !Number.isNaN(filename) will be true for nearly all strings. Convert the string to a number first before checking.
Suggested fix
-if (uri.length && !Number.isNaN(filename)) {
+if (uri.length && !Number.isNaN(Number(filename))) {
filename = `${uri.pop()}/${filename}`;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (uri.length && !Number.isNaN(filename)) { | |
| filename = `${uri.pop()}/${filename}`; | |
| if (uri.length && !Number.isNaN(Number(filename))) { | |
| filename = `${uri.pop()}/${filename}`; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/modules/debugbar/assets/debugbar.js` around lines 656 - 657, The
condition using Number.isNaN(filename) is incorrect because filename is a
string; parse filename to a number first and test that parsed value for NaN
before using it: convert filename (e.g., via Number(filename) or
parseInt/parseFloat as appropriate) to a numeric variable (e.g., parsedFilename)
and then use if (uri.length && !Number.isNaN(parsedFilename)) to decide whether
to prepend uri.pop() to filename; update references in the same block that use
filename and parsedFilename accordingly.
Comment on lines
+1624
to
+1629
| const stack = this.getHeader(response, `${this.headerName}-stack`); | ||
| if (stack) { | ||
| const stackIds = JSON.parse(stack); | ||
| stackIds.forEach((id) => { | ||
| this.debugbar.loadDataSet(id, ' (stacked)', null, false); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check if the file exists and get its line count
wc -l htdocs/modules/debugbar/assets/debugbar.jsRepository: XOOPS/XoopsCore25
Length of output: 108
🏁 Script executed:
# Read the specific lines mentioned in the review
sed -n '1620,1635p' htdocs/modules/debugbar/assets/debugbar.jsRepository: XOOPS/XoopsCore25
Length of output: 583
🏁 Script executed:
# Let's also look for any existing error handling around this code
sed -n '1610,1650p' htdocs/modules/debugbar/assets/debugbar.jsRepository: XOOPS/XoopsCore25
Length of output: 1476
Wrap JSON.parse() in try-catch to prevent exception from breaking request handling.
JSON.parse(stack) throws a SyntaxError if the stack header contains malformed JSON, which terminates the handle() method and prevents subsequent debug data loading attempts from executing. Since the stack header originates from HTTP responses, it must be treated as untrusted input.
Suggested fix
const stack = this.getHeader(response, `${this.headerName}-stack`);
if (stack) {
- const stackIds = JSON.parse(stack);
- stackIds.forEach((id) => {
- this.debugbar.loadDataSet(id, ' (stacked)', null, false);
- });
+ try {
+ const stackIds = JSON.parse(stack);
+ stackIds.forEach((id) => {
+ this.debugbar.loadDataSet(id, ' (stacked)', null, false);
+ });
+ } catch (_err) {
+ // ignore malformed stack header and continue normal processing
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const stack = this.getHeader(response, `${this.headerName}-stack`); | |
| if (stack) { | |
| const stackIds = JSON.parse(stack); | |
| stackIds.forEach((id) => { | |
| this.debugbar.loadDataSet(id, ' (stacked)', null, false); | |
| }); | |
| const stack = this.getHeader(response, `${this.headerName}-stack`); | |
| if (stack) { | |
| try { | |
| const stackIds = JSON.parse(stack); | |
| stackIds.forEach((id) => { | |
| this.debugbar.loadDataSet(id, ' (stacked)', null, false); | |
| }); | |
| } catch (_err) { | |
| // ignore malformed stack header and continue normal processing | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/modules/debugbar/assets/debugbar.js` around lines 1624 - 1629, The
JSON.parse call reading the stack header can throw on malformed input; update
the code in the handler that reads this header (use getHeader(this, response,
`${this.headerName}-stack`) context or the same block where headerName and
getHeader are used) to wrap JSON.parse(stack) in a try-catch, and if parsing
fails, swallow or log the error (but do not rethrow) and skip iterating stackIds
so subsequent calls like this.debugbar.loadDataSet(...) still run for any other
valid data; ensure you reference the same symbols (getHeader, headerName,
debugbar.loadDataSet) when making the change.
Comment on lines
+16
to
+18
| const htmlize = PhpDebugBar.Widgets.htmlize = function (text) { | ||
| const str = String(text ?? ''); | ||
| return str.replace(/\n/g, '<br>').replace(/\s/g, ' '); |
There was a problem hiding this comment.
htmlize() must escape HTML entities before returning HTML fragments.
Current implementation only transforms whitespace/newlines. Values containing <, >, &, quotes can execute when later assigned via innerHTML.
Suggested fix
const htmlize = PhpDebugBar.Widgets.htmlize = function (text) {
const str = String(text ?? '');
- return str.replace(/\n/g, '<br>').replace(/\s/g, ' ');
+ return str
+ .replace(/&/g, '&')
+ .replace(/</g, '<')
+ .replace(/>/g, '>')
+ .replace(/"/g, '"')
+ .replace(/'/g, ''')
+ .replace(/\n/g, '<br>')
+ .replace(/\s/g, ' ');
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const htmlize = PhpDebugBar.Widgets.htmlize = function (text) { | |
| const str = String(text ?? ''); | |
| return str.replace(/\n/g, '<br>').replace(/\s/g, ' '); | |
| const htmlize = PhpDebugBar.Widgets.htmlize = function (text) { | |
| const str = String(text ?? ''); | |
| return str | |
| .replace(/&/g, '&') | |
| .replace(/</g, '<') | |
| .replace(/>/g, '>') | |
| .replace(/"/g, '"') | |
| .replace(/'/g, ''') | |
| .replace(/\n/g, '<br>') | |
| .replace(/\s/g, ' '); | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/modules/debugbar/assets/widgets.js` around lines 16 - 18, The htmlize
function (PhpDebugBar.Widgets.htmlize) currently only converts newlines and
spaces and fails to escape HTML entities, allowing injection when its return is
later used with innerHTML; update htmlize to first escape the characters &, <,
>, " and ' (replace & first to avoid double-encoding), then convert newlines to
<br> and spaces to so the returned string contains escaped HTML plus the
intended whitespace markup, preserving safe fragments for insertion as
innerHTML.
Comment on lines
+653
to
+658
| const link = document.createElement('a'); | ||
| link.classList.add(csscls('filter')); | ||
| link.classList.add(csscls(type)); | ||
| link.textContent = value; | ||
| link.setAttribute('rel', value); | ||
| link.addEventListener('click', function () { |
There was a problem hiding this comment.
Use semantic buttons for filter and dataset toolbar actions.
These controls are action triggers but are rendered as <a> without href. Replace with <button type="button"> to restore keyboard semantics and accessibility.
Also applies to: 682-686, 1100-1129
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/modules/debugbar/assets/widgets.js` around lines 653 - 658, Replace
non-semantic anchors used as action triggers with real buttons: where the code
creates an anchor element (const link = document.createElement('a')) in
widgets.js (and the similar blocks around the other occurrences), create a
button element instead (document.createElement('button')) and set button.type =
'button'; keep the same classList additions (csscls('filter') and csscls(type)),
textContent, and event listener logic, and move any semantic attributes like rel
to a data- attribute (e.g., data-rel) if needed for behavior; update all similar
blocks referenced (the current diff, the block at 682-686, and 1100-1129) so
they use buttons to restore keyboard/accessibility semantics.
Comment on lines
+20
to
+67
| table.style.display = 'flex'; | ||
| table.style.gap = '10px'; | ||
| table.style.alignItems = 'center'; | ||
|
|
||
| // METHOD | ||
| const method = document.createElement('span'); | ||
| method.classList.add(csscls('method')); | ||
| method.textContent = request.method; | ||
| method.style.fontWeight = 'bold'; | ||
| method.style.minWidth = '60px'; | ||
| table.append(method); | ||
|
|
||
| // URL | ||
| const url = document.createElement('span'); | ||
| url.classList.add(csscls('url')); | ||
| url.textContent = request.url; | ||
| url.style.flex = '1'; | ||
| url.style.overflow = 'hidden'; | ||
| url.style.textOverflow = 'ellipsis'; | ||
| url.style.whiteSpace = 'nowrap'; | ||
| table.append(url); | ||
|
|
||
| // STATUS | ||
| const status = document.createElement('span'); | ||
| status.classList.add(csscls('status')); | ||
| status.textContent = request.status; | ||
| status.style.minWidth = '40px'; | ||
| status.style.textAlign = 'center'; | ||
| // Color code status | ||
| if (typeof request.status === 'number') { | ||
| if (request.status >= 200 && request.status < 300) { | ||
| status.style.color = '#4caf50'; | ||
| } else if (request.status >= 300 && request.status < 400) { | ||
| status.style.color = '#ff9800'; | ||
| } else if (request.status >= 400) { | ||
| status.style.color = '#f44336'; | ||
| } | ||
| } | ||
| table.append(status); | ||
|
|
||
| // DURATION | ||
| if (request.duration !== null && typeof request.duration !== 'undefined') { | ||
| const duration = document.createElement('span'); | ||
| duration.classList.add(csscls('duration')); | ||
| duration.textContent = request.duration; | ||
| duration.style.minWidth = '60px'; | ||
| duration.style.textAlign = 'right'; | ||
| table.append(duration); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Prefer CSS classes over inline styles for request summary layout.
The repeated inline style mutations make theming and future styling changes harder. Move these visual rules into widget CSS classes and keep JS focused on structure/data.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/modules/debugbar/assets/widgets/http/widget.js` around lines 20 - 67,
The code sets many inline styles on the DOM nodes (table, method, url, status,
duration) — replace these inline style mutations with semantic CSS classes and
move the visual rules into the widget stylesheet: add classes (e.g.,
csscls('summary-row') on the table, csscls('method'), csscls('url'),
csscls('status'), csscls('duration')) and remove style.* assignments in the code
that manipulate display, gap, alignItems, fontWeight, minWidth, flex, overflow,
textOverflow, whiteSpace, textAlign and color logic; keep only structural/data
assignments (textContent, classList.add) and implement the layout, spacing,
truncation and status color rules in the CSS rule set for those classes,
including status color rules via status class variants or modifiers rather than
inline color.
Bug fixes: 1. sqlqueries/widget.js:93 — valueTd was built but never appended to tr when rendering object values (params/backtrace details were invisible) 2. openhandler.js — Removed dead loadLink (created but never appended, duplicated uriLink functionality) 3. openhandler.js — Wired methodLink into the method column so clicking a method filters by it (was plain text before) 4. openhandler.js:110 — Excluded (Method) placeholder from being submitted as a real filter value 5. mails/widget.js:36 — Added event.stopPropagation() to "View Mail" click to prevent it from bubbling up and toggling the headers panel Code quality: 6. highlight.css:131,161 — Fixed malformed CSS selectors with duplicated [data-theme='dark'] (nested .hljs-meta children weren't matching) 7. templates/widget.css:31 — Removed duplicated CSS selector line 8. sqlqueries/widget.js:63 — innerHTML → textContent for caption rendering 9. admin/index.php:27 — Removed unnecessary \constant() wrapper
There was a problem hiding this comment.
Actionable comments posted: 10
♻️ Duplicate comments (2)
htdocs/modules/debugbar/assets/openhandler.js (2)
76-79:⚠️ Potential issue | 🟠 MajorPropagate AJAX errors instead of collapsing them into success.
htdocs/modules/debugbar/assets/widgets.js:1412-1425expectsfind()callbacks to receive(data, err). The currentcallback([])failure path makes failed scans look like empty results, and it also makes a failedclear()still close the panel as if the delete worked.Suggested fix
this.clearbtn.addEventListener('click', () => { - self.clear(() => { - self.hide(); + self.clear((_, err) => { + if (!err) { + self.hide(); + } }); }); ... fetch(url, { method: 'GET', headers: { Accept: 'application/json' } }) - .then(data => data.json()) - .then(callback) - .catch(() => { - callback([]); - }); + .then((response) => { + if (!response.ok) { + throw new Error(`OpenHandler request failed with status ${response.status}`); + } + return response.json(); + }) + .then( + (payload) => { + if (typeof callback === 'function') { + callback(payload, null); + } + }, + (err) => { + if (typeof callback === 'function') { + callback([], err); + } + } + );Also applies to: 236-246
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/debugbar/assets/openhandler.js` around lines 76 - 79, The click handler currently calls self.clear(() => { self.hide(); }) which collapses all errors into success; update the callback signature used by clear() to follow the expected (data, err) pattern used by find() and propagate AJAX errors by passing the error as the second argument (e.g., callback(null, err)) instead of calling callback([]) on failure, and only call self.hide() when err is falsy (i.e., on success); apply the same change to the other clear call in the file (around the 236-246 region) so failed clears do not close the panel and callers receive (data, err).
226-227:⚠️ Potential issue | 🟠 MajorRoute
clearthrough a POST-only code path.
clear()currently goes through the same GET/query-string transport as read operations. If the paired PHP handler still accepts that route, storage deletion remains CSRF- and prefetch-triggerable. This needs a POST request with the XOOPS security token and a server-side method/token check.Read-only verification
#!/bin/bash # Confirm the PHP clear handler rejects GET and validates a XOOPS security token. rg -n -C3 --type=php --glob 'htdocs/modules/debugbar/**' --glob 'htdocs/xoops_lib/vendor/php-debugbar/**' 'op.*clear|clear.*op|REQUEST_METHOD|xoopsSecurity|checkToken|security'Also applies to: 236-237
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/debugbar/assets/openhandler.js` around lines 226 - 227, The clear() client call currently issues a GET-style request; change it to POST and include the XOOPS security token so server-side CSRF checks can validate the action. Specifically, in openhandler.js update the clear() method (and the analogous call at lines ~236-237) to send a POST request via this.ajax with a payload like { op: 'clear', [tokenName]: tokenValue } where tokenName matches the server-side expected token parameter and tokenValue is read from the existing XOOPS/global token holder (e.g., a xoopsSecurityToken or similar global/hidden input accessor used elsewhere in the app). Ensure the ajax options explicitly set method: 'POST' (and appropriate content type) so the request goes through the POST-only path the PHP handler requires.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@htdocs/modules/debugbar/assets/openhandler.js`:
- Around line 102-117: Replace the button 'click' handlers with form 'submit'
handlers so implicit submissions and Enter key presses work and you don't rely
on this.parentElement; locate the code that creates searchBtn and its click
listener (and the similar block at lines 119-136) and: attach a 'submit'
listener to the actual <form> element, call event.preventDefault(), build
FormData from event.currentTarget (the form) instead of this.parentElement,
filter values the same way, then call self.find(search, 0,
self.handleFind.bind(self)); remove the button-specific click handlers after
moving logic to the form submit handler.
- Around line 216-219: The pager can remain hidden across queries because
find(filters, offset, callback) never resets the pager visibility for fresh
queries; update the find method (and/or the caller handleFind) so that when
starting a fresh query (offset === 0 or no offset) you reset the pager state —
e.g., clear any page-related counters and explicitly show the loadmore button
element (reference: find, handleFind, loadmorebtn, last_find_request) before
making the ajax call; keep the existing hide behavior for short result sets
after results return.
In `@htdocs/modules/debugbar/assets/widgets/mails/widget.js`:
- Around line 36-39: The click handler for link (in the link.addEventListener
callback) assumes window.open returned a valid window and immediately
dereferences popup.document; guard against popup being null or closed by
checking the result of window.open (e.g., if (!popup || popup.closed) return or
handle gracefully) before accessing documentToWriteTo, and ensure any subsequent
writes or focus calls on popup are only executed when popup is valid.
- Around line 52-65: The code creates a "Text version" <details> block
unconditionally when mail.html exists, causing an empty preview if mail.body is
absent; update the logic around mail.html handling in the widget so you only
create and append the <details> and <summary> elements (and append bodyPre) when
mail.body is truthy (e.g., check mail.body before creating details/summary),
otherwise leave bodyHTML as bodyPre.outerHTML or an empty string as appropriate;
refer to the variables bodyPre, bodyHTML, htmlIframeHTML, mail.body, mail.html,
details, and summary to find and change the branch that constructs the Text
version block.
- Around line 67-72: The preview currently sets the iframe srcdoc directly from
mail.html (see htmlIframe and setAttribute('srcdoc', mail.html)), which still
allows remote img/link/CSS to load; fix by sanitizing and locking down the HTML
before assigning it: generate a sanitizedHtml from mail.html that (1) injects a
strict CSP meta tag into the top of the document to block external network
requests (e.g. default-src 'none'; img-src data:; style-src 'unsafe-inline'
data:), and (2) strips or rewrites remote src/href attributes on <img>, <link>,
<script>, <source>, <iframe> and inline CSS url(...) to data: or remove them;
then set htmlIframe.setAttribute('srcdoc', sanitizedHtml) while keeping sandbox
and referrerpolicy.
In `@htdocs/modules/debugbar/assets/widgets/sqlqueries/widget.js`:
- Around line 289-295: The sortState and originalData variables are declared
outside the per-dataset lifecycle and persist across updates causing stale rows;
move their state to be reset/scoped when new data is applied (e.g., make them
properties on the widget instance or reinitialize them inside the data/setData
handler that processes data.statements) so that this.list rendering uses the
current dataset only; specifically update references to sortState and
originalData near the this.list creation and inside any data update logic (the
setData/data setter or the handler that assigns this.list.setData) to reassign
sortState = 'none' and originalData = null whenever new data arrives.
- Around line 167-174: Replace mouse-only anchors/spans with proper
keyboard-focusable buttons: change filterLink and toggleLink from <a> without
href to <button type="button"> (preserve csscls('filter')/csscls('toggle')
classes and innerText), change copyBtn and sortIcon from <span> to <button
type="button"> (preserve their classes), move any non-standard attributes like
rel to data-rel, and attach the same click handlers (e.g.,
this.onFilterClick(filterButton), this.onToggleDuplicateClick(toggleButton),
copy handler, sort handler) so keyboard users can activate them; keep existing
CSS and update any code references to use the new element variables (filterLink
→ filterButton, toggleLink → toggleButton etc.) and add appropriate aria-labels
when the intent isn't clear from text.
- Around line 56-69: renderList currently always creates/appends a new <thead>,
producing multiple theads when the same table is rendered twice (e.g., params +
backtrace). Change renderList (and the similar block at the other occurrence) to
reuse an existing thead when present: before creating/appending a new thead,
check table.querySelector('thead') and if one exists, insert the caption row
into that existing thead (or skip creating a new thead altogether); otherwise
create and append a thead as now. Ensure you reference and update the renderList
function and the duplicate-thethead block at the other occurrence so no table
ends up with more than one <thead>.
- Around line 298-310: The data-binding callback (this.bindAttr('data', ...))
returns early when the PDO collector is empty but does not clear previous UI
state; before the early return, clear this.list (set empty data), clear
this.status.innerHTML, reset this.filters = [], hide toolbar
(this.toolbar.hidden = true), and remove existing filter chips (same loop over
toolbar.querySelectorAll(`.${csscls('filter')}`)) so stale queries/filters are
not shown on the next request.
In `@htdocs/modules/debugbar/assets/widgets/templates/widget.css`:
- Line 44: The CSS property background-color in the widget template uses the
mixed-case keyword "currentColor"; update it to the lowercase "currentcolor" to
satisfy the value-keyword-case Stylelint rule and maintain consistent CSS
keyword casing (locate the background-color: currentColor; declaration in the
widget.css template and replace the value with currentcolor).
---
Duplicate comments:
In `@htdocs/modules/debugbar/assets/openhandler.js`:
- Around line 76-79: The click handler currently calls self.clear(() => {
self.hide(); }) which collapses all errors into success; update the callback
signature used by clear() to follow the expected (data, err) pattern used by
find() and propagate AJAX errors by passing the error as the second argument
(e.g., callback(null, err)) instead of calling callback([]) on failure, and only
call self.hide() when err is falsy (i.e., on success); apply the same change to
the other clear call in the file (around the 236-246 region) so failed clears do
not close the panel and callers receive (data, err).
- Around line 226-227: The clear() client call currently issues a GET-style
request; change it to POST and include the XOOPS security token so server-side
CSRF checks can validate the action. Specifically, in openhandler.js update the
clear() method (and the analogous call at lines ~236-237) to send a POST request
via this.ajax with a payload like { op: 'clear', [tokenName]: tokenValue } where
tokenName matches the server-side expected token parameter and tokenValue is
read from the existing XOOPS/global token holder (e.g., a xoopsSecurityToken or
similar global/hidden input accessor used elsewhere in the app). Ensure the ajax
options explicitly set method: 'POST' (and appropriate content type) so the
request goes through the POST-only path the PHP handler requires.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0b4f4dd8-4998-432c-9259-1183ec50d9be
📒 Files selected for processing (6)
htdocs/modules/debugbar/admin/index.phphtdocs/modules/debugbar/assets/highlight.csshtdocs/modules/debugbar/assets/openhandler.jshtdocs/modules/debugbar/assets/widgets/mails/widget.jshtdocs/modules/debugbar/assets/widgets/sqlqueries/widget.jshtdocs/modules/debugbar/assets/widgets/templates/widget.css
Comment on lines
+102
to
+117
| const searchBtn = document.createElement('button'); | ||
| searchBtn.textContent = 'Search'; | ||
| searchBtn.type = 'submit'; | ||
| searchBtn.addEventListener('click', function (e) { | ||
| self.table.innerHTML = ''; | ||
| const search = {}; | ||
| const formData = new FormData(this.parentElement); | ||
| for (const [name, value] of formData.entries()) { | ||
| if (value && !(name === 'method' && value.startsWith('('))) { | ||
| search[name] = value; | ||
| } | ||
| } | ||
|
|
||
| self.find(search, 0, self.handleFind.bind(self)); | ||
| e.preventDefault(); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Handle the filter form on submit, not the button click.
This is more robust for implicit submissions and avoids depending on this.parentElement to find the form.
Suggested refactor
- const searchBtn = document.createElement('button');
- searchBtn.textContent = 'Search';
- searchBtn.type = 'submit';
- searchBtn.addEventListener('click', function (e) {
- self.table.innerHTML = '';
- const search = {};
- const formData = new FormData(this.parentElement);
- for (const [name, value] of formData.entries()) {
- if (value && !(name === 'method' && value.startsWith('('))) {
- search[name] = value;
- }
- }
-
- self.find(search, 0, self.handleFind.bind(self));
- e.preventDefault();
- });
-
const form = document.createElement('form');
form.innerHTML = '<br/><b>Filter results</b><br/>'
+ '<select name="method"><option selected>(Method)</option><option>GET</option><option>POST</option><option>PUT</option><option>DELETE</option></select>';
+
+ const searchBtn = document.createElement('button');
+ searchBtn.textContent = 'Search';
+ searchBtn.type = 'submit';
+ form.addEventListener('submit', (e) => {
+ e.preventDefault();
+ self.table.innerHTML = '';
+ const search = {};
+ const formData = new FormData(form);
+ for (const [name, value] of formData.entries()) {
+ if (value && !(name === 'method' && value.startsWith('('))) {
+ search[name] = value;
+ }
+ }
+ self.find(search, 0, self.handleFind.bind(self));
+ });Also applies to: 119-136
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/modules/debugbar/assets/openhandler.js` around lines 102 - 117,
Replace the button 'click' handlers with form 'submit' handlers so implicit
submissions and Enter key presses work and you don't rely on this.parentElement;
locate the code that creates searchBtn and its click listener (and the similar
block at lines 119-136) and: attach a 'submit' listener to the actual <form>
element, call event.preventDefault(), build FormData from event.currentTarget
(the form) instead of this.parentElement, filter values the same way, then call
self.find(search, 0, self.handleFind.bind(self)); remove the button-specific
click handlers after moving logic to the form submit handler.
Comment on lines
+67
to
+72
| const htmlIframe = document.createElement('iframe'); | ||
| htmlIframe.setAttribute('width', '100%'); | ||
| htmlIframe.setAttribute('height', '400px'); | ||
| htmlIframe.setAttribute('sandbox', ''); | ||
| htmlIframe.setAttribute('referrerpolicy', 'no-referrer'); | ||
| htmlIframe.setAttribute('srcdoc', mail.html); |
There was a problem hiding this comment.
Block remote assets inside the HTML preview.
Assigning raw mail.html to srcdoc still allows remote <img>, <link>, and CSS URLs in the message to load when the preview opens. That leaks the developer's browser to third-party hosts and tracking pixels despite sandbox and referrerpolicy.
Suggested fix
const htmlIframe = document.createElement('iframe');
htmlIframe.setAttribute('width', '100%');
htmlIframe.setAttribute('height', '400px');
htmlIframe.setAttribute('sandbox', '');
htmlIframe.setAttribute('referrerpolicy', 'no-referrer');
- htmlIframe.setAttribute('srcdoc', mail.html);
+ const csp = `<meta http-equiv="Content-Security-Policy" content="default-src 'none'; img-src data: cid:; style-src 'unsafe-inline'">`;
+ htmlIframe.setAttribute('srcdoc', `${csp}${mail.html}`);
htmlIframeHTML = htmlIframe.outerHTML;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/modules/debugbar/assets/widgets/mails/widget.js` around lines 67 - 72,
The preview currently sets the iframe srcdoc directly from mail.html (see
htmlIframe and setAttribute('srcdoc', mail.html)), which still allows remote
img/link/CSS to load; fix by sanitizing and locking down the HTML before
assigning it: generate a sanitizedHtml from mail.html that (1) injects a strict
CSP meta tag into the top of the document to block external network requests
(e.g. default-src 'none'; img-src data:; style-src 'unsafe-inline' data:), and
(2) strips or rewrites remote src/href attributes on <img>, <link>, <script>,
<source>, <iframe> and inline CSS url(...) to data: or remove them; then set
htmlIframe.setAttribute('srcdoc', sanitizedHtml) while keeping sandbox and
referrerpolicy.
Comment on lines
+56
to
+69
| renderList(table, caption, data) { | ||
| const thead = document.createElement('thead'); | ||
|
|
||
| const tr = document.createElement('tr'); | ||
| const nameTh = document.createElement('th'); | ||
| nameTh.colSpan = 2; | ||
| nameTh.classList.add(csscls('name')); | ||
| nameTh.textContent = caption; | ||
| tr.append(nameTh); | ||
| thead.append(tr); | ||
|
|
||
| table.append(thead); | ||
|
|
||
| const tbody = document.createElement('tbody'); |
There was a problem hiding this comment.
renderList() creates invalid table markup when both sections are present.
The helper always appends a <thead>, but a query with both params and backtrace calls it twice on the same table. That leaves multiple <thead> elements in one table.
🛠️ Minimal fix
renderList(table, caption, data) {
- const thead = document.createElement('thead');
-
- const tr = document.createElement('tr');
+ const tbody = document.createElement('tbody');
+ const headerTr = document.createElement('tr');
const nameTh = document.createElement('th');
nameTh.colSpan = 2;
nameTh.classList.add(csscls('name'));
nameTh.textContent = caption;
- tr.append(nameTh);
- thead.append(tr);
-
- table.append(thead);
-
- const tbody = document.createElement('tbody');
+ headerTr.append(nameTh);
+ tbody.append(headerTr);Also applies to: 245-257
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/modules/debugbar/assets/widgets/sqlqueries/widget.js` around lines 56
- 69, renderList currently always creates/appends a new <thead>, producing
multiple theads when the same table is rendered twice (e.g., params +
backtrace). Change renderList (and the similar block at the other occurrence) to
reuse an existing thead when present: before creating/appending a new thead,
check table.querySelector('thead') and if one exists, insert the caption row
into that existing thead (or skip creating a new thead altogether); otherwise
create and append a thead as now. Ensure you reference and update the renderList
function and the duplicate-thethead block at the other occurrence so no table
ends up with more than one <thead>.
Comment on lines
+167
to
+174
| const filterLink = document.createElement('a'); | ||
| filterLink.classList.add(csscls('filter')); | ||
| filterLink.textContent = stmt.connection; | ||
| filterLink.setAttribute('rel', stmt.connection); | ||
| filterLink.addEventListener('click', () => { | ||
| this.onFilterClick(filterLink); | ||
| }); | ||
| this.toolbar.append(filterLink); |
There was a problem hiding this comment.
Use keyboard-focusable controls for these actions.
filterLink/toggleLink are anchors without href, and copyBtn/sortIcon are clickable <span> elements. Those controls are mouse-only in practice, so filtering, duplicate toggling, sorting, and copy cannot be triggered from the keyboard.
🛠️ Example fix pattern
- const toggleLink = document.createElement('a');
+ const toggleLink = document.createElement('button');
+ toggleLink.type = 'button';
toggleLink.classList.add(csscls('duplicates'));
toggleLink.textContent = duplicatedText;Apply the same button pattern to filterLink, copyBtn, and sortIcon while keeping the existing CSS classes.
Also applies to: 182-190, 355-367, 376-402
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/modules/debugbar/assets/widgets/sqlqueries/widget.js` around lines 167
- 174, Replace mouse-only anchors/spans with proper keyboard-focusable buttons:
change filterLink and toggleLink from <a> without href to <button type="button">
(preserve csscls('filter')/csscls('toggle') classes and innerText), change
copyBtn and sortIcon from <span> to <button type="button"> (preserve their
classes), move any non-standard attributes like rel to data-rel, and attach the
same click handlers (e.g., this.onFilterClick(filterButton),
this.onToggleDuplicateClick(toggleButton), copy handler, sort handler) so
keyboard users can activate them; keep existing CSS and update any code
references to use the new element variables (filterLink → filterButton,
toggleLink → toggleButton etc.) and add appropriate aria-labels when the intent
isn't clear from text.
| mask-size: contain; | ||
| mask-repeat: no-repeat; | ||
| mask-position: center; | ||
| background-color: currentColor; |
There was a problem hiding this comment.
Use lowercase currentcolor per CSS value-keyword-case convention.
Stylelint flags this as a violation. While CSS keywords are case-insensitive, consistent lowercase improves maintainability.
🔧 Proposed fix
- background-color: currentColor;
+ background-color: currentcolor;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| background-color: currentColor; | |
| background-color: currentcolor; |
🧰 Tools
🪛 Stylelint (17.4.0)
[error] 44-44: Expected "currentColor" to be "currentcolor" (value-keyword-case)
(value-keyword-case)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/modules/debugbar/assets/widgets/templates/widget.css` at line 44, The
CSS property background-color in the widget template uses the mixed-case keyword
"currentColor"; update it to the lowercase "currentcolor" to satisfy the
value-keyword-case Stylelint rule and maintain consistent CSS keyword casing
(locate the background-color: currentColor; declaration in the widget.css
template and replace the value with currentcolor).
Resolve conflict in LostPassSecurity.php by accepting master's version (refactored rate limiter with XoopsTokenHandler). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Comment on lines
+19
to
+21
| const items = this.list.el.querySelectorAll(`li[connection="${connection}"]`); | ||
| for (const item of items) { | ||
| item.hidden = !item.hidden; |
There was a problem hiding this comment.
The connection value from statement data is interpolated directly into a CSS attribute selector without escaping: `li[connection="${connection}"]`. If stmt.connection ever contained characters like "] or \, it could break the selector or cause unexpected behavior. Consider using CSS.escape(connection) for the attribute value, or use querySelectorAll with a different filtering approach such as iterating over items and comparing via getAttribute.
Suggested change
| const items = this.list.el.querySelectorAll(`li[connection="${connection}"]`); | |
| for (const item of items) { | |
| item.hidden = !item.hidden; | |
| const items = this.list.el.querySelectorAll('li[connection]'); | |
| for (const item of items) { | |
| if (item.getAttribute('connection') === connection) { | |
| item.hidden = !item.hidden; | |
| } |
Comment on lines
25
to
53
| @@ -45,197 +51,368 @@ | |||
| window.getSelection().removeAllRanges(); | |||
| }; | |||
| select(code); | |||
There was a problem hiding this comment.
document.execCommand('copy') is deprecated and will eventually be removed from browsers. Since this PR modernizes the codebase away from jQuery, consider also migrating to the modern navigator.clipboard.writeText() API, which is available in all current browsers. For example: navigator.clipboard.writeText(code.textContent).then(...). This would also simplify the code by removing the select() helper.
Comment on lines
+290
to
+291
| let sortState = 'none'; // 'none', 'asc', 'desc' | ||
| let originalData = null; |
There was a problem hiding this comment.
sortState and originalData are let variables in render() scope, but they are not reset when the bindAttr callback fires again (e.g., on dataset switch). A new sort icon is created each time with the initial text 'Sort ⇅', but sortState might still be 'desc' or 'asc' from a previous dataset. This means the first click on the new sort icon will follow the stale state (e.g., skip to 'asc' instead of starting from 'desc'). Reset sortState = 'none' and originalData = null at the beginning of the bindAttr callback to ensure consistent behavior when data changes.
| link.classList.add(csscls('editor-link')); | ||
| link.addEventListener('click', (event) => { | ||
| event.stopPropagation(); | ||
| const popup = window.open('about:blank', 'Mail Preview', 'width=650,height=440,scrollbars=yes'); |
There was a problem hiding this comment.
window.open() can return null when the browser blocks the popup. Line 39 will throw TypeError: Cannot read properties of null (reading 'document') in that case. Add a null check: if (!popup) return; after the window.open() call.
Suggested change
| const popup = window.open('about:blank', 'Mail Preview', 'width=650,height=440,scrollbars=yes'); | |
| const popup = window.open('about:blank', 'Mail Preview', 'width=650,height=440,scrollbars=yes'); | |
| if (!popup) { | |
| return; | |
| } |
- mails/widget.js: guard window.open() null return when popup is blocked, skip empty "Text version" section for HTML-only mails - sqlqueries/widget.js: clear stale data when collector is empty, reset sort state on dataset switch, escape connection value in CSS selector with CSS.escape(), fix multiple thead elements by using tbody sections instead - openhandler.js: re-show loadmore button when starting a fresh query at offset 0 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
| li.style.cursor = 'pointer'; | ||
| li.addEventListener('click', () => { | ||
| if (window.getSelection().type === 'Range') { | ||
| return ''; |
There was a problem hiding this comment.
The click handler returns an empty string (return '';) instead of return;. While the return value of a DOM event listener is typically ignored, returning an empty string is misleading. This should be return; to be consistent with the templates widget (line 115 of widgets/templates/widget.js).
Suggested change
| return ''; | |
| return; |
| content: "ℹ"; | ||
| color: #cbcf38; | ||
| } | ||
| div.phpdebugbar-widgets-messages li.phpdebugbar-widgets-list-item span.phpdebugbar-widgets-value.phpdebugbar-widgets-debug:before { |
There was a problem hiding this comment.
The .phpdebugbar-widgets-debug:before pseudo-element sets a color but is missing the content property. The old code had content: "\f188" (Font Awesome bug icon), and the other message levels have been updated with Unicode character content (e.g., "✓", "⚠", "✖", "ℹ"). Without content, this pseudo-element won't render at all, meaning debug-level messages will have no icon. A content value like "🐛" or "⚙" should be added.
Suggested change
| div.phpdebugbar-widgets-messages li.phpdebugbar-widgets-list-item span.phpdebugbar-widgets-value.phpdebugbar-widgets-debug:before { | |
| div.phpdebugbar-widgets-messages li.phpdebugbar-widgets-list-item span.phpdebugbar-widgets-value.phpdebugbar-widgets-debug:before { | |
| content: "🐛"; |
| } | ||
| if (stmt.backtrace && Object.keys(stmt.backtrace).length > 0) { | ||
| const values = []; | ||
| for (const trace of stmt.backtrace.values()) { |
There was a problem hiding this comment.
stmt.backtrace is a plain JavaScript object (parsed from JSON), not a Map, so it does not have a .values() method. Calling stmt.backtrace.values() will throw a TypeError at runtime. Use Object.values(stmt.backtrace) instead.
Suggested change
| for (const trace of stmt.backtrace.values()) { | |
| for (const trace of Object.values(stmt.backtrace)) { |
| errorSpan.textContent = `[${stmt.error_code}] ${stmt.error_message}`; | ||
| li.append(errorSpan); | ||
| } | ||
|
|
There was a problem hiding this comment.
Trailing whitespace on this line.
Suggested change
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (6)
htdocs/modules/debugbar/assets/openhandler.js (3)
230-241:⚠️ Potential issue | 🔴 CriticalDo not send
clearover GET.
clear()is state-changing, but it still goes through the query-string GET path. That keeps it exposed to CSRF, prefetch, and caching side effects. Send it as POST with a CSRF token and have the server reject non-POST clears.Run this to confirm the backend
clearpath enforces method and token checks. Expect a POST-only branch with XOOPS token validation before storage is mutated.#!/bin/bash fd 'OpenHandler\.php$' htdocs/modules/debugbar htdocs/xoops_lib/vendor/php-debugbar rg -n "REQUEST_METHOD|xoopsSecurity|check.*Token|\\bclear\\b|\\$_(GET|POST|REQUEST)" htdocs/modules/debugbar htdocs/xoops_lib/vendor/php-debugbar -C2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/debugbar/assets/openhandler.js` around lines 230 - 241, The clear() path currently sends state-changing data via GET in openhandler.js: modify ajax() so that when data.op === 'clear' (or when clear() calls it) it sends a POST request instead of GET and appends a CSRF token parameter (retrieve token from existing page/global XOOPS token getter or DOM) in the POST body (use URLSearchParams or FormData) and update clear() to ensure it sets op:'clear' and provides the token; also update the server-side handler to reject non-POST clears and validate the XOOPS security token before mutating storage (ensure OpenHandler.php/clear branch checks REQUEST_METHOD === 'POST' and calls xoopsSecurity->checkToken or equivalent and returns an error for invalid/missing tokens).
20-23:⚠️ Potential issue | 🟠 MajorUse buttons for these actions.
These controls are all rendered as
<a>withouthref, so they are not tabbable and do not announce correctly to assistive tech. Switch them to<button type="button">; the icon-only close button also needs an accessible label.♿ Minimal pattern
- this.closebtn = document.createElement('a'); + this.closebtn = document.createElement('button'); + this.closebtn.type = 'button'; + this.closebtn.setAttribute('aria-label', 'Close open handler'); this.closebtn.classList.add(csscls('closebtn')); this.closebtn.innerHTML = '<i class="phpdebugbar-icon phpdebugbar-icon-x"></i>';Also applies to: 51-76, 142-174
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/debugbar/assets/openhandler.js` around lines 20 - 23, The controls in openhandler.js are created as anchors (e.g., this.closebtn via createElement('a')) without href which breaks keyboard focus and accessibility; change these to createElement('button') with type="button" and ensure the icon-only close control (this.closebtn) has an accessible label (e.g., aria-label or visually hidden text). Apply the same change to the other anchor-based action controls in the file (the similar blocks around lines 51-76 and 142-174) so all interactive controls are keyboard-focusable and announced correctly by assistive tech.
102-117:⚠️ Potential issue | 🟠 MajorHandle filters on the form
submitevent.Pressing Enter in the URI/IP fields bypasses this click handler and falls back to native form submission, so the search logic is skipped. Move the logic to a single
form.addEventListener('submit', ...)and buildFormDatafrom the form itself.Also applies to: 119-136
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/debugbar/assets/openhandler.js` around lines 102 - 117, The click handler on searchBtn skips when Enter submits the form; move the logic into a single form.addEventListener('submit', ...) handler (replace the searchBtn.click listener), build const formData = new FormData(form) using the actual form element instead of this.parentElement, iterate entries to populate the search object (same filtering: skip empty and skip method starting with '('), call self.table.innerHTML = '' then self.find(search, 0, self.handleFind.bind(self)), and call event.preventDefault(); apply the same replacement for the duplicate block referenced around searchBtn/self.find/self.handleFind.htdocs/modules/debugbar/assets/widgets/sqlqueries/widget.js (2)
163-170:⚠️ Potential issue | 🟠 MajorUse keyboard-focusable controls for filter/copy/sort actions.
These controls are still rendered as anchors without
hrefor clickable spans, so keyboard users cannot reliably trigger them. Switch them to<button type="button">and addaria-labels to icon-only buttons like copy.♿ Minimal pattern
- const copyBtn = document.createElement('span'); + const copyBtn = document.createElement('button'); + copyBtn.type = 'button'; + copyBtn.setAttribute('aria-label', 'Copy SQL to clipboard'); copyBtn.setAttribute('title', 'Copy to clipboard'); copyBtn.classList.add(csscls('copy-clipboard'));Also applies to: 178-187, 360-387
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/debugbar/assets/widgets/sqlqueries/widget.js` around lines 163 - 170, The anchor-based filter controls (created as filterLink) are not keyboard-focusable when missing href; replace these <a> elements with <button type="button"> elements (e.g., create a button, keep classList using csscls('filter'), set textContent or innerHTML, and attach the same click handler this.onFilterClick) and move non-URL attributes like rel into a data attribute (e.g., data-connection) if needed; also update other similar controls (the blocks around the other anchor creation sites and the copy/sort icon-only buttons) to use <button type="button"> and add descriptive aria-label attributes for icon-only buttons so they are accessible to keyboard and screen-reader users.
16-23:⚠️ Potential issue | 🟠 MajorFilter visibility is order-dependent and breaks after a re-render.
Both
onFilterClick()and the duplicate toggle flipitem.hiddenin place. That makes combined filters depend on click order, and the laterthis.list.set('data', data.statements)on sort recreates the rows and drops duplicate/hidden state entirely. Keep the active filter flags on the widget and recompute row classes and visibility after every render instead of toggling DOM state.Also applies to: 335-342, 363-371, 388-406
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/debugbar/assets/widgets/sqlqueries/widget.js` around lines 16 - 23, The current onFilterClick toggles DOM state (item.hidden) making filter results order-dependent and lost on re-render; instead add widget-level state (e.g., a Set or Map like this.excludedConnections) and in onFilterClick update that state for the connection rather than toggling item.hidden, then call a single method (e.g., updateRowVisibility or recomputeRowClasses) after any render()/this.list.set('data', data.statements) to apply the active filters to each row by reading the connection attribute and adding/removing the CSS class (csscls('excluded')) and setting hidden accordingly; apply the same pattern to the other toggle locations (the duplicates noted) so visibility is always derived from widget state, not in-place DOM flips.htdocs/modules/debugbar/assets/widgets/mails/widget.js (1)
75-82:⚠️ Potential issue | 🟠 MajorHTML previews still leak remote resources.
srcdoc = mail.htmlstill allows external images, stylesheets, and tracking pixels from the original message to load when a preview is opened. That leaks the developer's browser to third-party hosts. Sanitize the HTML or prepend a restrictive CSP before assigningsrcdoc.🔒 Minimal hardening
- htmlIframe.setAttribute('srcdoc', mail.html); + const csp = `<meta http-equiv="Content-Security-Policy" content="default-src 'none'; img-src data: cid:; style-src 'unsafe-inline'">`; + htmlIframe.setAttribute('srcdoc', `${csp}${mail.html}`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/debugbar/assets/widgets/mails/widget.js` around lines 75 - 82, The current assignment htmlIframe.setAttribute('srcdoc', mail.html) can load remote resources; to fix, either sanitize mail.html to strip or rewrite external resource URLs (e.g., remove external <img>, <link>, <script> or rewrite src/href to data: or local proxies) before assigning it, or prepend a restrictive CSP meta tag to the srcdoc that blocks external loads (for example a meta http-equiv="Content-Security-Policy" with default-src 'none'; img-src 'self' data:; style-src 'self' 'unsafe-inline'). Update the code around htmlIframe/htmlIframeHTML and the htmlIframe.setAttribute('sandbox', '') call so the iframe remains sandboxed (do not add allow-same-origin/allow-top-navigation) and ensure referrerpolicy remains no-referrer; choose one approach (sanitization via a library or CSP meta injection) and apply it consistently before setting srcdoc.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@htdocs/modules/debugbar/assets/openhandler.js`:
- Around line 240-250: The fetch promise currently collapses transport/JSON
failures into an empty payload by calling callback([]) in the catch; update the
fetch handling in the fetch(url, { ... }) chain so you only call the success
path (callback with the parsed dataset) when the response is successful (e.g.,
response.ok and JSON parsed), and for network/JSON errors or non-2xx responses
surface the error separately (e.g., call an error handler or pass an Error
object to a dedicated onError callback) instead of calling callback([]); ensure
any UI methods that hide or clear the panel (clear(), load()) are only invoked
from the success branch (where a proper dataset object is returned) and not from
the catch branch.
In `@htdocs/modules/debugbar/assets/widgets/mails/widget.js`:
- Around line 32-38: Replace the anchor element creation with a real button:
instead of document.createElement('a') create document.createElement('button'),
set its type to "button", keep the title ('Mail Preview'), textContent ('View
Mail'), class addition (csscls('editor-link')), and the existing click handler
that calls event.stopPropagation() and opens the popup; ensure no href is added
and behavior otherwise remains unchanged in widget.js where the variable link is
created and used.
---
Duplicate comments:
In `@htdocs/modules/debugbar/assets/openhandler.js`:
- Around line 230-241: The clear() path currently sends state-changing data via
GET in openhandler.js: modify ajax() so that when data.op === 'clear' (or when
clear() calls it) it sends a POST request instead of GET and appends a CSRF
token parameter (retrieve token from existing page/global XOOPS token getter or
DOM) in the POST body (use URLSearchParams or FormData) and update clear() to
ensure it sets op:'clear' and provides the token; also update the server-side
handler to reject non-POST clears and validate the XOOPS security token before
mutating storage (ensure OpenHandler.php/clear branch checks REQUEST_METHOD ===
'POST' and calls xoopsSecurity->checkToken or equivalent and returns an error
for invalid/missing tokens).
- Around line 20-23: The controls in openhandler.js are created as anchors
(e.g., this.closebtn via createElement('a')) without href which breaks keyboard
focus and accessibility; change these to createElement('button') with
type="button" and ensure the icon-only close control (this.closebtn) has an
accessible label (e.g., aria-label or visually hidden text). Apply the same
change to the other anchor-based action controls in the file (the similar blocks
around lines 51-76 and 142-174) so all interactive controls are
keyboard-focusable and announced correctly by assistive tech.
- Around line 102-117: The click handler on searchBtn skips when Enter submits
the form; move the logic into a single form.addEventListener('submit', ...)
handler (replace the searchBtn.click listener), build const formData = new
FormData(form) using the actual form element instead of this.parentElement,
iterate entries to populate the search object (same filtering: skip empty and
skip method starting with '('), call self.table.innerHTML = '' then
self.find(search, 0, self.handleFind.bind(self)), and call
event.preventDefault(); apply the same replacement for the duplicate block
referenced around searchBtn/self.find/self.handleFind.
In `@htdocs/modules/debugbar/assets/widgets/mails/widget.js`:
- Around line 75-82: The current assignment htmlIframe.setAttribute('srcdoc',
mail.html) can load remote resources; to fix, either sanitize mail.html to strip
or rewrite external resource URLs (e.g., remove external <img>, <link>, <script>
or rewrite src/href to data: or local proxies) before assigning it, or prepend a
restrictive CSP meta tag to the srcdoc that blocks external loads (for example a
meta http-equiv="Content-Security-Policy" with default-src 'none'; img-src
'self' data:; style-src 'self' 'unsafe-inline'). Update the code around
htmlIframe/htmlIframeHTML and the htmlIframe.setAttribute('sandbox', '') call so
the iframe remains sandboxed (do not add allow-same-origin/allow-top-navigation)
and ensure referrerpolicy remains no-referrer; choose one approach (sanitization
via a library or CSP meta injection) and apply it consistently before setting
srcdoc.
In `@htdocs/modules/debugbar/assets/widgets/sqlqueries/widget.js`:
- Around line 163-170: The anchor-based filter controls (created as filterLink)
are not keyboard-focusable when missing href; replace these <a> elements with
<button type="button"> elements (e.g., create a button, keep classList using
csscls('filter'), set textContent or innerHTML, and attach the same click
handler this.onFilterClick) and move non-URL attributes like rel into a data
attribute (e.g., data-connection) if needed; also update other similar controls
(the blocks around the other anchor creation sites and the copy/sort icon-only
buttons) to use <button type="button"> and add descriptive aria-label attributes
for icon-only buttons so they are accessible to keyboard and screen-reader
users.
- Around line 16-23: The current onFilterClick toggles DOM state (item.hidden)
making filter results order-dependent and lost on re-render; instead add
widget-level state (e.g., a Set or Map like this.excludedConnections) and in
onFilterClick update that state for the connection rather than toggling
item.hidden, then call a single method (e.g., updateRowVisibility or
recomputeRowClasses) after any render()/this.list.set('data', data.statements)
to apply the active filters to each row by reading the connection attribute and
adding/removing the CSS class (csscls('excluded')) and setting hidden
accordingly; apply the same pattern to the other toggle locations (the
duplicates noted) so visibility is always derived from widget state, not
in-place DOM flips.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 612a3f14-acd3-4093-b943-dd5aa68be2a4
📒 Files selected for processing (3)
htdocs/modules/debugbar/assets/openhandler.jshtdocs/modules/debugbar/assets/widgets/mails/widget.jshtdocs/modules/debugbar/assets/widgets/sqlqueries/widget.js
Comment on lines
240
to
+250
| fetch(url, { | ||
| method: "GET", | ||
| method: 'GET', | ||
| headers: { | ||
| "Accept": "application/json", | ||
| }, | ||
| Accept: 'application/json' | ||
| } | ||
| }) | ||
| .then((data) => data.json()) | ||
| .then(callback); | ||
| .then(data => data.json()) | ||
| .then(callback) | ||
| .catch(() => { | ||
| callback([]); | ||
| }); |
There was a problem hiding this comment.
Don't collapse transport failures into an empty payload.
The catch path makes network/JSON errors indistinguishable from a valid empty result. clear() will still hide the panel on failure, and load() can pass [] where the callback expects a dataset object. Surface the error separately and only run success UI on successful responses.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/modules/debugbar/assets/openhandler.js` around lines 240 - 250, The
fetch promise currently collapses transport/JSON failures into an empty payload
by calling callback([]) in the catch; update the fetch handling in the
fetch(url, { ... }) chain so you only call the success path (callback with the
parsed dataset) when the response is successful (e.g., response.ok and JSON
parsed), and for network/JSON errors or non-2xx responses surface the error
separately (e.g., call an error handler or pass an Error object to a dedicated
onError callback) instead of calling callback([]); ensure any UI methods that
hide or clear the panel (clear(), load()) are only invoked from the success
branch (where a proper dataset object is returned) and not from the catch
branch.
Comment on lines
+32
to
+38
| const link = document.createElement('a'); | ||
| link.setAttribute('title', 'Mail Preview'); | ||
| link.textContent = 'View Mail'; | ||
| link.classList.add(csscls('editor-link')); | ||
| link.addEventListener('click', (event) => { | ||
| event.stopPropagation(); | ||
| const popup = window.open('about:blank', 'Mail Preview', 'width=650,height=440,scrollbars=yes'); |
There was a problem hiding this comment.
Use a real button for the preview action.
View Mail is rendered as an <a> without href, so it is not keyboard-focusable. Make it a <button type="button"> and keep the existing click handler and stopPropagation().
♿ Minimal fix
- const link = document.createElement('a');
+ const link = document.createElement('button');
+ link.type = 'button';
link.setAttribute('title', 'Mail Preview');
link.textContent = 'View Mail';
link.classList.add(csscls('editor-link'));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const link = document.createElement('a'); | |
| link.setAttribute('title', 'Mail Preview'); | |
| link.textContent = 'View Mail'; | |
| link.classList.add(csscls('editor-link')); | |
| link.addEventListener('click', (event) => { | |
| event.stopPropagation(); | |
| const popup = window.open('about:blank', 'Mail Preview', 'width=650,height=440,scrollbars=yes'); | |
| const link = document.createElement('button'); | |
| link.type = 'button'; | |
| link.setAttribute('title', 'Mail Preview'); | |
| link.textContent = 'View Mail'; | |
| link.classList.add(csscls('editor-link')); | |
| link.addEventListener('click', (event) => { | |
| event.stopPropagation(); | |
| const popup = window.open('about:blank', 'Mail Preview', 'width=650,height=440,scrollbars=yes'); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/modules/debugbar/assets/widgets/mails/widget.js` around lines 32 - 38,
Replace the anchor element creation with a real button: instead of
document.createElement('a') create document.createElement('button'), set its
type to "button", keep the title ('Mail Preview'), textContent ('View Mail'),
class addition (csscls('editor-link')), and the existing click handler that
calls event.stopPropagation() and opens the popup; ensure no href is added and
behavior otherwise remains unchanged in widget.js where the variable link is
created and used.
Comment on lines
+640
to
+642
| if ($this->queryLogMode === 1 && $level !== LogLevel::ERROR) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
This check skips non-error queries in 'slow & errors only' mode, but it does not check whether the query is actually slow. A slow query should still be logged. The condition should also allow queries that exceed the slow query threshold (e.g., checking $isSlow or comparing $duration to $this->slowQueryThreshold) so they are not silently dropped.
| this.$list.$el.find('.' + csscls('list-item')).eq(sql[stmt].keys[i]) | ||
| .addClass(csscls('sql-duplicate')); | ||
| for (let i = 0; i < sql[stmt].keys.length; i++) { | ||
| const listItems = this.list.el.querySelectorAll(`.${csscls('list-item')}`); |
There was a problem hiding this comment.
The querySelectorAll call is executed inside an inner loop (for each duplicate key index), re-querying the DOM on every iteration. Move the query outside both loops (after this.list.set('data', data.statements)) so the NodeList is fetched once and reused.
| if (sortState === 'none') { | ||
| sortState = 'desc'; | ||
| sortIcon.textContent = '↓'; | ||
| originalData = [...data.statements]; |
There was a problem hiding this comment.
When sorting, data.statements.sort(...) mutates the original array in place, so the originalData spread copy is taken from the already-sorted array (on subsequent clicks) rather than the true original order. The originalData should be captured on the first click before the sort, or preferably captured when data first arrives (e.g., in the bindAttr callback before any sort happens).
Suggested change
| originalData = [...data.statements]; | |
| if (!originalData) { | |
| originalData = [...data.statements]; | |
| } |
| if (pageOffset === 0) { | ||
| this.loadmorebtn.style.display = ''; | ||
| } | ||
| const data = Object.assign({ op: 'find' }, filters, { max: this.get('items_per_page'), offset: pageOffset }); |
There was a problem hiding this comment.
The find method now always injects op: 'find' into the request data. However, in the old code the find method did not set op — the server may rely on the absence of op or a different default. If the server's open handler already defaults to 'find' when no op is provided, this is harmless, but if it doesn't recognize op=find it could break. Verify that the server-side open handler accepts op=find for the find/list operation.
Suggested change
| const data = Object.assign({ op: 'find' }, filters, { max: this.get('items_per_page'), offset: pageOffset }); | |
| const data = Object.assign({}, filters, { max: this.get('items_per_page'), offset: pageOffset }); |
| $checkedDirs = []; | ||
| foreach ($vendorPaths as $path) { | ||
| $checkedDirs[] = basename(dirname(dirname(dirname(dirname($path))))) . '/…/' . basename($path); | ||
| $checkedDirs[] = basename(dirname($path, 4)) . '/…/' . basename($path); |
There was a problem hiding this comment.
The dirname() function's second parameter (levels) was introduced in PHP 7.0, but the old code used nested dirname() calls for 4 levels. The refactored dirname($path, 4) is functionally equivalent only if the path has at least 4 directory levels. Since the paths are now shorter ($vendorBase . '/php-debugbar/php-debugbar/resources' has fewer segments than before), dirname($path, 4) may go too far up. Verify this produces the expected error message path.
Suggested change
| $checkedDirs[] = basename(dirname($path, 4)) . '/…/' . basename($path); | |
| $checkedDirs[] = $path; |
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.
Summary by CodeRabbit
New Features
Improvements