feat: personal learning — favorites, study notes, and account deletion - #198
Conversation
✅ Deploy Preview for dev-bayanflow ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
github-actions
Bot
added
documentation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds Supabase-backed favorites and notes, a self-service account deletion flow, updated legal/docs/translations, and a typing guard for full-screen shortcuts. ChangesPersonal Learning Features
Full-screen Keyboard Shortcut Fix
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Preview for Bayan Flow Staging ready!
Preview alias |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (16)
supabase/functions/delete-account/index.ts (3)
57-74: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAvoid leaking raw internal error messages to clients.
Both the
deleteError.message(400) and the catch-allmessage(500) are returned verbatim to the caller. This can expose internal implementation/database details. Log the detail server-side and return a generic message to the client.🛡️ Proposed fix to avoid exposing internal errors
if (deleteError) { - return new Response(JSON.stringify({ error: deleteError.message }), { + console.error('delete-account: deleteUser failed', deleteError); + return new Response(JSON.stringify({ error: 'Account deletion failed' }), { status: 400, headers: { ...corsHeaders, 'Content-Type': 'application/json' }, }); } @@ } catch (error) { const message = error instanceof Error ? error.message : 'Unknown error'; - return new Response(JSON.stringify({ error: message }), { + console.error('delete-account: unexpected error', error); + return new Response(JSON.stringify({ error: 'Internal server error' }), { status: 500, headers: { ...corsHeaders, 'Content-Type': 'application/json' }, }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@supabase/functions/delete-account/index.ts` around lines 57 - 74, The delete-account handler is returning raw internal error text from both the deleteError path and the catch block, which can leak implementation details to clients. Update the delete-account function to keep the detailed error in server-side logging, but return a generic client-facing message for both the delete operation failure and the catch-all failure. Use the existing delete-account logic around deleteError and the catch(error) branch to locate the response handling.
8-12: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winRestrict CORS origin on the destructive delete-account endpoint.
Access-Control-Allow-Origin: '*'permits any origin to call this irreversible action. Since auth relies on a Bearer token (not cookies), CSRF risk is low, but scoping this to the app's known origin(s) is cheap hardening for a destructive endpoint.🔒 Proposed fix to restrict CORS origin
+const ALLOWED_ORIGIN = Deno.env.get('ALLOWED_ORIGIN') ?? 'https://bayanflow.com'; + const corsHeaders = { - 'Access-Control-Allow-Origin': '*', + 'Access-Control-Allow-Origin': ALLOWED_ORIGIN, 'Access-Control-Allow-Headers': 'authorization, x-client-info, apikey, content-type', };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@supabase/functions/delete-account/index.ts` around lines 8 - 12, The delete-account endpoint is using a wildcard CORS origin, which is too permissive for an irreversible action. Update the corsHeaders constant in the delete-account function to allow only the app’s known origin(s) instead of '*', and keep the existing allowed headers intact. If needed, make the origin dynamic based on the request Origin header or an allowlist, but ensure the handler’s CORS response is restricted in the delete-account endpoint logic.
6-75: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftNo tests cover this critical, irreversible endpoint.
Static analysis flags essentially the entire file as untested. Given this performs permanent account deletion, add Deno-based tests (or extract the handler logic into a testable pure function) covering the unauthorized, wrong-method, delete-error, and success paths.
Based on learnings, coverage gaps flagged by codecov/patch across lines 6-75 indicate this new file has no exercised test path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@supabase/functions/delete-account/index.ts` around lines 6 - 75, The delete-account endpoint in Deno.serve is entirely untested, so add coverage for the critical paths by either extracting the request handling into a testable helper or testing the handler directly. Focus tests on the Deno.serve flow and the key symbols createClient, supabaseAdmin.auth.getUser, and supabaseAdmin.auth.admin.deleteUser, covering wrong HTTP method, missing/invalid Authorization, failed user lookup, deleteUser error, and successful deletion with the expected status codes and JSON responses.Source: Linters/SAST tools
src/services/favoritesService.test.js (1)
1-129: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for
addFavorite's registry-validation branch andremoveOrphanFavorites.Current tests don't exercise the "Unknown algorithm" rejection in
addFavorite(favoritesService.js lines 94-96) orremoveOrphanFavoritesat all.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/favoritesService.test.js` around lines 1 - 129, The favoritesService tests are missing coverage for two important branches: the “Unknown algorithm” validation in addFavorite and the orphan cleanup path in removeOrphanFavorites. Update favoritesService.test.js to add a test that passes an invalid algorithm key into addFavorite and asserts it rejects with the unknown-algorithm error, using addFavorite and PERSONAL_LEARNING_ERRORS to locate the behavior. Also add a test for removeOrphanFavorites that seeds orphan favorites, invokes the function, and verifies the expected Supabase delete/cleanup call occurs and only valid favorites remain, referencing removeOrphanFavorites and getOrphanFavorites.src/hooks/useFavorites.test.js (1)
36-98: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for failure and slot-limit paths.
Current tests only cover happy-path hydrate/toggle. Consider adding cases for:
listFavoritesrejecting,addFavorite/removeFavoriterejecting (verifying rollback toprevious), andtoggleFavoritereturning{ reason: 'slot_limit' }whenisAtSlotLimitis true.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks/useFavorites.test.js` around lines 36 - 98, The useFavorites test suite only covers the successful hydrate and toggle flows, so add cases in useFavorites.test.js for the failure and limit branches. Extend the renderHook coverage to verify that listFavorites rejecting is handled, that toggleFavorite rolls back to the previous favorites state when addFavorite or removeFavorite rejects, and that the hook returns { reason: 'slot_limit' } when isAtSlotLimit is true. Use the existing useFavorites, listFavorites, addFavorite, removeFavorite, and isAtSlotLimit paths to locate the relevant behavior.Source: Linters/SAST tools
src/hooks/useFavorites.js (1)
32-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winError branches of
hydrate()are untested.Per the codecov hint, lines 53-54 and 56-58 (stale-request bail-out and the catch block) aren't covered by any test. Worth adding a case where
listFavoritesrejects to confirm favorites reset to[]and the error is logged rather than crashing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks/useFavorites.js` around lines 32 - 63, The error-handling paths in hydrate are missing test coverage, including the stale-request bail-out and the catch branch. Add a test for useFavorites that mocks listFavorites to reject and verifies hydrate logs the failure via console.error and resets favorites to an empty array instead of throwing. If needed, also cover the requestRef/requestId mismatch path so the early return behavior in hydrate remains exercised.Source: Linters/SAST tools
src/components/AlgorithmDropdown.jsx (1)
97-196: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd test coverage for the
favorited = truestar state.Both new tests in
AlgorithmDropdown.test.jsxonly exercise the "add to favorites" (unfavorited) path. Thefavorited === truebranches (filled star, amber color, "remove from favorites" aria-label at Lines 173, 178-180, 190) are untested, matching the codecov flags on those exact lines.✅ Suggested additional test
it('shows remove-favorite state when algorithm is already favorited', () => { renderWithI18n( <AlgorithmDropdown {...defaultProps} isDropdownOpen={true} user={{ id: 'user-1' }} isAuthenticated={true} categoryType="sorting" isFavorite={() => true} /> ); expect( screen.getByRole('button', { name: /remove bubble sort from favorites/i }) ).toBeInTheDocument(); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/AlgorithmDropdown.jsx` around lines 97 - 196, Add test coverage for the already-favorited star state in AlgorithmDropdown. Update AlgorithmDropdown.test.jsx to render the dropdown with isFavorite returning true and assert the favorited UI branches in the AlgorithmDropdown list item: the Star icon uses the filled/amber styling and the star button gets the “remove from favorites” aria-label. Use the existing AlgorithmDropdown, isFavorite, and handleStarClick behavior to locate the affected branches.src/components/FavoritesDropdown.jsx (1)
100-104: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winEmpty-state hint inside the open dropdown is untested.
The "shows empty state hint" test never opens the dropdown (no click on the toggle button), so it only verifies the closed-button label (Line 76), not this hint paragraph. Matches the codecov gap on Lines 101-103.
✅ Suggested test fix
it('shows empty state hint', async () => { render( <FavoritesDropdown favorites={[]} slotLimit={20} onSelect={vi.fn()} isPlaying={false} /> ); + + fireEvent.click(screen.getByText('settings.favoriteAlgorithmsEmpty')); expect( screen.getByText('settings.favoriteAlgorithmsHint') ).toBeInTheDocument(); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/FavoritesDropdown.jsx` around lines 100 - 104, The empty-state hint rendered by FavoritesDropdown is not being exercised because the test only checks the closed toggle label and never opens the dropdown. Update the “shows empty state hint” test to click the dropdown toggle first, then assert the hint paragraph from FavoritesDropdown is visible when count is 0. Use the component’s toggle/open behavior and the empty-state text key to target the correct UI state.src/utils/noteHtmlSanitizer.js (2)
41-41: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winTruncation can leave dangling/unbalanced HTML.
sanitized.slice(0, NOTE_MAX_HTML_LENGTH)can cut mid-tag (e.g.,<stro), storing malformed HTML that renders oddly when reloaded. Since the content is already sanitized before slicing, re-sanitizing the truncated substring is cheap and guarantees well-formed output.🩹 Proposed fix
- return sanitized.slice(0, NOTE_MAX_HTML_LENGTH); + return DOMPurify.sanitize(sanitized.slice(0, NOTE_MAX_HTML_LENGTH), { + ALLOWED_TAGS, + ALLOWED_ATTR: [], + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/noteHtmlSanitizer.js` at line 41, The truncation in noteHtmlSanitizer’s return path can leave malformed HTML if `sanitized.slice(0, NOTE_MAX_HTML_LENGTH)` cuts through a tag. Update the `sanitizeNoteHtml` flow so the truncated substring is passed back through the sanitizer (or equivalent HTML normalization step) before returning, ensuring the output remains well-formed after length limiting.
48-63: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider avoiding
innerHTMLfor text extraction on this exported utility.
getPlainTextFromNoteHtmlsetsel.innerHTML = htmldirectly. In the current call graph (viaisNoteContentEmpty/assertNotePlainTextLengthinnotesService.upsertNote),htmlis always pre-sanitized, so this isn't actively exploitable today. However, this is an exported, general-purpose function with no indication in its name/signature that it requires pre-sanitized input — a future caller invoking it on raw content would reintroduce an XSS surface (event handlers likeonerrorfire even on detached elements). UsingDOMParser, which doesn't execute scripts or fetch resources, removes this risk entirely regardless of caller.🛡️ Proposed fix
- if (typeof document !== 'undefined') { - const el = document.createElement('div'); - el.innerHTML = html; - return (el.textContent ?? '').trim(); - } + if (typeof DOMParser !== 'undefined') { + const doc = new DOMParser().parseFromString(html, 'text/html'); + return (doc.body.textContent ?? '').trim(); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/noteHtmlSanitizer.js` around lines 48 - 63, `getPlainTextFromNoteHtml` currently uses `innerHTML` on a detached element, which is unsafe for a general-purpose exported helper. Update the implementation in `noteHtmlSanitizer.js` to extract text without assigning raw HTML to `innerHTML`, preferably by parsing with `DOMParser` in the browser path and keeping the existing fallback for non-DOM environments. Preserve the current trimming/empty-input behavior, and ensure `isNoteContentEmpty` and `assertNotePlainTextLength` continue to work through the same `getPlainTextFromNoteHtml` API.Source: Linters/SAST tools
src/components/NoteEditor.jsx (2)
26-47: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winMemoize the
extensionsarray to avoid reconfiguring the editor on every keystroke.
extensionsis a fresh array (with newStarterKit/Placeholderinstances) on every render ofNoteEditor. SinceonUpdatesetseditorHtmlin the parent on every keystroke, this component re-renders on every keystroke too. Tiptap'suseEditorcompares extensions by reference identity when deciding whether to re-apply options: "We often encourage putting extensions inlined in the options object, so we will do a slightly deeper comparison here" but that comparison still treats a new extension instance as changed. The docs also advise: "Use the deps array sparingly. Only include dependencies when you actually need to recreate the editor instance... updating editor options or content through commands is more efficient." Wrapping the array inuseMemo(keyed onplaceholder) avoids unnecessary option re-application on every render.⚡ Proposed fix
-import { useEffect } from 'react'; +import { useEffect, useMemo } from 'react'; function NoteEditor({ content, placeholder, onUpdate }) { + const extensions = useMemo( + () => [ + StarterKit.configure({ + heading: { levels: [2, 3] }, + codeBlock: false, + strike: false, + horizontalRule: false, + }), + Placeholder.configure({ placeholder }), + ], + [placeholder] + ); + const editor = useEditor({ - extensions: [ - StarterKit.configure({ - heading: { levels: [2, 3] }, - codeBlock: false, - strike: false, - horizontalRule: false, - }), - Placeholder.configure({ placeholder }), - ], + extensions, content,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/NoteEditor.jsx` around lines 26 - 47, The NoteEditor hook is recreating a fresh extensions array on every render, which causes useEditor to re-apply editor options unnecessarily during typing. Memoize the extensions value in NoteEditor with useMemo, keyed on placeholder, so StarterKit.configure and Placeholder.configure are only recreated when their inputs change. Keep the onUpdate behavior unchanged, but ensure the extensions reference stays stable across parent re-renders.
63-77: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
ToolButtonis redefined on every render, forcing React to unmount/remount all toolbar buttons on every keystroke.Since
contentchanges on every keystroke (parent state update),NoteEditorre-renders, andToolButton(a new function reference each time) is treated by React as a different component type, tearing down and recreating the 5 toolbar buttons every render. Hoist it outside the component.♻️ Proposed fix
+function ToolButton({ onClick, isActive, label, children }) { + return ( + <button + type="button" + onClick={onClick} + className={`p-2 rounded-md transition-colors ${ + isActive + ? 'bg-theme-primary-light text-theme-primary' + : 'text-text-secondary hover:bg-surface hover:text-text-primary' + }`} + aria-label={label} + aria-pressed={isActive} + > + {children} + </button> + ); +} + function NoteEditor({ content, placeholder, onUpdate }) { ... - const ToolButton = ({ onClick, isActive, label, children }) => ( - <button - type="button" - onClick={onClick} - className={`p-2 rounded-md transition-colors ${ - isActive - ? 'bg-theme-primary-light text-theme-primary' - : 'text-text-secondary hover:bg-surface hover:text-text-primary' - }`} - aria-label={label} - aria-pressed={isActive} - > - {children} - </button> - );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/NoteEditor.jsx` around lines 63 - 77, The ToolButton component is being recreated inside NoteEditor on every render, so React treats it as a new component and remounts the toolbar buttons whenever content changes. Hoist ToolButton out of NoteEditor so it has a stable identity, and keep using its existing props (onClick, isActive, label, children) from the toolbar render to avoid unnecessary unmount/remount cycles.src/components/AlgorithmInsightPanel.jsx (1)
201-262: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTabs lack full ARIA tab-panel wiring (
aria-controls/id/role="tabpanel").
role="tablist"/role="tab"are present, but the content region (Line 241-263) isn't marked asrole="tabpanel"witharia-labelledbypointing back to the active tab, and theTabButtons don't setaria-controls. Screen readers won't announce the relationship between the selected tab and its panel.Also applies to: 268-285
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/AlgorithmInsightPanel.jsx` around lines 201 - 262, The tab UI in AlgorithmInsightPanel is missing the full ARIA tab/panel linkage. Update TabButton so each tab exposes aria-controls tied to its matching panel, and mark the content region rendered for InsightTabBody and AlgorithmNotesTab as role="tabpanel" with an id and aria-labelledby pointing back to the active tab. Keep the existing activeTab switching logic, but ensure the insight and notes panels each have stable ids and the selected tab references the currently shown panel.src/services/notesService.test.js (1)
81-101: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTest doesn't verify sanitized payload actually reaches
.upsert().Only the mocked DB response is asserted (hardcoded to
'<p>note</p>'), not the arguments passed toupsert. A regression that skips sanitization before the DB call would still pass this test.✅ Proposed fix: assert the actual upsert payload
expect(upsert).toHaveBeenCalled(); + expect(upsert).toHaveBeenCalledWith( + expect.objectContaining({ body_html: '<p>note</p>' }), + expect.anything() + ); expect(result?.body_html).toBe('<p>note</p>');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/notesService.test.js` around lines 81 - 101, The test in upsertNote only checks the mocked return value, so it never verifies that sanitized HTML is actually sent into .upsert(). Update the notesService.upsertNote test to inspect the arguments passed to the mocked upsert call and assert the payload contains the sanitized body_html (not the unsanitized input), while still keeping the existing result assertion from the single() response.src/hooks/useNoteAutosave.test.js (2)
23-28: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo coverage for save failure/retry behavior.
upsertNotealways resolves in this suite, so the retry/backoff path inperformSave(and its self-referential deadlock) is completely untested, matching the codecov gap onuseNoteAutosave.jslines 89-103.Want me to draft a test that rejects
upsertNoteonce and asserts a subsequent save eventually resolves/settles (which would have caught the deadlock)?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks/useNoteAutosave.test.js` around lines 23 - 28, The useNoteAutosave test suite only covers successful saves, so the retry/backoff path in performSave is untested. Add a test in useNoteAutosave.test.js that makes upsertNote reject on the first call and then succeed on a later attempt, and assert that the save flow eventually settles rather than hanging; target the performSave behavior in useNoteAutosave via the mocked upsertNote and any exposed save trigger.
30-43: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest harness can't actually change
categoryType/algorithmKeyacross renders.
overridesis captured once whenrenderAutosaveis called;rerender({ isActive: true })(line 82) only variesisActive, so this never exercises a real algorithm switch — despite the test name.🔧 Proposed fix: allow overriding algorithm key per render
function renderAutosave(overrides = {}) { return renderHook( - ({ isActive }) => + ({ isActive, algorithmKey = 'bubbleSort' }) => useNoteAutosave({ user, categoryType: ALGORITHM_TYPES.SORTING, - algorithmKey: 'bubbleSort', + algorithmKey, getContentHtml: () => contentHtml, isActive, ...overrides, }), - { initialProps: { isActive: true } } + { initialProps: { isActive: true, algorithmKey: 'bubbleSort' } } ); }Then
rerender({ isActive: true, algorithmKey: 'selectionSort' })would exercise the real flush-before-switch path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks/useNoteAutosave.test.js` around lines 30 - 43, The `renderAutosave` test helper in `useNoteAutosave.test.js` captures `categoryType` and `algorithmKey` once, so `rerender` only changes `isActive` and never tests a real algorithm switch. Update the test harness around `renderAutosave` and `renderHook` so `algorithmKey` (and `categoryType` if needed) can be supplied per render via props or overrides, then change the switch test to rerender with a different `algorithmKey` and verify the flush-before-switch path in `useNoteAutosave`.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/DEVELOPMENT.md`:
- Line 1039: The deploy command for the delete-account function is using an
invalid JWT flag override. Update the command in the deployment instructions to
omit --no-verify-jwt entirely and rely on the default verify_jwt = true behavior
from config.toml/dashboard; keep the guidance aligned with the deploy command
wording in the docs.
In `@src/components/AlgorithmInsightPanel.jsx`:
- Around line 69-82: The note flush in AlgorithmInsightPanel’s handleClose and
handleTabChange can reject and prevent the panel from closing or switching tabs.
Update these handlers to wrap notesFlushRef.current() in try/catch, keep the
existing flush behavior, and always continue to setActiveTab and call onClose
even when the save fails so a flush error does not block UI navigation.
In `@src/components/AlgorithmNotesTab.jsx`:
- Around line 38-54: The editor state in AlgorithmNotesTab is being reused
across algorithmKey changes, which can briefly show and potentially save the
previous algorithm’s note under the new key. Update the AlgorithmNotesTab effect
that syncs initialHtml from useNoteAutosave so it clears or resets editorHtml
whenever algorithmKey changes, or alternatively key the mounted tab/component by
algorithmKey in AlgorithmInsightPanel to force a fresh state per algorithm.
In `@src/components/AlgorithmNotesTab.test.jsx`:
- Around line 12-20: The NoteEditor mock is using the wrong prop API, so this
test won’t verify how AlgorithmNotesTab passes data into the real component.
Update the vi.mock for NoteEditor to destructure the actual props it receives
from AlgorithmNotesTab and mirror NoteEditor’s real interface ({ content,
placeholder, onUpdate }) so the textarea reflects content and calls onUpdate,
ensuring wiring regressions are caught.
In `@src/hooks/useNoteAutosave.js`:
- Line 102: The fire-and-forget calls to performSave in useNoteAutosave are
allowing retries-exhausted errors to escape as unhandled promise rejections.
Update each void performSave() call site to explicitly handle the returned
promise with a catch that swallows or routes already-handled failures through
existing saveStatus logic, and keep performSave’s final throw for the awaited
path only. Use performSave and the related saveStatus updates as the key symbols
when fixing all three call sites.
- Around line 138-148: The debounce timer created in scheduleSave can still fire
after the hook is unmounted, leading to late calls into performSave and state
updates from useNoteAutosave. Add an unmount cleanup in the hook to clear
debounceRef.current and reset it, using the existing
scheduleSave/debounceRef/performSave logic as the entry points so no pending
timeout survives component teardown.
- Around line 81-116: The retry path in useNoteAutosave’s performSave is
self-referential because it calls performSave again while inFlightRef.current
still holds the active savePromise, causing the in-flight guard to return the
same promise and deadlock. Update the retry logic so retries bypass or reset the
in-flight state before re-invoking performSave, and make sure the retry branch
still respects dirtyRef.current and MAX_RETRIES without awaiting the current
promise. Keep the fix localized to performSave, retryCountRef, inFlightRef, and
the finalization logic in the finally block.
In `@src/pages/VisualizerApp.jsx`:
- Around line 818-830: The favorite-limit notice in VisualizerApp.jsx has a dead
exit animation, no accessibility announcement, and duplicates slot-limit logic.
Wrap the conditional motion.div for favoriteNotice in AnimatePresence so the
exit transition runs, add an appropriate aria-live/role for screen readers, and
use the existing favoriteSlotLimit value from useFavorites(user) instead of
calling getFavoriteSlotLimit(user) again.
In `@src/services/favoritesService.js`:
- Around line 88-118: Move the favorite slot cap enforcement out of addFavorite
and into the database layer so it cannot be bypassed by direct inserts or stale
client values. Update the favorite_algorithms insert path in addFavorite to rely
on server-side validation, and add a trigger/function on the table that checks
the user’s current favorite count against their allowed slot limit before
insert. Use addFavorite, favorite_algorithms, and the existing
PERSONAL_LEARNING_ERRORS.FAVORITE_SLOT_LIMIT_REACHED handling as the key
integration points when wiring the new server-side check.
In `@supabase/migrations/20260704120000_personal_learning_favorites_notes.sql`:
- Around line 3-9: The per-user favorite cap is only enforced in the client-side
addFavorite flow, so direct writes to favorite_algorithms can bypass it. Add
server-side enforcement in the database by introducing a trigger or guarded RPC
around favorite_algorithms inserts that checks the current count against the
slot limit before allowing a new row, and wire it to the existing
favorite_algorithms table so the limit cannot be exceeded through direct
inserts.
---
Nitpick comments:
In `@src/components/AlgorithmDropdown.jsx`:
- Around line 97-196: Add test coverage for the already-favorited star state in
AlgorithmDropdown. Update AlgorithmDropdown.test.jsx to render the dropdown with
isFavorite returning true and assert the favorited UI branches in the
AlgorithmDropdown list item: the Star icon uses the filled/amber styling and the
star button gets the “remove from favorites” aria-label. Use the existing
AlgorithmDropdown, isFavorite, and handleStarClick behavior to locate the
affected branches.
In `@src/components/AlgorithmInsightPanel.jsx`:
- Around line 201-262: The tab UI in AlgorithmInsightPanel is missing the full
ARIA tab/panel linkage. Update TabButton so each tab exposes aria-controls tied
to its matching panel, and mark the content region rendered for InsightTabBody
and AlgorithmNotesTab as role="tabpanel" with an id and aria-labelledby pointing
back to the active tab. Keep the existing activeTab switching logic, but ensure
the insight and notes panels each have stable ids and the selected tab
references the currently shown panel.
In `@src/components/FavoritesDropdown.jsx`:
- Around line 100-104: The empty-state hint rendered by FavoritesDropdown is not
being exercised because the test only checks the closed toggle label and never
opens the dropdown. Update the “shows empty state hint” test to click the
dropdown toggle first, then assert the hint paragraph from FavoritesDropdown is
visible when count is 0. Use the component’s toggle/open behavior and the
empty-state text key to target the correct UI state.
In `@src/components/NoteEditor.jsx`:
- Around line 26-47: The NoteEditor hook is recreating a fresh extensions array
on every render, which causes useEditor to re-apply editor options unnecessarily
during typing. Memoize the extensions value in NoteEditor with useMemo, keyed on
placeholder, so StarterKit.configure and Placeholder.configure are only
recreated when their inputs change. Keep the onUpdate behavior unchanged, but
ensure the extensions reference stays stable across parent re-renders.
- Around line 63-77: The ToolButton component is being recreated inside
NoteEditor on every render, so React treats it as a new component and remounts
the toolbar buttons whenever content changes. Hoist ToolButton out of NoteEditor
so it has a stable identity, and keep using its existing props (onClick,
isActive, label, children) from the toolbar render to avoid unnecessary
unmount/remount cycles.
In `@src/hooks/useFavorites.js`:
- Around line 32-63: The error-handling paths in hydrate are missing test
coverage, including the stale-request bail-out and the catch branch. Add a test
for useFavorites that mocks listFavorites to reject and verifies hydrate logs
the failure via console.error and resets favorites to an empty array instead of
throwing. If needed, also cover the requestRef/requestId mismatch path so the
early return behavior in hydrate remains exercised.
In `@src/hooks/useFavorites.test.js`:
- Around line 36-98: The useFavorites test suite only covers the successful
hydrate and toggle flows, so add cases in useFavorites.test.js for the failure
and limit branches. Extend the renderHook coverage to verify that listFavorites
rejecting is handled, that toggleFavorite rolls back to the previous favorites
state when addFavorite or removeFavorite rejects, and that the hook returns {
reason: 'slot_limit' } when isAtSlotLimit is true. Use the existing
useFavorites, listFavorites, addFavorite, removeFavorite, and isAtSlotLimit
paths to locate the relevant behavior.
In `@src/hooks/useNoteAutosave.test.js`:
- Around line 23-28: The useNoteAutosave test suite only covers successful
saves, so the retry/backoff path in performSave is untested. Add a test in
useNoteAutosave.test.js that makes upsertNote reject on the first call and then
succeed on a later attempt, and assert that the save flow eventually settles
rather than hanging; target the performSave behavior in useNoteAutosave via the
mocked upsertNote and any exposed save trigger.
- Around line 30-43: The `renderAutosave` test helper in
`useNoteAutosave.test.js` captures `categoryType` and `algorithmKey` once, so
`rerender` only changes `isActive` and never tests a real algorithm switch.
Update the test harness around `renderAutosave` and `renderHook` so
`algorithmKey` (and `categoryType` if needed) can be supplied per render via
props or overrides, then change the switch test to rerender with a different
`algorithmKey` and verify the flush-before-switch path in `useNoteAutosave`.
In `@src/services/favoritesService.test.js`:
- Around line 1-129: The favoritesService tests are missing coverage for two
important branches: the “Unknown algorithm” validation in addFavorite and the
orphan cleanup path in removeOrphanFavorites. Update favoritesService.test.js to
add a test that passes an invalid algorithm key into addFavorite and asserts it
rejects with the unknown-algorithm error, using addFavorite and
PERSONAL_LEARNING_ERRORS to locate the behavior. Also add a test for
removeOrphanFavorites that seeds orphan favorites, invokes the function, and
verifies the expected Supabase delete/cleanup call occurs and only valid
favorites remain, referencing removeOrphanFavorites and getOrphanFavorites.
In `@src/services/notesService.test.js`:
- Around line 81-101: The test in upsertNote only checks the mocked return
value, so it never verifies that sanitized HTML is actually sent into .upsert().
Update the notesService.upsertNote test to inspect the arguments passed to the
mocked upsert call and assert the payload contains the sanitized body_html (not
the unsanitized input), while still keeping the existing result assertion from
the single() response.
In `@src/utils/noteHtmlSanitizer.js`:
- Line 41: The truncation in noteHtmlSanitizer’s return path can leave malformed
HTML if `sanitized.slice(0, NOTE_MAX_HTML_LENGTH)` cuts through a tag. Update
the `sanitizeNoteHtml` flow so the truncated substring is passed back through
the sanitizer (or equivalent HTML normalization step) before returning, ensuring
the output remains well-formed after length limiting.
- Around line 48-63: `getPlainTextFromNoteHtml` currently uses `innerHTML` on a
detached element, which is unsafe for a general-purpose exported helper. Update
the implementation in `noteHtmlSanitizer.js` to extract text without assigning
raw HTML to `innerHTML`, preferably by parsing with `DOMParser` in the browser
path and keeping the existing fallback for non-DOM environments. Preserve the
current trimming/empty-input behavior, and ensure `isNoteContentEmpty` and
`assertNotePlainTextLength` continue to work through the same
`getPlainTextFromNoteHtml` API.
In `@supabase/functions/delete-account/index.ts`:
- Around line 57-74: The delete-account handler is returning raw internal error
text from both the deleteError path and the catch block, which can leak
implementation details to clients. Update the delete-account function to keep
the detailed error in server-side logging, but return a generic client-facing
message for both the delete operation failure and the catch-all failure. Use the
existing delete-account logic around deleteError and the catch(error) branch to
locate the response handling.
- Around line 8-12: The delete-account endpoint is using a wildcard CORS origin,
which is too permissive for an irreversible action. Update the corsHeaders
constant in the delete-account function to allow only the app’s known origin(s)
instead of '*', and keep the existing allowed headers intact. If needed, make
the origin dynamic based on the request Origin header or an allowlist, but
ensure the handler’s CORS response is restricted in the delete-account endpoint
logic.
- Around line 6-75: The delete-account endpoint in Deno.serve is entirely
untested, so add coverage for the critical paths by either extracting the
request handling into a testable helper or testing the handler directly. Focus
tests on the Deno.serve flow and the key symbols createClient,
supabaseAdmin.auth.getUser, and supabaseAdmin.auth.admin.deleteUser, covering
wrong HTTP method, missing/invalid Authorization, failed user lookup, deleteUser
error, and successful deletion with the expected status codes and JSON
responses.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 55bfffea-9800-4e16-8148-3434d5b58725
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (43)
AGENTS.mddocs/AGENTS_REFERENCE.mddocs/DEVELOPMENT.mdpackage.jsonsrc/components/AlgorithmDropdown.jsxsrc/components/AlgorithmDropdown.test.jsxsrc/components/AlgorithmInsightPanel.jsxsrc/components/AlgorithmInsightPanel.test.jsxsrc/components/AlgorithmNotesTab.jsxsrc/components/AlgorithmNotesTab.test.jsxsrc/components/FavoritesDropdown.jsxsrc/components/FavoritesDropdown.test.jsxsrc/components/NoteEditor.jsxsrc/components/SettingsPanel.jsxsrc/constants/personalLearning.jssrc/content/legal/privacy.en.jssrc/content/legal/privacy.en.test.jssrc/content/legal/terms.en.jssrc/content/legal/terms.en.test.jssrc/hooks/useFavorites.jssrc/hooks/useFavorites.test.jssrc/hooks/useFullScreen.jssrc/hooks/useFullScreen.test.jssrc/hooks/useNoteAutosave.jssrc/hooks/useNoteAutosave.test.jssrc/i18n/locales/ar/translation.jsonsrc/i18n/locales/en/translation.jsonsrc/i18n/locales/fr/translation.jsonsrc/pages/ProfileSettingsPage.jsxsrc/pages/ProfileSettingsPage.test.jsxsrc/pages/VisualizerApp.jsxsrc/services/__tests__/entitlementService.test.jssrc/services/authService.jssrc/services/authService.test.jssrc/services/entitlementService.jssrc/services/favoritesService.jssrc/services/favoritesService.test.jssrc/services/notesService.jssrc/services/notesService.test.jssrc/test/supabaseMock.jssrc/utils/noteHtmlSanitizer.jssupabase/functions/delete-account/index.tssupabase/migrations/20260704120000_personal_learning_favorites_notes.sql
| const [editorHtml, setEditorHtml] = useState(''); | ||
| const getContentHtml = useCallback(() => editorHtml, [editorHtml]); | ||
|
|
||
| const { initialHtml, isLoadingNote, saveStatus, scheduleSave, flushSave } = | ||
| useNoteAutosave({ | ||
| user, | ||
| categoryType, | ||
| algorithmKey, | ||
| getContentHtml, | ||
| isActive, | ||
| }); | ||
|
|
||
| useEffect(() => { | ||
| if (!isLoadingNote) { | ||
| setEditorHtml(initialHtml); | ||
| } | ||
| }, [initialHtml, isLoadingNote, categoryType, algorithmKey]); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the relevant components and parent switch logic.
git ls-files 'src/components/*Algorithm*' 'src/components/*Notes*' 'src/components/*Insight*' 'src/components/*VisualizerApp*' 'src/components/*Category*' | sed 's#^`#FILE` #'
printf '\n--- matches for AlgorithmNotesTab / AlgorithmInsightPanel ---\n'
rg -n "AlgorithmNotesTab|AlgorithmInsightPanel|notes tab|insight panel" src/components src -g '!**/node_modules/**' || true
printf '\n--- file outlines ---\n'
for f in \
src/components/AlgorithmNotesTab.jsx \
src/components/AlgorithmInsightPanel.jsx \
src/components/VisualizerApp.jsx \
src/components/*Category*Visualizer*.jsx \
src/components/*Category*.jsx
do
if [ -f "$f" ]; then
echo "### $f"
ast-grep outline "$f" --view expanded || true
fi
doneRepository: ayoub3bidi/bayan-flow
Length of output: 8353
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the note tab and insight panel implementations.
sed -n '1,220p' src/components/AlgorithmNotesTab.jsx
printf '\n--- AlgorithmInsightPanel excerpt ---\n'
sed -n '1,340p' src/components/AlgorithmInsightPanel.jsx
printf '\n--- VisualizerApp excerpt around panel rendering ---\n'
sed -n '840,980p' src/pages/VisualizerApp.jsxRepository: ayoub3bidi/bayan-flow
Length of output: 15712
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
files = [
Path("src/components/AlgorithmNotesTab.jsx"),
Path("src/components/AlgorithmInsightPanel.jsx"),
Path("src/pages/VisualizerApp.jsx"),
]
for path in files:
print(f"\n### {path}\n")
text = path.read_text()
lines = text.splitlines()
if path.name == "AlgorithmNotesTab.jsx":
start, end = 1, min(len(lines), 220)
elif path.name == "AlgorithmInsightPanel.jsx":
start, end = 1, min(len(lines), 340)
else:
# focus on render around the lazy component
for i, line in enumerate(lines, 1):
if "AlgorithmInsightPanel" in line:
start = max(1, i - 40)
end = min(len(lines), i + 80)
break
else:
start, end = 1, min(len(lines), 220)
for i in range(start, end + 1):
print(f"{i:4d}: {lines[i-1]}")
PYRepository: ayoub3bidi/bayan-flow
Length of output: 20182
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the exact JSX around the insight panel and the note tab.
grep -n "AlgorithmInsightPanel" -n src/pages/VisualizerApp.jsx
printf '\n--- VisualizerApp around AlgorithmInsightPanel ---\n'
sed -n '880,950p' src/pages/VisualizerApp.jsx
printf '\n--- AlgorithmInsightPanel around AlgorithmNotesTab ---\n'
grep -n "AlgorithmNotesTab" src/components/AlgorithmInsightPanel.jsx
sed -n '230,280p' src/components/AlgorithmInsightPanel.jsx
printf '\n--- AlgorithmNotesTab full file if small enough ---\n'
wc -l src/components/AlgorithmNotesTab.jsx
sed -n '1,180p' src/components/AlgorithmNotesTab.jsxRepository: ayoub3bidi/bayan-flow
Length of output: 6549
src/components/AlgorithmNotesTab.jsx:50-54 — Clear the editor state on algorithm change. AlgorithmInsightPanel keeps the same AlgorithmNotesTab mounted while algorithmKey changes, so this effect can re-apply the previous initialHtml before the new note finishes loading. That can briefly show the old algorithm’s note and save it under the new key; reset editorHtml on key change or key the tab by algorithmKey to close the gap.
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] 51-51: Avoid using the initial state variable in setState
Context: setEditorHtml(initialHtml)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(setstate-same-var)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/AlgorithmNotesTab.jsx` around lines 38 - 54, The editor state
in AlgorithmNotesTab is being reused across algorithmKey changes, which can
briefly show and potentially save the previous algorithm’s note under the new
key. Update the AlgorithmNotesTab effect that syncs initialHtml from
useNoteAutosave so it clears or resets editorHtml whenever algorithmKey changes,
or alternatively key the mounted tab/component by algorithmKey in
AlgorithmInsightPanel to force a fresh state per algorithm.
| export async function addFavorite( | ||
| userId, | ||
| category, | ||
| algorithmKey, | ||
| { slotLimit, currentCount } | ||
| ) { | ||
| if (!isAlgorithmInRegistry(category, algorithmKey)) { | ||
| throw new Error('Unknown algorithm'); | ||
| } | ||
|
|
||
| if (currentCount >= slotLimit) { | ||
| const error = new Error('Favorite slot limit reached'); | ||
| error.code = PERSONAL_LEARNING_ERRORS.FAVORITE_SLOT_LIMIT_REACHED; | ||
| throw error; | ||
| } | ||
|
|
||
| const supabase = getSupabaseClient(); | ||
| if (!supabase) { | ||
| throw new Error('Supabase is not configured'); | ||
| } | ||
|
|
||
| const { error } = await supabase.from('favorite_algorithms').insert({ | ||
| user_id: userId, | ||
| category, | ||
| algorithm_key: algorithmKey, | ||
| }); | ||
|
|
||
| if (error) { | ||
| throw error; | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd personal_learning_favorites_notes supabase/migrations --exec cat {} \;Repository: ayoub3bidi/bayan-flow
Length of output: 2503
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== favorites references ==\n'
rg -n "favorite_algorithms|FAVORITE_SLOT_LIMIT_REACHED|slotLimit|currentCount|favorite slot" .
printf '\n== migration files mentioning favorite_algorithms ==\n'
fd -a favorite_algorithms supabase/migrations -x sh -c 'echo "--- {}"; sed -n "1,220p" "{}"'Repository: ayoub3bidi/bayan-flow
Length of output: 5271
Enforce the favorite slot cap server-side addFavorite only checks caller-provided slotLimit/currentCount; public.favorite_algorithms has ownership RLS but no count-based constraint, so a direct insert can exceed the tier limit. Move the cap into a trigger/function on the table so stale or untrusted clients can’t bypass it.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/services/favoritesService.js` around lines 88 - 118, Move the favorite
slot cap enforcement out of addFavorite and into the database layer so it cannot
be bypassed by direct inserts or stale client values. Update the
favorite_algorithms insert path in addFavorite to rely on server-side
validation, and add a trigger/function on the table that checks the user’s
current favorite count against their allowed slot limit before insert. Use
addFavorite, favorite_algorithms, and the existing
PERSONAL_LEARNING_ERRORS.FAVORITE_SLOT_LIMIT_REACHED handling as the key
integration points when wiring the new server-side check.
| create table public.favorite_algorithms ( | ||
| user_id uuid not null references auth.users (id) on delete cascade, | ||
| category text not null, | ||
| algorithm_key text not null, | ||
| created_at timestamptz not null default now(), | ||
| primary key (user_id, category, algorithm_key) | ||
| ); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether any existing migration already enforces a per-user row-count limit.
rg -n "trigger|count\(" supabase/migrationsRepository: ayoub3bidi/bayan-flow
Length of output: 451
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== relevant migration =="
sed -n '1,220p' supabase/migrations/20260704120000_personal_learning_favorites_notes.sql
echo
echo "== favorites service =="
sed -n '1,220p' src/services/favoritesService.js
echo
echo "== search for favorite_algorithms policies/triggers/checks =="
rg -n -C 3 "favorite_algorithms|slotLimit|currentCount|auth\.uid\(\)|policy|trigger|check" supabase/migrations src/servicesRepository: ayoub3bidi/bayan-flow
Length of output: 25920
Enforce the favorite slot cap server-side RLS only checks ownership here; the currentCount/slotLimit check in addFavorite() is client-controlled, so direct inserts can exceed the per-user limit. Add a database trigger or RPC guard if this cap must hold.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@supabase/migrations/20260704120000_personal_learning_favorites_notes.sql`
around lines 3 - 9, The per-user favorite cap is only enforced in the
client-side addFavorite flow, so direct writes to favorite_algorithms can bypass
it. Add server-side enforcement in the database by introducing a trigger or
guarded RPC around favorite_algorithms inserts that checks the current count
against the slot limit before allowing a new row, and wire it to the existing
favorite_algorithms table so the limit cannot be exceeded through direct
inserts.
…mponents - Add 4 tests for NoteEditor (toolbar, useEditor, onUpdate, null editor) - Add 5 tests for useFavorites (unauthenticated, slot limit, error, rollback add/remove) - Add 4 tests for AlgorithmNotesTab (loading, saving/saved/error/dirty save status) - Add 2 tests for AlgorithmInsightPanel (tab switching, tab reset on close) - Add 2 tests for AlgorithmDropdown (favorited label, locked star hiding) - Add 3 tests for useNoteAutosave (error on load, dirty status, empty user) - Remove 3 flaky tests (NoteEditor content sync, autosave retry/timeout) - Exclude supabase/functions/ from Vitest coverage (Deno Edge Functions)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/hooks/useFavorites.test.js (1)
45-146: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider adding a stale-response race test.
useFavoritesguards against out-of-order responses viarequestRef(bumped on each hydrate call, checked before applying results). None of the current tests exercise rapid user switches that would trigger this guard (e.g., rerender with a second user before the firstlistFavoritespromise resolves). Given the PR's focus on expanding hook test coverage, this would close a coverage gap on a concurrency-sensitive path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks/useFavorites.test.js` around lines 45 - 146, Add a test in useFavorites.test.js that covers the stale-response race handled by requestRef in useFavorites. Simulate a slow listFavorites call for the first user, rerender the hook with a second user before it resolves, then resolve the first request and verify its result is ignored and only the latest hydrate updates favorites. Use the existing renderHook, rerender, listFavorites, and requestRef behavior as the target path.src/components/NoteEditor.test.jsx (1)
72-104: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
mockUseEditor.mockImplementationleaks across tests.
vi.clearAllMocks()inbeforeEachclears call history but not custom implementations set viamockImplementation. The custom implementation set in "calls onUpdate when editor emits change" (Lines 76-92) will persist into subsequent tests unless explicitly overridden (as the next test happens to do viamockReturnValueOnce). This creates a fragile, order-dependent suite — reordering or adding tests after this one could silently pick up the stale implementation.♻️ Proposed fix
beforeEach(() => { - vi.clearAllMocks(); + vi.resetAllMocks(); + mockUseEditor.mockReturnValue(editor); });Or restore the default implementation at the end of the affected test using
mockUseEditor.mockImplementation(defaultImpl).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/NoteEditor.test.jsx` around lines 72 - 104, The mockUseEditor.mockImplementation set in the NoteEditor test is leaking into later tests because clearAllMocks does not reset custom implementations. In the "calls onUpdate when editor emits change" test, save the default mock behavior and restore it after the test, or use a scoped override/reset so NoteEditor and mockUseEditor do not keep the custom onUpdate-capturing implementation beyond that case.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/AlgorithmDropdown.test.jsx`:
- Line 7: The test file imports beforeEach from vitest but never uses it, so
remove the unused beforeEach symbol from the import in
AlgorithmDropdown.test.jsx while keeping the other Vitest helpers intact.
In `@src/components/AlgorithmInsightPanel.test.jsx`:
- Around line 81-110: The “resets tab to insight when reopened” test in
AlgorithmInsightPanel does not verify reopening behavior because it only
rerenders with isOpen=false and never asserts after reopening. Update the test
to close the panel and then reopen it with isOpen=true, then assert the active
tab resets to the default insight tab instead of remaining on “My Notes”; use
the existing AlgorithmInsightPanel render, rerender, and tab assertions to
validate the reopen state.
---
Nitpick comments:
In `@src/components/NoteEditor.test.jsx`:
- Around line 72-104: The mockUseEditor.mockImplementation set in the NoteEditor
test is leaking into later tests because clearAllMocks does not reset custom
implementations. In the "calls onUpdate when editor emits change" test, save the
default mock behavior and restore it after the test, or use a scoped
override/reset so NoteEditor and mockUseEditor do not keep the custom
onUpdate-capturing implementation beyond that case.
In `@src/hooks/useFavorites.test.js`:
- Around line 45-146: Add a test in useFavorites.test.js that covers the
stale-response race handled by requestRef in useFavorites. Simulate a slow
listFavorites call for the first user, rerender the hook with a second user
before it resolves, then resolve the first request and verify its result is
ignored and only the latest hydrate updates favorites. Use the existing
renderHook, rerender, listFavorites, and requestRef behavior as the target path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d3e75686-8a18-40df-8373-6e39ae2655e9
📒 Files selected for processing (7)
src/components/AlgorithmDropdown.test.jsxsrc/components/AlgorithmInsightPanel.test.jsxsrc/components/AlgorithmNotesTab.test.jsxsrc/components/NoteEditor.test.jsxsrc/hooks/useFavorites.test.jssrc/hooks/useNoteAutosave.test.jsvitest.config.js
…tor, and autosave - Add FavoritesDropdown tests: open empty state, disabled when playing - Add AlgorithmInsightPanel tests: no insight content, no year in MetaItem - Add NoteEditor tests: setContent on mount when different, skip when same - Add useNoteAutosave tests: online event trigger, unmount flush, algorithm key switch - Refactor useNoteAutosave test helper to support algorithmKey via rerender props
There was a problem hiding this comment.
🧹 Nitpick comments (4)
src/hooks/useNoteAutosave.test.js (2)
197-222: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTest doesn't verify the "flushes" half of its name.
The test asserts
getNoteis called with the newalgorithmKey(reload), but never asserts thatupsertNotewas called to flush the dirty content from the previousalgorithmKeybefore reloading, even though the test title is "flushes and reloads when algorithmKey changes."✅ Suggested addition
rerender({ isActive: true, algorithmKey: 'selectionSort' }); + await waitFor(() => { + expect(upsertNote).toHaveBeenCalledWith( + 'user-1', + ALGORITHM_TYPES.SORTING, + 'bubbleSort', + '<p>updated</p>' + ); + }); + await waitFor(() => { expect(getNote).toHaveBeenCalledWith( 'user-1', ALGORITHM_TYPES.SORTING, 'selectionSort' ); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks/useNoteAutosave.test.js` around lines 197 - 222, The test for useAutosave’s algorithmKey change path only checks the reload behavior and misses the flush behavior in its name. Update the “flushes and reloads when algorithmKey changes” case to also assert that the pending save is flushed by calling upsertNote before getNote reloads the new note. Use the existing scheduleSave, rerender, upsertNote, and getNote setup in the test to verify both calls occur in the expected sequence.
150-172: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueState update outside
act()ononlinedispatch.
window.dispatchEvent(new Event('online'))triggersperformSave, which synchronously callssetSaveStatus('saving'), outsideact(). The subsequentwaitFormasks failures but React may emit act-warnings in the console.🧪 Suggested wrap
- window.dispatchEvent(new Event('online')); + act(() => { + window.dispatchEvent(new Event('online')); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks/useNoteAutosave.test.js` around lines 150 - 172, The online event in useNoteAutosave.test.js is firing a state update outside React’s act boundary because window.dispatchEvent(new Event('online')) immediately triggers performSave and setSaveStatus('saving'). Wrap the online dispatch for this test in act() within the existing renderAutosave / scheduleSave flow so the state transition is flushed before the waitFor assertion and the test avoids act warnings.src/components/AlgorithmInsightPanel.test.jsx (1)
131-146: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTest doesn't verify "without year" behavior.
The test name claims to check that MetaItem renders without a year, but it only asserts the algorithm name renders and a tablist exists — it never checks for the absence of the year label/value (or presence of the inventor-only meta row).
✅ Suggested addition
const nameElements = screen.getAllByText('Insertion Sort'); expect(nameElements.length).toBeGreaterThan(0); expect(screen.getAllByRole('tablist').length).toBeGreaterThan(0); + expect(screen.queryByText('Year')).not.toBeInTheDocument();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/AlgorithmInsightPanel.test.jsx` around lines 131 - 146, The test named for AlgorithmInsightPanel should actually validate the “without year” case instead of only checking the title and tablist. Update the test around AlgorithmInsightPanel/renderWithI18n to assert that no year label/value is rendered when the algorithm year is null, and optionally verify the inventor-only MetaItem row appears. Use the existing symbols AlgorithmInsightPanel, renderWithI18n, and screen to locate and strengthen the assertion.src/components/NoteEditor.test.jsx (1)
115-139: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated mock editor scaffolding.
matchedEditorre-implements the same chain/toggle/getHTML/commands shape presumably already produced by the hoisteduseEditormock. Extracting a sharedcreateMockEditor(overrides)factory would reduce duplication and keep both tests in sync if the editor mock shape changes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/NoteEditor.test.jsx` around lines 115 - 139, The test in NoteEditor.test.jsx duplicates the same mocked editor shape in matchedEditor instead of reusing the shared useEditor mock setup. Extract the common chain/isActive/getHTML/commands structure into a createMockEditor(overrides) helper and use it here with only the getHTML/content overrides needed, so the NoteEditor test stays aligned with any future editor mock shape changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/components/AlgorithmInsightPanel.test.jsx`:
- Around line 131-146: The test named for AlgorithmInsightPanel should actually
validate the “without year” case instead of only checking the title and tablist.
Update the test around AlgorithmInsightPanel/renderWithI18n to assert that no
year label/value is rendered when the algorithm year is null, and optionally
verify the inventor-only MetaItem row appears. Use the existing symbols
AlgorithmInsightPanel, renderWithI18n, and screen to locate and strengthen the
assertion.
In `@src/components/NoteEditor.test.jsx`:
- Around line 115-139: The test in NoteEditor.test.jsx duplicates the same
mocked editor shape in matchedEditor instead of reusing the shared useEditor
mock setup. Extract the common chain/isActive/getHTML/commands structure into a
createMockEditor(overrides) helper and use it here with only the getHTML/content
overrides needed, so the NoteEditor test stays aligned with any future editor
mock shape changes.
In `@src/hooks/useNoteAutosave.test.js`:
- Around line 197-222: The test for useAutosave’s algorithmKey change path only
checks the reload behavior and misses the flush behavior in its name. Update the
“flushes and reloads when algorithmKey changes” case to also assert that the
pending save is flushed by calling upsertNote before getNote reloads the new
note. Use the existing scheduleSave, rerender, upsertNote, and getNote setup in
the test to verify both calls occur in the expected sequence.
- Around line 150-172: The online event in useNoteAutosave.test.js is firing a
state update outside React’s act boundary because window.dispatchEvent(new
Event('online')) immediately triggers performSave and setSaveStatus('saving').
Wrap the online dispatch for this test in act() within the existing
renderAutosave / scheduleSave flow so the state transition is flushed before the
waitFor assertion and the test avoids act warnings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8d031179-9f3e-419f-8215-028f15adf0db
📒 Files selected for processing (4)
src/components/AlgorithmInsightPanel.test.jsxsrc/components/FavoritesDropdown.test.jsxsrc/components/NoteEditor.test.jsxsrc/hooks/useNoteAutosave.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/FavoritesDropdown.test.jsx
- 🔴 Fix critical self-referential deadlock in useNoteAutosave retry path - 🟠 Wrap flush calls in try/catch so errors don't block UI - 🟠 Catch unhandled rejections from void performSave() call sites - 🟠 Add server-side favorite slot limit enforcement (DB trigger) - 🟡 Add debounce timer cleanup on unmount - 🟡 Fix mock prop names in AlgorithmNotesTab.test.jsx - 🟡 Complete reset-on-reopen test in AlgorithmInsightPanel.test.jsx - 🟡 Add flush assertion in useNoteAutosave algorithmKey change test - 🟡 Wrap online event dispatch in act() in useNoteAutosave.test.js - 🟡 Fix favorite notice: wrap in AnimatePresence, add aria-live, reuse slotLimit - 🔵 Remove unused beforeEach import in AlgorithmDropdown.test.jsx - 🔵 Fix mock leak in NoteEditor.test.jsx (resetAllMocks + restore default) - 🔵 Add stale-response race test in useFavorites.test.js - 🔵 Harden delete-account Edge Function (generic errors, scoped CORS, extract handler) - 🔵 Fix docs/DEVELOPMENT.md JWT flag wording - Use refs for save category/key to ensure flush uses old key before switch
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
supabase/migrations/20260705120000_enforce_favorite_slot_limit.sql (1)
4-24: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueConsider pinning
search_pathon these functions.Neither function sets
SET search_path, which the Supabase linter flags asfunction_search_path_mutable. Actual risk here is low since all objects are fully qualified, but addingset search_path = ''(or= public) hardens against search-path hijacking and clears the linter warning.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@supabase/migrations/20260705120000_enforce_favorite_slot_limit.sql` around lines 4 - 24, Pin the search path on both public.get_favorite_slot_limit and public.check_favorite_slot_limit to satisfy the Supabase linter and harden against search-path hijacking. Update these function definitions to include an explicit SET search_path clause (for example, empty or public) while keeping the existing fully qualified references intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@supabase/migrations/20260705120000_enforce_favorite_slot_limit.sql`:
- Around line 13-24: The trigger function check_favorite_slot_limit currently
checks count(*) without serializing concurrent inserts, so two sessions can both
pass and exceed the cap. Add a per-user transaction advisory lock at the start
of check_favorite_slot_limit, keyed by new.user_id, so all inserts for the same
user are serialized before the favorite_algorithms count is evaluated. Keep the
existing limit check and exception behavior unchanged after the lock is
acquired.
---
Nitpick comments:
In `@supabase/migrations/20260705120000_enforce_favorite_slot_limit.sql`:
- Around line 4-24: Pin the search path on both public.get_favorite_slot_limit
and public.check_favorite_slot_limit to satisfy the Supabase linter and harden
against search-path hijacking. Update these function definitions to include an
explicit SET search_path clause (for example, empty or public) while keeping the
existing fully qualified references intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5cb2e173-7264-4e0c-84d9-a9f2914b4e3b
📒 Files selected for processing (13)
docs/DEVELOPMENT.mdsrc/components/AlgorithmDropdown.test.jsxsrc/components/AlgorithmInsightPanel.jsxsrc/components/AlgorithmInsightPanel.test.jsxsrc/components/AlgorithmNotesTab.test.jsxsrc/components/NoteEditor.test.jsxsrc/hooks/useFavorites.test.jssrc/hooks/useNoteAutosave.jssrc/hooks/useNoteAutosave.test.jssrc/pages/VisualizerApp.jsxsrc/pages/VisualizerApp.test.jsxsupabase/functions/delete-account/index.tssupabase/migrations/20260705120000_enforce_favorite_slot_limit.sql
✅ Files skipped from review due to trivial changes (1)
- docs/DEVELOPMENT.md
🚧 Files skipped from review as they are similar to previous changes (9)
- supabase/functions/delete-account/index.ts
- src/components/AlgorithmInsightPanel.test.jsx
- src/components/NoteEditor.test.jsx
- src/components/AlgorithmNotesTab.test.jsx
- src/hooks/useFavorites.test.js
- src/hooks/useNoteAutosave.test.js
- src/pages/VisualizerApp.jsx
- src/hooks/useNoteAutosave.js
- src/components/AlgorithmInsightPanel.jsx
- Pin search_path on both functions (Supabase linter hardening) - Add per-user pg_advisory_xact_lock to serialize concurrent inserts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
supabase/migrations/20260705120000_enforce_favorite_slot_limit.sql (1)
20-20: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider the two-argument advisory-lock form to avoid cross-feature key collisions.
The single-key
pg_advisory_xact_lock(bigint)shares one global lock space, so thehashtext(...)value could collide with advisory locks taken elsewhere in the app. Using the two-argument formpg_advisory_xact_lock(classid int, objid int)lets you reserve a dedicated namespace for this feature.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@supabase/migrations/20260705120000_enforce_favorite_slot_limit.sql` at line 20, The favorite-slot limit trigger is using the single-key pg_advisory_xact_lock via hashtext(...), which can collide with unrelated advisory locks. Update the locking in the migration’s trigger/function that enforces the favorite slot limit to use the two-argument pg_advisory_xact_lock(classid, objid) form instead, choosing a dedicated namespace for this feature while keeping the lock tied to new.user_id in the existing favorite_slot_* logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@supabase/migrations/20260705120000_enforce_favorite_slot_limit.sql`:
- Line 20: The favorite-slot limit trigger is using the single-key
pg_advisory_xact_lock via hashtext(...), which can collide with unrelated
advisory locks. Update the locking in the migration’s trigger/function that
enforces the favorite slot limit to use the two-argument
pg_advisory_xact_lock(classid, objid) form instead, choosing a dedicated
namespace for this feature while keeping the lock tied to new.user_id in the
existing favorite_slot_* logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e6284899-cf99-4e7c-a723-ec652aa3f2b7
📒 Files selected for processing (1)
supabase/migrations/20260705120000_enforce_favorite_slot_limit.sql
1 participant
Contribution workflow
develop: This PR targetsdevelop, notmain.Description
Adds personal learning features for signed-in users: star algorithms as favorites (20-slot limit on Free tier), write per-algorithm study notes inside the Algorithm Insight panel, and delete your account from Profile Settings. Anonymous users can still use the visualizer; favorites require sign-in and notes inherit the existing Insight panel gate.
The Insight panel is refactored into a Learn | My Notes tabbed layout (Code Panel–style segmented tabs at the top). TipTap loads lazily on first visit to My Notes. Favorites appear first in the Settings panel with a global favorites dropdown and per-row star toggles in the algorithm picker.
Type of Change
Related Issues
Fixes #
Changes Made
Favorites (sign-in only)
src/services/favoritesService.js— list/add/remove, slot limit errors, registry orphan cleanupsrc/hooks/useFavorites.js— hydrate on sign-in, optimistic toggle, slot trackingsrc/constants/personalLearning.js— Free: 20 slots; Pro placeholder: 100src/services/entitlementService.js—getFavoriteSlotLimit(user)src/components/FavoritesDropdown.jsx— global list with category labels and slot counter (top of Settings panel)src/components/AlgorithmDropdown.jsx— star toggle (hover on desktop, always visible on mobile / when favorited)src/pages/VisualizerApp.jsx—handleFavoriteNavigate, slot-full toast, sign-in gate viaSignInPromptModalStudy notes (Insight panel)
src/components/AlgorithmInsightPanel.jsx— tabs-first layout; Learn | My Notes (Learn tab useslearnTitlei18n key)src/components/AlgorithmNotesTab.jsx+src/components/NoteEditor.jsx— lazy TipTap editor, minimal toolbarsrc/hooks/useNoteAutosave.js— debounced save, flush on tab/algorithm switch, offline retrysrc/services/notesService.js+src/utils/noteHtmlSanitizer.js— upsert/delete-on-empty, DOMPurify allowlist@tiptap/react,@tiptap/starter-kit,@tiptap/extension-placeholder,isomorphic-dompurifyAccount deletion
supabase/functions/delete-account/index.ts— JWT verify +auth.admin.deleteUser(CASCADE profile, favorites, notes)src/services/authService.js—deleteAccount()→functions.invoke('delete-account')then sign-outsrc/pages/ProfileSettingsPage.jsx— danger zone with type-DELETEconfirmationDatabase (Supabase)
supabase/migrations/20260704120000_personal_learning_favorites_notes.sql—favorite_algorithms,algorithm_notes, RLS, grantsbayan-flow(qketsapzqpzmccljfjcm); Edge Functiondelete-accountdeployed (v1,verify_jwt: true)UX fixes bundled in this PR
src/hooks/useFullScreen.js— ignoreF/Escapeshortcuts while typing in inputs or TipTap (contenteditable)src/hooks/useNoteAutosave.js— stableuser.iddeps; no editor unmount flash during autosave reloadDocs, legal, i18n
AGENTS.md— Free tier contracts for favorites (20 slots), notes, account deletiondocs/AGENTS_REFERENCE.md— tables, services, hooks, tests catalogdocs/DEVELOPMENT.md— Edge Function deploy stepssrc/content/legal/privacy.en.js— favorites/notes storage, self-service deletion, slot limitsrc/content/legal/terms.en.js— User Content section for notesTests
favoritesService,notesService,useFavorites,useNoteAutosave,useFullScreen,FavoritesDropdown,AlgorithmNotesTab, plus component/service test updatessrc/test/supabaseMock.js— insert/delete/upsert/order,functions.invokeAlgorithm Details (if applicable)
N/A — no new algorithms.
Testing
pnpm test:coverage)Test Results
Bundle note:
NoteEditor-*.js≈ 136 KB gzip (lazy-loaded; not in main bundle).Manual verification
Prerequisites: Signed-in Google user on dev/staging with Supabase env vars configured.
Favorites
1 / 20); at 20 slots, unfavorited star shows slot-full messagefavoritefeature key)Notes
Saving…/Saved)fdoes not toggle fullscreenar): editordir="auto", toolbar LTRAccount deletion
DELETE→ account removed, session clearedScreenshots/GIFs
Add screenshots from staging QA before merge if desired.
Code Quality
pnpm lint)pnpm format:check)Performance Impact
NoteEditor); Insight panel already lazy fromVisualizerAppAccessibility
aria-pressed; fullscreen shortcuts suppressed while editing)aria-label, tab labels, save statusaria-live)Breaking Changes
Checklist
delete-accountEdge Function deployed toqketsapzqpzmccljfjcmAdditional Notes
Explicitly out of scope
Deploy / ops checklist for reviewer
personal_learning_favorites_notesis applied on Supabase (already on live project)delete-accountis active with JWT verification enabledsrc/content/legal/)Reviewer Guidelines:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation