refactored the code into different files to make it clearer for contribution by Axthauvin · Pull Request #1 · Axthauvin/gogo

Axthauvin · 2025-11-14T20:08:01Z

No description provided.

Major refactor splitting logic into core, background, modules, UI components, and pages directories. Migrates onboarding and popup scripts to pages, introduces modular imports/exports, and updates HTML/CSS to use new structure. Improves maintainability and scalability by organizing features into dedicated files and updating references throughout the project.

Introduces Jest configuration and setup files, updates .gitignore for test-related artifacts, adds test scripts and devDependencies to package.json, and provides a thorough test suite for settings modules in src/settings.test.js.

Added new test files for import/export, search, shortcuts, theme, DOM, and URL utilities. Refactored and moved settings page tests to src/pages/settings.test.js. Updated onboarding and settings logic to support onboarding step handling. Added build script to package.json.

Copilot

Pull Request Overview

This PR refactors the codebase from a monolithic architecture into a well-organized modular structure to improve maintainability and facilitate contributions. The main settings.js file (~1200 lines) has been split into focused modules organized by concern.

Key changes:

Introduced modular architecture with core/, modules/, ui/, utils/, background/, and pages/ directories
Added comprehensive test suite using Jest with test files for multiple modules
Separated CSS into themed variables and component-specific styles
Extracted reusable utilities for URL manipulation and DOM operations

Reviewed Changes

Copilot reviewed 47 out of 50 changed files in this pull request and generated 16 comments.

Show a summary per file

File	Description
`src/utils/url.js` & `url.test.js`	URL utility functions with validation and normalization
`src/utils/dom.js` & `dom.test.js`	DOM manipulation helpers for safe HTML handling
`src/core/browserAPI.js`	Cross-browser API abstraction layer
`src/core/storage.js`	Storage API wrapper with async helpers
`src/core/constants.js`	Centralized configuration constants
`src/modules/shortcuts/*.js`	Shortcut CRUD operations, validation, and name generation
`src/modules/theme/*.js`	Theme management with system preference detection
`src/modules/search/*.js`	Search and autocomplete functionality
`src/modules/import-export/*.js`	Import/export logic for shortcuts
`src/ui/components/*.js`	Reusable UI components (modal, toast, cards, lists)
`src/ui/forms/*.js`	Form handlers for shortcuts and options
`src/ui/navigation.js`	Section navigation controller
`src/pages/settings.js`	Main settings page orchestrator
`src/pages/popup.js`	Extension popup handler
`src/pages/onboarding.js`	Onboarding flow logic
`src/background/*.js`	Background service worker with omnibox and navigation handlers
`src/styles/*.css`	Refactored styles with CSS variables and theming
`src/settings.html`	Updated HTML with new script imports and simplified section IDs
`package.json`	Added Jest testing infrastructure
`jest.config.js`	Jest configuration for ES modules

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-14T20:12:50Z

+      '&lt;script&gt;alert("xss")&lt;/script&gt;'
+    );
+    expect(escapeHtml("Hello & goodbye")).toBe("Hello &amp; goodbye");
+    expect(escapeHtml('"quoted"')).toBe('"quoted"');


The test expects escapeHtml to NOT escape double quotes, but the actual implementation using div.textContent will escape them. The test expectation on line 16 is incorrect. When you set textContent to '"quoted"' and read innerHTML, you'll get "quoted" in most browsers, not "quoted".

Copilot · 2025-11-14T20:12:50Z

+          const indexToRemove =
+            this.editingIndex > duplicate.duplicateIndex
+              ? this.editingIndex
+              : this.editingIndex;


Potential logic error: The ternary operator will always assign this.editingIndex to indexToRemove regardless of the condition. Both branches of the ternary are identical. This should likely be:

const indexToRemove = this.editingIndex > duplicate.duplicateIndex ? this.editingIndex : this.editingIndex - 1;

or the logic needs to be reconsidered based on the intent.

Copilot · 2025-11-14T20:12:51Z

+  // Initialize theme
+  await initTheme();
+
+  // Initialize modal
+  modal.init();
+
+  // Initialize navigation
+  navigation = new Navigation();
+
+  // Initialize shortcut list
+  const listElement = document.getElementById("alias-list");
+  shortcutList = new ShortcutList(listElement);
+
+  // Setup callbacks
+  shortcutList.onEdit = handleEdit;
+  shortcutList.onDelete = () => shortcutList.reload();
+
+  // Load shortcuts
+  await shortcutList.reload();
+
+  // Initialize shortcut form
+  shortcutForm = new ShortcutForm();
+  shortcutForm.onSave = () => shortcutList.reload();
+
+  // Initialize options form
+  optionsForm = new OptionsForm();
+  optionsForm.onDataCleared = () => shortcutList.reload();
+
+  // Setup search
+  setupSearch();
+
+  // Setup export/import
+  setupExportImport();
+
+  // Handle query parameters
+  handleQueryParams();
+
+  // Check onboarding status
+  checkOnboarding();
+  setupShortcutCards();


Missing error handling in async function. The init function performs multiple async operations but doesn't have a try-catch block. If any initialization step fails (e.g., theme loading, storage access), the error will be unhandled. Consider wrapping the initialization in a try-catch block and providing appropriate error handling or user feedback.

Suggested change

// Initialize theme

await initTheme();

// Initialize modal

modal.init();

// Initialize navigation

navigation = new Navigation();

// Initialize shortcut list

const listElement = document.getElementById("alias-list");

shortcutList = new ShortcutList(listElement);

// Setup callbacks

shortcutList.onEdit = handleEdit;

shortcutList.onDelete = () => shortcutList.reload();

// Load shortcuts

await shortcutList.reload();

// Initialize shortcut form

shortcutForm = new ShortcutForm();

shortcutForm.onSave = () => shortcutList.reload();

// Initialize options form

optionsForm = new OptionsForm();

optionsForm.onDataCleared = () => shortcutList.reload();

// Setup search

setupSearch();

// Setup export/import

setupExportImport();

// Handle query parameters

handleQueryParams();

// Check onboarding status

checkOnboarding();

setupShortcutCards();

try {

// Initialize theme

await initTheme();

// Initialize modal

modal.init();

// Initialize navigation

navigation = new Navigation();

// Initialize shortcut list

const listElement = document.getElementById("alias-list");

shortcutList = new ShortcutList(listElement);

// Setup callbacks

shortcutList.onEdit = handleEdit;

shortcutList.onDelete = () => shortcutList.reload();

// Load shortcuts

await shortcutList.reload();

// Initialize shortcut form

shortcutForm = new ShortcutForm();

shortcutForm.onSave = () => shortcutList.reload();

// Initialize options form

optionsForm = new OptionsForm();

optionsForm.onDataCleared = () => shortcutList.reload();

// Setup search

setupSearch();

// Setup export/import

setupExportImport();

// Handle query parameters

handleQueryParams();

// Check onboarding status

checkOnboarding();

setupShortcutCards();

} catch (error) {

console.error("Failed to initialize settings page:", error);

showToast("An error occurred while loading the settings page. Please try again.", "error");

}

Copilot · 2025-11-14T20:12:51Z

+export function addCustomMapping(domain, shortcut) {
+  COMMON_MAPPINGS[domain] = shortcut;


The addCustomMapping function mutates the COMMON_MAPPINGS constant object, which is a best practice violation. Consider making COMMON_MAPPINGS a mutable variable (use let) or returning a new object instead of mutating the constant.

Copilot · 2025-11-14T20:12:51Z

+  "devDependencies": {
+    "@jest/globals": "^30.2.0",
+    "jest": "^30.2.0",
+    "jest-environment-jsdom": "^30.2.0"


Missing dependency in package.json. The test files use jsdom (imported in settings.test.js and themeManager.test.js), but it's not listed in devDependencies. Add "jsdom": "^23.0.0" or appropriate version to the devDependencies.

Suggested change

"jest-environment-jsdom": "^30.2.0"

"jest-environment-jsdom": "^30.2.0",

"jsdom": "^23.0.0"

Copilot · 2025-11-14T20:12:53Z

+
+// Core modules
+import { initTheme } from "../modules/theme/themeManager.js";
+import { getStorage, setStorage } from "../core/storage.js";


Unused import setStorage.

Suggested change

import { getStorage, setStorage } from "../core/storage.js";

import { getStorage } from "../core/storage.js";

Copilot · 2025-11-14T20:12:53Z

+ * Tests for DOM utility functions
+ */
+
+import { jest } from "@jest/globals";


Unused import jest.

Copilot · 2025-11-14T20:12:53Z

+ * Tests for URL utility functions
+ */
+
+import { jest } from "@jest/globals";


Unused import jest.

Copilot · 2025-11-14T20:12:54Z

+import { jest } from "@jest/globals";
+
+// Import functions to test
+const { isValidUrl, normalizeUrl, extractDomain } = await import("./url.js");


Unused variable normalizeUrl.

Copilot · 2025-11-14T20:12:54Z

+import { jest } from "@jest/globals";
+
+// Import functions to test
+const { isValidUrl, normalizeUrl, extractDomain } = await import("./url.js");


Unused variable extractDomain.

Axthauvin added 3 commits November 11, 2025 23:46

Add Jest testing setup and comprehensive tests

c1bc75c

Introduces Jest configuration and setup files, updates .gitignore for test-related artifacts, adds test scripts and devDependencies to package.json, and provides a thorough test suite for settings modules in src/settings.test.js.

Axthauvin requested a review from Copilot November 14, 2025 20:08

Copilot started reviewing on behalf of Axthauvin November 14, 2025 20:08 View session

Axthauvin merged commit 43eb1e8 into main Nov 14, 2025
5 checks passed

Copilot finished reviewing on behalf of Axthauvin November 14, 2025 20:10

Copilot AI reviewed Nov 14, 2025

View reviewed changes

Navigation Menu

Repository navigation

refactored the code into different files to make it clearer for contribution - #1

refactored the code into different files to make it clearer for contribution#1
Axthauvin merged 3 commits into
mainfrom
refactor

Axthauvin commented Nov 14, 2025

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Nov 14, 2025

Uh oh!

Copilot AI Nov 14, 2025

Uh oh!

Copilot AI Nov 14, 2025

Uh oh!

Copilot AI Nov 14, 2025

Uh oh!

Copilot AI Nov 14, 2025

Uh oh!

Copilot AI Nov 14, 2025

Uh oh!

Copilot AI Nov 14, 2025

Uh oh!

Copilot AI Nov 14, 2025

Uh oh!

Copilot AI Nov 14, 2025

Uh oh!

Copilot AI Nov 14, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		export function addCustomMapping(domain, shortcut) {
		COMMON_MAPPINGS[domain] = shortcut;

	"jest-environment-jsdom": "^30.2.0"
	"jest-environment-jsdom": "^30.2.0",
	"jsdom": "^23.0.0"

	import { getStorage, setStorage } from "../core/storage.js";
	import { getStorage } from "../core/storage.js";

Conversation

Axthauvin commented Nov 14, 2025

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants