feat: add question collection and bookmark feature#1
Open
stevehuuuu wants to merge 1 commit intoUtkarshPrajapati:mainfrom
Open
feat: add question collection and bookmark feature#1stevehuuuu wants to merge 1 commit intoUtkarshPrajapati:mainfrom
stevehuuuu wants to merge 1 commit intoUtkarshPrajapati:mainfrom
Conversation
📝 WalkthroughWalkthroughThe changes implement a favorites feature for the browser extension, enabling users to favorite webpage URLs via a star icon button in the overlay, store them in Chrome local storage, and manage the favorites list through a new tabbed interface in the popup with add and remove functionality. Changes
Sequence DiagramsequenceDiagram
participant User
participant Content as content.js<br/>(Overlay)
participant Storage as chrome.storage.local
participant Popup as popup.js<br/>(Popup UI)
User->>Content: Click favorite star button
Content->>Storage: Read current favorites list
activate Storage
Storage-->>Content: Return favorites array
deactivate Storage
Content->>Content: Toggle current URL in list
Content->>Storage: Write updated favorites
activate Storage
Storage-->>Content: Confirm write
deactivate Storage
Content->>Content: Update star icon (color/style)
User->>Popup: Click "Favorites" tab
Popup->>Storage: Read favorites from storage
activate Storage
Storage-->>Popup: Return favorites array
deactivate Storage
Popup->>Popup: Sort by timestamp (descending)
Popup->>Popup: Render favorites list with remove buttons
User->>Popup: Click remove on favorite item
Popup->>Storage: Update favorites array
activate Storage
Storage-->>Popup: Confirm write
deactivate Storage
Popup->>Popup: Re-render favorites list
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@extension/content.js`:
- Around line 24-26: The overlay only calls checkIsFavorite() once at init so
the star UI doesn't update if the shared "favorites" storage is modified
elsewhere; add a storage change listener that watches chrome.storage (or
browser.storage) changes for the "favorites" key and re-run checkIsFavorite()
(or a small helper that updates the element with id 'leetcode-helper-favorite'
and its filled/unfilled state) when the favorites value changes, ensuring
toggleFavorite() continues to work correctly after external updates.
In `@extension/popup.js`:
- Around line 312-345: The current UI sorts the favorites array
(favorites.sort(...)) but sets data-index on each .remove-favorite button, then
calls removeFavorite(index), causing mismatches between the displayed order and
underlying storage; change the buttons to store a stable identifier (e.g.,
item.id or item.url) in a data attribute (data-id), update the click handler to
read that id and call removeFavoriteById(id) (or modify removeFavorite to accept
an id), and implement removal by finding and splicing the stored array entry
that matches the id (instead of using the sorted row index); if favorites
objects lack a stable id, add one when saving (UUID or use url) and use that for
matching.
- Around line 324-329: The current code builds favorite rows by concatenating
into the html string (the `html += \`...\`` block that injects `item.title` and
`item.url`) which allows stored values to break out of markup; change the
renderer in extension/popup.js to create elements with DOM APIs instead of
template strings: use document.createElement for the wrapper div and anchor, set
anchor.href via setAttribute only after validating the URL (use the URL
constructor and allow only http/https), set text nodes via textContent for the
title and date (no innerHTML), and append children with appendChild; if URL
validation fails, do not set a clickable href (or fall back to a safe action) so
untrusted data cannot execute script.
- Around line 52-68: The click wiring uses selectors and ID patterns that don't
match popup.html, so event handlers never attach and renderFavorites() can't
run; update the button selector and content ID convention to match the DOM (use
document.querySelectorAll('.tab') or ensure buttons have .tab-btn) and change
the content lookup from document.getElementById(`${tab}-tab`) to
document.getElementById(`${tab}-content`) (or otherwise map data-tab values to
the corresponding content IDs), ensure the favorites panel exists (create an
element with id="favorites-content") and that the favorites button has
data-tab="favorites", and keep the renderFavorites() call when tab ===
'favorites' so the list renders into the new `#favorites-content` container.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 49a67e3c-c524-4a3e-b7ed-5646c44ee71c
📒 Files selected for processing (3)
extension/content.jsextension/popup.htmlextension/popup.js
Comment on lines
+24
to
+26
| document.getElementById('leetcode-helper-favorite').addEventListener('click', toggleFavorite); | ||
| // 检查当前题目是否已收藏 | ||
| checkIsFavorite(); |
There was a problem hiding this comment.
Re-sync the star when favorites changes outside the page.
This only checks storage once during initialization. If the popup removes the current problem from favorites (extension/popup.js Lines 362-369), the overlay icon stays filled and the next click can add the bookmark back instead of removing it.
Suggested fix
document.getElementById('leetcode-helper-favorite').addEventListener('click', toggleFavorite);
- // 检查当前题目是否已收藏
checkIsFavorite();
+ chrome.storage.onChanged.addListener((changes, areaName) => {
+ if (areaName === 'local' && changes.favorites) {
+ checkIsFavorite();
+ }
+ });📝 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.
Suggested change
| document.getElementById('leetcode-helper-favorite').addEventListener('click', toggleFavorite); | |
| // 检查当前题目是否已收藏 | |
| checkIsFavorite(); | |
| document.getElementById('leetcode-helper-favorite').addEventListener('click', toggleFavorite); | |
| checkIsFavorite(); | |
| chrome.storage.onChanged.addListener((changes, areaName) => { | |
| if (areaName === 'local' && changes.favorites) { | |
| checkIsFavorite(); | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@extension/content.js` around lines 24 - 26, The overlay only calls
checkIsFavorite() once at init so the star UI doesn't update if the shared
"favorites" storage is modified elsewhere; add a storage change listener that
watches chrome.storage (or browser.storage) changes for the "favorites" key and
re-run checkIsFavorite() (or a small helper that updates the element with id
'leetcode-helper-favorite' and its filled/unfilled state) when the favorites
value changes, ensuring toggleFavorite() continues to work correctly after
external updates.
Comment on lines
+52
to
+68
| const tabBtns = document.querySelectorAll('.tab-btn'); | ||
| const tabContents = document.querySelectorAll('.tab-content'); | ||
| tabBtns.forEach(btn => { | ||
| btn.addEventListener('click', () => { | ||
| const tab = btn.dataset.tab; | ||
| // 切换按钮状态 | ||
| tabBtns.forEach(b => b.classList.remove('active')); | ||
| btn.classList.add('active'); | ||
| // 切换内容 | ||
| tabContents.forEach(content => content.classList.remove('active')); | ||
| document.getElementById(`${tab}-tab`).classList.add('active'); | ||
| // 如果是收藏tab，加载收藏列表 | ||
| if (tab === 'favorites') { | ||
| renderFavorites(); | ||
| } | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The new favorites tab wiring does not match the popup DOM.
popup.html still renders .tab buttons and #settings-content / #about-content panels, so this loop never attaches today and renderFavorites() is unreachable. There is also no #favorites-tab container to render into. Even after adding .tab-btn, document.getElementById(\${tab}-tab`)` would hit the button IDs for settings/about instead of the content panels.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@extension/popup.js` around lines 52 - 68, The click wiring uses selectors and
ID patterns that don't match popup.html, so event handlers never attach and
renderFavorites() can't run; update the button selector and content ID
convention to match the DOM (use document.querySelectorAll('.tab') or ensure
buttons have .tab-btn) and change the content lookup from
document.getElementById(`${tab}-tab`) to
document.getElementById(`${tab}-content`) (or otherwise map data-tab values to
the corresponding content IDs), ensure the favorites panel exists (create an
element with id="favorites-content") and that the favorites button has
data-tab="favorites", and keep the renderFavorites() call when tab ===
'favorites' so the list renders into the new `#favorites-content` container.
Comment on lines
+312
to
+345
| // 按收藏时间倒序排列 | ||
| favorites.sort((a, b) => b.timestamp - a.timestamp); | ||
|
|
||
| let html = ` | ||
| <div style="margin-bottom: 16px;"> | ||
| <h3 style="margin-bottom: 8px;"><i class="fas fa-star" style="color: #f1c40f; margin-right: 6px;"></i> Your Favorites</h3> | ||
| <p style="font-size: 14px; color: var(--medium-text);">${favorites.length} saved problems</p> | ||
| </div> | ||
| <div style="max-height: 400px; overflow-y: auto; padding-right: 4px;"> | ||
| `; | ||
|
|
||
| favorites.forEach((item, index) => { | ||
| html += ` | ||
| <div class="favorite-item" style="background: var(--dark-card); border: 1px solid var(--dark-border); border-radius: 8px; padding: 12px; margin-bottom: 8px; display: flex; align-items: center; justify-content: space-between;"> | ||
| <a href="${item.url}" target="_blank" style="color: var(--light-text); text-decoration: none; flex: 1; margin-right: 8px;"> | ||
| <div style="font-weight: 500; margin-bottom: 4px;">${item.title}</div> | ||
| <div style="font-size: 12px; color: var(--medium-text);">${new Date(item.timestamp).toLocaleDateString()}</div> | ||
| </a> | ||
| <button class="remove-favorite" data-index="${index}" style="background: transparent; border: none; color: var(--error-color); cursor: pointer; padding: 4px 8px; border-radius: 4px; transition: background 0.2s;"> | ||
| <i class="fas fa-trash"></i> | ||
| </button> | ||
| </div> | ||
| `; | ||
| }); | ||
|
|
||
| html += '</div>'; | ||
| favoritesTab.innerHTML = html; | ||
|
|
||
| // 添加删除收藏事件 | ||
| document.querySelectorAll('.remove-favorite').forEach(btn => { | ||
| btn.addEventListener('click', (e) => { | ||
| e.stopPropagation(); | ||
| const index = parseInt(btn.dataset.index); | ||
| removeFavorite(index); |
There was a problem hiding this comment.
Delete favorites by a stable identifier, not the sorted row index.
The list is rendered after sorting by timestamp, but removeFavorite() reloads the unsorted storage array and splices the same index. Removing the first visible row can delete a different favorite from storage.
Suggested fix
- favorites.forEach((item, index) => {
+ favorites.forEach((item) => {
html += `
<div class="favorite-item" style="background: var(--dark-card); border: 1px solid var(--dark-border); border-radius: 8px; padding: 12px; margin-bottom: 8px; display: flex; align-items: center; justify-content: space-between;">
<a href="${item.url}" target="_blank" style="color: var(--light-text); text-decoration: none; flex: 1; margin-right: 8px;">
<div style="font-weight: 500; margin-bottom: 4px;">${item.title}</div>
<div style="font-size: 12px; color: var(--medium-text);">${new Date(item.timestamp).toLocaleDateString()}</div>
</a>
- <button class="remove-favorite" data-index="${index}" style="background: transparent; border: none; color: var(--error-color); cursor: pointer; padding: 4px 8px; border-radius: 4px; transition: background 0.2s;">
+ <button class="remove-favorite" data-url="${encodeURIComponent(item.url)}" data-timestamp="${item.timestamp}" style="background: transparent; border: none; color: var(--error-color); cursor: pointer; padding: 4px 8px; border-radius: 4px; transition: background 0.2s;">
<i class="fas fa-trash"></i>
</button>
</div>
`;
});
@@
document.querySelectorAll('.remove-favorite').forEach(btn => {
btn.addEventListener('click', (e) => {
e.stopPropagation();
- const index = parseInt(btn.dataset.index);
- removeFavorite(index);
+ removeFavorite(decodeURIComponent(btn.dataset.url), Number(btn.dataset.timestamp));
});
});
@@
-function removeFavorite(index) {
+function removeFavorite(url, timestamp) {
chrome.storage.local.get('favorites', (result) => {
- let favorites = result.favorites || [];
- favorites.splice(index, 1);
+ const favorites = (result.favorites || []).filter(
+ item => !(item.url === url && item.timestamp === timestamp)
+ );
chrome.storage.local.set({favorites: favorites}, () => {
renderFavorites();
});
});
}Also applies to: 362-368
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@extension/popup.js` around lines 312 - 345, The current UI sorts the
favorites array (favorites.sort(...)) but sets data-index on each
.remove-favorite button, then calls removeFavorite(index), causing mismatches
between the displayed order and underlying storage; change the buttons to store
a stable identifier (e.g., item.id or item.url) in a data attribute (data-id),
update the click handler to read that id and call removeFavoriteById(id) (or
modify removeFavorite to accept an id), and implement removal by finding and
splicing the stored array entry that matches the id (instead of using the sorted
row index); if favorites objects lack a stable id, add one when saving (UUID or
use url) and use that for matching.
Comment on lines
+324
to
+329
| html += ` | ||
| <div class="favorite-item" style="background: var(--dark-card); border: 1px solid var(--dark-border); border-radius: 8px; padding: 12px; margin-bottom: 8px; display: flex; align-items: center; justify-content: space-between;"> | ||
| <a href="${item.url}" target="_blank" style="color: var(--light-text); text-decoration: none; flex: 1; margin-right: 8px;"> | ||
| <div style="font-weight: 500; margin-bottom: 4px;">${item.title}</div> | ||
| <div style="font-size: 12px; color: var(--medium-text);">${new Date(item.timestamp).toLocaleDateString()}</div> | ||
| </a> |
There was a problem hiding this comment.
Do not inject stored favorites into innerHTML.
item.title and item.url are persisted from page DOM / window.location.href in extension/content.js. Rendering them through template strings lets a crafted title or URL break out of the markup and execute in extension context. Build these rows with DOM APIs (textContent, href, setAttribute) and validate the URL before linking.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@extension/popup.js` around lines 324 - 329, The current code builds favorite
rows by concatenating into the html string (the `html += \`...\`` block that
injects `item.title` and `item.url`) which allows stored values to break out of
markup; change the renderer in extension/popup.js to create elements with DOM
APIs instead of template strings: use document.createElement for the wrapper div
and anchor, set anchor.href via setAttribute only after validating the URL (use
the URL constructor and allow only http/https), set text nodes via textContent
for the title and date (no innerHTML), and append children with appendChild; if
URL validation fails, do not set a clickable href (or fall back to a safe
action) so untrusted data cannot execute script.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Add user question collection and bookmark feature, support categorization and quick access, pass all frontend and backend tests. Closes #19
Summary by CodeRabbit
Release Notes