Closed
Conversation
📝 WalkthroughWalkthroughThis PR introduces a comprehensive bookmark feature for a Flask blog application. It adds a new Changes
Sequence DiagramsequenceDiagram
participant Client as Client (Browser)
participant Server as Server (Flask)
participant DB as Database
Client->>Client: User clicks bookmark button
Client->>Server: POST /api/bookmark/post_id<br/>(CSRF token, session)
Server->>Server: Verify authentication<br/>from session
Server->>DB: Query existing Bookmark<br/>record
DB-->>Server: Bookmark found or not found
alt Bookmark exists
Server->>DB: DELETE Bookmark record
DB-->>Server: Deletion confirmed
else Bookmark not exists
Server->>DB: INSERT new Bookmark<br/>record
DB-->>Server: Insertion confirmed
end
Server-->>Client: Response with<br/>bookmarked: true/false
Client->>Client: Update button UI<br/>(style, icon, title)
Note over Client: Bookmark state reflected
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 🧪 Unit Test Generation v2 is now available!We have significantly improved our unit test generation capabilities. To enable: Add this to your reviews:
finishing_touches:
unit_tests:
enabled: trueTry it out by using the Have feedback? Share your thoughts on our Discord thread! 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 |
Hifumi12135
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@app/models.py`:
- Around line 80-83: The user_id and post_id foreign key columns are currently
nullable which breaks the uniqueness intent; update the bookmark model's user_id
and post_id Column definitions to include nullable=False (i.e., set
db.Column(..., db.ForeignKey("users.user_id", ondelete="CASCADE"),
nullable=False) and similarly for post_id) so NULLs cannot be inserted and any
existing UniqueConstraint on this model will behave correctly; locate the
columns named user_id and post_id in the bookmark model in app/models.py and add
nullable=False to each db.Column call.
In `@app/routes/bookmark.py`:
- Around line 86-134: The /my-bookmarks view (function my_bookmarks on the
bookmark_blueprint) conflicts with the existing HTML route and returns JSON
containing Post.banner (LargeBinary) which isn't JSON-safe; rename the endpoint
(e.g., "/api/my-bookmarks" or "/bookmarks/json") to avoid the collision with
app/routes/my_bookmarks.py and update any clients, and change the response for
Post.banner so it is not raw LargeBinary — either omit banner from the
bookmarked_posts payload or convert post.banner to a JSON-safe string (e.g.,
base64-encode) before adding it to the dict; keep the rest of the logic and
error handling the same.
In `@app/scripts/migrate_bookmarks.py`:
- Line 14: The file imports a non-existent factory create_app; either add a
create_app() factory to app.py or change the import/usage in
migrate_bookmarks.py to use the exported Flask instance (e.g., import the app
object from app) and update any call sites that call create_app() (see the
invocation around line 20) to use the imported app instance instead; reference
symbols: create_app (remove or implement) and app (use the Flask app object) so
the module imports and invocation match what app.py actually exports.
In `@app/scripts/test_bookmark_api.py`:
- Around line 12-70: The test_bookmark_functionality currently prints failures
but still returns True; update each failure branch in functions like
test_bookmark_functionality (including server connection, unauthenticated POST
to /api/bookmark/1, GET /api/bookmark/status/1 checks, and /my-bookmarks
redirect check) to immediately fail the test by either returning False or
raising an exception (choose one consistent behavior) instead of merely
printing; ensure the success path only returns True at the end when no failure
branch was taken, and keep the error output (response.status_code,
response.text, exception message) in the failure return/exception to aid
debugging.
In `@app/scripts/test_bookmarks.py`:
- Line 14: The test imports a nonexistent factory function create_app; instead
import the module-level Flask instance named app (the Flask variable defined in
app.py) and use that in the tests—replace the create_app import with importing
app (e.g., from app import app as flask_app) and update any references to
create_app() to use flask_app (or flask_app.test_client()) so the tests use the
existing Flask application instance.
In `@app/static/js/bookmark.js`:
- Around line 2-46: The code uses a global csrfToken that isn't defined on
non-post pages and causes a ReferenceError; in app/static/js/bookmark.js, guard
against an undefined csrfToken by defining a local safe token (e.g., const
safeCsrf = typeof csrfToken !== 'undefined' ? csrfToken : '') at the top of the
file and use safeCsrf in the fetch headers inside toggleBookmark and
checkBookmarkStatus (replace 'X-CSRFToken': csrfToken || '' with the guarded
variable) so bookmark.js can be loaded globally without throwing.
In `@app/templates/components/post_card_macro.html`:
- Around line 12-28: Replace direct session indexing with
session.get('username') in the conditional that renders the bookmark button to
avoid KeyError (update the check around the bookmark block that currently uses
session['username']), and add an accessible label to the icon-only button (the
element with class "bookmark-btn" and onclick="toggleBookmark") by including an
aria-label (and keep the existing title) so screen readers can announce its
purpose; keep the existing toggleBookmark(postId, this) call and data-post-id
attribute unchanged.
🧹 Nitpick comments (6)
app/scripts/test_bookmarks.py (1)
36-62: Test may fail on duplicate unique constraint if run multiple times.If a bookmark already exists for the first user/post combination (e.g., from a previous interrupted run), the test will fail due to the unique constraint
_user_post_uc. Consider checking for an existing bookmark first or using a more isolated test approach.♻️ Suggested improvement
# Create a test bookmark + # Check if bookmark already exists + existing = Bookmark.query.filter_by( + user_id=user.user_id, + post_id=post.id + ).first() + if existing: + Log.info("Test bookmark already exists, cleaning up first") + db.session.delete(existing) + db.session.commit() + bookmark = Bookmark( user_id=user.user_id, post_id=post.idapp/scripts/migrate_bookmarks.py (1)
21-24: Migration may be redundant with application startup.Looking at
app/database.py(lines 10-21),init_db()already callsdb.create_all()during application startup, which creates the bookmarks table automatically when theBookmarkmodel is registered. This script may only be needed for databases created before this model was added.Also, the comment on line 22 is slightly misleading -
db.create_all()creates all missing tables defined in models, not just the bookmarks table.Consider updating the comment for clarity:
- # Create all tables (this will only create the new bookmarks table) + # Create all tables (creates any missing tables defined in models)app/templates/my_bookmarks.html (2)
36-39: Move macro import outside the loop for efficiency.The
post_cardmacro import is inside the{% for %}loop, causing it to be re-imported on each iteration. Move it before the loop for better performance.♻️ Proposed fix
{% if bookmarked_posts %} + {% from "components/post_card_macro.html" import post_card with context %} <div class="grid grid-cols-1 sm:grid-cols-2 lg:grid-cols-3 xl:grid-cols-4 gap-6"> {% for post in bookmarked_posts %} - {% from "components/post_card_macro.html" import post_card with context %} {{ post_card(post=post, author_profile_picture=get_profile_picture(post[5])) }} {% endfor %} </div>
38-38: Magic indexpost[5]is fragile and unclear.The
post[5]relies on the tuple ordering from the route handler. If the data structure inapp/routes/my_bookmarks.pychanges, this will silently break. Consider using a dictionary or named tuple in the route to make field access explicit (e.g.,post['author']orpost.author).app/scripts/test_bookmark_api.py (1)
18-66: Add request timeouts to avoid hanging runs.
These calls can block indefinitely if the server is down or slow.✅ Suggested fix
- response = requests.get(BASE_URL) + response = requests.get(BASE_URL, timeout=5) @@ - response = requests.post(f"{BASE_URL}/api/bookmark/1") + response = requests.post(f"{BASE_URL}/api/bookmark/1", timeout=5) @@ - response = requests.get(f"{BASE_URL}/api/bookmark/status/1") + response = requests.get(f"{BASE_URL}/api/bookmark/status/1", timeout=5) @@ - response = requests.get(f"{BASE_URL}/my-bookmarks", allow_redirects=False) + response = requests.get(f"{BASE_URL}/my-bookmarks", allow_redirects=False, timeout=5)app/routes/my_bookmarks.py (1)
27-52: Avoid loading large Post blobs you don’t render.
post.bannerand fullpost.contentare loaded for every bookmark but aren’t needed for the card view; this can be costly.✅ Suggested fix (example)
+from sqlalchemy.orm import load_only @@ - bookmarks = db.session.query(Bookmark, Post).join( + bookmarks = db.session.query(Bookmark, Post).options( + load_only( + Post.id, Post.title, Post.tags, Post.author, + Post.views, Post.time_stamp, Post.last_edit_time_stamp, + Post.category, Post.url_id, Post.abstract + ) + ).join( Post, Bookmark.post_id == Post.id ).filter( Bookmark.user_id == user.user_id ).order_by( Bookmark.time_stamp.desc() ).all()
Comment on lines
+80
to
+83
| id = db.Column(db.Integer, primary_key=True, autoincrement=True) | ||
| user_id = db.Column(db.Integer, db.ForeignKey("users.user_id", ondelete="CASCADE")) | ||
| post_id = db.Column(db.Integer, db.ForeignKey("posts.id", ondelete="CASCADE")) | ||
| time_stamp = db.Column(db.Integer, default=current_time_stamp) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd / && find . -name "models.py" -type f 2>/dev/null | head -20Repository: DogukanUrker/FlaskBlog
Length of output: 479
🏁 Script executed:
# First, locate the file structure
git ls-files | grep -E "(models|app)" | head -20Repository: DogukanUrker/FlaskBlog
Length of output: 574
🏁 Script executed:
# Read the models.py file, specifically around lines 80-83
cat -n app/models.py | sed -n '70,95p'Repository: DogukanUrker/FlaskBlog
Length of output: 973
🏁 Script executed:
# Check the entire Bookmark model definition
cat -n app/models.py | grep -A 20 "class Bookmark"Repository: DogukanUrker/FlaskBlog
Length of output: 735
Enforce NOT NULL for bookmark foreign keys.
The user_id and post_id columns are nullable by default. In SQL, multiple NULL values satisfy unique constraints (NULL ≠ NULL), allowing multiple bookmarks with missing user or post references. This defeats the uniqueness constraint intent (ensuring each user can bookmark each post only once). Add nullable=False to both foreign key columns.
Suggested fix
- user_id = db.Column(db.Integer, db.ForeignKey("users.user_id", ondelete="CASCADE"))
- post_id = db.Column(db.Integer, db.ForeignKey("posts.id", ondelete="CASCADE"))
+ user_id = db.Column(db.Integer, db.ForeignKey("users.user_id", ondelete="CASCADE"), nullable=False)
+ post_id = db.Column(db.Integer, db.ForeignKey("posts.id", ondelete="CASCADE"), nullable=False)🤖 Prompt for AI Agents
In `@app/models.py` around lines 80 - 83, The user_id and post_id foreign key
columns are currently nullable which breaks the uniqueness intent; update the
bookmark model's user_id and post_id Column definitions to include
nullable=False (i.e., set db.Column(..., db.ForeignKey("users.user_id",
ondelete="CASCADE"), nullable=False) and similarly for post_id) so NULLs cannot
be inserted and any existing UniqueConstraint on this model will behave
correctly; locate the columns named user_id and post_id in the bookmark model in
app/models.py and add nullable=False to each db.Column call.
Comment on lines
+86
to
+134
| @bookmark_blueprint.route("/my-bookmarks") | ||
| def my_bookmarks(): | ||
| """Display user's bookmarked posts""" | ||
| try: | ||
| # Check if user is logged in | ||
| if "username" not in session: | ||
| return jsonify({"error": "User not logged in"}), 401 | ||
|
|
||
| # Get user ID | ||
| user = User.query.filter_by(username=session["username"]).first() | ||
| if not user: | ||
| return jsonify({"error": "User not found"}), 404 | ||
|
|
||
| # Get user's bookmarks with post details | ||
| bookmarks = db.session.query(Bookmark, Post).join( | ||
| Post, Bookmark.post_id == Post.id | ||
| ).filter( | ||
| Bookmark.user_id == user.user_id | ||
| ).order_by( | ||
| Bookmark.time_stamp.desc() | ||
| ).all() | ||
|
|
||
| # Format data for template | ||
| bookmarked_posts = [] | ||
| for bookmark, post in bookmarks: | ||
| bookmarked_posts.append({ | ||
| 'id': post.id, | ||
| 'title': post.title, | ||
| 'tags': post.tags, | ||
| 'content': post.content, | ||
| 'banner': post.banner, | ||
| 'author': post.author, | ||
| 'views': post.views, | ||
| 'time_stamp': post.time_stamp, | ||
| 'last_edit_time_stamp': post.last_edit_time_stamp, | ||
| 'category': post.category, | ||
| 'url_id': post.url_id, | ||
| 'abstract': post.abstract, | ||
| 'bookmark_time': bookmark.time_stamp | ||
| }) | ||
|
|
||
| return jsonify({ | ||
| "bookmarked_posts": bookmarked_posts, | ||
| "username": user.username | ||
| }), 200 | ||
|
|
||
| except Exception as e: | ||
| Log.error(f"Error getting user bookmarks: {e}") | ||
| return jsonify({"error": "Internal server error"}), 500 No newline at end of file |
There was a problem hiding this comment.
Resolve /my-bookmarks route conflict and JSON serialization risk.
There’s already an HTML page at /my-bookmarks in app/routes/my_bookmarks.py. This JSON endpoint will collide. Also, post.banner is LargeBinary and will likely fail JSON serialization.
✅ Suggested fix (example)
-@bookmark_blueprint.route("/my-bookmarks")
+@bookmark_blueprint.route("/api/my-bookmarks")
def my_bookmarks():
@@
- 'banner': post.banner,
- 'content': post.content,
+ # Consider returning banner_url instead of raw binary,
+ # and omit large content if not needed by the client.
'author': post.author,
@@
- 'abstract': post.abstract,
+ 'abstract': post.abstract,🤖 Prompt for AI Agents
In `@app/routes/bookmark.py` around lines 86 - 134, The /my-bookmarks view
(function my_bookmarks on the bookmark_blueprint) conflicts with the existing
HTML route and returns JSON containing Post.banner (LargeBinary) which isn't
JSON-safe; rename the endpoint (e.g., "/api/my-bookmarks" or "/bookmarks/json")
to avoid the collision with app/routes/my_bookmarks.py and update any clients,
and change the response for Post.banner so it is not raw LargeBinary — either
omit banner from the bookmarked_posts payload or convert post.banner to a
JSON-safe string (e.g., base64-encode) before adding it to the dict; keep the
rest of the logic and error handling the same.
|
|
||
| from database import db, init_db | ||
| from models import Bookmark | ||
| from app import create_app |
There was a problem hiding this comment.
create_app function does not exist in app.py.
Same issue as in test_bookmarks.py - the app module doesn't export a create_app factory function.
🐛 Proposed fix
-from app import create_app
+from app import appThen update line 20:
- app = create_app()
- with app.app_context():
+ with app.app_context():🤖 Prompt for AI Agents
In `@app/scripts/migrate_bookmarks.py` at line 14, The file imports a non-existent
factory create_app; either add a create_app() factory to app.py or change the
import/usage in migrate_bookmarks.py to use the exported Flask instance (e.g.,
import the app object from app) and update any call sites that call create_app()
(see the invocation around line 20) to use the imported app instance instead;
reference symbols: create_app (remove or implement) and app (use the Flask app
object) so the module imports and invocation match what app.py actually exports.
Comment on lines
+12
to
+70
| def test_bookmark_functionality(): | ||
| """Test bookmark functionality""" | ||
| print("🧪 Testing Bookmark Functionality") | ||
| print("=" * 50) | ||
|
|
||
| # Test 1: Check if server is running | ||
| print("1. Testing server connection...") | ||
| try: | ||
| response = requests.get(BASE_URL) | ||
| if response.status_code == 200: | ||
| print("✅ Server is running") | ||
| else: | ||
| print(f"❌ Server returned status {response.status_code}") | ||
| return False | ||
| except Exception as e: | ||
| print(f"❌ Server connection failed: {e}") | ||
| return False | ||
|
|
||
| # Test 2: Test bookmark API without login | ||
| print("\n2. Testing bookmark API without login...") | ||
| try: | ||
| response = requests.post(f"{BASE_URL}/api/bookmark/1") | ||
| if response.status_code == 401: | ||
| print("✅ Correctly rejected unauthenticated request") | ||
| else: | ||
| print(f"❌ Unexpected status code: {response.status_code}") | ||
| print(f"Response: {response.text}") | ||
| except Exception as e: | ||
| print(f"❌ API test failed: {e}") | ||
|
|
||
| # Test 3: Test bookmark status API without login | ||
| print("\n3. Testing bookmark status API without login...") | ||
| try: | ||
| response = requests.get(f"{BASE_URL}/api/bookmark/status/1") | ||
| if response.status_code == 200: | ||
| data = response.json() | ||
| if data.get("bookmarked") == False: | ||
| print("✅ Correctly returned False for unauthenticated user") | ||
| else: | ||
| print(f"❌ Unexpected response: {data}") | ||
| else: | ||
| print(f"❌ Unexpected status code: {response.status_code}") | ||
| except Exception as e: | ||
| print(f"❌ API test failed: {e}") | ||
|
|
||
| # Test 4: Test my-bookmarks page without login | ||
| print("\n4. Testing my-bookmarks page without login...") | ||
| try: | ||
| response = requests.get(f"{BASE_URL}/my-bookmarks", allow_redirects=False) | ||
| if response.status_code == 302: | ||
| print("✅ Correctly redirected to login page") | ||
| print(f"Redirect location: {response.headers.get('Location')}") | ||
| else: | ||
| print(f"❌ Unexpected status code: {response.status_code}") | ||
| except Exception as e: | ||
| print(f"❌ Page test failed: {e}") | ||
|
|
||
| print("\n🎯 Bookmark functionality tests completed!") | ||
| return True |
There was a problem hiding this comment.
Fail the script when any test step fails.
Currently, unexpected status codes only print errors and the function still returns True, which can mask regressions.
✅ Suggested fix
def test_bookmark_functionality():
"""Test bookmark functionality"""
+ success = True
@@
- else:
+ else:
print(f"❌ Unexpected status code: {response.status_code}")
print(f"Response: {response.text}")
+ success = False
@@
- else:
+ else:
print(f"❌ Unexpected status code: {response.status_code}")
+ success = False
@@
- else:
+ else:
print(f"❌ Unexpected status code: {response.status_code}")
+ success = False
@@
- return True
+ return success🤖 Prompt for AI Agents
In `@app/scripts/test_bookmark_api.py` around lines 12 - 70, The
test_bookmark_functionality currently prints failures but still returns True;
update each failure branch in functions like test_bookmark_functionality
(including server connection, unauthenticated POST to /api/bookmark/1, GET
/api/bookmark/status/1 checks, and /my-bookmarks redirect check) to immediately
fail the test by either returning False or raising an exception (choose one
consistent behavior) instead of merely printing; ensure the success path only
returns True at the end when no failure branch was taken, and keep the error
output (response.status_code, response.text, exception message) in the failure
return/exception to aid debugging.
|
|
||
| from database import db, init_db | ||
| from models import User, Post, Bookmark | ||
| from app import create_app |
There was a problem hiding this comment.
create_app function does not exist in app.py.
The app.py module creates the Flask application at module level (app = Flask(...) at line 136), not via a factory function. This import will fail with ImportError.
🐛 Proposed fix
-from app import create_app
+from app import appThen update line 20:
- app = create_app()
- with app.app_context():
+ with app.app_context():📝 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
| from app import create_app | |
| from app import app |
🤖 Prompt for AI Agents
In `@app/scripts/test_bookmarks.py` at line 14, The test imports a nonexistent
factory function create_app; instead import the module-level Flask instance
named app (the Flask variable defined in app.py) and use that in the
tests—replace the create_app import with importing app (e.g., from app import
app as flask_app) and update any references to create_app() to use flask_app (or
flask_app.test_client()) so the tests use the existing Flask application
instance.
Comment on lines
+2
to
+46
| async function toggleBookmark(postId, button) { | ||
| try { | ||
| const response = await fetch(`/api/bookmark/${postId}`, { | ||
| method: 'POST', | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| 'X-CSRFToken': csrfToken || '' | ||
| } | ||
| }); | ||
|
|
||
| if (response.ok) { | ||
| const data = await response.json(); | ||
|
|
||
| // Update button appearance | ||
| if (data.bookmarked) { | ||
| button.classList.remove('btn-ghost'); | ||
| button.classList.add('btn-primary'); | ||
| button.innerHTML = '<i class="ti ti-bookmark-filled text-xl"></i>'; | ||
| button.title = 'Remove bookmark'; | ||
| } else { | ||
| button.classList.remove('btn-primary'); | ||
| button.classList.add('btn-ghost'); | ||
| button.innerHTML = '<i class="ti ti-bookmark text-xl"></i>'; | ||
| button.title = 'Add bookmark'; | ||
| } | ||
|
|
||
| // Show success message (optional) | ||
| console.log(data.message); | ||
| } else { | ||
| console.error('Failed to toggle bookmark'); | ||
| alert('Please login to bookmark posts'); | ||
| } | ||
| } catch (error) { | ||
| console.error('Error toggling bookmark:', error); | ||
| alert('An error occurred. Please try again.'); | ||
| } | ||
| } | ||
|
|
||
| // Check bookmark status for a post | ||
| async function checkBookmarkStatus(postId, button) { | ||
| try { | ||
| const response = await fetch(`/api/bookmark/status/${postId}`, { | ||
| headers: { | ||
| 'X-CSRFToken': csrfToken || '' | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for csrfToken definition across the codebase
rg -n "csrfToken" --type html --type js -B 2 -A 2Repository: DogukanUrker/FlaskBlog
Length of output: 910
🏁 Script executed:
# Find where bookmark.js is included in templates
rg -n "bookmark\.js" --type htmlRepository: DogukanUrker/FlaskBlog
Length of output: 177
🏁 Script executed:
# Check layout.html for csrfToken definition and script order
cat -n app/templates/layout.html | head -100Repository: DogukanUrker/FlaskBlog
Length of output: 3154
🏁 Script executed:
# Find all references to toggleBookmark and checkBookmarkStatus
rg -n "toggleBookmark|checkBookmarkStatus" --type htmlRepository: DogukanUrker/FlaskBlog
Length of output: 264
🏁 Script executed:
# Find where post_card_macro is included
rg -n "post_card_macro" --type htmlRepository: DogukanUrker/FlaskBlog
Length of output: 645
🏁 Script executed:
# Check if csrfToken is defined in category.html, user.html, search.html, index.html, my_bookmarks.html
for file in app/templates/category.html app/templates/user.html app/templates/search.html app/templates/index.html app/templates/my_bookmarks.html; do
echo "=== $file ==="
grep -n "csrfToken" "$file" || echo "NOT FOUND"
doneRepository: DogukanUrker/FlaskBlog
Length of output: 290
Guard against undefined csrfToken to prevent runtime errors on non-post pages.
The csrfToken variable is defined only in post.html but bookmark.js is loaded globally in layout.html and called from post_card_macro.html which is used across category, user, search, index, and bookmarks pages. Clicking the bookmark button on any of these pages will throw a ReferenceError and break bookmark functionality.
✅ Suggested fix
async function toggleBookmark(postId, button) {
try {
+ const csrf = window.csrfToken || '';
const response = await fetch(`/api/bookmark/${postId}`, {
method: 'POST',
headers: {
'Content-Type': 'application/json',
- 'X-CSRFToken': csrfToken || ''
+ 'X-CSRFToken': csrf
}
});
@@
async function checkBookmarkStatus(postId, button) {
try {
+ const csrf = window.csrfToken || '';
const response = await fetch(`/api/bookmark/status/${postId}`, {
headers: {
- 'X-CSRFToken': csrfToken || ''
+ 'X-CSRFToken': csrf
}
});Alternatively, define csrfToken globally in layout.html before loading bookmark.js.
📝 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
| async function toggleBookmark(postId, button) { | |
| try { | |
| const response = await fetch(`/api/bookmark/${postId}`, { | |
| method: 'POST', | |
| headers: { | |
| 'Content-Type': 'application/json', | |
| 'X-CSRFToken': csrfToken || '' | |
| } | |
| }); | |
| if (response.ok) { | |
| const data = await response.json(); | |
| // Update button appearance | |
| if (data.bookmarked) { | |
| button.classList.remove('btn-ghost'); | |
| button.classList.add('btn-primary'); | |
| button.innerHTML = '<i class="ti ti-bookmark-filled text-xl"></i>'; | |
| button.title = 'Remove bookmark'; | |
| } else { | |
| button.classList.remove('btn-primary'); | |
| button.classList.add('btn-ghost'); | |
| button.innerHTML = '<i class="ti ti-bookmark text-xl"></i>'; | |
| button.title = 'Add bookmark'; | |
| } | |
| // Show success message (optional) | |
| console.log(data.message); | |
| } else { | |
| console.error('Failed to toggle bookmark'); | |
| alert('Please login to bookmark posts'); | |
| } | |
| } catch (error) { | |
| console.error('Error toggling bookmark:', error); | |
| alert('An error occurred. Please try again.'); | |
| } | |
| } | |
| // Check bookmark status for a post | |
| async function checkBookmarkStatus(postId, button) { | |
| try { | |
| const response = await fetch(`/api/bookmark/status/${postId}`, { | |
| headers: { | |
| 'X-CSRFToken': csrfToken || '' | |
| } | |
| async function toggleBookmark(postId, button) { | |
| try { | |
| const csrf = window.csrfToken || ''; | |
| const response = await fetch(`/api/bookmark/${postId}`, { | |
| method: 'POST', | |
| headers: { | |
| 'Content-Type': 'application/json', | |
| 'X-CSRFToken': csrf | |
| } | |
| }); | |
| if (response.ok) { | |
| const data = await response.json(); | |
| // Update button appearance | |
| if (data.bookmarked) { | |
| button.classList.remove('btn-ghost'); | |
| button.classList.add('btn-primary'); | |
| button.innerHTML = '<i class="ti ti-bookmark-filled text-xl"></i>'; | |
| button.title = 'Remove bookmark'; | |
| } else { | |
| button.classList.remove('btn-primary'); | |
| button.classList.add('btn-ghost'); | |
| button.innerHTML = '<i class="ti ti-bookmark text-xl"></i>'; | |
| button.title = 'Add bookmark'; | |
| } | |
| // Show success message (optional) | |
| console.log(data.message); | |
| } else { | |
| console.error('Failed to toggle bookmark'); | |
| alert('Please login to bookmark posts'); | |
| } | |
| } catch (error) { | |
| console.error('Error toggling bookmark:', error); | |
| alert('An error occurred. Please try again.'); | |
| } | |
| } | |
| // Check bookmark status for a post | |
| async function checkBookmarkStatus(postId, button) { | |
| try { | |
| const csrf = window.csrfToken || ''; | |
| const response = await fetch(`/api/bookmark/status/${postId}`, { | |
| headers: { | |
| 'X-CSRFToken': csrf | |
| } |
🤖 Prompt for AI Agents
In `@app/static/js/bookmark.js` around lines 2 - 46, The code uses a global
csrfToken that isn't defined on non-post pages and causes a ReferenceError; in
app/static/js/bookmark.js, guard against an undefined csrfToken by defining a
local safe token (e.g., const safeCsrf = typeof csrfToken !== 'undefined' ?
csrfToken : '') at the top of the file and use safeCsrf in the fetch headers
inside toggleBookmark and checkBookmarkStatus (replace 'X-CSRFToken': csrfToken
|| '' with the guarded variable) so bookmark.js can be loaded globally without
throwing.
Comment on lines
+12
to
+28
| <div class="flex justify-between items-start gap-2"> | ||
| <a | ||
| href="{{ url_for('post.post', slug=get_slug_from_post_title(post[1]), url_id=post[10]) }}" | ||
| class="card-title text-lg link link-hover line-clamp-2 flex-1" | ||
| > | ||
| {{ post[1] }} | ||
| </a> | ||
| {% if session['username'] %} | ||
| <button | ||
| class="btn btn-ghost btn-xs bookmark-btn" | ||
| data-post-id="{{ post[0] }}" | ||
| onclick="toggleBookmark({{ post[0] }}, this)" | ||
| title="Add bookmark" | ||
| > | ||
| <i class="ti ti-bookmark text-lg"></i> | ||
| </button> | ||
| {% endif %} |
There was a problem hiding this comment.
Add safe session access and accessible labeling for the bookmark button.
session['username'] can raise a KeyError and the icon-only button is missing an accessible label.
✅ Suggested fix
-{% if session['username'] %}
- <button
+{% if session.get('username') %}
+ <button
+ type="button"
class="btn btn-ghost btn-xs bookmark-btn"
data-post-id="{{ post[0] }}"
onclick="toggleBookmark({{ post[0] }}, this)"
- title="Add bookmark"
+ title="Add bookmark"
+ aria-label="Add bookmark"
>📝 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="flex justify-between items-start gap-2"> | |
| <a | |
| href="{{ url_for('post.post', slug=get_slug_from_post_title(post[1]), url_id=post[10]) }}" | |
| class="card-title text-lg link link-hover line-clamp-2 flex-1" | |
| > | |
| {{ post[1] }} | |
| </a> | |
| {% if session['username'] %} | |
| <button | |
| class="btn btn-ghost btn-xs bookmark-btn" | |
| data-post-id="{{ post[0] }}" | |
| onclick="toggleBookmark({{ post[0] }}, this)" | |
| title="Add bookmark" | |
| > | |
| <i class="ti ti-bookmark text-lg"></i> | |
| </button> | |
| {% endif %} | |
| <div class="flex justify-between items-start gap-2"> | |
| <a | |
| href="{{ url_for('post.post', slug=get_slug_from_post_title(post[1]), url_id=post[10]) }}" | |
| class="card-title text-lg link link-hover line-clamp-2 flex-1" | |
| > | |
| {{ post[1] }} | |
| </a> | |
| {% if session.get('username') %} | |
| <button | |
| type="button" | |
| class="btn btn-ghost btn-xs bookmark-btn" | |
| data-post-id="{{ post[0] }}" | |
| onclick="toggleBookmark({{ post[0] }}, this)" | |
| title="Add bookmark" | |
| aria-label="Add bookmark" | |
| > | |
| <i class="ti ti-bookmark text-lg"></i> | |
| </button> | |
| {% endif %} |
🤖 Prompt for AI Agents
In `@app/templates/components/post_card_macro.html` around lines 12 - 28, Replace
direct session indexing with session.get('username') in the conditional that
renders the bookmark button to avoid KeyError (update the check around the
bookmark block that currently uses session['username']), and add an accessible
label to the icon-only button (the element with class "bookmark-btn" and
onclick="toggleBookmark") by including an aria-label (and keep the existing
title) so screen readers can announce its purpose; keep the existing
toggleBookmark(postId, this) call and data-post-id attribute unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #
Proposed Changes
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.