fix: simplify sidenav UI and resolve issue #5431 #5385
Conversation
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. 💤 Files selected but had no reviewable changes (1)
You can disable this status message by setting the WalkthroughWidespread formatting and EOF-normalization across many files; two JavaScript behavioral changes (console.error → alert and removal of an error log), and a CSS edit that likely introduces an unmatched closing brace. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
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 |
|
👋 Hi @Saptami191! 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! |
📊 Monthly LeaderboardHi @Saptami191! Here's how you rank for January 2026:
Leaderboard based on contributions in January 2026. Keep up the great work! 🚀 |
❌ Pre-commit checks failedThe pre-commit hooks found issues that need to be fixed. Please run the following commands locally to fix them: # Install pre-commit if you haven't already
pip install pre-commit
# Run pre-commit on all files
pre-commit run --all-files
# Or run pre-commit on staged files only
pre-commit runAfter running these commands, the pre-commit hooks will automatically fix most issues. 💡 Tip: You can set up pre-commit to run automatically on every commit by running: pre-commit installPre-commit output (last 100 lines)For more information, see the pre-commit documentation. |
github-actions
bot
added
pre-commit: failed
❌ Pre-commit checks failedThe pre-commit hooks found issues that need to be fixed. Please run the following commands locally to fix them: # Install pre-commit if you haven't already
pip install pre-commit
# Run pre-commit on all files
pre-commit run --all-files
# Or run pre-commit on staged files only
pre-commit runAfter running these commands, the pre-commit hooks will automatically fix most issues. 💡 Tip: You can set up pre-commit to run automatically on every commit by running: pre-commit installPre-commit output (last 100 lines)For more information, see the pre-commit 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
website/static/organization/js/hunt_controller.js (2)
23-23: Remove all console statements flagged by the pipeline.The linter has identified multiple
console.error()statements across this file that must be removed before merge. These appear in error handling blocks (lines 23, 42, 107, 239, 260, 559, 601, 402) and prevent the pipeline from passing.🔎 Proposed fix: Remove console statements
For each occurrence, remove the entire
console.error(...)line. Example:- console.error('[add_prize] Could not find list-prize-container element');Apply this pattern to all flagged locations:
- Line 23:
console.error('[add_prize] Could not find list-prize-container element');- Line 42:
console.error(\[add_prize] Could not find element: ${key}`);`- Line 107:
console.error('[add_prize] Error:', error);- Line 239:
console.error('[PublishBughunt] Form not found');- Line 260:
console.error('[PublishBughunt] CSRF token not found in form');- Line 559:
console.error('[displayLogoPreview] Required elements not found');- Line 601:
console.error('Error:', error);- Line 402:
console.error('Error reading file');Since error information is being lost, consider using
alert()or a UI error state instead to inform users of issues.Also applies to: 42-42, 107-107, 239-239, 260-260, 559-559, 601-601, 402-402
13-13: Fix pervasive indentation inconsistencies throughout the file.Multiple lines have irregular indentation that degrades code readability. Lines 13, 69, 122, 130, 176, 219, 266, 283, 311, 315, 321, 322, 325, and 367 are noticeably mis-aligned. Use consistent spacing (2 or 4 spaces) aligned with the enclosing function/block scope.
website/static/js/issue.js (1)
388-388: Remove console.error statement flagged by the pipeline.Line 388 contains a
console.error()call in thefetchIssueSuggestionscatch block that blocks the pipeline. This must be removed before merge.🔎 Proposed fix: Remove the console statement
try { if (!append) { displayLoadingIndicator(); } else { appendLoadingIndicator(); } // Get issues from backend API const url = `/api/v1/issues?page=${page}`; const response = await fetch(url); if (!response.ok) throw new Error('API request failed'); const data = await response.json(); // Check for pagination headers (if provided) const linkHeader = response.headers.get('Link'); hasMorePages = linkHeader && linkHeader.includes('rel="next"'); // Filter issues by number containing the query const filteredIssues = data.results .filter(issue => issue.id.toString().includes(query)) .slice(0, 5); // Limit to 5 results per page // Cache the results cache.set(cacheKey, { issues: filteredIssues, hasMorePages: hasMorePages }); if (query === currentSearch) { displaySuggestions(filteredIssues, append); } } catch (error) { - console.error('Error fetching issues:', error); if (!append) { hideSuggestionBox(); } else { removeLoadingIndicator(); } } finally { isLoadingMore = false; }The error handling via
hideSuggestionBox()andremoveLoadingIndicator()already provides graceful fallback behavior, so the console statement can be safely removed without loss of user-facing functionality.
🧹 Nitpick comments (1)
website/documents/BltWeeklyActivityOfContributor.md (1)
44-44: Reduce wordiness on line 44.The phrase "large number of users and activity metrics" is unnecessarily verbose. Consider using "many users and activity metrics" or "numerous users and activity metrics" for conciseness.
🔎 Suggested improvement
- **Scalability**: The table is designed to handle a large number of users and activity metrics, maintaining performance and user experience even with extensive data. + **Scalability**: The table is designed to handle many users and activity metrics, maintaining performance and user experience even with extensive data.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (8)
website/static/images/tech/ml.svgis excluded by!**/*.svgwebsite/static/images/tech/nlp.svgis excluded by!**/*.svgwebsite/static/images/tech/oauth.svgis excluded by!**/*.svgwebsite/static/images/tech/python.svgis excluded by!**/*.svgwebsite/static/images/tech/sql.svgis excluded by!**/*.svgwebsite/static/images/tech/tailwind.svgis excluded by!**/*.svgwebsite/static/img/tomato-svgrepo-com.svgis excluded by!**/*.svgwebsite/static/js/jquery.sparkline.min.jsis excluded by!**/*.min.js
📒 Files selected for processing (40)
.github/codeql-config.yml.github/workflows/add-files-changed-label.yml.github/workflows/auto-fix-main-precommit.yml.github/workflows/auto-fix-pr-precommit.yml.lgtm.ymlBACON/bacon-etch.yamlBACON/ord-server/example-split.yamlBACON/regtest/bitcoin.confProcfiledocs/features.mdersLENOVOOneDriveDesktopowaspBLTwebsite/documents/BltAboutUs.mdwebsite/documents/BltBLTV.mdwebsite/documents/BltChangePassword.mdwebsite/documents/BltCommunityMembers.mdwebsite/documents/BltCompanyDashboard.mdwebsite/documents/BltCompanyListingPage.mdwebsite/documents/BltCompanyScoreboard.mdwebsite/documents/BltDetails.mdwebsite/documents/BltDetailsFromOwasp.mdwebsite/documents/BltGlobalLeaderboard.mdwebsite/documents/BltInvite.mdwebsite/documents/BltLoginPage.mdwebsite/documents/BltSignUpPage.mdwebsite/documents/BltStartaBughunt.mdwebsite/documents/BltStats.mdwebsite/documents/BltTerms.mdwebsite/documents/BltTrademarksSearch.mdwebsite/documents/BltTrademarksSearchResults.mdwebsite/documents/BltUserProfile.mdwebsite/documents/BltWeeklyActivityOfContributor.mdwebsite/static/css/app.min.csswebsite/static/css/checkInModal.csswebsite/static/js/debug-panel.jswebsite/static/js/gsoc_pr_report.jswebsite/static/js/issue.jswebsite/static/js/messages.jswebsite/static/js/reminder_settings.jswebsite/static/organization/js/hunt_controller.jswebsite/templates/includes/sidenav.html
🧰 Additional context used
🪛 GitHub Actions: Check Console Statements
website/static/organization/js/hunt_controller.js
[error] 23-582: Multiple console statements detected in file. Remove all console.* statements (console.log, console.error, etc.).
website/static/js/issue.js
[error] 388-388: Console statement detected: console.error('Error fetching issues:', error);
🪛 LanguageTool
website/documents/BltWeeklyActivityOfContributor.md
[style] ~44-~44: To reduce wordiness, try specifying a number or using “many” or “numerous” instead.
Context: ...lity**: The table is designed to handle a large number of users and activity metrics, maintaining...
(LARGE_NUMBER_OF)
🪛 markdownlint-cli2 (0.18.1)
website/documents/BltTerms.md
94-94: Bare URL used
(MD034, no-bare-urls)
⏰ 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). (3)
- GitHub Check: Run Tests
- GitHub Check: Run pre-commit
- GitHub Check: docker-test
🔇 Additional comments (37)
.lgtm.yml (1)
29-29: Scope mismatch: Changes extend beyond stated sidenav template objective.The PR objectives state "Scope: intentionally limited to the sidenav template only," but this file (
.lgtm.yml) is a LGTM static analysis configuration file unrelated to the sidenav UI. This contradicts the stated PR scope. Additionally, the AI-generated summary mentions "formatting corrections across the codebase," which further suggests broader-than-documented changes.The trailing newline addition itself is a standard and harmless formatting improvement. However, please clarify whether this scope expansion is intentional or accidental, and ensure it aligns with the PR review expectations.
website/documents/BltLoginPage.md (1)
47-47: Verify alignment with PR scope and feature implementation.This documentation change appears unrelated to the PR's stated objective (simplifying the sidenav UI in
website/templates/includes/sidenav.html). Before merging, clarify:
- Is this change in scope for this PR, or should it be split into a separate issue?
- Is the form persistence feature actually implemented in the login page code, or is this aspirational documentation?
Additionally, if credentials are indeed being stored in the browser, ensure proper security measures are in place (e.g., client-side only, no sensitive data in localStorage/sessionStorage, compliance with security best practices).
.github/workflows/auto-fix-pr-precommit.yml (1)
467-467: Trailing newline addition aligns with project conventions.The EOF newline normalization is a standard hygiene fix applied across the codebase by pre-commit hooks and does not alter any workflow behavior or logic.
website/documents/BltInvite.md (1)
79-79: Scope mismatch: PR objectives vs. actual changes.The PR objectives describe simplifying the sidenav UI to resolve issue #5431, but this file is a markdown documentation file with only a trailing newline added. This suggests the PR scope may extend beyond the stated sidenav refactoring to include formatting corrections across the codebase.
Please clarify:
- Is this formatting change intentional and part of a broader code hygiene effort?
- Does the PR actually include both sidenav refactoring AND formatting fixes, or are the objectives incomplete/outdated?
The trailing newline addition itself is a best practice and looks good, but the mismatch creates uncertainty about the true scope of this PR.
website/documents/BltSignUpPage.md (1)
1-44:⚠️ Scope mismatch: File outside stated PR objectives.The PR objectives explicitly state the scope is "intentionally limited to the sidenav template only," detailing changes to navigation grouping, mobile scrolling, and active-section auto-expand. However, the file under review (
website/documents/BltSignUpPage.md) is Sign Up Page documentation with no connection to the sidenav template.Please clarify: Was this file intended to be included in this PR, or has scope crept beyond the stated objectives?
website/documents/BltChangePassword.md (1)
44-44: Trailing newline addition is good practice, but verify scope alignment and pre-commit resolution.The formatting adjustment (trailing newline at end of file) is a best practice for markdown and text files. However, this file is outside the stated PR scope. Per the PR objectives, changes should be "intentionally limited to the sidenav template only," yet this PR appears to bundle formatting fixes across multiple documentation files.
Additionally, the PR status shows
pre-commit: failed. Verify that:
- This scope expansion (docs + sidenav) was intentional
- All remaining pre-commit failures have been addressed (especially in the primary sidenav changes)
- The pre-commit checks pass after this round of formatting fixes
website/documents/BltTerms.md (1)
94-94: Approved: trailing newline formatting fix.The addition of a trailing newline at EOF is correct and aligns with standard markdown formatting practices. This change mirrors similar formatting adjustments across the documentation files in this PR.
As a follow-up (optional for future passes): the static analysis tool flagged a pre-existing style issue (MD034: no-bare-urls) on line 94, where the email address could be wrapped in angle brackets (
<blt-support@owasp.org>) to conform to markdown linting standards. This is outside the current PR scope but could be addressed in a future formatting pass if desired..github/codeql-config.yml (1)
43-43: Minor formatting: trailing newline addition.Adding the missing trailing newline aligns with pre-commit and linting standards. No functional changes to the CodeQL configuration.
.github/workflows/add-files-changed-label.yml (1)
130-130: Minor formatting: trailing newline at EOF.Adding the trailing newline to the final log statement is a pre-commit formatting normalization. No impact on workflow logic or behavior.
.github/workflows/auto-fix-main-precommit.yml (2)
372-372: Minor formatting: trailing newline at EOF.Adding the trailing newline after the exit statement is a pre-commit formatting normalization. No impact on error handling or workflow exit behavior.
1-372:⚠️ Scope mismatch: PR objectives describe sidenav UI changes, but provided files show only formatting fixes.The PR objectives state the scope is "intentionally limited to the sidenav template only," with changes including grouped navigation links, reduced mobile scrolling, and auto-expanding active sections. However, all three provided files are GitHub Actions workflows and CodeQL config files—no sidenav template or component files are present.
The AI summary mentions "refactoring the website navigation sidebar component," but this cannot be verified from the provided files.
Please clarify:
- Are the sidenav template changes in additional files not shown in this review?
- If the PR scope has changed, please update the PR description to reflect the actual changes (trailing newline formatting).
BACON/regtest/bitcoin.conf (1)
13-13: Cosmetic trailing newline addition — LGTM.Adding a trailing newline at end-of-file is a standard code hygiene practice and has no functional impact on Bitcoin's config parsing.
BACON/bacon-etch.yaml (1)
12-12: Critical YAML syntax fix — well caught.The original
-- file:uses incorrect YAML list syntax. The double dash--is not a valid list indicator; the correct syntax is a single dash-. This change fixes a structural YAML parsing error that would have prevented the inscriptions list from being correctly interpreted.Procfile (1)
3-3: LGTM - Formatting improvement, but inconsistent with PR objectives.The addition of a trailing newline is a valid formatting improvement that aligns with POSIX standards and common linting requirements. However, this change is inconsistent with the PR objectives, which describe sidenav UI template modifications limited to the sidenav template only. The Procfile is a deployment configuration file unrelated to the sidenav UI.
This appears to be part of a broader formatting cleanup across the codebase (as noted in the AI summary), which may explain the "pre-commit: failed" status.
website/documents/BltStats.md (1)
46-46: Trailing newline formatting — approved.This change adds a missing trailing newline at EOF, a formatting standard with no functional impact. No content changes detected.
website/documents/BltTrademarksSearchResults.md (1)
42-42: Trailing newline formatting — approved.This change adds a missing trailing newline at EOF, consistent with formatting standards. No content changes detected.
website/documents/BltUserProfile.md (1)
35-35: Trailing newline formatting — approved.This change adds a missing trailing newline at EOF. No content changes detected.
website/documents/BltAboutUs.md (1)
98-98: Scope mismatch detected: PR objectives vs. provided filesThe PR objectives state changes are limited to the sidenav template to "simplify the mobile/hamburger sidenav UI" and resolve issue #5431. However, this file is a markdown documentation file with a formatting-only change (EOF newline adjustment). The sidenav template file referenced in the PR objectives is not provided in this review.
Please clarify:
- Is the sidenav template file included in this PR?
- Are these documentation formatting changes intentional or unrelated to the stated PR objective?
- Why does the AI summary mention both "formatting corrections across the codebase" and "refactoring the website navigation sidebar component" as if they are separate activities?
website/documents/BltDetails.md (1)
28-28: Trailing newline addition is appropriate.The EOF newline aligns with common pre-commit hook requirements and linting conventions.
website/documents/BltCompanyDashboard.md (1)
45-45: Formatting adjustment preserves content integrity.The line content remains unchanged; any repositioning aligns with document consistency.
docs/features.md (1)
56-56: Markdown list syntax correction.The bullet prefix change from
--to-fixes invalid markdown list syntax and aligns with markdown specifications.website/documents/BltCommunityMembers.md (1)
42-42: Trailing newline formatting applied.This is a standard pre-commit formatting adjustment with no semantic impact.
website/documents/BltTrademarksSearch.md (1)
39-39: Bullet formatting adjustment.The content is unchanged; this is a formatting-only adjustment (diff-marker prefix change) with no semantic impact.
website/documents/BltCompanyListingPage.md (1)
42-42: Bullet formatting adjustment.Content is unchanged; formatting-only adjustment consistent with changes in other documentation files.
website/documents/BltDetailsFromOwasp.md (1)
1-431: Scope discrepancy: Documentation changes vs. PR objectives.The PR objectives state this work is limited to "simplify the mobile/hamburger sidenav UI to improve usability on small screens" with changes "intentionally limited to the sidenav template only." However, this file is markdown documentation for the OWASP project overview, not a sidenav template. The AI summary also mentions broader "formatting corrections across the codebase."
Can you clarify the actual scope of this PR? Are there sidenav template files that should be included in this review, or has the scope expanded beyond the stated objectives?
website/documents/BltBLTV.md (1)
43-43: Formatting change approved; scope concern noted.The trailing newline addition is technically sound, but this file change appears unrelated to the stated PR objective of simplifying sidenav UI. This aligns with the broader formatting corrections mentioned in the AI summary, which extends beyond the "intentionally limited" scope claimed in the PR objectives.
Please clarify whether the PR scope has broadened to include documentation/formatting corrections across the codebase.
website/documents/BltGlobalLeaderboard.md (1)
43-43: Formatting change approved; consistent with file-wide pattern.The trailing newline addition is technically sound. This follows the same formatting pattern observed in BltBLTV.md and aligns with the PR-wide newline normalization mentioned in the AI summary.
website/documents/BltStartaBughunt.md (2)
46-46: Markdown formatting normalization looks good.The bullet point normalization from
--to-improves consistency with standard markdown syntax.
1-46: Scope mismatch: PR objectives vs. actual changes.The PR objectives state that changes are "intentionally limited to the sidenav template only" to "simplify the mobile/hamburger sidenav UI." However, the file under review is a documentation markdown file (
website/documents/BltStartaBughunt.md), which doesn't align with this scope.The AI summary indicates the PR includes widespread formatting changes (trailing newlines, bullet normalization, CSS/YAML fixes) across multiple file types beyond the sidenav template. This appears to be scope creep from the stated objective.
Please clarify: Are these documentation formatting changes intentional, or should the PR scope remain limited to sidenav template changes as described?
website/static/js/reminder_settings.js (1)
51-51: Trailing newline addition looks good.This follows POSIX conventions (files should end with a newline). No functional changes detected; code logic remains intact.
website/static/js/debug-panel.js (1)
511-511: Trailing newline addition approved.This follows POSIX conventions. The surrounding debug panel code is well-structured with solid error handling and defensive null-checking practices.
website/static/js/gsoc_pr_report.js (1)
939-939: Trailing newline addition approved.Follows POSIX conventions. The report generation code demonstrates solid defensive practices (escaped JSON parsing, safe logging, external script timeout handling).
website/static/js/messages.js (1)
111-111: LGTM!Trailing newline formatting corrected. No functional changes.
website/static/js/issue.js (1)
735-735: LGTM!Trailing newline formatting corrected. No functional changes.
ersLENOVOOneDriveDesktopowaspBLT (1)
1-8: Cannot review: Git log output provided instead of actual source code.The content provided is git commit metadata (commit hash, author, date, diffstat) rather than the actual file content. The referenced file
website/templates/includes/sidenav.htmlwith the 309 insertions and 679 deletions is not included here.Additionally, the filename
ersLENOVOOneDriveDesktopowaspBLTappears malformed—it looks like a corrupted local Windows path fragment rather than a valid repository file path.Please re-submit with the actual
website/templates/includes/sidenav.htmltemplate content so I can review the sidenav UI changes.website/static/css/app.min.css (1)
7607-7607: LGTM! Essential CSS syntax fix.This adds the missing closing brace for the
.fa-angle-rightrule block (lines 7597-7606), fixing a CSS syntax error that would have prevented proper parsing.website/static/css/checkInModal.css (1)
147-147: No CSS syntax error found. The CSS file has properly balanced braces (22 opening, 22 closing), and the closing brace at line 147 correctly closes the final style rule. The stylesheet is syntactically valid.Likely an incorrect or invalid review comment.
BACON/ord-server/example-split.yaml
Outdated
| - address: bcrt1pvn4u72tm3c6jzg2ea6h0ldhvlr60mzfnlrn587el82ug4lasrdwqvqgrst | ||
| runes: | ||
| BLT•BACON•TOKENS: 1 | ||
| BLT•BACON•TOKENS: 1 |
There was a problem hiding this comment.
Change appears to be out of scope for this PR.
The PR objectives explicitly state: "Scope: intentionally limited to the sidenav template only" and describe UI improvements for mobile navigation. However, this file (BACON/ord-server/example-split.yaml) is a Bitcoin ORD server configuration file, completely unrelated to sidenav UI changes.
While adding a trailing newline is technically correct and follows best practices, this change should either:
- Be removed from this PR and included in a separate formatting/linting PR, or
- The PR objectives should be updated to reflect that this includes broader formatting fixes across the codebase
🤖 Prompt for AI Agents
In BACON/ord-server/example-split.yaml around line 11, this unrelated formatting
change (adding a trailing newline to a Bitcoin ORD server config) is outside the
PR scope which is limited to the sidenav template; either revert/remove this
file from the PR (unstage/reset the change or revert the commit that touched
this file) so only sidenav changes remain, or update the PR description to
explicitly include repository-wide formatting/linting changes and ensure the
commit message documents the formatting scope.
| G@ surerowr Bloomberg’ | ||
| Fa CUARDSQUARE BIONIC | ||
| 4:SailPoint root | ||
| 4:SailPoint root |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the file exists and read line 432 with context
if [ -f "website/documents/BltDetailsFromOwasp.md" ]; then
echo "=== Line 432 with 10 lines of context above and below ==="
sed -n '422,442p' "website/documents/BltDetailsFromOwasp.md" | cat -n -v
echo ""
echo "=== Total lines in file ==="
wc -l "website/documents/BltDetailsFromOwasp.md"
else
echo "File not found"
fiRepository: OWASP-BLT/BLT
Length of output: 427
Remove malformed content from Corporate Supporters section.
Line 432 contains "4:SailPoint root" with an unusual "4:" prefix that doesn't match standard documentation formatting. This line, along with surrounding lines showing prefixes like "G@" and "Fa" plus character encoding corruption, indicates this section contains formatting artifacts or corrupted content that should be removed or properly reformatted.
🤖 Prompt for AI Agents
In website/documents/BltDetailsFromOwasp.md around line 432, remove the
malformed entry "4:SailPoint root" (and any nearby prefixed/corrupted artifacts
like "G@" or "Fa" or stray character-encoding fragments) from the Corporate
Supporters section and replace with either a correctly formatted supporter line
or delete the line entirely; ensure the surrounding list follows the document's
standard formatting (no numeric prefixes like "4:" unless consistent with the
list style) and run a quick visual scan of nearby lines to remove any other
encoding artifacts so the section is clean and consistent.
github-project-automation
bot
moved this from Backlog
to Ready
in 📌 OWASP BLT Project Board
Saptami191
force-pushed
the
fix-5431-sidenav-clean
branch
from
1cd5331 to
69fbfb4
Compare
github-actions
bot
added
changes-requested
❌ Pre-commit checks failedThe pre-commit hooks found issues that need to be fixed. Please run the following commands locally to fix them: # Install pre-commit if you haven't already
pip install pre-commit
# Run pre-commit on all files
pre-commit run --all-files
# Or run pre-commit on staged files only
pre-commit runAfter running these commands, the pre-commit hooks will automatically fix most issues. 💡 Tip: You can set up pre-commit to run automatically on every commit by running: pre-commit installPre-commit output (last 100 lines)For more information, see the pre-commit documentation. |
github-actions
bot
added
tests: passed
github-actions
bot
added
files-changed: 47
![sentry[bot]](https://avatars.githubusercontent.com/u/1396951?s=60&v=4){kind=small}
| <!-- Submenu --> | ||
| <div class="ml-8 mt-1 space-y-1"> | ||
| <a href="https://github.com/OWASP-BLT/BLT/" | ||
| target="_blank" | ||
| rel="noopener noreferrer" | ||
| class="group flex items-center px-2 py-2 text-lg font-medium rounded-md text-gray-700 dark:text-gray-300 hover:bg-gray-100 dark:hover:bg-gray-800 hover:text-[#e74c3c] dark:hover:text-[#e74c3c] transition-all duration-200"> | ||
| <div class="mr-3 flex-shrink-0 w-5 h-5 flex items-center justify-center text-gray-500 dark:text-gray-400 group-hover:text-[#e74c3c] dark:group-hover:text-[#e74c3c] transition-all duration-200"> | ||
| <i class="fas fa-code-branch"></i> | ||
| </div> | ||
| <span class="truncate">{% trans "BLT Core" %}</span> | ||
| </a> |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
website/documents/BltWeeklyActivityOfContributor.md (1)
44-44: Reduce wordiness: Replace "large number of" with a more concise alternative.The static analysis tool flags the phrase "large number of users and activity metrics" as unnecessarily verbose. Consider using "many" or "numerous" instead for improved clarity.
🔎 Proposed improvement
- **Scalability**: The table is designed to handle a large number of users and activity metrics, maintaining performance and user experience even with extensive data. + **Scalability**: The table is designed to handle many users and activity metrics, maintaining performance and user experience even with extensive data.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (8)
website/static/images/tech/ml.svgis excluded by!**/*.svgwebsite/static/images/tech/nlp.svgis excluded by!**/*.svgwebsite/static/images/tech/oauth.svgis excluded by!**/*.svgwebsite/static/images/tech/python.svgis excluded by!**/*.svgwebsite/static/images/tech/sql.svgis excluded by!**/*.svgwebsite/static/images/tech/tailwind.svgis excluded by!**/*.svgwebsite/static/img/tomato-svgrepo-com.svgis excluded by!**/*.svgwebsite/static/js/jquery.sparkline.min.jsis excluded by!**/*.min.js
📒 Files selected for processing (39)
.github/codeql-config.yml.github/workflows/add-files-changed-label.yml.github/workflows/auto-fix-main-precommit.yml.github/workflows/auto-fix-pr-precommit.yml.lgtm.ymlBACON/bacon-etch.yamlBACON/ord-server/example-split.yamlBACON/regtest/bitcoin.confProcfiledocs/features.mdwebsite/documents/BltAboutUs.mdwebsite/documents/BltBLTV.mdwebsite/documents/BltChangePassword.mdwebsite/documents/BltCommunityMembers.mdwebsite/documents/BltCompanyDashboard.mdwebsite/documents/BltCompanyListingPage.mdwebsite/documents/BltCompanyScoreboard.mdwebsite/documents/BltDetails.mdwebsite/documents/BltDetailsFromOwasp.mdwebsite/documents/BltGlobalLeaderboard.mdwebsite/documents/BltInvite.mdwebsite/documents/BltLoginPage.mdwebsite/documents/BltSignUpPage.mdwebsite/documents/BltStartaBughunt.mdwebsite/documents/BltStats.mdwebsite/documents/BltTerms.mdwebsite/documents/BltTrademarksSearch.mdwebsite/documents/BltTrademarksSearchResults.mdwebsite/documents/BltUserProfile.mdwebsite/documents/BltWeeklyActivityOfContributor.mdwebsite/static/css/app.min.csswebsite/static/css/checkInModal.csswebsite/static/js/debug-panel.jswebsite/static/js/gsoc_pr_report.jswebsite/static/js/issue.jswebsite/static/js/messages.jswebsite/static/js/reminder_settings.jswebsite/static/organization/js/hunt_controller.jswebsite/templates/includes/sidenav.html
✅ Files skipped from review due to trivial changes (7)
- .github/workflows/add-files-changed-label.yml
- BACON/ord-server/example-split.yaml
- website/documents/BltChangePassword.md
- website/static/js/messages.js
- website/static/js/reminder_settings.js
- website/documents/BltCompanyScoreboard.md
- website/documents/BltCompanyListingPage.md
🚧 Files skipped from review as they are similar to previous changes (25)
- website/static/js/gsoc_pr_report.js
- website/documents/BltUserProfile.md
- website/documents/BltLoginPage.md
- website/documents/BltTrademarksSearch.md
- .lgtm.yml
- website/static/css/app.min.css
- website/static/organization/js/hunt_controller.js
- Procfile
- .github/codeql-config.yml
- website/documents/BltStartaBughunt.md
- website/static/js/debug-panel.js
- website/documents/BltAboutUs.md
- website/static/js/issue.js
- website/documents/BltInvite.md
- website/documents/BltStats.md
- website/documents/BltGlobalLeaderboard.md
- .github/workflows/auto-fix-pr-precommit.yml
- BACON/regtest/bitcoin.conf
- website/documents/BltSignUpPage.md
- website/documents/BltDetailsFromOwasp.md
- docs/features.md
- website/documents/BltCommunityMembers.md
- website/documents/BltBLTV.md
- .github/workflows/auto-fix-main-precommit.yml
- website/documents/BltDetails.md
🧰 Additional context used
🪛 LanguageTool
website/documents/BltWeeklyActivityOfContributor.md
[style] ~44-~44: To reduce wordiness, try specifying a number or using “many” or “numerous” instead.
Context: ...lity**: The table is designed to handle a large number of users and activity metrics, maintaining...
(LARGE_NUMBER_OF)
🪛 markdownlint-cli2 (0.18.1)
website/documents/BltTerms.md
94-94: Bare URL used
(MD034, no-bare-urls)
⏰ 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 (7)
BACON/bacon-etch.yaml (2)
1-12: Major inconsistency: File content doesn't match PR objectives.The PR objectives state this PR "simplifies the sidenav UI" with changes "limited to the sidenav template only." However, this file (
BACON/bacon-etch.yaml) is a blockchain/rune etching configuration file that has no relationship to sidenav UI components.Either the PR description is incorrect, or this file shouldn't be part of this PR.
12-12: LGTM! Valid YAML syntax correction.The change from double-dash (
--) to single-dash (-) is correct. YAML list items must use a single dash followed by a space. The double-dash syntax would cause parsing errors.website/documents/BltTrademarksSearchResults.md (1)
42-42: Scope discrepancy: PR objective vs. actual changes.This file change contradicts the stated PR scope. The PR objectives explicitly state that changes are "intentionally limited to the sidenav template only," but this is a documentation file (BltTrademarksSearchResults.md) with formatting adjustments. The line 42 change appears to be trailing whitespace or formatting normalization unrelated to sidenav UI simplification.
Please clarify whether:
- These documentation changes are intentional and part of a separate formatting effort, or
- This represents unintended scope creep that should be reverted to keep the PR focused on the sidenav template fix.
website/documents/BltCompanyDashboard.md (1)
45-45: Scope discrepancy: Unexplained no-op change to documentation file.This file also contradicts the PR scope (sidenav template only). Line 45 shows identical content being deleted and re-inserted ("Efficient Data Retrieval"), which suggests either a no-op formatting change or an accidental modification. If the intent was only to normalize trailing newlines across the repo, this should be separated into a distinct formatting PR.
Please confirm:
- Whether this documentation change is intentional, or
- Whether these documentation files should be reverted to keep the PR focused on the sidenav template fix.
website/static/css/checkInModal.css (2)
1-147: PR scope mismatch: checkInModal.css changes not mentioned in objectives.The PR objectives state that changes are "intentionally limited to the sidenav template only" to simplify mobile navigation UI. However, this file (
checkInModal.css) contains modal styling, not sidenav-related code. There's no mention of modal changes in the PR description.Please clarify:
- Are these CSS changes intentional and related to the sidenav work?
- Should this file be part of a separate PR focused on CSS formatting/fixes?
- Are there other files changed beyond the stated sidenav template?
147-147: No CSS syntax error detected in this change. The file has balanced braces (22 opening, 22 closing) and valid CSS syntax. The closing brace at line 147 properly closes the.modal-form::-webkit-scrollbar-thumb:hoverrule block.Likely an incorrect or invalid review comment.
website/documents/BltWeeklyActivityOfContributor.md (1)
1-44: No issues found.This PR performs a repository-wide line-ending normalization. The change to
BltWeeklyActivityOfContributor.mdis appropriate—only the EOF newline was modified, with no content changes. This file's inclusion in the normalization is correct.
github-actions
bot
added
tests: failed
|
All requested changes have been addressed and CI is green. |
gojo-satorou-v7
left a comment
gojo-satorou-v7
left a comment
There was a problem hiding this comment.
Please update the PR to include only relevant files. It looks like this branch is outdated.
Fixes #5431 This PR simplifies the mobile/hamburger sidenav UI to improve usability on small screens. Changes: - Grouped navigation links into collapsible sections - Reduced scrolling on mobile - Auto-expands the active section - Preserves all existing navigation functionality Scope: Changes are intentionally limited to the sidenav template only.
Saptami191
force-pushed
the
fix-5431-sidenav-clean
branch
from
08c81cf to
043cc61
Compare
github-actions
bot
added
files-changed: 1
and removed
files-changed: 47
|
Hi @gojo-satorou-v7 — I’ve cleaned up the branch and limited the PR strictly to website/templates/includes/sidenav.html. |
S3DFX-CYBER
left a comment
S3DFX-CYBER
left a comment
There was a problem hiding this comment.
Hi @Saptami191, thanks for working on the mobile UI! I noticed a few things that might be blocking this:
1.Check your CSS: CodeRabbit flagged an unmatched closing brace in checkInModal.css. That might cause layout issues.
2.Alert vs Console: I noticed you replaced console.error with alert(). Usually, we want to avoid alert() as it blocks the main thread and can be a bit intrusive for users. Maybe we could use a non-blocking toast notification instead?
3.Pre-commit: You have a lot of EOF/formatting changes that are causing the pre-commit to fail. Running pre-commit run --all-files locally should help clean those up.
Great work on the collapsible sections though!
|
hi @S3DFX-CYBER , |
github-actions
bot
added
last-active: 0d
3 participants
Fixes #5431
This PR simplifies the mobile/hamburger sidenav UI to improve usability on small screens.
Changes
Scope
Changes are intentionally limited to the sidenav template only.
Testing
Summary by CodeRabbit
Style
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.