fix: tabs not being selected with fragments that contain unescaped html special chars #214
Conversation
WalkthroughThe pull request introduces a new static method Changes
Sequence DiagramsequenceDiagram
participant URL as URL Hash
participant Util as Util.selectElementFromUrlHash()
participant Tab as Tab Header Element
URL->>Util: Provide URL hash
Util-->>Util: Decode and escape hash
Util-->>Util: Construct tab header ID
Util->>Tab: Find corresponding tab element
Util-->>URL: Return tab header or undefined
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
modules/ext.tabberNeue/Util.js (2)
81-83: Add input type validation.The input validation could be more robust by checking the type of
urlHash.- if( !urlHash ) { + if( !urlHash || typeof urlHash !== 'string' ) { return; }🧰 Tools
🪛 eslint
[error] 81-81: Expected space(s) after "if".
(keyword-spacing)
89-94: Consider adding error logging for debugging.When the tab element is not found on the first attempt, it would be helpful to log this for debugging purposes.
if ( !activeTabFromUrlHash ) { + mw.log.debug( '[TabberNeue] Tab not found with ID:', idFromUrlHash ); // Retry getting the tab after escaping html special chars to correctly select // the tab for cases where the fragment does not use the escaped version const specialCharEscapedHash = mw.html.escape( idFromUrlHash ); activeTabFromUrlHash = document.getElementById( specialCharEscapedHash );modules/ext.tabberNeue/ext.tabberNeue.js (1)
Line range hint
485-490: Consider adding error logging for debugging.When a tab is not found for a hash change, it would be helpful to log this for debugging purposes.
const tab = Util.selectElementFromUrlHash( hash ); if ( tab ) { event.preventDefault(); tab.click(); + } else { + mw.log.debug( '[TabberNeue] No matching tab found for hash:', hash ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
modules/ext.tabberNeue/Util.js(1 hunks)modules/ext.tabberNeue/ext.tabberNeue.js(2 hunks)
🧰 Additional context used
📓 Learnings (1)
modules/ext.tabberNeue/ext.tabberNeue.js (1)
Learnt from: alistair3149
PR: StarCitizenTools/mediawiki-extensions-TabberNeue#189
File: modules/ext.tabberNeue/ext.tabberNeue.js:427-430
Timestamp: 2024-11-16T08:18:43.156Z
Learning: In `modules/ext.tabberNeue/ext.tabberNeue.js`, within the `getActiveTab` method of the `TabberBuilder` class, the check `if (idFromUrlHash === escapedHash)` ensures that if replacing the `'tabber-tabpanel-'` prefix does not modify the `escapedHash`, then the hash does not represent a valid tabpanel ID, and the current check is sufficient.
🪛 eslint
modules/ext.tabberNeue/Util.js
[error] 81-81: Expected space(s) after "if".
(keyword-spacing)
[error] 95-95: Expected space(s) after "if".
(keyword-spacing)
[error] 101-101: Expected space(s) after "if".
(keyword-spacing)
🔇 Additional comments (1)
modules/ext.tabberNeue/ext.tabberNeue.js (1)
Line range hint
429-436: LGTM! Clean refactor of the tab selection logic.The code has been simplified by moving the URL hash handling to the utility function while maintaining the necessary validation checks. The check for
closest('.tabber__tabs')ensures that the selected tab belongs to the current tabber instance.
modules/ext.tabberNeue/Util.js
Outdated
| * @return {Element} The element of the matching tab header. | ||
| */ | ||
| static selectElementFromUrlHash( urlHash ) { | ||
| if( !urlHash ) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix spacing after if statements.
Add space after if keywords to comply with the coding style.
- if( !urlHash ) {
+ if ( !urlHash ) {
- if( !activeTabFromUrlHash ) {
+ if ( !activeTabFromUrlHash ) {
- if( activeTabFromUrlHash.classList.contains( 'tabber__tab' ) ) {
+ if ( activeTabFromUrlHash.classList.contains( 'tabber__tab' ) ) {Also applies to: 95-95, 101-101
🧰 Tools
🪛 eslint
[error] 81-81: Expected space(s) after "if".
(keyword-spacing)
#209 fix once again and should really be the last one.
Fragments containing unescaped special chars like
'instead of'are not selected. I just added a retry when selecting the tab element with the escaped version if the first try failed. Checking whether its already escaped would be much more complex so this should be good enough.I've also moved the fragment transformation and element selection into a util function and added a check to make sure the selected element is generated by tabber (the previous code, at least in the hashchange event part, would trigger for any hashchange and could trigger the click event on any element).
Summary by CodeRabbit
New Features
Refactor