Conversation
mikelolasagasti
left a comment
There was a problem hiding this comment.
Non-blocking suggestion: activateTabbedPanels() calls activateTab(), which already updates the URL with history.replaceState. The additional history.replaceState in pjax:end is therefore redundant and could be removed. activateTabbedPanels() itself should remain.
There was a problem hiding this comment.
🟡 Changes recommended
Malformed fragments can break initialization, and PJAX fallback navigation still loses the active tab.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Persists the selected notice tab through URL hashes and PJAX pagination.
Changes:
- Initializes and updates the active tab from the URL fragment.
- Corrects PJAX binding and restores tabs after pagination.
File summaries
| File | Description |
|---|---|
app/assets/javascripts/errbit.js |
Adds hash-based tab persistence and updates PJAX handling. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if (currentTab) { | ||
| history.replaceState(history.state, '', '#' + currentTab); | ||
| } |
| return(false); | ||
| }); | ||
| activateTab($('.tab-bar ul li a.button[rel=' + currentTab + ']')); | ||
| var tab = $('.tab-bar ul li a.button[rel=' + currentTab + ']'); |
dbd1eb6 to
0ed4581
Compare
|
Thanks. Updated with suggested changes |
|
@espen can you update the commit to fix the merge conflict? |
0ed4581 to
9a3aebc
Compare
|
let me know if there is anything else to fix |
The tab links on the problem page set a fragment (#params, #backtrace, etc.) that was never read back, so opening a link with a hash or paging through notices with Older/Newer always reset the view to Summary and dropped the hash from the URL. Initialize the current tab from location.hash on load, write the hash via history.replaceState when a tab is activated, and re-append it after pjax pagination so the URL stays shareable. Unknown hashes fall back to the Summary tab. Also fix the pjax setup for the vendored jquery-pjax API: the container must be passed as a string selector ($(document).pjax(links, '#content')) instead of calling .pjax() on the container element, which threw "expected string value for 'container' option" and fell back to full page loads on every pagination click. The replaceState calls preserve history.state so pjax's back/forward handling keeps working. Co-Authored-By: Claude <noreply@anthropic.com>
9a3aebc to
0070a63
Compare
Often I want to compare URL params for a group of logged errors. This was tedious as I had to click previous error, switch tab, click previous error, switch tab. This PR will keep focus on selected tab. I am not familiar with pjax so had Claude arrange this PR for me. Let me know if there is a preferred different approach.
By Claude:
The tab links on the problem page set a fragment (#params, #backtrace, etc.) that was never read back, so opening a link with a hash or paging through notices with Older/Newer always reset the view to Summary and dropped the hash from the URL.
Initialize the current tab from location.hash on load, write the hash via history.replaceState when a tab is activated, and re-append it after pjax pagination so the URL stays shareable. Unknown hashes fall back to the Summary tab.
Also fix the pjax setup for the vendored jquery-pjax API: the container must be passed as a string selector ($(document).pjax(links, '#content')) instead of calling .pjax() on the container element, which threw "expected string value for 'container' option" and fell back to full page loads on every pagination click. The replaceState calls preserve history.state so pjax's back/forward handling keeps working.