Enhance VulnerableApp Legacy UI to have toggle feature as well as Card generation logic#652
Open
luks-santos wants to merge 4 commits into
Open
Conversation
…at/legacy-ui-challenge-toggle-cards
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR adds a Scanner/Challenge mode toggle to the UI, mock vulnerability data with challenge cards, CSS for mode/tile styling, and JS to compute, render, and switch between scanner help and interactive challenge cards when levels are selected. ChangesChallenge Mode and Scanner/Challenge Toggle
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #652 +/- ##
=========================================
Coverage 54.69% 54.69%
Complexity 663 663
=========================================
Files 91 91
Lines 3587 3587
Branches 397 397
=========================================
Hits 1962 1962
Misses 1448 1448
Partials 177 177 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/main/resources/static/vulnerableApp.js (3)
486-511: ⚡ Quick winConsider caching challenge cards to avoid redundant lookups.
The function calls
_getChallengeCardsForLevelevery time it renders (lines 489-493). When called from the level selection flow, these cards were already retrieved at lines 57-61. While the duplicate lookup isn't critical, storing the current level's challenge cards in a module-level variable would eliminate this redundancy.⚡ Optional: Cache challenge cards
+let currentLevelChallengeCards = []; + function _callbackForInnerMasterOnClickEvent( vulnerableAppEndPointData, id, key, vulnerabilitySelected ) { return function () { if (currentId == id && currentKey == key) { return; } currentId = id; currentKey = key; clearSelectedInnerMaster(); vulnerabilityLevelSelected = vulnerableAppEndPointData[id]["Detailed Information"][key]["Level"]; this.classList.add("active-item"); - let levelChallengeCards = _getChallengeCardsForLevel( + currentLevelChallengeCards = _getChallengeCardsForLevel( vulnerableAppEndPointData, id, key ); - _updateChallengeToggleAvailability(levelChallengeCards); + _updateChallengeToggleAvailability(currentLevelChallengeCards); _renderDetailMode(vulnerableAppEndPointData); // ... rest of function }; } function _renderDetailMode(vulnerableAppEndPointData) { let challengeCardsContainer = document.getElementById("challengeCards"); let scannerHelp = document.getElementById("scannerHelp"); - let challengeCards = _getChallengeCardsForLevel( - vulnerableAppEndPointData, - currentId, - currentKey - ); + let challengeCards = currentLevelChallengeCards; challengeCardsContainer.innerHTML = ""; // ... rest of function }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/resources/static/vulnerableApp.js` around lines 486 - 511, _renderDetailMode calls _getChallengeCardsForLevel on every render; add a module-level cache (e.g., currentChallengeCards with associated keys/currentId and currentKey) and update it when the level selection flow first retrieves cards, then have _renderDetailMode use the cachedChallengeCards if currentId/currentKey match; if cache miss, call _getChallengeCardsForLevel, store the result in the module-level cache, and proceed as before. Ensure the cache is keyed by currentId/currentKey and reference the symbols _renderDetailMode, _getChallengeCardsForLevel, currentId, currentKey, and buildChallengeCards when making the replacement.
397-408: ⚡ Quick winPrevent redundant payload rendering on repeated clicks.
The "Show Payload" button rebuilds the payload DOM on every click. This is wasteful and could confuse users if animations or state exist. Consider disabling the button after the first click or checking if content is already rendered.
⚡ Suggested fix: Disable button after first click
showPayloadBtn.addEventListener("click", function () { payloadEl.innerHTML = ""; let desc = document.createElement("p"); desc.classList.add("challenge-card-payload-description"); desc.textContent = payload["description"] || ""; let value = document.createElement("code"); value.classList.add("challenge-card-payload-value"); value.textContent = payload["value"] || ""; payloadEl.appendChild(desc); payloadEl.appendChild(value); payloadEl.classList.remove("hide-component"); + showPayloadBtn.disabled = true; });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/resources/static/vulnerableApp.js` around lines 397 - 408, The click handler for showPayloadBtn repeatedly rebuilds payloadEl every click; make it idempotent by early-returning if payload is already shown (e.g., check payloadEl.classList.contains("hide-component") or payloadEl.children.length) or by disabling the button after the first render (set showPayloadBtn.disabled = true and/or remove the event listener inside the handler). Update the handler for showPayloadBtn to skip DOM creation when payloadEl is already visible and only append the description/value once (use the existing payload variable and payloadEl), thereby preventing redundant rendering and state/animation duplication.
370-372: 💤 Low valueConsider making hint ordering more explicit.
The sort comparator uses
|| 0as a fallback for missingorderproperties. While this works, it could be clearer about the intent.💡 Optional: More explicit ordering
- let hints = (card["hints"] || []).slice().sort(function (a, b) { - return (a["order"] || 0) - (b["order"] || 0); - }); + let hints = (card["hints"] || []).slice().sort(function (a, b) { + let orderA = typeof a["order"] === "number" ? a["order"] : 0; + let orderB = typeof b["order"] === "number" ? b["order"] : 0; + return orderA - orderB; + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/resources/static/vulnerableApp.js` around lines 370 - 372, The sort comparator for hints is unclear because it uses `|| 0` to fallback when `order` is missing; update the comparator used where `hints` is computed (the slice().sort(...) on card["hints"]) to make ordering explicit: coerce `a.order` and `b.order` to numeric values and use a clear default (for example with nullish coalescing `??` or Number parsing) or extract a small helper like `getOrder(item)` that returns a numeric order defaulting to 0, then sort by `getOrder(a) - getOrder(b)` so missing/invalid orders are handled explicitly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/resources/static/vulnerableApp.js`:
- Line 194: The update() function repeatedly calls
_addingEventListenerToModeToggle which re-registers handlers on the same mode
toggle buttons; modify _addingEventListenerToModeToggle so it guards against
duplicate listeners by either (a) tracking a boolean like
modeToggleListenersAttached and returning early if true, or (b) removing
existing listeners before adding new ones (or use the { once: true } option
where appropriate); ensure the guard references the same DOM selectors used in
_addingEventListenerToModeToggle and set/clear the flag when toggles are
created/destroyed so listeners are only attached once per element.
---
Nitpick comments:
In `@src/main/resources/static/vulnerableApp.js`:
- Around line 486-511: _renderDetailMode calls _getChallengeCardsForLevel on
every render; add a module-level cache (e.g., currentChallengeCards with
associated keys/currentId and currentKey) and update it when the level selection
flow first retrieves cards, then have _renderDetailMode use the
cachedChallengeCards if currentId/currentKey match; if cache miss, call
_getChallengeCardsForLevel, store the result in the module-level cache, and
proceed as before. Ensure the cache is keyed by currentId/currentKey and
reference the symbols _renderDetailMode, _getChallengeCardsForLevel, currentId,
currentKey, and buildChallengeCards when making the replacement.
- Around line 397-408: The click handler for showPayloadBtn repeatedly rebuilds
payloadEl every click; make it idempotent by early-returning if payload is
already shown (e.g., check payloadEl.classList.contains("hide-component") or
payloadEl.children.length) or by disabling the button after the first render
(set showPayloadBtn.disabled = true and/or remove the event listener inside the
handler). Update the handler for showPayloadBtn to skip DOM creation when
payloadEl is already visible and only append the description/value once (use the
existing payload variable and payloadEl), thereby preventing redundant rendering
and state/animation duplication.
- Around line 370-372: The sort comparator for hints is unclear because it uses
`|| 0` to fallback when `order` is missing; update the comparator used where
`hints` is computed (the slice().sort(...) on card["hints"]) to make ordering
explicit: coerce `a.order` and `b.order` to numeric values and use a clear
default (for example with nullish coalescing `??` or Number parsing) or extract
a small helper like `getOrder(item)` that returns a numeric order defaulting to
0, then sort by `getOrder(a) - getOrder(b)` so missing/invalid orders are
handled explicitly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 44368e0c-2e5a-45b9-8410-81cf9b408730
📒 Files selected for processing (4)
src/main/resources/static/index.htmlsrc/main/resources/static/mockAllEndPointJson.jsonsrc/main/resources/static/vulnerableApp.csssrc/main/resources/static/vulnerableApp.js
| }); | ||
| }); | ||
| _addingEventListenerToShowHideHelpButton(vulnerableAppEndPointData); | ||
| _addingEventListenerToModeToggle(vulnerableAppEndPointData); |
There was a problem hiding this comment.
Prevent event listener accumulation.
The function _addingEventListenerToModeToggle is called inside update(), which can be invoked multiple times (e.g., when switching vulnerabilities). Each invocation registers new listeners on the same mode toggle buttons without removing previous ones, causing memory leaks and multiple handler executions per click.
🔧 Recommended fix: Guard against duplicate listeners
Option 1: Use a flag to register listeners only once:
+let modeToggleListenersRegistered = false;
+
function update(vulnerableAppEndPointData) {
const masterItems = document.querySelectorAll(".master-item");
handleElementAutoSelection(vulnerableAppEndPointData, 0);
masterItems.forEach((item) => {
item.addEventListener("click", function () {
clearSelectedMaster();
this.classList.add("active-item");
detail.classList.remove("hidden-md-down");
handleElementAutoSelection(vulnerableAppEndPointData, this.id);
});
});
_addingEventListenerToShowHideHelpButton(vulnerableAppEndPointData);
- _addingEventListenerToModeToggle(vulnerableAppEndPointData);
+ if (!modeToggleListenersRegistered) {
+ _addingEventListenerToModeToggle(vulnerableAppEndPointData);
+ modeToggleListenersRegistered = true;
+ }
}Option 2: Use { once: true } if listeners should only fire once, or move registration outside update() to a one-time initialization block.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/resources/static/vulnerableApp.js` at line 194, The update()
function repeatedly calls _addingEventListenerToModeToggle which re-registers
handlers on the same mode toggle buttons; modify
_addingEventListenerToModeToggle so it guards against duplicate listeners by
either (a) tracking a boolean like modeToggleListenersAttached and returning
early if true, or (b) removing existing listeners before adding new ones (or use
the { once: true } option where appropriate); ensure the guard references the
same DOM selectors used in _addingEventListenerToModeToggle and set/clear the
flag when toggles are created/destroyed so listeners are only attached once per
element.
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
2 participants
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.
This PR introduces a Scanner / Challenge mode toggle to the legacy UI, along with the challenge cards experience for practicing vulnerabilities. The implementation reuses and extends the existing master-detail logic of the legacy UI as much as possible and follows closely the design and behavior described in #570.
What was implemented
Mode toggle (Scanner / Challenge) in the navbar, allowing the user to switch between the existing Scanner view and the new Challenge view.
Challenge cards rendered per vulnerability level, each containing:
ChallengeText).Progressive hint reveal hints are shown one at a time via a "Reveal hint" button.
Gated payload the "Show Payload" button only unlocks after all hints have been revealed.
Notes
The logic was built on top of the existing legacy UI patterns (master-detail, vanilla JS, AJAX), trying to stay as faithful as possible to what was outlined in #570.
A mock fixture (
mockAllEndPointJson.json) was added to support local testing of the toggle and challenge cards. It is not referenced by the HTML, the app still loads the realallEndPointJsonendpoint.Closes #571
Screenshots
Summary by CodeRabbit