Feat 2 add browser sync #14
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's Guide by SourceryThis pull request adds functionality to export and import user shortcuts from/to a JSON file. It includes UI elements for triggering these actions and implements logic to handle conflicts during the import process. No diagrams generated as the changes look simple and do not need a visual representation. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @lacostenycoder - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider updating the title or description to clarify that this feature implements manual JSON import/export rather than native browser sync.
- The logic for filtering
localStoragekeys before processing is duplicated; consider extracting it into a reusable helper function. - Consider managing the new CSS dependency (
bootstrap.min.css) through a package manager or CDN instead of adding the file directly to the repository.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| function exportShortcuts() { | ||
| // Create an object to hold all shortcuts | ||
| var shortcuts = {}; | ||
| for (var i = 0; i < localStorage.length; i++) { |
There was a problem hiding this comment.
suggestion: Consider reusing the storedKeys array for exporting shortcuts.
Since you already filter and store the shortcut keys in the storedKeys variable for display, using it in exportShortcuts would remove duplicate filtering logic and ensure consistency between displayed and exported shortcuts.
Suggested implementation:
// Function to export shortcuts to a JSON file
function exportShortcuts() {
// Create an object to hold all shortcuts
var shortcuts = {};
storedKeys.forEach(function(key) {
shortcuts[key] = localStorage[key];
});Ensure that the storedKeys array is defined and available in the scope where exportShortcuts is declared.
| key !== 'deletedShortcuts') { | ||
| var url = localStorage[key]; | ||
| shortcuts[key] = url; | ||
| } |
There was a problem hiding this comment.
issue (complexity): Consider refactoring the filtering and conflict-resolution logic into dedicated helper functions to improve code readability and maintainability.
Refactor the filtering and conflict-resolution logic into dedicated helper functions to reduce nesting and duplicate code without changing functionality.
For example, extract the duplicate key filtering into a helper:
```js
function isShortcutKey(key) {
return key !== 'lastSynced' &&
!key.startsWith('lastSynced_') &&
key !== 'shortcutsMeta' &&
key !== 'deletedShortcuts';
}Use this helper in both export and storedKeys generation:
var storedKeys = Object.keys(localStorage).filter(isShortcutKey);Similarly, for the nested conflict resolution in importShortcuts, consider extracting a function:
function resolveImport(key, newUrl, importMode) {
if (!localStorage[key]) {
localStorage[key] = newUrl;
return 'added';
}
switch (importMode) {
case 2:
localStorage[key] = newUrl;
return 'updated';
case 3:
if (localStorage[key] !== newUrl) {
let newKey = key + "_imported";
let counter = 1;
while (localStorage[newKey]) {
newKey = key + "_imported_" + counter++;
}
localStorage[newKey] = newUrl;
return 'added';
}
return 'skipped';
default:
return 'skipped';
}
}Then process imports like:
for (var key in shortcuts) {
var status = resolveImport(key, shortcuts[key], importMode);
if (status === 'added') added++;
else if (status === 'updated') updated++;
else skipped++;
}These changes centralize duplicate logic and flatten nested conditionals while maintaining current behavior.
Comment on lines
+2
to
+8
| var storedKeys = Object.keys(localStorage).filter(function(key) { | ||
| // Filter out special keys that shouldn't be displayed as shortcuts | ||
| return key !== 'lastSynced' && | ||
| !key.startsWith('lastSynced_') && | ||
| key !== 'shortcutsMeta' && | ||
| key !== 'deletedShortcuts'; | ||
| }); |
There was a problem hiding this comment.
issue (code-quality): Use const or let instead of var. (avoid-using-var)
Explanation
`const` is preferred as it ensures you cannot reassign references (which can lead to buggy and confusing code). `let` may be used if you need to reassign references - it's preferred to `var` because it is block- rather than function-scoped.From the Airbnb JavaScript Style Guide
Comment on lines
+11
to
+45
| function exportShortcuts() { | ||
| // Create an object to hold all shortcuts | ||
| var shortcuts = {}; | ||
| for (var i = 0; i < localStorage.length; i++) { | ||
| var key = localStorage.key(i); | ||
| // Only export actual shortcuts, not metadata | ||
| if (key !== 'lastSynced' && | ||
| !key.startsWith('lastSynced_') && | ||
| key !== 'shortcutsMeta' && | ||
| key !== 'deletedShortcuts') { | ||
| var url = localStorage[key]; | ||
| shortcuts[key] = url; | ||
| } | ||
| } | ||
|
|
||
| var storedKeys = Object.keys(localStorage); | ||
| // Convert to JSON string | ||
| var jsonData = JSON.stringify(shortcuts, null, 2); | ||
|
|
||
| // Create a blob and download link | ||
| var blob = new Blob([jsonData], {type: 'application/json'}); | ||
| var url = URL.createObjectURL(blob); | ||
|
|
||
| // Create a temporary link and trigger download | ||
| var a = document.createElement('a'); | ||
| a.href = url; | ||
| a.download = 'chrome-quick-links-shortcuts.json'; | ||
| document.body.appendChild(a); | ||
| a.click(); | ||
|
|
||
| // Clean up | ||
| setTimeout(function() { | ||
| document.body.removeChild(a); | ||
| window.URL.revokeObjectURL(url); | ||
| }, 0); | ||
| } |
There was a problem hiding this comment.
issue (code-quality): Avoid function declarations, favouring function assignment expressions, inside blocks. (avoid-function-declarations-in-blocks)
Explanation
Function declarations may be hoisted in Javascript, but the behaviour is inconsistent between browsers. Hoisting is generally confusing and should be avoided. Rather than using function declarations inside blocks, you should use function expressions, which create functions in-scope.| // Function to export shortcuts to a JSON file | ||
| function exportShortcuts() { | ||
| // Create an object to hold all shortcuts | ||
| var shortcuts = {}; |
There was a problem hiding this comment.
issue (code-quality): Use const or let instead of var. (avoid-using-var)
Explanation
`const` is preferred as it ensures you cannot reassign references (which can lead to buggy and confusing code). `let` may be used if you need to reassign references - it's preferred to `var` because it is block- rather than function-scoped.From the Airbnb JavaScript Style Guide
| } else if (importMode === 3 && localStorage[key] !== newUrl) { | ||
| // Merge mode - if URLs differ, create a new shortcut with a suffix | ||
| var newKey = key + "_imported"; | ||
| var counter = 1; |
There was a problem hiding this comment.
issue (code-quality): Use const or let instead of var. (avoid-using-var)
Explanation
`const` is preferred as it ensures you cannot reassign references (which can lead to buggy and confusing code). `let` may be used if you need to reassign references - it's preferred to `var` because it is block- rather than function-scoped.From the Airbnb JavaScript Style Guide
Comment on lines
163
to
172
| var doDelete = function() { | ||
| var key = this.id.split('-')[1]; | ||
| if ( confirm('Are you sure?') ) { | ||
| delete localStorage[key]; | ||
| if(chrome.storage){ | ||
| chrome.storage.sync.remove(key); | ||
| } | ||
| window.location = window.location; | ||
| } | ||
| if (confirm('Are you sure?')) { | ||
| delete localStorage[key]; | ||
| if(chrome.storage){ | ||
| chrome.storage.sync.remove(key); | ||
| } | ||
| window.location = window.location; | ||
| } | ||
| } |
There was a problem hiding this comment.
issue (code-quality): Use const or let instead of var. (avoid-using-var)
Explanation
`const` is preferred as it ensures you cannot reassign references (which can lead to buggy and confusing code). `let` may be used if you need to reassign references - it's preferred to `var` because it is block- rather than function-scoped.From the Airbnb JavaScript Style Guide
| if(chrome.storage){ | ||
| chrome.storage.sync.remove(key); | ||
| } | ||
| window.location = window.location; |
There was a problem hiding this comment.
suggestion (code-quality): Assigning a variable to itself has no effect. (dont-self-assign-variables)
Suggested change
| window.location = window.location; | |
| ; |
Explanation
Assigning a variable to itself has no effect, and is therefore either redundant or a mistake.
Comment on lines
174
to
182
| function checkExample(){ | ||
| var tr = document.getElementsByTagName('tr'); | ||
| if(tr.length > 1) { | ||
| var example = document.getElementById('example'); | ||
| example.remove(); | ||
| if (example) { | ||
| example.remove(); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
issue (code-quality): Avoid function declarations, favouring function assignment expressions, inside blocks. (avoid-function-declarations-in-blocks)
Explanation
Function declarations may be hoisted in Javascript, but the behaviour is inconsistent between browsers. Hoisting is generally confusing and should be avoided. Rather than using function declarations inside blocks, you should use function expressions, which create functions in-scope.| var url = localStorage[key]; | ||
| document.getElementById("edit-" + key).addEventListener("click", doEdit, false); | ||
| document.getElementById("delete-" + key).addEventListener("click", doDelete, false); | ||
| var editBtn = document.getElementById("edit-" + key); |
There was a problem hiding this comment.
issue (code-quality): Use const or let instead of var. (avoid-using-var)
Explanation
`const` is preferred as it ensures you cannot reassign references (which can lead to buggy and confusing code). `let` may be used if you need to reassign references - it's preferred to `var` because it is block- rather than function-scoped.From the Airbnb JavaScript Style Guide
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Chrome browser sync was not straightforward to implement, however this approach allows users to import/export their shortcuts using a simple json file format. Easy, and just works.
Summary by Sourcery
Add functionality to import and export user shortcuts using JSON files.
New Features:
Enhancements:
Chores: