timeslider: respect showAuthorshipColors and padFontFamily settings#7899
Merged
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
Review Summary by Qodo(Agentic_describe updated until commit 5e039a8)Respect authorship colors, font family, and line numbers in timeslider
WalkthroughsDescription• Gate authorship color CSS on .authorColors class to respect visibility setting • Bridge pad editor settings (colors, font, line numbers) to embedded timeslider iframe • Read and persist view preferences from cookie on timeslider init and change • Refactor settings-bridge wiring into unified listener management system • Add Playwright tests for authorship colors and font persistence Diagramflowchart LR
PadEditor["Pad Editor Settings"]
Cookie["Preference Cookie"]
Bridge["Settings Bridge<br/>pad_mode.ts"]
Timeslider["Timeslider iframe<br/>BroadcastSlider"]
UI["UI Elements<br/>colors/font/lines"]
PadEditor -->|"setShowAuthorColors<br/>setPadFontFamily<br/>setShowLineNumbers"| Bridge
Cookie -->|"readPadPrefs on init"| Timeslider
Bridge -->|"call methods"| Timeslider
Timeslider -->|"applyShowAuthorColors<br/>applyPadFontFamily<br/>applyShowLineNumbers"| UI
Timeslider -->|"setPadPref"| Cookie
File Changes1. src/static/js/broadcast.ts
|
Code Review by Qodo
1. Double change event firing
|
- nice-select.ts: dispatch native change event after jQuery trigger so addEventListener-based bridges (pad_mode.ts) fire (jQuery 3.7.1 trigger() does not dispatch native DOM events) - timeslider.ts: fix font-family reset (jQuery 3 ignores null css value); add showAuthorColors + showLineNumbers + padFontFamily support; wire cookie read/write for all three settings - broadcast.ts: gate author color CSS on .authorColors class - pad_mode.ts: bridge author colors, font family, line numbers checkboxes from pad settings into embedded timeslider iframe - add Playwright test for showAuthorshipColors
Refactor only — no behaviour change. pad_mode.ts: collapse the five ad-hoc listener stores (playbackChange- Listener, followChangeListener, authorColorsListeners, fontFamily- Listeners, lineNumbersListeners) into the single outerControlListeners list via a shared bindOuter() helper, so every outer-control listener is registered and torn down through one uniform path. The three view-setting bridges (author colours, font family, line numbers) become one data-driven bridgeView() over their element ids instead of three near-identical closures. timeslider.ts: hoist applyPadFontFamily to a top-level helper next to applyShowAuthorColors (keeping the '' reset for jQuery 3), and co-locate the three BroadcastSlider view-setting methods that were split across the function. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
JohnMcLear
force-pushed
the
fix/timeslider-settings
branch
from
03251f8 to
5e039a8
Compare
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Code review by qodo was updated up to the latest commit 5e039a8 |
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
+168
to
+172
| const $nativeSelect = $dropdown.prev('select'); | ||
| $nativeSelect.val($option.data('value')).trigger('change'); | ||
| // Fire native event for handlers attached via addEventListener (e.g. | ||
| // the pad_mode.ts settings bridge to the embedded timeslider iframe). | ||
| $nativeSelect[0]?.dispatchEvent(new Event('change', {bubbles: true})); |
There was a problem hiding this comment.
1. Double change event firing 🐞 Bug ≡ Correctness
The nice-select option click handler triggers both trigger('change') and a native
dispatchEvent('change'), so change handlers can run twice for one user action. This can
duplicate side effects such as pad.setMyViewOption() → pad.applyOptionsChange() when selecting
font/language/etc., and can also double-run the new addEventListener-based pad→timeslider bridge
handlers.
Agent Prompt
### Issue description
`nice-select` currently emits **two** change notifications for a single option click:
1) a jQuery `trigger('change')`
2) a native `dispatchEvent(new Event('change'))`
Because jQuery registers its `.on('change')` handlers via a native `addEventListener` (jQuery eventHandle), the native `dispatchEvent()` will also invoke jQuery handlers. The result is that jQuery handlers run once via `trigger()` and again via the native dispatch.
### Issue Context
This was added to support native `addEventListener` handlers (e.g., the new `pad_mode.ts` settings bridge), but keeping both calls causes duplicate handler execution.
### Fix Focus Areas
- src/static/js/vendors/nice-select.ts[157-173]
### Suggested fix
Update the option click handler to emit **only one** change event. The simplest fix is:
- Keep setting the value via `$nativeSelect.val(...)`
- **Remove** `.trigger('change')`
- Keep **only** the native `dispatchEvent(new Event('change', {bubbles: true}))`
This single native event will still trigger:
- native `addEventListener('change', ...)` listeners
- jQuery `.on('change', ...)` listeners (via jQuery’s native eventHandle)
- `onchange` property handlers
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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
1 participant
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
Fixes two settings that were not properly applied in the timeslider view: authorship colors visibility and font type selection.
Changes
Authorship colors (broadcast.ts)
.authorColorsclass prefix so toggling the setting in the pad editor hides authorship colors in the timeslider (matching ace2_inner.ts behaviour)Timeslider settings bridge (timeslider.ts + pad_mode.ts)
BroadcastSlider.setShowAuthorColors()andBroadcastSlider.setPadFontFamily()for parent-to-iframe dynamic propagationpad_mode.tswireSettingsBridges, with proper teardownFont family (timeslider.ts)
padFontFamilycookie at init time so preferences set in the pad editor are honoured on first paintRegression tests
Testing