feat(site): add auth, reading progress, and streak tracking - #396
Open
AshfaqAIML wants to merge 1 commit into
Open
feat(site): add auth, reading progress, and streak tracking#396AshfaqAIML wants to merge 1 commit into
AshfaqAIML wants to merge 1 commit into
Conversation
Add lightweight client-side features that enhance the learning experience: - auth.js: Signup/login/logout with SHA-256 password hashing via Web Crypto, session persistence, profile menu in header - reading-progress.js: Auto-saves scroll position, reading time, and completion percentage per lesson. Continue Reading card on homepage. - streak.js: GitHub-style reading streak tracking (consecutive days, longest streak, total days, configurable minimum reading time) - styles: Auth modal, profile dropdown, continue card, streak widget, bottom reading progress bar — all using existing CSS design tokens - Hooks into index.html and lesson.html with minimal additions; no existing code modified or replaced
📝 WalkthroughWalkthroughAdds browser-side authentication with cross-tab sessions, per-user lesson progress, reading streaks, authentication controls, progress widgets, and lesson scroll tracking across the site pages. ChangesAuthentication and reading engagement
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant PageUI
participant AIFSAuth
participant localStorage
participant AIFSReadingProgress
participant AIFSStreak
PageUI->>AIFSAuth: Submit signup or login credentials
AIFSAuth->>localStorage: Store user and session
AIFSAuth-->>PageUI: Update authentication state
PageUI->>AIFSReadingProgress: Start lesson tracking
AIFSReadingProgress->>localStorage: Store scroll and reading time
AIFSReadingProgress->>AIFSStreak: Update qualifying reading minutes
AIFSStreak->>localStorage: Store streak statistics
AIFSReadingProgress-->>PageUI: Notify progress changes
AIFSStreak-->>PageUI: Notify streak changes
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 Warning |
![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 (2)
site/reading-progress.js (1)
23-28: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
SAVE_INTERVAL_MSnever drives a timer, andsaveTimerstays unused.
saveTimeris declared at line 25 and cleared at line 138, but no code assigns it. Progress is persisted only on scroll events and onbeforeunload. A reader who does not scroll records no reading time until unload, andbeforeunloadis unreliable on mobile browsers.Add a periodic save with
setIntervalassigned tosaveTimer, and also persist onvisibilitychange. Alternatively removesaveTimerif the current behavior is intended.Also applies to: 112-129
🤖 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 `@site/reading-progress.js` around lines 23 - 28, The reading-progress logic does not use SAVE_INTERVAL_MS or assign saveTimer, so passive readers are not persisted reliably. Update the initialization/lifecycle flow around saveTimer and the existing save routine to start a setInterval using SAVE_INTERVAL_MS, persist progress on visibilitychange, and clear the interval during teardown alongside the existing cleanup.site/lesson.html (1)
3813-3821: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse the scroll percentage calculation from
reading-progress.js.This block repeats
calcScrollPctfromsite/reading-progress.jslines 162-166. Two copies of the same formula will drift.Export the helper from the module and call it here.
♻️ Proposed change
In
site/reading-progress.js, add the helper to the public API:window.AIFSReadingProgress = { getProgress: getProgress, + calcScrollPct: calcScrollPct,Then use it here:
var progressBar = document.getElementById('readingProgressFill'); if (progressBar) { window.addEventListener('scroll', function () { - var scrollTop = window.scrollY || document.documentElement.scrollTop; - var docHeight = document.documentElement.scrollHeight - window.innerHeight; - var pct = docHeight > 0 ? Math.min(100, (scrollTop / docHeight) * 100) : 0; - progressBar.style.width = pct + '%'; + progressBar.style.width = window.AIFSReadingProgress.calcScrollPct() + '%'; }, { passive: 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 `@site/lesson.html` around lines 3813 - 3821, Export the existing calcScrollPct helper from the reading-progress module, then update the scroll handler that manages readingProgressFill to call that shared helper instead of duplicating the scroll percentage formula; preserve the current width update and passive listener behavior.
🤖 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 `@site/auth.js`:
- Around line 38-42: Update site/auth.js lines 38-42 in saveUsers to return the
write result or rethrow storage errors instead of swallowing them; update
site/auth.js lines 53-62 so signup() and login() report failure and do not
return success when session persistence fails; update site/streak.js lines 44-49
so update and reset propagate failed writes and notify listeners only after
persistence succeeds.
- Around line 112-124: Serialize all localStorage read-modify-write operations
to prevent stale whole-map writes across tabs. In site/auth.js lines 112-124,
move the users read, duplicate check, mutation, and save into one transaction
that does not span the password-hashing await; in site/streak.js lines 57-60,
130-158, and 162-165, route streak updates and resets through the same
transaction mechanism, including each operation’s read, calculation or mutation,
and save.
- Around line 81-90: Replace the single SHA-256 derivation in hashPassword with
a trusted, memory-hard password KDF and store only its verifier parameters and
output in the local credential record; never persist reusable passwords. Update
the corresponding authentication verification and user-creation flows in
site/auth.js to use the KDF format consistently, including a per-user salt and
appropriate work factors, while preserving the existing credential behavior.
In `@site/index.html`:
- Around line 1832-1921: The authentication controller and modal are duplicated
across both pages and must be centralized. In site/index.html lines 1832-1921,
create site/auth-ui.js containing the shared auth UI logic, load it after
auth.js with a versioned script URL, and retain only page-specific Continue
Reading and streak logic inline; in site/lesson.html lines 3713-3803, remove the
duplicate block and load the shared script. Also update site/lesson.html lines
1734-1759 with the same dialog accessibility attributes and focusable
toggle-link behavior as the index modal, or have both pages render the modal
from the shared script; ensure the shared submit handler includes the requested
try/catch and result.errors fallback.
- Line 1935: Update the resume URL construction in the continueBtn handler to
URL-encode last.path before appending it as the path query parameter, preserving
the existing lesson.html destination and resume behavior.
- Around line 1874-1892: Update the authForm submit handler around the
AIFSAuth.signup/login await calls to catch rejected authentication promises and
display a generic visible error while keeping the modal open. Also guard the
result.ok false path so a missing or invalid result.errors array falls back to
the same generic message instead of calling join unconditionally.
- Around line 1365-1390: Update the auth modal markup and related handlers to
make it keyboard accessible: give authOverlay role="dialog" and
aria-modal="true", make authToggleLink a keyboard-focusable control that
activates on Enter, and move focus into the modal when it opens. Add an Escape
key handler alongside closeAuth that closes the modal only while authOverlay has
the open class, and update the auth-switch styling selector if the toggle
becomes a button.
In `@site/reading-progress.js`:
- Around line 143-160: Update persistNow to accumulate the minutes elapsed
across persistence calls before invoking AIFSStreak.updateStreak, rather than
passing only the current readSeconds interval. Preserve the existing persistence
behavior and reset timing while ensuring updateStreak receives the cumulative
minutes for the reading session.
- Line 146: Update the readSeconds accumulation near sessionStart so elapsed
time is added only while the document is visible, preventing background-tab idle
time from contributing to reading progress or streak thresholds. Use the
existing visibility state/API and preserve the current persistence behavior for
visible sessions.
In `@site/style.css`:
- Around line 2084-2098: The `.auth-field input` selector removes the default
outline with `outline: none` on line 2092, leaving only a subtle border-color
shift on focus that is hard for keyboard users to see. Either restore the
outline or add a more visible focus indicator such as box-shadow to the `:focus`
state. Additionally, add visible focus styles (outline, box-shadow, or similar)
to the `.profile-btn`, `.login-btn`, `.auth-submit`, `.auth-close`, and
`.profile-dropdown-item` selectors to ensure all interactive form and button
elements provide clear focus indicators for keyboard navigation.
---
Nitpick comments:
In `@site/lesson.html`:
- Around line 3813-3821: Export the existing calcScrollPct helper from the
reading-progress module, then update the scroll handler that manages
readingProgressFill to call that shared helper instead of duplicating the scroll
percentage formula; preserve the current width update and passive listener
behavior.
In `@site/reading-progress.js`:
- Around line 23-28: The reading-progress logic does not use SAVE_INTERVAL_MS or
assign saveTimer, so passive readers are not persisted reliably. Update the
initialization/lifecycle flow around saveTimer and the existing save routine to
start a setInterval using SAVE_INTERVAL_MS, persist progress on
visibilitychange, and clear the interval during teardown alongside the existing
cleanup.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 09a75142-1e90-4dfe-889d-03a87ce82985
📒 Files selected for processing (6)
site/auth.jssite/index.htmlsite/lesson.htmlsite/reading-progress.jssite/streak.jssite/style.css
Comment on lines
+38
to
+42
| function saveUsers(users) { | ||
| try { | ||
| localStorage.setItem(USERS_KEY, JSON.stringify(users)); | ||
| } catch (e) {} | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Surface storage write failures.
localStorage.setItem() can fail. These helpers suppress the error and then act as if the write succeeded. signup() and login() can return { ok: true } after state was not persisted. writeAll() can notify listeners about a streak that disappears after reload. (html.spec.whatwg.org)
- site/auth.js#L38-L42: Return a write result or throw a storage error to the caller.
- site/auth.js#L53-L62: Do not notify listeners when session persistence fails.
- site/streak.js#L44-L49: Propagate failed writes to update and reset callers, and notify only after persistence succeeds.
📍 Affects 2 files
site/auth.js#L38-L42(this comment)site/auth.js#L53-L62site/streak.js#L44-L49
🤖 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 `@site/auth.js` around lines 38 - 42, Update site/auth.js lines 38-42 in
saveUsers to return the write result or rethrow storage errors instead of
swallowing them; update site/auth.js lines 53-62 so signup() and login() report
failure and do not return success when session persistence fails; update
site/streak.js lines 44-49 so update and reset propagate failed writes and
notify listeners only after persistence succeeds.
Comment on lines
+81
to
+90
| async function hashPassword(salt, password) { | ||
| var enc = new TextEncoder(); | ||
| var data = enc.encode(salt + password); | ||
| var buf = await crypto.subtle.digest('SHA-256', data); | ||
| var arr = new Uint8Array(buf); | ||
| var hex = ''; | ||
| for (var i = 0; i < arr.length; i++) { | ||
| hex += ('0' + arr[i].toString(16)).slice(-2); | ||
| } | ||
| return hex; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- file size ---\n'
wc -l site/auth.js
printf '\n--- auth.js outline ---\n'
ast-grep outline site/auth.js --view expanded || true
printf '\n--- relevant auth.js sections ---\n'
sed -n '1,180p' site/auth.js | cat -n
printf '\n--- searches for auth storage/policy/usages ---\n'
rg -n "aifs:auth:users|currentUser|currentPassword|setPassword|setUser|userPassword|max|min|password|minLength|sha|SHA|digest|salt" . --glob '!node_modules' --glob '!dist' --glob '!build' | sed -n '1,240p'Repository: rohitg00/ai-engineering-from-scratch
Length of output: 265
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- deterministic digest behavior and offline-guessing feasibility ---'
node - <<'JS'
const crypto = require('crypto');
for (const length of [6, 8, 12]) {
const bytes = crypto.randomBytes(8);
const enc = new TextEncoder();
const arr = new Uint8Array(enc.encode(bytes.toString('hex') + 'a'.repeat(length)));
const start = process.hrtime.bigint();
const N = 100_000;
for (let i = 0; i < N; i++) {
crypto.createHash('sha256').update(arr).digest('hex');
}
const ns = Number(process.hrtime.bigint() - start);
const perNs = ns / N;
const perYearGuesses = 3600n * 24n * 365n * BigInt(Math.trunc(1e9 / perNs));
const perYearForAll6Chars = perYearGuesses / BigInt(95 ** length);
console.log({
passwordLength: length,
digestTimePerIterationNs: perNs.toFixed(2),
estimatedHashesPerSecond: Math.round(Math.pow(10, 9) / perNs),
estimatedGuessesPerYear: perYearGuesses.toString(),
perYearSearchTimeForAllPasswordSpace6Chars: perYearForAll6Chars < 1n ? '<1s' : Math.ceil(Number(perYearForAll6Chars)).toString(),
});
}
JSRepository: rohitg00/ai-engineering-from-scratch
Length of output: 836
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- file size ---'
wc -l site/auth.js
printf '%s\n' '--- auth.js outline ---'
ast-grep outline site/auth.js --view expanded || true
printf '%s\n' '--- relevant auth.js sections ---'
sed -n '1,180p' site/auth.js | cat -n
printf '%s\n' '--- searches for auth storage/policy/usages ---'
rg -n "aifs:auth:users|currentUser|currentPassword|setPassword|setUser|userPassword|max|min|password|minLength|sha|SHA|digest|salt" . --glob '!node_modules' --glob '!dist' --glob '!build' | sed -n '1,240p'Repository: rohitg00/ai-engineering-from-scratch
Length of output: 50392
Weak Cryptography (CWE-916): Use of Password Hash With Insufficient Computational Effort
Reachability: Internal
Reachability path
● Entry
site/reading-progress.js:30
userKey
│
▼
● Hop
site/streak.js:185
onChange
│
▼
● Sink
site/auth.js
Use a password KDF for locally stored credentials.
site/auth.js stores salt/hash in localStorage, accepts passwords with only pw.length >= 6, and derives the verifier with one SHA-256 digest. If aifs:auth:users is exposed, an attacker can test short password guesses offline. Move credential storage to a trusted verifier with a memory-hard KDF; for local-only use, do not persist reusable passwords. (NIST SP 800-63B)
🤖 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 `@site/auth.js` around lines 81 - 90, Replace the single SHA-256 derivation in
hashPassword with a trusted, memory-hard password KDF and store only its
verifier parameters and output in the local credential record; never persist
reusable passwords. Update the corresponding authentication verification and
user-creation flows in site/auth.js to use the KDF format consistently,
including a per-user salt and appropriate work factors, while preserving the
existing credential behavior.
Comment on lines
+112
to
+124
| var users = getUsers(); | ||
| var key = email.toLowerCase().trim(); | ||
| if (users[key]) return { ok: false, errors: ['An account with this email already exists.'] }; | ||
|
|
||
| var salt = randomHex(16); | ||
| var hash = await hashPassword(salt, password); | ||
| users[key] = { | ||
| name: name.trim(), | ||
| hash: hash, | ||
| salt: salt, | ||
| createdAt: Date.now() | ||
| }; | ||
| saveUsers(users); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Serialize each localStorage read-modify-write transaction.
signup() captures users before await hashPassword(). Two signups can write from separate stale objects, and the final saveUsers() deletes the other account. updateStreak() and resetStreak() use the same whole-map pattern. A delayed tab can overwrite a newer streak or restore reset data. Storage events occur after writes and do not serialize these operations. The platform specification advises applications to assume that no cross-context storage lock exists. (html.spec.whatwg.org)
- site/auth.js#L112-L124: Put the account read, duplicate check, mutation, and save in one cross-tab transaction.
- site/streak.js#L57-L60: Do not overwrite a whole stale streak map without serialization.
- site/streak.js#L130-L158: Include the streak read, calculation, and save in the same transaction.
- site/streak.js#L162-L165: Run reset through the same transaction as updates.
📍 Affects 2 files
site/auth.js#L112-L124(this comment)site/streak.js#L57-L60site/streak.js#L130-L158site/streak.js#L162-L165
🤖 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 `@site/auth.js` around lines 112 - 124, Serialize all localStorage
read-modify-write operations to prevent stale whole-map writes across tabs. In
site/auth.js lines 112-124, move the users read, duplicate check, mutation, and
save into one transaction that does not span the password-hashing await; in
site/streak.js lines 57-60, 130-158, and 162-165, route streak updates and
resets through the same transaction mechanism, including each operation’s read,
calculation or mutation, and save.
Comment on lines
+1365
to
+1390
| <div class="auth-overlay" id="authOverlay"> | ||
| <div class="auth-modal" style="position:relative"> | ||
| <button class="auth-close" id="authClose" type="button" aria-label="Close">×</button> | ||
| <div class="auth-modal-title" id="authTitle">Log In</div> | ||
| <div class="auth-error" id="authError"></div> | ||
| <form id="authForm" autocomplete="on"> | ||
| <div class="auth-field" id="authNameField" style="display:none"> | ||
| <label for="authName">Name</label> | ||
| <input type="text" id="authName" placeholder="Your name" autocomplete="name"> | ||
| </div> | ||
| <div class="auth-field"> | ||
| <label for="authEmail">Email</label> | ||
| <input type="email" id="authEmail" placeholder="you@example.com" autocomplete="email" required> | ||
| </div> | ||
| <div class="auth-field"> | ||
| <label for="authPassword">Password</label> | ||
| <input type="password" id="authPassword" placeholder="Min. 6 characters" autocomplete="current-password" required> | ||
| </div> | ||
| <button class="auth-submit" type="submit" id="authSubmit">Log In</button> | ||
| </form> | ||
| <div class="auth-switch" id="authSwitch"> | ||
| Don't have an account? <a id="authToggleLink">Sign up</a> | ||
| </div> | ||
| </div> | ||
| </div> | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Make the authentication modal keyboard accessible.
Three gaps block keyboard-only use:
authToggleLinkis an<a>with nohref. It is not focusable and does not respond to Enter.- The overlay has no
role="dialog"and noaria-modal="true", so screen readers do not announce it. - Escape does not close the modal, and focus is not moved into it after opening.
♿ Proposed markup change
- <div class="auth-overlay" id="authOverlay">
- <div class="auth-modal" style="position:relative">
+ <div class="auth-overlay" id="authOverlay">
+ <div class="auth-modal" style="position:relative" role="dialog" aria-modal="true" aria-labelledby="authTitle">
<button class="auth-close" id="authClose" type="button" aria-label="Close">×</button>
@@
<div class="auth-switch" id="authSwitch">
- Don't have an account? <a id="authToggleLink">Sign up</a>
+ Don't have an account? <button class="auth-toggle-link" id="authToggleLink" type="button">Sign up</button>
</div>Add an Escape handler next to closeAuth:
document.addEventListener('keydown', function (e) {
if (e.key === 'Escape' && authOverlay.classList.contains('open')) closeAuth();
});If you change the element to a button, update the .auth-switch a rule in site/style.css (line 2127) to also match .auth-toggle-link.
🤖 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 `@site/index.html` around lines 1365 - 1390, Update the auth modal markup and
related handlers to make it keyboard accessible: give authOverlay role="dialog"
and aria-modal="true", make authToggleLink a keyboard-focusable control that
activates on Enter, and move focus into the modal when it opens. Add an Escape
key handler alongside closeAuth that closes the modal only while authOverlay has
the open class, and update the auth-switch styling selector if the toggle
becomes a button.
Comment on lines
+1832
to
+1921
| <script> | ||
| (function () { | ||
| var authOverlay = document.getElementById('authOverlay'); | ||
| var authClose = document.getElementById('authClose'); | ||
| var authForm = document.getElementById('authForm'); | ||
| var authTitle = document.getElementById('authTitle'); | ||
| var authError = document.getElementById('authError'); | ||
| var authSubmit = document.getElementById('authSubmit'); | ||
| var authToggleLink = document.getElementById('authToggleLink'); | ||
| var authNameField = document.getElementById('authNameField'); | ||
| var loginBtn = document.getElementById('loginBtn'); | ||
| var profileBtn = document.getElementById('profileBtn'); | ||
| var profileAvatar = document.getElementById('profileAvatar'); | ||
| var profileName = document.getElementById('profileName'); | ||
| var profileDropdown = document.getElementById('profileDropdown'); | ||
| var logoutBtn = document.getElementById('logoutBtn'); | ||
|
|
||
| var isSignup = false; | ||
|
|
||
| function openAuth(mode) { | ||
| isSignup = mode === 'signup'; | ||
| authTitle.textContent = isSignup ? 'Sign Up' : 'Log In'; | ||
| authSubmit.textContent = isSignup ? 'Sign Up' : 'Log In'; | ||
| authNameField.style.display = isSignup ? '' : 'none'; | ||
| authError.className = 'auth-error'; | ||
| authError.textContent = ''; | ||
| authToggleLink.textContent = isSignup ? 'Log in' : 'Sign up'; | ||
| authForm.reset(); | ||
| authOverlay.classList.add('open'); | ||
| document.body.style.overflow = 'hidden'; | ||
| } | ||
|
|
||
| function closeAuth() { | ||
| authOverlay.classList.remove('open'); | ||
| document.body.style.overflow = ''; | ||
| } | ||
|
|
||
| loginBtn.addEventListener('click', function () { openAuth('login'); }); | ||
| authClose.addEventListener('click', closeAuth); | ||
| authOverlay.addEventListener('click', function (e) { if (e.target === authOverlay) closeAuth(); }); | ||
| authToggleLink.addEventListener('click', function () { openAuth(isSignup ? 'login' : 'signup'); }); | ||
|
|
||
| authForm.addEventListener('submit', async function (e) { | ||
| e.preventDefault(); | ||
| var name = document.getElementById('authName').value; | ||
| var email = document.getElementById('authEmail').value; | ||
| var password = document.getElementById('authPassword').value; | ||
| var result; | ||
| if (isSignup) { | ||
| result = await window.AIFSAuth.signup(name, email, password); | ||
| } else { | ||
| result = await window.AIFSAuth.login(email, password); | ||
| } | ||
| if (result.ok) { | ||
| closeAuth(); | ||
| updateAuthUI(); | ||
| } else { | ||
| authError.textContent = result.errors.join(' '); | ||
| authError.className = 'auth-error visible'; | ||
| } | ||
| }); | ||
|
|
||
| profileBtn.addEventListener('click', function (e) { | ||
| e.stopPropagation(); | ||
| profileDropdown.classList.toggle('open'); | ||
| }); | ||
| document.addEventListener('click', function () { profileDropdown.classList.remove('open'); }); | ||
|
|
||
| logoutBtn.addEventListener('click', function () { | ||
| window.AIFSAuth.logout(); | ||
| profileDropdown.classList.remove('open'); | ||
| updateAuthUI(); | ||
| }); | ||
|
|
||
| function updateAuthUI() { | ||
| var user = window.AIFSAuth.currentUser(); | ||
| if (user) { | ||
| loginBtn.style.display = 'none'; | ||
| profileBtn.style.display = ''; | ||
| profileAvatar.textContent = user.name.charAt(0).toUpperCase(); | ||
| profileName.textContent = user.name.split(' ')[0]; | ||
| } else { | ||
| loginBtn.style.display = ''; | ||
| profileBtn.style.display = 'none'; | ||
| profileDropdown.classList.remove('open'); | ||
| } | ||
| } | ||
|
|
||
| window.AIFSAuth.onChange(updateAuthUI); | ||
| updateAuthUI(); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Extract the duplicated authentication UI into a shared script. The same authentication controller, roughly 90 lines, is copied verbatim into both pages, and the modal markup is copied as well. Every fix must then be applied twice, including the error handling raised on site/index.html lines 1874-1892 and the accessibility changes raised on lines 1365-1390. The two copies will drift.
site/index.html#L1832-L1921: move this block into a newsite/auth-ui.js, load it with<script src="auth-ui.js?v=...">afterauth.js, and keep only the page-specific Continue Reading and streak logic inline.site/lesson.html#L3713-L3803: delete this block and load the samesite/auth-ui.js. Until the extraction lands, apply thetry/catchandresult.errorsfallback from thesite/index.htmlcomment to the submit handler at lines 3756-3774 as well.site/lesson.html#L1734-L1759: apply the samerole="dialog",aria-modal, and focusable toggle-link changes assite/index.htmllines 1365-1390, or render the modal markup from the shared script so one template serves both pages.
📍 Affects 2 files
site/index.html#L1832-L1921(this comment)site/lesson.html#L3713-L3803site/lesson.html#L1734-L1759
🤖 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 `@site/index.html` around lines 1832 - 1921, The authentication controller and
modal are duplicated across both pages and must be centralized. In
site/index.html lines 1832-1921, create site/auth-ui.js containing the shared
auth UI logic, load it after auth.js with a versioned script URL, and retain
only page-specific Continue Reading and streak logic inline; in site/lesson.html
lines 3713-3803, remove the duplicate block and load the shared script. Also
update site/lesson.html lines 1734-1759 with the same dialog accessibility
attributes and focusable toggle-link behavior as the index modal, or have both
pages render the modal from the shared script; ensure the shared submit handler
includes the requested try/catch and result.errors fallback.
Comment on lines
+1874
to
+1892
| authForm.addEventListener('submit', async function (e) { | ||
| e.preventDefault(); | ||
| var name = document.getElementById('authName').value; | ||
| var email = document.getElementById('authEmail').value; | ||
| var password = document.getElementById('authPassword').value; | ||
| var result; | ||
| if (isSignup) { | ||
| result = await window.AIFSAuth.signup(name, email, password); | ||
| } else { | ||
| result = await window.AIFSAuth.login(email, password); | ||
| } | ||
| if (result.ok) { | ||
| closeAuth(); | ||
| updateAuthUI(); | ||
| } else { | ||
| authError.textContent = result.errors.join(' '); | ||
| authError.className = 'auth-error visible'; | ||
| } | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Handle a rejected authentication promise and a missing errors array.
Two failure paths are unguarded here. First, signup and login use SHA-256 through crypto.subtle, which is undefined on an insecure origin such as plain HTTP or a file:// page. The await then rejects, the handler stops, and the modal stays open with no message. Second, result.errors.join(' ') throws if a rejection path returns ok: false without an errors array.
Wrap the call in try/catch and fall back to a generic message.
🐛 Proposed fix
var result;
- if (isSignup) {
- result = await window.AIFSAuth.signup(name, email, password);
- } else {
- result = await window.AIFSAuth.login(email, password);
- }
- if (result.ok) {
+ try {
+ if (isSignup) {
+ result = await window.AIFSAuth.signup(name, email, password);
+ } else {
+ result = await window.AIFSAuth.login(email, password);
+ }
+ } catch (err) {
+ result = { ok: false, errors: ['Sign-in is unavailable in this browser context.'] };
+ }
+ if (result && result.ok) {
closeAuth();
updateAuthUI();
} else {
- authError.textContent = result.errors.join(' ');
+ authError.textContent = ((result && result.errors) || ['Something went wrong.']).join(' ');
authError.className = 'auth-error visible';
}📝 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
| authForm.addEventListener('submit', async function (e) { | |
| e.preventDefault(); | |
| var name = document.getElementById('authName').value; | |
| var email = document.getElementById('authEmail').value; | |
| var password = document.getElementById('authPassword').value; | |
| var result; | |
| if (isSignup) { | |
| result = await window.AIFSAuth.signup(name, email, password); | |
| } else { | |
| result = await window.AIFSAuth.login(email, password); | |
| } | |
| if (result.ok) { | |
| closeAuth(); | |
| updateAuthUI(); | |
| } else { | |
| authError.textContent = result.errors.join(' '); | |
| authError.className = 'auth-error visible'; | |
| } | |
| }); | |
| authForm.addEventListener('submit', async function (e) { | |
| e.preventDefault(); | |
| var name = document.getElementById('authName').value; | |
| var email = document.getElementById('authEmail').value; | |
| var password = document.getElementById('authPassword').value; | |
| var result; | |
| try { | |
| if (isSignup) { | |
| result = await window.AIFSAuth.signup(name, email, password); | |
| } else { | |
| result = await window.AIFSAuth.login(email, password); | |
| } | |
| } catch (err) { | |
| result = { ok: false, errors: ['Sign-in is unavailable in this browser context.'] }; | |
| } | |
| if (result && result.ok) { | |
| closeAuth(); | |
| updateAuthUI(); | |
| } else { | |
| authError.textContent = ((result && result.errors) || ['Something went wrong.']).join(' '); | |
| authError.className = 'auth-error visible'; | |
| } | |
| }); |
🤖 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 `@site/index.html` around lines 1874 - 1892, Update the authForm submit handler
around the AIFSAuth.signup/login await calls to catch rejected authentication
promises and display a generic visible error while keeping the modal open. Also
guard the result.ok false path so a missing or invalid result.errors array falls
back to the same generic message instead of calling join unconditionally.
| var pct = Math.round(last.scrollPct || 0); | ||
| document.getElementById('continuePct').textContent = pct + '% read'; | ||
| document.getElementById('continueMeta').textContent = 'Last opened ' + new Date(last.lastOpened).toLocaleDateString(); | ||
| document.getElementById('continueBtn').href = 'lesson.html?path=' + last.path; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Encode the lesson path in the resume URL.
last.path is inserted into the query string without encoding. A path that contains #, &, or + produces a URL that lesson.html parses incorrectly, and the resume button then loads the wrong lesson.
🐛 Proposed fix
- document.getElementById('continueBtn').href = 'lesson.html?path=' + last.path;
+ document.getElementById('continueBtn').href = 'lesson.html?path=' + encodeURIComponent(last.path);📝 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('continueBtn').href = 'lesson.html?path=' + last.path; | |
| document.getElementById('continueBtn').href = 'lesson.html?path=' + encodeURIComponent(last.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 `@site/index.html` at line 1935, Update the resume URL construction in the
continueBtn handler to URL-encode last.path before appending it as the path
query parameter, preserving the existing lesson.html destination and resume
behavior.
Comment on lines
+143
to
+160
| function persistNow() { | ||
| if (!currentPath) return; | ||
| var scrollPct = calcScrollPct(); | ||
| var readSeconds = (Date.now() - sessionStart) / 1000; | ||
| var completed = scrollPct > 90; | ||
| saveProgress(currentPath, { | ||
| scrollPct: scrollPct, | ||
| readSeconds: readSeconds, | ||
| lastOpened: Date.now(), | ||
| completed: completed | ||
| }); | ||
| sessionStart = Date.now(); | ||
|
|
||
| if (window.AIFSStreak) { | ||
| var minutes = readSeconds / 60; | ||
| window.AIFSStreak.updateStreak(minutes); | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the streak update contract and threshold handling.
fd 'streak.js' --exec cat -nRepository: rohitg00/ai-engineering-from-scratch
Length of output: 7267
Accumulate the passed minutes before calling updateStreak.
AIFSStreak.updateStreak accepts one call-level hoursRead value and compares it against the daily minimum. persistNow sends only the seconds since the last persist, so the call value stays below the threshold and no reading day is registered. Sum the cumulative minutes passed into persistNow before calling updateStreak.
🤖 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 `@site/reading-progress.js` around lines 143 - 160, Update persistNow to
accumulate the minutes elapsed across persistence calls before invoking
AIFSStreak.updateStreak, rather than passing only the current readSeconds
interval. Preserve the existing persistence behavior and reset timing while
ensuring updateStreak receives the cumulative minutes for the reading session.
| function persistNow() { | ||
| if (!currentPath) return; | ||
| var scrollPct = calcScrollPct(); | ||
| var readSeconds = (Date.now() - sessionStart) / 1000; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
readSeconds measures wall-clock time, not reading time.
sessionStart advances only on persist. A tab that stays open in the background keeps accumulating time, and the next persist adds the whole idle span. This inflates readSeconds and any streak threshold derived from it.
Gate the accumulation on document visibility.
♻️ Proposed change
function persistNow() {
if (!currentPath) return;
var scrollPct = calcScrollPct();
- var readSeconds = (Date.now() - sessionStart) / 1000;
+ var elapsed = (Date.now() - sessionStart) / 1000;
+ // Ignore spans that exceed a plausible foreground window.
+ var readSeconds = document.hidden ? 0 : Math.min(elapsed, 300);🤖 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 `@site/reading-progress.js` at line 146, Update the readSeconds accumulation
near sessionStart so elapsed time is added only while the document is visible,
preventing background-tab idle time from contributing to reading progress or
streak thresholds. Use the existing visibility state/API and preserve the
current persistence behavior for visible sessions.
Comment on lines
+2084
to
+2098
| .auth-field input { | ||
| width: 100%; | ||
| padding: 10px 12px; | ||
| border: 1px solid var(--rule-soft); | ||
| background: transparent; | ||
| color: var(--ink); | ||
| font-family: var(--font-mono); | ||
| font-size: 0.88rem; | ||
| outline: none; | ||
| transition: border-color 0.15s; | ||
| } | ||
|
|
||
| .auth-field input:focus { | ||
| border-color: var(--blueprint); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Keep a visible focus indicator on the authentication inputs.
Line 2092 sets outline: none. The only remaining focus cue is the border-color change at line 2097, which is a 1px shift and is hard to see. Keyboard users lose the focus position. The new buttons (.profile-btn, .login-btn, .auth-submit, .auth-close, .profile-dropdown-item) also define no focus style.
♿ Proposed change
.auth-field input:focus {
border-color: var(--blueprint);
}
+
+.auth-field input:focus-visible,
+.profile-btn:focus-visible,
+.login-btn:focus-visible,
+.auth-submit:focus-visible,
+.auth-close:focus-visible,
+.profile-dropdown-item:focus-visible {
+ outline: 2px solid var(--blueprint);
+ outline-offset: 2px;
+}📝 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
| .auth-field input { | |
| width: 100%; | |
| padding: 10px 12px; | |
| border: 1px solid var(--rule-soft); | |
| background: transparent; | |
| color: var(--ink); | |
| font-family: var(--font-mono); | |
| font-size: 0.88rem; | |
| outline: none; | |
| transition: border-color 0.15s; | |
| } | |
| .auth-field input:focus { | |
| border-color: var(--blueprint); | |
| } | |
| .auth-field input { | |
| width: 100%; | |
| padding: 10px 12px; | |
| border: 1px solid var(--rule-soft); | |
| background: transparent; | |
| color: var(--ink); | |
| font-family: var(--font-mono); | |
| font-size: 0.88rem; | |
| outline: none; | |
| transition: border-color 0.15s; | |
| } | |
| .auth-field input:focus { | |
| border-color: var(--blueprint); | |
| } | |
| .auth-field input:focus-visible, | |
| .profile-btn:focus-visible, | |
| .login-btn:focus-visible, | |
| .auth-submit:focus-visible, | |
| .auth-close:focus-visible, | |
| .profile-dropdown-item:focus-visible { | |
| outline: 2px solid var(--blueprint); | |
| outline-offset: 2px; | |
| } |
🤖 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 `@site/style.css` around lines 2084 - 2098, The `.auth-field input` selector
removes the default outline with `outline: none` on line 2092, leaving only a
subtle border-color shift on focus that is hard for keyboard users to see.
Either restore the outline or add a more visible focus indicator such as
box-shadow to the `:focus` state. Additionally, add visible focus styles
(outline, box-shadow, or similar) to the `.profile-btn`, `.login-btn`,
`.auth-submit`, `.auth-close`, and `.profile-dropdown-item` selectors to ensure
all interactive form and button elements provide clear focus indicators for
keyboard navigation.
Author
|
feat(site): add auth, reading progress, and streak tracking Summary Adds three lightweight, localStorage-based features that enhance the learning experience on the static site. No backend, no new dependencies, and no changes to existing behavior — all functionality is additive and follows the project's existing IIFE module architecture. What's included 1. User accounts (
2. Reading progress (
3. Reading streak (
Design & performance
Files added
Files modified
Verification
|
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
2 participants
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 lightweight client-side features that enhance the learning experience:
What this PR does
Kind of change
Checklist
LESSON_TEMPLATE.mdstructure[Name](phases/...)), not bare textdocs/en.mdclaimsPhase / lesson
Notes for reviewer