Skip to content

fix(url-title): enhance suggestion display logic in url-title_old.vtl#35406

Merged
nicobytes merged 1 commit intomainfrom
cf-issue
Apr 21, 2026
Merged

fix(url-title): enhance suggestion display logic in url-title_old.vtl#35406
nicobytes merged 1 commit intomainfrom
cf-issue

Conversation

@nicobytes
Copy link
Copy Markdown
Member

This pull request refactors the way the slug suggestion link is created and displayed in the url-title_old.vtl file. The changes improve the code's robustness and maintainability by replacing direct HTML string manipulation with safe DOM methods, and by updating event handling for the input field.

Refactoring and event handling improvements:

  • Replaced direct assignment of HTML strings to suggestion.innerHTML with programmatic creation and configuration of the <a> element using DOM methods, reducing the risk of XSS and improving code clarity.
  • Added a keyup event listener to the input field via JavaScript instead of using the inline onkeyup attribute, promoting separation of concerns and making the code easier to maintain.

@github-actions github-actions Bot added the Area : Backend PR changes Java/Maven backend code label Apr 21, 2026
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Apr 21, 2026

Claude finished @nicobytes's task in 53s —— View job


PR Review

One notable bug fix (possibly unintentional): The script uses type="module" (line 1), which scopes all declarations to the module — they are not accessible as globals. The old onclick="applySuggestion('${newSlug}'); return false" attribute would throw a ReferenceError at runtime because applySuggestion is not in the global scope. The new closure-based addEventListener correctly fixes this, regardless of whether it was the stated intent.

Issues worth addressing:

  1. keyup should be inputkeyup misses paste, drag-and-drop, autofill, and speech input. Since you're already adding the listener via JS, switching to the input event costs nothing and covers all cases. The old onkeyup attribute had the same gap, but that's not a reason to keep it.

    input.addEventListener('input', handleInput);
  2. Redundant suggestion.innerHTML = '' on line 26 — The early-return branch now clears innerHTML before hiding. Since suggestion.style.display = 'none' already makes it invisible and line 30 clears it before re-rendering, the line-26 clear does nothing observable. Harmless, but noise. If the intent is defensive cleanup, a comment would clarify why.

  3. No null guard on document.getElementById — Pre-existing, not introduced here, but both showSuggestion and applySuggestion will throw if the element is missing. Low risk in practice.

Everything else is clean. The XSS fix is correct (textContent vs template literal in innerHTML), the closure captures newSlug correctly per call, and the innerHTML = '' before re-rendering prevents link accumulation on rapid calls.

@nicobytes nicobytes enabled auto-merge April 21, 2026 20:31
@nicobytes nicobytes added this pull request to the merge queue Apr 21, 2026
Merged via the queue into main with commit 314cced Apr 21, 2026
49 of 50 checks passed
@nicobytes nicobytes deleted the cf-issue branch April 21, 2026 22:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI: Safe To Rollback Area : Backend PR changes Java/Maven backend code

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

3 participants