Conversation
📝 WalkthroughWalkthroughThe pull request adds a view-limiting modal feature to the recently viewed items tracking system. It increases the storage cap from 10 to 20 items, introduces a conditional modal trigger based on item count and course URL pattern matching, and includes a new modal template for display to users. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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
🤖 Fix all issues with AI agents
In `@tcf_website/static/common/recently_viewed.js`:
- Around line 81-88: Remove the stray blank line/whitespace above the if-block
and change single quotes to double quotes per ESLint: ensure the block using
previousPaths, courseRegex, currentUrl and the modal call uses
$("#viewLimitModal").modal("show"); (i.e., use double quotes for the selector
and the "show" argument) while keeping the courseRegex and logic unchanged.
- Around line 70-79: previousPaths and previousPathsTitles are getting out of
sync because only titles are sliced to 10; remove the premature 10-item slice
and keep storage logic limited to 20 items (slice both previousPaths and
previousPathsTitles to 20) and instead derive the 10-item display list at render
time; specifically, revert or remove the block that does previousPathsTitles =
previousPathsTitles.slice(0, 10) and ensure any rendering code uses
previousPaths.slice(0, 10) and previousPathsTitles.slice(0, 10) together so
indexes remain aligned.
In `@tcf_website/templates/common/view_limit_modal.html`:
- Line 10: Fix the typo in the modal paragraph text: locate the paragraph
element with class "mb-4" in view_limit_modal.html (the line containing '<p
class="mb-4">Consider making a review to help others students like you.</p>')
and change the copy from "others students" to "other students" so the sentence
reads "Consider making a review to help other students like you."
- Around line 3-4: The modal has aria-labelledby="viewLimitModalLabel" but no
element with id "viewLimitModalLabel" and it unnaturally prevents dismissal via
data-backdrop="static" and data-keyboard="false"; fix by either adding
id="viewLimitModalLabel" to the modal title element (the <h5> used for the modal
heading) so the aria-labelledby points to a real element, and remove the
heavy-handed attributes data-backdrop and data-keyboard from the <div
id="viewLimitModal"> so users can dismiss the non-critical prompt with outside
click or Escape, or alternatively remove the aria-labelledby attribute if you
prefer not to add the id—ensure the change keeps accessibility intact by having
a valid label reference or using aria-label.
🧹 Nitpick comments (1)
tcf_website/static/common/recently_viewed.js (1)
82-88: Modal re-shows every time the count hits 10, 15, or 20 — consider a cooldown or session flag.Because
previousPaths.lengthis checked on every page load, a user who has exactly 10 stored paths will see the modal each time they revisit a qualifying course page without adding a new unique course (the count stays at 10 after dedup). More importantly, there's no mechanism to avoid annoying repeat prompts across sessions. Consider setting alocalStorageflag (e.g.,lastModalShownCount) to only trigger once per threshold crossing.
| // Only display last 10 items | ||
| if (previousPaths.length > 10) { | ||
| previousPaths = previousPaths.slice(0, 10); | ||
| // previousPaths = previousPaths.slice(0, 10); | ||
| previousPathsTitles = previousPathsTitles.slice(0, 10); | ||
| } | ||
| // Prevent too many courses from being stored, max 20 | ||
| if (previousPaths.length > 20) { | ||
| previousPaths = previousPaths.slice(0, 20); | ||
| previousPathsTitles = previousPathsTitles.slice(0, 20); | ||
| } |
There was a problem hiding this comment.
Bug: previousPaths and previousPathsTitles go out of sync.
Line 72 comments out the slice for previousPaths while Line 73 still slices previousPathsTitles to 10. These two arrays are parallel (same index = same item). Once previousPaths has more than 10 entries, titles will have fewer entries than paths, breaking the index correspondence. Any code that iterates both arrays together (e.g., the history/recently-viewed display) will pair wrong titles with wrong URLs or have missing titles.
If the intent is to keep 20 paths for the modal counter but only display 10 in the UI, derive the display list at read time rather than corrupting the stored data.
Proposed fix — keep both arrays in sync, derive display count at read time
- // Only display last 10 items
- if (previousPaths.length > 10) {
- // previousPaths = previousPaths.slice(0, 10);
- previousPathsTitles = previousPathsTitles.slice(0, 10);
- }
- // Prevent too many courses from being stored, max 20
- if (previousPaths.length > 20) {
- previousPaths = previousPaths.slice(0, 20);
- previousPathsTitles = previousPathsTitles.slice(0, 20);
- }
+ // Prevent too many courses from being stored, max 20
+ if (previousPaths.length > 20) {
+ previousPaths = previousPaths.slice(0, 20);
+ previousPathsTitles = previousPathsTitles.slice(0, 20);
+ }Then wherever the recently-viewed list is displayed, limit to the first 10 entries at read time.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Only display last 10 items | |
| if (previousPaths.length > 10) { | |
| previousPaths = previousPaths.slice(0, 10); | |
| // previousPaths = previousPaths.slice(0, 10); | |
| previousPathsTitles = previousPathsTitles.slice(0, 10); | |
| } | |
| // Prevent too many courses from being stored, max 20 | |
| if (previousPaths.length > 20) { | |
| previousPaths = previousPaths.slice(0, 20); | |
| previousPathsTitles = previousPathsTitles.slice(0, 20); | |
| } | |
| // Prevent too many courses from being stored, max 20 | |
| if (previousPaths.length > 20) { | |
| previousPaths = previousPaths.slice(0, 20); | |
| previousPathsTitles = previousPathsTitles.slice(0, 20); | |
| } |
🤖 Prompt for AI Agents
In `@tcf_website/static/common/recently_viewed.js` around lines 70 - 79,
previousPaths and previousPathsTitles are getting out of sync because only
titles are sliced to 10; remove the premature 10-item slice and keep storage
logic limited to 20 items (slice both previousPaths and previousPathsTitles to
20) and instead derive the 10-item display list at render time; specifically,
revert or remove the block that does previousPathsTitles =
previousPathsTitles.slice(0, 10) and ensure any rendering code uses
previousPaths.slice(0, 10) and previousPathsTitles.slice(0, 10) together so
indexes remain aligned.
|
|
||
| if (previousPaths.length % 5 === 0 && previousPaths.length >= 10) { | ||
| // Check if URL matches .../course/[letters]/[numbers] and ends there | ||
| const courseRegex = /course\/[a-zA-Z]+\/\d+\/?$/; | ||
| if (courseRegex.test(currentUrl)) { | ||
| $('#viewLimitModal').modal('show'); | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix lint/formatting failures flagged by CI.
Line 81 has an extra blank line with whitespace (prettier failure), and Line 86 should use double quotes per ESLint config.
Proposed fix
-
-
- if (previousPaths.length % 5 === 0 && previousPaths.length >= 10) {
+ if (previousPaths.length % 5 === 0 && previousPaths.length >= 10) {
// Check if URL matches .../course/[letters]/[numbers] and ends there
const courseRegex = /course\/[a-zA-Z]+\/\d+\/?$/;
if (courseRegex.test(currentUrl)) {
- $('#viewLimitModal').modal('show');
+ $("#viewLimitModal").modal("show");
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (previousPaths.length % 5 === 0 && previousPaths.length >= 10) { | |
| // Check if URL matches .../course/[letters]/[numbers] and ends there | |
| const courseRegex = /course\/[a-zA-Z]+\/\d+\/?$/; | |
| if (courseRegex.test(currentUrl)) { | |
| $('#viewLimitModal').modal('show'); | |
| } | |
| } | |
| if (previousPaths.length % 5 === 0 && previousPaths.length >= 10) { | |
| // Check if URL matches .../course/[letters]/[numbers] and ends there | |
| const courseRegex = /course\/[a-zA-Z]+\/\d+\/?$/; | |
| if (courseRegex.test(currentUrl)) { | |
| $("#viewLimitModal").modal("show"); | |
| } | |
| } |
🧰 Tools
🪛 GitHub Actions: Continuous Integration
[error] 81-81: prettier/prettier: Delete ⏎·· (formatting issue).
🪛 GitHub Check: eslint
[failure] 86-86:
Replace '#viewLimitModal').modal('show' with "#viewLimitModal").modal("show"
[failure] 81-81:
Delete ⏎··
🤖 Prompt for AI Agents
In `@tcf_website/static/common/recently_viewed.js` around lines 81 - 88, Remove
the stray blank line/whitespace above the if-block and change single quotes to
double quotes per ESLint: ensure the block using previousPaths, courseRegex,
currentUrl and the modal call uses $("#viewLimitModal").modal("show"); (i.e.,
use double quotes for the selector and the "show" argument) while keeping the
courseRegex and logic unchanged.
| <div class="modal fade" id="viewLimitModal" tabindex="-1" role="dialog" aria-labelledby="viewLimitModalLabel" | ||
| aria-hidden="true" data-backdrop="static" data-keyboard="false"> |
There was a problem hiding this comment.
aria-labelledby="viewLimitModalLabel" references a nonexistent element.
No element in this template has id="viewLimitModalLabel". Either add the id to the <h5> on Line 9 or remove the attribute.
Also, data-backdrop="static" and data-keyboard="false" feel heavy-handed for a non-critical promotional prompt — users can't dismiss by clicking outside or pressing Escape. Consider removing these attributes for a friendlier UX.
Proposed fix
-<div class="modal fade" id="viewLimitModal" tabindex="-1" role="dialog" aria-labelledby="viewLimitModalLabel"
- aria-hidden="true" data-backdrop="static" data-keyboard="false">
+<div class="modal fade" id="viewLimitModal" tabindex="-1" role="dialog" aria-labelledby="viewLimitModalLabel"
+ aria-hidden="true">
<div class="modal-dialog modal-dialog-centered" role="document">
<div class="modal-content">
<div class="modal-body text-center pt-4 pb-4">
<img src="{% static 'base/img/new_logo.svg' %}" alt="theCourseForum" class="img-fluid mb-3" style="max-height: 60px;">
- <h5 class="mb-3">You've viewed a lot of courses!</h5>
+ <h5 id="viewLimitModalLabel" class="mb-3">You've viewed a lot of courses!</h5>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <div class="modal fade" id="viewLimitModal" tabindex="-1" role="dialog" aria-labelledby="viewLimitModalLabel" | |
| aria-hidden="true" data-backdrop="static" data-keyboard="false"> | |
| <div class="modal fade" id="viewLimitModal" tabindex="-1" role="dialog" aria-labelledby="viewLimitModalLabel" | |
| aria-hidden="true"> | |
| <div class="modal-dialog modal-dialog-centered" role="document"> | |
| <div class="modal-content"> | |
| <div class="modal-body text-center pt-4 pb-4"> | |
| <img src="{% static 'base/img/new_logo.svg' %}" alt="theCourseForum" class="img-fluid mb-3" style="max-height: 60px;"> | |
| <h5 id="viewLimitModalLabel" class="mb-3">You've viewed a lot of courses!</h5> |
🤖 Prompt for AI Agents
In `@tcf_website/templates/common/view_limit_modal.html` around lines 3 - 4, The
modal has aria-labelledby="viewLimitModalLabel" but no element with id
"viewLimitModalLabel" and it unnaturally prevents dismissal via
data-backdrop="static" and data-keyboard="false"; fix by either adding
id="viewLimitModalLabel" to the modal title element (the <h5> used for the modal
heading) so the aria-labelledby points to a real element, and remove the
heavy-handed attributes data-backdrop and data-keyboard from the <div
id="viewLimitModal"> so users can dismiss the non-critical prompt with outside
click or Escape, or alternatively remove the aria-labelledby attribute if you
prefer not to add the id—ensure the change keeps accessibility intact by having
a valid label reference or using aria-label.
| <div class="modal-body text-center pt-4 pb-4"> | ||
| <img src="{% static 'base/img/new_logo.svg' %}" alt="theCourseForum" class="img-fluid mb-3" style="max-height: 60px;"> | ||
| <h5 class="mb-3">You've viewed a lot of courses!</h5> | ||
| <p class="mb-4">Consider making a review to help others students like you.</p> |
There was a problem hiding this comment.
Typo: "others students" → "other students".
Proposed fix
- <p class="mb-4">Consider making a review to help others students like you.</p>
+ <p class="mb-4">Consider making a review to help other students like you.</p>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <p class="mb-4">Consider making a review to help others students like you.</p> | |
| <p class="mb-4">Consider making a review to help other students like you.</p> |
🤖 Prompt for AI Agents
In `@tcf_website/templates/common/view_limit_modal.html` at line 10, Fix the typo
in the modal paragraph text: locate the paragraph element with class "mb-4" in
view_limit_modal.html (the line containing '<p class="mb-4">Consider making a
review to help others students like you.</p>') and change the copy from "others
students" to "other students" so the sentence reads "Consider making a review to
help other students like you."
2 participants
What I did
Screenshots
Summary by CodeRabbit
New Features
Improvements