Add README viewer modal #162
Conversation
![sentry[bot]](https://avatars.githubusercontent.com/u/1396951?s=60&v=4){kind=small}
| text: "Readme", | ||
| href: project.readme_url, | ||
| href: helpers.readme_project_path(project), |
There was a problem hiding this comment.
The helper method helpers.readme_project_path(project) is being called but this method is not defined in the codebase (verified by ripgrep search). This will cause a NoMethodError at runtime. You need to either: (1) define this helper method in a helper file, or (2) use the standard Rails path helper directly. Since you're using member do get :readme end in routes, the correct helper should be readme_project_path(project).
Severity: CRITICAL
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: app/components/project_show_card_component.html.erb#L92-L93
Potential issue: The helper method `helpers.readme_project_path(project)` is being
called but this method is not defined in the codebase (verified by ripgrep search). This
will cause a NoMethodError at runtime. You need to either: (1) define this helper method
in a helper file, or (2) use the standard Rails path helper directly. Since you're using
`member do get :readme end` in routes, the correct helper should be
`readme_project_path(project)`.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 3637112
| <%= turbo_frame_tag "readme_modal" do %> | ||
| <div class="modal-backdrop" | ||
| data-controller="modal" | ||
| data-action="click->modal#closeBackground keyup@window->modal#closeWithEsc"> | ||
| <div class="modal-content"> | ||
| <div class="modal-header"> | ||
| <h2>README</h2> | ||
| <button type="button" class="modal-close" data-action="modal#close" aria-label="Close modal"> | ||
| <%= inline_svg_tag "icons/close.svg" %> | ||
| </button> | ||
| </div> | ||
| <div class="modal-body"> | ||
| <%= sanitize(@html) %> | ||
| </div> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
The modal uses data-action="click->modal#closeBackground keyup@window->modal#closeWithEsc" for keyboard and click-outside handling. However, there's no focus trap or focus restoration logic. When the modal opens, focus should move to the modal (or close button), and when it closes, focus should return to the triggering element. This improves accessibility for keyboard and screen reader users. Consider adding autofocus to the close button or using a focus management library.
Severity: MEDIUM
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: app/views/projects/readme.html.erb#L1-L16
Potential issue: The modal uses `data-action="click->modal#closeBackground
keyup@window->modal#closeWithEsc"` for keyboard and click-outside handling. However,
there's no focus trap or focus restoration logic. When the modal opens, focus should
move to the modal (or close button), and when it closes, focus should return to the
triggering element. This improves accessibility for keyboard and screen reader users.
Consider adding `autofocus` to the close button or using a focus management library.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 3637112
| if (e) e.preventDefault(); | ||
| this.element.remove(); |
There was a problem hiding this comment.
The close method at line 14 calls this.element.remove() without checking if the element exists or is already removed. If called multiple times or if the element is removed externally, this could cause an error. Additionally, if the Turbo frame replacement is still in progress, removing the element synchronously could cause issues. Consider using a safer approach: this.element?.remove?.() or wrapping in a try-catch.
Severity: MEDIUM
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: app/javascript/controllers/modal_controller.js#L13-L14
Potential issue: The `close` method at line 14 calls `this.element.remove()` without
checking if the element exists or is already removed. If called multiple times or if the
element is removed externally, this could cause an error. Additionally, if the Turbo
frame replacement is still in progress, removing the element synchronously could cause
issues. Consider using a safer approach: `this.element?.remove?.()` or wrapping in a
try-catch.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 3637112
| disconnect() { | ||
| document.body.style.overflow = ""; |
There was a problem hiding this comment.
The disconnect method restores document.body.style.overflow by setting it to an empty string. However, if multiple modals are nested or opened in quick succession, this could prematurely unlock scrolling before all modals are closed. Consider using a counter or stack to track open modals. Alternatively, use a class-based approach: add/remove a class from document.body and manage overflow via CSS.
Severity: MEDIUM
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: app/javascript/controllers/modal_controller.js#L8-L9
Potential issue: The `disconnect` method restores `document.body.style.overflow` by
setting it to an empty string. However, if multiple modals are nested or opened in quick
succession, this could prematurely unlock scrolling before all modals are closed.
Consider using a counter or stack to track open modals. Alternatively, use a class-based
approach: add/remove a class from `document.body` and manage overflow via CSS.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 3637112
| content, | ||
| options: { | ||
| parse: { smart: true }, | ||
| render: { unsafe: false } |
There was a problem hiding this comment.
Ah, I'm quite concerned about the XSS possibilities
There was a problem hiding this comment.
I could extend it, any suggestions?
deoshreyas
force-pushed
the
add-readme-modal
branch
from
44e61f2 to
22560e2
Compare
3 participants
Adds a modal to view README files from the project page. Some sanitization for unsafe HTML (can extend if needed).