Convert High-Impact Images to WebP and Update Template References #5263
Conversation
|
👋 Hi @Nachiket-Roy! This pull request needs a peer review before it can be merged. Please request a review from a team member who is not:
Once a valid peer review is submitted, this check will pass automatically. Thank you! |
github-actions
bot
added
files-changed: 67
WalkthroughAdds django-silk and integrates Silk middleware/URL conditioned on DEBUG and not TESTING, adjusts debug tooling in settings, makes TEMPLATES and staticfiles storage environment-driven, adds a PNG→WebP conversion script, updates many templates to reference .webp assets and fixes a small template typo/conditional. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
❌ Tests failedThe Django tests found issues that need to be fixed. Please review the test output below and fix the failing tests. How to run tests locally# Install dependencies
poetry install --with dev
# Run all tests
poetry run python manage.py test
# Run tests with verbose output
poetry run python manage.py test -v 3
# Run a specific test
poetry run python manage.py test app.tests.TestClass.test_methodTest output (last 100 lines)For more information, see the Django testing documentation. |
❌ Tests failedThe Django tests found issues that need to be fixed. Please review the test output below and fix the failing tests. How to run tests locally# Install dependencies
poetry install --with dev
# Run all tests
poetry run python manage.py test
# Run tests with verbose output
poetry run python manage.py test -v 3
# Run a specific test
poetry run python manage.py test app.tests.TestClass.test_methodTest output (last 100 lines)For more information, see the Django testing documentation. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
website/templates/search.html (1)
490-496: Fix project logo references:organizationis undefined in the projects loopInside the
{% for project in projects %}section, both project and project.organization logos are rendered usingorganization.logo, which is not defined in this scope and will either fail or accidentally reuse a stale variable from a previous loop.You likely want to reference the project’s own logo fields:
{% if project.logo %} - <img class="h-24 w-24 object-cover rounded-lg border border-indigo-200 mr-6" - src="{{ organization.logo.url }}" - alt="{{ organization.name }} Logo" - height="96" - width="96"> + <img class="h-24 w-24 object-cover rounded-lg border border-indigo-200 mr-6" + src="{{ project.logo.url }}" + alt="{{ project.name }} Logo" + height="96" + width="96"> {% else %} ... {% endif %} ... {% if project.organization %} {% if project.organization.logo %} - <img class="h-24 w-24 object-cover rounded-lg border border-indigo-200 mr-6" - src="{{ organization.logo.url }}" - alt="{{ organization.name }} Logo" - height="96" - width="96"> + <img class="h-24 w-24 object-cover rounded-lg border border-indigo-200 mr-6" + src="{{ project.organization.logo.url }}" + alt="{{ project.organization.name }} Logo" + height="96" + width="96"> {% endif %} <h3 ...> Organization: <span class="text-blue-600">{{ project.organization.name }}</span> </h3> {% endif %}This will ensure the correct images render and avoid depending on an out-of-scope variable.
Also applies to: 539-545
blt/settings.py (2)
21-23: Avoid loggingDATABASE_URLand other secretsPrinting the
.envpath is fine, but loggingDATABASE_URLrisks exposing credentials in logs; this is particularly problematic in shared or production environments. Consider removing theDATABASE_URLprint or guarding it behind a local-only debug flag that never runs in deployed environments.
113-136: Move Silk middleware into the DEBUG block to match INSTALLED_APPS configurationSilk middleware is currently in
MIDDLEWAREunconditionally (line 114), but thesilkapp is only added toINSTALLED_APPSwhenDEBUG=True(line 106). This mismatch causes the middleware to attempt importing a module that won't exist in production, risking import errors or unexpected behavior. Additionally, running the profiler in production creates both performance and security concerns.Update the configuration to gate Silk middleware by DEBUG:
MIDDLEWARE = [ - "silk.middleware.SilkyMiddleware", "django.contrib.sessions.middleware.SessionMiddleware", "blt.middleware.domain.DomainMiddleware", @@ -126,7 +125,9 @@ MIDDLEWARE = [ "blt.middleware.user_visit_tracking.VisitTrackingMiddleware", ] if DEBUG: + MIDDLEWARE = ["silk.middleware.SilkyMiddleware", *MIDDLEWARE] MIDDLEWARE += ("livereload.middleware.LiveReloadScript", "debug_toolbar.middleware.DebugToolbarMiddleware") INTERNAL_IPS = ["127.0.0.1"]This ensures Silk runs first (per django-silk documentation) and only when DEBUG is enabled, matching the INSTALLED_APPS behavior.
🧹 Nitpick comments (8)
scripts/convert_selected_to_webp.py (1)
14-16: Verify the features directory exists before globbing.If the features directory doesn't exist,
glob("*.png")will silently return an empty list, which may not be the intended behavior.Consider adding a check:
# Add all PNGs from img/features/ features_dir = Path("website/static/img/features") if features_dir.exists(): TARGET_FILES += [str(p) for p in features_dir.glob("*.png")] +else: + print(f"⚠️ Features directory not found: {features_dir}")website/templates/comments2.html (1)
256-347: Comment create/update JS looks good; consider trimming input and cleaning up minor redundancyThe AJAX flows for posting and updating comments (loading states, CSRF header, error fallback) look solid and are a nice UX improvement.
Two small, optional tweaks:
- Reject pure‑whitespace comments by trimming before the emptiness check.
- In
remove_update_label, the$('#update_comment_btn').off('click')after you’ve just renamed the id topost_comment_btnis effectively a no‑op and can be dropped.For example:
- let comment = document.getElementById("comment").value; - if (comment == "") { + let comment = document.getElementById("comment").value.trim(); + if (!comment) { return; }You can mirror the same in
update_comment. The rest of the logic can remain as is.Also applies to: 350-385
website/templates/includes/issue_right_info.html (1)
5-26: Avatar fallback switched to WebP and still preserves gravatar / custom avatar chainThe long conditional
srcchain correctly keeps the existing order (user avatar → gravatar → anonymous dummy-user.webp), just updating the final fallback to WebP. The rest of the block is unchanged logically.If you touch this again later, consider breaking the
imgtag over multiple lines with an{% if %}block for readability, but it’s fine functionally as is.website/templates/search.html (1)
820-871: Start‑chat fetch handler and CSRF helper look fineThe DOMContentLoaded handler that wires
.start-chatbuttons tofetch("/messaging/start-thread/...")with a JSON body and CSRF header fromgetCookie("csrftoken")is sound. Error handling viacatchand a simple alert is acceptable for this context.Optional for later: consider checking
response.okbeforeresponse.json()to handle non‑JSON error responses more gracefully.website/templates/issue.html (2)
200-227:deleteIssuefetch flow is correct; consider aligning CSRF handling with the rest of the appThe delete handler correctly:
- Confirms with the user before proceeding.
- Sends a POST to
/delete_issue/<id>/withX-CSRFTokenand JSON content type.- Redirects on success and shows a notification on failure.
If you later centralize CSRF handling (e.g., via a shared
getCookie("csrftoken")helper like insearch.html), this would be a good place to reuse it instead of inlining{{ csrf_token }}in JS.
255-295: Markdown rendering + issue‑linkification works, but consider guarding DOMPurify and scoping the#123regexThe DOMContentLoaded handler that:
- Uses
markdown-itto renderobject.markdown_description,- Rewrites
#123patterns into/issue/123links, and- Sanitizes via
DOMPurify.sanitizewith a tight allowlistis a good pattern for safe rich text.
Two follow‑ups you might consider:
- Guard DOMPurify: if the CDN script fails to load,
DOMPurifywill be undefined and this block will throw. A simpleif (window.DOMPurify)(or try/catch) would make this more robust.- Regex scope: the
/#(\d+)/greplacement runs on the full rendered HTML string, so any#123inside attributes or code blocks is also rewritten. That may be acceptable, but if you want cleaner behavior you could instead walk text nodes and apply the regex only to text content.Functionally this is fine today; these are just hardening suggestions.
blt/urls.py (1)
1245-1268: Silk URLs are correctly gated behindDEBUG; consider folding into the existing blockAdding:
if settings.DEBUG: urlpatterns += [ path("silk/", include("silk.urls", namespace="silk")), ]ensures the Silk UI is only exposed when
DEBUGis true, which is exactly what you want for a profiler.For tidiness, you could merge this into the existing
if settings.DEBUG:block a few lines above (where you add__debug__/and the debug API endpoints), but that’s purely stylistic.blt/settings.py (1)
164-211: TEMPLATES DEBUG/prod split is good; consider basing it on the final DEBUG valueThe separate DEBUG vs non-DEBUG
TEMPLATESconfigs (simple vs cached loaders) are reasonable, but they’re evaluated before the"DYNO"block later that forcesDEBUG = Falseon Heroku. If someone misconfiguresDEBUGvia env in a dyno environment, you could end up withDEBUG=Falsebut still using the “debug” templates configuration.Consider either:
- Moving the entire
if DEBUG: ... else: ...TEMPLATESblock below the"DYNO"logic so it uses the finalDEBUGvalue, or- Introducing a separate flag (e.g.,
DEBUG_TEMPLATES) derived from env and using that consistently for bothDEBUGand template configuration.Also, double-check that all previously-configured context processors and options are preserved in both branches so behavior doesn’t diverge unexpectedly across environments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
blt/settings.py(5 hunks)blt/urls.py(1 hunks)pyproject.toml(1 hunks)scripts/convert_selected_to_webp.py(1 hunks)website/templates/bacon.html(1 hunks)website/templates/comments2.html(14 hunks)website/templates/domain.html(8 hunks)website/templates/features.html(13 hunks)website/templates/home.html(26 hunks)website/templates/includes/_activity.html(1 hunks)website/templates/includes/_bug.html(1 hunks)website/templates/includes/_new_activity.html(2 hunks)website/templates/includes/issue_right_info.html(10 hunks)website/templates/issue.html(5 hunks)website/templates/organization/dashboard/add_slack_integration.html(1 hunks)website/templates/organization/dashboard/organization_integrations.html(2 hunks)website/templates/organization/dashboard/organization_team_overview.html(7 hunks)website/templates/search.html(24 hunks)website/templates/users.html(4 hunks)
🔇 Additional comments (28)
website/templates/organization/dashboard/add_slack_integration.html (1)
41-45: Verify WebP asset availability and browser compatibility.The change references
slack_icon.webp, but I need to confirm:
- The WebP file exists in
website/static/images/directory- WebP browser support aligns with your target audience (older browsers may lack native support)
Consider adding a fallback mechanism (e.g.,
<picture>element with a PNG fallback) if broad browser compatibility is required.website/templates/organization/dashboard/organization_integrations.html (2)
45-49: Verify WebP asset availability (Active Integrations section).Confirm that
slack_icon.webpexists inwebsite/static/images/and that WebP format is compatible with your target browsers. Consider a<picture>element with PNG fallback if needed for older browser support.
143-147: Verify WebP asset availability (Available Integrations section).Confirm that
slack_icon.webpexists inwebsite/static/images/and that WebP format is compatible with your target browsers. Consider a<picture>element with PNG fallback if needed for older browser support.website/templates/includes/_bug.html (1)
32-32: LGTM! Default avatar fallback updated to WebP.The fallback avatar image has been updated to use the WebP format, maintaining consistency with the project-wide asset migration.
website/templates/includes/_activity.html (1)
32-32: LGTM! Default avatar fallback updated to WebP.Consistent with the WebP migration across avatar fallback images.
website/templates/users.html (2)
8-9: Cosmetic text reflow.These line breaks improve readability without changing the rendered content.
Also applies to: 18-19
73-73: LGTM! Avatar fallbacks updated to WebP across all contexts.All avatar fallback references (including
onerrorattributes) have been consistently updated to use WebP format.Also applies to: 102-102, 110-110, 113-113
website/templates/includes/_new_activity.html (3)
57-57: LGTM! Avatar fallback updated to WebP.
68-84: Verify text reflow and content reorganization.The username, domain, timestamp, URL, and comments have been restructured with different line breaks and positioning. While this appears to be intentional formatting, ensure the rendered layout displays correctly.
87-94: Verify button duplication is intentional.A new wrapper div has been added with two buttons for status and label display. Confirm this restructuring is intentional and doesn't duplicate existing buttons elsewhere in the template.
website/templates/home.html (7)
7-9: Cosmetic text reflow in meta blocks.Line breaks improve readability without changing rendered content.
Also applies to: 15-17
147-151: Cosmetic text reflow in content paragraphs.Paragraph text has been reflowed across lines for better source readability without affecting the rendered output.
Also applies to: 163-167, 746-750
459-459: LGTM! Avatar fallbacks updated to WebP in leaderboard section.Both the main avatar image and fallback in the
onerrorattribute now reference the WebP format.Also applies to: 461-461
1273-1275: Cosmetic text reflow in Google supporter description.Text reflowed for readability without changing content.
262-266: Template variable formatting changes.Template variables and expressions have been reformatted across multiple lines for consistency. These are cosmetic changes that don't affect functionality.
Also applies to: 272-276, 282-286, 292-296, 301-303, 377-379, 591-593, 621-623, 653-655, 684-686, 788-790, 814-816, 853-855, 861-863, 906-910, 918-920, 947-949, 958-960, 970-972
1337-1345: JavaScript formatting cleanup.Minor formatting improvements including removing an empty line and adjusting function spacing.
1266-1266: Google logo updated to WebP format.The Google logo reference has been updated from PNG to WebP as part of the high-impact image optimization. The WebP file exists at the expected location.
website/templates/domain.html (3)
14-16: Cosmetic text and template variable reflow.These formatting changes improve source code readability without affecting rendered output.
Also applies to: 76-77, 99-100, 206-208, 222-224, 255-257, 264-266, 269-271, 274-276, 365-367
354-354: LGTM! Avatar fallbacks updated to WebP with proper error handling.Both avatar sources properly fall back to the WebP format in their
onerrorattributes.Also applies to: 361-361
388-427: JavaScript formatting cleanup.The chart initialization code has been reformatted with consistent indentation and spacing. No functional changes.
website/templates/bacon.html (1)
34-34: LGTM! Image format updated to WebP.The bacon image reference has been successfully updated to use the WebP format, which provides better compression and smaller file sizes. The bacon.webp file exists at the expected location, and there are no broken references to the old image format in the active codebase.
website/templates/comments2.html (1)
190-223: Dropdown initialization and teardown are robustThe
initDropDownimplementation (closing other menus first and closing on outside click) plus wiring it from$(document).readyis clean and avoids multiple listener stacking.No changes needed here.
Also applies to: 452-455
website/templates/organization/dashboard/organization_team_overview.html (1)
78-99: WebP avatar fallbacks and formatting changes look correctBoth organization members and daily report rows now fall back to
images/dummy-user.webpand the surrounding username rendering is unchanged aside from line wrapping. No functional issues spotted.Also applies to: 260-282
website/templates/includes/issue_right_info.html (1)
110-224: Multi-line{{ ... }}wrapping is safeAll the places where you’ve rewrapped Django template expressions across lines (e.g.,
object.created,object.get_label_display,object.cve_id,github_issues_url,email_clicks,email_events) remain valid and won’t change the rendered values.No action needed.
website/templates/features.html (1)
37-1102: WebP asset migration in Features page looks consistentThe Features grid now consistently uses
.webpunderimg/features/(andimages/bacon.webp), matching the stated optimization goal. All surrounding template tags and links are untouched aside from line wrapping.Assuming the corresponding
.webpfiles exist in static storage, this should be a safe, purely-presentational change.Would you mind double‑checking (e.g., via
collectstaticor a quick run in DEBUG) that every referencedimg/features/*.webpandimages/bacon.webpactually exists and serves correctly?website/templates/issue.html (1)
117-133: Reporter avatar fallback updated to WebP and remains functionally correctThe reporter avatar still prefers a user profile avatar, then gravatar, and now falls back to
images/dummy-user.webp. The logic is unchanged aside from the image format and aligns with other templates in this PR.No changes needed.
blt/settings.py (2)
105-107: Debug-only INSTALLED_APPS additions look appropriateAppending
silk,debug_toolbar, andlivereloadonly whenDEBUGis true keeps production lean and avoids unnecessary debug dependencies in normal runs; no change needed here.
316-318: WhiteNoiseCompressedManifestStaticFilesStorageswitch is consistent across environmentsUsing
whitenoise.storage.CompressedManifestStaticFilesStoragefor thestaticfilesstorage in both the GCS/Heroku and local branches lines up withwhitenoise.middleware.WhiteNoiseMiddlewareand ensures compressed, hashed static assets everywhere; this looks correct.Also applies to: 331-333
github-project-automation
bot
moved this from Backlog
to Ready
in 📌 OWASP BLT Project Board
github-actions
bot
added
the
changes-requested
![sentry[bot]](https://avatars.githubusercontent.com/u/1396951?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
blt/settings.py (2)
114-132: DuplicateSilkyMiddlewareinMIDDLEWARElist.
silk.middleware.SilkyMiddlewareappears at both line 115 and line 131. This will double-process requests, causing incorrect profiling data and potential performance degradation.Keep only one instance—typically at the top for accurate request timing.
MIDDLEWARE = [ "silk.middleware.SilkyMiddleware", "django.contrib.sessions.middleware.SessionMiddleware", # ... other middleware ... "blt.middleware.user_visit_tracking.VisitTrackingMiddleware", - "silk.middleware.SilkyMiddleware", ]
104-107: Duplicate"silk"inINSTALLED_APPSwill crash the app in DEBUG mode.
"silk"is added unconditionally at line 104 and again conditionally at line 107 whenDEBUG=True. Django's app registry raisesImproperlyConfiguredfor duplicate entries.Remove
"silk"from either the base tuple or the DEBUG block.INSTALLED_APPS = ( # ... other apps ... - "silk", ) if DEBUG: - INSTALLED_APPS += ("silk", "debug_toolbar", "livereload") + INSTALLED_APPS += ("debug_toolbar", "livereload", "silk")
🧹 Nitpick comments (3)
scripts/convert_selected_to_webp.py (1)
24-37: Good fix on error logging!The previous review concern about silent failures has been properly addressed with clear logging at each stage.
Consider these optional enhancements for production robustness:
- Skip existing files: Check if the WebP already exists and is newer than the source PNG to avoid redundant conversions.
- Return status: Return a boolean or status enum so callers can track success/failure counts.
- Verify output: After saving, verify the output file exists and has non-zero size.
Example for skipping existing files:
def convert_to_webp(png_path_str): png_path = Path(png_path_str) if not png_path.exists(): logger.warning(f"Not found: {png_path}") return webp_path = png_path.with_suffix(".webp") + if webp_path.exists() and webp_path.stat().st_mtime > png_path.stat().st_mtime: + logger.info(f"Skipped (up-to-date): {webp_path}") + return + try: img = Image.open(png_path).convert("RGBA") img.save(webp_path, "webp", quality=85, method=6) logger.info(f"Converted: {png_path} → {webp_path}") except Exception as e: logger.error(f"Failed: {png_path} — {e}")pyproject.toml (1)
73-73: Consider movingdjango-silkto dev dependencies.Silk is only activated when
DEBUG=True(perblt/settings.py), so it doesn't need to be installed in production. Moving it to[tool.poetry.group.dev.dependencies]alongsidedjango-debug-toolbaranddjango-livereload-serverwould reduce the production dependency footprint.-django-silk = "^5.4.0"Add to dev dependencies section instead:
[tool.poetry.group.dev.dependencies] # ... existing entries ... django-silk = "^5.4.0"blt/settings.py (1)
104-137: Make Silk integration consistently DEBUG-only.Currently,
"silk"is added unconditionally toINSTALLED_APPSandSilkyMiddlewareis unconditionally inMIDDLEWARE, but Silk profiling is only useful in development. For consistency and to avoid production overhead:
- Move
"silk"to the DEBUG block inINSTALLED_APPS- Add
SilkyMiddlewareconditionally in the DEBUG middleware blockINSTALLED_APPS = ( # ... other apps ... "channels", - "silk", ) if DEBUG: - INSTALLED_APPS += ("silk", "debug_toolbar", "livereload") + INSTALLED_APPS += ("debug_toolbar", "livereload", "silk") MIDDLEWARE = [ - "silk.middleware.SilkyMiddleware", "django.contrib.sessions.middleware.SessionMiddleware", # ... rest of middleware ... "blt.middleware.user_visit_tracking.VisitTrackingMiddleware", - "silk.middleware.SilkyMiddleware", ] if DEBUG: - MIDDLEWARE += ("livereload.middleware.LiveReloadScript", "debug_toolbar.middleware.DebugToolbarMiddleware") + MIDDLEWARE = ["silk.middleware.SilkyMiddleware"] + MIDDLEWARE + MIDDLEWARE += ["livereload.middleware.LiveReloadScript", "debug_toolbar.middleware.DebugToolbarMiddleware"] INTERNAL_IPS = ["127.0.0.1"]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
blt/settings.py(5 hunks)pyproject.toml(1 hunks)scripts/convert_selected_to_webp.py(1 hunks)website/templates/comments2.html(14 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- website/templates/comments2.html
🔇 Additional comments (7)
scripts/convert_selected_to_webp.py (4)
1-8: LGTM! Clean imports and logging setup.The logging configuration is appropriate for a conversion script, providing clear output without excessive verbosity.
10-21: LGTM! Pragmatic approach to target file selection.The combination of explicit high-impact files plus automatic discovery in the features directory aligns well with the PR's optimization strategy.
39-41: LGTM! Clean and straightforward.The main function effectively coordinates the batch conversion process.
44-45: LGTM! Standard Python entrypoint pattern.Properly allows the script to be both executable and importable.
blt/settings.py (3)
134-137: LGTM!The conditional addition of debug middleware and
INTERNAL_IPSconfiguration is appropriate for DEBUG mode.
166-212: LGTM!The conditional TEMPLATES configuration properly uses
APP_DIRS=Truefor development flexibility and cached template loaders for production performance. SettingAPP_DIRS=Falsewhen using custom loaders is required by Django.
317-335: LGTM!Using
whitenoise.storage.CompressedManifestStaticFilesStorageenables pre-compressed static files with cache-busting hashes—a recommended optimization for production deployments with WhiteNoise.
❌ Tests failedThe Django tests found issues that need to be fixed. Please review the test output below and fix the failing tests. How to run tests locally# Install dependencies
poetry install --with dev
# Run all tests
poetry run python manage.py test
# Run tests with verbose output
poetry run python manage.py test -v 3
# Run a specific test
poetry run python manage.py test app.tests.TestClass.test_methodTest output (last 100 lines)For more information, see the Django testing documentation. |
❌ Tests failedThe Django tests found issues that need to be fixed. Please review the test output below and fix the failing tests. How to run tests locally# Install dependencies
poetry install --with dev
# Run all tests
poetry run python manage.py test
# Run tests with verbose output
poetry run python manage.py test -v 3
# Run a specific test
poetry run python manage.py test app.tests.TestClass.test_methodTest output (last 100 lines)For more information, see the Django testing documentation. |
github-actions
bot
removed
the
changes-requested
❌ Tests failedThe Django tests found issues that need to be fixed. Please review the test output below and fix the failing tests. How to run tests locally# Install dependencies
poetry install --with dev
# Run all tests
poetry run python manage.py test
# Run tests with verbose output
poetry run python manage.py test -v 3
# Run a specific test
poetry run python manage.py test app.tests.TestClass.test_methodTest output (last 100 lines)For more information, see the Django testing documentation. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
blt/settings.py (1)
129-136: Silky middleware is duplicated in DEBUG and enabled even whensilkis not installedTwo related issues here:
silk.middleware.SilkyMiddlewareis in the baseMIDDLEWARElist (Line 129) and also prepended in theif DEBUG:block (Line 134), so withDEBUG=Truethe middleware runs twice on every request.- Outside DEBUG,
INSTALLED_APPSdoes not include"silk", butMIDDLEWAREstill containssilk.middleware.SilkyMiddleware, which contradicts the django-silk installation guidance that requires adding both'silk'toINSTALLED_APPSandSilkyMiddlewaretoMIDDLEWAREtogether.(silk.readthedocs.io)This can cause startup or runtime issues and unnecessary overhead in non-debug environments.
A minimal fix is to remove the unconditional middleware entry and keep the debug-only injection:
MIDDLEWARE = [ "django.contrib.sessions.middleware.SessionMiddleware", @@ - "blt.middleware.user_visit_tracking.VisitTrackingMiddleware", - "silk.middleware.SilkyMiddleware", + "blt.middleware.user_visit_tracking.VisitTrackingMiddleware", ] if DEBUG: - MIDDLEWARE = ["silk.middleware.SilkyMiddleware"] + MIDDLEWARE + MIDDLEWARE = ["silk.middleware.SilkyMiddleware"] + MIDDLEWARE MIDDLEWARE += ["livereload.middleware.LiveReloadScript", "debug_toolbar.middleware.DebugToolbarMiddleware"] INTERNAL_IPS = ["127.0.0.1"]This ensures:
- In DEBUG: Silk is both installed and its middleware is active (once, at the front of the chain).
- In non-DEBUG: Neither the Silk app nor its middleware are active.
🧹 Nitpick comments (1)
blt/settings.py (1)
165-211: TEMPLATES now depend onDEBUGbefore the later Heroku (DYNO) override
TEMPLATESis selected based onDEBUGhere, but the"DYNO" in os.environblock later in the file forcibly setsDEBUG = Falsefor Heroku. That means if someone accidentally setsDEBUG=Truevia environment in Heroku, you’ll still end up with the debug template configuration (APP_DIRS=True, no cached loader) even thoughDEBUGis later forced toFalse.This can leave you in a partially “debug-like” template mode in production.
Consider refactoring so that the final value of
DEBUG(including the Heroku override) is computed before TEMPLATES and other DEBUG-dependent settings, e.g.:
- Move the
if "DYNO" in os.environblock above the TEMPLATES definition, or- Compute a single
IS_DEBUG = (env DEBUG) and "DYNO" not in os.environflag once, setDEBUG = IS_DEBUG, and base TEMPLATES on that.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
blt/settings.py(5 hunks)pyproject.toml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pyproject.toml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Run Tests
- GitHub Check: docker-test
🔇 Additional comments (2)
blt/settings.py (2)
105-106: Debug-only INSTALLED_APPS additions look goodConditionally adding
debug_toolbar,livereload, andsilkwhenDEBUGisTruekeeps these tools out of production and matches common Django patterns; no issues here from a settings perspective. Please just ensure these packages are present in your dev dependencies so imports don’t fail at startup.
317-333: Switch toCompressedManifestStaticFilesStoragelooks correctUsing
whitenoise.storage.CompressedManifestStaticFilesStoragefor"staticfiles"in both the Heroku and localSTORAGESblocks aligns with current WhiteNoise recommendations for hashed + compressed static assets.(whitenoise.readthedocs.io)Just confirm that:
- Your WhiteNoise version supports
CompressedManifestStaticFilesStorageat this import path (it has for multiple major versions), and- You don’t also define
STATICFILES_STORAGE, which would conflict withSTORAGES["staticfiles"]in recent Django versions.(learn.microsoft.com)
❌ Tests failedThe Django tests found issues that need to be fixed. Please review the test output below and fix the failing tests. How to run tests locally# Install dependencies
poetry install --with dev
# Run all tests
poetry run python manage.py test
# Run tests with verbose output
poetry run python manage.py test -v 3
# Run a specific test
poetry run python manage.py test app.tests.TestClass.test_methodTest output (last 100 lines)For more information, see the Django testing documentation. |
|
@coderabbitai why the tests are failing? |
|
@coderabbitai but i already patched it to if settings.DEBUG and not settings.TESTING:
urlpatterns += [
path("silk/", include("silk.urls", namespace="silk")),
] |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1270 🏁 Script executed: Length of output: 470 🏁 Script executed: Length of output: 3494 🏁 Script executed: Length of output: 19312 🏁 Script executed: Length of output: 2266 🏁 Script executed: Length of output: 1697 🏁 Script executed: Length of output: 4046 🏁 Script executed: Length of output: 137 🏁 Script executed: Length of output: 68 🏁 Script executed: Length of output: 6945 🏁 Script executed: Length of output: 5533 @Nachiket-Roy, you're absolutely correct! I can confirm the silk fix has been applied in if settings.DEBUG and not settings.TESTING:
urlpatterns += [
path("silk/", include("silk.urls", namespace="silk")),
]✅ The silk issue is resolved! However, the tests are still failing, but now for a completely different reason. The latest test run shows a new template syntax error in The New IssueThis error is in Looking at the template structure, line 808-809 has a multiline {% if not issues and not domains and not users and not organizations and not projects and not tags and not
repos %}This appears to be malformed - the line break in the middle of the How to FixCheck
{% if not issues and not domains and not users and not organizations and not projects and not tags and not repos %}
The formatting changes you made to wrap long lines in the templates may have accidentally broken this template tag syntax. |
|
@coderabbitai check if removed some code by mistake which should not be a part of this pr? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 30153 🏁 Script executed: Length of output: 28037 🏁 Script executed: Length of output: 332 @Nachiket-Roy, good news: No code was accidentally removed! The template tag structure is perfectly balanced: Template Tag Counts:
✅ All tags are balanced, nothing was deleted. The Real ProblemHowever, I found what's causing the template syntax error. Your formatting changes broke Django template syntax rules. Throughout ❌ Invalid (what you have now): <span class="text-[#e74c3c] font-bold bg-red-50 px-3 py-1 rounded-full">{{
organizations|length }}</span>❌ Another example: <a href="{{ domain.get_absolute_url }}" class="hover:text-blue-600">{{
domain.name }}</a>Django does NOT allow breaking The FixYou need to keep all ✅ Correct: <span class="text-[#e74c3c] font-bold bg-red-50 px-3 py-1 rounded-full">{{ organizations|length }}</span>Or if the line is too long, break the HTML tags instead: <span class="text-[#e74c3c] font-bold bg-red-50 px-3 py-1 rounded-full">
{{ organizations|length }}
</span>The diff shows dozens of these broken template variables throughout the file. You'll need to fix all instances where |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
website/templates/search.html (2)
483-488: Fix incorrect variable reference in project logo display.Line 485 references
organization.logo.url, but this block iterates overprojects(line 480). The logo should reference the current project, not an undefinedorganizationvariable. This will cause a runtime error or display no logo.Apply this diff:
{% if project.logo %} <img class="h-24 w-24 object-cover rounded-lg border border-indigo-200 mr-6" - src="{{ organization.logo.url }}" - alt="{{ organization.name }} Logo" + src="{{ project.logo.url }}" + alt="{{ project.name }} Logo" height="96" width="96">
528-535: Fix incorrect variable reference in project organization logo display.Line 531 references
organization.logo.url, butorganizationis not the loop variable. In this context, it should beproject.organization.logo.urlto correctly reference the project's associated organization logo.Apply this diff:
{% if project.organization %} {% if project.organization.logo %} <img class="h-24 w-24 object-cover rounded-lg border border-indigo-200 mr-6" - src="{{ organization.logo.url }}" - alt="{{ organization.name }} Logo" + src="{{ project.organization.logo.url }}" + alt="{{ project.organization.name }} Logo" height="96" width="96">
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (7)
website/templates/domain.html(8 hunks)website/templates/home.html(17 hunks)website/templates/includes/_new_activity.html(1 hunks)website/templates/includes/issue_right_info.html(2 hunks)website/templates/issue.html(4 hunks)website/templates/organization/dashboard/organization_team_overview.html(7 hunks)website/templates/search.html(16 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- website/templates/organization/dashboard/organization_team_overview.html
- website/templates/includes/issue_right_info.html
- website/templates/issue.html
- website/templates/home.html
- website/templates/domain.html
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Run Tests
- GitHub Check: docker-test
🔇 Additional comments (1)
website/templates/includes/_new_activity.html (1)
57-57: WebP fallback image correctly updated and verified.The change properly updates the avatar fallback to use
dummy-user.webp, which exists inwebsite/static/images/and is included in the PR. The Django template syntax is valid and aligns with the WebP migration objective.
❌ Tests failedThe Django tests found issues that need to be fixed. Please review the test output below and fix the failing tests. How to run tests locally# Install dependencies
poetry install --with dev
# Run all tests
poetry run python manage.py test
# Run tests with verbose output
poetry run python manage.py test -v 3
# Run a specific test
poetry run python manage.py test app.tests.TestClass.test_methodTest output (last 100 lines)For more information, see the Django testing documentation. |
github-actions
bot
added
the
changes-requested
❌ Tests failedThe Django tests found issues that need to be fixed. Please review the test output below and fix the failing tests. How to run tests locally# Install dependencies
poetry install --with dev
# Run all tests
poetry run python manage.py test
# Run tests with verbose output
poetry run python manage.py test -v 3
# Run a specific test
poetry run python manage.py test app.tests.TestClass.test_methodTest output (last 100 lines)For more information, see the Django testing documentation. |
|
@coderabbitai why the tests are failing now? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 289 🏁 Script executed: Length of output: 3168 🏁 Script executed: Length of output: 84 🏁 Script executed: Length of output: 227 🏁 Script executed: Length of output: 3919 🏁 Script executed: Length of output: 73207 🏁 Script executed: Length of output: 1106 🏁 Script executed: Length of output: 2057 🏁 Script executed: Length of output: 1989 🏁 Script executed: Length of output: 1052 @Nachiket-Roy, the tests are still failing due to template syntax errors in The ProblemYour PR modified Key Issues Found:
The FixYou need to fix
SummaryThe same formatting issues that affected |
github-actions
bot
removed
the
changes-requested
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
website/templates/profile.html (2)
65-65: Typo: “Follwings” → “Following”Fix copy for clarity and i18n consistency.
- {{ following|length }} <span ...> {% trans "Follwings" %} </span> + {{ following|length }} <span ...> {% trans "Following" %} </span>
546-554: Stray “=” in class outputWhen pr.is_merged, class contains a literal “=” token. Clean it to avoid invalid class names.
- <span class="... {% if pr.is_merged %} = text-purple-700 {% elif pr.state == 'open' %} text-yellow-700 {% else %} text-red-700 {% endif %} text-lg"> + <span class="... {% if pr.is_merged %} text-purple-700 {% elif pr.state == 'open' %} text-yellow-700 {% else %} text-red-700 {% endif %} text-lg">
♻️ Duplicate comments (1)
website/templates/comments2.html (1)
135-139: Child comment avatar uses viewer’s avatar, not reply author’sUse the child comment author’s avatar, with dummy-user.webp fallback, mirroring the top-level block.
- <img class="w-8 h-8 rounded-full object-cover" - {% if user.userprofile.user_avatar %} src="{{ MEDIA_URL }}{{ user.userprofile.user_avatar }}" {% else %} src="{% static 'images/dummy-user.webp' %}" {% endif %} - alt="{{ child_comment.author }} profile" - width="32" - height="32"> + <img class="w-8 h-8 rounded-full object-cover" + {% if child_comment.author_fk.user_avatar %} + src="{{ MEDIA_URL }}{{ child_comment.author_fk.user_avatar }}" + {% else %} + src="{% static 'images/dummy-user.webp' %}" + {% endif %} + alt="{{ child_comment.author }} profile" + width="32" + height="32">(Adjust author_fk to actual relation if different.)
🧹 Nitpick comments (1)
website/templates/features.html (1)
32-36: WebP migration looks solidAll featured tiles now reference WebP; alt text and dimensions preserved.
Optional: add
for broader fallback If you want graceful PNG/JPEG fallback for older browsers, wrap each image with a picture element.
<picture> <source srcset="{% static 'img/features/pr-review.webp' %}" type="image/webp"> <img src="{% static 'img/features/pr-review.png' %}" alt="PR_review" width="200" height="100" class="rounded-md mb-3 w-full h-52 object-contain"> </picture>Also applies to: 54-59, 76-81, 98-103, 120-125, 142-147, 161-166, 180-185, 199-204, 218-223, 237-242, 256-261, 275-280, 294-299, 313-318, 332-337, 351-356, 370-375, 389-394, 408-413, 427-432, 446-451, 465-470, 484-489, 503-508, 522-527, 541-546, 560-565, 579-584, 598-603, 617-622, 636-641, 655-660, 674-679, 693-698, 712-717, 731-736, 750-755, 769-774, 788-793, 807-812, 826-831, 856-861, 878-883, 900-905, 923-928
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (9)
website/templates/comments2.html(2 hunks)website/templates/domain.html(1 hunks)website/templates/features.html(46 hunks)website/templates/home.html(2 hunks)website/templates/includes/issue_right_info.html(2 hunks)website/templates/issue.html(1 hunks)website/templates/organization/dashboard/organization_team_overview.html(2 hunks)website/templates/profile.html(4 hunks)website/templates/users.html(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- website/templates/issue.html
- website/templates/includes/issue_right_info.html
- website/templates/organization/dashboard/organization_team_overview.html
- website/templates/users.html
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Run Tests
- GitHub Check: docker-test
🔇 Additional comments (4)
website/templates/profile.html (1)
289-289: LGTM on WebP fallbacksonerror fallback to dummy-user.webp is correct and non-breaking. No further action needed.
Also applies to: 437-437
website/templates/domain.html (1)
345-346: LGTM on WebP fallbacksGood defensive onerror swap to dummy-user.webp for leaderboard avatars.
Also applies to: 352-353
website/templates/comments2.html (1)
54-56: LGTM on default avatar swapTop-level comment fallback to dummy-user.webp looks good.
website/templates/home.html (1)
437-440: LGTM on WebP swaps
- Avatar onerror fallback to dummy-user.webp is correct.
- Google logo switched to WebP; dimensions preserved.
Also applies to: 1198-1203
github-actions
bot
added
tests: passed
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
blt/settings.py (1)
132-135: Dev-only middleware ordering looks good; consider centralizing the "debug tools enabled" flagPrepending
SilkyMiddlewareand appending livereload/debug-toolbar only whenDEBUG and not TESTINGis correct and avoids production/test issues. One robustness tweak to consider:
- Define a single flag (e.g.,
DEBUG_TOOLING_ENABLED = DEBUG and not TESTING) after allDEBUG/TESTINGlogic is finalized (including theDYNOoverride at line 285), and reuse it for:
- Adding dev apps to
INSTALLED_APPS(line 105)- Adding dev middleware (line 132)
- Configuring debug toolbar panels (line 141)
- Wiring dev URLs in
blt/urls.py(line 1265)This pattern (
DEBUG and not TESTING) appears at least 4 times across the codebase; centralizing keeps all debug/profiling toggles consistent if the wayDEBUG/TESTINGare set changes later.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
blt/settings.py(5 hunks)website/templates/profile.html(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Run Tests
- GitHub Check: docker-test
🔇 Additional comments (6)
website/templates/profile.html (5)
65-65: Typo fix approved.Changed button label from "Follwings" to "Following" — straightforward correction.
477-477: WebP fallback update consistent.Updated onerror fallback from
dummy-user.pngtodummy-user.webpin the top-findings section.
587-587: Template conditional syntax corrected.Fixed the inline conditional class binding. The structure now correctly applies text color classes based on PR state (
pr.is_merged,pr.state == 'open', or default). The conditional logic is sound and properly closed.
782-782: WebP updates in legacy code section.Updated image fallback references in the commented-out old code block (lines 782, 932). While these lines are commented out and not active, the updates maintain consistency with the WebP migration across the file.
Also applies to: 932-932
329-329: WebP fallback update is correct and deployment-ready.The onerror fallback has been correctly updated from
dummy-user.pngtodummy-user.webp. The WebP asset exists atwebsite/static/images/dummy-user.webpand is ready for deployment.blt/settings.py (1)
316-316: Staticfiles storage switched toCompressedManifestStaticFilesStoragein all environmentsUsing
whitenoise.storage.CompressedManifestStaticFilesStorageforstaticfilesin both the Heroku and non-Heroku branches aligns with WhiteNoise best practices. The middleware is correctly positioned right afterSecurityMiddleware, which is the recommended placement. This configuration will improve caching and compression behavior for static files across all environments.Also applies to: 331-331
github-actions
bot
added
has-peer-review
3 participants
Related to : #5251
This PR implements image optimization strategy by:
Converted high-impact .png images to .webp format:
dummy-user.png
bacon.png, googlelogo_color_272x92dp.png, slack_icon.png
All images in img/features/
Updating corresponding template references from .png to .webp
Keeping original .png files untouched for now (to avoid breaking links elsewhere)
Including silk package setup
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.