Skip to content

Make review popup #1227

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
jackrhoa wants to merge 2 commits into
base: dev
Choose a base branch
from
+34 −2
Draft

Make review popup #1227

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
58f6c4e
modify logic
jackrhoa Feb 13, 2026
afe201b
improve handling
jackrhoa Feb 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 16 additions & 2 deletions tcf_website/static/common/recently_viewed.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,11 +67,25 @@
previousPaths.unshift(currentUrl);
previousPathsTitles.unshift(title);

// Keep only the top 10 items
// 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);
}
Comment on lines +70 to +79
Copy link
Contributor

@coderabbitai coderabbitai bot Feb 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

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.

Suggested change
// 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.



Check failure on line 81 in tcf_website/static/common/recently_viewed.js

View workflow job for this annotation

GitHub Actions / eslint 

Delete `⏎··`
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');

Check failure on line 86 in tcf_website/static/common/recently_viewed.js

View workflow job for this annotation

GitHub Actions / eslint 

Replace `'#viewLimitModal').modal('show'` with `"#viewLimitModal").modal("show"`
}
}
Comment on lines +81 to +88
Copy link
Contributor

@coderabbitai coderabbitai bot Feb 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

Suggested change
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.


// Save back to localStorage
localStorage.setItem("previous_paths", JSON.stringify(previousPaths));
Expand Down
3 changes: 3 additions & 0 deletions tcf_website/templates/base/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,9 @@
<!-- Adblock modal -->
{% include "common/adblock_modal.html" %}

<!-- View limit modal -->
{% include "common/view_limit_modal.html" %}

<!-- Google Tag Manager (noscript) -->
<noscript><iframe src="https://www.googletagmanager.com/ns.html?id=GTM-54T5TXX"
height="0" width="0" style="display:none;visibility:hidden"></iframe></noscript>
Expand Down
15 changes: 15 additions & 0 deletions tcf_website/templates/common/view_limit_modal.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
<!-- View limit modal (imported in index.html) -->
{% load static %}
<div class="modal fade" id="viewLimitModal" tabindex="-1" role="dialog" aria-labelledby="viewLimitModalLabel"
aria-hidden="true" data-backdrop="static" data-keyboard="false">
Comment on lines +3 to +4
Copy link
Contributor

@coderabbitai coderabbitai bot Feb 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

Suggested change
<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-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>
<p class="mb-4">Consider making a review to help others students like you.</p>
Copy link
Contributor

@coderabbitai coderabbitai bot Feb 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

Suggested change
<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."

<button type="button" class="btn btn-primary bg-tcf-indigo pl-4 pr-4" data-dismiss="modal">Close</button>
</div>
</div>
</div>
</div>
Loading