Feature/refactor code into folders #53
Conversation
|
""" WalkthroughThis change refactors a monolithic JavaScript file into a modular, folder-based structure, separating features, utilities, and styles into dedicated files and directories. It introduces new CSS files for base, main, and dark themes, modularizes JavaScript by feature and utility, and updates the HTML to reference these new modules, aligning with a clear, maintainable project architecture. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant index.html
participant app.js
participant storage.js
participant dom.js
participant formatter.js
participant compare.js
participant codegen.js
participant convert.js
participant mockgen.js
participant editor.js
User->>index.html: Loads page
index.html->>app.js: Loads main app script
app.js->>storage.js: loadGlobalState()
storage.js-->>app.js: Restores UI state & tabs
app.js->>formatter.js: Initialize formatter tabs (if mode active)
app.js->>compare.js: Initialize compare tabs (if mode active)
app.js->>codegen.js: Initialize codegen tabs (if mode active)
app.js->>convert.js: Initialize convert mode (if mode active)
app.js->>mockgen.js: Initialize mockgen mode (if mode active)
app.js->>editor.js: Initialize editor tabs (if mode active)
User->>app.js: Switches mode / interacts with UI
app.js->>dom.js: UI actions (copy, rename, reorder, modal, dark mode)
app.js->>storage.js: saveGlobalState() on relevant actions
Note over app.js,storage.js: Autosave and state persistence
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate Unit Tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (26)
json-tool/public/assets/css/base.css (3)
1-8: Use a CSS reset and box‐sizing. Without a reset, some elements (lists, headings, form controls) may render inconsistently across browsers. Consider importing a normalize stylesheet or adding at the top:html { box-sizing: border-box; } *, *::before, *::after { box-sizing: inherit; }
2-7: Prefer relative units for spacing. Hard-codedmargin: 20pxand default font sizing can hinder scalability and user-zoom accessibility. Useremor CSS custom properties (e.g.,margin: 1.25rem) to keep spacing consistent.
10-13: Leverage logical properties and variables for headings. Instead ofdisplay: inline-blockandmargin-right, consider usingmargin-inline-endor defining a spacing variable (e.g.,--spacing-lg) to maintain consistency across components.json-tool/public/assets/js/features/formatter/formatter.html (2)
1-4: Avoid inlineonclickhandlers; bind events in JS. Inline attributes mix markup with behavior. Informatter.js, select the button by class or data‐attribute and useaddEventListener('click', addFormatterTab)for cleaner separation and easier testing.
5-8: Enhance accessibility for dynamic content. The container receiving new tab panels should use ARIA roles and live regions, for example:<div id="formatter-tab-contents" role="tablist" aria-live="polite"></div>so screen readers announce updates when tabs are added.
json-tool/public/assets/js/features/codegen/codegen.html (2)
1-4: Use data attributes and JS for event handling. Replaceonclick="addCodegenTab()"with a data attribute (e.g.,data-action="add-tab") and attach the handler incodegen.js. This decouples HTML from behavior and improves maintainability.
5-8: Add ARIA roles for tabs. To support assistive technologies, consider marking the containers:<div id="codegen-tabs-container" class="tabs" role="tablist"></div> <div id="codegen-tab-contents" role="tabpanel" aria-labelledby="..."></div>and dynamically link tabs and panels via
aria-labelledby.json-tool/public/assets/js/features/compare/compare.html (2)
1-4: Decouple markup and script logic. Instead ofonclick="addCompareTab()", assign a data-attribute or class to the button and registeraddCompareTabincompare.jsviaaddEventListener. This simplifies testing and future refactors.
5-8: Improve semantic structure and accessibility. Wrap the tab container in a<section aria-label="Compare Tabs">or addrole="tablist", and ensure dynamically inserted elements have properrole="tab"andaria-selectedattributes.json-tool/public/index.html (2)
9-11: Optimize theme loading. You now load three CSS files. To respect user preferences and reduce render-blocking, consider splittingthemes.cssintodark.cssandlight.cssand using:<link rel="stylesheet" href="assets/css/themes/light.css" media="(prefers-color-scheme: light)" /> <link rel="stylesheet" href="assets/css/themes/dark.css" media="(prefers-color-scheme: dark)" />This avoids flash of incorrect theme.
25-36: Review script loading strategy and module usage. All core and feature scripts load synchronously in the<head>withoutdefer. This can block HTML parsing. Consider:
- Adding
deferto each<script>if they don’t need to run immediately.- Moving scripts to just before
</body>.- Verifying if any of these files use
import/export; if so, mark them withtype="module".These changes improve performance and ensure ES modules work correctly.
json-tool/README.md (1)
11-39: Add language specifier to the code blockThe project structure code block should specify a language for proper syntax highlighting.
-``` +```plaintext json-tool/ ├── public/ │ ├── index.html # Main entry point🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
13-13: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
json-tool/public/assets/js/features/formatter/formatter.js (1)
194-208: Use optional chaining in file upload conditionAs suggested by static analysis, use optional chaining for cleaner code.
- if (inputElement.files && inputElement.files[0]) { + if (inputElement.files?.[0]) {🧰 Tools
🪛 Biome (1.9.4)
[error] 195-195: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
json-tool/public/assets/js/core/json-utils.js (4)
1-11: Enhance error handling in autoFormatTextareaThe function silently ignores invalid JSON input. Consider adding an optional error callback parameter to report validation issues to the user.
-function autoFormatTextarea(textarea) { +function autoFormatTextarea(textarea, onError) { try { const parsed = JSON.parse(textarea.value); textarea.value = JSON.stringify(parsed, null, 2); } catch (e) { - // Do nothing if invalid + // Optional error callback + if (typeof onError === 'function') { + onError(e.message); + } } }
13-107: Clean up unusedfocusedNodevariable in createTreeViewThe function declares
focusedNodebut never uses it. Either utilize it for keyboard navigation or remove it.function createTreeView(data, parentElement) { parentElement.innerHTML = ""; - let focusedNode = null;
122-149: Implement position fixing for tree context menuThe context menu is created with absolute positioning but lacks boundary checks to ensure it stays within the viewport.
function showContextMenu(e, path, value) { const menu = document.createElement("div"); menu.className = "tree-context-menu"; - menu.style.left = `${e.pageX}px`; - menu.style.top = `${e.pageY}px`; + + // Add menu to DOM first so we can calculate its dimensions + document.body.appendChild(menu); + + // Position the menu to ensure it stays within viewport + const rect = menu.getBoundingClientRect(); + const viewportWidth = window.innerWidth; + const viewportHeight = window.innerHeight; + + // Calculate position + let left = e.pageX; + let top = e.pageY; + + // Adjust if menu would go off right edge + if (left + rect.width > viewportWidth) { + left = viewportWidth - rect.width - 5; + } + + // Adjust if menu would go off bottom edge + if (top + rect.height > viewportHeight) { + top = viewportHeight - rect.height - 5; + } + + menu.style.left = `${left}px`; + menu.style.top = `${top}px`;
167-172: Document the orphaned capitalize functionThe
capitalizefunction is defined but not used within this file. Add documentation to clarify its purpose or refactor it to a separate utilities module if used elsewhere./** * Utility function to capitalize the first letter of a string. * This function is used across various features in the application * for presentation formatting. * * @param {string} str - The string to capitalize * @return {string} The capitalized string */ function capitalize(str) { return str.charAt(0).toUpperCase() + str.slice(1); }json-tool/public/assets/js/features/convert/convert.js (1)
16-43: Enhance Python dict conversion with better handling of edge casesThe current implementation uses simple regex replacements which may not handle all Python dictionary syntax features.
Consider enhancing the conversion to handle more Python-specific syntax:
function convert() { const input = document.getElementById("convert-input").value; const output = document.getElementById("convert-output"); try { if (currentConvertMode === "dict-to-json") { // Convert Python dict string → JSON const jsonCompatible = input .replace(/'/g, '"') + // Handle Python tuples by replacing them with arrays + .replace(/\(([^)]*)\)/g, (match, p1) => `[${p1}]`) .replace(/\bTrue\b/g, "true") .replace(/\bFalse\b/g, "false") .replace(/\bNone\b/g, "null"); const parsed = JSON.parse(jsonCompatible); output.textContent = JSON.stringify(parsed, null, 2); } else { // Convert JSON → Python dict string const parsed = JSON.parse(input); let dictStr = JSON.stringify(parsed, null, 2) .replace(/"/g, "'") .replace(/\btrue\b/g, "True") .replace(/\bfalse\b/g, "False") .replace(/\bnull\b/g, "None"); + // Convert empty arrays to empty tuples for Python + dictStr = dictStr.replace(/\[\s*\]/g, "()"); output.textContent = dictStr; } } catch (e) { output.textContent = "Error: " + e.message; } }Also, add a warning about conversion limitations:
+ // Add a note about limitations + const noteElement = document.createElement("div"); + noteElement.className = "conversion-note"; + noteElement.textContent = "Note: This conversion handles basic types but may not support all Python syntax features."; + document.getElementById("convert-section").appendChild(noteElement);json-tool/public/assets/js/app.js (2)
16-47: Refactor switchMode to reduce repetitionThe
switchModefunction contains repetitive code for each mode. Consider refactoring to reduce duplication.function switchMode(mode) { - document.getElementById("formatter-section").style.display = "none"; - document.getElementById("compare-section").style.display = "none"; - document.getElementById("codegen-section").style.display = "none"; - document.getElementById("convert-section").style.display = "none"; - document.getElementById("mockgen-section").style.display = "none"; + // Hide all sections + const sections = ["formatter", "compare", "codegen", "convert", "mockgen"]; + sections.forEach(section => { + document.getElementById(`${section}-section`).style.display = "none"; + }); document .querySelectorAll(".mode-selector button") .forEach((btn) => btn.classList.remove("active")); - if (mode === "formatter") { - document.getElementById("formatter-section").style.display = "block"; - document.getElementById("mode-formatter-btn").classList.add("active"); - } else if (mode === "compare") { - document.getElementById("compare-section").style.display = "block"; - document.getElementById("mode-compare-btn").classList.add("active"); - } else if (mode === "codegen") { - document.getElementById("codegen-section").style.display = "block"; - document.getElementById("mode-codegen-btn").classList.add("active"); - } else if (mode === "convert") { - document.getElementById("convert-section").style.display = "block"; - document.getElementById("mode-convert-btn").classList.add("active"); - } else if (mode === "mockgen") { - document.getElementById("mockgen-section").style.display = "block"; - document.getElementById("mode-mockgen-btn").classList.add("active"); - renderMockgenDocs(); - } + // Show selected section + if (sections.includes(mode)) { + document.getElementById(`${mode}-section`).style.display = "block"; + document.getElementById(`mode-${mode}-btn`).classList.add("active"); + + // Special handling for mockgen mode + if (mode === "mockgen") { + renderMockgenDocs(); + } + } saveGlobalState(); }
68-97: Consider refactoring keyboard shortcuts for better maintainabilityThe keyboard shortcut implementation has repetitive code and condition checks. Consider refactoring to a more maintainable approach.
/* ========== Keyboard Shortcuts ========== */ -document.addEventListener("keydown", (e) => { - if ( - e.ctrlKey && - e.key.toLowerCase() === "t" && - document.getElementById("formatter-section").style.display !== "none" - ) { - e.preventDefault(); - addFormatterTab(); - } - if ( - e.ctrlKey && - e.key.toLowerCase() === "w" && - document.getElementById("formatter-section").style.display !== "none" - ) { - e.preventDefault(); - const activeTab = document.querySelector( - "#formatter-tab-contents .json-tab-content.active" - ); - if (activeTab) closeFormatterTab(activeTab.id); - } - if (e.ctrlKey && (e.key === "/" || e.key === "?")) { - e.preventDefault(); - toggleShortcutModal(); - } - if (e.key === "Escape") { - const modal = document.getElementById("shortcut-modal"); - if (modal.style.display === "block") toggleShortcutModal(); - } -}); +document.addEventListener("keydown", (e) => { + const activeMode = getActiveMode(); + const shortcuts = { + // Global shortcuts + "ctrl+/": () => toggleShortcutModal(), + "ctrl+?": () => toggleShortcutModal(), + "escape": () => { + const modal = document.getElementById("shortcut-modal"); + if (modal.style.display === "block") toggleShortcutModal(); + }, + // Mode-specific shortcuts + "formatter": { + "ctrl+t": () => addFormatterTab(), + "ctrl+w": () => { + const activeTab = document.querySelector( + "#formatter-tab-contents .json-tab-content.active" + ); + if (activeTab) closeFormatterTab(activeTab.id); + } + } + }; + + // Generate key string (e.g., "ctrl+t") + const keyCombo = [ + e.ctrlKey ? "ctrl" : "", + e.shiftKey ? "shift" : "", + e.altKey ? "alt" : "", + e.key.toLowerCase() === " " ? "space" : e.key.toLowerCase() + ].filter(Boolean).join("+"); + + // Execute matching shortcut + if (shortcuts[keyCombo]) { + e.preventDefault(); + shortcuts[keyCombo](); + } else if (shortcuts[activeMode]?.[keyCombo]) { + e.preventDefault(); + shortcuts[activeMode][keyCombo](); + } +});This approach is more declarative and makes it easier to add new shortcuts.
json-tool/public/assets/js/features/compare/compare.js (3)
28-51: Duplicate code between createCompareTab and createCompareTabWithData.There's significant code duplication between
createCompareTab()andcreateCompareTabWithData(). Consider refactoring to extract common code into a helper function to improve maintainability.You could create a helper function like:
+function setupCompareTab(tabId, tabButton, tabName) { + // Common code for creating tab buttons and content + tabButton.className = "tab-button"; + tabButton.setAttribute("data-tab", tabId); + tabButton.onclick = () => switchCompareTab(tabId); + tabButton.innerHTML = `<span class="tab-name">${tabName}</span> + <span class="close-tab" onclick="closeCompareTab('${tabId}', event)">×</span>`; + tabButton.addEventListener("dblclick", () => + openTabRenameTooltip(tabId, "compare") + ); + const tabsContainer = document.getElementById("compare-tabs-container"); + const addButton = tabsContainer.querySelector(".add-tab-button"); + tabsContainer.insertBefore(tabButton, addButton); + + // Create tab content + const tabContent = document.createElement("div"); + tabContent.id = tabId; + tabContent.className = "json-tab-content"; + tabContent.innerHTML = ` + <div style="display:flex; gap:10px;"> + <textarea class="json-input-left" placeholder="Enter Left JSON" style="width:48%; height:200px;"></textarea> + <textarea class="json-input-right" placeholder="Enter Right JSON" style="width:48%; height:200px;"></textarea> + </div> + <button onclick="compareJSONs('${tabId}')">Compare JSONs</button> + <div class="compare-result" style="margin-top:10px;"></div> + `; + document.getElementById("compare-tab-contents").appendChild(tabContent); + return tabContent; +}Then use it in both functions to reduce duplication.
161-162: Simplistic diff algorithm.The current diff detection only identifies if two lines are exactly the same or different. This doesn't highlight the specific differences within each line, which can make it hard to spot small changes.
Consider implementing a more granular diff that highlights specific parts of the line that differ, similar to how git diff works. You could use a library like diff-match-patch or jsdiff.
175-193: Confirm dialog doesn't provide context.The confirmation dialog for closing a tab doesn't indicate which tab is being closed or if there's unsaved work.
- if (!confirm("Are you sure you want to close this tab?")) return; + const tabName = document.querySelector( + `#compare-tabs-container .tab-button[data-tab="${tabId}"] .tab-name` + ).textContent; + if (!confirm(`Are you sure you want to close tab "${tabName}"?`)) return;json-tool/public/assets/js/core/storage.js (1)
14-89: Missing validation for tab data structure.The saveGlobalState function doesn't validate the structure of the data it's collecting, which could lead to errors if the DOM structure changes.
Consider adding validation for the tab data before saving it to localStorage. This would make the code more robust against changes in the DOM structure.
function saveGlobalState() { const state = { darkMode: document.body.classList.contains("dark-mode"), activeMode: getActiveMode(), formatter: { activeTab: document.querySelector( "#formatter-tab-contents .json-tab-content.active" )?.id || "", tabs: [], }, compare: { activeTab: document.querySelector("#compare-tab-contents .json-tab-content.active") ?.id || "", tabs: [], }, codegen: { activeTab: document.querySelector("#codegen-tab-contents .json-tab-content.active") ?.id || "", tabs: [], }, }; + // Validate state before saving + function validateTabData(tab) { + return tab && typeof tab.id === 'string' && tab.id.length > 0; + } // Formatter tabs document .querySelectorAll("#formatter-tabs-container .tab-button[data-tab]") .forEach((btn) => { const tabId = btn.getAttribute("data-tab"); const name = btn.querySelector(".tab-name").textContent; const color = btn.querySelector(".tab-color-picker")?.value || "#e0e0e0"; const content = document.querySelector("#" + tabId + " .json-input")?.value || ""; - state.formatter.tabs.push({ + const tabData = { id: tabId, name, color, content, - }); + }; + if (validateTabData(tabData)) { + state.formatter.tabs.push(tabData); + } });json-tool/public/assets/js/features/codegen/codegen.js (1)
10-51: Duplicate code in tab creation functions.Similar to the compare.js file, there's significant duplication between
createCodegenTab()andcreateCodegenTabWithData().Extract common code into a helper function to improve maintainability, similar to what was suggested for the compare module.
Also applies to: 53-95
json-tool/public/assets/js/features/mockgen/mockgen.js (1)
107-111: Redundant array checks.Multiple functions check if data is an array and has elements in a redundant way.
Simplify the checks:
- if ( - !Array.isArray(data) || - data.length === 0 || - typeof data[0] !== "object" - ) { + if (!Array.isArray(data) || !data.length || typeof data[0] !== "object") {Apply this pattern in other similar checks at lines 156 and 203.
Also applies to: 156-156, 203-203
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
index.js(0 hunks)json-tool/README.md(4 hunks)json-tool/public/assets/css/base.css(1 hunks)json-tool/public/assets/css/main.css(6 hunks)json-tool/public/assets/css/themes/themes.css(1 hunks)json-tool/public/assets/js/app.js(1 hunks)json-tool/public/assets/js/core/dom.js(1 hunks)json-tool/public/assets/js/core/json-utils.js(1 hunks)json-tool/public/assets/js/core/storage.js(1 hunks)json-tool/public/assets/js/features/codegen/codegen.html(1 hunks)json-tool/public/assets/js/features/codegen/codegen.js(1 hunks)json-tool/public/assets/js/features/compare/compare.html(1 hunks)json-tool/public/assets/js/features/compare/compare.js(1 hunks)json-tool/public/assets/js/features/convert/convert.js(1 hunks)json-tool/public/assets/js/features/formatter/formatter.html(1 hunks)json-tool/public/assets/js/features/formatter/formatter.js(1 hunks)json-tool/public/assets/js/features/mockgen/mockgen.js(1 hunks)json-tool/public/index.html(3 hunks)
💤 Files with no reviewable changes (1)
- index.js
🧰 Additional context used
🧬 Code Graph Analysis (2)
json-tool/public/assets/js/app.js (1)
json-tool/public/assets/js/features/mockgen/mockgen.js (1)
mode(83-85)
json-tool/public/assets/js/core/storage.js (3)
json-tool/public/assets/js/features/codegen/codegen.js (4)
tabId(12-12)tabId(55-55)lang(117-117)codegenTabCount(2-2)json-tool/public/assets/js/features/compare/compare.js (3)
tabId(12-12)tabId(56-56)compareTabCount(2-2)json-tool/public/assets/js/features/formatter/formatter.js (4)
tabId(14-14)content(162-162)content(212-212)formatterTabCount(4-4)
🪛 markdownlint-cli2 (0.17.2)
json-tool/README.md
13-13: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
🪛 LanguageTool
json-tool/README.md
[uncategorized] ~95-~95: Loose punctuation mark.
Context: ... - Core Utilities - storage.js: Local storage and state management - ...
(UNLIKELY_OPENING_PUNCTUATION)
🪛 Biome (1.9.4)
json-tool/public/assets/js/features/formatter/formatter.js
[error] 21-21: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 24-24: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 74-74: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 195-195: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
json-tool/public/assets/js/features/codegen/codegen.js
[error] 161-161: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
[error] 194-194: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
[error] 232-232: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
🔇 Additional comments (28)
json-tool/public/index.html (1)
7-8: Verify the favicon path. Thehref="assets/json.svg"must point to the actual location ofjson.svgunderpublic/. If the file was relocated during the refactor, update this path; otherwise browsers will 404.json-tool/public/assets/css/themes/themes.css (5)
1-11: Well-organized dark mode styling for tabsThe tab button styling for dark mode is clean and provides good visual contrast between normal and active states.
12-26: Consistent dark mode base stylingThe basic dark mode styling establishes a good foundation with appropriate background and text colors.
28-51: Good form element styling for dark modeThe styling for inputs, search containers, and tooltips maintains consistency with the dark theme.
53-65: Clear button state handling for both themesGood approach using both
.dark-modeand:not(.dark-mode)selectors to ensure proper button styling in both themes.
67-110: Comprehensive dark mode styling for complex componentsThe dark mode styling for context menus, tables, and markdown previews is thorough and maintains visual consistency.
json-tool/README.md (3)
41-75: Comprehensive feature documentationThe expanded feature list clearly describes all capabilities of the JSON tool, which aligns well with the modular restructuring of the codebase.
91-111: Well-structured architecture documentationThe architecture section provides a clear overview of how the application is organized, matching the PR objectives of restructuring the code into modular components.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~95-~95: Loose punctuation mark.
Context: ... - Core Utilities -storage.js: Local storage and state management - ...(UNLIKELY_OPENING_PUNCTUATION)
112-122: Updated planned featuresThe planned features list has been expanded with new items that align with the more modular architecture.
json-tool/public/assets/css/main.css (4)
52-52: Improved transition propertiesCombined transitions for more fluid UI element animations is a good improvement.
171-211: Enhanced tree view visualizationThe tree view styling has been significantly improved with:
- Better visual hierarchy using position relative and vertical connector lines
- Cleaner toggler arrow implementation using ::before pseudo-element
- Added keyboard accessibility with focus styling
This is a great improvement to both visual design and accessibility.
213-223: Clean removal of deprecated stylesGood practice keeping commented code to document what was replaced, while still cleaning up the file.
337-348: Improved CSS organizationCondensing the type color classes improves readability while maintaining the same functionality.
json-tool/public/assets/js/core/dom.js (3)
4-45: Well-implemented tab reordering functionalityThe tab reordering implementation is robust with proper event handling for all drag operations.
47-83: Mode-aware tab renaming implementationThe tab renaming functionality is well-structured with mode-specific container selection and proper event handling.
85-107: Clean utility functions implementationThe clipboard and file download utilities are well-implemented with proper error handling and resource cleanup.
json-tool/public/assets/js/features/formatter/formatter.js (6)
4-10: LGTM - Basic formatter tab setup implemented correctlyThe implementation for initializing tabs and the
addFormatterTabfunction look good. The function properly creates a new tab, switches to it, and saves the application state.
87-99: LGTM - Tab switching works correctlyThe tab switching implementation correctly handles toggling the active state between tabs.
126-142: LGTM - Preview tab switching is correctly implementedThe implementation for switching between different preview types (raw, tree, error) works as expected.
210-224: LGTM - File download implementation is correctThe download functionality is well-implemented using the standard approach with
URL.createObjectURL. Good job properly cleaning up withURL.revokeObjectURLafterward.
226-244: LGTM - Tab closing works correctlyThe tab closing functionality is well-implemented with proper event handling, user confirmation, and switching to another tab when needed.
246-249: LGTM - Copy functionality is clean and conciseThe implementation correctly finds the raw JSON content and copies it to clipboard.
json-tool/public/assets/js/features/convert/convert.js (2)
1-14: LGTM - Convert direction switching is well implementedThe implementation for switching between conversion directions is clear and appropriately resets input/output fields.
45-48: LGTM - Copy output functionality works as expectedThe copying functionality is concise and uses the shared clipboard utility correctly.
json-tool/public/assets/js/app.js (4)
1-14: LGTM - Active mode detection works correctlyThe function correctly determines the active mode based on section visibility.
49-66: LGTM - Theme toggling with view update is well implementedThe shortcut modal toggle and dark mode functionality with conditional diff preview updates work correctly.
99-115: LGTM - Initialization with default tabs is properly implementedThe initialization code correctly loads the global state and creates default tabs when needed.
41-44:✅ Verification successful
Verify renderMockgenDocs function existence and behavior
The
renderMockgenDocs()function is called when switching to mockgen mode, but might depend on a state that needs to be determined first.Based on the relevant code snippet, the mockgen module appears to rely on a radio button selection to determine mode. Ensure that
renderMockgenDocs()correctly handles the initial state before any radio buttons are selected.
🏁 Script executed:
#!/bin/bash # Find the renderMockgenDocs function implementation echo "Searching for renderMockgenDocs function..." rg -A 10 "function renderMockgenDocs" --type jsLength of output: 492
#!/bin/bash
Display the mockgen.js file to determine how mockgenDocs is initialized and used
sed -n '1,200p' json-tool/public/assets/js/features/mockgen/mockgen.js
#!/bin/bash
Check where and how mockgenDocs is defined in mockgen.js
echo "Inspecting mockgenDocs declaration and initialization..."
rg -C 5 "mockgenDocs" json-tool/public/assets/js/features/mockgen/mockgen.jsExamine the click handler in app.js to see mode switching logic
echo "Inspecting mode-mockgen-btn click handler in app.js..."
rg -C 5 "mode-mockgen-btn" json-tool/public/assets/js/app.js
🏁 Script executed:
#!/bin/bash # Search for mockgenDocs references and initialization in mockgen.js rg -C5 "mockgenDocs" json-tool/public/assets/js/features/mockgen/mockgen.js # Inspect the mockgen mode switch handler in app.js rg -C5 "mode-mockgen-btn" json-tool/public/assets/js/app.jsLength of output: 869
renderMockgenDocs function validated
The
renderMockgenDocs()implementation lives in
json-tool/public/assets/js/features/mockgen/mockgen.jsand simply renders the constantmockgenDocsviamarked.parse(). It does not rely on any additional UI state beyond the presence of the target element, so invoking it immediately on “mockgen” mode switch is safe.No changes are required here.
json-tool/public/index.html
Outdated
| <input | ||
| type="text" | ||
| id="userInput" | ||
| placeholder="File Download Name(optional)" | ||
| /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance the download‐name input’s semantics and accessibility. The id="userInput" is too generic and lacks a <label>. Rename to something like mock-download-filename, add a <label for="mock-download-filename">Download Filename (optional):</label>, and include a name attribute. For example:
- <input type="text" id="userInput" placeholder="File Download Name(optional)" />
+ <label for="mock-download-filename">Download Filename (optional):</label>
+ <input id="mock-download-filename" name="downloadName" type="text"
+ placeholder="e.g., data.json" />This improves clarity and screen-reader support.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <input | |
| type="text" | |
| id="userInput" | |
| placeholder="File Download Name(optional)" | |
| /> | |
| <label for="mock-download-filename">Download Filename (optional):</label> | |
| <input | |
| id="mock-download-filename" | |
| name="downloadName" | |
| type="text" | |
| placeholder="e.g., data.json" | |
| /> |
🤖 Prompt for AI Agents (early access)
In json-tool/public/index.html around lines 181 to 185, the input element for
the download name uses a generic id "userInput" and lacks an associated label,
which reduces accessibility. Rename the id to "mock-download-filename", add a
corresponding <label> element with the text "Download Filename (optional):"
linked via the for attribute, and include a name attribute on the input element
to improve semantics and screen-reader support.
|
|
||
| if (draggedBtn && draggedBtn !== button) { | ||
| container.insertBefore(draggedBtn, button); | ||
| saveGlobalState(); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify saveGlobalState implementation
The function calls saveGlobalState() which isn't defined in this file. Ensure this function is properly imported or defined in the global scope.
Also applies to: 81-81
🏁 Script executed:
#!/bin/bash
# Search for saveGlobalState definition/import
echo "Searching for saveGlobalState definition..."
rg "function saveGlobalState|const saveGlobalState|let saveGlobalState|var saveGlobalState" -A 3
echo -e "\nSearching for saveGlobalState import..."
rg "import.*saveGlobalState|require.*saveGlobalState"Length of output: 659
Verify saveGlobalState availability
saveGlobalState is defined in json-tool/public/assets/js/core/storage.js but is not imported in dom.js. You have two options to ensure it’s available when you call it in json-tool/public/assets/js/core/dom.js (both at lines 41 and 81):
• If you rely on global scripts, make sure in your HTML that storage.js is loaded before dom.js.
• If you’re using ES modules, update storage.js to export the function:
// storage.js
export function saveGlobalState() { … }and at the top of dom.js import it:
// dom.js
import { saveGlobalState } from './storage.js';🤖 Prompt for AI Agents (early access)
In json-tool/public/assets/js/core/dom.js at lines 41 and 81, the function
saveGlobalState is called but not defined or imported in this file. To fix this,
either ensure that storage.js (where saveGlobalState is defined) is loaded
before dom.js in the HTML if using global scripts, or if using ES modules,
export saveGlobalState from storage.js and import it at the top of dom.js with
"import { saveGlobalState } from './storage.js';".
| function updateFormatterTabColor(tabId, colorValue) { | ||
| // If needed, update visual indicators here. | ||
| saveGlobalState(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement the updateFormatterTabColor function
The updateFormatterTabColor function is nearly empty with just a comment. Consider implementing proper functionality or documenting why it's not needed.
function updateFormatterTabColor(tabId, colorValue) {
- // If needed, update visual indicators here.
+ // Update the tab button color
+ const tabButton = document.querySelector(
+ `#formatter-tabs-container .tab-button[data-tab="${tabId}"]`
+ );
+ if (tabButton) {
+ tabButton.style.backgroundColor = colorValue;
+ }
saveGlobalState();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function updateFormatterTabColor(tabId, colorValue) { | |
| // If needed, update visual indicators here. | |
| saveGlobalState(); | |
| } | |
| function updateFormatterTabColor(tabId, colorValue) { | |
| // Update the tab button color | |
| const tabButton = document.querySelector( | |
| `#formatter-tabs-container .tab-button[data-tab="${tabId}"]` | |
| ); | |
| if (tabButton) { | |
| tabButton.style.backgroundColor = colorValue; | |
| } | |
| saveGlobalState(); | |
| } |
🤖 Prompt for AI Agents (early access)
In json-tool/public/assets/js/features/formatter/formatter.js around lines 189
to 192, the updateFormatterTabColor function is currently a stub with only a
comment and a call to saveGlobalState. Implement this function to update the
visual color indicator of the formatter tab identified by tabId using the
provided colorValue, such as by modifying the tab's DOM element style or class.
If no visual update is required, add a clear comment explaining why the function
is intentionally left minimal.
| function updateFormatterPreview(tabId) { | ||
| const tabContent = document.getElementById(tabId); | ||
| const textarea = tabContent.querySelector(".json-input"); | ||
| const rawPreview = tabContent.querySelector(".raw-json"); | ||
| const errorMessage = tabContent.querySelector(".error-message"); | ||
| try { | ||
| const parsed = JSON.parse(textarea.value); | ||
| const formatted = JSON.stringify(parsed, null, 2); | ||
| rawPreview.textContent = formatted; | ||
| createTreeView(parsed, tabContent.querySelector(".tree-view")); | ||
| errorMessage.textContent = ""; | ||
| showFormatterPreviewTab(tabId, "raw"); | ||
| textarea.value = formatted; | ||
| } catch (e) { | ||
| errorMessage.textContent = "Error: " + e.message; | ||
| showFormatterPreviewTab(tabId, "error"); | ||
| } | ||
| document.querySelectorAll(".tree-key").forEach((key) => { | ||
| key.addEventListener("focus", () => { | ||
| key.scrollIntoView({ block: "nearest", behavior: "smooth" }); | ||
| }); | ||
| }); | ||
| saveGlobalState(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve event handler management for tree keys
The function adds focus event listeners to all .tree-key elements every time it's called, which could create duplicate listeners and impact performance.
- document.querySelectorAll(".tree-key").forEach((key) => {
- key.addEventListener("focus", () => {
- key.scrollIntoView({ block: "nearest", behavior: "smooth" });
- });
- });
+ // Add event delegation instead of multiple event listeners
+ document.addEventListener("focus", (e) => {
+ if (e.target.classList.contains("tree-key")) {
+ e.target.scrollIntoView({ block: "nearest", behavior: "smooth" });
+ }
+ }, true);This event delegation approach should be implemented once at initialization rather than on every preview update.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents (early access)
In json-tool/public/assets/js/features/formatter/formatter.js around lines 101
to 124, the updateFormatterPreview function adds focus event listeners to all
.tree-key elements every time it runs, causing duplicate listeners and
performance issues. To fix this, remove the event listener attachment from this
function and instead implement a single event delegation for focus events on
.tree-key elements once during the initial setup or page load, ensuring
listeners are not repeatedly added.
| function searchFormatterJSON(tabId) { | ||
| const tabContent = document.getElementById(tabId); | ||
| const searchInput = tabContent | ||
| .querySelector(".search-input") | ||
| .value.trim() | ||
| .toLowerCase(); | ||
| const rawPreview = tabContent.querySelector(".raw-json"); | ||
| const treeView = tabContent.querySelector(".tree-view"); | ||
| tabContent.querySelectorAll(".highlight").forEach((el) => { | ||
| const parent = el.parentNode; | ||
| parent.replaceChild(document.createTextNode(el.textContent), el); | ||
| }); | ||
| if (!searchInput) return; | ||
| const regex = new RegExp( | ||
| `(${searchInput.replace(/[.*+?^${}()|[\]\\]/g, "\\$&")})`, | ||
| "gi" | ||
| ); | ||
| if (rawPreview.classList.contains("active")) { | ||
| const content = rawPreview.textContent; | ||
| rawPreview.innerHTML = content.replace( | ||
| regex, | ||
| '<span class="highlight">$1</span>' | ||
| ); | ||
| } | ||
| if (treeView.classList.contains("active")) { | ||
| function highlightNode(node) { | ||
| if (node.nodeType === Node.TEXT_NODE) { | ||
| const matches = node.nodeValue.match(regex); | ||
| if (matches) { | ||
| const span = document.createElement("span"); | ||
| span.innerHTML = node.nodeValue.replace( | ||
| regex, | ||
| '<span class="highlight">$1</span>' | ||
| ); | ||
| node.parentNode.replaceChild(span, node); | ||
| } | ||
| } else if (node.nodeType === Node.ELEMENT_NODE && node.childNodes) { | ||
| node.childNodes.forEach((child) => highlightNode(child)); | ||
| } | ||
| } | ||
| treeView.childNodes.forEach((child) => highlightNode(child)); | ||
| } | ||
| saveGlobalState(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider security improvements in search functionality
When implementing search highlighting, the code uses innerHTML with user input which could potentially introduce XSS vulnerabilities, even with the regex escape.
Consider using a safer approach with textContent and DOM manipulation:
- rawPreview.innerHTML = content.replace(
- regex,
- '<span class="highlight">$1</span>'
- );
+ // Safer approach using DOM methods
+ const parts = content.split(regex);
+ rawPreview.textContent = ''; // Clear existing content
+ parts.forEach((part, i) => {
+ rawPreview.appendChild(document.createTextNode(part));
+ if (i < parts.length - 1) {
+ const highlight = document.createElement('span');
+ highlight.className = 'highlight';
+ // Re-find the matched text to preserve case
+ const match = content.substring(
+ content.indexOf(part) + part.length,
+ content.indexOf(parts[i + 1])
+ );
+ highlight.textContent = match;
+ rawPreview.appendChild(highlight);
+ }
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function searchFormatterJSON(tabId) { | |
| const tabContent = document.getElementById(tabId); | |
| const searchInput = tabContent | |
| .querySelector(".search-input") | |
| .value.trim() | |
| .toLowerCase(); | |
| const rawPreview = tabContent.querySelector(".raw-json"); | |
| const treeView = tabContent.querySelector(".tree-view"); | |
| tabContent.querySelectorAll(".highlight").forEach((el) => { | |
| const parent = el.parentNode; | |
| parent.replaceChild(document.createTextNode(el.textContent), el); | |
| }); | |
| if (!searchInput) return; | |
| const regex = new RegExp( | |
| `(${searchInput.replace(/[.*+?^${}()|[\]\\]/g, "\\$&")})`, | |
| "gi" | |
| ); | |
| if (rawPreview.classList.contains("active")) { | |
| const content = rawPreview.textContent; | |
| rawPreview.innerHTML = content.replace( | |
| regex, | |
| '<span class="highlight">$1</span>' | |
| ); | |
| } | |
| if (treeView.classList.contains("active")) { | |
| function highlightNode(node) { | |
| if (node.nodeType === Node.TEXT_NODE) { | |
| const matches = node.nodeValue.match(regex); | |
| if (matches) { | |
| const span = document.createElement("span"); | |
| span.innerHTML = node.nodeValue.replace( | |
| regex, | |
| '<span class="highlight">$1</span>' | |
| ); | |
| node.parentNode.replaceChild(span, node); | |
| } | |
| } else if (node.nodeType === Node.ELEMENT_NODE && node.childNodes) { | |
| node.childNodes.forEach((child) => highlightNode(child)); | |
| } | |
| } | |
| treeView.childNodes.forEach((child) => highlightNode(child)); | |
| } | |
| saveGlobalState(); | |
| } | |
| function searchFormatterJSON(tabId) { | |
| const tabContent = document.getElementById(tabId); | |
| const searchInput = tabContent | |
| .querySelector(".search-input") | |
| .value.trim() | |
| .toLowerCase(); | |
| const rawPreview = tabContent.querySelector(".raw-json"); | |
| const treeView = tabContent.querySelector(".tree-view"); | |
| // Remove any existing highlights | |
| tabContent.querySelectorAll(".highlight").forEach((el) => { | |
| const parent = el.parentNode; | |
| parent.replaceChild(document.createTextNode(el.textContent), el); | |
| }); | |
| if (!searchInput) return; | |
| const regex = new RegExp( | |
| `(${searchInput.replace(/[.*+?^${}()|[\]\\]/g, "\\$&")})`, | |
| "gi" | |
| ); | |
| if (rawPreview.classList.contains("active")) { | |
| const content = rawPreview.textContent; | |
| // Safer approach using DOM methods | |
| const parts = content.split(regex); | |
| rawPreview.textContent = ""; // Clear existing content | |
| parts.forEach((part, i) => { | |
| // Always append the plain text part | |
| rawPreview.appendChild(document.createTextNode(part)); | |
| // If there's a next part, it's a matched segment | |
| if (i < parts.length - 1) { | |
| const highlight = document.createElement("span"); | |
| highlight.className = "highlight"; | |
| // Re-find the matched text to preserve original casing | |
| const start = content.indexOf(part, (i === 0 ? 0 : undefined)) + part.length; | |
| const end = start + parts[i + 1].length; | |
| const match = content.slice(start, end); | |
| highlight.textContent = match; | |
| rawPreview.appendChild(highlight); | |
| } | |
| }); | |
| } | |
| if (treeView.classList.contains("active")) { | |
| function highlightNode(node) { | |
| if (node.nodeType === Node.TEXT_NODE) { | |
| const matches = node.nodeValue.match(regex); | |
| if (matches) { | |
| const span = document.createElement("span"); | |
| span.innerHTML = node.nodeValue.replace( | |
| regex, | |
| '<span class="highlight">$1</span>' | |
| ); | |
| node.parentNode.replaceChild(span, node); | |
| } | |
| } else if (node.nodeType === Node.ELEMENT_NODE) { | |
| node.childNodes.forEach((child) => highlightNode(child)); | |
| } | |
| } | |
| treeView.childNodes.forEach((child) => highlightNode(child)); | |
| } | |
| saveGlobalState(); | |
| } |
🤖 Prompt for AI Agents (early access)
In json-tool/public/assets/js/features/formatter/formatter.js between lines 144
and 187, the search highlighting uses innerHTML with user input, risking XSS
vulnerabilities despite regex escaping. To fix this, replace innerHTML usage
with safer DOM manipulation by creating text nodes and span elements manually.
For each match, split the text node and insert a span with the highlight class
around the matched text using textContent, avoiding direct HTML injection.
| </div> | ||
| <pre class="code-output" style="margin-top:10px; overflow:auto;"></pre> | ||
| `; |
There was a problem hiding this comment.
Missing Copy button in createCodegenTabWithData.
The createCodegenTabWithData() function doesn't include the "Copy Code" button that's present in createCodegenTab().
<button onclick="generateCode('${tabId}')">Generate Code</button>
+ <button class="copy-button" onclick="copyCodeOutput('${tabId}')">Copy Code</button>
</div>Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents (early access)
In json-tool/public/assets/js/features/codegen/codegen.js around lines 81 to 83,
the createCodegenTabWithData() function is missing the "Copy Code" button that
exists in createCodegenTab(). To fix this, add the HTML and event handler code
for the "Copy Code" button inside the createCodegenTabWithData() function's
returned markup, ensuring it matches the implementation in createCodegenTab()
for consistent UI and functionality.
| } else if (Array.isArray(value)) { | ||
| if (value.length > 0) { | ||
| const elem = value[0]; | ||
| if (typeof elem === "object" && elem !== null) { | ||
| const subInterface = interfaceName + capitalize(key); | ||
| result += ` ${key}: ${subInterface}[];\n`; | ||
| result += generateTypeScript(elem, subInterface); | ||
| } else { | ||
| result += ` ${key}: ${typeof elem}[];\n`; | ||
| } | ||
| } else { | ||
| result += ` ${key}: any[];\n`; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improvement needed in TypeScript array type generation.
The TypeScript code generator only looks at the first element of an array to determine the type, which could lead to incorrect types if the array has mixed types.
Consider inspecting all array elements to determine a more accurate type:
} else if (Array.isArray(value)) {
if (value.length > 0) {
- const elem = value[0];
- if (typeof elem === "object" && elem !== null) {
- const subInterface = interfaceName + capitalize(key);
- result += ` ${key}: ${subInterface}[];\n`;
- result += generateTypeScript(elem, subInterface);
- } else {
- result += ` ${key}: ${typeof elem}[];\n`;
- }
+ // Check for mixed types in array
+ const types = new Set(value.map(item => typeof item));
+ const hasObjects = value.some(item => typeof item === "object" && item !== null);
+
+ if (types.size === 1 && hasObjects) {
+ // All items are objects of the same type
+ const subInterface = interfaceName + capitalize(key);
+ result += ` ${key}: ${subInterface}[];\n`;
+ result += generateTypeScript(value[0], subInterface);
+ } else if (types.size === 1) {
+ // All items are primitives of the same type
+ result += ` ${key}: ${Array.from(types)[0]}[];\n`;
+ } else {
+ // Mixed types
+ result += ` ${key}: any[];\n`;
+ }
} else {
result += ` ${key}: any[];\n`;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } else if (Array.isArray(value)) { | |
| if (value.length > 0) { | |
| const elem = value[0]; | |
| if (typeof elem === "object" && elem !== null) { | |
| const subInterface = interfaceName + capitalize(key); | |
| result += ` ${key}: ${subInterface}[];\n`; | |
| result += generateTypeScript(elem, subInterface); | |
| } else { | |
| result += ` ${key}: ${typeof elem}[];\n`; | |
| } | |
| } else { | |
| result += ` ${key}: any[];\n`; | |
| } | |
| } else if (Array.isArray(value)) { | |
| if (value.length > 0) { | |
| // Check for mixed types in array | |
| const types = new Set(value.map(item => typeof item)); | |
| const hasObjects = value.some(item => typeof item === "object" && item !== null); | |
| if (types.size === 1 && hasObjects) { | |
| // All items are objects of the same type | |
| const subInterface = interfaceName + capitalize(key); | |
| result += ` ${key}: ${subInterface}[];\n`; | |
| result += generateTypeScript(value[0], subInterface); | |
| } else if (types.size === 1) { | |
| // All items are primitives of the same type | |
| result += ` ${key}: ${Array.from(types)[0]}[];\n`; | |
| } else { | |
| // Mixed types | |
| result += ` ${key}: any[];\n`; | |
| } | |
| } else { | |
| result += ` ${key}: any[];\n`; | |
| } |
🤖 Prompt for AI Agents (early access)
In json-tool/public/assets/js/features/codegen/codegen.js around lines 165 to
177, the code currently infers the TypeScript array type by only inspecting the
first element, which can cause incorrect typing for arrays with mixed element
types. To fix this, modify the logic to iterate over all elements in the array,
collect their types, and determine a unified type that accurately represents all
elements. Then generate the TypeScript array type based on this combined type
instead of just the first element.
| function renderMockgenDocs() { | ||
| document.getElementById("mockgen-docs-preview").innerHTML = | ||
| marked.parse(mockgenDocs); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
No error handling for marked library.
The renderMockgenDocs() function doesn't handle potential errors from the marked library.
function renderMockgenDocs() {
- document.getElementById("mockgen-docs-preview").innerHTML =
- marked.parse(mockgenDocs);
+ try {
+ const docsPreview = document.getElementById("mockgen-docs-preview");
+ if (docsPreview) {
+ docsPreview.innerHTML = marked.parse(mockgenDocs);
+ }
+ } catch (e) {
+ console.error("Error rendering markdown:", e);
+ const docsPreview = document.getElementById("mockgen-docs-preview");
+ if (docsPreview) {
+ docsPreview.innerHTML = "<p>Error rendering documentation. Please try again.</p>";
+ }
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function renderMockgenDocs() { | |
| document.getElementById("mockgen-docs-preview").innerHTML = | |
| marked.parse(mockgenDocs); | |
| } | |
| function renderMockgenDocs() { | |
| try { | |
| const docsPreview = document.getElementById("mockgen-docs-preview"); | |
| if (docsPreview) { | |
| docsPreview.innerHTML = marked.parse(mockgenDocs); | |
| } | |
| } catch (e) { | |
| console.error("Error rendering markdown:", e); | |
| const docsPreview = document.getElementById("mockgen-docs-preview"); | |
| if (docsPreview) { | |
| docsPreview.innerHTML = "<p>Error rendering documentation. Please try again.</p>"; | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents (early access)
In json-tool/public/assets/js/features/mockgen/mockgen.js around lines 267 to
270, the renderMockgenDocs function calls marked.parse without error handling.
Wrap the call to marked.parse in a try-catch block to catch any exceptions
thrown by the marked library. In the catch block, handle the error gracefully,
for example by logging the error and displaying a fallback message or empty
content in the "mockgen-docs-preview" element.
| let userInputValue = ""; // This variable will store filename input from user using the following event listener: | ||
|
|
||
| document.getElementById("userInput").addEventListener("input", function (e) { | ||
| userInputValue = e.target.value; | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Global variable and event listener outside of load context.
The event listener setup is placed directly in the global scope rather than in an initialization function.
Move the event listener setup to a proper initialization function that can be called when the page loads:
-let userInputValue = ""; // This variable will store filename input from user using the following event listener:
-
-document.getElementById("userInput").addEventListener("input", function (e) {
- userInputValue = e.target.value;
-});
+let userInputValue = "";
+
+function initializeMockgen() {
+ // Set up event listeners
+ const userInput = document.getElementById("userInput");
+ if (userInput) {
+ userInput.addEventListener("input", function (e) {
+ userInputValue = e.target.value;
+ });
+ }
+
+ // Other initialization code...
+}
+
+// Call this from your main app initialization📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let userInputValue = ""; // This variable will store filename input from user using the following event listener: | |
| document.getElementById("userInput").addEventListener("input", function (e) { | |
| userInputValue = e.target.value; | |
| }); | |
| let userInputValue = ""; | |
| function initializeMockgen() { | |
| // Set up event listeners | |
| const userInput = document.getElementById("userInput"); | |
| if (userInput) { | |
| userInput.addEventListener("input", function (e) { | |
| userInputValue = e.target.value; | |
| }); | |
| } | |
| // Other initialization code... | |
| } | |
| // Call this from your main app initialization |
🤖 Prompt for AI Agents (early access)
In json-tool/public/assets/js/features/mockgen/mockgen.js around lines 179 to
183, the event listener for the "userInput" element is set up directly in the
global scope. To fix this, move the event listener setup inside a dedicated
initialization function, such as initUserInputListener, and call this function
when the page loads to ensure proper timing and avoid polluting the global
scope.
| const [min, max] = schema.split("|")[1].split("-").map(Number); | ||
| return faker.number.int({ min, max }); | ||
| } | ||
|
|
||
| const fakerFn = schema.split("."); | ||
| let val = faker; | ||
| for (const part of fakerFn) { | ||
| val = val?.[part]; | ||
| } | ||
| if (typeof val === "function") return val(); | ||
| throw new Error(`Invalid faker path: "${schema}"`); | ||
| } else { |
There was a problem hiding this comment.
Unsafe usage of faker library.
The code uses the path traversal pattern to access faker functions, which may cause errors if the path doesn't exist.
Add more robust error checking when accessing faker functions:
const fakerFn = schema.split(".");
let val = faker;
for (const part of fakerFn) {
- val = val?.[part];
+ if (val && part in val) {
+ val = val[part];
+ } else {
+ throw new Error(`Invalid faker path: Part "${part}" not found in "${schema}"`);
+ }
}
- if (typeof val === "function") return val();
- throw new Error(`Invalid faker path: "${schema}"`);
+ if (typeof val === "function") {
+ try {
+ return val();
+ } catch (e) {
+ throw new Error(`Error executing faker function "${schema}": ${e.message}`);
+ }
+ }
+ throw new Error(`"${schema}" is not a function in faker`);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const [min, max] = schema.split("|")[1].split("-").map(Number); | |
| return faker.number.int({ min, max }); | |
| } | |
| const fakerFn = schema.split("."); | |
| let val = faker; | |
| for (const part of fakerFn) { | |
| val = val?.[part]; | |
| } | |
| if (typeof val === "function") return val(); | |
| throw new Error(`Invalid faker path: "${schema}"`); | |
| } else { | |
| const fakerFn = schema.split("."); | |
| let val = faker; | |
| for (const part of fakerFn) { | |
| if (val && part in val) { | |
| val = val[part]; | |
| } else { | |
| throw new Error(`Invalid faker path: Part "${part}" not found in "${schema}"`); | |
| } | |
| } | |
| if (typeof val === "function") { | |
| try { | |
| return val(); | |
| } catch (e) { | |
| throw new Error(`Error executing faker function "${schema}": ${e.message}`); | |
| } | |
| } | |
| throw new Error(`"${schema}" is not a function in faker`); |
🤖 Prompt for AI Agents (early access)
In json-tool/public/assets/js/features/mockgen/mockgen.js around lines 66 to 77,
the code accesses faker functions using a path traversal pattern without
verifying each step exists, which can cause runtime errors. To fix this, add
checks to ensure each part of the faker path exists before accessing it, and
handle cases where the path is invalid by throwing a clear error. This will make
the access to faker functions more robust and prevent undefined errors.
|
@Angekarara : Can you address the conflicts and lgtm but could you address conflicts? Thank you so much for your time!! Your contributions matters!! Looking forward for more contributions |
Hi @shravan20 , Thanks for your feedback! I’ll start working on resolving the conflicts and update the PR soon. |
Hello @shravan20 , Thanks you for your feedback once again. I've resolved the conflicts, let me know when you spot anything else. Thanks! |
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (1)
json-tool/public/index.html (1)
53-77: Make sidebar items keyboard-navigable
<div class="feature-item" onclick="…">is not focusable and lacks ARIA roles.
Replace with a<button>(or addrole="button"+tabindex="0"and
keydownhandler) to improve accessibility and comply with WCAG.
♻️ Duplicate comments (7)
json-tool/public/index.html (1)
218-222: Input element still uses generic iduserInputwithout a label (see previous review)json-tool/public/assets/js/features/formatter/formatter.js (4)
124-129: Repeated listener attachment on every preview updateAdding
focuslisteners to all.tree-keynodes each timeupdateFormatterPreviewruns will leak listeners and hurt performance.
Use event delegation or attach once during tree construction.
168-172: Search highlighting still usesinnerHTMLwith user inputDirectly injecting the regex-replaced string keeps an XSS vector open.
Switch to DOM-based highlighting or sanitise the string before assignment.
195-197:updateFormatterTabColorremains a stubFunction still does nothing beyond saving state; implement the visual update or document why it’s unnecessary.
56-74: Duplicate preview sections with identical IDs still presentThe two blocks create the same
*-raw-previewand*-tree-previewelements, causing ID collisions and rendering issues.
Please remove one block or generate unique IDs.json-tool/public/assets/js/features/codegen/codegen.js (2)
70-82: “Copy Code” button still absent in restored tabs
createCodegenTabWithData()omits the copy button present in new tabs, leading to inconsistent UX.
175-184: Array type inference still only checks first elementThe TypeScript generator looks at
value[0]and assumes homogeneity. Mixed-type arrays will be mistyped.
🧹 Nitpick comments (8)
json-tool/public/assets/js/app.js (1)
24-30: Avoid brittle attribute-selector – switch to data-attributes or a lookup mapQuerying by
document.querySelector(`.feature-item[onclick*="${mode}"]`)relies on substring matching inside the inline
onclickattribute.
• Any accidental overlap (e.g."edit"vs"editor") selects multiple items.
• Inline handlers are harder to lint, minify and audit.Prefer:
- <div class="feature-item" onclick="switchMode('formatter')"> + <div class="feature-item" data-mode="formatter">and in JS:
document .querySelector(`.feature-item[data-mode="${mode}"]`) ?.classList.add("active");json-tool/public/index.html (1)
313-327: Adddeferto locally-hosted scripts for faster first paintAll core/feature scripts are plain
<script>tags that block rendering. Adding
deferkeeps execution order but frees the parser:- <script src="assets/js/core/storage.js"></script> + <script defer src="assets/js/core/storage.js"></script>Repeat for the remaining JS includes.
json-tool/public/assets/css/base.css (1)
1-7: Globalbox-sizingreset missingWithout it, padding/borders break expected width calculations across the new
responsive layouts. Add once at the top:+*, +*::before, +*::after { + box-sizing: border-box; +}json-tool/public/assets/js/core/dom.js (1)
37-75: Tooltip can render off-screen
tooltip.style.left = rect.left + "px";ignores viewport width; long tab bars
cause the rename box to overflow. Clamp the position:const maxLeft = window.innerWidth - tooltip.offsetWidth - 8; tooltip.style.left = Math.min(rect.left, maxLeft) + 'px';json-tool/public/assets/js/features/editor/editor.js (1)
161-191: Reordering logic duplicated – reuse core utilityThis block copies the drag code from
core/dom.js. Replace with:enableTabReordering('editor-tabs-container');after adding/removing buttons.
json-tool/public/assets/js/features/compare/compare.js (1)
48-50: Avoid redundant state persistence
createCompareTab()already callssaveGlobalState()at the end; calling it again inaddCompareTab()triggers an immediate duplicate write.
Consider dropping one of the calls to reduce unnecessary I/O.json-tool/public/assets/js/features/codegen/codegen.js (1)
38-50: Enable tab reordering for new tabs too
createCodegenTab()doesn’t callenableTabReordering, so freshly added tabs can’t be dragged until reload. Add the call for parity withcreateCodegenTabWithData().json-tool/README.md (1)
13-13: Specify language for fenced code blockAdd a language after the backticks (e.g., ```text) to satisfy Markdown-lint and enable syntax highlighting.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
json-tool/json.svgis excluded by!**/*.svg
📒 Files selected for processing (15)
json-tool/README.md(1 hunks)json-tool/public/assets/css/base.css(1 hunks)json-tool/public/assets/css/main.css(1 hunks)json-tool/public/assets/css/themes/dark.css(1 hunks)json-tool/public/assets/js/app.js(1 hunks)json-tool/public/assets/js/core/dom.js(1 hunks)json-tool/public/assets/js/core/json-utils.js(1 hunks)json-tool/public/assets/js/core/storage.js(1 hunks)json-tool/public/assets/js/features/codegen/codegen.js(1 hunks)json-tool/public/assets/js/features/compare/compare.js(1 hunks)json-tool/public/assets/js/features/convert/convert.js(1 hunks)json-tool/public/assets/js/features/editor/editor.js(1 hunks)json-tool/public/assets/js/features/formatter/formatter.js(1 hunks)json-tool/public/assets/js/features/mockgen/mockgen.js(1 hunks)json-tool/public/index.html(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- json-tool/public/assets/css/themes/dark.css
🚧 Files skipped from review as they are similar to previous changes (5)
- json-tool/public/assets/js/core/json-utils.js
- json-tool/public/assets/css/main.css
- json-tool/public/assets/js/features/convert/convert.js
- json-tool/public/assets/js/core/storage.js
- json-tool/public/assets/js/features/mockgen/mockgen.js
🧰 Additional context used
🧬 Code Graph Analysis (2)
json-tool/public/assets/js/app.js (2)
json-tool/public/assets/js/features/mockgen/mockgen.js (1)
mode(83-85)json-tool/public/assets/js/core/dom.js (1)
modal(143-143)
json-tool/public/assets/js/core/dom.js (4)
json-tool/public/assets/js/features/codegen/codegen.js (8)
textarea(44-44)textarea(84-84)textarea(112-112)tabId(11-11)tabId(54-54)tabButton(13-13)tabButton(55-55)tabButton(149-151)json-tool/public/assets/js/features/formatter/formatter.js (6)
textarea(83-83)textarea(109-109)parsed(113-113)tabId(11-11)tabButton(14-14)tabButton(248-250)json-tool/public/assets/js/features/compare/compare.js (5)
tabId(11-11)tabId(55-55)tabButton(13-13)tabButton(56-56)tabButton(162-164)json-tool/public/assets/js/app.js (1)
modal(67-67)
🪛 LanguageTool
json-tool/README.md
[uncategorized] ~96-~96: Loose punctuation mark.
Context: ... - Core Utilities - storage.js: Local storage and state management - ...
(UNLIKELY_OPENING_PUNCTUATION)
🪛 markdownlint-cli2 (0.17.2)
json-tool/README.md
13-13: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
🔇 Additional comments (1)
json-tool/public/assets/js/app.js (1)
41-77: Keyboard shortcut may shadow browser defaults & accessibility
Ctrl/Cmd + Shift + StriggerssaveEditorContent, overriding the browser’s native
“Save Page As”. Consider a less intrusive combo (e.g.Ctrl + Alt + S) or ask
for explicit user opt-in. At minimum, wrap the block in a focus–context check so
the handler is ignored when an input/textarea is focused.
| /* ========== Tab reordering ========== */ | ||
| function enableTabReordering(containerId) { | ||
| const container = document.getElementById(containerId); | ||
| const tabButtons = container.querySelectorAll(".tab-button[data-tab]"); | ||
|
|
||
| tabButtons.forEach((button) => { | ||
| button.draggable = true; | ||
|
|
||
| button.addEventListener("dragstart", (e) => { | ||
| e.dataTransfer.setData("text/plain", button.getAttribute("data-tab")); | ||
| button.classList.add("dragging"); | ||
| }); | ||
|
|
||
| button.addEventListener("dragend", () => { | ||
| button.classList.remove("dragging"); | ||
| container | ||
| .querySelectorAll(".tab-button") | ||
| .forEach((b) => b.classList.remove("drag-over")); | ||
| }); | ||
|
|
||
| button.addEventListener("dragover", (e) => { | ||
| e.preventDefault(); | ||
| button.classList.add("drag-over"); | ||
| }); | ||
|
|
||
| button.addEventListener("dragleave", () => { | ||
| button.classList.remove("drag-over"); | ||
| }); | ||
|
|
||
| button.addEventListener("drop", (e) => { | ||
| e.preventDefault(); | ||
| const draggedId = e.dataTransfer.getData("text/plain"); | ||
| const draggedBtn = container.querySelector(`[data-tab="${draggedId}"]`); | ||
|
|
||
| button.classList.remove("drag-over"); | ||
|
|
||
| if (draggedBtn && draggedBtn !== button) { | ||
| container.insertBefore(draggedBtn, button); | ||
| saveGlobalState(); | ||
| } | ||
| }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Duplication with enableEditorTabReordering – extract once
Almost identical drag-and-drop logic exists in features/editor/editor.js.
Expose a single utility (already here) and have the editor call
enableTabReordering('editor-tabs-container') instead of its own copy.
🤖 Prompt for AI Agents
In json-tool/public/assets/js/core/dom.js around lines 160 to 202, the
drag-and-drop tab reordering logic duplicates similar code in
features/editor/editor.js. To fix this, keep the enableTabReordering function
here as a reusable utility and remove the duplicate code from editor.js. Then
update the editor code to call enableTabReordering with the appropriate
container ID (e.g., 'editor-tabs-container') instead of implementing its own
drag-and-drop logic.
| function saveEditorContent(tabId) { | ||
| const content = editorInstances[tabId].getMarkdown(); | ||
| localStorage.setItem(tabId, content); | ||
| updateEditorGlobalState(); | ||
| Swal.fire({ | ||
| toast: true, | ||
| position: "top-end", | ||
| icon: "success", | ||
| title: "Autosaved", | ||
| showConfirmButton: false, | ||
| timer: 1500, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Autosave toast every 5 s will spam users
saveEditorContent() always fires a success toast; when called by the global
autosave timer this shows repeatedly. Add a silent flag or throttle the toast
for autosave calls.
🤖 Prompt for AI Agents
In json-tool/public/assets/js/features/editor/editor.js around lines 76 to 88,
the saveEditorContent function always shows a success toast, causing spam when
autosave triggers frequently. Modify the function to accept a silent flag
parameter; when true, skip showing the toast. Update all calls to
saveEditorContent accordingly, passing silent=true for autosave calls to prevent
repeated toast notifications.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (6)
json-tool/public/assets/js/features/formatter/formatter.js (4)
124-128: Focus-scroll handler is re-attached on every keystrokeEach call to
updateFormatterPreviewattaches a freshfocuslistener to every.tree-key, producing O(N²) listener growth and memory leaks.Previous review already flagged this.
Implement a single delegated listener once at startup instead.
170-173:innerHTML-based highlighting → XSS vectorUsing user-supplied search text inside
innerHTMLopens an injection path, even with simple regex escaping.Prefer DOM manipulation:
-rawPreview.innerHTML = content.replace(regex,'<span class="highlight">$1</span>'); +rawPreview.textContent = ""; +content.split(regex).forEach((part, idx) => { + rawPreview.append(document.createTextNode(part)); + if (idx < content.split(regex).length - 1) { + const mark = document.createElement("span"); + mark.className = "highlight"; + mark.textContent = RegExp.$1; // original match + rawPreview.append(mark); + } +});Apply the same principle in the recursive tree view highlight block.
Also applies to: 180-183
195-197:updateFormatterTabColorstill emptyThis stub was flagged previously; either implement DOM update or clearly comment.
56-74: Duplicate preview sections create invalid DOM IDs & break tab toggling
#${tabId}-raw-previewand#${tabId}-tree-previeware rendered twice.
Having two identical IDs violates HTML spec, confusesquerySelector, and breaksshowFormatterPreviewTabtoggling logic.@@ - <div id="${tabId}-raw-preview" class="preview-section active"> - <pre class="raw-json"></pre> - </div> - <div id="${tabId}-tree-preview" class="preview-section"> - <div class="tree-view"></div> - </div> - <div id="${tabId}-error-preview" class="preview-section"> - <div class="error-message"></div> - </div> - - <div id="${tabId}-raw-preview" class="preview-section active"> - <button class="copy-button" onclick="copyRawJSON('${tabId}')">Copy</button> - <pre class="raw-json"></pre> - </div> - <div id="${tabId}-tree-preview" class="preview-section"> - <button class="copy-button" onclick="copyRawJSON('${tabId}')">Copy JSON</button> - <div class="tree-view"></div> - </div> - <pre class="code-output" style="margin-top:10px; overflow:auto;"></pre> + <div id="${tabId}-raw-preview" class="preview-section active"> + <button class="copy-button" onclick="copyRawJSON('${tabId}')"> + Copy + </button> + <pre class="raw-json"></pre> + </div> + <div id="${tabId}-tree-preview" class="preview-section"> + <button class="copy-button" onclick="copyRawJSON('${tabId}')"> + Copy JSON + </button> + <div class="tree-view"></div> + </div> + <div id="${tabId}-error-preview" class="preview-section"> + <div class="error-message"></div> + </div> + <pre class="code-output" style="margin-top:10px; overflow:auto;"></pre>Remove the first triplet or the second—but keep just one set.
Do a quick manual test afterwards;showFormatterPreviewTabshould correctly flip between raw / tree / error panes.json-tool/public/assets/js/features/codegen/codegen.js (2)
70-82: “Copy Code” button still missing increateCodegenTabWithDataPrevious review flagged this; the button exists in
createCodegenTab()(line 39) but not here, so restored tabs lose the copy capability.
Please add the button for feature-parity.
175-184: Array type inference still inspects only the first elementLogic unchanged from the earlier review; mixed-type arrays will be typed incorrectly (
any[]or wrong primitive). Consider iterating over all elements to build an accurate union / fallback type.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
json-tool/public/assets/js/core/dom.js(1 hunks)json-tool/public/assets/js/features/codegen/codegen.js(1 hunks)json-tool/public/assets/js/features/compare/compare.js(1 hunks)json-tool/public/assets/js/features/editor/editor.js(1 hunks)json-tool/public/assets/js/features/formatter/formatter.js(1 hunks)json-tool/public/index.html(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- json-tool/public/index.html
- json-tool/public/assets/js/features/editor/editor.js
- json-tool/public/assets/js/features/compare/compare.js
- json-tool/public/assets/js/core/dom.js
🧰 Additional context used
🧬 Code Graph Analysis (1)
json-tool/public/assets/js/features/formatter/formatter.js (4)
json-tool/public/assets/js/features/editor/editor.js (6)
tabId(5-5)tabButton(17-17)tabContent(35-35)content(77-77)result(98-105)remaining(116-118)json-tool/public/assets/js/core/dom.js (2)
tabButton(59-61)parsed(45-45)json-tool/public/assets/js/features/convert/convert.js (1)
parsed(31-31)json-tool/public/assets/js/features/mockgen/mockgen.js (5)
content(203-203)blob(180-180)url(181-181)a(182-182)result(58-58)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (6)
json-tool/public/assets/js/features/formatter/formatter.js (4)
56-74: Duplicate preview sections create conflicting IDs
raw-previewandtree-previeware rendered twice, producing identical element IDs and breakingquerySelectorlook-ups and ARIA uniqueness assumptions. Remove the second block (lines 66-74) or generate unique IDs.- <div id="${tabId}-raw-preview" class="preview-section active"> - <button class="copy-button" onclick="copyRawJSON('${tabId}')">Copy</button> - <pre class="raw-json"></pre> - </div> - <div id="${tabId}-tree-preview" class="preview-section"> - <button class="copy-button" onclick="copyRawJSON('${tabId}')">Copy JSON</button> - <div class="tree-view"></div> - </div> - <pre class="code-output" style="margin-top:10px; overflow:auto;"></pre> + <!-- second preview block removed – IDs must be unique -->
198-200:updateFormatterTabColoris still a stubThe colour picker currently saves state but never reflects the change in the UI, making the picker useless. Re-implement the DOM update or add a TODO explaining the omission.
127-131: Event listeners are re-attached on every keystroke
updateFormatterPreviewadds a freshfocushandler to every.tree-keyeach time the user types. This leaks memory and degrades performance for large documents. Move the listener setup to a one-time delegated handler during module initialisation.
170-187: Search highlighting still relies oninnerHTML→ XSS vectorUser input is injected via
innerHTML, allowing crafted strings such as"<img onerror=alert(1)>"to execute. Replace with safe DOM construction (textContent+createElement) or a library that handles escaping.json-tool/public/assets/js/features/codegen/codegen.js (2)
83-94: “Copy Code” button still missing in restored tabs
The past review flagged this exact omission; it hasn’t been addressed. Add the same<button class="copy-button" …>element that exists increateCodegenTab().
187-196: Type inference for arrays still inspects only the first elementPrevious feedback recommended scanning the entire array to avoid incorrect types for mixed arrays; the logic remains unchanged.
🧹 Nitpick comments (3)
json-tool/public/assets/js/features/formatter/formatter.js (3)
107-123: Parsing JSON on everyinputis expensive—debounce or defer
JSON.parse/stringifyruns for every character typed. For large payloads this blocks the UI. DebounceupdateFormatterPreview(e.g., 300 ms) or trigger only onblur/explicit “Format” action.
271-273: Unused variable:focusedNode
focusedNodeis declared but never referenced. Remove it or implement the intended focus-tracking logic.
74-75: Dead markup:code-outputelement is never populatedA
<pre class="code-output">is inserted yet no code writes to it. Delete or hook it up to actual output to avoid confusing future maintainers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
json-tool/public/assets/js/features/codegen/codegen.js(1 hunks)json-tool/public/assets/js/features/formatter/formatter.js(1 hunks)
🔇 Additional comments (1)
json-tool/public/assets/js/features/formatter/formatter.js (1)
83-88:autoFormatTextareacomes from elsewhere—verify availabilityThe function is invoked on
pasteandblurbut is not defined in this module. Ensure it exists globally or import it; otherwise these listeners will throw at runtime.
| let result = ""; | ||
| if (isRoot) { | ||
| result += | ||
| "from dataclasses import dataclass\\nfrom typing import Any, List\\n\\n"; | ||
| } | ||
| result += `@dataclass\\nclass ${className}:\\n`; |
There was a problem hiding this comment.
generatePython outputs literal “\n” instead of newlines
Double-escaping the newline results in the characters “\n” being written into the generated code.
- "from dataclasses import dataclass\\nfrom typing import Any, List\\n\\n";
+ "from dataclasses import dataclass\nfrom typing import Any, List\n\n";Apply the same fix to the \\n prepends at lines 231 and 244.
Also applies to: 229-232, 244-244
🤖 Prompt for AI Agents
In json-tool/public/assets/js/features/codegen/codegen.js around lines 213 to
218, the generatePython function outputs literal "\\n" instead of actual
newlines due to double-escaping. Replace all occurrences of "\\n" with "\n" to
ensure newlines are correctly rendered in the generated code. Apply this fix
consistently to similar newline strings at lines 229-232, 231, and 244 as well.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
json-tool/public/assets/js/features/codegen/codegen.js (3)
93-104: Copy-button still missing increateCodegenTabWithDataThe markup for restored tabs omits the “Copy Code” button that exists for newly-created tabs. Users cannot copy previously saved output without it.
<button onclick="generateCode('${tabId}')">Generate Code</button> + <button class="copy-button" onclick="copyCodeOutput('${tabId}')">Copy Code</button>
198-206: Array type inference still only looks at the first element
generateTypeScript()bases the array element type onvalue[0], giving wrong types for mixed-type arrays or empty slots.- const elem = value[0]; - if (typeof elem === "object" && elem !== null) { + const elemTypes = new Set(value.map(v => (v === null ? "null" : Array.isArray(v) ? "array" : typeof v))); + const hasOnlyObjects = elemTypes.size === 1 && elemTypes.has("object"); + if (hasOnlyObjects) {Consider inspecting all elements (or falling back to
any[]) to avoid incorrect declarations.
222-255: Escaped “\n” results in literal backslashes in generated PythonThe generator still writes
@dataclass\\nclass …and prepends"\\n"on recursion, so the output contains\ncharacters instead of real newlines.- result += `@dataclass\\nclass ${className}:\\n`; + result += `@dataclass\nclass ${className}:\n`; ... - result += "\\n" + generatePython(elem, subClass, false); + result += "\n" + generatePython(elem, subClass, false); ... - result += "\\n" + generatePython(value, subClass, false); + result += "\n" + generatePython(value, subClass, false);
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
json-tool/public/assets/js/features/codegen/codegen.js (3)
93-103: “Copy Code” button still missing in restored tabsThe UI loses the copy-to-clipboard capability whenever a tab is rebuilt from saved state because the corresponding
<button>element is not included in thecreateCodegenTabWithData()markup.
This was reported earlier but the fix hasn’t landed in the current diff.<button onclick="generateCode('${tabId}')">Generate Code</button> + <button class="copy-button" onclick="copyCodeOutput('${tabId}')">Copy Code</button>
197-206: Array type inference still uses the first element only
generateTypeScript()continues to look atvalue[0]to decide the whole array’s type, producing wrong typings for mixed-type arrays.
See the earlier review with a full patch that scans the entire array or falls back toany[].
223-256: Python generator outputs literal “\n” – wrong new-line handlingEscaping the backslash causes the string
\nto appear in the generated code instead of real line breaks.-result += `@dataclass\\nclass ${className}:\\n`; +result += `@dataclass\nclass ${className}:\n`; - result += "\\n" + generatePython(elem, subClass, false); + result += "\n" + generatePython(elem, subClass, false); - result += "\\n" + generatePython(value, subClass, false); + result += "\n" + generatePython(value, subClass, false);Without this fix the emitted Python is syntactically invalid.
🧹 Nitpick comments (1)
json-tool/public/assets/js/features/codegen/codegen.js (1)
276-279: Preferconstoverletfor never-reassigned variables
subStructis assigned once and never mutated—declare it withconstto convey intent and satisfy linters.- let subStruct = structName + capitalize(key); + const subStruct = structName + capitalize(key);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
json-tool/public/assets/js/features/codegen/codegen.js(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
json-tool/public/assets/js/features/codegen/codegen.js (4)
json-tool/public/assets/js/features/editor/editor.js (5)
tabId(5-5)tabButton(17-17)tabContent(35-35)result(98-105)remaining(116-118)json-tool/public/assets/js/features/formatter/formatter.js (16)
tabId(11-11)tabButton(14-14)tabButton(251-253)tabsContainer(30-30)addButton(31-31)tabContent(37-37)tabContent(108-108)tabContent(136-136)tabContent(154-154)tabContent(219-219)tabContent(254-254)textarea(83-83)textarea(109-109)selectedTab(97-97)result(240-247)remaining(257-259)json-tool/public/assets/js/features/compare/compare.js (16)
tabId(11-11)tabId(55-55)tabButton(13-13)tabButton(56-56)tabButton(165-167)tabsContainer(22-22)tabsContainer(65-65)addButton(23-23)addButton(66-66)tabContent(26-26)tabContent(68-68)tabContent(111-111)tabContent(168-168)selectedTab(100-100)result(154-161)remaining(171-173)json-tool/public/assets/js/core/dom.js (1)
tabButton(59-61)
🪛 Biome (1.9.4)
json-tool/public/assets/js/features/codegen/codegen.js
[error] 278-278: This let declares a variable that is only assigned once.
'subStruct' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
🔇 Additional comments (1)
json-tool/public/assets/js/features/codegen/codegen.js (1)
64-70: Potential tab-ID drift after state restore
codegenTabCount++is invoked when recreating tabs from persisted data.
If saved IDs are non-sequential or the highest index is larger than the number of restored tabs, subsequent calls tocreateCodegenTab()may generate duplicate IDs.
Consider computingcodegenTabCountfrom the maximum numeric suffix in existing IDs instead of blindly incrementing.
|
@shravan20 , done working on the reviews i was given. Thanks! |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
json-tool/public/assets/js/features/formatter/formatter.js (4)
209-211:updateFormatterTabColoris still a stubThe function merely calls
saveGlobalState(). Implement the visual update or document why it is intentionally left blank.
181-187: innerHTML-based highlighting is still vulnerable to XSS
The previous review already flagged this; user input is still injected viainnerHTMLin both raw and tree views, enabling script injection with a crafted search string.Refactor to build DOM nodes (
textContent,createTextNode,appendChild) instead of template-based replacement.Also applies to: 193-197
138-142: Focus listeners are re-added on every preview update
updateFormatterPreviewattachesfocuslisteners to every.tree-keyeach time it runs, causing O(N²) listener growth. Move this logic to a one-time delegated listener (e.g.,document.addEventListener("focus", …, true)).
67-85: Duplicate preview sections create conflicting IDs
#${tabId}-raw-previewand#${tabId}-tree-previeware declared twice, which violates the uniqueness guarantee of DOM IDs and breaks query selectors & event delegation.- <div id="${tabId}-raw-preview" class="preview-section active"> - <pre class="raw-json"></pre> - </div> - <div id="${tabId}-tree-preview" class="preview-section"> - <div class="tree-view"></div> - </div> - <div id="${tabId}-error-preview" class="preview-section"> - <div class="error-message"></div> - </div> - - <div id="${tabId}-raw-preview" class="preview-section active"> - <button class="copy-button" onclick="copyRawJSON('${tabId}')">Copy</button> - <pre class="raw-json"></pre> - </div> - <div id="${tabId}-tree-preview" class="preview-section"> - <button class="copy-button" onclick="copyRawJSON('${tabId}')">Copy JSON</button> - <div class="tree-view"></div> - </div> - <pre class="code-output" style="margin-top:10px; overflow:auto;"></pre> + <div id="${tabId}-raw-preview" class="preview-section active"> + <button class="copy-button" onclick="copyRawJSON('${tabId}')">Copy</button> + <pre class="raw-json"></pre> + </div> + <div id="${tabId}-tree-preview" class="preview-section"> + <button class="copy-button" onclick="copyRawJSON('${tabId}')">Copy JSON</button> + <div class="tree-view"></div> + </div> + <div id="${tabId}-error-preview" class="preview-section"> + <div class="error-message"></div> + </div> + <pre class="code-output" style="margin-top:10px; overflow:auto;"></pre>Remove the duplicated block (lines 77-85) or merge the copy buttons into the first definition to restore ID uniqueness.
🧹 Nitpick comments (1)
json-tool/public/assets/js/features/formatter/formatter.js (1)
282-284:focusedNodeis declared but never usedRemove the unused variable to avoid confusion and satisfy linters.
- const focusedNode = null;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
json-tool/public/assets/js/features/formatter/formatter.html(1 hunks)json-tool/public/assets/js/features/formatter/formatter.js(1 hunks)json-tool/public/index.html(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- json-tool/public/index.html
🧰 Additional context used
🪛 HTMLHint (1.5.0)
json-tool/public/assets/js/features/formatter/formatter.html
[error] 2-2: Doctype must be declared before any non-comment content.
(doctype-first)
🔇 Additional comments (1)
json-tool/public/assets/js/features/formatter/formatter.html (1)
1-11: HTML snippet is fine without a<!DOCTYPE>This fragment is dynamically injected into an existing document; therefore, the HTMLHint
doctype-firstwarning is not applicable.
Fixes #17
Changes Made:
Restructured the project into a modular and scalable layout under the json-tool/ root.
Separated frontend files into public/ for better clarity and potential deployment readiness.
Organized all JavaScript logic under public/assets/js/ with further modularization:
Split styling into base.css, main.css, and a themes/ directory.
Introduced formatter.html to isolate formatter-specific UI logic.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Documentation
Style
Chores