fix: improve comment page #166
Conversation
…o 404 not found when going to a non existing comment
…on going over the width it should)
📝 WalkthroughWalkthroughThe pull request restructures the comments section frontend and backend. Backend controller adds null-safety checks for NPC and voice actor retrieval, plus assets for an audio player. The view reorganizes with a hero section containing audio player, new card-based comment layout, and restructured form. Comprehensive CSS refactoring introduces theme variables and component styling. JavaScript updates selectors to match the new DOM structure. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
js/comments.js (2)
100-110:⚠️ Potential issue | 🟡 MinorClient-side XSS via unsanitized string interpolation into HTML template.
Guest name (
$("#name").val()) and comment content (.val().replace(/\n/g, '<br>')) are inserted into the HTML template via.replace()without escaping. A user entering<img src=x onerror=alert(1)>in the name or comment field would get script execution when the template is prepended to the DOM.While this is "self-XSS" (only affects the poster's own session until page reload), it's still good practice to sanitize. Consider escaping HTML entities before interpolation:
Proposed helper + usage
function escapeHtml(str) { var div = document.createElement('div'); div.appendChild(document.createTextNode(str)); return div.innerHTML; }Then use it when building the comment from guest input:
- name = $("#name").val(); + name = escapeHtml($("#name").val()); ... - content = $("#content-guest").val().replace(/\n/g, '<br>'); + content = escapeHtml($("#content-guest").val()).replace(/\n/g, '<br>');
125-142:⚠️ Potential issue | 🟠 Major
deleteCommentusesevent.targetwhich may not carrydata-comment-id.If the user clicks the
×text node or a child element inside.delete-comment-button,event.targetmay not be the button itself. Line 130 correctly uses.closest('.comment-card'), but line 133 still reads$(event.target).attr('data-comment-id'), which would returnundefinedifevent.targetis a child node.Use
.closest()consistently:Proposed fix
function deleteComment(event) { if (!confirm('Do you really want to delete this comment?')) { return; } - $deletingComment = $(event.target).closest('.comment-card'); + var $btn = $(event.target).closest('.delete-comment-button'); + $deletingComment = $btn.closest('.comment-card'); $.ajax({ - url: "contents/npc/" + npcId + "/comments/" + $("form").attr('data-recording-id') + "/delete/" + $(event.target).attr('data-comment-id'), + url: "contents/npc/" + npcId + "/comments/" + $("form").attr('data-recording-id') + "/delete/" + $btn.attr('data-comment-id'), type: 'DELETE',
🤖 Fix all issues with AI agents
In `@Controllers/Website/Comments.php`:
- Around line 53-65: The code currently returns 404 when getVoiceActor() is
null, blocking comments for NPCs without an assigned actor; instead, remove the
early return and set self::$data['comments_voice_actor_id'] to a sentinel (e.g.,
0 or null) when $npc->getVoiceActor() yields null/false so the comments page can
load; locate the block around getNpc(self::$data['comments_recording']->npcId)
and the voice actor check, replace the return 404 path with assigning the
sentinel to self::$data['comments_voice_actor_id'] while preserving the normal
assignment when a User is returned.
In `@css/comments.css`:
- Around line 553-556: The .comment-avatar at the 40rem breakpoint is set to
50×50 but is still constrained by the global min-width: 60px and min-height:
60px; update the media query that sets .comment-avatar to width:50px and
height:50px to also include min-width:50px and min-height:50px (or
remove/override the larger min values) so the avatar can actually shrink; target
the .comment-avatar rule inside the 40rem media query to make this change.
- Around line 574-577: The responsive CSS selector is misspelled: replace
.comment-toggle-btn with the correct .comments-toggle-btn in the rule that
groups it with .comment-submit-btn (the rule that sets width: 100% for both);
update the selector so it reads .comments-toggle-btn, .comment-submit-btn {
width: 100%; } to match the HTML class used and ensure the style applies.
In `@Views/comments.phtml`:
- Around line 70-73: Fix the typo in the disclaimer text by replacing the
sentence "Your IP and e-mail addresses and will be stored, but will never be
publicly visible." with "Your IP and e-mail address will be stored, but will
never be publicly visible." — locate the exact string in the comments template
and update it accordingly.
🧹 Nitpick comments (2)
js/comments.js (1)
28-37: 1500 ms slide animation feels sluggish.The
slideDown(1500)andslideUp(1500)durations are 1.5 seconds each. This is noticeably slow for toggling a form and may frustrate users on repeated interactions. A value in the 300–500 ms range is more typical for slide toggles.css/comments.css (1)
3-19: Duplicated CSS variables acrosscomments.cssandnpc.css.The
:rootvariables here (lines 3–19) are identical in value to those innpc.css(lines 3–19), differing only in the--comments-vs--npc-prefix. Both files also share near-identical hero, card, back-button, and reduced-motion patterns.Consider extracting shared tokens into a common base stylesheet (e.g.,
--vow-gradient-purple,--vow-card-shadow, etc.) and referencing them from both files. This would reduce the maintenance burden when theme values change.
| $npc = $cnm->getNpc(self::$data['comments_recording']->npcId); | ||
| if ($npc === null || $npc === false) { | ||
| //NPC associated with this recording was not found | ||
| return 404; | ||
| } | ||
|
|
||
| $voiceActor = $npc->getVoiceActor(); | ||
| if ($voiceActor === null || $voiceActor === false) { | ||
| //Voice actor for this NPC was not found | ||
| return 404; | ||
| } | ||
|
|
||
| self::$data['comments_voice_actor_id'] = $voiceActor->getId(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for voice_actor_id usage to understand downstream impact
rg -n 'voice_actor_id' -C 3Repository: Team-VoW/VoicesOfWynn-Website
Length of output: 6013
🏁 Script executed:
# Check the NPC class to understand getVoiceActor behavior and return types
rg -n 'getVoiceActor|class Npc' --type=php -A 10Repository: Team-VoW/VoicesOfWynn-Website
Length of output: 10165
🏁 Script executed:
# Look for how comments view uses the voice actor ID
fd -e 'php' -e 'js' -e 'html' | xargs rg -l 'comments_voice_actor_id'Repository: Team-VoW/VoicesOfWynn-Website
Length of output: 105
Returning 404 when voice actor is missing prevents viewing comments on unassigned NPCs.
NPCs can legitimately have no voice actor assigned—the database schema explicitly allows NULL values for voice_actor_id with ON DELETE SET NULL, and the NPC page itself already handles this case (showing "Nobody" in the cast list). The getVoiceActor() method returns ?User, confirming it can be null.
Returning 404 on line 62 blocks users from viewing or posting comments on recordings for NPCs without an assigned voice actor. Instead, use a sentinel value so the comments page loads:
Proposed fix
$voiceActor = $npc->getVoiceActor();
- if ($voiceActor === null || $voiceActor === false) {
- //Voice actor for this NPC was not found
- return 404;
- }
-
- self::$data['comments_voice_actor_id'] = $voiceActor->getId();
+ self::$data['comments_voice_actor_id'] = ($voiceActor !== null && $voiceActor !== false)
+ ? $voiceActor->getId()
+ : 0;The author badge comparison in the view will still work correctly—passing 0 or null will simply prevent the badge from displaying when there is no voice actor.
🤖 Prompt for AI Agents
In `@Controllers/Website/Comments.php` around lines 53 - 65, The code currently
returns 404 when getVoiceActor() is null, blocking comments for NPCs without an
assigned actor; instead, remove the early return and set
self::$data['comments_voice_actor_id'] to a sentinel (e.g., 0 or null) when
$npc->getVoiceActor() yields null/false so the comments page can load; locate
the block around getNpc(self::$data['comments_recording']->npcId) and the voice
actor check, replace the return 404 path with assigning the sentinel to
self::$data['comments_voice_actor_id'] while preserving the normal assignment
when a User is returned.
There was a problem hiding this comment.
Agreed, @kmaxii remove the 404 return for no-voice-actor scenario.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
ShadyMedic
left a comment
ShadyMedic
left a comment
There was a problem hiding this comment.
Fix those small things and it's good to go.
| $npc = $cnm->getNpc(self::$data['comments_recording']->npcId); | ||
| if ($npc === null || $npc === false) { | ||
| //NPC associated with this recording was not found | ||
| return 404; | ||
| } | ||
|
|
||
| $voiceActor = $npc->getVoiceActor(); | ||
| if ($voiceActor === null || $voiceActor === false) { | ||
| //Voice actor for this NPC was not found | ||
| return 404; | ||
| } | ||
|
|
||
| self::$data['comments_voice_actor_id'] = $voiceActor->getId(); |
There was a problem hiding this comment.
Agreed, @kmaxii remove the 404 return for no-voice-actor scenario.
2 participants
Summary by CodeRabbit
New Features
Improvements