Fix frontend highlighter crash when issue ID is missing#1415
Fix frontend highlighter crash when issue ID is missing#1415SteveJonesDev wants to merge 1 commit intodevelopfrom
Conversation
Summary of ChangesHello @SteveJonesDev, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the stability of the frontend highlighter by addressing potential runtime crashes. It introduces defensive programming within the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
WalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively addresses the reported crash in showIssue by adding guards for uninitialized issues and for missing issue IDs. The accompanying tests are a good addition. However, the fix introduces a new critical bug: descriptionOpen is now called in code paths where it will crash because it doesn't expect this.issues to be uninitialized. The new tests unfortunately hide this problem by mocking descriptionOpen. I've provided details in a comment. Additionally, I've suggested a small performance improvement for showIssue.
| if ( ! Array.isArray( this.issues ) ) { | ||
| this.currentButtonIndex = -1; | ||
| this.currentIssueStatus = __( 'The element was not found on the page.', 'accessibility-checker' ); | ||
| this.descriptionOpen( id ); |
There was a problem hiding this comment.
This call to this.descriptionOpen(id) will cause a crash. Inside descriptionOpen (line 762), this.issues.find(...) is called without checking if this.issues is an array. Since in this code block this.issues is not an array, it will lead to a TypeError. The test for this case in showIssue.test.js mocks descriptionOpen, which is why this crash is not caught by tests.
Furthermore, even if this.issues is an array but the id is not found (the case on line 552), descriptionOpen currently does nothing, so the currentIssueStatus message you set is never displayed to the user.
descriptionOpen needs to be refactored to handle these cases: it should not crash if this.issues is not an array, and it should display a message (like currentIssueStatus) when an issue is not found.
| const issue = this.issues.find( ( i ) => i.id === id ); | ||
| this.currentButtonIndex = this.issues.findIndex( ( i ) => i.id === id ); | ||
| if ( ! issue ) { | ||
| this.currentIssueStatus = __( 'The element was not found on the page.', 'accessibility-checker' ); | ||
| this.descriptionOpen( id ); | ||
| return; | ||
| } |
There was a problem hiding this comment.
This block can be made more efficient by using only findIndex and avoiding iterating over the issues array twice (once with find and once with findIndex).
| const issue = this.issues.find( ( i ) => i.id === id ); | |
| this.currentButtonIndex = this.issues.findIndex( ( i ) => i.id === id ); | |
| if ( ! issue ) { | |
| this.currentIssueStatus = __( 'The element was not found on the page.', 'accessibility-checker' ); | |
| this.descriptionOpen( id ); | |
| return; | |
| } | |
| this.currentButtonIndex = this.issues.findIndex( ( i ) => i.id === id ); | |
| if ( -1 === this.currentButtonIndex ) { | |
| this.currentIssueStatus = __( 'The element was not found on the page.', 'accessibility-checker' ); | |
| this.descriptionOpen( id ); | |
| return; | |
| } | |
| const issue = this.issues[ this.currentButtonIndex ]; |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2e065e94bb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if ( ! Array.isArray( this.issues ) ) { | ||
| this.currentButtonIndex = -1; | ||
| this.currentIssueStatus = __( 'The element was not found on the page.', 'accessibility-checker' ); | ||
| this.descriptionOpen( id ); |
There was a problem hiding this comment.
In the new !Array.isArray(this.issues) guard, showIssue still calls descriptionOpen(id) before returning, but descriptionOpen immediately does this.issues.find(...) (same file), so this path still throws a TypeError when issues is null/undefined and the highlighter crashes in the exact scenario this patch is meant to fix. The added Jest test masks this by mocking descriptionOpen, so the runtime crash is not covered.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/frontendHighlighterApp/index.js`:
- Around line 541-546: The guard detects uninitialized this.issues but still
calls descriptionOpen(id), which will call this.issues.find() and throw; either
stop calling descriptionOpen when this.issues is not an array (i.e., remove the
descriptionOpen(id) call in the if block that sets
currentButtonIndex/currentIssueStatus) or add an early-return guard inside
descriptionOpen(dataId) that checks if Array.isArray(this.issues) and returns
immediately if not; update the code around currentButtonIndex/currentIssueStatus
and descriptionOpen to use one of these fixes to avoid the TypeError.
In `@tests/jest/frontendHighlighterApp/showIssue.test.js`:
- Around line 48-64: The test reveals a latent crash because descriptionOpen
assumes this.issues is an array; add a null/array guard inside descriptionOpen
(the method named descriptionOpen should check if Array.isArray(this.issues)
before calling this.issues.find()) and return or handle the "not found" case
safely when issues is null/undefined; alternatively, remove the call to
descriptionOpen from the null-issues branch inside showIssue, but the preferred
fix is to harden descriptionOpen by validating this.issues before using .find().
🧹 Nitpick comments (1)
src/frontendHighlighterApp/index.js (1)
550-554: Guard prevents crash, but error message won't display.This guard correctly prevents accessing
issue.tooltipandissue.elementwhen the issue isn't found. However,descriptionOpenonly renders content whenmatchingObjis truthy (line 764), so thecurrentIssueStatusmessage won't actually be shown to users.This is a minor UX gap—the crash is prevented, but users won't see feedback. Consider logging to console or showing the panel with just the status message.
| if ( ! Array.isArray( this.issues ) ) { | ||
| this.currentButtonIndex = -1; | ||
| this.currentIssueStatus = __( 'The element was not found on the page.', 'accessibility-checker' ); | ||
| this.descriptionOpen( id ); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Calling descriptionOpen when this.issues is null will crash.
The guard correctly detects uninitialized issues, but then calls descriptionOpen(id) which attempts to call this.issues.find() at line 762. This will throw TypeError: Cannot read property 'find' of null, defeating the graceful error handling intent.
🐛 Proposed fix: Skip descriptionOpen when issues aren't initialized
if ( ! Array.isArray( this.issues ) ) {
this.currentButtonIndex = -1;
this.currentIssueStatus = __( 'The element was not found on the page.', 'accessibility-checker' );
- this.descriptionOpen( id );
return;
}Alternatively, add a guard at the start of descriptionOpen:
descriptionOpen( dataId ) {
if ( ! Array.isArray( this.issues ) ) {
return;
}
// ... rest of method
}Based on learnings: "Gracefully handle JavaScript errors to avoid breaking accessibility features"
🤖 Prompt for AI Agents
In `@src/frontendHighlighterApp/index.js` around lines 541 - 546, The guard
detects uninitialized this.issues but still calls descriptionOpen(id), which
will call this.issues.find() and throw; either stop calling descriptionOpen when
this.issues is not an array (i.e., remove the descriptionOpen(id) call in the if
block that sets currentButtonIndex/currentIssueStatus) or add an early-return
guard inside descriptionOpen(dataId) that checks if Array.isArray(this.issues)
and returns immediately if not; update the code around
currentButtonIndex/currentIssueStatus and descriptionOpen to use one of these
fixes to avoid the TypeError.
| it( 'does not throw when issues are not initialized', () => { | ||
| const subject = { | ||
| removeSelectedClasses: jest.fn(), | ||
| descriptionOpen: jest.fn(), | ||
| issues: null, | ||
| currentButtonIndex: null, | ||
| currentIssueStatus: null, | ||
| }; | ||
|
|
||
| expect( () => { | ||
| AccessibilityCheckerHighlight.prototype.showIssue.call( subject, 'missing' ); | ||
| } ).not.toThrow(); | ||
|
|
||
| expect( subject.currentButtonIndex ).toBe( -1 ); | ||
| expect( subject.currentIssueStatus ).toBe( 'The element was not found on the page.' ); | ||
| expect( subject.descriptionOpen ).toHaveBeenCalledWith( 'missing' ); | ||
| } ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the main implementation file that contains descriptionOpen and showIssue
fd -type f -name "*.js" | grep -E "(index|showIssue)" | head -20Repository: equalizedigital/accessibility-checker
Length of output: 251
🏁 Script executed:
# Search for the descriptionOpen method definition
rg -A 10 "descriptionOpen\s*[:=]|descriptionOpen\s*\(" --type js | head -50Repository: equalizedigital/accessibility-checker
Length of output: 3378
🏁 Script executed:
# Search for the showIssue method implementation
rg -A 20 "showIssue\s*[:=]|showIssue\s*\(" --type js | head -80Repository: equalizedigital/accessibility-checker
Length of output: 5320
🏁 Script executed:
# Check for null/undefined guards in descriptionOpen
rg -B 5 -A 15 "descriptionOpen.*\{" --type js | grep -A 15 "this.issues"Repository: equalizedigital/accessibility-checker
Length of output: 1112
Test masks a latent crash in descriptionOpen when this.issues is null.
The showIssue method correctly guards against null issues with if ( ! Array.isArray( this.issues ) ) before calling descriptionOpen. However, the real descriptionOpen implementation calls this.issues.find() without checking if this.issues is null first. Because the test mocks descriptionOpen, it never executes the crashing code, making the test pass while masking a real production bug.
Fix by either:
- Adding a null guard in
descriptionOpenbefore accessingthis.issues.find(), or - Not calling
descriptionOpenin the null-issues path withinshowIssue
🤖 Prompt for AI Agents
In `@tests/jest/frontendHighlighterApp/showIssue.test.js` around lines 48 - 64,
The test reveals a latent crash because descriptionOpen assumes this.issues is
an array; add a null/array guard inside descriptionOpen (the method named
descriptionOpen should check if Array.isArray(this.issues) before calling
this.issues.find()) and return or handle the "not found" case safely when issues
is null/undefined; alternatively, remove the call to descriptionOpen from the
null-issues branch inside showIssue, but the preferred fix is to harden
descriptionOpen by validating this.issues before using .find().
1 participant
Summary
this.issuesis uninitializedundefinedRoot cause
showIssue assumed the issue list was always initialized and that every requested id existed. In those edge cases it attempted to read issue.tooltip and issue.element from undefined, causing a runtime crash.
Evidence type
tests/jest/frontendHighlighterApp/showIssue.test.jsand confirmed full Jest + PHP test suites passRisk assessment
Environment limitations
Exit handler never called) in this runner, but lint and test commands executed successfully with the current dependency stateSummary by CodeRabbit
Bug Fixes
Tests